From 573ac5721c0f52cba69bf137f233631d63664eae Mon Sep 17 00:00:00 2001 From: Devansh Jain <31609257+devanshrj@users.noreply.github.com> Date: Wed, 25 Feb 2026 19:45:58 -0800 Subject: [PATCH] fix: prevent stale settings writes from clobbering global pins (#1150) --- src/settings-manager.ts | 40 +++++++++--- src/tests/settings-manager.test.ts | 97 ++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 10 deletions(-) diff --git a/src/settings-manager.ts b/src/settings-manager.ts index d0e946f..c91dcf9 100644 --- a/src/settings-manager.ts +++ b/src/settings-manager.ts @@ -190,6 +190,20 @@ class SettingsManager { // 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(); + // Keys explicitly changed by this process. Only these keys are written back, + // preventing stale in-memory values from clobbering external updates. + private dirtyKeys = new Set(); + + // Mark keys as managed AND dirty (i.e. this process owns the value and it + // should be written back on persist). The only call-site that should add to + // managedKeys *without* calling this helper is the disk-load path in + // initialize(), where we want to track the key but preserve external edits. + private markDirty(...keys: string[]): void { + for (const key of keys) { + this.managedKeys.add(key); + this.dirtyKeys.add(key); + } + } /** * Initialize the settings manager (loads from disk) @@ -206,7 +220,7 @@ class SettingsManager { // Create default settings file this.settings = { ...DEFAULT_SETTINGS }; for (const key of Object.keys(DEFAULT_SETTINGS)) { - this.managedKeys.add(key); + this.markDirty(key); } await this.persistSettings(); } else { @@ -236,7 +250,7 @@ class SettingsManager { 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.markDirty(key); } this.initialized = true; @@ -307,8 +321,7 @@ class SettingsManager { } this.settings = updatedSettings; - this.managedKeys.add("refreshToken"); - this.managedKeys.add("env"); + this.markDirty("refreshToken", "env"); await this.persistSettings(); debugWarn("settings", "Successfully migrated tokens to secrets"); @@ -374,7 +387,7 @@ class SettingsManager { if (agents.length > 0) { this.settings = { ...this.settings, agents }; - this.managedKeys.add("agents"); + this.markDirty("agents"); // Persist the migration (async, fire-and-forget) this.persistSettings().catch((error) => { console.warn("Failed to persist agents array migration:", error); @@ -480,10 +493,10 @@ class SettingsManager { }; for (const key of Object.keys(otherUpdates)) { - this.managedKeys.add(key); + this.markDirty(key); } if (updatedEnv) { - this.managedKeys.add("env"); + this.markDirty("env"); } // Handle secure tokens in keychain @@ -543,7 +556,7 @@ class SettingsManager { if (secureTokens.refreshToken) { fallbackSettings.refreshToken = secureTokens.refreshToken; - this.managedKeys.add("refreshToken"); + this.markDirty("refreshToken"); } if (secureTokens.apiKey) { @@ -551,7 +564,7 @@ class SettingsManager { ...fallbackSettings.env, LETTA_API_KEY: secureTokens.apiKey, }; - this.managedKeys.add("env"); + this.markDirty("env"); } this.settings = fallbackSettings; @@ -704,6 +717,11 @@ class SettingsManager { unknown >; for (const key of this.managedKeys) { + // Preserve external updates (including deletions) for keys this + // process never touched. + if (!this.dirtyKeys.has(key)) { + continue; + } if (key in settingsRecord) { merged[key] = settingsRecord[key]; } else { @@ -1567,7 +1585,7 @@ class SettingsManager { const settings = this.getSettings(); const { oauthState: _, ...rest } = settings; this.settings = { ...DEFAULT_SETTINGS, ...rest }; - this.managedKeys.add("oauthState"); + this.markDirty("oauthState"); this.persistSettings().catch((error) => { console.error( "Failed to persist settings after clearing OAuth state:", @@ -1682,6 +1700,7 @@ class SettingsManager { } this.settings = updatedSettings; + this.markDirty("refreshToken", "tokenExpiresAt", "deviceId", "env"); await this.persistSettings(); } @@ -1709,6 +1728,7 @@ class SettingsManager { this.pendingWrites.clear(); this.secretsAvailable = null; this.managedKeys.clear(); + this.dirtyKeys.clear(); } } diff --git a/src/tests/settings-manager.test.ts b/src/tests/settings-manager.test.ts index 7d2ba40..5ea4846 100644 --- a/src/tests/settings-manager.test.ts +++ b/src/tests/settings-manager.test.ts @@ -1128,6 +1128,103 @@ describe("Settings Manager - Managed Keys Preservation", () => { expect(raw.lastAgent).toBe("agent-abc"); }); + test("External updates to managed keys are preserved when this process didn't change them", async () => { + const { writeFile, readFile, mkdir } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + const settingsPath = join(settingsDir, "settings.json"); + await mkdir(settingsDir, { recursive: true }); + + await writeFile( + settingsPath, + JSON.stringify({ + tokenStreaming: true, + pinnedAgents: ["agent-a"], + pinnedAgentsByServer: { + "api.letta.com": ["agent-a"], + }, + }), + ); + + await settingsManager.initialize(); + + // Simulate another process appending a new global pin while this process + // is still running with stale in-memory settings. + const externallyUpdated = JSON.parse( + await readFile(settingsPath), + ) as Record; + + const pinnedByServer = (externallyUpdated.pinnedAgentsByServer as Record< + string, + string[] + >) || { "api.letta.com": [] }; + pinnedByServer["api.letta.com"] = [ + ...(pinnedByServer["api.letta.com"] || []), + "agent-b", + ]; + externallyUpdated.pinnedAgentsByServer = pinnedByServer; + + const pinned = (externallyUpdated.pinnedAgents as string[]) || []; + externallyUpdated.pinnedAgents = [...pinned, "agent-b"]; + + await writeFile(settingsPath, JSON.stringify(externallyUpdated)); + + // Trigger an unrelated write from this process. + settingsManager.updateSettings({ lastAgent: "agent-current" }); + await settingsManager.flush(); + + const raw = JSON.parse(await readFile(settingsPath)) as Record< + string, + unknown + >; + expect(raw.lastAgent).toBe("agent-current"); + expect((raw.pinnedAgents as string[]) || []).toContain("agent-b"); + expect( + (raw.pinnedAgentsByServer as Record)?.[ + "api.letta.com" + ] || [], + ).toContain("agent-b"); + }); + + test("External deletion of managed keys is preserved when this process didn't change them", async () => { + const { writeFile, readFile, mkdir } = await import("../utils/fs.js"); + const settingsDir = join(testHomeDir, ".letta"); + const settingsPath = join(settingsDir, "settings.json"); + await mkdir(settingsDir, { recursive: true }); + + await writeFile( + settingsPath, + JSON.stringify({ + tokenStreaming: true, + pinnedAgents: ["agent-a"], + pinnedAgentsByServer: { + "api.letta.com": ["agent-a"], + }, + }), + ); + + await settingsManager.initialize(); + + // Simulate another process removing managed pin keys. + const externallyUpdated = JSON.parse( + await readFile(settingsPath), + ) as Record; + delete externallyUpdated.pinnedAgents; + delete externallyUpdated.pinnedAgentsByServer; + await writeFile(settingsPath, JSON.stringify(externallyUpdated)); + + // Trigger an unrelated write from this process. + settingsManager.updateSettings({ lastAgent: "agent-current" }); + await settingsManager.flush(); + + const raw = JSON.parse(await readFile(settingsPath)) as Record< + string, + unknown + >; + expect(raw.lastAgent).toBe("agent-current"); + expect("pinnedAgents" in raw).toBe(false); + expect("pinnedAgentsByServer" in raw).toBe(false); + }); + 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.