From 8521189044e2d3d52d89d422fa741b04285ab5c6 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 3 Apr 2026 17:57:27 +0100 Subject: [PATCH] fix: adversarial review findings for METHOD_IMPLEMENTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address 3 findings from Codex adversarial review: 1. Legacy OVERRIDES backward compat: map OVERRIDES → METHOD_OVERRIDES at the _impactImpl API boundary before VALID_RELATION_TYPES filtering. Prevents silent traversal widening when clients send legacy type name. 2. Thread METHOD_IMPLEMENTS through MCP tools: - Added to VALID_RELATION_TYPES (impact traversal) - Added to IMPACT_RELATION_CONFIDENCE (0.85 floor) - Added to context() incoming/outgoing Cypher queries - Updated security + confidence tests 3. Transitive interface ancestor support: emitMethodImplementsEdges() now uses gatherAncestors() + buildTransitiveEdgeTypes() to walk the full inheritance chain. C implements B extends A (interface) now links C.foo to both B.foo and A.foo. Diamond paths deduplicated. 4 new unit tests covering transitive chains, inherited contracts, diamond dedup, and class-only exclusion. --- gitnexus/src/core/ingestion/mro-processor.ts | 51 ++++--- gitnexus/src/mcp/local/local-backend.ts | 25 +++- gitnexus/test/unit/impact-confidence.test.ts | 7 +- gitnexus/test/unit/mro-processor.test.ts | 134 +++++++++++++++++++ gitnexus/test/unit/security.test.ts | 3 +- 5 files changed, 191 insertions(+), 29 deletions(-) diff --git a/gitnexus/src/core/ingestion/mro-processor.ts b/gitnexus/src/core/ingestion/mro-processor.ts index 16fa0f666..10c915c32 100644 --- a/gitnexus/src/core/ingestion/mro-processor.ts +++ b/gitnexus/src/core/ingestion/mro-processor.ts @@ -488,42 +488,53 @@ function emitMethodImplementsEdges( bucket.push({ methodId, parameterTypes }); } - // For each parent, check if it's an interface/trait or connected via IMPLEMENTS - for (const parentId of parentIds) { - const parentNode = graph.getNode(parentId); - if (!parentNode) continue; + // Collect ALL transitive ancestors and classify each as EXTENDS or IMPLEMENTS + const allAncestors = gatherAncestors(classId, parentMap); + const ancestorEdgeTypes = buildTransitiveEdgeTypes(classId, parentMap, parentEdgeType); - const isInterfaceLike = parentNode.label === 'Interface' || parentNode.label === 'Trait'; - const edgeType = parentEdgeType.get(classId)?.get(parentId); - if (!isInterfaceLike && edgeType !== 'IMPLEMENTS') continue; + // Dedup set: avoid duplicate edges from diamond paths + const emitted = new Set(); - // Get parent's methods - const parentMethodIds = methodMap.get(parentId) ?? []; + // For each ancestor, check if it's an interface/trait or classified as IMPLEMENTS + for (const ancestorId of allAncestors) { + const ancestorNode = graph.getNode(ancestorId); + if (!ancestorNode) continue; - for (const parentMethodId of parentMethodIds) { - const parentMethodNode = graph.getNode(parentMethodId); - if (!parentMethodNode || parentMethodNode.label === 'Property') continue; + const isInterfaceLike = ancestorNode.label === 'Interface' || ancestorNode.label === 'Trait'; + const classifiedEdgeType = ancestorEdgeTypes.get(ancestorId); + if (!isInterfaceLike && classifiedEdgeType !== 'IMPLEMENTS') continue; - const parentName = parentMethodNode.properties.name as string; - const parentParamTypes = - (parentMethodNode.properties.parameterTypes as string[] | undefined) ?? []; + // Get ancestor's methods + const ancestorMethodIds = methodMap.get(ancestorId) ?? []; + + for (const ancestorMethodId of ancestorMethodIds) { + const ancestorMethodNode = graph.getNode(ancestorMethodId); + if (!ancestorMethodNode || ancestorMethodNode.label === 'Property') continue; + + const ancestorName = ancestorMethodNode.properties.name as string; + const ancestorParamTypes = + (ancestorMethodNode.properties.parameterTypes as string[] | undefined) ?? []; // Find matching method in own class by name + parameterTypes - const candidates = ownMethodsByName.get(parentName); + const candidates = ownMethodsByName.get(ancestorName); if (!candidates) continue; for (const candidate of candidates) { - if (parameterTypesMatch(candidate.parameterTypes, parentParamTypes)) { + if (parameterTypesMatch(candidate.parameterTypes, ancestorParamTypes)) { + const edgeKey = `${candidate.methodId}->${ancestorMethodId}`; + if (emitted.has(edgeKey)) break; + emitted.add(edgeKey); + graph.addRelationship({ - id: generateId('METHOD_IMPLEMENTS', `${candidate.methodId}->${parentMethodId}`), + id: generateId('METHOD_IMPLEMENTS', edgeKey), sourceId: candidate.methodId, - targetId: parentMethodId, + targetId: ancestorMethodId, type: 'METHOD_IMPLEMENTS', confidence: 1.0, reason: '', }); edgeCount++; - break; // first match wins for this parent method + break; // first match wins for this ancestor method } } } diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 00b5d4282..1d54cbb21 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -97,6 +97,7 @@ export const VALID_RELATION_TYPES = new Set([ 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', + 'METHOD_IMPLEMENTS', 'ACCESSES', 'HANDLES_ROUTE', 'FETCHES', @@ -117,7 +118,8 @@ export const VALID_RELATION_TYPES = new Set([ * CALLS / IMPORTS – direct, strongly-typed references → 0.9 * EXTENDS – class hierarchy, statically verifiable → 0.85 * IMPLEMENTS – interface contract, statically verifiable → 0.85 - * METHOD_OVERRIDES – method override, statically verifiable → 0.85 + * METHOD_OVERRIDES – method override, statically verifiable → 0.85 + * METHOD_IMPLEMENTS – interface method implementation, statically verifiable → 0.85 * HAS_METHOD – structural containment → 0.95 * HAS_PROPERTY – structural containment → 0.95 * ACCESSES – field read/write, may be indirect → 0.8 @@ -130,6 +132,7 @@ export const IMPACT_RELATION_CONFIDENCE: Readonly> = { EXTENDS: 0.85, IMPLEMENTS: 0.85, METHOD_OVERRIDES: 0.85, + METHOD_IMPLEMENTS: 0.85, HAS_METHOD: 0.95, HAS_PROPERTY: 0.95, ACCESSES: 0.8, @@ -1215,7 +1218,7 @@ export class LocalBackend { repo.id, ` MATCH (caller)-[r:CodeRelation]->(n {id: $symId}) - WHERE r.type IN ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', 'ACCESSES'] + WHERE r.type IN ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS', 'ACCESSES'] RETURN r.type AS relType, caller.id AS uid, caller.name AS name, caller.filePath AS filePath, labels(caller)[0] AS kind LIMIT 30 `, @@ -1305,7 +1308,7 @@ export class LocalBackend { repo.id, ` MATCH (n {id: $symId})-[r:CodeRelation]->(target) - WHERE r.type IN ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', 'ACCESSES'] + WHERE r.type IN ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS', 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', 'METHOD_IMPLEMENTS', 'ACCESSES'] RETURN r.type AS relType, target.id AS uid, target.name AS name, target.filePath AS filePath, labels(target)[0] AS kind LIMIT 30 `, @@ -1924,9 +1927,13 @@ export class LocalBackend { const { target, direction } = params; const maxDepth = params.maxDepth || 3; + // Map legacy relation type names before filtering (backward compat for OVERRIDES → METHOD_OVERRIDES) + const mappedRelTypes = params.relationTypes?.map((t: string) => + t === 'OVERRIDES' ? 'METHOD_OVERRIDES' : t, + ); const rawRelTypes = - params.relationTypes && params.relationTypes.length > 0 - ? params.relationTypes.filter((t) => VALID_RELATION_TYPES.has(t)) + mappedRelTypes && mappedRelTypes.length > 0 + ? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t)) : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS']; const relationTypes = rawRelTypes.length > 0 ? rawRelTypes : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS']; @@ -2472,9 +2479,13 @@ export class LocalBackend { const symType = typeof labelRaw === 'string' && labelRaw.trim().length > 0 ? labelRaw.trim() : ''; + // Map legacy relation type names (backward compat for OVERRIDES → METHOD_OVERRIDES) + const mappedRelTypes = opts.relationTypes?.map((t: string) => + t === 'OVERRIDES' ? 'METHOD_OVERRIDES' : t, + ); const rawRelTypes = - opts.relationTypes && opts.relationTypes.length > 0 - ? opts.relationTypes.filter((t) => VALID_RELATION_TYPES.has(t)) + mappedRelTypes && mappedRelTypes.length > 0 + ? mappedRelTypes.filter((t: string) => VALID_RELATION_TYPES.has(t)) : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS']; const relationTypes = rawRelTypes.length > 0 ? rawRelTypes : ['CALLS', 'IMPORTS', 'EXTENDS', 'IMPLEMENTS']; diff --git a/gitnexus/test/unit/impact-confidence.test.ts b/gitnexus/test/unit/impact-confidence.test.ts index b87356f63..d5ecefc0f 100644 --- a/gitnexus/test/unit/impact-confidence.test.ts +++ b/gitnexus/test/unit/impact-confidence.test.ts @@ -34,10 +34,14 @@ describe('IMPACT_RELATION_CONFIDENCE', () => { expect(IMPACT_RELATION_CONFIDENCE['IMPLEMENTS']).toBe(0.85); }); - it('OVERRIDES has confidence 0.85 (statically verifiable override)', () => { + it('METHOD_OVERRIDES has confidence 0.85 (statically verifiable override)', () => { expect(IMPACT_RELATION_CONFIDENCE['METHOD_OVERRIDES']).toBe(0.85); }); + it('METHOD_IMPLEMENTS has confidence 0.85 (statically verifiable implementation)', () => { + expect(IMPACT_RELATION_CONFIDENCE['METHOD_IMPLEMENTS']).toBe(0.85); + }); + it('HAS_METHOD has confidence 0.95 (structural containment)', () => { expect(IMPACT_RELATION_CONFIDENCE['HAS_METHOD']).toBe(0.95); }); @@ -77,6 +81,7 @@ describe('confidenceForRelType', () => { expect(confidenceForRelType('EXTENDS')).toBe(0.85); expect(confidenceForRelType('IMPLEMENTS')).toBe(0.85); expect(confidenceForRelType('METHOD_OVERRIDES')).toBe(0.85); + expect(confidenceForRelType('METHOD_IMPLEMENTS')).toBe(0.85); expect(confidenceForRelType('HAS_METHOD')).toBe(0.95); expect(confidenceForRelType('HAS_PROPERTY')).toBe(0.95); expect(confidenceForRelType('ACCESSES')).toBe(0.8); diff --git a/gitnexus/test/unit/mro-processor.test.ts b/gitnexus/test/unit/mro-processor.test.ts index 6126fb74a..9be1a9566 100644 --- a/gitnexus/test/unit/mro-processor.test.ts +++ b/gitnexus/test/unit/mro-processor.test.ts @@ -71,6 +71,25 @@ function addExtends( }); } +function addInterfaceExtends( + graph: KnowledgeGraph, + childName: string, + parentName: string, + childLabel: 'Interface' | 'Trait' = 'Interface', + parentLabel: 'Interface' | 'Trait' = 'Interface', +) { + const childId = generateId(childLabel, childName); + const parentId = generateId(parentLabel, parentName); + graph.addRelationship({ + id: generateId('EXTENDS', `${childId}->${parentId}`), + sourceId: childId, + targetId: parentId, + type: 'EXTENDS', + confidence: 1.0, + reason: '', + }); +} + function addImplements( graph: KnowledgeGraph, childName: string, @@ -714,6 +733,121 @@ describe('computeMRO', () => { expect(result.methodImplementsEdges).toBe(0); }); + describe('METHOD_IMPLEMENTS transitive ancestors', () => { + it('transitive interface chain: C.foo links to both B.foo and A.foo', () => { + // A (Interface) has foo, B (Interface) has foo extends A, C (Class) implements B + const graph = createKnowledgeGraph(); + addClass(graph, 'A', 'java', 'Interface'); + addClass(graph, 'B', 'java', 'Interface'); + addClass(graph, 'C', 'java'); + + addInterfaceExtends(graph, 'B', 'A'); + addImplements(graph, 'C', 'B'); + + const aFoo = addMethod(graph, 'A', 'foo', 'Interface'); + const bFoo = addMethod(graph, 'B', 'foo', 'Interface'); + addMethod(graph, 'C', 'foo'); + + const result = computeMRO(graph); + + const edges: any[] = []; + graph.forEachRelationship((rel) => { + if (rel.type === 'METHOD_IMPLEMENTS') edges.push(rel); + }); + + // C.foo should link to both B.foo and A.foo + expect(edges.some((e) => e.targetId === bFoo)).toBe(true); + expect(edges.some((e) => e.targetId === aFoo)).toBe(true); + expect(result.methodImplementsEdges).toBeGreaterThanOrEqual(2); + }); + + it('inherited contract method only on grandparent: C.bar links to A.bar', () => { + // A (Interface) has bar, B (Interface) extends A but has NO bar, C implements B + const graph = createKnowledgeGraph(); + addClass(graph, 'A', 'java', 'Interface'); + addClass(graph, 'B', 'java', 'Interface'); + addClass(graph, 'C', 'java'); + + addInterfaceExtends(graph, 'B', 'A'); + addImplements(graph, 'C', 'B'); + + const aBar = addMethod(graph, 'A', 'bar', 'Interface'); + // B has no bar method + addMethod(graph, 'C', 'bar'); + + const result = computeMRO(graph); + + const edges: any[] = []; + graph.forEachRelationship((rel) => { + if (rel.type === 'METHOD_IMPLEMENTS') edges.push(rel); + }); + + // C.bar should link to A.bar even though A is not a direct parent + expect(edges.some((e) => e.targetId === aBar)).toBe(true); + expect(result.methodImplementsEdges).toBeGreaterThanOrEqual(1); + }); + + it('diamond deduplication: E.foo gets exactly one edge to A.foo', () => { + // A (Interface) has foo + // B (Interface) has foo, extends A + // D (Interface) has foo, extends A + // E (Class) implements B and D + const graph = createKnowledgeGraph(); + addClass(graph, 'A', 'java', 'Interface'); + addClass(graph, 'B', 'java', 'Interface'); + addClass(graph, 'D', 'java', 'Interface'); + addClass(graph, 'E', 'java'); + + addInterfaceExtends(graph, 'B', 'A'); + addInterfaceExtends(graph, 'D', 'A'); + addImplements(graph, 'E', 'B'); + addImplements(graph, 'E', 'D'); + + const aFoo = addMethod(graph, 'A', 'foo', 'Interface'); + const bFoo = addMethod(graph, 'B', 'foo', 'Interface'); + const dFoo = addMethod(graph, 'D', 'foo', 'Interface'); + addMethod(graph, 'E', 'foo'); + + const result = computeMRO(graph); + + const eFoo = generateId('Method', 'E.foo'); + const edges: any[] = []; + graph.forEachRelationship((rel) => { + if (rel.type === 'METHOD_IMPLEMENTS') edges.push(rel); + }); + + // Filter to only edges FROM E.foo + const eFooEdges = edges.filter((e) => e.sourceId === eFoo); + + // E.foo should link to B.foo, D.foo, and exactly ONE A.foo (deduplicated) + expect(eFooEdges.filter((e) => e.targetId === bFoo)).toHaveLength(1); + expect(eFooEdges.filter((e) => e.targetId === dFoo)).toHaveLength(1); + expect(eFooEdges.filter((e) => e.targetId === aFoo)).toHaveLength(1); + // Total from E.foo: 3 edges (B.foo + D.foo + A.foo), not 4 + expect(eFooEdges).toHaveLength(3); + }); + + it('no transitive through class-only chain', () => { + // A (Class) has foo, B (Class) extends A has foo, C (Class) extends B has foo + const graph = createKnowledgeGraph(); + addClass(graph, 'A', 'java'); + addClass(graph, 'B', 'java'); + addClass(graph, 'C', 'java'); + + addExtends(graph, 'B', 'A'); + addExtends(graph, 'C', 'B'); + + addMethod(graph, 'A', 'foo'); + addMethod(graph, 'B', 'foo'); + addMethod(graph, 'C', 'foo'); + + const result = computeMRO(graph); + + // All class-extends, no interface involved → 0 METHOD_IMPLEMENTS edges + expect(result.methodImplementsEdges).toBe(0); + }); + }); + it('is queryable via MATCH pattern', () => { const graph = createKnowledgeGraph(); addClass(graph, 'IRepo', 'typescript', 'Interface'); diff --git a/gitnexus/test/unit/security.test.ts b/gitnexus/test/unit/security.test.ts index d17441ff3..ac6cb8d0e 100644 --- a/gitnexus/test/unit/security.test.ts +++ b/gitnexus/test/unit/security.test.ts @@ -106,7 +106,7 @@ describe('isWriteQuery', () => { describe('VALID_RELATION_TYPES', () => { it('contains all expected relation types', () => { - expect(VALID_RELATION_TYPES.size).toBe(13); + expect(VALID_RELATION_TYPES.size).toBe(14); for (const t of [ 'CALLS', 'IMPORTS', @@ -115,6 +115,7 @@ describe('VALID_RELATION_TYPES', () => { 'HAS_METHOD', 'HAS_PROPERTY', 'METHOD_OVERRIDES', + 'METHOD_IMPLEMENTS', 'ACCESSES', 'HANDLES_ROUTE', 'FETCHES',