fix(install): persist revoked builds to .modules.yaml (#12221) by aqeelat · Pull Request #12224 · pnpm/pnpm

aqeelat · 2026-06-05T15:21:19Z

Summary

Fix pnpm approve-builds reporting "no packages awaiting approval" when a build-script dependency whose approval was revoked (e.g. after git stash drops allowBuilds from pnpm-workspace.yaml) is re-added.

Root cause

The revocation detection in mutateModules populates ignoredBuilds in memory, but the install path's writeModulesManifest had already run, so .modules.yaml never recorded the revoked packages. pnpm approve-builds reads .modules.yaml to find pending approvals, finds nothing, and reports "no packages awaiting approval".

Fix

After the revocation scan, re-read the manifest from disk and write back the updated ignoredBuilds. This only touches the ignoredBuilds field so the install's other state (hoistedDependencies, pendingBuilds, etc.) is preserved.

Test

Added a regression test in pnpm/test/install/lifecycleScripts.ts that simulates the stash scenario: install a build-script package, approve it, remove the config files while keeping node_modules, re-add the package, then verify approve-builds still works. The test fails without the fix and passes with it.

Fixes #12221

Summary by CodeRabbit

Bug Fixes
- Fixed approve-builds to correctly handle revoked build-script approvals when a dependency is stashed/removed and later re-added, so it no longer incorrectly reports “no packages awaiting approval.”
- Improved install post-processing to properly reconcile ignoredBuilds and persist the corrected ignored/revoked state in .modules.yaml.
Tests
- Added an end-to-end test covering deleting and recreating workspace/lock files, then re-running approve-builds --all to confirm approvals are reapplied.
Chores
- Added a changeset documenting the patch fix.

coderabbitai · 2026-06-05T15:21:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 597dc245-bc70-410a-bf36-2770263626c6

📥 Commits

Reviewing files that changed from the base of the PR and between 9178e3f and c6ed9f5.

📒 Files selected for processing (1)

.changeset/fix-approve-builds-stash-revocation.md

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

.changeset/fix-approve-builds-stash-revocation.md

📝 Walkthrough

Walkthrough

Detect revoked build approvals during install post-processing, persist the updated ignoredBuilds into the on-disk .modules.yaml when revocations occur, and add an end-to-end test plus a changeset documenting the fix.

Changes

Revoked builds detection and state persistence

Layer / File(s)	Summary
Modules manifest read and revoked-builds persistence `installing/deps-installer/src/install/index.ts`	Imports `readModulesManifest`, refactors `ignoredBuilds` initialization from install results, detects `revokedBuilds`, and when revocations occur (and modules-dir updates are allowed) re-reads the on-disk `.modules.yaml` and writes back the merged `ignoredBuilds` to avoid clobbering earlier manifest fields.
End-to-end test for approve-builds after stash `pnpm/test/install/lifecycleScripts.ts`	Adds a Jest test exercising the stash/re-add workflow: add ignored dependency, approve builds, remove workspace state, re-add dependency, assert `modulesManifest.ignoredBuilds` contains it (using `parse`), and verify `approve-builds --all` succeeds without the false "no packages awaiting approval" message.
Changeset documenting the revocation fix `.changeset/fix-approve-builds-stash-revocation.md`	Documents the bugfix ensuring revoked packages are recorded in `.modules.yaml` so `approve-builds` can detect them on subsequent runs (references `#12221`).

Sequence Diagram(s)

sequenceDiagram
  participant Installer
  participant ModulesManifest
  participant ApproveCLI
  Installer->>ModulesManifest: read on-disk .modules.yaml (readModulesManifest)
  Installer->>Installer: compute ignoredBuildsFromInstall, detect revokedBuilds
  Installer->>ModulesManifest: write merged ignoredBuilds to .modules.yaml (writeModulesManifest)
  ApproveCLI->>ModulesManifest: read ignoredBuilds for approval (getAutomaticallyIgnoredBuilds)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

pnpm/pnpm#11366: Overlapping updates to installing/deps-installer/src/install/index.ts and ignored-builds handling that touch similar .modules.yaml preservation logic.

Suggested labels

area: cli/dlx

Suggested reviewers

zkochan

Poem

🐰 I hopped through manifests, small and spry,
Found ignored builds that slipped on by,
I read the YAML, merged what was lost,
Restored approvals without a cost,
Now approve-builds sings — hop, celebrate!

🚥 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 summarizes the main change: persisting revoked builds to .modules.yaml, which is the core fix for issue `#12221`.
Linked Issues check	✅ Passed	The code changes fully address issue `#12221`: revoked build approvals are now correctly persisted in .modules.yaml so approve-builds can detect them on re-adds.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the revoked builds persistence issue; no unrelated modifications detected.

_{✏️ 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.}

qodo-free-for-open-source-projects · 2026-06-05T15:21:38Z

Review Summary by Qodo

Persist revoked builds to .modules.yaml for approve-builds

🐞 Bug fix

Walkthroughs

Description

• Persist revoked build approvals to .modules.yaml after revocation scan
• Re-read manifest from disk and merge revoked builds before writing
• Prevents loss of revocation state when approval config is removed
• Added regression test simulating git stash scenario with re-added dependencies

Diagram

flowchart LR
  A["Install runs<br/>writeModulesManifest"] --> B["Revocation scan<br/>detects revoked builds"]
  B --> C["Re-read manifest<br/>from disk"]
  C --> D["Merge revoked builds<br/>with on-disk state"]
  D --> E["Write updated<br/>.modules.yaml"]
  E --> F["approve-builds finds<br/>pending approvals"]

File Changes

1. installing/deps-installer/src/install/index.ts 🐞 Bug fix +24/-2

Re-read and persist revoked builds to manifest

• Import readModulesManifest and StrictModules from modules-yaml package
• Track revoked builds with revokedBuilds flag during revocation scan
• After revocation detection, re-read .modules.yaml from disk if revokes occurred
• Merge revoked builds with on-disk ignoredBuilds set and write back manifest

installing/deps-installer/src/install/index.ts

2. pnpm/test/install/lifecycleScripts.ts 🧪 Tests +35/-0

Add regression test for stash revocation scenario

• Add regression test for issue #12221 simulating git stash scenario
• Test installs package, approves builds, removes config files, re-adds package
• Verify revoked builds are recorded in .modules.yaml
• Confirm approve-builds successfully finds pending approvals after re-add

pnpm/test/install/lifecycleScripts.ts

3. .changeset/fix-approve-builds-stash-revocation.md 📝 Documentation +5/-0

Changelog entry for revocation fix
• Document fix for approve-builds reporting false "no packages awaiting approval"
• Explain revocation scenario when allowBuilds config is dropped
• Reference issue #12221
.changeset/fix-approve-builds-stash-revocation.md

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes an edge case where build-script approvals can become “revoked” (e.g. after stashing workspace config) and then not be detected by pnpm approve-builds, by ensuring revoked packages are correctly persisted in .modules.yaml.

Changes:

Added regression test covering approve-builds after stashing and re-adding a build-script dependency.
Updated install flow to detect revoked build approvals and update .modules.yaml accordingly.
Added a changeset describing the patch fix and linking the reported issue.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
pnpm/test/install/lifecycleScripts.ts	Adds regression test for the stash/re-add approve-builds scenario.
installing/deps-installer/src/install/index.ts	Re-reads and updates `.modules.yaml` when approvals are revoked post-install.
.changeset/fix-approve-builds-stash-revocation.md	Documents the fix as a patch changeset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

aqeelat · 2026-06-06T14:24:44Z

@coderabbitai full review

coderabbitai · 2026-06-06T14:24:50Z

✅ Action performed

Full review finished.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

After a build-script dependency whose approval was revoked (e.g. via git stash dropping allowBuilds from pnpm-workspace.yaml) is re-added, the revocation detection populated ignoredBuilds in memory but the install path's writeModulesManifest had already run, so .modules.yaml never recorded the revoked packages. pnpm approve-builds then read an empty ignoredBuilds and reported 'no packages awaiting approval'. Re-read the manifest from disk after the revocation scan and write back the updated ignoredBuilds, merging with any entries the install path captured.

- Inline dead ignoredBuildsFromInstall indirection - Drop unsafe allowBuilds cast in the regression test

qodo-free-for-open-source-projects · 2026-06-16T07:59:55Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Copilot AI review requested due to automatic review settings June 5, 2026 15:21

aqeelat requested a review from zkochan as a code owner June 5, 2026 15:21

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Comment thread installing/deps-installer/src/install/index.ts Outdated

Comment thread installing/deps-installer/src/install/index.ts Outdated

Comment thread pnpm/test/install/lifecycleScripts.ts Outdated

coderabbitai Bot approved these changes Jun 5, 2026

View reviewed changes

aqeelat force-pushed the fix/approve-builds-stash-repro branch from 7aad289 to b6e6a69 Compare June 5, 2026 16:32

aqeelat requested a review from Copilot June 6, 2026 12:04

Copilot AI reviewed Jun 6, 2026

View reviewed changes

aqeelat force-pushed the fix/approve-builds-stash-repro branch from b6e6a69 to 9178e3f Compare June 6, 2026 12:23

aqeelat mentioned this pull request Jun 6, 2026

Investigate: can runUnignoredDependencyBuilds leave stale ignoredBuilds in .modules.yaml? #12244

Open

aqeelat requested a review from Copilot June 6, 2026 14:24

Copilot AI reviewed Jun 6, 2026

View reviewed changes

Comment thread pnpm/test/install/lifecycleScripts.ts Outdated

Comment thread installing/deps-installer/src/install/index.ts Outdated

Comment thread installing/deps-installer/src/install/index.ts

aqeelat and others added 2 commits June 16, 2026 09:47

refactor(install): address review comments

c6ed9f5

- Inline dead ignoredBuildsFromInstall indirection - Drop unsafe allowBuilds cast in the regression test

zkochan force-pushed the fix/approve-builds-stash-repro branch from 9178e3f to c6ed9f5 Compare June 16, 2026 07:59

zkochan approved these changes Jun 16, 2026

View reviewed changes

zkochan merged commit 564619f into pnpm:main Jun 16, 2026
18 checks passed

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Summary

Root cause

Fix

Test

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!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested labels

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aqeelat commented Jun 6, 2026

Uh oh!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Great, no issues found!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aqeelat commented Jun 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading

coderabbitai Bot commented Jun 6, 2026 •

edited

Loading

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

edited

Loading