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.
1 issue found across 4 files
Confidence score: 5/5
- In
.github/workflows/test.yml, the new job runs on pushes tomain/master, using a Windows VM and fullpnpm installfor only three tests; restrict it to pull requests to avoid unnecessary CI cost.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/test.yml">
<violation number="1" location=".github/workflows/test.yml:46">
P3: This new job runs on every push to main/master as well as PRs, spending a Windows VM and a full `pnpm install` to execute only three tests. Gate it with `if: github.event_name == 'pull_request'`, matching the other add-on jobs in this file (`test-binary` and `test-npm-package`).</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| # The main test job runs on Linux, where the real cmd.exe spawn tests are | ||
| # skipped. Run them on Windows so command quoting stays covered. | ||
| test-windows-spawn: | ||
| runs-on: windows-latest |
There was a problem hiding this comment.
P3: This new job runs on every push to main/master as well as PRs, spending a Windows VM and a full pnpm install to execute only three tests. Gate it with if: github.event_name == 'pull_request', matching the other add-on jobs in this file (test-binary and test-npm-package).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/test.yml, line 46:
<comment>This new job runs on every push to main/master as well as PRs, spending a Windows VM and a full `pnpm install` to execute only three tests. Gate it with `if: github.event_name == 'pull_request'`, matching the other add-on jobs in this file (`test-binary` and `test-npm-package`).</comment>
<file context>
@@ -40,6 +40,31 @@ jobs:
+ # The main test job runs on Linux, where the real cmd.exe spawn tests are
+ # skipped. Run them on Windows so command quoting stays covered.
+ test-windows-spawn:
+ runs-on: windows-latest
+
+ steps:
</file context>
| runs-on: windows-latest | |
| if: github.event_name == 'pull_request' | |
| runs-on: windows-latest |
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).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.