Retry malformed file notes

scripts/generate-summaries.mjs

@@ -173,6 +173,7 @@ const proseCodePointLimit = 1_200;
 const detailItemLimit = 4;
 const riskItemLimit = 3;
 const listItemCodePointLimit = 500;
+const fileNoteAttemptLimit = 3;
 const jobsValue = option('--jobs') || '3';
 if (!/^[1-9]\d*$/.test(jobsValue) || Number(jobsValue) > 8) {
   fail('--jobs must be a number from 1 to 8');
@@ -1269,10 +1270,10 @@ try {
     }
     if (batch.length) batches.push(batch);
     let nextBatch = 0;
-    const requestBatch = async (index, batchPaths) => {
+    const requestBatch = async (index, batchPaths, attempt) => {
       const schemaPath = resolve(
         temporaryDirectory,
-        `summary-schema-${index + 1}.json`,
+        `summary-schema-${index + 1}-${attempt}.json`,
       );
       writeFileSync(
         schemaPath,
@@ -1291,7 +1292,7 @@ try {
       );
       const inputPath = resolve(
         temporaryDirectory,
-        `summary-input-${index + 1}.json`,
+        `summary-input-${index + 1}-${attempt}.json`,
       );
       writeFileSync(inputPath, input);
       const invocation = agentCommand({
@@ -1307,7 +1308,7 @@ try {
       });
 
       console.error(
-        `Asking ${selectedAgent} for batch ${index + 1} of ${batches.length} (${batchPaths.length} changed files)...`,
+        `Asking ${selectedAgent} for batch ${index + 1} of ${batches.length} (${batchPaths.length} changed files, attempt ${attempt} of ${fileNoteAttemptLimit})...`,
       );
       return requestAgent(
         invocation,
@@ -1321,45 +1322,73 @@ try {
     };
     const runBatch = async (index) => {
       const batchPaths = batches[index];
-      let outcome;
-      try {
-        outcome = await requestBatch(index, batchPaths);
-      } catch (error) {
-        if (interrupted) throw error;
-        const reason = failureReason(error);
-        console.error(
-          error instanceof Error ? error.message : String(error),
+      let pendingPaths = batchPaths;
+      for (
+        let attempt = 1;
+        attempt <= fileNoteAttemptLimit && pendingPaths.length;
+        attempt += 1
+      ) {
+        let outcome;
+        let requestFailed = false;
+        try {
+          outcome = await requestBatch(index, pendingPaths, attempt);
+        } catch (error) {
+          if (interrupted) throw error;
+          requestFailed = true;
+          const reason = failureReason(error);
+          console.error(
+            error instanceof Error ? error.message : String(error),
+          );
+          outcome = {
+            files: {},
+            failedFiles: pendingPaths.map((path) => ({ path, reason })),
+            errors: [],
+          };
+        }
+        const requestedPaths = new Set(pendingPaths);
+        const retryableFailures = requestFailed
+          ? []
+          : outcome.failedFiles.filter(
+              (failure) => requestedPaths.has(failure.path),
+            );
+        const finalAttempt =
+          requestFailed || attempt === fileNoteAttemptLimit;
+        const keptFailures = outcome.failedFiles.filter(
+          (failure) =>
+            requestedPaths.has(failure.path)
+              ? finalAttempt
+              : !completeFileNote(workingSummaries.files[failure.path]),
         );
-        outcome = {
-          files: {},
-          failedFiles: batchPaths.map((path) => ({ path, reason })),
-          errors: [],
+        workingSummaries = {
+          ...(workingSummaries.change
+            ? { change: workingSummaries.change }
+            : {}),
+          files: {
+            ...workingSummaries.files,
+            ...outcome.files,
+          },
+          meta: {
+            ...workingSummaries.meta,
+            status: 'generating',
+            generatedAt: new Date().toISOString(),
+          },
         };
-      }
-      workingSummaries = {
-        ...(workingSummaries.change
-          ? { change: workingSummaries.change }
-          : {}),
-        files: {
-          ...workingSummaries.files,
-          ...outcome.files,
-        },
-        meta: {
-          ...workingSummaries.meta,
-          status: 'generating',
-          generatedAt: new Date().toISOString(),
-        },
-      };
-      workingSummaries = addFailures(
-        workingSummaries,
-        outcome.failedFiles,
-        outcome.errors,
-      );
-      storeProgress(rawSnapshot, workingSummaries);
-      if (batchPaths.length) {
-        console.log(
-          `Wrote ${Object.keys(workingSummaries.files).length} of ${paths.length} agent notes to ${summariesPath}`,
+        workingSummaries = addFailures(
+          workingSummaries,
+          keptFailures,
+          finalAttempt || retryableFailures.length === 0
+            ? outcome.errors
+            : [],
         );
+        storeProgress(rawSnapshot, workingSummaries);
+        if (pendingPaths.length) {
+          console.log(
+            `Wrote ${Object.keys(workingSummaries.files).length} of ${paths.length} agent notes to ${summariesPath}`,
+          );
+        }
+        pendingPaths = [...new Set(
+          retryableFailures.map((failure) => failure.path),
+        )];
       }
     };
     const workers = Array.from(

tests/generate-summaries.test.mjs

@@ -265,6 +265,54 @@ process.stdout.write(JSON.stringify(response));
   return { bin, calls };
 }
 
+async function malformedFileNoteCodex(root, recoverOnCall) {
+  const bin = join(root, "malformed-file-note-codex.mjs");
+  const calls = join(root, "malformed-file-note-calls.jsonl");
+  await writeFile(
+    bin,
+    `#!/usr/bin/env node
+import { appendFileSync, existsSync, readFileSync } from "node:fs";
+const input = JSON.parse(readFileSync(0, "utf8"));
+const call = existsSync(${JSON.stringify(calls)})
+  ? readFileSync(${JSON.stringify(calls)}, "utf8").trim().split("\\n").length + 1
+  : 1;
+appendFileSync(
+  ${JSON.stringify(calls)},
+  JSON.stringify({
+    files: input.files.map((file) => file.path).sort(),
+    existing: Object.keys(input.existingFileNotes || {}).sort(),
+  }) + "\\n",
+);
+const note = (path) => ({
+  path,
+  title: "Note for " + path + " from call " + call,
+  what: "Explains " + path + ".",
+  why: "This file changed.",
+  details: [],
+  risks: [],
+});
+if (input.files.length) {
+  const files = input.files.map((file) => note(file.path));
+  const malformed = files.find((file) => file.path === "changed.txt");
+  if (malformed && call < ${recoverOnCall}) delete malformed.details;
+  process.stdout.write(JSON.stringify({ files }));
+} else {
+  process.stdout.write(JSON.stringify({
+    change: {
+      title: "Keep recovered notes",
+      summary: "Retries malformed file notes without replacing valid notes.",
+      why: "Completes the review in the same run when possible.",
+      highlights: [],
+      risks: [],
+    },
+  }));
+}
+`,
+  );
+  await chmod(bin, 0o755);
+  return { bin, calls };
+}
+
 function run(repo, args, options = {}) {
   return spawnSync(process.execPath, [script, "--repo", repo, ...args], {
     encoding: "utf8",
@@ -1111,6 +1159,99 @@ test("marks note generation as failed when Codex misses a changed file", async (
   }
 });
 
+test("retries only malformed file notes and keeps valid notes", async () => {
+  const repo = await makeRepo();
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+
+  try {
+    const codex = await malformedFileNoteCodex(repo, 2);
+    const result = run(repo, [
+      "--range",
+      "HEAD~1..HEAD",
+      "--codex-bin",
+      codex.bin,
+      "--jobs",
+      "1",
+      "--summaries",
+      summaries,
+      "--output",
+      output,
+    ]);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.deepEqual(await recordedCalls(codex.calls), [
+      { files: ["added.txt", "changed.txt"], existing: [] },
+      { files: ["changed.txt"], existing: ["added.txt"] },
+      { files: [], existing: ["added.txt", "changed.txt"] },
+    ]);
+    const writtenNotes = JSON.parse(await readFile(summaries, "utf8"));
+    assert.equal(writtenNotes.meta.status, "complete");
+    assert.equal(writtenNotes.files["added.txt"].title, "Note for added.txt from call 1");
+    assert.equal(writtenNotes.files["changed.txt"].title, "Note for changed.txt from call 2");
+    assert.ok(!Object.hasOwn(writtenNotes.meta, "failedFiles"));
+    const built = JSON.parse(await readFile(output, "utf8"));
+    assert.equal(built.notes.complete, true);
+    assert.equal(built.notes.completedFiles, 2);
+  } finally {
+    await rm(repo, { recursive: true, force: true });
+  }
+});
+
+test("stops malformed file note retries at the limit and retries on restart", async () => {
+  const repo = await makeRepo();
+  const summaries = join(repo, "notes.json");
+  const output = join(repo, "diff-data.json");
+
+  try {
+    const codex = await malformedFileNoteCodex(repo, 5);
+    const args = [
+      "--range",
+      "HEAD~1..HEAD",
+      "--codex-bin",
+      codex.bin,
+      "--jobs",
+      "1",
+      "--summaries",
+      summaries,
+      "--output",
+      output,
+    ];
+    const failed = run(repo, args);
+
+    assert.equal(failed.status, 1);
+    assert.deepEqual(await recordedCalls(codex.calls), [
+      { files: ["added.txt", "changed.txt"], existing: [] },
+      { files: ["changed.txt"], existing: ["added.txt"] },
+      { files: ["changed.txt"], existing: ["added.txt"] },
+      { files: [], existing: ["added.txt"] },
+    ]);
+    const failedNotes = JSON.parse(await readFile(summaries, "utf8"));
+    assert.equal(failedNotes.meta.status, "failed");
+    assert.equal(failedNotes.change.title, "Keep recovered notes");
+    assert.equal(failedNotes.files["added.txt"].title, "Note for added.txt from call 1");
+    assert.deepEqual(failedNotes.meta.failedFiles, [
+      {
+        path: "changed.txt",
+        reason: "changed.txt has missing: details",
+      },
+    ]);
+
+    const recovered = run(repo, args);
+    assert.equal(recovered.status, 0, recovered.stderr);
+    assert.deepEqual((await recordedCalls(codex.calls)).at(-1), {
+      files: ["changed.txt"],
+      existing: ["added.txt"],
+    });
+    const recoveredNotes = JSON.parse(await readFile(summaries, "utf8"));
+    assert.equal(recoveredNotes.meta.status, "complete");
+    assert.equal(recoveredNotes.change.title, "Keep recovered notes");
+    assert.ok(!Object.hasOwn(recoveredNotes.meta, "failedFiles"));
+  } finally {
+    await rm(repo, { recursive: true, force: true });
+  }
+});
+
 test("clears prior failure details after a successful snapshot retry", async () => {
   const directory = await mkdtemp(join(tmpdir(), "diffsplain-retry-"));
   const input = join(directory, "input.json");
@@ -1209,6 +1350,11 @@ test("keeps completed batches after malformed output or a provider exit", async
         built.files.find((file) => file.path === "added.txt").noteReady,
         true,
       );
+      const calls = await recordedCalls(codex.calls);
+      assert.equal(
+        calls.filter((call) => call.files[0]?.path === "changed.txt").length,
+        1,
+      );
     } finally {
       await rm(repo, { recursive: true, force: true });
     }

tests/presenter-recovery.test.mjs

@@ -172,8 +172,11 @@ test('keeps completed notes and resumes only queued work after cancellation', as
 import { appendFileSync, existsSync, readFileSync } from 'node:fs';
 const input = JSON.parse(readFileSync(0, 'utf8'));
 const paths = input.files.map((file) => file.path);
+const priorCalls = existsSync(${JSON.stringify(calls)})
+  ? readFileSync(${JSON.stringify(calls)}, 'utf8').trim().split('\\n').filter(Boolean).length
+  : 0;
 appendFileSync(${JSON.stringify(calls)}, JSON.stringify(paths) + '\\n');
-if (paths.includes('second.txt') && !existsSync(${JSON.stringify(resume)})) {
+if (paths.length && priorCalls === 1 && !existsSync(${JSON.stringify(resume)})) {
   Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 10_000);
 }
 const response = paths.length
@@ -198,18 +201,22 @@ process.stdout.write(JSON.stringify(response));
     await chmod(codex, 0o755);
     first = present(repo, summaries, output, codex);
 
-    await waitFor(async () => {
+    const progress = await waitFor(async () => {
       const notes = JSON.parse(await readFile(summaries, 'utf8'));
       const seen = recordedCalls(await readFile(calls, 'utf8'));
-      return notes.files?.['first.txt'] && seen.some((paths) => paths[0] === 'second.txt')
-        ? notes
+      const fileAttempts = seen.filter((paths) => paths.length);
+      return fileAttempts.length >= 2 && notes.files?.[fileAttempts[0][0]]
+        ? { notes, fileAttempts }
         : undefined;
     });
     assert.deepEqual(await stop(first), { code: 0, signal: null });
     first = undefined;
 
     const partial = JSON.parse(await readFile(summaries, 'utf8'));
-    assert.deepEqual(Object.keys(partial.files), ['first.txt']);
+    const completedPath = progress.fileAttempts[0][0];
+    const queuedPath = progress.fileAttempts[1][0];
+    assert.notEqual(completedPath, queuedPath);
+    assert.deepEqual(Object.keys(partial.files), [completedPath]);
     assert.equal(partial.meta.status, 'generating');
 
     await writeFile(resume, '');
@@ -222,8 +229,8 @@ process.stdout.write(JSON.stringify(response));
     assert.equal(complete.change.title, 'Recovered change');
 
     const attempted = recordedCalls(await readFile(calls, 'utf8'));
-    assert.equal(attempted.filter((paths) => paths[0] === 'first.txt').length, 1);
-    assert.equal(attempted.filter((paths) => paths[0] === 'second.txt').length, 2);
+    assert.equal(attempted.filter((paths) => paths[0] === completedPath).length, 1);
+    assert.equal(attempted.filter((paths) => paths[0] === queuedPath).length, 2);
     assert.equal(attempted.filter((paths) => paths.length === 0).length, 1);
   } finally {
     await stopIfRunning(first);