Skip to content

Check cask receipt tap when untapping - #24228

Merged
p-linnane merged 1 commit into
mainfrom
untap-cask-tap
Oct 11, 2026
Merged

p-linnane merged 1 commit into
mainfrom
untap-cask-tap

Conversation

@p-linnane

Copy link
Copy Markdown
Contributor

brew untap treated any installed cask whose token is also in the tap as installed from it, so untapping a development fork of homebrew/cask offered to uninstall casks installed from homebrew/cask. It now checks the tap recorded in each cask's install receipt, as it already does for formula kegs. Casks without a receipt still fall back to the tap being untapped.

See https://github.com/orgs/Homebrew/discussions/7117.


  • 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.

Codex (GPT-6 Astra) drafted the change and tests, checked the tests fail without the fix and ran brew lgtm --online; Claude Code (Opus 5.5) reviewed it, repeated that check independently and ran brew lgtm --online on this commit. I reviewed the diff.


Only select installed casks whose install receipt records the tap being
untapped, matching the check already made for formula kegs. Cask tokens
are not unique across taps, so a fork of homebrew/cask otherwise claimed
casks installed from homebrew/cask. Casks without a receipt still fall
back to the tap they were loaded from.

Signed-off-by: Patrick Linnane <patrick@linnane.io>
@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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: d4ebe292-5be4-40b2-9ac4-63b90ee20fcd

📥 Commits

Reviewing files that changed from the base of the PR and between 9276d7a and 8e4c255.


📒 Files selected for processing (2)
  • Library/Homebrew/cmd/untap.rb
  • Library/Homebrew/test/cmd/untap_spec.rb

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


📜 Recent review details
⏰ Context from checks skipped due to timeout. (20)
  • GitHub Check: docker (arm64 Ubuntu 26.04)
  • GitHub Check: docker (x86_64 Ubuntu 24.04)
  • GitHub Check: docker (arm64 Ubuntu 24.04)
  • GitHub Check: docker (x86_64 Ubuntu 26.04)
  • GitHub Check: tests (online, 1/2)
  • GitHub Check: tests (load-only Linux)
  • GitHub Check: tests (macOS, 2/2)
  • GitHub Check: tests (online, 2/2)
  • GitHub Check: workflow_syntax
  • GitHub Check: tests (Linux, 1/2)
  • GitHub Check: tests (no Sorbet)
  • GitHub Check: tests (generic OS, 1/2)
  • GitHub Check: tests (load-only macOS)
  • GitHub Check: tests (Linux, 2/2)
  • GitHub Check: tests (generic OS, 2/2)
  • GitHub Check: tests (macOS, 1/2)
  • GitHub Check: syntax
  • GitHub Check: autogenerated
  • GitHub Check: Analyze
  • GitHub Check: docs

🧰 Additional context used
📚 Code guidelines (1)
Generated by CodeRabbit — auto-discovered

📓 Path-based instructions (2)
Test command behavior and its observable effects, including stdout, stderr, exit status, and state changes as relevant.

📄 CodeRabbit inference engine (Generated by CodeRabbit)

Files:

  • Library/Homebrew/test/cmd/untap_spec.rb

Implement CLI commands as `Homebrew::Cmd` classes inheriting from `AbstractCommand`; declare help text and argument metadata in `cmd_args`, and put command behavior in `run`.

📄 CodeRabbit inference engine (Generated by CodeRabbit)

Files:

  • Library/Homebrew/cmd/untap.rb

🔇 Additional comments (2)
Library/Homebrew/cmd/untap.rb (1)

139-139: LGTM!


Library/Homebrew/test/cmd/untap_spec.rb (1)

252-252: LGTM!

Also applies to: 272-272, 275-275, 298-304, 306-310



📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Untapping a repository now correctly excludes installed casks recorded as coming from a different repository.
  • Tests

    • Added coverage for casks installed from another repository and for installed casks whose receipt has been removed.

Walkthrough

installed_casks_for now includes an installed cask only when its recorded tap matches the queried tap. Tests verify that casks from another tap are excluded and that a cask remains included after its receipt is removed.

Changes

Untap cask filtering

Layer / File(s) Summary
Filter installed casks by tap
Library/Homebrew/cmd/untap.rb, Library/Homebrew/test/cmd/untap_spec.rb
installed_casks_for filters by installation status and recorded tap. Tests cover a cask from another tap and a cask whose receipt was removed.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mikemcquaid


Merge Risk | ⚪ Minimal · up to 8e4c2

Merge Risk: ⚪ Minimal · up to 8e4c2

Untap now excludes casks whose receipts identify another tap while retaining receipt-less casks through fallback metadata. No concrete merge-blocking risk remains.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 8e4c2

The change narrows cask selection to the recorded installation tap, reducing the chance of uninstalling an unrelated same-name cask. Existing confirmation and failure-handling behavior remains unchanged. No material security risk introduced or worsened by this change was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — For stable installation metadata, the added conjunction cannot enlarge the selected cask set. The affected selection remains limited to installed tokens discovered through the requested tap, with the same downstream uninstall and tap-removal consumers.

Security Findings and Attack Paths

  • inferred — A same-token definition in another tap no longer suffices to classify a receipt-bearing installation as owned by that tap. This closes the described unintended-selection path. The existing cask-loader call remains before the predicate, so the change should not be interpreted as a sandbox for tap definitions.

Trust Boundaries and Controls

  • observed — The newly consumed receipt tap passes through JSON parsing, Tap.fetch and tap equality rather than becoming an executable command. Tap.fetch rejects malformed slash structure and canonicalizes identity. These controls validate identity representation, not receipt authenticity.

Resilience and Maintainability Implications

  • inferred — Invalid receipt JSON or tap identity can abort processing before the current tap’s uninstall path. Earlier requested taps may already have completed. Receipt objects are cached, and the command has no transaction spanning selection and cleanup; concurrent metadata changes therefore remain outside the demonstrated guarantees.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: checking the cask receipt tap during untapping.
Description check Passed The description directly explains the bug, the fix, the fallback behavior, and the added test and validation steps.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed 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.

@p-linnane
p-linnane added this pull request to the merge queue Oct 11, 2026
Merged via the queue into main with commit ea07035 Oct 11, 2026
54 checks passed
@p-linnane
p-linnane deleted the untap-cask-tap branch October 11, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants