From 22a1c3060391a60e7456eaf746c655ff5002e74d Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:59:56 -0700 Subject: [PATCH] fix(lint): move the basedpyright heap flag into the type check gate The 12 GB NODE_OPTIONS setting lived only in the Makefile export and the CI env line, so any hand-run gate pipeline forgot it and node OOMed at the ~4 GB default after 80 seconds, with || true feeding the gate empty output. The gate now spawns basedpyright itself for both the head and base passes, appends the heap flag last so it wins node's last-flag-wins resolution while preserving other caller flags, and fails loudly on crash exit codes instead of reading them as zero errors. --- .github/workflows/test-linting.yml | 3 +- Makefile | 6 +- scripts/type_check_gate.py | 67 ++++++++++++++++------ tests/test_litellm/test_type_check_gate.py | 43 ++++++++++++++ 4 files changed, 94 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index 8d2b2c2f972..b539ec4be88 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -104,9 +104,8 @@ jobs: - name: Check basedpyright budget (delta vs base) env: BASE_SHA: ${{ github.event.pull_request.base.sha }} - NODE_OPTIONS: --max-old-space-size=12288 run: | - (uv run --no-sync basedpyright --outputjson || true) | uv run --no-sync python scripts/type_check_gate.py --base "$BASE_SHA" + uv run --no-sync python scripts/type_check_gate.py --base "$BASE_SHA" - name: Check tests/e2e basedpyright (zero errors) env: diff --git a/Makefile b/Makefile index f4494680e13..0b59b2f3e95 100644 --- a/Makefile +++ b/Makefile @@ -176,10 +176,8 @@ lint-ruff-FULL-dev: install-dev if [ -n "$$files" ]; then echo "$$files" | xargs $(UV_RUN) ruff check; \ else echo "No changed .py files to check."; fi -lint-basedpyright lint-basedpyright-budget-update: export NODE_OPTIONS := --max-old-space-size=12288 - lint-basedpyright: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) - ($(UV_RUN) basedpyright --outputjson || true) | $(UV_RUN) python scripts/type_check_gate.py --base origin/litellm_internal_staging + $(UV_RUN) python scripts/type_check_gate.py --base origin/litellm_internal_staging lint-e2e-basedpyright: $(LINT_E2E_DEP_INSTALL) $(UV_RUN) basedpyright tests/e2e @@ -192,7 +190,7 @@ lint-type-discipline: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) # --update lowers each limit by what this branch fixed since its branch point, so # it needs the base ref fetched to resolve the merge-base. lint-basedpyright-budget-update: install-dev lint-fetch-base - ($(UV_RUN) basedpyright --outputjson || true) | $(UV_RUN) python scripts/type_check_gate.py --update + $(UV_RUN) python scripts/type_check_gate.py --update lint-format: format-check diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index 2c5306cec7d..1bce746b5e2 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -12,12 +12,16 @@ a red once two PRs each land near the limit 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 its limit still fails. -Head counts are read from stdin (the caller runs basedpyright once and pipes -``--outputjson`` in). The base count only matters once some rule is over its -limit, so when none is the base pass is skipped outright. When it is needed, it -is a second basedpyright pass over a detached worktree at the merge-base, run -under the same environment so import resolution matches, and its per-rule -counts are cached under the repo's git common dir keyed by merge-base commit, +The gate runs basedpyright itself, for both the head and the base pass, with +``NODE_OPTIONS`` raised to the heap this repo needs: basedpyright's node +process OOMs at the ~4 GB default, and when callers had to remember the flag, +every hand-copied pipeline (Makefile, CI, a dev running the recipe by hand) +was one forgotten env line away from an 80-second crash. The base count only +matters once some rule is over its limit, so when none is the base pass is +skipped outright. When it is needed, it is a second basedpyright pass over a +detached worktree at the merge-base, run under the same environment so import +resolution matches, and its per-rule counts are cached under the repo's git +common dir keyed by merge-base commit, ``pyrightconfig.json``, and ``uv.lock``, so re-runs against the same branch point pay for it once. ``--update`` ratchets each rule's ``limit`` down by the number of errors this branch fixed relative to its branch point (the merge-base), @@ -51,6 +55,11 @@ UV_LOCK = REPO_ROOT / "uv.lock" DEFAULT_BASE = "origin/litellm_internal_staging" CACHE_FILE_PREFIX = "basedpyright-base-" +# basedpyright's node process needs more than the ~4 GB default heap on this +# repo; appended last so it wins node's last-flag-wins resolution over any +# caller-set value while preserving the caller's other NODE_OPTIONS flags. +NODE_HEAP_OPTION = "--max-old-space-size=12288" + # Bucket for a basedpyright diagnostic with no `rule`. Counted so it's gated. UNCODED = "" @@ -107,6 +116,29 @@ def _run(cmd: list[str], cwd: Path = REPO_ROOT) -> str: return proc.stdout +def node_options_with_heap(base_env: Mapping[str, str]) -> str: + return f"{base_env.get('NODE_OPTIONS', '')} {NODE_HEAP_OPTION}".strip() + + +def run_basedpyright(cwd: Path = REPO_ROOT) -> str: + """One basedpyright pass over `cwd` with the raised node heap exported. + + Exit 0 (clean) and 1 (errors found) are both output-bearing runs; anything + else is a crash and fails loudly instead of reading as zero errors.""" + exe = shutil.which("basedpyright") or "basedpyright" + proc = subprocess.run( + [exe, "--outputjson"], + cwd=cwd, + capture_output=True, + text=True, + env={**os.environ, "NODE_OPTIONS": node_options_with_heap(os.environ)}, + ) + if proc.returncode not in (0, 1): + sys.stderr.write(proc.stderr) + raise SystemExit(f"basedpyright exited {proc.returncode}") + return proc.stdout + + @contextlib.contextmanager def _temp_worktree(ref: str) -> Iterator[Path]: parent = Path(tempfile.mkdtemp(prefix="bpr_base_")) @@ -128,13 +160,9 @@ 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) + return count_basedpyright(run_basedpyright(worktree), root=worktree) def over_ceiling( @@ -259,9 +287,10 @@ def is_vacuous_run( counts: Mapping[str, int], budget: Mapping[str, Mapping[str, int]] ) -> bool: """True when nothing was parsed but the budget expects errors -- the - signature of a type checker that crashed or produced no output. The CI pipe - swallows the tool's exit code (`tool || true`), so without this guard an - empty run would clear every limit and pass silently.""" + signature of a type checker that produced no output. `run_basedpyright` + already fails crash exit codes, so this guards the remaining case: a run + that exits cleanly while emitting nothing, which would otherwise clear + every limit and pass silently.""" return not counts and any(spec["limit"] for spec in budget.values()) @@ -289,7 +318,7 @@ def ratcheted_budget( def cmd_update(current: Mapping[str, int], base_ref: str = DEFAULT_BASE) -> None: """Ratchet each rule's limit down by the errors this branch fixed. - `current` is the working-tree count (piped in); the reference count comes + `current` is the working-tree count; the reference count comes from a second basedpyright pass over a detached worktree at the branch point (the merge-base with `base_ref`), so a branch's fixes tighten its own ceilings by exactly what they cleared since it diverged, and limits never rise. @@ -305,9 +334,8 @@ def cmd_update(current: Mapping[str, int], base_ref: str = DEFAULT_BASE) -> None ) -def cmd_check(base_ref: str) -> None: +def cmd_check(head: Mapping[str, int], base_ref: str) -> None: budget = json.loads(BUDGET_PATH.read_text()) - head = count_basedpyright(sys.stdin.read()) if is_vacuous_run(head, budget): expected = sum(spec["limit"] for spec in budget.values()) print( @@ -355,10 +383,11 @@ def main() -> None: parser.add_argument("--base", default=DEFAULT_BASE) parser.add_argument("--update", action="store_true") args = parser.parse_args() + head = count_basedpyright(run_basedpyright()) if args.update: - cmd_update(count_basedpyright(sys.stdin.read()), args.base) + cmd_update(head, args.base) else: - cmd_check(args.base) + cmd_check(head, 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 66a28360af9..08813d5b0b0 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -1,5 +1,6 @@ import importlib.util import json +import os from pathlib import Path _MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "type_check_gate.py" @@ -69,6 +70,48 @@ def test_symlinked_root_keeps_diagnostics_in_tree(tmp_path): assert gate.count_basedpyright(payload, root=link) == {"reportArgumentType": 1} +def test_node_options_with_heap_sets_the_flag_in_a_bare_env(): + assert gate.node_options_with_heap({}) == gate.NODE_HEAP_OPTION + + +def test_node_options_with_heap_appends_after_caller_flags_so_it_wins(): + # node resolves a repeated --max-old-space-size last-wins, so ours must come + # after any caller-set value while keeping their other flags. + merged = gate.node_options_with_heap( + {"NODE_OPTIONS": "--max-old-space-size=4096 --no-warnings"} + ) + assert merged == f"--max-old-space-size=4096 --no-warnings {gate.NODE_HEAP_OPTION}" + + +def _stub_basedpyright(tmp_path, monkeypatch, script_body): + stub = tmp_path / "basedpyright" + stub.write_text(f"#!/bin/sh\n{script_body}\n") + stub.chmod(0o755) + monkeypatch.setenv("PATH", str(tmp_path), prepend=os.pathsep) + + +def test_run_basedpyright_exports_the_raised_heap_to_the_child(tmp_path, monkeypatch): + captured = tmp_path / "node_options.txt" + _stub_basedpyright( + tmp_path, + monkeypatch, + f'echo "$NODE_OPTIONS" > "{captured}"\necho \'{{"generalDiagnostics": []}}\'', + ) + monkeypatch.delenv("NODE_OPTIONS", raising=False) + assert json.loads(gate.run_basedpyright(cwd=tmp_path)) == {"generalDiagnostics": []} + assert captured.read_text().strip() == gate.NODE_HEAP_OPTION + + +def test_run_basedpyright_fails_loudly_on_a_crash_exit_code(tmp_path, monkeypatch): + import pytest + + # 134 is SIGABRT, what node dies with on a heap OOM; it must never read as a + # clean zero-error run. + _stub_basedpyright(tmp_path, monkeypatch, "exit 134") + with pytest.raises(SystemExit): + gate.run_basedpyright(cwd=tmp_path) + + def test_at_or_under_ceiling_passes(): budget = {"no-any-return": {"limit": 5}} assert gate.evaluate({"no-any-return": 5}, {}, budget) == []