From 3909acadd79e1cc3f40193843151d67857be7fb2 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 16:12:57 +0000 Subject: [PATCH] train forget: restore the local notes ref on a rejected lease and refuse every in-flight state forgetNotes removed the note locally before the leased push, so a rejected lease left a local notes ref without the note that the next plain persist pushed without a lease. It now snapshots the ref and restores it on rejection, reporting failed removals separately. forget refused only merging/gate-pending/free-pending; built, pushed, free-green, dispatch-unknown and dispatch-absent are in flight too and are refused the same way. CHANGELOG and ship-policy say so. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/ship-policy.md | 5 ++++- lib/ship-queue-train-forget.ts | 14 +++++++++----- lib/ship-queue-train-record.ts | 15 ++++++++++----- test/ship-queue-train-ops.test.ts | 29 ++++++++++++++++++++++------- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77b2946ff..65636d2de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - **`QUEUE_SCREEN_RED`**: a RED screen refuses `ready` under `--screen` and `--no-screen` alike, is never re-dispatched at the same merge SHA, and `train build` refuses to seat the pin. **`QUEUE_SCREEN_NOT_RUN`**: a screen run that concluded success with no slice (its route job found the head superseded by a push, or the plan selected nothing) is `NOT-RUN`: `verify --screen` refuses, the receipt records `NOT_RUN`, `ready --screen` dispatches afresh instead of following it, and `train build` seats the pin unscreened with a `SCREEN: … counted as not-run` line. - **Scratch-merge screens (#3465)**: when GitHub's `refs/pull//merge` lags the PR head, is missing or cannot be fetched, `ready --screen` plans the screen on a scratch merge of the head onto the base, pushed as `screen/-` (`SCREEN_MERGE:` names it); only a real conflict with the base is `QUEUE_SCREEN_STALE_REF`, whose text now says so, and an unreachable remote is `POLICY_SOURCE_UNAVAILABLE`. - **`train watch --train [--timeout ]`** (`lib/ship-queue-train-watch.ts`): the one resumable follow command. It reads the record, runs the step its state asks for (`train build --train` to push, run the free tests and dispatch the gate; `train status` to settle an unconfirmed dispatch), waits for the pending run by polling gh, reads the verdict and prints the last step's `NEXT:`. No process state: rerun it after an interruption and it continues from the record; it never dispatches anything the record would not dispatch on its own. -- **`train prune --stale`**: a `merging` or `green` train whose every PR is MERGED or CLOSED on GitHub is marked `stale` (a new terminal state), its branch and the PRs' `screen/-*` branches deleted, each PR's state logged on a `STALE:` line; a train with an OPEN PR is kept and says why. **`train forget --train | --receipt @ [--dry-run]`** (`lib/ship-queue-train-forget.ts`) removes one settled train (attempts included) or one READY receipt from local state and from the notes ref with an exact `--force-with-lease` on `refs/notes/gstack-trains`; a merging, gate-pending or free-pending train is refused. +- **`train prune --stale`**: a `merging` or `green` train whose every PR is MERGED or CLOSED on GitHub is marked `stale` (a new terminal state), its branch and the PRs' `screen/-*` branches deleted, each PR's state logged on a `STALE:` line; a train with an OPEN PR is kept and says why. **`train forget --train | --receipt @ [--dry-run]`** (`lib/ship-queue-train-forget.ts`) removes one settled train (attempts included) or one READY receipt from local state and from the notes ref with an exact `--force-with-lease` on `refs/notes/gstack-trains`; a train still building, running or merging is refused, and a rejected lease removes nothing. - **`verify --post-merge [--timeout ]`** (`lib/ship-queue-post-merge.ts`): after a merge, waits for every check run attached to the merged commit to settle, prints one `CHECK: ` per check and `POST_MERGE: green|red|pending|none (passed= failed= pending= skipped=)`; green needs a completed check and no failure, `neutral`/`skipped` count as skipped, a failure is **`QUEUE_POST_MERGE_RED`**, a timeout is `QUEUE_TRAIN_PENDING` with the same command as `NEXT:`. - **`ready … --dispatch-ci [--timeout ]`** (`lib/ship-queue-dispatch-ci.ts`): for a PR that conflicts with the base and so has no pull_request CI, dispatches each named workflow on the branch (skipped when a run at this head already concluded), waits for the runs (`CI_DISPATCH:` lines), clears every requires-remote pregate lane whose workflow is one of them with the run URL (`PREGATE_CLEARED:`), then reads the head as usual. A red run clears no lane and refuses; a fork head is `QUEUE_TRAIN_UNTRUSTED`. This replaces the four-step hand procedure in docs/ship-policy.md. - **`train build --dry-run --stack-tests [--stack-tests-command ""]`** (`lib/ship-queue-stack-tests.ts`): runs each PR's new test files on the integrated prefix that includes it (T) in a scratch worktree and prints `STACK_TESTS: # T new test file(s) pass|FAIL ()` or `no new test files`; a failure is **`QUEUE_TRAIN_STACK_RED`** with nothing written, pushed or dispatched. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index e11fff06e..eada39397 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -575,7 +575,10 @@ PRs landed through another train or by hand): it logs each PR's state on a @ [--dry-run]` removes one record (a settled train with its attempts, or one READY receipt) from local state and from `refs/notes/gstack-trains` with an exact lease on the notes ref, printing `FORGET:` then `FORGOTTEN:`; a -merging, gate-pending or free-pending train is refused. +train that is still building, running or merging (`built`, `pushed`, +`free-pending`, `free-green`, `dispatch-unknown`, `dispatch-absent`, +`gate-pending`, `merging`) is refused; a rejected lease removes nothing and +restores the local notes ref. **Following a train without a daemon.** `gstack-ship-queue train watch --train [--timeout ]` is the one resumable follow command: it reads the diff --git a/lib/ship-queue-train-forget.ts b/lib/ship-queue-train-forget.ts index 8c2556291..99895e6b9 100644 --- a/lib/ship-queue-train-forget.ts +++ b/lib/ship-queue-train-forget.ts @@ -4,15 +4,19 @@ * refs/notes/gstack-trains under an exact lease, then delete the local files * (until now a stale record needed a hand-run * `git notes remove` plus a file hunt on every machine that ran the train). - * A rejected lease removes nothing. Other machines that still hold the local - * files can push the record back with their next write; the output says so. + * A rejected lease removes nothing (the local notes ref is restored too, so a + * later write cannot carry the removal without a lease). Other machines that + * still hold the local files can push the record back with their next write; + * the output says so. */ import * as fs from 'node:fs'; import type { QueueContext } from './ship-queue'; import { outOk, outRefused, trainEnv, type TrainOut } from './ship-queue-train'; -import { attemptsOf, forgetNotes, loadBisect, loadReady, loadTrain, noteName, readyPath, rebuildFromNotes, trainDir, type AnyRecord } from './ship-queue-train-record'; +import { attemptsOf, forgetNotes, loadBisect, loadReady, loadTrain, noteName, readyPath, rebuildFromNotes, trainDir, type AnyRecord, type TrainState } from './ship-queue-train-record'; const Q = 'gstack-ship-queue'; +/** States with work still in flight (a branch being built or pushed, free tests or a gate running, a merge under way): forget never settles them. */ +const LIVE_STATES: readonly TrainState[] = ['built', 'pushed', 'free-pending', 'free-green', 'dispatch-unknown', 'dispatch-absent', 'gate-pending', 'merging']; export function trainForget(ctx: QueueContext, o: { train?: string; receipt?: string; dryRun: boolean }): TrainOut { const env = trainEnv(ctx); @@ -24,8 +28,8 @@ export function trainForget(ctx: QueueContext, o: { train?: string; receipt?: st if (o.train) { const t = loadTrain(o.train); if (!t) return outRefused({ code: 'QUEUE_PROOF_MISSING', message: `no train record ${o.train} here or in refs/notes/gstack-trains` }, [], `${Q} train status`, 'none'); - if (t.state === 'merging' || t.state === 'gate-pending' || t.state === 'free-pending') { - return outRefused({ code: 'QUEUE_PROOF_MISSING', message: `train ${t.nonce} is ${t.state}; forget settles nothing that is still running or merging (train prune --stale for a train whose PRs merged elsewhere)` }, [], `${Q} train status --train ${t.nonce}`, trainDir(t.nonce)); + if (LIVE_STATES.includes(t.state)) { + return outRefused({ code: 'QUEUE_PROOF_MISSING', message: `train ${t.nonce} is ${t.state}; forget settles nothing that is still building, running or merging (train prune --stale for a train whose PRs merged elsewhere)` }, [], `${Q} train status --train ${t.nonce}`, trainDir(t.nonce)); } const bisect = loadBisect(t.nonce); records.push(t, ...attemptsOf(t), ...(bisect ? [bisect] : [])); diff --git a/lib/ship-queue-train-record.ts b/lib/ship-queue-train-record.ts index 4fcdfefb9..07e356791 100644 --- a/lib/ship-queue-train-record.ts +++ b/lib/ship-queue-train-record.ts @@ -241,21 +241,26 @@ function parseNote(text: string): AnyRecord | null { * (a rejected lease returns `pushed: false` and nothing is deleted locally). * Names are note names (noteName); a name with no note is reported missing. */ -export function forgetNotes(repoRoot: string, names: string[], remote = 'origin'): { removed: string[]; missing: string[]; pushed: boolean; lease: string | null; detail: string } { +export function forgetNotes(repoRoot: string, names: string[], remote = 'origin'): { removed: string[]; missing: string[]; failed: string[]; pushed: boolean; lease: string | null; detail: string } { const f = fetchNotes(repoRoot, remote); - if (!f.ok) return { removed: [], missing: [], pushed: false, lease: null, detail: f.detail }; + if (!f.ok) return { removed: [], missing: [], failed: [], pushed: false, lease: null, detail: f.detail }; const lease = git(repoRoot, ['rev-parse', '--verify', '--quiet', NOTES_REMOTE_REF]).out || null; + const before = git(repoRoot, ['rev-parse', '--verify', '--quiet', NOTES_REF]).out || null; const listed = git(repoRoot, ['notes', `--ref=${NOTES_REF}`, 'list']).out.split('\n').map(l => l.split(' ')[1]).filter(Boolean); const removed: string[] = []; const missing: string[] = []; + const failed: string[] = []; + const restore = () => { if (before) git(repoRoot, ['update-ref', NOTES_REF, before]); }; for (const name of names) { const blob = git(repoRoot, ['hash-object', '--stdin'], undefined, name).out; if (!listed.includes(blob)) { missing.push(name); continue; } - if (git(repoRoot, ['notes', `--ref=${NOTES_REF}`, 'remove', blob], notesEnv()).ok) removed.push(name); else missing.push(name); + if (git(repoRoot, ['notes', `--ref=${NOTES_REF}`, 'remove', blob], notesEnv()).ok) removed.push(name); else failed.push(name); } - if (!removed.length) return { removed, missing, pushed: false, lease, detail: 'no note to remove' }; + if (failed.length) { restore(); return { removed: [], missing, failed, pushed: false, lease, detail: `git notes remove failed for ${failed.join(', ')}; the local notes ref was restored` }; } + if (!removed.length) return { removed, missing, failed, pushed: false, lease, detail: 'no note to remove' }; const p = git(repoRoot, ['push', '-q', lease ? `--force-with-lease=${NOTES_REF}:${lease}` : `--force-with-lease=${NOTES_REF}`, remote, `${NOTES_REF}:${NOTES_REF}`]); - return { removed, missing, pushed: p.ok, lease, detail: p.ok ? `notes pushed to ${remote} ${NOTES_REF} (lease ${lease?.slice(0, 12) ?? 'new ref'})` : `git push ${NOTES_REF} rejected: ${p.err.replace(/\s+/g, ' ').trim().slice(0, 160)}` }; + if (!p.ok) restore(); + return { removed, missing, failed, pushed: p.ok, lease, detail: p.ok ? `notes pushed to ${remote} ${NOTES_REF} (lease ${lease?.slice(0, 12) ?? 'new ref'})` : `git push ${NOTES_REF} rejected: ${p.err.replace(/\s+/g, ' ').trim().slice(0, 160)}; the local notes ref was restored` }; } /** Rebuild local records from the notes ref: a note whose record is missing locally, or newer than the local copy, is written. */ diff --git a/test/ship-queue-train-ops.test.ts b/test/ship-queue-train-ops.test.ts index d1b39013b..7efa76aeb 100644 --- a/test/ship-queue-train-ops.test.ts +++ b/test/ship-queue-train-ops.test.ts @@ -223,7 +223,7 @@ describe('train prune --stale and train watch', () => { // Value: protects=a stale train record or READY receipt leaves refs/notes/gstack-trains through one command under an exact lease, with the local files, instead of a hand-run git notes remove on every machine; // fails_when=forget removes the wrong notes, deletes locally when the push failed, or forgets a merging train; why_new=no CLI removed notes (merge-train skill, known gaps); seam=none; no_claim=a concurrent writer's race (the lease is exercised only as a fast-forward) - test('train forget removes a settled train (attempts included) or a READY receipt from the notes ref and the state root, leaves other records alone, and refuses a merging train', () => { + test('train forget removes a settled train (attempts included) or a READY receipt from the notes ref and the state root, leaves other records alone, keeps everything (the local notes ref too) when the lease is rejected, and refuses a train still building, running or merging', () => { const fx = make(); const { nonce, h1 } = greenTrain(fx); expect(queue(fx, ['train', 'verdict', '--train', nonce]).status).toBe(EXIT.ok); @@ -266,12 +266,27 @@ describe('train prune --stale and train watch', () => { expect(queue(fx, ['train', 'forget']).status).toBe(EXIT.usage); const { nonce: second } = (() => { readyReceipt(fx, '3', h3); readyReceipt(fx, '4', h4); const r = queue(fx, ['train', 'build', '--prs', `3@${h3},4@${h4}`, '--free-timeout', '1']); return { nonce: nonceOf(r.out) }; })(); expect(queue(fx, ['train', 'verdict', '--train', second]).status).toBe(EXIT.ok); - const t = JSON.parse(fs.readFileSync(path.join(fx.state, 'ship-queue', 'trains', second, 'train.json'), 'utf8')); - t.state = 'merging'; - fs.writeFileSync(path.join(fx.state, 'ship-queue', 'trains', second, 'train.json'), JSON.stringify(t)); - const merging = queue(fx, ['train', 'forget', '--train', second]); - expect(merging.status).toBe(EXIT.refused); - expect(merging.err).toMatch(/is merging; forget settles nothing that is still running or merging .*\(QUEUE_PROOF_MISSING\)/); + const hook = path.join(fx.repo.origin, 'hooks', 'pre-receive'); + fs.mkdirSync(path.dirname(hook), { recursive: true }); + fs.writeFileSync(hook, '#!/bin/sh\nwhile read o n r; do case "$r" in refs/notes/*) echo "notes frozen" >&2; exit 1;; esac; done\nexit 0\n', { mode: 0o755 }); + const localNotes = fx.repo.git('rev-parse', 'refs/notes/gstack-trains'); + const rejected = queue(fx, ['train', 'forget', '--train', second]); + expect(rejected.status).toBe(EXIT.refused); + expect(rejected.err).toMatch(/the notes ref was not updated \(git push refs\/notes\/gstack-trains rejected: .*notes frozen.*; the local notes ref was restored\); nothing deleted locally.*\(QUEUE_PROOF_MISSING\)/); + expect(fs.existsSync(path.join(fx.state, 'ship-queue', 'trains', second, 'train.json'))).toBe(true); + expect(fx.repo.git('rev-parse', 'refs/notes/gstack-trains')).toBe(localNotes); + expect(fx.repo.git('notes', '--ref=refs/notes/gstack-trains', 'list').split('\n').length).toBeGreaterThan(0); + fs.rmSync(hook); + const trainFile = path.join(fx.state, 'ship-queue', 'trains', second, 'train.json'); + for (const state of ['dispatch-unknown', 'merging']) { + const t = JSON.parse(fs.readFileSync(trainFile, 'utf8')); + t.state = state; + fs.writeFileSync(trainFile, JSON.stringify(t)); + const live = queue(fx, ['train', 'forget', '--train', second]); + expect(live.status).toBe(EXIT.refused); + expect(live.err).toMatch(new RegExp(`is ${state}; forget settles nothing that is still building, running or merging .*\\(QUEUE_PROOF_MISSING\\)`)); + expect(fs.existsSync(trainFile)).toBe(true); + } }, 120_000); // Value: protects=a restarted controller re-arms with one command: watch continues from the record (free tests, gate dispatch, the pending run, the verdict) and prints NEXT;