fix(onboard): persist per-sandbox dashboard ports for multi-sandbox UI access#2008
Open
nvshaxie wants to merge 2 commits intoNVIDIA:mainfrom
Open
fix(onboard): persist per-sandbox dashboard ports for multi-sandbox UI access#2008nvshaxie wants to merge 2 commits intoNVIDIA:mainfrom
nvshaxie wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
…I access Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughPer-sandbox dashboard ports are introduced: ports are derived, persisted to the registry, validated for conflicts during onboarding, and used during recovery and port-forwarding instead of a single global dashboard port. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "CLI / User"
participant Onboard as "onboard.ts"
participant Registry as "registry"
participant Agent as "Port Forwarder / Agent"
participant UI as "OpenClaw UI"
User->>Onboard: createSandbox(sandboxName, options)
Onboard->>Registry: getSandbox(sandboxName)
Registry-->>Onboard: existingEntry? (includes dashboardPort?)
Onboard->>Onboard: resolve dashboardPort (stored || DEFAULT_DASHBOARD_PORT)
Onboard->>Onboard: findDashboardPortConflict(sandboxName, dashboardPort)
alt conflict found
Onboard->>User: write error & exit(1)
else no conflict
Onboard->>Registry: registerSandbox(..., dashboardPort)
Registry-->>Onboard: ack
Onboard->>Agent: ensureSandboxPortForward(sandboxName, dashboardPort)
Agent-->>UI: forward localhost:dashboardPort -> UI endpoint
Onboard->>User: print access info (tokenized URL via forwarded port)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nemoclaw.ts (1)
244-268: LGTM — per-sandbox port retrieval with legacy fallback is correctly implemented.The
!= nullcheck properly handles legacy registry entries missingdashboardPort, and the fallback toDEFAULT_DASHBOARD_PORTpreserves backward compatibility.Optional defensive improvement: Consider validating that the computed port is a finite number to guard against corrupted registry data:
,
🛡️ Optional: Add NaN guard for corrupted registry data
const sandbox = registry.getSandbox(sandboxName); - const dashboardPort = - sandbox?.dashboardPort != null ? Number(sandbox.dashboardPort) : DEFAULT_DASHBOARD_PORT; + const rawPort = sandbox?.dashboardPort != null ? Number(sandbox.dashboardPort) : DEFAULT_DASHBOARD_PORT; + const dashboardPort = Number.isFinite(rawPort) ? rawPort : DEFAULT_DASHBOARD_PORT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 244 - 268, The computed dashboardPort may become NaN if registry.getSandbox(sandboxName)?.dashboardPort contains corrupted non-numeric data; after the existing ternary that sets dashboardPort (using Number(sandbox.dashboardPort) or DEFAULT_DASHBOARD_PORT), validate the resulting numeric value with Number.isFinite (or Number.isNaN) and, if it is not a finite number, fall back to DEFAULT_DASHBOARD_PORT so downstream uses (agentRuntime.buildRecoveryScript, the curl liveness check, and nohup gateway run) always receive a valid port number.src/lib/onboard.ts (1)
5523-5526: Minor redundancy in fallback chain.On line 5526,
chatUiUrl || \http://127.0.0.1:${storedPort}\`` is redundant becausechatUiUrlis guaranteed to be truthy after the assignment on line 5525 (it has its own fallback). The extra fallback is harmless but adds cognitive load.♻️ Suggested simplification
const storedPort = getSandboxDashboardPort(sandboxName); const chatUiUrl = options.chatUiUrl || process.env.CHAT_UI_URL || `http://127.0.0.1:${storedPort}`; - const dashboardPort = Number(getDashboardForwardPort(chatUiUrl || `http://127.0.0.1:${storedPort}`)); + const dashboardPort = Number(getDashboardForwardPort(chatUiUrl));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 5523 - 5526, The code redundantly passes a second fallback when calling getDashboardForwardPort because chatUiUrl is already guaranteed to be non-empty after its assignment; remove the extra fallback and call getDashboardForwardPort with chatUiUrl directly. Update the dashboardPort assignment (variable: dashboardPort) to use Number(getDashboardForwardPort(chatUiUrl)) and keep storedPort and chatUiUrl logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/onboard.test.ts`:
- Around line 2789-2794: The test currently spreads process.env into the spawn
env which allows an external NEMOCLAW_DASHBOARD_PORT to alter behavior; change
the env creation so NEMOCLAW_DASHBOARD_PORT cannot be inherited (e.g., build a
local env object from { ...process.env, HOME: tmpDir, PATH:
`${fakeBin}:${process.env.PATH || ""}`, NEMOCLAW_NON_INTERACTIVE: "1" } and then
delete env.NEMOCLAW_DASHBOARD_PORT or omit that key before passing it), ensuring
the conflict path in this test remains deterministic; update the env usage where
the spawn (or equivalent) is called in test/onboard.test.ts.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 5523-5526: The code redundantly passes a second fallback when
calling getDashboardForwardPort because chatUiUrl is already guaranteed to be
non-empty after its assignment; remove the extra fallback and call
getDashboardForwardPort with chatUiUrl directly. Update the dashboardPort
assignment (variable: dashboardPort) to use
Number(getDashboardForwardPort(chatUiUrl)) and keep storedPort and chatUiUrl
logic unchanged.
In `@src/nemoclaw.ts`:
- Around line 244-268: The computed dashboardPort may become NaN if
registry.getSandbox(sandboxName)?.dashboardPort contains corrupted non-numeric
data; after the existing ternary that sets dashboardPort (using
Number(sandbox.dashboardPort) or DEFAULT_DASHBOARD_PORT), validate the resulting
numeric value with Number.isFinite (or Number.isNaN) and, if it is not a finite
number, fall back to DEFAULT_DASHBOARD_PORT so downstream uses
(agentRuntime.buildRecoveryScript, the curl liveness check, and nohup gateway
run) always receive a valid port number.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 519118d3-a171-4a1b-ba93-228c2f4805c2
📒 Files selected for processing (6)
src/lib/onboard.tssrc/lib/ports.tssrc/lib/registry.tssrc/nemoclaw.tstest/onboard.test.tstest/registry.test.ts
Made-with: Cursor
Author
|
Addressed the review feedback in follow-up commit
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/onboard.test.ts (1)
2789-2797:⚠️ Potential issue | 🟠 MajorMake this conflict test deterministic by isolating
NEMOCLAW_DASHBOARD_PORT.On Line 2793, inheriting
process.envdirectly allows hostNEMOCLAW_DASHBOARD_PORTto bypass the default-18789conflict path and make the test flaky.🔧 Suggested patch
+ // Ensure host shell env cannot change the requested dashboard port in this test. + // eslint-disable-next-line `@typescript-eslint/no-unused-vars` + const { NEMOCLAW_DASHBOARD_PORT: _ignoredDashboardPort, ...inheritedEnv } = process.env; const result = spawnSync(process.execPath, [scriptPath], { cwd: repoRoot, encoding: "utf-8", env: { - ...process.env, + ...inheritedEnv, HOME: tmpDir, PATH: `${fakeBin}:${process.env.PATH || ""}`, NEMOCLAW_NON_INTERACTIVE: "1", }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2789 - 2797, The test's spawnSync invocation inherits process.env allowing an external NEMOCLAW_DASHBOARD_PORT to make the conflict branch nondeterministic; in the spawnSync env object (the call that assigns result via spawnSync in test/onboard.test.ts) explicitly set NEMOCLAW_DASHBOARD_PORT to the expected default (e.g., "18789") or remove it (set to undefined/"") so the test always hits the default-port conflict path instead of being influenced by the host environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/onboard.test.ts`:
- Around line 2789-2797: The test's spawnSync invocation inherits process.env
allowing an external NEMOCLAW_DASHBOARD_PORT to make the conflict branch
nondeterministic; in the spawnSync env object (the call that assigns result via
spawnSync in test/onboard.test.ts) explicitly set NEMOCLAW_DASHBOARD_PORT to the
expected default (e.g., "18789") or remove it (set to undefined/"") so the test
always hits the default-port conflict path instead of being influenced by the
host environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2da1f758-eb55-4a7a-919a-074c84b0f359
📒 Files selected for processing (3)
src/lib/onboard.tssrc/nemoclaw.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/nemoclaw.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change prevents multi-sandbox onboard flows from reusing the same dashboard port and silently repointing the OpenClaw UI at the most recently created sandbox.
It persists a
dashboardPortper sandbox in the registry, fails fast when a new sandbox tries to claim a port already owned by another sandbox, and preserves backward compatibility for legacy sandbox entries that do not yet have adashboardPortfield.Fixes #1179.
What changed
dashboardPortin sandbox registry entriesdashboardPortas the historical default port18789dashboardPortTest plan
npm run build:cli1878919000dashboardPortdo not falsely block onboarding on a new port19000,19001)openshell forward listshows separate forwards for:187891900019001dashboardPortfor newly created sandboxesNotes
A separate OpenShell policy application issue was observed during Brev validation (
openshell policy set ... -> Unimplemented), but it occurs after sandbox creation and forward setup and is not part of this fix.Made with Cursor
Summary by CodeRabbit
New Features
Tests