From 2305526774b033a2e743396120cb34611ae29f2c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 04:33:37 -0800 Subject: [PATCH] review-plan: a granted fix round after the targeted read plans one targeted-only reader (GST-1607) (#4064) * review-plan: a granted fix round after the targeted read plans one targeted-only reader (GST-1607) After the owner granted a fix round past the loop cap, the granted round's fix moved the product patch the targeted record names, so the receipt asked for a fresh targeted read while the plan printed done (harness-evals #271). With that --grant (checked against the approval record as before), any change since the read except plain docs now plans NEXT: targeted on the GST-1598 route: one targeted reader on HEAD for the head classes. A head with no targeted class adds no reader; no grant keeps done. The Capy pass program now runs a targeted manifest's voices stage, so a NEXT: targeted pass driven through it creates its reader (it activated voices only for fix-check). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: changelog fragment for GST-1607 Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: review-policy names the granted case in the targeted-only skip list (docs audit) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-plan | 8 +- changelog.d/gst-1607-granted-targeted.md | 1 + docs/review-policy.md | 15 ++- lib/capy-review-pass.js | 2 +- lib/review-targeted-delta.ts | 8 +- ship/sections/pr-body.md | 2 +- ship/sections/pr-body.md.tmpl | 2 +- test/review-pass.test.ts | 21 ++++ test/review-plan-granted-targeted.test.ts | 99 +++++++++++++++++++ .../ship/gst-1607-granted-targeted.json | 4 + 10 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 changelog.d/gst-1607-granted-targeted.md create mode 100644 test/review-plan-granted-targeted.test.ts create mode 100644 test/size-ledger/ship/gst-1607-granted-targeted.json diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 67f468aba..fde4e7432 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -73,7 +73,8 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do reader last read it: the same patch skips it; any other id, none, or uncommitted edits dispatch it again. With --range-base naming the commit that read happened on, a loop that is done plans NEXT: targeted (one targeted reader on HEAD, TARGETED_DELTA: names the range) when the only change since is docs the - receipt cannot carry the record across (a money, security or numeric claim; GST-1598) + receipt cannot carry the record across (a money, security or numeric claim; GST-1598), or, with a + --grant this invocation checks, any change but plain docs (the granted fix round; GST-1607) --grant the owner's grant of one scoped fix-check after round 2, recorded with gstack-approvals add (approved, its ask or scope naming review_loop_grant and the stop head's sha, at or after --range-base and at or before HEAD; needs --range-base): the stop after a round-2 full pass with a fixed blocking finding or a @@ -234,9 +235,10 @@ function main(): number { }); if (postReview && pass > 0) plan.next.reason = `edits after the review bound (Step 16 stage 2.1) plan as a fix to a gate-class path, never a note: ${plan.next.reason}`; const ranArg = flag('--targeted-ran'); - const delta = plan.next.step === 'done' && rangeBase && ranArg && targeted.classes.length && !ran ? targetedDelta(root, ref, rangeBase, ranArg) : null; + const delta = plan.next.step === 'done' && rangeBase && ranArg && targeted.classes.length && !ran ? targetedDelta(root, ref, rangeBase, ranArg, !!grant) : null; if (delta?.state === 'dispatch') { - plan.next = { step: 'targeted', reason: `the loop is done and the only change since the targeted read at ${delta.range_base.slice(0, 12)} is docs whose claim the record cannot carry (${delta.paths.join(', ')}): one targeted reader reads HEAD for ${classList(targeted.classes)}; it is not a review round` }; + const why = grant ? `the owner's grant ${grant} covers the fix round that changed ${delta.paths.join(', ')}` : `the only change since is docs whose claim the record cannot carry (${delta.paths.join(', ')})`; + plan.next = { step: 'targeted', reason: `the loop is done and ${why} after the targeted read at ${delta.range_base.slice(0, 12)}: one targeted reader reads HEAD for ${classList(targeted.classes)}; it is not a review round` }; plan.dispatches.push({ role: 'targeted', name: 'targeted', effort: ROLE_EFFORT.targeted, exclude: active ? [...active.specs] : [], classes: [...targeted.classes], ...(active?.keep.length ? { keep: [...active.keep] } : {}) }); } else if (delta) plan.skipped.push(`targeted-only read (${delta.reason})`); const targetedDeltaOut = delta?.state === 'dispatch' ? { range_base: delta.range_base, paths: delta.paths, classes: targeted.classes } : null; diff --git a/changelog.d/gst-1607-granted-targeted.md b/changelog.d/gst-1607-granted-targeted.md new file mode 100644 index 000000000..e611537ab --- /dev/null +++ b/changelog.d/gst-1607-granted-targeted.md @@ -0,0 +1 @@ +- **A granted fix round no longer needs a second grant to refresh the targeted read.** When the owner grants a fix round past the review-loop cap, that fix changes the product patch the targeted record names, so the receipt asks for a new targeted read. The plan used to print `done` once the granted fix-check came back clean, which left the lane needing another manual grant. `gstack-review-plan` now takes the same `--grant` (checked as before) and plans `NEXT: targeted`: one targeted reader on the head for its targeted classes. Without a grant, a code change still plans `done`. The Capy pass program now runs a targeted manifest's voices stage too, so that reader actually runs there. A granted fix that edits a gate-class path still stops for the owner. (GST-1607) diff --git a/docs/review-policy.md b/docs/review-policy.md index 074a3b5d3..20ec7f7b0 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -540,13 +540,24 @@ among its plan lines. After `verify --targeted-out`, pass the new `targeted_patch_id` and the plan is `done`. Every other case stays `done` and prints `SKIP: targeted-only read -()`. That covers a code or binary change, which takes the review loop -as before; plain docs, which the record carries; a range_base the +()`. That covers a code or binary change with no `--grant` (the granted +case is below), which takes the review loop as before; plain docs, which the record carries; a range_base the `--targeted-ran` patch doesn't match; and uncommitted edits. `--force targeted` refuses (exit 2) and names these flags, because the targeted reader is no specialist (`lib/review-targeted-delta.ts`, `test/review-plan-targeted-delta.test.ts`). +A fix round the owner granted past the cap has the same problem in code +(harness-evals #271, GST-1607). The granted round's fix moves the product +patch the targeted record names, so the receipt asks for a fresh read once +the loop is done. With that grant passed as `--grant` (checked as above: +an approved `review_loop_grant` row naming a stop head inside +range_base..HEAD), any change since the read except plain docs plans the +same `NEXT: targeted`, and the reason names the grant. A head with no +targeted class adds no reader, and the same change with no grant keeps +`done` (`test/review-plan-granted-targeted.test.ts`). The Capy pass program +runs a targeted manifest's voices stage, so this one reader runs there too. + ## What readers read under `review_exclude_paths` diff --git a/lib/capy-review-pass.js b/lib/capy-review-pass.js index 7b1752a6c..aa6e1adad 100644 --- a/lib/capy-review-pass.js +++ b/lib/capy-review-pass.js @@ -146,7 +146,7 @@ const beforeKey = input.stage === 'voices' ? 'before_voices' : 'before'; try { let m = await load(); - if (input.stage !== 'voices' && m.next === 'fix-check') active.push('voices'); + if (input.stage !== 'voices' && (m.next === 'fix-check' || m.next === 'targeted')) active.push('voices'); if (active.includes('voices')) { const v = await sh('cat ' + q(voiceFile) + ' 2>/dev/null || true'); if (v.out.trim()) voices = JSON.parse(v.out); diff --git a/lib/review-targeted-delta.ts b/lib/review-targeted-delta.ts index cfb49bbe2..14a25c0b5 100644 --- a/lib/review-targeted-delta.ts +++ b/lib/review-targeted-delta.ts @@ -7,7 +7,9 @@ * asks this module when the loop is done and the head has targeted classes the passed --targeted-ran no longer covers. * It dispatches one targeted reader on HEAD only when range_base is the commit that read happened on (its targeted * patch-id is --targeted-ran), every path whose branch patch changed since is docs class and not binary, and the change - * is not plain docs. Anything else keeps `done` with the reason. + * is not plain docs. With the owner's review_loop_grant for a stop head inside range_base..HEAD (`granted`, checked by the + * plan before it gets here), a code change since the read takes the same one reader (GST-1607, harness-evals #271: the + * granted fix round moved the product patch the record names). Anything else keeps `done` with the reason. */ import { spawnSync } from 'node:child_process'; import { classifyPath } from './review-depth'; @@ -17,7 +19,7 @@ import { plainDocsSince } from './ship-receipt-depth'; export type TargetedDelta = { state: 'dispatch'; range_base: string; paths: string[] } | { state: 'skip'; reason: string }; /** Whether the change since `rangeBase` (where the targeted reader read the patch `ran`) earns the targeted-only read. */ -export function targetedDelta(root: string, base: string, rangeBase: string, ran: string): TargetedDelta { +export function targetedDelta(root: string, base: string, rangeBase: string, ran: string, granted = false): TargetedDelta { const git = (args: string[], input?: string) => { const r = spawnSync('git', ['-c', 'core.quotePath=false', ...args], { cwd: root, input, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); return r.status === 0 ? r.stdout : null; @@ -41,7 +43,7 @@ export function targetedDelta(root: string, base: string, rangeBase: string, ran if (!paths.length) return skip(`the branch patch is unchanged since range_base ${read.slice(0, 12)}`); const binary = new Set((git(['diff', '--numstat', '-z', '--no-renames', headBase, head]) ?? '').split('\0').filter(l => l.startsWith('-\t-\t')).map(l => l.slice(4))); const other = paths.filter(p => classifyPath(p, root) !== 'docs' || binary.has(p)); - if (other.length) return skip(`the change since range_base ${read.slice(0, 12)} is not docs-only (${other.join(', ')}), so it takes the review loop`); + if (other.length && !granted) return skip(`the change since range_base ${read.slice(0, 12)} is not docs-only (${other.join(', ')}), so it takes the review loop`); if (plainDocsSince(root, read, head, base)) return skip(`the change since range_base ${read.slice(0, 12)} is plain docs, which the targeted record carries (GST-1195)`); return { state: 'dispatch', range_base: read, paths }; } diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index e99104884..afffac3e5 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -227,7 +227,7 @@ NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: ~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` -If write refuses `review_targeted` for a docs claim added after the read, rerun Step 9's plan with `--range-base +If write refuses `review_targeted` for a docs claim added after the read, or after a granted fix round (add its `--grant`), rerun Step 9's plan with `--range-base --targeted-ran `; on `NEXT: targeted`, run that one reader (`pass`, its voices stage, `verify --targeted-out .gstack/tmp/targeted.json`) and write again. A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index b74d6ec78..d89ace81e 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -210,7 +210,7 @@ NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: ~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` -If write refuses `review_targeted` for a docs claim added after the read, rerun Step 9's plan with `--range-base +If write refuses `review_targeted` for a docs claim added after the read, or after a granted fix round (add its `--grant`), rerun Step 9's plan with `--range-base --targeted-ran `; on `NEXT: targeted`, run that one reader (`pass`, its voices stage, `verify --targeted-out .gstack/tmp/targeted.json`) and write again. A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue diff --git a/test/review-pass.test.ts b/test/review-pass.test.ts index bc6250369..f6e4503cc 100644 --- a/test/review-pass.test.ts +++ b/test/review-pass.test.ts @@ -239,6 +239,27 @@ describe('the targeted reader in a pass (GST-257 on main)', () => { }, 180_000); }); +describe('the targeted-only read in the Capy pass program (GST-1598, GST-1607)', () => { + // Value: protects=a NEXT: targeted manifest driven through the checked-in pass program runs its one targeted reader (voices stage) + // and ends on NEXT: judge; fails_when=the program runs only first, specialists and red-team for a targeted manifest, so it creates + // no reader (lib/capy-review-pass.js activated voices only for fix-check); seam=lib/capy-review-pass.js stage selection; + // no_claim=what the reader finds + test('a targeted-only manifest creates the one targeted reader', async () => { + const { dir, env } = repo(); + const sh = (c: string) => { const r = spawnSync('bash', ['-c', c], { cwd: dir, env, encoding: 'utf8', timeout: 60_000 }); if (r.status !== 0) throw new Error(r.stderr); return r.stdout; }; + sh("printf 'export const spendCapUsd = 25;\\n' > src/billing.ts && git add -A && git commit -qm money"); + const read = sh('git rev-parse HEAD').trim(); + const plan = spawnSync(path.join(BIN, 'gstack-review-plan'), ['--base', 'main', '--gate-tagged', '0', '--json'], { cwd: dir, env, encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); + const ran = JSON.parse(plan.stdout).targeted.targeted_patch_id as string; + sh("mkdir -p changelog.d && echo '- Each PR eval run now costs about $4.20.' > changelog.d/cost.md && git add -A && git commit -qm claim"); + const h = host(dir, env); + const res = await runPass(h, { base: 'main', out: path.join(dir, '.state/run/p1'), pass: 'td1p1', planFlags: ['--gate-tagged', '0', '--pass', '1', '--last', 'fix-check', '--fixed-below', '--range-base', read, '--targeted-ran', ran] }); + expect(res).toContain('NEXT: targeted'); + expect(h.creates.map(c => c.title)).toEqual(['ship td1p1 targeted']); + expect(res).toContain('NEXT: judge'); + }, 180_000); +}); + describe('a large diff (GST-507)', () => { // Value: protects=a pass on a results PR with thousands of files reads gstack-review-plan's whole JSON plan through the pipe and stages its readers; fails_when=either bin exits before its stdout drains, so the plan JSON arrives cut short and pass dies on JSON.parse; // why_new=HARN-1 hit it on a 4,000-file receipt tree (a 1.39 MB plan); seam=none; no_claim=what reviewers read on a large diff (GST-508) diff --git a/test/review-plan-granted-targeted.test.ts b/test/review-plan-granted-targeted.test.ts new file mode 100644 index 000000000..224341bc2 --- /dev/null +++ b/test/review-plan-granted-targeted.test.ts @@ -0,0 +1,99 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { parseReviewTargeted, plainDocsSince, targetedClassesSince } from '../lib/ship-receipt-depth'; + +const BIN = path.join(import.meta.dir, '..', 'bin'); +const dirs: string[] = []; +afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); }); + +const money = (cap: number) => `export const spendCapUsd = ${cap};\nexport function charge(amountUsd: number) { if (amountUsd > spendCapUsd) throw new Error("over cap"); return amountUsd; }\n`; + +/** + * The harness-evals #271 shape: the targeted reader read R, the loop stopped at the cap on S (a fix after R), the owner + * granted one scoped fix-check naming S, and the granted round's fix commit moved HEAD past S. + */ +function granted(file: string, body: (n: number) => string) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'granted-targeted-')); + const state = fs.mkdtempSync(path.join(os.tmpdir(), 'granted-targeted-state-')); + dirs.push(dir, state); + const env: NodeJS.ProcessEnv = { ...process.env, GSTACK_STATE_ROOT: state, GSTACK_APPROVALS_MIRROR: '0' }; + delete env.GSTACK_APPROVALS_DIR; + const git = (...a: string[]) => spawnSync('git', a, { cwd: dir, encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + const commit = (files: Record, msg: string) => { + for (const [p, text] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(dir, p)), { recursive: true }); fs.writeFileSync(path.join(dir, p), text); } + git('add', '.'); git('commit', '-qm', msg); + return git('rev-parse', 'HEAD'); + }; + git('init', '-q', '-b', 'main'); git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); git('config', 'commit.gpgsign', 'false'); + commit({ 'README.md': '# x\n' }, 'base'); + git('checkout', '-qb', 'feat'); + const read = commit({ [file]: body(25) }, 'feature'); + const plan = (args: string[]) => spawnSync(path.join(BIN, 'gstack-review-plan'), ['--base', 'main', '--gate-tagged', '0', ...args], { cwd: dir, encoding: 'utf8', timeout: 60_000, env }); + const patchOf = () => JSON.parse(plan(['--json']).stdout).targeted.targeted_patch_id as string; + const readPatch = patchOf(); + const stop = commit({ [file]: body(30) }, 'round 2 fix'); + const add = spawnSync(path.join(BIN, 'gstack-approvals'), ['add', '--ask', `Owner grant: one scoped fix-check after round 2 (review_loop_grant ${stop.slice(0, 12)})`, '--words', 'do it', '--by', 'GStack Minder', '--scope', 'this /ship invocation', '--json'], { cwd: dir, encoding: 'utf8', timeout: 60_000, env }); + expect(add.status, add.stderr).toBe(0); + const grant = JSON.parse(add.stdout).added[0] as string; + const head = commit({ [file]: body(35) }, 'granted round fix'); + const doneArgs = (ran: string) => ['--pass', '2', '--last', 'fix-check', '--fix-checks', '1', '--range-base', read, '--targeted-ran', ran]; + return { dir, read, readPatch, head, grant, plan, patchOf, doneArgs }; +} +const dispatches = (stdout: string) => stdout.split('\n').filter(l => l.startsWith('DISPATCH: ')); + +describe('GST-1607: a granted fix round after the targeted read gets one targeted-only reader', () => { + // Value: protects=the #271 shape has an exit: after the granted fix-check the loop is done, the granted round's fix moved the + // product patch the targeted record names, and with that grant the plan dispatches exactly one targeted reader for the head's + // classes, then plans done once its id comes back; the receipt refuses the record read before the fix and binds the one read + // at HEAD; fails_when=the plan prints NEXT: done with no reader (harness-evals #271), or keeps dispatching after the read; + // seam=bin/gstack-review-plan, parseReviewTargeted with the receipt's own carry predicates; no_claim=what the reader finds + test('the #271 shape: one targeted reader under the grant, then the receipt binds its record', () => { + const r = granted('src/billing.ts', money); + const p = r.plan([...r.doneArgs(r.readPatch), '--grant', r.grant]); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toContain(`NEXT: targeted (the loop is done and the owner's grant ${r.grant}`); + expect(dispatches(p.stdout)).toEqual(['DISPATCH: targeted targeted effort=full classes=money']); + expect(p.stdout).toContain(`TARGETED_DELTA: range_base=${r.read} paths=src/billing.ts classes=money`); + const after = r.plan([...r.doneArgs(r.patchOf()), '--grant', r.grant]); + expect(after.stdout).toContain('NEXT: done'); + expect(dispatches(after.stdout)).toEqual([]); + const record = (at: string) => ({ classes: ['money'], rules: 'r', brief_sha: 'a'.repeat(64), status: 'done', findings: 0, raised_by: 'targeted', head: at }); + const productOf = (c: string) => spawnSync('git', ['patch-id', '--verbatim'], { input: spawnSync('git', ['diff', 'main', c], { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout, cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout.split(' ')[0]; + const bind = (at: string) => parseReviewTargeted(record(at), productOf(r.head)!, productOf, from => targetedClassesSince(r.dir, from, r.head, 'main'), from => plainDocsSince(r.dir, from, r.head, 'main')); + expect(bind(r.read).ok).toBe(false); + expect(bind(r.head)).toMatchObject({ ok: true, targeted: { classes: ['money'], status: 'done' } }); + }); + + // Value: protects=a granted round on a head with no targeted class adds no reader; fails_when=the grant alone dispatches the targeted + // reader; seam=bin/gstack-review-plan; no_claim=the fix-check itself + test('a granted fix on a head with no targeted class adds no reader', () => { + const r = granted('src/format.ts', n => `export const width = ${n};\nexport const pad = (s: string) => s.padEnd(width);\n`); + const p = r.plan([...r.doneArgs(r.readPatch), '--grant', r.grant]); + expect(p.status, p.stderr).toBe(0); + expect(p.stdout).toContain('TARGETED: classes=none'); + expect(p.stdout).toContain('NEXT: done'); + expect(dispatches(p.stdout)).toEqual([]); + }); + + // Value: protects=without the owner's grant a code change after the read keeps today's route; fails_when=a code change alone + // dispatches the targeted-only reader; seam=bin/gstack-review-plan; no_claim=how finish routes it + test('the same code change with no grant keeps done', () => { + const r = granted('src/billing.ts', money); + const p = r.plan(r.doneArgs(r.readPatch)); + expect(p.stdout).toContain('NEXT: done'); + expect(dispatches(p.stdout)).toEqual([]); + expect(p.stdout).toContain('SKIP: targeted-only read (the change since range_base'); + }); + + // Value: protects=a grant the approval record does not hold refuses before any plan; fails_when=a fabricated --grant prints a plan or + // dispatches the reader; seam=bin/gstack-review-plan's grant check; no_claim=who may grant + test('a fabricated grant refuses', () => { + const r = granted('src/billing.ts', money); + const p = r.plan([...r.doneArgs(r.readPatch), '--grant', 'a-000000000000']); + expect([p.status, p.stdout]).toEqual([3, '']); + expect(p.stderr).toContain('--grant a-000000000000'); + }); +}); diff --git a/test/size-ledger/ship/gst-1607-granted-targeted.json b/test/size-ledger/ship/gst-1607-granted-targeted.json new file mode 100644 index 000000000..ef0296779 --- /dev/null +++ b/test/size-ledger/ship/gst-1607-granted-targeted.json @@ -0,0 +1,4 @@ +{ + "bytes": 50, + "why": "GST-1607: Step 19 names the --grant for the targeted-only read after a granted fix round" +}