Skip to content

chore(lint): stop ratcheting *-budget.json on PR branches - #39937

Merged
yassin-berriai merged 2 commits into
litellm_internal_stagingfrom
litellm_ratchet_budgets_on_staging
Sep 6, 2026
Merged

yassin-berriai merged 2 commits into
litellm_internal_stagingfrom
litellm_ratchet_budgets_on_staging

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • Every PR that fixes LIT/ruff/TQ violations commits lowered *-budget.json limits
  • Concurrent PRs edit the same "limit" lines, so nearly every PR hits a merge conflict
  • test_quality_gate.py even fails a PR that did not ratchet, forcing the edit

How it solves it:

  • CLAUDE.md now says never to touch budget files or run make lint-budget-update on a PR branch
  • test_quality_gate.py drops its "you must ratchet" failure and its unused touches_measured_tree base scan
  • The ratchet itself moves to a scheduled Devin automation that opens its own PR against litellm_internal_staging (set up separately, not in this repo)

User Flow

Before: a contributor with two open PRs that both fix Final annotations cannot merge the second one

  1. They open PR A and PR B off litellm_internal_staging, each fixing a few LIT010 violations and each committing type-discipline-budget.json with a lowered LIT010 limit as CLAUDE.md required
  2. PR A merges
  3. PR B's page shows "This branch has conflicts that must be resolved: type-discipline-budget.json" and the merge button is disabled
  4. They resolve the conflict by hand, push, wait for CI again, and repeat for every other PR they have open

After: the same two PRs merge back to back with no conflict

  1. They open PR A and PR B, each fixing a few LIT010 violations, neither touching any budget file
  2. PR A merges, PR B's merge button stays green, PR B merges
  3. On its schedule, the ratchet automation opens one PR lowering the limits by what both cleared; that PR is the only place budget files change

Relevant issues

Slack thread in #eng, 2026-09-05, "this file is frequently causing merge conflicts on all of my PRs"

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*, make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to 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

Behavior change is in the CI gate, so the proof is the gate command the automation and CI run

After (bf38443)

  1. BASE=$(git rev-parse origin/litellm_internal_staging~8) (86e54a0)
  2. uv run --no-sync python scripts/type_discipline_gate.py --update --base "$BASE"
  3. Output: Ratcheted LIT-rule limits down by 3 violations this branch fixed (32s wall clock), diff lowers LIT001 22180 -> 22179 and LIT004 38 -> 36. This is the command the automation runs against the last ratchet commit
  4. uv run --no-sync pytest -q tests/test_litellm/test_test_quality_gate.py tests/test_litellm/test_budget_ratchet_check.py: 37 passed

Type

🚄 Infrastructure

Caveats (if any)

Medium

  • Until the automation runs, a rule that a merged PR cleared keeps its old ceiling, so a later PR could re-add up to that many violations without failing the gate. The window is one automation interval
  • PR CI reads the budget file at the PR head, so a PR branched before a ratchet is gated against slightly stale (higher) limits until it rebases. Same as today

Low

  • Open PRs that already carry budget edits still merge fine; they only conflict with each other until they rebase

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

Link to Devin session: https://app.devin.ai/sessions/191f37becd4f4fe38996e84753162c28
Open in Devin Desktop: https://app.devin.ai/desktop/session/191f37becd4f4fe38996e84753162c28?variant=devin
Requested by: @yassin-berriai

…f PR branches

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves lint-budget ratcheting from contributor branches to a write-capable workflow on litellm_internal_staging and removes the test-quality gate's PR-side requirement to commit lowered budgets.

  • Adds a workflow that runs all four budget gates in update mode and pushes resulting budget changes.
  • Updates contributor guidance to prohibit budget edits on PR branches.
  • Removes test-quality gate logic and tests for detecting unratcheted fixes.
  • The workflow does not bind each run's checked-out head to its push event, allowing overlapping ranges to be ratcheted more than once.

Confidence Score: 4/5

The PR is not safe to merge until each workflow run measures a head fixed to its own push range and the new ratchet behavior has regression coverage.

A realistic timing window lets an older run check out code from a later push while retaining its original base, causing the later push's reductions to be applied once by each run and potentially driving limits below current counts.

Files Needing Attention: .github/workflows/ratchet-budgets.yml, tests/test_litellm/test_test_quality_gate.py

Important Files Changed

Filename Overview
.github/workflows/ratchet-budgets.yml Adds the staging ratchet workflow, but its branch-tip checkout can make concurrent push ranges overlap and double-apply reductions.
scripts/test_quality_gate.py Removes PR-side stale-ceiling enforcement and retains update-mode delta ratcheting for the staging workflow.
tests/test_litellm/test_test_quality_gate.py Removes obsolete unratcheted tests without replacing them with coverage of the workflow now responsible for ratcheting.
CLAUDE.md Updates contributor guidance consistently with the intended staging-only ratchet process.

Reviews (1): Last reviewed commit: "ci(lint): ratchet *-budget.json on litel..." | Re-trigger Greptile

Comment thread .github/workflows/ratchet-budgets.yml Outdated
steps:
- uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0
with:
ref: litellm_internal_staging

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Push ranges can overlap

If a second push lands after an older ratchet run starts but before its checkout resolves, the older run checks out the newer branch tip while keeping its original event's before SHA as the base. It then ratchets fixes from both pushes, and the second push's own run ratchets its fixes again. Because each gate subtracts the measured reduction from the existing limit, this can count the same fixes twice and lower staging limits below the actual violation counts.

@@ -73,29 +71,6 @@ def test_ratchet_lowers_a_rule_introduced_on_this_branch_like_any_other():
assert updated["TQ001"]["limit"] == 4


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Ratchet coverage was removed

These changes remove the previous ratchet-enforcement tests without adding coverage for the workflow that now owns this behavior. No test models queued push ranges or verifies that each fix is applied exactly once. This violates the repository directive that modifications to existing tests must not weaken regression coverage, so replacement coverage is required before merging.

Rule Used: What: Flag any modifications to existing tests and... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…cheduled automation

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration devin-ai-integration Bot changed the title ci(lint): ratchet *-budget.json on litellm_internal_staging instead of PR branches chore(lint): stop ratcheting *-budget.json on PR branches Sep 5, 2026
@yassin-berriai
yassin-berriai merged commit 168a005 into litellm_internal_staging Sep 6, 2026
176 of 181 checks passed
@yassin-berriai
yassin-berriai deleted the litellm_ratchet_budgets_on_staging branch September 6, 2026 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants