fix(native): promote this-dispatch scope tests from todo to active

[carlos-alm]

Summary

• Removes the if (engine === 'native') guard that was skipping three negative assertions with .todo in this-dispatch-scope.test.ts
• The class-scoped this-dispatch fix landed in edge_builder.rs in fix(edge_builder): restrict same-file this-dispatch fallback to caller's own class #1343 (commit 16af0e0), and the prototype extraction was added to the Rust extractor before v3.11.2; CI compiles the native addon from source on every PR so the binary used in tests already includes both fixes
• All four assertions now run for both wasm and native engines

Closes #1413

[claude]

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

---

PR Review — fix(native): promote this-dispatch scope tests from todo to active

• Gather context and read changed files
• Phase 1: Root cause analysis
• Phase 2: Configuration change detection
• Phase 3: Test integrity assessment
• Phase 4: Code quality review
• Phase 5: Backlog compliance
• Final assessment

---

Root Cause Analysis

Problem Being Solved: Three test assertions in this-dispatch-scope.test.ts were guarded by if (engine === 'native') { it.todo(...) } to skip running negative-case assertions (false-positive edge prevention) for the native engine. The original guard was added because the prebuilt native binary at the time didn't include the class-scoped this-dispatch fix from edge_builder.rs. Now that CI compiles the native addon from source on every PR, the fix is always available — the guard is no longer needed.

Why This Approach: The code in crates/codegraph-core/src/edge_builder.rs:506-544 clearly contains the class-scoped lookup fix (prefer Shape.area over Calculator.area when resolving this.area() inside Shape.describe). Removing the if (engine === 'native') guard and .todo stubs is the correct action — the tests should now pass for both engines.

Risk Assessment: Low. This is a pure test-promotion change. No production code is modified. No assertions are weakened — three previously skipped .todo tests are promoted to full active assertions. The positive case (Shape.describe → Shape.area) already ran for both engines.

---

Phase 2: Configuration Change Detection

No linting rules, TypeScript flags, test timeouts, or build configuration changed. ✓

---

Phase 3: Test Integrity

No red flags. The changes are strictly test-strengthening:

• Before: 3 negative-case assertions were it.todo(...) for native → effectively skipped
• After: All 4 assertions (tests/integration/this-dispatch-scope.test.ts:65-106) run unconditionally for both wasm and native engines

The if (engine === 'native') { ... } else { ... } branch structure at the removed lines was the only thing preventing these assertions from running. All assertion logic is preserved verbatim — the diff is structural cleanup only.

