fix(cli): group callers/callees/impact by definition (#1512) (#1801)

* fix(cli): port upstream symbol lookup consistency (#1656, #1512)

Port ferrine/fix/symbol-lookup-consistency at
c0ccbacd3f52007b65ce5b9599fa7a086501ac39 onto current main.
Qualified CLI queries use the shared matcher and ambiguous names disclose
their targets. Keep total/limit/truncated and the human truncation notice
from #1674, and share the matcher with main's named-symbol-flow module.

Refs #1512. Upstream PR: #1656.

Co-authored-by: ferres <justferres@yandex.ru>

* fix(cli): group traversal results by definition (#1512)

Extend upstream PR #1656, ported in 7038fb4f, so callers/callees/impact
show separate sections for each definition and accept --file using the
same groupDefinitions helper as MCP. Preserve same-file overload groups,
path/suffix matching, and the explicit fallback when no file matches.

JSON definitions carry their roots, own neighbors/affected nodes, and
edges. Retain the legacy top-level lists as an explicitly labeled union
and preserve #1674 total/limit/truncated; each callers/callees definition
also reports its own limit and truncation metadata.

Validation: npm run build; 141 tests across 14 targeted suites, including
32 CLI regression tests. The Linux /workspace/cg1512-repro failure now
passes for all three commands, with and without --file.

Fixes #1512.
Upstream PR: #1656 (ferrine/fix/symbol-lookup-consistency @ c0ccbacd).

---------

Co-authored-by: Colby McHenry <colbymchenry@users.noreply.github.com>
Co-authored-by: ferres <justferres@yandex.ru>
This commit is contained in:
Colby Mchenry
2026-09-08 17:17:31 -05:00
committed by GitHub
co-authored by Colby McHenry ferres
parent 71d049cd28
commit 8c9c4761b0
8 changed files with 867 additions and 343 deletions
+262
View File
@@ -0,0 +1,262 @@
/** CLI parity with MCP definition grouping and file narrowing (#1512, #1656). */
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { spawnSync } from 'child_process';
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';
import { CodeGraph } from '../src';
import { ToolHandler } from '../src/mcp/tools';
import { lookupSymbolNodes } from '../src/graph/symbol-lookup';
const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js');
const COMMANDS = ['callers', 'callees', 'impact'] as const;
type Command = typeof COMMANDS[number];
let projectRoot: string;
let cg: CodeGraph;
let handler: ToolHandler;
function runCli(command: Command, symbol = 'handle', args: string[] = []) {
return spawnSync(process.execPath, [BIN, command, '-p', projectRoot, ...args, '--', symbol], {
encoding: 'utf-8',
env: { ...process.env, CODEGRAPH_NO_DAEMON: '1', CODEGRAPH_WASM_RELAUNCHED: '1', NO_COLOR: '1' },
timeout: 30_000,
});
}
function json(command: Command, symbol = 'handle', args: string[] = []) {
const result = runCli(command, symbol, [...args, '--json']);
expect(result.status, result.stderr).toBe(0);
return JSON.parse(result.stdout);
}
function resultKey(command: Command) {
return command === 'impact' ? 'affected' : command;
}
function write(file: string, source: string) {
const absolute = path.join(projectRoot, file);
fs.mkdirSync(path.dirname(absolute), { recursive: true });
fs.writeFileSync(absolute, source);
}
beforeAll(async () => {
projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-cli-1512-'));
for (const [dir, helper] of [['a', 'alpha'], ['b', 'beta']]) {
write(`${dir}/${helper}.js`, `export function ${helper}() { return 1; }\n`);
write(`${dir}/svc.js`, `import { ${helper} } from './${helper}.js';\nexport function handle() { return ${helper}(); }\n`);
write(`${dir}/main.js`, `import { handle } from './svc.js';\nexport function ${dir}Main() { return handle(); }\nexport function ${dir}Entry() { return ${dir}Main(); }\n`);
write(`${dir}/work.js`, `import { shared } from '../shared.js';\nimport { ${helper} } from './${helper}.js';\nexport function work() { shared(); return ${helper}(); }\n`);
write(`${dir}/work-caller.js`, `import { work } from './work.js';\nexport function ${dir}Worker() { work(); }\n`);
}
write('shared.js', 'export function shared() {}\n');
write('both.js', "import { work as aWork } from './a/work.js';\nimport { work as bWork } from './b/work.js';\nexport function both() { aWork(); bWork(); }\n");
write('quiet-a.js', 'export function quiet() {}\n');
write('quiet-b.js', "import { alpha } from './a/alpha.js';\nexport function quiet() { alpha(); }\nexport function wake() { quiet(); }\n");
write('scopes.ts', [
'function leftOnly() {}',
'function rightOnly() {}',
'export class Left { run() { leftOnly(); } }',
'export class Right { run() { rightOnly(); } }',
].join('\n'));
// Java overloads have separate bodies/nodes; TS signature-only overloads are
// intentionally skipped by extraction, so they cannot exercise grouping.
write('Overloads.java', [
'public class Overloads {',
' static String stringIdentity(String value) { return value; }',
' static int intIdentity(int value) { return value; }',
' public static String convert(String value) { return stringIdentity(value); }',
' public static int convert(int value) { return intIdentity(value); }',
' public static void convertCaller() { convert(1); convert("value"); }',
'}',
].join('\n'));
for (let i = 0; i < 55; i++) {
write(`crowd/def-${i}.js`, "import { shared } from '../shared.js';\nexport function crowded() { shared(); }\n");
}
cg = CodeGraph.initSync(projectRoot);
await cg.indexAll();
handler = new ToolHandler(cg);
}, 30_000);
afterAll(() => {
handler?.closeAll();
cg?.close();
if (projectRoot) fs.rmSync(projectRoot, { recursive: true, force: true });
});
describe.each(COMMANDS)('%s definition grouping (#1512)', (command) => {
it('attributes every result and graph edge to its definition in JSON', () => {
const out = json(command);
expect(out.ambiguous).toBe(true);
expect(out.aggregation).toBe('union');
expect(out.definitions).toHaveLength(2);
const key = resultKey(command);
for (const [dir, other] of [['a', 'b'], ['b', 'a']]) {
const group = out.definitions.find((d: any) => d.definition.filePath === `${dir}/svc.js`);
expect(group.definition).toMatchObject({ name: 'handle', kind: 'function', startLine: 2 });
expect(group.roots).toHaveLength(1);
expect(group[key].length).toBeGreaterThan(0);
expect(group[key].every((n: any) => n.filePath.startsWith(`${dir}/`))).toBe(true);
expect(JSON.stringify(group)).not.toContain(`"filePath":"${other}/`);
const actual = cg.getNodesByName('handle').find(n => n.filePath === `${dir}/svc.js`)!;
const expectedNodes = command === 'impact'
? [...cg.getImpactRadius(actual.id, 2).nodes.values()]
: cg[command === 'callers' ? 'getCallers' : 'getCallees'](actual.id).map(c => c.node);
expect(new Set(group[key].map((n: any) => n.id))).toEqual(new Set(expectedNodes.map(n => n.id)));
const ids = new Set([...group.roots, ...group[key].map((n: any) => n.id)]);
expect(group.edges.length).toBeGreaterThan(0);
for (const edge of group.edges) {
expect(ids.has(edge.source)).toBe(true);
expect(ids.has(edge.target)).toBe(true);
}
}
});
it('prints each definition above only its own results', () => {
const result = runCli(command);
expect(result.status, result.stderr).toBe(0);
expect(result.stdout).toContain('2 distinct definitions');
expect(result.stdout).toContain('--file');
const sections = result.stdout.split(/(?=function handle \(javascript\) — [ab]\/svc\.js:2)/).slice(1);
expect(sections).toHaveLength(2);
for (const section of sections) {
const dir = section.includes('— a/svc.js:2') ? 'a' : 'b';
expect(section).toContain(command === 'callees' ? `${dir}/${dir === 'a' ? 'alpha' : 'beta'}.js` : `${dir}/main.js`);
expect(section).not.toContain(dir === 'a' ? 'b/' : 'a/');
}
});
it.each(['a/svc.js', './a/svc.js'])('--file %s selects the same definition as MCP', async (file) => {
const out = json(command, 'handle', ['--file', file]);
expect(out.definitions).toHaveLength(1);
expect(out.definitions[0].definition.filePath).toBe('a/svc.js');
expect(out.targets.every((n: any) => n.filePath === 'a/svc.js')).toBe(true);
expect(out.ambiguous).toBe(false);
expect(out.filteredOut).toBe(false);
expect(out[resultKey(command)].every((n: any) => n.filePath.startsWith('a/'))).toBe(true);
const human = runCli(command, 'handle', ['--file', file]).stdout;
const mcp = (await handler.execute(`codegraph_${command}`, { symbol: 'handle', file })).content[0]?.text ?? '';
for (const text of [human, mcp]) {
expect(text).not.toContain('b/');
expect(text).not.toContain('distinct definitions');
expect(text).toContain(command === 'callees' ? 'a/alpha.js' : 'a/main.js');
}
});
it('a suffix matching both files keeps both definitions', () => {
const out = json(command, 'handle', ['-f', 'svc.js']);
expect(out.definitions).toHaveLength(2);
expect(out.filteredOut).toBe(false);
});
it('a non-matching file discloses the fallback in JSON and text', async () => {
const note = 'no definition of "handle" matches file "missing.js" — showing all definitions instead.';
const out = json(command, 'handle', ['--file', 'missing.js']);
expect(out.filteredOut).toBe(true);
expect(out.note).toBe(note);
expect(out.definitions).toHaveLength(2);
expect(runCli(command, 'handle', ['--file', 'missing.js']).stdout).toContain(note);
const mcp = await handler.execute(`codegraph_${command}`, { symbol: 'handle', file: 'missing.js' });
expect(mcp.content[0]?.text).toContain(note);
});
it('keeps same-file overloads together as MCP does', () => {
const out = json(command, 'convert');
expect(cg.getNodesByName('convert').length).toBeGreaterThan(1);
expect(out.definitions).toHaveLength(1);
expect(out.definitions[0].roots.length).toBeGreaterThan(1);
expect(out.ambiguous).toBe(false);
expect(lookupSymbolNodes(cg, 'convert').ambiguous).toBe(false);
expect(out.definitions[0][resultKey(command)].length).toBeGreaterThan(0);
});
it('does not substitute another definition for an unknown qualified name', () => {
const out = runCli(command, 'Missing.run');
expect(out.status, out.stderr).toBe(0);
expect(out.stdout).toContain('Symbol "Missing.run" not found');
expect(out.stdout).not.toContain('leftOnly');
expect(out.stdout).not.toContain('rightOnly');
});
});
describe('CLI definition boundaries and limits', () => {
it('separates different qualified names within the same file', () => {
const out = json('callees', 'run', ['--file', 'scopes.ts']);
expect(out.definitions).toHaveLength(2);
for (const name of ['Left', 'Right']) {
const group = out.definitions.find((d: any) => d.definition.qualifiedName === `${name}::run`);
expect(group.callees.map((n: any) => n.name)).toEqual([`${name.toLowerCase()}Only`]);
}
const qualified = json('callees', 'Left.run');
expect(qualified.definitions).toHaveLength(1);
expect(qualified.callees.map((n: any) => n.name)).toEqual(['leftOnly']);
});
it.each(['callers', 'callees'] as const)('%s includes definitions with no edges', (command) => {
const out = json(command, 'quiet');
expect(out.definitions).toHaveLength(2);
const empty = out.definitions.find((d: any) => d.definition.filePath === 'quiet-a.js');
expect(empty[command]).toEqual([]);
expect(empty.edges).toEqual([]);
expect(empty).toMatchObject({ total: 0, limit: 20, truncated: false });
expect(runCli(command, 'quiet').stdout).toContain(`(no ${command})`);
});
it('keeps shared callers and callees in each definition instead of deduplicating across them', () => {
for (const command of ['callers', 'callees'] as const) {
const out = json(command, 'work');
expect(out.definitions).toHaveLength(2);
for (const group of out.definitions) {
expect(group[command].map((n: any) => n.name)).toContain(command === 'callers' ? 'both' : 'shared');
}
expect(out[command].filter((n: any) => n.name === (command === 'callers' ? 'both' : 'shared'))).toHaveLength(1);
}
});
it.each(['callers', 'callees'] as const)('%s preserves union metadata and limits each definition independently', (command) => {
// Callers include the importing file nodes as well as the calling functions.
const total = command === 'callers' ? 6 : 3;
const perDefinition = command === 'callers' ? 4 : 2;
const out = json(command, 'work', ['--limit', '1']);
expect(out).toMatchObject({ total, limit: 1, truncated: true });
expect(out[command]).toHaveLength(1);
for (const group of out.definitions) {
expect(group).toMatchObject({ total: perDefinition, limit: 1, truncated: true });
expect(group[command]).toHaveLength(1);
expect(group.edges).toHaveLength(1);
expect(group.edges[0][command === 'callers' ? 'source' : 'target']).toBe(group[command][0].id);
}
const human = runCli(command, 'work', ['--limit', '1']).stdout;
expect(human.split(`Showing 1 of ${perDefinition}; pass --limit to widen.`)).toHaveLength(3);
const complete = json(command, 'work', ['--limit', '100']);
expect(complete).toMatchObject({ total, limit: 100, truncated: false });
for (const group of complete.definitions) {
expect(group).toMatchObject({ total: perDefinition, limit: 100, truncated: false });
expect(group[command]).toHaveLength(perDefinition);
}
});
it('applies impact depth within each definition and reports its own graph counts', () => {
for (const depth of [1, 2]) {
const out = json('impact', 'handle', ['--depth', String(depth)]);
expect(out.depth).toBe(depth);
// Each root also has an importing file node at depth one.
expect(out.nodeCount).toBe(2 * (depth + 2));
expect(out.edgeCount).toBe(2 * (depth + 1));
for (const group of out.definitions) {
expect(group.nodeCount).toBe(depth + 2);
expect(group.affected).toHaveLength(group.nodeCount);
expect(group.edgeCount).toBe(depth + 1);
expect(group.edges).toHaveLength(group.edgeCount);
}
}
});
it('enumerates definitions beyond the FTS cap and can narrow to any of them', () => {
const out = json('callees', 'crowded');
expect(out.definitions).toHaveLength(55);
for (const group of out.definitions) expect(group.callees.map((n: any) => n.name)).toEqual(['shared']);
const narrowed = json('callees', 'crowded', ['--file', 'crowd/def-54.js']);
expect(narrowed.definitions).toHaveLength(1);
expect(narrowed.definitions[0].definition.filePath).toBe('crowd/def-54.js');
});
});
+172
View File
@@ -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>): 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);
});
});