Skip to content

refactor(onboard): extract dashboard port create resolver - #5128

Merged
cv merged 6 commits into
mainfrom
codex/onboard-dashboard-port-flow
Jun 11, 2026
Merged

refactor(onboard): extract dashboard port create resolver#5128
cv merged 6 commits into
mainfrom
codex/onboard-dashboard-port-flow

Conversation

@cv

@cv cv commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Extracts createSandbox dashboard port and CHAT_UI_URL resolution into the dashboard-port helper so the precedence chain can be reviewed and tested outside the full onboarding flow. This draft PR is stacked on #5125 and keeps Hermes dashboard forwarding orchestration in onboard.ts for a later slice.

Related Issue

Refs #3802

Changes

  • Added resolveCreateSandboxDashboardPort to centralize --control-ui-port, CHAT_UI_URL, registry, agent, and default port precedence.
  • Replaced the inline createSandbox dashboard port/URL block with the helper while preserving the existing effectivePort and chatUiUrl flow.
  • Added focused coverage for precedence, warning emission, schemeless CHAT_UI_URL normalization, remote host URL rewriting, and malformed CHAT_UI_URL behavior.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

Targeted checks run:

  • npx @biomejs/biome lint src/lib/onboard.ts src/lib/onboard/dashboard-port.ts src/lib/onboard/dashboard-port.test.ts
  • npm run typecheck:cli
  • npm run build:cli
  • git diff --check
  • npx vitest run src/lib/onboard/dashboard-port.test.ts test/onboard.test.ts test/onboard-custom-dockerfile.test.ts test/shellquote-sandbox.test.ts

Required live validation:


Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • Refactor

    • Refactored internal dashboard port and URL selection logic for improved maintainability.
  • Tests

    • Added comprehensive tests for dashboard port selection and URL normalization behavior.

@cv cv self-assigned this Jun 10, 2026
@copy-pr-bot

copy-pr-bot Bot commented Jun 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c98e1a0e-7d5c-41db-9750-7b99f7ea89d7

📥 Commits

Reviewing files that changed from the base of the PR and between 56e5dfd and f959ccc.

📒 Files selected for processing (3)
  • src/lib/onboard.ts
  • src/lib/onboard/dashboard-port.test.ts
  • src/lib/onboard/dashboard-port.ts

📝 Walkthrough

Walkthrough

This PR refactors dashboard port and chat UI URL selection logic from inline code in createSandbox into a dedicated, reusable resolver function with explicit port precedence, URL normalization, and comprehensive test coverage.

Changes

Dashboard Port Resolution Refactoring

