Conversation
When package.json pins pnpm via `packageManager` or an exact `devEngines.packageManager.version`, self-update the bootstrap up front. The bootstrap's `pnpm store path` skips pnpm's auto-switch (the `store` command sets `skipPackageManagerCheck = true`), so it reports the bootstrap's `STORE_VERSION` while the user's actual install runs under the pinned version and writes to a different STORE_VERSION — breaking `cache: true` and `setup-node`'s `cache: pnpm` on cold caches.
📝 WalkthroughWalkthroughreadTargetVersion now normalizes packageManager (strip Changespnpm Cache Path Validation Fix
Sequence DiagramsequenceDiagram
participant PackageJSON as package.json
participant readTargetVersion as readTargetVersion()
participant runSelfInstaller as runSelfInstaller()
participant FileSystem as FileSystem
participant Cache as pnpm store
PackageJSON->>readTargetVersion: packageManager / devEngines.packageManager
readTargetVersion-->>runSelfInstaller: resolved target version (string) or throws
runSelfInstaller->>runSelfInstaller: pnpm self-update <target>
runSelfInstaller->>FileSystem: check PNPM_HOME/bin exists
alt PNPM_HOME/bin exists
FileSystem-->>runSelfInstaller: exists
runSelfInstaller-->>Cache: set binDest -> PNPM_HOME/bin
else PNPM_HOME/bin missing
FileSystem-->>runSelfInstaller: missing
runSelfInstaller-->>Cache: fallback binDest -> pnpmHome
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/test.yaml (1)
209-216: ⚡ Quick winConsider adding a test case for
packageManagerwith integrity hash suffix.The fix includes stripping the integrity hash from
packageManagervalues (e.g.,[email protected]+sha512.abc...). This scenario from issue#233isn't covered in thecache_store_pathmatrix. Adding a test case would help guard against regressions in the hash-stripping logic.Suggested additional matrix entry
include: - label: 'packageManager [email protected]' manifest: '{"packageManager":"[email protected]","dependencies":{"is-odd":"3.0.1"}}' + - label: 'packageManager [email protected] with integrity hash' + manifest: '{"packageManager":"[email protected]+sha512.1234567890abcdef","dependencies":{"is-odd":"3.0.1"}}' - label: 'devEngines exact [email protected]' manifest: '{"devEngines":{"packageManager":{"name":"pnpm","version":"10.33.0"}},"dependencies":{"is-odd":"3.0.1"}}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml around lines 209 - 216, Add a matrix test entry that covers packageManager values with an integrity-hash suffix so the cache path hashing/strip logic is exercised; specifically add an include item in the strategy.matrix (alongside the existing 'packageManager [email protected]' and 'devEngines exact [email protected]') with a manifest where packageManager is set to a value containing a suffix like "[email protected]+sha512.<hash>" and dependencies similar to the other entries, and optionally a matching devEngines variant to validate the stripping logic in both packageManager and devEngines handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/test.yaml:
- Around line 209-216: Add a matrix test entry that covers packageManager values
with an integrity-hash suffix so the cache path hashing/strip logic is
exercised; specifically add an include item in the strategy.matrix (alongside
the existing 'packageManager [email protected]' and 'devEngines exact [email protected]')
with a manifest where packageManager is set to a value containing a suffix like
"[email protected]+sha512.<hash>" and dependencies similar to the other entries, and
optionally a matching devEngines variant to validate the stripping logic in both
packageManager and devEngines handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8971dc09-f3e9-4d12-ba3a-7b4ff2d5d83f
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
.github/workflows/test.yamlsrc/install-pnpm/run.ts
📜 Review details
🔇 Additional comments (8)
src/install-pnpm/run.ts (7)
101-106: LGTM!The filesystem check to determine actual binary location handles the no-op self-update edge case correctly. When the target version equals the bootstrap version,
pnpm self-updateis a no-op and won't createPNPM_HOME/bin/pnpm, so falling back topnpmHomeis the right behavior.
109-116: LGTM!The expanded comment clearly explains the rationale for setting
pmOnFail=downloadonly in this branch, and the scoping decision to preserve strictonFailbehavior when users pass an explicitversion:input.
144-149: LGTM!The integrity hash stripping logic correctly handles both
pnpm@<version>andpnpm@<version>+<integrity>formats using.split('+')[0].
151-160: LGTM!The comparison now correctly uses the normalized
packageManagerVersion(without integrity hash) against theversioninput, preventing false mismatches like10.33.0vs10.33.0+sha512....
162-179: LGTM!The devEngines priority logic correctly matches pnpm's
getWantedPackageManagerbehavior. The early return ofundefinedfor ranges (lines 177-178) ensures the bootstrap auto-switch path is used, sincepnpm self-updaterequires a specific version.
181-183: LGTM!The fallback to
packageManagerVersion(already stripped of integrity hash) correctly completes the priority chain: devEngines → packageManager → error.
199-202: LGTM!The regex correctly identifies exact semver versions (including optional prerelease and build metadata) while rejecting range specifiers like
>=,^,~, and wildcards. This enables the intended behavior of triggering self-update only for exact pins..github/workflows/test.yaml (1)
232-241: LGTM!The test correctly validates the fix for
#233by verifying that the store path reported bypnpm store path --silentactually exists on disk. This directly guards against the "Path Validation Error" that occurs when the cached path doesn't exist.
On Windows pnpm self-update writes `.bin/bin/pnpm` (a JS launcher), not `.bin/bin/pnpm.exe`, so the previous existsSync probe always fell back to pnpmHome and the bin_dest output pointed at the bootstrap pnpm. Check whether the `bin` directory itself exists.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install-pnpm/run.ts (1)
151-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the same precedence for mismatch validation and target selection.
Line 152 still validates
version:only againstpackageManager, but Lines 173-183 now treat exactdevEngines.packageManager.versionas the preferred manifest pin. That meansversion:can be rejected even when it matches the version this function would otherwise select. Please compare against the effective manifest target instead ofpackageManagerVersionalone.Proposed fix
const packageManagerVersion = typeof packageManager === 'string' && packageManager.startsWith('pnpm@') ? packageManager.slice('pnpm@'.length).split('+')[0] : undefined + + const effectiveManifestVersion = + devEngines?.packageManager?.name === 'pnpm' + ? (devEngines.packageManager.version != null && isExactSemver(devEngines.packageManager.version) + ? devEngines.packageManager.version + : undefined) + : packageManagerVersion if (version) { - if (packageManagerVersion && packageManagerVersion !== version) { + if (effectiveManifestVersion && effectiveManifestVersion !== version) { throw new Error(`Multiple versions of pnpm specified: - version ${version} in the GitHub Action config with the key "version" - version ${packageManager} in the package.json with the key "packageManager" Remove one of these versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`) } return version } - if (devEngines?.packageManager?.name === 'pnpm') { - const v = devEngines.packageManager.version - if (v != null && isExactSemver(v)) { - return v - } - return undefined - } - - if (packageManagerVersion) { - return packageManagerVersion - } + if (devEngines?.packageManager?.name === 'pnpm') return effectiveManifestVersion + if (effectiveManifestVersion) return effectiveManifestVersion🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install-pnpm/run.ts` around lines 151 - 183, The validation currently rejects the Action `version` by comparing it only to `packageManagerVersion`, but the selection logic later prefers an exact `devEngines.packageManager.version` (checked via `devEngines?.packageManager?.version` and `isExactSemver`) over `packageManagerVersion`; update the mismatch check so it compares `version` against the effective manifest target (the value you would return from the manifest logic: prefer exact `devEngines.packageManager.version` when present, otherwise `packageManagerVersion`) instead of `packageManagerVersion` alone—compute that effective target once (reuse the same logic used in the later return path) and use it in the `if (packageManagerVersion && packageManagerVersion !== version)` validation (or replace that check) to ensure consistent precedence between validation and selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/install-pnpm/run.ts`:
- Around line 151-183: The validation currently rejects the Action `version` by
comparing it only to `packageManagerVersion`, but the selection logic later
prefers an exact `devEngines.packageManager.version` (checked via
`devEngines?.packageManager?.version` and `isExactSemver`) over
`packageManagerVersion`; update the mismatch check so it compares `version`
against the effective manifest target (the value you would return from the
manifest logic: prefer exact `devEngines.packageManager.version` when present,
otherwise `packageManagerVersion`) instead of `packageManagerVersion`
alone—compute that effective target once (reuse the same logic used in the later
return path) and use it in the `if (packageManagerVersion &&
packageManagerVersion !== version)` validation (or replace that check) to ensure
consistent precedence between validation and selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a99954b8-d39d-4ddc-8924-9dccbaabc0a7
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/install-pnpm/run.ts
`pnpm self-update` resolves semver ranges to a specific version, so `devEngines.packageManager.version: ">=10 <11"` can also go through the self-update path. That makes readTargetVersion total — it always returns a string or throws — so the runtime auto-switch fallback (and the `pnpm_config_pm_on_fail=download` export from #252) is no longer reachable and gets removed. Adds a range case to the cache_store_path matrix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install-pnpm/run.ts`:
- Around line 150-166: The code returns devEngines.packageManager.version
directly (and falls back to packageManagerVersion) and the comment incorrectly
claims "pnpm self-update" accepts semver ranges; you must either resolve any
semver range to an exact version or validate and fail early. Update the logic
around reading devEngines.packageManager.version and packageManagerVersion (the
value passed to "pnpm self-update") to detect range expressions (using semver
utilities) and if a range is provided resolve it to a specific version (e.g., by
querying the registry or using semver.maxSatisfying against available tags) or
throw a clear error indicating an exact version is required; also update/remove
the incorrect comment about range support for "pnpm self-update".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9fbcce0-72a5-4e2d-b5f3-c56c0fa0853f
⛔ Files ignored due to path filters (1)
dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
.github/workflows/test.yamlsrc/install-pnpm/run.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yaml
📜 Review details
🔇 Additional comments (3)
src/install-pnpm/run.ts (3)
87-105: LGTM!The self-update logic correctly handles both the successful update case (returning
PNPM_HOME/bin) and the no-op case where target version equals bootstrap version (falling back topnpmHome). The comments clearly explain why this fallback is necessary.
132-137: LGTM!The integrity hash stripping logic correctly handles the
pnpm@<version>+<integrity>format. The.split('+')[0]approach safely handles both cases: versions with integrity hashes and plain versions without them.
139-148: LGTM!The version mismatch check now correctly compares the stripped
packageManagerVersionagainst the action input, preventing false positives from integrity hash suffixes. The error message helpfully shows the originalpackageManagervalue for debugging.
The v6.0.0–v6.0.6 self-update bootstrap bug (pnpm/action-setup#256) that caused ERR_PNPM_BROKEN_LOCKFILE when `packageManager` is set in package.json has been fixed upstream in v6.0.7 and polished in v6.0.8 (released 2026-05-12). Replace the v4.4.0 workaround with the real fix. Ref: pnpm/action-setup#256 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
1 participant
Summary
package.jsonpins pnpm viapackageManagerordevEngines.packageManager.version(exact or range), self-update the bootstrap pnpm to that version up front instead of deferring to pnpm's runtime auto-switch.pnpm store path(thestorecommand setsskipPackageManagerCheck = truein pnpm 11.0.x) doesn't trigger the auto-switch, so the bootstrap reports its ownSTORE_VERSIONwhile the user's actualpnpm installruns under the pinned version and writes to a differentSTORE_VERSION. That mismatch is what makescache: true(andactions/setup-node@v6'scache: pnpm) fail on a cold cache withPath Validation Error: Path(s) specified in the action for caching do(es) not exist.pnpm self-updateaccepts both exact versions and semver ranges, so we pass either through directly. That makesreadTargetVersiontotal — it always returns a string or throws — so the runtime auto-switch fallback and thepnpm_config_pm_on_fail=downloadexport added for Action fails with devEngines.packageManager.onFail="error" when version input is omitted #252 are no longer reachable and get removed.pnpm self-updateis a no-op (the pinned version resolves to the bootstrap version),PNPM_HOME/binis not created — fall back topnpmHomeforbinDest. Also strip the integrity hash frompackageManager: pnpm@<ver>+<sha>before comparing againstversion:input (pre-existing false-mismatch).Fixes #233.
Test plan
cache_store_pathmatrix intest.yamlpins pnpm 10 viapackageManager, exactdevEngines, and a>=10 <11range, runs the action withcache: true+run_install, then assertspnpm store pathresolves to a directory that exists on disk — fails onmaster, passes on this branch.Manifest pinmatrix still passes (exact pins now go through self-update; range pins also go through self-update and resolve to whatever satisfies the range, so the>= 9.15.0assertions still hold).SmokeandStandalonematrices still pass (explicitversion:input path is unchanged).Summary by CodeRabbit
Tests
Bug Fixes