diff --git a/README.md b/README.md index 39b09b7d2..a5e04e728 100644 --- a/README.md +++ b/README.md @@ -660,6 +660,14 @@ UPSTREAM (what depends on this): Options: `maxDepth`, `minConfidence`, `relationTypes` (`CALLS`, `IMPORTS`, `EXTENDS`, `IMPLEMENTS`), `includeTests`, `limit` (max symbols per depth, default 100), `offset` (pagination start per depth), `summaryOnly` (counts and risk only, omits symbol list) +**Disambiguation** — when several symbols share the target name, `impact` returns a ranked `ambiguous` candidate list instead of guessing. Narrow it with `target_uid` (exact, zero-ambiguity), `file_path`, or `kind` (`Function`, `Class`, `Method`, …). From the CLI these are `--uid`, `--file`, and `--kind`, matching `gitnexus context`: + +```bash +gitnexus impact get_embeddings # → ambiguous: lists ranked candidates +gitnexus impact get_embeddings --file src/embed.py # → resolves to the one in that file +gitnexus impact get_embeddings --uid "Function:src/embed.py:get_embeddings" # exact +``` + ### Process-Grouped Search ``` diff --git a/gitnexus/README.md b/gitnexus/README.md index b313a7f46..e0ba4283e 100644 --- a/gitnexus/README.md +++ b/gitnexus/README.md @@ -170,6 +170,13 @@ gitnexus clean --all --force # Delete all indexes gitnexus wiki [path] # Generate LLM-powered docs from knowledge graph gitnexus wiki --model # Wiki with custom LLM model (default: gpt-4o-mini) +# Direct graph queries — the same tools the MCP server exposes, no MCP daemon needed +gitnexus query "" # Process-grouped hybrid search +gitnexus context [--uid | --file ] # 360° symbol view; flags disambiguate a shared name +gitnexus impact [--uid | --file | --kind ] # Blast radius; flags disambiguate a shared name +gitnexus detect-changes # Map the working-tree diff to affected symbols and execution flows +gitnexus cypher "" # Run a raw Cypher query against the knowledge graph + # Repository groups (multi-repo / monorepo service tracking) gitnexus group create # Create a repository group gitnexus group add # Add a repo to a group. is a hierarchy path (e.g. hr/hiring/backend); is the repo's name from the registry (see `gitnexus list`) diff --git a/gitnexus/src/cli/help-i18n.ts b/gitnexus/src/cli/help-i18n.ts index 8d620fc6f..5fb42dbe1 100644 --- a/gitnexus/src/cli/help-i18n.ts +++ b/gitnexus/src/cli/help-i18n.ts @@ -101,6 +101,9 @@ const OPTION_DESCRIPTION_KEYS = { 'context|--content': 'help.option.content', 'impact|-d, --direction ': 'help.option.impact.direction', 'impact|-r, --repo ': 'help.option.repo.target', + 'impact|-u, --uid ': 'help.option.context.uid', + 'impact|-f, --file ': 'help.option.context.file', + 'impact|--kind ': 'help.option.impact.kind', 'impact|--depth ': 'help.option.impact.depth', 'impact|--include-tests': 'help.option.impact.includeTests', 'impact|--limit ': 'help.option.impact.limit', diff --git a/gitnexus/src/cli/i18n/en.ts b/gitnexus/src/cli/i18n/en.ts index 51cbc2816..e4778fb58 100644 --- a/gitnexus/src/cli/i18n/en.ts +++ b/gitnexus/src/cli/i18n/en.ts @@ -43,8 +43,11 @@ export const en = { 'tool.noIndexed': 'GitNexus: No indexed repositories found. Run: gitnexus analyze', 'tool.usage.query': 'Usage: gitnexus query ', 'tool.usage.context': 'Usage: gitnexus context [--uid ] [--file ]', - 'tool.usage.impact': 'Usage: gitnexus impact [--direction upstream|downstream]', + 'tool.usage.impact': + 'Usage: gitnexus impact [--uid ] [--file ] [--kind ] [--direction upstream|downstream]', 'tool.usage.cypher': 'Usage: gitnexus cypher ', + 'tool.warn.unknownKind': + "--kind '{{kind}}' is not a known symbol kind (e.g. Function, Class, Method); it will not narrow the result.", 'tool.detectChanges.noChanges': 'No changes detected.', 'tool.detectChanges.changesSummary': 'Changes: {{files}} files, {{symbols}} symbols', 'tool.detectChanges.affectedProcesses': 'Affected processes: {{count}}', @@ -213,6 +216,8 @@ export const en = { 'help.option.repo.target': 'Target repository', 'help.option.context.uid': 'Direct symbol UID (zero-ambiguity lookup)', 'help.option.context.file': 'File path to disambiguate common names', + 'help.option.impact.kind': + 'Kind filter to disambiguate common names (e.g. Function, Class, Method)', 'help.option.impact.direction': 'upstream (dependants) or downstream (dependencies)', 'help.option.impact.depth': 'Max relationship depth (default: 3)', 'help.option.impact.includeTests': 'Include test files in results', diff --git a/gitnexus/src/cli/i18n/zh-CN.ts b/gitnexus/src/cli/i18n/zh-CN.ts index 2a1374fc6..cd488de0a 100644 --- a/gitnexus/src/cli/i18n/zh-CN.ts +++ b/gitnexus/src/cli/i18n/zh-CN.ts @@ -47,8 +47,11 @@ export const zhCN = { 'tool.noIndexed': 'GitNexus:未找到已索引仓库。请运行:gitnexus analyze', 'tool.usage.query': '用法:gitnexus query <搜索词>', 'tool.usage.context': '用法:gitnexus context <符号名> [--uid ] [--file <路径>]', - 'tool.usage.impact': '用法:gitnexus impact <符号名> [--direction upstream|downstream]', + 'tool.usage.impact': + '用法:gitnexus impact <符号名> [--uid ] [--file <路径>] [--kind <类型>] [--direction upstream|downstream]', 'tool.usage.cypher': '用法:gitnexus cypher ', + 'tool.warn.unknownKind': + "--kind '{{kind}}' 不是已知的符号类型(如 Function、Class、Method),不会用于缩小结果范围。", 'tool.detectChanges.noChanges': '未检测到变更。', 'tool.detectChanges.changesSummary': '变更:{{files}} 个文件,{{symbols}} 个符号', 'tool.detectChanges.affectedProcesses': '受影响流程:{{count}}', @@ -199,6 +202,7 @@ export const zhCN = { 'help.option.repo.target': '目标仓库', 'help.option.context.uid': '直接符号 UID(零歧义查找)', 'help.option.context.file': '用于消除常见名称歧义的文件路径', + 'help.option.impact.kind': '用于消除常见名称歧义的类型过滤(如 Function、Class、Method)', 'help.option.impact.direction': 'upstream(依赖它的项)或 downstream(它依赖的项)', 'help.option.impact.depth': '最大关系遍历深度(默认:3)', 'help.option.impact.includeTests': '在结果中包含测试文件', diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 67b34387a..b599f7e9e 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -219,10 +219,16 @@ program .action(createLbugLazyAction(() => import('./tool.js'), 'contextCommand')); program - .command('impact ') + .command('impact [target]') .description('Blast radius analysis: what breaks if you change a symbol') .option('-d, --direction ', 'upstream (dependants) or downstream (dependencies)', 'upstream') .option('-r, --repo ', 'Target repository') + .option('-u, --uid ', 'Direct symbol UID (zero-ambiguity lookup)') + .option('-f, --file ', 'File path to disambiguate common names') + .option( + '--kind ', + 'Kind filter to disambiguate common names (e.g. Function, Class, Method)', + ) .option('--depth ', 'Max relationship depth (default: 3)') .option('--include-tests', 'Include test files in results') .option('--limit ', 'Max symbols per depth level (default: 100)') diff --git a/gitnexus/src/cli/tool.ts b/gitnexus/src/cli/tool.ts index 0e70c3970..5801677b9 100644 --- a/gitnexus/src/cli/tool.ts +++ b/gitnexus/src/cli/tool.ts @@ -16,8 +16,8 @@ */ import { writeSync } from 'node:fs'; -import { LocalBackend } from '../mcp/local/local-backend.js'; -import { cliErrorKey } from './cli-message.js'; +import { LocalBackend, VALID_NODE_LABELS } from '../mcp/local/local-backend.js'; +import { cliErrorKey, cliWarnKey } from './cli-message.js'; import { formatDetectChangesResult } from './detect-changes-format.js'; let _backend: LocalBackend | null = null; @@ -94,6 +94,11 @@ export async function contextCommand( content?: boolean; }, ): Promise { + // Reject a `--`-prefixed uid swallowed from a following flag (see impactCommand). + if (options?.uid?.startsWith('--')) { + cliErrorKey('tool.usage.context'); + process.exit(1); + } if (!name?.trim() && !options?.uid) { cliErrorKey('tool.usage.context'); process.exit(1); @@ -111,10 +116,13 @@ export async function contextCommand( } export async function impactCommand( - target: string, + target?: string, options?: { direction?: string; repo?: string; + uid?: string; + file?: string; + kind?: string; depth?: string; includeTests?: boolean; limit?: string; @@ -122,10 +130,25 @@ export async function impactCommand( summaryOnly?: boolean; }, ): Promise { - if (!target?.trim()) { + // A `--`-prefixed uid means Commander swallowed a following flag as the uid + // value (e.g. `impact --uid --file x` → uid === '--file'). Reject it rather + // than forwarding a garbage uid that would silently resolve to not-found. + if (options?.uid?.startsWith('--')) { cliErrorKey('tool.usage.impact'); process.exit(1); } + // Target is an optional positional: a uid alone is enough to resolve (parity + // with `context [name]`). Only error when neither a target nor a uid is given. + if (!target?.trim() && !options?.uid) { + cliErrorKey('tool.usage.impact'); + process.exit(1); + } + // Soft-validate --kind: an unknown kind is a no-op hint (the backend scores + // it but it matches nothing), so warn and proceed rather than rejecting — + // parity with the lenient MCP surface and forward-compatible with new labels. + if (options?.kind && !VALID_NODE_LABELS.has(options.kind)) { + cliWarnKey('tool.warn.unknownKind', { kind: options.kind }); + } try { const backend = await getBackend(); @@ -134,7 +157,10 @@ export async function impactCommand( const parsedLimit = Number.isFinite(rawLimit) ? rawLimit : undefined; const parsedOffset = Number.isFinite(rawOffset) ? rawOffset : undefined; const result = await backend.callTool('impact', { - target, + target: target || undefined, + target_uid: options?.uid, + file_path: options?.file, + kind: options?.kind, direction: options?.direction || 'upstream', maxDepth: options?.depth ? parseInt(options.depth, 10) : undefined, includeTests: options?.includeTests ?? false, diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 5a9290394..24e68facb 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -2920,8 +2920,14 @@ export class LocalBackend { typeof opts.offset === 'number' && Number.isFinite(opts.offset) ? opts.offset : 0; const paginationOffset = Math.max(0, Math.trunc(rawOffset)); const summaryOnly = opts.summaryOnly ?? false; - const relTypeFilter = relationTypes.map((t) => `'${t}'`).join(', '); - const confidenceFilter = minConfidence > 0 ? ` AND r.confidence >= ${minConfidence}` : ''; + // Bind the BFS frontier query's filters as parameters (#1907 review F5): + // node ids and relation types as bound lists, the confidence floor as a + // bound number — no string interpolation reaches the query text. Preserve + // the original "no confidence clause when minConfidence <= 0" behavior: an + // unconditional `>= 0` would wrongly exclude NULL-confidence edges that the + // unfiltered query includes. + const safeMinConfidence = Number.isFinite(minConfidence) ? minConfidence : 0; + const confidenceFilter = safeMinConfidence > 0 ? ' AND r.confidence >= $minConfidence' : ''; const symId = sym.id || sym[0]; @@ -3009,15 +3015,19 @@ export class LocalBackend { for (let depth = 1; depth <= maxDepth && frontier.length > 0; depth++) { const nextFrontier: string[] = []; - // Batch frontier nodes into a single Cypher query per depth level - const idList = frontier.map((id) => `'${id.replace(/'/g, "''")}'`).join(', '); + // Batch frontier nodes into a single Cypher query per depth level. + // ids/types/confidence are bound parameters (see above) — no interpolation. const query = direction === 'upstream' - ? `MATCH (caller)-[r:CodeRelation]->(n) WHERE n.id IN [${idList}] AND r.type IN [${relTypeFilter}]${confidenceFilter} RETURN n.id AS sourceId, caller.id AS id, caller.name AS name, labels(caller)[0] AS type, caller.filePath AS filePath, r.type AS relType, r.confidence AS confidence` - : `MATCH (n)-[r:CodeRelation]->(callee) WHERE n.id IN [${idList}] AND r.type IN [${relTypeFilter}]${confidenceFilter} RETURN n.id AS sourceId, callee.id AS id, callee.name AS name, labels(callee)[0] AS type, callee.filePath AS filePath, r.type AS relType, r.confidence AS confidence`; + ? `MATCH (caller)-[r:CodeRelation]->(n) WHERE n.id IN $frontierIds AND r.type IN $relTypes${confidenceFilter} RETURN n.id AS sourceId, caller.id AS id, caller.name AS name, labels(caller)[0] AS type, caller.filePath AS filePath, r.type AS relType, r.confidence AS confidence` + : `MATCH (n)-[r:CodeRelation]->(callee) WHERE n.id IN $frontierIds AND r.type IN $relTypes${confidenceFilter} RETURN n.id AS sourceId, callee.id AS id, callee.name AS name, labels(callee)[0] AS type, callee.filePath AS filePath, r.type AS relType, r.confidence AS confidence`; try { - const related = await executeQuery(repo.id, query); + const related = await executeParameterized(repo.id, query, { + frontierIds: frontier, + relTypes: relationTypes, + ...(safeMinConfidence > 0 ? { minConfidence: safeMinConfidence } : {}), + }); for (const rel of related) { const relId = rel.id || rel[1]; diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index e9d529643..075f80782 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -1534,3 +1534,82 @@ describe('CLI end-to-end', () => { }, 35000); }); }); + +// ─── impact disambiguation flags reach the backend at runtime (#1907 U2) ── +// The mocked unit test proves the CLI option → callTool param mapping; this +// proves the flags survive the real Commander → lazy-action → impactCommand → +// callTool chain by spawning the actual CLI. The F2 gap is *flag-forwarding*, +// so a uniquely-named fixture symbol is enough — no ambiguous fixture needed. +// Tests self-skip when the environment cannot index the fixture (e.g. a +// worktree without the built parse-worker); CI validates the real path. +describe('impact disambiguation flags reach the backend (e2e, #1907)', () => { + const SYMBOL = 'formatResponse'; // uniquely named, in mini-repo/src/formatter.ts + let uid: string | undefined; + let symbolFile: string | undefined; + + beforeAll(() => { + // Idempotent: the earlier analyze test may already have indexed mini-repo. + runCli('analyze', MINI_REPO, 60000); + // Derive the real uid + filePath from context so the test is robust to the + // exact uid format rather than hard-coding `Function::`. + const ctx = runCliRaw(['context', SYMBOL, '--repo', 'mini-repo'], MINI_REPO, 30000); + if (ctx.status === 0) { + try { + const parsed = JSON.parse(ctx.stdout.trim()); + uid = parsed?.symbol?.uid; + symbolFile = parsed?.symbol?.filePath; + } catch { + /* leave undefined → tests self-skip below */ + } + } + }); + + it('forwards --uid alone with no positional target (U1 + --uid end-to-end)', () => { + if (!uid) return; // environment could not index — validated in CI + const res = runCliRaw(['impact', '--uid', uid, '--repo', 'mini-repo'], MINI_REPO, 30000); + if (res.status === null) return; + expect(res.status).toBe(0); + const out = JSON.parse(res.stdout.trim()); + expect(out).not.toHaveProperty('error'); + expect(out.target?.id).toBe(uid); + }); + + it('forwards --file: the correct file resolves, a wrong file does not (negative control)', () => { + if (!uid || !symbolFile) return; + + const ok = runCliRaw( + ['impact', SYMBOL, '--file', symbolFile, '--repo', 'mini-repo'], + MINI_REPO, + 30000, + ); + if (ok.status === null) return; + expect(ok.status).toBe(0); + const okOut = JSON.parse(ok.stdout.trim()); + expect(okOut.status).not.toBe('ambiguous'); + expect(okOut.target?.filePath).toBe(symbolFile); + + // Wrong --file hint → CONTAINS matches nothing → must NOT resolve to the + // formatter.ts symbol. Proves the --file value reached the resolver. + const wrong = runCliRaw( + ['impact', SYMBOL, '--file', 'does/not/exist/nowhere.ts', '--repo', 'mini-repo'], + MINI_REPO, + 30000, + ); + if (wrong.status === null) return; + const wrongOut = JSON.parse(wrong.stdout.trim()); + expect(wrongOut.error !== undefined || wrongOut.target?.filePath !== symbolFile).toBe(true); + }); + + it('forwards --kind: exit 0 with the kind hint applied', () => { + if (!uid) return; + const res = runCliRaw( + ['impact', SYMBOL, '--kind', 'Function', '--repo', 'mini-repo'], + MINI_REPO, + 30000, + ); + if (res.status === null) return; + expect(res.status).toBe(0); + const out = JSON.parse(res.stdout.trim()); + expect(out).not.toHaveProperty('error'); + }); +}); diff --git a/gitnexus/test/integration/local-backend-calltool.test.ts b/gitnexus/test/integration/local-backend-calltool.test.ts index cd4b34d9c..e2640deab 100644 --- a/gitnexus/test/integration/local-backend-calltool.test.ts +++ b/gitnexus/test/integration/local-backend-calltool.test.ts @@ -302,6 +302,116 @@ withTestLbugDB( } }); }); + + // ─── impact disambiguation + label-scoped resolution (#1907) ───────── + // Covers the disambiguation surface the CLI --uid/--file/--kind flags + // wire through to, and guards the label-scoped resolver against the + // binder failure that motivated the fix. + describe('impact disambiguation (#1907)', () => { + let backend: LocalBackend; + + beforeAll(async () => { + const ext = handle as typeof handle & { _backend?: LocalBackend }; + if (!ext._backend) { + throw new Error( + 'LocalBackend not initialized — afterSetup did not attach _backend to handle', + ); + } + backend = ext._backend; + }); + + it('reports an ambiguous target with disambiguation guidance', async () => { + // Two Methods named 'authenticate' (AuthService + BaseService). + const result = await backend.callTool('impact', { + target: 'authenticate', + direction: 'upstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.status).toBe('ambiguous'); + expect(result.message).toMatch(/disambiguate/i); + const uids = (result.candidates ?? []).map((c: any) => c.uid); + expect(uids).toContain('method:AuthService.authenticate'); + expect(uids).toContain('method:BaseService.authenticate'); + }); + + it('resolves the ambiguous target via target_uid (the --uid flag path)', async () => { + const result = await backend.callTool('impact', { + target: 'authenticate', + target_uid: 'method:BaseService.authenticate', + direction: 'upstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.status).not.toBe('ambiguous'); + // target_uid selects the exact symbol, bypassing the name ranker. + expect(result.target?.id).toBe('method:BaseService.authenticate'); + expect(result.target?.filePath).toBe('src/base.ts'); + }); + + it('resolves the ambiguous target via file_path (the --file flag path)', async () => { + const result = await backend.callTool('impact', { + target: 'authenticate', + file_path: 'src/base.ts', + direction: 'upstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.status).not.toBe('ambiguous'); + expect(result.target?.id).toBe('method:BaseService.authenticate'); + }); + + it('does not crash when a name collides across symbol and non-symbol labels', async () => { + // 'alpha' exists as both a Function and a Tool sharing src/tools.py. + // The Tool node table has no startLine/endLine columns, so the + // resolver's `RETURN n.startLine` projection only binds because the + // candidate set also contains a label that *does* have those columns + // (lenient multi-table binding). This guards that the disambiguation + // path keeps tolerating non-symbol node types — and would catch a + // future naive label-scoping that reintroduces the #1907 binder error + // ("Cannot find property … for n") by matching property-poor tables + // in isolation. + const result = await backend.callTool('impact', { + target: 'alpha', + direction: 'upstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.status).toBe('ambiguous'); + const uids = (result.candidates ?? []).map((c: any) => c.uid); + expect(uids).toContain('func:alpha'); + expect(uids).toContain('Tool:alpha'); + }); + + it('context resolves the same cross-label collision without crashing', async () => { + const result = await backend.callTool('context', { name: 'alpha' }); + expect(result).not.toHaveProperty('error'); + expect(result.status).toBe('ambiguous'); + const uids = (result.candidates ?? []).map((c: any) => c.uid); + expect(uids).toContain('func:alpha'); + // Assert the non-symbol Tool node stays in the candidate set, not just + // that nothing crashed — a regression that silently dropped Tool from + // the lenient-binding match would otherwise pass the non-crash check. + expect(uids).toContain('Tool:alpha'); + }); + + it('ranks the kind-matching candidate first when kind is supplied (the --kind flag path)', async () => { + // 'alpha' is both a Function (func:alpha) and a Tool (Tool:alpha). + // kind only adds +0.20 in scoreCandidate, so 0.50 + 0.20 = 0.70 stays + // below the 0.95 confident-resolution threshold — the response is still + // ambiguous. What kind buys is ranking: the Function is promoted above + // the non-matching Tool. This exercises the scoreCandidate kind branch + // against a real DB rather than only through the mocked CLI unit test. + const result = await backend.callTool('impact', { + target: 'alpha', + kind: 'Function', + direction: 'upstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.status).toBe('ambiguous'); + const candidates = result.candidates ?? []; + expect(candidates[0]?.uid).toBe('func:alpha'); + expect(candidates[0]?.kind).toBe('Function'); + const tool = candidates.find((c: any) => c.uid === 'Tool:alpha'); + expect(candidates[0]?.score).toBeGreaterThan(tool?.score); + }); + }); }, { seed: LOCAL_BACKEND_SEED_DATA, @@ -327,3 +437,68 @@ withTestLbugDB( }, }, ); + +// ─── impact BFS bound parameters (#1907 review F5) ─────────────────────── +// Isolated DB (not the shared seed) with a frontier node whose id contains a +// single quote. Under the old string-interpolated query this id had to be +// hand-escaped; the parameterized query (executeParameterized with bound +// $frontierIds/$relTypes) carries it as data. Guards that a quote-bearing id +// traverses without a Prepare/parser error, and that a no-caller symbol +// returns an empty result rather than erroring. +withTestLbugDB( + 'local-backend-impact-param', + (handle) => { + describe('impact BFS bound parameters (#1907 F5)', () => { + let backend: LocalBackend; + + beforeAll(() => { + const ext = handle as typeof handle & { _backend?: LocalBackend }; + if (!ext._backend) { + throw new Error('LocalBackend not initialized — afterSetup did not attach _backend'); + } + backend = ext._backend; + }); + + it('traverses a caller whose id contains a single quote without a query error', async () => { + const result = await backend.callTool('impact', { target: 'sink', direction: 'upstream' }); + expect(result).not.toHaveProperty('error'); + const d1 = result.byDepth?.[1] || result.byDepth?.['1'] || []; + const callerIds = d1.map((d: any) => d.uid ?? d.id); + expect(callerIds).toContain("func:o'd"); + }); + + it('returns an empty result (not an error) for a symbol with no callers', async () => { + const result = await backend.callTool('impact', { + target: 'sink', + direction: 'downstream', + }); + expect(result).not.toHaveProperty('error'); + expect(result.impactedCount).toBe(0); + }); + }); + }, + { + seed: [ + `CREATE (a:Function {id: "func:o'd", name: 'odd', filePath: 'src/q.ts', startLine: 1, endLine: 3, isExported: true, content: 'function odd() {}', description: 'caller with a quote in its id'})`, + `CREATE (b:Function {id: 'func:sink', name: 'sink', filePath: 'src/q.ts', startLine: 5, endLine: 8, isExported: true, content: 'function sink() {}', description: 'callee'})`, + `MATCH (a:Function), (b:Function) WHERE a.id = "func:o'd" AND b.id = 'func:sink' + CREATE (a)-[:CodeRelation {type: 'CALLS', confidence: 1.0, reason: 'direct', step: 0}]->(b)`, + ], + poolAdapter: true, + afterSetup: async (handle) => { + vi.mocked(listRegisteredRepos).mockResolvedValue([ + { + name: 'param-repo', + path: '/param/repo', + storagePath: handle.tmpHandle.dbPath, + indexedAt: new Date().toISOString(), + lastCommit: 'abc123', + stats: { files: 1, nodes: 2, communities: 0, processes: 0 }, + }, + ]); + const backend = new LocalBackend(); + await backend.init(); + (handle as any)._backend = backend; + }, + }, +); diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 55e9b9a6a..f645c1b19 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -649,19 +649,26 @@ describe('LocalBackend.callTool', () => { it('impact byDepth items include a processes field (default empty when no processes)', async () => { // Resolver returns target; BFS returns one frontier caller; no STEP_IN_PROCESS rows. - (executeParameterized as any).mockResolvedValue([ - { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, - ]); - (executeQuery as any).mockResolvedValue([ - { - id: 'func:caller', - name: 'caller', - type: 'Function', - filePath: 'src/uses-main.ts', - relType: 'CALLS', - confidence: 0.9, - }, - ]); + (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // BFS frontier query is now parameterized (#1907 U3). + if (cypher.includes('r.type IN') && !cypher.includes('STEP_IN_PROCESS')) { + return Promise.resolve([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + } + // Symbol resolution. + return Promise.resolve([ + { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, + ]); + }); + (executeQuery as any).mockResolvedValue([]); const result = await backend.callTool('impact', { target: 'main', direction: 'upstream' }); const d1 = result.byDepth?.[1] || result.byDepth?.['1'] || []; @@ -674,6 +681,19 @@ describe('LocalBackend.callTool', () => { it('impact populates byDepth processes when STEP_IN_PROCESS rows exist', async () => { (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // BFS frontier query is now parameterized (#1907 U3). + if (cypher.includes('r.type IN') && !cypher.includes('STEP_IN_PROCESS')) { + return Promise.resolve([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + } // Symbol resolver name-lookup if (cypher.includes('WHERE n.name =')) { return Promise.resolve([ @@ -739,6 +759,20 @@ describe('LocalBackend.callTool', () => { it('impact summaryOnly:true skips the per-symbol STEP_IN_PROCESS enrichment pass', async () => { // Resolver returns target; BFS returns one caller; aggregation returns one process row. (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // BFS frontier query is now parameterized (#1907 U3) — return a caller so + // the per-symbol-skip assertion below is meaningful (not vacuous). + if (cypher.includes('r.type IN') && !cypher.includes('STEP_IN_PROCESS')) { + return Promise.resolve([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/a.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + } if (cypher.includes('WHERE n.name =')) { return Promise.resolve([ { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, @@ -811,6 +845,19 @@ describe('LocalBackend.callTool', () => { await backend.init(); (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // BFS frontier query is now parameterized (#1907 U3). + if (cypher.includes('r.type IN') && !cypher.includes('STEP_IN_PROCESS')) { + return Promise.resolve([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + } // UID resolver if (cypher.includes('WHERE n.id = $uid')) { return Promise.resolve([ diff --git a/gitnexus/test/unit/cli-impact-disambiguation.test.ts b/gitnexus/test/unit/cli-impact-disambiguation.test.ts new file mode 100644 index 000000000..5c20b6bc0 --- /dev/null +++ b/gitnexus/test/unit/cli-impact-disambiguation.test.ts @@ -0,0 +1,140 @@ +/** + * Unit Tests: CLI `impact` disambiguation flag wiring (#1907) + * + * The CLI `impact` command gained --uid / --file / --kind so that, when impact + * reports an `ambiguous` target, users can follow the "disambiguate" guidance + * straight from the terminal (previously only the MCP tool accepted these). + * These tests pin that impactCommand forwards the flags to + * callTool('impact', …) under the backend's parameter names + * (target_uid / file_path / kind) — the same names the MCP impact tool uses. + * + * The LocalBackend is fully mocked: this isolates the CLI option → tool param + * mapping from any graph/DB behaviour. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { callTool, init } = vi.hoisted(() => ({ + callTool: vi.fn(), + init: vi.fn().mockResolvedValue(true), +})); + +vi.mock('../../src/mcp/local/local-backend.js', () => ({ + LocalBackend: class { + init = init; + callTool = callTool; + }, + // U4: impactCommand imports VALID_NODE_LABELS to soft-validate --kind. + VALID_NODE_LABELS: new Set(['Function', 'Class', 'Interface', 'Method', 'Constructor']), +})); + +// impactCommand prints its result via fs.writeSync(fd 1, …). Silence that so +// the assertion-only test does not write JSON to the runner's stdout. tool.ts +// uses only writeSync from node:fs, so a full mock is safe here (matches the +// pattern in tool-direct-cli.test.ts). +vi.mock('node:fs', () => ({ + writeSync: vi.fn(), +})); + +import { impactCommand } from '../../src/cli/tool.js'; + +describe('CLI impact disambiguation flags (#1907)', () => { + beforeEach(() => { + callTool.mockReset(); + callTool.mockResolvedValue({ status: 'found', impactedCount: 0 }); + }); + + it('forwards --uid/--file/--kind as target_uid/file_path/kind', async () => { + await impactCommand('get_embeddings', { + direction: 'upstream', + uid: 'Function:isma/scripts/ingest_md_file.py:get_embeddings', + file: 'isma/scripts/ingest_md_file.py', + kind: 'Function', + }); + + expect(callTool).toHaveBeenCalledTimes(1); + expect(callTool).toHaveBeenCalledWith( + 'impact', + expect.objectContaining({ + target: 'get_embeddings', + target_uid: 'Function:isma/scripts/ingest_md_file.py:get_embeddings', + file_path: 'isma/scripts/ingest_md_file.py', + kind: 'Function', + direction: 'upstream', + }), + ); + }); + + it('leaves disambiguation params undefined when no flags are supplied', async () => { + await impactCommand('AuthService', { direction: 'upstream' }); + + expect(callTool).toHaveBeenCalledTimes(1); + const params = callTool.mock.calls[0][1] as Record; + expect(params.target).toBe('AuthService'); + expect(params.target_uid).toBeUndefined(); + expect(params.file_path).toBeUndefined(); + expect(params.kind).toBeUndefined(); + }); + + // U1 (#1914 review F1): impact's positional target is now optional, so a uid + // alone resolves — parity with `context [name]`. + it('resolves uid-only with no positional target (parity with context)', async () => { + await impactCommand(undefined, { + direction: 'upstream', + uid: 'Function:src/auth.ts:login', + }); + + expect(callTool).toHaveBeenCalledTimes(1); + const params = callTool.mock.calls[0][1] as Record; + expect(params.target_uid).toBe('Function:src/auth.ts:login'); + expect(params.target).toBeUndefined(); + }); + + it('errors when neither a target nor a uid is provided', async () => { + const exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('process.exit'); + }) as never); + + await expect(impactCommand(undefined, {})).rejects.toThrow('process.exit'); + expect(exitSpy).toHaveBeenCalledWith(1); + expect(callTool).not.toHaveBeenCalled(); + + exitSpy.mockRestore(); + }); + + it('rejects a --prefixed uid value (a flag swallowed by Commander) without forwarding it', async () => { + const exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => { + throw new Error('process.exit'); + }) as never); + + await expect(impactCommand(undefined, { uid: '--file' })).rejects.toThrow('process.exit'); + expect(callTool).not.toHaveBeenCalled(); + + exitSpy.mockRestore(); + }); + + // U4 (#1914 review F3): an unknown --kind warns to stderr but still resolves. + it('warns on an unknown --kind but still forwards the request', async () => { + const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + + await impactCommand('login', { kind: 'Funktion', direction: 'upstream' }); + + expect(callTool).toHaveBeenCalledTimes(1); + const stderr = stderrSpy.mock.calls.map((c) => String(c[0])).join(''); + expect(stderr).toContain('Funktion'); + expect(stderr).toContain('not a known symbol kind'); + + stderrSpy.mockRestore(); + }); + + it('does not warn for a known --kind', async () => { + const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true); + + await impactCommand('login', { kind: 'Function', direction: 'upstream' }); + + expect(callTool).toHaveBeenCalledTimes(1); + const stderr = stderrSpy.mock.calls.map((c) => String(c[0])).join(''); + expect(stderr).not.toContain('not a known symbol kind'); + + stderrSpy.mockRestore(); + }); +}); diff --git a/gitnexus/test/unit/cli-index-help.test.ts b/gitnexus/test/unit/cli-index-help.test.ts index a50937ba7..3beaeff9a 100644 --- a/gitnexus/test/unit/cli-index-help.test.ts +++ b/gitnexus/test/unit/cli-index-help.test.ts @@ -196,13 +196,18 @@ describe('CLI help surface', () => { expect(result.stdout).toContain('--file '); }); - it('impact help keeps repo and include-tests flags', () => { + it('impact help keeps repo, include-tests, and disambiguation flags', () => { const result = runHelp('impact'); expect(result.status).toBe(0); expect(result.stdout).toContain('--depth '); expect(result.stdout).toContain('--include-tests'); expect(result.stdout).toContain('--repo '); + // Disambiguation flags (#1907) — mirror the context help test so a + // missing-flag regression on impact is caught here too. + expect(result.stdout).toContain('--uid '); + expect(result.stdout).toContain('--file '); + expect(result.stdout).toContain('--kind '); }); it('detect-changes help exposes compare scope and base-ref flags', () => { diff --git a/gitnexus/test/unit/impact-batching-grouping.test.ts b/gitnexus/test/unit/impact-batching-grouping.test.ts index 098d72cd7..e600042de 100644 --- a/gitnexus/test/unit/impact-batching-grouping.test.ts +++ b/gitnexus/test/unit/impact-batching-grouping.test.ts @@ -68,30 +68,9 @@ describe('impact: batching and grouping', () => { const chunkSizes: number[] = []; let chunkCallIndex = 0; - executeQueryMock.mockImplementation(async (...args: any[]) => { - const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); - // Depth traversal query (find related nodes) -- return 250 impacted ids - if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { - const res: any[] = []; - for (let i = 0; i < 250; i++) { - res.push({ - id: `node-${i}`, - name: `n${i}`, - filePath: `file-${i}.js`, - relType: 'CALLS', - confidence: null, - }); - } - return res; - } - - // NOTE: process-chunk enrichment previously used executeQuery; our - // implementation now calls executeParameterized for those chunks. We - // still keep this branch to support any legacy calls, but primary - // chunk tracking will be handled via executeParameterizedMock below. - - return []; - }); + // BFS frontier query is now parameterized (#1907 U3) — handled in + // executeParameterizedMock below; executeQuery is unused by the impact path. + executeQueryMock.mockImplementation(async () => []); // Handle parameterized calls (including chunked STEP_IN_PROCESS queries) executeParameterizedMock.mockImplementation(async (...args: any[]) => { @@ -117,6 +96,20 @@ describe('impact: batching and grouping', () => { }, ]; } + // BFS frontier query (parameterized #1907 U3): return the 250 impacted ids. + if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { + const res: any[] = []; + for (let i = 0; i < 250; i++) { + res.push({ + id: `node-${i}`, + name: `n${i}`, + filePath: `file-${i}.js`, + relType: 'CALLS', + confidence: null, + }); + } + return res; + } // Default target resolution return [{ id: 'sym1', name: 'Target', filePath: 'f' }]; }); @@ -151,6 +144,19 @@ describe('impact: batching and grouping', () => { executeParameterizedMock.mockImplementation(async (...args: any[]) => { const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); + // BFS frontier query (parameterized #1907 U3): return 6 impacted nodes. + if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { + const res: any[] = []; + for (let i = 0; i < 6; i++) + res.push({ + id: `node-${i}`, + name: `n${i}`, + filePath: `file-${i}.js`, + relType: 'CALLS', + confidence: null, + }); + return res; + } if (!query.includes('STEP_IN_PROCESS')) return [{ id: 'symA', name: 'TargetA', filePath: 'f' }]; // For STEP_IN_PROCESS in this test, return grouping rows @@ -190,25 +196,9 @@ describe('impact: batching and grouping', () => { ]; }); - // Prepare impacted nodes: smaller set for clarity (6 nodes -> chunk size default 100 so single chunk) - executeQueryMock.mockImplementation(async (...args: any[]) => { - const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); - if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { - // return 6 nodes - const res: any[] = []; - for (let i = 0; i < 6; i++) - res.push({ - id: `node-${i}`, - name: `n${i}`, - filePath: `file-${i}.js`, - relType: 'CALLS', - confidence: null, - }); - return res; - } - - return []; - }); + // BFS frontier query is now parameterized (#1907 U3) — handled in + // executeParameterizedMock above; executeQuery is unused by the impact path. + executeQueryMock.mockImplementation(async () => []); const params = { target: 'TargetA', direction: 'downstream', maxDepth: 1 } as any; const res = await (backend as any)._impactImpl(repoHandle, params); @@ -243,23 +233,9 @@ describe('impact: batching and grouping', () => { (backend as any).repos.set(repoHandle.id, repoHandle); (backend as any).ensureInitialized = vi.fn().mockResolvedValue(undefined); - // Depth traversal returns 500 impacted nodes - executeQueryMock.mockImplementation(async (...args: any[]) => { - const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); - if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { - const res: any[] = []; - for (let i = 0; i < 500; i++) - res.push({ - id: `node-${i}`, - name: `n${i}`, - filePath: `file-${i}.js`, - relType: 'CALLS', - confidence: null, - }); - return res; - } - return []; - }); + // BFS frontier query is now parameterized (#1907 U3) — handled in + // executeParameterizedMock below; executeQuery is unused by the impact path. + executeQueryMock.mockImplementation(async () => []); const chunkSizes: number[] = []; @@ -294,6 +270,19 @@ describe('impact: batching and grouping', () => { return [{ name: 'ModuleA' }]; } + // BFS frontier query (parameterized #1907 U3): return 500 impacted nodes. + if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { + const res: any[] = []; + for (let i = 0; i < 500; i++) + res.push({ + id: `node-${i}`, + name: `n${i}`, + filePath: `file-${i}.js`, + relType: 'CALLS', + confidence: null, + }); + return res; + } // Default: target resolution return [{ id: 'symX', name: 'TargetX', filePath: 'f' }]; }); diff --git a/gitnexus/test/unit/impact-pagination.test.ts b/gitnexus/test/unit/impact-pagination.test.ts index a9d604c26..163159240 100644 --- a/gitnexus/test/unit/impact-pagination.test.ts +++ b/gitnexus/test/unit/impact-pagination.test.ts @@ -46,30 +46,35 @@ function makeBackend() { return { backend, repoHandle }; } +// The BFS frontier query is now parameterized (bound $frontierIds/$relTypes, +// #1907 U3), so the caller rows come back through executeParameterizedMock +// (matched on `r.type IN`) rather than executeQueryMock. Symbol resolution and +// the label-enrichment UNION still fall through to the default symbol row. function setupMultiDepthHub(d1Count: number, d2Count: number) { let depth = 0; executeParameterizedMock.mockImplementation(async (...args: any[]) => { const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); if (query.includes('STEP_IN_PROCESS')) return []; if (query.includes('MEMBER_OF')) return []; + if (query.includes('r.type IN')) { + depth++; + const count = depth === 1 ? d1Count : depth === 2 ? d2Count : 0; + const res: any[] = []; + for (let i = 0; i < count; i++) { + res.push({ + id: `d${depth}-caller-${i}`, + name: `d${depth}caller${i}`, + filePath: `src/d${depth}-caller-${i}.ts`, + relType: 'CALLS', + confidence: null, + }); + } + return res; + } return [{ id: 'hub1', name: 'HubSymbol', filePath: 'hub.ts' }]; }); - executeQueryMock.mockImplementation(async () => { - depth++; - const count = depth === 1 ? d1Count : depth === 2 ? d2Count : 0; - const res: any[] = []; - for (let i = 0; i < count; i++) { - res.push({ - id: `d${depth}-caller-${i}`, - name: `d${depth}caller${i}`, - filePath: `src/d${depth}-caller-${i}.ts`, - relType: 'CALLS', - confidence: null, - }); - } - return res; - }); + executeQueryMock.mockImplementation(async () => []); } function setupHubSymbol(count: number) { @@ -77,12 +82,7 @@ function setupHubSymbol(count: number) { const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); if (query.includes('STEP_IN_PROCESS')) return []; if (query.includes('MEMBER_OF')) return []; - return [{ id: 'hub1', name: 'HubSymbol', filePath: 'hub.ts' }]; - }); - - executeQueryMock.mockImplementation(async (...args: any[]) => { - const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); - if (query.includes('r.type IN') && !query.includes('STEP_IN_PROCESS')) { + if (query.includes('r.type IN')) { const res: any[] = []; for (let i = 0; i < count; i++) { res.push({ @@ -95,8 +95,10 @@ function setupHubSymbol(count: number) { } return res; } - return []; + return [{ id: 'hub1', name: 'HubSymbol', filePath: 'hub.ts' }]; }); + + executeQueryMock.mockImplementation(async () => []); } describe('impact: pagination and summaryOnly (#414)', () => {