livecheck: really use referenced livecheck options - #23775
Conversation
When I introduced `Livecheck::Options` I added some logic in `Livecheck::latest_version` that was intended to fall back to a referenced package's options but that will never happen, as `livecheck.options` is never `nil`. Even if we used `livecheck.options.presence`, it wouldn't work as expected because we want to combine options from the initial `livecheck` block with the referenced check (including overriding an option that they both set). Basically, we should start with the referenced `livecheck` block options (if any) and then merge options from the initial `livecheck` block (so we can override, if necessary). This was my original idea of how this should work (and part of the reason why I added `Options#merge`) but I didn't catch this oversight until I was testing a referenced `livecheck` block recently. This will become a little trickier if we add support for multi-level references (i.e., A references B which references C and options can be set in each) but this is fairly straightforward for now. This addresses the issue by reworking how we handle referenced options to ensure we merge when a reference is used in a `livecheck` block. This also adds tests that fail without this fix as well as other tests that provide 100% coverage for reference-related code in `latest_version`.
There was a problem hiding this comment.
馃煛 Changes recommended
Conflicting POST options can cause a referenced value to override the initial block unexpectedly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes inheritance and overriding of Livecheck URL options from referenced formulae and casks.
Changes:
- Merges referenced and local Livecheck options.
- Adds exhaustive reference handling and regression coverage.
File summaries
| File | Description |
|---|---|
Library/Homebrew/livecheck/livecheck.rb |
Merges referenced options and handles exhaustive reference types. |
Library/Homebrew/test/livecheck/livecheck_spec.rb |
Tests option inheritance, overrides, and default checks. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| referenced_livecheck = referenced_formula_or_cask&.livecheck | ||
|
|
||
| livecheck_options = livecheck.options || referenced_livecheck&.options | ||
| livecheck_options = referenced_livecheck&.options&.merge(livecheck.options) || livecheck.options |
There was a problem hiding this comment.
Hmm, something like this would be a suitable workaround but this also reminds me that we can't set a nil option value in the initial livecheck block to unset an option that's inherited from a referenced livecheck block. In this context, livecheck.option and referenced_livecheck.option would both be Option objects and #merge calls #to_h internally, which calls #to_hash and that method filters out nil values. This is a quirk of how Sorbet has implemented #to_hash for T::Struct (iirc, something to do with what Stripe's code requires), so I may need to manually implement that method to work around this limitation.
However, that's something for a follow-up PR (we only have two livecheck blocks that use a reference with an override and both only use regex). I'll update this commit to include a related workaround for the flagged issue in a moment.
Edit: I ended up going down a different path to address this issue inside of Options instead. I'll push a commit sometime tomorrow when I'm done.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Looks good when 馃煝, thanks!
brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?I implemented this fix by hand and confirmed it through manual testing. Afterward, I used Claude Code (Opus 5 high) to review the change and implement tests to cover the modified line.
The produced tests covered the primary scenarios involved in the fix but not everything. The tests exercised the code in a basic way (to produce coverage) but didn't check debug or [verbose] JSON output to confirm that the related code works as expected end-to-end. I reworked those tests until I was satisfied with them and added more tests to better test the fix.
I then expanded the tests to provide 100% coverage for all the logic in
latest_versionthat relates to referencedlivecheckblocks and used Claude Code to review the final changes. Overall, AI was helpful for reviewing but I could have implemented the tests entirely by hand from the start (though having AI produce a starting point saved me some typing).When I introduced
Livecheck::OptionsI added some logic inLivecheck::latest_versionthat was intended to fall back to a referenced package's options but that will never happen, aslivecheck.optionsis nevernil. Even if we usedlivecheck.options.presence, it wouldn't work as expected because we want to combine options from the initiallivecheckblock with the referenced check (including overriding an option that they both set). Basically, we should start with the referencedlivecheckblock options (if any) and then merge options from the initiallivecheckblock (so we can override, if necessary).This was my original idea of how this should work (and part of the reason why I added
Options#merge) but I didn't catch this oversight until I was testing a referencedlivecheckblock recently. This will become a little trickier if we add support for multi-level references (i.e., A references B which references C and options can be set in each) but this is fairly straightforward for now.This addresses the issue by reworking how we handle referenced options to ensure we merge when a reference is used in a
livecheckblock. This also adds tests that fail without this fix as well as other tests that provide 100% coverage for reference-related code inlatest_version.[If there's any question, the
T.absurd(ref_formula_or_cask) # simplecov:disableelse branches only serve the purpose of getting those case statements to 100% coverage (which isn't possible without anelsebranch). This is something you'll see again in an upcoming PR where I'm expanding livecheck test coverage further.]You can reproduce this issue by running
brew livecheck --debug tableau(which outputs "URL Options: {user_agent: :browser}") and then compare the output tobrew livecheck --debug tableau-prep(which omits the user agent option). With the changes in this PR, the user agent option from the referenced check is also present fortableau-prep, as intended. [Thetableau-prepcheck intermittently works without the:browseruser agent but this will become a more notable issue when we deprecate the user agent fallback logic in the near future, as the check would fail in that scenario.]