bundle cleanup: report failed removals by edel67 · Pull Request #24289 · Homebrew/brew

edel67 · 2026-10-17T16:13:01Z

brew bundle cleanup can print misleading removal counts and exit successfully after uninstall or untap commands fail. It now reports these failures through ofail, attempts the remaining removals and cache cleanup and exits with a non-zero status even if later commands succeed. Unexpected errors can still interrupt cleanup, and completed removals and trust changes are not rolled back.

A shared helper reports per-item uninstall failures across extension managers and uses successful commands for the Uninstalled count. Cask, formula and untap operations run in batches, so their failure messages warn that some items may already have been removed. Go cleanup, which deletes executables directly, now reports filesystem errors and counts only files it removes.

Reproduction

Run this isolated npm cleanup check from a Homebrew checkout. It uses /usr/bin/false to produce an uninstall failure without changing packages or trust, testing the handler's error output and failure flag rather than the full command's exit status.

HOMEBREW_DEVELOPER=1 HOMEBREW_NO_INSTALL_FROM_API=1 ./bin/brew ruby -- - <<'RUBY'
require "bundle"
require "bundle/extensions/npm"
Homebrew::Bundle::Npm.define_singleton_method(:package_manager_executable) do
  Pathname("/usr/bin/false")
end
Homebrew::Bundle::Npm.cleanup!(["example"])
puts "Failure recorded: #{Homebrew.failed?}"
RUBY

Result	Original cleanup method	Updated cleanup method
Error	None	`Error: Failed to uninstall npm packages: example`
Summary	`Uninstalled 1 npm packages`	`Uninstalled 0 npm packages`
`Homebrew.failed?`	`false`	`true`

Verification

./bin/brew lgtm --online passed on macOS, covering typechecking, style and all 10 affected spec files. All 24 selected failure regressions fail against the original implementation and pass with this fix. The same 10 spec files passed on Ubuntu 24.04 ARM64: 283 examples, 0 failures. GitHub CI for d334ab134d passed 50 checks, with 4 deployment and publishing checks skipped.

Have you followed our Contributing guidelines?
Have you checked for other open Pull Requests for the same change?
Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
Have you explained why you'd like these changes included, not just what they do?
For bug fixes, have you given step-by-step brew commands to reproduce the bug?
Have you written new tests (excluding integration tests)? Here's an example.
Have you successfully run brew lgtm (style, typechecking and tests) locally?

I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

I used OpenAI Codex to help apply the revised design consistently across the Go, WinGet and Flatpak cleanup handlers, which I was less familiar with. It also helped draft the initial regression tests, narrow the patch and review the code. I reviewed the changes one code block at a time to understand and assess them myself.

Summary by CodeRabbit

Bug Fixes
- Cleanup now reports failed removals, continues with remaining items where possible, and counts only successful removals.
- Cleanup exits with a failure status when removal commands fail.
- Improved handling of missing executables and removal errors across supported package types.
- Cleanup output now uses clearer, consistent package and application names.
Documentation
- Clarified what happens after cleanup errors, including which changes are not rolled back and how to retry.

MikeMcQuaid

This diff is too big for this change. Please review it yourself and slim it down considerably.

coderabbitai · 2026-10-17T16:20:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 69449eb0-93a7-4ef2-9d41-31fbc2959b8f

📥 Commits

Reviewing files that changed from the base of the PR and between d334ab1 and e48c597.

📒 Files selected for processing: 14

Library/Homebrew/bundle/extensions/extension.rb
Library/Homebrew/bundle/extensions/flatpak.rb
Library/Homebrew/bundle/extensions/go.rb
Library/Homebrew/bundle/extensions/mac_app_store.rb
Library/Homebrew/bundle/extensions/vscode_extension.rb
Library/Homebrew/bundle/extensions/winget.rb
Library/Homebrew/test/bundle/flatpak_spec.rb
Library/Homebrew/test/bundle/go_spec.rb
Library/Homebrew/test/bundle/krew_spec.rb
Library/Homebrew/test/bundle/mac_app_store_spec.rb
Library/Homebrew/test/bundle/npm_spec.rb
Library/Homebrew/test/bundle/vscode_extension_spec.rb
Library/Homebrew/test/bundle/winget_spec.rb
Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb

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

Library/Homebrew/test/bundle/krew_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Brew Bundle cleanup now reports failed removal commands, continues through remaining extension items, and counts successful removals. Go executable cleanup handles missing files separately from other unlink errors. Tests and documentation cover failure reporting, continuation, and retry behaviour.

