From 75e2454d1dc25e30db29c9a07e13569b74b86aee Mon Sep 17 00:00:00 2001
From: easong-openai <easong@openai.com>
Date: Wed, 16 Apr 2025 12:30:43 -0700
Subject: [PATCH] (feat) gracefully handle invalid commands (#79)

* handle invalid commands
* better test
* format
---
 codex-cli/src/utils/agent/sandbox/raw-exec.ts | 17 +++--
 .../tests/invalid-command-handling.test.ts    | 66 +++++++++++++++++++
 2 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 codex-cli/tests/invalid-command-handling.test.ts

diff --git a/codex-cli/src/utils/agent/sandbox/raw-exec.ts b/codex-cli/src/utils/agent/sandbox/raw-exec.ts
index fb5d9b75..c0dd7c91 100644
--- a/codex-cli/src/utils/agent/sandbox/raw-exec.ts
+++ b/codex-cli/src/utils/agent/sandbox/raw-exec.ts
@@ -122,13 +122,16 @@ export function exec(
       abortSignal.addEventListener("abort", abortHandler, { once: true });
     }
   }
-  if (!child.pid) {
-    return Promise.resolve({
-      stdout: "",
-      stderr: `likely failed because ${prog} could not be found`,
-      exitCode: 1,
-    });
-  }
+  // If spawning the child failed (e.g. the executable could not be found)
+  // `child.pid` will be undefined *and* an `error` event will be emitted on
+  // the ChildProcess instance.  We intentionally do **not** bail out early
+  // here.  Returning prematurely would leave the `error` event without a
+  // listener which – in Node.js – results in an "Unhandled 'error' event"
+  // process‑level exception that crashes the CLI.  Instead we continue with
+  // the normal promise flow below where we are guaranteed to attach both the
+  // `error` and `exit` handlers right away.  Either of those callbacks will
+  // resolve the promise and translate the failure into a regular
+  // ExecResult object so the rest of the agent loop can carry on gracefully.
 
   const stdoutChunks: Array<Buffer> = [];
   const stderrChunks: Array<Buffer> = [];
diff --git a/codex-cli/tests/invalid-command-handling.test.ts b/codex-cli/tests/invalid-command-handling.test.ts
new file mode 100644
index 00000000..6619b4f2
--- /dev/null
+++ b/codex-cli/tests/invalid-command-handling.test.ts
@@ -0,0 +1,66 @@
+import { describe, it, expect, vi } from "vitest";
+
+// ---------------------------------------------------------------------------
+// Low‑level rawExec test ------------------------------------------------------
+// ---------------------------------------------------------------------------
+
+import { exec as rawExec } from "../src/utils/agent/sandbox/raw-exec.js";
+
+describe("rawExec – invalid command handling", () => {
+  it("resolves with non‑zero exit code when executable is missing", async () => {
+    const cmd = ["definitely-not-a-command-1234567890"];
+
+    const result = await rawExec(cmd, {}, []);
+
+    expect(result.exitCode).not.toBe(0);
+    expect(result.stderr.length).toBeGreaterThan(0);
+  });
+});
+
+// ---------------------------------------------------------------------------
+// Higher‑level handleExecCommand test ----------------------------------------
+// ---------------------------------------------------------------------------
+
+// Mock approvals and logging helpers so the test focuses on execution flow.
+vi.mock("@lib/approvals.js", () => {
+  return {
+    __esModule: true,
+    canAutoApprove: () =>
+      ({ type: "auto-approve", runInSandbox: false } as any),
+    isSafeCommand: () => null,
+  };
+});
+
+vi.mock("@lib/format-command.js", () => {
+  return {
+    __esModule: true,
+    formatCommandForDisplay: (cmd: Array<string>) => cmd.join(" "),
+  };
+});
+
+vi.mock("../src/utils/agent/log.js", () => ({
+  __esModule: true,
+  log: () => {},
+  isLoggingEnabled: () => false,
+}));
+
+import { handleExecCommand } from "../src/utils/agent/handle-exec-command.js";
+
+describe("handleExecCommand – invalid executable", () => {
+  it("returns non‑zero exit code for 'git show' as a single argv element", async () => {
+    const execInput = { cmd: ["git show"] } as any;
+    const config = { model: "any", instructions: "" } as any;
+    const policy = { mode: "auto" } as any;
+    const getConfirmation = async () => ({ review: "yes" } as any);
+
+    const { outputText, metadata } = await handleExecCommand(
+      execInput,
+      config,
+      policy,
+      getConfirmation,
+    );
+
+    expect(metadata["exit_code"]).not.toBe(0);
+    expect(String(outputText).length).toBeGreaterThan(0);
+  });
+});