From 95cf833708bbecab0e3335f23764a5daed263455 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 02:56:14 +0000 Subject: [PATCH] fix: close review-cycle gaps (a stamp must change a stamp file, result requires a full --head and fails closed without headRefOid, digits-only --pr) with tests for every cycle-3 branch Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- bin/gstack-ship-receipt | 6 ++++-- lib/ship-queue-train-ready.ts | 2 +- lib/ship-queue.ts | 3 ++- test/gstack-ship-queue.test.ts | 5 +++++ test/gstack-ship-receipt.test.ts | 32 +++++++++++++++++++++++++------- test/ship-queue-train.test.ts | 16 ++++++++++++++++ 7 files changed, 54 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c90a95e7a..50fefcca3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ - **Gate edit (size budgets):** `test/helpers/carve-guards.ts` raises ship's `maxSkeletonBytes` 81,000 → 81,600 and `maxSizeRatio` 1.6690 → 1.6740 for Step 22 and the Step 18 routing, offset in part by tightening Steps 14, 16, 18 and 20 prose (measured skeleton 81,535, union 314,027; the 2026-10-14 tightening had left 2 bytes of headroom). No threshold, trial count or case kind changed. ### Tests -- `test/ship-queue-train.test.ts`: READY, train build (dry run and live) and verify --train refuse a missing, invalid, other-head and other-PR receipt and admit the current one; the pinned-head receipt admits a stamp commit; verify refuses `QUEUE_PROOF_MISSING` when gh cannot read the PR; `--screen` refuses a lagging or missing merge ref and never follows a 0-case screen. The train fixture's `refs/pull//merge` is now a real merge commit (`mergeRefCommit`), as GitHub keeps it. `test/gstack-ship-queue.test.ts`: `ready` at a stamp commit (slot 1, and slot 2 with main merged in) accepts the pin's receipt; a bare merge of main with no stamp on top, or an edit on the pin side under the slot-2 stamp, is `QUEUE_RECEIPT_STALE`. `test/auq-error-fallback-hook.test.ts`: the unattended directive names the same `GSTACK_RESULT` writers as docs/unattended.md and `gstack-skill-start` (GSTA-91). `test/gstack-ship-receipt.test.ts`: the `bindReceipt` head-matching table (12-character floor, other PR, one-line `SHIP_RECEIPT:`), `result --body` refusals, `--head`, a non-git cwd and gh failures. The train fixture's `openPr` now writes a receipt for the head it opens (`shipped: false` leaves the body empty). `test/install-verdict.test.ts` pins the GSTA-85 fix in both twins; `test/gstack-skill-start.test.ts` pins the new unattended /ship block. +- `test/ship-queue-train.test.ts`: READY, train build (dry run and live) and verify --train refuse a missing, invalid, other-head and other-PR receipt and admit the current one; the pinned-head receipt admits a stamp commit; verify refuses `QUEUE_PROOF_MISSING` when gh cannot read the PR; `--screen` refuses a lagging or missing merge ref and never follows a 0-case screen. The train fixture's `refs/pull//merge` is now a real merge commit (`mergeRefCommit`), as GitHub keeps it. `test/gstack-ship-queue.test.ts`: `ready` at a stamp commit (slot 1, and slot 2 with main merged in) accepts the pin's receipt; a bare merge of main with no stamp on top, an empty commit over the merge, or an edit on the pin side under the slot-2 stamp, is `QUEUE_RECEIPT_STALE`. `gstack-ship-receipt result --pr ` refuses `QUEUE_RECEIPT_STALE` when HEAD is not the PR's pushed head and prints `status=incomplete` when gh returns no head or the run record cannot be written; `ready --screen` names a failed fetch of the merge ref instead of blaming a conflict. `test/auq-error-fallback-hook.test.ts`: the unattended directive names the same `GSTACK_RESULT` writers as docs/unattended.md and `gstack-skill-start` (GSTA-91). `test/gstack-ship-receipt.test.ts`: the `bindReceipt` head-matching table (12-character floor, other PR, one-line `SHIP_RECEIPT:`), `result --body` refusals, `--head`, a non-git cwd and gh failures. The train fixture's `openPr` now writes a receipt for the head it opens (`shipped: false` leaves the body empty). `test/install-verdict.test.ts` pins the GSTA-85 fix in both twins; `test/gstack-skill-start.test.ts` pins the new unattended /ship block. ## [1.91.224.0] - 2026-10-25 diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index a798e0922..ffe697f90 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -203,6 +203,7 @@ function cmdRead(rest: string[], json: boolean): number { function cmdResult(rest: string[], json: boolean): number { const bodyFile = flag(rest, '--body'); const pr = flag(rest, '--pr'); + if (pr !== undefined && !/^\d+$/.test(pr)) { process.stderr.write(`gstack-ship-receipt result: --pr must be a PR number, got ${JSON.stringify(pr.slice(0, 40))}\n`); return EXIT.usage; } const repoRoot = git(['rev-parse', '--show-toplevel']); const head = flag(rest, '--head') ?? (repoRoot ? git(['rev-parse', 'HEAD'], repoRoot) : undefined); const refused = (code: 'QUEUE_RECEIPT_MISSING' | 'QUEUE_RECEIPT_STALE', message: string) => { @@ -211,7 +212,7 @@ function cmdResult(rest: string[], json: boolean): number { return EXIT.refused; }; if (!head) { process.stderr.write('gstack-ship-receipt result: --head is required outside a git repository\n'); return EXIT.usage; } - if (!/^[0-9a-f]{12,64}$/i.test(head)) { process.stderr.write(`gstack-ship-receipt result: --head must be a hex commit id of at least 12 characters, got ${JSON.stringify(head.slice(0, 40))}\n`); return EXIT.usage; } + if (!/^[0-9a-f]{40,64}$/i.test(head)) { process.stderr.write(`gstack-ship-receipt result: --head must be a full hex commit id, got ${JSON.stringify(head.slice(0, 40))}\n`); return EXIT.usage; } const incomplete = (why: string) => { process.stderr.write(`gstack-ship-receipt result: ${why}\n`); process.stdout.write(json ? JSON.stringify({ skill: 'ship', status: 'incomplete', run: '-', message: why }) + '\n' : 'GSTACK_RESULT: skill=ship status=incomplete run=-\n'); @@ -229,7 +230,8 @@ function cmdResult(rest: string[], json: boolean): number { if (!r.ok) return incomplete(`gh pr view failed: ${r.err.trim()}`); let pushed: unknown; try { const v = JSON.parse(r.out); body = v.body ?? ''; pushed = v.headRefOid; } catch { return incomplete('gh returned no JSON body'); } - if (typeof pushed === 'string' && /^[0-9a-f]{12,64}$/i.test(pushed) && !pushed.toLowerCase().startsWith(head.toLowerCase())) { + if (typeof pushed !== 'string' || !/^[0-9a-f]{12,64}$/i.test(pushed)) return incomplete(`gh pr view returned no headRefOid for PR #${pr}`); + if (!pushed.toLowerCase().startsWith(head.toLowerCase())) { return refused('QUEUE_RECEIPT_STALE', `HEAD ${head.slice(0, 12)} is not PR #${pr}'s pushed head ${pushed.slice(0, 12)}; push it, then rerun gstack-ship-receipt result`); } source = `PR #${pr}${repo ? ` (${repo})` : ''}`; diff --git a/lib/ship-queue-train-ready.ts b/lib/ship-queue-train-ready.ts index 949665847..2dda070d0 100644 --- a/lib/ship-queue-train-ready.ts +++ b/lib/ship-queue-train-ready.ts @@ -118,7 +118,7 @@ export async function trainReady(ctx: QueueContext, o: ReadyOptions): Promise !!sha && git(root, ['merge-base', '--is-ancestor', sha, baseSha]).ok; const merge = git(root, ['rev-parse', '--verify', '--quiet', `${head}^1`]).out; - if (!merge || !stampOnly(merge, head)) return [head]; + const stamp = merge && merge !== head ? treeReceipt(ctx, policy, merge, head) : null; + if (!stamp || 'message' in stamp || stamp.tree !== 'same-modulo-stamps' || stamp.files.length === 0) return [head]; const parents = git(root, ['rev-list', '--parents', '-n', '1', merge]).out.split(' ').slice(1); if (parents.length !== 2) return [head]; const pinSide = onBase(parents[0]!) ? parents[1]! : onBase(parents[1]!) ? parents[0]! : ''; diff --git a/test/gstack-ship-queue.test.ts b/test/gstack-ship-queue.test.ts index 92961196f..0607a61f8 100644 --- a/test/gstack-ship-queue.test.ts +++ b/test/gstack-ship-queue.test.ts @@ -330,6 +330,11 @@ describe('ready', () => { const bare = queue(repo, ['ready', '--pr', '1', '--pregate', pregateFor(repo, bareMerge)], shim(bareMerge, shippedBody('1', pin))); expect(bare.status).toBe(EXIT.refused); expect(bare.err).toMatch(staleRe(bareMerge)); + repo.git('commit', '-q', '--allow-empty', '-m', 'empty commit is not a stamp'); + const emptyOnMerge = repo.git('rev-parse', 'HEAD'); + const empty = queue(repo, ['ready', '--pr', '1', '--pregate', pregateFor(repo, emptyOnMerge)], shim(emptyOnMerge, shippedBody('1', pin))); + expect(empty.status).toBe(EXIT.refused); + expect(empty.err).toMatch(staleRe(emptyOnMerge)); repo.git('reset', '-q', '--hard', pin); fs.appendFileSync(path.join(repo.work, 'pr-s.txt'), 'real change before the merge\n'); repo.git('commit', '-qam', 'edit on the pin side'); diff --git a/test/gstack-ship-receipt.test.ts b/test/gstack-ship-receipt.test.ts index ebdfb1deb..35c783381 100644 --- a/test/gstack-ship-receipt.test.ts +++ b/test/gstack-ship-receipt.test.ts @@ -154,9 +154,11 @@ describe('gstack-ship-receipt result (/ship terminal line, GBRA-108)', () => { const head = repo.git('rev-parse', 'HEAD'); const state = path.join(repo.root, 'state'); const env = { ...ghShim(repo, { - 5: { body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 5, head, base: 'main', queue_mode: 'stamp-at-merge' } as any) }, - 6: { body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 6, head: 'b'.repeat(40), base: 'main' } as any) }, - 7: { body: 'No receipt here.\n' }, + 5: { headRefOid: head, body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 5, head, base: 'main', queue_mode: 'stamp-at-merge' } as any) }, + 6: { headRefOid: head, body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 6, head: 'b'.repeat(40), base: 'main' } as any) }, + 7: { headRefOid: head, body: 'No receipt here.\n' }, + 9: { headRefOid: 'd'.repeat(40), body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 9, head, base: 'main' } as any) }, + 10: { body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 10, head, base: 'main' } as any) }, }), GSTACK_STATE_ROOT: state, GSTACK_SESSION_KIND: 'unattended' }; const ok = runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '5'], env); expect(ok.status).toBe(EXIT.ok); @@ -179,6 +181,13 @@ describe('gstack-ship-receipt result (/ship terminal line, GBRA-108)', () => { const missing = runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '7', '--json'], env); expect(missing.status).toBe(EXIT.refused); expect(JSON.parse(missing.out)).toMatchObject({ skill: 'ship', status: 'refused', run: '-', code: 'QUEUE_RECEIPT_MISSING' }); + const unpushed = runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '9'], env); + expect([unpushed.status, unpushed.out.trim()]).toEqual([EXIT.refused, 'GSTACK_RESULT: skill=ship status=refused run=-']); + expect(unpushed.err).toContain(`HEAD ${head.slice(0, 12)} is not PR #9's pushed head dddddddddddd; push it, then rerun gstack-ship-receipt result`); + const noOid = runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '10'], env); + expect([noOid.status, noOid.out.trim()]).toEqual([EXIT.fail, 'GSTACK_RESULT: skill=ship status=incomplete run=-']); + expect(noOid.err).toContain('gh pr view returned no headRefOid for PR #10'); + expect(runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '--repo'], env).status).toBe(EXIT.usage); const bodyFile = path.join(repo.work, '.gstack/tmp/ship-receipt.md'); fs.mkdirSync(path.dirname(bodyFile), { recursive: true }); @@ -189,6 +198,15 @@ describe('gstack-ship-receipt result (/ship terminal line, GBRA-108)', () => { expect(JSON.parse(fs.readFileSync(path.join(repo.root, 'run-out', 'ship-run.json'), 'utf8'))).toMatchObject({ head, pr: null, receipt_source: bodyFile, session_kind: 'unattended' }); expect(runBin(repo, 'gstack-ship-receipt', ['result', '--body', path.join(repo.root, 'absent.md')], env).status).toBe(EXIT.refused); expect(runBin(repo, 'gstack-ship-receipt', ['result'], env).status).toBe(EXIT.usage); + const relative = runBin(repo, 'gstack-ship-receipt', ['result', '--body', bodyFile, '--out', 'rel-out'], env); + expect(relative.status).toBe(EXIT.ok); + expect(relative.out.trim()).toBe(`GSTACK_RESULT: skill=ship status=complete run=${path.join(repo.work, 'rel-out')}`); + expect(fs.existsSync(path.join(repo.work, 'rel-out', 'ship-run.json'))).toBe(true); + const blocker = path.join(repo.root, 'not-a-dir'); + fs.writeFileSync(blocker, 'file\n'); + const unwritable = runBin(repo, 'gstack-ship-receipt', ['result', '--body', bodyFile, '--out', path.join(blocker, 'sub')], env); + expect([unwritable.status, unwritable.out.trim()]).toEqual([EXIT.fail, 'GSTACK_RESULT: skill=ship status=incomplete run=-']); + expect(unwritable.err).toContain(`cannot write the run record under ${path.join(blocker, 'sub')}`); expect(fs.readdirSync(path.join(state, 'ship', 'runs')).length).toBe(1); }); @@ -218,7 +236,7 @@ describe('gstack-ship-receipt result (/ship terminal line, GBRA-108)', () => { const invalid = runBin(repo, 'gstack-ship-receipt', ['result', '--body', invalidFile], env); expect([invalid.status, invalid.out.trim()]).toEqual([EXIT.refused, 'GSTACK_RESULT: skill=ship status=refused run=-']); expect(invalid.err).toMatch(/the receipt's ship receipt is invalid \(.*\).*\(QUEUE_RECEIPT_MISSING/); - const otherPrEnv = { ...env, ...ghShim(repo, { 8: { body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 99, head, base: 'main' } as any) } }) }; + const otherPrEnv = { ...env, ...ghShim(repo, { 8: { headRefOid: head, body: placeReceiptFirst('Prose.\n', { schema_version: 1, pr: 99, head, base: 'main' } as any) } }) }; const otherPr = runBin(repo, 'gstack-ship-receipt', ['result', '--pr', '8'], otherPrEnv); expect([otherPr.status, otherPr.out.trim()]).toEqual([EXIT.refused, 'GSTACK_RESULT: skill=ship status=refused run=-']); expect(otherPr.err).toContain("PR #8's ship receipt names pr=99, not this PR"); @@ -229,10 +247,10 @@ describe('gstack-ship-receipt result (/ship terminal line, GBRA-108)', () => { expect(JSON.parse(fs.readFileSync(path.join(repo.root, 'pinned-out', 'ship-run.json'), 'utf8')).head).toBe('c'.repeat(40)); const badHead = runBin(repo, 'gstack-ship-receipt', ['result', '--body', pinned, '--head', '../../etc'], env); expect([badHead.status, badHead.out]).toEqual([EXIT.usage, '']); - expect(badHead.err).toContain('--head must be a hex commit id of at least 12 characters'); - const shortHead = runBin(repo, 'gstack-ship-receipt', ['result', '--body', pinned, '--head', 'abcdef1'], env); + expect(badHead.err).toContain('--head must be a full hex commit id'); + const shortHead = runBin(repo, 'gstack-ship-receipt', ['result', '--body', pinned, '--head', head.slice(0, 12)], env); expect([shortHead.status, shortHead.out]).toEqual([EXIT.usage, '']); - expect(shortHead.err).toContain('--head must be a hex commit id of at least 12 characters'); + expect(shortHead.err).toContain('--head must be a full hex commit id'); const noGit = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-receipt-nogit-')); const outside = runBin(repo, 'gstack-ship-receipt', ['result', '--body', pinned], env, noGit); expect([outside.status, outside.err.trim()]).toEqual([EXIT.usage, 'gstack-ship-receipt result: --head is required outside a git repository']); diff --git a/test/ship-queue-train.test.ts b/test/ship-queue-train.test.ts index c301172f3..afbb98f04 100644 --- a/test/ship-queue-train.test.ts +++ b/test/ship-queue-train.test.ts @@ -12,6 +12,7 @@ * prune, ready's screen choice, and the policy gate. */ import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; @@ -215,6 +216,16 @@ describe('ready under a train policy: the screen choice', () => { const missing = queue(fx, ['ready', '--pr', '1', '--pregate', pregate2, '--screen']); expect(missing.status).toBe(EXIT.refused); expect(missing.err).toMatch(/has no refs\/pull\/1\/merge.*\(QUEUE_SCREEN_STALE_REF\)/); + const tree = repo.git('rev-parse', `${moved}^{tree}`); + const main = repo.git('rev-parse', 'origin/main'); + const badMerge = spawnSync('git', ['-C', repo.origin, 'hash-object', '-t', 'commit', '-w', '--stdin', '--literally'], { input: `tree ${tree}\nparent ${main}\nparent ${moved}\nauthor A 1700000000 +0000\ncommitter B 1700000000 +0000\n\nmerge without a committer email\n`, encoding: 'utf8' }).stdout.trim(); + spawnSync('git', ['-C', repo.origin, 'update-ref', 'refs/pull/1/merge', badMerge]); + repo.git('config', 'fetch.fsckObjects', 'true'); + const unfetchable = queue(fx, ['ready', '--pr', '1', '--pregate', pregate2, '--screen']); + expect(unfetchable.status).toBe(EXIT.refused); + expect(unfetchable.err).toMatch(/could not fetch refs\/pull\/1\/merge from origin: .*\(QUEUE_SCREEN_STALE_REF\)/); + expect(line(unfetchable.out, 'NEXT:')).toBe('NEXT: check the remote, then gstack-ship-queue ready --pr 1 --screen'); + repo.git('config', '--unset', 'fetch.fsckObjects'); expect(dispatches(fx)).toEqual([]); expect(fs.existsSync(path.join(fx.state, 'ship-queue', 'ready'))).toBe(false); repo.git('push', '-q', '-f', 'origin', `${mergeRefCommit(fx, moved)}:refs/pull/1/merge`); @@ -247,6 +258,11 @@ describe('ready under a train policy: the screen choice', () => { expect(followed.status).toBe(EXIT.ok); expect(line(followed.out, 'SCREEN_DISPATCH:')).toBe('SCREEN_DISPATCH: following run 102 (same merge SHA, base and policy; no duplicate)'); expect(dispatches(fx, 'evals.yml').length).toBe(3); + const liveBuild = queue(fx, ['train', 'build', '--prs', `1@${head}`, '--dry-run']); + expect([liveBuild.status, line(liveBuild.out, 'ORDER:'), line(liveBuild.out, 'SCREEN:')]).toEqual([EXIT.ok, 'ORDER: 1:#1(screened)', '']); + fx.write(d => { d.rules['evals.yml'] = { status: 'in_progress', conclusion: null }; }); + const runningBuild = queue(fx, ['train', 'build', '--prs', `1@${head}`, '--dry-run']); + expect([runningBuild.status, line(runningBuild.out, 'ORDER:'), line(runningBuild.out, 'SCREEN:')]).toEqual([EXIT.ok, 'ORDER: 1:#1(screened)', '']); }, 120_000); });