From 131d411ae499ef8bfed14f326123b2881313ecb8 Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Sat, 18 Apr 2026 12:52:42 +0100 Subject: [PATCH] feat(mcp): rank context/impact disambiguation candidates and expose kind/file_path hints (#888) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(mcp): rank context/impact disambiguation candidates and expose kind/file_path hints The `context` MCP tool already returned `{ status: 'ambiguous', candidates }` when a name hit multiple symbols, but the candidates were returned in arbitrary DB order and the only hint it accepted was file_path. The `impact` tool was worse: when its name resolver found multiple viable matches it silently picked the first one from a priority UNION, with no signal back to the caller that a different symbol might have been intended. Both failure modes were flagged in issue #470 and reconfirmed in the comments by a second user who described impact as returning "incorrect parsing results and meaningless tool calls" in the multi-match case. Changes: * Add `resolveSymbolCandidates(repo, query, hints)` private helper on LocalBackend. Single place that: - Short-circuits on direct uid (zero-ambiguity) - Runs the same name-or-qualified-id match as before, with LIMIT 20 (was 10) so the ranker has headroom instead of arbitrary truncation - Preserves the #480 Class/Constructor preference -- when the only ambiguity is a Class and its own Constructor, the Class wins silently - Scores each candidate (pure TS, no extra DB round-trip): base 0.50, +0.40 for file_path match, +0.20 for kind match, plus a small kind-priority tiebreaker (Class > Interface > Function > Method > Constructor) when no explicit kind hint is given - Sorts desc by score with stable tiebreakers (shorter filePath, then lex uid) - Promotes to a single confident resolve when the top score is >= 0.95 AND beats the runner-up by >= 0.10 -- lets a strong hint cut through without forcing the caller through a disambiguation round-trip * Rewire `context()` to use the shared helper. Response shape is a strict superset of today's: candidates gain a `score` field, the existing `{ uid, name, kind, filePath, line }` keys are preserved so every downstream consumer (rename, eval-server formatter, etc.) keeps working. New `kind` input hint accepted. * Rewire `impact()` to use the shared helper. Now emits the same `{ status: 'ambiguous', candidates, impactedCount: 0, risk: 'UNKNOWN' }` shape instead of silent first-pick. New inputs accepted: `target_uid`, `file_path`, `kind`. * Update tool schemas in mcp/tools.ts to advertise the new inputs and describe ranked disambiguation. Backward compatibility: The #480 Class/Constructor collapse is preserved and covered by the existing java-class-impact integration test (still green). The ambiguous response shape is a strict superset -- `eval-formatters` unit test that parses the old shape is unchanged and still passes. `impact` going from silent-first-pick to structured ambiguous is a semantic improvement that is the entire point of the issue; callers relying on silent first-pick now get an actionable response. Scope declined for v1: module/community hint -- the issue lists it as one of several hints, but kind + file_path cover the vast majority of disambiguation needs in practice, and a community-label filter requires an extra graph query per candidate. Natural v2 follow-up. Tests: calltool-dispatch.test.ts gains 5 new cases covering file_path boost, kind hint boost, impact ambiguous shape, impact target_uid short-circuit, and score field presence on the existing ambiguous test. Plus the extended assertions on the existing `context tool returns disambiguation for multiple matches`. Verification: npx vitest run test/unit/calltool-dispatch.test.ts -> 64 pass npx vitest run test/integration/java-class-impact.test.ts -> pass npm run test:unit -> 3642 pass (4 pre-existing env failures unchanged: skip-git-cli needs built dist/, git-utils tmpdir on Windows worktree -- same on main) npx tsc --noEmit -> clean Closes #470 * fix(mcp): enrich labels from UNION when labels(n)[0] is empty; address review findings CI on PR #888 caught 13 integration-test failures I did not cover locally: my resolver refactor collected candidates via `labels(n)[0] AS type`, but LadybugDB returns an empty string for that projection on certain node types (most importantly Class). With an empty `type`, impact's downstream `_runImpactBFS` no longer recognised `symType === 'Class' | 'Interface'` and stopped seeding Constructor + File nodes into the frontier, so the "impact(upstream) surfaces the file importer" assertion broke across 11 language fixtures plus 2 OVERRIDES filter tests. The original impact resolver worked around this by running a prioritised UNION across Class/Interface/Function/Method/Constructor and picking the first hit. My refactor dropped that. Fix: keep the simple candidate MATCH but enrich types afterward via a single scoped UNION query, so every candidate carries an accurate label for both scoring and downstream BFS seeding. The UID direct-lookup path is patched the same way. Also addresses the findings from the senior reviewer on PR #888: * MIGRATION.md: document the `impact` behavioural change (silent first- pick → structured `{ status: 'ambiguous', candidates }`) so downstream callers know to branch on `result.status` before reading byDepth/ summary. `context` is unchanged shape-wise (strict superset). * New test: `context tool promotes top candidate via scoring when multiple rows survive DB pre-filter`. The review flagged that the existing file_path test works only because the mock ignores WHERE parameters -- the scored-promotion path (top ≥ 0.95 AND gap > 0.09) wasn't directly exercised. The new test uses two candidates both in App.tsx-containing paths plus a kind hint so promotion is decided by scoring, not DB pre-filtering. Also tightened the comment on the earlier file_path test to describe the mock vs production divergence honestly. * NIT: added a paragraph explaining why `scored.length >= 2` is kept as a defensive guard even though the `normalized.length === 1` early return already covers the single-candidate path. * Integration: two tests in `local-backend-calltool.test.ts` targeted `'authenticate'`, which now correctly resolves as ambiguous (two Method nodes: AuthService.authenticate and BaseService.authenticate). Updated both to pass `file_path: 'src/auth.ts'` so they exercise the new disambiguation API and still assert the METHOD_OVERRIDES filtering they were originally about. Edge case fix in the promotion gap check: IEEE754 makes 0.50 + 0.40 + 0.20 - 0.90 = 0.09999999999999998 instead of exactly 0.10, which would otherwise break the "winner clearly dominates" intent for legitimate 1.00 vs 0.90 cases. Changed `>= 0.10` to `> 0.09`; same user-facing intent, no floating-point sensitivity. Verification (all from gitnexus/): npx vitest run test/integration/class-impact-all-languages.test.ts -> 52 pass (was 11 FAIL on CI before this fix) npx vitest run test/integration/local-backend-calltool.test.ts -> 18 pass (was 2 FAIL on CI before this fix) npx vitest run test/integration/java-class-impact.test.ts -> 10 pass (regression guard for #480 preserved) npx vitest run test/unit/calltool-dispatch.test.ts -> 65 pass (1 new test + 4 from original #470 PR) npm run test:unit -> 3626 pass, 4 pre-existing env failures unchanged npx tsc --noEmit -> clean --- MIGRATION.md | 44 ++ gitnexus/src/mcp/local/local-backend.ts | 512 ++++++++++++------ gitnexus/src/mcp/tools.ts | 23 +- .../local-backend-calltool.test.ts | 11 + gitnexus/test/unit/calltool-dispatch.test.ts | 209 +++++++ 5 files changed, 646 insertions(+), 153 deletions(-) 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 () => {