feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

[justin808]

Summary

• Adds a tool-agnostic REACT_ON_RAILS_BASE_PORT env var that derives all service ports from a single base value (Rails = base+0, webpack = base+1, renderer = base+2)
• Also recognizes CONDUCTOR_PORT as a fallback, so Conductor.build works out of the box
• Makes all Procfiles use ${PORT:-3000} and ${RENDERER_PORT:-3800} patterns instead of hardcoded values
• Propagates renderer port via RENDERER_PORT and REACT_RENDERER_URL env vars for automatic Pro Node Renderer configuration

This enables coding agent tools (Conductor.build, Codex, Quad Code, etc.) to run multiple worktrees concurrently without port conflicts — just set one env var and everything works.

Changes

Core logic:

• PortSelector: new base port priority tier (checked before explicit env vars and auto-detect)
• ServerManager.configure_ports: sets RENDERER_PORT and REACT_RENDERER_URL when base port is active

Procfile templates (all now use env var fallbacks):

• OSS: Procfile.dev, Procfile.dev-prod-assets, Procfile.dev-static-assets
• Pro: Procfile.dev, Procfile.prod, Procfile.static
• Generator (pro_setup.rb): uses ${RENDERER_PORT:-3800} in generated Procfile lines

Documentation:

• .env.example: documents base port, RENDERER_PORT, and REACT_RENDERER_URL
• process-managers.md: new "Coding Agent / CI Integration" and "Manual Worktree Port Setup (Pro)" sections

Test plan

• PortSelector unit tests: base port derivation, CONDUCTOR_PORT fallback, priority ordering, out-of-range fallback, nil renderer when no base port
• RuboCop passes on all changed files
• All pre-commit and pre-push hooks pass
• CI passes
• Manual test: REACT_ON_RAILS_BASE_PORT=4000 bin/dev prints correct port assignments
• Generator tests pass (updated assertions for new Procfile pattern)

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches bin/dev port selection and environment propagation across multiple modes, so misconfiguration could break local dev startup or point SSR at the wrong renderer URL/port. Changes are well-covered by extensive new/updated specs but affect widely-used developer tooling.

Overview
Adds deterministic “base port” mode to bin/dev: when REACT_ON_RAILS_BASE_PORT (or fallback CONDUCTOR_PORT) is set, Rails/webpack/node-renderer ports are derived as base+0/+1/+2 and override any conflicting per-service env vars, with warnings.

Improves dev reliability around Pro renderer configuration by syncing RENDERER_PORT ↔ REACT_RENDERER_URL, warning on legacy RENDERER_URL usage/mismatches, and preserving more runtime env vars across Bundler’s env reset so spawned processes keep the derived ports/URLs.

Hardens copied-worktree startup/kill flows by cleaning stale Overmind sockets and cross-directory tmp/pids/server.pid, updates Procfiles and generator output to use ${PORT:-...} / ${RENDERER_PORT:-...} defaults, and expands docs/tests to cover the new precedence rules and edge cases.

