fix: pnpm self-update binary shadowed by bootstrap on PATH#230
fix: pnpm self-update binary shadowed by bootstrap on PATH#230zkochan merged 3 commits intopnpm:masterfrom
Conversation
The bootstrap pnpm binary at PNPM_HOME shadows the self-updated binary at PNPM_HOME/bin because addPath(pnpmHome) was called after addPath(pnpmHome/bin), giving the bootstrap higher PATH precedence. Swap the addPath order so the self-updated binary takes priority. The bootstrap is invoked via absolute path, so this doesn't affect the bootstrap step. Also extract the version from the packageManager field and pass it to self-update explicitly, instead of returning undefined and relying on the bootstrap's auto-switching which can modify the lockfile. Fixes pnpm#225, fixes pnpm#227, fixes pnpm#228
|
Please address this asap 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR fixes pnpm/action-setup v6 using the bootstrap pnpm binary instead of the requested pnpm version, ensuring pnpm self-update results are honored (including when the requested version comes from packageManager in package.json).
Changes:
- Adjust PATH setup ordering so the self-updated pnpm binary location takes precedence over the bootstrap binary location.
- Parse
packageManager: "pnpm@<version>"to extract an explicit target version forpnpm self-update.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (typeof packageManager === 'string' && packageManager.startsWith('pnpm@')) { | ||
| return undefined | ||
| return packageManager.replace('pnpm@', '').split('+')[0] | ||
| } |
There was a problem hiding this comment.
readTargetVersion() now derives the version via packageManager.replace('pnpm@', '').split('+')[0]. For malformed values like "pnpm@" or "pnpm@+sha512...", this returns an empty string, which then causes runSelfInstaller() to skip self-update (because if (targetVersion) is falsey) and succeed using the bootstrap pnpm. Consider validating the extracted version (e.g., match ^pnpm@([^+]+) and ensure it’s non-empty after trimming) and throwing a clear error when the packageManager field is present but doesn’t contain a usable version.
Keep only the PATH ordering fix from the previous commit. The packageManager field handling is a separate concern and needs more discussion before being bundled with this fix.
zkochan
changed the title
|
I will merge the fix related to PATH. I am not convinced the other change is needed. pnpm should be able to automatically switch versions. |
The previous commit's dist/index.js was built with a stale node_modules and didn't match a reproducible build, breaking the check-dist CI job.
The existing version tests only check output format via regex, which is why the PATH-shadowing bug (#230) slipped through — the bootstrap pnpm's version string matched the regex just as well as the requested version. - test_version_respects_request: runs the action with `version: 9.15.5` and `version: 10.33.0` (both differ from the bootstrap) and asserts that `pnpm --version` matches exactly. Regression test for #225/#230. - test_package_manager_field: writes a `packageManager: pnpm@<v>` entry into package.json, runs the action with no `version:` input, and asserts exact match. Reproduces #227; currently expected to fail since `packageManager` extraction was intentionally not added.
Fixes #229
Problem
pnpm self-updateinstalls the target version toPNPM_HOME/bin/pnpm, but the bootstrap binary atPNPM_HOME/pnpmhas higher PATH precedence becauseaddPath(pnpmHome)was called afteraddPath(pnpmHome/bin).@actions/core'saddPathprepends, so the later call wins — the bootstrap version shadows the self-updated binary.Fix
Swap the
addPathcall order soPNPM_HOME/bin(whereself-updateputs the target binary) has higher PATH precedence. The bootstrap pnpm is invoked via absolute path, so this doesn't affect the bootstrap step.Verification
Reproduced locally by simulating the action flow:
version: 10