diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index d42af0dbf..c6006e477 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -357,6 +357,7 @@ function main(): number { return r.ok ? EXIT.ok : EXIT.fail; } if (cmd === 'verify') { + if (flag('--targeted-out')) fs.rmSync(flag('--targeted-out')!, { force: true }); const v = verify(ledger); for (const r of v.rows) process.stdout.write(`${r.kind} ${r.reader} ${r.status} ${r.detail}\n`); if (!v.briefs) { process.stdout.write('VERIFY: no brief records (this run\'s ledger kept no prompts; dispatched-vs-rendered cannot be checked)\n'); return EXIT.refused; } @@ -364,7 +365,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 9f5e84c4b..ee4dc0beb 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -97,7 +97,7 @@ 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 and a change since it carries a targeted class. write and carry + commit the reader read (head) is not an ancestor or a change since it carries a targeted class (gate scan included). 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 @@ -742,7 +742,7 @@ function cmdCarry(rest: string[], json: boolean): number { const errors = validateReceipt(carried); if (errors.length) return refused('RECEIPT_CARRY_REFUSED', `the carried receipt does not validate: ${errors[0]!.message} at ${errors[0]!.path}`); const targetedVerdict = receiptTargetedAtHead(repoRoot, baseRef, carried, head); - if (!targetedVerdict.ok) return refused('RECEIPT_CARRY_REFUSED', `${targetedVerdict.line}: ${targetedVerdict.message}, so the queue would refuse the carried receipt; run /ship on ${head.slice(0, 12)}`); + if (!targetedVerdict.ok) return refused('RECEIPT_CARRY_REFUSED', `${targetedVerdict.line}: ${targetedVerdict.message}, so the queue refuses it once #3859 lands; run /ship on ${head.slice(0, 12)}`); const newBody = placeReceiptFirst(body, carried); const line = receiptSummaryLine(carried); if (scopedDryRun) return refused('RECEIPT_CARRY_SCOPED_REVIEW', scopedDryRun); diff --git a/changelog.d/gst-986-targeted-recorder.md b/changelog.d/gst-986-targeted-recorder.md index d83a3b2b9..13f2098ab 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 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. +- **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. +- **`write` and `carry` refuse a receipt the queue refuses once #3859 lands (#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 c1265efd1..2e77474ef 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -378,13 +378,15 @@ 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, 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 +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 +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`, -`RECEIPT_CARRY_REFUSED`), so /ship never publishes a receipt the queue would refuse. +`RECEIPT_CARRY_REFUSED`). The queue's own refusal arrives with #3859; until it lands, `ready` +only reports the targeted line. `gstack-ship-queue ready` and `train build` print `REVIEW_TARGETED: required= receipt= verdict=ok|missing` and refuse nothing yet; enforcement comes in a later @@ -416,7 +418,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 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 | +| `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 when a change after its read commit carries a targeted class or the read commit is not an ancestor, 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 55d8e3d9f..1985f2688 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3235,7 +3235,8 @@ 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 and a change after it carries a targeted class (money, gate, -auth or secret code changed after the targeted reader read it). +auth or secret code changed after the targeted reader read it, a test edit the +gate scan tags included), or that is not an ancestor of HEAD. **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/review-scope.ts b/lib/review-scope.ts index 388bff174..620c3c11a 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -394,7 +394,7 @@ export function ledgerPass(ledger: string): LedgerPass { * identity rows and ledgers share until rows carry the ledger id. * Fix-checks (GST-986, #3862; `ledgerPasses`, one per ledger): fix_checks is the larger of the review-fix-check rows and the * fix-check ledgers, so one logged both ways counts once. When fix-checks ran without their review-fix-check rows (more fix-check - * ledgers than rows), a blocker no row disposes counts as fixed if a fix-check ledger started after the ledger it came from: + * ledgers than rows), a blocker no row disposes counts as fixed if a fix-check ledger started after the last ledger that raised it: * decision 3.5 runs a fix-check only after the loop applied that pass's fixes, and a blocker the loop does not fix is deferred * or noted by a row. A blocker the last pass found stays unrecorded, and with every fix-check logged the rows decide. */ @@ -434,6 +434,7 @@ export function summarizeReviewLoop(rows: readonly Record[], since: let ledgerOnlyFixed = 0; const pairedIds = new Set(); const followedIds = new Set(); + const lastPaired = new Map(); const fixCheckStarts = ledgerPasses.filter(p => p.fixCheck && Number.isFinite(p.started)).map(p => p.started); const rowFixChecks = loopRows.filter(r => r.skill === FIX_CHECK_SKILL).length; const followed = (n: number) => fixCheckStarts.length > rowFixChecks && fixCheckStarts.some(t => t > (ledgerPasses[n]?.started ?? Infinity)); @@ -441,9 +442,10 @@ export function summarizeReviewLoop(rows: readonly Record[], since: for (const b of list) { const by = b.id.startsWith('fix-check') ? null : raiserOf(b.id); const id = b.locus ? [...seen].find(([k, s]) => s.candidate && s.locus === b.locus && (!s.raiser || !by || s.raiser === by) && !paired.has(`${n}\0${k}`))?.[0] : undefined; - if (id) { paired.add(`${n}\0${id}`); pairedIds.add(id); if (followed(n)) followedIds.add(id); } else { ledgerOnly++; if (followed(n)) ledgerOnlyFixed++; } + if (id) { paired.add(`${n}\0${id}`); pairedIds.add(id); if (!((ledgerPasses[lastPaired.get(id) ?? -1]?.started ?? -Infinity) > (ledgerPasses[n]?.started ?? -Infinity))) lastPaired.set(id, n); } else { ledgerOnly++; if (followed(n)) ledgerOnlyFixed++; } } } + for (const [id, n] of lastPaired) if (followed(n)) followedIds.add(id); const blockers = [...seen].filter(([k, s]) => s.blocks || (pairedIds.has(k) && !s.classified)); for (const [k, s] of blockers) if (!s.disposed && followedIds.has(k)) { s.disposed = true; fixed.add(k); } const unrecorded = ledgerOnly - ledgerOnlyFixed + blockers.filter(([, s]) => !s.disposed).length; diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 77051de5c..0e1c46b42 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -131,8 +131,9 @@ 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 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). + * 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. */ 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 }); @@ -146,7 +147,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 && 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`); + 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`); 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() } }; } @@ -231,19 +232,30 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, return { ...b, depthLine: d.line, targetedLine }; } +/** + * The targeted classes of what changed from `from` to the checked-out `head`, with gstack-gate-diff run over that range whenever + * a non-docs path changed (a test edit that skips a case is gate class); null when `from` is not an ancestor of `head`, the gate + * scan did not run, 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); + let gateTagged: number | null = 0; + if (gateScanNeeded(at.c.paths, at.c.root)) { + const g = runScriptSync(path.join(BIN_DIR, 'gstack-gate-diff'), [from], { cwd: root, encoding: 'utf8', timeout: 120_000, maxBuffer: 16 * 1024 * 1024 }); + const m = /GATE_SUMMARY: .*?tagged=\{([^}]*)\}/.exec(g.stdout ?? ''); + if (g.status !== 0 || !m) return null; + gateTagged = m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0); + } + const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged }); + return t.errors.length ? null : t.classes; +} + /** * 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. */ -/** 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);