Repository navigation
fix(brew): remove protected cask app backups - #11219
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe cask app installation flow now delegates staged replacement to ChangesApplication replacement flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant swap_app
participant macOSFilesystem
Installer->>swap_app: staged app and target paths
swap_app->>macOSFilesystem: stage old app and install staged app
swap_app->>macOSFilesystem: repair permissions and remove backup
Installer->>macOSFilesystem: remove quarantine metadata
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryFixes protected macOS cask app-backup cleanup.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains in scope for this follow-up review. Important Files Changed
Reviews (5): Last reviewed commit: "fix(brew): drop needless borrows in cask..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/system/packages/brew/cask.rs (1)
730-730: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHandle protected old-app cleanup before merge.
After Line 723 installs the replacement, Line 730 propagates
Permission deniedfor ACL-protected old contents. The upgrade is reported as failed despite the new app being live, and the added macOS test intentionally remains red. Make protected-content cleanup succeed before merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/packages/brew/cask.rs` at line 730, Update the cleanup flow around file::remove_all(&old_target) to handle ACL-protected old-app contents before merging, so Permission denied does not propagate after the replacement is installed. Ensure protected-content removal succeeds and the upgrade completes successfully while preserving the existing replacement-installation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/system/packages/brew/cask.rs`:
- Line 730: Update the cleanup flow around file::remove_all(&old_target) to
handle ACL-protected old-app contents before merging, so Permission denied does
not propagate after the replacement is installed. Ensure protected-content
removal succeeds and the upgrade completes successfully while preserving the
existing replacement-installation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e47e1fd-1e0d-4fd0-86df-f0a80734e30b
📒 Files selected for processing (1)
src/system/packages/brew/cask.rs
jdx
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/system/packages/brew/cask.rs (1)
702-733: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOld-app cleanup failure after a successful swap wrongly fails the whole install and skips quarantine removal.
swap_app's finalremove_app(&old_target)?(Line 731) runs after the rename totargetalready succeeded — the new app is live. If this backup cleanup fails (the exact scenario this PR's new test reproduces),swap_appreturnsErr, which propagates throughinstall_app'sswap_app(&target, &tmp_target)?(Line 702) and skips the quarantinexattrremoval (Lines 703-709). The caller ends up reporting a failed install/upgrade even though the new app was successfully installed — just with a stale, undeletable backup left behind and Gatekeeper quarantine still attached.Don't let old-backup cleanup failure fail an already-successful swap; log and continue instead.
🐛 Proposed fix
if let Err(e) = file::rename(&tmp_target, &target) { // Restore the old app if the swap failed. if old_target.exists() { let _ = file::rename(&old_target, &target); } return Err(e); } - remove_app(&old_target)?; - Ok(()) + if let Err(e) = remove_app(&old_target) { + warn!( + "brew-cask: failed to remove old app backup {}: {e}", + old_target.display() + ); + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/packages/brew/cask.rs` around lines 702 - 733, Update swap_app so failure from the final remove_app(&old_target) cleanup after successfully renaming the new app is logged and ignored rather than propagated. Preserve error propagation for the actual swap operations and ensure install_app continues to quarantine removal even when the stale backup cannot be deleted.
🧹 Nitpick comments (1)
src/system/packages/brew/cask.rs (1)
735-784: 🧹 Nitpick | 🔵 TrivialEscalation design looks sound;
is_permission_deniedcorrectly downcasts throughwrap_err_with.The retry/repair/sudo-escalate/retry sequence in
remove_appis reasonable, and per eyre's docs, downcasting to the originalio::Errorstill works afterfile::remove_all'swrap_err_withwrapping, sois_permission_deniedwill correctly detect the wrappedPermissionDeniederrors.One thing worth calling out operationally: this path calls
sudo::run("chown", ...)from inside a fallback cleanup routine reachable from ordinaryinstall_appcalls. In a normal (non-CI) environment with an interactive TTY, a plainmiseinstall/upgrade could unexpectedly prompt for a sudo password when it hits a protected leftover app — worth confirming this UX is intended/documented, since it's a new escalation surface introduced silently as part of app cleanup rather than an explicit user-facing "elevate" step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/packages/brew/cask.rs` around lines 735 - 784, Review the sudo escalation in remove_app, which can prompt for a password during ordinary install_app cleanup. Confirm that this behavior is intentional and documented; if not, remove or gate the sudo::run("chown", ...) fallback behind an explicit elevation path rather than triggering it silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/system/packages/brew/cask.rs`:
- Around line 702-733: Update swap_app so failure from the final
remove_app(&old_target) cleanup after successfully renaming the new app is
logged and ignored rather than propagated. Preserve error propagation for the
actual swap operations and ensure install_app continues to quarantine removal
even when the stale backup cannot be deleted.
---
Nitpick comments:
In `@src/system/packages/brew/cask.rs`:
- Around line 735-784: Review the sudo escalation in remove_app, which can
prompt for a password during ordinary install_app cleanup. Confirm that this
behavior is intentional and documented; if not, remove or gate the
sudo::run("chown", ...) fallback behind an explicit elevation path rather than
triggering it silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a3600ffe-0bab-48d5-a2b0-585e4ed605ce
📒 Files selected for processing (1)
src/system/packages/brew/cask.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/cli/test_system_install_brew_macos_slow (1)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the new
cleanuptrap itself.Because
cleanupruns only onEXIT, the assertions after reinstall execute before it and therefore do not verify removal of stale.mise-old-*or.mise-tmp-*paths. Add a fixture/subprocess that leaves each stale path and checks the filesystem after exit; cover both patterns.Also applies to: 44-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cli/test_system_install_brew_macos_slow` around lines 16 - 25, Extend the test around the cleanup function and its EXIT trap to exercise cleanup in a separate subprocess, creating one stale /Applications/Hidden Bar.mise-old-* path and one .mise-tmp-* path before the subprocess exits. After the subprocess completes, assert both paths were removed, while preserving the existing reinstall assertions and cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/cli/test_system_install_brew_macos_slow`:
- Around line 16-25: Extend the test around the cleanup function and its EXIT
trap to exercise cleanup in a separate subprocess, creating one stale
/Applications/Hidden Bar.mise-old-* path and one .mise-tmp-* path before the
subprocess exits. After the subprocess completes, assert both paths were
removed, while preserving the existing reinstall assertions and cleanup
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3edfa10b-0a85-4a3c-9960-2557cbeaf402
📒 Files selected for processing (1)
e2e/cli/test_system_install_brew_macos_slow
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.12 x -- echo |
31.2 ± 4.0 | 21.0 | 49.3 | 1.08 ± 0.16 |
mise x -- echo |
28.9 ± 1.9 | 23.7 | 35.8 | 1.00 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.12 env |
25.7 ± 1.9 | 21.2 | 39.0 | 1.00 |
mise env |
28.2 ± 4.1 | 21.0 | 47.7 | 1.10 ± 0.18 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.12 hook-env |
30.4 ± 4.9 | 20.8 | 46.1 | 1.28 ± 0.23 |
mise hook-env |
23.8 ± 1.8 | 20.6 | 29.6 | 1.00 |
✅ Performance improvement for hook-env is 28% |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.7.12 ls |
19.3 ± 3.4 | 16.2 | 29.9 | 1.00 |
mise ls |
28.9 ± 1.9 | 22.2 | 34.9 | 1.50 ± 0.28 |
ls is 50% |
xtasks/test/perf
| Command | mise-2026.7.12 | mise | Variance |
|---|---|---|---|
| install (cached) | 263ms | 277ms | -5% |
| ls (cached) | 92ms | 101ms | -8% |
| bin-paths (cached) | 92ms | 94ms | -2% |
| task-ls (cached) | 166ms | 171ms | -2% |
Clippy 1.94's needless_borrows_for_generic_args flagged the &&Path arguments to file::rename in swap_app, failing the lint CI job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/system/packages/brew/cask.rs (1)
4644-4654: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the ACL on
targetwhen the swap fails before backup creation.If
swap_apperrors before renaming the existing app,old_targetdoes not exist and the protected ACL remains ontarget. The test’s cleanup then cannot reliably remove the temporary directory. Clear the ACL fromtargetwhenresult.is_err()and no backup exists.Proposed fix
- if old_target.exists() { + let cleanup_path = if old_target.exists() { + Some(old_target.as_path()) + } else if result.is_err() && target.exists() { + Some(target.as_path()) + } else { + None + }; + + if let Some(path) = cleanup_path { let status = std::process::Command::new("chmod") .arg("-RN") - .arg(&old_target) + .arg(path) .status()?; assert!(status.success()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/system/packages/brew/cask.rs` around lines 4644 - 4654, Update the swap failure cleanup around swap_app so that when result.is_err() and old_target does not exist, it also clears the ACL from target before test cleanup. Preserve the existing old_target cleanup path and use the same chmod -RN behavior for target.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/system/packages/brew/cask.rs`:
- Around line 4644-4654: Update the swap failure cleanup around swap_app so that
when result.is_err() and old_target does not exist, it also clears the ACL from
target before test cleanup. Preserve the existing old_target cleanup path and
use the same chmod -RN behavior for target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: dfda62bd-fff4-41b5-aaa6-4c233c7f8237
📒 Files selected for processing (1)
src/system/packages/brew/cask.rs
Problem
Follow-up to discussion #11168. After the
/usr/localbinary-target fix, upgrading an existingdocker-desktopcask could fail while deleting the app backup:Root cause
The cask app swap renames the existing bundle to
.mise-old-*, installs the new bundle, and then recursively removes the backup. Docker app bundles can contain ownership-, ACL-, or flag-protected descendants, so an ordinary unprivilegedremove_dir_allcan fail even though renaming the top-level bundle succeeded.Fix
Match Homebrew's permission-recovery approach when app-backup removal returns
PermissionDenied:Ordinary deletion remains the fast path. Permission repair and elevation are only attempted after
EACCES.The same cleanup is used for stale
.mise-old-*bundles left by an interrupted or failed prior upgrade.Tests
Docker.app. Before the fix it reproduced the reportedPermission deniederror; after the fix it passes.hiddenbarcask, protect its installed app contents with a delete-deny ACL, force a full reinstall, and verify the app is installed without a stale.mise-old-*backup.cargo test system::packages::brew::cask::tests -- --nocapture— 84 passedmise run test:e2e e2e/cli/test_system_install_brew_macos_slow— local harness passes and safely skips the CI-only/Applicationsmutation;unit-macosexercises it on the ephemeral runnermise run lint-fixmise run lintThis PR was generated by an AI coding assistant.
Summary by CodeRabbit
Bug Fixes
Tests
Note
Medium Risk
Touches macOS cask install/upgrade paths and may invoke sudo chown when deleting protected app bundles; behavior change is scoped to permission-denied removal with tests covering the swap path.
Overview
Fixes brew-cask upgrades that could fail after the new app was already in place when deleting the
.mise-old-*backup hitPermission denied(e.g. docker-desktop with ACL/flag-protected bundle contents).install_appnow delegates the atomic rename toswap_app, which usesremove_appfor stale backups and post-swap cleanup.remove_appretries like Homebrew: clear flags, restore owner perms, strip ACLs, then optionalsudo chownbefore another delete. If backup removal still fails after a successful swap, the install warns instead of failing so quarantine stripping still runs.Tests: macOS unit test with a delete-deny ACL on an existing app; slow CI E2E reinstalls hiddenbar with protected
Contentsand asserts no leftover.mise-old-*; E2E cleanup clears ACLs on temp/old app paths.Reviewed by Cursor Bugbot for commit 41fc234. Bugbot is set up for automated code reviews on this repo. Configure here.