mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 06:24:41 +01:00
fix(review-plan): diff scope from origin/<base>; a scope error selects every scope-gated specialist
Without a local base branch gstack-diff-scope <base> 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>
This commit is contained in:
1 parent
193e8e9651
commit
0d86763399
4 files changed
+24
-4
No files matched your search
+3
-2
@@ -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 => {
|
||||
|
||||
@@ -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 });
|
||||
|
||||
@@ -11,6 +11,15 @@ const none = scanNumbers({});
|
||||
const base = (o: Partial<PlanInput>): 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<typeof planReview>) => 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'],
|
||||
|
||||
@@ -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/<base> like collectLive, so a clone with no local base branch keeps its scope-gated specialists; fails_when=gstack-diff-scope <base> 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/<base> 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');
|
||||
|
||||
Reference in new issue
Block a user