From b9c5fa82a56e1cc15b15d68a5f147cd92da00d0c Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 6 Nov 2026 14:19:09 +0000 Subject: [PATCH] fix(ship-receipt): a carried-audit retry never drops an integrity refusal (GST-1616) 918ab93 retried writeExcludeBinds with shorter carried-audit lists on any failure, so an unreviewed edit to a file a carried audit edited (one code names, so the selection digest did not move) bound once that audit was dropped (outside voice on #4071). Only the reconstruction digest mismatch retries now; an edit after an audit, a file changed between audits and a foreign HEAD end the check. The repro is a test that must refuse. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ship-receipt-depth.ts | 10 ++++++---- test/review-exclude.test.ts | 7 +++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 29fb0861a..dc7c32e31 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -331,8 +331,9 @@ const treatment = (x: ActiveExclusion, p: string) => (x.excluded.has(p) ? 'exclu * (`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. 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 + * one at a time down to none (a review that ran between two audits already read the earlier one's edits); only that digest + * mismatch retries, so an integrity refusal (an edit after an audit, a file changed between audits, a foreign HEAD) ends it, and + * 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 } { @@ -379,7 +380,7 @@ 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 bindAudits = (carried: CarriedAudit[]): { ok: true; line: string } | { ok: false; why: string } => { + const bindAudits = (carried: CarriedAudit[]): { ok: true; line: string } | { ok: false; why: string; retry?: true } => { 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) { @@ -422,7 +423,7 @@ export function writeExcludeBinds(root: string, base: string, head: string, revi 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`); + if (before.record.digest !== given) return { retry: true, ...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`); @@ -438,6 +439,7 @@ export function writeExcludeBinds(root: string, base: string, head: string, revi const r = bindAudits(earlier.slice(k)); if (r.ok) return r; first ??= r; + if (!r.retry) return first; } return first!; } diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index b1c1b639f..ccf92c229 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -730,7 +730,8 @@ describe('the receipt binds the selection the reviewers read', () => { // (GST-1616: gbrain #8678 refused RECEIPT_INVALID field=review_exclude because the first audit's committed file stayed in the // selection, and the only way out was HEAD's digest, which skips review of the delta), while a non-audit edit to an excluded // file and another run's audit list still refuse; fails_when=write resets only the latest audit's files, takes a carried list - // from another run, or resets an earlier audit's file that something else edited later; why_new=GST-1616; + // from another run, resets an earlier audit's file that something else edited later, or drops that audit on a retry so an + // unreviewed edit to a file code names binds (the #4071 outside voice); why_new=GST-1616; // seam=gstack-docs-candidate snapshot --run --pre --carry, then gstack-ship-receipt write --review-exclude --docs-post --docs-pre; // no_claim=how many audits one run may launch (ship/sections/documentation.md) describe('two docs audits in one /ship run (GST-1616)', () => { @@ -786,7 +787,9 @@ describe('the receipt binds the selection the reviewers read', () => { refusedWith(writeWith(stray), stray, 'is not the selection HEAD has with the files under review_exclude_paths'); const later = twoAudits({ between: { 'docs/guide.md': '# guide\n\nEdited after the docs audit.\n' } }); refusedWith(writeWith(later), later, 'docs/guide.md at HEAD is not the content audit a1 recorded'); - }, 240_000); + const kept = twoAudits({ a1: { 'docs/parity.md': '# parity\naudited\n' }, between: { 'docs/parity.md': '# parity\nan unreviewed edit\n' } }); + refusedWith(writeWith(kept), kept, 'docs/parity.md at HEAD is not the content audit a1 recorded'); + }, 360_000); test('another run\'s audit list is refused, at the snapshot and at write', () => { const a = twoAudits();