Claude finished @carlos-alm's task —— View job

PR Review — feat(resolver): backfill returnTypeMap/callAssignments for native engine (Phase 8.2 parity)

Root Cause Analysis

Problem Being Solved: The native Rust engine never runs extractReturnTypeMapWalk, leaving returnTypeMap and callAssignments as undefined on ExtractorOutput. The propagateReturnTypesAcrossFiles function in build-edges.ts guards on symbols.callAssignments?.length, which evaluates to falsy for undefined, so cross-file return-type propagation was silently skipped for all native-engine parse results.

Why This Approach: Rather than patching every downstream consumer to handle undefined defensively, the fix runs a TSC-backed backfill inside the already-existing enrichTypeMapWithTsc pass — the only place where both the TS compiler and fileSymbols are available together. The guard (returnTypeMap === undefined) is a clean sentinel that distinguishes native from WASM without needing an explicit engine flag. This is architecturally sound.

Risk Assessment: Low. The guard ensures WASM/JS paths are entirely unaffected. The new helpers touch only in-memory state; no DB I/O, no schema changes. The approach degrades gracefully (no-op) when typescript is unavailable.

Phase 2: Configuration Change Detection

No biome/lint rules relaxed, no timeouts increased, no TypeScript strict flags disabled, no module resolution changes. Clean.

Phase 3: Test Integrity

Tests are solid:

• 4 positive cases (function decls, qualified methods, arrow fns, callAssignments)
• 1 WASM no-overwrite guard (confirms the sentinel logic is bi-directional)
• 1 JS file skip (confirms isTsFile gate)

No weakened assertions. The WASM guard test (ts-resolver.test.ts:131) correctly asserts the exact same object reference (toBe(preExisting)) — strong. One note: the guard test uses confidence: 0.85 on a pre-existing entry and does not assert that enrichTypeMapWithTsc left it at 0.85 (it does not upgrade low-confidence entries in returnTypeMap when already set). This is correct behavior — the guard bails entirely when returnTypeMap !== undefined.

Phase 4: Code Quality

Concerns (minor to moderate):

1. enrichCallAssignments does not de-duplicate — ts-resolver.ts:385
callAssignments.push(...) is called unconditionally. If enrichTypeMapWithTsc were called twice on the same fileSymbols (e.g., in a future watch-mode scenario), entries would be duplicated. The WASM path clears and rebuilds during extraction, so it doesn't hit this. For the native path, the function is called once per build, so this is safe today — but fragile. A guard like if (!callAssignments.some(c => c.varName === varName)) or simply clearing callAssignments before push would make this robust.

2. Arrow function backfill may miss anonymous initializers — ts-resolver.ts:311–316
The code captures fnName = node.name.text from the VariableDeclaration's identifier. This correctly maps const makeWidget = () => ... → "makeWidget". However, it does not handle export const makeWidget = () => ... at module scope with destructuring patterns. This is a limitation of the existing enrichSourceFile approach (carried over), not a regression introduced here.

3. currentClass state tracking uses function-scoped mutation — ts-resolver.ts:289–299
The pattern const saved = currentClass; ... currentClass = saved; is a correct and idiomatic manual stack save for a recursive AST visitor. However, nested class expressions (rare but valid in TS) are handled correctly because the visitor recurses after the save/restore. Good.

4. Checker can be called after program goes out of scope in a future refactor
Currently checker is always used within the same call frame as program, so this is safe. Worth noting for any future refactor that might cache checker across calls.

5. callAssignments linked to returnTypeMap === undefined is a surprising coupling — ts-resolver.ts:125–131

if (symbols.returnTypeMap === undefined) {
  symbols.returnTypeMap = new Map();
  symbols.callAssignments = [];   // ← both are set together
  ...
}

The assumption is that both fields are either both defined (WASM) or both undefined (native). This matches the current ExtractorOutput contract. However, the type allows returnTypeMap and callAssignments to be independently undefined. If a future extractor sets returnTypeMap but not callAssignments, this branch skips enrichCallAssignments silently. A more robust guard would be:

if (symbols.returnTypeMap === undefined) {
  symbols.returnTypeMap = new Map();
  enrichReturnTypeMap(ts, sourceFile, checker, symbols.returnTypeMap);
}
if (symbols.callAssignments === undefined) {
  symbols.callAssignments = [];
  enrichCallAssignments(ts, sourceFile, symbols.typeMap, symbols.callAssignments);
}

