From 29f3dfbaff101b63eeca62569779e965bae59324 Mon Sep 17 00:00:00 2001 From: Dylan Moore <60218243+dymoo@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:41:24 +0100 Subject: [PATCH] fix: fail closed on incomplete review context --- README.md | 6 ++++ src/context.js | 31 ++++++++++++---- src/index.js | 17 ++++++--- test/context.test.js | 42 +++++++++++++++++++++- test/e2e.test.js | 84 ++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 167 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 5b961d5..f853ab8 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,12 @@ ceiling and upgrade path records a deliberate trade-off. Outputs are `reviewed` (`true` or `false`) and `findings` (the number that survived verification). +`reviewed=true` is fail-closed coverage evidence. The action fails instead of +setting it when a non-ignored file has no textual diff, is binary, exceeds the +file limit, contains a hunk over the per-request token budget, or is dropped +after the total input budget is exhausted. Deliberately ignored paths remain +listed in the sticky summary. + Example project guidance: ```yaml diff --git a/src/context.js b/src/context.js index 8505529..ab04e06 100644 --- a/src/context.js +++ b/src/context.js @@ -59,6 +59,10 @@ export function selectFiles(files, config) { const selected = []; const skipped = []; for (const file of files) { + if (matchAny(file.path, config.ignore)) { + skipped.push({ path: file.path, reason: 'ignored' }); + continue; + } if (file.binary) { skipped.push({ path: file.path, reason: 'binary' }); continue; @@ -67,10 +71,6 @@ export function selectFiles(files, config) { skipped.push({ path: file.path, reason: 'no textual changes' }); continue; } - if (matchAny(file.path, config.ignore)) { - skipped.push({ path: file.path, reason: 'ignored' }); - continue; - } if (selected.length >= config.maxFiles) { skipped.push({ path: file.path, reason: `over max-files (${config.maxFiles})` }); continue; @@ -149,7 +149,7 @@ export function renderRows(rows) { /** * Render one file into one or more text blocks, each within `chunkTokens`. - * @returns {{path: string, text: string, tokens: number}[]} + * @returns {{path: string, text: string, tokens: number, truncated?: boolean}[]} */ export function renderFile(file, content, config) { const newLines = content === null || content === undefined ? null : splitLines(content); @@ -182,7 +182,7 @@ export function renderFile(file, content, config) { Math.floor((config.chunkTokens - estimateTokens(prefix) - estimateTokens(suffix) - 2) * 4), ); const text = `${prefix}${section.slice(0, keep)}\n${suffix}`; - blocks.push({ path: file.path, text, tokens: estimateTokens(text) }); + blocks.push({ path: file.path, text, tokens: estimateTokens(text), truncated: true }); continue; } if (currentTokens + t > budget) flush(); @@ -227,6 +227,25 @@ export function buildChunks(rendered, config) { return { chunks, dropped }; } +/** + * A green review must mean every non-ignored textual change reached the model. + * The action used to report skipped/truncated/dropped material only in its + * summary while still emitting reviewed=true. That makes the status unusable + * as a required safety gate. + */ +export function assertCompleteReviewContext({ skipped = [], rendered = [], dropped = [] }) { + const incomplete = skipped.filter((item) => item.reason !== 'ignored'); + const truncatedPaths = [...new Set(rendered.filter((block) => block.truncated).map((block) => block.path))]; + const failures = [ + ...incomplete.map((item) => `${item.path}: ${item.reason}`), + ...truncatedPaths.map((path) => `${path}: hunk exceeded the per-request token budget`), + ...dropped.map((item) => `${item.path}: ${item.reason}`), + ]; + if (failures.length > 0) { + throw new Error(`Review context is incomplete:\n- ${failures.join('\n- ')}`); + } +} + /** * Keep a verifier's context small but centred on the line under review. * Falls back to the head of the text when the line cannot be located. diff --git a/src/index.js b/src/index.js index 4092ed9..8ea4676 100644 --- a/src/index.js +++ b/src/index.js @@ -2,7 +2,14 @@ import * as core from './core.js'; import { readConfig, readEvent, severityRank } from './config.js'; import { GitHub } from './github.js'; import { parseDiff, anchorFinding, lineText } from './diff.js'; -import { selectFiles, renderFile, buildChunks, sliceAround, estimateTokens } from './context.js'; +import { + selectFiles, + renderFile, + buildChunks, + assertCompleteReviewContext, + sliceAround, + estimateTokens, +} from './context.js'; import { LLM } from './llm.js'; import { findFindings, refuteFinding } from './review.js'; import { mergeFindings, fingerprint, collectFingerprints } from './findings.js'; @@ -31,9 +38,10 @@ async function main() { core.info(`Reviewing ${ctx.owner}/${ctx.repo}#${ctx.prNumber} with ${config.model}`); core.info(`${files.length} changed files, ${selected.length} to review, ${skipped.length} skipped`); + assertCompleteReviewContext({ skipped }); if (!selected.length) { - const summary = renderSummary(emptyResult(pr), config); + const summary = renderSummary(emptyResult(pr, skipped), config); core.appendSummary(summary); await upsertSummary(gh, ctx, summary); setOutputs(true, 0); @@ -74,6 +82,7 @@ async function main() { rendered = selected.flatMap((file, index) => renderFile(file, contents[index], config)); ({ chunks, dropped } = buildChunks(rendered, config)); + assertCompleteReviewContext({ rendered, dropped }); core.info( `Prepared ${chunks.length} request(s), ~${chunks.reduce((total, chunk) => total + chunk.tokens, 0)} tokens${ dropped.length ? `, ${dropped.length} file(s) dropped for budget` : '' @@ -222,13 +231,13 @@ function setOutputs(reviewed, findings) { core.setOutput('findings', String(findings)); } -function emptyResult(pr) { +function emptyResult(pr, skipped) { return { pr, anchored: [], demoted: [], duplicates: 0, - skipped: [], + skipped, dropped: [], summaries: ['No reviewable changes in this pull request.'], refuted: 0, diff --git a/test/context.test.js b/test/context.test.js index 115aa43..b8400fe 100644 --- a/test/context.test.js +++ b/test/context.test.js @@ -7,6 +7,7 @@ import { selectFiles, renderFile, buildChunks, + assertCompleteReviewContext, sliceAround, estimateTokens, } from '../src/context.js'; @@ -38,7 +39,7 @@ test('default ignores catch lockfiles and build output', () => { test('selectFiles reports why each file was skipped', () => { const files = parseDiff([APP_DIFF, BINARY_DIFF].join('\n')); - const { selected, skipped } = selectFiles(files, { ...CONFIG, ignore: ['**/*.png'] }); + const { selected, skipped } = selectFiles(files, { ...CONFIG, ignore: [] }); assert.deepEqual( selected.map((f) => f.path), ['src/app.js'], @@ -47,6 +48,17 @@ test('selectFiles reports why each file was skipped', () => { assert.equal(skipped[0].reason, 'binary'); }); +test('an explicit ignore remains intentional even when the file is binary', () => { + const files = parseDiff(BINARY_DIFF); + const { selected, skipped } = selectFiles(files, { + ...CONFIG, + ignore: ['**/*.png'], + }); + assert.deepEqual(selected, []); + assert.deepEqual(skipped, [{ path: 'logo.png', reason: 'ignored' }]); + assert.doesNotThrow(() => assertCompleteReviewContext({ skipped })); +}); + test('max-files is reported rather than silently applied', () => { const files = parseDiff(APP_DIFF); const capped = selectFiles(files, { ...CONFIG, maxFiles: 0 }); @@ -54,6 +66,25 @@ test('max-files is reported rather than silently applied', () => { assert.match(capped.skipped[0].reason, /max-files/); }); +test('review coverage fails closed for every non-ignored skipped file', () => { + assert.doesNotThrow(() => + assertCompleteReviewContext({ + skipped: [{ path: 'pnpm-lock.yaml', reason: 'ignored' }], + }), + ); + assert.throws( + () => + assertCompleteReviewContext({ + skipped: [ + { path: 'logo.png', reason: 'binary' }, + { path: 'rename-only.js', reason: 'no textual changes' }, + { path: 'over-limit.js', reason: 'over max-files (60)' }, + ], + }), + /logo\.png: binary[\s\S]*rename-only\.js: no textual changes[\s\S]*over-limit\.js: over max-files/, + ); +}); + test('rendering widens each hunk with real source and keeps both line numbers correct', () => { const file = parseDiff(APP_DIFF)[0]; const [block] = renderFile(file, APP_CONTENT, CONFIG); @@ -102,6 +133,11 @@ test('one oversized hunk is truncated within the advertised request budget', () const [block] = renderFile(file, source, { ...CONFIG, contextLines: 0, chunkTokens: 1000 }); assert.ok(block.tokens <= 1000, `block of ${block.tokens} tokens exceeds the request budget`); assert.match(block.text, /hunk truncated/); + assert.equal(block.truncated, true); + assert.throws( + () => assertCompleteReviewContext({ rendered: [block] }), + /huge\.js: hunk exceeded the per-request token budget/, + ); }); test('chunking packs blocks up to the request budget and drops over the total budget', () => { @@ -119,6 +155,10 @@ test('chunking packs blocks up to the request budget and drops over the total bu const budgeted = buildChunks(blocks, { chunkTokens: 2500, maxInputTokens: 2000 }); assert.equal(budgeted.dropped.length, 4); assert.match(budgeted.dropped[0].reason, /max-input-tokens/); + assert.throws( + () => assertCompleteReviewContext({ dropped: budgeted.dropped }), + /f2\.js: over max-input-tokens \(2000\)/, + ); }); test('sliceAround centres on the requested line', () => { diff --git a/test/e2e.test.js b/test/e2e.test.js index e007fa3..cb8a248 100644 --- a/test/e2e.test.js +++ b/test/e2e.test.js @@ -12,7 +12,7 @@ import { fileURLToPath } from 'node:url'; import { GitHub } from '../src/github.js'; import { openRepo } from '../src/repo.js'; import { fingerprint } from '../src/findings.js'; -import { APP_DIFF, APP_CONTENT } from './fixtures.js'; +import { APP_DIFF, BINARY_DIFF, APP_CONTENT } from './fixtures.js'; const ENTRY = fileURLToPath(new URL('../src/index.js', import.meta.url)); @@ -62,6 +62,17 @@ function makeTarball(files, symlinks = {}) { return data; } +function addedFileDiff(filePath, content) { + return [ + `diff --git a/${filePath} b/${filePath}`, + 'new file mode 100644', + '--- /dev/null', + `+++ b/${filePath}`, + '@@ -0,0 +1,1 @@', + `+${content}`, + ].join('\n'); +} + const REPO_FILES = { 'src/app.js': APP_CONTENT, 'src/db.js': 'export const db = {\n get(id) { return rows[id]; }, // undefined when missing\n};\n', @@ -97,6 +108,7 @@ function standardReply({ findings = FINDINGS, verdict = 'real' } = {}) { * rejectTools?: boolean, * repoFiles?: Record, * baseRepoFiles?: Record, + * diffText?: string, * symlinks?: Record, * issueComments?: any[], * reviewComments?: any[] @@ -107,6 +119,7 @@ async function stubServer({ rejectTools = false, repoFiles = REPO_FILES, baseRepoFiles = BASE_REPO_FILES, + diffText = APP_DIFF, symlinks = {}, issueComments = [], reviewComments = [], @@ -153,7 +166,7 @@ async function stubServer({ }); } if (url.pathname === '/repos/o/r/pulls/1' && req.method === 'GET') { - if ((req.headers.accept || '').includes('diff')) return send(200, APP_DIFF, 'text/plain'); + if ((req.headers.accept || '').includes('diff')) return send(200, diffText, 'text/plain'); return send(200, { number: 1, state: 'open', @@ -391,6 +404,73 @@ test('a pull request symlink cannot read outside the extracted repository', asyn assert.match(await repo.read('src/db.js'), /undefined when missing/); }); +test('a binary pull request change fails instead of claiming complete review coverage', async (t) => { + const { server, captured, port } = await stubServer({ + diffText: BINARY_DIFF, + repoFiles: { 'README.md': 'binary-only review fixture\n' }, + }); + t.after(() => server.close()); + + const run = await runAction(port); + assert.notEqual(run.code, 0); + assert.match(`${run.stdout}\n${run.stderr}`, /Review context is incomplete:[\s\S]*logo\.png: binary/); + assert.equal(captured.llmRequests.length, 0); + assert.deepEqual(run.outputs, {}); +}); + +test('an oversized hunk fails before any model request or reviewed output', async (t) => { + const source = 'x'.repeat(121_000); + const { server, captured, port } = await stubServer({ + diffText: addedFileDiff('oversized.js', source), + repoFiles: { 'oversized.js': source }, + }); + t.after(() => server.close()); + + const run = await runAction(port); + assert.notEqual(run.code, 0); + assert.match(`${run.stdout}\n${run.stderr}`, /oversized\.js: hunk exceeded the per-request token budget/); + assert.equal(captured.llmRequests.length, 0); + assert.deepEqual(run.outputs, {}); +}); + +test('total token exhaustion fails before any model request or reviewed output', async (t) => { + /** @type {Record} */ + const repoFiles = {}; + const diffs = []; + for (let index = 0; index < 5; index++) { + const filePath = `large-${index}.js`; + const source = String(index).repeat(100_000); + repoFiles[filePath] = source; + diffs.push(addedFileDiff(filePath, source)); + } + const { server, captured, port } = await stubServer({ + diffText: diffs.join('\n'), + repoFiles, + }); + t.after(() => server.close()); + + const run = await runAction(port); + assert.notEqual(run.code, 0); + assert.match(`${run.stdout}\n${run.stderr}`, /large-4\.js: over max-input-tokens \(120000\)/); + assert.equal(captured.llmRequests.length, 0); + assert.deepEqual(run.outputs, {}); +}); + +test('an ignored-only change remains visible in the successful sticky summary', async (t) => { + const { server, captured, port } = await stubServer({ + diffText: addedFileDiff('pnpm-lock.yaml', 'lockfileVersion: 9'), + repoFiles: { 'pnpm-lock.yaml': 'lockfileVersion: 9' }, + }); + t.after(() => server.close()); + + const run = await runAction(port); + assert.equal(run.code, 0, `action failed:\n${run.stdout}\n${run.stderr}`); + assert.equal(captured.llmRequests.length, 0); + assert.equal(run.outputs.reviewed, 'true'); + assert.match(captured.createdComments[0].body, /Not reviewed \(1\)/); + assert.match(captured.createdComments[0].body, /`pnpm-lock\.yaml` — ignored/); +}); + test('an unrelated event is skipped without spending a model request', async (t) => { const { server, captured, port } = await stubServer(); t.after(() => server.close());