Replace node --version probe with no-fork PATH walk#1129
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryReplaces the
Confidence Score: 5/5Safe to merge — the PATH walk is a straightforward drop-in for the subprocess probe with no change to the return value or downstream exec call. The substitution is mechanically simple: find a file named No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[binstub invoked] --> B[shakapacker_node_binary]
B --> C{Gem.win_platform?}
C -- Yes --> D["extensions = ['', ...PATHEXT.split(';')]"]
C -- No --> E["extensions = ['']"]
D --> F[Walk ENV PATH directories]
E --> F
F --> G{dir empty?}
G -- Yes --> H[skip]
H --> F
G -- No --> I[For each ext in extensions]
I --> J["candidate = File.join(dir, 'node' + ext)"]
J --> K{File.file? AND File.executable?}
K -- Yes --> L[return 'node']
K -- No --> I
I -- exhausted --> F
F -- exhausted --> M["warn: Could not find Node.js"]
M --> N[exit 1]
L --> O["exec node_bin, script_path, *ARGV"]
Reviews (1): Last reviewed commit: "Replace `node --version` probe with a no..." | Re-trigger Greptile |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| extensions.each do |ext| | ||
| candidate = File.join(dir, "#{node_bin}#{ext}") | ||
| return node_bin if File.file?(candidate) && File.executable?(candidate) |
There was a problem hiding this comment.
Subtle behavioral change worth calling out: system("node", "--version") confirmed the binary was actually runnable — a wrong-arch binary (e.g., x86 node on an ARM machine), corrupted file, or empty-file-with-executable-bit would have failed the old probe and produced Shakapacker's friendly error. With the PATH walk, those same cases silently pass detection and the error surfaces later as a raw OS exec failure (Errno::ENOEXEC or similar) with no Shakapacker context.
Whether that tradeoff is acceptable depends on how likely those scenarios are in practice. For most dev environments it'll never matter, but it's worth a comment or doc note if this is intended.
| # subprocess on every binstub invocation. On Windows, also probe | ||
| # PATHEXT-style extensions because executables there end in | ||
| # .exe/.cmd/.bat. | ||
| extensions = if Gem.win_platform? |
There was a problem hiding this comment.
When ENV["PATHEXT"] is set but empty (e.g. PATHEXT=""), "".split(";") returns [], so extensions becomes [""] — only the bare node name is checked, and node.exe / node.cmd are skipped. Practically this won't happen on any real Windows install (empty PATHEXT breaks far more things), but if you want to be defensive the fallback condition could be:
| extensions = if Gem.win_platform? | |
| extensions = if Gem.win_platform? | |
| pathext = ENV["PATHEXT"].to_s | |
| pathext = ".COM;.EXE;.BAT;.CMD" if pathext.empty? | |
| ["", *pathext.split(";")] |
Not a blocker — just a heads-up.
Review: Replace node --version probe with no-fork PATH walkOverall: Clean, well-motivated optimization with good test coverage (including the Behavioral nuance (minor)
This is a reasonable tradeoff since exec failures in those cases are still clear enough in practice — just flagging it as intentional vs. oversight. Windows PATHEXT with empty string (very minor)If Other observations
No blockers. The optimization is valid and the implementation is correct for the common case. |
![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: c11c5d94b8
ℹ️ 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".
| [""] | ||
| end | ||
|
|
||
| ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir| |
There was a problem hiding this comment.
This PATH walk changes behavior versus the previous system("node", "--version") probe: if PATH is unset, ENV["PATH"].to_s becomes "", so the loop never checks anything and the binstub exits with “Could not find Node.js executable.” The previous implementation delegated lookup to process execution semantics, which can still resolve commands via the platform default search path (commonly including /usr/bin), so this introduces a false negative in sanitized environments that clear PATH. The same regression is duplicated in the other synced helper copies.
Useful? React with 👍 / 👎.
justin808
force-pushed
the
jg/1123-binstub-sync-coverage
branch
from
27a0e53 to
e1af730
Compare
Address the low-priority polish item from the PR #1104 review. `shakapacker_node_binary` previously verified Node was installed by running `system("node", "--version")`, which forks a subprocess on every binstub invocation just to confirm presence. Replace with a direct ENV["PATH"] walk, falling back to PATHEXT-style extensions on Windows. Cuts a fork+exec from every `bin/shakapacker-config` and `bin/diff-bundler-config` call. Applied identically across the three checked-in helper binstubs (lib/install/bin/shakapacker-config, lib/install/bin/diff-bundler-config, spec/dummy/bin/shakapacker-config) and the createBinStub template in package/configExporter/cli.ts. binstub_sync_spec.rb + test/configExporter/createBinStub.test.js (from PR #1128) keep the four copies byte-aligned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg/1123-node-binary-path-walk
branch
from
c11c5d9 to
1889de4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1889de42f1
ℹ️ 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".
| end | ||
|
|
||
| ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir| | ||
| next if dir.empty? |
There was a problem hiding this comment.
Skipping empty PATH segments changes command-resolution semantics versus the previous system("node", "--version") probe: in POSIX, an empty segment in PATH (e.g. :/usr/bin or /usr/bin:) means “search the current directory.” With next if dir.empty?, the binstub now reports Node missing when node is only discoverable via that current-directory segment, even though the subsequent exec "node", ... would have found it before this change. This is a behavioral regression in environments that rely on empty PATH entries.
Useful? React with 👍 / 👎.
Code ReviewOverall this is a clean, well-structured optimization. The change is applied byte-identically across all four synced copies, test coverage is solid (including the Behavioral change worth acknowledgingThe old
This is unlikely to affect real users, but it is a real regression in error-message quality worth calling out — particularly since the original Open-source maintainability trade-offThe PR description labels this a low-priority optimization. The new code is ~20 lines replacing 1 line and introduces Windows-specific conditional logic. Per the project's own open-source guidelines ("prefer removing complexity over adding it"), the trade-off deserves explicit sign-off: the performance gain is real but narrow in impact, while the added complexity is permanent. This is not a blocker, just something for the reviewer to weigh consciously. Windows
|
|
|
||
| extensions.each do |ext| | ||
| candidate = File.join(dir, "#{node_bin}#{ext}") | ||
| return node_bin if File.file?(candidate) && File.executable?(candidate) |
There was a problem hiding this comment.
Subtle semantic change from the old system("node", "--version"): this verifies the binary exists and has the executable bit set, but not that it is actually runnable. A corrupted binary, an architecture-incompatible binary, or a broken shim will pass this check and then fail at exec time with an OS-level error rather than Shakapacker's helpful [Shakapacker] Could not find Node.js executable message. Worth documenting as an accepted trade-off.
| # PATHEXT-style extensions because executables there end in | ||
| # .exe/.cmd/.bat. | ||
| extensions = if Gem.win_platform? | ||
| ["", *(ENV["PATHEXT"] || ".COM;.EXE;.BAT;.CMD").split(";")] |
There was a problem hiding this comment.
The "" first element intentionally checks node without any extension before trying the PATHEXT variants — this mirrors how the Windows CreateProcess API works (no-extension is tried first). The fallback .COM;.EXE;.BAT;.CMD is reasonable, though modern Windows typically also includes .PS1 in PATHEXT. Node.js ships as .exe so this covers the real-world case correctly.
justin808
merged commit 4749299
into
jg/1123-binstub-sync-coverage
1 participant
Summary
Address the low-priority optimization from the PR #1104 follow-up review.
This is PR 3 of 3 for #1123 — the smallest of the three, isolated so it can land or be punted independently.
Change
shakapacker_node_binaryin the helper binstubs previously verified Node was installed by runningsystem("node", "--version")with both streams piped to/dev/null. That forks a subprocess on every binstub invocation — just to learn whether Node exists. Replaced with a direct walk ofENV["PATH"]that also probesPATHEXT-style extensions on Windows (sonode.exe/node.cmdare still discoverable). One fork+exec saved perbin/shakapacker-configandbin/diff-bundler-configcall.The return value is unchanged (the literal string
"node"), so the subsequentexec node_bin, script_path, *ARGVstill goes through the OS'sPATHresolution exactly as before.Applied byte-identically across all four copies kept in sync by PR #1128 (
binstub_sync_spec.rb+test/configExporter/createBinStub.test.js):lib/install/bin/shakapacker-configlib/install/bin/diff-bundler-configspec/dummy/bin/shakapacker-configcreateBinStubtemplate inpackage/configExporter/cli.tsStacked on PR #1128
This branch is stacked on top of
jg/1123-binstub-sync-coverage(PR #1128). GitHub will auto-update the base tomainonce #1128 merges. If #1128 is closed instead, this PR can rebase ontomainwith a trivial change (only the comment header andNODE_ENVline differ, and only the comment-header lines need replaying).Refs #1123.
Test plan
bundle exec rspec spec/shakapacker/binstub_sync_spec.rb— 2 examples pass (dummy↔install parity preserved)bundle exec rspec spec/shakapacker/helper_binstubs_spec.rb— 8 examples pass, including the "node unavailable" path that usesPATH=/nonexistentyarn jest test/configExporter/createBinStub.test.js— 2 examples pass (template still matches install templates byte-for-byte)bundle exec rubocop— no offenses on changed Ruby filesyarn eslint— no errors on changed TS fileruby -con all three modified binstubs — syntax OK🤖 Generated with Claude Code
Note
Medium Risk
Touches the helper binstubs used to launch Node-based tooling; any bug in the new
PATH/PATHEXTscan could prevent commands from running, especially on Windows.Overview
Replaces
shakapacker_node_binary’ssystem("node", "--version")check with a no-fork scan ofENV["PATH"], including WindowsPATHEXTextension probing.Applies the same updated Node lookup logic across all four synced helper-binstub copies (
lib/install/bin/*,spec/dummy/bin/*, and thecreateBinStubtemplate inpackage/configExporter/cli.ts).Reviewed by Cursor Bugbot for commit 1889de4. Bugbot is set up for automated code reviews on this repo. Configure here.