diff --git a/CHANGELOG.md b/CHANGELOG.md index e3e7b2ea3..c557d34ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,10 @@ - **Carry across rewritten history**: `write` records `head_tree` and `product_patch_id`, and each carry rebinds both and keeps earlier trees in `carry.trees`. A head that is no longer an ancestor carries only when its tree is one the receipt covers and its product patch-id against the current base matches; it records `carry.ancestry: rewritten` and needs no old commit, so a fresh clone carries too. A different tree is the new `RECEIPT_CARRY_TREE_MISMATCH`. The reviewed tree on a base that moved under it is `RECEIPT_CARRY_PRODUCT_CHANGED`. - **Docs**: docs/ship-policy.md#receipt-carry opens with a four-row table: merge with the product unchanged, merge that changes product, rebase onto a covered tree, and any other rebase. docs/captain.md and captain/templates/CREW.md say to merge main in and never rebase a reviewed branch, and the troubleshooting entries for the carry refusals match the new rules. +- **Stacked PRs** (Fixes #3570): after a stack parent lands and the child is retargeted to the base with the base merged in, `carry --base ` compares HEAD's product diff against the base with the range the receipt reviewed against the parent branch. That is the recorded `product_patch_id`, or the range against `origin/` for an older receipt. Equal carries with the reason `stack parent landed`; anything else refuses. Until now every stacked child paid a full /ship once its parent merged (gstack-evals#19). + ### Tests +- `test/ship-receipt-carry-squash.test.ts`: a child carries after its parent squash-merges and main is merged in. A child edited after the merge and a parent that never landed refuse. An older receipt falls back to `origin/` and refuses once that branch is deleted. - `test/ship-receipt-carry-rewrite.test.ts`: a rebase onto the covered tree carries from a fresh clone that never fetched the old head, a different tree refuses with `RECEIPT_CARRY_TREE_MISMATCH`, the reviewed tree on a moved base refuses, and a merge that changes only a registered output's diff carries with a pregate whose regen check proved it and refuses without one or with a pregate from another registry, and a hand-written file under an output glob that the generator never writes stays product. - `test/gstack-ship-receipt.test.ts`: the written receipt's key order includes `head_tree` and `product_patch_id`. diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index d3e2c342b..18e5c5045 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -542,8 +542,23 @@ function cmdCarry(rest: string[], json: boolean): number { // planCarry names at most CARRY_NAMED_PATHS differing paths, so the per-path comparison (two patch-ids each) stops there, // and it walks only the paths the branch itself touched against the base at either head: a base merge brings in every // path the base changed, none of which can differ between the two base diffs. + // A stacked child whose parent landed on the base (#3570): the receipt reviewed the child's own range against the + // parent branch, and the PR now targets the base. That reviewed range (the recorded product patch-id, else the one + // against origin/ when it still resolves) must be HEAD's whole product diff against the base: the parent + // fully landed and the child is unchanged. Anything else fails closed. + const reviewedBase = receipt.base.replace(/^origin\//, ''); + const retarget = reviewedBase !== base.replace(/^origin\//, ''); + let reviewed = productFrom; + if (retarget) { + const parentRef = `origin/${reviewedBase}`; + const recorded = receipt.product_patch_id !== undefined ? (receipt.product_patch_id === 'none' ? '' : receipt.product_patch_id) + : git(['rev-parse', '--verify', '--quiet', `${parentRef}^{commit}`], repoRoot) ? productPatchAgainstBase(repoRoot, rules, parentRef, from) : { error: `the receipt records no product patch-id and ${parentRef} no longer resolves` }; + if (typeof recorded !== 'string') return refused('RECEIPT_CARRY_REFUSED', `the receipt was reviewed against ${reviewedBase}, not ${base}, and its reviewed product diff is unavailable: ${recorded.error}; run /ship on ${head.slice(0, 12)}`); + reviewed = recorded; + } const changedProduct: string[] = []; - if (productFrom !== productHead) { + if (retarget && reviewed !== productHead) changedProduct.push(`HEAD's product diff against ${base} is not the one reviewed against ${reviewedBase} (the parent has not fully landed on ${base}, or the branch changed since)`); + else if (productFrom !== productHead && !retarget) { const touched = new Set([...(git(['diff', '--name-only', fromBase, from], repoRoot) ?? '').split('\n'), ...(git(['diff', '--name-only', headBase, head], repoRoot) ?? '').split('\n')].filter(Boolean)); for (const p of [...touched].sort()) { if (changedProduct.length >= CARRY_NAMED_PATHS) break; @@ -557,7 +572,7 @@ function cmdCarry(rest: string[], json: boolean): number { // Evidence for the record, never the decision: a slow range-diff is reported unavailable rather than waited for. const rangeRaw = spawnSync('git', ['range-diff', '--no-color', `${fromBase}..${from}`, `${headBase}..${head}`], { cwd: repoRoot, encoding: 'utf8', timeout: 15_000, maxBuffer: 64 * 1024 * 1024 }); const rangeDiff = rangeRaw.status === 0 ? (rangeRaw.stdout.trim().split('\n').slice(0, 20).join('\n') || '(empty: identical ranges)') : `unavailable: ${(rangeRaw.stderr ?? '').trim().split('\n')[0] || (rangeRaw.signal ? `range-diff killed by ${rangeRaw.signal} after 15 s` : 'range-diff failed')}`; - plan = planCarry({ from, head, changed, productPatchId: { from: productFrom, head: productHead }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff }); + plan = planCarry({ from, head, changed, productPatchId: { from: reviewed as string, head: productHead as string }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff, ...(retarget ? { retargetedFrom: reviewedBase } : {}) }); } if (!plan.ok) return refused(plan.code, plan.message); // Eligibility is not publication: a receipt that recorded a pregate is rebound only with a pregate current for HEAD, diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 61e0059ef..b954d0bfe 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -901,6 +901,20 @@ receipt when the base moves: | a merge of the base whose resolution changed product code | a fresh /ship (`RECEIPT_CARRY_PRODUCT_CHANGED`) | | a rebase or force-push onto a tree the receipt covers, product diff against the new base unchanged | carries (`carry.ancestry: rewritten`) | | any other rebase or force-push | a fresh /ship (`RECEIPT_CARRY_TREE_MISMATCH`) | +| a stacked PR whose parent landed (squash or merge), retargeted to the base with the base merged in, HEAD's product diff exactly the one reviewed against the parent branch | carries (reason `stack parent landed`) | + +**Stacked PRs (#3570).** A child PR's receipt is written against its parent +branch, so its `base` names that branch and its `product_patch_id` is the +child's own range. After the parent lands, the PR is retargeted to the base, and +/ship's carry check runs with `--base `. When that base differs from the +receipt's, the carry compares the reviewed range (the recorded +`product_patch_id`, or for an older receipt the range against +`origin/` while that branch still resolves) with HEAD's whole product +diff against the base. Equal means the parent fully landed and the child is +unchanged, so the receipt carries. A parent that has not landed, a child edited +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`. Since the release that added `review_scope`. A `/ship` receipt names the head it reviewed, so a conflict-only merge of the base or a test-only fix used to diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index f0f574faf..3bb588677 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -4730,7 +4730,10 @@ registered output the pregate's regen check proved (listed under `proven`, checked against the same base registry), nor part of the base merge has a different patch than the reviewed head had. After a rebase it means HEAD has the reviewed tree but the base moved under it, so the diff a reviewer would see -is not the reviewed one. The reviewed verdict does not cover HEAD, so the +is not the reviewed one. For a stacked PR retargeted to the base after its +parent landed, it means HEAD's product diff against the base is not the range +reviewed against the parent branch: the parent has not fully landed, or the +branch changed since. The reviewed verdict does not cover HEAD, so the receipt is not carried. The refusal names the changed product paths. **Fix.** Run /ship on the new head. The receipt carries only across a diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index aa86abf11..afc77370a 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -164,6 +164,8 @@ export interface CarryPlanInput { changedProduct: string[]; mergeCommits: number; rangeDiff: string; + /** The base the receipt was reviewed against when it differs from the carry's (a stack parent that landed, #3570): productPatchId.from is then the recorded diff against it. */ + retargetedFrom?: string; } export interface CarryRecord { from: string; reason: string; product_patch_id: string; changed_paths: string[]; merge_commits: number; range_diff: string; covers: string[]; reruns: string[]; trees?: string[]; ancestry?: 'linear' | 'rewritten' } export type CarryPlan = { ok: true; carry: CarryRecord } | { ok: false; code: 'RECEIPT_CARRY_PRODUCT_CHANGED' | 'RECEIPT_CARRY_REFUSED' | 'RECEIPT_CARRY_TREE_MISMATCH'; message: string }; @@ -230,7 +232,7 @@ export function planCarry(input: CarryPlanInput): CarryPlan { const named = input.changedProduct.slice(0, CARRY_NAMED_PATHS); return { ok: false, code: 'RECEIPT_CARRY_PRODUCT_CHANGED', message: `product diff changed between ${input.from.slice(0, 12)} and ${input.head.slice(0, 12)}: ${named.join(', ') || 'patch-id differs'}${input.changedProduct.length > named.length ? ` (+${input.changedProduct.length - named.length} more)` : ''}` }; } - const reasons: string[] = []; + const reasons: string[] = input.retargetedFrom ? [`stack parent ${input.retargetedFrom} landed`] : []; if (input.mergeCommits > 0) reasons.push(input.mergeCommits === 1 ? 'base merge' : `${input.mergeCommits} base merges`); if (input.testPatchId.from !== input.testPatchId.head) reasons.push('test-only change'); if (input.changed.some(c => c.class === 'metadata')) reasons.push('release metadata'); diff --git a/test/ship-receipt-carry-squash.test.ts b/test/ship-receipt-carry-squash.test.ts new file mode 100644 index 000000000..8c00defea --- /dev/null +++ b/test/ship-receipt-carry-squash.test.ts @@ -0,0 +1,108 @@ +/** + * gstack-ship-receipt carry for a stacked child after its parent squash-merges (#3570, + * docs/ship-policy.md#receipt-carry): the receipt reviewed the child's own range against the parent + * branch, and that range must be HEAD's whole product diff against the base the PR now targets. + * Anything else (the parent not landed, the child changed) fails closed. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { EXIT } from '../lib/headless-artifacts'; +import { extractReceipt, renderReceiptBlock } from '../lib/ship-receipt'; +import { cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); +const receiptIn = (body: string) => extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; +const carry = (repo: FixtureRepo, body: string) => runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); +const SUB = "exports.add = (a, b) => a + b;\nexports.sub = (a, b) => a - b;\n"; + +/** main <- parent (lib/util.js) <- feature (lib/retry.js, its /ship receipt written against the parent branch). */ +function stack(repo: FixtureRepo): { parent: string; child: string; body: string } { + repo.git('checkout', '-q', '-b', 'parent'); + const parent = commit(repo, { 'lib/util.js': SUB }, 'parent: sub'); + repo.git('push', '-q', 'origin', 'parent'); + repo.git('checkout', '-q', 'feature'); + repo.git('reset', '-q', '--hard', parent); + const child = commit(repo, { 'lib/retry.js': "exports.MAX_RETRIES = 4;\nexports.TIMEOUT_MS = 5000;\nexports.retry = (fn) => fn();\n" }, 'child: four retries'); + const body = path.join(repo.root, 'body.md'); + const w = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '8', '--base', 'parent', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body]); + expect(w.status).toBe(EXIT.ok); + return { parent, child, body }; +} +/** Land `files` on main as one commit (the parent's squash merge; none when the parent never lands), then an unrelated main commit, and merge main into the child. */ +function landOnMain(repo: FixtureRepo, files: Record): void { + repo.git('checkout', '-q', 'main'); + if (Object.keys(files).length) { + for (const [rel, text] of Object.entries(files)) write(repo.work, rel, text); + repo.git('add', '-A'); repo.git('commit', '-qm', 'squash: parent (#7)'); + } + write(repo.work, 'docs/README.md', '# docs after the parent\n'); + repo.git('add', '-A'); repo.git('commit', '-qm', 'main moves on'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feature'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); +} + +describe('carry after a stack parent squash-merges (#3570)', () => { + // Value: protects=a stacked child keeps its receipt when its parent squash-merges and main is merged in (gstack-evals#19 + // paid a full /ship for it); fails_when=carry compares the child's whole diff from the old fork point; seam=none; + // no_claim=gh pr edit (--body stands in) + test('the child carries when the range it was reviewed for is HEAD\'s whole product diff against main', () => { + const repo = makeFixtureRepo(); repos.push(repo); + const { child, body } = stack(repo); + const reviewed = receiptIn(body).product_patch_id; + landOnMain(repo, { 'lib/util.js': SUB }); + const head = repo.git('rev-parse', 'HEAD'); + const out = carry(repo, body); + expect(out.status).toBe(EXIT.ok); + expect(out.out).toContain(`CARRY: carried from ${child.slice(0, 12)} to ${head.slice(0, 12)}`); + const r = receiptIn(body); + expect(r.head).toBe(head); + expect(r.carry!.reason).toBe('stack parent parent landed + base merge'); + expect(r.product_patch_id).toBe(reviewed); + }, 90_000); + + // Value: protects=a receipt written before product_patch_id still carries while the parent branch resolves, and refuses by + // name once it is gone; fails_when=the fallback reads another ref or carries with no reviewed diff; seam=none; no_claim=- + test('without a recorded product patch-id the child range comes from origin/, and a deleted parent branch refuses', () => { + const repo = makeFixtureRepo(); repos.push(repo); + const { parent, child, body } = stack(repo); + landOnMain(repo, { 'lib/util.js': SUB }); + const { product_patch_id: _dropped, head_tree: _tree, ...legacy } = receiptIn(body); + fs.writeFileSync(body, renderReceiptBlock(legacy as any)); + expect(receiptIn(body).product_patch_id).toBeUndefined(); + repo.git('push', '-q', 'origin', '--delete', 'parent'); + repo.git('fetch', '-q', '--prune', 'origin'); + const gone = carry(repo, body); + expect(gone.status).toBe(EXIT.refused); + expect(gone.out).toContain('CARRY: refused (RECEIPT_CARRY_REFUSED)'); + expect(gone.err).toContain('the receipt was reviewed against parent, not main, and its reviewed product diff is unavailable'); + expect(receiptIn(body).head).toBe(child); + repo.git('push', '-q', 'origin', `${parent}:refs/heads/parent`); + repo.git('fetch', '-q', 'origin'); + expect(carry(repo, body).status).toBe(EXIT.ok); + }, 90_000); + + // Value: protects=fail closed: a child changed after the parent merged, or a parent that never reached main, takes a fresh + // /ship; fails_when=the comparison ignores the child's remainder or the parent's absence; seam=none; no_claim=- + test('a product edit on the child after the merge, or a parent that never landed, refuses with RECEIPT_CARRY_PRODUCT_CHANGED', () => { + const repo = makeFixtureRepo(); repos.push(repo); + const { child, body } = stack(repo); + landOnMain(repo, { 'lib/util.js': SUB }); + commit(repo, { 'lib/retry.js': "exports.MAX_RETRIES = 5;\nexports.TIMEOUT_MS = 5000;\nexports.retry = (fn) => fn();\n" }, 'child: five'); + const edited = carry(repo, body); + expect(edited.status).toBe(EXIT.refused); + expect(edited.out).toContain('CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)'); + expect(receiptIn(body).head).toBe(child); + + const other = makeFixtureRepo(); repos.push(other); + const s = stack(other); + landOnMain(other, {}); + const abandoned = carry(other, s.body); + expect(abandoned.status).toBe(EXIT.refused); + expect(abandoned.out).toContain('CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)'); + expect(abandoned.err).toContain('is not the one reviewed against parent (the parent has not fully landed on main'); + expect(receiptIn(s.body).head).toBe(s.child); + }, 120_000); +});