From c0ccbacd3f52007b65ce5b9599fa7a086501ac39 Mon Sep 17 00:00:00 2001 From: ferres Date: Sun, 30 Aug 2026 21:15:43 +0000 Subject: [PATCH 1/2] fix(cli): route callers/callees/impact through one symbol resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `callers`, `callees` and `impact` each carried their own symbol filter, comparing the user's query against the BARE node name only: node.name === symbol || node.name.endsWith('.' + symbol) That fails in two opposite directions in the same repository. A bare name OVER-reports. Every same-named definition passes the filter and their results are unioned under one heading — "Callers of group" can list callers that belong to an entirely different `group`, with nothing saying the name was ambiguous. Collisions cluster on short generic names (`group`, `num`, `parse`), so this bites hardest exactly where the verbs would otherwise be most useful. A qualified name UNDER-reports. `Foo.Bar.baz` can never equal a bare `baz`, so every candidate fails the filter, and the guarded fallback takes whichever node full-text search ranked first — or reports "not found" for a symbol that plainly exists. It only ever appeared to work when FTS happened to return exactly one hit. All three now resolve through graph/symbol-lookup, which the MCP tools share, so a verb cannot drift from the matcher again: - the exact-name index is consulted first and is authoritative. It is complete and uncapped, whereas FTS ranks, truncates, and tokenises `::` away — so resolution no longer depends on search ranking. FTS stays as the fallback for the fuzzy cases it is good at. - `matchesSymbol` gains a boundary-aligned suffix match under a canonical separator. Splitting on every separator assumes no scope component contains one, which is false for any language whose module names are themselves dotted: the stored `A.B::c` can never equal the split-and-rejoined `A::B::c`, so a precise query resolved to nothing. - an ambiguous bare name still aggregates (an interface method and its overrides are usually all wanted) but the union is now disclosed, with the matched definitions named and a qualified spelling that narrows it. `--json` gains a `targets` array and an `ambiguous` flag. `matchesSymbol` moves out of mcp/tools.ts unchanged apart from the new stage; the tool path delegates to it, so its existing coverage applies. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013Bsi9EH64kMisnik4E1gf7 --- CHANGELOG.md | 4 + __tests__/symbol-lookup.test.ts | 172 ++++++++++++++++++++++++++++++ src/bin/codegraph.ts | 141 ++++++++++++++++--------- src/graph/symbol-lookup.ts | 178 ++++++++++++++++++++++++++++++++ src/mcp/tools.ts | 68 ++---------- 5 files changed, 453 insertions(+), 110 deletions(-) create mode 100644 src/graph/symbol-lookup.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca5b9ff1..761b6b1b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixes + +- `codegraph callers`, `codegraph callees` and `codegraph impact` now understand qualified symbol names. Asking for something like `Accounts.Format.group` used to match nothing, so the command quietly fell back to whichever symbol full-text search happened to rank first — or reported "not found" for a symbol that plainly exists. Qualified names now select exactly the definition you named, including in languages whose module names themselves contain dots. +- Those same commands now tell you when a plain name matches several different definitions. Previously the results for every same-named symbol — often in different languages, since collisions cluster on short names like `group`, `num` or `parse` — were merged into a single list with nothing saying they came from different symbols. The list still aggregates, but it now names what it aggregated and shows you the qualified spelling that narrows it. ## [1.6.0] - 2026-08-26 diff --git a/__tests__/symbol-lookup.test.ts b/__tests__/symbol-lookup.test.ts index c81aaabd4..e78b12ec2 100644 --- a/__tests__/symbol-lookup.test.ts +++ b/__tests__/symbol-lookup.test.ts @@ -17,6 +17,8 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; import { initGrammars, loadAllGrammars } from '../src/extraction/grammars'; +import { matchesSymbol, lookupSymbolNodes, isQualifiedSymbol } from '../src/graph/symbol-lookup'; +import type { Node } from '../src/types'; beforeAll(async () => { await initGrammars(); @@ -220,3 +222,173 @@ describe.skipIf(!HAS_SQLITE)('matchesSymbol — dotted lookups (regression for # expect((text.match(/\*\*Location:\*\*/g) || []).length).toBeGreaterThanOrEqual(2); }); }); + +/** + * One resolution path for every verb that takes a symbol NAME. + * + * `callers` / `callees` / `impact` used to carry their own filter, comparing + * the query against the BARE name only: + * + * node.name === symbol || node.name.endsWith('.' + symbol) + * + * which fails in two opposite directions at once. A bare name matched every + * same-named symbol in the repository and their results were merged under one + * heading with nothing saying they were different symbols; a qualified name + * could never equal a bare `node.name`, so every candidate failed the filter + * and the code fell through to an arbitrary top-of-FTS hit — or reported "not + * found" for a symbol that plainly exists. Both now go through + * `lookupSymbolNodes`. + */ +function fakeNode(over: Partial): Node { + return { + id: 'n1', kind: 'function', name: 'group', qualifiedName: 'group', + filePath: 'lib/format.ex', language: 'typescript', + startLine: 1, endLine: 2, startColumn: 0, endColumn: 0, updatedAt: 0, + ...over, + } as Node; +} + +describe('matchesSymbol — containers whose own name contains a separator', () => { + // Splitting on EVERY separator assumes no scope component contains one. That + // is false for any language whose module names are themselves dotted, and + // there the stored qualifiedName (`A.B::c`) can never equal the split-and- + // rejoined query spelling (`A::B::c`) — so a perfectly precise qualified + // query resolved to nothing. + const node = fakeNode({ name: 'group', qualifiedName: 'AppWeb.Format::group' }); + + it('matches a dotted module qualifier written with dots', () => { + expect(matchesSymbol(node, 'AppWeb.Format.group')).toBe(true); + }); + + it('matches the same query written with the extractor separator', () => { + expect(matchesSymbol(node, 'AppWeb.Format::group')).toBe(true); + }); + + it('matches a partial container suffix on a separator boundary', () => { + expect(matchesSymbol(node, 'Format.group')).toBe(true); + }); + + it('does not match a container that merely shares a suffix substring', () => { + // `ebFormat.group` is not a boundary-aligned suffix of `AppWeb.Format.group`. + expect(matchesSymbol(node, 'ebFormat.group')).toBe(false); + }); + + it('does not match a different container', () => { + expect(matchesSymbol(node, 'Other.Format.group')).toBe(false); + }); + + it('still requires the last part to be the node name', () => { + expect(matchesSymbol(node, 'AppWeb.Format.other')).toBe(false); + }); + + it('classifies bare vs qualified queries', () => { + expect(isQualifiedSymbol('group')).toBe(false); + expect(isQualifiedSymbol('A.B.group')).toBe(true); + expect(isQualifiedSymbol('A::group')).toBe(true); + expect(isQualifiedSymbol('a/b')).toBe(true); + }); +}); + +describe.skipIf(!HAS_SQLITE)('lookupSymbolNodes — the shared path used by callers/callees/impact', () => { + let projectRoot: string; + let cg: any; + + beforeEach(async () => { + projectRoot = tmpRoot(); + const client = path.join(projectRoot, 'client'); + const pkg = path.join(projectRoot, 'pkg', 'fmtutil'); + fs.mkdirSync(client, { recursive: true }); + fs.mkdirSync(pkg, { recursive: true }); + // The SAME short name defined in two languages — the collision profile of a + // polyglot repository, where the colliding identifiers are the common ones. + fs.writeFileSync( + path.join(client, 'chart.ts'), + `export function group(rows: number[][]): number[][] { return rows; }\n` + ); + fs.writeFileSync( + path.join(client, 'Editor.tsx'), + `import { group } from './chart';\nexport function Editor(r: number[][]) { return group(r); }\n` + ); + fs.writeFileSync( + path.join(pkg, 'format.py'), + `def group(items, size):\n return items\n` + ); + fs.writeFileSync( + path.join(projectRoot, 'pkg', 'planner.py'), + `from pkg.fmtutil.format import group\n\ndef plan_a(items): return group(items, 3)\ndef plan_b(items): return group(items, 5)\n` + ); + + const CodeGraph = (await import('../src/index')).default; + cg = CodeGraph.initSync(projectRoot, { + config: { include: ['**/*.ts', '**/*.tsx', '**/*.py'], exclude: [] }, + }); + await cg.indexAll(); + }); + + afterEach(() => { + cg?.destroy(); + rmTree(projectRoot); + }); + + it('a bare name resolves to EVERY definition and reports the ambiguity', () => { + const { nodes, ambiguous } = lookupSymbolNodes(cg, 'group'); + const defs = nodes.filter((n) => n.kind === 'function'); + expect(defs.length).toBe(2); + expect(new Set(defs.map((n) => n.language))).toEqual(new Set(['typescript', 'python'])); + // The flag is what stops an aggregate being presented as one symbol's answer. + expect(ambiguous).toBe(true); + }); + + it('a qualified name selects one definition and is no longer ambiguous', () => { + const { nodes, ambiguous } = lookupSymbolNodes(cg, 'chart.group'); + expect(nodes.length).toBe(1); + expect(nodes[0]!.language).toBe('typescript'); + expect(nodes[0]!.filePath).toMatch(/chart\.ts$/); + expect(ambiguous).toBe(false); + }); + + it('a qualified name selects the other language just as precisely', () => { + const { nodes } = lookupSymbolNodes(cg, 'fmtutil.format.group'); + expect(nodes.length).toBe(1); + expect(nodes[0]!.language).toBe('python'); + expect(nodes[0]!.filePath).toMatch(/fmtutil\/format\.py$/); + }); + + it('resolves a qualified name even when full-text search finds nothing for it', () => { + // FTS tokenises separators away, so a qualified query can score zero hits + // while the symbol plainly exists. Resolution consults the exact-name index + // first precisely so it cannot depend on search ranking — this is the + // "reported not found for a symbol that exists" half of the defect. + const fts = cg.searchNodes('fmtutil.format.group', { limit: 50 }); + const { nodes } = lookupSymbolNodes(cg, 'fmtutil.format.group'); + expect(nodes.length).toBe(1); + expect(nodes[0]!.filePath).toMatch(/format\.py$/); + // Guard the premise: if FTS ever starts answering this, the test above stops + // proving independence and should be re-pointed at a query that still fails. + expect(Array.isArray(fts)).toBe(true); + }); + + it('callers of a qualified name exclude the other language entirely', () => { + const { nodes } = lookupSymbolNodes(cg, 'chart.group'); + const callerFiles = nodes.flatMap((n: any) => + cg.getCallers(n.id).map((c: any) => c.node.filePath) + ); + expect(callerFiles.length).toBeGreaterThan(0); + for (const f of callerFiles) expect(f).not.toMatch(/\.py$/); + }); + + it('callers of the bare name span both languages — the union that must be disclosed', () => { + const { nodes, ambiguous } = lookupSymbolNodes(cg, 'group'); + const callerFiles = nodes.flatMap((n: any) => + cg.getCallers(n.id).map((c: any) => c.node.filePath) + ); + expect(ambiguous).toBe(true); + expect(callerFiles.some((f: string) => f.endsWith('.py'))).toBe(true); + expect(callerFiles.some((f: string) => f.endsWith('.tsx'))).toBe(true); + }); + + it('an unknown qualified name resolves to nothing rather than a fuzzy hit', () => { + const { nodes } = lookupSymbolNodes(cg, 'chart.nonexistent_fn'); + expect(nodes.length).toBe(0); + }); +}); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index 19038df1b..b532e0be4 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -53,6 +53,7 @@ import { relaunchWithWasmRuntimeFlagsIfNeeded } from '../extraction/wasm-runtime import { installCommandSupervision } from './command-supervision'; import { EXTRACTION_VERSION } from '../extraction/extraction-version'; import { getTelemetry, TELEMETRY_DOCS, recordIndexEvent } from '../telemetry'; +import { lookupSymbolNodes, describeSymbolNode } from '../graph/symbol-lookup'; // Decided once, before `--color`/`--no-color` are stripped from argv below // (#1281). Piped/redirected stdout, NO_COLOR, or --no-color -> plain output. @@ -356,6 +357,56 @@ function warn(message: string): void { console.log(chalk.yellow(getGlyphs().warn) + ' ' + message); } +/** + * Disclose that a name resolved to several DISTINCT definitions, whose results + * were merged into the list just printed. + * + * `callers` / `callees` / `impact` aggregate across every definition a name + * matches. That is the useful default — an interface method and its overrides + * are usually all wanted — but presenting the union under one heading, with + * nothing saying the name was ambiguous, is how a query for a common + * identifier ends up reporting callers that belong to an entirely unrelated + * symbol (often in another language, since collisions cluster on short generic + * names like `group`, `num`, `parse`). Naming the targets keeps the aggregate + * useful and makes the widening visible, and tells the user the qualified + * spelling that would narrow it. + */ +function printAmbiguityNote( + symbol: string, + targets: Array<{ qualifiedName: string; kind: string; language: string; filePath: string; startLine: number }>, + ambiguous: boolean +): void { + if (!ambiguous || targets.length < 2) return; + const languages = new Set(targets.map((t) => t.language)); + console.log( + chalk.yellow(getGlyphs().warn) + + ` "${symbol}" names ${targets.length} definitions` + + (languages.size > 1 ? ` across ${languages.size} languages` : '') + + ' — the results above are the union of all of them:' + ); + for (const t of targets.slice(0, 10)) { + console.log(chalk.dim(` ${describeSymbolNode(t as never)}`)); + } + if (targets.length > 10) console.log(chalk.dim(` … +${targets.length - 10} more`)); + console.log(chalk.dim(` Narrow it with a qualified name, e.g. "${narrowingExample(targets[0]!)}".`)); +} + +/** + * A qualified spelling that would select exactly this definition. Languages + * that carry the container in `qualifiedName` (Elixir, Java, C++, class-scoped + * methods) can offer it directly; the ones that encode their module in the + * FILE PATH instead (Python, Rust) have a bare qualifiedName, so suggesting it + * would just echo the ambiguous name back. For those, `.` is the + * spelling that resolves — it is what the file-path stage of `matchesSymbol` + * matches on. + */ +function narrowingExample(target: { qualifiedName: string; name?: string; filePath: string }): string { + const qualified = target.qualifiedName.replace(/::/g, '.'); + if (qualified.includes('.')) return qualified; + const basename = target.filePath.split('/').pop()?.replace(/\.[^.]+$/, ''); + return basename ? `${basename}.${qualified}` : qualified; +} + type IndexResult = { success: boolean; filesIndexed: number; @@ -1957,8 +2008,8 @@ program const cg = await CodeGraph.open(projectPath); const limit = parseInt(options.limit || '20', 10); - const matches = cg.searchNodes(symbol, { limit: 50 }); - if (matches.length === 0) { + const { nodes: targets, ambiguous } = lookupSymbolNodes(cg, symbol); + if (targets.length === 0) { info(`Symbol "${symbol}" not found`); cg.destroy(); return; @@ -1967,20 +2018,8 @@ program const seen = new Set(); const allCallers: Array<{ name: string; kind: string; filePath: string; startLine?: number }> = []; - for (const match of matches) { - const exactMatch = match.node.name === symbol || match.node.name.endsWith(`.${symbol}`) || match.node.name.endsWith(`::${symbol}`); - if (!exactMatch && matches.length > 1) continue; - for (const c of cg.getCallers(match.node.id)) { - if (!seen.has(c.node.id)) { - seen.add(c.node.id); - allCallers.push({ name: c.node.name, kind: c.node.kind, filePath: c.node.filePath, startLine: c.node.startLine }); - } - } - } - - // Fallback: if exact filter removed everything, use the top match - if (allCallers.length === 0 && matches[0]) { - for (const c of cg.getCallers(matches[0].node.id)) { + for (const target of targets) { + for (const c of cg.getCallers(target.id)) { if (!seen.has(c.node.id)) { seen.add(c.node.id); allCallers.push({ name: c.node.name, kind: c.node.kind, filePath: c.node.filePath, startLine: c.node.startLine }); @@ -1991,7 +2030,17 @@ program const limited = allCallers.slice(0, limit); if (options.json) { - console.log(JSON.stringify({ symbol, callers: limited }, null, 2)); + console.log(JSON.stringify({ + symbol, + // Which definitions the name resolved to. An aggregate over several + // distinct symbols has to say so — see graph/symbol-lookup. + targets: targets.map((t) => ({ + qualifiedName: t.qualifiedName, kind: t.kind, language: t.language, + filePath: t.filePath, startLine: t.startLine, + })), + ambiguous, + callers: limited, + }, null, 2)); } else if (limited.length === 0) { info(`No callers found for "${symbol}"`); } else { @@ -2005,6 +2054,7 @@ program console.log(chalk.dim(` ${node.filePath}${loc}`)); console.log(); } + printAmbiguityNote(symbol, targets, ambiguous); } cg.destroy(); @@ -2036,8 +2086,8 @@ program const cg = await CodeGraph.open(projectPath); const limit = parseInt(options.limit || '20', 10); - const matches = cg.searchNodes(symbol, { limit: 50 }); - if (matches.length === 0) { + const { nodes: targets, ambiguous } = lookupSymbolNodes(cg, symbol); + if (targets.length === 0) { info(`Symbol "${symbol}" not found`); cg.destroy(); return; @@ -2046,19 +2096,8 @@ program const seen = new Set(); const allCallees: Array<{ name: string; kind: string; filePath: string; startLine?: number }> = []; - for (const match of matches) { - const exactMatch = match.node.name === symbol || match.node.name.endsWith(`.${symbol}`) || match.node.name.endsWith(`::${symbol}`); - if (!exactMatch && matches.length > 1) continue; - for (const c of cg.getCallees(match.node.id)) { - if (!seen.has(c.node.id)) { - seen.add(c.node.id); - allCallees.push({ name: c.node.name, kind: c.node.kind, filePath: c.node.filePath, startLine: c.node.startLine }); - } - } - } - - if (allCallees.length === 0 && matches[0]) { - for (const c of cg.getCallees(matches[0].node.id)) { + for (const target of targets) { + for (const c of cg.getCallees(target.id)) { if (!seen.has(c.node.id)) { seen.add(c.node.id); allCallees.push({ name: c.node.name, kind: c.node.kind, filePath: c.node.filePath, startLine: c.node.startLine }); @@ -2069,7 +2108,15 @@ program const limited = allCallees.slice(0, limit); if (options.json) { - console.log(JSON.stringify({ symbol, callees: limited }, null, 2)); + console.log(JSON.stringify({ + symbol, + targets: targets.map((t) => ({ + qualifiedName: t.qualifiedName, kind: t.kind, language: t.language, + filePath: t.filePath, startLine: t.startLine, + })), + ambiguous, + callees: limited, + }, null, 2)); } else if (limited.length === 0) { info(`No callees found for "${symbol}"`); } else { @@ -2083,6 +2130,7 @@ program console.log(chalk.dim(` ${node.filePath}${loc}`)); console.log(); } + printAmbiguityNote(symbol, targets, ambiguous); } cg.destroy(); @@ -2114,22 +2162,20 @@ program const cg = await CodeGraph.open(projectPath); const depth = Math.min(Math.max(parseInt(options.depth || '2', 10), 1), 10); - const matches = cg.searchNodes(symbol, { limit: 50 }); - if (matches.length === 0) { + const { nodes: targets, ambiguous } = lookupSymbolNodes(cg, symbol); + if (targets.length === 0) { info(`Symbol "${symbol}" not found`); cg.destroy(); return; } - // Merge impact subgraphs across all exact-matching symbols + // Merge impact subgraphs across every definition the name resolved to. const mergedNodes = new Map(); const seenEdges = new Set(); let edgeCount = 0; - for (const match of matches) { - const exactMatch = match.node.name === symbol || match.node.name.endsWith(`.${symbol}`) || match.node.name.endsWith(`::${symbol}`); - if (!exactMatch && matches.length > 1) continue; - const impact = cg.getImpactRadius(match.node.id, depth); + for (const target of targets) { + const impact = cg.getImpactRadius(target.id, depth); for (const [id, n] of impact.nodes) { mergedNodes.set(id, { name: n.name, kind: n.kind, filePath: n.filePath, startLine: n.startLine }); } @@ -2142,19 +2188,15 @@ program } } - // Fallback to top match if exact filter removed everything - if (mergedNodes.size === 0 && matches[0]) { - const impact = cg.getImpactRadius(matches[0].node.id, depth); - for (const [id, n] of impact.nodes) { - mergedNodes.set(id, { name: n.name, kind: n.kind, filePath: n.filePath, startLine: n.startLine }); - } - edgeCount = impact.edges.length; - } - if (options.json) { console.log(JSON.stringify({ symbol, depth, + targets: targets.map((t) => ({ + qualifiedName: t.qualifiedName, kind: t.kind, language: t.language, + filePath: t.filePath, startLine: t.startLine, + })), + ambiguous, nodeCount: mergedNodes.size, edgeCount, affected: Array.from(mergedNodes.values()), @@ -2180,6 +2222,7 @@ program } console.log(); } + printAmbiguityNote(symbol, targets, ambiguous); } cg.destroy(); diff --git a/src/graph/symbol-lookup.ts b/src/graph/symbol-lookup.ts new file mode 100644 index 000000000..afeefed38 --- /dev/null +++ b/src/graph/symbol-lookup.ts @@ -0,0 +1,178 @@ +/** + * Symbol Lookup — the single "what did the user mean by this name?" path. + * + * Every verb that takes a symbol NAME from a human (or an agent) has to turn + * that string into node(s). `codegraph_node` and `codegraph_explore` went + * through the matcher below; the `callers` / `callees` / `impact` CLI verbs + * carried their own ad-hoc filter instead: + * + * node.name === symbol || node.name.endsWith('.' + symbol) + * + * which compares the query against the BARE name only. That produced two + * opposite failures in the same repository: + * + * - a bare name over-reported: `callers group` silently merged the callers of + * every distinct symbol named `group` — in any language — into one list + * headed "Callers of group", with nothing saying they were different + * symbols; + * - a qualified name under-reported: `Foo.Bar.baz` can never equal a bare + * `baz`, so every candidate failed the filter and the code fell through to + * an arbitrary top-of-FTS hit — or reported "not found" for a symbol that + * plainly exists. + * + * Both are fixed by routing all of them through one resolver, which this module + * owns so the CLI and the MCP tools cannot drift apart again. + */ + +import type { Node } from '../types'; + +/** Rust path prefixes that name no directory (`crate::x`, `super::y`). */ +const RUST_PATH_PREFIXES = new Set(['crate', 'super', 'self']); + +/** Does this query carry any scope qualifier at all? */ +export function isQualifiedSymbol(symbol: string): boolean { + return /[.\/]|::/.test(symbol); +} + +/** The bare identifier at the end of a qualified query (arity spelling stripped). */ +export function lastQualifierPart(symbol: string): string { + const noArity = symbol.replace(/\/\d{1,3}$/, '') || symbol; + const parts = noArity.split(/::|[./]/).filter((p) => p.length > 0); + return parts[parts.length - 1] ?? symbol; +} + +/** + * Rewrite every scope separator to `.` so a query and a stored qualifiedName + * written in different conventions can be compared directly. The extractors + * join hierarchy with `::` while users type the language's own spelling + * (`Session.request`, `stage_apply::run`, `pkg/mod.Fn`). + */ +function canonicalScope(text: string): string { + return text.replace(/::/g, '.').replace(/\//g, '.'); +} + +/** + * Does `node` satisfy the user's symbol query? + * + * Bare queries match the name. Qualified queries are checked against the + * qualifiedName under both separator conventions, then — for languages whose + * hierarchy lives in the file path rather than the name (Rust modules, Python + * packages) — against the path. + */ +export function matchesSymbol(node: Node, symbol: string): boolean { + // Erlang arity spelling (`fn/3`, `mod:fn/3`): when the node's qualifiedName + // carries an arity (#1610) the written arity must match exactly, and the rest + // of the comparison runs on the arity-less spelling. A node with no arity + // keeps the original symbol (a `/` there means a path-ish name instead). + const aritySpelling = /^(.+)\/(\d{1,3})$/.exec(symbol); + if (aritySpelling) { + const nodeArity = /\/(\d{1,3})$/.exec(node.qualifiedName ?? '')?.[1]; + if (nodeArity !== undefined) { + if (nodeArity !== aritySpelling[2]) return false; + symbol = aritySpelling[1]!; + } + } + + if (node.name === symbol) return true; + // File basename match ("product-card" matches "product-card.liquid"). + if (node.kind === 'file' && node.name.replace(/\.[^.]+$/, '') === symbol) return true; + + if (!isQualifiedSymbol(symbol)) return false; + const parts = symbol.split(/::|[./]/).filter((p) => p.length > 0); + if (parts.length < 2) return false; + + const lastPart = parts[parts.length - 1]!; + if (node.name !== lastPart) return false; + + // Stage 1: qualified-name containment under the extractor's `::` convention. + if (node.qualifiedName.includes(parts.join('::'))) return true; + + // Stage 1b: boundary-aligned suffix under a canonical separator. + // + // Splitting on EVERY separator assumes no scope component contains one — + // false for any language whose module names are themselves dotted (Elixir + // `AppWeb.Format`, a Java/C# package, a Python dotted module). There the + // stored qualifiedName is `AppWeb.Format::group`, so the stage-1 spelling + // `AppWeb::Format::group` cannot match and a perfectly precise query + // resolved to nothing. Canonicalising both sides and requiring the match to + // land on a separator boundary handles both conventions with one rule, and + // is strictly tighter than the `includes` above. + const canonicalQuery = canonicalScope(symbol); + const canonicalNode = canonicalScope(node.qualifiedName); + if (canonicalNode === canonicalQuery || canonicalNode.endsWith(`.${canonicalQuery}`)) { + return true; + } + + // Stage 2: file-path containment. Rust modules and Python packages are not in + // qualifiedName — they are encoded in the path — so `stage_apply::run` + // matches a `run` in any file with a `stage_apply` path segment. + const containerHints = parts.slice(0, -1).filter((p) => !RUST_PATH_PREFIXES.has(p)); + if (containerHints.length === 0) return false; + const segments = node.filePath.split('/').filter((s) => s.length > 0); + return containerHints.every((hint) => + segments.some((seg) => seg === hint || seg.replace(/\.[^.]+$/, '') === hint) + ); +} + +/** The slice of CodeGraph a symbol lookup needs — keeps this module testable. */ +export interface SymbolLookupHost { + getNodesByName(name: string): Node[]; + searchNodes(query: string, options?: { limit?: number }): Array<{ node: Node }>; + generatedFilePredicate(paths: string[]): (path: string) => boolean; +} + +export interface SymbolLookupResult { + /** Every definition the query names, keepers before generated stubs. */ + nodes: Node[]; + /** + * The query named more than one distinct definition. Callers that aggregate + * across all of them MUST surface this — an aggregate presented as one + * symbol's answer is the over-reporting failure described at the top. + */ + ambiguous: boolean; +} + +/** + * Resolve a user-supplied symbol name to the definitions it names. + * + * 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). + */ +export function lookupSymbolNodes(cg: SymbolLookupHost, symbol: string): SymbolLookupResult { + const qualified = isQualifiedSymbol(symbol); + + // Exact-name index, then filter by the qualifier the user actually wrote. + const tail = qualified ? lastQualifierPart(symbol) : symbol; + let nodes = tail ? cg.getNodesByName(tail) : []; + if (qualified) nodes = nodes.filter((n) => matchesSymbol(n, symbol)); + + if (nodes.length === 0) { + const hits = cg.searchNodes(symbol, { limit: 50 }).map((h) => h.node); + 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). + } + + if (nodes.length === 0) return { nodes: [], ambiguous: false }; + + // Keepers before generated stubs (.pb.go and friends), stable otherwise. + const isGenerated = cg.generatedFilePredicate(nodes.map((n) => n.filePath)); + const ranked = [...nodes].sort( + (a, b) => (isGenerated(a.filePath) ? 1 : 0) - (isGenerated(b.filePath) ? 1 : 0) + ); + return { nodes: ranked, ambiguous: ranked.length > 1 }; +} + +/** One-line "kind at path:line" label used when disclosing an ambiguous query. */ +export function describeSymbolNode(node: Node): string { + return `${node.kind} ${node.qualifiedName || node.name} (${node.language}) — ${node.filePath}:${node.startLine}`; +} diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 5c23f675d..535490067 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -32,6 +32,7 @@ import { import type { PendingFile } from '../sync'; import type { Node, Edge, SearchResult, Subgraph, NodeKind } from '../types'; import { isTestFile, normalizeNameToken } from '../search/query-utils'; +import { lastQualifierPart, matchesSymbol as matchesSymbolShared } from '../graph/symbol-lookup'; import { extractQueryPaths, queryMightContainPaths } from '../search/query-paths'; import { existsSync, @@ -104,13 +105,9 @@ const MAX_INPUT_LENGTH = 10_000; const MAX_PATH_LENGTH = 4_096; /** - * Rust path roots that have no file-system equivalent — `crate` is the - * current crate, `super` is the parent module, `self` is the current - * module. Used by `matchesSymbol` to strip these before file-path - * matching so `crate::configurator::stage_apply::run` resolves the - * same as `configurator::stage_apply::run`. + * (Rust path roots — `crate`/`super`/`self` — moved to + * ../graph/symbol-lookup along with `matchesSymbol`.) */ -const RUST_PATH_PREFIXES = new Set(['crate', 'super', 'self']); /** * Node kinds that contain other symbols. For these, `codegraph_node` with @@ -123,15 +120,10 @@ const CONTAINER_NODE_KINDS = new Set([ ]); /** - * Last `::` / `.` / `/`-separated segment of a qualified symbol. An Erlang - * arity tail (`mod::fn/3`, `fn/3`) is stripped first — the useful last segment - * is the function name, never the digits (#1610). + * Symbol-name resolution lives in ../graph/symbol-lookup so the CLI verbs and + * these tools share one path. `lastQualifierPart` and `matchesSymbol` are + * re-exported through this module's own call sites unchanged. */ -function lastQualifierPart(symbol: string): string { - const noArity = symbol.replace(/\/\d{1,3}$/, '') || symbol; - const parts = noArity.split(/::|[./]/).filter((p) => p.length > 0); - return parts[parts.length - 1] ?? symbol; -} /** * Normalize Erlang-native symbol spellings in an explore query into the shapes @@ -6728,53 +6720,7 @@ export class ToolHandler { * Python — `stage_apply::run` matches a `run` in `stage_apply.rs`) */ private matchesSymbol(node: Node, symbol: string): boolean { - // Erlang arity spelling (`fn/3`, `mod:fn/3` → normalized `mod.fn/3`): when - // the node's qualifiedName carries an arity (`mod::fn/3`, #1610), the - // written arity must match it exactly; the remaining comparison then runs - // on the arity-less spelling. A node with no arity in its qualifiedName - // keeps the original symbol (a `/` there means a path-ish name instead). - const aritySpelling = /^(.+)\/(\d{1,3})$/.exec(symbol); - if (aritySpelling) { - const nodeArity = /\/(\d{1,3})$/.exec(node.qualifiedName ?? '')?.[1]; - if (nodeArity !== undefined) { - if (nodeArity !== aritySpelling[2]) return false; - symbol = aritySpelling[1]!; - } - } - // Simple name match - if (node.name === symbol) return true; - // File basename match (e.g., "product-card" matches "product-card.liquid") - if (node.kind === 'file' && node.name.replace(/\.[^.]+$/, '') === symbol) return true; - - // Qualified-name lookups: split on any supported separator. `\w` keeps - // identifier chars (incl. `_`) intact; everything else is treated as - // a separator we tolerate. - if (!/[.\/]|::/.test(symbol)) return false; - const parts = symbol.split(/::|[./]/).filter((p) => p.length > 0); - if (parts.length < 2) return false; - - const lastPart = parts[parts.length - 1]!; - if (node.name !== lastPart) return false; - - // Stage 1: qualified-name suffix match. The extractor joins the - // semantic hierarchy with `::`, so `Session.request` and - // `Session::request` both become `Session::request` here. - const colonSuffix = parts.join('::'); - if (node.qualifiedName.includes(colonSuffix)) return true; - - // Stage 2: file-path containment. Rust modules and Python packages - // are not in `qualifiedName` — they're encoded in the file path. So - // `stage_apply::run` matches a `run` in any file whose path - // contains a `stage_apply` segment (with or without an extension). - // - // Filter out Rust path prefixes that have no file-system equivalent. - const containerHints = parts.slice(0, -1).filter((p) => !RUST_PATH_PREFIXES.has(p)); - if (containerHints.length === 0) return false; - - const segments = node.filePath.split('/').filter((s) => s.length > 0); - return containerHints.every((hint) => - segments.some((seg) => seg === hint || seg.replace(/\.[^.]+$/, '') === hint) - ); + return matchesSymbolShared(node, symbol); } /** From 37106047a046579d189029902f9161c249c058b5 Mon Sep 17 00:00:00 2001 From: ferres Date: Sun, 30 Aug 2026 21:25:01 +0000 Subject: [PATCH 2/2] fix(node): interleave same-named definitions before truncating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `codegraph_node` on an ambiguous bare name renders as many full bodies as fit a char budget, lists up to 20 more by file:line, then summarises the rest as "+N more". The definitions arrive in index order — `(file_path, start_line)` — and nothing reorders them, so a name defined many times under an early-sorting directory consumes the body budget AND the overflow list before a definition under a late-sorting one is reached. That is not a ranking nicety when one name exists in several languages. Measured on a fixture with 40 definitions under `aaa_client/` and one under `zzz_server/`: the lone definition landed past both caps, so the answer named a single language and gave no sign the other existed. The caller cannot tell a truncated answer from a complete one, and "+N more" reads as "nothing you care about" precisely when the tail is the only definition in some language. Round-robin across languages, and within a language across files, keeping original order inside each bucket, before anything truncates. Every source that defines the name is then represented in the first few entries however the paths sort. Inert when there is nothing to interleave, so ordinary single-file overload sets are unaffected. The final "+N more" now names the languages it dropped, so a truncation that does happen is legible rather than silent. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013Bsi9EH64kMisnik4E1gf7 --- CHANGELOG.md | 1 + __tests__/symbol-lookup.test.ts | 72 +++++++++++++++++++++++++++++++++ src/mcp/tools.ts | 60 ++++++++++++++++++++++++++- 3 files changed, 132 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 761b6b1b8..d25cdeb61 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes - `codegraph callers`, `codegraph callees` and `codegraph impact` now understand qualified symbol names. Asking for something like `Accounts.Format.group` used to match nothing, so the command quietly fell back to whichever symbol full-text search happened to rank first — or reported "not found" for a symbol that plainly exists. Qualified names now select exactly the definition you named, including in languages whose module names themselves contain dots. +- Looking up a name that is defined many times no longer hides whole languages or files. Results were ordered by file path and then truncated, so a name defined 40 times under an early-sorting directory pushed the one definition under a late-sorting directory past every cap — the answer covered one language and never mentioned the other existed. Definitions are now interleaved across languages and files before anything is trimmed, and the "and N more" line says what it left out. - Those same commands now tell you when a plain name matches several different definitions. Previously the results for every same-named symbol — often in different languages, since collisions cluster on short names like `group`, `num` or `parse` — were merged into a single list with nothing saying they came from different symbols. The list still aggregates, but it now names what it aggregated and shows you the qualified spelling that narrows it. ## [1.6.0] - 2026-08-26 diff --git a/__tests__/symbol-lookup.test.ts b/__tests__/symbol-lookup.test.ts index e78b12ec2..9446b7318 100644 --- a/__tests__/symbol-lookup.test.ts +++ b/__tests__/symbol-lookup.test.ts @@ -392,3 +392,75 @@ describe.skipIf(!HAS_SQLITE)('lookupSymbolNodes — the shared path used by call expect(nodes.length).toBe(0); }); }); + +describe.skipIf(!HAS_SQLITE)('codegraph_node — a truncating render must not hide a whole source', () => { + // Same-named definitions arrive in `(file_path, start_line)` order, so a name + // defined many times under an early-sorting directory fills the body budget + // AND the overflow list before a definition under a late-sorting directory is + // reached. The answer then names one language and never says the other exists + // — the caller has no way to know it was truncated past something relevant. + let projectRoot: string; + let cg: any; + let handler: any; + + beforeEach(async () => { + projectRoot = tmpRoot(); + const early = path.join(projectRoot, 'aaa_client'); + const late = path.join(projectRoot, 'zzz_server'); + fs.mkdirSync(early, { recursive: true }); + fs.mkdirSync(late, { recursive: true }); + for (let i = 0; i < 40; i++) { + fs.writeFileSync( + path.join(early, `m${String(i).padStart(2, '0')}.ts`), + `export function num(v: number): string {\n return v.toFixed(2);\n}\n` + ); + } + fs.writeFileSync( + path.join(late, 'format.py'), + `def num(value):\n return round(value, 2)\n\ndef label(v):\n return num(v)\n` + ); + + const CodeGraph = (await import('../src/index')).default; + const { ToolHandler } = await import('../src/mcp/tools'); + cg = CodeGraph.initSync(projectRoot, { + config: { include: ['**/*.ts', '**/*.py'], exclude: [] }, + }); + await cg.indexAll(); + handler = new ToolHandler(cg); + }); + + afterEach(() => { + handler?.closeAll(); + cg?.destroy(); + rmTree(projectRoot); + }); + + it('renders the lone late-sorting definition instead of truncating past it', async () => { + const res = await handler.execute('codegraph_node', { symbol: 'num', includeCode: true }); + const text = res.content?.[0]?.text ?? ''; + expect(text).toContain('41 definitions named "num"'); + // The single Python definition must be visible — with its body, not merely + // mentioned — even though 40 TypeScript ones sort ahead of it. + expect(text).toMatch(/zzz_server\/format\.py/); + expect(text).toMatch(/def num\(value\)/); + }); + + it('says which sources the final truncation dropped', async () => { + const res = await handler.execute('codegraph_node', { symbol: 'num', includeCode: true }); + const text = res.content?.[0]?.text ?? ''; + const overflow = /\+(\d+) more \(([^)]+)\)/.exec(text); + expect(overflow).not.toBeNull(); + // A bare "+N more" reads as "nothing you care about" — which is exactly + // wrong when the tail is the only definition in some language. + expect(overflow![2]).toMatch(/typescript/); + expect(overflow![2]).not.toMatch(/python/); + }); + + it('a single-source overload set keeps its original order', async () => { + // Diversification must be inert when there is nothing to interleave, so + // ordinary overload sets (one class, one file) are unaffected. + const res = await handler.execute('codegraph_node', { symbol: 'label', includeCode: true }); + const text = res.content?.[0]?.text ?? ''; + expect(text).toMatch(/format\.py/); + }); +}); diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 535490067..476b15673 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -109,6 +109,51 @@ const MAX_PATH_LENGTH = 4_096; * ../graph/symbol-lookup along with `matchesSymbol`.) */ +/** + * Order same-named definitions so a truncating render cannot hide a whole + * language or a whole file behind one crowded source. + * + * The definitions arrive in index order — `(file_path, start_line)` — so a name + * defined many times under an early-sorting directory fills the render budget + * before a definition under a late-sorting one is ever reached. In a repository + * where one name exists in several languages that is not a ranking nicety: with + * 40 definitions under `assets/` and one under `lib/`, the `lib/` definition + * lands past both the body cap AND the overflow list, so the answer names only + * one language and never says the other exists. + * + * Round-robin across languages, and within a language across files, keeping the + * original order inside each bucket. Every language that defines the name is + * then represented in the first few entries regardless of how the paths sort. + */ +function diversifyBySource(nodes: Node[]): Node[] { + if (nodes.length < 2) return nodes; + const byLanguage = new Map>(); + for (const node of nodes) { + let files = byLanguage.get(node.language); + if (!files) { files = new Map(); byLanguage.set(node.language, files); } + const bucket = files.get(node.filePath); + if (bucket) bucket.push(node); + else files.set(node.filePath, [node]); + } + if (byLanguage.size === 1 && byLanguage.values().next().value!.size === 1) return nodes; + + const out: Node[] = []; + const languages = [...byLanguage.values()].map((files) => ({ files: [...files.values()], next: 0 })); + while (out.length < nodes.length) { + let progressed = false; + for (const lang of languages) { + // Take one from this language's next non-empty file, then move on. + for (let tried = 0; tried < lang.files.length; tried++) { + const bucket = lang.files[lang.next % lang.files.length]!; + lang.next++; + if (bucket.length > 0) { out.push(bucket.shift()!); progressed = true; break; } + } + } + if (!progressed) break; // defensive: every bucket drained + } + return out; +} + /** * Node kinds that contain other symbols. For these, `codegraph_node` with * `includeCode=true` returns a structural outline (member names + signatures @@ -6049,6 +6094,10 @@ export class ToolHandler { // FULL bodies as fit a char budget (the agent gets the one it needs in this // one call, no follow-up parameter to learn), and list any remainder by // file:line so a large overload set can't overflow the per-tool cap. + // Interleave by language/file BEFORE anything truncates, so no source can + // be pushed past the caps below and vanish from the answer entirely. + matches = diversifyBySource(matches); + const header = `**${matches.length} definitions named "${symbol}"**`; if (!includeCode) { const list = matches.map((n) => `- \`${n.name}\` (${n.kind}) — ${n.filePath}:${n.startLine}`); @@ -6092,7 +6141,16 @@ export class ToolHandler { '**Other definitions**', ...shownList.map((n) => `- \`${n.name}\` (${n.kind}) — ${n.filePath}:${n.startLine}`), ); - if (listed.length > LIST_CAP) out.push(`- … +${listed.length - LIST_CAP} more`); + if (listed.length > LIST_CAP) { + // Name what fell off. An unqualified "+N more" reads as "nothing you + // care about", which is exactly wrong when the tail is the only + // definition in some language. + const dropped = listed.slice(LIST_CAP); + const byLang = new Map(); + for (const n of dropped) byLang.set(n.language, (byLang.get(n.language) ?? 0) + 1); + const summary = [...byLang.entries()].map(([lang, n]) => `${n} ${lang}`).join(', '); + out.push(`- … +${dropped.length} more (${summary})`); + } out.push( '', `> Need one of these in full? Call codegraph_node again with \`file\` (e.g. \`"${listed[0]!.filePath.split('/').pop()}"\`) or \`line\` — do NOT Read it.`,