From 67f43b843583976fb81a4a68056163b651637a96 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 15:00:23 +0000 Subject: [PATCH] fix: pre-landing review fix (GST-967 round 2, granted a-a09a908e77c2): a grant names review_loop_grant and its stop head, bound by --range-base Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-plan | 14 +++++---- changelog.d/gst-967-gate-voices.md | 4 +-- docs/review-policy.md | 10 ++++--- ship/sections/review-army.md | 2 +- ship/sections/review-army.md.tmpl | 2 +- test/review-depth.test.ts | 2 +- test/review-packets-removed.test.ts | 2 +- test/review-pass.test.ts | 5 ++-- test/review-plan-cli.test.ts | 30 ++++++++++++------- .../size-ledger/ship/gst-967-gate-voices.json | 2 +- 10 files changed, 44 insertions(+), 29 deletions(-) diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 1ac958ca3..793a4d1f3 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -56,9 +56,9 @@ const HELP = `gstack-review-plan — the review depth of the next /ship step (do --targeted-ran the TARGETED line's targeted_patch_id (the whole committed diff, tests included) when the targeted reader last read it: the same patch skips it; any other id, none, or uncommitted edits dispatch it again --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_exception): the stop after a round-2 full pass with a fixed blocking - finding or a gate-class edit plans that fix-check instead; a fix-check never earns another; an id the - branch's record does not hold that way is refused (exit 3) + 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 + gate-class edit plans that fix-check instead; a fix-check never earns another; any other row is refused (exit 3) --fix-check-prompt print the fix-check reviewer's prompt from --fix-list, --hunks, --tests and --callers files Depth (R51, risk not size): full on gate class, a risk class (money, auth, secret, security, data) or any DEPTH_ERROR; @@ -102,13 +102,17 @@ function main(): number { if (has('--grant') && (!grant || grant.startsWith('-'))) { process.stderr.write('gstack-review-plan: --grant needs the approval id gstack-approvals add printed\n'); return EXIT.usage; } const rangeBase = flag('--range-base'); if (has('--range-base') && (!rangeBase || rangeBase.startsWith('-'))) { process.stderr.write('gstack-review-plan: --range-base needs the fix range\'s base commit (this pass\'s PASS_HEAD)\n'); return EXIT.usage; } + if (grant && !rangeBase) { process.stderr.write('gstack-review-plan: --grant needs --range-base (round 2\'s PASS_HEAD), which binds the grant to the stop it answers\n'); return EXIT.usage; } const root = git(['rev-parse', '--show-toplevel'])?.trim(); if (!root) { process.stderr.write('gstack-review-plan: run it inside a git checkout\n'); return EXIT.fail; } process.chdir(root); if (grant) { const loc = locate({ cwd: root, env: process.env }); - const rows = 'file' in loc ? readRecord(loc).rows : []; - if (!rows.some(r => r.id === grant && r.outcome === 'approved' && `${r.ask} ${r.scope}`.includes('review_loop_exception'))) { process.stderr.write(`gstack-review-plan: --grant ${grant}: no approved row ${grant} naming review_loop_exception in this branch's approval record (${'file' in loc ? 'gstack-approvals list' : loc.message}); record the owner's grant with gstack-approvals add --ask '... (review_loop_exception)' first\n`); return EXIT.refused; } + const row = ('file' in loc ? readRecord(loc).rows : []).find(r => r.id === grant && r.outcome === 'approved' && `${r.ask} ${r.scope}`.includes('review_loop_grant')); + const passHead = git(['rev-parse', '--verify', '--quiet', `${rangeBase}^{commit}`])?.trim(); + const ancestor = (a: string, d: string) => git(['merge-base', '--is-ancestor', a, d]) !== null; + const stopHead = row && passHead ? `${row.ask} ${row.scope}`.match(/\b[0-9a-f]{7,40}\b/g)?.map(s => git(['rev-parse', '--verify', '--quiet', `${s}^{commit}`])?.trim()).find(s => s && ancestor(passHead, s) && ancestor(s, 'HEAD')) : undefined; + if (!stopHead) { process.stderr.write(`gstack-review-plan: --grant ${grant}: no approved row ${grant} naming review_loop_grant and this invocation's stop head (a commit at or after --range-base ${rangeBase}, at or before HEAD) in this branch's approval record (${'file' in loc ? 'gstack-approvals list' : loc.message}); record the owner's grant with gstack-approvals add --ask '... (review_loop_grant )' first\n`); return EXIT.refused; } } const collected = collectLive(root, base); if (collected.errors.length) { diff --git a/changelog.d/gst-967-gate-voices.md b/changelog.d/gst-967-gate-voices.md index 7ae04b0e1..2e10d4922 100644 --- a/changelog.d/gst-967-gate-voices.md +++ b/changelog.d/gst-967-gate-voices.md @@ -1,8 +1,8 @@ -**/ship now runs Step 11's voices before any gate-class stop, and a recorded owner grant runs its fix-check.** On a gate-class change, a blocker fixed in review round 2 used to stop the run for the owner before Step 11 ran. If the owner then chose "ship with open notes", gate code shipped with no adversarial, outside or targeted review (C6-36 after-opus). `gstack-review-plan` now answers `NEXT: voices` for any gate-class stop while Step 11 has not run. /ship runs Step 11 as it does on `done`, asks the plan again with `--adversarial-ran`, and the stop then stands. The round cap still limits fix rounds; it no longer skips the required voices. Second, the owner's grant of one scoped fix-check after round 2 had no way in: the plan took no grant input, and the pass program printed `PASS: none next=stop`. Now `gstack-review-plan --grant ` checks that the branch's `gstack-approvals` record holds that id as an approved grant naming `review_loop_exception` (exit 3 for any other row), and plans the one fix-check in place of the stop after a round-2 full pass. The pass program then dispatches that fix-check like any other. A fix-check never earns another, so the grant cannot repeat. After `NEXT: voices`, a blocking finding the voices queue goes back to the plan as `--blocking-open` instead of starting a new review pass. +**/ship now runs Step 11's voices before any gate-class stop, and a recorded owner grant runs its fix-check.** On a gate-class change, a blocker fixed in review round 2 used to stop the run for the owner before Step 11 ran. If the owner then chose "ship with open notes", gate code shipped with no adversarial, outside or targeted review (C6-36 after-opus). `gstack-review-plan` now answers `NEXT: voices` for any gate-class stop while Step 11 has not run. /ship runs Step 11 as it does on `done`, asks the plan again with `--adversarial-ran`, and the stop then stands. The round cap still limits fix rounds; it no longer skips the required voices. Second, the owner's grant of one scoped fix-check after round 2 had no way in: the plan took no grant input, and the pass program printed `PASS: none next=stop`. Now `gstack-review-plan --grant ` checks that the branch's `gstack-approvals` record holds that id as an approved grant naming `review_loop_grant` and this run's stop head (exit 3 for any other row, a grant recorded for an earlier /ship on the branch included), and plans the one fix-check in place of the stop after a round-2 full pass. The pass program then dispatches that fix-check like any other. A fix-check never earns another, so the grant cannot repeat. After `NEXT: voices`, a blocking finding the voices queue goes back to the plan as `--blocking-open` instead of starting a new review pass. ### Tests - `test/review-depth.test.ts`: a gate-class round-2 blocker fix plans `voices` (adversarial, outside and targeted) before every stop and stops once they ran. A grant plans one fix-check after round 2, after the voices on gate class, and never a second. The transitions table and rule 3 in `docs/review-policy.md` say both. All four failed at main 1022f4f. -- `test/review-plan-cli.test.ts`: `--grant` with a recorded grant prints `NEXT: fix-check`; an unrelated approval or an unrecorded id exits 3; after a fix-check the grant never plans another. Failed at main 1022f4f (`NEXT: stop`). +- `test/review-plan-cli.test.ts`: `--grant` with a recorded grant prints `NEXT: fix-check`; an unrelated approval, an unrecorded id, a `review_loop_exception` acceptance row or a grant for an earlier head exits 3; after a fix-check the grant never plans another. Failed at main 1022f4f (`NEXT: stop`). - `test/review-depth.test.ts`: an invariant over every transitions-table state (gate class or not, Step 11 run or not, grant or not) and a dispatch-superset no-loss check against main's plans. - `test/review-pass.test.ts`: the pass program runs the granted fix-check reader instead of `PASS: none next=stop`. Failed at main 1022f4f. diff --git a/docs/review-policy.md b/docs/review-policy.md index 5d2d40e82..21c8af115 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -104,11 +104,13 @@ incomplete coverage, which blocks until the reviewer is rerun. notes" without them (GST-967). A third round is the owner's grant of one scoped fix-check (recorded in `review_loop_exception`), never the loop's: once `gstack-approvals add` records the grant (its ask or scope - naming `review_loop_exception`), `gstack-review-plan --grant ` + naming `review_loop_grant` and the stop head's sha), + `gstack-review-plan --grant --range-base ` plans that fix-check in place of the stop after a round-2 full pass with a - fixed blocking finding or a gate-class edit, and refuses any other row. A - fix-check never earns another, so the grant cannot repeat, and a stop after a - round-2 fix-check still goes to the owner. The cap approves nothing. + fixed blocking finding or a gate-class edit, and refuses any other row, + including a grant for a stop head before `--range-base` (an earlier /ship). + A fix-check never earns another, so the grant cannot repeat, and a stop after + a round-2 fix-check still goes to the owner. The cap approves nothing. ### Transitions diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index fa5cddae0..54997ac49 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -821,7 +821,7 @@ with each flag that holds (`--fixed-blocking`, `--fixed-below`, `--blocking-open `--touched-gate` for a fix to an item 2.5 path, `--critical-found` when this pass logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `, `--depth-floor ` when given, `--grant ` once the owner's grant of one scoped fix-check is -in `gstack-approvals` naming `review_loop_exception`), `--range-base $PASS_HEAD` and `--pass1-depth `; +in `gstack-approvals` naming `review_loop_grant` and the stop head), `--range-base $PASS_HEAD` and `--pass1-depth `; its `NEXT:` alone picks the next decision: `stop` 2, `full` 3, `fix-check` 3.5, `done` 4, `voices` 4 (Step 11 runs before the stop). `scoped` is 3 too. A full pass 1 always gets a full pass 2. Other scopes apply decisions 2–4 in order. diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 198512d0e..a5ed62ba2 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -230,7 +230,7 @@ with each flag that holds (`--fixed-blocking`, `--fixed-below`, `--blocking-open `--touched-gate` for a fix to an item 2.5 path, `--critical-found` when this pass logged a CRITICAL, `--adversarial-ran` once Step 11's native pass ran, `--reviewed-numbers `, `--depth-floor ` when given, `--grant ` once the owner's grant of one scoped fix-check is -in `gstack-approvals` naming `review_loop_exception`), `--range-base $PASS_HEAD` and `--pass1-depth `; +in `gstack-approvals` naming `review_loop_grant` and the stop head), `--range-base $PASS_HEAD` and `--pass1-depth `; its `NEXT:` alone picks the next decision: `stop` 2, `full` 3, `fix-check` 3.5, `done` 4, `voices` 4 (Step 11 runs before the stop). `scoped` is 3 too. A full pass 1 always gets a full pass 2. Other scopes apply decisions 2–4 in order. diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index 002da4b4c..a4fde116c 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -107,7 +107,7 @@ describe('GST-967: the round cap limits fix rounds, not the gate-class voices; a expect(table).toContain('| a round-2 full pass that stops for a fixed blocking finding or a gate-class edit, with the owner\'s recorded grant (`--grant `) | one scoped fix-check (round 3), after Step 11 on gate class |'); const rule3 = doc.slice(doc.indexOf('3. **Two rounds is the cap**'), doc.indexOf('')).replace(/\s+/g, ' '); expect(rule3).toContain("The cap limits fix rounds, not Step 11's voices"); - expect(rule3).toContain('`gstack-review-plan --grant `'); + expect(rule3).toContain('`gstack-review-plan --grant --range-base `'); }); }); diff --git a/test/review-packets-removed.test.ts b/test/review-packets-removed.test.ts index aa1d856ab..5cd0bd0d4 100644 --- a/test/review-packets-removed.test.ts +++ b/test/review-packets-removed.test.ts @@ -65,7 +65,7 @@ const NO_LOSS_REPLAYS = { } as const; type Replay = keyof typeof NO_LOSS_REPLAYS; const REVIEWER_BRIEFS: Record = { - 'ship/sections/review-army.md': { sha: 'c22f1da7f5ce7ecfd8d83fdaa135ed2fd1207d7db6f04321bd92b1524fd70c94', replay: 'GST-967' }, + 'ship/sections/review-army.md': { sha: 'dd447aa0e3d84c0f7007c9e578480cc50ac274e6741ab743fafc187601207a70', replay: 'GST-967' }, 'ship/sections/adversarial.md': { sha: '5d712bb568573931108a79b1bd40b74db6189610f47c547099c13098ef67bef8', replay: 'GST-967' }, 'review/sections/review-army.md': { sha: '78329b64640260cd8831ca81a9189f3f8dcade9c406431631d3f2cd798cd3967', replay: '#3638' }, 'review/sections/adversarial.md': { sha: 'e6962fa25b78f362654f2954f79683e524d2ebd452d1494f8a2b099888e8e255', replay: 'G1' }, diff --git a/test/review-pass.test.ts b/test/review-pass.test.ts index 8f2fddc20..2e8ec1126 100644 --- a/test/review-pass.test.ts +++ b/test/review-pass.test.ts @@ -744,7 +744,8 @@ describe('Capy pass program (fake host, real bins)', () => { // why_new=GST-967; seam=the pass program; no_claim=the fix-check reviewer's verdict test('GST-967: a recorded grant after round 2 runs its one scoped fix-check through the program', async () => { const { dir, env } = repo(); - const add = spawnSync(path.join(BIN, 'gstack-approvals'), ['add', '--ask', 'Owner grant: one scoped fix-check after round 2 (review_loop_exception)', '--words', 'one scoped fix-check after round 2', '--by', 'GStack Minder', '--scope', 'this /ship invocation', '--json'], { cwd: dir, env: { ...env, GSTACK_APPROVALS_MIRROR: '0' }, encoding: 'utf8', timeout: 60_000 }); + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: dir, encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + const add = spawnSync(path.join(BIN, 'gstack-approvals'), ['add', '--ask', `Owner grant: one scoped fix-check after round 2 (review_loop_grant ${head.slice(0, 12)})`, '--words', 'one scoped fix-check after round 2', '--by', 'GStack Minder', '--scope', 'this /ship invocation', '--json'], { cwd: dir, env: { ...env, GSTACK_APPROVALS_MIRROR: '0' }, encoding: 'utf8', timeout: 60_000 }); expect(add.status, add.stderr).toBe(0); const id = JSON.parse(add.stdout).added[0] as string; const files = path.join(dir, '.state/gfc'); @@ -752,7 +753,7 @@ describe('Capy pass program (fake host, real bins)', () => { const fc: Record = {}; for (const [k, v] of [['fix_list', '1. src/a.ts: guard v80\n'], ['hunks', 'diff --git a/src/a.ts b/src/a.ts\n+export const v81 = 81;\n'], ['tests', 'bun test: 3 pass, 0 fail\n'], ['callers', 'none\n']]) { fc[k] = path.join(files, k + '.txt'); fs.writeFileSync(fc[k]!, v!); } const h = host(dir, env); - const res = await runPass(h, { base: 'main', out: path.join(dir, '.state/run/gfc'), pass: 'x30gfc', planFlags: ['--pass', '2', '--last', 'full', '--fixed-blocking', '--grant', id], fixCheck: fc, logStart: ['review-fix-check'] }); + const res = await runPass(h, { base: 'main', out: path.join(dir, '.state/run/gfc'), pass: 'x30gfc', planFlags: ['--pass', '2', '--last', 'full', '--fixed-blocking', '--range-base', head, '--grant', id], fixCheck: fc, logStart: ['review-fix-check'] }); expect(res).not.toContain('PASS: none'); expect(res).toContain(`NEXT: fix-check (the owner's grant ${id}`); expect(h.creates.map(c => c.title)).toEqual(['ship x30gfc fix-check']); diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index cd1dee108..1fe679b17 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -62,33 +62,41 @@ describe('gstack-review-plan', () => { expect(r.stdout).not.toContain('NEXT: stop'); expect(run(repo({ 'src/a.ts': 'export const a = 1;\n' }), ['--pass', '1', '--last', 'fix-check', '--fixed-below', '--blocking-open']).stdout).toContain('NEXT: stop'); }); - // Value: protects=a grant the owner recorded with gstack-approvals is something the plan can use: --grant plans the one scoped fix-check after a round-2 full pass, the fix-check never earns another, and an id the record does not hold as a review_loop_exception grant is refused; fails_when=the CLI ignores the grant and prints NEXT: stop (C6-36 after-opus-grant), accepts an unrelated approval or an unrecorded id, or grants a second fix-check; - // why_new=GST-967; seam=bin/gstack-review-plan; no_claim=who may grant (the owner's words are the approval record's) - test('GST-967: --grant names a recorded approval and plans its one scoped fix-check after round 2; an unrecorded id is refused', () => { + // Value: protects=a grant the owner recorded with gstack-approvals is something the plan can use, once, in the invocation it answers: --grant plans the one scoped fix-check after a round-2 full pass when the row names review_loop_grant and the stop head (at or after --range-base, at or before HEAD); the fix-check never earns another; an unrecorded id, an unrelated approval, a review_loop_exception acceptance row, or a grant for an earlier head is refused; fails_when=the CLI ignores the grant and prints NEXT: stop (C6-36 after-opus-grant), accepts any of those rows, or grants a second fix-check; + // why_new=GST-967 (round-2 red team: a grant from an earlier /ship on the branch could be reused); seam=bin/gstack-review-plan; no_claim=who may grant (the owner's words are the approval record's) + test('GST-967: --grant plans the one scoped fix-check for the stop head it names; any other row, or a grant for an earlier head, is refused', () => { const r = repo({ 'src/a.ts': 'export const a = 1;\n' }); const env: NodeJS.ProcessEnv = { ...process.env, GSTACK_STATE_ROOT: r.state, GSTACK_APPROVALS_MIRROR: '0' }; delete env.GSTACK_APPROVALS_DIR; + const git = (...a: string[]) => spawnSync('git', a, { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + const commit = (n: number) => { fs.appendFileSync(path.join(r.dir, 'src/a.ts'), `export const v${n} = ${n};\n`); git('commit', '-qam', `c${n}`); return git('rev-parse', 'HEAD'); }; + const stale = git('rev-parse', 'HEAD'); + const passHead = commit(1); + const stopHead = commit(2); const add = (ask: string) => { const a = spawnSync(path.join(path.dirname(BIN), 'gstack-approvals'), ['add', '--ask', ask, '--words', 'do it', '--by', 'GStack Minder', '--scope', 'this /ship invocation', '--json'], { cwd: r.dir, encoding: 'utf8', timeout: 60_000, env }); expect(a.status, a.stderr).toBe(0); return JSON.parse(a.stdout).added[0] as string; }; - const id = add('Owner grant: one scoped fix-check after round 2 (review_loop_exception)'); - const unrelated = add('Approve the plan for the README rewrite'); + const id = add(`Owner grant: one scoped fix-check after round 2 (review_loop_grant ${stopHead.slice(0, 12)})`); + const refusedIds = [add(`Owner grant from an earlier run (review_loop_grant ${stale.slice(0, 12)})`), add('Owner grant: one scoped fix-check (review_loop_grant)'), + add(`Owner accepts the stop after the fix-check (review_loop_exception ${stopHead.slice(0, 12)})`), add('Approve the plan for the README rewrite'), 'a-000000000000']; const plan = (a: string[]) => spawnSync(BIN, ['--base', 'main', '--gate-tagged', '0', ...a], { cwd: r.dir, encoding: 'utf8', timeout: 60_000, env }); - const capStop = ['--pass', '2', '--last', 'full', '--fixed-blocking']; + const capStop = ['--pass', '2', '--last', 'full', '--fixed-blocking', '--range-base', passHead]; expect(plan(capStop).stdout).toContain('NEXT: stop'); const granted = plan([...capStop, '--grant', id]); expect(granted.status, granted.stderr).toBe(0); expect(granted.stdout).toContain(`NEXT: fix-check (the owner's grant ${id}`); expect(granted.stdout).toContain('DISPATCH: fix-check fix-check effort=full'); - expect(plan(['--pass', '2', '--last', 'fix-check', '--fixed-blocking', '--grant', id]).stdout).toContain('NEXT: stop'); - expect(plan(['--pass', '1', '--last', 'fix-check', '--fixed-blocking', '--grant', id]).stdout).toContain('NEXT: stop'); - for (const other of ['a-000000000000', unrelated]) { + commit(3); + expect(plan([...capStop, '--grant', id]).stdout).toContain(`NEXT: fix-check (the owner's grant ${id}`); + expect(plan(['--pass', '2', '--last', 'fix-check', '--fixed-blocking', '--range-base', passHead, '--grant', id]).stdout).toContain('NEXT: stop'); + for (const other of refusedIds) { const refused = plan([...capStop, '--grant', other]); - expect([refused.status, refused.stdout]).toEqual([3, '']); - expect(refused.stderr).toContain(`no approved row ${other} naming review_loop_exception`); + expect([other, refused.status, refused.stdout]).toEqual([other, 3, '']); + expect(refused.stderr).toContain(`--grant ${other}`); } + expect(plan([...capStop.slice(0, -2), '--grant', id]).status).toBe(2); expect(plan([...capStop, '--grant', '--json']).status).toBe(2); }); test('--fix-check-prompt renders the fix-check reviewer prompt from the parent\'s files and refuses a missing one', () => { diff --git a/test/size-ledger/ship/gst-967-gate-voices.json b/test/size-ledger/ship/gst-967-gate-voices.json index d452e50fd..d0abeb43a 100644 --- a/test/size-ledger/ship/gst-967-gate-voices.json +++ b/test/size-ledger/ship/gst-967-gate-voices.json @@ -1,4 +1,4 @@ { - "bytes": 463, + "bytes": 477, "why": "GST-967: Step 11 voices before any gate-class stop (NEXT: voices) and the owner's recorded grant (--grant) in the repeat decision and Step 11" }