bump-cask-pr: handle nil cask version, add tests

[samford]

• 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 for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

The shortened_version method in the bump-cask-pr command can produce an error if cask.version is nil. For example, this can happen on Linux when a cask only sets the version in macOS on_system blocks. This modifies the method to explicitly raise a CaskInvalidError when cask.version is nil, as we need to know the existing version to be able to determine how to handle the new version. [It's possible to fall back to the full version instead but that can allow certain types of duplicate PRs when cask.version is or isn't nil on a given host.]

This also adds tests for the shortened_version method, bringing it to 100% coverage.

---

For what it's worth, to allow bump-cask-pr to bump casks where the version is only set in macOS on_system blocks, we would need to simulate macOS earlier in bump-cask-pr (i.e., when defining cask in run). Some casks loudly fail early as invalid, while others may only end up with invalid values like a nil version. This is something for a follow-up PR but I'll tinker with this to see if we can implement it now or if we have to wait for more reliable indicators of OS requirements (something else that I'm working on).

[MikeMcQuaid]

Thanks @samford!

[samford]

Thinking about this some more, we shouldn't default to the full version when cask.version is nil, as this could potentially allow bump-cask-pr to create duplicate PRs. For example:

• A version bump PR has already been created but not merged, updating the cask from 1.2.2,1 to 1.2.3,1. This PR was created by running bump-cask-pr in an environment where cask.version is not nil, so the before_comma version was used in the commit/PR title (i.e., "cask_name 1.2.3") because version.before_comma and cask_version.before_comma differ.
• A user runs bump-cask-pr for 1.2.3,1 in an environment where cask.version is nil (i.e., version is only set in OS-specific on_system blocks that don't apply to the host). bump-cask-pr defaults to the full version (1.2.3,1) and uses that to identify duplicate PRs, so it doesn't see the less-specific "cask_name 1.2.3" as a duplicate.
• bump-cask-pr mistakenly creates a duplicate PR using the full version

I think bump-cask-pr can trigger an autoupdate before bumping sometimes but that would only prevent this scenario if the version bump PR has already been merged (i.e., the update wouldn't lead to any changes to the cask file).

---

With that in mind, I think a nil cask.version value should continue to be an error condition, as we can't properly determine how to handle the version without knowing the existing cask.version.before_comma value. I've reworked this to raise, as we need to handle a nil cask.version value in some way. [This is something that should be caught by brew typecheck but, for whatever reason, the version method in the generated sorbet/cask/cask.rbi file has a T.untyped return type instead of the T.nilable(DSL::Version) return type for Cask::DSL.version.]

If we want to be able to bump these types of macOS-only casks on Linux, we'll have to simulate OS/arch earlier in bump-cask-pr. Until then, this error will avoid certain types of duplicate cask PRs.

[MikeMcQuaid]

Thanks @samford!

If we want to be able to bump these types of macOS-only casks on Linux, we'll have to simulate OS/arch earlier in bump-cask-pr. Until then, this error will avoid certain types of duplicate cask PRs.

Personally, I'd rather that we actually saw the duplicate cask PRs before doing this. I think bumping on Linux is higher priority than fixing a bug that we haven't yet seen in the wild (although please correct me if wrong there).

[samford]

Personally, I'd rather that we actually saw the duplicate cask PRs before doing this. I think bumping on Linux is higher priority than fixing a bug that we haven't yet seen in the wild (although please correct me if wrong there).

I may not have been clear in my previous comment but the current state of this PR simply maintains the status quo [in terms of shortened_version logic]. cask.version.before_comma currently produces an error when version is nil and this PR explicitly raises an error in that scenario as a way of handling the NilClass part of the cask.version T.nilable(DSL::Version) return type. brew typecheck doesn't currently surface this type issue (as the cask.rbi return type for version is incorrectly T.untyped) but it will after the changes in Doug's forwardables-lookup-table branch land. [I'm working on a PR to upgrade Cask::DSL to typed: strict, add missing method signatures, and fix all the resulting type errors, so this PR is a small part of that effort.]

Regarding the duplicate PR issue, it may not be something that would happen often (i.e., bumping this category of macOS-only cask on Linux hasn't been possible before, I don't imagine many Linux users would think to bump a macOS cask, etc.) but it's something that will predictably occur under those specific circumstances. We have some very enthusiastic contributors in homebrew/cask, so I wouldn't bet on this issue not appearing at some point.

If we had a pressing need to enable bumping this category of macOS-only casks on Linux, then I could understand taking a calculated risk. Unless that's the case, I think we should just wait until we can ensure that this type of bump works the same on Linux as it does on macOS. Outside of duplicate PRs, there may be other Linux-specific issues that I haven't seen or thought of yet (off the top of my head, brew audit also needs to be simulated and it can fail with /usr/bin/env: ‘plutil’: No such file or directory when simulating macOS on Linux).

Having bump-cask-pr work the same on Linux as it does on macOS is something that I would like to make possible (though I have no personal need for it) but it will be after we can reasonably identify the OS/architecture for a given cask (so we can expand OS/arch simulation but only for appropriate OS/arch targets). At the very least, it will be after we can adequately detect on_system method call usage (via UsesOnSystem) and maybe after all macOS-only casks have an explicit macOS dependency (once I'm done with UsesOnSystem, I'll create a PR to propose allowing depends_on :macos and depends_on :linux in casks). I'm also working on resolving issues with macOS-only casks that Linux can't read, so that's another hurdle (but specific to certain casks).

Long story short: defaulting to version when cask.version is nil wasn't the right approach but the bump-cask-pr story on Linux will improve when the right pieces are in place to do it correctly.

[MikeMcQuaid]

If we had a pressing need to enable bumping this category of macOS-only casks on Linux, then I could understand taking a calculated risk.

I think we have a more pressing need for this known shortcoming than avoiding duplicates which may be likely be is still an unknown.

Long story short: defaulting to version when cask.version is nil wasn't the right approach but the bump-cask-pr story on Linux will improve when the right pieces are in place to do it correctly.

Sounds good. I'm fine to merge as-is if desired. Thanks @samford!

[samford]

I think we have a more pressing need for this known shortcoming than avoiding duplicates which may be likely be is still an unknown.

If it's not clear from the above, the duplicate PR issue is something that can be reliably replicated under the described conditions. The only mitigating factor is that it's not the most likely set of circumstances to occur right now (though that could change at any time).

If you want to replicate the issue on Linux, you can recreate the conditions using a recent cockatrice update as an example (it has a version with commas and isn't in the autobump list):

1. Apply the bump-cask-pr changes in a4b5d62
2. cd ~/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-cask/
3. git checkout b2607fb66ceedfcf0d66851a111c2875d2740c67 (this is the commit before the "cockatrice 2.10.2" version bump commit)
4. brew bump-cask-pr --no-fork --write-only --version="2.10.2,2025-04-03,Omenpath,2.10.2" cockatrice

check_pull_requests won't error because the "cockatrice 2.10.2" PR isn't seen as a duplicate (i.e., cask.version.before_comma is nil on Linux, so the full version would be used when checking for duplicate PRs):

==> git add /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/cockatrice.rb
==> git checkout --no-track -b bump-cockatrice-2.10.2-2025-04-03-Omenpath-2.10.2 origin/master
==> git commit --no-edit --verbose --message='cockatrice 2.10.2,2025-04-03,Omenpath,2.10.2' -- /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/cockatrice.rb
==> git push --set-upstream https://github.com/Homebrew/homebrew-cask bump-cockatrice-2.10.2-2025-04-03-Omenpath-2.10.2:bump-cockatrice-2.10.2-2025-04-03-Omenpath-2.10.2
==> git checkout --quiet -
==> create pull request with GitHub API (base branch: master)

The expected output is:

Error: These  pull requests are duplicates:
cockatrice 2.10.2 https://github.com/Homebrew/homebrew-cask/pull/207770
Duplicate PRs must not be opened.
Manually open these PRs if you are sure that they are not duplicates (and tell us that in the PR).

The existing shortened_version logic produces the following error, which is effectively preventing Linux users from being able to bump these types of macOS casks (thankfully so, as we need to simulate macOS earlier to do it correctly):

Error: undefined method `before_comma' for nil
Warning: Removed Sorbet lines from backtrace!
Rerun with `--verbose` to see the original backtrace
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/dev-cmd/bump-cask-pr.rb:181:in `shortened_version'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/dev-cmd/bump-cask-pr.rb:142:in `run'
/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:95:in `<main>'

The current state of this PR maintains the existing logic but handles the error condition by raising a CaskInvalidError:

Error: Cask 'cockatrice' definition is invalid: invalid 'version' value: nil

Hopefully this clarifies the duplicate PR issue and why I backtracked from that initial idea.

I'm fine to merge as-is if desired.

Thanks! It looks like this PR has gotten into a bad state where it's not recognizing subsequent pushes to the branch but let me see if closing/reopening it will allow me to merge this (otherwise I'll have to recreate the PR).

[samford]

Ha yeah, the reopen button is disabled because "The bump-cask-pr-rework-shortened_version branch was force-pushed or recreated" (I rebased this an hour ago). Oh well, I can just bundle this fix into my type-related work on Cask::DSL (which I'll eventually create a PR for sometime after I'm done with the OnSystem/depends_on work).