From c90eb7e96f7ed9ffe4b6d1601860ebffaaec0ef7 Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Fri, 12 Jun 2026 20:14:45 -0700 Subject: [PATCH] feat: ruff strict-rule suppressions baseline gate (#30303) * feat: add ruff strict-rule suppressions baseline gate Introduce a stricter ruff rule set (typed params, no Any, complexity and arg-count caps, mutable-default and global-rebinding checks) grandfathered against the current tree and enforced as a budget rather than zero-tolerance ruff-strict.toml defines the 9 rules separately from ruff.toml so the existing ruff check stays green. scripts/ruff_suppressions.py builds the per-file, per-rule baseline in ruff-suppressions.json and gates CI by failing when the total grows past the baseline plus a 0.5% slack margin. The baseline ratchets down via `make lint-suppressions-update` after fixes * fix: surface per-file drift as a warning on a passing suppressions check Greptile flagged that cmd_check computed per-file regressions but only printed them on failure, so violations shifted between files (or a brand-new file under the slack) passed with a silent OK. Print them as a non-fatal warning on the pass path too; pass/fail behavior is unchanged * refactor: gate strict ruff rules on the delta vs base, not a frozen baseline The committed total-count baseline went stale against a moving base. CI lints the PR merged with the current staging tip, so violations merged by other PRs counted against this PR and tripped the budget even though nothing here touched them Replace it with a drift-proof gate. scripts/ruff_strict_gate.py runs ruff on the head, keeps only violations on lines this change adds relative to the merge-base, and fails when a rule exceeds its per-rule allowance in ruff-strict-budget.json (all 0 today). Because the base is measured live, base drift cancels out and only what the change introduces is gated. Drops ruff-suppressions.json and the old suppressions script * chore: allow 5 new ANN001/ANN003/ANN401 per change Give the three annotation-completeness rules a small per-change allowance so a large new module is not blocked over a few untyped params or kwargs, while the correctness and structural rules (B006, C901, PLR0913, PLW0603, RUF012, ANN002) stay at 0 * feat: add TID251 typing.Any/Dict import ban and widen annotation budgets Add TID251 (flake8-tidy-imports banned-api) to ruff-strict.toml, banning new imports of typing.Any and typing.Dict and steering new code toward structured types. It counts the import site, about one per file, so it is set non-blocking at 50 as a forward-looking signal Widen the annotation-completeness budgets so they nudge rather than block: ANN001 50, ANN401 50, ANN003 25. Correctness and structural rules stay at 0 * refactor: make the strict gate a drift-safe per-rule total ceiling Switch the gate from a per-change allowance to a hard ceiling on each rule's total count across the codebase. The ceiling is baseline + slack in ruff-strict-budget.json, with baseline captured from today's tree To stay drift-safe, the gate counts each rule on the head and on the merge-base (via a throwaway git worktree) and fails a rule only when its head total is over the ceiling and higher than the base, so base drift never blames a change that did not add to that rule. Annotation rules keep generous slack (ANN001 and ANN401 50, ANN003 25, TID251 50); structural and correctness rules are frozen at today's count. Add make lint-strict-budget-update to re-capture baselines * chore: give the structural strict rules a cushion of 3 To be liberal to start, B006, C901, PLR0913, PLW0603, RUF012, and ANN002 each get a slack of 3 instead of 0, so an occasional legitimate case is not hard-blocked. The annotation budgets are unchanged, and these ratchet down later * feat: ban more typing collection aliases and tighten annotation slack to 10 Add typing.List, typing.Set, typing.MutableSequence, and typing.MutableMapping to the TID251 banned-api list, steering new code toward tuple, Sequence, Mapping, frozenset, and frozen dataclasses. This raises TID251's baseline to 2404 Bring the three rules that were at slack 50 (ANN001, ANN401, TID251) down to 10 * docs: document the strict-gate ratchet and Any-avoidance in CLAUDE.md Add a line on running make lint-strict-budget-update to knock baselines down after fixes, and a line on validating untyped inputs in the caller rather than spending the Any budget * feat: make it a bit more strict --------- Co-authored-by: mateo-berri <277851410+mateo-berri@users.noreply.github.com> --- .github/workflows/test-linting.yml | 6 + CLAUDE.md | 6 +- Makefile | 11 +- ruff-strict-budget.json | 12 ++ ruff-strict.toml | 20 +++ scripts/ruff_strict_gate.py | 162 ++++++++++++++++++++ tests/test_litellm/test_ruff_strict_gate.py | 84 ++++++++++ 7 files changed, 299 insertions(+), 2 deletions(-) create mode 100644 ruff-strict-budget.json create mode 100644 ruff-strict.toml create mode 100644 scripts/ruff_strict_gate.py create mode 100644 tests/test_litellm/test_ruff_strict_gate.py diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index b5e45a38cf9..f212dd9d15e 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -67,6 +67,12 @@ jobs: uv run --no-sync ruff check . cd .. + - name: Check strict-rule budget (delta vs base) + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + uv run --no-sync python scripts/ruff_strict_gate.py --base "$BASE_SHA" + - name: Print OpenAI version run: | uv run --no-sync python -c "import openai; print(f'OpenAI version: {openai.__version__}')" diff --git a/CLAUDE.md b/CLAUDE.md index 93059febb28..32fd0aadddb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,6 +36,8 @@ Don't hesitate to use values in .env to get needed API keys and other secrets, a Run tests, format your code, and lint your code before each commit +When you fix strict-rule violations gated by `ruff-strict-budget.json`, run `make lint-strict-budget-update` and commit the lowered baselines so the ceilings ratchet down instead of leaving stale headroom + Ask to commit and push your work when you're done (or if you're confident that your code is good and works, just do it) When you must use real LLM models to, for example, write e2e tests, write a QA runbook, etc., make sure to use the latest models (doesn't have to be smartest, can also be a modern small, fast one. No strong preference for smart vs fast here, just use something modern) as of the year and month of the current date. Do a web search as necessary to figure that out @@ -57,7 +59,7 @@ Follow these coding conventions for new/updated code (a three-line fix in a lega - Composition over inheritance - Never-nester: early returns over deep nesting - Don't throw; model failures as values (One function (e.g., raise_public) maps error union to existing public exception contracts via exhaustive match + assert_never) -- No mutation; don't reassign variables. Instead of mutable lists and dicts, prefer tuples, frozen dataclasses (with slots=True), etc. +- No mutation; don't reassign variables, global or local. Instead of mutable lists and dicts, prefer tuples, frozen dataclasses (with slots=True), etc. - Use dependency injection - Fully typed; no `Any` or coarse types like `dict[str, Any]` or just `dict`. Every function parameter must be strongly typed - Use tagged unions + match @@ -65,6 +67,8 @@ Follow these coding conventions for new/updated code (a three-line fix in a lega - No file sprawl: deliberate file and folder structure - Standard over hand-rolled: use the official SDK or a library where one exists; where none does, follow industry standards instead of inventing local conventions +if you're trying to create a new function that relies on untyped stuff, instead of adding more Any's and bringing it closer to the max, just validate it in the caller (a simple function that returns the typed thing or raises will do) and then pass the now typed variable in + Follow conventional commits for commit names and PR titles ## Think Before Coding diff --git a/Makefile b/Makefile index 3d7b51bc745..fe4be29f9b6 100644 --- a/Makefile +++ b/Makefile @@ -5,6 +5,7 @@ test-unit-integrations test-unit-core-utils test-unit-other test-unit-root \ test-proxy-unit-a test-proxy-unit-b test-integration test-unit-helm \ info lint lint-dev format \ + lint-strict-budget lint-strict-budget-update \ install-dev install-proxy-dev install-test-deps install-hooks \ install-helm-unittest check-circular-imports check-import-safety @@ -24,6 +25,8 @@ help: @echo " make lint-ruff - Run Ruff linting only" @echo " make lint-mypy - Run MyPy type checking only" @echo " make lint-black - Check Black formatting (matches CI)" + @echo " make lint-strict-budget - Gate the codebase total of each strict ruff rule against its ceiling" + @echo " make lint-strict-budget-update - Re-capture per-rule baselines in ruff-strict-budget.json (ratchet)" @echo " make check-circular-imports - Check for circular imports" @echo " make check-import-safety - Check import safety" @echo " make test - Run all tests" @@ -122,6 +125,12 @@ lint-mypy: install-dev lint-black: format-check +lint-strict-budget: install-dev + $(UV_RUN) python scripts/ruff_strict_gate.py + +lint-strict-budget-update: install-dev + $(UV_RUN) python scripts/ruff_strict_gate.py --update + check-circular-imports: install-dev cd litellm && $(UV_RUN) python ../tests/documentation_tests/test_circular_imports.py && cd .. @@ -129,7 +138,7 @@ check-import-safety: install-dev @$(UV_RUN) python -c "from litellm import *; print('[from litellm import *] OK! no issues!');" || (echo '🚨 import failed, this means you introduced unprotected imports! 🚨'; exit 1) # Combined linting (matches test-linting.yml workflow) -lint: format-check lint-ruff lint-mypy check-circular-imports check-import-safety +lint: format-check lint-ruff lint-mypy check-circular-imports check-import-safety lint-strict-budget # Faster linting for local development (only checks changed code) lint-dev: lint-format-changed lint-mypy check-circular-imports check-import-safety diff --git a/ruff-strict-budget.json b/ruff-strict-budget.json new file mode 100644 index 00000000000..6363b72353f --- /dev/null +++ b/ruff-strict-budget.json @@ -0,0 +1,12 @@ +{ + "ANN001": { "baseline": 2865, "slack": 10 }, + "ANN002": { "baseline": 64, "slack": 3 }, + "ANN003": { "baseline": 759, "slack": 10 }, + "ANN401": { "baseline": 1885, "slack": 10 }, + "B006": { "baseline": 180, "slack": 3 }, + "C901": { "baseline": 301, "slack": 3 }, + "PLR0913": { "baseline": 1813, "slack": 3 }, + "PLW0603": { "baseline": 183, "slack": 3 }, + "RUF012": { "baseline": 158, "slack": 3 }, + "TID251": { "baseline": 2404, "slack": 10 } +} diff --git a/ruff-strict.toml b/ruff-strict.toml new file mode 100644 index 00000000000..03145255ebf --- /dev/null +++ b/ruff-strict.toml @@ -0,0 +1,20 @@ +extend = "ruff.toml" + +[lint] +select = ["ANN001", "ANN002", "ANN003", "ANN401", "B006", "C901", "PLR0913", "PLW0603", "RUF012", "TID251"] +extend-select = [] + +[lint.mccabe] +max-complexity = 15 + +[lint.pylint] +max-args = 5 + +[lint.flake8-tidy-imports.banned-api] +"typing.Any".msg = "Use a concrete type. Frozen slots=True dataclass (preferred) / NamedTuple / ReadOnly TypedDict for payloads." +"typing_extensions.Any".msg = "Same as typing.Any." +"typing.List".msg = "tuple[X, ...] for state, Sequence[X] for params." +"typing.Dict".msg = "Frozen dataclass / NamedTuple / ReadOnly TypedDict; create a Mapping alias with concrete value types if truly dynamic." +"typing.Set".msg = "frozenset[X] or AbstractSet[X]." +"typing.MutableSequence".msg = "Sequence[X]." +"typing.MutableMapping".msg = "See typing.Dict." \ No newline at end of file diff --git a/scripts/ruff_strict_gate.py b/scripts/ruff_strict_gate.py new file mode 100644 index 00000000000..5951a1215ed --- /dev/null +++ b/scripts/ruff_strict_gate.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +"""Total-count gate for the strict ruff rules in ruff-strict.toml. + +Each rule has a hard ceiling (baseline + slack) in ruff-strict-budget.json. The +gate counts each rule across the whole tree and fails when a rule is both over +its ceiling and higher than the base it merges into, so a change is blamed for +the violations it adds, never for drift that already exists in the base. +""" + +import argparse +import json +import re +import shutil +import subprocess +import sys +import tempfile +from collections import Counter +from pathlib import Path +from typing import NamedTuple + +REPO_ROOT = Path(__file__).resolve().parent.parent +STRICT_CONFIG = REPO_ROOT / "ruff-strict.toml" +BUDGET_PATH = REPO_ROOT / "ruff-strict-budget.json" +TARGET = "litellm" +DEFAULT_BASE = "origin/litellm_internal_staging" + +_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") + + +class Violation(NamedTuple): + file: str + line: int + code: str + + +class Breach(NamedTuple): + rule: str + total: int + cap: int + added: int + + +def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: + proc = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) + if proc.returncode not in (0, 1): + sys.stderr.write(proc.stderr) + raise SystemExit(f"{cmd[0]} exited {proc.returncode}") + return proc.stdout + + +def _ruff_json(cwd: Path, config: Path) -> list: + raw = _run( + ["ruff", "check", TARGET, "--config", str(config), "--output-format", "json"], + cwd=cwd, + ) + return json.loads(raw or "[]") + + +def head_violations() -> list: + out = [] + for item in _ruff_json(REPO_ROOT, STRICT_CONFIG): + name = Path(item["filename"]) + rel = ( + (name if name.is_absolute() else REPO_ROOT / name) + .resolve() + .relative_to(REPO_ROOT) + .as_posix() + ) + out.append(Violation(rel, item["location"]["row"], item["code"])) + return out + + +def count_by_rule(violations: list) -> dict: + return dict(Counter(v.code for v in violations)) + + +def base_counts(ref: str) -> dict: + parent = Path(tempfile.mkdtemp(prefix="ruff_base_")) + worktree = parent / "wt" + try: + _run(["git", "worktree", "add", "--detach", str(worktree), ref]) + shutil.copy(STRICT_CONFIG, worktree / "ruff-strict.toml") + items = _ruff_json(worktree, worktree / "ruff-strict.toml") + return dict(Counter(item["code"] for item in items)) + finally: + _run(["git", "worktree", "remove", "--force", str(worktree)]) + shutil.rmtree(parent, ignore_errors=True) + + +def evaluate(head: dict, base: dict, budget: dict) -> list: + breaches = [] + for rule, spec in budget.items(): + cap = spec["baseline"] + spec["slack"] + total = head.get(rule, 0) + if total > cap and total > base.get(rule, 0): + breaches.append(Breach(rule, total, cap, total - base.get(rule, 0))) + return sorted(breaches) + + +def parse_changed_lines(diff_text: str) -> dict: + changed: dict = {} + path = None + for line in diff_text.splitlines(): + if line.startswith("+++ b/"): + path = line[6:] + elif path and (match := _HUNK.match(line)): + start = int(match.group(1)) + count = int(match.group(2)) if match.group(2) is not None else 1 + changed.setdefault(path, set()).update(range(start, start + count)) + return changed + + +def introduced(violations: list, changed: dict) -> list: + return [v for v in violations if v.line in changed.get(v.file, set())] + + +def cmd_check(base: str) -> None: + budget = json.loads(BUDGET_PATH.read_text()) + head = head_violations() + base_point = _run(["git", "merge-base", base, "HEAD"]).strip() or base + breaches = evaluate(count_by_rule(head), base_counts(base_point), budget) + if not breaches: + print(f"OK: every strict rule is within its codebase ceiling (base {base})") + return + new = introduced( + head, + parse_changed_lines( + _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) + ), + ) + print(f"FAIL: strict-rule totals exceed their ceiling (base {base}):") + for breach in breaches: + print( + f" {breach.rule}: total {breach.total} over cap {breach.cap} (this change added {breach.added})" + ) + for violation in sorted(v for v in new if v.code == breach.rule): + print(f" {violation.file}:{violation.line}") + print( + "Reduce the new violations or remove an equal number elsewhere; the ceiling is baseline + slack in ruff-strict-budget.json." + ) + raise SystemExit(1) + + +def cmd_update() -> None: + budget = json.loads(BUDGET_PATH.read_text()) + head = count_by_rule(head_violations()) + for rule in budget: + budget[rule]["baseline"] = head.get(rule, 0) + BUDGET_PATH.write_text(json.dumps(budget, indent=2, sort_keys=True) + "\n") + print("Re-captured per-rule baselines from the current tree") + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--base", default=DEFAULT_BASE) + parser.add_argument("--update", action="store_true") + args = parser.parse_args() + cmd_update() if args.update else cmd_check(args.base) + + +if __name__ == "__main__": + main() diff --git a/tests/test_litellm/test_ruff_strict_gate.py b/tests/test_litellm/test_ruff_strict_gate.py new file mode 100644 index 00000000000..22255f0555e --- /dev/null +++ b/tests/test_litellm/test_ruff_strict_gate.py @@ -0,0 +1,84 @@ +import importlib.util +from pathlib import Path + +import pytest + +_MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ruff_strict_gate.py" +_spec = importlib.util.spec_from_file_location("ruff_strict_gate", _MODULE_PATH) +gate = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(gate) + +Violation = gate.Violation + + +def rule(name, baseline, slack): + return {name: {"baseline": baseline, "slack": slack}} + + +def test_under_ceiling_passes(): + assert gate.evaluate({"ANN001": 100}, {"ANN001": 100}, rule("ANN001", 90, 20)) == [] + + +def test_ceiling_is_baseline_plus_slack_boundary(): + budget = rule("ANN001", 90, 20) # cap 110 + at = gate.evaluate({"ANN001": 110}, {"ANN001": 90}, budget) + over = gate.evaluate({"ANN001": 111}, {"ANN001": 90}, budget) + assert at == [] + assert [b.rule for b in over] == ["ANN001"] + assert over[0].cap == 110 + assert over[0].added == 21 + + +def test_over_ceiling_and_change_added_fails(): + breaches = gate.evaluate({"C901": 11}, {"C901": 9}, rule("C901", 10, 0)) + assert [b.rule for b in breaches] == ["C901"] + assert breaches[0].added == 2 + + +def test_base_already_over_ceiling_change_added_nothing_is_not_blamed(): + # drift safety: base is over cap, this change leaves the count where it is + assert gate.evaluate({"C901": 15}, {"C901": 15}, rule("C901", 10, 0)) == [] + + +def test_change_that_reduces_an_over_ceiling_rule_is_not_blamed(): + # still over cap, but moving the right direction + assert gate.evaluate({"C901": 14}, {"C901": 16}, rule("C901", 10, 0)) == [] + + +def test_rules_are_independent(): + budget = {**rule("ANN001", 100, 50), **rule("C901", 10, 0)} + breaches = gate.evaluate( + {"ANN001": 130, "C901": 11}, {"ANN001": 100, "C901": 10}, budget + ) + assert [b.rule for b in breaches] == ["C901"] # ANN001 130 <= 150, C901 11 > 10 + + +def test_missing_rule_counts_as_zero(): + assert gate.evaluate({}, {}, rule("C901", 0, 0)) == [] + + +def test_parse_changed_lines_maps_added_lines_per_file(): + diff = ( + "+++ b/litellm/a.py\n" + "@@ -10 +10,3 @@\n+x\n+y\n+z\n" + "+++ b/litellm/b.py\n" + "@@ -5,2 +7 @@\n+q\n" + ) + changed = gate.parse_changed_lines(diff) + assert changed["litellm/a.py"] == {10, 11, 12} + assert changed["litellm/b.py"] == {7} + + +def test_introduced_keeps_only_violations_on_changed_lines(): + violations = [ + Violation("litellm/a.py", 10, "ANN001"), + Violation("litellm/a.py", 99, "C901"), + ] + assert gate.introduced(violations, {"litellm/a.py": {10}}) == [ + Violation("litellm/a.py", 10, "ANN001") + ] + + +@pytest.mark.parametrize("hunk", ["@@ -1 +1 @@", "@@ -1,0 +1,2 @@"]) +def test_parse_changed_lines_handles_single_and_ranged_hunks(hunk): + assert gate.parse_changed_lines(f"+++ b/litellm/a.py\n{hunk}\n")["litellm/a.py"]