From 918ab93d8c962562d86a6c8b558a4647d65d12bb Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 6 Nov 2026 13:43:50 +0000 Subject: [PATCH] fix: pre-landing review fixes Review-Finding: review-p1r/specialist-batched#1 reader=testing Review-Finding: review-p1r/red-team#1 reader=red-team Review-Finding: review-p1r/red-team#2 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ship-receipt-depth.ts | 117 ++++++++++++++++++++---------------- test/review-exclude.test.ts | 60 ++++++++++++++++-- 2 files changed, 119 insertions(+), 58 deletions(-) diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 0260b3689..29fb0861a 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -34,7 +34,7 @@ import { reviewPolicy, type ReviewPolicy } from './ship-policy-review'; import { TARGETED_CLASSES, classList, targetedClasses, type TargetedClass } from './review-targeted'; import { runScriptSync } from './script-invocation'; import { bindReceipt, type ReceiptBindingCode } from './ship-receipt'; -import { carriedAudits, docsExceptionLine, parseReceiptDocs } from './ship-receipt-docs'; +import { carriedAudits, docsExceptionLine, parseReceiptDocs, type CarriedAudit } from './ship-receipt-docs'; export type ReceiptDepth = NonNullable; export type ReviewLoopException = NonNullable; @@ -328,10 +328,12 @@ const treatment = (x: ActiveExclusion, p: string) => (x.excluded.has(p) ? 'exclu * and every other file is selected alike on both sides. The audit's files are those whose content differs between `docsPre` * (the Prepare snapshot) and the post snapshot, reset to the Prepare content, so uncommitted docs the reviewers read stay; without * it every such file is an audit file, reset to the snapshot's HEAD. A post snapshot that carries this /ship run's earlier audits - * (`earlier`, all of its run_id; GST-1616) also resets each file one of them edited that changed since its HEAD, to its content + * (`earlier`, all of its run_id; GST-1616) also resets each file one of them edited that changed since its HEAD or that this audit edited again, to its content * before the first of them, when HEAD (or the latest audit's Prepare snapshot, for a file it edited again) holds what the last of - * them left. Content compares as git stores it or as it checks out (autocrlf, filters). Anything else, an unreadable snapshot - * included, refuses with `why`. + * them left. When the whole carried list does not rebuild the reviewed selection, write retries without the oldest carried audits, + * one at a time down to none (a review that ran between two audits already read the earlier one's edits); the refusal names the + * whole list's reason. Content compares as git stores it or as it checks out (autocrlf, filters). Anything else, an unreadable + * snapshot included, refuses with `why`. */ export function writeExcludeBinds(root: string, base: string, head: string, review: { exclude: Exclusion; tagged: ReadonlySet | null }, flagged: string | undefined, docsPost: string | undefined, docsPre?: string): { ok: true; line?: string } | { ok: false; why?: string } { const x = review.exclude; @@ -377,58 +379,67 @@ export function writeExcludeBinds(root: string, base: string, head: string, revi if (!holds(head, p, post.hashes[p] ?? null)) return no(`${p} at HEAD is not the content the snapshot recorded (an edit after the audit)`); } const auditFiles = pre ? changed.filter(p => !(p in pre.hashes) || pre.hashes[p] !== post.hashes[p]) : changed; - const resets = new Map(auditFiles.map(p => [p, { rev: at, want: pre ? pre.hashes[p] : undefined, from: `--docs-pre ${docsPre}` }])); - const left = new Map(); - for (const e of earlier) { - const moved = git(root, ['merge-base', '--is-ancestor', e.head, at]).ok ? since(e.head) : null; - if (!moved) return no(`carried audit ${e.audit_id}'s HEAD ${short(e.head)} is not an ancestor of ${short(at)}, or git could not diff it`); - for (const [p, { before, after }] of Object.entries(e.edited)) { - if (!moved.includes(p)) continue; - const prior = left.get(p); - if (prior && prior.after !== before) return no(`${p} changed between audit ${prior.audit} and audit ${e.audit_id}, outside both`); - if (!prior) resets.set(p, { rev: e.head, want: before, from: `audit ${e.audit_id}'s post snapshot` }); - left.set(p, { after, audit: e.audit_id }); + const bindAudits = (carried: CarriedAudit[]): { ok: true; line: string } | { ok: false; why: string } => { + const resets = new Map(auditFiles.map(p => [p, { rev: at, want: pre ? pre.hashes[p] : undefined, from: `--docs-pre ${docsPre}` }])); + const left = new Map(); + for (const e of carried) { + const moved = git(root, ['merge-base', '--is-ancestor', e.head, at]).ok ? since(e.head) : null; + if (!moved) return no(`carried audit ${e.audit_id}'s HEAD ${short(e.head)} is not an ancestor of ${short(at)}, or git could not diff it`); + for (const [p, { before, after }] of Object.entries(e.edited)) { + if (!moved.includes(p) && !auditFiles.includes(p)) continue; + const prior = left.get(p); + if (prior && prior.after !== before) return no(`${p} changed between audit ${prior.audit} and audit ${e.audit_id}, outside both`); + if (!prior) resets.set(p, { rev: e.head, want: before, from: `audit ${e.audit_id}'s post snapshot` }); + left.set(p, { after, audit: e.audit_id }); + } } - } - for (const [p, { after, audit }] of left) { - const kept = auditFiles.includes(p) ? (p in pre!.hashes ? pre!.hashes[p] === after : holds(at, p, after)) : holds(head, p, after); - if (!kept) return no(`${p} at ${auditFiles.includes(p) ? `this audit's Prepare snapshot` : 'HEAD'} is not the content audit ${audit} recorded (an edit after that audit)`); - } - const files = [...resets.keys()].sort(); - if (!files.length) return no(`--docs-pre ${docsPre}: the audit changed none of ${changed.join(',')}, so it did not change the selection`); - const gitDir = git(root, ['rev-parse', '--absolute-git-dir']).out; - const index = path.join(gitDir, `gstack-receipt-${process.pid}-${randomBytes(4).toString('hex')}.index`); - const env = { ...process.env, GIT_INDEX_FILE: index, GIT_AUTHOR_NAME: 'gstack-ship-receipt', GIT_AUTHOR_EMAIL: 'ship-receipt@gstack.local', GIT_COMMITTER_NAME: 'gstack-ship-receipt', GIT_COMMITTER_EMAIL: 'ship-receipt@gstack.local', GIT_AUTHOR_DATE: '2000-01-01T00:00:00Z', GIT_COMMITTER_DATE: '2000-01-01T00:00:00Z' }; - const run = (args: string[], input?: string) => { const r = spawnSync('git', args, { cwd: root, encoding: 'utf8', env, input, timeout: 60_000 }); return { ok: r.status === 0, out: (r.stdout ?? '').trim() }; }; - const treeEntry = (rev: string, p: string) => run(['ls-tree', '-z', '--full-tree', rev, '--', p]).out.split('\0').find(l => l.endsWith(`\t${p}`)); - let reset: string; - try { - const entries: string[] = []; - for (const [p, { rev, want, from }] of resets) { - const atEntry = treeEntry(rev, p); - if (want === undefined || (atEntry && holds(rev, p, want))) entries.push(atEntry ? `${atEntry}\0` : `0 ${'0'.repeat(rev.length)}\t${p}\0`); - else if (want === null) entries.push(`0 ${'0'.repeat(rev.length)}\t${p}\0`); - else if (typeof want === 'string' && run(['cat-file', '-e', `${want}^{blob}`]).ok) entries.push(`${(treeEntry(head, p) ?? atEntry ?? '100644').split(' ')[0]} blob ${want}\t${p}\0`); - else return no(`${p}'s content before the audit (${short(String(want))} in ${from}) is not in git's object store, so the selection the reviewers read cannot be rebuilt`); + for (const [p, { after, audit }] of left) { + const kept = auditFiles.includes(p) ? (p in pre!.hashes ? pre!.hashes[p] === after : holds(at, p, after)) : holds(head, p, after); + if (!kept) return no(`${p} at ${auditFiles.includes(p) ? `this audit's Prepare snapshot` : 'HEAD'} is not the content audit ${audit} recorded (an edit after that audit)`); } - const tree = run(['read-tree', head]).ok && run(['update-index', '-z', '--index-info'], entries.join('')).ok ? run(['write-tree']) : { ok: false, out: '' }; - const c = tree.ok ? run(['commit-tree', '--no-gpg-sign', tree.out, '-p', head, '-m', 'gstack-ship-receipt: HEAD before the docs audit']) : tree; - if (!c.ok) return no(`git could not build HEAD with ${files.join(',')} reset to before the audit`); - reset = c.out; - } finally { fs.rmSync(index, { force: true }); } - const { ref, sha } = writeBase(root, base); - const before = exclusionAt(root, ref, sha, reset, collectCommit(root, sha ?? ref, reset), review.tagged, 'gstack-ship-receipt'); - if (before.state !== 'active') return no(`the selection with ${files.join(',')} reset to before the audit cannot be read`); - if (before.record.digest !== given) return no(`the reviewed digest ${short(given)} is not the selection HEAD has with the files under review_exclude_paths that changed since the snapshot reset to ${pre ? 'their --docs-pre content' : short(at)}${left.size ? ` and the files audit ${earlier.map(e => e.audit_id).join(',')} edited reset to before that audit` : ''} (${short(before.record.digest)}), so more than the docs audit changed what the reviewers read`); - for (const p of files) for (const [side, s] of [['the review', before], ['HEAD', x]] as const) { - const t = treatment(s, p); - if (!['absent', 'excluded', 'named-by-code'].includes(t)) return no(`${p} is ${t} at ${side}: the docs audit binds only docs the exclusion hides or keeps because code names them`); + const files = [...resets.keys()].sort(); + if (!files.length) return no(`--docs-pre ${docsPre}: the audit changed none of ${changed.join(',')}, so it did not change the selection`); + const gitDir = git(root, ['rev-parse', '--absolute-git-dir']).out; + const index = path.join(gitDir, `gstack-receipt-${process.pid}-${randomBytes(4).toString('hex')}.index`); + const env = { ...process.env, GIT_INDEX_FILE: index, GIT_AUTHOR_NAME: 'gstack-ship-receipt', GIT_AUTHOR_EMAIL: 'ship-receipt@gstack.local', GIT_COMMITTER_NAME: 'gstack-ship-receipt', GIT_COMMITTER_EMAIL: 'ship-receipt@gstack.local', GIT_AUTHOR_DATE: '2000-01-01T00:00:00Z', GIT_COMMITTER_DATE: '2000-01-01T00:00:00Z' }; + const run = (args: string[], input?: string) => { const r = spawnSync('git', args, { cwd: root, encoding: 'utf8', env, input, timeout: 60_000 }); return { ok: r.status === 0, out: (r.stdout ?? '').trim() }; }; + const treeEntry = (rev: string, p: string) => run(['ls-tree', '-z', '--full-tree', rev, '--', p]).out.split('\0').find(l => l.endsWith(`\t${p}`)); + let reset: string; + try { + const entries: string[] = []; + for (const [p, { rev, want, from }] of resets) { + const atEntry = treeEntry(rev, p); + if (want === undefined || (atEntry && holds(rev, p, want))) entries.push(atEntry ? `${atEntry}\0` : `0 ${'0'.repeat(rev.length)}\t${p}\0`); + else if (want === null) entries.push(`0 ${'0'.repeat(rev.length)}\t${p}\0`); + else if (typeof want === 'string' && run(['cat-file', '-e', `${want}^{blob}`]).ok) entries.push(`${(treeEntry(head, p) ?? atEntry ?? '100644').split(' ')[0]} blob ${want}\t${p}\0`); + else return no(`${p}'s content before the audit (${short(String(want))} in ${from}) is not in git's object store, so the selection the reviewers read cannot be rebuilt`); + } + const tree = run(['read-tree', head]).ok && run(['update-index', '-z', '--index-info'], entries.join('')).ok ? run(['write-tree']) : { ok: false, out: '' }; + const c = tree.ok ? run(['commit-tree', '--no-gpg-sign', tree.out, '-p', head, '-m', 'gstack-ship-receipt: HEAD before the docs audit']) : tree; + if (!c.ok) return no(`git could not build HEAD with ${files.join(',')} reset to before the audit`); + reset = c.out; + } finally { fs.rmSync(index, { force: true }); } + const { ref, sha } = writeBase(root, base); + const before = exclusionAt(root, ref, sha, reset, collectCommit(root, sha ?? ref, reset), review.tagged, 'gstack-ship-receipt'); + if (before.state !== 'active') return no(`the selection with ${files.join(',')} reset to before the audit cannot be read`); + if (before.record.digest !== given) return no(`the reviewed digest ${short(given)} is not the selection HEAD has with the files under review_exclude_paths that changed since the snapshot reset to ${pre ? 'their --docs-pre content' : short(at)}${left.size ? ` and the files audit ${carried.map(e => e.audit_id).join(',')} edited reset to before that audit` : ''} (${short(before.record.digest)}), so more than the docs audit changed what the reviewers read`); + for (const p of files) for (const [side, s] of [['the review', before], ['HEAD', x]] as const) { + const t = treatment(s, p); + if (!['absent', 'excluded', 'named-by-code'].includes(t)) return no(`${p} is ${t} at ${side}: the docs audit binds only docs the exclusion hides or keeps because code names them`); + } + const audit = new Set(files); + for (const p of new Set([...before.excluded, ...x.excluded, ...before.kept.map(k => k.path), ...x.kept.map(k => k.path)])) { + if (!audit.has(p) && treatment(before, p) !== treatment(x, p)) return no(`${p} is ${treatment(before, p)} at the review and ${treatment(x, p)} at HEAD`); + } + return { ok: true, line: `REVIEW_EXCLUDE_DOCS_AUDIT: reviewed=${short(given)} head=${short(x.record.digest)} snapshot=${short(at)} files=${files.join(',')}${carried.length ? ` audits=${[...carried.map(e => e.audit_id), post.audit_id].join(',')}` : ''} (the selection changed only by this /ship run's Step 14.5 docs audits' files under review_exclude_paths; the receipt records HEAD's)` }; + }; + let first: { ok: false; why: string } | undefined; + for (let k = 0; k <= earlier.length; k++) { + const r = bindAudits(earlier.slice(k)); + if (r.ok) return r; + first ??= r; } - const audit = new Set(files); - for (const p of new Set([...before.excluded, ...x.excluded, ...before.kept.map(k => k.path), ...x.kept.map(k => k.path)])) { - if (!audit.has(p) && treatment(before, p) !== treatment(x, p)) return no(`${p} is ${treatment(before, p)} at the review and ${treatment(x, p)} at HEAD`); - } - return { ok: true, line: `REVIEW_EXCLUDE_DOCS_AUDIT: reviewed=${short(given)} head=${short(x.record.digest)} snapshot=${short(at)} files=${files.join(',')}${earlier.length ? ` audits=${[...earlier.map(e => e.audit_id), post.audit_id].join(',')}` : ''} (the selection changed only by this /ship run's Step 14.5 docs audits' files under review_exclude_paths; the receipt records HEAD's)` }; + return first!; } /** The queue's refusal when carryExcludeBinds does not bind the receipt at `head`; null when it does. */ export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base: string): string | null { diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index a0d63d9a3..b1c1b639f 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -736,25 +736,28 @@ describe('the receipt binds the selection the reviewers read', () => { describe('two docs audits in one /ship run (GST-1616)', () => { const RUN = '/records/ship-8678.md'; const BRANCH = { 'lib/reader.js': "exports.p = 'docs/parity.md';\n", 'docs/parity.md': '# parity\n' }; - /** The #8678 shape: reviewed; audit a1 edits docs/guide.md (excluded, untouched by the branch) and is committed; main moves and is merged; the release is restamped with `between`; audit a3 adds docs/new.md, its post snapshot carries a1's unless `carry` is false, and it is committed. */ - const twoAudits = (opts: { between?: Record; carry?: boolean } = {}) => { + /** The #8678 shape: reviewed; audit a1 edits docs/guide.md (excluded, untouched by the branch) and is committed; main moves and is merged; the release is restamped with `between`; audit a3 adds docs/new.md, its post snapshot carries a1's unless `carry` is false, and it is committed. `a1`/`a3` replace the audits' edits (null deletes) and `rereview` re-takes the reviewed digest after the restamp. */ + const twoAudits = (opts: { between?: Record; carry?: boolean; a1?: Record; a3?: Record; rereview?: boolean } = {}) => { const repo = makeFixtureRepo({ prefix: 'gstack-exclude-audits-', baseFiles: { '.gstack/ship-policy.json': policyText(['docs/**', 'test/fixtures/goldens/**']), 'docs/guide.md': '# guide\n', 'lib/index.js': "exports.k = 'docs/key-files.md';\n" } }); repos.push(repo); commit(repo, BRANCH); - const reviewed = JSON.parse(plan(repo, ['--json']).out).review_exclude.digest as string; + const digest = () => JSON.parse(plan(repo, ['--json']).out).review_exclude.digest as string; + let reviewed = digest(); + const apply = (files: Record) => { for (const [p, c] of Object.entries(files)) if (c === null) fs.rmSync(path.join(repo.work, p)); else write(repo.work, p, c); }; const snapshot = (id: string, out: string, extra: string[] = [], run = RUN) => { const file = path.join(repo.root, out); return { file, ...runBin(repo, 'gstack-docs-candidate', ['snapshot', '--out', file, '--audit-id', id, '--mode', 'edit', '--base', repo.git('rev-parse', 'origin/main'), '--docs', 'docs', '--run', run, ...extra]) }; }; const saved = (s: ReturnType) => { expect({ status: s.status, err: s.err }).toMatchObject({ status: 0 }); return s.file; }; const pre1 = saved(snapshot('a1', 'a1.json')); - write(repo.work, 'docs/guide.md', '# guide\n\nUpdated by the docs audit.\n'); + apply(opts.a1 ?? { 'docs/guide.md': '# guide\n\nUpdated by the docs audit.\n' }); const post1 = saved(snapshot('a1', 'a1-post.json', ['--pre', pre1])); commit(repo, {}, 'docs audit a1'); onBase(repo, { 'notes/base.md': '# base\n' }); commit(repo, { VERSION: '1.0.1\n', ...opts.between }, 'restamp'); + if (opts.rereview) reviewed = digest(); const pre = saved(snapshot('a3', 'a3.json')); - write(repo.work, 'docs/new.md', '# new\n'); + apply(opts.a3 ?? { 'docs/new.md': '# new\n' }); const post = saved(snapshot('a3', 'a3-post.json', ['--pre', pre, ...(opts.carry === false ? [] : ['--carry', post1])])); commit(repo, {}, 'docs audit a3'); return { repo, reviewed, pre, post, post1, snapshot, body: path.join(repo.root, 'body.md') }; @@ -798,6 +801,53 @@ describe('the receipt binds the selection the reviewers read', () => { fs.writeFileSync(forged, JSON.stringify(post)); refusedWith(writeWith(a, forged), a, 'audit a1 of another /ship run'); }, 240_000); + + // Value: protects=write refuses a carried list it cannot trust: no --docs-pre, a --docs-pre of another run, a malformed, duplicated + // or run-less earlier, a carried HEAD off the snapshot's history, or a file changed between two audits outside both; + // fails_when=a guard in writeExcludeBinds or carriedAudits is dropped and write binds the reviewed digest anyway; + // why_new=only the success path, stray/later edits and another run's entry were covered; seam=none; no_claim=snapshot-side refusals + test('write refuses a carried audit list it cannot trust', () => { + const a = twoAudits(); + refusedWith(writeReceipt(a.repo, a.body, ['--review-exclude', a.reviewed, '--docs-post', a.post, ...targetedAtHead(a.repo)]), a, 'carries audit a1, so write needs --docs-pre'); + const forge = (name: string, edit: (j: any) => void) => { const j = JSON.parse(fs.readFileSync(name === 'pre' ? a.pre : a.post, 'utf8')); edit(j); const f = path.join(a.repo.root, `forged-${name}-${Math.random().toString(36).slice(2)}.json`); fs.writeFileSync(f, JSON.stringify(j)); return f; }; + const otherPre = forge('pre', j => { j.run_id = '/records/another-run.md'; }); + refusedWith(writeReceipt(a.repo, a.body, ['--review-exclude', a.reviewed, '--docs-post', a.post, '--docs-pre', otherPre, ...targetedAtHead(a.repo)]), a, 'its HEAD, audit_id or run_id differs'); + const orphan = a.repo.git('commit-tree', '-m', 'orphan', a.repo.git('hash-object', '-t', 'tree', '-w', '/dev/null')); + const rows: Array<[(j: any) => void, string]> = [ + [j => { j.earlier = j.earlier[0]; }, 'its earlier field is not a list of audits'], + [j => { j.earlier[0].edited = []; }, 'is not { run_id, audit_id, head, edited }'], + [j => { j.earlier.push(j.earlier[0]); }, 'carries audit a1 twice'], + [j => { j.earlier[0].head = orphan; }, `carried audit a1's HEAD ${orphan.slice(0, 12)} is not an ancestor`], + [j => { j.earlier.push({ ...j.earlier[0], audit_id: 'a2', edited: { 'docs/guide.md': { before: null, after: null } } }); }, 'docs/guide.md changed between audit a1 and audit a2, outside both'], + ]; + for (const [edit, why] of rows) refusedWith(writeWith(a, forge('post', edit)), a, why); + const runless = (j: any) => { delete j.run_id; }; + refusedWith(writeReceipt(a.repo, a.body, ['--review-exclude', a.reviewed, '--docs-post', forge('post', runless), '--docs-pre', forge('pre', runless), ...targetedAtHead(a.repo)]), a, 'names no run_id'); + }, 240_000); + + // Value: protects=write binds an honest re-audit that undid, or re-edited, the carried audit's file, and one whose review ran + // after the carried audit; fails_when=the moved skip drops a file the re-audit reverted, the reset target or the Prepare-side + // check for a twice-edited file regresses, or write insists on the carried audit when the review already read it; + // why_new=the other GST-1616 rows edit disjoint files and review before both audits; seam=none; no_claim=a live /ship run + test('write binds a re-audit that reverts or re-edits the carried audit\'s file, and a review between the two audits', () => { + const binds = (a: ReturnType, files: string, audits: string) => { + const w = writeWith(a); + expect({ status: w.status, err: w.status === EXIT.ok ? '' : w.err }).toEqual({ status: EXIT.ok, err: '' }); + expect(w.err + w.out).toContain(`files=${files}${audits} (`); + }; + binds(twoAudits({ a1: { 'docs/x.md': '# x\n' }, a3: { 'docs/x.md': null, 'docs/new.md': '# new\n' } }), 'docs/new.md,docs/x.md', ' audits=a1,a3'); + binds(twoAudits({ a1: { 'docs/guide.md': null }, a3: { 'docs/guide.md': '# guide\n', 'docs/new.md': '# new\n' } }), 'docs/guide.md,docs/new.md', ' audits=a1,a3'); + const again = twoAudits({ a3: { 'docs/guide.md': '# guide\n\nEdited again by the re-audit.\n', 'docs/new.md': '# new\n' } }); + binds(again, 'docs/guide.md,docs/new.md', ' audits=a1,a3'); + const between = path.join(again.repo.root, 'between.md'); + fs.writeFileSync(between, '# guide\n\nEdited between the audits.\n'); + const pre = JSON.parse(fs.readFileSync(again.pre, 'utf8')); + pre.hashes['docs/guide.md'] = again.repo.git('hash-object', '-w', between); + fs.writeFileSync(again.pre, JSON.stringify(pre)); + fs.rmSync(again.body, { force: true }); + refusedWith(writeWith(again), again, "docs/guide.md at this audit's Prepare snapshot is not the content audit a1 recorded"); + binds(twoAudits({ a1: { 'docs/x.md': '# x\n' }, rereview: true }), 'docs/new.md', ''); + }, 480_000); }); });