Repository navigation
bundle cleanup: report failed removals - #24289
Conversation
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
This diff is too big for this change. Please review it yourself and slim it down considerably.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description clearly explains the failed-removal reporting, failure propagation, successful-removal counting, tests, and verification covered by the changes. |
| Title check | |
The title clearly and concisely summarises 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
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.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the duplicate qualified-formula receipt collision that can leave an installation unprotected.
1 open issue
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
594c06b to
b58a83c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb: 1
668-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the CLI exit status for cleanup failures.
The failure examples call
described_class.cleanupdirectly. They checkHomebrew.failed?and stderr, but they do not exercise thebrew bundle cleanupprocess. The normalbrewpath exits with status 1 whenHomebrew.failed?is true. Add a command-level failure example that asserts the non-zero exit status, stderr, and relevant stdout. The related tests also callcleanupdirectly, 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
📒 Files selected for processing: 17
Library/Homebrew/bundle/extensions/extension.rbLibrary/Homebrew/bundle/extensions/flatpak.rbLibrary/Homebrew/bundle/extensions/go.rbLibrary/Homebrew/bundle/extensions/mac_app_store.rbLibrary/Homebrew/bundle/extensions/vscode_extension.rbLibrary/Homebrew/bundle/extensions/winget.rbLibrary/Homebrew/bundle/subcommand/cleanup.rbLibrary/Homebrew/test/bundle/cargo_spec.rbLibrary/Homebrew/test/bundle/flatpak_spec.rbLibrary/Homebrew/test/bundle/go_spec.rbLibrary/Homebrew/test/bundle/krew_spec.rbLibrary/Homebrew/test/bundle/mac_app_store_spec.rbLibrary/Homebrew/test/bundle/npm_spec.rbLibrary/Homebrew/test/bundle/uv_spec.rbLibrary/Homebrew/test/bundle/vscode_extension_spec.rbLibrary/Homebrew/test/bundle/winget_spec.rbLibrary/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.
A CLI test would add coverage, but the code that converts |
There was a problem hiding this comment.
🧹 Nitpick comments: 1
Library/Homebrew/test/cmd/bundle/cleanup_subcommand_spec.rb: 1
668-673: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe 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
📒 Files selected for processing: 21
Library/Homebrew/bundle/extensions/cargo.rbLibrary/Homebrew/bundle/extensions/extension.rbLibrary/Homebrew/bundle/extensions/flatpak.rbLibrary/Homebrew/bundle/extensions/go.rbLibrary/Homebrew/bundle/extensions/krew.rbLibrary/Homebrew/bundle/extensions/mac_app_store.rbLibrary/Homebrew/bundle/extensions/npm.rbLibrary/Homebrew/bundle/extensions/uv.rbLibrary/Homebrew/bundle/extensions/vscode_extension.rbLibrary/Homebrew/bundle/extensions/winget.rbLibrary/Homebrew/bundle/subcommand/cleanup.rbLibrary/Homebrew/test/bundle/cargo_spec.rbLibrary/Homebrew/test/bundle/flatpak_spec.rbLibrary/Homebrew/test/bundle/go_spec.rbLibrary/Homebrew/test/bundle/krew_spec.rbLibrary/Homebrew/test/bundle/mac_app_store_spec.rbLibrary/Homebrew/test/bundle/npm_spec.rbLibrary/Homebrew/test/bundle/uv_spec.rbLibrary/Homebrew/test/bundle/vscode_extension_spec.rbLibrary/Homebrew/test/bundle/winget_spec.rbLibrary/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.
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
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
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.
d334ab1 to
e48c597
Compare
|
@MikeMcQuaid The shared cleanup now uses 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 |
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.
Yup sounds good. |
Honestly, |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Report failed Go metadata lookups. · go.rb:101-115
Library/Homebrew/bundle/extensions/go.rb:101-115
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport failed Go metadata lookups.
When a configured Go cleanup item runs,
Utils.popen_readreturns output for a failedgo envorgo version -mcommand because both calls use the unsafe default.cleanup!does not inspect the command status. A failed metadata lookup can therefore skip the executable and reportRemoved 0 Go executableswithout settingHomebrew.failed. Handle these subprocess failures at this cleanup boundary.Suggested fix
- gobin = Utils.popen_read(go, "env", "GOBIN", err: :err).chomp - gopath = Utils.popen_read(go, "env", "GOPATH", err: :err).chomp + begin + gobin = Utils.popen_read(go, "env", "GOBIN", err: :err, safe: true).chomp + gopath = Utils.popen_read(go, "env", "GOPATH", err: :err, safe: true).chomp + rescue ErrorDuringExecution => e + ofail "Failed to inspect Go environment: #{e}" + return + end ... - output = Utils.popen_read(go, "version", "-m", binary, err: File::NULL) + begin + output = Utils.popen_read(go, "version", "-m", binary, err: File::NULL, safe: true) + rescue ErrorDuringExecution => e + ofail "Failed to inspect Go executable #{binary}: #{e}" + next + end
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b3bd36c9-e64a-4d26-b458-b5997713a9af
📒 Files selected for processing: 2
Library/Homebrew/bundle/extensions/go.rbLibrary/Homebrew/bundle/subcommand/cleanup.rb
🚧 Files skipped from review as they are similar to previous changes: 1
- Library/Homebrew/bundle/subcommand/cleanup.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.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
One more question and good to go.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
This looks great now, thanks for the work here and for persisting through the feedback. You rock!
brew bundle cleanupcan print misleading removal counts and exit successfully after uninstall or untap commands fail. It now reports these failures throughofail, 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
Uninstalledcount. 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/falseto 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.Error: Failed to uninstall npm packages: exampleUninstalled 1 npm packagesUninstalled 0 npm packagesHomebrew.failed?falsetrueVerification
./bin/brew lgtm --onlinepassed on macOS, covering typechecking, style and all 10 affected spec files. The same 10 spec files passed on Ubuntu 24.04 ARM64: 287 examples, 0 failures. GitHub CI for45d6b3895fpassed 49 checks, with 4 checks skipped.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?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.