diff --git a/scripts/check_layer_imports.py b/scripts/check_layer_imports.py index 59991561ffd..34a259d7256 100644 --- a/scripts/check_layer_imports.py +++ b/scripts/check_layer_imports.py @@ -1,6 +1,5 @@ import argparse import ast -import subprocess import sys from collections.abc import Callable, Iterable, Iterator, Sequence from dataclasses import dataclass @@ -9,6 +8,8 @@ from pathlib import Path from types import MappingProxyType from typing import Final, Literal +from lint_base_counts import REPO_GIT, Git, resolve_base_point + PACKAGE_ROOT: Final = Path("litellm") ALLOWLIST: Final = Path("scripts/layer_imports_allowlist.txt") LEAF_MODULES: Final = frozenset( @@ -172,11 +173,10 @@ def report( return new + stale + added -def git_base_allowlist(ref: str) -> frozenset[str] | None: - shown: Final = subprocess.run( - ["git", "show", f"{ref}:{ALLOWLIST.as_posix()}"], capture_output=True, text=True, check=False - ) - return parse_allowlist(shown.stdout) if shown.returncode == 0 else None +def git_base_allowlist(ref: str, git: Git = REPO_GIT) -> frozenset[str] | None: + base_point: Final = resolve_base_point(ref, git) + tracked: Final = git(["ls-tree", "--name-only", base_point, "--", ALLOWLIST.as_posix()]).strip() + return parse_allowlist(git(["show", f"{base_point}:{ALLOWLIST.as_posix()}"])) if tracked else None class Arguments(argparse.Namespace): @@ -189,7 +189,7 @@ def main( ) -> int: parser: Final = argparse.ArgumentParser(description="Check SDK/proxy layer import rules") parser.add_argument( - "--base", help="Also reject allowlist entries missing from the allowlist at this ref (CI passes the merge base)" + "--base", help="Also reject allowlist entries missing from the allowlist at the merge base with this ref" ) args: Final = parser.parse_args(argv, namespace=Arguments()) paths: Final = tuple(sorted(PACKAGE_ROOT.rglob("*.py"))) diff --git a/tests/unit/test_check_layer_imports.py b/tests/unit/test_check_layer_imports.py index 7d84984c7c6..505ee53ced2 100644 --- a/tests/unit/test_check_layer_imports.py +++ b/tests/unit/test_check_layer_imports.py @@ -2,7 +2,7 @@ from pathlib import Path import pytest -from scripts.check_layer_imports import main +from scripts.check_layer_imports import git_base_allowlist, main def build_tree(root: Path, files: dict[str, str], allowlist: str = "") -> None: @@ -194,3 +194,24 @@ def test_missing_base_allowlist_skips_growth_check( ) assert run_check(tmp_path, monkeypatch, capsys, None) == (0, "") + + +def test_base_allowlist_is_read_at_the_merge_base_not_the_branch_tip() -> None: + allowlist = "scripts/layer_imports_allowlist.txt" + outputs = { + ("merge-base", "origin/main", "HEAD"): "forkpoint\n", + ("ls-tree", "--name-only", "forkpoint", "--", allowlist): f"{allowlist}\n", + ("show", f"forkpoint:{allowlist}"): "L3 module litellm/types/foo.py litellm.router\n", + ("ls-tree", "--name-only", "origin/main", "--", allowlist): f"{allowlist}\n", + ("show", f"origin/main:{allowlist}"): "", + } + + assert git_base_allowlist("origin/main", lambda args: outputs.get(tuple(args), "")) == frozenset( + {"L3 module litellm/types/foo.py litellm.router"} + ) + + +def test_base_without_an_allowlist_skips_the_growth_check() -> None: + outputs = {("merge-base", "origin/main", "HEAD"): "forkpoint\n"} + + assert git_base_allowlist("origin/main", lambda args: outputs.get(tuple(args), "")) is None