Skip to content

fix(scripts): spawn pnpm without the Windows .cmd shim - #1776

Open
HuzaifaAbdulRehman wants to merge 4 commits into
openai:mainfrom
HuzaifaAbdulRehman:fix/windows-pnpm-spawn
Open

fix(scripts): spawn pnpm without the Windows .cmd shim#1776
HuzaifaAbdulRehman wants to merge 4 commits into
openai:mainfrom
HuzaifaAbdulRehman:fix/windows-pnpm-spawn

Conversation

@HuzaifaAbdulRehman

@HuzaifaAbdulRehman HuzaifaAbdulRehman commented Aug 29, 2026

Copy link
Copy Markdown

Summary

Node has refused to spawn .cmd without a shell since 20.12.2, so run.mjs:55 dies with spawnSync pnpm.cmd EINVAL, and AGENTS.md makes that script mandatory. scripts/prettier-changed.mjs:66 shares the defect in the format-check step.

Execa is already a dev dependency and resolves Windows shims itself, so both now use execaSync. Only the bootstrap install runs before node_modules exists; it takes no dynamic arguments, so a fixed cmd.exe line covers it.

Test plan

  • pnpm exec vitest run scripts/pnpm-bootstrap.test.mjs --mode full
  • .agents/skills/code-change-verification/scripts/run.sh on a clean Windows checkout
  • a changed build%PATH%probe.ts now formats instead of failing

Issue number

Closes #1775

Checks

No package code changed, so no changeset. No public API touched.

Node has refused to spawn .cmd and .bat files without a shell since 20.12.2,
18.20.2 and 21.7.3, so spawnSync('pnpm.cmd', args) fails with EINVAL. That
broke code-change-verification on its first step and prettier-changed.mjs on
the format-check step, leaving Windows contributors unable to run the
verification the contributor guide requires.

Build the cmd.exe command line explicitly and pass it verbatim instead of
using shell: true, which joins argv without quoting and would split
`pnpm -r -F "@openai/*" dist:check` into several arguments.

Escape cmd.exe metacharacters twice. pnpm.cmd is a batch shim that forwards
%*, so the first parse consumes one layer and the shim's own expansion is
parsed again. Arguments carrying % are refused outright, since cmd.exe
expands it inside quotes and no escape survives both parses.
@changeset-bot

changeset-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 0f012a1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@liuxusummer

Copy link
Copy Markdown

The Windows failure is real, but I think the custom two-pass cmd.exe quoting layer is a larger compatibility surface than this bootstrap script needs. One concrete regression is that quoteWindowsArgument() rejects %; prettier-changed.mjs passes Git filenames, and % is valid in a Windows filename, so a legitimate changed file can now make format verification fail before Prettier runs.

This repository already depends on execa, whose documented Windows behavior resolves and executes .cmd/.bat files without a shell and performs argument quoting itself. The bootstrap constraint is that run.mjs must install dependencies before it can import Execa, but that can be kept narrow: use a fixed, repository-controlled cmd.exe /d /s /c "pnpm.cmd i --frozen-lockfile" only for the initial Windows install (there are no dynamic arguments to quote), then dynamically import execaSync and use it for the remaining pnpm steps. prettier-changed.mjs runs after dependencies exist and can use execaSync directly.

That would avoid maintaining a general-purpose cmd parser/escaper and would preserve filenames containing %. At minimum, could the PR add a clean-checkout Windows integration test plus a changed filename containing % before adopting the custom quoting contract?

@HuzaifaAbdulRehman

Copy link
Copy Markdown
Author

You are right about %. I confirmed it here: build%PATH%.ts throws in quoteWindowsArgument, while execa passes it through unchanged. Reworking along the lines you describe, with a fixed cmd.exe line for the bootstrap install only and execaSync after that, plus the clean-checkout Windows run and the % filename case.

One thing worth recording: execa 9.6.1 still lets a crafted quote through a .cmd shim, so x"&echo ...&" ran its second command for me. Not reachable from git filenames, since Windows disallows quotes in names.

Execa is already a dev dependency, resolves the Windows pnpm shim without a
shell and quotes arguments itself, so the hand-written cmd.exe escaping is not
needed. That escaping also rejected arguments containing %, which is legal in a
Windows file name, so prettier-changed.mjs could fail on a legitimate changed
file before Prettier ran.

