From 07226145f95818b65611b4075f1d65ba7226ab8a Mon Sep 17 00:00:00 2001 From: harjoth Date: Fri, 14 Aug 2026 22:28:14 -0700 Subject: [PATCH] fix(sandbox): report why a sandbox config read failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readSandboxConfig raised a diagnostic carrying the reason OpenShell gave for a failed `sandbox exec -- cat`, but raised it inside a try whose catch discarded every error. The reason never reached the caller, so every failed read reported the generic "Is the sandbox running?" text — wrong whenever the sandbox was running and the exec failed for another reason. Let that diagnostic reach the caller. When OpenShell reports no reason, the stopped-sandbox text stays as the best remaining guess. The reason comes from stderr and the spawn error only. `result.output` is stdout-first, and stdout here is the config the read printed, so using it would put config contents into a CLI error. Refs: #9104 Signed-off-by: harjoth --- src/lib/sandbox/config.ts | 26 ++-- ...box-config-read-failure-diagnostic.test.ts | 121 ++++++++++++++++++ 2 files changed, 139 insertions(+), 8 deletions(-) create mode 100644 test/sandbox-config-read-failure-diagnostic.test.ts diff --git a/src/lib/sandbox/config.ts b/src/lib/sandbox/config.ts index 221df870315..b2bec0175bc 100644 --- a/src/lib/sandbox/config.ts +++ b/src/lib/sandbox/config.ts @@ -462,15 +462,25 @@ function readSandboxConfig(sandboxName: string, target: AgentConfigTarget): Conf }, ); if (result.error || result.signal || result.status !== 0) { - const detail = result.error?.message || result.stderr?.trim() || result.output; - configFail( - ` Cannot read ${target.agentName} config (${target.configPath})${detail ? `: ${detail}` : "."}`, - ); + // Diagnostic channels only. `result.output` is stdout-first, and stdout + // here is the agent config `cat` printed, so echoing it would put config + // contents — credentials included — into a CLI error. + const detail = result.error?.message || result.stderr?.trim(); + // Preserve a failed exec's detail. `configFail` throws, so it must not be + // caught and replaced with the generic stopped-sandbox message below. + if (detail) { + configFail(` Cannot read ${target.agentName} config (${target.configPath}): ${detail}`); + } + raw = ""; + } else { + // `output` is display-normalized with trim(); the transaction digest must + // bind the exact bytes returned by `cat`, including its final newline. + raw = result.stdout ?? result.output ?? ""; } - // `output` is display-normalized with trim(); the transaction digest must - // bind the exact bytes returned by `cat`, including its final newline. - raw = result.stdout ?? result.output ?? ""; - } catch { + } catch (error) { + // Only unexpected capture failures become empty reads. A diagnostic raised + // above already names the reason and must reach the caller (#9104). + if (error instanceof SandboxConfigError) throw error; raw = ""; } diff --git a/test/sandbox-config-read-failure-diagnostic.test.ts b/test/sandbox-config-read-failure-diagnostic.test.ts new file mode 100644 index 00000000000..c3742879613 --- /dev/null +++ b/test/sandbox-config-read-failure-diagnostic.test.ts @@ -0,0 +1,121 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Regression for #9104. + * + * `readSandboxConfig` runs `openshell sandbox exec -- cat ` and, on + * a failed exec, raises a diagnostic carrying the reason OpenShell reported. + * That diagnostic was raised inside a `try` whose `catch` discarded every + * error, so the reason never reached the user: every failed read reported the + * generic "Is the sandbox running?" text instead. A reporter watching a Ready + * sandbox was told it was not running. + * + * These tests drive the real read path — real `spawnSync`, real + * `captureOpenshellCommand` — against a stub OpenShell binary selected through + * `NEMOCLAW_OPENSHELL_BIN`, so they fail if the reason is discarded again. + */ + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { + DEFAULT_AGENT_CONFIG, + readSandboxConfig, + SandboxConfigError, +} from "../src/lib/sandbox/config"; + +const EXEC_FAILURE_REASON = "exec session setup failed: container not ready"; + +let home: string; + +/** + * Install a stub `openshell` whose `sandbox exec` fails, writing `stderr` to + * stderr and `stdout` to stdout — the two channels the diagnostic chooses + * between. + */ +function stubOpenshell(stderr: string, stdout = ""): void { + const binary = path.join(home, "openshell"); + fs.writeFileSync( + binary, + [ + "#!/usr/bin/env bash", + `printf '%s' ${JSON.stringify(stdout)}`, + `printf '%s' ${JSON.stringify(stderr)} >&2`, + "exit 1", + ].join("\n"), + { mode: 0o755 }, + ); + vi.stubEnv("NEMOCLAW_OPENSHELL_BIN", binary); +} + +/** Read the config and return the diagnostic lines the CLI would print. */ +function readAndCaptureLines(): string { + const error = (() => { + try { + readSandboxConfig("alpha", DEFAULT_AGENT_CONFIG); + return null; + } catch (thrown) { + return thrown; + } + })(); + + expect(error).toBeInstanceOf(SandboxConfigError); + return (error as SandboxConfigError).lines.join("\n"); +} + +describe("failed sandbox config reads report OpenShell failures (#9104)", () => { + beforeEach(() => { + home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-9104-")); + }); + + afterEach(() => { + fs.rmSync(home, { recursive: true, force: true }); + }); + + it("reports the reason OpenShell gave for the failed read", () => { + stubOpenshell(EXEC_FAILURE_REASON); + + const lines = readAndCaptureLines(); + + // The operator needs the actual reason to act on: the read failed because + // the exec session could not be set up, not because the sandbox is stopped. + expect(lines).toContain(EXEC_FAILURE_REASON); + expect(lines).toContain("Cannot read openclaw config (/sandbox/.openclaw/openclaw.json)"); + }); + + it("does not blame a stopped sandbox when OpenShell reported another reason", () => { + stubOpenshell(EXEC_FAILURE_REASON); + + const lines = readAndCaptureLines(); + + // #9104: the sandbox was Ready. Claiming otherwise sends the operator to + // the wrong remedy, and `readInSandboxConfigOrFail` appends "Start the + // sandbox and retry." to any message carrying this question. + expect(lines).not.toContain("Is the sandbox running?"); + }); + + it("keeps the stopped-sandbox question when OpenShell reported no reason", () => { + stubOpenshell(""); + + const lines = readAndCaptureLines(); + + // With nothing to report, the stopped sandbox stays the best guess — this + // is the pre-existing text and it must survive the fix above. + expect(lines).toContain("Is the sandbox running?"); + }); + + it("never echoes the partial config a failed read printed", () => { + // A read that fails partway still puts config bytes on stdout. Those bytes + // are the agent config, so the diagnostic must come from stderr alone. + stubOpenshell("", '{"agents":{"apiKey":"sk-secret-9104"}}'); + + const lines = readAndCaptureLines(); + + expect(lines).not.toContain("sk-secret-9104"); + expect(lines).toContain("Is the sandbox running?"); + }); +});