formula_auditor: audit no_autobump! stanza on version bump#21550
formula_auditor: audit no_autobump! stanza on version bump#21550
no_autobump! stanza on version bump#21550Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Homebrew’s auditing to flag misuse of the no_autobump! because: :requires_manual_review reason not only for new formulae/casks, but also when an existing formula’s version is bumped (using Git history to detect the version change).
Changes:
- Rename/expand the shared
no_autobumpaudit helper to support both “new package” and “existing package on version bump” messaging. - Update
FormulaAuditor#audit_no_autobumpto run an additional audit when the stable version differs fromorigin/HEAD. - Adjust cask auditing and add/reshape formula auditor specs around the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
Library/Homebrew/utils/shared_audits.rb |
Generalizes the no_autobump audit message helper and changes the emitted warning text. |
Library/Homebrew/formula_auditor.rb |
Adds Git-based version-change detection to trigger no_autobump auditing on version bumps. |
Library/Homebrew/cask/audit.rb |
Switches to the new shared audit helper for new cask no_autobump validation. |
Library/Homebrew/test/formula_auditor_spec.rb |
Adds git-backed test setup and new cases for the version-bump no_autobump audit behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "`:requires_manual_review` is a temporary reason intended for existing packages, use a different reason instead." | ||
| msg = new_package ? "use a different reason instead" : "change or remove autobump exclusion reason" | ||
|
|
||
| "`:requires_manual_review` is a temporary to-be deprecated reason, #{msg}." |
There was a problem hiding this comment.
The warning text reads awkwardly ("temporary to-be deprecated reason"). Consider rephrasing to standard grammar (e.g., "temporary reason slated for deprecation") so the audit output is clear and consistent.
| "`:requires_manual_review` is a temporary to-be deprecated reason, #{msg}." | |
| "`:requires_manual_review` is a temporary reason slated for deprecation, #{msg}." |
There was a problem hiding this comment.
Can you add a #odeprecated somewhere to be uncommented when we're ready to deprecate for the next major/minor release?
I skipped cask auditor as there is no casks that use `:requires_manual_review` Signed-off-by: botantony <antonsm21@gmail.com>
d71258f to
ea885ba
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
@botantony Can you explain a bit more what this is doing any why? The PR description doesn't really provide any context. Thanks!
| "`:requires_manual_review` is a temporary reason intended for existing packages, use a different reason instead." | ||
| msg = new_package ? "use a different reason instead" : "change or remove autobump exclusion reason" | ||
|
|
||
| "`:requires_manual_review` is a temporary to-be deprecated reason, #{msg}." |
There was a problem hiding this comment.
Can you add a #odeprecated somewhere to be uncommented when we're ready to deprecate for the next major/minor release?
Signed-off-by: botantony <antonsm21@gmail.com>
|
@MikeMcQuaid right now there are a lot of formulae in core that use See https://machomebrew.slack.com/archives/C06G173B7/p1770641042793459, CC @SMillerDev |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
@botantony Sorry, I don't think it makes sense to push this work onto contributors. I wouldn't know based on the instructions from you above or in this PR what needs to be done and how I'd find out. Instead, I'd suggest either you do this work or create an issue to track progress and documentation so maintainers can do it.
|
If we just need visibility on this, without causing CI to fail - what about triaging with a label? It's immediately obvious that a review could be beneficial, but is optional for the reviewer. I'm not sure too what extent labels just become noise? |
|
Maybe we can just have the style check remove this version of the |
@SMillerDev how do we know if the autobump will work? I'm maybe missing something but I thought the point of the change was to deliberately have a human verify that these actually work? |
|
The build will fail if the autobump results differ from the actual version right? Or is that only for casks? |
brew lgtm(style, typechecking and tests) with your changes locally?I skipped cask auditor as there is no casks that use
:requires_manual_review