From 84e12a441113fd0ef8f4dfb1697c0148257ede69 Mon Sep 17 00:00:00 2001 From: nate-berri Date: Sat, 10 Oct 2026 15:03:39 +0000 Subject: [PATCH] ci(lint): reject layer allowlist entries added since the merge base Part of LIT-9423 --- .github/workflows/test-linting.yml | 2 +- Makefile | 2 +- scripts/check_layer_imports.py | 47 +++++++++++++++++++---- scripts/pre_commit_lint.sh | 2 +- tests/unit/test_check_layer_imports.py | 52 +++++++++++++++++--------- 5 files changed, 78 insertions(+), 27 deletions(-) diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index 6cb01775e80..da85d134294 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -193,7 +193,7 @@ jobs: - name: Check layer imports if: steps.changes.outputs.decision != 'skip' - run: uv run --no-sync python scripts/check_layer_imports.py + run: uv run --no-sync python scripts/check_layer_imports.py --base "$GATE_BASE_SHA" - name: Check import safety if: steps.changes.outputs.decision != 'skip' diff --git a/Makefile b/Makefile index f80009df6f8..ee3c336bae7 100644 --- a/Makefile +++ b/Makefile @@ -230,7 +230,7 @@ lint-gate: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/ruff_strict_gate.py --base "$(BASE_REF)" check-layer-imports: $(LINT_DEP_INSTALL) - $(UV_RUN) python scripts/check_layer_imports.py + $(UV_RUN) python scripts/check_layer_imports.py --base "$$($(RESOLVE_BASE))" check-import-safety: $(LINT_DEP_INSTALL) @$(UV_RUN) python -c "from litellm import *; print('[from litellm import *] OK! no issues!');" || (echo '🚨 import failed, this means you introduced unprotected imports! 🚨'; exit 1) diff --git a/scripts/check_layer_imports.py b/scripts/check_layer_imports.py index ef2a0b1b65d..623abf7d0ce 100644 --- a/scripts/check_layer_imports.py +++ b/scripts/check_layer_imports.py @@ -1,6 +1,8 @@ +import argparse import ast +import subprocess import sys -from collections.abc import Callable, Iterable, Iterator +from collections.abc import Callable, Iterable, Iterator, Sequence from dataclasses import dataclass from itertools import chain, product from pathlib import Path @@ -140,12 +142,14 @@ def violations(paths: tuple[Path, ...]) -> tuple[Violation, ...]: ) -def read_allowlist(path: Path) -> frozenset[str]: - lines: Final = (" ".join(line.split()) for line in path.read_text(encoding="utf-8").splitlines()) +def parse_allowlist(text: str) -> frozenset[str]: + lines: Final = (" ".join(line.split()) for line in text.splitlines()) return frozenset(line for line in lines if line and not line.startswith("#")) -def report(found: tuple[Violation, ...], allowed: frozenset[str]) -> tuple[str, ...]: +def report( + found: tuple[Violation, ...], allowed: frozenset[str], base_allowed: frozenset[str] | None +) -> tuple[str, ...]: first_seen: Final = MappingProxyType({violation.key: violation for violation in reversed(found)}) new: Final = tuple( f"{violation.edge.path}:{violation.edge.line}: {violation.rule} forbids " @@ -157,12 +161,41 @@ def report(found: tuple[Violation, ...], allowed: frozenset[str]) -> tuple[str, f"{ALLOWLIST}: stale entry '{key}' no longer matches an import, delete it" for key in sorted(allowed - first_seen.keys()) ) - return new + stale + added: Final = ( + tuple( + f"{ALLOWLIST}: entry '{key}' is not in the base allowlist, fix the import instead of allowlisting it" + for key in sorted(allowed - base_allowed) + ) + if base_allowed is not None + else () + ) + return new + stale + added -def main() -> int: +def git_base_allowlist(ref: str) -> frozenset[str] | None: + merge_base: Final = subprocess.run( + ["git", "merge-base", ref, "HEAD"], capture_output=True, text=True, check=True + ).stdout.strip() + shown: Final = subprocess.run( + ["git", "show", f"{merge_base}:{ALLOWLIST.as_posix()}"], capture_output=True, text=True, check=False + ) + return parse_allowlist(shown.stdout) if shown.returncode == 0 else None + + +class Arguments(argparse.Namespace): + base: str | None = None + + +def main( + argv: Sequence[str] | None = None, + base_allowlist: Callable[[str], frozenset[str] | None] = git_base_allowlist, +) -> int: + parser: Final = argparse.ArgumentParser(description="Check SDK/proxy layer import rules") + parser.add_argument("--base", help="Also reject allowlist entries missing at the merge base with this ref") + args: Final = parser.parse_args(argv, namespace=Arguments()) paths: Final = tuple(sorted(PACKAGE_ROOT.rglob("*.py"))) - problems: Final = report(violations(paths), read_allowlist(ALLOWLIST)) + base_allowed: Final = base_allowlist(args.base) if args.base else None + problems: Final = report(violations(paths), parse_allowlist(ALLOWLIST.read_text(encoding="utf-8")), base_allowed) if problems: print("\n".join(problems), file=sys.stderr) return 1 diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index 0098e909af0..e3eaecca4c4 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -107,7 +107,7 @@ if [ -n "$(scope_match '^(litellm/proxy/_experimental/mcp_server/|scripts/check_ uv run --no-sync python scripts/check_mcp_operation_boundary.py || exit 1 fi if [ -n "$(scope_match '^(litellm/.*\.py|scripts/check_layer_imports\.py|scripts/layer_imports_allowlist\.txt)$')" ]; then - uv run --no-sync python scripts/check_layer_imports.py || exit 1 + uv run --no-sync python scripts/check_layer_imports.py ${base_ref:+--base "$base_ref"} || exit 1 fi e2e_py_files=$(scope_match "$e2e_py_pattern") test_tree_files=$(scope_match "$test_tree_pattern") diff --git a/tests/unit/test_check_layer_imports.py b/tests/unit/test_check_layer_imports.py index 2d3db0c93a7..7d84984c7c6 100644 --- a/tests/unit/test_check_layer_imports.py +++ b/tests/unit/test_check_layer_imports.py @@ -1,13 +1,9 @@ -import subprocess -import sys from pathlib import Path import pytest from scripts.check_layer_imports import main -SCRIPT = Path(__file__).resolve().parents[2] / "scripts/check_layer_imports.py" - def build_tree(root: Path, files: dict[str, str], allowlist: str = "") -> None: for relative, source in { @@ -23,9 +19,14 @@ def build_tree(root: Path, files: dict[str, str], allowlist: str = "") -> None: (root / "scripts/layer_imports_allowlist.txt").write_text(allowlist) -def run_check(root: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]) -> tuple[int, str]: +def run_check( + root: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + base_allowed: frozenset[str] | None = None, +) -> tuple[int, str]: monkeypatch.chdir(root) - code = main() + code = main(["--base", "origin/main"], lambda _ref: base_allowed) return code, capsys.readouterr().err @@ -162,17 +163,34 @@ def test_allowlisted_violation_passes_and_stale_entry_fails( ] -def test_command_exit_code(tmp_path: Path) -> None: - build_tree(tmp_path, {"litellm/types/foo.py": "from litellm.router import Router\n"}) - rejected = subprocess.run( - [sys.executable, "-I", str(SCRIPT)], cwd=tmp_path, capture_output=True, text=True, check=False +def test_allowlist_entry_added_since_base_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + build_tree( + tmp_path, + { + "litellm/types/foo.py": "from litellm.router import Router\n", + "litellm/types/bar.py": "import litellm.main\n", + }, + "L3 module litellm/types/foo.py litellm.router\nL3 module litellm/types/bar.py litellm.main\n", ) - assert rejected.returncode == 1 - assert "litellm/types/foo.py:1: L3 forbids" in rejected.stderr + base = frozenset({"L3 module litellm/types/foo.py litellm.router"}) - (tmp_path / "scripts/layer_imports_allowlist.txt").write_text("L3 module litellm/types/foo.py litellm.router\n") - accepted = subprocess.run( - [sys.executable, "-I", str(SCRIPT)], cwd=tmp_path, capture_output=True, text=True, check=False + assert run_check(tmp_path, monkeypatch, capsys, base) == ( + 1, + "scripts/layer_imports_allowlist.txt: entry 'L3 module litellm/types/bar.py litellm.main' is not in the " + "base allowlist, fix the import instead of allowlisting it\n", ) - assert accepted.returncode == 0 - assert accepted.stdout == "Layer imports: passed\n" + assert run_check(tmp_path, monkeypatch, capsys, base | {"L3 module litellm/types/bar.py litellm.main"}) == (0, "") + + +def test_missing_base_allowlist_skips_growth_check( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + build_tree( + tmp_path, + {"litellm/types/foo.py": "from litellm.router import Router\n"}, + "L3 module litellm/types/foo.py litellm.router\n", + ) + + assert run_check(tmp_path, monkeypatch, capsys, None) == (0, "")