Skip to content

advisory-match: emit split affected intervals - #24223

Merged
p-linnane merged 1 commit into
mainfrom
advisory-split-intervals
Oct 11, 2026
Merged

p-linnane merged 1 commit into
mainfrom
advisory-split-intervals

Conversation

@p-linnane

Copy link
Copy Markdown
Contributor

brew advisory-match held any history that left an affected range and later returned as an unrepresentable interval, so those candidates were never emitted. It now emits every affected interval across the latest macOS and Linux on ARM and Intel; reconciliation still can't fill reviewed gaps, expand coverage or reopen a range.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

Codex (GPT-6 Astra) drafted the change and tests and ran brew lgtm --online; Claude Code (Opus 5.5) reviewed it and ran brew lgtm --online on this commit. I reviewed the diff.


Reconstruct every affected interval from complete formula history and
union known intervals across macOS and Linux on ARM and Intel, so
histories that leave and re-enter an affected range are no longer held
as unrepresentable. Reconciliation still cannot fill reviewed gaps,
expand coverage or reopen a terminal range.

Signed-off-by: Patrick Linnane <patrick@linnane.io>
@p-linnane
p-linnane added this pull request to stack #24226 October 11, 2026 06:15
@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8c807341-610f-4c0e-826b-96eb7e68e766

📥 Commits

Reviewing files that changed from the base of the PR and between b922623 and 49036b9.


📒 Files selected for processing (7)
  • Library/Homebrew/dev-cmd/advisory-match.rb
  • Library/Homebrew/test/dev-cmd/advisory-match_spec.rb
  • Library/Homebrew/test/vulns/match_spec.rb
  • Library/Homebrew/test/vulns/osv_export_spec.rb
  • Library/Homebrew/vulns/match.rb
  • Library/Homebrew/vulns/osv_export.rb
  • docs/Advisory-Matching.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📜 Recent review details
⏰ Context from checks skipped due to timeout. (8)
  • GitHub Check: test-bot (macOS arm64, padded prefix)
  • GitHub Check: test-bot (Linux Homebrew glibc)
  • GitHub Check: docker (x86_64 Ubuntu 24.04)
  • GitHub Check: docker (arm64 Ubuntu 24.04)
  • GitHub Check: tests (macOS, 1/2)
  • GitHub Check: tests (online, 1/2)
  • GitHub Check: tests (macOS, 2/2)
  • GitHub Check: tests (online, 2/2)

🧰 Additional context used
📚 Code guidelines (1)
docs/AGENTS.md — auto-discovered

📓 Path-based instructions (1)
Source excerpt: Use the system Homebrew `brew bundle exec ...`

📄 CodeRabbit inference engine (docs/AGENTS.md)

Files:

  • docs/Advisory-Matching.md

🪛 LanguageTool
docs/Advisory-Matching.md

[style] ~18-~18: ‘new record’ might be wordy. Consider a shorter alternative.
Context: ...e or uncomparable history can prevent a new record from being emitted. Disjoint affected r...

(EN_WORDINESS_PREMIUM_NEW_RECORD)


🔇 Additional comments (1)
Library/Homebrew/vulns/match.rb (1)

75-92: Fix Interval.union so an open interval does not drop later intervals.

The loop handles a merged open interval (previous.fixed.nil?) with no else branch. Any later interval is then skipped without an error. An open interval already covers every later version, so the result stays correct. One case breaks: an open interval sorts before a closed interval that starts at the same version. If a later platform's introduced is lower, the sort places it first and the merge handles it. The merge step also loses the open state. Suppose previous is closed and the incoming interval overlaps it and is open, so interval.fixed is nil. Then last_fixed stays nil and the merged interval becomes open. That is correct. I found no realistic failure, so no change is required.



📝 Summary

Summary by CodeRabbit

  • New Features

    • Advisory matching now supports multiple affected version ranges, including disjoint ranges, across supported macOS and Linux platforms.
    • Complete history can identify when a package was never affected and prevent unsupported ranges from being exported as fixed.
  • Bug Fixes

    • Reconciliation preserves unaffected gaps and reviewed ranges, and holds records for review when platform history or evidence is incomplete.
    • Range repairs avoid expanding ranges, filling safe gaps, or reopening completed ranges.
  • Documentation

    • Updated advisory-matching guidance to explain multi-range histories, reconciliation limits, and review requirements.

