Conversation
Both of these wrappers were being stamped out from templates, but each instantiation was identical to all the others. The npm wrapper did nothing but exec the npm binary and pass along all command-line arguments, so this commit drops that wrapper altogether. As long as we put the npm binary on the `PATH`, we can just call it directly instead. The node wrapper is still useful because it adds an important `--require` flag, but this commit sets up a global wrapper instead of having every `js_binary` stamp out its own copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
marked this pull request as ready for review
August 13, 2026 18:24
acozzette
commented
Aug 13, 2026
![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: 8269b5962b
ℹ️ 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".
jbedard
approved these changes
Aug 13, 2026
The launcher puts the directory holding JS_BINARY__NODE_WRAPPER on the PATH so children resolve `node` to the fs-patched wrapper. That only holds if the directory contains a single PATH-resolvable `node`: a POSIX shell picks the extensionless file, cmd.exe picks node.bat/.cmd/.exe via PATHEXT. Under runfiles only the selected wrapper is a runfile, so this passes. In the JS_BINARY__NO_RUNFILES configuration the launcher resolves the wrapper from the source directory in the execroot, which holds both `node` and `node.bat`, and the test fails. The existing spawn tests cannot cover this on Windows: they assert `which node == process.execPath`, but bootstrap.cjs skips the process.execPath override on win32, so they fail there for unrelated pre-existing reasons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Runs //js/private/test/node_bin:node_bin_path_test on windows-latest, once with --noenable_runfiles (expected to FAIL: this is the reproduction) and once with --enable_runfiles (expected to PASS, isolating the failure to the no-runfiles path). Runs the single target rather than //... because the root workspace is excluded from the smoke matrix on Windows due to broad pre-existing failures. Deliberately left out of the `conclusion` needs list so an intentionally-red diagnostic does not block the merge gate. Drop this commit, along with the temporary smoke gate removal, before merging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The launcher puts the wrapper's directory on the PATH. Under JS_BINARY__NO_RUNFILES
(Windows with enable_runfiles False) that directory is the source directory in the
execroot, which holds every file in the package -- so a shared directory made both `node`
and `node.bat` resolvable as `node`, and a POSIX-shell child could resolve the
extensionless wrapper while the launcher had selected node.bat.
Reproduced on CI before the fix, with node_bin_path_test under --noenable_runfiles:
node wrapper: D:/_bazel/execroot/_main/js/private/node_bin/node.bat
PATH directory: D:/_bazel/execroot/_main/js/private/node_bin
node candidates: ["node","node.bat"]
Only the runfiles case was safe, since just the selected wrapper is a runfile there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
custom_layers_test_app.listing has no target: the js_image_layer it belonged to was renamed to custom_layers_nomatch, and it is missing the _node / _node_modules / _package_store_* siblings every other listing group has. Nothing asserts against it, so it had silently gone stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Putting the Node.js toolchain's bin directory on the PATH avoided the per-target wrapper action, but it put more than npm there. Restore the wrapper as a shared source file, the same way the node wrapper works, which keeps the action saving (TemplateExpand actions across //examples/... stay at 142, down from 240 before this branch) while fixing: * PATH pollution. nodeinfo.npm may be an absolute path from a custom node_toolchain, so a js_binary using a non-hermetic npm was prepending e.g. /usr/local/bin ahead of everything the caller had on the PATH, for the binary and its whole child-process tree. The wrapper directory holds only the wrapper. * An unpatched node on the PATH. The toolchain's bin directory also holds its own node launcher, which does not apply the fs patches. That made the order of the two PATH prepends load-bearing; it no longer is, since neither directory can shadow the other. Each wrapper keeps its own directory for the reason the node wrappers were split: under JS_BINARY__NO_RUNFILES the directory on the PATH is the source directory in the execroot, so anything sharing it is on the PATH too. A shared directory would put `npm` on the PATH even without include_npm. node_bin_path_test now checks both wrappers and gains an include_npm variant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
changed the title
The launcher's JS_BINARY__NO_RUNFILES branch resolves the node and npm wrappers from the source directory in the execroot rather than from runfiles, which is how both wrappers ended up PATH-resolvable as `node` in the same directory. Nothing covered it: the tests that would catch it live in the root workspace, which is excluded from the smoke matrix on Windows because of broad pre-existing failures there. Give the matrix workspace entries `flags` and `targets`, so a Windows entry can run just //js/private/test/node_bin:all from the root workspace under --noenable_runfiles instead of the whole //... . The temporary standalone job that reproduced the bug is removed, since this supersedes it. Matrix jobs are now named by slug rather than path, so the new entry does not render as `smoke (., windows-latest)` -- the combination the exclude above it removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restores .github/workflows/ci-workflows.yaml to its state on main: the smoke job's branch gate is back, along with its original job naming and matrix, and the temporary Windows job is gone. Drops the plan to cover the JS_BINARY__NO_RUNFILES launcher path on Windows from CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This test is not particularly useful because it covers a Windows issue that we can't easily cover in CI.
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
2 participants
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.
Both of these wrappers were being stamped out from templates, one per js_binary target. Since each instantiation is identical to all the others, let's just have a single global copy.
This change also removes js/private/test/image/custom_layers_test_app.listing, which appears to be unreferenced.
Changes are visible to end-users: no
Test plan