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"]