diff --git a/bin/gstack-doctor-components.sh b/bin/gstack-doctor-components.sh index 9c6db1111..8cace00ad 100644 --- a/bin/gstack-doctor-components.sh +++ b/bin/gstack-doctor-components.sh @@ -303,6 +303,7 @@ gstack_result_anchor() { QUEUE_RECEIPT_MISSING) printf '%s' 'queue-receipt-missing' ;; QUEUE_RECEIPT_STALE) printf '%s' 'queue-receipt-stale' ;; QUEUE_RECEIPT_CONDITIONAL) printf '%s' 'queue-receipt-conditional' ;; + QUEUE_RECEIPT_BASE_MISMATCH) printf '%s' 'queue-receipt-base-mismatch' ;; BASE_RED_UNCONFIGURED) printf '%s' 'base-red-unconfigured' ;; BASE_RED_UNNAMED) printf '%s' 'base-red-unnamed' ;; BASE_RED_REPRO_ERROR) printf '%s' 'base-red-repro-error' ;; diff --git a/changelog.d/capy-receipt-base-matches-queue-base.md b/changelog.d/capy-receipt-base-matches-queue-base.md new file mode 100644 index 000000000..8d1d8541b --- /dev/null +++ b/changelog.d/capy-receipt-base-matches-queue-base.md @@ -0,0 +1 @@ +- **The merge queue no longer lands a stacked child's parent commits under the child's receipt (GST-2153).** A child PR shipped against its parent branch (#3570) has a receipt that covers only the child's own change. If that child was retargeted to main while the parent was still open, `gstack-ship-queue ready`, `land --pin` and train build used to accept the receipt, so the parent's commits could land without their own review, READY or merged checks. They now refuse it with `QUEUE_RECEIPT_BASE_MISMATCH` until the parent lands, the base is merged into the child, and `gstack-ship-receipt carry --base ` moves the receipt to the base. Receipts written against the base, including stacked children reviewed against main (GST-1199), are unaffected. diff --git a/crew/test/ship-queue-train.test.ts b/crew/test/ship-queue-train.test.ts index 2aaf4a6bc..b58170ee1 100644 --- a/crew/test/ship-queue-train.test.ts +++ b/crew/test/ship-queue-train.test.ts @@ -567,6 +567,8 @@ describe('train build', () => { expect(queue(fx, ['train', 'build', '--prs', `1@${h1},2@${h2}`, '--dry-run']).err).toMatch(/PR #2's ship receipt is invalid \(.*\).*\(QUEUE_RECEIPT_MISSING\)/); setPrBody(fx, '2', shippedBody('1', h2)); expect(queue(fx, ['train', 'build', '--prs', `1@${h1},2@${h2}`, '--dry-run']).err).toMatch(/PR #2's ship receipt names pr=1, not this PR.*\(QUEUE_RECEIPT_STALE\)/); + setPrBody(fx, '2', shippedBody('2', h2, { base: 'pr-1' })); + expect(queue(fx, ['train', 'build', '--prs', `1@${h1},2@${h2}`, '--dry-run']).err).toMatch(/PR #2's ship receipt was reviewed against pr-1, not main.*\(QUEUE_RECEIPT_BASE_MISMATCH\)/); expect(remoteRefs(fx)).toBe(refsBefore); expect(dispatches(fx)).toEqual([]); expect(fs.existsSync(path.join(fx.state, 'ship-queue', 'trains'))).toBe(false); diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 002d3a6ea..dbb3ceeb2 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1985,6 +1985,11 @@ and from then on names the base it was carried to. A parent that has not landed, since, or a parent that landed with changes the child never merged all leave a different diff and refuse with `RECEIPT_CARRY_PRODUCT_CHANGED`. An older receipt whose parent branch is gone refuses with `RECEIPT_CARRY_REFUSED`. +Until the carry moves it, `ready`, `land --pin` and train build refuse a +receipt whose `base` is not the queue's base with +`QUEUE_RECEIPT_BASE_MISMATCH` (GST-2153): a child retargeted to the base while +its parent is still open would otherwise land the parent's commits under a +receipt that reviewed only the child. **Stacked PRs reviewed against the base (GST-1199).** A child PR that targets the base while stacked has a receipt whose `base` is the base and whose @@ -2378,8 +2383,9 @@ restamping`, GST-1245), so a reset stamp needs no hand cleanup: without a READY receipt (`gstack-ship-queue ready --pr ` records one on `refs/notes/gstack-trains` once its pregate, full_e2e and receipt checks pass; land checks nothing weaker), a /ship receipt that is missing or - bound to another head (`QUEUE_RECEIPT_MISSING`, `QUEUE_RECEIPT_STALE`), and - required checks that are not green at the pin. + bound to another head (`QUEUE_RECEIPT_MISSING`, `QUEUE_RECEIPT_STALE`) or + reviewed against another branch than the base + (`QUEUE_RECEIPT_BASE_MISMATCH`), and required checks that are not green at the pin. 2. **Lock.** Take the repository land lock, `refs/notes/gstack-land` (its own ref, never the trains notes ref), by exact lease. It records the PR, holder, phase and journal. A held lock is `QUEUE_LAND_LOCKED` naming the holder; @@ -2453,6 +2459,7 @@ release-file rows): | the PR is open, on the base, from this repository | `QUEUE_NOT_READY`, `QUEUE_TRAIN_UNTRUSTED` | | the PR head is the pin | [`QUEUE_LAND_PIN_MOVED`](troubleshooting.md#queue-land-pin-moved) | | the PR body's /ship receipt is bound to the pin | `QUEUE_RECEIPT_MISSING`, `QUEUE_RECEIPT_STALE` | +| the receipt's `base` is the queue's base | [`QUEUE_RECEIPT_BASE_MISMATCH`](troubleshooting.md#queue-receipt-base-mismatch) | | a READY note from `ready` exists at the pin | `QUEUE_NOT_READY` | | no freeze is active (with `--repair`: one is, and its repair slot is free or already this pin; a backstop freeze left from before `release_gate` only warns; see [direct freeze](#direct-freeze)) and no land lock is held | [`QUEUE_TRAIN_FROZEN`](troubleshooting.md#queue-train-frozen), `QUEUE_NOT_READY`, `QUEUE_LAND_LOCKED` | | VERSION, the `package.json` version, the CHANGELOG and `.gstack/version-path` are unchanged since the merge base | [`QUEUE_LAND_STAMPED`](troubleshooting.md#queue-land-stamped) | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 78d465972..f466da9b8 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -6238,6 +6238,26 @@ READYs, trains, carries or merges on it, except on a train with `gates`, where `), then run `/ship --resume `, which reruns Step 5 and rewrites the receipt without `base_red`. + +### `QUEUE_RECEIPT_BASE_MISMATCH` + +**Meaning.** The PR body's ship receipt was reviewed against another branch +than the queue's base: its `base` names a parent branch, as a +[stacked child's receipt](ship-policy.md) does (#3570), while the PR now +targets the base. That receipt covers only the change since the parent, so +the parent's commits the PR carries were never reviewed under it. `ready`, +`land --pin` and train build refuse it while the parent is open, instead of +landing the parent's commits past the parent's own receipt and READY. +A receipt whose `base` is the queue's base (`main` or `origin/main`), +including a stacked child reviewed against the base (GST-1199), is +unaffected. + +**Fix.** Land the parent first, merge the base into this branch and push. +Then run `gstack-ship-receipt carry --pr --base `: it moves the receipt to the base when the parent fully landed +and the child is unchanged, and from then on the receipt names the base. If +the carry refuses, rerun /ship against the base. Then rerun +`gstack-ship-queue ready --pr `. + ### `BASE_RED_UNCONFIGURED` diff --git a/lib/result-codes-release.ts b/lib/result-codes-release.ts index 8dc6bffbf..e5655583f 100644 --- a/lib/result-codes-release.ts +++ b/lib/result-codes-release.ts @@ -244,6 +244,11 @@ export const RELEASE_RESULT_CODES = { summary: 'the PR body’s /ship receipt is conditional: Step 5 failed on cases gstack-base-red proved are the base branch’s own, so /ship paused before READY', fix: 'wait for the base to pass those cases (gstack-base-red wait --pause ), then /ship --resume , which re-runs Step 5 and rewrites the receipt without base_red; only ready --base-red --fixer READYs a conditional receipt, and it rides only a train with gates whose slot 1 is its fixer', }, + QUEUE_RECEIPT_BASE_MISMATCH: { + anchor: 'queue-receipt-base-mismatch', + summary: 'the PR body’s /ship receipt was reviewed against another branch than the queue’s base (a #3570 stacked child retargeted before its parent landed), so it covers only the change since that branch', + fix: 'land the parent first, merge the base into this branch and push, then gstack-ship-receipt carry --pr --base moves the receipt to the base (it carries only when the parent fully landed and the child is unchanged), else rerun /ship against the base; then gstack-ship-queue ready again', + }, BASE_RED_UNCONFIGURED: { anchor: 'base-red-unconfigured', summary: 'the base branch’s ship policy declares no base_red block, so gstack-base-red cannot reproduce a Step 5 failure there and every failure stays in-branch', diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 9a54f3e03..4772e54aa 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -504,6 +504,8 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, if (!b.ok) return b; const self = merged ? null : baseContainsHead(root, baseRef, b.head); if (self) return { ok: false, code: 'BASE_CONTAINS_HEAD', message: self, line: b.line }; + const based = merged ? b : bindReceipt(body, pr, accepted, { base: baseRef }); + if (!based.ok) return based; const ancestor = (commit: string) => b.receipt.carry?.ancestry === 'rewritten' || git(root, ['merge-base', '--is-ancestor', commit, b.head]).ok; const problems = exceptionProblems(body, b.receipt, ancestor); if (problems.length) return { ok: false, code: 'QUEUE_RECEIPT_EXCEPTION_INVALID', message: `the receipt's owner exception does not stand: ${problems.join('; ')}`, line: b.line }; diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index 04cb4cc4a..8d80c357b 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -71,7 +71,7 @@ export function extractReceipt(body: string): { found: boolean; receipt?: ShipRe } } -export type ReceiptBindingCode = 'QUEUE_RECEIPT_MISSING' | 'QUEUE_RECEIPT_STALE' | 'QUEUE_RECEIPT_CONDITIONAL'; +export type ReceiptBindingCode = 'QUEUE_RECEIPT_MISSING' | 'QUEUE_RECEIPT_STALE' | 'QUEUE_RECEIPT_CONDITIONAL' | 'QUEUE_RECEIPT_BASE_MISMATCH'; export type ReceiptBinding = | { ok: true; receipt: ShipReceipt; head: string; line: string } | { ok: false; code: ReceiptBindingCode; message: string; line: string }; @@ -87,9 +87,12 @@ export type ReceiptBinding = * A receipt with a `base_red` block (lib/base-red.ts) is conditional: its * Step 5 did not pass, so it binds only for a caller that asks for it * (`/ship`'s terminal line, which reports the pause) and is refused - * everywhere else. + * everywhere else. With `base` (the queue's base), a receipt reviewed against + * another branch is refused: a #3570 child retargeted before its parent + * landed reviewed only its own change, and the carry rewrites `base` once the + * parent has landed (GST-2153). */ -export function bindReceipt(body: string, pr: string | undefined, accepted: readonly string[], opts: { conditional?: boolean } = {}): ReceiptBinding { +export function bindReceipt(body: string, pr: string | undefined, accepted: readonly string[], opts: { conditional?: boolean; base?: string } = {}): ReceiptBinding { const subject = pr === undefined ? "the receipt's" : `PR #${pr}'s`; const ex = extractReceipt(body); if (!ex.found) return { ok: false, code: 'QUEUE_RECEIPT_MISSING', message: `${subject} body carries no ship receipt`, line: 'SHIP_RECEIPT: none in the PR body' }; @@ -105,6 +108,11 @@ export function bindReceipt(body: string, pr: string | undefined, accepted: read return { ok: false, code: 'QUEUE_RECEIPT_STALE', message: `${subject} ship receipt is for ${oneLine(r.head.slice(0, 12))}, not the head ${want} being merged`, line }; } if (pr !== undefined && r.pr !== undefined && String(r.pr) !== pr) return { ok: false, code: 'QUEUE_RECEIPT_STALE', message: `${subject} ship receipt names pr=${r.pr}, not this PR`, line }; + const branch = (ref: string) => ref.replace(/^refs\/remotes\/[^/]+\//, '').replace(/^origin\//, ''); + if (opts.base !== undefined && branch(r.base) !== branch(opts.base)) { + const [was, want] = [oneLine(r.base).slice(0, 120), branch(opts.base)]; + return { ok: false, code: 'QUEUE_RECEIPT_BASE_MISMATCH', message: `${subject} ship receipt was reviewed against ${was}, not ${want}, so it covers only the change since ${was}; land the parent (${was}) first, merge ${want} into this branch and push, then gstack-ship-receipt carry --pr ${pr ?? ''} --base ${want} moves the receipt to ${want}`, line }; + } if (r.base_red && !opts.conditional) return { ok: false, code: 'QUEUE_RECEIPT_CONDITIONAL', message: `${subject} ship receipt is paused on a red base (${oneLine(r.base_red.cases.join(', ')).slice(0, 160)} fail at ${oneLine(r.base_red.base.slice(0, 12))}); /ship --resume rewrites it once the base passes them`, line }; return { ok: true, receipt: r, head, line }; } diff --git a/test/gstack-ship-queue.test.ts b/test/gstack-ship-queue.test.ts index 3ae78ec48..30eb1027e 100644 --- a/test/gstack-ship-queue.test.ts +++ b/test/gstack-ship-queue.test.ts @@ -361,6 +361,23 @@ describe('ready', () => { expect(queue(repo, ['ready', '--pr', '1'], shim(moved, green)).err).toMatch(/uncommitted tracked changes: pr-a\.txt.*\(QUEUE_NOT_READY\)/); }, 180_000); + test('QUEUE_RECEIPT_BASE_MISMATCH: a #3570 child retargeted to main while its parent is open is refused; a GST-1199 child whose receipt names main READYs', () => { + const repo = fixture(); + makePr(repo, 'parent'); + repo.git('push', '-q', 'origin', 'HEAD:refs/heads/parent'); + fs.writeFileSync(path.join(repo.work, 'child.txt'), 'child\n'); + repo.git('add', '-A'); + repo.git('commit', '-q', '-m', 'child'); + const head = repo.git('rev-parse', 'HEAD'); + const pregate = pregateFor(repo, head); + const shim = (base: string) => ghQueueShim(repo, { prs: { '2': { headRefOid: head, url: 'https://github.com/o/r/pull/2', baseRefName: 'main', body: shippedBody('2', head, { base }) } }, checks: { [head]: green } }); + const retargeted = queue(repo, ['ready', '--pr', '2', '--pregate', pregate], shim('parent')); + expect(retargeted.status).toBe(EXIT.refused); + expect(retargeted.out).toContain(`SHIP_RECEIPT: pr=2 head=${head.slice(0, 12)} version=- base=parent`); + expect(retargeted.err).toMatch(/PR #2's ship receipt was reviewed against parent, not main.*\(QUEUE_RECEIPT_BASE_MISMATCH/); + for (const base of ['main', 'origin/main']) expect(queue(repo, ['ready', '--pr', '2', '--pregate', pregate], shim(base)).status).toBe(EXIT.ok); + }, 120_000); + test('STAMP_VERSION is unstamped for a PR whose own diff leaves VERSION alone after main moved to a newer version, and the stamp for one that changes it (GST-770 follow-up)', () => { const repo = fixture(); const head = makePr(repo, 'pr-a'); diff --git a/test/ship-queue-land-direct.test.ts b/test/ship-queue-land-direct.test.ts index f91ff4ab4..032750b31 100644 --- a/test/ship-queue-land-direct.test.ts +++ b/test/ship-queue-land-direct.test.ts @@ -528,6 +528,35 @@ describe('land --pin: refusals (nothing merged)', () => { refused(fx, land(fx, '15', pin), 'QUEUE_RECEIPT_MISSING'); }, 90_000); + test('QUEUE_RECEIPT_BASE_MISMATCH: a #3570 child retargeted to main while its parent is open lands nothing; a GST-1199 child reviewed against main still lands', () => { + const fx = setup(); + const { repo } = fx; + openPr(fx, '17'); + const child = (pr: string, base: string): string => { + repo.git('checkout', '-q', 'origin/pr-17', '-B', 'pr-' + pr); + fs.writeFileSync(path.join(repo.work, 'pr-' + pr + '.txt'), pr + '\n'); + fs.writeFileSync(path.join(repo.work, 'changelog.d', 'pr-' + pr + '.md'), '- #' + pr + ' change\n'); + repo.git('add', '-A'); + repo.git('commit', '-q', '-m', 'pr ' + pr); + repo.git('push', '-q', '-f', 'origin', 'HEAD:refs/heads/pr-' + pr); + const head = repo.git('rev-parse', 'HEAD'); + fx.write(d => { d.prs[pr] = { state: 'OPEN', baseRefName: 'main', headRefName: 'pr-' + pr, isCrossRepository: false, title: 'PR ' + pr, body: shippedBody(pr, head, { base }) }; }); + markReady(fx, pr, head); + repo.git('checkout', '-q', '--detach', 'origin/main'); + return head; + }; + const before = remote(fx, 'main'); + const retargeted = child('171', 'pr-17'); + const r = land(fx, '171', retargeted); + refused(fx, r, 'QUEUE_RECEIPT_BASE_MISMATCH'); + expect(r.err).toContain("PR #171's ship receipt was reviewed against pr-17, not main"); + expect(remote(fx, 'main')).toBe(before); + const reviewedOnMain = child('172', 'origin/main'); + const ok = land(fx, '172', reviewedOnMain); + expect(ok.err).toBe(''); + expect(ok.status).toBe(EXIT.ok); + }, 90_000); + test('QUEUE_NOT_READY: no READY note at the pin', () => { const fx = setup(); const pin = openPr(fx, '16', { ready: false });