From 00dc899d34f089cfc79114f921d413df969fb792 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 16 Jun 2026 07:36:01 +0000 Subject: [PATCH] feat(impact): add opt-in mode param with hard validation (U1) Add mode:'callgraph'|'pdg' to the impact MCP tool. callgraph (default) is byte-identical; pdg routes to a stub (U3/U4). Backend hard-validates the enum (schema enum is advisory), rejects relationTypes/crossDepth/ minConfidence and @group targets under pdg, and forks the ambiguous branch so no callgraph BFS runs under pdg. CLI --mode wired. Refs U1 of docs/plans/2026-06-16-001-feat-pdg-impact-mode-and-accuracy-harness-plan.md --- gitnexus/src/cli/index.ts | 5 + gitnexus/src/cli/tool.ts | 4 + gitnexus/src/mcp/local/local-backend.ts | 196 +++++++++++++++++++ gitnexus/src/mcp/tools.ts | 9 + gitnexus/test/unit/calltool-dispatch.test.ts | 166 ++++++++++++++++ gitnexus/test/unit/tools.test.ts | 18 ++ 6 files changed, 398 insertions(+) diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 4ac78ae85..ee857c74d 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -338,6 +338,11 @@ program .command('impact [target]') .description('Blast radius analysis: what breaks if you change a symbol') .option('-d, --direction ', 'upstream (dependants) or downstream (dependencies)', 'upstream') + .option( + '--mode ', + 'Engine: callgraph (default) or pdg (opt-in, intra-procedural; needs analyze --pdg)', + 'callgraph', + ) .option('-r, --repo ', 'Target repository') .option('--branch ', 'Scope to a specific branch index (multi-branch repos)') .option('-u, --uid ', 'Direct symbol UID (zero-ambiguity lookup)') diff --git a/gitnexus/src/cli/tool.ts b/gitnexus/src/cli/tool.ts index 111bb4bbc..130f909bf 100644 --- a/gitnexus/src/cli/tool.ts +++ b/gitnexus/src/cli/tool.ts @@ -124,6 +124,7 @@ export async function impactCommand( target?: string, options?: { direction?: string; + mode?: string; repo?: string; branch?: string; uid?: string; @@ -168,6 +169,9 @@ export async function impactCommand( file_path: options?.file, kind: options?.kind, direction: options?.direction || 'upstream', + // Forward the engine selector; backend validates the enum (callgraph/pdg) + // and treats the default 'callgraph' identically to an omitted mode. + mode: options?.mode, maxDepth: options?.depth ? parseInt(options.depth, 10) : undefined, includeTests: options?.includeTests ?? false, repo: options?.repo, diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index c20433670..dd6d315e1 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -96,6 +96,32 @@ function resolveAliasString(canonical: unknown, legacy: unknown): string | undef } return undefined; } + +/** The two impact engines (KTD1). `'callgraph'` is the default/established path. */ +export type ImpactMode = 'callgraph' | 'pdg'; + +/** + * Validate the `impact` `mode` param (KTD5 — backend hard-gate). + * + * The MCP JSON-schema `enum` is advisory only (server.ts forwards args + * unvalidated and `callTool` is reachable directly), so this backend check is + * the real boundary — mirroring `_pdgQueryImpl`'s `mode` enum validation. A + * typo'd mode silently running callgraph is exactly the silent fallback this + * forbids (it would make the accuracy harness compare callgraph-vs-callgraph + * and report perfect parity). + * + * Absent / `undefined` / `'callgraph'` all resolve to `'callgraph'` (the + * unchanged default path). `'pdg'` is valid. Anything else — `'PDG'`, `'pgd'`, + * `''`, or a non-string (`0`, `null`, …) — returns a structured `{ error }`, + * never a callgraph result. + */ +function validateImpactMode(rawMode: unknown): { mode: ImpactMode } | { error: string } { + if (rawMode === undefined || rawMode === 'callgraph') return { mode: 'callgraph' }; + if (rawMode === 'pdg') return { mode: 'pdg' }; + return { + error: `Invalid "mode": expected "callgraph" or "pdg", got ${JSON.stringify(rawMode)}.`, + }; +} // AI context generation is CLI-only (gitnexus analyze) // import { generateAIContextFiles } from '../../cli/ai-context.js'; @@ -425,7 +451,15 @@ interface ImpactParams { file_path?: string; kind?: string; direction: 'upstream' | 'downstream'; + /** + * Blast-radius engine (KTD1/KTD5). Absent / `undefined` / `'callgraph'` → + * the unchanged inter-procedural symbol→symbol BFS. `'pdg'` → the opt-in, + * intra-procedural Program Dependence Graph traversal (`_runImpactPDG`). + * Validated in `_impactImpl`; any other value is a hard `{ error }`. + */ + mode?: ImpactMode; maxDepth?: number; + crossDepth?: number; relationTypes?: string[]; includeTests?: boolean; minConfidence?: number; @@ -4238,6 +4272,56 @@ export class LocalBackend { await this.ensureInitialized(repo); const { target, direction } = params; + + // ── Dispatch order (KTD5) ────────────────────────────────────────── + // (1) Validate `mode`. Absent/'callgraph' → unchanged path; 'pdg' → the + // intra-procedural PDG engine (stubbed in U1); anything else → hard error. + // This MUST come before resolveSymbolCandidates so the ambiguous branch can + // fork on the validated mode and never run the callgraph fan-out under pdg. + const modeResult = validateImpactMode(params.mode); + if ('error' in modeResult) { + return { + error: modeResult.error, + target: { name: target }, + direction, + impactedCount: 0, + risk: 'UNKNOWN', + }; + } + const mode = modeResult.mode; + + if (mode === 'pdg') { + // KTD12 — param-compatibility hard rejections (decided as errors, NOT + // silent ignores and NOT an `ignoredParams` echo). Each names a symbol- + // graph / cross-repo concept the PDG engine cannot honor: + // relationTypes → names symbol edges (PDG walks BasicBlock edges). + // crossDepth → cross-repo hops (PDG is single-repo intra-procedural). + // minConfidence → CDG/RD edges may carry no confidence → would drop all. + // A loud failure beats a quietly-wrong result. (@group targets are + // rejected at the group-forward boundary in callToolAtGroupRepo before + // they ever reach here; see KTD12.) + const incompatible: string[] = []; + if (params.relationTypes !== undefined) incompatible.push('relationTypes'); + if (params.crossDepth !== undefined) incompatible.push('crossDepth'); + if (params.minConfidence !== undefined) incompatible.push('minConfidence'); + if (incompatible.length > 0) { + return { + error: + `Parameter(s) ${incompatible.join(', ')} are not supported with mode:'pdg' ` + + `(intra-procedural, single-repo, dependence-edge based). Remove them or use mode:'callgraph'.`, + target: { name: target }, + direction, + impactedCount: 0, + risk: 'UNKNOWN', + }; + } + } + + // (2) PDG-layer presence probe — STUB for U1; U2 fills in the four-state + // degradation contract (no-layer / partial / unknown / ready) here, before + // any DB scan, so a missing `--pdg` layer returns a guidance note rather + // than a confusing empty traversal. Intentionally a no-op for now. + const maxDepth = params.maxDepth || 3; // Map legacy relation type names before filtering (backward compat for OVERRIDES → METHOD_OVERRIDES) const mappedRelTypes = params.relationTypes?.flatMap((t: string) => @@ -4300,6 +4384,44 @@ export class LocalBackend { } if (outcome.kind === 'ambiguous') { + // KTD5 ambiguous trap — under mode:'pdg' we MUST NOT fall into the + // callgraph fan-out below: it runs `_runImpactBFS` per candidate, which + // would silently execute the call-graph engine under a `pdg` call (the + // exact silent fallback KTD5 forbids). For U1 the pdg ambiguous path + // returns the candidate list WITHOUT any callgraph probe; the full pdg + // ambiguous handling (per-candidate PDG summaries / ranking) lands in U4. + if (mode === 'pdg') { + const AMBIGUOUS_MAX_CANDIDATES = 6; + const truncated = outcome.candidates.length > AMBIGUOUS_MAX_CANDIDATES; + const shown = outcome.candidates.slice(0, AMBIGUOUS_MAX_CANDIDATES); + return { + status: 'ambiguous', + mode, + message: + `Found ${outcome.candidates.length} symbols matching '${target}'` + + (truncated ? ` (showing ${shown.length} of ${outcome.candidates.length})` : '') + + `. Disambiguate with target_uid (or file_path/kind) for a single ` + + `authoritative PDG result.`, + target: { name: target }, + direction, + totalCandidates: outcome.candidates.length, + // No single resolved symbol → impactedCount stays 0 / risk UNKNOWN + // (UNKNOWN must never read as "safe to refactor"). No callgraph + // fan-out runs, so there is no per-candidate blast radius here yet. + impactedCount: 0, + risk: 'UNKNOWN', + ...(truncated && { candidatesTruncated: true }), + candidates: shown.map((c) => ({ + uid: c.id, + name: c.name, + kind: c.type, + filePath: c.filePath, + line: c.startLine, + score: Number(c.score.toFixed(2)), + })), + }; + } + // #2129 — a bare name that collides with several symbols must NOT report a // bare `impactedCount: 0`. The real blast radius lives under whichever // candidate the caller meant; a flat zero here is precisely the silent @@ -4425,6 +4547,27 @@ export class LocalBackend { }; const symType = outcome.resolvedLabel || outcome.symbol.type || ''; + // (4) single → route the resolved symbol to the engine selected by `mode`. + // The PDG engine is a stub in U1 (full traversal lands in U3/U4); crucially + // it does NOT touch `_runImpactBFS`, so a `pdg` call never runs callgraph. + if (mode === 'pdg') { + return this._runImpactPDG({ + repo, + sym, + symType, + direction, + maxDepth, + limit: Number.isFinite(params.limit) ? params.limit : 100, + offset: Number.isFinite(params.offset) ? params.offset : 0, + summaryOnly: params.summaryOnly, + // KTD2 extraction-seam discipline: hand the engine its DB dependency + // explicitly rather than `this.`-binding it, so the traversal (U3/U4) + // can later move to a standalone `pdg-impact.ts` as a move, not a + // rewrite. The U3 block-anchor / projection resolvers join here. + executeParameterized, + }); + } + const effectiveRelationTypes = (symType === 'Class' || symType === 'Interface') && !hasExplicitRelationTypes && @@ -4443,6 +4586,44 @@ export class LocalBackend { }); } + /** + * PDG-backed blast radius (`mode:'pdg'`) — STUB (U1). + * + * The real engine (U3/U4) resolves the target symbol to its BasicBlocks, + * runs a direction-aware bounded BFS over the persisted `CDG` + + * `REACHING_DEF` edges (KTD4 truth table, KTD11 query constraints), then + * projects the reachable blocks back to owning symbols and assembles a + * consumer-safe result (KTD8 parity matrix). U1 ships only the param / + * validation surface, so this returns a structured "pending" payload. + * + * KTD2 extraction-seam discipline: written as a method taking its DB + * dependency as an explicit parameter (not reaching back through `this.` for + * the query path) so the traversal can later be lifted to a standalone + * `gitnexus/src/mcp/local/pdg-impact.ts` engine as a *move*, not a rewrite. + * The stub does not yet consume `executeParameterized` — the U3 anchor / + * BFS / projection code joins here. + */ + private async _runImpactPDG(deps: { + repo: RepoHandle; + sym: { id: string; name: string; filePath: string }; + symType: string; + direction: 'upstream' | 'downstream'; + maxDepth: number; + limit: number; + offset: number; + summaryOnly?: boolean; + executeParameterized: typeof executeParameterized; + }): Promise { + return { + error: 'pdg mode not yet implemented (U3/U4)', + mode: 'pdg', + target: { name: deps.sym.name, id: deps.sym.id, filePath: deps.sym.filePath }, + direction: deps.direction, + impactedCount: 0, + risk: 'UNKNOWN', + }; + } + /** * #1858 — epistemic lower-bound detection. * @@ -5348,6 +5529,21 @@ export class LocalBackend { const svc = this.getGroupService(); if (method === 'impact') { + // KTD5/KTD12 — validate `mode` at the group-forward boundary too (the + // JSON-schema enum is advisory). An invalid mode errors; `mode:'pdg'` is + // rejected for @group targets because PDG impact is single-repo and + // intra-procedural — there is no cross-repo dependence graph to walk. + // Rejecting here (before groupImpact) is the KTD12 @group hard error. + const groupModeResult = validateImpactMode(params.mode); + if ('error' in groupModeResult) return { error: groupModeResult.error }; + if (groupModeResult.mode === 'pdg') { + return { + error: + "mode:'pdg' is not supported for @group targets — PDG impact is " + + 'single-repo and intra-procedural. Run pdg impact against an ' + + 'individual indexed repository instead.', + }; + } const impactArgs: Record = { name: groupName, repo: resolved.repoPath, diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 959f6a565..d481d4939 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -409,6 +409,8 @@ Each edit is tagged with confidence: description: `Analyze the blast radius of changing a code symbol. Returns affected symbols grouped by depth, plus risk assessment, affected execution flows, and affected modules. +MODE (opt-in): "callgraph" (default) walks symbol→symbol edges (CALLS/IMPORTS/EXTENDS/IMPLEMENTS) — inter-procedural, the established behavior. "pdg" computes the blast radius from the persisted Program Dependence Graph (control + data dependence) — finer-grained WITHIN a function but intra-procedural, and requires an index built with \`gitnexus analyze --pdg\`. The two modes answer the same question with different engines; pdg is incompatible with relationTypes/crossDepth/minConfidence and with @group targets (each rejected). + WHEN TO USE: Before making code changes — especially refactoring, renaming, or modifying shared code. Shows what would break. AFTER THIS: Review d=1 items (WILL BREAK). Use context() on high-risk symbols. @@ -450,6 +452,13 @@ SERVICE: optional monorepo path prefix (case-sensitive path segments). When "rep type: 'string', description: 'upstream (what depends on this) or downstream (what this depends on)', }, + mode: { + type: 'string', + enum: ['callgraph', 'pdg'], + default: 'callgraph', + description: + "Blast-radius engine. 'callgraph' (default) = inter-procedural symbol→symbol traversal (current behavior). 'pdg' = opt-in, intra-procedural Program Dependence Graph traversal (control + data dependence); requires an index built with `gitnexus analyze --pdg`. The pdg mode is incompatible with relationTypes/crossDepth/minConfidence and with @group targets — each is rejected, not silently ignored.", + }, file_path: { type: 'string', description: 'File path hint to disambiguate common names', diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 84bdc4e69..5e590cf93 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -1385,6 +1385,172 @@ describe('LocalBackend.callTool', () => { }); }); +// ─── impact mode param (KTD1/KTD5/KTD12 — U1) ─────────────────────── +// +// The MCP JSON-schema enum is advisory only (server forwards args +// unvalidated, callTool is reachable directly), so the backend `mode` +// validation is load-bearing. These tests pin: callgraph is the unchanged +// default, pdg routes to the stub and NEVER the callgraph BFS, invalid modes +// hard-error, and the KTD12 incompatible params / @group targets are rejected. + +describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => { + let backend: LocalBackend; + + // Resolve the target to a single Function so impact reaches the single-branch + // dispatch (callgraph BFS or the pdg stub). The callgraph BFS then issues + // executeQuery for its frontier; the pdg stub does not. + function resolveSingleTarget() { + (executeParameterized as any).mockResolvedValue([ + { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, + ]); + (executeQuery as any).mockResolvedValue([]); + } + + beforeEach(async () => { + vi.clearAllMocks(); + platformMocks.isVectorExtensionSupportedByPlatform.mockReturnValue(true); + backend = new LocalBackend(); + setupSingleRepo(); + await backend.init(); + }); + + it('mode absent → callgraph result (target populated, no mode-error, BFS runs)', async () => { + resolveSingleTarget(); + const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); + const result = await backend.callTool('impact', { target: 'main', direction: 'upstream' }); + // A clean callgraph result carries no mode/stub error and runs the BFS. + expect(result.error ?? '').not.toMatch(/Invalid "mode"/); + expect(result.error ?? '').not.toMatch(/not yet implemented/); + expect(result.target).toBeDefined(); + expect(bfsSpy).toHaveBeenCalledTimes(1); + }); + + it("mode:'callgraph' and mode:undefined are byte-identical to absent (regression guard)", async () => { + resolveSingleTarget(); + const absent = await backend.callTool('impact', { target: 'main', direction: 'upstream' }); + const callgraph = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: 'callgraph', + }); + const undef = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: undefined, + }); + expect(callgraph).toEqual(absent); + expect(undef).toEqual(absent); + }); + + it("mode:'pdg' routes to the PDG stub and NEVER runs the callgraph BFS (KTD5)", async () => { + resolveSingleTarget(); + const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: 'pdg', + }); + // Stub payload — pending until U3/U4. + expect(result.error).toMatch(/not yet implemented/); + expect(result.mode).toBe('pdg'); + // The callgraph engine must never be invoked under a pdg call. + expect(bfsSpy).not.toHaveBeenCalled(); + }); + + it.each([['PDG'], ['pgd'], [''], [0], [null]])( + 'invalid mode %j → structured {error}, never a callgraph result (KTD5 anti-silent-fallback)', + async (bad) => { + resolveSingleTarget(); + const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: bad as any, + }); + expect(result.error).toMatch(/Invalid "mode"/); + expect(result.risk).toBe('UNKNOWN'); + // A typo'd mode must NEVER quietly run callgraph. + expect(bfsSpy).not.toHaveBeenCalled(); + }, + ); + + it.each([ + ['relationTypes', { relationTypes: ['CALLS'] }], + ['crossDepth', { crossDepth: 2 }], + ['minConfidence', { minConfidence: 0.5 }], + ])("mode:'pdg' + %s → hard {error} (KTD12, not a silent ignore)", async (_label, extra) => { + resolveSingleTarget(); + const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: 'pdg', + ...extra, + }); + expect(result.error).toMatch(/not supported with mode:'pdg'/); + expect(result.error).toContain(_label); + // Hard error — never silently ignored, never the callgraph fan-out. + expect(bfsSpy).not.toHaveBeenCalled(); + }); + + it("ambiguous target under mode:'pdg' never invokes the callgraph fan-out (KTD5 ambiguous trap)", async () => { + // Two same-name Functions → resolver returns ambiguous. + (executeParameterized as any).mockResolvedValue([ + { id: 'func:login:1', name: 'login', type: 'Function', filePath: 'src/auth.ts', startLine: 5 }, + { + id: 'func:login:2', + name: 'login', + type: 'Function', + filePath: 'src/admin/login.ts', + startLine: 8, + }, + ]); + const bfsSpy = vi.spyOn(backend as any, '_runImpactBFS'); + const result = await backend.callTool('impact', { + target: 'login', + direction: 'upstream', + mode: 'pdg', + }); + expect(result.status).toBe('ambiguous'); + expect(result.mode).toBe('pdg'); + expect(result.candidates).toHaveLength(2); + expect(result.impactedCount).toBe(0); + expect(result.risk).toBe('UNKNOWN'); + // The callgraph per-candidate probe fan-out MUST NOT run under pdg. + expect(bfsSpy).not.toHaveBeenCalled(); + // No per-candidate blast radius is computed yet (U4), so the candidate + // entries carry no impactedCount field from a callgraph probe. + for (const c of result.candidates) { + expect(c.impactedCount).toBeUndefined(); + } + }); + + it("@group target with mode:'pdg' is rejected (KTD12 — PDG is single-repo)", async () => { + resolveAtMemberMock.mockResolvedValue({ ok: true, repoPath: '/tmp/test-project' }); + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: 'pdg', + repo: '@grp', + }); + expect(result.error).toMatch(/not supported for @group targets/); + }); + + it("@group target with mode:'callgraph' still forwards to group impact (unchanged)", async () => { + resolveAtMemberMock.mockResolvedValue({ ok: true, repoPath: '/tmp/test-project' }); + // groupImpact is reached only if the mode gate passes; we don't assert its + // payload (group infra is stubbed), only that no mode-error short-circuited. + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + mode: 'callgraph', + repo: '@grp', + }); + expect(result?.error ?? '').not.toMatch(/not supported for @group targets/); + expect(result?.error ?? '').not.toMatch(/Invalid "mode"/); + }); +}); + // ─── Repo resolution ──────────────────────────────────────────────── describe('LocalBackend.resolveRepo', () => { diff --git a/gitnexus/test/unit/tools.test.ts b/gitnexus/test/unit/tools.test.ts index f1b3dc83b..0841b06ca 100644 --- a/gitnexus/test/unit/tools.test.ts +++ b/gitnexus/test/unit/tools.test.ts @@ -276,6 +276,24 @@ describe('GITNEXUS_TOOLS', () => { expect(relProp.items).toEqual({ type: 'string' }); }); + it('impact advertises a mode param (callgraph default; pdg opt-in) — not a new tool (KTD1)', () => { + // KTD1: pdg impact ships as a PARAM on the existing tool, so the tool count + // must NOT change (asserted at 17 above) and `impact` must expose `mode`. + const impactTool = GITNEXUS_TOOLS.find((t) => t.name === 'impact')!; + const modeProp = impactTool.inputSchema.properties.mode; + expect(modeProp).toBeDefined(); + expect(modeProp.type).toBe('string'); + expect(modeProp.enum).toEqual(['callgraph', 'pdg']); + expect(modeProp.default).toBe('callgraph'); + // The description must teach the opt-in / intra-procedural / --pdg contract. + expect(modeProp.description).toContain('pdg'); + expect(modeProp.description).toContain('--pdg'); + expect(modeProp.description.toLowerCase()).toContain('intra-procedural'); + // The tool-level description must mention the mode so an LLM discovers it. + expect(impactTool.description.toLowerCase()).toContain('mode'); + expect(impactTool.description).toContain('pdg'); + }); + it('route_map description defers to api_impact for pre-change analysis', () => { const routeMapTool = GITNEXUS_TOOLS.find((t) => t.name === 'route_map')!; expect(routeMapTool.description).toContain('api_impact');