From cfda4e868d7a8607edf049cb6768398db451c46c Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 19:36:33 +0000 Subject: [PATCH] fix: GST-986 bind a targeted record only to the product patch the reader read The targeted gate reader at 0715327 found that the classifier-only binding rule kept a stale record bound when a post-read edit removed an enforcing line with no money token. Restore main's equal-product-patch check and keep the gate-scan refusal on top of it. Docs-only binding moves to GST-1031. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-986-targeted-recorder.md | 2 +- docs/review-policy.md | 8 +++---- lib/ship-receipt-depth.ts | 10 ++++---- test/review-targeted-record.test.ts | 30 ++++++++++++++++++------ 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/changelog.d/gst-986-targeted-recorder.md b/changelog.d/gst-986-targeted-recorder.md index 94c10f58b..f9701c467 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 nothing since that commit carries a money, gate, auth or secret change (a test edit that skips a case counts as gate), 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. +- **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 is the receipt's and nothing since it carries a money, gate, auth or secret change (a test edit that skips a case counts as gate), so a record can no longer vouch for code it never read. - **`write` and `carry` refuse a receipt the queue refuses (#3854, #3859).** 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 e63737023..a18ca0079 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -404,10 +404,10 @@ 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 is an ancestor of HEAD and nothing changed after it carries a targeted class, with -the gate scan run over that range: a money, gate, auth or secret change after the read refuses -with `RECEIPT_INVALID`, a test edit the gate scan tags (a skipped case) included, while a -version, docs, TODOS.md (Steps 14 and 14.5) or untagged test commit still binds. Every verify +that commit is an ancestor of HEAD, its product patch is the receipt's, and nothing changed +after it carries a targeted class, with the gate scan run over that range: any product change +after the read refuses with `RECEIPT_INVALID`, docs and TODOS.md commits included, and so +does a test edit the gate scan tags (a skipped case). Every verify removes `--targeted-out` before it writes a record, so an earlier run's record never reaches Step 19. `write` and `carry` (which rescans the gate at the new head) also refuse a receipt whose head needs a class no `done` record covers (`RECEIPT_INVALID`, diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index a8013e887..134698330 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -133,10 +133,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 when `classesSince` reads no targeted class in what changed after the read (the docs and TODOS.md /ship commits - * after Step 11; a test change the gate scan tags is gate class though the product patch leaves tests out), so a record never - * vouches for targeted-class work it did not read (GST-986); without `classesSince` it binds only on an equal product patch. + * `head` is the commit the reader read; `productPatchOf` gives that commit's product patch-id by the receipt's own rules. The + * record binds only on an equal product patch, and with `classesSince` also only when nothing changed after the read carries a + * targeted class (a test change the gate scan tags is gate class though the product patch leaves tests out), so a record never + * vouches for work it did not read (GST-986). */ 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 }); @@ -150,7 +150,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 (classesSince ? classesSince(v.head)?.length !== 0 : 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 && 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() } }; } diff --git a/test/review-targeted-record.test.ts b/test/review-targeted-record.test.ts index a717d3163..3eeb5909f 100644 --- a/test/review-targeted-record.test.ts +++ b/test/review-targeted-record.test.ts @@ -150,11 +150,10 @@ describe('write binds a targeted record only to the product patch the reader rea expect(old.err).toContain('review_targeted.head'); }); - // Value: protects=/ship's Steps 14 and 14.5 commit TODOS.md and docs after the Step 11 read, and those commits carry no targeted - // class, so the record still binds (pass 1 Red Team CRITICAL on this PR: every money, gate, auth or secret ship whose doc audit - // edited a file stopped at Step 19 with RECEIPT_INVALID); a money edit after the read is still refused; fails_when=write - // refuses a docs-only change after the read or accepts a class-bearing one; seam=gstack-ship-receipt write --review-targeted - test('a docs and TODOS.md commit after the read binds; a money edit after it is refused', () => { + // Value: protects=a record binds only to the product patch the reader read, so a docs or TODOS.md commit after the read (which + // changes the product patch) is refused like a money edit; binding docs-only commits is planned under GST-1031 with a full + // review; fails_when=write binds a record whose read product patch is not the receipt's; seam=gstack-ship-receipt write --review-targeted + test('a docs and TODOS.md commit after the read is refused; a money edit after it is refused', () => { const { repo, base, head } = moneyBranch(); const record = path.join(repo.root, 'targeted.json'); expect(verify(repo, targetedRead(repo, base, 'h1', 'NO FINDINGS'), record).status).toBe(0); @@ -162,8 +161,8 @@ describe('write binds a targeted record only to the product patch the reader rea commit(repo, 'docs/charges.md', '# Charges\n\nThe cap is enforced per run.\n', 'Step 14.5 docs'); commit(repo, 'TODOS.md', '# TODOS\n\n- [x] enforce the cap\n', 'Step 14 TODOS'); const docs = write(repo, ['--review-targeted', `@${record}`]); - expect(docs.status, docs.err).toBe(EXIT.ok); - expect(JSON.parse(docs.out).review_targeted).toMatchObject({ classes: ['money'], status: 'done' }); + expect(docs.status).toBe(EXIT.fail); + expect(docs.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); commit(repo, 'src/charge.ts', 'export const spend_cap_usd = 50;\n', 'money edit after verify'); const stale = write(repo, ['--review-targeted', `@${record}`]); @@ -171,6 +170,23 @@ describe('write binds a targeted record only to the product patch the reader rea expect(stale.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); }); + // Value: protects=a code edit after the read that drops a cap's enforcing line carries no money token, so the classifier tags + // nothing, but the product patch the reader read is gone and the record must not bind (targeted gate reader on this PR at + // 071532752, CRITICAL lib/ship-receipt-depth.ts:153); fails_when=write binds a record whose read product patch is not the + // receipt's; seam=gstack-ship-receipt write --review-targeted + test('a code edit after the read that removes an enforcing line with no money token is refused', () => { + const { repo, base } = moneyBranch(); + commit(repo, 'src/runner.ts', 'export function run(spent: number, limit: number) {\n if (spent > limit) throw new Error("over");\n}\n', 'enforce the limit'); + const head = repo.git('rev-parse', 'HEAD'); + const record = path.join(repo.root, 'targeted.json'); + expect(verify(repo, targetedRead(repo, base, 'h1', 'NO FINDINGS'), record).status).toBe(0); + expect(write(repo, ['--review-targeted', `@${record}`]).status).toBe(EXIT.ok); + commit(repo, 'src/runner.ts', 'export function run(spent: number, limit: number) {\n}\n', 'drop the enforcing line'); + const dropped = write(repo, ['--review-targeted', `@${record}`]); + expect(dropped.status).toBe(EXIT.fail); + expect(dropped.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); + }); + // Value: protects=a test edit after the read that skips a case is gate-class work the reader never saw, so the record does not // bind though the product patch (which leaves tests out) is unchanged (round 2 outside voice and adversarial on this PR); // a record whose read commit is not an ancestor of the head is refused; fails_when=write binds either;