fix(onboard): format dashboard-port conflict as CLI error, not stack trace - #2220
Conversation
📝 WalkthroughWalkthroughIntroduces exported helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
6006-6016: HardenfindDashboardForwardOwnerparsing against non-table lines.Current parsing reads
parts[2]from every trimmed line. Ifopenshell forward listincludes non-table output, this can false-match and incorrectly block onboarding. Consider validating row shape (and header skip) likesrc/lib/sandbox-session-state.ts:59-87.Suggested patch
function findDashboardForwardOwner(forwardListOutput, portToStop) { - if (!forwardListOutput) return null; - const portLine = forwardListOutput - .split("\n") - .map((l) => l.trim()) - .find((l) => { - const parts = l.split(/\s+/); - return parts[2] === portToStop; - }); - return portLine ? (portLine.split(/\s+/)[0] ?? null) : null; + if (!forwardListOutput || portToStop === null || portToStop === undefined) return null; + const targetPort = String(portToStop); + const lines = String(forwardListOutput) + .split("\n") + .map((l) => l.trim()) + .filter(Boolean); + + for (const line of lines) { + if (/^\s*SANDBOX\s/i.test(line)) continue; + const parts = line.split(/\s+/); + if (parts.length < 4) continue; + if (parts[2] === targetPort && /^\d+$/.test(parts[3])) { + return parts[0] ?? null; + } + } + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 6006 - 6016, The function findDashboardForwardOwner currently indexes parts[2] on every trimmed line which can mis-parse non-table or header lines; update it to first skip empty lines and known header lines, then split each line and validate the row shape (e.g., require parts.length >= 3 and that parts[2] matches the expected port format or pattern) before comparing to portToStop, and only then return parts[0] as the owner; reference the function name findDashboardForwardOwner and variables forwardListOutput and portToStop when making the change and follow the header/row-shape checks used in sandbox-session-state.ts as a model.
🤖 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 5369-5373: Replace the CommonJS require/require.cache usage with
ESM dynamic import: compute the onboard module file URL from import.meta.url
(use pathToFileURL on the computed onboardPath) and await import(...) to load
it, using a cache-busting query string (e.g., ?t=Date.now()) if you need to
reload the module during tests; then destructure findDashboardForwardOwner from
the imported module. Remove any references to require.cache and require; use
import(...) with the file:// URL built from repoRoot/onboardPath and
import.meta.url instead to satisfy ESM test rules.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 6006-6016: The function findDashboardForwardOwner currently
indexes parts[2] on every trimmed line which can mis-parse non-table or header
lines; update it to first skip empty lines and known header lines, then split
each line and validate the row shape (e.g., require parts.length >= 3 and that
parts[2] matches the expected port format or pattern) before comparing to
portToStop, and only then return parts[0] as the owner; reference the function
name findDashboardForwardOwner and variables forwardListOutput and portToStop
when making the change and follow the header/row-shape checks used in
sandbox-session-state.ts as a model.
🪄 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: dccb482a-19f5-4821-8a89-ed1f6c8d600e
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
…A#2220 CR) Address CodeRabbit review on NVIDIA#2220: > Use ESM loading instead of require in test/ TypeScript files. This > test uses CommonJS module loading (require/require.cache), which > violates the test ESM rule. Promote findDashboardForwardOwner to the top-of-file static import list and drop the require/require.cache block. The regex parser is pure, so cache-busting via `await import(url + '?t=...')` isn't needed — a static import keeps the test simple and matches the ESM convention documented in AGENTS.md. Tests - test/onboard.test.ts -t "NVIDIA#2169" still passes (1/1). Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
Applied CodeRabbit's ESM suggestion — promoted |
…trace (closes NVIDIA#2169) When a user runs `nemoclaw onboard` for a second sandbox while the first sandbox already forwards port 18789, ensureDashboardForward() threw a raw Error. The top-level IIFE in nemoclaw.ts has no catch, so the user saw a Node unhandled-rejection stack trace from onboard.js:6022 instead of a clean preflight-style message. Match the established preflight pattern (console.error + process.exit(1)) so the output is: Port 18789 is already forwarded for sandbox 'test21'. Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790) before onboarding a second sandbox. Extract the forward-list column parsing into a pure helper findDashboardForwardOwner() so the parse logic is directly unit-testable without exercising the process-exit path. Export it for the new test. Tests - test/onboard.test.ts: +1 new case covering canonical forward-list format, port-in-list (match), port-not-in-list (null), empty/null/ undefined inputs (null), and a false-positive substring guard. - Full suite: 134 tests pass (was 133 before this change). Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
…A#2220 CR) Address CodeRabbit review on NVIDIA#2220: > Use ESM loading instead of require in test/ TypeScript files. This > test uses CommonJS module loading (require/require.cache), which > violates the test ESM rule. Promote findDashboardForwardOwner to the top-of-file static import list and drop the require/require.cache block. The regex parser is pure, so cache-busting via `await import(url + '?t=...')` isn't needed — a static import keeps the test simple and matches the ESM convention documented in AGENTS.md. Tests - test/onboard.test.ts -t "NVIDIA#2169" still passes (1/1). Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
32cc9c8 to
ea86d3c
Compare
|
Rebased onto current |
|
✨ Thanks for submitting this PR that proposes a fix for the dashboard-port conflict error — this could help provide a cleaner error message for users. Related open issues: |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Clean fix. throw new Error → console.error + process.exit(1) matches the established preflight pattern. The findDashboardForwardOwner extraction is good — pure helper, directly testable, column-based parsing avoids substring false positives. Test coverage is solid.
LGTM.
…IDIA#2221 tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Merge conflicts resolved. The contributor's fork has branch protection that prevents direct pushes, so the resolved branch is at @latenighthackathon — to pick up the resolution: Both your |
|
@ericksoa thanks for resolving the conflicts and walking me through the pickup — fetched Also loosened the fork ruleset to exempt non-default branches from the admin-only rule, so direct maintainer pushes (not just the Update Branch merge button) should work on future PRs without needing the detour. Cheers! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 6150-6152: The error message currently prints a hardcoded example
port; update the console.error call in src/lib/onboard.ts (the console.error
that suggests "Set CHAT_UI_URL ... e.g. http://127.0.0.1:18790") to avoid
hardcoding 18790 — instead derive the suggested port dynamically from the
existing portToStop variable (e.g., suggest portToStop + 1) or use a generic
placeholder (e.g., "http://127.0.0.1:<port>") so users are not pointed back to
the conflicting port; modify the console.error invocation to interpolate the
computed port or placeholder accordingly.
🪄 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: Enterprise
Run ID: 0ac2e71b-dce1-475f-92ea-3366ff9f0e0f
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
| console.error( | ||
| ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`, | ||
| ); |
There was a problem hiding this comment.
Avoid hardcoding the example port in the conflict message.
If the user already set CHAT_UI_URL to 18790 and that port conflicts, this message still suggests http://127.0.0.1:18790, which points them back to the failing port. Derive the example from portToStop or make it generic.
💡 Proposed fix
- console.error(
- ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`,
- );
+ const examplePort = Number.isFinite(Number(portToStop))
+ ? String(Number(portToStop) + 1)
+ : "18790";
+ console.error(
+ ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:${examplePort})`,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error( | |
| ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`, | |
| ); | |
| const examplePort = Number.isFinite(Number(portToStop)) | |
| ? String(Number(portToStop) + 1) | |
| : "18790"; | |
| console.error( | |
| ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:${examplePort})`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/onboard.ts` around lines 6150 - 6152, The error message currently
prints a hardcoded example port; update the console.error call in
src/lib/onboard.ts (the console.error that suggests "Set CHAT_UI_URL ... e.g.
http://127.0.0.1:18790") to avoid hardcoding 18790 — instead derive the
suggested port dynamically from the existing portToStop variable (e.g., suggest
portToStop + 1) or use a generic placeholder (e.g., "http://127.0.0.1:<port>")
so users are not pointed back to the conflicting port; modify the console.error
invocation to interpolate the computed port or placeholder accordingly.
…trace (NVIDIA#2220) ## Summary When a user runs `nemoclaw onboard` for a second sandbox while the first sandbox already forwards port 18789, `ensureDashboardForward()` threw a raw `Error`. The top-level IIFE in `nemoclaw.ts` has no catch, so the user saw a Node unhandled-rejection stack trace originating at `onboard.js:6022` instead of a clean preflight-style message. This PR matches the established preflight pattern (`console.error` + `process.exit(1)`), so the user now sees: ``` Port 18789 is already forwarded for sandbox 'test21'. Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790) before onboarding a second sandbox. ``` ## Related Issue Closes NVIDIA#2169 ## Changes - **`src/lib/onboard.ts`** — replaced `throw new Error(...)` in `ensureDashboardForward()` with the standard `console.error(...)` + `process.exit(1)` pattern used by every other user-facing preflight failure in this file (e.g. missing Docker, openshell version gate). - Extracted the forward-list column parsing into a pure `findDashboardForwardOwner(output, port)` helper so the parse logic is directly unit-testable without exercising the exit path. Exported it. ## Testing - [x] `npx vitest run test/onboard.test.ts` — 134 tests pass (+1 new for `findDashboardForwardOwner`) - [x] `npm run build:cli` + `npm run typecheck:cli` clean Executed: - New test case covers: canonical column format match, port-not-in-list → `null`, empty/`null`/`undefined` inputs → `null`, and a false-positive substring guard (port number appearing inside a sandbox name). ## Checklist - [x] Follows [Conventional Commits](https://www.conventionalcommits.org/) - [x] Commit is signed (SSH) - [x] DCO Signed-off-by trailer present Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved onboarding port-conflict handling: conflicts now emit a clear, multi-line message and exit cleanly, avoiding raw stack traces. * More accurate detection of which sandbox owns a forwarded port to prevent false positives in port resolution. * **Tests** * Added regression tests covering forwarded-port parsing and related onboarding scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wscurran
added
area: cli
4 participants
Summary
When a user runs
nemoclaw onboardfor a second sandbox while the first sandbox already forwards port 18789,ensureDashboardForward()threw a rawError. The top-level IIFE innemoclaw.tshas no catch, so the user saw a Node unhandled-rejection stack trace originating atonboard.js:6022instead of a clean preflight-style message.This PR matches the established preflight pattern (
console.error+process.exit(1)), so the user now sees:Related Issue
Closes #2169
Changes
src/lib/onboard.ts— replacedthrow new Error(...)inensureDashboardForward()with the standardconsole.error(...)+process.exit(1)pattern used by every other user-facing preflight failure in this file (e.g. missing Docker, openshell version gate).findDashboardForwardOwner(output, port)helper so the parse logic is directly unit-testable without exercising the exit path. Exported it.Testing
npx vitest run test/onboard.test.ts— 134 tests pass (+1 new forfindDashboardForwardOwner)npm run build:cli+npm run typecheck:clicleanExecuted:
null, empty/null/undefinedinputs →null, and a false-positive substring guard (port number appearing inside a sandbox name).Checklist
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests