mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 10:40:32 +01:00
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/<base> 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>
This commit is contained in:
1 parent
dfdaea2efd
commit
00cb80cce8
10 files changed
+162
-3
No files matched your search
@@ -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' ;;
|
||||
|
||||
@@ -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/<base>` 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).
|
||||
+10
-2
@@ -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/<base>` 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 <d>`, 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
|
||||
|
||||
@@ -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.
|
||||
|
||||
<a id="queue-base-unfetched"></a>
|
||||
### `QUEUE_BASE_UNFETCHED`
|
||||
|
||||
**Meaning.** `gstack-ship-queue ready` (or the stamp path of `land`) could not
|
||||
fetch `origin/<base>`, 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/<base>` 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 <base>` work in this clone (network, remote
|
||||
URL, credentials), then rerun the same command.
|
||||
|
||||
<a id="queue-proof-missing"></a>
|
||||
### `QUEUE_PROOF_MISSING`
|
||||
|
||||
|
||||
@@ -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/<base>, so it did not compare the receipt with a base ref it could not refresh',
|
||||
fix: 'fix the fetch (git fetch origin <base>), 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)',
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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/<base> 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);
|
||||
});
|
||||
@@ -0,0 +1,104 @@
|
||||
/**
|
||||
* gstack-ship-queue ready refreshes origin/<base> 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<string, string>) => runBin(repo, 'gstack-ship-queue', [...args, '--base', 'main'], env);
|
||||
|
||||
function write(repo: FixtureRepo, files: Record<string, string>, 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/<base> 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/<base> 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);
|
||||
});
|
||||
Reference in new issue
Block a user