Fix bin/dev showing 'Process Manager Not Found' when overmind exists but exits

[justin808]

Summary

Fixes a bug where bin/dev shows the misleading error "Process Manager Not Found" when overmind is installed but exits with a non-zero code (e.g., when a Procfile process crashes).

Problem

After the v16.2.0.beta.12 release, users reported seeing:

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
ERROR: Process Manager Not Found
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

To run the React on Rails development server, you need either 'overmind' or
'foreman' installed on your system.

This error appeared even though overmind was installed and working. It occurred when overmind successfully started but then exited with a non-zero code (e.g., when a process in the Procfile died).

Root Cause

The run_with_process_manager method couldn't distinguish between:

• Process not found (should show installation help)
• Process found but exited with error (should just exit, process manager already showed its error)

The old logic:

return if run_process_if_available("overmind", ...)
return if run_process_if_available("foreman", ...)
show_process_manager_installation_help  # Always shown if neither returned!

When system('overmind', 'start', '-f', procfile) exited with error, it returned false, not nil, so the code continued to try foreman. If foreman wasn't installed, it showed the "not found" message incorrectly.

Solution

Changed run_process_if_available to return three distinct states:

• true: Process found and exited successfully (exit code 0)
• false: Process found but exited with error (non-zero exit code)
• nil: Process not found/available

Updated the error handling logic:

overmind_result = run_process_if_available("overmind", ...)
return if overmind_result == true  # Success!

foreman_result = run_process_if_available("foreman", ...)  
return if foreman_result == true  # Success!

# If found but failed, exit silently (they showed their own errors)
exit 1 if overmind_result == false || foreman_result == false

# Only show "not found" if NEITHER was found
show_process_manager_installation_help
exit 1

Testing

• ✅ Verified overmind detection still works correctly
• ✅ Tested scenario where overmind exits with error - correctly exits without showing "not found"
• ✅ Tested scenario where neither is installed - correctly shows installation help
• ✅ RuboCop passes
• ✅ RBS signatures updated and validated
• ✅ Manual testing in spec/dummy

Impact

Users will no longer see the confusing "Process Manager Not Found" error when their process manager is actually installed but a Procfile process crashes or exits with error.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 18 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3bd2f10 and ed84a0b.

📒 Files selected for processing (3)

• lib/react_on_rails/dev/process_manager.rb (3 hunks)
• sig/react_on_rails/dev/process_manager.rbs (1 hunks)
• spec/react_on_rails/dev/process_manager_spec.rb (2 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jg-/fix-bin-dev-overmind

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review Summary

This PR effectively fixes the misleading error message issue when process managers exit with errors. The implementation is solid, but I have identified one critical test coverage gap and some minor improvement opportunities.

✅ Strengths

1. Clear Problem & Solution: The PR description excellently documents the issue, root cause, and solution with examples
2. Type Safety: RBS signature correctly updated to reflect the tri-state return value (bool | nil)
3. Backward Compatible: Changes are purely internal - no API changes
4. Security: Existing valid_procfile_path? validation remains intact
5. Well Commented: The new inline comments clearly explain the tri-state logic

⚠️ Critical Issue: Test Coverage Gap

The existing tests do not cover the new tri-state logic. Specifically:

Lines 73-86 in process_manager_spec.rb:

it "uses foreman when overmind not available and foreman is available" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false)  # ❌ Should be nil
  # ...
end

it "exits with error when no process manager available" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false)  # ❌ Should be nil
  # ...
end

These tests now have incorrect semantics:

• They mock false (process found but failed) when they should mock nil (process not found)
• This means the actual new behavior (exit silently when false) is NOT tested

Missing test case:

it "exits silently when overmind is installed but exits with error" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false)
  expect(described_class).not_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")
end

📋 Recommended Test Updates

# Fix existing tests to use correct return values
it "uses foreman when overmind not available and foreman is available" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(nil)  # ✅ Not found
  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
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(nil)  # ✅ Not found
  expect(described_class).to receive(:run_process_if_available)
    .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(nil)   # ✅ Not found
  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")
end

# Add new test case for the actual bug fix
it "exits silently when overmind is installed but exits with error" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false)  # ✅ Found but failed
  expect(described_class).not_to receive(:run_process_if_available).with("foreman", anything)
  expect(described_class).not_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")
end

it "tries foreman when overmind not found, exits silently when foreman fails" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(nil)   # ✅ Not found
  expect(described_class).to receive(:run_process_if_available)
    .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(false)  # ✅ Found but failed
  expect(described_class).not_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")
end

💡 Minor Improvements (Optional)

