From 6299a97a48353b6c677a15732ddc6120e96b4a71 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 21:32:57 +0000 Subject: [PATCH] fix(ship-queue): direct land fails closed without a merge base; stamping land refuses under merge: direct; Windows curation for the gh shim test Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-queue | 4 +-- docs/ship-policy.md | 2 +- lib/ship-queue-land-direct.ts | 12 ++++--- lib/ship-queue-land.ts | 1 + scripts/lib/windows-curation.ts | 1 + test/ship-queue-land-direct.test.ts | 54 +++++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 7 deletions(-) diff --git a/bin/gstack-ship-queue b/bin/gstack-ship-queue index 8bdeba736..3575a29da 100755 --- a/bin/gstack-ship-queue +++ b/bin/gstack-ship-queue @@ -176,8 +176,8 @@ async function main(argv: string[]): Promise { const l = await import('../lib/ship-queue-land'); if (args.includes('--census')) return finish('land', l.census(ctx)); if (!pr || !/^\d+$/.test(pr)) return usage('land needs --pr (or --census)'); - const pin = flag(args, '--pin'); - if (pin !== undefined) { + if (args.includes('--pin')) { + const pin = flag(args, '--pin') ?? ''; if (!/^[0-9a-f]{40,64}$/.test(pin)) return usage('--pin needs the full head sha the READY names'); return finish('land', (await import('../lib/ship-queue-land-direct')).landDirect(ctx, { pr, pin, dryRun: args.includes('--dry-run') })); } diff --git a/docs/ship-policy.md b/docs/ship-policy.md index e376df703..059d36596 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -63,7 +63,7 @@ refuses the file (`POLICY_GSTACK_TOO_OLD`). `validate` ends with | `min_gstack_version` | version or `null` | `null` | The oldest gstack that may read the file; see the compatibility rule above. | | `docs` | `{ caps: [{ path, max_bytes }] }` or `null` | `null` | Documentation files with a byte cap. `/ship` Step 14.5 runs `gstack-ship-receipt docs-caps --base ` (the caps come from `origin/`'s policy, never the branch's copy; `--policy ` is for a repository bootstrapping its policy) before the verification gate and prints one `DOCS_CAP: / ok\|over` line per cap; any `over` (an absent file counts) exits 1 and blocks the ship until the file is trimmed or the cap raised on the base branch (`DOCS_AUDIT_BLOCKED`). At most 64 caps, each path contained. See [the ship receipt](#ship-receipt). Needs `min_gstack_version`. | | `train` | `{ gate_workflow, free_workflow, full_gate_check, record_artifact, max_size, fill_hours, screen_selection, trial_reservation_usd, land? }`, `{ gates, land, max_size, fill_hours }` or `null` | `null` | Turns on [merge trains](#run-a-merge-train), read from the base revision only. Every field is required and an unknown field is `POLICY_INVALID`: `max_size` 1 to 6, `fill_hours` above 0 and at most 24, `screen_selection` `gstack-paid` \| `none`, `trial_reservation_usd` above 0 and at most 100. A block with `gates` ([any repository](#train-gates)) takes `max_size` 1 to 5 and needs `min_gstack_version` at least `TRAIN_GATES_KEY_VERSION`. `null` keeps the per-PR queue exactly as it is. Needs `min_gstack_version`. | -| `merge` | `train` \| `direct` | `train` | `direct` lands each READY PR at its own green head with [`land --pr --pin `](#land-direct): no train and no stamp commit, and `train build` refuses. A PR then leaves VERSION, the `package.json` version and the CHANGELOG at its merge base, and the release sets them. `train` keeps merge trains. Read from the base revision only. Needs `min_gstack_version`. | +| `merge` | `train` \| `direct` | `train` | `direct` lands each READY PR at its own green head with [`land --pr --pin `](#land-direct): no train and no stamp commit, and `train build` refuses. A PR then leaves VERSION, the `package.json` version and the CHANGELOG at its merge base, and the release sets them. Needs `changelog_fragments`, and the stamping `land --pr ` refuses under it. `train` keeps merge trains. Read from the base revision only. Needs `min_gstack_version`. | | `base_red` | `{ setup, select, timeout_sec }` or `null` | `null` | How `/ship` Step 5 proves a failure is the base branch's own before pausing on it; see [base_red](#base-red). Read from the base revision only. Needs `min_gstack_version`. | | `changelog_fragments` | `{ dir, required_checks? }` or `null` | `null` | PRs carry unversioned [CHANGELOG fragments](#changelog-fragments) in `dir` (one flat, repo-relative directory, no `..`) and the merge step ([`land`](#land)) assigns the version. `required_checks` (up to 50 distinct names) are the checks [`train land`](#stamped-trains-under-changelog_fragments-train-land) requires present and green on a stamp commit. `null` keeps `[Unreleased]` sections and per-PR stamps. Needs `min_gstack_version`. | diff --git a/lib/ship-queue-land-direct.ts b/lib/ship-queue-land-direct.ts index 16ff84b2a..4c1b663b1 100644 --- a/lib/ship-queue-land-direct.ts +++ b/lib/ship-queue-land-direct.ts @@ -26,11 +26,12 @@ function view(ctx: QueueContext, pr: string): DirectPr | QueueFailure { try { return JSON.parse(r.out) as DirectPr; } catch { return { code: 'QUEUE_NOT_READY', message: `gh pr view ${pr} failed: ${(r.err ?? r.out).trim().slice(0, 160)}` }; } } -/** Release files the pin changed since its merge base: the version file, the manifest version and the CHANGELOG. */ -export function stampedFiles(root: string, policy: ShipPolicy, mergeBase: string, pin: string): string[] { +/** Release files the pin changed since its merge base: the version file, the manifest version and the CHANGELOG; an unreadable version is an error, never a pass. */ +export function stampedFiles(root: string, policy: ShipPolicy, mergeBase: string, pin: string): string[] | { error: string } { const out: string[] = []; const [was, now] = [versionAtRef(root, mergeBase), versionAtRef(root, pin)]; - if ('version' in was && 'version' in now && was.version !== now.version) out.push(now.rel); + if ('error' in was || 'error' in now) return { error: 'error' in was ? was.error : (now as { error: string }).error }; + if (was.version !== now.version) out.push(now.rel); const npm = (rev: string) => { const r = git(root, ['show', `${rev}:package.json`]); try { return r.ok ? JSON.parse(r.out).version ?? null : null; } catch { return null; } }; if (!('rel' in now && now.rel === 'package.json') && npm(mergeBase) !== npm(pin)) out.push('package.json version'); if (policy.changelog && git(root, ['diff', '--quiet', mergeBase, pin, '--', policy.changelog]).ok === false) out.push(policy.changelog); @@ -61,6 +62,7 @@ export function landDirect(ctx: QueueContext, o: DirectOptions): StepOutput { const loaded = loadBasePolicy(ctx); if ('message' in loaded) return fail([], loaded); const policy = loaded.policy; + if (policy.merge === 'direct' && !policy.changelog_fragments) return fail([], { code: 'QUEUE_NOT_READY', message: 'merge: direct needs changelog_fragments: a PR carries its entry as a fragment, and ready records READY notes only under that key', fix: 'add "changelog_fragments": { "dir": "changelog.d" } to .gstack/ship-policy.json on the base' }); if (policy.merge !== 'direct') return fail([], { code: 'QUEUE_NOT_READY', message: `the base policy sets merge: ${policy.merge}; land --pin merges only under merge: direct`, fix: 'land through the train (gstack-ship-queue train build), or set "merge": "direct" in .gstack/ship-policy.json on the base' }); const info = view(ctx, o.pr); if ('code' in info) return fail([], info); @@ -88,7 +90,9 @@ export function landDirect(ctx: QueueContext, o: DirectOptions): StepOutput { if (lock.state === 'held') return fail(lines, { code: 'QUEUE_LAND_LOCKED', message: `${lockLabel(lock.lock)} holds the land lock; a direct land would race it onto main` }); const mergeBase = git(root, ['merge-base', `origin/${ctx.base}`, o.pin]).out; - const stamped = isSha(mergeBase) ? stampedFiles(root, policy, mergeBase, o.pin) : []; + if (!isSha(mergeBase)) return fail(lines, { code: 'QUEUE_NOT_READY', message: `cannot compute the merge base of the pin ${o.pin.slice(0, 12)} and origin/${ctx.base}, so the release-file check cannot run`, fix: `git fetch origin ${ctx.base} and the PR head (an unshallow clone), then rerun land` }); + const stamped = stampedFiles(root, policy, mergeBase, o.pin); + if ('error' in stamped) return fail(lines, { code: 'QUEUE_NOT_READY', message: `cannot read the version at the pin or its merge base: ${stamped.error}` }); if (stamped.length) { const files = stamped.filter(f => f !== 'package.json version'); const restore = files.length ? `git checkout $(git merge-base origin/${ctx.base} HEAD) -- ${files.join(' ')}` : ''; diff --git a/lib/ship-queue-land.ts b/lib/ship-queue-land.ts index 38ef98ca9..a564072ee 100644 --- a/lib/ship-queue-land.ts +++ b/lib/ship-queue-land.ts @@ -325,6 +325,7 @@ export function land(ctx: QueueContext, o: LandOptions): StepOutput { if ('message' in loaded) return fail([], loaded, run); const policy = loaded.policy; if (o.repair) return repair(ctx, policy, readJson(file), file, run); + if (policy.merge === 'direct') return fail([], { code: 'QUEUE_NOT_READY', message: 'the base policy sets merge: direct, where land never stamps', fix: `gstack-ship-queue land --pr ${o.pr} --pin ` }, run); const info = prInfo(ctx, o.pr); if ('code' in info) return fail([], info, run); if (info.isCrossRepository) return fail([], { code: 'QUEUE_TRAIN_UNTRUSTED', message: `#${o.pr} is a fork head; land stamps and pushes only crew branches` }, run); diff --git a/scripts/lib/windows-curation.ts b/scripts/lib/windows-curation.ts index 569a09462..dc82a023c 100644 --- a/scripts/lib/windows-curation.ts +++ b/scripts/lib/windows-curation.ts @@ -51,6 +51,7 @@ const WINDOWS_FRAGILE_PATTERNS: Array<{ pattern: RegExp; reason: string }> = [ // when possible; this list is for environment-/runtime-specific tests where // the failure mode is structural rather than detectable via source-file scan. export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }> = [ + { file: 'test/ship-queue-land-direct.test.ts', reason: 'the same extensionless node gh shim as test/ship-queue-land.test.ts; windows-latest launches the runner\'s gh.exe instead (no GH_TOKEN), so every land case fails (windows-scoped-shard, run 41720760327); land is a controller command run on Linux and macOS' }, { file: 'test/ship-queue-land.test.ts', reason: 'its gh shim is an extensionless #!/usr/bin/env node script on a colon-joined PATH, which windows-latest cannot launch, so land reaches the real gh (no GH_TOKEN) and every case fails (windows-free-shard 6, run 40824965988); land is a controller command run on Linux and macOS' }, // Like test/paid-rerun.test.ts (excluded by pattern), it replays a gate capture through the paid lane's --report and --rerun-plan CLI. { file: 'test/shared-libs-index-flags-readonly.test.ts', reason: 'its assume-unchanged replay seeds the review through test/helpers/shared-libs-path-fixture.ts, which launches bin/gstack-review-log (a bash script) directly; windows-latest refuses it as not found in $PATH (windows-free-shard 5, run 40727825859)' }, diff --git a/test/ship-queue-land-direct.test.ts b/test/ship-queue-land-direct.test.ts index 058f70adc..bae403c9c 100644 --- a/test/ship-queue-land-direct.test.ts +++ b/test/ship-queue-land-direct.test.ts @@ -271,6 +271,60 @@ describe('land --pin: refusals (nothing merged)', () => { }, 90_000); }); +describe('land --pin: fail closed and merge methods', () => { + test('the old stamping land refuses under merge: direct, and a --pin with no sha is a usage error', () => { + const fx = setup(); + const pin = openPr(fx, '23'); + const old = runBin(fx.repo, 'gstack-ship-queue', ['land', '--pr', '23', '--base', 'main', '--no-wait'], fx.env); + expect(old.status).not.toBe(EXIT.ok); + expect(old.err).toContain('merge: direct'); + expect(old.err).toContain('land --pr 23 --pin'); + expect(remote(fx, 'pr-23')).toBe(pin); + const dangling = runBin(fx.repo, 'gstack-ship-queue', ['land', '--pr', '23', '--base', 'main', '--pin'], fx.env); + expect(dangling.status).toBe(EXIT.usage); + expect(fx.read().mergeArgs).toBeUndefined(); + }, 90_000); + + test('a pin with no merge base on the base branch refuses instead of skipping the release-file check', () => { + const fx = setup(); + const { repo } = fx; + repo.git('checkout', '-q', '--orphan', 'pr-24'); + fs.writeFileSync(path.join(repo.work, 'VERSION'), '9.9.9.9\n'); + repo.git('add', '-A'); + repo.git('commit', '-q', '-m', 'unrelated history'); + repo.git('push', '-q', '-f', 'origin', 'HEAD:refs/heads/pr-24'); + const pin = repo.git('rev-parse', 'HEAD'); + fx.write(d => { d.prs['24'] = { state: 'OPEN', baseRefName: 'main', headRefName: 'pr-24', isCrossRepository: false, title: 'PR 24', body: shippedBody('24', pin) }; }); + markReady(fx, '24', pin); + repo.git('checkout', '-q', '--detach', 'origin/main'); + const r = land(fx, '24', pin); + expect(r.status).not.toBe(EXIT.ok); + expect(r.err).toContain('cannot compute the merge base'); + expect(fx.read().mergeArgs).toBeUndefined(); + }, 90_000); + + test('history: merge-only merges with --merge; a PR on another base refuses', () => { + const fx = setup({ ...POLICY, history: 'merge-only' }); + const pin = openPr(fx, '25'); + fx.write(d => { d.prs['25'].baseRefName = 'dev'; }); + const other = land(fx, '25', pin); + expect(other.err).toContain('targets dev, not main'); + expect(fx.read().mergeArgs).toBeUndefined(); + fx.write(d => { d.prs['25'].baseRefName = 'main'; }); + const r = land(fx, '25', pin); + expect(r.err).toBe(''); + expect(fx.read().mergeArgs).toEqual(['25', '--merge', '--match-head-commit', pin]); + }, 90_000); + + test('merge: direct without changelog_fragments refuses with the key to add', () => { + const fx = setup({ ...POLICY, changelog_fragments: null }); + const pin = openPr(fx, '26'); + const r = land(fx, '26', pin); + expect(r.err).toContain('merge: direct needs changelog_fragments'); + expect(fx.read().mergeArgs).toBeUndefined(); + }, 90_000); +}); + describe('merge: direct | train', () => { test('train build refuses under merge: direct and names land', () => { const fx = setup();