Layer / File(s) Summary
Dashboard port resolver contract and implementation
src/lib/onboard/dashboard-port.ts
New exported CreateSandboxDashboardPortInput/CreateSandboxDashboardPortResult interfaces and resolveCreateSandboxDashboardPort function that selects a port via ordered fallbacks (control UI port → parsed chat UI URL port → persisted port → agent forward port → provided default → DASHBOARD_PORT), computes effective port using an injectable port-availability function, optionally warns when effective port differs, and normalizes/rewrites the chat UI URL to match the selected port.
Integration in createSandbox flow
src/lib/onboard.ts
Replaced inline port/URL priority resolution and manual CHAT_UI_URL parsing with a single call to resolveCreateSandboxDashboardPort, passing sandbox name, control UI port, environment chat UI URL, persisted registry data, agent forward port, default port, current forward-list output, and a warning callback; returns { effectivePort, chatUiUrl } used downstream.
Test coverage for resolver
src/lib/onboard/dashboard-port.test.ts
New test suite verifying port-selection precedence (--control-ui-port override), CHAT_UI_URL handling (precedence, port rewriting, schemeless normalization to http://), fallback through persisted/agent/default ports, optional warning on port unavailability, error throwing on malformed URLs when the env URL would be used, and graceful ignore of malformed URLs when --control-ui-port supplies the URL.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • prekshivyas

Poem

🐰 A port resolver hops into view,
With precedence logic tried and true,
URLs normalized, warnings take flight,
The sandbox dashboard shines bright! 🌟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: extracting dashboard port creation logic into a dedicated resolver function, which is the primary refactoring activity across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 codex/onboard-dashboard-port-flow

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

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor

Findings: 0 needs attention, 0 worth checking, 0 nice ideas
Since last review: 1 prior item resolved, 2 still apply, 0 new items found

Consider writing more tests for
  • **Runtime validation** — Auto-allocated dashboard port consistency across create flow — when the requested CHAT_UI_URL port is occupied, onboard create uses the same replacement port in CHAT_UI_URL, NEMOCLAW_DASHBOARD_PORT, dashboard readiness probing, registry metadata, and final ensureDashboardForward.. The pure resolver has good unit coverage, but the changed behavior ultimately crosses OpenShell forward state, sandbox env injection, dashboard readiness probing, registry metadata, and Hermes dashboard orchestration.
  • **Runtime validation** — Hermes reserved API port rejection through create resolver — Hermes onboard still rejects port 8642 when selected via --control-ui-port, CHAT_UI_URL, or persisted registry state after the resolver extraction.. The pure resolver has good unit coverage, but the changed behavior ultimately crosses OpenShell forward state, sandbox env injection, dashboard readiness probing, registry metadata, and Hermes dashboard orchestration.
  • **Runtime validation** — Valid CHAT_UI_URL without explicit port preserves host/path but injects the allocated/default dashboard port — for example, https://chat.example.test/ui/ produces the expected host/path and effective port.. The pure resolver has good unit coverage, but the changed behavior ultimately crosses OpenShell forward state, sandbox env injection, dashboard readiness probing, registry metadata, and Hermes dashboard orchestration.
  • **Runtime validation** — Issue Umbrella: refactor onboarding into a serializable FSM #3802 acceptance traceability — add or identify test evidence once literal issue body/comments are available.. The pure resolver has good unit coverage, but the changed behavior ultimately crosses OpenShell forward state, sandbox env injection, dashboard readiness probing, registry metadata, and Hermes dashboard orchestration.
  • **Acceptance clause:** Refs Umbrella: refactor onboarding into a serializable FSM #3802 — add test evidence or identify existing coverage. The PR body references Umbrella: refactor onboarding into a serializable FSM #3802, but deterministic linkedIssues is empty and no literal issue body/comments were provided, so issue acceptance clauses could not be extracted or mapped to the diff.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard-e2e, sandbox-operations-e2e, concurrent-gateway-ports-e2e
Optional E2E: test-e2e-port-overrides, hermes-dashboard-e2e, dashboard-remote-bind-e2e

Dispatch hint: cloud-onboard-e2e,sandbox-operations-e2e,concurrent-gateway-ports-e2e

Auto-dispatched E2E: cloud-onboard-e2e, sandbox-operations-e2e, concurrent-gateway-ports-e2e via nightly-e2e.yaml at f959cccf6410b1c77c55d54a2482b878aa462a13nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard-e2e (high): Validates the real non-interactive install/onboard path creates a sandbox and brings the dashboard/inference stack up after the dashboard-port resolution refactor.
  • sandbox-operations-e2e (high): Contains a multi-sandbox onboard path that sets CHAT_UI_URL for the second sandbox; this directly exercises the changed CHAT_UI_URL port precedence and URL rewrite behavior in createSandbox.
  • concurrent-gateway-ports-e2e (high): Directly verifies two sandboxes on one host get distinct dashboard ports and that NemoClaw list reports distinct dashboard URLs, covering the allocation/collision behavior changed in dashboard-port.ts.

Optional E2E

  • test-e2e-port-overrides (medium): Useful adjacent PR self-hosted coverage for dashboard port override propagation inside the sandbox image, though it does not directly exercise host-side createSandbox CHAT_UI_URL selection.
  • hermes-dashboard-e2e (high): Optional confidence for agent/dashboard variants that depend on the same effectivePort computed during sandbox creation, especially when Hermes dashboard forwarding is enabled.
  • dashboard-remote-bind-e2e (medium): Optional regression coverage for dashboard forwarding and remote dashboard reachability; adjacent to dashboard port behavior but primarily covers bind address rather than port resolution.

New E2E recommendations

  • onboarding-dashboard-port-precedence (high): Existing E2E has indirect CHAT_UI_URL and multi-sandbox coverage, but no focused live test that proves the full precedence order --control-ui-port > CHAT_UI_URL > registry > agent.forwardPort > default, nor malformed CHAT_UI_URL behavior in a real onboard command.
    • Suggested test: Add a focused E2E that runs real nemoclaw onboard with explicit --control-ui-port and CHAT_UI_URL combinations, then verifies OpenShell forward list, NemoClaw registry dashboardPort, generated dashboard URL, and readiness probe all use the same effective port.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: cloud-onboard-e2e,sandbox-operations-e2e,concurrent-gateway-ports-e2e

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: ubuntu-repo-cloud-openclaw
Optional Vitest E2E scenarios: ubuntu-repo-docker-post-reboot-recovery

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • ubuntu-repo-cloud-openclaw: Changes alter sandbox creation dashboard port resolution and CHAT_UI_URL/forward port selection in the main onboarding path. The live-supported Ubuntu repo cloud OpenClaw scenario exercises sandbox creation with the standard Docker runtime.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional Vitest E2E scenarios

  • ubuntu-repo-docker-post-reboot-recovery: Optional adjacent coverage: this live-supported lifecycle scenario also performs cloud OpenClaw onboarding before post-reboot recovery checks, so it can provide additional confidence that dashboard port allocation remains compatible with lifecycle recovery flows.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-docker-post-reboot-recovery

Relevant changed files

  • src/lib/onboard.ts
  • src/lib/onboard/dashboard-port.ts

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27267579798
Target ref: codex/onboard-dashboard-port-flow
Requested jobs: concurrent-gateway-ports-e2e,double-onboard-e2e
Summary: 2 passed, 0 failed, 0 skipped

Job Result
concurrent-gateway-ports-e2e ✅ success
double-onboard-e2e ✅ success

@wscurran wscurran added area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change labels Jun 10, 2026
@wscurran

Copy link
Copy Markdown
Contributor

@cv cv added the v0.0.64 label Jun 10, 2026
Base automatically changed from codex/onboard-gateway-helper-flow to main June 11, 2026 06:25
@cv
cv marked this pull request as ready for review June 11, 2026 07:45
@cv
cv merged commit c27d8fa into main Jun 11, 2026
39 checks passed
@cv
cv deleted the codex/onboard-dashboard-port-flow branch June 11, 2026 07:55
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 27332001138
Target ref: f959cccf6410b1c77c55d54a2482b878aa462a13
Workflow ref: main
Requested jobs: cloud-onboard-e2e,sandbox-operations-e2e,concurrent-gateway-ports-e2e
Summary: 3 passed, 0 failed, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
concurrent-gateway-ports-e2e ✅ success
sandbox-operations-e2e ✅ success

cv added a commit that referenced this pull request Jun 11, 2026
## Summary
Centralizes the reused-sandbox dashboard update path so interactive and
non-interactive sandbox reuse share the same forward, CHAT_UI_URL,
Hermes dashboard, and registry metadata handling. This draft PR is
stacked on #5128 and keeps behavior unchanged while removing duplicated
createSandbox logic.

## Related Issue
Refs #3802

## Changes
- Added applyReusedSandboxDashboardState to
`src/lib/onboard/sandbox-reuse.ts` for reused dashboard forward and
metadata updates.
- Replaced the duplicated non-interactive and interactive reuse branches
in `createSandbox` with the shared helper.
- Added focused coverage for CHAT_UI_URL updates, Hermes dashboard
forwarding, reused metadata updates, and gateway registry fields.

## Type of Change
- [x] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Verification
Targeted checks run:
- npx @biomejs/biome lint src/lib/onboard.ts
src/lib/onboard/sandbox-reuse.ts src/lib/onboard/sandbox-reuse.test.ts
- npm run typecheck:cli
- npm run build:cli
- git diff --check
- npx vitest run src/lib/onboard/sandbox-reuse.test.ts
src/lib/onboard/sandbox-registry-metadata.test.ts test/onboard.test.ts
test/shellquote-sandbox.test.ts

GitHub validation:
- PR check suite: passing after commit `2d9d81a84`
- PR Review Advisor: passing, 0 needs attention / 0 worth checking / 0
nice ideas
- CodeRabbit: skipped while draft
- Required live E2E: `double-onboard-e2e` and `token-rotation-e2e`
passed in https://github.com/NVIDIA/NemoClaw/actions/runs/27269640744
- Required scenario E2E: `ubuntu-repo-cloud-openclaw` passed in
https://github.com/NVIDIA/NemoClaw/actions/runs/27269640556

- [ ] `npx prek run --all-files` passes
- [ ] `npm test` passes
- [x] Tests added or updated for new or changed behavior
- [x] No secrets, API keys, or credentials committed
- [ ] Docs updated for user-facing behavior changes
- [ ] `npm run docs` builds without warnings (doc changes only)
- [ ] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
Signed-off-by: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Refactor**
* Improved sandbox creation dashboard port allocation logic for more
reliable port resolution and URL handling.
* Enhanced sandbox reuse flow to better manage dashboard state and port
forwarding across reused sandbox instances.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants