fix: clobbering user settings (#1003)
Co-authored-by: cpacker <packercharles@gmail.com> Co-authored-by: Letta <noreply@letta.com>
This commit is contained in:
@@ -184,6 +184,10 @@ class SettingsManager {
|
||||
private initialized = false;
|
||||
private pendingWrites = new Set<Promise<void>>();
|
||||
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<string>();
|
||||
|
||||
/**
|
||||
* 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<string, unknown> = { ...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();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
|
||||
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<string, unknown>;
|
||||
|
||||
// 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<string, unknown>;
|
||||
|
||||
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<string, unknown>)?.LETTA_API_KEY).toBe(
|
||||
"sk-fallback-test",
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user