fix(cli): exit non-zero for user-error/startup surfaces riding oclif.exit === 0 (#5974)

src/lib/cli/oclif-runner.test.ts

@@ -3,17 +3,21 @@
 
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 
-const { executeMock, loadMock, runCommandMock } = vi.hoisted(() => ({
-  executeMock: vi.fn(),
+const { flushMock, handleMock, loadMock, runCommandMock, runMock } = vi.hoisted(() => ({
+  flushMock: vi.fn(),
+  handleMock: vi.fn(),
   loadMock: vi.fn(),
   runCommandMock: vi.fn(),
+  runMock: vi.fn(),
 }));
 
 vi.mock("@oclif/core", () => ({
   Config: {
     load: loadMock,
   },
-  execute: executeMock,
+  flush: flushMock,
+  handle: handleMock,
+  run: runMock,
 }));
 
 import { runOclifArgv, runOclifCommandById } from "./oclif-runner";
@@ -46,22 +50,26 @@ describe("runOclifArgv", () => {
   let originalArgv: string[];
 
   beforeEach(() => {
-    executeMock.mockReset();
+    flushMock.mockReset();
+    handleMock.mockReset();
     loadMock.mockReset();
     runCommandMock.mockReset();
+    runMock.mockReset();
     loadMock.mockResolvedValue(makeConfig());
     originalArgv = process.argv;
     process.argv = ["/usr/bin/node", "/repo/bin/nemoclaw.js", "alpha", "status"];
+    process.exitCode = undefined;
   });
 
   afterEach(() => {
     process.argv = originalArgv;
+    process.exitCode = undefined;
   });
 
   it("executes native oclif argv with branded package metadata", async () => {
     const config = makeConfig();
     loadMock.mockResolvedValue(config);
-    executeMock.mockImplementation(async () => {
+    runMock.mockImplementation(async () => {
       expect(process.argv).toEqual([
         "/usr/bin/node",
         "/repo/bin/nemoclaw.js",
@@ -77,21 +85,20 @@ describe("runOclifArgv", () => {
     expect(process.argv).toEqual(["/usr/bin/node", "/repo/bin/nemoclaw.js", "alpha", "status"]);
 
     expect(loadMock).toHaveBeenCalledWith("/repo");
-    expect(executeMock).toHaveBeenCalledWith({
-      args: ["sandbox", "channels", "start", "--help"],
-      loadOptions: {
-        root: "/repo",
-        pjson: config.pjson,
-      },
+    expect(runMock).toHaveBeenCalledWith(["sandbox", "channels", "start", "--help"], {
+      root: "/repo",
+      pjson: config.pjson,
     });
+    expect(flushMock).toHaveBeenCalled();
+    expect(handleMock).not.toHaveBeenCalled();
     expect(config.pjson.oclif.bin).toBe("nemoclaw");
     expect(config.options.pjson.oclif.bin).toBe("nemoclaw");
     expect(config.plugins.get("root")?.pjson.oclif.bin).toBe("nemoclaw");
   });
 
-  it("restores process argv when native oclif execution throws", async () => {
+  it("delegates ordinary native-route failures to oclif's handler and restores argv", async () => {
     const error = new Error("Missing 1 required arg: channel");
-    executeMock.mockImplementation(async () => {
+    runMock.mockImplementation(async () => {
       expect(process.argv).toEqual([
         "/usr/bin/node",
         "/repo/bin/nemoclaw.js",
@@ -103,19 +110,79 @@ describe("runOclifArgv", () => {
       throw error;
     });
 
-    await expect(
-      runOclifArgv(["sandbox", "channels", "add", "alpha"], { rootDir: "/repo" }),
-    ).rejects.toBe(error);
+    await runOclifArgv(["sandbox", "channels", "add", "alpha"], { rootDir: "/repo" });
 
+    // oclif's handle() owns pretty-printing and process exit for ordinary
+    // failures (it never returns control for a real error), so we just forward.
+    expect(handleMock).toHaveBeenCalledWith(error);
     expect(process.argv).toEqual(["/usr/bin/node", "/repo/bin/nemoclaw.js", "alpha", "status"]);
   });
+
+  it("forces a non-zero exit for native-route errors riding oclif.exit === 0 (#5974)", async () => {
+    // oclif's handle() would Exit.exit(0) for this error, silently reporting
+    // success on the native `internal`/`sandbox` routes. The native path must
+    // mirror runOclifCommandById: surface the message and exit non-zero, never
+    // delegating to handle() (which would exit 0).
+    class WeirdError extends Error {
+      oclif = { exit: 0 };
+    }
+    runMock.mockRejectedValue(new WeirdError("sandbox transport closed unexpectedly"));
+    const errorLine = vi.fn();
+
+    await runOclifArgv(["sandbox", "list"], { rootDir: "/repo", error: errorLine });
+
+    expect(process.exitCode).toBe(1);
+    expect(errorLine).toHaveBeenCalledWith("  sandbox transport closed unexpectedly");
+    expect(handleMock).not.toHaveBeenCalled();
+    expect(process.argv).toEqual(["/usr/bin/node", "/repo/bin/nemoclaw.js", "alpha", "status"]);
+  });
+
+  it("falls back to a generic line for blank-message native-route oclif.exit === 0 errors (#5974)", async () => {
+    class BlankError extends Error {
+      oclif = { exit: 0 };
+    }
+    runMock.mockRejectedValue(new BlankError(""));
+    const errorLine = vi.fn();
+
+    await runOclifArgv(["sandbox", "list"], { rootDir: "/repo", error: errorLine });
+
+    expect(process.exitCode).toBe(1);
+    expect(errorLine).toHaveBeenCalledOnce();
+    const [line] = errorLine.mock.calls[0];
+    expect(String(line).trim().length).toBeGreaterThan(0);
+    expect(handleMock).not.toHaveBeenCalled();
+  });
+
+  it("keeps a genuine native-route ExitError(0) as a graceful exit (#5974)", async () => {
+    // Command.exit(0) / --help on the native route must stay silent and
+    // delegate to oclif's handler, which performs the graceful exit 0.
+    // This mocks handleOclif to assert delegation; the runtime counterpart
+    // (real `nemoclaw sandbox --help` → exit 0 through the actual binary) is
+    // locked by test/exit-code-user-error-surfaces.test.ts
+    // ("a native-route --help stays a clean exit 0").
+    class ExitError extends Error {
+      oclif = { exit: 0 };
+    }
+    const exitError = new ExitError("EEXIT: 0");
+    runMock.mockRejectedValue(exitError);
+    const errorLine = vi.fn();
+
+    await runOclifArgv(["sandbox", "list"], { rootDir: "/repo", error: errorLine });
+
+    expect(errorLine).not.toHaveBeenCalled();
+    // The runner must NOT force a failure code here — handle() owns the
+    // graceful exit 0 for a genuine ExitError(0).
+    expect(process.exitCode).toBeUndefined();
+    expect(handleMock).toHaveBeenCalledWith(exitError);
+  });
 });
 
 describe("runOclifCommandById", () => {
   let originalArgv: string[];
 
   beforeEach(() => {
-    executeMock.mockReset();
+    flushMock.mockReset();
+    handleMock.mockReset();
     runCommandMock.mockReset();
     loadMock.mockReset();
     loadMock.mockResolvedValue(makeConfig());
@@ -195,11 +262,12 @@ describe("runOclifCommandById", () => {
     expect(errorLine).not.toHaveBeenCalled();
   });
 
-  it("surfaces errors that happen to carry oclif.exit === 0 instead of swallowing them (#2666)", 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.
+  it("surfaces AND fails on errors that merely carry oclif.exit === 0 (#2666, #5974)", async () => {
+    // #2666 stopped this branch silently swallowing an arbitrary error that
+    // rode the same `oclif.exit === 0` channel (e.g. propagated from inside a
+    // command's run()) — but it still reported success. #5974: such an error
+    // is a genuine failure, so surface the message AND exit non-zero so `$?`
+    // stays scriptable. Only a real oclif ExitError(0) stays exit 0.
     class WeirdError extends Error {
       oclif = { exit: 0 };
     }
@@ -210,16 +278,17 @@ describe("runOclifCommandById", () => {
 
     await runOclifCommandById("status", ["my-assist"], { rootDir: "/repo", error: errorLine });
 
-    expect(process.exitCode).toBe(0);
+    expect(process.exitCode).toBe(1);
     expect(errorLine).toHaveBeenCalledWith(
       "  Could not verify sandbox 'my-assist' against the live OpenShell gateway",
     );
   });
 
-  it("falls back to a generic line when the error message is empty (#2666)", async () => {
+  it("falls back to a generic line and still fails when the message is empty (#2666, #5974)", 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.
+    // empty message (or one that trims to empty), still emit *something* and
+    // exit non-zero so the user is never left looking at exit 0 + blank
+    // stdout/stderr.
     class BlankError extends Error {
       oclif = { exit: 0 };
     }
@@ -228,7 +297,7 @@ describe("runOclifCommandById", () => {
 
     await runOclifCommandById("status", ["my-assist"], { rootDir: "/repo", error: errorLine });
 
-    expect(process.exitCode).toBe(0);
+    expect(process.exitCode).toBe(1);
     expect(errorLine).toHaveBeenCalledOnce();
     const [line] = errorLine.mock.calls[0];
     expect(String(line).trim().length).toBeGreaterThan(0);

src/lib/cli/oclif-runner.ts

@@ -1,7 +1,12 @@
 // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-import { Config as OclifConfig, execute as executeOclif } from "@oclif/core";
+import {
+  flush as flushOclif,
+  handle as handleOclif,
+  Config as OclifConfig,
+  run as runOclif,
+} from "@oclif/core";
 
 import { CLI_NAME } from "./branding";
 
@@ -95,18 +100,23 @@ export async function runOclifCommandById(
   } 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}`);
+      // Only oclif's own ExitError(0) is an intentional graceful exit (e.g.
+      // Command.exit(0) / --help — its message is the synthetic "EEXIT: 0",
+      // which must stay silent). Keep that path at exit 0.
+      if (isOclifExitError(error)) {
+        process.exitCode = 0;
+        return;
       }
-      process.exitCode = 0;
+      // #5974: any OTHER error that merely happens to carry oclif.exit === 0
+      // is a genuine failure that bubbled out of a command's run(). #2666
+      // stopped it being silently swallowed (exit 0 + empty output); here we
+      // also refuse to report success for it — surface its message AND exit
+      // non-zero so `$?` stays scriptable. Fall back to a generic line if
+      // formatOclifError() returns empty so a blank message never reintroduces
+      // the silent path.
+      const message = formatOclifError(error) || "Command exited with no output.";
+      errorLine(`  ${message}`);
+      process.exitCode = 1;
       return;
     }
 
@@ -133,18 +143,48 @@ export async function runOclifCommandById(
 export async function runOclifArgv(args: string[], opts: OclifCommandRunOptions): Promise<void> {
   const config = await OclifConfig.load(opts.rootDir);
   applyBrandedBin(config);
+  const errorLine = opts.error ?? console.error;
   const originalArgv = process.argv;
   // oclif's parse-error help renderer consults process.argv, not just the
-  // explicit execute({ args }) value, so keep both views on the native route.
+  // explicit run() args, so keep both views on the native route.
   process.argv = [originalArgv[0] ?? process.execPath, originalArgv[1] ?? CLI_NAME, ...args];
   try {
-    await executeOclif({
-      args,
-      loadOptions: {
-        root: opts.rootDir,
-        pjson: config.pjson,
-      },
-    });
+    // Mirror @oclif/core's execute() (run → flush → handle) by hand so the
+    // native argv path keeps oclif's command lookup, parsing, help rendering,
+    // and pretty-printed errors while letting us intercept one case below.
+    await runOclif(args, { root: opts.rootDir, pjson: config.pjson });
+    await flushOclif();
+  } catch (error) {
+    await flushOclif();
+    // #5974: same hardening as runOclifCommandById. oclif's own handle() would
+    // run Exit.exit(err.oclif?.exit ?? 1) here, so a non-ExitError that merely
+    // carries oclif.exit === 0 (propagated out of a command's run()) would
+    // silently exit 0 — reporting success for a real failure on the native
+    // `internal`/`sandbox` routes. Surface the message and force a non-zero
+    // exit instead; only a genuine ExitError(0) stays a graceful exit.
+    //
+    // Mechanism asymmetry (why process.exitCode here, exit()/throw in
+    // runOclifCommandById): this native path mirrors oclif's execute() (run →
+    // flush → handle), so for the intercepted case we set process.exitCode and
+    // return rather than delegating to handleOclif() (the non-intercepted
+    // branch below). handleOclif() IS oclif's handle(), which would re-run
+    // Exit.exit(0) for this error and undo the fix, so process.exitCode +
+    // return is the only way to force a non-zero code without re-entering
+    // handle(). runOclifCommandById does not route through handle() at all — it
+    // maps errors to codes by hand (its injected exit() for parse/ExitError, a
+    // re-throw otherwise) — but applies the identical oclif.exit === 0 guard.
+    // Removal condition: drop this guard once @oclif/core's handle() no longer
+    // exits 0 for a non-ExitError that carries oclif.exit === 0.
+    const exitCode = getOclifExitCode(error);
+    if (exitCode === 0 && !isOclifExitError(error)) {
+      const message = formatOclifError(error) || "Command exited with no output.";
+      errorLine(`  ${message}`);
+      process.exitCode = 1;
+      return;
+    }
+    // Everything else (parse errors, ExitError, ordinary failures) keeps
+    // oclif's standard handling: pretty-print, optional help, and process exit.
+    await handleOclif(error as Parameters<typeof handleOclif>[0]);
   } finally {
     process.argv = originalArgv;
   }

src/lib/onboard/machine/handlers/provider-inference.test.ts

@@ -657,6 +657,36 @@ describe("handleProviderInferenceState", () => {
     expect(calls.reconcileRouter).toHaveBeenCalledOnce();
   });
 
+  // #5974 instance 5: the Model Router Python preflight (`prepareModelRouterVenv`)
+  // throws a plain Error (e.g. "above supported ceiling", with no `oclif.exit`)
+  // out of `reconcileModelRouter`. The routed branch must catch that throw and
+  // exit non-zero via `exitProcess(1)` so onboard reports the failure to `$?`,
+  // rather than the throw being swallowed or riding the oclif runner. The error
+  // reasons themselves are locked by `model-router-python.test.ts`.
+  it("exits non-zero when model router reconciliation throws (#5974)", async () => {
+    const session = createSession({ provider: "nvidia-router", model: "router/model" });
+    session.steps.provider_selection.status = "complete";
+    const { deps, calls } = createDeps({
+      isInferenceRouteReady: vi.fn(() => true),
+      reconcileModelRouter: vi.fn(async () => {
+        throw new Error("version 3.14.0 above supported ceiling 3.14.0 (exclusive)");
+      }),
+    });
+
+    await expect(
+      handleProviderInferenceState({
+        ...baseOptions(deps, session),
+        resume: true,
+        sandboxName: "router-sandbox",
+      }),
+    ).rejects.toThrow("exit 1");
+
+    expect(calls.exit).toHaveBeenCalledWith(1);
+    expect(calls.error).toHaveBeenCalledWith(
+      expect.stringContaining("Failed to reconcile model router"),
+    );
+  });
+
   // Regression: #4564. On resume the routed provider was only reconciled, never
   // re-upserted, so a stale localhost base URL recorded by an earlier run could
   // survive in the gateway and break inference.local from the sandbox.