Conversation
runClientCommand() launches .cmd shims such as npx.cmd through `cmd.exe /d /s /c`, and escaped the resolved program path with the same escapeCmdArg() used for arguments. That produces ^"C:\Program Files\...^". For arguments this works, because cmd.exe strips the carets and passes the quotes on to the program. The program path, though, is parsed by cmd.exe itself, where ^" is a literal character rather than a quote, so the path splits at the first space: '"C:\Program' is not recognized as an internal or external command This broke `firecrawl setup mcp`, skills installs and every other setup step that runs npx whenever Node lives under C:\Program Files, which is the default install location. Quote the program path with plain quotes (Windows paths cannot contain `"`) and leave argument escaping unchanged. Escaping the arguments with unescaped outer quotes as well would stop the carets from being consumed, so an argument like `https://host/mcp?a=1&b=2` would arrive as `a=1^&b=2`. The existing win32 test mocked child_process and asserted the ^"...^" form, so it could not see the failure. Update that assertion, and add a win32-only test that spawns a real cmd.exe with a .cmd shim under "Program Files (x86)" and checks the argv it receives. Fixes firecrawl#188
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
cmd.exe expands %VAR% (and !VAR! under delayed expansion) even inside double quotes, where a caret is a literal character. With the command path now in plain quotes, a launcher under a directory such as `dir%USERNAME%x` was rewritten to the variable's value and failed with "The system cannot find the path specified" (the previous caret-escaped form handled this, but only for paths without spaces). Close the quotes around each % and ! and caret-escape it outside them, e.g. "C:\a"^%"X"^%"b\npx.cmd", so the path is passed through literally whether or not it contains spaces. Extend the real cmd.exe test to cover %USERNAME% in the path, with and without spaces.
|
@cubic-dev-ai review this PR |
@KassaSana I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
setup-windows-spawn.test.ts only runs on win32, and the main test job runs on ubuntu-latest, so those tests were always skipped in CI. Add a windows-latest job that runs just that file, reusing the existing Node and pnpm setup steps. It targets the one file rather than adding Windows to the main job, because the full suite has pre-existing Windows-only failures (path assumptions in setup, credentials and web-defaults tests) that are identical on main.
|
@cubic-dev-ai review this PR |
@KassaSana I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes Windows cmd.exe quoting for CLI commands by using real quotes on the command token while keeping caret-escaping for arguments, verified by real cmd.exe spawn tests and CI. The fix is focused and clearly beneficial.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes Windows cmd.exe quoting for CLI command paths by using real quotes on the command token while keeping caret-escaping for arguments, with real cmd.exe spawn tests and a Windows CI job. Bounded, test-verified bug fix.
Re-trigger cubic
|
@developersdigest could you approve the workflow runs when you get a chance? They're waiting on first-time contributor approval, and this PR adds a Windows job that exercises the fix for #188 against a real cmd.exe. Thanks! |
Problem
On Windows,
firecrawl setup mcp(and every other setup step that runsnpx) fails whenever Node is installed underC:\Program Files\, which is the default install location:Fixes #188
Root cause
runClientCommand()launches.cmdshims throughcmd.exe /d /s /cand escaped the resolved program path with the sameescapeCmdArg()used for arguments, producing^"C:\Program Files\nodejs\npx.cmd^".That escaping is correct for arguments: cmd.exe strips the carets and passes the quotes on to the program's C-runtime parser. The program path, though, is parsed by cmd.exe itself, where
^"is a literal character rather than a quote, so the path splits at the first space.Changes
quoteCmdCommand()and use it for the command token only. It wraps the program path in plain quotes (Windows paths cannot contain", and one would be rejected). cmd.exe still expands%VAR%/!VAR!inside quotes, where a caret is literal, so each%/!is moved outside the quotes and caret-escaped:"C:\a"^%"X"^%"b\npx.cmd". Argument escaping is unchanged.runClientCommandso it can be tested against a realcmd.exe.setup.test.tsmockschild_processand asserted the^"…^"form, so it passed while the real spawn failed. Updated it to assert a plainly quoted command token.setup-windows-spawn.test.ts(describe.runIf(win32)) spawns a realcmd.exewith a.cmdshim underProgram Files (x86)and checks that the argv arrives exactly, including&and${FIRECRAWL_API_KEY}. It also covers a path containing%USERNAME%, with and without spaces (thanks cubic for flagging that case).test-windows-spawnjob onwindows-latest(pull requests only, liketest-binaryandtest-npm-package) that runs justsetup-windows-spawn.test.ts, since the maintestjob runs on Linux where those tests are skipped.This deliberately differs from the patch suggested in #188, which changed
escapeCmdArgfor every argument. With unescaped outer quotes cmd.exe stops consuming the carets, sohttps://x.dev/mcp?a=1&b=2would arrive asa=1^&b=2. Thanks to @matheusjosedesouzabispo-blip for pinpointingescapeCmdArg.Note: #187 moves
runClientCommandintosrc/utils/run-client-command.tswith the sameescapeCmdArg(resolved)line. If that lands first, the same one-line change applies there, and I'm happy to rebase.Testing
Windows 11, Node 26, pnpm 10.12.1 (the repo's pinned version)
Program Files (x86)fails before the fix with'"C:\...\Program' is not recognized, and the%USERNAME%cases fail with the plain-quote version. All 3 pass now.firecrawl setup mcp --project --agent claude-code -y --keyless'"C:\Program' is not recognized …→Failed to configure Firecrawl MCPDone!, writes.mcp.jsonwithhttps://mcp.firecrawl.dev/v2/mcpmainand on this branch (Windows path assumptions insetup,credentialsandweb-defaultstests). This PR adds no new failures.Linux, Node 22 (container),
pnpm buildthenvitest runmain: 619 passed. This branch: 619 passed, plus 3 skipped (the win32-only tests).pnpm type-checkandpnpm buildpass. Prettier is clean on the changed files, and the pre-commit lint-staged hook ran.