Fix nested pnpm spawns on Windows

.gitattributes

@@ -0,0 +1,7 @@
+# Every checkout gets LF, on every platform.
+# Mantainers are mostly on macOS/Linux, where this is the default but on Windows
+# `core.autocrlf=true` is the default, and a CRLF working copy breaks anything comparing
+# file bytes: the release-manifest golden test hashes scripts/release/testdata fixtures, and
+# `types:generate --check` diffs each committed worker-configuration.d.ts against freshly generated
+# output.
+* text=auto eol=lf

AGENTS.md

@@ -69,7 +69,7 @@ To test changes:
 - `build:app:dev` is the same build with `minify: false`, run by the `pnpm dev-server` pre-flight so its `app.txt` matches what the watcher's un-skippable initial build will write — otherwise `emitAppText` rewrites the file and Wrangler restarts the worker mid-startup. It captures only `app.txt`, since `dist-app/` has no reader outside `vite.app.config.ts`. `build` and `deploy` still use `build:app`, so nothing unminified ships, and `build-app.mjs` always sets `GATEKEEPER_APP_UNMINIFIED` explicitly — an inherited value would otherwise make a production build unminified and get it cached that way.
 
   Two structural constraints explain the file layout. Vite+ reads per-package settings only from `vite.config.*`, which the SPA's own build config occupied, so that moved to `vite.app.config.ts` (referenced by `build-app.mjs -c`, `tsconfig.vite.json` and gatekeeper-context's `__tests__/vite-config.test.ts`). And a task may not share a name with a package.json script, so the `build:app` script is gone and `build` calls `vp run --cache build:app` instead, `deploy` the same with `--no-cache`. Don't define the task in the workspace-root config: it gets created for *every* package, including the root, which then fails.
-- The five packages whose tests run in workerd (`router`, `typed-storage`, `backend-utils`, `workshop-backend`, `gatekeeper-scheduler`) load `test-setup/assert-workerd.ts` as a `setupFiles` entry. It throws unless `navigator.userAgent` is `Cloudflare-Workers`, so a `@cloudflare/vitest-pool-workers` pool that fails to start fails the suite instead of silently falling back to Node — which otherwise looks like a pass in the packages that import no `cloudflare:*` module. Don't remove it to make a suite green.
+- The five packages whose tests run in workerd (`router`, `typed-storage`, `backend-utils`, `workshop-backend`, `gatekeeper-scheduler`) load `scripts/assert-workerd.ts` as a `setupFiles` entry. It throws unless `navigator.userAgent` is `Cloudflare-Workers`, so a `@cloudflare/vitest-pool-workers` pool that fails to start fails the suite instead of silently falling back to Node — which otherwise looks like a pass in the packages that import no `cloudflare:*` module. Don't remove it to make a suite green.
 
 Linting (oxlint, via Vite+):
 - `pnpm lint` runs what CI enforces: `lint:check` (oxlint), `types:scripts` and `types:check`. Run this before pushing.

packages/backend-utils/vitest.config.ts

@@ -52,6 +52,6 @@ export default defineConfig({
     include: ["__tests__/*.test.ts"],
     // Asserts the pool actually started; only one file here imports `cloudflare:workers`, so the
     // rest would pass under a Node fallback without noticing.
-    setupFiles: ["../../test-setup/assert-workerd.ts"],
+    setupFiles: ["../../scripts/assert-workerd.ts"],
   },
 });

packages/gatekeeper-context/build-app.mjs

@@ -3,6 +3,8 @@
 import { execFileSync } from "node:child_process";
 import { resolve } from "node:path";
 import { fileURLToPath } from "node:url";
+import { resolveBinEntry } from "../../scripts/bin-entry.ts";
+import { pnpmCommand } from "../../scripts/pnpm-command.ts";
 
 const pkgDir = resolve(fileURLToPath(import.meta.url), "..");
 const watch = process.argv.includes("--watch");
@@ -15,9 +17,16 @@ console.log(
     ? "watching context library app for changes…"
     : "building context library app single-file bundle…",
 );
