[STG-2279] fix(cli): quote win32 npx spawn + bound skills installer stages

[shrey150]

Summary

Linear: https://linear.app/browserbase/issue/STG-2279/fix-windows-skills-add-npx-quoting-and-bound-installer-timeouts

Fixes browse skills add on Windows (cmd.exe spawn quoting) and bounds the two unbounded skills-installer stages (the npx skills add child and the catalog/file fetches).

Impact if merged

skills.add succeeds for 3.2% of Windows users (54 in the last 30d) — effectively broken for every default C:\Program Files\nodejs Node install, because the npx child is spawned through cmd.exe with an unquoted path. Windows devs are a smaller slice of the CLI base, but skill installers are the single highest-value cohort in our telemetry: 7x the engagement (median 12.5 vs 2 commands) and 19x the multi-day retention (28.4% vs 1.5%) of non-installers, and skills.find→add is an agent-facing funnel (51% of finders attempt an install within an hour). This also bounds the two unbounded installer stages (npx child: 180s; catalog fetches: 10s) — today they can hang forever, feeding the slow-failure retry loops that dominate telemetry volume.

Implementation notes

Root cause. findExecutable resolves npx via PATH+PATHEXT to npx.cmd on Windows, and spawnPassthrough spawns it with shell: true (required for .cmd/.bat shims). Node's shell: true joins command+args unquoted into cmd.exe /d /s /c "...", so C:\Program Files\nodejs\npx.cmd splits at the space and cmd executes C:\Program → 'C:\Program' is not recognized → exit 1. Install-path args under C:\Users\<First Last>\... break the same way.

Why not shell: false. Spawning a .cmd directly with shell: false throws EINVAL on all current Node versions — the CVE-2024-27980 hardening (Node 18.20.x / 20.12.x / 21.7.x+) forbids spawning batch files without a shell because cmd.exe argument splitting cannot be made injection-safe generically. So the shell path is mandatory for .cmd shims, and the args must be quoted for cmd.

Quoting semantics. quoteForCmdShell wraps tokens containing whitespace, quotes, or cmd metacharacters (^ & | < >) in double quotes, doubling embedded quotes. Node wraps the joined string in outer quotes after /d /s /c; with /s, cmd strips only those outer quotes and executes the inner, correctly-quoted line:

before:  cmd.exe /d /s /c "C:\Program Files\nodejs\npx.cmd --yes skills add C:\Users\First Last\..."
after:   cmd.exe /d /s /c ""C:\Program Files\nodejs\npx.cmd" --yes skills add "C:\Users\First Last\...""

Alternative considered. Resolving npx-cli.js next to npx.cmd and spawning process.execPath with shell: false would avoid cmd quoting entirely, but the npx.cmd → npx-cli.js relative layout differs across npm versions and Node distribution channels (nvm-windows, Volta, Scoop shims, fnm), so it trades a well-understood quoting rule for fragile path archaeology. The quoting approach is smaller and matches what cross-platform tools (e.g. cross-spawn) do.

Bounding the installer stages.

• spawnPassthrough now enforces a 180s deadline: SIGTERM, then SIGKILL after 5s if the child ignores it. A timed-out install fails with a clear message and a distinct skill_install_timeout result code through the existing fail/resultCode plumbing from [STG-2192] fix(cli): make browse skills add failures diagnosable + fail cleanly on unknown skill #2210.
• The catalog file-list fetch, the direct-Blob HEAD probe, and skill-file downloads now use AbortSignal.timeout(10s). An aborted catalog fetch is classified exactly like a network failure (unavailable), preserving the existing fallback semantics.
• Both deadlines are env-overridable (BROWSE_SKILLS_INSTALL_TIMEOUT_MS, BROWSE_SKILLS_FETCH_TIMEOUT_MS), following the module's existing BROWSE_SKILLS_* override pattern; this is also what makes the deadlines provable end-to-end in tests.

E2E Test Matrix

All commands ran against the locally built CLI (<local build>/bin/run.js) on macOS (darwin/arm64).

