Skip to content

HeaderMatch: pass all headers into strategy block#22124

Merged
MikeMcQuaid merged 2 commits intomainfrom
livecheck/header_match-allow-all_headers-argument
May 3, 2026
Merged

HeaderMatch: pass all headers into strategy block#22124
MikeMcQuaid merged 2 commits intomainfrom
livecheck/header_match-allow-all_headers-argument

Conversation

@samford
Copy link
Copy Markdown
Member

@samford samford commented May 2, 2026


  • 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? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • 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. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes. Non-maintainers may only have one AI-assisted/generated PR open at a time.

This modifies livecheck's HeaderMatch strategy to allow strategy blocks to use an all_headers argument to receive the original array of response headers rather than a merged hash that only includes the last instance of a given header. Over the years, we've encountered a few checks where a server makes multiple redirections (i.e., there are multiple responses with a Location header) but we need to target a Location header that isn't the last occurrence. This hasn't been possible due to the merged_headers setup, so we've simply worked around it.

We recently ran into this shortcoming again in homebrew-cask but there isn't a way to work around it this time, so this is now strictly necessary. This implementation borrows from my prior art in the Sparkle strategy, where the name of the first argument to a strategy block is strictly enforced and the name dictates what value is provided. I've ensured that all HeaderMatch strategy blocks in core/cask use a headers argument, so they will continue working as expected.

Copilot AI review requested due to automatic review settings May 2, 2026 16:56
@samford samford mentioned this pull request May 2, 2026
10 tasks
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the livecheck HeaderMatch strategy so strategy blocks can opt into receiving either a merged headers hash (headers) or the full array of per-response headers (all_headers), enabling version extraction from non-final redirect headers.

Changes:

  • Change HeaderMatch.versions_from_content to accept an array of response header hashes and provide either merged headers or all headers to the strategy block based on the first block argument name.
  • Update HeaderMatch.find_versions to pass the full headers array through to versions_from_content.
  • Expand/update HeaderMatch strategy specs to cover the new all_headers behavior and error handling.

Reviewed changes

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

File Description
Library/Homebrew/livecheck/strategy/header_match.rb Adjusts strategy logic to pass full headers array or merged hash into the strategy block and updates header parsing flow.
Library/Homebrew/test/livecheck/strategy/header_match_spec.rb Updates tests for the new array-of-headers input and adds coverage for all_headers block behavior and signature validation.

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

Comment thread Library/Homebrew/test/livecheck/strategy/header_match_spec.rb
Comment thread Library/Homebrew/livecheck/strategy/header_match.rb Outdated
Comment thread Library/Homebrew/livecheck/strategy/header_match.rb
Comment thread Library/Homebrew/livecheck/strategy/header_match.rb Outdated
@samford samford force-pushed the livecheck/header_match-allow-all_headers-argument branch from 8cca5b9 to fb8ecba Compare May 2, 2026 17:38
samford added 2 commits May 2, 2026 17:28
This modifies livecheck's `HeaderMatch` strategy to allow `strategy`
blocks to use an `all_headers` argument to receive the original array
of response headers rather than a merged hash that only includes the
last instance of a given header. Over the years, we've encountered a
few checks where a server makes multiple redirections (i.e., there are
multiple responses with a `Location` header) but we need to target a
`Location` header that isn't the last occurrence. This hasn't been
possible due to the `merged_headers` setup, so we've simply worked
around it.

We recently ran into this shortcoming again in homebrew-cask but there
isn't a way to work around it this time, so this is now strictly
necessary. This implementation borrows from my prior art in the
`Sparkle` strategy, where the name of the first argument to a
`strategy` block is strictly enforced and the name dictates what value
is provided. I've ensured that all `HeaderMatch` strategy blocks in
core/cask use a `headers` argument, so they will continue working as
expected.
This updates the `Sparkle` strategy's `strategy` block handling logic
to incorporate improvements that were identified as part of working on
`HeaderMatch` `strategy` block handling.
@samford samford force-pushed the livecheck/header_match-allow-all_headers-argument branch from fb8ecba to e26e98d Compare May 2, 2026 21:28
Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue May 3, 2026
Merged via the queue into main with commit 48d6030 May 3, 2026
34 of 36 checks passed
@MikeMcQuaid MikeMcQuaid deleted the livecheck/header_match-allow-all_headers-argument branch May 3, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants