Repository navigation
Conversation
Share the existing Node loader classification with static findings and reuse the source's binding index. Keep ordinary, optional, wrapper-var, createRequire and bundled-loader behavior while excluding local calls. Fixes morluto#2890
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. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: c05f7eb723
ℹ️ 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".
| if (name === "__webpack_require__" || name === moduleRequireName) | ||
| return { specifier: argumentValue(node.arguments[0]) }; | ||
| if ( | ||
| (name === "require" && isUnshadowedRequireCall(node)) || |
There was a problem hiding this comment.
In a CommonJS source such as require?.("./dep"); require = localLoader, Node executes the first load before the assignment, but this new gate delegates to unshadowedJavaScriptRequireCalls, whose file-wide requireLoaderWasWritten flag rejects every call as soon as any unbound/program require write exists. The semantic module-link pass only recognizes CallExpression, not this OptionalCallExpression, so no later projection restores the edge and the final application graph omits a dependency that actually loaded; make write handling position/control-flow aware rather than invalidating all calls.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| requireCalls ??= unshadowedJavaScriptRequireCalls( | ||
| file, | ||
| undefined, | ||
| collectSemanticReferences(file.program, bindings), |
There was a problem hiding this comment.
On a large source containing any bare require, the first invocation of this lazy initializer drains collectSemanticReferencesSteps synchronously through completeSemanticSteps and then unshadowedJavaScriptRequireCalls performs another synchronous full-AST traversal. Because both passes run inside one traverseStaticSourceSteps.next(), their 1,024-node yields never reach completeJavaScriptAnalysisSteps, so its 8 ms checkpoints cannot process cancellation or control I/O until both whole-file passes finish; expose a stepped classifier or yield these passes instead of completing them inside the callback.
AGENTS.md reference: AGENTS.md:L35-L37
Useful? React with 👍 / 👎.
Yield reference indexing and loader classification to the analysis scheduler. Keep visitor callbacks limited to the completed call set. Preserve loader calls before later program-level writes. Keep deferred and same-statement execution order conservative, and cover both public adapters and reference-source imports with real Node controls.
| const requireCalls = yield* unshadowedJavaScriptRequireCallsSteps( | ||
| file, | ||
| undefined, | ||
| yield* collectSemanticReferencesSteps(file.program, bindings), | ||
| ); |
There was a problem hiding this comment.
analyzeParsedJavaScriptStaticSourceSteps now collects and stores every lexical reference before checking whether the source uses require. Files with no require calls previously skipped this work. Large ESM files now pay for another whole-file walk and a map of unrelated references.
This is non-blocking. Keep the yielding steps, but skip this collection when no loader call needs it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptStaticAnalysis.ts
Line: 103-107
Comment:
**Files without loaders do extra work**
`analyzeParsedJavaScriptStaticSourceSteps` now collects and stores every lexical reference before checking whether the source uses `require`. Files with no `require` calls previously skipped this work. Large ESM files now pay for another whole-file walk and a map of unrelated references.
This is non-blocking. Keep the yielding steps, but skip this collection when no loader call needs it.
---
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!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0cf34787
ℹ️ 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".
| ): boolean => | ||
| ancestors.some( | ||
| (ancestor) => | ||
| t.isFunction(ancestor) || |
There was a problem hiding this comment.
When a computed object/class method key contains require?.("./dep") and a later top-level statement reassigns require, the key runs immediately during object/class creation, before that assignment. ObjectMethod and ClassMethod satisfy this blanket t.isFunction(ancestor) predicate, so the call is marked deferred, loaderIntact becomes false, and optional calls have no semantic module-link fallback, dropping a dependency that actually loaded. The new blanket predicate is fresh evidence beyond the earlier top-level reassignment case; restrict deferred classification to method parameter/body regions, matching the semantic index's outer-scope handling for computed keys.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| yield; | ||
| if ( | ||
| !t.isIdentifier(declaration.id, { name: "require" }) || | ||
| !t.isCallExpression(declaration.init) || |
There was a problem hiding this comment.
In TypeScript source such as const require = createRequire(import.meta.url) as NodeRequire; require("./dep"), the type assertion is erased and Node receives the same immutable loader, but this raw t.isCallExpression(declaration.init) check rejects the TSAsExpression. The later call resolves to the program binding, so the unbound-wrapper branch rejects it and semantic require links do not restore it, causing the application graph to omit the real dependency; unwrap erased expression syntax before validating this initializer.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| const requireCalls = yield* unshadowedJavaScriptRequireCallsSteps( | ||
| file, | ||
| undefined, | ||
| yield* collectSemanticReferencesSteps(file.program, bindings), |
There was a problem hiding this comment.
For a large generated source with no require uses, this unfiltered call still materializes every semantic reference in the file, and the classifier then copies every fact into another map before hasRequire can return false; application analysis subsequently collects the full reference set again in its semantic phase. Because source length is not an admission rule and analysis has per-source heap/time budgets, this avoidable O(reference-count) retained copy can turn an otherwise analyzable bundle into a timeout or heap failure; collect only the require and relevant createRequire facts, or reuse the later semantic work.
AGENTS.md reference: AGENTS.md:L46-L50
Useful? React with 👍 / 👎.
morluto
added
bug
Summary
Fixes #2890. Application analysis no longer invents a CommonJS file dependency when a bare
requirecall resolves to a local binding. For example,function run(require) { return require("./ghost.cjs"); }previously created a high-confidence, resolvedimportsedge toghost.cjseven whenrunreceived an ordinary local function.Problem and expected behavior
The static-findings path recognized the callee name while the semantic and reference-source paths already had lexical binding information. Before this change, the public CLI and stdio MCP both return five false imports for parameter, optional-parameter, hoisted-function, catch and block cases in the regression fixture. Real Node runs of those self-authored cases print
localwithout loading the throwingghost.cjscontrol.The corrected graph omits those false edges and retains ordinary and optional CommonJS loaders, wrapper
var require, and directcreateRequire(import.meta.url)calls. Calls before a later program-level reassignment remain visible; deferred calls and writes are not ordered by their textual offsets.Change and scope
Move the existing reference-source loader classifier to the shared JavaScript domain and use it from both consumers. Static analysis reuses its binding index and yields through reference collection, reference indexing, loader inspection, and call selection before visiting static findings.
isUnshadowedRequireCallonly queries the completed set. The reference-source adapter synchronously drives the same stepped classifier; it retains its existing synchronous interface.The shared scan records candidate calls and loader writes together. It preserves calls in earlier program statements instead of applying one file-wide “ever written” flag. Function bodies, instance initializers, unplaced writes, and same-statement ordering stay conservative; source position alone does not establish invocation order. Initialized wrapper
vardeclarations and destructuring writes use the same ordering rule.Bare
requirecalls use this gate; recognized bundle-factory loaders keep their established path. Member-call heuristics are outside this correction. The old reference-specific module is removed, and the formerly separate top-level-var traversal is folded into the shared call/write scan.This covers the cooperative classification, same responsiveness finding, and calls before reassignment review cases.
Ordinary static-analysis correctness fix. Maintainer scope decision: not applicable. This addresses a remaining application-graph consumer after the semantic-origin and reference-source fixes in #509 / #1657; it does not replace those fixes.
Contract and boundary impact
Expected effects:
Evidence and regression coverage
The acceptance regression exercises compiled CLI and stdio MCP, validates their result schema, and executes the self-authored cases with real Node as an independent binding control. It fails through both public interfaces on
3e7c6302and passes after the fix. REA itself continues to inspect target code as data.Positive controls retain all four genuine-loader forms. Existing reference-source, computed-require, dynamic-import, bundle module-index, Electron interop, semantic-link, source-location and large/deep-source cases also pass.
Validation performed
macOS arm64; Node 24.18.0 / npm 11.16.0; updated 2026-10-14:
3e7c6302: 2 failed (CLI and stdio MCP), exposing five false edges.c05f7eb: both public adapters omit the optional load before reassignment. The final cooperative regression also fails on that commit: one yielded step reads 5,000 distinct identifiers, exceeding the two-traversal-chunk bound.varinitialization and destructuring, plus calls/writes in deferred functions and instance fields.referenceSourceShadowedRequire.test.tsandjavascriptStaticAnalysis.cooperative.test.ts).npm run build:cached,npm run check:fast,npm run knip,npm run verify:architecture-guards,npm run verify:test-discovery: passed; 1,090 test files discovered exactly once. Changed-file formatting passes.npm run check: still stops on the existing formatting failures in unchangedsrc/domain/functionComparison.test.tsandsrc/ida/IdaOperationRunner.ts. Separate typecheck/lint, knip and architecture checks complete successfully.The deterministic responsiveness regression measures bounded identifier work at yielded traversal checkpoints rather than asserting a machine-dependent millisecond deadline. Existing scheduler cancellation and real worker lifecycle tests were also run. It does not establish a hard end-to-end cancellation bound.
Cost and scheduling check
Five serialized, alternating before/after runs compare
c05f7ebwith the review correction on the same 138,889-byte source containing 5,000 genuine loader calls. Each run uses a fresh Node process and the compiled production modules; both versions retain all 5,000 references.Wall/CPU intervals exclude import and parsing setup; RSS includes the entire process. These are small source-analysis samples, not agent-token measurements, an end-to-end speedup or a general performance guarantee. The final step includes pre-existing synchronous result/Electron projection outside this require-classification correction; it is shown separately and remains a responsiveness boundary. The deterministic test bounds intermediate traversal work, not that final projection.
Compatibility, safety, and release
No breaking schema or installation change. macOS public CLI/MCP and real Node controls were verified; Windows/Linux and native providers were not run. No Hopper/Ghidra/IDA behavior is claimed. Fixture execution is test-only and uses self-authored sources.
Before merge
Prepared with AI assistance; all reported executions were run locally.