From 00cb80cce8abb2acd29f0e3966f45aa6d6dd3950 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 9 Nov 2026 17:03:46 -0800 Subject: [PATCH] Make the ship queue fetch the latest main before it checks a pull request (GST-2232) (#4256) * Make ship-queue ready fetch the base before it checks the receipt (GST-2232) ready read origin/ as the clone last fetched it, so a lane a few landings behind measured the PR from an older merge base and refused a good receipt with a false RECEIPT_INVALID field=review_exclude. ready now fetches the base first and refuses QUEUE_BASE_UNFETCHED when it cannot; the stamp path of land refuses the same way instead of ignoring a failed fetch. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Regenerate the doctor component list for the new queue refusal code (GST-2232) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Keep the new ready test off the Windows-safe set and say which land path uses the new refusal (GST-2232) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-doctor-components.sh | 1 + .../ship-queue-ready-fetches-base-gst-2232.md | 4 + docs/ship-policy.md | 12 +- docs/troubleshooting.md | 14 +++ lib/result-codes-release.ts | 5 + lib/ship-queue-land.ts | 2 +- lib/ship-queue.ts | 2 + scripts/lib/windows-curation.ts | 4 + test/ship-queue-land.test.ts | 17 +++ test/ship-queue-ready-fresh-base.test.ts | 104 ++++++++++++++++++ 10 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 changelog.d/ship-queue-ready-fetches-base-gst-2232.md create mode 100644 test/ship-queue-ready-fresh-base.test.ts diff --git a/bin/gstack-doctor-components.sh b/bin/gstack-doctor-components.sh index 1a29a9c1d..e84015ad7 100644 --- a/bin/gstack-doctor-components.sh +++ b/bin/gstack-doctor-components.sh @@ -262,6 +262,7 @@ gstack_result_anchor() { STABLE_RELEASE_UNKNOWN) printf '%s' 'stable-release-unknown' ;; PREGATE_GSTACK_TOO_OLD) printf '%s' 'pregate-gstack-too-old' ;; QUEUE_NOT_READY) printf '%s' 'queue-not-ready' ;; + QUEUE_BASE_UNFETCHED) printf '%s' 'queue-base-unfetched' ;; QUEUE_PROOF_MISSING) printf '%s' 'queue-proof-missing' ;; QUEUE_STAMP_NOT_STAMP_ONLY) printf '%s' 'queue-stamp-not-stamp-only' ;; QUEUE_VERIFY_MISMATCH) printf '%s' 'queue-verify-mismatch' ;; diff --git a/changelog.d/ship-queue-ready-fetches-base-gst-2232.md b/changelog.d/ship-queue-ready-fetches-base-gst-2232.md new file mode 100644 index 000000000..6e48314c8 --- /dev/null +++ b/changelog.d/ship-queue-ready-fetches-base-gst-2232.md @@ -0,0 +1,4 @@ +--- +important: blocker +--- +- **`gstack-ship-queue ready` no longer refuses a good receipt because the lane's copy of the base was old.** `ready` read `origin/` as the clone last fetched it, so a lane a few landings behind measured the PR from an older merge base, counted files from PRs that had already landed, and refused with a false `RECEIPT_INVALID field=review_exclude` (gbrain #8991). `ready` now fetches the base before it reads the policy or checks any receipt field, and refuses with the new `QUEUE_BASE_UNFETCHED` when the fetch fails; the stamp path of `land` refuses the same way instead of carrying on with the old ref. The selection itself was already measured from the merge base (the PR's own files), and a new test pins that (GST-2232). diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 17eb8c464..453696a36 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -732,7 +732,13 @@ slot. The queue enforces this at every step from the PR body alone: `REVIEW_TARGETED:` line) when a targeted class the head needs has no done targeted pass in the receipt, and `QUEUE_RECEIPT_EXCEPTION_INVALID` when an owner exception it records does not stand ([the ship receipt](#ship-receipt)). - There is no flag around it. + There is no flag around it. Before any of these checks, `ready` fetches + `origin/` and reads the policy and every receipt field against that + fresh ref, so a lane clone whose base is behind the remote never turns a good + receipt into `RECEIPT_INVALID` (GST-2232); a failed fetch refuses with + `QUEUE_BASE_UNFETCHED` before anything is compared. The stamp path of `land` + refuses the same way; `land --pin` (`merge: direct`) refuses a failed fetch + with `QUEUE_NOT_READY`, as it already did. - `train build` refuses a pin whose PR has no current receipt with the same codes, naming the PR, before anything is written, pushed or dispatched, `--dry-run` included. @@ -1546,7 +1552,9 @@ same one (a mixed set makes it `VERIFY: dirty`). `gstack-ship-receipt write --review-exclude `, with that digest, records `review_exclude` (source, patterns, counts, the gate-tagged kept paths and three hashes). The `digest` hashes the declared patterns and the excluded and kept lists; `source` is provenance only. Write, carry and the queue -each recompute the digest at the current base and compare it, so a base merge +each recompute the digest at the current base and compare it (the queue fetches +that base first), over the PR's own files: the diff from the head's merge base +with that base, never a two-dot diff against the base tip, so a base merge or a moved base that leaves the patterns and the lists unchanged still carries and binds. A different digest means the digest write was given (or the receipt recorded) names another selection than the head has now: write refuses with `RECEIPT_INVALID field=review_exclude`, carry with diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 3a4609292..42f793518 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -5490,6 +5490,20 @@ the same `train land` command; it adopts its own journaled stamp commit. For an unreadable land lock, rerun the same `land` command once origin answers; it resumes its own lock. + +### `QUEUE_BASE_UNFETCHED` + +**Meaning.** `gstack-ship-queue ready` (or the stamp path of `land`) could not +fetch `origin/`, so it stopped before reading the policy or comparing +any receipt field. Both compare the receipt's recorded selection, depth and +targeted classes with what the current base gives the head; a local +`origin/` a few landings old measures the PR from an older merge base, +pulls in files of PRs that already landed, and reports a false +`RECEIPT_INVALID field=review_exclude` (GST-2232). Nothing was written. + +**Fix.** Make `git fetch origin ` work in this clone (network, remote +URL, credentials), then rerun the same command. + ### `QUEUE_PROOF_MISSING` diff --git a/lib/result-codes-release.ts b/lib/result-codes-release.ts index 40db17e36..43a80af26 100644 --- a/lib/result-codes-release.ts +++ b/lib/result-codes-release.ts @@ -36,6 +36,11 @@ export const RELEASE_RESULT_CODES = { summary: 'the lane is not ready to report: the working tree, HEAD or CI state does not match the pushed PR head', fix: 'commit and push the work (or wait for CI on that exact head), then rerun gstack-ship-queue ready', }, + QUEUE_BASE_UNFETCHED: { + anchor: 'queue-base-unfetched', + summary: 'ready, or the stamp path of land, could not fetch origin/, so it did not compare the receipt with a base ref it could not refresh', + fix: 'fix the fetch (git fetch origin ), then rerun the same command', + }, QUEUE_PROOF_MISSING: { anchor: 'queue-proof-missing', summary: 'a proof input the queue step needs is missing or does not match (gated head, base, stamp journal, version or tree)', diff --git a/lib/ship-queue-land.ts b/lib/ship-queue-land.ts index b853cae5d..3f58372e6 100644 --- a/lib/ship-queue-land.ts +++ b/lib/ship-queue-land.ts @@ -341,7 +341,7 @@ export function land(ctx: QueueContext, o: LandOptions): StepOutput { const recovered = j ? null : stampPin(root, info.headRefName, info.headRefOid, o.pr); const pin = j?.reviewed_pin ?? recovered ?? info.headRefOid; const lines = [`LAND: #${o.pr} branch=${info.headRefName} pin=${pin}${j ? ` (resuming at ${j.phase})` : recovered ? ` (the head ${info.headRefOid.slice(0, 12)} is land's stamp commit; restamping from its pin)` : ''}`, ...(reset ? [reset] : [])]; - git(root, ['fetch', '-q', 'origin', ctx.base]); + if (!git(root, ['fetch', '-q', 'origin', ctx.base]).ok) return fail(lines, { code: 'QUEUE_BASE_UNFETCHED', message: `cannot fetch origin/${ctx.base}; land checks the receipt against the current base, never a ref it did not refresh`, fix: `fix the fetch (git fetch origin ${ctx.base}), then rerun land` }, run); const receipt = bindShippedReceipt(root, `origin/${ctx.base}`, info.body ?? '', o.pr, [pin], mergedResume); lines.push(receipt.line, ...(receipt.depthLine ? [receipt.depthLine] : []), ...(receipt.targetedLine ? [receipt.targetedLine] : [])); if (!receipt.ok) return fail(lines, { code: receipt.code, message: receipt.message, fix: 'run /ship at the pinned head first' }, run); diff --git a/lib/ship-queue.ts b/lib/ship-queue.ts index d03b01653..b396b0403 100644 --- a/lib/ship-queue.ts +++ b/lib/ship-queue.ts @@ -259,6 +259,8 @@ function gateLine(policy: LoadedPolicy, attempt: AttemptRecord | null): string { /** READY: one block, identifiers first, about the exact pushed PR head. */ export function ready(ctx: QueueContext, o: { pr: string; pregate?: string; gateExit?: number; gateLog?: string }): StepOutput { const run = PREGATE_OUT_REL; + const fetched = git(ctx.repoRoot, ['fetch', '--no-tags', '-q', 'origin', `+refs/heads/${ctx.base}:refs/remotes/origin/${ctx.base}`]); + if (!fetched.ok) return fail({ code: 'QUEUE_BASE_UNFETCHED', message: `cannot fetch origin/${ctx.base} (${fetched.err.trim().split('\n')[0]?.slice(0, 200) ?? ''}); ready reads the policy and checks the receipt against the current base, never a ref it did not refresh`, fix: `fix the fetch (git fetch origin ${ctx.base}), then rerun ready` }, [], run); const policy = loadBasePolicy(ctx); if ('message' in policy) return fail(policy, [], run); const head = git(ctx.repoRoot, ['rev-parse', 'HEAD']).out; diff --git a/scripts/lib/windows-curation.ts b/scripts/lib/windows-curation.ts index 0851e3488..38fe3b82f 100644 --- a/scripts/lib/windows-curation.ts +++ b/scripts/lib/windows-curation.ts @@ -313,6 +313,10 @@ export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }> file: 'test/gstack-ship-queue.test.ts', reason: 'spawns bin/gstack-ship-queue (shebang) and a #!/bin/sh gh shim through test/helpers/restamp-fixture.ts and test/helpers/ship-queue-fixture.ts', }, + { + file: 'test/ship-queue-ready-fresh-base.test.ts', + reason: 'spawns bin/gstack-ship-queue (shebang) and a #!/bin/sh gh shim through test/helpers/restamp-fixture.ts and test/helpers/ship-queue-fixture.ts (windows-scoped-shard, run 42720453184)', + }, { file: 'crew/test/release-gate-workflow.test.ts', reason: 'the gate-step case splices a native temp-path stub into the step\'s bash -e script in place of crew/bin/gstack-release-gate, and on windows-latest its gated case exits non-zero (expected 0; windows-scoped-shard, run 42421330820); release-gate.yml itself runs only on ubuntu-latest', diff --git a/test/ship-queue-land.test.ts b/test/ship-queue-land.test.ts index 94088a127..13f816660 100644 --- a/test/ship-queue-land.test.ts +++ b/test/ship-queue-land.test.ts @@ -786,3 +786,20 @@ describe('land: the READY receipt', () => { expect(land(fx, ['--pr', '28']).out).toContain('PUSHED: '); }, 90_000); }); + +describe('land refreshes the base before its receipt check (GST-2232)', () => { + // Value: protects=land never binds a receipt against an origin/ it could not refresh; fails_when=land ignores a failed + // base fetch and compares the receipt with the clone's old ref (the false RECEIPT_INVALID ready gave gbrain #8991); + // why_new=every land test reaches a working origin; seam=land -> bindShippedReceipt; no_claim=merge: direct land (it refused + // a failed fetch already) + test('an unfetchable origin/main refuses QUEUE_BASE_UNFETCHED before the receipt is compared or anything is pushed', () => { + const fx = setup(); + const pin = openPr(fx, '7'); + fx.repo.git('remote', 'set-url', 'origin', path.join(fx.repo.root, 'gone.git')); + const r = land(fx, ['--pr', '7']); + expect(r.status).toBe(EXIT.refused); + expect(r.err).toMatch(/cannot fetch origin\/main.*\(QUEUE_BASE_UNFETCHED\)/); + expect(r.out).not.toContain('SHIP_RECEIPT:'); + expect(fx.repo.git('--git-dir', fx.repo.origin, 'rev-parse', 'refs/heads/pr-7')).toBe(pin); + }, 90_000); +}); diff --git a/test/ship-queue-ready-fresh-base.test.ts b/test/ship-queue-ready-fresh-base.test.ts new file mode 100644 index 000000000..a03556269 --- /dev/null +++ b/test/ship-queue-ready-fresh-base.test.ts @@ -0,0 +1,104 @@ +/** + * gstack-ship-queue ready refreshes origin/ before it compares any receipt field (GST-2232): a lane clone whose + * origin/main is behind the real remote must not turn a good receipt into RECEIPT_INVALID. The review_exclude + * selection is the PR's own files, measured from the merge base with the fresh base, never a two-dot diff against + * the base tip. Real fixture repos (bare origin plus working clone) and an offline gh. + */ +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 { REVIEW_EXCLUDE_KEY_VERSION } from '../lib/ship-policy'; +import { BASE_POLICY, cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; +import { ghQueueShim } from './helpers/ship-queue-fixture'; +import { shippedBody } from './helpers/ship-queue-train-fixture'; + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); + +const green = { check_runs: [{ status: 'completed', conclusion: 'success' }] }; +const queue = (repo: FixtureRepo, args: string[], env: Record) => runBin(repo, 'gstack-ship-queue', [...args, '--base', 'main'], env); + +function write(repo: FixtureRepo, files: Record, message: string): string { + for (const [rel, text] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(repo.work, rel)), { recursive: true }); + fs.writeFileSync(path.join(repo.work, rel), text); + } + repo.git('add', '-A'); + repo.git('commit', '-q', '-m', message); + return repo.git('rev-parse', 'HEAD'); +} + +/** Lands `branch` on the remote main as a merge commit (merge: direct), then fetches, as another lane's landing does. */ +function land(repo: FixtureRepo, branch: string): string { + const back = repo.git('rev-parse', '--abbrev-ref', 'HEAD'); + repo.git('checkout', '-q', '-B', 'landing', 'origin/main'); + repo.git('merge', '-q', '--no-ff', '-m', `Merge ${branch}`, branch); + repo.git('push', '-q', 'origin', 'HEAD:main'); + repo.git('fetch', '-q', 'origin'); + repo.git('checkout', '-q', back); + return repo.git('rev-parse', 'origin/main'); +} + +/** + * GBRA-108's shape: #2 is stacked on #1, both add a file under review_exclude_paths, and #1 landed. Against the fresh base the + * merge base is #1's head, so the selection holds #2's file alone; against the pre-landing base it also holds #1's. + */ +function stackedAfterParentLanded() { + const repo = makeFixtureRepo({ prefix: 'gstack-ready-base-', policy: { ...BASE_POLICY, review_exclude_paths: ['fixtures/goldens/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION } }); + repos.push(repo); + const before = repo.git('rev-parse', 'origin/main'); + repo.git('checkout', '-q', '-b', 'parent', 'origin/main'); + write(repo, { 'fixtures/goldens/parent.json': '{"parent": 1}\n', 'parent.txt': 'parent\n' }, 'parent'); + repo.git('checkout', '-q', '-b', 'child'); + const head = write(repo, { 'fixtures/goldens/child.json': '{"child": 2}\n', 'child.txt': 'child\n' }, 'child'); + const landed = land(repo, 'parent'); + const tree = runBin(repo, 'gstack-wtree', []).out.trim(); + const pregate = path.join(repo.root, 'pregate.json'); + fs.writeFileSync(pregate, JSON.stringify({ schema_version: 1, tree, head, verdict: 'pass', checks: [{ id: 'unit', status: 'pass' }] })); + const ready = (digest: string) => queue(repo, ['ready', '--pr', '2', '--pregate', pregate], ghQueueShim(repo, { prs: { '2': { headRefOid: head, url: 'https://github.com/o/r/pull/2', body: shippedBody('2', head, { review_exclude: { source: 'origin/main', patterns: ['fixtures/goldens/**'], applied: ['fixtures/goldens/**'], excluded: 1, excluded_bytes: 0, kept: 0, gate_tagged: [], patterns_sha256: 'b'.repeat(64), list_sha256: 'c'.repeat(64), digest } }) } }, checks: { [head]: green } })); + const probe = ready('a'.repeat(64)); + expect(probe.status, probe.out + probe.err).toBe(EXIT.refused); + const digest = /field=review_exclude expected=([0-9a-f]{64}) recorded=a{64} source=origin\/main@([0-9a-f]{12})/.exec(probe.err); + expect(digest?.[2], probe.out + probe.err).toBe(landed.slice(0, 12)); + return { repo, before, landed, head, digest: digest![1]!, ready }; +} + +describe('ready refreshes origin/ before comparing receipt fields (GST-2232)', () => { + // Value: protects=a lane whose origin/main is three landings old READYs a receipt /ship wrote against the real base instead + // of refusing RECEIPT_INVALID (GBRA-108, gbrain #8991); fails_when=ready reads the local origin/ without fetching, or + // compares against a ref it could not refresh; why_new=every earlier ready test pushes and fetches before it runs, so the + // local ref was never behind; seam=ready -> bindShippedReceipt -> excludeMismatch; no_claim=land's own fetch (it refetches) + test('a stale local origin/main is fetched, so the receipt binds; an unfetchable base refuses QUEUE_BASE_UNFETCHED before any field is compared', () => { + const { repo, before, landed, digest, ready } = stackedAfterParentLanded(); + expect(ready(digest).status).toBe(EXIT.ok); + repo.git('update-ref', 'refs/remotes/origin/main', before); + const stale = ready(digest); + expect([stale.status, stale.err]).toEqual([EXIT.ok, '']); + expect(stale.out).toContain(`base: origin/main@${landed}`); + expect(repo.git('rev-parse', 'origin/main')).toBe(landed); + repo.git('update-ref', 'refs/remotes/origin/main', before); + repo.git('remote', 'set-url', 'origin', path.join(repo.root, 'gone.git')); + const offline = ready(digest); + expect(offline.status).toBe(EXIT.refused); + expect(offline.err).toMatch(/cannot fetch origin\/main.*\(QUEUE_BASE_UNFETCHED\)/); + expect(offline.err).not.toContain('field=review_exclude'); + }, 180_000); + + // Value: protects=the queue's review_exclude selection is the PR's own files (merge-base diff), so a file another PR landed + // on the base after this one forked never enters it; fails_when=the selection is a two-dot diff against the base tip (the + // landed file reads as this PR deleting it) and every PR behind a landing refuses RECEIPT_INVALID; why_new=no ready test + // moved the base under an exclusion; seam=reviewExclude's range via bindShippedReceipt; no_claim=a squash-landed stacked + // parent, whose files the merge-base diff still holds on both the review and the queue side (GSTA-475's class) + test('a file under review_exclude_paths that landed on the base after the PR forked does not change the selection', () => { + const { repo, digest, ready } = stackedAfterParentLanded(); + const back = repo.git('rev-parse', '--abbrev-ref', 'HEAD'); + repo.git('checkout', '-q', '-b', 'other', 'origin/main'); + write(repo, { 'fixtures/goldens/other.json': '{"other": 3}\n', 'other.txt': 'other\n' }, 'other'); + repo.git('checkout', '-q', back); + const moved = land(repo, 'other'); + const after = ready(digest); + expect([after.status, after.err]).toEqual([EXIT.ok, '']); + expect(after.out).toContain(`base: origin/main@${moved}`); + }, 180_000); +});