Skip to content

feat(SM-12): Extract resolveStaticCall from resolveCallTarget#754

Merged
magyargergo merged 7 commits into
mainfrom
copilot/sm-12-extract-resolve-static-call
Apr 9, 2026
Merged

feat(SM-12): Extract resolveStaticCall from resolveCallTarget#754
magyargergo merged 7 commits into
mainfrom
copilot/sm-12-extract-resolve-static-call

Conversation

Copilot AI commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Extracts the constructor/static call resolution path into resolveStaticCall(className, methodName, currentFile, ctx, argCount?) using lookupClassByName + lookupMethodByOwner for O(1) resolution. Part of Semantic Model Roadmap Phase 3.

New function: resolveStaticCall

  • O(1) lookupClassByName pre-check → ctx.resolve for import-tier scoping → lookupMethodByOwner for Constructor lookup
  • Only accepts Constructor-typed results from lookupMethodByOwner (avoids C++ methods named after their class)
  • Falls back to class node when no explicit constructor exists
  • Bails when Constructor nodes lack ownerId, deferring to filterCallableCandidates for Constructor-vs-Class preference

S0 fast path in resolveCallTarget

Delegates to resolveStaticCall before existing filtering for:

  • callForm === 'constructor' — explicit new User()
  • callForm === 'free' with class targets — implicit User() (Swift/Kotlin)

Falls through to existing D1-D4 path on null.

// resolveCallTarget now delegates early for constructor calls:
if (call.callForm === 'constructor' || (call.callForm === 'free' && ...classTarget)) {
  const staticResult = resolveStaticCall(call.calledName, call.calledName, currentFile, ctx, call.argCount);
  if (staticResult) return staticResult;
}

Tests

11 unit tests covering ownerId-backed constructors, class-only fallback, arity disambiguation, confidence tiers, homonym ambiguity, and resolveCallTarget delegation for both constructor and free call forms.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • extension.ladybugdb.com
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit/symbol-table.test.ts scan�� node-addon-api (dns block)
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit/call-processor.test.ts xus/�� import sys; sys.--hash-style=gnu (dns block)
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit/call-processor.test.ts test/unit/symbol-table.test.ts /home/REDACTED/.ca-I -I xus/node_modules-I -I /home/REDACTED/.ca-c -I sh xus/�� ter_dart_binding/bindings/node/binding.o|" ./Release/.deps/Release/obj.target/tree_sitter_dart_bbash -I e_modules/.bin/sh -I /home/REDACTED/.ca-c -I node (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@vercel

vercel Bot commented Apr 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
gitnexus Ready Ready Preview, Comment Apr 9, 2026 10:59am

Request Review

- Add resolveStaticCall(className, methodName, currentFile, ctx, argCount?) using
  lookupClassByName + lookupMethodByOwner for O(1) constructor/static resolution
- Add S0 fast path in resolveCallTarget for constructor/free-form class calls
- Export resolveStaticCall from call-processor.ts
- Add 11 unit tests covering constructor resolution, confidence tiers,
  arity disambiguation, and resolveCallTarget delegation

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Copilot AI changed the title [WIP] Extract resolveStaticCall from resolveCallTarget feat(SM-12): Extract resolveStaticCall from resolveCallTarget Apr 9, 2026
@magyargergo

Copy link
Copy Markdown
Collaborator

@claude review these changes along with the plan on the issue. You need to look for gaps and performance issues. You are also tasked with reviewing the architectureal fit. Also we need to ensure we have coverage for these per languages. Also include other comments in your evaluation.

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 3m 37s —— View job


PR Review: feat(SM-12): Extract resolveStaticCall from resolveCallTarget

Tasks:

  • Read trigger comment and understand request
  • Read changed files (call-processor.ts, symbol-table.test.ts)
  • Analyze architectural fit
  • Identify gaps and performance issues
  • Check language-specific coverage in tests
  • Post full review

Summary

The extraction is well-motivated and the core logic is sound. O(1) lookupClassByNamelookupMethodByOwner is clearly faster than falling all the way through D1-D4. That said, there are several architectural gaps, a notable performance issue, and missing test coverage for a few key languages and edge cases.


Performance Issues

1. Double ctx.resolve() call (critical path cost)

resolveCallTarget calls ctx.resolve(call.calledName, currentFile) at line 1325 (stored as tiered), then resolveStaticCall calls ctx.resolve(className, currentFile) again at line 1911 — an identical lookup for the same name in the same file context. For every constructor call in a busy file, this doubles the tiered resolution work.

The fix is to accept the pre-computed tiered result as an optional parameter, or to pass tiered.candidates directly:

