fix(cli): wipe persistent workspace state on sandbox destroy (#5449)

src/lib/actions/sandbox/destroy.ts

@@ -19,12 +19,12 @@ import {
   shouldStopHostServicesAfterDestroy,
 } from "../../domain/sandbox/destroy";
 import {
-  SANDBOX_PROVIDER_SUFFIXES,
   emitProviderDetachResidualHint,
   runSandboxProviderPreDeleteCleanup,
+  SANDBOX_PROVIDER_SUFFIXES,
 } from "../../onboard/sandbox-provider-cleanup";
-import { redact } from "../../security/redact";
 import { parseLiveSandboxNames } from "../../runtime-recovery";
+import { redact } from "../../security/redact";
 import { killTimer as defaultKillShieldsTimer } from "../../shields/timer-control";
 import type { Session } from "../../state/onboard-session";
 import * as onboardSession from "../../state/onboard-session";
@@ -40,6 +40,7 @@ import {
   selectGatewayForSandboxDestroy,
 } from "./destroy-gateway";
 import { getSandboxTargetGatewayName } from "./gateway-target";
+import { wipeSandboxState, type WipeSandboxStateDeps } from "./wipe-state";
 
 type DockerRmi = (tag: string, opts?: { ignoreError?: boolean }) => { status: number | null };
 
@@ -291,6 +292,11 @@ export function cleanupShieldsDestroyArtifacts(
   });
 }
 
