fix(coding-agents): stop the prompt hook wiping the retain cursor; write state atomically (#3412)

Found porting #3136 (unlocked state read-modify-write on Windows). The race it
describes is far less severe here — state is one file per session rather than a
dict keyed by session, so a lost update cannot drop other sessions — but looking
for it surfaced a worse, unconditional bug underneath.

The retain cursor was a FIELD of the session cache, and the prompt hook writes a
fresh `{turns, reflectAnswer, pages}` object rather than merging. So every user
prompt dropped the cursor the previous Stop had written; the next Stop found
none and rewrote the whole document. The incremental write-back added in #3336
therefore never engaged past a session's first turn on ANY hook harness — seven
of eleven. Not a race: deterministic, every session, every turn.

  after Stop   : {"retain":{"turns":5,...}}
  after prompt : {"turns":2,"pages":{...}}     <- cursor gone
  cursor now   : undefined

No test caught it because the unit tests inject a memoryCursorStore directly and
never exercise the file store against a real prompt-hook write. The regression
test added here does exactly that interleaving.

The cursor now lives in its own file. That makes the invariant structural rather
than a convention every future writer has to remember: the two writers have
different lifecycles, no longer share a record, and neither can clobber the
other — which also leaves the concurrent read-modify-write #3136 measured with
nothing to lose here.

State writes are also atomic now (temp file + rename). A plain writeFileSync can
be observed half-written and leaves a fragment behind if the process is killed
mid-write; rename is atomic on POSIX and replaces on Windows. The per-agent
plugin's Python writes already had os.replace() — this had no equivalent.

Cannot be verified on Windows from here; the atomicity is platform-independent
and the clobber fix is verified on macOS.

Author: nicoloboschi

hindsight-integrations/coding-agents/src/core/session-cache.test.ts

@@ -11,7 +11,10 @@ const HARNESS = "codex-cursor-test";
 const sessions = ["s1", "s2"];
 
 afterEach(() => {
-  for (const s of sessions) rmSync(sessionCacheFile(HARNESS, s), { force: true });
+  for (const s of sessions) {
+    rmSync(sessionCacheFile(HARNESS, s), { force: true });
+    rmSync(sessionCacheFile(HARNESS, s).replace(/\.json$/, ".retain.json"), { force: true });
+  }
 });
 
 describe("fileCursorStore", () => {
@@ -38,14 +41,39 @@ describe("fileCursorStore", () => {
     expect(fileCursorStore(HARNESS).read("s1")).toBeUndefined();
   });
 
-  it("preserves the rest of the session cache it shares a file with", () => {
+  it("survives the prompt hook rewriting the session cache", () => {
+    // The regression this file exists for. The cursor used to be a FIELD of the session cache, and
+    // the prompt hook writes a fresh {turns, reflectAnswer, pages} object rather than merging — so
+    // every user prompt dropped it, the next Stop found no cursor, and the incremental write-back
+    // silently degraded to a full replace on every hook harness after a session's first turn.
+    const store = fileCursorStore(HARNESS);
+    store.write("s1", { turns: 5, fingerprint: "f", bank: "b1" });
+
+    writeSessionCache(sessionCacheFile(HARNESS, "s1"), {
+      turns: 2,
+      pages: { atTurn: 2, list: [] },
+    });
+
+    expect(store.read("s1")).toEqual({ turns: 5, fingerprint: "f", bank: "b1" });
+  });
+
+  it("leaves the session cache alone", () => {
+    // The inverse direction: writing a cursor must not disturb the recall state either.
     const file = sessionCacheFile(HARNESS, "s1");
     writeSessionCache(file, { turns: 3, reflectAnswer: "already ran" });
     fileCursorStore(HARNESS).write("s1", { turns: 2, fingerprint: "f", bank: "b1" });
-    expect(readSessionCache(file)).toEqual({
-      turns: 3,
-      reflectAnswer: "already ran",
-      retain: { turns: 2, fingerprint: "f", bank: "b1" },
-    });
+    expect(readSessionCache(file)).toEqual({ turns: 3, reflectAnswer: "already ran" });
+  });
+
+  it("never exposes a half-written cursor", () => {
+    // Writes go through a temp file and a rename, so a reader sees the old value or the new one.
+    const store = fileCursorStore(HARNESS);
+    store.write("s1", { turns: 1, fingerprint: "a", bank: "b1" });
+    for (let i = 2; i <= 30; i++) {
+      store.write("s1", { turns: i, fingerprint: "a".repeat(i * 200), bank: "b1" });
+      const seen = store.read("s1");
+      expect(seen?.turns).toBe(i);
+      expect(seen?.fingerprint).toHaveLength(i * 200);
+    }
   });
 });

hindsight-integrations/coding-agents/src/core/session-cache.ts

@@ -1,4 +1,4 @@
-import { mkdirSync, readFileSync, writeFileSync } from "node:fs";
+import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { dirname, join } from "node:path";
 import type { PageRef } from "./knowledge-injection";
@@ -13,8 +13,6 @@ export interface SessionCache {
   /** SessionStart saw a new/empty bank; consume this on prompt one, then allow reflect. */
   deferInitialReflect?: boolean;
   pages?: { atTurn: number; list: PageRef[] };
-  /** How much of this session's transcript is already in its document (core/retain-cursor.ts). */
-  retain?: RetainCursor;
 }
 
 export function sessionCacheFile(harness: string, sessionId: string): string {
@@ -29,28 +27,74 @@ export function readSessionCache(cacheFile: string): SessionCache {
   }
 }
 
+/**
+ * Replace a state file atomically: write a sibling temp file, then rename over the target.
+ *
+ * A plain writeFileSync is not atomic — a reader can observe a half-written file, and a process
+ * killed mid-write leaves one behind. rename() is atomic on POSIX and replaces the destination on
+ * Windows, so a reader sees either the old contents or the new, never a fragment (#3136, which
+ * measured this class on the per-agent plugin; its Python state writes already had os.replace()).
+ */
+function writeFileAtomic(path: string, body: string): void {
+  mkdirSync(dirname(path), { recursive: true });
+  // The temp name is per-process, so two writers cannot collide on it.
+  const tmp = `${path}.${process.pid}.tmp`;
+  try {
+    writeFileSync(tmp, body);
+    renameSync(tmp, path);
+  } catch (e) {
+    try {
+      rmSync(tmp, { force: true });
+    } catch {
+      /* nothing further to do */
+    }
+    throw e;
+  }
+}
+
 export function writeSessionCache(cacheFile: string, cache: SessionCache): void {
   try {
-    mkdirSync(dirname(cacheFile), { recursive: true });
-    writeFileSync(cacheFile, JSON.stringify(cache));
+    writeFileAtomic(cacheFile, JSON.stringify(cache));
   } catch {
     /* session state is best-effort */
   }
 }
 
+/** The cursor's own file, deliberately NOT the shared session cache — see fileCursorStore. */
+function cursorFile(harness: string, sessionId: string): string {
+  return join(tmpdir(), `hindsight-${harness}`, `${sessionId}.retain.json`);
+}
+
 /**
- * Retain cursor kept in the per-session temp file, for the hook harnesses: Stop runs in a fresh
- * process every time, so "what have I already written" cannot live in memory.
+ * Retain cursor for the hook harnesses: Stop runs in a fresh process every time, so "what have I
+ * already written" cannot live in memory.
  *
- * Losing this file (temp cleanup, reboot) is not a correctness problem — a missing cursor means the
- * next retain replaces the whole document, which is exactly what the plugin did before appends.
+ * It lives in its OWN file rather than a field of the session cache. It shared that file until now,
+ * and the prompt hook — which writes a fresh `{turns, reflectAnswer, pages}` object rather than
+ * merging — dropped the cursor on EVERY user prompt. The next Stop then found none and rewrote the
+ * whole document, so the incremental write-back never engaged past a session's first turn on any
+ * hook harness. Separate files make that structural: the two writers have different lifecycles and
+ * no longer share a record, so neither can clobber the other, and the concurrent read-modify-write
+ * that #3136 measured has nothing left to lose here.
+ *
+ * Losing the file (temp cleanup, reboot) is still not a correctness problem — a missing cursor
+ * means the next retain replaces the whole document, exactly as it did before appends existed.
  */
 export function fileCursorStore(harness: string): RetainCursorStore {
   return {
-    read: (sessionId) => readSessionCache(sessionCacheFile(harness, sessionId)).retain,
+    read: (sessionId) => {
+      try {
+        return JSON.parse(readFileSync(cursorFile(harness, sessionId), "utf8")) as RetainCursor;
+      } catch {
+        return undefined;
+      }
+    },
     write: (sessionId, cursor) => {
-      const file = sessionCacheFile(harness, sessionId);
-      writeSessionCache(file, { ...readSessionCache(file), retain: cursor });
+      try {
+        writeFileAtomic(cursorFile(harness, sessionId), JSON.stringify(cursor));
+      } catch {
+        /* best-effort: a cursor that cannot be written costs a replace, never data */
+      }
     },
   };
 }