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

@@ -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
 

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
 

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

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 <ref>`` 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

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") == ()