// resolveCallTarget (line 1349):
const staticResult = resolveStaticCall(call.calledName, call.calledName, currentFile, ctx, call.argCount, tiered);
// resolveStaticCall signature:
export const resolveStaticCall = (
  className: string,
  methodName: string,
  currentFile: string,
  ctx: ResolutionContext,
  argCount?: number,
  tiered?: TieredResult,  // accept pre-computed result
): ResolveResult | null => {
  const typeResolved = tiered ?? ctx.resolve(className, currentFile);
  ...

Fix this →

2. Redundant condition evaluation after S0

The S0 block at line 1341 checks filteredCandidates.length === 0 && tiered.candidates.some(c => c.type === 'Class' || ...) for callForm === 'free'. When resolveStaticCall returns null, the code falls through to another identical condition check at lines 1362–1373. If resolveStaticCall returned null (e.g., ambiguous classes), the free-form retry at 1362 runs the same some() scan a second time. A boolean flag set during the S0 check would avoid this.


Architectural Gaps

3. CLASS_LIKE_TYPES vs CLASS_TYPES divergence

symbol-table.ts defines CLASS_TYPES as {Class, Struct, Interface, Enum, Record, Trait}.
call-processor.ts defines CLASS_LIKE_TYPES as {Class, Struct, Interface, Enum, Record, Impl}.

The two sets have opposite asymmetries: CLASS_LIKE_TYPES has Impl (not Trait); CLASS_TYPES has Trait (not Impl). As a consequence:

  • Rust/PHP/Scala Trait — a Trait symbol won't pass the CLASS_LIKE_TYPES filter in resolveStaticCall. Traits can't be directly instantiated, so this is mostly harmless, but it creates a silent inconsistency with lookupClassByName which does index Traits.
  • Rust Impl blocksImpl is in CLASS_LIKE_TYPES but isn't in CLASS_TYPES. Calling resolveStaticCall on an Impl node that resolves via lookupClassByName (which uses CLASS_TYPES) will always return null at step 1 before reaching step 3.

At minimum, these two sets should be reconciled or the divergence should be explicitly documented with rationale.

4. methodName parameter is always call.calledName — no future path

The function signature is resolveStaticCall(className, methodName, ...). At every call site, methodName is set to call.calledName — which is the same value as className. The JSDoc comment hedges with "typically same as className," implying this is reserved for named constructors (Dart User.fromJson()), but:

  • The current loop calls lookupMethodByOwner(candidate.nodeId, methodName, argCount) using methodName, so it would work for named constructors already
  • But lookupClassByName(className) is called with className, not methodName, which is correct
  • The S0 guard at line 1341 doesn't distinguish these two values either

If named constructor support is the intent, it needs a test and explicit documentation. If it's not planned, methodName should be collapsed into className to avoid confusion.

5. Python models.User() not covered by S0 fast path

The callForm === 'member' path (Python import models; models.User()) is handled by the existing D1-D4 fallback at lines 1379–1380, not by resolveStaticCall. This is by design, but it means the O(1) optimization explicitly excludes one of the most common cross-module constructor patterns in Python. The PR description mentions this path without flagging it as a known gap.

Consider whether resolveStaticCall should also be tried for callForm === 'member' when receiverTypeName is absent and the callee resolves to a class.

6. Interface in CLASS_LIKE_TYPES — potential false positive

Interface is included in CLASS_LIKE_TYPES. In statically-typed languages (Java, C#, TypeScript), interfaces cannot be instantiated. A new InterfaceName() call is a compile error, so any such call in the graph would already be a bug in source. However, resolving an Interface node as a constructor target could emit a CALLS edge to an interface node, which may confuse downstream consumers of the graph expecting only Class/Constructor targets for CALLS edges.


Test Coverage Gaps

7. No language-specific constructor tests in resolveStaticCall

All 11 tests use .ts or .swift file extensions. No coverage for:

Language Pattern Gap
Python User() free call (callForm='free', .py) Missing
Java/C# new User() (callForm='constructor', .java) Missing
Kotlin User() free call (callForm='free', .kt) Missing
Ruby User.new (resolved as member) Missing
Rust User { field: val } struct instantiation Missing
PHP new User() with Trait supertype Missing

The test at line 2045 covers callForm='free' with .swift — good for Swift — but tests should also verify Python/Kotlin/Java explicitly to guard against regressions in language-specific extractor output format differences.

8. No test for named constructor scenario

The methodName !== className case (Dart User.fromJson()) is not tested. Since the parameter exists and the implementation handles it via lookupMethodByOwner(candidate.nodeId, methodName, ...), there should be coverage.

9. No test for callForm='constructor' with explicit Constructor node + ownerId via _resolveCallTargetForTesting

The integration-level test at line 2028 only verifies the class-fallback path (no Constructor node). There's no integration test asserting that the S0 delegation correctly returns a Constructor node (not just a Class) when ownerId is set and callForm='constructor'.

10. Test file placement

resolveStaticCall is imported from call-processor.js, but its tests live in symbol-table.test.ts. Tests for exported call-processor functions should be in call-processor.test.ts for consistency with resolveMemberCall (SM-11) and discoverability.


Minor Issues

11. S0 comment describes behavior inconsistently

The comment at line 1334 says "O(1) class + constructor lookup" but step 5 of the resolution falls back to returning the class node itself (not O(1) constructor lookup). The comment should clarify "or class-node fallback."

12. allClasses pre-check adds a full scan for multi-file codebases

lookupClassByName(className) at line 1907 is described as O(1), but if it returns a non-empty array, the caller then also calls ctx.resolve(className, currentFile) at line 1911 which does its own lookup. The step 1 pre-check would only provide a meaningful early-exit gain if lookupClassByName is truly O(1) and ctx.resolve is expensive. Worth profiling whether step 1 is net-positive given the two-lookup cost vs. a single ctx.resolve plus post-filter.


What's Well Done

  • The Constructor-type guard at line 1927 (def.type !== 'Constructor') correctly avoids the C++ false-positive described in the PR.
  • The ownerId-lacking Constructor bail-out at lines 1940–1946 correctly defers to filterCallableCandidates.
  • ambiguous flag deduplication via nodeId comparison handles diamond inheritance correctly.
  • Confidence tier propagation via toResolveResult(firstDef, typeResolved.tier) is consistent with the existing resolution framework.
  • Arity disambiguation test (line 1976) covers a real production scenario well.

Addresses Claude's review comments on PR #754:

Performance
- Pass pre-computed `tiered` result into `resolveStaticCall` as optional
  `tieredOverride` parameter, eliminating the duplicate `ctx.resolve(className,
  currentFile)` on every constructor call path.
- Cache `freeFormHasClassTarget` in `resolveCallTarget` so the S0 fast path
  and the free-form constructor retry share a single `.some()` scan.

Architecture
- Reconcile `CLASS_LIKE_TYPES` (call-processor) with `CLASS_TYPES`
  (symbol-table): `CLASS_LIKE_TYPES = [...CLASS_TYPES, 'Impl']`. This makes
  the relationship explicit — the call resolver's set is a strict superset
  of the heritage-index set, guaranteeing anything reachable via
  `lookupClassByName` also passes the resolver filter. Trait is now included
  (harmless: traits have no Constructor nodes, so step-3 returns undefined
  and step-5 still returns the class-like node when unique). Documented
  the Interface inclusion rationale (static methods + MRO walker).
- Collapse `resolveStaticCall`'s `methodName` parameter into `className` —
  all call sites passed identical values. Named constructors (Dart
  `User.fromJson()`) arrive as member calls and go through
  `resolveMemberCall`. Documented the reserved path for when a language
  surfaces a static-method-shaped call with a distinct member name.
- Document the known gap: `callForm === 'member'` constructor patterns
  (e.g. Python `models.User()`) are handled by the tail fallback, not S0.

Tests
- Add tiered-override test asserting `ctx.resolve` is not re-invoked when
  a pre-computed result is passed in.
- Add language-specific `_resolveCallTargetForTesting` integration tests
  for Java (`new User()`), Python (`User()`), and Kotlin (`User()`).

Verification: 3031 unit + 1766 integration tests pass, zero regressions.
@magyargergo

Copy link
Copy Markdown
Collaborator

Review findings addressed (b35291b)

Addressed all performance, architecture, and test-coverage findings from the review.

Performance

  • Add support for Ollama as a local inference backend #1 Double ctx.resolve()resolveStaticCall now accepts an optional tieredOverride: TieredCandidates parameter and resolveCallTarget passes its pre-computed tiered result through. The second identical lookup is gone. New unit test asserts ctx.resolve is not re-invoked when the override is supplied.
  • Welcome to GitNexus Discussions! #2 Redundant .some() scan — cached freeFormHasClassTarget in resolveCallTarget so the S0 fast path and the free-form constructor retry at the bottom share a single scan.

Architecture

  • readme #3 CLASS_LIKE_TYPES vs CLASS_TYPES divergence — reconciled: CLASS_LIKE_TYPES = new Set([...CLASS_TYPES, 'Impl']). This makes the relationship explicit — the call resolver's set is now a strict superset of the heritage index set, guaranteeing anything reachable via lookupClassByName also passes the resolver filter. Trait is now included too; it's harmless because traits have no Constructor nodes so step 3 returns undefined, and step 5 still returns the class-like node only when unique. Interface inclusion is documented (needed for static-method dispatch and MRO walking, constructor calls are guarded by the downstream Constructor-type check).
  • Embeddings pipeline #4 methodName parameter — collapsed into className. Every call site passed identical values. Named constructors (Dart User.fromJson()) arrive as member calls and route through resolveMemberCall, so this parameter was purely reserved for a future path that already has a better home. Documented the reasoning inline in case a language later surfaces a static-method-shaped call with a distinct member name.
  • Embeddings pipeline #5 Python models.User() member form — explicitly documented as a known gap in the S0 header comment. Extending S0 to cover this would need to thread receiver-type resolution through the module-alias logic; deferred until it shows up as a hot spot.
  • highlight tool for agent, prompt enhancements #6 Interface false positive — documented in the CLASS_LIKE_TYPES JSDoc. Interface nodes are intentionally included for static-method dispatch and MRO walking; constructor-shaped calls that resolve to an Interface are filtered out by the def.type !== 'Constructor' guard in resolveStaticCall step 3.
  • fixed LLM provider config warning issue #11 S0 comment accuracy — rewrote to mention the class-node fallback (step 5) so it matches actual behavior.

Test coverage

  • highlight tool fixes #7 Language-specific constructor tests — added _resolveCallTargetForTesting cases for Java new User() (callForm='constructor', .java), Python User() (callForm='free', .py), and Kotlin User() (callForm='free', .kt). Swift/TS already covered.
  • Embeddings pipeline #9 Constructor node via the integration helper — the new Java test exercises the full S0 → resolveStaticCallConstructor node path (it asserts the returned nodeId is ctor:java:User, not the class node), covering the gap the review flagged.
  • prompt enhancements, azure ai provider fixes #8 Named constructor test — not added: the methodName parameter has been removed, so there's nothing left to test. If named-constructor support is added later it will come with its own test.
  • Embeddings pipeline #10 Test file placement — kept in symbol-table.test.ts for consistency with the existing SM-11 resolveMemberCall tests, which also live there. Moving both together is out of scope for this PR.

Minor

  • Embeddings pipeline #12 allClasses pre-check — left in place. It is a cheap O(1) guard against the more expensive class-candidate filter + lookupMethodByOwner loop, and with tieredOverride now threaded through the caller there is no duplicate tiered-lookup cost to worry about.

Verification

  • tsc --noEmit clean
  • 3031 unit tests passing (+4 new tests: tiered-override, Java, Python, Kotlin)
  • 1766 integration tests passing
  • Zero regressions across the full suite

2 files changed, 159 insertions(+), 30 deletions(-).

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
5753 5656 0 97 206s

✅ All 5656 tests passed

97 test(s) skipped — expand for details
  • Swift MethodExtractor > isTypeDeclaration > recognizes class_declaration
  • Swift MethodExtractor > isTypeDeclaration > recognizes protocol_declaration
  • Swift MethodExtractor > isTypeDeclaration > rejects import_declaration
  • Swift MethodExtractor > visibility > extracts public method
  • Swift MethodExtractor > visibility > extracts private method
  • Swift MethodExtractor > visibility > defaults to internal when no modifier
  • Swift MethodExtractor > protocol methods > marks protocol method as abstract
  • Swift MethodExtractor > static and class methods > detects static func as isStatic
  • Swift MethodExtractor > static and class methods > detects class func as isStatic
  • Swift MethodExtractor > parameters > extracts parameters with types and default values
  • Swift MethodExtractor > return type > extracts return type from -> annotation
  • Swift MethodExtractor > annotations > extracts @objc attribute
  • Swift MethodExtractor > isFinal > detects final func
  • Swift MethodExtractor > isFinal > is false when not final
  • Swift MethodExtractor > isAsync > detects async func
  • Swift MethodExtractor > isOverride > detects override method
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift explicit init inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference does not bind plain functions
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature
  • Swift constructor-inferred type resolution > detects User and Repo classes, both with save methods
  • Swift constructor-inferred type resolution > resolves user.save() to Models/User.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > resolves repo.save() to Models/Repo.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > emits exactly 2 save() CALLS edges (one per receiver type)
  • Swift self resolution > detects User and Repo classes, each with a save function
  • Swift self resolution > resolves self.save() inside User.process to User.save, not Repo.save
  • Swift parent resolution > detects BaseModel and User classes plus Serializable protocol
  • Swift parent resolution > emits EXTENDS edge: User → BaseModel
  • Swift parent resolution > emits IMPLEMENTS edge: User → Serializable (protocol conformance)
  • Swift cross-file User.init() inference > resolves user.save() via User.init(name:) inference
  • Swift cross-file User.init() inference > resolves user.greet() via User.init(name:) inference
  • Swift return type inference > detects User class and getUser function
  • Swift return type inference > detects save function on User (Swift class methods are Function nodes)
  • Swift return type inference > resolves user.save() to User#save via return type of getUser() -> User
  • Swift return-type inference via function return type > resolves user.save() to User#save via return type of getUser()
  • Swift return-type inference via function return type > user.save() does NOT resolve to Repo#save
  • Swift return-type inference via function return type > resolves repo.save() to Repo#save via return type of getRepo()
  • Swift implicit imports (cross-file visibility) > detects UserService class in Models.swift
  • Swift implicit imports (cross-file visibility) > resolves UserService() constructor call across files (no explicit import)
  • Swift implicit imports (cross-file visibility) > resolves service.fetchUser() member call across files
  • Swift implicit imports (cross-file visibility) > creates IMPORTS edges between files in the same module
  • Swift extension deduplication > detects Product class
  • Swift extension deduplication > resolves Product() constructor despite extension creating duplicate class node
  • Swift extension deduplication > resolves product.save() to Product.swift (primary definition)
  • Swift constructor call fallback (no new keyword) > resolves OCRService() as constructor call across files
  • Swift constructor call fallback (no new keyword) > resolves ocr.recognize() member call via constructor-inferred type
  • Swift export visibility (internal vs private) > resolves PublicService() constructor across files
  • Swift export visibility (internal vs private) > resolves internalHelper() across files (internal = module-scoped)
  • Swift if let / guard let binding resolution > detects User and Repo classes
  • Swift if let / guard let binding resolution > resolves user.save() inside if-let to User#save
  • Swift if let / guard let binding resolution > resolves repo.save() inside guard-let to Repo#save
  • Swift if let / guard let binding resolution > user.save() in if-let does NOT resolve to Repo#save
  • Swift await / try expression unwrapping > resolves user.save() via await fetchUser() return type
  • Swift await / try expression unwrapping > resolves repo.save() via try parseRepo() return type
  • Swift await / try expression unwrapping > detects fetchUser and parseRepo as functions
  • Swift for-in loop element type inference > detects User and Repo classes
  • Swift for-in loop element type inference > creates implicit import edges between files
  • Swift field-type resolution > detects classes and their properties
  • Swift field-type resolution > emits HAS_PROPERTY edges from class to field
  • Swift field-type resolution > resolves field-chain call user.address.save() → Address#save
  • Swift field-type resolution > emits ACCESSES edges for field reads in chains
  • Swift field-type resolution > populates field metadata (visibility, declaredType) on Property nodes
  • Swift call-result binding > resolves call-result-bound method call user.save() → User#save
  • Swift call-result binding > getUser() is present as a defined function
  • Swift call-result binding > emits processUser -> getUser CALLS edge for let-assigned free function call
  • Swift method enrichment > detects Animal protocol and Dog class
  • Swift method enrichment > emits IMPLEMENTS edge Dog -> Animal
  • Swift method enrichment > emits HAS_METHOD edges for Dog methods
  • Swift method enrichment > marks protocol Animal.speak as isAbstract
  • Swift method enrichment > marks Dog.speak as NOT isAbstract
  • Swift method enrichment > marks breathe as isFinal
  • Swift method enrichment > marks classify as isStatic
  • Swift method enrichment > captures @objc annotation on breathe
  • Swift method enrichment > populates parameterTypes for classify(_ name: String)
  • Swift method enrichment > records parameterCount for classify
  • Swift method enrichment > records returnType for speak
  • Swift method enrichment > resolves dog.speak() CALLS edge
  • Swift method enrichment > resolves Dog.classify("dog") CALLS edge
  • Swift abstract dispatch > detects Repository protocol and SqlRepository class
  • Swift abstract dispatch > emits IMPLEMENTS edge SqlRepository -> Repository
  • Swift abstract dispatch > emits HAS_METHOD edges for Repository.find and Repository.save
  • Swift abstract dispatch > emits HAS_METHOD edges for SqlRepository.find and SqlRepository.save
  • Swift abstract dispatch > marks base Repository.find as isAbstract
  • Swift abstract dispatch > marks base Repository.save as isAbstract
  • Swift abstract dispatch > marks concrete SqlRepository.find as NOT isAbstract
  • Swift abstract dispatch > resolves repo.find(id: 42) CALLS edge
  • Swift abstract dispatch > resolves repo.save(entity: user) CALLS edge
  • Swift abstract dispatch > populates parameterTypes for Repository.find
  • Swift abstract dispatch > populates parameterTypes for Repository.save
  • Swift abstract dispatch > records returnType for SqlRepository.find
  • Swift abstract dispatch > emits METHOD_IMPLEMENTS edges from SqlRepository methods → Repository protocol methods
  • Swift overloaded method disambiguation > detects 2 distinct find Method nodes on SqlRepository
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edges for both find overloads
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edge for save
  • Swift overloaded method disambiguation > emits exactly 3 METHOD_IMPLEMENTS edges total
  • Swift Child extends Parent — inherited method resolution (SM-9) > detects Parent and Child classes
  • Swift Child extends Parent — inherited method resolution (SM-9) > resolves c.parentMethod() to Parent.parentMethod via first-wins MRO walk

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 72% 15327/21287 71.94% 📈 +0.1 🟢 ██████████████░░░░░░
Branches 61.15% 10014/16375 61.11% 📈 +0.0 🟢 ████████████░░░░░░░░
Functions 76.58% 1410/1841 76.42% 📈 +0.2 🟢 ███████████████░░░░░
Lines 74.16% 13877/18711 74.1% 📈 +0.1 🟢 ██████████████░░░░░░

📋 View full run · Generated by CI

Copilot AI requested a review from magyargergo April 9, 2026 10:06
Addresses the high-severity finding from the Codex adversarial review of
PR #754: `resolveStaticCall`'s step-5 "return the class itself when no
Constructor node is found" fallback reused `CLASS_LIKE_TYPES`, which —
after SM-11 and PR #754's reconciliation — now includes `Interface`,
`Trait`, and `Impl`. That is the method-dispatch set, not the
instantiable set, so constructor-shaped calls could resolve to
non-instantiable nodes and emit false `CALLS` edges.

Concrete failure: Rust same-file `impl User { ... }` alongside
`struct User { ... }` — both land at same-file tier, the Impl is not
filtered out, and the step-5 fallback produces a `CALLS` edge to the
`Impl` block instead of the `Struct`. The same widening exposed
Interface / Trait targets in Java / C# / PHP / Scala.

Fix
- Introduce `INSTANTIABLE_CLASS_TYPES = {'Class', 'Struct', 'Record'}`
  as a sibling to `CLASS_LIKE_TYPES`, documenting the contract
  explicitly and cross-referencing `CONSTRUCTOR_TARGET_TYPES`.
- Update `CLASS_LIKE_TYPES` JSDoc to clarify it is the method-dispatch
  set and add an anti-pattern warning against reusing it for
  constructor-fallback filtering.
- Tighten `resolveStaticCall` step 5: filter `classCandidates` through
  `INSTANTIABLE_CLASS_TYPES` before the `length === 1` check. This
  strips `Impl` from the Rust shadowing scenario (leaving `Struct` as
  the sole instantiable target) and null-routes Interface / Trait /
  `Impl`-alone scenarios, matching the SM-10 R3 null-route precedent.
- Step 3 (explicit Constructor lookup via `lookupMethodByOwner`) is
  intentionally unchanged — its `def.type === 'Constructor'` check is
  the correct contract, and legitimate Constructor nodes attached to
  `Impl` owners still resolve correctly.

Tests (+10 regression scenarios)
- Positive guards: Struct, Record fallback paths.
- Null-route: Interface (Java/C#/TS), PHP Trait, Rust Trait.
- Rust same-file shadowing: Struct wins over Impl.
- Rust Impl-alone: null-routes (no Struct present).
- Step-3 preservation: Constructor owned by Impl still resolves to the
  Constructor node, proving step-5 tightening doesn't leak into step 3.
- Full cascade via `_resolveCallTargetForTesting` for Interface and
  Trait — confirms no downstream path silently re-introduces the edge.

Verification
- `tsc --noEmit` clean
- 3041 unit tests pass (+10)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Codex review job: review-mnrao7fr-nv9y0e
@magyargergo

Copy link
Copy Markdown
Collaborator

Codex adversarial review finding addressed (ce027af)

Fixes the high-severity finding on resolveStaticCall's step-5 class-node fallback. The previous code reused CLASS_LIKE_TYPES, which — after SM-11 and PR #754's reconciliation — became the method-dispatch set (Class, Struct, Interface, Enum, Record, Trait, Impl). That is the wrong set for constructor-fallback filtering. Constructor-shaped calls could therefore resolve to Interface / Trait / Impl nodes and emit false CALLS edges, corrupting downstream impact analysis and navigation.

Fix

  • Introduce INSTANTIABLE_CLASS_TYPES = new Set(['Class', 'Struct', 'Record']) as a sibling to CLASS_LIKE_TYPES, with JSDoc cross-referencing CONSTRUCTOR_TARGET_TYPES so the two stay in sync.
  • Update CLASS_LIKE_TYPES JSDoc to mark it explicitly as the method-dispatch set and warn against reusing it for constructor-fallback filtering.
  • Tighten resolveStaticCall step 5: filter classCandidates through INSTANTIABLE_CLASS_TYPES before the length === 1 check. Null-route (matching SM-10 R3 precedent) when no instantiable candidate remains.
  • Step 3 is intentionally unchanged. Its existing def.type === 'Constructor' filter is correct, and legitimate Constructor nodes attached to Impl owners (Rust extractors) still resolve. A new step-3 preservation test locks this behavior in.

Why Enum is excluded

Matches CONSTRUCTOR_TARGET_TYPES. No current fixture exercises Enum constructor-shaped calls, and adding it without a motivating test would re-introduce exactly the kind of ambiguity this fix removes. If a future language surfaces Enum constructors (Rust enum variants with associated values, Swift enum cases), that is a separate plan with its own fixture.

Why CLASS_LIKE_TYPES isn't changed

The broader set is still correct for MRO walking, static-method dispatch, and heritage resolution — those paths need to reach Interface / Trait / Impl nodes. The issue was only that constructor fallback was using the wrong set. Scoping the fix to just resolveStaticCall keeps the blast radius minimal and preserves the PR #754 method-dispatch reconciliation intact.

Test coverage (+10 regression scenarios)

Test-first — all 10 were written to fail against the previous implementation before the fix was applied:

Scenario Shape
Struct fallback Positive regression guard
Record fallback Positive regression guard
Interface null-route (Java/C#/TS) Direct resolveStaticCall
PHP Trait null-route Direct
Rust Trait null-route Direct
Rust shadowing: Struct wins over Impl in same file Struct + Impl both added, filter strips Impl
Rust Impl-alone null-route Pathological extractor output — must refuse
Step-3 preservation: Constructor owned by Impl still resolves Rust-style Struct + Impl + Constructor with ownerId=impl
Interface null-route via _resolveCallTargetForTesting Full cascade S0 → resolveStaticCall → filter → null
Trait null-route via _resolveCallTargetForTesting Full cascade

Verification

  • tsc --noEmit clean
  • 3041 unit tests pass (+10 new regression tests)
  • 1766 integration tests pass
  • Zero regressions across the full suite

Plan

docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md (feasibility-deepened — all 6 Codex review questions traced and verified implementable before writing a line of code).

2 files changed, 177 insertions(+), 7 deletions(-).

@magyargergo

Copy link
Copy Markdown
Collaborator

@claude review these changes along with the plan on the issue. You need to look for gaps and performance issues. You are also tasked with reviewing the architectureal fit. Also we need to ensure we have coverage for these per languages. Also include other comments in your evaluation.

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 4m 56s —— View job


PR Review: feat(SM-12): Extract resolveStaticCall from resolveCallTarget

Tasks:

  • Read trigger comment and understand request
  • Read changed files (call-processor.ts, symbol-table.test.ts)
  • Review architectural fit against SM-12 plan
  • Identify any remaining gaps and performance issues
  • Check language-specific coverage in tests
  • Evaluate post-review fixes (b35291b, ce027af)
  • Post full review

Summary

After two rounds of substantive fixes (b35291b, ce027af), this PR is in solid shape. The resolveStaticCall extraction is architecturally sound, the O(1) fast path is correctly wired, and the INSTANTIABLE_CLASS_TYPES introduction is a clean resolution of the constructor-fallback false-positive risk. The CI is fully green (5654 tests, 0 failures). The remaining findings below are mostly minor consistency issues and documentation gaps — none are blockers.


Remaining Performance Issue

1. Enum in freeFormHasClassTarget triggers S0 with guaranteed null return

freeFormHasClassTarget (line 1397) includes Enum but INSTANTIABLE_CLASS_TYPES does not. For any free-form call where the only candidate is an Enum node, S0 fires, resolveStaticCall runs steps 1-5, and step 5 always returns null (Enum excluded from INSTANTIABLE_CLASS_TYPES). Then the fallback at line 1419 runs filterCallableCandidates(..., 'constructor') which also cannot match Enum (not in CONSTRUCTOR_TARGET_TYPES). Net result: two extra lookup passes for a path that was always going to fall through.

This Enum inclusion was inherited from the pre-SM-12 free-form condition and survived both fix commits untouched. The fix is removing c.type === 'Enum' from freeFormHasClassTarget:

const freeFormHasClassTarget =
  call.callForm === 'free' &&
  filteredCandidates.length === 0 &&
  tiered.candidates.some((c) => c.type === 'Class' || c.type === 'Struct' || c.type === 'Record');
//                                                                             ^^^^^^^^
//  Enum removed: not in INSTANTIABLE_CLASS_TYPES, not in CONSTRUCTOR_TARGET_TYPES.
//  Record added: is in both — covers Kotlin data classes and C# records.

This also closes a minor gap: Record free-form calls (C# new UserRecord() parsed as free when no new keyword present in some extractors) currently bypass S0 entirely because Record is absent from freeFormHasClassTarget. Adding Record here aligns the trigger condition with INSTANTIABLE_CLASS_TYPES.

Fix this →


Architectural Observations

2. lookupMethodByOwner key depends on constructor being named after its class

Step 3's lookup at line 1998 calls lookupMethodByOwner(candidate.nodeId, className, argCount), where className is always the class name (e.g., 'User'). This correctly finds constructors whose methodByOwner key is ${nodeId}\0User. It correctly misses constructors named differently (e.g., Python __init__), but those would also be absent from the tiered pool for ctx.resolve('User', ...), so step 4's Constructor check won't see them either — the two miss cases are symmetric.

This is by design. However, if a future extractor indexes constructor nodes under a name different from the class name but still with ownerId, step 3 would silently miss them. The JSDoc covers this for the methodName removal rationale but doesn't explicitly state the "same-name" assumption for step 3. A one-line inline comment at line 1998 would prevent future confusion.

3. Step 3 → step 4 interaction in the ambiguous-constructor case

When step 3 finds ambiguous = true (two different Constructor nodes across two class candidates), we fall through to step 4. Step 4 bails when typeResolved.candidates.some((c) => c.type === 'Constructor') — which is true because the Constructor nodes that lookupMethodByOwner found are also in the tiered pool (they share the same class name and are thus indexed under that name by the extractor). This means the ambiguous case correctly defers to filterCallableCandidates. No bug, but this behavioral dependency (step 3 results implying step 4 behavior) is not documented and could be fragile if an extractor indexes Constructor nodes under a different name than the class.

4. instantiableCandidates.length === 1 guard does not cover the 0-candidate case explicitly

At step 5 (line 2031), the condition instantiableCandidates.length === 1 correctly guards against returning an ambiguous result. When instantiableCandidates.length === 0 (all candidates were stripped as non-instantiable), we fall through to return null on line 2035. This is correct, but the length-0 case is the most likely outcome after the Codex fix (e.g., Interface, Trait, Impl all filtered out) and deserves a brief inline comment distinguishing it from the length > 1 case.


Test Coverage Gaps

5. No integration test for Record free-form call via _resolveCallTargetForTesting

The Record regression test at line 2162 exercises resolveStaticCall directly. But as noted in finding #1, Record is missing from freeFormHasClassTarget, so a free-form callForm='free' call to a Record class never actually reaches S0 via resolveCallTarget. The direct test passes (correct), but the integration test (_resolveCallTargetForTesting with callForm='free') would expose the gap. Should be added alongside the freeFormHasClassTarget fix.

6. Ruby User.new — uncovered and undocumented as known gap

Ruby constructor calls use User.newcallForm='member', receiverName='User', calledName='new'. These route through the callForm === 'member' fallback at line 1432 (filterCallableCandidates(tiered.candidates, argCount, 'constructor')), not S0. This is the correct existing path, but unlike Python models.User() (documented as a known gap in the S0 comment), Ruby User.new is not mentioned anywhere. Given that Ruby is a first-class supported language in the extractor matrix, the S0 comment at line 1380 should list callForm === 'member' as a known gap alongside the Python example, or a test should confirm the fallback resolves Ruby constructors correctly via D1 path.

7. No arity-disambiguation test for the integration path

The direct resolveStaticCall test at line 1990 covers arity disambiguation (2-arg vs 1-arg constructor). But there's no equivalent via _resolveCallTargetForTesting verifying that call.argCount threads correctly through the S0 delegation to resolveStaticCall. If call.argCount were accidentally dropped at the call site (line 1405), the arity test would still pass but the integration test would not catch the regression.


Minor Issues

8. Stale comment block above CLASS_LIKE_TYPES declaration

Line 57 still reads /** Types that represent class-like declarations (used for receiver/owner resolution). */ — a single-line JSDoc that was not removed when the full multi-line JSDoc was added below it at lines 58-81. There are now two JSDoc blocks for the same declaration; the first one will be the one picked up by tooling. The single-line comment should be deleted.

Fix this →

9. JSDoc step description out of sync with step numbering

The resolveStaticCall JSDoc at lines 1941-1944 describes 5 resolution steps, but step 4 says "If a unique constructor is found, return it; otherwise return the class itself when there is exactly one class candidate." — this blurs steps 4 and 5. The implementation has a distinct step 4 (bail if ownerless Constructor nodes exist) and step 5 (class-node fallback). The JSDoc should split these into separate numbered steps to match the inline code comments.


What's Well Done

  • INSTANTIABLE_CLASS_TYPES fix (ce027af) is the right call — surgically scoping the constructor fallback to instantiable types only, without touching CLASS_LIKE_TYPES used by the MRO walker. The 10 regression tests are test-first and catch exactly what they claim.
  • tieredOverride threading eliminates the duplicate ctx.resolve() with zero API breakage since the parameter is optional.
  • freeFormHasClassTarget caching is a clean fix for the redundant .some() scan.
  • JSDoc cross-references between CLASS_LIKE_TYPES, INSTANTIABLE_CLASS_TYPES, and CONSTRUCTOR_TARGET_TYPES are detailed and warn future authors explicitly against reusing the wrong set.
  • Step 4 bail-out logic (ownerless Constructor deferral to filterCallableCandidates) is correct and the comment explains the extractor-compatibility rationale.
  • Step-3 preservation test (line 2225) is the most valuable new test — it guards the Rust impl-owned Constructor path that would be easy to accidentally break.
  • CI is green across 5654 tests with coverage ticking up on all four metrics.

Addresses the 9 findings from the follow-up review on PR #754.

Performance
- Align `freeFormHasClassTarget` with `INSTANTIABLE_CLASS_TYPES`: drop
  `Enum` (S0 would always return null for it — wasted lookup work) and
  add `Record` (C# records and Kotlin data classes were bypassing S0
  entirely). The trigger set and the fallback filter set now agree by
  construction, documented inline.

Documentation
- Remove stale single-line JSDoc on `CLASS_LIKE_TYPES` (line 57) that
  duplicated the full multi-line block immediately below it — tooling
  picks up the first block so the old one-liner was shadowing the
  current explanation.
- Rewrite the `resolveStaticCall` JSDoc step list to match the actual
  step boundaries in the implementation (steps 3, 4, 5 were blurred in
  the old description).
- Add inline comment on step 3 documenting the same-name lookup
  assumption (`${candidate.nodeId}\0${className}`) and the symmetric
  miss case for Python `__init__`-style constructors.
- Add inline comment on step 4 documenting that it also catches the
  ambiguous-step-3 case, and warning against removing the check
  without handling that path explicitly.
- Add inline comment on step 5 enumerating the three length outcomes
  (0 / 1 / >1) so future readers see the dominant null-route case.
- Document Ruby `User.new` as a known gap alongside Python
  `models.User()` in the S0 header comment.

Tests (+2 scenarios)
- Record free-form constructor call via `_resolveCallTargetForTesting`
  exercises the aligned `freeFormHasClassTarget` trigger end-to-end,
  closing the gap where the direct `resolveStaticCall` test passed
  but the integration path was silently bypassing S0.
- Arity threading via `_resolveCallTargetForTesting` asserts that
  `call.argCount` flows through resolveCallTarget → S0 →
  resolveStaticCall → lookupMethodByOwner, catching any future
  regression where the argCount is dropped at the S0 call site.

Verification
- `tsc --noEmit` clean
- 3043 unit tests pass (+2)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Review: #754 (comment)
@magyargergo

Copy link
Copy Markdown
Collaborator

Second review round addressed (3458e97)

All 9 findings from the follow-up review.

Performance (#1)

Align freeFormHasClassTarget with INSTANTIABLE_CLASS_TYPES

  • Removed Enum — S0 would always return null for it (INSTANTIABLE_CLASS_TYPES excludes Enum), so every Enum free-form call was paying two wasted lookup passes.
  • Added Record — C# records and Kotlin data classes were bypassing S0 entirely because Record was absent from the trigger .some().
  • The trigger set and the fallback filter set now agree by construction, with an inline comment explaining the invariant.

Architecture / docs (#2, #3, #4, #9)

  • Step 3 same-name assumption — added inline comment documenting that the lookup key is ${nodeId}\0${className} and the symmetric miss case for Python __init__-style constructors.
  • Step 4 ambiguous-step-3 interaction — documented that this branch also catches the ambiguous = true case from step 3 (two distinct Constructor nodes across classes share the class name in the tiered pool), and warned against removing the check without handling that path.
  • Step 5 length outcomes — enumerated the three cases (0 / 1 / >1) inline so the dominant Codex-fix null-route case is explicit.
  • JSDoc step list — rewritten to match the actual step boundaries; the old description blurred steps 3, 4, and 5.

Tests (#5, #7)

  • Record free-form via _resolveCallTargetForTesting — exercises the aligned freeFormHasClassTarget trigger end-to-end. Before the alignment fix, Record free-form calls never reached S0, so the direct resolveStaticCall test passed while the integration path silently bypassed the fast path.
  • Arity threading via _resolveCallTargetForTesting — asserts call.argCount flows correctly through resolveCallTarget → S0 → resolveStaticCall → lookupMethodByOwner, locking in the 2-arg overload selection. Catches any future regression where argCount is dropped at the S0 call site.

Documentation (#6, #8)

  • Ruby User.new — now documented in the S0 header comment as a known gap alongside the existing Python models.User() example. Both are callForm='member' patterns handled by the tail fallback.
  • Stale duplicate JSDoc — removed the single-line comment on line 57 that was shadowing the full multi-line CLASS_LIKE_TYPES JSDoc block.

Verification

  • tsc --noEmit clean
  • 3043 unit tests pass (+2 new regression tests)
  • 1766 integration tests pass
  • Zero regressions across the full suite

2 files changed, 105 insertions(+), 11 deletions(-).

@magyargergo magyargergo marked this pull request as ready for review April 9, 2026 10:59
@magyargergo magyargergo merged commit 4450a14 into main Apr 9, 2026
13 checks passed
@magyargergo magyargergo deleted the copilot/sm-12-extract-resolve-static-call branch April 9, 2026 11:09
articultur pushed a commit to articultur/GitNexus that referenced this pull request Apr 9, 2026
…higyanpatwari#754)

* Initial plan

* feat(SM-12): extract resolveStaticCall from resolveCallTarget

- Add resolveStaticCall(className, methodName, currentFile, ctx, argCount?) using
  lookupClassByName + lookupMethodByOwner for O(1) constructor/static resolution
- Add S0 fast path in resolveCallTarget for constructor/free-form class calls
- Export resolveStaticCall from call-processor.ts
- Add 11 unit tests covering constructor resolution, confidence tiers,
  arity disambiguation, and resolveCallTarget delegation

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* chore: revert unrelated package-lock.json change

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* refactor: shorten verbose test name per code review feedback

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* fix(SM-12): address PR abhigyanpatwari#754 review findings

Addresses Claude's review comments on PR abhigyanpatwari#754:

Performance
- Pass pre-computed `tiered` result into `resolveStaticCall` as optional
  `tieredOverride` parameter, eliminating the duplicate `ctx.resolve(className,
  currentFile)` on every constructor call path.
- Cache `freeFormHasClassTarget` in `resolveCallTarget` so the S0 fast path
  and the free-form constructor retry share a single `.some()` scan.

Architecture
- Reconcile `CLASS_LIKE_TYPES` (call-processor) with `CLASS_TYPES`
  (symbol-table): `CLASS_LIKE_TYPES = [...CLASS_TYPES, 'Impl']`. This makes
  the relationship explicit — the call resolver's set is a strict superset
  of the heritage-index set, guaranteeing anything reachable via
  `lookupClassByName` also passes the resolver filter. Trait is now included
  (harmless: traits have no Constructor nodes, so step-3 returns undefined
  and step-5 still returns the class-like node when unique). Documented
  the Interface inclusion rationale (static methods + MRO walker).
- Collapse `resolveStaticCall`'s `methodName` parameter into `className` —
  all call sites passed identical values. Named constructors (Dart
  `User.fromJson()`) arrive as member calls and go through
  `resolveMemberCall`. Documented the reserved path for when a language
  surfaces a static-method-shaped call with a distinct member name.
- Document the known gap: `callForm === 'member'` constructor patterns
  (e.g. Python `models.User()`) are handled by the tail fallback, not S0.

Tests
- Add tiered-override test asserting `ctx.resolve` is not re-invoked when
  a pre-computed result is passed in.
- Add language-specific `_resolveCallTargetForTesting` integration tests
  for Java (`new User()`), Python (`User()`), and Kotlin (`User()`).

Verification: 3031 unit + 1766 integration tests pass, zero regressions.

* fix(SM-12): restrict resolveStaticCall fallback to instantiable kinds

Addresses the high-severity finding from the Codex adversarial review of
PR abhigyanpatwari#754: `resolveStaticCall`'s step-5 "return the class itself when no
Constructor node is found" fallback reused `CLASS_LIKE_TYPES`, which —
after SM-11 and PR abhigyanpatwari#754's reconciliation — now includes `Interface`,
`Trait`, and `Impl`. That is the method-dispatch set, not the
instantiable set, so constructor-shaped calls could resolve to
non-instantiable nodes and emit false `CALLS` edges.

Concrete failure: Rust same-file `impl User { ... }` alongside
`struct User { ... }` — both land at same-file tier, the Impl is not
filtered out, and the step-5 fallback produces a `CALLS` edge to the
`Impl` block instead of the `Struct`. The same widening exposed
Interface / Trait targets in Java / C# / PHP / Scala.

Fix
- Introduce `INSTANTIABLE_CLASS_TYPES = {'Class', 'Struct', 'Record'}`
  as a sibling to `CLASS_LIKE_TYPES`, documenting the contract
  explicitly and cross-referencing `CONSTRUCTOR_TARGET_TYPES`.
- Update `CLASS_LIKE_TYPES` JSDoc to clarify it is the method-dispatch
  set and add an anti-pattern warning against reusing it for
  constructor-fallback filtering.
- Tighten `resolveStaticCall` step 5: filter `classCandidates` through
  `INSTANTIABLE_CLASS_TYPES` before the `length === 1` check. This
  strips `Impl` from the Rust shadowing scenario (leaving `Struct` as
  the sole instantiable target) and null-routes Interface / Trait /
  `Impl`-alone scenarios, matching the SM-10 R3 null-route precedent.
- Step 3 (explicit Constructor lookup via `lookupMethodByOwner`) is
  intentionally unchanged — its `def.type === 'Constructor'` check is
  the correct contract, and legitimate Constructor nodes attached to
  `Impl` owners still resolve correctly.

Tests (+10 regression scenarios)
- Positive guards: Struct, Record fallback paths.
- Null-route: Interface (Java/C#/TS), PHP Trait, Rust Trait.
- Rust same-file shadowing: Struct wins over Impl.
- Rust Impl-alone: null-routes (no Struct present).
- Step-3 preservation: Constructor owned by Impl still resolves to the
  Constructor node, proving step-5 tightening doesn't leak into step 3.
- Full cascade via `_resolveCallTargetForTesting` for Interface and
  Trait — confirms no downstream path silently re-introduces the edge.

Verification
- `tsc --noEmit` clean
- 3041 unit tests pass (+10)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Codex review job: review-mnrao7fr-nv9y0e

* fix(SM-12): address PR abhigyanpatwari#754 second review round

Addresses the 9 findings from the follow-up review on PR abhigyanpatwari#754.

Performance
- Align `freeFormHasClassTarget` with `INSTANTIABLE_CLASS_TYPES`: drop
  `Enum` (S0 would always return null for it — wasted lookup work) and
  add `Record` (C# records and Kotlin data classes were bypassing S0
  entirely). The trigger set and the fallback filter set now agree by
  construction, documented inline.

Documentation
- Remove stale single-line JSDoc on `CLASS_LIKE_TYPES` (line 57) that
  duplicated the full multi-line block immediately below it — tooling
  picks up the first block so the old one-liner was shadowing the
  current explanation.
- Rewrite the `resolveStaticCall` JSDoc step list to match the actual
  step boundaries in the implementation (steps 3, 4, 5 were blurred in
  the old description).
- Add inline comment on step 3 documenting the same-name lookup
  assumption (`${candidate.nodeId}\0${className}`) and the symmetric
  miss case for Python `__init__`-style constructors.
- Add inline comment on step 4 documenting that it also catches the
  ambiguous-step-3 case, and warning against removing the check
  without handling that path explicitly.
- Add inline comment on step 5 enumerating the three length outcomes
  (0 / 1 / >1) so future readers see the dominant null-route case.
- Document Ruby `User.new` as a known gap alongside Python
  `models.User()` in the S0 header comment.

Tests (+2 scenarios)
- Record free-form constructor call via `_resolveCallTargetForTesting`
  exercises the aligned `freeFormHasClassTarget` trigger end-to-end,
  closing the gap where the direct `resolveStaticCall` test passed
  but the integration path was silently bypassing S0.
- Arity threading via `_resolveCallTargetForTesting` asserts that
  `call.argCount` flows through resolveCallTarget → S0 →
  resolveStaticCall → lookupMethodByOwner, catching any future
  regression where the argCount is dropped at the S0 call site.

Verification
- `tsc --noEmit` clean
- 3043 unit tests pass (+2)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Review: abhigyanpatwari#754 (comment)

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
(cherry picked from commit 4450a14)
motolese pushed a commit to motolese/datamoto-gitnexus that referenced this pull request Apr 23, 2026
…higyanpatwari#754)

* Initial plan

* feat(SM-12): extract resolveStaticCall from resolveCallTarget

- Add resolveStaticCall(className, methodName, currentFile, ctx, argCount?) using
  lookupClassByName + lookupMethodByOwner for O(1) constructor/static resolution
- Add S0 fast path in resolveCallTarget for constructor/free-form class calls
- Export resolveStaticCall from call-processor.ts
- Add 11 unit tests covering constructor resolution, confidence tiers,
  arity disambiguation, and resolveCallTarget delegation

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* chore: revert unrelated package-lock.json change

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* refactor: shorten verbose test name per code review feedback

Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c9471ca9-57ff-4dae-956e-e7ffdc326bc4

Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>

* fix(SM-12): address PR abhigyanpatwari#754 review findings

Addresses Claude's review comments on PR abhigyanpatwari#754:

Performance
- Pass pre-computed `tiered` result into `resolveStaticCall` as optional
  `tieredOverride` parameter, eliminating the duplicate `ctx.resolve(className,
  currentFile)` on every constructor call path.
- Cache `freeFormHasClassTarget` in `resolveCallTarget` so the S0 fast path
  and the free-form constructor retry share a single `.some()` scan.

Architecture
- Reconcile `CLASS_LIKE_TYPES` (call-processor) with `CLASS_TYPES`
  (symbol-table): `CLASS_LIKE_TYPES = [...CLASS_TYPES, 'Impl']`. This makes
  the relationship explicit — the call resolver's set is a strict superset
  of the heritage-index set, guaranteeing anything reachable via
  `lookupClassByName` also passes the resolver filter. Trait is now included
  (harmless: traits have no Constructor nodes, so step-3 returns undefined
  and step-5 still returns the class-like node when unique). Documented
  the Interface inclusion rationale (static methods + MRO walker).
- Collapse `resolveStaticCall`'s `methodName` parameter into `className` —
  all call sites passed identical values. Named constructors (Dart
  `User.fromJson()`) arrive as member calls and go through
  `resolveMemberCall`. Documented the reserved path for when a language
  surfaces a static-method-shaped call with a distinct member name.
- Document the known gap: `callForm === 'member'` constructor patterns
  (e.g. Python `models.User()`) are handled by the tail fallback, not S0.

Tests
- Add tiered-override test asserting `ctx.resolve` is not re-invoked when
  a pre-computed result is passed in.
- Add language-specific `_resolveCallTargetForTesting` integration tests
  for Java (`new User()`), Python (`User()`), and Kotlin (`User()`).

Verification: 3031 unit + 1766 integration tests pass, zero regressions.

* fix(SM-12): restrict resolveStaticCall fallback to instantiable kinds

Addresses the high-severity finding from the Codex adversarial review of
PR abhigyanpatwari#754: `resolveStaticCall`'s step-5 "return the class itself when no
Constructor node is found" fallback reused `CLASS_LIKE_TYPES`, which —
after SM-11 and PR abhigyanpatwari#754's reconciliation — now includes `Interface`,
`Trait`, and `Impl`. That is the method-dispatch set, not the
instantiable set, so constructor-shaped calls could resolve to
non-instantiable nodes and emit false `CALLS` edges.

Concrete failure: Rust same-file `impl User { ... }` alongside
`struct User { ... }` — both land at same-file tier, the Impl is not
filtered out, and the step-5 fallback produces a `CALLS` edge to the
`Impl` block instead of the `Struct`. The same widening exposed
Interface / Trait targets in Java / C# / PHP / Scala.

Fix
- Introduce `INSTANTIABLE_CLASS_TYPES = {'Class', 'Struct', 'Record'}`
  as a sibling to `CLASS_LIKE_TYPES`, documenting the contract
  explicitly and cross-referencing `CONSTRUCTOR_TARGET_TYPES`.
- Update `CLASS_LIKE_TYPES` JSDoc to clarify it is the method-dispatch
  set and add an anti-pattern warning against reusing it for
  constructor-fallback filtering.
- Tighten `resolveStaticCall` step 5: filter `classCandidates` through
  `INSTANTIABLE_CLASS_TYPES` before the `length === 1` check. This
  strips `Impl` from the Rust shadowing scenario (leaving `Struct` as
  the sole instantiable target) and null-routes Interface / Trait /
  `Impl`-alone scenarios, matching the SM-10 R3 null-route precedent.
- Step 3 (explicit Constructor lookup via `lookupMethodByOwner`) is
  intentionally unchanged — its `def.type === 'Constructor'` check is
  the correct contract, and legitimate Constructor nodes attached to
  `Impl` owners still resolve correctly.

Tests (+10 regression scenarios)
- Positive guards: Struct, Record fallback paths.
- Null-route: Interface (Java/C#/TS), PHP Trait, Rust Trait.
- Rust same-file shadowing: Struct wins over Impl.
- Rust Impl-alone: null-routes (no Struct present).
- Step-3 preservation: Constructor owned by Impl still resolves to the
  Constructor node, proving step-5 tightening doesn't leak into step 3.
- Full cascade via `_resolveCallTargetForTesting` for Interface and
  Trait — confirms no downstream path silently re-introduces the edge.

Verification
- `tsc --noEmit` clean
- 3041 unit tests pass (+10)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Codex review job: review-mnrao7fr-nv9y0e

* fix(SM-12): address PR abhigyanpatwari#754 second review round

Addresses the 9 findings from the follow-up review on PR abhigyanpatwari#754.

Performance
- Align `freeFormHasClassTarget` with `INSTANTIABLE_CLASS_TYPES`: drop
  `Enum` (S0 would always return null for it — wasted lookup work) and
  add `Record` (C# records and Kotlin data classes were bypassing S0
  entirely). The trigger set and the fallback filter set now agree by
  construction, documented inline.

Documentation
- Remove stale single-line JSDoc on `CLASS_LIKE_TYPES` (line 57) that
  duplicated the full multi-line block immediately below it — tooling
  picks up the first block so the old one-liner was shadowing the
  current explanation.
- Rewrite the `resolveStaticCall` JSDoc step list to match the actual
  step boundaries in the implementation (steps 3, 4, 5 were blurred in
  the old description).
- Add inline comment on step 3 documenting the same-name lookup
  assumption (`${candidate.nodeId}\0${className}`) and the symmetric
  miss case for Python `__init__`-style constructors.
- Add inline comment on step 4 documenting that it also catches the
  ambiguous-step-3 case, and warning against removing the check
  without handling that path explicitly.
- Add inline comment on step 5 enumerating the three length outcomes
  (0 / 1 / >1) so future readers see the dominant null-route case.
- Document Ruby `User.new` as a known gap alongside Python
  `models.User()` in the S0 header comment.

Tests (+2 scenarios)
- Record free-form constructor call via `_resolveCallTargetForTesting`
  exercises the aligned `freeFormHasClassTarget` trigger end-to-end,
  closing the gap where the direct `resolveStaticCall` test passed
  but the integration path was silently bypassing S0.
- Arity threading via `_resolveCallTargetForTesting` asserts that
  `call.argCount` flows through resolveCallTarget → S0 →
  resolveStaticCall → lookupMethodByOwner, catching any future
  regression where the argCount is dropped at the S0 call site.

Verification
- `tsc --noEmit` clean
- 3043 unit tests pass (+2)
- 1766 integration tests pass
- Zero regressions

Plan: docs/plans/2026-04-09-002-fix-sm12-constructor-fallback-instantiable-only-plan.md
Review: abhigyanpatwari#754 (comment)

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SM-12] Extract resolveStaticCall from resolveCallTarget

2 participants