refactor: clarify RSC migration parser and release safeguards (#3343)

## Summary

Follow-up to #3219 / fixes #3258. This PR is now rebased onto current
`origin/main`.

### RSC migration parser cleanup

- Rename `rsc_plugin_without_client_references?` to
`any_rsc_plugin_missing_client_references?` and document the existential
semantics.
- Consolidate lightweight JS scanner support/limitation notes onto
`advance_js_scan_state`, including caller coverage and
regex/template-literal caveats.
- Clarify why `add_rsc_client_references_setup` keeps defensive
duplicate-helper guards.

### Adjacent hardening

- Strengthen Node renderer `SECURITY:` comments for CommonJS wrapping
via both `additionalContext` and `supportModules`.
- Make the package-json publish rewrite use a same-directory temp file
plus atomic rename before marking the file as changed.
- Add direct coverage for the package-json pin fallback warning
formatter.
- Keep the retry-sleep guard in release metadata lookup explicit and
make the webpack helper test description path-agnostic.

## Review Follow-Ups

- **Must-fix:** none. Thread-aware review scan currently shows 0
unresolved review threads.
- **Optional fixed:** added the direct `supportModules` security anchor
and the `package_json_pin_fallback_warning` formatter spec.
- **Discuss / advice:** the prior `Generator tests / examples (3.2,
minimum)` failure was caused by `npm install shakapacker@10.1.0`
returning `ETARGET` before that npm version was available. The registry
now resolves `shakapacker@10.1.0`, and the React 16 pinned example task
passed locally.
- **Discuss / non-code CI:** the current `claude-review` failure is the
Claude Action hitting its weekly limit (`resets May 29, 3am UTC`), not a
repository test failure.

## CI / Test Plan

- [x] Rebased onto `origin/main`
- [x] `npm view shakapacker@10.1.0 version`
- [x] `(cd react_on_rails && bundle exec rake
run_rspec:shakapacker_examples_react16)`
- [x] `(cd react_on_rails && BUNDLE_FORCE_RUBY_PLATFORM=true bundle exec
rspec spec/react_on_rails/shakapacker_examples_rake_spec.rb
spec/react_on_rails/release_rake_helpers_spec.rb
spec/react_on_rails/generators/js_dependency_manager_spec.rb)`
- [x] Pre-push hook: branch Ruby RuboCop on changed Ruby files

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Changes RSC config auto-migration control flow and npm publish
package.json handling; mistakes could corrupt webpack configs or leave
bad publish manifests, though new specs cover the release path.
> 
> **Overview**
> This PR tightens **RSC webpack config migration** logic and documents
**Node renderer sandboxing**, plus small **release** and **generator**
hardening.
> 
> **RSC clientReferences migration** renames
`rsc_plugin_without_client_references?` to
**`any_rsc_plugin_missing_client_references?`** and documents that it is
an existential check (not the complement of “all plugins define
clientReferences”). **`prepare_rsc_client_references_rewrite!`**
replaces the old prepare helper so helper injection and rewrite
decisions stay correct when some plugins already use scoped
`clientReferences`. **`rsc_plugin_sections_safe_to_rewrite?`** now takes
**`is_server:`** when counting unparseable sections. Lightweight JS
scanner limits and caller contracts are **centralized on
`advance_js_scan_state`**, with extra comments on defensive
duplicate-helper guards.
> 
> **Node renderer config** comments add explicit **SECURITY** notes that
**`supportModules: true`** and a plain-object **`additionalContext`**
both enable CommonJS wrapping and host **`require`**.
> 
> **npm publish** rewrites `package.json` via a **same-directory temp
file and atomic rename**, sets the restore **`changed`** flag only after
rename succeeds, and adds a spec for failed rename cleanup.
> 
> **JS dependency fallback** emits a clearer warning listing
**`name@version`** pins written to `package.json` when install fails.
The webpack helper test description is made path-agnostic.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
92f41ed. 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**
  * More robust npm publishing via atomic package.json replacement.
* Improved React Server Components setup handling for multi-plugin
configurations.

* **Documentation**
  * Expanded security notes for supportModules and sandboxing behavior.

* **Improvements**
  * Clearer, better-formatted package manager warning messages.

* **Tests**
* Updated/added tests and path-agnostic test descriptions for related
warnings.

<!-- review_stack_entry_start -->

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3343?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)

