From ef77620b001197a750e0df982530228af795ab46 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 13 Aug 2026 14:54:06 -0700 Subject: [PATCH 01/11] fix(sandbox): fail closed on ambiguous sandbox-name during destroy (#8999) A foreign container that borrows a real sandbox's openshell.ai/sandbox-name label (different sandbox-workspace, no managed-by=openshell marker) let 'nemoclaw destroy' proceed and remove the real sandbox: the mutable name alone could no longer prove which container was meant. Add a Docker-only, pre-destructive identity guard. classifyDestroyContainer Identity queries containers by the sandbox-name label only (not managed-by, so an impostor stays visible) and refuses when any match lacks the managed marker or the managed set spans more than one workspace / sandbox-id. The guard runs after the confirm prompt and before any destructive step, so an ambiguous target exits non-zero with nothing removed. A Docker probe failure is non-blocking so a normal destroy is never wedged by an unreachable daemon; Podman keeps its existing single-identity resolver guard. Signed-off-by: Aarav Sharma --- .../destroy-container-identity.test.ts | 132 +++++++++++++ .../sandbox/destroy-container-identity.ts | 183 ++++++++++++++++++ src/lib/actions/sandbox/destroy.test.ts | 67 ++++++- src/lib/actions/sandbox/destroy.ts | 56 ++++++ 4 files changed, 437 insertions(+), 1 deletion(-) create mode 100644 src/lib/actions/sandbox/destroy-container-identity.test.ts create mode 100644 src/lib/actions/sandbox/destroy-container-identity.ts diff --git a/src/lib/actions/sandbox/destroy-container-identity.test.ts b/src/lib/actions/sandbox/destroy-container-identity.test.ts new file mode 100644 index 00000000000..3695d893fcf --- /dev/null +++ b/src/lib/actions/sandbox/destroy-container-identity.test.ts @@ -0,0 +1,132 @@ +// 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 { + classifyDestroyContainerIdentity, + formatAmbiguousDestroyIdentity, +} from "./destroy-container-identity"; + +type Row = { + id: string; + managedBy?: string; + workspace?: string; + sandboxId?: string; +}; + +function fakeDockerRun(rows: Row[], status = 0): { status: number; stdout: string } { + const stdout = rows + .map((r) => [r.id, r.managedBy ?? "", r.workspace ?? "", r.sandboxId ?? ""].join("\t")) + .join("\n"); + return { status, stdout }; +} + +const MANAGED = { + id: "aaaa000000000000", + managedBy: "openshell", + workspace: "default", + sandboxId: "sb-real", +} as const; + +const FOREIGN = { + id: "ffff000000000000", + managedBy: "", + workspace: "foreign", + sandboxId: "", +} as const; + +describe("classifyDestroyContainerIdentity", () => { + it("is clear when no container carries the sandbox-name label", () => { + const dockerRun = vi.fn(() => fakeDockerRun([])); + expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + }); + + it("is clear for exactly one managed container", () => { + const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); + expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + }); + + it("refuses when a foreign container shares the sandbox-name label (#8999 repro)", () => { + // The exact repro: a real managed sandbox plus a busybox that borrows the + // sandbox-name label with a different workspace and no managed marker. + const dockerRun = vi.fn(() => fakeDockerRun([MANAGED, FOREIGN])); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expect(verdict.status).toBe("ambiguous"); + if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expect(verdict.foreign).toHaveLength(1); + expect(verdict.foreign[0].id).toBe(FOREIGN.id); + expect(verdict.managed).toHaveLength(1); + expect(verdict.reason).toContain("managed-by"); + }); + + it("refuses a foreign-only match with no managed container behind it", () => { + const dockerRun = vi.fn(() => fakeDockerRun([FOREIGN])); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expect(verdict.status).toBe("ambiguous"); + if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expect(verdict.managed).toHaveLength(0); + expect(verdict.foreign).toHaveLength(1); + }); + + it("refuses when managed containers span more than one workspace", () => { + const dockerRun = vi.fn(() => + fakeDockerRun([ + MANAGED, + { id: "bbbb000000000000", managedBy: "openshell", workspace: "other", sandboxId: "sb-real" }, + ]), + ); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expect(verdict.status).toBe("ambiguous"); + if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expect(verdict.reason).toContain("workspace"); + }); + + it("refuses when managed containers span more than one sandbox-id", () => { + const dockerRun = vi.fn(() => + fakeDockerRun([ + MANAGED, + { id: "cccc000000000000", managedBy: "openshell", workspace: "default", sandboxId: "sb-two" }, + ]), + ); + expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("ambiguous"); + }); + + it("does not block when the Docker probe fails (ambiguity unprovable)", () => { + const dockerRun = vi.fn(() => ({ status: 1, stdout: "", stderr: "Cannot connect to daemon" })); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expect(verdict.status).toBe("probe-failed"); + if (verdict.status !== "probe-failed") throw new Error("unreachable"); + expect(verdict.detail).toContain("daemon"); + }); + + it("ignores blank and malformed lines without misclassifying", () => { + const dockerRun = vi.fn(() => ({ + status: 0, + stdout: `\n \n${["aaaa000000000000", "openshell", "default", "sb-real"].join("\t")}\n`, + })); + expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + }); + + it("filters ONLY on the sandbox-name label so foreign containers stay visible", () => { + const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); + classifyDestroyContainerIdentity("destroytest", { dockerRun }); + const argv = dockerRun.mock.calls[0][0] as string[]; + expect(argv).toContain("label=openshell.ai/sandbox-name=destroytest"); + expect(argv.some((a) => a.includes("managed-by="))).toBe(false); + }); +}); + +describe("formatAmbiguousDestroyIdentity", () => { + it("names the refusal, both container roles, and the recovery step", () => { + const verdict = classifyDestroyContainerIdentity("destroytest", { + dockerRun: () => fakeDockerRun([MANAGED, FOREIGN]), + }); + if (verdict.status !== "ambiguous") throw new Error("expected ambiguous verdict"); + const lines = formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n"); + expect(lines).toContain("Refusing to destroy sandbox 'destroytest'"); + expect(lines).toContain("Unexpected container:"); + expect(lines).toContain("Managed sandbox container:"); + expect(lines).toContain("nemoclaw destroytest destroy --yes"); + }); +}); diff --git a/src/lib/actions/sandbox/destroy-container-identity.ts b/src/lib/actions/sandbox/destroy-container-identity.ts new file mode 100644 index 00000000000..166b29f0411 --- /dev/null +++ b/src/lib/actions/sandbox/destroy-container-identity.ts @@ -0,0 +1,183 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { dockerRun } from "../../adapters/docker"; +import { + OPENSHELL_MANAGED_BY_LABEL, + OPENSHELL_MANAGED_BY_VALUE, + OPENSHELL_SANDBOX_ID_LABEL, + OPENSHELL_SANDBOX_NAME_LABEL, +} from "../../onboard/openshell-docker-sandbox-containers"; + +/** Workspace label OpenShell stamps on every managed sandbox container. */ +export const OPENSHELL_SANDBOX_WORKSPACE_LABEL = "openshell.ai/sandbox-workspace"; + +const DOCKER_IDENTITY_PROBE_TIMEOUT_MS = 30_000; + +/** One container carrying the destroy target's `sandbox-name` label. */ +export type SandboxNameLabeledContainer = { + id: string; + managedBy: string; + workspace: string; + sandboxId: string; +}; + +/** + * Verdict for whether the `sandbox-name` a destroy targets maps to a single + * unambiguous managed container identity. + * + * - `clear` — no labeled container, or exactly one managed identity. Destroy + * may proceed. + * - `ambiguous` — a container claims the target `sandbox-name` but is not the + * single managed sandbox (a foreign container carrying the label, or managed + * containers spanning more than one workspace / sandbox-id). Destroy must + * fail closed so it never removes the real sandbox behind an impostor's name. + * - `probe-failed` — Docker could not be queried, so ambiguity can neither be + * proven nor ruled out. Non-blocking: the destroy's existing lower-layer + * guards remain in force. + */ +export type DestroyContainerIdentityVerdict = + | { status: "clear" } + | { status: "probe-failed"; detail: string } + | { + status: "ambiguous"; + sandboxName: string; + reason: string; + foreign: SandboxNameLabeledContainer[]; + managed: SandboxNameLabeledContainer[]; + }; + +export type ClassifyDestroyContainerIdentityDeps = { + dockerRun?: typeof dockerRun; +}; + +const IDENTITY_FORMAT = [ + "{{.ID}}", + `{{.Label "${OPENSHELL_MANAGED_BY_LABEL}"}}`, + `{{.Label "${OPENSHELL_SANDBOX_WORKSPACE_LABEL}"}}`, + `{{.Label "${OPENSHELL_SANDBOX_ID_LABEL}"}}`, +].join("\t"); + +function resultText(result: { + stdout?: string | Buffer | null; + stderr?: string | Buffer | null; +}): string { + return `${String(result.stderr || "")} ${String(result.stdout || "")}`.trim(); +} + +function parseIdentityRows(stdout: string): SandboxNameLabeledContainer[] { + return stdout + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter(Boolean) + .map((line) => { + const [id = "", managedBy = "", workspace = "", sandboxId = ""] = line.split("\t"); + return { + id: id.trim(), + managedBy: managedBy.trim(), + workspace: workspace.trim(), + sandboxId: sandboxId.trim(), + }; + }) + .filter((row) => row.id.length > 0); +} + +/** + * Classify every Docker container carrying `openshell.ai/sandbox-name=` + * to decide whether the destroy target resolves to a single managed identity. + * + * The lookup deliberately filters ONLY on the mutable `sandbox-name` label — + * not on `managed-by=openshell` — so a foreign container that borrows the name + * without the managed marker is still seen. A genuine managed sandbox carries + * `managed-by=openshell` and one consistent `sandbox-workspace` / `sandbox-id`; + * anything else sharing the name makes the identity ambiguous. + */ +export function classifyDestroyContainerIdentity( + sandboxName: string, + deps: ClassifyDestroyContainerIdentityDeps = {}, +): DestroyContainerIdentityVerdict { + const run = deps.dockerRun ?? dockerRun; + const result = run( + [ + "ps", + "-a", + "--no-trunc", + "--filter", + `label=${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, + "--format", + IDENTITY_FORMAT, + ], + { + ignoreError: true, + suppressOutput: true, + timeout: DOCKER_IDENTITY_PROBE_TIMEOUT_MS, + }, + ); + if (Number(result.status ?? 1) !== 0) { + return { + status: "probe-failed", + detail: resultText(result) || "docker ps did not complete successfully", + }; + } + + const rows = parseIdentityRows(String(result.stdout ?? "")); + if (rows.length === 0) return { status: "clear" }; + + const managed = rows.filter((row) => row.managedBy === OPENSHELL_MANAGED_BY_VALUE); + const foreign = rows.filter((row) => row.managedBy !== OPENSHELL_MANAGED_BY_VALUE); + + if (foreign.length > 0) { + return { + status: "ambiguous", + sandboxName, + reason: + `${String(foreign.length)} container(s) carry the '${OPENSHELL_SANDBOX_NAME_LABEL}=` + + `${sandboxName}' label without the '${OPENSHELL_MANAGED_BY_LABEL}=` + + `${OPENSHELL_MANAGED_BY_VALUE}' marker`, + foreign, + managed, + }; + } + + const workspaces = new Set(managed.map((row) => row.workspace)); + const sandboxIds = new Set(managed.map((row) => row.sandboxId).filter(Boolean)); + if (workspaces.size > 1 || sandboxIds.size > 1) { + return { + status: "ambiguous", + sandboxName, + reason: + `managed containers for '${sandboxName}' span ${String(workspaces.size)} workspace(s) ` + + `and ${String(sandboxIds.size)} sandbox-id(s)`, + foreign, + managed, + }; + } + + return { status: "clear" }; +} + +/** Human-readable lines describing an ambiguous-identity refusal. */ +export function formatAmbiguousDestroyIdentity( + verdict: Extract, + cliName: string, +): string[] { + const describe = (row: SandboxNameLabeledContainer): string => + `${row.id.slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${row.managedBy || ""}, ` + + `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${row.workspace || ""})`; + const lines = [ + `Refusing to destroy sandbox '${verdict.sandboxName}': ${verdict.reason}.`, + "Destroy fails closed because the sandbox-name no longer identifies a single container, " + + "so it cannot prove which container it would remove.", + ]; + for (const row of verdict.foreign) { + lines.push(` Unexpected container: ${describe(row)}`); + } + for (const row of verdict.managed) { + lines.push(` Managed sandbox container: ${describe(row)}`); + } + lines.push( + "Remove or relabel the unexpected container(s), then re-run " + + `'${cliName} ${verdict.sandboxName} destroy --yes'.`, + ); + return lines; +} diff --git a/src/lib/actions/sandbox/destroy.test.ts b/src/lib/actions/sandbox/destroy.test.ts index db2f370c534..b1e6a16f3f3 100644 --- a/src/lib/actions/sandbox/destroy.test.ts +++ b/src/lib/actions/sandbox/destroy.test.ts @@ -4,7 +4,7 @@ import path from "node:path"; import { describe, expect, it, vi } from "vitest"; -import { cleanupSandboxServices } from "./destroy"; +import { assertUnambiguousDestroyContainerIdentity, cleanupSandboxServices } from "./destroy"; const SANDBOX = "mybox"; const mainPidDir = path.resolve("/tmp", `nemoclaw-services-${SANDBOX}`); @@ -70,3 +70,68 @@ describe("cleanupSandboxServices Google Chat tunnel cleanup (#7317)", () => { expect(rmSync).toHaveBeenCalledWith(googlechatPidDir, { recursive: true, force: true }); }); }); + +describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { + const dockerSandbox = { openshellDriver: "docker" } as { openshellDriver: string | null }; + + it("refuses destroy when a foreign container shares the sandbox-name label", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const classify = vi.fn(() => ({ + status: "ambiguous" as const, + sandboxName: "destroytest", + reason: "a foreign container carries the label", + foreign: [{ id: "ffff", managedBy: "", workspace: "foreign", sandboxId: "" }], + managed: [{ id: "aaaa", managedBy: "openshell", workspace: "default", sandboxId: "sb" }], + })); + + const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { + getSandbox: vi.fn(() => dockerSandbox) as never, + classify: classify as never, + }); + + expect(proceed).toBe(false); + expect(classify).toHaveBeenCalledWith("destroytest"); + expect(error).toHaveBeenCalled(); + error.mockRestore(); + }); + + it("proceeds for a clear single managed identity", () => { + const classify = vi.fn(() => ({ status: "clear" as const })); + expect( + assertUnambiguousDestroyContainerIdentity("destroytest", { + getSandbox: vi.fn(() => dockerSandbox) as never, + classify: classify as never, + }), + ).toBe(true); + }); + + it("does not probe or block a non-Docker runtime provider", () => { + const classify = vi.fn(); + const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { + getSandbox: vi.fn(() => ({ openshellDriver: "podman" })) as never, + classify: classify as never, + }); + expect(proceed).toBe(true); + expect(classify).not.toHaveBeenCalled(); + }); + + it("proceeds but warns when the Docker probe cannot prove identity", () => { + const warn = vi.fn(); + const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { + getSandbox: vi.fn(() => dockerSandbox) as never, + classify: vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })) as never, + warn, + }); + expect(proceed).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("daemon down")); + }); + + it("treats an unknown/null driver as Docker (the default) and still guards", () => { + const classify = vi.fn(() => ({ status: "clear" as const })); + assertUnambiguousDestroyContainerIdentity("destroytest", { + getSandbox: vi.fn(() => ({ openshellDriver: null })) as never, + classify: classify as never, + }); + expect(classify).toHaveBeenCalledWith("destroytest"); + }); +}); diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 0f4c55dea67..fb368df2517 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -41,6 +41,11 @@ import * as onboardSession from "../../state/onboard-session"; import { resolveNemoclawStateDir } from "../../state/paths"; import * as registry from "../../state/registry"; import { confirmSandboxDestroy } from "./destroy-confirmation"; +import { + classifyDestroyContainerIdentity, + type DestroyContainerIdentityVerdict, + formatAmbiguousDestroyIdentity, +} from "./destroy-container-identity"; import { executeSandboxDestroy, redactDestroyError, @@ -458,6 +463,53 @@ export async function destroySandbox( return withMcpLifecycleLock(sandboxName, () => destroySandboxUnlocked(sandboxName, options)); } +export type AssertUnambiguousDestroyIdentityDeps = { + getSandbox?: typeof registry.getSandbox; + classify?: typeof classifyDestroyContainerIdentity; + warn?: (message: string) => void; +}; + +/** + * Fail closed before any destructive step when the target `sandbox-name` maps + * to more than one container identity on the Docker runtime. + * + * A foreign container that borrows a real sandbox's `sandbox-name` label (with + * a different workspace / without the managed marker) must never let destroy + * silently remove the real sandbox: the name alone can no longer prove which + * container is meant, so the only safe action is to refuse (#8999). The check + * is Docker-only — Podman lifecycle enforces exact single-container identity in + * its own resolver — and a probe that cannot reach Docker is non-blocking so a + * normal destroy is never wedged by an unreachable daemon. + * + * Returns `true` when destroy may proceed and `false` when it was refused. + */ +export function assertUnambiguousDestroyContainerIdentity( + sandboxName: string, + deps: AssertUnambiguousDestroyIdentityDeps = {}, +): boolean { + const getSandbox = deps.getSandbox ?? registry.getSandbox; + const classify = deps.classify ?? classifyDestroyContainerIdentity; + const warn = deps.warn ?? defaultDestroyWarn; + const providerId = normalizeRuntimeProviderIdentity(getSandbox(sandboxName)?.openshellDriver); + if (providerId !== "docker") return true; + + const verdict: DestroyContainerIdentityVerdict = classify(sandboxName); + if (verdict.status === "ambiguous") { + for (const line of formatAmbiguousDestroyIdentity(verdict, CLI_NAME)) { + console.error(` ${line}`); + } + return false; + } + if (verdict.status === "probe-failed") { + // Ambiguity can neither be proven nor ruled out; proceed under the + // destroy's existing lower-layer guards rather than wedge a normal destroy. + warn( + `Could not verify container identity for '${sandboxName}' before destroy: ${verdict.detail}`, + ); + } + return true; +} + async function destroySandboxUnlocked( sandboxName: string, options: string[] | DestroySandboxOptions = {}, @@ -465,6 +517,10 @@ async function destroySandboxUnlocked( const normalized = normalizeDestroySandboxOptions(options); if (!(await confirmSandboxDestroy(sandboxName, normalized))) return; + if (!assertUnambiguousDestroyContainerIdentity(sandboxName)) { + process.exit(1); + } + const { cleanupGatewayName, runOpenshell, sandbox, sandboxConfirmedAbsent } = prepareSandboxDestroy(sandboxName); const priorHttpsPinRouteId = parseHttpsPinRouteId(sandbox?.endpointUrl); From 4bbf7a17e0ca60f2d31be77975c38183f8eed794 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 13 Aug 2026 15:33:21 -0700 Subject: [PATCH 02/11] fix(sandbox): fail closed on unprovable destroy identity (#8999) Address review on #9042: - Fail closed when the Docker identity probe cannot run, since the lower layer can still delete through the gateway (was warn+proceed). - Treat a managed container missing its sandbox-workspace or sandbox-id label as ambiguous instead of filtering blank ids before the uniqueness check. - Show sandbox-id in the refusal diagnostics and use neutral recovery wording. - Keep changed test bodies linear (no if statements) via narrowing assertion helpers, satisfying the codebase-growth guardrail. Signed-off-by: Aarav Sharma --- .../destroy-container-identity.test.ts | 58 +++++++++++++++---- .../sandbox/destroy-container-identity.ts | 21 ++++++- src/lib/actions/sandbox/destroy.test.ts | 4 +- src/lib/actions/sandbox/destroy.ts | 14 +++-- 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-container-identity.test.ts b/src/lib/actions/sandbox/destroy-container-identity.test.ts index 3695d893fcf..4332778e0ee 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.test.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it, vi } from "vitest"; import { classifyDestroyContainerIdentity, + type DestroyContainerIdentityVerdict, formatAmbiguousDestroyIdentity, } from "./destroy-container-identity"; @@ -22,6 +23,20 @@ function fakeDockerRun(rows: Row[], status = 0): { status: number; stdout: strin return { status, stdout }; } +// Narrowing assertion helpers keep test bodies linear (no branching): they +// assert the verdict shape and let TypeScript narrow the union afterwards. +function expectAmbiguous( + verdict: DestroyContainerIdentityVerdict, +): asserts verdict is Extract { + expect(verdict.status).toBe("ambiguous"); +} + +function expectProbeFailed( + verdict: DestroyContainerIdentityVerdict, +): asserts verdict is Extract { + expect(verdict.status).toBe("probe-failed"); +} + const MANAGED = { id: "aaaa000000000000", managedBy: "openshell", @@ -52,8 +67,7 @@ describe("classifyDestroyContainerIdentity", () => { // sandbox-name label with a different workspace and no managed marker. const dockerRun = vi.fn(() => fakeDockerRun([MANAGED, FOREIGN])); const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expectAmbiguous(verdict); expect(verdict.foreign).toHaveLength(1); expect(verdict.foreign[0].id).toBe(FOREIGN.id); expect(verdict.managed).toHaveLength(1); @@ -63,8 +77,7 @@ describe("classifyDestroyContainerIdentity", () => { it("refuses a foreign-only match with no managed container behind it", () => { const dockerRun = vi.fn(() => fakeDockerRun([FOREIGN])); const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expectAmbiguous(verdict); expect(verdict.managed).toHaveLength(0); expect(verdict.foreign).toHaveLength(1); }); @@ -77,8 +90,7 @@ describe("classifyDestroyContainerIdentity", () => { ]), ); const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); + expectAmbiguous(verdict); expect(verdict.reason).toContain("workspace"); }); @@ -92,11 +104,35 @@ describe("classifyDestroyContainerIdentity", () => { expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("ambiguous"); }); + it("refuses a managed container missing its sandbox-id label (identity unprovable) (#8999)", () => { + // A second same-name row that is managed and in the same workspace but has + // no sandbox-id must not be waved through as a single clear identity: the + // blank label used to be filtered out before the uniqueness check. + const dockerRun = vi.fn(() => + fakeDockerRun([ + MANAGED, + { id: "dddd000000000000", managedBy: "openshell", workspace: "default", sandboxId: "" }, + ]), + ); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expectAmbiguous(verdict); + expect(verdict.reason).toContain("missing a required"); + expect(verdict.managed).toHaveLength(2); + }); + + it("refuses a lone managed container missing its workspace label (#8999)", () => { + const dockerRun = vi.fn(() => + fakeDockerRun([{ id: "eeee000000000000", managedBy: "openshell", workspace: "", sandboxId: "sb-real" }]), + ); + const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + expectAmbiguous(verdict); + expect(verdict.reason).toContain("missing a required"); + }); + it("does not block when the Docker probe fails (ambiguity unprovable)", () => { const dockerRun = vi.fn(() => ({ status: 1, stdout: "", stderr: "Cannot connect to daemon" })); const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("probe-failed"); - if (verdict.status !== "probe-failed") throw new Error("unreachable"); + expectProbeFailed(verdict); expect(verdict.detail).toContain("daemon"); }); @@ -118,15 +154,17 @@ describe("classifyDestroyContainerIdentity", () => { }); describe("formatAmbiguousDestroyIdentity", () => { - it("names the refusal, both container roles, and the recovery step", () => { + it("names the refusal, both container roles with sandbox-id, and neutral recovery", () => { const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun: () => fakeDockerRun([MANAGED, FOREIGN]), }); - if (verdict.status !== "ambiguous") throw new Error("expected ambiguous verdict"); + expectAmbiguous(verdict); const lines = formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n"); expect(lines).toContain("Refusing to destroy sandbox 'destroytest'"); expect(lines).toContain("Unexpected container:"); expect(lines).toContain("Managed sandbox container:"); + expect(lines).toContain("sandbox-id=sb-real"); + expect(lines).toContain("Inspect, remove, or relabel the conflicting container"); expect(lines).toContain("nemoclaw destroytest destroy --yes"); }); }); diff --git a/src/lib/actions/sandbox/destroy-container-identity.ts b/src/lib/actions/sandbox/destroy-container-identity.ts index 166b29f0411..55d13e44463 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.ts @@ -139,8 +139,22 @@ export function classifyDestroyContainerIdentity( }; } + const managedMissingLabels = managed.filter((row) => !row.workspace || !row.sandboxId); + if (managedMissingLabels.length > 0) { + return { + status: "ambiguous", + sandboxName, + reason: + `${String(managedMissingLabels.length)} managed container(s) for '${sandboxName}' are ` + + `missing a required '${OPENSHELL_SANDBOX_WORKSPACE_LABEL}' or '${OPENSHELL_SANDBOX_ID_LABEL}' ` + + "label, so the identity cannot be proven", + foreign, + managed, + }; + } + const workspaces = new Set(managed.map((row) => row.workspace)); - const sandboxIds = new Set(managed.map((row) => row.sandboxId).filter(Boolean)); + const sandboxIds = new Set(managed.map((row) => row.sandboxId)); if (workspaces.size > 1 || sandboxIds.size > 1) { return { status: "ambiguous", @@ -163,7 +177,8 @@ export function formatAmbiguousDestroyIdentity( ): string[] { const describe = (row: SandboxNameLabeledContainer): string => `${row.id.slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${row.managedBy || ""}, ` + - `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${row.workspace || ""})`; + `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${row.workspace || ""}, ` + + `${OPENSHELL_SANDBOX_ID_LABEL}=${row.sandboxId || ""})`; const lines = [ `Refusing to destroy sandbox '${verdict.sandboxName}': ${verdict.reason}.`, "Destroy fails closed because the sandbox-name no longer identifies a single container, " + @@ -176,7 +191,7 @@ export function formatAmbiguousDestroyIdentity( lines.push(` Managed sandbox container: ${describe(row)}`); } lines.push( - "Remove or relabel the unexpected container(s), then re-run " + + "Inspect, remove, or relabel the conflicting container(s), then re-run " + `'${cliName} ${verdict.sandboxName} destroy --yes'.`, ); return lines; diff --git a/src/lib/actions/sandbox/destroy.test.ts b/src/lib/actions/sandbox/destroy.test.ts index b1e6a16f3f3..de3c33e228f 100644 --- a/src/lib/actions/sandbox/destroy.test.ts +++ b/src/lib/actions/sandbox/destroy.test.ts @@ -115,14 +115,14 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { expect(classify).not.toHaveBeenCalled(); }); - it("proceeds but warns when the Docker probe cannot prove identity", () => { + it("fails closed when the Docker probe cannot prove identity (#8999)", () => { const warn = vi.fn(); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, classify: vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })) as never, warn, }); - expect(proceed).toBe(true); + expect(proceed).toBe(false); expect(warn).toHaveBeenCalledWith(expect.stringContaining("daemon down")); }); diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index fb368df2517..6711d7de66a 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -478,8 +478,9 @@ export type AssertUnambiguousDestroyIdentityDeps = { * silently remove the real sandbox: the name alone can no longer prove which * container is meant, so the only safe action is to refuse (#8999). The check * is Docker-only — Podman lifecycle enforces exact single-container identity in - * its own resolver — and a probe that cannot reach Docker is non-blocking so a - * normal destroy is never wedged by an unreachable daemon. + * its own resolver. A probe that cannot reach Docker also fails closed: the + * lower layer can still delete through the gateway, so an unverifiable identity + * must not be allowed to proceed. * * Returns `true` when destroy may proceed and `false` when it was refused. */ @@ -501,11 +502,14 @@ export function assertUnambiguousDestroyContainerIdentity( return false; } if (verdict.status === "probe-failed") { - // Ambiguity can neither be proven nor ruled out; proceed under the - // destroy's existing lower-layer guards rather than wedge a normal destroy. + // Identity can neither be proven nor ruled out, and the lower layer can + // still delete through the gateway — so fail closed rather than destroy a + // target we cannot identify (#8999). warn( - `Could not verify container identity for '${sandboxName}' before destroy: ${verdict.detail}`, + `Refusing to destroy '${sandboxName}': could not verify container identity before ` + + `destroy: ${verdict.detail}. Ensure Docker is reachable, then re-run.`, ); + return false; } return true; } From 069c3477a1d9e2dc3e0ceecd2126cb3321b5976c Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 13 Aug 2026 15:58:52 -0700 Subject: [PATCH 03/11] refactor(sandbox): move destroy identity docker probe into an adapter (#8999) classifyDestroyContainerIdentity called dockerRun and parsed docker ps output directly from the actions layer. Move the host/process boundary into a probeSandboxNameContainers adapter (src/lib/adapters/docker) that returns parsed rows or a probe failure, and make the classifier a pure function over that explicit probe result. The destroy action composes the adapter, the classifier, and the refusal output. Behavior is unchanged: probe failure still fails closed, a managed container missing sandbox-workspace or sandbox-id is still ambiguous, and the foreign-label #8999 repro is still refused. Probe parsing/argv coverage moves to the new adapter test; the classifier test now drives the pure function over probe results. Addresses CodeRabbit review on PR #9042. Signed-off-by: Aarav Sharma --- .../destroy-container-identity.test.ts | 104 ++++++----------- .../sandbox/destroy-container-identity.ts | 107 +++++------------ src/lib/actions/sandbox/destroy.test.ts | 19 ++- src/lib/actions/sandbox/destroy.ts | 5 +- src/lib/adapters/docker/index.ts | 1 + .../adapters/docker/sandbox-identity.test.ts | 50 ++++++++ src/lib/adapters/docker/sandbox-identity.ts | 108 ++++++++++++++++++ 7 files changed, 242 insertions(+), 152 deletions(-) create mode 100644 src/lib/adapters/docker/sandbox-identity.test.ts create mode 100644 src/lib/adapters/docker/sandbox-identity.ts diff --git a/src/lib/actions/sandbox/destroy-container-identity.test.ts b/src/lib/actions/sandbox/destroy-container-identity.test.ts index 4332778e0ee..760cbc3cd46 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.test.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.test.ts @@ -1,26 +1,22 @@ // 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 { describe, expect, it } from "vitest"; +import type { + SandboxIdentityProbe, + SandboxNameLabeledContainer, +} from "../../adapters/docker/sandbox-identity"; import { classifyDestroyContainerIdentity, type DestroyContainerIdentityVerdict, formatAmbiguousDestroyIdentity, } from "./destroy-container-identity"; -type Row = { - id: string; - managedBy?: string; - workspace?: string; - sandboxId?: string; -}; - -function fakeDockerRun(rows: Row[], status = 0): { status: number; stdout: string } { - const stdout = rows - .map((r) => [r.id, r.managedBy ?? "", r.workspace ?? "", r.sandboxId ?? ""].join("\t")) - .join("\n"); - return { status, stdout }; +// The classifier is pure over a probe result: the Docker call + output parsing +// live in the probeSandboxNameContainers adapter (see sandbox-identity.test.ts). +function ok(rows: SandboxNameLabeledContainer[]): SandboxIdentityProbe { + return { status: "ok", rows }; } // Narrowing assertion helpers keep test bodies linear (no branching): they @@ -31,42 +27,33 @@ function expectAmbiguous( expect(verdict.status).toBe("ambiguous"); } -function expectProbeFailed( - verdict: DestroyContainerIdentityVerdict, -): asserts verdict is Extract { - expect(verdict.status).toBe("probe-failed"); -} - -const MANAGED = { +const MANAGED: SandboxNameLabeledContainer = { id: "aaaa000000000000", managedBy: "openshell", workspace: "default", sandboxId: "sb-real", -} as const; +}; -const FOREIGN = { +const FOREIGN: SandboxNameLabeledContainer = { id: "ffff000000000000", managedBy: "", workspace: "foreign", sandboxId: "", -} as const; +}; describe("classifyDestroyContainerIdentity", () => { it("is clear when no container carries the sandbox-name label", () => { - const dockerRun = vi.fn(() => fakeDockerRun([])); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + expect(classifyDestroyContainerIdentity("destroytest", ok([])).status).toBe("clear"); }); it("is clear for exactly one managed container", () => { - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + expect(classifyDestroyContainerIdentity("destroytest", ok([MANAGED])).status).toBe("clear"); }); it("refuses when a foreign container shares the sandbox-name label (#8999 repro)", () => { // The exact repro: a real managed sandbox plus a busybox that borrows the // sandbox-name label with a different workspace and no managed marker. - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED, FOREIGN])); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + const verdict = classifyDestroyContainerIdentity("destroytest", ok([MANAGED, FOREIGN])); expectAmbiguous(verdict); expect(verdict.foreign).toHaveLength(1); expect(verdict.foreign[0].id).toBe(FOREIGN.id); @@ -75,89 +62,72 @@ describe("classifyDestroyContainerIdentity", () => { }); it("refuses a foreign-only match with no managed container behind it", () => { - const dockerRun = vi.fn(() => fakeDockerRun([FOREIGN])); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); + const verdict = classifyDestroyContainerIdentity("destroytest", ok([FOREIGN])); expectAmbiguous(verdict); expect(verdict.managed).toHaveLength(0); expect(verdict.foreign).toHaveLength(1); }); it("refuses when managed containers span more than one workspace", () => { - const dockerRun = vi.fn(() => - fakeDockerRun([ + const verdict = classifyDestroyContainerIdentity( + "destroytest", + ok([ MANAGED, { id: "bbbb000000000000", managedBy: "openshell", workspace: "other", sandboxId: "sb-real" }, ]), ); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); expectAmbiguous(verdict); expect(verdict.reason).toContain("workspace"); }); it("refuses when managed containers span more than one sandbox-id", () => { - const dockerRun = vi.fn(() => - fakeDockerRun([ + const verdict = classifyDestroyContainerIdentity( + "destroytest", + ok([ MANAGED, { id: "cccc000000000000", managedBy: "openshell", workspace: "default", sandboxId: "sb-two" }, ]), ); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("ambiguous"); + expect(verdict.status).toBe("ambiguous"); }); it("refuses a managed container missing its sandbox-id label (identity unprovable) (#8999)", () => { // A second same-name row that is managed and in the same workspace but has // no sandbox-id must not be waved through as a single clear identity: the // blank label used to be filtered out before the uniqueness check. - const dockerRun = vi.fn(() => - fakeDockerRun([ + const verdict = classifyDestroyContainerIdentity( + "destroytest", + ok([ MANAGED, { id: "dddd000000000000", managedBy: "openshell", workspace: "default", sandboxId: "" }, ]), ); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); expectAmbiguous(verdict); expect(verdict.reason).toContain("missing a required"); expect(verdict.managed).toHaveLength(2); }); it("refuses a lone managed container missing its workspace label (#8999)", () => { - const dockerRun = vi.fn(() => - fakeDockerRun([{ id: "eeee000000000000", managedBy: "openshell", workspace: "", sandboxId: "sb-real" }]), + const verdict = classifyDestroyContainerIdentity( + "destroytest", + ok([{ id: "eeee000000000000", managedBy: "openshell", workspace: "", sandboxId: "sb-real" }]), ); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); expectAmbiguous(verdict); expect(verdict.reason).toContain("missing a required"); }); - it("does not block when the Docker probe fails (ambiguity unprovable)", () => { - const dockerRun = vi.fn(() => ({ status: 1, stdout: "", stderr: "Cannot connect to daemon" })); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expectProbeFailed(verdict); - expect(verdict.detail).toContain("daemon"); - }); - - it("ignores blank and malformed lines without misclassifying", () => { - const dockerRun = vi.fn(() => ({ - status: 0, - stdout: `\n \n${["aaaa000000000000", "openshell", "default", "sb-real"].join("\t")}\n`, - })); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); - }); - - it("filters ONLY on the sandbox-name label so foreign containers stay visible", () => { - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); - classifyDestroyContainerIdentity("destroytest", { dockerRun }); - const argv = dockerRun.mock.calls[0][0] as string[]; - expect(argv).toContain("label=openshell.ai/sandbox-name=destroytest"); - expect(argv.some((a) => a.includes("managed-by="))).toBe(false); + it("passes a probe failure through as a fail-closed verdict (ambiguity unprovable)", () => { + const verdict = classifyDestroyContainerIdentity("destroytest", { + status: "probe-failed", + detail: "Cannot connect to the Docker daemon", + }); + expect(verdict.status).toBe("probe-failed"); }); }); describe("formatAmbiguousDestroyIdentity", () => { it("names the refusal, both container roles with sandbox-id, and neutral recovery", () => { - const verdict = classifyDestroyContainerIdentity("destroytest", { - dockerRun: () => fakeDockerRun([MANAGED, FOREIGN]), - }); + const verdict = classifyDestroyContainerIdentity("destroytest", ok([MANAGED, FOREIGN])); expectAmbiguous(verdict); const lines = formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n"); expect(lines).toContain("Refusing to destroy sandbox 'destroytest'"); diff --git a/src/lib/actions/sandbox/destroy-container-identity.ts b/src/lib/actions/sandbox/destroy-container-identity.ts index 55d13e44463..7325c5b79d6 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.ts @@ -1,7 +1,11 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { dockerRun } from "../../adapters/docker"; +import { + OPENSHELL_SANDBOX_WORKSPACE_LABEL, + type SandboxIdentityProbe, + type SandboxNameLabeledContainer, +} from "../../adapters/docker/sandbox-identity"; import { OPENSHELL_MANAGED_BY_LABEL, OPENSHELL_MANAGED_BY_VALUE, @@ -9,18 +13,12 @@ import { OPENSHELL_SANDBOX_NAME_LABEL, } from "../../onboard/openshell-docker-sandbox-containers"; -/** Workspace label OpenShell stamps on every managed sandbox container. */ -export const OPENSHELL_SANDBOX_WORKSPACE_LABEL = "openshell.ai/sandbox-workspace"; - -const DOCKER_IDENTITY_PROBE_TIMEOUT_MS = 30_000; - -/** One container carrying the destroy target's `sandbox-name` label. */ -export type SandboxNameLabeledContainer = { - id: string; - managedBy: string; - workspace: string; - sandboxId: string; -}; +export { + OPENSHELL_SANDBOX_WORKSPACE_LABEL, + probeSandboxNameContainers, + type SandboxIdentityProbe, + type SandboxNameLabeledContainer, +} from "../../adapters/docker/sandbox-identity"; /** * Verdict for whether the `sandbox-name` a destroy targets maps to a single @@ -33,8 +31,9 @@ export type SandboxNameLabeledContainer = { * containers spanning more than one workspace / sandbox-id). Destroy must * fail closed so it never removes the real sandbox behind an impostor's name. * - `probe-failed` — Docker could not be queried, so ambiguity can neither be - * proven nor ruled out. Non-blocking: the destroy's existing lower-layer - * guards remain in force. + * proven nor ruled out. The action fails closed on this: the lower layer can + * still delete through the gateway, so an unverifiable identity must not be + * allowed to proceed (#8999). */ export type DestroyContainerIdentityVerdict = | { status: "clear" } @@ -47,80 +46,26 @@ export type DestroyContainerIdentityVerdict = managed: SandboxNameLabeledContainer[]; }; -export type ClassifyDestroyContainerIdentityDeps = { - dockerRun?: typeof dockerRun; -}; - -const IDENTITY_FORMAT = [ - "{{.ID}}", - `{{.Label "${OPENSHELL_MANAGED_BY_LABEL}"}}`, - `{{.Label "${OPENSHELL_SANDBOX_WORKSPACE_LABEL}"}}`, - `{{.Label "${OPENSHELL_SANDBOX_ID_LABEL}"}}`, -].join("\t"); - -function resultText(result: { - stdout?: string | Buffer | null; - stderr?: string | Buffer | null; -}): string { - return `${String(result.stderr || "")} ${String(result.stdout || "")}`.trim(); -} - -function parseIdentityRows(stdout: string): SandboxNameLabeledContainer[] { - return stdout - .split(/\r?\n/u) - .map((line) => line.trim()) - .filter(Boolean) - .map((line) => { - const [id = "", managedBy = "", workspace = "", sandboxId = ""] = line.split("\t"); - return { - id: id.trim(), - managedBy: managedBy.trim(), - workspace: workspace.trim(), - sandboxId: sandboxId.trim(), - }; - }) - .filter((row) => row.id.length > 0); -} - /** - * Classify every Docker container carrying `openshell.ai/sandbox-name=` - * to decide whether the destroy target resolves to a single managed identity. + * Classify the containers a Docker probe found for a target `sandbox-name` and + * decide whether the destroy resolves to a single managed identity. * - * The lookup deliberately filters ONLY on the mutable `sandbox-name` label — - * not on `managed-by=openshell` — so a foreign container that borrows the name - * without the managed marker is still seen. A genuine managed sandbox carries - * `managed-by=openshell` and one consistent `sandbox-workspace` / `sandbox-id`; - * anything else sharing the name makes the identity ambiguous. + * Pure over an explicit probe result — the Docker call and its output parsing + * live in the `probeSandboxNameContainers` adapter, so this function makes only + * the identity decision and is trivially dependency-free to test. A genuine + * managed sandbox carries `managed-by=openshell` and one consistent + * `sandbox-workspace` / `sandbox-id`; anything else sharing the name makes the + * identity ambiguous. */ export function classifyDestroyContainerIdentity( sandboxName: string, - deps: ClassifyDestroyContainerIdentityDeps = {}, + probe: SandboxIdentityProbe, ): DestroyContainerIdentityVerdict { - const run = deps.dockerRun ?? dockerRun; - const result = run( - [ - "ps", - "-a", - "--no-trunc", - "--filter", - `label=${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, - "--format", - IDENTITY_FORMAT, - ], - { - ignoreError: true, - suppressOutput: true, - timeout: DOCKER_IDENTITY_PROBE_TIMEOUT_MS, - }, - ); - if (Number(result.status ?? 1) !== 0) { - return { - status: "probe-failed", - detail: resultText(result) || "docker ps did not complete successfully", - }; + if (probe.status === "probe-failed") { + return { status: "probe-failed", detail: probe.detail }; } - const rows = parseIdentityRows(String(result.stdout ?? "")); + const rows = probe.rows; if (rows.length === 0) return { status: "clear" }; const managed = rows.filter((row) => row.managedBy === OPENSHELL_MANAGED_BY_VALUE); diff --git a/src/lib/actions/sandbox/destroy.test.ts b/src/lib/actions/sandbox/destroy.test.ts index de3c33e228f..469b26f4d0b 100644 --- a/src/lib/actions/sandbox/destroy.test.ts +++ b/src/lib/actions/sandbox/destroy.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { describe, expect, it, vi } from "vitest"; +import { classifyDestroyContainerIdentity } from "./destroy-container-identity"; import { assertUnambiguousDestroyContainerIdentity, cleanupSandboxServices } from "./destroy"; const SANDBOX = "mybox"; @@ -84,13 +85,16 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { managed: [{ id: "aaaa", managedBy: "openshell", workspace: "default", sandboxId: "sb" }], })); + const probe = vi.fn(() => ({ status: "ok" as const, rows: [] })); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, + probe: probe as never, classify: classify as never, }); expect(proceed).toBe(false); - expect(classify).toHaveBeenCalledWith("destroytest"); + expect(probe).toHaveBeenCalledWith("destroytest"); + expect(classify).toHaveBeenCalledWith("destroytest", { status: "ok", rows: [] }); expect(error).toHaveBeenCalled(); error.mockRestore(); }); @@ -100,26 +104,32 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { expect( assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, + probe: vi.fn(() => ({ status: "ok" as const, rows: [] })) as never, classify: classify as never, }), ).toBe(true); }); it("does not probe or block a non-Docker runtime provider", () => { + const probe = vi.fn(); const classify = vi.fn(); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => ({ openshellDriver: "podman" })) as never, + probe: probe as never, classify: classify as never, }); expect(proceed).toBe(true); + expect(probe).not.toHaveBeenCalled(); expect(classify).not.toHaveBeenCalled(); }); it("fails closed when the Docker probe cannot prove identity (#8999)", () => { const warn = vi.fn(); + const probe = vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, - classify: vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })) as never, + probe: probe as never, + classify: classifyDestroyContainerIdentity, warn, }); expect(proceed).toBe(false); @@ -127,11 +137,14 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { }); it("treats an unknown/null driver as Docker (the default) and still guards", () => { + const probe = vi.fn(() => ({ status: "ok" as const, rows: [] })); const classify = vi.fn(() => ({ status: "clear" as const })); assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => ({ openshellDriver: null })) as never, + probe: probe as never, classify: classify as never, }); - expect(classify).toHaveBeenCalledWith("destroytest"); + expect(probe).toHaveBeenCalledWith("destroytest"); + expect(classify).toHaveBeenCalledWith("destroytest", { status: "ok", rows: [] }); }); }); diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 6711d7de66a..308db2dc925 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -45,6 +45,7 @@ import { classifyDestroyContainerIdentity, type DestroyContainerIdentityVerdict, formatAmbiguousDestroyIdentity, + probeSandboxNameContainers, } from "./destroy-container-identity"; import { executeSandboxDestroy, @@ -465,6 +466,7 @@ export async function destroySandbox( export type AssertUnambiguousDestroyIdentityDeps = { getSandbox?: typeof registry.getSandbox; + probe?: typeof probeSandboxNameContainers; classify?: typeof classifyDestroyContainerIdentity; warn?: (message: string) => void; }; @@ -489,12 +491,13 @@ export function assertUnambiguousDestroyContainerIdentity( deps: AssertUnambiguousDestroyIdentityDeps = {}, ): boolean { const getSandbox = deps.getSandbox ?? registry.getSandbox; + const probe = deps.probe ?? probeSandboxNameContainers; const classify = deps.classify ?? classifyDestroyContainerIdentity; const warn = deps.warn ?? defaultDestroyWarn; const providerId = normalizeRuntimeProviderIdentity(getSandbox(sandboxName)?.openshellDriver); if (providerId !== "docker") return true; - const verdict: DestroyContainerIdentityVerdict = classify(sandboxName); + const verdict: DestroyContainerIdentityVerdict = classify(sandboxName, probe(sandboxName)); if (verdict.status === "ambiguous") { for (const line of formatAmbiguousDestroyIdentity(verdict, CLI_NAME)) { console.error(` ${line}`); diff --git a/src/lib/adapters/docker/index.ts b/src/lib/adapters/docker/index.ts index fc6d4250ba7..9a3d32418b7 100644 --- a/src/lib/adapters/docker/index.ts +++ b/src/lib/adapters/docker/index.ts @@ -11,3 +11,4 @@ export * from "./image"; export * from "./container"; export * from "./volume"; export * from "./login"; +export * from "./sandbox-identity"; diff --git a/src/lib/adapters/docker/sandbox-identity.test.ts b/src/lib/adapters/docker/sandbox-identity.test.ts new file mode 100644 index 00000000000..5f8ca85fbb7 --- /dev/null +++ b/src/lib/adapters/docker/sandbox-identity.test.ts @@ -0,0 +1,50 @@ +// 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 { probeSandboxNameContainers, type SandboxIdentityProbe } from "./sandbox-identity"; + +// Narrowing assertion helper keeps test bodies linear (no branching). +function expectOk( + probe: SandboxIdentityProbe, +): asserts probe is Extract { + expect(probe.status).toBe("ok"); +} + +const MANAGED_LINE = ["aaaa000000000000", "openshell", "default", "sb-real"].join("\t"); + +describe("probeSandboxNameContainers", () => { + it("parses the tab-separated identity rows Docker returns", () => { + const dockerRun = vi.fn(() => ({ status: 0, stdout: MANAGED_LINE })); + const probe = probeSandboxNameContainers("destroytest", { dockerRun } as never); + expectOk(probe); + expect(probe.rows).toEqual([ + { id: "aaaa000000000000", managedBy: "openshell", workspace: "default", sandboxId: "sb-real" }, + ]); + }); + + it("ignores blank and malformed lines without inventing rows", () => { + const dockerRun = vi.fn(() => ({ status: 0, stdout: `\n \n${MANAGED_LINE}\n` })); + const probe = probeSandboxNameContainers("destroytest", { dockerRun } as never); + expectOk(probe); + expect(probe.rows).toHaveLength(1); + }); + + it("filters ONLY on the sandbox-name label so foreign containers stay visible", () => { + const dockerRun = vi.fn((_args: readonly string[], _opts?: unknown) => ({ status: 0, stdout: "" })); + probeSandboxNameContainers("destroytest", { dockerRun } as never); + const argv = dockerRun.mock.calls[0][0] as readonly string[]; + expect(argv).toContain("label=openshell.ai/sandbox-name=destroytest"); + expect(argv.some((a) => a.includes("managed-by="))).toBe(false); + }); + + it("reports a probe failure when docker ps exits non-zero", () => { + const dockerRun = vi.fn(() => ({ status: 1, stdout: "", stderr: "Cannot connect to daemon" })); + const probe = probeSandboxNameContainers("destroytest", { dockerRun } as never); + expect(probe.status).toBe("probe-failed"); + expect((probe as Extract).detail).toContain( + "daemon", + ); + }); +}); diff --git a/src/lib/adapters/docker/sandbox-identity.ts b/src/lib/adapters/docker/sandbox-identity.ts new file mode 100644 index 00000000000..4775cde570c --- /dev/null +++ b/src/lib/adapters/docker/sandbox-identity.ts @@ -0,0 +1,108 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { + OPENSHELL_MANAGED_BY_LABEL, + OPENSHELL_SANDBOX_ID_LABEL, + OPENSHELL_SANDBOX_NAME_LABEL, +} from "../../onboard/openshell-docker-sandbox-containers"; +import { dockerRun } from "./run"; + +/** Workspace label OpenShell stamps on every managed sandbox container. */ +export const OPENSHELL_SANDBOX_WORKSPACE_LABEL = "openshell.ai/sandbox-workspace"; + +const DOCKER_IDENTITY_PROBE_TIMEOUT_MS = 30_000; + +/** One container carrying the destroy target's `sandbox-name` label. */ +export type SandboxNameLabeledContainer = { + id: string; + managedBy: string; + workspace: string; + sandboxId: string; +}; + +/** + * Result of probing Docker for every container carrying a given `sandbox-name`. + * + * - `ok` — Docker answered; `rows` is every labeled container (possibly empty). + * - `probe-failed` — Docker could not be queried, so identity can neither be + * proven nor ruled out. The classifier turns this into a fail-closed verdict. + */ +export type SandboxIdentityProbe = + | { status: "ok"; rows: SandboxNameLabeledContainer[] } + | { status: "probe-failed"; detail: string }; + +export type ProbeSandboxNameContainersDeps = { + dockerRun?: typeof dockerRun; +}; + +const IDENTITY_FORMAT = [ + "{{.ID}}", + `{{.Label "${OPENSHELL_MANAGED_BY_LABEL}"}}`, + `{{.Label "${OPENSHELL_SANDBOX_WORKSPACE_LABEL}"}}`, + `{{.Label "${OPENSHELL_SANDBOX_ID_LABEL}"}}`, +].join("\t"); + +function resultText(result: { + stdout?: string | Buffer | null; + stderr?: string | Buffer | null; +}): string { + return `${String(result.stderr || "")} ${String(result.stdout || "")}`.trim(); +} + +function parseIdentityRows(stdout: string): SandboxNameLabeledContainer[] { + return stdout + .split(/\r?\n/u) + .map((line) => line.trim()) + .filter(Boolean) + .map((line) => { + const [id = "", managedBy = "", workspace = "", sandboxId = ""] = line.split("\t"); + return { + id: id.trim(), + managedBy: managedBy.trim(), + workspace: workspace.trim(), + sandboxId: sandboxId.trim(), + }; + }) + .filter((row) => row.id.length > 0); +} + +/** + * Query every Docker container carrying `openshell.ai/sandbox-name=` and + * return the parsed identity rows (or a probe failure). + * + * The lookup deliberately filters ONLY on the mutable `sandbox-name` label — + * not on `managed-by=openshell` — so a foreign container that borrows the name + * without the managed marker stays visible to the classifier. This adapter owns + * the host/process boundary (the `docker ps` call and its output parsing); the + * classification decision over these rows is a separate pure function. + */ +export function probeSandboxNameContainers( + sandboxName: string, + deps: ProbeSandboxNameContainersDeps = {}, +): SandboxIdentityProbe { + const run = deps.dockerRun ?? dockerRun; + const result = run( + [ + "ps", + "-a", + "--no-trunc", + "--filter", + `label=${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, + "--format", + IDENTITY_FORMAT, + ], + { + ignoreError: true, + suppressOutput: true, + timeout: DOCKER_IDENTITY_PROBE_TIMEOUT_MS, + }, + ); + if (Number(result.status ?? 1) !== 0) { + return { + status: "probe-failed", + detail: resultText(result) || "docker ps did not complete successfully", + }; + } + return { status: "ok", rows: parseIdentityRows(String(result.stdout ?? "")) }; +} From 4541797919726d892c386198661f09407503e335 Mon Sep 17 00:00:00 2001 From: Aarav Sharma Date: Thu, 13 Aug 2026 16:12:37 -0700 Subject: [PATCH 04/11] test(sandbox): add flow-level destroy-identity refusal test + doc the refusal (#8999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address follow-up review on PR #9042: - Flow-level negative test: drive the public destroySandbox path with an ambiguous Docker identity (foreign container borrowing the sandbox-name label) and assert it refuses before any destructive step — process exits non-zero, sandbox delete is never reached, and the registry row is kept. The harness gains a sandboxIdentityProbeOutput option feeding the identity probe (docker ps -a), so removing the guard's exit would now fail a test. - Assert the probe passes -a in the adapter command-contract test so a stopped foreign container with the same label stays visible (CodeRabbit). - Document the destroy identity refusal in the command reference: it runs before deletion regardless of --force/--yes/NEMOCLAW_NON_INTERACTIVE, and a probe failure also fails closed. Signed-off-by: Aarav Sharma --- docs/reference/commands.mdx | 5 ++++ src/lib/actions/sandbox/destroy-flow.test.ts | 23 +++++++++++++++++++ .../adapters/docker/sandbox-identity.test.ts | 3 +++ test/helpers/destroy-flow-test-harness.ts | 17 +++++++++++--- 4 files changed, 45 insertions(+), 3 deletions(-) diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 870fa56bec5..a014f461d4a 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2207,6 +2207,11 @@ If you want to upgrade the sandbox while preserving state, use `$$nemoclaw If the Hermes sandbox has managed MCP entries, shields must be down before destroy can scrub their adapter configuration. diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 97986044577..aa25d24385e 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -428,4 +428,27 @@ describe("destroySandbox flow", () => { harness.runOpenshellSpy, ); }); + + it("refuses destroy and never reaches sandbox delete when a foreign container shares the sandbox-name label (#8999)", async () => { + // The full public path, not just the guard unit: an ambiguous Docker + // identity must abort before any destructive step. The probe returns a real + // managed sandbox plus a foreign container that borrows the sandbox-name + // label (no managed marker, different workspace). + const harness = createDestroyHarness({ + openshellDriver: "docker", + sandboxIdentityProbeOutput: + "alpha-managed\topenshell\tdefault\tsb-real\nalpha-foreign\t\tforeign\t\n", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(harness.events).not.toContain("delete"); + const reachedDelete = harness.runOpenshellSpy.mock.calls.some( + ([args]) => Array.isArray(args) && args[0] === "sandbox" && args[1] === "delete", + ); + expect(reachedDelete).toBe(false); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.errorSpy).toHaveBeenCalled(); + }); }); diff --git a/src/lib/adapters/docker/sandbox-identity.test.ts b/src/lib/adapters/docker/sandbox-identity.test.ts index 5f8ca85fbb7..96bc291a749 100644 --- a/src/lib/adapters/docker/sandbox-identity.test.ts +++ b/src/lib/adapters/docker/sandbox-identity.test.ts @@ -35,6 +35,9 @@ describe("probeSandboxNameContainers", () => { const dockerRun = vi.fn((_args: readonly string[], _opts?: unknown) => ({ status: 0, stdout: "" })); probeSandboxNameContainers("destroytest", { dockerRun } as never); const argv = dockerRun.mock.calls[0][0] as readonly string[]; + // `-a` keeps stopped containers visible: a stopped foreign container that + // borrows the sandbox-name label must still make the identity ambiguous. + expect(argv).toContain("-a"); expect(argv).toContain("label=openshell.ai/sandbox-name=destroytest"); expect(argv.some((a) => a.includes("managed-by="))).toBe(false); }); diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index cd7b37f6aa7..edb5fa9bc58 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -64,6 +64,7 @@ type DestroyHarnessOptions = { registeredSandboxCount?: number; restoreMcpError?: string; sandboxPresent?: boolean; + sandboxIdentityProbeOutput?: string; shieldsDown?: boolean; shieldsUpError?: Error; workload?: SandboxWorkloadReceipt; @@ -240,9 +241,19 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr : names; return matchedNames.length > 0 ? `${matchedNames.join("\n")}\n` : ""; }); - const dockerRunSpy = vi - .spyOn(dockerRun, "dockerRun") - .mockReturnValue({ status: 0 } as ReturnType); + // The destroy identity guard (#8999) probes `docker ps -a --filter + // label=...sandbox-name=` before any destructive step. Feed that probe + // from `sandboxIdentityProbeOutput` (raw tab-separated rows) so a flow test + // can exercise an ambiguous identity; every other dockerRun stays a no-op. + const dockerRunSpy = vi.spyOn(dockerRun, "dockerRun").mockImplementation((args: unknown) => { + const argv = Array.isArray(args) ? args.map(String) : []; + const isIdentityProbe = + argv[0] === "ps" && argv.some((a) => a.includes("label=openshell.ai/sandbox-name=")); + return { + status: 0, + stdout: isIdentityProbe ? (options.sandboxIdentityProbeOutput ?? "") : "", + } as ReturnType; + }); const selectGatewaySpy = vi .spyOn(destroyGateway, "selectGatewayForSandboxDestroy") .mockImplementation(() => undefined); From e763576f87461f90a30cb1211b153f457185e2b7 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 13 Aug 2026 16:23:43 -0700 Subject: [PATCH 05/11] fix(sandbox): preserve destroy container identity Signed-off-by: Apurv Kumaria --- docs/reference/commands.mdx | 35 +++ .../destroy-container-identity.test.ts | 155 ++++++++---- .../sandbox/destroy-container-identity.ts | 183 -------------- src/lib/actions/sandbox/destroy-execution.ts | 81 ++++++ src/lib/actions/sandbox/destroy-flow.test.ts | 238 +++++++++++++++++- src/lib/actions/sandbox/destroy-preflight.ts | 6 +- src/lib/actions/sandbox/destroy-presence.ts | 170 +++++++++++++ src/lib/actions/sandbox/destroy.test.ts | 31 ++- src/lib/actions/sandbox/destroy.ts | 93 +++++-- .../adapters/docker/inspect-identity.test.ts | 59 +++++ src/lib/adapters/docker/inspect.ts | 80 ++++++ test/helpers/destroy-flow-test-harness.ts | 24 +- 12 files changed, 863 insertions(+), 292 deletions(-) delete mode 100644 src/lib/actions/sandbox/destroy-container-identity.ts create mode 100644 src/lib/adapters/docker/inspect-identity.test.ts diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index acabd927416..d59ea0df7e6 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2215,6 +2215,41 @@ If you want to upgrade the sandbox while preserving state, use `$$nemoclaw +Do not remove or recreate a container until you verify its purpose, ownership, and data-retention requirements. +Removing or recreating a container can discard state that is not stored in a volume. + + +Resolve a conflict through the workflow that created the conflicting container. +Docker cannot change labels on an existing container. +Rerun the query after you resolve the conflict. +Rerun `destroy` only when the query returns one complete expected label set that you verified belongs to the target sandbox, or no containers after you independently confirm that the sandbox is absent. + If the Hermes sandbox has managed MCP entries, shields must be down before destroy can scrub their adapter configuration. diff --git a/src/lib/actions/sandbox/destroy-container-identity.test.ts b/src/lib/actions/sandbox/destroy-container-identity.test.ts index 3695d893fcf..1fb4e115df7 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.test.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.test.ts @@ -5,8 +5,9 @@ import { describe, expect, it, vi } from "vitest"; import { classifyDestroyContainerIdentity, + type DestroyContainerIdentityVerdict, formatAmbiguousDestroyIdentity, -} from "./destroy-container-identity"; +} from "./destroy-presence"; type Row = { id: string; @@ -15,11 +16,17 @@ type Row = { sandboxId?: string; }; -function fakeDockerRun(rows: Row[], status = 0): { status: number; stdout: string } { - const stdout = rows - .map((r) => [r.id, r.managedBy ?? "", r.workspace ?? "", r.sandboxId ?? ""].join("\t")) - .join("\n"); - return { status, stdout }; +function observeRows(rows: Row[], malformedRows = 0) { + return { + status: "observed" as const, + rows: rows.map((row) => ({ + id: row.id, + managedBy: row.managedBy ?? "", + workspace: row.workspace ?? "", + sandboxId: row.sandboxId ?? "", + })), + malformedRows, + }; } const MANAGED = { @@ -36,24 +43,34 @@ const FOREIGN = { sandboxId: "", } as const; +function expectAmbiguous( + verdict: DestroyContainerIdentityVerdict, +): Extract { + expect(verdict.status).toBe("ambiguous"); + return verdict as Extract; +} + describe("classifyDestroyContainerIdentity", () => { it("is clear when no container carries the sandbox-name label", () => { - const dockerRun = vi.fn(() => fakeDockerRun([])); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + expect(classifyDestroyContainerIdentity("destroytest", observeRows([]))).toEqual({ + status: "clear", + identity: null, + }); }); it("is clear for exactly one managed container", () => { - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + expect(classifyDestroyContainerIdentity("destroytest", observeRows([MANAGED]))).toEqual({ + status: "clear", + identity: MANAGED, + }); }); - it("refuses when a foreign container shares the sandbox-name label (#8999 repro)", () => { + it("refuses when a foreign container shares the sandbox-name label (#8999)", () => { // The exact repro: a real managed sandbox plus a busybox that borrows the // sandbox-name label with a different workspace and no managed marker. - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED, FOREIGN])); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([MANAGED, FOREIGN])), + ); expect(verdict.foreign).toHaveLength(1); expect(verdict.foreign[0].id).toBe(FOREIGN.id); expect(verdict.managed).toHaveLength(1); @@ -61,72 +78,102 @@ describe("classifyDestroyContainerIdentity", () => { }); it("refuses a foreign-only match with no managed container behind it", () => { - const dockerRun = vi.fn(() => fakeDockerRun([FOREIGN])); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([FOREIGN])), + ); expect(verdict.managed).toHaveLength(0); expect(verdict.foreign).toHaveLength(1); }); it("refuses when managed containers span more than one workspace", () => { - const dockerRun = vi.fn(() => - fakeDockerRun([ + const observe = vi.fn(() => + observeRows([ MANAGED, - { id: "bbbb000000000000", managedBy: "openshell", workspace: "other", sandboxId: "sb-real" }, + { + id: "bbbb000000000000", + managedBy: "openshell", + workspace: "other", + sandboxId: "sb-real", + }, ]), ); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("ambiguous"); - if (verdict.status !== "ambiguous") throw new Error("unreachable"); - expect(verdict.reason).toContain("workspace"); + const verdict = expectAmbiguous(classifyDestroyContainerIdentity("destroytest", observe())); + expect(verdict.reason).toContain("2 managed containers"); }); it("refuses when managed containers span more than one sandbox-id", () => { - const dockerRun = vi.fn(() => - fakeDockerRun([ + const observe = vi.fn(() => + observeRows([ MANAGED, - { id: "cccc000000000000", managedBy: "openshell", workspace: "default", sandboxId: "sb-two" }, + { + id: "cccc000000000000", + managedBy: "openshell", + workspace: "default", + sandboxId: "sb-two", + }, ]), ); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("ambiguous"); + expect(classifyDestroyContainerIdentity("destroytest", observe()).status).toBe("ambiguous"); + }); + + it("refuses multiple managed containers even when their mutable labels match", () => { + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity( + "destroytest", + observeRows([MANAGED, { ...MANAGED, id: "dddd000000000000" }]), + ), + ); + expect(verdict.reason).toContain("2 managed containers"); + }); + + it.each([ + ["workspace", { ...MANAGED, workspace: "" }], + ["sandbox ID", { ...MANAGED, sandboxId: "" }], + ])("refuses a managed container with no %s", (_label, row) => { + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([row])), + ); + expect(verdict.reason).toContain("missing"); }); - it("does not block when the Docker probe fails (ambiguity unprovable)", () => { - const dockerRun = vi.fn(() => ({ status: 1, stdout: "", stderr: "Cannot connect to daemon" })); - const verdict = classifyDestroyContainerIdentity("destroytest", { dockerRun }); - expect(verdict.status).toBe("probe-failed"); - if (verdict.status !== "probe-failed") throw new Error("unreachable"); - expect(verdict.detail).toContain("daemon"); + it("refuses malformed Docker identity output", () => { + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([], 1)), + ); + expect(verdict.reason).toContain("malformed container identity"); }); - it("ignores blank and malformed lines without misclassifying", () => { - const dockerRun = vi.fn(() => ({ - status: 0, - stdout: `\n \n${["aaaa000000000000", "openshell", "default", "sb-real"].join("\t")}\n`, - })); - expect(classifyDestroyContainerIdentity("destroytest", { dockerRun }).status).toBe("clear"); + it("refuses terminal-control label output without rendering it", () => { + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([], 1)), + ); + expect(formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n")).not.toContain("\u001b"); }); - it("filters ONLY on the sandbox-name label so foreign containers stay visible", () => { - const dockerRun = vi.fn(() => fakeDockerRun([MANAGED])); - classifyDestroyContainerIdentity("destroytest", { dockerRun }); - const argv = dockerRun.mock.calls[0][0] as string[]; - expect(argv).toContain("label=openshell.ai/sandbox-name=destroytest"); - expect(argv.some((a) => a.includes("managed-by="))).toBe(false); + it("reports a failed Docker probe when identity cannot be proven", () => { + const verdict = classifyDestroyContainerIdentity("destroytest", { + status: "probe-failed", + detail: "Cannot connect to daemon", + }); + expect(verdict).toEqual({ + status: "probe-failed", + detail: expect.stringContaining("daemon"), + }); }); }); describe("formatAmbiguousDestroyIdentity", () => { it("names the refusal, both container roles, and the recovery step", () => { - const verdict = classifyDestroyContainerIdentity("destroytest", { - dockerRun: () => fakeDockerRun([MANAGED, FOREIGN]), - }); - if (verdict.status !== "ambiguous") throw new Error("expected ambiguous verdict"); + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([MANAGED, FOREIGN])), + ); const lines = formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n"); expect(lines).toContain("Refusing to destroy sandbox 'destroytest'"); - expect(lines).toContain("Unexpected container:"); + expect(lines).toContain("Conflicting container:"); expect(lines).toContain("Managed sandbox container:"); - expect(lines).toContain("nemoclaw destroytest destroy --yes"); + expect(lines).toContain("openshell.ai/sandbox-id=sb-real"); + expect(lines).toContain("Resolve the conflict through the workflow that owns the container"); + expect(lines).toContain("nemoclaw destroytest destroy"); + expect(lines).not.toContain("--yes"); }); }); diff --git a/src/lib/actions/sandbox/destroy-container-identity.ts b/src/lib/actions/sandbox/destroy-container-identity.ts deleted file mode 100644 index 166b29f0411..00000000000 --- a/src/lib/actions/sandbox/destroy-container-identity.ts +++ /dev/null @@ -1,183 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -import { dockerRun } from "../../adapters/docker"; -import { - OPENSHELL_MANAGED_BY_LABEL, - OPENSHELL_MANAGED_BY_VALUE, - OPENSHELL_SANDBOX_ID_LABEL, - OPENSHELL_SANDBOX_NAME_LABEL, -} from "../../onboard/openshell-docker-sandbox-containers"; - -/** Workspace label OpenShell stamps on every managed sandbox container. */ -export const OPENSHELL_SANDBOX_WORKSPACE_LABEL = "openshell.ai/sandbox-workspace"; - -const DOCKER_IDENTITY_PROBE_TIMEOUT_MS = 30_000; - -/** One container carrying the destroy target's `sandbox-name` label. */ -export type SandboxNameLabeledContainer = { - id: string; - managedBy: string; - workspace: string; - sandboxId: string; -}; - -/** - * Verdict for whether the `sandbox-name` a destroy targets maps to a single - * unambiguous managed container identity. - * - * - `clear` — no labeled container, or exactly one managed identity. Destroy - * may proceed. - * - `ambiguous` — a container claims the target `sandbox-name` but is not the - * single managed sandbox (a foreign container carrying the label, or managed - * containers spanning more than one workspace / sandbox-id). Destroy must - * fail closed so it never removes the real sandbox behind an impostor's name. - * - `probe-failed` — Docker could not be queried, so ambiguity can neither be - * proven nor ruled out. Non-blocking: the destroy's existing lower-layer - * guards remain in force. - */ -export type DestroyContainerIdentityVerdict = - | { status: "clear" } - | { status: "probe-failed"; detail: string } - | { - status: "ambiguous"; - sandboxName: string; - reason: string; - foreign: SandboxNameLabeledContainer[]; - managed: SandboxNameLabeledContainer[]; - }; - -export type ClassifyDestroyContainerIdentityDeps = { - dockerRun?: typeof dockerRun; -}; - -const IDENTITY_FORMAT = [ - "{{.ID}}", - `{{.Label "${OPENSHELL_MANAGED_BY_LABEL}"}}`, - `{{.Label "${OPENSHELL_SANDBOX_WORKSPACE_LABEL}"}}`, - `{{.Label "${OPENSHELL_SANDBOX_ID_LABEL}"}}`, -].join("\t"); - -function resultText(result: { - stdout?: string | Buffer | null; - stderr?: string | Buffer | null; -}): string { - return `${String(result.stderr || "")} ${String(result.stdout || "")}`.trim(); -} - -function parseIdentityRows(stdout: string): SandboxNameLabeledContainer[] { - return stdout - .split(/\r?\n/u) - .map((line) => line.trim()) - .filter(Boolean) - .map((line) => { - const [id = "", managedBy = "", workspace = "", sandboxId = ""] = line.split("\t"); - return { - id: id.trim(), - managedBy: managedBy.trim(), - workspace: workspace.trim(), - sandboxId: sandboxId.trim(), - }; - }) - .filter((row) => row.id.length > 0); -} - -/** - * Classify every Docker container carrying `openshell.ai/sandbox-name=` - * to decide whether the destroy target resolves to a single managed identity. - * - * The lookup deliberately filters ONLY on the mutable `sandbox-name` label — - * not on `managed-by=openshell` — so a foreign container that borrows the name - * without the managed marker is still seen. A genuine managed sandbox carries - * `managed-by=openshell` and one consistent `sandbox-workspace` / `sandbox-id`; - * anything else sharing the name makes the identity ambiguous. - */ -export function classifyDestroyContainerIdentity( - sandboxName: string, - deps: ClassifyDestroyContainerIdentityDeps = {}, -): DestroyContainerIdentityVerdict { - const run = deps.dockerRun ?? dockerRun; - const result = run( - [ - "ps", - "-a", - "--no-trunc", - "--filter", - `label=${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, - "--format", - IDENTITY_FORMAT, - ], - { - ignoreError: true, - suppressOutput: true, - timeout: DOCKER_IDENTITY_PROBE_TIMEOUT_MS, - }, - ); - if (Number(result.status ?? 1) !== 0) { - return { - status: "probe-failed", - detail: resultText(result) || "docker ps did not complete successfully", - }; - } - - const rows = parseIdentityRows(String(result.stdout ?? "")); - if (rows.length === 0) return { status: "clear" }; - - const managed = rows.filter((row) => row.managedBy === OPENSHELL_MANAGED_BY_VALUE); - const foreign = rows.filter((row) => row.managedBy !== OPENSHELL_MANAGED_BY_VALUE); - - if (foreign.length > 0) { - return { - status: "ambiguous", - sandboxName, - reason: - `${String(foreign.length)} container(s) carry the '${OPENSHELL_SANDBOX_NAME_LABEL}=` + - `${sandboxName}' label without the '${OPENSHELL_MANAGED_BY_LABEL}=` + - `${OPENSHELL_MANAGED_BY_VALUE}' marker`, - foreign, - managed, - }; - } - - const workspaces = new Set(managed.map((row) => row.workspace)); - const sandboxIds = new Set(managed.map((row) => row.sandboxId).filter(Boolean)); - if (workspaces.size > 1 || sandboxIds.size > 1) { - return { - status: "ambiguous", - sandboxName, - reason: - `managed containers for '${sandboxName}' span ${String(workspaces.size)} workspace(s) ` + - `and ${String(sandboxIds.size)} sandbox-id(s)`, - foreign, - managed, - }; - } - - return { status: "clear" }; -} - -/** Human-readable lines describing an ambiguous-identity refusal. */ -export function formatAmbiguousDestroyIdentity( - verdict: Extract, - cliName: string, -): string[] { - const describe = (row: SandboxNameLabeledContainer): string => - `${row.id.slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${row.managedBy || ""}, ` + - `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${row.workspace || ""})`; - const lines = [ - `Refusing to destroy sandbox '${verdict.sandboxName}': ${verdict.reason}.`, - "Destroy fails closed because the sandbox-name no longer identifies a single container, " + - "so it cannot prove which container it would remove.", - ]; - for (const row of verdict.foreign) { - lines.push(` Unexpected container: ${describe(row)}`); - } - for (const row of verdict.managed) { - lines.push(` Managed sandbox container: ${describe(row)}`); - } - lines.push( - "Remove or relabel the unexpected container(s), then re-run " + - `'${cliName} ${verdict.sandboxName} destroy --yes'.`, - ); - return lines; -} diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 0629dc97ef7..d481c06b586 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -18,6 +18,12 @@ import { redact, redactFull } from "../../security/redact"; import { withTimerBoundShieldsMutationLockAsync } from "../../shields/timer-bound-lock"; import { readTimerMarker } from "../../shields/timer-control"; import type { SandboxEntry } from "../../state/registry"; +import { + classifyDestroyContainerIdentity, + isSameDestroyContainerIdentity, + observeDestroyContainerIdentity, + type SandboxNameLabeledContainer, +} from "./destroy-presence"; import type { DestroyRunOpenshell } from "./destroy-gateway"; import { finalizeMcpBridgesAfterSandboxDelete, @@ -44,6 +50,8 @@ type SandboxDestroyExecutionInput = { sandbox: SandboxEntry | null; sandboxConfirmedAbsent: boolean; sandboxName: string; + expectedContainerIdentity?: SandboxNameLabeledContainer | null; + stopInferenceResources?: () => void; runtimeProviders?: RuntimeProviderBundleRegistry; deps?: { readTimerMarker?: typeof readTimerMarker; @@ -252,10 +260,33 @@ export async function executeSandboxDestroy({ sandbox, sandboxConfirmedAbsent, sandboxName, + expectedContainerIdentity, + stopInferenceResources = () => undefined, runtimeProviders = CURRENT_RUNTIME_PROVIDER_BUNDLES, deps = {}, }: SandboxDestroyExecutionInput): Promise { return withTimerBoundShieldsMutationLockAsync(sandboxName, "destroy sandbox", async () => { + const identityStillMatches = (): boolean => + expectedContainerIdentity === undefined || + isSameDestroyContainerIdentity( + expectedContainerIdentity, + classifyDestroyContainerIdentity(sandboxName, observeDestroyContainerIdentity(sandboxName)), + ); + const identityDriftResult = ( + phase: string, + mcpRecoveryFailure?: string, + ): SandboxDestroyExecutionResult => ({ + ok: false, + deleteOutput: `Docker container identity changed ${phase}; no sandbox delete was attempted.`, + exitCode: 1, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + mcpRecoveryFailure, + shieldsRelockRequiresGateway: false, + }); + if (!identityStillMatches()) { + return identityDriftResult("before destroy preparation"); + } let runtimeProvider: RuntimeProviderBundle | null = null; if (sandbox) { try { @@ -295,14 +326,64 @@ export async function executeSandboxDestroy({ // discarded during preparation. Remaining entries are the durable exact // provider ownership manifest and must survive an unconfirmed delete. const hasMcpOwnership = mcpPreparation.entries.length > 0; + if (!identityStillMatches()) { + const mcpRecoveryFailure = sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { + hardenedForDelete: false, + hardeningFailed: false, + }); + return identityDriftResult("during destroy preparation", mcpRecoveryFailure); + } + try { + stopInferenceResources(); + } catch (error) { + const mcpRecoveryFailure = sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { + hardenedForDelete: false, + hardeningFailed: false, + }); + return { + ok: false, + deleteOutput: + `Could not stop managed inference resources before sandbox deletion: ${redactDestroyError(error)}. ` + + "No workspace wipe, provider cleanup, or sandbox deletion was attempted.", + exitCode: 1, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + mcpRecoveryFailure, + shieldsRelockRequiresGateway: false, + }; + } const hardened = wipeAndHardenLiveSandbox(sandboxName, sandboxConfirmedAbsent, deps); const detachProviders = (): DetachSandboxProvidersResult => runSandboxProviderPreDeleteCleanup(sandboxName, { runOpenshell, redact }); + if (!identityStillMatches()) { + const mcpRecoveryFailure = sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); + return identityDriftResult("before provider cleanup", mcpRecoveryFailure); + } const detachOutcome: DetachSandboxProvidersResult = sandboxConfirmedAbsent ? { detached: [], failures: [] } : runtimeProvider?.cleanup.supported === true && sandbox ? runtimeProvider.cleanup.prepareDestroy({ sandbox, sandboxName }, { detachProviders }) : detachProviders(); + // The final exact proof runs immediately before OpenShell delete. A Docker- + // socket actor remains a trusted host authority; this closes the actionable + // multi-step window without claiming a cross-engine transaction. + if (!identityStillMatches()) { + const detachedDetail = + detachOutcome.detached.length > 0 + ? ` Provider cleanup detached ${detachOutcome.detached.join(", ")}; rerun the owning setup workflow to restore those attachments.` + : ""; + const mcpRecoveryFailure = sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); + const result = identityDriftResult("at the delete boundary", mcpRecoveryFailure); + return { ...result, deleteOutput: `${result.deleteOutput}${detachedDetail}` }; + } const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { ignoreError: true, stdio: ["ignore", "pipe", "pipe"], diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 97986044577..cae69a52c07 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -88,18 +88,21 @@ describe("destroySandbox flow", () => { false, ], ["NEMOCLAW_NON_INTERACTIVE=1", "linux", {}, "1", false], - ] as const)("applies the final-gateway default for %s on %s (#4662)", async (_scenario, platform, options, nonInteractive, cleanupExpected) => { - vi.spyOn(process, "platform", "get").mockReturnValue(platform); - vi.stubEnv("NEMOCLAW_NON_INTERACTIVE", nonInteractive); - const harness = createDestroyHarness(); - - await expect(harness.destroySandbox("alpha", options)).resolves.toBeUndefined(); - - expect(harness.promptSpy).not.toHaveBeenCalled(); - expect(harness.cleanupGatewaySpy.mock.calls).toEqual( - cleanupExpected ? [["nemoclaw-19080", harness.runOpenshellSpy]] : [], - ); - }); + ] as const)( + "applies the final-gateway default for %s on %s (#4662)", + async (_scenario, platform, options, nonInteractive, cleanupExpected) => { + vi.spyOn(process, "platform", "get").mockReturnValue(platform); + vi.stubEnv("NEMOCLAW_NON_INTERACTIVE", nonInteractive); + const harness = createDestroyHarness(); + + await expect(harness.destroySandbox("alpha", options)).resolves.toBeUndefined(); + + expect(harness.promptSpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy.mock.calls).toEqual( + cleanupExpected ? [["nemoclaw-19080", harness.runOpenshellSpy]] : [], + ); + }, + ); it("stops before local cleanup when OpenShell fails to delete the live sandbox", async () => { const harness = createDestroyHarness({ @@ -113,6 +116,217 @@ describe("destroySandbox flow", () => { expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); }); + it("refuses before destructive work when Docker identity cannot be inspected", async () => { + const harness = createDestroyHarness({ + dockerRunResult: { status: 1, stderr: "Docker daemon unavailable" }, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n")).toContain( + "Docker container identity could not be inspected", + ); + expect(harness.runOpenshellSpy).not.toHaveBeenCalled(); + expect(harness.events).toEqual([]); + expect(harness.selectGatewaySpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); + expect(harness.stopAllSpy).not.toHaveBeenCalled(); + expect(harness.killTimerSpy).not.toHaveBeenCalled(); + expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); + expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); + expect(harness.killStaleProxySpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledOnce(); + }); + + it("refuses before destructive work when multiple Docker identities share the name", async () => { + const rows = [ + "aaaa000000000000\topenshell\tdefault\tsb-alpha", + "ffff000000000000\t\tforeign\t", + ].join("\n"); + const harness = createDestroyHarness({ + dockerRunResult: { status: 0, stdout: rows }, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.runOpenshellSpy).not.toHaveBeenCalled(); + expect(harness.events).toEqual([]); + expect(harness.selectGatewaySpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); + expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); + expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); + expect(harness.killStaleProxySpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledOnce(); + }); + + it("refuses identity drift after read-only destroy preflight (#8999)", async () => { + const managed = "aaaa000000000000\topenshell\tdefault\tsb-alpha"; + const foreign = "ffff000000000000\t\tforeign\t"; + const harness = createDestroyHarness({ + dockerRunResults: [ + { status: 0, stdout: managed }, + { status: 0, stdout: [managed, foreign].join("\n") }, + ], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.selectGatewaySpy).toHaveBeenCalledOnce(); + expect(harness.runOpenshellSpy).toHaveBeenCalledWith( + ["sandbox", "list", "-o", "json"], + expect.any(Object), + ); + expect(harness.events).toEqual([]); + expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); + expect(harness.killStaleProxySpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.updateSessionSpy).not.toHaveBeenCalled(); + expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); + expect(harness.stopAllSpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledTimes(2); + }); + + it.each([ + [ + "a replacement identity", + { status: 0, stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta" }, + ], + ["container absence", { status: 0, stdout: "" }], + ["a failed revalidation probe", { status: 1, stderr: "daemon unavailable" }], + ])("refuses %s before sandbox mutation", async (_scenario, changedIdentity) => { + const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; + const harness = createDestroyHarness({ + dockerRunResults: [managed, managed, changedIdentity], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.events).toEqual([]); + expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); + expect(harness.killStaleProxySpy).not.toHaveBeenCalled(); + expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + }); + + it("refuses an absent-to-present replacement before sandbox mutation", async () => { + const absent = { status: 0, stdout: "" }; + const replacement = { + status: 0, + stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta", + }; + const harness = createDestroyHarness({ + sandboxPresent: false, + dockerRunResults: [absent, absent, replacement], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.events).toEqual([]); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + }); + + it("restores MCP preparation when identity changes before wipe", async () => { + const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; + const replacement = { + status: 0, + stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta", + }; + const harness = createDestroyHarness({ + mcpServers: ["github"], + dockerRunResults: [managed, managed, managed, replacement], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.events).toEqual(["mcp-prepare", "mcp-restore"]); + expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + }); + + it("restores MCP preparation when managed inference cleanup fails before wipe", async () => { + const secretMarker = "inference-cleanup-secret"; + const harness = createDestroyHarness({ + mcpServers: ["github"], + stopInferenceError: `OPENAI_API_KEY=${secretMarker}`, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.events).toEqual(["mcp-prepare", "mcp-restore"]); + expect(harness.stopNimByNameSpy).toHaveBeenCalledOnce(); + expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( + ["sandbox", "exec", "alpha"], + expect.anything(), + ); + expect(harness.events).not.toContain("wipe"); + expect(harness.events).not.toContain("detach"); + expect(harness.events).not.toContain("delete"); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain("Could not stop managed inference resources"); + expect(errorOutput).not.toContain(secretMarker); + }); + + it("revalidates immediately before delete and reports partial provider preparation", async () => { + const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; + const replacement = { + status: 0, + stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta", + }; + const harness = createDestroyHarness({ + dockerRunResults: [managed, managed, managed, managed, managed, replacement], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expect(harness.events).toEqual(["wipe", "detach", "mcp-restore"]); + expect( + harness.runOpenshellSpy.mock.calls.some( + ([args]) => Array.isArray(args) && args[0] === "sandbox" && args[1] === "delete", + ), + ).toBe(false); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + }); + + it("keeps the final exact probe immediately adjacent to sandbox delete", async () => { + const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; + const trace: string[] = []; + const harness = createDestroyHarness({ + dockerRunResult: managed, + onDockerRun: (call) => trace.push(`probe:${String(call)}`), + }); + harness.runOpenshellSpy.mockImplementation((args: unknown) => { + const argv = Array.isArray(args) ? args : []; + if (`${String(argv[0])}:${String(argv[1])}` === "sandbox:delete") { + trace.push("delete"); + return { status: 0, stdout: "", stderr: "" }; + } + if (`${String(argv[0])}:${String(argv[1])}` === "sandbox:list") { + return { status: 0, stdout: "[]", stderr: "" }; + } + return { status: 0, stdout: "", stderr: "" }; + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(trace.slice(-2)).toEqual([ + `probe:${String(harness.dockerRunSpy.mock.calls.length)}`, + "delete", + ]); + }); + it("preserves provider and registry ownership when runtime authority is unknown", async () => { const harness = createDestroyHarness({ openshellDriver: "unknown-runtime", diff --git a/src/lib/actions/sandbox/destroy-preflight.ts b/src/lib/actions/sandbox/destroy-preflight.ts index 2b8fb1ce449..dc2b688d726 100644 --- a/src/lib/actions/sandbox/destroy-preflight.ts +++ b/src/lib/actions/sandbox/destroy-preflight.ts @@ -16,7 +16,10 @@ export type SandboxDestroyPreflight = { sandboxConfirmedAbsent: boolean; }; -function stopSandboxInferenceResources(sandboxName: string, sandbox: SandboxEntry | null): void { +export function stopSandboxInferenceResources( + sandboxName: string, + sandbox: SandboxEntry | null, +): void { const nim = require("../../inference/nim") as { stopNimContainer: (name: string, opts?: { silent?: boolean }) => void; stopNimContainerByName: (name: string) => void; @@ -76,6 +79,5 @@ export function prepareSandboxDestroy(sandboxName: string): SandboxDestroyPrefli assertMcpAdapterConfigMutationsAllowed(sandboxName, sandbox, mcpEntriesRequiringConfigMutation); } - stopSandboxInferenceResources(sandboxName, sandbox); return { cleanupGatewayName, runOpenshell, sandbox, sandboxConfirmedAbsent }; } diff --git a/src/lib/actions/sandbox/destroy-presence.ts b/src/lib/actions/sandbox/destroy-presence.ts index a03dad14f70..dfb2c613683 100644 --- a/src/lib/actions/sandbox/destroy-presence.ts +++ b/src/lib/actions/sandbox/destroy-presence.ts @@ -1,6 +1,176 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import { + OPENSHELL_MANAGED_BY_LABEL, + OPENSHELL_MANAGED_BY_VALUE, + OPENSHELL_SANDBOX_ID_LABEL, + OPENSHELL_SANDBOX_NAME_LABEL, +} from "../../onboard/openshell-docker-sandbox-containers"; +import { sanitizeReadinessText } from "../../readiness/sanitize"; +import { + type DockerSandboxIdentityObservation, + inspectDockerSandboxIdentities, +} from "../../adapters/docker/inspect"; + +/** Workspace label OpenShell stamps on every managed sandbox container. */ +export const OPENSHELL_SANDBOX_WORKSPACE_LABEL = "openshell.ai/sandbox-workspace"; + +const IDENTITY_VALUE_MAX_LENGTH = 256; +const IDENTITY_DIAGNOSTIC_MAX_LENGTH = 500; + +/** One container carrying the destroy target's `sandbox-name` label. */ +export type SandboxNameLabeledContainer = { + id: string; + managedBy: string; + workspace: string; + sandboxId: string; +}; + +/** Verdict for whether destroy resolved one complete managed container identity. */ +export type DestroyContainerIdentityVerdict = + | { status: "clear"; identity: SandboxNameLabeledContainer | null } + | { status: "probe-failed"; detail: string } + | { + status: "ambiguous"; + sandboxName: string; + reason: string; + foreign: SandboxNameLabeledContainer[]; + managed: SandboxNameLabeledContainer[]; + }; + +function observeDockerSandboxIdentities(sandboxName: string): DockerSandboxIdentityObservation { + return inspectDockerSandboxIdentities(`${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, { + managedBy: OPENSHELL_MANAGED_BY_LABEL, + workspace: OPENSHELL_SANDBOX_WORKSPACE_LABEL, + sandboxId: OPENSHELL_SANDBOX_ID_LABEL, + }); +} + +/** Read the host observation consumed by the pure identity classifier. */ +export function observeDestroyContainerIdentity( + sandboxName: string, +): DockerSandboxIdentityObservation { + return observeDockerSandboxIdentities(sandboxName); +} + +/** + * Classify every Docker container carrying `openshell.ai/sandbox-name=`. + * The query intentionally does not filter by managed-by so a foreign container + * borrowing the mutable name remains visible and makes destroy fail closed. + */ +export function classifyDestroyContainerIdentity( + sandboxName: string, + observation: DockerSandboxIdentityObservation, +): DestroyContainerIdentityVerdict { + if (observation.status === "probe-failed") { + return { + status: "probe-failed", + detail: + sanitizeReadinessText(observation.detail, IDENTITY_DIAGNOSTIC_MAX_LENGTH) || + "docker ps did not complete successfully", + }; + } + + const { malformedRows, rows } = observation; + const managed = rows.filter((row) => row.managedBy === OPENSHELL_MANAGED_BY_VALUE); + const foreign = rows.filter((row) => row.managedBy !== OPENSHELL_MANAGED_BY_VALUE); + + if (malformedRows > 0) { + return { + status: "ambiguous", + sandboxName, + reason: `Docker returned ${String(malformedRows)} malformed container identity row(s)`, + foreign, + managed, + }; + } + if (rows.length === 0) return { status: "clear", identity: null }; + if (foreign.length > 0) { + return { + status: "ambiguous", + sandboxName, + reason: + `${String(foreign.length)} container(s) carry the '${OPENSHELL_SANDBOX_NAME_LABEL}=` + + `${sandboxName}' label without the '${OPENSHELL_MANAGED_BY_LABEL}=` + + `${OPENSHELL_MANAGED_BY_VALUE}' marker`, + foreign, + managed, + }; + } + if (managed.length !== 1) { + return { + status: "ambiguous", + sandboxName, + reason: + `${String(managed.length)} managed containers carry the ` + + `'${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}' label; expected exactly one`, + foreign, + managed, + }; + } + + const [identity] = managed; + if (!identity.workspace || !identity.sandboxId) { + const missingLabels = [ + identity.workspace ? null : OPENSHELL_SANDBOX_WORKSPACE_LABEL, + identity.sandboxId ? null : OPENSHELL_SANDBOX_ID_LABEL, + ].filter((label): label is string => label !== null); + return { + status: "ambiguous", + sandboxName, + reason: `the managed container is missing ${missingLabels.join(" and ")}`, + foreign, + managed, + }; + } + return { status: "clear", identity }; +} + +/** Require the same immutable container row, including the already-absent state. */ +export function isSameDestroyContainerIdentity( + expected: SandboxNameLabeledContainer | null, + verdict: DestroyContainerIdentityVerdict, +): boolean { + if (verdict.status !== "clear") return false; + if (expected === null || verdict.identity === null) return expected === verdict.identity; + return ( + expected.id === verdict.identity.id && + expected.managedBy === verdict.identity.managedBy && + expected.workspace === verdict.identity.workspace && + expected.sandboxId === verdict.identity.sandboxId + ); +} + +/** Human-readable lines describing an ambiguous-identity refusal. */ +export function formatAmbiguousDestroyIdentity( + verdict: Extract, + cliName: string, +): string[] { + const display = (value: string, fallback = ""): string => + sanitizeReadinessText(value || fallback, IDENTITY_VALUE_MAX_LENGTH); + const describe = (row: SandboxNameLabeledContainer): string => + `${display(row.id).slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${display(row.managedBy)}, ` + + `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${display(row.workspace)}, ` + + `${OPENSHELL_SANDBOX_ID_LABEL}=${display(row.sandboxId)})`; + const sandboxName = display(verdict.sandboxName); + const lines = [ + `Refusing to destroy sandbox '${sandboxName}': ${sanitizeReadinessText(verdict.reason, IDENTITY_DIAGNOSTIC_MAX_LENGTH)}.`, + "NemoClaw could not verify one complete container identity for this sandbox name, so destroy fails closed.", + ]; + for (const row of verdict.foreign) { + lines.push(` Conflicting container: ${describe(row)}`); + } + for (const row of verdict.managed) { + lines.push(` Managed sandbox container: ${describe(row)}`); + } + lines.push( + "Inspect containers with the sandbox-name label. Resolve the conflict through the workflow " + + `that owns the container, then rerun '${display(cliName)} ${sandboxName} destroy'.`, + ); + return lines; +} + export type DestroySandboxPresence = "present" | "absent" | "unknown"; function isStrictSandboxListJsonRow(value: unknown): value is { name: string } { diff --git a/src/lib/actions/sandbox/destroy.test.ts b/src/lib/actions/sandbox/destroy.test.ts index b1e6a16f3f3..698bb3e424c 100644 --- a/src/lib/actions/sandbox/destroy.test.ts +++ b/src/lib/actions/sandbox/destroy.test.ts @@ -75,7 +75,7 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { const dockerSandbox = { openshellDriver: "docker" } as { openshellDriver: string | null }; it("refuses destroy when a foreign container shares the sandbox-name label", () => { - const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const error = vi.fn(); const classify = vi.fn(() => ({ status: "ambiguous" as const, sandboxName: "destroytest", @@ -87,22 +87,28 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, classify: classify as never, + error, }); expect(proceed).toBe(false); expect(classify).toHaveBeenCalledWith("destroytest"); expect(error).toHaveBeenCalled(); - error.mockRestore(); }); it("proceeds for a clear single managed identity", () => { - const classify = vi.fn(() => ({ status: "clear" as const })); + const identity = { + id: "aaaa000000000000", + managedBy: "openshell", + workspace: "default", + sandboxId: "sb", + }; + const classify = vi.fn(() => ({ status: "clear" as const, identity })); expect( assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, classify: classify as never, }), - ).toBe(true); + ).toEqual({ kind: "docker", identity }); }); it("does not probe or block a non-Docker runtime provider", () => { @@ -111,23 +117,26 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { getSandbox: vi.fn(() => ({ openshellDriver: "podman" })) as never, classify: classify as never, }); - expect(proceed).toBe(true); + expect(proceed).toEqual({ kind: "not-docker" }); expect(classify).not.toHaveBeenCalled(); }); - it("proceeds but warns when the Docker probe cannot prove identity", () => { - const warn = vi.fn(); + it("refuses when the Docker probe cannot prove identity", () => { + const error = vi.fn(); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => dockerSandbox) as never, classify: vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })) as never, - warn, + error, }); - expect(proceed).toBe(true); - expect(warn).toHaveBeenCalledWith(expect.stringContaining("daemon down")); + expect(proceed).toBe(false); + expect(error).toHaveBeenCalledWith(expect.stringContaining("daemon down")); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("No sandbox resources were removed"), + ); }); it("treats an unknown/null driver as Docker (the default) and still guards", () => { - const classify = vi.fn(() => ({ status: "clear" as const })); + const classify = vi.fn(() => ({ status: "clear" as const, identity: null })); assertUnambiguousDestroyContainerIdentity("destroytest", { getSandbox: vi.fn(() => ({ openshellDriver: null })) as never, classify: classify as never, diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index fb368df2517..1582b9cb2e4 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -41,11 +41,6 @@ import * as onboardSession from "../../state/onboard-session"; import { resolveNemoclawStateDir } from "../../state/paths"; import * as registry from "../../state/registry"; import { confirmSandboxDestroy } from "./destroy-confirmation"; -import { - classifyDestroyContainerIdentity, - type DestroyContainerIdentityVerdict, - formatAmbiguousDestroyIdentity, -} from "./destroy-container-identity"; import { executeSandboxDestroy, redactDestroyError, @@ -53,10 +48,19 @@ import { } from "./destroy-execution"; import { cleanupGatewayAfterLastSandbox } from "./destroy-gateway"; import { shouldCleanupGatewayAfterConfirmedFinalDestroy } from "./destroy-gateway-cleanup"; -import { prepareSandboxDestroy } from "./destroy-preflight"; +import { + classifyDestroyContainerIdentity, + classifyDestroySandboxPresence, + type DestroyContainerIdentityVerdict, + formatAmbiguousDestroyIdentity, + isSameDestroyContainerIdentity, + observeDestroyContainerIdentity, + type SandboxNameLabeledContainer, +} from "./destroy-presence"; +import { prepareSandboxDestroy, stopSandboxInferenceResources } from "./destroy-preflight"; import { type WipeSandboxStateDeps, wipeSandboxState } from "./wipe-state"; -export { classifyDestroySandboxPresence } from "./destroy-presence"; +export { classifyDestroySandboxPresence }; type RemoveSandboxImageDeps = { getSandbox?: typeof registry.getSandbox; @@ -465,10 +469,14 @@ export async function destroySandbox( export type AssertUnambiguousDestroyIdentityDeps = { getSandbox?: typeof registry.getSandbox; - classify?: typeof classifyDestroyContainerIdentity; - warn?: (message: string) => void; + classify?: (sandboxName: string) => DestroyContainerIdentityVerdict; + error?: (message: string) => void; }; +export type DestroyContainerIdentityProof = + | { kind: "not-docker" } + | { kind: "docker"; identity: SandboxNameLabeledContainer | null }; + /** * Fail closed before any destructive step when the target `sandbox-name` maps * to more than one container identity on the Docker runtime. @@ -478,36 +486,53 @@ export type AssertUnambiguousDestroyIdentityDeps = { * silently remove the real sandbox: the name alone can no longer prove which * container is meant, so the only safe action is to refuse (#8999). The check * is Docker-only — Podman lifecycle enforces exact single-container identity in - * its own resolver — and a probe that cannot reach Docker is non-blocking so a - * normal destroy is never wedged by an unreachable daemon. + * its own resolver. A probe that cannot reach Docker also refuses destruction: + * an inconclusive identity check cannot authorize an irreversible operation. * - * Returns `true` when destroy may proceed and `false` when it was refused. + * Returns a provider-discriminated proof containing the exact accepted Docker + * identity, or `false` when destroy was refused. */ export function assertUnambiguousDestroyContainerIdentity( sandboxName: string, deps: AssertUnambiguousDestroyIdentityDeps = {}, -): boolean { +): DestroyContainerIdentityProof | false { const getSandbox = deps.getSandbox ?? registry.getSandbox; - const classify = deps.classify ?? classifyDestroyContainerIdentity; - const warn = deps.warn ?? defaultDestroyWarn; + const classify = + deps.classify ?? + ((name: string) => + classifyDestroyContainerIdentity(name, observeDestroyContainerIdentity(name))); + const error = deps.error ?? ((message: string) => console.error(` ${message}`)); const providerId = normalizeRuntimeProviderIdentity(getSandbox(sandboxName)?.openshellDriver); - if (providerId !== "docker") return true; + if (providerId !== "docker") return { kind: "not-docker" }; - const verdict: DestroyContainerIdentityVerdict = classify(sandboxName); + const verdict = classify(sandboxName); if (verdict.status === "ambiguous") { for (const line of formatAmbiguousDestroyIdentity(verdict, CLI_NAME)) { - console.error(` ${line}`); + error(line); } return false; } if (verdict.status === "probe-failed") { - // Ambiguity can neither be proven nor ruled out; proceed under the - // destroy's existing lower-layer guards rather than wedge a normal destroy. - warn( - `Could not verify container identity for '${sandboxName}' before destroy: ${verdict.detail}`, + error( + `Refusing to destroy sandbox '${sandboxName}': Docker container identity could not be ` + + `inspected (${redactDestroyError(verdict.detail)}). No sandbox resources were removed. ` + + "Correct the reported Docker error, then rerun the destroy command.", ); + return false; } - return true; + return { kind: "docker", identity: verdict.identity }; +} + +function sameDestroyIdentityProof( + expected: DestroyContainerIdentityProof, + actual: DestroyContainerIdentityProof, +): boolean { + if (expected.kind !== actual.kind) return false; + if (expected.kind === "not-docker" || actual.kind === "not-docker") return true; + return isSameDestroyContainerIdentity(expected.identity, { + status: "clear", + identity: actual.identity, + }); } async function destroySandboxUnlocked( @@ -517,12 +542,29 @@ async function destroySandboxUnlocked( const normalized = normalizeDestroySandboxOptions(options); if (!(await confirmSandboxDestroy(sandboxName, normalized))) return; - if (!assertUnambiguousDestroyContainerIdentity(sandboxName)) { + const initialIdentity = assertUnambiguousDestroyContainerIdentity(sandboxName); + if (initialIdentity === false) { process.exit(1); } const { cleanupGatewayName, runOpenshell, sandbox, sandboxConfirmedAbsent } = prepareSandboxDestroy(sandboxName); + // Docker has no transaction spanning inspection and OpenShell deletion. Recheck + // after the read-only preflight and before stopping local services or entering + // the destructive execution path, so identity drift during preflight fails + // closed. Docker-host administrators remain a trusted local authority. + const preMutationIdentity = assertUnambiguousDestroyContainerIdentity(sandboxName); + if ( + preMutationIdentity === false || + !sameDestroyIdentityProof(initialIdentity, preMutationIdentity) + ) { + if (preMutationIdentity !== false) { + console.error( + ` Refusing to destroy sandbox '${sandboxName}': Docker container identity changed during preflight. No sandbox resources were removed.`, + ); + } + process.exit(1); + } const priorHttpsPinRouteId = parseHttpsPinRouteId(sandbox?.endpointUrl); const destructiveResult = await executeSandboxDestroy({ cleanupShieldsArtifacts: cleanupShieldsDestroyArtifacts, @@ -531,6 +573,9 @@ async function destroySandboxUnlocked( sandbox, sandboxConfirmedAbsent, sandboxName, + expectedContainerIdentity: + initialIdentity.kind === "docker" ? initialIdentity.identity : undefined, + stopInferenceResources: () => stopSandboxInferenceResources(sandboxName, sandbox), }); if (!destructiveResult.ok) { if (destructiveResult.deleteOutput) { diff --git a/src/lib/adapters/docker/inspect-identity.test.ts b/src/lib/adapters/docker/inspect-identity.test.ts new file mode 100644 index 00000000000..dbc8ae5f371 --- /dev/null +++ b/src/lib/adapters/docker/inspect-identity.test.ts @@ -0,0 +1,59 @@ +// 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 { inspectDockerSandboxIdentities } from "./inspect"; + +const LABELS = { + managedBy: "openshell.ai/managed-by", + workspace: "openshell.ai/sandbox-workspace", + sandboxId: "openshell.ai/sandbox-id", +}; + +describe("inspectDockerSandboxIdentities", () => { + it("queries only the sandbox-name label and parses one exact row", () => { + const inspect = vi.fn((_args: readonly string[]) => ({ + status: 0, + stdout: "aaaa000000000000\topenshell\tdefault\tsb-real", + })); + + expect( + inspectDockerSandboxIdentities("openshell.ai/sandbox-name=alpha", LABELS, inspect), + ).toEqual({ + status: "observed", + malformedRows: 0, + rows: [ + { + id: "aaaa000000000000", + managedBy: "openshell", + workspace: "default", + sandboxId: "sb-real", + }, + ], + }); + expect(inspect.mock.calls[0][0]).toContain("label=openshell.ai/sandbox-name=alpha"); + expect(inspect.mock.calls[0][0]).not.toContain("label=openshell.ai/managed-by=openshell"); + }); + + it.each([ + ["trailing vertical tab", "openshell\u000b"], + ["leading whitespace", " openshell"], + ])("rejects %s instead of normalizing a trusted label", (_label, managedBy) => { + expect( + inspectDockerSandboxIdentities("openshell.ai/sandbox-name=alpha", LABELS, () => ({ + status: 0, + stdout: `aaaa000000000000\t${managedBy}\tdefault\tsb-real`, + })), + ).toEqual({ status: "observed", rows: [], malformedRows: 1 }); + }); + + it("preserves probe diagnostics for caller-side sanitization", () => { + expect( + inspectDockerSandboxIdentities("openshell.ai/sandbox-name=alpha", LABELS, () => ({ + status: 1, + stderr: "daemon unavailable", + })), + ).toEqual({ status: "probe-failed", detail: "daemon unavailable" }); + }); +}); diff --git a/src/lib/adapters/docker/inspect.ts b/src/lib/adapters/docker/inspect.ts index b542118be95..437af7ce009 100644 --- a/src/lib/adapters/docker/inspect.ts +++ b/src/lib/adapters/docker/inspect.ts @@ -3,6 +3,86 @@ import { type DockerCaptureOptions, type DockerRunOptions, dockerCapture, dockerRun } from "./run"; +export type DockerSandboxIdentityRow = { + id: string; + managedBy: string; + workspace: string; + sandboxId: string; +}; + +export type DockerSandboxIdentityObservation = + | { status: "observed"; rows: DockerSandboxIdentityRow[]; malformedRows: number } + | { status: "probe-failed"; detail: string }; + +type DockerSandboxIdentityInspect = ( + args: readonly string[], + opts?: DockerRunOptions, +) => Partial, "error" | "status" | "stderr" | "stdout">>; + +const DOCKER_IDENTITY_PROBE_TIMEOUT_MS = 30_000; +const DOCKER_IDENTITY_PROBE_MAX_BUFFER_BYTES = 256 * 1024; +const IDENTITY_VALUE_MAX_LENGTH = 256; +const DOCKER_CONTAINER_ID_PATTERN = /^[0-9a-f]{12,64}$/iu; +const UNSAFE_IDENTITY_TEXT_PATTERN = + /[\u0000-\u001f\u007f-\u009f\u061c\u200e\u200f\u2028-\u202e\u2066-\u2069]/u; + +function isExactBoundedIdentityText(value: string): boolean { + return ( + value === value.trim() && + value.length <= IDENTITY_VALUE_MAX_LENGTH && + !UNSAFE_IDENTITY_TEXT_PATTERN.test(value) + ); +} + +/** Inspect and parse the exact Docker rows used by sandbox destroy. */ +export function inspectDockerSandboxIdentities( + sandboxNameLabel: string, + labelKeys: { managedBy: string; workspace: string; sandboxId: string }, + inspect: DockerSandboxIdentityInspect = dockerRun, +): DockerSandboxIdentityObservation { + const format = [ + "{{.ID}}", + `{{.Label "${labelKeys.managedBy}"}}`, + `{{.Label "${labelKeys.workspace}"}}`, + `{{.Label "${labelKeys.sandboxId}"}}`, + ].join("\t"); + const result = inspect( + ["ps", "-a", "--no-trunc", "--filter", `label=${sandboxNameLabel}`, "--format", format], + { + ignoreError: true, + maxBuffer: DOCKER_IDENTITY_PROBE_MAX_BUFFER_BYTES, + suppressOutput: true, + timeout: DOCKER_IDENTITY_PROBE_TIMEOUT_MS, + }, + ); + if (Number(result.status ?? 1) !== 0) { + const detail = [result.error?.message, result.stderr, result.stdout] + .filter((value) => value !== undefined && value !== null && String(value).length > 0) + .map(String) + .join(" ") + .trim(); + return { status: "probe-failed", detail }; + } + + const rows: DockerSandboxIdentityRow[] = []; + let malformedRows = 0; + for (const rawLine of String(result.stdout ?? "").split(/\r?\n/u)) { + if (rawLine.length === 0) continue; + const fields = rawLine.split("\t"); + if ( + fields.length !== 4 || + !DOCKER_CONTAINER_ID_PATTERN.test(fields[0] ?? "") || + !fields.every(isExactBoundedIdentityText) + ) { + malformedRows += 1; + continue; + } + const [id, managedBy, workspace, sandboxId] = fields as [string, string, string, string]; + rows.push({ id, managedBy, workspace, sandboxId }); + } + return { status: "observed", rows, malformedRows }; +} + export function dockerInspect(args: readonly string[], opts: DockerRunOptions = {}) { return dockerRun(["inspect", ...args], opts); } diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index cd7b37f6aa7..ff791308ad1 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -6,7 +6,7 @@ import { createRequire } from "node:module"; import { expect, type MockInstance, vi } from "vitest"; import type { SandboxWorkloadReceipt } from "../../src/lib/state/registry"; -type DestroySandbox = typeof import("../../src/lib/actions/sandbox/destroy")["destroySandbox"]; +type DestroySandbox = (typeof import("../../src/lib/actions/sandbox/destroy"))["destroySandbox"]; const requireDist = createRequire( new URL("../../src/lib/actions/sandbox/destroy-flow.test.ts", import.meta.url), @@ -51,6 +51,9 @@ type DestroyHarnessOptions = { deleteOutput?: string; deleteStatus?: number; dockerPsOutput?: string; + dockerRunResult?: { status: number | null; stdout?: string; stderr?: string }; + dockerRunResults?: Array<{ status: number | null; stdout?: string; stderr?: string }>; + onDockerRun?: (call: number) => void; endpointUrl?: string; finalizeMcpBridgeError?: string; finalizeMcpError?: string; @@ -66,6 +69,7 @@ type DestroyHarnessOptions = { sandboxPresent?: boolean; shieldsDown?: boolean; shieldsUpError?: Error; + stopInferenceError?: string; workload?: SandboxWorkloadReceipt; }; @@ -240,9 +244,12 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr : names; return matchedNames.length > 0 ? `${matchedNames.join("\n")}\n` : ""; }); - const dockerRunSpy = vi - .spyOn(dockerRun, "dockerRun") - .mockReturnValue({ status: 0 } as ReturnType); + const dockerRunSpy = vi.spyOn(dockerRun, "dockerRun").mockImplementation(() => { + const call = dockerRunSpy.mock.calls.length; + options.onDockerRun?.(call); + const result = options.dockerRunResults?.[call - 1] ?? options.dockerRunResult ?? { status: 0 }; + return result as ReturnType; + }); const selectGatewaySpy = vi .spyOn(destroyGateway, "selectGatewayForSandboxDestroy") .mockImplementation(() => undefined); @@ -251,14 +258,18 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr .mockImplementation(() => undefined); vi.spyOn(sandboxProviderCleanup, "runSandboxProviderPreDeleteCleanup").mockImplementation(() => { events.push("detach"); - return { failures: [] }; + return { detached: [], failures: [] }; }); vi.spyOn(sandboxProviderCleanup, "emitProviderDetachResidualHint").mockImplementation( () => undefined, ); const stopNimByNameSpy = vi .spyOn(nim, "stopNimContainerByName") - .mockImplementation(() => undefined); + .mockImplementation(() => { + if (options.stopInferenceError !== undefined) { + throw new Error(options.stopInferenceError); + } + }); vi.spyOn(nim, "stopNimContainer").mockImplementation(() => undefined); const killStaleProxySpy = vi .spyOn(ollamaProxy, "killStaleProxy") @@ -308,6 +319,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr const prepareMcpBridgesForDestroySpy = vi .spyOn(mcpBridge, "prepareMcpBridgesForDestroy") .mockImplementation(async () => { + events.push("mcp-prepare"); if (options.prepareMcpBridgeError !== undefined) { throw new McpBridgeError(options.prepareMcpBridgeError); } From 76c84a52de33853cd73f94f0723b785b58481201 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 13 Aug 2026 16:26:22 -0700 Subject: [PATCH 06/11] fix(sandbox): close destroy revalidation gaps Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/destroy-execution.ts | 91 ++++++++++++++++---- src/lib/actions/sandbox/destroy-flow.test.ts | 47 +++++++++- 2 files changed, 116 insertions(+), 22 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index d481c06b586..c44ffe3679d 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -214,7 +214,7 @@ async function restoreMcpAfterDeleteAbort( } await restoreMcpBridgesAfterDestroyAbort(sandboxName, preparation); } catch (error) { - recoveryFailure = error instanceof Error ? error.message : String(error); + recoveryFailure = redactDestroyError(error); } finally { if (openedRollbackWindow) { try { @@ -224,7 +224,7 @@ async function restoreMcpAfterDeleteAbort( allowLegacyHermesProtocol: true, }); } catch (error) { - const detail = error instanceof Error ? error.message : String(error); + const detail = redactDestroyError(error); recoveryFailure = recoveryFailure ? `${recoveryFailure}; shields re-lock failed: ${detail}` : `shields re-lock failed: ${detail}`; @@ -266,26 +266,50 @@ export async function executeSandboxDestroy({ deps = {}, }: SandboxDestroyExecutionInput): Promise { return withTimerBoundShieldsMutationLockAsync(sandboxName, "destroy sandbox", async () => { - const identityStillMatches = (): boolean => - expectedContainerIdentity === undefined || - isSameDestroyContainerIdentity( - expectedContainerIdentity, - classifyDestroyContainerIdentity(sandboxName, observeDestroyContainerIdentity(sandboxName)), + type IdentityContinuity = + | { status: "match" } + | { status: "changed" } + | { status: "ambiguous"; detail: string } + | { status: "probe-failed"; detail: string }; + const inspectIdentityContinuity = (): IdentityContinuity => { + if (expectedContainerIdentity === undefined) return { status: "match" }; + const verdict = classifyDestroyContainerIdentity( + sandboxName, + observeDestroyContainerIdentity(sandboxName), ); - const identityDriftResult = ( + if (isSameDestroyContainerIdentity(expectedContainerIdentity, verdict)) { + return { status: "match" }; + } + if (verdict.status === "probe-failed") { + return { status: "probe-failed", detail: redactDestroyError(verdict.detail) }; + } + if (verdict.status === "ambiguous") { + return { status: "ambiguous", detail: redactDestroyError(verdict.reason) }; + } + return { status: "changed" }; + }; + const identityRefusalResult = ( phase: string, + continuity: Exclude, mcpRecoveryFailure?: string, + earlierCleanupDetail = "", ): SandboxDestroyExecutionResult => ({ ok: false, - deleteOutput: `Docker container identity changed ${phase}; no sandbox delete was attempted.`, + deleteOutput: + continuity.status === "probe-failed" + ? `Docker container identity could not be inspected ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` + : continuity.status === "ambiguous" + ? `Docker container identity became ambiguous ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` + : `Docker container identity changed ${phase}; no sandbox delete was attempted.${earlierCleanupDetail}`, exitCode: 1, gatewayUnreachable: false, mcpOwnershipRequiresGateway: false, mcpRecoveryFailure, shieldsRelockRequiresGateway: false, }); - if (!identityStillMatches()) { - return identityDriftResult("before destroy preparation"); + const initialContinuity = inspectIdentityContinuity(); + if (initialContinuity.status !== "match") { + return identityRefusalResult("before destroy preparation", initialContinuity); } let runtimeProvider: RuntimeProviderBundle | null = null; if (sandbox) { @@ -326,14 +350,19 @@ export async function executeSandboxDestroy({ // discarded during preparation. Remaining entries are the durable exact // provider ownership manifest and must survive an unconfirmed delete. const hasMcpOwnership = mcpPreparation.entries.length > 0; - if (!identityStillMatches()) { + const preparedContinuity = inspectIdentityContinuity(); + if (preparedContinuity.status !== "match") { const mcpRecoveryFailure = sandboxConfirmedAbsent ? undefined : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { hardenedForDelete: false, hardeningFailed: false, }); - return identityDriftResult("during destroy preparation", mcpRecoveryFailure); + return identityRefusalResult( + "during destroy preparation", + preparedContinuity, + mcpRecoveryFailure, + ); } try { stopInferenceResources(); @@ -356,14 +385,35 @@ export async function executeSandboxDestroy({ shieldsRelockRequiresGateway: false, }; } + const postInferenceContinuity = inspectIdentityContinuity(); + if (postInferenceContinuity.status !== "match") { + const mcpRecoveryFailure = sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { + hardenedForDelete: false, + hardeningFailed: false, + }); + return identityRefusalResult( + "after managed inference cleanup", + postInferenceContinuity, + mcpRecoveryFailure, + " Managed inference cleanup may already be partial; inspect or restart its resources before retrying.", + ); + } const hardened = wipeAndHardenLiveSandbox(sandboxName, sandboxConfirmedAbsent, deps); const detachProviders = (): DetachSandboxProvidersResult => runSandboxProviderPreDeleteCleanup(sandboxName, { runOpenshell, redact }); - if (!identityStillMatches()) { + const preProviderContinuity = inspectIdentityContinuity(); + if (preProviderContinuity.status !== "match") { const mcpRecoveryFailure = sandboxConfirmedAbsent ? undefined : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); - return identityDriftResult("before provider cleanup", mcpRecoveryFailure); + return identityRefusalResult( + "before provider cleanup", + preProviderContinuity, + mcpRecoveryFailure, + " Managed inference cleanup and workspace wipe or hardening may already have run; inspect those resources before retrying.", + ); } const detachOutcome: DetachSandboxProvidersResult = sandboxConfirmedAbsent ? { detached: [], failures: [] } @@ -373,7 +423,8 @@ export async function executeSandboxDestroy({ // The final exact proof runs immediately before OpenShell delete. A Docker- // socket actor remains a trusted host authority; this closes the actionable // multi-step window without claiming a cross-engine transaction. - if (!identityStillMatches()) { + const deleteBoundaryContinuity = inspectIdentityContinuity(); + if (deleteBoundaryContinuity.status !== "match") { const detachedDetail = detachOutcome.detached.length > 0 ? ` Provider cleanup detached ${detachOutcome.detached.join(", ")}; rerun the owning setup workflow to restore those attachments.` @@ -381,8 +432,12 @@ export async function executeSandboxDestroy({ const mcpRecoveryFailure = sandboxConfirmedAbsent ? undefined : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); - const result = identityDriftResult("at the delete boundary", mcpRecoveryFailure); - return { ...result, deleteOutput: `${result.deleteOutput}${detachedDetail}` }; + return identityRefusalResult( + "at the delete boundary", + deleteBoundaryContinuity, + mcpRecoveryFailure, + ` Managed inference cleanup and workspace wipe or hardening may already have run; inspect those resources before retrying.${detachedDetail}`, + ); } const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { ignoreError: true, diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index cae69a52c07..9a332e4b193 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -255,10 +255,12 @@ describe("destroySandbox flow", () => { }); it("restores MCP preparation when managed inference cleanup fails before wipe", async () => { - const secretMarker = "inference-cleanup-secret"; + const inferenceSecretMarker = "inference-cleanup-secret"; + const recoverySecretMarker = "mcp-recovery-secret"; const harness = createDestroyHarness({ mcpServers: ["github"], - stopInferenceError: `OPENAI_API_KEY=${secretMarker}`, + restoreMcpError: `OPENAI_API_KEY=${recoverySecretMarker}`, + stopInferenceError: `OPENAI_API_KEY=${inferenceSecretMarker}`, }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); @@ -275,9 +277,46 @@ describe("destroySandbox flow", () => { expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); expect(errorOutput).toContain("Could not stop managed inference resources"); - expect(errorOutput).not.toContain(secretMarker); + expect(errorOutput).not.toContain(inferenceSecretMarker); + expect(errorOutput).not.toContain(recoverySecretMarker); }); + it.each([ + [ + "a replacement identity", + { status: 0, stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta" }, + "Docker container identity changed after managed inference cleanup", + ], + [ + "a failed Docker reinspection", + { status: 1, stderr: "daemon unavailable" }, + "Docker container identity could not be inspected after managed inference cleanup: daemon unavailable", + ], + ])( + "restores MCP preparation and refuses workspace wipe after %s", + async (_scenario, changedIdentity, expectedMessage) => { + const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; + const harness = createDestroyHarness({ + mcpServers: ["github"], + dockerRunResults: [managed, managed, managed, managed, changedIdentity], + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow( + "process.exit(1)", + ); + + expect(harness.events).toEqual(["mcp-prepare", "mcp-restore"]); + expect(harness.stopNimByNameSpy).toHaveBeenCalledOnce(); + expect(harness.events).not.toContain("wipe"); + expect(harness.events).not.toContain("detach"); + expect(harness.events).not.toContain("delete"); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain(expectedMessage); + expect(errorOutput).toContain("Managed inference cleanup may already be partial"); + }, + ); + it("revalidates immediately before delete and reports partial provider preparation", async () => { const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; const replacement = { @@ -285,7 +324,7 @@ describe("destroySandbox flow", () => { stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta", }; const harness = createDestroyHarness({ - dockerRunResults: [managed, managed, managed, managed, managed, replacement], + dockerRunResults: [managed, managed, managed, managed, managed, managed, replacement], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); From 001d266fa75c8cab605a0398b2362d60706cef96 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 13 Aug 2026 16:32:29 -0700 Subject: [PATCH 07/11] test(sandbox): retain contributor destroy contracts Signed-off-by: Apurv Kumaria --- docs/reference/commands.mdx | 1 + src/lib/adapters/docker/inspect-identity.test.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 435d18cd603..4ef2531ba6c 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2238,6 +2238,7 @@ For one matching container, the command continues only when all these labels hav - A nonempty `openshell.ai/sandbox-id` If the initial inspection cannot complete, more than one container matches, a matching container has conflicting or incomplete labels, or Docker returns malformed identity data, `destroy` exits before changing sandbox resources. +The identity checks still apply with `--force`, `--yes`, or `NEMOCLAW_NON_INTERACTIVE=1`; those controls authorize confirmation but do not authorize an unproven container identity. NemoClaw rechecks the exact identity after read-only preflight, before provider cleanup, and synchronously at the sandbox-deletion boundary. If a later recheck detects drift or fails, `destroy` refuses sandbox deletion, restores managed MCP preparation when possible, preserves local ownership state, and reports any earlier cleanup already performed. If Docker cannot complete the inspection, correct the reported Docker error before you rerun `destroy`. diff --git a/src/lib/adapters/docker/inspect-identity.test.ts b/src/lib/adapters/docker/inspect-identity.test.ts index dbc8ae5f371..7eff8c7afb8 100644 --- a/src/lib/adapters/docker/inspect-identity.test.ts +++ b/src/lib/adapters/docker/inspect-identity.test.ts @@ -32,6 +32,7 @@ describe("inspectDockerSandboxIdentities", () => { }, ], }); + expect(inspect.mock.calls[0][0]).toContain("-a"); expect(inspect.mock.calls[0][0]).toContain("label=openshell.ai/sandbox-name=alpha"); expect(inspect.mock.calls[0][0]).not.toContain("label=openshell.ai/managed-by=openshell"); }); From 00dbe6990a6504aeb538865393febe2f27a6f1ee Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 13 Aug 2026 16:43:19 -0700 Subject: [PATCH 08/11] test(sandbox): keep destroy flow cases linear Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/destroy-flow.test.ts | 20 ++++++++------------ test/helpers/destroy-flow-test-harness.ts | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 9a332e4b193..3a92dbb6d46 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -23,6 +23,7 @@ import { import { createDestroyHarness, resetDestroyModuleCache, + traceDestroyBoundaryCalls, } from "../../../../test/helpers/destroy-flow-test-harness"; describe("destroySandbox flow", () => { @@ -143,7 +144,7 @@ describe("destroySandbox flow", () => { it("refuses before destructive work when multiple Docker identities share the name", async () => { const rows = [ - "aaaa000000000000\topenshell\tdefault\tsb-alpha", + "aaaa000000000000\topenshell\tdefault\tsb-real", "ffff000000000000\t\tforeign\t", ].join("\n"); const harness = createDestroyHarness({ @@ -152,8 +153,13 @@ describe("destroySandbox flow", () => { await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain("could not verify one complete container identity"); + expect(errorOutput).toContain("sb-real"); expect(harness.runOpenshellSpy).not.toHaveBeenCalled(); expect(harness.events).toEqual([]); + expect(harness.events).not.toContain("wipe"); + expect(harness.events).not.toContain("detach"); expect(harness.selectGatewaySpy).not.toHaveBeenCalled(); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.updateSessionSpy).not.toHaveBeenCalled(); @@ -346,17 +352,7 @@ describe("destroySandbox flow", () => { dockerRunResult: managed, onDockerRun: (call) => trace.push(`probe:${String(call)}`), }); - harness.runOpenshellSpy.mockImplementation((args: unknown) => { - const argv = Array.isArray(args) ? args : []; - if (`${String(argv[0])}:${String(argv[1])}` === "sandbox:delete") { - trace.push("delete"); - return { status: 0, stdout: "", stderr: "" }; - } - if (`${String(argv[0])}:${String(argv[1])}` === "sandbox:list") { - return { status: 0, stdout: "[]", stderr: "" }; - } - return { status: 0, stdout: "", stderr: "" }; - }); + traceDestroyBoundaryCalls(harness, trace); await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index ff791308ad1..43032c2e9ab 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -115,6 +115,24 @@ export function loadDestroySandboxPresenceClassifier(): DestroySandboxPresenceCl return destroyModule.classifyDestroySandboxPresence; } +export function traceDestroyBoundaryCalls( + harness: Pick, + trace: string[], +): void { + harness.runOpenshellSpy.mockImplementation((args: unknown) => { + const argv = Array.isArray(args) ? args : []; + switch (`${String(argv[0])}:${String(argv[1])}`) { + case "sandbox:delete": + trace.push("delete"); + return { status: 0, stdout: "", stderr: "" }; + case "sandbox:list": + return { status: 0, stdout: "[]", stderr: "" }; + default: + return { status: 0, stdout: "", stderr: "" }; + } + }); +} + export function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarness { resetDestroyModuleCache(); const events: string[] = []; From 3bff2fa98d23c1ec12312229160a8014b44b12ea Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 13 Aug 2026 16:59:13 -0700 Subject: [PATCH 09/11] test(sandbox): tighten destroy review contracts Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/destroy-execution.ts | 46 ++++++++----------- src/lib/actions/sandbox/destroy-flow.test.ts | 29 +++++++++--- .../runtime-provider-contract.test.ts | 1 + test/helpers/destroy-flow-test-harness.ts | 32 ++++++++++--- 4 files changed, 70 insertions(+), 38 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index c44ffe3679d..3d179d8a370 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -50,8 +50,11 @@ type SandboxDestroyExecutionInput = { sandbox: SandboxEntry | null; sandboxConfirmedAbsent: boolean; sandboxName: string; + // `undefined` disables Docker identity gating for non-Docker providers. + // `null` records confirmed absence; an object records the one managed + // container observed by the pre-destroy guard. expectedContainerIdentity?: SandboxNameLabeledContainer | null; - stopInferenceResources?: () => void; + stopInferenceResources: () => void; runtimeProviders?: RuntimeProviderBundleRegistry; deps?: { readTimerMarker?: typeof readTimerMarker; @@ -261,7 +264,7 @@ export async function executeSandboxDestroy({ sandboxConfirmedAbsent, sandboxName, expectedContainerIdentity, - stopInferenceResources = () => undefined, + stopInferenceResources, runtimeProviders = CURRENT_RUNTIME_PROVIDER_BUNDLES, deps = {}, }: SandboxDestroyExecutionInput): Promise { @@ -350,14 +353,19 @@ export async function executeSandboxDestroy({ // discarded during preparation. Remaining entries are the durable exact // provider ownership manifest and must survive an unconfirmed delete. const hasMcpOwnership = mcpPreparation.entries.length > 0; + const notHardened: HardenedDeleteState = { + hardenedForDelete: false, + hardeningFailed: false, + }; + const restoreMcpForAbort = async ( + hardenedState: HardenedDeleteState, + ): Promise => + sandboxConfirmedAbsent + ? undefined + : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardenedState); const preparedContinuity = inspectIdentityContinuity(); if (preparedContinuity.status !== "match") { - const mcpRecoveryFailure = sandboxConfirmedAbsent - ? undefined - : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { - hardenedForDelete: false, - hardeningFailed: false, - }); + const mcpRecoveryFailure = await restoreMcpForAbort(notHardened); return identityRefusalResult( "during destroy preparation", preparedContinuity, @@ -367,12 +375,7 @@ export async function executeSandboxDestroy({ try { stopInferenceResources(); } catch (error) { - const mcpRecoveryFailure = sandboxConfirmedAbsent - ? undefined - : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { - hardenedForDelete: false, - hardeningFailed: false, - }); + const mcpRecoveryFailure = await restoreMcpForAbort(notHardened); return { ok: false, deleteOutput: @@ -387,12 +390,7 @@ export async function executeSandboxDestroy({ } const postInferenceContinuity = inspectIdentityContinuity(); if (postInferenceContinuity.status !== "match") { - const mcpRecoveryFailure = sandboxConfirmedAbsent - ? undefined - : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, { - hardenedForDelete: false, - hardeningFailed: false, - }); + const mcpRecoveryFailure = await restoreMcpForAbort(notHardened); return identityRefusalResult( "after managed inference cleanup", postInferenceContinuity, @@ -405,9 +403,7 @@ export async function executeSandboxDestroy({ runSandboxProviderPreDeleteCleanup(sandboxName, { runOpenshell, redact }); const preProviderContinuity = inspectIdentityContinuity(); if (preProviderContinuity.status !== "match") { - const mcpRecoveryFailure = sandboxConfirmedAbsent - ? undefined - : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); + const mcpRecoveryFailure = await restoreMcpForAbort(hardened); return identityRefusalResult( "before provider cleanup", preProviderContinuity, @@ -429,9 +425,7 @@ export async function executeSandboxDestroy({ detachOutcome.detached.length > 0 ? ` Provider cleanup detached ${detachOutcome.detached.join(", ")}; rerun the owning setup workflow to restore those attachments.` : ""; - const mcpRecoveryFailure = sandboxConfirmedAbsent - ? undefined - : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); + const mcpRecoveryFailure = await restoreMcpForAbort(hardened); return identityRefusalResult( "at the delete boundary", deleteBoundaryContinuity, diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 3a92dbb6d46..162895ba6f5 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -155,6 +155,8 @@ describe("destroySandbox flow", () => { const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); expect(errorOutput).toContain("could not verify one complete container identity"); + expect(errorOutput).toContain("Managed sandbox container: aaaa00000000"); + expect(errorOutput).toContain("Conflicting container: ffff00000000"); expect(errorOutput).toContain("sb-real"); expect(harness.runOpenshellSpy).not.toHaveBeenCalled(); expect(harness.events).toEqual([]); @@ -175,7 +177,7 @@ describe("destroySandbox flow", () => { const managed = "aaaa000000000000\topenshell\tdefault\tsb-alpha"; const foreign = "ffff000000000000\t\tforeign\t"; const harness = createDestroyHarness({ - dockerRunResults: [ + dockerRunResultSequence: [ { status: 0, stdout: managed }, { status: 0, stdout: [managed, foreign].join("\n") }, ], @@ -210,7 +212,7 @@ describe("destroySandbox flow", () => { ])("refuses %s before sandbox mutation", async (_scenario, changedIdentity) => { const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; const harness = createDestroyHarness({ - dockerRunResults: [managed, managed, changedIdentity], + dockerRunResultSequence: [managed, managed, changedIdentity], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); @@ -221,6 +223,7 @@ describe("destroySandbox flow", () => { expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledTimes(3); }); it("refuses an absent-to-present replacement before sandbox mutation", async () => { @@ -231,7 +234,7 @@ describe("destroySandbox flow", () => { }; const harness = createDestroyHarness({ sandboxPresent: false, - dockerRunResults: [absent, absent, replacement], + dockerRunResultSequence: [absent, absent, replacement], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); @@ -239,6 +242,7 @@ describe("destroySandbox flow", () => { expect(harness.events).toEqual([]); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledTimes(3); }); it("restores MCP preparation when identity changes before wipe", async () => { @@ -249,7 +253,7 @@ describe("destroySandbox flow", () => { }; const harness = createDestroyHarness({ mcpServers: ["github"], - dockerRunResults: [managed, managed, managed, replacement], + dockerRunResultSequence: [managed, managed, managed, replacement], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); @@ -258,6 +262,7 @@ describe("destroySandbox flow", () => { expect(harness.stopNimByNameSpy).not.toHaveBeenCalled(); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledTimes(4); }); it("restores MCP preparation when managed inference cleanup fails before wipe", async () => { @@ -304,7 +309,7 @@ describe("destroySandbox flow", () => { const managed = { status: 0, stdout: "aaaa000000000000\topenshell\tdefault\tsb-alpha" }; const harness = createDestroyHarness({ mcpServers: ["github"], - dockerRunResults: [managed, managed, managed, managed, changedIdentity], + dockerRunResultSequence: [managed, managed, managed, managed, changedIdentity], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow( @@ -330,7 +335,16 @@ describe("destroySandbox flow", () => { stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta", }; const harness = createDestroyHarness({ - dockerRunResults: [managed, managed, managed, managed, managed, managed, replacement], + detachedProviders: ["provider-a"], + dockerRunResultSequence: [ + managed, // Initial guard before read-only preflight. + managed, // Guard after preflight and before mutation. + managed, // Execution-entry continuity guard. + managed, // Guard after MCP destroy preparation. + managed, // Guard after managed inference cleanup. + managed, // Guard before provider cleanup. + replacement, // Final guard immediately before sandbox deletion. + ], }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); @@ -343,6 +357,9 @@ describe("destroySandbox flow", () => { ).toBe(false); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.retirePortableLifecycleReceiptSpy).not.toHaveBeenCalled(); + expect(harness.dockerRunSpy).toHaveBeenCalledTimes(7); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain("Provider cleanup detached provider-a"); }); it("keeps the final exact probe immediately adjacent to sandbox delete", async () => { diff --git a/src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts b/src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts index 92f92e1ac89..8f05ecf3aff 100644 --- a/src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts +++ b/src/lib/onboard/runtime-provider/runtime-provider-contract.test.ts @@ -993,6 +993,7 @@ describe("socket-free MXC action contract", () => { sandbox: entry, sandboxConfirmedAbsent: false, sandboxName, + stopInferenceResources: vi.fn(), runtimeProviders: providers, deps: { readTimerMarker: () => null, diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 43032c2e9ab..7ca7a023302 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -52,8 +52,13 @@ type DestroyHarnessOptions = { deleteStatus?: number; dockerPsOutput?: string; dockerRunResult?: { status: number | null; stdout?: string; stderr?: string }; - dockerRunResults?: Array<{ status: number | null; stdout?: string; stderr?: string }>; + dockerRunResultSequence?: Array<{ + status: number | null; + stdout?: string; + stderr?: string; + }>; onDockerRun?: (call: number) => void; + detachedProviders?: string[]; endpointUrl?: string; finalizeMcpBridgeError?: string; finalizeMcpError?: string; @@ -262,10 +267,25 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr : names; return matchedNames.length > 0 ? `${matchedNames.join("\n")}\n` : ""; }); - const dockerRunSpy = vi.spyOn(dockerRun, "dockerRun").mockImplementation(() => { - const call = dockerRunSpy.mock.calls.length; - options.onDockerRun?.(call); - const result = options.dockerRunResults?.[call - 1] ?? options.dockerRunResult ?? { status: 0 }; + let identityProbeCall = 0; + const dockerRunSpy = vi.spyOn(dockerRun, "dockerRun").mockImplementation((args: unknown) => { + const argv = Array.isArray(args) ? args.map(String) : []; + const filterIndex = argv.indexOf("--filter"); + const isIdentityProbe = + argv[0] === "ps" && + argv.includes("-a") && + argv.includes("--no-trunc") && + filterIndex >= 0 && + argv[filterIndex + 1]?.startsWith("label=openshell.ai/sandbox-name=") === true; + if (!isIdentityProbe) { + return (options.dockerRunResult ?? { status: 0 }) as ReturnType; + } + identityProbeCall += 1; + options.onDockerRun?.(identityProbeCall); + const result = + options.dockerRunResultSequence?.[identityProbeCall - 1] ?? + options.dockerRunResult ?? + { status: 0 }; return result as ReturnType; }); const selectGatewaySpy = vi @@ -276,7 +296,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr .mockImplementation(() => undefined); vi.spyOn(sandboxProviderCleanup, "runSandboxProviderPreDeleteCleanup").mockImplementation(() => { events.push("detach"); - return { detached: [], failures: [] }; + return { detached: options.detachedProviders ?? [], failures: [] }; }); vi.spyOn(sandboxProviderCleanup, "emitProviderDetachResidualHint").mockImplementation( () => undefined, From 68abe49dc8753b630555e5e6f203b13c166fa0d5 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Thu, 13 Aug 2026 17:09:56 -0700 Subject: [PATCH 10/11] fix(sandbox): quote destroy identity labels Adapt the printable-label diagnostic hardening and regression test from PR #9057. Signed-off-by: Dongni Yang --- .../destroy-container-identity.test.ts | 21 ++++++++++++++++++- src/lib/actions/sandbox/destroy-presence.ts | 7 ++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-container-identity.test.ts b/src/lib/actions/sandbox/destroy-container-identity.test.ts index 1fb4e115df7..a63f1f61fbf 100644 --- a/src/lib/actions/sandbox/destroy-container-identity.test.ts +++ b/src/lib/actions/sandbox/destroy-container-identity.test.ts @@ -171,9 +171,28 @@ describe("formatAmbiguousDestroyIdentity", () => { expect(lines).toContain("Refusing to destroy sandbox 'destroytest'"); expect(lines).toContain("Conflicting container:"); expect(lines).toContain("Managed sandbox container:"); - expect(lines).toContain("openshell.ai/sandbox-id=sb-real"); + expect(lines).toContain('openshell.ai/sandbox-id="sb-real"'); expect(lines).toContain("Resolve the conflict through the workflow that owns the container"); expect(lines).toContain("nemoclaw destroytest destroy"); expect(lines).not.toContain("--yes"); }); + + it("quotes label values so printable delimiters cannot forge adjacent fields", () => { + const foreign = { + ...FOREIGN, + managedBy: 'foreign", openshell.ai/sandbox-workspace="default', + workspace: 'foo, bar"baz', + sandboxId: 'sb-foreign", openshell.ai/managed-by="openshell', + }; + const verdict = expectAmbiguous( + classifyDestroyContainerIdentity("destroytest", observeRows([MANAGED, foreign])), + ); + const lines = formatAmbiguousDestroyIdentity(verdict, "nemoclaw").join("\n"); + expect(lines).toContain(`openshell.ai/managed-by=${JSON.stringify(foreign.managedBy)}`); + expect(lines).toContain(`openshell.ai/sandbox-workspace=${JSON.stringify(foreign.workspace)}`); + expect(lines).toContain(`openshell.ai/sandbox-id=${JSON.stringify(foreign.sandboxId)}`); + expect(lines).not.toContain( + 'openshell.ai/managed-by="foreign", openshell.ai/sandbox-workspace="default"', + ); + }); }); diff --git a/src/lib/actions/sandbox/destroy-presence.ts b/src/lib/actions/sandbox/destroy-presence.ts index dfb2c613683..b3cf519ad7f 100644 --- a/src/lib/actions/sandbox/destroy-presence.ts +++ b/src/lib/actions/sandbox/destroy-presence.ts @@ -149,10 +149,11 @@ export function formatAmbiguousDestroyIdentity( ): string[] { const display = (value: string, fallback = ""): string => sanitizeReadinessText(value || fallback, IDENTITY_VALUE_MAX_LENGTH); + const displayLabel = (value: string): string => JSON.stringify(display(value)); const describe = (row: SandboxNameLabeledContainer): string => - `${display(row.id).slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${display(row.managedBy)}, ` + - `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${display(row.workspace)}, ` + - `${OPENSHELL_SANDBOX_ID_LABEL}=${display(row.sandboxId)})`; + `${display(row.id).slice(0, 12)} (${OPENSHELL_MANAGED_BY_LABEL}=${displayLabel(row.managedBy)}, ` + + `${OPENSHELL_SANDBOX_WORKSPACE_LABEL}=${displayLabel(row.workspace)}, ` + + `${OPENSHELL_SANDBOX_ID_LABEL}=${displayLabel(row.sandboxId)})`; const sandboxName = display(verdict.sandboxName); const lines = [ `Refusing to destroy sandbox '${sandboxName}': ${sanitizeReadinessText(verdict.reason, IDENTITY_DIAGNOSTIC_MAX_LENGTH)}.`, From 8561f2921ce68cdbe0bf117bdf9ef1d639836914 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Thu, 13 Aug 2026 18:56:57 -0700 Subject: [PATCH 11/11] fix(sandbox): preserve provider-neutral destroy checks --- docs/reference/commands.mdx | 4 +- src/lib/actions/sandbox/destroy-execution.ts | 14 +-- src/lib/actions/sandbox/destroy-flow.test.ts | 4 +- src/lib/actions/sandbox/destroy-presence.ts | 59 +++++++++++ src/lib/actions/sandbox/destroy.test.ts | 19 ++-- src/lib/actions/sandbox/destroy.ts | 102 ++++--------------- 6 files changed, 99 insertions(+), 103 deletions(-) diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 74f3feda95c..9dbbb943654 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2260,11 +2260,11 @@ For common recovery steps, refer to [Docker is not running](troubleshooting#dock If `destroy` reports conflicting, incomplete, or malformed identity data, inspect the matching containers: -```bash +~~~bash docker ps -a --no-trunc \ --filter "label=openshell.ai/sandbox-name=my-assistant" \ --format 'table {{.ID}}\t{{.Label "openshell.ai/managed-by"}}\t{{.Label "openshell.ai/sandbox-workspace"}}\t{{.Label "openshell.ai/sandbox-id"}}' -``` +~~~ The labels show what each container claims. They do not prove container ownership. diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 3d179d8a370..3fc1c0b366a 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -50,7 +50,7 @@ type SandboxDestroyExecutionInput = { sandbox: SandboxEntry | null; sandboxConfirmedAbsent: boolean; sandboxName: string; - // `undefined` disables Docker identity gating for non-Docker providers. + // `undefined` delegates identity gating to the runtime provider. // `null` records confirmed absence; an object records the one managed // container observed by the pre-destroy guard. expectedContainerIdentity?: SandboxNameLabeledContainer | null; @@ -300,10 +300,10 @@ export async function executeSandboxDestroy({ ok: false, deleteOutput: continuity.status === "probe-failed" - ? `Docker container identity could not be inspected ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` + ? `Container identity could not be inspected ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` : continuity.status === "ambiguous" - ? `Docker container identity became ambiguous ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` - : `Docker container identity changed ${phase}; no sandbox delete was attempted.${earlierCleanupDetail}`, + ? `Container identity became ambiguous ${phase}: ${continuity.detail}. No sandbox delete was attempted.${earlierCleanupDetail}` + : `Container identity changed ${phase}; no sandbox delete was attempted.${earlierCleanupDetail}`, exitCode: 1, gatewayUnreachable: false, mcpOwnershipRequiresGateway: false, @@ -416,9 +416,9 @@ export async function executeSandboxDestroy({ : runtimeProvider?.cleanup.supported === true && sandbox ? runtimeProvider.cleanup.prepareDestroy({ sandbox, sandboxName }, { detachProviders }) : detachProviders(); - // The final exact proof runs immediately before OpenShell delete. A Docker- - // socket actor remains a trusted host authority; this closes the actionable - // multi-step window without claiming a cross-engine transaction. + // The final identity proof runs immediately before OpenShell delete. A + // runtime administrator remains a trusted host authority; this closes the + // multi-step window without claiming a cross-runtime transaction. const deleteBoundaryContinuity = inspectIdentityContinuity(); if (deleteBoundaryContinuity.status !== "match") { const detachedDetail = diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 162895ba6f5..40eac504650 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -296,12 +296,12 @@ describe("destroySandbox flow", () => { [ "a replacement identity", { status: 0, stdout: "bbbb000000000000\topenshell\tdefault\tsb-beta" }, - "Docker container identity changed after managed inference cleanup", + "Container identity changed after managed inference cleanup", ], [ "a failed Docker reinspection", { status: 1, stderr: "daemon unavailable" }, - "Docker container identity could not be inspected after managed inference cleanup: daemon unavailable", + "Container identity could not be inspected after managed inference cleanup: daemon unavailable", ], ])( "restores MCP preparation and refuses workspace wipe after %s", diff --git a/src/lib/actions/sandbox/destroy-presence.ts b/src/lib/actions/sandbox/destroy-presence.ts index b3cf519ad7f..0760c5a28c3 100644 --- a/src/lib/actions/sandbox/destroy-presence.ts +++ b/src/lib/actions/sandbox/destroy-presence.ts @@ -39,6 +39,18 @@ export type DestroyContainerIdentityVerdict = managed: SandboxNameLabeledContainer[]; }; +export type AssertUnambiguousDestroyIdentityDeps = { + providerId: string; + redact: (detail: string) => string; + cliName?: string; + classify?: (sandboxName: string) => DestroyContainerIdentityVerdict; + error?: (message: string) => void; +}; + +export type DestroyContainerIdentityProof = { + identity: SandboxNameLabeledContainer | null | undefined; +}; + function observeDockerSandboxIdentities(sandboxName: string): DockerSandboxIdentityObservation { return inspectDockerSandboxIdentities(`${OPENSHELL_SANDBOX_NAME_LABEL}=${sandboxName}`, { managedBy: OPENSHELL_MANAGED_BY_LABEL, @@ -172,6 +184,53 @@ export function formatAmbiguousDestroyIdentity( return lines; } +/** + * Fail closed when a Docker sandbox name does not resolve to one complete + * container identity. Other runtime providers own their identity checks. + */ +export function assertUnambiguousDestroyContainerIdentity( + sandboxName: string, + deps: AssertUnambiguousDestroyIdentityDeps, +): DestroyContainerIdentityProof | false { + const classify = + deps.classify ?? + ((name: string) => + classifyDestroyContainerIdentity(name, observeDestroyContainerIdentity(name))); + const error = deps.error ?? ((message: string) => console.error(` ${message}`)); + if (deps.providerId !== "docker") return { identity: undefined }; + + const verdict = classify(sandboxName); + if (verdict.status === "ambiguous") { + for (const line of formatAmbiguousDestroyIdentity(verdict, deps.cliName ?? "nemoclaw")) { + error(line); + } + return false; + } + if (verdict.status === "probe-failed") { + error( + `Refusing to destroy sandbox '${sandboxName}': Docker container identity could not be ` + + `inspected (${deps.redact(verdict.detail)}). No sandbox resources were removed. ` + + "Correct the reported Docker error, then rerun the destroy command.", + ); + return false; + } + return { identity: verdict.identity }; +} + +/** Compare provider-owned identity proofs across two destroy checkpoints. */ +export function isSameDestroyContainerIdentityProof( + expected: DestroyContainerIdentityProof, + actual: DestroyContainerIdentityProof, +): boolean { + if (expected.identity === undefined || actual.identity === undefined) { + return expected.identity === actual.identity; + } + return isSameDestroyContainerIdentity(expected.identity, { + status: "clear", + identity: actual.identity, + }); +} + export type DestroySandboxPresence = "present" | "absent" | "unknown"; function isStrictSandboxListJsonRow(value: unknown): value is { name: string } { diff --git a/src/lib/actions/sandbox/destroy.test.ts b/src/lib/actions/sandbox/destroy.test.ts index 698bb3e424c..f621f220f87 100644 --- a/src/lib/actions/sandbox/destroy.test.ts +++ b/src/lib/actions/sandbox/destroy.test.ts @@ -85,7 +85,8 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { })); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { - getSandbox: vi.fn(() => dockerSandbox) as never, + providerId: dockerSandbox.openshellDriver ?? "docker", + redact: String, classify: classify as never, error, }); @@ -105,26 +106,29 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { const classify = vi.fn(() => ({ status: "clear" as const, identity })); expect( assertUnambiguousDestroyContainerIdentity("destroytest", { - getSandbox: vi.fn(() => dockerSandbox) as never, + providerId: dockerSandbox.openshellDriver ?? "docker", + redact: String, classify: classify as never, }), - ).toEqual({ kind: "docker", identity }); + ).toEqual({ identity }); }); it("does not probe or block a non-Docker runtime provider", () => { const classify = vi.fn(); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { - getSandbox: vi.fn(() => ({ openshellDriver: "podman" })) as never, + providerId: "podman", + redact: String, classify: classify as never, }); - expect(proceed).toEqual({ kind: "not-docker" }); + expect(proceed).toEqual({ identity: undefined }); expect(classify).not.toHaveBeenCalled(); }); it("refuses when the Docker probe cannot prove identity", () => { const error = vi.fn(); const proceed = assertUnambiguousDestroyContainerIdentity("destroytest", { - getSandbox: vi.fn(() => dockerSandbox) as never, + providerId: dockerSandbox.openshellDriver ?? "docker", + redact: String, classify: vi.fn(() => ({ status: "probe-failed" as const, detail: "daemon down" })) as never, error, }); @@ -138,7 +142,8 @@ describe("assertUnambiguousDestroyContainerIdentity (#8999)", () => { it("treats an unknown/null driver as Docker (the default) and still guards", () => { const classify = vi.fn(() => ({ status: "clear" as const, identity: null })); assertUnambiguousDestroyContainerIdentity("destroytest", { - getSandbox: vi.fn(() => ({ openshellDriver: null })) as never, + providerId: "docker", + redact: String, classify: classify as never, }); expect(classify).toHaveBeenCalledWith("destroytest"); diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 1582b9cb2e4..710dc66988b 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -49,18 +49,14 @@ import { import { cleanupGatewayAfterLastSandbox } from "./destroy-gateway"; import { shouldCleanupGatewayAfterConfirmedFinalDestroy } from "./destroy-gateway-cleanup"; import { - classifyDestroyContainerIdentity, + assertUnambiguousDestroyContainerIdentity, classifyDestroySandboxPresence, - type DestroyContainerIdentityVerdict, - formatAmbiguousDestroyIdentity, - isSameDestroyContainerIdentity, - observeDestroyContainerIdentity, - type SandboxNameLabeledContainer, + isSameDestroyContainerIdentityProof, } from "./destroy-presence"; import { prepareSandboxDestroy, stopSandboxInferenceResources } from "./destroy-preflight"; import { type WipeSandboxStateDeps, wipeSandboxState } from "./wipe-state"; -export { classifyDestroySandboxPresence }; +export { assertUnambiguousDestroyContainerIdentity, classifyDestroySandboxPresence }; type RemoveSandboxImageDeps = { getSandbox?: typeof registry.getSandbox; @@ -467,74 +463,6 @@ export async function destroySandbox( return withMcpLifecycleLock(sandboxName, () => destroySandboxUnlocked(sandboxName, options)); } -export type AssertUnambiguousDestroyIdentityDeps = { - getSandbox?: typeof registry.getSandbox; - classify?: (sandboxName: string) => DestroyContainerIdentityVerdict; - error?: (message: string) => void; -}; - -export type DestroyContainerIdentityProof = - | { kind: "not-docker" } - | { kind: "docker"; identity: SandboxNameLabeledContainer | null }; - -/** - * Fail closed before any destructive step when the target `sandbox-name` maps - * to more than one container identity on the Docker runtime. - * - * A foreign container that borrows a real sandbox's `sandbox-name` label (with - * a different workspace / without the managed marker) must never let destroy - * silently remove the real sandbox: the name alone can no longer prove which - * container is meant, so the only safe action is to refuse (#8999). The check - * is Docker-only — Podman lifecycle enforces exact single-container identity in - * its own resolver. A probe that cannot reach Docker also refuses destruction: - * an inconclusive identity check cannot authorize an irreversible operation. - * - * Returns a provider-discriminated proof containing the exact accepted Docker - * identity, or `false` when destroy was refused. - */ -export function assertUnambiguousDestroyContainerIdentity( - sandboxName: string, - deps: AssertUnambiguousDestroyIdentityDeps = {}, -): DestroyContainerIdentityProof | false { - const getSandbox = deps.getSandbox ?? registry.getSandbox; - const classify = - deps.classify ?? - ((name: string) => - classifyDestroyContainerIdentity(name, observeDestroyContainerIdentity(name))); - const error = deps.error ?? ((message: string) => console.error(` ${message}`)); - const providerId = normalizeRuntimeProviderIdentity(getSandbox(sandboxName)?.openshellDriver); - if (providerId !== "docker") return { kind: "not-docker" }; - - const verdict = classify(sandboxName); - if (verdict.status === "ambiguous") { - for (const line of formatAmbiguousDestroyIdentity(verdict, CLI_NAME)) { - error(line); - } - return false; - } - if (verdict.status === "probe-failed") { - error( - `Refusing to destroy sandbox '${sandboxName}': Docker container identity could not be ` + - `inspected (${redactDestroyError(verdict.detail)}). No sandbox resources were removed. ` + - "Correct the reported Docker error, then rerun the destroy command.", - ); - return false; - } - return { kind: "docker", identity: verdict.identity }; -} - -function sameDestroyIdentityProof( - expected: DestroyContainerIdentityProof, - actual: DestroyContainerIdentityProof, -): boolean { - if (expected.kind !== actual.kind) return false; - if (expected.kind === "not-docker" || actual.kind === "not-docker") return true; - return isSameDestroyContainerIdentity(expected.identity, { - status: "clear", - identity: actual.identity, - }); -} - async function destroySandboxUnlocked( sandboxName: string, options: string[] | DestroySandboxOptions = {}, @@ -542,25 +470,30 @@ async function destroySandboxUnlocked( const normalized = normalizeDestroySandboxOptions(options); if (!(await confirmSandboxDestroy(sandboxName, normalized))) return; - const initialIdentity = assertUnambiguousDestroyContainerIdentity(sandboxName); + const inspectContainerIdentity = () => + assertUnambiguousDestroyContainerIdentity(sandboxName, { + cliName: CLI_NAME, + providerId: normalizeRuntimeProviderIdentity( + registry.getSandbox(sandboxName)?.openshellDriver, + ), + redact: redactDestroyError, + }); + const initialIdentity = inspectContainerIdentity(); if (initialIdentity === false) { process.exit(1); } const { cleanupGatewayName, runOpenshell, sandbox, sandboxConfirmedAbsent } = prepareSandboxDestroy(sandboxName); - // Docker has no transaction spanning inspection and OpenShell deletion. Recheck - // after the read-only preflight and before stopping local services or entering - // the destructive execution path, so identity drift during preflight fails - // closed. Docker-host administrators remain a trusted local authority. - const preMutationIdentity = assertUnambiguousDestroyContainerIdentity(sandboxName); + // Recheck identity after read-only preflight and before local mutation. + const preMutationIdentity = inspectContainerIdentity(); if ( preMutationIdentity === false || - !sameDestroyIdentityProof(initialIdentity, preMutationIdentity) + !isSameDestroyContainerIdentityProof(initialIdentity, preMutationIdentity) ) { if (preMutationIdentity !== false) { console.error( - ` Refusing to destroy sandbox '${sandboxName}': Docker container identity changed during preflight. No sandbox resources were removed.`, + ` Refusing to destroy sandbox '${sandboxName}': Container identity changed during preflight. No sandbox resources were removed.`, ); } process.exit(1); @@ -573,8 +506,7 @@ async function destroySandboxUnlocked( sandbox, sandboxConfirmedAbsent, sandboxName, - expectedContainerIdentity: - initialIdentity.kind === "docker" ? initialIdentity.identity : undefined, + expectedContainerIdentity: initialIdentity.identity, stopInferenceResources: () => stopSandboxInferenceResources(sandboxName, sandbox), }); if (!destructiveResult.ok) {