ci: fail PRs that change the public litellm API without declaring it - #36224
Open
ryan-crabbe-berri wants to merge 2 commits into
Open
ci: fail PRs that change the public litellm API without declaring it#36224ryan-crabbe-berri wants to merge 2 commits into
ryan-crabbe-berri wants to merge 2 commits into
Conversation
Adds a griffe-based CI gate over the importable litellm SDK surface. Breaking changes (removed objects, changed signatures, changed defaults) need a Conventional Commits declaration, either a bang after the type or a BREAKING CHANGE: footer. Newly exported top-level names need a feat: or fix: title so a chore/refactor PR cannot quietly widen the public API. Scope is deliberately narrower than raw griffe output: litellm.proxy.* is excluded because the proxy's contract is HTTP rather than Python, and re-exported stdlib names are excluded because their canonical home is another package. On a 17-day sample window that took the finding count from 46 to 11, with all 11 genuine.
Contributor
Greptile SummaryThe PR adds a CI gate that compares the public
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| .github/scripts/check_api_breaking_changes.py | Implements static Griffe-based API comparison, ownership filtering, declaration parsing, verdict selection, and CI reporting; the previously reported added-name filtering defect is addressed. |
| .github/workflows/check-api-breaking-changes.yml | Runs the API comparison for relevant Python and gate changes using the pull request base SHA and pinned tooling. |
| tests/test_litellm/test_github_api_breaking_changes.py | Covers declaration parsing, scope and alias handling, API-delta decisions, deduplication, and output rendering. |
Reviews (2): Last reviewed commit: "fix: apply canonical-ownership scoping t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@greptileai re review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Problem this solves:
import litellmchore:PR can quietly widen the public APIHow it solves it:
feat!:or aBREAKING CHANGE:footerfeat:orfix:titleRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Run at commit
f9df01b675, against real repo history rather than fixtures. The commands below are what the CI step runs, with the base ref pointed at a past commit so there is something to find.214945ais 17 days of history back fromHEADPer-PR noise was sampled on three recent merges (#36054, #35137, #36014) and all three came back clean, so this should not red-CI ordinary work. The whole run takes about 9 seconds.
Type
🚄 Infrastructure
Changes
.github/scripts/check_api_breaking_changes.pyloads thelitellmpackage at the base ref and at the PR head with griffe, statically, never importing it, then applies two gates.The first is griffe's own breaking-change detection: removed objects, removed or reordered parameters, parameters that became required, changed defaults, incompatible return types. Those fail the job unless the PR declares a breaking change the Conventional Commits way, either a
!after the type in the title or aBREAKING CHANGE:footer in the body. The repo already gates PR titles on Conventional Commits and squash-merge uses the PR title and body as the commit message, so the declaration ends up in the history where a changelog can find it.The second gate covers additions. A new
litellm.<name>export needs afeat:orfix:title, which stops achore:orrefactor:PR from widening the public surface as a side effect.Scope is narrower than raw griffe output, deliberately.
litellm.proxy.*is excluded because the proxy's contract is HTTP and belongs to a spec diff rather than a Python API diff, and re-exported stdlib names are excluded because deletingfrom typing import Unionduring the ongoingX | Nonemodernization is not a break for anyone. On the 17-day sample window that scoping cut findings from 46 to 11, and the 11 above are all real. Attribute value changes stay advisory and never block, since the same modernization rewrites them constantly.Ownership is decided by following the alias chain to where a name actually lives, which matters because the re-export is often indirect:
litellm.Finalreachestyping.Finalthroughlitellm.scheduler, so a one-hop check would have called it ours and failed anyrefactor:PR that added a type import tolitellm/__init__.py.litellm.BedrockLLMresolves inside the package over the same machinery and is still caught.The 44 unit tests cover the decision logic, the scope filter, alias-chain resolution, and rendering, using fakes for griffe objects rather than a live git repo. Thirteen targeted mutations of the gate (letting
chorewiden the surface, putting proxy internals back in scope, following only one alias hop, skipping the scope filter on added names, dropping the dedupe, reordering the two checks, loosening the footer regex, ignoring the!) were each killed by the suite.Final Attestation