Make ship-queue ready refuse when its READY note doesn't reach origin (GST-2242) (#4254)

* Make ship-queue ready refuse when its READY note doesn't reach origin (GST-2242)

ready exited 0 and printed "notes pushed" even when the READY note never reached origin, so land refused the pin with QUEUE_NOT_READY (#4245). The notes writer now reads origin's ref back after each push, and train ready refuses with QUEUE_READY_NOTE_UNPUSHED when origin still lacks the note after three tries.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Name the changelog fragment for the branch (GST-2242)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Regenerate the doctor component list for the new ready result code (GST-2242)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Say that ready's notes check covers train policies, and how to rerun it with its gate flags (GST-2242)

Review-Finding: p1/red-team#2 reader=red-team
Review-Finding: p1/red-team#1 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Skip the ready notes-push test on Windows, where the gh shim can't run (GST-2242)

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

@@ -317,6 +317,7 @@ gstack_result_anchor() {
     QUEUE_SCREEN_NOT_RUN) printf '%s' 'queue-screen-not-run' ;;
     QUEUE_TRAIN_STACK_RED) printf '%s' 'queue-train-stack-red' ;;
     QUEUE_POST_MERGE_RED) printf '%s' 'queue-post-merge-red' ;;
+    QUEUE_READY_NOTE_UNPUSHED) printf '%s' 'queue-ready-note-unpushed' ;;
     QUEUE_SCREEN_RED) printf '%s' 'queue-screen-red' ;;
     READY_SCREEN_CHOICE_REQUIRED) printf '%s' 'ready-screen-choice-required' ;;
     PREGATE_TIMING_ASSERTION) printf '%s' 'pregate-timing-assertion' ;;

changelog.d/capy-make-ship-queue-ready.md

