diff --git a/src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts b/src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts new file mode 100644 index 0000000000..9b77d1723a --- /dev/null +++ b/src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts @@ -0,0 +1,80 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { captureOpenshell, runOpenshell } from "../../adapters/openshell/runtime"; +import type { SandboxMessagingPlan } from "../../messaging/manifest"; +import { ensureMessagingHostForwardAfterRebuild } from "./messaging-host-forward-lifecycle"; + +vi.mock("../../adapters/openshell/runtime", () => ({ + captureOpenshell: vi.fn(), + getOpenshellBinary: vi.fn(() => "openshell"), + runOpenshell: vi.fn(() => ({ status: 0 })), +})); + +vi.mock("../../onboard/forward-start", () => ({ + buildDetachedForwardStartSpawn: vi.fn(() => vi.fn()), + buildForwardStartProgressLogger: vi.fn(() => vi.fn()), + runDetachedForwardStartWithRetries: vi.fn(() => ({ ok: true, diagnostic: "" })), +})); + +function makePlan(): SandboxMessagingPlan { + return { + schemaVersion: 1, + sandboxName: "demo", + agent: "openclaw", + workflow: "onboard", + channels: [ + { + channelId: "teams", + displayName: "Microsoft Teams", + authMode: "token-paste", + active: true, + selected: true, + configured: true, + disabled: false, + inputs: [], + hooks: [], + hostForward: { + channelId: "teams", + port: 3978, + label: "Microsoft Teams webhook", + }, + }, + ], + disabledChannels: [], + credentialBindings: [], + networkPolicy: { presets: [], entries: [] }, + agentRender: [], + buildSteps: [], + stateUpdates: [], + healthChecks: [], + }; +} + +describe("ensureMessagingHostForwardAfterRebuild", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("preserves a failed list probe and skips forward cleanup (#8522)", () => { + vi.mocked(captureOpenshell).mockReturnValue({ status: 1, output: "" }); + + const ok = ensureMessagingHostForwardAfterRebuild("demo", makePlan()); + + expect(ok).toBe(true); + expect(captureOpenshell).toHaveBeenCalledTimes(2); + expect(captureOpenshell).toHaveBeenNthCalledWith( + 1, + ["forward", "list"], + expect.objectContaining({ ignoreError: true }), + ); + expect(captureOpenshell).toHaveBeenNthCalledWith( + 2, + ["forward", "list"], + expect.objectContaining({ ignoreError: true, timeout: expect.any(Number) }), + ); + expect(runOpenshell).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts b/src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts index 1e720a7ab5..96cf908856 100644 --- a/src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts +++ b/src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts @@ -18,7 +18,9 @@ import { parseForwardList } from "../../state/sandbox-session"; import { classifyForwardHealthWithReachability, isLocalForwardReachable } from "./forward-health"; function captureOpenShellOutput(args: string[], opts: Record = {}): string | null { - const result = captureOpenshell(args, opts as Parameters[1]); + const result = captureOpenshell(args, { ...opts, ignoreError: true } as Parameters< + typeof captureOpenshell + >[1]); return result.status === 0 ? result.output : null; } diff --git a/src/lib/onboard/agent-fixed-forward.test.ts b/src/lib/onboard/agent-fixed-forward.test.ts new file mode 100644 index 0000000000..981561f1ea --- /dev/null +++ b/src/lib/onboard/agent-fixed-forward.test.ts @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { ensureAgentFixedForward } from "./agent-fixed-forward"; + +vi.mock("./forward-start", () => ({ + buildDetachedForwardStartSpawn: vi.fn(() => vi.fn()), + buildForwardStartProgressLogger: vi.fn(() => vi.fn()), + runDetachedForwardStartWithRetries: vi.fn(() => ({ ok: true, diagnostic: "" })), +})); + +function makeDeps(runCaptureOpenshell: () => string | null) { + return { + runOpenshell: vi.fn(() => ({ status: 0 })), + runCaptureOpenshell: vi.fn(runCaptureOpenshell), + openshellArgv: (args: string[]) => ["openshell", ...args], + cliName: () => "nemoclaw", + sleep: vi.fn(), + }; +} + +describe("ensureAgentFixedForward", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("skips the stop when the capture runner reports a failed `forward list` (#8522)", () => { + const deps = makeDeps(() => null); + + const started = ensureAgentFixedForward(deps, "my-sandbox", 18789, "messaging webhook"); + + expect(started).toBe(true); + expect(deps.runCaptureOpenshell).toHaveBeenCalledWith( + ["forward", "list"], + expect.objectContaining({ timeout: expect.any(Number) }), + ); + expect(deps.runOpenshell).not.toHaveBeenCalled(); + }); + + it("runs a sandbox-scoped `forward stop` when the `forward list` is genuinely empty (#8522)", () => { + const deps = makeDeps(() => "SANDBOX BIND PORT PID STATUS"); + + const started = ensureAgentFixedForward(deps, "my-sandbox", 18789, "messaging webhook"); + + expect(started).toBe(true); + expect(deps.runCaptureOpenshell).toHaveBeenCalledWith( + ["forward", "list"], + expect.objectContaining({ timeout: expect.any(Number) }), + ); + expect(deps.runOpenshell).toHaveBeenCalledWith(["forward", "stop", "18789", "my-sandbox"], { + ignoreError: true, + suppressOutput: true, + }); + }); +}); diff --git a/src/lib/onboard/agent-fixed-forward.ts b/src/lib/onboard/agent-fixed-forward.ts index bdfc46a754..b65d501e9f 100644 --- a/src/lib/onboard/agent-fixed-forward.ts +++ b/src/lib/onboard/agent-fixed-forward.ts @@ -29,7 +29,7 @@ export function ensureAgentFixedForward( const stopForwardForSandbox = (portToStop: string | number) => bestEffortForwardStopForSandbox( deps.runOpenshell, - (args, opts) => (deps.runCaptureOpenshell(args, opts) ?? "") as string, + (args, opts) => deps.runCaptureOpenshell(args, opts), portToStop, sandboxName, ); @@ -39,9 +39,7 @@ export function ensureAgentFixedForward( buildDetachedForwardStartSpawn( deps.openshellArgv(["forward", "start", "--background", forwardTarget, sandboxName]), ), - () => - (deps.runCaptureOpenshell(["forward", "list"], { timeout: OPENSHELL_PROBE_TIMEOUT_MS }) ?? - "") as string, + () => deps.runCaptureOpenshell(["forward", "list"], { timeout: OPENSHELL_PROBE_TIMEOUT_MS }), { port, sandboxName }, () => { deps.sleep(1); diff --git a/src/lib/onboard/dashboard-forward-control.test.ts b/src/lib/onboard/dashboard-forward-control.test.ts new file mode 100644 index 0000000000..1c6081636b --- /dev/null +++ b/src/lib/onboard/dashboard-forward-control.test.ts @@ -0,0 +1,25 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it, vi } from "vitest"; + +import { createSandboxForwardStopper } from "./dashboard-forward-control"; + +describe("createSandboxForwardStopper", () => { + it("skips the stop when the forward-list capture fails (#8522)", () => { + const runOpenshell = vi.fn(); + const runCaptureOpenshell = vi.fn().mockReturnValue(null); + const stopForward = createSandboxForwardStopper({ + runOpenshell, + runCaptureOpenshell, + sandboxName: "my-sandbox", + }); + + expect(stopForward(18789)).toBe("list-failed"); + expect(runCaptureOpenshell).toHaveBeenCalledWith( + ["forward", "list"], + expect.objectContaining({ timeout: 15_000 }), + ); + expect(runOpenshell).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/onboard/dashboard-forward-control.ts b/src/lib/onboard/dashboard-forward-control.ts index 391e01c79f..73a09ae135 100644 --- a/src/lib/onboard/dashboard-forward-control.ts +++ b/src/lib/onboard/dashboard-forward-control.ts @@ -32,13 +32,7 @@ export function createSandboxForwardStopper(deps: { if (stoppedPorts.has(portKey)) return null; const result = bestEffortForwardStopForSandbox( deps.runOpenshell, - (args, opts) => { - const output = deps.runCaptureOpenshell(args, opts); - if (output === null) { - throw new Error("Failed to list OpenShell forwards before stopping dashboard forward"); - } - return output; - }, + (args, opts) => deps.runCaptureOpenshell(args, opts), port, deps.sandboxName, ); diff --git a/src/lib/onboard/dashboard.ts b/src/lib/onboard/dashboard.ts index 3939969f99..924e0d9fe3 100644 --- a/src/lib/onboard/dashboard.ts +++ b/src/lib/onboard/dashboard.ts @@ -326,9 +326,7 @@ export function createOnboardDashboardHelpers(deps: OnboardDashboardDeps): Onboa buildDetachedForwardStartSpawn( deps.openshellArgv(["forward", "start", "--background", actualTarget, sandboxName]), ), - () => - (deps.runCaptureOpenshell(["forward", "list"], { timeout: OPENSHELL_PROBE_TIMEOUT_MS }) ?? - "") as string, + () => deps.runCaptureOpenshell(["forward", "list"], { timeout: OPENSHELL_PROBE_TIMEOUT_MS }), { port: actualPort, sandboxName }, () => { deps.sleep(1); diff --git a/src/lib/onboard/forward-cleanup.test.ts b/src/lib/onboard/forward-cleanup.test.ts index c3ffc22008..0ee0647d13 100644 --- a/src/lib/onboard/forward-cleanup.test.ts +++ b/src/lib/onboard/forward-cleanup.test.ts @@ -41,8 +41,8 @@ describe("bestEffortForwardStopForSandbox", () => { ["forward", "list"], expect.objectContaining({ timeout: 15_000 }), ); - // Caller must NOT pass ignoreError; failures should throw so the catch - // branch returns "list-failed" instead of running a stop with no owner data. + // The helper must not suppress list failures. A runner may throw or return + // null, but it must not convert a failed probe to empty output. expect(fetch).not.toHaveBeenCalledWith( ["forward", "list"], expect.objectContaining({ ignoreError: true }), @@ -88,6 +88,7 @@ describe("bestEffortForwardStopForSandbox", () => { const outcome = bestEffortForwardStopForSandbox(run, fetch, 18789, "my-sandbox"); + expect(fetch).toHaveBeenCalledWith(["forward", "list"], expect.anything()); expect(outcome).toBe("list-failed"); // Without ownership data, a port-only stop could kill another // sandbox's forward — better to leave the port alone and let the @@ -95,6 +96,17 @@ describe("bestEffortForwardStopForSandbox", () => { expect(run).not.toHaveBeenCalled(); }); + it("skips the stop entirely when `forward list` reports failure as null (owner unknown)", () => { + const run = vi.fn(); + const fetch = vi.fn().mockReturnValue(null); + + const outcome = bestEffortForwardStopForSandbox(run, fetch, 18789, "my-sandbox"); + + expect(fetch).toHaveBeenCalledWith(["forward", "list"], expect.anything()); + expect(outcome).toBe("list-failed"); + expect(run).not.toHaveBeenCalled(); + }); + it("ignores forwards with non-live status when deciding ownership", () => { // `getOccupiedPorts` filters by `isLiveForwardStatus`, so a "stopped" // entry on the requested port should be treated as no-entry (not as a diff --git a/src/lib/onboard/forward-cleanup.ts b/src/lib/onboard/forward-cleanup.ts index 5ccffe71f5..17cfc0a887 100644 --- a/src/lib/onboard/forward-cleanup.ts +++ b/src/lib/onboard/forward-cleanup.ts @@ -13,7 +13,7 @@ export type ForwardStopRunner = ( export type ForwardListRunner = ( args: string[], opts: { ignoreError?: boolean; timeout?: number }, -) => string; +) => string | null; /** * `openshell forward stop ` — port-scoped, kills whatever forward is @@ -64,12 +64,14 @@ export function bestEffortForwardStopForSandbox( port: string | number, sandboxName: string, ): "stopped" | "owned-other" | "no-entry" | "list-failed" { - // Let runCaptureOpenshell throw on failure/timeout so the catch branch - // returns "list-failed". With ignoreError: true the runner would swallow - // the error and return "", which getOccupiedPorts parses as an empty map - // and the "no-entry" branch below would still run the stop — exactly the - // collateral-damage case this helper exists to avoid. - let listOutput = ""; + // A runner reports failure either by throwing or by returning null; both + // mean "list-failed" here. Do not pass either result to getOccupiedPorts, + // which parses an empty string into an empty map, so the "no-entry" branch + // below would still run the stop against this sandbox's own live forward + // without any ownership evidence. A runner that ignores the command failure + // itself must convert it to null, never to an empty string, which is + // indistinguishable from a genuinely empty forward list. + let listOutput: string | null = null; try { listOutput = runCaptureOpenshell(["forward", "list"], { timeout: OPENSHELL_PROBE_TIMEOUT_MS, @@ -77,6 +79,9 @@ export function bestEffortForwardStopForSandbox( } catch { return "list-failed"; } + if (listOutput === null) { + return "list-failed"; + } const owner = getOccupiedPorts(listOutput).get(String(port)) ?? null; if (owner && owner !== sandboxName) { return "owned-other"; diff --git a/src/lib/onboard/forward-start.test.ts b/src/lib/onboard/forward-start.test.ts index 1a67d4d93c..5b9aaa4789 100644 --- a/src/lib/onboard/forward-start.test.ts +++ b/src/lib/onboard/forward-start.test.ts @@ -527,6 +527,35 @@ describe("runDetachedForwardStartWithDiagnostics", () => { } }); + it("does not accept a live port when the ownership lookup returns null (#8522)", () => { + const fetchList = vi.fn().mockReturnValue(null); + const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { + fs.writeSync(stderr, "ssh exited before local forward listener opened on 127.0.0.1:18789\n"); + return { pid: 790 }; + }); + const isPortListening = vi.fn().mockReturnValue(true); + const realKill = process.kill; + const killSpy = vi.fn(); + (process as { kill: typeof process.kill }).kill = killSpy as unknown as typeof process.kill; + + try { + const result = runDetachedForwardStartWithDiagnostics( + spawn, + fetchList, + { port: 18789, sandboxName: "my-sandbox" }, + { overallTimeoutMs: 180_000, sleepMs: vi.fn(), isPortListening }, + ); + + expect(result.ok).toBe(false); + expect(result.reason).toBe("listener-start-failure"); + expect(result.diagnostic).toMatch(/openshell forward list failed:.*no forward list result/i); + expect(isPortListening).not.toHaveBeenCalled(); + expect(killSpy).toHaveBeenCalledWith(790, "SIGTERM"); + } finally { + (process as { kill: typeof process.kill }).kill = realKill; + } + }); + it("rejects a live port without the established untracked-forward diagnostic (#7266)", () => { const fetchList = vi.fn().mockReturnValue(forwardListWith([])); const spawn = vi.fn().mockImplementation(({ stderr }: { stderr: number }) => { diff --git a/src/lib/onboard/forward-start.ts b/src/lib/onboard/forward-start.ts index deb0740514..16612a5830 100644 --- a/src/lib/onboard/forward-start.ts +++ b/src/lib/onboard/forward-start.ts @@ -24,7 +24,7 @@ import { cleanupTempDir, secureTempFile } from "./temp-files"; // The CLI's exit code is no longer the success signal — the appearance of // the live forward in the list is. -export type ForwardListFetcher = () => string; +export type ForwardListFetcher = () => string | null; export type DetachedForwardSpawnRunner = (stdio: { stdout: number; stderr: number }) => { pid?: number; @@ -378,11 +378,16 @@ export function runDetachedForwardStartWithDiagnostics( while (Date.now() < deadline) { let list = ""; try { - list = fetchForwardList() || ""; - // Clear the cached transient error so a recovered gateway does not - // leave a stale "openshell forward list failed: …" suffix on the - // eventual timeout diagnostic. - lastFetchError = null; + const fetchedList = fetchForwardList(); + if (fetchedList === null) { + lastFetchError = "OpenShell returned no forward list result"; + } else { + list = fetchedList; + // Clear the cached transient error so a recovered gateway does not + // leave a stale "openshell forward list failed: …" suffix on the + // eventual timeout diagnostic. + lastFetchError = null; + } } catch (err) { lastFetchError = err instanceof Error ? err.message : String(err); }