fix(config): apply pmOnFail default to devEngines.packageManager (singular) by shiminshen · Pull Request #11682 · pnpm/pnpm

shiminshen · 2026-05-16T05:17:22Z

Summary

The pnpm v11 release notes migration table documents the pmOnFail default as download:

Removed setting	Replace with
`managePackageManagerVersions: true`	`pmOnFail: download` (default)
...	...

The legacy packageManager field already gets that default applied at the central onFail-resolution site in getConfig — but the singular form of devEngines.packageManager short-circuited it by setting onFail = 'error' inside parseDevEnginesPackageManager. So a project that pinned a different pnpm via devEngines.packageManager and ran pnpm install from a mismatched pnpm saw a hard version mismatch error instead of the auto-download the docs promise.

Reproduction (from the issue):

git clone --branch switch-from-yarn-v1-to-pnpm-v11 https://github.com/sql-formatter-org/sql-formatter.git
cd sql-formatter
pnpm install    # pre-fix: ERROR "This project is configured to use ... of pnpm"

After this PR, the same install resolves to onFail: 'download' and switches to the wanted version, matching pnpm install --pm-on-fail=download.

The change

parseDevEnginesPackageManager (singular case) no longer applies a local ?? 'error' default. The local onFail is left undefined when the user didn't set it, so the central pmOnFail default at getConfig line ~666 picks up 'download'.

The array form of devEngines.packageManager keeps its existing per-element defaults ('error' for the last entry, 'ignore' for the rest). Those reflect explicit prioritization by the user (alternatives list), not a system-wide fallback, so they should stay.

Explicit onFail values continue to win in all cases.

Behavior change

This is technically a behavior change: previously devEngines.packageManager: { name: 'pnpm', version: '<other>' } (no onFail) errored out; now it switches versions. That matches the documented contract. The pre-existing test devEngines.packageManager defaults to onFail=error cemented the buggy behavior — it has been retargeted to assert the new default (onFail=download) using the corepack-fallthrough pattern from the adjacent onFail=download surfaces a regular error under corepack test, so the assertion stays fast and deterministic without a real download round-trip.

Test plan

