From 826e991576f867c4fe9b3d603b1556284a03a768 Mon Sep 17 00:00:00 2001 From: Maximo Guk <62088388+Maximo-Guk@users.noreply.github.com> Date: Mon, 17 Aug 2026 18:44:35 -0500 Subject: [PATCH] Fix nested pnpm spawns on Windows so run-local and dev-server start 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 (ENOENT), and naming the shim explicitly fails too since the CVE-2024-27980 fix (EINVAL). `shell: true` is not a fix here: a shell re-splits the command line, and these callers pass absolute paths built from the checkout location, which break apart on a path containing a space. Add scripts/pnpm-command.ts, which runs pnpm's own JS entry point under `node` when `npm_execpath` points at it, so argv survives byte-for-byte with no shell. Guarded on the path, because under `npm run` that variable points at npm and substituting it unchecked would run npm against a pnpm workspace. Where pnpm is a native executable the plain `pnpm` fallback still resolves it, since PATH lookup appends `.exe` but not `.cmd`. That alone is not enough for the `pnpm exec` sites: Vite+ runs task commands with a filtered environment that omits `npm_execpath`, so build-app.mjs has no way back to pnpm at all. Extract the existing resolveBinEntry() out of run-dev-server.ts into scripts/bin-entry.ts and reach those binaries directly, which also skips ~0.33s of pnpm startup per call. Verified on Windows 11 / Node 24.19.0 / pnpm 11.17.0: `pnpm run-local` goes from failing at the first `pnpm install` to serving HTTP 200 on 8787. Fixes #19 Fixes #120 Co-Authored-By: Claude Opus 5 --- .gitattributes | 7 ++ AGENTS.md | 2 +- packages/backend-utils/vitest.config.ts | 2 +- packages/gatekeeper-context/build-app.mjs | 13 +++- packages/gatekeeper-scheduler/build-app.mjs | 13 +++- .../gatekeeper-scheduler/vitest.config.ts | 2 +- packages/router/vitest.config.ts | 2 +- packages/typed-storage/vitest.config.ts | 2 +- packages/workshop-backend/vitest.config.ts | 2 +- .../vitest.integration.config.ts | 2 +- {test-setup => scripts}/assert-workerd.ts | 0 scripts/bin-entry.test.ts | 65 +++++++++++++++++++ scripts/bin-entry.ts | 28 ++++++++ scripts/env-passthrough.test.ts | 4 +- scripts/generate-worker-types.ts | 19 +++++- scripts/pnpm-command.test.ts | 51 +++++++++++++++ scripts/pnpm-command.ts | 39 +++++++++++ scripts/run-dev-server.ts | 31 +++------ scripts/run-local.ts | 14 ++-- 19 files changed, 255 insertions(+), 43 deletions(-) create mode 100644 .gitattributes rename {test-setup => scripts}/assert-workerd.ts (100%) create mode 100644 scripts/bin-entry.test.ts create mode 100644 scripts/bin-entry.ts create mode 100644 scripts/pnpm-command.test.ts create mode 100644 scripts/pnpm-command.ts diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..d48db77d0 --- /dev/null +++ b/.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 diff --git a/AGENTS.md b/AGENTS.md index a222d9efb..0c3ab3943 100644 --- a/AGENTS.md +++ b/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. diff --git a/packages/backend-utils/vitest.config.ts b/packages/backend-utils/vitest.config.ts index f5eb5ac96..c3f31654c 100644 --- a/packages/backend-utils/vitest.config.ts +++ b/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"], }, }); diff --git a/packages/gatekeeper-context/build-app.mjs b/packages/gatekeeper-context/build-app.mjs index 4b5ec686d..ec838c533 100644 --- a/packages/gatekeeper-context/build-app.mjs +++ b/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", diff --git a/packages/gatekeeper-scheduler/build-app.mjs b/packages/gatekeeper-scheduler/build-app.mjs index 14e076e59..0138fd53c 100644 --- a/packages/gatekeeper-scheduler/build-app.mjs +++ b/packages/gatekeeper-scheduler/build-app.mjs @@ -1,6 +1,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 packageDirectory = resolve(fileURLToPath(import.meta.url), ".."); const watch = process.argv.includes("--watch"); @@ -8,9 +10,16 @@ const watch = process.argv.includes("--watch"); // 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", diff --git a/packages/gatekeeper-scheduler/vitest.config.ts b/packages/gatekeeper-scheduler/vitest.config.ts index fe5cce6f1..44f7ac5d3 100644 --- a/packages/gatekeeper-scheduler/vitest.config.ts +++ b/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"], }, }); diff --git a/packages/router/vitest.config.ts b/packages/router/vitest.config.ts index 748520f1f..02e29ae22 100644 --- a/packages/router/vitest.config.ts +++ b/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'], }, }) diff --git a/packages/typed-storage/vitest.config.ts b/packages/typed-storage/vitest.config.ts index 77e2ccfff..cd49e7c0b 100644 --- a/packages/typed-storage/vitest.config.ts +++ b/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'], }, }) diff --git a/packages/workshop-backend/vitest.config.ts b/packages/workshop-backend/vitest.config.ts index 65d54cfcc..1b40e0330 100644 --- a/packages/workshop-backend/vitest.config.ts +++ b/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'], }, }) diff --git a/packages/workshop-backend/vitest.integration.config.ts b/packages/workshop-backend/vitest.integration.config.ts index 43fdcfbcc..425664b28 100644 --- a/packages/workshop-backend/vitest.integration.config.ts +++ b/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 diff --git a/test-setup/assert-workerd.ts b/scripts/assert-workerd.ts similarity index 100% rename from test-setup/assert-workerd.ts rename to scripts/assert-workerd.ts diff --git a/scripts/bin-entry.test.ts b/scripts/bin-entry.test.ts new file mode 100644 index 000000000..b90cecb25 --- /dev/null +++ b/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/` 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); + }); +}); diff --git a/scripts/bin-entry.ts b/scripts/bin-entry.ts new file mode 100644 index 000000000..5f6fcdf13 --- /dev/null +++ b/scripts/bin-entry.ts @@ -0,0 +1,28 @@ +// Locate the JS entry point behind a `node_modules/.bin/` shim, so a tool can be spawned as +// `node ` 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/`, 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; + } +} diff --git a/scripts/env-passthrough.test.ts b/scripts/env-passthrough.test.ts index b86e4a3b8..3fd1989cc 100644 --- a/scripts/env-passthrough.test.ts +++ b/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)) { diff --git a/scripts/generate-worker-types.ts b/scripts/generate-worker-types.ts index a101f9e27..a458b0b3d 100644 --- a/scripts/generate-worker-types.ts +++ b/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 { 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); diff --git a/scripts/pnpm-command.test.ts b/scripts/pnpm-command.test.ts new file mode 100644 index 000000000..8916f13b3 --- /dev/null +++ b/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"]]); + }); +}); diff --git a/scripts/pnpm-command.ts b/scripts/pnpm-command.ts new file mode 100644 index 000000000..53c6b5db9 --- /dev/null +++ b/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]; +} diff --git a/scripts/run-dev-server.ts b/scripts/run-dev-server.ts index 4b9fd6e3e..71cde0aee 100644 --- a/scripts/run-dev-server.ts +++ b/scripts/run-dev-server.ts @@ -12,7 +12,7 @@ // VITE_BACKEND_HOST=localhost:9000 Also pass --port 9000 to wrangler dev. import { - existsSync, readFileSync, writeFileSync, readdirSync, statSync, realpathSync, + existsSync, readFileSync, writeFileSync, readdirSync, statSync, } from "node:fs"; import { spawn, type ChildProcess } from "node:child_process"; import { connect } from "node:net"; @@ -20,8 +20,10 @@ import { constants } from "node:os"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; import { parse } from "jsonc-parser"; +import { resolveBinEntry } from "./bin-entry.ts"; import { getDevServerConfig } from "./dev-server-config.ts"; import { killProcessTree } from "./kill-process-tree.ts"; +import { pnpmCommand } from "./pnpm-command.ts"; import type { ServiceBinding, WranglerBuild } from "./release/manifest-lib.ts"; const SCRIPTS_DIR = dirname(fileURLToPath(import.meta.url)); @@ -268,10 +270,10 @@ try { [join(WORKSHOP_BACKEND_DIR, "scripts", "build-format-blueprints.mjs")], WORKSHOP_BACKEND_DIR, ), - runBuild("configurator UIs", "pnpm", - ["exec", "vp", "run", "-r", "--cache", "build:configurator", "--dev"], ROOT), - runBuild("gatekeeper app UIs", "pnpm", - ["exec", "vp", "run", "-r", "--cache", "build:app:dev"], ROOT), + runBuild("configurator UIs", + ...pnpmCommand(["exec", "vp", "run", "-r", "--cache", "build:configurator", "--dev"]), ROOT), + runBuild("gatekeeper app UIs", + ...pnpmCommand(["exec", "vp", "run", "-r", "--cache", "build:app:dev"]), ROOT), ]); } catch (err) { // The SIGTERM handler killing the builds also lands here, as the rejection of whichever build @@ -337,21 +339,6 @@ function bindingName(gk: Gatekeeper): string { // an unrecognised command still works, just at the original speed. // --------------------------------------------------------------------------- -// Absolute path to the JS entry point behind `node_modules/.bin/`, or null if it cannot be -// found. Resolved from the package's own node_modules so pnpm's per-package layout is respected. -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; - } -} - // Whether wrapping `path` in plain double quotes is safe in the shell that runs the rewritten // command: inside them POSIX shells still expand `$` and backticks and collapse `\\`, cmd still // expands `%`, and an embedded quote or a trailing backslash would break the quoting itself. @@ -564,9 +551,9 @@ console.log(`\nStarting: wrangler dev ${args.join(" ")}\n`); // Reached directly for the same reason the generated custom builds are; falls back to `pnpm exec` if // it cannot be resolved. const wranglerEntry = resolveBinEntry(ROOT, "wrangler"); -const [wranglerCommand, wranglerArgv] = wranglerEntry +const [wranglerCommand, wranglerArgv]: [string, string[]] = wranglerEntry ? [process.execPath, [wranglerEntry, "dev", ...args]] - : ["pnpm", ["exec", "wrangler", "dev", ...args]]; + : pnpmCommand(["exec", "wrangler", "dev", ...args]); // `spawn`, not `execFileSync`, so the deferred watchers can start once the server is up. It stays in // this process group with the terminal attached, so Ctrl-C reaches it as before. diff --git a/scripts/run-local.ts b/scripts/run-local.ts index e01e3f345..21a9da657 100644 --- a/scripts/run-local.ts +++ b/scripts/run-local.ts @@ -22,6 +22,7 @@ import { existsSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; import { dirname, join, relative, sep } from "node:path"; import { fileURLToPath } from "node:url"; import { getDevServerConfig } from "./dev-server-config.ts"; +import { pnpmCommand } from "./pnpm-command.ts"; const ROOT = dirname(dirname(fileURLToPath(import.meta.url))); const STAMP_PATH = join(ROOT, ".run-local-stamp"); @@ -121,21 +122,22 @@ const outputsPresent = existsSync(FRONTEND_DIST) && existsSync(TYPED_STORAGE_DIS const needsBuild = stamp !== sourceHash || !outputsPresent; const needsInstall = needsBuild || !existsSync(NODE_MODULES); -function run(cmd: string, args: string[]): void { - console.log(`\n> ${cmd} ${args.join(" ")}`); - execFileSync(cmd, args, { stdio: "inherit", cwd: ROOT }); +function runPnpm(args: string[]): void { + console.log(`\n> pnpm ${args.join(" ")}`); + const [command, argv] = pnpmCommand(args); + execFileSync(command, argv, { stdio: "inherit", cwd: ROOT }); } if (needsInstall) { - run("pnpm", ["install"]); + runPnpm(["install"]); } else { console.log("Dependencies up to date; skipping install."); } if (needsBuild) { // Build only what's required to run locally (no full-repo type-check / no frontend tsc). - run("pnpm", ["--filter", "@gadgets/typed-storage", "build"]); - run("pnpm", ["--filter", "@gadgets/workshop-frontend", "exec", "vite", "build"]); + runPnpm(["--filter", "@gadgets/typed-storage", "build"]); + runPnpm(["--filter", "@gadgets/workshop-frontend", "exec", "vite", "build"]); // Record the stamp only after a successful build so an interrupted build retries next time. writeFileSync(STAMP_PATH, sourceHash + "\n");