diff --git a/packages/core/src/__tests__/code-review-manager.test.ts b/packages/core/src/__tests__/code-review-manager.test.ts index 8c9e4802af..9a972643d3 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, @@ -17,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, @@ -631,6 +633,79 @@ 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("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 756cc3d1c1..8b412c5858 100644 --- a/packages/core/src/code-review-manager.ts +++ b/packages/core/src/code-review-manager.ts @@ -34,6 +34,19 @@ 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. +// 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; + 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 +84,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,