diff --git a/src/hooks/bundled/boot-md/handler.test.ts b/src/hooks/bundled/boot-md/handler.test.ts index de2abd6475f..46466b254b8 100644 --- a/src/hooks/bundled/boot-md/handler.test.ts +++ b/src/hooks/bundled/boot-md/handler.test.ts @@ -37,6 +37,7 @@ function makeEvent(overrides?: Partial): InternalHookEvent { context: {}, timestamp: new Date(), messages: [], + postHookActions: [], ...overrides, }; } diff --git a/src/hooks/bundled/session-memory/handler.test.ts b/src/hooks/bundled/session-memory/handler.test.ts index fb7e9ca0a4d..a94efde7c3b 100644 --- a/src/hooks/bundled/session-memory/handler.test.ts +++ b/src/hooks/bundled/session-memory/handler.test.ts @@ -1,3 +1,4 @@ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -572,4 +573,376 @@ describe("session-memory hook", () => { expect(memoryContent).toContain("user: Only message 1"); expect(memoryContent).toContain("assistant: Only message 2"); }); + + // Uses the exported drain utility from internal-hooks.ts so tests share + // the exact same drain semantics as production (snapshot → clear → sequential + // await). Errors are rethrown (rather than swallowed) so test failures surface + // the actual error message instead of a confusing downstream assertion failure. + async function drainActions(event: { postHookActions: Array<() => Promise | void> }) { + const { drainPostHookActions } = await import("../../internal-hooks.js"); + await drainPostHookActions(event.postHookActions, (err) => { + throw err; + }); + } + + 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 }); + 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 }, + }); + event.context.blockSessionSave = true; + + await handler(event); + await drainActions(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("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 drainActions(event); + + memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(memoryFiles).toHaveLength(0); + }); + + it("late-block retraction restores pre-existing file instead of deleting (slug collision)", async () => { + const tempDir = await createCaseWorkspace("block-save-restore"); + 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: "first session" }]), + }); + + // Pin crypto.randomUUID AND timestamp to force deterministic fallback slug — + // both handler calls produce the same HHMMSS prefix (fixed timestamp) + // and the same random suffix (pinned UUID). LLM slug generation is + // disabled in the test environment (VITEST=true), so the collision + // is exercised entirely through the fallback path. Without pinning + // the clock, a wall-clock second boundary between event1 and event2 + // would produce different HHMMSS prefixes → no collision. + vi.spyOn(crypto, "randomUUID").mockReturnValue("aaaa1111-2222-3333-4444-555566667777"); + const fixedTimestamp = new Date("2024-01-15T12:34:56.000Z"); + + try { + // First handler: creates memory file with deterministic slug. + const event1 = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + event1.timestamp = fixedTimestamp; + await handler(event1); + await drainActions(event1); + + const memoryDir = path.join(tempDir, "memory"); + const files1 = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(files1).toHaveLength(1); + const collidingFile = files1[0]; + const collidingPath = path.join(memoryDir, collidingFile); + const originalContent = await fs.readFile(collidingPath, "utf-8"); + expect(originalContent).toContain("first session"); + + // Second handler: same deterministic slug → overwrites the file (collision). + const sessionFile2 = await writeWorkspaceFile({ + dir: sessionsDir, + name: "test-session2.jsonl", + content: createMockSessionContent([{ role: "user", content: "second session" }]), + }); + const event2 = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s2", sessionFile: sessionFile2 }, + }); + event2.timestamp = fixedTimestamp; + await handler(event2); + + // Verify the file was overwritten by second handler. + const overwrittenContent = await fs.readFile(collidingPath, "utf-8"); + expect(overwrittenContent).toContain("second session"); + + // Late-block: retraction should restore the FIRST session's content. + event2.context.blockSessionSave = true; + await drainActions(event2); + + const restoredContent = await fs.readFile(collidingPath, "utf-8"); + expect(restoredContent).toContain("first session"); + expect(restoredContent).not.toContain("second session"); + } finally { + vi.restoreAllMocks(); + } + }); + + 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 }); + 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 }, + }); + event.context.sessionSaveContent = "Custom summary from upstream hook"; + + await handler(event); + await drainActions(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("Custom summary from upstream 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 drainActions(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"); + await fs.mkdir(sessionsDir, { recursive: true }); + const sessionFile = await writeWorkspaceFile({ + dir: sessionsDir, + name: "test-session.jsonl", + content: createMockSessionContent([{ role: "user", content: "sensitive data" }]), + }); + + const event = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + event.context.sessionSaveContent = ""; + + await handler(event); + await drainActions(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(""); + }); + + 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"); + }); + + it("blockSessionSave pre-set then cleared with sessionSaveContent creates file (mkdir edge case)", async () => { + // Regression: when blockSessionSave is true initially, the inline write + // is skipped — including the fs.mkdir. If a later hook clears the flag + // and sets sessionSaveContent, the post-hook write must create the + // directory itself or it fails with ENOENT. + const tempDir = await createCaseWorkspace("block-then-clear"); + 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 }, + }); + event.context.blockSessionSave = true; + + // Handler runs — inline write is skipped, memoryDir never created + await handler(event); + + const memoryDir = path.join(tempDir, "memory"); + const existsBefore = await fs + .stat(memoryDir) + .then(() => true) + .catch(() => false); + expect(existsBefore).toBe(false); + + // A later hook clears blockSessionSave and sets custom content + event.context.blockSessionSave = false; + event.context.sessionSaveContent = "Replacement content from policy hook"; + + // Post-hook should create the directory and write the file + await drainActions(event); + + 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("Replacement content from policy hook"); + }); + + it("blockSessionSave takes precedence over sessionSaveContent (both pre-set)", async () => { + const tempDir = await createCaseWorkspace("block-beats-content-pre"); + 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 }, + }); + event.context.blockSessionSave = true; + event.context.sessionSaveContent = "Should not appear"; + + await handler(event); + await drainActions(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("blockSessionSave takes precedence over sessionSaveContent (both late-set)", async () => { + const tempDir = await createCaseWorkspace("block-beats-content-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 inline (no flags set yet) + await handler(event); + + // Later hooks set both flags + event.context.blockSessionSave = true; + event.context.sessionSaveContent = "Should not appear"; + + await drainActions(event); + + const memoryDir = path.join(tempDir, "memory"); + const memoryFiles = (await fs.readdir(memoryDir)).filter((f) => f.endsWith(".md")); + expect(memoryFiles).toHaveLength(0); + }); + + it("blockSessionSave pre-set then cleared without sessionSaveContent warns and writes nothing", async () => { + const tempDir = await createCaseWorkspace("block-cleared-no-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: "will not be saved" }]), + }); + + const event = createHookEvent("command", "new", "agent:main:main", { + cfg: { agents: { defaults: { workspace: tempDir } } } satisfies OpenClawConfig, + previousSessionEntry: { sessionId: "s1", sessionFile }, + }); + + // Pre-set blockSessionSave — handler skips transcript loading + inline write + event.context.blockSessionSave = true; + + await handler(event); + + // A later hook clears blockSessionSave but forgets to set sessionSaveContent. + // Since the transcript was never loaded, no file can be produced. + event.context.blockSessionSave = false; + + await drainActions(event); + + // No memory file should exist — the transcript was never loaded + const memoryDir = path.join(tempDir, "memory"); + const memoryFiles = await fs.readdir(memoryDir).catch(() => [] as string[]); + expect(memoryFiles.filter((f) => f.endsWith(".md"))).toHaveLength(0); + }); }); diff --git a/src/hooks/bundled/session-memory/handler.ts b/src/hooks/bundled/session-memory/handler.ts index 32fc36b23f0..1a8fb597e05 100644 --- a/src/hooks/bundled/session-memory/handler.ts +++ b/src/hooks/bundled/session-memory/handler.ts @@ -5,6 +5,7 @@ * Creates a new dated memory file with LLM-generated slug */ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -207,6 +208,14 @@ const saveSessionToMemory: HookHandler = async (event) => { log.debug("Hook triggered for reset/new command", { action: event.action }); const context = event.context || {}; + + // 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 = typeof context.workspaceDir === "string" && context.workspaceDir.trim().length > 0 @@ -224,7 +233,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); @@ -279,8 +287,22 @@ const saveSessionToMemory: HookHandler = async (event) => { let slug: string | null = null; let sessionContent: string | null = null; + const hasCustomContent = typeof context.sessionSaveContent === "string"; - if (sessionFile) { + // Short-circuit transcript loading and LLM slug generation when + // blockSessionSave is already set — no point loading sensitive content + // or sending it to a model provider when saving is explicitly blocked. + const blockPreSet = context.blockSessionSave === true; + + // Known limitation: if an earlier hook pre-sets sessionSaveContent and + // a later hook *clears* it (expecting a revert to the default + // transcript), the transcript is not available — it was never loaded + // because hasCustomContent was true at this point. The post-hook + // cannot fall back to the default entry without re-reading the session + // file and re-running slug generation. In practice, hooks that want + // to override earlier custom content should set their own + // sessionSaveContent rather than clearing it. + if (sessionFile && !hasCustomContent && !blockPreSet) { // Get recent conversation content, with fallback to rotated reset transcript. sessionContent = await getRecentSessionContentWithResetFallback(sessionFile, messageCount); log.debug("Session content loaded", { @@ -304,10 +326,15 @@ const saveSessionToMemory: HookHandler = async (event) => { } } - // If no slug, use timestamp + // If no slug, use timestamp with a random suffix to avoid collisions. + // Second-resolution (HHMMSS) alone can collide when automated or + // multi-channel setups emit rapid /new or /reset commands within the + // same second — both writes target the same filename and the later + // one silently overwrites the earlier memory entry. if (!slug) { const timeSlug = now.toISOString().split("T")[1].split(".")[0].replace(/:/g, ""); - slug = timeSlug.slice(0, 4); // HHMM + const rand = crypto.randomUUID().replace(/-/g, "").slice(0, 4); + slug = `${timeSlug.slice(0, 6)}-${rand}`; log.debug("Using fallback timestamp slug", { slug }); } @@ -326,35 +353,268 @@ const saveSessionToMemory: HookHandler = async (event) => { const sessionId = (sessionEntry.sessionId as string) || "unknown"; const source = (context.commandSource as string) || "unknown"; - // Build Markdown entry - const entryParts = [ - `# Session: ${dateStr} ${timeStr} UTC`, - "", - `- **Session Key**: ${displaySessionKey}`, - `- **Session ID**: ${sessionId}`, - `- **Source**: ${source}`, - "", - ]; + // Use custom content from upstream hook if available, otherwise build entry. + // hasCustomContent (set above) already gates session loading + slug generation. + // When blockPreSet is true, skip entry construction entirely — the inline + // write won't happen and the value would be discarded. + let entry: string; + if (blockPreSet) { + // Block takes precedence — skip entry construction entirely since the + // inline write won't happen and the value would be discarded. + entry = ""; + if (hasCustomContent) { + log.debug( + "blockSessionSave pre-set — sessionSaveContent was also set but will be ignored " + + "(blockSessionSave takes precedence over sessionSaveContent)", + ); + } else { + log.debug("Session save blocked by upstream hook (inline check)"); + } + } else if (hasCustomContent) { + // An empty string is a valid redaction signal — hooks may intentionally + // set it to persist a blank marker while avoiding transcript retention. + entry = context.sessionSaveContent as string; + log.debug("Using custom session content from upstream hook", { + length: entry.length, + }); + } else { + const entryParts = [ + `# Session: ${dateStr} ${timeStr} UTC`, + "", + `- **Session Key**: ${displaySessionKey}`, + `- **Session ID**: ${sessionId}`, + `- **Source**: ${source}`, + "", + ]; - // Include conversation content if available - if (sessionContent) { - entryParts.push("## Conversation Summary", "", sessionContent, ""); + if (sessionContent) { + entryParts.push("## Conversation Summary", "", sessionContent, ""); + } + + entry = entryParts.join("\n"); } - const entry = entryParts.join("\n"); + // 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. + // + // Before writing, snapshot any pre-existing file content so that late-block + // retraction can restore it instead of deleting — preventing accidental + // erasure of prior memory files when LLM slugs collide on the same day. + let preExistingContent: string | null = null; + if (blockPreSet) { + // Already logged above — nothing to write. + } else { + await fs.mkdir(memoryDir, { recursive: true }); + try { + preExistingContent = await fs.readFile(memoryFilePath, "utf-8"); + } catch (err: unknown) { + // File doesn't exist yet — normal case, nothing to preserve. + // Rethrow non-ENOENT errors (EACCES, EISDIR, etc.) to avoid silently + // losing preExistingContent, which would cause late-block retraction + // to delete the file instead of restoring a prior session's history. + if ( + err instanceof Error && + "code" in err && + (err as NodeJS.ErrnoException).code !== "ENOENT" + ) { + throw err; + } + } + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: entry, + encoding: "utf-8", + }); + log.debug("Memory file written successfully"); - // Write under memory root with alias-safe file validation. - await writeFileWithinRoot({ - rootDir: memoryDir, - relativePath: filename, - data: entry, - encoding: "utf-8", + 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 inlineWriteHappened = !blockPreSet; + const writtenEntry = inlineWriteHappened ? entry : null; + // Post-hook callback — errors propagate to the framework's per-action + // catch in triggerInternalHook, which provides consistent log formatting + // and per-action isolation. + event.postHookActions.push(async () => { + // If a later hook blocked the save, retract the file we just wrote. + // If the file existed before our write (slug collision), restore the + // original content instead of deleting — avoids erasing prior history. + if (event.context.blockSessionSave === true && inlineWriteHappened) { + // Privacy note: late-set blockSessionSave retracts the file but does NOT + // prevent transcript content from having already been sent to the LLM + // provider for slug generation — but only when the transcript was actually + // loaded (i.e. no custom content was pre-set). When hasCustomContent is + // true, transcript loading and LLM calls were skipped entirely. + if (!hasCustomContent && sessionContent) { + // Only warn when transcript was actually loaded and potentially + // sent to the LLM for slug generation. When sessionFile was null + // or sessionContent failed to load, no data left the device. + log.warn( + "blockSessionSave was set by a late hook — memory file will be retracted, but " + + "transcript content may have already been sent to the LLM provider for slug generation. " + + "To prevent transcript processing entirely, set blockSessionSave before the " + + "session-memory handler runs.", + ); + } + // Verify we're reverting our own write before touching the file. + // A concurrent session (e.g. /new, /reset) may have written to the + // same filename between our inline write and this post-hook drain. + // If the current content doesn't match what we wrote, skip retraction + // to avoid clobbering the other session's data. + let currentContent: string | null = null; + try { + currentContent = await fs.readFile(memoryFilePath, "utf-8"); + } catch (err: unknown) { + if ( + err instanceof Error && + "code" in err && + (err as NodeJS.ErrnoException).code === "ENOENT" + ) { + if (preExistingContent !== null) { + // Our inline write overwrote a pre-existing entry (slug collision), + // and the file was subsequently deleted externally. Restore the + // prior session's content — it was lost to our inline overwrite. + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: preExistingContent, + encoding: "utf-8", + }); + log.debug( + "Session save retracted by post-hook — pre-existing file restored after external deletion", + ); + } else { + // No prior content existed — file was externally deleted, nothing to restore. + log.debug("Session save retraction skipped — file already removed"); + } + return; + } + throw err; + } + + if (currentContent !== writtenEntry) { + // File content differs from what we wrote — another session has + // written to this file since our inline write. Do not clobber. + log.warn( + "Session save retraction skipped — file was modified by another " + + "session since our inline write (concurrent save detected)", + ); + return; + } + + if (preExistingContent !== null) { + // Slug collision: another entry already existed at this filename + // before our inline write. Restore the original content rather + // than deleting — preserves the prior session's history. + // writeFileWithinRoot errors (e.g. ENOENT if memoryDir was + // removed after our inline write) are NOT swallowed — they + // indicate a real filesystem inconsistency that must surface. + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: preExistingContent, + encoding: "utf-8", + }); + log.debug("Session save retracted by post-hook — pre-existing file restored"); + } else { + await fs.unlink(memoryFilePath); + log.debug("Session save retracted by post-hook (blockSessionSave)"); + } + return; + } + + // If a later hook set sessionSaveContent, overwrite with new content. + // blockSessionSave takes precedence — never create/overwrite a file that + // was blocked, even if sessionSaveContent is also set. + const postContent = event.context.sessionSaveContent; + if ( + event.context.blockSessionSave !== true && + typeof postContent === "string" && + // Two distinct intents: write if no inline write happened (writtenEntry + // is null because blockPreSet was true) OR if the content changed. + (writtenEntry === null || postContent !== writtenEntry) + ) { + // Verify ownership before overwriting — if another concurrent run wrote + // to the same file since our inline write, do not clobber their content. + // Same TOCTOU guard as the late-block retraction path. + if (writtenEntry !== null) { + let currentContent: string | null = null; + try { + currentContent = await fs.readFile(memoryFilePath, "utf-8"); + } catch (err: unknown) { + if ( + err instanceof Error && + "code" in err && + (err as NodeJS.ErrnoException).code === "ENOENT" + ) { + // File was externally deleted — safe to recreate with new content. + currentContent = null; + } else { + throw err; + } + } + if (currentContent !== null && currentContent !== writtenEntry) { + log.warn( + "Session save content replacement skipped — file was modified by another " + + "session since our inline write (concurrent save detected)", + ); + return; + } + } + + // Ensure memoryDir exists — the inline write may have been + // skipped (e.g. blockSessionSave was true initially) so mkdir + // might never have run. + await fs.mkdir(memoryDir, { recursive: true }); + await writeFileWithinRoot({ + rootDir: memoryDir, + relativePath: filename, + data: postContent, + encoding: "utf-8", + }); + log.debug("Session save content replaced by post-hook (sessionSaveContent)", { + length: postContent.length, + }); + } else if ( + event.context.blockSessionSave !== true && + writtenEntry === null && + typeof postContent !== "string" + ) { + // blockSessionSave was pre-set (causing writtenEntry=null and no inline + // write), then a later hook cleared it without providing sessionSaveContent. + // The transcript was never loaded, so we cannot produce a file. Warn so + // plugin authors know to supply content when un-blocking. + log.warn( + "blockSessionSave was cleared but no sessionSaveContent provided — " + + "no memory file written. Transcript was not loaded because " + + "sessionSaveContent or blockSessionSave was pre-set during handler " + + "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.", + ); + } }); - 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}`); } 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..ffa7639bcb0 100644 --- a/src/hooks/internal-hooks.test.ts +++ b/src/hooks/internal-hooks.test.ts @@ -445,6 +445,116 @@ 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("drains post-hook actions even when no handlers are registered", async () => { + const event = createInternalHookEvent("command", "new", "test-session"); + let ran = false; + event.postHookActions.push(() => { + ran = true; + }); + // No handlers registered — post-hooks should still drain + await triggerInternalHook(event); + expect(ran).toBe(true); + }); + + it("clears postHookActions after drain — re-trigger is a no-op", async () => { + const event = createInternalHookEvent("command", "new", "test-session"); + let count = 0; + event.postHookActions.push(() => { + count++; + }); + await triggerInternalHook(event); + expect(count).toBe(1); + // Second trigger on same event should not re-execute + await triggerInternalHook(event); + expect(count).toBe(1); + expect(event.postHookActions).toEqual([]); + }); + }); + 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..de3a7a79c0a 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; @@ -268,15 +274,16 @@ export function getRegisteredEventKeys(): string[] { * @param event - The event to trigger */ export async function triggerInternalHook(event: InternalHookEvent): Promise { + // Normalize postHookActions before entering the handler loop — handlers + // may push to this array during execution. Legacy callers that construct + // hook events without the field would otherwise hit a TypeError on .push(). + event.postHookActions ??= []; + const typeHandlers = handlers.get(event.type) ?? []; const specificHandlers = handlers.get(`${event.type}:${event.action}`) ?? []; const allHandlers = [...typeHandlers, ...specificHandlers]; - if (allHandlers.length === 0) { - return; - } - for (const handler of allHandlers) { try { await handler(event); @@ -285,6 +292,59 @@ 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 an immediate re-drain with + * no intervening pushes is a no-op. Note: if an action pushes new callbacks + * *during* its own execution, those entries land in the already-cleared source + * array and will persist after this function returns — a subsequent drain + * call will execute them. + * Errors are caught per-action via try/catch. Per-action isolation (one + * failure doesn't block others) holds only when `onError` does not rethrow. + * If `onError` rethrows, subsequent actions are skipped — callers that want + * fail-fast semantics (e.g. tests) can use this intentionally. + * + * 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 - Per-action error handler. Required to force callers to be explicit + * about error handling — pass a logger in production, a rethrower in tests, or + * `() => {}` only when intentionally swallowing errors. + */ +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) { + onError(err); + } + } } /** @@ -308,6 +368,7 @@ export function createInternalHookEvent( context, timestamp: new Date(), messages: [], + postHookActions: [], }; } diff --git a/src/hooks/llm-slug-generator.ts b/src/hooks/llm-slug-generator.ts index eb355fc3289..dd562642ebd 100644 --- a/src/hooks/llm-slug-generator.ts +++ b/src/hooks/llm-slug-generator.ts @@ -2,6 +2,7 @@ * LLM-based slug generator for session memory filenames */ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -50,8 +51,16 @@ Reply with ONLY the slug, nothing else. Examples: "vendor-pitch", "api-design", const provider = parsed?.provider ?? DEFAULT_PROVIDER; const model = parsed?.model ?? DEFAULT_MODEL; + // Security: disable tools for this one-shot call. The prompt embeds + // up to 2 000 chars of raw conversation content, which is attacker- + // controllable. Without disableTools the embedded agent inherits the + // full tool set (exec, file write, messaging, …), so a crafted + // conversation could prompt-inject the slug-generation call into + // executing arbitrary side-effects *before* the (well-sanitised) slug + // text is extracted. Slug generation is pure text — it never needs + // tool access. const result = await runEmbeddedPiAgent({ - sessionId: `slug-generator-${Date.now()}`, + sessionId: `slug-generator-${crypto.randomUUID()}`, sessionKey: "temp:slug-generator", agentId, sessionFile: tempSessionFile, @@ -61,8 +70,9 @@ Reply with ONLY the slug, nothing else. Examples: "vendor-pitch", "api-design", prompt, provider, model, + disableTools: true, timeoutMs: 15_000, // 15 second timeout - runId: `slug-gen-${Date.now()}`, + runId: `slug-gen-${crypto.randomUUID()}`, }); // Extract text from payloads