Repository navigation
fix(javascript): ignore a require call that a local binding shadows - #2921
XiaoBinGan wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c10ce28aa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // shadows the ambient loader, so only an unshadowed reference to it counts. | ||
| // The module's own factory require name is a real binding, so it keeps its | ||
| // own lane below. | ||
| if (name === "require" && isGlobalRequire) |
There was a problem hiding this comment.
For calls such as module.require("./dep.cjs"), calleeName returns "module.require", so this exact-name condition is false and neither later branch accepts the call. The previous name.endsWith(".require") lane was removed rather than preserved, causing these real loader calls to disappear from references and from downstream application import edges; keep a separate member-call branch while applying the shadow check only to bare require.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| node, | ||
| name, | ||
| moduleRequireName, | ||
| isUnmodifiedGlobalReference(node.callee, bindings, name), |
There was a problem hiding this comment.
For ESM using the standard const require = createRequire(import.meta.url) pattern, the local binding makes this helper return false, so a real subsequent require("./dep.cjs") call is now omitted even though it was recorded before. The repository already treats this direct binding as a valid loader in directCreateRequireBinding in src/domain/referenceSourceRequireBindings.ts; distinguish known loader-producing bindings instead of rejecting every resolved require, otherwise ESM dependency edges disappear from application analysis.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Both P1s are fixed in Root cause. My first revision replaced Structural fix.
Validation. The regression test now covers all four lanes:
Remaining cleanup, not folded in here. |
|
These two reviews were computed against
The regression test in that commit is the evidence rather than the file text: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc7bce7f79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expression !== null && | ||
| expression !== undefined && | ||
| t.isCallExpression(expression) && | ||
| calleeName(expression.callee).endsWith("createRequire") |
There was a problem hiding this comment.
For the valid ESM case import { createRequire as makeRequire } from "node:module"; const require = makeRequire(import.meta.url); require("./dep.cjs"), calleeName(expression.callee) is makeRequire, so this spelling check returns false and removes the static reference and downstream application import edge. Fresh evidence after the earlier createRequire comment is that the added suffix-only fix still rejects aliases, even though directCreateRequireBinding already follows each ImportSpecifier.local in src/domain/referenceSourceRequireBindings.ts:127-145; resolve that parsed import provenance rather than matching the callee text.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| ): boolean => { | ||
| const binding = resolveSemanticBindingState(state, callee, name); | ||
| if (binding === undefined) | ||
| return isUnmodifiedGlobalReference(callee, state, name); |
There was a problem hiding this comment.
In a .cjs input such as globalThis.require = shim; require("./dep"), the assignment changes a global-object property but not Node's wrapper-local loader parameter. isUnmodifiedGlobalReference rejects the call because the ambient-write index covers ["global", "require"] (javascriptSemanticAmbientWrites.ts:280-293), so this change deletes a real static/application import edge even though no lexical require was shadowed; keep the CommonJS wrapper binding distinct from globalThis.require.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| const binding = resolveSemanticBindingState(state, callee, name); | ||
| if (binding === undefined) | ||
| return isUnmodifiedGlobalReference(callee, state, name); | ||
| const initializer = semanticStableInitializer(binding, state)?.node; |
There was a problem hiding this comment.
For a .cjs input such as var require; require("./dep.cjs"), the semantic index resolves a program binding but it has no stable initializer, so this branch returns false and the static/application import edge is lost. An uninitialized top-level var only redeclares Node's wrapper parameter and does not replace its loader value; hasUninitializedTopLevelVarRequire in src/domain/referenceSourceRequireBindings.ts:39-40 and 91-111 already preserves this exact case, so keep that no-op wrapper redeclaration as a possible loader rather than treating it as absent.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
|
All three findings are valid, and they share one root cause: this change re-derives "is this call the Node loader" by name inside the static path instead of using the owner's fact — the shape the repository's own rule forbids.
The correct boundary is I am reworking the change to delegate to that owner rather than adding a fourth case to the heuristic. Until then I am not claiming these are fixed, and the original reported behaviour (a shadowing parameter must not produce an |
| callee: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| if (!t.isIdentifier(callee)) return false; |
There was a problem hiding this comment.
The identifier-only check rejects real loaders created through a namespace import. For example, import * as mod from "node:module"; const require = mod.createRequire(import.meta.url); require("./real.cjs"); was recognized before this change. Now isNodeCreateRequireFactory rejects mod.createRequire, so ./real.cjs disappears from the static references.
Check the member's module origin rather than rejecting every member call, and add a regression test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 259
Comment:
**Namespace-created loaders lose dependencies**
The identifier-only check rejects real loaders created through a namespace import. For example, `import * as mod from "node:module"; const require = mod.createRequire(import.meta.url); require("./real.cjs");` was recognized before this change. Now `isNodeCreateRequireFactory` rejects `mod.createRequire`, so `./real.cjs` disappears from the static `references`.
Check the member's module origin rather than rejecting every member call, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| // Node's CommonJS wrapper supplies a lexical `require` parameter. Writes | ||
| // to globalThis.require do not replace it; retain that possibility for | ||
| // script/CommonJS source where wrapper mode is possible. | ||
| if (name === "require" && isScriptProgram(callee, state)) return true; |
There was a problem hiding this comment.
The new script shortcut ignores direct assignments to require. With require = () => "local"; require("./ghost.cjs");, no lexical binding is created, so this branch returns true even though the source replaced the loader. The previous isUnmodifiedGlobalReference check rejected this visible write. The analyzer now records a dependency for a call that loads nothing.
Keep the distinction between globalThis.require and the wrapper's require, but still reject direct writes to the latter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 234
Comment:
**Replaced loaders invent dependencies**
The new script shortcut ignores direct assignments to `require`. With `require = () => "local"; require("./ghost.cjs");`, no lexical binding is created, so this branch returns `true` even though the source replaced the loader. The previous `isUnmodifiedGlobalReference` check rejected this visible write. The analyzer now records a dependency for a call that loads nothing.
Keep the distinction between `globalThis.require` and the wrapper's `require`, but still reject direct writes to the latter.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| binding.definitions.every(({ kind }) => kind === "variable") && | ||
| binding.initializers.length === 0 && |
There was a problem hiding this comment.
isTopLevelCommonJsVarRequire also accepts let require;. Both var and let declarations get the semantic kind "variable", and a top-level declaration without a value has no initializer. Thus let require; function run() { return require("./ghost.cjs"); } passes this check even though let creates a separate binding, not a wrapper redeclaration. The analyzer records a false loader reference even when run is never called.
Check that the actual declaration uses var, and add a negative test for let.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 294-295
Comment:
**Let bindings become loaders**
`isTopLevelCommonJsVarRequire` also accepts `let require;`. Both `var` and `let` declarations get the semantic kind `"variable"`, and a top-level declaration without a value has no initializer. Thus `let require; function run() { return require("./ghost.cjs"); }` passes this check even though `let` creates a separate binding, not a wrapper redeclaration. The analyzer records a false loader reference even when `run` is never called.
Check that the actual declaration uses `var`, and add a negative test for `let`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75aafcf6cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| callee: t.Node, | ||
| state: JavaScriptSemanticAnalysisState, | ||
| ): boolean => { | ||
| if (!t.isIdentifier(callee)) return false; |
There was a problem hiding this comment.
When ESM uses import * as Module from "node:module"; const require = Module.createRequire(import.meta.url); require("./dep.cjs"), the factory callee is a MemberExpression, so this guard returns false even though the namespace binding's directOrigins proves the same Node export; the resulting static reference and application imports edge disappear. Fresh evidence beyond the earlier aliased named-import report is this final identifier-only guard, so resolve the root import plus the createRequire member path as the neighboring worker-import logic does.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| isTopLevelCommonJsVarRequire(binding, callee, state) | ||
| ) | ||
| return true; | ||
| const initializer = semanticStableInitializer(binding, state)?.node; |
There was a problem hiding this comment.
In a script/CommonJS file with require("./real.cjs"); var require = shim;, the top-level var shares Node's wrapper parameter and its initializer does not run until after the first call, so that call still uses the loader. This whole-binding initializer lookup sees shim for every reference and rejects the earlier call, removing the static reference and application import edge. Fresh evidence beyond the prior uninitialized-var case is execution ordering: a later initializer must not retroactively classify an earlier reference, so preserve the loader possibility until a write that can precede that call.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| node, | ||
| name, | ||
| moduleRequireName, | ||
| isRequireLoaderReference(node.callee, bindings, name), |
There was a problem hiding this comment.
isRequireLoaderReference is evaluated eagerly for every call and new expression, although requireCallSpecifier reads the result only for the exact name require; ordinary local and global calls now traverse semantic scope chains, sometimes again through initializer import provenance. On large minified bundles where calls dominate nodes, this adds avoidable analysis CPU and time proportional to all call sites, so guard this lookup with name === "require" or make the argument lazy.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
75aafcf to
f9898f6
Compare
| const initializer = semanticStableInitializer(binding, state)?.node; | ||
| const expression = | ||
| initializer !== undefined && t.isVariableDeclarator(initializer) | ||
| ? initializer.init | ||
| : initializer; | ||
| return ( | ||
| expression !== null && | ||
| expression !== undefined && | ||
| t.isCallExpression(expression) && | ||
| isNodeCreateRequireFactory(expression.callee, state) |
There was a problem hiding this comment.
isRequireLoaderReference ignores the initializer's projection. With import { createRequire } from 'node:module'; const { resolve: require } = createRequire(import.meta.url); require('./ghost.cjs');, the local require calls the loader's resolve method, not the loader. The new check still records ./ghost.cjs as a dependency even though the call never loads it.
Require an empty projection before treating the binding as the returned loader, and add a regression test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 242-251
Comment:
**Loader properties become imports**
`isRequireLoaderReference` ignores the initializer's `projection`. With `import { createRequire } from 'node:module'; const { resolve: require } = createRequire(import.meta.url); require('./ghost.cjs');`, the local `require` calls the loader's `resolve` method, not the loader. The new check still records `./ghost.cjs` as a dependency even though the call never loads it.
Require an empty `projection` before treating the binding as the returned loader, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9898f68a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| t.isCallExpression(expression) && | ||
| isNodeCreateRequireFactory(expression.callee, state) |
There was a problem hiding this comment.
In a CommonJS module using const loader = require; { const require = loader; require("./dep.cjs"); }, semanticStableInitializer returns the loader identifier, but this call-expression-only gate returns false, so the static reference and the imports edge created from it in src/application/javascript/JavaScriptArtifactGraphFindings.ts:82-162 disappear. Fresh evidence beyond the createRequire threads is that this binding is a direct alias of the ambient wrapper loader rather than another factory-import spelling; follow stable identifier aliases (or retain the candidate) before rejecting the binding.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
f9898f6 to
45ecff9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45ecff9622
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ( | ||
| expression !== null && | ||
| expression !== undefined && | ||
| t.isCallExpression(expression) && |
There was a problem hiding this comment.
When TypeScript writes a loader as const require = createRequire(import.meta.url) as NodeRequire, semanticStableInitializer returns the TSAsExpression, so this strict CallExpression check is false even though the assertion is erased at runtime. A subsequent require("./dep.cjs") is therefore omitted from references and from the application imports edge produced in JavaScriptArtifactGraphFindings.ts; this is fresh syntax evidence beyond the earlier bare-createRequire case, because that bare form now works while the parser's supported type-only wrappers still need unwrapJavaScriptExpression before classification.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
`requireCallSpecifier` treated any call named `require` as the CommonJS loader, so a local function parameter named `require` produced a loader reference and the application graph invented an `imports` edge for it. Take whether the callee is an unshadowed loader reference, computed with `isUnmodifiedGlobalReference`, which matches the ownership rule the semantic path already applies in `javascriptSemanticRequireOrigin.ts`. Member calls and the module factory's own require parameter keep their lanes. Validation: the new test records the shadowed call without this change and not with it, while an unshadowed require is still recorded; `src/domain/javascript/*.test.ts` passes 57 files / 1283 tests; `check:fast` passes.
The shadow check dropped two lanes the static path recorded before, which the
review found:
- `module.require("./dep.cjs")` has the callee name `module.require`, which no
remaining branch accepts, so member loader calls disappeared.
- `const require = createRequire(import.meta.url)` is a real loader, but its
local binding made the check refuse the call.
Restore the member lane, and recognize a binding whose stable initializer calls
`createRequire`. A parameter or any other binding only shares the name with the
loader and is still refused.
Validation: the regression test covers all four lanes: an unshadowed require, a
member require, a createRequire binding, and a shadowing parameter that must not
be recorded.
45ecff9 to
9ed55f6
Compare
| binding?.runtimeImport === true && | ||
| binding.definitions.length === 1 && | ||
| binding.directOrigins.length === 1 && |
There was a problem hiding this comment.
isNodeCreateRequireFactory rejects factories loaded through CommonJS. For example, const { createRequire } = require("node:module"); function run() { const require = createRequire(__filename); return require("./real.cjs"); } run(); creates a real loader, but the factory binding has runtimeImport: false and no directOrigins. The new check therefore drops ./real.cjs from references and the application graph.
Recover the factory's CommonJS origin instead of requiring an ESM import, and add a regression test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysisCalls.ts
Line: 263-265
Comment:
**CommonJS loader dependencies disappear**
`isNodeCreateRequireFactory` rejects factories loaded through CommonJS. For example, `const { createRequire } = require("node:module"); function run() { const require = createRequire(__filename); return require("./real.cjs"); } run();` creates a real loader, but the factory binding has `runtimeImport: false` and no `directOrigins`. The new check therefore drops `./real.cjs` from `references` and the application graph.
Recover the factory's CommonJS origin instead of requiring an ESM import, and add a regression test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ed55f69cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| t.isCallExpression(expression) && | ||
| isNodeCreateRequireFactory(expression.callee, state) |
There was a problem hiding this comment.
For RequireJS/AMD sources using the simplified CommonJS form define(function (require) { return require("./dep"); }), that parameter is the real module loader, not a shadowing local. It has no stable initializer, so this createRequire-only predicate now returns false; the moduleRequireName escape hatch is not populated because javascriptStaticAnalysisBundler.ts:42-49 only accepts webpack/rspack chunk-array registrations. The static reference and downstream imports edge that the previous name-based scanner emitted therefore disappear for every such factory, so recognize the define factory's loader parameter before rejecting resolved bindings.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
|
Closing as superseded by #2989, which remains open for the shadowed-require fix. It covers the loader-versus-local-binding distinction here and carries broader producer and public CLI/MCP coverage, including bundle-factory loader ownership, createRequire bindings, hoisted declarations, and reassignment/loop ordering. I compared both current diffs. This closes the duplicate implementation; the underlying fix is still pending review and merge in #2989. Thank you for the contribution. |
morluto
added
bug
Summary
Only treat a call named
requireas the CommonJS loader when the callee is anunshadowed reference to it, so a locally bound
requireno longer invents animportsedge.Problem and expected behavior
analyze-javascript-applicationreports a resolved, high-confidenceimportsrelationship for a call to a local function parameter named
require.Lexical binding already establishes that the call is not the CommonJS loader, so
the graph must not carry that edge.
Minimal reproduction, as reported:
src/domain/javascript/javascriptStaticAnalysisCalls.tsdecided this by namealone:
Change and scope
requireCallSpecifiernow receives whether the callee is an unshadowed loaderreference, computed with
isUnmodifiedGlobalReference— the helper this filealready uses for worker constructors, and the same ownership rule the semantic
path applies in
javascriptSemanticRequireOrigin.ts, whose contract is torecover an unshadowed literal require origin.
Two lanes deliberately keep their behavior:
name.endsWith(".require")(member calls such asmodule.require) is aproperty access, not a lexical binding.
moduleRequireNameis the module factory's own require parameter(
javascriptStaticAnalysisBundler.ts), which is a real binding. A plainCommonJS module's ambient
requirehas no binding, so ordinary calls arestill detected.
Contract and boundary impact
The producing boundary keeps ownership of the decision. The semantic path
already refused a shadowed
require, so both paths now agree instead of one ofthem re-deriving the call from its name.
Evidence and regression coverage
src/domain/javascript/javascriptStaticAnalysis.shadowed_require.test.tsreproduces the reported source and covers both directions:
ghostspecifier ispresent in the references); with it, it is absent.
const loaded = require("./real.cjs")is still recorded, so no legitimateloader call is lost.
npm run test:focused -- src/domain/javascript/*.test.tspasses: 57 files,1283 tests.
npm run check:fastpasses.Unavailable coverage
The end-to-end
analyze-javascript-applicationedge was not re-run as aboundary test; the evidence is at the reference boundary that produces it
(
JavaScriptModuleRelationships.tsselects references whose kind isrequire).No real-provider verification is involved.