diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py index 10a78483643..e97cd1bca00 100644 --- a/scripts/budget_ratchet_check.py +++ b/scripts/budget_ratchet_check.py @@ -10,7 +10,10 @@ content at the merge-base with the target branch and fails (exits 1, red) if: * a rule was dropped from a budget (its ceiling effectively became infinite), or * an entire budget file was deleted. -New rules and lowered/equal limits are fine. +New rules and lowered/equal limits are fine. So is a rule that graduated: once a +paired config (ruff.toml for the ruff-strict budget) selects the rule outright it +hard-fails at the first violation, which is stricter than any ceiling the budget +could hold, so dropping its entry tightens the guard rather than removing it. This is deliberately NOT a gating check. It should turn the run red so that a loosening is impossible to miss in review, but it must stay OUT of the @@ -30,7 +33,9 @@ import argparse import json import subprocess import sys +import tomllib from pathlib import Path +from types import MappingProxyType from typing import NamedTuple REPO_ROOT = Path(__file__).resolve().parent.parent @@ -40,6 +45,7 @@ DEFAULT_BUDGETS: tuple[str, ...] = ( "type-discipline-budget.json", "basedpyright-code-budget.json", ) +GRADUATION_CONFIGS = MappingProxyType({"ruff-strict-budget.json": "ruff.toml"}) class Regression(NamedTuple): @@ -106,24 +112,57 @@ def _limits(budget: dict) -> dict[str, int]: } +def selectors_hard_failed_by(lint: dict) -> tuple[str, ...]: + """A ruff `[lint]` table's selected codes, minus anything `ignore` turns back off. + + `lint.ignore` wins over `lint.extend-select` in ruff, so an ignored code is not + actually enforced and must not count as a graduation. + """ + ignored = tuple(lint.get("ignore", ())) + return tuple( + selector + for selector in lint.get("extend-select", ()) + if not (ignored and selector.startswith(ignored)) + ) + + +def graduated_selectors(rel: str) -> tuple[str, ...]: + """Selectors the budget's paired ruff config hard-fails, so its ceiling is moot.""" + config = GRADUATION_CONFIGS.get(rel) + if config is None or not (REPO_ROOT / config).exists(): + return () + return selectors_hard_failed_by( + tomllib.loads((REPO_ROOT / config).read_text()).get("lint", {}) + ) + + def _regression_detail( rule: str, base_limits: dict[str, int], head_limits: dict[str, int], + graduated: tuple[str, ...], ) -> str | None: - """Why `rule` regressed vs base, or None when it held flat or fell. + """Why `rule` regressed vs base, or None when it held flat, fell, or graduated. - A dropped rule is terminal; otherwise the only loosening left is a raised limit. + A dropped rule is terminal unless it graduated; otherwise the only loosening + left is a raised limit. """ base_limit = base_limits[rule] if rule not in head_limits: + if graduated and rule.startswith(graduated): + return None return f"rule dropped (limit {base_limit} -> removed)" if head_limits[rule] > base_limit: return f"limit raised {base_limit} -> {head_limits[rule]}" return None -def regressions_for(rel: str, base: dict | None, head: dict | None) -> list[Regression]: +def regressions_for( + rel: str, + base: dict | None, + head: dict | None, + graduated: tuple[str, ...] = (), +) -> list[Regression]: if base is None: return [] # new budget file: nothing to ratchet against yet if head is None: @@ -133,7 +172,7 @@ def regressions_for(rel: str, base: dict | None, head: dict | None) -> list[Regr return [ Regression(rel, rule, detail) for rule in sorted(base_limits) - if (detail := _regression_detail(rule, base_limits, head_limits)) is not None + if (detail := _regression_detail(rule, base_limits, head_limits, graduated)) is not None ] @@ -164,7 +203,7 @@ def main() -> int: print(f"skip {rel}: new file (no base at {args.base} to ratchet against)") continue checked.append(rel) - regressions.extend(regressions_for(rel, base, head)) + regressions.extend(regressions_for(rel, base, head, graduated_selectors(rel))) if regressions: print( diff --git a/tests/test_litellm/test_budget_ratchet_check.py b/tests/test_litellm/test_budget_ratchet_check.py index 1972c1b6386..22d05f4d00d 100644 --- a/tests/test_litellm/test_budget_ratchet_check.py +++ b/tests/test_litellm/test_budget_ratchet_check.py @@ -68,6 +68,50 @@ def test_new_rule_in_head_is_clean(): assert ratchet.regressions_for("b.json", {}, {"new-rule": _spec_of(5)}) == [] +def test_dropped_rule_that_graduated_to_a_hard_failing_config_is_clean(): + base = {"UP006": _spec_of(0)} + assert ratchet.regressions_for("b.json", base, {}, graduated=("UP006",)) == [] + + +def test_graduation_matches_by_prefix_like_ruff_selectors_do(): + base = {"ANN202": _spec_of(865)} + assert ratchet.regressions_for("b.json", base, {}, graduated=("ANN",)) == [] + + +def test_an_unrelated_graduation_does_not_excuse_a_dropped_rule(): + base = {"C901": _spec_of(3)} + regs = ratchet.regressions_for("b.json", base, {}, graduated=("UP006", "SIM118")) + assert [r.rule for r in regs] == ["C901"] + assert "dropped" in regs[0].detail + + +def test_graduation_never_excuses_a_raised_limit(): + base = {"UP006": _spec_of(0)} + regs = ratchet.regressions_for("b.json", base, {"UP006": _spec_of(7)}, graduated=("UP006",)) + assert [r.rule for r in regs] == ["UP006"] + assert "0 -> 7" in regs[0].detail + + +def test_graduated_selectors_come_from_the_paired_ruff_config(): + selectors = ratchet.graduated_selectors("ruff-strict-budget.json") + assert "UP006" in selectors + assert "ANN" not in selectors + + +def test_budgets_without_a_paired_config_can_never_graduate(): + assert ratchet.graduated_selectors("type-discipline-budget.json") == () + assert ratchet.graduated_selectors("basedpyright-code-budget.json") == () + + +def test_a_selector_the_config_also_ignores_does_not_count_as_graduated(): + lint = {"ignore": ["UP006"], "extend-select": ["UP006", "SIM118"]} + assert ratchet.selectors_hard_failed_by(lint) == ("SIM118",) + + +def test_selectors_hard_failed_by_reads_a_config_with_no_ignore_list(): + assert ratchet.selectors_hard_failed_by({"extend-select": ["UP006"]}) == ("UP006",) + + def test_deleted_budget_file_is_a_regression(): regs = ratchet.regressions_for("b.json", {"LIT006": _spec_of(1)}, None) assert [r.rule for r in regs] == ["*"]