Changes: Brew Bundle cleanup

Layer / File(s)	Summary
Shared extension cleanup and provider behaviour `Library/Homebrew/bundle/extensions/`, `Library/Homebrew/test/bundle/`	The shared cleanup helper reports unsuccessful uninstalls, continues processing, and counts successful removals. Provider cleanup methods use the helper or handle executable unlink errors directly. Tests cover command results, output, failure state, and removal counts.
Cleanup command failure handling `Library/Homebrew/bundle/subcommand/cleanup.rb`, `Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb`, `docs/Brew-Bundle-and-Brewfile.md`	Cask, formula, and tap removal failures are reported. Tests cover failure state and subsequent cleanup work. Documentation describes non-zero exit status, incomplete cleanup, and retry behaviour.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to e48c5

Brew bundle cleanup now reports failed removals, continues the remaining work, and exits non-zero on failure. No merge-blocking risk was identified.

Security architecture risk: 🔵 Low · up to e48c5

The change improves failure visibility while preserving the inspected cleanup targets and command privileges. Risk is limited to integrating the shared result contract across package managers. Cleanup remains non-transactional, and interruption and concurrent execution were not runtime-validated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The inspected changes affect packages and executables already reachable through the invoking cleanup process and external package managers. Flatpak retains system-wide removal, and WinGet retains exact ID and source selection. The provider refactoring does not establish a new privilege grant or broader removal scope.

Trust Boundaries and Controls

observed — The shared helper passes each selected item to its provider without constructing a shell command. Inspected provider sinks retain separate command arguments, and Go deletes the literal selected filesystem path. Flatpak now uses the resolved package-manager executable instead of a bare command name.

Resilience and Maintainability Implications

observed — Failure reporting sets Homebrew.failed without clearing earlier failures, preserving a nonzero terminal status after later successful work. The existing environment wrapper restores overridden variables in ensure, including exception unwinding. Inspected regression tests cover unsuccessful command results and failure retention after cache cleanup; they were not executed in this review.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: reporting failed removals during bundle cleanup.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Copilot

🟡 Changes recommended

Resolve the duplicate qualified-formula receipt collision that can leave an installation unprotected.

1 open issue

Preserve receipts for duplicate formula names during fallback · New

What changed in this PR

Improves brew bundle cleanup by reporting failed removals, preserving failure status, and strengthening formula receipt protection.

Changes:

Adds failure reporting and accurate removal counts.
Improves manager-specific cleanup and receipt matching.
Adds regression tests and documentation.

File	Summary
`Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb`	Tests cleanup failure behavior.
`Library/Homebrew/test/bundle/winget_spec.rb`	Tests WinGet cleanup handling.
`Library/Homebrew/test/bundle/vscode_extension_spec.rb`	Tests VS Code extension cleanup.
`Library/Homebrew/test/bundle/uv_spec.rb`	Tests uv cleanup handling.
`Library/Homebrew/test/bundle/npm_spec.rb`	Tests npm failure reporting.
`Library/Homebrew/test/bundle/mac_app_store_spec.rb`	Tests Mac App Store cleanup.
`Library/Homebrew/test/bundle/krew_spec.rb`	Tests krew cleanup handling.
`Library/Homebrew/test/bundle/go_spec.rb`	Tests Go executable cleanup.
`Library/Homebrew/test/bundle/flatpak_spec.rb`	Tests Flatpak cleanup handling.
`Library/Homebrew/test/bundle/cargo_spec.rb`	Tests Cargo cleanup handling.
`Library/Homebrew/test/bundle/bundle_spec.rb`	Tests bundle receipt behavior.
`Library/Homebrew/bundle/subcommand/cleanup.rb`	Reports failures and continues cleanup.
`Library/Homebrew/bundle/extensions/winget.rb`	Updates WinGet cleanup behavior.
`Library/Homebrew/bundle/extensions/vscode_extension.rb`	Updates VS Code cleanup behavior.
`Library/Homebrew/bundle/extensions/uv.rb`	Updates uv cleanup behavior.
`Library/Homebrew/bundle/extensions/npm.rb`	Reports npm uninstall failures.
`Library/Homebrew/bundle/extensions/mac_app_store.rb`	Updates Mac App Store cleanup.
`Library/Homebrew/bundle/extensions/krew.rb`	Updates krew cleanup behavior.
`Library/Homebrew/bundle/extensions/go.rb`	Reports executable deletion errors.
`Library/Homebrew/bundle/extensions/flatpak.rb`	Updates Flatpak cleanup behavior.
`Library/Homebrew/bundle/extensions/extension.rb`	Adds shared cleanup failure handling.
`Library/Homebrew/bundle/extensions/cargo.rb`	Updates Cargo cleanup behavior.
`Library/Homebrew/bundle.rb`	Improves formula receipt protection.
`docs/Brew-Bundle-and-Brewfile.md`	Documents cleanup failure semantics.

