diff --git a/.github/workflows/publish-basedpyright-base-counts.yml b/.github/workflows/publish-basedpyright-base-counts.yml deleted file mode 100644 index 27d4682dbd9..00000000000 --- a/.github/workflows/publish-basedpyright-base-counts.yml +++ /dev/null @@ -1,66 +0,0 @@ -name: Publish basedpyright base counts - -# Every commit on main or litellm_internal_staging can become a future merge-base. -# Publishing its per-rule basedpyright counts as an artifact lets -# scripts/type_check_gate.py download them in seconds instead of paying a -# 60-110s second basedpyright pass on every fresh worktree or moved merge-base. -# No concurrency group on purpose: runs must never cancel each other, because -# every sha's artifact matters (any of them can become a merge-base). - -on: - push: - branches: - - main - - litellm_internal_staging - workflow_dispatch: - inputs: - ref: - description: "Ref to compute and publish base counts for (defaults to the workflow run's commit)" - required: false - -permissions: - contents: read - -jobs: - publish: - runs-on: ubuntu-latest - timeout-minutes: 20 - - steps: - - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - with: - ref: ${{ inputs.ref || github.sha }} - clean: true - persist-credentials: false - - - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 - with: - python-version: "3.12" - - - name: Set up uv - uses: ./.github/actions/setup-uv-with-retries - with: - version: "0.10.9" - - - name: Cache the Rust build - uses: ./.github/actions/cache-cargo-build - - - name: Cache Prisma binaries - uses: ./.github/actions/cache-prisma-binaries - - # The gate provisions its own measurement env (.venv-typecheck: a frozen - # uv sync of its canonical dependency groups plus a generated Prisma - # client), so no install step here can drift from what local runs measure. - - name: Emit basedpyright counts for HEAD - run: | - python scripts/type_check_gate.py --emit-counts-dir "$RUNNER_TEMP/basedpyright-counts" - counts_file=$(ls "$RUNNER_TEMP"/basedpyright-counts/basedpyright-counts-*.json) - echo "COUNTS_ARTIFACT_NAME=$(basename "$counts_file" .json)" >> "$GITHUB_ENV" - - - name: Upload counts artifact - uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 - with: - name: ${{ env.COUNTS_ARTIFACT_NAME }} - path: ${{ runner.temp }}/basedpyright-counts/ - if-no-files-found: error diff --git a/.github/workflows/publish-lint-base-counts.yml b/.github/workflows/publish-lint-base-counts.yml new file mode 100644 index 00000000000..0051c4be523 --- /dev/null +++ b/.github/workflows/publish-lint-base-counts.yml @@ -0,0 +1,97 @@ +name: Publish lint base counts + +# Every commit on main or litellm_internal_staging can become a future merge-base. +# Publishing its per-rule counts for each lint gate (strict ruff, type discipline, +# test quality, basedpyright) as an artifact lets the gates download them through +# scripts/lint_base_counts.py in seconds instead of scanning the merge-base tree in +# a throwaway worktree on every fresh checkout or moved merge-base. +# No concurrency group on purpose: runs must never cancel each other, because +# every sha's artifact matters (any of them can become a merge-base). + +on: + push: + branches: + - main + - litellm_internal_staging + workflow_dispatch: + inputs: + ref: + description: "Ref to compute and publish base counts for (defaults to the workflow run's commit)" + required: false + +permissions: + contents: read + +jobs: + publish: + runs-on: ubuntu-latest + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + include: + - checker: ruff-strict + script: scripts/ruff_strict_gate.py + - checker: type-discipline + script: scripts/type_discipline_gate.py + - checker: test-quality + script: scripts/test_quality_gate.py + - checker: basedpyright + script: scripts/type_check_gate.py + + steps: + - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + with: + ref: ${{ inputs.ref || github.sha }} + clean: true + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Set up uv + uses: ./.github/actions/setup-uv-with-retries + with: + version: "0.10.9" + + - name: Cache the Rust build + if: matrix.checker == 'basedpyright' + uses: ./.github/actions/cache-cargo-build + + - name: Cache Prisma binaries + if: matrix.checker == 'basedpyright' + uses: ./.github/actions/cache-prisma-binaries + + # The three source scanners only need the pinned dev tools (ruff and the + # stdlib checkers), the same versions test-linting.yml's lint job runs. + - name: Install the dev tools + if: matrix.checker != 'basedpyright' + run: | + uv sync --frozen --only-group dev --no-install-project + + - name: Emit ${{ matrix.checker }} counts for HEAD + if: matrix.checker != 'basedpyright' + run: | + uv run --no-sync python ${{ matrix.script }} --emit-counts-dir "$RUNNER_TEMP/lint-counts" + + # The basedpyright gate provisions its own measurement env (.venv-typecheck: + # a frozen uv sync of its canonical dependency groups plus a generated Prisma + # client), so no install step here can drift from what local runs measure. + - name: Emit basedpyright counts for HEAD + if: matrix.checker == 'basedpyright' + run: | + python scripts/type_check_gate.py --emit-counts-dir "$RUNNER_TEMP/lint-counts" + + - name: Name the counts artifact + run: | + counts_file=$(ls "$RUNNER_TEMP"/lint-counts/${{ matrix.checker }}-counts-*.json) + echo "COUNTS_ARTIFACT_NAME=$(basename "$counts_file" .json)" >> "$GITHUB_ENV" + + - name: Upload counts artifact + uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1 + with: + name: ${{ env.COUNTS_ARTIFACT_NAME }} + path: ${{ runner.temp }}/lint-counts/ + if-no-files-found: error diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index 9f1283da19e..6dd6f57ac83 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -19,9 +19,9 @@ jobs: lint: runs-on: ubuntu-latest timeout-minutes: 15 - # actions: read lets scripts/type_check_gate.py download the base-counts - # artifact published by publish-basedpyright-base-counts.yml instead of - # re-running basedpyright over the merge-base tree. + # actions: read lets the four lint gates download the base-counts artifacts + # published by publish-lint-base-counts.yml instead of re-scanning the + # merge-base tree in a throwaway worktree. permissions: contents: read pull-requests: read @@ -142,18 +142,24 @@ jobs: run: | uv run --no-sync ruff check --config ruff-tests.toml tests - - name: Check strict-rule budget (delta vs base) + - name: Check strict ruff rules (delta vs merge-base counts) if: steps.changes.outputs.decision != 'skip' + env: + GH_TOKEN: ${{ github.token }} run: | uv run --no-sync python scripts/ruff_strict_gate.py --base "$GATE_BASE_SHA" - - name: Check type-discipline budget (mutable collections / casts / type guards / kwargs / unexplained suppressions, delta vs base) + - name: Check type discipline (mutable collections / casts / type guards / kwargs / unexplained suppressions, delta vs merge-base counts) if: steps.changes.outputs.decision != 'skip' + env: + GH_TOKEN: ${{ github.token }} run: | uv run --no-sync python scripts/type_discipline_gate.py --base "$GATE_BASE_SHA" - - name: Check test-quality budget (zero-assert / mock-echo tests, sys.path.insert, raw env writes, litellm global mutation, credential-gated skips, conftest snapshot inventory, delta vs base) + - name: Check test quality (zero-assert / mock-echo tests, sys.path.insert, raw env writes, litellm global mutation, credential-gated skips, conftest snapshot inventory, delta vs merge-base counts) if: steps.changes.outputs.decision != 'skip' + env: + GH_TOKEN: ${{ github.token }} run: | uv run --no-sync python scripts/test_quality_gate.py --base "$GATE_BASE_SHA" @@ -162,7 +168,7 @@ jobs: run: | uv run --no-sync python -c "import openai; print(f'OpenAI version: {openai.__version__}')" - - name: Check basedpyright budget (delta vs base) + - name: Check basedpyright (delta vs merge-base counts) if: steps.changes.outputs.decision != 'skip' env: GH_TOKEN: ${{ github.token }} @@ -190,40 +196,6 @@ jobs: run: | uv run --no-sync python -c "from litellm import *" || (echo '🚨 import failed, this means you introduced unprotected imports! 🚨'; exit 1) - # Intentionally NON-GATING. This job turns red when a *-budget.json ceiling is - # raised (or a rule/budget is dropped) so a loosening is obvious in review, but it - # must be kept OUT of the branch-protection required-checks list so a justified - # bump can still be merged by a human who has seen and accepted the red. - budget-ratchet: - runs-on: ubuntu-latest - timeout-minutes: 5 - permissions: - contents: read - - steps: - - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 - with: - fetch-depth: 1 - persist-credentials: false - - - name: Fetch ratchet base - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - run: | - retry() { "$@" || { sleep 15; "$@"; } || { sleep 30; "$@"; }; } - retry git fetch --no-tags --depth=1 origin "$BASE_SHA" - - - name: Set up Python - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 - with: - python-version: "3.12" - - - name: Ratchet check (budgets may only decrease; non-gating) - env: - BASE_SHA: ${{ github.event.pull_request.base.sha }} - run: | - python scripts/budget_ratchet_check.py --base "$BASE_SHA" - secret-scan: runs-on: ubuntu-latest timeout-minutes: 5 diff --git a/CLAUDE.md b/CLAUDE.md index 41678432989..de8a0022c77 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,11 +52,11 @@ Don't hesitate to use values in .env to get needed API keys and other secrets, a Python max line length is 120, not 88 -Never edit or commit `ruff-strict-budget.json`, `type-discipline-budget.json`, `basedpyright-code-budget.json`, or `test-quality-budget.json` on a PR branch, and don't run `make lint-budget-update` there. A scheduled Devin automation lowers the limits on the default branch in its own PR by exactly what landed since the last ratchet, so concurrent PRs don't fight over the same `"limit"` lines. Keep the hosted automation's target in sync when the repository default changes. If your branch already carries a budget edit, drop it before opening the PR +The four lint gates (`scripts/ruff_strict_gate.py`, `scripts/type_discipline_gate.py`, `scripts/test_quality_gate.py`, `scripts/type_check_gate.py`) compare each rule's codebase count on your branch against the count at the merge-base with `litellm_internal_staging`, plus the small per-rule headroom listed in each gate script's `HEADROOM`. There are no budget files to edit or ratchet: when a gate fails, fix the violations the branch introduced or remove at least as many of that rule elsewhere in the tree `make check` (f.k.a. `make pre-commit`, which still works identically as an alias) saves its complete output to a log file in .git (overwriting previous 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 -`make check`, `make lint`, `scripts/pre_commit_lint.sh`, and the standalone budget gates (`scripts/ruff_strict_gate.py`, `scripts/type_discipline_gate.py`, `scripts/type_check_gate.py`) each hold one of 2 machine-wide slots, so when other sessions or worktrees on the same box are already running heavy work, yours prints "all N machine-wide slots are busy; queueing" and then stays quiet until a slot frees. Give the command a long timeout and let it wait rather than killing it, retrying it, or assuming it hung. Don't change the # of machine-wide slots or make it unlimited by setting `LITELLM_GATE_SLOTS=0` +`make check`, `make lint`, `scripts/pre_commit_lint.sh`, and the standalone lint gates (`scripts/ruff_strict_gate.py`, `scripts/type_discipline_gate.py`, `scripts/type_check_gate.py`) each hold one of 2 machine-wide slots, so when other sessions or worktrees on the same box are already running heavy work, yours prints "all N machine-wide slots are busy; queueing" and then stays quiet until a slot frees. Give the command a long timeout and let it wait rather than killing it, retrying it, or assuming it hung. Don't change the # of machine-wide slots or make it unlimited by setting `LITELLM_GATE_SLOTS=0` 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/Makefile b/Makefile index ab11220821f..43ca8fec84a 100644 --- a/Makefile +++ b/Makefile @@ -5,9 +5,8 @@ test-unit-integrations test-unit-core-utils test-unit-other test-unit-root \ test-proxy-unit-a test-proxy-unit-b test-integration test-unit-helm \ info lint lint-inner lint-dev lint-checks format \ - lint-basedpyright lint-e2e-basedpyright lint-basedpyright-budget-update lint-type-discipline lint-type-discipline-budget-update \ - lint-ruff-budget lint-ruff-budget-update lint-budget-update lint-gate \ - lint-test-quality lint-test-quality-budget-update \ + lint-basedpyright lint-e2e-basedpyright lint-type-discipline \ + lint-ruff-strict lint-gate lint-test-quality \ install-dev install-proxy-dev install-test-deps install-hooks \ install-helm-unittest check-circular-imports check-import-safety check check-inner pre-commit \ lint-install lint-fetch-base bootstrap @@ -31,13 +30,10 @@ help: @echo " make lint-ruff - Run Ruff linting only" @echo " make lint-basedpyright - Run basedpyright strict, gated by per-rule error counts" @echo " make lint-e2e-basedpyright - Run basedpyright over tests/e2e (zero errors allowed)" - @echo " make lint-basedpyright-budget-update - Ratchet basedpyright limits down by what this branch fixed" @echo " make lint-format - Check ruff format formatting (matches CI)" - @echo " make lint-ruff-budget - Gate the codebase total of each strict ruff rule against its limit" + @echo " make lint-ruff-strict - Gate each strict ruff rule's codebase total against its merge-base count" @echo " make lint-gate - Strict ruff gate in CI-parity mode (fetches the default branch, simulates the merge)" - @echo " make lint-ruff-budget-update - Ratchet ruff-strict-budget.json limits down by what this branch fixed" - @echo " make lint-test-quality - Gate the test suite against test-quality-budget.json" - @echo " make lint-budget-update - Ratchet all budgets down (ruff + type-discipline + test quality + basedpyright)" + @echo " make lint-test-quality - Gate the test suite's TQ counts against their merge-base counts" @echo " make check-circular-imports - Check for circular imports" @echo " make check-import-safety - Check import safety" @echo " make test - Run all tests" @@ -138,7 +134,7 @@ lint-fetch-base: # Mirror test-linting.yml's lint job environment: the proxy-dev group plus a generated # Prisma client, so `basedpyright tests/e2e` resolves the same modules CI does. The -# budget gate itself no longer measures here (scripts/type_check_gate.py provisions its +# basedpyright gate itself no longer measures here (scripts/type_check_gate.py provisions its # own .venv-typecheck). --inexact tops up the venv instead of pruning the proxy extras # gen:api and the running proxy need. lint-install: @@ -207,25 +203,20 @@ lint-basedpyright: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) lint-e2e-basedpyright: $(LINT_E2E_DEP_INSTALL) $(UV_RUN) basedpyright tests/e2e -# Type-discipline budget (mutable collections / casts / type guards / kwargs / +# Type-discipline gate (mutable collections / casts / type guards / kwargs / # unexplained suppressions), the test-linting.yml step `make lint` used to omit. lint-type-discipline: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/type_discipline_gate.py --base "$(BASE_REF)" -# Test-quality budget (zero-assert / mock-echo tests, sys.path.insert, raw env writes, +# Test-quality gate (zero-assert / mock-echo tests, sys.path.insert, raw env writes, # litellm module-global mutation, credential-gated skips, conftest snapshot # inventory), counted across tests/ the same delta-vs-base way. lint-test-quality: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/test_quality_gate.py --base "$(BASE_REF)" -# --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 - $(UV_RUN) python scripts/type_check_gate.py --update --base "$(BASE_REF)" - lint-format: format-check -lint-ruff-budget: install-dev +lint-ruff-strict: install-dev $(UV_RUN) python scripts/ruff_strict_gate.py --base "$(BASE_REF)" # Strict gate, invoked the same way CI does in test-linting.yml so a local pass @@ -233,18 +224,6 @@ lint-ruff-budget: install-dev lint-gate: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/ruff_strict_gate.py --base "$(BASE_REF)" -lint-ruff-budget-update: install-dev - $(UV_RUN) python scripts/ruff_strict_gate.py --update --base "$(BASE_REF)" - -lint-type-discipline-budget-update: install-dev - $(UV_RUN) python scripts/type_discipline_gate.py --update --base "$(BASE_REF)" - -lint-test-quality-budget-update: install-dev - $(UV_RUN) python scripts/test_quality_gate.py --update --base "$(BASE_REF)" - -# Ratchet all budgets in one shot (ruff strict + type-discipline + test quality + basedpyright) -lint-budget-update: lint-ruff-budget-update lint-type-discipline-budget-update lint-test-quality-budget-update lint-basedpyright-budget-update - check-circular-imports: $(LINT_DEP_INSTALL) cd litellm && $(UV_RUN) python ../tests/documentation_tests/test_circular_imports.py && cd .. @@ -254,7 +233,7 @@ check-import-safety: $(LINT_DEP_INSTALL) # Combined linting, isomorphic to test-linting.yml's lint job so a local pass means a # green CI lint: it installs the same env (proxy-dev + generated Prisma client) and then # runs the diff-scoped ruff format check, whole-tree ruff check, the strict-rule / -# type-discipline / basedpyright budgets as a delta vs the base, then the circular-import +# type-discipline / basedpyright gates as a delta vs the base, then the circular-import # and import-safety checks. Steps that compare against the base resolve it the same way CI # does (merge-base with origin's current default branch). Setup (env sync, Prisma client, # base fetch) runs once up front; the checks themselves are independent, so a sub-make diff --git a/basedpyright-code-budget.json b/basedpyright-code-budget.json deleted file mode 100644 index 57ca267e504..00000000000 --- a/basedpyright-code-budget.json +++ /dev/null @@ -1,146 +0,0 @@ -{ - "reportAny": { - "limit": 13429 - }, - "reportArgumentType": { - "limit": 2198 - }, - "reportAssignmentType": { - "limit": 319 - }, - "reportAttributeAccessIssue": { - "limit": 480 - }, - "reportCallIssue": { - "limit": 112 - }, - "reportConstantRedefinition": { - "limit": 40 - }, - "reportDeprecated": { - "limit": 209 - }, - "reportDuplicateImport": { - "limit": 19 - }, - "reportExplicitAny": { - "limit": 3369 - }, - "reportFunctionMemberAccess": { - "limit": 7 - }, - "reportGeneralTypeIssues": { - "limit": 101 - }, - "reportIncompatibleMethodOverride": { - "limit": 56 - }, - "reportIncompatibleVariableOverride": { - "limit": 8 - }, - "reportInconsistentOverload": { - "limit": 12 - }, - "reportIndexIssue": { - "limit": 24 - }, - "reportInvalidTypeForm": { - "limit": 30 - }, - "reportInvalidTypeVarUse": { - "limit": 1 - }, - "reportMatchNotExhaustive": { - "limit": 0 - }, - "reportMissingParameterType": { - "limit": 5570 - }, - "reportMissingTypeArgument": { - "limit": 15281 - }, - "reportMissingTypeStubs": { - "limit": 40 - }, - "reportOperatorIssue": { - "limit": 0 - }, - "reportOptionalCall": { - "limit": 0 - }, - "reportOptionalIterable": { - "limit": 0 - }, - "reportOptionalMemberAccess": { - "limit": 0 - }, - "reportOptionalOperand": { - "limit": 0 - }, - "reportOptionalSubscript": { - "limit": 0 - }, - "reportPossiblyUnboundVariable": { - "limit": 56 - }, - "reportPrivateUsage": { - "limit": 1804 - }, - "reportRedeclaration": { - "limit": 8 - }, - "reportReturnType": { - "limit": 180 - }, - "reportTypedDictNotRequiredAccess": { - "limit": 22 - }, - "reportUndefinedVariable": { - "limit": 0 - }, - "reportUnknownArgumentType": { - "limit": 44358 - }, - "reportUnknownLambdaType": { - "limit": 109 - }, - "reportUnknownMemberType": { - "limit": 38271 - }, - "reportUnknownParameterType": { - "limit": 19584 - }, - "reportUnknownVariableType": { - "limit": 29814 - }, - "reportUnnecessaryCast": { - "limit": 110 - }, - "reportUnnecessaryComparison": { - "limit": 687 - }, - "reportUnnecessaryContains": { - "limit": 4 - }, - "reportUnnecessaryIsInstance": { - "limit": 816 - }, - "reportUntypedBaseClass": { - "limit": 0 - }, - "reportUntypedFunctionDecorator": { - "limit": 27 - }, - "reportUnusedClass": { - "limit": 21 - }, - "reportUnusedFunction": { - "limit": 136 - }, - "reportUnusedImport": { - "limit": 542 - }, - "reportUnusedVariable": { - "limit": 137 - } -} diff --git a/pyproject.toml b/pyproject.toml index f4f238dd4b9..b23429941d9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -334,6 +334,7 @@ version_files = [ ] [tool.pytest.ini_options] +pythonpath = ["scripts"] asyncio_mode = "auto" asyncio_default_fixture_loop_scope = "session" markers = [ diff --git a/ruff-strict-budget.json b/ruff-strict-budget.json deleted file mode 100644 index fd7b30bc314..00000000000 --- a/ruff-strict-budget.json +++ /dev/null @@ -1,260 +0,0 @@ -{ - "ANN001": { - "limit": 2956 - }, - "ANN002": { - "limit": 71 - }, - "ANN003": { - "limit": 806 - }, - "ANN201": { - "limit": 1979 - }, - "ANN202": { - "limit": 831 - }, - "ANN204": { - "limit": 683 - }, - "ANN205": { - "limit": 112 - }, - "ANN206": { - "limit": 133 - }, - "ANN401": { - "limit": 119 - }, - "ASYNC230": { - "limit": 11 - }, - "B004": { - "limit": 2 - }, - "B006": { - "limit": 176 - }, - "B008": { - "limit": 503 - }, - "B009": { - "limit": 52 - }, - "B010": { - "limit": 187 - }, - "B018": { - "limit": 2 - }, - "B019": { - "limit": 1 - }, - "B021": { - "limit": 1 - }, - "B026": { - "limit": 3 - }, - "BLE001": { - "limit": 2916 - }, - "C401": { - "limit": 8 - }, - "C404": { - "limit": 1 - }, - "C405": { - "limit": 19 - }, - "C408": { - "limit": 11 - }, - "C414": { - "limit": 4 - }, - "C419": { - "limit": 1 - }, - "C901": { - "limit": 306 - }, - "D419": { - "limit": 6 - }, - "DTZ001": { - "limit": 2 - }, - "DTZ003": { - "limit": 24 - }, - "DTZ005": { - "limit": 233 - }, - "DTZ006": { - "limit": 10 - }, - "DTZ007": { - "limit": 6 - }, - "DTZ011": { - "limit": 3 - }, - "EXE001": { - "limit": 4 - }, - "EXE002": { - "limit": 3 - }, - "F401": { - "limit": 12 - }, - "LOG015": { - "limit": 5 - }, - "N999": { - "limit": 1 - }, - "PERF102": { - "limit": 21 - }, - "PERF401": { - "limit": 12 - }, - "PERF403": { - "limit": 33 - }, - "PIE804": { - "limit": 18 - }, - "PIE810": { - "limit": 43 - }, - "PLC0206": { - "limit": 26 - }, - "PLC0414": { - "limit": 46 - }, - "PLR0124": { - "limit": 1 - }, - "PLR0206": { - "limit": 1 - }, - "PLR1704": { - "limit": 1 - }, - "PLR1714": { - "limit": 253 - }, - "PLW0127": { - "limit": 57 - }, - "PLW0602": { - "limit": 215 - }, - "PLW0603": { - "limit": 190 - }, - "PLW1508": { - "limit": 190 - }, - "PLW1510": { - "limit": 2 - }, - "PYI036": { - "limit": 3 - }, - "RET504": { - "limit": 173 - }, - "RUF012": { - "limit": 239 - }, - "RUF015": { - "limit": 8 - }, - "RUF019": { - "limit": 27 - }, - "RUF046": { - "limit": 4 - }, - "RUF059": { - "limit": 66 - }, - "RUF100": { - "limit": 0 - }, - "S110": { - "limit": 217 - }, - "S112": { - "limit": 22 - }, - "SIM101": { - "limit": 56 - }, - "SIM102": { - "limit": 310 - }, - "SIM103": { - "limit": 119 - }, - "SIM113": { - "limit": 3 - }, - "SIM115": { - "limit": 2 - }, - "SIM117": { - "limit": 6 - }, - "SIM201": { - "limit": 1 - }, - "SIM210": { - "limit": 8 - }, - "SIM211": { - "limit": 1 - }, - "SIM222": { - "limit": 1 - }, - "SIM401": { - "limit": 11 - }, - "TC004": { - "limit": 5 - }, - "TID251": { - "limit": 1035 - }, - "TRY002": { - "limit": 524 - }, - "TRY004": { - "limit": 96 - }, - "TRY201": { - "limit": 401 - }, - "TRY203": { - "limit": 109 - }, - "TRY300": { - "limit": 852 - }, - "UP028": { - "limit": 2 - }, - "UP031": { - "limit": 2 - }, - "UP036": { - "limit": 1 - } -} diff --git a/ruff.toml b/ruff.toml index 3ac4c1fc94d..e180e0a1af6 100644 --- a/ruff.toml +++ b/ruff.toml @@ -1,6 +1,6 @@ lint.ignore = ["F405", "E402", "F403"] # The second group is the strict gate's graduates: rules the codebase already has zero -# violations of, so they hard-fail here instead of being ratcheted in ruff-strict-budget.json. +# violations of, so they hard-fail here instead of being counted by the strict gate. # That gives editors and `ruff check --fix` the diagnostic, which the gate script cannot. lint.extend-select = [ "T20", "PGH004", "RUF008", "RUF009", "RUF100", diff --git a/scripts/budget_ratchet_check.py b/scripts/budget_ratchet_check.py deleted file mode 100644 index 485e118efd2..00000000000 --- a/scripts/budget_ratchet_check.py +++ /dev/null @@ -1,233 +0,0 @@ -#!/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 -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: - - * a rule's `limit` went up, - * a rule was dropped from a budget (its ceiling effectively became infinite), or - * an entire budget file was deleted. - -New rules and lowered/equal limits are fine. So is a rule that graduated: once a -paired config (ruff.toml for the ruff-strict budget) selects the rule outright it -hard-fails at the first violation, which is stricter than any ceiling the budget -could hold, so dropping its entry tightens the guard rather than removing it. - -This is deliberately NOT a gating check. It should turn the run red so that a -loosening is impossible to miss in review, but it must stay OUT of the -branch-protection required-checks list: a justified bump (e.g. banning a new API, -which mechanically raises a baseline) can then still be merged by a human who has -seen the red and accepted it. - -Usage: - python scripts/budget_ratchet_check.py [--base REF] [budget.json ...] - -""" - -from __future__ import annotations - -import argparse -import json -import subprocess -import sys -from pathlib import Path -from types import MappingProxyType -from typing import Final, NamedTuple - -if sys.version_info >= (3, 11): - import tomllib -else: - import tomli as tomllib - -REPO_ROOT = Path(__file__).resolve().parent.parent -DEFAULT_BUDGETS: tuple[str, ...] = ( - "ruff-strict-budget.json", - "type-discipline-budget.json", - "basedpyright-code-budget.json", - "test-quality-budget.json", -) -GRADUATION_CONFIGS = MappingProxyType({"ruff-strict-budget.json": "ruff.toml"}) - - -class Regression(NamedTuple): - budget: str - rule: str - detail: str - - -def _run(cmd: list[str]) -> subprocess.CompletedProcess[str]: - return subprocess.run(cmd, cwd=REPO_ROOT, capture_output=True, text=True) - - -def _merge_base(base: str) -> str: - """The common ancestor of `base` and HEAD, so unrelated base drift is ignored.""" - proc = _run(["git", "merge-base", base, "HEAD"]) - return proc.stdout.strip() or base - - -def _load_head(rel: str) -> dict | None: - path = REPO_ROOT / rel - if not path.exists(): - return None - return json.loads(path.read_text()) - - -def _ref_is_commit(ref: str) -> bool: - return ( - _run( - ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"] - ).returncode - == 0 - ) - - -def _load_base(rel: str, ref: str) -> dict | None: - """Budget content at `ref`, or None when the file did not exist there. - - `ref` is verified as a real commit by the caller, so a non-zero `git show` here means - the path was absent at that commit, not that the ref itself is unresolvable. - """ - proc = _run(["git", "show", f"{ref}:{rel}"]) - if proc.returncode != 0: - return None - return json.loads(proc.stdout) - - -def _ceiling(spec: dict) -> int: - """A rule's ceiling: its `limit`, or legacy `baseline + slack`. - - The base side of the diff can predate the `limit` migration, so a spec is read - under either schema and the two are compared on the same footing. - """ - if "limit" in spec: - return int(spec["limit"]) - return int(spec.get("baseline", 0)) + int(spec.get("slack", 0)) - - -def _limits(budget: dict) -> dict[str, int]: - """Map each rule to its ceiling; skip malformed specs.""" - return { - rule: _ceiling(spec) - for rule, spec in budget.items() - if isinstance(spec, dict) - } - - -def selectors_hard_failed_by(lint: dict) -> tuple[str, ...]: - """A ruff `[lint]` table's selected codes, minus anything `ignore` turns back off. - - `lint.ignore` wins over `lint.extend-select` in ruff, so an ignored code is not - actually enforced and must not count as a graduation. - """ - ignored = tuple(lint.get("ignore", ())) - return tuple( - selector - for selector in lint.get("extend-select", ()) - if not (ignored and selector.startswith(ignored)) - ) - - -def graduated_selectors(rel: str) -> tuple[str, ...]: - """Selectors the budget's paired ruff config hard-fails, so its ceiling is moot.""" - config = GRADUATION_CONFIGS.get(rel) - if config is None or not (REPO_ROOT / config).exists(): - return () - return selectors_hard_failed_by( - tomllib.loads((REPO_ROOT / config).read_text()).get("lint", {}) - ) - - -def _regression_detail( - rule: str, - base_limits: dict[str, int], - head_limits: dict[str, int], - graduated: tuple[str, ...], -) -> str | None: - """Why `rule` regressed vs base, or None when it held flat, fell, or graduated. - - A dropped rule is terminal unless it graduated; otherwise the only loosening - left is a raised limit. - """ - base_limit = base_limits[rule] - if rule not in head_limits: - if graduated and rule.startswith(graduated): - return None - return f"rule dropped (limit {base_limit} -> removed)" - if head_limits[rule] > base_limit: - return f"limit raised {base_limit} -> {head_limits[rule]}" - return None - - -def regressions_for( - rel: str, - base: dict | None, - head: dict | None, - graduated: tuple[str, ...] = (), -) -> list[Regression]: - if base is None: - return [] # new budget file: nothing to ratchet against yet - if head is None: - return [Regression(rel, "*", "budget file was deleted (every limit removed)")] - - base_limits, head_limits = _limits(base), _limits(head) - return [ - Regression(rel, rule, detail) - for rule in sorted(base_limits) - if (detail := _regression_detail(rule, base_limits, head_limits, graduated)) is not None - ] - - -def main() -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--base", help="Comparison ref (default: origin's current default branch)") - parser.add_argument("budgets", nargs="*", help="budget files to check") - args = parser.parse_args() - from default_branch import resolve_base_ref - - base_ref: Final = resolve_base_ref(args.base, REPO_ROOT) - budgets = args.budgets or list(DEFAULT_BUDGETS) - - ref = _merge_base(base_ref) - if not _ref_is_commit(ref): - print( - f"FAIL: base ref {ref!r} does not resolve to a commit, so the ratchet has nothing " - f"to compare against; refusing to pass vacuously (check the --base / BASE_SHA value)", - file=sys.stderr, - ) - return 1 - - regressions: list[Regression] = [] - checked: list[str] = [] - for rel in budgets: - base = _load_base(rel, ref) - head = _load_head(rel) - if base is None and head is None: - continue - if base is None: - print(f"skip {rel}: new file (no base at {base_ref} to ratchet against)") - continue - checked.append(rel) - regressions.extend(regressions_for(rel, base, head, graduated_selectors(rel))) - - if regressions: - print( - f"FAIL: budget limit(s) loosened vs base {base_ref} (merge-base {ref[:12]}):" - ) - for reg in regressions: - print(f" {reg.budget} {reg.rule}: {reg.detail}") - print( - "Budgets are one-way ratchets and may only go down or stay flat. This " - "check is non-gating: if the increase is justified (e.g. a newly banned " - "API), a human can merge over the red after acknowledging it." - ) - return 1 - - suffix = f" ({', '.join(checked)})" if checked else "" - print(f"OK: no budget limit increased vs base {base_ref}{suffix}") - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/scripts/check_test_quality.py b/scripts/check_test_quality.py index 41342acd23a..885b6d10bfc 100644 --- a/scripts/check_test_quality.py +++ b/scripts/check_test_quality.py @@ -4,8 +4,8 @@ Sibling of scripts/check_type_discipline.py, same output contract (``path:line: CODE message``) and same stdlib-only constraint, aimed at the test tree instead of the package. Each rule is a shape the testing-strategy audit -measured and named; scripts/test_quality_gate.py caps the codebase total of each -one against test-quality-budget.json so the counts can only ratchet down. +measured and named; scripts/test_quality_gate.py fails any change that grows the +codebase total of one past its merge-base count. Rules ----- diff --git a/scripts/gate_slot_lock.py b/scripts/gate_slot_lock.py index 999b28ced1c..c1ab00461e0 100644 --- a/scripts/gate_slot_lock.py +++ b/scripts/gate_slot_lock.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 """Machine-wide slot lock for this repo's heavy entrypoints. -`make check`, `make lint`, and the standalone budget gates +`make check`, `make lint`, and the standalone lint gates (scripts/ruff_strict_gate.py, scripts/type_discipline_gate.py, scripts/type_check_gate.py) each hold one of N machine-wide slots while they run, so however many sessions and worktrees share one machine, at most N of diff --git a/scripts/lint_base_counts.py b/scripts/lint_base_counts.py new file mode 100644 index 00000000000..9371200f8ce --- /dev/null +++ b/scripts/lint_base_counts.py @@ -0,0 +1,313 @@ +#!/usr/bin/env python3 +"""Merge-base counts for the delta-vs-base lint gates. + +Each gate (scripts/ruff_strict_gate.py, scripts/type_discipline_gate.py, +scripts/type_check_gate.py, scripts/test_quality_gate.py) counts its rules +across the whole tree at HEAD and at the merge-base with the branch the change +merges into, and fails only when a rule grew past the merge-base count plus +that rule's fixed headroom. There is no committed budget: the merge-base count +is the ceiling, so it moves only when the base branch does. + +The merge-base counts come from, in order, the disk cache under the git common +dir, the CI artifact publish-lint-base-counts.yml uploads for every push to the +default branch, and a scan of the base tree in a temporary worktree. Every +entry is keyed by the merge-base commit plus the checker's fingerprints (its +config, its rule logic, its tool version), so counts measured under a different +rule set are never matched, only recomputed. +""" + +from __future__ import annotations + +import hashlib +import io +import json +import os +import re +import subprocess +import sys +import zipfile +from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import Final, NamedTuple, TypeAlias + +REPO_ROOT: Final = Path(__file__).resolve().parents[1] +CACHE_DIR_NAME: Final = "litellm-lint-cache" +CACHE_KEEP_ENTRIES: Final = 8 +GH_TIMEOUT_SECONDS: Final = 10 + +_ORIGIN_SLUG: Final = re.compile(r"(?:git@github\.com:|https://github\.com/)([^/]+/[^/]+?)(?:\.git)?/?") + +Counts: TypeAlias = Mapping[str, int] +GhOutput: TypeAlias = Callable[[Sequence[str]], bytes | None] + + +class Breach(NamedTuple): + rule: str + total: int + cap: int + added: int + + +@dataclass(frozen=True, slots=True) +class Checker: + name: str + fingerprints: tuple[str, ...] + + def key(self, base_point: str) -> str: + return cache_key(base_point, self.fingerprints) + + def artifact_name(self, base_point: str) -> str: + return f"{self.name}-counts-{self.key(base_point)}" + + def cache_file_name(self, base_point: str) -> str: + return f"{self.name}-base-{self.key(base_point)}.json" + + def cache_glob(self) -> str: + return f"{self.name}-base-*.json" + + +Fetch: TypeAlias = Callable[[Checker, str], Counts | None] + + +def sha256_of(path: Path) -> str: + return hashlib.sha256(path.read_bytes()).hexdigest() + + +def cache_key(base_point: str, fingerprints: Sequence[str]) -> str: + return hashlib.sha256("|".join((base_point, *fingerprints)).encode()).hexdigest()[:16] + + +def _git(args: Sequence[str], cwd: Path) -> str: + proc: Final = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + if proc.returncode not in (0, 1): + sys.stderr.write(proc.stderr) + raise SystemExit(f"git exited {proc.returncode}") + return proc.stdout + + +def head_sha(cwd: Path = REPO_ROOT) -> str: + return _git(["rev-parse", "HEAD"], cwd).strip() + + +def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: + """The snapshot commit base counts are measured at: merge-base(base_ref, HEAD), + made aware of an in-progress merge. Mid-merge, HEAD is still the pre-merge tip, + so its merge-base is the old branch point and every violation the base gained + since then would be blamed on this change. While MERGE_HEAD exists, prefer + merge-base(base_ref, MERGE_HEAD) whenever it is the newer of the two.""" + head_point: Final = _git(["merge-base", base_ref, "HEAD"], cwd).strip() + if not head_point: + return base_ref + merge_head: Final = _git(["rev-parse", "--verify", "--quiet", "MERGE_HEAD"], cwd).strip() + if not merge_head: + return head_point + merge_point: Final = _git(["merge-base", base_ref, merge_head], cwd).strip() + if not merge_point: + return head_point + older: Final = _git(["merge-base", head_point, merge_point], cwd).strip() + return merge_point if older == head_point else head_point + + +def default_cache_dir(cwd: Path = REPO_ROOT) -> Path: + common: Final = Path(_git(["rev-parse", "--git-common-dir"], cwd).strip()) + resolved: Final = common if common.is_absolute() else cwd / common + return resolved / CACHE_DIR_NAME + + +def validated_counts(data: object) -> Counts | None: + counts: Final = data.get("counts") if isinstance(data, dict) else None + if not isinstance(counts, dict): + return None + if not all( + isinstance(code, str) and isinstance(total, int) and not isinstance(total, bool) + for code, total in counts.items() + ): + return None + return counts + + +def load_cached_counts(path: Path) -> Counts | None: + try: + data: Final = json.loads(path.read_text()) + except (OSError, json.JSONDecodeError): + return None + return validated_counts(data) + + +def scratch_path(path: Path) -> Path: + return path.with_name(f".{path.name}.{os.getpid()}.tmp") + + +def counts_payload(base_point: str, counts: Counts) -> str: + return json.dumps({"base_point": base_point, "counts": dict(sorted(counts.items()))}, indent=2) + "\n" + + +def entry_recency(path: Path) -> float: + try: + return path.stat().st_mtime + except OSError: + return 0.0 + + +def evicted_beyond_cap(entries: Sequence[Path], keep: int) -> tuple[Path, ...]: + newest_first: Final = sorted(entries, key=entry_recency, reverse=True) + return tuple(newest_first[keep:]) + + +def store_counts(directory: Path, checker: Checker, base_point: str, counts: Counts) -> Path: + directory.mkdir(parents=True, exist_ok=True) + path: Final = directory / checker.cache_file_name(base_point) + scratch: Final = scratch_path(path) + scratch.write_text(counts_payload(base_point, counts)) + scratch.replace(path) + siblings: Final = tuple(entry for entry in directory.glob(checker.cache_glob()) if entry != path) + for stale in evicted_beyond_cap(siblings, CACHE_KEEP_ENTRIES - 1): + stale.unlink(missing_ok=True) + return path + + +def parse_origin_slug(url: str) -> str | None: + match: Final = _ORIGIN_SLUG.fullmatch(url.strip()) + return match.group(1) if match else None + + +def origin_slug(cwd: Path = REPO_ROOT) -> str | None: + proc: Final = subprocess.run(["git", "remote", "get-url", "origin"], cwd=cwd, capture_output=True, text=True) + return parse_origin_slug(proc.stdout) if proc.returncode == 0 else None + + +def gh_output(args: Sequence[str]) -> bytes | None: + try: + proc: Final = subprocess.run(["gh", *args], capture_output=True, timeout=GH_TIMEOUT_SECONDS) + except (OSError, subprocess.SubprocessError): + return None + return proc.stdout if proc.returncode == 0 else None + + +def _parsed_json(raw: bytes) -> object | None: + try: + return json.loads(raw) + except ValueError: + return None + + +def _artifact_download_url(listing: object) -> str | None: + artifacts: Final = listing.get("artifacts") if isinstance(listing, dict) else None + if not isinstance(artifacts, list) or not artifacts: + return None + newest: Final = artifacts[0] + if not isinstance(newest, dict) or newest.get("expired"): + return None + url: Final = newest.get("archive_download_url") + return url if isinstance(url, str) else None + + +def _counts_json_from_zip(zip_bytes: bytes) -> object | None: + try: + with zipfile.ZipFile(io.BytesIO(zip_bytes)) as archive: + members: Final = tuple(name for name in archive.namelist() if name.endswith(".json")) + if len(members) != 1: + return None + return json.loads(archive.read(members[0])) + except (zipfile.BadZipFile, ValueError, OSError): + return None + + +def counts_for_base(payload: object, base_point: str) -> Counts | None: + if not isinstance(payload, dict) or payload.get("base_point") != base_point: + return None + counts: Final = validated_counts(payload) + return counts if counts else None + + +def _fetch_fallback(reason: str) -> None: + sys.stderr.write(f"{reason}; computing base counts locally\n") + + +def fetch_ci_base_counts( + checker: Checker, + base_point: str, + gh: GhOutput = gh_output, + cwd: Path = REPO_ROOT, +) -> Counts | None: + """Base counts from the CI artifact published for `base_point`, or None. + + Every failure mode (no gh, no auth, offline, expired or missing artifact, + malformed payload, counts for a different commit) returns None so the + caller falls back to the local base scan; the fetch is an optimization and + must never make the gate less available than local compute alone.""" + slug: Final = origin_slug(cwd) + if slug is None: + return _fetch_fallback("origin remote is not a github.com URL") + name: Final = checker.artifact_name(base_point) + listing: Final = gh(["api", f"repos/{slug}/actions/artifacts?name={name}&per_page=1"]) + if listing is None: + return _fetch_fallback(f"could not list CI artifacts named {name}") + url: Final = _artifact_download_url(_parsed_json(listing)) + if url is None: + return _fetch_fallback(f"no usable CI artifact named {name}") + zip_bytes: Final = gh(["api", url]) + if zip_bytes is None: + return _fetch_fallback(f"download failed for CI artifact {name}") + counts: Final = counts_for_base(_counts_json_from_zip(zip_bytes), base_point) + if counts is None: + return _fetch_fallback(f"CI artifact {name} is not valid base counts for {base_point[:12]}") + sys.stderr.write(f"base counts fetched from CI artifact {name}\n") + return counts + + +def base_counts_cached( + checker: Checker, + base_point: str, + compute: Callable[[str], Counts], + cache_dir: Path | None = None, + fetch: Fetch = fetch_ci_base_counts, +) -> Counts: + """`compute` memoized on disk. The base tree at a given commit is immutable, + so its counts are a pure function of the merge-base plus the checker's + fingerprints in the cache key; an empty result is never stored because it is + the signature of a crashed pass, not a clean tree. On a disk miss the counts + CI already published for the merge-base are fetched before the expensive + local base scan; a fetch miss of any kind computes locally.""" + directory: Final = default_cache_dir() if cache_dir is None else cache_dir + cached: Final = load_cached_counts(directory / checker.cache_file_name(base_point)) + if cached is not None: + return cached + fetched: Final = fetch(checker, base_point) + if fetched: + store_counts(directory, checker, base_point, fetched) + return fetched + counts: Final = compute(base_point) + if counts: + store_counts(directory, checker, base_point, counts) + return counts + + +def emit_counts(checker: Checker, counts: Counts, directory: Path, head_point: str) -> Path: + """Write HEAD's per-rule counts as the file the publisher workflow uploads. + + The filename stem is exactly the artifact name `fetch_ci_base_counts` will + later look up for this commit, so emit and fetch cannot drift apart. Empty + counts are refused: a pass that produced nothing almost certainly crashed, + and publishing it would poison every branch that fetches it.""" + if not counts: + print( + f"FAIL: {checker.name} produced no violations; refusing to publish empty base " + "counts because the pass almost certainly crashed or emitted nothing." + ) + raise SystemExit(1) + name: Final = checker.artifact_name(head_point) + directory.mkdir(parents=True, exist_ok=True) + path: Final = directory / f"{name}.json" + path.write_text(counts_payload(head_point, counts)) + print(f"Emitted base counts for {head_point} as {name}.json ({sum(counts.values())} violations total)") + return path + + +def evaluate(head: Counts, base: Counts, headroom: Counts) -> tuple[Breach, ...]: + return tuple( + Breach(rule, total, base.get(rule, 0) + headroom.get(rule, 0), total - base.get(rule, 0)) + for rule, total in sorted(head.items()) + if total > base.get(rule, 0) + headroom.get(rule, 0) + ) diff --git a/scripts/pre_commit_lint.sh b/scripts/pre_commit_lint.sh index 1abd415d237..808c8096cfb 100755 --- a/scripts/pre_commit_lint.sh +++ b/scripts/pre_commit_lint.sh @@ -12,10 +12,10 @@ # - litellm/ Python -> `make lint` (test-linting.yml's lint job) # - tests/e2e Python -> `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) -# - tests/ Python, ruff-tests.toml, test-quality-budget.json, scripts/check_test_quality.py, +# - tests/ Python, ruff-tests.toml, scripts/check_test_quality.py, # scripts/test_quality_gate.py # -> ruff over ruff-tests.toml + `make lint-test-quality` (test-linting.yml's -# test-tree ruff and test-quality budget steps) +# test-tree ruff and test-quality gate steps) # - dashboard -> prettier + eslint + lint budgets (test-litellm-ui-build.yml's frontend-lint) # - proxy/types -> regenerate the lazy OpenAPI snapshot and dashboard API types, fail on drift (check-ui-api-types.yml) # @@ -32,7 +32,7 @@ set -eu # before anything else, so N parallel `make check` runs across worktrees execute two # at a time instead of thrashing the machine. The wrapper exports # LITELLM_GATE_SLOT_HELD, so this re-exec happens exactly once and everything this -# script spawns (make lint, the budget gates) skips its own acquisition. +# script spawns (make lint, the lint gates) skips its own acquisition. script_dir=$(python3 -c 'import os, sys; print(os.path.dirname(os.path.realpath(sys.argv[1])))' "$0") if [ -z "${LITELLM_GATE_SLOT_HELD:-}" ]; then exec python3 "$script_dir/gate_slot_lock.py" "$0" "$@" @@ -96,7 +96,7 @@ existing_files() { litellm_py_pattern='^litellm/.*\.py$' e2e_py_pattern='^tests/e2e/.*\.py$' -test_tree_pattern='^(tests/.*\.py|ruff-tests\.toml|test-quality-budget\.json|scripts/(check_test_quality|test_quality_gate)\.py)$' +test_tree_pattern='^(tests/.*\.py|ruff-tests\.toml|scripts/(check_test_quality|test_quality_gate)\.py)$' spec_pattern='^(litellm/(proxy|types)/.*|ui/litellm-dashboard/(scripts/gen-api-types\.mjs|package\.json|package-lock\.json|src/lib/http/schema\.d\.ts))$' ui_prettier_pattern='^ui/litellm-dashboard/.*\.(js|jsx|ts|tsx|mjs|cjs|json|css|scss|md|mdx|yml|yaml|html)$' ui_eslint_pattern='^ui/litellm-dashboard/.*\.(js|jsx|ts|tsx|mjs|cjs)$' @@ -143,7 +143,7 @@ if [ -n "$staged" ]; then } warn_skipped "Python lint (make lint)" "$litellm_py_pattern" "$litellm_py_files" warn_skipped "tests/e2e checks (basedpyright + raw HTTP client ban)" "$e2e_py_pattern" "$e2e_py_files" - warn_skipped "test-tree lint (ruff-tests.toml + test-quality budget)" "$test_tree_pattern" "$test_tree_files" + warn_skipped "test-tree lint (ruff-tests.toml + test-quality gate)" "$test_tree_pattern" "$test_tree_files" warn_skipped "dashboard lint (prettier + eslint + lint budgets)" "$ui_prettier_pattern" "$ui_prettier_changed" warn_skipped "dashboard API-type sync (npm run gen:api)" "$spec_pattern" "$spec_files" fi @@ -300,9 +300,9 @@ if [ -n "$test_tree_files" ] && [ -z "$litellm_py_files" ]; then echo "check: linting the test tree (ruff check --config ruff-tests.toml tests)" uv run --no-sync ruff check --config ruff-tests.toml tests \ || { echo "✗ Test-tree ruff failed. Fix the errors above, then re-run make check." >&2; status=1; } - echo "check: checking the test-quality budget (make lint-test-quality)" + echo "check: checking the test-quality gate (make lint-test-quality)" make lint-test-quality \ - || { echo "✗ Test-quality budget failed. Fix the errors above, then re-run make check." >&2; status=1; } + || { echo "✗ Test-quality gate failed. Fix the errors above, then re-run make check." >&2; status=1; } fi if [ -n "${python_pid:-}" ]; then @@ -330,7 +330,7 @@ summary_item() { echo "check: summary" summary_item "Python lint (make lint)" "$litellm_py_files" "no litellm/ Python files in scope" summary_item "tests/e2e checks (basedpyright + raw HTTP client ban)" "$e2e_py_files" "no tests/e2e Python files in scope" -summary_item "test-tree lint (ruff-tests.toml + test-quality budget)" "$test_tree_files" \ +summary_item "test-tree lint (ruff-tests.toml + test-quality gate)" "$test_tree_files" \ "no tests/ Python files or test-tree lint inputs in scope" summary_item "dashboard lint (prettier + eslint + lint budgets)" "$ui_prettier_changed$ui_eslint_changed" "no dashboard files in scope" summary_item "dashboard API-type sync (npm run gen:api)" "$spec_files" "no litellm/proxy, litellm/types, or generator files in scope" diff --git a/scripts/ruff_strict_gate.py b/scripts/ruff_strict_gate.py index 8da10dd76f0..717ddc38466 100644 --- a/scripts/ruff_strict_gate.py +++ b/scripts/ruff_strict_gate.py @@ -1,12 +1,18 @@ #!/usr/bin/env python3 -"""Total-count gate for the strict ruff rules in ruff-strict.toml. +"""Delta-vs-base gate for the strict ruff rules in ruff-strict.toml. -Each rule has a hard ``limit`` in ruff-strict-budget.json. The gate counts each -rule across the whole tree and fails when a rule is both over its limit and -higher than the base it merges into, so a change is blamed for the violations it -adds, never for drift that already exists in the base. ``--update`` ratchets each -rule's limit down by the number of violations this branch fixed relative to its -branch point (the merge-base). +Each rule is counted across the whole tree at HEAD and at the merge-base with +the branch this change merges into, and the gate fails only when a rule grew +past the merge-base count plus its headroom in HEADROOM (none today), so a +change is blamed for the violations it adds, never for drift that already sits +in the base. There is no committed budget: the merge-base count is the +ceiling, so it moves only when the base branch does. + +The merge-base counts come from scripts/lint_base_counts.py: the disk cache, +then the CI artifact published for that commit, then a ruff pass over a +detached worktree at the merge-base under the current ruff configs. +``--emit-counts-dir`` writes HEAD's counts as the file that artifact is built +from. """ import argparse @@ -17,15 +23,28 @@ import subprocess import sys import tempfile from collections import Counter +from collections.abc import Mapping, Sequence from pathlib import Path +from types import MappingProxyType from typing import Final, NamedTuple -REPO_ROOT = Path(__file__).resolve().parent.parent -STRICT_CONFIG = REPO_ROOT / "ruff-strict.toml" -BUDGET_PATH = REPO_ROOT / "ruff-strict-budget.json" -TARGET = "litellm" +from lint_base_counts import ( + Checker, + base_counts_cached, + emit_counts, + evaluate, + head_sha, + resolve_base_point, + sha256_of, +) -_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +REPO_ROOT: Final = Path(__file__).resolve().parent.parent +STRICT_CONFIG: Final = REPO_ROOT / "ruff-strict.toml" +BASE_CONFIG: Final = REPO_ROOT / "ruff.toml" +TARGET: Final = "litellm" +HEADROOM: Final[Mapping[str, int]] = MappingProxyType({}) + +_HUNK: Final = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") class Violation(NamedTuple): @@ -34,38 +53,20 @@ class Violation(NamedTuple): code: str -class Breach(NamedTuple): - rule: str - total: int - cap: int - added: int - - -def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: - proc = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) +def _run(cmd: Sequence[str], cwd: Path = REPO_ROOT) -> str: + proc: Final = 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 -def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: - """The snapshot commit base counts are measured at: merge-base(base_ref, HEAD), - made aware of an in-progress merge. Mid-merge, HEAD is still the pre-merge tip, - so its merge-base is the old branch point and every violation the base gained - since then would be blamed on this change. While MERGE_HEAD exists, prefer - merge-base(base_ref, MERGE_HEAD) whenever it is the newer of the two.""" - head_point: Final = _run(["git", "merge-base", base_ref, "HEAD"], cwd=cwd).strip() - if not head_point: - return base_ref - merge_head: Final = _run(["git", "rev-parse", "--verify", "--quiet", "MERGE_HEAD"], cwd=cwd).strip() - if not merge_head: - return head_point - merge_point: Final = _run(["git", "merge-base", base_ref, merge_head], cwd=cwd).strip() - if not merge_point: - return head_point - older: Final = _run(["git", "merge-base", head_point, merge_point], cwd=cwd).strip() - return merge_point if older == head_point else head_point +def ruff_version() -> str: + return _run(["ruff", "--version"]).strip() + + +def checker_identity() -> Checker: + return Checker("ruff-strict", (sha256_of(STRICT_CONFIG), sha256_of(BASE_CONFIG), ruff_version())) def _ruff_json(cwd: Path, config: Path) -> list: @@ -76,7 +77,7 @@ def _ruff_json(cwd: Path, config: Path) -> list: return json.loads(raw or "[]") -def head_violations() -> list: +def head_violations() -> list[Violation]: out = [] for item in _ruff_json(REPO_ROOT, STRICT_CONFIG): name = Path(item["filename"]) @@ -90,47 +91,26 @@ def head_violations() -> list: return out -def count_by_rule(violations: list) -> dict: +def count_by_rule(violations: Sequence[Violation]) -> dict[str, int]: return dict(Counter(v.code for v in violations)) -def base_counts(ref: str) -> dict: - parent = Path(tempfile.mkdtemp(prefix="ruff_base_")) - worktree = parent / "wt" +def base_counts(ref: str) -> dict[str, int]: + parent: Final = Path(tempfile.mkdtemp(prefix="ruff_base_")) + worktree: Final = parent / "wt" try: _run(["git", "worktree", "add", "--detach", str(worktree), ref]) - shutil.copy(STRICT_CONFIG, worktree / "ruff-strict.toml") - items = _ruff_json(worktree, worktree / "ruff-strict.toml") + shutil.copy(BASE_CONFIG, worktree / BASE_CONFIG.name) + shutil.copy(STRICT_CONFIG, worktree / STRICT_CONFIG.name) + items: Final = _ruff_json(worktree, worktree / STRICT_CONFIG.name) return dict(Counter(item["code"] for item in items)) finally: _run(["git", "worktree", "remove", "--force", str(worktree)]) shutil.rmtree(parent, ignore_errors=True) -def over_ceiling(head: dict, budget: dict) -> frozenset: - """Rules whose head count already exceeds their limit. - - A rule can only breach when it is over its limit, so when none are the base - comparison cannot change the verdict and the base worktree scan can be skipped. - """ - return frozenset( - rule for rule, spec in budget.items() - if head.get(rule, 0) > spec["limit"] - ) - - -def evaluate(head: dict, base: dict, budget: dict) -> list: - breaches = [] - for rule, spec in budget.items(): - cap = spec["limit"] - total = head.get(rule, 0) - if total > cap and total > base.get(rule, 0): - breaches.append(Breach(rule, total, cap, total - base.get(rule, 0))) - return sorted(breaches) - - -def parse_changed_lines(diff_text: str) -> dict: - changed: dict = {} +def parse_changed_lines(diff_text: str) -> dict[str, set[int]]: + changed: dict[str, set[int]] = {} path = None for line in diff_text.splitlines(): if line.startswith("+++ b/"): @@ -142,84 +122,52 @@ def parse_changed_lines(diff_text: str) -> dict: return changed -def introduced(violations: list, changed: dict) -> list: +def introduced(violations: Sequence[Violation], changed: Mapping[str, set[int]]) -> list[Violation]: 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() - head_counts = count_by_rule(head) - if not over_ceiling(head_counts, budget): - print(f"OK: every strict rule is within its codebase ceiling (base {base})") - return - base_point = resolve_base_point(base) - breaches = evaluate(head_counts, base_counts(base_point), budget) - if not breaches: - print(f"OK: every strict rule is within its codebase ceiling (base {base})") - return - new = introduced( - head, - parse_changed_lines( - _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) - ), + head: Final = head_violations() + base_point: Final = resolve_base_point(base) + breaches: Final = evaluate( + count_by_rule(head), base_counts_cached(checker_identity(), base_point, base_counts), HEADROOM ) - print(f"FAIL: strict-rule totals exceed their limit (base {base}):") + if not breaches: + print(f"OK: no strict rule grew past its merge-base count (base {base})") + return + diff: Final = _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) + new: Final = introduced(head, parse_changed_lines(diff)) + print(f"FAIL: strict-rule totals grew past their merge-base count (base {base}):") for breach in breaches: - print( - f" {breach.rule}: total {breach.total} over limit {breach.cap} (this change added {breach.added})" - ) + print(f" {breach.rule}: total {breach.total} over ceiling {breach.cap} (this change added {breach.added})") for violation in sorted(v for v in new if v.code == breach.rule): print(f" {violation.file}:{violation.line}") print( - "Reduce the new violations or remove an equal number elsewhere; the ceiling is the limit in ruff-strict-budget.json." + "Reduce the new violations or remove an equal number elsewhere; the ceiling is the " + "merge-base count plus the rule's headroom in scripts/ruff_strict_gate.py." ) raise SystemExit(1) -def ratcheted_budget(budget: dict, current: dict, base: dict) -> dict: - """Each rule's limit lowered by the violations `current` fixed vs `base`. - - `base` is the count at the branch point (the commit this branch diverged - from). The drop is clamped to what was actually cleared (a rule that grew - stays put), so the limit only ever falls. - """ - return { - rule: { - "limit": max(0, spec["limit"] - max(0, base.get(rule, 0) - current.get(rule, 0))) - } - for rule, spec in sorted(budget.items()) - } - - -def cmd_update(base_ref: str) -> None: - """Ratchet each rule's limit down by the violations this branch fixed. - - The working-tree count is compared against a ruff 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. - """ - budget = json.loads(BUDGET_PATH.read_text()) - base_point = resolve_base_point(base_ref) - updated = ratcheted_budget( - budget, count_by_rule(head_violations()), base_counts(base_point) - ) - BUDGET_PATH.write_text(json.dumps(updated, indent=2, sort_keys=True) + "\n") - cleared = sum(budget[rule]["limit"] - updated[rule]["limit"] for rule in updated) - print(f"Ratcheted strict-rule limits down by {cleared} violations this branch fixed") - - def main() -> None: - parser = argparse.ArgumentParser(description=__doc__) + parser: Final = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", help="Comparison ref (default: origin's current default branch)") - parser.add_argument("--update", action="store_true") - args = parser.parse_args() + parser.add_argument( + "--emit-counts-dir", + type=Path, + help="Write HEAD's per-rule counts to this directory as a base-counts artifact instead of gating", + ) + args: Final = parser.parse_args() from default_branch import resolve_base_ref from gate_slot_lock import held_slot + if args.emit_counts_dir is not None: + with held_slot(): + emit_counts(checker_identity(), count_by_rule(head_violations()), args.emit_counts_dir, head_sha()) + return base_ref: Final = resolve_base_ref(args.base, REPO_ROOT) with held_slot(): - cmd_update(base_ref) if args.update else cmd_check(base_ref) + cmd_check(base_ref) if __name__ == "__main__": diff --git a/scripts/test_quality_gate.py b/scripts/test_quality_gate.py index e486324c741..8046d446bb7 100644 --- a/scripts/test_quality_gate.py +++ b/scripts/test_quality_gate.py @@ -1,32 +1,29 @@ #!/usr/bin/env python3 -"""Total-count gate for the TQ* rules in scripts/check_test_quality.py. +"""Delta-vs-base gate for the TQ* rules in scripts/check_test_quality.py. Sibling of scripts/type_discipline_gate.py, pointed at the test tree instead of -the package. Each rule listed in test-quality-budget.json has a hard ``limit``. -The gate counts each rule across the whole `tests` tree and fails when a rule is -both over its limit and higher than the base it merges into, so a change is -blamed for the violations it adds, never for drift that already exists in the -base. - -Every rule is seeded at exactly its count on the day the gate landed, so the -suite's existing debt is grandfathered and any net-new violation trips the gate -immediately. ``--update`` ratchets a limit down by the violations fixed relative -to ``--base``, so the ceilings only ever fall. Base counts are measured with the -*current* checker, so a rule introduced on this branch is counted at the base too -and ratchets like every other one. The ratchet runs as a scheduled automation -against the repository's default branch, not on PR branches, so concurrent PRs never -race to edit the same limit. +the package. Each rule is counted across the whole `tests` tree at HEAD and at +the merge-base with the branch this change merges into, and the gate fails only +when a rule grew past the merge-base count, so a change is blamed for the +violations it adds, never for drift that already exists in the base. There is +no committed budget: the merge-base count is the ceiling, so it moves only when +the base branch does. The deliberate difference from its sibling: this gate has no headroom anywhere. -Type discipline seeded LIT010/LIT011 at 1.5x to leave room for an in-flight -sweep; a test-quality violation has no such transition to absorb, so the line is -today's count and the only legal direction is down. +Type discipline keeps the LIT010/LIT011 pool its seeding left; a test-quality +violation has no such transition to absorb, so the line is the merge-base count +and the only legal direction is down. + +The merge-base counts come from scripts/lint_base_counts.py: the disk cache, +then the CI artifact published for that commit, then a pass of the current +checker over a detached worktree at the merge-base, so a rule introduced on +this branch is counted at the base too. ``--emit-counts-dir`` writes HEAD's +counts as the file that artifact is built from. """ from __future__ import annotations import argparse -import json import re import shutil import signal @@ -39,10 +36,20 @@ from pathlib import Path from types import FrameType, MappingProxyType from typing import Final, NamedTuple +from lint_base_counts import ( + Checker, + base_counts_cached, + emit_counts, + evaluate, + head_sha, + resolve_base_point, + sha256_of, +) + REPO_ROOT: Final = Path(__file__).resolve().parent.parent CHECKER: Final = REPO_ROOT / "scripts" / "check_test_quality.py" -BUDGET_PATH: Final = REPO_ROOT / "test-quality-budget.json" TARGET: Final = "tests" +HEADROOM: Final[Mapping[str, int]] = MappingProxyType({}) TERMINATION_SIGNALS: Final = (signal.SIGTERM, signal.SIGHUP) _HUNK: Final = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", re.MULTILINE) @@ -56,13 +63,6 @@ class Violation(NamedTuple): code: str -class Breach(NamedTuple): - rule: str - total: int - cap: int - added: int - - def _run(cmd: Sequence[str], cwd: Path = REPO_ROOT) -> str: proc: Final = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) if proc.returncode not in (0, 1): @@ -71,22 +71,8 @@ def _run(cmd: Sequence[str], cwd: Path = REPO_ROOT) -> str: return proc.stdout -def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: - """The snapshot commit base counts are measured at: merge-base(base_ref, HEAD), - made aware of an in-progress merge. Mid-merge, HEAD is still the pre-merge tip, - so its merge-base is the old branch point and every violation the base gained - since then would be blamed on this change.""" - head_point: Final = _run(["git", "merge-base", base_ref, "HEAD"], cwd=cwd).strip() - if not head_point: - return base_ref - merge_head: Final = _run(["git", "rev-parse", "--verify", "--quiet", "MERGE_HEAD"], cwd=cwd).strip() - if not merge_head: - return head_point - merge_point: Final = _run(["git", "merge-base", base_ref, merge_head], cwd=cwd).strip() - if not merge_point: - return head_point - older: Final = _run(["git", "merge-base", head_point, merge_point], cwd=cwd).strip() - return merge_point if older == head_point else head_point +def checker_identity() -> Checker: + return Checker("test-quality", (sha256_of(CHECKER),)) def _check(root: Path, checker: Path) -> tuple[Violation, ...]: @@ -143,26 +129,6 @@ def base_counts(ref: str, repo_root: Path = REPO_ROOT, checker: Path = CHECKER) shutil.rmtree(parent, ignore_errors=True) -def over_ceiling(head: Mapping[str, int], budget: Mapping[str, Mapping[str, int]]) -> frozenset[str]: - """Rules whose head count already exceeds their limit. When none are, the base - comparison cannot change the verdict and the base worktree scan is skipped.""" - return frozenset( - rule for rule, spec in budget.items() if head.get(rule, 0) > spec["limit"] - ) - - -def evaluate( - head: Mapping[str, int], - base: Mapping[str, int], - budget: Mapping[str, Mapping[str, int]], -) -> tuple[Breach, ...]: - return tuple(sorted( - Breach(rule, head.get(rule, 0), spec["limit"], head.get(rule, 0) - base.get(rule, 0)) - for rule, spec in budget.items() - if head.get(rule, 0) > spec["limit"] and head.get(rule, 0) > base.get(rule, 0) - )) - - def _hunk_lines(body: str) -> frozenset[int]: return frozenset( line @@ -191,92 +157,48 @@ def introduced( def cmd_check(base: str) -> None: - budget: Final = json.loads(BUDGET_PATH.read_text()) head: Final = head_violations() - head_counts: Final = count_by_rule(head) - if not over_ceiling(head_counts, budget): - print(f"OK: every TQ rule is within its test-suite ceiling (base {base})") - return base_point: Final = resolve_base_point(base) - base_at_point: Final = base_counts(base_point) - breaches: Final = evaluate(head_counts, base_at_point, budget) - if not breaches: - print(f"OK: every TQ rule is within its test-suite ceiling (base {base})") - return - new: Final = introduced( - head, - parse_changed_lines( - _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) - ), + breaches: Final = evaluate( + count_by_rule(head), base_counts_cached(checker_identity(), base_point, base_counts), HEADROOM ) - print(f"FAIL: TQ-rule totals exceed their limit (base {base}):") + if not breaches: + print(f"OK: no TQ rule grew past its merge-base count (base {base})") + return + diff: Final = _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) + new: Final = introduced(head, parse_changed_lines(diff)) + print(f"FAIL: TQ-rule totals grew past their merge-base count (base {base}):") for breach in breaches: - print( - f" {breach.rule}: total {breach.total} over limit {breach.cap} " - f"(this change added {breach.added})" - ) + print(f" {breach.rule}: total {breach.total} over ceiling {breach.cap} (this change added {breach.added})") for violation in sorted(v for v in new if v.code == breach.rule): print(f" {violation.file}:{violation.line}") print( - "Fix the new violations, or give each one a reason " - "(`# test-quality-ok: `), or remove an equal number elsewhere; " - "the ceiling is the limit in test-quality-budget.json. " - "Run `python scripts/check_test_quality.py tests/` to see every finding." + "Fix the new violations, or give each one a reason (`# test-quality-ok: `), or remove an " + "equal number elsewhere; the ceiling is the merge-base count. Run " + "`python scripts/check_test_quality.py tests/` to see every finding." ) raise SystemExit(1) -def ratcheted_budget( - budget: Mapping[str, Mapping[str, int]], - current: Mapping[str, int], - base: Mapping[str, int], -) -> Mapping[str, Mapping[str, int]]: - """Each rule's limit lowered by the violations `current` fixed vs `base`. The drop - is clamped to what was actually cleared, so a limit only ever falls.""" - return MappingProxyType({ - rule: {"limit": max(0, spec["limit"] - max(0, base.get(rule, 0) - current.get(rule, 0)))} - for rule, spec in sorted(budget.items()) - }) - - -def cmd_update(base_ref: str) -> None: - """Ratchet each rule's limit down by the violations this branch fixed.""" - budget: Final = json.loads(BUDGET_PATH.read_text()) - base_point: Final = resolve_base_point(base_ref) - updated: Final = ratcheted_budget( - budget, count_by_rule(head_violations()), base_counts(base_point) - ) - BUDGET_PATH.write_text(json.dumps(dict(updated), indent=2, sort_keys=True) + "\n") - cleared: Final = sum(budget[rule]["limit"] - updated[rule]["limit"] for rule in updated) - print(f"Ratcheted TQ-rule limits down by {cleared} violations this branch fixed") - - -def cmd_seed() -> None: - """Write the budget from the working tree's current counts. Used once, to land - the gate; afterwards `--update` is the only thing that may move a limit.""" - counts: Final = count_by_rule(head_violations()) - BUDGET_PATH.write_text( - json.dumps({rule: {"limit": counts[rule]} for rule in sorted(counts)}, indent=2) + "\n" - ) - print(f"Seeded {BUDGET_PATH.name} at " + ", ".join(f"{r}={counts[r]}" for r in sorted(counts))) - - def main() -> None: parser: Final = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", help="Comparison ref (default: origin's current default branch)") - parser.add_argument("--update", action="store_true") - parser.add_argument("--seed", action="store_true") + parser.add_argument( + "--emit-counts-dir", + type=Path, + help="Write HEAD's per-rule counts to this directory as a base-counts artifact instead of gating", + ) args: Final = parser.parse_args() from default_branch import resolve_base_ref from gate_slot_lock import held_slot + if args.emit_counts_dir is not None: + with held_slot(): + emit_counts(checker_identity(), count_by_rule(head_violations()), args.emit_counts_dir, head_sha()) + return + base_ref: Final = resolve_base_ref(args.base, REPO_ROOT) with held_slot(): - if args.seed: - cmd_seed() - elif args.update: - cmd_update(resolve_base_ref(args.base, REPO_ROOT)) - else: - cmd_check(resolve_base_ref(args.base, REPO_ROOT)) + cmd_check(base_ref) if __name__ == "__main__": diff --git a/scripts/type_check_gate.py b/scripts/type_check_gate.py index 78f74ec65a1..55c6d3f41c6 100644 --- a/scripts/type_check_gate.py +++ b/scripts/type_check_gate.py @@ -2,15 +2,14 @@ """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: {limit}}``, the same shape as -``ruff-strict-budget.json``. A rule fails only when its codebase-wide total is -both over its ``limit`` *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 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. +(``reportAny``, ``reportArgumentType``, ...) at HEAD and at the merge-base with +the branch this change merges into. A rule fails only when its codebase-wide +total grew past the merge-base count plus its headroom in HEADROOM (10 for the +two Any-discipline rules, 0 for everything else), so a change is blamed for the +errors it adds, never for drift that already sits in the base, and an unrelated +PR never inherits a red from what landed next to it: its count equals its base. +There is no committed budget: the merge-base count is the ceiling, so it moves +only when the base branch does. Installed packages are part of the measurement: a typed dependency that is present changes what basedpyright can prove (and therefore which diagnostics @@ -28,21 +27,16 @@ 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 +was one forgotten env line away from an 80-second crash. The base pass is a +second basedpyright run over a detached worktree at the merge-base, under the +same environment so import resolution matches, and scripts/lint_base_counts.py +spares it whenever it can: the per-rule counts are cached under the repo's git common dir keyed by merge-base commit, ``pyrightconfig.json``, ``uv.lock``, -the Prisma schema, and the dependency-group set, so re-runs against the same -branch point pay for it once. A CI workflow publishes every staging commit's counts as -an artifact (``--emit-counts-dir`` is its entry point), and on a disk-cache miss -the gate first tries to download the merge-base's artifact through the ``gh`` -CLI; any fetch failure falls back silently to the local base pass, so the gate -never gets worse than it was without CI. ``--update`` ratchets each rule's ``limit`` down by the -number of errors this branch fixed relative to its branch point (the merge-base), -so the headroom you were granted shrinks by exactly what you cleared and never -grows. +the Prisma schema, and the dependency-group set, and on a disk-cache miss the +artifact publish-lint-base-counts.yml uploaded for the merge-base is +downloaded through the ``gh`` CLI (``--emit-counts-dir`` is the publisher's +entry point); any fetch failure falls back silently to the local base pass, so +the gate never gets worse than it was without CI. ``--outputjson`` is used rather than text diagnostics because the latter wrap across lines, leaving the ``(reportRule)`` on a continuation line away from the @@ -53,33 +47,36 @@ carries an unambiguous ``rule`` field. import argparse import contextlib import hashlib -import io import json import os -import re import shutil import subprocess import sys import tempfile -import zipfile from collections import Counter -from collections.abc import Callable, Iterator, Mapping, Sequence +from collections.abc import Callable, Iterator, Mapping from pathlib import Path -from typing import Final, NamedTuple +from types import MappingProxyType +from typing import Final + +from lint_base_counts import ( + Checker, + base_counts_cached, + emit_counts, + evaluate, + head_sha, + resolve_base_point, +) REPO_ROOT = Path(__file__).resolve().parent.parent -BUDGET_PATH = REPO_ROOT / "basedpyright-code-budget.json" PYRIGHT_CONFIG = REPO_ROOT / "pyrightconfig.json" UV_LOCK = REPO_ROOT / "uv.lock" -CACHE_FILE_PREFIX = "basedpyright-base-" -CACHE_KEEP_ENTRIES = 8 -ARTIFACT_NAME_PREFIX = "basedpyright-counts-" -GH_TIMEOUT_SECONDS = 10 # The one environment every basedpyright pass measures in. The group set is # the slim one the CI publisher has always installed (not bootstrap's fatter -# --extra proxy env), so the committed budgets stay valid; changing it re-keys -# every cache and artifact fingerprint, so stale counts can never be matched. +# --extra proxy env), so published and cached counts stay comparable; changing +# it re-keys every cache and artifact fingerprint, so stale counts can never be +# matched. TYPECHECK_ENV_DIR = REPO_ROOT / ".venv-typecheck" TYPECHECK_DEP_GROUPS = ("proxy-dev", "e2e-dev") PRISMA_GENERATE_SCRIPT = REPO_ROOT / "scripts" / "prisma_generate_if_needed.py" @@ -93,17 +90,7 @@ NODE_HEAP_OPTION = "--max-old-space-size=8192" # Bucket for a basedpyright diagnostic with no `rule`. Counted so it's gated. UNCODED = "" -# Limit for a rule that shows up at HEAD but isn't in the budget at all -- a -# brand-new error category (new construct, or a tool/version change). The rule -# fails once it clears this many errors. -DEFAULT_LIMIT = 10 - - -class Breach(NamedTuple): - code: str - total: int - cap: int - added: int +HEADROOM: Final[Mapping[str, int]] = MappingProxyType({"reportAny": 10, "reportExplicitAny": 10}) def _to_relative(raw: str, root: Path) -> str | None: @@ -238,25 +225,6 @@ def run_basedpyright(cwd: Path = REPO_ROOT, env_dir: Path = TYPECHECK_ENV_DIR) - return proc.stdout -def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: - """The snapshot commit base counts are measured at: merge-base(base_ref, HEAD), - made aware of an in-progress merge. Mid-merge, HEAD is still the pre-merge tip, - so its merge-base is the old branch point and every violation the base gained - since then would be blamed on this change. While MERGE_HEAD exists, prefer - merge-base(base_ref, MERGE_HEAD) whenever it is the newer of the two.""" - head_point: Final = _run(["git", "merge-base", base_ref, "HEAD"], cwd=cwd).strip() - if not head_point: - return base_ref - merge_head: Final = _run(["git", "rev-parse", "--verify", "--quiet", "MERGE_HEAD"], cwd=cwd).strip() - if not merge_head: - return head_point - merge_point: Final = _run(["git", "merge-base", base_ref, merge_head], cwd=cwd).strip() - if not merge_point: - return head_point - older: Final = _run(["git", "merge-base", head_point, merge_point], cwd=cwd).strip() - return merge_point if older == head_point else head_point - - @contextlib.contextmanager def _temp_worktree(ref: str) -> Iterator[Path]: parent = Path(tempfile.mkdtemp(prefix="bpr_base_")) @@ -283,21 +251,6 @@ def base_counts(ref: str) -> dict[str, int]: return count_basedpyright(run_basedpyright(worktree), root=worktree) -def over_ceiling( - head: Mapping[str, int], budget: Mapping[str, Mapping[str, int]] -) -> frozenset[str]: - """Rules whose head count already exceeds their limit. - - A rule can only breach when it is over its limit, so when none are the base - comparison cannot change the verdict and the base worktree pass can be skipped. - """ - return frozenset( - code - for code, total in head.items() - if total > (budget[code]["limit"] if code in budget else DEFAULT_LIMIT) - ) - - def environment_fingerprints( dep_groups: tuple[str, ...] = TYPECHECK_DEP_GROUPS, ) -> tuple[str, ...]: @@ -311,377 +264,66 @@ def environment_fingerprints( ) -def cache_key(base_point: str, fingerprints: tuple[str, ...]) -> str: - return hashlib.sha256("|".join((base_point, *fingerprints)).encode()).hexdigest()[ - :16 - ] - - -def cache_path( - directory: Path, base_point: str, fingerprints: tuple[str, ...] -) -> Path: - return directory / f"{CACHE_FILE_PREFIX}{cache_key(base_point, fingerprints)}.json" - - -def default_cache_dir() -> Path: - common = Path(_run(["git", "rev-parse", "--git-common-dir"]).strip()) - resolved = common if common.is_absolute() else REPO_ROOT / common - return resolved / "litellm-lint-cache" - - -def validated_counts(data: object) -> dict[str, int] | None: - counts: Final = data.get("counts") if isinstance(data, dict) else None - if not isinstance(counts, dict): - return None - if not all( - isinstance(code, str) and isinstance(total, int) and not isinstance(total, bool) - for code, total in counts.items() - ): - return None - return counts - - -def load_cached_counts(path: Path) -> dict[str, int] | None: - try: - data = json.loads(path.read_text()) - except (OSError, json.JSONDecodeError): - return None - return validated_counts(data) - - -def scratch_path(path: Path) -> Path: - """In-flight scratch for the tmp+rename write. Dot-prefixed so the prune - glob in `store_counts` can never match it (a concurrent run would otherwise - unlink it between write and rename), and pid-suffixed so two concurrent - writers of the same entry never share a scratch.""" - return path.with_name(f".{path.name}.{os.getpid()}.tmp") - - -def counts_payload(base_point: str, counts: Mapping[str, int]) -> str: - return ( - json.dumps( - {"base_point": base_point, "counts": dict(sorted(counts.items()))}, - indent=2, - ) - + "\n" - ) - - -def entry_recency(path: Path) -> float: - try: - return path.stat().st_mtime - except OSError: - return 0.0 - - -def evicted_beyond_cap(entries: Sequence[Path], keep: int) -> tuple[Path, ...]: - newest_first: Final = sorted(entries, key=entry_recency, reverse=True) - return tuple(newest_first[keep:]) - - -def store_counts( - directory: Path, path: Path, base_point: str, counts: Mapping[str, int] -) -> None: - directory.mkdir(parents=True, exist_ok=True) - scratch = scratch_path(path) - scratch.write_text(counts_payload(base_point, counts)) - scratch.replace(path) - siblings: Final = tuple( - entry for entry in directory.glob(f"{CACHE_FILE_PREFIX}*.json") if entry != path - ) - for stale in evicted_beyond_cap(siblings, CACHE_KEEP_ENTRIES - 1): - stale.unlink(missing_ok=True) - - -def parse_origin_slug(url: str) -> str | None: - match: Final = re.fullmatch( - r"(?:git@github\.com:|https://github\.com/)([^/]+/[^/]+?)(?:\.git)?/?", - url.strip(), - ) - return match.group(1) if match else None - - -def origin_slug() -> str | None: - proc: Final = subprocess.run( - ["git", "remote", "get-url", "origin"], - cwd=REPO_ROOT, - capture_output=True, - text=True, - ) - if proc.returncode != 0: - return None - return parse_origin_slug(proc.stdout) - - -def artifact_name(base_point: str) -> str: - return f"{ARTIFACT_NAME_PREFIX}{cache_key(base_point, environment_fingerprints())}" - - -def _gh_output(args: list[str]) -> bytes | None: - try: - proc = subprocess.run( - ["gh", *args], capture_output=True, timeout=GH_TIMEOUT_SECONDS - ) - except (OSError, subprocess.SubprocessError): - return None - return proc.stdout if proc.returncode == 0 else None - - -def _parsed_json(raw: bytes) -> object | None: - try: - return json.loads(raw) - except ValueError: - return None - - -def _artifact_download_url(listing: object) -> str | None: - artifacts: Final = listing.get("artifacts") if isinstance(listing, dict) else None - if not isinstance(artifacts, list) or not artifacts: - return None - newest: Final = artifacts[0] - if not isinstance(newest, dict) or newest.get("expired"): - return None - url: Final = newest.get("archive_download_url") - return url if isinstance(url, str) else None - - -def _counts_json_from_zip(zip_bytes: bytes) -> object | None: - try: - with zipfile.ZipFile(io.BytesIO(zip_bytes)) as archive: - members: Final = [ - name for name in archive.namelist() if name.endswith(".json") - ] - if len(members) != 1: - return None - return json.loads(archive.read(members[0])) - except (zipfile.BadZipFile, ValueError, OSError): - return None - - -def counts_for_base(payload: object, base_point: str) -> dict[str, int] | None: - if not isinstance(payload, dict) or payload.get("base_point") != base_point: - return None - counts: Final = validated_counts(payload) - return counts if counts else None - - -def _fetch_fallback(reason: str) -> None: - sys.stderr.write(f"{reason}; computing base counts locally\n") - - -def fetch_ci_base_counts( - base_point: str, - gh_output: Callable[[list[str]], bytes | None] = _gh_output, -) -> dict[str, int] | None: - """Base counts from the CI artifact published for `base_point`, or None. - - Every failure mode (no gh, no auth, offline, expired or missing artifact, - malformed payload, counts for a different commit) returns None so the - caller falls back to the local base pass; the fetch is an optimization and - must never make the gate less available than local compute alone.""" - slug: Final = origin_slug() - if slug is None: - return _fetch_fallback("origin remote is not a github.com URL") - name: Final = artifact_name(base_point) - listing: Final = gh_output( - ["api", f"repos/{slug}/actions/artifacts?name={name}&per_page=1"] - ) - if listing is None: - return _fetch_fallback(f"could not list CI artifacts named {name}") - url: Final = _artifact_download_url(_parsed_json(listing)) - if url is None: - return _fetch_fallback(f"no usable CI artifact named {name}") - zip_bytes: Final = gh_output(["api", url]) - if zip_bytes is None: - return _fetch_fallback(f"download failed for CI artifact {name}") - counts: Final = counts_for_base(_counts_json_from_zip(zip_bytes), base_point) - if counts is None: - return _fetch_fallback( - f"CI artifact {name} is not valid base counts for {base_point[:12]}" - ) - sys.stderr.write(f"base counts fetched from CI artifact {name}\n") - return counts - - -def base_counts_cached( - base_point: str, - cache_dir: Path | None = None, - compute: Callable[[str], dict[str, int]] = base_counts, - fetch: Callable[[str], dict[str, int] | None] = fetch_ci_base_counts, -) -> dict[str, int]: - """`base_counts` memoized on disk. The base tree at a given commit is - immutable, so its counts are a pure function of the merge-base plus the - environment fingerprints in the cache key; an empty result is never stored - because it is the signature of a crashed pass, not a clean tree. On a disk - miss the counts CI already published for the merge-base are fetched before - the expensive local base pass; a fetch miss of any kind computes locally.""" - directory = default_cache_dir() if cache_dir is None else cache_dir - path = cache_path(directory, base_point, environment_fingerprints()) - cached = load_cached_counts(path) - if cached is not None: - return cached - fetched: Final = fetch(base_point) - if fetched: - store_counts(directory, path, base_point, fetched) - return fetched - counts = compute(base_point) - if counts: - store_counts(directory, path, base_point, counts) - return counts - - -def evaluate( - head: Mapping[str, int], - base: Mapping[str, int], - budget: Mapping[str, Mapping[str, int]], -) -> list[Breach]: - breaches = [] - for code, total in head.items(): - spec = budget.get(code) - cap = spec["limit"] if spec else DEFAULT_LIMIT - prior = base.get(code, 0) - if total > cap and total > prior: - breaches.append(Breach(code, total, cap, total - prior)) - return sorted(breaches) - - -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 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()) - - -def ratcheted_budget( - budget: Mapping[str, Mapping[str, int]], - current: Mapping[str, int], - base: Mapping[str, int], -) -> dict[str, dict[str, int]]: - """Each rule's limit lowered by the errors `current` fixed vs `base`. - - `base` is the count at the branch point (the commit this branch diverged - from). The drop is clamped to what was actually cleared (a rule that grew - stays put), so the limit only ever falls. Rules absent from the budget are - dropped: a genuinely new error category is added to the JSON deliberately, - not on update. - """ - return { - code: { - "limit": max(0, spec["limit"] - max(0, base.get(code, 0) - current.get(code, 0))) - } - for code, spec in sorted(budget.items()) - } - - -def cmd_update(current: Mapping[str, int], base_ref: str) -> None: - """Ratchet each rule's limit down by the errors this branch fixed. - - `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. - """ - budget = json.loads(BUDGET_PATH.read_text()) if BUDGET_PATH.exists() else {} - base_point = resolve_base_point(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) - print( - f"Ratcheted basedpyright limits down by {cleared} errors this branch fixed " - f"across {len(updated)} rules" - ) - - -def cmd_emit_counts(head: Mapping[str, int], directory: Path, head_sha: str) -> None: - """Write HEAD's per-rule counts as the file the publisher workflow uploads. - - The filename stem is exactly the artifact name `fetch_ci_base_counts` will - later look up for this commit, so emit and fetch cannot drift apart. Empty - counts are refused for the same reason `is_vacuous_run` exists: a pass that - produced nothing almost certainly crashed, and publishing it would poison - every branch that fetches it.""" - if not head: - print( - "FAIL: basedpyright produced no errors; refusing to publish empty base " - "counts because the pass almost certainly crashed or emitted nothing." - ) - raise SystemExit(1) - name: Final = artifact_name(head_sha) - directory.mkdir(parents=True, exist_ok=True) - (directory / f"{name}.json").write_text(counts_payload(head_sha, head)) - print( - f"Emitted base counts for {head_sha} as {name}.json " - f"({sum(head.values())} errors total)" - ) +def checker_identity() -> Checker: + return Checker("basedpyright", environment_fingerprints()) def cmd_check(head: Mapping[str, int], base_ref: str) -> None: - budget = json.loads(BUDGET_PATH.read_text()) - if is_vacuous_run(head, budget): - expected = sum(spec["limit"] for spec in budget.values()) + if not head: print( - 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." + "FAIL: basedpyright produced no errors. The type checker almost certainly " + "crashed or emitted nothing; refusing to certify a vacuous run." ) raise SystemExit(1) - if not over_ceiling(head, budget): + base_point: Final = resolve_base_point(base_ref) + base: Final = base_counts_cached(checker_identity(), base_point, base_counts) + if not base: print( - f"OK: every rule is within its basedpyright limit ({sum(head.values())} errors total)" - ) - return - base_point = resolve_base_point(base_ref) - base = base_counts_cached(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." + f"FAIL: basedpyright produced no errors for the base tree at {base_point[:12]}, " + "so every rule would look freshly added. The base pass almost certainly " + "crashed; refusing to blame this change for it." ) raise SystemExit(1) - breaches = evaluate(head, base, budget) + breaches: Final = evaluate(head, base, HEADROOM) if not breaches: print( - f"OK: every rule is within its basedpyright limit or no higher than base ({sum(head.values())} errors total)" + f"OK: no basedpyright rule grew past its merge-base count " + f"({sum(head.values())} errors total, base {base_point[:12]})" ) return - print("FAIL: basedpyright errors exceed the per-rule limit:") + print(f"FAIL: basedpyright errors grew past their merge-base count (base {base_point[:12]}):") for breach in breaches: - print( - f" {breach.code}: total {breach.total} over limit {breach.cap} (this change added {breach.added})" - ) + print(f" {breach.rule}: total {breach.total} over ceiling {breach.cap} (this change added {breach.added})") print( - "Reduce the new errors or remove an equal number elsewhere; the ceiling is " - "the limit in basedpyright-code-budget.json." + "Reduce the new errors or remove an equal number elsewhere; the ceiling is the " + "merge-base count plus the rule's headroom in scripts/type_check_gate.py." ) - summary = "; ".join(f"{b.code} {b.total}/{b.cap} (+{b.added})" for b in breaches) + summary: Final = "; ".join(f"{b.rule} {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: Final = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", help="Comparison ref (default: origin's current default branch)") - parser.add_argument("--update", action="store_true") - parser.add_argument("--emit-counts-dir", type=Path) - args = parser.parse_args() + parser.add_argument( + "--emit-counts-dir", + type=Path, + help="Write HEAD's per-rule counts to this directory as a base-counts artifact instead of gating", + ) + args: Final = parser.parse_args() from default_branch import resolve_base_ref from gate_slot_lock import held_slot - base_ref: Final = None if args.emit_counts_dir is not None else resolve_base_ref(args.base, REPO_ROOT) + if args.emit_counts_dir is not None: + with held_slot(): + ensure_typecheck_env() + emit_counts(checker_identity(), count_basedpyright(run_basedpyright()), args.emit_counts_dir, head_sha()) + return + base_ref: Final = resolve_base_ref(args.base, REPO_ROOT) with held_slot(): ensure_typecheck_env() - head = count_basedpyright(run_basedpyright()) - if args.emit_counts_dir is not None: - cmd_emit_counts( - head, args.emit_counts_dir, _run(["git", "rev-parse", "HEAD"]).strip() - ) - elif base_ref is not None: - cmd_update(head, base_ref) if args.update else cmd_check(head, base_ref) + cmd_check(count_basedpyright(run_basedpyright()), base_ref) if __name__ == "__main__": diff --git a/scripts/type_discipline_gate.py b/scripts/type_discipline_gate.py index 40e61cf7265..daaa9f1720c 100644 --- a/scripts/type_discipline_gate.py +++ b/scripts/type_discipline_gate.py @@ -1,52 +1,61 @@ #!/usr/bin/env python3 -"""Total-count gate for the LIT* rules in scripts/check_type_discipline.py. +"""Delta-vs-base gate for the LIT* rules in scripts/check_type_discipline.py. -Sibling of scripts/ruff_strict_gate.py. Each rule listed in -type-discipline-budget.json has a hard ``limit``. The gate counts each rule -across the whole `litellm` tree and fails when a rule is both over its limit and -higher than the base it merges into, so a change is blamed for the violations it -adds, never for drift that already exists in the base. +Sibling of scripts/ruff_strict_gate.py. Each rule is counted across the whole +`litellm` tree at HEAD and at the merge-base with the branch this change merges +into, and the gate fails only when a rule grew past the merge-base count plus +its headroom in HEADROOM, so a change is blamed for the violations it adds, +never for drift that already exists in the base. There is no committed budget: +the merge-base count is the ceiling, so it moves only when the base branch does. -Rules not present in the budget are ignored, but today every rule the checker -emits is gated: LIT001 (mutable collection in any annotation), LIT002 -(mutable-collection construction), LIT003/LIT004 (noqa / pyright-mypy ignore -without codes or reason), LIT006 (cast), LIT008 (`**kwargs`), LIT009 (inert -`# type: ignore`, dead syntax while enableTypeIgnoreComments is false), LIT010 -(assignment without a Final declaration; suppress deliberate rebinding with -`# rebind-ok: `), LIT011 (parameter rebinding or in-place mutation), and -LIT012 (TypedDict field without a `ReadOnly[...]` qualifier; suppress with -`# writable-ok: `) carry limits at or above their current count to -ratchet down; LIT005 (`*-ok` suppression without a reason) is frozen at limit 0 -so any net-new reasonless suppression trips the gate; and LIT007 -(TypeGuard/TypeIs) is a hard zero. -LIT010 and LIT011 were seeded at 1.5x the count left after the sweep that -annotated every never-rebound name with Final, so that headroom is the hard -line new code cannot cross. -``--update`` ratchets a limit down by the violations this branch fixed relative -to its branch point (the merge-base). A rule absent from the budget at the -merge-base was seeded on this branch; ``--update`` leaves its limit untouched, -because the base tree predates the rule and its whole grandfathered count would -otherwise be misread as "fixed", collapsing the deliberate headroom to zero. +Every rule the checker emits is gated: LIT001 (mutable collection in any +annotation), LIT002 (mutable-collection construction), LIT003/LIT004 (noqa / +pyright-mypy ignore without codes or reason), LIT005 (`*-ok` suppression +without a reason), LIT006 (cast), LIT007 (TypeGuard/TypeIs), LIT008 +(`**kwargs`), LIT009 (inert `# type: ignore`, dead syntax while +enableTypeIgnoreComments is false), LIT010 (assignment without a Final +declaration; suppress deliberate rebinding with `# rebind-ok: `), +LIT011 (parameter rebinding or in-place mutation), and LIT012 (TypedDict field +without a `ReadOnly[...]` qualifier; suppress with `# writable-ok: `). +Every rule has zero headroom except LIT010 and LIT011, which keep the pool +their seeding left them: a change may add that many before the gate trips. + +The merge-base counts come from scripts/lint_base_counts.py: the disk cache, +then the CI artifact published for that commit, then a pass of the current +checker over a detached worktree at the merge-base, so a rule change on this +branch is measured on both sides. ``--emit-counts-dir`` writes HEAD's counts +as the file that artifact is built from. """ import argparse -import json import re import shutil import subprocess import sys import tempfile from collections import Counter +from collections.abc import Mapping, Sequence from pathlib import Path +from types import MappingProxyType from typing import Final, NamedTuple -REPO_ROOT = Path(__file__).resolve().parent.parent -CHECKER = REPO_ROOT / "scripts" / "check_type_discipline.py" -BUDGET_PATH = REPO_ROOT / "type-discipline-budget.json" -TARGET = "litellm" +from lint_base_counts import ( + Checker, + base_counts_cached, + emit_counts, + evaluate, + head_sha, + resolve_base_point, + sha256_of, +) -_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") -_LINE = re.compile(r"^(?P.+?):(?P\d+): (?PLIT\d+) ") +REPO_ROOT: Final = Path(__file__).resolve().parent.parent +CHECKER: Final = REPO_ROOT / "scripts" / "check_type_discipline.py" +TARGET: Final = "litellm" +HEADROOM: Final[Mapping[str, int]] = MappingProxyType({"LIT010": 44, "LIT011": 5}) + +_HUNK: Final = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +_LINE: Final = re.compile(r"^(?P.+?):(?P\d+): (?PLIT\d+) ") class Violation(NamedTuple): @@ -55,41 +64,19 @@ class Violation(NamedTuple): code: str -class Breach(NamedTuple): - rule: str - total: int - cap: int - added: int - - -def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: - proc = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) +def _run(cmd: Sequence[str], cwd: Path = REPO_ROOT) -> str: + proc: Final = 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 -def resolve_base_point(base_ref: str, cwd: Path = REPO_ROOT) -> str: - """The snapshot commit base counts are measured at: merge-base(base_ref, HEAD), - made aware of an in-progress merge. Mid-merge, HEAD is still the pre-merge tip, - so its merge-base is the old branch point and every violation the base gained - since then would be blamed on this change. While MERGE_HEAD exists, prefer - merge-base(base_ref, MERGE_HEAD) whenever it is the newer of the two.""" - head_point: Final = _run(["git", "merge-base", base_ref, "HEAD"], cwd=cwd).strip() - if not head_point: - return base_ref - merge_head: Final = _run(["git", "rev-parse", "--verify", "--quiet", "MERGE_HEAD"], cwd=cwd).strip() - if not merge_head: - return head_point - merge_point: Final = _run(["git", "merge-base", base_ref, merge_head], cwd=cwd).strip() - if not merge_point: - return head_point - older: Final = _run(["git", "merge-base", head_point, merge_point], cwd=cwd).strip() - return merge_point if older == head_point else head_point +def checker_identity() -> Checker: + return Checker("type-discipline", (sha256_of(CHECKER),)) -def _check(root: Path, checker: Path) -> list: +def _check(root: Path, checker: Path) -> list[Violation]: # 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. root = root.resolve() @@ -106,22 +93,22 @@ def _check(root: Path, checker: Path) -> list: return found -def head_violations() -> list: +def head_violations() -> list[Violation]: return _check(REPO_ROOT, CHECKER) -def count_by_rule(violations: list) -> dict: +def count_by_rule(violations: Sequence[Violation]) -> dict[str, int]: return dict(Counter(v.code for v in violations)) -def base_counts(ref: str) -> dict: - parent = Path(tempfile.mkdtemp(prefix="lit_base_")) - worktree = parent / "wt" +def base_counts(ref: str) -> dict[str, int]: + parent: Final = Path(tempfile.mkdtemp(prefix="lit_base_")) + worktree: Final = parent / "wt" try: _run(["git", "worktree", "add", "--detach", str(worktree), ref]) # Measure the base with the *current* rule logic, not whatever shipped at base. (worktree / "scripts").mkdir(parents=True, exist_ok=True) - checker = worktree / "scripts" / "check_type_discipline.py" + checker: Final = worktree / "scripts" / "check_type_discipline.py" shutil.copy(CHECKER, checker) return count_by_rule(_check(worktree, checker)) finally: @@ -134,30 +121,8 @@ def base_counts(ref: str) -> dict: shutil.rmtree(parent, ignore_errors=True) -def over_ceiling(head: dict, budget: dict) -> frozenset: - """Rules whose head count already exceeds their limit. - - A rule can only breach when it is over its limit, so when none are the base - comparison cannot change the verdict and the base worktree scan can be skipped. - """ - return frozenset( - rule for rule, spec in budget.items() - if head.get(rule, 0) > spec["limit"] - ) - - -def evaluate(head: dict, base: dict, budget: dict) -> list: - breaches = [] - for rule, spec in budget.items(): - cap = spec["limit"] - total = head.get(rule, 0) - if total > cap and total > base.get(rule, 0): - breaches.append(Breach(rule, total, cap, total - base.get(rule, 0))) - return sorted(breaches) - - -def parse_changed_lines(diff_text: str) -> dict: - changed: dict = {} +def parse_changed_lines(diff_text: str) -> dict[str, set[int]]: + changed: dict[str, set[int]] = {} path = None for line in diff_text.splitlines(): if line.startswith("+++ b/"): @@ -169,109 +134,55 @@ def parse_changed_lines(diff_text: str) -> dict: return changed -def introduced(violations: list, changed: dict) -> list: +def introduced(violations: Sequence[Violation], changed: Mapping[str, set[int]]) -> list[Violation]: 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() - head_counts = count_by_rule(head) - if not over_ceiling(head_counts, budget): - print(f"OK: every LIT rule is within its codebase ceiling (base {base})") - return - base_point = resolve_base_point(base) - breaches = evaluate(head_counts, base_counts(base_point), budget) - if not breaches: - print(f"OK: every LIT rule is within its codebase ceiling (base {base})") - return - new = introduced( - head, - parse_changed_lines( - _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) - ), + head: Final = head_violations() + base_point: Final = resolve_base_point(base) + breaches: Final = evaluate( + count_by_rule(head), base_counts_cached(checker_identity(), base_point, base_counts), HEADROOM ) - print(f"FAIL: LIT-rule totals exceed their limit (base {base}):") + if not breaches: + print(f"OK: no LIT rule grew past its merge-base count (base {base})") + return + diff: Final = _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) + new: Final = introduced(head, parse_changed_lines(diff)) + print(f"FAIL: LIT-rule totals grew past their merge-base count (base {base}):") for breach in breaches: - print( - f" {breach.rule}: total {breach.total} over limit {breach.cap} (this change added {breach.added})" - ) + print(f" {breach.rule}: total {breach.total} over ceiling {breach.cap} (this change added {breach.added})") for violation in sorted(v for v in new if v.code == breach.rule): print(f" {violation.file}:{violation.line}") print( "Remove the new violations, give each a reason (`# noqa: XXX # `, " - "`# pyright: ignore[rule] # `, `# mutable-ok: `, " - "`# cast-ok: `, `# guard-ok: `, `# kwargs-ok: `, " - "`# rebind-ok: `, `# writable-ok: `), or remove an equal " - "number elsewhere; the ceiling " - "is the limit in type-discipline-budget.json." + "`# pyright: ignore[rule] # `, `# mutable-ok: `, `# cast-ok: `, " + "`# guard-ok: `, `# kwargs-ok: `, `# rebind-ok: `, " + "`# writable-ok: `), or remove an equal number elsewhere; the ceiling is the " + "merge-base count plus the rule's headroom in scripts/type_discipline_gate.py." ) raise SystemExit(1) -def ratcheted_budget(budget: dict, current: dict, base: dict, seeded: frozenset = frozenset()) -> dict: - """Each rule's limit lowered by the violations `current` fixed vs `base`. - - `base` is the count at the branch point (the commit this branch diverged - from). The drop is clamped to what was actually cleared (a rule that grew - stays put), so the limit only ever falls. Rules in `seeded` were introduced - on this branch with deliberate grandfathered headroom; their limits pass - through untouched, since the base predates the rule and comparing against it - would misread the entire grandfathered count as fixed. - """ - return { - rule: { - "limit": spec["limit"] if rule in seeded - else max(0, spec["limit"] - max(0, base.get(rule, 0) - current.get(rule, 0))) - } - for rule, spec in sorted(budget.items()) - } - - -def _base_budget_rules(base_point: str) -> frozenset: - proc = subprocess.run( - ["git", "show", f"{base_point}:{BUDGET_PATH.name}"], - cwd=REPO_ROOT, capture_output=True, text=True, - ) - if proc.returncode != 0: - return frozenset() - return frozenset(json.loads(proc.stdout)) - - -def cmd_update(base_ref: str) -> None: - """Ratchet each rule's limit down by the violations this branch fixed. - - The working-tree count is compared against a checker 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. - """ - budget = json.loads(BUDGET_PATH.read_text()) - base_point = resolve_base_point(base_ref) - seeded = frozenset(budget) - _base_budget_rules(base_point) - updated = ratcheted_budget( - budget, count_by_rule(head_violations()), base_counts(base_point), seeded - ) - BUDGET_PATH.write_text(json.dumps(updated, indent=2, sort_keys=True) + "\n") - cleared = sum(budget[rule]["limit"] - updated[rule]["limit"] for rule in updated) - print(f"Ratcheted LIT-rule limits down by {cleared} violations this branch fixed") - if seeded: - print( - "Left untouched (seeded on this branch, absent from the base budget): " - + ", ".join(sorted(seeded)) - ) - - def main() -> None: - parser = argparse.ArgumentParser(description=__doc__) + parser: Final = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", help="Comparison ref (default: origin's current default branch)") - parser.add_argument("--update", action="store_true") - args = parser.parse_args() + parser.add_argument( + "--emit-counts-dir", + type=Path, + help="Write HEAD's per-rule counts to this directory as a base-counts artifact instead of gating", + ) + args: Final = parser.parse_args() from default_branch import resolve_base_ref from gate_slot_lock import held_slot + if args.emit_counts_dir is not None: + with held_slot(): + emit_counts(checker_identity(), count_by_rule(head_violations()), args.emit_counts_dir, head_sha()) + return base_ref: Final = resolve_base_ref(args.base, REPO_ROOT) with held_slot(): - cmd_update(base_ref) if args.update else cmd_check(base_ref) + cmd_check(base_ref) if __name__ == "__main__": diff --git a/test-quality-budget.json b/test-quality-budget.json deleted file mode 100644 index 3c12371f02f..00000000000 --- a/test-quality-budget.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "TQ001": { - "limit": 733 - }, - "TQ002": { - "limit": 737 - }, - "TQ003": { - "limit": 62 - }, - "TQ004": { - "limit": 469 - }, - "TQ005": { - "limit": 2399 - }, - "TQ006": { - "limit": 34 - }, - "TQ007": { - "limit": 117 - }, - "TQ008": { - "limit": 10993 - } -} diff --git a/tests/test_litellm/test_budget_ratchet_check.py b/tests/test_litellm/test_budget_ratchet_check.py deleted file mode 100644 index 22d05f4d00d..00000000000 --- a/tests/test_litellm/test_budget_ratchet_check.py +++ /dev/null @@ -1,158 +0,0 @@ -"""Tests for scripts/budget_ratchet_check.py. - -The guard's contract is "limits may only fall": a raised limit, a dropped rule, or -a deleted file is a regression, while a lowered/equal limit, a brand-new rule, or a -brand-new budget file is fine. Each branch is pinned here. -""" - -import importlib.util -import subprocess -import sys -from pathlib import Path - -_MODULE_PATH = ( - Path(__file__).resolve().parents[2] / "scripts" / "budget_ratchet_check.py" -) -_spec = importlib.util.spec_from_file_location("budget_ratchet_check", _MODULE_PATH) -ratchet = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(ratchet) - - -def _spec_of(limit): - return {"limit": limit} - - -def test_limits_read_the_limit_and_skip_malformed(): - limits = ratchet._limits({"LIT006": _spec_of(1023), "junk": 5}) - assert limits == {"LIT006": 1023} # malformed (non-dict) spec ignored - - -def test_limits_fall_back_to_legacy_baseline_plus_slack(): - # The base side of a diff can predate the `limit` migration; its ceiling is - # baseline + slack, read on the same footing as a new-schema `limit`. - assert ratchet._limits({"LIT006": {"baseline": 1013, "slack": 10}}) == {"LIT006": 1023} - - -def test_migration_from_legacy_schema_to_equal_limit_is_clean(): - # baseline+slack (1023) -> limit 1023 is the same ceiling, so no regression. - base = {"LIT006": {"baseline": 1013, "slack": 10}} - assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1023)}) == [] - # ...and a genuine raise across the migration is still caught. - regs = ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1024)}) - assert [r.rule for r in regs] == ["LIT006"] and "1023 -> 1024" in regs[0].detail - - -def test_raised_limit_is_a_regression(): - base = {"LIT006": _spec_of(1023)} - head = {"LIT006": _spec_of(1024)} - regs = ratchet.regressions_for("b.json", base, head) - assert [r.rule for r in regs] == ["LIT006"] - assert "1023 -> 1024" in regs[0].detail - - -def test_lowered_or_equal_limit_is_clean(): - base = {"LIT006": _spec_of(1023)} - # limit drops - assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1000)}) == [] - # nothing changes - assert ratchet.regressions_for("b.json", base, {"LIT006": _spec_of(1023)}) == [] - - -def test_dropped_rule_is_a_regression(): - regs = ratchet.regressions_for("b.json", {"LIT007": _spec_of(0)}, {}) - assert [r.rule for r in regs] == ["LIT007"] - assert "dropped" in regs[0].detail - - -def test_new_rule_in_head_is_clean(): - assert ratchet.regressions_for("b.json", {}, {"new-rule": _spec_of(5)}) == [] - - -def test_dropped_rule_that_graduated_to_a_hard_failing_config_is_clean(): - base = {"UP006": _spec_of(0)} - assert ratchet.regressions_for("b.json", base, {}, graduated=("UP006",)) == [] - - -def test_graduation_matches_by_prefix_like_ruff_selectors_do(): - base = {"ANN202": _spec_of(865)} - assert ratchet.regressions_for("b.json", base, {}, graduated=("ANN",)) == [] - - -def test_an_unrelated_graduation_does_not_excuse_a_dropped_rule(): - base = {"C901": _spec_of(3)} - regs = ratchet.regressions_for("b.json", base, {}, graduated=("UP006", "SIM118")) - assert [r.rule for r in regs] == ["C901"] - assert "dropped" in regs[0].detail - - -def test_graduation_never_excuses_a_raised_limit(): - base = {"UP006": _spec_of(0)} - regs = ratchet.regressions_for("b.json", base, {"UP006": _spec_of(7)}, graduated=("UP006",)) - assert [r.rule for r in regs] == ["UP006"] - assert "0 -> 7" in regs[0].detail - - -def test_graduated_selectors_come_from_the_paired_ruff_config(): - selectors = ratchet.graduated_selectors("ruff-strict-budget.json") - assert "UP006" in selectors - assert "ANN" not in selectors - - -def test_budgets_without_a_paired_config_can_never_graduate(): - assert ratchet.graduated_selectors("type-discipline-budget.json") == () - assert ratchet.graduated_selectors("basedpyright-code-budget.json") == () - - -def test_a_selector_the_config_also_ignores_does_not_count_as_graduated(): - lint = {"ignore": ["UP006"], "extend-select": ["UP006", "SIM118"]} - assert ratchet.selectors_hard_failed_by(lint) == ("SIM118",) - - -def test_selectors_hard_failed_by_reads_a_config_with_no_ignore_list(): - assert ratchet.selectors_hard_failed_by({"extend-select": ["UP006"]}) == ("UP006",) - - -def test_deleted_budget_file_is_a_regression(): - regs = ratchet.regressions_for("b.json", {"LIT006": _spec_of(1)}, None) - assert [r.rule for r in regs] == ["*"] - assert "deleted" in regs[0].detail - - -def test_new_budget_file_has_nothing_to_ratchet(): - assert ratchet.regressions_for("b.json", None, {"LIT006": _spec_of(1)}) == [] - - -def test_default_budgets_watch_every_budget_file_in_the_repo(): - # This job is the repo's only ceiling-raise alarm, so every *-budget.json on disk must be - # watched; a budget left out of DEFAULT_BUDGETS (e.g. basedpyright-code-budget.json) can be - # loosened with no signal. Equality also catches a phantom entry that no longer exists. - repo_root = _MODULE_PATH.parents[1] - on_disk = frozenset(p.name for p in repo_root.glob("*budget*.json")) - assert on_disk == frozenset(ratchet.DEFAULT_BUDGETS) - - -# --------------------------------------------------------------------------- # -# Base-ref resolution: a bad ref must fail loudly, never pass vacuously -# --------------------------------------------------------------------------- # - - -def test_ref_is_commit_distinguishes_real_from_bogus(): - assert ratchet._ref_is_commit("HEAD") is True - assert ratchet._ref_is_commit("definitely-not-a-real-ref-zzz") is False - - -def test_load_base_reads_a_present_file_and_none_for_an_absent_one(): - # A real budget file exists at HEAD; a made-up path is absent at the same (valid) ref. - assert ratchet._load_base("type-discipline-budget.json", "HEAD") is not None - assert ratchet._load_base("scripts/no-such-budget-xyz.json", "HEAD") is None - - -def test_unresolvable_base_ref_exits_nonzero_instead_of_skipping(): - proc = subprocess.run( - [sys.executable, str(_MODULE_PATH), "--base", "definitely-not-a-real-ref-zzz"], - cwd=_MODULE_PATH.parents[1], - capture_output=True, - text=True, - ) - assert proc.returncode == 1 - assert "does not resolve to a commit" in proc.stderr diff --git a/tests/test_litellm/test_check_type_discipline.py b/tests/test_litellm/test_check_type_discipline.py index 2d49332e687..7e823bac3c0 100644 --- a/tests/test_litellm/test_check_type_discipline.py +++ b/tests/test_litellm/test_check_type_discipline.py @@ -7,7 +7,6 @@ a test fail. The comment-scanner cases are the regression for the readline path: """ import importlib.util -import json import os import re import subprocess @@ -16,6 +15,8 @@ from pathlib import Path import pytest +import type_discipline_gate as gate + _REPO_ROOT = Path(__file__).resolve().parents[2] _MODULE_PATH = _REPO_ROOT / "scripts" / "check_type_discipline.py" _spec = importlib.util.spec_from_file_location("check_type_discipline", _MODULE_PATH) @@ -688,17 +689,16 @@ def test_writable_ok_without_reason_is_lit005_and_does_not_suppress(tmp_path): # --------------------------------------------------------------------------- # -# Budget integrity: every emittable LIT rule (bar the LIT000 read/parse error) is gated +# Gate integrity: headroom only ever names rules the checker can emit # --------------------------------------------------------------------------- # -def test_budget_covers_exactly_the_checker_rules(): - budget = json.loads((_REPO_ROOT / "type-discipline-budget.json").read_text()) +def test_headroom_names_only_rules_the_checker_emits(): emitted = set(re.findall(r"LIT\d{3}", _MODULE_PATH.read_text(encoding="utf-8"))) - {"LIT000"} - assert set(budget) == emitted - for spec in budget.values(): - assert isinstance(spec["limit"], int) - assert spec["limit"] >= 0 + assert set(gate.HEADROOM) <= emitted + for value in gate.HEADROOM.values(): + assert isinstance(value, int) + assert value > 0 _FANS_OUT = checker._worker_count(checker.PARALLEL_MIN_PATHS) > 1 diff --git a/tests/test_litellm/test_default_branch.py b/tests/test_litellm/test_default_branch.py index a1b2a8c5c91..327cc49fe0b 100644 --- a/tests/test_litellm/test_default_branch.py +++ b/tests/test_litellm/test_default_branch.py @@ -28,7 +28,7 @@ def remote_and_clone(tmp_path: Path) -> tuple[Path, Path]: (seed / "scripts").mkdir() for name in ( "default_branch.py", - "budget_ratchet_check.py", + "lint_base_counts.py", "ruff_strict_gate.py", "type_discipline_gate.py", "test_quality_gate.py", @@ -39,11 +39,9 @@ def remote_and_clone(tmp_path: Path) -> tuple[Path, Path]: shutil.copyfile(ROOT / "Makefile", seed / "Makefile") (seed / "litellm").mkdir() (seed / "litellm" / "example.py").write_text("value = 0\n") - (seed / "ruff-strict-budget.json").write_text('{"C901": {"limit": 1}}\n') _commit(seed, "staging base") _git(seed, "checkout", "-qb", "main") (seed / "litellm" / "example.py").write_text("value = 1\n") - (seed / "ruff-strict-budget.json").write_text('{"C901": {"limit": 0}}\n') _commit(seed, "main base") remote: Final = tmp_path / "remote.git" _git(tmp_path, "clone", "-q", "--bare", str(seed), str(remote)) @@ -121,28 +119,6 @@ def test_explicit_base_works_without_remote_access( assert "No changed litellm Python files" in checked.stdout -def test_budget_ratchet_compares_against_new_default(remote_and_clone: tuple[Path, Path]) -> None: - remote, repo = remote_and_clone - _git(remote, "symbolic-ref", "HEAD", "refs/heads/main") - resolved: Final = _resolve(repo) - assert resolved.returncode == 0, resolved.stderr - _git(repo, "checkout", "-qb", "litellm_feature", "origin/main") - (repo / "ruff-strict-budget.json").write_text('{"C901": {"limit": 1}}\n') - command: Final = [sys.executable, "scripts/budget_ratchet_check.py"] - checked: Final = subprocess.run(command, cwd=repo, capture_output=True, text=True, check=False) - assert checked.returncode == 1 - assert "limit raised 0 -> 1" in checked.stdout - assert "base origin/main" in checked.stdout - overridden: Final = subprocess.run( - [*command, "--base", "origin/litellm_internal_staging"], - cwd=repo, - capture_output=True, - text=True, - check=False, - ) - assert overridden.returncode == 0, overridden.stdout + overridden.stderr - - def _freshness(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: return subprocess.run( [ @@ -192,7 +168,6 @@ def test_migration_freshness_refuses_unavailable_remote(remote_and_clone: tuple[ @pytest.mark.parametrize( "gate", [ - "budget_ratchet_check", "ruff_strict_gate", "type_discipline_gate", "test_quality_gate", @@ -213,14 +188,11 @@ def test_each_gate_refuses_an_unverifiable_default(remote_and_clone: tuple[Path, assert "Cannot verify the base branch against origin" in result.stderr -@pytest.mark.parametrize( - "target", ["lint-format-check-changed", "lint-test-quality", "lint-test-quality-budget-update"] -) +@pytest.mark.parametrize("target", ["lint-format-check-changed", "lint-test-quality"]) def test_direct_make_target_fetches_default_once(remote_and_clone: tuple[Path, Path], target: str) -> None: _, repo = remote_and_clone trace: Final = repo.parent / "git-trace.jsonl" shutil.copyfile(ROOT / "scripts" / "check_test_quality.py", repo / "scripts" / "check_test_quality.py") - shutil.copyfile(ROOT / "test-quality-budget.json", repo / "test-quality-budget.json") (repo / "tests").mkdir() result: Final = subprocess.run( ["make", "-o", "install-dev", target, "LINT_DEP_INSTALL=", "UV_RUN=env"], diff --git a/tests/test_litellm/test_lint_base_counts.py b/tests/test_litellm/test_lint_base_counts.py new file mode 100644 index 00000000000..c49bd0a1c6e --- /dev/null +++ b/tests/test_litellm/test_lint_base_counts.py @@ -0,0 +1,443 @@ +"""Tests for scripts/lint_base_counts.py, the merge-base counting shared by the +four lint gates: the ceiling rule with per-rule headroom, the on-disk cache and +its eviction, the CI artifact fetch, the artifact emit, and the merge-base +resolution.""" + +import fnmatch +import io +import json +import os +import subprocess +import zipfile +from pathlib import Path + +import pytest + +import lint_base_counts as counts + +_CHECKER = counts.Checker("basedpyright", ("f1", "f2")) +_OTHER_CHECKER = counts.Checker("ruff-strict", ("f1", "f2")) + + +def test_evaluate_passes_a_rule_that_did_not_grow(): + assert counts.evaluate({"LIT006": 12}, {"LIT006": 12}, {}) == () + + +def test_evaluate_blames_one_new_violation_of_a_zero_headroom_rule(): + assert counts.evaluate({"LIT006": 13}, {"LIT006": 12}, {}) == (counts.Breach("LIT006", 13, 12, 1),) + + +def test_evaluate_lets_a_rule_grow_up_to_its_headroom_and_no_further(): + headroom = {"LIT010": 44} + assert counts.evaluate({"LIT010": 144}, {"LIT010": 100}, headroom) == () + assert counts.evaluate({"LIT010": 145}, {"LIT010": 100}, headroom) == (counts.Breach("LIT010", 145, 144, 45),) + + +def test_evaluate_headroom_applies_only_to_the_rule_it_names(): + assert counts.evaluate({"LIT006": 13}, {"LIT006": 12}, {"LIT010": 44}) == (counts.Breach("LIT006", 13, 12, 1),) + + +def test_evaluate_counts_a_rule_absent_from_the_base_as_zero(): + assert counts.evaluate({"NEW99": 1}, {}, {}) == (counts.Breach("NEW99", 1, 0, 1),) + + +def test_evaluate_never_blames_a_change_that_reduced_a_rule(): + assert counts.evaluate({"LIT006": 11}, {"LIT006": 12}, {}) == () + + +def test_evaluate_reports_every_grown_rule_sorted_by_name(): + head = {"TQ008": 3, "TQ001": 2, "TQ003": 5} + base = {"TQ008": 2, "TQ001": 1, "TQ003": 5} + assert [b.rule for b in counts.evaluate(head, base, {})] == ["TQ001", "TQ008"] + + +def test_evaluate_ignores_a_base_rule_the_head_fixed_entirely(): + assert counts.evaluate({}, {"LIT006": 12}, {}) == () + + +def test_cache_key_changes_with_base_point_and_each_fingerprint(): + key = counts.cache_key("abc", ("cfg", "lock")) + assert counts.cache_key("abc", ("cfg", "lock")) == key + assert counts.cache_key("def", ("cfg", "lock")) != key + assert counts.cache_key("abc", ("cfg2", "lock")) != key + assert counts.cache_key("abc", ("cfg", "lock2")) != key + + +def test_checker_names_its_artifact_and_cache_file_by_the_same_key(): + key = counts.cache_key("abc123", ("f1", "f2")) + assert _CHECKER.artifact_name("abc123") == f"basedpyright-counts-{key}" + assert _CHECKER.cache_file_name("abc123") == f"basedpyright-base-{key}.json" + assert fnmatch.fnmatch(_CHECKER.cache_file_name("abc123"), _CHECKER.cache_glob()) + + +def test_checkers_with_the_same_fingerprints_never_share_a_name(): + assert _CHECKER.artifact_name("abc123") != _OTHER_CHECKER.artifact_name("abc123") + assert not fnmatch.fnmatch(_OTHER_CHECKER.cache_file_name("abc123"), _CHECKER.cache_glob()) + + +def test_cached_counts_round_trip(tmp_path): + path = counts.store_counts(tmp_path, _CHECKER, "abc123", {"reportAny": 3, "reportCall": 1}) + assert path == tmp_path / _CHECKER.cache_file_name("abc123") + assert counts.load_cached_counts(path) == {"reportAny": 3, "reportCall": 1} + + +@pytest.mark.parametrize( + "content", + [ + None, + "{not json", + json.dumps(["counts"]), + json.dumps({"base_point": "abc"}), + json.dumps({"counts": {"reportAny": "three"}}), + json.dumps({"counts": {"reportAny": True}}), + ], +) +def test_missing_corrupt_or_misshapen_cache_reads_as_none(tmp_path, content): + path = tmp_path / "cache.json" + if content is not None: + path.write_text(content) + assert counts.load_cached_counts(path) is None + + +def test_scratch_is_invisible_to_the_prune_glob(): + scratch = counts.scratch_path(Path("/c") / _CHECKER.cache_file_name("abc")) + assert not fnmatch.fnmatch(scratch.name, _CHECKER.cache_glob()) + + +def test_store_prune_spares_a_concurrent_runs_in_flight_scratch(tmp_path): + foreign = counts.scratch_path(tmp_path / _CHECKER.cache_file_name("other")) + foreign.write_text("{}") + mine = counts.store_counts(tmp_path, _CHECKER, "mine", {"reportAny": 1}) + assert foreign.exists() + assert counts.load_cached_counts(mine) == {"reportAny": 1} + + +def test_store_keeps_a_concurrent_worktrees_entry_for_another_branch_point(tmp_path): + old = counts.store_counts(tmp_path, _CHECKER, "old", {"reportAny": 1}) + new = counts.store_counts(tmp_path, _CHECKER, "new", {"reportAny": 2}) + assert counts.load_cached_counts(old) == {"reportAny": 1} + assert counts.load_cached_counts(new) == {"reportAny": 2} + + +def test_store_evicts_only_the_oldest_entries_beyond_the_cap(tmp_path): + aged = tuple( + counts.store_counts(tmp_path, _CHECKER, f"base{age}", {"reportAny": age}) + for age in range(counts.CACHE_KEEP_ENTRIES) + ) + for age, path in enumerate(aged): + os.utime(path, (age, age)) + newest = counts.store_counts(tmp_path, _CHECKER, "newest", {"reportAny": 99}) + assert not aged[0].exists() + assert all(path.exists() for path in aged[1:]) + assert counts.load_cached_counts(newest) == {"reportAny": 99} + + +def test_store_never_evicts_the_entry_it_just_wrote_even_on_mtime_ties(tmp_path): + for index in range(counts.CACHE_KEEP_ENTRIES + 2): + path = counts.store_counts(tmp_path, _CHECKER, f"base{index}", {"reportAny": 1}) + os.utime(path, (9_999_999_999, 9_999_999_999)) + mine = counts.store_counts(tmp_path, _CHECKER, "mine", {"reportAny": 2}) + assert counts.load_cached_counts(mine) == {"reportAny": 2} + assert len(list(tmp_path.glob(_CHECKER.cache_glob()))) == counts.CACHE_KEEP_ENTRIES + + +def test_store_eviction_never_touches_another_checkers_entries(tmp_path): + other = counts.store_counts(tmp_path, _OTHER_CHECKER, "base", {"E501": 1}) + os.utime(other, (1, 1)) + for index in range(counts.CACHE_KEEP_ENTRIES + 1): + counts.store_counts(tmp_path, _CHECKER, f"base{index}", {"reportAny": 1}) + assert counts.load_cached_counts(other) == {"E501": 1} + + +def _no_fetch(checker, base_point): + return None + + +def _never(reason): + def callback(*args): + raise AssertionError(reason) + + return callback + + +def test_base_counts_cached_returns_the_hit_without_recomputing(tmp_path): + counts.store_counts(tmp_path, _CHECKER, "abc123", {"reportAny": 7}) + assert counts.base_counts_cached( + _CHECKER, + "abc123", + _never("a cache hit must not re-run the base pass"), + cache_dir=tmp_path, + fetch=_never("a cache hit must not reach for CI"), + ) == {"reportAny": 7} + + +def test_base_counts_cached_computes_once_then_hits(tmp_path): + calls = [] + + def fake(ref): + calls.append(ref) + return {"reportAny": 4} + + first = counts.base_counts_cached(_CHECKER, "abc123", fake, cache_dir=tmp_path, fetch=_no_fetch) + second = counts.base_counts_cached(_CHECKER, "abc123", fake, cache_dir=tmp_path, fetch=_no_fetch) + assert first == second == {"reportAny": 4} + assert calls == ["abc123"] + + +def test_base_counts_cached_keeps_each_checker_apart(tmp_path): + counts.store_counts(tmp_path, _OTHER_CHECKER, "abc123", {"E501": 7}) + assert counts.base_counts_cached( + _CHECKER, "abc123", lambda ref: {"reportAny": 4}, cache_dir=tmp_path, fetch=_no_fetch + ) == {"reportAny": 4} + + +def test_an_empty_base_pass_is_never_cached(tmp_path): + calls = [] + + def crashed(ref): + calls.append(ref) + return {} + + assert counts.base_counts_cached(_CHECKER, "abc123", crashed, cache_dir=tmp_path, fetch=_no_fetch) == {} + assert counts.base_counts_cached(_CHECKER, "abc123", crashed, cache_dir=tmp_path, fetch=_no_fetch) == {} + assert calls == ["abc123", "abc123"] + assert list(tmp_path.iterdir()) == [] + + +def test_base_counts_cached_uses_fetched_counts_and_persists_them(tmp_path): + fetched = counts.base_counts_cached( + _CHECKER, + "abc123", + _never("fetched counts must skip the local base pass"), + cache_dir=tmp_path, + fetch=lambda checker, base_point: {"reportAny": 9}, + ) + assert fetched == {"reportAny": 9} + assert counts.load_cached_counts(tmp_path / _CHECKER.cache_file_name("abc123")) == {"reportAny": 9} + assert counts.base_counts_cached( + _CHECKER, + "abc123", + _never("the persisted fetch must satisfy later runs"), + cache_dir=tmp_path, + fetch=_never("the persisted fetch must satisfy later runs"), + ) == {"reportAny": 9} + + +def test_base_counts_cached_hands_the_fetcher_the_checker_and_base_point(tmp_path): + seen = [] + + def fetch(checker, base_point): + seen.append((checker, base_point)) + return None + + counts.base_counts_cached(_CHECKER, "abc123", lambda ref: {"reportAny": 4}, cache_dir=tmp_path, fetch=fetch) + assert seen == [(_CHECKER, "abc123")] + + +def test_base_counts_cached_falls_back_to_compute_on_a_fetch_miss(tmp_path): + calls = [] + + def local(ref): + calls.append(ref) + return {"reportAny": 4} + + assert counts.base_counts_cached(_CHECKER, "abc123", local, cache_dir=tmp_path, fetch=_no_fetch) == { + "reportAny": 4 + } + assert calls == ["abc123"] + + +def test_base_counts_cached_treats_empty_fetched_counts_as_a_miss(tmp_path): + assert counts.base_counts_cached( + _CHECKER, + "abc123", + lambda ref: {"reportAny": 2}, + cache_dir=tmp_path, + fetch=lambda checker, base_point: {}, + ) == {"reportAny": 2} + assert counts.load_cached_counts(tmp_path / _CHECKER.cache_file_name("abc123")) == {"reportAny": 2} + + +def test_origin_slug_parsing_supports_ssh_and_https_github_forms(): + assert counts.parse_origin_slug("git@github.com:BerriAI/litellm.git") == "BerriAI/litellm" + assert counts.parse_origin_slug("git@github.com:BerriAI/litellm") == "BerriAI/litellm" + assert counts.parse_origin_slug("https://github.com/BerriAI/litellm.git") == "BerriAI/litellm" + assert counts.parse_origin_slug("https://github.com/BerriAI/litellm") == "BerriAI/litellm" + assert counts.parse_origin_slug("https://github.com/BerriAI/litellm/") == "BerriAI/litellm" + + +def test_origin_slug_parsing_rejects_non_github_urls(): + assert counts.parse_origin_slug("https://gitlab.com/BerriAI/litellm.git") is None + assert counts.parse_origin_slug("git@bitbucket.org:BerriAI/litellm.git") is None + assert counts.parse_origin_slug("not a url") is None + assert counts.parse_origin_slug("") is None + + +def _artifact_zip(payload): + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("counts.json", json.dumps(payload)) + return buffer.getvalue() + + +def _gh_stub(listing, zip_bytes, seen=None): + def gh_output(args): + if seen is not None: + seen.append(tuple(args)) + if args[-1].startswith("repos/"): + return json.dumps(listing).encode() + return zip_bytes + + return gh_output + + +def _live_listing(): + return {"artifacts": [{"expired": False, "archive_download_url": "https://api.github.com/x/zip"}]} + + +def test_fetcher_returns_counts_from_a_matching_artifact(capsys): + payload = {"base_point": "abc123", "counts": {"reportAny": 3}} + fetched = counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(_live_listing(), _artifact_zip(payload))) + assert fetched == {"reportAny": 3} + assert "fetched from CI artifact" in capsys.readouterr().err + + +def test_fetcher_asks_for_the_artifact_named_by_the_checker_and_base_point(): + seen = [] + payload = {"base_point": "abc123", "counts": {"reportAny": 3}} + counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(_live_listing(), _artifact_zip(payload), seen)) + listing_request = seen[0][-1] + assert f"name={_CHECKER.artifact_name('abc123')}" in listing_request + assert seen[1][-1] == "https://api.github.com/x/zip" + + +def test_fetcher_rejects_an_artifact_for_a_different_base_point(): + payload = {"base_point": "someothersha", "counts": {"reportAny": 3}} + assert ( + counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(_live_listing(), _artifact_zip(payload))) is None + ) + + +@pytest.mark.parametrize("bad_counts", [{}, {"reportAny": "three"}, {"reportAny": True}]) +def test_fetcher_rejects_empty_or_misshapen_artifact_counts(bad_counts): + payload = {"base_point": "abc123", "counts": bad_counts} + assert ( + counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(_live_listing(), _artifact_zip(payload))) is None + ) + + +def test_fetcher_rejects_an_expired_artifact(): + listing = {"artifacts": [{"expired": True, "archive_download_url": "https://api.github.com/x/zip"}]} + payload = {"base_point": "abc123", "counts": {"reportAny": 3}} + assert counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(listing, _artifact_zip(payload))) is None + + +def test_fetcher_misses_when_no_artifact_is_published(): + assert counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub({"artifacts": []}, b"")) is None + + +def test_fetcher_misses_when_gh_is_unusable(capsys): + assert counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=lambda args: None) is None + assert "computing base counts locally" in capsys.readouterr().err + + +def test_fetcher_misses_on_a_corrupt_artifact_archive(): + assert counts.fetch_ci_base_counts(_CHECKER, "abc123", gh=_gh_stub(_live_listing(), b"not a zip")) is None + + +def test_emit_writes_the_artifact_json_named_by_the_head_key(tmp_path, capsys): + counts.emit_counts(_CHECKER, {"reportAny": 3, "aRule": 1}, tmp_path, "deadbeef") + name = _CHECKER.artifact_name("deadbeef") + assert json.loads((tmp_path / f"{name}.json").read_text()) == { + "base_point": "deadbeef", + "counts": {"aRule": 1, "reportAny": 3}, + } + summary = capsys.readouterr().out + assert "deadbeef" in summary + assert name in summary + assert "4" in summary + + +def test_emit_refuses_to_publish_empty_counts(tmp_path): + with pytest.raises(SystemExit): + counts.emit_counts(_CHECKER, {}, tmp_path, "deadbeef") + assert list(tmp_path.iterdir()) == [] + + +def test_emitted_file_is_the_one_the_fetcher_looks_up(tmp_path): + written = counts.emit_counts(_CHECKER, {"reportAny": 3}, tmp_path, "deadbeef") + payload = json.loads(written.read_text()) + assert counts.counts_for_base(payload, "deadbeef") == {"reportAny": 3} + assert counts.counts_for_base(payload, "someothersha") is None + listing_zip = _artifact_zip(payload) + assert counts.fetch_ci_base_counts(_CHECKER, "deadbeef", gh=_gh_stub(_live_listing(), listing_zip)) == { + "reportAny": 3 + } + + +def _git(cwd, *args): + proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) + assert proc.returncode == 0, proc.stderr + return proc.stdout.strip() + + +def _commit(cwd, name): + (cwd / name).write_text(name) + _git(cwd, "add", "-A") + _git(cwd, "commit", "-q", "-m", name) + return _git(cwd, "rev-parse", "HEAD") + + +def _init_repo(tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q", "-b", "main") + _git(repo, "config", "user.email", "gate@example.com") + _git(repo, "config", "user.name", "gate") + _git(repo, "config", "commit.gpgsign", "false") + return repo + + +def _branched_repo(tmp_path): + repo = _init_repo(tmp_path) + branch_point = _commit(repo, "shared.txt") + _git(repo, "checkout", "-q", "-b", "feature") + _commit(repo, "feature.txt") + _git(repo, "checkout", "-q", "main") + base_tip = _commit(repo, "drift.txt") + _git(repo, "checkout", "-q", "feature") + return repo, branch_point, base_tip + + +def test_base_point_is_the_branch_point_when_no_merge_is_in_progress(tmp_path): + repo, branch_point, _ = _branched_repo(tmp_path) + assert counts.resolve_base_point("main", cwd=repo) == branch_point + + +def test_base_point_mid_merge_advances_to_the_merged_in_base_tip(tmp_path): + repo, _, base_tip = _branched_repo(tmp_path) + _git(repo, "merge", "--no-commit", "--no-ff", "main") + assert counts.resolve_base_point("main", cwd=repo) == base_tip + + +def test_base_point_mid_merge_of_an_older_side_branch_keeps_the_newer_branch_point(tmp_path): + repo = _init_repo(tmp_path) + _commit(repo, "shared.txt") + _git(repo, "checkout", "-q", "-b", "old-side") + _commit(repo, "old.txt") + _git(repo, "checkout", "-q", "main") + newer_point = _commit(repo, "drift.txt") + _git(repo, "checkout", "-q", "-b", "feature") + _commit(repo, "feature.txt") + _git(repo, "merge", "--no-commit", "--no-ff", "old-side") + assert counts.resolve_base_point("main", cwd=repo) == newer_point + + +def test_head_sha_is_the_checked_out_commit(tmp_path): + repo, _, _ = _branched_repo(tmp_path) + assert counts.head_sha(cwd=repo) == _git(repo, "rev-parse", "HEAD") + + +def test_default_cache_dir_lives_under_the_shared_git_dir(tmp_path): + repo, _, _ = _branched_repo(tmp_path) + assert counts.default_cache_dir(cwd=repo) == repo / ".git" / counts.CACHE_DIR_NAME diff --git a/tests/test_litellm/test_pre_commit_lint.py b/tests/test_litellm/test_pre_commit_lint.py index e12da0833dc..ad47be527c2 100644 --- a/tests/test_litellm/test_pre_commit_lint.py +++ b/tests/test_litellm/test_pre_commit_lint.py @@ -12,9 +12,9 @@ import pytest ROOT = Path(__file__).resolve().parents[2] SCRIPT = ROOT / "scripts" / "pre_commit_lint.sh" WHOLE_TREE_RUFF = "run --no-sync ruff check --config ruff-tests.toml tests" -TEST_TREE_RAN = "ran: test-tree lint (ruff-tests.toml + test-quality budget)" +TEST_TREE_RAN = "ran: test-tree lint (ruff-tests.toml + test-quality gate)" TEST_TREE_SKIPPED = ( - "skipped: test-tree lint (ruff-tests.toml + test-quality budget) " + "skipped: test-tree lint (ruff-tests.toml + test-quality gate) " "(no tests/ Python files or test-tree lint inputs in scope)" ) @@ -475,7 +475,6 @@ def test_tests_only_change_runs_the_whole_test_tree_ruff_and_the_quality_gate(tm "changed", [ "ruff-tests.toml", - "test-quality-budget.json", "scripts/check_test_quality.py", "scripts/test_quality_gate.py", "tests/e2e/test_x.py", @@ -528,7 +527,7 @@ def test_a_failing_quality_gate_fails_a_tests_only_run(tmp_path: Path) -> None: _stage_file(repo, "tests/test_a.py", "def test_a() -> None: ...\n") proc = _run(repo, bin_dir, {"STUB_FAIL": "test-quality"}) assert proc.returncode == 1 - assert "Test-quality budget failed" in proc.stdout + proc.stderr + assert "Test-quality gate failed" in proc.stdout + proc.stderr assert "check: FAIL" in proc.stdout @@ -571,7 +570,7 @@ def test_partial_staging_warns_when_test_files_are_left_unstaged(tmp_path: Path) (repo / "tests" / "test_a.py").write_text("def test_a() -> None:\n assert True\n") proc = _run(repo, bin_dir, {"STUB_ARGS_DIR": str(args_dir)}) assert proc.returncode == 0, proc.stdout + proc.stderr - assert "SKIPPED test-tree lint (ruff-tests.toml + test-quality budget)" in proc.stdout + assert "SKIPPED test-tree lint (ruff-tests.toml + test-quality gate)" in proc.stdout assert "tests/test_a.py" in proc.stdout assert _recorded(args_dir, "ruff_tests.args") == [] assert _recorded(args_dir, "make.args") == [] diff --git a/tests/test_litellm/test_ruff_strict_gate.py b/tests/test_litellm/test_ruff_strict_gate.py index 8fa9a18cf53..5b28aae9d3d 100644 --- a/tests/test_litellm/test_ruff_strict_gate.py +++ b/tests/test_litellm/test_ruff_strict_gate.py @@ -1,4 +1,3 @@ -import importlib.util import json import re import shutil @@ -8,80 +7,21 @@ from pathlib import Path import pytest +import lint_base_counts +import ruff_strict_gate as gate + if sys.version_info >= (3, 11): import tomllib else: import tomli as tomllib _REPO_ROOT = Path(__file__).resolve().parents[2] -_MODULE_PATH = _REPO_ROOT / "scripts" / "ruff_strict_gate.py" -_spec = importlib.util.spec_from_file_location("ruff_strict_gate", _MODULE_PATH) -gate = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(gate) Violation = gate.Violation _ENABLED_BY_RUFF_DEFAULTS = frozenset({"F401"}) -def rule(name, limit): - return {name: {"limit": limit}} - - -def test_under_ceiling_passes(): - assert gate.evaluate({"ANN001": 100}, {"ANN001": 100}, rule("ANN001", 110)) == [] - - -def test_ceiling_is_the_limit_boundary(): - budget = rule("ANN001", 110) - at = gate.evaluate({"ANN001": 110}, {"ANN001": 90}, budget) - over = gate.evaluate({"ANN001": 111}, {"ANN001": 90}, budget) - assert at == [] - assert [b.rule for b in over] == ["ANN001"] - assert over[0].cap == 110 - assert over[0].added == 21 - - -def test_over_ceiling_and_change_added_fails(): - breaches = gate.evaluate({"C901": 11}, {"C901": 9}, rule("C901", 10)) - assert [b.rule for b in breaches] == ["C901"] - assert breaches[0].added == 2 - - -def test_base_already_over_ceiling_change_added_nothing_is_not_blamed(): - # drift safety: base is over limit, this change leaves the count where it is - assert gate.evaluate({"C901": 15}, {"C901": 15}, rule("C901", 10)) == [] - - -def test_change_that_reduces_an_over_ceiling_rule_is_not_blamed(): - # still over limit, but moving the right direction - assert gate.evaluate({"C901": 14}, {"C901": 16}, rule("C901", 10)) == [] - - -def test_rules_are_independent(): - budget = {**rule("ANN001", 150), **rule("C901", 10)} - breaches = gate.evaluate( - {"ANN001": 130, "C901": 11}, {"ANN001": 100, "C901": 10}, budget - ) - assert [b.rule for b in breaches] == ["C901"] # ANN001 130 <= 150, C901 11 > 10 - - -def test_missing_rule_counts_as_zero(): - assert gate.evaluate({}, {}, rule("C901", 0)) == [] - - -def test_update_ratchets_limit_down_by_what_the_branch_fixed_never_up(): - budget = {**rule("ANN001", 150), **rule("C901", 10)} - # ANN001 fixed 20 (100 -> 80) so its limit falls 150 -> 130; C901 grew, so its - # limit holds flat at 10 (a fix must never loosen a ceiling). - current = {"ANN001": 80, "C901": 12} - base = {"ANN001": 100, "C901": 9} - assert gate.ratcheted_budget(budget, current, base) == { - "ANN001": {"limit": 130}, - "C901": {"limit": 10}, - } - - def test_parse_changed_lines_maps_added_lines_per_file(): diff = ( "+++ b/litellm/a.py\n" @@ -109,62 +49,6 @@ def test_parse_changed_lines_handles_single_and_ranged_hunks(hunk): assert gate.parse_changed_lines(f"+++ b/litellm/a.py\n{hunk}\n")["litellm/a.py"] -def test_over_ceiling_flags_only_counts_above_the_limit(): - budget = rule("C901", 10) - assert gate.over_ceiling({"C901": 10}, budget) == frozenset() - assert gate.over_ceiling({"C901": 11}, budget) == frozenset({"C901"}) - assert gate.over_ceiling({}, budget) == frozenset() - - -def test_over_ceiling_ignores_rules_missing_from_the_budget(): - assert gate.over_ceiling({"NEW99": 100}, rule("C901", 10)) == frozenset() - - -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 _git(cwd, *args): - proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) - assert proc.returncode == 0, proc.stderr - return proc.stdout.strip() - - -def _commit(cwd, name): - (cwd / name).write_text(name) - _git(cwd, "add", "-A") - _git(cwd, "commit", "-q", "-m", name) - return _git(cwd, "rev-parse", "HEAD") - - -def _branched_repo(tmp_path): - repo = tmp_path / "repo" - repo.mkdir() - _git(repo, "init", "-q", "-b", "main") - _git(repo, "config", "user.email", "gate@example.com") - _git(repo, "config", "user.name", "gate") - _git(repo, "config", "commit.gpgsign", "false") - branch_point = _commit(repo, "shared.txt") - _git(repo, "checkout", "-q", "-b", "feature") - _commit(repo, "feature.txt") - _git(repo, "checkout", "-q", "main") - base_tip = _commit(repo, "drift.txt") - _git(repo, "checkout", "-q", "feature") - return repo, branch_point, base_tip - - -def test_base_point_is_the_branch_point_when_no_merge_is_in_progress(tmp_path): - repo, branch_point, _ = _branched_repo(tmp_path) - assert gate.resolve_base_point("main", cwd=repo) == branch_point - - -def test_base_point_mid_merge_advances_to_the_merged_in_base_tip(tmp_path): - repo, _, base_tip = _branched_repo(tmp_path) - _git(repo, "merge", "--no-commit", "--no-ff", "main") - assert gate.resolve_base_point("main", cwd=repo) == base_tip - - def _lint_section(config_name: str) -> dict: return tomllib.loads((_REPO_ROOT / config_name).read_text())["lint"] @@ -189,10 +73,6 @@ def _selected_by_the_normal_config() -> frozenset: return frozenset(_lint_section("ruff.toml")["extend-select"]) | _ENABLED_BY_RUFF_DEFAULTS -def _budgeted_rules() -> frozenset: - return frozenset(json.loads((_REPO_ROOT / "ruff-strict-budget.json").read_text())) - - def _ruff_binary() -> str | None: beside_interpreter = Path(sys.executable).with_name("ruff") return str(beside_interpreter) if beside_interpreter.exists() else shutil.which("ruff") @@ -302,37 +182,6 @@ def test_every_base_owned_rule_is_external_or_selected_in_the_strict_config(all_ ) -def test_every_budgeted_rule_is_one_the_gate_actually_measures(): - selectors = tuple(_lint_section("ruff-strict.toml")["select"]) - unmeasured = frozenset(code for code in _budgeted_rules() if not code.startswith(selectors)) - assert unmeasured == frozenset(), ( - f"the gate never counts {sorted(unmeasured)}, so their ceilings are dead config that reads " - "as coverage. Either select them in ruff-strict.toml or drop them from the budget." - ) - - -@_needs_ruff -def test_every_strict_selected_rule_is_budgeted_or_hard_failed_by_the_base_config(all_ruff_rule_codes): - strict_enabled = frozenset( - code - for code in all_ruff_rule_codes - if code.startswith(tuple(_lint_section("ruff-strict.toml")["select"])) - ) - base_hard_failed = tuple(_lint_section("ruff.toml")["extend-select"]) - unpoliced = frozenset( - code - for code in strict_enabled - if code not in _budgeted_rules() - and not code.startswith(base_hard_failed) - and code not in _ENABLED_BY_RUFF_DEFAULTS - ) - assert unpoliced == frozenset(), ( - f"nothing enforces {sorted(unpoliced)}: the gate skips rules missing from the budget, and " - "the base config does not hard-fail them. Re-add a budget ceiling or graduate them into " - "ruff.toml's lint.extend-select." - ) - - @_needs_ruff def test_a_noqa_for_a_strict_gate_rule_survives_the_normal_ruff_run(): assert "RUF100" not in _ruff_output_for_noqa("ANN202") @@ -373,3 +222,23 @@ def test_a_graduated_rule_can_still_be_suppressed_without_tripping_unused_noqa() output = _ruff_output_for_source(suppressed) assert "UP006" not in output assert "RUF100" not in output + + +@_needs_ruff +def test_the_checker_identity_is_rekeyed_by_either_ruff_config_or_the_ruff_version(): + identity = gate.checker_identity() + assert identity.name == "ruff-strict" + assert identity.fingerprints == ( + lint_base_counts.sha256_of(gate.STRICT_CONFIG), + lint_base_counts.sha256_of(gate.BASE_CONFIG), + gate.ruff_version(), + ) + + +def test_every_headroom_rule_is_one_the_strict_config_selects(): + selectors = tuple(_strict_selected()) + unmeasured = frozenset(code for code in gate.HEADROOM if not code.startswith(selectors)) + assert unmeasured == frozenset(), ( + f"the gate never counts {sorted(unmeasured)}, so headroom for them is dead config that reads " + "as coverage. Either select them in ruff-strict.toml or drop the headroom entry." + ) diff --git a/tests/test_litellm/test_test_quality_gate.py b/tests/test_litellm/test_test_quality_gate.py index 6652211a828..ae60eacb46c 100644 --- a/tests/test_litellm/test_test_quality_gate.py +++ b/tests/test_litellm/test_test_quality_gate.py @@ -1,12 +1,12 @@ """Tests for scripts/test_quality_gate.py. -The gate's whole value is that it blames a change only for what it adds and that a -limit can never rise. Both live in pure functions, so they are tested directly: -`evaluate` for the blame rule, `ratcheted_budget` for the one-way ratchet, and -`parse_changed_lines` for the diff scan that turns a breach into file:line. +The gate's whole value is that it blames a change only for what it adds. That lives +in pure functions, so they are tested directly: `lint_base_counts.evaluate` for the +blame rule and `parse_changed_lines` for the diff scan that turns a breach into +file:line. The base scan spawns a worktree and a checker subprocess, so its cleanup +on termination is driven end to end. """ -import importlib.util import os import signal import subprocess @@ -17,75 +17,48 @@ from contextlib import suppress from pathlib import Path from typing import NamedTuple +import lint_base_counts +import test_quality_gate as gate + _REPO_ROOT = Path(__file__).resolve().parents[2] _MODULE_PATH = _REPO_ROOT / "scripts" / "test_quality_gate.py" -_spec = importlib.util.spec_from_file_location("test_quality_gate", _MODULE_PATH) -gate = importlib.util.module_from_spec(_spec) -# @dataclass(slots=True) rebuilds its class through sys.modules[__module__], so the -# module has to be registered before exec_module runs or Scope fails to construct. -sys.modules[_spec.name] = gate -_spec.loader.exec_module(gate) - -_BUDGET = {"TQ001": {"limit": 10}, "TQ003": {"limit": 5}} _SCAN_BASE = ( - "import importlib.util, pathlib, sys\n" - "spec = importlib.util.spec_from_file_location('test_quality_gate', sys.argv[1])\n" - "gate = importlib.util.module_from_spec(spec)\n" - "sys.modules[spec.name] = gate\n" - "spec.loader.exec_module(gate)\n" - "gate.base_counts('HEAD', repo_root=pathlib.Path(sys.argv[2]), checker=pathlib.Path(sys.argv[3]))\n" + "import pathlib, sys\n" + "import test_quality_gate as gate\n" + "gate.base_counts('HEAD', repo_root=pathlib.Path(sys.argv[1]), checker=pathlib.Path(sys.argv[2]))\n" ) _SCAN_BASE_WITH_SIGHUP_IGNORED = "import signal\nsignal.signal(signal.SIGHUP, signal.SIG_IGN)\n" + _SCAN_BASE -def test_a_rule_within_its_limit_is_not_a_breach(): - assert gate.evaluate({"TQ001": 10}, {"TQ001": 10}, _BUDGET) == () +def test_a_rule_that_did_not_grow_is_not_a_breach(): + assert lint_base_counts.evaluate({"TQ001": 10}, {"TQ001": 10}, {}) == () -def test_a_rule_over_its_limit_that_the_change_added_is_a_breach(): - breaches = gate.evaluate({"TQ001": 12}, {"TQ001": 10}, _BUDGET) +def test_a_rule_the_change_grew_is_a_breach_for_exactly_what_it_added(): + breaches = lint_base_counts.evaluate({"TQ001": 12}, {"TQ001": 10}, {}) assert [(b.rule, b.total, b.cap, b.added) for b in breaches] == [("TQ001", 12, 10, 2)] def test_drift_already_in_the_base_is_not_blamed_on_the_change(): - assert gate.evaluate({"TQ001": 14}, {"TQ001": 14}, _BUDGET) == () + assert lint_base_counts.evaluate({"TQ001": 14}, {"TQ001": 14}, {}) == () -def test_a_change_that_reduces_an_over_limit_rule_is_not_blamed(): - assert gate.evaluate({"TQ001": 13}, {"TQ001": 14}, _BUDGET) == () +def test_a_change_that_reduces_a_rule_is_not_blamed(): + assert lint_base_counts.evaluate({"TQ001": 13}, {"TQ001": 14}, {}) == () def test_a_rule_absent_from_head_counts_as_zero(): - assert gate.evaluate({}, {}, _BUDGET) == () + assert lint_base_counts.evaluate({}, {}, {}) == () -def test_over_ceiling_names_only_the_rules_above_their_limit(): - assert gate.over_ceiling({"TQ001": 11, "TQ003": 5}, _BUDGET) == frozenset({"TQ001"}) +def test_the_gate_grants_no_headroom_to_any_rule(): + assert dict(gate.HEADROOM) == {} -def test_over_ceiling_is_empty_when_everything_fits(): - assert gate.over_ceiling({"TQ001": 10, "TQ003": 4}, _BUDGET) == frozenset() - - -def test_ratchet_lowers_a_limit_by_what_the_branch_fixed(): - updated = gate.ratcheted_budget(_BUDGET, {"TQ001": 6}, {"TQ001": 10}) - assert updated["TQ001"]["limit"] == 6 - - -def test_ratchet_never_raises_a_limit_when_violations_grew(): - updated = gate.ratcheted_budget(_BUDGET, {"TQ001": 20}, {"TQ001": 10}) - assert updated["TQ001"]["limit"] == 10 - - -def test_ratchet_never_goes_below_zero(): - updated = gate.ratcheted_budget({"TQ001": {"limit": 2}}, {"TQ001": 0}, {"TQ001": 100}) - assert updated["TQ001"]["limit"] == 0 - - -def test_ratchet_lowers_a_rule_introduced_on_this_branch_like_any_other(): - updated = gate.ratcheted_budget(_BUDGET, {"TQ001": 4}, {"TQ001": 10}) - assert updated["TQ001"]["limit"] == 4 +def test_the_checker_identity_is_keyed_on_the_checker_source(): + identity = gate.checker_identity() + assert identity == lint_base_counts.Checker("test-quality", (lint_base_counts.sha256_of(gate.CHECKER),)) def test_parse_changed_lines_groups_hunks_under_their_own_file(): @@ -132,14 +105,6 @@ def test_introduced_keeps_only_violations_on_changed_lines(): assert kept == (gate.Violation("tests/a.py", 3, "TQ001"),) -def test_the_shipped_budget_covers_every_rule_the_checker_can_emit(): - import json - - budget = json.loads((_REPO_ROOT / "test-quality-budget.json").read_text()) - assert set(budget) == {"TQ001", "TQ002", "TQ003", "TQ004", "TQ005", "TQ006", "TQ007", "TQ008"} - assert all(spec["limit"] >= 0 for spec in budget.values()) - - def _git(cwd: Path, *args: str) -> str: proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) assert proc.returncode == 0, proc.stderr @@ -202,7 +167,8 @@ def _base_scan_stalled_in_its_checker(tmp_path: Path, driver: str) -> _StalledSc temp_dir = tmp_path / "tmp" temp_dir.mkdir() scan = subprocess.Popen( - [sys.executable, "-c", driver, str(_MODULE_PATH), str(repo), str(slow_checker)], + [sys.executable, "-c", driver, str(repo), str(slow_checker)], + cwd=_MODULE_PATH.parent, env={**os.environ, "TMPDIR": str(temp_dir)}, ) if not _wait_until(scanning.exists, 30): diff --git a/tests/test_litellm/test_type_check_gate.py b/tests/test_litellm/test_type_check_gate.py index d104e4ca0c8..d54e0a0b406 100644 --- a/tests/test_litellm/test_type_check_gate.py +++ b/tests/test_litellm/test_type_check_gate.py @@ -1,14 +1,11 @@ import hashlib -import importlib.util import json -import os -import subprocess from pathlib import Path -_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) -_spec.loader.exec_module(gate) +import pytest + +import lint_base_counts +import type_check_gate as gate ROOT = gate.REPO_ROOT @@ -132,99 +129,6 @@ def test_run_basedpyright_fails_loudly_on_a_crash_exit_code(tmp_path): gate.run_basedpyright(cwd=tmp_path, env_dir=env_dir) -def test_at_or_under_ceiling_passes(): - budget = {"no-any-return": {"limit": 5}} - assert gate.evaluate({"no-any-return": 5}, {}, budget) == [] - - -def test_one_more_error_than_ceiling_fails(): - budget = {"no-any-return": {"limit": 5}} - assert gate.evaluate({"no-any-return": 6}, {}, budget) == [ - gate.Breach("no-any-return", 6, 5, 6) - ] - - -def test_limit_absorbs_increase_up_to_it_then_fails_past_it(): - budget = {"arg-type": {"limit": 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_limit(): - assert gate.evaluate({"brand-new": gate.DEFAULT_LIMIT}, {}, {}) == [] - assert gate.evaluate({"brand-new": gate.DEFAULT_LIMIT + 1}, {}, {}) == [ - gate.Breach( - "brand-new", - gate.DEFAULT_LIMIT + 1, - gate.DEFAULT_LIMIT, - gate.DEFAULT_LIMIT + 1, - ) - ] - - -def test_drift_already_over_cap_in_base_is_not_blamed_on_a_flat_change(): - # The bystander case: a rule sits over its limit because two earlier PRs - # summed past it. A PR that branches off that base and adds nothing must pass - # -- total > limit but total == base, so the `> base` guard spares it. - budget = {"arg-type": {"limit": 10}} - 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 limit 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": {"limit": 10}} - 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": {"limit": 10}} - 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": {"limit": 4898}} - assert gate.is_vacuous_run({}, budget) is True - - -def test_genuine_zero_and_empty_budget_are_not_vacuous(): - assert gate.is_vacuous_run({}, {}) is False - assert gate.is_vacuous_run({}, {"no-untyped-def": {"limit": 0}}) is False - assert ( - gate.is_vacuous_run({"arg-type": 1}, {"arg-type": {"limit": 10}}) is False - ) - - -def test_update_ratchets_a_limit_down_by_what_the_branch_fixed(): - # A rule that dropped from 40 (branch point) to 30 (current) fixed 10, so its - # limit of 100 falls to 90 -- the granted headroom (60) is preserved, not the - # raw count. - budget = {"reportAny": {"limit": 100}} - assert gate.ratcheted_budget(budget, {"reportAny": 30}, {"reportAny": 40}) == { - "reportAny": {"limit": 90} - } - - -def test_update_never_raises_a_limit_when_a_rule_grows(): - # Adding violations must not loosen the ceiling; the limit holds flat. - budget = {"reportAny": {"limit": 100}} - assert gate.ratcheted_budget(budget, {"reportAny": 55}, {"reportAny": 40}) == { - "reportAny": {"limit": 100} - } - - -def test_update_clamps_a_limit_at_zero_never_negative(): - budget = {"reportAny": {"limit": 5}} - assert gate.ratcheted_budget(budget, {"reportAny": 0}, {"reportAny": 40}) == { - "reportAny": {"limit": 0} - } - - def test_malformed_basedpyright_json_exits_loudly_not_as_zero_errors(): import pytest @@ -238,35 +142,6 @@ def test_empty_basedpyright_payload_counts_zero(): assert gate.count_basedpyright("") == {} -def test_over_ceiling_flags_only_rules_above_their_limit(): - budget = {"reportAny": {"limit": 10}} - assert gate.over_ceiling({"reportAny": 10}, budget) == frozenset() - assert gate.over_ceiling({"reportAny": 11}, budget) == frozenset({"reportAny"}) - assert gate.over_ceiling({}, budget) == frozenset() - - -def test_over_ceiling_holds_unbudgeted_rules_to_the_default_limit(): - assert gate.over_ceiling({"brand-new": gate.DEFAULT_LIMIT}, {}) == frozenset() - assert gate.over_ceiling({"brand-new": gate.DEFAULT_LIMIT + 1}, {}) == frozenset( - {"brand-new"} - ) - - -def test_over_ceiling_is_independent_across_rules(): - budget = {"reportAny": {"limit": 10}, "reportArgumentType": {"limit": 5}} - assert gate.over_ceiling( - {"reportAny": 9, "reportArgumentType": 6}, budget - ) == frozenset({"reportArgumentType"}) - - -def test_cache_key_changes_with_base_point_and_each_fingerprint(): - key = gate.cache_key("abc", ("cfg", "lock")) - assert gate.cache_key("abc", ("cfg", "lock")) == key - assert gate.cache_key("def", ("cfg", "lock")) != key - assert gate.cache_key("abc", ("cfg2", "lock")) != key - assert gate.cache_key("abc", ("cfg", "lock2")) != key - - def test_fingerprints_carry_the_dependency_group_set(): # Counts measured under one group set must never be compared against # another's: the fingerprint difference re-keys every cache entry and @@ -350,381 +225,17 @@ def test_ensure_env_is_silent_when_the_env_already_exists(tmp_path, capsys): assert capsys.readouterr().err == "" -def test_cached_counts_round_trip(tmp_path): - path = gate.cache_path(tmp_path, "abc123", ("f1", "f2")) - gate.store_counts(tmp_path, path, "abc123", {"reportAny": 3, "reportCall": 1}) - assert gate.load_cached_counts(path) == {"reportAny": 3, "reportCall": 1} +def test_the_checker_identity_is_keyed_on_the_environment_fingerprints(): + assert gate.checker_identity() == lint_base_counts.Checker("basedpyright", gate.environment_fingerprints()) -def test_missing_corrupt_or_misshapen_cache_reads_as_none(tmp_path): - path = tmp_path / "cache.json" - assert gate.load_cached_counts(path) is None - path.write_text("{not json") - assert gate.load_cached_counts(path) is None - path.write_text(json.dumps(["counts"])) - assert gate.load_cached_counts(path) is None - path.write_text(json.dumps({"base_point": "abc"})) - assert gate.load_cached_counts(path) is None - path.write_text(json.dumps({"counts": {"reportAny": "three"}})) - assert gate.load_cached_counts(path) is None - path.write_text(json.dumps({"counts": {"reportAny": True}})) - assert gate.load_cached_counts(path) is None +def test_headroom_is_kept_only_for_the_any_discipline_rules(): + assert set(gate.HEADROOM) == {"reportAny", "reportExplicitAny"} + assert all(isinstance(value, int) and value > 0 for value in gate.HEADROOM.values()) -def test_scratch_is_invisible_to_the_prune_glob(): - import fnmatch - - scratch = gate.scratch_path(gate.cache_path(Path("/c"), "abc", ("f",))) - assert not fnmatch.fnmatch(scratch.name, f"{gate.CACHE_FILE_PREFIX}*") - - -def test_store_prune_spares_a_concurrent_runs_in_flight_scratch(tmp_path): - foreign = gate.scratch_path(gate.cache_path(tmp_path, "other", ("f",))) - foreign.parent.mkdir(parents=True, exist_ok=True) - foreign.write_text("{}") - mine = gate.cache_path(tmp_path, "mine", ("f",)) - gate.store_counts(tmp_path, mine, "mine", {"reportAny": 1}) - assert foreign.exists() - assert gate.load_cached_counts(mine) == {"reportAny": 1} - - -def test_store_keeps_a_concurrent_worktrees_entry_for_another_branch_point(tmp_path): - old = gate.cache_path(tmp_path, "old", ("f",)) - gate.store_counts(tmp_path, old, "old", {"reportAny": 1}) - new = gate.cache_path(tmp_path, "new", ("f",)) - gate.store_counts(tmp_path, new, "new", {"reportAny": 2}) - assert gate.load_cached_counts(old) == {"reportAny": 1} - assert gate.load_cached_counts(new) == {"reportAny": 2} - - -def test_store_evicts_only_the_oldest_entries_beyond_the_cap(tmp_path): - aged = [ - gate.cache_path(tmp_path, f"base{i}", ("f",)) - for i in range(gate.CACHE_KEEP_ENTRIES) - ] - for age, path in enumerate(aged): - gate.store_counts(tmp_path, path, f"base{age}", {"reportAny": age}) - os.utime(path, (age, age)) - newest = gate.cache_path(tmp_path, "newest", ("f",)) - gate.store_counts(tmp_path, newest, "newest", {"reportAny": 99}) - assert not aged[0].exists() - assert all(path.exists() for path in aged[1:]) - assert gate.load_cached_counts(newest) == {"reportAny": 99} - - -def test_store_never_evicts_the_entry_it_just_wrote_even_on_mtime_ties(tmp_path): - others = [ - gate.cache_path(tmp_path, f"base{i}", ("f",)) - for i in range(gate.CACHE_KEEP_ENTRIES + 2) - ] - for path in others: - gate.store_counts(tmp_path, path, path.name, {"reportAny": 1}) - os.utime(path, (9_999_999_999, 9_999_999_999)) - mine = gate.cache_path(tmp_path, "mine", ("f",)) - gate.store_counts(tmp_path, mine, "mine", {"reportAny": 2}) - assert gate.load_cached_counts(mine) == {"reportAny": 2} - survivors = list(tmp_path.glob(f"{gate.CACHE_FILE_PREFIX}*.json")) - assert len(survivors) == gate.CACHE_KEEP_ENTRIES - - -def _no_fetch(ref): - return None - - -def _never(reason): - def callback(ref): - raise AssertionError(reason) - - return callback - - -def test_base_counts_cached_returns_the_hit_without_recomputing(tmp_path): - path = gate.cache_path(tmp_path, "abc123", gate.environment_fingerprints()) - gate.store_counts(tmp_path, path, "abc123", {"reportAny": 7}) - - assert gate.base_counts_cached( - "abc123", - cache_dir=tmp_path, - compute=_never("a cache hit must not re-run the base pass"), - fetch=_never("a cache hit must not reach for CI"), - ) == {"reportAny": 7} - - -def test_base_counts_cached_computes_once_then_hits(tmp_path): - calls = [] - - def fake(ref): - calls.append(ref) - return {"reportAny": 4} - - first = gate.base_counts_cached( - "abc123", cache_dir=tmp_path, compute=fake, fetch=_no_fetch - ) - second = gate.base_counts_cached( - "abc123", cache_dir=tmp_path, compute=fake, fetch=_no_fetch - ) - assert first == second == {"reportAny": 4} - assert calls == ["abc123"] - - -def test_an_empty_base_pass_is_never_cached(tmp_path): - calls = [] - - def crashed(ref): - calls.append(ref) - return {} - - assert ( - gate.base_counts_cached( - "abc123", cache_dir=tmp_path, compute=crashed, fetch=_no_fetch - ) - == {} - ) - assert ( - gate.base_counts_cached( - "abc123", cache_dir=tmp_path, compute=crashed, fetch=_no_fetch - ) - == {} - ) - assert calls == ["abc123", "abc123"] - assert list(tmp_path.iterdir()) == [] - - -def test_base_counts_cached_uses_fetched_counts_and_persists_them(tmp_path): - counts = gate.base_counts_cached( - "abc123", - cache_dir=tmp_path, - compute=_never("fetched counts must skip the local base pass"), - fetch=lambda ref: {"reportAny": 9}, - ) - assert counts == {"reportAny": 9} - path = gate.cache_path(tmp_path, "abc123", gate.environment_fingerprints()) - assert gate.load_cached_counts(path) == {"reportAny": 9} - assert gate.base_counts_cached( - "abc123", - cache_dir=tmp_path, - compute=_never("the persisted fetch must satisfy later runs"), - fetch=_never("the persisted fetch must satisfy later runs"), - ) == {"reportAny": 9} - - -def test_base_counts_cached_falls_back_to_compute_on_a_fetch_miss(tmp_path): - calls = [] - - def local(ref): - calls.append(ref) - return {"reportAny": 4} - - assert gate.base_counts_cached( - "abc123", cache_dir=tmp_path, compute=local, fetch=_no_fetch - ) == {"reportAny": 4} - assert calls == ["abc123"] - - -def test_base_counts_cached_treats_empty_fetched_counts_as_a_miss(tmp_path): - assert gate.base_counts_cached( - "abc123", - cache_dir=tmp_path, - compute=lambda ref: {"reportAny": 2}, - fetch=lambda ref: {}, - ) == {"reportAny": 2} - path = gate.cache_path(tmp_path, "abc123", gate.environment_fingerprints()) - assert gate.load_cached_counts(path) == {"reportAny": 2} - - -def test_origin_slug_parsing_supports_ssh_and_https_github_forms(): - assert gate.parse_origin_slug("git@github.com:BerriAI/litellm.git") == "BerriAI/litellm" - assert gate.parse_origin_slug("git@github.com:BerriAI/litellm") == "BerriAI/litellm" - assert gate.parse_origin_slug("https://github.com/BerriAI/litellm.git") == "BerriAI/litellm" - assert gate.parse_origin_slug("https://github.com/BerriAI/litellm") == "BerriAI/litellm" - assert gate.parse_origin_slug("https://github.com/BerriAI/litellm/") == "BerriAI/litellm" - - -def test_origin_slug_parsing_rejects_non_github_urls(): - assert gate.parse_origin_slug("https://gitlab.com/BerriAI/litellm.git") is None - assert gate.parse_origin_slug("git@bitbucket.org:BerriAI/litellm.git") is None - assert gate.parse_origin_slug("not a url") is None - assert gate.parse_origin_slug("") is None - - -def _artifact_zip(payload): - import io - import zipfile - - buffer = io.BytesIO() - with zipfile.ZipFile(buffer, "w") as archive: - archive.writestr("basedpyright-counts.json", json.dumps(payload)) - return buffer.getvalue() - - -def _gh_stub(listing, zip_bytes): - def gh_output(args): - if args[-1].startswith("repos/"): - return json.dumps(listing).encode() - return zip_bytes - - return gh_output - - -def _live_listing(): - return { - "artifacts": [ - {"expired": False, "archive_download_url": "https://api.github.com/x/zip"} - ] - } - - -def test_fetcher_returns_counts_from_a_matching_artifact(capsys): - payload = {"base_point": "abc123", "counts": {"reportAny": 3}} - fetched = gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub(_live_listing(), _artifact_zip(payload)) - ) - assert fetched == {"reportAny": 3} - assert "fetched from CI artifact" in capsys.readouterr().err - - -def test_fetcher_rejects_an_artifact_for_a_different_base_point(): - payload = {"base_point": "someothersha", "counts": {"reportAny": 3}} - assert ( - gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub(_live_listing(), _artifact_zip(payload)) - ) - is None - ) - - -def test_fetcher_rejects_empty_or_misshapen_artifact_counts(): - for counts in ({}, {"reportAny": "three"}, {"reportAny": True}): - payload = {"base_point": "abc123", "counts": counts} - assert ( - gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub(_live_listing(), _artifact_zip(payload)) - ) - is None - ) - - -def test_fetcher_rejects_an_expired_artifact(): - listing = { - "artifacts": [ - {"expired": True, "archive_download_url": "https://api.github.com/x/zip"} - ] - } - payload = {"base_point": "abc123", "counts": {"reportAny": 3}} - assert ( - gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub(listing, _artifact_zip(payload)) - ) - is None - ) - - -def test_fetcher_misses_when_no_artifact_is_published(): - assert ( - gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub({"artifacts": []}, b"") - ) - is None - ) - - -def test_fetcher_misses_when_gh_is_unusable(capsys): - assert gate.fetch_ci_base_counts("abc123", gh_output=lambda args: None) is None - assert "computing base counts locally" in capsys.readouterr().err - - -def test_fetcher_misses_on_a_corrupt_artifact_archive(): - assert ( - gate.fetch_ci_base_counts( - "abc123", gh_output=_gh_stub(_live_listing(), b"not a zip") - ) - is None - ) - - -def test_emit_writes_the_artifact_json_named_by_the_head_key(tmp_path, capsys): - gate.cmd_emit_counts({"reportAny": 3, "aRule": 1}, tmp_path, "deadbeef") - key = gate.cache_key("deadbeef", gate.environment_fingerprints()) - path = tmp_path / f"basedpyright-counts-{key}.json" - assert json.loads(path.read_text()) == { - "base_point": "deadbeef", - "counts": {"aRule": 1, "reportAny": 3}, - } - summary = capsys.readouterr().out - assert "deadbeef" in summary - assert key in summary - assert "4" in summary - - -def test_emit_refuses_to_publish_empty_counts(tmp_path): - import pytest - - with pytest.raises(SystemExit): - gate.cmd_emit_counts({}, tmp_path, "deadbeef") - assert list(tmp_path.iterdir()) == [] - - -def test_emitted_file_round_trips_through_the_fetch_validation(tmp_path): - gate.cmd_emit_counts({"reportAny": 3}, tmp_path, "deadbeef") - key = gate.cache_key("deadbeef", gate.environment_fingerprints()) - payload = json.loads((tmp_path / f"basedpyright-counts-{key}.json").read_text()) - assert gate.counts_for_base(payload, "deadbeef") == {"reportAny": 3} - assert gate.counts_for_base(payload, "someothersha") is None - - -def _git(cwd, *args): - proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) - assert proc.returncode == 0, proc.stderr - return proc.stdout.strip() - - -def _commit(cwd, name): - (cwd / name).write_text(name) - _git(cwd, "add", "-A") - _git(cwd, "commit", "-q", "-m", name) - return _git(cwd, "rev-parse", "HEAD") - - -def _init_repo(tmp_path): - repo = tmp_path / "repo" - repo.mkdir() - _git(repo, "init", "-q", "-b", "main") - _git(repo, "config", "user.email", "gate@example.com") - _git(repo, "config", "user.name", "gate") - _git(repo, "config", "commit.gpgsign", "false") - return repo - - -def _branched_repo(tmp_path): - repo = _init_repo(tmp_path) - branch_point = _commit(repo, "shared.txt") - _git(repo, "checkout", "-q", "-b", "feature") - _commit(repo, "feature.txt") - _git(repo, "checkout", "-q", "main") - base_tip = _commit(repo, "drift.txt") - _git(repo, "checkout", "-q", "feature") - return repo, branch_point, base_tip - - -def test_base_point_is_the_branch_point_when_no_merge_is_in_progress(tmp_path): - repo, branch_point, _ = _branched_repo(tmp_path) - assert gate.resolve_base_point("main", cwd=repo) == branch_point - - -def test_base_point_mid_merge_advances_to_the_merged_in_base_tip(tmp_path): - repo, _, base_tip = _branched_repo(tmp_path) - _git(repo, "merge", "--no-commit", "--no-ff", "main") - assert gate.resolve_base_point("main", cwd=repo) == base_tip - - -def test_base_point_mid_merge_of_an_older_side_branch_keeps_the_newer_branch_point(tmp_path): - repo = _init_repo(tmp_path) - _commit(repo, "shared.txt") - _git(repo, "checkout", "-q", "-b", "old-side") - _commit(repo, "old.txt") - _git(repo, "checkout", "-q", "main") - newer_point = _commit(repo, "drift.txt") - _git(repo, "checkout", "-q", "-b", "feature") - _commit(repo, "feature.txt") - _git(repo, "merge", "--no-commit", "--no-ff", "old-side") - assert gate.resolve_base_point("main", cwd=repo) == newer_point +def test_no_head_output_is_refused_as_vacuous_before_any_base_lookup(capsys): + with pytest.raises(SystemExit) as exit_info: + gate.cmd_check({}, "irrelevant-base-ref") + assert exit_info.value.code == 1 + assert "vacuous" 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 1832668e7c3..a16840f64b0 100644 --- a/tests/test_litellm/test_type_discipline_gate.py +++ b/tests/test_litellm/test_type_discipline_gate.py @@ -1,106 +1,64 @@ """Tests for scripts/type_discipline_gate.py. -The gate's correctness lives in two pure functions: `over_ceiling` (which decides -whether the expensive base worktree scan is even needed) and `evaluate` (the -drift-safe breach check). Both are pinned here. +The gate compares each LIT rule's codebase count against the merge-base count plus +the headroom the gate script grants, so what is pinned here is the identity that +keys those base counts, the headroom the gate deliberately keeps, and the diff scan +that turns a breach into file:line. """ -import importlib.util -import subprocess -from pathlib import Path - -_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) -_spec.loader.exec_module(gate) +import lint_base_counts +import type_discipline_gate as gate -def _budget(limit): - return {"LIT006": {"limit": limit}} +def test_headroom_is_kept_only_for_the_seeded_final_and_rebind_rules(): + assert set(gate.HEADROOM) == {"LIT010", "LIT011"} + assert all(isinstance(value, int) and value > 0 for value in gate.HEADROOM.values()) -def test_over_ceiling_flags_only_counts_above_the_limit(): - budget = _budget(12) - assert gate.over_ceiling({"LIT006": 12}, budget) == frozenset() # at limit - assert gate.over_ceiling({"LIT006": 13}, budget) == frozenset({"LIT006"}) # over limit - assert gate.over_ceiling({}, budget) == frozenset() # missing rule counts as zero +def test_the_checker_identity_is_keyed_on_the_checker_source(): + identity = gate.checker_identity() + assert identity == lint_base_counts.Checker("type-discipline", (lint_base_counts.sha256_of(gate.CHECKER),)) -def test_over_ceiling_is_independent_across_rules(): - budget = {"LIT001": {"limit": 5}, "LIT006": {"limit": 10}} - assert gate.over_ceiling({"LIT001": 6, "LIT006": 10}, budget) == frozenset({"LIT001"}) +def test_parse_changed_lines_groups_hunks_under_their_own_file(): + diff = ( + "diff --git a/litellm/a.py b/litellm/a.py\n" + "--- a/litellm/a.py\n" + "+++ b/litellm/a.py\n" + "@@ -0,0 +3,2 @@\n" + "+one\n" + "+two\n" + "diff --git a/litellm/b.py b/litellm/b.py\n" + "--- a/litellm/b.py\n" + "+++ b/litellm/b.py\n" + "@@ -0,0 +10 @@\n" + "+only\n" + ) + changed = gate.parse_changed_lines(diff) + assert changed["litellm/a.py"] == {3, 4} + assert changed["litellm/b.py"] == {10} -def test_evaluate_blames_only_a_rule_over_limit_and_over_base(): - budget = _budget(10) - # over limit and grown vs base -> breach - assert [b.rule for b in gate.evaluate({"LIT006": 12}, {"LIT006": 9}, budget)] == ["LIT006"] - # over limit but flat vs base (pre-existing drift) -> not blamed - assert gate.evaluate({"LIT006": 12}, {"LIT006": 12}, budget) == [] - # within limit -> not blamed regardless of base - assert gate.evaluate({"LIT006": 10}, {"LIT006": 0}, budget) == [] +def test_parse_changed_lines_handles_several_hunks_in_one_file(): + diff = ( + "+++ b/litellm/a.py\n" + "@@ -0,0 +1,2 @@\n" + "+a\n" + "@@ -9,0 +20,1 @@\n" + "+b\n" + ) + assert gate.parse_changed_lines(diff)["litellm/a.py"] == {1, 2, 20} -def test_update_ratchets_limit_down_by_what_the_branch_fixed_never_up(): - budget = {"LIT001": {"limit": 100}, "LIT006": {"limit": 10}} - # LIT001 fixed 15 (60 -> 45) so its limit falls 100 -> 85; LIT006 grew, so its - # limit holds flat at 10. - current = {"LIT001": 45, "LIT006": 12} - base = {"LIT001": 60, "LIT006": 9} - assert gate.ratcheted_budget(budget, current, base) == { - "LIT001": {"limit": 85}, - "LIT006": {"limit": 10}, - } +def test_parse_changed_lines_on_an_empty_diff_is_empty(): + assert gate.parse_changed_lines("") == {} -def test_update_leaves_rules_seeded_on_this_branch_untouched(): - # A rule absent from the base budget was seeded with grandfathered headroom on - # this branch; the base tree predates the rule (e.g. no Final annotations yet), - # so ratcheting against it would collapse the deliberate headroom. - budget = {"LIT001": {"limit": 100}, "LIT010": {"limit": 24600}} - current = {"LIT001": 45, "LIT010": 16400} - base = {"LIT001": 60, "LIT010": 40000} - assert gate.ratcheted_budget(budget, current, base, frozenset({"LIT010"})) == { - "LIT001": {"limit": 85}, - "LIT010": {"limit": 24600}, - } - - -def _git(cwd, *args): - proc = subprocess.run(["git", *args], cwd=cwd, capture_output=True, text=True) - assert proc.returncode == 0, proc.stderr - return proc.stdout.strip() - - -def _commit(cwd, name): - (cwd / name).write_text(name) - _git(cwd, "add", "-A") - _git(cwd, "commit", "-q", "-m", name) - return _git(cwd, "rev-parse", "HEAD") - - -def _branched_repo(tmp_path): - repo = tmp_path / "repo" - repo.mkdir() - _git(repo, "init", "-q", "-b", "main") - _git(repo, "config", "user.email", "gate@example.com") - _git(repo, "config", "user.name", "gate") - _git(repo, "config", "commit.gpgsign", "false") - branch_point = _commit(repo, "shared.txt") - _git(repo, "checkout", "-q", "-b", "feature") - _commit(repo, "feature.txt") - _git(repo, "checkout", "-q", "main") - base_tip = _commit(repo, "drift.txt") - _git(repo, "checkout", "-q", "feature") - return repo, branch_point, base_tip - - -def test_base_point_is_the_branch_point_when_no_merge_is_in_progress(tmp_path): - repo, branch_point, _ = _branched_repo(tmp_path) - assert gate.resolve_base_point("main", cwd=repo) == branch_point - - -def test_base_point_mid_merge_advances_to_the_merged_in_base_tip(tmp_path): - repo, _, base_tip = _branched_repo(tmp_path) - _git(repo, "merge", "--no-commit", "--no-ff", "main") - assert gate.resolve_base_point("main", cwd=repo) == base_tip +def test_introduced_keeps_only_violations_on_changed_lines(): + violations = ( + gate.Violation("litellm/a.py", 3, "LIT006"), + gate.Violation("litellm/a.py", 99, "LIT006"), + gate.Violation("litellm/b.py", 3, "LIT001"), + ) + kept = gate.introduced(violations, {"litellm/a.py": {3}}) + assert kept == [gate.Violation("litellm/a.py", 3, "LIT006")] diff --git a/type-discipline-budget.json b/type-discipline-budget.json deleted file mode 100644 index e7186dfe186..00000000000 --- a/type-discipline-budget.json +++ /dev/null @@ -1,38 +0,0 @@ -{ - "LIT001": { - "limit": 22180 - }, - "LIT002": { - "limit": 26729 - }, - "LIT003": { - "limit": 261 - }, - "LIT004": { - "limit": 38 - }, - "LIT005": { - "limit": 0 - }, - "LIT006": { - "limit": 1035 - }, - "LIT007": { - "limit": 0 - }, - "LIT008": { - "limit": 945 - }, - "LIT009": { - "limit": 0 - }, - "LIT010": { - "limit": 16426 - }, - "LIT011": { - "limit": 5506 - }, - "LIT012": { - "limit": 4486 - } -}