+// Re-export so existing callers (tests, downstream code) keep working after
+// the wipe was extracted out of the destroy monolith (#5455 PRA-2).
+export { wipeSandboxState };
+export type { WipeSandboxStateDeps };
+
 export async function destroySandbox(
   sandboxName: string,
   options: string[] | DestroySandboxOptions = {},
@@ -366,6 +372,15 @@ export async function destroySandbox(
   // recorded for this sandbox, not whichever gateway happens to be active.
   const cleanupGatewayName = getSandboxTargetGatewayName(sandboxName);
   selectGatewayForSandboxDestroy(sandboxName, cleanupGatewayName, runOpenshell);
+
+  // Wipe persistent state AFTER the gateway is selected so the exec targets
+  // the sandbox's recorded gateway (#5455 PRA-5), but BEFORE delete because
+  // `sandbox delete` unmounts the PVC and `rm -rf` could no longer reach it.
+  // PRA-2's later ask to defer past delete is physically impossible and
+  // contradicts PRA-5; the wipe-state docstring covers the full source-
+  // boundary justification.
+  wipeSandboxState(sandboxName);
+
   const detachOutcome = runSandboxProviderPreDeleteCleanup(sandboxName, {
     runOpenshell,
     redact,

src/lib/actions/sandbox/wipe-state.ts

@@ -0,0 +1,225 @@
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+import path from "node:path";
+
+import { YW, R } from "../../cli/terminal-style";
+import { shellQuote } from "../../core/shell-quote";
+import * as registry from "../../state/registry";
+
+type RunOpenshellResult = { status: number | null };
+type RunOpenshell = (args: string[], opts?: Record<string, unknown>) => RunOpenshellResult;
+
+type AgentStateInfo = {
+  configPaths: { dir: string };
+  stateDirs: string[];
+  stateFiles: { path: string }[];
+};
+
+export type WipeSandboxStateDeps = {
+  getSandbox?: typeof registry.getSandbox;
+  loadAgent?: (name: string) => AgentStateInfo;
+  runOpenshell?: RunOpenshell;
+  /**
+   * Optional warning sink. Defaults to `console.warn`. Matches the
+   * `removeShieldsState` pattern so tests can capture warnings without
+   * spying on the console global (#5455 Ultra PRA-2).
+   */
+  warn?: (message: string) => void;
+};
+
+/**
+ * Wipe a sandbox's persistent state (the agent-manifest state dirs/files such
+ * as `workspace/USER.md`) while the sandbox is still live, before
+ * `openshell sandbox delete`.
+ *
+ * Source-of-truth review for the PVC wipe workaround (#5449 / #5455 PRA-5):
+ *
+ * - Invalid state: `openshell sandbox delete` tears down the pod but leaves
+ *   the per-sandbox persistent volume (a k3s local-path PVC keyed by sandbox
+ *   name, living inside the shared `openshell-cluster-nemoclaw` Docker
+ *   volume) intact. Re-onboarding with the same name rebinds that PVC and
+ *   resurrects the old workspace files (USER.md, SOUL.md, ...).
+ * - Source boundary: the durable PVC retention is owned upstream by
+ *   OpenShell's `sandbox delete` semantics. This wipe is a host-side
+ *   workaround so destroy is the inverse of `backupSandboxState`: it removes
+ *   exactly the set the snapshot/backup path treats as durable state, plus
+ *   the discovered multi-agent `workspace-*` dirs.
+ * - Source-fix constraint: making `openshell sandbox delete` purge the PVC
+ *   by default is an upstream OpenShell change and would also affect
+ *   non-NemoClaw consumers that rely on PVC retention. NemoClaw needs the
+ *   clean-re-onboard contract today, so the wipe issues `sandbox exec` while
+ *   the sandbox is still live and lets the subsequent `sandbox delete` tear
+ *   the pod down.
+ * - Regression test: test/destroy-wipe-sandbox-state.test.ts covers the
+ *   workspace target, the multi-agent glob, the best-effort warn path, the
+ *   path-escape rejection (state_dirs + state_files), and the contract
+ *   assertion that the script targets workspace/ under the config dir with
+ *   no `..` segments or quoted absolute path arguments.
+ * - Removal condition: drop this wipe when OpenShell's `sandbox delete`
+ *   removes the per-sandbox PVC by default or exposes a documented
+ *   delete-with-pvc flag NemoClaw can pass, and the agent-manifest schema
+ *   exposes a typed state-target API so the path normalization here can
+ *   move into the manifest loader.
+ *
+ * Best-effort: a stopped sandbox (e.g. gateway down) makes the exec fail; we
+ * warn and let destroy proceed rather than block teardown. Mirrors the
+ * `removeShieldsState` pattern.
+ *
+ * Must be called AFTER `selectGatewayForSandboxDestroy()` so the exec runs
+ * against the sandbox's recorded gateway, not whichever gateway happened to
+ * be active when destroy was invoked (#5455 PRA-5).
+ *
+ * See: https://github.com/NVIDIA/NemoClaw/issues/5449
+ */
+export function wipeSandboxState(sandboxName: string, deps: WipeSandboxStateDeps = {}): void {
+  const getSandbox = deps.getSandbox ?? registry.getSandbox;
+  const loadAgentDef =
+    deps.loadAgent ??
+    ((name: string) =>
+      (require("../../agent/defs") as { loadAgent: (n: string) => AgentStateInfo }).loadAgent(
+        name,
+      ));
+  const warn = deps.warn ?? ((message: string) => console.warn(message));
+  const runOpenshell =
+    deps.runOpenshell ??
+    ((args: string[], opts?: Record<string, unknown>) => {
+      const runtime = require("../../adapters/openshell/runtime") as { runOpenshell: RunOpenshell };
+      return runtime.runOpenshell(args, opts);
+    });
+
+  const agentName = getSandbox(sandboxName)?.agent || "openclaw";
+  let agent: AgentStateInfo;
+  try {
+    agent = loadAgentDef(agentName);
+  } catch (error) {
+    const message = error instanceof Error ? error.message : String(error);
+    warn(`  ${YW}⚠${R} Could not resolve agent '${agentName}' to wipe workspace state: ${message}`);
+    return;
+  }
+
+  const dir = agent.configPaths?.dir;
+  if (!dir) return;
+
+  // Reject unsafe agent config roots before constructing the wipe command
+  // (#5455 PRA-2). The script issues `cd ${dir} && rm -rf -- ...` so a
+  // manifest that declared a top-level dir like `/`, `/etc`, even `/sandbox`
+  // (no subdirectory), or anything that uses `..` / `.` / extra slashes to
+  // escape after the prefix (e.g. `/sandbox/../etc`) would let the rm phase
+  // delete outside the intended agent scope.
+  //
+  // Normalize `dir` via path.posix.resolve() first so `..`/`.`/`//` are folded
+  // away, then enforce two invariants on the normalized form:
+  //   1. absolute and under `/sandbox/` with at least one more segment
+  //   2. the original (un-normalized) input must equal the normalized form,
+  //      so a manifest declaring `/sandbox/../etc` is rejected even though
+  //      it resolves to `/etc` which is not under `/sandbox/` anyway -- this
+  //      makes the rejection reason explicit instead of relying on the
+  //      startsWith check.
+  //
+  // Every shipped agent manifest declares `/sandbox/.<agent>` today
+  // (openclaw, hermes, langchain-deepagents-code), so this is a precondition
+  // for the existing fleet, not a behavior change.
+  const SANDBOX_ROOT = "/sandbox/";
+  const normalizedDir = path.posix.resolve(dir);
+  // Distinguish the two failure modes for Ultra PRA-8: the un-normalized form
+  // (contains `..`, `.`, `//`, or is a relative path) vs the resolved form
+  // escaping `/sandbox/`.
+  const notAbsoluteOrNormalized = !path.posix.isAbsolute(dir) || normalizedDir !== dir;
+  const escapesSandboxRoot =
+    !normalizedDir.startsWith(SANDBOX_ROOT) || normalizedDir === SANDBOX_ROOT.replace(/\/$/, "");
+  if (notAbsoluteOrNormalized || escapesSandboxRoot) {
+    const reason = notAbsoluteOrNormalized
+      ? `was not a normalized absolute path (contains '..', '.', '//', or is relative)`
+      : `resolves outside ${SANDBOX_ROOT}<agent-name>`;
+    warn(
+      `  ${YW}⚠${R} Refusing to wipe workspace state for '${sandboxName}': ` +
+        `agent '${agentName}' declared config dir '${dir}' which ${reason}`,
+    );
+    return;
+  }
+
+  // Validate every manifest-derived relative path resolves under `dir`. A
+  // manifest declaring `state_dirs: ["../etc"]` or an absolute path like
+  // `/etc/passwd` would otherwise be shell-quoted and fed straight into
+  // `rm -rf -- ...` inside `cd ${dir}`, where the relative form would
+  // traverse outside the agent config directory. Use POSIX semantics
+  // explicitly so the boundary check matches the Linux sandbox shell that
+  // will execute the script, not the host OS the CLI happens to run on
+  // (#5455 PRA-3).
+  const resolvedDir = path.posix.resolve(dir);
+  const validateManifestPath = (p: string): string | null => {
+    // Reject `..` segments and absolute paths up-front, BEFORE normalization
+    // resolves them away. `path.posix.resolve()` happily folds `../<dir>/foo`
+    // into a path under `dir` if the basenames align, but the raw `..` would
+    // still reach the destructive shell command and the manifest contract
+    // says state targets must be relative names under the agent config dir
+    // (#5455 PRA-1 / CodeRabbit security). Defense-in-depth.
+    if (path.posix.isAbsolute(p) || p.split("/").includes("..")) {
+      warn(
+        `  ${YW}⚠${R} Skipping state path '${p}' from agent '${agentName}' manifest: ` +
+          `must be relative and contain no '..' segments`,
+      );
+      return null;
+    }
+    // Second-line check: even relative paths without `..` must resolve under
+    // the config dir. Catches symbolic edge cases this validator does not
+    // model explicitly.
+    const resolved = path.posix.resolve(resolvedDir, p);
+    if (resolved !== resolvedDir && !resolved.startsWith(`${resolvedDir}/`)) {
+      warn(
+        `  ${YW}⚠${R} Skipping state path '${p}' from agent '${agentName}' manifest: ` +
+          `resolves outside ${dir}`,
+      );
+      return null;
+    }
+    return p;
+  };
+  const validStateDirs = agent.stateDirs
+    .map(validateManifestPath)
+    .filter((p): p is string => p !== null);
+  const validStateFiles = agent.stateFiles
+    .map((file) => validateManifestPath(file.path))
+    .filter((p): p is string => p !== null);
+
+  const targets = [
+    ...validStateDirs.map(shellQuote),
+    ...validStateFiles.map(shellQuote),
+    // Left unquoted so the sandbox shell expands the multi-agent
+    // `workspace-<name>` glob (#1260). A no-match leaves the literal token,
+    // which `rm -rf` silently ignores.
+    "workspace-*",
+  ];
+
+  // cd into the config dir first so relative names and the glob resolve there;
+  // `exit 0` keeps a partially provisioned (dir-absent) sandbox a clean no-op.
+  const script = `cd ${shellQuote(dir)} 2>/dev/null || exit 0; rm -rf -- ${targets.join(" ")}`;
+
+  const result = runOpenshell(
+    ["sandbox", "exec", "--name", sandboxName, "--", "sh", "-c", script],
+    {
+      ignoreError: true,
+      stdio: ["ignore", "ignore", "ignore"],
+    },
+  );
+  if (result.status !== 0) {
+    // #5455 PRA-2 (best-effort failure semantics, justified): destroy must
+    // remove the registry entry and tear down the OpenShell pod even when
+    // the wipe exec returns non-zero. The most common nonzero path is "the
+    // sandbox is no longer live" (gateway down, container already stopped,
+    // openshell connectivity transient): blocking destroy there would leave
+    // the user with an unkillable broken sandbox. The next re-onboard with
+    // the same name is the only path where stale workspace state actually
+    // surfaces, so the contract is: warn loudly here, let destroy proceed,
+    // and the re-onboard banner re-surfaces the warning if the PVC is
+    // detected as non-empty. The behavioral validation for that full
+    // destroy -> re-onboard -> clean-workspace contract (#5455 PRA-1) is an
+    // E2E concern -- the helper-level test below pins the warning and the
+    // CLI-level lifecycle test in test/cli/destroy-gateway-cleanup.test.ts
+    // pins the gateway-select-then-exec-then-delete order.
+    warn(
+      `  ${YW}⚠${R} Could not wipe workspace state for '${sandboxName}' (sandbox not live?); ` +
+        "re-onboarding with the same name may resurface old files.",
+    );
+  }
+}

test/cli/destroy-gateway-cleanup.test.ts

@@ -519,8 +519,94 @@ describe("CLI dispatch", () => {
     expect(selectIndex).toBeGreaterThanOrEqual(0);
     expect(deleteIndex).toBeGreaterThan(selectIndex);
     expect(lines.slice(deleteIndex + 1)).toContain("sandbox list");
+
+    // #5455 PRA-2: the persistent-state wipe (`sandbox exec --name alpha ...`)
+    // MUST come after gateway select and before sandbox delete. Running the
+    // wipe before gateway selection would have it land on whichever gateway
+    // happened to be currently active (`other-gateway` in this fixture), so
+    // a same-named sandbox there could get its workspace wiped while the
+    // intended PVC on `nemoclaw-8081` is left intact. Lock the order in.
+    const wipeIndex = lines.findIndex((line) => line.startsWith("sandbox exec --name alpha"));
+    expect(wipeIndex, "destroy did not issue the persistent-state wipe exec").toBeGreaterThan(
+      selectIndex,
+    );
+    expect(wipeIndex).toBeLessThan(deleteIndex);
   });
 
+  // #5455 Ultra PRA-3: when `--cleanup-gateway` is passed, the gateway-destroy
+  // tears the gateway runtime down after the sandbox is deleted. The wipe
+  // still has to land BEFORE `sandbox delete` (otherwise the PVC is gone),
+  // and the `gateway destroy / gateway remove` has to come AFTER it
+  // (otherwise the gateway the wipe exec targets is gone). Pin the full
+  // gateway-select -> wipe exec -> sandbox delete -> gateway teardown chain.
+  it(
+    "destroys with --cleanup-gateway and runs gateway-select -> wipe -> delete -> gateway-destroy in order",
+    testTimeoutOptions(30_000),
+    () => {
+      const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cli-destroy-cleanup-order-"));
+      const localBin = path.join(home, "bin");
+      const registryDir = path.join(home, ".nemoclaw");
+      const openshellLog = path.join(home, "openshell.log");
+      fs.mkdirSync(localBin, { recursive: true });
+      fs.mkdirSync(registryDir, { recursive: true });
+      fs.writeFileSync(
+        path.join(registryDir, "sandboxes.json"),
+        JSON.stringify({
+          sandboxes: {
+            alpha: {
+              name: "alpha",
+              model: "test-model",
+              provider: "nvidia-prod",
+              gpuEnabled: false,
+              policies: [],
+              gatewayName: "nemoclaw-8081",
+              gatewayPort: 8081,
+            },
+          },
+          defaultSandbox: "alpha",
+        }),
+        { mode: 0o600 },
+      );
+      fs.writeFileSync(
+        path.join(localBin, "openshell"),
+        [
+          "#!/bin/sh",
+          `log_file=${JSON.stringify(openshellLog)}`,
+          'printf \'%s\\n\' "$*" >> "$log_file"',
+          'if [ "$1" = "sandbox" ] && [ "$2" = "list" ]; then',
+          '  printf "NAME STATUS\\n"',
+          "fi",
+          "exit 0",
+        ].join("\n"),
+        { mode: 0o755 },
+      );
+      fs.writeFileSync(path.join(localBin, "docker"), "#!/bin/sh\nexit 0\n", { mode: 0o755 });
+      fs.writeFileSync(path.join(localBin, "pgrep"), "#!/bin/sh\nexit 1\n", { mode: 0o755 });
+      fs.writeFileSync(path.join(localBin, "lsof"), "#!/bin/sh\nexit 1\n", { mode: 0o755 });
+
+      const r = runWithEnv(
+        "alpha destroy -y --cleanup-gateway",
+        { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}` },
+        30_000,
+      );
+
+      expect(r.code, r.out).toBe(0);
+      const lines = fs.readFileSync(openshellLog, "utf8").trim().split("\n");
+      const selectIndex = lines.indexOf("gateway select nemoclaw-8081");
+      const wipeIndex = lines.findIndex((line) => line.startsWith("sandbox exec --name alpha"));
+      const deleteIndex = lines.indexOf("sandbox delete alpha");
+      const gatewayDestroyIndex = lines.findIndex(
+        (line) =>
+          line === "gateway remove nemoclaw-8081" || line === "gateway destroy -g nemoclaw-8081",
+      );
+
+      expect(selectIndex, "gateway select did not run").toBeGreaterThanOrEqual(0);
+      expect(wipeIndex, "wipe exec did not run").toBeGreaterThan(selectIndex);
+      expect(deleteIndex, "sandbox delete did not run").toBeGreaterThan(wipeIndex);
+      expect(gatewayDestroyIndex, "gateway teardown did not run").toBeGreaterThan(deleteIndex);
+    },
+  );
+
   it("fails destroy when openshell sandbox delete returns a real error", () => {
     const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-cli-destroy-failure-"));
     const localBin = path.join(home, "bin");