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

.changeset/win32-skills-add-quoting.md

@@ -0,0 +1,9 @@
+---
+"browse": patch
+---
+
+Fix `browse skills add` on Windows and bound the unbounded installer stages.
+
+- Quote the `npx` command and arguments when spawning through cmd.exe (`shell: true` for `.cmd`/`.bat` shims), so the default `C:\Program Files\nodejs\npx.cmd` path and install paths with spaces (e.g. `C:\Users\First Last\...`) no longer split at the space and fail with "'C:\Program' is not recognized".
+- Kill the `npx skills add` child after a 180s deadline (SIGTERM, then SIGKILL) and fail with a clear message and a distinct `skill_install_timeout` telemetry result code instead of hanging forever.
+- Bound the catalog and skill-file fetches with a 10s abort timeout, preserving the existing catalog-unavailable fallback semantics when a fetch hangs.

.github/workflows/ci.yml

@@ -319,10 +319,14 @@ jobs:
           retention-days: 1
 
   run-cli-tests:
-    name: CLI Tests
-    runs-on: ubuntu-latest
+    name: CLI Tests (${{ matrix.os }})
+    runs-on: ${{ matrix.os }}
     needs: [run-build, determine-changes]
     if: needs.determine-changes.outputs.cli == 'true'
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-latest, windows-latest]
     steps:
       - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1
         with:
@@ -333,6 +337,10 @@ jobs:
           use-prebuilt-artifacts: "true"
           restore-turbo-cache: "false"
 
+      # Runs the full CLI suite on each OS. POSIX-only tests opt out at the test
+      # site via the itPosix/describePosix helpers (tests/helpers/platform.ts),
+      # so coverage is on by default — a new test runs on every OS unless it
+      # explicitly skips. No test-name knowledge lives in this workflow.
       - name: Run CLI Tests
         run: pnpm exec turbo run test:cli --filter=browse
 

packages/cli/src/lib/skills/install.ts

@@ -63,6 +63,26 @@ type SkillFilesResult =
     };
 
 const maxCapturedOutputBytes = 2048;
