Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,31 @@ jobs:
- name: Run tests
run: pnpm run test

# 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

@cubic-dev-ai cubic-dev-ai Bot Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Suggested change
runs-on: windows-latest
if: github.event_name == 'pull_request'
runs-on: windows-latest
Fix with cubic


steps:
- name: Checkout repository
uses: actions/checkout@v6

- name: Setup Node.js
uses: actions/setup-node@v6
with:
node-version: '22'

- name: Setup pnpm
uses: pnpm/action-setup@v4
with:
version: 10.12.1

- name: Install dependencies
run: pnpm install --frozen-lockfile

- name: Run Windows spawn tests
run: pnpm exec vitest run src/__tests__/commands/setup-windows-spawn.test.ts

test-binary:
if: github.event_name == 'pull_request'
strategy:
Expand Down
58 changes: 58 additions & 0 deletions src/__tests__/commands/setup-windows-spawn.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { describe, expect, it } from 'vitest';
import {
mkdirSync,
mkdtempSync,
readFileSync,
rmSync,
writeFileSync,
} from 'fs';
import os from 'os';
import path from 'path';
import { runClientCommand } from '../../commands/setup';

// Spawns a real cmd.exe, unlike setup.test.ts which mocks child_process.
describe.runIf(process.platform === 'win32')(
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
'runClientCommand on Windows (real cmd.exe)',
() => {
it.each([
['spaces and parentheses', 'Program Files (x86)'],
// cmd.exe expands %VAR% even inside quotes, so the path must not be
// rewritten to the variable's value.
['a %VAR% sequence', 'dir %USERNAME% x'],
['a %VAR% sequence without spaces', 'dir%USERNAME%x'],
])(
'launches a .cmd shim under a path with %s and passes argv through exactly',
(_, dirName) => {
const root = mkdtempSync(path.join(os.tmpdir(), 'firecrawl-spawn-'));
const bin = path.join(root, dirName, 'nodejs');
const outFile = path.join(root, 'argv.json');
try {
mkdirSync(bin, { recursive: true });
writeFileSync(
path.join(bin, 'record-argv.js'),
'require("fs").writeFileSync(process.env.ARGV_OUT, JSON.stringify(process.argv.slice(2)));\n'
);
writeFileSync(
path.join(bin, 'record-argv.cmd'),
'@echo off\r\nnode "%~dp0record-argv.js" %*\r\n'
);
const args = [
'--name',
'firecrawl',
'https://mcp.firecrawl.dev/v2/mcp?a=1&b=2',
'Authorization: Bearer ${FIRECRAWL_API_KEY}',
];

runClientCommand(path.join(bin, 'record-argv.cmd'), args, {
stdio: 'pipe',
env: { ...process.env, ARGV_OUT: outFile },
});

expect(JSON.parse(readFileSync(outFile, 'utf8'))).toEqual(args);
} finally {
rmSync(root, { recursive: true, force: true });
}
}
);
}
);
6 changes: 5 additions & 1 deletion src/__tests__/commands/setup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,11 @@ describe('handleSetupCommand', () => {
expect(command).toBe('cmd.exe');
expect(passthruArgs.slice(0, 3)).toEqual(['/d', '/s', '/c']);
expect(opts?.windowsVerbatimArguments).toBe(true);
expect(passthruArgs[3]).toContain(`^\"${path.join(bin, 'npx.CMD')}^\"`);
// cmd.exe parses the command token itself, so its quotes must stay real
// quotes: a caret-escaped ^" is a literal character there, and the path
// would split at the space in "Program Files".
expect(passthruArgs[3]).toContain(`""${path.join(bin, 'npx.CMD')}" `);
expect(passthruArgs[3]).not.toContain(`^\"${path.join(bin, 'npx.CMD')}`);
expect(passthruArgs[3]).toContain('add-mcp@1.14.0');
expect(passthruArgs[3]).toContain(
'^"Authorization: Bearer ${FIRECRAWL_API_KEY}^"'
Expand Down
19 changes: 17 additions & 2 deletions src/commands/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,21 @@ function escapeCmdArg(arg: string): string {
return quoted.replace(CMD_META_CHARS, '^$1');
}

/** Quote the program path that cmd.exe itself resolves. Unlike arguments, this
* token is parsed by cmd.exe, where a caret-escaped ^" is a literal character
* rather than a quote, so escaping it would split paths such as
* `C:\Program Files\nodejs\npx.cmd` at the space. Windows paths cannot contain
* `"`. cmd.exe still expands `%VAR%` (and `!VAR!` under delayed expansion)
* inside quotes, and a caret is literal there, so each `%`/`!` is placed
* outside the quotes and caret-escaped: `"C:\a"^%"X"^%"b\npx.cmd"`. */
function quoteCmdCommand(command: string): string {
rejectCommandControlCharacters(command, 'Command');
if (command.includes('"')) {
throw new Error('Command path contains an unsupported quote character.');
}
return `"${command.replace(/[%!]/g, '"^$&"')}"`;
}

function windowsPathExtensions(env: NodeJS.ProcessEnv): string[] {
const configured = env.PATHEXT ?? '.COM;.EXE;.BAT;.CMD';
return configured
Expand Down Expand Up @@ -168,7 +183,7 @@ function resolveWindowsCommand(
* On every other platform we spawn the binary directly with no shell, exactly as
* `execFileSync` did before.
*/
function runClientCommand(
export function runClientCommand(
command: string,
args: string[],
options: Parameters<typeof execFileSync>[2]
Expand All @@ -189,7 +204,7 @@ function runClientCommand(
return;
}

const line = [escapeCmdArg(resolved), ...args.map(escapeCmdArg)].join(' ');
const line = [quoteCmdCommand(resolved), ...args.map(escapeCmdArg)].join(' ');
const comspec = env.ComSpec ?? env.COMSPEC ?? 'cmd.exe';
const windowsOptions = {
...options,
Expand Down