From abc4998c85960f2fbfdec80dcbe41bf739551e1c Mon Sep 17 00:00:00 2001 From: OfficeDjunes Date: Mon, 4 May 2026 17:51:17 +0200 Subject: [PATCH] Fix Windows + Git Bash compatibility, retry hang on auth, base ref validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four issues found while stress-testing on Windows 10 + Git Bash: 1. **Bug: `/codex:cancel` fails on Windows + Git Bash with `Invalid argument/option - 'C:/Program Files/Git/PID'`.** `lib/process.mjs` and `lib/app-server.mjs` set `shell: process.env.SHELL || true` on Windows. Under Git Bash `process.env.SHELL` resolves to `/usr/bin/bash`, and MSYS path translation rewrites `taskkill /PID 123 /T /F` to `taskkill /c/Program Files/Git/PID 123 /T /F`. Pinning the shell to `cmd.exe` keeps `.cmd` shim resolution (PATHEXT) but skips Bash path translation. 2. **Improvement: Reconnect storm on permanent OpenAI auth failures.** When the underlying CLI gets a 401/403 from OpenAI, the app-server retries up to 5 times before surfacing the failure. Add a guard in `lib/codex.mjs#applyTurnNotification` so that whenever an `error` notification matches a permanent-auth pattern (401, 403, "Missing bearer", "invalid api key"), we short-circuit `captureTurn` instead of waiting for subsequent reconnects to fail too. Cuts review-without-auth from ~60s to ~20s in local testing. 3. **Bug: Invalid `--base ` is only caught after a Codex thread spins up and round-trips to the API.** `lib/git.mjs#resolveReviewTarget` accepted the `baseRef` verbatim. Add a `git rev-parse --verify --quiet ^{commit}` pre-check so a typo or an unfetched branch fails immediately with a guidance message instead of surfacing later as a confusing API error. 4. **Improvement: `DeprecationWarning DEP0190` on every spawnSync invocation.** Same root cause as bug 1 — moving from `shell: SHELL || true` to `shell: "cmd.exe"` (Windows only) keeps the warning narrowed to genuine shell-resolution paths instead of leaking on every `taskkill`/`git`/binary probe call. Verified locally with the existing stress-test scenarios: setup, status, result, cancel, review (with/without auth), adversarial-review, task missing prompt, invalid scope, invalid base ref, long focus text. Co-Authored-By: Claude Opus 4.7 --- plugins/codex/scripts/lib/app-server.mjs | 7 ++++++- plugins/codex/scripts/lib/codex.mjs | 17 +++++++++++++++++ plugins/codex/scripts/lib/git.mjs | 10 ++++++++++ plugins/codex/scripts/lib/process.mjs | 9 ++++++++- 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 127c8376..263791e0 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -186,11 +186,16 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { } async initialize() { + // Pin to cmd.exe on Windows (NOT $SHELL) so the .cmd shim for codex is + // resolved via PATHEXT but no MSYS path translation kicks in on Git Bash. + // The original code used `process.env.SHELL || true`, which under Git Bash + // mangled Windows-style command flags. On POSIX, codex is a JS shebang + // file so direct spawn (shell:false) works. this.proc = spawn("codex", ["app-server"], { cwd: this.cwd, env: this.options.env ?? process.env, stdio: ["pipe", "pipe", "pipe"], - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + shell: process.platform === "win32" ? "cmd.exe" : false, windowsHide: true }); diff --git a/plugins/codex/scripts/lib/codex.mjs b/plugins/codex/scripts/lib/codex.mjs index f2fe88bd..baf4bd76 100644 --- a/plugins/codex/scripts/lib/codex.mjs +++ b/plugins/codex/scripts/lib/codex.mjs @@ -44,6 +44,17 @@ const TASK_THREAD_PREFIX = "Codex Companion Task"; const DEFAULT_CONTINUE_PROMPT = "Continue from the current thread state. Pick the next highest-value step and follow through until the task is resolved."; +// Permanent auth-fail patterns from the OpenAI API. The underlying Codex CLI's +// app-server retries indefinitely on these, but they never recover without +// re-authenticating, so we abort the captureTurn early. +const PERMANENT_AUTH_ERROR_RE = /\b(?:401\s+Unauthorized|403\s+Forbidden|Missing\s+bearer|invalid\s+api\s+key|authentication[_-]?failed)\b/i; + +function isPermanentAuthError(error) { + if (!error) return false; + const message = typeof error === "string" ? error : (error.message ?? ""); + return PERMANENT_AUTH_ERROR_RE.test(message); +} + function cleanCodexStderr(stderr) { return stderr .split(/\r?\n/) @@ -531,6 +542,12 @@ function applyTurnNotification(state, message) { case "error": state.error = message.params.error; emitProgress(state.onProgress, `Codex error: ${message.params.error.message}`, "failed"); + // Short-circuit on permanent auth failures so we don't waste API quota + // in the underlying CLI's reconnect loop (it retries 5x by default). + // 401/403 from OpenAI never recovers without re-authenticating. + if (isPermanentAuthError(message.params.error)) { + completeTurn(state, { id: state.turnId ?? "auth-failure", status: "failed" }); + } break; case "turn/completed": if ((message.params.threadId ?? null) !== state.threadId) { diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index 1749cfc8..99328a70 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -140,6 +140,16 @@ export function resolveReviewTarget(cwd, options = {}) { const supportedScopes = new Set(["auto", "working-tree", "branch"]); if (baseRef) { + // Verify the ref exists locally before we kick off a Codex thread; without + // this check the user gets a confusing "Reconnecting..." API error several + // seconds later instead of the actual root cause (typo or unfetched ref). + const verify = git(cwd, ["rev-parse", "--verify", "--quiet", `${baseRef}^{commit}`]); + if (verify.status !== 0) { + throw new Error( + `Base ref "${baseRef}" does not resolve to a commit in this repository. ` + + `Did you mean a different branch, or do you need to fetch it first (e.g. \`git fetch origin ${baseRef}\`)?` + ); + } return { mode: "branch", label: `branch diff against ${baseRef}`, diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index af28d1cf..eca6042d 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -2,6 +2,13 @@ import { spawnSync } from "node:child_process"; import process from "node:process"; export function runCommand(command, args = [], options = {}) { + // On Windows we MUST go through a shell to resolve npm-installed CLIs, which + // are .cmd shims (PATHEXT lookup is shell-only). The previous code used + // `process.env.SHELL || true`, which under Git Bash resolved to /usr/bin/bash + // and rewrote args like `/PID 123 /T /F` as Unix paths + // (`/c/Program Files/Git/PID 123 /T /F`), breaking taskkill. Pin to cmd.exe + // so we keep .cmd resolution but skip MSYS path translation. On POSIX + // shell:false is fine. const result = spawnSync(command, args, { cwd: options.cwd, env: options.env, @@ -9,7 +16,7 @@ export function runCommand(command, args = [], options = {}) { input: options.input, maxBuffer: options.maxBuffer, stdio: options.stdio ?? "pipe", - shell: process.platform === "win32" ? (process.env.SHELL || true) : false, + shell: process.platform === "win32" ? "cmd.exe" : false, windowsHide: true });