Fail closed on incomplete review context

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

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.

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,

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,13 +48,43 @@ 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 });
   assert.equal(capped.selected.length, 0);
   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', () => {

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<string, string>,
  *   baseRepoFiles?: Record<string, string>,
+ *   diffText?: string,
  *   symlinks?: Record<string, string>,
  *   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<string, string>} */
+  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());