Recover incremental summaries after interruption or snapshot change

scripts/build-diff-data.mjs

@@ -1040,14 +1040,19 @@ function build() {
   const failureByPath = new Map(
     failedFiles.map((failure) => [failure.path, failure.reason]),
   );
+  const emptyReviewComplete =
+    filesWithoutSummaries.length === 0 &&
+    generatedFor === reviewFingerprint &&
+    sourceSummaries.meta?.status === 'complete';
   const summariesAreComplete =
     summariesAreFresh &&
     failedFiles.length === 0 &&
     summaryErrors.length === 0 &&
-    completeChangeSummary(sourceSummaries.change) &&
-    filesWithoutSummaries.every((file) =>
-      completeFileSummary(sourceSummaries.files?.[file.path]),
-    );
+    (emptyReviewComplete ||
+      (completeChangeSummary(sourceSummaries.change) &&
+        filesWithoutSummaries.every((file) =>
+          completeFileSummary(sourceSummaries.files?.[file.path]),
+        )));
   const completedFiles = noSummaries
     ? 0
     : filesWithoutSummaries.filter((file) =>

scripts/generate-summaries.mjs

@@ -3,6 +3,7 @@
 import { spawn, spawnSync } from 'node:child_process';
 import { createHash } from 'node:crypto';
 import {
+  existsSync,
   mkdirSync,
   mkdtempSync,
   readFileSync,
@@ -105,18 +106,10 @@ const outputPath = resolve(
 );
 const codexBin = option('--codex-bin') || process.env.CODEX_BIN;
 let selectedAgent;
-try {
-  selectedAgent = await selectCodingAgent(
-    option('--agent'),
-    (agent) =>
-      commandAvailable(codingAgentBinary(agent, { codexBin })),
-  );
-} catch (error) {
-  fail(error.message);
-}
-const agentBinary = codingAgentBinary(selectedAgent, { codexBin });
+let agentBinary;
 const model = option('--model');
 const reasoning = option('--reasoning');
+const requestedAgent = option('--agent');
 const batchSizeValue = option('--batch-size') || '12';
 const reasoningLevels = new Set([
   'minimal',
@@ -128,10 +121,13 @@ const reasoningLevels = new Set([
 if (reasoning && !reasoningLevels.has(reasoning)) {
   fail('--reasoning must be minimal, low, medium, high, or xhigh');
 }
-try {
-  assertReasoningSupported(selectedAgent, reasoning);
-} catch (error) {
-  fail(error.message);
+if (requestedAgent) {
+  try {
+    await selectCodingAgent(requestedAgent, async () => true);
+    assertReasoningSupported(requestedAgent, reasoning);
+  } catch (error) {
+    fail(error instanceof Error ? error.message : String(error));
+  }
 }
 if (!/^[1-9]\d*$/.test(batchSizeValue) || Number(batchSizeValue) > 50) {
   fail('--batch-size must be a number from 1 to 50');
@@ -157,10 +153,13 @@ const snapshotPath = option('--snapshot');
 const activeAgentProcesses = new Set();
 let interrupted = false;
 
-process.once('SIGTERM', () => {
+function interrupt() {
   interrupted = true;
   for (const child of activeAgentProcesses) child.kill('SIGTERM');
-});
+}
+
+process.once('SIGINT', interrupt);
+process.once('SIGTERM', interrupt);
 
 if (range && (base || head)) {
   fail('--range cannot be used with --base or --head');
@@ -629,13 +628,19 @@ function metadataList(value) {
 function summaryFailureState(snapshot, summaries) {
   const failedFiles = metadataList(summaries.meta?.failedFiles);
   const errors = metadataList(summaries.meta?.errors);
+  const emptyReviewComplete =
+    snapshot.files.length === 0 &&
+    summaries.meta?.status === 'complete' &&
+    summaries.meta?.reviewFingerprint ===
+      snapshot.notes?.reviewFingerprint;
   const complete = [
     failedFiles.length === 0,
     errors.length === 0,
-    completeChangeNote(summaries.change),
-    snapshot.files.every((file) =>
-      completeFileNote(summaries.files?.[file.path]),
-    ),
+    emptyReviewComplete ||
+      (completeChangeNote(summaries.change) &&
+        snapshot.files.every((file) =>
+          completeFileNote(summaries.files?.[file.path]),
+        )),
   ].every(Boolean);
   return { failedFiles, errors, complete };
 }
@@ -734,6 +739,35 @@ function readJson(file, fallback) {
   }
 }
 
+function readSummaryState(file) {
+  try {
+    const value = JSON.parse(readFileSync(file, 'utf8'));
+    const valid =
+      value &&
+      typeof value === 'object' &&
+      !Array.isArray(value);
+    return valid
+      ? { value, damaged: false }
+      : { value: {}, damaged: true };
+  } catch {
+    return { value: {}, damaged: existsSync(file) };
+  }
+}
+
+async function selectAgentForNotes() {
+  try {
+    selectedAgent = await selectCodingAgent(
+      requestedAgent,
+      (agent) => commandAvailable(codingAgentBinary(agent, { codexBin })),
+    );
+    assertReasoningSupported(selectedAgent, reasoning);
+    agentBinary = codingAgentBinary(selectedAgent, { codexBin });
+  } catch (error) {
+    if (error instanceof Error) error.exitCode = 2;
+    throw error;
+  }
+}
+
 function addFailures(summaries, failedFiles = [], errors = []) {
   const priorFailedFiles = metadataList(summaries.meta?.failedFiles);
   const priorErrors = metadataList(summaries.meta?.errors);
@@ -877,13 +911,7 @@ function fileFingerprint(file) {
     .digest('hex');
 }
 
-const generationSettings = {
-  agent: selectedAgent,
-  model: model || null,
-  reasoning: reasoning || null,
-};
-
-function generationSettingsMatch(meta) {
+function generationSettingsMatch(meta, generationSettings) {
   if (!meta || typeof meta !== 'object' || typeof meta.agent !== 'string') {
     return false;
   }
@@ -909,7 +937,13 @@ try {
   );
   const snapshot = cleanSnapshot(rawSnapshot);
   const paths = snapshot.files.map((file) => file.path);
-  const previousSummaries = readJson(summariesPath, {});
+  const previousState = readSummaryState(summariesPath);
+  const previousSummaries = previousState.value;
+  if (previousState.damaged) {
+    console.error(
+      `Saved notes at ${summariesPath} are damaged. Rebuilding them from the current review.`,
+    );
+  }
   if (paths.length === 0) {
     workingSnapshot = rawSnapshot;
     workingSummaries = {
@@ -928,6 +962,12 @@ try {
     publish(rawSnapshot, workingSummaries);
     console.log('No changed files to summarize.');
   } else {
+    await selectAgentForNotes();
+    const generationSettings = {
+      agent: selectedAgent,
+      model: model || null,
+      reasoning: reasoning || null,
+    };
     const startedAt = new Date().toISOString();
     const previousFiles =
       previousSummaries.files &&
@@ -947,7 +987,10 @@ try {
     const fileFingerprints = Object.fromEntries(
       paths.map((path) => [path, fileFingerprint(rawFiles.get(path))]),
     );
-    const settingsMatch = generationSettingsMatch(previousSummaries.meta);
+    const settingsMatch = generationSettingsMatch(
+      previousSummaries.meta,
+      generationSettings,
+    );
     const summaryFiles = new Map(
       snapshot.files.map((file) => [file.path, file]),
     );
@@ -1237,7 +1280,8 @@ try {
   }
   if (!interrupted) {
     console.error(error instanceof Error ? error.message : String(error));
-    process.exitCode = 1;
+    process.exitCode =
+      error instanceof Error && error.exitCode === 2 ? 2 : 1;
   }
 } finally {
   rmSync(temporaryDirectory, { recursive: true, force: true });

scripts/present.mjs

@@ -228,15 +228,19 @@ function snapshotState() {
     const snapshot = JSON.parse(readFileSync(outputPath, 'utf8'));
     if (snapshot.notes?.reviewFingerprint) {
       const fingerprint = snapshot.notes.reviewFingerprint;
+      const emptyReview =
+        Array.isArray(snapshot.files) && snapshot.files.length === 0;
       return {
         fingerprint,
         hasCurrentAgentNotes:
           snapshot.notes.complete &&
           snapshot.notes.fresh &&
           snapshot.notes.generatedFor === fingerprint &&
-          snapshot.notes.agent === selectedAgent &&
-          (snapshot.notes.model || null) === (cli.model || null) &&
-          (snapshot.notes.reasoning || null) === (cli.reasoning || null),
+          (emptyReview ||
+            (snapshot.notes.agent === selectedAgent &&
+              (snapshot.notes.model || null) === (cli.model || null) &&
+              (snapshot.notes.reasoning || null) ===
+                (cli.reasoning || null))),
       };
     }
     const reviewData = {

tests/generate-summaries.test.mjs

@@ -4,6 +4,7 @@ import {
   chmod,
   mkdir,
   mkdtemp,
+  readdir,
   readFile,
   rm,
   stat,
@@ -1680,3 +1681,154 @@ test("drops removed files without regenerating unchanged file notes", async () =
     await rm(repo, { recursive: true, force: true });
   }
 });
+
+test("completes an empty review without looking for an agent", async () => {
+  const repo = await makeRepo();
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+
+  try {
+    const result = run(repo, [
+      "--base",
+      "HEAD",
+      "--head",
+      "HEAD",
+      "--codex-bin",
+      join(repo, "missing-codex"),
+      "--summaries",
+      summaries,
+      "--output",
+      output,
+    ]);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.match(result.stdout, /No changed files to summarize/);
+    const notes = JSON.parse(await readFile(summaries, "utf8"));
+    const snapshot = JSON.parse(await readFile(output, "utf8"));
+    assert.equal(notes.meta.status, "complete");
+    assert.deepEqual(snapshot.files, []);
+    assert.equal(snapshot.notes.complete, true);
+    assert.equal(snapshot.notes.status, "complete");
+  } finally {
+    await rm(repo, { recursive: true, force: true });
+  }
+});
+
+test("cleans temporary review data when agent selection fails", async () => {
+  const repo = await makeRepo();
+  const temporaryRoot = await mkdtemp(join(tmpdir(), "diffsplain-tmp-"));
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+
+  try {
+    const result = run(
+      repo,
+      [
+        "--range",
+        "HEAD~1..HEAD",
+        "--agent",
+        "codex",
+        "--codex-bin",
+        join(repo, "missing-codex"),
+        "--summaries",
+        summaries,
+        "--output",
+        output,
+      ],
+      { env: { ...process.env, TMPDIR: temporaryRoot } },
+    );
+
+    assert.notEqual(result.status, 0);
+    assert.match(result.stderr, /not available/i);
+    assert.deepEqual(await readdir(temporaryRoot), []);
+  } finally {
+    await rm(temporaryRoot, { recursive: true, force: true });
+    await rm(repo, { recursive: true, force: true });
+  }
+});
+
+for (const damagedState of ["{ damaged", "null"]) {
+test(`rebuilds damaged note state ${JSON.stringify(damagedState)} instead of reusing it`, async () => {
+  const repo = await makeRepo();
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+
+  try {
+    await writeFile(summaries, damagedState);
+    const codex = await recordingCodex(repo);
+    const result = run(repo, [
+      "--range",
+      "HEAD~1..HEAD",
+      "--codex-bin",
+      codex.bin,
+      "--summaries",
+      summaries,
+      "--output",
+      output,
+    ]);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.match(result.stderr, /Saved notes .* damaged\. Rebuilding/);
+    const notes = JSON.parse(await readFile(summaries, "utf8"));
+    assert.equal(notes.meta.status, "complete");
+    assert.equal((await recordedCalls(codex.calls)).length, 2);
+  } finally {
+    await rm(repo, { recursive: true, force: true });
+  }
+});
+}
+
+test("interrupting an agent leaves published notes incomplete", async () => {
+  const repo = await makeRepo();
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+  const started = join(repo, "agent-started");
+  const codexBin = join(repo, "slow-codex.mjs");
+  let child;
+
+  try {
+    await writeFile(
+      codexBin,
+      `#!/usr/bin/env node
+import { writeFileSync } from "node:fs";
+writeFileSync(${JSON.stringify(started)}, "started");
+Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 5_000);
+process.stdout.write("{}");
+`,
+    );
+    await chmod(codexBin, 0o755);
+    child = spawn(
+      process.execPath,
+      [
+        script,
+        "--repo",
+        repo,
+        "--range",
+        "HEAD~1..HEAD",
+        "--codex-bin",
+        codexBin,
+        "--summaries",
+        summaries,
+        "--output",
+        output,
+      ],
+      { stdio: "ignore" },
+    );
+    await waitFor(async () => (await stat(started)).isFile() ? true : undefined);
+    child.kill("SIGTERM");
+    const interruptedResult = await new Promise((resolve, reject) => {
+      child.once("error", reject);
+      child.once("close", (code, signal) => resolve({ code, signal }));
+    });
+    child = undefined;
+
+    const notes = JSON.parse(await readFile(summaries, "utf8"));
+    const snapshot = JSON.parse(await readFile(output, "utf8"));
+    assert.deepEqual(interruptedResult, { code: 0, signal: null });
+    assert.notEqual(notes.meta.status, "complete");
+    assert.notEqual(snapshot.notes?.complete, true);
+  } finally {
+    if (child && !child.killed) child.kill("SIGTERM");
+    await rm(repo, { recursive: true, force: true });
+  }
+});