mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-07-31 20:32:20 +03:00
* fix(cli): escape codex args and stop aborting on exit in launch-codex `launch-codex` spawns `codex.cmd` with `shell: true` on Windows, so Node joins argv with plain spaces and no escaping (DEP0190). This mangles every Windows invocation, not only the ones with a multi-word user argument, because the injected `-c` provider flags carry quoted TOML values: ["-c","model_provider=omniroute", ..., "model_providers.omniroute.base_url=http://localhost:20128/v1", "fix","the","bug"] cmd.exe strips the TOML quotes (`model_provider=omniroute` no longer parses as a TOML string), splits multi-word arguments, and swallows everything after an unquoted `&`. The same defect was fixed for `launch` in #8837; this ports it to `launch-codex`, which that PR disclosed but left unfixed. - extract the escaping into `bin/cli/utils/winShellArgs.mjs` and reuse it from both launchers instead of keeping a private copy in `launch.mjs` - quote the codex argv (provider flags + profile + pass-through args) on the win32 shell path; argv is untouched off Windows, where no shell is involved - replace `process.exit()` in the command action with `process.exitCode`: on any non-zero child exit it aborted with the libuv `!(handle->flags & UV_HANDLE_CLOSING)` assertion while the inherited stdio handles were closing Test: `tests/unit/cli/launch-codex-windows-spawn-args.test.ts` pins the exact encoding with golden strings (the cmd.exe round-trip is Windows-only and skips on Linux CI, so without goldens CI would guard nothing) and round-trips the real provider flags through a probe `.cmd` shim that forwards `%*`. * docs(changelog): add fragment for #8856 Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
130 lines
4.9 KiB
TypeScript
130 lines
4.9 KiB
TypeScript
import { test } from "node:test";
|
|
import assert from "node:assert/strict";
|
|
import { spawn } from "node:child_process";
|
|
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
|
|
import { join } from "node:path";
|
|
import { tmpdir } from "node:os";
|
|
|
|
import {
|
|
buildCodexProviderArgs,
|
|
quoteCodexArgs,
|
|
resolveCodexSpawn,
|
|
} from "../../../bin/cli/commands/launch-codex.mjs";
|
|
|
|
const isWindows = process.platform === "win32";
|
|
|
|
test("resolveCodexSpawn: win32 spawns codex.cmd through a shell", () => {
|
|
const { command, shell } = resolveCodexSpawn("win32");
|
|
assert.equal(command, "codex.cmd");
|
|
assert.equal(shell, true);
|
|
});
|
|
|
|
test("resolveCodexSpawn: non-Windows platforms spawn the bare binary without a shell", () => {
|
|
for (const platform of ["linux", "darwin", "freebsd"]) {
|
|
const { command, shell } = resolveCodexSpawn(platform);
|
|
assert.equal(command, "codex", `${platform} command`);
|
|
assert.equal(shell, undefined, `${platform} shell`);
|
|
}
|
|
});
|
|
|
|
test("quoteCodexArgs leaves argv untouched off Windows (no shell, no quoting)", () => {
|
|
const args = [...buildCodexProviderArgs("http://localhost:20128"), "explain this repo"];
|
|
assert.deepEqual(quoteCodexArgs(args, "linux"), args);
|
|
});
|
|
|
|
// The injected provider flags are the reason this bug bit every Windows
|
|
// invocation, not just the ones with a multi-word user argument: their TOML
|
|
// values are quoted, and cmd.exe eats those quotes when the line is unescaped.
|
|
test("quoteCodexArgs escapes the injected -c provider flags on win32", () => {
|
|
const input = buildCodexProviderArgs("http://localhost:20128");
|
|
const quoted = quoteCodexArgs(input, "win32");
|
|
assert.equal(quoted.length, input.length);
|
|
for (const [i, arg] of quoted.entries()) {
|
|
assert.notEqual(arg, input[i], `argument ${i} must be escaped`);
|
|
assert.match(arg, /"/, `argument ${i} must be quoted`);
|
|
}
|
|
});
|
|
|
|
// The cmd.exe round-trip below is the real proof, but it can only run on
|
|
// Windows. These golden strings pin the exact encoding so CI (Linux) still
|
|
// fails if the escaping changes — e.g. if the double caret-escape required by
|
|
// the `.cmd` shim's %* re-parse is ever reduced back to a single pass.
|
|
test("quoteCodexArgs: exact win32 encoding (golden)", () => {
|
|
const golden: Array<[string, string]> = [
|
|
["-c", '^^^"-c^^^"'],
|
|
[
|
|
'model_providers.omniroute.base_url="http://localhost:20128/v1"',
|
|
'^^^"model_providers.omniroute.base_url=\\^^^"http://localhost:20128/v1\\^^^"^^^"',
|
|
],
|
|
[
|
|
"model_providers.omniroute.requires_openai_auth=false",
|
|
'^^^"model_providers.omniroute.requires_openai_auth=false^^^"',
|
|
],
|
|
["fix the bug & ship it", '^^^"fix^^^ the^^^ bug^^^ ^^^&^^^ ship^^^ it^^^"'],
|
|
["", '""'],
|
|
];
|
|
for (const [input, expected] of golden) {
|
|
assert.equal(
|
|
quoteCodexArgs([input], "win32")[0],
|
|
expected,
|
|
`encoding of ${JSON.stringify(input)}`
|
|
);
|
|
}
|
|
});
|
|
|
|
test("quoteCodexArgs does not mutate the caller's array", () => {
|
|
const input = ["-c", "model_provider=\"omniroute\""];
|
|
quoteCodexArgs(input, "win32");
|
|
assert.deepEqual(input, ["-c", 'model_provider="omniroute"']);
|
|
});
|
|
|
|
// The real contract: whatever we hand to spawn(shell:true) must arrive at the
|
|
// child's argv byte-identical. Verified against a probe .cmd through the same
|
|
// cmd.exe path the launcher uses.
|
|
test(
|
|
"quoteCodexArgs survives a real cmd.exe round-trip",
|
|
{ skip: isWindows ? false : "windows-only: exercises the cmd.exe shell path" },
|
|
async () => {
|
|
const dir = mkdtempSync(join(tmpdir(), "omniroute-codex-argv-"));
|
|
try {
|
|
// Mirror the real shape of `codex.cmd`: an npm .cmd shim forwarding %*
|
|
// to a node script. Printing argv as JSON keeps the oracle exact.
|
|
writeFileSync(join(dir, "argv.mjs"), "console.log(JSON.stringify(process.argv.slice(2)));\n");
|
|
const probe = join(dir, "probe.cmd");
|
|
writeFileSync(probe, ["@echo off", 'node "%~dp0argv.mjs" %*'].join("\r\n") + "\r\n");
|
|
|
|
const args = [
|
|
...buildCodexProviderArgs("http://localhost:20128"),
|
|
"--profile",
|
|
"auto-best-coding",
|
|
"In one short line: say BANANA",
|
|
'quotes " and & ampersands | pipes',
|
|
"percent %PATH% and caret ^ and bang !",
|
|
"trailing backslash \\",
|
|
"",
|
|
];
|
|
|
|
const received = await new Promise<string[]>((resolve, reject) => {
|
|
const child = spawn(probe, quoteCodexArgs(args, "win32"), {
|
|
shell: true,
|
|
windowsHide: true,
|
|
});
|
|
let out = "";
|
|
child.stdout.on("data", (c) => (out += c));
|
|
child.on("error", reject);
|
|
child.on("exit", () => {
|
|
try {
|
|
resolve(JSON.parse(out.trim().split(/\r?\n/).pop() ?? "[]"));
|
|
} catch (err) {
|
|
reject(new Error(`probe did not emit argv JSON: ${out}`, { cause: err }));
|
|
}
|
|
});
|
|
});
|
|
|
|
assert.deepEqual(received, args, "child argv must match what the caller passed");
|
|
} finally {
|
|
rmSync(dir, { recursive: true, force: true });
|
|
}
|
|
}
|
|
);
|