Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions src/lib/actions/sandbox/messaging-host-forward-lifecycle.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
4 changes: 3 additions & 1 deletion src/lib/actions/sandbox/messaging-host-forward-lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ import { parseForwardList } from "../../state/sandbox-session";
import { classifyForwardHealthWithReachability, isLocalForwardReachable } from "./forward-health";

function captureOpenShellOutput(args: string[], opts: Record<string, unknown> = {}): string | null {
const result = captureOpenshell(args, opts as Parameters<typeof captureOpenshell>[1]);
const result = captureOpenshell(args, { ...opts, ignoreError: true } as Parameters<
typeof captureOpenshell
>[1]);
return result.status === 0 ? result.output : null;
}

Expand Down
57 changes: 57 additions & 0 deletions src/lib/onboard/agent-fixed-forward.test.ts
Original file line number Diff line number Diff line change
@@ -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,
});
});
});
6 changes: 2 additions & 4 deletions src/lib/onboard/agent-fixed-forward.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand All @@ -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);
Expand Down
25 changes: 25 additions & 0 deletions src/lib/onboard/dashboard-forward-control.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
8 changes: 1 addition & 7 deletions src/lib/onboard/dashboard-forward-control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
4 changes: 1 addition & 3 deletions src/lib/onboard/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 14 additions & 2 deletions src/lib/onboard/forward-cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
Expand Down Expand Up @@ -88,13 +88,25 @@ 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
// helper's retry / next poll observe the real state.
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
Expand Down
19 changes: 12 additions & 7 deletions src/lib/onboard/forward-cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export type ForwardStopRunner = (
export type ForwardListRunner = (
args: string[],
opts: { ignoreError?: boolean; timeout?: number },
) => string;
) => string | null;

/**
* `openshell forward stop <port>` — port-scoped, kills whatever forward is
Expand Down Expand Up @@ -64,19 +64,24 @@ 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,
});
} 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";
Expand Down
29 changes: 29 additions & 0 deletions src/lib/onboard/forward-start.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand Down
17 changes: 11 additions & 6 deletions src/lib/onboard/forward-start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Loading