diff --git a/docs/content/agent-notes.mdx b/docs/content/agent-notes.mdx index c298da3..c6b43a8 100644 --- a/docs/content/agent-notes.mdx +++ b/docs/content/agent-notes.mdx @@ -28,6 +28,19 @@ npx diffsplain \ The default batch size is 12 files. Smaller batches add the first notes to the open page sooner. +## File limits and failed notes + +Diffsplain sends a file’s full patch when it is at most 180,000 bytes. For a +larger patch, it sends the short patch excerpt shown in the page. If that +excerpt still makes the file input larger than 2,000,000 bytes, Diffsplain +rejects the file without starting an agent for it. + +A bad file note or a failed agent pass does not remove valid notes from other +files. Diffsplain publishes each complete note, marks the run as failed, exits +with an error, and lists each failed path and reason in `meta.failedFiles`. +It ignores notes for paths outside the requested batch and reports those paths +as failures. + ## Run one note pass Generate or revise notes without starting the page: diff --git a/scripts/build-diff-data.mjs b/scripts/build-diff-data.mjs index a2f0bc6..808d877 100644 --- a/scripts/build-diff-data.mjs +++ b/scripts/build-diff-data.mjs @@ -176,6 +176,22 @@ const completeChangeSummary = (value) => completeText(value.why) && completeList(value.highlights) && completeList(value.risks); +const failedFileRecords = (value) => + Array.isArray(value) + ? value + .filter( + (item) => + item && + typeof item.path === 'string' && + item.path.trim() && + typeof item.reason === 'string' && + item.reason.trim(), + ) + .map((item) => ({ + path: item.path.trim(), + reason: item.reason.trim(), + })) + : []; function fileSummary(path, value) { return { @@ -931,8 +947,19 @@ function build() { : undefined; const summariesAreFresh = !generatedFor || generatedFor === reviewFingerprint; const sourceSummaries = summariesAreFresh ? summaryDoc : {}; + const failedFiles = summariesAreFresh + ? failedFileRecords(summaryDoc.meta?.failedFiles) + : []; + const summaryErrors = summariesAreFresh + ? cleanList(summaryDoc.meta?.errors) + : []; + const failureByPath = new Map( + failedFiles.map((failure) => [failure.path, failure.reason]), + ); const summariesAreComplete = summariesAreFresh && + failedFiles.length === 0 && + summaryErrors.length === 0 && completeChangeSummary(sourceSummaries.change) && filesWithoutSummaries.every((file) => completeFileSummary(sourceSummaries.files?.[file.path]), @@ -954,6 +981,9 @@ function build() { noteReady: Boolean( completeFileSummary(sourceSummaries.files?.[file.path]), ), + ...(failureByPath.has(file.path) + ? { noteFailure: failureByPath.get(file.path) } + : {}), })); const change = changeSummary(sourceSummaries.change, target.changeDefaults); const content = { @@ -979,6 +1009,8 @@ function build() { status: noteStatus, completedFiles, totalFiles: filesWithoutSummaries.length, + ...(failedFiles.length ? { failedFiles } : {}), + ...(summaryErrors.length ? { errors: summaryErrors } : {}), ...(typeof summaryDoc.meta?.model === 'string' ? { model: summaryDoc.meta.model } : {}), diff --git a/scripts/generate-summaries.mjs b/scripts/generate-summaries.mjs index e1d3454..a63982d 100644 --- a/scripts/generate-summaries.mjs +++ b/scripts/generate-summaries.mjs @@ -132,6 +132,8 @@ if (!/^[1-9]\d*$/.test(batchSizeValue) || Number(batchSizeValue) > 50) { } const batchSize = Number(batchSizeValue); const batchByteLimit = 180_000; +const softFileByteLimit = 180_000; +const hardInputByteLimit = 2_000_000; const jobsValue = option('--jobs') || '3'; if (!/^[1-9]\d*$/.test(jobsValue) || Number(jobsValue) > 8) { fail('--jobs must be a number from 1 to 8'); @@ -241,8 +243,9 @@ function cleanSnapshot(snapshot) { .filter((file) => !excluded.has(file.path)) .map((file) => { const fullPatch = typeof file.patch === 'string' ? file.patch : ''; - const useSnippet = fullPatch.length > 180_000; - return { + const useSnippet = + Buffer.byteLength(fullPatch) > softFileByteLimit; + const summaryFile = { path: file.path, ...(file.oldPath ? { oldPath: file.oldPath } : {}), status: file.status, @@ -252,6 +255,16 @@ function cleanSnapshot(snapshot) { patch: useSnippet ? file.snippet : fullPatch, patchIsExcerpt: useSnippet, }; + const inputBytes = Buffer.byteLength(JSON.stringify(summaryFile)); + return { + ...summaryFile, + ...(inputBytes > hardInputByteLimit + ? { + summaryFailure: + `The file input is ${inputBytes} bytes after using its patch excerpt; the hard limit is ${hardInputByteLimit} bytes.`, + } + : {}), + }; }); return { @@ -292,12 +305,12 @@ function batchInput(snapshot, rawSnapshot, paths, existingFiles) { })), files: snapshot.files .filter((file) => selected.has(file.path)) - .map((file) => ({ ...file })), + .map(({ summaryFailure: _summaryFailure, ...file }) => file), ...(Object.keys(existingFileNotes).length ? { existingFileNotes } : {}), }; let encoded = JSON.stringify(result); - if (Buffer.byteLength(encoded) > 2_000_000) { + if (Buffer.byteLength(encoded) > hardInputByteLimit) { for (const file of result.files) { const source = rawSnapshot.files.find((item) => item.path === file.path); file.patch = source?.snippet || ''; @@ -305,9 +318,9 @@ function batchInput(snapshot, rawSnapshot, paths, existingFiles) { } encoded = JSON.stringify(result); } - if (Buffer.byteLength(encoded) > 2_000_000) { + if (Buffer.byteLength(encoded) > hardInputByteLimit) { throw new Error( - `The batch containing ${paths.join(', ')} is too large to summarize`, + `The agent input containing ${paths.join(', ')} is larger than the ${hardInputByteLimit}-byte hard limit`, ); } return encoded; @@ -426,97 +439,174 @@ function exactFields(value, fields, label) { } } -function normalizeResponse( - value, - paths, - { includeChange = true } = {}, -) { - if (!value || typeof value !== 'object' || Array.isArray(value)) { +function isObject(value) { + return Boolean(value) && + typeof value === 'object' && + !Array.isArray(value); +} + +function responseObject(value) { + if (!isObject(value)) { throw new Error('Agent response must be an object'); } - const allowed = new Set(['change', 'files']); - if (Object.keys(value).some((field) => !allowed.has(field))) { + return value; +} + +function unsupportedResponseFields(value) { + return Object.keys(value).filter( + (field) => !['change', 'files'].includes(field), + ); +} + +function normalizeChangeResponse(value) { + const response = responseObject(value); + if (unsupportedResponseFields(response).length) { throw new Error('Agent response has unsupported fields'); } - if (includeChange) { - exactFields( - value.change, - ['title', 'summary', 'why', 'highlights', 'risks'], - 'Change note', - ); + exactFields( + response.change, + ['title', 'summary', 'why', 'highlights', 'risks'], + 'Change note', + ); + return { + title: normalizedText(response.change.title, 'change.title'), + summary: normalizedText(response.change.summary, 'change.summary'), + why: normalizedText(response.change.why, 'change.why'), + highlights: normalizedList( + response.change.highlights, + 'change.highlights', + ), + risks: normalizedList(response.change.risks, 'change.risks'), + }; +} + +function arrayFileEntry(note) { + if (!isObject(note)) { + return { error: 'Agent response has a malformed file note' }; + } + const path = typeof note.path === 'string' ? note.path.trim() : ''; + return path + ? { entry: { path, note, arrayForm: true } } + : { error: 'Agent response has a file note without a path' }; +} + +function arrayFileEntries(notes) { + const result = { entries: [], errors: [] }; + for (const note of notes) { + const item = arrayFileEntry(note); + if (item.entry) result.entries.push(item.entry); + if (item.error) result.errors.push(item.error); } - let fileValues = {}; - if (!paths.length) { + return result; +} + +function fileResponseEntries(value) { + const response = responseObject(value); + const errors = unsupportedResponseFields(response).map( + (field) => `Agent response has unsupported field: ${field}`, + ); + if (Array.isArray(response.files)) { + const arrayResult = arrayFileEntries(response.files); return { - ...(includeChange - ? { - change: { - title: normalizedText(value.change.title, 'change.title'), - summary: normalizedText( - value.change.summary, - 'change.summary', - ), - why: normalizedText(value.change.why, 'change.why'), - highlights: normalizedList( - value.change.highlights, - 'change.highlights', - ), - risks: normalizedList(value.change.risks, 'change.risks'), - }, - } - : {}), - files: {}, + entries: arrayResult.entries, + errors: [...errors, ...arrayResult.errors], }; } - if (Array.isArray(value.files)) { - fileValues = {}; - for (const note of value.files) { - exactFields( - note, - ['path', 'title', 'what', 'why', 'details', 'risks'], - 'File note', - ); - const path = normalizedText(note.path, 'files.path'); - if (fileValues[path]) throw new Error(`Duplicate file note: ${path}`); - fileValues[path] = note; - } - } else { - fileValues = value.files; + if (!isObject(response.files)) { + throw new Error('Agent response has no file notes object'); } - exactFields(fileValues, paths, 'File notes'); + return { + entries: Object.entries(response.files).map(([path, note]) => ({ + path, + note, + arrayForm: false, + })), + errors, + }; +} - const files = {}; - for (const path of paths) { - const note = fileValues[path]; - if (!Array.isArray(value.files)) { - exactFields(note, ['title', 'what', 'why', 'details', 'risks'], path); +function normalizeFileNote(note, path, arrayForm) { + exactFields( + note, + [ + ...(arrayForm ? ['path'] : []), + 'title', + 'what', + 'why', + 'details', + 'risks', + ], + path, + ); + return { + title: normalizedText(note.title, `${path}.title`), + what: normalizedText(note.what, `${path}.what`), + why: normalizedText(note.why, `${path}.why`), + details: normalizedList(note.details, `${path}.details`), + risks: normalizedList(note.risks, `${path}.risks`), + }; +} + +function indexFileEntries(entries, expected) { + const byPath = new Map(); + const failedFiles = []; + for (const entry of entries) { + if (!expected.has(entry.path)) { + failedFiles.push({ + path: entry.path, + reason: 'Agent output included a file outside this batch.', + }); + continue; } - files[path] = { - title: normalizedText(note.title, `${path}.title`), - what: normalizedText(note.what, `${path}.what`), - why: normalizedText(note.why, `${path}.why`), - details: normalizedList(note.details, `${path}.details`), - risks: normalizedList(note.risks, `${path}.risks`), + const values = byPath.get(entry.path) || []; + byPath.set(entry.path, [...values, entry]); + } + return { byPath, failedFiles }; +} + +function normalizeFileEntry(path, values) { + if (values.length !== 1) { + return { + failure: { + path, + reason: values.length + ? 'Agent output repeated this file.' + : 'Agent output omitted this file.', + }, + }; + } + try { + return { + note: normalizeFileNote( + values[0].note, + path, + values[0].arrayForm, + ), + }; + } catch (error) { + return { + failure: { + path, + reason: error instanceof Error ? error.message : String(error), + }, }; } +} - return { - ...(includeChange - ? { - change: { - title: normalizedText(value.change.title, 'change.title'), - summary: normalizedText(value.change.summary, 'change.summary'), - why: normalizedText(value.change.why, 'change.why'), - highlights: normalizedList( - value.change.highlights, - 'change.highlights', - ), - risks: normalizedList(value.change.risks, 'change.risks'), - }, - } - : {}), - files, - }; +function normalizeFileResponse(value, paths) { + const expected = new Set(paths); + const files = {}; + const { entries, errors } = fileResponseEntries(value); + const indexed = indexFileEntries(entries, expected); + for (const path of paths) { + const result = normalizeFileEntry( + path, + indexed.byPath.get(path) || [], + ); + if (result.note) files[path] = result.note; + if (result.failure) indexed.failedFiles.push(result.failure); + } + return { files, failedFiles: indexed.failedFiles, errors }; } function writeJsonAtomic(file, value) { @@ -526,6 +616,46 @@ function writeJsonAtomic(file, value) { renameSync(temporary, file); } +function metadataList(value) { + return Array.isArray(value) ? value : []; +} + +function summaryFailureState(snapshot, summaries) { + const failedFiles = metadataList(summaries.meta?.failedFiles); + const errors = metadataList(summaries.meta?.errors); + const complete = [ + failedFiles.length === 0, + errors.length === 0, + completeChangeNote(summaries.change), + snapshot.files.every((file) => + completeFileNote(summaries.files?.[file.path]), + ), + ].every(Boolean); + return { failedFiles, errors, complete }; +} + +function snapshotFileWithNote(file, summaries, failureByPath) { + const nextFile = { ...file }; + delete nextFile.noteFailure; + const note = summaries.files?.[file.path]; + if (completeFileNote(note)) { + return { ...nextFile, summary: note, noteReady: true }; + } + const failure = failureByPath.get(file.path); + return { + ...nextFile, + noteReady: false, + ...(failure ? { noteFailure: failure } : {}), + }; +} + +function notesWithoutFailures(notes = {}) { + const nextNotes = { ...notes }; + delete nextNotes.failedFiles; + delete nextNotes.errors; + return nextNotes; +} + function publishSnapshot(snapshot, summaries) { const current = readJson(outputPath, null); const reviewFingerprint = snapshot.notes?.reviewFingerprint; @@ -537,17 +667,13 @@ function publishSnapshot(snapshot, summaries) { throw new Error('The diff changed while agent notes were being written'); } - const complete = - completeChangeNote(summaries.change) && - snapshot.files.every((file) => - completeFileNote(summaries.files?.[file.path]), - ); - const files = snapshot.files.map((file) => { - const note = summaries.files?.[file.path]; - return completeFileNote(note) - ? { ...file, summary: note, noteReady: true } - : { ...file, noteReady: false }; - }); + const state = summaryFailureState(snapshot, summaries); + const failureByPath = new Map( + state.failedFiles.map((failure) => [failure.path, failure.reason]), + ); + const files = snapshot.files.map((file) => + snapshotFileWithNote(file, summaries, failureByPath), + ); const content = { ...snapshot, ...(completeChangeNote(summaries.change) @@ -555,15 +681,19 @@ function publishSnapshot(snapshot, summaries) { : {}), files, notes: { - ...snapshot.notes, + ...notesWithoutFailures(snapshot.notes), generatedFor: reviewFingerprint, fresh: true, - complete, - status: complete + complete: state.complete, + status: state.complete ? 'complete' : summaries.meta?.status || 'generating', completedFiles: files.filter((file) => file.noteReady).length, totalFiles: files.length, + ...(state.failedFiles.length + ? { failedFiles: state.failedFiles } + : {}), + ...(state.errors.length ? { errors: state.errors } : {}), ...(model ? { model } : {}), ...(reasoning ? { reasoning } : {}), }, @@ -597,6 +727,34 @@ function readJson(file, fallback) { } } +function addFailures(summaries, failedFiles = [], errors = []) { + const priorFailedFiles = metadataList(summaries.meta?.failedFiles); + const priorErrors = metadataList(summaries.meta?.errors); + const uniqueFailures = new Map( + [...priorFailedFiles, ...failedFiles].map((failure) => [ + `${failure.path}\0${failure.reason}`, + failure, + ]), + ); + const nextErrors = [...new Set([...priorErrors, ...errors])]; + return { + ...summaries, + meta: { + ...summaries.meta, + ...(uniqueFailures.size + ? { failedFiles: [...uniqueFailures.values()] } + : {}), + ...(nextErrors.length ? { errors: nextErrors } : {}), + }, + }; +} + +function failureReason(error) { + const message = error instanceof Error ? error.message : String(error); + return message.split('\n').find((line) => line.trim())?.trim() || + 'Agent note generation failed.'; +} + function runAgent(invocation, input) { return new Promise((resolvePromise, rejectPromise) => { const child = spawn(invocation.command, invocation.args, { @@ -651,7 +809,7 @@ function runAgent(invocation, input) { ); return; } - resolvePromise(stdoutText); + resolvePromise({ stdout: stdoutText, stderr: stderrText }); }); if (invocation.input === 'stdin') child.stdin.end(input); else child.stdin.end(); @@ -763,8 +921,12 @@ try { const fileFingerprints = Object.fromEntries( paths.map((path) => [path, fileFingerprint(rawFiles.get(path))]), ); + const summaryFiles = new Map( + snapshot.files.map((file) => [file.path, file]), + ); const reusableFiles = {}; const changedPaths = []; + const inputFailures = []; for (const path of paths) { if ( !force && @@ -772,6 +934,11 @@ try { completeFileNote(previousFiles[path]) ) { reusableFiles[path] = previousFiles[path]; + } else if (summaryFiles.get(path)?.summaryFailure) { + inputFailures.push({ + path, + reason: summaryFiles.get(path).summaryFailure, + }); } else { changedPaths.push(path); } @@ -784,7 +951,7 @@ try { const needsGeneration = changedPaths.length > 0 || changeNeedsRefresh; workingSnapshot = rawSnapshot; - workingSummaries = { + workingSummaries = addFailures({ ...(!changeNeedsRefresh ? { change: previousSummaries.change } : {}), files: reusableFiles, meta: { @@ -795,11 +962,15 @@ try { : previousSummaries.meta?.generatedAt ? { generatedAt: previousSummaries.meta.generatedAt } : {}), - status: needsGeneration ? 'generating' : 'complete', + status: needsGeneration + ? 'generating' + : inputFailures.length + ? 'failed' + : 'complete', ...(model ? { model } : {}), ...(reasoning ? { reasoning } : {}), }, - }; + }, inputFailures); writeJsonAtomic(summariesPath, workingSummaries); publish(rawSnapshot, workingSummaries); @@ -823,8 +994,7 @@ try { } if (batch.length) batches.push(batch); let nextBatch = 0; - const runBatch = async (index) => { - const batchPaths = batches[index]; + const requestBatch = async (index, batchPaths) => { const schemaPath = resolve( temporaryDirectory, `summary-schema-${index + 1}.json`, @@ -864,18 +1034,41 @@ try { console.error( `Asking ${selectedAgent} for batch ${index + 1} of ${batches.length} (${batchPaths.length} changed files)...`, ); - const stdout = await runAgent(invocation, input); - const response = parseAgentResponse(selectedAgent, stdout); - const normalized = normalizeResponse(response, batchPaths, { - includeChange: false, - }); + const result = await runAgent(invocation, input); + if (result.stderr.trim()) { + console.error( + `${selectedAgent} wrote diagnostic output:\n${result.stderr.trim()}`, + ); + } + return normalizeFileResponse( + parseAgentResponse(selectedAgent, result.stdout), + batchPaths, + ); + }; + 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), + ); + outcome = { + files: {}, + failedFiles: batchPaths.map((path) => ({ path, reason })), + errors: [], + }; + } workingSummaries = { ...(workingSummaries.change ? { change: workingSummaries.change } : {}), files: { ...workingSummaries.files, - ...normalized.files, + ...outcome.files, }, meta: { ...workingSummaries.meta, @@ -883,6 +1076,11 @@ try { generatedAt: new Date().toISOString(), }, }; + workingSummaries = addFailures( + workingSummaries, + outcome.failedFiles, + outcome.errors, + ); writeJsonAtomic(summariesPath, workingSummaries); publish(rawSnapshot, workingSummaries); if (batchPaths.length) { @@ -894,7 +1092,7 @@ try { const workers = Array.from( { length: Math.min(jobs, batches.length) }, async () => { - while (nextBatch < batches.length) { + while (!interrupted && nextBatch < batches.length) { const index = nextBatch; nextBatch += 1; await runBatch(index); @@ -903,83 +1101,106 @@ try { ); await Promise.all(workers); if (changeNeedsRefresh) { - const schemaPath = resolve( - temporaryDirectory, - 'change-summary-schema.json', - ); - const schema = outputSchema([]); - writeFileSync( - schemaPath, - `${JSON.stringify(schema, null, 2)}\n`, - ); - const input = batchInput( - snapshot, - rawSnapshot, - [], - workingSummaries.files, - ); - const inputPath = resolve( - temporaryDirectory, - 'change-summary-input.json', - ); - writeFileSync(inputPath, input); - const invocation = agentCommand({ - agent: selectedAgent, - binary: agentBinary, - model, - reasoning, - prompt: promptFor([]), - schema, - schemaPath, - inputPath, - workingDirectory: root, - }); - console.error(`Asking ${selectedAgent} for the change note...`); - const stdout = await runAgent(invocation, input); - const normalized = normalizeResponse( - parseAgentResponse(selectedAgent, stdout), - [], - ); - workingSummaries = { - change: normalized.change, - files: workingSummaries.files, - meta: { - ...workingSummaries.meta, - status: 'complete', - generatedAt: new Date().toISOString(), - }, - }; - writeJsonAtomic(summariesPath, workingSummaries); - publish(rawSnapshot, workingSummaries); - console.log(`Updated the change note in ${summariesPath}`); - } else if (batches.length) { - workingSummaries = { - ...workingSummaries, - meta: { - ...workingSummaries.meta, - status: 'complete', - generatedAt: new Date().toISOString(), - }, - }; - writeJsonAtomic(summariesPath, workingSummaries); - publish(rawSnapshot, workingSummaries); + try { + const schemaPath = resolve( + temporaryDirectory, + 'change-summary-schema.json', + ); + const schema = outputSchema([]); + writeFileSync( + schemaPath, + `${JSON.stringify(schema, null, 2)}\n`, + ); + const input = batchInput( + snapshot, + rawSnapshot, + [], + workingSummaries.files, + ); + const inputPath = resolve( + temporaryDirectory, + 'change-summary-input.json', + ); + writeFileSync(inputPath, input); + const invocation = agentCommand({ + agent: selectedAgent, + binary: agentBinary, + model, + reasoning, + prompt: promptFor([]), + schema, + schemaPath, + inputPath, + workingDirectory: root, + }); + console.error(`Asking ${selectedAgent} for the change note...`); + const result = await runAgent(invocation, input); + if (result.stderr.trim()) { + console.error( + `${selectedAgent} wrote diagnostic output:\n${result.stderr.trim()}`, + ); + } + const normalized = normalizeChangeResponse( + parseAgentResponse(selectedAgent, result.stdout), + ); + workingSummaries = { + change: normalized, + files: workingSummaries.files, + meta: workingSummaries.meta, + }; + console.log(`Updated the change note in ${summariesPath}`); + } catch (error) { + if (interrupted) throw error; + console.error( + error instanceof Error ? error.message : String(error), + ); + workingSummaries = addFailures( + workingSummaries, + [], + [`Change note: ${failureReason(error)}`], + ); + } } - if (batches.length === 0) { + const failedFiles = workingSummaries.meta.failedFiles || []; + const generationErrors = workingSummaries.meta.errors || []; + const complete = + failedFiles.length === 0 && + generationErrors.length === 0 && + completeChangeNote(workingSummaries.change) && + paths.every((path) => + completeFileNote(workingSummaries.files[path]), + ); + workingSummaries = { + ...workingSummaries, + meta: { + ...workingSummaries.meta, + status: complete ? 'complete' : 'failed', + generatedAt: new Date().toISOString(), + }, + }; + writeJsonAtomic(summariesPath, workingSummaries); + publish(rawSnapshot, workingSummaries); + if (batches.length === 0 && inputFailures.length === 0) { console.log('No file summaries changed.'); } + for (const failure of failedFiles) { + console.error(`${failure.path}: ${failure.reason}`); + } + for (const error of generationErrors) console.error(error); + if (!complete) process.exitCode = 1; console.log(`Rebuilt ${outputPath}`); } } catch (error) { if (!interrupted && workingSummaries && workingSnapshot) { try { - workingSummaries = { + workingSummaries = addFailures({ ...workingSummaries, meta: { ...workingSummaries.meta, status: 'failed', generatedAt: new Date().toISOString(), }, - }; + }, [], [failureReason(error)]); writeJsonAtomic(summariesPath, workingSummaries); publish(workingSnapshot, workingSummaries); } catch {} diff --git a/tests/coding-agents.test.mjs b/tests/coding-agents.test.mjs index 8828bab..b57d971 100644 --- a/tests/coding-agents.test.mjs +++ b/tests/coding-agents.test.mjs @@ -1,14 +1,38 @@ import assert from 'node:assert/strict'; +import { chmod, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import test from 'node:test'; import { agentDisabledReason, agentCommand, codingAgentBinary, + findCommand, parseAgentResponse, selectCodingAgent, summaryAgentEnvironment, } from '../scripts/coding-agents.mjs'; +test('discovers executable providers on the configured path', async () => { + const directory = await mkdtemp(join(tmpdir(), 'diffsplain-provider-')); + const executable = join(directory, 'summary-agent'); + const plainFile = join(directory, 'plain-file'); + try { + await writeFile(executable, '#!/bin/sh\nexit 0\n'); + await chmod(executable, 0o755); + await writeFile(plainFile, 'not executable\n'); + const options = { + env: { PATH: directory }, + platform: 'linux', + }; + assert.equal(await findCommand('summary-agent', options), executable); + assert.equal(await findCommand('plain-file', options), undefined); + assert.equal(await findCommand('missing-agent', options), undefined); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + test('selects the first available agent in fallback order', async () => { const checked = []; const selected = await selectCodingAgent(undefined, async (agent) => { diff --git a/tests/generate-summaries.test.mjs b/tests/generate-summaries.test.mjs index fdb8fbd..f89b331 100644 --- a/tests/generate-summaries.test.mjs +++ b/tests/generate-summaries.test.mjs @@ -15,6 +15,29 @@ import test from "node:test"; const script = new URL("../scripts/generate-summaries.mjs", import.meta.url) .pathname; +const summaryEnvironmentNames = new Set([ + "CODEX_HOME", + "COMSPEC", + "HOME", + "HOMEDRIVE", + "HOMEPATH", + "HTTP_PROXY", + "HTTPS_PROXY", + "LANG", + "LC_ALL", + "NODE_EXTRA_CA_CERTS", + "NO_PROXY", + "PATH", + "SSL_CERT_FILE", + "SystemRoot", + "SYSTEMROOT", + "TEMP", + "TERM", + "TMP", + "TMPDIR", + "USERPROFILE", + "__CF_USER_TEXT_ENCODING", +]); function git(repo, ...args) { return execFileSync("git", ["-C", repo, ...args], { @@ -117,6 +140,70 @@ async function recordedCalls(file) { .map((line) => JSON.parse(line)); } +async function containmentCodex(root, mode = "valid") { + const bin = join(root, `containment-${mode}-codex.mjs`); + const calls = join(root, `containment-${mode}-calls.jsonl`); + await writeFile( + bin, + `#!/usr/bin/env node +import { appendFileSync, readFileSync } from "node:fs"; +const inputText = readFileSync(0, "utf8"); +const input = JSON.parse(inputText); +appendFileSync( + ${JSON.stringify(calls)}, + JSON.stringify({ + args: process.argv.slice(2), + cwd: process.cwd(), + envKeys: Object.keys(process.env).sort(), + files: input.files.map((file) => ({ + path: file.path, + patchBytes: Buffer.byteLength(file.patch || ""), + patchIsExcerpt: file.patchIsExcerpt, + })), + inputText, + }) + "\\n", +); +const selected = input.files[0]?.path; +if (${JSON.stringify(mode)} === "malformed" && selected === "changed.txt") { + process.stdout.write("{not json"); + process.exit(0); +} +if (${JSON.stringify(mode)} === "exit" && selected === "changed.txt") { + process.stderr.write("provider diagnostic for changed.txt\\n"); + process.exit(7); +} +if (${JSON.stringify(mode)} === "diagnostic" && input.files.length) { + process.stderr.write("non-fatal provider diagnostic\\n"); +} +const note = (path) => ({ + path, + title: "Note for " + path, + what: "Explains " + path + ".", + why: "This file changed.", + details: [], + risks: [], +}); +const response = input.files.length + ? { files: input.files.map((file) => note(file.path)) } + : { + change: { + title: "Contained notes", + summary: "Keeps valid file notes.", + why: "Reports failed files without dropping good notes.", + highlights: [], + risks: [], + }, + }; +if (${JSON.stringify(mode)} === "extra" && input.files.length) { + response.files.push(note("outside.txt")); +} +process.stdout.write(JSON.stringify(response)); +`, + ); + await chmod(bin, 0o755); + return { bin, calls }; +} + function run(repo, args, options = {}) { return spawnSync(process.execPath, [script, "--repo", repo, ...args], { encoding: "utf8", @@ -152,6 +239,85 @@ function notes(files) { }; } +function snapshot(files) { + return { + version: "input", + generatedAt: new Date().toISOString(), + repo: { + name: "fixture", + root: "/fixture", + base: "base", + head: "head", + target: { kind: "range" }, + }, + change: { + title: "Contain file failures", + summary: "Tests summary input limits.", + why: "Keeps valid notes.", + highlights: [], + risks: [], + }, + files: files.map((file) => ({ + status: "modified", + additions: 1, + deletions: 1, + isBinary: false, + isTruncated: true, + totalDiffLines: 1, + ...file, + })), + notes: { + reviewFingerprint: "a".repeat(64), + fresh: false, + complete: false, + status: "idle", + completedFiles: 0, + totalFiles: files.length, + }, + }; +} + +async function limitFixture(directory) { + const paths = { + summaries: join(directory, "notes.json"), + input: join(directory, "input.json"), + output: join(directory, "output.json"), + }; + await writeFile( + paths.input, + JSON.stringify( + snapshot([ + { path: "small.txt", patch: "small", snippet: "small" }, + { + path: "soft.txt", + patch: "s".repeat(180_001), + snippet: "short excerpt", + }, + { + path: "hard.txt", + patch: "h".repeat(2_000_100), + snippet: "h".repeat(2_000_100), + }, + ]), + ), + ); + return paths; +} + +function assertFileLimitCalls(calls) { + const fileInputs = calls.flatMap((call) => call.files); + assert.ok(fileInputs.some((file) => file.path === "small.txt")); + assert.ok( + fileInputs.some( + (file) => + file.path === "soft.txt" && + file.patchIsExcerpt === true && + file.patchBytes < 180_000, + ), + ); + assert.ok(!fileInputs.some((file) => file.path === "hard.txt")); +} + test("generates notes with Codex and rebuilds a selected range", async () => { const repo = await makeRepo(); const summaries = join(repo, "notes.json"); @@ -236,6 +402,57 @@ test("generates notes with Codex and rebuilds a selected range", async () => { } }); +test("runs a discovered provider with the summary process boundary", async () => { + const repo = await makeRepo(); + const summaries = join(repo, "notes.json"); + const output = join(repo, "diff-data.json"); + + try { + const codex = await containmentCodex(repo, "diagnostic"); + const result = run( + repo, + [ + "--range", + "HEAD~1..HEAD", + "--codex-bin", + codex.bin, + "--summaries", + summaries, + "--output", + output, + ], + { env: { ...process.env, PRIVATE_AGENT_TOKEN: "do-not-pass" } }, + ); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stderr, /non-fatal provider diagnostic/); + + const [fileCall] = await recordedCalls(codex.calls); + const input = JSON.parse(fileCall.inputText); + assert.deepEqual( + input.files.map((file) => file.path), + ["added.txt", "changed.txt"], + ); + assert.equal(fileCall.args[0], "exec"); + assert.equal( + fileCall.args[fileCall.args.indexOf("-C") + 1].replace( + /^\/private/, + "", + ), + fileCall.cwd.replace(/^\/private/, ""), + ); + assert.match(fileCall.cwd, /diffsplain-agent-/); + assert.ok(!fileCall.envKeys.includes("PRIVATE_AGENT_TOKEN")); + assert.deepEqual( + fileCall.envKeys.filter( + (name) => !summaryEnvironmentNames.has(name), + ), + [], + ); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + test("generates notes with Claude, Copilot, and OpenCode", async () => { for (const agent of ["claude", "copilot", "opencode"]) { const repo = await makeRepo(); @@ -600,16 +817,205 @@ test("marks note generation as failed when Codex misses a changed file", async ( assert.match(result.stderr, /added\.txt|every changed file|missing/i); const writtenNotes = JSON.parse(await readFile(summaries, "utf8")); assert.equal(writtenNotes.meta.status, "failed"); - assert.deepEqual(writtenNotes.files, {}); + assert.deepEqual(Object.keys(writtenNotes.files), ["changed.txt"]); + assert.deepEqual(writtenNotes.meta.failedFiles, [ + { + path: "added.txt", + reason: "Agent output omitted this file.", + }, + ]); const snapshot = JSON.parse(await readFile(output, "utf8")); assert.equal(snapshot.notes.status, "failed"); - assert.equal(snapshot.notes.completedFiles, 0); + assert.equal(snapshot.notes.completedFiles, 1); + assert.equal( + snapshot.files.find((file) => file.path === "changed.txt").noteReady, + true, + ); + assert.match( + snapshot.files.find((file) => file.path === "added.txt").noteFailure, + /omitted/i, + ); } 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"); + const summaries = join(directory, "notes.json"); + const output = join(directory, "output.json"); + + try { + const prior = snapshot([ + { + path: "changed.txt", + patch: "changed patch", + snippet: "changed excerpt", + noteFailure: "The prior agent failed.", + }, + ]); + prior.notes.status = "failed"; + prior.notes.failedFiles = [ + { path: "changed.txt", reason: "The prior agent failed." }, + ]; + prior.notes.errors = ["The prior provider stopped."]; + await writeFile(input, JSON.stringify(prior)); + const codex = await fakeCodex( + directory, + notes({ + "changed.txt": { + title: "Recover the note", + what: "Writes a valid note on retry.", + why: "Clears prior failure details.", + details: [], + risks: [], + }, + }), + ); + + const result = run(directory, [ + "--snapshot", + input, + "--codex-bin", + codex.bin, + "--summaries", + summaries, + "--output", + output, + ]); + + assert.equal(result.status, 0, result.stderr); + const retried = JSON.parse(await readFile(output, "utf8")); + assert.equal(retried.notes.status, "complete"); + assert.equal(retried.notes.complete, true); + assert.ok(!Object.hasOwn(retried.notes, "failedFiles")); + assert.ok(!Object.hasOwn(retried.notes, "errors")); + assert.ok(!Object.hasOwn(retried.files[0], "noteFailure")); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + +test("keeps completed batches after malformed output or a provider exit", async () => { + for (const mode of ["malformed", "exit"]) { + const repo = await makeRepo(); + const summaries = join(repo, "notes.json"); + const output = join(repo, "diff-data.json"); + try { + const codex = await containmentCodex(repo, mode); + const result = run(repo, [ + "--range", + "HEAD~1..HEAD", + "--codex-bin", + codex.bin, + "--batch-size", + "1", + "--jobs", + "1", + "--summaries", + summaries, + "--output", + output, + ]); + + assert.equal(result.status, 1); + assert.match( + result.stderr, + mode === "malformed" + ? /valid summary JSON/ + : /provider diagnostic for changed\.txt/, + ); + const writtenNotes = JSON.parse(await readFile(summaries, "utf8")); + assert.deepEqual(Object.keys(writtenNotes.files), ["added.txt"]); + assert.deepEqual( + writtenNotes.meta.failedFiles.map((failure) => failure.path), + ["changed.txt"], + ); + const built = JSON.parse(await readFile(output, "utf8")); + assert.equal(built.notes.completedFiles, 1); + assert.equal( + built.files.find((file) => file.path === "added.txt").noteReady, + true, + ); + } finally { + await rm(repo, { recursive: true, force: true }); + } + } +}); + +test("keeps valid notes and rejects output for an extra path", async () => { + const repo = await makeRepo(); + const summaries = join(repo, "notes.json"); + const output = join(repo, "diff-data.json"); + try { + const codex = await containmentCodex(repo, "extra"); + const result = run(repo, [ + "--range", + "HEAD~1..HEAD", + "--codex-bin", + codex.bin, + "--summaries", + summaries, + "--output", + output, + ]); + + assert.equal(result.status, 1); + assert.match(result.stderr, /outside\.txt/); + const writtenNotes = JSON.parse(await readFile(summaries, "utf8")); + assert.deepEqual(Object.keys(writtenNotes.files).sort(), [ + "added.txt", + "changed.txt", + ]); + assert.deepEqual(writtenNotes.meta.failedFiles, [ + { + path: "outside.txt", + reason: "Agent output included a file outside this batch.", + }, + ]); + const built = JSON.parse(await readFile(output, "utf8")); + assert.equal(built.notes.status, "failed"); + assert.equal(built.notes.completedFiles, 2); + assert.equal(built.notes.complete, false); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + +test("uses an excerpt at the soft limit and rejects the hard limit", async () => { + const directory = await mkdtemp(join(tmpdir(), "diffsplain-limits-")); + try { + const paths = await limitFixture(directory); + const codex = await containmentCodex(directory); + const result = run(directory, [ + "--snapshot", + paths.input, + "--codex-bin", + codex.bin, + "--summaries", + paths.summaries, + "--output", + paths.output, + ]); + + assert.equal(result.status, 1); + assertFileLimitCalls(await recordedCalls(codex.calls)); + const writtenNotes = JSON.parse( + await readFile(paths.summaries, "utf8"), + ); + assert.deepEqual(Object.keys(writtenNotes.files).sort(), [ + "small.txt", + "soft.txt", + ]); + assert.equal(writtenNotes.meta.failedFiles[0].path, "hard.txt"); + assert.match(writtenNotes.meta.failedFiles[0].reason, /hard limit/i); + } finally { + await rm(directory, { recursive: true, force: true }); + } +}); + test("shows coding agent stderr when note generation fails", async () => { const repo = await makeRepo(); const summaries = join(repo, "notes.json"); @@ -753,6 +1159,106 @@ process.stdout.write(JSON.stringify({ } }); +test("stops scheduling batches after an interruption", async () => { + const repo = await makeRepo(); + const summaries = join(repo, "notes.json"); + const output = join(repo, "diff-data.json"); + const codexBin = join(repo, "interruptible-codex.mjs"); + const calls = join(repo, "codex-calls.jsonl"); + let child; + + try { + await writeFile( + codexBin, + `#!/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({ call }) + "\\n"); +if (call === 1) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 30_000); +} +const note = (path) => ({ + path, + title: "Note for " + path, + what: "Explains " + path + ".", + why: "This file changed.", + details: [], + risks: [], +}); +process.stdout.write(JSON.stringify( + input.files.length + ? { files: input.files.map((file) => note(file.path)) } + : { + change: { + title: "Interrupted notes", + summary: "Stops after a termination signal.", + why: "Avoids starting more agent work.", + highlights: [], + risks: [], + }, + }, +)); +`, + ); + await chmod(codexBin, 0o755); + + child = spawn( + process.execPath, + [ + script, + "--repo", + repo, + "--range", + "HEAD~1..HEAD", + "--codex-bin", + codexBin, + "--batch-size", + "1", + "--jobs", + "1", + "--summaries", + summaries, + "--output", + output, + ], + { encoding: "utf8", stdio: "pipe" }, + ); + + await waitFor(async () => { + const recorded = await readFile(calls, "utf8"); + return recorded.trim() ? true : undefined; + }); + child.kill("SIGTERM"); + const result = await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("close", (code, signal) => resolve({ code, signal })); + }); + child = undefined; + + assert.deepEqual(result, { code: 0, signal: null }); + const recorded = (await readFile(calls, "utf8")) + .trim() + .split("\n") + .filter(Boolean); + assert.equal(recorded.length, 1); + } finally { + if (child && !child.killed) child.kill("SIGTERM"); + await rm(repo, { + recursive: true, + force: true, + maxRetries: 3, + retryDelay: 50, + }); + } +}); + test("accepts the array form required by the Codex output schema", async () => { const repo = await makeRepo(); const summaries = join(repo, "notes.json");