<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

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

Author: justin808

packages/react-on-rails-pro-node-renderer/src/shared/configBuilder.ts

@@ -50,19 +50,23 @@ export interface Config {
   // See docs/oss/building-features/node-renderer/js-configuration.md#runtime-globals-for-ssr-and-rsc.
   // This option is required to equal `true` if you want to use loadable components.
   // Setting this value to false causes the NodeRenderer to behave like ExecJS.
+  // SECURITY: When `supportModules: true`, the renderer also wraps the bundle and injects the
+  // host `require` regardless of `additionalContext`. See the detailed `additionalContext`
+  // security note below.
   supportModules: boolean;
   // additionalContext enables you to specify additional NodeJS objects (usually from
   // https://nodejs.org/api/globals.html) to add to the VM context in addition to our supportModules defaults.
   // Object shorthand notation may be used, but is not required.
   // Example: { URL, URLSearchParams, Crypto }
-  // NOTE: Any plain object value (including an empty `{}`) puts the renderer into CommonJS
-  // execution mode, wrapping the bundle and granting it access to the host's `require`
-  // (e.g. `require('fs')`, `require('child_process')`). An empty `{}` is treated the same
-  // as any other plain object -- it opts into CommonJS mode even though it adds no globals.
-  // Use only with trusted bundle sources.
+  // SECURITY: Any plain object value (including an empty `{}`) puts the renderer into CommonJS
+  // execution mode. The bundle is wrapped via `module.wrap()` and receives the host process's
+  // unrestricted `require`, granting full access to Node.js built-ins such as `fs`,
+  // `child_process`, and `os`. This disables VM sandboxing for the bundle, even when no globals
+  // are added. Only use with fully trusted, first-party bundle sources.
   // To keep the VM sandboxed without `require`, set BOTH `additionalContext: null` AND
-  // `supportModules: false`. When `supportModules: true`, the renderer wraps the bundle and
-  // injects the host `require` regardless of `additionalContext`.
+  // `supportModules: false`.
+  // SECURITY: When `supportModules: true`, the renderer also wraps the bundle and injects the
+  // host `require` regardless of `additionalContext`.
   // Mechanically, "wrapping" means the renderer passes the bundle source through `module.wrap()`
   // (the standard CommonJS `(function (exports, require, module, __filename, __dirname) { ... })`
   // wrapper) and then invokes the wrapped function with the host `require`. See the `buildVM`

packages/react-on-rails/tests/webpackHelpers.test.ts

@@ -8,7 +8,7 @@ describe('webpackHelpers', () => {
       expect(reactDomClientWarning.test(message)).toBe(true);
     });
 
-    it('matches the webpack 4 "Module not found" warning for react-dom/client', () => {
+    it('matches the "Module not found" warning regardless of file path', () => {
       const message =
         "Module not found: Error: Can't resolve 'react-dom/client' in '/app/node_modules/react-on-rails/src'";
       expect(reactDomClientWarning.test(message)).toBe(true);

rakelib/release.rake

@@ -980,14 +980,35 @@ def replace_workspace_protocol_dependencies_for_publish!(package_json, package_v
   changed
 end
 
+def write_publishable_package_json(package_json_path, package_json)
+  tmp = Tempfile.create(["package-json-", ".json"], File.dirname(package_json_path))
+  tmp_path = tmp.path
+  renamed = false
+
+  begin
+    tmp.write("#{JSON.pretty_generate(package_json)}\n")
+    tmp.chmod(File.stat(package_json_path).mode & 0o777)
+    tmp.close
+    File.rename(tmp_path, package_json_path)
+    renamed = true
+  ensure
+    tmp.close unless tmp.closed?
+    File.unlink(tmp_path) if !renamed && File.exist?(tmp_path)
+  end
+end
+
 def with_publishable_package_json(dir, package_version)
   package_json_path = File.join(dir, "package.json")
   changed = false
   original_content = File.read(package_json_path)
   package_json = JSON.parse(original_content)
 
-  changed = replace_workspace_protocol_dependencies_for_publish!(package_json, package_version)
-  File.write(package_json_path, "#{JSON.pretty_generate(package_json)}\n") if changed
+  if replace_workspace_protocol_dependencies_for_publish!(package_json, package_version)
+    write_publishable_package_json(package_json_path, package_json)
+    # Only flip `changed` after the atomic same-directory rename succeeds so the `ensure`
+    # restore runs only after package.json was actually replaced.
+    changed = true
+  end
 
   yield
 ensure
@@ -1027,10 +1048,10 @@ def fetch_npm_package_metadata_with_retries(package_ref, registry_url:, attempts
 
     last_output = output
     last_status = status
-    next if attempt == attempts - 1
-
-    puts "npm did not return #{package_ref} yet; retrying in #{retry_delay_seconds} seconds..."
-    sleep retry_delay_seconds
+    unless attempt == attempts - 1
+      puts "npm did not return #{package_ref} yet; retrying in #{retry_delay_seconds} seconds..."
+      sleep retry_delay_seconds
+    end
   end
 
   [last_output, last_status]

react_on_rails/lib/generators/react_on_rails/js_dependency_manager.rb

@@ -578,12 +578,19 @@ def write_versioned_package_specs_to_package_json(packages, dev:)
         end
 
         File.write("package.json", "#{JSON.pretty_generate(content)}\n")
+        GeneratorMessages.add_warning(package_json_pin_fallback_warning(versioned_packages))
         true
       rescue StandardError => e
         GeneratorMessages.add_warning("⚠️  Could not write dependency pins to package.json: #{e.message}")
         false
       end
 
+      def package_json_pin_fallback_warning(versioned_packages)
+        pinned_list = versioned_packages.map { |name, version| "#{name}@#{version}" }.join(", ")
+        "⚠️  Package manager install failed. Wrote the following version pins to package.json " \
+          "so you can rerun your package manager manually: #{pinned_list}"
+      end
+
       def fallback_package_manager
         package_manager = GeneratorMessages.detect_package_manager(app_root: destination_root)
         return package_manager if GeneratorMessages.supported_package_manager?(package_manager)

react_on_rails/lib/generators/react_on_rails/rsc_setup/client_references.rb

@@ -121,41 +121,42 @@ def js_line_ending(content)
         end
 
         def update_existing_rsc_webpack_config(config_path, content, is_server:)
-          return unless rsc_plugin_sections_safe_to_rewrite?(config_path, content)
+          return unless rsc_plugin_sections_safe_to_rewrite?(config_path, content, is_server: is_server)
           return if rsc_plugin_uses_scoped_client_references?(content, is_server: is_server)
-
-          # May inject the scoped helper before the rewrite step re-reads the config from disk.
-          return unless prepare_rsc_client_references_setup(config_path, content, is_server: is_server)
-          return unless rsc_plugin_needs_client_references_rewrite?(content, is_server: is_server)
+          return unless prepare_rsc_client_references_rewrite!(config_path, content, is_server: is_server)
 
           return if rewrite_rsc_plugin_client_references(config_path, is_server: is_server)
 
           rollback_incomplete_rsc_client_references_setup(config_path, content)
           warn_missing_rsc_plugin_target(config_path, is_server: is_server)
         end
 
-        def prepare_rsc_client_references_setup(config_path, content, is_server:)
+        def prepare_rsc_client_references_rewrite!(config_path, content, is_server:)
+          # This prepares the rewrite and returns whether it should continue. When it returns true,
+          # the scoped helper may already have been injected on disk so
+          # `rewrite_rsc_plugin_client_references` can re-read fresh content with valid offsets.
           if rsc_plugin_references_any_scoped_client_references?(content, is_server: is_server)
-            return ensure_rsc_client_references_setup(config_path, content, is_server: is_server)
+            return false unless ensure_rsc_client_references_setup(config_path, content, is_server: is_server)
+
+            return rsc_plugin_needs_client_references_rewrite?(content, is_server: is_server)
           end
 
           rewritable_rsc_plugin?(config_path, content, is_server: is_server) &&
             ensure_rsc_client_references_setup(config_path, content, is_server: is_server)
         end
 
         def rsc_plugin_needs_client_references_rewrite?(content, is_server:)
-          rsc_plugin_without_client_references?(content, is_server: is_server)
+          any_rsc_plugin_missing_client_references?(content, is_server: is_server)
         end
 
         # Detects RSCWebpackPlugin option blocks that the lightweight JS scanner could not parse
         # cleanly (most often a regex literal with an unmatched `{` / `}` that walks the depth
         # counter past the real closing brace). When found, we warn and refuse to rewrite anything
         # in the file so a sibling rewrite cannot accidentally splice into a wrong location.
-        def rsc_plugin_sections_safe_to_rewrite?(config_path, content)
-          # The unparseable count is file-wide: the partition increments it for every invocation
-          # that cannot be parsed, regardless of the `is_server` argument. The argument only
-          # filters the target-specific safe bucket, which is ignored here.
-          unparseable = rsc_plugin_option_sections_partition(content, is_server: true).fetch(:unparseable)
+        def rsc_plugin_sections_safe_to_rewrite?(config_path, content, is_server:)
+          # `:unparseable` is file-wide: the partitioner increments it before filtering parseable
+          # sections by the current `is_server` target.
+          unparseable = rsc_plugin_option_sections_partition(content, is_server: is_server).fetch(:unparseable)
           return true if unparseable.zero?
 
           warn_unparseable_rsc_plugin_sections(config_path, unparseable)
@@ -165,7 +166,7 @@ def rsc_plugin_sections_safe_to_rewrite?(config_path, content)
         def rewritable_rsc_plugin?(config_path, content, is_server:)
           # Mixed same-target plugins are still rewritable: the later rewrite only updates plugins
           # missing clientReferences and leaves sibling custom clientReferences untouched.
-          return true if rsc_plugin_without_client_references?(content, is_server: is_server)
+          return true if any_rsc_plugin_missing_client_references?(content, is_server: is_server)
 
           if rsc_plugin_defines_client_references?(content, is_server: is_server)
             GeneratorMessages.add_warning(
@@ -263,7 +264,12 @@ def rsc_plugin_defines_client_references?(content, is_server:)
           end
         end
 
-        def rsc_plugin_without_client_references?(content, is_server:)
+        # Existential check: returns true when at least one matching plugin section is missing a
+        # top-level `clientReferences:` key. Pairs with `rsc_plugin_defines_client_references?`,
+        # which uses the same any-section semantics for the opposite condition. The two are not
+        # complements when multiple plugin sections exist — a file with one configured plugin and
+        # one unconfigured plugin returns true from both.
+        def any_rsc_plugin_missing_client_references?(content, is_server:)
           rsc_plugin_option_sections(content, is_server: is_server).any? do |section|
             !rsc_plugin_body_has_top_level_key?(section.fetch(:body), "clientReferences")
           end
@@ -273,8 +279,8 @@ def rsc_plugin_without_client_references?(content, is_server:)
         # so `clientReferences:` / `isServer:` substrings inside strings are not mis-detected.
         # Shares the `advance_js_scan_state` family used by `js_top_level_position?` and
         # `matching_js_closing_brace` so all JS-aware passes follow the same comment/string rules.
-        # Regex literals (e.g. `/a{2}/`) are still outside this scanner's supported surface
-        # because brace quantifiers can confuse `matching_js_closing_brace`'s depth counter.
+        # See `advance_js_scan_state` for the scanner's supported surface (including the regex-
+        # literal and nested-template-literal limits that callers must be aware of).
         def rsc_plugin_options_without_comments(options)
           result = String.new(capacity: options.length)
           state = nil
@@ -434,15 +440,12 @@ def first_significant_js_index(content, start_index)
         end
 
         # Expects `content[open_index] == "{"`; callers pass the options-object opening brace.
-        # This lightweight scanner treats template literals as opaque strings (backtick to backtick).
-        # Simple `${...}` expressions are handled correctly: while in the backtick state every
-        # character — including `{` and `}` inside the expression — is consumed as string content
-        # and never reaches the depth counter. The real unsupported case is *nested* template
-        # literals (e.g. `` `outer ${`inner`}` ``) where the inner backtick falsely closes the outer
-        # string state, exposing later braces to the depth counter. Callers detect that via
-        # `rsc_plugin_options_followed_by_close_paren?` and mark the section unparseable rather
-        # than producing a corrupt rewrite. Regex literals are outside this scanner's supported
-        # surface for the same reason.
+        # See `advance_js_scan_state` for the scanner's supported surface — in short, simple
+        # `${...}` interpolations inside template literals stay inside the string state, while
+        # nested template literals and regex literals fall outside the scanner. When the depth
+        # counter is confused by either, the section is caught downstream via
+        # `rsc_plugin_options_followed_by_close_paren?` and marked unparseable so the migration
+        # warns the user instead of corrupting the rewrite.
         def matching_js_closing_brace(content, open_index)
           depth = 0
           index = open_index
@@ -475,8 +478,51 @@ def matching_js_closing_brace(content, open_index)
           nil
         end
 
-        # Return index is the last consumed character. Line comments leave the newline
-        # for the caller's normal index increment; block comments consume the closing slash.
+        # Central dispatcher for the lightweight JS scanner shared by every JS-aware pass in this
+        # generator (`matching_js_closing_brace`, `js_top_level_position?`, `js_code_position?`,
+        # `rsc_plugin_options_without_comments`, `first_significant_js_index`,
+        # `rsc_plugin_options_followed_by_close_paren?`, `last_js_code_char_index`,
+        # `last_js_code_line_start`). Return index is the last consumed character. Line comments
+        # leave the newline for the caller's normal index increment; block comments consume the
+        # closing slash.
+        #
+        # Supported lexical constructs:
+        # - Line comments (`// ...\n`) and block comments (`/* ... */`).
+        # - Single-quoted (`'...'`), double-quoted (`"..."`), and template-literal (`` `...` ``)
+        #   strings, including escape sequences and the simple `${expr}` interpolation form
+        #   (interpolation braces stay inside the string state and never reach the depth counter).
+        #
+        # Outside the supported surface for callers that do **not** run `js_regex_literal_start?`
+        # preprocessing (e.g. `matching_js_closing_brace`, `rsc_plugin_options_without_comments`),
+        # the scanner cannot distinguish these from the syntax they shadow, so `{`/`}` characters
+        # they contain can confuse the depth counter. `js_top_level_position?` and
+        # `js_code_position?` handle regex literals via their own pre-pass and are unaffected.
+        # - Regex literals (e.g. `/a{2}/`, `/\{/`, `/[{]/`): not recognized as a distinct state,
+        #   so brace-containing patterns walk the depth counter past the real options close. The
+        #   user-facing warning text in `warn_unparseable_rsc_plugin_sections` calls these out
+        #   explicitly.
+        # - Nested template literals (`` `outer ${`inner`}` ``): the inner backtick falsely closes
+        #   the outer string state, exposing later braces to the depth counter.
+        #
+        # The downstream `rsc_plugin_option_sections_partition` catches both failure modes by
+        # requiring the matched closing `}` to be followed by `)`. When it isn't, the section is
+        # marked unparseable and `warn_unparseable_rsc_plugin_sections` asks the user to add
+        # `clientReferences:` manually — the migration declines to rewrite rather than risk
+        # corrupting the config.
+        #
+        # Future expansion (only worth doing if a real-world RSC plugin options block needs it):
+        # 1. Add a `:regex_literal` state alongside the string and comment states. Track regex
+        #    contexts by detecting `/` after a token that legally precedes a regex literal
+        #    (`=`, `(`, `,`, `:`, `;`, `?`, `!`, `&&`, `||`, `return`, `typeof`, etc.) and consume
+        #    until the unescaped closing `/` plus any flags. The token-context check is necessary
+        #    because the same `/` character means division in expression position.
+        # 2. Add a stack-based template-literal state so nested `` `...${`inner`}...` `` pairs
+        #    track depth instead of toggling a single boolean state.
+        # Regex literals require expanding `advance_js_default_scan_state`; nested template
+        # literals would also require replacing `advance_js_string_state` with stack-aware
+        # handling. Both changes need a new state-machine branch; the current callers were
+        # specifically designed around the simpler scanner and would need re-validation against
+        # the expanded state set.
         #
         # IMPORTANT CALLER CONTRACT — block-comment exit:
         # When this returns from a `*/` exit, `state` is cleared, but `char` is still the `*` and
@@ -915,9 +961,16 @@ def rsc_client_references_setup_anchor?(content, is_server:)
         # this helper deliberately omits the `RSCWebpackPlugin` import that `inject_rsc_*_imports`
         # adds on the from-scratch path — adding it here would produce a duplicate import.
         def add_rsc_client_references_setup(config_path, content, existing_imports_content, is_server:)
-          # Belt-and-suspenders: the only caller, `ensure_rsc_client_references_setup`, already
-          # checks both `scoped_rsc_client_references_defined?` and `rsc_client_references_defined?`
-          # before delegating here. The guards are kept so the helper is safe to call directly.
+          # The only caller, `ensure_rsc_client_references_setup`, already runs these same checks
+          # before delegating here, so in normal flow both conditions evaluate to `false` and no
+          # early return is triggered. They are kept (rather than deleted) so a future second
+          # caller — or a refactor that bypasses `ensure_rsc_client_references_setup` — cannot
+          # accidentally splice a second `const rscClientReferences = { ... }` into a file that
+          # already declares one. JavaScript would reject that with an
+          # `Identifier 'rscClientReferences' has already been declared` SyntaxError at config
+          # load, and the cost of the duplicate check is two boolean ops on the already-loaded
+          # file body. Leaving the method defensive is cheaper than re-deriving the precondition
+          # at each new call site.
           return false if scoped_rsc_client_references_defined?(content)
           return false if rsc_client_references_defined?(content)
 

react_on_rails/spec/react_on_rails/generators/js_dependency_manager_spec.rb

@@ -228,6 +228,16 @@ def errors
       expect(package_json.dig("dependencies", "react-on-rails-pro")).to eq("16.7.0-rc.1")
     end
 
+    it "formats the package.json pin fallback warning with name@version package specs" do
+      warning = instance.send(
+        :package_json_pin_fallback_warning,
+        [["react", "18.0.0"], ["react-dom", "18.0.0"]]
+      )
+
+      expect(warning).to include("react@18.0.0, react-dom@18.0.0")
+      expect(warning).to include("Wrote the following version pins to package.json")
+    end
+
     it "does not write unversioned dependencies to package.json when package-manager install fails" do
       File.write("package.json", "#{JSON.pretty_generate({ 'dependencies' => {} })}\n")
       instance.add_npm_dependencies_result = false