From d2e831032c158efd24a6c3749d30351162d805bb Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 20 Apr 2026 12:45:11 +0800 Subject: [PATCH 1/4] fix(#2063): use agent forwardPort in dashboard wait and guard ensureDashboardForward against port collision When onboarding a non-OpenClaw agent (e.g. Hermes, forwardPort=8642) next to an existing OpenClaw sandbox (forwardPort=18789), the dashboard readiness loop probed the wrong port (hardcoded DASHBOARD_PORT=18789) inside the Hermes sandbox. This caused a spurious 28-second wait and a misleading "Dashboard taking longer than expected" warning on every Hermes onboard, masking the real health state. Additionally ensureDashboardForward had no guard against a port already forwarded by a different sandbox. If two agents share the same forward port the first sandbox's forward would be silently stolen, leaving it unreachable. - Replace hardcoded DASHBOARD_PORT with effectivePort (already computed as agent.forwardPort for non-OpenClaw agents) in the readiness wait loop - Add a pre-check in ensureDashboardForward: inspect `openshell forward list` before issuing stop/start; throw a clear actionable error if the port is claimed by a different sandbox - Add gateway-state.test.ts (20 tests) covering isGatewayHealthy, getGatewayReuseState, isSandboxReady, hasStaleGateway, and related pure classifiers Fixes #2063 Signed-off-by: Dongni Yang Co-Authored-By: Claude Sonnet 4.6 --- src/lib/gateway-state.test.ts | 129 ++++++++++++++++++++++++++++++++++ src/lib/onboard.ts | 13 +++- 2 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 src/lib/gateway-state.test.ts diff --git a/src/lib/gateway-state.test.ts b/src/lib/gateway-state.test.ts new file mode 100644 index 00000000000..2a8841eec54 --- /dev/null +++ b/src/lib/gateway-state.test.ts @@ -0,0 +1,129 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, it, expect } from "vitest"; +import { + isGatewayHealthy, + getGatewayReuseState, + isSandboxReady, + hasStaleGateway, + getReportedGatewayName, + isGatewayConnected, +} from "../../dist/lib/gateway-state"; + +// Realistic openshell CLI output fixtures +const STATUS_CONNECTED_NEMOCLAW = " Status: Connected\n Gateway: nemoclaw\n"; +const GW_INFO_NEMOCLAW = " Gateway: nemoclaw\n Status: Running\n"; +const ACTIVE_INFO_NEMOCLAW = " Gateway: nemoclaw\n Gateway endpoint: http://localhost:8080\n"; + +const STATUS_CONNECTED_OTHER = " Status: Connected\n Gateway: other-gateway\n"; +const GW_INFO_NO_METADATA = "No gateway metadata found\n Gateway: nemoclaw\n"; +const STATUS_DISCONNECTED = " Status: Disconnected\n Gateway: nemoclaw\n"; + +describe("isGatewayHealthy", () => { + it("returns true when gateway is running — second onboard should reuse, not restart", () => { + expect(isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( + true, + ); + }); + + it("returns false when not connected", () => { + expect(isGatewayHealthy(STATUS_DISCONNECTED, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( + false, + ); + }); + + it("returns false when gateway metadata is absent", () => { + expect(isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, "", ACTIVE_INFO_NEMOCLAW)).toBe(false); + }); + + it("returns false when active gateway is a foreign name", () => { + expect(isGatewayHealthy(STATUS_CONNECTED_OTHER, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( + false, + ); + }); + + it("returns false when gwInfo contains no-metadata marker", () => { + expect( + isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NO_METADATA, ACTIVE_INFO_NEMOCLAW), + ).toBe(false); + }); +}); + +describe("getGatewayReuseState", () => { + it("returns healthy for a fully running gateway", () => { + expect(getGatewayReuseState(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( + "healthy", + ); + }); + + it("returns foreign-active when a different gateway is active", () => { + expect(getGatewayReuseState(STATUS_CONNECTED_OTHER, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( + "foreign-active", + ); + }); + + it("returns stale when gateway metadata exists but is disconnected", () => { + expect(getGatewayReuseState(STATUS_DISCONNECTED, GW_INFO_NEMOCLAW, "")).toBe("stale"); + }); + + it("returns missing when no state is present", () => { + expect(getGatewayReuseState("", "", "")).toBe("missing"); + }); +}); + +describe("hasStaleGateway", () => { + it("returns true for known gateway info", () => { + expect(hasStaleGateway(GW_INFO_NEMOCLAW)).toBe(true); + }); + + it("returns false when no-metadata marker is present", () => { + expect(hasStaleGateway(GW_INFO_NO_METADATA)).toBe(false); + }); + + it("returns false for empty string", () => { + expect(hasStaleGateway("")).toBe(false); + }); +}); + +describe("getReportedGatewayName", () => { + it("parses gateway name from status output", () => { + expect(getReportedGatewayName(STATUS_CONNECTED_NEMOCLAW)).toBe("nemoclaw"); + }); + + it("returns null for empty output", () => { + expect(getReportedGatewayName("")).toBeNull(); + }); +}); + +describe("isGatewayConnected", () => { + it("returns true when Connected is present", () => { + expect(isGatewayConnected(STATUS_CONNECTED_NEMOCLAW)).toBe(true); + }); + + it("returns false when Disconnected", () => { + expect(isGatewayConnected(STATUS_DISCONNECTED)).toBe(false); + }); +}); + +describe("isSandboxReady", () => { + it("returns true for a Ready sandbox", () => { + const output = " my-sandbox Ready running\n"; + expect(isSandboxReady(output, "my-sandbox")).toBe(true); + }); + + it("returns false for NotReady", () => { + const output = " my-sandbox NotReady pending\n"; + expect(isSandboxReady(output, "my-sandbox")).toBe(false); + }); + + it("returns false for a different sandbox name", () => { + const output = " other-sandbox Ready running\n"; + expect(isSandboxReady(output, "my-sandbox")).toBe(false); + }); + + it("strips ANSI codes before matching", () => { + const output = "\x1b[32m my-sandbox\x1b[0m Ready running\n"; + expect(isSandboxReady(output, "my-sandbox")).toBe(true); + }); +}); diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index caea888140b..cf2a0d8d602 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -2869,7 +2869,7 @@ async function createSandbox( console.log(" Waiting for NemoClaw dashboard to become ready..."); for (let i = 0; i < 15; i++) { const readyMatch = runCapture( - `openshell sandbox exec ${shellQuote(sandboxName)} curl -sf http://localhost:${DASHBOARD_PORT}/ 2>/dev/null || echo "no"`, + `openshell sandbox exec ${shellQuote(sandboxName)} curl -sf http://localhost:${effectivePort}/ 2>/dev/null || echo "no"`, { ignoreError: true }, ); if (readyMatch && !readyMatch.includes("no")) { @@ -4730,6 +4730,17 @@ const { resolveDashboardForwardTarget, buildControlUiUrls } = dashboard; function ensureDashboardForward(sandboxName, chatUiUrl = `http://127.0.0.1:${CONTROL_UI_PORT}`) { const portToStop = getDashboardForwardPort(chatUiUrl); const forwardTarget = getDashboardForwardTarget(chatUiUrl); + // Detect port already claimed by a different sandbox and fail fast with an + // 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 }); + if (existingForwards?.includes(`:${portToStop}`) && !existingForwards?.includes(sandboxName)) { + throw new Error( + `Port ${portToStop} is already forwarded for another sandbox. ` + + `Set NEMOCLAW_DASHBOARD_PORT to a different port before onboarding ` + + `a second sandbox.`, + ); + } runOpenshell(["forward", "stop", portToStop], { ignoreError: true }); // Use stdio "ignore" to prevent spawnSync from waiting on inherited pipe fds. // The --background flag forks a child that inherits stdout/stderr; if those are From bcaa37fcc64cac091eaf0518374790816d3c380e Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 20 Apr 2026 13:45:57 +0800 Subject: [PATCH 2/4] =?UTF-8?q?fix(onboard):=20address=20CodeRabbit=20revi?= =?UTF-8?q?ew=20=E2=80=94=20line-level=20forward=20collision=20check=20and?= =?UTF-8?q?=20test=20mock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ensureDashboardForward: replace blob substring match with line-level parser so the collision check is scoped to the exact port entry (format: " -> :") rather than the full listing - Error message now references CHAT_UI_URL instead of NEMOCLAW_DASHBOARD_PORT — the env var only applies to OpenClaw default port; agents with explicit forwardPort in their manifest need CHAT_UI_URL to override the local listen address - test/onboard.test.ts: fix runCapture mock to match openshellShellCommand output ('\''sandbox'\'' '\''exec'\'') so the dashboard-ready loop exits on the first iteration instead of sleeping 28 s and timing out the test - Remove duplicate src/lib/gateway-state.test.ts (superseded by the existing test/gateway-state.test.ts which covers the same functions with broader scenarios) Signed-off-by: Dongni Yang Co-Authored-By: Claude Sonnet 4.6 --- src/lib/gateway-state.test.ts | 129 ---------------------------------- src/lib/onboard.ts | 18 +++-- test/onboard.test.ts | 2 +- 3 files changed, 15 insertions(+), 134 deletions(-) delete mode 100644 src/lib/gateway-state.test.ts diff --git a/src/lib/gateway-state.test.ts b/src/lib/gateway-state.test.ts deleted file mode 100644 index 2a8841eec54..00000000000 --- a/src/lib/gateway-state.test.ts +++ /dev/null @@ -1,129 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { describe, it, expect } from "vitest"; -import { - isGatewayHealthy, - getGatewayReuseState, - isSandboxReady, - hasStaleGateway, - getReportedGatewayName, - isGatewayConnected, -} from "../../dist/lib/gateway-state"; - -// Realistic openshell CLI output fixtures -const STATUS_CONNECTED_NEMOCLAW = " Status: Connected\n Gateway: nemoclaw\n"; -const GW_INFO_NEMOCLAW = " Gateway: nemoclaw\n Status: Running\n"; -const ACTIVE_INFO_NEMOCLAW = " Gateway: nemoclaw\n Gateway endpoint: http://localhost:8080\n"; - -const STATUS_CONNECTED_OTHER = " Status: Connected\n Gateway: other-gateway\n"; -const GW_INFO_NO_METADATA = "No gateway metadata found\n Gateway: nemoclaw\n"; -const STATUS_DISCONNECTED = " Status: Disconnected\n Gateway: nemoclaw\n"; - -describe("isGatewayHealthy", () => { - it("returns true when gateway is running — second onboard should reuse, not restart", () => { - expect(isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( - true, - ); - }); - - it("returns false when not connected", () => { - expect(isGatewayHealthy(STATUS_DISCONNECTED, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( - false, - ); - }); - - it("returns false when gateway metadata is absent", () => { - expect(isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, "", ACTIVE_INFO_NEMOCLAW)).toBe(false); - }); - - it("returns false when active gateway is a foreign name", () => { - expect(isGatewayHealthy(STATUS_CONNECTED_OTHER, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( - false, - ); - }); - - it("returns false when gwInfo contains no-metadata marker", () => { - expect( - isGatewayHealthy(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NO_METADATA, ACTIVE_INFO_NEMOCLAW), - ).toBe(false); - }); -}); - -describe("getGatewayReuseState", () => { - it("returns healthy for a fully running gateway", () => { - expect(getGatewayReuseState(STATUS_CONNECTED_NEMOCLAW, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( - "healthy", - ); - }); - - it("returns foreign-active when a different gateway is active", () => { - expect(getGatewayReuseState(STATUS_CONNECTED_OTHER, GW_INFO_NEMOCLAW, ACTIVE_INFO_NEMOCLAW)).toBe( - "foreign-active", - ); - }); - - it("returns stale when gateway metadata exists but is disconnected", () => { - expect(getGatewayReuseState(STATUS_DISCONNECTED, GW_INFO_NEMOCLAW, "")).toBe("stale"); - }); - - it("returns missing when no state is present", () => { - expect(getGatewayReuseState("", "", "")).toBe("missing"); - }); -}); - -describe("hasStaleGateway", () => { - it("returns true for known gateway info", () => { - expect(hasStaleGateway(GW_INFO_NEMOCLAW)).toBe(true); - }); - - it("returns false when no-metadata marker is present", () => { - expect(hasStaleGateway(GW_INFO_NO_METADATA)).toBe(false); - }); - - it("returns false for empty string", () => { - expect(hasStaleGateway("")).toBe(false); - }); -}); - -describe("getReportedGatewayName", () => { - it("parses gateway name from status output", () => { - expect(getReportedGatewayName(STATUS_CONNECTED_NEMOCLAW)).toBe("nemoclaw"); - }); - - it("returns null for empty output", () => { - expect(getReportedGatewayName("")).toBeNull(); - }); -}); - -describe("isGatewayConnected", () => { - it("returns true when Connected is present", () => { - expect(isGatewayConnected(STATUS_CONNECTED_NEMOCLAW)).toBe(true); - }); - - it("returns false when Disconnected", () => { - expect(isGatewayConnected(STATUS_DISCONNECTED)).toBe(false); - }); -}); - -describe("isSandboxReady", () => { - it("returns true for a Ready sandbox", () => { - const output = " my-sandbox Ready running\n"; - expect(isSandboxReady(output, "my-sandbox")).toBe(true); - }); - - it("returns false for NotReady", () => { - const output = " my-sandbox NotReady pending\n"; - expect(isSandboxReady(output, "my-sandbox")).toBe(false); - }); - - it("returns false for a different sandbox name", () => { - const output = " other-sandbox Ready running\n"; - expect(isSandboxReady(output, "my-sandbox")).toBe(false); - }); - - it("strips ANSI codes before matching", () => { - const output = "\x1b[32m my-sandbox\x1b[0m Ready running\n"; - expect(isSandboxReady(output, "my-sandbox")).toBe(true); - }); -}); diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 718da9a7310..4fdc92ea115 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -5562,11 +5562,21 @@ 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 }); - if (existingForwards?.includes(`:${portToStop}`) && !existingForwards?.includes(sandboxName)) { + // Parse line-by-line to avoid false positives from substring matches. + // Each line has the format: " -> :" + const portLine = existingForwards + ?.split("\n") + .map((l) => l.trim()) + .find((l) => { + const localPort = l.split(/\s+/)[0]; + return localPort === portToStop; + }); + const portOwner = portLine ? (portLine.split(/\s+/)[2]?.split(":")?.[0] ?? null) : null; + if (portOwner !== null && portOwner !== sandboxName) { throw new Error( - `Port ${portToStop} is already forwarded for another sandbox. ` + - `Set NEMOCLAW_DASHBOARD_PORT to a different port before onboarding ` + - `a second sandbox.`, + `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.`, ); } runOpenshell(["forward", "stop", portToStop], { ignoreError: true }); diff --git a/test/onboard.test.ts b/test/onboard.test.ts index 185559e7be5..1c466ecb642 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -3293,7 +3293,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return "my-assistant"; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'forward' 'list'")) return ""; - if (command.includes("sandbox exec") && command.includes("curl")) return "ok"; + if (command.includes("'sandbox' 'exec'") && command.includes("curl")) return "ok"; return ""; }; From cf1c597a468ef99fc63fd717fbb2d9325226ce08 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 20 Apr 2026 15:49:09 +0800 Subject: [PATCH 3/4] fix(onboard): fix forward list parser column indices and update test mocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openshell forward list outputs SANDBOX BIND PORT PID STATUS columns. The ensureDashboardForward parser was reading column [0] (SANDBOX name) as the port and column [2] with colon-split for the owner, which never matched any real output — silently disabling the collision guard. Fix: read PORT from column [2] and SANDBOX name from column [0]. Update all test mocks from the fictional "PORT -> NAME:PORT" format to match the real openshell output: "NAME 127.0.0.1 PORT PID STATUS". Regression introduced in this branch; related to open issue #2007. Signed-off-by: Dongni Yang --- src/lib/onboard.ts | 9 +++++---- test/onboard.test.ts | 22 +++++++++++----------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/lib/onboard.ts b/src/lib/onboard.ts index 4fdc92ea115..1e8cc78830d 100644 --- a/src/lib/onboard.ts +++ b/src/lib/onboard.ts @@ -5563,15 +5563,16 @@ function ensureDashboardForward(sandboxName, chatUiUrl = `http://127.0.0.1:${CON // (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. - // Each line has the format: " -> :" + // 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 localPort = l.split(/\s+/)[0]; - return localPort === portToStop; + const parts = l.split(/\s+/); + return parts[2] === portToStop; }); - const portOwner = portLine ? (portLine.split(/\s+/)[2]?.split(":")?.[0] ?? null) : null; + const portOwner = portLine ? (portLine.split(/\s+/)[0] ?? null) : null; if (portOwner !== null && portOwner !== sandboxName) { throw new Error( `Port ${portToStop} is already forwarded for sandbox '${portOwner}'. ` + diff --git a/test/onboard.test.ts b/test/onboard.test.ts index 1c466ecb642..e9b59f183bb 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -2418,7 +2418,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -2529,7 +2529,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -2627,7 +2627,7 @@ runner.runCapture = (command) => { if (normalized.includes("'sandbox' 'list'")) return "my-assistant Ready"; // Custom port: dashboard readiness curl uses 19000 (DASHBOARD_PORT from env) if (normalized.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:19000/'")) return "ok"; - if (normalized.includes("'forward' 'list'")) return "19000 -> my-assistant:19000"; + if (normalized.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 19000 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -2760,7 +2760,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'provider' 'get'")) return "Provider: discord-bridge"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; if (command.includes("'sandbox' 'exec'") && command.includes("'curl'")) return "ok"; return ""; }; @@ -3003,7 +3003,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; // All messaging providers already exist in gateway if (command.includes("'provider' 'get'")) return "Provider: exists"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.getSandbox = () => ({ name: "my-assistant", gpuEnabled: false }); @@ -3403,7 +3403,7 @@ runner.runFile = (file, args = [], opts = {}) => { runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return "my-assistant"; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.getSandbox = () => ({ name: "my-assistant", gpuEnabled: false }); @@ -4324,7 +4324,7 @@ runner.runCapture = (command) => { return sandboxListCalls >= 2 ? "my-assistant Ready" : "my-assistant Pending"; } if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -4436,7 +4436,7 @@ runner.runFile = (file, args = [], opts = {}) => { runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return "my-assistant"; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.getSandbox = () => ({ name: "my-assistant", gpuEnabled: false }); @@ -4725,7 +4725,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -4856,7 +4856,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; @@ -5132,7 +5132,7 @@ runner.runCapture = (command) => { if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; - if (command.includes("'forward' 'list'")) return "18789 -> my-assistant:18789"; + if (command.includes("'forward' 'list'")) return "my-assistant 127.0.0.1 18789 12345 running"; return ""; }; registry.registerSandbox = () => true; From 2ef0638d3c54894a416fe01b894b1cf839f67eff Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Mon, 20 Apr 2026 07:56:27 -0700 Subject: [PATCH 4/4] test(onboard): add coverage for forward port collision guard Exercises the ensureDashboardForward collision path added in this PR: mock `forward list` to return a different sandbox owning port 18789, then assert createSandbox throws with the expected error message. --- test/onboard.test.ts | 91 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/test/onboard.test.ts b/test/onboard.test.ts index e9b59f183bb..fb5eed8f420 100644 --- a/test/onboard.test.ts +++ b/test/onboard.test.ts @@ -2492,6 +2492,97 @@ const { createSandbox } = require(${onboardPath}); ); }); + it("rejects sandbox creation when the dashboard port is already forwarded for a different sandbox", async () => { + const repoRoot = path.join(import.meta.dirname, ".."); + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-forward-collision-")); + const fakeBin = path.join(tmpDir, "bin"); + const scriptPath = path.join(tmpDir, "forward-collision-check.js"); + const onboardPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "onboard.js")); + const runnerPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "runner.js")); + const registryPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "registry.js")); + const preflightPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "preflight.js")); + const credentialsPath = JSON.stringify(path.join(repoRoot, "dist", "lib", "credentials.js")); + + fs.mkdirSync(fakeBin, { recursive: true }); + fs.writeFileSync(path.join(fakeBin, "openshell"), "#!/usr/bin/env bash\nexit 0\n", { + mode: 0o755, + }); + + const script = String.raw` +const runner = require(${runnerPath}); +const registry = require(${registryPath}); +const preflight = require(${preflightPath}); +const credentials = require(${credentialsPath}); +const childProcess = require("node:child_process"); +const { EventEmitter } = require("node:events"); + +runner.run = (command, opts = {}) => { + return { status: 0 }; +}; +runner.runFile = (file, args = [], opts = {}) => { + return { status: 0 }; +}; +runner.runCapture = (command) => { + if (command.includes("'sandbox' 'get' 'my-assistant'")) return ""; + if (command.includes("'sandbox' 'list'")) return "my-assistant Ready"; + if (command.includes("'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'")) return "ok"; + // Port 18789 is already forwarded by a DIFFERENT sandbox (other-sandbox) + if (command.includes("'forward' 'list'")) return "other-sandbox 127.0.0.1 18789 99999 running"; + return ""; +}; +registry.registerSandbox = () => true; +registry.removeSandbox = () => true; +preflight.checkPortAvailable = async () => ({ ok: true }); +credentials.prompt = async () => ""; + +childProcess.spawn = (...args) => { + const child = new EventEmitter(); + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + process.nextTick(() => { + child.stdout.emit("data", Buffer.from("Created sandbox: my-assistant\n")); + child.emit("close", 0); + }); + return child; +}; + +const { createSandbox } = require(${onboardPath}); + +(async () => { + process.env.OPENSHELL_GATEWAY = "nemoclaw"; + await createSandbox(null, "gpt-5.4"); + // Should not reach here — the collision guard must throw. + console.log("ERROR_NO_THROW"); + process.exit(1); +})().catch((error) => { + console.log(JSON.stringify({ error: error.message })); +}); +`; + fs.writeFileSync(scriptPath, script); + + const result = spawnSync(process.execPath, [scriptPath], { + cwd: repoRoot, + encoding: "utf-8", + env: { + ...process.env, + HOME: tmpDir, + PATH: `${fakeBin}:${process.env.PATH || ""}`, + NEMOCLAW_NON_INTERACTIVE: "1", + }, + }); + + assert.ok(!result.stdout.includes("ERROR_NO_THROW"), "expected createSandbox to throw on port collision"); + const payloadLine = result.stdout + .trim() + .split("\n") + .slice() + .reverse() + .find((line) => line.startsWith("{") && line.endsWith("}")); + assert.ok(payloadLine, `expected JSON error payload in stdout:\n${result.stdout}`); + const payload = JSON.parse(payloadLine); + assert.match(payload.error, /Port 18789 is already forwarded for sandbox 'other-sandbox'/); + }); + it("binds the dashboard forward to 0.0.0.0 when CHAT_UI_URL points to a remote host", async () => { const repoRoot = path.join(import.meta.dirname, ".."); const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-remote-forward-"));