Only the bootstrap install runs before node_modules exists. It takes no
caller-supplied arguments, so it keeps a fixed cmd.exe command line and
everything after it goes through execaSync.
@HuzaifaAbdulRehman

HuzaifaAbdulRehman commented Aug 31, 2026

Copy link
Copy Markdown
Author

@seratch this is my first PR here and the CI workflow is sitting at action_required, so its test, coverage and Windows jobs have not run. Could you approve them?

On the approach: after the review feedback I dropped the hand-rolled cmd.exe quoting for execaSync, keeping a fixed command line only for the bootstrap install that runs before node_modules exists. Happy to split prettier-changed.mjs into its own PR if you would rather take the verification script alone.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One failure-reporting regression remains in the Execa path. With reject: false, Execa returns the error object, and in Execa 9.6.1 exitCode is undefined when the subprocess fails to spawn. runPnpm() currently discards that error state and passes only exitCode/signal into reportExit(), so an ENOENT/spawn failure is reported as terminated by an unknown signal; the old spawnSync path explicitly checked result.error and printed the actual startup error.

Could we preserve Execa's spawn failure here, for example by checking result.failed when exitCode and signal are absent and surfacing shortMessage/originalMessage? scripts/prettier-changed.mjs has the same edge case: a failed spawn with reject: false now falls through to process.exit(prettier.exitCode ?? 1) without reporting the returned Execa error. A focused missing-pnpm test would pin both diagnostics.

With reject: false Execa returns its error rather than throwing, and a process
that never started carries no exitCode and no signal. runPnpm read exitCode
alone, so a missing pnpm was reported as "terminated by an unknown signal" and
the startup error that spawnSync exposed through result.error was lost.
prettier-changed.mjs exited 1 without reporting it at all.

Add execaRunOutcome, which separates a normal exit, a signal, and a process
that never started, and surface the Execa message in the last case. Both
scripts now share it.
@HuzaifaAbdulRehman

HuzaifaAbdulRehman commented Sep 1, 2026

Copy link
Copy Markdown
Author

You are right, thanks for catching it. An Execa result for a process that never started carries failed: true and code: 'ENOENT' but no exitCode and no signal, so reportExit reached its signal branch. Running the old logic against a real spawn failure shows what a contributor would have seen:

OLD -> terminated by an unknown signal
NEW -> FAILED TO START: The "cwd" option is invalid: ...

My earlier check missed this because I probed a missing binary, which on Windows resolves through cmd and comes back with exit 1. Only a genuine spawn failure has the empty shape.

Fixed in 03ae793. execaRunOutcome in scripts/pnpm-bootstrap.mjs separates a normal exit, a signal, and a process that never started, surfacing originalMessage or shortMessage in the last case. Both run.mjs and prettier-changed.mjs use it, so the second throws with the startup error rather than exiting 1 in silence.

Five tests cover it. One triggers a real Execa spawn failure instead of a hand-built object, and three of the five fail against the previous logic.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. The Execa approach addresses the earlier quoting and startup-error concerns.

Before merging, please add the Windows-only pnpm shim test to Windows CI and provide results from the actual verification entrypoint on a clean Windows checkout, including the pnpm installation method. The evidence should cover bootstrap installation, the compound validation arguments, formatting a changed filename containing %, and failure exit-code propagation.

The current Windows job runs only sandbox tests, while the Linux jobs skip the new Windows-only case, so the green checks do not yet establish that the affected workflow works end to end.

@HuzaifaAbdulRehman

Copy link
Copy Markdown
Author

Added the Windows-only shim test to the Windows CI job. In a clean Windows worktree with pnpm 11.24.0 installed through Corepack, the PowerShell verification script passed bootstrap, the initial build, compound validation, % filename formatting, lint, build and dist checks, and all 10 shim tests. It then returned 1 for 105 existing Unix-only failures involving /bin/sh and symlinks, and propagated that code correctly.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked current 0f012a11. My earlier spawn-failure diagnostic concern is fixed: the Execa result keeps the failed-start state and surfaces the startup message, and the focused bootstrap suite passes locally (9 passed, 1 Windows-only skipped on macOS). The author has also now provided the clean-Windows verification requested by @seratch: Corepack-installed pnpm 11.24.0, bootstrap, compound validation args, a % filename, shim tests, and failure exit-code propagation. I do not see a remaining blocker in the affected path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

code-change-verification cannot run on Windows under Node 20.12.2 or newer

4 participants