A NestJS-style monorepo has one UserService/UserModule/UserRepository per app; with no package concept for TS they share one global name scope and agents visibly warned that CodeGraph was mixing unrelated classes. Two distinct problems, two fixes: 1. TOOL AGGREGATION. callers/callees returned one merged list across every same-named match, and impact merged all their blast radii into a single overstated subgraph. Now: matches group into DISTINCT DEFINITIONS (filePath + qualifiedName — same-file overloads still merge, that's the overload feature) and render one file-labeled section per definition; a new `file` argument (path or suffix, like codegraph_node's) narrows to one definition, suppressing the stale aggregation note; a non-matching `file` falls back to all definitions with a note. server-instructions documents the behavior. 2. RESOLUTION WRONG EDGES. Auditing a real monorepo (amplication, 54k nodes) found 1,036 cross-package `references` edges into duplicated names. Root cause: the React framework resolver ran PascalCase component resolution on refs from PLAIN .ts FILES (a GraphQL types file's own `Account` type alias lost to an arbitrary same-named CLASS in another package — the resolver's blind `components[0]` fallback at confidence 0.8 outranked the name-matcher's proximity-correct 0.7). Component resolution is now gated to JSX-capable refs (tsx/jsx) and never guesses among multiple candidates without a positional signal (same-dir / component-dir / unique). Cross-package wrong edges: 1,036 -> 40 (-96%; the remainder are genuine shared-model imports and codegen template scaffolds), with the freed refs re-resolving to the correct same-file/same-package targets. excalidraw (a real React repo) is a zero-delta control — legitimate component refs all carry same-dir/component-dir signals. Graph-level separation was verified correct on a fixture before any changes (import + proximity resolution keeps apps apart) — the conflation was tool-level plus the react-resolver edge class. Tests: 6-test e2e suite (grouped callers/callees, per-definition impact radii, file narrowing, fallback note, cross-app edge isolation) + react resolver unit tests updated to production reality (tsx refs resolve, plain-ts refs decline). Full suite 1398 passed. EXTRACTION_VERSION 23 -> 24 (re-index to drop the wrong cross-package edges). Closes #764 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
dce61a5f4a
commit
222af6b87c
@@ -581,12 +581,23 @@ from ..services import auth_service
|
||||
line: 10,
|
||||
column: 5,
|
||||
filePath: 'src/App.tsx',
|
||||
language: 'typescript' as const,
|
||||
// Refs extracted from .tsx files carry language 'tsx' — component
|
||||
// resolution is gated to JSX-capable refs (#764: PascalCase TYPE refs
|
||||
// from plain .ts files were resolving to arbitrary same-named classes).
|
||||
language: 'tsx' as const,
|
||||
};
|
||||
|
||||
const result = reactResolver!.resolve(ref, context);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.targetNodeId).toBe('component:src/Button.tsx:Button:5');
|
||||
|
||||
// The same PascalCase name referenced from a plain .ts file is a TYPE
|
||||
// reference, not a component usage — component resolution must decline
|
||||
// and leave it to proximity-aware name matching (#764: a .ts GraphQL
|
||||
// types file's own `Account` alias was losing to an arbitrary same-named
|
||||
// class in another monorepo package).
|
||||
const tsRef = { ...ref, filePath: 'src/models.ts', language: 'typescript' as const };
|
||||
expect(reactResolver!.resolve(tsRef, context)).toBeNull();
|
||||
});
|
||||
|
||||
it('should resolve custom hook references', () => {
|
||||
|
||||
@@ -0,0 +1,138 @@
|
||||
/**
|
||||
* Same-named symbols across monorepo apps (#764).
|
||||
*
|
||||
* A NestJS-style monorepo has one `UserService` (and friends) per app. The
|
||||
* graph keeps them as distinct nodes (import + proximity resolution), but the
|
||||
* MCP tools used to AGGREGATE them: callers/callees returned one merged list
|
||||
* and impact merged both blast radii — the conflation agents warned about.
|
||||
*
|
||||
* Now: multiple DISTINCT definitions (different file/qualified-name) render
|
||||
* one section per definition, and `file` narrows to a single definition.
|
||||
* Same-file overloads still merge (that's the overload feature).
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as path from 'path';
|
||||
import * as os from 'os';
|
||||
import { CodeGraph } from '../src';
|
||||
import { ToolHandler } from '../src/mcp/tools';
|
||||
import { initGrammars, loadAllGrammars } from '../src/extraction/grammars';
|
||||
|
||||
let tmpDir: string;
|
||||
let cg: CodeGraph;
|
||||
let handler: ToolHandler;
|
||||
|
||||
const text = async (tool: string, args: Record<string, unknown>): Promise<string> => {
|
||||
const res = await handler.execute(tool, args);
|
||||
return res.content?.[0]?.text ?? '';
|
||||
};
|
||||
|
||||
beforeAll(async () => {
|
||||
await initGrammars();
|
||||
await loadAllGrammars();
|
||||
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-764-'));
|
||||
const mk = (rel: string, content: string) => {
|
||||
const p = path.join(tmpDir, rel);
|
||||
fs.mkdirSync(path.dirname(p), { recursive: true });
|
||||
fs.writeFileSync(p, content);
|
||||
};
|
||||
|
||||
for (const app of ['billing', 'admin']) {
|
||||
mk(
|
||||
`apps/${app}/src/users/user.service.ts`,
|
||||
[
|
||||
"import { UserRepository } from './user.repository';",
|
||||
'export class UserService {',
|
||||
' constructor(private readonly repo: UserRepository) {}',
|
||||
' findAll(): string[] {',
|
||||
` return this.repo.load_${app}();`,
|
||||
' }',
|
||||
'}',
|
||||
].join('\n')
|
||||
);
|
||||
mk(
|
||||
`apps/${app}/src/users/user.repository.ts`,
|
||||
`export class UserRepository {\n load_${app}(): string[] { return []; }\n}\n`
|
||||
);
|
||||
mk(
|
||||
`apps/${app}/src/users/user.controller.ts`,
|
||||
[
|
||||
"import { UserService } from './user.service';",
|
||||
'export class UserController {',
|
||||
' constructor(private readonly users: UserService) {}',
|
||||
' list(): string[] { return this.users.findAll(); }',
|
||||
'}',
|
||||
].join('\n')
|
||||
);
|
||||
}
|
||||
|
||||
cg = CodeGraph.initSync(tmpDir);
|
||||
await cg.indexAll();
|
||||
handler = new ToolHandler(cg);
|
||||
}, 120_000);
|
||||
|
||||
afterAll(() => {
|
||||
cg?.destroy();
|
||||
if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
describe('same-named symbols across apps (#764)', () => {
|
||||
it('graph keeps the apps apart: no cross-app edges at all', () => {
|
||||
const billing = new Set(
|
||||
cg.getNodesByName('findAll').filter((n) => n.filePath.includes('billing')).map((n) => n.id)
|
||||
);
|
||||
for (const id of billing) {
|
||||
for (const e of cg.getIncomingEdges(id)) {
|
||||
const src = cg.getNode(e.source);
|
||||
expect(src?.filePath.includes('admin')).toBe(false);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
it('callers: one section per distinct definition, each with only its own callers', async () => {
|
||||
const out = await text('codegraph_callers', { symbol: 'findAll' });
|
||||
expect(out).toContain('2 distinct definitions');
|
||||
// Section per definition…
|
||||
expect(out).toContain('apps/admin/src/users/user.service.ts');
|
||||
expect(out).toContain('apps/billing/src/users/user.service.ts');
|
||||
// …and the billing section must list the billing controller, not admin's.
|
||||
const billingSection = out.slice(out.indexOf('apps/billing/src/users/user.service.ts'));
|
||||
const billingBody = billingSection.slice(0, billingSection.indexOf('###', 3) > 0 ? billingSection.indexOf('###', 3) : undefined);
|
||||
expect(billingBody).toContain('apps/billing/src/users/user.controller.ts');
|
||||
expect(billingBody).not.toContain('apps/admin/src/users/user.controller.ts');
|
||||
});
|
||||
|
||||
it('callers: `file` narrows to one definition (flat list, no stale aggregation note)', async () => {
|
||||
const out = await text('codegraph_callers', {
|
||||
symbol: 'findAll',
|
||||
file: 'apps/billing/src/users/user.service.ts',
|
||||
});
|
||||
expect(out).not.toContain('distinct definitions');
|
||||
expect(out).toContain('apps/billing/src/users/user.controller.ts');
|
||||
expect(out).not.toContain('apps/admin/');
|
||||
expect(out).not.toContain('Aggregated results');
|
||||
});
|
||||
|
||||
it('callers: a non-matching `file` falls back to all definitions with a note', async () => {
|
||||
const out = await text('codegraph_callers', { symbol: 'findAll', file: 'apps/nonexistent/x.ts' });
|
||||
expect(out).toContain('no definition of "findAll" matches file');
|
||||
expect(out).toContain('2 distinct definitions');
|
||||
});
|
||||
|
||||
it('impact: separate blast radius per definition, never a merged one', async () => {
|
||||
const out = await text('codegraph_impact', { symbol: 'UserService' });
|
||||
expect(out).toContain('2 distinct definitions');
|
||||
// Each section's count covers ONE app (service + ctor + findAll +
|
||||
// controller side), not the union of both.
|
||||
const counts = [...out.matchAll(/affects (\d+) symbols/g)].map((m) => Number(m[1]));
|
||||
expect(counts).toHaveLength(2);
|
||||
for (const c of counts) expect(c).toBeLessThanOrEqual(7);
|
||||
});
|
||||
|
||||
it('callees: grouped the same way', async () => {
|
||||
const out = await text('codegraph_callees', { symbol: 'list' });
|
||||
expect(out).toContain('2 distinct definitions');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user