From e1dcb740a0f533dda0839a8cc606782dc5ac94e0 Mon Sep 17 00:00:00 2001 From: mateo Date: Sat, 8 Aug 2026 07:30:18 +0000 Subject: [PATCH] refactor(ci): ratchet the grandfathered extra="allow" list through the budget gate Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .github/workflows/test-code-quality.yml | 8 - CLAUDE.md | 2 +- extra-allow-budget.json | 61 +++++++ scripts/budget_ratchet_check.py | 5 +- scripts/pre_commit_lint.sh | 17 +- .../ban_pydantic_extra_allow.py | 149 ++++-------------- .../test_ban_pydantic_extra_allow.py | 91 ++++++----- 7 files changed, 150 insertions(+), 183 deletions(-) create mode 100644 extra-allow-budget.json diff --git a/.github/workflows/test-code-quality.yml b/.github/workflows/test-code-quality.yml index a02962ec568..3f488acc6b6 100644 --- a/.github/workflows/test-code-quality.yml +++ b/.github/workflows/test-code-quality.yml @@ -119,14 +119,6 @@ jobs: - name: ban_pydantic_extra_allow run: uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py - - name: ban_pydantic_extra_allow (grandfathered list only shrinks) - if: github.event_name == 'pull_request' - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - run: | - git fetch --no-tags --depth=1 origin "$BASE_SHA" - uv run --no-sync python ./tests/code_coverage_tests/ban_pydantic_extra_allow.py --base "$BASE_SHA" - - name: check_fastuuid_usage run: uv run --no-sync python ./tests/code_coverage_tests/check_fastuuid_usage.py diff --git a/CLAUDE.md b/CLAUDE.md index 54b4d5ba392..0697d6bf522 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,7 +43,7 @@ When you fix violations gated by `ruff-strict-budget.json`, `type-discipline-bud `make pre-commit` saves its complete output to a log file in .git (overwriting previous pre-commit logs) and prints that path as its first and last output lines. To inspect a run, read or grep that log instead of re-running the multi-minute checks just to see a different slice -New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models that already had it, so don't add it to a new model. That list only shrinks: the check fails when your branch adds an entry to it, so removing a model's `extra="allow"` is the only edit it accepts. `make pre-commit` runs it on any commit that touches `litellm/` Python +New Pydantic models must declare the fields they accept. `extra="allow"` is banned by `tests/code_coverage_tests/ban_pydantic_extra_allow.py`, which grandfathers the models `extra-allow-budget.json` lists, so don't add it to a new model. That budget ratchets like the others: its `limit` must equal the number of models it lists, so grandfathering one more means raising the limit, which reds the non-gating `budget-ratchet` job for a human to accept. Clean a model up and you lower both. `make pre-commit` runs the ban on any commit that touches `litellm/` Python If you're trying to create a new function that relies on untyped stuff, instead of adding more Any's and pushing `reportAny` / `reportExplicitAny` closer to their basedpyright ceilings, just validate it in the caller with Pydantic (a model or `TypeAdapter` that returns the typed thing or raises will do) and then pass the now typed variable in diff --git a/extra-allow-budget.json b/extra-allow-budget.json new file mode 100644 index 00000000000..c5f8ba85940 --- /dev/null +++ b/extra-allow-budget.json @@ -0,0 +1,61 @@ +{ + "extra_allow_models": { + "limit": 54, + "models": [ + "litellm/llms/anthropic/common_utils.py::_ReplayedSearchQuery", + "litellm/llms/anthropic/common_utils.py::_ReplayedServerToolUse", + "litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchResult", + "litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchToolResult", + "litellm/llms/base_llm/ocr/transformation.py::OCRPage", + "litellm/llms/base_llm/ocr/transformation.py::OCRPageImage", + "litellm/llms/base_llm/ocr/transformation.py::OCRResponse", + "litellm/llms/base_llm/ocr/transformation.py::OCRUsageInfo", + "litellm/llms/base_llm/sandbox/transformation.py::CodeExecutionResult", + "litellm/llms/base_llm/sandbox/transformation.py::ContainerHandle", + "litellm/llms/base_llm/search/transformation.py::SearchResponse", + "litellm/llms/base_llm/search/transformation.py::SearchResult", + "litellm/proxy/_types.py::CoordinationRedisParams", + "litellm/proxy/_types.py::ModelInfo", + "litellm/proxy/_types.py::TeamDefaultSettings", + "litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py::UISettings", + "litellm/router_strategy/auto_router/litellm_encoder.py::CustomDenseEncoder", + "litellm/router_strategy/complexity_router/config.py::ComplexityRouterConfig", + "litellm/router_strategy/quality_router/config.py::QualityRouterConfig", + "litellm/router_strategy/quality_router/config.py::RoutingPreferences", + "litellm/types/agents.py::AgentCreateResponse", + "litellm/types/agents.py::AgentDeleteResult", + "litellm/types/agents.py::AgentListResponse", + "litellm/types/agents.py::AgentVersionsResponse", + "litellm/types/agents.py::LiteLLMSendMessageResponse", + "litellm/types/completion.py::CompletionRequest", + "litellm/types/embedding.py::EmbeddingRequest", + "litellm/types/fine_tuning.py::OpenAIFineTuningHyperparameters", + "litellm/types/guardrails.py::BaseLitellmParams", + "litellm/types/llms/anthropic.py::AnthropicResponseContentBlockToolUse", + "litellm/types/llms/anthropic.py::AnthropicResponseUsageBlock", + "litellm/types/llms/base.py::BaseLiteLLMOpenAIResponseObject", + "litellm/types/llms/base.py::HiddenParams", + "litellm/types/llms/openai.py::GenericEvent", + "litellm/types/llms/openai.py::Hyperparameters", + "litellm/types/llms/openai.py::InputTokensDetails", + "litellm/types/llms/openai.py::LiteLLMFineTuningJobCreate", + "litellm/types/llms/openai.py::OutputTokensDetails", + "litellm/types/llms/openai.py::ResponseAPIUsage", + "litellm/types/prompts/init_prompts.py::PromptInfo", + "litellm/types/prompts/init_prompts.py::PromptLiteLLMParams", + "litellm/types/proxy/guardrails/guardrail_hooks/cisco_ai_defense.py::CiscoAIDefenseGuardrailConfigModelOptionalParams", + "litellm/types/proxy/guardrails/guardrail_hooks/generic_guardrail_api.py::GuardrailToolParam", + "litellm/types/proxy/guardrails/guardrail_hooks/straiker.py::StraikerWebhookResponse", + "litellm/types/rag.py::RAGIngestRequest", + "litellm/types/rag.py::RAGQueryRequest", + "litellm/types/realtime.py::RealtimeSessionConfig", + "litellm/types/realtime.py::RealtimeTranscriptionSessionRequest", + "litellm/types/realtime.py::RealtimeTranscriptionSessionResponse", + "litellm/types/router.py::Deployment", + "litellm/types/router.py::GenericLiteLLMParams", + "litellm/types/router.py::LiteLLM_Params", + "litellm/types/router.py::ModelInfo", + "litellm/types/utils.py::ImageResponse" + ] + } +} diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py index 10a78483643..562b849e180 100644 --- a/scripts/budget_ratchet_check.py +++ b/scripts/budget_ratchet_check.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 """Non-gating ratchet guard: budget limits 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 its `limit`, and that limit is meant to be +Every `*-budget.json` file (ruff-strict, type-discipline, basedpyright-code, extra-allow) +is a one-way ratchet: each rule's ceiling is its `limit`, and that limit is 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: @@ -39,6 +39,7 @@ DEFAULT_BUDGETS: tuple[str, ...] = ( "ruff-strict-budget.json", "type-discipline-budget.json", "basedpyright-code-budget.json", + "extra-allow-budget.json", ) diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index 0267f1a5ed6..37a75a8f749 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -6,8 +6,8 @@ # - litellm/ Python staged -> `make lint` (test-linting.yml's lint job) # - tests/e2e Python staged -> `make lint-e2e-basedpyright` (test-linting.yml's e2e type-check step) # + raw HTTP client ban (test-code-quality.yml's check_e2e_no_raw_requests) -# - litellm/ Python or this -# check's own list staged -> extra="allow" ban (test-code-quality.yml's ban_pydantic_extra_allow) +# - litellm/ Python or the +# grandfathered list staged -> extra="allow" ban (test-code-quality.yml's ban_pydantic_extra_allow) # - dashboard staged -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) # - proxy/types staged -> regenerate dashboard API types and fail on drift (check-ui-api-types.yml) # @@ -49,9 +49,9 @@ litellm_py_files=$(staged_match '^litellm/.*\.py$') e2e_py_files=$(staged_match '^tests/e2e/.*\.py$') # ruff format (and CI's format step) skip enterprise; the rest of make lint covers it. fmt_files=$(printf '%s\n' "$litellm_py_files" | grep -v '^litellm/enterprise/' || true) -# The extra="allow" ban reads all of litellm/, and its grandfathered list is data the -# check itself gates, so editing either can turn ban_pydantic_extra_allow red. -extra_allow_files=$(staged_match '^(litellm/.*\.py|tests/code_coverage_tests/ban_pydantic_extra_allow\.py)$') +# The extra="allow" ban reads all of litellm/, and its grandfathered models are a budget +# the check reads, so editing either can turn ban_pydantic_extra_allow red. +extra_allow_files=$(staged_match '^(litellm/.*\.py|extra-allow-budget\.json|tests/code_coverage_tests/ban_pydantic_extra_allow\.py)$') # check-ui-api-types.yml triggers on any file under litellm/proxy or litellm/types # (Prisma schema and configs included, not just Python) plus the generator and its # lockfiles, so match that whole trigger set rather than a Python subset. @@ -162,14 +162,11 @@ if [ -n "$e2e_py_files" ]; then fi # Around two seconds over all of litellm/, so it runs inline rather than behind a job. -# CI ratchets the grandfathered list against the base branch tip; locally the closest -# equivalent is the merge-base, and the check reports the ref it couldn't read. +# Growing the list is the budget-ratchet job's business, which is non-gating and CI-only. if [ -n "$extra_allow_files" ]; then echo "pre-commit: checking new pydantic models don't set extra=\"allow\" (ban_pydantic_extra_allow)" - extra_allow_base=$(git merge-base origin/litellm_internal_staging HEAD 2>/dev/null || true) uv run --no-sync python tests/code_coverage_tests/ban_pydantic_extra_allow.py \ - ${extra_allow_base:+--base "$extra_allow_base"} \ - || { echo "✗ New extra=\"allow\" model or a grown grandfathered list. Declare the fields the model accepts, then re-run make pre-commit." >&2; status=1; } + || { echo "✗ New extra=\"allow\" model, or extra-allow-budget.json is stale. Declare the fields the model accepts, then re-run make pre-commit." >&2; status=1; } fi dashboard_checks() { diff --git a/tests/code_coverage_tests/ban_pydantic_extra_allow.py b/tests/code_coverage_tests/ban_pydantic_extra_allow.py index 1b8c12086f1..f9bd3a3cfcb 100644 --- a/tests/code_coverage_tests/ban_pydantic_extra_allow.py +++ b/tests/code_coverage_tests/ban_pydantic_extra_allow.py @@ -2,81 +2,34 @@ ``extra="allow"`` silently accepts undeclared keys, so typos survive validation and real fields (pricing on ``ModelInfo``, for example) never get declared anywhere. The -models listed in ``GRANDFATHERED`` predate this check and stay allowed; anything new -must declare its fields. +models listed in ``extra-allow-budget.json`` predate this check and stay allowed; +anything new must declare its fields. -The list is one way. With ``--base `` the check also fails when it gained an entry -relative to that ref, so the only edits to it that pass are removals. +That list is a budget like the others, so ``scripts/budget_ratchet_check.py`` is what +reds when it grows, and its ``limit`` must equal the number of models it lists. """ -import argparse import ast +import json import os -import subprocess import sys +from types import MappingProxyType from typing import Final, Iterator, NamedTuple, Sequence SCAN_ROOT: Final = "litellm" -SELF_PATH: Final = "tests/code_coverage_tests/ban_pydantic_extra_allow.py" +BUDGET_PATH: Final = "extra-allow-budget.json" +BUDGET_RULE: Final = "extra_allow_models" -GRANDFATHERED: Final = frozenset( - { - "litellm/llms/anthropic/common_utils.py::_ReplayedSearchQuery", - "litellm/llms/anthropic/common_utils.py::_ReplayedServerToolUse", - "litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchResult", - "litellm/llms/anthropic/common_utils.py::_ReplayedWebSearchToolResult", - "litellm/llms/base_llm/ocr/transformation.py::OCRPage", - "litellm/llms/base_llm/ocr/transformation.py::OCRPageImage", - "litellm/llms/base_llm/ocr/transformation.py::OCRResponse", - "litellm/llms/base_llm/ocr/transformation.py::OCRUsageInfo", - "litellm/llms/base_llm/sandbox/transformation.py::CodeExecutionResult", - "litellm/llms/base_llm/sandbox/transformation.py::ContainerHandle", - "litellm/llms/base_llm/search/transformation.py::SearchResponse", - "litellm/llms/base_llm/search/transformation.py::SearchResult", - "litellm/proxy/_types.py::CoordinationRedisParams", - "litellm/proxy/_types.py::ModelInfo", - "litellm/proxy/_types.py::TeamDefaultSettings", - "litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py::UISettings", - "litellm/router_strategy/auto_router/litellm_encoder.py::CustomDenseEncoder", - "litellm/router_strategy/complexity_router/config.py::ComplexityRouterConfig", - "litellm/router_strategy/quality_router/config.py::QualityRouterConfig", - "litellm/router_strategy/quality_router/config.py::RoutingPreferences", - "litellm/types/agents.py::AgentCreateResponse", - "litellm/types/agents.py::AgentDeleteResult", - "litellm/types/agents.py::AgentListResponse", - "litellm/types/agents.py::AgentVersionsResponse", - "litellm/types/agents.py::LiteLLMSendMessageResponse", - "litellm/types/completion.py::CompletionRequest", - "litellm/types/embedding.py::EmbeddingRequest", - "litellm/types/fine_tuning.py::OpenAIFineTuningHyperparameters", - "litellm/types/guardrails.py::BaseLitellmParams", - "litellm/types/llms/anthropic.py::AnthropicResponseContentBlockToolUse", - "litellm/types/llms/anthropic.py::AnthropicResponseUsageBlock", - "litellm/types/llms/base.py::BaseLiteLLMOpenAIResponseObject", - "litellm/types/llms/base.py::HiddenParams", - "litellm/types/llms/openai.py::GenericEvent", - "litellm/types/llms/openai.py::Hyperparameters", - "litellm/types/llms/openai.py::InputTokensDetails", - "litellm/types/llms/openai.py::LiteLLMFineTuningJobCreate", - "litellm/types/llms/openai.py::OutputTokensDetails", - "litellm/types/llms/openai.py::ResponseAPIUsage", - "litellm/types/prompts/init_prompts.py::PromptInfo", - "litellm/types/prompts/init_prompts.py::PromptLiteLLMParams", - "litellm/types/proxy/guardrails/guardrail_hooks/cisco_ai_defense.py::CiscoAIDefenseGuardrailConfigModelOptionalParams", - "litellm/types/proxy/guardrails/guardrail_hooks/generic_guardrail_api.py::GuardrailToolParam", - "litellm/types/proxy/guardrails/guardrail_hooks/straiker.py::StraikerWebhookResponse", - "litellm/types/rag.py::RAGIngestRequest", - "litellm/types/rag.py::RAGQueryRequest", - "litellm/types/realtime.py::RealtimeSessionConfig", - "litellm/types/realtime.py::RealtimeTranscriptionSessionRequest", - "litellm/types/realtime.py::RealtimeTranscriptionSessionResponse", - "litellm/types/router.py::Deployment", - "litellm/types/router.py::GenericLiteLLMParams", - "litellm/types/router.py::LiteLLM_Params", - "litellm/types/router.py::ModelInfo", - "litellm/types/utils.py::ImageResponse", - } -) + +class Budget(NamedTuple): + limit: int + models: frozenset[str] + + +def read_budget(path: str) -> Budget: + with open(path, encoding="utf-8") as handle: + rule: Final = MappingProxyType(json.load(handle)[BUDGET_RULE]) + return Budget(limit=int(rule["limit"]), models=frozenset(rule["models"])) class Violation(NamedTuple): @@ -274,48 +227,13 @@ def find_extra_allow_models(base_dir: str) -> tuple[Violation, ...]: ) -def parse_grandfathered(source: str) -> frozenset[str]: - """The entries ``GRANDFATHERED`` lists in ``source``, so another revision of this file - can be read without importing it. The entries are string literals, so they are collected - from the assignment's subtree rather than evaluating the ``frozenset`` call around them.""" - return frozenset( - node.value - for statement in ast.parse(source).body - for name, value in _assigned_names(statement) - if name == "GRANDFATHERED" - for node in ast.walk(value) - if isinstance(node, ast.Constant) and isinstance(node.value, str) - ) - - -def _source_at(ref: str) -> str | None: - completed: Final = subprocess.run( - ["git", "show", f"{ref}:{SELF_PATH}"], capture_output=True, text=True, check=False - ) - return completed.stdout if completed.returncode == 0 else None - - -def added_grandfathered(base_ref: str) -> tuple[str, ...]: - base_source: Final = _source_at(base_ref) - if base_source is None: - print(f"{SELF_PATH} does not exist at {base_ref}, so there is no list to ratchet against yet.") - return () - return tuple(sorted(GRANDFATHERED - parse_grandfathered(base_source))) - - def main() -> int: - parser: Final = argparse.ArgumentParser(description=__doc__) - parser.add_argument( - "--base", - help="git ref to ratchet GRANDFATHERED against, failing when this branch added an entry", - ) - args: Final = parser.parse_args() - - base_dir = os.getcwd() - found = find_extra_allow_models(base_dir) - violations = tuple(violation for violation in found if violation.identifier() not in GRANDFATHERED) - stale = tuple(sorted(GRANDFATHERED - {violation.identifier() for violation in found})) - added = added_grandfathered(args.base) if args.base else () + base_dir: Final = os.getcwd() + budget: Final = read_budget(os.path.join(base_dir, BUDGET_PATH)) + found: Final = find_extra_allow_models(base_dir) + violations: Final = tuple(violation for violation in found if violation.identifier() not in budget.models) + stale: Final = tuple(sorted(budget.models - {violation.identifier() for violation in found})) + miscounted: Final = budget.limit != len(budget.models) for violation in violations: print(f'{violation.file}:{violation.line}: {violation.model} sets extra="allow"') @@ -324,25 +242,24 @@ def main() -> int: f'\nFound {len(violations)} new Pydantic model(s) using extra="allow".\n' 'Declare the fields you accept instead. extra="allow" hides typos and\n' "leaves real fields undocumented and untyped. If a model genuinely has to\n" - "forward opaque provider payloads, add it to GRANDFATHERED in\n" - "tests/code_coverage_tests/ban_pydantic_extra_allow.py with a reason in the PR." + f"forward opaque provider payloads, add it to {BUDGET_RULE} in {BUDGET_PATH},\n" + "raise the limit to match, and say why in the PR: raising it reds the\n" + "non-gating budget-ratchet check so the loosening is seen and accepted." ) if stale: print( - '\nThese GRANDFATHERED entries no longer use extra="allow" (or moved).\n' - "Remove them so the list keeps ratcheting down:" + f'\nThese {BUDGET_PATH} models no longer use extra="allow" (or moved).\n' + "Remove them and lower the limit so it keeps ratcheting down:" ) for entry in stale: print(f" {entry}") - if added: + if miscounted: print( - f"\nThis branch added {len(added)} entry/entries to GRANDFATHERED. The list only\n" - "shrinks, so declare the fields these models accept instead of grandfathering them:" + f"\n{BUDGET_PATH} lists {len(budget.models)} models under a limit of {budget.limit}.\n" + "They must match, since the limit is what the budget ratchet reads." ) - for entry in added: - print(f" {entry}") - if violations or stale or added: + if violations or stale or miscounted: return 1 print('No new extra="allow" Pydantic models found.') return 0 diff --git a/tests/test_litellm/test_ban_pydantic_extra_allow.py b/tests/test_litellm/test_ban_pydantic_extra_allow.py index b64af3570be..d302f558f1c 100644 --- a/tests/test_litellm/test_ban_pydantic_extra_allow.py +++ b/tests/test_litellm/test_ban_pydantic_extra_allow.py @@ -1,7 +1,8 @@ """Tests for the extra="allow" ban at tests/code_coverage_tests/ban_pydantic_extra_allow.py.""" +import importlib.util +import json import os -import subprocess import sys import pytest @@ -13,6 +14,11 @@ import ban_pydantic_extra_allow as checker # noqa: E402 _REPO_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..") +_RATCHET_PATH = os.path.join(_REPO_ROOT, "scripts", "budget_ratchet_check.py") +_ratchet_spec = importlib.util.spec_from_file_location("budget_ratchet_check", _RATCHET_PATH) +ratchet = importlib.util.module_from_spec(_ratchet_spec) +_ratchet_spec.loader.exec_module(ratchet) + @pytest.mark.parametrize( "source", @@ -187,64 +193,57 @@ def test_reports_nested_class_with_qualified_name(): assert [violation.identifier() for violation in violations] == ["litellm/types/thing.py::Outer.Inner"] -def test_grandfathered_list_matches_repo(): - """Every grandfathered entry must still exist, and nothing new may be added.""" +def test_budget_matches_repo(): + """Every grandfathered model must still exist, and nothing new may be added.""" + budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH)) found = frozenset(violation.identifier() for violation in checker.find_extra_allow_models(_REPO_ROOT)) - assert sorted(found - checker.GRANDFATHERED) == [] - assert sorted(checker.GRANDFATHERED - found) == [] + assert sorted(found - budget.models) == [] + assert sorted(budget.models - found) == [] -def test_parse_grandfathered_reads_the_list_of_another_revision(): - source = 'OTHER = frozenset({"nope"})\nGRANDFATHERED = frozenset(\n {\n "a.py::A",\n "b.py::B",\n }\n)\n' - assert checker.parse_grandfathered(source) == frozenset({"a.py::A", "b.py::B"}) +def test_budget_limit_matches_the_models_it_lists(): + """The limit is what the budget ratchet reads, so it can't drift from the list.""" + budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH)) + assert budget.limit == len(budget.models) -def test_parse_grandfathered_reads_this_files_own_list(): - with open(os.path.join(_REPO_ROOT, checker.SELF_PATH), encoding="utf-8") as handle: - assert checker.parse_grandfathered(handle.read()) == checker.GRANDFATHERED +def _write_budget(path, limit, models): + with open(path, "w", encoding="utf-8") as handle: + json.dump({checker.BUDGET_RULE: {"limit": limit, "models": list(models)}}, handle) + return path -def _baseline_commit(repo, entries): - """A one-commit repo whose copy of the checker grandfathers exactly ``entries``.""" - target = os.path.join(repo, checker.SELF_PATH) - os.makedirs(os.path.dirname(target), exist_ok=True) - with open(target, "w", encoding="utf-8") as handle: - handle.write("GRANDFATHERED = frozenset(\n {\n") - handle.writelines(f' "{entry}",\n' for entry in entries) - handle.write(" }\n)\n") - for command in ( - ["git", "init", "-q", "."], - ["git", "add", checker.SELF_PATH], - ["git", "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-qm", "baseline"], - ): - subprocess.run(command, cwd=repo, check=True) - return subprocess.run( - ["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True - ).stdout.strip() +def test_read_budget_reads_the_limit_and_the_models(tmp_path): + path = _write_budget(tmp_path / "extra-allow-budget.json", 2, ["a.py::A", "b.py::B"]) + assert checker.read_budget(str(path)) == checker.Budget(limit=2, models=frozenset({"a.py::A", "b.py::B"})) @pytest.mark.parametrize( - "baseline, expected", + "limit, expected_exit", [ - pytest.param(checker.GRANDFATHERED, [], id="unchanged_list_passes"), - pytest.param( - checker.GRANDFATHERED | {"litellm/types/gone.py::Gone"}, - [], - id="removing_an_entry_passes", - ), - pytest.param( - checker.GRANDFATHERED - {"litellm/types/utils.py::ImageResponse"}, - ["litellm/types/utils.py::ImageResponse"], - id="adding_an_entry_is_reported", - ), + pytest.param(1, 0, id="limit_matching_the_single_model_passes"), + pytest.param(2, 1, id="limit_above_the_models_listed_fails"), ], ) -def test_added_grandfathered_only_reports_growth(tmp_path, monkeypatch, baseline, expected): - base_sha = _baseline_commit(str(tmp_path), sorted(baseline)) +def test_main_requires_the_limit_to_match_the_list(tmp_path, monkeypatch, limit, expected_exit): + """A limit above the list would buy silent headroom the budget ratchet can't see.""" + model_dir = tmp_path / checker.SCAN_ROOT / "types" + model_dir.mkdir(parents=True) + (model_dir / "thing.py").write_text('class Foo(BaseModel):\n model_config = ConfigDict(extra="allow")\n') + _write_budget(tmp_path / checker.BUDGET_PATH, limit, [f"{checker.SCAN_ROOT}/types/thing.py::Foo"]) monkeypatch.chdir(tmp_path) - assert list(checker.added_grandfathered(base_sha)) == expected + assert checker.main() == expected_exit -def test_added_grandfathered_skips_a_base_without_the_checker(tmp_path, monkeypatch): - monkeypatch.chdir(tmp_path) - assert checker.added_grandfathered("HEAD") == () +def test_raising_the_limit_reds_the_budget_ratchet(): + """Grandfathering one more model means raising the limit, which the shared ratchet catches.""" + budget = checker.read_budget(os.path.join(_REPO_ROOT, checker.BUDGET_PATH)) + base = {checker.BUDGET_RULE: {"limit": budget.limit}} + head = {checker.BUDGET_RULE: {"limit": budget.limit + 1}} + regressions = ratchet.regressions_for(checker.BUDGET_PATH, base, head) + assert [regression.rule for regression in regressions] == [checker.BUDGET_RULE] + assert ratchet.regressions_for(checker.BUDGET_PATH, base, {checker.BUDGET_RULE: {"limit": budget.limit - 1}}) == [] + + +def test_the_budget_ratchet_watches_this_budget(): + assert checker.BUDGET_PATH in ratchet.DEFAULT_BUDGETS