One minor note: the PR description references commit 16af0e0 (PR #1343) as the source of the edge_builder.rs fix, but that short hash doesn't resolve in the current local history (possibly squash-merged). That said, the fix is verifiably present at edge_builder.rs:506-544 — the class-scoped lookup logic is in the codebase, so the claim is substantively correct even if the commit reference is unresolvable.

---

Phase 4: Code Quality

The resulting test file is strictly cleaner. The removed if/else guard was a code smell that silently diverged native and WASM test coverage. No issues with:

• The describe.each(ENGINES) pattern (line 50) — idiomatic vitest parameterization
• The beforeAll fixture setup (lines 54-59) — correct temp dir isolation
• The SQL query in readCallEdges (lines 31-46) — parameterized via prepare, no injection risk
• The error messages in expect(...) calls include full edge dumps for easy diagnosis

---

Phase 5: Backlog Compliance

Axis	Result
Zero-dep	✓ — no new dependencies
Foundation-aligned	✓ — test quality improvement, no principles violated
Problem-fit	4/5 — directly improves the accuracy of the native engine's edge extraction, which is what AI agents rely on for blast-radius and call-chain queries
Breaking	No — test-only change, no API/CLI/DB schema changes
Tier	Tier 1

---

Final Assessment

Root Cause Analysis
  Problem Being Solved: Native engine was silently skipping 3 negative-case
    this-dispatch assertions due to a binary-gap guard that's no longer needed.
  Why This Approach: The fix is present in edge_builder.rs:506-544; CI builds
    from source. Removing the guard is the right action.
  Risk Assessment: Very low. Pure test promotion, no production code changes.

Backlog Compliance
  Zero-dep:           ✓
  Foundation-aligned: ✓
  Problem-fit:        4/5
  Breaking:           No
  Tier:               1

Critical Concerns
  None.

Final Recommendation
  Rating: ⭐⭐⭐⭐ (4/5)
  Action: APPROVE
  Reasoning: Straightforward, well-justified test promotion. The underlying fix
    is confirmed present in the Rust source (edge_builder.rs:506-544). The change
    strengthens coverage without weakening any assertion. The only minor issue is
    an unresolvable commit hash (16af0e0) in the PR description, but the
    substantive claim is correct.

[greptile-apps]

Greptile Summary

This PR promotes three negative-assertion tests from .todo to active for the native engine in this-dispatch-scope.test.ts, after the underlying Rust fix landed in edge_builder.rs (#1343) and CI compiles the native addon from source. It also adds a timeout substring check to the network-error guard in embedding-regression.test.ts so that HuggingFace fetch-layer timeouts (whose original errno is lost when wrapped as EngineError) cause the suite to skip gracefully rather than fail.

• this-dispatch-scope.test.ts: Removes the if (engine === 'native') guard and three .todo stubs; all four this-dispatch assertions now run for both wasm and native.
• embedding-regression.test.ts: Adds msg.toLowerCase().includes('timeout') to isNetworkError, with a comment explaining the ENGINE_UNAVAILABLE wrapping that loses the original error code.

Confidence Score: 5/5

Safe to merge — both changes are test-only and the underlying Rust fix that makes the native assertions correct is already compiled into the binary used by CI.

The this-dispatch changes are a straight mechanical promotion of three previously-skipped assertions; no production logic is touched. The timeout substring addition in the embedding regression guard is narrow in scope and correctly handles the HF error-wrapping case described in the comment.

No files require special attention.

Important Files Changed

Filename	Overview
tests/integration/this-dispatch-scope.test.ts	Removes the native-engine guard and three .todo stubs; all four this-dispatch assertions now run for wasm and native. Clean mechanical change with no logic alteration to the assertions themselves.
tests/search/embedding-regression.test.ts	Adds a broad 'timeout' substring check to the network-error skip guard so that HF fetch-layer timeouts don't cause CI failures when the original errno is lost. Low risk in this test-only path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["describe.each(ENGINES)\n['wasm', 'native']"] --> B["beforeAll: buildGraph fixture"]
    B --> C["it: emits Shape.describe → Shape.area\n✅ both engines"]
    B --> D["it: does NOT emit Shape.describe → Calculator.area\n✅ both engines (was native .todo)"]
    B --> E["it: does NOT emit Shape.describe → Formatter.area\n✅ both engines (was native .todo)"]
    B --> F["it: does NOT emit Caller.run → Sibling.area\n✅ both engines (was native .todo)"]

    G["buildEmbeddings\n(embedding-regression)"] --> H{Error thrown?}
    H -- No --> I["run regression tests"]
    H -- Yes --> J{isNetworkError?}
    J -- "msg includes '429'" --> K["rateLimited = true\nskip all tests"]
    J -- "msg includes 'timeout' (NEW)" --> K
    J -- "code: ECONNRESET / ETIMEDOUT\n/ ENOTFOUND / etc." --> K
    J -- "other error" --> L["re-throw → CI failure"]

Loading

_{Reviews (8): Last reviewed commit: "Merge branch 'main' into fix/native-this..." | Re-trigger Greptile}

[carlos-alm]

Addressed Claude's review:

• Root cause analysis confirmed: the edge_builder.rs fix (commit 16af0e0, PR fix(edge_builder): restrict same-file this-dispatch fallback to caller's own class #1343) is verifiably present in the codebase at edge_builder.rs:506-544. The commit hash does resolve in git history (not a squash-merge artifact as noted).
• No code changes needed — the review confirmed this is a straightforward, low-risk test promotion.
• Rating: 4/5, APPROVE. No action items.

[carlos-alm]

@greptileai

[carlos-alm]

Fixed CI failure: the Windows CI was failing in tests/search/embedding-regression.test.ts because the HuggingFace model download timed out. The HF library throws a generic error that gets wrapped by loadModel as EngineError (code: ENGINE_UNAVAILABLE), losing the original ETIMEDOUT errno. The existing network-error guard only matched on raw errno codes, so the wrapped timeout error was re-thrown instead of gracefully skipping.

Fix: added msg.toLowerCase().includes('timeout') to the isNetworkError check, which catches HF's 'Request timeout error occurred' message regardless of how it's wrapped.