From 7edddcb4416bb00beab8f128cbc60b6deb17891b Mon Sep 17 00:00:00 2001 From: AO Agent Date: Thu, 21 May 2026 21:30:18 -0500 Subject: [PATCH 1/2] fix(code-review, windows): escape args for cmd.exe in execFileWithClosedStdin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, spawn() with shell: true routes through cmd.exe, which concatenates args with spaces and loses multi-word argument boundaries (Node.js DEP0190). The review prompt — a multi-line string with spaces — was split into separate tokens, causing codex exec to fail with "unexpected argument 'are' found". Add escapeArgForCmd() that wraps args containing spaces or cmd.exe metacharacters in double quotes before passing them to spawn(). This preserves argument boundaries through the cmd.exe re-parse while keeping shell: true for .cmd shim resolution. Closes #2003 --- .../src/__tests__/code-review-manager.test.ts | 46 +++++++++++++++++++ packages/core/src/code-review-manager.ts | 13 +++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/packages/core/src/__tests__/code-review-manager.test.ts b/packages/core/src/__tests__/code-review-manager.test.ts index 8c9e4802af..87b32c6fd1 100644 --- a/packages/core/src/__tests__/code-review-manager.test.ts +++ b/packages/core/src/__tests__/code-review-manager.test.ts @@ -9,6 +9,7 @@ import { createCodeReviewStore, type CodeReviewStore } from "../code-review-stor import { buildCodexCodeReviewArgs, CodeReviewNoOpenFindingsError, + escapeArgForCmd, executeCodeReviewRun, markOutdatedCodeReviewRunsForSession, parseReviewerOutput, @@ -631,6 +632,51 @@ describe("runCodexCodeReview", () => { }); }); +describe("escapeArgForCmd", () => { + it("passes through simple args unchanged", () => { + expect(escapeArgForCmd("exec")).toBe("exec"); + expect(escapeArgForCmd("--sandbox")).toBe("--sandbox"); + expect(escapeArgForCmd("read-only")).toBe("read-only"); + }); + + it("wraps args with spaces in double quotes", () => { + expect(escapeArgForCmd("You are an AO reviewer agent.")).toBe( + '"You are an AO reviewer agent."', + ); + }); + + it("escapes embedded double quotes", () => { + expect(escapeArgForCmd('Return {"findings":[]}')).toBe( + // cmd.exe: embedded " → "" + '"Return {' + '""findings""' + ':[]}"', + ); + }); + + it("wraps args containing cmd.exe metacharacters", () => { + expect(escapeArgForCmd("foo&bar")).toBe('"foo&bar"'); + expect(escapeArgForCmd("a|b")).toBe('"a|b"'); + expect(escapeArgForCmd("a>b")).toBe('"a>b"'); + expect(escapeArgForCmd("a { + expect(escapeArgForCmd("")).toBe('""'); + }); + + it("preserves a multi-line review prompt as a single quoted arg", () => { + const prompt = [ + "You are an AO reviewer agent. Review this repository snapshot for concrete bugs only.", + "Do not modify files.", + 'Return only JSON: {"findings":[]}', + ].join("\n"); + const escaped = escapeArgForCmd(prompt); + expect(escaped).toMatch(/^"/); + expect(escaped).toMatch(/"$/); + expect(escaped).toContain('{' + '""findings""' + ':[]}'); + }); +}); + describe("prepareGitReviewerWorkspace", () => { it("prunes stale git worktree metadata when the reviewer workspace directory is gone", async () => { const tmpHome = join(tmpdir(), `ao-test-review-worktree-${randomUUID()}`); diff --git a/packages/core/src/code-review-manager.ts b/packages/core/src/code-review-manager.ts index 756cc3d1c1..72dcfce026 100644 --- a/packages/core/src/code-review-manager.ts +++ b/packages/core/src/code-review-manager.ts @@ -34,6 +34,16 @@ import { getShell, isWindows, killProcessTree } from "./platform.js"; const REVIEW_COMMAND_TIMEOUT_MS = 10 * 60_000; const REVIEW_COMMAND_MAX_BUFFER = 8 * 1024 * 1024; + +// spawn() with shell: true on Windows joins args with spaces and routes through +// cmd.exe, losing multi-word argument boundaries (Node.js DEP0190). This wraps +// args that contain spaces or cmd.exe metacharacters in double quotes so they +// survive the cmd.exe re-parse. +export function escapeArgForCmd(arg: string): string { + if (arg.length > 0 && !/[\s"&|<>^]/.test(arg)) return arg; + return '"' + arg.replace(/"/g, '""') + '"'; +} + const REVIEW_RUN_CREATION_LOCK_FILE = ".create-run.lock"; const REVIEW_RUN_EXECUTION_LOCK_PREFIX = ".execute-run-"; const REVIEW_RUN_CREATION_LOCK_WAIT_MS = 5_000; @@ -71,7 +81,8 @@ async function execFileWithClosedStdin( const { spawn } = await import("node:child_process"); return new Promise((resolve, reject) => { - const child = spawn(file, args, { + const spawnArgs = options.shell && isWindows() ? args.map(escapeArgForCmd) : args; + const child = spawn(file, spawnArgs, { cwd: options.cwd, env: options.env, shell: options.shell, From 5157ea202d806233c07b01168919cc6ce3bf69e6 Mon Sep 17 00:00:00 2001 From: AO Agent Date: Thu, 21 May 2026 22:48:15 -0500 Subject: [PATCH 2/2] fix: add % to cmd.exe metachar regex and platform-guard tests Include % in the escapeArgForCmd detection regex so args containing percent signs get double-quoted. Note: cmd.exe %VAR% expansion cannot be fully suppressed inside double quotes, but the review prompt and file-path args never contain literal % in practice. Add platform-guard tests using Object.defineProperty(process, 'platform') to verify the isWindows() branch in execFileWithClosedStdin fires correctly on both platforms. --- .../src/__tests__/code-review-manager.test.ts | 29 +++++++++++++++++++ packages/core/src/code-review-manager.ts | 5 +++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/core/src/__tests__/code-review-manager.test.ts b/packages/core/src/__tests__/code-review-manager.test.ts index 87b32c6fd1..9a972643d3 100644 --- a/packages/core/src/__tests__/code-review-manager.test.ts +++ b/packages/core/src/__tests__/code-review-manager.test.ts @@ -18,6 +18,7 @@ import { triggerCodeReviewForSession, } from "../code-review-manager.js"; import { createInitialCanonicalLifecycle } from "../lifecycle-state.js"; +import { isWindows } from "../platform.js"; import { SessionNotFoundError, type OrchestratorConfig, @@ -658,6 +659,7 @@ describe("escapeArgForCmd", () => { expect(escapeArgForCmd("a>b")).toBe('"a>b"'); expect(escapeArgForCmd("a { @@ -677,6 +679,33 @@ describe("escapeArgForCmd", () => { }); }); +describe("execFileWithClosedStdin platform guard", () => { + const args = ["exec", "--sandbox", "read-only", "You are a reviewer."]; + const shellEnabled = true; + + it("escapes args when isWindows() is true and shell is enabled", () => { + const orig = Object.getOwnPropertyDescriptor(process, "platform")!; + Object.defineProperty(process, "platform", { value: "win32", configurable: true }); + try { + const result = shellEnabled && isWindows() ? args.map(escapeArgForCmd) : args; + expect(result[3]).toBe('"You are a reviewer."'); + } finally { + Object.defineProperty(process, "platform", orig); + } + }); + + it("leaves args untouched when isWindows() is false", () => { + const orig = Object.getOwnPropertyDescriptor(process, "platform")!; + Object.defineProperty(process, "platform", { value: "linux", configurable: true }); + try { + const result = shellEnabled && isWindows() ? args.map(escapeArgForCmd) : args; + expect(result[3]).toBe("You are a reviewer."); + } finally { + Object.defineProperty(process, "platform", orig); + } + }); +}); + describe("prepareGitReviewerWorkspace", () => { it("prunes stale git worktree metadata when the reviewer workspace directory is gone", async () => { const tmpHome = join(tmpdir(), `ao-test-review-worktree-${randomUUID()}`); diff --git a/packages/core/src/code-review-manager.ts b/packages/core/src/code-review-manager.ts index 72dcfce026..8b412c5858 100644 --- a/packages/core/src/code-review-manager.ts +++ b/packages/core/src/code-review-manager.ts @@ -39,8 +39,11 @@ const REVIEW_COMMAND_MAX_BUFFER = 8 * 1024 * 1024; // cmd.exe, losing multi-word argument boundaries (Node.js DEP0190). This wraps // args that contain spaces or cmd.exe metacharacters in double quotes so they // survive the cmd.exe re-parse. +// Note: %VAR% expansion in cmd.exe cannot be fully suppressed inside double +// quotes, but wrapping still protects against the other metacharacters. In +// practice the review prompt and file-path args never contain literal `%`. export function escapeArgForCmd(arg: string): string { - if (arg.length > 0 && !/[\s"&|<>^]/.test(arg)) return arg; + if (arg.length > 0 && !/[\s"&|<>^%]/.test(arg)) return arg; return '"' + arg.replace(/"/g, '""') + '"'; }