Consistent cask installed checks across all code paths by koddsson · Pull Request #21880 · Homebrew/brew

koddsson · 2026-04-01T12:04:53Z

Several code paths that answer "is this cask installed?" were using different heuristics (raw ls on Caskroom, directory existence, Caskroom.casks) that could disagree with Cask#installed?. This consolidates them to use .metadata directory presence as the source of truth.

Add Cask::Caskroom.cask_installed?(token) as a canonical utility method, consistent with Cask#installed?
Update Caskroom.any_casks_installed? to check .metadata
Update list.sh bash fast path to check .metadata
Update cmd/list.rb to use Caskroom.casks in all code paths
Update cmd/update-report.rb to delegate to Caskroom.cask_installed?

Follows up on #21630 which added .select(&:installed?) to Caskroom.casks but didn't update the other code paths.

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests (excluding integration tests) for your changes? Here's an example.
Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

AI was used to generate or assist with generating this PR. Claude Code (claude-opus-4-6) was used to help identify the inconsistent code paths, implement the fixes, and write tests. All changes were reviewed and validated by a human.

koddsson · 2026-04-01T12:10:21Z

~~Tests were failing for me locally and I couldn't figure it out. Decided to push up here as a draft and see if it fails in CI to see if my set up is borked.~~

My setup was borked!

koddsson · 2026-04-01T12:31:21Z

I'm leaning heavily on the LLM for the bash since that's not my strong suite. It reads good to me but I'm happy to change anything if it's shit.

Copilot

Pull request overview

This PR aims to make “is this cask installed?” checks consistent across Homebrew by centralizing the predicate in Cask::Caskroom and updating multiple call sites (Ruby commands and the brew list bash fast path) to rely on .metadata-based installation state.

Changes:

Add Cask::Caskroom.cask_installed?(token) and update any_casks_installed? to use .metadata.
Update brew list implementations (bash list.sh and Ruby cmd/list.rb) to list only installed casks consistently.
Update cmd/update-report.rb to delegate cask-installed checks to the new utility method, with accompanying tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Library/Homebrew/cask/caskroom.rb	Introduces `cask_installed?` and changes `any_casks_installed?` to use `.metadata`.
Library/Homebrew/list.sh	Changes bash fast-path cask listing to filter by `.metadata`.
Library/Homebrew/cmd/list.rb	Changes Ruby list command to source casks from `Cask::Caskroom.casks` for more consistent installed filtering.
Library/Homebrew/cmd/update-report.rb	Delegates cask installation detection to `Cask::Caskroom.cask_installed?`.
Library/Homebrew/test/cask/caskroom_spec.rb	Adds unit tests for `cask_installed?` and `any_casks_installed?`.
Library/Homebrew/test/cmd/list_spec.rb	Adds a test asserting cask listing uses `Caskroom.casks`.
Library/Homebrew/test/cmd/update-report_spec.rb	Adds a test asserting delegation to `Cask::Caskroom.cask_installed?`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T12:39:28Z

Library/Homebrew/cask/caskroom.rb

+    # Check if a cask with the given token is installed, consistent with {Cask#installed?}.
+    # Uses the presence of a `.metadata` directory as the source of truth.
+    sig { params(token: String).returns(T::Boolean) }
+    def self.cask_installed?(token)
+      (path/token/".metadata").directory?
+    end


Cask::Caskroom.cask_installed? currently returns true based solely on the existence of a .metadata directory, but Cask#installed? requires a valid installed_caskfile under .metadata/.../Casks/. This can misclassify partially/corrupt installs as installed and reintroduce the inconsistency that Caskroom.casks.select(&:installed?) was meant to avoid. Consider matching Cask#installed? more closely (e.g., check for the presence of an installed caskfile in the expected metadata structure) or update the method documentation/name to reflect the weaker predicate.

Library/Homebrew/list.sh

Copilot · 2026-04-01T12:39:29Z

Library/Homebrew/cmd/list.rb

+            if args.t?
+              casks.sort_by! { |c| c.install_time || Time.at(0) }.reverse!
+            else
+              casks.sort_by!(&:token)
+            end
+            casks.reverse! if args.r?
+            cask_names = casks.map(&:token)
+            if cask_names.present?
+              ohai "Casks" if $stdout.tty? && !args.cask?
+              if args.public_send(:"1?") || !$stdout.tty?
+                puts cask_names
+              else
+                puts Formatter.columns(cask_names)
+              end


In the args.no_named? path, cask output no longer uses ls_args like the formula output does, so -l (and tty column formatting) have no effect for casks, despite -l/-t/-r/-1 being documented as applying to “formulae and/or casks”. If the intent is to keep ls-compatible output, consider printing casks via ls over the filtered installed cask directories (or otherwise honoring -l), and ensure -t matches the documented “time modified” semantics if that’s still desired.

Suggested change

if args.t?

casks.sort_by! { |c| c.install_time || Time.at(0) }.reverse!

else

casks.sort_by!(&:token)

end

casks.reverse! if args.r?

cask_names = casks.map(&:token)

if cask_names.present?

ohai "Casks" if $stdout.tty? && !args.cask?

if args.public_send(:"1?") || !$stdout.tty?

puts cask_names

else

puts Formatter.columns(cask_names)

end

if casks.present?

ohai "Casks" if $stdout.tty? && !args.cask?

system_command! "ls",

args: [*ls_args, *casks.map(&:caskroom_path)],

print_stdout: true

Library/Homebrew/cask/caskroom.rb

Several code paths that determine whether a cask is installed were using different heuristics (raw `ls`, directory existence, `Caskroom.casks`), which could disagree with `Cask#installed?`. This consolidates them all to use `.metadata` directory presence as the source of truth. - Add `Cask::Caskroom.cask_installed?` utility method - Update `Caskroom.any_casks_installed?` to check `.metadata` - Update `list.sh` bash fast path to check `.metadata` - Update `cmd/list.rb` to use `Caskroom.casks` in all code paths - Update `cmd/update-report.rb` to delegate to `Caskroom.cask_installed?` - Add unit tests for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T12:57:48Z

Library/Homebrew/list.sh

+    local installed_cask_names=()
+    for dir in "${HOMEBREW_CASKROOM}"/*/
+    do
+      [[ -d "${dir}.metadata" ]] && installed_cask_names+=("$(basename "${dir}")")


The filter here only checks for the existence of a .metadata directory, but Cask#installed?/Cask::Caskroom.cask_installed? require an installed caskfile under .metadata/<version>/<timestamp>/Casks/*.rb|*.json. As a result, brew list can still show “installed” casks with empty/corrupt metadata, reintroducing the inconsistent-installedness this PR aims to remove. Update the shell fast-path to check for the presence of an installed caskfile (matching the same pattern as Cask::Caskroom.cask_installed?).

Suggested change

[[ -d "${dir}.metadata" ]] && installed_cask_names+=("$(basename "${dir}")")

if compgen -G "${dir}.metadata"/*/*/Casks/*.rb > /dev/null \

|| compgen -G "${dir}.metadata"/*/*/Casks/*.json > /dev/null

then

installed_cask_names+=("$(basename "${dir}")")

fi

MikeMcQuaid

Hey @koddsson, thanks for the PR! A bunch of comments for ya.

MikeMcQuaid · 2026-04-01T12:57:19Z

Library/Homebrew/cask/caskroom.rb

    sig { returns(T::Boolean) }
    def self.any_casks_installed?
-      paths.any?
+      paths.any? { |p| Pathname.glob(p/".metadata"/"*"/"*"/"Casks"/"*.{rb,json}").any? }


We shouldn't duplicate this glob and instead split to a function/variable/constant.

MikeMcQuaid · 2026-04-01T12:57:35Z

Library/Homebrew/cask/caskroom.rb

      paths.map { |path| path.basename.to_s }
    end

+    # Check if a cask with the given token is installed, consistent with {Cask#installed?}.


Suggested change

# Check if a cask with the given token is installed, consistent with {Cask#installed?}.

# Check if a cask with the given token is installed, called by {Cask#installed?}.

MikeMcQuaid · 2026-04-01T12:59:11Z

Library/Homebrew/cmd/list.rb

          end
-          if !args.formula? && Cask::Caskroom.any_casks_installed?
-            ohai "Casks" if $stdout.tty? && !args.cask?
-            system_command! "ls", args: [*ls_args, Cask::Caskroom.path], print_stdout: true


Not using ls here any more is a behaviour change.

Yeah for consistency I wanted to use the same method everywhere since I'm getting different results when running brew list and brew info --installed. I could revert this to continue to use ls but maybe do a similar check for the metadata directory as well instead of this?

Yeh, this is similar to comment below "I'm vaguely concerned we're just papering over an actual problem here. This doesn't seem consistent with how formulae are treated any more?"

I want to make sure we keep using ls but also that we map how we do things with formulae. It may be there's too much deviation here to do that precisely.

At least we should e.g. add some logic to brew cleanup and/or brew doctor so we don't just silently ignore this broken Cask directories.

MikeMcQuaid · 2026-04-01T13:00:07Z

Library/Homebrew/cmd/update-report.rb

@@ -938,7 +938,7 @@ def installed?(formula)

  sig { params(cask: String).returns(T::Boolean) }
  def cask_installed?(cask)


Maybe worth just inlining this method?

MikeMcQuaid · 2026-04-01T13:00:41Z

Library/Homebrew/list.sh

-    then
-      ohai "Casks"
-    fi
+    # Filter to only casks with installed metadata, consistent with


I'm vaguely concerned we're just papering over an actual problem here. This doesn't seem consistent with how formulae are treated any more?

The actual problem of these directories existing without metadata? I haven't been able to track it down yet but I've seen it in the wild before. I was hoping to make the different ways to list casks consistent at least since brew list and brew info --installed are returning different things at the moment. I could also go into the other direction and just always consider a Cask installed if the directory exists and don't look at the metadata just that these are consistent across functions. I think formulae just use ls so that maybe is the way to go?

Yeh, that feels like the actual problem. I believe you that it occurs in the wild, it just feels like we're moving further in the direction of silently ignoring the problem rather than making it both non-fatal and warning (with brew doctor or opoo warning messages) or self-healing (with brew cleanup/brew autoremove).

Thoughts?

Yeah I think you are right. I made commits to add checks in brew doctor and brew cleanup (I can also do autoremove, I don't think I've ever used that before). I can remove the bits where we are messing with the heuristics of cask installs and just keep that last commit behind then?

I can remove the bits where we are messing with the heuristics of cask installs and just keep that last commit behind then?

Yeh, I think so. I think it's worth looking at the existing PRs you opened and re-reviewing in the context of this conversation so that we can both slim this one down and figure out ways to fix this somewhat automatically.

Note that brew cleanup runs periodically automatically (unless disabled) so this may also help us here.

- Extract duplicated caskfile glob into private `caskfile?` method - Fix comment: "consistent with" → "called by" - Restore `ls` for cask output in Ruby path to preserve -l/-t formatting - Inline `ReporterHub#cask_installed?` at call sites - Update test stubs to match Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Rather than silently filtering stale Caskroom directories (those without valid installed metadata), surface them to the user: - Add `Cask::Caskroom.stale_cask_dirs` to find leftover directories - Add `check_cask_stale_dirs` doctor check that warns and suggests cleanup - Add `cleanup_stale_cask_dirs` to `brew cleanup` to remove them - Add unit tests for `stale_cask_dirs` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

MikeMcQuaid · 2026-04-01T15:57:35Z

Library/Homebrew/list.sh

-    then
-      ohai "Casks"
-    fi
+    # Filter to only casks with installed metadata, consistent with


I can remove the bits where we are messing with the heuristics of cask installs and just keep that last commit behind then?

Yeh, I think so. I think it's worth looking at the existing PRs you opened and re-reviewing in the context of this conversation so that we can both slim this one down and figure out ways to fix this somewhat automatically.

Note that brew cleanup runs periodically automatically (unless disabled) so this may also help us here.

MikeMcQuaid · 2026-04-01T15:58:03Z

Library/Homebrew/cleanup.rb

        end
        Cleanup.autoremove(dry_run: dry_run?) unless Homebrew::EnvConfig.no_autoremove?

+        cleanup_stale_cask_dirs


Worth checking if this will run in the periodic cleanup. If not: it should!

MikeMcQuaid · 2026-04-01T15:58:51Z

Library/Homebrew/cask/caskroom.rb

+
+    # Return tokens for Caskroom directories that have no valid installed caskfile.
+    sig { returns(T::Array[String]) }
+    def self.stale_cask_dirs


Suggested change

def self.stale_cask_dirs

def self.corrupt_cask_dirs

or something?

If my understanding is right: they are missing essential/expected metadata?

koddsson marked this pull request as ready for review April 1, 2026 12:31

Copilot AI review requested due to automatic review settings April 1, 2026 12:31

Copilot started reviewing on behalf of koddsson April 1, 2026 12:32 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

koddsson force-pushed the consistent-cask-installed-check branch from a02777b to 230c29b Compare April 1, 2026 12:50

koddsson requested a review from Copilot April 1, 2026 12:51

Copilot started reviewing on behalf of koddsson April 1, 2026 12:52 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

MikeMcQuaid reviewed Apr 1, 2026

View reviewed changes

koddsson and others added 2 commits April 1, 2026 13:47

MikeMcQuaid reviewed Apr 1, 2026

View reviewed changes

	# Check if a cask with the given token is installed, consistent with {Cask#installed?}.
	# Check if a cask with the given token is installed, called by {Cask#installed?}.

		@@ -938,7 +938,7 @@ def installed?(formula)

		sig { params(cask: String).returns(T::Boolean) }
		def cask_installed?(cask)

Uh oh!

Conversation

koddsson commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koddsson commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koddsson commented Apr 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

koddsson commented Apr 1, 2026 •

edited

Loading

koddsson commented Apr 1, 2026 •

edited

Loading