[codex] Fix ESM-safe config helper binstubs#1104
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughBinstubs ChangesESM-Compatible Binstubs via Ruby Wrappers
Sequence DiagramsequenceDiagram
participant User
participant RubyWrapper as Ruby Wrapper<br/>(bin/shakapacker-config)
participant Node as Node Binary
participant CJSScript as .cjs Script<br/>(shakapacker-config.cjs)
participant ConfigExporter as configExporter
User->>RubyWrapper: bin/shakapacker-config [args]
RubyWrapper->>RubyWrapper: locate Gemfile ↦ app_root
RubyWrapper->>RubyWrapper: verify node --version exists
RubyWrapper->>RubyWrapper: set RAILS_ENV, NODE_ENV env defaults
RubyWrapper->>RubyWrapper: resolve node_modules/.../shakapacker-config.cjs
RubyWrapper->>Node: exec node <script> <args>
Node->>CJSScript: require and execute
CJSScript->>ConfigExporter: require('./configExporter').run(args)
ConfigExporter->>CJSScript: return exitCode (Promise or number)
CJSScript->>Node: process.exit(exitCode)
Node->>User: exit with code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Code ReviewGood approach to fix the ESM compatibility problem. Converting JS binstubs to Ruby wrappers that IssuesRoot detection inconsistency between installed and generated binstubs
The old This is likely correct — the script now executes from within Minor
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Rake fallback exec needs explicit Ruby interpreter
- The fallback now invokes the gem script with RbConfig.ruby so it does not depend on the gem file execute bit.
Or push these changes by commenting:
@cursor push 5adaae4e86
Preview (5adaae4e86)
diff --git a/lib/tasks/shakapacker/export_bundler_config.rake b/lib/tasks/shakapacker/export_bundler_config.rake
--- a/lib/tasks/shakapacker/export_bundler_config.rake
+++ b/lib/tasks/shakapacker/export_bundler_config.rake
@@ -1,3 +1,5 @@
+require "rbconfig"
+
namespace :shakapacker do
desc <<~DESC
Export webpack or rspack configuration for debugging and analysis
@@ -54,7 +56,7 @@
$stderr.puts ""
Dir.chdir(Rails.root) do
- exec(gem_bin_path, *ARGV[1..])
+ exec(RbConfig.ruby, gem_bin_path, *ARGV[1..])
end
else
# Pass through command-line arguments after the task nameYou can send follow-ups to the cloud agent here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code ReviewOverall: The approach is sound — replacing JS binstubs with Ruby wrappers that Issues1. Test path mismatch on macOS (potential CI-local divergence) In Fix: use 2. The 3. Silent fallback to When neither Minor / nits4. The dummy binstub uses 5. Redundant
Upgrade pathExisting apps that already have the old JavaScript binstubs installed (e.g., a |
Review: ESM-safe config helper binstubsThe overall approach is well-chosen: converting the app-installed JS binstubs to Ruby wrappers that Bugs / correctness
In exec(bin_path.to_s, *ARGV[1..])This relies on the OS honouring the exec(RbConfig.ruby, bin_path.to_s, *ARGV[1..])Design concerns
Checking
In Test coverage gapThe Minor
Overall this is a solid fix for a real pain point. The issues above are mostly minor or have straightforward fixes; the Yarn PnP |
Code ReviewThe core approach is sound: converting JS binstubs to Ruby wrappers that Issues
The installed No helpful error when
In Minor
|
ReviewClean fix for the ESM parse issue. Replacing the JS binstubs with Ruby wrappers is the right call — the shebang approach is unambiguous regardless of the app's Must-fix
The fallback path (binstub absent) was correctly updated to exec(RbConfig.ruby, bin_path.to_s, *ARGV[1..])SuggestionsCHANGELOG.md — This is a user-visible bug fix (ESM apps silently broke). Per project guidelines, an entry belongs under Code duplication — Looks good
|
Review feedback addressed — 459668fI triaged the 14 outstanding inline comments + the 5 general review summaries and pushed 459668f. All review threads are now resolved. Code changes in this commit
Items already addressed in earlier commits on this branchThese came up repeatedly across the five review summaries and were resolved before this commit landed: switching from Verification
🤖 Generated with Claude Code |
Code ReviewVerdict: Looks good with minor suggestions. The approach is sound — Ruby wrappers sidestep ESM parse errors entirely because Node never sees them, and the Main concern: 4-way duplication
The "Keep in sync" comments are honest, but any change to the fallback logic (e.g., re-adding the Not a blocker, but worth addressing before the helper logic grows. Minor issues (see inline comments)
|
justin808
force-pushed
the
codex/fix-esm-config-binstubs
branch
from
e8292dc to
935cf92
Compare
|
Code review posted as inline comments. |
justin808
force-pushed
the
codex/fix-esm-config-binstubs
branch
from
935cf92 to
21179d8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
NODE_ENVdefault ignoresRAILS_ENV, diverging from ecosystem convention- Updated the binstubs and generated binstub template to derive default NODE_ENV from RAILS_ENV, using development only for development/test and production otherwise.
Or push these changes by commenting:
@cursor push 13c16e0b3e
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 21179d8. Configure here.
|
Clean, well-motivated fix overall -- see inline comments for specifics. Summary of findings: (1) The sync comment in cli.ts says 'update all three' but spec/dummy/bin/shakapacker-config is a fourth copy not mentioned -- a future contributor will miss it. (2) The binstub_sync_spec test name 'documents every divergent binstub and only divergent binstubs' overstates what it checks; the 'only' direction is handled by the loop above it, not by this test. (3) Minor: the Node probe via system() forks a subprocess; RbConfig::Utils.find_executable walks PATH without forking. (4) Informational: Ruby exec on Windows spawns a child rather than replacing the process -- worth a comment so it is not 'fixed' later. The RbConfig.ruby change in the rake task and the createRequire(__filename) anchor in the .cjs script are both good calls. Test coverage with real subprocesses is solid. Approve pending the sync-comment fix. |
![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: 21179d8bb7
ℹ️ 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".
Greptile SummaryReplaces the app-installed
Confidence Score: 4/5Safe to merge; the Ruby-wrapper approach correctly solves the ESM parse problem and the new .cjs files preserve the original JS logic faithfully. The core change is well-structured: Ruby wrappers delegate to CommonJS scripts, the rake task now explicitly passes RbConfig.ruby for portability, and tests cover the ESM success path, missing-script, and no-node error cases. The only gap is that all three Ruby wrapper surfaces leave exec calls unguarded by a rescue, so an unexpected exec failure produces a raw Ruby backtrace rather than a Shakapacker-branded error message. lib/install/bin/shakapacker-config, lib/install/bin/diff-bundler-config, and the generated template in package/configExporter/cli.ts all share the same unrescued exec pattern and should be updated together. Important Files Changed
Sequence DiagramsequenceDiagram
participant Rake as rake shakapacker:export_bundler_config
participant Ruby as Ruby (RbConfig.ruby)
participant Binstub as bin/shakapacker-config (Ruby wrapper)
participant Node as node
participant CJS as package/bin/shakapacker-config.cjs
participant TS as configExporter run()
Rake->>Ruby: "exec(RbConfig.ruby, binstub_path, *ARGV[1..])"
Ruby->>Binstub: runs binstub script
Binstub->>Binstub: shakapacker_app_root()
Binstub->>Binstub: shakapacker_node_binary() via system node --version
Binstub->>Binstub: "verify node_modules/shakapacker/package/bin/*.cjs exists"
Binstub->>Node: "exec node_bin, script_path, *ARGV"
Node->>CJS: runs .cjs file
CJS->>TS: run(process.argv.slice(2))
TS-->>CJS: Promise exitCode
CJS-->>Node: process.exit(exitCode)
Reviews (1): Last reviewed commit: "Allow shakapacker-config binstubs to div..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Dir.chdir(app_root) do | ||
| exec node_bin, script_path, *ARGV | ||
| end |
There was a problem hiding this comment.
exec fails after the node existence check (e.g., the binary is found by system but is then unexecutable due to a race or filesystem error), Ruby raises an unhandled Errno::ENOENT/Errno::EACCES and the user sees a raw backtrace instead of a helpful message. A rescue around the exec block would surface a cleaner error.
| Dir.chdir(app_root) do | |
| exec node_bin, script_path, *ARGV | |
| end | |
| Dir.chdir(app_root) do | |
| exec node_bin, script_path, *ARGV | |
| rescue SystemCallError => e | |
| warn "[Shakapacker] Failed to exec #{node_bin.inspect}: #{e.message}" | |
| exit 1 | |
| end |
| Dir.chdir(app_root) do | ||
| exec node_bin, script_path, *ARGV | ||
| end |
There was a problem hiding this comment.
exec failure path as in shakapacker-config — if the binary disappears between the system probe and the exec call the user gets a raw Ruby backtrace.
| Dir.chdir(app_root) do | |
| exec node_bin, script_path, *ARGV | |
| end | |
| Dir.chdir(app_root) do | |
| exec node_bin, script_path, *ARGV | |
| rescue SystemCallError => e | |
| warn "[Shakapacker] Failed to exec #{node_bin.inspect}: #{e.message}" | |
| exit 1 | |
| end |
|
|
||
| APP_ROOT = File.expand_path("..", __dir__) | ||
| Dir.chdir(APP_ROOT) do | ||
| exec "node", "./node_modules/.bin/shakapacker-config", *ARGV | ||
| Dir.chdir(app_root) do | ||
| exec node_bin, script_path, *ARGV | ||
| end |
There was a problem hiding this comment.
createBinStub template emits the same Dir.chdir { exec … } pattern without a rescue. Any app that installs the binstub via --init will get the same raw-backtrace failure path. If the rescue is added to lib/install/bin/shakapacker-config, the template here should be updated in the same change to keep the three in sync (as the comment at line 526 already requests).
| it "documents every divergent binstub and only divergent binstubs" do | ||
| install_basenames = Dir.glob(File.join(gem_root, "lib/install/bin/*")).map { |p| File.basename(p) } | ||
| bin_basenames = Dir.glob(File.join(gem_root, "bin/*")).map { |p| File.basename(p) } | ||
| actually_shared = install_basenames & bin_basenames | ||
|
|
||
| documented_but_missing = INTENTIONALLY_DIVERGENT_BINSTUBS - actually_shared | ||
| expect(documented_but_missing).to be_empty, | ||
| "INTENTIONALLY_DIVERGENT_BINSTUBS lists #{documented_but_missing.inspect} " \ | ||
| "but those files are no longer present in both bin/ and lib/install/bin/. " \ | ||
| "Remove them from the list." | ||
| end |
There was a problem hiding this comment.
documented_but_missing — i.e., that nothing in INTENTIONALLY_DIVERGENT_BINSTUBS has been deleted. The complementary direction (a binstub present in both dirs with diverging content that is not in the list) is caught by the per-binstub equality tests above, not by this test. Consider renaming to "documents every intentionally-divergent binstub" so the scope is accurate.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Code Review: Fix ESM-safe config helper binstubsOverall: This is a clean, well-reasoned fix. Converting JS binstubs to Ruby wrappers is the right approach — it sidesteps the ESM parsing problem entirely rather than trying to work around it inside JavaScript. Tests are thorough and cover the key scenarios. A few things worth noting: Code duplication across three filesThe
The "Keep in sync" comments acknowledge this but don't enforce it — there's no spec that compares the two install binstubs against the template output. If the template drifts from the hand-maintained files, the only signal is a careful reader noticing the comment. Given these are 10-line helpers, a snapshot/fixture test that generates the stub from the template and diffs it against the install binstub would catch drift automatically. Mixed Ruby/JS interpolation in the
|
| # Keep in sync with lib/install/bin/shakapacker-config and | ||
| # lib/install/bin/diff-bundler-config; update all three when changing helpers. |
There was a problem hiding this comment.
Worth adding a note here that #{} in the lines below is Ruby interpolation syntax — JavaScript template literals only interpolate ${...}, so these pass through verbatim to the generated file. Without a comment, a future reader may assume #{} is a typo or an unused interpolation and "fix" it.
| def shakapacker_node_binary | ||
| node_bin = "node" | ||
| return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL) | ||
|
|
||
| warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \ |
There was a problem hiding this comment.
The system call forks a subprocess on every invocation just to confirm Node is reachable. For a low-cost alternative that stays in-process:
| def shakapacker_node_binary | |
| node_bin = "node" | |
| return node_bin if system(node_bin, "--version", out: File::NULL, err: File::NULL) | |
| warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \ | |
| def shakapacker_node_binary | |
| node_bin = "node" | |
| found = ENV.fetch("PATH", "").split(File::PATH_SEPARATOR).any? do |dir| | |
| File.executable?(File.join(dir, node_bin)) | |
| end | |
| return node_bin if found | |
| warn "[Shakapacker] Could not find Node.js executable #{node_bin.inspect}. " \ | |
| "Install Node.js and try again." | |
| exit 1 | |
| end |
Not a blocker — the system approach is clear and the subprocess cost is tiny at startup — but worth considering if you'd prefer to avoid the fork.
|
|
||
| it "falls back to the current directory when #{command}'s parent has no Gemfile" do | ||
| Dir.mktmpdir("shakapacker-binstub-") do |app_path| | ||
| FileUtils.mkdir_p(File.join(app_path, "bin")) | ||
| install_fake_node_script(app_path, command) | ||
|
|
||
| binstub_path = File.join(app_path, "bin", command) | ||
| FileUtils.cp(File.join(gem_root, "lib", "install", "bin", command), binstub_path) | ||
| FileUtils.chmod(0o755, binstub_path) | ||
|
|
||
| output_path = File.join(app_path, "binstub-output.json") | ||
| _stdout, stderr, status = Open3.capture3( | ||
| { "SHAKAPACKER_BINSTUB_OUTPUT" => output_path }, | ||
| binstub_path, | ||
| chdir: app_path | ||
| ) | ||
|
|
||
| expect(status).to be_success, stderr | ||
| expect(JSON.parse(File.read(output_path))).to eq( | ||
| "cwd" => File.realpath(app_path), | ||
| "argv" => [] | ||
| ) | ||
| end |
There was a problem hiding this comment.
This test case sets up a directory without a Gemfile, so shakapacker_app_root will warn and fall back to Dir.pwd. The test passes chdir: app_path, so Dir.pwd equals app_path, and the fake node script is installed there. This means the test is really validating "fallback works and the script is still found at cwd" — the description "falls back to the current directory" captures that correctly.
One small thing: it might be worth asserting that stderr includes the fallback warning so that if the fallback logic changes silently, this test catches it.
Address the medium-priority test-coverage follow-ups from the PR #1104 review. 1. Cover spec/dummy/bin/shakapacker-config in the sync spec. The dummy binstub used by the test app must stay byte-identical to the install template; otherwise a stale wrapper could silently mask install- template changes. 2. Add a JS test that invokes createBinStub for both helper binstub names and asserts the generated content is byte-identical to the checked-in lib/install/bin/* files. Exports createBinStub from package/configExporter/cli.ts so the test can reach it. This closes the gap where the Ruby sync spec couldn't reach into the JS template. 3. Assert the `[Shakapacker] No Gemfile found at …` warning in helper_binstubs_spec.rb so the fallback path can't lose its warning silently. 4. Update the "Keep in sync" comment in all four locations to list all four copies (the previous wording said "all three" and missed the dummy binstub). 5. Rename the "documents every divergent binstub and only divergent binstubs" example to "all documented divergent binstubs still exist in both directories" so the name matches what the test actually verifies — the "only" guarantee comes from the iteration loop, not this example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
## Summary Address the **low-priority optimization** from the [PR #1104 follow-up review](#1123). 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_binary` in the helper binstubs previously verified Node was installed by running `system("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 of `ENV["PATH"]` that also probes `PATHEXT`-style extensions on Windows (so `node.exe`/`node.cmd` are still discoverable). One fork+exec saved per `bin/shakapacker-config` and `bin/diff-bundler-config` call. The return value is unchanged (the literal string `"node"`), so the subsequent `exec node_bin, script_path, *ARGV` still goes through the OS's `PATH` resolution 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-config` - `lib/install/bin/diff-bundler-config` - `spec/dummy/bin/shakapacker-config` - the `createBinStub` template in `package/configExporter/cli.ts` ### Stacked on PR #1128 This branch is stacked on top of `jg/1123-binstub-sync-coverage` (PR #1128). GitHub will auto-update the base to `main` once #1128 merges. If #1128 is closed instead, this PR can rebase onto `main` with a trivial change (only the comment header and `NODE_ENV` line differ, and only the comment-header lines need replaying). Refs #1123. ## Test plan - [x] `bundle exec rspec spec/shakapacker/binstub_sync_spec.rb` — 2 examples pass (dummy↔install parity preserved) - [x] `bundle exec rspec spec/shakapacker/helper_binstubs_spec.rb` — 8 examples pass, including the "node unavailable" path that uses `PATH=/nonexistent` - [x] `yarn jest test/configExporter/createBinStub.test.js` — 2 examples pass (template still matches install templates byte-for-byte) - [x] `bundle exec rubocop` — no offenses on changed Ruby files - [x] `yarn eslint` — no errors on changed TS file - [x] `ruby -c` on all three modified binstubs — syntax OK 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches the helper binstubs used to launch Node-based tooling; any bug in the new `PATH`/`PATHEXT` scan could prevent commands from running, especially on Windows. > > **Overview** > Replaces `shakapacker_node_binary`’s `system("node", "--version")` check with a no-fork scan of `ENV["PATH"]`, including Windows `PATHEXT` extension probing. > > Applies the same updated Node lookup logic across all four synced helper-binstub copies (`lib/install/bin/*`, `spec/dummy/bin/*`, and the `createBinStub` template in `package/configExporter/cli.ts`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1889de4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address the medium-priority test-coverage follow-ups from the PR #1104 review. 1. Cover spec/dummy/bin/shakapacker-config in the sync spec. The dummy binstub used by the test app must stay byte-identical to the install template; otherwise a stale wrapper could silently mask install- template changes. 2. Add a JS test that invokes createBinStub for both helper binstub names and asserts the generated content is byte-identical to the checked-in lib/install/bin/* files. Exports createBinStub from package/configExporter/cli.ts so the test can reach it. This closes the gap where the Ruby sync spec couldn't reach into the JS template. 3. Assert the `[Shakapacker] No Gemfile found at …` warning in helper_binstubs_spec.rb so the fallback path can't lose its warning silently. 4. Update the "Keep in sync" comment in all four locations to list all four copies (the previous wording said "all three" and missed the dummy binstub). 5. Rename the "documents every divergent binstub and only divergent binstubs" example to "all documented divergent binstubs still exist in both directories" so the name matches what the test actually verifies — the "only" guarantee comes from the iteration loop, not this example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary Stamps the v10.1.0 stable release header (was `v10.1.0-rc.2`) and curates the prerelease entries per `/update-changelog` guidance. ## Curation applied - **Merged PR #1133 → PR #1096** (Added: supplemental npm packages). #1133 restructured the supplemental deps from direct → peer in rc.2, so the original entry's "bundle the managed-build stack as direct dependencies" description was wrong for stable consumers. Rewrote to describe the final peer-deps shape (auto-installed on npm 7+) and credit both PRs. The #1133 standalone Changed entry was removed. - **Merged PR #1132 → PR #1104** (Fixed: ESM binstubs). #1132 fixed the `export_bundler_config` dispatch regression that #1104 introduced in rc.1 when switching from JS to Ruby binstubs; stable users never see the bug, so the fix is RC-only churn that belongs folded into the original entry. Also folded in #1132's minor improvements (NODE_ENV from RAILS_ENV mapping, Node lookup without execution, friendlier non-Error CLI messages). - **Merged PR #1106 → PR #1086** (Fixed: local-path version detection). Both shipped in rc.0 as iterations of the same regex fix; merged into a single entry covering `../`, `./`, and `file:` patterns. - **Lightened Migration Notes**: removed "bundled stack" language now that supplementals declare required peers instead of direct deps. - **Skipped post-rc.2 commit**: PR #1138 (skill tooling guidance) is internal — not user-visible. - **Compare links**: `[Unreleased]` now points from `v10.1.0`; replaced `[v10.1.0-rc.2]` with `[v10.1.0]: v10.0.0...v10.1.0`. ## Test plan - [x] `yarn lint` passes - [x] `yarn prettier --check CHANGELOG.md` passes - [x] File ends with trailing newline - [x] No duplicate section headings - [x] All merged PRs credited After merge, run `bundle exec rake release` (no args) — the rake task picks up `v10.1.0` from CHANGELOG.md and auto-creates the GitHub release. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only changelog edits with no runtime or dependency behavior changes. > > **Overview** > **Curates `CHANGELOG.md` for the v10.1.0 stable release** (replacing the `v10.1.0-rc.2` header dated May 25, 2026) so release notes match what stable users actually ship. > > Prerelease churn is folded into fewer, accurate bullets: supplemental npm packages (**#1096** + **#1133**) now describe **required peer dependencies** (not direct “bundled” deps), with the standalone **#1133** *Changed* entry removed; ESM/config binstub fixes (**#1104** + **#1132**) and local-path version detection (**#1086** + **#1106**) are single *Fixed* items. **Migration Notes** no longer imply a direct bundled stack on supplementals. > > Footer compare links: **`[Unreleased]`** from `v10.1.0`, **`[v10.1.0]`** replaces the rc.2 link (`v10.0.0...v10.1.0`). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2b57d5f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address the medium-priority test-coverage follow-ups from the PR #1104 review. 1. Cover spec/dummy/bin/shakapacker-config in the sync spec. The dummy binstub used by the test app must stay byte-identical to the install template; otherwise a stale wrapper could silently mask install- template changes. 2. Add a JS test that invokes createBinStub for both helper binstub names and asserts the generated content is byte-identical to the checked-in lib/install/bin/* files. Exports createBinStub from package/configExporter/cli.ts so the test can reach it. This closes the gap where the Ruby sync spec couldn't reach into the JS template. 3. Assert the `[Shakapacker] No Gemfile found at …` warning in helper_binstubs_spec.rb so the fallback path can't lose its warning silently. 4. Update the "Keep in sync" comment in all four locations to list all four copies (the previous wording said "all three" and missed the dummy binstub). 5. Rename the "documents every divergent binstub and only divergent binstubs" example to "all documented divergent binstubs still exist in both directories" so the name matches what the test actually verifies — the "only" guarantee comes from the iteration loop, not this example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address the medium-priority test-coverage follow-ups from the PR #1104 review. 1. Cover spec/dummy/bin/shakapacker-config in the sync spec. The dummy binstub used by the test app must stay byte-identical to the install template; otherwise a stale wrapper could silently mask install- template changes. 2. Add a JS test that invokes createBinStub for both helper binstub names and asserts the generated content is byte-identical to the checked-in lib/install/bin/* files. Exports createBinStub from package/configExporter/cli.ts so the test can reach it. This closes the gap where the Ruby sync spec couldn't reach into the JS template. 3. Assert the `[Shakapacker] No Gemfile found at …` warning in helper_binstubs_spec.rb so the fallback path can't lose its warning silently. 4. Update the "Keep in sync" comment in all four locations to list all four copies (the previous wording said "all three" and missed the dummy binstub). 5. Rename the "documents every divergent binstub and only divergent binstubs" example to "all documented divergent binstubs still exist in both directories" so the name matches what the test actually verifies — the "only" guarantee comes from the iteration loop, not this example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#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>
1 participant
Summary
shakapacker-configanddiff-bundler-configJavaScript binstubs with Ruby wrappers so apps using"type": "module"do not parse them as ESM.cjsentrypoints and npmbinmappings for both helpers.cjsentrypointFixes #1101.
Validation
bundle exec rspec spec/shakapacker/helper_binstubs_spec.rbbundle exec rubocop lib/install/bin/shakapacker-config lib/install/bin/diff-bundler-config lib/tasks/shakapacker/export_bundler_config.rake spec/shakapacker/helper_binstubs_spec.rb spec/dummy/bin/shakapacker-configyarn eslint package/bin/shakapacker-config.cjs package/bin/diff-bundler-config.cjs package/configExporter/cli.tsruby -c lib/install/bin/shakapacker-config && ruby -c lib/install/bin/diff-bundler-config && ruby -c spec/dummy/bin/shakapacker-config && ruby -c lib/tasks/shakapacker/export_bundler_config.rakenode --check package/bin/shakapacker-config.cjs && node --check package/bin/diff-bundler-config.cjsNote
Medium Risk
Changes how
shakapacker-config/diff-bundler-configare installed and executed (Ruby wrapper -> Node.cjs), which can affect CLI/rake task behavior across environments and Windows/path setups, but is scoped to helper binstubs.Overview
Fixes ESM apps (those with
"type": "module") failing to runbin/shakapacker-configandbin/diff-bundler-configby replacing the gem-installed JS binstub templates with Ruby wrappers that locatenodeandexecpackaged CommonJS scripts undernode_modules/shakapacker/package/bin/*.cjs.Adds the
.cjspackage entrypoints and npmbinmappings inpackage.json, updates the config-exporter’s generated binstub template to emit the same Ruby wrapper (with explicitchmod), and adjusts theshakapacker:export_bundler_configrake task to invoke binstubs viaRbConfig.rubyfor consistent Ruby/shebang behavior. Tests were updated/added to allow intentional binstub divergence and to validate wrapper behavior/error cases.Reviewed by Cursor Bugbot for commit 21179d8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
shakapacker-configanddiff-bundler-configbinstub commands to work properly in apps configured with ESM ("type": "module"). Existing applications should re-runbundle exec rake shakapacker:binstubsto install the updated binstubs.