diff --git a/.changeset/win32-skills-add-quoting.md b/.changeset/win32-skills-add-quoting.md new file mode 100644 index 000000000..f13fc52f5 --- /dev/null +++ b/.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. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c96697445..2cceede26 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/packages/cli/src/lib/skills/install.ts b/packages/cli/src/lib/skills/install.ts index 2eee173bd..31d074454 100644 --- a/packages/cli/src/lib/skills/install.ts +++ b/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 { - 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 { 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 { async function fetchFromUrl(url: URL, label: string): Promise { 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 { 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 { + const useWindowsShell = shouldUseWindowsShell(command); + const spawnCommand = useWindowsShell ? quoteForCmdShell(command) : command; + const spawnArgs = useWindowsShell ? args.map(quoteForCmdShell) : args; return await new Promise((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, }); }); }); diff --git a/packages/cli/tests/cli-functions-contract.test.ts b/packages/cli/tests/cli-functions-contract.test.ts index f2ee8e75c..4f211de90 100644 --- a/packages/cli/tests/cli-functions-contract.test.ts +++ b/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(); diff --git a/packages/cli/tests/cli-templates.test.ts b/packages/cli/tests/cli-templates.test.ts index 4278e7eb5..077f43ed5 100644 --- a/packages/cli/tests/cli-templates.test.ts +++ b/packages/cli/tests/cli-templates.test.ts @@ -10,6 +10,7 @@ import { type FakeBrowserbaseServer, } from "./helpers/fake-browserbase-server.js"; import { runCli } from "./helpers/run-cli.js"; +import { itPosix } from "./helpers/platform.js"; interface TemplateFixture { category: string[]; @@ -241,114 +242,122 @@ describe("templates commands", () => { }); }); - it("clones TypeScript templates with create-browser-app via npx", async () => { - const stubDir = await createTempDir("browse-templates-ts-stub-"); - const logPath = join(stubDir, "commands.log"); - await writeExecutable( - join(stubDir, "npx"), - [ - "#!/bin/sh", - 'if [ "$1" = "--version" ]; then', - " exit 0", - "fi", - 'printf \'npx %s\\n\' "$*" >> "$BB_STUB_LOG"', - 'project="$2"', - 'mkdir -p "$project"', - 'printf \'{"name":"stub-app","scripts":{"dev":"tsx index.ts"}}\\n\' > "$project/package.json"', - "printf 'BROWSERBASE_API_KEY=\\n' > \"$project/.env.example\"", - ].join("\n"), - ); - - await withTemplatesApi(async ({ baseUrl, requests }) => { - const cwd = await createTempDir("browse-templates-ts-project-"); - const dest = join(cwd, "my-scraper"); - const result = await runCli( - ["templates", "clone", "amazon-product-scraping", dest], - { - env: { - BB_STUB_LOG: logPath, - BROWSERBASE_TEMPLATES_API: baseUrl, - PATH: `${stubDir}:${process.env.PATH ?? ""}`, - }, - }, - ); - - expect(result.exitCode).toBe(0); - expect(requests.map((request) => request.path)).toEqual([ - "/amazon-product-scraping", - ]); - expect(await readFile(logPath, "utf8")).toContain( - "npx create-browser-app@latest my-scraper --template amazon-product-scraping", - ); - expect(result.stdout).toContain( - `Scaffolding typescript/amazon-product-scraping into ${dest}...`, - ); - expect(result.stdout).toContain(`Template scaffolded to ${dest}`); - expect(result.stdout).toContain(`cd ${dest}`); - expect(result.stdout).toContain("npm install"); - expect(result.stdout).toContain("cp .env.example .env"); - expect(result.stdout).toContain("npm run dev"); - expect(await readFile(join(dest, "package.json"), "utf8")).toContain( - "stub-app", + itPosix( + "clones TypeScript templates with create-browser-app via npx", + async () => { + const stubDir = await createTempDir("browse-templates-ts-stub-"); + const logPath = join(stubDir, "commands.log"); + await writeExecutable( + join(stubDir, "npx"), + [ + "#!/bin/sh", + 'if [ "$1" = "--version" ]; then', + " exit 0", + "fi", + 'printf \'npx %s\\n\' "$*" >> "$BB_STUB_LOG"', + 'project="$2"', + 'mkdir -p "$project"', + 'printf \'{"name":"stub-app","scripts":{"dev":"tsx index.ts"}}\\n\' > "$project/package.json"', + "printf 'BROWSERBASE_API_KEY=\\n' > \"$project/.env.example\"", + ].join("\n"), ); - }); - }); - it("clones Python templates with create-browser-app via uvx", async () => { - const stubDir = await createTempDir("browse-templates-py-stub-"); - const logPath = join(stubDir, "commands.log"); - await writeExecutable( - join(stubDir, "uvx"), - [ - "#!/bin/sh", - 'if [ "$1" = "--version" ]; then', - " exit 0", - "fi", - 'printf \'uvx %s\\n\' "$*" >> "$BB_STUB_LOG"', - 'project="$2"', - 'mkdir -p "$project"', - 'printf \'print("hello")\\n\' > "$project/main.py"', - "printf 'BROWSERBASE_API_KEY=\\n' > \"$project/.env.example\"", - 'printf \'[project]\\nname = "stub-py"\\nversion = "0.1.0"\\n\' > "$project/pyproject.toml"', - ].join("\n"), - ); - - await withTemplatesApi(async ({ baseUrl }) => { - const cwd = await createTempDir("browse-templates-py-project-"); - const dest = join(cwd, "py-scraper"); - const result = await runCli( - [ - "templates", - "clone", - "amazon-product-scraping", - "--language", - "python", - dest, - ], - { - env: { - BB_STUB_LOG: logPath, - BROWSERBASE_TEMPLATES_API: baseUrl, - PATH: `${stubDir}:${process.env.PATH ?? ""}`, + await withTemplatesApi(async ({ baseUrl, requests }) => { + const cwd = await createTempDir("browse-templates-ts-project-"); + const dest = join(cwd, "my-scraper"); + const result = await runCli( + ["templates", "clone", "amazon-product-scraping", dest], + { + env: { + BB_STUB_LOG: logPath, + BROWSERBASE_TEMPLATES_API: baseUrl, + PATH: `${stubDir}:${process.env.PATH ?? ""}`, + }, }, - }, + ); + + expect(result.exitCode).toBe(0); + expect(requests.map((request) => request.path)).toEqual([ + "/amazon-product-scraping", + ]); + expect(await readFile(logPath, "utf8")).toContain( + "npx create-browser-app@latest my-scraper --template amazon-product-scraping", + ); + expect(result.stdout).toContain( + `Scaffolding typescript/amazon-product-scraping into ${dest}...`, + ); + expect(result.stdout).toContain(`Template scaffolded to ${dest}`); + expect(result.stdout).toContain(`cd ${dest}`); + expect(result.stdout).toContain("npm install"); + expect(result.stdout).toContain("cp .env.example .env"); + expect(result.stdout).toContain("npm run dev"); + expect(await readFile(join(dest, "package.json"), "utf8")).toContain( + "stub-app", + ); + }); + }, + ); + + itPosix( + "clones Python templates with create-browser-app via uvx", + async () => { + const stubDir = await createTempDir("browse-templates-py-stub-"); + const logPath = join(stubDir, "commands.log"); + await writeExecutable( + join(stubDir, "uvx"), + [ + "#!/bin/sh", + 'if [ "$1" = "--version" ]; then', + " exit 0", + "fi", + 'printf \'uvx %s\\n\' "$*" >> "$BB_STUB_LOG"', + 'project="$2"', + 'mkdir -p "$project"', + 'printf \'print("hello")\\n\' > "$project/main.py"', + "printf 'BROWSERBASE_API_KEY=\\n' > \"$project/.env.example\"", + 'printf \'[project]\\nname = "stub-py"\\nversion = "0.1.0"\\n\' > "$project/pyproject.toml"', + ].join("\n"), ); - expect(result.exitCode).toBe(0); - expect(await readFile(logPath, "utf8")).toContain( - "uvx create-browser-app py-scraper --template amazon-product-scraping", - ); - expect(result.stdout).toContain( - `Scaffolding python/amazon-product-scraping into ${dest}...`, - ); - expect(result.stdout).toContain("uv sync"); - expect(result.stdout).toContain("cp .env.example .env"); - expect(result.stdout).toContain("python main.py"); - expect(await readFile(join(dest, "main.py"), "utf8")).toContain("hello"); - }); - }); + await withTemplatesApi(async ({ baseUrl }) => { + const cwd = await createTempDir("browse-templates-py-project-"); + const dest = join(cwd, "py-scraper"); + const result = await runCli( + [ + "templates", + "clone", + "amazon-product-scraping", + "--language", + "python", + dest, + ], + { + env: { + BB_STUB_LOG: logPath, + BROWSERBASE_TEMPLATES_API: baseUrl, + PATH: `${stubDir}:${process.env.PATH ?? ""}`, + }, + }, + ); + + expect(result.exitCode).toBe(0); + expect(await readFile(logPath, "utf8")).toContain( + "uvx create-browser-app py-scraper --template amazon-product-scraping", + ); + expect(result.stdout).toContain( + `Scaffolding python/amazon-product-scraping into ${dest}...`, + ); + expect(result.stdout).toContain("uv sync"); + expect(result.stdout).toContain("cp .env.example .env"); + expect(result.stdout).toContain("python main.py"); + expect(await readFile(join(dest, "main.py"), "utf8")).toContain( + "hello", + ); + }); + }, + ); - it("prints clone results as JSON", async () => { + itPosix("prints clone results as JSON", async () => { const stubDir = await createTempDir("browse-templates-json-stub-"); await writeExecutable( join(stubDir, "npx"), diff --git a/packages/cli/tests/driver-foundation.test.ts b/packages/cli/tests/driver-foundation.test.ts index cb08cf212..312e362d2 100644 --- a/packages/cli/tests/driver-foundation.test.ts +++ b/packages/cli/tests/driver-foundation.test.ts @@ -4,6 +4,7 @@ import net from "node:net"; import { tmpdir } from "node:os"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { itPosix } from "./helpers/platform.js"; import { resolveConnectionTarget, @@ -402,26 +403,29 @@ describe("driver foundation", () => { } }); - it("does not remove daemon files for an alive unresponsive daemon", async () => { - const daemonDir = await mkdtemp(join(tmpdir(), "browse-driver-test-")); - cleanupPaths.push(daemonDir); - const previousDaemonDir = process.env.BROWSE_DAEMON_DIR; - process.env.BROWSE_DAEMON_DIR = daemonDir; - const session = "alive-unresponsive"; - const pidPath = getPidPath(session); - const socketPath = getSocketPath(session); + itPosix( + "does not remove daemon files for an alive unresponsive daemon", + async () => { + const daemonDir = await mkdtemp(join(tmpdir(), "browse-driver-test-")); + cleanupPaths.push(daemonDir); + const previousDaemonDir = process.env.BROWSE_DAEMON_DIR; + process.env.BROWSE_DAEMON_DIR = daemonDir; + const session = "alive-unresponsive"; + const pidPath = getPidPath(session); + const socketPath = getSocketPath(session); - try { - await writeFile(pidPath, String(process.pid)); - await writeFile(socketPath, "not-a-socket"); + try { + await writeFile(pidPath, String(process.pid)); + await writeFile(socketPath, "not-a-socket"); - await expect(getDriverStatus(session)).resolves.toBeNull(); - await expect(access(pidPath)).resolves.toBeUndefined(); - await expect(access(socketPath)).resolves.toBeUndefined(); - } finally { - restoreEnv("BROWSE_DAEMON_DIR", previousDaemonDir); - } - }); + await expect(getDriverStatus(session)).resolves.toBeNull(); + await expect(access(pidPath)).resolves.toBeUndefined(); + await expect(access(socketPath)).resolves.toBeUndefined(); + } finally { + restoreEnv("BROWSE_DAEMON_DIR", previousDaemonDir); + } + }, + ); it("checks target compatibility after waiting for the daemon lock", async () => { const daemonDir = await mkdtemp(join(tmpdir(), "browse-driver-test-")); diff --git a/packages/cli/tests/helpers/platform.ts b/packages/cli/tests/helpers/platform.ts new file mode 100644 index 000000000..f20cbe335 --- /dev/null +++ b/packages/cli/tests/helpers/platform.ts @@ -0,0 +1,13 @@ +import { describe, it } from "vitest"; + +/** + * Helpers for tests that only run on POSIX. They depend on shell-script + * executable stubs (`#!/bin/sh`), `chmod`/uid file modes, or other POSIX-only + * behavior, so they skip on Windows until the harness writes `.cmd` stubs. + * + * The win32 CI leg (run-cli-tests-win32 in ci.yml) runs the full suite; these + * guards are how a test opts out of Windows. Coverage is on by default — a new + * test runs on Windows unless it explicitly uses these. + */ +export const itPosix = it.runIf(process.platform !== "win32"); +export const describePosix = describe.runIf(process.platform !== "win32"); diff --git a/packages/cli/tests/package-manifest.test.ts b/packages/cli/tests/package-manifest.test.ts index 168832f3c..b1e5cc7d1 100644 --- a/packages/cli/tests/package-manifest.test.ts +++ b/packages/cli/tests/package-manifest.test.ts @@ -4,13 +4,14 @@ import { dirname, resolve } from "node:path"; import { promisify } from "node:util"; import { fileURLToPath } from "node:url"; -import { describe, expect, it } from "vitest"; +import { describe, expect } from "vitest"; +import { itPosix } from "./helpers/platform.js"; const execFileAsync = promisify(execFile); const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); describe("package manifest", () => { - it("generates and packages the oclif manifest", async () => { + itPosix("generates and packages the oclif manifest", async () => { const [manifestJson, packageJson] = await Promise.all([ readFile(resolve(repoRoot, "oclif.manifest.json"), "utf8"), readFile(resolve(repoRoot, "package.json"), "utf8"), diff --git a/packages/cli/tests/skills-install.test.ts b/packages/cli/tests/skills-install.test.ts index 18c2ee4ed..c39fec2f9 100644 --- a/packages/cli/tests/skills-install.test.ts +++ b/packages/cli/tests/skills-install.test.ts @@ -1,15 +1,32 @@ import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { createServer, type Server } from "node:http"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { afterEach, describe, expect, it } from "vitest"; -import { shouldUseWindowsShell } from "../src/lib/skills/install.js"; +import { + quoteForCmdShell, + shouldUseWindowsShell, + spawnPassthrough, +} from "../src/lib/skills/install.js"; import { runCli } from "./helpers/run-cli.js"; +import { itPosix } from "./helpers/platform.js"; const cleanupPaths: string[] = []; +const cleanupServers: Server[] = []; afterEach(async () => { + while (cleanupServers.length > 0) { + const server = cleanupServers.pop(); + if (server) { + server.closeAllConnections(); + await new Promise((resolvePromise) => { + server.close(() => resolvePromise()); + }); + } + } + while (cleanupPaths.length > 0) { const path = cleanupPaths.pop(); if (path) await rm(path, { recursive: true, force: true }); @@ -17,7 +34,7 @@ afterEach(async () => { }); describe("skills install", () => { - it("installs the bundled browse CLI skill", async () => { + itPosix("installs the bundled browse CLI skill", async () => { const stubDir = await createTempDir("browse-skills-install-bin-"); const logPath = join(stubDir, "npx.log"); await writeNpxStub(stubDir); @@ -42,6 +59,140 @@ describe("skills install", () => { expect(shouldUseWindowsShell("/usr/local/bin/npx", "darwin")).toBe(false); expect(shouldUseWindowsShell("C:\\npm\\npx.exe", "win32")).toBe(false); }); + + itPosix( + "fails with a timeout message when the npx child hangs past the deadline", + async () => { + const stubDir = await createTempDir("browse-skills-timeout-bin-"); + await writeSleepingNpxStub(stubDir); + + const result = await runCli(["skills", "install"], { + env: { + PATH: stubDir, + BROWSE_SKILLS_INSTALL_TIMEOUT_MS: "1000", + }, + }); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("Skill install timed out after 1s"); + }, + ); + + itPosix( + "falls back to the GitHub installer when the catalog fetch hangs", + async () => { + const stubDir = await createTempDir("browse-skills-hang-bin-"); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir); + const { server, baseUrl } = await startHangingServer(); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "airline.example/book-flight-ab12cd"], + { + env: { + BB_STUB_LOG: logPath, + BROWSE_SKILLS_API_BASE_URL: baseUrl, + BROWSE_SKILLS_BLOB_BASE_URL: baseUrl, + BROWSE_SKILLS_FETCH_TIMEOUT_MS: "500", + PATH: stubDir, + }, + }, + ); + + expect(result.exitCode).toBe(0); + await expect(readFile(logPath, "utf8")).resolves.toContain( + "--yes skills add browserbase/browse.sh --skill airline.example/book-flight-ab12cd", + ); + }, + ); +}); + +describe("quoteForCmdShell", () => { + it("leaves plain tokens untouched", () => { + expect(quoteForCmdShell("npx")).toBe("npx"); + expect(quoteForCmdShell("--yes")).toBe("--yes"); + expect(quoteForCmdShell("browserbase/browse.sh")).toBe( + "browserbase/browse.sh", + ); + expect(quoteForCmdShell("C:\\nodejs\\npx.cmd")).toBe("C:\\nodejs\\npx.cmd"); + }); + + it("quotes the default Windows Node install path", () => { + expect(quoteForCmdShell("C:\\Program Files\\nodejs\\npx.cmd")).toBe( + '"C:\\Program Files\\nodejs\\npx.cmd"', + ); + }); + + it("quotes install paths with spaces", () => { + expect( + quoteForCmdShell("C:\\Users\\First Last\\.config\\browserbase\\skill"), + ).toBe('"C:\\Users\\First Last\\.config\\browserbase\\skill"'); + }); + + it("doubles embedded quotes", () => { + expect(quoteForCmdShell('say "hi" now')).toBe('"say ""hi"" now"'); + }); + + it("quotes cmd metacharacters", () => { + expect(quoteForCmdShell("a&b")).toBe('"a&b"'); + expect(quoteForCmdShell("a|b")).toBe('"a|b"'); + expect(quoteForCmdShell("a^b")).toBe('"a^b"'); + expect(quoteForCmdShell("ab")).toBe('"a>b"'); + }); + + it("quotes the empty token", () => { + expect(quoteForCmdShell("")).toBe('""'); + }); + + it("builds an intact cmd.exe command line for a default Windows install", () => { + const command = "C:\\Program Files\\nodejs\\npx.cmd"; + const args = [ + "--yes", + "skills", + "add", + "C:\\Users\\First Last\\.config\\browserbase\\skills\\x\\y", + ]; + + // Before the fix Node joined the tokens unquoted, so cmd.exe split the + // command at the space and executed `C:\Program` ("'C:\Program' is not + // recognized as an internal or external command"). + const unquoted = [command, ...args].join(" "); + expect(unquoted).toContain("C:\\Program Files\\"); + expect(unquoted.startsWith('"')).toBe(false); + + const quoted = [command, ...args].map(quoteForCmdShell).join(" "); + expect(quoted).toBe( + '"C:\\Program Files\\nodejs\\npx.cmd" --yes skills add "C:\\Users\\First Last\\.config\\browserbase\\skills\\x\\y"', + ); + }); +}); + +describe("spawnPassthrough", () => { + it("kills a hung child after the deadline and reports a timeout", async () => { + const start = Date.now(); + const result = await spawnPassthrough( + process.execPath, + ["-e", "setTimeout(() => {}, 600_000);"], + 500, + ); + + expect(result.timedOut).toBe(true); + expect(result.exitCode).toBe(1); + expect(Date.now() - start).toBeLessThan(10_000); + }); + + it("does not flag fast children as timed out", async () => { + const result = await spawnPassthrough( + process.execPath, + ["-e", "process.exit(0);"], + 30_000, + ); + + expect(result.timedOut).toBe(false); + expect(result.exitCode).toBe(0); + }); }); async function createTempDir(prefix: string): Promise { @@ -60,3 +211,37 @@ async function writeNpxStub(stubDir: string): Promise { ); await chmod(stubPath, 0o755); } + +async function writeSleepingNpxStub(stubDir: string): Promise { + const stubPath = join(stubDir, "npx"); + // PATH is stripped to the stub dir in these tests, so use an absolute path. + await writeFile( + stubPath, + ["#!/bin/sh", "exec /bin/sleep 600", ""].join("\n"), + ); + await chmod(stubPath, 0o755); +} + +// Accepts connections but never responds, so fetches hang until aborted. +async function startHangingServer(): Promise<{ + server: Server; + baseUrl: string; +}> { + const server = createServer(() => { + // Intentionally never write a response. + }); + + await new Promise((resolvePromise) => { + server.listen(0, "127.0.0.1", resolvePromise); + }); + + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("Could not start hanging server."); + } + + return { + server, + baseUrl: `http://127.0.0.1:${address.port}`, + }; +} diff --git a/packages/cli/tests/skills.test.ts b/packages/cli/tests/skills.test.ts index 103b06709..0229a51db 100644 --- a/packages/cli/tests/skills.test.ts +++ b/packages/cli/tests/skills.test.ts @@ -6,6 +6,7 @@ import { tmpdir } from "node:os"; import { afterEach, describe, expect, it } from "vitest"; import { runCli } from "./helpers/run-cli.js"; +import { itPosix } from "./helpers/platform.js"; const cleanupPaths: string[] = []; const cleanupServers: Server[] = []; @@ -298,213 +299,228 @@ describe("skills", () => { ); }); - it("fails cleanly when a non-generated skill is missing from the catalog", async () => { - const stubDir = await createTempDir("browse-skills-missing-bin-"); - const logPath = join(stubDir, "npx.log"); - await writeNpxStub(stubDir, logPath); - // Empty server: the file API returns 404 for the requested id. - const { server, baseUrl } = await startFakeSkillServer({}); - cleanupServers.push(server); - - const result = await runCli( - ["skills", "add", "amazon.com/buy-something-fake"], - { - env: { - BROWSE_SKILLS_API_BASE_URL: baseUrl, - PATH: stubDir, + itPosix( + "fails cleanly when a non-generated skill is missing from the catalog", + async () => { + const stubDir = await createTempDir("browse-skills-missing-bin-"); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir, logPath); + // Empty server: the file API returns 404 for the requested id. + const { server, baseUrl } = await startFakeSkillServer({}); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "amazon.com/buy-something-fake"], + { + env: { + BROWSE_SKILLS_API_BASE_URL: baseUrl, + PATH: stubDir, + }, }, - }, - ); - - expect(result.exitCode).toBe(1); - expect(result.stderr).toContain( - 'Skill "amazon.com/buy-something-fake" not found in the catalog', - ); - expect(result.stderr).toContain("browse skills find amazon.com"); - // It must NOT have shelled out to clone the browse.sh repo. - await expect( - readFile(logPath, "utf8").catch(() => ""), - ).resolves.not.toContain("browserbase/browse.sh"); - }); - - it("installs suffix-shaped catalog skills from GitHub when the file API returns 404", async () => { - const stubDir = await createTempDir("browse-skills-suffix-catalog-bin-"); - const logPath = join(stubDir, "npx.log"); - await writeNpxStub(stubDir, logPath); - const { server, baseUrl } = await startFakeSkillServer({}); - cleanupServers.push(server); + ); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain( + 'Skill "amazon.com/buy-something-fake" not found in the catalog', + ); + expect(result.stderr).toContain("browse skills find amazon.com"); + // It must NOT have shelled out to clone the browse.sh repo. + await expect( + readFile(logPath, "utf8").catch(() => ""), + ).resolves.not.toContain("browserbase/browse.sh"); + }, + ); - const result = await runCli( - ["skills", "add", "airline.example/book-flight"], - { - env: { - BROWSE_SKILLS_API_BASE_URL: baseUrl, - PATH: stubDir, + itPosix( + "installs suffix-shaped catalog skills from GitHub when the file API returns 404", + async () => { + const stubDir = await createTempDir("browse-skills-suffix-catalog-bin-"); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir, logPath); + const { server, baseUrl } = await startFakeSkillServer({}); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "airline.example/book-flight"], + { + env: { + BROWSE_SKILLS_API_BASE_URL: baseUrl, + PATH: stubDir, + }, }, - }, - ); - - expect(result.exitCode).toBe(0); - expect(result.stdout).not.toContain("Downloaded"); - await expect(readFile(logPath, "utf8")).resolves.toContain( - "--yes skills add browserbase/browse.sh --skill airline.example/book-flight", - ); - }); - - it("installs suffix-shaped catalog skills from GitHub when the file API is unavailable and no Blob fallback exists", async () => { - const stubDir = await createTempDir( - "browse-skills-suffix-unavailable-bin-", - ); - const logPath = join(stubDir, "npx.log"); - await writeNpxStub(stubDir, logPath); - const { server, baseUrl } = await startFakeSkillServer({ - "/api/skills/airline.example/book-flight/files": { - status: 500, - body: "server error", - }, - }); - cleanupServers.push(server); + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain("Downloaded"); + await expect(readFile(logPath, "utf8")).resolves.toContain( + "--yes skills add browserbase/browse.sh --skill airline.example/book-flight", + ); + }, + ); - const result = await runCli( - ["skills", "add", "airline.example/book-flight"], - { - env: { - BROWSE_SKILLS_API_BASE_URL: baseUrl, - BROWSE_SKILLS_BLOB_BASE_URL: baseUrl, - PATH: stubDir, + itPosix( + "installs suffix-shaped catalog skills from GitHub when the file API is unavailable and no Blob fallback exists", + async () => { + const stubDir = await createTempDir( + "browse-skills-suffix-unavailable-bin-", + ); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir, logPath); + const { server, baseUrl } = await startFakeSkillServer({ + "/api/skills/airline.example/book-flight/files": { + status: 500, + body: "server error", }, - }, - ); - - expect(result.exitCode).toBe(0); - expect(result.stdout).not.toContain("Downloaded"); - await expect(readFile(logPath, "utf8")).resolves.toContain( - "--yes skills add browserbase/browse.sh --skill airline.example/book-flight", - ); - }); - - it("downloads generated skills from the Browse.sh file API before installing", async () => { - const stubDir = await createTempDir("browse-skills-api-bin-"); - const configHome = await createTempDir("browse-skills-config-"); - const logPath = join(stubDir, "npx.log"); - await writeNpxStub(stubDir, logPath); - - const { server, baseUrl } = await startFakeSkillServer({ - "/api/skills/mcdonalds.order.online/order-delivery-42q71n/files": ( - origin, - ) => - JSON.stringify({ - skillId: "mcdonalds.order.online/order-delivery-42q71n", - files: [ - { - path: "SKILL.md", - url: `${origin}/downloads/order-delivery/SKILL.md`, - }, - { - path: "REFERENCE.md", - url: `${origin}/downloads/order-delivery/REFERENCE.md`, - }, - ], - }), - "/downloads/order-delivery/SKILL.md": [ - "---", - "name: order-delivery", - "description: Place a McDonald's delivery order.", - "---", - "", - "# Order delivery", - "", - ].join("\n"), - "/downloads/order-delivery/REFERENCE.md": "Reference\n", - }); - cleanupServers.push(server); - - const result = await runCli( - ["skills", "add", "mcdonalds.order.online/order-delivery-42q71n"], - { - env: { - BROWSE_SKILLS_API_BASE_URL: baseUrl, - PATH: stubDir, - XDG_CONFIG_HOME: configHome, + }); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "airline.example/book-flight"], + { + env: { + BROWSE_SKILLS_API_BASE_URL: baseUrl, + BROWSE_SKILLS_BLOB_BASE_URL: baseUrl, + PATH: stubDir, + }, }, - }, - ); - - expect(result.exitCode).toBe(0); - expect(result.stdout).toContain("Downloaded 2 skill files"); - - const installPath = join( - configHome, - "browserbase", - "skills", - "mcdonalds.order.online", - "order-delivery-42q71n", - ); - await expect( - readFile(join(installPath, "SKILL.md"), "utf8"), - ).resolves.toContain("name: order-delivery"); - await expect( - readFile(join(installPath, "REFERENCE.md"), "utf8"), - ).resolves.toBe("Reference\n"); - await expect(readFile(logPath, "utf8")).resolves.toContain( - `--yes skills add ${installPath}`, - ); - }); - - it("falls back to direct SKILL.md download when the file API is unavailable for a suffix-shaped skill", async () => { - const stubDir = await createTempDir("browse-skills-api-fallback-bin-"); - const configHome = await createTempDir( - "browse-skills-api-fallback-config-", - ); - const logPath = join(stubDir, "npx.log"); - await writeNpxStub(stubDir, logPath); - - const { server, baseUrl } = await startFakeSkillServer({ - "/api/skills/mcdonalds.order.online/order-delivery-42q71n/files": { - status: 500, - body: "server error", - }, - "/skills/mcdonalds.order.online/order-delivery-42q71n/SKILL.md": [ - "---", - "name: order-delivery", - "description: Place a McDonald's delivery order.", - "---", - "", - "# Order delivery", - "", - ].join("\n"), - }); - cleanupServers.push(server); + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain("Downloaded"); + await expect(readFile(logPath, "utf8")).resolves.toContain( + "--yes skills add browserbase/browse.sh --skill airline.example/book-flight", + ); + }, + ); - const result = await runCli( - ["skills", "add", "mcdonalds.order.online/order-delivery-42q71n"], - { - env: { - BROWSE_SKILLS_API_BASE_URL: baseUrl, - BROWSE_SKILLS_BLOB_BASE_URL: baseUrl, - PATH: stubDir, - XDG_CONFIG_HOME: configHome, + itPosix( + "downloads generated skills from the Browse.sh file API before installing", + async () => { + const stubDir = await createTempDir("browse-skills-api-bin-"); + const configHome = await createTempDir("browse-skills-config-"); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir, logPath); + + const { server, baseUrl } = await startFakeSkillServer({ + "/api/skills/mcdonalds.order.online/order-delivery-42q71n/files": ( + origin, + ) => + JSON.stringify({ + skillId: "mcdonalds.order.online/order-delivery-42q71n", + files: [ + { + path: "SKILL.md", + url: `${origin}/downloads/order-delivery/SKILL.md`, + }, + { + path: "REFERENCE.md", + url: `${origin}/downloads/order-delivery/REFERENCE.md`, + }, + ], + }), + "/downloads/order-delivery/SKILL.md": [ + "---", + "name: order-delivery", + "description: Place a McDonald's delivery order.", + "---", + "", + "# Order delivery", + "", + ].join("\n"), + "/downloads/order-delivery/REFERENCE.md": "Reference\n", + }); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "mcdonalds.order.online/order-delivery-42q71n"], + { + env: { + BROWSE_SKILLS_API_BASE_URL: baseUrl, + PATH: stubDir, + XDG_CONFIG_HOME: configHome, + }, }, - }, - ); + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("Downloaded 2 skill files"); + + const installPath = join( + configHome, + "browserbase", + "skills", + "mcdonalds.order.online", + "order-delivery-42q71n", + ); + await expect( + readFile(join(installPath, "SKILL.md"), "utf8"), + ).resolves.toContain("name: order-delivery"); + await expect( + readFile(join(installPath, "REFERENCE.md"), "utf8"), + ).resolves.toBe("Reference\n"); + await expect(readFile(logPath, "utf8")).resolves.toContain( + `--yes skills add ${installPath}`, + ); + }, + ); - expect(result.exitCode).toBe(0); - expect(result.stdout).toContain("Downloaded 1 skill file"); - - const installPath = join( - configHome, - "browserbase", - "skills", - "mcdonalds.order.online", - "order-delivery-42q71n", - ); - await expect( - readFile(join(installPath, "SKILL.md"), "utf8"), - ).resolves.toContain("name: order-delivery"); - await expect(readFile(logPath, "utf8")).resolves.toContain( - `--yes skills add ${installPath}`, - ); - }); + itPosix( + "falls back to direct SKILL.md download when the file API is unavailable for a suffix-shaped skill", + async () => { + const stubDir = await createTempDir("browse-skills-api-fallback-bin-"); + const configHome = await createTempDir( + "browse-skills-api-fallback-config-", + ); + const logPath = join(stubDir, "npx.log"); + await writeNpxStub(stubDir, logPath); + + const { server, baseUrl } = await startFakeSkillServer({ + "/api/skills/mcdonalds.order.online/order-delivery-42q71n/files": { + status: 500, + body: "server error", + }, + "/skills/mcdonalds.order.online/order-delivery-42q71n/SKILL.md": [ + "---", + "name: order-delivery", + "description: Place a McDonald's delivery order.", + "---", + "", + "# Order delivery", + "", + ].join("\n"), + }); + cleanupServers.push(server); + + const result = await runCli( + ["skills", "add", "mcdonalds.order.online/order-delivery-42q71n"], + { + env: { + BROWSE_SKILLS_API_BASE_URL: baseUrl, + BROWSE_SKILLS_BLOB_BASE_URL: baseUrl, + PATH: stubDir, + XDG_CONFIG_HOME: configHome, + }, + }, + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("Downloaded 1 skill file"); + + const installPath = join( + configHome, + "browserbase", + "skills", + "mcdonalds.order.online", + "order-delivery-42q71n", + ); + await expect( + readFile(join(installPath, "SKILL.md"), "utf8"), + ).resolves.toContain("name: order-delivery"); + await expect(readFile(logPath, "utf8")).resolves.toContain( + `--yes skills add ${installPath}`, + ); + }, + ); it("rejects invalid skill ids", async () => { const result = await runCli(["skills", "add", "../bad"]); @@ -522,7 +538,7 @@ describe("skills", () => { expect(result.stderr).toContain("browse skills find"); }); - it("rejects unsafe API file paths", async () => { + itPosix("rejects unsafe API file paths", async () => { const stubDir = await createTempDir("browse-skills-unsafe-bin-"); const configHome = await createTempDir("browse-skills-unsafe-config-"); const logPath = join(stubDir, "npx.log");