[hussnainahmedd]

Problem

analyze-javascript-application (CLI and stdio MCP) emits a resolved, high-confidence imports edge for a require(...) call even when require resolves to a local binding — e.g. a function parameter, a hoisted function declaration, a catch binding, or a block-scoped binding. That is a confident-looking false fact in the evidence graph: lexical binding already establishes the call is not the CommonJS loader.

Minimal repro from the issue (still failing on current main):

// main.cjs
function run(require) {
  return require("./ghost.cjs");
}
console.log(run(() => "local"));

Node prints local, but REA returns an imports edge (kind: "require", resolution_status: "resolved").

Root cause: inspectCall already receives the lexical binding index (JavaScriptSemanticAnalysisState), but requireCallSpecifier in src/domain/javascript/javascriptStaticAnalysisCalls.ts recognizes the bare require name syntactically without consulting it. JavaScriptArtifactGraphFindings then resolves the candidate specifier to an inventoried file and emits the edge.

Fix

requireCallSpecifier now consults the binding index at the call site before emitting a require reference for a bare require/require?.() callee:

• Shadowed (parameter, hoisted function, catch, block binding, initialized var/let/const, import binding) → no reference, no edge.
• Unbound (Node's wrapper loader) → reference kept.
• Uninitialized top-level var require → reference kept (retains the wrapper loader, per the issue spec).
• Direct createRequire(import.meta.url) alias (const/let, incl. behind erased TS syntax like as NodeRequire, incl. aliased imports) → reference kept.
• Writes are handled positionally, not with a file-wide flag: a call that runs before every write to require still sees the loader, so require?.("./dep"); require = localLoader and eagerly evaluated computed method keys ({ [require?.("./dep")]() {} }) keep their edges. A call at/after a write is treated as shadowed.

The check is local to the call site (resolveSemanticBindingState, O(scope depth)); no file-wide AST walk is introduced. The gate applies only to the bare require name — __webpack_require__, module-scoped require names, and bundle-factory loaders are untouched.

Verification

• New regression suite src/domain/javascript/javascriptStaticAnalysisShadowedRequire.test.ts (16 tests): the issue repro (plain + optional call), destructured param, hoisted function, catch, block and initialized-var shadows emit no reference; unbound, optional-unbound, uninitialized top-level var, createRequire (plain + as-cast + aliased import) keep theirs; loader-before-reassignment, computed-method-key, mixed-file, and call-before-write/call-after-write cases behave as specified. All 16 pass.
• Existing suites re-run against the change (node-based runner; see environment note): all src/domain/javascript tests — 1289 passed, 0 failed (one fast-check property suite excluded, see below); tests/boundary/filesystem/referenceSourceShadowedRequire.test.js — pass (reference-source path unaffected); tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.js — 6 passed (full application-graph pipeline).
• npm run verify:module-boundaries — pass ("Checked resolved module boundaries in 1758 source files").
• tsc --noEmit: 823 errors before and after the change — identical count, all pre-existing (corrupted .d.ts files in node_modules from a disk-full npm ci in this sandbox); zero errors in the touched files.

Environment note: this sandbox cannot spawn vitest workers (SIGBUS, also breaks the repo's own verify:test-discovery script and the oxfmt pre-commit hook), and ESLint is not installed here. Tests were executed by compiling with the repo's TypeScript and running the compiled suites through a small node-based vitest-API shim. CI should run the standard gates.

Relation to #2892

This addresses every review point left on the closed PR #2892:

1. Whole-file AST walks (Greptile, 2 comments): that PR ran collectSemanticReferences + two full traversals synchronously inside the analysis steps. This fix resolves scope per call site via the already-computed binding index — no new walks, and files without require calls pay nothing beyond a string comparison.
2. Loader calls before reassignment (Codex): the old file-wide requireLoaderWasWritten flag dropped every call once any write existed. Writes are now compared positionally against each call, so require?.("./dep"); require = localLoader keeps its edge.
3. Computed method keys treated as deferred (Codex): no deferred/eager ancestor classification exists here at all — the positional write check handles { [require?.("./dep")]() {} } correctly without walking ancestors.
4. Unwrapping createRequire initializers (Codex): the initializer is passed through unwrapJavaScriptExpression (unwraps TSAsExpression and friends) before matching, so const require = createRequire(import.meta.url) as NodeRequire is recognized.
5. Filtering semantic references (Codex): no reference collection or copying is added; the classifier reuses the shared lexical analyzer that the findings pass already receives.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[greptile-apps]

Confidence Score: 1/5

[Medium impact] Refines how the code graph detects shadowed require calls.

The PR still needs fixes because skipped writes and computed field keys can produce wrong dependency graphs.

Findings

1.  Skipped writes hide real dependencies ▶
2.  Skipped assignments invent imports ▶
3.  Class key imports disappear ▶
4.  Repeated writes slow analysis ▶

Fix with agent prompt

### Issue 1
src/domain/javascript/javascriptStaticAnalysisCalls.ts:564-568
`requireWriteShadowsCall` treats a skipped write as proof that the loader was replaced. In `var require; if (false) require = localLoader; require("./dep.cjs");`, the call still uses Node's loader, but the earlier assignment causes its dependency to disappear. The same problem affects writes in empty loops.

Check whether the write must run before the call, not just whether it appears earlier.

### Issue 2
src/domain/javascript/javascriptStaticAnalysisCalls.ts:401-415
A skipped `createRequire` assignment can turn a local call into a dependency. For example, `import { createRequire } from "node:module"; let require = () => "local"; if (false) require = createRequire(import.meta.url); require("./ghost.cjs");` selects the assignment as the establishing write even though it never runs.

`conditionalInitializers` only marks conditional `var` declarations, so it does not reject this assignment. Check that the establishing write runs before the call, not just that it appears earlier.

### Issue 3
src/domain/javascript/javascriptStaticAnalysisCalls.ts:862-865
Static class fields and computed keys do not run in source order. In `var require; class C { static value = (require = () => "local"); [require("./real.cjs")]() {} }`, the computed key runs before the static field replaces the loader. Both are assigned the same enclosing scope here, so the earlier-looking write drops the real dependency. Account for class evaluation order before comparing write positions.

### Issue 4
src/domain/javascript/javascriptStaticAnalysisCalls.ts:310-316
Every bare `require` call now scans the binding’s full `definitions` array with `filter` and `every`, then potentially scans it again with `some`. Each assignment adds another definition, so files with many calls and writes pay calls × writes work, plus a temporary array per call.

Cache the binding’s declaration facts and index its write positions once instead of rescanning them.

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!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR checks lexical bindings before recording bare require calls.

• Bare require calls now follow their lexical bindings.

The merge-base checks differed, so the full PR diff was reviewed as the changes since the previous review. No new comments were added; previous findings were rechecked. Supersession could not be verified because GitHub access was unavailable.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A["Bare require call"] --> B["Resolve lexical binding"]
  B --> C{"Binding found?"}
  C -->|No| D["Keep reference"]
  C -->|Yes| E{"Recovered factory loader?"}
  E -->|Yes| F["Check writes before call"]
  E -->|No| G{"Retained var or createRequire loader?"}
  G -->|No| H["Skip reference"]
  G -->|Yes| F
  F --> I{"Loader replaced?"}
  I -->|Yes| H
  I -->|No| D

Loading

_{Reviews (14) · Last reviewed commit: "Merge branch 'main' into fix/2890-shadow..." · Reviewed by Greptile}

[greptile-apps]

Repeated writes slow analysis

Every bare require call now scans the binding’s full definitions array with filter and every, then potentially scans it again with some. Each assignment adds another definition, so files with many calls and writes pay calls × writes work, plus a temporary array per call.

Cache the binding’s declaration facts and index its write positions once instead of rescanning them.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 310-316

Comment:
**Repeated writes slow analysis**

Every bare `require` call now scans the binding’s full `definitions` array with `filter` and `every`, then potentially scans it again with `some`. Each assignment adds another definition, so files with many calls and writes pay calls × writes work, plus a temporary array per call.

Cache the binding’s declaration facts and index its write positions once instead of rescanning them.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

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!

[hussnainahmedd]

Thanks for the thorough review — all five findings are addressed in 4c4c273:

Bundle dependencies disappear — the factory's loader scope id is now recorded in the module range index, so a bare require(...) that resolves to the factory's own require parameter keeps its dependency, while a nested binding with the same name stays shadowed. (javascriptStaticAnalysisBundler.ts, javascriptStaticAnalysisCalls.ts)

Local calls still invent dependencies — the binding index now records the var/let/const declaration kind, and the wrapper-loader exception is restricted to var. let require; require?.("./ghost.cjs") no longer emits a reference.

Assignment hides its own dependency — a write now takes effect at the end of its assignment expression rather than at the target's position, so require = require("./dep.cjs") keeps the right-hand call. Writes inside a function body only shadow calls in that same body, since source order can't establish when the body runs.

Namespace loaders lose dependencies — isDirectCreateRequireBinding now resolves createRequire through its imported origin, so import * as module from "node:module"; const require = module.createRequire(import.meta.url) keeps working.

Repeated writes slow analysis — declaration facts and write positions are computed once per binding (WeakMap cache) instead of rescanning definitions for every bare require call.

Tests: extended javascriptStaticAnalysisShadowedRequire.test.ts to 24 tests (was 16), covering each finding plus a same-function-body shadowing guard; full src/domain/javascript suite passes (1307 tests), boundary shadowed-require test passes, tsc and npm run lint clean.

Also updated the branch — it's now current with main.

[hussnainahmedd]

Addressed all four greptile P1 findings in src/domain/javascript/javascriptStaticAnalysisCalls.ts (verified each against the analyzer first — all four reproduced as false require dependencies):

1. Local factory calls become dependencies — confirmed: a hoisted function require() in the factory body merged into the loader parameter's binding, so the scopeId-only check exempted it. The factory exemption now requires the binding's definitions to be only the loader parameter (assignments keep existing behavior); a merged declaration binding is treated as a local shadow.
2. Missing properties become loaders — confirmed. isDirectCreateRequireBinding now requires an empty projection, so const { missing: require } = createRequire(...) is no longer treated as the loader.
3. Rest objects become loaders — confirmed. var {...require} = {} copies through a reference initializer with copyKind set; the retained-loader check now rejects bindings whose declaration assigns via a rest-element copy instead of inferring from the initializer count.
4. Loop replacements arrive too late — confirmed for both for...of and for...in. Writes from loop statements are now modeled at body entry (the loop variable is assigned before each body execution) instead of at the statement end, so calls inside the body are correctly shadowed while calls in the iterable and before the loop still see the loader.

Added 11 regression tests to javascriptStaticAnalysisShadowedRequire.test.ts covering each case plus the keep-behavior counterparts (loader call before the loop, iterable call). Tests: shadowed-require suite 34/34, plus related semantic-analysis suites (74) and application-graph/dataflow suites (88) all green; tsc --noEmit clean, oxlint/oxfmt clean.

[hussnainahmedd]

Thanks for the review — verified all three P1s against the code with probe tests, and all three were valid:

• Uninitialized var redeclaration of the loader parameter: entry(module, exports, require) { var require; require("dep"); } was dropping the real dep. Fixed — the gate now treats an uninitialized var redeclaration as loader-retaining (it hoists without touching the parameter value); initialized declarations and hoisted functions still shadow.
• Assignment accepted before write-order check: require = () => "local"; require("ghost") invented a false dep. Fixed — the factory gate now runs the same positional write analysis as the top-level case: a call at/after an effective write in the same body is shadowed, earlier calls keep the loader. Also fixed nearestFunctionNode to walk from the node's parent, so a write whose RHS is itself a function (like the arrow above) is attributed to the enclosing body rather than the assigned function.
• Dropped createRequire forms: both valid. import mod from "node:module" + mod.createRequire(...) is now accepted — ESM import bindings are read-only, so the default export's identity is as stable as the namespace form. And createRequire(fileURLToPath(import.meta.url)) / createRequire(__filename) are accepted via the shared file-identity check (the argument only selects the resolution base).

Added a regression test per case. Full src/domain/javascript suite (1328 tests) green, tsc/oxlint/oxfmt clean, branch updated to latest main.

[hussnainahmedd]

Thanks for the three P1s — I verified each one with a failing probe first, and all three were valid. Fixed in 84ea39f with regression tests:

• Local key calls become dependencies — nearestFunctionNode now skips a computed method key, which executes in the enclosing scope rather than inside the method it names. So require = () => "local" before { [require("ghost")]() {} } correctly shadows the key call instead of recording a false dependency.
• Copied objects become loaders — isLoaderRetainingParameterBinding now rejects rest-element copies in referenceInitializers, mirroring the check the top-level loader path already had. var {...require} = {} no longer passes as loader-retaining.
• Earlier dependencies disappear — both the createRequire-alias check and the uninitialized-var loader check now treat declaration initializers as positional writes instead of rejecting the binding for the whole file. A call before a later write keeps its dependency; a call at or after the write is shadowed.

Validation: full src/domain/javascript suite green (1336 tests, 8 new regression cases), tsc/oxlint/oxfmt clean apart from one pre-existing tsc typing nit in the test file's it.each callback that predates this change.

[greptile-apps]

Skipped writes hide real dependencies

requireWriteShadowsCall treats a skipped write as proof that the loader was replaced. In var require; if (false) require = localLoader; require("./dep.cjs");, the call still uses Node's loader, but the earlier assignment causes its dependency to disappear. The same problem affects writes in empty loops.

Check whether the write must run before the call, not just whether it appears earlier.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 564-568

Comment:
**Skipped writes hide real dependencies**

`requireWriteShadowsCall` treats a skipped write as proof that the loader was replaced. In `var require; if (false) require = localLoader; require("./dep.cjs");`, the call still uses Node's loader, but the earlier assignment causes its dependency to disappear. The same problem affects writes in empty loops.

Check whether the write must run before the call, not just whether it appears earlier.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[hussnainahmedd]

Good catch — fixed. A write in an instance field initializer was scoped to its own field node, so a call in a later field of the same class never saw it, even though field initializers run in textual order during construction.

nearestFunctionNode now resolves deferred instance-field values to their shared class scope, so an earlier field's write provably shadows a later field's call, while the writes stay deferred until construction — they still can't hide top-level or method-scope dependencies, and static fields, computed keys, and decorators keep their previous behavior.

Added regression tests for the cross-field case plus pins for field order, per-class scoping, and method calls. The full src/domain/javascript suite stays green (1554 passing; the 3 failures in unrelated electron/semantic test files reproduce on pristine upstream main).

[hussnainahmedd]

Good catch — fixed. isDirectCreateRequireBinding only looked at the first initializer, so a later require = createRequire(import.meta.url) that restores the loader was never recognized, and the import edge got dropped.

The gate now treats the last createRequire-alias write that takes effect before the call as the establishing write: writes after it still shadow the call, writes before it were already overwritten by it. Verified with a failing probe first, then added regression tests — restored loader keeps the import, a re-shadow after the restore stays dropped, and calls on either side of a restore both keep theirs. 63/63 shadowed-require tests green (the 3 remaining suite failures are pre-existing on main). Also synced the branch with upstream main.

[greptile-apps]

Skipped assignments invent imports

A skipped createRequire assignment can turn a local call into a dependency. For example, import { createRequire } from "node:module"; let require = () => "local"; if (false) require = createRequire(import.meta.url); require("./ghost.cjs"); selects the assignment as the establishing write even though it never runs.

conditionalInitializers only marks conditional var declarations, so it does not reject this assignment. Check that the establishing write runs before the call, not just that it appears earlier.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 399-404

Comment:
**Skipped assignments invent imports**

A skipped `createRequire` assignment can turn a local call into a dependency. For example, `import { createRequire } from "node:module"; let require = () => "local"; if (false) require = createRequire(import.meta.url); require("./ghost.cjs");` selects the assignment as the establishing write even though it never runs.

`conditionalInitializers` only marks conditional `var` declarations, so it does not reject this assignment. Check that the establishing write runs before the call, not just that it appears earlier.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[hussnainahmedd]

Both P1s verified true — reproduced each with a failing probe before fixing:

1. Skipped assignments invent imports (4266412006): confirmed — conditionalInitializers only marks conditional var declarations, so if (x) require = createRequire(import.meta.url) was accepted as the establishing write even though it may never run. The establishing write must now provably execute before the call (same branch-alignment check the shadow gate already uses), which covers conditional assignments too.

2. Later replacements are missed (4266412017): confirmed — binding.initializers collects declaration initializers before assignment writes, so the array-index slice missed a declaration that executes after the establishing write. Later writes are now compared by execution position rather than array index.

Fixed in a6e65557. Validation: shadowed-require suite 65/65 (2 new regression tests); full src/domain/javascript suite 1584/1587 with the 3 failures confirmed pre-existing on pristine HEAD; tsc shows only the 2 pre-existing errors (saxes dep, it.each inference); oxlint 0 errors, oxfmt clean.

[greptile-apps]

Class key imports disappear

Static class fields and computed keys do not run in source order. In var require; class C { static value = (require = () => "local"); [require("./real.cjs")]() {} }, the computed key runs before the static field replaces the loader. Both are assigned the same enclosing scope here, so the earlier-looking write drops the real dependency. Account for class evaluation order before comparing write positions.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 731-734

Comment:
**Class key imports disappear**

Static class fields and computed keys do not run in source order. In `var require; class C { static value = (require = () => "local"); [require("./real.cjs")]() {} }`, the computed key runs before the static field replaces the loader. Both are assigned the same enclosing scope here, so the earlier-looking write drops the real dependency. Account for class evaluation order before comparing write positions.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[hussnainahmedd]

Both P1s verified as real bugs against the runtime and fixed — thanks for the sharp catches.

Loop initializers invent imports: a classic for initializer runs exactly once before the test and body, so writeMustRunBeforeCall no longer treats the loop as a conditional ancestor when the write sits in the loop head. for (var require = () => "local"; false;) {} require("./ghost.cjs") no longer invents the dependency; writes in the loop update/test/body keep the old conditional behavior.

Class key imports disappear: verified against Node that computed method keys evaluate before static field initializers and static blocks run, so source order can't decide shadowing across those slots. writeFactPrecedesCall now compares class-definition evaluation phases (computed key < static initializer) when write and call are in the same class; same-phase and everything else keep the positional behavior.

Verification: 7 new regression tests (both reported snippets plus the mirrored orderings and a for-update guard), targeted suite 72/72, full src/domain/javascript suite 1591/1594 with only the 3 pre-existing failures (electronWrappedCallPaths, javascriptSemanticAmbientWrites, native_bindings). tsc --noEmit shows only the 2 pre-existing errors, oxlint/oxfmt clean. Branch is updated with upstream main.

[hussnainahmedd]

Good catch — verified true with a failing probe first: var require; do { require = () => "local"; } while (false); require("./ghost.cjs"); was inventing a false dependency on ./ghost.cjs, because writeMustRunBeforeCall treated the do-while body as work that might be skipped even though it always runs once.

Fixed by no longer treating a do...while as a conditional construct for a write in its own body — mirroring the existing for-initializer exception in the same function. while, classic for bodies, and for...of/for...in may still skip their body, so they stay conditional, and a write nested in a skipped branch (e.g. a do-while inside an if (false)) still doesn't shadow.

Added 5 regression cases: a do-while body write shadowing a later call, a looping do-while, a call in the loop test — plus loader-side counterparts (do-while inside a skipped if, and a call before the loop keeps the loader). Shadowed-require suite: 77/77 green; full src/domain/javascript suite: 1605/1605 green; tsc/oxlint/oxfmt show only the two pre-existing issues, nothing new.

Fix commit: 68cb6c6. Re: inline comment 4267079662.

[hussnainahmedd]

Thanks — verified this one. A createRequire declaration inside a branch (e.g. if (c) { var require = createRequire(import.meta.url); require("./dep.cjs"); }) lost its dependency: conditionalInitializers made isDirectAliasWrite reject the initializer outright, before the shared-branch check could decide.

Now the branch-alignment check (writeMustRunBeforeCall) decides instead — the initializer establishes the alias exactly when the call sits in the same branch, so it provably runs first on every execution reaching the call. Calls outside the branch keep the existing conservative behavior.

Added 4 regression tests to the shadowed-require suite (81/81 green); full src/domain/javascript suite 1609/1609 green, no new tsc/lint/format issues.

[hussnainahmedd]

Good catch — verified true with a failing probe first: var require; while ((require = () => "local", false)) {} require("./ghost.cjs") invented an import for ./ghost.cjs, and the same held for for-condition and do...while-test variants.

The root cause: writeMustRunBeforeCall treated a write in any loop as conditional, but loop conditions aren't work the loop may skip — a while test, a classic for test (after its initializer), and a do...while test (after its body) always evaluate at least once when the loop statement runs, unlike loop bodies which may never run. So a write there provably runs before any call after the loop, and the loader replacement is real.

Fixed writeMustRunBeforeCall to honor those always-executing loop parts; a classic for update still stays conditional (it only runs after a body iteration), as do while/for...of/for...in bodies. Added 4 regression tests — the full shadowed-require suite is 85/85 green and the rest of the javascript domain is unaffected.

Greptile Review will re-run on the updated branch.

[morluto]

Superseded by eef4a0bfb4f005dcfb0d16cf6a6d5bc85d743e23. Require and chunk references now derive from shared lexical binding ownership and recovered bundler identities; uncertain writes remain unresolved evidence instead of becoming confident import edges. Validation: src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts (27 tests), javascriptSemanticAnalysis.require_links.test.ts (24), and javascriptSemanticMutationInitializers.test.ts (3) passed in the phase-five final log. These pin shadowed bindings, wrapper-loader cases, and uncertain mutation behavior; this remains static analysis, not runtime execution of every JavaScript program. Thanks to the PR author and reviewers.