From cde764a133c75c1d7c965d4c33c7fc1b2b52d97c Mon Sep 17 00:00:00 2001 From: mateo Date: Sat, 8 Aug 2026 05:53:55 +0000 Subject: [PATCH] ci: fail when the grandfathered extra="allow" list grows, run the ban in pre-commit Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/test-code-quality.yml | 8 +++ CLAUDE.md | 2 +- scripts/pre_commit_lint.sh | 16 ++++++ .../ban_pydantic_extra_allow.py | 57 ++++++++++++++++++- .../test_ban_pydantic_extra_allow.py | 57 +++++++++++++++++++ 5 files changed, 138 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test-code-quality.yml b/.github/workflows/test-code-quality.yml index 3f488acc6b6..a02962ec568 100644 --- a/.github/workflows/test-code-quality.yml +++ b/.github/workflows/test-code-quality.yml @@ -119,6 +119,14 @@ jobs: - name: ban_pydantic_extra_allow run: uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py + - name: ban_pydantic_extra_allow (grandfathered list only shrinks) + if: github.event_name == 'pull_request' + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + git fetch --no-tags --depth=1 origin "$BASE_SHA" + uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py --base "$BASE_SHA" + - name: check_fastuuid_usage run: uv run --no-sync python ./tests/code_coverage_tests/check_fastuuid_usage.py diff --git a/CLAUDE.md b/CLAUDE.md index 44fc09f0b5d..54b4d5ba392 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,7 +43,7 @@ When you fix violations gated by `ruff-strict-budget.json`, `type-discipline-bud `make pre-commit` saves its complete output to a log file in .git (overwriting previous pre-commit logs) and prints that path as its first and last output lines. To inspect a run, read or grep that log instead of re-running the multi-minute checks just to see a different slice -New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models that already had it, so don't add it to a new model and don't grow the grandfathered list without a real reason +New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models that already had it, so don't add it to a new model. That list only shrinks: the check fails when your branch adds an entry to it, so removing a model's `extra="allow"` is the only edit it accepts. `make pre-commit` runs it on any commit that touches `litellm/` Python If you're trying to create a new function that relies on untyped stuff, instead of adding more Any's and pushing `reportAny` / `reportExplicitAny` closer to their basedpyright ceilings, just validate it in the caller with Pydantic (a model or `TypeAdapter` that returns the typed thing or raises will do) and then pass the now typed variable in diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index af8335e0e84..0267f1a5ed6 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -6,6 +6,8 @@ # - litellm/ Python staged -> `make lint` (test-linting.yml's lint job) # - tests/e2e Python staged -> `make lint-e2e-basedpyright` (test-linting.yml's e2e type-check step) # + raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests) +# - litellm/ Python or this +# check's own list staged -> extra="allow" ban (test-code-quality.yml's ban_pydantic_extra_allow) # - dashboard staged -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) # - proxy/types staged -> regenerate dashboard API types and fail on drift (check-ui-api-types.yml) # @@ -47,6 +49,9 @@ litellm_py_files=$(staged_match '^litellm/.*\.py$') e2e_py_files=$(staged_match '^tests/e2e/.*\.py$') # ruff format (and CI's format step) skip enterprise; the rest of make lint covers it. fmt_files=$(printf '%s\n' "$litellm_py_files" | grep -v '^litellm/enterprise/' || true) +# The extra="allow" ban reads all of litellm/, and its grandfathered list is data the +# check itself gates, so editing either can turn ban_pydantic_extra_allow red. +extra_allow_files=$(staged_match '^(litellm/.*\.py|tests/code_coverage_tests/ban_pydantic_extra_allow\.py)$') # check-ui-api-types.yml triggers on any file under litellm/proxy or litellm/types # (Prisma schema and configs included, not just Python) plus the generator and its # lockfiles, so match that whole trigger set rather than a Python subset. @@ -156,6 +161,17 @@ if [ -n "$e2e_py_files" ]; then || { echo "✗ Raw HTTP client import in tests/e2e. Route the call through tests/e2e/e2e_http.py, then re-run make pre-commit." >&2; status=1; } fi +# Around two seconds over all of litellm/, so it runs inline rather than behind a job. +# CI ratchets the grandfathered list against the base branch tip; locally the closest +# equivalent is the merge-base, and the check reports the ref it couldn't read. +if [ -n "$extra_allow_files" ]; then + echo "pre-commit: checking new pydantic models don't set extra=\"allow\" (ban_pydantic_extra_allow)" + extra_allow_base=$(git merge-base origin/litellm_internal_staging HEAD 2>/dev/null || true) + uv run --no-sync python tests/code_coverage_tests/ban_pydantic_extra_allow.py \ + ${extra_allow_base:+--base "$extra_allow_base"} \ + || { echo "✗ New extra=\"allow\" model or a grown grandfathered list. Declare the fields the model accepts, then re-run make pre-commit." >&2; status=1; } +fi + dashboard_checks() { echo "pre-commit: linting dashboard (prettier + eslint + lint budgets)" if [ ! -d ui/litellm-dashboard/node_modules ]; then diff --git a/tests/code_coverage_tests/ban_pydantic_extra_allow.py b/tests/code_coverage_tests/ban_pydantic_extra_allow.py index 50f7c5364ad..02fab32f51f 100644 --- a/tests/code_coverage_tests/ban_pydantic_extra_allow.py +++ b/tests/code_coverage_tests/ban_pydantic_extra_allow.py @@ -4,14 +4,20 @@ real fields (pricing on ``ModelInfo``, for example) never get declared anywhere. The models listed in ``GRANDFATHERED`` predate this check and stay allowed; anything new must declare its fields. + +The list is one way. With ``--base `` the check also fails when it gained an entry +relative to that ref, so the only edits to it that pass are removals. """ +import argparse import ast import os +import subprocess import sys from typing import Final, Iterator, NamedTuple, Sequence SCAN_ROOT: Final = "litellm" +SELF_PATH: Final = "tests/code_coverage_tests/ban_pydantic_extra_allow.py" GRANDFATHERED: Final = frozenset( { @@ -193,6 +199,11 @@ def _iter_classes(body: Sequence[ast.stmt], prefix: str = "") -> Iterator[tuple[ def find_violations_in_source(source: str, relative_path: str) -> tuple[Violation, ...]: + """Every opt-in this check understands spells ``allow`` in the module that declares the + model, whether as a literal, an ``Extra.allow`` attribute, or a constant it resolves, so a + module without that substring cannot hold one and is not worth parsing.""" + if "allow" not in source: + return () tree: Final = ast.parse(source, filename=relative_path) bindings: Final = _module_bindings(tree.body) return tuple( @@ -218,11 +229,48 @@ def find_extra_allow_models(base_dir: str) -> tuple[Violation, ...]: ) +def parse_grandfathered(source: str) -> frozenset[str]: + """The entries ``GRANDFATHERED`` lists in ``source``, so another revision of this file + can be read without importing it. The entries are string literals, so they are collected + from the assignment's subtree rather than evaluating the ``frozenset`` call around them.""" + return frozenset( + node.value + for statement in ast.parse(source).body + for name, value in _assigned_names(statement) + if name == "GRANDFATHERED" + for node in ast.walk(value) + if isinstance(node, ast.Constant) and isinstance(node.value, str) + ) + + +def _source_at(ref: str) -> str | None: + completed: Final = subprocess.run( + ["git", "show", f"{ref}:{SELF_PATH}"], capture_output=True, text=True, check=False + ) + return completed.stdout if completed.returncode == 0 else None + + +def added_grandfathered(base_ref: str) -> tuple[str, ...]: + base_source: Final = _source_at(base_ref) + if base_source is None: + print(f"{SELF_PATH} does not exist at {base_ref}, so there is no list to ratchet against yet.") + return () + return tuple(sorted(GRANDFATHERED - parse_grandfathered(base_source))) + + def main() -> int: + parser: Final = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base", + help="git ref to ratchet GRANDFATHERED against, failing when this branch added an entry", + ) + args: Final = parser.parse_args() + base_dir = os.getcwd() found = find_extra_allow_models(base_dir) violations = tuple(violation for violation in found if violation.identifier() not in GRANDFATHERED) stale = tuple(sorted(GRANDFATHERED - {violation.identifier() for violation in found})) + added = added_grandfathered(args.base) if args.base else () for violation in violations: print(f'{violation.file}:{violation.line}: {violation.model} sets extra="allow"') @@ -241,8 +289,15 @@ def main() -> int: ) for entry in stale: print(f" {entry}") + if added: + print( + f"\nThis branch added {len(added)} entry/entries to GRANDFATHERED. The list only\n" + "shrinks, so declare the fields these models accept instead of grandfathering them:" + ) + for entry in added: + print(f" {entry}") - if violations or stale: + if violations or stale or added: return 1 print('No new extra="allow" Pydantic models found.') return 0 diff --git a/tests/test_litellm/test_ban_pydantic_extra_allow.py b/tests/test_litellm/test_ban_pydantic_extra_allow.py index 3e11501bbca..ae7e470bd46 100644 --- a/tests/test_litellm/test_ban_pydantic_extra_allow.py +++ b/tests/test_litellm/test_ban_pydantic_extra_allow.py @@ -1,6 +1,7 @@ """Tests for the extra="allow" ban at tests/code_coverage_tests/ban_pydantic_extra_allow.py.""" import os +import subprocess import sys import pytest @@ -153,3 +154,59 @@ def test_grandfathered_list_matches_repo(): found = frozenset(violation.identifier() for violation in checker.find_extra_allow_models(_REPO_ROOT)) assert sorted(found - checker.GRANDFATHERED) == [] assert sorted(checker.GRANDFATHERED - found) == [] + + +def test_parse_grandfathered_reads_the_list_of_another_revision(): + source = 'OTHER = frozenset({"nope"})\nGRANDFATHERED = frozenset(\n {\n "a.py::A",\n "b.py::B",\n }\n)\n' + assert checker.parse_grandfathered(source) == frozenset({"a.py::A", "b.py::B"}) + + +def test_parse_grandfathered_reads_this_files_own_list(): + with open(os.path.join(_REPO_ROOT, checker.SELF_PATH), encoding="utf-8") as handle: + assert checker.parse_grandfathered(handle.read()) == checker.GRANDFATHERED + + +def _baseline_commit(repo, entries): + """A one-commit repo whose copy of the checker grandfathers exactly ``entries``.""" + target = os.path.join(repo, checker.SELF_PATH) + os.makedirs(os.path.dirname(target), exist_ok=True) + with open(target, "w", encoding="utf-8") as handle: + handle.write("GRANDFATHERED = frozenset(\n {\n") + handle.writelines(f' "{entry}",\n' for entry in entries) + handle.write(" }\n)\n") + for command in ( + ["git", "init", "-q", "."], + ["git", "add", checker.SELF_PATH], + ["git", "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-qm", "baseline"], + ): + subprocess.run(command, cwd=repo, check=True) + return subprocess.run( + ["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + +@pytest.mark.parametrize( + "baseline, expected", + [ + pytest.param(checker.GRANDFATHERED, [], id="unchanged_list_passes"), + pytest.param( + checker.GRANDFATHERED | {"litellm/types/gone.py::Gone"}, + [], + id="removing_an_entry_passes", + ), + pytest.param( + checker.GRANDFATHERED - {"litellm/types/utils.py::ImageResponse"}, + ["litellm/types/utils.py::ImageResponse"], + id="adding_an_entry_is_reported", + ), + ], +) +def test_added_grandfathered_only_reports_growth(tmp_path, monkeypatch, baseline, expected): + base_sha = _baseline_commit(str(tmp_path), sorted(baseline)) + monkeypatch.chdir(tmp_path) + assert list(checker.added_grandfathered(base_sha)) == expected + + +def test_added_grandfathered_skips_a_base_without_the_checker(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + assert checker.added_grandfathered("HEAD") == ()