From 84d644f35687bdde393e3e8e57186ce986840bf9 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 13:21:53 +0000 Subject: [PATCH] fix(ship): GST-986 write/carry report fail-closed targeted classes; docs, changelog, adversarial repin (brief bodies unchanged) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 1 + changelog.d/gst-986-targeted-recorder.md | 6 +++++ docs/review-policy.md | 15 ++++++++--- docs/troubleshooting.md | 18 ++++++++++--- lib/ship-receipt-depth.ts | 10 ++++++-- test/review-packets-removed.test.ts | 32 +++++++++++++++++++++++- test/ship-receipt-depth.test.ts | 6 +++-- 7 files changed, 75 insertions(+), 13 deletions(-) create mode 100644 changelog.d/gst-986-targeted-recorder.md diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 302e5d0fc..b35ab34c9 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -268,6 +268,7 @@ function cmdWrite(rest: string[], json: boolean): number { return EXIT.fail; } const targetedVerdict = receiptTargetedAtHead(repoRoot, `origin/${base.replace(/^origin\//, '')}`, receipt, receipt.head); + if (targetedVerdict.line.includes('(unchecked: ')) process.stderr.write(targetedVerdict.line + '\n'); if (!targetedVerdict.ok) { process.stderr.write(`${targetedVerdict.line}\n${resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${targetedVerdict.message}; run the targeted reader on HEAD and pass its record (--review-targeted @); nothing written`)}\n`); return EXIT.fail; diff --git a/changelog.d/gst-986-targeted-recorder.md b/changelog.d/gst-986-targeted-recorder.md new file mode 100644 index 000000000..cf8f2e30e --- /dev/null +++ b/changelog.d/gst-986-targeted-recorder.md @@ -0,0 +1,6 @@ +### Fixed + +- **The targeted reader's record is honest from verify to the receipt.** `gstack-review-dispatch verify` no longer marks a completed targeted reader `status=failed findings=0` because a Step 11 fix moved HEAD after it read (gstack-evals#140, C5-36 and C6-36). The record now says `done` with its real finding count and names the commit it read. `gstack-ship-receipt write --review-targeted` binds it only while that commit's product patch matches the receipt's, so a record verified before a product change can no longer vouch for it. +- **`write` and `carry` refuse a receipt the queue would refuse (#3854).** When the head touches money, gate, auth or secret code and no done targeted pass covers it, `write` refuses with `RECEIPT_INVALID` and `carry` with `RECEIPT_CARRY_REFUSED`, instead of publishing a receipt `ready` turns away. Legacy receipts still carry. +- **/ship passes the targeted record to the receipt (#3853).** Step 11's verify writes `.gstack/tmp/targeted.json`, and the Step 19, handoff and driver receipt writes pass it with `--review-targeted`. +- **Fix-check rounds count without hand edits (#3862).** `gstack-review-scope summary --run` counts fix-checks from the dispatch ledgers when no `review-fix-check` row was logged, and counts the blockers those fix-checks followed as fixed. diff --git a/docs/review-policy.md b/docs/review-policy.md index a5fd51cb7..83e112f4d 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -373,8 +373,15 @@ The brief states the blocking floor in one sentence, and its money block cites the checklist's Forced-CRITICAL rule on uncapped spend, so a money finding can 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. +renders its brief; `verify --targeted-out .gstack/tmp/targeted.json` writes the record that +`gstack-ship-receipt write --review-targeted @.gstack/tmp/targeted.json` puts in the receipt +(Step 19, the handoff and the driver pass it when the file exists). The record is `done` when +the reader got its brief verbatim and its reply was collected on a committed diff, whatever +happened to HEAD after, and it names the commit it read (`head`). `write` binds it only while +that commit's product patch is the receipt's: a product change after the read refuses with +`RECEIPT_INVALID`, and a test-only or version commit still binds. `write` and `carry` also +refuse a receipt whose head needs a class no `done` record covers (`RECEIPT_INVALID`, +`RECEIPT_CARRY_REFUSED`), so /ship never publishes a receipt the queue would refuse. `gstack-ship-queue ready` and `train build` print `REVIEW_TARGETED: required= receipt= verdict=ok|missing` and refuse nothing yet; enforcement comes in a later @@ -402,11 +409,11 @@ puts in the receipt: | `review_loop.noted` | below-floor findings listed in the PR body | | `review_loop.fixed` | findings fixed in this PR | | `review_loop.scope` | the `review_scope` the loop ran under | -| `review_loop.fix_checks` | fix-checks this ship ran (never passes) | +| `review_loop.fix_checks` | fix-checks this ship ran (never passes): the larger of the `review-fix-check` rows and the ledgers holding a fix-check reader's review. When fix-checks ran without their rows, a blocker no row disposes counts as fixed once a later fix-check ledger followed it (decision 3.5 runs one only after the fixes) | | `review_loop.rounds` | passes plus fix-checks (R51 caps it at 2; the queue's `REVIEW_DEPTH:` line prints it) | | `review_loop.dispatches` | reviewer dispatches by role: `specialist`, `red-team`, `adversarial-native`, `outside-voice`, `fix-check`, `targeted` | | `review_loop.est_input_tokens` | prompt plus diff bytes the dispatches read, divided by 4: a relative estimate for comparing runs, not a bill | -| `review_targeted` | the [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code): the `classes` it covered, `rules`, `brief_sha`, the receipt's own `product_patch_id`, `status` (`done` only with its output collected and the committed diff it was rendered on still the one at verify, else `failed`; absent when write got no record, which the queue reports as `legacy`), `findings` and `raised_by`; a carry keeps it while the product patch is unchanged | +| `review_targeted` | the [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code): the `classes` it covered, `rules`, `brief_sha`, the receipt's own `product_patch_id`, `status` (`done` only with its brief sent verbatim and its output collected on a committed diff, else `failed`; `write` refuses a record whose read commit has another product patch, and refuses to write without a record when the head needs a class), `findings` and `raised_by`; a carry keeps it while the product patch is unchanged | Each review row may carry `dispatches: [{role, effort, prompt_bytes, diff_bytes, status}]`, and each finding `raised_by` (the reviewer that raised it, diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 6634be10f..4863f326d 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3230,10 +3230,17 @@ for an older PR, write it once by hand with the same command. **Meaning.** The block exists but does not validate against the `ship-receipt` schema (`gstack-artifact schema ship-receipt`). `gstack-ship-receipt write` also refuses with this code, and writes nothing, when `--head` is not the -checked-out HEAD: the receipt binds the tree the review read. +checked-out HEAD: the receipt binds the tree the review read. It refuses the +same way when the head touches money, gate, auth or secret code and no done +targeted record covers those classes (`REVIEW_TARGETED: … verdict=missing|failed`), +or when `--review-targeted` names a read commit whose product patch is not +the receipt's (the product changed after the targeted reader read it). **Fix.** Regenerate it with `gstack-ship-receipt write`; never hand-edit the -block. For the `--head` refusal, check out that commit and write there. +block. For the `--head` refusal, check out that commit and write there. For a +targeted refusal, run the targeted reader on HEAD, `gstack-review-dispatch verify +--targeted-out .gstack/tmp/targeted.json`, and write again with +`--review-targeted @.gstack/tmp/targeted.json`. ### `EXECUTION_UNKNOWN` @@ -5695,7 +5702,10 @@ working tree's status. It also refuses when an owner exception the receipt recor no longer stands, and, for a receipt from a gstack that records review depth, when HEAD needs a deeper review than the receipt records, `--head` is not the checked-out HEAD, or the receipt records `gstack_version` without -`review_depth` ([ship-policy.md](ship-policy.md#ship-receipt)). The receipt in +`review_depth` ([ship-policy.md](ship-policy.md#ship-receipt)). It refuses, too, when +HEAD needs a targeted class (money, gate, auth, secret) that the receipt's +`review_targeted` does not cover with a done pass, since the queue would refuse +that receipt. The receipt in the body is unchanged and still names the old head, so `gstack-ship-queue ready` refuses `QUEUE_RECEIPT_STALE`. @@ -5703,7 +5713,7 @@ ready` refuses `QUEUE_RECEIPT_STALE`. `gstack-ship-receipt carry --pr ` at the new head. When the history is already rewritten and the receipt predates `head_tree`, run /ship on the new head; next time merge the base in instead of rebasing. For a retargeted stacked -PR whose older receipt lost its parent branch, or a depth or exception +PR whose older receipt lost its parent branch, or a depth, targeted or exception refusal, run /ship on the new head. When `--head` is not the checked-out HEAD, check it out and carry there. diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index f9e8d1ce3..7885d2ece 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -230,9 +230,15 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, return { ...b, depthLine: d.line, targetedLine }; } -/** receiptTargetedVerdict at the receipt's head, with the classes the queue computes: what write and carry check before they publish (#3854). */ +/** + * receiptTargetedVerdict at the receipt's head, with the classes the queue computes: what write and carry check before they + * publish (#3854). Classes that failed closed on an input this checkout cannot read (no origin base, no gate scan) are reported, + * never refused on here: the queue reads its own inputs at ready and refuses there if the head needs the reader. + */ export function receiptTargetedAtHead(root: string, baseRef: string, r: ShipReceipt, head: string): DepthVerdict { const at = headInputs(root, baseRef, head); const gateTagged = boundScan(r, head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0); - return receiptTargetedVerdict(targetedClasses({ ...at.c, scope: at.scope, gateTagged }).classes, r); + const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged }); + const v = receiptTargetedVerdict(t.classes, r); + return v.ok || !t.errors.length ? v : { ok: true, line: `${v.line} (unchecked: ${t.errors.join('; ')}; the queue decides at ready)` }; } diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index c8ab8f706..e85fbcfc5 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -8,7 +8,9 @@ import { describe, test, expect } from 'bun:test'; import { spawnSync } from 'child_process'; import { createHash } from 'crypto'; import * as fs from 'fs'; +import * as os from 'os'; import * as path from 'path'; +import { sectionTexts } from '../lib/review-brief'; const ROOT = path.join(import.meta.dir, '..'); const REMOVED = [ @@ -57,12 +59,13 @@ const NO_LOSS_REPLAYS = { 'L': "GSTA-167 variant G Arm L: live no-loss replay of one batched reader (testing and security plus scoped checklists) and the Red Team on its findings, 2 passes, scored L∘G1 against G1 on the paired rule (part 2)", 'GST-257': "GST-257 PR 1 dispatch-superset no-loss check (test/review-targeted.test.ts): on every fixture diff the branch dispatches main's reviewers at 8be7f21 unchanged plus only the targeted reader, and over the last 100 non-release commits on main it lost none; adds the targeted role and brief to the G1 and L bytes, whose existing reviewer text is unchanged", 'GST-850': "GST-850 dispatch-superset no-loss check: on the 26 GSTA-167 no-loss units the branch plan (wider security and money classes, write-up text in the batched diff) drops no reviewer role main dispatches, so no saved blocker can be lost, and over the last 100 main commits no commit with a gate or risk class drops depth (test/review-risk-routing.test.ts, test/review-risk-linear.test.ts); reviewer text is unchanged", + 'GST-986': "GST-986 brief-identity no-loss check (the brief-body test below): every reviewer brief lib/review-brief.ts cuts from ship/sections/adversarial.md is byte-identical to the bodies at the GST-257 pin; only the lead's Step 11 instruction naming the targeted record's path changed, outside the cut spans", 'R51': "R51 risk-tier routing no-loss re-score (GSTA-167): the saved panel findings (349 records over 24 of the 26 no-loss units) re-scored against the branch's plan lose no blocker whose only finders are roles it drops, and every gate, security, user-data and money fixture keeps main's reviewers plus the full panel (test/review-targeted.test.ts, test/review-risk-routing.test.ts); reviewer text is unchanged", } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; const REVIEWER_BRIEFS: Record = { 'ship/sections/review-army.md': { sha: 'b7ca291a1cc63750bfb9710a4c12fd971523b9598da2a32ef80fb258eb0eceab', replay: 'L' }, - 'ship/sections/adversarial.md': { sha: 'd776ee5bc6e0c13930f8f4de05bad2f1ed9e52bc9fc5df80ae3ba7680455b2bb', replay: 'GST-257' }, + 'ship/sections/adversarial.md': { sha: 'b70577b6987869acd76cc3865f16eeeed743558c0ea09e5f9933d0d8aec8b91e', replay: 'GST-986' }, 'review/sections/review-army.md': { sha: '78329b64640260cd8831ca81a9189f3f8dcade9c406431631d3f2cd798cd3967', replay: '#3638' }, 'review/sections/adversarial.md': { sha: 'e6962fa25b78f362654f2954f79683e524d2ebd452d1494f8a2b099888e8e255', replay: 'G1' }, 'review/checklist.md': { sha: '607788afeb2c83f2dff1dccd58d212616ec72393d28cda2fa18690c1a83df699', replay: '#3638' }, @@ -83,7 +86,34 @@ const REVIEWER_BRIEFS: Record = { 'lib/review-risk.ts': { sha: '18f0330ddfda0032eb93516b0eb941fa4244e0a68b028e19c436c74609093b03', replay: 'GST-850' }, }; +/** The reviewer brief bodies lib/review-brief.ts cuts out of the ship sections, at the bytes replays L and GST-257 admitted. */ +const BRIEF_BODIES: Record = { + specialist: '1ebe47ddb3291a9e4fe90b5a260eb57e328144959549bc95c119b68d23652cb5', + framing: 'aad667b5ea4c6d23d930dafffa38178c04be388cd4364fb73759fc92af17bb63', + fixtures: '9517c86befe773e76acbf30a0642ed828e211b8ecb524325702072b26429ef16', + redTeam: '1cca7957d13f9da08434870290b92958f37750207047b92c9aa52a802da789e8', + native: 'ddb1041ec1e4dd373c3e4ab9017e49316e78c230055c1ef3db1bbba518b143a2', + outside: 'bd96406da28b03742384560425a333b6d6d7ed2be95bd2026c90e0039632cee2', +}; + describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () => { + // Value: protects=the GST-986 repin of ship/sections/adversarial.md admits only a lead-instruction edit: every brief a reviewer + // reads renders byte-identical to the no-loss-tested bodies; fails_when=an edit inside a cut span changes a brief while the + // section pin moves; seam=sectionTexts, the function every render uses; no_claim=the checklists (pinned by file hash below) + test('every reviewer brief cut from the ship sections is byte-identical to its pinned body', () => { + const hashes = (root: string) => Object.fromEntries(Object.entries(sectionTexts(root)).map(([k, v]) => [k, createHash('sha256').update(v).digest('hex')])); + expect(hashes(ROOT)).toEqual(BRIEF_BODIES); + const copy = fs.mkdtempSync(path.join(os.tmpdir(), 'brief-bodies-')); + try { + fs.cpSync(path.join(ROOT, 'ship', 'sections'), path.join(copy, 'ship', 'sections'), { recursive: true }); + const adversarial = path.join(copy, 'ship', 'sections', 'adversarial.md'); + fs.writeFileSync(adversarial, fs.readFileSync(adversarial, 'utf8').replace('Recommendation:', 'Recommendation (short):')); + expect(hashes(copy)).not.toEqual(BRIEF_BODIES); + } finally { + fs.rmSync(copy, { recursive: true, force: true }); + } + }); + test('every pinned reviewer brief, checklist and depth rule hashes to its pin', () => { const drift = Object.entries(REVIEWER_BRIEFS).filter(([file, pin]) => { const text = fs.readFileSync(path.join(ROOT, file), 'utf8').replace(/\r\n/g, '\n'); diff --git a/test/ship-receipt-depth.test.ts b/test/ship-receipt-depth.test.ts index 4b7cf240f..c9c20dea4 100644 --- a/test/ship-receipt-depth.test.ts +++ b/test/ship-receipt-depth.test.ts @@ -19,6 +19,8 @@ afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); const receiptIn = (body: string) => extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; const writeReceipt = (repo: FixtureRepo, body: string, extra: string[] = []) => runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body, ...extra]); +/** A done targeted record read at HEAD on every class: since GST-986, write refuses a money, gate, auth or secret head without one. */ +const targetedAtHead = (repo: FixtureRepo) => ['--review-targeted', JSON.stringify({ classes: ['money', 'gate', 'auth', 'secret'], rules: 'r', brief_sha: 'a'.repeat(64), status: 'done', findings: 0, raised_by: 'targeted', head: repo.git('rev-parse', 'HEAD') })]; const lines = (n: number) => Array.from({ length: n }, (_, i) => `exports.f${i} = () => ${i};`).join('\n') + '\n'; const rewrite = (body: string, edit: (r: Record) => void) => { const text = fs.readFileSync(body, 'utf8'); @@ -62,7 +64,7 @@ describe('the queue refuses a receipt below the depth its head needs', () => { const repo = makeFixtureRepo(); repos.push(repo); const head = commit(repo, { 'lib/billing.js': lines(80) }); const body = path.join(repo.root, 'body.md'); - expect(writeReceipt(repo, body).status).toBe(EXIT.ok); + expect(writeReceipt(repo, body, targetedAtHead(repo)).status).toBe(EXIT.ok); 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]); @@ -100,7 +102,7 @@ describe('the queue refuses a receipt below the depth its head needs', () => { const repo = makeFixtureRepo(); repos.push(repo); const head = commit(repo, { 'lib/auth.js': lines(3) }); const body = path.join(repo.root, 'body.md'); - expect(writeReceipt(repo, body).status).toBe(EXIT.ok); + expect(writeReceipt(repo, body, targetedAtHead(repo)).status).toBe(EXIT.ok); expect(receiptIn(body).review_depth).toMatchObject({ depth: 'full', class: 'gate' }); const bind = (text: string) => bindShippedReceipt(repo.work, 'origin/main', text, '7', [head]); const light = bind(rewrite(body, r => { r.review_depth.depth = 'light'; }));