Fix nested pnpm spawns on Windows

[Maximo-Guk]

Fixes #19, fixes #120. Supersedes #107 by @Sainigurnoor511, whose npm_execpath approach this builds on. Stacked on #233.

pnpm run-local and pnpm dev-server cannot start on Windows: pnpm on PATH is a .cmd shim, which Node cannot spawn without a shell

Changed it so that the pnpm exec sites now resolve their binary directly, only falling back to pnpm if the binary is not found. We were actually already using this approach for the run-dev-server file to reduce overhead from invoking pnpm ( ~0.33 s of overhead compared to invoking node binary directly ).

Verified on my Windows 11 VM.

[github-actions]

Preview: maximo-wi-4209cff1

https://maximo-wi-4209cff1-os-public-router.gadgets-staging.workers.dev

Dashboard · deleted when this PR closes

[ask-bonk]

realpathSync correctly removed from the import. No dangling references.

I've completed a thorough review. The PR is a clean, well-documented, well-tested refactor. Let me summarize.

Review

This is a high-quality, focused change. I found no actionable issues.

What it does, and why it's correct:

• scripts/pnpm-command.ts centralizes the Windows-safe pnpm spawn. Routing through node <npm_execpath> avoids both the ENOENT/EINVAL from spawning a .cmd shim without a shell and the argv re-splitting that shell: true would cause on paths with spaces. The PNPM_JS_ENTRY guard correctly rejects npm's npm-cli.js (preventing an accidental npm install against a pnpm workspace) and falls back to bare pnpm off-Windows and when npm_execpath is absent — preserving today's loud-failure behavior rather than silently doing the wrong thing.
• scripts/bin-entry.ts cleanly extracts resolveBinEntry from run-dev-server.ts (removing the now-unused realpathSync import) and reuses it in both build-app.mjs files, where the npm_execpath trick is unavailable because Vite+ strips the environment. The .bin-entry-then-pnpm exec-fallback ordering is right.
• Invocation paths line up with the env assumptions: run-local/run-dev-server run under pnpm run (so npm_execpath is present for the pnpmCommand sites), while the build-app.mjs task path resolves the vite entry directly.

Testing: The new tests cover the meaningful cases — .cjs/.mjs entries, the npm-CLI rejection, space-in-path argument integrity, empty/absent npm_execpath, non-Windows passthrough, and resolveBinEntry string/object bin, spaced paths, and every null path. Ran them locally: 10/10 pass. The bin-entry test's use of a tmpdir (to avoid polluting run-local's source hash on parallel node --test) is a nice touch.

