diff --git a/.github/workflows/spend-logs-guard.yml b/.github/workflows/spend-logs-guard.yml index 80a503679ec..e5bfeedaaa0 100644 --- a/.github/workflows/spend-logs-guard.yml +++ b/.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 diff --git a/ci_cd/spend_logs_guard.py b/ci_cd/spend_logs_guard.py index a04999c47c0..8f05c7aeb71 100644 --- a/ci_cd/spend_logs_guard.py +++ b/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//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"(? 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//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 diff --git a/tests/unit/test_spend_logs_guard.py b/tests/unit/test_spend_logs_guard.py index 9a033045dd0..9a5bcacbe91 100644 --- a/tests/unit/test_spend_logs_guard.py +++ b/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//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