From dad3b8f6f2222189601ca28adf3a98a75a25f29f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Fri, 2 Oct 2026 21:39:31 +0100 Subject: [PATCH] fix(mcp): reject invalid symbol identities before graph reads (#3451) --- gitnexus/src/mcp/local/local-backend.ts | 42 +++- .../local-backend-calltool.test.ts | 95 +++++++ gitnexus/test/unit/calltool-dispatch.test.ts | 9 +- .../unit/symbol-identity-validation.test.ts | 234 ++++++++++++++++++ 4 files changed, 374 insertions(+), 6 deletions(-) create mode 100644 gitnexus/test/unit/symbol-identity-validation.test.ts diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 5b26beeea..5b2f62a9c 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -324,6 +324,28 @@ function nonBlankUid(value: unknown): string | undefined { return typeof value === 'string' ? value.trim() || undefined : undefined; } +const SYMBOL_IDENTITY_RECOVERY_SUGGESTION = + 'Run gitnexus analyze --force from the affected repository root to rebuild the index.'; + +class SymbolIdentityError extends Error { + constructor() { + super('The index returned an invalid symbol identity. ' + SYMBOL_IDENTITY_RECOVERY_SUGGESTION); + this.name = 'SymbolIdentityError'; + } +} + +/** Validate database identities before using them as graph traversal anchors. */ +function assertSymbolIdentity(id: unknown, expectedUid?: string): asserts id is string { + if ( + typeof id !== 'string' || + !id.trim() || + id.includes('\0') || + (expectedUid !== undefined && id !== expectedUid) + ) { + throw new SymbolIdentityError(); + } +} + interface StringAliasDefinition { canonical: string; aliases: readonly string[]; @@ -4529,6 +4551,7 @@ export class LocalBackend { endLine: (r.endLine ?? r[5]) as number, ...(include_content ? { content: (r.content ?? r[6]) as string | undefined } : {}), }; + assertSymbolIdentity(symbol.id, uid); // Same LadybugDB label-enrichment as the name-based path: a UID // pointing at a Class must still surface `type: 'Class'` so impact's // Class/Interface BFS seed fires. No-op when type is already set. @@ -4662,6 +4685,9 @@ export class LocalBackend { endLine: (r.endLine ?? r[5]) as number, ...(include_content ? { content: (r.content ?? r[6]) as string | undefined } : {}), })); + // Reject the whole result before narrowing or scoring: dropping a corrupt + // candidate could make an unrelated surviving symbol look unambiguous. + for (const candidate of normalized) assertSymbolIdentity(candidate.id); // An exact File path wins over anchored suffix candidates. Without this, // `lib/a.ts` and `src/lib/a.ts` both score as File candidates and turn an @@ -4815,6 +4841,9 @@ export class LocalBackend { return await this._contextImpl(repo, params); } catch (err: any) { const msg = (err instanceof Error ? err.message : String(err)) || 'Context query failed'; + if (err instanceof SymbolIdentityError) { + return { error: msg, recoverySuggestion: SYMBOL_IDENTITY_RECOVERY_SUGGESTION }; + } if (isWalCorruptionError(err)) { return { error: msg, @@ -7119,8 +7148,16 @@ export class LocalBackend { // Return structured error instead of crashing (#321) const message = (err instanceof Error ? err.message : String(err)) || 'Impact analysis failed'; - const suggestion = 'The graph query failed — try gitnexus context as a fallback'; - const recoverySuggestion = isWalCorruptionError(err) ? WAL_RECOVERY_SUGGESTION : undefined; + const recoverySuggestion = + err instanceof SymbolIdentityError + ? SYMBOL_IDENTITY_RECOVERY_SUGGESTION + : isWalCorruptionError(err) + ? WAL_RECOVERY_SUGGESTION + : undefined; + const suggestion = + err instanceof SymbolIdentityError + ? SYMBOL_IDENTITY_RECOVERY_SUGGESTION + : 'The graph query failed — try gitnexus context as a fallback'; if (params.mode === 'pdg') { // Symbol resolution never reached the catch with a resolved symbol (the // throw can originate before/within resolution), so the envelope carries @@ -9138,6 +9175,7 @@ export class LocalBackend { ]; try { + assertSymbolIdentity(sym.id ?? sym[0], uid); // skipPerSymbolEnrichment suppresses ONLY the per-symbol STEP_IN_PROCESS // enrichment pass while preserving byDepth. Group-mode cross-repo fan-out // may fan across many repos; the per-symbol pass adds up to MAX_CHUNKS diff --git a/gitnexus/test/integration/local-backend-calltool.test.ts b/gitnexus/test/integration/local-backend-calltool.test.ts index f618a3c5b..35eabbdd4 100644 --- a/gitnexus/test/integration/local-backend-calltool.test.ts +++ b/gitnexus/test/integration/local-backend-calltool.test.ts @@ -956,3 +956,98 @@ withTestLbugDB( }, }, ); + +const PYTHON_METHOD_ID = 'Method:tests/test_supervisor.py:Supervisor.run'; +const PYTHON_CALLER_ID = 'Function:tests/test_supervisor.py:test_run'; +const SWIFT_METHOD_ID = 'Method:Sources/Supervisor.swift:Supervisor.run'; +const SWIFT_CALLER_ID = 'Constructor:Sources/Supervisor.swift:Supervisor.init'; + +withTestLbugDB( + 'symbol-identity-isolation-3424', + (handle) => { + describe('mixed Python/Swift symbol identity isolation (#3424)', () => { + let backend: LocalBackend; + + beforeAll(() => { + backend = (handle as typeof handle & { _backend: LocalBackend })._backend; + }); + + it.each(['name and file', 'UID'])('keeps Python context isolated by %s', async (lookup) => { + const params = + lookup === 'UID' + ? { uid: PYTHON_METHOD_ID } + : { name: 'run', file_path: 'tests/test_supervisor.py' }; + const result = await backend.callTool('context', params); + expect(result).not.toHaveProperty('error'); + expect(result.symbol.uid).toBe(PYTHON_METHOD_ID); + expect(result.incoming.calls.map((caller: { uid: string }) => caller.uid)).toEqual([ + PYTHON_CALLER_ID, + ]); + }); + + it.each(['name and file', 'UID'])('keeps Python impact isolated by %s', async (lookup) => { + const params = + lookup === 'UID' + ? { target_uid: PYTHON_METHOD_ID } + : { target: 'run', file_path: 'tests/test_supervisor.py' }; + const result = await backend.callTool('impact', { + ...params, + direction: 'upstream', + includeTests: true, + }); + expect(result).not.toHaveProperty('error'); + expect(result.target.id).toBe(PYTHON_METHOD_ID); + expect(result.impactedCount).toBe(1); + expect(result.byDepth[1].map((caller: { id: string }) => caller.id)).toEqual([ + PYTHON_CALLER_ID, + ]); + }); + + it('keeps the unrelated Swift constructor queryable', async () => { + const context = await backend.callTool('context', { uid: SWIFT_METHOD_ID }); + expect(context).not.toHaveProperty('error'); + expect(context.symbol.uid).toBe(SWIFT_METHOD_ID); + expect(context.incoming.calls.map((caller: { uid: string }) => caller.uid)).toEqual([ + SWIFT_CALLER_ID, + ]); + const impact = await backend.callTool('impact', { + target_uid: SWIFT_METHOD_ID, + direction: 'upstream', + includeTests: true, + }); + expect(impact).not.toHaveProperty('error'); + expect(impact.target.id).toBe(SWIFT_METHOD_ID); + expect(impact.impactedCount).toBe(1); + expect(impact.byDepth[1].map((caller: { id: string }) => caller.id)).toEqual([ + SWIFT_CALLER_ID, + ]); + }); + }); + }, + { + seed: [ + `CREATE (:Method {id: '${PYTHON_METHOD_ID}', name: 'run', filePath: 'tests/test_supervisor.py', startLine: 3, endLine: 5})`, + `CREATE (:Function {id: '${PYTHON_CALLER_ID}', name: 'test_run', filePath: 'tests/test_supervisor.py', startLine: 7, endLine: 9})`, + `CREATE (:Method {id: '${SWIFT_METHOD_ID}', name: 'run', filePath: 'Sources/Supervisor.swift', startLine: 3, endLine: 5})`, + `CREATE (:Constructor {id: '${SWIFT_CALLER_ID}', name: 'init', filePath: 'Sources/Supervisor.swift', startLine: 7, endLine: 9})`, + `MATCH (a:Function), (b:Method) WHERE a.id = '${PYTHON_CALLER_ID}' AND b.id = '${PYTHON_METHOD_ID}' CREATE (a)-[:CodeRelation {type: 'CALLS', confidence: 1.0, reason: 'direct', step: 0}]->(b)`, + `MATCH (a:Constructor), (b:Method) WHERE a.id = '${SWIFT_CALLER_ID}' AND b.id = '${SWIFT_METHOD_ID}' CREATE (a)-[:CodeRelation {type: 'CALLS', confidence: 1.0, reason: 'direct', step: 0}]->(b)`, + ], + poolAdapter: true, + afterSetup: async (handle) => { + vi.mocked(listRegisteredRepos).mockResolvedValue([ + { + name: 'mixed-language-repo', + path: '/mixed-language/repo', + storagePath: handle.tmpHandle.dbPath, + indexedAt: new Date().toISOString(), + lastCommit: 'abc123', + stats: { files: 2, nodes: 4, communities: 0, processes: 0 }, + }, + ]); + const backend = new LocalBackend(); + await backend.init(); + (handle as typeof handle & { _backend: LocalBackend })._backend = backend; + }, + }, +); diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 6a0a98d4a..aeefc94a4 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -624,8 +624,8 @@ describe('LocalBackend.callTool', () => { }); it('reports UNKNOWN instead of a blast radius when the target resolves without a node id (#3354)', async () => { - // Every query returns the same id-less row: the resolver picks it as the - // single match, and the frontier query would answer for no symbol at all. + // Every query returns the same id-less row: resolution must reject it + // before a frontier query could answer for no symbol at all. (executeParameterized as any).mockResolvedValue([{ name: 'runSweep', type: 'Function' }]); const result = await backend.callTool('impact', { target: 'runSweep', direction: 'upstream' }); @@ -635,7 +635,8 @@ describe('LocalBackend.callTool', () => { impactedCount: null, risk: 'UNKNOWN', }); - expect(result.error).toMatch(/without a node id/); + expect(result.error).toMatch(/invalid symbol identity/); + expect(result.recoverySuggestion).toContain('gitnexus analyze --force'); expect(result).not.toHaveProperty('byDepthCounts'); }); @@ -2466,7 +2467,7 @@ describe('LocalBackend.callTool', () => { }, ]); - const result = await backend.impactByUid('test-project', 'uid:main', 'upstream', { + const result = await backend.impactByUid('test-project', 'func:main', 'upstream', { maxDepth: 5, relationTypes: ['CALLS'], minConfidence: 0, diff --git a/gitnexus/test/unit/symbol-identity-validation.test.ts b/gitnexus/test/unit/symbol-identity-validation.test.ts new file mode 100644 index 000000000..7fe380a20 --- /dev/null +++ b/gitnexus/test/unit/symbol-identity-validation.test.ts @@ -0,0 +1,234 @@ +/** Regression for unusable database lookup identities (#3424). */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { lbugMocks } = vi.hoisted(() => ({ + lbugMocks: { + initLbug: vi.fn().mockResolvedValue(undefined), + executeQuery: vi.fn().mockResolvedValue([]), + executeParameterized: vi.fn().mockResolvedValue([]), + closeLbug: vi.fn().mockResolvedValue(undefined), + isLbugReady: vi.fn().mockReturnValue(true), + }, +})); + +vi.mock('../../src/core/lbug/pool-adapter.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, ...lbugMocks }; +}); + +vi.mock('../../src/mcp/core/lbug-adapter.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, ...lbugMocks }; +}); + +vi.mock('../../src/storage/repo-manager.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + listRegisteredRepos: vi.fn().mockResolvedValue([ + { + name: 'test-project', + path: '/tmp/test-project', + storagePath: '/tmp/.gitnexus/test-project', + indexedAt: '2024-06-01T12:00:00Z', + lastCommit: 'abc123', + stats: { files: 10, nodes: 50, edges: 100, communities: 3, processes: 5 }, + }, + ]), + cleanupOldKuzuFiles: vi.fn().mockResolvedValue({ found: false, needsReindex: false }), + findSiblingClones: vi.fn().mockResolvedValue([]), + }; +}); + +vi.mock('../../src/core/git-staleness.js', () => ({ + checkStaleness: vi.fn().mockReturnValue({ isStale: false, commitsBehind: 0 }), + checkStalenessAsync: vi.fn().mockResolvedValue({ isStale: false, commitsBehind: 0 }), + checkCwdMatch: vi.fn().mockResolvedValue({ match: 'none' }), +})); + +vi.mock('../../src/storage/git.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getGitRoot: vi.fn().mockReturnValue(null) }; +}); + +vi.mock('../../src/core/search/bm25-index.js', () => ({ + searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), +})); + +vi.mock('../../src/mcp/core/embedder.js', () => ({ + embedQuery: vi.fn().mockResolvedValue([]), + getEmbeddingDims: vi.fn().mockReturnValue(384), +})); + +import { LocalBackend } from '../../src/mcp/local/local-backend.js'; +import { executeParameterized } from '../../src/mcp/core/lbug-adapter.js'; + +const SYMBOL = { + id: 'Method:tests/test_supervisor.py:Supervisor.test_run', + name: 'test_run', + type: 'Method', + filePath: 'tests/test_supervisor.py', + startLine: 3, + endLine: 5, +}; + +const badIds = [ + ['missing', undefined], + ['null', null], + ['number', 42], + ['empty', ''], + ['blank', ' \t '], + ['NUL-only', '\0\0'], + ['embedded NUL', 'Method:tests/test_supervisor.py:\0test_run'], +] as const; + +function row(id: unknown, shape: string): unknown { + return shape === 'tuple' + ? [id, SYMBOL.name, SYMBOL.type, SYMBOL.filePath, SYMBOL.startLine, SYMBOL.endLine] + : { ...SYMBOL, id }; +} + +const surfaces = [ + { tool: 'context', params: { name: SYMBOL.name, file_path: SYMBOL.filePath } }, + { tool: 'impact', params: { target: SYMBOL.name, direction: 'upstream' } }, + { tool: 'impact', params: { target: SYMBOL.name, direction: 'upstream', mode: 'pdg' } }, +] as const; + +describe('symbol lookup identity validation (#3424)', () => { + let backend: LocalBackend; + + beforeEach(async () => { + vi.clearAllMocks(); + backend = new LocalBackend(); + await backend.init(); + (backend as any).ensureInitialized = vi.fn().mockResolvedValue(undefined); + }); + + function lookupRows(rows: unknown[]) { + vi.mocked(executeParameterized).mockImplementation(async (_db, _query, params) => { + return params?.symName || params?.uid ? rows : []; + }); + } + + function expectNoExpansion() { + const queries = vi.mocked(executeParameterized).mock.calls.map(([, query]) => query); + expect(queries.length).toBeGreaterThan(0); + expect(queries.every((query) => !query.includes('CodeRelation'))).toBe(true); + expect(queries.every((query) => !query.includes('UNION'))).toBe(true); + } + + function expectIdentityError(result: any, tool: string) { + expect(result.error).toMatch(/symbol identity/i); + expect(result.recoverySuggestion).toMatch(/analyze.*--force/); + expect(result.epistemic).not.toBe('exact'); + expect(result).not.toHaveProperty('symbol'); + expect(result).not.toHaveProperty('incoming'); + if (tool === 'impact') { + expect(result.risk).toBe('UNKNOWN'); + expect(result.impactedCount).toBeNull(); + expect(result.target).not.toHaveProperty('id'); + expect(result.suggestion ?? '').not.toContain('context'); + } + expectNoExpansion(); + } + + for (const surface of surfaces) { + describe(`${surface.tool} ${'mode' in surface.params ? surface.params.mode : 'default'}`, () => { + for (const shape of ['object', 'tuple']) { + it.each(badIds)(`rejects %s IDs in ${shape} rows before traversal`, async (_label, id) => { + lookupRows([row(id, shape)]); + const result = await backend.callTool(surface.tool, surface.params); + expectIdentityError(result, surface.tool); + }); + it.each(badIds)( + `rejects %s IDs from exact UID lookups in ${shape} rows`, + async (_label, id) => { + lookupRows([row(id, shape)]); + const uidParam = + surface.tool === 'context' ? { uid: SYMBOL.id } : { target_uid: SYMBOL.id }; + expectIdentityError( + await backend.callTool(surface.tool, { ...surface.params, ...uidParam }), + surface.tool, + ); + }, + ); + } + + it('does not choose a healthy candidate beside a corrupt candidate', async () => { + lookupRows([SYMBOL, { ...SYMBOL, id: '\0', type: '' }]); + expectIdentityError(await backend.callTool(surface.tool, surface.params), surface.tool); + }); + + it('rejects a different identity returned for an exact UID', async () => { + lookupRows([{ ...SYMBOL, id: 'Constructor:Services.swift:Service.init' }]); + const uidParam = + surface.tool === 'context' ? { uid: SYMBOL.id } : { target_uid: SYMBOL.id }; + expectIdentityError( + await backend.callTool(surface.tool, { ...surface.params, ...uidParam }), + surface.tool, + ); + }); + + it('keeps an empty lookup distinct from an invalid identity', async () => { + lookupRows([]); + const result = await backend.callTool(surface.tool, surface.params); + expect(result.error).toMatch(/not found/); + expect(result.recoverySuggestion).toBeUndefined(); + }); + }); + } + + it('validates every row before exact File narrowing', async () => { + lookupRows([ + { ...SYMBOL, id: 'File:tests/test_supervisor.py', name: 'test_supervisor.py' }, + { ...SYMBOL, id: undefined }, + ]); + expectIdentityError(await backend.callTool('context', { name: SYMBOL.filePath }), 'context'); + }); + + for (const shape of ['object', 'tuple']) { + it(`accepts an opaque legacy identity in a healthy ${shape} row`, async () => { + lookupRows([row('func:alpha', shape)]); + const result = await backend.callTool('context', { uid: 'func:alpha' }); + expect(result).not.toHaveProperty('error'); + expect(result.symbol.uid).toBe('func:alpha'); + }); + } + + it('preserves a valid ID byte-for-byte instead of trimming it', async () => { + lookupRows([{ ...SYMBOL, id: 'func:alpha ' }]); + const result = await backend.callTool('context', { name: SYMBOL.name }); + expect(result).not.toHaveProperty('error'); + expect(result.symbol.uid).toBe('func:alpha '); + }); + + describe('group impact UID adapter', () => { + const opts = { maxDepth: 3, relationTypes: ['CALLS'], minConfidence: 0, includeTests: true }; + + it.each(badIds)('rejects a %s persisted ID before BFS', async (_label, id) => { + lookupRows([{ ...SYMBOL, id }]); + const bfs = vi.spyOn(backend as any, '_runImpactBFS').mockResolvedValue({ byDepth: {} }); + const repoId = [...(backend as any).repos.keys()][0]; + expect(await backend.impactByUid(repoId, SYMBOL.id, 'upstream', opts)).toBeNull(); + expect(bfs).not.toHaveBeenCalled(); + }); + + it('rejects an otherwise valid mismatched UID', async () => { + lookupRows([{ ...SYMBOL, id: 'route:other' }]); + const bfs = vi.spyOn(backend as any, '_runImpactBFS').mockResolvedValue({ byDepth: {} }); + const repoId = [...(backend as any).repos.keys()][0]; + expect(await backend.impactByUid(repoId, SYMBOL.id, 'upstream', opts)).toBeNull(); + expect(bfs).not.toHaveBeenCalled(); + }); + + it('accepts a legitimate synthetic identity', async () => { + lookupRows([{ ...SYMBOL, id: 'Route:svc:/health' }]); + const bfs = vi.spyOn(backend as any, '_runImpactBFS').mockResolvedValue({ byDepth: {} }); + const repoId = [...(backend as any).repos.keys()][0]; + expect(await backend.impactByUid(repoId, 'Route:svc:/health', 'upstream', opts)).toEqual({ + byDepth: {}, + }); + expect(bfs).toHaveBeenCalledOnce(); + }); + }); +});