Non-blocking observations (not part of this PR's scope, no change requested):

• scripts/generate-worker-types.ts:129 still does spawnSync("pnpm", ["exec", "wrangler", ...]), which has the same Windows failure mode. It's a separate codegen tool outside the run-local/dev-server startup path this PR targets (pnpm run-local cannot start on Windows: spawnSync pnpm ENOENT #19, Windows: pnpm run-local fails with spawnSync pnpm ENOENT #120), so it's not a regression here — just a candidate for the same treatment in a follow-up.

LGTM!

github run

[ask-bonk]

The review is complete. All files examined, tests run (14/14 pass), and I've verified the correctness of the design decisions.

Review

This is a clean, focused, well-tested refactor that fixes a real Windows blocker (#19, #120). I found no actionable issues.

What it does, and why it's correct:

• scripts/pnpm-command.ts centralizes the Windows-safe pnpm spawn. Routing through node <npm_execpath> avoids both the ENOENT/EINVAL from spawning a .cmd shim without a shell and the argv re-splitting shell: true would cause on spaced paths. The PNPM_JS_ENTRY guard correctly rejects npm's npm-cli.js and falls back to bare pnpm off-Windows and when npm_execpath is absent — deliberately preserving today's loud ENOENT rather than silently reaching for the wrong package manager.
• scripts/bin-entry.ts cleanly extracts resolveBinEntry from run-dev-server.ts and is reused by both build-app.mjs files and generate-worker-types.ts, where npm_execpath is unavailable (Vite+ strips the environment). realpathSync correctly resolves pnpm's symlinked layout before joining the bin entry, and the .bin-entry-then-pnpm exec-fallback ordering is right.
• generate-worker-types.ts gets the same treatment (this closes the loop on the prior review's "non-blocking observation" — it's no longer a straggler), plus a helpful result.error surfacing so a spawn failure no longer masquerades as a wrangler failure with blank output.
• .gitattributes (eol=lf) is a necessary companion: byte-hashing tests (release-manifest golden, types:generate --check) would break under Windows' core.autocrlf. I confirmed no files are committed with CRLF, so this won't cause renormalization churn.
• The env-passthrough.test.ts change (packages/${name} instead of join) is a genuine Windows portability fix for the test's own EXPECTED key comparison, which uses /.

Testing: New tests cover the meaningful cases — .cjs/.mjs entries, npm-CLI rejection, space-in-path integrity, empty/absent npm_execpath, non-Windows passthrough, and resolveBinEntry's string/object bin, spaced paths, and every null path. Ran the full scripts suite locally: 14/14 pass. The tmpdir isolation in bin-entry.test.ts (to avoid polluting run-local's source hash under parallel node --test) is a nice touch.

One design point worth noting (not a defect): the bare-pnpm fallback resolves pnpm.exe but not pnpm.cmd, so a direct node scripts/run-local.ts (no pnpm ancestor) on a machine where pnpm is only a .cmd shim would still fail. The PR documents this as the intended loud-failure behavior, and the supported entry points (pnpm run-local/pnpm dev-server) always supply npm_execpath, so this is a deliberate, reasonable choice rather than a gap.

LGTM!

github run

[ask-bonk]

Behavior parity confirmed. The change from execFileSync(cmd, args, ...) to execFileSync(command, argv, ...) where [command, argv] = pnpmCommand(args) is a clean refactor — off-Windows it produces exactly ["pnpm", args].

My review is complete. I've examined all 11 changed files, verified the resolveBinEntry extraction is byte-faithful to the original, confirmed the test suite passes (14/14 across the three affected test files), checked there are no remaining direct pnpm spawns anywhere in the repo, and verified no CRLF files exist that .gitattributes would renormalize.

This is a clean, well-scoped, well-documented refactor that fixes a genuine Windows blocker (#19, #120). The design is sound:

• pnpm-command.ts routes through node <npm_execpath> to avoid the .cmd-shim spawn failure and shell: true's argv re-splitting, with the PNPM_JS_ENTRY guard correctly rejecting npm's CLI and falling back to loud pnpm when the env var is absent.
• bin-entry.ts faithfully extracts resolveBinEntry and reuses it in build-app.mjs (both) and generate-worker-types.ts, where npm_execpath is unavailable under Vite+'s stripped environment.
• generate-worker-types.ts also gains a result.error surfacing so a spawn failure no longer masquerades as a blank wrangler failure.
• .gitattributes (eol=lf) is a correct companion for the byte-hashing tests, and no committed files carry CRLF, so it won't cause churn.

I found no actionable issues. The prior review's design notes (bare-pnpm fallback resolving pnpm.exe not pnpm.cmd, and the deliberate loud-failure for direct node scripts/run-local.ts without a pnpm ancestor) are documented intentional choices, and the supported entry points always supply npm_execpath.

LGTM!

github run

[ask-bonk]

No CRLF-tracked files, so .gitattributes won't cause renormalization churn.

I've reviewed all 11 changed files. My analysis:

• scripts/pnpm-command.ts: The node <npm_execpath> approach correctly avoids the .cmd-shim spawn failure and shell: true argv re-splitting. The PNPM_JS_ENTRY regex correctly matches pnpm.cjs/pnpm.mjs while rejecting npm's npm-cli.js, falling back to bare pnpm when the var is absent. platform/env params keep the Windows branch testable off-Windows.
• scripts/bin-entry.ts: resolveBinEntry is a byte-faithful extraction of the original in run-dev-server.ts (confirmed against base commit 31ce73b), correctly removing the now-unused realpathSync import from that file.
• scripts/generate-worker-types.ts: The .bin-then-pnpm exec-fallback reuse is correct, and the new result.error surfacing genuinely fixes a case where a spawn failure previously masqueraded as a blank wrangler failure. env: process.env is preserved so npm_execpath reaches the pnpm fallback.
• Both build-app.mjs files: consistent reuse; GATEKEEPER_APP_UNMINIFIED still set explicitly.
• .gitattributes (eol=lf): a correct companion for the byte-hashing tests; confirmed no tracked files carry CRLF, so no renormalization churn.
• env-passthrough.test.ts: the packages/${name} change is a legitimate Windows fix (the EXPECTED keys use /). The new lowercase npm_execpath reads don't affect its uppercase-only scanner, so no EXPECTED update is required.

Ran the three affected test files: 14/14 pass. No remaining direct pnpm/.cmd-shim spawns exist in TS sources (the pnpm exec strings in wrangler.jsonc build.command entries are rewritten by withoutPnpmIndirection in dev). The design decisions (bare-pnpm resolving pnpm.exe not pnpm.cmd; loud-failure for a pnpm-less direct node invocation) are documented, intentional choices, and the supported entry points always supply npm_execpath.

I found no actionable issues.

LGTM!

github run

[ask-bonk]

github run

[ask-bonk]

@Maximo-Guk Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

[github-actions]

Preview: pr235-maximo-window-4209cff1

https://pr235-maximo-window-4209cff1-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[Maximo-Guk]

bonk break glass