From 193a7d1ac3863c55cdf90ae2046ebe89e6afb9cd Mon Sep 17 00:00:00 2001 From: Example User Date: Sun, 28 Sep 2025 15:28:28 +0300 Subject: [PATCH 01/11] Fix foreman execution in bundler context with intelligent fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit resolves the issue where bin/dev fails when foreman is not included in the Gemfile (following React on Rails best practices). The fix implements an intelligent fallback strategy: 1. First attempt: Try running foreman within bundler context 2. Fallback: Use Bundler.with_unbundled_env to run system-installed foreman 3. Enhanced error handling with helpful user guidance Key improvements: - Maintains compatibility with projects that include foreman in Gemfile - Supports React on Rails best practice of system-installed foreman - Provides clear error messages linking to documentation - Comprehensive test coverage for all fallback scenarios Fixes #1832 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/process_manager.rb | 81 ++++++++++++- .../dev/process_manager_spec.rb | 107 +++++++++++++++++- 2 files changed, 179 insertions(+), 9 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 8e70ad85ed..3dc420e75c 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -33,19 +33,88 @@ def run_with_process_manager(procfile) if installed?("overmind") system("overmind", "start", "-f", procfile) - elsif installed?("foreman") - system("foreman", "start", "-f", procfile) + elsif foreman_available? + run_foreman(procfile) else - warn <<~MSG - NOTICE: - For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. - MSG + show_process_manager_installation_help exit 1 end end private + # Check if foreman is available in either bundler context or system-wide + def foreman_available? + installed?("foreman") || foreman_available_in_system? + end + + # Try to run foreman with intelligent fallback strategy + # First attempt: within bundler context (for projects that include foreman in Gemfile) + # Fallback: outside bundler context (for projects following React on Rails best practices) + def run_foreman(procfile) + success = if installed?("foreman") + # Try within bundle context first + system("foreman", "start", "-f", procfile) + else + false + end + + # If bundler context failed or foreman not in bundle, try system foreman + return if success + + run_foreman_outside_bundle(procfile) + end + + # Run foreman outside of bundler context using Bundler.with_unbundled_env + # This allows using system-installed foreman even when it's not in the Gemfile + def run_foreman_outside_bundle(procfile) + if defined?(Bundler) + Bundler.with_unbundled_env do + system("foreman", "start", "-f", procfile) + end + else + # Fallback if Bundler is not available + system("foreman", "start", "-f", procfile) + end + end + + # Check if foreman is available system-wide (outside bundle context) + def foreman_available_in_system? + if defined?(Bundler) + Bundler.with_unbundled_env do + installed?("foreman") + end + else + false + end + end + + # Improved error message with helpful guidance + def show_process_manager_installation_help + warn <<~MSG + ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + ERROR: Process Manager Not Found + ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + To run the React on Rails development server, you need either 'overmind' or + 'foreman' installed on your system. + + RECOMMENDED INSTALLATION: + + macOS: brew install overmind + Linux: gem install foreman + + IMPORTANT: + DO NOT add foreman to your Gemfile. Install it globally on your system. + + For more information about why foreman should not be in your Gemfile, see: + https://github.com/shakacode/react_on_rails/blob/master/docs/javascript/foreman-issues.md + + After installation, try running this script again. + ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + MSG + end + def valid_procfile_path?(procfile) # Reject paths with shell metacharacters return false if procfile.match?(/[;&|`$(){}\[\]<>]/) diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 978ada0bce..b538e749fd 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -56,17 +56,19 @@ described_class.run_with_process_manager("Procfile.dev") end - it "uses foreman when overmind not available" do + it "uses foreman when overmind not available and foreman is in bundle" do allow(described_class).to receive(:installed?).with("overmind").and_return(false) + allow(described_class).to receive(:foreman_available?).and_return(true) allow(described_class).to receive(:installed?).with("foreman").and_return(true) - expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") + expect(described_class).to receive(:run_foreman).with("Procfile.dev") described_class.run_with_process_manager("Procfile.dev") end it "exits with error when no process manager available" do allow(described_class).to receive(:installed?).with("overmind").and_return(false) - allow(described_class).to receive(:installed?).with("foreman").and_return(false) + allow(described_class).to receive(:foreman_available?).and_return(false) + expect(described_class).to receive(:show_process_manager_installation_help) expect_any_instance_of(Kernel).to receive(:exit).with(1) described_class.run_with_process_manager("Procfile.dev") @@ -79,4 +81,103 @@ described_class.run_with_process_manager("Procfile.dev") end end + + describe ".foreman_available?" do + it "returns true when foreman is available in bundle context" do + allow(described_class).to receive(:installed?).with("foreman").and_return(true) + allow(described_class).to receive(:foreman_available_in_system?).and_return(false) + + expect(described_class.send(:foreman_available?)).to be true + end + + it "returns true when foreman is available system-wide" do + allow(described_class).to receive(:installed?).with("foreman").and_return(false) + allow(described_class).to receive(:foreman_available_in_system?).and_return(true) + + expect(described_class.send(:foreman_available?)).to be true + end + + it "returns false when foreman is not available anywhere" do + allow(described_class).to receive(:installed?).with("foreman").and_return(false) + allow(described_class).to receive(:foreman_available_in_system?).and_return(false) + + expect(described_class.send(:foreman_available?)).to be false + end + end + + describe ".run_foreman" do + before do + allow_any_instance_of(Kernel).to receive(:system).and_return(true) + end + + it "tries bundle context first when foreman is in bundle" do + allow(described_class).to receive(:installed?).with("foreman").and_return(true) + expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev").and_return(true) + expect(described_class).not_to receive(:run_foreman_outside_bundle) + + described_class.send(:run_foreman, "Procfile.dev") + end + + it "falls back to system foreman when bundle context fails" do + allow(described_class).to receive(:installed?).with("foreman").and_return(true) + expect_any_instance_of(Kernel).to receive(:system) + .with("foreman", "start", "-f", "Procfile.dev").and_return(false) + expect(described_class).to receive(:run_foreman_outside_bundle).with("Procfile.dev") + + described_class.send(:run_foreman, "Procfile.dev") + end + + it "uses system foreman directly when not in bundle" do + allow(described_class).to receive(:installed?).with("foreman").and_return(false) + expect(described_class).to receive(:run_foreman_outside_bundle).with("Procfile.dev") + + described_class.send(:run_foreman, "Procfile.dev") + end + end + + describe ".run_foreman_outside_bundle" do + it "uses Bundler.with_unbundled_env when Bundler is available" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") + + described_class.send(:run_foreman_outside_bundle, "Procfile.dev") + end + + it "falls back to direct system call when Bundler is not available" do + hide_const("Bundler") + expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") + + described_class.send(:run_foreman_outside_bundle, "Procfile.dev") + end + end + + describe ".foreman_available_in_system?" do + it "checks foreman availability outside bundle context" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(described_class).to receive(:installed?).with("foreman").and_return(true) + + expect(described_class.send(:foreman_available_in_system?)).to be true + end + + it "returns false when Bundler is not available" do + hide_const("Bundler") + + expect(described_class.send(:foreman_available_in_system?)).to be false + end + end + + describe ".show_process_manager_installation_help" do + it "displays helpful error message with installation instructions" do + expect { described_class.send(:show_process_manager_installation_help) } + .to output(/ERROR: Process Manager Not Found/).to_stderr + expect { described_class.send(:show_process_manager_installation_help) } + .to output(/DO NOT add foreman to your Gemfile/).to_stderr + expect { described_class.send(:show_process_manager_installation_help) } + .to output(/foreman-issues\.md/).to_stderr + end + end end From 7bb2c8367aef6e880c39da884f4cc9e1661d7845 Mon Sep 17 00:00:00 2001 From: Example User Date: Sun, 28 Sep 2025 19:12:50 +0300 Subject: [PATCH 02/11] Enhance process manager with universal bundler context fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit generalizes the foreman-specific bundler context fix to work for ALL process managers (overmind, foreman, and any future additions). Key improvements: - Context-aware process detection for all processes, not just foreman - Universal fallback mechanism using Bundler.with_unbundled_env - Consistent API with run_process(process, args) for any process manager - Enhanced test coverage for generalized functionality Benefits: - Robust handling of bundler context for any process manager - Future-proof architecture for new process managers - Consistent behavior across all system commands - Eliminates bundler interceptor issues universally Builds on previous commit to create a comprehensive solution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/process_manager.rb | 68 +++++++----- .../dev/process_manager_spec.rb | 105 +++++++++++------- 2 files changed, 102 insertions(+), 71 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 3dc420e75c..8247786134 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -4,11 +4,10 @@ module ReactOnRails module Dev class ProcessManager class << self + # Check if a process is available and usable in the current execution context + # This accounts for bundler context where system commands might be intercepted def installed?(process) - IO.popen([process, "-v"], &:close) - true - rescue Errno::ENOENT - false + installed_in_current_context?(process) end def ensure_procfile(procfile) @@ -31,10 +30,10 @@ def run_with_process_manager(procfile) # Clean up stale files before starting FileManager.cleanup_stale_files - if installed?("overmind") - system("overmind", "start", "-f", procfile) - elsif foreman_available? - run_foreman(procfile) + if process_available?("overmind") + run_process("overmind", ["start", "-f", procfile]) + elsif process_available?("foreman") + run_process("foreman", ["start", "-f", procfile]) else show_process_manager_installation_help exit 1 @@ -43,50 +42,63 @@ def run_with_process_manager(procfile) private - # Check if foreman is available in either bundler context or system-wide - def foreman_available? - installed?("foreman") || foreman_available_in_system? + # Check if a process is actually usable in the current execution context + # This is important for commands that might be intercepted by bundler + def installed_in_current_context?(process) + # Try to execute the process with a simple flag to see if it works + # Use system() because that's how we'll actually call it later + system(process, "--version", out: File::NULL, err: File::NULL) + rescue Errno::ENOENT + false + end + + # Check if a process is available in either current context or system-wide + def process_available?(process) + installed?(process) || process_available_in_system?(process) end - # Try to run foreman with intelligent fallback strategy - # First attempt: within bundler context (for projects that include foreman in Gemfile) - # Fallback: outside bundler context (for projects following React on Rails best practices) - def run_foreman(procfile) - success = if installed?("foreman") - # Try within bundle context first - system("foreman", "start", "-f", procfile) + # Try to run a process with intelligent fallback strategy + # First attempt: within current context (for processes that are in the current bundle) + # Fallback: outside bundler context (for system-installed processes) + def run_process(process, args) + success = if installed?(process) + # Process works in current context - use it directly + system(process, *args) else false end - # If bundler context failed or foreman not in bundle, try system foreman + # If current context failed or process not available, try system process return if success - run_foreman_outside_bundle(procfile) + run_process_outside_bundle(process, args) end - # Run foreman outside of bundler context using Bundler.with_unbundled_env - # This allows using system-installed foreman even when it's not in the Gemfile - def run_foreman_outside_bundle(procfile) + # Run a process outside of bundler context using Bundler.with_unbundled_env + # This allows using system-installed processes even when they're not in the Gemfile + def run_process_outside_bundle(process, args) if defined?(Bundler) Bundler.with_unbundled_env do - system("foreman", "start", "-f", procfile) + system(process, *args) end else # Fallback if Bundler is not available - system("foreman", "start", "-f", procfile) + system(process, *args) end end - # Check if foreman is available system-wide (outside bundle context) - def foreman_available_in_system? + # Check if a process is available system-wide (outside bundle context) + def process_available_in_system?(process) if defined?(Bundler) Bundler.with_unbundled_env do - installed?("foreman") + # Use system() directly to check if process exists outside bundler context + system(process, "--version", out: File::NULL, err: File::NULL) end else false end + rescue Errno::ENOENT + false end # Improved error message with helpful guidance diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index b538e749fd..4122a798d0 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -18,15 +18,23 @@ end describe ".installed?" do - it "returns true when process is available" do - allow(IO).to receive(:popen).with(["overmind", "-v"]).and_return("Some version info") + it "returns true when process is available in current context" do + expect_any_instance_of(Kernel).to receive(:system) + .with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(true) expect(described_class).to be_installed("overmind") end - it "returns false when process is not available" do - allow(IO).to receive(:popen).with(["nonexistent", "-v"]).and_raise(Errno::ENOENT) + it "returns false when process is not available in current context" do + expect_any_instance_of(Kernel).to receive(:system) + .with("nonexistent", "--version", out: File::NULL, err: File::NULL).and_raise(Errno::ENOENT) expect(described_class.installed?("nonexistent")).to be false end + + it "returns false when process returns false" do + expect_any_instance_of(Kernel).to receive(:system) + .with("failing_process", "--version", out: File::NULL, err: File::NULL).and_return(false) + expect(described_class.installed?("failing_process")).to be false + end end describe ".ensure_procfile" do @@ -50,24 +58,23 @@ end it "uses overmind when available" do - allow(described_class).to receive(:installed?).with("overmind").and_return(true) - expect_any_instance_of(Kernel).to receive(:system).with("overmind", "start", "-f", "Procfile.dev") + allow(described_class).to receive(:process_available?).with("overmind").and_return(true) + expect(described_class).to receive(:run_process).with("overmind", ["start", "-f", "Procfile.dev"]) described_class.run_with_process_manager("Procfile.dev") end - it "uses foreman when overmind not available and foreman is in bundle" do - allow(described_class).to receive(:installed?).with("overmind").and_return(false) - allow(described_class).to receive(:foreman_available?).and_return(true) - allow(described_class).to receive(:installed?).with("foreman").and_return(true) - expect(described_class).to receive(:run_foreman).with("Procfile.dev") + it "uses foreman when overmind not available and foreman is available" do + allow(described_class).to receive(:process_available?).with("overmind").and_return(false) + allow(described_class).to receive(:process_available?).with("foreman").and_return(true) + expect(described_class).to receive(:run_process).with("foreman", ["start", "-f", "Procfile.dev"]) described_class.run_with_process_manager("Procfile.dev") end it "exits with error when no process manager available" do - allow(described_class).to receive(:installed?).with("overmind").and_return(false) - allow(described_class).to receive(:foreman_available?).and_return(false) + allow(described_class).to receive(:process_available?).with("overmind").and_return(false) + allow(described_class).to receive(:process_available?).with("foreman").and_return(false) expect(described_class).to receive(:show_process_manager_installation_help) expect_any_instance_of(Kernel).to receive(:exit).with(1) @@ -75,98 +82,110 @@ end it "cleans up stale files before starting" do - allow(described_class).to receive(:installed?).with("overmind").and_return(true) + allow(described_class).to receive(:process_available?).with("overmind").and_return(true) + allow(described_class).to receive(:run_process) expect(ReactOnRails::Dev::FileManager).to receive(:cleanup_stale_files) described_class.run_with_process_manager("Procfile.dev") end end - describe ".foreman_available?" do - it "returns true when foreman is available in bundle context" do + describe ".process_available?" do + it "returns true when process is available in current context" do allow(described_class).to receive(:installed?).with("foreman").and_return(true) - allow(described_class).to receive(:foreman_available_in_system?).and_return(false) + allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(false) - expect(described_class.send(:foreman_available?)).to be true + expect(described_class.send(:process_available?, "foreman")).to be true end - it "returns true when foreman is available system-wide" do + it "returns true when process is available system-wide" do allow(described_class).to receive(:installed?).with("foreman").and_return(false) - allow(described_class).to receive(:foreman_available_in_system?).and_return(true) + allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(true) - expect(described_class.send(:foreman_available?)).to be true + expect(described_class.send(:process_available?, "foreman")).to be true end - it "returns false when foreman is not available anywhere" do + it "returns false when process is not available anywhere" do allow(described_class).to receive(:installed?).with("foreman").and_return(false) - allow(described_class).to receive(:foreman_available_in_system?).and_return(false) + allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(false) - expect(described_class.send(:foreman_available?)).to be false + expect(described_class.send(:process_available?, "foreman")).to be false end end - describe ".run_foreman" do + describe ".run_process" do before do allow_any_instance_of(Kernel).to receive(:system).and_return(true) end - it "tries bundle context first when foreman is in bundle" do + it "tries current context first when process works there" do allow(described_class).to receive(:installed?).with("foreman").and_return(true) expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev").and_return(true) - expect(described_class).not_to receive(:run_foreman_outside_bundle) + expect(described_class).not_to receive(:run_process_outside_bundle) - described_class.send(:run_foreman, "Procfile.dev") + described_class.send(:run_process, "foreman", ["start", "-f", "Procfile.dev"]) end - it "falls back to system foreman when bundle context fails" do + it "falls back to system process when current context fails" do allow(described_class).to receive(:installed?).with("foreman").and_return(true) expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "start", "-f", "Procfile.dev").and_return(false) - expect(described_class).to receive(:run_foreman_outside_bundle).with("Procfile.dev") + expect(described_class).to receive(:run_process_outside_bundle).with("foreman", ["start", "-f", "Procfile.dev"]) - described_class.send(:run_foreman, "Procfile.dev") + described_class.send(:run_process, "foreman", ["start", "-f", "Procfile.dev"]) end - it "uses system foreman directly when not in bundle" do - allow(described_class).to receive(:installed?).with("foreman").and_return(false) - expect(described_class).to receive(:run_foreman_outside_bundle).with("Procfile.dev") + it "uses system process directly when not available in current context" do + allow(described_class).to receive(:installed?).with("overmind").and_return(false) + expect(described_class).to receive(:run_process_outside_bundle).with("overmind", ["start", "-f", "Procfile.dev"]) - described_class.send(:run_foreman, "Procfile.dev") + described_class.send(:run_process, "overmind", ["start", "-f", "Procfile.dev"]) end end - describe ".run_foreman_outside_bundle" do + describe ".run_process_outside_bundle" do it "uses Bundler.with_unbundled_env when Bundler is available" do bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") - described_class.send(:run_foreman_outside_bundle, "Procfile.dev") + described_class.send(:run_process_outside_bundle, "foreman", ["start", "-f", "Procfile.dev"]) end it "falls back to direct system call when Bundler is not available" do hide_const("Bundler") expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") - described_class.send(:run_foreman_outside_bundle, "Procfile.dev") + described_class.send(:run_process_outside_bundle, "foreman", ["start", "-f", "Procfile.dev"]) end end - describe ".foreman_available_in_system?" do - it "checks foreman availability outside bundle context" do + describe ".process_available_in_system?" do + it "checks process availability outside bundle context" do bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield - expect(described_class).to receive(:installed?).with("foreman").and_return(true) + expect_any_instance_of(Kernel).to receive(:system) + .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(true) - expect(described_class.send(:foreman_available_in_system?)).to be true + expect(described_class.send(:process_available_in_system?, "foreman")).to be true end it "returns false when Bundler is not available" do hide_const("Bundler") - expect(described_class.send(:foreman_available_in_system?)).to be false + expect(described_class.send(:process_available_in_system?, "foreman")).to be false + end + + it "returns false when process fails outside bundle context" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect_any_instance_of(Kernel).to receive(:system) + .with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(false) + + expect(described_class.send(:process_available_in_system?, "overmind")).to be false end end From d63cee87a596ca32505cf6118ea56cf6376b9cd0 Mon Sep 17 00:00:00 2001 From: Example User Date: Sun, 28 Sep 2025 19:22:54 +0300 Subject: [PATCH 03/11] Optimize process manager performance and robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit improves the process manager implementation with several optimizations: Performance improvements: - Eliminate redundant system calls in run_process_if_available - Streamlined control flow with early returns - More efficient process availability checking Robustness enhancements: - Support multiple version flags per process (--version, -v, -V) - Process-specific version flag mapping for better compatibility - Improved fallback strategy that avoids double execution Code quality: - Cleaner, more readable method structure - Better separation of concerns - Enhanced test coverage for version flag handling - RuboCop compliant code style The implementation is now more efficient and robust while maintaining full backward compatibility and comprehensive bundler context handling. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/process_manager.rb | 71 +++++------ .../dev/process_manager_spec.rb | 113 +++++++++--------- 2 files changed, 96 insertions(+), 88 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 8247786134..e67b066fee 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -30,14 +30,12 @@ def run_with_process_manager(procfile) # Clean up stale files before starting FileManager.cleanup_stale_files - if process_available?("overmind") - run_process("overmind", ["start", "-f", procfile]) - elsif process_available?("foreman") - run_process("foreman", ["start", "-f", procfile]) - else - show_process_manager_installation_help - exit 1 - end + # Try process managers in order of preference + return if run_process_if_available("overmind", ["start", "-f", procfile]) + return if run_process_if_available("foreman", ["start", "-f", procfile]) + + show_process_manager_installation_help + exit 1 end private @@ -45,33 +43,38 @@ def run_with_process_manager(procfile) # Check if a process is actually usable in the current execution context # This is important for commands that might be intercepted by bundler def installed_in_current_context?(process) - # Try to execute the process with a simple flag to see if it works + # Try to execute the process with version flags to see if it works # Use system() because that's how we'll actually call it later - system(process, "--version", out: File::NULL, err: File::NULL) + version_flags_for(process).any? do |flag| + system(process, flag, out: File::NULL, err: File::NULL) + end rescue Errno::ENOENT false end - # Check if a process is available in either current context or system-wide - def process_available?(process) - installed?(process) || process_available_in_system?(process) + # Get appropriate version flags for different processes + def version_flags_for(process) + case process + when "overmind" + ["--version"] + when "foreman" + ["--version", "-v"] + else + ["--version", "-v", "-V"] + end end - # Try to run a process with intelligent fallback strategy - # First attempt: within current context (for processes that are in the current bundle) - # Fallback: outside bundler context (for system-installed processes) - def run_process(process, args) - success = if installed?(process) - # Process works in current context - use it directly - system(process, *args) - else - false - end - - # If current context failed or process not available, try system process - return if success - - run_process_outside_bundle(process, args) + # Try to run a process if it's available, with intelligent fallback strategy + # Returns true if process was found and executed, false if not available + def run_process_if_available(process, args) + # First attempt: try in current context (works for bundled processes) + return system(process, *args) if installed?(process) + + # Second attempt: try in system context (works for system-installed processes) + return run_process_outside_bundle(process, args) if process_available_in_system?(process) + + # Process not available in either context + false end # Run a process outside of bundler context using Bundler.with_unbundled_env @@ -89,13 +92,13 @@ def run_process_outside_bundle(process, args) # Check if a process is available system-wide (outside bundle context) def process_available_in_system?(process) - if defined?(Bundler) - Bundler.with_unbundled_env do - # Use system() directly to check if process exists outside bundler context - system(process, "--version", out: File::NULL, err: File::NULL) + return false unless defined?(Bundler) + + Bundler.with_unbundled_env do + # Try version flags to check if process exists outside bundler context + version_flags_for(process).any? do |flag| + system(process, flag, out: File::NULL, err: File::NULL) end - else - false end rescue Errno::ENOENT false diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 4122a798d0..b8dd43381e 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -30,11 +30,23 @@ expect(described_class.installed?("nonexistent")).to be false end - it "returns false when process returns false" do + it "returns false when all version flags fail" do expect_any_instance_of(Kernel).to receive(:system) .with("failing_process", "--version", out: File::NULL, err: File::NULL).and_return(false) + expect_any_instance_of(Kernel).to receive(:system) + .with("failing_process", "-v", out: File::NULL, err: File::NULL).and_return(false) + expect_any_instance_of(Kernel).to receive(:system) + .with("failing_process", "-V", out: File::NULL, err: File::NULL).and_return(false) expect(described_class.installed?("failing_process")).to be false end + + it "returns true when second version flag succeeds" do + expect_any_instance_of(Kernel).to receive(:system) + .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) + expect_any_instance_of(Kernel).to receive(:system) + .with("foreman", "-v", out: File::NULL, err: File::NULL).and_return(true) + expect(described_class.installed?("foreman")).to be true + end end describe ".ensure_procfile" do @@ -58,23 +70,26 @@ end it "uses overmind when available" do - allow(described_class).to receive(:process_available?).with("overmind").and_return(true) - expect(described_class).to receive(:run_process).with("overmind", ["start", "-f", "Procfile.dev"]) + expect(described_class).to receive(:run_process_if_available) + .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(true) described_class.run_with_process_manager("Procfile.dev") end it "uses foreman when overmind not available and foreman is available" do - allow(described_class).to receive(:process_available?).with("overmind").and_return(false) - allow(described_class).to receive(:process_available?).with("foreman").and_return(true) - expect(described_class).to receive(:run_process).with("foreman", ["start", "-f", "Procfile.dev"]) + expect(described_class).to receive(:run_process_if_available) + .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false) + expect(described_class).to receive(:run_process_if_available) + .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(true) described_class.run_with_process_manager("Procfile.dev") end it "exits with error when no process manager available" do - allow(described_class).to receive(:process_available?).with("overmind").and_return(false) - allow(described_class).to receive(:process_available?).with("foreman").and_return(false) + expect(described_class).to receive(:run_process_if_available) + .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false) + expect(described_class).to receive(:run_process_if_available) + .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(false) expect(described_class).to receive(:show_process_manager_installation_help) expect_any_instance_of(Kernel).to receive(:exit).with(1) @@ -82,64 +97,38 @@ end it "cleans up stale files before starting" do - allow(described_class).to receive(:process_available?).with("overmind").and_return(true) - allow(described_class).to receive(:run_process) + allow(described_class).to receive(:run_process_if_available).and_return(true) expect(ReactOnRails::Dev::FileManager).to receive(:cleanup_stale_files) described_class.run_with_process_manager("Procfile.dev") end end - describe ".process_available?" do - it "returns true when process is available in current context" do + describe ".run_process_if_available" do + it "returns true and runs process when available in current context" do allow(described_class).to receive(:installed?).with("foreman").and_return(true) - allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(false) + expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev").and_return(true) - expect(described_class.send(:process_available?, "foreman")).to be true + result = described_class.send(:run_process_if_available, "foreman", ["start", "-f", "Procfile.dev"]) + expect(result).to be true end - it "returns true when process is available system-wide" do + it "tries system context when not available in current context" do allow(described_class).to receive(:installed?).with("foreman").and_return(false) allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(true) + expect(described_class).to receive(:run_process_outside_bundle) + .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(true) - expect(described_class.send(:process_available?, "foreman")).to be true - end - - it "returns false when process is not available anywhere" do - allow(described_class).to receive(:installed?).with("foreman").and_return(false) - allow(described_class).to receive(:process_available_in_system?).with("foreman").and_return(false) - - expect(described_class.send(:process_available?, "foreman")).to be false - end - end - - describe ".run_process" do - before do - allow_any_instance_of(Kernel).to receive(:system).and_return(true) - end - - it "tries current context first when process works there" do - allow(described_class).to receive(:installed?).with("foreman").and_return(true) - expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev").and_return(true) - expect(described_class).not_to receive(:run_process_outside_bundle) - - described_class.send(:run_process, "foreman", ["start", "-f", "Procfile.dev"]) - end - - it "falls back to system process when current context fails" do - allow(described_class).to receive(:installed?).with("foreman").and_return(true) - expect_any_instance_of(Kernel).to receive(:system) - .with("foreman", "start", "-f", "Procfile.dev").and_return(false) - expect(described_class).to receive(:run_process_outside_bundle).with("foreman", ["start", "-f", "Procfile.dev"]) - - described_class.send(:run_process, "foreman", ["start", "-f", "Procfile.dev"]) + result = described_class.send(:run_process_if_available, "foreman", ["start", "-f", "Procfile.dev"]) + expect(result).to be true end - it "uses system process directly when not available in current context" do - allow(described_class).to receive(:installed?).with("overmind").and_return(false) - expect(described_class).to receive(:run_process_outside_bundle).with("overmind", ["start", "-f", "Procfile.dev"]) + it "returns false when process not available anywhere" do + allow(described_class).to receive(:installed?).with("nonexistent").and_return(false) + allow(described_class).to receive(:process_available_in_system?).with("nonexistent").and_return(false) - described_class.send(:run_process, "overmind", ["start", "-f", "Procfile.dev"]) + result = described_class.send(:run_process_if_available, "nonexistent", ["start"]) + expect(result).to be false end end @@ -162,7 +151,7 @@ end describe ".process_available_in_system?" do - it "checks process availability outside bundle context" do + it "checks process availability outside bundle context with version flags" do bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield @@ -178,14 +167,30 @@ expect(described_class.send(:process_available_in_system?, "foreman")).to be false end - it "returns false when process fails outside bundle context" do + it "tries multiple version flags before failing" do bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield expect_any_instance_of(Kernel).to receive(:system) - .with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(false) + .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) + expect_any_instance_of(Kernel).to receive(:system) + .with("foreman", "-v", out: File::NULL, err: File::NULL).and_return(true) + + expect(described_class.send(:process_available_in_system?, "foreman")).to be true + end + end + + describe ".version_flags_for" do + it "returns specific flags for overmind" do + expect(described_class.send(:version_flags_for, "overmind")).to eq(["--version"]) + end + + it "returns multiple flags for foreman" do + expect(described_class.send(:version_flags_for, "foreman")).to eq(["--version", "-v"]) + end - expect(described_class.send(:process_available_in_system?, "overmind")).to be false + it "returns generic flags for unknown processes" do + expect(described_class.send(:version_flags_for, "unknown")).to eq(["--version", "-v", "-V"]) end end From ad9a44ffedbe2b70b750567b790cb69ec9ec3f19 Mon Sep 17 00:00:00 2001 From: Example User Date: Sun, 28 Sep 2025 19:34:22 +0300 Subject: [PATCH 04/11] Add timeout protection for version checking operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a 5-second timeout to all version checking operations to prevent the development setup from hanging indefinitely on unresponsive process managers. Safety improvements: - Timeout protection for installed_in_current_context? checks - Timeout protection for process_available_in_system? checks - Graceful handling of Timeout::Error alongside Errno::ENOENT - Prevents hanging development setup on problematic installations Implementation details: - Uses Ruby's Timeout module with 5-second limit - Only applied to version checking, not main process execution - Main process execution remains unlimited (expected for dev servers) - Comprehensive test coverage for timeout scenarios The timeout is conservative (5 seconds) to account for slow systems while still preventing indefinite hangs that would block developer workflow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/process_manager.rb | 16 ++++++++++---- .../dev/process_manager_spec.rb | 22 +++++++++++++++++-- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index e67b066fee..2b532ccd66 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "timeout" + module ReactOnRails module Dev class ProcessManager @@ -46,9 +48,12 @@ def installed_in_current_context?(process) # Try to execute the process with version flags to see if it works # Use system() because that's how we'll actually call it later version_flags_for(process).any? do |flag| - system(process, flag, out: File::NULL, err: File::NULL) + # Add timeout to prevent hanging on version checks + Timeout.timeout(5) do + system(process, flag, out: File::NULL, err: File::NULL) + end end - rescue Errno::ENOENT + rescue Errno::ENOENT, Timeout::Error false end @@ -97,10 +102,13 @@ def process_available_in_system?(process) Bundler.with_unbundled_env do # Try version flags to check if process exists outside bundler context version_flags_for(process).any? do |flag| - system(process, flag, out: File::NULL, err: File::NULL) + # Add timeout to prevent hanging on version checks + Timeout.timeout(5) do + system(process, flag, out: File::NULL, err: File::NULL) + end end end - rescue Errno::ENOENT + rescue Errno::ENOENT, Timeout::Error false end diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index b8dd43381e..3e0a7d51d6 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -19,18 +19,19 @@ describe ".installed?" do it "returns true when process is available in current context" do + expect(Timeout).to receive(:timeout).with(5).and_yield expect_any_instance_of(Kernel).to receive(:system) .with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(true) expect(described_class).to be_installed("overmind") end it "returns false when process is not available in current context" do - expect_any_instance_of(Kernel).to receive(:system) - .with("nonexistent", "--version", out: File::NULL, err: File::NULL).and_raise(Errno::ENOENT) + expect(Timeout).to receive(:timeout).with(5).and_raise(Errno::ENOENT) expect(described_class.installed?("nonexistent")).to be false end it "returns false when all version flags fail" do + expect(Timeout).to receive(:timeout).with(5).exactly(3).times.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("failing_process", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) @@ -41,12 +42,18 @@ end it "returns true when second version flag succeeds" do + expect(Timeout).to receive(:timeout).with(5).twice.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "-v", out: File::NULL, err: File::NULL).and_return(true) expect(described_class.installed?("foreman")).to be true end + + it "returns false when version check times out" do + expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error) + expect(described_class.installed?("hanging_process")).to be false + end end describe ".ensure_procfile" do @@ -155,6 +162,7 @@ bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(Timeout).to receive(:timeout).with(5).and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(true) @@ -171,6 +179,7 @@ bundler_double = class_double(Bundler) stub_const("Bundler", bundler_double) expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(Timeout).to receive(:timeout).with(5).twice.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) @@ -178,6 +187,15 @@ expect(described_class.send(:process_available_in_system?, "foreman")).to be true end + + it "returns false when version check times out in system context" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error) + + expect(described_class.send(:process_available_in_system?, "hanging_process")).to be false + end end describe ".version_flags_for" do From 1feef0589852cf6760e5d3804e2be0a20680bf74 Mon Sep 17 00:00:00 2001 From: Example User Date: Mon, 29 Sep 2025 19:18:58 +0300 Subject: [PATCH 05/11] Add Bundler API compatibility with DRY helper method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement with_unbundled_context helper that supports both modern (with_unbundled_env) and legacy (with_clean_env) Bundler APIs, with graceful fallback for very old versions. This addresses backward compatibility concerns while maintaining clean, DRY code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/react_on_rails/dev/process_manager.rb | 19 +++++-- .../dev/process_manager_spec.rb | 54 ++++++++++++++----- 2 files changed, 57 insertions(+), 16 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 2b532ccd66..7fb1acf5ad 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -82,11 +82,11 @@ def run_process_if_available(process, args) false end - # Run a process outside of bundler context using Bundler.with_unbundled_env + # Run a process outside of bundler context # This allows using system-installed processes even when they're not in the Gemfile def run_process_outside_bundle(process, args) if defined?(Bundler) - Bundler.with_unbundled_env do + with_unbundled_context do system(process, *args) end else @@ -99,7 +99,7 @@ def run_process_outside_bundle(process, args) def process_available_in_system?(process) return false unless defined?(Bundler) - Bundler.with_unbundled_env do + with_unbundled_context do # Try version flags to check if process exists outside bundler context version_flags_for(process).any? do |flag| # Add timeout to prevent hanging on version checks @@ -112,6 +112,19 @@ def process_available_in_system?(process) false end + # DRY helper method for Bundler context switching with API compatibility + # Supports both new (with_unbundled_env) and legacy (with_clean_env) Bundler APIs + def with_unbundled_context(&block) + if Bundler.respond_to?(:with_unbundled_env) + Bundler.with_unbundled_env(&block) + elsif Bundler.respond_to?(:with_clean_env) + Bundler.with_clean_env(&block) + else + # Fallback if neither method is available (very old Bundler versions) + yield + end + end + # Improved error message with helpful guidance def show_process_manager_installation_help warn <<~MSG diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 3e0a7d51d6..08264cfb72 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -140,10 +140,8 @@ end describe ".run_process_outside_bundle" do - it "uses Bundler.with_unbundled_env when Bundler is available" do - bundler_double = class_double(Bundler) - stub_const("Bundler", bundler_double) - expect(bundler_double).to receive(:with_unbundled_env).and_yield + it "uses with_unbundled_context when Bundler is available" do + expect(described_class).to receive(:with_unbundled_context).and_yield expect_any_instance_of(Kernel).to receive(:system).with("foreman", "start", "-f", "Procfile.dev") described_class.send(:run_process_outside_bundle, "foreman", ["start", "-f", "Procfile.dev"]) @@ -159,9 +157,7 @@ describe ".process_available_in_system?" do it "checks process availability outside bundle context with version flags" do - bundler_double = class_double(Bundler) - stub_const("Bundler", bundler_double) - expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(described_class).to receive(:with_unbundled_context).and_yield expect(Timeout).to receive(:timeout).with(5).and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(true) @@ -176,9 +172,7 @@ end it "tries multiple version flags before failing" do - bundler_double = class_double(Bundler) - stub_const("Bundler", bundler_double) - expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(described_class).to receive(:with_unbundled_context).and_yield expect(Timeout).to receive(:timeout).with(5).twice.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) @@ -189,9 +183,7 @@ end it "returns false when version check times out in system context" do - bundler_double = class_double(Bundler) - stub_const("Bundler", bundler_double) - expect(bundler_double).to receive(:with_unbundled_env).and_yield + expect(described_class).to receive(:with_unbundled_context).and_yield expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error) expect(described_class.send(:process_available_in_system?, "hanging_process")).to be false @@ -212,6 +204,42 @@ end end + describe ".with_unbundled_context" do + it "uses with_unbundled_env when available" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + allow(bundler_double).to receive(:respond_to?).with(:with_unbundled_env).and_return(true) + expect(bundler_double).to receive(:with_unbundled_env).and_yield + + yielded = false + described_class.send(:with_unbundled_context) { yielded = true } + expect(yielded).to be true + end + + it "falls back to with_clean_env when with_unbundled_env not available" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + allow(bundler_double).to receive(:respond_to?).with(:with_unbundled_env).and_return(false) + allow(bundler_double).to receive(:respond_to?).with(:with_clean_env).and_return(true) + expect(bundler_double).to receive(:with_clean_env).and_yield + + yielded = false + described_class.send(:with_unbundled_context) { yielded = true } + expect(yielded).to be true + end + + it "yields directly when neither method is available" do + bundler_double = class_double(Bundler) + stub_const("Bundler", bundler_double) + allow(bundler_double).to receive(:respond_to?).with(:with_unbundled_env).and_return(false) + allow(bundler_double).to receive(:respond_to?).with(:with_clean_env).and_return(false) + + yielded = false + described_class.send(:with_unbundled_context) { yielded = true } + expect(yielded).to be true + end + end + describe ".show_process_manager_installation_help" do it "displays helpful error message with installation instructions" do expect { described_class.send(:show_process_manager_installation_help) } From 153e9651be99d74dc6e9f558e05c249d6430a4b6 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:06:14 +0000 Subject: [PATCH 06/11] Add timeout protection for version checking operations Extract hardcoded timeout value to VERSION_CHECK_TIMEOUT constant for better maintainability and testability. Co-authored-by: Abanoub Ghadban --- lib/react_on_rails/dev/process_manager.rb | 7 +++++-- spec/react_on_rails/dev/process_manager_spec.rb | 16 ++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 7fb1acf5ad..7ecae3996c 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -5,6 +5,9 @@ module ReactOnRails module Dev class ProcessManager + # Timeout for version check operations to prevent hanging + VERSION_CHECK_TIMEOUT = 5 + class << self # Check if a process is available and usable in the current execution context # This accounts for bundler context where system commands might be intercepted @@ -49,7 +52,7 @@ def installed_in_current_context?(process) # Use system() because that's how we'll actually call it later version_flags_for(process).any? do |flag| # Add timeout to prevent hanging on version checks - Timeout.timeout(5) do + Timeout.timeout(VERSION_CHECK_TIMEOUT) do system(process, flag, out: File::NULL, err: File::NULL) end end @@ -103,7 +106,7 @@ def process_available_in_system?(process) # Try version flags to check if process exists outside bundler context version_flags_for(process).any? do |flag| # Add timeout to prevent hanging on version checks - Timeout.timeout(5) do + Timeout.timeout(VERSION_CHECK_TIMEOUT) do system(process, flag, out: File::NULL, err: File::NULL) end end diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 08264cfb72..4301105f61 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -19,19 +19,19 @@ describe ".installed?" do it "returns true when process is available in current context" do - expect(Timeout).to receive(:timeout).with(5).and_yield + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield expect_any_instance_of(Kernel).to receive(:system) .with("overmind", "--version", out: File::NULL, err: File::NULL).and_return(true) expect(described_class).to be_installed("overmind") end it "returns false when process is not available in current context" do - expect(Timeout).to receive(:timeout).with(5).and_raise(Errno::ENOENT) + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Errno::ENOENT) expect(described_class.installed?("nonexistent")).to be false end it "returns false when all version flags fail" do - expect(Timeout).to receive(:timeout).with(5).exactly(3).times.and_yield + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).exactly(3).times.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("failing_process", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) @@ -42,7 +42,7 @@ end it "returns true when second version flag succeeds" do - expect(Timeout).to receive(:timeout).with(5).twice.and_yield + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).twice.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) @@ -51,7 +51,7 @@ end it "returns false when version check times out" do - expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error) + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Timeout::Error) expect(described_class.installed?("hanging_process")).to be false end end @@ -158,7 +158,7 @@ describe ".process_available_in_system?" do it "checks process availability outside bundle context with version flags" do expect(described_class).to receive(:with_unbundled_context).and_yield - expect(Timeout).to receive(:timeout).with(5).and_yield + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(true) @@ -173,7 +173,7 @@ it "tries multiple version flags before failing" do expect(described_class).to receive(:with_unbundled_context).and_yield - expect(Timeout).to receive(:timeout).with(5).twice.and_yield + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).twice.and_yield expect_any_instance_of(Kernel).to receive(:system) .with("foreman", "--version", out: File::NULL, err: File::NULL).and_return(false) expect_any_instance_of(Kernel).to receive(:system) @@ -184,7 +184,7 @@ it "returns false when version check times out in system context" do expect(described_class).to receive(:with_unbundled_context).and_yield - expect(Timeout).to receive(:timeout).with(5).and_raise(Timeout::Error) + expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_raise(Timeout::Error) expect(described_class.send(:process_available_in_system?, "hanging_process")).to be false end From 5740b0e9771572038dac4a2a20e05ba3dac68c82 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:19:11 +0000 Subject: [PATCH 07/11] Apply CodeRabbit review suggestions - Extract process managers to PROCESS_MANAGERS constant for DRY code - Relax procfile path validation (remove shell metacharacter check) - Refactor tests to use around hook with ensure clause for proper cleanup - Close file descriptors properly to avoid leaks Co-authored-by: Abanoub Ghadban --- lib/react_on_rails/dev/process_manager.rb | 16 +++++++----- .../dev/process_manager_spec.rb | 26 ++++++++++++------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index 7ecae3996c..edf7da26ff 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -8,6 +8,9 @@ class ProcessManager # Timeout for version check operations to prevent hanging VERSION_CHECK_TIMEOUT = 5 + # Process managers in order of preference + PROCESS_MANAGERS = %w[overmind foreman].freeze + class << self # Check if a process is available and usable in the current execution context # This accounts for bundler context where system commands might be intercepted @@ -36,8 +39,9 @@ def run_with_process_manager(procfile) FileManager.cleanup_stale_files # Try process managers in order of preference - return if run_process_if_available("overmind", ["start", "-f", procfile]) - return if run_process_if_available("foreman", ["start", "-f", procfile]) + PROCESS_MANAGERS.each do |pm| + return if run_process_if_available(pm, ["start", "-f", procfile]) + end show_process_manager_installation_help exit 1 @@ -155,11 +159,9 @@ def show_process_manager_installation_help end def valid_procfile_path?(procfile) - # Reject paths with shell metacharacters - return false if procfile.match?(/[;&|`$(){}\[\]<>]/) - - # Ensure it's a readable file - File.readable?(procfile) + # system is invoked with args (no shell), so shell metacharacters are safe. + # Ensure it's a readable regular file. + File.file?(procfile) && File.readable?(procfile) rescue StandardError false end diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 4301105f61..aa279c0c48 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -5,16 +5,22 @@ RSpec.describe ReactOnRails::Dev::ProcessManager do # Suppress stdout/stderr during tests - before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") - end - - after(:all) do - $stderr = @original_stderr - $stdout = @original_stdout + around do |example| + original_stderr = $stderr + original_stdout = $stdout + null_stderr = File.open(File::NULL, "w") + null_stdout = File.open(File::NULL, "w") + + begin + $stderr = null_stderr + $stdout = null_stdout + example.run + ensure + $stderr = original_stderr + $stdout = original_stdout + null_stderr.close + null_stdout.close + end end describe ".installed?" do From b778a93cf1aea6b57e08fda7385685fa35985d57 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 2 Oct 2025 10:30:17 +0000 Subject: [PATCH 08/11] Revert process_manager.rb changes from commit 5740b0e - Remove PROCESS_MANAGERS constant (revert to explicit calls) - Revert procfile validation to stricter shell metacharacter check - Keep spec file changes from commit 5740b0e (around hook improvements) Co-authored-by: Abanoub Ghadban --- lib/react_on_rails/dev/process_manager.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/react_on_rails/dev/process_manager.rb b/lib/react_on_rails/dev/process_manager.rb index edf7da26ff..7ecae3996c 100644 --- a/lib/react_on_rails/dev/process_manager.rb +++ b/lib/react_on_rails/dev/process_manager.rb @@ -8,9 +8,6 @@ class ProcessManager # Timeout for version check operations to prevent hanging VERSION_CHECK_TIMEOUT = 5 - # Process managers in order of preference - PROCESS_MANAGERS = %w[overmind foreman].freeze - class << self # Check if a process is available and usable in the current execution context # This accounts for bundler context where system commands might be intercepted @@ -39,9 +36,8 @@ def run_with_process_manager(procfile) FileManager.cleanup_stale_files # Try process managers in order of preference - PROCESS_MANAGERS.each do |pm| - return if run_process_if_available(pm, ["start", "-f", procfile]) - end + return if run_process_if_available("overmind", ["start", "-f", procfile]) + return if run_process_if_available("foreman", ["start", "-f", procfile]) show_process_manager_installation_help exit 1 @@ -159,9 +155,11 @@ def show_process_manager_installation_help end def valid_procfile_path?(procfile) - # system is invoked with args (no shell), so shell metacharacters are safe. - # Ensure it's a readable regular file. - File.file?(procfile) && File.readable?(procfile) + # Reject paths with shell metacharacters + return false if procfile.match?(/[;&|`$(){}\[\]<>]/) + + # Ensure it's a readable file + File.readable?(procfile) rescue StandardError false end From 00b3ea4cb3ba1d071af1cb5db3a70ed9929ccf41 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Thu, 2 Oct 2025 11:36:33 +0000 Subject: [PATCH 09/11] Fix RSpec/ExpectOutput violations by removing global stream reassignment Remove the around hook that was mutating $stdout/$stderr, which triggered RuboCop's RSpec/ExpectOutput warnings. Tests that need to capture output already use RSpec's .to output(...).to_stderr matcher correctly. Co-authored-by: Abanoub Ghadban --- .../react_on_rails/dev/process_manager_spec.rb | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index aa279c0c48..3a5f728914 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -4,24 +4,6 @@ require "react_on_rails/dev/process_manager" RSpec.describe ReactOnRails::Dev::ProcessManager do - # Suppress stdout/stderr during tests - around do |example| - original_stderr = $stderr - original_stdout = $stdout - null_stderr = File.open(File::NULL, "w") - null_stdout = File.open(File::NULL, "w") - - begin - $stderr = null_stderr - $stdout = null_stdout - example.run - ensure - $stderr = original_stderr - $stdout = original_stdout - null_stderr.close - null_stdout.close - end - end describe ".installed?" do it "returns true when process is available in current context" do From 5a856884bb9b37dfab47aa388a189bb1e8ed7631 Mon Sep 17 00:00:00 2001 From: Abanoub Ghadban Date: Thu, 2 Oct 2025 18:18:00 +0300 Subject: [PATCH 10/11] Update spec/react_on_rails/dev/process_manager_spec.rb Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- spec/react_on_rails/dev/process_manager_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 3a5f728914..6a73727e04 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -4,7 +4,8 @@ require "react_on_rails/dev/process_manager" RSpec.describe ReactOnRails::Dev::ProcessManager do - +RSpec.describe ReactOnRails::Dev::ProcessManager do + describe ".installed?" do describe ".installed?" do it "returns true when process is available in current context" do expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield From 6b5708f8ad2cddb6767d5d7ae554e607edd3b9e7 Mon Sep 17 00:00:00 2001 From: Abanoub Ghadban Date: Thu, 2 Oct 2025 18:18:40 +0300 Subject: [PATCH 11/11] Remove duplicate RSpec.describe for ProcessManager --- spec/react_on_rails/dev/process_manager_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/react_on_rails/dev/process_manager_spec.rb b/spec/react_on_rails/dev/process_manager_spec.rb index 6a73727e04..eda46a8352 100644 --- a/spec/react_on_rails/dev/process_manager_spec.rb +++ b/spec/react_on_rails/dev/process_manager_spec.rb @@ -4,8 +4,6 @@ require "react_on_rails/dev/process_manager" RSpec.describe ReactOnRails::Dev::ProcessManager do -RSpec.describe ReactOnRails::Dev::ProcessManager do - describe ".installed?" do describe ".installed?" do it "returns true when process is available in current context" do expect(Timeout).to receive(:timeout).with(described_class::VERSION_CHECK_TIMEOUT).and_yield