Repository navigation
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: 1879b07a0b
ℹ️ 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 (metadata.kind !== "absent") | ||
| return resolvePackageSpecifier(input, candidate, subpath, metadata); |
There was a problem hiding this comment.
When a CommonJS bare specifier contains a literal % (for example, require("fi%xture")) and both node_modules/fi%xture.js and node_modules/fi%xture/package.json exist, Node skips package-exports resolution because its CJS exports matcher excludes %, then selects the sibling file. These specifiers pass the existing admission checks, so this unconditional metadata return instead selects or rejects according to package.json, producing a graph result that disagrees with the runtime; gate metadata preemption with the same exports-package-name eligibility before bypassing file-first lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949bce39ea
ℹ️ 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".
| while (true) { | ||
| const candidate = posix.join(directory, "node_modules", packageName); | ||
| const metadata = readPackageMetadata(input, candidate); | ||
| if (packageExportsEligible(input) && metadata.kind !== "absent") |
There was a problem hiding this comment.
When node_modules/example/package.json has a scalar root "exports": false (or a number) beside node_modules/example.js, Node 24 ignores that root value and continues to the sibling file. readPackageMetadata leaves the scalar intact, so this new early return passes it to selectExportTarget, reports rejected, and never executes the file-first block below; normalize package metadata with Node's field semantics before deciding whether exports preempts fallback.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| ): CandidateResolution => { | ||
| const metadata = readPackageMetadata(input, directory); | ||
| if (metadata.kind === "unavailable") | ||
| const exportsEligible = packageExportsEligible(input); |
There was a problem hiding this comment.
When analyzing node_modules/fi%xture/run.cjs inside a package such as { "name": "fi%xture", "exports": "./actual.cjs", "main": "./legacy.cjs" }, require("fi%xture") takes Node's package self-resolution path, which consults its own exports without the ordinary CommonJS exports-name regex. Reusing packageExportsEligible here therefore suppresses the valid export and falls through to main/index, so Node selects actual.cjs while the graph selects legacy.cjs; retain source-package self handling before applying this gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08a140201d
ℹ️ 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".
| }; | ||
|
|
||
| const packageExportsEligible = (input: ResolveArtifactPathInput): boolean => { | ||
| if (input.moduleKind !== "require") return true; |
There was a problem hiding this comment.
When an import bare specifier has % in its package-name segment (for example, import("fi%xture")), Node 24 rejects it with ERR_INVALID_MODULE_SPECIFIER before looking at package.json. Returning true for every non-CommonJS request lets the new metadata-preemption path resolve node_modules/fi%xture/actual.cjs (including when a same-named sibling file is present), so the analyzed graph records a module edge that cannot exist at runtime; only percent signs in the subpath of an otherwise valid package name should remain eligible.
Useful? React with 👍 / 👎.
N0zoM1z0
left a comment
N0zoM1z0
left a comment
There was a problem hiding this comment.
Independent review of 21685762c03c5d174aa07d9412d45244b42bf866, integrated locally with main e90acaea674f5f7f9f17ea472104afdc1422ada6 on an isolated branch.
I reproduced the four wrong edges on fresh main 41544fb952aef84fcf6ff5d9a5284cf2b2985053 using a 12-file, 436-byte target. Native Node 24.18.0 resolution selected the exported file, refused the blocked root, selected the package self export, and rejected the percent-containing ESM package name. Compiled public CLI and real stdio SDK MCP both disagreed before this change and agreed after it. Entire Evidence envelopes were authenticated and normalized CLI/MCP results matched; caller specifiers, source digests and ranges were checked. Targets were not executed.
Local validation passed: 22 files / 376 tests (274 application tests, 82 package-export boundary cases, four public CLI/MCP acceptance tests, and 14 cancellation cases), single-threaded compile and typecheck, lint/module boundaries, changed-file formatting and architecture guards. Flameox 0.2.9 retained complete test output and the native V8 launcher profile. That profile does not establish child-worker CPU or allocation behavior. Native Windows and complete Node loader compatibility were not independently verified.
The CI shard-2 failure is in the unrelated process-capture cleanup journey (localToolContracts.test.ts): an environment ownership-token read returned EACCES for a live candidate, producing cleanup_incomplete. Package acceptance tests in that job passed. I am investigating this separately and waiting for all CI checks; I have not merged this PR.
This branch has not been deployed
2 participants
Summary
Fixes #1761.
JavaScript analysis can resolve
require("fixture")orimport ... from "fixture"tonode_modules/fixture.jseven whenfixture/package.jsonexports a different file or rejects the request. Consult package exports at the appropriate lookup stage while preserving Node's root metadata semantics, CommonJS legacy lookup and package self-reference precedence.Problem and expected behavior
With both
node_modules/fixture.jsand a package declaring"exports": "./actual.cjs", Node selectsnode_modules/fixture/actual.cjs; REA previously reported the sibling as resolved. Blocked, missing, and invalid exports also incorrectly produced successful sibling edges.The Node resolution algorithm checks eligible package exports before file and directory fallback. Absent/null exports and ignored boolean/numeric root values retain CommonJS file-first behavior. Ordinary CommonJS dependency lookup excludes literal
%from exports package names, so an outsiderequire("fi%xture")selectsnode_modules/fi%xture.js. A matching self-reference from inside that named package instead uses its exports before dependency lookup, even when the name contains%.Change and scope
Normalize package metadata once when reading it: retain string/object root exports and leave nested target values intact for validation. Read only same-container metadata. Before CommonJS dependency lookup, check the source's nearest package scope for an exact-name or subpath self-reference; stop at the first package metadata or a
node_modulesboundary. Self-reference exports share the existing target selection and refusal handling.For ordinary dependency lookup, apply Node's CommonJS exports-name eligibility before metadata can preempt a sibling file; compare the matched name with the parsed package name to handle scoped names correctly. Ineligible names retain legacy file/directory lookup, including
mainfallback and subpaths whose parent package metadata is invalid. Explicit ESM imports reject percent signs anywhere in the package name before metadata or sibling lookup. Percent-containing subpaths of eligible packages still use exports. Preserve exact target selection, refusal outcomes, directory lookup, and source provenance. ESM self-resolution is outside this change.Contract and boundary impact
Evidence and regression coverage
mainfallback, subpaths with malformed parent metadata, and percent subpaths of eligible packages.node_modules/@scope/selected/actual.cjs; a blocked root returnsresolved_path: null,resolution_status: "external"; an outsiderequire("fi%xture")resolves tonode_modules/fi%xture.js, while the package's own reference selectsnode_modules/fi%xture/actual.cjs.For evidence-bearing changes:
Validation performed
npm run test:focused -- tests/boundary/javascript/packageConditionalExports.test.ts tests/acceptance/applications/packageSubpathsCli.test.ts— 84 passed.npm run test:local -- src/application/javascript/*.test.ts— 17 files, 274 passed.npm run check— typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards passed; lint reported 728 warnings and zero errors.npm run check:fast— passed.git diff --checkand independent correctness/security review — passed.Executed on Ubuntu/WSL x64 with pinned Node 24.18.0 and npm 11.16.0.
Compatibility, safety, and release
Review checklist