From ef39e56b16c8cc2112ebde0ce019dcb472cfd0f1 Mon Sep 17 00:00:00 2001 From: nate-berri Date: Sat, 10 Oct 2026 18:56:45 +0000 Subject: [PATCH] fix(lint): read the base layer allowlist at the merge base make lint and the pre-commit hook pass the base branch tip, so an allowlist entry removed on main after this branch split off was reported as newly added. The checker now resolves the merge base itself with the same helper the other gates use --- scripts/check_layer_imports.py | 14 +++++++------- tests/unit/test_check_layer_imports.py | 23 ++++++++++++++++++++++- 2 files changed, 29 insertions(+), 8 deletions(-) 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