fix(ci): spend logs guard requires the approval to cover the head's spend-log lines (#48904)

* fix(ci): spend logs guard requires the approval to cover the head's spend-log lines

An approving review survives later pushes (dismiss_stale_reviews_on_push is off on main), so a PR approved once could add more LiteLLM_SpendLogs schema or migration changes and the guard kept passing. The guard now reads the review's commit_id and passes only when the approval is on the head, or the head adds no spend-log line the approved commit did not already carry.

* fix(ci): keep approved_by untouched, add latest_approval beside it for the stale-approval check

---------

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

ci_cd/spend_logs_guard.py

@@ -4,7 +4,9 @@ needs an approving review from the spend-log owner before it can merge.
 CODEOWNERS can only match file paths, and every schema change edits the same schema.prisma copies,
 so a path rule would put every migration on the owner. This gate reads the diff instead: when no
 added line in the watched files mentions the table it passes at once, otherwise it passes only once
-the owner's latest review on the pull request is an approval. The workflow re-runs on every review.
+the owner's latest review on the pull request is an approval that still covers every spend-log line
+in the head (approvals survive pushes, so a later commit may not add spend-log lines the owner never
+saw). The workflow re-runs on every review.
 """
 
 from __future__ import annotations
@@ -13,7 +15,7 @@ import argparse
 import re
 import subprocess
 import sys
-from collections.abc import Sequence
+from collections.abc import Callable, Sequence
 from pathlib import Path
 from typing import Final
 
@@ -37,6 +39,7 @@ class Review(BaseModel):
     model_config = ConfigDict(frozen=True, extra="ignore")
     state: str
     user: Reviewer
+    commit_id: str | None = None
 
 
 _REVIEWS: Final = TypeAdapter(list[Review])
@@ -75,6 +78,24 @@ def approved_by(reviews: Sequence[Review], login: str) -> bool:
     return bool(states) and states[-1] == "APPROVED"
 
 
+def latest_approval(reviews: Sequence[Review], login: str) -> Review | None:
+    """`login`'s most recent APPROVED review on the pull request, the one GitHub keeps in force after pushes."""
+    own: Final = tuple(review for review in reviews if review.user.login == login and review.state == "APPROVED")
+    return own[-1] if own else None
+
+
+def approval_covers(
+    approval: Review, head: str, hits: Sequence[str], hits_at: Callable[[str], Sequence[str] | None]
+) -> bool:
+    """True when the approval is on the head, or the head adds no spend-log line the approved commit did not."""
+    if approval.commit_id is None:
+        return False
+    if approval.commit_id == head:
+        return True
+    reviewed: Final = hits_at(approval.commit_id)
+    return reviewed is not None and set(hits) <= set(reviewed)
+
+
 def added_diff(base: str, head: str) -> str:
     result: Final = subprocess.run(
         ("git", "diff", "--unified=0", "--no-color", f"{base}..{head}", "--", *WATCHED, MIGRATIONS_PREFIX),
@@ -85,6 +106,15 @@ def added_diff(base: str, head: str) -> str:
     return result.stdout
 
 
+def fetched(commit: str) -> bool:
+    result: Final = subprocess.run(("git", "fetch", "--no-tags", "--depth=1", "origin", commit), check=False)
+    return result.returncode == 0
+
+
+def reviewed_hits(base: str, commit: str) -> tuple[str, ...] | None:
+    return spend_log_hits(added_diff(base, commit)) if fetched(commit) else None
+
+
 class Arguments(BaseModel):
     model_config = ConfigDict(frozen=True)
     base: str
@@ -104,14 +134,21 @@ def main(argv: Sequence[str]) -> int:
     if not hits:
         sys.stdout.write("No added line in schema.prisma or a migration names LiteLLM_SpendLogs; nothing to review.\n")
         return 0
-    if approved_by(_REVIEWS.validate_json(args.reviews.read_bytes()), args.approver):
+    reviews: Final = _REVIEWS.validate_json(args.reviews.read_bytes())
+    approval: Final = latest_approval(reviews, args.approver) if approved_by(reviews, args.approver) else None
+    if approval is not None and approval_covers(
+        approval, args.head, hits, lambda commit: reviewed_hits(args.base, commit)
+    ):
         sys.stdout.write(f"LiteLLM_SpendLogs change approved by {args.approver}: {', '.join(hits)}\n")
         return 0
     listed: Final = "".join(f"  {hit}\n" for hit in hits)
-    sys.stdout.write(
+    reason: Final = (
         f"This pull request changes LiteLLM_SpendLogs and needs an approving review from @{args.approver}:\n"
-        f"{listed}The check re-runs when a review is submitted.\n"
+        if approval is None
+        else f"LiteLLM_SpendLogs lines changed after @{args.approver} approved {approval.commit_id}, "
+        f"a new approving review is needed:\n"
     )
+    sys.stdout.write(f"{reason}{listed}The check re-runs when a review is submitted.\n")
     return 1
 
 

tests/unit/test_spend_logs_guard.py

@@ -42,6 +42,10 @@ class TestSpendLogHits:
         assert guard.spend_log_hits(diff) == ("litellm/proxy/schema.prisma: a LiteLLM_SpendLogs",)
 
 
+def _review(login: str, state: str, commit_id: str | None = "head") -> "guard.Review":
+    return guard.Review(state=state, user=guard.Reviewer(login=login), commit_id=commit_id)
+
+
 class TestApprovedBy:
     def test_the_latest_review_of_the_approver_decides(self):
         reviews: Final = tuple(
@@ -61,6 +65,42 @@ class TestApprovedBy:
         assert guard.approved_by((), "kerry-berri") is False
 
 
+class TestLatestApproval:
+    def test_the_approvers_last_approved_review_is_returned_with_its_commit(self):
+        reviews: Final = (
+            _review("kerry-berri", "APPROVED", "old"),
+            _review("someone-else", "APPROVED", "head"),
+            _review("kerry-berri", "APPROVED", "new"),
+            _review("kerry-berri", "COMMENTED", "head"),
+        )
+        assert guard.latest_approval(reviews, "kerry-berri") is reviews[2]
+        assert guard.latest_approval(reviews[:2], "kerry-berri") is reviews[0]
+        assert guard.latest_approval(reviews[1:2], "kerry-berri") is None
+        assert guard.latest_approval((), "kerry-berri") is None
+
+
+class TestApprovalCovers:
+    _HITS: Final = ("schema.prisma: a LiteLLM_SpendLogs", "schema.prisma: b LiteLLM_SpendLogs")
+
+    def test_an_approval_on_the_head_covers_it_without_reading_the_approved_commit(self):
+        def unreadable(commit: str) -> None:
+            raise AssertionError(f"read {commit}")
+
+        assert guard.approval_covers(_review("kerry-berri", "APPROVED", "head"), "head", self._HITS, unreadable) is True
+
+    def test_an_older_approval_covers_the_head_only_when_it_saw_every_spend_log_line(self):
+        approval: Final = _review("kerry-berri", "APPROVED", "old")
+        assert guard.approval_covers(approval, "head", self._HITS, lambda commit: self._HITS) is True
+        assert guard.approval_covers(approval, "head", self._HITS[:1], lambda commit: self._HITS) is True
+        assert guard.approval_covers(approval, "head", self._HITS, lambda commit: self._HITS[:1]) is False
+
+    def test_an_approval_whose_commit_cannot_be_read_does_not_cover_the_head(self):
+        gone: Final = _review("kerry-berri", "APPROVED", "gone")
+        assert guard.approval_covers(gone, "head", self._HITS, lambda commit: None) is False
+        without_commit: Final = _review("kerry-berri", "APPROVED", None)
+        assert guard.approval_covers(without_commit, "head", self._HITS, lambda commit: self._HITS) is False
+
+
 class TestExitCode:
     def test_a_pull_request_without_spend_log_lines_passes_without_reviews(self, tmp_path, monkeypatch, capsys):
         monkeypatch.setattr(guard, "added_diff", lambda base, head: _diff("schema.prisma", "  foo String", 1))
@@ -75,5 +115,15 @@ class TestExitCode:
         reviews.write_text('[{"user": {"login": "someone-else"}, "state": "APPROVED"}]')
         assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 1
         assert "schema.prisma: x LiteLLM_SpendLogs" in capsys.readouterr().out
-        reviews.write_text('[{"user": {"login": "kerry-berri"}, "state": "APPROVED"}]')
+        reviews.write_text('[{"user": {"login": "kerry-berri"}, "state": "APPROVED", "commit_id": "b"}]')
         assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 0
+
+    def test_an_approval_before_a_new_spend_log_line_was_pushed_fails(self, tmp_path, monkeypatch, capsys):
+        monkeypatch.setattr(guard, "added_diff", lambda base, head: _diff("schema.prisma", "  x LiteLLM_SpendLogs", 7))
+        monkeypatch.setattr(guard, "reviewed_hits", lambda base, commit: () if commit == "old" else None, raising=False)
+        reviews: Final = tmp_path / "r.json"
+        reviews.write_text('[{"user": {"login": "kerry-berri"}, "state": "APPROVED", "commit_id": "old"}]')
+        assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 1
+        assert "changed after @kerry-berri approved old" in capsys.readouterr().out
+        reviews.write_text('[{"user": {"login": "kerry-berri"}, "state": "APPROVED", "commit_id": "gone"}]')
+        assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 1