ci(spend-logs-guard): read the diff from the pulls/files API instead of fetching PR commits

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

.github/workflows/spend-logs-guard.yml

@@ -1,6 +1,6 @@
 name: Spend logs guard
 
-on: # zizmor: ignore[dangerous-triggers] runs the base branch's code only; the PR's diff is read as data and never executed
+on: # zizmor: ignore[dangerous-triggers] runs the base branch's code only; the PR's diff is read from the API as data and never executed
   pull_request_target:
     branches:
       - main
@@ -25,25 +25,17 @@ jobs:
         with:
           ref: ${{ github.event.pull_request.base.ref }}
           persist-credentials: false
-      - name: Fetch the pull request head, its merge base and its reviews
-        id: revisions
+      - name: Fetch the pull request's changed files and reviews
         env:
           GH_TOKEN: ${{ github.token }}
-          BASE_SHA: ${{ github.event.pull_request.base.sha }}
-          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
           PR_NUMBER: ${{ github.event.pull_request.number }}
         run: |
-          merge_base="$(gh api "repos/${GITHUB_REPOSITORY}/compare/${BASE_SHA}...${HEAD_SHA}" --jq '.merge_base_commit.sha')"
-          git fetch --no-tags --depth=1 origin "$merge_base" "$HEAD_SHA"
+          gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/files" --paginate --slurp | jq 'add // []' > "$RUNNER_TEMP/files.json"
           gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews" --paginate --slurp | jq 'add // []' > "$RUNNER_TEMP/reviews.json"
-          echo "merge_base=$merge_base" >> "$GITHUB_OUTPUT"
       - name: Set up uv
         uses: ./.github/actions/setup-uv-with-retries
         with:
           version: "0.10.9"
       - name: Run the guard
-        env:
-          MERGE_BASE: ${{ steps.revisions.outputs.merge_base }}
-          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
         run: |
-          uv run --frozen python ci_cd/spend_logs_guard.py --base "$MERGE_BASE" --head "$HEAD_SHA" --approver kerry-berri --reviews "$RUNNER_TEMP/reviews.json"
+          uv run --frozen python ci_cd/spend_logs_guard.py --files "$RUNNER_TEMP/files.json" --reviews "$RUNNER_TEMP/reviews.json" --approver kerry-berri

ci_cd/spend_logs_guard.py

@@ -2,16 +2,16 @@
 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.
