From 5b12a30293893c6d2bf0af3bca4a080d0edc95df Mon Sep 17 00:00:00 2001 From: Kian Jones <11655409+kianjones9@users.noreply.github.com> Date: Wed, 18 Feb 2026 15:15:51 -0800 Subject: [PATCH] fix: clobbering user settings (#1003) Co-authored-by: cpacker Co-authored-by: Letta --- src/settings-manager.ts | 47 ++++++++++-- src/tests/settings-manager.test.ts | 114 +++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 6 deletions(-) diff --git a/src/settings-manager.ts b/src/settings-manager.ts index 5c38898..58542dd 100644 --- a/src/settings-manager.ts +++ b/src/settings-manager.ts @@ -184,6 +184,10 @@ class SettingsManager { private initialized = false; private pendingWrites = new Set>(); private secretsAvailable: boolean | null = null; + // Keys loaded from the file or explicitly set via updateSettings(). + // persistSettings() only writes these keys, so manual file edits for + // keys we never touched are preserved instead of being clobbered by defaults. + private managedKeys = new Set(); /** * Initialize the settings manager (loads from disk) @@ -199,6 +203,9 @@ class SettingsManager { if (!exists(settingsPath)) { // Create default settings file this.settings = { ...DEFAULT_SETTINGS }; + for (const key of Object.keys(DEFAULT_SETTINGS)) { + this.managedKeys.add(key); + } await this.persistSettings(); } else { // Read and parse settings @@ -206,6 +213,9 @@ class SettingsManager { const loadedSettings = JSON.parse(content) as Settings; // Merge with defaults in case new fields were added this.settings = { ...DEFAULT_SETTINGS, ...loadedSettings }; + for (const key of Object.keys(loadedSettings)) { + this.managedKeys.add(key); + } } this.initialized = true; @@ -223,6 +233,9 @@ class SettingsManager { } catch (error) { console.error("Error loading settings, using defaults:", error); this.settings = { ...DEFAULT_SETTINGS }; + for (const key of Object.keys(DEFAULT_SETTINGS)) { + this.managedKeys.add(key); + } this.initialized = true; // Still check secrets support and try to migrate in case of partial failure @@ -292,6 +305,8 @@ class SettingsManager { } this.settings = updatedSettings; + this.managedKeys.add("refreshToken"); + this.managedKeys.add("env"); await this.persistSettings(); debugWarn("settings", "Successfully migrated tokens to secrets"); @@ -357,6 +372,7 @@ class SettingsManager { if (agents.length > 0) { this.settings = { ...this.settings, agents }; + this.managedKeys.add("agents"); // Persist the migration (async, fire-and-forget) this.persistSettings().catch((error) => { console.warn("Failed to persist agents array migration:", error); @@ -461,6 +477,13 @@ class SettingsManager { ...(updatedEnv && { env: { ...this.settings.env, ...updatedEnv } }), }; + for (const key of Object.keys(otherUpdates)) { + this.managedKeys.add(key); + } + if (updatedEnv) { + this.managedKeys.add("env"); + } + // Handle secure tokens in keychain const secureTokens: SecureTokens = {}; if (apiKey) { @@ -518,6 +541,7 @@ class SettingsManager { if (secureTokens.refreshToken) { fallbackSettings.refreshToken = secureTokens.refreshToken; + this.managedKeys.add("refreshToken"); } if (secureTokens.apiKey) { @@ -525,6 +549,7 @@ class SettingsManager { ...fallbackSettings.env, LETTA_API_KEY: secureTokens.apiKey, }; + this.managedKeys.add("env"); } this.settings = fallbackSettings; @@ -645,12 +670,20 @@ class SettingsManager { } } - // Merge: existing fields + our managed settings - // Our settings take precedence for fields we manage - const merged = { - ...existingSettings, - ...this.settings, - }; + // Only write keys we loaded from the file or explicitly set via updateSettings(). + // This preserves manual file edits for keys we never touched (e.g. defaults). + const merged: Record = { ...existingSettings }; + const settingsRecord = this.settings as unknown as Record< + string, + unknown + >; + for (const key of this.managedKeys) { + if (key in settingsRecord) { + merged[key] = settingsRecord[key]; + } else { + delete merged[key]; + } + } await writeFile(settingsPath, JSON.stringify(merged, null, 2)); } catch (error) { @@ -1446,6 +1479,7 @@ class SettingsManager { const settings = this.getSettings(); const { oauthState: _, ...rest } = settings; this.settings = { ...DEFAULT_SETTINGS, ...rest }; + this.managedKeys.add("oauthState"); this.persistSettings().catch((error) => { console.error( "Failed to persist settings after clearing OAuth state:", @@ -1586,6 +1620,7 @@ class SettingsManager { this.initialized = false; this.pendingWrites.clear(); this.secretsAvailable = null; + this.managedKeys.clear(); } } diff --git a/src/tests/settings-manager.test.ts b/src/tests/settings-manager.test.ts index 115c85a..37947a9 100644 --- a/src/tests/settings-manager.test.ts +++ b/src/tests/settings-manager.test.ts @@ -522,6 +522,39 @@ describe("Settings Manager - Reset", () => { const settings = settingsManager.getSettings(); expect(settings.tokenStreaming).toBe(true); }); + + test("Reset clears managedKeys so stale keys don't leak into next session", async () => { + const { writeFile, mkdir } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + await mkdir(settingsDir, { recursive: true }); + + // First session: write a setting that will be tracked in managedKeys + await settingsManager.initialize(); + settingsManager.updateSettings({ lastAgent: "agent-first-session" }); + await new Promise((resolve) => setTimeout(resolve, 100)); + await settingsManager.reset(); + + // Second session: write a completely fresh file with a different key + await writeFile( + join(settingsDir, "settings.json"), + JSON.stringify({ tokenStreaming: true }), + ); + await settingsManager.initialize(); + + // After re-init, managedKeys should only contain keys from the new file. + // Persisting should write tokenStreaming but NOT ghost-write lastAgent from + // the previous session's managedKeys. + settingsManager.updateSettings({ enableSleeptime: false }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + await settingsManager.reset(); + await settingsManager.initialize(); + + const settings = settingsManager.getSettings(); + expect(settings.tokenStreaming).toBe(true); + // lastAgent was only set in the first session — should not reappear + expect(settings.lastAgent).toBeNull(); + }); }); // ============================================================================ @@ -1011,3 +1044,84 @@ describe("Settings Manager - Toolset Preferences", () => { ); }); }); + +// ============================================================================ +// Managed Keys / Settings Preservation Tests +// ============================================================================ + +describe("Settings Manager - Managed Keys Preservation", () => { + test("Unknown top-level keys in the file are preserved across writes", async () => { + const { writeFile, mkdir } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + await mkdir(settingsDir, { recursive: true }); + + // Simulate a user manually adding a key that Letta Code doesn't know about + await writeFile( + join(settingsDir, "settings.json"), + JSON.stringify({ + tokenStreaming: true, + myCustomFlag: "keep-me", + }), + ); + + await settingsManager.initialize(); + + // Update an unrelated setting — should not clobber myCustomFlag + settingsManager.updateSettings({ lastAgent: "agent-abc" }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Read the raw file to confirm myCustomFlag survived + const { readFile } = await import("../utils/fs.js"); + const raw = JSON.parse( + await readFile(join(settingsDir, "settings.json")), + ) as Record; + + expect(raw.myCustomFlag).toBe("keep-me"); + expect(raw.tokenStreaming).toBe(true); + expect(raw.lastAgent).toBe("agent-abc"); + }); + + test("No-keychain fallback persists refreshToken and LETTA_API_KEY to file", async () => { + // On machines with a keychain, tokens go to the keychain, not the file. + // On machines without a keychain, tokens must fall back to the file. + // Both paths are exercised here depending on the environment. + const secretsAvail = await isKeychainAvailable(); + if (secretsAvail) { + // On machines with a keychain, tokens go to the keychain, not the file. + // Test the fallback indirectly: if secrets are available the tokens + // should NOT be in the file (they're in the keychain). + await settingsManager.initialize(); + settingsManager.updateSettings({ refreshToken: "rt-keychain-test" }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const { readFile } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + const raw = JSON.parse( + await readFile(join(settingsDir, "settings.json")), + ) as Record; + + // With keychain available, refreshToken goes to keychain not file + expect(raw.refreshToken).toBeUndefined(); + } else { + // No keychain: tokens fall back to the settings file and must be persisted + await settingsManager.initialize(); + settingsManager.updateSettings({ + refreshToken: "rt-fallback-test", + env: { LETTA_API_KEY: "sk-fallback-test" }, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const { readFile } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + const raw = JSON.parse( + await readFile(join(settingsDir, "settings.json")), + ) as Record; + + expect(raw.refreshToken).toBe("rt-fallback-test"); + // LETTA_API_KEY also falls back to the file when keychain is unavailable + expect((raw.env as Record)?.LETTA_API_KEY).toBe( + "sk-fallback-test", + ); + } + }); +});