Fix legacy JS binstub dispatch and NODE_ENV default in helper binstubs (#1127)

## Summary

Address the **high-priority bug fixes** from the [PR #1104 follow-up
review](#1123) on
ESM-safe config binstubs.

This is **PR 1 of 3** for #1123 — focused on user-visible regressions.
Test-coverage and polish items will follow in separate PRs to keep
review scope tight.

### Fixes

- **#1 — rake task crash for upgraded apps with a legacy JS binstub.**
`shakapacker:export_bundler_config` previously invoked
`bin/shakapacker-config` via `RbConfig.ruby` unconditionally. Apps
upgraded from older Shakapacker versions still carry a `#!/usr/bin/env
node` JavaScript binstub that this gem did not rewrite — running it
through Ruby would try to parse JS as Ruby and crash. The task now
detects the shebang and dispatches: Ruby binstubs keep the
version-manager protection from #1104; legacy JS binstubs are exec'd
directly so their shebang resolves Node, and the user is nudged to run
`rake shakapacker:binstubs` to upgrade. A new
`Shakapacker::Utils::Misc.js_binstub?` helper makes the detection
unit-testable.

- **#2 — `NODE_ENV` default ignored `RAILS_ENV` in helper binstubs.**
The helper binstubs hard-coded `ENV["NODE_ENV"] ||= "development"`,
which diverged from `Shakapacker.ensure_node_env!`. Concretely,
`RAILS_ENV=production bin/shakapacker-config show-config` with
`NODE_ENV` unset spawned Node with `NODE_ENV=development` and produced
the development bundler config. The default now mirrors the Ruby logic
(`DEV_ENVS = %w[development test]`): only development and test default
to `NODE_ENV=development`; everything else defaults to `production`.
Applied in all four sync'd copies: `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`.

- **#6 — `error.message` guard.** `package/bin/shakapacker-config.cjs`
and `bin/shakapacker-config` now stringify non-`Error` throws (`error
instanceof Error ? error.message : String(error)`) instead of printing
`undefined`, matching the existing `formatError` pattern in
`diff-bundler-config.cjs`.

- **#7 — `NODE_ENV` default in `diff-bundler-config`.** Resolved by #2's
fix: kept the default (now using the corrected logic) with a comment
explaining why it stays consistent with `shakapacker-config`.

### Follow-up PRs

Items deferred to keep this PR focused:

- **PR 2** — test-coverage gaps: dummy binstub sync coverage (#3),
`createBinStub` template sync spec (#4), fallback-warning assertion
(#5), "Keep in sync" comments updated to list all four locations (#8),
`binstub_sync_spec` test rename (#9).
- **PR 3** — polish: replace `system(node, "--version")` probe with a
no-fork PATH walk (#10).

Refs #1123.

## Test plan

- [x] `bundle exec rspec spec/shakapacker/utils_misc_spec.rb` — new unit
tests for `js_binstub?` (8 examples)
- [x] `bundle exec rspec spec/shakapacker/helper_binstubs_spec.rb` —
existing helper binstub specs still pass (8 examples)
- [x] `bundle exec rspec spec/shakapacker/binstub_sync_spec.rb` — sync
spec still passes
- [x] `bundle exec rspec spec/shakapacker/doctor_spec.rb` — doctor specs
(147 examples) still pass
- [x] `bundle exec rubocop` — no offenses on changed Ruby files
- [x] `yarn eslint` — no errors on changed JS/TS files
- [x] `ruby -c` on all modified Ruby binstubs/rake task — syntax OK

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Medium risk because it changes how rake tasks and binstubs choose
interpreters and default environment variables, which can affect
build/config export behavior across environments.
> 
> **Overview**
> **Fixes config-export helper regressions for upgraded apps.**
`shakapacker:export_bundler_config` now detects legacy `#!/usr/bin/env
node` `bin/shakapacker-config` shims and execs them directly (while
still running modern Ruby binstubs via `RbConfig.ruby`), avoiding
crashes and prompting users to regenerate binstubs.
> 
> **Aligns environment defaults across helper binstubs.**
`shakapacker-config`/`diff-bundler-config` now default `NODE_ENV` to
`development` only for `RAILS_ENV` in `development|test`, otherwise
`production`, and both Node shims now stringify non-`Error` throws for
clearer stderr. Adds a unit-tested
`Shakapacker::Utils::Misc.js_binstub?` helper and updates the changelog.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
08c3c39. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Fixed a regression when exporting bundler config after upgrades from
older installations.
* Improved detection of legacy JavaScript helper scripts (including
`env`-style shebangs) and now runs the appropriate Node or Ruby
interpreter.
* Enhanced error handling and messages when the JavaScript or Ruby
runtime can’t be executed, including upgrade guidance to refresh helper
scripts.
* **Tests**
* Expanded coverage for helper-script interpreter detection, guidance
output, and failure scenarios (including missing runtimes and varied
shebang formats).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: justin808

CHANGELOG.md

@@ -22,6 +22,7 @@
 ### Fixed
 
 - **Fixed dev-server passthrough argument validation messages**. [PR #1180](https://github.com/shakacode/shakapacker/pull/1180) by [justin808](https://github.com/justin808). `bin/shakapacker-dev-server -- --host ...` or `--port ...` now fails with a clearer Shakapacker message instead of forwarding the separator to the bundler; set `dev_server.host` and `dev_server.port` in `config/shakapacker.yml` instead.
+- **Fixed `shakapacker:export_bundler_config` regression on apps upgraded from older Shakapacker versions**. [PR #1127](https://github.com/shakacode/shakapacker/pull/1127) by [justin808](https://github.com/justin808). The task previously invoked `bin/shakapacker-config` unconditionally via `RbConfig.ruby`, which crashed when the file was still the legacy JavaScript binstub (`#!/usr/bin/env node`) left over from earlier installs. The task now inspects the shebang and exec's the file directly when it points at Node, while keeping the Ruby+`RbConfig.ruby` path for the current Ruby binstub. Affected users are also nudged to run `bundle exec rake shakapacker:binstubs` to refresh their helper binstub. Refs [#1123](https://github.com/shakacode/shakapacker/issues/1123).
 - **Fixed `shakapacker:doctor` Sass implementation detection for `sass-embedded` apps**. [PR #1178](https://github.com/shakacode/shakapacker/pull/1178) by [justin808](https://github.com/justin808). The doctor now accepts either modern Sass implementation package (`sass` or `sass-embedded`) when Sass files are present, matching Shakapacker's default modern Sass API instead of incorrectly requiring Dart Sass. New installs also include `sass` alongside `sass-loader`. Fixes [#1172](https://github.com/shakacode/shakapacker/issues/1172).
 - **Fixed compiler strategies ignoring the instance config of custom `Shakapacker::Instance` objects.** [PR #1147](https://github.com/shakacode/shakapacker/pull/1147) by [justin808](https://github.com/justin808). Ports [#976](https://github.com/shakacode/shakapacker/pull/976) by [brunodccarvalho](https://github.com/brunodccarvalho). Strategies now read the instance-specific config and watch both webpack and rspack config directories.
 

lib/shakapacker/utils/misc.rb

@@ -2,12 +2,18 @@
 
 require "English"
 require "rake/file_utils"
+require "shellwords"
 
 module Shakapacker
   module Utils
     class Misc
       extend FileUtils
 
+      NODE_BINSTUB_EXECUTABLES = %w[node nodejs].freeze
+      ENV_FLAGS_WITH_ARGUMENTS = %w[-u --unset -C --chdir -P --path -a --argv0].freeze
+      private_constant :NODE_BINSTUB_EXECUTABLES
+      private_constant :ENV_FLAGS_WITH_ARGUMENTS
+
       def self.uncommitted_changes?(message_handler)
         return false if ENV["COVERAGE"] == "true"
 
@@ -31,6 +37,38 @@ def self.object_to_boolean(value)
       def self.sh_in_dir(dir, *shell_commands)
         shell_commands.flatten.each { |shell_command| sh %(cd '#{dir}' && #{shell_command.strip}) }
       end
+
+      def self.js_binstub_executable(path)
+        return nil unless File.file?(path)
+
+        shebang = File.open(path, "rb") { |f| f.gets }.to_s.chomp
+        return nil unless shebang.start_with?("#!")
+
+        shebang_tokens = begin
+          Shellwords.split(shebang.delete_prefix("#!"))
+        rescue ArgumentError
+          return nil
+        end
+        executable = shebang_tokens.first.to_s
+
+        if File.basename(executable) == "env"
+          shebang_tokens = shebang_tokens.drop(1)
+          while (env_token = shebang_tokens.first)
+            if env_token.start_with?("-")
+              env_flag = shebang_tokens.shift
+              shebang_tokens.shift if ENV_FLAGS_WITH_ARGUMENTS.include?(env_flag)
+            elsif env_token.match?(/\A[A-Za-z_][A-Za-z0-9_]*=/)
+              shebang_tokens.shift
+            else
+              break
+            end
+          end
+          executable = shebang_tokens.first.to_s
+        end
+
+        # Preserve direct interpreter paths so stale absolute Node shebangs fail with actionable binstub guidance.
+        NODE_BINSTUB_EXECUTABLES.include?(File.basename(executable)) ? executable : nil
+      end
     end
   end
 end

lib/tasks/shakapacker/export_bundler_config.rake

@@ -1,21 +1,6 @@
-namespace :shakapacker do
-  def shakapacker_config_binstub_command(bin_path)
-    # Read in binary mode so Windows CRLF line endings do not leak \r into the shebang.
-    shebang = File.open(bin_path, "rb", &:gets).to_s
-    command = shebang.delete_prefix("#!").strip.split(/\s+/)
-    executable = File.basename(command.first.to_s)
-
-    if executable == "env"
-      executable = File.basename(command.drop(1).find { |part| !part.start_with?("-") }.to_s)
-    end
-
-    # Legacy JS binstubs are dispatched via PATH lookup; Kernel#exec resolves
-    # "node" through the shell's PATH. The Ruby binstubs perform their own
-    # explicit lookup via shakapacker_find_executable, which matters more on
-    # Windows where PATHEXT is involved.
-    executable == "node" ? ["node", bin_path.to_s] : [RbConfig.ruby, bin_path.to_s]
-  end
+require "shakapacker/utils/misc"
 
+namespace :shakapacker do
   desc <<~DESC
     Export webpack or rspack configuration for debugging and analysis
 
@@ -62,7 +47,7 @@ namespace :shakapacker do
     # Try to use the binstub if it exists, otherwise use the gem's version
     bin_path = Rails.root.join("bin/shakapacker-config")
 
-    unless File.exist?(bin_path)
+    unless File.file?(bin_path)
       # Binstub not installed, use the gem's version directly
       gem_bin_path = File.expand_path("../../install/bin/shakapacker-config", __dir__)
 
@@ -75,10 +60,34 @@ namespace :shakapacker do
       end
     else
       # Pass through command-line arguments after the task name.
-      # Ruby binstubs run under the same Ruby as Rake; legacy JavaScript
-      # binstubs from upgraded apps still need Node until users refresh them.
+      #
+      # Modern Ruby binstubs (the current default) are invoked with
+      # RbConfig.ruby so they run under the same Ruby as Rake — this avoids
+      # version-manager/shebang mismatches and works on Windows.
+      #
+      # Legacy JavaScript binstubs left over from earlier Shakapacker
+      # versions (`#!/usr/bin/env node`) are invoked through Node until
+      # users refresh them with `rake shakapacker:binstubs`.
+      js_binstub_executable = Shakapacker::Utils::Misc.js_binstub_executable(bin_path)
+
       Dir.chdir(Rails.root) do
-        Kernel.exec(*shakapacker_config_binstub_command(bin_path), *ARGV[1..])
+        if js_binstub_executable
+          $stderr.puts "Note: bin/shakapacker-config is a legacy JavaScript binstub."
+          $stderr.puts "Run `bundle exec rake shakapacker:binstubs` to upgrade to the Ruby binstub."
+          $stderr.puts ""
+          begin
+            Kernel.exec(js_binstub_executable, bin_path.to_s, *ARGV[1..])
+          rescue Errno::ENOENT, Errno::EACCES
+            abort "Error: could not execute '#{js_binstub_executable}' because it was not found or is not executable.\n" \
+                  "Run `bundle exec rake shakapacker:binstubs` to upgrade to the Ruby binstub."
+          end
+        else
+          begin
+            Kernel.exec(RbConfig.ruby, bin_path.to_s, *ARGV[1..])
+          rescue Errno::ENOENT, Errno::EACCES
+            abort "Error: Ruby interpreter '#{RbConfig.ruby}' was not found or is not executable."
+          end
+        end
       end
     end
   end

spec/shakapacker/export_bundler_config_task_spec.rb

@@ -2,6 +2,7 @@
 require "pathname"
 require "rake"
 require "rbconfig"
+require "stringio"
 require "tmpdir"
 
 RSpec.describe "shakapacker:export_bundler_config" do
@@ -29,10 +30,12 @@ def write_app_binstub(app_path, content)
     binstub_path
   end
 
-  def invoke_task(app_path, *args)
+  def invoke_task(app_path, *args, exec_error: nil)
     captured_exec = nil
     stub_const("Rails", double(root: Pathname.new(app_path)))
     allow(Kernel).to receive(:exec) do |*exec_args|
+      raise exec_error if exec_error
+
       captured_exec = exec_args
     end
 
@@ -42,14 +45,89 @@ def invoke_task(app_path, *args)
     captured_exec
   end
 
+  def capture_stderr
+    stderr_output = StringIO.new
+    original_stderr = $stderr
+    $stderr = stderr_output
+    yield stderr_output
+  ensure
+    $stderr = original_stderr
+  end
+
   it "runs upgraded apps' legacy JavaScript binstub with node" do
     Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
       binstub_path = write_app_binstub(app_path, <<~JS)
         #!/usr/bin/env node
         console.log("legacy binstub")
       JS
 
+      allow($stderr).to receive(:puts)
+
       expect(invoke_task(app_path, "--doctor")).to eq(["node", binstub_path, "--doctor"])
+      expect($stderr).to have_received(:puts).with(a_string_including("legacy JavaScript binstub"))
+      expect($stderr).to have_received(:puts).with(a_string_including("rake shakapacker:binstubs"))
+    end
+  end
+
+  it "runs nodejs legacy binstubs with nodejs and warns about the legacy binstub" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      binstub_path = write_app_binstub(app_path, <<~JS)
+        #!/usr/bin/env nodejs
+        console.log("legacy binstub")
+      JS
+
+      allow($stderr).to receive(:puts)
+
+      expect(invoke_task(app_path, "--doctor")).to eq(
+        ["nodejs", binstub_path, "--doctor"]
+      )
+      expect($stderr).to have_received(:puts).with(a_string_including("legacy JavaScript binstub"))
+      expect($stderr).to have_received(:puts).with(a_string_including("rake shakapacker:binstubs"))
+    end
+  end
+
+  it "aborts with upgrade guidance when a legacy JavaScript binstub executable is missing" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      write_app_binstub(app_path, <<~JS)
+        #!/usr/bin/env node
+        console.log("legacy binstub")
+      JS
+
+      capture_stderr do |stderr_output|
+        expect { invoke_task(app_path, "--doctor", exec_error: Errno::ENOENT.new) }.to raise_error(SystemExit)
+        expect(stderr_output.string).to include("could not execute 'node' because it was not found or is not executable")
+        expect(stderr_output.string).to include("rake shakapacker:binstubs")
+      end
+    end
+  end
+
+  it "aborts with upgrade guidance when a legacy JavaScript binstub executable is not executable" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      write_app_binstub(app_path, <<~JS)
+        #!/usr/bin/env node
+        console.log("legacy binstub")
+      JS
+
+      capture_stderr do |stderr_output|
+        expect { invoke_task(app_path, "--doctor", exec_error: Errno::EACCES.new) }.to raise_error(SystemExit)
+        expect(stderr_output.string).to include("could not execute 'node' because it was not found or is not executable")
+        expect(stderr_output.string).to include("rake shakapacker:binstubs")
+      end
+    end
+  end
+
+  it "aborts with guidance when a direct-path legacy JavaScript binstub executable is missing" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      write_app_binstub(app_path, <<~JS)
+        #!/usr/local/bin/node
+        console.log("legacy binstub")
+      JS
+
+      capture_stderr do |stderr_output|
+        expect { invoke_task(app_path, "--doctor", exec_error: Errno::ENOENT.new) }.to raise_error(SystemExit)
+        expect(stderr_output.string).to include("could not execute '/usr/local/bin/node' because it was not found or is not executable")
+        expect(stderr_output.string).to include("rake shakapacker:binstubs")
+      end
     end
   end
 
@@ -63,4 +141,27 @@ def invoke_task(app_path, *args)
       expect(invoke_task(app_path, "--save")).to eq([RbConfig.ruby, binstub_path, "--save"])
     end
   end
+
+  it "uses the gem version when bin/shakapacker-config is a directory" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      FileUtils.mkdir_p(File.join(app_path, "bin", "shakapacker-config"))
+      gem_bin_path = File.expand_path("../../lib/install/bin/shakapacker-config", __dir__)
+
+      expect(invoke_task(app_path)).to eq([RbConfig.ruby, gem_bin_path])
+    end
+  end
+
+  it "aborts when the Ruby binstub interpreter is missing" do
+    Dir.mktmpdir("shakapacker-export-bundler-config-") do |app_path|
+      write_app_binstub(app_path, <<~RUBY)
+        #!/usr/bin/env ruby
+        puts "ruby binstub"
+      RUBY
+
+      capture_stderr do |stderr_output|
+        expect { invoke_task(app_path, "--save", exec_error: Errno::ENOENT.new) }.to raise_error(SystemExit)
+        expect(stderr_output.string).to include("Ruby interpreter '#{RbConfig.ruby}' was not found or is not executable")
+      end
+    end
+  end
 end

spec/shakapacker/utils_misc_spec.rb

@@ -0,0 +1,121 @@
+require "spec_helper"
+require "fileutils"
+require "tempfile"
+require "shakapacker/utils/misc"
+
+RSpec.describe Shakapacker::Utils::Misc do
+  describe ".js_binstub_executable" do
+    def with_binstub(contents)
+      file = Tempfile.new(["binstub-", ""])
+      file.binmode
+      file.write(contents)
+      file.close
+      yield file.path
+    ensure
+      file&.close
+      file&.unlink
+    end
+
+    it "returns node for a node `env` shebang" do
+      with_binstub("#!/usr/bin/env node\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "node"
+      end
+    end
+
+    it "returns node for a node `env` shebang with CRLF line endings" do
+      with_binstub("#!/usr/bin/env node\r\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "node"
+      end
+    end
+
+    it "returns node for a node `env` shebang with env flags" do
+      with_binstub("#!/usr/bin/env -S node --no-warnings\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "node"
+      end
+    end
+
+    it "returns node for a node `env` shebang with env flags that take arguments" do
+      with_binstub("#!/usr/bin/env -u NODE_PATH -C /tmp -P /opt/node/bin -a node-shim node\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "node"
+      end
+    end
+
+    it "returns node for a node `env` shebang with environment assignments" do
+      with_binstub("#!/usr/bin/env NODE_PATH=/opt/local/lib PATH=/opt/node/bin node\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "node"
+      end
+    end
+
+    it "returns the executable path for a direct node shebang" do
+      with_binstub("#!/usr/local/bin/node\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "/usr/local/bin/node"
+      end
+    end
+
+    it "returns nodejs for a nodejs shebang" do
+      with_binstub("#!/usr/bin/env nodejs\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "nodejs"
+      end
+    end
+
+    it "returns the executable path for a direct nodejs shebang" do
+      with_binstub("#!/usr/bin/nodejs\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to eq "/usr/bin/nodejs"
+      end
+    end
+
+    it "returns nil for a ruby `env` shebang" do
+      with_binstub("#!/usr/bin/env ruby\nputs 'ruby'\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil for a direct ruby shebang" do
+      with_binstub("#!/usr/bin/ruby\nputs 'ruby'\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil when node appears after the shebang interpreter" do
+      with_binstub("#!/usr/bin/ruby node\nputs 'ruby'\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil for shebangs that only contain node as a substring" do
+      with_binstub("#!/usr/bin/env nodemon\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil when the file has no shebang" do
+      with_binstub("just text\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil for malformed shebangs" do
+      with_binstub("#!/usr/bin/env 'node\nconsole.log('legacy')\n") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil for an empty file" do
+      with_binstub("") do |path|
+        expect(described_class.js_binstub_executable(path)).to be_nil
+      end
+    end
+
+    it "returns nil when the path does not exist" do
+      expect(
+        described_class.js_binstub_executable("/no/such/path/anywhere")
+      ).to be_nil
+    end
+
+    it "returns nil when the path is a directory" do
+      Dir.mktmpdir do |dir|
+        expect(described_class.js_binstub_executable(dir)).to be_nil
+      end
+    end
+  end
+end