From f8107c84f4155ab08d351a3b264ccf769be3ccee Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Mon, 3 Nov 2025 19:20:55 -0800 Subject: [PATCH] fix: prevent tests from overwriting real settings.json (#49) Co-authored-by: Letta --- src/settings-manager.ts | 64 ++++++++++++++++++++++-------- src/tests/settings-manager.test.ts | 23 ++++++----- 2 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/settings-manager.ts b/src/settings-manager.ts index 6ed8db4..a4e8379 100644 --- a/src/settings-manager.ts +++ b/src/settings-manager.ts @@ -49,6 +49,7 @@ class SettingsManager { private projectSettings: Map = new Map(); private localProjectSettings: Map = new Map(); private initialized = false; + private pendingWrites = new Set>(); /** * Initialize the settings manager (loads from disk) @@ -112,10 +113,15 @@ class SettingsManager { this.settings = { ...this.settings, ...updates }; - // Persist asynchronously (fire and forget, errors logged) - this.persistSettings().catch((error) => { - console.error("Failed to persist settings:", error); - }); + // Persist asynchronously (track promise for testing) + const writePromise = this.persistSettings() + .catch((error) => { + console.error("Failed to persist settings:", error); + }) + .finally(() => { + this.pendingWrites.delete(writePromise); + }); + this.pendingWrites.add(writePromise); } /** @@ -189,10 +195,15 @@ class SettingsManager { const updated = { ...current, ...updates }; this.projectSettings.set(workingDirectory, updated); - // Persist asynchronously - this.persistProjectSettings(workingDirectory).catch((error) => { - console.error("Failed to persist project settings:", error); - }); + // Persist asynchronously (track promise for testing) + const writePromise = this.persistProjectSettings(workingDirectory) + .catch((error) => { + console.error("Failed to persist project settings:", error); + }) + .finally(() => { + this.pendingWrites.delete(writePromise); + }); + this.pendingWrites.add(writePromise); } /** @@ -202,7 +213,8 @@ class SettingsManager { if (!this.settings) return; const settingsPath = this.getSettingsPath(); - const dirPath = join(homedir(), ".letta"); + const home = process.env.HOME || homedir(); + const dirPath = join(home, ".letta"); try { if (!exists(dirPath)) { @@ -254,7 +266,9 @@ class SettingsManager { } private getSettingsPath(): string { - return join(homedir(), ".letta", "settings.json"); + // Respect process.env.HOME for testing (homedir() ignores it) + const home = process.env.HOME || homedir(); + return join(home, ".letta", "settings.json"); } private getProjectSettingsPath(workingDirectory: string): string { @@ -334,10 +348,15 @@ class SettingsManager { const updated = { ...current, ...updates }; this.localProjectSettings.set(workingDirectory, updated); - // Persist asynchronously - this.persistLocalProjectSettings(workingDirectory).catch((error) => { - console.error("Failed to persist local project settings:", error); - }); + // Persist asynchronously (track promise for testing) + const writePromise = this.persistLocalProjectSettings(workingDirectory) + .catch((error) => { + console.error("Failed to persist local project settings:", error); + }) + .finally(() => { + this.pendingWrites.delete(writePromise); + }); + this.pendingWrites.add(writePromise); } /** @@ -366,13 +385,26 @@ class SettingsManager { } /** - * Reset the manager (mainly for testing) + * Wait for all pending writes to complete. + * Useful in tests to ensure writes finish before cleanup. */ - reset(): void { + async flush(): Promise { + await Promise.all(Array.from(this.pendingWrites)); + } + + /** + * Reset the manager (mainly for testing). + * Waits for pending writes to complete before resetting. + */ + async reset(): Promise { + // Wait for pending writes BEFORE clearing state + await this.flush(); + this.settings = null; this.projectSettings.clear(); this.localProjectSettings.clear(); this.initialized = false; + this.pendingWrites.clear(); } } diff --git a/src/tests/settings-manager.test.ts b/src/tests/settings-manager.test.ts index 5c52a52..a5742cb 100644 --- a/src/tests/settings-manager.test.ts +++ b/src/tests/settings-manager.test.ts @@ -11,7 +11,7 @@ let testProjectDir: string; beforeEach(async () => { // Reset settings manager FIRST before changing HOME - settingsManager.reset(); + await settingsManager.reset(); // Create temporary directories for testing testHomeDir = await mkdtemp(join(tmpdir(), "letta-test-home-")); @@ -22,15 +22,16 @@ beforeEach(async () => { }); afterEach(async () => { + // Wait for all pending writes to complete BEFORE restoring HOME + // This prevents test writes from leaking into real settings after HOME is restored + await settingsManager.reset(); + // Clean up test directories await rm(testHomeDir, { recursive: true, force: true }); await rm(testProjectDir, { recursive: true, force: true }); - // Restore original HOME + // Restore original HOME AFTER reset completes process.env.HOME = originalHome; - - // Reset settings manager after each test - settingsManager.reset(); }); // ============================================================================ @@ -63,7 +64,7 @@ describe("Settings Manager - Initialization", () => { await new Promise((resolve) => setTimeout(resolve, 100)); // Reset and re-initialize - settingsManager.reset(); + await settingsManager.reset(); await settingsManager.initialize(); const settings = settingsManager.getSettings(); @@ -194,7 +195,7 @@ describe("Settings Manager - Global Settings", () => { await new Promise((resolve) => setTimeout(resolve, 100)); // Reset and reload - settingsManager.reset(); + await settingsManager.reset(); await settingsManager.initialize(); const settings = settingsManager.getSettings(); @@ -265,7 +266,7 @@ describe("Settings Manager - Project Settings", () => { await new Promise((resolve) => setTimeout(resolve, 100)); // Clear cache and reload - settingsManager.reset(); + await settingsManager.reset(); await settingsManager.initialize(); const reloaded = await settingsManager.loadProjectSettings(testProjectDir); @@ -356,7 +357,7 @@ describe("Settings Manager - Local Project Settings", () => { await new Promise((resolve) => setTimeout(resolve, 100)); // Clear cache and reload - settingsManager.reset(); + await settingsManager.reset(); await settingsManager.initialize(); const reloaded = await settingsManager.loadLocalProjectSettings(testProjectDir); @@ -443,7 +444,7 @@ describe("Settings Manager - Reset", () => { await settingsManager.initialize(); settingsManager.updateSettings({ lastAgent: "agent-reset-test" }); - settingsManager.reset(); + await settingsManager.reset(); // Should throw error after reset expect(() => settingsManager.getSettings()).toThrow(); @@ -456,7 +457,7 @@ describe("Settings Manager - Reset", () => { // Wait for persist await new Promise((resolve) => setTimeout(resolve, 100)); - settingsManager.reset(); + await settingsManager.reset(); await settingsManager.initialize(); const settings = settingsManager.getSettings();