From 2e8ed2a918366157ab05e6331844453a0385cb9e Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 08:36:51 +0000 Subject: [PATCH] fix: plan the full pass 2 when the fix range touches a gate path or is empty (GSTA-152 PR B review) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- bin/gstack-review-plan | 11 +++++++---- docs/review-policy.md | 5 +++-- lib/review-depth.ts | 6 ++++-- test/review-depth.test.ts | 6 +++++- test/review-plan-cli.test.ts | 13 +++++++++++++ 6 files changed, 33 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4cd82205..8f6253ddb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ - **Docs**: docs/ship-budget.md (linked from docs/unattended.md), and a paragraph each in the Capy skill's SKILL.md and captain.md. ### Changed -- **`gstack-review-plan --range-base `** (`lib/review-depth.ts`): counts the committed fix range `..HEAD` the way the depth counts runtime lines, prints `FIX_RANGE: runtime_lines=|unread () base=` (`fix_range` in `--json`), and `NEXT: scoped` when the scoped pass applies. `nextStep` takes the depth and `History.rangeRuntimeLines`; `SCOPED_RANGE_LINES` (150) joins `DEPTH_RULES`, so `rules=` changes. `REVIEW_PLAN:` ends with the step's `reason="…"`. Uncommitted or untracked edits, a range base that is not an ancestor of HEAD, and a range the classifier cannot count all plan the full pass. +- **`gstack-review-plan --range-base `** (`lib/review-depth.ts`): counts the committed fix range `..HEAD` the way the depth counts runtime lines, prints `FIX_RANGE: runtime_lines=|unread () base=` (`fix_range` in `--json`), and `NEXT: scoped` when the scoped pass applies. `nextStep` takes the depth and `History.rangeRuntimeLines`; `SCOPED_RANGE_LINES` (150) joins `DEPTH_RULES`, so `rules=` changes. `REVIEW_PLAN:` ends with the step's `reason="…"`. Uncommitted or untracked edits, a range base that is not an ancestor of HEAD, an empty range, and a range the classifier cannot count all plan the full pass. A range that touches a gate-class path (a revert of a gate edit included) plans the full pass without `--touched-gate` and sets `fix_range.touched_gate`. - **Scoped dispatches**: the fix-check, every force-included specialist, the Red Team, the native adversarial pass when the range changes runtime lines, and the outside voice when published numbers changed. Step 11's voices that have not run yet stay listed. A light diff's raise to standard applies to the scoped set only. - **Step 9** (`ship/sections/review-army.md`): the repeat decision passes `--range-base $PASS_HEAD` and maps `NEXT: scoped` to decision 3; a scoped pass logs a full-pass `review` row with `"scope":"scoped"`, so it counts toward the cap and its roles land in `review_loop.dispatches` with no receipt schema change. - **`raised_by` on classified findings** (`gstack-review-scope classify`, `lib/review-scope.ts`): the reviewer that raised each finding (`core`, `qa`, a specialist, `red-team`, `adversarial-native`, `outside-voice`, `fix-check`), from the finding's `raised_by` or `specialist` tag, else `unknown`. Step 9 copies it into the review-log row. Older rows without it still summarize. diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 562e1e788..59df799af 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -107,7 +107,7 @@ function main(): number { const tiersSetting = runScriptSync(path.join(binDir, 'gstack-config'), ['get', 'review_effort_tiers'], { encoding: 'utf8', timeout: 15_000 }); const auditEffort = has('--no-tiers') || (tiersSetting.stdout ?? '').trim() === 'off' ? 'host' : 'light'; const { added, diffLines, paths, deleted, stampOnly, perPath, registry } = collected; - let fixRange: { base: string; runtime_lines: number | null; error?: string } | undefined; + let fixRange: { base: string; runtime_lines: number | null; error?: string; touched_gate?: true } | undefined; if (rangeBase) { const unread = (error: string) => ({ base: rangeBase, runtime_lines: null, error }); const resolved = git(['rev-parse', '--verify', '--quiet', `${rangeBase}^{commit}`])?.trim(); @@ -115,14 +115,17 @@ function main(): number { else if (git(['merge-base', '--is-ancestor', resolved, 'HEAD']) === null) fixRange = unread('not an ancestor of HEAD'); else if (git(['diff', '--quiet', 'HEAD', '--']) === null || collected.untracked.length) fixRange = unread('uncommitted or untracked edits: commit the fixes first'); else { - const range = classifyDiff({ ...collectCommit(root, resolved, 'HEAD'), registry, gateTagged: 0 }); - fixRange = range.errors.length ? unread(range.errors.map(e => `${e.code} ${e.input}`).join('; ')) : { base: rangeBase, runtime_lines: range.runtimeLines }; + const commit = collectCommit(root, resolved, 'HEAD'); + const range = classifyDiff({ ...commit, registry, gateTagged: 0 }); + fixRange = range.errors.length ? unread(range.errors.map(e => `${e.code} ${e.input}`).join('; ')) + : !commit.paths.length ? unread('the fix range is empty: commit the fix after the range base') + : { base: rangeBase, runtime_lines: range.runtimeLines, ...(range.class === 'gate' ? { touched_gate: true as const } : {}) }; } } const plan = planReview({ ...collected, scope, gateTagged, numbers: scanNumbers(added), force, gated: list('--gated'), floor, criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), - history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below'), blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate'), rangeRuntimeLines: fixRange?.runtime_lines }, + history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below'), blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate, rangeRuntimeLines: fixRange?.runtime_lines }, }); const gateScan = { tagged: gateTagged, head }; if (has('--json')) { diff --git a/docs/review-policy.md b/docs/review-policy.md index 4b71f9e85..a51cc458e 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -134,8 +134,9 @@ force-included specialist and the outside voice when published numbers changed. Step 11's voices that have not run yet stay listed, as on a full pass 2. It writes a full-pass `review` row with `"scope":"scoped"`, so it counts toward the cap, and a blocker it leaves open stops for the owner as before. A fix that -touched a gate-class path, a larger range, or a range, classifier or depth it -cannot read keeps the full pass 2. The 150-line figure is a starting value from +touched a gate-class path (read from the range itself, so a revert of a gate +edit counts without `--touched-gate`), a larger range, an empty range, or a +range, classifier or depth it cannot read keeps the full pass 2. The 150-line figure is a starting value from a 24-PR backtest (GSTA-152 D6), to revisit once classified findings carry `raised_by`. diff --git a/lib/review-depth.ts b/lib/review-depth.ts index 6d796717a..62094ed44 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -354,14 +354,16 @@ export function planReview(i: PlanInput): ReviewPlan { } } else if (next.step === 'scoped') { add('fix-check', 'fix-check', 'full'); - for (const n of SPECIALISTS.filter(forcedBy(i))) add('specialist', n, 'full'); + const forced = SPECIALISTS.filter(forcedBy(i)); + for (const n of forced) add('specialist', n, 'full'); + skipped.push(forced.length ? 'specialists (scoped pass; forced ones dispatched)' : 'specialists (scoped pass)'); add('red-team', 'red-team', 'full'); const stepElevenPending = !i.adversarialRan && cls === 'code'; if ((h.rangeRuntimeLines ?? 0) > 0 || stepElevenPending) add('adversarial-native', 'adversarial', 'full'); else skipped.push('adversarial-native (scoped pass: the fix range changes no runtime lines)'); if (stepElevenPending) add('outside-voice', 'outside-voice', 'host'); else if (numbersChanged) add('outside-voice', 'outside-voice (numbers changed)', 'host'); - else skipped.push('specialists and outside-voice (scoped pass)'); + else skipped.push('outside-voice (scoped pass)'); } else if (next.step === 'fix-check') { add('fix-check', 'fix-check', 'full'); if (numbersChanged) add('outside-voice', 'outside-voice (numbers changed)', 'host'); diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index 3e934da21..d7f10c1ab 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -307,7 +307,11 @@ describe('D6: the scoped second pass after a blocking fix (GSTA-152 PR B)', () = expect(p.dispatches.every(d => d.effort === 'full' && d.exclude.length === 0)).toBe(true); }); test('Step 11 voices that have not run yet stay listed, as on a full pass 2', () => { - expect(r(planReview(n19({ adversarialRan: false })))).toEqual(['fix-check:fix-check', 'red-team:red-team', 'adversarial-native:adversarial', 'outside-voice:outside-voice']); + const p = planReview(n19({ adversarialRan: false })); + expect(r(p)).toEqual(['fix-check:fix-check', 'red-team:red-team', 'adversarial-native:adversarial', 'outside-voice:outside-voice']); + expect(p.skipped).toContain('specialists (scoped pass)'); + expect(planReview(n19()).skipped).toEqual(expect.arrayContaining(['specialists (scoped pass)', 'outside-voice (scoped pass)'])); + expect(planReview(n19({ force: ['security'] })).skipped).toContain('specialists (scoped pass; forced ones dispatched)'); }); test.each([ ['full depth (a gate tag)', { gateTagged: 1 }], diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index 6817dcd60..346e59431 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -194,6 +194,19 @@ describe('gstack-review-plan: the scoped second pass (GSTA-152 D6)', () => { expect(dirty).toMatch(/^NEXT: full /m); expect(run(r, [...pass2, '--range-base']).status).toBe(2); }); + test('a fix range that touches a gate path keeps the full pass 2 without --touched-gate, and an empty range is unread', () => { + const r = repo({ 'src/a.ts': code(80), '.github/workflows/ci.yml': 'on: push\n' }); + const git = (...a: string[]) => spawnSync('git', a, { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }); + const rangeBase = git('rev-parse', 'HEAD').stdout.trim(); + expect(run(r, [...pass2, '--range-base', rangeBase]).stdout).toMatch(/^FIX_RANGE: unread \(the fix range is empty: .*\) base=/m); + git('rm', '-q', '.github/workflows/ci.yml'); + fs.writeFileSync(path.join(r.dir, 'src', 'b.ts'), code(2, 'b')); + git('add', '.'); git('commit', '-qm', 'fix: revert the workflow edit'); + const out = run(r, [...pass2, '--range-base', rangeBase]).stdout; + expect(out).toMatch(/^REVIEW_DEPTH: standard /m); + expect(out).toMatch(/^NEXT: full \(a blocking finding was fixed and a fix touched a gate-class path/m); + expect(JSON.parse(run(r, [...pass2, '--range-base', rangeBase, '--json']).stdout)).toMatchObject({ next: { step: 'full' }, fix_range: { touched_gate: true } }); + }); test('--depth-floor full opts out of the scoped pass', () => { const { r, rangeBase } = fixed(10); expect(run(r, [...pass2, '--range-base', rangeBase, '--depth-floor', 'full']).stdout).toMatch(/^NEXT: full \(a blocking finding was fixed and the diff is at full depth/m);