fix(acpx): make Claude CLI spawn robust on Windows#51366
fix(acpx): make Claude CLI spawn robust on Windows#51366ShionEria wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3a4dc0a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const child = spawn(target.command, target.args, { | ||
| stdio: ["pipe", "pipe", "inherit"], | ||
| env: process.env, | ||
| }); |
There was a problem hiding this comment.
Wrap Windows .cmd/.bat agent commands before spawning
On Windows hosts where targetCommand resolves to a wrapper script such as ...\\claude.cmd --stdio or npx.cmd, this still hands the wrapper directly to child_process.spawn(). The repo already documents that direct .cmd/.bat spawns are rejected on modern Node (src/process/exec.ts:44-47) and routes them through cmd.exe or a resolved entrypoint elsewhere, so these installations will still fail to start the Claude agent even though the new parser now accepts those paths.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a real Windows bug where Claude CLI paths containing spaces (e.g.
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-proxy.mjs
Line: 8-9
Comment:
**`.cmd`/`.bat` files cannot be spawned directly without `shell: true`**
The `WINDOWS_EXECUTABLE_PATH_RE` regex includes `cmd`, `bat`, and `ps1` as recognized Windows executable extensions, but the `spawn` call uses `shell: false` (the default). On Windows, `.cmd`, `.bat`, and `.ps1` files are not executable binaries — they require the shell (`cmd.exe` or `powershell.exe`) to interpret them. Spawning one of these directly will result in an `ENOENT` or `UNKNOWN` error at runtime.
If these extensions are genuinely expected to be valid target commands, the `spawn` call should conditionally set `shell: true` (or pass via `cmd /c`/`powershell -File`) when the resolved command ends with one of these extensions.
If they are only included in the regex as a future-proofing measure but aren't currently supported as target commands, they should be removed from the regex to avoid giving users a false sense that they'll work.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-proxy.mjs
Line: 88-94
Comment:
**Unquoted Windows path with backslashes in arguments is silently corrupted**
When `splitWindowsExecutableCommand` successfully extracts the command, `rest` is passed to `splitCommandParts`. If `rest` contains an unquoted Windows path (e.g. a `--input C:\Users\data.json` argument), `splitCommandParts` is still in Unix-mode and will treat the `\` as an escape character — consuming it and yielding `C:Usersdata.json` silently.
For example:
```
C:\Program Files\Claude\claude.exe --config C:\Users\me\cfg.json
^^^^^^^^^^^^^^^^^^ parsed by splitCommandParts
```
would produce `args: ["--config", "C:Usersmecfg.json"]`.
In practice this is unlikely to affect the current Claude CLI use case (`--stdio`, `--dangerously-skip-permissions`, etc. are plain flags), but it is a silent correctness gap. Consider documenting that argument values containing Windows paths must be quoted, or apply a similar regex-aware treatment to the `rest` portion.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-agent-command.test.ts
Line: 61-71
Comment:
**No test for double-backslash sequences inside quoted Windows paths**
`splitCommandParts` now treats `\\` inside double-quoted strings as a single `\` (escape sequence), while `\X` (any non-special char) is kept as a literal `\X`. The test only exercises the literal case. A test for the escaped-backslash round-trip (i.e. the path that `quoteCommandPart` in `mcp-agent-command.ts` would actually produce — double-escaped backslashes) is missing:
```ts
it("parses double-escaped backslashes inside quoted Windows paths", () => {
// quoteCommandPart produces "C:\\\\Program Files\\\\Claude\\\\claude.exe"
const parsed = splitCommandLine(
'"C:\\\\Program Files\\\\Claude\\\\claude.exe" --stdio',
"win32",
);
expect(parsed).toEqual({
command: "C:\\Program Files\\Claude\\claude.exe",
args: ["--stdio"],
});
});
```
Without this test, a regression in `\\` handling could go undetected even though it would break the live code path (since the payload always stores the output of `quoteCommandPart`, which doubles the backslashes).
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix(acpx): make Clau..." |
| const WINDOWS_EXECUTABLE_PATH_RE = | ||
| /^(?<command>(?:[A-Za-z]:[\\/]|\\\\[^\\/]+[\\/][^\\/]+[\\/]).*?\.(?:exe|cmd|bat|com|ps1|cjs|mjs|js))(?=\s|$)(?:\s+(?<rest>.*))?$/i; |
There was a problem hiding this comment.
.cmd/.bat files cannot be spawned directly without shell: true
The WINDOWS_EXECUTABLE_PATH_RE regex includes cmd, bat, and ps1 as recognized Windows executable extensions, but the spawn call uses shell: false (the default). On Windows, .cmd, .bat, and .ps1 files are not executable binaries — they require the shell (cmd.exe or powershell.exe) to interpret them. Spawning one of these directly will result in an ENOENT or UNKNOWN error at runtime.
If these extensions are genuinely expected to be valid target commands, the spawn call should conditionally set shell: true (or pass via cmd /c/powershell -File) when the resolved command ends with one of these extensions.
If they are only included in the regex as a future-proofing measure but aren't currently supported as target commands, they should be removed from the regex to avoid giving users a false sense that they'll work.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-proxy.mjs
Line: 8-9
Comment:
**`.cmd`/`.bat` files cannot be spawned directly without `shell: true`**
The `WINDOWS_EXECUTABLE_PATH_RE` regex includes `cmd`, `bat`, and `ps1` as recognized Windows executable extensions, but the `spawn` call uses `shell: false` (the default). On Windows, `.cmd`, `.bat`, and `.ps1` files are not executable binaries — they require the shell (`cmd.exe` or `powershell.exe`) to interpret them. Spawning one of these directly will result in an `ENOENT` or `UNKNOWN` error at runtime.
If these extensions are genuinely expected to be valid target commands, the `spawn` call should conditionally set `shell: true` (or pass via `cmd /c`/`powershell -File`) when the resolved command ends with one of these extensions.
If they are only included in the regex as a future-proofing measure but aren't currently supported as target commands, they should be removed from the regex to avoid giving users a false sense that they'll work.
How can I resolve this? If you propose a fix, please make it concise.| return null; | ||
| } | ||
| const rest = match.groups.rest?.trim() ?? ""; | ||
| return { | ||
| command: match.groups.command, | ||
| args: rest ? splitCommandParts(rest) : [], | ||
| }; |
There was a problem hiding this comment.
When splitWindowsExecutableCommand successfully extracts the command, rest is passed to splitCommandParts. If rest contains an unquoted Windows path (e.g. a --input C:\Users\data.json argument), splitCommandParts is still in Unix-mode and will treat the \ as an escape character — consuming it and yielding C:Usersdata.json silently.
For example:
C:\Program Files\Claude\claude.exe --config C:\Users\me\cfg.json
^^^^^^^^^^^^^^^^^^ parsed by splitCommandParts
would produce args: ["--config", "C:Usersmecfg.json"].
In practice this is unlikely to affect the current Claude CLI use case (--stdio, --dangerously-skip-permissions, etc. are plain flags), but it is a silent correctness gap. Consider documenting that argument values containing Windows paths must be quoted, or apply a similar regex-aware treatment to the rest portion.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-proxy.mjs
Line: 88-94
Comment:
**Unquoted Windows path with backslashes in arguments is silently corrupted**
When `splitWindowsExecutableCommand` successfully extracts the command, `rest` is passed to `splitCommandParts`. If `rest` contains an unquoted Windows path (e.g. a `--input C:\Users\data.json` argument), `splitCommandParts` is still in Unix-mode and will treat the `\` as an escape character — consuming it and yielding `C:Usersdata.json` silently.
For example:
```
C:\Program Files\Claude\claude.exe --config C:\Users\me\cfg.json
^^^^^^^^^^^^^^^^^^ parsed by splitCommandParts
```
would produce `args: ["--config", "C:Usersmecfg.json"]`.
In practice this is unlikely to affect the current Claude CLI use case (`--stdio`, `--dangerously-skip-permissions`, etc. are plain flags), but it is a silent correctness gap. Consider documenting that argument values containing Windows paths must be quoted, or apply a similar regex-aware treatment to the `rest` portion.
How can I resolve this? If you propose a fix, please make it concise.| it("parses quoted Windows executable paths without dropping backslashes", () => { | ||
| const parsed = splitCommandLine( | ||
| '"C:\\Program Files\\Claude\\claude.exe" --stdio --flag "two words"', | ||
| "win32", | ||
| ); | ||
|
|
||
| expect(parsed).toEqual({ | ||
| command: "C:\\Program Files\\Claude\\claude.exe", | ||
| args: ["--stdio", "--flag", "two words"], | ||
| }); | ||
| }); |
There was a problem hiding this comment.
splitCommandParts now treats \\ inside double-quoted strings as a single \ (escape sequence), while \X (any non-special char) is kept as a literal \X. The test only exercises the literal case. A test for the escaped-backslash round-trip (i.e. the path that quoteCommandPart in mcp-agent-command.ts would actually produce — double-escaped backslashes) is missing:
it("parses double-escaped backslashes inside quoted Windows paths", () => {
// quoteCommandPart produces "C:\\\\Program Files\\\\Claude\\\\claude.exe"
const parsed = splitCommandLine(
'"C:\\\\Program Files\\\\Claude\\\\claude.exe" --stdio',
"win32",
);
expect(parsed).toEqual({
command: "C:\\Program Files\\Claude\\claude.exe",
args: ["--stdio"],
});
});Without this test, a regression in \\ handling could go undetected even though it would break the live code path (since the payload always stores the output of quoteCommandPart, which doubles the backslashes).
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-agent-command.test.ts
Line: 61-71
Comment:
**No test for double-backslash sequences inside quoted Windows paths**
`splitCommandParts` now treats `\\` inside double-quoted strings as a single `\` (escape sequence), while `\X` (any non-special char) is kept as a literal `\X`. The test only exercises the literal case. A test for the escaped-backslash round-trip (i.e. the path that `quoteCommandPart` in `mcp-agent-command.ts` would actually produce — double-escaped backslashes) is missing:
```ts
it("parses double-escaped backslashes inside quoted Windows paths", () => {
// quoteCommandPart produces "C:\\\\Program Files\\\\Claude\\\\claude.exe"
const parsed = splitCommandLine(
'"C:\\\\Program Files\\\\Claude\\\\claude.exe" --stdio',
"win32",
);
expect(parsed).toEqual({
command: "C:\\Program Files\\Claude\\claude.exe",
args: ["--stdio"],
});
});
```
Without this test, a regression in `\\` handling could go undetected even though it would break the live code path (since the payload always stores the output of `quoteCommandPart`, which doubles the backslashes).
How can I resolve this? If you propose a fix, please make it concise.
Summary
Testing
Fixes #50352