From 0d8676339983d11f5836dcec6f220c8d57c5b366 Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 30 Oct 2026 02:21:11 +0000 Subject: [PATCH] fix(review-plan): diff scope from origin/; a scope error selects every scope-gated specialist Without a local base branch gstack-diff-scope returned SCOPE_ERROR=no_base and the plan silently dropped security and performance (the depth already failed closed to full). The plan now resolves the base like collectLive, and an unreadable scope keeps every scope-gated specialist. Failing-first tests in test/review-depth.test.ts and test/review-plan-cli.test.ts. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-depth.ts | 5 +++-- lib/review-plan-live.ts | 4 ++-- test/review-depth.test.ts | 9 +++++++++ test/review-plan-cli.test.ts | 10 ++++++++++ 4 files changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/review-depth.ts b/lib/review-depth.ts index bce1fffef..6914a39c3 100644 --- a/lib/review-depth.ts +++ b/lib/review-depth.ts @@ -273,6 +273,7 @@ const forcedBy = (i: PlanInput) => (n: string) => i.force === 'all' || (Array.is /** The specialists a full pass selects for a class: class sets the eligible set, hit-rate gating applies inside it, gate class ignores gating. */ export function specialistsFor(cls: DiffClass, i: PlanInput, skipped: string[] = []): Specialist[] { const s = i.scope ?? {}; + const unknown = !!s.SCOPE_ERROR; const forced = forcedBy(i); if (cls === 'docs') { skipped.push('specialists (docs class)'); return SPECIALISTS.filter(forced); } if (cls === 'tests') { skipped.push('specialists except testing (tests class)'); return SPECIALISTS.filter(n => n === 'testing' || forced(n)); } @@ -282,9 +283,9 @@ export function specialistsFor(cls: DiffClass, i: PlanInput, skipped: string[] = if (i.diffLines < SMALL_DIFF_LINES) return false; switch (n) { case 'testing': case 'maintainability': return true; - case 'security': return s.SCOPE_AUTH === 'true' || (s.SCOPE_BACKEND === 'true' && i.diffLines > LARGE_DIFF_LINES); + case 'security': return unknown || s.SCOPE_AUTH === 'true' || (s.SCOPE_BACKEND === 'true' && i.diffLines > LARGE_DIFF_LINES); case 'simplification': return i.diffLines > LARGE_DIFF_LINES; - default: return scopeWants[n]!(s); + default: return unknown || scopeWants[n]!(s); } }; return SPECIALISTS.filter(n => { diff --git a/lib/review-plan-live.ts b/lib/review-plan-live.ts index 712beee09..3d04c072b 100644 --- a/lib/review-plan-live.ts +++ b/lib/review-plan-live.ts @@ -8,7 +8,7 @@ import * as path from 'node:path'; import { spawnSync } from 'node:child_process'; import { runScriptSync } from './script-invocation'; -import { collectCommit, collectLive, type Collected } from './review-collect'; +import { collectCommit, collectLive, resolveBaseRef, type Collected } from './review-collect'; import { classifyDiff, planReview, scanNumbers, SCOPED_RANGE_LINES, type Depth, type History, type ReviewPlan } from './review-depth'; export interface PlanFlags { @@ -90,7 +90,7 @@ export function livePlan(f: PlanFlags, binDir: string, bin: string): LivePlan | const m = /GATE_SUMMARY: .*?tagged=\{([^}]*)\}/.exec(g.stdout ?? ''); return g.status === 0 && m ? m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0) : null; }; - const scope = scopeOf([f.base]); + const scope = scopeOf([resolveBaseRef(root, f.base)]); let gateTagged = f.gateTagged !== undefined ? f.gateTagged : gateTagsOf([f.base]); if (gateTagged !== null && !Number.isFinite(gateTagged)) gateTagged = null; const tiersSetting = runScriptSync(path.join(binDir, 'gstack-config'), ['get', 'review_effort_tiers'], { encoding: 'utf8', timeout: 15_000 }); diff --git a/test/review-depth.test.ts b/test/review-depth.test.ts index d7f10c1ab..323175d21 100644 --- a/test/review-depth.test.ts +++ b/test/review-depth.test.ts @@ -11,6 +11,15 @@ const none = scanNumbers({}); const base = (o: Partial): PlanInput => ({ paths: ['src/app.ts'], scope: { SCOPE_BACKEND: 'true', SCOPE_STATE_MACHINE: 'false' }, gateTagged: 0, diffLines: 120, history: { fullPasses: 0, last: 'none' }, numbers: none, ...o }); const roles = (p: ReturnType) => p.dispatches.map(d => `${d.role}:${d.name}:${d.effort}`); +describe('a scope error fails the specialist selection closed', () => { + // Value: protects=an unreadable diff scope selects every scope-gated specialist instead of none; fails_when=SCOPE_ERROR leaves all SCOPE_* false and security/performance drop silently; + // why_new=GSTA-167 dogfood on #3633 (no local main); seam=none; no_claim=the scope tool's own errors + test('SCOPE_ERROR=no_base keeps security, performance, api-contract, data-migration and design', () => { + const names = planReview(base({ scope: { SCOPE_ERROR: 'no_base' }, diffLines: 1000 })).dispatches.filter(d => d.role === 'specialist').map(d => d.name); + for (const n of ['security', 'performance', 'api-contract', 'data-migration', 'design']) expect(names).toContain(n); + }); +}); + describe('classifyPath: agent-read markdown is code', () => { test.each([ ['README.md', 'docs'], ['CHANGELOG.md', 'docs'], ['docs/howto-x.md', 'docs'], ['TODOS.md', 'docs'], diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index 1f7bc9128..ed804d350 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -41,6 +41,16 @@ describe('gstack-review-plan', () => { expect(plan.audit_effort).toBe('light'); expect(JSON.parse(run(r, ['--json', '--no-tiers']).stdout).audit_effort).toBe('host'); }); + // Value: protects=the plan reads diff scope from origin/ like collectLive, so a clone with no local base branch keeps its scope-gated specialists; fails_when=gstack-diff-scope fails on a missing local ref and security/performance silently drop; + // why_new=GSTA-167 dogfood on #3633: a Capy checkout has no local main; seam=gstack-diff-scope; no_claim=which specialists a scope selects + test('no local base branch: scope comes from origin/ and the scope-gated specialists stay', () => { + const r = repo({ 'src/a.ts': Array.from({ length: 80 }, (_, i) => `export const v${i} = ${i};`).join('\n') + '\n' }); + const git = (...a: string[]) => spawnSync('git', a, { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }); + git('update-ref', 'refs/remotes/origin/main', 'main'); git('branch', '-D', 'main'); + const out = run(r, []).stdout; + expect(out).not.toContain('scope_error'); + expect(out).toContain('DISPATCH: specialist performance'); + }); test('a below-floor fix after pass 1 plans one fix-check', () => { const r = run(repo({ 'src/a.ts': 'export const a = 1;\n' }), ['--pass', '1', '--last', 'full', '--fixed-below']); expect(r.stdout).toContain('next=fix-check dispatches=1');