test(extractor): verify exported arrow function funcStack tracking in extractSpreadForOfWalk

[carlos-alm]

Summary

• Adds 3 regression tests to tests/parsers/javascript.test.ts confirming that export const f = (arr) => { for (const x of arr) x(); } correctly sets enclosingFunc: 'f' in forOfBindings
• The recursive walk in extractSpreadForOfWalk visits variable_declarator regardless of whether it is nested under a plain lexical_declaration or an export_statement, so the gap reported in the PR feat(resolver): resolve prototype-based method calls, spread/iteration callbacks, func-prop this-dispatch, and object-rest param dispatch (JS) #1331 review was already closed by commit a6c5d2d when the variable_declarator branch was added to the funcStack push chain
• These tests document and gate that behavior so it cannot silently regress

Test plan

• npx vitest run tests/parsers/javascript.test.ts -t "Phase 8.3e: extractSpreadForOfWalk" — all 3 new tests pass
• Full tests/parsers/javascript.test.ts suite remains green (91/91)

Closes #1354

[claude]

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

---

PR Review

• Gather context
• Read changed test file
• Review source code for extractSpreadForOfWalk
• Analyze test quality and correctness
• Post final review

---

Root Cause Analysis

Problem Being Solved: Issue #1354 flagged a potential gap: when an arrow function is export const f = ... rather than plain const f = ..., could extractSpreadForOfWalk fail to push f onto funcStack, causing enclosingFunc to be wrong for for-of bindings inside it?

Why This Approach: The claim (and these tests confirm it) is that the gap never actually existed: the walk function in extractSpreadForOfWalk iterates all children recursively (src/extractors/javascript.ts:2047-2049) without filtering out export_statement nodes. The AST path is export_statement → lexical_declaration → variable_declarator, and the variable_declarator branch at line 1913 fires regardless of what wraps the lexical_declaration. No special handling for export_statement was ever needed.

Risk Assessment: Low. This is test-only; no production code is touched. The three tests are positive-case only, with no weakened assertions or skipped validation.

---

Test Quality Assessment

Test 1 (tracks plain const arrow function) — Correct and useful as a baseline. Without this, the exported variant below has no non-exported counterpart to compare against.

Test 2 (tracks exported const arrow function) — The core regression gate. Correct assertion. Uses toContainEqual + expect.objectContaining, which is the right pattern here since forOfBindings may contain other entries.

Test 3 (records correct varName and sourceName) — Checks all three fields of the ForOfBinding shape in one go for the exported form. Good completeness. One minor note: process is a Node.js global — it is not in BUILTIN_GLOBALS (which guards JS builtins like Math, console), so this shouldn't cause a false negative, but using a more neutral name (e.g. transform) would make the intent clearer and avoid future confusion if BUILTIN_GLOBALS ever expands.

