Fix Windows CLI bridge for cmd launchers#64
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for windowsVerbatimArguments when spawning subprocesses on Windows, and refactors createSpawnPlan to accept a mockable runtime for improved testability. It also adds corresponding unit tests. The reviewer pointed out that using the call command to execute batch files via cmd.exe introduces issues with double-parsing, specifically affecting variable expansion and caret escaping. They suggested wrapping the entire command string in an extra pair of double quotes instead of using call, and provided suggestions to update both the implementation and the test assertions accordingly.
| command: process.env.ComSpec || "cmd.exe", | ||
| args: ["/d", "/s", "/c", [quoteWindowsCmdArg(resolved), ...args.map(quoteWindowsCmdArg)].join(" ")] | ||
| command: runtime.comSpec ?? process.env.ComSpec ?? "cmd.exe", | ||
| args: ["/d", "/s", "/c", ["call", quoteWindowsCmdArg(resolved), ...args.map(quoteWindowsCmdArg)].join(" ")], |
There was a problem hiding this comment.
Using call to execute batch files via cmd.exe introduces two major issues with argument parsing:
- Variable Expansion:
callperforms an extra round of variable expansion. If any argument contains%(e.g.,%VAR%or%1),cmd.exewill attempt to expand it as a batch variable, which can lead to unexpected behavior or security issues if the arguments contain user-controlled input. - Caret Escaping:
callparses carets (^) twice, which can strip or corrupt arguments containing carets.
Instead of using call, the standard and robust way to prevent cmd.exe from stripping quotes under /s is to wrap the entire command string in an extra pair of double quotes (e.g., cmd.exe /d /s /c ""path\to\tool.cmd" "arg1" "arg2""). This avoids the second parsing pass entirely, making argument passing much more reliable and predictable.
| args: ["/d", "/s", "/c", ["call", quoteWindowsCmdArg(resolved), ...args.map(quoteWindowsCmdArg)].join(" ")], | |
| args: ["/d", "/s", "/c", '"' + [quoteWindowsCmdArg(resolved), ...args.map(quoteWindowsCmdArg)].join(' ') + '"'], |
| args: [ | ||
| "/d", | ||
| "/s", | ||
| "/c", | ||
| "call \"C:\\Users\\example\\AppData\\Roaming\\npm\\codex.cmd\" \"exec\" \"--color\" \"never\" \"-\"" | ||
| ], |
There was a problem hiding this comment.
Update the test expectation to match the robust double-quoted command string format instead of the call wrapper.
| args: [ | |
| "/d", | |
| "/s", | |
| "/c", | |
| "call \"C:\\Users\\example\\AppData\\Roaming\\npm\\codex.cmd\" \"exec\" \"--color\" \"never\" \"-\"" | |
| ], | |
| args: [ | |
| "/d", | |
| "/s", | |
| "/c", | |
| "\"\"C:\\Users\\example\\AppData\\Roaming\\npm\\codex.cmd\" \"exec\" \"--color\" \"never\" \"-\"\"" | |
| ], |
1 participant
Summary
.cmdand.batlaunchers by resolving them correctly and invoking them throughcmd.exewith verbatim argumentscall "<tool>.cmd" ...wrapper so MartinLoop can launch Codex and similar Windows shims from governed runsVerification
pnpm --filter @martin/adapters test -- --run tests/claude-cli.test.tspnpm --filter @martin/adapters lintpnpm buildpnpm lintpnpm oss:validatepnpm --filter @martinloop/mcp verify:release--engine codexrun against the seeded demo workspace now gets past the original Windows launcher failure and reaches downstream runtime behaviorNotes
pnpm testis still intermittently timing out on one existing Windows rollback test inpackages/core/tests/runtime.test.tsunder full-suite load on this machine. The same test passes in isolation, so I left that unrelated instability out of this PR.