From f0958de1912e95861fad8b4c09001ed5ccbcaa71 Mon Sep 17 00:00:00 2001 From: zeroaltitude Date: Mon, 9 Mar 2026 11:42:20 -0700 Subject: [PATCH] fix: add diagnostic for cleared sessionSaveContent, export drainPostHookActions - Add log.debug when a post-hook clears pre-set sessionSaveContent, making the silent no-op visible to plugin authors (symmetric with blockSessionSave cleared warning) - Extract drainPostHookActions as an exported utility from internal-hooks.ts so handler tests share the exact production drain semantics instead of maintaining a divergent copy - Update handler.test.ts to import and use the shared drain utility Addresses greptile review feedback for confidence score improvement. --- .../bundled/session-memory/handler.test.ts | 46 +++++++------------ src/hooks/bundled/session-memory/handler.ts | 16 +++++++ src/hooks/internal-hooks.ts | 36 ++++++++++++--- 3 files changed, 62 insertions(+), 36 deletions(-) diff --git a/src/hooks/bundled/session-memory/handler.test.ts b/src/hooks/bundled/session-memory/handler.test.ts index dc996ad6d89..79e47ffba3b 100644 --- a/src/hooks/bundled/session-memory/handler.test.ts +++ b/src/hooks/bundled/session-memory/handler.test.ts @@ -573,24 +573,12 @@ describe("session-memory hook", () => { expect(memoryContent).toContain("assistant: Only message 2"); }); - // Helper to drain postHookActions with per-action error isolation, - // matching triggerInternalHook's actual drain behaviour. - async function drainPostHookActions(event: { - postHookActions: Array<() => Promise | void>; - }) { - // Snapshot before draining — matches triggerInternalHook's production - // semantics (prevents self-scheduling actions from executing in the - // same drain cycle). - const pending = [...event.postHookActions]; - // Clear source array so re-drain is a no-op (matches production). - event.postHookActions.length = 0; - for (const action of pending) { - try { - await action(); - } catch { - // Per-action isolation — one failure doesn't block others. - } - } + // Uses the exported drain utility from internal-hooks.ts so tests share + // the exact same drain semantics as production (snapshot → clear → sequential + // await → per-action error isolation). + async function drainActions(event: { postHookActions: Array<() => Promise | void> }) { + const { drainPostHookActions } = await import("../../internal-hooks.js"); + await drainPostHookActions(event.postHookActions); } it("blockSessionSave (pre-set) prevents memory file creation", async () => { @@ -610,7 +598,7 @@ describe("session-memory hook", () => { event.context.blockSessionSave = true; await handler(event); - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]); @@ -643,7 +631,7 @@ describe("session-memory hook", () => { event.context.blockSessionSave = true; // Post-hook action retracts the file - await drainPostHookActions(event); + await drainActions(event); memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); expect(memoryFiles).toHaveLength(0); @@ -678,7 +666,7 @@ describe("session-memory hook", () => { }); event1.timestamp = fixedTimestamp; await handler(event1); - await drainPostHookActions(event1); + await drainActions(event1); const memoryDir = path.join(tempDir, "memory"); const files1 = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -707,7 +695,7 @@ describe("session-memory hook", () => { // Late-block: retraction should restore the FIRST session's content. event2.context.blockSessionSave = true; - await drainPostHookActions(event2); + await drainActions(event2); const restoredContent = await fs.readFile(collidingPath, "utf-8"); expect(restoredContent).toContain("first session"); @@ -734,7 +722,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = "Custom summary from upstream hook"; await handler(event); - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -766,7 +754,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = "Redacted by policy"; // Post-hook action overwrites - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -792,7 +780,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = ""; await handler(event); - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -861,7 +849,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = "Replacement content from policy hook"; // Post-hook should create the directory and write the file - await drainPostHookActions(event); + await drainActions(event); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); expect(files.length).toBeGreaterThan(0); @@ -887,7 +875,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = "Should not appear"; await handler(event); - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]); @@ -916,7 +904,7 @@ describe("session-memory hook", () => { event.context.blockSessionSave = true; event.context.sessionSaveContent = "Should not appear"; - await drainPostHookActions(event); + await drainActions(event); const memoryDir = path.join(tempDir, "memory"); const memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -947,7 +935,7 @@ describe("session-memory hook", () => { // Since the transcript was never loaded, no file can be produced. event.context.blockSessionSave = false; - await drainPostHookActions(event); + await drainActions(event); // No memory file should exist — the transcript was never loaded const memoryDir = path.join(tempDir, "memory"); diff --git a/src/hooks/bundled/session-memory/handler.ts b/src/hooks/bundled/session-memory/handler.ts index 329fec97b48..1ccf017451d 100644 --- a/src/hooks/bundled/session-memory/handler.ts +++ b/src/hooks/bundled/session-memory/handler.ts @@ -495,6 +495,22 @@ const saveSessionToMemory: HookHandler = async (event) => { "execution. To write a file after clearing blockSessionSave, also " + "provide sessionSaveContent with the desired content.", ); + } else if ( + event.context.blockSessionSave !== true && + writtenEntry !== null && + typeof postContent !== "string" && + hasCustomContent + ) { + // sessionSaveContent was pre-set (inline write used custom content), + // then a later hook cleared it. The file retains the pre-set content. + // This is a no-op — to revert to transcript content, the clearing hook + // must provide its own sessionSaveContent. Log for diagnostics so + // plugin authors know their clearing was silently ignored. + log.debug( + "sessionSaveContent was cleared by a post-hook but the inline write " + + "already used the pre-set content. File retains pre-set content. " + + "To override, set sessionSaveContent to the desired replacement.", + ); } }); } catch (err) { diff --git a/src/hooks/internal-hooks.ts b/src/hooks/internal-hooks.ts index 7099f9f7f3d..5ae1e127695 100644 --- a/src/hooks/internal-hooks.ts +++ b/src/hooks/internal-hooks.ts @@ -297,16 +297,38 @@ export async function triggerInternalHook(event: InternalHookEvent): Promise { + const message = err instanceof Error ? err.message : String(err); + log.error(`Post-hook action error [${event.type}:${event.action}]: ${message}`); + }); +} + +/** + * Drain an array of post-hook action callbacks. + * + * Snapshots the array before iterating so that self-scheduling actions + * (ones that push new callbacks) do not execute in the same drain cycle. + * Clears the source array after snapshotting so re-draining is a no-op. + * Errors are caught per-action so one failure doesn't block others. + * + * Exported for use in tests that need to drain post-hook actions without + * going through the full triggerInternalHook pipeline, ensuring they + * share the same drain semantics as production. + * + * @param actions - The postHookActions array to drain (mutated: cleared after snapshot). + * @param onError - Optional per-action error handler. Defaults to swallowing errors silently. + */ +export async function drainPostHookActions( + actions: Array<() => Promise | void>, + onError?: (err: unknown) => void, +): Promise { + const pending = [...actions]; + actions.length = 0; + for (const action of pending) { try { await action(); } catch (err) { - const message = err instanceof Error ? err.message : String(err); - log.error(`Post-hook action error [${event.type}:${event.action}]: ${message}`); + onError?.(err); } } }