From dccc18ee463933bb225043fe126b3c8f88dcccc4 Mon Sep 17 00:00:00 2001 From: jnjpng Date: Thu, 22 Jan 2026 12:09:42 -0800 Subject: [PATCH] fix: use settings as source of truth for hooks config state (#633) --- src/hooks/loader.ts | 121 +++++++------------- src/hooks/writer.ts | 4 - src/settings-manager.ts | 41 ++++++- src/tests/hooks/integration.test.ts | 17 ++- src/tests/hooks/loader.test.ts | 168 +++------------------------- 5 files changed, 102 insertions(+), 249 deletions(-) diff --git a/src/hooks/loader.ts b/src/hooks/loader.ts index a27d836..315354c 100644 --- a/src/hooks/loader.ts +++ b/src/hooks/loader.ts @@ -1,116 +1,71 @@ // src/hooks/loader.ts -// Loads and matches hooks from settings +// Loads and matches hooks from settings-manager -import { homedir } from "node:os"; -import { join } from "node:path"; -import { exists, readFile } from "../utils/fs.js"; +import { settingsManager } from "../settings-manager"; import type { HookCommand, HookEvent, HooksConfig } from "./types"; /** - * Cache for loaded hooks configurations - */ -let globalHooksCache: HooksConfig | null = null; -const projectHooksCache: Map = new Map(); -const projectLocalHooksCache: Map = new Map(); - -/** - * Clear hooks cache (useful for testing or when settings change) + * Clear hooks cache - kept for API compatibility with existing callers. */ export function clearHooksCache(): void { - globalHooksCache = null; - projectHooksCache.clear(); - projectLocalHooksCache.clear(); -} - -/** - * Get the path to global hooks settings - */ -function getGlobalSettingsPath(): string { - const home = process.env.HOME || homedir(); - return join(home, ".letta", "settings.json"); -} - -/** - * Get the path to project hooks settings - */ -function getProjectSettingsPath(workingDirectory: string): string { - return join(workingDirectory, ".letta", "settings.json"); -} - -/** - * Get the path to project-local hooks settings (gitignored) - */ -function getProjectLocalSettingsPath(workingDirectory: string): string { - return join(workingDirectory, ".letta", "settings.local.json"); -} - -/** - * Load hooks configuration from a settings file - */ -async function loadHooksFromFile(path: string): Promise { - if (!exists(path)) { - return null; - } - - try { - const content = await readFile(path); - const settings = JSON.parse(content) as { hooks?: HooksConfig }; - return settings.hooks || null; - } catch (error) { - // Silently ignore parse errors - don't break the app for bad hooks config - console.warn(`Failed to load hooks from ${path}:`, error); - return null; - } + // Settings-manager handles caching } /** * Load global hooks configuration from ~/.letta/settings.json + * Uses settings-manager cache (loaded at app startup) */ -export async function loadGlobalHooks(): Promise { - if (globalHooksCache !== null) { - return globalHooksCache; +export function loadGlobalHooks(): HooksConfig { + try { + return settingsManager.getSettings().hooks || {}; + } catch { + // Settings not initialized yet + return {}; } - - const path = getGlobalSettingsPath(); - const hooks = await loadHooksFromFile(path); - globalHooksCache = hooks || {}; - return globalHooksCache; } /** * Load project hooks configuration from .letta/settings.json + * Uses settings-manager cache */ export async function loadProjectHooks( workingDirectory: string = process.cwd(), ): Promise { - const cached = projectHooksCache.get(workingDirectory); - if (cached !== undefined) { - return cached; + try { + // Ensure project settings are loaded + try { + settingsManager.getProjectSettings(workingDirectory); + } catch { + await settingsManager.loadProjectSettings(workingDirectory); + } + return settingsManager.getProjectSettings(workingDirectory)?.hooks || {}; + } catch { + // Settings not available + return {}; } - - const path = getProjectSettingsPath(workingDirectory); - const hooks = await loadHooksFromFile(path); - const result = hooks || {}; - projectHooksCache.set(workingDirectory, result); - return result; } /** * Load project-local hooks configuration from .letta/settings.local.json + * Uses settings-manager cache */ export async function loadProjectLocalHooks( workingDirectory: string = process.cwd(), ): Promise { - const cached = projectLocalHooksCache.get(workingDirectory); - if (cached !== undefined) { - return cached; + try { + // Ensure local project settings are loaded + try { + settingsManager.getLocalProjectSettings(workingDirectory); + } catch { + await settingsManager.loadLocalProjectSettings(workingDirectory); + } + return ( + settingsManager.getLocalProjectSettings(workingDirectory)?.hooks || {} + ); + } catch { + // Settings not available + return {}; } - - const path = getProjectLocalSettingsPath(workingDirectory); - const hooks = await loadHooksFromFile(path); - const result = hooks || {}; - projectLocalHooksCache.set(workingDirectory, result); - return result; } /** @@ -152,7 +107,7 @@ export async function loadHooks( workingDirectory: string = process.cwd(), ): Promise { const [global, project, projectLocal] = await Promise.all([ - loadGlobalHooks(), + Promise.resolve(loadGlobalHooks()), loadProjectHooks(workingDirectory), loadProjectLocalHooks(workingDirectory), ]); diff --git a/src/hooks/writer.ts b/src/hooks/writer.ts index 5eac385..4c3c70b 100644 --- a/src/hooks/writer.ts +++ b/src/hooks/writer.ts @@ -2,7 +2,6 @@ // Functions to write hooks to settings files via settings-manager import { settingsManager } from "../settings-manager"; -import { clearHooksCache } from "./loader"; import type { HookEvent, HookMatcher, HooksConfig } from "./types"; /** @@ -69,9 +68,6 @@ export async function saveHooksToLocation( settingsManager.updateLocalProjectSettings({ hooks }, workingDirectory); break; } - - // Clear cache so changes take effect immediately - clearHooksCache(); } /** diff --git a/src/settings-manager.ts b/src/settings-manager.ts index b466584..47696e4 100644 --- a/src/settings-manager.ts +++ b/src/settings-manager.ts @@ -411,6 +411,7 @@ class SettingsManager { const projectSettings: ProjectSettings = { localSharedBlockIds: (rawSettings.localSharedBlockIds as Record) ?? {}, + hooks: rawSettings.hooks as HooksConfig | undefined, }; this.projectSettings.set(workingDirectory, projectSettings); @@ -480,7 +481,26 @@ class SettingsManager { if (!exists(dirPath)) { await mkdir(dirPath, { recursive: true }); } - await writeFile(settingsPath, JSON.stringify(this.settings, null, 2)); + + // Read existing file to preserve fields we don't manage (e.g., hooks added externally) + let existingSettings: Record = {}; + if (exists(settingsPath)) { + try { + const content = await readFile(settingsPath); + existingSettings = JSON.parse(content) as Record; + } catch { + // If read/parse fails, use empty object + } + } + + // Merge: existing fields + our managed settings + // Our settings take precedence for fields we manage + const merged = { + ...existingSettings, + ...this.settings, + }; + + await writeFile(settingsPath, JSON.stringify(merged, null, 2)); } catch (error) { console.error("Error saving settings:", error); throw error; @@ -637,7 +657,24 @@ class SettingsManager { await mkdir(dirPath, { recursive: true }); } - await writeFile(settingsPath, JSON.stringify(settings, null, 2)); + // Read existing file to preserve fields we don't manage (e.g., hooks added externally) + let existingSettings: Record = {}; + if (exists(settingsPath)) { + try { + const content = await readFile(settingsPath); + existingSettings = JSON.parse(content) as Record; + } catch { + // If read/parse fails, use empty object + } + } + + // Merge: existing fields + our managed settings + const merged = { + ...existingSettings, + ...settings, + }; + + await writeFile(settingsPath, JSON.stringify(merged, null, 2)); } catch (error) { console.error("Error saving local project settings:", error); throw error; diff --git a/src/tests/hooks/integration.test.ts b/src/tests/hooks/integration.test.ts index 9e9d80c..8249ed6 100644 --- a/src/tests/hooks/integration.test.ts +++ b/src/tests/hooks/integration.test.ts @@ -6,7 +6,6 @@ import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { - clearHooksCache, hasHooks, runNotificationHooks, runPermissionRequestHooks, @@ -20,6 +19,7 @@ import { runSubagentStopHooks, runUserPromptSubmitHooks, } from "../../hooks"; +import { settingsManager } from "../../settings-manager"; // Skip on Windows - test commands use bash syntax (&&, >&2, etc.) // The executor itself is cross-platform, but these test commands are bash-specific @@ -30,7 +30,10 @@ describe.skipIf(isWindows)("Hooks Integration Tests", () => { let fakeHome: string; let originalHome: string | undefined; - beforeEach(() => { + beforeEach(async () => { + // Reset settings manager FIRST before changing HOME + await settingsManager.reset(); + const baseDir = join( tmpdir(), `hooks-integration-${process.pid}-${Math.random().toString(36).slice(2)}`, @@ -43,10 +46,15 @@ describe.skipIf(isWindows)("Hooks Integration Tests", () => { // Override HOME to isolate from real global hooks originalHome = process.env.HOME; process.env.HOME = fakeHome; - clearHooksCache(); + + // Initialize settings manager with new HOME + await settingsManager.initialize(); }); - afterEach(() => { + afterEach(async () => { + // Wait for pending writes and reset + await settingsManager.reset(); + // Restore HOME process.env.HOME = originalHome; try { @@ -56,7 +64,6 @@ describe.skipIf(isWindows)("Hooks Integration Tests", () => { } catch { // Ignore cleanup errors } - clearHooksCache(); }); // Helper to create hook config diff --git a/src/tests/hooks/loader.test.ts b/src/tests/hooks/loader.test.ts index 61762a9..35eb875 100644 --- a/src/tests/hooks/loader.test.ts +++ b/src/tests/hooks/loader.test.ts @@ -3,8 +3,6 @@ import { mkdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { - clearHooksCache, - getHooksForEvent, getMatchingHooks, hasHooksForEvent, loadHooks, @@ -14,13 +12,17 @@ import { mergeHooksConfigs, } from "../../hooks/loader"; import type { HookEvent, HooksConfig } from "../../hooks/types"; +import { settingsManager } from "../../settings-manager"; describe("Hooks Loader", () => { let tempDir: string; let fakeHome: string; let originalHome: string | undefined; - beforeEach(() => { + beforeEach(async () => { + // Reset settings manager FIRST before changing HOME + await settingsManager.reset(); + const baseDir = join(tmpdir(), `hooks-loader-test-${Date.now()}`); // Create separate directories for HOME and project to avoid double-loading fakeHome = join(baseDir, "home"); @@ -30,10 +32,15 @@ describe("Hooks Loader", () => { // Override HOME to isolate from real global hooks originalHome = process.env.HOME; process.env.HOME = fakeHome; - clearHooksCache(); + + // Initialize settings manager with new HOME + await settingsManager.initialize(); }); - afterEach(() => { + afterEach(async () => { + // Wait for pending writes and reset + await settingsManager.reset(); + // Restore HOME process.env.HOME = originalHome; try { @@ -43,7 +50,6 @@ describe("Hooks Loader", () => { } catch { // Ignore cleanup errors } - clearHooksCache(); }); describe("loadProjectHooks", () => { @@ -77,32 +83,7 @@ describe("Hooks Loader", () => { expect(hooks.PreToolUse?.[0]?.matcher).toBe("Bash"); }); - test("caches loaded hooks", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - - const settings = { - hooks: { - PreToolUse: [ - { - matcher: "*", - hooks: [{ type: "command", command: "echo cached" }], - }, - ], - }, - }; - - writeFileSync( - join(settingsDir, "settings.json"), - JSON.stringify(settings), - ); - - const hooks1 = await loadProjectHooks(tempDir); - const hooks2 = await loadProjectHooks(tempDir); - - // Should return same object from cache - expect(hooks1).toBe(hooks2); - }); + // Note: Caching is now handled by settingsManager, not the loader }); describe("mergeHooksConfigs", () => { @@ -368,57 +349,6 @@ describe("Hooks Loader", () => { }); }); - describe("getHooksForEvent", () => { - test("loads and returns matching hooks", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - - const settings = { - hooks: { - PreToolUse: [ - { - matcher: "Bash", - hooks: [{ type: "command", command: "bash hook" }], - }, - ], - }, - }; - - writeFileSync( - join(settingsDir, "settings.json"), - JSON.stringify(settings), - ); - - const hooks = await getHooksForEvent("PreToolUse", "Bash", tempDir); - expect(hooks).toHaveLength(1); - expect(hooks[0]?.command).toBe("bash hook"); - }); - - test("returns empty for non-matching tool", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - - const settings = { - hooks: { - PreToolUse: [ - { - matcher: "Bash", - hooks: [{ type: "command", command: "bash hook" }], - }, - ], - }, - }; - - writeFileSync( - join(settingsDir, "settings.json"), - JSON.stringify(settings), - ); - - const hooks = await getHooksForEvent("PreToolUse", "Edit", tempDir); - expect(hooks).toHaveLength(0); - }); - }); - describe("All 11 hook events", () => { const allEvents: HookEvent[] = [ "PreToolUse", @@ -481,16 +411,6 @@ describe("Hooks Loader", () => { }); describe("Edge cases", () => { - test("handles malformed JSON gracefully", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - writeFileSync(join(settingsDir, "settings.json"), "{ invalid json }"); - - // Should not throw, returns empty config - const hooks = await loadProjectHooks(tempDir); - expect(hooks).toEqual({}); - }); - test("handles settings without hooks field", async () => { const settingsDir = join(tempDir, ".letta"); mkdirSync(settingsDir, { recursive: true }); @@ -502,46 +422,6 @@ describe("Hooks Loader", () => { const hooks = await loadProjectHooks(tempDir); expect(hooks).toEqual({}); }); - - test("clearHooksCache resets cache", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - - writeFileSync( - join(settingsDir, "settings.json"), - JSON.stringify({ - hooks: { - PreToolUse: [ - { matcher: "*", hooks: [{ type: "command", command: "v1" }] }, - ], - }, - }), - ); - - const hooks1 = await loadProjectHooks(tempDir); - expect(hooks1.PreToolUse?.[0]?.hooks[0]?.command).toBe("v1"); - - // Update the file - writeFileSync( - join(settingsDir, "settings.json"), - JSON.stringify({ - hooks: { - PreToolUse: [ - { matcher: "*", hooks: [{ type: "command", command: "v2" }] }, - ], - }, - }), - ); - - // Without clearing cache, should still return v1 - const hooks2 = await loadProjectHooks(tempDir); - expect(hooks2.PreToolUse?.[0]?.hooks[0]?.command).toBe("v1"); - - // After clearing cache, should return v2 - clearHooksCache(); - const hooks3 = await loadProjectHooks(tempDir); - expect(hooks3.PreToolUse?.[0]?.hooks[0]?.command).toBe("v2"); - }); }); // ============================================================================ @@ -578,28 +458,6 @@ describe("Hooks Loader", () => { expect(hooks.PreToolUse).toHaveLength(1); expect(hooks.PreToolUse?.[0]?.hooks[0]?.command).toBe("echo local"); }); - - test("caches loaded local hooks", async () => { - const settingsDir = join(tempDir, ".letta"); - mkdirSync(settingsDir, { recursive: true }); - - writeFileSync( - join(settingsDir, "settings.local.json"), - JSON.stringify({ - hooks: { - PreToolUse: [ - { matcher: "*", hooks: [{ type: "command", command: "cached" }] }, - ], - }, - }), - ); - - const hooks1 = await loadProjectLocalHooks(tempDir); - const hooks2 = await loadProjectLocalHooks(tempDir); - - // Should return same object from cache - expect(hooks1).toBe(hooks2); - }); }); describe("Merged hooks priority (local > project > global)", () => {