fix: keep a live binary file whose bytes cannot be read; tests render outside voices from a clean checkout and read the ship fixture text

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

docs/ship-policy.md

@@ -1145,7 +1145,7 @@ Everything else stays in every reader's diff with one reason code:
 | `code` | it is code or a test, not content; a binary file with a source or script extension (`.js`, `.py`, `.sh`, `.wasm`) counts as code |
 | `secret-line` | an added line reads a credential (the targeted pass's secret rule) |
 | `named-by-code` | a code or test file that stays in review (not docs, a data file, a `.log` or binary, and not itself excluded) names its full repository path, at the head or the merge base |
-| `consumer-scan-unavailable` | the `git grep` for those names failed, so every candidate is kept |
+| `consumer-scan-unavailable` | the `git grep` for those names failed, so every candidate is kept; also a live binary file whose bytes could not be read |
 | `gate-scan-unavailable` | the gate scan failed or did not run at the head, so no hunk's tag is known and every candidate is kept |
 
 Only a full path counts as a name. A basename such as `result.json` would keep

lib/review-exclude.ts

@@ -126,7 +126,7 @@ function select(i: ExcludeInput, patterns: readonly string[]): { byPattern: Map<
  * the file on a live tree, or the old blob for a deletion. A branch's .gitattributes can make git call a text file binary;
  * only real bytes make it binary content here.
  */
-function binaryContent(i: ExcludeInput, files: ReadonlyMap<string, Matched>, claimed: ReadonlySet<string>): Set<string> {
+function binaryContent(i: ExcludeInput, files: ReadonlyMap<string, Matched>, claimed: ReadonlySet<string>, unread: Set<string>): Set<string> {
   const out = new Set<string>();
   const real = (sha: string) => /^[0-9a-f]{40,64}$/.test(sha) && !/^0+$/.test(sha);
   const bySha = new Map<string, string[]>();
@@ -134,7 +134,7 @@ function binaryContent(i: ExcludeInput, files: ReadonlyMap<string, Matched>, cla
     const m = files.get(p);
     if (!m) continue;
     if (!i.head && m.newMode !== '000000') {
-      try { const fd = fs.openSync(path.join(i.root, p), 'r'); const buf = Buffer.alloc(8000); const n = fs.readSync(fd, buf, 0, 8000, 0); fs.closeSync(fd); if (buf.subarray(0, n).includes(0)) out.add(p); } catch {}
+      try { const fd = fs.openSync(path.join(i.root, p), 'r'); const buf = Buffer.alloc(8000); const n = fs.readSync(fd, buf, 0, 8000, 0); fs.closeSync(fd); if (buf.subarray(0, n).includes(0)) out.add(p); } catch { unread.add(p); }
       continue;
     }
     const sha = real(m.newSha) ? m.newSha : real(m.oldSha) ? m.oldSha : '';
@@ -232,9 +232,10 @@ export function reviewExclude(i: ExcludeInput): Exclusion {
   if (!patterns.length) { const head = headPatterns(i); return head.length ? { state: 'head-only', patterns: head, source } : { state: 'none' }; }
   const sel = select(i, patterns);
   if ('error' in sel) return { state: 'refused', code: 'REVIEW_DIFF_UNREADABLE', lines: [`${i.context}: git could not list the files review_exclude_paths matches (${sel.error}); fix: git fetch origin ${base}, then rerun once; nothing was planned (REVIEW_DIFF_UNREADABLE)`] };
-  const binary = binaryContent(i, sel.files, new Set(i.perPath.filter(e => e.binary && sel.files.has(e.path)).map(e => e.path)));
+  const unread = new Set<string>();
+  const binary = binaryContent(i, sel.files, new Set(i.perPath.filter(e => e.binary && sel.files.has(e.path)).map(e => e.path)), unread);
   const reasons = new Map<string, KeptReason | null>();
-  for (const [p, m] of sel.files) reasons.set(p, pathReason(i, p, m, binary));
+  for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(i, p, m, binary));
   const candidates = new Set([...reasons].filter(([, r]) => r === null).map(([p]) => p));
   const named = grepFixed(i, [...candidates].filter(p => !p.includes('\n')), candidates, [i.mergeBase]);
   for (const p of candidates) {

test/defensive-review-framing.test.ts

@@ -1,7 +1,7 @@
 import { describe, expect, test } from 'bun:test';
 import * as fs from 'fs';
 import * as path from 'path';
-import { DEFENSIVE_REVIEW_FRAMING, FIXTURE_SUMMARY_MODE } from '../scripts/resolvers/defensive-review';
+import { DEFENSIVE_REVIEW_FRAMING, FIXTURE_SUMMARY_MODE, fixtureSummaryMode } from '../scripts/resolvers/defensive-review';
 import { generateReviewArmy } from '../scripts/resolvers/review-army';
 import { HOST_PATHS } from '../scripts/resolvers/types';
 
@@ -27,7 +27,7 @@ describe('defensive-security framing reaches every adversarial dispatch', () =>
     test(`${skill}: adversarial subagent keeps the framing and fixture summary mode`, () => {
       const prompt = between(adversarial, 'Subagent prompt:', 'Think like an attacker');
       expect(prompt).toContain(DEFENSIVE_REVIEW_FRAMING);
-      expect(prompt).toContain(FIXTURE_SUMMARY_MODE);
+      expect(prompt).toContain(fixtureSummaryMode(skill === 'ship'));
     });
 
     test(`${skill}: Red Team prompt starts from the framing and fixture summary mode`, () => {
@@ -41,7 +41,7 @@ describe('defensive-security framing reaches every adversarial dispatch', () =>
       const dispatch = between(army, '### Dispatch specialists in parallel', '**Subagent configuration:**');
       const securityOnly = between(dispatch, '4. **Defensive framing (Security specialist and the Red Team below only).**', 'Instructions:');
       expect(securityOnly).toContain(DEFENSIVE_REVIEW_FRAMING);
-      expect(securityOnly).toContain(FIXTURE_SUMMARY_MODE);
+      expect(securityOnly).toContain(fixtureSummaryMode(skill === 'ship'));
       expect(securityOnly).toContain('including Testing, reads the full diff with fixtures');
       const sharedInstructions = dispatch.slice(dispatch.indexOf('Instructions:'));
       expect(sharedInstructions).not.toContain(FRAMING_SENTENCE);

test/review-dispatch.test.ts

@@ -1,4 +1,4 @@
-import { afterEach, describe, expect, test } from 'bun:test';
+import { afterAll, afterEach, describe, expect, test } from 'bun:test';
 import { spawnSync } from 'node:child_process';
 import * as fs from 'node:fs';
 import * as os from 'node:os';
@@ -11,6 +11,16 @@ const BIN = path.join(ROOT, 'bin', 'gstack-review-dispatch');
 const dirs: string[] = [];
 afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); });
 const tmp = () => { const d = fs.mkdtempSync(path.join(os.tmpdir(), 'review-dispatch-')); dirs.push(d); return d; };
+/** A clean checkout of HEAD: the outside voice embeds the diff against its base, and a build that rewrites tracked files in ROOT can push that past the reviewer diff ceiling. */
+let clean = '';
+const cleanCheckout = () => {
+  if (clean) return clean;
+  const dir = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'review-dispatch-clean-')), 'wt');
+  const r = spawnSync('git', ['-C', ROOT, 'worktree', 'add', '--detach', dir, 'HEAD'], { encoding: 'utf8', timeout: 120_000 });
+  if (r.status !== 0) throw new Error(`git worktree add failed: ${r.stderr}`);
+  return (clean = dir);
+};
+afterAll(() => { if (clean) spawnSync('git', ['-C', ROOT, 'worktree', 'remove', '--force', clean], { encoding: 'utf8', timeout: 60_000 }); });
 /** A reader that follows its brief's workspace paragraph starts its reply with the REVIEWED_TREE line of that checkout (GST-482). */
 function treeLineOf(cwd: string): string {
   const git = (a: string[]) => spawnSync('git', ['-C', cwd, ...a], { encoding: 'utf8', timeout: 30_000 }).stdout.trim();
@@ -165,10 +175,11 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
     const spec = p.w('spec.out', '{"severity":"CRITICAL","path":"a.ts","line":3,"category":"logic","summary":"off by one","specialist":"testing"}\n{"severity":"INFORMATIONAL","path":"a.ts","line":9,"category":"style","summary":"name","specialist":"testing"}\n');
     const outside = p.w('outside.out', 'Findings:\n1. **High**: retry loop never backs off (q.ts:78)\n2. Low - log line is noisy\nRecommendation: Fix the retry because it DoSes the pool\n');
     const c1 = run(['collect', '--ledger', p.ledger, '--role', 'specialist', '--name', 'testing', '--output', spec, '--merged', p.merged]);
-    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 10_000 }).stdout.trim();
-    const ov = field(run(['render', '--role', 'outside-voice', '--out', p.out, '--diff-base', head]).stdout, 'file');
+    const wt = cleanCheckout();
+    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: wt, encoding: 'utf8', timeout: 10_000 }).stdout.trim();
+    const ov = field(run(['render', '--role', 'outside-voice', '--out', p.out, '--diff-base', head], wt).stdout, 'file');
     run(['sent', '--ledger', p.ledger, '--role', 'outside-voice', '--sent', ov]);
-    const c2 = run(['collect', '--ledger', p.ledger, '--role', 'outside-voice', '--output', outside, '--merged', p.merged]);
+    const c2 = run(['collect', '--ledger', p.ledger, '--role', 'outside-voice', '--output', outside, '--merged', p.merged], wt);
     expect([c1.stdout.includes('findings=2'), c2.stdout.includes('findings=2')]).toEqual([true, true]);
     const merged = fs.readFileSync(p.merged, 'utf8').trim().split('\n').map(l => JSON.parse(l) as MergedLine);
     expect(merged.map(m => [m.id, m.label])).toEqual([['specialist/testing#1', 'CRITICAL'], ['specialist/testing#2', 'INFORMATIONAL'], ['outside-voice#1', 'High'], ['outside-voice#2', 'Low']]);
@@ -455,9 +466,10 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
     run(['judge', '--ledger', p.ledger, '--merged', p.merged, '--judged', path.join(p.out, 'judged.jsonl')]);
     const v = run(['verify', '--ledger', p.ledger]);
     expect([v.status, v.stdout.includes('replaced_by_a_review'), v.stdout.includes('VERIFY: clean briefs=1 outputs=2 voided=1')]).toEqual([0, true, true]);
-    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 10_000 }).stdout.trim();
+    const wt = cleanCheckout();
+    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: wt, encoding: 'utf8', timeout: 10_000 }).stdout.trim();
     const o = tmp();
