cask: fix migration when the new cask is already installed#23076
Merged
Conversation
`Cask::Migrator.migrate_if_needed` determined the token to migrate from the new cask's installed caskfile, but that lookup checks the new token before any old tokens. When both the old and the new cask were installed, it always found the new cask's own caskfile, decided nothing needed migrating and left the old cask's Caskroom directory in place. That directory then loaded as the cask it was renamed to, so the same cask was listed, reported as outdated and upgraded twice. Instead, migrate every old token that still has its own Caskroom directory. When the new cask is already installed under its own token the old cask is a separate installation that cannot be moved to the new token, so uninstall it (loaded from its own installed caskfile, so that its own artifacts are the ones removed) and leave behind the same symlink that a migration would. Casks that are renamed into each other can share artifacts, so skip any artifact that the new cask installs too: uninstalling those would remove them from the new cask, which stays installed. Also deduplicate `Cask::Caskroom.casks` so that an old token which has not been migrated yet cannot list the same cask twice.
a4bc8e6 to
3405c57
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes cask migration when a cask is renamed to a token that is already installed, ensuring the old-token install is correctly handled (including safe artifact removal) and preventing duplicate listing of the renamed cask while it still exists on disk.
Changes:
- Update
Cask::Migratorto discover all installed old tokens from Caskroom state, and to uninstall (not move) the old-token install when the new token is already installed, while preserving shared artifacts. - Add support in
Cask::Installerto restrict which artifacts are uninstalled (used to avoid removing artifacts owned by the still-installed new cask). - Deduplicate
Cask::Caskroom.casksby resolved cask identity and add coverage for both migration and listing behavior (including shared-artifact scenarios).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/cask/migrator.rb | Reworks migration discovery and behavior for “new token already installed” cases; adds shared-artifact-aware uninstall path. |
| Library/Homebrew/cask/installer.rb | Adds default_uninstall_artifacts: to restrict uninstall to a specific artifact subset; prevents overwrite when already set. |
| Library/Homebrew/cask/caskroom.rb | Deduplicates installed cask enumeration to avoid listing the same resolved cask twice. |
| Library/Homebrew/test/cask/migrator_spec.rb | Adds unit/spec coverage for old-token discovery, uninstall-vs-move behavior, dry-run behavior, and shared-artifact handling. |
| Library/Homebrew/test/cask/caskroom_spec.rb | Adds regression test ensuring renamed-but-not-migrated installs don’t cause duplicate listing. |
| Library/Homebrew/test/support/fixtures/cask/Casks/local-caffeine-clone.rb | Adds fixture cask to test shared-artifact migration/uninstall behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a cask is renamed to a cask that is already installed, the migration silently does nothing and the old cask is left behind in the Caskroom.
Cask::Migrator.migrate_if_neededworked out which token to migrate fromnew_cask.installed_caskfile. That lookup returns the first match, so with both casks installed it always found the new cask's own caskfile, saw that the installed token already matched, and returned early.The leftover directory then loads as the cask it was renamed to, because the loader resolves the rename before it falls back to the installed caskfile. Cask::Caskroom.casks returns the same cask twice, so it gets listed, reported as outdated and upgraded twice:
There's also no way out of that state, since brew uninstall --cask codex-app resolves the rename and uninstalls chatgpt instead.
Changes;
uninstall --> installon migration, in the same waybrew upgradedoes.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Claude Opus 4.8 Max was used to debug the issue and implement the fix.