From 4ca50346db70155302f069b2d6b1d3fae3906fd1 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 05:44:32 +0000 Subject: [PATCH 1/9] fix: grant summarizer dirs explicitly instead of relying on bypassPermissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wiki worker and the memory backfill both spawn `claude -p` with `--permission-mode bypassPermissions` and then hand it absolute paths outside the session cwd: the worker's `$TMPDIR/deeplake-wiki-*` scratch dir, and the backfill's transcript + staging dirs. An enterprise policy can set `"disableBypassPermissionsMode": "disable"` (macOS: /Library/Application Support/ClaudeCode/managed-settings.json). The child then falls back to normal permissioning, refuses every out-of-cwd path, and — because print mode cannot prompt — exits 0 having written nothing: running claude -p claude -p exited (code 0) no summary file generated On such a machine every summary is a header-only stub and every backfill session reports `no-summary`. Name the dirs and tools instead. `--add-dir` + `--allowedTools Read Write` is both policy-proof and least-privilege — the summarizer never needs more than Read and Write — so a caller supplying grants gets them INSTEAD of the bypass. Callers that supply none keep the old blanket bypass, so nothing else changes. Second fix, same root cause: exit 0 is not proof of work. When the worker had pre-seeded `tmpSummary` with the stored summary (the resumed-session path), an exit-0-no-write run uploaded that base back verbatim AND stamped `lastSummaryCount`, slicing the unread events away forever — so a session stayed a placeholder run after run, by construction. The existing `summaryChanged` guard was only consulted when the child exited non-zero; apply it on the success path too. Validated out-of-band on a policy-managed macOS host, which CI cannot reproduce: the wiki prompt now yields a real summary where it previously yielded none, and stageSession returns ok:true instead of reason:"no-summary". Tests: the exit-0-wrote-nothing regression (no upload, offset not advanced — verified to fail against the pre-fix worker) and its mirror (a run that does rewrite the summary still uploads and stamps), plus the grants contract: grants replace the bypass, no grants keep it, an empty grants object degrades to the bypass rather than granting nothing, and the Windows `.cmd` branch quotes a path containing spaces. The existing wiki-worker assertion that pinned the bypass flags is updated to pin the grants. --- src/hooks/wiki-worker-spawn.ts | 70 +++++++-- src/hooks/wiki-worker.ts | 18 ++- src/skillify/stage-memory.ts | 28 +++- tests/claude-code/wiki-worker.test.ts | 54 ++++++- tests/shared/claude-permission-grants.test.ts | 145 ++++++++++++++++++ 5 files changed, 294 insertions(+), 21 deletions(-) create mode 100644 tests/shared/claude-permission-grants.test.ts diff --git a/src/hooks/wiki-worker-spawn.ts b/src/hooks/wiki-worker-spawn.ts index 79f374cea..066f70f4d 100644 --- a/src/hooks/wiki-worker-spawn.ts +++ b/src/hooks/wiki-worker-spawn.ts @@ -2,13 +2,10 @@ import type { ExecFileSyncOptions } from "node:child_process"; import { binNeedsShell, shellFile } from "../utils/resolve-cli-bin.js"; /** Fixed flags for the summary-generation `claude -p` call (no user input). */ -const CLAUDE_FLAGS = [ - "--no-session-persistence", - "--model", - "haiku", - "--permission-mode", - "bypassPermissions", -] as const; +const CLAUDE_BASE_FLAGS = ["--no-session-persistence", "--model", "haiku"] as const; + +/** Blanket grant, used only when the caller names no explicit grants. */ +const CLAUDE_BYPASS_FLAGS = ["--permission-mode", "bypassPermissions"] as const; export interface ClaudeInvocation { file: string; @@ -16,6 +13,43 @@ export interface ClaudeInvocation { options: ExecFileSyncOptions; } +/** + * Explicit grants for call sites that read/write files OUTSIDE the session cwd. + * + * `bypassPermissions` is NOT honored under an enterprise policy that sets + * `"disableBypassPermissionsMode": "disable"` (macOS: + * /Library/Application Support/ClaudeCode/managed-settings.json). The child then + * falls back to normal permissioning, refuses every path outside the working + * directory — the wiki worker's `$TMPDIR/deeplake-wiki-*` scratch dir, or a + * backfill's transcript + staging dir — and exits 0 having written nothing. On + * such a machine every summary stays a header-only stub and every backfill + * reports `no-summary`. + * + * Naming the dirs and tools instead is both policy-proof and least-privilege + * (the summarizer only ever needs Read + Write), so a caller that supplies + * grants gets them INSTEAD of the bypass, not in addition to it. + */ +export interface ClaudeGrants { + /** Directories to expose to the child (each becomes `--add-dir `). */ + addDirs?: string[]; + /** Tools to pre-approve, e.g. `["Read", "Write"]`. */ + allowedTools?: string[]; +} + +/** + * `quotePaths` is for the Windows `.cmd` branch, where args are re-joined into a + * shell command line: a temp dir there routinely contains spaces + * (`C:\Users\First Last\AppData\Local\Temp`) and would otherwise split. + */ +export function permissionFlags(grants: ClaudeGrants | undefined, quotePaths = false): string[] { + if (!grants) return [...CLAUDE_BYPASS_FLAGS]; + const flags: string[] = []; + for (const dir of grants.addDirs ?? []) flags.push("--add-dir", quotePaths ? `"${dir}"` : dir); + if (grants.allowedTools?.length) flags.push("--allowedTools", ...grants.allowedTools); + // An empty grants object would otherwise leave the child with no grant at all. + return flags.length > 0 ? flags : [...CLAUDE_BYPASS_FLAGS]; +} + /** * Build the `execFileSync` descriptor for the summary-generation claude call. * @@ -30,11 +64,15 @@ export interface ClaudeInvocation { * prompt as a positional arg, no shell — so the already-working path stays * byte-identical. */ -export function buildClaudeInvocation(claudeBin: string, prompt: string): ClaudeInvocation { +export function buildClaudeInvocation( + claudeBin: string, + prompt: string, + grants?: ClaudeGrants, +): ClaudeInvocation { if (binNeedsShell(claudeBin)) { return { file: shellFile(claudeBin), - args: ["-p", ...CLAUDE_FLAGS], + args: ["-p", ...CLAUDE_BASE_FLAGS, ...permissionFlags(grants, true)], // windowsHide: the wiki worker is a detached, console-less process, so // without CREATE_NO_WINDOW Windows allocates a visible console window // (titled after the CLI exe) for the child. No-op on POSIX. @@ -43,7 +81,7 @@ export function buildClaudeInvocation(claudeBin: string, prompt: string): Claude } return { file: claudeBin, - args: ["-p", prompt, ...CLAUDE_FLAGS], + args: ["-p", prompt, ...CLAUDE_BASE_FLAGS, ...permissionFlags(grants)], options: { stdio: ["ignore", "pipe", "pipe"], windowsHide: true }, }; } @@ -102,6 +140,14 @@ export function buildStdinPromptInvocation(bin: string, flags: string[], prompt: } /** Claude variant of {@link buildStdinPromptInvocation} (same fixed flags as the argv path). */ -export function buildClaudeStdinInvocation(claudeBin: string, prompt: string): ClaudeInvocation { - return buildStdinPromptInvocation(claudeBin, ["-p", ...CLAUDE_FLAGS], prompt); +export function buildClaudeStdinInvocation( + claudeBin: string, + prompt: string, + grants?: ClaudeGrants, +): ClaudeInvocation { + return buildStdinPromptInvocation( + claudeBin, + ["-p", ...CLAUDE_BASE_FLAGS, ...permissionFlags(grants)], + prompt, + ); } diff --git a/src/hooks/wiki-worker.ts b/src/hooks/wiki-worker.ts index 637b85520..6e90e3103 100644 --- a/src/hooks/wiki-worker.ts +++ b/src/hooks/wiki-worker.ts @@ -325,7 +325,14 @@ async function main(): Promise { let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; try { - const inv = buildClaudeInvocation(cfg.claudeBin, prompt); + // tmpDir holds both the session JSONL to read and the summary to write, + // and lives outside the session cwd — grant it explicitly rather than + // relying on bypassPermissions, which an enterprise policy can disable + // (see ClaudeGrants in wiki-worker-spawn.ts). + const inv = buildClaudeInvocation(cfg.claudeBin, prompt, { + addDirs: [tmpDir], + allowedTools: ["Read", "Write"], + }); execFileSync(inv.file, inv.args, { ...inv.options, timeout: 120_000, @@ -355,6 +362,15 @@ async function main(): Promise { : "claude -p failed without producing a new summary; skipping upload"); return; } + // Exit 0 is not proof of work: a child that cannot reach tmpDir (or simply + // declines) exits 0 having written nothing, leaving the pre-seeded base + // summary in place. Uploading it unchanged would still advance the offset + // and slice those events away forever, which is how a session gets stuck + // as a header-only placeholder run after run. + if (!summaryChanged) { + wlog("claude -p exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + return; + } if (raw.trim()) { // Stamp the offset ourselves so the persisted summary is authoritative // and never depends on the LLM echoing the bookkeeping line. diff --git a/src/skillify/stage-memory.ts b/src/skillify/stage-memory.ts index 5b3013dee..81f8221de 100644 --- a/src/skillify/stage-memory.ts +++ b/src/skillify/stage-memory.ts @@ -18,10 +18,10 @@ import { spawn } from "node:child_process"; import { existsSync, mkdirSync, readFileSync, writeFileSync, unlinkSync } from "node:fs"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { WIKI_PROMPT_TEMPLATE } from "../hooks/spawn-wiki-worker.js"; -import { buildClaudeInvocation } from "../hooks/wiki-worker-spawn.js"; +import { buildClaudeInvocation, type ClaudeGrants } from "../hooks/wiki-worker-spawn.js"; import { resolveCliBin } from "../utils/resolve-cli-bin.js"; import { EmbedClient } from "../embeddings/client.js"; import { embeddingsDisabled } from "../embeddings/disable.js"; @@ -62,7 +62,12 @@ export interface StageOptions { * Injectable for tests; defaults to the real `claude -p` spawn. A real * agent writes the summary file named in the prompt as a side effect. */ - runAgent?: (claudeBin: string, prompt: string, timeoutMs: number) => Promise; + runAgent?: ( + claudeBin: string, + prompt: string, + timeoutMs: number, + grants?: ClaudeGrants, + ) => Promise; /** Embed `text` locally; null when unavailable. Injectable for tests. */ embed?: (text: string) => Promise; } @@ -124,12 +129,17 @@ export function planClaudeSpawn(inv: ReturnType): }; } -function runClaude(claudeBin: string, prompt: string, timeoutMs: number): Promise { +function runClaude( + claudeBin: string, + prompt: string, + timeoutMs: number, + grants?: ClaudeGrants, +): Promise { // Reuse the live wiki-worker's invocation builder so the prompt-as-arg vs // prompt-over-stdin (Windows `.cmd` shim) handling stays identical to the // proven SessionEnd path. A bare `spawn(bin, ["-p", prompt, ...])` cannot // launch a `.cmd` shim and would blow the command-line length on Windows. - const plan = planClaudeSpawn(buildClaudeInvocation(claudeBin, prompt)); + const plan = planClaudeSpawn(buildClaudeInvocation(claudeBin, prompt, grants)); return new Promise((resolve) => { const child = spawn(plan.file, plan.args, { stdio: plan.stdio, @@ -213,7 +223,13 @@ export async function stageSession(input: StageSessionInput, opts: StageOptions) } const runAgent = opts.runAgent ?? runClaude; - const ran = await runAgent(opts.claudeBin, prompt, opts.timeoutMs); + // The transcript and the staging dir both live outside the session cwd, so the + // agent needs them granted explicitly — bypassPermissions alone is silently + // ignored under an enterprise policy and every session stages as `no-summary`. + const ran = await runAgent(opts.claudeBin, prompt, opts.timeoutMs, { + addDirs: [dirname(input.jsonlPath), stagingDir], + allowedTools: ["Read", "Write"], + }); if (!existsSync(summaryPath)) { return { sessionId: key, ok: false, embedded: false, reason: ran ? "no-summary" : "claude-failed" }; } diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index 14648adb0..c22f239ce 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -290,8 +290,14 @@ describe("wiki-worker — happy path", () => { expect(calledArgs).toContain("--no-session-persistence"); expect(calledArgs).toContain("--model"); expect(calledArgs).toContain("haiku"); - expect(calledArgs).toContain("--permission-mode"); - expect(calledArgs).toContain("bypassPermissions"); + // The scratch dir is granted explicitly instead of relying on + // bypassPermissions, which an enterprise policy can disable. + expect(calledArgs).not.toContain("--permission-mode"); + expect(calledArgs).not.toContain("bypassPermissions"); + expect(calledArgs).toContain("--add-dir"); + expect(calledArgs).toContain("--allowedTools"); + expect(calledArgs).toContain("Read"); + expect(calledArgs).toContain("Write"); // Prompt template was expanded with real values const prompt = calledArgs[1]; @@ -378,6 +384,50 @@ describe("wiki-worker — happy path", () => { expect(releaseLockMock).toHaveBeenCalledWith("sid-worker"); }); + it("does NOT upload or advance the offset when claude -p exits 0 having written nothing", async () => { + // The customer-reported failure, reproduced at the seam that made it + // permanent. Under an enterprise policy that disables bypassPermissions the + // child cannot reach tmpDir, cannot prompt (print mode), and exits 0 with + // the summary file untouched — leaving the pre-seeded prior summary on disk. + // Treating that as success re-uploaded the placeholder verbatim AND stamped + // lastSummaryCount, slicing the unread events away forever, so every later + // run summarized nothing. Exit 0 is not proof of work. + mkFetch(undefined, 1, true, 14); + // Captured inside the mock: the worker rmSync's tmpDir on the way out, so + // the pre-seed can only be observed while the child is "running". + let preSeeded: string | null = null; + execFileSyncMock.mockImplementation((_bin: string, args: string[]) => { + const summaryPath = args[1].match(/SUMMARY=(\S+)/)![1]; + preSeeded = existsSync(summaryPath) ? readFileSync(summaryPath, "utf-8") : null; + return Buffer.from(""); // exit 0, wrote nothing + }); + await runWorker(); + + // The worker did pre-seed the prior summary (otherwise this test would + // pass for the wrong reason — an absent file, not an unchanged one). + expect(preSeeded).toContain("JSONL offset"); + expect(uploadSummaryMock).not.toHaveBeenCalled(); + expect(finalizeSummaryMock).not.toHaveBeenCalled(); + const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); + expect(log).toContain("left the pre-seeded summary unchanged"); + expect(releaseLockMock).toHaveBeenCalledWith("sid-worker"); + }); + + it("still uploads and advances the offset when claude -p rewrites the pre-seeded summary", async () => { + // The mirror of the guard above: a real run that DOES rewrite the file must + // keep uploading and stamping, so the fix cannot silently freeze summaries. + mkFetch(undefined, 1, true, 14); + execFileSyncMock.mockImplementation((_bin: string, args: string[]) => { + const summaryPath = args[1].match(/SUMMARY=(\S+)/)![1]; + writeFileSync(summaryPath, "# Session X\n\n## What Happened\nfresh body written by this run.\n"); + return Buffer.from(""); + }); + await runWorker(); + + expect(uploadSummaryMock).toHaveBeenCalledTimes(1); + expect(finalizeSummaryMock).toHaveBeenCalledWith("sid-worker", 14); + }); + it("defaults to /sessions/unknown/ when the path SELECT returns no rows", async () => { mkFetch(undefined, 0); execFileSyncMock.mockImplementation((_bin: string, args: string[]) => { diff --git a/tests/shared/claude-permission-grants.test.ts b/tests/shared/claude-permission-grants.test.ts new file mode 100644 index 000000000..42515d6a2 --- /dev/null +++ b/tests/shared/claude-permission-grants.test.ts @@ -0,0 +1,145 @@ +import { afterEach, describe, expect, it } from "vitest"; + +/** + * Contract for the explicit permission grants that replaced the blanket + * `--permission-mode bypassPermissions` on the summarizer's `claude -p` calls. + * + * Why the swap: an enterprise policy can set + * `"disableBypassPermissionsMode": "disable"`, after which the child ignores + * the bypass, falls back to normal permissioning, refuses every path outside + * the session cwd — which is exactly where the wiki worker's scratch dir and + * the backfill's transcript/staging dirs live — and, because print mode cannot + * prompt, exits 0 having written nothing. Naming the dirs and tools is both + * policy-proof and least-privilege. + * + * Two halves matter equally: grants must REPLACE the bypass when supplied, and + * the bypass must survive untouched for every caller that supplies none. + */ + +import { + buildClaudeInvocation, + buildClaudeStdinInvocation, + permissionFlags, +} from "../../src/hooks/wiki-worker-spawn.js"; + +const realPlatform = process.platform; +function setPlatform(p: NodeJS.Platform): void { + Object.defineProperty(process, "platform", { value: p, configurable: true }); +} +afterEach(() => { + Object.defineProperty(process, "platform", { value: realPlatform, configurable: true }); +}); + +describe("permissionFlags", () => { + it("falls back to the blanket bypass when no grants are named", () => { + expect(permissionFlags(undefined)).toEqual(["--permission-mode", "bypassPermissions"]); + }); + + it("emits one --add-dir per granted dir, then the allowed tools", () => { + expect(permissionFlags({ addDirs: ["/a", "/b"], allowedTools: ["Read", "Write"] })) + .toEqual(["--add-dir", "/a", "--add-dir", "/b", "--allowedTools", "Read", "Write"]); + }); + + it("accepts dirs without tools, and tools without dirs", () => { + expect(permissionFlags({ addDirs: ["/a"] })).toEqual(["--add-dir", "/a"]); + expect(permissionFlags({ allowedTools: ["Read"] })).toEqual(["--allowedTools", "Read"]); + }); + + it("falls back to the bypass rather than leaving the child with no grant at all", () => { + // An empty grants object is a caller bug; degrading to today's behaviour + // beats silently spawning a child that can reach nothing. + expect(permissionFlags({})).toEqual(["--permission-mode", "bypassPermissions"]); + expect(permissionFlags({ addDirs: [], allowedTools: [] })).toEqual(["--permission-mode", "bypassPermissions"]); + }); + + it("quotes granted paths only when asked (the Windows shell branch)", () => { + const dir = "C:\\Users\\First Last\\AppData\\Local\\Temp\\deeplake-wiki-1"; + expect(permissionFlags({ addDirs: [dir] }, false)).toEqual(["--add-dir", dir]); + expect(permissionFlags({ addDirs: [dir] }, true)).toEqual(["--add-dir", `"${dir}"`]); + }); +}); + +describe("buildClaudeInvocation — grants vs bypass", () => { + it("replaces the bypass with the grants on the POSIX argv path", () => { + setPlatform("linux"); + const inv = buildClaudeInvocation("/usr/local/bin/claude", "PROMPT", { + addDirs: ["/tmp/deeplake-wiki-1"], + allowedTools: ["Read", "Write"], + }); + expect(inv.args).toEqual([ + "-p", "PROMPT", + "--no-session-persistence", + "--model", "haiku", + "--add-dir", "/tmp/deeplake-wiki-1", + "--allowedTools", "Read", "Write", + ]); + expect(inv.args).not.toContain("bypassPermissions"); + }); + + it("leaves the bypass in place for a caller that names no grants", () => { + setPlatform("linux"); + const inv = buildClaudeInvocation("/usr/local/bin/claude", "PROMPT"); + expect(inv.args).toEqual([ + "-p", "PROMPT", + "--no-session-persistence", + "--model", "haiku", + "--permission-mode", "bypassPermissions", + ]); + }); + + it("quotes the granted dir on the Windows .cmd branch, where args are re-joined into a command line", () => { + // A Windows temp dir routinely contains spaces; unquoted it would split + // into two args and the grant would silently name the wrong directory. + setPlatform("win32"); + const dir = "C:\\Users\\First Last\\AppData\\Local\\Temp\\deeplake-wiki-1"; + const inv = buildClaudeInvocation("C:\\npm\\claude.cmd", "PROMPT", { + addDirs: [dir], + allowedTools: ["Read", "Write"], + }); + expect(inv.args).toEqual([ + "-p", + "--no-session-persistence", + "--model", "haiku", + "--add-dir", `"${dir}"`, + "--allowedTools", "Read", "Write", + ]); + // The prompt still rides stdin, never the command line. + expect(inv.args).not.toContain("PROMPT"); + expect(inv.options.input).toBe("PROMPT"); + }); + + it("keeps the bypass on the Windows .cmd branch when no grants are named", () => { + setPlatform("win32"); + const inv = buildClaudeInvocation("C:\\npm\\claude.cmd", "PROMPT"); + expect(inv.args).toEqual([ + "-p", + "--no-session-persistence", + "--model", "haiku", + "--permission-mode", "bypassPermissions", + ]); + }); +}); + +describe("buildClaudeStdinInvocation — grants vs bypass", () => { + it("carries the grants through the stdin variant", () => { + setPlatform("linux"); + const inv = buildClaudeStdinInvocation("/usr/local/bin/claude", "PROMPT", { + addDirs: ["/tmp/staging"], + allowedTools: ["Read", "Write"], + }); + expect(inv.args).toEqual([ + "-p", + "--no-session-persistence", + "--model", "haiku", + "--add-dir", "/tmp/staging", + "--allowedTools", "Read", "Write", + ]); + expect(inv.options.input).toBe("PROMPT"); + }); + + it("keeps the bypass in the stdin variant when no grants are named", () => { + setPlatform("linux"); + const inv = buildClaudeStdinInvocation("/usr/local/bin/claude", "PROMPT"); + expect(inv.args).toContain("bypassPermissions"); + }); +}); From 72cddefc655bfc0d1c2f9d881a23f8d5862bd076 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 05:44:58 +0000 Subject: [PATCH 2/9] fix(skillify): extend the explicit permission grants to the remaining out-of-cwd spawns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit covered the wiki worker and the memory backfill. Four other call sites spawn `claude -p --permission-mode bypassPermissions`; two of them hand the child a path outside the session cwd and so carry the same bug: - src/commands/mine-local.ts — the gate prompt names a verdict path under the per-session tmp dir. - src/skillify/gate-runner.ts — same, via the skillify worker's tmp dir. Both offer the model a stdout fallback, so they degrade rather than fail outright, but under a policy that disables bypassPermissions the Write-tool branch is dead and the verdict depends on the model choosing to print instead. Grant the dir by name, same mechanism (`permissionFlags` is now exported). The other two need no grant, and this is deliberate, not an oversight: - src/skillify/advisor.ts — prompt is inlined, verdict is parsed from stdout; no path is ever handed to the child. - src/docs/refresh-llm.ts — source content is inlined in the prompt and the generated markdown is read back from stdout; the codex branch's `--dangerously-bypass-approvals-and-sandbox` is a different CLI's flag, unaffected by the Claude enterprise policy. Non-claude agents keep their existing argv untouched: grants are Claude flags. Tests: gate-runner and mine-local argv assertions for both halves of the contract — grants supplied means the bypass is gone and the dir is named; no grants means the argv is byte-identical to today's. --- src/commands/mine-local.ts | 20 ++++++++++-- src/skillify/gate-runner.ts | 18 +++++++++-- src/skillify/skillify-worker.ts | 4 +++ .../mine-local-orchestrator.test.ts | 9 ++++++ .../claude-code/skillify-gate-runner.test.ts | 32 ++++++++++++++++++- 5 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/commands/mine-local.ts b/src/commands/mine-local.ts index b1113bbd1..8eb8e9d84 100644 --- a/src/commands/mine-local.ts +++ b/src/commands/mine-local.ts @@ -38,6 +38,7 @@ import { import { extractPairs, type Pair } from "../skillify/extractors/index.js"; import { findAgentBin, type Agent } from "../skillify/gate-runner.js"; import { extractJsonBlock } from "../skillify/gate-parser.js"; +import { permissionFlags, type ClaudeGrants } from "../hooks/wiki-worker-spawn.js"; import { resolveSkillsRoot, writeNewSkill, listSkills, parseFrontmatter } from "../skillify/skill-writer.js"; import { detectAgentSkillsRoots } from "../skillify/agent-roots.js"; import { fanOutSymlinks } from "../skillify/pull.js"; @@ -91,6 +92,12 @@ function runGateViaStdin(opts: { bin: string; prompt: string; timeoutMs: number; + /** + * Dirs/tools to grant explicitly instead of the blanket `bypassPermissions`, + * which an enterprise policy can disable. See ClaudeGrants in + * ../hooks/wiki-worker-spawn.ts. + */ + grants?: ClaudeGrants; }): Promise<{ stdout: string; stderr: string; errored: boolean; errorMessage?: string }> { return new Promise((resolve) => { if (opts.agent !== "claude_code") { @@ -116,7 +123,7 @@ function runGateViaStdin(opts: { "-p", "--no-session-persistence", "--model", "haiku", - "--permission-mode", "bypassPermissions", + ...permissionFlags(opts.grants), ]; const child = spawn(opts.bin, args, { stdio: ["pipe", "pipe", "pipe"], @@ -586,7 +593,16 @@ async function runMineLocalImpl(args: string[]): Promise { const prompt = buildSessionPrompt(tail, s, verdictPath); writeFileSync(join(sessionTmp, "prompt.txt"), prompt); - const gate = await runGateViaStdin({ agent: gateAgent, bin: gateBin, prompt, timeoutMs: GATE_TIMEOUT_MS }); + // sessionTmp holds the verdict path named in the prompt and lives outside + // the session cwd, so grant it explicitly rather than relying on the + // bypass an enterprise policy can disable. + const gate = await runGateViaStdin({ + agent: gateAgent, + bin: gateBin, + prompt, + timeoutMs: GATE_TIMEOUT_MS, + grants: { addDirs: [sessionTmp], allowedTools: ["Read", "Write"] }, + }); try { writeFileSync(join(sessionTmp, "gate-stdout.txt"), gate.stdout); if (gate.stderr) writeFileSync(join(sessionTmp, "gate-stderr.txt"), gate.stderr); diff --git a/src/skillify/gate-runner.ts b/src/skillify/gate-runner.ts index 8c60726ca..fa7704da5 100644 --- a/src/skillify/gate-runner.ts +++ b/src/skillify/gate-runner.ts @@ -6,7 +6,7 @@ * codex / cursor / hermes installed never needs `claude` in PATH. * * Per-agent invocation: - * claude_code → `claude -p --no-session-persistence --model haiku --permission-mode bypassPermissions` + * claude_code → `claude -p --no-session-persistence --model haiku ` * codex → `codex exec --dangerously-bypass-approvals-and-sandbox ` * cursor → `cursor-agent --print --model --force --output-format text ` * hermes → `hermes -z --provider -m --yolo --ignore-user-config` @@ -14,9 +14,15 @@ * The worker passes a verdict-write path inside the prompt; the runner * captures stdout regardless so the worker's stdout-fallback path still * works on agents whose models don't reliably use the Write tool. + * + * That verdict path lives outside the session cwd, so the claude_code caller + * names it via `grants` (`--add-dir` + `--allowedTools`). Callers that name no + * grants keep the blanket `--permission-mode bypassPermissions`. */ import { existsSync } from "node:fs"; + +import { permissionFlags, type ClaudeGrants } from "../hooks/wiki-worker-spawn.js"; import { createRequire } from "node:module"; // We need `child_process.execFileSync` to actually spawn the agent CLI for @@ -61,6 +67,14 @@ export interface GateRunOptions { piModel?: string; /** Max wall-clock for the CLI call; default 120s. */ timeoutMs?: number; + /** + * claude_code only — dirs/tools to grant explicitly instead of the blanket + * `bypassPermissions`, which an enterprise policy can disable. The gate + * prompt names a verdict path outside the session cwd, so a caller that + * wants the Write-tool branch to work under such a policy must grant that + * dir. See ClaudeGrants in ../hooks/wiki-worker-spawn.ts. + */ + grants?: ClaudeGrants; } export interface GateRunResult { @@ -153,7 +167,7 @@ export function buildArgs(agent: Agent, prompt: string, opts: GateRunOptions): s "-p", prompt, "--no-session-persistence", "--model", "haiku", - "--permission-mode", "bypassPermissions", + ...permissionFlags(opts.grants), ]; case "codex": return [ diff --git a/src/skillify/skillify-worker.ts b/src/skillify/skillify-worker.ts index 65363d699..28d98e6b9 100644 --- a/src/skillify/skillify-worker.ts +++ b/src/skillify/skillify-worker.ts @@ -399,6 +399,10 @@ async function main(): Promise { agent: gateAgent, prompt, bin: cfg.gateBin, + // The prompt offers the model a verdict path inside tmpDir, which lives + // outside the session cwd. Grant it explicitly so the Write-tool branch + // survives an enterprise policy that disables bypassPermissions. + grants: { addDirs: [tmpDir], allowedTools: ["Read", "Write"] }, cursorModel: cfg.cursorModel, hermesProvider: cfg.hermesProvider, hermesModel: cfg.hermesModel, diff --git a/tests/claude-code/mine-local-orchestrator.test.ts b/tests/claude-code/mine-local-orchestrator.test.ts index e3795769f..d4d05f59a 100644 --- a/tests/claude-code/mine-local-orchestrator.test.ts +++ b/tests/claude-code/mine-local-orchestrator.test.ts @@ -239,6 +239,15 @@ describe("runMineLocal: orchestrator branches", () => { await mod.runMineLocal([]); expect(spawnCalls).toHaveLength(1); + // The gate prompt offers the model a verdict path inside the per-session + // tmp dir, which is outside the session cwd. That dir must be granted by + // name: `bypassPermissions` is silently ignored under an enterprise policy + // (disableBypassPermissionsMode), and the child then refuses the path. + expect(spawnCalls[0].args).not.toContain("--permission-mode"); + expect(spawnCalls[0].args).not.toContain("bypassPermissions"); + expect(spawnCalls[0].args).toContain("--add-dir"); + expect(spawnCalls[0].args).toContain("--allowedTools"); + expect(spawnCalls[0].args.slice(spawnCalls[0].args.indexOf("--allowedTools"))).toEqual(["--allowedTools", "Read", "Write"]); expect(writeNewSkill).toHaveBeenCalledTimes(1); expect(writeNewSkill.mock.calls[0][0].name).toBe("useful-skill"); expect(fanOutSymlinks).toHaveBeenCalledTimes(1); diff --git a/tests/claude-code/skillify-gate-runner.test.ts b/tests/claude-code/skillify-gate-runner.test.ts index e0b7984f9..aacff5001 100644 --- a/tests/claude-code/skillify-gate-runner.test.ts +++ b/tests/claude-code/skillify-gate-runner.test.ts @@ -74,7 +74,7 @@ describe("runGate dispatch", () => { // produces. (Previously these spawned `/usr/bin/echo` and grepped stdout, // which only works on POSIX; buildArgs is the deterministic, cross-platform // seam for the same contract.) - it("constructs claude_code invocation with --model haiku + bypassPermissions", () => { + it("constructs claude_code invocation with --model haiku + bypassPermissions when no grants are named", () => { const args = buildArgs("claude_code", "PROMPT_MARKER", { agent: "claude_code", prompt: "PROMPT_MARKER" }); expect(args).toContain("PROMPT_MARKER"); expect(args).toContain("--model"); @@ -82,6 +82,36 @@ describe("runGate dispatch", () => { expect(args).toContain("bypassPermissions"); }); + it("swaps bypassPermissions for the named grants when the caller supplies them", () => { + // The worker names the tmp dir holding the verdict path. bypassPermissions + // is silently ignored under an enterprise policy that sets + // disableBypassPermissionsMode, so it must not be the only grant. + const args = buildArgs("claude_code", "PROMPT_MARKER", { + agent: "claude_code", + prompt: "PROMPT_MARKER", + grants: { addDirs: ["/tmp/skillify-x"], allowedTools: ["Read", "Write"] }, + }); + expect(args).not.toContain("--permission-mode"); + expect(args).not.toContain("bypassPermissions"); + expect(args).toEqual([ + "-p", "PROMPT_MARKER", + "--no-session-persistence", + "--model", "haiku", + "--add-dir", "/tmp/skillify-x", + "--allowedTools", "Read", "Write", + ]); + }); + + it("ignores grants for non-claude agents (their CLIs have no --add-dir)", () => { + const args = buildArgs("codex", "PROMPT_MARKER", { + agent: "codex", + prompt: "PROMPT_MARKER", + grants: { addDirs: ["/tmp/skillify-x"], allowedTools: ["Read", "Write"] }, + }); + expect(args).not.toContain("--add-dir"); + expect(args).toContain("--dangerously-bypass-approvals-and-sandbox"); + }); + it("constructs codex invocation with exec + --dangerously-bypass-approvals-and-sandbox", () => { const args = buildArgs("codex", "PROMPT_MARKER", { agent: "codex", prompt: "PROMPT_MARKER" }); expect(args).toContain("PROMPT_MARKER"); From f4b13088d9f71bb47c3f81eee7591ad886620fdd Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 05:45:08 +0000 Subject: [PATCH 3/9] test: drop the customer reference from the regression-test comment --- tests/claude-code/wiki-worker.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index c22f239ce..aaf8b49e5 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -385,8 +385,7 @@ describe("wiki-worker — happy path", () => { }); it("does NOT upload or advance the offset when claude -p exits 0 having written nothing", async () => { - // The customer-reported failure, reproduced at the seam that made it - // permanent. Under an enterprise policy that disables bypassPermissions the + // The field failure, reproduced at the seam that made it permanent. Under an enterprise policy that disables bypassPermissions the // child cannot reach tmpDir, cannot prompt (print mode), and exits 0 with // the summary file untouched — leaving the pre-seeded prior summary on disk. // Treating that as success re-uploaded the placeholder verbatim AND stamped From b21b2e1f66d88b684bab7dd01f43990d8cdb02ff Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 05:54:37 +0000 Subject: [PATCH 4/9] test: pin the granted directory values, not just the flag names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Presence-only assertions (`toContain("--add-dir")`) pass just as happily on a grant that names the wrong directory — which would leave the child exactly as unable to reach the scratch dir as the bypass it replaced. Pin the argv tail instead: the granted dir must be the worker's own tmpDir, and mine-local's must be THIS session's tmp dir (the one holding the verdict path named in the prompt), each followed by exactly `--allowedTools Read Write`. Both assertions verified to go red when the granted dir is swapped for a neighbouring path. Raised by CodeRabbit on the PR. --- tests/claude-code/mine-local-orchestrator.test.ts | 9 ++++++--- tests/claude-code/wiki-worker.test.ts | 11 +++++++---- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/claude-code/mine-local-orchestrator.test.ts b/tests/claude-code/mine-local-orchestrator.test.ts index d4d05f59a..af56dc4b3 100644 --- a/tests/claude-code/mine-local-orchestrator.test.ts +++ b/tests/claude-code/mine-local-orchestrator.test.ts @@ -245,9 +245,12 @@ describe("runMineLocal: orchestrator branches", () => { // (disableBypassPermissionsMode), and the child then refuses the path. expect(spawnCalls[0].args).not.toContain("--permission-mode"); expect(spawnCalls[0].args).not.toContain("bypassPermissions"); - expect(spawnCalls[0].args).toContain("--add-dir"); - expect(spawnCalls[0].args).toContain("--allowedTools"); - expect(spawnCalls[0].args.slice(spawnCalls[0].args.indexOf("--allowedTools"))).toEqual(["--allowedTools", "Read", "Write"]); + // Pin the granted VALUES: the dir must be THIS session's tmp dir (the one + // holding the verdict path named in the prompt), not merely some dir. + const addDirAt = spawnCalls[0].args.indexOf("--add-dir"); + expect(addDirAt).toBeGreaterThan(-1); + expect(spawnCalls[0].args[addDirAt + 1]).toMatch(/[/\\]mine-local-\d+[/\\]s-sess-aaa$/); + expect(spawnCalls[0].args.slice(addDirAt + 2)).toEqual(["--allowedTools", "Read", "Write"]); expect(writeNewSkill).toHaveBeenCalledTimes(1); expect(writeNewSkill.mock.calls[0][0].name).toBe("useful-skill"); expect(fanOutSymlinks).toHaveBeenCalledTimes(1); diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index aaf8b49e5..f1b2d9305 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -294,10 +294,13 @@ describe("wiki-worker — happy path", () => { // bypassPermissions, which an enterprise policy can disable. expect(calledArgs).not.toContain("--permission-mode"); expect(calledArgs).not.toContain("bypassPermissions"); - expect(calledArgs).toContain("--add-dir"); - expect(calledArgs).toContain("--allowedTools"); - expect(calledArgs).toContain("Read"); - expect(calledArgs).toContain("Write"); + // Pin the granted VALUES, not just the flag names: a grant naming the + // wrong directory reads identically to a correct one under a presence-only + // assertion, and would leave the child just as unable to reach tmpDir. + expect(calledArgs.slice(calledArgs.indexOf("--add-dir"))).toEqual([ + "--add-dir", tmpDir, + "--allowedTools", "Read", "Write", + ]); // Prompt template was expanded with real values const prompt = calledArgs[1]; From 3e0bea1d71983436fbcbc25c303b05129a5f5010 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 18:43:45 +0000 Subject: [PATCH 5/9] fix(wiki): apply the exit-0 guard to the codex, cursor, hermes and pi workers too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each harness ships its own ~330-380 line wiki-worker, and all four carried a verbatim copy of the bug the previous commits fixed in the claude worker: they compute `summaryChanged` but consult it only when the child exits NON-zero. An agent CLI that exits 0 without writing therefore re-uploads the pre-seeded placeholder AND stamps `lastSummaryCount`, slicing the unread events away forever — the session is stuck as a header-only stub run after run. The permission half of the fix does not apply here: these workers spawn their own CLI (codex / cursor-agent / hermes / pi) with that CLI's own bypass flag, none of which is governed by the Claude enterprise policy. Only the exit-0-is-not-proof-of-work half is shared, so only that is ported. Each worker gets the same guard and a regression test asserting no upload and no offset advance on an exit-0-wrote-nothing run. All four tests were verified to fail with the guard removed. --- src/hooks/codex/wiki-worker.ts | 9 +++++++ src/hooks/cursor/wiki-worker.ts | 9 +++++++ src/hooks/hermes/wiki-worker.ts | 9 +++++++ src/hooks/pi/wiki-worker.ts | 9 +++++++ tests/codex/codex-wiki-worker.test.ts | 16 +++++++++++++ tests/cursor/cursor-wiki-worker.test.ts | 32 +++++++++++++++++++++++++ tests/hermes/hermes-wiki-worker.test.ts | 32 +++++++++++++++++++++++++ tests/pi/pi-wiki-worker.test.ts | 32 +++++++++++++++++++++++++ 8 files changed, 148 insertions(+) diff --git a/src/hooks/codex/wiki-worker.ts b/src/hooks/codex/wiki-worker.ts index ff2745874..9fba645e8 100644 --- a/src/hooks/codex/wiki-worker.ts +++ b/src/hooks/codex/wiki-worker.ts @@ -271,6 +271,15 @@ async function main(): Promise { : "codex exec failed without producing a new summary; skipping upload"); return; } + // Exit 0 is not proof of work: a child that cannot reach tmpDir (or simply + // declines) exits 0 having written nothing, leaving the pre-seeded base + // summary in place. Uploading it unchanged would still advance the offset + // and slice those events away forever, which is how a session gets stuck + // as a header-only placeholder run after run. + if (!summaryChanged) { + wlog("codex exec exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + return; + } if (raw.trim()) { // Stamp the offset ourselves so the persisted summary is authoritative // and never depends on the LLM echoing the bookkeeping line. diff --git a/src/hooks/cursor/wiki-worker.ts b/src/hooks/cursor/wiki-worker.ts index c7a03c27b..9334b6845 100644 --- a/src/hooks/cursor/wiki-worker.ts +++ b/src/hooks/cursor/wiki-worker.ts @@ -312,6 +312,15 @@ async function main(): Promise { : "cursor-agent --print failed without producing a new summary; skipping upload"); return; } + // Exit 0 is not proof of work: a child that cannot reach tmpDir (or simply + // declines) exits 0 having written nothing, leaving the pre-seeded base + // summary in place. Uploading it unchanged would still advance the offset + // and slice those events away forever, which is how a session gets stuck + // as a header-only placeholder run after run. + if (!summaryChanged) { + wlog("cursor-agent --print exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + return; + } if (raw.trim()) { // Stamp the offset ourselves so the persisted summary is authoritative // and never depends on the LLM echoing the bookkeeping line. diff --git a/src/hooks/hermes/wiki-worker.ts b/src/hooks/hermes/wiki-worker.ts index 335348ca5..3c793b8fc 100644 --- a/src/hooks/hermes/wiki-worker.ts +++ b/src/hooks/hermes/wiki-worker.ts @@ -324,6 +324,15 @@ async function main(): Promise { : "hermes -z failed without producing a new summary; skipping upload"); return; } + // Exit 0 is not proof of work: a child that cannot reach tmpDir (or simply + // declines) exits 0 having written nothing, leaving the pre-seeded base + // summary in place. Uploading it unchanged would still advance the offset + // and slice those events away forever, which is how a session gets stuck + // as a header-only placeholder run after run. + if (!summaryChanged) { + wlog("hermes -z exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + return; + } if (raw.trim()) { // Stamp the offset ourselves so the persisted summary is authoritative // and never depends on the LLM echoing the bookkeeping line. diff --git a/src/hooks/pi/wiki-worker.ts b/src/hooks/pi/wiki-worker.ts index 8b03fb66f..a5a229cc4 100644 --- a/src/hooks/pi/wiki-worker.ts +++ b/src/hooks/pi/wiki-worker.ts @@ -263,6 +263,15 @@ async function main(): Promise { : "pi --print failed without producing a new summary; skipping upload"); return; } + // Exit 0 is not proof of work: a child that cannot reach tmpDir (or simply + // declines) exits 0 having written nothing, leaving the pre-seeded base + // summary in place. Uploading it unchanged would still advance the offset + // and slice those events away forever, which is how a session gets stuck + // as a header-only placeholder run after run. + if (!summaryChanged) { + wlog("pi --print exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + return; + } if (raw.trim()) { // Stamp the offset ourselves so the persisted summary is authoritative // and never depends on the LLM echoing the bookkeeping line. diff --git a/tests/codex/codex-wiki-worker.test.ts b/tests/codex/codex-wiki-worker.test.ts index 13ce5b4a6..c5450a3a7 100644 --- a/tests/codex/codex-wiki-worker.test.ts +++ b/tests/codex/codex-wiki-worker.test.ts @@ -270,6 +270,22 @@ describe("codex wiki-worker — happy path", () => { expect(releaseLockMock).toHaveBeenCalledWith("sid-codex"); }); + it("does NOT upload or advance the offset when codex exec exits 0 having written nothing", async () => { + // Exit 0 is not proof of work. A child that cannot reach tmpDir (or simply + // declines) exits 0 with the summary file untouched, leaving the pre-seeded + // prior summary in place. Treating that as success re-uploaded the + // placeholder verbatim AND stamped lastSummaryCount, slicing the unread + // events away forever — so every later run summarized nothing. + mkFetch(1, true, 9); + execFileSyncMock.mockImplementation(() => Buffer.from("")); + await runWorker(); + expect(uploadSummaryMock).not.toHaveBeenCalled(); + expect(finalizeSummaryMock).not.toHaveBeenCalled(); + const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); + expect(log).toContain("left the pre-seeded summary unchanged"); + expect(releaseLockMock).toHaveBeenCalledWith("sid-codex"); + }); + it("ignores a stale sidecar offset when no existing summary was loaded", async () => { // Sidecar says 3 processed, but SELECT summary returns nothing (row gone). // Without a base summary the offset is meaningless — regenerate from scratch diff --git a/tests/cursor/cursor-wiki-worker.test.ts b/tests/cursor/cursor-wiki-worker.test.ts index 11eea2eae..1127df673 100644 --- a/tests/cursor/cursor-wiki-worker.test.ts +++ b/tests/cursor/cursor-wiki-worker.test.ts @@ -174,6 +174,38 @@ describe("cursor wiki-worker — behavior", () => { expect(releaseLockMock).toHaveBeenCalledWith("sid-cursor"); }); + it("does NOT upload or advance the offset when the agent exits 0 having written nothing", async () => { + // Exit 0 is not proof of work. A child that cannot reach tmpDir (or simply + // declines) exits 0 with the summary file untouched, leaving the pre-seeded + // prior summary in place. Treating that as success re-uploaded the + // placeholder verbatim AND stamped lastSummaryCount, slicing the unread + // events away forever — so every later run summarized nothing. + fetchMock.mockImplementation(async (_u: string, init: any) => { + const sql = JSON.parse(init.body).query as string; + if (sql.startsWith("SELECT count(*) AS n")) return jsonResp({ columns: ["n"], rows: [[9]] }); + if (sql.startsWith("SELECT message, creation_date")) { + return jsonResp({ + columns: ["message", "creation_date"], + rows: Array.from({ length: 9 }, (_, i) => [JSON.stringify({ type: "user_message", content: `hello cursor ${i}` }), "2026-04-20T00:00:00Z"]), + }); + } + if (sql.startsWith("SELECT DISTINCT path")) { + return jsonResp({ columns: ["path"], rows: [["/sessions/alice/alice_org_default_sid-cursor.jsonl"]] }); + } + if (sql.startsWith("SELECT summary FROM")) { + return jsonResp({ columns: ["summary"], rows: [["# Session X\n- **JSONL offset**: 7\n\n## What Happened\nprior"]] }); + } + throw new Error(`unexpected query: ${sql}`); + }); + execFileSyncMock.mockImplementation(() => Buffer.from("")); + await runWorker(); + expect(uploadSummaryMock).not.toHaveBeenCalled(); + expect(finalizeSummaryMock).not.toHaveBeenCalled(); + const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); + expect(log).toContain("left the pre-seeded summary unchanged"); + expect(releaseLockMock).toHaveBeenCalledWith("sid-cursor"); + }); + it("reads events from the local cache and issues NO self-session SELECTs", async () => { readCacheMock.mockReturnValue( Array.from({ length: 4 }, (_, i) => JSON.stringify({ type: "user_message", content: `cache ${i}` })), diff --git a/tests/hermes/hermes-wiki-worker.test.ts b/tests/hermes/hermes-wiki-worker.test.ts index c0bbf729a..f68f5df9e 100644 --- a/tests/hermes/hermes-wiki-worker.test.ts +++ b/tests/hermes/hermes-wiki-worker.test.ts @@ -180,6 +180,38 @@ describe("hermes wiki-worker — behavior", () => { expect(releaseLockMock).toHaveBeenCalledWith("sid-hermes"); }); + it("does NOT upload or advance the offset when the agent exits 0 having written nothing", async () => { + // Exit 0 is not proof of work. A child that cannot reach tmpDir (or simply + // declines) exits 0 with the summary file untouched, leaving the pre-seeded + // prior summary in place. Treating that as success re-uploaded the + // placeholder verbatim AND stamped lastSummaryCount, slicing the unread + // events away forever — so every later run summarized nothing. + fetchMock.mockImplementation(async (_u: string, init: any) => { + const sql = JSON.parse(init.body).query as string; + if (sql.startsWith("SELECT count(*) AS n")) return jsonResp({ columns: ["n"], rows: [[9]] }); + if (sql.startsWith("SELECT message, creation_date")) { + return jsonResp({ + columns: ["message", "creation_date"], + rows: Array.from({ length: 9 }, (_, i) => [JSON.stringify({ type: "user_message", content: `hello hermes ${i}` }), "2026-04-20T00:00:00Z"]), + }); + } + if (sql.startsWith("SELECT DISTINCT path")) { + return jsonResp({ columns: ["path"], rows: [["/sessions/alice/alice_org_default_sid-hermes.jsonl"]] }); + } + if (sql.startsWith("SELECT summary FROM")) { + return jsonResp({ columns: ["summary"], rows: [["# Session X\n- **JSONL offset**: 7\n\n## What Happened\nprior"]] }); + } + throw new Error(`unexpected query: ${sql}`); + }); + execFileSyncMock.mockImplementation(() => Buffer.from("")); + await runWorker(); + expect(uploadSummaryMock).not.toHaveBeenCalled(); + expect(finalizeSummaryMock).not.toHaveBeenCalled(); + const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); + expect(log).toContain("left the pre-seeded summary unchanged"); + expect(releaseLockMock).toHaveBeenCalledWith("sid-hermes"); + }); + it("reads events from the local cache and issues NO self-session SELECTs", async () => { readCacheMock.mockReturnValue( Array.from({ length: 4 }, (_, i) => JSON.stringify({ type: "user_message", content: `cache ${i}` })), diff --git a/tests/pi/pi-wiki-worker.test.ts b/tests/pi/pi-wiki-worker.test.ts index 8aaf85b73..f2c26defa 100644 --- a/tests/pi/pi-wiki-worker.test.ts +++ b/tests/pi/pi-wiki-worker.test.ts @@ -169,6 +169,38 @@ describe("pi wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(releaseLockMock).toHaveBeenCalledWith("sid-pi"); }); + + it("does NOT upload or advance the offset when the agent exits 0 having written nothing", async () => { + // Exit 0 is not proof of work. A child that cannot reach tmpDir (or simply + // declines) exits 0 with the summary file untouched, leaving the pre-seeded + // prior summary in place. Treating that as success re-uploaded the + // placeholder verbatim AND stamped lastSummaryCount, slicing the unread + // events away forever — so every later run summarized nothing. + fetchMock.mockImplementation(async (_u: string, init: any) => { + const sql = JSON.parse(init.body).query as string; + if (sql.startsWith("SELECT count(*) AS n")) return jsonResp({ columns: ["n"], rows: [[9]] }); + if (sql.startsWith("SELECT message, creation_date")) { + return jsonResp({ + columns: ["message", "creation_date"], + rows: Array.from({ length: 9 }, (_, i) => [JSON.stringify({ type: "user_message", content: `hello pi ${i}` }), "2026-04-20T00:00:00Z"]), + }); + } + if (sql.startsWith("SELECT DISTINCT path")) { + return jsonResp({ columns: ["path"], rows: [["/sessions/alice/alice_org_default_sid-pi.jsonl"]] }); + } + if (sql.startsWith("SELECT summary FROM")) { + return jsonResp({ columns: ["summary"], rows: [["# Session X\n- **JSONL offset**: 7\n\n## What Happened\nprior"]] }); + } + throw new Error(`unexpected query: ${sql}`); + }); + execFileSyncMock.mockImplementation(() => Buffer.from("")); + await runWorker(); + expect(uploadSummaryMock).not.toHaveBeenCalled(); + expect(finalizeSummaryMock).not.toHaveBeenCalled(); + const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); + expect(log).toContain("left the pre-seeded summary unchanged"); + expect(releaseLockMock).toHaveBeenCalledWith("sid-pi"); + }); }); const promptOf = (a: string[]) => a.find((x) => typeof x === "string" && x.includes("SUMMARY="))!; From 0eddb2f869c809cfac074cdef4ed7583cd48510b Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 19:01:43 +0000 Subject: [PATCH 6/9] fix(wiki): skip only when the agent never WROTE, not merely when bytes match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review caught a regression this branch introduced: comparing content alone cannot tell "exited 0 having written nothing" from "correctly regenerated byte-identical text". An agent that legitimately rewrites the same summary would have been treated as a no-op, so the offset would never advance and those rows would be re-summarized on every future run, forever. Check the file's mtime alongside its content: skip only when the run neither changed the bytes NOR touched the file. That is exactly the "wrote nothing" condition the guard is for. Applied to all five workers. Tests, both verified to fail against the weaker implementation: - an identical-content rewrite must still upload and advance the offset; - stageSession must pass the transcript dir + staging dir with Read/Write — the existing tests inject a runAgent that ignores its grants argument, so dropping the grants entirely would have left them green. --- src/hooks/codex/wiki-worker.ts | 11 ++++++++--- src/hooks/cursor/wiki-worker.ts | 11 ++++++++--- src/hooks/hermes/wiki-worker.ts | 11 ++++++++--- src/hooks/pi/wiki-worker.ts | 11 ++++++++--- src/hooks/wiki-worker.ts | 11 ++++++++--- tests/claude-code/stage-memory.test.ts | 23 ++++++++++++++++++++++- tests/claude-code/wiki-worker.test.ts | 24 ++++++++++++++++++++++-- tests/codex/codex-wiki-worker.test.ts | 2 +- tests/cursor/cursor-wiki-worker.test.ts | 2 +- tests/hermes/hermes-wiki-worker.test.ts | 2 +- tests/pi/pi-wiki-worker.test.ts | 2 +- 11 files changed, 88 insertions(+), 22 deletions(-) diff --git a/src/hooks/codex/wiki-worker.ts b/src/hooks/codex/wiki-worker.ts index 9fba645e8..29453ea68 100644 --- a/src/hooks/codex/wiki-worker.ts +++ b/src/hooks/codex/wiki-worker.ts @@ -7,7 +7,7 @@ * Invoked by stop.ts as: node wiki-worker.js */ -import { readFileSync, writeFileSync, existsSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -239,6 +239,7 @@ async function main(): Promise { wlog("running codex exec"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { const inv = buildTrailingPromptInvocation(cfg.codexBin, [ "exec", @@ -276,8 +277,12 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - if (!summaryChanged) { - wlog("codex exec exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + // mtime, not just content: an agent that legitimately regenerates the + // SAME text still touched the file, and skipping that would freeze the + // offset and re-summarize those rows on every future run. Only a run + // that neither changed the bytes NOR touched the file wrote nothing. + if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + wlog("codex exec exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } if (raw.trim()) { diff --git a/src/hooks/cursor/wiki-worker.ts b/src/hooks/cursor/wiki-worker.ts index 9334b6845..7262edb0f 100644 --- a/src/hooks/cursor/wiki-worker.ts +++ b/src/hooks/cursor/wiki-worker.ts @@ -12,7 +12,7 @@ * differs: codex shells `codex exec`, we shell `cursor-agent --print --model X`. */ -import { readFileSync, writeFileSync, existsSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -274,6 +274,7 @@ async function main(): Promise { wlog(`running cursor-agent --print (model=${cfg.cursorModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // cursor-agent --print is the non-interactive headless mode. --force // auto-allows tools (matches the bypass-approvals semantic codex used). @@ -317,8 +318,12 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - if (!summaryChanged) { - wlog("cursor-agent --print exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + // mtime, not just content: an agent that legitimately regenerates the + // SAME text still touched the file, and skipping that would freeze the + // offset and re-summarize those rows on every future run. Only a run + // that neither changed the bytes NOR touched the file wrote nothing. + if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + wlog("cursor-agent --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } if (raw.trim()) { diff --git a/src/hooks/hermes/wiki-worker.ts b/src/hooks/hermes/wiki-worker.ts index 3c793b8fc..3353b9fa2 100644 --- a/src/hooks/hermes/wiki-worker.ts +++ b/src/hooks/hermes/wiki-worker.ts @@ -12,7 +12,7 @@ * differs: codex shells `codex exec`, we shell `hermes -z --provider X -m Y`. */ -import { readFileSync, writeFileSync, existsSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -276,6 +276,7 @@ async function main(): Promise { wlog(`running hermes -z (provider=${cfg.hermesProvider}, model=${cfg.hermesModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // hermes -z (--oneshot) is the non-interactive mode. --yolo // auto-approves tool use within the spawned hermes process. @@ -329,8 +330,12 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - if (!summaryChanged) { - wlog("hermes -z exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + // mtime, not just content: an agent that legitimately regenerates the + // SAME text still touched the file, and skipping that would freeze the + // offset and re-summarize those rows on every future run. Only a run + // that neither changed the bytes NOR touched the file wrote nothing. + if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + wlog("hermes -z exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } if (raw.trim()) { diff --git a/src/hooks/pi/wiki-worker.ts b/src/hooks/pi/wiki-worker.ts index a5a229cc4..0eadf0340 100644 --- a/src/hooks/pi/wiki-worker.ts +++ b/src/hooks/pi/wiki-worker.ts @@ -16,7 +16,7 @@ * we shell `pi --print --provider

--model `. Same query/upload paths. */ -import { readFileSync, writeFileSync, existsSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -224,6 +224,7 @@ async function main(): Promise { wlog(`running pi --print (provider=${cfg.piProvider}, model=${cfg.piModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // pi --print is the non-interactive mode; it bypasses extension // discovery (modes/print-mode.js doesn't import ExtensionRunner), @@ -268,8 +269,12 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - if (!summaryChanged) { - wlog("pi --print exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + // mtime, not just content: an agent that legitimately regenerates the + // SAME text still touched the file, and skipping that would freeze the + // offset and re-summarize those rows on every future run. Only a run + // that neither changed the bytes NOR touched the file wrote nothing. + if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + wlog("pi --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } if (raw.trim()) { diff --git a/src/hooks/wiki-worker.ts b/src/hooks/wiki-worker.ts index 6e90e3103..d63c667ec 100644 --- a/src/hooks/wiki-worker.ts +++ b/src/hooks/wiki-worker.ts @@ -7,7 +7,7 @@ * Invoked by session-end.ts as: node wiki-worker.js */ -import { readFileSync, writeFileSync, existsSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildClaudeInvocation } from "./wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -324,6 +324,7 @@ async function main(): Promise { wlog("running claude -p"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // tmpDir holds both the session JSONL to read and the summary to write, // and lives outside the session cwd — grant it explicitly rather than @@ -367,8 +368,12 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - if (!summaryChanged) { - wlog("claude -p exited 0 but left the pre-seeded summary unchanged; skipping upload to avoid advancing the offset"); + // mtime, not just content: an agent that legitimately regenerates the + // SAME text still touched the file, and skipping that would freeze the + // offset and re-summarize those rows on every future run. Only a run + // that neither changed the bytes NOR touched the file wrote nothing. + if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + wlog("claude -p exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } if (raw.trim()) { diff --git a/tests/claude-code/stage-memory.test.ts b/tests/claude-code/stage-memory.test.ts index c2f86fa4c..c07095755 100644 --- a/tests/claude-code/stage-memory.test.ts +++ b/tests/claude-code/stage-memory.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { mkdtempSync, writeFileSync, readFileSync, existsSync, rmSync, chmodSync, mkdirSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { stageSession, resolveClaudeBin, planClaudeSpawn, type StageOptions } from "../../src/skillify/stage-memory.js"; import { readPendingMemoryManifest } from "../../src/skillify/pending-memory-manifest.js"; @@ -48,6 +48,27 @@ function opts(over: Partial = {}): StageOptions { } describe("stageSession", () => { + it("grants the transcript dir and the staging dir explicitly, with Read+Write", async () => { + // The backfill hands the agent two paths outside the session cwd. Without + // an explicit grant those are refused under an enterprise policy that + // disables bypassPermissions, and every session stages as `no-summary`. + // The other tests inject a runAgent that ignores its grants argument, so + // dropping the grants entirely would leave them green — this one pins it. + let seen: any; + await stageSession(input(), opts({ + runAgent: async (_bin, prompt, _timeout, grants) => { + seen = grants; + const m = prompt.match(/SUMMARY FILE to write: (\S+)/); + if (m) writeFileSync(m[1], "# Session s1\n## What Happened\nreal content\n"); + return true; + }, + })); + expect(seen).toBeDefined(); + expect(seen.allowedTools).toEqual(["Read", "Write"]); + expect(seen.addDirs).toContain(dirname(jsonlPath)); + expect(seen.addDirs).toContain(stagingDir); + }); + it("stages summary + manifest row on success", async () => { const r = await stageSession(input(), opts()); expect(r).toMatchObject({ ok: true, embedded: false }); diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index f1b2d9305..ecfbd4f50 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync, utimesSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -411,10 +411,30 @@ describe("wiki-worker — happy path", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("left the pre-seeded summary unchanged"); + expect(log).toContain("never wrote the summary"); expect(releaseLockMock).toHaveBeenCalledWith("sid-worker"); }); + it("still uploads when the agent legitimately rewrites the summary to the SAME bytes", async () => { + // Content equality alone cannot tell "wrote nothing" from "correctly + // regenerated identical text". Treating the second as the first would + // freeze the offset and re-summarize those rows on every future run, so + // the guard also checks whether the file was touched. + mkFetch(undefined, 1, true, 14); + execFileSyncMock.mockImplementation((_bin: string, args: string[]) => { + const summaryPath = args[1].match(/SUMMARY=(\S+)/)![1]; + const identical = readFileSync(summaryPath, "utf-8"); + // Ensure the mtime moves even on a fast filesystem clock. + utimesSync(summaryPath, new Date(Date.now() + 5000), new Date(Date.now() + 5000)); + writeFileSync(summaryPath, identical); + utimesSync(summaryPath, new Date(Date.now() + 5000), new Date(Date.now() + 5000)); + return Buffer.from(""); + }); + await runWorker(); + expect(uploadSummaryMock).toHaveBeenCalledTimes(1); + expect(finalizeSummaryMock).toHaveBeenCalledWith("sid-worker", 14); + }); + it("still uploads and advances the offset when claude -p rewrites the pre-seeded summary", async () => { // The mirror of the guard above: a real run that DOES rewrite the file must // keep uploading and stamping, so the fix cannot silently freeze summaries. diff --git a/tests/codex/codex-wiki-worker.test.ts b/tests/codex/codex-wiki-worker.test.ts index c5450a3a7..896bb41a9 100644 --- a/tests/codex/codex-wiki-worker.test.ts +++ b/tests/codex/codex-wiki-worker.test.ts @@ -282,7 +282,7 @@ describe("codex wiki-worker — happy path", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("left the pre-seeded summary unchanged"); + expect(log).toContain("never wrote the summary"); expect(releaseLockMock).toHaveBeenCalledWith("sid-codex"); }); diff --git a/tests/cursor/cursor-wiki-worker.test.ts b/tests/cursor/cursor-wiki-worker.test.ts index 1127df673..73ae7a2f3 100644 --- a/tests/cursor/cursor-wiki-worker.test.ts +++ b/tests/cursor/cursor-wiki-worker.test.ts @@ -202,7 +202,7 @@ describe("cursor wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("left the pre-seeded summary unchanged"); + expect(log).toContain("never wrote the summary"); expect(releaseLockMock).toHaveBeenCalledWith("sid-cursor"); }); diff --git a/tests/hermes/hermes-wiki-worker.test.ts b/tests/hermes/hermes-wiki-worker.test.ts index f68f5df9e..f27df2e30 100644 --- a/tests/hermes/hermes-wiki-worker.test.ts +++ b/tests/hermes/hermes-wiki-worker.test.ts @@ -208,7 +208,7 @@ describe("hermes wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("left the pre-seeded summary unchanged"); + expect(log).toContain("never wrote the summary"); expect(releaseLockMock).toHaveBeenCalledWith("sid-hermes"); }); diff --git a/tests/pi/pi-wiki-worker.test.ts b/tests/pi/pi-wiki-worker.test.ts index f2c26defa..87e267e6a 100644 --- a/tests/pi/pi-wiki-worker.test.ts +++ b/tests/pi/pi-wiki-worker.test.ts @@ -198,7 +198,7 @@ describe("pi wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("left the pre-seeded summary unchanged"); + expect(log).toContain("never wrote the summary"); expect(releaseLockMock).toHaveBeenCalledWith("sid-pi"); }); }); From a9cce8468c2d64b8cdc1d92cae99f4ff30e98e97 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 19:05:54 +0000 Subject: [PATCH 7/9] fix(wiki): backdate the pre-seeded summary so "never written" cannot be misread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pushback, correctly: comparing mtime against "just now" leaves a hole. On a coarse-resolution filesystem (FAT's 2s granularity, say) an agent that rewrites the summary to identical bytes inside a single clock tick keeps the timestamp, so the guard reads it as "never wrote" and freezes the offset — exactly the regression the mtime check was added to prevent. Stamp the pre-seeded file a minute into the past before handing it to the child. Any write the child makes lands far outside any plausible filesystem granularity, so an unchanged timestamp now genuinely means nothing was written. Applied to all five workers. The identical-rewrite test no longer forces the mtime forward — that dodged the very case under test. It now performs a plain same-bytes rewrite, and was verified to fail BOTH without the backdating (the coarse-clock hole) and with a content-only guard (the original regression). --- src/hooks/codex/wiki-worker.ts | 9 ++++++++- src/hooks/cursor/wiki-worker.ts | 9 ++++++++- src/hooks/hermes/wiki-worker.ts | 9 ++++++++- src/hooks/pi/wiki-worker.ts | 9 ++++++++- src/hooks/wiki-worker.ts | 9 ++++++++- tests/claude-code/wiki-worker.test.ts | 11 +++++------ 6 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/hooks/codex/wiki-worker.ts b/src/hooks/codex/wiki-worker.ts index 29453ea68..50e49df1b 100644 --- a/src/hooks/codex/wiki-worker.ts +++ b/src/hooks/codex/wiki-worker.ts @@ -7,7 +7,7 @@ * Invoked by stop.ts as: node wiki-worker.js */ -import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, utimesSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -239,6 +239,13 @@ async function main(): Promise { wlog("running codex exec"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + // Backdate the pre-seeded file before handing it to the child. Comparing + // mtime against "a minute ago" instead of "just now" closes the only hole + // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, + // say) a same-tick rewrite would otherwise keep the timestamp and read as + // "never written". Any write by the child lands far outside that window. + const MTIME_SENTINEL = new Date(Date.now() - 60_000); + if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { const inv = buildTrailingPromptInvocation(cfg.codexBin, [ diff --git a/src/hooks/cursor/wiki-worker.ts b/src/hooks/cursor/wiki-worker.ts index 7262edb0f..dc6de226c 100644 --- a/src/hooks/cursor/wiki-worker.ts +++ b/src/hooks/cursor/wiki-worker.ts @@ -12,7 +12,7 @@ * differs: codex shells `codex exec`, we shell `cursor-agent --print --model X`. */ -import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, utimesSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -274,6 +274,13 @@ async function main(): Promise { wlog(`running cursor-agent --print (model=${cfg.cursorModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + // Backdate the pre-seeded file before handing it to the child. Comparing + // mtime against "a minute ago" instead of "just now" closes the only hole + // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, + // say) a same-tick rewrite would otherwise keep the timestamp and read as + // "never written". Any write by the child lands far outside that window. + const MTIME_SENTINEL = new Date(Date.now() - 60_000); + if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // cursor-agent --print is the non-interactive headless mode. --force diff --git a/src/hooks/hermes/wiki-worker.ts b/src/hooks/hermes/wiki-worker.ts index 3353b9fa2..ada49d9f4 100644 --- a/src/hooks/hermes/wiki-worker.ts +++ b/src/hooks/hermes/wiki-worker.ts @@ -12,7 +12,7 @@ * differs: codex shells `codex exec`, we shell `hermes -z --provider X -m Y`. */ -import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, utimesSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -276,6 +276,13 @@ async function main(): Promise { wlog(`running hermes -z (provider=${cfg.hermesProvider}, model=${cfg.hermesModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + // Backdate the pre-seeded file before handing it to the child. Comparing + // mtime against "a minute ago" instead of "just now" closes the only hole + // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, + // say) a same-tick rewrite would otherwise keep the timestamp and read as + // "never written". Any write by the child lands far outside that window. + const MTIME_SENTINEL = new Date(Date.now() - 60_000); + if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // hermes -z (--oneshot) is the non-interactive mode. --yolo diff --git a/src/hooks/pi/wiki-worker.ts b/src/hooks/pi/wiki-worker.ts index 0eadf0340..c9a00bb10 100644 --- a/src/hooks/pi/wiki-worker.ts +++ b/src/hooks/pi/wiki-worker.ts @@ -16,7 +16,7 @@ * we shell `pi --print --provider

--model `. Same query/upload paths. */ -import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, utimesSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -224,6 +224,13 @@ async function main(): Promise { wlog(`running pi --print (provider=${cfg.piProvider}, model=${cfg.piModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + // Backdate the pre-seeded file before handing it to the child. Comparing + // mtime against "a minute ago" instead of "just now" closes the only hole + // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, + // say) a same-tick rewrite would otherwise keep the timestamp and read as + // "never written". Any write by the child lands far outside that window. + const MTIME_SENTINEL = new Date(Date.now() - 60_000); + if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // pi --print is the non-interactive mode; it bypasses extension diff --git a/src/hooks/wiki-worker.ts b/src/hooks/wiki-worker.ts index d63c667ec..423de337f 100644 --- a/src/hooks/wiki-worker.ts +++ b/src/hooks/wiki-worker.ts @@ -7,7 +7,7 @@ * Invoked by session-end.ts as: node wiki-worker.js */ -import { readFileSync, writeFileSync, existsSync, statSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; +import { readFileSync, writeFileSync, existsSync, statSync, utimesSync, appendFileSync, mkdirSync, rmSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { buildClaudeInvocation } from "./wiki-worker-spawn.js"; import { dirname, join } from "node:path"; @@ -324,6 +324,13 @@ async function main(): Promise { wlog("running claude -p"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; + // Backdate the pre-seeded file before handing it to the child. Comparing + // mtime against "a minute ago" instead of "just now" closes the only hole + // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, + // say) a same-tick rewrite would otherwise keep the timestamp and read as + // "never written". Any write by the child lands far outside that window. + const MTIME_SENTINEL = new Date(Date.now() - 60_000); + if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; try { // tmpDir holds both the session JSONL to read and the summary to write, diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index ecfbd4f50..a918dc457 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync, utimesSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -423,11 +423,10 @@ describe("wiki-worker — happy path", () => { mkFetch(undefined, 1, true, 14); execFileSyncMock.mockImplementation((_bin: string, args: string[]) => { const summaryPath = args[1].match(/SUMMARY=(\S+)/)![1]; - const identical = readFileSync(summaryPath, "utf-8"); - // Ensure the mtime moves even on a fast filesystem clock. - utimesSync(summaryPath, new Date(Date.now() + 5000), new Date(Date.now() + 5000)); - writeFileSync(summaryPath, identical); - utimesSync(summaryPath, new Date(Date.now() + 5000), new Date(Date.now() + 5000)); + // A plain rewrite of the same bytes, with NO mtime manipulation — the + // worst case for a content-only check, and the case a coarse filesystem + // clock would hide if the guard compared against "just now". + writeFileSync(summaryPath, readFileSync(summaryPath, "utf-8")); return Buffer.from(""); }); await runWorker(); From 0ad55a03a98b39bed449ea26ee4a7f963f7645ec Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 19:10:00 +0000 Subject: [PATCH 8/9] fix(wiki): make the wrote-nothing check survive a filesystem that refuses utimes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pushback, again correct on both counts: - the `utimesSync` call sat OUTSIDE the child's try block, so a filesystem that rejects it (read-only, exotic mount) would abort the worker outright — a regression this branch introduced; - a filesystem that accepts utimes and silently ignores it leaves the baseline at "now", where the coarse-clock hole reopens. Both are now handled in one place. `markSummaryUnwritten` backdates the pre-seeded file and verifies the stamp actually took, reporting `trusted: false` when utimes throws or does nothing; `summaryWasWritten` falls back to content comparison when the timestamp cannot be trusted. That fallback errs toward skipping the upload: re-summarizing the same rows next run wastes work, whereas a wrong upload destroys events — never the other way round. The logic lived in five copies across the workers, so it moves to the `wiki-offset` module they all already import, and is unit-tested directly: untouched file, identical rewrite, different content, first run with no pre-seed, utimes throwing, utimes silently ignored, and the file vanishing mid-check. --- src/hooks/codex/wiki-worker.ts | 17 +--- src/hooks/cursor/wiki-worker.ts | 17 +--- src/hooks/hermes/wiki-worker.ts | 17 +--- src/hooks/pi/wiki-worker.ts | 17 +--- src/hooks/wiki-offset.ts | 57 +++++++++++++ src/hooks/wiki-worker.ts | 17 +--- tests/shared/wiki-summary-written.test.ts | 98 +++++++++++++++++++++++ 7 files changed, 170 insertions(+), 70 deletions(-) create mode 100644 tests/shared/wiki-summary-written.test.ts diff --git a/src/hooks/codex/wiki-worker.ts b/src/hooks/codex/wiki-worker.ts index 50e49df1b..9353a6793 100644 --- a/src/hooks/codex/wiki-worker.ts +++ b/src/hooks/codex/wiki-worker.ts @@ -13,7 +13,7 @@ import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { finalizeSummary, releaseLock, readState } from "../summary-state.js"; -import { capLinesByBytes, newRowsFromWindow, stampOffset, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; +import { capLinesByBytes, markSummaryUnwritten, newRowsFromWindow, stampOffset, summaryWasWritten, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; import { redactSecrets } from "../shared/redact.js"; import { uploadSummary } from "../upload-summary.js"; import { log as _log } from "../../utils/debug.js"; @@ -239,14 +239,7 @@ async function main(): Promise { wlog("running codex exec"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; - // Backdate the pre-seeded file before handing it to the child. Comparing - // mtime against "a minute ago" instead of "just now" closes the only hole - // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, - // say) a same-tick rewrite would otherwise keep the timestamp and read as - // "never written". Any write by the child lands far outside that window. - const MTIME_SENTINEL = new Date(Date.now() - 60_000); - if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); - const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; + const summaryBaseline = markSummaryUnwritten(tmpSummary, { existsSync, utimesSync, statSync }); try { const inv = buildTrailingPromptInvocation(cfg.codexBin, [ "exec", @@ -284,11 +277,7 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - // mtime, not just content: an agent that legitimately regenerates the - // SAME text still touched the file, and skipping that would freeze the - // offset and re-summarize those rows on every future run. Only a run - // that neither changed the bytes NOR touched the file wrote nothing. - if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + if (!summaryWasWritten(tmpSummary, summaryBaseline, summaryChanged, { statSync })) { wlog("codex exec exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } diff --git a/src/hooks/cursor/wiki-worker.ts b/src/hooks/cursor/wiki-worker.ts index dc6de226c..e46204dcd 100644 --- a/src/hooks/cursor/wiki-worker.ts +++ b/src/hooks/cursor/wiki-worker.ts @@ -20,7 +20,7 @@ import { fileURLToPath } from "node:url"; import { finalizeSummary, releaseLock, readState } from "../summary-state.js"; import { readSessionEventCache } from "../session-event-cache.js"; import { buildSessionPath } from "../../utils/session-path.js"; -import { capLinesByBytes, newRowsFromWindow, stampOffset, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; +import { capLinesByBytes, markSummaryUnwritten, newRowsFromWindow, stampOffset, summaryWasWritten, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; import { redactSecrets } from "../shared/redact.js"; import { uploadSummary } from "../upload-summary.js"; import { log as _log } from "../../utils/debug.js"; @@ -274,14 +274,7 @@ async function main(): Promise { wlog(`running cursor-agent --print (model=${cfg.cursorModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; - // Backdate the pre-seeded file before handing it to the child. Comparing - // mtime against "a minute ago" instead of "just now" closes the only hole - // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, - // say) a same-tick rewrite would otherwise keep the timestamp and read as - // "never written". Any write by the child lands far outside that window. - const MTIME_SENTINEL = new Date(Date.now() - 60_000); - if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); - const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; + const summaryBaseline = markSummaryUnwritten(tmpSummary, { existsSync, utimesSync, statSync }); try { // cursor-agent --print is the non-interactive headless mode. --force // auto-allows tools (matches the bypass-approvals semantic codex used). @@ -325,11 +318,7 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - // mtime, not just content: an agent that legitimately regenerates the - // SAME text still touched the file, and skipping that would freeze the - // offset and re-summarize those rows on every future run. Only a run - // that neither changed the bytes NOR touched the file wrote nothing. - if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + if (!summaryWasWritten(tmpSummary, summaryBaseline, summaryChanged, { statSync })) { wlog("cursor-agent --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } diff --git a/src/hooks/hermes/wiki-worker.ts b/src/hooks/hermes/wiki-worker.ts index ada49d9f4..f1c878b60 100644 --- a/src/hooks/hermes/wiki-worker.ts +++ b/src/hooks/hermes/wiki-worker.ts @@ -19,7 +19,7 @@ import { fileURLToPath } from "node:url"; import { finalizeSummary, releaseLock, readState } from "../summary-state.js"; import { readSessionEventCache } from "../session-event-cache.js"; import { buildSessionPath } from "../../utils/session-path.js"; -import { capLinesByBytes, newRowsFromWindow, stampOffset, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; +import { capLinesByBytes, markSummaryUnwritten, newRowsFromWindow, stampOffset, summaryWasWritten, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; import { redactSecrets } from "../shared/redact.js"; import { uploadSummary } from "../upload-summary.js"; import { log as _log } from "../../utils/debug.js"; @@ -276,14 +276,7 @@ async function main(): Promise { wlog(`running hermes -z (provider=${cfg.hermesProvider}, model=${cfg.hermesModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; - // Backdate the pre-seeded file before handing it to the child. Comparing - // mtime against "a minute ago" instead of "just now" closes the only hole - // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, - // say) a same-tick rewrite would otherwise keep the timestamp and read as - // "never written". Any write by the child lands far outside that window. - const MTIME_SENTINEL = new Date(Date.now() - 60_000); - if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); - const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; + const summaryBaseline = markSummaryUnwritten(tmpSummary, { existsSync, utimesSync, statSync }); try { // hermes -z (--oneshot) is the non-interactive mode. --yolo // auto-approves tool use within the spawned hermes process. @@ -337,11 +330,7 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - // mtime, not just content: an agent that legitimately regenerates the - // SAME text still touched the file, and skipping that would freeze the - // offset and re-summarize those rows on every future run. Only a run - // that neither changed the bytes NOR touched the file wrote nothing. - if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + if (!summaryWasWritten(tmpSummary, summaryBaseline, summaryChanged, { statSync })) { wlog("hermes -z exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } diff --git a/src/hooks/pi/wiki-worker.ts b/src/hooks/pi/wiki-worker.ts index c9a00bb10..fbbeb5bea 100644 --- a/src/hooks/pi/wiki-worker.ts +++ b/src/hooks/pi/wiki-worker.ts @@ -22,7 +22,7 @@ import { buildTrailingPromptInvocation } from "../wiki-worker-spawn.js"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { finalizeSummary, releaseLock, readState } from "../summary-state.js"; -import { capLinesByBytes, newRowsFromWindow, stampOffset, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; +import { capLinesByBytes, markSummaryUnwritten, newRowsFromWindow, stampOffset, summaryWasWritten, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "../wiki-offset.js"; import { redactSecrets } from "../shared/redact.js"; import { uploadSummary } from "../upload-summary.js"; import { log as _log } from "../../utils/debug.js"; @@ -224,14 +224,7 @@ async function main(): Promise { wlog(`running pi --print (provider=${cfg.piProvider}, model=${cfg.piModel})`); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; - // Backdate the pre-seeded file before handing it to the child. Comparing - // mtime against "a minute ago" instead of "just now" closes the only hole - // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, - // say) a same-tick rewrite would otherwise keep the timestamp and read as - // "never written". Any write by the child lands far outside that window. - const MTIME_SENTINEL = new Date(Date.now() - 60_000); - if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); - const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; + const summaryBaseline = markSummaryUnwritten(tmpSummary, { existsSync, utimesSync, statSync }); try { // pi --print is the non-interactive mode; it bypasses extension // discovery (modes/print-mode.js doesn't import ExtensionRunner), @@ -276,11 +269,7 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - // mtime, not just content: an agent that legitimately regenerates the - // SAME text still touched the file, and skipping that would freeze the - // offset and re-summarize those rows on every future run. Only a run - // that neither changed the bytes NOR touched the file wrote nothing. - if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + if (!summaryWasWritten(tmpSummary, summaryBaseline, summaryChanged, { statSync })) { wlog("pi --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } diff --git a/src/hooks/wiki-offset.ts b/src/hooks/wiki-offset.ts index 633c7b5da..b5a6ea7b5 100644 --- a/src/hooks/wiki-offset.ts +++ b/src/hooks/wiki-offset.ts @@ -126,3 +126,60 @@ function truncateUtf8(s: string, maxBytes: number): string { const decoder = new TextDecoder("utf-8", { fatal: false }); return decoder.decode(buf.subarray(0, maxBytes)).replace(/�+$/, ""); } + +/** + * Did THIS run's agent actually write the summary? + * + * Exit 0 is not proof of work: a child that cannot reach the scratch dir (an + * enterprise policy disabling bypassPermissions, say) exits 0 having written + * nothing, leaving the pre-seeded summary in place. Uploading that unchanged + * placeholder still advances the offset and slices the unread events away + * forever, so the worker must be able to tell "wrote nothing" from "wrote". + * + * Content equality alone cannot: an agent that legitimately regenerates the + * same text would read as a no-op and freeze the offset. So the baseline + * stamps the file a minute into the past and the check compares timestamps — + * any real write lands far outside any plausible filesystem granularity. + * + * When the filesystem will not cooperate (utimes throwing or silently doing + * nothing) the timestamp proves nothing, and the check falls back to content. + * That errs toward skipping: re-summarizing the same rows next run wastes work, + * whereas a wrong upload destroys events. Never the other way round. + */ +export interface SummaryBaseline { + mtimeMs: number; + /** False when utimes threw or left the timestamp unchanged. */ + trusted: boolean; +} + +const SUMMARY_BACKDATE_MS = 60_000; + +export function markSummaryUnwritten( + path: string, + fs: Pick, +): SummaryBaseline { + if (!fs.existsSync(path)) return { mtimeMs: 0, trusted: true }; + const sentinel = new Date(Date.now() - SUMMARY_BACKDATE_MS); + try { + fs.utimesSync(path, sentinel, sentinel); + const mtimeMs = fs.statSync(path).mtimeMs; + // A filesystem that ignores utimes reports something far from the sentinel. + return { mtimeMs, trusted: Math.abs(mtimeMs - sentinel.getTime()) < 2_000 }; + } catch { + return { mtimeMs: 0, trusted: false }; + } +} + +export function summaryWasWritten( + path: string, + baseline: SummaryBaseline, + contentChanged: boolean, + fs: Pick, +): boolean { + if (!baseline.trusted) return contentChanged; + try { + return fs.statSync(path).mtimeMs !== baseline.mtimeMs; + } catch { + return contentChanged; + } +} diff --git a/src/hooks/wiki-worker.ts b/src/hooks/wiki-worker.ts index 423de337f..525a3c08d 100644 --- a/src/hooks/wiki-worker.ts +++ b/src/hooks/wiki-worker.ts @@ -19,7 +19,7 @@ const dlog = (msg: string) => _log("wiki-worker", msg); import { finalizeSummary, releaseLock, readState } from "./summary-state.js"; import { readSessionEventCache } from "./session-event-cache.js"; import { buildSessionPath } from "../utils/session-path.js"; -import { capLinesByBytes, newRowsFromWindow, stampOffset, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "./wiki-offset.js"; +import { capLinesByBytes, markSummaryUnwritten, newRowsFromWindow, stampOffset, summaryWasWritten, WIKI_FALLBACK_MAX_ROWS, WIKI_JSONL_MAX_BYTES } from "./wiki-offset.js"; import { redactSecrets } from "./shared/redact.js"; import { uploadSummary } from "./upload-summary.js"; import { embedSummaryWithWarmup } from "../embeddings/embed-summary.js"; @@ -324,14 +324,7 @@ async function main(): Promise { wlog("running claude -p"); let execSucceeded = false; const summaryBeforeExec = existsSync(tmpSummary) ? readFileSync(tmpSummary, "utf-8") : null; - // Backdate the pre-seeded file before handing it to the child. Comparing - // mtime against "a minute ago" instead of "just now" closes the only hole - // in the wrote-nothing check: on a coarse-resolution filesystem (FAT's 2s, - // say) a same-tick rewrite would otherwise keep the timestamp and read as - // "never written". Any write by the child lands far outside that window. - const MTIME_SENTINEL = new Date(Date.now() - 60_000); - if (existsSync(tmpSummary)) utimesSync(tmpSummary, MTIME_SENTINEL, MTIME_SENTINEL); - const summaryMtimeBefore = existsSync(tmpSummary) ? statSync(tmpSummary).mtimeMs : 0; + const summaryBaseline = markSummaryUnwritten(tmpSummary, { existsSync, utimesSync, statSync }); try { // tmpDir holds both the session JSONL to read and the summary to write, // and lives outside the session cwd — grant it explicitly rather than @@ -375,11 +368,7 @@ async function main(): Promise { // summary in place. Uploading it unchanged would still advance the offset // and slice those events away forever, which is how a session gets stuck // as a header-only placeholder run after run. - // mtime, not just content: an agent that legitimately regenerates the - // SAME text still touched the file, and skipping that would freeze the - // offset and re-summarize those rows on every future run. Only a run - // that neither changed the bytes NOR touched the file wrote nothing. - if (!summaryChanged && statSync(tmpSummary).mtimeMs === summaryMtimeBefore) { + if (!summaryWasWritten(tmpSummary, summaryBaseline, summaryChanged, { statSync })) { wlog("claude -p exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); return; } diff --git a/tests/shared/wiki-summary-written.test.ts b/tests/shared/wiki-summary-written.test.ts new file mode 100644 index 000000000..a040d5930 --- /dev/null +++ b/tests/shared/wiki-summary-written.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync, writeFileSync, existsSync, utimesSync, statSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { markSummaryUnwritten, summaryWasWritten } from "../../src/hooks/wiki-offset.js"; + +/** + * "Did the agent actually write?" is the decision that separates a real summary + * from the placeholder-forever bug: answering "yes" when the child wrote nothing + * uploads the pre-seeded stub AND advances the offset, destroying the unread + * events. These pin the answer on every filesystem the workers can land on. + */ + +const realFs = { existsSync, utimesSync, statSync }; + +function withFile(body: (path: string) => void): void { + const dir = mkdtempSync(join(tmpdir(), "summary-written-")); + try { body(join(dir, "summary.md")); } finally { rmSync(dir, { recursive: true, force: true }); } +} + +describe("markSummaryUnwritten / summaryWasWritten", () => { + it("reports NOT written when the agent leaves the pre-seeded file alone", () => { + withFile((p) => { + writeFileSync(p, "# placeholder\n"); + const base = markSummaryUnwritten(p, realFs); + expect(base.trusted).toBe(true); + expect(summaryWasWritten(p, base, false, realFs)).toBe(false); + }); + }); + + it("reports WRITTEN when the agent rewrites the identical bytes", () => { + // The regression the timestamp check exists for: content equality alone + // would call this a no-op and freeze the offset forever. + withFile((p) => { + writeFileSync(p, "# same\n"); + const base = markSummaryUnwritten(p, realFs); + writeFileSync(p, "# same\n"); + expect(summaryWasWritten(p, base, false, realFs)).toBe(true); + }); + }); + + it("reports WRITTEN when the agent writes different content", () => { + withFile((p) => { + writeFileSync(p, "# before\n"); + const base = markSummaryUnwritten(p, realFs); + writeFileSync(p, "# after\n"); + expect(summaryWasWritten(p, base, true, realFs)).toBe(true); + }); + }); + + it("treats a first run with no pre-seeded file as written when content appeared", () => { + withFile((p) => { + const base = markSummaryUnwritten(p, realFs); + expect(base).toEqual({ mtimeMs: 0, trusted: true }); + writeFileSync(p, "# fresh\n"); + expect(summaryWasWritten(p, base, true, realFs)).toBe(true); + }); + }); + + it("does not throw — and distrusts the timestamp — when utimes is rejected", () => { + // A read-only or exotic filesystem must not abort the worker: before this + // was routed through the helper, the utimes call sat outside the try and a + // throw killed the run outright. + withFile((p) => { + writeFileSync(p, "# ro\n"); + const fs = { existsSync, statSync, utimesSync: () => { throw new Error("EPERM"); } }; + const base = markSummaryUnwritten(p, fs); + expect(base.trusted).toBe(false); + // Untrusted timestamps fall back to content, erring toward skipping. + expect(summaryWasWritten(p, base, false, realFs)).toBe(false); + expect(summaryWasWritten(p, base, true, realFs)).toBe(true); + }); + }); + + it("distrusts the timestamp when utimes silently does nothing", () => { + // The coarse-clock hole: a filesystem that accepts utimes and ignores it + // would leave the baseline at "now", where a same-tick identical rewrite + // is indistinguishable from no write at all. + withFile((p) => { + writeFileSync(p, "# noop\n"); + const fs = { existsSync, statSync, utimesSync: () => { /* silently ignored */ } }; + const base = markSummaryUnwritten(p, fs); + expect(base.trusted).toBe(false); + expect(summaryWasWritten(p, base, false, realFs)).toBe(false); + }); + }); + + it("falls back to content when the file vanishes before the check", () => { + withFile((p) => { + writeFileSync(p, "# gone\n"); + const base = markSummaryUnwritten(p, realFs); + rmSync(p); + expect(summaryWasWritten(p, base, true, realFs)).toBe(true); + expect(summaryWasWritten(p, base, false, realFs)).toBe(false); + }); + }); +}); From 221642557d4bc191e1337378b786d1667b61d478 Mon Sep 17 00:00:00 2001 From: Emanuele Fenocchi Date: Thu, 3 Sep 2026 19:17:18 +0000 Subject: [PATCH 9/9] test: assert each worker's full no-write message, not a shared substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Raised by CodeRabbit. `toContain("never wrote the summary")` passes for all five workers, so a guard copy-pasted from one worker into another — precisely how the original bug reached all five — would satisfy every one of these tests. Pin the complete per-worker line instead. Verified by making the pi worker log codex's message: its test goes red. --- tests/claude-code/wiki-worker.test.ts | 2 +- tests/codex/codex-wiki-worker.test.ts | 2 +- tests/cursor/cursor-wiki-worker.test.ts | 2 +- tests/hermes/hermes-wiki-worker.test.ts | 2 +- tests/pi/pi-wiki-worker.test.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/claude-code/wiki-worker.test.ts b/tests/claude-code/wiki-worker.test.ts index a918dc457..2197620ad 100644 --- a/tests/claude-code/wiki-worker.test.ts +++ b/tests/claude-code/wiki-worker.test.ts @@ -411,7 +411,7 @@ describe("wiki-worker — happy path", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("never wrote the summary"); + expect(log).toContain("claude -p exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); expect(releaseLockMock).toHaveBeenCalledWith("sid-worker"); }); diff --git a/tests/codex/codex-wiki-worker.test.ts b/tests/codex/codex-wiki-worker.test.ts index 896bb41a9..6056da647 100644 --- a/tests/codex/codex-wiki-worker.test.ts +++ b/tests/codex/codex-wiki-worker.test.ts @@ -282,7 +282,7 @@ describe("codex wiki-worker — happy path", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("never wrote the summary"); + expect(log).toContain("codex exec exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); expect(releaseLockMock).toHaveBeenCalledWith("sid-codex"); }); diff --git a/tests/cursor/cursor-wiki-worker.test.ts b/tests/cursor/cursor-wiki-worker.test.ts index 73ae7a2f3..94acf9efa 100644 --- a/tests/cursor/cursor-wiki-worker.test.ts +++ b/tests/cursor/cursor-wiki-worker.test.ts @@ -202,7 +202,7 @@ describe("cursor wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("never wrote the summary"); + expect(log).toContain("cursor-agent --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); expect(releaseLockMock).toHaveBeenCalledWith("sid-cursor"); }); diff --git a/tests/hermes/hermes-wiki-worker.test.ts b/tests/hermes/hermes-wiki-worker.test.ts index f27df2e30..18f77f479 100644 --- a/tests/hermes/hermes-wiki-worker.test.ts +++ b/tests/hermes/hermes-wiki-worker.test.ts @@ -208,7 +208,7 @@ describe("hermes wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("never wrote the summary"); + expect(log).toContain("hermes -z exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); expect(releaseLockMock).toHaveBeenCalledWith("sid-hermes"); }); diff --git a/tests/pi/pi-wiki-worker.test.ts b/tests/pi/pi-wiki-worker.test.ts index 87e267e6a..634368238 100644 --- a/tests/pi/pi-wiki-worker.test.ts +++ b/tests/pi/pi-wiki-worker.test.ts @@ -198,7 +198,7 @@ describe("pi wiki-worker — behavior", () => { expect(uploadSummaryMock).not.toHaveBeenCalled(); expect(finalizeSummaryMock).not.toHaveBeenCalled(); const log = readFileSync(join(hooksDir, "wiki.log"), "utf-8"); - expect(log).toContain("never wrote the summary"); + expect(log).toContain("pi --print exited 0 but never wrote the summary; skipping upload to avoid advancing the offset"); expect(releaseLockMock).toHaveBeenCalledWith("sid-pi"); }); });