diff --git a/scripts/build-diff-data.mjs b/scripts/build-diff-data.mjs index 50fa976..f59d97e 100644 --- a/scripts/build-diff-data.mjs +++ b/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) => diff --git a/scripts/generate-summaries.mjs b/scripts/generate-summaries.mjs index aeb42f2..f6582fd 100644 --- a/scripts/generate-summaries.mjs +++ b/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 }); diff --git a/scripts/present.mjs b/scripts/present.mjs index 0e3ddf1..192624c 100755 --- a/scripts/present.mjs +++ b/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 = { diff --git a/tests/generate-summaries.test.mjs b/tests/generate-summaries.test.mjs index 9d8ff31..8e93d05 100644 --- a/tests/generate-summaries.test.mjs +++ b/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 }); + } +});