From d6410339978d36140aa3f4ee1fbe209302b9cdd3 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sat, 11 Apr 2026 10:10:00 +0100 Subject: [PATCH] refactor(SM-19): address code review findings with clean-code pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review on commit f424685e surfaced one P1 correctness regression and two P2 maintainability concerns. This commit closes all ten findings: P1 — Alias helper placement regression - resolveModuleAliasedCall now runs as a FALLBACK in the typed-member branch, after resolveMemberCall/resolveMemberCallByFile return null. Previously it short-circuited BEFORE scoped resolvers, leaking unrelated homonyms from the aliased file when a local var coincidentally matched a module alias. - Added type-file verification guard: alias narrowing only fires when the alias target file is among the receiver type's defining files. Prevents cross-type false positives and hardens SM-10 R3. P2 — Thin-dispatcher drift (roadmap Phase 3) - Extracted disambiguateByOverloadOrArgTypes shared helper. Centralizes the overloadHints → preComputedArgTypes precedence rule used by both member and constructor resolvers. - Folded constructor overload disambiguation into resolveStaticCall as step 4.5 (between the ambiguous-pool bail and the instantiable-class fallback). resolveStaticCall now accepts optional overloadHints / preComputedArgTypes symmetric with resolveMemberCallByFile. - Dispatcher's constructor branch returns to a 2-line delegation. - resolveMemberCallByFile now calls the shared helper instead of inlining the ternary. P2 — Missing test coverage - owner-scoped wins over alias narrowing (alias with unrelated target class must not override unique owner-scoped answer) - alias narrowing rejects unrelated target type (type-file guard) - alias fallthrough: receiverName not in alias map - alias fallthrough: alias target file has no matching method (overloadHints-for-constructor variant transitively covered via the extracted helper's member-path tests; direct dispatcher test deferred as it requires real OverloadHints fixture parsing) P3 — Clarity and durability - Stripped "Codex SM-19 Finding N" prefixes from comments. Replaced with durable explanations of WHY each guarded branch exists. - Added cross-reference comment at the tail-branch resolveModuleAliasedCall call site pointing to the typed-member branch usage. Verification: 3195 unit + 1766 resolver integration + 2398 full integration tests pass. tsc --noEmit clean. prettier clean. Plan: docs/plans/2026-04-11-002-fix-sm19-code-review-findings-plan.md --- gitnexus/src/core/ingestion/call-processor.ts | 158 +++++++++----- gitnexus/test/unit/call-processor.test.ts | 200 ++++++++++++++++-- gitnexus/test/unit/symbol-table.test.ts | 13 +- 3 files changed, 296 insertions(+), 75 deletions(-) diff --git a/gitnexus/src/core/ingestion/call-processor.ts b/gitnexus/src/core/ingestion/call-processor.ts index e61c48808..5edd72737 100644 --- a/gitnexus/src/core/ingestion/call-processor.ts +++ b/gitnexus/src/core/ingestion/call-processor.ts @@ -1465,6 +1465,31 @@ const tryOverloadDisambiguation = ( return matchCandidatesByArgTypes(candidates, argTypes); }; +/** + * Apply overload-hint or arg-type disambiguation to a pre-filtered candidate + * pool. Returns the unique survivor, or null when neither signal is present, + * neither can disambiguate, or the pool remains ambiguous. + * + * Precedence rule: `overloadHints` wins over `preComputedArgTypes` when both + * are supplied. The AST-based disambiguator has access to live type inference + * hooks, whereas `preComputedArgTypes` is a worker-path pre-computation that + * may be coarser-grained. + * + * Single source of truth for the narrowing-signal precedence used by member + * and constructor resolution paths. Add a new narrowing signal here once, not + * at each call site. + */ +const disambiguateByOverloadOrArgTypes = ( + pool: SymbolDefinition[], + overloadHints: OverloadHints | undefined, + preComputedArgTypes: (string | undefined)[] | undefined, +): SymbolDefinition | null => { + if (!overloadHints && !preComputedArgTypes) return null; + if (overloadHints) return tryOverloadDisambiguation(pool, overloadHints); + if (preComputedArgTypes) return matchCandidatesByArgTypes(pool, preComputedArgTypes); + return null; +}; + /** * Collapse Swift-extension duplicate Class/Struct candidates to the primary * definition, preferring the shortest file path. @@ -1611,11 +1636,11 @@ const resolveMemberCallByFile = ( // Overload disambiguation on the narrowed pool if (fileFiltered.length > 1 || ownerFiltered.length > 1) { const overloadPool = ownerFiltered.length > 1 ? ownerFiltered : fileFiltered; - const disambiguated = overloadHints - ? tryOverloadDisambiguation(overloadPool, overloadHints) - : preComputedArgTypes - ? matchCandidatesByArgTypes(overloadPool, preComputedArgTypes) - : null; + const disambiguated = disambiguateByOverloadOrArgTypes( + overloadPool, + overloadHints, + preComputedArgTypes, + ); if (disambiguated) return toResolveResult(disambiguated, typeResolved.tier); } @@ -1687,43 +1712,19 @@ const resolveCallTarget = ( ); } if (call.callForm === 'constructor') { - const staticResult = resolveStaticCall( - call.calledName, - currentFile, - ctx, - call.argCount, - tiered, + return ( + resolveStaticCall( + call.calledName, + currentFile, + ctx, + call.argCount, + tiered, + overloadHints, + preComputedArgTypes, + ) ?? singleCandidate(tiered, call.argCount, 'constructor') ); - if (staticResult) return staticResult; - - // Codex SM-19 Finding 2: When `resolveStaticCall` bails on ambiguous or - // ownerless Constructor pools, give overload/arg-type disambiguation a - // chance before null-routing. Only engages when the caller supplied a - // narrowing signal — preserves SM-10 R3 for genuinely ambiguous cases. - if (overloadHints || preComputedArgTypes) { - const ctorPool = filterCallableCandidates(tiered.candidates, call.argCount, 'constructor'); - if (ctorPool.length > 1) { - const disambiguated = overloadHints - ? tryOverloadDisambiguation(ctorPool, overloadHints) - : preComputedArgTypes - ? matchCandidatesByArgTypes(ctorPool, preComputedArgTypes) - : null; - if (disambiguated) return toResolveResult(disambiguated, tiered.tier); - } - } - return singleCandidate(tiered, call.argCount, 'constructor'); } if (call.receiverTypeName) { - // Codex SM-19 Finding 1: Consult module-alias narrowing BEFORE the - // owner-scoped / file-scoped resolvers. When the caller imports two - // homonym receiver types from different files, import-scoped tiering - // does not narrow (both files are in scope) and the owner/file fallback - // sees genuine ambiguity. An active module alias on `call.receiverName` - // is the only remaining disambiguation signal; without this call the - // dispatcher null-routes silently and drops a valid CALLS edge. - const aliasResult = resolveModuleAliasedCall(call, currentFile, ctx, widenCache, tiered); - if (aliasResult) return aliasResult; - // Skip the owner-scoped MRO path when the tiered pool has genuine // overload ambiguity that needs D1-D4+E handling, not D0. const skipMember = @@ -1753,26 +1754,48 @@ const resolveCallTarget = ( ); if (memberResult) return memberResult; - // singleCandidate tail fallback — but only when the receiver type - // did NOT resolve to any indexed type. This reproduces the old - // resolveCallTarget's D1-D4 null-route guard (SM-10 R3): when the - // type IS in the index but file/owner filtering produced zero - // matches, that's a genuine miss and we must null-route rather than - // fall through to an unscoped singleCandidate that ignores the - // receiver's class hierarchy. + // Module-alias narrowing runs as a FALLBACK, after owner/file-scoped + // resolvers have returned null. This ordering is load-bearing: placing + // alias narrowing first would short-circuit unique owner-scoped answers + // when a local variable coincidentally matches an alias name, leaking + // unrelated homonyms from the aliased file onto the wrong receiver type. // - // When the type is NOT in the index (e.g. PHP 'mixed', dynamic - // types, unresolvable aliases), the scoped resolvers had nothing to - // work with and singleCandidate is the correct last resort — it - // picks the globally-unique candidate if one exists. - // - // ctx.resolve is cached per (name, file) pair, so this call is free. + // The type-file verification guard is load-bearing for SM-10 R3: an + // alias is only a VALID narrowing signal when the alias target file is + // among the receiver type's defining files. If the alias points at a + // file that does not hold `receiverTypeName`, any candidate we would + // pick from there would belong to an unrelated class — a cross-type + // false positive. ctx.resolve is cached per (name, file), so resolving + // the receiver type a second time here is free. const typeResolves = ctx.resolve(call.receiverTypeName, currentFile); + const aliasMap = ctx.moduleAliasMap?.get(currentFile); + const aliasTargetFile = + call.receiverName && aliasMap ? aliasMap.get(call.receiverName) : undefined; + if ( + aliasTargetFile && + typeResolves && + typeResolves.candidates.some((c) => c.filePath === aliasTargetFile) + ) { + const aliasResult = resolveModuleAliasedCall(call, currentFile, ctx, widenCache, tiered); + if (aliasResult) return aliasResult; + } + + // SM-10 R3 null-route: when the receiver type resolves to indexed types + // but no scoped resolver (nor the guarded alias fallback) produced a + // match, that's a genuine miss — refuse to emit a CALLS edge rather + // than guess via an unscoped singleCandidate that ignores the class + // hierarchy. When the type is NOT in the index (PHP `mixed`, dynamic + // types, unresolvable aliases), the scoped resolvers had nothing to + // work with and singleCandidate is the correct last resort. if (typeResolves && typeResolves.candidates.length > 0) { return null; // null-route: type resolved, no candidate matched } return singleCandidate(tiered, call.argCount, call.callForm); } + // Member call with no inferred receiver type — e.g. Python `mod.fn()` + // where `mod` is a module alias. Module-alias narrowing is the primary + // disambiguation signal here. Also consulted from the typed-member + // branch above as a guarded fallback after owner/file-scoped resolvers. return ( resolveModuleAliasedCall(call, currentFile, ctx, widenCache, tiered) ?? singleCandidate(tiered, call.argCount, call.callForm) @@ -1790,9 +1813,6 @@ const resolveCallTarget = ( // classes (e.g. User.save@100 and Repo.save@200 are distinct keys). // Lookup uses a secondary funcName-only index built in lookupReceiverType. -/** Extract the function name from a scope key ("funcName@startIndex" → "funcName"). */ -const extractFuncNameFromScope = (scope: string): string => scope.slice(0, scope.indexOf('@')); - /** Extract the bare function name from a sourceId. * Handles both unqualified ("Function:filepath:funcName" → "funcName") * and qualified ("Function:filepath:ClassName.funcName" → "funcName"). @@ -2236,6 +2256,8 @@ export const resolveStaticCall = ( ctx: ResolutionContext, argCount?: number, tieredOverride?: TieredCandidates, + overloadHints?: OverloadHints, + preComputedArgTypes?: (string | undefined)[], ): ResolveResult | null => { // 1. Pre-check: does a class with this name exist at all? (O(1)) // This guards against the expensive `ctx.resolve` walk when the name @@ -2297,10 +2319,30 @@ export const resolveStaticCall = ( // with two distinct Constructor nodes across multiple class candidates): // the same Constructor nodes are indexed under the class name in the // tiered pool, so `.some(Constructor)` is true here and we defer to - // `filterCallableCandidates` downstream rather than guess which overload - // to pick. Do not remove this check without also handling the ambiguous - // step-3 path explicitly. + // step 4.5 (overload/arg-type disambiguation) or the caller's fallback. + // Do not remove this check without also handling the ambiguous step-3 + // path explicitly. if (typeResolved.candidates.some((c) => c.type === 'Constructor')) { + // 4.5. Overload / arg-type disambiguation for ambiguous or ownerless + // Constructor pools. When the caller supplied a narrowing signal + // (AST-based overload hints from the sequential path, or pre- + // computed arg types from the worker path), give disambiguation a + // chance before null-routing. Symmetric with resolveMemberCallByFile's + // disambiguation pass — both resolvers now share the same signal + // precedence via disambiguateByOverloadOrArgTypes. Only fires when + // at least one narrowing signal is present; preserves SM-10 R3 for + // genuinely ambiguous cases with no disambiguating input. + if (overloadHints || preComputedArgTypes) { + const ctorPool = filterCallableCandidates(typeResolved.candidates, argCount, 'constructor'); + if (ctorPool.length > 1) { + const disambiguated = disambiguateByOverloadOrArgTypes( + ctorPool, + overloadHints, + preComputedArgTypes, + ); + if (disambiguated) return toResolveResult(disambiguated, typeResolved.tier); + } + } return null; } diff --git a/gitnexus/test/unit/call-processor.test.ts b/gitnexus/test/unit/call-processor.test.ts index 96b97f858..e69250ceb 100644 --- a/gitnexus/test/unit/call-processor.test.ts +++ b/gitnexus/test/unit/call-processor.test.ts @@ -2495,11 +2495,12 @@ describe('processCalls — D0 MRO fast path (SM-10)', () => { }); it('module-alias guard (real homonym): both files imported, alias narrows typed member call to aliased file', async () => { - // Codex SM-19 adversarial review Finding 1: When BOTH homonym files are - // imported by the caller, import-scoped tiering no longer narrows the - // tiered pool — the dispatcher sees two `save` candidates. Module-alias - // narrowing is the only remaining disambiguation signal. The typed-member - // branch must consult the alias map or null-route silently. + // When both homonym files are imported by the caller, import-scoped + // tiering no longer narrows the tiered pool — the dispatcher sees two + // `save` candidates. Module-alias narrowing is the only remaining + // disambiguation signal. The typed-member branch must consult the alias + // map (as a guarded fallback after owner/file-scoped resolvers fail) or + // null-route silently. const authModFile = 'src/auth_mod.py'; const userModFile = 'src/user_mod.py'; const appFile = 'src/app.py'; @@ -2548,14 +2549,189 @@ describe('processCalls — D0 MRO fast path (SM-10)', () => { expect(rels[0].targetId).toBe(authSaveId); }); + it('owner-scoped wins over alias narrowing: unique owner-scoped answer beats coincidental alias on unrelated file', async () => { + // Receiver type `User` has exactly one definition, in models.py. Module + // alias `auth → auth.py` exists (because the caller also imports auth.py + // for its own reasons), and auth.py contains an unrelated `Widget` class + // with a homonym `save` method. The caller has `receiverName='auth'` + // (e.g., a local variable coincidentally named `auth`), + // `receiverTypeName='User'`. Owner-scoped resolution must win — alias + // narrowing must not short-circuit a unique correct answer with an + // unrelated homonym from the aliased file. + const modelsFile = 'src/models.py'; + const authFile = 'src/auth.py'; + const appFile = 'src/app.py'; + const modelsUserId = 'class:src/models.py:User'; + const authWidgetId = 'class:src/auth.py:Widget'; + const modelsSaveId = 'method:src/models.py:User:save'; + const authSaveId = 'method:src/auth.py:Widget:save'; + + ctx.symbols.add(modelsFile, 'User', modelsUserId, 'Class'); + ctx.symbols.add(authFile, 'Widget', authWidgetId, 'Class'); + ctx.symbols.add(modelsFile, 'save', modelsSaveId, 'Method', { + ownerId: modelsUserId, + returnType: 'None', + }); + ctx.symbols.add(authFile, 'save', authSaveId, 'Method', { + ownerId: authWidgetId, + returnType: 'None', + }); + ctx.importMap.set(appFile, new Set([modelsFile, authFile])); + ctx.moduleAliasMap.set(appFile, new Map([['auth', authFile]])); + + const calls: ExtractedCall[] = [ + { + filePath: appFile, + calledName: 'save', + sourceId: 'Function:src/app.py:run', + argCount: 1, + callForm: 'member', + receiverName: 'auth', + receiverTypeName: 'User', + }, + ]; + + await processCallsFromExtracted(graph, calls, ctx); + + const rels = graph.relationships.filter((r) => r.type === 'CALLS'); + // Owner-scoped runs first and uniquely resolves User.save to models.py. + // Alias narrowing never fires because the scoped resolver already won. + expect(rels).toHaveLength(1); + expect(rels[0].targetId).toBe(modelsSaveId); + }); + + it('alias narrowing rejects unrelated target type: null-route when alias file does not hold receiver type', async () => { + // Receiver type `User` lives only in models.py, but has no `save` method + // defined. Alias `auth → auth.py`, and auth.py contains an unrelated + // `Widget.save`. Owner-scoped and file-scoped resolvers return null (no + // save on User). Without the type-file verification guard, alias + // narrowing would pick auth.py's `Widget.save` — a cross-type false + // positive. With the guard, auth.py is not in the receiver type's + // defining-files set (which is {models.py}), so alias narrowing bails + // and SM-10 R3 null-routes. + const modelsFile = 'src/models.py'; + const authFile = 'src/auth.py'; + const appFile = 'src/app.py'; + const modelsUserId = 'class:src/models.py:User'; + const authWidgetId = 'class:src/auth.py:Widget'; + const authSaveId = 'method:src/auth.py:Widget:save'; + + ctx.symbols.add(modelsFile, 'User', modelsUserId, 'Class'); + ctx.symbols.add(authFile, 'Widget', authWidgetId, 'Class'); + // NO save on User — deliberately absent to force null-route. + ctx.symbols.add(authFile, 'save', authSaveId, 'Method', { + ownerId: authWidgetId, + returnType: 'None', + }); + ctx.importMap.set(appFile, new Set([modelsFile, authFile])); + ctx.moduleAliasMap.set(appFile, new Map([['auth', authFile]])); + + const calls: ExtractedCall[] = [ + { + filePath: appFile, + calledName: 'save', + sourceId: 'Function:src/app.py:run', + argCount: 1, + callForm: 'member', + receiverName: 'auth', + receiverTypeName: 'User', + }, + ]; + + await processCallsFromExtracted(graph, calls, ctx); + + const rels = graph.relationships.filter((r) => r.type === 'CALLS'); + // Null-route: no CALLS edge. The type-file guard prevented the alias + // from leaking auth.py's Widget.save onto a User-typed receiver. + expect(rels).toHaveLength(0); + }); + + it('alias fallthrough: receiverName not in alias map falls through to owner-scoped resolver', async () => { + // Receiver variable `user` does NOT match any alias entry (alias only + // covers `auth`). Owner-scoped resolution must run to completion and + // pick models.py's User.save — the alias helper's early-bail must not + // interfere with unrelated typed member calls. This exercises the 99% + // hot path where alias narrowing is irrelevant. + const modelsFile = 'src/models.py'; + const authFile = 'src/auth.py'; + const appFile = 'src/app.py'; + const modelsUserId = 'class:src/models.py:User'; + const modelsSaveId = 'method:src/models.py:User:save'; + + ctx.symbols.add(modelsFile, 'User', modelsUserId, 'Class'); + ctx.symbols.add(modelsFile, 'save', modelsSaveId, 'Method', { + ownerId: modelsUserId, + returnType: 'None', + }); + ctx.importMap.set(appFile, new Set([modelsFile, authFile])); + ctx.moduleAliasMap.set(appFile, new Map([['auth', authFile]])); + + const calls: ExtractedCall[] = [ + { + filePath: appFile, + calledName: 'save', + sourceId: 'Function:src/app.py:run', + argCount: 0, + callForm: 'member', + receiverName: 'user', // NOT 'auth' — no alias match + receiverTypeName: 'User', + }, + ]; + + await processCallsFromExtracted(graph, calls, ctx); + + const rels = graph.relationships.filter((r) => r.type === 'CALLS'); + expect(rels).toHaveLength(1); + expect(rels[0].targetId).toBe(modelsSaveId); + }); + + it('alias fallthrough: alias target file has no matching method falls through to owner-scoped', async () => { + // Alias `auth → empty.py` where empty.py exists in the import map but + // has no `save` method at all. Owner-scoped finds models.py's User.save + // uniquely. Even if the type-file guard let alias narrowing fire (it + // won't, because empty.py isn't in the receiver type's files), the + // helper would return null and resolution must still succeed. + const modelsFile = 'src/models.py'; + const emptyFile = 'src/empty.py'; + const appFile = 'src/app.py'; + const modelsUserId = 'class:src/models.py:User'; + const modelsSaveId = 'method:src/models.py:User:save'; + + ctx.symbols.add(modelsFile, 'User', modelsUserId, 'Class'); + ctx.symbols.add(modelsFile, 'save', modelsSaveId, 'Method', { + ownerId: modelsUserId, + returnType: 'None', + }); + // empty.py: no symbols at all. + ctx.importMap.set(appFile, new Set([modelsFile, emptyFile])); + ctx.moduleAliasMap.set(appFile, new Map([['auth', emptyFile]])); + + const calls: ExtractedCall[] = [ + { + filePath: appFile, + calledName: 'save', + sourceId: 'Function:src/app.py:run', + argCount: 0, + callForm: 'member', + receiverName: 'auth', + receiverTypeName: 'User', + }, + ]; + + await processCallsFromExtracted(graph, calls, ctx); + + const rels = graph.relationships.filter((r) => r.type === 'CALLS'); + expect(rels).toHaveLength(1); + expect(rels[0].targetId).toBe(modelsSaveId); + }); + it('constructor overload disambiguation: same-arity ownerless constructors picked via preComputedArgTypes', async () => { - // Codex SM-19 adversarial review Finding 2: When two homonym constructors - // across different files have the same arity but different parameter types, - // `resolveStaticCall` correctly bails (step 3 ambiguity → step 4 bail because - // the tiered pool contains Constructor nodes). Before this fix the dispatcher - // then fell through to `singleCandidate` which also bailed because two - // constructors survive arity filtering. With overload disambiguation after - // `resolveStaticCall`, `preComputedArgTypes` picks the string overload. + // When two homonym constructors across different files have the same + // arity but different parameter types, `resolveStaticCall` correctly + // bails (step 3 ambiguity → step 4 bail because the tiered pool contains + // Constructor nodes). Step 4.5 then runs overload/arg-type disambiguation + // on the constructor-filtered pool, picking the string overload when the + // caller supplies matching `argTypes` / `preComputedArgTypes`. const userFile = 'src/models/User.ts'; const repoFile = 'src/models/Repo.ts'; const appFile = 'src/app.ts'; diff --git a/gitnexus/test/unit/symbol-table.test.ts b/gitnexus/test/unit/symbol-table.test.ts index a8af3ccdb..0f12851d6 100644 --- a/gitnexus/test/unit/symbol-table.test.ts +++ b/gitnexus/test/unit/symbol-table.test.ts @@ -1757,11 +1757,14 @@ describe('resolveCallTarget thin dispatcher (SM-19)', () => { // Python-style: `import auth; auth.User.save()` where BOTH auth.py and // other.py define a `User` class with a `save` method. // - // Codex SM-19 adversarial review Finding 1: the thin dispatcher must - // consult module-alias narrowing for typed member calls BEFORE falling - // through to owner/file-scoped resolvers. With both homonym files - // imported, owner-scoped resolution sees genuine ambiguity and the only - // remaining disambiguation signal is the alias on `call.receiverName`. + // When both homonym files are imported, owner-scoped resolution sees + // genuine ambiguity (both `User` classes own a `save` method) and the + // only remaining disambiguation signal is the module alias on + // `call.receiverName`. The dispatcher consults alias narrowing as a + // guarded fallback after owner/file-scoped resolvers return null; the + // type-file verification guard requires the alias target file to be + // among the receiver type's defining files before alias narrowing is + // considered a valid signal. ctx.symbols.add('src/auth.py', 'User', 'class:auth:User', 'Class'); ctx.symbols.add('src/auth.py', 'save', 'method:auth:User:save', 'Method', { returnType: 'None',