From 5297ddf42db6a7f87866eb1f6d7300fd5e7cd71c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 5 Nov 2026 00:33:21 -0800 Subject: [PATCH] fix(ship-receipt): carry survives a base policy change that selects no PR file (GST-1415) (#4030) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 7 +- .../capy-gst-1415-carry-exclude-selection.md | 1 + docs/ship-policy.md | 3 +- lib/review-exclude.ts | 4 + lib/ship-receipt-depth.ts | 16 +++- test/review-exclude.test.ts | 73 +++++++++++++++++++ 6 files changed, 98 insertions(+), 6 deletions(-) create mode 100644 changelog.d/capy-gst-1415-carry-exclude-selection.md diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 86b5e96ca..fc5b4dd36 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -51,7 +51,7 @@ import { import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; import { comparePath, contextPacket, PACKET_SECTION_CAP, readScopedReview, touchedPaths } from '../lib/ship-patch'; import { planRecheck, readRecheck, recheckPacket } from '../lib/ship-recheck'; -import { deeper, depthBelow, exceptionHead, exceptionLoopProblem, exceptionProblems, parseReviewLoopException, parseReviewTargeted, plainDocsSince, platformReceiptDepth, receiptExclude, receiptReviewAtHead, receiptTargetedAtHead, reviewLoopExceptionLine, targetedClassesSince } from '../lib/ship-receipt-depth'; +import { carryExcludeBinds, deeper, depthBelow, exceptionHead, exceptionLoopProblem, exceptionProblems, parseReviewLoopException, parseReviewTargeted, plainDocsSince, platformReceiptDepth, receiptExclude, receiptReviewAtHead, receiptTargetedAtHead, reviewLoopExceptionLine, targetedClassesSince } from '../lib/ship-receipt-depth'; import { RISK_CLASSES, type RiskClass } from '../lib/review-risk'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; @@ -104,7 +104,8 @@ 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; 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; carry also + carries when neither the recorded selection nor HEAD's holds a file the PR changed. 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. --no-stop (a JSON array of {class, reason, evidence}: each routine stop the run recorded and went on past, class one of @@ -870,7 +871,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 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 (!carryExcludeBinds(review.exclude, receipt)) 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 and one of the two selections holds a file the PR changed (carry compares those, not the review ledger), so carry does not apply; run /ship on ${head.slice(0, 12)}`); const riskOf = (d: { class: string; risk?: readonly string[] }) => `${d.class}:${[...(d.risk ?? [])].sort().join(',')}`; if (recheck && riskOf(now) !== riskOf(receipt.review_depth)) return refused('RECEIPT_CARRY_REFUSED', `HEAD ${head.slice(0, 12)} is risk class ${riskOf(now)}, not the reviewed ${riskOf(receipt.review_depth)}, so a scoped re-check cannot stand in for review; 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)}`); diff --git a/changelog.d/capy-gst-1415-carry-exclude-selection.md b/changelog.d/capy-gst-1415-carry-exclude-selection.md new file mode 100644 index 000000000..73a33c6e5 --- /dev/null +++ b/changelog.d/capy-gst-1415-carry-exclude-selection.md @@ -0,0 +1 @@ +- **Receipt carry survives a base policy change that excludes none of the PR's files.** `gstack-ship-receipt carry` refused with `review_exclude changed` whenever main's `review_exclude_paths` digest moved, even when no file the PR changed fell under any pattern, so harness-evals #263 paid a full five-reader re-review of an unchanged product. Carry now also carries when neither the receipt's recorded selection nor HEAD's holds a file the PR changed (a pattern gained, added, removed or reverted), and the carried receipt records HEAD's digest so the queue binds it. A pattern change that selects a PR file, excluded or kept, and an unreadable or malformed base policy still refuse. (GST-1415) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 0f018fc77..de2584b46 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1474,7 +1474,8 @@ 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. -The queue makes one exception (GST-1408): a receipt with no `review_exclude`, written before the base gained the key, still binds when no file the head changes matches any current pattern, excluded or kept, because its reviewers read the same full diff; a recorded digest that differs, a matched file and an unreadable base policy still refuse, and carry still compares digests. +The queue makes one exception (GST-1408): a receipt with no `review_exclude`, written before the base gained the key, still binds when no file the head changes matches any current pattern, excluded or kept, because its reviewers read the same full diff; a recorded digest that differs, a matched file and an unreadable base policy still refuse. +Carry makes the same exception in both directions (GST-1415): when the digests differ but neither the receipt's recorded selection (no `review_exclude`, or one whose list hash is the empty selection's and whose digest is built from it) nor HEAD's holds a file the PR changed, the receipt carries and records HEAD's digest, so a pattern the base gains, adds, removes or reverts does not void it; a selection that holds a PR file, excluded or kept, and an unreadable base policy still refuse. **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/lib/review-exclude.ts b/lib/review-exclude.ts index 4a6fc00bf..c43e80927 100644 --- a/lib/review-exclude.ts +++ b/lib/review-exclude.ts @@ -72,6 +72,10 @@ export interface ExcludeInput { } const sha256 = (s: string) => createHash('sha256').update(s).digest('hex'); +/** The list_sha256 of a selection that holds no file: no pattern matched the range, so applied, excluded and kept are all empty. */ +const EMPTY_LIST_SHA256 = sha256(JSON.stringify({ applied: [], excluded: [], kept: [] })); +/** A recorded selection holds no file only when its list hash is the empty list's and its digest is built from that hash. */ +export const recordSelectsNoFile = (r: Pick) => r.list_sha256 === EMPTY_LIST_SHA256 && r.digest === sha256(`${r.patterns_sha256}\n${EMPTY_LIST_SHA256}`); const shellQuote = (s: string) => `'${s.replace(/'/g, "'\\''")}'`; const literal = (p: string) => `:(top,literal)${p}`; const argBytes = (paths: Iterable) => { let n = 0; for (const p of paths) n += Buffer.byteLength(shellQuote(literal(p))) + 1; return n; }; diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index e60a776bf..50645976f 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -22,7 +22,7 @@ import * as path from 'node:path'; import { baseContainsHead } from './default-base'; import type { ShipReceipt } from './headless-artifacts'; import { collectCommit } from './review-collect'; -import { reviewExclude, type Exclusion } from './review-exclude'; +import { recordSelectsNoFile, reviewExclude, type Exclusion } from './review-exclude'; import { isUnavailable } from './ship-policy'; import { loadPolicyAt } from './ship-policy-at'; import { classifyPath, depthOfCommitInput, gateScanNeeded, platformFits, scanNumbers, type Depth } from './review-depth'; @@ -285,11 +285,23 @@ function headInputs(root: string, baseRef: string, head: string, r?: ShipReceipt * or kept (GST-1408): the selection then equals the full diff that receipt's review read, so a pattern the base gained later * changes nothing the reviewers saw. */ +/** No file the head changed matches any pattern the base policy declares, so each reviewer reads the full diff (GST-1408). */ +const selectsNoFile = (x: Exclusion) => x.state === 'none' || x.state === 'head-only' || (x.state === 'active' && x.record.unmatched.length === x.record.patterns.length); +/** + * Carry's exclusion check: the recorded digest against the one HEAD has under the current base policy, except that a policy + * change alone does not void the receipt (GST-1415): when neither the receipt's selection nor HEAD's holds a file the PR + * changed, each reviewer read the full diff and still would, whatever the patterns. A policy that cannot be read never binds. + */ +export function carryExcludeBinds(x: Exclusion, r: ShipReceipt): boolean { + if (x.state === 'refused') return false; + const e = r.review_exclude; + return (x.state === 'active' ? x.record.digest : 'none') === (e?.digest ?? 'none') || (selectsNoFile(x) && (!e || recordSelectsNoFile(e))); +} export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base: string): string | null { const expected = x.state === 'active' ? x.record.digest : 'none'; const recorded = r.review_exclude?.digest ?? 'none'; if (expected === recorded) return null; - if (recorded === 'none' && x.state === 'active' && x.record.unmatched.length === x.record.patterns.length) return null; + if (recorded === 'none' && selectsNoFile(x)) 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 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)`; diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index d87523d91..34a363d8a 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -420,6 +420,79 @@ describe('the receipt binds the selection the reviewers read', () => { expect(b.code).toBe('POLICY_INVALID'); }, 240_000); }); + + // Value: protects=a base policy change that selects no file the PR changed (a pattern gained, added beside another, removed or + // reverted) no longer voids the receipt at carry, and the queue binds the carried receipt, while a changed pattern that selects + // a PR file (excluded or kept) and an unreadable or malformed base policy still refuse; fails_when=carry compares the policy + // digest alone (GST-1415: harness-evals #263 re-ran a full five-reader review of an unchanged product), or the acceptance + // spreads to a selection that holds a PR file; why_new=GST-1415; seam=gstack-ship-receipt carry after a base merge, then + // bindShippedReceipt on the carried receipt; no_claim=the queue's own comparison of uncarried receipts (GST-1408 above) + describe('carry after a base policy change (GST-1415)', () => { + const CODE = { 'lib/reader.js': 'exports.p = 1;\n', 'docs/guide.md': '# guide\n' }; + const NO_KEY = JSON.stringify({}) + '\n'; + const shipped = (files: Record, basePolicy: string[] | null) => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-carry-', baseFiles: basePolicy ? { '.gstack/ship-policy.json': policyText(basePolicy) } : {} }); + repos.push(repo); + commit(repo, files); + const body = path.join(repo.root, 'body.md'); + const extra = [...targetedAtHead(repo)]; + if (basePolicy) { + const expected = writeReceipt(repo, body, extra).err.match(/field=review_exclude expected=([0-9a-f]{64}) /)?.[1]; + expect(expected).toBeDefined(); + extra.push('--review-exclude', expected!); + } + const w = writeReceipt(repo, body, extra); + expect({ status: w.status, err: w.err }).toMatchObject({ status: EXIT.ok }); + return { repo, body, recorded: receiptIn(body).review_exclude?.digest ?? 'none' }; + }; + const carry = (repo: FixtureRepo, body: string) => runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + + test('a pattern change that selects no PR file carries, and the queue binds the carried receipt', () => { + const cases: Array<{ name: string; before: string[] | null; after: string }> = [ + { name: 'gained', before: null, after: policyText(['receipts/**']) }, + { name: 'added beside another', before: ['receipts/**'], after: policyText(['receipts/**', 'results/**']) }, + { name: 'removed beside another', before: ['receipts/**', 'results/**'], after: policyText(['receipts/**']) }, + { name: 'reverted', before: ['receipts/**'], after: NO_KEY }, + ]; + for (const c of cases) { + const { repo, body, recorded } = shipped(CODE, c.before); + onBase(repo, { '.gstack/ship-policy.json': c.after }); + const carried = carry(repo, body); + expect({ name: c.name, status: carried.status, err: carried.status === EXIT.ok ? '' : carried.err }).toEqual({ name: c.name, status: EXIT.ok, err: '' }); + const head = repo.git('rev-parse', 'HEAD'); + const now = receiptIn(body); + expect({ name: c.name, head: now.head, moved: (now.review_exclude?.digest ?? 'none') !== recorded }).toEqual({ name: c.name, head, moved: true }); + const b = bindShippedReceipt(repo.work, 'origin/main', fs.readFileSync(body, 'utf8'), undefined, [head]); + expect({ name: c.name, ok: b.ok, message: b.ok ? '' : b.message }).toEqual({ name: c.name, ok: true, message: '' }); + } + }, 240_000); + + test('a pattern change that selects a PR file, excluded or kept, still refuses carry', () => { + const touches: Array> = [{ 'receipts/a/result.json': '{"ok": true}\n' }, { 'receipts/run.js': 'exports.run = 1;\n' }]; + const changes: Array<[string[] | null, string]> = [[null, policyText(['receipts/**'])], [['receipts/**'], NO_KEY]]; + for (const touched of touches) { + for (const [before, after] of changes) { + const { repo, body, recorded } = shipped({ ...CODE, ...touched }, before); + onBase(repo, { '.gstack/ship-policy.json': after }); + const refused = carry(repo, body); + expect({ touched: Object.keys(touched)[0], before, status: refused.status }).toEqual({ touched: Object.keys(touched)[0], before, status: EXIT.refused }); + expect(refused.err).toContain(`review_exclude changed: recorded=${recorded} `); + expect(refused.err).toContain('(RECEIPT_CARRY_REFUSED;'); + } + } + }, 240_000); + + test('an unreadable or malformed base policy still refuses carry', () => { + for (const text of [JSON.stringify({ review_exclude_paths: 'receipts/**', min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n', '{ not json\n']) { + const { repo, body } = shipped(CODE, null); + onBase(repo, { '.gstack/ship-policy.json': text }); + const refused = carry(repo, body); + expect({ text, status: refused.status }).toEqual({ text, status: EXIT.refused }); + expect(refused.err).toContain('RECEIPT_CARRY_REFUSED'); + expect(refused.err).toContain('so its review_exclude_paths cannot be read'); + } + }, 240_000); + }); }); describe('a branch cannot hide code under a base pattern', () => {