From 7064b2bf094c8ff9012d6b9a679c7ff3390974ba Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:57:01 -0700 Subject: [PATCH] fix(ci): run the risk gate on pull_request and follow the one-pager rubric The workflow now triggers on pull_request instead of pull_request_target, which the auto-approve one-pager and the 2026-06-18 CI exposure post-mortem both forbid, and the job skips fork PRs since the read-only token cannot label them. The floor now follows the rubric's author row (Devin-opened low, human-opened medium, fork high) through a new authors.low list, and adds a paths.medium list for agent instruction files, conftest, helm, and docker. Two guards compare both sides of a file through git show: model map rows that change or disappear stop being low, and a budget file that raises a limit stops being low. A skip or importorskip call added to an existing test file with no net new test scores high. The last 400 staging merges score 218 high, 179 medium, 3 low; the author row alone moves 37 human-opened docs and test PRs from low to medium --- .github/risk-tiers.yml | 18 +++ .github/scripts/risk_tier.py | 150 ++++++++++++++++++-- .github/workflows/risk-gate.yml | 13 +- tests/test_litellm/test_github_risk_tier.py | 130 ++++++++++++++++- 4 files changed, 291 insertions(+), 20 deletions(-) diff --git a/.github/risk-tiers.yml b/.github/risk-tiers.yml index 63d61a69f22..512fa9d6761 100644 --- a/.github/risk-tiers.yml +++ b/.github/risk-tiers.yml @@ -35,6 +35,13 @@ paths: - "**/package*.json" - "**/Dockerfile*" - "scripts/*gate*" + medium: + - "**/CLAUDE.md" + - "**/AGENTS.md" + - "**/GEMINI.md" + - "**/conftest.py" + - "helm/**" + - "docker/**" low: - "cookbook/**" - "model_prices_and_context_window.json" @@ -65,3 +72,14 @@ tests: - "**/*.test.*" - "**/*.spec.*" - "**/__tests__/**" + +authors: + low: + - "devin-ai-integration[bot]" + +guards: + additive_rows: + - "model_prices_and_context_window.json" + - "litellm/model_prices_and_context_window_backup.json" + lowered_limits: + - "*-budget.json" diff --git a/.github/scripts/risk_tier.py b/.github/scripts/risk_tier.py index b210172ebed..6f6d6089ace 100644 --- a/.github/scripts/risk_tier.py +++ b/.github/scripts/risk_tier.py @@ -5,23 +5,26 @@ import json import re import subprocess import sys -from collections.abc import Sequence -from dataclasses import dataclass +from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass, replace from pathlib import Path from types import MappingProxyType from typing import Final, Literal, TypeAlias import yaml -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, TypeAdapter, ValidationError from typing_extensions import ReadOnly, TypedDict Tier: TypeAlias = Literal["low", "medium", "high"] +ReadFile: TypeAlias = Callable[[str, str], str | None] +RewriteCheck: TypeAlias = Callable[[str | None, str | None], str | None] TEST_DEF_RE: Final = re.compile(r"^\s*(?:async\s+)?def\s+test_|^\s*(?:it|test)\(") SKIP_RE: Final = re.compile( r"pytest\.mark\.(?:skip(?!if)|xfail)|unittest\.skip\b" r"|\b(?:it|test|describe)\.(?:skip\(\s*[\"'`]|only\()|\bx(?:it|test|describe)\(" ) +SKIP_CALL_RE: Final = re.compile(r"\bpytest\.(?:skip|importorskip)\(|\b(?:it|test|describe)\.fixme\(") ASSERT_RE: Final = re.compile(r"^\s*assert\b|\bexpect\(") GLOB_TOKEN_RE: Final = re.compile(r"(\*\*/|\*\*|\*|\?)") DIFF_BLOCK_SEPARATOR: Final = "\ndiff --git " @@ -32,6 +35,7 @@ QUOTED_PATH_ESCAPES: Final = MappingProxyType( {"a": "\a", "b": "\b", "f": "\f", "n": "\n", "r": "\r", "t": "\t", "v": "\v", '"': '"', "\\": "\\"} ) MAX_LISTED_PATHS: Final = 5 +JSON_OBJECT: Final = TypeAdapter(dict[str, object]) class SizeLimit(BaseModel): @@ -56,6 +60,7 @@ class ModuleRules(BaseModel): class PathRules(BaseModel): model_config = ConfigDict(extra="forbid", frozen=True) high: tuple[str, ...] + medium: tuple[str, ...] = () low: tuple[str, ...] @@ -64,12 +69,25 @@ class TestRules(BaseModel): files: tuple[str, ...] +class AuthorRules(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True) + low: tuple[str, ...] + + +class GuardRules(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True) + additive_rows: tuple[str, ...] = () + lowered_limits: tuple[str, ...] = () + + class RiskConfig(BaseModel): model_config = ConfigDict(extra="forbid", frozen=True) paths: PathRules modules: ModuleRules size: SizeRules tests: TestRules + authors: AuthorRules + guards: GuardRules = GuardRules() @dataclass(frozen=True, slots=True) @@ -90,6 +108,7 @@ class FileChange: added_lines: tuple[str, ...] deleted_lines: tuple[str, ...] previous_path: str | None = None + guarded_rewrite: str | None = None @property def paths(self) -> tuple[str, ...]: @@ -104,29 +123,45 @@ class FileChange: class Rules: config: RiskConfig high_paths: PathMatcher + medium_paths: PathMatcher low_paths: PathMatcher test_files: PathMatcher size_ignored: PathMatcher + additive_rows: PathMatcher + lowered_limits: PathMatcher @staticmethod def from_config(config: RiskConfig) -> Rules: return Rules( config=config, high_paths=PathMatcher.from_globs(config.paths.high), + medium_paths=PathMatcher.from_globs(config.paths.medium), low_paths=PathMatcher.from_globs(config.paths.low), test_files=PathMatcher.from_globs(config.tests.files), size_ignored=PathMatcher.from_globs(config.size.ignore), + additive_rows=PathMatcher.from_globs(config.guards.additive_rows), + lowered_limits=PathMatcher.from_globs(config.guards.lowered_limits), ) def path_tier(self, path: str) -> Tier: if self.high_paths.matches(path): return "high" + if self.medium_paths.matches(path): + return "medium" if self.test_files.matches(path) or self.low_paths.matches(path): return "low" return "medium" def change_tier(self, change: FileChange) -> Tier: - return highest(tuple(self.path_tier(path) for path in change.paths)) + rewrite_tier: Final[Tier] = "low" if change.guarded_rewrite is None else "medium" + return highest((rewrite_tier, *(self.path_tier(path) for path in change.paths))) + + def rewrite_check(self, path: str) -> RewriteCheck | None: + if self.additive_rows.matches(path): + return rewritten_rows + if self.lowered_limits.matches(path): + return raised_limits + return None def is_production(self, change: FileChange) -> bool: return self.change_tier(change) != "low" @@ -263,7 +298,14 @@ def paths_factor(changes: Sequence[FileChange], rules: Rules) -> Factor: ) return Factor("paths", "high", f"always-human: {_listed(always_human)}") case "medium": - return Factor("paths", "medium", f"{len(matching)} file(s) outside the docs, tests, and model map tiers") + rewritten: Final = tuple( + f"{change.guarded_rewrite} in `{change.path}`" for change in matching if change.guarded_rewrite + ) + outside: Final = len(matching) - len(rewritten) + outside_note: Final = ( + (f"{outside} file(s) outside the docs, tests, and model map tiers",) if outside else () + ) + return Factor("paths", "medium", "; ".join((*rewritten, *outside_note))) case "low": return Factor("paths", "low", "docs, tests, cookbook, or model map only") @@ -312,12 +354,17 @@ def tests_factor(changes: Sequence[FileChange], rules: Rules) -> Factor: test_changes: Final = tuple(change for change in changes if rules.test_files.matches(change.path)) net_tests: Final = _net(test_changes, TEST_DEF_RE) net_skips: Final = _net(test_changes, SKIP_RE) + silenced: Final = sum( + max(_net((change,), SKIP_CALL_RE), 0) for change in test_changes if _net((change,), TEST_DEF_RE) <= 0 + ) net_asserts: Final = _net(test_changes, ASSERT_RE) production_changed: Final = any(rules.is_production(change) for change in changes) if net_tests < 0: return Factor("tests", "high", f"{-net_tests} test(s) removed") if net_skips > 0: return Factor("tests", "high", f"{net_skips} skip marker(s) added") + if silenced > 0: + return Factor("tests", "high", f"{silenced} skip call(s) added to existing tests") if net_asserts < 0: return Factor("tests", "high", f"{-net_asserts} assertion(s) removed") if not production_changed: @@ -329,10 +376,12 @@ def tests_factor(changes: Sequence[FileChange], rules: Rules) -> Factor: return Factor("tests", "medium", "production code changed with no test touched") -def author_factor(author: str, from_fork: bool) -> Factor: +def author_factor(author: str, from_fork: bool, rules: Rules) -> Factor: if from_fork: return Factor("author", "high", f"`{author}` from a fork") - return Factor("author", "low", f"`{author}` on an internal branch") + if author in rules.config.authors.low: + return Factor("author", "low", f"`{author}` opened it on an internal branch") + return Factor("author", "medium", f"`{author}` opened it by hand on an internal branch") def classify(changes: Sequence[FileChange], author: str, from_fork: bool, rules: Rules) -> Verdict: @@ -341,11 +390,88 @@ def classify(changes: Sequence[FileChange], author: str, from_fork: bool, rules: modules_factor(changes, rules), size_factor(changes, rules), tests_factor(changes, rules), - author_factor(author, from_fork), + author_factor(author, from_fork, rules), ) return Verdict(highest(tuple(factor.tier for factor in factors)), factors) +def _json_object(text: str | None) -> Mapping[str, object] | None: + if text is None: + return None + try: + return MappingProxyType(JSON_OBJECT.validate_json(text)) + except ValidationError: + return None + + +def _as_object(value: object) -> Mapping[str, object] | None: + try: + return MappingProxyType(JSON_OBJECT.validate_python(value)) + except ValidationError: + return None + + +def rewritten_rows(base_text: str | None, head_text: str | None) -> str | None: + if base_text is None: + return None + base: Final = _json_object(base_text) + head: Final = _json_object(head_text) + if base is None or head is None: + return "not a JSON object on both sides" + rewritten: Final = sum(1 for key, value in base.items() if key not in head or head[key] != value) + return f"{rewritten} existing row(s) changed or removed" if rewritten else None + + +def _limits(value: object, prefix: str = "") -> tuple[tuple[str, float], ...]: + node: Final = _as_object(value) + if node is None: + return () + limit: Final = node.get("limit") + own: Final = ((prefix, float(limit)),) if isinstance(limit, int | float) and not isinstance(limit, bool) else () + nested: Final = tuple( + pair for key, child in node.items() if key != "limit" for pair in _limits(child, f"{prefix}/{key}") + ) + return (*own, *nested) + + +def raised_limits(base_text: str | None, head_text: str | None) -> str | None: + if base_text is None: + return None + base: Final = _json_object(base_text) + head: Final = _json_object(head_text) + if base is None or head is None: + return "not a JSON object on both sides" + base_limits: Final = MappingProxyType(dict(_limits(base))) + raised: Final = sum(1 for key, limit in _limits(head) if key not in base_limits or limit > base_limits[key]) + return f"{raised} limit(s) raised" if raised else None + + +def with_guards( + changes: Sequence[FileChange], rules: Rules, base: str, head: str, read_file: ReadFile +) -> tuple[FileChange, ...]: + return tuple(_guarded(change, rules, base, head, read_file) for change in changes) + + +def _guarded(change: FileChange, rules: Rules, base: str, head: str, read_file: ReadFile) -> FileChange: + check: Final = rules.rewrite_check(change.path) + if check is None: + return change + reason: Final = check(read_file(base, change.previous_path or change.path), read_file(head, change.path)) + return change if reason is None else replace(change, guarded_rewrite=reason) + + +def git_show(repo: Path, rev: str, path: str) -> str | None: + completed: Final = subprocess.run( + ("git", "show", f"{rev}:{path}"), + cwd=repo, + capture_output=True, + encoding="utf-8", + errors="replace", + check=False, + ) + return completed.stdout if completed.returncode == 0 else None + + def git_diff(repo: Path, base: str, head: str) -> str: completed: Final = subprocess.run( ("git", "-c", "core.quotePath=false", "diff", "--find-renames", "--no-ext-diff", "-U0", base, head), @@ -384,7 +510,13 @@ class CliArgs(BaseModel): def main(argv: Sequence[str] | None = None) -> int: args: Final = CliArgs.model_validate(vars(build_parser().parse_args(argv))) rules: Final = Rules.from_config(load_config(args.config)) - changes: Final = parse_diff(git_diff(args.repo, args.base, args.head)) + changes: Final = with_guards( + parse_diff(git_diff(args.repo, args.base, args.head)), + rules, + args.base, + args.head, + lambda rev, path: git_show(args.repo, rev, path), + ) verdict: Final = classify(changes, args.author, args.from_fork, rules) if args.json_out is not None: args.json_out.write_text(verdict.to_json(), encoding="utf-8") diff --git a/.github/workflows/risk-gate.yml b/.github/workflows/risk-gate.yml index 0e3645b83a7..654df5225bd 100644 --- a/.github/workflows/risk-gate.yml +++ b/.github/workflows/risk-gate.yml @@ -1,7 +1,7 @@ name: risk-gate -on: # zizmor: ignore[dangerous-triggers] runs the base branch's copy of the gate only; the PR's diff is read as data and never executed - pull_request_target: +on: + pull_request: types: [opened, synchronize, reopened, ready_for_review] branches: - litellm_internal_staging @@ -16,6 +16,7 @@ concurrency: jobs: floor: name: risk-gate floor (shadow) + if: github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latest timeout-minutes: 10 permissions: @@ -43,23 +44,17 @@ jobs: with: version: "0.10.9" - - name: Compute the floor tier with the base branch's copy of the gate + - name: Compute the floor tier env: MERGE_BASE: ${{ steps.revisions.outputs.merge_base }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} - FROM_FORK: ${{ github.event.pull_request.head.repo.full_name != github.repository }} run: | - fork_flag=() - if [ "$FROM_FORK" = "true" ]; then - fork_flag=(--from-fork) - fi uv run --frozen python .github/scripts/risk_tier.py \ --config .github/risk-tiers.yml \ --base "$MERGE_BASE" \ --head "$HEAD_SHA" \ --author "$PR_AUTHOR" \ - "${fork_flag[@]}" \ --json-out "${RUNNER_TEMP}/risk.json" \ | tee -a "$GITHUB_STEP_SUMMARY" diff --git a/tests/test_litellm/test_github_risk_tier.py b/tests/test_litellm/test_github_risk_tier.py index 696f5464d78..a7a23c609f7 100644 --- a/tests/test_litellm/test_github_risk_tier.py +++ b/tests/test_litellm/test_github_risk_tier.py @@ -236,6 +236,7 @@ def test_unconditional_skip_marker_is_high(risk_tier, rules, marker): ' pytest.skip("needs a real key")', '@unittest.skipIf(sys.platform == "win32", "posix only")', '@unittest.skipUnless(HAS_REDIS, "needs redis")', + 'redis = pytest.importorskip("redis")', ], ) def test_conditional_skip_is_not_a_silenced_test(risk_tier, rules, guard): @@ -247,6 +248,29 @@ def test_conditional_skip_is_not_a_silenced_test(risk_tier, rules, guard): assert factor.reason == "1 test(s) added" +@pytest.mark.parametrize( + ("path", "line"), + [ + ("tests/test_litellm/test_a.py", ' pytest.skip("flaky since the refactor, see LIT-0000")'), + ("tests/test_litellm/test_a.py", 'redis = pytest.importorskip("redis")'), + ("ui/litellm-dashboard/src/app/login/LoginPage.test.tsx", " test.fixme(true, 'broken after the redesign');"), + ], +) +def test_skip_call_added_to_an_existing_test_is_high_even_without_production_code(risk_tier, rules, path, line): + verdict = _verdict(risk_tier, rules, _file_diff(path, added=(line,))) + assert verdict.tier == "high" + assert _factor(verdict, "tests").reason == "1 skip call(s) added to existing tests" + + +def test_removing_a_skip_call_is_not_silencing(risk_tier, rules): + diff = _file_diff("tests/test_litellm/test_a.py", deleted=(' pytest.skip("flaky")',)) + _file_diff( + "litellm/llms/anthropic/chat/x.py", added=("x = 1",) + ) + factor = _factor(_verdict(risk_tier, rules, diff), "tests") + assert factor.tier == "medium" + assert factor.reason == "tests edited, none added" + + def test_weakened_assertions_are_high(risk_tier, rules): diff = _file_diff( "tests/test_litellm/test_a.py", @@ -311,7 +335,7 @@ def test_typescript_tests_count_like_python_ones(risk_tier, rules, added, expect ("author", "from_fork", "expected"), [ (DEVIN, False, "low"), - ("mateo-berri", False, "low"), + ("mateo-berri", False, "medium"), (DEVIN, True, "high"), ("jairandresdiazp", True, "high"), ], @@ -355,7 +379,17 @@ def test_author_factor(risk_tier, rules, author, from_fork, expected): ("ui/litellm-dashboard/src/app/login/LoginPage.test.tsx", "low"), ("tests/e2e/x.py", "low"), ("litellm-proxy-extras/tests/test_x.py", "low"), - ("helm/litellm-helm/tests/x.yaml", "low"), + ("helm/litellm-helm/tests/x.yaml", "medium"), + ("helm/litellm-helm/templates/tests/test-connection.yaml", "medium"), + ("docker/tests/nonroot.yaml", "medium"), + ("docker/entrypoint.sh", "medium"), + ("CLAUDE.md", "medium"), + ("AGENTS.md", "medium"), + ("GEMINI.md", "medium"), + ("litellm/proxy/_experimental/mcp_server/CLAUDE.md", "medium"), + ("tests/conftest.py", "medium"), + ("tests/test_litellm/conftest.py", "medium"), + ("tests/e2e/junit_properties.py", "low"), ("ui/litellm-dashboard/tests/x.spec.ts", "low"), ("litellm-rust/crates/core/tests/x.rs", "low"), ("enterprise/litellm_enterprise/proxy/common_utils/x.py", "medium"), @@ -371,6 +405,73 @@ def test_path_tier_from_the_checked_in_config(rules, path, expected): assert rules.path_tier(path) == expected +def test_human_opened_docs_change_is_medium_on_the_author_factor_only(risk_tier, rules): + verdict = _verdict(risk_tier, rules, _file_diff("README.md", added=("hello",)), author="mateo-berri") + assert verdict.tier == "medium" + assert {factor.name: factor.tier for factor in verdict.factors} == { + "paths": "low", + "modules": "low", + "size": "low", + "tests": "low", + "author": "medium", + } + + +MODEL_MAP = "model_prices_and_context_window.json" +BUDGET = "ruff-strict-budget.json" + + +def _guarded_verdict(risk_tier, rules, path: str, base: str | None, head: str | None): + contents = {("base", path): base, ("head", path): head} + diff = _file_diff(path, added=("changed",)) + changes = risk_tier.with_guards(risk_tier.parse_diff(diff), rules, "base", "head", lambda rev, p: contents[(rev, p)]) + return risk_tier.classify(changes, DEVIN, False, rules) + + +def test_additive_model_map_rows_stay_low(risk_tier, rules): + base = json.dumps({"gpt-x": {"input_cost_per_token": 1e-6}}) + head = json.dumps({"gpt-x": {"input_cost_per_token": 1e-6}, "gpt-y": {"input_cost_per_token": 2e-6}}) + verdict = _guarded_verdict(risk_tier, rules, MODEL_MAP, base, head) + assert verdict.tier == "low" + assert _factor(verdict, "paths").reason == "docs, tests, cookbook, or model map only" + + +@pytest.mark.parametrize( + "head", + [ + {"gpt-x": {"input_cost_per_token": 3e-6}}, + {"gpt-x-renamed": {"input_cost_per_token": 1e-6}}, + {}, + ], +) +def test_changed_or_removed_model_map_rows_are_medium(risk_tier, rules, head): + base = json.dumps({"gpt-x": {"input_cost_per_token": 1e-6}}) + verdict = _guarded_verdict(risk_tier, rules, MODEL_MAP, base, json.dumps(head)) + assert verdict.tier == "medium" + assert _factor(verdict, "paths").tier == "medium" + assert _factor(verdict, "paths").reason == f"1 existing row(s) changed or removed in `{MODEL_MAP}`" + + +def test_new_model_map_file_or_broken_json_is_never_low(risk_tier, rules): + assert _guarded_verdict(risk_tier, rules, MODEL_MAP, None, "{}").tier == "low" + assert _factor(_guarded_verdict(risk_tier, rules, MODEL_MAP, "{}", "not json"), "paths").tier == "medium" + assert _factor(_guarded_verdict(risk_tier, rules, MODEL_MAP, "{}", None), "paths").tier == "medium" + + +def test_lowered_budget_limits_stay_low_and_raised_ones_are_medium(risk_tier, rules): + base = json.dumps({"ANN001": {"limit": 10}, "B006": {"limit": 5}}) + lowered = json.dumps({"ANN001": {"limit": 9}, "B006": {"limit": 5}}) + raised = json.dumps({"ANN001": {"limit": 10}, "B006": {"limit": 6}}) + new_rule = json.dumps({"ANN001": {"limit": 10}, "B006": {"limit": 5}, "B999": {"limit": 1}}) + dropped_rule = json.dumps({"ANN001": {"limit": 10}}) + assert _guarded_verdict(risk_tier, rules, BUDGET, base, lowered).tier == "low" + assert _guarded_verdict(risk_tier, rules, BUDGET, base, dropped_rule).tier == "low" + for head in (raised, new_rule): + verdict = _guarded_verdict(risk_tier, rules, BUDGET, base, head) + assert verdict.tier == "medium" + assert _factor(verdict, "paths").reason == f"1 limit(s) raised in `{BUDGET}`" + + def test_single_star_does_not_cross_directories(risk_tier): assert risk_tier.glob_to_regex("Dockerfile*").fullmatch("Dockerfile.database") assert risk_tier.glob_to_regex("Dockerfile*").fullmatch("docker/Dockerfile.database") is None @@ -513,6 +614,31 @@ def test_main_end_to_end_against_a_git_repo(risk_tier, tmp_path, capsys): assert payload["summary"] == printed +def test_main_reads_both_sides_of_the_model_map_from_git(risk_tier, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(tmp_path, "init", "-q", "-b", "main", "repo") + model_map = repo / "model_prices_and_context_window.json" + model_map.write_text(json.dumps({"gpt-x": {"input_cost_per_token": 1e-6}}, indent=1)) + _git(repo, "add", ".") + _git(repo, "commit", "-q", "-m", "base") + base = _git(repo, "rev-parse", "HEAD") + model_map.write_text(json.dumps({"gpt-x": {"input_cost_per_token": 2e-6}}, indent=1)) + _git(repo, "commit", "-q", "-am", "reprice") + head = _git(repo, "rev-parse", "HEAD") + json_out = tmp_path / "risk.json" + + exit_code = risk_tier.main( + ["--config", str(CONFIG_PATH), "--base", base, "--head", head, "--author", DEVIN, "--repo", str(repo), "--json-out", str(json_out)] + ) + + assert exit_code == 0 + payload = json.loads(json_out.read_text()) + assert payload["tier"] == "medium" + paths = next(factor for factor in payload["factors"] if factor["name"] == "paths") + assert paths["reason"] == "1 existing row(s) changed or removed in `model_prices_and_context_window.json`" + + def test_git_quoted_filename_still_reaches_the_paths_factor(risk_tier, rules, tmp_path): repo = tmp_path / "repo" (repo / "litellm" / "proxy" / "auth").mkdir(parents=True)