From 0f6ca6f2541b44d8e6c69f464a0f5451bb124efe Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 27 Oct 2026 22:37:47 +0000 Subject: [PATCH] fix: pre-landing review fixes (runnable clear-time NEXT, stale exit only on a fetched main, open-slot freeze control, LF size-ledger fixture) - verify's clear-time NEXT names gh pr view --json state,headRefOid per unreadable slot instead of a pasteable #n@sha that views the current branch's PR - QUEUE_TRAIN_STALE needs a fetched main (never under --no-fetch) and reports a freeze restored from the notes ref instead of saying the queue is not frozen - freezeQueue docstring matches the unreadable field - new test: a train with a slot still OPEN freezes even when another train verified main's sha - test/size-ledger.test.ts fixture repo sets core.autocrlf false (Windows Free Tests shard 4 counted CRLF bytes) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/ship-policy.md | 9 ++++++--- docs/troubleshooting.md | 6 ++++-- lib/ship-queue-train-verify.ts | 24 ++++++++++++++---------- test/ship-queue-train.test.ts | 25 ++++++++++++++++++++++++- test/size-ledger.test.ts | 1 + 5 files changed, 49 insertions(+), 16 deletions(-) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 017879b45..d8b40e32c 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -645,15 +645,18 @@ record comes from the one GitHub read that decided to freeze, never a second one: a slot that read MERGED stays merged even when GitHub stops answering before the record is written, and a slot unreadable beside a merged one is left out of `unmerged`, recorded under `unreadable` and printed on an -`unreadable:` line; the `NEXT:` after the repair names it (`gh pr view -#@ (unreadable at the freeze); seat each that is OPEN in a train`). +`unreadable:` line; the `NEXT:` after the repair names it (`gh pr view +--json state,headRefOid (unreadable at the freeze: #@); seat each +that is OPEN in a train`). When every slot of the train already merged on GitHub and main's SHA is a prefix another train's verify confirmed (read after the notes ref, so a machine that never ran that train still sees it), the mismatch says nothing about this train: verify refuses with `QUEUE_TRAIN_STALE` and `NEXT: gstack-ship-queue train prune --stale --train `, freezing nothing and dispatching no backstop (train 24's record froze the queue that way on a main -train 28 had verified, #3519). When +train 28 had verified, #3519). Only a verify that fetched main takes this +exit: under `--no-fetch` the local main may be older than GitHub's, so the +mismatch freezes as before. When whether a slot merged on GitHub cannot be read, verify refuses (`QUEUE_PROOF_MISSING`, "whether # merged on GitHub could not be read") and asks to be rerun once `gh pr view` answers; an unreadable state is never diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 6ebf7e13b..7ff082041 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -4358,8 +4358,10 @@ did both on a main train 28 had verified, #3519). Other trains are read after the notes ref, so a machine that never ran the later train still sees it. **Fix.** `gstack-ship-queue train prune --stale --train ` settles the -record. When some slot is not merged, or no train verified main's SHA, verify -freezes as before (`QUEUE_TRAIN_FROZEN`). +record. When some slot is not merged, no train verified main's SHA, or verify +ran with `--no-fetch`, verify freezes as before (`QUEUE_TRAIN_FROZEN`). A queue +another machine already froze stays frozen; the refusal prints its freeze +lines. ### `QUEUE_TRAIN_FROZEN` diff --git a/lib/ship-queue-train-verify.ts b/lib/ship-queue-train-verify.ts index 0ee8de761..4d75b33ce 100644 --- a/lib/ship-queue-train-verify.ts +++ b/lib/ship-queue-train-verify.ts @@ -114,9 +114,10 @@ function published(env: TrainEnv, t: TrainRecord, i: number, main: string): { sa * anything remote, then the backstop gate is dispatched on main's sha. * `states` is the one GitHub read of every unverified slot, made by the * caller: a slot that read MERGED is merged, verified tree or not, and a slot - * that could not be read is left out of `unmerged` and named in the reason - * (nothing reads it again; the operator seats it by hand once gh answers), so - * the rebuild after the repair never seats a PR that may already be in. + * that could not be read is left out of `unmerged`, recorded under + * `unreadable` and named in the reason and in the NEXT after the repair (the + * operator seats it by hand once gh answers), so the rebuild after the repair + * never seats a PR that may already be in. */ async function freezeQueue(env: TrainEnv, t: TrainRecord, reason: string, mainSha: string, states: ReadonlyMap): Promise { const now = nowIso(); @@ -169,7 +170,7 @@ function verifiedElsewhere(env: TrainEnv, t: TrainRecord, mainSha: string): stri * plain refusal (nothing was published). A GitHub state that cannot be read * is neither: the verify refuses and asks to be rerun, never "nothing merged". */ -async function stopped(env: TrainEnv, t: TrainRecord, code: 'QUEUE_TRAIN_TREE_MISMATCH' | 'QUEUE_TRAIN_MOVED', message: string, mainSha: string, lines: string[]): Promise { +async function stopped(env: TrainEnv, t: TrainRecord, code: 'QUEUE_TRAIN_TREE_MISMATCH' | 'QUEUE_TRAIN_MOVED', message: string, mainSha: string, lines: string[], fetched: boolean): Promise { const run = trainFile(t.nonce); // One GitHub read per unverified slot decides both the freeze and its record. const states = new Map(t.slots.filter(s => !t.merged.some(m => m.pr === s.pr)).map(s => [s.pr, prState(env.ctx, s.pr)] as const)); @@ -177,8 +178,11 @@ async function stopped(env: TrainEnv, t: TrainRecord, code: 'QUEUE_TRAIN_TREE_MI const unreadable = landed ? [] : [...states].filter(([, state]) => state === 'unreadable').map(([pr]) => `#${pr}`); if (unreadable.length) return outRefused({ code: 'QUEUE_PROOF_MISSING', message: `${message}; whether ${unreadable.join(', ')} merged on GitHub could not be read, so whether the queue must freeze is unknown` }, lines, `${Q} verify --pr ${t.slots[0]!.pr} --train ${t.nonce} (once gh pr view answers)`, run); if (!landed) return outFailed({ code, message: `${message}; nothing of train ${t.nonce} merged, so the queue is not frozen` }, lines, `${Q} train build ${prsArgs(t.slots, t.explicit_order)} (a new train on current main)`, run); - const later = t.slots.every(s => t.merged.some(m => m.pr === s.pr) || states.get(s.pr) === 'MERGED') ? verifiedElsewhere(env, t, mainSha) : null; - if (later) return outRefused({ code: 'QUEUE_TRAIN_STALE', message: `${message}; every slot of train ${t.nonce} merged on GitHub and main ${mainSha.slice(0, 12)} is ${later}, so this record is stale and the queue is not frozen` }, lines, `${Q} train prune --stale --train ${t.nonce}`, run); + const later = fetched && t.slots.every(s => t.merged.some(m => m.pr === s.pr) || states.get(s.pr) === 'MERGED') ? verifiedElsewhere(env, t, mainSha) : null; + if (later) { + const held = activeFreeze(env.repo.keys); + return outRefused({ code: 'QUEUE_TRAIN_STALE', message: `${message}; every slot of train ${t.nonce} merged on GitHub and main ${mainSha.slice(0, 12)} is ${later}, so this record is stale and ${held ? `freezes nothing (the queue is already frozen by train ${held.train})` : 'the queue is not frozen'}` }, held ? [...lines, ...frozenLines(held)] : lines, `${Q} train prune --stale --train ${t.nonce}`, run); + } const f = activeFreeze(env.repo.keys) ?? await freezeQueue(env, t, message, mainSha, states); return outFailed({ code, message: `${message}; the queue is frozen` }, [...lines, ...frozenLines(f)], `${Q} train build --repair --prs @`, run); } @@ -210,13 +214,13 @@ export async function verifyTrain(ctx: QueueContext, o: { pr: string; train: str if (post.same) return confirmMerged(env, t, o.pr, main, after.index, [...lines, `main: ${main} = T${after.index} (${post.detail})`]); const pre = sameModuloStamps(env, before.commit, main); if (!pre.same) { - return stopped(env, t, 'QUEUE_TRAIN_TREE_MISMATCH', `main ${main.slice(0, 12)} is neither T${before.index} nor T${after.index} modulo stamps (vs T${after.index}: ${post.detail})`, main, lines); + return stopped(env, t, 'QUEUE_TRAIN_TREE_MISMATCH', `main ${main.slice(0, 12)} is neither T${before.index} nor T${after.index} modulo stamps (vs T${after.index}: ${post.detail})`, main, lines, !o.noFetch); } const headRef = o.noFetch ? '' : (git(ctx.repoRoot, ['fetch', '--no-tags', '-q', env.remote, `+refs/pull/${o.pr}/head:refs/gstack/queue/pr-${o.pr}`]).ok ? `refs/gstack/queue/pr-${o.pr}` : ''); const head = headRef ? git(ctx.repoRoot, ['rev-parse', headRef]).out : t.slots[i]!.head; const pinned = head === t.slots[i]!.head ? { same: true, detail: 'the pinned head' } : sameModuloStamps(env, t.slots[i]!.head, head); const stampOnly = pinned.same ? pinned : mergedIn(env, t, i, head, main); - if (!stampOnly.same) return stopped(env, t, 'QUEUE_TRAIN_MOVED', `#${o.pr} moved past its pinned head ${t.slots[i]!.head.slice(0, 12)} and is not T${after.index} modulo stamps (${head.slice(0, 12)}: ${stampOnly.detail}; against the pin it ${pinned.detail})`, main, lines); + if (!stampOnly.same) return stopped(env, t, 'QUEUE_TRAIN_MOVED', `#${o.pr} moved past its pinned head ${t.slots[i]!.head.slice(0, 12)} and is not T${after.index} modulo stamps (${head.slice(0, 12)}: ${stampOnly.detail}; against the pin it ${pinned.detail})`, main, lines, !o.noFetch); const info = prInfo(ctx, o.pr); if ('error' in info) return outRefused({ code: 'QUEUE_PROOF_MISSING', message: info.error }, lines, `${Q} verify --pr ${o.pr} --train ${t.nonce}`, run); const shipped = bindReceipt(info.body, o.pr, [t.slots[i]!.head, head]); @@ -241,8 +245,8 @@ function confirmMerged(env: TrainEnv, t: TrainRecord, pr: string, main: string, persist(env.ctx.repoRoot, [freeze], { remote: env.remote }); lines.push(`FREEZE: cleared (${freeze.cleared_by})`); const rebuild = freeze.unmerged.length ? `${Q} train build --prs ${pinList(freeze.unmerged)}` : ''; - const unread = (freeze.unreadable ?? []).map(u => `#${u.pr}@${u.head.slice(0, 12)}`); - const check = unread.length ? `gh pr view ${unread.join(', ')} (unreadable at the freeze); seat each that is OPEN in a train` : ''; + const unread = freeze.unreadable ?? []; + const check = unread.length ? `${unread.map(u => `gh pr view ${u.pr} --json state,headRefOid`).join(' && ')} (unreadable at the freeze: ${unread.map(u => `#${u.pr}@${u.head.slice(0, 12)}`).join(', ')}); seat each that is OPEN in a train` : ''; return outOk(lines, [rebuild, check].filter(Boolean).join('; then ') || 'none', run); } return outOk(lines, nextFor(t), run); diff --git a/test/ship-queue-train.test.ts b/test/ship-queue-train.test.ts index bb46fc718..3ad14c6a8 100644 --- a/test/ship-queue-train.test.ts +++ b/test/ship-queue-train.test.ts @@ -1092,7 +1092,7 @@ describe('verify, freeze, repair and clear', () => { publishMain(fx, trainJson(fx, rn).prefixes[1].commit, '1.2.6.0'); const cleared = queue(fx, ['verify', '--pr', '3', '--train', rn], {}, coord); expect(cleared.out).toMatch(/^FREEZE: cleared /m); - expect(next(cleared.out)).toBe(`NEXT: gh pr view #2@${h2.slice(0, 12)} (unreadable at the freeze); seat each that is OPEN in a train`); + expect(next(cleared.out)).toBe(`NEXT: gh pr view 2 --json state,headRefOid (unreadable at the freeze: #2@${h2.slice(0, 12)}); seat each that is OPEN in a train`); }, 300_000); // Value: protects=verify on a train whose every slot already merged, against a main another train verified, refuses as stale instead of freezing the queue and dispatching a backstop (#3519 c: train 24's record froze the queue at 9ccb5233, T1 of verified train 28, run 40586582766); fails_when=stopped() freezes when every slot merged and main's sha is another train's verified prefix, or refuses without that proof; why_new=every freeze test above has an unmerged or unverified main; seam=a second train record written into the state root; no_claim=what prune --stale then does (covered by the prune tests) @@ -1127,6 +1127,29 @@ describe('verify, freeze, repair and clear', () => { expect(line(frozen.out, 'QUEUE_TRAIN_FROZEN:')).toMatch(/^QUEUE_TRAIN_FROZEN: train \S+ merged #1,#2; main /); }, 300_000); + // Value: protects=a train with a slot still OPEN on GitHub freezes on a mismatch even when main's sha is another train's verified prefix; fails_when=the every-slot-merged guard before verifiedElsewhere is dropped or weakened, so a live train refuses as stale and skips the freeze; why_new=the test above only covers an all-merged train; seam=none; no_claim=prune behavior + test("a train with a slot still open freezes on the mismatch even when another train verified main's sha", () => { + const fx = make(); + const { nonce } = twoPrTrain(fx); + expect(queue(fx, ['train', 'verdict', '--train', nonce]).status).toBe(EXIT.ok); + const t = trainJson(fx, nonce); + const coord = coordinatorClone(fx.repo); + const v = (pr: string) => queue(fx, ['verify', '--pr', pr, '--train', nonce], {}, coord); + publishMain(fx, t.prefixes[1].commit, '1.2.4.0'); + expect(v('1').status).toBe(EXIT.ok); + const later = publishMain(fx, t.prefixes[2].commit, '1.2.5.0', { file: 'stray.txt', text: 'a later train\n' }); + const tree = fx.repo.git('rev-parse', `${later}^{tree}`); + const otherNonce = `${nonce}-later`; + const other = { ...t, nonce: otherNonce, state: 'merging', prefixes: [t.prefixes[0], { index: 1, commit: later, tree, pr: '9' }], merged: [{ pr: '9', sha: later, tree, at: '2026-10-18T13:00:00.000Z' }] }; + fs.mkdirSync(path.join(fx.state, 'ship-queue', 'trains', otherNonce), { recursive: true }); + fs.writeFileSync(path.join(fx.state, 'ship-queue', 'trains', otherNonce, 'train.json'), JSON.stringify(other)); + const partial = v('2'); + expect(partial.status).toBe(EXIT.fail); + expect(partial.err).not.toContain('(QUEUE_TRAIN_STALE)'); + expect(line(partial.out, 'QUEUE_TRAIN_FROZEN:')).toMatch(/^QUEUE_TRAIN_FROZEN: train \S+ merged #1; main /); + expect(fs.existsSync(path.join(fx.state, 'ship-queue', 'freeze', `${repoKey(fx)}.json`))).toBe(true); + }, 300_000); + // Value: protects=a post-merge CHANGELOG difference is excused only when main's parent is T modulo stamps, so a foreign commit between the verified head and the squash freezes; fails_when=published() skips or inverts the parent check; why_new=the two tests above reach the CHANGELOG rule only with a parent that is T1 modulo stamps; seam=none; no_claim=a parent differing from T1 in the CHANGELOG alone test("a post-merge CHANGELOG difference is not excused when main's parent is not the verified prefix modulo stamps: the queue freezes naming the parent", () => { const fx = make(); diff --git a/test/size-ledger.test.ts b/test/size-ledger.test.ts index 8bd65188b..26c2141d9 100644 --- a/test/size-ledger.test.ts +++ b/test/size-ledger.test.ts @@ -45,6 +45,7 @@ function mainRepo(): { root: string; main: string } { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'size-ledger-')); roots.push(root); git(root, 'init', '-q', '-b', 'main'); + git(root, 'config', 'core.autocrlf', 'false'); write(root, 'demo/sections/a.md', '# A\n'); write(root, 'demo/sections/b.md', '# B\n'); write(root, 'demo/SKILL.md', '## Preamble\n' + 'x'.repeat(MAIN_UNION - 8 - 13) + '\n');