scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-09-11 22:53:04 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

refactor(SM-19): address code review findings with clean-code pass

Browse source 
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
This commit is contained in:
Gergo Magyar 2026-04-11 10:10:00 +01:00
parent f424685e83
commit d641033997
3 changed files with 296 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

158
gitnexus/src/core/ingestion/call-processor.ts
View file

@ -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;
}

200
gitnexus/test/unit/call-processor.test.ts
View file

@ -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';

13
gitnexus/test/unit/symbol-table.test.ts
View file

@ -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',