From 2d0d8bed5d9196c61d1a5d52300d8642760650f7 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:16:08 +0000 Subject: [PATCH 1/2] fix(ship-queue): refuse a head whose required targeted review is missing or failed (GST-966) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-doctor-components.sh | 1 + changelog.d/gst-966-ready-targeted-missing.md | 7 ++++ docs/review-policy.md | 7 ++-- docs/ship-policy.md | 19 ++++++---- docs/troubleshooting.md | 17 +++++++++ lib/result-codes-release.ts | 5 +++ lib/ship-queue-land.ts | 2 +- lib/ship-queue-train.ts | 2 +- lib/ship-receipt-depth.ts | 36 ++++++++++++------- test/gstack-ship-queue.test.ts | 28 +++++++++++++++ test/review-targeted.test.ts | 29 ++++++++------- test/ship-receipt-depth.test.ts | 7 ++-- 12 files changed, 120 insertions(+), 40 deletions(-) create mode 100644 changelog.d/gst-966-ready-targeted-missing.md diff --git a/bin/gstack-doctor-components.sh b/bin/gstack-doctor-components.sh index 960129724..b20466f5f 100644 --- a/bin/gstack-doctor-components.sh +++ b/bin/gstack-doctor-components.sh @@ -308,6 +308,7 @@ gstack_result_anchor() { BASE_RED_WAIT_TIMEOUT) printf '%s' 'base-red-wait-timeout' ;; QUEUE_RECEIPT_DEPTH_RAISED) printf '%s' 'queue-receipt-depth-raised' ;; QUEUE_RECEIPT_EXCEPTION_INVALID) printf '%s' 'queue-receipt-exception-invalid' ;; + QUEUE_RECEIPT_TARGETED_MISSING) printf '%s' 'queue-receipt-targeted-missing' ;; QUEUE_SCREEN_STALE_REF) printf '%s' 'queue-screen-stale-ref' ;; QUEUE_SCREEN_NOT_RUN) printf '%s' 'queue-screen-not-run' ;; QUEUE_TRAIN_STACK_RED) printf '%s' 'queue-train-stack-red' ;; diff --git a/changelog.d/gst-966-ready-targeted-missing.md b/changelog.d/gst-966-ready-targeted-missing.md new file mode 100644 index 000000000..6fd176a27 --- /dev/null +++ b/changelog.d/gst-966-ready-targeted-missing.md @@ -0,0 +1,7 @@ +**The queue now refuses a head whose required targeted review is missing or failed (GST-966).** On gstack-evals#140, the receipt showed `REVIEW_TARGETED: required=auth receipt=legacy verdict=missing` and `gstack-ship-queue ready` still exited 0, so a skipped required reader looked the same as a clean one. + +### Fixed +- **`lib/ship-receipt-depth.ts`:** `ready`, `train build` and `land` refuse `QUEUE_RECEIPT_TARGETED_MISSING` when the head needs a targeted class (money, gate, auth or secret) that no done targeted pass in the receipt covers: no pass recorded, a failed or unrun pass, or a pass on other classes. The line says `verdict=failed` when the recorded pass failed. A head that needs no targeted class passes as before. `train build` and `land` print the `REVIEW_TARGETED:` line with the refusal. + +### Tests +- `test/gstack-ship-queue.test.ts`: `ready` on an auth head with #140's receipt shape (missing) and with a failed pass refuses with the new code; a done pass and a head that needs no class pass. On main the missing case exits 0. diff --git a/docs/review-policy.md b/docs/review-policy.md index 2cf6a453b..ba8c48f79 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -375,10 +375,11 @@ block without new floor words. The reader is read-only, and `NO FINDINGS` is a result. `gstack-review-dispatch render --role targeted --classes ` renders its brief; `verify --targeted-out ` writes the record that `gstack-ship-receipt write --review-targeted @` puts in the receipt. -`gstack-ship-queue ready` and `train build` print +`gstack-ship-queue ready`, `train build` and `land` print `REVIEW_TARGETED: required= receipt= -verdict=ok|missing` and refuse nothing yet; enforcement comes in a later -release. +verdict=ok|missing|failed` and refuse +[`QUEUE_RECEIPT_TARGETED_MISSING`](troubleshooting.md#queue-receipt-targeted-missing) +when a required class has no done pass covering it. Over the last 100 non-release commits on main (8663c3d2..8be7f213), money hit 21, gate 18, auth 3 and secret 8. The gate class first used the gate paths, diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 26f3ef71e..7e67efa9e 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -246,8 +246,10 @@ result is not a current pass, and (`QUEUE_RECEIPT_MISSING`, `QUEUE_RECEIPT_STALE`) when the PR body carries no /ship receipt for that head: /ship runs before every READY ([details](#ship-before-ready)). It also refuses `QUEUE_RECEIPT_DEPTH_RAISED` when the receipt's review depth is below -what the head needs and `QUEUE_RECEIPT_EXCEPTION_INVALID` when an owner -exception the receipt records does not stand ([the ship receipt](#ship-receipt)). +what the head needs, `QUEUE_RECEIPT_TARGETED_MISSING` when a targeted class +the head needs has no done targeted pass in the receipt, and +`QUEUE_RECEIPT_EXCEPTION_INVALID` when an owner exception the receipt records +does not stand ([the ship receipt](#ship-receipt)). Send the block as printed. **2. Lane behind another PR: gate ahead.** Gate the tree the queue will have @@ -683,8 +685,10 @@ slot. The queue enforces this at every step from the PR body alone: head being READY'd (or its `pr` is another PR); the informational `SHIP_RECEIPT:` line still prints. It refuses `QUEUE_RECEIPT_DEPTH_RAISED` (with a `REVIEW_DEPTH:` line) when the receipt records a review depth below - what the head needs, and `QUEUE_RECEIPT_EXCEPTION_INVALID` when an owner - exception it records does not stand ([the ship receipt](#ship-receipt)). + what the head needs, `QUEUE_RECEIPT_TARGETED_MISSING` (with a + `REVIEW_TARGETED:` line) when a targeted class the head needs has no done + targeted pass in the receipt, and `QUEUE_RECEIPT_EXCEPTION_INVALID` when an + owner exception it records does not stand ([the ship receipt](#ship-receipt)). There is no flag around it. - `train build` refuses a pin whose PR has no current receipt with the same codes, naming the PR, before anything is written, pushed or dispatched, @@ -803,9 +807,10 @@ its record. Refusals, all before any spend: `QUEUE_TRAIN_UNTRUSTED` (a fork head), `QUEUE_TRAIN_MOVED` (a head moved from its pin), `QUEUE_RECEIPT_MISSING` and `QUEUE_RECEIPT_STALE` (the PR body has no /ship receipt for the pin; see [/ship before READY](#ship-before-ready)), -`QUEUE_RECEIPT_DEPTH_RAISED` and `QUEUE_RECEIPT_EXCEPTION_INVALID` (the -receipt's review depth is below what the pin needs, or an owner exception it -records does not stand; see [the ship receipt](#ship-receipt)), +`QUEUE_RECEIPT_DEPTH_RAISED`, `QUEUE_RECEIPT_TARGETED_MISSING` and +`QUEUE_RECEIPT_EXCEPTION_INVALID` (the receipt's review depth is below what +the pin needs, a targeted class the pin needs has no done targeted pass, or an +owner exception it records does not stand; see [the ship receipt](#ship-receipt)), `QUEUE_NOT_READY` (no READY receipt at that head), `QUEUE_TRAIN_CONFLICT` (the printed `QUEUE_TRAIN_CONFLICT: train stopped at # (conflicts: ) after merging #,#; nothing dispatched, no branch pushed; fix: …` line names diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index d73967bb1..273d32573 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -5387,6 +5387,23 @@ body no longer carries the exception's line word for word /ship again and record the owner's acceptance with `--docs` or `--review-loop-exception` ([ship-policy.md](ship-policy.md#ship-receipt)). + +### `QUEUE_RECEIPT_TARGETED_MISSING` + +**Meaning.** `ready`, `train build` or `land` recomputed the targeted +classes the head needs (money, gate, auth, secret) from git, and the receipt's +`review_targeted` does not cover every one of them with a `done` pass. The +`REVIEW_TARGETED:` line prints the required classes, the classes a done pass +covered (`legacy` when the receipt records no targeted pass) and +`verdict=missing`, or `verdict=failed` when the recorded pass failed. A +head that needs no class passes whatever the receipt records. The receipt is +unchanged. + +**Fix.** Run /ship on the head the READY names so the targeted reader runs on +every required class and `gstack-ship-receipt write --review-targeted` records +a done pass ([review-policy.md](review-policy.md#targeted-pass-on-money-gate-auth-and-secret-code)). +Never edit `review_targeted` by hand. + ### `QUEUE_SCREEN_STALE_REF` diff --git a/lib/result-codes-release.ts b/lib/result-codes-release.ts index 3797aeae2..06358082f 100644 --- a/lib/result-codes-release.ts +++ b/lib/result-codes-release.ts @@ -264,6 +264,11 @@ export const RELEASE_RESULT_CODES = { summary: 'the PR body’s /ship receipt records an owner exception (docs or review loop) that is malformed, lacks the fix-check it follows, or whose line the body no longer carries verbatim', fix: 'restore the exception line in the PR body word for word, or run /ship again and record the owner’s acceptance with --docs or --review-loop-exception', }, + QUEUE_RECEIPT_TARGETED_MISSING: { + anchor: 'queue-receipt-targeted-missing', + summary: 'the head touches money, gate, auth or secret code that needs the targeted reader, and the PR body’s /ship receipt records no done targeted pass covering every required class (none recorded, failed, not run, or other classes)', + fix: 'run /ship on the head the READY names so the targeted reader covers every class the REVIEW_TARGETED line lists as required; the receipt is left unchanged', + }, QUEUE_SCREEN_STALE_REF: { anchor: 'queue-screen-stale-ref', summary: 'GitHub’s refs/pull//merge is missing or still merges an older head than the PR’s: the PR conflicts with the base (a CHANGELOG-only conflict counts), so a screen dispatched on it would plan no cases and report success', diff --git a/lib/ship-queue-land.ts b/lib/ship-queue-land.ts index dd8d22d31..7047579e3 100644 --- a/lib/ship-queue-land.ts +++ b/lib/ship-queue-land.ts @@ -339,7 +339,7 @@ export function land(ctx: QueueContext, o: LandOptions): StepOutput { const lines = [`LAND: #${o.pr} branch=${info.headRefName} pin=${pin}${j ? ` (resuming at ${j.phase})` : recovered ? ` (the head ${info.headRefOid.slice(0, 12)} is land's stamp commit; restamping from its pin)` : ''}`]; git(root, ['fetch', '-q', 'origin', ctx.base]); const receipt = bindShippedReceipt(root, `origin/${ctx.base}`, info.body ?? '', o.pr, [pin]); - lines.push(receipt.line, ...(receipt.depthLine ? [receipt.depthLine] : [])); + lines.push(receipt.line, ...(receipt.depthLine ? [receipt.depthLine] : []), ...(receipt.targetedLine ? [receipt.targetedLine] : [])); if (!receipt.ok) return fail(lines, { code: receipt.code, message: receipt.message, fix: 'run /ship at the pinned head first' }, run); if (!policy.changelog_fragments) return fail(lines, { code: 'QUEUE_NOT_READY', message: 'the base policy has changelog_fragments off; land merges only under the key, where ready records the READY receipt it checks' }, run); const keys = repoIdentity(root).keys; diff --git a/lib/ship-queue-train.ts b/lib/ship-queue-train.ts index b589ddfa9..2cc453be2 100644 --- a/lib/ship-queue-train.ts +++ b/lib/ship-queue-train.ts @@ -367,7 +367,7 @@ function admitSlots(env: TrainEnv, pins: Array<{ pr: string; sha: string }>, dry } const ready = loadReady(env.repo.keys, p.pr, info.headRefOid); const shipped = bindTrainReceipt(ctx.repoRoot, `refs/remotes/${env.remote}/${ctx.base}`, info.body, p.pr, [info.headRefOid], !!ready?.base_red); - if (!shipped.ok) return outRefused({ code: shipped.code, message: shipped.message }, [shipped.line, ...(shipped.depthLine ? [shipped.depthLine] : [])], `#${p.pr}'s lane runs /ship, then ${Q} ready --pr ${p.pr} --screen|--no-screen; then ${Q} train build --prs `, 'none'); + if (!shipped.ok) return outRefused({ code: shipped.code, message: shipped.message }, [shipped.line, ...(shipped.depthLine ? [shipped.depthLine] : []), ...(shipped.targetedLine ? [`${shipped.targetedLine} pr=${p.pr}`] : [])], `#${p.pr}'s lane runs /ship, then ${Q} ready --pr ${p.pr} --screen|--no-screen; then ${Q} train build --prs `, 'none'); lines.push(`${shipped.targetedLine} pr=${p.pr}`); if (!ready) return outRefused({ code: 'QUEUE_NOT_READY', message: `no READY receipt for #${p.pr} at ${info.headRefOid.slice(0, 12)} (local state or ${'refs/notes/gstack-trains'})` }, [], `the lane runs ${Q} ready --pr ${p.pr} --screen|--no-screen`, 'none'); let screen = ready.screen; diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index ad8933cfb..7240a49f6 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -13,8 +13,8 @@ * The queue recomputes the depth the head needs from git objects * (lib/review-collect.ts) and refuses a receipt below it. * `review_targeted`: the targeted pass on money, gate, auth and secret code - * (lib/review-targeted.ts), which the queue reports beside the depth and does - * not refuse on yet (GST-257 PR 1). Kept beside + * (lib/review-targeted.ts); the queue reports it beside the depth and refuses + * a head whose required classes the pass did not cover (GST-966). Kept beside * lib/ship-receipt.ts so that module stays under the size ratchet. */ import { spawnSync } from 'node:child_process'; @@ -143,16 +143,25 @@ export function parseReviewTargeted(value: unknown, productPatchId: string): { o return { ok: true, targeted: { classes, rules: v.rules as string, brief_sha: v.brief_sha, product_patch_id: productPatchId, status: v.status as ReviewTargeted['status'], findings: v.findings, raised_by: (v.raised_by as string).trim() } }; } -/** The queue's report-only line: the classes the head needs against the classes a done targeted pass covered; never a refusal (GST-257 PR 1). */ -export function receiptTargetedLine(required: readonly TargetedClass[], r: ShipReceipt): string { +export interface DepthVerdict { ok: boolean; line: string; message?: string } + +/** + * The classes the head needs against the classes a done targeted pass covered; a required class left uncovered (no pass, + * a failed or unrun pass, or a pass on other classes) is refused (GST-966). A receipt with neither gstack_version nor + * review_depth predates targeted receipts, as receiptDepthVerdict treats it, and is accepted as legacy. + */ +export function receiptTargetedVerdict(required: readonly TargetedClass[], r: ShipReceipt): DepthVerdict { const rt = r.review_targeted; const covered: readonly string[] = rt?.status === 'done' ? rt.classes : []; - const ok = required.every(c => covered.includes(c)); - return `REVIEW_TARGETED: required=${classList(required)} receipt=${rt ? classList(covered) : 'legacy'} verdict=${ok ? 'ok' : 'missing'}`; + const missing = required.filter(c => !covered.includes(c)); + const verdict = !missing.length ? 'ok' : rt?.status === 'failed' ? 'failed' : 'missing'; + const line = `REVIEW_TARGETED: required=${classList(required)} receipt=${rt ? classList(covered) : 'legacy'} verdict=${verdict}`; + if (!missing.length) return { ok: true, line }; + if (!rt && !r.review_depth && !r.gstack_version) return { ok: true, line: `${line} (legacy receipt, written before receipts recorded review_targeted; accepted)` }; + const why = !rt ? 'records no targeted pass' : rt.status === 'failed' ? 'records that its targeted pass failed' : rt.status === 'not-run' ? 'records that its targeted pass did not run' : `covers ${classList(covered)} only`; + return { ok: false, line, message: `the head requires the targeted reader on ${missing.join(', ')}, but the receipt ${why}` }; } -export interface DepthVerdict { ok: boolean; line: string; message?: string } - /** The head's committed range against the base, read once for the depth verdict and the targeted line. */ interface HeadInputs { baseSha: { ok: boolean; out: string }; c: ReturnType; scope: Record } function headInputs(root: string, baseRef: string, head: string): HeadInputs { @@ -189,10 +198,10 @@ export function receiptDepthVerdict(root: string, baseRef: string, r: ShipReceip return { ok: false, line, message: `the receipt ${why}, but head ${head.slice(0, 12)} needs ${need.depth} (${need.reasons.join(', ')}${errs ? `; ${errs}` : ''}); the receipt is unchanged; run /ship on ${head.slice(0, 12)}` }; } -/** bindReceipt plus the owner exceptions and the review depth: what READY and train build accept; the targeted line is reported, never refused on. */ +/** bindReceipt plus the owner exceptions, the review depth and the targeted pass: what READY, train build and land accept. */ export function bindShippedReceipt(root: string, baseRef: string, body: string, pr: string | undefined, accepted: readonly string[]): | { ok: true; receipt: ShipReceipt; head: string; line: string; depthLine: string; targetedLine: string } - | { ok: false; code: ReceiptBindingCode | 'QUEUE_RECEIPT_DEPTH_RAISED' | 'QUEUE_RECEIPT_EXCEPTION_INVALID'; message: string; line: string; depthLine?: string; targetedLine?: string } { + | { ok: false; code: ReceiptBindingCode | 'QUEUE_RECEIPT_DEPTH_RAISED' | 'QUEUE_RECEIPT_EXCEPTION_INVALID' | 'QUEUE_RECEIPT_TARGETED_MISSING'; message: string; line: string; depthLine?: string; targetedLine?: string } { const b = bindReceipt(body, pr, accepted); if (!b.ok) return b; const ancestor = (commit: string) => b.receipt.carry?.ancestry === 'rewritten' || git(root, ['merge-base', '--is-ancestor', commit, b.head]).ok; @@ -201,8 +210,9 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, const at = headInputs(root, baseRef, b.head); const d = receiptDepthVerdict(root, baseRef, b.receipt, b.head, at); const gateTagged = boundScan(b.receipt, b.head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0); - const targetedLine = receiptTargetedLine(targetedClasses({ ...at.c, scope: at.scope, gateTagged }).classes, b.receipt); - if (!d.ok) return { ok: false, code: 'QUEUE_RECEIPT_DEPTH_RAISED', message: d.message!, line: b.line, depthLine: d.line, targetedLine }; - return { ...b, depthLine: d.line, targetedLine }; + const t = receiptTargetedVerdict(targetedClasses({ ...at.c, scope: at.scope, gateTagged }).classes, b.receipt); + if (!d.ok) return { ok: false, code: 'QUEUE_RECEIPT_DEPTH_RAISED', message: d.message!, line: b.line, depthLine: d.line, targetedLine: t.line }; + if (!t.ok) return { ok: false, code: 'QUEUE_RECEIPT_TARGETED_MISSING', message: `${t.message}; the receipt is unchanged; run /ship on ${b.head.slice(0, 12)}`, line: b.line, depthLine: d.line, targetedLine: t.line }; + return { ...b, depthLine: d.line, targetedLine: t.line }; } diff --git a/test/gstack-ship-queue.test.ts b/test/gstack-ship-queue.test.ts index 7de60fa39..71657bb2c 100644 --- a/test/gstack-ship-queue.test.ts +++ b/test/gstack-ship-queue.test.ts @@ -361,6 +361,34 @@ describe('ready', () => { expect(queue(repo, ['ready', '--pr', '1'], shim(moved, green)).err).toMatch(/uncommitted tracked changes: pr-a\.txt.*\(QUEUE_NOT_READY\)/); }, 180_000); + // Value: protects=a head whose required targeted reader is missing or failed is never READY (GST-966, gstack-evals#140); + // fails_when=ready prints REVIEW_TARGETED verdict=missing or failed and still exits 0, or refuses a covered or unrequired head; + // seam=bindShippedReceipt (ready, train build and land share it); no_claim=the ledger recording a clean reader as failed (GSTA-167) + test('ready refuses QUEUE_RECEIPT_TARGETED_MISSING when a required targeted class is missing or failed, and passes a covered head', () => { + const repo = fixture(); + const head = makePr(repo, 'pr-auth', { file: 'session.ts' }); + const pregate = pregateFor(repo, head); + const shim = (body: string) => ghQueueShim(repo, { prs: { '1': { headRefOid: head, url: 'https://github.com/o/r/pull/1', body } }, checks: { [head]: green } }); + const base = repo.git('rev-parse', 'origin/main'); + const depth = { gstack_version: '1.92.61.0', review_depth: { depth: 'full' as const, class: 'code', runtime_lines: 1, rules: '551486253e156572', base, registry: base, gate_scan: { tagged: 0, head }, reasons: ['runtime=1'], risk: [] } }; + const targeted = (status: 'done' | 'failed') => ({ ...depth, review_targeted: { classes: ['auth' as const], rules: 'r', brief_sha: 'a'.repeat(64), product_patch_id: 'p', status, findings: 0, raised_by: 'targeted' } }); + const legacy = queue(repo, ['ready', '--pr', '1', '--pregate', pregate], shim(shippedBody('1', head, depth))); + expect(line(legacy.out, 'REVIEW_TARGETED:')).toBe('REVIEW_TARGETED: required=auth receipt=legacy verdict=missing'); + expect(legacy.status).toBe(EXIT.refused); + expect(legacy.err).toMatch(/requires the targeted reader on auth.*records no targeted pass.*\(QUEUE_RECEIPT_TARGETED_MISSING\)/); + const failed = queue(repo, ['ready', '--pr', '1', '--pregate', pregate], shim(shippedBody('1', head, targeted('failed')))); + expect(line(failed.out, 'REVIEW_TARGETED:')).toBe('REVIEW_TARGETED: required=auth receipt=none verdict=failed'); + expect(failed.status).toBe(EXIT.refused); + expect(failed.err).toMatch(/requires the targeted reader on auth.*targeted pass failed.*\(QUEUE_RECEIPT_TARGETED_MISSING\)/); + const done = queue(repo, ['ready', '--pr', '1', '--pregate', pregate], shim(shippedBody('1', head, targeted('done')))); + expect(done.status, done.err).toBe(EXIT.ok); + expect(line(done.out, 'REVIEW_TARGETED:')).toBe('REVIEW_TARGETED: required=auth receipt=auth verdict=ok'); + const plain = makePr(repo, 'pr-plain'); + const unrequired = queue(repo, ['ready', '--pr', '1', '--pregate', pregateFor(repo, plain)], ghQueueShim(repo, { prs: { '1': { headRefOid: plain, body: shippedBody('1', plain) } }, checks: { [plain]: green } })); + expect(unrequired.status, unrequired.err).toBe(EXIT.ok); + expect(line(unrequired.out, 'REVIEW_TARGETED:')).toBe('REVIEW_TARGETED: required=none receipt=legacy verdict=ok'); + }, 120_000); + // Value: protects=the re-READY after a slot's stamp push accepts the receipt /ship wrote for the pinned head, and only then; // fails_when=ready binds the receipt to HEAD alone (every stamped READY turns QUEUE_RECEIPT_STALE) or accepts any ancestor's receipt; // why_new=the READY test above binds receipts to the exact head only; seam=none; no_claim=real gh and CI (shims; unit only) diff --git a/test/review-targeted.test.ts b/test/review-targeted.test.ts index 375cf1d01..156e1bee9 100644 --- a/test/review-targeted.test.ts +++ b/test/review-targeted.test.ts @@ -3,7 +3,7 @@ * (lib/review-targeted.ts, docs/review-policy.md#targeted-pass-on-money-gate-auth-and-secret-code). * The classifier, the plan's TARGETED and DISPATCH lines, the dispatch-superset no-loss check * against main's reviewers, the production path from render to the receipt and the review-loop - * summary, and the queue's report-only REVIEW_TARGETED line. + * summary, and the queue's REVIEW_TARGETED verdict (refused when a required class is uncovered, GST-966). */ import { afterEach, describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; @@ -15,7 +15,7 @@ import { classifyDiff, planReview, scanNumbers, type PlanInput } from '../lib/re import { raisedBy, summarizeReviewLoop } from '../lib/review-scope'; import { TARGETED_RULES, targetedBrief, targetedBriefSha, targetedClasses, type TargetedClass, type TargetedInput } from '../lib/review-targeted'; import { carryReceipt, extractReceipt, renderReceiptBlock } from '../lib/ship-receipt'; -import { bindShippedReceipt, parseReviewTargeted, receiptTargetedLine } from '../lib/ship-receipt-depth'; +import { bindShippedReceipt, parseReviewTargeted, receiptTargetedVerdict } from '../lib/ship-receipt-depth'; import type { ShipReceipt } from '../lib/headless-artifacts'; import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; import { checkoutTree } from '../lib/review-brief'; @@ -333,9 +333,9 @@ describe('the targeted role end to end on a fixture repository', () => { expect(ok).toMatchObject({ ok: true, targetedLine: 'REVIEW_TARGETED: required=money receipt=money verdict=ok' }); const { review_targeted: _drop, ...legacy } = receipt; const missing = bindShippedReceipt(repo.work, 'origin/main', renderReceiptBlock(legacy as ShipReceipt), undefined, [head]); - expect(missing).toMatchObject({ ok: true, targetedLine: 'REVIEW_TARGETED: required=money receipt=legacy verdict=missing' }); + expect(missing).toMatchObject({ ok: false, code: 'QUEUE_RECEIPT_TARGETED_MISSING', targetedLine: 'REVIEW_TARGETED: required=money receipt=legacy verdict=missing' }); const failed = bindShippedReceipt(repo.work, 'origin/main', renderReceiptBlock({ ...receipt, review_targeted: { ...receipt.review_targeted!, status: 'failed' } }), undefined, [head]); - expect(failed).toMatchObject({ ok: true, targetedLine: 'REVIEW_TARGETED: required=money receipt=none verdict=missing' }); + expect(failed).toMatchObject({ ok: false, code: 'QUEUE_RECEIPT_TARGETED_MISSING', targetedLine: 'REVIEW_TARGETED: required=money receipt=none verdict=failed' }); }, 180_000); }); @@ -345,16 +345,19 @@ describe('the receipt record and the queue line', () => { expect(parseReviewTargeted(record, 'p1')).toEqual({ ok: true, targeted: { ...record, classes: ['money', 'gate'], product_patch_id: 'p1' } as any }); for (const bad of [{ ...record, classes: ['cost'] }, { ...record, status: 'skipped' }, { ...record, findings: -1 }, { ...record, brief_sha: 'abc' }, { ...record, extra: 1 }, null]) expect(parseReviewTargeted(bad, 'p1').ok).toBe(false); }); - // Value: protects=PR 1's queue only reports; fails_when=the line misreads a legacy, failed or partial receipt as covered, or a - // receipt without classes as required; seam=receiptTargetedLine (the line ready and train build print) - test('REVIEW_TARGETED reports ok, missing and legacy', () => { + // Value: protects=the queue refuses an uncovered required class (GST-966); fails_when=the verdict misreads a legacy, failed or partial + // receipt as covered, or a receipt without classes as required; seam=receiptTargetedVerdict (what ready, train build and land refuse on) + test('REVIEW_TARGETED reports ok, missing, failed and legacy', () => { const r = (rt?: object) => ({ schema_version: 1, head: 'h', base: 'main', ...(rt ? { review_targeted: { ...record, product_patch_id: 'p', ...rt } } : {}) }) as ShipReceipt; - expect(receiptTargetedLine([], r())).toBe('REVIEW_TARGETED: required=none receipt=legacy verdict=ok'); - expect(receiptTargetedLine(['money'], r())).toBe('REVIEW_TARGETED: required=money receipt=legacy verdict=missing'); - expect(receiptTargetedLine(['money'], r({}))).toBe('REVIEW_TARGETED: required=money receipt=gate,money verdict=ok'); - expect(receiptTargetedLine(['money', 'secret'], r({}))).toBe('REVIEW_TARGETED: required=money,secret receipt=gate,money verdict=missing'); - expect(receiptTargetedLine(['gate'], r({ status: 'failed' }))).toBe('REVIEW_TARGETED: required=gate receipt=none verdict=missing'); - expect(receiptTargetedLine([], r({ classes: [], status: 'not-run' }))).toBe('REVIEW_TARGETED: required=none receipt=none verdict=ok'); + expect(receiptTargetedVerdict([], r())).toEqual({ ok: true, line: 'REVIEW_TARGETED: required=none receipt=legacy verdict=ok' }); + expect(receiptTargetedVerdict(['money'], r())).toEqual({ ok: true, line: 'REVIEW_TARGETED: required=money receipt=legacy verdict=missing (legacy receipt, written before receipts recorded review_targeted; accepted)' }); + const current = { ...r(), gstack_version: '1.92.61.0' } as ShipReceipt; + expect(receiptTargetedVerdict(['money'], current)).toMatchObject({ ok: false, line: 'REVIEW_TARGETED: required=money receipt=legacy verdict=missing', message: expect.stringContaining('records no targeted pass') }); + expect(receiptTargetedVerdict(['money'], r({}))).toEqual({ ok: true, line: 'REVIEW_TARGETED: required=money receipt=gate,money verdict=ok' }); + expect(receiptTargetedVerdict(['money', 'secret'], r({}))).toMatchObject({ ok: false, line: 'REVIEW_TARGETED: required=money,secret receipt=gate,money verdict=missing', message: expect.stringContaining('on secret, but the receipt covers gate,money only') }); + expect(receiptTargetedVerdict(['gate'], r({ status: 'failed' }))).toMatchObject({ ok: false, line: 'REVIEW_TARGETED: required=gate receipt=none verdict=failed', message: expect.stringContaining('targeted pass failed') }); + expect(receiptTargetedVerdict(['gate'], r({ status: 'not-run' }))).toMatchObject({ ok: false, line: 'REVIEW_TARGETED: required=gate receipt=none verdict=missing', message: expect.stringContaining('did not run') }); + expect(receiptTargetedVerdict([], r({ classes: [], status: 'not-run' }))).toEqual({ ok: true, line: 'REVIEW_TARGETED: required=none receipt=none verdict=ok' }); }); test('a carry keeps review_targeted with the unchanged product patch', () => { const receipt = { schema_version: 1, head: 'a'.repeat(40), base: 'main', product_patch_id: 'p', review_targeted: { ...record, product_patch_id: 'p' } } as ShipReceipt; diff --git a/test/ship-receipt-depth.test.ts b/test/ship-receipt-depth.test.ts index 4b7cf240f..2fd996f70 100644 --- a/test/ship-receipt-depth.test.ts +++ b/test/ship-receipt-depth.test.ts @@ -66,8 +66,11 @@ describe('the queue refuses a receipt below the depth its head needs', () => { expect(receiptIn(body).review_depth).toMatchObject({ depth: 'full', runtime_lines: 80, risk: ['money'] }); const base = repo.git('rev-parse', 'origin/main').slice(0, 12); const bind = (text: string) => bindShippedReceipt(repo.work, 'origin/main', text, '7', [head]); - const good = bind(fs.readFileSync(body, 'utf8')); - expect(good.ok).toBe(true); + const untargeted = bind(fs.readFileSync(body, 'utf8')); + expect(!untargeted.ok && untargeted.code).toBe('QUEUE_RECEIPT_TARGETED_MISSING'); + expect(!untargeted.ok && untargeted.depthLine).toContain('REVIEW_DEPTH: receipt=full required=full'); + const good = bind(rewrite(body, r => { r.review_targeted = { classes: ['money'], rules: 'r', brief_sha: 'a'.repeat(64), product_patch_id: r.product_patch_id, status: 'done', findings: 0, raised_by: 'targeted' }; })); + expect(good.ok, !good.ok ? good.message : '').toBe(true); expect(good.ok && good.depthLine).toContain('REVIEW_DEPTH: receipt=full required=full runtime_lines=80 class=code'); const low = bind(rewrite(body, r => { r.review_depth.depth = 'standard'; })); From 2212c957693e38ab271948224a79bf51bd4782d9 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 10:12:15 +0000 Subject: [PATCH 2/2] test: type the GST-966 ready fixture's review_depth class as a literal (typecheck:test ratchet) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/gstack-ship-queue.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/gstack-ship-queue.test.ts b/test/gstack-ship-queue.test.ts index 71657bb2c..6a2f6c3cd 100644 --- a/test/gstack-ship-queue.test.ts +++ b/test/gstack-ship-queue.test.ts @@ -370,7 +370,7 @@ describe('ready', () => { const pregate = pregateFor(repo, head); const shim = (body: string) => ghQueueShim(repo, { prs: { '1': { headRefOid: head, url: 'https://github.com/o/r/pull/1', body } }, checks: { [head]: green } }); const base = repo.git('rev-parse', 'origin/main'); - const depth = { gstack_version: '1.92.61.0', review_depth: { depth: 'full' as const, class: 'code', runtime_lines: 1, rules: '551486253e156572', base, registry: base, gate_scan: { tagged: 0, head }, reasons: ['runtime=1'], risk: [] } }; + const depth = { gstack_version: '1.92.61.0', review_depth: { depth: 'full' as const, class: 'code' as const, runtime_lines: 1, rules: '551486253e156572', base, registry: base, gate_scan: { tagged: 0, head }, reasons: ['runtime=1'], risk: [] } }; const targeted = (status: 'done' | 'failed') => ({ ...depth, review_targeted: { classes: ['auth' as const], rules: 'r', brief_sha: 'a'.repeat(64), product_patch_id: 'p', status, findings: 0, raised_by: 'targeted' } }); const legacy = queue(repo, ['ready', '--pr', '1', '--pregate', pregate], shim(shippedBody('1', head, depth))); expect(line(legacy.out, 'REVIEW_TARGETED:')).toBe('REVIEW_TARGETED: required=auth receipt=legacy verdict=missing');