1. Lines 120-126 in process_manager_spec.rb: The test returns false when process not available anywhere should expect nil instead of false:

   result = described_class.send(:run_process_if_available, "nonexistent", ["start"])
   expect(result).to be_nil  # Not be false

2. Code clarity: The condition on line 50 could be more explicit:

   # Current
   exit 1 if overmind_result == false || foreman_result == false
   
   # Suggestion (more explicit about tri-state)
   exit 1 if [overmind_result, foreman_result].any? { |r| r == false }

   Though the current version is perfectly fine.

🎯 Action Items

Before merging:

1. ✅ Update existing tests to use nil for "not found" scenarios (lines 73-90)
2. ✅ Add test case for the actual bug: overmind installed but exits with error
3. ✅ Add test case for: overmind not found, foreman found but exits with error
4. ✅ Update line 120-126 test to expect nil instead of false
5. ✅ Run: rake run_rspec:gem to verify all tests pass

📊 Overall Assessment

Quality: ⭐⭐⭐⭐ (4/5) - Excellent implementation with one critical test gap

Risk: 🟡 Low-Medium - Logic is correct but untested behavior is risky

Recommendation: Request changes - Fix test coverage before merging to ensure the fix actually works as intended and prevent regressions.

---

Great work on the bug fix! The code changes are spot-on. Just need to ensure the tests properly validate the new behavior. 🚀

[claude]

Code Review

✅ Overall Assessment

This is a well-crafted bug fix that addresses a legitimate UX issue. The solution is clean, well-documented, and properly tested.

---

🎯 Strengths

1. Clear Problem Identification: The PR correctly identifies that the previous code couldn't distinguish between "process not found" vs "process found but failed."

2. Elegant Three-State Solution: Using true/false/nil to represent three distinct states is idiomatic Ruby:

   • true = success
   • false = found but failed
   • nil = not found

3. Type Safety: The RBS signature was properly updated to reflect the new return type: (bool | nil).

4. Comprehensive Documentation: In-line comments clearly explain the three-state return values in both the method definition and the calling code.

5. Security Maintained: No changes to the valid_procfile_path? security validation - good!

---

⚠️ Critical Issue: Test Coverage Gap

The existing tests do NOT cover the bug that was fixed.

Looking at spec/react_on_rails/dev/process_manager_spec.rb:

Lines 73-79 test the case where overmind returns false and then foreman returns true:

it "uses foreman when overmind not available and foreman is available" do
  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)

The problem: This test expects the OLD behavior where run_process_if_available returns false when not available. But after your changes, it should return nil when not available.

Lines 82-91 have the same issue - they test both returning false, but should test both returning nil.

Lines 120-126 correctly test returning false when process isn't available, but this should now be nil.

---

📝 Required Test Updates

You need to update the tests to reflect the new three-state behavior:

1. Test the actual bug scenario:

it "exits with error when overmind is found but exits with error, without showing 'not found' message" do
  expect(described_class).to receive(:run_process_if_available)
    .with("overmind", ["start", "-f", "Procfile.dev"]).and_return(false)  # Found but failed
  expect(described_class).to receive(:run_process_if_available)
    .with("foreman", ["start", "-f", "Procfile.dev"]).and_return(nil)  # Not found
  
  # Should exit WITHOUT showing installation help
  expect(described_class).not_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")
end

2. Update existing tests to use nil for "not found":

Line 75: Change .and_return(false) to .and_return(nil)

Lines 83-86: Change both .and_return(false) to .and_return(nil)

Line 124: Change expect(result).to be false to expect(result).to be_nil

3. Add test for "found but failed" returning false:

it "returns false when process is found but exits with error" 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").and_return(false)  # Exit code non-zero
  
  result = described_class.send(:run_process_if_available, "overmind", ["start", "-f", "Procfile.dev"])
  expect(result).to be false  # Not nil, not true - explicitly false
end

---

🔍 Edge Case Considerations

1. What if both overmind AND foreman fail?

   • Current code: exits with code 1 (correct)
   • Should add a test for: overmind=false, foreman=false

2. What if overmind succeeds with exit code 0?

   • Current code: returns immediately (correct)
   • Existing test on line 66-70 covers this ✅

3. What if first process fails, second succeeds?

   • Current code: tries second, returns if successful (correct)
   • Need to update line 73-79 test to use nil instead of false ⚠️

---

📊 Code Quality

• ✅ Follows Ruby style conventions
• ✅ Comments are clear and helpful
• ✅ RuboCop passes (per PR description)
• ✅ RBS signatures updated
• ✅ Security validation untouched
• ✅ No backwards-compatibility hacks

---

🎬 Recommendation

Do not merge until test coverage is updated. The fix is correct, but the tests are asserting the OLD behavior, which means:

1. They're currently passing when they should be failing
2. They won't catch regressions if someone reverts this logic

Action items:

1. Update existing tests to use nil for "not found" cases
2. Add new test for the actual bug: overmind found but fails, foreman not installed
3. Add test for both process managers found but both fail
4. Run rake run_rspec:gem to ensure all tests pass

---

💡 Minor Suggestion: Documentation

Consider adding a brief note in the method's YARD docs:

# Try to run a process if it's available, with intelligent fallback strategy
#
# @param process [String] Name of the process to run
# @param args [Array<String>] Arguments to pass to the process
# @return [Boolean, nil] true if successful, false if found but failed, nil if not found
# @example
#   run_process_if_available("overmind", ["start", "-f", "Procfile"])
def run_process_if_available(process, args)

---

Great work identifying and fixing this issue! The logic is sound - just needs the test coverage to match. 🚀

[claude]

Code Review for PR #2087

Summary

This PR fixes a legitimate bug where bin/dev incorrectly showed "Process Manager Not Found" when overmind was installed but exited with a non-zero status. The fix is well-designed and thoroughly tested.

---

✅ Strengths

1. Excellent Problem Analysis

The PR description clearly explains the root cause: Ruby's system() returns false for non-zero exit codes and nil for process-not-found errors, but the old code couldn't distinguish between them.

2. Clean Three-State Return Value Design

# Returns:
#   - true if process ran and exited successfully (exit code 0)
#   - false if process ran but exited with error (non-zero exit code)
#   - nil if process was not found/available

This is idiomatic Ruby and makes the logic in run_with_process_manager crystal clear.

3. Comprehensive Test Coverage

The tests now cover all important scenarios:

• ✅ Overmind success (line 66-70)
• ✅ Overmind not found, foreman success (line 73-79)
• ✅ Overmind fails, foreman not found (line 82-91)
• ✅ Overmind fails, foreman succeeds (line 93-102)
• ✅ Both fail (line 104-113)
• ✅ Overmind not found, foreman fails (line 115-124)
• ✅ Neither found (line 126-135)

4. Correct RBS Type Signature Update

sig/react_on_rails/dev/process_manager.rbs:14 correctly changed from -> bool to -> (bool | nil).

5. Clear Comments and Documentation

The inline comments in run_with_process_manager (lines 39-41, 48-49) make the logic easy to follow.

---

🔍 Code Quality Observations

1. Logic Correctness ✅

The control flow in run_with_process_manager (lines 42-54) is correct:

overmind_result = run_process_if_available("overmind", ["start", "-f", procfile])
return if overmind_result == true  # Success path

foreman_result = run_process_if_available("foreman", ["start", "-f", procfile])
return if foreman_result == true  # Success path

# Exit silently if found but failed (they showed their own error)
exit 1 if overmind_result == false || foreman_result == false

# Only show "not found" if NEITHER was found
show_process_manager_installation_help
exit 1

This correctly handles all cases without showing misleading errors.

2. Test Quality ✅

• Tests use proper mocking to isolate behavior
• Tests verify that show_process_manager_installation_help is NOT called when it shouldn't be
• Test descriptions clearly explain the scenario being tested

3. Security ✅

No security concerns. The existing valid_procfile_path? check already prevents shell injection.

4. Performance ✅

No performance impact. The change only affects control flow logic.

---

📝 Minor Suggestions (Optional)

1. Consider Explicit nil? Check

The current code uses == true and == false, which works but could be slightly more explicit:

# Current (works fine)
return if overmind_result == true

# Alternative (more explicit about three states)
return if overmind_result.equal?(true)
exit 1 if overmind_result.equal?(false)

However, the current approach is perfectly fine and arguably more readable.

2. Test Naming

Line 93: "exits silently when overmind fails but foreman succeeds" - this test name suggests it exits, but it actually succeeds (foreman runs successfully). Consider:

it "succeeds with foreman when overmind fails but foreman succeeds" do

But this is nitpicky - the test logic is correct (line 99 confirms no exit is called).

---

🎯 Verdict

Approved ✅

This is high-quality code that:

• Fixes a real user-facing bug
• Uses clean, idiomatic Ruby patterns
• Has excellent test coverage
• Maintains type safety with updated RBS signatures
• Follows the project's coding standards

No blocking issues. The minor suggestions above are entirely optional.

---

📋 Checklist Verification

Per CLAUDE.md requirements:

• ✅ RuboCop compliance (assumed based on PR description)
• ✅ RBS signatures updated and validated
• ✅ Comprehensive test coverage
• ✅ Clear documentation in code comments
• ✅ Follows Ruby best practices

Great work! 🚀