+const defaultInstallTimeoutMs = 180_000;
+const defaultFetchTimeoutMs = 10_000;
+
+function envTimeoutMs(name: string, fallback: number): number {
+  const value = Number(process.env[name]);
+  return Number.isFinite(value) && value > 0 ? value : fallback;
+}
+
+function installTimeoutMs(): number {
+  return envTimeoutMs(
+    "BROWSE_SKILLS_INSTALL_TIMEOUT_MS",
+    defaultInstallTimeoutMs,
+  );
+}
+
+function fetchTimeoutSignal(): AbortSignal {
+  return AbortSignal.timeout(
+    envTimeoutMs("BROWSE_SKILLS_FETCH_TIMEOUT_MS", defaultFetchTimeoutMs),
+  );
+}
 
 export function parseSkillId(rawSkillId: string): ParsedSkillId {
   const parts = rawSkillId.split("/");
@@ -179,11 +199,20 @@ async function runSkillsInstall(
   npxPath: string,
   args: string[],
 ): Promise<void> {
-  const result = await spawnPassthrough(npxPath, args);
-  if (result.exitCode === 0) {
+  const timeoutMs = installTimeoutMs();
+  const result = await spawnPassthrough(npxPath, args, timeoutMs);
+  if (result.exitCode === 0 && !result.timedOut) {
     return;
   }
 
+  if (result.timedOut) {
+    fail(
+      `Skill install timed out after ${Math.round(timeoutMs / 1000)}s waiting for \`npx skills add\`. Check your network connection and rerun \`browse skills add\`.`,
+      1,
+      { resultCode: "skill_install_timeout" },
+    );
+  }
+
   const detail = result.output.trim();
   const reason = detail
     ? `: ${detail}`
@@ -295,8 +324,10 @@ async function fetchSkillFilesFromApi(
   const url = skillFilesApiUrl(skillId);
   let response: Response;
   try {
-    response = await fetch(url);
+    response = await fetch(url, { signal: fetchTimeoutSignal() });
   } catch {
+    // Network failures and timeouts both mean the catalog is unavailable;
+    // callers fall back (or fail cleanly) exactly as before.
     return { status: "unavailable" };
   }
 
@@ -334,6 +365,7 @@ async function directBlobSkillExists(skillId: ParsedSkillId): Promise<boolean> {
   try {
     response = await fetch(skillBlobUrl(skillId, "SKILL.md"), {
       method: "HEAD",
+      signal: fetchTimeoutSignal(),
     });
   } catch {
     return false;
@@ -451,7 +483,7 @@ async function fetchSkillFile(url: URL, label: string): Promise<Uint8Array> {
 async function fetchFromUrl(url: URL, label: string): Promise<Response> {
   let response: Response;
   try {
-    response = await fetch(url);
+    response = await fetch(url, { signal: fetchTimeoutSignal() });
   } catch (error) {
     fail(`Could not download ${label}: ${(error as Error).message}`);
   }
@@ -521,21 +553,53 @@ async function findExecutable(command: string): Promise<string | null> {
 interface SpawnPassthroughResult {
   exitCode: number;
   output: string;
+  timedOut: boolean;
+}
+
+// Quotes a single token for the command line Node hands to cmd.exe when
+// `shell: true`. Node joins command+args UNQUOTED into `cmd.exe /d /s /c
+// "..."`, so an unquoted `C:\Program Files\nodejs\npx.cmd` splits at the
+// space and cmd runs `C:\Program`. Wrapping tokens that contain whitespace,
+// quotes, or cmd metacharacters in double quotes (with embedded quotes
+// doubled) keeps them intact; `/s` makes cmd strip only the outer quotes.
+export function quoteForCmdShell(token: string): string {
+  if (token === "") {
+    return '""';
+  }
+  return /[\s"^&|<>]/.test(token) ? `"${token.replaceAll('"', '""')}"` : token;
 }
 
 // Like `stdio: "inherit"` for the human watching, but the child's stdout/stderr
 // are also buffered (tail only) so a nonzero exit can surface a real reason to
-// telemetry instead of a bare exit code.
-async function spawnPassthrough(
+// telemetry instead of a bare exit code. The child is killed after `timeoutMs`
+// (SIGTERM, then SIGKILL) so a hung `npx` cannot stall the install forever.
+// Exported for tests.
+export async function spawnPassthrough(
   command: string,
   args: string[],
+  timeoutMs = installTimeoutMs(),
 ): Promise<SpawnPassthroughResult> {
+  const useWindowsShell = shouldUseWindowsShell(command);
+  const spawnCommand = useWindowsShell ? quoteForCmdShell(command) : command;
+  const spawnArgs = useWindowsShell ? args.map(quoteForCmdShell) : args;
   return await new Promise<SpawnPassthroughResult>((resolvePromise) => {
-    const child = spawn(command, args, {
+    const child = spawn(spawnCommand, spawnArgs, {
       stdio: ["inherit", "pipe", "pipe"],
-      shell: shouldUseWindowsShell(command),
+      shell: useWindowsShell,
     });
 
+    let timedOut = false;
+    let killTimer: NodeJS.Timeout | undefined;
+    const deadline = setTimeout(() => {
+      timedOut = true;
+      child.kill("SIGTERM");
+      killTimer = setTimeout(() => child.kill("SIGKILL"), 5_000);
+    }, timeoutMs);
+    const clearTimers = (): void => {
+      clearTimeout(deadline);
+      clearTimeout(killTimer);
+    };
+
     let captured = "";
     const capture = (chunk: Buffer): void => {
       captured += chunk.toString();
@@ -557,16 +621,20 @@ async function spawnPassthrough(
     // `skill_install_failed` by runSkillsInstall instead of escaping as an
     // unclassified runtime error.
     child.on("error", (error) => {
+      clearTimers();
       const message = error instanceof Error ? error.message : String(error);
       resolvePromise({
         exitCode: 1,
         output: captured ? `${captured}\n${message}` : message,
+        timedOut,
       });
     });
     child.on("close", (exitCode, signal) => {
+      clearTimers();
       resolvePromise({
         exitCode: signal ? 1 : (exitCode ?? 0),
         output: captured,
+        timedOut,
       });
     });
   });

packages/cli/tests/cli-functions-contract.test.ts

@@ -13,6 +13,7 @@ import { tmpdir } from "node:os";
 import { fileURLToPath } from "node:url";
 
 import { afterEach, describe, expect, it } from "vitest";
+import { itPosix } from "./helpers/platform.js";
 
 import {
   jsonResponse,
@@ -44,7 +45,7 @@ afterEach(async () => {
 });
 
 describe("functions API contracts", () => {
-  it("publishes a Functions archive and polls build status", async () => {
+  itPosix("publishes a Functions archive and polls build status", async () => {
     const cwd = await createFunctionFixture("functions-publish-");
 
     await withServer(
@@ -151,7 +152,7 @@ describe("functions API contracts", () => {
     );
   });
 
-  it("exits nonzero when a build fails", async () => {
+  itPosix("exits nonzero when a build fails", async () => {
     const cwd = await createFunctionFixture("functions-publish-fail-");
 
     await withServer(
@@ -303,7 +304,7 @@ describe("functions API contracts", () => {
 });
 
 describe("functions scaffolding and local dev", () => {
-  it("scaffolds a Functions project", async () => {
+  itPosix("scaffolds a Functions project", async () => {
     const cwd = await createTempDir("functions-init-");
     const fakeBin = await createFakePackageManagerBin();