Fix shell injection vulnerabilities and RuboCop violations

Security fixes:
- Replace backticks with secure Open3.capture3 calls in system_checker.rb
- Fix node_missing? and cli_exists? methods to prevent shell injection
- Update check_node_version to use Open3 instead of shell redirection

Code quality improvements:
- Fix semantic version comparison bug (replace float math with Gem::Version)
- Correct debug command from node --inspect-brk to ./bin/shakapacker --debug-shakapacker
- Add trailing newline to USAGE file
- Fix RuboCop violations with inline disable comments

Test updates:
- Update test stubs to mock Open3.capture3 instead of backticks
- Fix test expectations for updated version comparison logic
- Use verified doubles and proper line length formatting

Documentation:
- Add prominent linting requirements to CLAUDE.md
- Emphasize mandatory RuboCop compliance before commits

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

Co-Authored-By: Claude <[email protected]>

Author: justin808

CLAUDE.md

@@ -2,6 +2,15 @@
 
 This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
 
+## ⚠️ CRITICAL REQUIREMENTS
+
+**BEFORE EVERY COMMIT/PUSH:**
+1. **ALWAYS run `bundle exec rubocop` and fix ALL violations**
+2. **ALWAYS ensure files end with a newline character**
+3. **NEVER push without running full lint check first**
+
+These requirements are non-negotiable. CI will fail if not followed.
+
 ## Development Commands
 
 ### Essential Commands
@@ -11,7 +20,8 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
   - Ruby tests: `rake run_rspec`
   - JavaScript tests: `yarn run test` or `rake js_tests`
   - All tests: `rake` (default task runs lint and all tests except examples)
-- **Linting**:
+- **Linting** (MANDATORY BEFORE EVERY COMMIT):
+  - **REQUIRED**: `bundle exec rubocop` - Must pass with zero offenses
   - All linters: `rake lint` (runs ESLint and RuboCop)
   - ESLint only: `yarn run lint` or `rake lint:eslint`
   - RuboCop only: `rake lint:rubocop`
@@ -20,7 +30,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
   - Check formatting without fixing: `yarn start format.listDifferent`
 - **Build**: `yarn run build` (compiles TypeScript to JavaScript in node_package/lib)
 - **Type checking**: `yarn run type-check`
-- **Before git push**: `rake lint` and autofix and resolve non-autofix issues.
+- **⚠️ MANDATORY BEFORE GIT PUSH**: `bundle exec rubocop` and fix ALL violations + ensure trailing newlines
 
 ### Development Setup Commands
 

lib/generators/react_on_rails/USAGE

@@ -62,4 +62,4 @@ Exit codes:
 For more help:
     • Documentation: https://github.com/shakacode/react_on_rails
     • Issues: https://github.com/shakacode/react_on_rails/issues
-    • Discord: https://discord.gg/reactrails
+    • Discord: https://discord.gg/reactrails

lib/react_on_rails/system_checker.rb

@@ -203,7 +203,7 @@ def check_react_on_rails_npm_package
       add_warning("⚠️  Could not parse package.json")
     end
 
-    def check_package_version_sync
+    def check_package_version_sync # rubocop:disable Metrics/CyclomaticComplexity
       return unless File.exist?("package.json")
 
       begin
@@ -225,7 +225,7 @@ def check_package_version_sync
           gem_major = gem_version.split(".")[0].to_i
           npm_major = clean_npm_version.split(".")[0].to_i
 
-          if gem_major != npm_major
+          if gem_major != npm_major # rubocop:disable Style/NegatedIfElseCondition
             add_error(<<~MSG.strip)
               🚫 Major version mismatch detected:
               • Gem version: #{gem_version} (major: #{gem_major})
@@ -311,7 +311,7 @@ def suggest_webpack_inspection
 
       add_info("💡 Advanced webpack debugging:")
       add_info("    1. Add 'debugger;' before 'module.exports' in config/webpack/webpack.config.js")
-      add_info("    2. Run: node --inspect-brk ./bin/shakapacker")
+      add_info("    2. Run: ./bin/shakapacker --debug-shakapacker")
       add_info("    3. Open Chrome DevTools to inspect config object")
       add_info("    📖 See: https://github.com/shakacode/shakapacker/blob/main/docs/troubleshooting.md#debugging-your-webpack-config")
 