6. backfilledFiles counter includes files where backfill ran but produced zero results
backfilledFiles is only incremented when returnTypeMap.size > 0 || callAssignments.length > 0 (line 130), which is correct. The debug log is accurate. ✓

Phase 5: Backlog Compliance

• Zero-dep: ✓ — No new runtime dependencies. Builds on the already-lazy typescript import established in Phase 8.1.
• Foundation-aligned: ✓ — Fully local, no cloud, no code generation, no agent behavior. Strengthens the core graph pipeline.
• Problem-fit: 5 — This directly reduces AI hallucinated edges. When the native engine parsed a TS codebase, cross-file return-type propagation was silently broken; call-edge accuracy was lower for native users. Fixing this means AI agents querying the graph see correct blast radius and dependency chains.
• Breaking: No — purely additive in-memory enrichment. API, CLI output, DB schema, and MCP tools are unchanged.
• Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Critical Concerns

None that block merge. Concern #5 (independent guards for returnTypeMap vs callAssignments) is the most actionable — a small defensive improvement worth considering before shipping.

Final Recommendation

## Root Cause Analysis
Problem Being Solved: Native engine left returnTypeMap/callAssignments undefined,
  silently disabling cross-file return-type propagation for all native-parsed TS files.
Why This Approach: Backfill inside the existing TSC pass is the right seam —
  both the compiler and fileSymbols are already available there.
Risk Assessment: Low — WASM/JS paths unaffected by the undefined sentinel guard;
  no DB changes; degrades gracefully without typescript package.

## Backlog Compliance
- Zero-dep: ✓
- Foundation-aligned: ✓
- Problem-fit: 5 — directly fixes silent accuracy regression for native-engine users
- Breaking: No
- Tier: 1

## Critical Concerns
- Minor: callAssignments push without dedup (safe today, fragile for future watch-mode)
- Minor: coupled guard for returnTypeMap+callAssignments could silently skip callAssignments
  if a future extractor sets one but not the other (consider independent guards)

## Final Recommendation
- Rating: ⭐⭐⭐⭐ (4/5)
- Action: APPROVE WITH CONDITIONS
- Reasoning: Correct fix, well-scoped, good test coverage, no regressions.
  Suggest addressing the coupled guard (concern #5 above) before or shortly after merge —
  either inline or as a follow-up issue — to keep the contract explicit as the extractor
  landscape grows.

Greptile Summary

This PR extends the Phase 8.1 TSC enrichment pass to backfill returnTypeMap and callAssignments for the native Rust engine, which leaves those fields undefined after extraction. Two new helpers — enrichReturnTypeMap and enrichCallAssignments — walk each TypeScript source file using the compiler API and populate these fields when they are absent, including Promise unwrapping for async functions.

• enrichReturnTypeMap captures module-level function declarations, class methods (qualified as ClassName.methodName), and top-level arrow/function-expression variable assignments; it stops body recursion at function and method boundaries to prevent local helpers from leaking into cross-file type resolution.
• enrichCallAssignments collects const x = fn() call-expression assignments not yet resolved in typeMap, using a two-pass deduplication strategy to exclude ambiguous variable names (same name, different callees) before pushing entries for cross-file propagation.
• Six new tests cover function declarations, qualified methods, arrow functions, callAssignment backfill, WASM no-overwrite, async unwrapping, and method-body exclusion.

Confidence Score: 3/5

The backfill logic has a gap in its recursion guard that allows local variable declarations inside constructor and accessor bodies to enter returnTypeMap under bare names, causing incorrect cross-file type resolution.

The body-descent stop in enrichReturnTypeMap covers FunctionDeclaration and MethodDeclaration but misses ConstructorDeclaration, GetAccessorDeclaration, and SetAccessorDeclaration. A local arrow function inside a constructor is matched by the VariableDeclaration branch and added to returnTypeMap under its bare name, exactly the cross-file pollution the doc comment says is prevented. The existing test only exercises a MethodDeclaration body so CI does not catch this gap.

src/domain/graph/resolver/ts-resolver.ts — the visit function inside enrichReturnTypeMap needs additional early-return guards for ConstructorDeclaration, GetAccessorDeclaration, and SetAccessorDeclaration.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[enrichTypeMapWithTsc] --> B{sourceFile found?}
    B -- No --> SKIP[skip file]
    B -- Yes --> C[enrichSourceFile typeMap Phase 8.1]
    C --> D{returnTypeMap === undefined?}
    D -- No JS/WASM path --> F
    D -- Yes native engine --> E[enrichReturnTypeMap walk SourceFile]
    E --> E1{node kind?}
    E1 -- FunctionDeclaration --> E2[writeEntry stop]
    E1 -- MethodDeclaration --> E3[writeEntry stop]
    E1 -- ClassDeclaration/Expression --> E4[enter class scope forEachChild stop]
    E1 -- VariableDeclaration arrow/fn init --> E5[writeEntry stop]
    E1 -- ConstructorDeclaration GetAccessor SetAccessor --> E6[forEachChild body descent NOT guarded]
    E1 -- other --> E7[forEachChild]
    E --> F{callAssignments === undefined?}
    F -- No --> G
    F -- Yes --> H[enrichCallAssignments two-pass dedup]
    H --> H1[collect candidates by varName]
    H1 --> H2[push only where uniqueCallees.size===1]
    H2 --> G[debug log backfilledFiles count]

Reviews (5): Last reviewed commit: "fix(resolver): exclude ambiguous callAss..." | Re-trigger Greptile

Codegraph Impact Analysis

7 functions changed → 9 callers affected across 3 files

• enrichTypeMapWithTsc in src/domain/graph/resolver/ts-resolver.ts:81 (3 transitive callers)
• enrichReturnTypeMap in src/domain/graph/resolver/ts-resolver.ts:305 (3 transitive callers)
• resolveReturnTypeName in src/domain/graph/resolver/ts-resolver.ts:317 (7 transitive callers)
• writeEntry in src/domain/graph/resolver/ts-resolver.ts:340 (7 transitive callers)
• visit in src/domain/graph/resolver/ts-resolver.ts:354 (7 transitive callers)
• enrichCallAssignments in src/domain/graph/resolver/ts-resolver.ts:412 (3 transitive callers)
• visit in src/domain/graph/resolver/ts-resolver.ts:421 (7 transitive callers)

Addressed Claude's feedback from the automated review:

Concern #5 (coupled returnTypeMap/callAssignments guard): Split into independent guards in ad9bf59. Each field now has its own === undefined check so a future extractor that sets one but not the other is handled correctly without silently skipping either enrichment pass.

Lint (format error): Fixed the long ternary in the debug log call — reformatted to wrap at 80 chars (formatter required the break).

Greptile P2 — async Promise: Addressed — see inline reply. resolveReturnTypeName now unwraps Promise<T> → T via checker.getTypeArguments.

Greptile P2 — local functions in returnTypeMap: Addressed — see inline reply. Visitor stops recursion at function/method body boundaries, with a test to confirm.

Concern #1 (callAssignments dedup): Not addressed — the push without dedup is safe today because the pass runs once per build. Adding dedup would require comparing all fields of CallAssignment objects. Given the current call-once contract, this is a valid future hardening but not a correctness issue today. If watch-mode ever calls this twice, the dedup logic should be added then.

@greptileai

@claude

Claude finished @carlos-alm's task —— View job

Follow-up Review

• Read current state of ts-resolver.ts (post-ad9bf59)
• Read current test file
• Verify all raised concerns were addressed
• Post review summary

Verification of Addressed Concerns

All concerns from the original review and both Greptile P2 issues have been properly addressed in ad9bf59. Here's what I found:

Concern #5 (coupled guard) — ✅ Fixed correctly

ts-resolver.ts:128–138 — now uses two independent === undefined guards:

if (symbols.returnTypeMap === undefined) {
  symbols.returnTypeMap = new Map();
  enrichReturnTypeMap(ts, sourceFile, checker, symbols.returnTypeMap);
  if (symbols.returnTypeMap.size > 0) didBackfill = true;
}
if (symbols.callAssignments === undefined) {
  symbols.callAssignments = [];
  enrichCallAssignments(ts, sourceFile, symbols.typeMap, symbols.callAssignments);
  if (symbols.callAssignments.length > 0) didBackfill = true;
}

Each field is handled independently. A future extractor that sets one but not the other is handled correctly.

Greptile P2 — Async Promise<T> unwrapping — ✅ Fixed correctly

ts-resolver.ts:312–333 — resolveReturnTypeName now unwraps Promise<T>:

const outerSym = retType.getSymbol() ?? retType.aliasSymbol;
if (outerSym?.getName() === 'Promise') {
  const args = checker.getTypeArguments(retType as import('typescript').TypeReference);
  if (args.length > 0) retType = args[0]!;
}

Test at line 154 confirms both async function fetchOrder(): Promise<Order> and async loadOrder(): Promise<Order> produce { type: 'Order', confidence: 1.0 }. Solid.

Greptile P2 — Local function body scoping — ✅ Fixed correctly

ts-resolver.ts:349–392 — the visitor now stops recursion at function/method body boundaries. isFunctionDeclaration and isMethodDeclaration branches both return after recording the entry (no ts.forEachChild into the body). isVariableDeclaration with arrow/function initializer also returns without recursing. Only "other node kinds" fall through to ts.forEachChild. Test at line 179 asserts helper (defined inside MyService.doWork) does NOT appear in returnTypeMap. This is the correct fix.

One small observation (non-blocking):

enrichCallAssignments at ts-resolver.ts:405–434 still recurses fully into function bodies via ts.forEachChild — there's no boundary stopping. This is consistent behavior since callAssignments are about variable assignments, and unlike returnTypeMap keys, varNames in callAssignments are not used as bare lookup keys in returnTypeMap — they're the target of resolution, not the source. The cross-file spurious match risk does not apply here. This is intentionally different from enrichReturnTypeMap and is correct.

Format fix — ✅

ts-resolver.ts:141–147 — the debug log ternary is correctly wrapped to stay within 80 chars.

Test Coverage Assessment

8 tests total (was 6, now 8 with the async and local-scope tests):

All originally-missing cases are now covered. The test for async functions uses both a module-level async function and an async method in a class, which exercises both code paths in enrichReturnTypeMap.

Final Assessment

All raised concerns have been addressed correctly. The implementation is sound, the guard split is clean, the async unwrapping is correct, and the scoping fix is verified by test. No remaining concerns.

Rating: ⭐⭐⭐⭐⭐ (5/5) — LGTM

View job

Fixed in 0b1740b: enrichCallAssignments now uses the same two-pass "unambiguous names only" heuristic as enrichSourceFile. First pass collects all {varName, calleeName} candidates; second pass only pushes entries where a given varName maps to exactly one distinct calleeName. Ambiguous bindings (same variable name, different callees across scopes) are excluded to prevent propagateReturnTypesAcrossFiles from silently resolving the wrong type. New test: 'excludes ambiguous callAssignments where same varName maps to different callees' verifies that const result = getA() in one method and const result = getB() in another produces zero entries in callAssignments for that varName.

@greptileai

@claude

Claude finished @carlos-alm's task —— View job

Follow-up Review — 0b1740b

• Gather context
• Read current state of enrichCallAssignments in ts-resolver.ts
• Read new test for ambiguous callAssignments
• Post final review

Verification of 0b1740b — Two-Pass Unambiguous Heuristic for enrichCallAssignments

Fix is correct and complete.

enrichCallAssignments at ts-resolver.ts:412–471 now mirrors the exact two-pass strategy from enrichSourceFile:

1. First pass (ts-resolver.ts:418–460): collect all { varName, calleeName, receiverTypeName } candidates into a Map<string, CallAssignment[]>, recurse freely into all scopes.
2. Second pass (ts-resolver.ts:462–469): only push entries where uniqueCallees.size === 1. If the same varName binds to two different callees across any scopes in the file, neither is pushed.

for (const entries of candidates.values()) {
  const uniqueCallees = new Set(entries.map((e) => e.calleeName));
  if (uniqueCallees.size === 1) {
    callAssignments.push(entries[0] as CallAssignment);
  }
}

This directly prevents the Greptile-flagged scenario where const result = getA() and const result = getB() in two different methods both landing in callAssignments, causing propagateReturnTypesAcrossFiles to silently pick the wrong type.

Test coverage — new test at ts-resolver.test.ts:131:

The test 'excludes ambiguous callAssignments where same varName maps to different callees' uses a file with:

• methodOne binding result = getA()
• methodTwo binding result = getB() (same varName, different callee)
• Module-level const unique = getUnique() (unambiguous)

Assertions:

• resultEntries has length 0 (ambiguous excluded) ✓
• uniqueEntry.calleeName === 'getUnique' (unambiguous included) ✓

This is the exact scenario that was problematic. The test is strong.

Full Concern Resolution Summary

Rating: ⭐⭐⭐⭐⭐ (5/5) — LGTM

All concerns raised in the original review and both Greptile P2 issues have been addressed. The implementation is correct, well-tested, and architecturally consistent with the existing enrichSourceFile strategy.

View job