diff --git a/MIGRATION.md b/MIGRATION.md index ec9fdabc2..88488b0ae 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -1,5 +1,49 @@ # Migration Guide +## `impact` tool may now return `{ status: 'ambiguous' }` (PR #888, issue #470) + +Before this change the `impact` MCP tool silently picked the first match +when the `target` name hit multiple symbols (Class → Interface → Function +→ Method → Constructor priority UNION). This often produced analysis for +the wrong symbol with no signal back to the caller. + +After this change, when the resolver finds more than one viable match +and the caller supplied none of `target_uid` / `file_path` / `kind`, +`impact` returns a disambiguation response shaped like: + +```json +{ + "status": "ambiguous", + "message": "Found N symbols matching ''. Use target_uid, file_path, or kind to disambiguate.", + "target": { "name": "" }, + "direction": "upstream", + "impactedCount": 0, + "risk": "UNKNOWN", + "candidates": [ + { "uid": "...", "name": "...", "kind": "Function", "filePath": "...", "line": 42, "score": 0.76 } + ] +} +``` + +### Do I need to migrate? + +**Probably not, but check for assumptions.** Callers that unconditionally +read `result.byDepth` / `result.summary` / `result.affected_processes` +without first checking `result.status` will now see `undefined` in the +ambiguous case. The fix is to branch on `result.status === 'ambiguous'` +first and follow up with `target_uid` (preferred) or `file_path` / `kind`. + +The `context` tool's ambiguous response is a strict superset of the +existing shape — every candidate gains a `score` field, no existing field +has changed. No migration required for `context` callers. + +### What happens on re-index? + +Nothing — this is an MCP-surface change only. The graph schema, indexer, +and stored data are untouched. + +--- + ## OVERRIDES → METHOD_OVERRIDES (PR #642) The `OVERRIDES` relationship type has been renamed to `METHOD_OVERRIDES` for diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index ea0529540..fe63ee1a2 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -1079,9 +1079,296 @@ export class LocalBackend { return result; } + /** + * Patch the `type` field on candidates whose `labels(n)[0]` projection + * came back empty — a known LadybugDB behaviour for several node types. + * + * Uses one scoped UNION query across the five priority labels rather + * than per-candidate round-trips, so cost is a single DB call regardless + * of how many candidates need enrichment. No-op when every candidate + * already has a non-empty type. + * + * Failures are swallowed: label enrichment is an optimisation for + * downstream scoring and #480 Class/Interface BFS seeding; if it fails + * the symbol still resolves, just without the kind-priority bonus. + */ + private async enrichCandidateLabels( + repo: RepoHandle, + candidates: Array<{ id: string; type: string }>, + ): Promise { + const ids = candidates.filter((c) => c.type === '' && c.id).map((c) => c.id); + if (ids.length === 0) return; + try { + const rows = await executeParameterized( + repo.id, + ` + MATCH (n:\`Class\`) WHERE n.id IN $ids RETURN n.id AS id, 'Class' AS label + UNION ALL + MATCH (n:\`Interface\`) WHERE n.id IN $ids RETURN n.id AS id, 'Interface' AS label + UNION ALL + MATCH (n:\`Function\`) WHERE n.id IN $ids RETURN n.id AS id, 'Function' AS label + UNION ALL + MATCH (n:\`Method\`) WHERE n.id IN $ids RETURN n.id AS id, 'Method' AS label + UNION ALL + MATCH (n:\`Constructor\`) WHERE n.id IN $ids RETURN n.id AS id, 'Constructor' AS label + `, + { ids }, + ); + const labelById = new Map(); + for (const r of rows as any[]) { + const id = (r.id ?? r[0]) as string; + const label = (r.label ?? r[1]) as string; + if (id && label && !labelById.has(id)) labelById.set(id, label); + } + for (const c of candidates) { + if (c.type === '' && labelById.has(c.id)) c.type = labelById.get(c.id) as string; + } + } catch { + /* best-effort — downstream resolvers still work without the label */ + } + } + + /** + * Score a symbol candidate for disambiguation ranking. + * + * Deterministic, no DB round-trip: + * - base 0.50 + * - +0.40 when file_path hint matches (substring, case-insensitive) + * - +0.20 when kind hint exactly matches the candidate's kind + * - when no kind hint, a small priority bonus (Class > Interface > + * Function > Method > Constructor) to preserve the intuition that + * class-level names are usually what the user wanted. + * + * Capped at 1.0. Intentionally simple and inspectable — a future v2 can + * plug in BM25/embedding signals here without changing the surrounding + * resolver shape. + */ + private scoreCandidate( + c: { kind: string; filePath: string }, + hints: { file_path?: string; kind?: string }, + ): number { + let s = 0.5; + if (hints.file_path && c.filePath && typeof c.filePath === 'string') { + if (c.filePath.toLowerCase().includes(hints.file_path.toLowerCase())) { + s += 0.4; + } + } + if (hints.kind && c.kind === hints.kind) { + s += 0.2; + } + if (!hints.kind) { + const priority: Record = { + Class: 5, + Interface: 4, + Function: 3, + Method: 2, + Constructor: 1, + }; + s += (priority[c.kind] ?? 0) * 0.02; + } + return Math.min(1.0, s); + } + + /** + * Shared symbol resolver used by `context` and `impact`. + * + * Returns one of: + * - `{ kind: 'ok', symbol, resolvedLabel }` — single confident match + * (either direct UID, only one candidate after filtering, Class/ + * Constructor collapse, or a top-scoring candidate with a clear gap + * to the runner-up). + * - `{ kind: 'ambiguous', candidates }` — multiple viable matches, + * sorted by score desc. Each candidate carries a relevance score. + * - `{ kind: 'not_found' }` — no matches at all. + * + * Preserves the #480 Class/Constructor preference: when the only + * ambiguity is between a Class and its own Constructor (same name, + * same filePath), the Class wins silently. + */ + private async resolveSymbolCandidates( + repo: RepoHandle, + query: { uid?: string; name?: string; include_content?: boolean }, + hints: { file_path?: string; kind?: string }, + ): Promise< + | { + kind: 'ok'; + symbol: { + id: string; + name: string; + type: string; + filePath: string; + startLine: number; + endLine: number; + content?: string; + }; + resolvedLabel: string; + } + | { + kind: 'ambiguous'; + candidates: Array<{ + id: string; + name: string; + type: string; + filePath: string; + startLine: number; + endLine: number; + score: number; + }>; + } + | { kind: 'not_found' } + > { + const { uid, name, include_content } = query; + const selectClause = `n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine${include_content ? ', n.content AS content' : ''}`; + + // Direct UID — zero-ambiguity path. + if (uid) { + const rows = await executeParameterized( + repo.id, + `MATCH (n {id: $uid}) RETURN ${selectClause} LIMIT 1`, + { uid }, + ); + if (rows.length === 0) return { kind: 'not_found' }; + const r = rows[0] as any; + const symbol = { + id: (r.id ?? r[0]) as string, + name: (r.name ?? r[1]) as string, + type: (r.type ?? r[2] ?? '') as string, + filePath: (r.filePath ?? r[3]) as string, + startLine: (r.startLine ?? r[4]) as number, + endLine: (r.endLine ?? r[5]) as number, + ...(include_content ? { content: (r.content ?? r[6]) as string | undefined } : {}), + }; + // Same LadybugDB label-enrichment as the name-based path: a UID + // pointing at a Class must still surface `type: 'Class'` so impact's + // Class/Interface BFS seed fires. No-op when type is already set. + await this.enrichCandidateLabels(repo, [symbol]); + return { kind: 'ok', symbol, resolvedLabel: symbol.type }; + } + + if (!name) return { kind: 'not_found' }; + + const isQualified = name.includes('/') || name.includes(':'); + let whereClause: string; + const queryParams: Record = { symName: name }; + if (hints.file_path) { + whereClause = `WHERE n.name = $symName AND n.filePath CONTAINS $filePath`; + queryParams.filePath = hints.file_path; + } else if (isQualified) { + whereClause = `WHERE n.id = $symName OR n.name = $symName`; + } else { + whereClause = `WHERE n.name = $symName`; + } + + // LIMIT 20 (was 10) — scoring is the point now, so give the ranker + // headroom instead of arbitrary truncation. + const rows = await executeParameterized( + repo.id, + `MATCH (n) ${whereClause} RETURN ${selectClause} LIMIT 20`, + queryParams, + ); + + if (rows.length === 0) return { kind: 'not_found' }; + + // Normalise row shape across object / tuple returns from LadybugDB. + const normalized = rows.map((r: any) => ({ + id: (r.id ?? r[0]) as string, + name: (r.name ?? r[1]) as string, + type: (r.type ?? r[2] ?? '') as string, + filePath: (r.filePath ?? r[3]) as string, + startLine: (r.startLine ?? r[4]) as number, + endLine: (r.endLine ?? r[5]) as number, + ...(include_content ? { content: (r.content ?? r[6]) as string | undefined } : {}), + })); + + // Enrich labels for any candidates where `labels(n)[0]` came back empty. + // LadybugDB returns an empty string for that projection on certain node + // types (notably Class), which left downstream consumers (impact's + // Class/Interface BFS seed, the kind-priority scoring bonus) unable to + // distinguish a Class target from "unknown kind". One scoped UNION + // across the five priority labels patches the type in-place without + // per-candidate round-trips. + await this.enrichCandidateLabels(repo, normalized); + + // Preserve #480 Class/Constructor collapse: if we have exactly one + // Class (or Interface) candidate and one Constructor sharing name + + // filePath, fold into the Class. This used to require a follow-up + // label query because LadybugDB sometimes returns an empty labels()[0] + // for Class nodes — enrichment above handles the empty-type case, but + // the `type === 'Constructor'` gate still correctly triggers when a + // Class and its Constructor share the name. + if (!hints.kind && normalized.length > 1) { + const ambiguousType = normalized.some((s) => s.type === '' || s.type === 'Constructor'); + if (ambiguousType) { + const candidateIds = normalized.map((s) => s.id).filter(Boolean); + for (const label of ['Class', 'Interface']) { + const labelRows = await executeParameterized( + repo.id, + `MATCH (n:\`${label}\`) WHERE n.id IN $candidateIds RETURN n.id AS id LIMIT 1`, + { candidateIds }, + ).catch(() => []); + if (labelRows.length > 0) { + const preferredId = (labelRows[0] as any).id ?? (labelRows[0] as any)[0]; + const preferred = normalized.find((s) => s.id === preferredId); + if (preferred) { + return { + kind: 'ok', + symbol: preferred, + resolvedLabel: label, + }; + } + } + } + } + } + + if (normalized.length === 1) { + return { + kind: 'ok', + symbol: normalized[0], + resolvedLabel: '', + }; + } + + // Score, sort desc, stable tiebreak on shorter filePath then lex uid. + const scored = normalized.map((s) => ({ + ...s, + score: this.scoreCandidate({ kind: s.type, filePath: s.filePath || '' }, hints), + })); + scored.sort((a, b) => { + if (b.score !== a.score) return b.score - a.score; + const fpA = (a.filePath || '').length; + const fpB = (b.filePath || '').length; + if (fpA !== fpB) return fpA - fpB; + return String(a.id).localeCompare(String(b.id)); + }); + + // Confident single-result: top score ≥ 0.95 AND beats runner-up by a + // clear margin. This lets a very strong file_path/kind hint resolve + // cleanly instead of forcing the caller through a disambiguation + // round-trip. + // + // The gap threshold uses `> 0.09` rather than `>= 0.10` on purpose: + // IEEE754 addition of the scoring terms (0.50 + 0.40 + 0.20 - 0.90 + // yields 0.09999999999999998, not exactly 0.10) would otherwise break + // the comparison for legitimate "top is 1.00, runner is 0.90" cases. + // The intent is a clearly-dominant winner; 0.09 is a large enough + // margin to mean that unambiguously. + // + // The `scored.length >= 2` guard is defensive. The `normalized.length === 1` + // early return above already handles the single-candidate path, so in + // practice `scored` always has at least two elements by the time we get + // here — keeping the guard means changes to the upstream early-return + // logic cannot accidentally index out of bounds at `scored[1]`. + if (scored.length >= 2 && scored[0].score >= 0.95 && scored[0].score - scored[1].score > 0.09) { + return { kind: 'ok', symbol: scored[0], resolvedLabel: scored[0].type }; + } + + return { kind: 'ambiguous', candidates: scored }; + } + /** * Context tool — 360-degree symbol view with categorized refs. - * Disambiguation when multiple symbols share a name. + * Disambiguation (ranked) when multiple symbols share a name. * UID-based direct lookup. No cluster in output. */ private async context( @@ -1090,124 +1377,47 @@ export class LocalBackend { name?: string; uid?: string; file_path?: string; + kind?: string; include_content?: boolean; }, ): Promise { await this.ensureInitialized(repo.id); - const { name, uid, file_path, include_content } = params; + const { name, uid, file_path, kind, include_content } = params; if (!name && !uid) { return { error: 'Either "name" or "uid" parameter is required.' }; } - // Step 1: Find the symbol - let symbols: any[]; + const outcome = await this.resolveSymbolCandidates( + repo, + { uid, name, include_content }, + { file_path, kind }, + ); - if (uid) { - symbols = await executeParameterized( - repo.id, - ` - MATCH (n {id: $uid}) - RETURN n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine${include_content ? ', n.content AS content' : ''} - LIMIT 1 - `, - { uid }, - ); - } else { - const isQualified = name!.includes('/') || name!.includes(':'); - - let whereClause: string; - let queryParams: Record; - if (file_path) { - whereClause = `WHERE n.name = $symName AND n.filePath CONTAINS $filePath`; - queryParams = { symName: name!, filePath: file_path }; - } else if (isQualified) { - whereClause = `WHERE n.id = $symName OR n.name = $symName`; - queryParams = { symName: name! }; - } else { - whereClause = `WHERE n.name = $symName`; - queryParams = { symName: name! }; - } - - symbols = await executeParameterized( - repo.id, - ` - MATCH (n) ${whereClause} - RETURN n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine${include_content ? ', n.content AS content' : ''} - LIMIT 10 - `, - queryParams, - ); - } - - if (symbols.length === 0) { + if (outcome.kind === 'not_found') { return { error: `Symbol '${name || uid}' not found` }; } - // Step 2: Disambiguation - // When multiple nodes share the same name (e.g. a Java Class and its - // Constructor both named 'SessionTracker'), prefer the Class node so - // context() returns the semantically meaningful result rather than - // triggering ambiguous disambiguation (#480). - // labels(n)[0] returns empty string in LadybugDB, so we resolve the - // preferred node by re-querying with explicit label filters, scoped to - // the candidate IDs already in symbols. - // - // Guard: only attempt Class-preference when at least one candidate has an - // empty/unknown type (LadybugDB limitation) or is a Constructor — meaning - // the ambiguity may be a Class/Constructor name collision rather than two - // genuinely distinct symbols (e.g. two Functions in different files). - // - // resolvedLabel is set here and threaded to Step 3 to avoid a redundant - // classCheck round-trip later. - let resolvedLabel = ''; - if (symbols.length > 1 && !uid) { - const hasAmbiguousType = symbols.some((s: any) => { - const t = s.type || s[2] || ''; - return t === '' || t === 'Constructor'; - }); - if (hasAmbiguousType) { - const candidateIds = symbols.map((s: any) => s.id || s[0]).filter(Boolean); - const PREFER_LABELS = ['Class', 'Interface']; - let preferred: any = null; - for (const label of PREFER_LABELS) { - const match = await executeParameterized( - repo.id, - ` - MATCH (n:\`${label}\`) WHERE n.id IN $candidateIds RETURN n.id AS id LIMIT 1 - `, - { candidateIds }, - ).catch(() => []); - if (match.length > 0) { - preferred = symbols.find((s: any) => (s.id || s[0]) === (match[0].id || match[0][0])); - if (preferred) { - resolvedLabel = label; - break; - } - } - } - if (preferred) symbols = [preferred]; - } - } - - if (symbols.length > 1 && !uid) { + if (outcome.kind === 'ambiguous') { return { status: 'ambiguous', - message: `Found ${symbols.length} symbols matching '${name}'. Use uid or file_path to disambiguate.`, - candidates: symbols.map((s: any) => ({ - uid: s.id || s[0], - name: s.name || s[1], - kind: s.type || s[2], - filePath: s.filePath || s[3], - line: s.startLine || s[4], + message: `Found ${outcome.candidates.length} symbols matching '${name}'. Use uid, file_path, or kind to disambiguate.`, + candidates: outcome.candidates.map((c) => ({ + uid: c.id, + name: c.name, + kind: c.type, + filePath: c.filePath, + line: c.startLine, + score: Number(c.score.toFixed(2)), })), }; } // Step 3: Build full context - const sym = symbols[0]; - const symId = sym.id || sym[0]; + const sym = outcome.symbol; + const resolvedLabel = outcome.resolvedLabel; + const symId = sym.id; // Categorized incoming refs const incomingRows = await executeParameterized( @@ -1901,6 +2111,9 @@ export class LocalBackend { repo: RepoHandle, params: { target: string; + target_uid?: string; + file_path?: string; + kind?: string; direction: 'upstream' | 'downstream'; maxDepth?: number; relationTypes?: string[]; @@ -1927,6 +2140,9 @@ export class LocalBackend { repo: RepoHandle, params: { target: string; + target_uid?: string; + file_path?: string; + kind?: string; direction: 'upstream' | 'downstream'; maxDepth?: number; relationTypes?: string[]; @@ -1969,65 +2185,57 @@ export class LocalBackend { const includeTests = params.includeTests ?? false; const minConfidence = params.minConfidence ?? 0; - // Resolve target by name, preferring Class/Interface over Constructor - // (fix #480: Java class and constructor share the same name). - // labels(n)[0] returns empty string in LadybugDB, so we use explicit - // label-typed sub-queries in a single UNION ordered by priority to avoid - // up to 6 serial round-trips for non-Class targets. - let sym: any = null; - let symType = ''; + // Resolve target via the shared symbol resolver. When the caller passes + // target_uid we skip the name lookup entirely (zero-ambiguity). Otherwise + // we rank candidates (#470) and either proceed with a confident single + // match, or return a structured ambiguous response instead of silently + // picking the wrong symbol. + // + // The resolver preserves the #480 Class/Constructor preference heuristic: + // when a Class and its Constructor share name + filePath, the Class is + // selected silently. + const outcome = await this.resolveSymbolCandidates( + repo, + { uid: params.target_uid, name: target }, + { file_path: params.file_path, kind: params.kind }, + ); - try { - const rows = await executeParameterized( - repo.id, - ` - MATCH (n:\`Class\`) WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath, 0 AS priority LIMIT 1 - UNION ALL - MATCH (n:\`Interface\`) WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath, 1 AS priority LIMIT 1 - UNION ALL - MATCH (n:\`Function\`) WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath, 2 AS priority LIMIT 1 - UNION ALL - MATCH (n:\`Method\`) WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath, 3 AS priority LIMIT 1 - UNION ALL - MATCH (n:\`Constructor\`) WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath, 4 AS priority LIMIT 1 - `, - { targetName: target }, - ).catch(() => []); - - if (rows.length > 0) { - // Pick the row with the lowest priority value (Class wins over Constructor) - const best = rows.reduce((a: any, b: any) => - (a.priority ?? a[3] ?? 99) <= (b.priority ?? b[3] ?? 99) ? a : b, - ); - sym = best; - const priorityToLabel = ['Class', 'Interface', 'Function', 'Method', 'Constructor']; - symType = priorityToLabel[best.priority ?? best[3]] ?? ''; - } - } catch { - /* fall through to unlabeled match */ + if (outcome.kind === 'not_found') { + const missing = params.target_uid ?? target; + return { + error: `Target '${missing}' not found`, + target: { name: target }, + direction, + impactedCount: 0, + risk: 'UNKNOWN', + }; } - // Fall back to unlabeled match for any other node type - if (!sym) { - const rows = await executeParameterized( - repo.id, - ` - MATCH (n) - WHERE n.name = $targetName - RETURN n.id AS id, n.name AS name, n.filePath AS filePath - LIMIT 1 - `, - { targetName: target }, - ); - if (rows.length > 0) sym = rows[0]; + if (outcome.kind === 'ambiguous') { + return { + status: 'ambiguous', + message: `Found ${outcome.candidates.length} symbols matching '${target}'. Use target_uid, file_path, or kind to disambiguate.`, + target: { name: target }, + direction, + impactedCount: 0, + risk: 'UNKNOWN', + candidates: outcome.candidates.map((c) => ({ + uid: c.id, + name: c.name, + kind: c.type, + filePath: c.filePath, + line: c.startLine, + score: Number(c.score.toFixed(2)), + })), + }; } - if (!sym) return { error: `Target '${target}' not found` }; + const sym = { + id: outcome.symbol.id, + name: outcome.symbol.name, + filePath: outcome.symbol.filePath, + }; + const symType = outcome.resolvedLabel || outcome.symbol.type || ''; return this._runImpactBFS(repo, sym, symType, direction, { maxDepth, diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 2c884d10b..7beb2e970 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -154,7 +154,7 @@ Shows categorized incoming/outgoing references (calls, imports, extends, impleme WHEN TO USE: After query() to understand a specific symbol in depth. When you need to know all callers, callees, and what execution flows a symbol participates in. AFTER THIS: Use impact() if planning changes, or READ gitnexus://repo/{name}/process/{processName} for full execution trace. -Handles disambiguation: if multiple symbols share the same name, returns candidates for you to pick from. Use uid param for zero-ambiguity lookup from prior results. +Handles disambiguation: if multiple symbols share the same name, returns ranked candidates (each with a relevance score) for you to pick from. Use uid for zero-ambiguity lookup, or narrow the search with file_path and/or kind hints. NOTE: ACCESSES edges (field read/write tracking) are included in context results with reason 'read' or 'write'. CALLS edges resolve through field access chains and method-call chains (e.g., user.address.getCity().save() produces CALLS edges at each step).`, inputSchema: { @@ -166,6 +166,11 @@ NOTE: ACCESSES edges (field read/write tracking) are included in context results description: 'Direct symbol UID from prior tool results (zero-ambiguity lookup)', }, file_path: { type: 'string', description: 'File path to disambiguate common names' }, + kind: { + type: 'string', + description: + "Kind filter to disambiguate common names (e.g. 'Function', 'Class', 'Method', 'Interface', 'Constructor')", + }, include_content: { type: 'boolean', description: 'Include full symbol source code (default: false)', @@ -265,16 +270,32 @@ Depth groups: TIP: Default traversal uses CALLS/IMPORTS/EXTENDS/IMPLEMENTS. For class members, include HAS_METHOD and HAS_PROPERTY in relationTypes. For field access analysis, include ACCESSES in relationTypes. +Handles disambiguation: when multiple symbols share the target name, returns ranked candidates (each with a relevance score) instead of silently picking one. Use target_uid for zero-ambiguity lookup, or narrow with file_path and/or kind hints. + EdgeType: CALLS, IMPORTS, EXTENDS, IMPLEMENTS, HAS_METHOD, HAS_PROPERTY, METHOD_OVERRIDES, METHOD_IMPLEMENTS, ACCESSES Confidence: 1.0 = certain, <0.8 = fuzzy match`, inputSchema: { type: 'object', properties: { target: { type: 'string', description: 'Name of function, class, or file to analyze' }, + target_uid: { + type: 'string', + description: + 'Direct symbol UID from prior tool results (zero-ambiguity lookup, skips target resolution)', + }, direction: { type: 'string', description: 'upstream (what depends on this) or downstream (what this depends on)', }, + file_path: { + type: 'string', + description: 'File path hint to disambiguate common names', + }, + kind: { + type: 'string', + description: + "Kind filter to disambiguate common names (e.g. 'Function', 'Class', 'Method', 'Interface', 'Constructor')", + }, maxDepth: { type: 'number', description: 'Max relationship depth (default: 3)', diff --git a/gitnexus/test/integration/local-backend-calltool.test.ts b/gitnexus/test/integration/local-backend-calltool.test.ts index 55e4110f3..52f9b2487 100644 --- a/gitnexus/test/integration/local-backend-calltool.test.ts +++ b/gitnexus/test/integration/local-backend-calltool.test.ts @@ -141,12 +141,20 @@ withTestLbugDB( }); it('filters by OVERRIDES only', async () => { + // The seed has two Method nodes named 'authenticate' (AuthService's + // override and BaseService's base). Per #470, `impact` now returns + // a ranked-ambiguous response when the target name hits multiple + // symbols, so we must disambiguate with file_path to get the + // AuthService override (the one with the outgoing METHOD_OVERRIDES + // edge we want to follow downstream). const result = await backend.callTool('impact', { target: 'authenticate', + file_path: 'src/auth.ts', direction: 'downstream', relationTypes: ['METHOD_OVERRIDES'], }); expect(result).not.toHaveProperty('error'); + expect(result.status).not.toBe('ambiguous'); // AuthService.authenticate overrides BaseService.authenticate expect(result.impactedCount).toBeGreaterThanOrEqual(1); const d1 = result.byDepth[1] || result.byDepth['1'] || []; @@ -158,12 +166,15 @@ withTestLbugDB( // Pass the LEGACY alias 'OVERRIDES' — impactByUid should flatMap-expand // it to ['OVERRIDES', 'METHOD_OVERRIDES'] so the METHOD_OVERRIDES edge // between BaseService.authenticate and AuthService.authenticate is found. + // file_path hint disambiguates the two 'authenticate' methods per #470. const result = await backend.callTool('impact', { target: 'authenticate', + file_path: 'src/auth.ts', direction: 'downstream', relationTypes: ['OVERRIDES'], }); expect(result).not.toHaveProperty('error'); + expect(result.status).not.toBe('ambiguous'); expect(result.impactedCount).toBeGreaterThanOrEqual(1); const d1 = result.byDepth[1] || result.byDepth['1'] || []; const names = d1.map((d: any) => d.name); diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index ebccb42a2..9a90b7030 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -248,6 +248,215 @@ describe('LocalBackend.callTool', () => { const result = await backend.callTool('context', { name: 'main' }); expect(result.status).toBe('ambiguous'); expect(result.candidates).toHaveLength(2); + + // #470: every candidate carries a relevance score in [0, 1] and the list + // is sorted descending by score (with deterministic tiebreakers). + for (const c of result.candidates) { + expect(typeof c.score).toBe('number'); + expect(c.score).toBeGreaterThanOrEqual(0); + expect(c.score).toBeLessThanOrEqual(1); + } + expect(result.candidates[0].score).toBeGreaterThanOrEqual(result.candidates[1].score); + }); + + it('context tool ranks file_path match higher than non-match (#470)', async () => { + (executeParameterized as any).mockResolvedValue([ + { + id: 'func:handleConnect:1', + name: 'handleConnect', + type: 'Function', + filePath: 'src/lib/socket.ts', + startLine: 10, + endLine: 20, + }, + { + id: 'func:handleConnect:2', + name: 'handleConnect', + type: 'Function', + filePath: 'src/App.tsx', + startLine: 42, + endLine: 60, + }, + ]); + const result = await backend.callTool('context', { + name: 'handleConnect', + file_path: 'App.tsx', + }); + // In production, `WHERE n.filePath CONTAINS $filePath` would pre-filter + // at the DB layer and only `src/App.tsx` would come back — resolving + // via the single-candidate early return rather than via scoring. The + // `executeParameterized` mock here returns both rows regardless of the + // WHERE clause parameters, so this asserts that the resolver ends up + // picking the App.tsx candidate in either case (via mock-relaxed DB + // pre-filter or via scoring promotion). The dedicated scoring-promotion + // path is covered by the next `it()` block below. + expect(result.status).toBe('found'); + expect(result.symbol.filePath).toBe('src/App.tsx'); + }); + + it('context tool promotes top candidate via scoring when multiple rows survive DB pre-filter (#470)', async () => { + // This test explicitly exercises the scored-promotion path (#470 + // review): both candidates satisfy the file_path hint (so DB + // pre-filter would return both in production), and promotion is + // determined purely by the combined file_path + kind score. + (executeParameterized as any).mockResolvedValue([ + { + id: 'fn:App:1', + name: 'render', + type: 'Function', + filePath: 'src/components/App.tsx', + startLine: 10, + endLine: 20, + }, + { + id: 'method:App:1', + name: 'render', + type: 'Method', + filePath: 'src/pages/App.tsx', + startLine: 5, + endLine: 15, + }, + ]); + const result = await backend.callTool('context', { + name: 'render', + file_path: 'App.tsx', + kind: 'Function', + }); + // Expected scoring: + // Function candidate: 0.50 base + 0.40 file_path + 0.20 kind = 1.10 → cap 1.00 + // Method candidate: 0.50 base + 0.40 file_path + 0.00 kind = 0.90 + // Top score ≥ 0.95 and beats runner-up by 0.10 → confident promotion + // to `{ status: 'found' }` with the Function. + expect(result.status).toBe('found'); + expect(result.symbol.filePath).toBe('src/components/App.tsx'); + expect(result.symbol.kind).toBe('Function'); + }); + + it('context tool returns ranked candidates when file_path only partially narrows (#470)', async () => { + (executeParameterized as any).mockResolvedValue([ + { + id: 'func:foo:1', + name: 'foo', + type: 'Function', + filePath: 'src/a.ts', + startLine: 1, + endLine: 5, + }, + { + id: 'func:foo:2', + name: 'foo', + type: 'Function', + filePath: 'src/b.ts', + startLine: 1, + endLine: 5, + }, + ]); + // No hints → both candidates score 0.56 (0.50 base + 0.06 Function + // priority). Tied scores fall back to deterministic tiebreakers. + const result = await backend.callTool('context', { name: 'foo' }); + expect(result.status).toBe('ambiguous'); + expect(result.candidates).toHaveLength(2); + expect(result.candidates[0].score).toBeCloseTo(0.56, 2); + expect(result.candidates[1].score).toBeCloseTo(0.56, 2); + }); + + it('context tool boosts the candidate whose kind matches the hint (#470)', async () => { + (executeParameterized as any).mockResolvedValue([ + { + id: 'method:save:1', + name: 'save', + type: 'Method', + filePath: 'src/service.ts', + startLine: 10, + endLine: 20, + }, + { + id: 'func:save:1', + name: 'save', + type: 'Function', + filePath: 'src/util.ts', + startLine: 5, + endLine: 15, + }, + ]); + const result = await backend.callTool('context', { name: 'save', kind: 'Function' }); + // When kind hint is given, kind-priority bonus is suppressed and +0.20 + // kind-match bonus applies instead. Function becomes the top candidate. + expect(result.status).toBe('ambiguous'); + expect(result.candidates[0].kind).toBe('Function'); + expect(result.candidates[0].score).toBeGreaterThan(result.candidates[1].score); + }); + + it('impact tool returns ambiguous shape with ranked candidates when target has multiple matches (#470)', async () => { + // resolveSymbolCandidates issues a single name query; mock it to return + // two Function rows in different files with no hints. + (executeParameterized as any).mockResolvedValue([ + { + id: 'func:login:1', + name: 'login', + type: 'Function', + filePath: 'src/auth.ts', + startLine: 5, + endLine: 15, + }, + { + id: 'func:login:2', + name: 'login', + type: 'Function', + filePath: 'src/admin/login.ts', + startLine: 8, + endLine: 20, + }, + ]); + + const result = await backend.callTool('impact', { target: 'login', direction: 'upstream' }); + + expect(result.status).toBe('ambiguous'); + expect(result.candidates).toHaveLength(2); + expect(result.impactedCount).toBe(0); + expect(result.risk).toBe('UNKNOWN'); + expect(result.target.name).toBe('login'); + for (const c of result.candidates) { + expect(typeof c.score).toBe('number'); + expect(c.uid).toBeDefined(); + expect(c.kind).toBe('Function'); + } + }); + + it('impact tool resolves via target_uid without running the name-based resolver (#470)', async () => { + // UID path: exactly one executeParameterized call for the lookup, then + // the BFS issues executeQuery calls (which we mock empty). Crucially, + // no `WHERE n.name =` query fires. + (executeParameterized as any).mockResolvedValue([ + { + id: 'uid:1234', + name: 'pickedByUid', + type: 'Function', + filePath: 'src/pick.ts', + startLine: 1, + endLine: 10, + }, + ]); + (executeQuery as any).mockResolvedValue([]); + + const result = await backend.callTool('impact', { + target: 'ignoredName', + target_uid: 'uid:1234', + direction: 'upstream', + }); + + // No ambiguous shape and no name-lookup error — the uid short-circuit won. + expect(result.status).not.toBe('ambiguous'); + expect(result.target).toBeDefined(); + + // All executeParameterized calls this test dispatched must have been + // uid-keyed, never name-keyed. That proves the name resolver was skipped. + const calls = (executeParameterized as any).mock.calls as Array< + [string, string, Record] + >; + for (const [, cypher] of calls) { + expect(cypher).not.toMatch(/WHERE n\.name = \$symName/); + } }); it('dispatches impact tool', async () => {