@@ -371,15 +371,15 @@ def check_webpack_config_content
     private
 
     def node_missing?
-      if ReactOnRails::Utils.running_on_windows?
-        `where node 2>/dev/null`.strip.empty?
-      else
-        `which node 2>/dev/null`.strip.empty?
-      end
+      command = ReactOnRails::Utils.running_on_windows? ? "where" : "which"
+      _stdout, _stderr, status = Open3.capture3(command, "node")
+      !status.success?
     end
 
     def cli_exists?(command)
-      system("which #{command} > /dev/null 2>&1")
+      which_command = ReactOnRails::Utils.running_on_windows? ? "where" : "which"
+      _stdout, _stderr, status = Open3.capture3(which_command, command)
+      status.success?
     end
 
     def detect_used_package_manager
@@ -589,16 +589,6 @@ def report_dependency_versions(package_json)
     end
     # rubocop:enable Metrics/CyclomaticComplexity
 
-    def extract_major_minor_version(version_string)
-      # Extract major.minor from version string like "8.1.0" or "7.2.1"
-      match = version_string.match(/^(\d+)\.(\d+)/)
-      return nil unless match
-
-      major = match[1].to_i
-      minor = match[2].to_i
-      major + (minor / 10.0)
-    end
-
     def report_shakapacker_version
       return unless File.exist?("Gemfile.lock")
 
@@ -626,13 +616,19 @@ def report_shakapacker_version_with_threshold
 
         if shakapacker_match
           version = shakapacker_match[1].strip
-          major_minor = extract_major_minor_version(version)
 
-          if major_minor && major_minor >= 8.2
-            add_success("✅ Shakapacker #{version} (supports React on Rails auto-registration)")
-          elsif major_minor
-            add_warning("⚠️  Shakapacker #{version} - Version 8.2+ needed for React on Rails auto-registration")
-          else
+          begin
+            # Use proper semantic version comparison
+            version_obj = Gem::Version.new(version)
+            threshold_version = Gem::Version.new("8.2")
+
+            if version_obj >= threshold_version
+              add_success("✅ Shakapacker #{version} (supports React on Rails auto-registration)")
+            else
+              add_warning("⚠️  Shakapacker #{version} - Version 8.2+ needed for React on Rails auto-registration")
+            end
+          rescue ArgumentError
+            # Fallback for invalid version strings
             add_success("✅ Shakapacker #{version}")
           end
         else

spec/lib/react_on_rails/system_checker_spec.rb

@@ -66,7 +66,8 @@
   describe "#check_node_version" do
     context "when Node.js version is too old" do
       before do
-        allow(checker).to receive(:`).with("node --version 2>/dev/null").and_return("v16.14.0\n")
+        allow(Open3).to receive(:capture3).with("node", "--version")
+                    .and_return(["v16.14.0\n", "", instance_double(Process::Status, success?: true)])
       end
 
       it "adds a warning message" do
@@ -78,7 +79,8 @@
 
     context "when Node.js version is compatible" do
       before do
-        allow(checker).to receive(:`).with("node --version 2>/dev/null").and_return("v18.17.0\n")
+        allow(Open3).to receive(:capture3).with("node", "--version")
+                    .and_return(["v18.17.0\n", "", instance_double(Process::Status, success?: true)])
       end
 
       it "adds a success message" do
@@ -91,7 +93,8 @@
 
     context "when Node.js version cannot be determined" do
       before do
-        allow(checker).to receive(:`).with("node --version 2>/dev/null").and_return("")
+        allow(Open3).to receive(:capture3).with("node", "--version")
+                    .and_return(["", "", instance_double(Process::Status, success?: false)])
       end
 
       it "does not add any messages" do
@@ -122,12 +125,19 @@
         allow(checker).to receive(:cli_exists?).with("yarn").and_return(true)
         allow(checker).to receive(:cli_exists?).with("pnpm").and_return(false)
         allow(checker).to receive(:cli_exists?).with("bun").and_return(false)
+        # Mock file existence checks for lock files so detect_used_package_manager returns nil
+        allow(File).to receive(:exist?).with("yarn.lock").and_return(false)
+        allow(File).to receive(:exist?).with("pnpm-lock.yaml").and_return(false)
+        allow(File).to receive(:exist?).with("bun.lockb").and_return(false)
+        allow(File).to receive(:exist?).with("package-lock.json").and_return(false)
       end
 
       it "adds a success message" do
         result = checker.check_package_manager
         expect(result).to be true
-        expect(checker.messages.any? { |msg| msg[:type] == :success && msg[:content].include?("npm, yarn") }).to be true
+        expect(checker.messages.any? do |msg|
+                 msg[:type] == :success && msg[:content].include?("Package managers available: npm, yarn")
+               end).to be true
       end
     end
   end
@@ -150,13 +160,17 @@
       before do
         allow(checker).to receive(:shakapacker_configured?).and_return(true)
         allow(checker).to receive(:check_shakapacker_in_gemfile)
+        allow(File).to receive(:exist?).with("Gemfile.lock").and_return(true)
+        lockfile_content = %(GEM\n  remote: https://rubygems.org/\n  specs:\n) +
+                           %(    activesupport (7.1.3.2)\n    shakapacker (8.2.0)\n      activesupport (>= 5.2)\n)
+        allow(File).to receive(:read).with("Gemfile.lock").and_return(lockfile_content)
       end
 
       it "adds a success message and checks gemfile" do
         result = checker.check_shakapacker_configuration
         expect(result).to be true
         expect(checker.messages.any? do |msg|
-                 msg[:type] == :success && msg[:content].include?("Shakapacker is properly configured")
+                 msg[:type] == :success && msg[:content].include?("Shakapacker 8.2.0")
                end).to be true
         expect(checker).to have_received(:check_shakapacker_in_gemfile)
       end
@@ -223,8 +237,9 @@
       end
 
       before do
-        allow(File).to receive(:exist?).with("Gemfile").and_return(true)
-        allow(File).to receive(:read).with("Gemfile").and_return(gemfile_content)
+        gemfile_path = ENV["BUNDLE_GEMFILE"] || "Gemfile"
+        allow(File).to receive(:exist?).with(gemfile_path).and_return(true)
+        allow(File).to receive(:read).with(gemfile_path).and_return(gemfile_content)
         stub_const("ReactOnRails::VERSION", "16.0.0")
       end
 
@@ -244,8 +259,9 @@
       end
 
       before do
-        allow(File).to receive(:exist?).with("Gemfile").and_return(true)
-        allow(File).to receive(:read).with("Gemfile").and_return(gemfile_content)
+        gemfile_path = ENV["BUNDLE_GEMFILE"] || "Gemfile"
+        allow(File).to receive(:exist?).with(gemfile_path).and_return(true)
+        allow(File).to receive(:read).with(gemfile_path).and_return(gemfile_content)
       end
 
       it "does not warn about exact versions" do
@@ -308,12 +324,14 @@
   describe "private methods" do
     describe "#cli_exists?" do
       it "returns true when command exists" do
-        allow(checker).to receive(:system).with("which npm > /dev/null 2>&1").and_return(true)
+        allow(Open3).to receive(:capture3).with("which", "npm")
+                    .and_return(["", "", instance_double(Process::Status, success?: true)])
         expect(checker.send(:cli_exists?, "npm")).to be true
       end
 
       it "returns false when command does not exist" do
-        allow(checker).to receive(:system).with("which nonexistent > /dev/null 2>&1").and_return(false)
+        allow(Open3).to receive(:capture3).with("which", "nonexistent")
+                    .and_return(["", "", instance_double(Process::Status, success?: false)])
         expect(checker.send(:cli_exists?, "nonexistent")).to be false
       end
     end
@@ -359,10 +377,7 @@
 
         messages = checker.messages
         expect(messages.any? do |msg|
-                 msg[:type] == :info && msg[:content].include?("React version: ^18.2.0")
-               end).to be true
-        expect(messages.any? do |msg|
-                 msg[:type] == :info && msg[:content].include?("React DOM version: ^18.2.0")
+                 msg[:type] == :success && msg[:content].include?("React ^18.2.0, React DOM ^18.2.0")
                end).to be true
       end
     end