-    const ov = field(run(['render', '--role', 'outside-voice', '--out', o, '--diff-base', head]).stdout, 'file');
+    const ov = field(run(['render', '--role', 'outside-voice', '--out', o, '--diff-base', head], wt).stdout, 'file');
     const ledger = path.join(o, 'review-ledger.jsonl'), merged = path.join(o, 'merged.jsonl');
     run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', ov]);
     const fail = path.join(o, 'timeout.out');
@@ -560,8 +572,9 @@ describe('reading the REVIEWED_TREE line (GST-482 review fixes)', () => {
     const out = tmp();
     const ledger = path.join(out, 'review-ledger.jsonl');
     const merged = path.join(out, 'merged.jsonl');
-    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim();
-    const file = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head]).stdout, 'file');
+    const wt = cleanCheckout();
+    const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: wt, encoding: 'utf8', timeout: 30_000 }).stdout.trim();
+    const file = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head], wt).stdout, 'file');
     run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', file]);
     const reply = path.join(out, 'r.out');
     fs.writeFileSync(reply, '1. **CRITICAL**: auth bypass in a.ts:3\nBlocks: yes\n\nRecommendation: request changes\n');

test/review-exclude.test.ts

@@ -9,7 +9,7 @@ import * as fs from 'node:fs';
 import * as path from 'node:path';
 import { EXIT } from '../lib/headless-artifacts';
 import { KEEP_ARG_MAX_BYTES, readSelection, reviewExclude, tooLargeLine, unreadableLine } from '../lib/review-exclude';
