Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions packages/core/src/__tests__/code-review-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { createCodeReviewStore, type CodeReviewStore } from "../code-review-stor
import {
buildCodexCodeReviewArgs,
CodeReviewNoOpenFindingsError,
escapeArgForCmd,
executeCodeReviewRun,
markOutdatedCodeReviewRunsForSession,
parseReviewerOutput,
Expand All @@ -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,
Expand Down Expand Up @@ -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<b")).toBe('"a<b"');
expect(escapeArgForCmd("a^b")).toBe('"a^b"');
expect(escapeArgForCmd("100%done")).toBe('"100%done"');
});

it("wraps empty string in double quotes", () => {
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", () => {
Comment thread
greptile-apps[bot] marked this conversation as resolved.
it("prunes stale git worktree metadata when the reviewer workspace directory is gone", async () => {
const tmpHome = join(tmpdir(), `ao-test-review-worktree-${randomUUID()}`);
Expand Down
16 changes: 15 additions & 1 deletion packages/core/src/code-review-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, '""') + '"';
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.

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;
Expand Down Expand Up @@ -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,
Expand Down