fix(mcp): reject invalid symbol identities before graph reads (#3451)

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 <symbol> 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 <symbol> 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

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;
+    },
+  },
+);

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,

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<typeof import('../../src/storage/repo-manager.js')>();
+  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<typeof import('../../src/storage/git.js')>();
+  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();
+    });
+  });
+});