From f26dbb60be67060bbcf09b265e5187e09e8037a6 Mon Sep 17 00:00:00 2001 From: Mateo Wang <277851410+mateo-berri@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:10:05 -0700 Subject: [PATCH] ci: make the basedpyright budget gate delta-vs-base (#31106) * ci: re-run absolute basedpyright budget gate on push to long-lived branches The basedpyright budget gate counts codebase-wide errors per rule against a committed ceiling, but it only ran on pull_request against each PR's own head. Two PRs that each pass in isolation can together push a per-rule count over its ceiling once both merge, and nothing re-evaluated the budget on the merge commit, so the breach only surfaced on the next PR that happened to be checked out after the count crossed the line. Add a push trigger on the long-lived branches and a post-merge-budget job that re-runs the absolute gate on the merged tree, catching the accumulation on the merge commit itself. The existing pull_request jobs are guarded so their delta-vs-base gates don't misfire on push, where no PR base SHA exists. * ci: shallow-fetch the post-merge-budget checkout The post-merge-budget job only runs basedpyright over the working tree and the committed budget file; it never inspects git history, unlike the lint job whose delta-vs-base gates need full history. Drop its checkout from fetch-depth: 0 to fetch-depth: 1 to avoid cloning the whole repo history. * ci: scope post-merge-budget push trigger to long-lived branches On a push event the branches filter matches the branch being pushed to, not the PR target. The litellm_** glob, correct for the pull_request filter where it matches the target branch, therefore fired the post-merge-budget basedpyright job on every short-lived feature branch carrying the litellm_ prefix (litellm_dev_*, litellm_add_*, and so on), duplicating the PR lint job and burning ~10 minutes of CI per push. Restrict the push trigger to the long-lived branches PRs actually merge into (main, litellm_internal_staging, litellm_oss_branch), where budget accumulation happens. The pull_request filter keeps litellm_** so PRs targeting any long-lived branch are still linted. * ci: make the basedpyright budget gate delta-vs-base The basedpyright gate counted absolute codebase-wide errors per rule against a committed ceiling and ran only on each PR's own head. Two PRs that each pass in isolation could together push a rule past its ceiling once both merged, and because the gate had no comparison against the base, the next unrelated PR branched off the now-over-ceiling tree inherited a red it did nothing to cause. Give it the same shape as the ruff strict gate: a rule fails only when its total is both over the ceiling and higher than the count on the merge-base it merges into. Drift already in the base is never blamed on a bystander, while any change that actually grows a rule past the cap still fails. Head counts come from the existing stdin pipe; the base count is a second basedpyright pass over a detached worktree at the merge-base, reusing the head environment so import resolution matches and no second uv sync is needed. This obsoletes the push-triggered post-merge-budget job (and its event guards), which only detected accumulation after the fact; the delta check blocks it on the PR instead. Slack for reportReturnType and reportUnnecessaryComparison is raised to give real headroom under the cap. * refactor(ci): give the base ref its own name in type_check_gate cmd_check cmd_check took a parameter named base that held a git ref string, then rebound the same name to the dict of base-tree error counts returned by base_counts. Rename the parameter to base_ref so the ref and the counts each keep a single name and type, matching the no-reassignment style used elsewhere; behavior is unchanged. --------- Co-authored-by: Claude --- .github/workflows/test-linting.yml | 8 +- Makefile | 3 +- basedpyright-code-budget.json | 4 +- scripts/type_check_gate.py | 137 +++++++++++++++------ tests/test_litellm/test_type_check_gate.py | 45 +++++-- 5 files changed, 148 insertions(+), 49 deletions(-) diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index de7e1b68346..950d6ca31a6 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -14,7 +14,7 @@ permissions: jobs: lint: runs-on: ubuntu-latest - timeout-minutes: 10 + timeout-minutes: 15 steps: - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 @@ -87,9 +87,11 @@ jobs: run: | uv run --no-sync python -c "import openai; print(f'OpenAI version: {openai.__version__}')" - - name: Run basedpyright type checking + - name: Check basedpyright budget (delta vs base) + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - (uv run --no-sync basedpyright --outputjson || true) | uv run --no-sync python scripts/type_check_gate.py + (uv run --no-sync basedpyright --outputjson || true) | uv run --no-sync python scripts/type_check_gate.py --base "$BASE_SHA" - name: Check for circular imports run: | diff --git a/Makefile b/Makefile index 27150aec938..076eac0f4a7 100644 --- a/Makefile +++ b/Makefile @@ -125,7 +125,8 @@ lint-ruff-FULL-dev: install-dev else echo "No changed .py files to check."; fi lint-basedpyright: install-dev - ($(UV_RUN) basedpyright --outputjson || true) | $(UV_RUN) python scripts/type_check_gate.py + git fetch origin litellm_internal_staging + ($(UV_RUN) basedpyright --outputjson || true) | $(UV_RUN) python scripts/type_check_gate.py --base origin/litellm_internal_staging lint-basedpyright-budget-update: install-dev ($(UV_RUN) basedpyright --outputjson || true) | $(UV_RUN) python scripts/type_check_gate.py --update diff --git a/basedpyright-code-budget.json b/basedpyright-code-budget.json index 7ba7656e407..f5b0a9aaf81 100644 --- a/basedpyright-code-budget.json +++ b/basedpyright-code-budget.json @@ -121,7 +121,7 @@ }, "reportReturnType": { "baseline": 126, - "slack": 13 + "slack": 100 }, "reportTypedDictNotRequiredAccess": { "baseline": 20, @@ -157,7 +157,7 @@ }, "reportUnnecessaryComparison": { "baseline": 683, - "slack": 10 + "slack": 100 }, "reportUnnecessaryContains": { "baseline": 4, diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index 0f9a44703f9..2ef332d91ea 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -1,21 +1,22 @@ #!/usr/bin/env python3 -"""Per-rule count gate for basedpyright. +"""Delta-vs-base per-rule gate for basedpyright. basedpyright's ``--outputjson`` is reduced to a count of errors per *rule* (``reportAny``, ``reportArgumentType``, ...) and checked against a committed budget of the form ``{rule: {baseline, slack}}``, the same shape as -``ruff-strict-budget.json``. A rule fails when its codebase-wide total exceeds -``baseline + slack``. Counts ignore file, line, and column, so a violation -moving anywhere in the tree is invisible; only the per-rule total moves the -needle. +``ruff-strict-budget.json``. A rule fails only when its codebase-wide total is +both over its ceiling (``baseline + slack``) *and* higher than the count on the +base it merges into, so a change is blamed for the errors it adds, never for +drift that already sits in the base. That ``> base`` guard is what stops an +unrelated PR from inheriting a red once two PRs each land near the ceiling and +their sum crosses it: the bystander's count equals its base, so it is spared, +while any PR that actually grows the rule past the cap still fails. -Unlike ``ruff_strict_gate.py`` this does *not* re-run the tool on the merge base -to compute a delta: a second basedpyright pass is minutes and gigabytes, whereas -ruff is milliseconds. The committed budget is the baseline instead -- exactly -how the previous per-file gate worked -- so keep it fresh with ``--update`` -(ratchet), which re-captures every rule's count from the current tree while -preserving each rule's slack. Tool output is read from stdin, so the caller -decides how to invoke basedpyright (and from which cwd). +Head counts are read from stdin (the caller runs basedpyright once and pipes +``--outputjson`` in); the base count is a second basedpyright pass over a +detached worktree at the merge-base, run under the same environment so import +resolution matches. ``--update`` re-captures the absolute per-rule baselines for +the ratchet, preserving each rule's slack. ``--outputjson`` is used rather than text diagnostics because the latter wrap across lines, leaving the ``(reportRule)`` on a continuation line away from the @@ -24,13 +25,21 @@ carries an unambiguous ``rule`` field. """ import argparse +import contextlib import json +import shutil +import subprocess import sys +import tempfile from collections import Counter +from collections.abc import Iterator, Mapping from pathlib import Path -from typing import Mapping, NamedTuple +from typing import NamedTuple REPO_ROOT = Path(__file__).resolve().parent.parent +BUDGET_PATH = REPO_ROOT / "basedpyright-code-budget.json" +PYRIGHT_CONFIG = REPO_ROOT / "pyrightconfig.json" +DEFAULT_BASE = "origin/litellm_internal_staging" # Bucket for a basedpyright diagnostic with no `rule`. Counted so it's gated. UNCODED = "" @@ -45,6 +54,7 @@ class Breach(NamedTuple): code: str total: int cap: int + added: int def _seed_slack(baseline: int) -> int: @@ -54,18 +64,19 @@ def _seed_slack(baseline: int) -> int: return 10 if baseline >= 50 else 3 -def _to_repo_relative(raw: str) -> str | None: +def _to_relative(raw: str, root: Path) -> str | None: path = Path(raw) - absolute = path if path.is_absolute() else Path.cwd() / path + absolute = path if path.is_absolute() else root / path try: - return absolute.resolve().relative_to(REPO_ROOT).as_posix() + return absolute.resolve().relative_to(root).as_posix() except ValueError: return None -def count_basedpyright(payload: str) -> dict[str, int]: - """Count in-repo basedpyright errors per rule from `--outputjson`. Warnings - and information are ignored; only `severity == "error"` is gated.""" +def count_basedpyright(payload: str, root: Path = REPO_ROOT) -> dict[str, int]: + """Count in-tree basedpyright errors per rule from `--outputjson`. Warnings + and information are ignored; only `severity == "error"` is gated. Files + outside `root` (the venv's site-packages, say) are dropped.""" try: data = json.loads(payload or "{}") except json.JSONDecodeError as exc: @@ -79,21 +90,62 @@ def count_basedpyright(payload: str) -> dict[str, int]: for diag in data.get("generalDiagnostics", []): if diag.get("severity") != "error": continue - if _to_repo_relative(diag.get("file", "")) is None: + if _to_relative(diag.get("file", ""), root) is None: continue counts[diag.get("rule") or UNCODED] += 1 return dict(counts) +def _run(cmd: list[str], 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 + + +@contextlib.contextmanager +def _temp_worktree(ref: str) -> Iterator[Path]: + parent = Path(tempfile.mkdtemp(prefix="bpr_base_")) + worktree = parent / "wt" + try: + _run(["git", "worktree", "add", "--detach", str(worktree), ref]) + yield worktree + finally: + subprocess.run( + ["git", "worktree", "remove", "--force", str(worktree)], + cwd=REPO_ROOT, + capture_output=True, + text=True, + ) + shutil.rmtree(parent, ignore_errors=True) + + +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 + the head environment's basedpyright (on PATH) so imports resolve the same.""" + 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 + ) + return count_basedpyright(proc.stdout, root=worktree) + + def evaluate( - counts: Mapping[str, int], budget: Mapping[str, Mapping[str, int]] + head: Mapping[str, int], + base: Mapping[str, int], + budget: Mapping[str, Mapping[str, int]], ) -> list[Breach]: breaches = [] - for code, total in counts.items(): + for code, total in head.items(): spec = budget.get(code) cap = spec["baseline"] + spec["slack"] if spec else DEFAULT_SLACK - if total > cap: - breaches.append(Breach(code, total, cap)) + prior = base.get(code, 0) + if total > cap and total > prior: + breaches.append(Breach(code, total, cap, total - prior)) return sorted(breaches) @@ -107,9 +159,6 @@ def is_vacuous_run( return not counts and any(spec["baseline"] for spec in budget.values()) -BUDGET_PATH = REPO_ROOT / "basedpyright-code-budget.json" - - def cmd_update(counts: Mapping[str, int]) -> None: existing = json.loads(BUDGET_PATH.read_text()) if BUDGET_PATH.exists() else {} budget = { @@ -127,9 +176,10 @@ def cmd_update(counts: Mapping[str, int]) -> None: ) -def cmd_check(counts: Mapping[str, int]) -> None: +def cmd_check(base_ref: str) -> None: budget = json.loads(BUDGET_PATH.read_text()) - if is_vacuous_run(counts, budget): + head = count_basedpyright(sys.stdin.read()) + if is_vacuous_run(head, budget): expected = sum(spec["baseline"] for spec in budget.values()) print( f"FAIL: basedpyright produced no errors, but {BUDGET_PATH.name} expects " @@ -137,27 +187,44 @@ def cmd_check(counts: Mapping[str, int]) -> None: f"nothing; refusing to certify a vacuous run." ) raise SystemExit(1) - breaches = evaluate(counts, budget) + base_point = _run(["git", "merge-base", base_ref, "HEAD"]).strip() or base_ref + base = base_counts(base_point) + if is_vacuous_run(base, budget): + print( + f"FAIL: basedpyright produced no errors 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, base, budget) if not breaches: print( - f"OK: every rule is within its basedpyright ceiling ({sum(counts.values())} errors total)" + f"OK: every rule is within its basedpyright ceiling or no higher than base ({sum(head.values())} errors total)" ) return print("FAIL: basedpyright errors exceed the per-rule ceiling:") for breach in breaches: - print(f" {breach.code}: {breach.total} errors over cap {breach.cap}") + print( + f" {breach.code}: total {breach.total} over cap {breach.cap} (this change added {breach.added})" + ) print( - "Resolve the new errors, or run 'make lint-basedpyright-budget-update' if the ceiling should move." + "Reduce the new errors or remove an equal number elsewhere; the ceiling is " + "baseline + slack in basedpyright-code-budget.json." ) + summary = "; ".join(f"{b.code} {b.total}/{b.cap} (+{b.added})" for b in breaches) + print(f"BREACHED RULES: {summary}") raise SystemExit(1) 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() - counts = count_basedpyright(sys.stdin.read()) - cmd_update(counts) if args.update else cmd_check(counts) + if args.update: + cmd_update(count_basedpyright(sys.stdin.read())) + else: + cmd_check(args.base) if __name__ == "__main__": diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index 18374c5db4b..e99ad0a4f41 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -56,29 +56,58 @@ def test_paths_outside_repo_are_skipped(): def test_at_or_under_ceiling_passes(): budget = {"no-any-return": {"baseline": 5, "slack": 0}} - assert gate.evaluate({"no-any-return": 5}, budget) == [] + assert gate.evaluate({"no-any-return": 5}, {}, budget) == [] def test_one_more_error_than_ceiling_fails(): budget = {"no-any-return": {"baseline": 5, "slack": 0}} - assert gate.evaluate({"no-any-return": 6}, budget) == [ - gate.Breach("no-any-return", 6, 5) + assert gate.evaluate({"no-any-return": 6}, {}, budget) == [ + gate.Breach("no-any-return", 6, 5, 6) ] def test_slack_absorbs_small_increase_then_fails_past_it(): budget = {"arg-type": {"baseline": 5, "slack": 5}} - assert gate.evaluate({"arg-type": 10}, budget) == [] - assert gate.evaluate({"arg-type": 11}, budget) == [gate.Breach("arg-type", 11, 10)] + assert gate.evaluate({"arg-type": 10}, {}, budget) == [] + assert gate.evaluate({"arg-type": 11}, {}, budget) == [ + gate.Breach("arg-type", 11, 10, 11) + ] def test_unbudgeted_new_code_uses_default_slack(): - assert gate.evaluate({"brand-new": gate.DEFAULT_SLACK}, {}) == [] - assert gate.evaluate({"brand-new": gate.DEFAULT_SLACK + 1}, {}) == [ - gate.Breach("brand-new", gate.DEFAULT_SLACK + 1, gate.DEFAULT_SLACK) + assert gate.evaluate({"brand-new": gate.DEFAULT_SLACK}, {}, {}) == [] + assert gate.evaluate({"brand-new": gate.DEFAULT_SLACK + 1}, {}, {}) == [ + gate.Breach( + "brand-new", + gate.DEFAULT_SLACK + 1, + gate.DEFAULT_SLACK, + gate.DEFAULT_SLACK + 1, + ) ] +def test_drift_already_over_cap_in_base_is_not_blamed_on_a_flat_change(): + # The bystander case: a rule sits over its ceiling because two earlier PRs + # summed past it. A PR that branches off that base and adds nothing must pass + # -- total > cap but total == base, so the `> base` guard spares it. + budget = {"arg-type": {"baseline": 5, "slack": 5}} + assert gate.evaluate({"arg-type": 12}, {"arg-type": 12}, budget) == [] + + +def test_change_that_grows_an_over_cap_rule_is_blamed_for_only_what_it_added(): + # Over cap AND above base: blamed, and `added` is the delta vs base, not the + # whole overage, so the message points at this change's contribution. + budget = {"arg-type": {"baseline": 5, "slack": 5}} + assert gate.evaluate({"arg-type": 14}, {"arg-type": 12}, budget) == [ + gate.Breach("arg-type", 14, 10, 2) + ] + + +def test_reducing_an_over_cap_rule_below_base_passes(): + budget = {"arg-type": {"baseline": 5, "slack": 5}} + assert gate.evaluate({"arg-type": 11}, {"arg-type": 12}, budget) == [] + + def test_no_output_against_a_nonempty_budget_is_a_vacuous_run(): # A crashed type checker emits nothing; the gate must not certify it as clean. budget = {"no-untyped-def": {"baseline": 4888, "slack": 10}}