No actionable comments were generated in the recent review. 🎉

Walkthrough

Capture and pass selected runtime ENV keys (PORT, SHAKAPACKER_DEV_SERVER_PORT) when launching processes outside Bundler by adding an ENV_KEYS_TO_PRESERVE constant and a helper that builds an env hash, then invoking system(env_overrides, process, *args) so those vars survive Bundler's environment restoration.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hop through envs with careful art,
I pluck the PORT and tuck it in my cart,
SHAKAPACKER's number snug and warm,
Bundler's gusts won't break their charm,
Off goes the process — steady, smart!

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 @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR fixes a well-understood Bundler behaviour where with_unbundled_env restores a pre-Bundler.setup snapshot of ENV, silently discarding runtime vars like PORT and SHAKAPACKER_DEV_SERVER_PORT that PortSelector sets after that snapshot is taken. The fix — capturing those vars before entering the block and forwarding them as an explicit env hash to system() — is the canonical solution used by Rails railties, Spring, and the codebase's own PackGenerator.

Key changes:

• Adds ENV_KEYS_TO_PRESERVE = %w[PORT SHAKAPACKER_DEV_SERVER_PORT].freeze constant inside the singleton class.
• Adds private helper preserve_runtime_env_vars that builds a hash of only the vars that are actually set.
• run_process_outside_bundle now calls system(env_overrides, process, *args) instead of system(process, *args).

Issues found:

• The two unit tests for run_process_outside_bundle were not updated for the new system() signature and will fail — system({}, "foreman", ...) does not match the existing with("foreman", ...) expectations.
• The RBS type signature file is missing the new preserve_runtime_env_vars method and ENV_KEYS_TO_PRESERVE constant declarations.

Confidence Score: 4/5

• Safe to merge after fixing the two broken unit tests; the production code change itself is correct and well-motivated.
• The implementation is sound and follows established patterns in both Rails and this codebase. The only blocker is that the existing run_process_outside_bundle specs were not updated to match the new system(env_hash, ...) call signature and will fail in CI. Once those two test expectations (and ideally a new positive test for env preservation) are updated, the PR is ready to merge.
• react_on_rails/spec/react_on_rails/dev/process_manager_spec.rb — two examples in .run_process_outside_bundle need updated system argument matchers.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant PS as PortSelector
    participant ENV as ENV (runtime)
    participant PM as ProcessManager
    participant BE as Bundler.with_unbundled_env
    participant FM as Foreman/Overmind

    PS->>ENV: ENV["PORT"] = "3001"
    PS->>ENV: ENV["SHAKAPACKER_DEV_SERVER_PORT"] = "3036"

    Note over PM: preserve_runtime_env_vars()
    PM->>ENV: read PORT, SHAKAPACKER_DEV_SERVER_PORT
    ENV-->>PM: { "PORT" => "3001", "SHAKAPACKER_DEV_SERVER_PORT" => "3036" }

    PM->>BE: with_unbundled_env { ... }
    Note over BE: Restores pre-Bundler ENV snapshot<br/>(PORT / SHAKAPACKER_DEV_SERVER_PORT now nil)

    BE->>FM: system(env_overrides, "foreman", *args)
    Note over FM: env_overrides re-injects PORT=3001<br/>SHAKAPACKER_DEV_SERVER_PORT=3036
    FM-->>PM: exit status

_{Reviews (1): Last reviewed commit: "fix: preserve runtime env vars across Bu..." | Re-trigger Greptile}

Review

The fix correctly addresses the root cause — Bundler.with_unbundled_env restoring the pre-Bundler ENV snapshot — and the approach of capturing vars before the block and passing them via system(env, ...) is the right pattern.

A few issues to address:

Broken existing test

The existing test at spec line 176 will now fail. Before this change, system was called as system(process, *args). After, it is called as system(env_overrides, process, *args) where env_overrides is {} when the port vars are absent. An empty hash as the first arg is a different method signature, so the expectation .with('foreman', 'start', ...) won't match .with({}, 'foreman', 'start', ...).

Constant defined on the singleton class

ENV_KEYS_TO_PRESERVE is defined inside class << self, which attaches it to the singleton class, not ProcessManager itself. Referencing it as ProcessManager::ENV_KEYS_TO_PRESERVE from outside the class will raise NameError. It should be moved outside class << self, alongside VERSION_CHECK_TIMEOUT at the top of the class.

Missing test coverage for the new behaviour

There are no tests that verify env vars are actually forwarded when set. Worth adding tests for: when PORT is set it should appear in the env hash passed to system; when SHAKAPACKER_DEV_SERVER_PORT is set the same; and when neither is set the hash is empty.

The core fix is correct and follows the established pattern (Rails railties, PackGenerator). Three issues need addressing before merge: (1) ENV_KEYS_TO_PRESERVE is inside class << self, making ProcessManager::ENV_KEYS_TO_PRESERVE raise NameError — move it outside alongside VERSION_CHECK_TIMEOUT. (2) The spec update uses a_kind_of(Hash) which does not verify the hash contents — add a test that sets ENV PORT and asserts it appears in the system() call. (3) react_on_rails/sig/react_on_rails/dev/process_manager.rbs is missing ENV_KEYS_TO_PRESERVE and preserve_runtime_env_vars declarations — steep check will flag them. See inline comments for details.

Review

Overall: solid fix, minimal and well-reasoned. The root cause analysis is accurate — Bundler.with_unbundled_env restores the pre-Bundler env snapshot, and the fix correctly captures the affected vars before entering the block and passes them explicitly to system() via Ruby's Kernel#system(env, cmd, *args) signature.

What's right

• Capturing env_overrides before with_unbundled_context (not inside it) ensures the values read from ENV are the current runtime values, not the restored pre-Bundler snapshot. The ordering is correct.
• The else branch (Bundler not defined) correctly skips env preservation — with_unbundled_env was never called in that path, so env vars are already in scope.
• process_available_in_system? does not need the same treatment since it only does a version check — env vars aren't relevant there.
• RBS signatures are updated in sync with the implementation change.
• The pattern matches Rails railties, Spring, and this codebase's own PackGenerator — consistent precedent.

Minor concerns

1. Hardcoded ENV_KEYS_TO_PRESERVE list (inline comment on line 17): The list is correct for today, but if PortSelector or Shakapacker adds new runtime env vars in the future they'll silently fail. A note in the constant comment about this being the extension point would help future maintainers.

2. Missing "both nil" test case (inline comment on spec line 204): The three test cases cover "both set", "one missing", and the generic "is a Hash" shape, but not "both nil → empty hash {}". system({}, cmd, *args) is valid Ruby but verifying this edge case explicitly would complete the contract.

Neither of these is a blocker. The fix is correct and the tests cover the meaningful cases.