diff --git a/src/hooks/bundled/session-memory/handler.test.ts b/src/hooks/bundled/session-memory/handler.test.ts index c2a7e07b79c..9bb2edaf68d 100644 --- a/src/hooks/bundled/session-memory/handler.test.ts +++ b/src/hooks/bundled/session-memory/handler.test.ts @@ -573,7 +573,16 @@ describe("session-memory hook", () => { expect(memoryContent).toContain("assistant: Only message 2"); }); - it("blockSessionSave prevents memory file creation", async () => { + // Helper to drain postHookActions (simulates what triggerInternalHook does) + async function drainPostHookActions(event: { + postHookActions: Array<() => Promise | void>; + }) { + for (const action of event.postHookActions) { + await action(); + } + } + + it("blockSessionSave (pre-set) prevents memory file creation", async () => { const tempDir = await createCaseWorkspace("block-save"); const sessionsDir = path.join(tempDir, "sessions"); await fs.mkdir(sessionsDir, { recursive: true }); @@ -590,13 +599,46 @@ describe("session-memory hook", () => { event.context.blockSessionSave = true; await handler(event); + await drainPostHookActions(event); const memoryDir = path.join(tempDir, "memory"); const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]); expect(memoryFiles.filter((f) => f.endsWith(".md"))).toHaveLength(0); }); - it("sessionSaveContent overrides saved content", async () => { + it("blockSessionSave (late-set) retracts memory file via postHookActions", async () => { + const tempDir = await createCaseWorkspace("block-save-late"); + const sessionsDir = path.join(tempDir, "sessions"); + await fs.mkdir(sessionsDir, { recursive: true }); + const sessionFile = await writeWorkspaceFile({ + dir: sessionsDir, + name: "test-session.jsonl", + content: createMockSessionContent([{ role: "user", content: "secret" }]), + }); + + const event = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + + // Handler writes the file inline (fail-safe) + await handler(event); + + const memoryDir = path.join(tempDir, "memory"); + let memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(memoryFiles.length).toBeGreaterThan(0); // file exists after inline write + + // A later hook sets blockSessionSave + event.context.blockSessionSave = true; + + // Post-hook action retracts the file + await drainPostHookActions(event); + + memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(memoryFiles).toHaveLength(0); + }); + + it("sessionSaveContent (pre-set) overrides saved content", async () => { const tempDir = await createCaseWorkspace("custom-content"); const sessionsDir = path.join(tempDir, "sessions"); await fs.mkdir(sessionsDir, { recursive: true }); @@ -613,6 +655,7 @@ describe("session-memory hook", () => { event.context.sessionSaveContent = "Custom summary from upstream hook"; await handler(event); + await drainPostHookActions(event); const memoryDir = path.join(tempDir, "memory"); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); @@ -622,6 +665,37 @@ describe("session-memory hook", () => { expect(content).not.toContain("original"); }); + it("sessionSaveContent (late-set) overwrites file via postHookActions", async () => { + const tempDir = await createCaseWorkspace("late-custom-content"); + const sessionsDir = path.join(tempDir, "sessions"); + await fs.mkdir(sessionsDir, { recursive: true }); + const sessionFile = await writeWorkspaceFile({ + dir: sessionsDir, + name: "test-session.jsonl", + content: createMockSessionContent([{ role: "user", content: "original" }]), + }); + + const event = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + + // Handler writes default content inline + await handler(event); + + // A later hook sets custom content + event.context.sessionSaveContent = "Redacted by policy"; + + // Post-hook action overwrites + await drainPostHookActions(event); + + const memoryDir = path.join(tempDir, "memory"); + const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(files.length).toBeGreaterThan(0); + const content = await fs.readFile(path.join(memoryDir, files[0]), "utf-8"); + expect(content).toBe("Redacted by policy"); + }); + it("sessionSaveContent empty string writes blank marker file", async () => { const tempDir = await createCaseWorkspace("empty-content"); const sessionsDir = path.join(tempDir, "sessions"); @@ -636,17 +710,40 @@ describe("session-memory hook", () => { cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, previousSessionEntry: { sessionId: "s1", sessionFile }, }); - // Empty string is a valid override — persists a blank marker without - // loading the transcript or generating an LLM slug. event.context.sessionSaveContent = ""; await handler(event); + await drainPostHookActions(event); const memoryDir = path.join(tempDir, "memory"); const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); expect(files.length).toBeGreaterThan(0); const content = await fs.readFile(path.join(memoryDir, files[0]), "utf-8"); - // Should be truly empty — blank marker file expect(content).toBe(""); }); + + it("fail-safe: file is preserved if postHookActions never drain", async () => { + const tempDir = await createCaseWorkspace("fail-safe"); + const sessionsDir = path.join(tempDir, "sessions"); + await fs.mkdir(sessionsDir, { recursive: true }); + const sessionFile = await writeWorkspaceFile({ + dir: sessionsDir, + name: "test-session.jsonl", + content: createMockSessionContent([{ role: "user", content: "important data" }]), + }); + + const event = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + + await handler(event); + // Deliberately do NOT drain postHookActions — simulates a system failure + + const memoryDir = path.join(tempDir, "memory"); + const files = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(files.length).toBeGreaterThan(0); + const content = await fs.readFile(path.join(memoryDir, files[0]), "utf-8"); + expect(content).toContain("important data"); + }); }); diff --git a/src/hooks/bundled/session-memory/handler.ts b/src/hooks/bundled/session-memory/handler.ts index 660934416fa..868c8752088 100644 --- a/src/hooks/bundled/session-memory/handler.ts +++ b/src/hooks/bundled/session-memory/handler.ts @@ -208,18 +208,12 @@ const saveSessionToMemory: HookHandler = async (event) => { const context = event.context || {}; - // Check if another hook (e.g., security plugin) blocked the save. - // NOTE: Internal hooks execute sequentially in FIFO registration order. - // Bundled hooks register before managed/workspace hooks (see workspace.ts - // loadHookEntries merge order), so this check only works when the policy - // decision is set by a typed plugin hook (registerTypedHook) that fires - // earlier in the agent lifecycle, or by a hook loaded via extraDirs that - // registers before bundled hooks. Managed/workspace internal hooks that - // set blockSessionSave on the same event will run AFTER this handler. - if (context.blockSessionSave === true) { - log.debug("Session save blocked by upstream hook"); - return; - } + // NOTE: blockSessionSave and sessionSaveContent are checked in a + // postHookActions callback (see bottom of this handler) so that hooks + // registered after this bundled handler can still set them. The file + // is written inline (fail-safe: if postHookActions never runs, data is + // preserved on disk). The post-hook callback handles retraction + // (blockSessionSave) and content replacement (sessionSaveContent). const cfg = context.cfg as OpenClawConfig | undefined; const contextWorkspaceDir = @@ -238,7 +232,6 @@ const saveSessionToMemory: HookHandler = async (event) => { sessionKey: event.sessionKey, }); const memoryDir = path.join(workspaceDir, "memory"); - await fs.mkdir(memoryDir, { recursive: true }); // Get today's date for filename const now = new Date(event.timestamp); @@ -368,18 +361,63 @@ const saveSessionToMemory: HookHandler = async (event) => { entry = entryParts.join("\n"); } - // Write under memory root with alias-safe file validation. - await writeFileWithinRoot({ - rootDir: memoryDir, - relativePath: filename, - data: entry, - encoding: "utf-8", - }); - log.debug("Memory file written successfully"); + // Write inline (fail-safe: if postHookActions never drains, the file + // is preserved on disk with the best content available at this point). + // If blockSessionSave was already set by an upstream hook, skip the write. + if (context.blockSessionSave === true) { + log.debug("Session save blocked by upstream hook (inline check)"); + } else { + await fs.mkdir(memoryDir, { recursive: true }); + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: entry, + encoding: "utf-8", + }); + log.debug("Memory file written successfully"); - // Log completion (but don't send user-visible confirmation - it's internal housekeeping) - const relPath = memoryFilePath.replace(os.homedir(), "~"); - log.info(`Session context saved to ${relPath}`); + const relPath = memoryFilePath.replace(os.homedir(), "~"); + log.info(`Session context saved to ${relPath}`); + } + + // Defer retraction/replacement to post-hook phase so that hooks + // registered after this handler can set blockSessionSave or + // sessionSaveContent and still have them honored. + const writtenEntry = context.blockSessionSave === true ? null : entry; + event.postHookActions.push(async () => { + try { + // If a later hook blocked the save, retract the file we just wrote. + if (event.context.blockSessionSave === true && writtenEntry !== null) { + try { + await fs.unlink(memoryFilePath); + log.debug("Session save retracted by post-hook (blockSessionSave)"); + } catch (err) { + // File may not exist if inline write also didn't happen — that's fine. + if ((err as NodeJS.ErrnoException).code !== "ENOENT") { + throw err; + } + } + return; + } + + // If a later hook set sessionSaveContent, overwrite with new content. + const postContent = event.context.sessionSaveContent; + if (typeof postContent === "string" && postContent !== writtenEntry) { + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: postContent, + encoding: "utf-8", + }); + log.debug("Session save content replaced by post-hook (sessionSaveContent)", { + length: postContent.length, + }); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + log.error(`Post-hook session-memory action failed: ${message}`); + } + }); } catch (err) { if (err instanceof Error) { log.error("Failed to save session memory", { diff --git a/src/hooks/internal-hooks.test.ts b/src/hooks/internal-hooks.test.ts index 8f71c6b80cf..c0a87b014bb 100644 --- a/src/hooks/internal-hooks.test.ts +++ b/src/hooks/internal-hooks.test.ts @@ -445,6 +445,100 @@ describe("hooks", () => { }); }); + describe("postHookActions", () => { + it("executes post-hook actions after all handlers complete", async () => { + const order: string[] = []; + + registerInternalHook("command:new", async (event) => { + order.push("handler-1"); + event.postHookActions.push(async () => { + order.push("post-action-1"); + }); + }); + + registerInternalHook("command:new", async () => { + order.push("handler-2"); + }); + + const event = createInternalHookEvent("command", "new", "test-session"); + await triggerInternalHook(event); + + expect(order).toEqual(["handler-1", "handler-2", "post-action-1"]); + }); + + it("post-hook actions see context set by later handlers", async () => { + let sawFlag = false; + + registerInternalHook("command:new", async (event) => { + event.postHookActions.push(async () => { + sawFlag = event.context.myFlag === true; + }); + }); + + registerInternalHook("command:new", async (event) => { + event.context.myFlag = true; + }); + + const event = createInternalHookEvent("command", "new", "test-session"); + await triggerInternalHook(event); + + expect(sawFlag).toBe(true); + }); + + it("post-hook action errors don't prevent other post-hook actions", async () => { + const executed: string[] = []; + + registerInternalHook("command:new", async (event) => { + event.postHookActions.push(async () => { + throw new Error("boom"); + }); + event.postHookActions.push(async () => { + executed.push("second-action"); + }); + }); + + const event = createInternalHookEvent("command", "new", "test-session"); + await triggerInternalHook(event); + + expect(executed).toEqual(["second-action"]); + }); + + it("multiple handlers can push post-hook actions; all execute in order", async () => { + const order: string[] = []; + + registerInternalHook("command:new", async (event) => { + event.postHookActions.push(() => { + order.push("from-handler-1"); + }); + }); + + registerInternalHook("command:new", async (event) => { + event.postHookActions.push(() => { + order.push("from-handler-2"); + }); + }); + + const event = createInternalHookEvent("command", "new", "test-session"); + await triggerInternalHook(event); + + expect(order).toEqual(["from-handler-1", "from-handler-2"]); + }); + + it("initializes postHookActions as empty array", () => { + const event = createInternalHookEvent("command", "new", "test-session"); + expect(event.postHookActions).toEqual([]); + }); + + it("does not run post-hook actions when no handlers are registered", async () => { + const event = createInternalHookEvent("command", "new", "test-session"); + event.postHookActions.push(() => { + throw new Error("should not run"); + }); + // triggerInternalHook returns early when no handlers — post-hooks don't run + await expect(triggerInternalHook(event)).resolves.not.toThrow(); + }); + }); + describe("getRegisteredEventKeys", () => { it("should return all registered event keys", () => { registerInternalHook("command:new", vi.fn()); diff --git a/src/hooks/internal-hooks.ts b/src/hooks/internal-hooks.ts index b73dcb75fab..a8d0736fbf0 100644 --- a/src/hooks/internal-hooks.ts +++ b/src/hooks/internal-hooks.ts @@ -169,6 +169,12 @@ export interface InternalHookEvent { timestamp: Date; /** Messages to send back to the user (hooks can push to this array) */ messages: string[]; + /** Deferred actions to run after all handlers complete. + * Handlers push async callbacks here; triggerInternalHook drains them + * sequentially after the main handler loop. This eliminates FIFO + * registration-order dependencies: a handler that runs early can defer + * work that depends on context set by later handlers. */ + postHookActions: Array<() => Promise | void>; } export type InternalHookHandler = (event: InternalHookEvent) => Promise | void; @@ -285,6 +291,21 @@ export async function triggerInternalHook(event: InternalHookEvent): Promise 0) { + for (const action of event.postHookActions) { + 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}`); + } + } + } } /** @@ -308,6 +329,7 @@ export function createInternalHookEvent( context, timestamp: new Date(), messages: [], + postHookActions: [], }; }