refactor(acpx): split Windows command parsing#60692
Conversation
PR SummaryMedium Risk Overview Adds focused Vitest coverage for Windows path/quoting edge cases and introduces an explicit Windows policy to reject direct wrapper-script commands ( Reviewed by Cursor Bugbot for commit 4895dc6. Bugbot is set up for automated code reviews on this repo. Configure here. |
154627e to
4895dc6
Compare
|
Landed via temp rebase onto main.
Thanks @steipete! |
Greptile SummaryThis PR extracts Windows command-line parsing from Confidence Score: 5/5Safe to merge; no P0 or P1 findings. The refactoring is clean and functionally correct. All remaining findings are P2 — a minor readability improvement to splitCommandLine and a missing session/fork test case that does not affect runtime behavior. No regressions introduced. No files require special attention.
|
| const parts = windowsCommand ?? splitCommandParts(value, platform); | ||
| if (parts.length === 0) { | ||
| throw new Error("Invalid agent command: empty command"); | ||
| } |
There was a problem hiding this comment.
parts.length === 0 silently no-ops when windowsCommand is the object branch
When splitWindowsExecutableCommand returns a { command, args } object, parts is that object and parts.length is undefined — not 0 — so the guard never fires on that code path. This is harmless in practice (the function only returns non-null on a successful regex match, guaranteeing a non-empty command), but the guard reads as if it protects both branches equally. A small clarifying comment or restructuring makes the intent explicit.
| const parts = windowsCommand ?? splitCommandParts(value, platform); | |
| if (parts.length === 0) { | |
| throw new Error("Invalid agent command: empty command"); | |
| } | |
| const windowsCommand = splitWindowsExecutableCommand(value, platform); | |
| if (windowsCommand) { | |
| assertSupportedWindowsCommand(windowsCommand.command, platform); | |
| return windowsCommand; | |
| } | |
| const parts = splitCommandParts(value, platform); | |
| if (parts.length === 0) { | |
| throw new Error("Invalid agent command: empty command"); | |
| } | |
| const parsed = { command: parts[0], args: parts.slice(1) }; | |
| assertSupportedWindowsCommand(parsed.command, platform); | |
| return parsed; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/acpx/src/runtime-internals/mcp-command-line.mjs
Line: 111-114
Comment:
**`parts.length === 0` silently no-ops when `windowsCommand` is the object branch**
When `splitWindowsExecutableCommand` returns a `{ command, args }` object, `parts` is that object and `parts.length` is `undefined` — not `0` — so the guard never fires on that code path. This is harmless in practice (the function only returns non-null on a successful regex match, guaranteeing a non-empty command), but the guard reads as if it protects both branches equally. A small clarifying comment or restructuring makes the intent explicit.
```suggestion
const windowsCommand = splitWindowsExecutableCommand(value, platform);
if (windowsCommand) {
assertSupportedWindowsCommand(windowsCommand.command, platform);
return windowsCommand;
}
const parts = splitCommandParts(value, platform);
if (parts.length === 0) {
throw new Error("Invalid agent command: empty command");
}
const parsed = { command: parts[0], args: parts.slice(1) };
assertSupportedWindowsCommand(parsed.command, platform);
return parsed;
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 154627eea8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args: parts.slice(1), | ||
| }; | ||
| } | ||
| import { splitCommandLine } from "./mcp-command-line.mjs"; |
There was a problem hiding this comment.
Ship the new MCP command-line helper with proxy asset
mcp-proxy.mjs now depends on ./mcp-command-line.mjs, but the postbuild static-asset list still only copies mcp-proxy.mjs into dist (scripts/runtime-postbuild.mjs STATIC_EXTENSION_ASSETS). That means packaged runs can fail with ERR_MODULE_NOT_FOUND when ACPX launches the proxy path, because dist/extensions/acpx/mcp-command-line.mjs is never produced.
Useful? React with 👍 / 👎.
Summary
mcp-proxy.mjsinto a dedicated helper modulecmd.exe /c,powershell.exe -File, ornode <script>Why
Follow-up to #60689.
This keeps the proxy entrypoint small, puts the Windows parser behavior under dedicated coverage, and makes unsupported direct wrapper-script spawning explicit instead of implicit.
Verification
pnpm exec vitest run --config vitest.extension-acpx.config.ts extensions/acpx/src/runtime-internals/mcp-command-line.test.ts extensions/acpx/src/runtime-internals/mcp-proxy.test.ts --reporter=verbose --maxWorkers=1 --no-file-parallelismpnpm tsgoandpnpm buildwere started in this workspace, but both long-running wrappers were sticky/noisy here; the focused ACPX suite above completed cleanlyRefs #60689.