Repository navigation
fix(javascript): retain unbound CommonJS semantic imports - #1799
Merged
Merged
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. |
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unbound literal CommonJS calls now contribute semantic module dependencies and the source module's import count. Existing static asset references remain available.
Fixes #1793.
Problem and expected behavior
With
main.cjscontainingrequire("./dependency.cjs");, main3ad5782dreturns one static require asset edge but zero semantic require module edges andimport_relationships: 0. An equivalent side-effect ESM import has a semantic module edge. The fixed CLI and stdio MCP return one semantic require edge andimport_relationships: 1, with the dependency path resolved and the original source digest/range retained.Change and scope
Collect unbound calls at the existing semantic projection owner. Its existing parser still decides whether a call is an unshadowed literal require. Keep the exact call node private to analysis and mark calls already represented by a binding or CommonJS export in a WeakSet. This avoids extra links for bound calls while retaining distinct nested calls, empty destructures, assignments and other unbound expressions.
Binding names remain null where no binding is known. Dynamic specifiers and shadowed calls remain excluded; dynamic member paths keep unknown binding provenance. Existing alias, empty-key and re-export precision is preserved. Move the existing alias/assignment case into the focused require module and update two goldens to retain dependencies without inventing dynamic bindings.
Contract and boundary impact
Evidence and regression coverage
Executed reproduction: a 101-byte, three-file target through freshly compiled public CLI and real SDK stdio MCP, on both latest merged main and the fixed branch. Native Node resolves the dependency without executing the fixture. Complete Evidence envelopes authenticate; normalized CLI/MCP results match, MCP text/structured content matches, and a subsequent ping succeeds.
Regression coverage: 19 new semantic cases and two compiled public journeys; existing binding/provenance coverage retained, including deep members and empty keys. The initial source/public regression run fails 15 cases on the affected producer and passes six controls.
Remaining proof gaps: Linux verification only; no real Ghidra, IDA, Hopper or browser workflow is involved.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.
Validation performed
Node 24.18.0 / npm 11.16.0, two CPUs, supervised 2 GiB process-family RSS limit:
npm run compile -- --singleThreaded— passed on exact head92e2ddfd2359209e36f1f528db48ee87dd96a160, based on merged main3ad5782d.--maxWorkers=1— 50 semantic/composition files with 890 passing cases, and 19 application/cancellation files with 297 passing cases.npm run typecheck -- --singleThreadedwithGOMEMLIMIT=768MiB,npm run lint(including module boundaries), changed-fileoxfmt --check,npm run verify:architecture-guards, andgit diff --check— all passed.Flameox capture workload exits and payload digests were checked; the profiles cover the Vitest launcher, so they establish neither child-worker CPU nor allocation improvements. The threaded aggregate hooks were bypassed with
HUSKY=0; the checks above ran explicitly within the resource budget. Full CI remains required before merge.Compatibility, safety, and release
Review checklist
type(scope): outcome.