+so a path rule would put every migration on the owner. This gate reads the pull request's diff from
+the GitHub API instead (`pulls/<n>/files`): 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 pull request's code is never fetched or run.
 """
 
 from __future__ import annotations
 
 import argparse
 import re
-import subprocess
 import sys
 from collections.abc import Sequence
 from pathlib import Path
@@ -26,6 +26,8 @@ WATCHED: Final = (
 )
 MIGRATIONS_PREFIX: Final = "litellm-proxy-extras/litellm_proxy_extras/migrations/"
 TABLE: Final = re.compile(r"(?<![A-Za-z0-9])LiteLLM_SpendLogs", re.IGNORECASE)
+# https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files (2026-10-26): at most 3000 files are listed
+FILES_API_CAP: Final = 3000
 
 
 class Reviewer(BaseModel):
@@ -39,34 +41,42 @@ class Review(BaseModel):
     user: Reviewer
 
 
+class ChangedFile(BaseModel):
+    """One entry of `GET /repos/{owner}/{repo}/pulls/{n}/files`. `patch` is omitted for binary or oversized diffs."""
+
+    model_config = ConfigDict(frozen=True, extra="ignore")
+    filename: str
+    changes: int
+    patch: str | None = None
+
+
 _REVIEWS: Final = TypeAdapter(list[Review])
+_FILES: Final = TypeAdapter(list[ChangedFile])
 
 
 def watched(path: str) -> bool:
     return path in WATCHED or (path.startswith(MIGRATIONS_PREFIX) and path.endswith("/migration.sql"))
 
 
-def _added_lines(section: str) -> tuple[str, ...]:
-    body: Final = section.split("\n@@", 1)
-    return tuple(line[1:] for line in body[1].splitlines() if line.startswith("+")) if len(body) == 2 else ()
+def _added_lines(patch: str) -> tuple[str, ...]:
+    return tuple(line[1:] for line in patch.splitlines() if line.startswith("+"))
 
 
-def spend_log_hits(diff: str) -> tuple[str, ...]:
+def spend_log_hits(files: Sequence[ChangedFile]) -> tuple[str, ...]:
     """`path: added line` for every added line in a watched file that names the table."""
-    sections: Final = re.split(r"^diff --git [^\n]*\n", diff, flags=re.MULTILINE)
-    return tuple(f"{path}: {line.strip()}" for path, line in _watched_added_lines(sections) if TABLE.search(line))
+    return tuple(f"{path}: {line.strip()}" for path, line in _watched_added_lines(files) if TABLE.search(line))
 
 
-def _watched_added_lines(sections: Sequence[str]) -> tuple[tuple[str, str], ...]:
-    pairs: Final = tuple((_path_of(section), section) for section in sections)
+def _watched_added_lines(files: Sequence[ChangedFile]) -> tuple[tuple[str, str], ...]:
     return tuple(
-        (path, line) for path, section in pairs if watched(path) for line in _added_lines(section)
+        (file.filename, line) for file in files if watched(file.filename) for line in _added_lines(file.patch or "")
     )  # comprehension-ok: path then its lines
 
 
-def _path_of(section: str) -> str:
-    match: Final = re.search(r"^\+\+\+ b/(.+)$", section, flags=re.MULTILINE)
-    return match.group(1) if match else ""
+def unreadable(files: Sequence[ChangedFile]) -> tuple[str, ...]:
+    """Watched files whose diff the API withheld, plus a marker when the file list itself was cut off."""
+    withheld: Final = tuple(f.filename for f in files if watched(f.filename) and f.changes and f.patch is None)
+    return withheld + (("(file list truncated by the API)",) if len(files) >= FILES_API_CAP else ())
 
 
 def approved_by(reviews: Sequence[Review], login: str) -> bool:
@@ -75,32 +85,29 @@ def approved_by(reviews: Sequence[Review], login: str) -> bool:
     return bool(states) and states[-1] == "APPROVED"
 
 
-def added_diff(base: str, head: str) -> str:
-    result: Final = subprocess.run(
-        ("git", "diff", "--unified=0", "--no-color", f"{base}..{head}", "--", *WATCHED, MIGRATIONS_PREFIX),
-        check=True,
-        capture_output=True,
-        text=True,
-    )
-    return result.stdout
-
-
 class Arguments(BaseModel):
     model_config = ConfigDict(frozen=True)
-    base: str
-    head: str
-    approver: str
+    files: Path
     reviews: Path
+    approver: str
 
 
 def main(argv: Sequence[str]) -> int:
     parser: Final = argparse.ArgumentParser(description=__doc__)
-    parser.add_argument("--base", required=True, help="merge base of the pull request")
-    parser.add_argument("--head", required=True, help="head sha of the pull request")
-    parser.add_argument("--approver", required=True, help="GitHub login whose approval unlocks the merge")
+    parser.add_argument("--files", required=True, help="JSON file with the pull request's `pulls/<n>/files` entries")
     parser.add_argument("--reviews", required=True, help="JSON file with the pull request's reviews")
+    parser.add_argument("--approver", required=True, help="GitHub login whose approval unlocks the merge")
     args: Final = Arguments.model_validate(vars(parser.parse_args(argv)))
-    hits: Final = spend_log_hits(added_diff(args.base, args.head))
+    files: Final = _FILES.validate_json(args.files.read_bytes())
+    hidden: Final = unreadable(files)
+    if hidden:
+        names: Final = "".join(f"  {name}\n" for name in hidden)
+        sys.stdout.write(
+            "The GitHub API did not return the full diff for a watched file, so LiteLLM_SpendLogs cannot be checked. "
+            f"Split the pull request:\n{names}"
+        )
+        return 1
+    hits: Final = spend_log_hits(files)
     if not hits:
         sys.stdout.write("No added line in schema.prisma or a migration names LiteLLM_SpendLogs; nothing to review.\n")
         return 0

tests/unit/test_spend_logs_guard.py

@@ -1,4 +1,5 @@
 import importlib.util
+import json
 import sys
 from pathlib import Path
 from typing import Final
@@ -10,36 +11,62 @@ guard: Final = importlib.util.module_from_spec(_SPEC)
 sys.modules["spend_logs_guard"] = guard
 _SPEC.loader.exec_module(guard)
 
+MIGRATION: Final = "litellm-proxy-extras/litellm_proxy_extras/migrations/20260101000000_x/migration.sql"
 
-def _diff(path: str, added: str, start: int = 10) -> str:
-    return f"diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -{start},0 +{start},1 @@\n+{added}\n"
+
+def _file(path: str, *lines: str, start: int = 10) -> dict[str, object]:
+    """An entry of `pulls/<n>/files`, with the lines as a hunk: a leading `-` removes, anything else adds."""
+    hunk: Final = "\n".join(line if line.startswith("-") else f"+{line}" for line in lines)
+    return {"filename": path, "changes": len(lines), "patch": f"@@ -{start},0 +{start},{len(lines)} @@\n{hunk}"}
+
+
+def _files(*entries: dict[str, object]) -> list[guard.ChangedFile]:
+    return guard._FILES.validate_json(json.dumps(entries))
 
 
 class TestSpendLogHits:
     def test_an_added_schema_line_naming_the_table_is_a_hit(self):
-        diff: Final = _diff(
-            "schema.prisma", '  @@index([api_key, startTime], map: "LiteLLM_SpendLogs_api_key_startTime_idx")', 740
-        )
-        assert guard.spend_log_hits(diff) == (
-            'schema.prisma: @@index([api_key, startTime], map: "LiteLLM_SpendLogs_api_key_startTime_idx")',
+        line: Final = '  @@index([api_key, startTime], map: "LiteLLM_SpendLogs_api_key_startTime_idx")'
+        assert guard.spend_log_hits(_files(_file("schema.prisma", line, start=740))) == (
+            f"schema.prisma: {line.strip()}",
         )
 
     def test_an_added_migration_line_is_a_hit_in_any_case(self):
-        path: Final = "litellm-proxy-extras/litellm_proxy_extras/migrations/20260101000000_x/migration.sql"
-        hits: Final = guard.spend_log_hits(_diff(path, 'CREATE INDEX "i" ON "litellm_spendlogs"("api_key");', 3))
-        assert hits == (f'{path}: CREATE INDEX "i" ON "litellm_spendlogs"("api_key");',)
+        files: Final = _files(_file(MIGRATION, 'CREATE INDEX "i" ON "litellm_spendlogs"("api_key");', start=3))
+        assert guard.spend_log_hits(files) == (f'{MIGRATION}: CREATE INDEX "i" ON "litellm_spendlogs"("api_key");',)
 
     def test_a_removed_line_or_another_table_or_an_unwatched_file_is_not_a_hit(self):
-        removed: Final = "--- a/schema.prisma\n+++ b/schema.prisma\n@@ -1,1 +1,0 @@\n-model LiteLLM_SpendLogs {\n"
-        other: Final = _diff("schema.prisma", "  team_id String  // LiteLLM_TeamTable, not MyLiteLLM_SpendLogs", 5)
-        elsewhere: Final = _diff("litellm/proxy/utils.py", 'table = "LiteLLM_SpendLogs"', 5)
-        assert guard.spend_log_hits(removed + other + elsewhere) == ()
-
-    def test_two_files_in_one_diff_are_attributed_to_their_own_path(self):
-        diff: Final = _diff("litellm/proxy/schema.prisma", "  a LiteLLM_SpendLogs", 1) + _diff(
-            "schema.prisma", "  b String", 1
+        files: Final = _files(
+            _file(
+                "schema.prisma",
+                "-model LiteLLM_SpendLogs {",
+                "  team_id String  // LiteLLM_TeamTable, MyLiteLLM_SpendLogs",
+            ),
+            _file("litellm/proxy/utils.py", 'table = "LiteLLM_SpendLogs"'),
+            {"filename": "docs/spend_logs.png", "changes": 0},
         )
-        assert guard.spend_log_hits(diff) == ("litellm/proxy/schema.prisma: a LiteLLM_SpendLogs",)
+        assert guard.spend_log_hits(files) == ()
+
+    def test_two_files_are_attributed_to_their_own_path(self):
+        files: Final = _files(
+            _file("litellm/proxy/schema.prisma", "  a LiteLLM_SpendLogs"), _file("schema.prisma", "  b")
+        )
+        assert guard.spend_log_hits(files) == ("litellm/proxy/schema.prisma: a LiteLLM_SpendLogs",)
+
+
+class TestUnreadable:
+    def test_a_watched_file_without_a_patch_is_unreadable_but_an_unwatched_or_unchanged_one_is_not(self):
+        files: Final = _files(
+            {"filename": MIGRATION, "changes": 900},
+            {"filename": "schema.prisma", "changes": 0},
+            {"filename": "ui/logo.png", "changes": 12},
+        )
+        assert guard.unreadable(files) == (MIGRATION,)
+
+    def test_a_file_list_at_the_api_cap_is_unreadable(self):
+        files: Final = _files(*(_file(f"litellm/f{i}.py", "x") for i in range(guard.FILES_API_CAP)))
+        assert guard.unreadable(files) == ("(file list truncated by the API)",)
+        assert guard.unreadable(files[:-1]) == ()
 
 
 class TestApprovedBy:
@@ -61,19 +88,26 @@ class TestApprovedBy:
         assert guard.approved_by((), "kerry-berri") is False
 
 
+def _run(tmp_path: Path, files: list[dict[str, object]], reviews: list[dict[str, object]]) -> int:
+    files_path: Final = tmp_path / "files.json"
+    reviews_path: Final = tmp_path / "reviews.json"
+    files_path.write_text(json.dumps(files))
+    reviews_path.write_text(json.dumps(reviews))
+    return guard.main(["--files", str(files_path), "--reviews", str(reviews_path), "--approver", "kerry-berri"])
+
+
 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))
-        reviews: Final = tmp_path / "r.json"
-        reviews.write_text("[]")
-        assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 0
+    def test_a_pull_request_without_spend_log_lines_passes_without_reviews(self, tmp_path, capsys):
+        assert _run(tmp_path, [_file("schema.prisma", "  foo String")], []) == 0
         assert "nothing to review" in capsys.readouterr().out
 
-    def test_a_spend_log_change_fails_until_the_approver_approves(self, tmp_path, monkeypatch, capsys):
-        monkeypatch.setattr(guard, "added_diff", lambda base, head: _diff("schema.prisma", "  x LiteLLM_SpendLogs", 7))
-        reviews: Final = tmp_path / "r.json"
-        reviews.write_text('[{"user": {"login": "someone-else"}, "state": "APPROVED"}]')
-        assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 1
+    def test_a_spend_log_change_fails_until_the_approver_approves(self, tmp_path, capsys):
+        files: Final = [_file("schema.prisma", "  x LiteLLM_SpendLogs", start=7)]
+        assert _run(tmp_path, files, [{"user": {"login": "someone-else"}, "state": "APPROVED"}]) == 1
         assert "schema.prisma: x LiteLLM_SpendLogs" in capsys.readouterr().out
-        reviews.write_text('[{"user": {"login": "kerry-berri"}, "state": "APPROVED"}]')
-        assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 0
+        assert _run(tmp_path, files, [{"user": {"login": "kerry-berri"}, "state": "APPROVED"}]) == 0
+
+    def test_a_withheld_patch_on_a_watched_file_fails_even_with_approval(self, tmp_path, capsys):
+        files: Final = [{"filename": MIGRATION, "changes": 2500}]
+        assert _run(tmp_path, files, [{"user": {"login": "kerry-berri"}, "state": "APPROVED"}]) == 1
+        assert MIGRATION in capsys.readouterr().out