From 652e7c6d068aff2bca0699ac9755dab6dafe6ea9 Mon Sep 17 00:00:00 2001 From: Justin Ling Date: Wed, 12 Aug 2026 16:51:20 +0800 Subject: [PATCH 1/3] Retry malformed file notes - Retry only files with invalid notes - Preserve valid notes across attempts - Cover retry limits and restart recovery --- scripts/generate-summaries.mjs | 104 +++++++++++++--------- tests/generate-summaries.test.mjs | 141 ++++++++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 40 deletions(-) diff --git a/scripts/generate-summaries.mjs b/scripts/generate-summaries.mjs index e3dd190..1f7af34 100644 --- a/scripts/generate-summaries.mjs +++ b/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,68 @@ 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; + try { + outcome = await requestBatch(index, pendingPaths, attempt); + } catch (error) { + if (interrupted) throw error; + 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 = outcome.failedFiles.filter( + (failure) => requestedPaths.has(failure.path), ); - outcome = { - files: {}, - failedFiles: batchPaths.map((path) => ({ path, reason })), - errors: [], + const finalAttempt = attempt === fileNoteAttemptLimit; + const keptFailures = outcome.failedFiles.filter( + (failure) => + requestedPaths.has(failure.path) + ? finalAttempt + : !completeFileNote(workingSummaries.files[failure.path]), + ); + 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( diff --git a/tests/generate-summaries.test.mjs b/tests/generate-summaries.test.mjs index 42c9261..2fdef8f 100644 --- a/tests/generate-summaries.test.mjs +++ b/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"); From e306a770bacddcddb9f2a2b0fdc8b9a04b199836 Mon Sep 17 00:00:00 2001 From: Justin Ling Date: Wed, 12 Aug 2026 17:12:21 +0800 Subject: [PATCH 2/3] Stop retrying failed summary requests - Preserve completed batches after provider failures - Verify failed files are requested only once --- scripts/generate-summaries.mjs | 13 +++++++++---- tests/generate-summaries.test.mjs | 5 +++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/scripts/generate-summaries.mjs b/scripts/generate-summaries.mjs index 1f7af34..9f82757 100644 --- a/scripts/generate-summaries.mjs +++ b/scripts/generate-summaries.mjs @@ -1329,10 +1329,12 @@ try { 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), @@ -1344,10 +1346,13 @@ try { }; } const requestedPaths = new Set(pendingPaths); - const retryableFailures = outcome.failedFiles.filter( - (failure) => requestedPaths.has(failure.path), - ); - const finalAttempt = attempt === fileNoteAttemptLimit; + 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) diff --git a/tests/generate-summaries.test.mjs b/tests/generate-summaries.test.mjs index 2fdef8f..e5e0515 100644 --- a/tests/generate-summaries.test.mjs +++ b/tests/generate-summaries.test.mjs @@ -1350,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 }); } From e4dc8f332770c87935c42f36e86e295e942e76e7 Mon Sep 17 00:00:00 2001 From: Justin Ling Date: Wed, 12 Aug 2026 17:22:39 +0800 Subject: [PATCH 3/3] Stabilize presenter recovery test ordering - Track prior presenter calls before simulating recovery - Assert completed and queued files by observed order --- tests/presenter-recovery.test.mjs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/presenter-recovery.test.mjs b/tests/presenter-recovery.test.mjs index ba0e2b5..0cdda94 100644 --- a/tests/presenter-recovery.test.mjs +++ b/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);