From 225a39b312356d5cacc24a7b473d3888c99ef4f9 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 01:53:33 +0000 Subject: [PATCH 01/10] fix(review): review_exclude_paths keeps native binaries, NUL-bearing code and attribute-binary secret lines; exact --review-exclude docs (GST-972) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 4 +- .../gst-972-review-exclude-follow-ups.md | 7 ++ docs/ship-policy.md | 14 ++-- docs/troubleshooting.md | 6 +- lib/review-exclude.ts | 53 ++++++++++--- lib/ship-receipt-depth.ts | 5 +- test/review-exclude.test.ts | 79 ++++++++++++++++++- 7 files changed, 145 insertions(+), 23 deletions(-) create mode 100644 changelog.d/gst-972-review-exclude-follow-ups.md diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 9e417faa9..866e77ec8 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -98,7 +98,9 @@ write flags: --pr --head --base --version --gated-tree --predecessor --gate (the digest= on gstack-review-dispatch verify's REVIEW_EXCLUDE_RECORD: line, which the reviewers' briefs name; none without that line): must equal the digest HEAD has under the base policy's review_exclude_paths, else - RECEIPT_INVALID field=review_exclude and nothing is written (rerun the review; write alone cannot clear it); recorded as review_exclude. Carry and the queue compare that digest. + RECEIPT_INVALID field=review_exclude and nothing is written; recorded as review_exclude. Carry and the queue compare that digest. + The check binds the digest to the base policy, not to a review: write does not read the review ledger, so the expected= + digest a refusal prints passes too. After a refusal, rerun the review and pass the digest its verify prints. --review-targeted ({classes, rules, brief_sha, status, findings, raised_by, head}: gstack-review-dispatch verify --targeted-out); recorded as review_targeted bound to the receipt's product_patch_id, and refused (RECEIPT_INVALID) when the commit the reader read (head) is not an ancestor or a change since it carries a targeted class (gate scan included). write and carry diff --git a/changelog.d/gst-972-review-exclude-follow-ups.md b/changelog.d/gst-972-review-exclude-follow-ups.md new file mode 100644 index 000000000..279d57153 --- /dev/null +++ b/changelog.d/gst-972-review-exclude-follow-ups.md @@ -0,0 +1,7 @@ +**`review_exclude_paths` keeps code it used to exclude, and the docs say exactly what the receipt check stops (GST-972).** Three ways to get code or a credential out of review under a base pattern are closed, each with a failing-first test in `test/review-exclude.test.ts`, and the receipt docs no longer over-claim. + +### Fixed +- **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A binary file now starting with ELF, Mach-O, PE, WebAssembly or `#!` magic is kept as `code` whatever its extension. +- **A NUL byte no longer makes code excludable.** A binary file was content unless its extension was on a short source list, so `.gradle`, `.sql`, `.tf`, `.groovy`, a `.p12` key store or an extensionless private key with a NUL byte was excluded. A binary file is now content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat` and data blobs); every other binary file is kept as `code`. +- **A branch's `.gitattributes` cannot hide a credential line.** Marking `receipts/**` binary left the collector's added lines empty, so the secret-line keep rule saw nothing and the file was excluded. For a file git calls binary only because of its attributes, the rule now reads `git diff --text --no-textconv --no-ext-diff`. +- **`gstack-ship-receipt write --review-exclude` docs are exact.** The help, `docs/troubleshooting.md`, `docs/ship-policy.md` and the queue's refusal line said a receipt write alone cannot clear `field=review_exclude`. Write checks the digest against the base policy and does not read the review ledger, so a write given the refusal's `expected=` digest passes without a review. The text now says that, and that the review rerun is the fix. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index d76edb603..551153b24 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1287,8 +1287,8 @@ A pattern that matches no changed file is listed as `unmatched=` on the plan lin **What is excluded, and what is kept.** A matched file is excluded only when it is content: a data file (JSON, JSONL, CSV, TSV, images), a docs-class file, a -`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content even under -`config/` or in a skill directory; only `policy-dir`, `gate-path`, `executable`, `link`, `gate-tagged`, or a source or script extension (`code`) keep it. +`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Under those rules it is content even under +`config/` or in a skill directory, and only `policy-dir`, `gate-path`, `executable`, `link` or `gate-tagged` keep it. Every other binary file is `code`: source with a NUL, a native library (`.node`, `.so`, `.dylib`, `.dll`), a compiled executable, a key store, a pickle, or a file with no extension. Everything else stays in every reader's diff with one reason code: | Reason | Kept because | @@ -1300,8 +1300,8 @@ Everything else stays in every reader's diff with one reason code: | `agent-prose` | it is a skill, section, resolver, template or rule file | | `config` | it is a data-shaped config file (`package.json`, `tsconfig*.json`, `config/`, dotfiles) | | `gate-tagged` | the gate scan tagged one of its hunks | -| `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) | +| `code` | it is code or a test, not content; a binary file outside the content extensions above, or one with executable magic, counts as code | +| `secret-line` | an added line reads a credential (the targeted pass's secret rule); for a file git calls binary only because of its attributes, the added lines come from `git diff --text`, which `.gitattributes` cannot change | | `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; 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 | @@ -1355,7 +1355,11 @@ and binds. A different digest means the reviewers read another selection: write refuses with `RECEIPT_INVALID field=review_exclude`, carry with `RECEIPT_CARRY_REFUSED review_exclude changed`, and the queue with `RECEIPT_INVALID field=review_exclude expected= recorded=`. The -fix is to rerun the review, never a receipt write alone. +fix is to rerun the review and write with the digest its `verify` prints. Write +checks the digest against the base policy, so a stale selection cannot be +recorded by accident, but it does not read the review ledger: a write given the +`expected=` digest passes without a review, and only the review run shows that +the reviewers read that selection. **Adopting it.** (1) Open a PR on the base that adds the patterns and `min_gstack_version` at least `REVIEW_EXCLUDE_KEY_VERSION`, and ship it like any diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 42c2a96ce..8f1d5aa9f 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3340,8 +3340,10 @@ use this code when the receipt's review_exclude digest (or the base policy's [`review_exclude_paths`](ship-policy.md#review-exclude-paths). The line names `expected=` (now), `recorded=` (what the review read, `none` when it read without exclusion) and `source=` (the base commit). The reviewers read another -selection than the one the head now has, so a receipt write alone cannot clear -it: rerun `/ship`'s review on the head (`gstack-review-dispatch pass --base `), +selection than the one the head now has. Write checks the digest against the base +policy only: it does not read the review ledger, so a write that passes the +`expected=` digest clears the field without a review. Do not do that. Rerun +`/ship`'s review on the head (`gstack-review-dispatch pass --base `), run `gstack-review-dispatch verify --ledger `, which prints `REVIEW_EXCLUDE_RECORD: digest=` from the briefs the readers got, then `gstack-ship-receipt write --pr --review-exclude --body `. diff --git a/lib/review-exclude.ts b/lib/review-exclude.ts index 80533bc7d..7f4c48d21 100644 --- a/lib/review-exclude.ts +++ b/lib/review-exclude.ts @@ -122,20 +122,25 @@ function select(i: ExcludeInput, patterns: readonly string[]): { byPattern: Map< return { byPattern, files }; } +/** Executable image magic: ELF, Mach-O (thin and universal; 0xcafebabe is also a Java class), PE, WebAssembly and a script's #!. */ +const NATIVE_MAGIC = [[0x7f, 0x45, 0x4c, 0x46], [0xfe, 0xed, 0xfa, 0xce], [0xfe, 0xed, 0xfa, 0xcf], [0xce, 0xfa, 0xed, 0xfe], [0xcf, 0xfa, 0xed, 0xfe], [0xca, 0xfe, 0xba, 0xbe], [0x4d, 0x5a], [0x00, 0x61, 0x73, 0x6d], [0x23, 0x21]]; +const nativeImage = (b: Buffer) => NATIVE_MAGIC.some(m => m.every((x, k) => b[k] === x)); + /** * The matched paths git calls binary whose bytes hold a NUL in their first 8,000 (git's own test), read from the head blob, * 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. + * only real bytes make it binary content here. A binary file that starts with executable image magic also goes in `native`. */ -function binaryContent(i: ExcludeInput, files: ReadonlyMap, claimed: ReadonlySet, unread: Set): Set { +function binaryContent(i: ExcludeInput, files: ReadonlyMap, claimed: ReadonlySet, unread: Set, native: Set): Set { const out = new Set(); + const sniff = (p: string, b: Buffer) => { if (!b.includes(0)) return; out.add(p); if (nativeImage(b)) native.add(p); }; const real = (sha: string) => /^[0-9a-f]{40,64}$/.test(sha) && !/^0+$/.test(sha); const bySha = new Map(); for (const p of claimed) { 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 { unread.add(p); } + 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); sniff(p, buf.subarray(0, n)); } catch { unread.add(p); } continue; } const sha = real(m.newSha) ? m.newSha : real(m.oldSha) ? m.oldSha : ''; @@ -144,8 +149,25 @@ function binaryContent(i: ExcludeInput, files: ReadonlyMap, cla for (const [sha, paths] of bySha) { const r = spawnSync('git', ['cat-file', 'blob', sha], { cwd: i.root, timeout: 30_000, maxBuffer: 8000 }); const read = r.status === 0 || (r.error as NodeJS.ErrnoException | undefined)?.code === 'ENOBUFS'; - const binary = read && (r.stdout ?? Buffer.alloc(0)).subarray(0, 8000).includes(0); - for (const p of paths) if (!read) unread.add(p); else if (binary) out.add(p); + const head = (r.stdout ?? Buffer.alloc(0)).subarray(0, 8000); + for (const p of paths) if (!read) unread.add(p); else sniff(p, head); + } + return out; +} + +/** + * The added lines of matched files git calls binary only because of their attributes (the bytes hold no NUL): the + * collector's added lines are empty for them, so the secret-line rule reads `git diff --text` instead, which no + * .gitattributes, textconv or external diff driver changes. A git failure puts the path in `unread`. + */ +function attributeBinaryLines(i: ExcludeInput, paths: readonly string[], unread: Set): Record { + const out: Record = {}; + const range = i.head ? [i.mergeBase, i.head] : [i.mergeBase]; + for (const p of paths) { + const r = git(i.root, ['diff', '--text', '--no-textconv', '--no-ext-diff', '--no-renames', '-U0', ...range, '--', literal(p)]); + if (!r.ok) { unread.add(p); continue; } + let hunk = false; + out[p] = r.out.split('\n').filter(l => (hunk ||= l.startsWith('@@')) && l.startsWith('+')).map(l => l.slice(1)); } return out; } @@ -159,17 +181,21 @@ function contentKind(i: ExcludeInput, p: string, binary: ReadonlySet): C return classifyPath(p, i.classRoot) === 'docs' ? 'docs' : null; } -/** Source and script extensions: a NUL in one does not make it excludable binary content. */ -const CODE_EXT = /\.(?:[cm]?[jt]sx?|py|rb|go|rs|java|kts?|scala|swift|exs?|php|cs|c|h|cc|cpp|hpp|mm?|dart|pl|pm|lua|sh|bash|zsh|fish|ps1|bat|cmd|vue|svelte|html?|wasm)$/i; +/** + * The only extensions a binary file may be excludable content under: images, fonts, media, documents, compressed archives + * and data blobs. Any other binary (source with a NUL, a native library or executable, a key store, a pickle, no + * extension) is code; so is one that starts with executable image magic whatever its extension. + */ +const BINARY_CONTENT_EXT = /\.(?:png|jpe?g|gif|webp|bmp|ico|avif|tiff?|heic|heif|woff2?|ttf|otf|eot|mp3|mp4|m4a|wav|ogg|oga|ogv|webm|mov|avi|mkv|flac|aac|opus|pdf|docx|xlsx|pptx|odt|ods|odp|gz|tgz|zip|tar|bz2|xz|zst|7z|bin|dat|parquet|arrow|feather|avro|orc|npy|npz|h5|hdf5|sqlite3?|db|safetensors)$/i; -/** Why a matched file is kept by its path, mode and lines alone; null for content the policy may exclude. A binary file readers would only see as "Binary files differ" is content unless its directory, mode or gate tags keep it. */ -function pathReason(i: ExcludeInput, p: string, m: Matched, binary: ReadonlySet): KeptReason | null { +/** Why a matched file is kept by its path, mode and lines alone; null for content the policy may exclude. A binary file readers would only see as "Binary files differ" is content unless its directory, mode, gate tags, extension or magic keep it. */ +function pathReason(i: ExcludeInput, p: string, m: Matched, binary: ReadonlySet, native: ReadonlySet): KeptReason | null { if (/^\.(gstack|github)\//.test(p)) return 'policy-dir'; if (isGatePath(p)) return 'gate-path'; if (m.oldMode === '100755' || m.newMode === '100755') return 'executable'; if ([m.oldMode, m.newMode].some(x => x === '120000' || x === '160000')) return 'link'; if (i.gateTagged?.has(p)) return 'gate-tagged'; - if (binary.has(p)) return CODE_EXT.test(p) ? 'code' : null; + if (binary.has(p)) return BINARY_CONTENT_EXT.test(p) && !native.has(p) ? null : 'code'; if (isAgentProse(p)) return 'agent-prose'; if (isDataConfig(p)) return 'config'; if (contentKind(i, p, binary) === null) return 'code'; @@ -227,9 +253,12 @@ export function reviewExclude(i: ExcludeInput): Exclusion { 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 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 native = new Set(); + const claimed = i.perPath.filter(e => e.binary && sel.files.has(e.path)).map(e => e.path); + const binary = binaryContent(i, sel.files, new Set(claimed), unread, native); + const text = { ...i, added: { ...i.added, ...attributeBinaryLines(i, claimed.filter(p => !binary.has(p) && !unread.has(p)), unread) } }; const reasons = new Map(); - for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(i, p, m, binary)); + for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(text, p, m, binary, native)); 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/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 7cd524523..b48ebdd90 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -224,7 +224,8 @@ function headInputs(root: string, baseRef: string, head: string, r?: ShipReceipt /** * The queue's exclusion check: the digest the receipt recorded (none when absent) against the one `head` has under the current * base policy. Only the digest counts (the policy's patterns and the excluded list), so a base merge or a moved base that leaves - * both unchanged binds; a changed selection needs the review rerun, which receipt write alone cannot stand in for. + * both unchanged binds; a changed selection needs the review rerun (write checks the same digest and does not read the + * review ledger). */ export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base: string): string | null { const expected = x.state === 'active' ? x.record.digest : 'none'; @@ -232,7 +233,7 @@ export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base if (expected === recorded) return null; const source = x.state === 'active' ? x.record.source : r.review_exclude?.source ?? base; const h = head.slice(0, 12); - return `field=review_exclude expected=${expected} recorded=${recorded} source=${source}: the reviewers read another selection than ${base}'s review_exclude_paths gives ${h}; the receipt is unchanged; fix: rerun /ship's review on ${h} (gstack-review-dispatch pass --base ${base.replace(/^(refs\/remotes\/)?origin\//, '')}), then gstack-ship-receipt write --pr --review-exclude --body with the digest gstack-review-dispatch verify --ledger prints on REVIEW_EXCLUDE_RECORD:; a receipt write alone does not clear this`; + return `field=review_exclude expected=${expected} recorded=${recorded} source=${source}: the reviewers read another selection than ${base}'s review_exclude_paths gives ${h}; the receipt is unchanged; fix: rerun /ship's review on ${h} (gstack-review-dispatch pass --base ${base.replace(/^(refs\/remotes\/)?origin\//, '')}), then gstack-ship-receipt write --pr --review-exclude --body with the digest gstack-review-dispatch verify --ledger prints on REVIEW_EXCLUDE_RECORD: (write checks the digest against the base policy and does not read the review ledger, so only the rerun shows the reviewers read this selection)`; } /** The receipt's recorded gate scan when it names this head; otherwise null (unavailable). */ const boundScan = (r: ShipReceipt, head: string): number | null => { diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index aefa18c08..de38365b1 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -11,7 +11,7 @@ import { EXIT } from '../lib/headless-artifacts'; import { KEEP_ARG_MAX_BYTES, readSelection, reviewExclude, tooLargeLine, unreadableLine } from '../lib/review-exclude'; import { collectCommit, collectLive } from '../lib/review-collect'; import { extractReceipt } from '../lib/ship-receipt'; -import { bindShippedReceipt, receiptReviewAtHead } from '../lib/ship-receipt-depth'; +import { bindShippedReceipt, excludeMismatch, receiptReviewAtHead } from '../lib/ship-receipt-depth'; import { REVIEW_EXCLUDE_KEY_VERSION, isUnavailable } from '../lib/ship-policy'; import { loadPolicyAt } from '../lib/ship-policy-at'; import { cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; @@ -313,6 +313,31 @@ describe('the receipt binds the selection the reviewers read', () => { expect(removed.status).toBe(EXIT.fail); expect(removed.err).toContain(`field=review_exclude expected=none recorded=${digest}`); }, 240_000); + + // Value: protects=every place that describes write --review-exclude says what it checks: the digest HEAD has under the base + // policy, not that a review read that selection; fails_when=the help, troubleshooting, ship-policy or the queue's refusal + // line claims a receipt write alone cannot clear field=review_exclude while a write with the refusal's own expected= digest + // clears it with no review run; why_new=GST-972 item 2 (the #3852 cross-family trail review); seam=gstack-ship-receipt write + // and --help, excludeMismatch, the two docs; no_claim=binding the digest to the review ledger (write never reads it) + test('write binds --review-exclude to the base policy, not to a review, and no description claims more', () => { + const repo = census(); + const body = path.join(repo.root, 'body.md'); + const refused = writeReceipt(repo, body, []); + const expected = refused.err.match(/field=review_exclude expected=([0-9a-f]{64}) /)?.[1]; + expect(expected).toBeDefined(); + expect(writeReceipt(repo, body, ['--review-exclude', expected!, ...targetedAtHead(repo)]).status).toBe(EXIT.ok); + const doc = (rel: string) => fs.readFileSync(path.join(import.meta.dir, '..', rel), 'utf8').replace(/\s+/g, ' '); + const claims = { + help: runBin(repo, 'gstack-ship-receipt', ['--help']).out.replace(/\s+/g, ' '), + troubleshooting: doc('docs/troubleshooting.md'), + shipPolicy: doc('docs/ship-policy.md'), + queue: excludeMismatch({ state: 'none' }, { review_exclude: { digest: expected } } as any, repo.git('rev-parse', 'HEAD'), 'origin/main')!, + }; + for (const [where, text] of Object.entries(claims)) { + expect({ where, overClaim: /write alone (cannot|does not) clear|never a receipt write alone/i.test(text) }).toEqual({ where, overClaim: false }); + expect({ where, says: text.includes('does not read the review ledger') }).toEqual({ where, says: true }); + } + }, 240_000); }); describe('a branch cannot hide code under a base pattern', () => { @@ -348,6 +373,58 @@ describe('a branch cannot hide code under a base pattern', () => { expect(p).toMatchObject({ depth: 'full', errors: expect.arrayContaining([expect.objectContaining({ code: 'binary_code_path', input: 'assets/loader.js' })]) }); }, 120_000); + // Value: protects=a compiled binary under a base pattern stays in review, because it is code; fails_when=a .node, .so, .so.N, + // .dylib or .dll file, or an ELF, Mach-O or PE image under a content extension (.bin, .dat), is excluded as binary content; + // why_new=GST-972 item 3; seam=bin/gstack-review-plan; no_claim=what a reader can see inside a binary + test('native binaries are code: .node, .so, .dylib, .dll and executable images under a content extension stay kept', () => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['assets/**']) } }); repos.push(repo); + fs.mkdirSync(path.join(repo.work, 'assets'), { recursive: true }); + const elf = Buffer.concat([Buffer.from([0x7f, 0x45, 0x4c, 0x46, 2, 1, 1, 0]), Buffer.alloc(24)]); + const macho = Buffer.concat([Buffer.from([0xcf, 0xfa, 0xed, 0xfe]), Buffer.alloc(28)]); + const pe = Buffer.concat([Buffer.from('MZ'), Buffer.alloc(30)]); + const files = { 'assets/addon.node': elf, 'assets/libx.dylib': macho, 'assets/libx.so': elf, 'assets/libx.so.1': elf, 'assets/setup.dat': pe, 'assets/tool.bin': elf, 'assets/x.dll': pe, 'assets/real.gz': GZ }; + for (const [rel, bytes] of Object.entries(files)) fs.writeFileSync(path.join(repo.work, rel), bytes); + commit(repo, {}); + const p = JSON.parse(plan(repo, ['--json']).out); + expect(p.review_exclude.kept_files).toEqual(Object.keys(files).filter(f => f !== 'assets/real.gz').map(f => ({ path: f, reason: 'code' }))); + expect(p.review_exclude).toMatchObject({ excluded: 1, content: { binary: 1 } }); + }, 120_000); + + // Value: protects=a NUL byte cannot turn code or a key file into excludable content; fails_when=a NUL-bearing file with a + // source extension outside the old CODE_EXT list (.gradle, .sql, .tf, .groovy), a key store (.p12) or an extensionless key + // is excluded as binary; why_new=GST-972 item 6 (#3852 targeted gate reader at 61803d9); seam=bin/gstack-review-plan; + // no_claim=the list of binary content extensions is complete (an unknown one is kept, never excluded) + test('a NUL-bearing file is binary content only under a known content extension; code and key files stay kept', () => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['assets/**']) } }); repos.push(repo); + fs.mkdirSync(path.join(repo.work, 'assets'), { recursive: true }); + const nul = (code: string) => Buffer.from(`/* \0 */ ${code}\n`); + const kept = { 'assets/Build.groovy': nul('println "x"'), 'assets/build.gradle': nul('apply plugin: "java"'), 'assets/id_ed25519': nul('-----BEGIN OPENSSH PRIVATE KEY-----'), 'assets/main.tf': nul('resource "x" "y" {}'), 'assets/query.sql': nul('DROP TABLE users;'), 'assets/server.p12': Buffer.from([0x30, 0x82, 0, 1, 2, 3]) }; + const content = { 'assets/report.pdf': GZ, 'assets/shot.png': GZ }; + for (const [rel, bytes] of Object.entries({ ...kept, ...content })) fs.writeFileSync(path.join(repo.work, rel), bytes); + commit(repo, {}); + const p = JSON.parse(plan(repo, ['--json']).out); + expect(p.review_exclude.kept_files).toEqual(Object.keys(kept).map(f => ({ path: f, reason: 'code' }))); + expect(p.review_exclude).toMatchObject({ excluded: 2, content: { binary: 2 } }); + }, 120_000); + + // Value: protects=a branch's .gitattributes cannot hide a credential line inside a matched file by making git call it + // binary, at the plan (live tree) and at the receipt (head blobs); fails_when=the secret-line rule reads only the + // collector's added lines, which git leaves empty for an attribute-binary file, so the file is excluded; why_new=GST-972 + // item 5 (#3852 round 3, outside-voice#2); seam=bin/gstack-review-plan and reviewExclude in head mode; no_claim=pregate's gitleaks scan + test('a branch .gitattributes marking receipts binary cannot hide a credential line: the keep rule reads the text', () => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['receipts/**']) } }); repos.push(repo); + commit(repo, { '.gitattributes': 'receipts/** binary\n', 'receipts/s.json': '{"cmd": "curl -H $OPENAI_API_KEY"}\n', 'receipts/a.json': '{"ok": true}\n' }); + const p = JSON.parse(plan(repo, ['--json']).out); + expect(p.review_exclude.kept_files).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); + expect(p.review_exclude).toMatchObject({ excluded: 1, content: { data: 1 } }); + const head = repo.git('rev-parse', 'HEAD'); + const baseSha = repo.git('rev-parse', 'origin/main'); + const c = collectCommit(repo.work, baseSha, head); + const x = reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: c.mergeBase!, head, perPath: c.perPath, added: c.added, classRoot: c.root ?? [], gateTagged: new Set(), context: 'test' }); + expect(x.state === 'active' && x.kept).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); + expect(x.state === 'active' && x.record.digest).toBe(p.review_exclude.digest); + }, 120_000); + test('an unavailable gate scan keeps every candidate instead of excluding it', () => { const repo = census(); const head = repo.git('rev-parse', 'HEAD'); From 78c09eea9d98bdfda59c25652e853aa9654facb9 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 01:55:42 +0000 Subject: [PATCH 02/10] test(review): GST-972 key-file fixture holds no key header the credential scan flags Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-exclude.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index de38365b1..8d7b4f942 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -398,7 +398,7 @@ describe('a branch cannot hide code under a base pattern', () => { const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['assets/**']) } }); repos.push(repo); fs.mkdirSync(path.join(repo.work, 'assets'), { recursive: true }); const nul = (code: string) => Buffer.from(`/* \0 */ ${code}\n`); - const kept = { 'assets/Build.groovy': nul('println "x"'), 'assets/build.gradle': nul('apply plugin: "java"'), 'assets/id_ed25519': nul('-----BEGIN OPENSSH PRIVATE KEY-----'), 'assets/main.tf': nul('resource "x" "y" {}'), 'assets/query.sql': nul('DROP TABLE users;'), 'assets/server.p12': Buffer.from([0x30, 0x82, 0, 1, 2, 3]) }; + const kept = { 'assets/Build.groovy': nul('println "x"'), 'assets/build.gradle': nul('apply plugin: "java"'), 'assets/id_ed25519': nul('openssh-key-v1'), 'assets/main.tf': nul('resource "x" "y" {}'), 'assets/query.sql': nul('DROP TABLE users;'), 'assets/server.p12': Buffer.from([0x30, 0x82, 0, 1, 2, 3]) }; const content = { 'assets/report.pdf': GZ, 'assets/shot.png': GZ }; for (const [rel, bytes] of Object.entries({ ...kept, ...content })) fs.writeFileSync(path.join(repo.work, rel), bytes); commit(repo, {}); From bbe8b11aca21823b7584aad0fa2931c95b619b80 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:04:55 +0000 Subject: [PATCH 03/10] fix: pre-landing review fixes attributeBinaryLines reads attribute-binary files in one git diff per argument-byte chunk instead of one per file (red-team#2). The changelog and ship-policy row say readers' own diffs still show such a kept file as binary (red-team#1, deferred as #3905). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .../gst-972-review-exclude-follow-ups.md | 2 +- docs/ship-policy.md | 2 +- lib/review-exclude.ts | 23 ++++++++++++++----- test/review-exclude.test.ts | 16 +++++++------ 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/changelog.d/gst-972-review-exclude-follow-ups.md b/changelog.d/gst-972-review-exclude-follow-ups.md index 279d57153..b1c4644db 100644 --- a/changelog.d/gst-972-review-exclude-follow-ups.md +++ b/changelog.d/gst-972-review-exclude-follow-ups.md @@ -3,5 +3,5 @@ ### Fixed - **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A binary file now starting with ELF, Mach-O, PE, WebAssembly or `#!` magic is kept as `code` whatever its extension. - **A NUL byte no longer makes code excludable.** A binary file was content unless its extension was on a short source list, so `.gradle`, `.sql`, `.tf`, `.groovy`, a `.p12` key store or an extensionless private key with a NUL byte was excluded. A binary file is now content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat` and data blobs); every other binary file is kept as `code`. -- **A branch's `.gitattributes` cannot hide a credential line.** Marking `receipts/**` binary left the collector's added lines empty, so the secret-line keep rule saw nothing and the file was excluded. For a file git calls binary only because of its attributes, the rule now reads `git diff --text --no-textconv --no-ext-diff`. +- **A branch's `.gitattributes` cannot get a file with a credential line excluded.** Marking `receipts/**` binary left the collector's added lines empty, so the secret-line keep rule saw nothing and the file was excluded. For a file git calls binary only because of its attributes, the rule now reads `git diff --text --no-textconv --no-ext-diff`, so the file stays in review and the plan lists it as `REVIEW_EXCLUDE_KEPT: secret-line`. Readers' own diffs still follow the branch's attributes and show that file as `Binary files differ`; reading such files as text in every reader's diff is a separate follow-up. - **`gstack-ship-receipt write --review-exclude` docs are exact.** The help, `docs/troubleshooting.md`, `docs/ship-policy.md` and the queue's refusal line said a receipt write alone cannot clear `field=review_exclude`. Write checks the digest against the base policy and does not read the review ledger, so a write given the refusal's `expected=` digest passes without a review. The text now says that, and that the review rerun is the fix. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 551153b24..536a3b44c 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1301,7 +1301,7 @@ Everything else stays in every reader's diff with one reason code: | `config` | it is a data-shaped config file (`package.json`, `tsconfig*.json`, `config/`, dotfiles) | | `gate-tagged` | the gate scan tagged one of its hunks | | `code` | it is code or a test, not content; a binary file outside the content extensions above, or one with executable magic, counts as code | -| `secret-line` | an added line reads a credential (the targeted pass's secret rule); for a file git calls binary only because of its attributes, the added lines come from `git diff --text`, which `.gitattributes` cannot change | +| `secret-line` | an added line reads a credential (the targeted pass's secret rule); for a file git calls binary only because of its attributes, the added lines come from `git diff --text`, which `.gitattributes` cannot change (readers' own diffs still follow the attributes and show the kept file as binary) | | `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; 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 | diff --git a/lib/review-exclude.ts b/lib/review-exclude.ts index 7f4c48d21..8d45cfacd 100644 --- a/lib/review-exclude.ts +++ b/lib/review-exclude.ts @@ -158,16 +158,27 @@ function binaryContent(i: ExcludeInput, files: ReadonlyMap, cla /** * The added lines of matched files git calls binary only because of their attributes (the bytes hold no NUL): the * collector's added lines are empty for them, so the secret-line rule reads `git diff --text` instead, which no - * .gitattributes, textconv or external diff driver changes. A git failure puts the path in `unread`. + * .gitattributes, textconv or external diff driver changes. One git diff per KEEP_ARG_MAX_BYTES of literal pathspecs; + * git prints one section per path in byte order, so a chunk whose sections do not line up with its sorted paths, or a + * git failure, puts every path of that chunk in `unread`. */ function attributeBinaryLines(i: ExcludeInput, paths: readonly string[], unread: Set): Record { const out: Record = {}; const range = i.head ? [i.mergeBase, i.head] : [i.mergeBase]; - for (const p of paths) { - const r = git(i.root, ['diff', '--text', '--no-textconv', '--no-ext-diff', '--no-renames', '-U0', ...range, '--', literal(p)]); - if (!r.ok) { unread.add(p); continue; } - let hunk = false; - out[p] = r.out.split('\n').filter(l => (hunk ||= l.startsWith('@@')) && l.startsWith('+')).map(l => l.slice(1)); + const chunks: string[][] = []; + let bytes = Infinity; + for (const p of [...paths].sort((a, b) => Buffer.compare(Buffer.from(a), Buffer.from(b)))) { + const n = argBytes([p]); + if (bytes + n > KEEP_ARG_MAX_BYTES) { chunks.push([]); bytes = 0; } + chunks[chunks.length - 1]!.push(p); + bytes += n; + } + for (const chunk of chunks) { + const r = git(i.root, ['diff', '--text', '--no-textconv', '--no-ext-diff', '--no-renames', '--src-prefix=a/', '--dst-prefix=b/', '-U0', ...range, '--', ...chunk.map(literal)]); + const sections = r.ok ? r.out.split(/^(?=diff --git )/m).filter(s => s.startsWith('diff --git ')) : []; + const aligned = sections.length === chunk.length && chunk.every((p, k) => /["\\\x00-\x1f\x7f]/.test(p) || sections[k]!.startsWith(`diff --git a/${p} b/${p}\n`)); + if (!aligned) { for (const p of chunk) unread.add(p); continue; } + chunk.forEach((p, k) => { let hunk = false; out[p] = sections[k]!.split('\n').filter(l => (hunk ||= l.startsWith('@@')) && l.startsWith('+')).map(l => l.slice(1)); }); } return out; } diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index 8d7b4f942..49847ad7d 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -407,21 +407,23 @@ describe('a branch cannot hide code under a base pattern', () => { expect(p.review_exclude).toMatchObject({ excluded: 2, content: { binary: 2 } }); }, 120_000); - // Value: protects=a branch's .gitattributes cannot hide a credential line inside a matched file by making git call it + // Value: protects=a branch's .gitattributes cannot get a matched file with a credential line excluded by making git call it // binary, at the plan (live tree) and at the receipt (head blobs); fails_when=the secret-line rule reads only the // collector's added lines, which git leaves empty for an attribute-binary file, so the file is excluded; why_new=GST-972 - // item 5 (#3852 round 3, outside-voice#2); seam=bin/gstack-review-plan and reviewExclude in head mode; no_claim=pregate's gitleaks scan - test('a branch .gitattributes marking receipts binary cannot hide a credential line: the keep rule reads the text', () => { + // item 5 (#3852 round 3, outside-voice#2); seam=bin/gstack-review-plan and reviewExclude in head mode; no_claim=pregate's gitleaks scan, what readers' own diffs show (they still follow the attributes) + test('a branch .gitattributes marking receipts binary cannot get a credential-line file excluded: the keep rule reads the text', () => { const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['receipts/**']) } }); repos.push(repo); - commit(repo, { '.gitattributes': 'receipts/** binary\n', 'receipts/s.json': '{"cmd": "curl -H $OPENAI_API_KEY"}\n', 'receipts/a.json': '{"ok": true}\n' }); + const secret = '{"cmd": "curl -H $OPENAI_API_KEY"}\n'; + commit(repo, { '.gitattributes': 'receipts/** binary\n', 'receipts/s.json': secret, 'receipts/a.b/c.json': secret, 'receipts/a.json': '{"ok": true}\n', 'receipts/a/t.json': '{"ok": true}\n' }); const p = JSON.parse(plan(repo, ['--json']).out); - expect(p.review_exclude.kept_files).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); - expect(p.review_exclude).toMatchObject({ excluded: 1, content: { data: 1 } }); + const kept = [{ path: 'receipts/a.b/c.json', reason: 'secret-line' }, { path: 'receipts/s.json', reason: 'secret-line' }]; + expect(p.review_exclude.kept_files).toEqual(kept); + expect(p.review_exclude).toMatchObject({ excluded: 2, content: { data: 2 } }); const head = repo.git('rev-parse', 'HEAD'); const baseSha = repo.git('rev-parse', 'origin/main'); const c = collectCommit(repo.work, baseSha, head); const x = reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: c.mergeBase!, head, perPath: c.perPath, added: c.added, classRoot: c.root ?? [], gateTagged: new Set(), context: 'test' }); - expect(x.state === 'active' && x.kept).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); + expect(x.state === 'active' && x.kept).toEqual(kept); expect(x.state === 'active' && x.record.digest).toBe(p.review_exclude.digest); }, 120_000); From 2ee8763ba37fe53855807789a0603bf45b2aa5b1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:16:37 +0000 Subject: [PATCH 04/10] docs: the native-magic check covers files git reports as binary; attribute-text files are #3910 Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-972-review-exclude-follow-ups.md | 2 +- docs/ship-policy.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog.d/gst-972-review-exclude-follow-ups.md b/changelog.d/gst-972-review-exclude-follow-ups.md index b1c4644db..b2368189d 100644 --- a/changelog.d/gst-972-review-exclude-follow-ups.md +++ b/changelog.d/gst-972-review-exclude-follow-ups.md @@ -1,7 +1,7 @@ **`review_exclude_paths` keeps code it used to exclude, and the docs say exactly what the receipt check stops (GST-972).** Three ways to get code or a credential out of review under a base pattern are closed, each with a failing-first test in `test/review-exclude.test.ts`, and the receipt docs no longer over-claim. ### Fixed -- **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A binary file now starting with ELF, Mach-O, PE, WebAssembly or `#!` magic is kept as `code` whatever its extension. +- **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A file git reports as binary that starts with ELF, Mach-O, PE, WebAssembly or `#!` magic is now kept as `code` whatever its extension. A branch's `.gitattributes` can still make git read such a file as text, which skips the byte check (#3910). - **A NUL byte no longer makes code excludable.** A binary file was content unless its extension was on a short source list, so `.gradle`, `.sql`, `.tf`, `.groovy`, a `.p12` key store or an extensionless private key with a NUL byte was excluded. A binary file is now content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat` and data blobs); every other binary file is kept as `code`. - **A branch's `.gitattributes` cannot get a file with a credential line excluded.** Marking `receipts/**` binary left the collector's added lines empty, so the secret-line keep rule saw nothing and the file was excluded. For a file git calls binary only because of its attributes, the rule now reads `git diff --text --no-textconv --no-ext-diff`, so the file stays in review and the plan lists it as `REVIEW_EXCLUDE_KEPT: secret-line`. Readers' own diffs still follow the branch's attributes and show that file as `Binary files differ`; reading such files as text in every reader's diff is a separate follow-up. - **`gstack-ship-receipt write --review-exclude` docs are exact.** The help, `docs/troubleshooting.md`, `docs/ship-policy.md` and the queue's refusal line said a receipt write alone cannot clear `field=review_exclude`. Write checks the digest against the base policy and does not read the review ledger, so a write given the refusal's `expected=` digest passes without a review. The text now says that, and that the review rerun is the fix. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 536a3b44c..54a99c39e 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1287,7 +1287,7 @@ A pattern that matches no changed file is listed as `unmatched=` on the plan lin **What is excluded, and what is kept.** A matched file is excluded only when it is content: a data file (JSON, JSONL, CSV, TSV, images), a docs-class file, a -`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Under those rules it is content even under +`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Only files git reports as binary get this byte check, so a branch's `.gitattributes` that makes git read a file as text skips it (#3910). Under those rules it is content even under `config/` or in a skill directory, and only `policy-dir`, `gate-path`, `executable`, `link` or `gate-tagged` keep it. Every other binary file is `code`: source with a NUL, a native library (`.node`, `.so`, `.dylib`, `.dll`), a compiled executable, a key store, a pickle, or a file with no extension. Everything else stays in every reader's diff with one reason code: From d1cb5511555a0c2c88eb782f64211b26e199c892 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:17:43 +0000 Subject: [PATCH 05/10] chore: changelog fragment named for the branch Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- ...lude-follow-ups.md => capy-reviewexcludepaths-keeps-native.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{gst-972-review-exclude-follow-ups.md => capy-reviewexcludepaths-keeps-native.md} (100%) diff --git a/changelog.d/gst-972-review-exclude-follow-ups.md b/changelog.d/capy-reviewexcludepaths-keeps-native.md similarity index 100% rename from changelog.d/gst-972-review-exclude-follow-ups.md rename to changelog.d/capy-reviewexcludepaths-keeps-native.md From ca2e4c2667e4c7b87fe88855c33571ce5a09a44a Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:19:53 +0000 Subject: [PATCH 06/10] docs: ship-policy config row names content binaries and keeps native ones (Step 14.5 audit) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/ship-policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 54a99c39e..6e6039a13 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -58,7 +58,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `gate_command` | command or `null` | `null` | The repo's full pre-merge gate. `gstack-ship-queue gate-ahead` and `ready` print it for the caller to run (in the background or in CI); gstack never executes it. Needs `min_gstack_version`. | | `full_e2e` | `true` \| `false` | `false` | `true`: every merge needs a passing full-gate result bound to a tree equal to the merging one modulo stamps; `gstack-ship-queue ready`, `stamp` and `verify` refuse without one (`QUEUE_GATE_MISSING`): a gate-ahead record, or the lane's `--gate-exit 0 --gate-log ` from its run on the gated head. Needs `min_gstack_version`. | | `review_scope` | `frozen` \| `diff-plus-blocking` \| `all` | `frozen` | The finding scope of `/ship`'s review loop; see [review_scope](#review-scope). `frozen` fixes the diff on pass 1, files blocking findings in unchanged code as issues, and later passes read only the range since the previous pass. `diff-plus-blocking` is the same but fixes a blocking finding in unchanged code in this PR instead of filing it. `all` is the earlier loop: every finding is fixed on every pass. Needs `min_gstack_version` when not `frozen`. | -| `review_exclude_paths` | git glob pathspecs | `[]` | Content files (data, docs, binary, `.log`) `/ship`'s reviewers leave out; read from the base only, with code, gate, config, executable and named-by-code files kept. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | +| `review_exclude_paths` | git glob pathspecs | `[]` | Content files (data, docs, content binaries such as images, fonts and archives, `.log`) `/ship`'s reviewers leave out; read from the base only, with code (native and other non-content binaries included), gate, config, executable and named-by-code files kept. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | | `min_gstack_version` | version or `null` | `null` | The oldest gstack that may read the file; see the compatibility rule above. | | `docs` | `{ caps: [{ path, max_bytes }] }` or `null` | `null` | Documentation files with a byte cap. `/ship` Step 14.5 runs `gstack-ship-receipt docs-caps --base ` (the caps come from `origin/`'s policy, never the branch's copy; `--policy ` is for a repository bootstrapping its policy) before the verification gate and prints one `DOCS_CAP: / ok\|over` line per cap; any `over` (an absent file counts) exits 1 and blocks the ship until the file is trimmed or the cap raised on the base branch (`DOCS_AUDIT_BLOCKED`). At most 64 caps, each path contained. See [the ship receipt](#ship-receipt). Needs `min_gstack_version`. | | `train` | `{ gate_workflow, free_workflow, full_gate_check, record_artifact, max_size, fill_hours, screen_selection, trial_reservation_usd, land? }`, `{ gates, land, max_size, fill_hours }` or `null` | `null` | Turns on [merge trains](#run-a-merge-train), read from the base revision only. Every field is required and an unknown field is `POLICY_INVALID`: `max_size` 1 to 6, `fill_hours` above 0 and at most 24, `screen_selection` `gstack-paid` \| `none`, `trial_reservation_usd` above 0 and at most 100. A block with `gates` ([any repository](#train-gates)) takes `max_size` 1 to 5 and needs `min_gstack_version` at least `TRAIN_GATES_KEY_VERSION`. `null` keeps the per-PR queue exactly as it is. Needs `min_gstack_version`. | From f73815f9eb9a63b11d90bcee38bd13638e2bb5d8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:22:09 +0000 Subject: [PATCH 07/10] docs: drop the post-review ship-policy sentence; #3910 stays in the changelog and PR notes Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/ship-policy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 6e6039a13..9ee82a608 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1287,7 +1287,7 @@ A pattern that matches no changed file is listed as `unmatched=` on the plan lin **What is excluded, and what is kept.** A matched file is excluded only when it is content: a data file (JSON, JSONL, CSV, TSV, images), a docs-class file, a -`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Only files git reports as binary get this byte check, so a branch's `.gitattributes` that makes git read a file as text skips it (#3910). Under those rules it is content even under +`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Under those rules it is content even under `config/` or in a skill directory, and only `policy-dir`, `gate-path`, `executable`, `link` or `gate-tagged` keep it. Every other binary file is `code`: source with a NUL, a native library (`.node`, `.so`, `.dylib`, `.dll`), a compiled executable, a key store, a pickle, or a file with no extension. Everything else stays in every reader's diff with one reason code: From bf48ff697a569bed9f5e9b062f8422abf0fc06b6 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 03:06:32 +0000 Subject: [PATCH 08/10] test(review): kept reasons typed as literals for the test typecheck ratchet Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-exclude.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index 49847ad7d..a6f9f3255 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -416,7 +416,7 @@ describe('a branch cannot hide code under a base pattern', () => { const secret = '{"cmd": "curl -H $OPENAI_API_KEY"}\n'; commit(repo, { '.gitattributes': 'receipts/** binary\n', 'receipts/s.json': secret, 'receipts/a.b/c.json': secret, 'receipts/a.json': '{"ok": true}\n', 'receipts/a/t.json': '{"ok": true}\n' }); const p = JSON.parse(plan(repo, ['--json']).out); - const kept = [{ path: 'receipts/a.b/c.json', reason: 'secret-line' }, { path: 'receipts/s.json', reason: 'secret-line' }]; + const kept = [{ path: 'receipts/a.b/c.json', reason: 'secret-line' as const }, { path: 'receipts/s.json', reason: 'secret-line' as const }]; expect(p.review_exclude.kept_files).toEqual(kept); expect(p.review_exclude).toMatchObject({ excluded: 2, content: { data: 2 } }); const head = repo.git('rev-parse', 'HEAD'); From 645770c3d15f78afeaae44de598511696f319350 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 03:41:59 +0000 Subject: [PATCH 09/10] fix(review): sniff executable magic on every file git's own bytes call binary, whatever .gitattributes says; exact review_exclude docs (GST-972 round 2) bytesBinary reads git diff --numstat with attributes from the empty tree, so a branch's '*.png diff -binary' cannot make an ELF read as text and skip the magic check (#3910). Adds 64-bit universal Mach-O magic. ship-policy's key row lists every kept class; troubleshooting and the write/queue refusal lines say a review_exclude mismatch compares the --review-exclude argument, not the review ledger. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 2 +- .../capy-reviewexcludepaths-keeps-native.md | 4 +-- docs/ship-policy.md | 6 ++--- docs/troubleshooting.md | 11 ++++---- lib/review-exclude.ts | 25 ++++++++++++++----- lib/ship-receipt-depth.ts | 2 +- test/review-exclude.test.ts | 24 ++++++++++++++++++ 7 files changed, 56 insertions(+), 18 deletions(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 866e77ec8..c9856ff09 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -231,7 +231,7 @@ function cmdWrite(rest: string[], json: boolean): number { const excludeNow = review.exclude.state === 'active' ? review.exclude.record : null; const excludeGiven = flag(rest, '--review-exclude') ?? 'none'; if ((excludeNow?.digest ?? 'none') !== excludeGiven) { - process.stderr.write(`gstack-ship-receipt write: field=review_exclude expected=${excludeNow?.digest ?? 'none'} recorded=${excludeGiven} source=${excludeNow?.source ?? `origin/${base} (no review_exclude_paths)`}: the review --review-exclude names read another selection than ${base}'s review_exclude_paths gives ${head.slice(0, 12)}; fix: rerun /ship's review on ${head.slice(0, 12)} (gstack-review-dispatch pass --base ${base}), run gstack-review-dispatch verify --ledger and write with the --review-exclude its REVIEW_EXCLUDE_RECORD: line prints (none when it prints no such line); nothing written (RECEIPT_INVALID)\n`); + process.stderr.write(`gstack-ship-receipt write: field=review_exclude expected=${excludeNow?.digest ?? 'none'} recorded=${excludeGiven} source=${excludeNow?.source ?? `origin/${base} (no review_exclude_paths)`}: the --review-exclude argument names another selection than ${base}'s review_exclude_paths gives ${head.slice(0, 12)} (write compares the argument, not the review ledger); fix: rerun /ship's review on ${head.slice(0, 12)} (gstack-review-dispatch pass --base ${base}), run gstack-review-dispatch verify --ledger and write with the --review-exclude its REVIEW_EXCLUDE_RECORD: line prints (none when it prints no such line); nothing written (RECEIPT_INVALID)\n`); return EXIT.fail; } const product = reviewedProduct(repoRoot, base, head, pregateFile); diff --git a/changelog.d/capy-reviewexcludepaths-keeps-native.md b/changelog.d/capy-reviewexcludepaths-keeps-native.md index b2368189d..45bfc6314 100644 --- a/changelog.d/capy-reviewexcludepaths-keeps-native.md +++ b/changelog.d/capy-reviewexcludepaths-keeps-native.md @@ -1,7 +1,7 @@ **`review_exclude_paths` keeps code it used to exclude, and the docs say exactly what the receipt check stops (GST-972).** Three ways to get code or a credential out of review under a base pattern are closed, each with a failing-first test in `test/review-exclude.test.ts`, and the receipt docs no longer over-claim. ### Fixed -- **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A file git reports as binary that starts with ELF, Mach-O, PE, WebAssembly or `#!` magic is now kept as `code` whatever its extension. A branch's `.gitattributes` can still make git read such a file as text, which skips the byte check (#3910). +- **Native binaries are code.** A `.node`, `.so`, `.dylib` or `.dll` file under a base pattern was excluded as binary content, and so was a compiled executable with a data extension. A binary file that starts with ELF, Mach-O (universal binaries included), PE, WebAssembly or `#!` magic is now kept as `code` whatever its extension. Binary here is git's own verdict on the bytes, read with no attributes from the tree as well as with the branch's, so a branch's `.gitattributes` (`*.png diff -binary`) cannot make an ELF read as text and skip the check (#3910). - **A NUL byte no longer makes code excludable.** A binary file was content unless its extension was on a short source list, so `.gradle`, `.sql`, `.tf`, `.groovy`, a `.p12` key store or an extensionless private key with a NUL byte was excluded. A binary file is now content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat` and data blobs); every other binary file is kept as `code`. - **A branch's `.gitattributes` cannot get a file with a credential line excluded.** Marking `receipts/**` binary left the collector's added lines empty, so the secret-line keep rule saw nothing and the file was excluded. For a file git calls binary only because of its attributes, the rule now reads `git diff --text --no-textconv --no-ext-diff`, so the file stays in review and the plan lists it as `REVIEW_EXCLUDE_KEPT: secret-line`. Readers' own diffs still follow the branch's attributes and show that file as `Binary files differ`; reading such files as text in every reader's diff is a separate follow-up. -- **`gstack-ship-receipt write --review-exclude` docs are exact.** The help, `docs/troubleshooting.md`, `docs/ship-policy.md` and the queue's refusal line said a receipt write alone cannot clear `field=review_exclude`. Write checks the digest against the base policy and does not read the review ledger, so a write given the refusal's `expected=` digest passes without a review. The text now says that, and that the review rerun is the fix. +- **`gstack-ship-receipt write --review-exclude` docs are exact.** The help, `docs/troubleshooting.md`, `docs/ship-policy.md` and the queue's refusal line said a receipt write alone cannot clear `field=review_exclude`. Write checks the digest against the base policy and does not read the review ledger, so a write given the refusal's `expected=` digest passes without a review. A mismatch proves only that the `--review-exclude` argument (or the digest the receipt recorded from it) differs from the head's selection, not what the readers read. The text now says that, and that the review rerun is the fix. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 9ee82a608..95f569b06 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -58,7 +58,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `gate_command` | command or `null` | `null` | The repo's full pre-merge gate. `gstack-ship-queue gate-ahead` and `ready` print it for the caller to run (in the background or in CI); gstack never executes it. Needs `min_gstack_version`. | | `full_e2e` | `true` \| `false` | `false` | `true`: every merge needs a passing full-gate result bound to a tree equal to the merging one modulo stamps; `gstack-ship-queue ready`, `stamp` and `verify` refuse without one (`QUEUE_GATE_MISSING`): a gate-ahead record, or the lane's `--gate-exit 0 --gate-log ` from its run on the gated head. Needs `min_gstack_version`. | | `review_scope` | `frozen` \| `diff-plus-blocking` \| `all` | `frozen` | The finding scope of `/ship`'s review loop; see [review_scope](#review-scope). `frozen` fixes the diff on pass 1, files blocking findings in unchanged code as issues, and later passes read only the range since the previous pass. `diff-plus-blocking` is the same but fixes a blocking finding in unchanged code in this PR instead of filing it. `all` is the earlier loop: every finding is fixed on every pass. Needs `min_gstack_version` when not `frozen`. | -| `review_exclude_paths` | git glob pathspecs | `[]` | Content files (data, docs, content binaries such as images, fonts and archives, `.log`) `/ship`'s reviewers leave out; read from the base only, with code (native and other non-content binaries included), gate, config, executable and named-by-code files kept. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | +| `review_exclude_paths` | git glob pathspecs | `[]` | Content files `/ship`'s reviewers leave out: data, docs, `.log`, and binaries under a content extension (images, fonts, archives and the like) that do not start with executable magic. Read from the base only. Code (native libraries and every other binary outside the content extensions included), gate paths, agent prose, config, executables, links, `.gstack/` and `.github/` files, gate-tagged, secret-line and named-by-code files stay in review. See [review_exclude_paths](#review-exclude-paths). Needs `min_gstack_version`. | | `min_gstack_version` | version or `null` | `null` | The oldest gstack that may read the file; see the compatibility rule above. | | `docs` | `{ caps: [{ path, max_bytes }] }` or `null` | `null` | Documentation files with a byte cap. `/ship` Step 14.5 runs `gstack-ship-receipt docs-caps --base ` (the caps come from `origin/`'s policy, never the branch's copy; `--policy ` is for a repository bootstrapping its policy) before the verification gate and prints one `DOCS_CAP: / ok\|over` line per cap; any `over` (an absent file counts) exits 1 and blocks the ship until the file is trimmed or the cap raised on the base branch (`DOCS_AUDIT_BLOCKED`). At most 64 caps, each path contained. See [the ship receipt](#ship-receipt). Needs `min_gstack_version`. | | `train` | `{ gate_workflow, free_workflow, full_gate_check, record_artifact, max_size, fill_hours, screen_selection, trial_reservation_usd, land? }`, `{ gates, land, max_size, fill_hours }` or `null` | `null` | Turns on [merge trains](#run-a-merge-train), read from the base revision only. Every field is required and an unknown field is `POLICY_INVALID`: `max_size` 1 to 6, `fill_hours` above 0 and at most 24, `screen_selection` `gstack-paid` \| `none`, `trial_reservation_usd` above 0 and at most 100. A block with `gates` ([any repository](#train-gates)) takes `max_size` 1 to 5 and needs `min_gstack_version` at least `TRAIN_GATES_KEY_VERSION`. `null` keeps the per-PR queue exactly as it is. Needs `min_gstack_version`. | @@ -1287,7 +1287,7 @@ A pattern that matches no changed file is listed as `unmatched=` on the plan lin **What is excluded, and what is kept.** A matched file is excluded only when it is content: a data file (JSON, JSONL, CSV, TSV, images), a docs-class file, a -`.log` file, or a file git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) whose bytes hold a NUL in their first 8,000, so a branch's `.gitattributes` cannot make a text file binary content. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O, PE, WebAssembly, `#!`). Under those rules it is content even under +`.log` file, or a binary file: one git reports as binary (`-\t-` in `--numstat`; readers would only see `Binary files differ`) under the branch's attributes or with no attributes from the tree, so a branch's `.gitattributes` cannot make an executable read as text, and whose bytes hold a NUL in their first 8,000, so it cannot make a text file binary content either. A binary file is content only under a known content extension (images, fonts, media, documents, compressed archives, `.bin`, `.dat`, and data blobs such as `.parquet`, `.npy` or `.sqlite`) and only when its bytes do not start with an executable image's magic (ELF, Mach-O including universal binaries, PE, WebAssembly, `#!`). Under those rules it is content even under `config/` or in a skill directory, and only `policy-dir`, `gate-path`, `executable`, `link` or `gate-tagged` keep it. Every other binary file is `code`: source with a NUL, a native library (`.node`, `.so`, `.dylib`, `.dll`), a compiled executable, a key store, a pickle, or a file with no extension. Everything else stays in every reader's diff with one reason code: @@ -1351,7 +1351,7 @@ paths and three hashes). The `digest` hashes the declared patterns and the excluded and kept lists; `source` is provenance only. Write, carry and the queue each recompute the digest at the current base and compare it, so a base merge or a moved base that leaves the patterns and the lists unchanged still carries -and binds. A different digest means the reviewers read another selection: +and binds. A different digest means the digest write was given (or the receipt recorded) names another selection than the head has now: write refuses with `RECEIPT_INVALID field=review_exclude`, carry with `RECEIPT_CARRY_REFUSED review_exclude changed`, and the queue with `RECEIPT_INVALID field=review_exclude expected= recorded=`. The diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 8f1d5aa9f..e05e7cb27 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3338,11 +3338,12 @@ targeted refusal, run the targeted reader on HEAD, `gstack-review-dispatch verif use this code when the receipt's review_exclude digest (or the `--review-exclude ` write was given) is not the digest HEAD has under the base policy's [`review_exclude_paths`](ship-policy.md#review-exclude-paths). The line -names `expected=` (now), `recorded=` (what the review read, `none` when it read -without exclusion) and `source=` (the base commit). The reviewers read another -selection than the one the head now has. Write checks the digest against the base -policy only: it does not read the review ledger, so a write that passes the -`expected=` digest clears the field without a review. Do not do that. Rerun +names `expected=` (now), `recorded=` (the `--review-exclude` argument, or the digest +the receipt recorded from it; `none` without one) and `source=` (the base commit). +A mismatch proves only that this argument differs from the selection the head has +now. Write compares the argument and does not read the review ledger, so the line +says nothing about what the readers read, and a write that passes the `expected=` digest clears +the field without a review. Do not do that. Rerun `/ship`'s review on the head (`gstack-review-dispatch pass --base `), run `gstack-review-dispatch verify --ledger `, which prints `REVIEW_EXCLUDE_RECORD: digest=` from the briefs the readers got, then diff --git a/lib/review-exclude.ts b/lib/review-exclude.ts index 8d45cfacd..78d9131f6 100644 --- a/lib/review-exclude.ts +++ b/lib/review-exclude.ts @@ -122,14 +122,27 @@ function select(i: ExcludeInput, patterns: readonly string[]): { byPattern: Map< return { byPattern, files }; } -/** Executable image magic: ELF, Mach-O (thin and universal; 0xcafebabe is also a Java class), PE, WebAssembly and a script's #!. */ -const NATIVE_MAGIC = [[0x7f, 0x45, 0x4c, 0x46], [0xfe, 0xed, 0xfa, 0xce], [0xfe, 0xed, 0xfa, 0xcf], [0xce, 0xfa, 0xed, 0xfe], [0xcf, 0xfa, 0xed, 0xfe], [0xca, 0xfe, 0xba, 0xbe], [0x4d, 0x5a], [0x00, 0x61, 0x73, 0x6d], [0x23, 0x21]]; +/** Executable image magic: ELF, Mach-O (thin, universal and 64-bit universal; 0xcafebabe is also a Java class), PE, WebAssembly and a script's #!. */ +const NATIVE_MAGIC = [[0x7f, 0x45, 0x4c, 0x46], [0xfe, 0xed, 0xfa, 0xce], [0xfe, 0xed, 0xfa, 0xcf], [0xce, 0xfa, 0xed, 0xfe], [0xcf, 0xfa, 0xed, 0xfe], [0xca, 0xfe, 0xba, 0xbe], [0xca, 0xfe, 0xba, 0xbf], [0x4d, 0x5a], [0x00, 0x61, 0x73, 0x6d], [0x23, 0x21]]; + +/** + * The matched paths git calls binary by their bytes alone: `git diff --numstat` with attributes read from the empty tree, so + * a branch's .gitattributes (`diff`, `-binary`, `text`) cannot make an executable image read as text and skip the + * magic check. null when git cannot run it (a git without --attr-source); the caller then reads every matched file's bytes. + */ +function bytesBinary(i: ExcludeInput, patterns: readonly string[]): string[] | null { + const empty = git(i.root, ['hash-object', '-t', 'tree', '--stdin'], ''); + if (!empty.ok) return null; + const range = i.head ? [i.mergeBase, i.head] : [i.mergeBase]; + const r = git(i.root, [`--attr-source=${empty.out.trim()}`, 'diff', '--numstat', '-z', '--no-renames', ...range, '--', ...patterns.map(p => `:(top,glob)${p}`)]); + return r.ok ? r.out.split('\0').filter(l => l.startsWith('-\t-\t')).map(l => l.slice(4)) : null; +} const nativeImage = (b: Buffer) => NATIVE_MAGIC.some(m => m.every((x, k) => b[k] === x)); /** - * The matched paths git calls binary whose bytes hold a NUL in their first 8,000 (git's own test), read from the head blob, - * 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. A binary file that starts with executable image magic also goes in `native`. + * The claimed paths (git calls them binary under the branch's attributes or under none: bytesBinary) whose bytes hold a NUL + * in their first 8,000 (git's own test), read from the head blob, 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. A binary file that starts with executable image magic also goes in `native`. */ function binaryContent(i: ExcludeInput, files: ReadonlyMap, claimed: ReadonlySet, unread: Set, native: Set): Set { const out = new Set(); @@ -266,7 +279,7 @@ export function reviewExclude(i: ExcludeInput): Exclusion { const unread = new Set(); const native = new Set(); const claimed = i.perPath.filter(e => e.binary && sel.files.has(e.path)).map(e => e.path); - const binary = binaryContent(i, sel.files, new Set(claimed), unread, native); + const binary = binaryContent(i, sel.files, new Set([...claimed, ...(bytesBinary(i, patterns) ?? sel.files.keys())]), unread, native); const text = { ...i, added: { ...i.added, ...attributeBinaryLines(i, claimed.filter(p => !binary.has(p) && !unread.has(p)), unread) } }; const reasons = new Map(); for (const [p, m] of sel.files) reasons.set(p, unread.has(p) ? 'consumer-scan-unavailable' : pathReason(text, p, m, binary, native)); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index b48ebdd90..165f56b93 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -233,7 +233,7 @@ export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base if (expected === recorded) return null; const source = x.state === 'active' ? x.record.source : r.review_exclude?.source ?? base; const h = head.slice(0, 12); - return `field=review_exclude expected=${expected} recorded=${recorded} source=${source}: the reviewers read another selection than ${base}'s review_exclude_paths gives ${h}; the receipt is unchanged; fix: rerun /ship's review on ${h} (gstack-review-dispatch pass --base ${base.replace(/^(refs\/remotes\/)?origin\//, '')}), then gstack-ship-receipt write --pr --review-exclude --body with the digest gstack-review-dispatch verify --ledger prints on REVIEW_EXCLUDE_RECORD: (write checks the digest against the base policy and does not read the review ledger, so only the rerun shows the reviewers read this selection)`; + return `field=review_exclude expected=${expected} recorded=${recorded} source=${source}: the receipt records another selection than ${base}'s review_exclude_paths gives ${h}; the receipt is unchanged; fix: rerun /ship's review on ${h} (gstack-review-dispatch pass --base ${base.replace(/^(refs\/remotes\/)?origin\//, '')}), then gstack-ship-receipt write --pr --review-exclude --body with the digest gstack-review-dispatch verify --ledger prints on REVIEW_EXCLUDE_RECORD: (write checks the digest against the base policy and does not read the review ledger, so only the rerun shows the reviewers read this selection)`; } /** The receipt's recorded gate scan when it names this head; otherwise null (unavailable). */ const boundScan = (r: ShipReceipt, head: string): number | null => { diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index a6f9f3255..29e750d72 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -390,6 +390,30 @@ describe('a branch cannot hide code under a base pattern', () => { expect(p.review_exclude).toMatchObject({ excluded: 1, content: { binary: 1 } }); }, 120_000); + // Value: protects=a branch's .gitattributes cannot get an executable image excluded by making git read it as text, and a + // 64-bit universal Mach-O is an executable image; fails_when=only files git reports as binary under the branch's own + // attributes get the magic check, so an ELF named .png under '*.png diff -binary' is excluded as data, or 0xcafebabf + // is not executable magic; why_new=GST-972 item 3 follow-up (#3910, #3902 outside voice); seam=bin/gstack-review-plan and + // reviewExclude in head mode; no_claim=a NUL-free script under a content extension + test('a branch .gitattributes that makes git read an ELF as text cannot get it excluded; fat64 Mach-O is native', () => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-', baseFiles: { '.gstack/ship-policy.json': policyText(['assets/**']) } }); repos.push(repo); + fs.mkdirSync(path.join(repo.work, 'assets'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'assets/native.png'), Buffer.concat([Buffer.from([0x7f, 0x45, 0x4c, 0x46, 2, 1, 1, 0]), Buffer.alloc(24)])); + fs.writeFileSync(path.join(repo.work, 'assets/fat64.dat'), Buffer.concat([Buffer.from([0xca, 0xfe, 0xba, 0xbf]), Buffer.alloc(28)])); + fs.writeFileSync(path.join(repo.work, 'assets/real.gz'), GZ); + commit(repo, { '.gitattributes': '*.png diff -binary\n' }); + const p = JSON.parse(plan(repo, ['--json']).out); + const kept = [{ path: 'assets/fat64.dat', reason: 'code' as const }, { path: 'assets/native.png', reason: 'code' as const }]; + expect(p.review_exclude.kept_files).toEqual(kept); + expect(p.review_exclude).toMatchObject({ excluded: 1, content: { binary: 1 } }); + const head = repo.git('rev-parse', 'HEAD'); + const baseSha = repo.git('rev-parse', 'origin/main'); + const c = collectCommit(repo.work, baseSha, head); + const x = reviewExclude({ root: repo.work, baseSha, ref: 'origin/main', mergeBase: c.mergeBase!, head, perPath: c.perPath, added: c.added, classRoot: c.root ?? [], gateTagged: new Set(), context: 'test' }); + expect(x.state === 'active' && x.kept).toEqual(kept); + expect(x.state === 'active' && x.record.digest).toBe(p.review_exclude.digest); + }, 120_000); + // Value: protects=a NUL byte cannot turn code or a key file into excludable content; fails_when=a NUL-bearing file with a // source extension outside the old CODE_EXT list (.gradle, .sql, .tf, .groovy), a key store (.p12) or an extensionless key // is excluded as binary; why_new=GST-972 item 6 (#3852 targeted gate reader at 61803d9); seam=bin/gstack-review-plan; From b1400587de8891738195ad5193482b2e2ef0fc7e Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 03:51:20 +0000 Subject: [PATCH 10/10] fix(ship-receipt): carry refusal names the --review-exclude digest, not what reviewers read Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index c9856ff09..d4db5f5ce 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -751,7 +751,7 @@ function cmdCarry(rest: string[], json: boolean): number { const now = review.depth; if (review.exclude.state === 'refused') return refused('RECEIPT_CARRY_REFUSED', `${review.exclude.lines.join('; ')}; run /ship on ${head.slice(0, 12)} once the base policy reads`); const digest = review.exclude.state === 'active' ? review.exclude.record.digest : 'none'; - if (digest !== (receipt.review_exclude?.digest ?? 'none')) return refused('RECEIPT_CARRY_REFUSED', `review_exclude changed: recorded=${receipt.review_exclude?.digest ?? 'none'} now=${digest} source=${review.exclude.state === 'active' ? review.exclude.record.source : `${base} (no review_exclude_paths)`}; the reviewers read another selection than HEAD ${head.slice(0, 12)} has under the base policy, so carry does not apply; run /ship on ${head.slice(0, 12)}`); + if (digest !== (receipt.review_exclude?.digest ?? 'none')) return refused('RECEIPT_CARRY_REFUSED', `review_exclude changed: recorded=${receipt.review_exclude?.digest ?? 'none'} now=${digest} source=${review.exclude.state === 'active' ? review.exclude.record.source : `${base} (no review_exclude_paths)`}; the receipt's digest is the --review-exclude argument write was given, and it names another selection than HEAD ${head.slice(0, 12)} has under the base policy (carry compares that digest, not the review ledger), so carry does not apply; run /ship on ${head.slice(0, 12)}`); if (depthBelow(receipt.review_depth.depth, now.depth)) return refused('RECEIPT_CARRY_REFUSED', `HEAD ${head.slice(0, 12)} needs ${now.depth} review depth (${now.reasons.join(', ')}), above the receipt's ${receipt.review_depth.depth}; run /ship on ${head.slice(0, 12)}`); depthField = { review_depth: { ...now, depth: deeper(receipt.review_depth.depth, now.depth) }, review_exclude: receiptExclude(review.exclude) }; }