diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 9c09f9808..6ef684acc 100644 --- a/docs/ship-policy.md +++ b/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 diff --git a/lib/review-exclude.ts b/lib/review-exclude.ts index a92e0f136..282594953 100644 --- a/lib/review-exclude.ts +++ b/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, claimed: ReadonlySet): Set { +function binaryContent(i: ExcludeInput, files: ReadonlyMap, claimed: ReadonlySet, unread: Set): Set { const out = new Set(); const real = (sha: string) => /^[0-9a-f]{40,64}$/.test(sha) && !/^0+$/.test(sha); const bySha = new Map(); @@ -134,7 +134,7 @@ function binaryContent(i: ExcludeInput, files: ReadonlyMap, 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(); + 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(); - 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) { diff --git a/test/defensive-review-framing.test.ts b/test/defensive-review-framing.test.ts index 464c87e09..8336dcda2 100644 --- a/test/defensive-review-framing.test.ts +++ b/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); diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 7a5cf7c41..10b8e2757 100644 --- a/test/review-dispatch.test.ts +++ b/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'); diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index a328090a5..e406ca1c3 100644 --- a/test/review-exclude.test.ts +++ b/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); });