diff --git a/src/lib/actions/sandbox/gateway-state.ts b/src/lib/actions/sandbox/gateway-state.ts index 71406a9a85e..5c61aaf2f6c 100644 --- a/src/lib/actions/sandbox/gateway-state.ts +++ b/src/lib/actions/sandbox/gateway-state.ts @@ -31,7 +31,7 @@ import { } from "../../adapters/openshell/timeouts"; import * as registry from "../../state/registry"; -type SandboxGatewayState = { +export type SandboxGatewayState = { state: string; output: string; activeGateway?: string | null; diff --git a/src/lib/actions/sandbox/status.ts b/src/lib/actions/sandbox/status.ts index f582a5a0ebf..ef907317d10 100644 --- a/src/lib/actions/sandbox/status.ts +++ b/src/lib/actions/sandbox/status.ts @@ -20,6 +20,7 @@ import { } from "../../adapters/openshell/runtime"; import * as registry from "../../state/registry"; import { resolveOpenshell } from "../../adapters/openshell/resolve"; +import type { SandboxGatewayState } from "./gateway-state"; import { getReconciledSandboxGatewayState, getSandboxGatewayStateForStatus, @@ -57,15 +58,33 @@ export function getSandboxStatusInferenceHealth( // eslint-disable-next-line complexity export async function showSandboxStatus(sandboxName: string): Promise { const sb = registry.getSandbox(sandboxName); - const lookup = await getReconciledSandboxGatewayState(sandboxName, { - getState: getSandboxGatewayStateForStatus, - }); - const liveResult = - lookup.state === "present" - ? await captureOpenshellForStatus(["inference", "get"], { - ignoreError: true, - }) - : null; + // #2666: never let an unexpected throw from the gateway probe (e.g. openshell + // hanging when its container is stopped and the published port is held by a + // foreign listener) suppress the sandbox header. The downstream switch + // handles `gateway_error` by printing an actionable block + exit(1), so a + // synthesized fallback keeps the user-visible contract intact. + let lookup: SandboxGatewayState; + try { + lookup = await getReconciledSandboxGatewayState(sandboxName, { + getState: getSandboxGatewayStateForStatus, + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + lookup = { + state: "gateway_error", + output: ` Could not probe live gateway state: ${message}`, + }; + } + let liveResult: Awaited> | null = null; + if (lookup.state === "present") { + try { + liveResult = await captureOpenshellForStatus(["inference", "get"], { + ignoreError: true, + }); + } catch { + liveResult = null; + } + } const live = liveResult && !isCommandTimeout(liveResult) ? parseGatewayInference(liveResult.output) : null; const currentModel = (live && live.model) || (sb && sb.model) || "unknown"; diff --git a/src/lib/cli/oclif-runner.test.ts b/src/lib/cli/oclif-runner.test.ts index 173187419b7..b21ec7ee790 100644 --- a/src/lib/cli/oclif-runner.test.ts +++ b/src/lib/cli/oclif-runner.test.ts @@ -99,12 +99,57 @@ describe("runRegisteredOclifCommand", () => { expect(exit).toHaveBeenCalledWith(2); }); - it("treats oclif help exits as success", async () => { - runCommandMock.mockRejectedValue({ oclif: { exit: 0 } }); + it("treats oclif graceful ExitError(0) as silent success", async () => { + // Mirrors what `Command.exit(0)` and `--help` actually throw in oclif: an + // ExitError instance whose synthetic `EEXIT: 0` message must NOT leak to + // the user. + class ExitError extends Error { + oclif = { exit: 0 }; + } + runCommandMock.mockRejectedValue(new ExitError("EEXIT: 0")); + const errorLine = vi.fn(); + + await runRegisteredOclifCommand("list", ["--help"], { rootDir: "/repo", error: errorLine }); + + expect(process.exitCode).toBe(0); + expect(errorLine).not.toHaveBeenCalled(); + }); + + it("#2666: surfaces errors that happen to carry oclif.exit === 0 instead of swallowing them", async () => { + // Before #2666 this branch silently set exit 0 and produced no output. + // The bug was an arbitrary error riding the same `oclif.exit === 0` + // channel, e.g. propagated from inside a command's run(). Surface the + // message so the user gets signal. + class WeirdError extends Error { + oclif = { exit: 0 }; + } + runCommandMock.mockRejectedValue(new WeirdError("Could not verify sandbox 'my-assist' against the live OpenShell gateway")); + const errorLine = vi.fn(); + + await runRegisteredOclifCommand("status", ["my-assist"], { rootDir: "/repo", error: errorLine }); + + expect(process.exitCode).toBe(0); + expect(errorLine).toHaveBeenCalledWith( + " Could not verify sandbox 'my-assist' against the live OpenShell gateway", + ); + }); + + it("#2666: falls back to a generic line when the error message is empty", async () => { + // Closes the residual silent path: if a non-ExitError(0) carries an + // empty message (or one that trims to empty), still emit *something* + // so the user is never left looking at exit 0 + blank stdout/stderr. + class BlankError extends Error { + oclif = { exit: 0 }; + } + runCommandMock.mockRejectedValue(new BlankError("")); + const errorLine = vi.fn(); - await runRegisteredOclifCommand("list", ["--help"], { rootDir: "/repo" }); + await runRegisteredOclifCommand("status", ["my-assist"], { rootDir: "/repo", error: errorLine }); expect(process.exitCode).toBe(0); + expect(errorLine).toHaveBeenCalledOnce(); + const [line] = errorLine.mock.calls[0]; + expect(String(line).trim().length).toBeGreaterThan(0); }); it("rethrows non-parse command failures", async () => { diff --git a/src/lib/cli/oclif-runner.ts b/src/lib/cli/oclif-runner.ts index ef15ad1c57b..674a354a428 100644 --- a/src/lib/cli/oclif-runner.ts +++ b/src/lib/cli/oclif-runner.ts @@ -81,6 +81,17 @@ export async function runRegisteredOclifCommand( } catch (error) { const exitCode = getOclifExitCode(error); if (exitCode === 0) { + // #2666: only oclif's own ExitError(0) is an intentional graceful + // exit (e.g. Command.exit(0) — message is the synthetic "EEXIT: 0"). + // Any OTHER error that happens to carry oclif.exit === 0 used to be + // silently swallowed here, producing exit 0 + completely empty + // stdout/stderr. Surface its message — and fall back to a generic + // line if formatOclifError() returns empty so we never reintroduce + // the silent path for an error whose message happens to be blank. + if (!isOclifExitError(error)) { + const message = formatOclifError(error) || "Command exited with no output."; + errorLine(` ${message}`); + } process.exitCode = 0; return; } diff --git a/src/lib/inference/health.test.ts b/src/lib/inference/health.test.ts index fe6504094fb..7627b082a2f 100644 --- a/src/lib/inference/health.test.ts +++ b/src/lib/inference/health.test.ts @@ -242,6 +242,7 @@ describe("inference health", () => { model: "moonshotai/kimi-k2.6", messages: [{ role: "user", content: "Reply with exactly: OK" }], max_tokens: 8, + chat_template_kwargs: { thinking: false }, }); }); diff --git a/src/lib/list-command-deps.ts b/src/lib/list-command-deps.ts index 14882f5a559..a6d50733faf 100644 --- a/src/lib/list-command-deps.ts +++ b/src/lib/list-command-deps.ts @@ -1,15 +1,47 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 - import * as onboardSession from "./state/onboard-session"; -import type { ListSandboxesCommandDeps } from "./inventory-commands"; +import type { ListSandboxesCommandDeps, SandboxEntry } from "./inventory-commands"; import { parseGatewayInference } from "./inference/config"; import { OPENSHELL_PROBE_TIMEOUT_MS } from "./adapters/openshell/timeouts"; import { parseSshProcesses, createSystemDeps } from "./state/sandbox-session"; import { resolveOpenshell } from "./adapters/openshell/resolve"; import { captureOpenshell } from "./adapters/openshell/runtime"; import { recoverRegistryEntries } from "./registry-recovery-action"; +import * as registry from "./state/registry"; + +interface RecoveredRegistry { + sandboxes: SandboxEntry[]; + defaultSandbox?: string | null; + recoveredFromSession?: boolean; + recoveredFromGateway?: number; +} + +interface RegistryFallback { + sandboxes: SandboxEntry[]; + defaultSandbox?: string | null; +} + +/** + * #2666 fallback wrapper: if the primary recovery throws (e.g. openshell + * hangs talking to a foreign port-holder), surface the registry-only + * listing instead of letting the throw propagate and silence output. + * + * Exported for direct unit testing — `buildListCommandDeps()` wires the + * real `recoverRegistryEntries` and `registry.listSandboxes` here. + */ +export async function recoverRegistryEntriesWithFallback( + primary: () => Promise, + fallback: () => RegistryFallback, +): Promise { + try { + return await primary(); + } catch { + const list = fallback(); + return { ...list, recoveredFromSession: false, recoveredFromGateway: 0 }; + } +} export function buildListCommandDeps(): ListSandboxesCommandDeps { const opsBinList = resolveOpenshell(); @@ -30,14 +62,27 @@ export function buildListCommandDeps(): ListSandboxesCommandDeps { }; return { - recoverRegistryEntries: () => recoverRegistryEntries(), - getLiveInference: () => - parseGatewayInference( - captureOpenshell(["inference", "get"], { - ignoreError: true, - timeout: OPENSHELL_PROBE_TIMEOUT_MS, - }).output, + // #2666: never let an unexpected throw from gateway-side recovery (e.g. + // openshell hanging on a foreign port-holder while its container is + // stopped) suppress the registry-only listing. The registry lives on + // disk and is independent of runtime state. + recoverRegistryEntries: () => + recoverRegistryEntriesWithFallback( + () => recoverRegistryEntries(), + () => registry.listSandboxes(), ), + getLiveInference: () => { + try { + return parseGatewayInference( + captureOpenshell(["inference", "get"], { + ignoreError: true, + timeout: OPENSHELL_PROBE_TIMEOUT_MS, + }).output, + ); + } catch { + return null; + } + }, loadLastSession: () => onboardSession.loadSession(), getActiveSessionCount: sessionDeps ? (name) => { diff --git a/test/repro-2666-silent-list-status.test.ts b/test/repro-2666-silent-list-status.test.ts new file mode 100644 index 00000000000..de2e22f68e3 --- /dev/null +++ b/test/repro-2666-silent-list-status.test.ts @@ -0,0 +1,271 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Regression coverage for #2666. + * + * When the openshell sandbox container is stopped AND the host-side + * gateway-published port is held by a foreign listener, the live-gateway + * recovery path inside `nemoclaw list` and the gateway-state probe inside + * `nemoclaw status` can fail unexpectedly. The bug surfaced as + * exit 0 + completely empty stdout/stderr — neither the registered sandbox + * listing nor the sandbox header reached the user. + * + * Two layers of fix: + * 1. Defensive try/catch wraps in status.ts and list-command-deps.ts. + * 2. The actual silent-fail in cli/oclif-runner.ts: errors carrying + * `oclif.exit === 0` were swallowed silently. Now only intentional + * ExitError(0) instances stay silent; anything else surfaces. + */ + +import { spawnSync } from "node:child_process"; +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 { + type ListSandboxesCommandDeps, + getSandboxInventory, + renderSandboxInventoryText, +} from "../dist/lib/inventory-commands.js"; +import { recoverRegistryEntriesWithFallback } from "../dist/lib/list-command-deps.js"; + +const CLI = path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"); + +function buildDepsWithThrowingRecovery(): ListSandboxesCommandDeps { + const registryFallback = { + sandboxes: [ + { + name: "my-assist", + model: "stored-model", + provider: "stored-provider", + gpuEnabled: false, + policies: ["pypi"], + agent: "openclaw", + }, + ], + defaultSandbox: "my-assist", + }; + // Simulates the deps behavior in list-command-deps.ts: the underlying + // recover throws (e.g. openshell hangs/errors talking to the foreign + // port-holder), and the wrapper falls back to the registry shape. + return { + recoverRegistryEntries: async () => { + try { + throw new Error("simulated openshell timeout / hang"); + } catch { + return { ...registryFallback, recoveredFromSession: false, recoveredFromGateway: 0 }; + } + }, + getLiveInference: () => null, + loadLastSession: () => ({ + sandboxName: "my-assist", + steps: { sandbox: { status: "complete" } }, + }), + }; +} + +describe("#2666 — silent empty output regression", () => { + it("nemoclaw list renders the registry-only listing when recovery fails", async () => { + const deps = buildDepsWithThrowingRecovery(); + const inventory = await getSandboxInventory(deps); + const lines: string[] = []; + renderSandboxInventoryText(inventory, (line?: string) => lines.push(String(line ?? ""))); + + const joined = lines.join("\n"); + expect(joined).toContain("my-assist"); + expect(joined).toContain("Sandboxes:"); + expect(lines.length).toBeGreaterThan(0); + }); + + it("getSandboxInventory does not throw when recovery returns the registry-only fallback", async () => { + const deps = buildDepsWithThrowingRecovery(); + const inventory = await getSandboxInventory(deps); + expect(inventory.sandboxes).toHaveLength(1); + expect(inventory.sandboxes[0].name).toBe("my-assist"); + expect(inventory.recovery.recoveredFromGateway).toBe(0); + expect(inventory.recovery.recoveredFromSession).toBe(false); + }); +}); + +describe("#2666 — list-command-deps resilience wrapper", () => { + // Exercises the actual exported `recoverRegistryEntriesWithFallback` from + // src/lib/list-command-deps.ts, not a parallel re-implementation. If the + // production wrapper regresses, these tests fail. + + it("returns the primary result on the happy path", async () => { + const primary = vi.fn(async () => ({ + sandboxes: [{ name: "happy", model: null, provider: null, gpuEnabled: false, policies: [] }], + defaultSandbox: "happy", + recoveredFromSession: true, + recoveredFromGateway: 2, + })); + const fallback = vi.fn(() => ({ sandboxes: [], defaultSandbox: null })); + + const result = await recoverRegistryEntriesWithFallback(primary, fallback); + + expect(primary).toHaveBeenCalledOnce(); + expect(fallback).not.toHaveBeenCalled(); + expect(result.sandboxes).toEqual([ + { name: "happy", model: null, provider: null, gpuEnabled: false, policies: [] }, + ]); + expect(result.recoveredFromGateway).toBe(2); + expect(result.recoveredFromSession).toBe(true); + }); + + it("falls back to the registry-only listing when primary throws", async () => { + const primary = vi.fn(async () => { + throw new Error("simulated openshell hang"); + }); + const fallback = vi.fn(() => ({ + sandboxes: [ + { name: "my-assist", model: "test-model", provider: "test-provider", gpuEnabled: false, policies: [] }, + ], + defaultSandbox: "my-assist", + })); + + const result = await recoverRegistryEntriesWithFallback(primary, fallback); + + expect(primary).toHaveBeenCalledOnce(); + expect(fallback).toHaveBeenCalledOnce(); + expect(result.sandboxes).toHaveLength(1); + expect(result.sandboxes[0].name).toBe("my-assist"); + // Fallback synthesizes recovery flags so downstream rendering treats the + // result as the registry-only state, not a partial recovery from gateway. + expect(result.recoveredFromGateway).toBe(0); + expect(result.recoveredFromSession).toBe(false); + }); +}); + +describe("#2666 — subprocess regression: simulated (container-stopped + foreign-port-holder)", () => { + // End-to-end test that runs the real `nemoclaw` binary against a fake + // `openshell` shell script simulating the bug repro: the openshell sandbox + // container is stopped AND a foreign listener holds port 8080. In that + // state, `openshell sandbox get` returns transport-error output and + // `openshell status` reports a refusing connection on port 8080. + // + // Pre-fix this combination silently produced exit 0 + empty stdout/stderr. + // Post-fix neither command may produce silent empty output: `list` must + // render the registered sandbox from disk, and `status` must produce a + // sandbox header plus an actionable error block. + + let home: string; + let binDir: string; + + beforeEach(() => { + home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-2666-repro-")); + binDir = path.join(home, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + + // Fake openshell that mirrors what users observed in the bug repro: + // - `openshell status` reports gateway nemoclaw with refusing connection + // - `openshell sandbox get ` exits non-zero with a transport error + // - `openshell sandbox list` and `inference get` fail to produce useful output + fs.writeFileSync( + path.join(binDir, "openshell"), + [ + "#!/usr/bin/env bash", + "case \"$*\" in", + " status)", + " cat <<'EOF'", + "Status: Disconnected", + " Gateway: nemoclaw", + " client error (Connect): tcp connect error: Connection refused (os error 61)", + "EOF", + " exit 1", + " ;;", + ' "gateway info -g nemoclaw")', + " echo 'Gateway: nemoclaw'", + " exit 0", + " ;;", + ' "sandbox get my-assist")', + " echo 'transport error: client error (Connect)' >&2", + " exit 1", + " ;;", + ' "sandbox list")', + " echo ''", + " exit 1", + " ;;", + ' "inference get")', + " echo ''", + " exit 1", + " ;;", + " *)", + " exit 0", + " ;;", + "esac", + ].join("\n"), + { mode: 0o755 }, + ); + + const registryDir = path.join(home, ".nemoclaw"); + fs.mkdirSync(registryDir, { recursive: true }); + fs.writeFileSync( + path.join(registryDir, "sandboxes.json"), + JSON.stringify({ + sandboxes: { + "my-assist": { + name: "my-assist", + model: "test-model", + provider: "nvidia-prod", + gpuEnabled: false, + policies: [], + }, + }, + defaultSandbox: "my-assist", + }), + { mode: 0o600 }, + ); + }); + + afterEach(() => { + fs.rmSync(home, { recursive: true, force: true }); + }); + + function runCli(args: string[]): { code: number; stdout: string; stderr: string } { + const result = spawnSync(process.execPath, [CLI, ...args], { + encoding: "utf-8", + timeout: 30_000, + env: { + ...process.env, + HOME: home, + PATH: `${binDir}:${process.env.PATH || ""}`, + NEMOCLAW_HEALTH_POLL_COUNT: "1", + NEMOCLAW_HEALTH_POLL_INTERVAL: "0", + NEMOCLAW_STATUS_PROBE_TIMEOUT_MS: "2000", + NEMOCLAW_TEST_NO_SLEEP: "1", + }, + }); + return { + code: result.status ?? -1, + stdout: result.stdout ?? "", + stderr: result.stderr ?? "", + }; + } + + it("nemoclaw list never produces silent empty output when openshell is broken", () => { + const { code, stdout, stderr } = runCli(["list"]); + const combined = `${stdout}\n${stderr}`; + // The exact failure mode pre-fix was exit 0 + completely empty output. + // The contract here is the negation of that — the user must see + // SOMETHING that includes the sandbox they registered on disk. + expect(combined.trim().length).toBeGreaterThan(0); + expect(combined).toContain("my-assist"); + // `list` succeeds even when the live gateway is unreachable: the + // registry-only listing is the documented fallback behavior (#2666). + expect(code).toBe(0); + }); + + it("nemoclaw status never produces silent empty output when openshell is broken", () => { + const { code, stdout, stderr } = runCli(["my-assist", "status"]); + const combined = `${stdout}\n${stderr}`; + // Must include the sandbox header AND an actionable hint. + expect(combined.trim().length).toBeGreaterThan(0); + expect(combined).toContain("my-assist"); + // `status` must exit non-zero when the live gateway can't be verified + // — that's the contract a watchdog wrapping the command relies on. + expect(code).not.toBe(0); + }); +});