@@ -0,0 +1,4 @@
+---
+important: blocker
+---
+- **Under a merge-train policy, `gstack-ship-queue ready` no longer reports READY when its note never reached origin.** `land --pin` reads the READY note from origin's `refs/notes/gstack-trains`, but `ready` exited 0 even when it couldn't push that note, so a lane sent READY and the land was refused with `QUEUE_NOT_READY` (#4245 lost a land round this way). After pushing, `ready` now fetches origin's notes ref back and prints `notes pushed` only when origin holds the note. If three tries fail, or the push exits 0 and origin still lacks the note, it refuses with `QUEUE_READY_NOTE_UNPUSHED` and keeps the local receipt, so rerunning the same `ready` command once the push works records it (GST-2242).

crew/lib/ship-queue-train-ready.ts

@@ -174,6 +174,19 @@ async function dispatchScreen(env: TrainEnv, o: ReadyOptions, slug: string | nul
   return { choice: { choice: 'screen', policy, nonce, run: found.databaseId, merge_sha: mergeSha }, lines: [`SCREEN_DISPATCH: ${dispatch.workflow} run ${found.databaseId} ${found.url} nonce ${nonce}${notFollowing}`] };
 }
 
+/**
+ * Record the READY receipt and require origin to hold its note: `land --pin` reads the note on origin, so a receipt
+ * written only here is not READY (reports/GSTA-461.md item 19). The local receipt stays for the rerun to push.
+ */
+function recordReceipt(env: TrainEnv, receipt: ReadyReceipt, lines: string[], again: string): TrainOut | null {
+  const file = readyPath(env.repo.key, receipt.pr, receipt.head);
+  const p = persist(env.ctx.repoRoot, [receipt], { remote: env.remote });
+  lines.push(`READY_RECEIPT: ${file} (${p.detail})`);
+  if (p.pushed) return null;
+  return outRefused({ code: 'QUEUE_READY_NOTE_UNPUSHED', message: `the READY receipt for #${receipt.pr} at ${receipt.head.slice(0, 12)} is written locally, but ${env.remote} does not hold its note on refs/notes/gstack-trains (${p.detail}); land reads that note, so this head is not READY` },
+    lines, `fix the notes push to ${env.remote}, then ${again}`, file);
+}
+
 const screenLine = (c: ScreenChoice) => `screen: ${c.choice}${c.policy ? ` policy=${c.policy} nonce=${c.nonce} run=${c.run ?? '-'}${c.verdict ? ` verdict=${c.verdict.outcome}` : ''}` : ''}`;
 
 /**
@@ -214,8 +227,8 @@ function conditionalReady(env: TrainEnv, o: ReadyOptions, lines: string[]): Trai
   lines.unshift(`READY #${o.pr} ${head} CONDITIONAL`, 'screen: no-screen');
   lines.push(`BASE_RED: ${red.cases.join(', ')} base=${red.base.slice(0, 12)} fixer=#${base_red.fixer} expires=${red.expires_at} (CI and the pregate ran on the broken base; the train gates every prefix and lands this PR only through an update commit with green checks)`);
   if (o.dryRun) return outOk([...lines, 'DRY-RUN: no READY receipt written'], `${Q} ready --pr ${o.pr} --no-screen --base-red --fixer ${base_red.fixer}`, 'none');
-  const p = persist(ctx.repoRoot, [receipt], { remote: env.remote });
-  lines.push(`READY_RECEIPT: ${readyPath(env.repo.key, o.pr, head)} (${p.detail})`);
+  const refused = recordReceipt(env, receipt, lines, `${Q} ready --pr ${o.pr} --no-screen --base-red --fixer ${base_red.fixer}`);
+  if (refused) return refused;
   return outOk(lines, `send the READY block; the coordinator builds the train with fixer #${base_red.fixer} at slot 1: ${Q} train build --prs ${base_red.fixer}@<sha>,${o.pr}@${head}`, readyPath(env.repo.key, o.pr, head));
 }
 
@@ -299,8 +312,8 @@ export async function trainReady(ctx: QueueContext, o: ReadyOptions): Promise<Tr
   const receipt: ReadyReceipt = { schema: 1, kind: 'ready', repo_key: env.repo.key, pr: o.pr, head, base_sha: baseSha, ready_at: prior?.ready_at ?? now, screen: choice, updated_at: now, ...(o.fixesBaseRed ? { fixes_base_red: o.fixesBaseRed } : {}), ...(plan.record ? { plan: plan.record } : {}) };
   if (o.fixesBaseRed) lines.push(`FIXES_BASE_RED: ${o.fixesBaseRed.join(', ')} (T0 may be red on exactly these gate cases while this PR rides at slot 1)`);
   if (o.dryRun) return outOk([...lines, 'DRY-RUN: no READY receipt written, nothing dispatched'], `${Q} ready --pr ${o.pr} --${o.screen}${o.broad ? ' --broad' : ''}`, 'none');
-  const p = persist(ctx.repoRoot, [receipt], { remote: env.remote });
-  lines.push(`READY_RECEIPT: ${readyPath(env.repo.key, o.pr, head)} (${p.detail})`);
+  const refused = recordReceipt(env, receipt, lines, `${Q} ready --pr ${o.pr} --${o.screen}${o.broad ? ' --broad' : ''}${o.fixesBaseRed ? ` --fixes-base-red ${o.fixesBaseRed.join(',')}` : ''}`);
+  if (refused) return refused;
   return outOk(lines, `send the READY block; the coordinator builds the train: ${Q} train build --prs ${o.pr}@${head},...`, readyPath(env.repo.key, o.pr, head));
 }
 

crew/lib/ship-queue-train-record.ts

@@ -287,10 +287,25 @@ function addNote(repoRoot: string, r: AnyRecord): boolean {
   return git(repoRoot, ['notes', `--ref=${NOTES_REF}`, 'add', '-f', '-F', '-', blob.out], notesEnv(), JSON.stringify(r)).ok;
 }
 
+/**
+ * Whether the remote's notes ref holds the local notes tip: fetched back after
+ * a push, because a push can exit 0 without the ref moving (a proxy that drops
+ * the update), and a later writer's push may have moved it past the tip.
+ */
+function remoteHoldsNotes(repoRoot: string, remote: string): { ok: boolean; detail: string } {
+  const tip = git(repoRoot, ['rev-parse', '--verify', '--quiet', NOTES_REF]).out;
+  const f = git(repoRoot, ['fetch', '--no-tags', '-q', remote, `+${NOTES_REF}:${NOTES_REMOTE_REF}`]);
+  if (!f.ok) return { ok: false, detail: /couldn't find remote ref|no such ref/i.test(f.err) ? `the push exited 0 but ${remote} has no ${NOTES_REF}` : `${remote} ${NOTES_REF} could not be read back after the push: ${f.err.slice(0, 160)}` };
+  const fetched = git(repoRoot, ['rev-parse', '--verify', '--quiet', NOTES_REMOTE_REF]).out;
+  if (tip && (fetched === tip || git(repoRoot, ['merge-base', '--is-ancestor', tip, fetched]).ok)) return { ok: true, detail: '' };
+  return { ok: false, detail: `the push exited 0 but ${remote} ${NOTES_REF} is ${fetched.slice(0, 12) || 'absent'}, which does not hold the local notes ${tip.slice(0, 12)}` };
+}
+
 /**
  * Write records locally, then as notes, then push the notes ref (fetch,
- * merge and retry on a race). A failed push leaves the local records and
- * notes for the next write to carry, and says so.
+ * merge and retry on a race) and read origin back to confirm it holds them.
+ * A failed or unconfirmed push leaves the local records and notes for the
+ * next write to carry, and says so.
  */
 export function persist(repoRoot: string, records: AnyRecord[], opts: { push?: boolean; remote?: string } = {}): { pushed: boolean; detail: string } {
   for (const r of records) writeJsonAtomic(recordPath(r), r);
@@ -300,12 +315,13 @@ export function persist(repoRoot: string, records: AnyRecord[], opts: { push?: b
   let last = '';
   for (let i = 0; i < 3; i++) {
     const p = git(repoRoot, ['push', '-q', remote, `${NOTES_REF}:${NOTES_REF}`]);
-    if (p.ok) return { pushed: true, detail: `notes pushed to ${remote} ${NOTES_REF}` };
-    last = p.err.slice(0, 160);
+    const held = p.ok ? remoteHoldsNotes(repoRoot, remote) : null;
+    if (held?.ok) return { pushed: true, detail: `notes pushed to ${remote} ${NOTES_REF}` };
+    last = held ? held.detail : p.err.slice(0, 160);
     const f = fetchNotes(repoRoot, remote);
     if (!f.ok) return { pushed: false, detail: f.detail };
   }
-  return { pushed: false, detail: `git push ${NOTES_REF} failed: ${last}` };
+  return { pushed: false, detail: `git push ${NOTES_REF} failed after 3 tries: ${last}` };
 }
 
 function newerOrMissing(r: AnyRecord): boolean {

crew/test/ship-queue-ready-notes-push.test.ts

@@ -0,0 +1,82 @@
+/**
+ * `gstack-ship-queue ready` under a train policy records its READY receipt on
+ * origin's refs/notes/gstack-trains, which `land --pin` reads. When that push
+ * never reaches origin, ready must refuse instead of printing "notes pushed"
+ * and exiting 0 (reports/GSTA-461.md item 19: #4245's land refused
+ * QUEUE_NOT_READY because origin had no note).
+ */
+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 { cleanup, runBin } from '../../test/helpers/restamp-fixture';
+import { line, openPr, queue, repoKey, trainFixture, type TrainFixture } from '../../test/helpers/ship-queue-train-fixture';
+
+const NOTES = 'refs/notes/gstack-trains';
+const fixtures: TrainFixture[] = [];
+afterEach(() => { while (fixtures.length) cleanup(fixtures.pop()!.repo); });
+
+function readyFixture() {
+  const fx = trainFixture();
+  fixtures.push(fx);
+  const head = openPr(fx, '1');
+  fx.write(d => { d.ci[head] = [{ status: 'completed', conclusion: 'success' }]; });
+  const tree = runBin(fx.repo, 'gstack-wtree', []).out.trim();
+  const pregate = path.join(fx.repo.root, 'pregate.json');
+  fs.writeFileSync(pregate, JSON.stringify({ schema_version: 1, tree, head, verdict: 'pass', checks: [{ id: 'unit', status: 'pass' }] }));
+  return { fx, head, pregate };
+}
+
+function hook(fx: TrainFixture, name: 'pre-receive' | 'post-receive', body: string): void {
+  fs.writeFileSync(path.join(fx.repo.origin, 'hooks', name), `#!/bin/sh\n${body}\n`, { mode: 0o755 });
+}
+
+const remoteNotes = (fx: TrainFixture) => fx.repo.git('ls-remote', 'origin', NOTES).split(/\s+/)[0] ?? '';
+const localNotes = (fx: TrainFixture) => fx.repo.git('rev-parse', '--verify', '--quiet', NOTES);
+
+describe('ready refuses a READY note that never reached origin', () => {
+  // Value: protects=a lane never reports READY while origin lacks the note land reads; fails_when=ready ignores persist's pushed:false and exits 0 with "notes pushed"; why_new=no case pushes ready's note to a remote that rejects it; seam=a pre-receive hook in the fixture's bare origin; no_claim=GitHub's own rejection messages
+  test('origin rejects refs/notes/*: ready exits refused with QUEUE_READY_NOTE_UNPUSHED and prints no success', () => {
+    const { fx, head, pregate } = readyFixture();
+    hook(fx, 'pre-receive', 'while read old new ref; do case "$ref" in refs/notes/*) echo "notes refused by fixture" >&2; exit 1;; esac; done; exit 0');
+    const r = queue(fx, ['ready', '--pr', '1', '--pregate', pregate, '--no-screen']);
+    expect(r.status).toBe(EXIT.refused);
+    expect(r.err).toContain('(QUEUE_READY_NOTE_UNPUSHED)');
+    expect(r.out).not.toContain('notes pushed');
+    expect(line(r.out, 'GSTACK_RESULT:')).toContain('status=refused');
+    expect(line(r.out, 'NEXT:')).toContain('gstack-ship-queue ready --pr 1 --no-screen');
+    expect(remoteNotes(fx)).toBe('');
+    expect(fs.existsSync(path.join(fx.state, 'ship-queue', 'ready', repoKey(fx), `1-${head}.json`))).toBe(true);
+  }, 120_000);
+
+  // Value: protects=success is printed only after origin is read back holding the note; fails_when=a push that exits 0 without moving origin's ref passes as pushed; why_new=the case above covers a push git reports as failed; seam=a post-receive hook that deletes the ref after accepting it, standing in for a proxy that drops the update; no_claim=which proxy dropped #4245's push
+  test('a push that exits 0 but leaves origin without the note is refused too', () => {
+    const { fx, pregate } = readyFixture();
+    hook(fx, 'post-receive', `git update-ref -d ${NOTES} 2>/dev/null; exit 0`);
+    const r = queue(fx, ['ready', '--pr', '1', '--pregate', pregate, '--no-screen']);
+    expect(r.status).toBe(EXIT.refused);
+    expect(r.err).toContain('(QUEUE_READY_NOTE_UNPUSHED)');
+    expect(r.out).not.toContain('notes pushed');
+    expect(remoteNotes(fx)).toBe('');
+  }, 120_000);
+
+  // Value: protects=the normal push still succeeds and origin holds exactly the local notes tip; fails_when=the confirmation read refuses a push that did land; why_new=hold case for the two refusals above; seam=none; no_claim=a real GitHub remote
+  test('hold: a normal push succeeds, prints notes pushed, and origin holds the local notes tip', () => {
+    const { fx, pregate } = readyFixture();
+    const r = queue(fx, ['ready', '--pr', '1', '--pregate', pregate, '--no-screen']);
+    expect(r.status).toBe(EXIT.ok);
+    expect(line(r.out, 'READY_RECEIPT:')).toEndWith(`(notes pushed to origin ${NOTES})`);
+    expect(remoteNotes(fx)).toBe(localNotes(fx));
+  }, 120_000);
+
+  // Value: protects=rerunning ready once origin accepts notes pushes the kept receipt and succeeds; fails_when=the refusal's NEXT command cannot recover; why_new=the documented fix for QUEUE_READY_NOTE_UNPUSHED; seam=the pre-receive hook removed between runs; no_claim=none beyond the fixture
+  test('rerunning ready after origin accepts notes again pushes the note and succeeds', () => {
+    const { fx, pregate } = readyFixture();
+    hook(fx, 'pre-receive', 'exit 1');
+    expect(queue(fx, ['ready', '--pr', '1', '--pregate', pregate, '--no-screen']).status).toBe(EXIT.refused);
+    fs.rmSync(path.join(fx.repo.origin, 'hooks', 'pre-receive'));
+    const again = queue(fx, ['ready', '--pr', '1', '--pregate', pregate, '--no-screen']);
+    expect(again.status).toBe(EXIT.ok);
+    expect(remoteNotes(fx)).toBe(localNotes(fx));
+  }, 120_000);
+});

docs/ship-policy.md

@@ -790,7 +790,11 @@ narrow screen, except `setup`, which no paid run executes: a narrow screen
 derives it like any other file. The nonce key carries the purpose and
 the policy, so a repeat follows the same run and a broad screen never adopts
 a narrow one. The READY block gains `screen: …`, and the receipt is written
-locally and to the notes ref. `--dry-run` prints the dispatch without sending
+locally and to the notes ref. `READY_RECEIPT:` says `notes pushed` only after
+`ready` reads origin's notes ref back and finds the note; after three tries
+without it, `ready` refuses with
+[`QUEUE_READY_NOTE_UNPUSHED`](troubleshooting.md#queue-ready-note-unpushed),
+because `land` reads the note on origin. `--dry-run` prints the dispatch without sending
 it. Before dispatching, `ready` fetches the merge ref and requires its second
 parent to be the PR head and its first parent the current base: a missing,
 lagging or unfetchable merge ref (GitHub stops recomputing it while the PR

docs/troubleshooting.md

@@ -6502,6 +6502,27 @@ CHANGELOG drops or renames a released heading (the message then names the
 CHANGELOG). `train prune --stale` deletes the scratch branches of merged or
 closed PRs.
 
+<a id="queue-ready-note-unpushed"></a>
+### `QUEUE_READY_NOTE_UNPUSHED`
+
+**Meaning.** `gstack-ship-queue ready` wrote the READY receipt in the local
+state root, but origin does not hold its note on `refs/notes/gstack-trains`.
+`ready` pushes the notes ref, then fetches it back and requires origin's tip
+to be the local tip or to contain it. It tries three times, fetching and
+merging origin's notes between tries. Either the push failed (a rejected ref,
+credentials, the network) or it exited 0 and origin still read back without
+the note, as when a proxy drops the update. `land --pin` reads the READY note
+on origin and refuses the pin with `QUEUE_NOT_READY` without it, so `ready` refuses
+here instead of printing `notes pushed`. Before this check, ready exited 0 and
+the land failed instead (#4245). Don't send the READY block.
+
+**Fix.** The `READY_RECEIPT:` line ends with the push's error. Fix what
+refused it, then run the `NEXT:` line, which is the same `ready` command
+without `--pregate`, `--gate-exit` or `--gate-log`: add back any of those you
+passed the first time. It keeps the first `ready_at`, follows a screen run already dispatched at the
+same merge SHA, and pushes the kept receipt. To see the raw error, run
+`git push origin refs/notes/gstack-trains:refs/notes/gstack-trains`.
+
 <a id="queue-screen-red"></a>
 ### `QUEUE_SCREEN_RED`
 

lib/result-codes-release.ts

@@ -314,6 +314,11 @@ export const RELEASE_RESULT_CODES = {
     summary: 'a push workflow on the merged commit (free tests, the Windows subset, the main backstop) concluded failure, timed out or was cancelled after the train merged it; a neutral or skipped check is not red',
     fix: 'read the red run (gstack-ship-queue verify --post-merge <sha> prints one CHECK: line per workflow), repair on a new PR or roll back the merge; gstack-ship-queue train status names the freeze if the backstop filed one',
   },
+  QUEUE_READY_NOTE_UNPUSHED: {
+    anchor: 'queue-ready-note-unpushed',
+    summary: 'ready wrote its READY receipt locally, but after three tries origin does not hold the note on refs/notes/gstack-trains (the push failed, or it exited 0 and origin read back without the note), so land would refuse the pin; the head is not READY',
+    fix: 'read the push error on the READY_RECEIPT line, fix what refused it (credentials, a hook or rule on refs/notes/*, the network), then rerun the printed gstack-ship-queue ready command, adding back any --pregate, --gate-exit or --gate-log you passed; it keeps ready_at and pushes the kept receipt',
+  },
   QUEUE_SCREEN_RED: {
     anchor: 'queue-screen-red',
     summary: 'the PR head’s dispatched screen run finished RED (its gate binding says so; a case that failed twice is real), so the head is not READY and a train will not seat it',

scripts/lib/windows-curation.ts

@@ -330,6 +330,10 @@ export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }>
     file: 'crew/test/ship-queue-train.test.ts',
     reason: 'drives bin/gstack-ship-queue against the stateful #!/bin/sh gh shim in test/helpers/ship-queue-train-fixture.ts; on windows-latest spawnSync(\'gh\') resolves the runner\'s gh.exe instead of the shim (windows-free-shard run 39152702056)',
   },
+  {
+    file: 'crew/test/ship-queue-ready-notes-push.test.ts',
+    reason: 'drives bin/gstack-ship-queue ready against the #!/bin/sh gh shim in test/helpers/ship-queue-train-fixture.ts, like crew/test/ship-queue-train.test.ts; on windows-latest spawnSync(\'gh\') resolves the runner\'s gh.exe instead (windows-scoped-shard run 42711409624)',
+  },
   {
     file: 'crew/test/ship-queue-train-bisect.test.ts',
     reason: 'split from crew/test/ship-queue-train.test.ts (GSTA-151): drives bin/gstack-ship-queue against the same #!/bin/sh gh shim in test/helpers/ship-queue-train-fixture.ts',