_{Review effort: Lite}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments: 1

Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb: 1

668-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the CLI exit status for cleanup failures.

The failure examples call described_class.cleanup directly. They check Homebrew.failed? and stderr, but they do not exercise the brew bundle cleanup process. The normal brew path exits with status 1 when Homebrew.failed? is true. Add a command-level failure example that asserts the non-zero exit status, stderr, and relevant stdout. The related tests also call cleanup directly, so they do not detect a regression in the CLI exit status.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: f1b2d764-52a0-4ce6-adc7-7a883fda8533

📥 Commits

Reviewing files that changed from the base of the PR and between 594c06b and b58a83c.

📒 Files selected for processing: 17

Library/Homebrew/bundle/extensions/extension.rb
Library/Homebrew/bundle/extensions/flatpak.rb
Library/Homebrew/bundle/extensions/go.rb
Library/Homebrew/bundle/extensions/mac_app_store.rb
Library/Homebrew/bundle/extensions/vscode_extension.rb
Library/Homebrew/bundle/extensions/winget.rb
Library/Homebrew/bundle/subcommand/cleanup.rb
Library/Homebrew/test/bundle/cargo_spec.rb
Library/Homebrew/test/bundle/flatpak_spec.rb
Library/Homebrew/test/bundle/go_spec.rb
Library/Homebrew/test/bundle/krew_spec.rb
Library/Homebrew/test/bundle/mac_app_store_spec.rb
Library/Homebrew/test/bundle/npm_spec.rb
Library/Homebrew/test/bundle/uv_spec.rb
Library/Homebrew/test/bundle/vscode_extension_spec.rb
Library/Homebrew/test/bundle/winget_spec.rb
Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

edel67 · 2026-10-18T06:01:52Z

Actionable comments posted: 1

🧹 Nitpick comments: 1

Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb: 1> 668-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the CLI exit status for cleanup failures.
The failure examples call described_class.cleanup directly. They check Homebrew.failed? and stderr, but they do not exercise the brew bundle cleanup process. The normal brew path exits with status 1 when Homebrew.failed? is true. Add a command-level failure example that asserts the non-zero exit status, stderr, and relevant stdout. The related tests also call cleanup directly, so they do not detect a regression in the CLI exit status.

ℹ️ Review info
⚙️ Run configuration
📥 Commits
📒 Files selected for processing: 17
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

A CLI test would add coverage, but the code that converts Homebrew.failed? to exit status 1 is unchanged. These tests check that failures are recorded and preserved through later cleanup. I’m leaving the additional CLI test out to keep this fix focused.

coderabbitai

🧹 Nitpick comments: 1

Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb: 1

668-673: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

The direct assertions cover Homebrew.failed? and the failure message, but they do not exercise the CLI entrypoint that converts that state into process exit status 1. A command-level example should cover the non-zero exit status and the observable output from the remaining successful cleanup.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 9c41281f-4226-4e5f-ade7-0302b788397f

📥 Commits

Reviewing files that changed from the base of the PR and between b58a83c and d334ab1.

📒 Files selected for processing: 21

Library/Homebrew/bundle/extensions/cargo.rb
Library/Homebrew/bundle/extensions/extension.rb
Library/Homebrew/bundle/extensions/flatpak.rb
Library/Homebrew/bundle/extensions/go.rb
Library/Homebrew/bundle/extensions/krew.rb
Library/Homebrew/bundle/extensions/mac_app_store.rb
Library/Homebrew/bundle/extensions/npm.rb
Library/Homebrew/bundle/extensions/uv.rb
Library/Homebrew/bundle/extensions/vscode_extension.rb
Library/Homebrew/bundle/extensions/winget.rb
Library/Homebrew/bundle/subcommand/cleanup.rb
Library/Homebrew/test/bundle/cargo_spec.rb
Library/Homebrew/test/bundle/flatpak_spec.rb
Library/Homebrew/test/bundle/go_spec.rb
Library/Homebrew/test/bundle/krew_spec.rb
Library/Homebrew/test/bundle/mac_app_store_spec.rb
Library/Homebrew/test/bundle/npm_spec.rb
Library/Homebrew/test/bundle/uv_spec.rb
Library/Homebrew/test/bundle/vscode_extension_spec.rb
Library/Homebrew/test/bundle/winget_spec.rb
Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

