Skip to content

fix: drop patchPnpmEnv so standalone+self-update works on Windows - #258

Merged
zkochan merged 1 commit into
masterfrom
fix-standalone-windows-path
May 11, 2026
Merged

zkochan merged 1 commit into
masterfrom
fix-standalone-windows-path

Conversation

@zkochan

@zkochan zkochan commented May 11, 2026 •

Copy link
Copy Markdown
Member

Summary

src/utils/patchPnpmEnv prepends dest/node_modules/.bin (and .bin/bin) to PATH before spawning pnpm install and pnpm store prune. On Windows in standalone mode, node_modules/.bin/pnpm.cmd is an npm-created shim that points at the bootstrap pnpm (currently 11.0.4) — npm linked it there when it installed @pnpm/exe from node_modules. The self-updated pnpm written by pnpm self-update lives at $PNPM_HOME/bin, separately added to PATH via addPath() in install-pnpm.

When the user requests a pnpm version different from the bootstrap under standalone: true on Windows, patchPnpmEnv's .bin entry shadows the self-updated $PNPM_HOME/bin, so the action's internal pnpm install runs on the bootstrap. On a 11.0.x bootstrap this breaks any 11.1+ install flag — e.g. --no-runtime:

ERROR  Unknown option: 'runtime'

POSIX standalone got lucky because .bin and $PNPM_HOME resolve to the same directory there. Non-standalone never tripped on this since the .bin/pnpm symlink for a regular pnpm package keeps working across self-updates.

Fix

  • Removed patchPnpmEnv and the now-empty src/utils/ module.
  • spawnSync in pnpm-install and pnpm-store-prune now inherits process.env, whose PATH is already correctly fronted by $PNPM_HOME/bin and $PNPM_HOME via addPath() in install-pnpm.

Regression test

Added standalone_windows_self_update to .github/workflows/test.yaml:

  • runs-on: windows-latest
  • standalone: true
  • version: 11.1.0 (different from bootstrap 11.0.4)
  • run_install with args: ['--no-runtime']

With the previous code, run_install would have routed through the bootstrap and failed with Unknown option: 'runtime'. With the fix, install runs on the self-updated 11.1.0 and the flag is honored.

Context

Originally found while building pnpm/setup (the new combined pnpm + JS runtime action that always runs standalone). The same bug existed in this repo but was latent because the existing standalone test job only runs on ubuntu, where .bin and $PNPM_HOME collapse to the same directory.

Test plan

  • All existing matrix jobs in Test Action pass
  • The new Standalone Windows self-update (PATH regression) job passes
  • pr-check / check-dist passes (committed dist/index.js matches source)

Summary by CodeRabbit

  • Bug Fixes

    • Fixed PATH shadowing regression in standalone mode on Windows that could cause incorrect pnpm version to be used.
  • Tests

    • Added Windows-specific test to verify standalone mode properly handles pnpm self-updates without PATH shadowing issues.

Review Change Stack

`patchPnpmEnv` prepended `dest/node_modules/.bin` to PATH before
spawning `pnpm install` / `pnpm store prune`. On Windows in standalone
mode, `.bin/pnpm.cmd` is an npm-created shim that always points at the
BOOTSTRAP pnpm (currently 11.0.4) — the binary npm linked when it
installed `@pnpm/exe` into `node_modules`. The self-updated pnpm
written by `pnpm self-update` lives at `$PNPM_HOME/bin`, which is
separately added to PATH via `addPath()` in install-pnpm.

When the user requested a pnpm version different from the bootstrap
under `standalone: true` on Windows, patchPnpmEnv's `.bin` entry
shadowed the self-updated `$PNPM_HOME/bin` and the action's internal
`pnpm install` ran on the bootstrap. On a pnpm 11.0.x bootstrap this
broke any 11.1+ install flag (e.g. `--no-runtime`), reporting:

    ERROR  Unknown option: 'runtime'

