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 efc9c278f..4b2f385b2 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -1070,6 +1070,55 @@ 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. * @@ -1171,19 +1220,20 @@ export class LocalBackend { ); if (rows.length === 0) return { kind: 'not_found' }; const r = rows[0] as any; - return { - kind: 'ok', - symbol: { - id: r.id ?? r[0], - name: r.name ?? r[1], - type: r.type ?? r[2] ?? '', - filePath: r.filePath ?? r[3], - startLine: r.startLine ?? r[4], - endLine: r.endLine ?? r[5], - ...(include_content ? { content: r.content ?? r[6] } : {}), - }, - resolvedLabel: '', + 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' }; @@ -1221,12 +1271,22 @@ export class LocalBackend { ...(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 — we still fall back to that check when type is - // blank on at least one candidate. + // 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) { @@ -1273,10 +1333,24 @@ export class LocalBackend { return String(a.id).localeCompare(String(b.id)); }); - // Confident single-result: top score ≥ 0.95 AND beats runner-up by ≥ 0.10. - // This lets a very strong file_path/kind hint resolve cleanly instead of - // forcing the caller through a disambiguation round-trip. - if (scored.length >= 2 && scored[0].score >= 0.95 && scored[0].score - scored[1].score >= 0.1) { + // 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 }; } 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 1ea5269e9..9a90b7030 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -282,13 +282,56 @@ describe('LocalBackend.callTool', () => { name: 'handleConnect', file_path: 'App.tsx', }); - // Single confident match expected (App.tsx hit gets 0.50 base + 0.40 - // file_path bonus + 0.06 Function priority = 0.96 ≥ 0.95 threshold and - // beats the other candidate by > 0.10). + // 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([ {