fix: respect pmOnFail ignore in self-update by marko1olo · Pull Request #12231 · pnpm/pnpm

marko1olo · 2026-06-05T21:29:36Z

syncEnvLockfile already avoids writing packageManagerDependencies when package manager checks are disabled, but pnpm self-update called resolvePackageManagerIntegrities() directly for devEngines.packageManager projects and could add the lockfile section despite pmOnFail: ignore.

This moves the lockfile persistence policy into @pnpm/config.reader so both CLI startup/switching and the self-updater use the same gate, including onFail: ignore.

Tests:

jest test/self-updater/selfUpdate.test.ts --runInBand --testNamePattern "package manager onFail is ignore" from engine/pm/commands: 1 passed
jest src/shouldPersistLockfile.test.ts --runInBand --globalSetup= --globalTeardown= --coverage=false from pnpm: 4 passed
eslint config/reader/src/index.ts pnpm/src/shouldPersistLockfile.ts pnpm/src/shouldPersistLockfile.test.ts engine/pm/commands/src/self-updater/selfUpdate.ts engine/pm/commands/test/self-updater/selfUpdate.test.ts
tsgo --build config/reader/tsconfig.json
tsgo --build engine/pm/commands/tsconfig.json
tsgo --build pnpm/tsconfig.json
git diff --check

Note: the repository pre-push hook was not used for the final push because the local Windows environment does not expose the project pnpm/pn/pnx shims globally; I ran the targeted checks above directly.

Summary by CodeRabbit

Bug Fixes
- Prevents writing packageManagerDependencies to the lockfile when package manager policy is set to ignore failures (onFail: ignore / pmOnFail: ignore).
Tests
- Added test coverage to assert lockfile behavior when package manager updates are ignored.
Chores
- Bumped package-manager-related packages and updated changelog.

Written by an agent (OpenAI Codex, GPT-5).

coderabbitai · 2026-06-05T21:29:51Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

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: 06994fbc-b64a-4e4e-b1f7-2c0816f7bac0

📥 Commits

Reviewing files that changed from the base of the PR and between 76c5d24 and 5da55bc.

📒 Files selected for processing (2)

engine/pm/commands/src/self-updater/selfUpdate.ts
pnpm/src/switchCliVersion.ts

✅ Files skipped from review due to trivial changes (1)

pnpm/src/switchCliVersion.ts

🚧 Files skipped from review as they are similar to previous changes (1)

engine/pm/commands/src/self-updater/selfUpdate.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Analyze (javascript)
GitHub Check: Compile & Lint

📝 Walkthrough

Walkthrough

This PR centralizes shouldPersistLockfile in @pnpm/config.reader, uses it to skip resolving/writing packageManagerDependencies during self-update when policy ignores failures, updates tests and changelog, and consolidates downstream imports.

Changes

Lock file persistence conditional on package manager policy

Layer / File(s)	Summary
shouldPersistLockfile contract and unit tests `config/reader/src/index.ts`, `config/reader/test/shouldPersistLockfile.test.ts`, `.changeset/clever-rocks-listen.md`	Adds exported `shouldPersistLockfile` to `@pnpm/config.reader` (returns `false` when `onFail: 'ignore'`, `true` when from `devEngines.packageManager`, otherwise requires semver major >= 12). Updates tests and changeset entry.
Self-update conditional persistence and integration test `engine/pm/commands/src/self-updater/selfUpdate.ts`, `engine/pm/commands/test/self-updater/selfUpdate.test.ts`	Imports `shouldPersistLockfile` and gates store-controller creation and `resolvePackageManagerIntegrities(...)` with the predicate when rewriting pinned `packageManager`/`devEngines.packageManager`. Adds a test verifying `packageManagerDependencies` is not written when `onFail: 'ignore'`.
Import consolidation across pnpm package `pnpm/src/switchCliVersion.ts`, `pnpm/src/syncEnvLockfile.ts`	Replaces local `./shouldPersistLockfile` imports by importing `shouldPersistLockfile` from `@pnpm/config.reader`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

pnpm/pnpm#11681: Related changes around when packageManagerDependencies is resolved/written into pnpm-lock.yaml.

Suggested reviewers

zkochan

Poem

A rabbit hops through code and cheer,
When "ignore" is set, the lock stays clear,
Self-update asks the rule, then skips the write,
Central config keeps the lockfile right,
Tiny paws keep builds polite 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.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 'fix: respect pmOnFail ignore in self-update' accurately describes the main change: fixing the self-update command to respect the pmOnFail ignore setting.
Linked Issues check	✅ Passed	The PR fully addresses all objectives from `#12228`: centralizing the persistence decision in `@pnpm/config.reader`, ensuring pmOnFail: ignore prevents writing packageManagerDependencies across all code paths, and fixing the self-updater.
Out of Scope Changes check	✅ Passed	All changes directly relate to fixing the pmOnFail ignore behavior in self-update; no unrelated modifications are present in the changeset.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.

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.}

qodo-free-for-open-source-projects · 2026-06-05T21:29:57Z

Review Summary by Qodo

Respect pmOnFail ignore in self-update lockfile persistence

🐞 Bug fix

Walkthroughs

Description

• Move shouldPersistLockfile logic to @pnpm/config.reader for consistency
• Respect pmOnFail: ignore policy in self-update lockfile persistence
• Prevent writing packageManagerDependencies when policy is ignore
• Add test coverage for ignore policy behavior

Diagram

flowchart LR
  A["shouldPersistLockfile moved<br/>to config.reader"] --> B["Self-updater uses<br/>shared gate"]
  B --> C["Respects onFail: ignore<br/>policy"]
  C --> D["No packageManagerDependencies<br/>written to lockfile"]

