From d11130378bc2076104dba41369906d9866848a96 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 3 Apr 2026 20:44:27 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20Codex=20Round=203=20=E2=80=94=20METHOD?= =?UTF-8?q?=5FIMPLEMENTS=20soundness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Depth-based nearest-match in findInheritedMethod: level-order BFS stops at the first depth with matches. C extends B extends A where both B.foo and A.foo exist now correctly returns B.foo (nearest) instead of treating the chain as ambiguous. 2. Concrete-source guard: skip Interface/Trait nodes entirely in emitMethodImplementsEdges, and skip abstract methods when building candidate lists. Prevents false edges like interface B.foo → A.foo. 3. Add OVERRIDES to default impact traversal fallback arrays so pre-rename indexes are traversed without explicit relationTypes. 6 new tests: nearest-match chain, deep single-match, interface-extends guard, abstract method skip, abstract+concrete mix, concrete regression. --- gitnexus/src/core/ingestion/mro-processor.ts | 79 +++++++----- gitnexus/src/mcp/local/local-backend.ts | 40 +++++- gitnexus/test/unit/mro-processor.test.ts | 122 +++++++++++++++++++ 3 files changed, 205 insertions(+), 36 deletions(-) diff --git a/gitnexus/src/core/ingestion/mro-processor.ts b/gitnexus/src/core/ingestion/mro-processor.ts index 9b95e2133..b1e28c859 100644 --- a/gitnexus/src/core/ingestion/mro-processor.ts +++ b/gitnexus/src/core/ingestion/mro-processor.ts @@ -479,6 +479,9 @@ function emitMethodImplementsEdges( const classNode = graph.getNode(classId); if (!classNode) continue; + // Interfaces and traits declare contracts — they don't implement them + if (classNode.label === 'Interface' || classNode.label === 'Trait') continue; + // Get this class's own methods const ownMethodIds = methodMap.get(classId) ?? []; @@ -490,6 +493,8 @@ function emitMethodImplementsEdges( for (const methodId of ownMethodIds) { const methodNode = graph.getNode(methodId); if (!methodNode || methodNode.label === 'Property') continue; + // Abstract methods don't satisfy interface contracts + if (methodNode.properties.isAbstract === true) continue; const name = methodNode.properties.name as string; const parameterTypes = (methodNode.properties.parameterTypes as string[] | undefined) ?? []; const parameterCount = methodNode.properties.parameterCount as number | undefined; @@ -632,48 +637,58 @@ function findInheritedMethod( } } - // Collect all matches across the full BFS, then check for ambiguity. - // If the same methodId is reachable via multiple paths (diamond), it's not - // ambiguous. Only distinct methodIds count as separate matches. - const matches = new Map(); + // Level-order BFS: process all ancestors at the current depth before + // advancing. Once any match is found at depth D, finish that depth and stop. + // Diamond dedup: same methodId via two paths at the same depth = 1 match. + let currentLevel = [...queue]; - while (queue.length > 0) { - const ancestorId = queue.shift()!; - if (visited.has(ancestorId)) continue; - visited.add(ancestorId); + while (currentLevel.length > 0) { + const matches = new Map(); + const nextLevel: string[] = []; - // Check this ancestor's methods - const methods = methodMap.get(ancestorId) ?? []; - for (const mid of methods) { - const mNode = graph.getNode(mid); - if (!mNode || mNode.label === 'Property') continue; - if (mNode.properties.name !== methodName) continue; + for (const ancestorId of currentLevel) { + if (visited.has(ancestorId)) continue; + visited.add(ancestorId); - const mParamTypes = (mNode.properties.parameterTypes as string[] | undefined) ?? []; - const mParamCount = mNode.properties.parameterCount as number | undefined; - if (parameterTypesMatch(mParamTypes, targetParamTypes, mParamCount, targetParamCount)) { - matches.set(mid, { methodId: mid, parameterTypes: mParamTypes }); + // Check this ancestor's methods + const methods = methodMap.get(ancestorId) ?? []; + for (const mid of methods) { + const mNode = graph.getNode(mid); + if (!mNode || mNode.label === 'Property') continue; + // Abstract inherited methods don't count as concrete implementations + if (mNode.properties.isAbstract === true) continue; + if (mNode.properties.name !== methodName) continue; + + const mParamTypes = (mNode.properties.parameterTypes as string[] | undefined) ?? []; + const mParamCount = mNode.properties.parameterCount as number | undefined; + if (parameterTypesMatch(mParamTypes, targetParamTypes, mParamCount, targetParamCount)) { + matches.set(mid, { methodId: mid, parameterTypes: mParamTypes }); + } } - } - // Continue walking EXTENDS parents of this ancestor - const grandparents = parentMap.get(ancestorId) ?? []; - const ancestorEdges = parentEdgeType.get(ancestorId); - for (const gp of grandparents) { - if (visited.has(gp)) continue; - const gpEdge = ancestorEdges?.get(gp); - if (gpEdge === 'EXTENDS') { - const gpNode = graph.getNode(gp); - if (gpNode && gpNode.label !== 'Interface' && gpNode.label !== 'Trait') { - queue.push(gp); + // Collect EXTENDS parents for the next depth level + const grandparents = parentMap.get(ancestorId) ?? []; + const ancestorEdges = parentEdgeType.get(ancestorId); + for (const gp of grandparents) { + if (visited.has(gp)) continue; + const gpEdge = ancestorEdges?.get(gp); + if (gpEdge === 'EXTENDS') { + const gpNode = graph.getNode(gp); + if (gpNode && gpNode.label !== 'Interface' && gpNode.label !== 'Trait') { + nextLevel.push(gp); + } } } } + + // If any matches found at this depth, decide and stop + if (matches.size === 1) return matches.values().next().value!; + if (matches.size > 1) return null; // ambiguous at same depth + + currentLevel = nextLevel; } - // Ambiguous: 2+ distinct methods from different ancestors - if (matches.size === 1) return matches.values().next().value!; - return null; // 0 matches or ambiguous (2+) + return null; // no matches found } /** diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index af129e2f8..5f39e5999 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -1920,11 +1920,27 @@ export class LocalBackend { const rawRelTypes = mappedRelTypes && mappedRelTypes.length > 0 ? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t)) - : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS']; + : [ + 'CALLS', + 'IMPORTS', + 'EXTENDS', + 'IMPLEMENTS', + 'METHOD_OVERRIDES', + 'OVERRIDES', + 'METHOD_IMPLEMENTS', + ]; const relationTypes = rawRelTypes.length > 0 ? rawRelTypes - : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS']; + : [ + 'CALLS', + 'IMPORTS', + 'EXTENDS', + 'IMPLEMENTS', + 'METHOD_OVERRIDES', + 'OVERRIDES', + 'METHOD_IMPLEMENTS', + ]; const includeTests = params.includeTests ?? false; const minConfidence = params.minConfidence ?? 0; @@ -2474,11 +2490,27 @@ export class LocalBackend { const rawRelTypes = mappedRelTypes && mappedRelTypes.length > 0 ? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t)) - : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS']; + : [ + 'CALLS', + 'IMPORTS', + 'EXTENDS', + 'IMPLEMENTS', + 'METHOD_OVERRIDES', + 'OVERRIDES', + 'METHOD_IMPLEMENTS', + ]; const relationTypes = rawRelTypes.length > 0 ? rawRelTypes - : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS']; + : [ + 'CALLS', + 'IMPORTS', + 'EXTENDS', + 'IMPLEMENTS', + 'METHOD_OVERRIDES', + 'OVERRIDES', + 'METHOD_IMPLEMENTS', + ]; try { return await this._runImpactBFS(repo, sym, symType, dir, { diff --git a/gitnexus/test/unit/mro-processor.test.ts b/gitnexus/test/unit/mro-processor.test.ts index 625249340..e9555cd0e 100644 --- a/gitnexus/test/unit/mro-processor.test.ts +++ b/gitnexus/test/unit/mro-processor.test.ts @@ -1139,5 +1139,127 @@ describe('computeMRO', () => { const fooEdge = mi.find((e) => e.sourceId === gbFoo); expect(fooEdge).toBeDefined(); }); + + it('C extends B extends A, B and A both have foo → returns B.foo (nearest)', () => { + // I { foo() }, A { foo() }, B extends A { foo() }, C extends B implements I { no foo } + const graph = createKnowledgeGraph(); + addClass(graph, 'I', 'java', 'Interface'); + addClass(graph, 'A', 'java'); + addClass(graph, 'B', 'java'); + addClass(graph, 'C', 'java'); + addImplements(graph, 'C', 'I'); + addExtends(graph, 'C', 'B'); + addExtends(graph, 'B', 'A'); + addMethod(graph, 'I', 'foo', 'Interface'); + addMethod(graph, 'A', 'foo'); + const bFoo = addMethod(graph, 'B', 'foo'); + // C has NO own foo — nearest is B.foo at depth 1 + + computeMRO(graph); + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + const fooEdge = mi.find((e) => e.sourceId === bFoo); + expect(fooEdge).toBeDefined(); + // A.foo should NOT be reached + const aFooId = generateId('Method', 'A.foo'); + const aFooEdge = mi.find((e) => e.sourceId === aFooId); + expect(aFooEdge).toBeUndefined(); + }); + + it('C extends B extends A, only A has foo → returns A.foo (single match at depth 2)', () => { + // I { foo() }, A { foo() }, B extends A { no foo }, C extends B implements I { no foo } + const graph = createKnowledgeGraph(); + addClass(graph, 'I', 'java', 'Interface'); + addClass(graph, 'A', 'java'); + addClass(graph, 'B', 'java'); + addClass(graph, 'C', 'java'); + addImplements(graph, 'C', 'I'); + addExtends(graph, 'C', 'B'); + addExtends(graph, 'B', 'A'); + addMethod(graph, 'I', 'foo', 'Interface'); + const aFoo = addMethod(graph, 'A', 'foo'); + // B has NO foo, C has NO foo — only A.foo at depth 2 + + computeMRO(graph); + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + const fooEdge = mi.find((e) => e.sourceId === aFoo); + expect(fooEdge).toBeDefined(); + }); + }); + + // ---- METHOD_IMPLEMENTS concrete-source guard ---------------------------- + describe('METHOD_IMPLEMENTS concrete-source guard', () => { + it('interface B extends interface A, B redeclares foo → 0 METHOD_IMPLEMENTS', () => { + const graph = createKnowledgeGraph(); + addClass(graph, 'A', 'java', 'Interface'); + addClass(graph, 'B', 'java', 'Interface'); + addInterfaceExtends(graph, 'B', 'A'); + addMethod(graph, 'A', 'foo', 'Interface'); + addMethod(graph, 'B', 'foo', 'Interface'); + + computeMRO(graph); + + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + expect(mi).toHaveLength(0); + }); + + it('abstract class C implements I, C has abstract foo → 0 METHOD_IMPLEMENTS for foo', () => { + const graph = createKnowledgeGraph(); + addClass(graph, 'I', 'java', 'Interface'); + addClass(graph, 'C', 'java', 'Class'); + addImplements(graph, 'C', 'I'); + addMethod(graph, 'I', 'foo', 'Interface'); + + // Add abstract method manually with isAbstract flag + const classId = generateId('Class', 'C'); + const methodId = generateId('Method', 'C.foo'); + graph.addNode({ + id: methodId, + label: 'Method', + properties: { name: 'foo', filePath: 'src/C.ts', isAbstract: true }, + }); + graph.addRelationship({ + id: generateId('HAS_METHOD', `${classId}->${methodId}`), + sourceId: classId, + targetId: methodId, + type: 'HAS_METHOD', + confidence: 1.0, + reason: '', + }); + + computeMRO(graph); + + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + expect(mi).toHaveLength(0); + }); + + it('abstract class C implements I, C has concrete bar → 1 METHOD_IMPLEMENTS for bar', () => { + const graph = createKnowledgeGraph(); + addClass(graph, 'I', 'java', 'Interface'); + addClass(graph, 'C', 'java', 'Class'); + addImplements(graph, 'C', 'I'); + addMethod(graph, 'I', 'bar', 'Interface'); + const cBar = addMethod(graph, 'C', 'bar'); + + computeMRO(graph); + + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + expect(mi).toHaveLength(1); + expect(mi[0].sourceId).toBe(cBar); + }); + + it('concrete class implements interface → 1 METHOD_IMPLEMENTS (regression)', () => { + const graph = createKnowledgeGraph(); + addClass(graph, 'I', 'java', 'Interface'); + addClass(graph, 'C', 'java', 'Class'); + addImplements(graph, 'C', 'I'); + addMethod(graph, 'I', 'foo', 'Interface'); + const cFoo = addMethod(graph, 'C', 'foo'); + + computeMRO(graph); + + const mi = graph.relationships.filter((r) => r.type === 'METHOD_IMPLEMENTS'); + expect(mi).toHaveLength(1); + expect(mi[0].sourceId).toBe(cFoo); + }); }); });