-import { collectCommit } from '../lib/review-collect';
+import { collectCommit, collectLive } from '../lib/review-collect';
 import { extractReceipt } from '../lib/ship-receipt';
 import { bindShippedReceipt } from '../lib/ship-receipt-depth';
 import { REVIEW_EXCLUDE_KEY_VERSION, isUnavailable, loadPolicyAt } from '../lib/ship-policy';
@@ -324,5 +324,22 @@ describe('a branch cannot hide code under a base pattern', () => {
     expect(blind.excluded.size).toBe(0);
     expect(blind.kept.filter(k => k.reason === 'gate-scan-unavailable').map(k => k.path).sort()).toEqual([...EXCLUDED, 'receipts/b/result.json', 'receipts/d/fixture.json'].sort());
   }, 120_000);
+
+  test('a live binary file whose bytes cannot be read is kept, not excluded', () => {
+    const repo = census();
+    const gz = path.join(repo.work, 'receipts/a/shot.png');
+    fs.writeFileSync(gz, Buffer.from([0x89, 0x50, 0, 1]));
+    commit(repo, {});
+    fs.writeFileSync(gz, Buffer.from([0x89, 0x50, 0, 2]));
+    fs.chmodSync(gz, 0o000);
+    try {
+      const c = collectLive(repo.work, 'origin/main');
+      const x = reviewExclude({ root: repo.work, baseSha: repo.git('rev-parse', 'origin/main'), ref: 'origin/main', mergeBase: c.mergeBase!, perPath: [...c.perPath, { path: 'receipts/a/shot.png', lines: 0, binary: true }], added: c.added, classRoot: repo.work, gateTagged: new Set(), context: 'test' });
+      expect(x.state).toBe('active');
+      if (x.state !== 'active') return;
+      expect(x.excluded.has('receipts/a/shot.png')).toBe(false);
+      expect(x.kept).toContainEqual({ path: 'receipts/a/shot.png', reason: 'consumer-scan-unavailable' });
+    } finally { fs.chmodSync(gz, 0o644); }
+  }, 120_000);
 });