New unit tests in config/reader/test/index.ts:
- devEngines.packageManager without onFail resolves to the documented pmOnFail default "download" (#11676)
- devEngines.packageManager with explicit onFail is respected (regression guard for #11676)
Updated e2e test devEngines.packageManager defaults to onFail=download (#11676) in pnpm/test/packageManagerCheck.test.ts — asserts the corepack-fallthrough behavior that confirms the resolved default is download.
Verified end-to-end against the compiled bundle with a standalone script: wantedPackageManager.onFail resolves to download when not set, stays as error when explicitly set.

Local Jest still fails for me with module is already linked under experimental-vm-modules on Node 22/24 (env issue, reproduces on pristine main) — relying on CI for the real test run.

pacquet parity note

Config-reader change. Pacquet currently install-only and does not consume devEngines.packageManager defaults yet. Flagging per CLAUDE.md so the parity shows up on the radar if/when pacquet grows this surface.

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

Release Notes

Bug Fixes
- Fixed package manager configuration to properly default to download behavior when onFail is not explicitly specified.
Documentation
- Updated documentation to clarify precedence rules for package manager failure handling.
Tests
- Added regression tests to verify correct default behavior.

…gular) The pnpm v11 release notes document the `pmOnFail` default as `download` (via the migration table that maps `managePackageManagerVersions: true` → `pmOnFail: download (default)`). The legacy `packageManager` field already gets that default applied at the central onFail-resolution site, but the singular form of `devEngines.packageManager` short-circuited it by setting `onFail = 'error'` inside `parseDevEnginesPackageManager`, so projects that pinned a different pnpm via `devEngines.packageManager` saw a hard version mismatch instead of an auto-download. Drop that local `?? 'error'` and let the central default apply. The array form of `devEngines.packageManager` keeps its own per-element defaults ('error' for the last entry, 'ignore' for the rest) — those reflect explicit prioritisation by the user, not a system-wide fallback. Explicit `onFail` values are still honored everywhere. Closes pnpm#11676.

cspell flagged the British spelling at pre-push.

coderabbitai · 2026-05-16T05:17:35Z

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: c69b0cf2-0d8a-4026-8013-f5012c9c14fc

📥 Commits

Reviewing files that changed from the base of the PR and between 496e655 and 6d23eb6.

📒 Files selected for processing (4)

.changeset/pmonfail-default-devengines-11676.md
config/reader/src/index.ts
config/reader/test/index.ts
pnpm/test/packageManagerCheck.test.ts

📜 Recent review details

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.

Files:

config/reader/test/index.ts
pnpm/test/packageManagerCheck.test.ts
config/reader/src/index.ts

**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use instanceof Error for error type checking in Jest tests. Use util.types.isNativeError() instead, as Jest runs tests in a VM context where instanceof checks can fail across realms.

Files:

pnpm/test/packageManagerCheck.test.ts

🧠 Learnings (1)

📚 Learning: 2026-05-14T09:04:00.133Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.

Applied to files:

config/reader/test/index.ts
pnpm/test/packageManagerCheck.test.ts
config/reader/src/index.ts

🔇 Additional comments (4)

.changeset/pmonfail-default-devengines-11676.md (1)

1-8: LGTM!

config/reader/src/index.ts (1)

660-664: LGTM!

Also applies to: 769-769, 786-790

config/reader/test/index.ts (1)

169-208: LGTM!

pnpm/test/packageManagerCheck.test.ts (1)

146-146: LGTM!

Also applies to: 156-164, 168-168

📝 Walkthrough

Walkthrough

Fixes the devEngines.packageManager.onFail defaulting logic to apply the documented central pmOnFail: "download" default instead of prematurely defaulting to "error". Includes updated documentation, config parsing logic changes, and regression tests at unit and integration levels to validate correct defaulting precedence.

Changes

pmOnFail Defaulting Precedence Fix

Layer / File(s)	Summary
Changeset and documentation updates `.changeset/pmonfail-default-devengines-11676.md`, `config/reader/src/index.ts`	Changeset documents the fix for `devEngines.packageManager` defaulting behavior; updated comments in `getConfig` clarify singular vs. array form defaulting logic and how `pmOnFail: "download"` is applied.
Config parsing logic `config/reader/src/index.ts`	`parseDevEnginesPackageManager` now permits `undefined` for the `onFail` variable and removes premature defaulting to `'error'` for singular `devEngines.packageManager`, allowing later config logic to apply the central `pmOnFail` default.
Unit tests `config/reader/test/index.ts`	Two regression tests validate that `devEngines.packageManager.onFail` defaults to `"download"` when omitted and that explicit `"error"` values are preserved.
Integration test update `pnpm/test/packageManagerCheck.test.ts`	Test description and assertions updated to reflect new `onFail=download` default; `COREPACK_ROOT` environment variable isolates the test and adds corepack-specific behavior assertion.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

#11674: The change to devEngines.packageManager handling affects package manager dependency recording/cleanup in the lockfile, which is the focus of that issue.

Suggested reviewers

zkochan

Poem

A rabbit hops through config land,
Where defaults now play fair and grand—
No more premature "error" claims,
download wins the defaulting games! 🐰✨

🚥 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 summarizes the main change: applying the pmOnFail default to the singular form of devEngines.packageManager.
Linked Issues check	✅ Passed	The PR directly addresses `#11676` by fixing the bug where pmOnFail='download' default was not applied to devEngines.packageManager singular form, with comprehensive test coverage added.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the pmOnFail default behavior: config reader logic, documentation updates, and targeted regression tests.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

karlhorky · 2026-05-17T12:22:22Z

@zkochan what do you think about this change?

From a quick look, it seems like an ok approach to leave pmOnFail to be undefined and allow it to fall back to the default 'download'.

welcome · 2026-05-17T12:48:02Z

Congrats on merging your first pull request! 🎉🎉🎉

damonshen17 added 2 commits May 16, 2026 13:15

chore: fix spelling (prioritisation → prioritization)

6d23eb6

cspell flagged the British spelling at pre-push.

shiminshen requested a review from zkochan as a code owner May 16, 2026 05:17

coderabbitai Bot approved these changes May 16, 2026

View reviewed changes

zkochan approved these changes May 17, 2026

View reviewed changes

zkochan merged commit ba2c884 into pnpm:main May 17, 2026
9 of 10 checks passed

zkochan mentioned this pull request May 17, 2026

fix: write packageManagerDependencies when devEngines.packageManager is set without onFail #11681

Merged

4 tasks

coderabbitai Bot mentioned this pull request May 21, 2026

fix: validate devEngines runtime onFail #11822

Merged

coderabbitai Bot mentioned this pull request Jun 5, 2026

fix: respect pmOnFail ignore in self-update #12231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(config): apply pmOnFail default to devEngines.packageManager (singular)#11682

fix(config): apply pmOnFail default to devEngines.packageManager (singular)#11682
zkochan merged 2 commits into
pnpm:mainfrom
shiminshen:fix/pmOnFail-default-11676

shiminshen commented May 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 16, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

karlhorky commented May 17, 2026

Uh oh!

Uh oh!

welcome Bot commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

shiminshen commented May 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The change

Behavior change

Test plan

pacquet parity note

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

karlhorky commented May 17, 2026

Uh oh!

Uh oh!

welcome Bot commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shiminshen commented May 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 16, 2026 •

edited

Loading