Fix: Universal process manager bundler context fallback #1833
Conversation
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 <[email protected]>
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 <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Bundler-aware process management: timeout-protected version checks, try-starting managers inside the bundle then outside it, Procfile path validation, unified installation/help messaging, and accompanying spec updates covering new helper methods and control flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant PM as ProcessManager
participant Bundler as Bundler
participant Sys as System
Dev->>PM: run_with_process_manager(procfile)
PM->>PM: validate procfile path
PM->>PM: iterate PROCESS_MANAGERS
PM->>PM: installed_in_current_context?(proc)
alt available in bundle
PM->>PM: run_process_if_available(proc, args) (inside bundle)
PM-->>Dev: started
else not in bundle
PM->>Bundler: with_unbundled_context {clear bundle env}
Bundler-->>PM: unbundled context
PM->>PM: process_available_in_system?(proc) (version checks, timeout)
alt available system-wide
PM->>Sys: run_process_outside_bundle(proc, args)
PM-->>Dev: started
else not found
PM-->>PM: try next manager
alt no managers left
PM-->>Dev: show_process_manager_installation_help (stderr)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1){Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2024-06-27T05:22:46.156ZApplied to files:
📚 Learning: 2025-09-29T14:45:42.687ZApplied to files:
🧬 Code graph analysis (1)spec/react_on_rails/dev/process_manager_spec.rb (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code ReviewThank you for this comprehensive fix! The solution elegantly handles the bundler context issue that was preventing foreman from working properly. Here's my review: ✅ Strengths
🔍 Observations & Suggestions
✅ Security & Performance
✅ Best Practices Compliance
🎯 Overall AssessmentThis is a high-quality PR that solves a real problem with a well-thought-out solution. The code is clean, well-tested, and maintains the project's best practices. The universal approach makes this future-proof for any process manager. Recommendation: ✅ Approve - Ready to merge after considering the minor suggestions above (none are blocking). Great work on identifying the root cause and implementing a robust solution! 🚀 |
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 <[email protected]>
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 <[email protected]>
Code Review for PR #1833: Universal Process Manager Bundler Context Fallback🎯 Overall AssessmentThis is a well-crafted solution that elegantly addresses the bundler context issue with process managers. The implementation is thoughtful, backwards-compatible, and future-proof. The code quality is excellent with comprehensive testing. ✅ Strengths1. Excellent Architecture
2. Robust Error Handling
3. Comprehensive Testing
4. Code Quality
🔍 Minor Suggestions for Improvement1. Consider Making Timeout ConfigurableThe 5-second timeout is hardcoded in multiple places. Consider extracting it to a constant: VERSION_CHECK_TIMEOUT = 5
Timeout.timeout(VERSION_CHECK_TIMEOUT) do
# ...
end2. Potential Race Condition in Version CheckThe version check creates subprocesses that might leave zombies if interrupted. Consider using Process.spawn with proper cleanup or Open3.popen3 for better process management. 3. Documentation EnhancementConsider adding YARD documentation to the new private methods, especially run_process_if_available and run_process_outside_bundle, as they implement the core fallback logic. ✅ Security & Performance
✅ Best Practices Compliance
📊 Test Coverage AnalysisThe test suite is thorough:
🎉 ConclusionAPPROVED - This is production-ready code that solves the problem elegantly while maintaining high code quality standards. The universal fallback pattern is a particularly clever solution that will benefit all users regardless of their process manager choice. The minor suggestions above are optional improvements that could be addressed in a follow-up PR if desired, but they don't block merging. Great work on this comprehensive fix! 🚀 |
Code Review for PR #1833: Universal Process Manager Bundler Context FallbackThank you for this comprehensive fix! The solution elegantly addresses the bundler context issue that was preventing ✅ Strengths1. Robust Fallback StrategyThe implementation of a two-tier fallback approach (current context → system context) is excellent:
2. Comprehensive Test CoverageExceptional testing with 22 test cases covering:
3. Improved Error UXThe new error message is significantly better:
4. Universal DesignThe refactoring from foreman-specific to process-agnostic code is forward-thinking and maintainable. 🔧 Suggestions for Improvement1. Timeout ConfigurationThe 5-second timeout is hardcoded. Consider making it configurable for slower systems: DEFAULT_VERSION_CHECK_TIMEOUT = 5
def installed_in_current_context?(process, timeout: DEFAULT_VERSION_CHECK_TIMEOUT)
# ...
Timeout.timeout(timeout) do2. Documentation EnhancementConsider adding a comment explaining why we need both # Check if a process is available system-wide (outside bundle context)
# This is needed because foreman might be installed globally but not in Gemfile,
# and bundler's binstub interception would prevent direct access
def process_available_in_system?(process)3. Performance OptimizationThe version checking logic could be optimized by caching results for the session: @version_check_cache ||= {}
return @version_check_cache[process] if @version_check_cache.key?(process)4. Minor Code RefinementIn def run_process_if_available(process, args)
# First attempt
if installed?(process)
system(process, *args)
return true
end
# Second attempt
if process_available_in_system?(process)
run_process_outside_bundle(process, args)
return true
end
false
end🛡️ Security & Performance✅ Security: Path validation remains intact, preventing shell injection 📝 Minor Observations
🎯 Overall AssessmentApproved with minor suggestions ✅ This is a well-thought-out solution that addresses the root cause while maintaining backward compatibility. The universal approach makes it future-proof for any new process managers. The test coverage is exemplary, and the error messages significantly improve the developer experience. The suggested improvements are optional enhancements that could be addressed in a follow-up PR if desired. The current implementation is solid and ready to merge. Great work on diagnosing and fixing this tricky bundler context issue! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
lib/react_on_rails/dev/process_manager.rb (2)
141-149: Harden procfile validation: ensure it’s a regular readable file.
File.readable?returns true for directories. Tighten to avoid passing a directory to-f.Apply this diff:
def valid_procfile_path?(procfile) # Reject paths with shell metacharacters return false if procfile.match?(/[;&|`$(){}\[\]<>]/) - # Ensure it's a readable file - File.readable?(procfile) + # Ensure it's a readable regular file + File.file?(procfile) && File.readable?(procfile) rescue StandardError false end
45-58: Minor: extract timeout to a constant and reuse.Hard‑coding
5in multiple places reduces clarity. Consider a single constant (e.g.,PROCESS_CHECK_TIMEOUT = 5) and reference it in both probes.For example:
# Near the top of the class PROCESS_CHECK_TIMEOUT = 5 # Usage Timeout.timeout(PROCESS_CHECK_TIMEOUT) { ... }spec/react_on_rails/dev/process_manager_spec.rb (4)
114-140: Add test: fallback when in‑context run fails (Bundler intercept).Currently missing the core regression test:
installed?true, in‑contextsystemreturns false, then fallback to out‑of‑bundle and succeed.Suggested spec:
it "falls back to system context when in-context execution 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) 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) result = described_class.send(:run_process_if_available, "foreman", ["start", "-f", "Procfile.dev"]) expect(result).to be true end
142-158: Add Bundler < 2 coverage forrun_process_outside_bundle.Test the
with_clean_envfallback whenwith_unbundled_envis not present.Suggested spec:
it "uses Bundler.with_clean_env when with_unbundled_env is unavailable" 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 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"]) end
160-199: Add Bundler < 2 coverage forprocess_available_in_system?.Ensure the availability probe works with
with_clean_envwhenwith_unbundled_envis absent.Suggested spec:
it "checks availability outside bundle using Bundler.with_clean_env when needed" 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 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) expect(described_class.send(:process_available_in_system?, "foreman")).to be true end
8-18: Close the File::NULL streams to avoid descriptor leaks.You reassign
$stdout/$stderrtoFile::NULLbut never close those handles.For example:
before(:all) do @original_stderr, @original_stdout = $stderr, $stdout @null_stderr = File.open(File::NULL, "w") @null_stdout = File.open(File::NULL, "w") $stderr = @null_stderr $stdout = @null_stdout end after(:all) do $stderr = @original_stderr $stdout = @original_stdout @null_stderr.close @null_stdout.close end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/dev/process_manager.rb(2 hunks)spec/react_on_rails/dev/process_manager_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}: Before every commit/push, runbundle exec rubocopand fix all violations in Ruby code
Let RuboCop handle all Ruby code formatting; never manually format Ruby files
Files:
spec/react_on_rails/dev/process_manager_spec.rblib/react_on_rails/dev/process_manager.rb
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
spec/react_on_rails/dev/process_manager_spec.rblib/react_on_rails/dev/process_manager.rb
🧬 Code graph analysis (1)
spec/react_on_rails/dev/process_manager_spec.rb (1)
lib/react_on_rails/dev/process_manager.rb (3)
installed?(7-151)installed?(11-13)run_with_process_manager(25-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (2)
lib/react_on_rails/dev/process_manager.rb (1)
35-41: Process manager order and messaging look good.Sequence (overmind → foreman) and the final guidance are clear. This will behave correctly once the fallback fix in
run_process_if_availablelands.Please re-run the failing scenario from the PR description after the fallback fix to confirm
bin/devnow succeeds under Bundler interception.spec/react_on_rails/dev/process_manager_spec.rb (1)
215-224: Help text assertions look solid.Covers headline, “do not bundle foreman” guidance, and docs link.
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 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
lib/react_on_rails/dev/process_manager.rb (4)
85-97: Make with_unbundled_context resilient to missing Bundler and simplify this call site.Right now you guard the call with defined?(Bundler). If with_unbundled_context handled “Bundler not defined” internally, you could always call it and drop the outer condition.
- def run_process_outside_bundle(process, args) - if defined?(Bundler) - with_unbundled_context do - system(process, *args) - end - else - # Fallback if Bundler is not available - system(process, *args) - end - end + def run_process_outside_bundle(process, args) + with_unbundled_context do + system(process, *args) + end + end
115-126: Harden helper to handle “Bundler not defined”; remove duplication at callers.Add a Bundler existence check here so callers can invoke unconditionally.
- 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 + def with_unbundled_context(&block) + unless defined?(Bundler) + return yield + end + + 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 + # Very old Bundler versions + yield + end + end
154-162: Overly strict path “sanitization” rejects valid paths; not needed without a shell.system is called with argv, not via a shell, so shell metacharacters in the procfile path don’t pose injection risk. The regex will block legitimate paths like “My (Project)/Procfile.dev”. Consider dropping it and rely on File.readable? (+ maybe NUL/newline check).
- def valid_procfile_path?(procfile) - # Reject paths with shell metacharacters - return false if procfile.match?(/[;&|`$(){}\[\]<>]/) - - # Ensure it's a readable file - File.readable?(procfile) + def valid_procfile_path?(procfile) + # Ensure it's a readable file; argv spawn avoids shell injection concerns + File.readable?(procfile) rescue StandardError false end
51-53: Tiny DRY: extract timeout into a private constant.Avoid repeating magic number 5; easier to tweak later and test.
class ProcessManager class << self + VERSION_CHECK_TIMEOUT = 5 + private_constant :VERSION_CHECK_TIMEOUT @@ - Timeout.timeout(5) do + Timeout.timeout(VERSION_CHECK_TIMEOUT) do system(process, flag, out: File::NULL, err: File::NULL) end @@ - Timeout.timeout(5) do + Timeout.timeout(VERSION_CHECK_TIMEOUT) do system(process, flag, out: File::NULL, err: File::NULL) endAlso applies to: 105-107
spec/react_on_rails/dev/process_manager_spec.rb (3)
8-18: Avoid FD leaks when silencing IO; prefer around(:each) or ensure close.before(:all) opens two File::NULL handles and never closes them. Use around with StringIO or ensure you close the files in after(:all).
- before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") - end + before(:all) do + @original_stderr = $stderr + @original_stdout = $stdout + @null_out = File.open(File::NULL, "w") + @null_err = File.open(File::NULL, "w") + $stdout = @null_out + $stderr = @null_err + end @@ - after(:all) do - $stderr = @original_stderr - $stdout = @original_stdout - end + after(:all) do + $stderr = @original_stderr + $stdout = @original_stdout + @null_out.close + @null_err.close + endAlternatively, use around(:each) with StringIO to isolate per‑example output.
142-156: run_process_outside_bundle exercised both branches.The helper usage is validated; will get simpler if you adopt unconditional with_unbundled_context.
243-252: Helpful messaging assertions.Good UX verification. Consider adding one spec for invalid procfile path (e.g., ’Procfile;rm’) to ensure we exit 1 with an error; it doubles as a regression test if you relax the sanitizer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/dev/process_manager.rb(2 hunks)spec/react_on_rails/dev/process_manager_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/dev/process_manager.rbspec/react_on_rails/dev/process_manager_spec.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.645Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.645Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
📚 Learning: 2025-09-29T14:45:42.645Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.645Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
lib/react_on_rails/dev/process_manager.rbspec/react_on_rails/dev/process_manager_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/dev/process_manager_spec.rb (1)
lib/react_on_rails/dev/process_manager.rb (3)
installed?(7-164)installed?(11-13)run_with_process_manager(25-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build
- GitHub Check: build-and-test
🔇 Additional comments (9)
lib/react_on_rails/dev/process_manager.rb (4)
35-41: Startup order and fallback trigger look solid.Prefers overmind, then foreman, and cleanly falls back to guidance + exit. No concerns.
45-58: Current‑context probe is appropriate and mirrors execution semantics.Using system(...) with version flags under a short timeout is exactly what we want here.
Based on learnings
72-83: Fallback strategy matches Bundler interception behavior.Short‑circuit on current‑context success; otherwise probe and run in an unbundled context. LGTM.
Based on learnings
98-113: System‑context availability check is correct and API‑compatible via the helper.The timeout + multi‑flag probe inside with_unbundled_context is good.
spec/react_on_rails/dev/process_manager_spec.rb (5)
79-112: High‑level flow tests read well.Happy path, fallback, and cleanup are covered succinctly.
114-140: run_process_if_available coverage is on point.Verifies in‑context, system‑context, and not‑available outcomes.
158-191: System‑context probe behavior well tested.Multi‑flag attempts and timeout branch are covered.
193-205: version_flags_for tests are precise and future‑proof.Clear expectations per process.
207-241: Bundler API compatibility thoroughly covered.Nice coverage of with_unbundled_env, with_clean_env, and bare yield.
| # 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 |
There was a problem hiding this comment.
timeout duration is hardcoded here and in the tests, probably better moving it to a constant variable
There was a problem hiding this comment.
@claude do it
There was a problem hiding this comment.
Claude finished @AbanoubGhadban's task —— View job
✅ Completed
I've successfully extracted the hardcoded timeout value to a constant as requested.
Changes made:
- ✅ Added
VERSION_CHECK_TIMEOUT = 5constant to the ProcessManager class - ✅ Replaced all hardcoded
5timeout values in the implementation (2 occurrences) - ✅ Updated all test expectations to use
described_class::VERSION_CHECK_TIMEOUT(8 occurrences) - ✅ Committed and pushed changes to the PR branch
Files modified:
lib/react_on_rails/dev/process_manager.rb- Added constant and updated 2 timeout referencesspec/react_on_rails/dev/process_manager_spec.rb- Updated 8 test expectations
The constant is now defined at the class level, making it easy to adjust the timeout duration in one place and improving both maintainability and testability.
--- | View commit
Extract hardcoded timeout value to VERSION_CHECK_TIMEOUT constant for better maintainability and testability. Co-authored-by: Abanoub Ghadban <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/react_on_rails/dev/process_manager.rb (1)
157-165: Overly strict procfile path validation blocks valid filenamesRejecting (), [], {}, <> prevents legitimate paths and isn’t needed since system is called with args (no shell). Tighten to regular, readable file check.
Apply this diff:
- def valid_procfile_path?(procfile) - # Reject paths with shell metacharacters - return false if procfile.match?(/[;&|`$(){}\[\]<>]/) - - # Ensure it's a readable file - File.readable?(procfile) - rescue StandardError - false - 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) + rescue StandardError + false + end
🧹 Nitpick comments (3)
lib/react_on_rails/dev/process_manager.rb (1)
8-10: DRY process manager selection for easy extensionExtract the process managers list to a constant and iterate. Eases future additions.
Apply these diffs:
Add constant near other constants:
class ProcessManager # Timeout for version check operations to prevent hanging VERSION_CHECK_TIMEOUT = 5 + PROCESS_MANAGERS = %w[overmind foreman].freezeIterate in run_with_process_manager:
- # 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]) + # Try process managers in order of preference + PROCESS_MANAGERS.each do |pm| + return if run_process_if_available(pm, ["start", "-f", procfile]) + endAlso applies to: 38-44
spec/react_on_rails/dev/process_manager_spec.rb (2)
8-18: Avoid global $stdout/$stderr reassignments; close FDs if keptReassigning globally is brittle and leaks descriptors. Either remove and rely on
to outputmatchers or at least close the opened files.Apply this to prevent leaks:
before(:all) do - @original_stderr = $stderr - @original_stdout = $stdout - $stderr = File.open(File::NULL, "w") - $stdout = File.open(File::NULL, "w") + @original_stderr = $stderr + @original_stdout = $stdout + @silenced_stderr = File.open(File::NULL, "w") + @silenced_stdout = File.open(File::NULL, "w") + $stderr = @silenced_stderr + $stdout = @silenced_stdout end after(:all) do $stderr = @original_stderr $stdout = @original_stdout + @silenced_stderr.close + @silenced_stdout.close endAlternatively, drop this block and use
allow(Kernel).to receive(:warn)to silence specific messages.
72-112: Add a spec for invalid Procfile path handlingCurrently no test asserts we exit on invalid paths. Add one to pin security behavior.
Example:
describe ".run_with_process_manager" do before do allow(ReactOnRails::Dev::FileManager).to receive(:cleanup_stale_files) allow_any_instance_of(Kernel).to receive(:system).and_return(true) allow(File).to receive(:readable?).and_return(true) end + it "exits with error on invalid procfile path" do + expect(File).to receive(:readable?).with("Procfile (dev)").and_return(true) + expect(described_class).to receive(:run_process_if_available).never + expect_any_instance_of(Kernel).to receive(:warn).with(/Invalid procfile path/) + expect_any_instance_of(Kernel).to receive(:exit).with(1) + described_class.run_with_process_manager("Procfile (dev)") + endNote: Update expected behavior if you adopt the relaxed validation from the lib change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/dev/process_manager.rb(2 hunks)spec/react_on_rails/dev/process_manager_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
spec/react_on_rails/dev/process_manager_spec.rblib/react_on_rails/dev/process_manager.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
spec/react_on_rails/dev/process_manager_spec.rblib/react_on_rails/dev/process_manager.rb
🧬 Code graph analysis (1)
spec/react_on_rails/dev/process_manager_spec.rb (1)
lib/react_on_rails/dev/process_manager.rb (3)
installed?(7-167)installed?(14-16)run_with_process_manager(28-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build
🔇 Additional comments (5)
lib/react_on_rails/dev/process_manager.rb (3)
48-61: Bundler-aware installed? check is correct and resilientUsing version flags with Timeout mirrors actual exec behavior under Bundler interception. Good alignment with the intended detection. Based on learnings
75-86: Universal fallback flow is soundFirst try in-context, then unbundled system context; clear true/false return contract. Matches PR goals. Based on learnings
118-129: Bundler API compatibility handled wellwith_unbundled_env/with_clean_env fallback covers Bundler < 2. Safe final yield.
spec/react_on_rails/dev/process_manager_spec.rb (2)
21-31: Spec coverage around installed? behavior is strongGood use of timeouts, multiple flags, and failure modes. Mirrors production logic. Based on learnings
Also applies to: 33-57
79-112: Overall spec suite aligns well with new flowValidates in-context, system fallback, Bundler API variants, and help messaging.
Also applies to: 114-141, 143-156, 158-191, 193-241, 243-252
@claude, apply the suggested changes. But on the tests, use the |
|
Claude finished @AbanoubGhadban's task —— View job ✅ Applied CodeRabbit Review Suggestions
Changes Applied
|
- 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 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/dev/process_manager.rb(2 hunks)spec/react_on_rails/dev/process_manager_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/dev/process_manager.rbspec/react_on_rails/dev/process_manager_spec.rb
🧠 Learnings (1)
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
lib/react_on_rails/dev/process_manager.rbspec/react_on_rails/dev/process_manager_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/dev/process_manager_spec.rb (1)
lib/react_on_rails/dev/process_manager.rb (2)
installed?(17-19)run_with_process_manager(31-48)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/dev/process_manager.rb
[warning] 43-43: Lint/NonLocalExitFromIterator: Non-local exit from iterator, without return value. next, break, Array#find, Array#any?, etc. is preferred.
spec/react_on_rails/dev/process_manager_spec.rb
[warning] 15-15: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stderr instead of mutating $stderr.
[warning] 16-16: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stdout instead of mutating $stdout.
[warning] 19-19: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stderr instead of mutating $stderr.
[warning] 20-20: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stdout instead of mutating $stdout.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: claude-review
| around do |example| | ||
| original_stderr = $stderr | ||
| original_stdout = $stdout | ||
| null_stderr = File.open(File::NULL, "w") | ||
| null_stdout = File.open(File::NULL, "w") | ||
|
|
||
| after(:all) do | ||
| $stderr = @original_stderr | ||
| $stdout = @original_stdout | ||
| begin | ||
| $stderr = null_stderr | ||
| $stdout = null_stdout | ||
| example.run | ||
| ensure | ||
| $stderr = original_stderr | ||
| $stdout = original_stdout | ||
| null_stderr.close | ||
| null_stdout.close | ||
| end |
There was a problem hiding this comment.
Silence IO without mutating $stdout/$stderr to appease RuboCop.
The new around hook still trips RSpec/ExpectOutput because it reassigns the global streams. Stub the writes instead—this keeps output quiet, auto-resets between examples, and clears the RuboCop failure.
- 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
+ around do |example|
+ allow($stdout).to receive(:write)
+ allow($stderr).to receive(:write)
+ example.run
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| around do |example| | |
| original_stderr = $stderr | |
| original_stdout = $stdout | |
| null_stderr = File.open(File::NULL, "w") | |
| null_stdout = File.open(File::NULL, "w") | |
| after(:all) do | |
| $stderr = @original_stderr | |
| $stdout = @original_stdout | |
| begin | |
| $stderr = null_stderr | |
| $stdout = null_stdout | |
| example.run | |
| ensure | |
| $stderr = original_stderr | |
| $stdout = original_stdout | |
| null_stderr.close | |
| null_stdout.close | |
| end | |
| around do |example| | |
| allow($stdout).to receive(:write) | |
| allow($stderr).to receive(:write) | |
| example.run | |
| end |
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 15-15: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stderr instead of mutating $stderr.
[warning] 16-16: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stdout instead of mutating $stdout.
[warning] 19-19: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stderr instead of mutating $stderr.
[warning] 20-20: RSpec/ExpectOutput: Use expect { ... }.to output(...).to_stdout instead of mutating $stdout.
🤖 Prompt for AI Agents
In spec/react_on_rails/dev/process_manager_spec.rb around lines 8 to 23, replace
the around hook that reassigns $stdout/$stderr with stubbing their output
methods so we don't mutate global streams; specifically, remove the File::NULL
open/reassign logic and instead stub $stdout and $stderr writes (e.g.
allow($stdout).to receive(:write).and_return(nil) and similarly stub :puts,
:print, and :flush on both streams) before example.run so output is suppressed
and RSpec/mock reset restores behavior automatically.
|
@claude revert changes made at file |
|
Claude finished @AbanoubGhadban's task —— View job ✅ Reverted process_manager.rb changes
SummarySuccessfully reverted the changes made to Reverted changes:
Preserved changes:
The file has been successfully reverted and pushed to the branch. View job | View commit | Branch: |
- 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 <[email protected]>
|
@claude fix linting check on CI |
|
Claude finished @AbanoubGhadban's task —— View job ✅ Fixed linting violations
SummaryFixed the RuboCop linting violations:
The tests that need to capture output (like Changes
View commit | Branch: |
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 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/react_on_rails/dev/process_manager_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
spec/react_on_rails/dev/process_manager_spec.rb
🧠 Learnings (2)
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
PR: shakacode/react_on_rails#1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/dev/process_manager_spec.rb
📚 Learning: 2025-09-29T14:45:42.687Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1833
File: lib/react_on_rails/dev/process_manager.rb:72-83
Timestamp: 2025-09-29T14:45:42.687Z
Learning: In Ruby bundler contexts, when bundler intercepts system commands for executables not in the Gemfile, both version checks (like `system("foreman", "--version")`) and execution commands (like `system("foreman", "start", ...)`) fail equally, both returning false. This means availability checks using version flags accurately reflect whether execution commands will work in the current bundler context.
Applied to files:
spec/react_on_rails/dev/process_manager_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/dev/process_manager_spec.rb (1)
lib/react_on_rails/dev/process_manager.rb (3)
installed?(7-167)installed?(14-16)run_with_process_manager(28-44)
🪛 GitHub Actions: Lint JS and Ruby
spec/react_on_rails/dev/process_manager_spec.rb
[error] 7-7: Rubocop: Layout/EmptyLinesAroundBlockBody - Extra empty line detected at block body beginning. Offense autocorrectable.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: claude-review
🔇 Additional comments (9)
spec/react_on_rails/dev/process_manager_spec.rb (9)
8-45: Comprehensive test coverage for installed?The test suite thoroughly covers all version check scenarios: current context availability, ENOENT failures, multiple flag attempts, partial success, and timeout handling. The expectations correctly use
VERSION_CHECK_TIMEOUTconstant and stub system calls with proper output redirection.
47-58: LGTM! Procfile validation tests are clear.Both scenarios are properly tested: successful validation when file exists and error exit when file is missing.
60-100: Well-structured tests for process manager orchestration.The tests verify the correct sequential fallback logic (overmind → foreman), error handling when no manager is available, and proper cleanup invocation. The setup block appropriately stubs common dependencies.
102-128: Excellent coverage of availability fallback logic.The tests correctly verify the three-tier check: (1) current context availability, (2) system context fallback, and (3) unavailability handling. The test isolation is proper with appropriate stubbing.
130-144: Proper testing of Bundler context awareness.The tests verify correct behavior with and without Bundler present, using
hide_constappropriately to simulate environments without Bundler.
146-179: Thorough testing of system-wide availability checks.The tests cover all important scenarios: successful detection with version flags, Bundler absence, multiple flag attempts, and timeout handling. The use of
VERSION_CHECK_TIMEOUTconstant is consistent.
181-193: Clear tests for version flag lookup.The tests verify correct flag sets for known processes (overmind, foreman) and the generic fallback for unknown processes.
195-229: Comprehensive Bundler API compatibility tests.The tests verify the correct fallback chain:
with_unbundled_env→with_clean_env→ direct yield. The use ofclass_doubleandstub_constproperly isolates the tests.
231-240: Appropriate verification of error messaging.The test correctly verifies that installation help is written to stderr and includes key information: error status, Gemfile warning, and documentation link reference.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary
Fixes #1832
This PR resolves the issue where
bin/devfails when foreman is not included in the Gemfile, implementing a comprehensive solution that works for all process managers (overmind, foreman, and future additions).Problem
The dummy app's
bin/devscript was failing with:However,
foreman start -f Procfile.devworked fine when run directly.Root Cause
Solution
🎯 Universal Bundler Context Fallback
Implemented an intelligent fallback strategy that works for all process managers:
Bundler.with_unbundled_envto access system-installed processes🚀 Key Improvements
installed?method correctly detects process usability in current execution contextrun_process_if_available(process, args)works for any process managerArchitecture
Before (Foreman-specific)
After (Universal)
Benefits
✅ Maintains best practices: Keeps foreman out of Gemfile
✅ Backwards compatible: Works with projects that do include process managers
✅ Future-proof: Any new process manager automatically gets fallback support
✅ Better UX: Clear error messages guide users to proper setup
✅ Robust: Handles both bundled and system installations universally
Testing
bin/devnow works correctlyRelated
docs/javascript/foreman-issues.md🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Tests