From aed046e5c6e83d5073df2e2f5b450e1143305c22 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Tue, 8 Sep 2026 19:40:48 -0500 Subject: [PATCH] fix: stop silent fuzzy symbol substitution (#1473) (#1809) Adapt @uvmplus's PR #1481 (3ecf7479) to the shared symbol lookup and named-symbol flow resolver on current main. Missing names return not found with suggestions, and exact matches with no callers stay empty. Preserve #1512 definition grouping and --file narrowing, #173 qualified misses, and codegraph_node's intentional fuzzy file lookup. Port the upstream regression suite and cover the moved shared lookup paths. Validation on Linux with Node 22: project build, 74 requested tests, 47 related flow tests, and 16 same-fixture CLI/MCP checks pass. Baseline captured 12 failing tests and 13 failing fixture checks. Fixes #1473. Supersedes #1481. Co-authored-by: Colby McHenry --- CHANGELOG.md | 1 + __tests__/no-silent-fuzzy-symbol.test.ts | 217 +++++++++++++++++++++++ __tests__/symbol-lookup.test.ts | 31 ++++ src/bin/codegraph.ts | 11 +- src/graph/named-symbol-flow.ts | 61 ++++--- src/graph/symbol-lookup.ts | 11 +- src/mcp/server-instructions.ts | 1 + src/mcp/tools.ts | 6 +- 8 files changed, 306 insertions(+), 33 deletions(-) create mode 100644 __tests__/no-silent-fuzzy-symbol.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 814f9e1..ffe37a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -136,6 +136,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes - `codegraph callers`, `codegraph callees` and `codegraph impact` now resolve qualified names, group results and JSON edges by definition, and accept `--file` to narrow ambiguous names; thanks @ferrine. (#1512, #1656) +- `codegraph callers`, `codegraph callees` and `codegraph impact` (CLI and MCP) now report missing names with did-you-mean suggestions instead of another symbol's results, and exact matches with no callers stay empty; thanks @uvmplus. (#1473, #1481) #### MCP / indexing diff --git a/__tests__/no-silent-fuzzy-symbol.test.ts b/__tests__/no-silent-fuzzy-symbol.test.ts new file mode 100644 index 0000000..80cc775 --- /dev/null +++ b/__tests__/no-silent-fuzzy-symbol.test.ts @@ -0,0 +1,217 @@ +/** + * #1473 — callers/callees/impact must not silently answer for a different + * symbol when the requested name has no exact match (or has an exact match + * with zero callers). Fuzzy FTS hits may appear only as did-you-mean hints. + */ + +import { describe, it, expect, beforeAll, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { initGrammars, loadAllGrammars } from '../src/extraction/grammars'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + +beforeAll(async () => { + await initGrammars(); + await loadAllGrammars(); +}); + +function hasSqliteBindings(): boolean { + try { + const { DatabaseSync } = require('node:sqlite'); + const db = new DatabaseSync(':memory:'); + db.close(); + return true; + } catch { + return false; + } +} +const HAS_SQLITE = hasSqliteBindings(); + +function tmpRoot(prefix: string): string { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function rmTree(dir: string): void { + if (fs.existsSync(dir)) fs.rmSync(dir, { recursive: true, force: true }); +} + +function runCli(args: string[], cwd: string): { stdout: string; status: number } { + try { + const stdout = execFileSync(process.execPath, [BIN, ...args], { + cwd, + encoding: 'utf-8', + env: { + ...process.env, + CODEGRAPH_NO_DAEMON: '1', + CODEGRAPH_TELEMETRY: '0', + }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { stdout, status: 0 }; + } catch (err: unknown) { + const e = err as { stdout?: string; status?: number }; + return { stdout: e.stdout ?? '', status: typeof e.status === 'number' ? e.status : 1 }; + } +} + +describe.skipIf(!HAS_SQLITE)('no silent fuzzy substitution (#1473) — MCP', () => { + let projectRoot: string; + let cg: any; + let handler: any; + + beforeEach(async () => { + projectRoot = tmpRoot('codegraph-1473-mcp-'); + const src = path.join(projectRoot, 'src', 'a', 'b', 'c'); + fs.mkdirSync(src, { recursive: true }); + fs.writeFileSync( + path.join(src, 'D.java'), + `package a.b.c;\n\npublic class D {\n public void e() { System.out.println("e"); }\n public void ef() { System.out.println("ef"); }\n}\n` + ); + fs.writeFileSync( + path.join(src, 'Caller.java'), + `package a.b.c;\n\npublic class Caller {\n public void callsEfOnly() { D d = new D(); d.ef(); }\n public void alsoCallsEf() { D d = new D(); d.ef(); }\n}\n` + ); + // Case-differing pair: exact Fetch has 0 callers; lowercase fetch has callers. + fs.writeFileSync( + path.join(projectRoot, 'src', 'Fetch.cs'), + `public class Torture {\n public void Fetch() {}\n}\n` + ); + fs.writeFileSync( + path.join(projectRoot, 'src', 'fetch.py'), + `def fetch():\n return 1\n\ndef load():\n return fetch()\n` + ); + + const CodeGraph = (await import('../src/index')).default; + const { ToolHandler } = await import('../src/mcp/tools'); + cg = CodeGraph.initSync(projectRoot); + await cg.indexAll(); + handler = new ToolHandler(cg); + }); + + afterEach(() => { + handler?.closeAll(); + cg?.destroy(); + rmTree(projectRoot); + }); + + async function text(tool: string, args: Record): Promise { + const res = await handler.execute(tool, args); + return res.content?.[0]?.text ?? ''; + } + + it('callers: missing name is not found (with did-you-mean), not a fuzzy hit labelled as the typed name', async () => { + const out = await text('codegraph_callers', { symbol: 'Calls' }); + expect(out).toMatch(/Symbol "Calls" not found/); + expect(out).toMatch(/Did you mean:/); + expect(out).not.toMatch(/Callees of Calls|Callers of Calls/); + expect(out).not.toMatch(/\bef\b/); + }); + + it('callees: missing name does not return another method\'s callees', async () => { + const out = await text('codegraph_callees', { symbol: 'Calls' }); + expect(out).toMatch(/Symbol "Calls" not found/); + expect(out).not.toContain('Callees of Calls'); + }); + + it('impact: missing prefix does not substitute a longer name', async () => { + const out = await text('codegraph_impact', { symbol: 'callsEf' }); + expect(out).toMatch(/Symbol "callsEf" not found/); + expect(out).toMatch(/Did you mean:.*callsEfOnly/); + // Suggestion only — must not claim impact results for the mistyped name. + expect(out).not.toMatch(/Impact:|"callsEf" affects|affected/); + }); + + it('callers: exact name with zero callers stays empty (no case-sibling substitution)', async () => { + const out = await text('codegraph_callers', { symbol: 'Fetch' }); + expect(out).toMatch(/No callers found for "Fetch"/); + expect(out).not.toContain('load'); + }); + + it('callers: real exact name still resolves', async () => { + const out = await text('codegraph_callers', { symbol: 'ef' }); + expect(out).toContain('Callers of ef'); + expect(out).toContain('callsEfOnly'); + expect(out).toContain('alsoCallsEf'); + }); + + it('findAllSymbols returns no nodes for a fuzzy-only hit', async () => { + const findAllSymbols = (handler as any).findAllSymbols.bind(handler); + const all = findAllSymbols(cg, 'Calls'); + expect(all.nodes).toEqual([]); + expect(all.note).toMatch(/Did you mean:/); + }); +}); + +describe.skipIf(!HAS_SQLITE || !fs.existsSync(BIN))('no silent fuzzy substitution (#1473) — CLI', () => { + let projectRoot: string; + + beforeEach(async () => { + projectRoot = tmpRoot('codegraph-1473-cli-'); + const src = path.join(projectRoot, 'src', 'a', 'b', 'c'); + fs.mkdirSync(src, { recursive: true }); + fs.writeFileSync( + path.join(src, 'D.java'), + `package a.b.c;\n\npublic class D {\n public void e() { System.out.println("e"); }\n public void ef() { System.out.println("ef"); }\n}\n` + ); + fs.writeFileSync( + path.join(src, 'Caller.java'), + `package a.b.c;\n\npublic class Caller {\n public void callsEfOnly() { D d = new D(); d.ef(); }\n public void alsoCallsEf() { D d = new D(); d.ef(); }\n}\n` + ); + fs.writeFileSync( + path.join(projectRoot, 'src', 'Fetch.cs'), + `public class Torture {\n public void Fetch() {}\n}\n` + ); + fs.writeFileSync( + path.join(projectRoot, 'src', 'fetch.py'), + `def fetch():\n return 1\n\ndef load():\n return fetch()\n` + ); + + const CodeGraph = (await import('../src/index')).default; + const cg = CodeGraph.initSync(projectRoot); + await cg.indexAll(); + cg.close(); + }); + + afterEach(() => { + rmTree(projectRoot); + }); + + it('callers: fuzzy-only name → not found with did-you-mean (JSON stays empty)', () => { + const { stdout } = runCli(['callers', 'Calls', '--json'], projectRoot); + // JSON path is only taken on a successful resolve; not-found prints info text. + expect(stdout).toMatch(/Symbol "Calls" not found/); + expect(stdout).toMatch(/did you mean/i); + expect(stdout).not.toMatch(/"name":\s*"ef"/); + }); + + it('callees: fuzzy-only name → not found', () => { + const { stdout } = runCli(['callees', 'Calls', '--json'], projectRoot); + expect(stdout).toMatch(/Symbol "Calls" not found/); + expect(stdout).not.toMatch(/"name":\s*"ef"/); + }); + + it('impact: prefix of a real name → not found', () => { + const { stdout } = runCli(['impact', 'callsEf', '--json'], projectRoot); + expect(stdout).toMatch(/Symbol "callsEf" not found/); + expect(stdout).toMatch(/did you mean:.*callsEfOnly/i); + // Not a successful JSON impact payload for the fuzzy hit. + expect(stdout).not.toMatch(/"affected"\s*:/); + expect(stdout).not.toMatch(/"symbol":\s*"callsEf"/); + }); + + it('callers: exact Fetch with zero callers → empty list, not fetch\'s callers', () => { + const { stdout } = runCli(['callers', 'Fetch', '--json'], projectRoot); + expect(stdout).toContain('"symbol": "Fetch"'); + expect(stdout).toMatch(/"callers":\s*\[\s*\]/); + expect(stdout).not.toContain('load'); + }); + + it('callers: exact ef still lists real callers', () => { + const { stdout } = runCli(['callers', 'ef', '--json'], projectRoot); + expect(stdout).toContain('callsEfOnly'); + expect(stdout).toContain('alsoCallsEf'); + }); +}); diff --git a/__tests__/symbol-lookup.test.ts b/__tests__/symbol-lookup.test.ts index e78b12e..9074336 100644 --- a/__tests__/symbol-lookup.test.ts +++ b/__tests__/symbol-lookup.test.ts @@ -157,6 +157,26 @@ describe.skipIf(!HAS_SQLITE)('matchesSymbol — module-qualified lookups (#173)' expect(matches.length).toBe(0); }); + it('findAllSymbols rejects a fuzzy-only bare prefix with a suggestion (#1473)', () => { + expect(cg.getNodesByName('run_due')).toEqual([]); + expect(cg.searchNodes('run_due').length).toBeGreaterThan(0); + const all = findAllSymbols(cg, 'run_due'); + expect(all.nodes).toEqual([]); + expect(all.note).toMatch(/Did you mean:.*run_due_tasks/); + }); + + it('findAllSymbols rejects an unknown qualifier even when the bare tail exists (#173)', () => { + expect(cg.getNodesByName('run').length).toBeGreaterThan(0); + expect(findAllSymbols(cg, 'missing::run').nodes).toEqual([]); + }); + + it('preserves codegraph_node file-basename lookup (#1473)', () => { + expect(cg.getNodesByName('stage_apply')).toEqual([]); + const matches = findSymbolMatches(cg, 'stage_apply'); + expect(matches.length).toBeGreaterThan(0); + expect(matches[0]!.filePath).toMatch(/configurator\/stage_apply\.rs$/); + }); + it('codegraph_node with a `file` hint pins an overloaded name to that file', async () => { // `run` is defined in BOTH stage_apply.rs and stage_detect.rs. A bare lookup // returns both; the `file` hint narrows to the one the caller saw in a trail. @@ -391,4 +411,15 @@ describe.skipIf(!HAS_SQLITE)('lookupSymbolNodes — the shared path used by call const { nodes } = lookupSymbolNodes(cg, 'chart.nonexistent_fn'); expect(nodes.length).toBe(0); }); + + it.each(['grou', 'Group'])('rejects fuzzy-only bare name "%s" (#1473)', (symbol) => { + expect(cg.getNodesByName(symbol)).toEqual([]); + expect(cg.searchNodes(symbol).length).toBeGreaterThan(0); + expect(lookupSymbolNodes(cg, symbol)).toEqual({ nodes: [], ambiguous: false }); + }); + + it('rejects an unknown qualifier even when the bare tail exists (#173)', () => { + expect(cg.getNodesByName('group').length).toBeGreaterThan(0); + expect(lookupSymbolNodes(cg, 'missing.group')).toEqual({ nodes: [], ambiguous: false }); + }); }); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index d52f35a..a6fe54c 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -365,6 +365,13 @@ function warn(message: string): void { console.log(chalk.yellow(getGlyphs().warn) + ' ' + message); } +/** "not found" (+ optional did-you-mean) when no exact symbol matches. */ +function formatSymbolNotFound(symbol: string, fuzzyNames: string[]): string { + const suggestions = [...new Set(fuzzyNames.filter((n) => n !== symbol))].slice(0, 3); + if (suggestions.length === 0) return `Symbol "${symbol}" not found`; + return `Symbol "${symbol}" not found — did you mean: ${suggestions.join(', ')}?`; +} + /** Compact node shape retained by the CLI's existing JSON lists. */ function cliNode(node: Node) { return { name: node.name, kind: node.kind, filePath: node.filePath, startLine: node.startLine }; @@ -2212,7 +2219,7 @@ for (const direction of ['callers', 'callees'] as const) { const limit = parseInt(options.limit || '20', 10); const { nodes: targets } = lookupSymbolNodes(cg, symbol); if (targets.length === 0) { - info(`Symbol "${symbol}" not found`); + info(formatSymbolNotFound(symbol, cg.searchNodes(symbol, { limit: 5 }).map((m) => m.node.name))); return; } @@ -2330,7 +2337,7 @@ program const depth = Math.min(Math.max(parseInt(options.depth || '2', 10), 1), 10); const { nodes: targets } = lookupSymbolNodes(cg, symbol); if (targets.length === 0) { - info(`Symbol "${symbol}" not found`); + info(formatSymbolNotFound(symbol, cg.searchNodes(symbol, { limit: 5 }).map((m) => m.node.name))); return; } diff --git a/src/graph/named-symbol-flow.ts b/src/graph/named-symbol-flow.ts index 484c3dc..a83edf2 100644 --- a/src/graph/named-symbol-flow.ts +++ b/src/graph/named-symbol-flow.ts @@ -44,6 +44,10 @@ export { RUST_PATH_PREFIXES, lastQualifierPart, matchesSymbol } from './symbol-l /** * Find ALL symbols matching a name. Used by callers/callees/impact to aggregate * results across all matching symbols (e.g., multiple classes with an `execute` method). + * + * Exact matches only (#1473): a missing / mistyped name must NOT silently + * resolve to the top fuzzy FTS hit under the caller's typed label. Closest + * hits may appear in `note` as a did-you-mean hint when `nodes` is empty. */ export function findAllSymbols(cg: CodeGraph, symbol: string): { nodes: Node[]; note: string } { // Nix option paths: the declaration is stored as `options.` and @@ -66,42 +70,57 @@ export function findAllSymbols(cg: CodeGraph, symbol: string): { nodes: Node[]; return { nodes, note: '' }; } } - let results = cg.searchNodes(symbol, { limit: 50 }); - // Mirror the fallback in `findSymbol` for qualified queries — FTS - // strips colons, so a module-qualified lookup needs a second pass - // by the bare last part. - if (results.length === 0 && /[.\/]|::/.test(symbol)) { - const tail = lastQualifierPart(symbol); - if (tail && tail !== symbol) results = cg.searchNodes(tail, { limit: 50 }); + const isQualified = /[.\/]|::/.test(symbol); + let exactNodes: Node[]; + + if (!isQualified) { + // Direct index — every exact-name overload, case-sensitive. Avoids FTS + // ranking a differently-cased sibling above the real node (#1473 Fetch). + exactNodes = cg.getNodesByName(symbol); + } else { + let results = cg.searchNodes(symbol, { limit: 50 }); + // Mirror findSymbolMatches — FTS strips colons, so re-search by bare tail. + if (results.length === 0) { + const tail = lastQualifierPart(symbol); + if (tail && tail !== symbol) results = cg.searchNodes(tail, { limit: 50 }); + } + exactNodes = results + .filter((r) => matchesSymbol(r.node, symbol)) + .map((r) => r.node); } - if (results.length === 0) { - return { nodes: [], note: '' }; + if (exactNodes.length === 0) { + const fuzzy = cg.searchNodes(symbol, { limit: 5 }); + const suggestions = [ + ...new Set(fuzzy.map((r) => r.node.name).filter((n) => n !== symbol)), + ].slice(0, 3); + const note = + suggestions.length > 0 + ? `\n\n> **Note:** no symbol named "${symbol}". Did you mean: ${suggestions.join(', ')}?` + : ''; + return { nodes: [], note }; } - const exactMatches = results.filter(r => matchesSymbol(r.node, symbol)); - - if (exactMatches.length <= 1) { - const node = exactMatches[0]?.node ?? results[0]!.node; - return { nodes: [node], note: '' }; + if (exactNodes.length === 1) { + return { nodes: exactNodes, note: '' }; } // Same generated-file down-rank as findSymbol — keeps callers/callees // /impact aggregation aligned (a query against "Send" returns the // hand-written implementations before the protobuf scaffold). - const isGen = cg.generatedFilePredicate(exactMatches.map((r) => r.node.filePath)); - const ranked = [...exactMatches].sort((a, b) => { - const aGen = isGen(a.node.filePath) ? 1 : 0; - const bGen = isGen(b.node.filePath) ? 1 : 0; + const isGen = cg.generatedFilePredicate(exactNodes.map((n) => n.filePath)); + const ranked = [...exactNodes].sort((a, b) => { + const aGen = isGen(a.filePath) ? 1 : 0; + const bGen = isGen(b.filePath) ? 1 : 0; return aGen - bGen; }); - const locations = ranked.map(r => - `${r.node.kind} at ${r.node.filePath}:${r.node.startLine}` + const locations = ranked.map( + (n) => `${n.kind} at ${n.filePath}:${n.startLine}` ); const note = `\n\n> **Note:** Aggregated results across ${ranked.length} symbols named "${symbol}": ${locations.join(', ')}`; - return { nodes: ranked.map(r => r.node), note }; + return { nodes: ranked, note }; } /** Node kinds that can sit on a call chain. */ diff --git a/src/graph/symbol-lookup.ts b/src/graph/symbol-lookup.ts index fc16286..2e7bae2 100644 --- a/src/graph/symbol-lookup.ts +++ b/src/graph/symbol-lookup.ts @@ -167,8 +167,8 @@ export function groupDefinitions( * The exact-name index is consulted FIRST and is authoritative: it is complete * and uncapped, whereas FTS ranks and truncates, and tokenises away `::` — so * a qualified query could miss a symbol that exists, or land on whatever - * happened to rank first. FTS remains as the fallback for the fuzzy cases it is - * genuinely good at (file basenames, partial names). + * happened to rank first. FTS candidates still have to satisfy the matcher; + * partial or mistyped names must never select the top fuzzy hit (#1473). */ export function lookupSymbolNodes(cg: SymbolLookupHost, symbol: string): SymbolLookupResult { const qualified = isQualifiedSymbol(symbol); @@ -183,12 +183,9 @@ export function lookupSymbolNodes(cg: SymbolLookupHost, symbol: string): SymbolL const exact = hits.filter((n) => matchesSymbol(n, symbol)); if (exact.length > 0) { nodes = exact; - } else if (!qualified && hits[0]) { - // A bare name with no exact definition may still mean a file basename. - nodes = [hits[0]]; } - // A qualified query with no exact match resolves to NOTHING rather than a - // misleading fuzzy hit (#173). + // Any query with no exact match resolves to NOTHING rather than a + // misleading fuzzy hit (#1473; qualified lookups already did this in #173). } if (nodes.length === 0) return { nodes: [], ambiguous: false }; diff --git a/src/mcp/server-instructions.ts b/src/mcp/server-instructions.ts index f7e6e02..4cb2215 100644 --- a/src/mcp/server-instructions.ts +++ b/src/mcp/server-instructions.ts @@ -56,6 +56,7 @@ calls; a grep/read exploration is dozens. - **Reading or editing a file/symbol you can name** → put its name or file path in the \`codegraph_explore\` query — it returns that current line-numbered source (safe to \`Edit\` from) with the call path and blast radius attached, so you don't Read it separately. For an overloaded name it returns every matching definition's body in one call. - **Need more?** Call \`codegraph_explore\` again with more specific names — treat the source it returns as already Read. Suggested call counts are advisory only, NOT a quota; extra calls are never rejected or rate-limited. - Qualified symbol names accept dots, \`::\`, or slashes, including containers whose names contain dots (for example, \`AppWeb.Format.group\`). +- Named-symbol call paths require exact matches; partial or mistyped names are never silently substituted as flow endpoints. If a graph query reports a missing symbol with did-you-mean suggestions, query the suggested name explicitly. ## Anti-patterns diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index dd57d8a..42fdd22 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -2375,7 +2375,7 @@ export class ToolHandler { const allMatches = this.findAllSymbols(cg, symbol); if (allMatches.nodes.length === 0) { - return this.textResult(`Symbol "${symbol}" not found in the codebase`); + return this.textResult(`Symbol "${symbol}" not found in the codebase${allMatches.note}`); } const { groups, filteredOut } = this.groupDefinitions(allMatches.nodes, fileFilter); @@ -2456,7 +2456,7 @@ export class ToolHandler { const allMatches = this.findAllSymbols(cg, symbol); if (allMatches.nodes.length === 0) { - return this.textResult(`Symbol "${symbol}" not found in the codebase`); + return this.textResult(`Symbol "${symbol}" not found in the codebase${allMatches.note}`); } const { groups, filteredOut } = this.groupDefinitions(allMatches.nodes, fileFilter); @@ -2534,7 +2534,7 @@ export class ToolHandler { const allMatches = this.findAllSymbols(cg, symbol); if (allMatches.nodes.length === 0) { - return this.textResult(`Symbol "${symbol}" not found in the codebase`); + return this.textResult(`Symbol "${symbol}" not found in the codebase${allMatches.note}`); } const { groups, filteredOut } = this.groupDefinitions(allMatches.nodes, fileFilter);