Conversation
There was a problem hiding this comment.
Pull request overview
Updates brew audit’s cask livecheck version validation to honor the newer livecheck throttle days: setting (including when throttling is inherited via a referenced cask), aligning behavior with brew livecheck and bump commands.
Changes:
- Extend cask audit livecheck logic to consider
throttle_days(and referenced-caskthrottle_days) when selectinglatestvslatest_throttled. - Add cask audit specs covering
throttle days:on a cask and via a referenced cask.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
Library/Homebrew/cask/audit.rb |
Makes audit’s livecheck version comparison respect throttle days: (including from referenced casks). |
Library/Homebrew/test/cask/audit_spec.rb |
Adds new spec contexts for throttle days: and referenced-throttle-days scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throttle_days ||= referenced_cask.livecheck.throttle_days | ||
| end | ||
|
|
||
| latest_version = (throttle || throttle_days) ? result[:latest_throttled] : result[:latest] |
There was a problem hiding this comment.
When throttling is enabled (either throttle or throttle days:), Homebrew::Livecheck.latest_version can legitimately return latest_throttled: nil (e.g. when the throttle interval hasn’t elapsed). In that case this code sets latest_version to nil and then emits an audit error like differs from '', which doesn’t respect throttling and produces a confusing message. Handle the latest_throttled.nil? case explicitly (e.g. treat the audit as throttled/skip without adding an error, or fall back to comparing against the current version).
| latest_version = (throttle || throttle_days) ? result[:latest_throttled] : result[:latest] | |
| throttled = throttle || throttle_days | |
| latest_version = throttled ? result[:latest_throttled] : result[:latest] | |
| if throttled && latest_version.nil? | |
| @livecheck_result = :skip | |
| return @livecheck_result | |
| end |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Looks good when you're happy and have all the reviews you want!
samford
left a comment
There was a problem hiding this comment.
This makes sense to me but this change doesn't appear to fully resolve the audit issue when tested on the linked alma homebrew-cask PR branch, as livecheck is giving a latest_throttled version of 0.0.750 (the latest version meeting the throttle rate) instead of 0.0.762 (the latest version, as the throttle days interval elapsed).
This happens because time-based throttling uses Git commit timestamps to identify when the package was last updated but formula_or_cask_last_updated_timestamp returns the timestamp for the 0.0.762 version update commit in the PR branch, so the throttle interval isn't viewed as having elapsed when auditing the cask. This approach works fine on the main branch (e.g., when running brew bump to update packages) but not when on a version update branch.
One solution would be to modify the git log command in formula_or_cask_last_commit_timestamp to only identify commits on the repository's HEAD branch (i.e., main for homebrew-cask). I've confirmed that this works as expected in practice, so I'll push a commit with this change if that makes sense to you.
brew lgtm(style, typechecking and tests) with your changes locally?I used Codex to help find the discrepancy between the
brew livecheckandbrew auditbehaviour.With the recent introduction of the
daysparameter tolivecheck throttle, the behaviour has been updated to be correctly reflected acrossbrew bumpandbrew livecheck- howeverbrew auditwas not receiving the change, as discovered in Homebrew/homebrew-cask#260573