^{Reviewed by Cursor Bugbot for commit d90fcef. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features

  • Dev ports (Rails, webpack, renderer) can be driven from a single base-port env var, deterministically deriving offsets.

• Behavior Changes

  • Procfiles and examples now honor PORT/RENDERER_PORT via shell-defaults so runtime ports can be overridden.
  • Dev tooling forwards renderer env vars to spawned processes and synchronizes renderer URL/port when derived.

• Documentation

  • Added guidance on base-port usage, precedence, bounds, and manual Pro renderer setup.

• Tests

  • Expanded coverage for base-port derivation, precedence, bounds, invalid values, and fallbacks.

• Chores

  • Minor .gitignore addition.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

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

Cohort / File(s)	Summary
Core Port Selection & Server Manager react_on_rails/lib/react_on_rails/dev/port_selector.rb, react_on_rails/lib/react_on_rails/dev/server_manager.rb	Adds deterministic base-port mode using REACT_ON_RAILS_BASE_PORT (then CONDUCTOR_PORT) to derive { rails, webpack, renderer } = base+0/+1/+2; adds parsing, validation, offset constants, and branches ServerManager to apply base-derived envs or fall back to explicit/env-default logic with renderer synchronization.
Generators & Templates react_on_rails/lib/generators/react_on_rails/pro_setup.rb, react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example	Generator output and .env.example document REACT_ON_RAILS_BASE_PORT and use shell-defaults for renderer (RENDERER_PORT=${RENDERER_PORT:-3800}); adds renderer example vars and mapping notes.
Process Env Preservation react_on_rails/lib/react_on_rails/dev/process_manager.rb	Expanded ENV_KEYS_TO_PRESERVE to include RENDERER_PORT and REACT_RENDERER_URL so those vars are forwarded when running processes outside Bundler.
Procfile Fixtures (dummy / spec) react_on_rails/spec/dummy/Procfile.dev, react_on_rails/spec/dummy/Procfile.*, react_on_rails_pro/spec/.../Procfile.*	Procfile entries now honor env-driven ports with shell-default fallbacks (e.g., -p ${PORT:-3000}, RENDERER_PORT=${RENDERER_PORT:-3800}) instead of hardcoded ports.
Tests / Specs react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb, react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb, react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb, react_on_rails/spec/react_on_rails/support/...	Adds/expands specs covering base-port behavior, precedence (REACT_ON_RAILS_BASE_PORT over CONDUCTOR_PORT), out-of-range and invalid-value fallbacks, asserts no port probing when base is used, and updates generator expectations for shell-default renderer port.
Docs & Misc docs/oss/building-features/process-managers.md, .gitignore	Documentation added/expanded describing base-port usage, precedence, and manual Pro worktree setup; .gitignore updated to ignore .gstack/.

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

Loading

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 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: introducing REACT_ON_RAILS_BASE_PORT for managing concurrent worktree ports, which is the primary feature across all modified files.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/concurrent-worktree-ports

---

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

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37a9ae17d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

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

Filename	Overview
react_on_rails/lib/react_on_rails/dev/port_selector.rb	New base-port logic: validates integer format (warns on bad value) but silently ignores out-of-range integers without a warning; otherwise well-structured with correct MAX_BASE_PORT guard.
react_on_rails/lib/react_on_rails/dev/server_manager.rb	Port configuration split into base-port (unconditional override) and explicit paths; sync_renderer_port_and_url mismatch check uses substring matching which can produce false negatives for port-prefix collisions.
react_on_rails/lib/react_on_rails/dev/process_manager.rb	Correctly extends ENV_KEYS_TO_PRESERVE with RENDERER_PORT and REACT_RENDERER_URL so the vars survive with_unbundled_env when spawning the process manager.
react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb	Top-level around block properly isolates all relevant env vars; comprehensive new coverage for base-port derivation, precedence, out-of-range, and overflow boundary.
react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb	New contexts cover base-port override, explicit RENDERER_PORT derivation, URL-port consistency warning, and internal-host URL (renderer.internal) — good coverage of the new sync logic.
react_on_rails/lib/generators/react_on_rails/pro_setup.rb	Generator line updated to RENDERER_PORT=${RENDERER_PORT:-3800} — allows runtime override while preserving the previous default, consistent with Procfile changes.
docs/oss/building-features/process-managers.md	Adds clear "Coding Agent / CI Integration" and "Manual Worktree Port Setup (Pro)" sections with port-offset table, priority chain, and configuration examples.

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

Loading

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

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb`:
- Around line 98-102: The base_port method currently allows ENV values up to
65_535 but then derives additional ports as base+1 and base+2; update the
validation in base_port (and any use of BASE_PORT_ENV_VARS) to ensure the chosen
base plus the highest offset (2) stays ≤ 65_535 (i.e., accept bases in the range
1..65_533), so change the conditional that returns val to check val between 1
and (65_535 - 2) and reject/skip values outside that safe range.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 842-845: The current code may set ENV["REACT_RENDERER_URL"] using
selected[:renderer] even when an existing ENV["RENDERER_PORT"] was preserved;
update the logic so REACT_RENDERER_URL is constructed from the effective port
value (the ENV["RENDERER_PORT"] after the ||= assignment) rather than from
selected[:renderer]; locate the block that references selected[:renderer] in
server_manager.rb (the if selected[:renderer] branch) and change the URL
construction to use ENV["RENDERER_PORT"] (or a local port variable assigned from
ENV["RENDERER_PORT"]) so the URL always matches the actual RENDERER_PORT in use.

In `@react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb`:
- Around line 10-15: Wrap each around hook's example.run in a begin...ensure so
ENV cleanup always runs (e.g., change the around do |example| block that sets
ENV["REACT_ON_RAILS_BASE_PORT"] to use begin; example.run; ensure ... end), and
in the "CONDUCTOR_PORT only" and "out-of-range base port" contexts explicitly
unset or restore both ENV["REACT_ON_RAILS_BASE_PORT"] and ENV["CONDUCTOR_PORT"]
(capture old values, set the test values, run example, then in ensure restore
originals or delete the keys when nil) so base_port's lookup order cannot be
influenced by ambient env vars.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e7ac5b86-7952-413d-89b3-278d22119277

📥 Commits

Reviewing files that changed from the base of the PR and between de5b53a and 37a9ae1.

📒 Files selected for processing (17)

• docs/oss/building-features/process-managers.md
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/lib/react_on_rails/dev/port_selector.rb
• react_on_rails/lib/react_on_rails/dev/server_manager.rb
• react_on_rails/spec/dummy/Procfile.dev
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static

[claude]

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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8da5a8a004

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

♻️ Duplicate comments (2)

react_on_rails/lib/react_on_rails/dev/server_manager.rb (1)

836-843: ⚠️ Potential issue | 🟠 Major

Apply selected ports as the effective env values.

PortSelector now gives the base port higher priority, but ||= lets pre-existing PORT / SHAKAPACKER_DEV_SERVER_PORT / RENDERER_PORT override that selection before child processes start. This can make REACT_ON_RAILS_BASE_PORT=4000 still boot on stale per-service ports, and it also preserves the previously flagged renderer URL drift.

🐛 Proposed fix

 def configure_ports
   selected = PortSelector.select_ports
-  ENV["PORT"] ||= selected[:rails].to_s
-  ENV["SHAKAPACKER_DEV_SERVER_PORT"] ||= selected[:webpack].to_s
+  ENV["PORT"] = selected[:rails].to_s
+  ENV["SHAKAPACKER_DEV_SERVER_PORT"] = selected[:webpack].to_s
   if selected[:renderer]
-    ENV["RENDERER_PORT"] ||= selected[:renderer].to_s
-    ENV["REACT_RENDERER_URL"] ||= "http://localhost:#{selected[:renderer]}"
+    ENV["RENDERER_PORT"] = selected[:renderer].to_s
+    ENV["REACT_RENDERER_URL"] ||= "http://localhost:#{ENV['RENDERER_PORT']}"
   end
 rescue PortSelector::NoPortAvailable => e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb` around lines 836 -
843, The configure_ports method currently uses ||= which lets pre-existing ENV
values override PortSelector.select_ports; change the assignments in
configure_ports (the selected = PortSelector.select_ports block) to always set
ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"], and when selected[:renderer]
set ENV["RENDERER_PORT"] and ENV["REACT_RENDERER_URL"] using direct assignment
(=) so the selected ports become the effective env values for child processes
and the renderer URL is updated consistently.

react_on_rails/lib/react_on_rails/dev/port_selector.rb (1)

98-102: ⚠️ Potential issue | 🟠 Major

Validate the base port against the highest derived offset.

Line 101 accepts 65_534 and 65_535, but select_ports derives webpack = base + 1 and renderer = base + 2, producing invalid ports above 65_535.

🐛 Proposed fix

 def base_port
+  max_base_port = 65_535 - BASE_PORT_RENDERER_OFFSET
   BASE_PORT_ENV_VARS.each do |var|
     val = ENV[var]&.to_i
-    return val if val&.between?(1, 65_535)
+    return val if val&.between?(1, max_base_port)
   end
   nil
 end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb` around lines 98 -
102, The base_port method currently accepts ENV values up to 65_535 but
select_ports computes webpack = base + 1 and renderer = base + 2, which can
yield ports > 65_535; update the validation in base_port (method name base_port,
referenced by select_ports) to enforce an upper bound of 65_533 (i.e., ensure
val.between?(1, 65_533) or subtract the maximum derived offset before
validation) so that adding +1 and +2 cannot produce invalid ports.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb`:
- Around line 98-102: The base_port method currently accepts ENV values up to
65_535 but select_ports computes webpack = base + 1 and renderer = base + 2,
which can yield ports > 65_535; update the validation in base_port (method name
base_port, referenced by select_ports) to enforce an upper bound of 65_533
(i.e., ensure val.between?(1, 65_533) or subtract the maximum derived offset
before validation) so that adding +1 and +2 cannot produce invalid ports.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 836-843: The configure_ports method currently uses ||= which lets
pre-existing ENV values override PortSelector.select_ports; change the
assignments in configure_ports (the selected = PortSelector.select_ports block)
to always set ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"], and when
selected[:renderer] set ENV["RENDERER_PORT"] and ENV["REACT_RENDERER_URL"] using
direct assignment (=) so the selected ports become the effective env values for
child processes and the renderer URL is updated consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 89dd322b-8ac2-4e3f-9f75-b9ef2476038a

📥 Commits

Reviewing files that changed from the base of the PR and between 4216467 and 8da5a8a.

📒 Files selected for processing (18)

• .gitignore
• docs/oss/building-features/process-managers.md
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/lib/react_on_rails/dev/port_selector.rb
• react_on_rails/lib/react_on_rails/dev/server_manager.rb
• react_on_rails/spec/dummy/Procfile.dev
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static

✅ Files skipped from review due to trivial changes (13)

• .gitignore
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails/spec/dummy/Procfile.dev
• docs/oss/building-features/process-managers.md

🚧 Files skipped from review as they are similar to previous changes (3)

• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb

[justin808]

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.

[chatgpt-codex-connector]

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

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me quickly verify the key changes in the updated code.

---

🏁 Script executed:

#!/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

Length of output: 58432

---

@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!

---

🧠 Learnings used

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/`

[justin808]

Round 24 review wrap-up — 8 fixes from claude[bot] code review

I (Claude Code) addressed every item from the latest claude[bot] review (1 review summary + 3 inline comments). All 8 changes landed in 11877c6 on jg/concurrent-worktree-ports.

MUST-FIX (2)

1. server_manager.rb configure_ports comment — rewrote the 6-line block (lines 965-972) so the SHAKAPACKER_DEV_SERVER_PORT claim is scoped to the non-base-port path; explicitly noted that base-port mode still sets SHAKAPACKER_DEV_SERVER_PORT = base + 1 for tooling consistency. Reply on the inline thread; resolved.
2. server_manager_spec.rb partial-shape stubs — updated all five select_ports! stubs in the file (lines 366, 397, 405, 420, 435) to the full hash shape { rails:, webpack:, renderer: nil, base_port_mode: false }, matching the shape already used at lines 69/655/677/705. Locks in the contract so a future selected.fetch(:base_port_mode) refactor cannot raise KeyError. All 189 server_manager_spec examples pass. Reply on the inline thread; resolved.

DISCUSS (1)

3. CONDUCTOR_PORT semantic-drift surfacing — kept the existing advisory-only contract (TOCTOU rationale at port_selector.rb:187-189) but enriched the warning. When CONDUCTOR_PORT is the source and the Rails port (base + 0) is bound, the warning now appends: "If your Conductor workspace exposes CONDUCTOR_PORT as the Rails port rather than a block base, set REACT_ON_RAILS_BASE_PORT explicitly to override." This surfaces the most likely Conductor-contract-drift failure mode immediately without changing the broader advisory→fail-fast design. Webpack/renderer collisions still get the plain warning since the hint would mislead there. New spec test locks the behavior.

OPTIONAL (5)

4. file_manager.rb working_directory_via_lsof rescue — split the rescue StandardError so Errno::ENOENT (lsof binary missing) stays DEBUG-gated (expected on minimal images), but every other StandardError (EMFILE, EINTR, EPERM, etc.) now always-warns. The cross-directory PID check can no longer silently misfire under resource pressure. Reply on the inline thread; resolved.
5. docs/oss/building-features/process-managers.md Pro-renderer activation note — added a "Heads up" callout in the base-port section explaining that setting RENDERER_PORT, RENDERER_URL, or REACT_RENDERER_URL activates the Pro-renderer code path even in OSS apps, and in base-port mode causes RENDERER_PORT/REACT_RENDERER_URL to be derived/propagated. Helps users avoid surprise when they reuse RENDERER_PORT for an unrelated purpose.
6. server_manager.rb legacy-RENDERER_URL warning text — rewrote lines 1023-1029 to separate two concepts that the old text conflated: (a) "RENDERER_URL alone activates the Pro renderer code path" and (b) "if REACT_ON_RAILS_BASE_PORT/CONDUCTOR_PORT is set, base-port mode derives RENDERER_PORT/REACT_RENDERER_URL from the base." Users not running base-port mode no longer see the misleading "base-port mode will derive…" tail.
7. CHANGELOG.md env-preservation addendum — extended the existing PR feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management #3142 entry to call out that RENDERER_PORT, REACT_RENDERER_URL, and SHAKAPACKER_SKIP_PRECOMPILE_HOOK are now preserved across Bundler's env reset (previously only PORT and SHAKAPACKER_DEV_SERVER_PORT were). This documents the real behavior change vs main.
8. file_manager.rb SOCKET_PROBE_TIMEOUT_SECS comment — extended the comment to acknowledge two consumption paths: synchronous failure is microseconds (so 150 ms is conservative), but the async IO::WaitWritable path can use the full 150 ms budget — that's the actual cap for slow-loopback or paused-PID cases.

Local verification

• bundle exec rspec spec/react_on_rails/dev/ — 355 examples, 0 failures (added 1 new CONDUCTOR_PORT hint test).
• bundle exec rubocop lib/react_on_rails/dev/ spec/react_on_rails/dev/ — no offenses. Added lib/react_on_rails/dev/port_selector.rb to Metrics/ClassLength exclude list with justification, matching the existing server_manager.rb treatment.

The 3 inline review threads are resolved. The 5 summary-only items are addressed in this comment plus the diff in 11877c6.

[claude]

Code Review

Overview

This PR adds REACT_ON_RAILS_BASE_PORT (with CONDUCTOR_PORT fallback) to derive all dev-service ports from one value, making concurrent worktrees and agent sandboxes collision-free. It also tightens FileManager to clean stale copied runtime files on startup and adds thorough port-env validation throughout bin/dev.

The design is solid: the priority chain is clear, ENV mutation is explicitly documented (! suffix, side-effect comments), the pro_renderer_active? gate keeps OSS apps clean, and test coverage is thorough. A few issues are worth addressing before merge.

---

Issues

1. invalid_base_port_warning can falsely promise fallthrough activation (port_selector.rb:264): remaining is filtered to non-empty env vars, but not validated. If REACT_ON_RAILS_BASE_PORT="disabled" and CONDUCTOR_PORT="abc", the warning says "Base port mode will still activate from CONDUCTOR_PORT" — but it won't, because CONDUCTOR_PORT is also invalid. The loop will issue a second "not a valid integer" warning for it and skip it, so base-port mode stays off. The current test suite covers the valid-CONDUCTOR_PORT case but misses the double-invalid scenario. Fix: validate remaining values in the filter (check match?(/\A\d+\z/) and range), or drop the fallthrough hint for the unusual "I set a non-integer to disable base-port" case.

2. clear_local_renderer_url_after_invalid_port deletes an explicitly-set env var (server_manager.rb:1247): When a user has RENDERER_PORT=abc and REACT_RENDERER_URL=http://localhost:3800, the invalid port causes the valid, user-supplied URL to be deleted. The rationale is to avoid a mismatch, but the net result is that Rails falls back to http://localhost:3800 — which is exactly what the user had set manually. Keeping REACT_RENDERER_URL and only clearing the invalid RENDERER_PORT (with a warning) would be less destructive and equally correct.

3. ArgumentError catch in socket_active? is too broad (file_manager.rb:111): The stated intent is to handle the "UNIX socket path too long" case from Socket.sockaddr_un, but catching all ArgumentError also silently swallows programming mistakes elsewhere in the method. Consider an explicit path-length guard before calling Socket.sockaddr_un, or rescuing only the specific downstream call.

4. Breaking rename: PortSelector.select_ports → select_ports! (port_selector.rb:68): Any third-party code or generator extensions calling the old method will get NoMethodError. Since this is an internal Dev module the blast radius is probably small, but a one-line deprecated alias (def select_ports; select_ports!; end) would make the transition safe for anyone who depended on the previous interface.

---

Minor Notes

• configured_renderer_port_for_kill falls back to 3800 when the Pro gem is loaded but no port env vars are set — bin/dev kill will attempt to kill port 3800 even if no renderer was ever started. This is a no-op, but silently widens the kill scope for any OSS+Pro gem app.

• The overwrite_invalid_port_env call in apply_explicit_port_env runs after select_ports! already cleared bad values via read_and_sanitize_port_env!. When PORT=abc was set, overwrite_invalid_port_env sees nil and sets the derived port silently (warning already fired earlier). This is correct but the coupling is subtle — a brief inline note in apply_explicit_port_env would help future readers.

• The MAX_BASE_PORT boundary test (spec line ~1770) asserts result[:renderer] both equals MAX_BASE_PORT + 2 and <= 65_535. The second assertion is redundant given the first; removing it avoids implying there is an independent upper-bound check.

---

Overall this is a well-thought-out feature with good documentation and edge-case coverage. The four numbered issues above are the ones worth addressing before merge; the minor notes are polish.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 9 total unresolved issues (including 8 from previous reviews).

^{Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 11877c6. Configure here.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[justin808]

Round 25 review wrap-up — 8 fixes from the latest claude/cursor bot pass

I (Claude Code) addressed every item from the latest claude[bot] code review plus the cursor[bot] inline (commit 2c67e07). All 5 unresolved inline threads have been replied-to and resolved; the 3 review-summary minor notes are addressed below.

Inline threads (resolved)

#	File	Bot	Disposition
1	port_selector.rb:264	claude	MUST-FIX — filter remaining with the same numeric + 1..MAX_BASE_PORT check as base_port_ports so REACT_ON_RAILS_BASE_PORT="disabled" + CONDUCTOR_PORT="abc" no longer falsely says "will still activate from CONDUCTOR_PORT". Two new specs lock the double-invalid + out-of-range cases.
2	file_manager.rb:111	claude	DISCUSS resolved — narrowed the ArgumentError rescue. Bot's literal suggestion (replace with Errno::ENAMETOOLONG) was wrong (Ruby raises ArgumentError, not Errno::ENAMETOOLONG, for "too long unix socket path"), so I wrapped only Socket.sockaddr_un in a tight begin/rescue ArgumentError. The wider connect block no longer catches ArgumentError, so a programming mistake in Socket.new / connect_nonblock now surfaces instead of being silently swallowed — which was the underlying concern.
3	server_manager.rb:1247	claude	DISCUSS resolved (kept behavior) — kept clear_local_renderer_url_after_invalid_port's delete-on-invalid-port behavior but expanded the comment to walk through both the URL-mismatches-default case (clearing prevents Rails/renderer mismatch) and the URL-matches-default case (clearing prevents future Procfile-default changes from silently re-introducing the mismatch).
4	port_selector.rb:68	claude	DISCUSS resolved (alias added) — added a quiet select_ports alias as a safety net for any external caller on the pre-bang name. The bang form remains preferred (round 23 rationale: surface ENV mutation at the call site); the alias is documented as deprecated and removable in a future major. Kept it quiet (no warn) to avoid CI churn for downstream users on a barely-used internal API.
5	server_manager.rb:728	cursor	OPTIONAL fixed — run_production_like now strips whitespace from a valid PORT (e.g. " 3001 " → "3001"), mirroring overwrite_invalid_port_env. All bin/dev modes now leave ENV["PORT"] in the same shape for downstream consumers.

Review-summary minor notes (no inline threads to resolve)

• Minor A — configured_renderer_port_for_kill was returning 3800 for OSS+Pro-gem apps that never run the renderer, silently widening bin/dev kill's scope. Extracted a renderer_env_signal? helper from pro_renderer_active?; the kill helper now returns nil when no renderer env var is set. Updated the spec that locked in the old behavior; added a regression spec for the legitimate REACT_RENDERER_URL=http://localhost (no port) → kill 3800 fallback.
• Minor B — extended apply_explicit_port_env's comment to spell out the upstream warning relationship: PortSelector.read_and_sanitize_port_env! has already cleared invalid PORT/SHAKAPACKER_DEV_SERVER_PORT values and warned, so overwrite_invalid_port_env silently writes the derived port — the warning is upstream, not missing.
• Minor C — dropped the redundant expect(result[:renderer]).to be <= 65_535 assertion in the MAX_BASE_PORT boundary context. The preceding eq(MAX_BASE_PORT + 2) already pins the value; the second line falsely implied an independent upper-bound check.

Verification

• 266 dev specs pass (3 new specs added across the round)
• RuboCop clean on lib/react_on_rails/dev and spec/react_on_rails/dev
• Pre-commit hooks (autofix + rubocop + trailing-newlines) pass

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Code Review: REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

Overview

This PR is a well-engineered feature that adds deterministic port allocation for multi-worktree and coding-agent environments. The design is sound — a single base port env var deriving all service ports via fixed offsets is the right abstraction. The test coverage is excellent, ENV isolation in tests is correctly implemented, and the backward-compatibility alias preserves external callers.

A few specific issues to address, roughly in priority order:

---

Issues

1. Missing deprecation warning on select_ports alias

The deprecated alias calls select_ports! silently. Any external caller (generator extension, host-app Rake task) who wired to select_ports before the rename won't know it's deprecated until they read the CHANGELOG. Recommend adding a warn line so gem consumers discover the rename at runtime rather than at the next major bump.

2. Inline rescue inside .each block in local_renderer_url_port_for_kill

Ruby allows rescue in a block body (rescuing that iteration), but it's easy to mistake for rescuing the entire method. A conventional begin/rescue inside the block would be clearer and more explicitly scoped.

3. lsof -Fn parsing could be fragile on older lsof

stdout.lines.find { |line| line.start_with?("n") } works correctly with modern lsof and -d cwd, but old versions on some Linux distros may interleave header lines. Adding -w (suppress lsof warnings) would reduce noise. Currently if no n-prefixed line appears the method returns nil (safe fallback — PID file is kept), so there is no data-integrity risk, just a potential silent miss.

4. same_working_directory? fallback doesn't resolve symlinks

The rescue SystemCallError fallback to File.expand_path does not resolve symlinks. If a directory exists but cannot be realpath-resolved (broken symlink mid-path, Errno::EACCES on an intermediate dir), expand_path may return two different-looking paths for the same physical directory. Narrowing the rescue to Errno::ENOENT and returning false for other errors is safer — the caller's nil return already keeps the PID file as the safe outcome.

5. renderer_env_signal? includes the very generic legacy RENDERER_URL

A user who has RENDERER_URL set for an unrelated purpose (it's a generic name) will inadvertently activate the Pro renderer code path, causing base-port mode to derive and inject RENDERER_PORT and REACT_RENDERER_URL into the environment. The docs warn about this side effect, but RENDERER_URL is generic enough that this could affect more users than expected. Consider whether RENDERER_URL alone (without the Pro gem loaded) should still activate the renderer path, or only RENDERER_PORT / REACT_RENDERER_URL should.

6. apply_base_port_env sets SHAKAPACKER_DEV_SERVER_PORT in production-like mode — env leaks to shell

This is documented as intentional for tooling consistency. However, after bin/dev prod the shell retains SHAKAPACKER_DEV_SERVER_PORT=base+1. If the user then unsets REACT_ON_RAILS_BASE_PORT in a new command but keeps the same shell session, a subsequent bin/dev picks up the stale SHAKAPACKER_DEV_SERVER_PORT as an explicit env var, bypassing base-port derivation. Worth a note in the docs for bin/dev prod.

---

Strengths

• Test isolation: around save/restore blocks for ENV in specs correctly prevent a Conductor workspace environment from contaminating the suite.
• socket_active?: Correctly handles TOCTOU between File.exist? and the connect attempt (caught by SystemCallError), ArgumentError for paths exceeding sun_path limit, and uses fiber-scheduler-compatible wait_writable.
• working_directory_for_pid: Two-stage /proc → lsof design is correct for cross-platform support. Nil-return safe fallback (keep the PID file) is the right invariant.
• preserve_runtime_env_vars nil semantics: Passing nil for unset keys explicitly unsets them in child processes, preventing Bundler from resurrecting a deleted invalid env var. This subtle fix is well-tested and the invariant is clearly documented.
• URL regex URL_WITH_EXPLICIT_PORT_RE: The userinfo-prefix anchor prevents matching password digits as a host port — confirmed by the dedicated test.
• base_port_hash vs base_port_ports split: Kill and help-display paths correctly use the silent form; only configure_ports emits the "Base port detected" banner.

---

Minor nits

• socket_active? has no coupling to PortSelector's state; extracting it into a SocketProbe module would reduce FileManager class size naturally and eliminate the RuboCop exception.
• In invalid_base_port_warning, the remaining filter does not check val <= PRIVILEGED_PORT_MAX, so a privileged-but-valid CONDUCTOR_PORT appears in the "will still activate from" message. Low priority since privileged ports are warned about but not rejected.
• The select_ports docstring says "Remove in a future major release" — a CHANGELOG entry or version-tagged TODO would make the intent more actionable.

[claude]

The deprecated alias calls select_ports! with no user-visible signal that the method is deprecated. Any external caller (generator extension, host-app Rake task) who wired to select_ports before the rename won't know until they read the CHANGELOG or hit a future removal.

Consider emitting a runtime deprecation warning:

Suggested change

	select_ports!(**kwargs)
	def select_ports(**kwargs)
	warn "[DEPRECATION] ReactOnRails::Dev::PortSelector.select_ports is deprecated; " \
	"use select_ports! instead. Called from #{caller_locations(1, 1).first}"
	select_ports!(**kwargs)
	end

[claude]

The inline rescue inside an .each block is valid Ruby (it rescues only the current iteration), but it reads as if it rescues the entire method. A reader skimming the code will see return parsed.port followed by rescue and assume the rescue applies to the block and the surrounding method body.

Suggested change

	return parsed.port
	def local_renderer_url_port_for_kill
	%w[REACT_RENDERER_URL RENDERER_URL].each do |var|
	url = ENV.fetch(var, nil)
	next if url.nil? || url.strip.empty?
	begin
	parsed = URI.parse(url)
	next unless localhost_hostname?(parsed.hostname)
	next unless url.match?(URL_WITH_EXPLICIT_PORT_RE)
	return parsed.port
	rescue URI::InvalidURIError
	next
	end
	end
	nil
	end

[claude]

The fallback to File.expand_path when File.realpath raises SystemCallError is too broad. expand_path does not resolve symlinks, so a directory that exists but can't be realpath-resolved (e.g., broken symlink mid-path, Errno::EACCES on an intermediate directory) can return two different-looking strings for the same physical path — falsely treating the same directory as "different" and deleting the PID file when it should be kept.

The safe outcome here is to keep the PID file (return false from cleanup_rails_pid_file). Propagating or returning nil from same_working_directory? achieves that:

Suggested change

	# syscall (Errno::EINTR), permission errors, etc. Always-warn so the
	def same_working_directory?(left, right)
	File.realpath(left) == File.realpath(right)
	rescue Errno::ENOENT
	# One path no longer exists (process exited mid-probe); treat as different.
	false
	end

[claude]

Adding -w to suppress lsof warnings and -F0n (null-delimited) instead of -Fn would make parsing more robust across old lsof versions that may emit header lines before the n-prefixed cwd entry. As-is, if lsof emits a non-cwd n-prefixed line before the cwd line, this returns the wrong path — low risk with -d cwd, but worth tightening.

Suggested change

	stdout, = Open3.capture2("lsof", "-a", "-p", pid.to_s, "-d", "cwd", "-Fn", "-w", err: File::NULL)