From 7f207c05bf5797f0a3a3e5056d2fd120da70556b Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 13:46:44 +0000 Subject: [PATCH] fix(ship): GST-986 review round 1: a record binds across class-free commits after the read; verify removes a stale targeted record Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-dispatch | 2 +- bin/gstack-ship-receipt | 9 +++++---- changelog.d/gst-986-targeted-recorder.md | 2 +- docs/review-policy.md | 9 ++++++--- docs/troubleshooting.md | 3 ++- lib/ship-receipt-depth.ts | 15 ++++++++++++--- 6 files changed, 27 insertions(+), 13 deletions(-) diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index 5bbc68cda..d42af0dbf 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -364,7 +364,7 @@ function main(): number { if (record) { process.stdout.write(`TARGETED_RECORD: classes=${record.classes.join(',')} status=${record.status} findings=${record.findings} brief=${record.brief_sha.slice(0, 12)} rules=${record.rules} read=${record.head?.slice(0, 12) ?? 'none'}\n`); if (flag('--targeted-out')) fs.writeFileSync(flag('--targeted-out')!, JSON.stringify(record) + '\n'); - } + } else if (flag('--targeted-out')) fs.rmSync(flag('--targeted-out')!, { force: true }); process.stdout.write(`VERIFY: ${v.clean ? 'clean' : 'dirty'} briefs=${v.briefs} outputs=${v.rows.filter(r => r.kind === 'OUTPUT').length} voided=${v.voided}${v.unavailable ? ` unavailable=${v.unavailable}` : ''}\n`); return v.clean ? EXIT.ok : EXIT.fail; } diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index b35ab34c9..9f5e84c4b 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -48,7 +48,7 @@ import { } from '../lib/ship-receipt'; import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; import { comparePath, contextPacket, PACKET_SECTION_CAP, readScopedReview, touchedPaths } from '../lib/ship-patch'; -import { deeper, depthBelow, exceptionProblems, parseReviewLoopException, parseReviewTargeted, receiptDepthAtHead, receiptTargetedAtHead, reviewLoopExceptionLine } from '../lib/ship-receipt-depth'; +import { deeper, depthBelow, exceptionProblems, parseReviewLoopException, parseReviewTargeted, receiptDepthAtHead, receiptTargetedAtHead, reviewLoopExceptionLine, targetedClassesSince } from '../lib/ship-receipt-depth'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; import { readPause, receiptBaseRed } from '../lib/base-red'; @@ -97,8 +97,9 @@ write flags: --pr --head --base --version --gated-tree --predecessor --gate ({classes, rules, brief_sha, status, findings, raised_by, head}: gstack-review-dispatch verify --targeted-out); recorded as review_targeted bound to the receipt's product_patch_id, and refused (RECEIPT_INVALID) when the - commit the reader read (head) has another product patch. write and carry refuse a receipt whose head needs a targeted class - (money, gate, auth, secret) that no done pass covers, as the queue would (RECEIPT_INVALID, RECEIPT_CARRY_REFUSED). + commit the reader read (head) has another product patch and a change since it carries a targeted class. write and carry + refuse a receipt whose head needs a targeted class (money, gate, auth, secret) that no done pass covers, as the queue's + targeted verdict does once #3859 lands (RECEIPT_INVALID, RECEIPT_CARRY_REFUSED). --approval-run (repeatable; resolves plan-sha citations) (W2: a class declared in origin/'s .gstack/approval-classes.json with hits needs a citation that resolves and binds to its effects: APPROVAL_CITATION_MISSING) --out --body . Defaults: head=HEAD, base=origin/HEAD's branch, version=the @@ -222,7 +223,7 @@ function cmdWrite(rest: string[], json: boolean): number { const product = reviewedProduct(repoRoot, base, head, pregateFile); let targeted: ReturnType | undefined; if (flag(rest, '--review-targeted') !== undefined) { - try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; } + try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head)); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; } } if (targeted && !targeted.ok) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${targeted.message}; nothing written`) + '\n'); return EXIT.fail; } const approval = approvalForReceipt({ repoRoot, base, head, citationText: [bodyText, ...flags(rest, '--approval')].join('\n'), runDirs: flags(rest, '--approval-run') }); diff --git a/changelog.d/gst-986-targeted-recorder.md b/changelog.d/gst-986-targeted-recorder.md index cf8f2e30e..d83a3b2b9 100644 --- a/changelog.d/gst-986-targeted-recorder.md +++ b/changelog.d/gst-986-targeted-recorder.md @@ -1,6 +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. +- **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 or nothing since carries a money, gate, auth or secret change, so a record can no longer vouch for targeted code it never read, and the docs and TODOS.md commits /ship makes after the read still bind. - **`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 83e112f4d..c1265efd1 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -378,8 +378,11 @@ renders its brief; `verify --targeted-out .gstack/tmp/targeted.json` writes the (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 +that commit's product patch is the receipt's, or while nothing changed after the read carries a +targeted class: a money, gate, auth or secret change after the read refuses with +`RECEIPT_INVALID`, and a test, version, docs or TODOS.md commit (Steps 14 and 14.5) still +binds. A verify whose ledger has no targeted render removes `--targeted-out`, so an earlier +run's record never reaches Step 19. `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 @@ -413,7 +416,7 @@ puts in the receipt: | `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 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 | +| `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 when a change after the read carries a targeted class, 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 4863f326d..55d8e3d9f 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3234,7 +3234,8 @@ 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). +the receipt's and a change after it carries a targeted class (money, gate, +auth or secret code 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. For a diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 7885d2ece..77051de5c 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -131,9 +131,10 @@ export type ReviewTargeted = NonNullable; /** * `write --review-targeted` input (gstack-review-dispatch verify --targeted-out), bound to the receipt's product patch-id here. * `head` is the commit the reader read; `productPatchOf` gives that commit's product patch-id by the receipt's own rules, and the - * record binds only when it equals the receipt's, so a record verified before a product change never vouches for it (GST-986). + * record binds when it equals the receipt's, or when `classesSince` reads no targeted class in what changed after the read (the + * docs and TODOS.md /ship commits after Step 11), so a record never vouches for targeted-class code it did not read (GST-986). */ -export function parseReviewTargeted(value: unknown, productPatchId: string, productPatchOf: (head: string) => string | undefined): { ok: true; targeted: ReviewTargeted } | { ok: false; message: string } { +export function parseReviewTargeted(value: unknown, productPatchId: string, productPatchOf: (head: string) => string | undefined, classesSince?: (read: string) => readonly TargetedClass[] | null): { ok: true; targeted: ReviewTargeted } | { ok: false; message: string } { const bad = (message: string) => ({ ok: false as const, message }); if (!value || typeof value !== 'object' || Array.isArray(value)) return bad('review_targeted must be { classes, rules, brief_sha, status, findings, raised_by, head }'); const v = value as Record; @@ -145,7 +146,7 @@ export function parseReviewTargeted(value: unknown, productPatchId: string, prod if (typeof v.findings !== 'number' || !Number.isInteger(v.findings) || v.findings < 0) return bad('review_targeted.findings must be an integer >= 0'); if (typeof v.head !== 'string' || !/^[0-9a-f]{40,64}$/.test(v.head)) return bad('review_targeted.head must be the commit the targeted reader read (rerun gstack-review-dispatch verify --targeted-out with this gstack)'); const read = productPatchOf(v.head); - if (read !== productPatchId) return bad(`the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read, so run the targeted reader on HEAD and verify again`); + if (read !== productPatchId && classesSince?.(v.head)?.length !== 0) return bad(`the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read, so run the targeted reader on HEAD and verify again`); const classes = TARGETED_CLASSES.filter(c => (v.classes as string[]).includes(c)); 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() } }; } @@ -235,6 +236,14 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, * 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. */ +/** The targeted classes of what changed from `from` to `head`; null when `from` is not an ancestor of `head` or a class input is unreadable. */ +export function targetedClassesSince(root: string, from: string, head: string): readonly TargetedClass[] | null { + if (!git(root, ['merge-base', '--is-ancestor', from, head]).ok) return null; + const at = headInputs(root, from, head); + const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged: gateScanNeeded(at.c.paths, at.c.root) ? null : 0 }); + return t.errors.length ? null : t.classes; +} + 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);