From d65587593f04f6bb004f33e39c9f61d1fc542ed7 Mon Sep 17 00:00:00 2001 From: Kerry Lu Date: Tue, 27 Oct 2026 12:49:23 -0700 Subject: [PATCH] ci: spend-logs-guard requires kerry-berri approval on LiteLLM_SpendLogs schema and migration changes (#48518) * ci: spend-logs-guard requires kerry-berri approval on LiteLLM_SpendLogs schema and migration changes Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * ci(spend-logs-guard): flatten paginated reviews with jq, check out the base ref Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * 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> * Revert "ci(spend-logs-guard): read the diff from the pulls/files API instead of fetching PR commits" This reverts commit 36da4fe034426194431aa8af92f502b819dff981. --------- Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/spend-logs-guard.yml | 49 ++++++++++ ci_cd/spend_logs_guard.py | 119 +++++++++++++++++++++++++ tests/unit/test_spend_logs_guard.py | 79 ++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 .github/workflows/spend-logs-guard.yml create mode 100644 ci_cd/spend_logs_guard.py create mode 100644 tests/unit/test_spend_logs_guard.py diff --git a/.github/workflows/spend-logs-guard.yml b/.github/workflows/spend-logs-guard.yml new file mode 100644 index 00000000000..80a503679ec --- /dev/null +++ b/.github/workflows/spend-logs-guard.yml @@ -0,0 +1,49 @@ +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 + pull_request_target: + branches: + - main + pull_request_review: + types: + - submitted + - dismissed + +permissions: + contents: read + pull-requests: read + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + spend-logs-guard: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + 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 + 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}/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" diff --git a/ci_cd/spend_logs_guard.py b/ci_cd/spend_logs_guard.py new file mode 100644 index 00000000000..a04999c47c0 --- /dev/null +++ b/ci_cd/spend_logs_guard.py @@ -0,0 +1,119 @@ +"""Required check: a pull request that touches LiteLLM_SpendLogs in schema.prisma or a migration +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. +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from collections.abc import Sequence +from pathlib import Path +from typing import Final + +from pydantic import BaseModel, ConfigDict, TypeAdapter + +WATCHED: Final = ( + "schema.prisma", + "litellm/proxy/schema.prisma", + "litellm-proxy-extras/litellm_proxy_extras/schema.prisma", +) +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 spend_log_hits(diff: str) -> 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)) + + +def _watched_added_lines(sections: Sequence[str]) -> tuple[tuple[str, str], ...]: + pairs: Final = tuple((_path_of(section), section) for section in sections) + return tuple( + (path, line) for path, section in pairs if watched(path) for line in _added_lines(section) + ) # 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 approved_by(reviews: Sequence[Review], login: str) -> bool: + """True when `login`'s most recent review on the pull request is an approval.""" + states: Final = tuple(review.state for review in reviews if review.user.login == login) + 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 + reviews: Path + + +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("--reviews", required=True, help="JSON file with the pull request's reviews") + args: Final = Arguments.model_validate(vars(parser.parse_args(argv))) + hits: Final = spend_log_hits(added_diff(args.base, args.head)) + 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): + 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( + 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" + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/tests/unit/test_spend_logs_guard.py b/tests/unit/test_spend_logs_guard.py new file mode 100644 index 00000000000..9a033045dd0 --- /dev/null +++ b/tests/unit/test_spend_logs_guard.py @@ -0,0 +1,79 @@ +import importlib.util +import sys +from pathlib import Path +from typing import Final + +_SCRIPT: Final = Path(__file__).resolve().parents[2] / "ci_cd" / "spend_logs_guard.py" +_SPEC: Final = importlib.util.spec_from_file_location("spend_logs_guard", _SCRIPT) +assert _SPEC is not None and _SPEC.loader is not None +guard: Final = importlib.util.module_from_spec(_SPEC) +sys.modules["spend_logs_guard"] = guard +_SPEC.loader.exec_module(guard) + + +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" + + +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")', + ) + + 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");',) + + 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 + ) + assert guard.spend_log_hits(diff) == ("litellm/proxy/schema.prisma: a LiteLLM_SpendLogs",) + + +class TestApprovedBy: + def test_the_latest_review_of_the_approver_decides(self): + reviews: Final = tuple( + guard.Review(state=state, user=guard.Reviewer(login=login)) + for login, state in ( + ("kerry-berri", "CHANGES_REQUESTED"), + ("someone-else", "APPROVED"), + ("kerry-berri", "APPROVED"), + ) + ) + assert guard.approved_by(reviews, "kerry-berri") is True + assert guard.approved_by(reviews[::-1], "kerry-berri") is False + + def test_no_review_from_the_approver_is_not_an_approval(self): + other: Final = guard.Review(state="APPROVED", user=guard.Reviewer(login="someone-else")) + assert guard.approved_by((other,), "kerry-berri") is False + assert guard.approved_by((), "kerry-berri") 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)) + reviews: Final = tmp_path / "r.json" + reviews.write_text("[]") + assert guard.main(["--base", "a", "--head", "b", "--approver", "kerry-berri", "--reviews", str(reviews)]) == 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 + 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