Command / flow	Observed output	Confidence / sufficiency
Windows execution (one-time windows-latest before/after run: actions/runs/27448084696)	System Node at C:\Program Files\nodejs (no setup-node), where npx → C:\Program Files\nodejs\npx.cmd. main: browse skills install → exit 1, verbatim 'C:\Program' is not recognized as an internal or external command. PR head d2e9098: exit 0, Installed 1 skill, ~\.agents\skills\browse\SKILL.md present; win32 vitest gate (quoting + shim + spawnPassthrough timeout) 10/10 passed.	Closes the Windows gap with a real before/after on the same runner layout (win25-vs2026): the exact predicted failure reproduces on main and the PR build installs end-to-end. Full evidence in the validation comment.
browse skills find flights (real catalog)	exit 0; returned google.com/search-flights-ts4g1f with full metadata	Proves catalog discovery is unaffected.
browse skills add google.com/search-flights-ts4g1f (real catalog, real npx)	exit 0; Downloaded 2 skill files to <config dir>; npx skills add installed the skill ("Installed 1 skill ... Done!")	Proves the darwin install path (quoting branch not taken) still works end-to-end with the new deadline code in place — no regression.
Quoting before/after for C:\Program Files\nodejs\npx.cmd (unit tests + helper output)	before: C:\Program Files\nodejs\npx.cmd --yes skills add C:\Users\First Last\... (unquoted → cmd runs C:\Program); after: "C:\Program Files\nodejs\npx.cmd" --yes skills add "C:\Users\First Last\..."	Reproduces the bug shape and asserts the exact corrected command line, incl. embedded-quote doubling, & | ^ < > metachars, and the empty token. Static proof only — see Windows row.
Hung npx stub (exec /bin/sleep 600) + BROWSE_SKILLS_INSTALL_TIMEOUT_MS=2000 → browse skills install	exit 1 after 2s elapsed (timed): Skill install timed out after 2s waiting for \npx skills add`...`	Proves the deadline kills a hung child and surfaces the timeout failure (skill_install_timeout flows through the same fail plumbing verified in #2210). Also covered by spawnPassthrough unit tests (timeout + non-timeout control).
Hung catalog server (accepts, never responds) at default timeouts → browse skills add google.com/search-flights-ts4g1f with stubbed npx	exit 0 after 21s (10s API fetch abort + 10s Blob HEAD abort); npx stub invoked with --yes skills add browserbase/browse.sh --skill google.com/search-flights-ts4g1f	Proves a hung catalog aborts at the 10s default and the catalog-unavailable fallback semantics are preserved. Previously this hung forever. Also covered by a fast CLI-level test with BROWSE_SKILLS_FETCH_TIMEOUT_MS=500.
npx vitest run (packages/cli)	15 files, 224 tests passed (incl. 13 in skills-install.test.ts)	Full CLI suite green; supporting evidence only.
pnpm lint (packages/cli)	exit 0 (prettier + eslint + tsc)	Supporting evidence only.

Windows gap closed: a one-time windows-latest before/after run (actions/runs/27448084696, details in the validation comment) reproduced the exact 'C:\Program' is not recognized failure on main and verified browse skills install succeeds end-to-end on this PR's build with the default C:\Program Files\nodejs system Node. Full Windows vitest: 205/224 passed; all 17 failures are pre-existing POSIX test-harness assumptions (#!/bin/sh npx stubs etc.), identical by construction on main.

🤖 Generated with Claude Code

---

Summary by cubic

Fixes Windows failures in browse skills add by quoting the npx command when spawned via cmd.exe, bounds installer/fetch stages to prevent hangs, and runs the full CLI test suite on Windows. Addresses Linear: STG-2279.

• Bug Fixes
  • Quote command and args when spawning .cmd/.bat through the shell, so C:\Program Files\nodejs\npx.cmd and paths with spaces work.
  • Add a 180s deadline to npx skills add (SIGTERM, then SIGKILL) and a 10s abort for catalog/file fetches; both overridable via BROWSE_SKILLS_INSTALL_TIMEOUT_MS and BROWSE_SKILLS_FETCH_TIMEOUT_MS; install timeouts surface skill_install_timeout.
  • Run the full CLI suite on Windows (windows-latest); POSIX-only tests are now guarded via itPosix/describePosix so Windows gets full coverage without brittle CI filters.

^{Written for commit 2fa7df9. Summary will update on new commits.}

[changeset-bot]

🦋 Changeset detected

Latest commit: 2fa7df9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[cubic-dev-ai]

No issues found across 3 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

Architecture diagram

sequenceDiagram
    participant User as CLI User
    participant CLI as browse CLI
    participant Spawn as spawnPassthrough()
    participant Child as npx Child Process
    participant FS as Filesystem (npx.cmd/.sh)
    participant Catalog as Catalog API (HTTP)
    participant Blob as Blob Store (HTTP)

    Note over CLI,Blob: NEW: Spawn Quoting (Windows fix)

    User->>CLI: browse skills add <skill-id>

    CLI->>CLI: resolve npx via PATH+PATHEXT
    CLI->>FS: findExecutable("npx")
    FS-->>CLI: "C:\Program Files\nodejs\npx.cmd" (win32)

    alt win32 + .cmd/.bat shim
        CLI->>CLI: quoteForCmdShell(command) + quoteForCmdShell(args)
        Note over CLI: Wraps space-containing tokens in double-quotes
        CLI->>Spawn: spawnPassthrough(quotedCommand, quotedArgs, timeoutMs)
        Spawn->>Spawn: shell: true (required for .cmd)
        Spawn->>Spawn: set deathTimer = setTimeout(timeoutMs)
        Spawn->>Child: cmd.exe /d/s/c ""C:\Program Files\nodejs\npx.cmd" --yes skills add ..."
    else darwin/linux or no .cmd
        CLI->>Spawn: spawnPassthrough(command, args, timeoutMs)
        Spawn->>Spawn: set deathTimer = setTimeout(timeoutMs)
        Spawn->>Child: /usr/local/bin/npx --yes skills add ...
    end

    Note over Spawn,Child: NEW: 180s install deadline (env BROWSE_SKILLS_INSTALL_TIMEOUT_MS)

    alt Child completes normally before deadline
        Child-->>Spawn: exit code 0
        Spawn-->>CLI: { exitCode: 0, timedOut: false }
        CLI->>CLI: install succeeded
        CLI-->>User: exit 0 + success message
    else Child exits with error
        Child-->>Spawn: exit code != 0
        Spawn-->>CLI: { exitCode: N, timedOut: false }
        CLI->>CLI: fail with error + resultCode: "skill_install_failed"
        CLI-->>User: exit 1 + error
    else Child hangs past timeout
        Spawn->>Spawn: deathTimer fires
        Spawn->>Child: SIGTERM
        Spawn->>Spawn: set killTimer = setTimeout(5s, SIGKILL)
        Spawn-->>CLI: { exitCode: 1, timedOut: true }
        CLI->>CLI: fail with message + resultCode: "skill_install_timeout"
        CLI-->>User: exit 1 + timeout message
    end

    Note over CLI,Catalog: NEW: 10s catalog fetch deadline (env BROWSE_SKILLS_FETCH_TIMEOUT_MS)

    CLI->>Catalog: GET /skills/<id>/files (signal: AbortSignal.timeout(10s))
    alt Catalog responds before deadline
        Catalog-->>CLI: file list JSON
        CLI->>Blob: HEAD /skills/<id>/SKILL.md (signal: AbortSignal.timeout(10s))
        Blob-->>CLI: 200 OK
        CLI->>Blob: GET /skills/<id>/<file> (signal: AbortSignal.timeout(10s))
        Blob-->>CLI: file bytes
        CLI->>CLI: proceed with install flow
    else Catalog or Blob timeout
        Catalog--xCLI: AbortError (timeout)
        CLI->>CLI: treat as "unavailable" (same as network failure)
        Note over CLI: Falls back to direct GitHub installer
        CLI->>Child: npx skills add ... (direct install)
    end

Loading

_{Re-trigger cubic}

[shrey150]

Win32 validation (one-time windows-latest run)

Closes the Windows verification gap called out in the E2E matrix. A throwaway workflow (win32-skills-validation.yml, on a now-deleted throwaway branch — PR diff untouched) ran a before/after on a GitHub-hosted windows-latest runner, deliberately without actions/setup-node so the CLI resolves the system Node at the space-containing default install path — the exact bug trigger.

Run (green): https://github.com/browserbase/stagehand/actions/runs/27448084696

• Repro on main (expected failure): https://github.com/browserbase/stagehand/actions/runs/27448084696/job/81137436168
• Verify PR head (expected success): https://github.com/browserbase/stagehand/actions/runs/27448084696/job/81137436160
• First iteration (same before/after results, full-suite step details): https://github.com/browserbase/stagehand/actions/runs/27447860478

Runner layout (both jobs)

ImageOS=win25-vs2026 ImageVersion=20260608.135.2
node: v22.22.3  npm: 10.9.8
where.exe npx:
  C:\Program Files\nodejs\npx
  C:\Program Files\nodejs\npx.cmd

npx resolves under C:\Program Files\nodejs — the space-containing path from the telemetry cohort.

Before — origin/main: node packages/cli/bin/run.js skills install

'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
Could not install skill: 'C:\Program' is not recognized as an internal or external command,
operable program or batch file.

Exit code 1 — verbatim, the exact failure shape predicted in the PR description.

After — PR head d2e909899: same command, same runner layout

◇  Found 1 skill
●  Installing to all 71 agents
◇  Installed 1 skill
   ✓ ~\.agents\skills\browse
└  Done!

Exit code 0, and the job asserted the installed file exists: C:\Users\runneradmin\.agents\skills\browse\SKILL.md. (The one ✗ browse → PromptScript: PromptScript does not support global skill installation line is upstream npx skills behavior for that agent target, identical on every platform — not Windows-related.)

Windows vitest results (packages/cli, PR head)

Suite	Result	Notes
Win32-relevant gate: vitest run tests/skills-install.test.ts -t "quoteForCmdShell|spawnPassthrough|uses a shell for Windows command shims"	10 passed, 3 skipped	All quoting tests, shim detection, and both spawnPassthrough timeout tests (incl. "kills a hung child after the deadline", 522ms) pass natively on Windows — this also covers the install-timeout behavior, so no separate BROWSE_SKILLS_INSTALL_TIMEOUT_MS smoke was needed.
Full CLI suite: vitest run	205 passed, 17 failed, 2 skipped (224)	All 17 failures are pre-existing POSIX assumptions in the test harness, not product code: 9 skills/skills-install tests stub npx as an extension-less #!/bin/sh script (unresolvable via PATHEXT on Windows → "npx is not installed") or exec /bin/sleep; the other 8 (cli-functions-contract ×3, cli-templates ×3, driver-foundation ×1, package-manifest ×1) have similar host-tooling/path assumptions. Same-by-construction on main and this PR.

Net

Same runner, same Node layout, same command: main fails with the exact predicted 'C:\Program' is not recognized error; this PR installs the skill end-to-end. The Windows row in the E2E matrix is updated to point at this run.

[ajmcquilkin]

I have no idea if this works on Windows or not, I would want to see platform matrix E2E tests in CI to validate that this install works