POSIX standalone got lucky because `.bin` and `$PNPM_HOME` resolve to
the same directory there. Non-standalone never tripped on this since
the `.bin/pnpm` symlink for a regular `pnpm` package keeps working
across self-updates.

Removed `patchPnpmEnv` and the now-empty `src/utils/` module.
`spawnSync` now inherits `process.env`, whose PATH is already
correctly fronted by `$PNPM_HOME/bin` and `$PNPM_HOME` via the
`addPath` calls in install-pnpm.

Added `standalone_windows_self_update` to test.yaml as a regression
guard: standalone on Windows + target 11.1.0 + `run_install` with
`--no-runtime`. With the previous code, the install would have run
under the bootstrap (11.0.4) and errored on the unknown flag.

Originally found while building pnpm/setup (the new combined
pnpm + runtime action).
@coderabbitai

coderabbitai Bot commented May 11, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 21d16555-fa59-432a-a8ac-c4a3c0ba95d1

📥 Commits

Reviewing files that changed from the base of the PR and between 551b42e and 3ecdba8.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**
📒 Files selected for processing (4)
  • .github/workflows/test.yaml
  • src/pnpm-install/index.ts
  • src/pnpm-store-prune/index.ts
  • src/utils/index.ts
💤 Files with no reviewable changes (1)
  • src/utils/index.ts
📜 Recent review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-11T12:04:07.383Z
Learnt from: zkochan
Repo: pnpm/action-setup PR: 256
File: src/install-pnpm/run.ts:150-166
Timestamp: 2026-05-11T12:04:07.383Z
Learning: In pnpm/action-setup, when validating or forwarding the version value used for `pnpm self-update` and/or `devEngines.packageManager.version`, do not restrict it to exact versions or tags only—pnpm supports semver ranges for these inputs. Ensure any code that parses/validates `devEngines.packageManager.version` (and the value passed to `pnpm self-update`) allows range syntax such as `^`, `~`, and comparators (e.g. `>=8 <10`) instead of rejecting anything that isn’t a single exact version. Note: the plain `packageManager` field is different, so apply this “allow semver ranges” rule specifically to `devEngines.packageManager.version` / `self-update` handling.

Applied to files:

  • src/pnpm-store-prune/index.ts
  • src/pnpm-install/index.ts
🔇 Additional comments (3)
src/pnpm-store-prune/index.ts (1)

12-17: Env inheritance change for prune path handling looks correct.

This keeps the PATH ordering from earlier setup steps and cleanly removes the Windows standalone shadowing risk without changing prune error handling.

src/pnpm-install/index.ts (1)

14-24: Install spawn behavior update is solid and well-documented.

Relying on inherited process.env here is the right fix for the standalone Windows shim-shadow scenario.

.github/workflows/test.yaml (1)

197-244: Great targeted regression coverage for Windows standalone self-update.

This job exercises the exact failure mode and adds a clear post-check on the resolved pnpm version.


📝 Walkthrough

Walkthrough

The PR removes custom environment patching utilities and updates pnpm spawn calls across the action to rely on inherited process environment instead, addressing a PATH-shadow regression in Windows standalone scenarios. A new regression test validates the fix.

Changes

Environment Patching Refactor

Layer / File(s) Summary
Utility Removal
src/utils/index.ts
getBinDest function and patchPnpmEnv helper are removed, along with their supporting imports.
Install Environment Handling
src/pnpm-install/index.ts
runPnpmInstall no longer passes custom env to spawnSync; inline comments explain why environment override is intentionally omitted to avoid PATH precedence changes that could break Windows self-updated pnpm.
Store Prune Environment Handling
src/pnpm-store-prune/index.ts
pruneStore removes patchPnpmEnv import and no longer overrides environment in spawnSync; relies on inherited process environment instead.
Windows Self-Update Regression Test
.github/workflows/test.yaml
New Windows-only job standalone_windows_self_update tests the self-update scenario with standalone: true and validates that pnpm --version resolves to the pinned 11.1.0 on PATH.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • pnpm/action-setup#249: Both PRs refactor how the action handles PNPM's bin destination and environment setup, removing or rewiring getBinDest and patchPnpmEnv.

Poem

🐰 No more patching paths, let them flow free,
The process env leads where pnpm should be.
On Windows it gleams with the self-update cure,
A test guards the regress—the fix is secure! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely captures the main fix: removing patchPnpmEnv to resolve a Windows standalone mode regression where PATH shadowing prevented self-updated pnpm from being used.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-standalone-windows-path

Comment @coderabbitai help to get the list of available commands and usage tips.

@zkochan
zkochan merged commit 3e83581 into master May 11, 2026
21 checks passed
dbmikus pushed a commit to gofixpoint/amika that referenced this pull request Oct 23, 2026
Bumps [pnpm/action-setup](https://github.com/pnpm/action-setup) from 5.0.0 to 6.1.0.

## Release notes

Sourced from pnpm/action-setup's releases.

### v6.1.0

### What's Changed
- feat: support pnpm v12 by `@zkochan` in pnpm/action-setup#288

Full Changelog: pnpm/action-setup@v6.0.10...v6.1.0

### v6.0.10

### What's Changed
- docs(README): point users to the successor pnpm/setup action by `@BlackHole1` in pnpm/action-setup#282
- refactor: introduce restore keys for cache by `@SukkaW` in pnpm/action-setup#280
- ci: use pnpm 11 for `pr-check` by `@jamietanna` in pnpm/action-setup#284
- fix: update pnpm to v11.19.0 by `@jamietanna` in pnpm/action-setup#283
- docs: update README to include devEngines.packageManager by `@nemchik` in pnpm/action-setup#273

### New Contributors
- `@jamietanna` made their first contribution in pnpm/action-setup#284
- `@nemchik` made their first contribution in pnpm/action-setup#273

Full Changelog: pnpm/action-setup@v6...v6.0.10

### v6.0.9

### What's Changed
- fix: update pnpm to v11.7.0 by `@zkochan` in pnpm/action-setup#267

Full Changelog: pnpm/action-setup@v6...v6.0.9

### v6.0.8

### What's Changed
- docs(README): fix `cache_dependency_path` type by `@haines` in pnpm/action-setup#257
- fix: drop patchPnpmEnv so standalone+self-update works on Windows by `@zkochan` in pnpm/action-setup#258
- fix: update pnpm to 11.1.1 by `@mungodewar` in pnpm/action-setup#248

### New Contributors
- `@mungodewar` made their first contribution in pnpm/action-setup#248

Full Changelog: pnpm/action-setup@v6.0.7...v6.0.8

### v6.0.7

### What's Changed
- fix: honor devEngines.packageManager.onFail=error (#252) by `@zkochan` in pnpm/action-setup#254
- fix: restore inputs from state in post by `@haines` in pnpm/action-setup#255
- fix: self-update bootstrap to packageManager-pinned version (#233) by `@zkochan` in pnpm/action-setup#256

### New Contributors
- `@haines` made their first contribution in pnpm/action-setup#255

Full Changelog: pnpm/action-setup@v6.0.6...v6.0.7

... (truncated)

## Commits
- `ea17c68` feat: support pnpm v12 (#288)
- `0977fd9` docs: Update README to include devEngines.packageManager (#273)
- `48261ac` fix: update pnpm to v11.19.0 (#283)
- `75677f7` ci: use pnpm 11 for `pr-check` (#284)
- `769ae71` refactor: introduce restore keys for cache (#280)
- `6fed91f` docs(README): point users to the successor pnpm/setup action (#282)
- `0ebf471` fix: update pnpm to v11.7.0 (#267)
- `0e279bb` fix: update pnpm to 11.1.1 (#248)
- `3e83581` fix: drop patchPnpmEnv so standalone+self-update works on Windows (#258)
- `551b42e` docs(README): fix `cache_dependency_path` type (#257)
- Additional commits viewable in compare view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant