diff --git a/ruff-strict-budget.json b/ruff-strict-budget.json index ae46f020de1..10c820324ea 100644 --- a/ruff-strict-budget.json +++ b/ruff-strict-budget.json @@ -1,7 +1,7 @@ { "ANN001": { "baseline": 2865, - "slack": 50 + "slack": 287 }, "ANN002": { "baseline": 64, @@ -9,19 +9,19 @@ }, "ANN003": { "baseline": 759, - "slack": 30 + "slack": 76 }, "ANN201": { "baseline": 1944, - "slack": 50 + "slack": 194 }, "ANN202": { "baseline": 858, - "slack": 30 + "slack": 86 }, "ANN204": { "baseline": 658, - "slack": 20 + "slack": 66 }, "ANN205": { "baseline": 117, @@ -33,7 +33,7 @@ }, "ANN401": { "baseline": 1886, - "slack": 50 + "slack": 189 }, "ASYNC230": { "baseline": 11, @@ -231,10 +231,6 @@ "baseline": 6, "slack": 3 }, - "PLR0913": { - "baseline": 1813, - "slack": 50 - }, "PLR1704": { "baseline": 3, "slack": 3 diff --git a/ruff-strict.toml b/ruff-strict.toml index 1caa3567872..d58885fe848 100644 --- a/ruff-strict.toml +++ b/ruff-strict.toml @@ -2,7 +2,7 @@ extend = "ruff.toml" [lint] preview = true -select = ["ANN", "ASYNC230", "B004", "B006", "B008", "B009", "B010", "B018", "B019", "B021", "B026", "B033", "BLE", "C401", "C404", "C405", "C408", "C414", "C419", "C901", "D419", "DTZ001", "DTZ003", "DTZ005", "DTZ006", "DTZ007", "DTZ011", "EXE001", "EXE002", "F401", "FURB136", "FURB168", "FURB188", "I001", "LOG015", "N999", "PERF102", "PERF401", "PERF402", "PERF403", "PIE790", "PIE800", "PIE804", "PIE810", "PLC0206", "PLC0208", "PLC0414", "PLR0124", "PLR0206", "PLR0402", "PLR0913", "PLR1704", "PLR1711", "PLR1714", "PLR1730", "PLR2044", "PLW0127", "PLW0133", "PLW0602", "PLW0603", "PLW1508", "PLW1510", "PYI030", "PYI036", "PYI041", "PYI064", "RET501", "RET504", "RUF010", "RUF012", "RUF015", "RUF019", "RUF022", "RUF023", "RUF046", "RUF051", "RUF059", "RUF100", "S110", "S112", "SIM101", "SIM102", "SIM103", "SIM113", "SIM114", "SIM115", "SIM117", "SIM118", "SIM201", "SIM210", "SIM211", "SIM222", "SIM401", "TC004", "TC005", "TID251", "TRY002", "TRY004", "TRY201", "TRY203", "TRY300", "UP006", "UP007", "UP008", "UP012", "UP018", "UP024", "UP028", "UP031", "UP032", "UP034", "UP035", "UP036", "UP037", "UP045"] +select = ["ANN", "ASYNC230", "B004", "B006", "B008", "B009", "B010", "B018", "B019", "B021", "B026", "B033", "BLE", "C401", "C404", "C405", "C408", "C414", "C419", "C901", "D419", "DTZ001", "DTZ003", "DTZ005", "DTZ006", "DTZ007", "DTZ011", "EXE001", "EXE002", "F401", "FURB136", "FURB168", "FURB188", "I001", "LOG015", "N999", "PERF102", "PERF401", "PERF402", "PERF403", "PIE790", "PIE800", "PIE804", "PIE810", "PLC0206", "PLC0208", "PLC0414", "PLR0124", "PLR0206", "PLR0402", "PLR1704", "PLR1711", "PLR1714", "PLR1730", "PLR2044", "PLW0127", "PLW0133", "PLW0602", "PLW0603", "PLW1508", "PLW1510", "PYI030", "PYI036", "PYI041", "PYI064", "RET501", "RET504", "RUF010", "RUF012", "RUF015", "RUF019", "RUF022", "RUF023", "RUF046", "RUF051", "RUF059", "RUF100", "S110", "S112", "SIM101", "SIM102", "SIM103", "SIM113", "SIM114", "SIM115", "SIM117", "SIM118", "SIM201", "SIM210", "SIM211", "SIM222", "SIM401", "TC004", "TC005", "TID251", "TRY002", "TRY004", "TRY201", "TRY203", "TRY300", "UP006", "UP007", "UP008", "UP012", "UP018", "UP024", "UP028", "UP031", "UP032", "UP034", "UP035", "UP036", "UP037", "UP045"] extend-select = [] [lint.mccabe] diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py index 861d65489e8..df9815d6557 100644 --- a/scripts/budget_ratchet_check.py +++ b/scripts/budget_ratchet_check.py @@ -1,16 +1,19 @@ #!/usr/bin/env python3 -"""Non-gating ratchet guard: budget ceilings may only fall, never rise. +"""Non-gating ratchet guard: budget baselines and ceilings may only fall, never rise. Every `*-budget.json` file (ruff-strict, type-discipline, basedpyright-code) is a -one-way ratchet: each rule's ceiling is `baseline + slack`, and the whole point is -to drive that number DOWN over time. This check compares every budget file against -its own content at the merge-base with the target branch and fails (exits 1, red) if: - - * a rule's ceiling went up, +one-way ratchet: each rule's ceiling is `baseline + slack`, and both the recorded +`baseline` (the live violation count) and that ceiling are meant to be driven DOWN +over time. This check compares every budget file against its own content at the +merge-base with the target branch and fails (exits 1, red) if: + + * a rule's ceiling (`baseline + slack`) went up, + * a rule's `baseline` went up, even if `slack` was lowered to keep the ceiling + flat (a higher baseline bakes in more accepted debt and must be acknowledged), * a rule was dropped from a budget (its ceiling effectively became infinite), or * an entire budget file was deleted. -New rules and lowered/equal ceilings are fine. +New rules and lowered/equal baselines and ceilings are fine. 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 @@ -66,7 +69,12 @@ def _load_head(rel: str) -> dict | None: def _ref_is_commit(ref: str) -> bool: - return _run(["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"]).returncode == 0 + return ( + _run( + ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"] + ).returncode + == 0 + ) def _load_base(rel: str, ref: str) -> dict | None: @@ -81,13 +89,55 @@ def _load_base(rel: str, ref: str) -> dict | None: return json.loads(proc.stdout) +def _baselines(budget: dict) -> dict[str, int]: + """Map each rule to its recorded baseline; skip malformed specs.""" + return { + rule: int(spec.get("baseline", 0)) + for rule, spec in budget.items() + if isinstance(spec, dict) + } + + def _caps(budget: dict) -> dict[str, int]: """Map each rule to its ceiling (baseline + slack); skip malformed specs.""" - caps: dict[str, int] = {} - for rule, spec in budget.items(): - if isinstance(spec, dict): - caps[rule] = int(spec.get("baseline", 0)) + int(spec.get("slack", 0)) - return caps + return { + rule: int(spec.get("baseline", 0)) + int(spec.get("slack", 0)) + for rule, spec in budget.items() + if isinstance(spec, dict) + } + + +def _regression_detail( + rule: str, + base_caps: dict[str, int], + head_caps: dict[str, int], + base_baselines: dict[str, int], + head_baselines: dict[str, int], +) -> str | None: + """Why `rule` regressed vs base, or None when it held flat or fell. + + A dropped rule is terminal; otherwise a raised ceiling and a raised baseline are + independent loosenings (the latter catches a baseline bump masked by a slack cut), + so both reasons are reported when both apply. + """ + base_cap = base_caps[rule] + if rule not in head_caps: + return f"rule dropped (ceiling {base_cap} -> removed)" + reasons = tuple( + message + for raised, message in ( + ( + head_caps[rule] > base_cap, + f"ceiling raised {base_cap} -> {head_caps[rule]}", + ), + ( + head_baselines[rule] > base_baselines[rule], + f"baseline raised {base_baselines[rule]} -> {head_baselines[rule]}", + ), + ) + if raised + ) + return "; ".join(reasons) or None def regressions_for(rel: str, base: dict | None, head: dict | None) -> list[Regression]: @@ -96,18 +146,17 @@ def regressions_for(rel: str, base: dict | None, head: dict | None) -> list[Regr if head is None: return [Regression(rel, "*", "budget file was deleted (every ceiling removed)")] - base_caps = _caps(base) - head_caps = _caps(head) + base_caps, head_caps = _caps(base), _caps(head) + base_baselines, head_baselines = _baselines(base), _baselines(head) return [ - Regression( - rel, - rule, - f"rule dropped (ceiling {base_cap} -> removed)" - if rule not in head_caps - else f"ceiling raised {base_cap} -> {head_caps[rule]}", + Regression(rel, rule, detail) + for rule in sorted(base_caps) + if ( + detail := _regression_detail( + rule, base_caps, head_caps, base_baselines, head_baselines + ) ) - for rule, base_cap in sorted(base_caps.items()) - if rule not in head_caps or head_caps[rule] > base_cap + is not None ] @@ -141,7 +190,9 @@ def main() -> int: regressions.extend(regressions_for(rel, base, head)) if regressions: - print(f"FAIL: budget ceiling(s) loosened vs base {args.base} (merge-base {ref[:12]}):") + print( + f"FAIL: budget baseline(s)/ceiling(s) loosened vs base {args.base} (merge-base {ref[:12]}):" + ) for reg in regressions: print(f" {reg.budget} {reg.rule}: {reg.detail}") print( diff --git a/tests/test_litellm/test_budget_ratchet_check.py b/tests/test_litellm/test_budget_ratchet_check.py index 9f19944fdba..77cee8a485c 100644 --- a/tests/test_litellm/test_budget_ratchet_check.py +++ b/tests/test_litellm/test_budget_ratchet_check.py @@ -1,7 +1,8 @@ """Tests for scripts/budget_ratchet_check.py. -The guard's whole contract is "ceilings may only fall": a raised ceiling, a dropped -rule, or a deleted file is a regression, while a lowered/equal ceiling, a brand-new +The guard's contract is "baselines and ceilings may only fall": a raised ceiling, a +raised baseline (even when slack is cut to keep the ceiling flat), a dropped rule, or +a deleted file is a regression, while a lowered/equal baseline and ceiling, a brand-new rule, or a brand-new budget file is fine. Each branch is pinned here. """ @@ -10,7 +11,9 @@ import sys from pathlib import Path -_MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "budget_ratchet_check.py" +_MODULE_PATH = ( + Path(__file__).resolve().parents[2] / "scripts" / "budget_ratchet_check.py" +) _spec = importlib.util.spec_from_file_location("budget_ratchet_check", _MODULE_PATH) ratchet = importlib.util.module_from_spec(_spec) _spec.loader.exec_module(ratchet) @@ -35,10 +38,30 @@ def test_raised_ceiling_is_a_regression(): def test_lowered_or_equal_ceiling_is_clean(): base = {"LIT006": _spec_of(1013, 10)} + # baseline drops, slack flat -> ceiling falls assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1000, 10)}) == [] + # nothing changes assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1013, 10)}) == [] - # slack traded for baseline at the same ceiling is fine - assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1023, 0)}) == [] + # slack cut while baseline holds -> ceiling falls, baseline flat + assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1013, 0)}) == [] + + +def test_raised_baseline_is_a_regression_even_when_ceiling_held_flat(): + # baseline 1013 -> 1023 with slack cut 10 -> 0 keeps the ceiling at 1023, but a + # higher baseline bakes in more accepted debt and must still surface as a regression + base = {"LIT006": _spec_of(1013, 10)} + regs = ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1023, 0)}) + assert [r.rule for r in regs] == ["LIT006"] + assert "baseline raised 1013 -> 1023" in regs[0].detail + assert "ceiling raised" not in regs[0].detail + + +def test_raised_baseline_and_ceiling_report_both_reasons(): + base = {"LIT006": _spec_of(1013, 10)} + regs = ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1100, 10)}) + assert [r.rule for r in regs] == ["LIT006"] + assert "ceiling raised 1023 -> 1110" in regs[0].detail + assert "baseline raised 1013 -> 1100" in regs[0].detail def test_dropped_rule_is_a_regression():