+// Reached directly: Vite+ runs tasks with a filtered environment that drops `npm_execpath`, so on
+// Windows there is no shell-free way back to pnpm. Falls back to `pnpm exec` if vite is missing.
+const viteArgs = ["build", "-c", "vite.app.config.ts", ...(watch ? ["--watch"] : [])];
+const viteEntry = resolveBinEntry(pkgDir, "vite");
+const [command, argv] = viteEntry
+  ? [process.execPath, [viteEntry, ...viteArgs]]
+  : pnpmCommand(["exec", "vite", ...viteArgs]);
 execFileSync(
-  "pnpm",
-  ["exec", "vite", "build", "-c", "vite.app.config.ts", ...(watch ? ["--watch"] : [])],
+  command,
+  argv,
   {
     cwd: pkgDir,
     stdio: "inherit",

packages/gatekeeper-scheduler/build-app.mjs

@@ -1,16 +1,25 @@
 import { execFileSync } from "node:child_process";
 import { resolve } from "node:path";
 import { fileURLToPath } from "node:url";
+import { resolveBinEntry } from "../../scripts/bin-entry.ts";
+import { pnpmCommand } from "../../scripts/pnpm-command.ts";
 
 const packageDirectory = resolve(fileURLToPath(import.meta.url), "..");
 const watch = process.argv.includes("--watch");
 // One-shot build that produces the same bytes `--watch` would, for the `pnpm dev-server`
 // pre-flight see the `unminified` note in vite.app.config.ts.
 const dev = process.argv.includes("--dev");
 
+// Reached directly: Vite+ runs tasks with a filtered environment that drops `npm_execpath`, so on
+// Windows there is no shell-free way back to pnpm. Falls back to `pnpm exec` if vite is missing.
+const viteArgs = ["build", "-c", "vite.app.config.ts", ...(watch ? ["--watch"] : [])];
+const viteEntry = resolveBinEntry(packageDirectory, "vite");
+const [command, argv] = viteEntry
+  ? [process.execPath, [viteEntry, ...viteArgs]]
+  : pnpmCommand(["exec", "vite", ...viteArgs]);
 execFileSync(
-  "pnpm",
-  ["exec", "vite", "build", "-c", "vite.app.config.ts", ...(watch ? ["--watch"] : [])],
+  command,
+  argv,
   {
     cwd: packageDirectory,
     stdio: "inherit",

packages/gatekeeper-scheduler/vitest.config.ts

@@ -31,6 +31,6 @@ export default defineConfig({
   test: {
     include: ["__tests__/*.test.ts"],
     // Asserts the pool actually started, rather than trusting a green run to mean workerd.
-    setupFiles: ["../../test-setup/assert-workerd.ts"],
+    setupFiles: ["../../scripts/assert-workerd.ts"],
   },
 });

packages/router/vitest.config.ts

@@ -19,6 +19,6 @@ export default defineConfig({
     include: ['__tests__/*.test.ts'],
     // Nothing here imports `cloudflare:test`, so a pool that failed to start would leave this suite
     // green while running under Node. The guard makes that fail loudly instead.
-    setupFiles: ['../../test-setup/assert-workerd.ts'],
+    setupFiles: ['../../scripts/assert-workerd.ts'],
   },
 })

packages/typed-storage/vitest.config.ts

@@ -19,6 +19,6 @@ export default defineConfig({
     include: ['__tests__/*.test.ts'],
     // Nothing here imports `cloudflare:test`, so a pool that failed to start would leave this suite
     // green while running under Node. The guard makes that fail loudly instead.
-    setupFiles: ['../../test-setup/assert-workerd.ts'],
+    setupFiles: ['../../scripts/assert-workerd.ts'],
   },
 })

packages/workshop-backend/vitest.config.ts

@@ -25,6 +25,6 @@ export default defineConfig({
   test: {
     include: ['__tests__/*.test.ts'],
     // Asserts the pool actually started, rather than trusting a green run to mean workerd.
-    setupFiles: ['../../test-setup/assert-workerd.ts'],
+    setupFiles: ['../../scripts/assert-workerd.ts'],
   },
 })

packages/workshop-backend/vitest.integration.config.ts

@@ -24,7 +24,7 @@ export default defineConfig({
   test: {
     include: ["__integration__/*.test.ts"],
     // Asserts the pool actually started, rather than trusting a green run to mean workerd.
-    setupFiles: ["../../test-setup/assert-workerd.ts"],
+    setupFiles: ["../../scripts/assert-workerd.ts"],
     // Whichever test runs first pays for workerd booting and instantiating the whole backend
     // bundle -- ~6s on a dev machine and roughly 3x that on a CI runner, while every subsequent
     // test in the file finishes in tens of milliseconds. The timeout has to clear that cold

scripts/bin-entry.test.ts

@@ -0,0 +1,65 @@
+import assert from "node:assert/strict";
+import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { dirname, join } from "node:path";
+import { after, describe, it } from "node:test";
+import { resolveBinEntry } from "./bin-entry.ts";
+
+// A fresh temp directory per run: `node --test` runs files in parallel, and anything written inside
+// the repo would land in `run-local`'s source hash. Torn down even when a test fails. Realpathed
+// because resolveBinEntry is, and on macOS `tmpdir()` is a symlink (/var -> /private/var).
+const root = realpathSync(mkdtempSync(join(tmpdir(), "bin-entry-")));
+after(() => rmSync(root, { recursive: true, force: true }));
+
+let caseCount = 0;
+
+// A package directory holding `node_modules/<name>` with the given manifest, plus each named entry
+// file. `entries` is separate from `bin` so a manifest can point at a file that is not there.
+function pkgWith(name: string, bin: unknown, entries: string[]): string {
+  const pkgDir = join(root, `case-${++caseCount}`);
+  const installed = join(pkgDir, "node_modules", name);
+  mkdirSync(installed, { recursive: true });
+  writeFileSync(join(installed, "package.json"), JSON.stringify({ name, bin }));
+  for (const entry of entries) {
+    const file = join(installed, entry);
+    mkdirSync(dirname(file), { recursive: true });
+    writeFileSync(file, "");
+  }
+  return pkgDir;
+}
+
+describe("resolveBinEntry", () => {
+  it("resolves a string `bin` to an absolute entry path", () => {
+    const pkgDir = pkgWith("vite", "bin/vite.js", ["bin/vite.js"]);
+    assert.equal(
+        resolveBinEntry(pkgDir, "vite"),
+        join(pkgDir, "node_modules", "vite", "bin", "vite.js"));
+  });
+
+  it("resolves the matching key of an object `bin`", () => {
+    const pkgDir = pkgWith("wrangler", { wrangler: "./main.js", other: "./other.js" }, ["main.js"]);
+    assert.equal(
+        resolveBinEntry(pkgDir, "wrangler"),
+        join(pkgDir, "node_modules", "wrangler", "main.js"));
+  });
+
+  // The case that broke the `shell: true` attempt at this bug: a checkout under a path with a space
+  // is re-split by the shell, so the entry has to reach the caller as one intact argv element.
+  it("resolves from a package directory whose path contains a space", () => {
+    const spaced = join(root, "cf os", "repo");
+    const installed = join(spaced, "node_modules", "vite");
+    mkdirSync(installed, { recursive: true });
+    writeFileSync(join(installed, "package.json"), JSON.stringify({ name: "vite", bin: "./cli.js" }));
+    writeFileSync(join(installed, "cli.js"), "");
+    assert.equal(resolveBinEntry(spaced, "vite"), join(installed, "cli.js"));
+  });
+
+  // Callers treat null as "leave the command as written", so a wrong guess must never be returned.
+  it("returns null when the bin cannot be resolved to a file that exists", () => {
+    assert.equal(resolveBinEntry(join(root, "nonexistent"), "vite"), null);
+    assert.equal(
+        resolveBinEntry(pkgWith("vite", { other: "./other.js" }, ["other.js"]), "vite"), null);
+    assert.equal(resolveBinEntry(pkgWith("vite", undefined, []), "vite"), null);
+    assert.equal(resolveBinEntry(pkgWith("vite", "./missing.js", []), "vite"), null);
+  });
+});

scripts/bin-entry.ts

@@ -0,0 +1,28 @@
+// Locate the JS entry point behind a `node_modules/.bin/<bin>` shim, so a tool can be spawned as
+// `node <entry>` rather than reached through `pnpm exec`.
+//
+// Two reasons callers want that. Speed: `pnpm exec` costs ~0.33s of process startup per call, which
+// for most of these builds is longer than the build itself. Portability: the `.bin` shim is a `.cmd`
+// file on Windows, which Node cannot spawn without a shell, and the `npm_execpath` fallback in
+// pnpm-command.ts is unavailable inside a Vite+ task -- `vp` runs task commands with a filtered
+// environment that does not include it.
+
+import { existsSync, readFileSync, realpathSync } from "node:fs";
+import { dirname, join } from "node:path";
+
+/**
+ * Absolute path to the JS entry point behind `node_modules/.bin/<bin>`, or null if it cannot be
+ * found. Resolved from `pkgDir`'s own node_modules so pnpm's per-package layout is respected.
+ */
+export function resolveBinEntry(pkgDir: string, bin: string): string | null {
+  try {
+    const manifestPath = realpathSync(join(pkgDir, "node_modules", bin, "package.json"));
+    const manifest = JSON.parse(readFileSync(manifestPath, "utf8"));
+    const relative = typeof manifest.bin === "string" ? manifest.bin : manifest.bin?.[bin];
+    if (!relative) return null;
+    const entry = join(dirname(manifestPath), relative);
+    return existsSync(entry) ? entry : null;
+  } catch {
+    return null;
+  }
+}

scripts/env-passthrough.test.ts

@@ -167,9 +167,11 @@ const matches = (name: string, pattern: string) =>
     .test(name);
 
 describe("build-time env passthrough", () => {
+  // These double as the keys compared against EXPECTED, so they are built with `/` rather than
+  // `join`, whose separator is platform-dependent. Forward slashes still resolve as paths on Windows.
   const areas = ["scripts", ...readdirSync("packages", { withFileTypes: true })
     .filter(entry => entry.isDirectory())
-    .map(entry => join("packages", entry.name))];
+    .map(entry => `packages/${entry.name}`)];
 
   it("uses only known categories", () => {
     for (const [area, groups] of Object.entries(EXPECTED)) {

scripts/generate-worker-types.ts

@@ -18,6 +18,8 @@ import { spawnSync } from "node:child_process";
 import { readdir, readFile, rm, writeFile } from "node:fs/promises";
 import { dirname, join, relative, resolve } from "node:path";
 import { fileURLToPath } from "node:url";
+import { resolveBinEntry } from "./bin-entry.ts";
+import { pnpmCommand } from "./pnpm-command.ts";
 
 const root = resolve(dirname(fileURLToPath(import.meta.url)), "..");
 const packagesDir = join(root, "packages");
@@ -122,15 +124,26 @@ async function generateOne(pkgDir: string): Promise<void> {
 
   try {
     const runtimeOnlyConfig = runtimeOnlyConfigs.get(pkgDir);
-    const args = ["exec", "wrangler", "types", outPath];
+    const args = ["types", outPath];
     if (runtimeOnlyConfig) {
       args.push("--config", runtimeOnlyConfig, "--include-env", "false");
     }
+    // Reached directly where wrangler resolves, which also avoids `pnpm exec` being unspawnable on
+    // Windows; packages without their own copy (mcp-shared) fall back through pnpm-command.ts.
+    const wranglerEntry = resolveBinEntry(pkgDir, "wrangler");
+    const [command, argv]: [string, string[]] = wranglerEntry
+      ? [process.execPath, [wranglerEntry, ...args]]
+      : pnpmCommand(["exec", "wrangler", ...args]);
     const result = spawnSync(
-      "pnpm",
-      args,
+      command,
+      argv,
       { cwd: pkgDir, encoding: "utf8", env: process.env },
     );
+    // A failure to spawn leaves `status` null with no output, which the check below would report as
+    // a wrangler failure with two blank lines. Surface the real cause instead.
+    if (result.error) {
+      throw new Error(`could not run wrangler in ${rel}: ${result.error.message}`);
+    }
     if (result.status !== 0) {
       console.error(result.stdout);
       console.error(result.stderr);

scripts/pnpm-command.test.ts

@@ -0,0 +1,51 @@
+import assert from "node:assert/strict";
+import { describe, it } from "node:test";
+import { pnpmCommand } from "./pnpm-command.ts";
+
+// Real `npm_execpath` values, as measured on Windows: pnpm installed through npm, pnpm installed
+// standalone, and -- the case the guard exists for -- the same variable under `npm run`.
+const PNPM_MJS = "C:\\nvm4w\\nodejs\\node_modules\\pnpm\\bin\\pnpm.mjs";
+const PNPM_CJS = "C:\\Users\\dev\\AppData\\Local\\pnpm\\bin\\pnpm.cjs";
+const NPM_CLI = "C:\\Users\\dev\\AppData\\Local\\nvm\\node_modules\\npm\\bin\\npm-cli.js";
+
+describe("pnpmCommand", () => {
+  it("runs pnpm's own entry point through node on Windows", () => {
+    const [command, args] = pnpmCommand(["install"], { npm_execpath: PNPM_MJS }, "win32");
+    assert.equal(command, process.execPath);
+    assert.deepEqual(args, [PNPM_MJS, "install"]);
+  });
+
+  it("recognises a .cjs entry as well as .mjs", () => {
+    const [command, args] = pnpmCommand(["install"], { npm_execpath: PNPM_CJS }, "win32");
+    assert.equal(command, process.execPath);
+    assert.deepEqual(args, [PNPM_CJS, "install"]);
+  });
+
+  // The reason `shell: true` was not an option: a shell would split this argument in two.
+  it("passes arguments through untouched, including one containing a space", () => {
+    const configPath = "C:\\Users\\Some Name\\cloudflare-os\\wrangler.jsonc";
+    const [, args] = pnpmCommand(
+        ["exec", "wrangler", "dev", "-c", configPath], { npm_execpath: PNPM_MJS }, "win32");
+    assert.deepEqual(args, [PNPM_MJS, "exec", "wrangler", "dev", "-c", configPath]);
+  });
+
+  // Substituting this unchecked would run `npm install` against a pnpm workspace.
+  it("rejects npm's CLI rather than using the wrong package manager", () => {
+    assert.deepEqual(
+        pnpmCommand(["install"], { npm_execpath: NPM_CLI }, "win32"), ["pnpm", ["install"]]);
+  });
+
+  // A direct `node scripts/run-local.ts` has no pnpm ancestor to inherit the variable from. Nothing
+  // can be substituted, so the call keeps whatever behaviour it has today.
+  it("falls back to bare pnpm when npm_execpath is absent or empty", () => {
+    assert.deepEqual(pnpmCommand(["install"], {}, "win32"), ["pnpm", ["install"]]);
+    assert.deepEqual(pnpmCommand(["install"], { npm_execpath: "" }, "win32"), ["pnpm", ["install"]]);
+  });
+
+  it("leaves other platforms exactly as they were", () => {
+    assert.deepEqual(
+        pnpmCommand(["install"], { npm_execpath: PNPM_MJS }, "linux"), ["pnpm", ["install"]]);
+    assert.deepEqual(
+        pnpmCommand(["install"], { npm_execpath: PNPM_MJS }, "darwin"), ["pnpm", ["install"]]);
+  });
+});

scripts/pnpm-command.ts

@@ -0,0 +1,39 @@
+// Spawn pnpm from a script without going through a shell.
+//
+// On Windows the `pnpm` on PATH is a `.cmd` shim. Node spawns processes directly rather than through
+// a shell, so there is no extensionless `pnpm` to execute and the call fails with ENOENT. Naming the
+// shim explicitly does not help either: since the fix for CVE-2024-27980 Node refuses to spawn
+// `.cmd`/`.bat` without a shell and fails with EINVAL instead.
+//
+// `shell: true` makes the call work but is not safe here. A shell re-splits the command line, and
+// the callers pass absolute paths built from the checkout location -- on a checkout whose path
+// contains a space (`C:\Users\Some Name\...`) those arguments break apart.
+//
+// `npm_execpath` is the way out: under `pnpm run` it holds pnpm's own JS entry point, which `node`
+// executes directly with no shell, so every argument keeps its exact value. Under `npm run` the same
+// variable points at npm's CLI instead, so it is checked before being used -- substituting it
+// unchecked would silently run `npm install` against a pnpm workspace.
+
+// pnpm's JS entry, as `npm_execpath` spells it (`.cjs` or `.mjs` depending on how pnpm was
+// installed). npm's `npm-cli.js` and the standalone `pnpm.exe`/`pnpm.cmd` shims do not match.
+const PNPM_JS_ENTRY = /[\\/]pnpm\.[cm]?js$/i;
+
+/**
+ * `[command, args]` for running pnpm with `args`, to hand to `spawn`, `spawnSync` or `execFileSync`.
+ *
+ * Off Windows, and on Windows whenever the launcher was not pnpm, this is a plain `["pnpm", args]`:
+ * an entry path this cannot support keeps today's loud ENOENT rather than quietly reaching for a
+ * different package manager.
+ *
+ * `env` and `platform` are parameters only so the Windows branch stays testable on other platforms.
+ */
+export function pnpmCommand(
+  args: string[],
+  env: NodeJS.ProcessEnv = process.env,
+  platform: NodeJS.Platform = process.platform,
+): [string, string[]] {
+  const execPath = env.npm_execpath ?? "";
+  return platform === "win32" && PNPM_JS_ENTRY.test(execPath)
+    ? [process.execPath, [execPath, ...args]]
+    : ["pnpm", args];
+}