edel67 · 2026-10-18T06:18:14Z

This diff is too big for this change. Please review it yourself and slim it down considerably.

I removed receipt fixes, parsing refactors and unrelated cleanup. I kept the code that detects failed removals, reports them, counts successes correctly and preserves the failure status, plus tests and documentation for that behavior. It still touches 22 files because the package managers have separate handlers, but the changed-line total dropped by ~80%.

MikeMcQuaid

Looking better, thanks! Perhaps needs a rebase and ideally a lot more DRYing up.

- Preserve failures while later cleanup steps continue. - Count successful operations without claiming failed removals.

edel67 · 2026-10-20T03:00:49Z

@MikeMcQuaid The shared cleanup now uses package_description with Utils.pluralize, but that changes "Krew plugin" to "krew plugin" and "Mac App Store app" to "app".

I'd prefer to preserve the existing wording without duplicating labels or changing naming elsewhere. Is there an existing way you'd suggest reusing those names? Otherwise, would a cleanup_description method defaulting to package_description, with overrides for the exceptions, be reasonable? I've kept that change out pending your feedback.

MikeMcQuaid · 2026-10-20T07:54:01Z

+If a removal command reports failure, cleanup reports the error and attempts the remaining removals and cache cleanup.
+It exits with a non-zero status.
+Unexpected errors can stop cleanup before it reaches the remaining work.
+
+A failed command may already have removed some items.
+Cleanup does not undo completed removals or changes to the trust store.
+After resolving the error, rerun cleanup with the same options to retry the remaining work.
+


Remove this

MikeMcQuaid · 2026-10-20T07:54:09Z

        :registered_extensions_to_install
      end

+      # Continue after unsuccessful commands while preserving the overall failure status.


Remove this

MikeMcQuaid · 2026-10-20T08:00:05Z

The shared cleanup now uses package_description with Utils.pluralize, but that changes "Krew plugin" to "krew plugin" and "Mac App Store app" to "app".

I'd prefer to preserve the existing wording without duplicating labels or changing naming elsewhere.

I think it's preferable to avoid the duplication rather than get keep that casing as-is. If you feel strongly: please add another generic method for this rather than having the repetition.

Otherwise, would a cleanup_description method defaulting to package_description, with overrides for the exceptions, be reasonable?

Yup sounds good.

Copilot AI lite review requested due to automatic review settings October 17, 2026 16:13

Copilot started reviewing on behalf of edel67 October 17, 2026 16:13

View session

MikeMcQuaid requested changes Oct 17, 2026

View reviewed changes

Copilot AI reviewed Oct 17, 2026

View reviewed changes

Comment thread Library/Homebrew/bundle.rb Outdated

edel67 force-pushed the bundle-cleanup-failure-reporting branch from 594c06b to b58a83c Compare October 17, 2026 18:44

coderabbitai Bot reviewed Oct 17, 2026

View reviewed changes

Comment thread Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb

coderabbitai Bot reviewed Oct 18, 2026

View reviewed changes

edel67 requested a review from MikeMcQuaid October 18, 2026 06:21

MikeMcQuaid reviewed Oct 18, 2026

View reviewed changes

Comment thread Library/Homebrew/bundle/extensions/flatpak.rb Outdated

Comment thread Library/Homebrew/bundle/extensions/extension.rb Outdated

edel67 added 4 commits October 18, 2026 14:04

bundle cleanup: report failed removals

a58ac8f

- Preserve failures while later cleanup steps continue. - Count successful operations without claiming failed removals.

bundle: use pluralize for Flatpak cleanup

e9b1bb4

bundle: share extension cleanup handling

cec300f

bundle: report missing cleanup executables

e48c597

edel67 force-pushed the bundle-cleanup-failure-reporting branch from d334ab1 to e48c597 Compare October 20, 2026 01:52

MikeMcQuaid reviewed Oct 20, 2026

View reviewed changes

Conversation

edel67 commented Oct 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reproduction

Verification

Summary by CodeRabbit

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Oct 17, 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

🟡 Changes recommended

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edel67 commented Oct 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

edel67 commented Oct 18, 2026

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edel67 commented Oct 20, 2026

Uh oh!

MikeMcQuaid Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Oct 20, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edel67 commented Oct 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 17, 2026 •

edited

Loading

edel67 commented Oct 18, 2026 •

edited

Loading