From f9df01b675f59419bfd4192181d8676f069868d5 Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Fri, 7 Aug 2026 13:46:18 -0700 Subject: [PATCH] fix: apply canonical-ownership scoping to added public names too The surface-widening layer compared raw top-level member names, so adding `from typing import Final` to litellm/__init__.py under a chore/refactor title would fail the gate even though the breaking layer explicitly excludes external re-exports. Scoping now follows the alias chain: litellm.Final resolves through litellm.scheduler.Final to typing.Final and is ignored, while litellm.BedrockLLM resolves inside the package and is still caught. --- .github/scripts/check_api_breaking_changes.py | 48 +++++++++++++---- .../test_github_api_breaking_changes.py | 54 +++++++++++++++++-- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/.github/scripts/check_api_breaking_changes.py b/.github/scripts/check_api_breaking_changes.py index 76742bd99c1..8d4e486a3d5 100644 --- a/.github/scripts/check_api_breaking_changes.py +++ b/.github/scripts/check_api_breaking_changes.py @@ -12,9 +12,9 @@ Two layers, both computed with griffe against the PR's base ref: 1. Breaking changes (removed objects, changed signatures, changed defaults). Allowed only when the PR declares a breaking change the Conventional Commits way: a `!` in the title type, or a `BREAKING CHANGE:` footer. - 2. Newly exported top-level names (`litellm.`). Allowed only under a - `feat` or `fix` title, so a `chore`/`refactor` PR cannot quietly widen - the public surface. + 2. Newly exported top-level names (`litellm.`) that the package owns. + Allowed only under a `feat` or `fix` title, so a `chore`/`refactor` PR + cannot quietly widen the public surface. Attribute *value* changes are advisory: `Union[A, B] -> A | B` rewrites and f-string conversions trip that check constantly without breaking anyone. @@ -118,17 +118,36 @@ def source_location(obj: Object | Alias) -> tuple[str | None, int | None]: return None, None -def home_path(obj: Object | Alias) -> str: +def lookup(roots: tuple[Module, ...], path: str) -> Object | Alias | None: + parts: Final = path.split(".") + for root in roots: + if parts[0] != root.name: + continue + try: + return root.get_member(parts[1:]) + except Exception: + continue + return None + + +def home_path(obj: Object | Alias, roots: tuple[Module, ...] = (), seen: frozenset[str] = frozenset()) -> str | None: try: return obj.canonical_path except Exception: - return str(getattr(obj, "target_path", obj.path)) + target: Final = getattr(obj, "target_path", None) + if target is None or target in seen: + return None + next_hop: Final = lookup(roots, target) + if next_hop is None: + return target + return home_path(next_hop, roots, seen | {target}) or target -def is_in_scope(obj: Object | Alias, package: str) -> bool: +def is_in_scope(obj: Object | Alias, package: str, roots: tuple[Module, ...] = ()) -> bool: if obj.path.startswith(OUT_OF_SCOPE_PREFIXES): return False - return home_path(obj).startswith(f"{package}.") + home: Final = home_path(obj, roots) + return home is not None and home.startswith(f"{package}.") def to_finding(breakage: Breakage, style: ExplanationStyle) -> ApiFinding: @@ -142,8 +161,12 @@ def to_finding(breakage: Breakage, style: ExplanationStyle) -> ApiFinding: ) -def top_level_names(module: Module) -> frozenset[str]: - return frozenset(name for name in module.members if not name.startswith("_")) +def top_level_names(module: Module, package: str, roots: tuple[Module, ...]) -> frozenset[str]: + return frozenset( + name + for name, member in module.members.items() + if not name.startswith("_") and is_in_scope(member, package, roots) + ) def build_delta( @@ -153,13 +176,16 @@ def build_delta( style: ExplanationStyle, package: str = DEFAULT_PACKAGE, ) -> ApiDelta: + roots: Final = (new, old) findings: Final = tuple( - dict.fromkeys(to_finding(breakage, style) for breakage in breakages if is_in_scope(breakage.obj, package)) + dict.fromkeys( + to_finding(breakage, style) for breakage in breakages if is_in_scope(breakage.obj, package, roots) + ) ) return ApiDelta( blocking=tuple(f for f in findings if f.kind not in ADVISORY_KINDS), advisory=tuple(f for f in findings if f.kind in ADVISORY_KINDS), - added_names=tuple(sorted(top_level_names(new) - top_level_names(old))), + added_names=tuple(sorted(top_level_names(new, package, roots) - top_level_names(old, package, roots))), ) diff --git a/tests/test_litellm/test_github_api_breaking_changes.py b/tests/test_litellm/test_github_api_breaking_changes.py index 6e267fdb68d..0cb7d58ba0f 100644 --- a/tests/test_litellm/test_github_api_breaking_changes.py +++ b/tests/test_litellm/test_github_api_breaking_changes.py @@ -168,8 +168,16 @@ class TestScope: obj = FakeObject("litellm.BedrockLLM", canonical=None, target="litellm.llms.bedrock.BedrockLLM") assert gate.is_in_scope(obj, "litellm") is True - def test_alias_without_target_falls_back_to_its_own_path(self): - assert gate.is_in_scope(FakeObject("litellm.Router", canonical=None), "litellm") is True + def test_alias_chained_through_an_internal_module_to_stdlib_is_out_of_scope(self): + chained = FakeModule("Final", external=("Final",)) + assert gate.is_in_scope(chained.members["Final"], "litellm", (chained,)) is False + + def test_alias_chained_within_the_package_stays_in_scope(self): + owned = FakeModule("completion") + assert gate.is_in_scope(owned.members["completion"], "litellm", (owned,)) is True + + def test_unidentifiable_alias_is_treated_as_out_of_scope(self): + assert gate.is_in_scope(FakeObject("litellm.Router", canonical=None), "litellm") is False class FakeKind: @@ -188,8 +196,28 @@ class FakeBreakage: class FakeModule: - def __init__(self, *names: str): - self.members = {name: object() for name in names} + """Top-level `litellm`. Names are owned by the package unless `external` says otherwise. + + `external` names mimic the real `litellm.Final`: re-exported through an + internal module, so only following the alias chain reveals they are stdlib. + """ + + name = "litellm" + + def __init__(self, *names: str, external: tuple[str, ...] = ()): + self.members = { + name: FakeObject(f"litellm.{name}", canonical=None, target=f"litellm.scheduler.{name}") + if name in external + else FakeObject(f"litellm.{name}", f"litellm.main.{name}") + for name in names + } + self._hops = { + f"scheduler.{name}": FakeObject(f"litellm.scheduler.{name}", canonical=None, target=f"typing.{name}") + for name in external + } + + def get_member(self, parts): + return self._hops[".".join(parts)] class TestBuildDelta: @@ -237,6 +265,24 @@ class TestBuildDelta: built = gate.build_delta(FakeModule("completion", "old_flag"), FakeModule("completion"), iter([]), style=None) assert built.added_names == () + def test_a_newly_imported_stdlib_name_is_not_surface_widening(self): + built = gate.build_delta( + FakeModule("completion"), + FakeModule("completion", "Final", external=("Final",)), + iter([]), + style=None, + ) + assert built.added_names == () + + def test_owned_additions_survive_alongside_ignored_stdlib_imports(self): + built = gate.build_delta( + FakeModule("completion"), + FakeModule("completion", "Final", "new_flag", external=("Final",)), + iter([]), + style=None, + ) + assert built.added_names == ("new_flag",) + class TestRendering: def test_summary_names_the_declaration_escape_hatch(self):