From 4a49ea89e43cdfeb09bd71cf3c50d71780e7b502 Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Wed, 22 Apr 2026 01:57:44 +0000 Subject: [PATCH 1/2] fix(onboard): format dashboard-port conflict as CLI error, not stack trace (closes #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 --- src/lib/onboard.ts | 42 +++++++++++++++++++++++++++--------------- test/onboard.test.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 0fd7a537f30..52b9ef7eebc 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -6000,6 +6000,21 @@ const CONTROL_UI_PORT = DASHBOARD_PORT; // isLoopbackHostname — see urlUtils import above const { resolveDashboardForwardTarget, buildControlUiUrls } = dashboard; +// Parses `openshell forward list` output and returns the sandbox currently +// owning `portToStop`, or null. Exported for unit testing — see #2169. +// Columns: SANDBOX BIND PORT PID STATUS (whitespace-separated). +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; +} + function ensureDashboardForward(sandboxName, chatUiUrl = `http://127.0.0.1:${CONTROL_UI_PORT}`) { const portToStop = getDashboardForwardPort(chatUiUrl); const forwardTarget = getDashboardForwardTarget(chatUiUrl); @@ -6007,23 +6022,19 @@ function ensureDashboardForward(sandboxName, chatUiUrl = `http://127.0.0.1:${CON // actionable message rather than silently stealing that sandbox's forward. // (Same sandbox is always allowed — covers reconnect and resume paths.) const existingForwards = runCaptureOpenshell(["forward", "list"], { ignoreError: true }); - // Parse line-by-line to avoid false positives from substring matches. - // openshell forward list columns: SANDBOX BIND PORT PID STATUS - // Port is at column index 2; sandbox name is at column index 0. - const portLine = existingForwards - ?.split("\n") - .map((l) => l.trim()) - .find((l) => { - const parts = l.split(/\s+/); - return parts[2] === portToStop; - }); - const portOwner = portLine ? (portLine.split(/\s+/)[0] ?? null) : null; + const portOwner = findDashboardForwardOwner(existingForwards, portToStop); if (portOwner !== null && portOwner !== sandboxName) { - throw new Error( - `Port ${portToStop} is already forwarded for sandbox '${portOwner}'. ` + - `Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790) ` + - `before onboarding a second sandbox.`, + // Match the preflight pattern (printed error + exit) instead of throwing, + // so the user sees a clean message rather than a raw Node stack trace + // from the top-level IIFE's unhandled rejection. See #2169. + console.error( + ` Port ${portToStop} is already forwarded for sandbox '${portOwner}'.`, + ); + console.error( + ` Set CHAT_UI_URL to a different local port (e.g. http://127.0.0.1:18790)`, ); + console.error(` before onboarding a second sandbox.`); + process.exit(1); } runOpenshell(["forward", "stop", portToStop], { ignoreError: true }); // Use stdio "ignore" to prevent spawnSync from waiting on inherited pipe fds. @@ -6829,6 +6840,7 @@ module.exports = { getDashboardForwardPort, getDashboardForwardStartCommand, getDashboardGuidanceLines, + findDashboardForwardOwner, startGatewayForRecovery, runCaptureOpenshell, setupInference, diff --git a/test/onboard.test.ts b/test/onboard.test.ts index ebfe0e645d0..91a4b787224 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -5364,4 +5364,32 @@ const { createSandbox } = require(${onboardPath}); "pullAndResolveBaseImageDigest must be called BEFORE patchStagedDockerfile — regression #1904", ); }); + + it("findDashboardForwardOwner parses openshell forward list column format (#2169)", () => { + const repoRoot = path.join(import.meta.dirname, ".."); + const onboardPath = path.join(repoRoot, "dist", "lib", "onboard.js"); + delete require.cache[onboardPath]; + const { findDashboardForwardOwner } = require(onboardPath); + + // Canonical openshell forward list output: SANDBOX BIND PORT PID STATUS + const forwardList = [ + "SANDBOX BIND PORT PID STATUS", + "test21 127.0.0.1 18789 42101 active", + "other 127.0.0.1 18790 42102 active", + ].join("\n"); + + // Port in use by another sandbox → return that sandbox's name + assert.equal(findDashboardForwardOwner(forwardList, "18789"), "test21"); + assert.equal(findDashboardForwardOwner(forwardList, "18790"), "other"); + // Port not in the list → null + assert.equal(findDashboardForwardOwner(forwardList, "18791"), null); + // Empty / missing input → null (no false positives) + assert.equal(findDashboardForwardOwner("", "18789"), null); + assert.equal(findDashboardForwardOwner(null, "18789"), null); + assert.equal(findDashboardForwardOwner(undefined, "18789"), null); + // Port string appearing as a substring somewhere other than column 2 must NOT + // match — guard against false-positive substring matches. + const falsePositive = "sandbox18789 127.0.0.1 42001 9999 active"; + assert.equal(findDashboardForwardOwner(falsePositive, "18789"), null); + }); }); From ea86d3cc62077beee82db26c5f54bae896231baa Mon Sep 17 00:00:00 2001 From: latenighthackathon Date: Wed, 22 Apr 2026 02:47:53 +0000 Subject: [PATCH 2/2] test(onboard): static ESM import for findDashboardForwardOwner (#2220 CR) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review on #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 "#2169" still passes (1/1). Signed-off-by: latenighthackathon --- test/onboard.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/onboard.test.ts b/test/onboard.test.ts index 91a4b787224..1462c658698 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -50,6 +50,7 @@ import { summarizeProbeFailure, shouldIncludeBuildContextPath, writeSandboxConfigSyncFile, + findDashboardForwardOwner, } from "../dist/lib/onboard"; import { stageOptimizedSandboxBuildContext } from "../dist/lib/sandbox-build-context"; import { buildWebSearchDockerConfig } from "../dist/lib/web-search"; @@ -5366,11 +5367,6 @@ const { createSandbox } = require(${onboardPath}); }); it("findDashboardForwardOwner parses openshell forward list column format (#2169)", () => { - const repoRoot = path.join(import.meta.dirname, ".."); - const onboardPath = path.join(repoRoot, "dist", "lib", "onboard.js"); - delete require.cache[onboardPath]; - const { findDashboardForwardOwner } = require(onboardPath); - // Canonical openshell forward list output: SANDBOX BIND PORT PID STATUS const forwardList = [ "SANDBOX BIND PORT PID STATUS",