Skip to content

Replace node --version probe with no-fork PATH walk #1129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justin808 merged 1 commit into from
May 24, 2026
Merged

Replace node --version probe with no-fork PATH walk #1129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion lib/install/bin/diff-bundler-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,24 @@ end

def shakapacker_node_binary
node_bin = "node"
return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL)
# Walk ENV["PATH"] directly instead of forking a `node --version`
# 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?
["", *(ENV["PATHEXT"] || ".COM;.EXE;.BAT;.CMD").split(";")]
else
[""]
end

ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir|
next if dir.empty?

extensions.each do |ext|
candidate = File.join(dir, "#{node_bin}#{ext}")
return node_bin if File.file?(candidate) && File.executable?(candidate)
end
end

warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \
"Install Node.js and try again."
Expand Down
19 changes: 18 additions & 1 deletion lib/install/bin/shakapacker-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,24 @@ end

def shakapacker_node_binary
node_bin = "node"
return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL)
# Walk ENV["PATH"] directly instead of forking a `node --version`
# 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?

@claude claude Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

["", *(ENV["PATHEXT"] || ".COM;.EXE;.BAT;.CMD").split(";")]

@claude claude Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else
[""]
end

ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir|

@chatgpt-codex-connector chatgpt-codex-connector Bot May 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve executable lookup when PATH is unset

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 👍 / 👎.

next if dir.empty?

@chatgpt-codex-connector chatgpt-codex-connector Bot May 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve current-directory PATH segments during lookup

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 👍 / 👎.


extensions.each do |ext|
candidate = File.join(dir, "#{node_bin}#{ext}")
return node_bin if File.file?(candidate) && File.executable?(candidate)

@claude claude Bot May 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@claude claude Bot May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \
"Install Node.js and try again."
Expand Down
19 changes: 18 additions & 1 deletion package/configExporter/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,24 @@ end

def shakapacker_node_binary
node_bin = "node"
return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL)
# Walk ENV["PATH"] directly instead of forking a \`node --version\`
# 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?
["", *(ENV["PATHEXT"] || ".COM;.EXE;.BAT;.CMD").split(";")]
else
[""]
end

ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir|
next if dir.empty?

extensions.each do |ext|
candidate = File.join(dir, "#{node_bin}#{ext}")
return node_bin if File.file?(candidate) && File.executable?(candidate)
end
end

warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \\
"Install Node.js and try again."
Expand Down
19 changes: 18 additions & 1 deletion spec/dummy/bin/shakapacker-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,24 @@ end

def shakapacker_node_binary
node_bin = "node"
return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL)
# Walk ENV["PATH"] directly instead of forking a `node --version`
# 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?
["", *(ENV["PATHEXT"] || ".COM;.EXE;.BAT;.CMD").split(";")]
else
[""]
end

ENV["PATH"].to_s.split(File::PATH_SEPARATOR).each do |dir|
next if dir.empty?

extensions.each do |ext|
candidate = File.join(dir, "#{node_bin}#{ext}")
return node_bin if File.file?(candidate) && File.executable?(candidate)
end
end

warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \
"Install Node.js and try again."
Expand Down
Loading