File Changes

1. config/reader/src/index.ts ✨ Enhancement +15/-0

Add shouldPersistLockfile function to config reader

• Added shouldPersistLockfile function to determine lockfile persistence policy
• Checks onFail: ignore first to prevent any persistence
• Handles devEngines.packageManager (always persists unless ignored)
• Handles legacy packageManager field (persists only for pnpm v12+)

config/reader/src/index.ts

2. engine/pm/commands/src/self-updater/selfUpdate.ts 🐞 Bug fix +10/-9

Gate lockfile persistence on shouldPersistLockfile check

• Import shouldPersistLockfile from @pnpm/config.reader
• Wrap resolvePackageManagerIntegrities call with shouldPersistLockfile check
• Prevents lockfile update when onFail: ignore policy is set
• Avoids unnecessary store controller creation when persistence is disabled

engine/pm/commands/src/self-updater/selfUpdate.ts

3. engine/pm/commands/test/self-updater/selfUpdate.test.ts 🧪 Tests +39/-0

Add test for onFail ignore policy in self-update
• Add test case for onFail: ignore policy behavior
• Verifies packageManagerDependencies is not written to lockfile
• Tests with devEngines.packageManager configuration
• Confirms self-update respects ignore policy
engine/pm/commands/test/self-updater/selfUpdate.test.ts

View more (3)

4. pnpm/src/shouldPersistLockfile.test.ts 🧪 Tests +3/-1

Update tests for onFail ignore policy

• Update test description to reflect ignore policy handling
• Add test case for onFail: ignore with devEngines.packageManager
• Add test case for onFail: ignore with legacy packageManager field
• Verify ignore policy takes precedence over other conditions

pnpm/src/shouldPersistLockfile.test.ts

5. pnpm/src/shouldPersistLockfile.ts Refactoring +1/-19

Re-export shouldPersistLockfile from config reader
• Remove function implementation from this file
• Re-export shouldPersistLockfile from @pnpm/config.reader
• Consolidate logic to single source of truth
pnpm/src/shouldPersistLockfile.ts

6. .changeset/clever-rocks-listen.md 📝 Documentation +7/-0

Add changeset for lockfile persistence fix

• Document patch version bump for three packages
• Reference issue #12228 in changelog
• Describe fix for packageManagerDependencies persistence with ignore policy

.changeset/clever-rocks-listen.md

zkochan · 2026-06-06T08:48:02Z

just remove this file now.

zkochan · 2026-06-06T08:48:18Z

  }
  return lockfilePinned ?? specMin
 }
-


not needed change.

marko1olo · 2026-06-07T13:17:29Z

Updated in 0750a86993 for the review comments:

removed pnpm/src/shouldPersistLockfile.ts
moved its tests to config/reader/test/shouldPersistLockfile.test.ts, next to the exported helper
changed pnpm/src/syncEnvLockfile.ts and pnpm/src/switchCliVersion.ts to import shouldPersistLockfile directly from @pnpm/config.reader
restored the unrelated whitespace-only hunk in selfUpdate.ts

Checks run locally:

jest test/shouldPersistLockfile.test.ts --runInBand --globalSetup= --globalTeardown= --coverage=false
jest test/self-updater/selfUpdate.test.ts --runInBand --testNamePattern "package manager onFail is ignore|resolved version still satisfies" --globalSetup= --globalTeardown= --coverage=false
jest src/syncEnvLockfile.test.ts --runInBand --globalSetup= --globalTeardown= --coverage=false
tsgo --build config/reader/tsconfig.json
tsgo --build engine/pm/commands/tsconfig.json
tsgo --build pnpm/tsconfig.json
eslint config/reader/test/shouldPersistLockfile.test.ts pnpm/src/syncEnvLockfile.ts pnpm/src/switchCliVersion.ts
git diff --check

The local pre-push hook itself could not run verbatim because this environment has no global pnpm/pnx shim, but I ran the relevant TypeScript builds and lint checks directly with the local toolchain. No pacquet/ or pnpr/ files changed, so the Rust pre-push path is not applicable.

marko1olo requested a review from zkochan as a code owner June 5, 2026 21:29

coderabbitai Bot approved these changes Jun 5, 2026

View reviewed changes

zkochan reviewed Jun 6, 2026

View reviewed changes

Comment thread pnpm/src/shouldPersistLockfile.ts Outdated

zkochan Jun 6, 2026

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.

just remove this file now.

zkochan reviewed Jun 6, 2026

View reviewed changes

marko1olo force-pushed the fix-pm-onfail-ignore-lockfile branch from acefdb4 to 0750a86 Compare June 7, 2026 13:17

marko1olo added 2 commits June 7, 2026 23:34

fix: respect pmOnFail ignore in self-update

d7b0de1

fix: preserve devEngines lockfile writes

76c5d24

zkochan force-pushed the fix-pm-onfail-ignore-lockfile branch from 0750a86 to 76c5d24 Compare June 7, 2026 21:38

fix: restore unrelated whitespace hunks

5da55bc

zkochan approved these changes Jun 7, 2026

View reviewed changes

zkochan merged commit 3537020 into pnpm:main Jun 7, 2026
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: respect pmOnFail ignore in self-update#12231

fix: respect pmOnFail ignore in self-update#12231
zkochan merged 3 commits into
pnpm:mainfrom
marko1olo:fix-pm-onfail-ignore-lockfile

marko1olo commented Jun 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026

Uh oh!

zkochan Jun 6, 2026

Uh oh!

zkochan Jun 6, 2026

Uh oh!

marko1olo commented Jun 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

marko1olo commented Jun 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 5, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

zkochan Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

zkochan Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

marko1olo commented Jun 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marko1olo commented Jun 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading