Walkthrough

Adds deterministic base-port support via REACT_ON_RAILS_BASE_PORT (fallback CONDUCTOR_PORT), deriving Rails/webpack/renderer at offsets +0/+1/+2; surfaces renderer in port selection; updates Procfiles, templates, docs, process/env preservation, and specs to prefer environment-driven port values with shell-default expansion.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Dev as bin/dev
    participant PS as ReactOnRails::Dev::PortSelector
    participant SM as ReactOnRails::Dev::ServerManager
    participant Svc as Services (Rails, Webpack, Node-Renderer)

    Dev->>PS: select_ports()
    alt REACT_ON_RAILS_BASE_PORT detected
        PS->>PS: validate base_port (1..MAX_BASE_PORT)
        PS->>PS: derive rails=base+0, webpack=base+1, renderer=base+2
        PS-->>Dev: {rails, webpack, renderer}
    else CONDUCTOR_PORT detected
        PS->>PS: parse conductor as base
        PS->>PS: derive rails=base+0, webpack=base+1, renderer=base+2
        PS-->>Dev: {rails, webpack, renderer}
    else
        PS->>PS: use explicit envs or probe free ports
        PS-->>Dev: {rails, webpack, nil}
    end

    Dev->>SM: configure_ports(selected)
    alt selected.renderer present
        SM->>SM: set ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"] (overwrite)
        SM->>SM: set ENV["RENDERER_PORT"], ENV["REACT_RENDERER_URL"]
        SM-->>Svc: processes inherit envs
    else
        SM->>SM: apply explicit/env-default PORT and dev-server using ||= 
        SM->>SM: set REACT_RENDERER_URL only if RENDERER_PORT present and URL unset
        SM-->>Svc: processes inherit envs (no renderer)
    end

    Svc->>Svc: bind services to assigned ports

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix: use env-var-driven ports in Procfile templates to support multiple worktrees #2539 — Modifies the same port-selection and ServerManager code paths and Procfile/.env templates; closely related.
• Improve create-react-on-rails-app fresh app onboarding #2849 — Overlaps changes in ServerManager and bin/dev startup/env handling.
• 🚀 Enhance bin/dev with configurable routes and improved production asset handling #1790 — Related edits to dev server tooling and ServerManager behavior.

Suggested labels

codex

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci2

Poem

🐰 I found a base port in the glen,
Rails, Webpack, Renderer — lined by ten,
Offsets hopped and ports took their place,
ENV whispers tied each little space,
I thump a drum — dev servers race 🥕

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.}

Code Review

Overview

This PR adds a REACT_ON_RAILS_BASE_PORT env var (with a CONDUCTOR_PORT fallback) that lets coding-agent tools assign all service ports from a single value — Rails = base+0, webpack = base+1, Pro renderer = base+2. All Procfiles are standardized to use ${PORT:-N} patterns and generator output is updated to match. The design is clean and the scope is well-contained.

What Works Well

• Priority chain is sensible — base port > explicit per-service vars > auto-detect. The BASE_PORT_ENV_VARS list makes it easy to add new tool-specific vars later.
• No probing when base port is set — deterministic and fast, which is exactly what CI/agent environments need.
• Test coverage is solid — all priority tiers, edge cases (out-of-range base, both vars set), and the renderer-nil path are covered. The around blocks cleanly save/restore env state.
• Procfile standardization is a good cleanup — ${PORT:-3000} should have been the pattern all along; this PR makes it consistent across OSS and Pro fixtures.
• configure_ports uses ||= correctly — won't stomp on env vars a user already set intentionally.

Issues

Bug (medium) — derived ports can overflow 65535.
base_port validates the raw env var is in 1..65535 but doesn't check that base + 2 (the renderer offset) also stays in range. REACT_ON_RAILS_BASE_PORT=65534 would produce a renderer port of 65536, which is invalid. The fix is to cap the valid base range at 65535 - BASE_PORT_RENDERER_OFFSET. See inline comment.

Misleading log when PORT is already in the environment.
select_ports prints "Base port 4000 detected. Using Rails :4000, webpack :4001, renderer :4002" unconditionally when a base port is found, but configure_ports uses ||= — so a pre-existing PORT=3000 would still be honoured and Rails would actually run on 3000. The log then contradicts reality. Harmless in the primary coding-agent use case (where PORT wouldn't be pre-set), but worth tightening. See inline comment.

Minor — DEFAULT_RENDERER_PORT constant is unused.
It's defined at the top of PortSelector but never referenced in code. The Procfiles and generator both hardcode 3800 inline. Either use the constant or remove it. See inline comment.

Nice-to-have — REACT_RENDERER_URL not auto-derived from explicit RENDERER_PORT.
Manual-worktree users who set RENDERER_PORT=3801 also have to remember to set REACT_RENDERER_URL=http://localhost:3801. A one-liner in configure_ports could derive it automatically. The docs cover it, but easy to miss. See inline comment (can defer to follow-up).

Test Coverage Gap

There's no test for the scenario where REACT_ON_RAILS_BASE_PORT is set and PORT is already in the environment. Given the misleading-log issue above, a test that asserts configure_ports honours the pre-existing PORT (or alternatively overrides it — whichever the intended behavior is) would prevent regressions once the behavior is clarified.

Docs

The new process-managers.md section and .env.example comments are clear and accurate. The offset table is a nice touch. One small nit: the "Priority chain" callout in the docs says "base port > explicit per-service env vars" — worth noting that this is the select_ports priority, not the final configure_ports priority (which still lets pre-existing env vars win via ||=).

Summary

The approach is sound and the feature is useful. The port-overflow bug is the one concrete correctness issue to fix before merging. The log/priority conflict is worth at least a decision: either document that base port always wins (and change ||= to = in configure_ports) or document that pre-existing env vars take final precedence (and fix the log). Everything else can be a follow-up.

🤖 Generated with Claude Code

Greptile Summary

Adds REACT_ON_RAILS_BASE_PORT (with CONDUCTOR_PORT as a fallback) to derive all dev service ports from a single base value, enabling concurrent worktree usage without manual per-service config. Procfiles are updated to shell-default patterns (${PORT:-3000}), renderer env vars are propagated through the process manager, and docs/.env.example are expanded with coding-agent guidance.

Prior review concerns — derived renderer port exceeding 65535 and test-environment isolation — have been addressed. Two minor points remain:

• base_port warns on non-integer values but silently ignores out-of-range integers; a consistent warning would help diagnose near-max typos.
• sync_renderer_port_and_url uses String#include? for the port-in-URL check, which can miss a mismatch when the renderer port number is a numeric prefix of the URL's actual port (e.g. 3800 vs. :38000).

Confidence Score: 5/5

Safe to merge — no blocking issues; remaining findings are minor UX/edge-case P2s.

Previous P0/P1 concerns (renderer port > 65535, test env isolation) have been fully addressed. The two remaining issues are P2: a missing warning for out-of-range base ports (silent fallback is safe) and a false-negative in the port-prefix substring check (only affects the warning, not actual behavior). Test coverage is thorough and the core port-selection logic is correct.

react_on_rails/lib/react_on_rails/dev/port_selector.rb and react_on_rails/lib/react_on_rails/dev/server_manager.rb have the minor findings noted above.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([bin/dev start]) --> B{REACT_ON_RAILS_BASE_PORT or CONDUCTOR_PORT set?}
    B -->|Yes - first valid var wins| C[base_port returns Integer]
    C --> D[select_ports: derive rails=base+0, webpack=base+1, renderer=base+2]
    D --> E[apply_base_port_env: unconditionally set PORT, SHAKAPACKER_DEV_SERVER_PORT, RENDERER_PORT, REACT_RENDERER_URL]
    B -->|No| F{PORT or SHAKAPACKER_DEV_SERVER_PORT set?}
    F -->|Yes| G[Use explicit per-service vars]
    F -->|No| H[Auto-detect free ports starting from defaults]
    G --> I[apply_explicit_port_env: PORT and WEBPACK assigned conditionally]
    H --> I
    I --> J{RENDERER_PORT set?}
    J -->|No| K([Done - renderer not configured])
    J -->|Yes| L{REACT_RENDERER_URL set?}
    L -->|No| M[Derive URL from RENDERER_PORT]
    L -->|Yes - port matches| N([Done - consistent])
    L -->|Yes - port mismatch| O[Warn: vars disagree]
    E --> P([ENV_KEYS_TO_PRESERVE propagated to spawned process manager])
    M --> P
    N --> P
    O --> P

_{Reviews (3): Last reviewed commit: "fix: second round of port selector revie..." | Re-trigger Greptile}

Code Review

Overall this is a clean, well-motivated feature. The implementation is straightforward and the test coverage is solid. I found a few issues worth addressing before merge.

Bug: base-port priority not enforced in configure_ports

The documented priority chain states "base port > explicit per-service env vars", but the implementation doesn't honour this. In server_manager.rb:

ENV["PORT"] ||= selected[:rails].to_s

If PORT is already set in the environment (e.g. by a CI system or a shell export), ||= is a no-op. select_ports still returns the base-derived port in selected[:rails], but the Procfile reads ${PORT:-3000} — so Rails actually starts on the pre-existing PORT value, silently disagreeing with the log line that says "Using Rails :<base+0>". Either:

• Override explicitly when a base port is detected: ENV["PORT"] = selected[:rails].to_s (only in the base-port branch), or
• Downgrade the documented priority to say base port only wins when PORT is not already set.

Bug: derived ports are not range-validated

base_port validates that the base itself is in 1..65_535, but the derived ports are not checked. A base port of 65_534 would produce renderer port 65_536, which is invalid and would cause a confusing runtime error rather than a clear message.

Dead code: DEFAULT_RENDERER_PORT = 3800

This constant is defined but never referenced anywhere in the Ruby source — the renderer default of 3800 lives only in the Procfile shell fallback (${RENDERER_PORT:-3800}). Either use the constant in code or remove it to avoid misleading future readers.

Missing test: base port takes priority over pre-set PORT

There is no test covering the interaction where REACT_ON_RAILS_BASE_PORT is set AND PORT is already set. This gap would have caught the ||= priority issue above.

Minor: CONDUCTOR_PORT assumption needs verification

The PR assumes CONDUCTOR_PORT is the workspace's allocated base port. If Conductor.build uses that variable for its own internal API rather than the app's port, silently treating it as a Rails base port could cause surprising conflicts. Worth a note in the docs or a code comment citing the Conductor.build docs page that confirms this.

None of these are blockers for the feature concept, but the ||= priority bug and the overflow bug could cause hard-to-diagnose issues in the exact concurrent-workspace scenario this PR is designed to solve.

Review Feedback Addressed (commit b6e16df)

I (Claude Code) went through all inline review comments from @claude, @cursor, @chatgpt-codex-connector, @coderabbitai, and @greptile and addressed every real issue raised. After triage (deduplicating overlapping comments across reviewers), six distinct concerns were identified and all six are now fixed in a single commit.

Fixes

1. Base port overflow (P1/Major from multiple reviewers) — PortSelector.base_port now validates against MAX_BASE_PORT (65_535 - BASE_PORT_RENDERER_OFFSET). A base of 65_534 previously passed the 1..65_535 check but produced an invalid renderer port of 65_536.
2. Base port priority violated by ||= (cursor High, claude) — ServerManager.configure_ports now splits the base-port and explicit-port paths. When a base port is active, PORT / SHAKAPACKER_DEV_SERVER_PORT / RENDERER_PORT / REACT_RENDERER_URL are assigned unconditionally so the effective ports match the Base port … detected log line even when those env vars were pre-set (e.g., from a .env file).
3. RENDERER_PORT / REACT_RENDERER_URL lost across Bundler.with_unbundled_env (codex P1) — Added both keys to ProcessManager::ENV_KEYS_TO_PRESERVE so they survive run_process_outside_bundle.
4. Missing test for configure_ports when PORT is pre-set with base port active (claude) — Added a when configuring ports with a base port active context to server_manager_spec that covers the override.
5. Unused DEFAULT_RENDERER_PORT constant (claude ×2, cursor Low) — Removed. The 3800 fallback only lives in the Procfile shell expression ${RENDERER_PORT:-3800}.
6. REACT_RENDERER_URL can drift from RENDERER_PORT (coderabbit Major, claude) — In the non-base-port path, configure_ports now derives REACT_RENDERER_URL from an explicit RENDERER_PORT when only the port is set. Added a regression spec.

Tests

Added 5 new specs to port_selector_spec.rb and server_manager_spec.rb covering the max-safe base port, overflow fallback, base-port override of pre-existing env vars, and explicit-RENDERER_PORT URL derivation. Full suite (176 examples) passes locally.

On the bot summary comments (@claude, @greptile, @coderabbitai)

Their summary bodies restate the inline findings that are already addressed above — no separate action needed. The earlier CodeRabbit "processing" notice is a stale status post.

All 16 unresolved review threads are now resolved.

To use Codex here, create an environment for this repo.

#!/bin/bash
echo "=== PortSelector: base_port method + MAX_BASE_PORT ==="
rg -n "MAX_BASE_PORT|base_port|BASE_PORT_RENDERER_OFFSET" react_on_rails/lib/react_on_rails/dev/port_selector.rb -A2 -B2

echo ""
echo "=== ServerManager: configure_ports ==="
cat react_on_rails/lib/react_on_rails/dev/server_manager.rb

echo ""
echo "=== ProcessManager: ENV_KEYS_TO_PRESERVE ==="
rg -n "ENV_KEYS_TO_PRESERVE" -A5 -B2

echo ""
echo "=== DEFAULT_RENDERER_PORT removed? ==="
rg -n "DEFAULT_RENDERER_PORT" react_on_rails/lib/react_on_rails/dev/port_selector.rb

@justin808 — Thank you for the thorough write-up! I've verified all six fixes in the updated code:

1. MAX_BASE_PORT overflow guard ✅ — base_port now validates against MAX_BASE_PORT = 65_535 - BASE_PORT_RENDERER_OFFSET, correctly rejecting values like 65,534 that would produce an out-of-range renderer port.

2. Unconditional base-port assignment ✅ — configure_ports is cleanly split into apply_base_port_env (assigns all four env vars unconditionally) and apply_explicit_port_env (uses ||=), so the "Base port detected" log line always matches the effective ports.

3. ENV_KEYS_TO_PRESERVE ✅ — Both RENDERER_PORT and REACT_RENDERER_URL are present in ProcessManager::ENV_KEYS_TO_PRESERVE, ensuring they survive Bundler.with_unbundled_env.

4. DEFAULT_RENDERER_PORT removed ✅ — No matches found; the 3800 fallback now lives exclusively in the Procfile shell expression.

5. REACT_RENDERER_URL/RENDERER_PORT drift (my Major comment) ✅ — apply_explicit_port_env now derives REACT_RENDERER_URL from an explicit RENDERER_PORT when the URL is unset.

Everything looks solid. The apply_base_port_env / apply_explicit_port_env split is a nice clean separation of concerns too. Great work resolving all of the feedback!

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 0
File: :0-0
Timestamp: 2026-04-13T02:29:20.920Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/engine.rb` and `react_on_rails/lib/react_on_rails/configuration.rb` (PR `#3094`): Do NOT reset `rendering_strategy` or `js_code_builder` inside `ReactOnRails.configure`. The Pro engine sets these exactly once during `after_initialize`; resetting them on reconfigure would silently drop Pro's NodeStrategy/ProJsCodeBuilder assignments. A proper reconfigure mechanism for these accessors is deferred to Step 3 of the strategy-pattern refactor (`#2905`).

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2284
File: react_on_rails/lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt:10-10
Timestamp: 2026-02-12T20:43:26.843Z
Learning: In React on Rails Pro initializer templates (e.g., `react_on_rails_pro.rb.tt`), the `config.renderer_password` uses `ENV.fetch("RENDERER_PASSWORD", "devPassword")` as a development starter configuration. This pattern is intentional: the node renderer requires explicit production deployment setup, and adding a production-only raise would break generator execution since initializers are loaded during `rails generate`. The pattern is consistent with existing manual setup documentation.

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/commonWebpackConfig.js : Changes to `commonWebpackConfig.js` affect all three webpack bundles (client, server, and RSC) in the Pro dummy app

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T23:15:11.806Z
Learning: Applies to CHANGELOG.md : Changelog format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` (no hash before PR number)

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,tsx,jsx,json,css,md} : Pro Prettier check should be run with: `cd react_on_rails_pro && pnpm run prettier --check .`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/react_on_rails_pro/**/*.rb : Pro gem unit tests should be run with: `cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/spec/**/*.rb : Pro integration tests should be run with: `cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T22:10:33.821Z
Learning: Applies to react_on_rails_pro/** : Ask before modifying the Pro package (`react_on_rails_pro/`)

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/packages/react-on-rails-pro/**/*.test.rsc.{ts,tsx} : Pro RSC tests should be run with: `pnpm --filter react-on-rails-pro run test:rsc`

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T23:15:11.806Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.{js,ts} : Use Playwright for E2E tests in `react_on_rails/spec/dummy/e2e/playwright/e2e/`

Code Review: PR #3142

Overview

Well-structured PR solving a real pain point (port conflicts across concurrent worktrees/agent sandboxes) with a clean priority-chain design. Test coverage is thorough and inline documentation is unusually good. One clear bug found where a test documents behavior that the implementation does not actually provide.

Bug: configured_renderer_port_for_kill does not extract port from a localhost REACT_RENDERER_URL

The newly added test "targets the local renderer URL port when RENDERER_PORT is not set" expects that when only REACT_RENDERER_URL=http://localhost:3900 is set (no RENDERER_PORT), kill_processes targets port 3900. The implementation does not do this.

Trace through configured_renderer_port_for_kill:

1. RENDERER_PORT is unset → valid_port_string? returns false → skip
2. remote_renderer_url_configured? returns false because http://localhost:3900 IS a localhost URL
3. Falls through to the hardcoded 3800 default
4. killable_ports builds [3000, 3001, 3800] — port 3900 is never scanned

The test asserts both expect(Open3).not_to receive(:capture2).with("lsof", "-ti", ":3800", ...) and expect(Process).to receive(:kill).with("TERM", 3901), both of which would fail.

Suggested fix — add a URL port extraction step before the fallback:

def configured_renderer_port_for_kill
  raw_port = ENV.fetch("RENDERER_PORT", nil)
  return raw_port.strip.to_i if valid_port_string?(raw_port)
  return nil if remote_renderer_url_configured?

  local_url = ENV.fetch("REACT_RENDERER_URL", nil) || ENV.fetch("RENDERER_URL", nil)
  if local_url && !local_url.strip.empty? && localhost_renderer_url?(local_url)
    port = URI.parse(local_url).port rescue nil
    return port if port&.between?(1, 65_535)
  end

  3800
end

Minor: ENV mutation in select_ports without a ! suffix

PortSelector.select_ports deletes invalid PORT/SHAKAPACKER_DEV_SERVER_PORT values from ENV via read_and_sanitize_port_env!. The docstring calls this out clearly, but a public class method mutating a global singleton without a ! suffix is still a surprise. Low severity for a dev-only tool.

Minor: SHAKAPACKER_DEV_SERVER_PORT set in prod mode

apply_base_port_env sets SHAKAPACKER_DEV_SERVER_PORT even in bin/dev prod, which does not run webpack-dev-server. The comment explains this is intentional for tooling consistency and tests lock it in. Valid design choice, but could confuse developers who inspect env vars after bin/dev prod.

What is done well

• Test depth: edge cases (IPv6 URLs, basic-auth passwords with digit substrings, whitespace-padded env vars, port-as-substring) reflect production incident thinking.
• No shell injection: all lsof/pgrep/Open3 calls use word-list argv throughout.
• socket_active? with bounded timeout: connect_nonblock + SO_ERROR probe avoids stalling on a stuck Overmind socket; wait_writable is correct for Fiber scheduler compatibility.
• Proc/lsof dual-probe in working_directory_for_pid: fallback from /proc/PID/cwd to lsof covers both Alpine containers (no lsof) and macOS (no /proc).
• URL_WITH_EXPLICIT_PORT_RE regex: userinfo prefix guard and IPv6 literal alternative correctly handle the basic-auth and IPv6 edge cases called out in the tests.
• valid_port_string? as shared predicate: keeps ServerManager and PortSelector from silently diverging on port string validation.
• base_port_hash vs base_port_ports: clean separation between the silent kill path and the startup path that logs the detected base port.
• CONDUCTOR_PORT caveat: best-effort disclaimer placed in both code comments and the docs.

CI is unchecked. The failing test will surface the bug automatically once CI runs.

Code Review: PR #3142 — REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

Overview

Clean, well-scoped feature. The design decisions are sound — the base_port_ports (verbose) vs base_port_hash (silent) split cleanly separates startup logging from kill-flow queries, ENV mutation is clearly documented with ! conventions, and the hedge on CONDUCTOR_PORT's undocumented contract is the right call.

Test coverage is excellent: 300+ new lines covering base-port derivation, priority chain, privileged ports, overflow, whitespace, IPv6, basic-auth URLs, and kill-process targeting. The with clean port env shared context protecting specs from shell leakage is a nice touch.

Issues

1. warn message missing WARNING: prefix (inconsistency)

In sync_renderer_port_and_url (server_manager.rb:1124), the derivation message is the only ENV-mutation warn line in the file without a WARNING: prefix:

warn "RENDERER_PORT=#{port} set without REACT_RENDERER_URL; deriving REACT_RENDERER_URL=#{derived}."

Every other ENV-mutation message starts with WARNING:. This breaks grep WARNING bin/dev.log filtering and is surprising for operators scanning output. See inline comment.

2. Whitespace-padded PORT / SHAKAPACKER_DEV_SERVER_PORT not normalized in ENV (explicit-port path)

valid_port_string? accepts " 3000 " as valid, so overwrite_invalid_port_env returns early and leaves the whitespace-padded value in ENV. The Procfile then receives ${PORT:-3000} → 3000 (with spaces), passed to rails s -p 3000 . Most shells handle this silently, but it's inconsistent with RENDERER_PORT, which is stripped before being written back:

# sync_renderer_port_and_url — RENDERER_PORT IS normalized
ENV["RENDERER_PORT"] = port if port != raw_port

Fix: add ENV[var_name] = existing.strip when the value is valid but contains surrounding whitespace. See inline comment.

3. pro_renderer_active? activates on the legacy RENDERER_URL

%w[RENDERER_PORT REACT_RENDERER_URL RENDERER_URL].any? { ... }

A user who still has RENDERER_URL in their env gets silently pulled into the Pro renderer path — including having REACT_RENDERER_URL derived or overridden in base-port mode. This is probably correct, but the existing rename warning ("RENDERER_URL is set but REACT_RENDERER_URL is not") doesn't mention that RENDERER_URL still activates Pro renderer behavior. A one-line addition would avoid surprise. See inline comment.

4. Double URI parse in local_renderer_url_port_for_kill (minor)

localhost_renderer_url?(url) already calls URI.parse(url) internally (and returns true only on success), then URI.parse(url).port immediately re-parses the same string. Trivial, but a single parse with result reuse would be cleaner. See inline comment.

Positive highlights

• Non-blocking socket probe with SOCKET_PROBE_TIMEOUT_SECS cap is well-implemented — prevents a hung Overmind socket from stalling startup indefinitely.
• /proc → lsof fallback for cross-directory PID checks handles both minimal Linux CI containers (no lsof) and macOS correctly.
• ENV_KEYS_TO_PRESERVE nil-values fix in preserve_runtime_env_vars closes the Bundler resurrection bug cleanly.
• CONDUCTOR_PORT contract hedging is appropriately documented without over-committing to an undocumented third-party API.
• URL_WITH_EXPLICIT_PORT_RE regex comments explaining the userinfo/IPv6 subtleties are genuinely useful for future maintainers.

Round 22 review wrap-up — 16 fixes from claude/cursor/codex bots

All new review threads opened against commit a7d748a6 have been audited and addressed in commit b1cc0bb7. 31 review threads resolved.

Bugs fixed

• run_production_like now rescues PortSelector::NoPortAvailable — exits cleanly with a one-line warning instead of producing an unhandled Ruby backtrace, mirroring configure_ports. New regression spec at server_manager_spec.rb.
• OSS apps no longer see a renderer port-in-use warning when base+2 collides with an unrelated service — threaded a pro_renderer: keyword through select_ports → base_port_ports → warn_if_derived_ports_in_use, populated by ServerManager from pro_renderer_active?. New spec at port_selector_spec.rb.

Polish

• Whitespace-padded but-otherwise-valid PORT / SHAKAPACKER_DEV_SERVER_PORT are now stripped and written back to ENV, matching the existing RENDERER_PORT normalization. Resolves the cross-var inconsistency the cursor and claude bots both flagged.
• The RENDERER_PORT-without-URL derivation message now starts with WARNING: so grep WARNING bin/dev.log catches it alongside every other ENV-mutation warning.
• PortSelector.valid_port_string? now uses the existing TCP_PORT_MAX constant instead of the literal 65_535.
• sync_renderer_port_and_url no longer pretends warn_url_without_port returns a meaningful value (return warn_url_without_port(url) → warn_url_without_port(url); return).
• FileManager::SOCKET_PROBE_TIMEOUT_SECS hoisted to the class body and reduced from 1.0 s to 0.15 s — UNIX socket connect to a dead listener fails in microseconds, so worst-case overhead across many stale sockets is now bounded.
• Dropped the dead path.nil? || path.empty? guard on File.readlink in working_directory_via_proc — File.readlink either returns a non-empty String or raises a rescued error.
• cleanup_socket_files (kill flow) now uses the same tmp/sockets/overmind*.sock glob as FileManager#cleanup_overmind_sockets, so renamed/copied variants like overmind-4100.sock are removed during bin/dev kill too. New spec covers a copied-app socket path.
• local_renderer_url_port_for_kill now caches the parsed URI; refactored localhost_renderer_url? to delegate to a new localhost_hostname? helper.
• Procfile.static delegates to pnpm run node-renderer so future package.json flag additions (--max-old-space-size, --experimental-vm-modules, etc.) propagate without manual sync.

Documentation

• select_ports docstring carries an explicit @side_effect annotation for the ENV mutation; the inner read_and_sanitize_port_env! already uses ! to make the mutation explicit at the call site. Held off on renaming the public method to avoid breaking direct callers — happy to follow up if you'd prefer.
• pro_renderer_active? now has an inline note explaining why the legacy RENDERER_URL is intentionally included; warn_if_legacy_renderer_url_env_used calls out that the legacy var still triggers the renderer path until the user renames.
• killable_ports has a paragraph explaining that during bin/dev kill the previous session's RENDERER_PORT/REACT_RENDERER_URL are not exported into the new shell, so in OSS apps the renderer port is included only when the Pro gem is actually loaded.
• BASE_PORT_ENV_VARS comment extended to call out a future CONDUCTOR_PORT semantics-change failure mode (block-base → Rails port directly) and how a future "validate derived ports are reachable on startup" path would catch it.

Already addressed earlier in this PR (resolved with rationale)

Comments r3181025355, r3180916302, r3180974669, r3180966942, r3180803438 — all addressed by 5ef6d502 + a7d748a6 (local_renderer_url_port_for_kill).
Comment r3180848346 (P1 — remote renderer URL) addressed by 5ef6d502.
Comments r3178803638, r3180729124 addressed by 53c62ee0 (pro_renderer_active? gate on killable_ports).
Comments r3180787034, r3180803444 addressed by 3d328f8b (Socket.new inside begin, ArgumentError in rescue list).
Comment r3178803549 addressed earlier (port_selector.rb:230 already uses .strip.empty?).

Verification

• bundle exec rubocop on all changed files: 0 offenses.
• bundle exec rspec spec/react_on_rails/dev/file_manager_spec.rb spec/react_on_rails/dev/port_selector_spec.rb spec/react_on_rails/dev/server_manager_spec.rb: 250 examples, 0 failures.

I (Claude Code) made these changes in a fresh worktree on origin/jg/concurrent-worktree-ports and pushed b1cc0bb7 as a fast-forward (no force-push).