Describe block naming — Phase 8.3e: extractSpreadForOfWalk — exported arrow function funcStack (#1354) is consistent with how other Phase-labelled suites are named in this file. No concern.

No assertions weakened, no skipped tests, no config changes — clean bill of health for Phases 1-3.

---

Backlog Compliance

• Zero-dep: ✓ — test-only, zero new runtime deps
• Foundation-aligned: ✓ — pure regression gating, no pipeline changes
• Problem-fit: 3 — keeps the extractor honest for the export-wrapped arrow case, which matters for accurate blast-radius analysis, but this is a guard rather than a new capability
• Breaking: No — purely additive
• Tier: 1

---

Critical Concerns

None. One minor suggestion:

process as a test identifier (tests/parsers/javascript.test.ts:928): process collides with the Node.js global. It won't cause a false negative today, but a neutral name like transform or handle would be safer if BUILTIN_GLOBALS is ever extended. Not blocking.

---

Final Recommendation

• Rating: ⭐⭐⭐⭐ (4/5)
• Action: APPROVE
• Reasoning: Three well-targeted regression tests that correctly verify the behavior described in the PR body. Source code analysis confirms the variable_declarator branch fires naturally through the recursive walk regardless of export wrapping — the tests gate real, correct behavior. No production code changes, no assertions weakened, no config relaxed. The only nit is the use of process as a test identifier, which is not blocking.

---

[greptile-apps]

Greptile Summary

This PR adds three regression tests verifying that extractSpreadForOfWalk correctly pushes exported const arrow functions onto funcStack — confirming that export const f = (arr) => { for (const x of arr) x(); } produces enclosingFunc: 'f' in forOfBindings. It also removes the now-redundant named import of ParamBinding from wasm-worker-protocol.ts, replacing it with the inline-import style already used by all sibling binding fields.

• New tests (Phase 8.3e): Three cases cover plain const, export const with a minimal name, and export const with full varName/sourceName/enclosingFunc assertions — gating the variable_declarator branch in extractSpreadForOfWalk against silent regression.
• Protocol cleanup: ParamBinding is removed from the top-level named import block in wasm-worker-protocol.ts; the field paramBindings?: ParamBinding[] in SerializedExtractorOutput is replaced by the equivalent inline-import form (import('../types.js').ParamBinding[]), matching the style of fnRefBindings, arrayElemBindings, and siblings.

Confidence Score: 5/5

Safe to merge — changes are test additions and an import style cleanup with no behavioral impact.

The test additions directly correspond to real branches in extractSpreadForOfWalk (the variable_declarator path at line 1913 of javascript.ts), and the assertions accurately reflect what the extractor produces. The protocol file change is purely cosmetic: the paramBindings field remains present on SerializedExtractorOutput and all downstream consumers in wasm-worker-pool.ts, wasm-worker-entry.ts, and build-edges.ts are unaffected.

No files require special attention.

Important Files Changed

Filename	Overview
tests/parsers/javascript.test.ts	Adds 3 regression tests for the exported-arrow-function path in extractSpreadForOfWalk; assertions match the actual walk logic and cover both the plain and export-wrapped cases cleanly.
src/domain/wasm-worker-protocol.ts	Style-only cleanup: removes named import of ParamBinding and replaces the field declaration with the inline-import form already used by all sibling binding fields; no functional or wire-format change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[extractSpreadForOfWalk walk node] --> B{node.type?}
    B -->|function_declaration| C[push nameNode.text to funcStack]
    B -->|method_definition| D[push qualified name to funcStack]
    B -->|variable_declarator| E{value is arrow_function or function_expression?}
    E -->|yes| F[push nameNode.text to funcStack]
    E -->|no| G[skip]
    B -->|for_of_statement| H[record ForOfBinding enclosingFunc = funcStack.top]
    B -->|export_statement| I[recurse into children]
    I --> B
    C --> J[recurse into children]
    D --> J
    F --> J
    G --> J
    J --> K[pop funcStack if pushed]

Loading

_{Reviews (3): Last reviewed commit: "fix: remove duplicate paramBindings in S..." | Re-trigger Greptile}

[greptile-apps]

process shadows a BUILTIN_GLOBALS entry, obscuring test intent

process appears on line 88 of src/extractors/javascript.ts inside BUILTIN_GLOBALS. A future reader of this test may reasonably assume it was chosen to verify how a built-in-named function interacts with the BUILTIN_GLOBALS guard — but BUILTIN_GLOBALS is never checked for enclosingFunc, so the name carries no semantic weight here. Using a clearly made-up name like handleItems or processItems would communicate that the test is purely about the export-wrapping path, not about built-in shadowing.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[carlos-alm]

Fixed — renamed process to handleItems in commit f01c339. The test now uses a clearly made-up name that conveys it's purely about the export-wrapping code path, not built-in shadowing.

[carlos-alm]

Addressed both concerns in commit f01c339:

CI fix (TS2300 duplicate identifier): The merge at 3c164f2 introduced a second paramBindings field at the bottom of SerializedExtractorOutput (using the top-level ParamBinding import) while the field already existed higher up using the inline import form. This caused TypeScript to fail with Duplicate identifier 'paramBindings', blocking every CI job. Removed the duplicate field and the now-unused ParamBinding top-level import.

Greptile / Claude nit (test identifier): Renamed process to handleItems in the Phase 8.3e test — process is a Node.js global and its presence in the fixture misleads readers into thinking the test targets built-in-shadowing behaviour.

[carlos-alm]

@greptileai

[carlos-alm]

@claude

[claude]

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

---

PR Re-Review

• Gather context
• Read latest changes (commit f01c339)
• Verify fixes
• Post final review

---

Follow-up Review (commit f01c339)

Both concerns from the previous review have been addressed correctly.

CI fix — duplicate paramBindings (src/domain/wasm-worker-protocol.ts): The duplicate field and its now-unused top-level ParamBinding import are gone. SerializedExtractorOutput now has a single paramBindings?: import('../types.js').ParamBinding[] entry at line 67, consistent with the inline-import style used by the surrounding fields. Clean.

Test identifier rename (tests/parsers/javascript.test.ts:923-934): process → handleItems. The test fixture now reads export const handleItems = (items) => { for (const cb of items) cb(); };, which unambiguously signals that the test is about the export-wrapping code path, not built-in shadowing. All three field assertions (varName: 'cb', sourceName: 'items', enclosingFunc: 'handleItems') remain correct and complete.

No new concerns.

---

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

---

View job