Walkthrough

Advisory history now supports multiple affected intervals. New records use combined history from macOS and Linux on ARM and Intel. Existing records can be reconciled against those histories when their ranges meet the validation rules.

Changes

Advisory history reconciliation

Layer / File(s) Summary
Reconstruct affected intervals
Library/Homebrew/vulns/match.rb, Library/Homebrew/test/vulns/match_spec.rb
History reconstruction now returns validated intervals. It emits events for each interval and checks patch versions against all reconstructed intervals.
Combine platform histories for new records
Library/Homebrew/dev-cmd/advisory-match.rb, Library/Homebrew/test/dev-cmd/advisory-match_spec.rb, docs/Advisory-Matching.md
New-record processing queries macOS and Linux on ARM and Intel, combines the results, and holds records when history or provenance is unresolved. Tests cover split histories and unavailable platform results.
Reconcile existing records and export ranges
Library/Homebrew/dev-cmd/advisory-match.rb, Library/Homebrew/vulns/osv_export.rb, Library/Homebrew/test/dev-cmd/advisory-match_spec.rb, Library/Homebrew/test/vulns/osv_export_spec.rb, docs/Advisory-Matching.md
Reconciliation accepts valid terminal multi-interval ranges and keeps proposed intervals within reviewed intervals. Initial-history repairs preserve complete incoming histories while retaining valid existing ranges.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~50 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant AdvisoryMatch
  participant PlatformView
  participant Match
  participant ReconciledHistory
  AdvisoryMatch->>PlatformView: Load history for a platform
  AdvisoryMatch->>Match: Reconcile platform observations
  Match-->>AdvisoryMatch: Return platform history
  AdvisoryMatch->>ReconciledHistory: Combine platform results
  ReconciledHistory-->>AdvisoryMatch: Return combined intervals
Loading

Suggested reviewers: andrew


Merge Risk | ⚪ Minimal · up to 49036

Merge Risk: ⚪ Minimal · up to 49036

The change lets advisory-match emit split affected intervals, with tests and docs updated alongside. No concrete merge-blocking issue was identified in the supplied material.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 49036

The change affects security advisory coverage, but the inspected paths preserve provenance checks and prevent reconciliation from reopening ranges, expanding reviewed coverage, or filling reviewed gaps. No introduced security issue was established. Publication behavior and broader deployment coverage remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected security asset is advisory coverage in records written to the selected output directory. Incorrect reconstruction could misstate affected package versions. Propagation into published feeds or downstream security decisions depends on publication behavior not established by the reviewed source.

Trust Boundaries and Controls

  • observed — Formula and upstream advisory evidence influence reconstructed coverage, but a computed history does not independently authorize mutation. Alias-target ambiguity, unsupported records, provenance mismatch, unresolved history, and interval containment failures block reconciliation before its existing filesystem sink.

Resilience and Maintainability Implications

  • observed — Historical load failures, unknown subjects, alias conflicts, overrides, and advisory patches produce unresolved outcomes rather than an inferred safe gap. A never-affected deletion follows the combined platform decision and existing-record validation, not an isolated missing query result.

Hardening Proposals

  • proposed — As separate hardening of the pre-existing persistence path, consider atomic record replacement and serialized validation/mutation to protect advisory integrity during interruption or concurrent authoring. This is not an observed PR-introduced finding.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: emitting split affected intervals in advisory matching.
Description check Passed The description directly explains the interval-emission change and the reconciliation limits, and it is consistent with the changeset.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@p-linnane
p-linnane added this pull request to the merge queue Oct 11, 2026
Merged via the queue into main with commit c1b0558 Oct 11, 2026
54 checks passed
@p-linnane
p-linnane deleted the advisory-split-intervals branch October 11, 2026 16:16
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.

2 participants