diff --git a/scripts/ruff_strict_gate.py b/scripts/ruff_strict_gate.py index 25f6c4d29ba..ea8002da1ce 100644 --- a/scripts/ruff_strict_gate.py +++ b/scripts/ruff_strict_gate.py @@ -17,6 +17,7 @@ import subprocess import sys import tempfile from collections import Counter +from collections.abc import Callable, Mapping from pathlib import Path from typing import NamedTuple @@ -50,6 +51,10 @@ def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: return proc.stdout +def _merge_base(base: str) -> str: + return _run(["git", "merge-base", base, "HEAD"]).strip() or base + + def _ruff_json(cwd: Path, config: Path) -> list: raw = _run( ["ruff", "check", TARGET, "--config", str(config), "--output-format", "json"], @@ -101,6 +106,16 @@ def over_ceiling(head: dict, budget: dict) -> frozenset: ) +def is_vacuous_run( + counts: Mapping[str, int], budget: Mapping[str, Mapping[str, int]] +) -> bool: + """True when nothing was counted but the budget expects violations -- the + signature of a scan that crashed or whose output failed to parse. Without + this guard an empty head scan would clear every limit and pass silently, + and an empty base scan would make every head violation look freshly added.""" + return not counts and any(spec["limit"] for spec in budget.values()) + + def evaluate(head: dict, base: dict, budget: dict) -> list: breaches = [] for rule, spec in budget.items(): @@ -128,15 +143,37 @@ 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() +def cmd_check( + base: str, + violations: Callable[[], list] = head_violations, + base_counts_for: Callable[[str], dict] = base_counts, + merge_base: Callable[[str], str] = _merge_base, + budget_path: Path = BUDGET_PATH, +) -> None: + budget = json.loads(budget_path.read_text()) + head = violations() head_counts = count_by_rule(head) + if is_vacuous_run(head_counts, budget): + expected = sum(spec["limit"] for spec in budget.values()) + print( + f"FAIL: ruff reported no strict-rule violations, but {budget_path.name} " + f"allows up to ~{expected}. The scan almost certainly crashed or emitted " + f"nothing; refusing to certify a vacuous run." + ) + raise SystemExit(1) if not over_ceiling(head_counts, budget): print(f"OK: every strict rule is within its codebase ceiling (base {base})") return - base_point = _run(["git", "merge-base", base, "HEAD"]).strip() or base - breaches = evaluate(head_counts, base_counts(base_point), budget) + base_point = merge_base(base) + base_totals = base_counts_for(base_point) + if is_vacuous_run(base_totals, budget): + print( + f"FAIL: ruff reported no strict-rule violations for the base tree at " + f"{base_point[:12]}, so every rule would look freshly added. The base scan " + f"almost certainly crashed; refusing to blame this change for it." + ) + raise SystemExit(1) + breaches = evaluate(head_counts, base_totals, budget) if not breaches: print(f"OK: every strict rule is within its codebase ceiling (base {base})") return @@ -182,7 +219,7 @@ def cmd_update(base_ref: str = DEFAULT_BASE) -> None: fixes tighten its own ceilings by exactly what they cleared since it diverged. """ budget = json.loads(BUDGET_PATH.read_text()) - base_point = _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref + base_point = _merge_base(base_ref) updated = ratcheted_budget( budget, count_by_rule(head_violations()), base_counts(base_point) ) diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index 2c5306cec7d..7ea1c20318f 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -59,6 +59,8 @@ UNCODED = "" # fails once it clears this many errors. DEFAULT_LIMIT = 10 +BASE_PASS_ATTEMPTS = 2 + class Breach(NamedTuple): code: str @@ -107,6 +109,10 @@ def _run(cmd: list[str], cwd: Path = REPO_ROOT) -> str: return proc.stdout +def _merge_base(base_ref: str) -> str: + return _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref + + @contextlib.contextmanager def _temp_worktree(ref: str) -> Iterator[Path]: parent = Path(tempfile.mkdtemp(prefix="bpr_base_")) @@ -124,6 +130,24 @@ def _temp_worktree(ref: str) -> Iterator[Path]: shutil.rmtree(parent, ignore_errors=True) +def run_base_pass( + run: Callable[[], "subprocess.CompletedProcess[str]"], + attempts: int = BASE_PASS_ATTEMPTS, +) -> "subprocess.CompletedProcess[str]": + for attempt in range(1, attempts + 1): + proc = run() + if proc.returncode in (0, 1): + return proc + sys.stderr.write( + f"basedpyright base pass exited {proc.returncode} " + f"(attempt {attempt}/{attempts}); stderr tail:\n{proc.stderr[-2000:]}\n" + ) + raise SystemExit( + f"basedpyright base pass crashed {attempts} times; its exit code and " + f"stderr are above" + ) + + def base_counts(ref: str) -> dict[str, int]: """basedpyright error counts per rule for the merge-base tree. The head config is copied in so the base is judged by today's rules, and the run uses @@ -131,8 +155,10 @@ def base_counts(ref: str) -> dict[str, int]: exe = shutil.which("basedpyright") or "basedpyright" with _temp_worktree(ref) as worktree: shutil.copy(PYRIGHT_CONFIG, worktree / "pyrightconfig.json") - proc = subprocess.run( - [exe, "--outputjson"], cwd=worktree, capture_output=True, text=True + proc = run_base_pass( + lambda: subprocess.run( + [exe, "--outputjson"], cwd=worktree, capture_output=True, text=True + ) ) return count_basedpyright(proc.stdout, root=worktree) @@ -295,7 +321,7 @@ def cmd_update(current: Mapping[str, int], base_ref: str = DEFAULT_BASE) -> None by exactly what they cleared since it diverged, and limits never rise. """ budget = json.loads(BUDGET_PATH.read_text()) if BUDGET_PATH.exists() else {} - base_point = _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref + base_point = _merge_base(base_ref) updated = ratcheted_budget(budget, current, base_counts_cached(base_point)) BUDGET_PATH.write_text(json.dumps(updated, indent=2, sort_keys=True) + "\n") cleared = sum(budget[code]["limit"] - updated[code]["limit"] for code in updated) @@ -305,13 +331,19 @@ def cmd_update(current: Mapping[str, int], base_ref: str = DEFAULT_BASE) -> None ) -def cmd_check(base_ref: str) -> None: - budget = json.loads(BUDGET_PATH.read_text()) - head = count_basedpyright(sys.stdin.read()) +def cmd_check( + base_ref: str, + head_payload: Callable[[], str] = sys.stdin.read, + base_counts_for: Callable[[str], dict[str, int]] = base_counts_cached, + merge_base: Callable[[str], str] = _merge_base, + budget_path: Path = BUDGET_PATH, +) -> None: + budget = json.loads(budget_path.read_text()) + head = count_basedpyright(head_payload()) if is_vacuous_run(head, budget): expected = sum(spec["limit"] for spec in budget.values()) print( - f"FAIL: basedpyright produced no errors, but {BUDGET_PATH.name} allows " + f"FAIL: basedpyright produced no errors, but {budget_path.name} allows " f"up to ~{expected}. The type checker almost certainly crashed or emitted " f"nothing; refusing to certify a vacuous run." ) @@ -321,8 +353,8 @@ def cmd_check(base_ref: str) -> None: f"OK: every rule is within its basedpyright limit ({sum(head.values())} errors total)" ) return - base_point = _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref - base = base_counts_cached(base_point) + base_point = merge_base(base_ref) + base = base_counts_for(base_point) if is_vacuous_run(base, budget): print( f"FAIL: basedpyright produced no errors for the base tree at " diff --git a/scripts/type_discipline_gate.py b/scripts/type_discipline_gate.py index bd8d16553b1..2032d0ec735 100644 --- a/scripts/type_discipline_gate.py +++ b/scripts/type_discipline_gate.py @@ -27,6 +27,7 @@ import subprocess import sys import tempfile from collections import Counter +from collections.abc import Callable, Mapping from pathlib import Path from typing import NamedTuple @@ -61,6 +62,10 @@ def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: return proc.stdout +def _merge_base(base: str) -> str: + return _run(["git", "merge-base", base, "HEAD"]).strip() or base + + def _check(root: Path, checker: Path) -> list: # Resolve root first: on macOS tempfile dirs (/var/...) resolve to /private/var/..., # and the checker prints already-resolved absolute paths, so relative_to would fail. @@ -118,6 +123,17 @@ def over_ceiling(head: dict, budget: dict) -> frozenset: ) +def is_vacuous_run( + counts: Mapping[str, int], budget: Mapping[str, Mapping[str, int]] +) -> bool: + """True when nothing was counted but the budget expects violations -- the + signature of a checker pass that crashed or whose output failed to parse + (say, after an output-format change). Without this guard an empty head pass + would clear every limit and pass silently, and an empty base pass would make + every head violation look freshly added.""" + return not counts and any(spec["limit"] for spec in budget.values()) + + def evaluate(head: dict, base: dict, budget: dict) -> list: breaches = [] for rule, spec in budget.items(): @@ -145,15 +161,37 @@ 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() +def cmd_check( + base: str, + violations: Callable[[], list] = head_violations, + base_counts_for: Callable[[str], dict] = base_counts, + merge_base: Callable[[str], str] = _merge_base, + budget_path: Path = BUDGET_PATH, +) -> None: + budget = json.loads(budget_path.read_text()) + head = violations() head_counts = count_by_rule(head) + if is_vacuous_run(head_counts, budget): + expected = sum(spec["limit"] for spec in budget.values()) + print( + f"FAIL: the LIT checker reported no violations, but {budget_path.name} " + f"allows up to ~{expected}. The pass almost certainly crashed or its " + f"output failed to parse; refusing to certify a vacuous run." + ) + raise SystemExit(1) if not over_ceiling(head_counts, budget): print(f"OK: every LIT rule is within its codebase ceiling (base {base})") return - base_point = _run(["git", "merge-base", base, "HEAD"]).strip() or base - breaches = evaluate(head_counts, base_counts(base_point), budget) + base_point = merge_base(base) + base_totals = base_counts_for(base_point) + if is_vacuous_run(base_totals, budget): + print( + f"FAIL: the LIT checker reported no violations for the base tree at " + f"{base_point[:12]}, so every rule would look freshly added. The base " + f"pass almost certainly crashed; refusing to blame this change for it." + ) + raise SystemExit(1) + breaches = evaluate(head_counts, base_totals, budget) if not breaches: print(f"OK: every LIT rule is within its codebase ceiling (base {base})") return @@ -203,7 +241,7 @@ def cmd_update(base_ref: str = DEFAULT_BASE) -> None: fixes tighten its own ceilings by exactly what they cleared since it diverged. """ budget = json.loads(BUDGET_PATH.read_text()) - base_point = _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref + base_point = _merge_base(base_ref) updated = ratcheted_budget( budget, count_by_rule(head_violations()), base_counts(base_point) ) diff --git a/tests/test_litellm/test_ruff_strict_gate.py b/tests/test_litellm/test_ruff_strict_gate.py index aad0e1bc9f9..c95ac3b73d4 100644 --- a/tests/test_litellm/test_ruff_strict_gate.py +++ b/tests/test_litellm/test_ruff_strict_gate.py @@ -110,3 +110,73 @@ def test_over_ceiling_ignores_rules_missing_from_the_budget(): def test_over_ceiling_is_independent_across_rules(): budget = {**rule("ANN001", 150), **rule("C901", 10)} assert gate.over_ceiling({"ANN001": 130, "C901": 11}, budget) == frozenset({"C901"}) + + +def test_no_violations_against_a_nonempty_budget_is_vacuous(): + assert gate.is_vacuous_run({}, rule("ANN001", 110)) is True + + +def test_genuine_zero_counts_are_not_vacuous(): + assert gate.is_vacuous_run({}, {}) is False + assert gate.is_vacuous_run({}, rule("ANN001", 0)) is False + assert gate.is_vacuous_run({"ANN001": 1}, rule("ANN001", 110)) is False + + +def _violations(code, count): + return [Violation("litellm/a.py", line, code) for line in range(1, count + 1)] + + +def _raise_if_called(*args): + raise AssertionError("must not be called") + + +def _budget_file(tmp_path, limit): + path = tmp_path / "budget.json" + path.write_text(f'{{"ANN001": {{"limit": {limit}}}}}') + return path + + +def test_check_rejects_a_vacuous_head_scan(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + violations=lambda: [], + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 110), + ) + assert "vacuous" in capsys.readouterr().out + + +def test_check_within_ceiling_skips_the_base_scan(tmp_path, capsys): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("ANN001", 3), + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 110), + ) + assert capsys.readouterr().out.startswith("OK") + + +def test_check_refuses_to_blame_the_change_for_a_vacuous_base_scan(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("ANN001", 3), + base_counts_for=lambda ref: {}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 2), + ) + assert "refusing to blame" in capsys.readouterr().out + + +def test_check_spares_a_bystander_whose_base_matches_head(tmp_path, capsys): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("ANN001", 3), + base_counts_for=lambda ref: {"ANN001": 3}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 2), + ) + assert capsys.readouterr().out.startswith("OK") diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index 66a28360af9..2e0c9abb2d6 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -1,7 +1,10 @@ import importlib.util import json +import subprocess from pathlib import Path +import pytest + _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "type_check_gate.py" _spec = importlib.util.spec_from_file_location("type_check_gate", _MODULE_PATH) gate = importlib.util.module_from_spec(_spec) @@ -163,8 +166,6 @@ def test_update_clamps_a_limit_at_zero_never_negative(): def test_malformed_basedpyright_json_exits_loudly_not_as_zero_errors(): - import pytest - with pytest.raises(SystemExit): gate.count_basedpyright("startup warning\n{not json") @@ -287,3 +288,121 @@ def test_an_empty_base_pass_is_never_cached(tmp_path): assert gate.base_counts_cached("abc123", cache_dir=tmp_path, compute=crashed) == {} assert calls == ["abc123", "abc123"] assert list(tmp_path.iterdir()) == [] + + +def _proc(returncode, stdout="", stderr=""): + return subprocess.CompletedProcess(["basedpyright"], returncode, stdout, stderr) + + +@pytest.mark.parametrize("code", [0, 1]) +def test_base_pass_accepts_clean_and_errorful_exit_codes_without_retry(code): + calls = [] + + def run(): + calls.append(code) + return _proc(code, stdout="{}") + + assert gate.run_base_pass(run).returncode == code + assert calls == [code] + + +@pytest.mark.parametrize("code", [2, 137, -9]) +def test_base_pass_retries_after_a_crash_and_reports_the_evidence(code, capsys): + procs = iter([_proc(code, stderr="node blew up"), _proc(0, stdout="{}")]) + assert gate.run_base_pass(lambda: next(procs)).returncode == 0 + err = capsys.readouterr().err + assert f"exited {code}" in err + assert "node blew up" in err + + +def test_base_pass_that_keeps_crashing_exits_loudly_not_as_zero_counts(capsys): + attempts = [] + + def crash(): + attempts.append(1) + return _proc(134, stderr="JavaScript heap out of memory") + + with pytest.raises(SystemExit): + gate.run_base_pass(crash) + assert len(attempts) == gate.BASE_PASS_ATTEMPTS + err = capsys.readouterr().err + assert err.count("exited 134") == gate.BASE_PASS_ATTEMPTS + assert "JavaScript heap out of memory" in err + + +def _payload(rule, count): + return json.dumps( + { + "generalDiagnostics": [ + _bpr(f"{ROOT}/litellm/x.py", "error", rule) for _ in range(count) + ] + } + ) + + +def _raise_if_called(*args): + raise AssertionError("must not be called") + + +def _budget_file(tmp_path, limit): + path = tmp_path / "budget.json" + path.write_text(json.dumps({"reportAny": {"limit": limit}})) + return path + + +def test_check_rejects_a_vacuous_head_run_before_touching_the_base(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + head_payload=lambda: "", + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 5), + ) + assert "vacuous" in capsys.readouterr().out + + +def test_check_within_limits_passes_without_a_base_pass(tmp_path, capsys): + gate.cmd_check( + "origin/main", + head_payload=lambda: _payload("reportAny", 3), + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 5), + ) + assert capsys.readouterr().out.startswith("OK") + + +def test_check_refuses_to_blame_the_change_for_a_vacuous_base_pass(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + head_payload=lambda: _payload("reportAny", 6), + base_counts_for=lambda ref: {}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 5), + ) + assert "refusing to blame" in capsys.readouterr().out + + +def test_check_spares_a_bystander_whose_base_matches_head(tmp_path, capsys): + gate.cmd_check( + "origin/main", + head_payload=lambda: _payload("reportAny", 6), + base_counts_for=lambda ref: {"reportAny": 6}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 5), + ) + assert capsys.readouterr().out.startswith("OK") + + +def test_check_fails_a_change_that_grew_a_rule_past_its_limit(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + head_payload=lambda: _payload("reportAny", 6), + base_counts_for=lambda ref: {"reportAny": 4}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 5), + ) + assert "BREACHED RULES" in capsys.readouterr().out diff --git a/tests/test_litellm/test_type_discipline_gate.py b/tests/test_litellm/test_type_discipline_gate.py index 8424d480fa6..57a4c8e1117 100644 --- a/tests/test_litellm/test_type_discipline_gate.py +++ b/tests/test_litellm/test_type_discipline_gate.py @@ -8,6 +8,8 @@ drift-safe breach check). Both are pinned here. import importlib.util from pathlib import Path +import pytest + _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "type_discipline_gate.py" _spec = importlib.util.spec_from_file_location("type_discipline_gate", _MODULE_PATH) gate = importlib.util.module_from_spec(_spec) @@ -50,3 +52,73 @@ def test_update_ratchets_limit_down_by_what_the_branch_fixed_never_up(): "LIT001": {"limit": 85}, "LIT006": {"limit": 10}, } + + +def test_no_violations_against_a_nonempty_budget_is_vacuous(): + assert gate.is_vacuous_run({}, _budget(12)) is True + + +def test_genuine_zero_counts_are_not_vacuous(): + assert gate.is_vacuous_run({}, {}) is False + assert gate.is_vacuous_run({}, _budget(0)) is False + assert gate.is_vacuous_run({"LIT006": 1}, _budget(12)) is False + + +def _violations(code, count): + return [gate.Violation("litellm/a.py", line, code) for line in range(1, count + 1)] + + +def _raise_if_called(*args): + raise AssertionError("must not be called") + + +def _budget_file(tmp_path, limit): + path = tmp_path / "budget.json" + path.write_text(f'{{"LIT006": {{"limit": {limit}}}}}') + return path + + +def test_check_rejects_a_vacuous_head_pass(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + violations=lambda: [], + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 12), + ) + assert "vacuous" in capsys.readouterr().out + + +def test_check_within_ceiling_skips_the_base_pass(tmp_path, capsys): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("LIT006", 3), + base_counts_for=_raise_if_called, + merge_base=_raise_if_called, + budget_path=_budget_file(tmp_path, 12), + ) + assert capsys.readouterr().out.startswith("OK") + + +def test_check_refuses_to_blame_the_change_for_a_vacuous_base_pass(tmp_path, capsys): + with pytest.raises(SystemExit): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("LIT006", 3), + base_counts_for=lambda ref: {}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 2), + ) + assert "refusing to blame" in capsys.readouterr().out + + +def test_check_spares_a_bystander_whose_base_matches_head(tmp_path, capsys): + gate.cmd_check( + "origin/main", + violations=lambda: _violations("LIT006", 3), + base_counts_for=lambda ref: {"LIT006": 3}, + merge_base=lambda base: "a" * 40, + budget_path=_budget_file(tmp_path, 2), + ) + assert capsys.readouterr().out.startswith("OK")