diff --git a/.github/workflows/publish-lint-base-counts.yml b/.github/workflows/publish-lint-base-counts.yml index 5623ab82b90..bbe9cadc893 100644 --- a/.github/workflows/publish-lint-base-counts.yml +++ b/.github/workflows/publish-lint-base-counts.yml @@ -35,6 +35,8 @@ jobs: script: scripts/type_discipline_gate.py - checker: test-quality script: scripts/test_quality_gate.py + - checker: openapi-docs + script: scripts/openapi_docs_gate.py - checker: basedpyright script: scripts/type_check_gate.py @@ -60,16 +62,24 @@ jobs: uses: ./.github/actions/cache-cargo-build - name: Cache Prisma binaries - if: matrix.checker == 'basedpyright' + if: matrix.checker == 'basedpyright' || matrix.checker == 'openapi-docs' uses: ./.github/actions/cache-prisma-binaries - # The three source scanners only need the pinned dev tools (ruff and the + # The source scanners only need the pinned dev tools (ruff and the # stdlib checkers), the same versions test-linting.yml's python job runs. - name: Install the dev tools - if: matrix.checker != 'basedpyright' + if: matrix.checker != 'basedpyright' && matrix.checker != 'openapi-docs' run: | uv sync --frozen --only-group dev --no-install-project + # The OpenAPI gate imports the proxy app, so it needs the same install the + # test-linting.yml python job uses, Prisma client included. + - name: Install the proxy + if: matrix.checker == 'openapi-docs' + run: | + uv sync --frozen --group proxy-dev --group e2e-dev --extra proxy + uv run --no-sync prisma generate --schema litellm/proxy/schema.prisma + - name: Emit ${{ matrix.checker }} counts for HEAD if: matrix.checker != 'basedpyright' run: | diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index e85aaacd3f8..2d0766e0468 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -156,6 +156,14 @@ jobs: run: | uv run --no-sync python scripts/test_quality_gate.py --base "$GATE_BASE_SHA" + - name: Check OpenAPI docs (POST routes without a request schema, description or 200 schema, delta vs merge-base counts) + if: steps.changes.outputs.decision != 'skip' + env: + GH_TOKEN: ${{ github.token }} + run: | + uv sync --frozen --group proxy-dev --group e2e-dev --extra proxy + uv run --no-sync python scripts/openapi_docs_gate.py --base "$GATE_BASE_SHA" + - name: Print OpenAI version if: steps.changes.outputs.decision != 'skip' run: | diff --git a/AGENTS.md b/AGENTS.md index 0254893554d..7f8e9cedc26 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,7 +54,7 @@ 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 -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 its merge-base with the default branch, and a rule may not grow. 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. The one exception is `reportAny` / `reportExplicitAny`, which share a fixed codebase-wide cap in `ANY_CAPS` in `scripts/type_check_gate.py` because Any spreads past the lines you touch. A branch may add Anys while the total stays under that cap. Only lower `ANY_CAPS`, and only in its own PR to the default branch, never raise it on a feature branch +The five lint gates (`scripts/ruff_strict_gate.py`, `scripts/type_discipline_gate.py`, `scripts/test_quality_gate.py`, `scripts/openapi_docs_gate.py`, `scripts/type_check_gate.py`) compare each rule's codebase count on your branch against the count at its merge-base with the default branch, and a rule may not grow. 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. The one exception is `reportAny` / `reportExplicitAny`, which share a fixed codebase-wide cap in `ANY_CAPS` in `scripts/type_check_gate.py` because Any spreads past the lines you touch. A branch may add Anys while the total stays under that cap. Only lower `ANY_CAPS`, and only in its own PR to the default branch, never raise it on a feature branch `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 diff --git a/Makefile b/Makefile index 88cce280065..2a96a8a5e93 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ test-rust-extension rust-sqlx-prepare \ info lint lint-inner lint-dev lint-checks format \ lint-basedpyright lint-e2e-basedpyright lint-type-discipline \ - lint-ruff-strict lint-gate lint-test-quality \ + lint-ruff-strict lint-gate lint-test-quality lint-openapi-docs \ 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 @@ -35,6 +35,7 @@ help: @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-test-quality - Gate the test suite's TQ counts against their merge-base counts" + @echo " make lint-openapi-docs - Gate undocumented POST routes in the proxy OpenAPI document 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" @@ -219,6 +220,12 @@ lint-type-discipline: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) lint-test-quality: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) $(UV_RUN) python scripts/test_quality_gate.py --base "$(BASE_REF)" +# OpenAPI docs gate (POST routes without a request body schema, a description or a +# 200 schema in app.openapi()), counted the same delta-vs-base way so a new endpoint +# has to show up documented in Swagger UI. +lint-openapi-docs: $(LINT_DEP_INSTALL) $(LINT_DEP_BASE) + $(UV_RUN) python scripts/openapi_docs_gate.py --base "$(BASE_REF)" + lint-format: format-check lint-ruff-strict: install-dev @@ -250,7 +257,7 @@ lint-inner: lint-install @base_ref=$$($(RESOLVE_BASE)) && \ $(MAKE) BASE_REF="$$base_ref" -j $(LINT_JOBS) $(LINT_OUTPUT_SYNC) LINT_DEP_INSTALL= LINT_E2E_DEP_INSTALL= LINT_DEP_BASE= lint-checks -lint-checks: lint-format-check-changed lint-ruff lint-gate lint-type-discipline lint-test-quality lint-basedpyright lint-e2e-basedpyright check-circular-imports check-import-safety +lint-checks: lint-format-check-changed lint-ruff lint-gate lint-type-discipline lint-test-quality lint-openapi-docs lint-basedpyright lint-e2e-basedpyright check-circular-imports check-import-safety # Faster linting for local development (only checks changed code) lint-dev: lint-format-changed check-circular-imports check-import-safety diff --git a/scripts/check_openapi_docs.py b/scripts/check_openapi_docs.py new file mode 100755 index 00000000000..26bb034bfa6 --- /dev/null +++ b/scripts/check_openapi_docs.py @@ -0,0 +1,73 @@ +#!/usr/bin/env python3 +"""List the POST operations in the proxy's OpenAPI document that Swagger UI cannot explain. + +Every finding is one line, `` ``, so scripts/openapi_docs_gate.py can +count them per rule the same delta-vs-base way as the other lint gates. The rules: + +OA001 no request body schema (the handler reads ``request.body()`` and never told FastAPI + what is in it, see ``inline_request_body`` in custom_openapi_spec.py for the fix) +OA002 no description (the handler has no docstring and the route no ``description=``) +OA003 the 200 response has no schema (no ``response_model`` and no ``responses={200: ...}``) +""" + +from __future__ import annotations + +import json +import sys +from collections.abc import Iterator, Mapping +from typing import Final, NamedTuple + +GATED_METHODS: Final = frozenset({"post"}) + + +class Violation(NamedTuple): + method: str + path: str + code: str + + +def _as_mapping(node: object) -> Mapping[str, object]: + return node if isinstance(node, Mapping) else {} + + +def _json_schema(container: object) -> object: + content: Final = _as_mapping(_as_mapping(container).get("content")) + return _as_mapping(content.get("application/json")).get("schema") + + +def _operation_codes(operation: Mapping[str, object]) -> Iterator[str]: + if not _json_schema(operation.get("requestBody")): + yield "OA001" + description: Final = operation.get("description") + if not isinstance(description, str) or not description.strip(): + yield "OA002" + if not _json_schema(_as_mapping(operation.get("responses")).get("200")): + yield "OA003" + + +def violations(spec: Mapping[str, object]) -> tuple[Violation, ...]: + paths: Final = _as_mapping(spec.get("paths")) + return tuple( + Violation(method.upper(), path, code) + for path, operations in sorted(paths.items()) + for method, operation in sorted(_as_mapping(operations).items()) + if method in GATED_METHODS + for code in _operation_codes(_as_mapping(operation)) + ) # comprehension-ok: one pass over path, method and code is the whole scan + + +def proxy_spec() -> Mapping[str, object]: + from litellm.proxy.proxy_server import app + + return _as_mapping(app.openapi()) + + +def main(argv: list[str] = sys.argv[1:]) -> int: + spec: Final = json.loads(open(argv[0]).read()) if argv else proxy_spec() + for violation in violations(spec): + print(f"{violation.method} {violation.path} {violation.code}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/openapi_docs_gate.py b/scripts/openapi_docs_gate.py new file mode 100755 index 00000000000..a2a8fad78e9 --- /dev/null +++ b/scripts/openapi_docs_gate.py @@ -0,0 +1,178 @@ +#!/usr/bin/env python3 +"""Delta-vs-base gate for the OA* rules in scripts/check_openapi_docs.py. + +Sibling of scripts/test_quality_gate.py, pointed at the proxy's OpenAPI document +instead of a source tree. Each rule is counted over every POST operation the proxy +serves 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 new endpoint has to +ship with a request schema, a description and a 200 schema while the undocumented +routes that already exist are only ever allowed to shrink. + +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 with that tree first on PYTHONPATH. +""" + +from __future__ import annotations + +import argparse +import os +import re +import shutil +import signal +import subprocess +import sys +import tempfile +from collections import Counter +from collections.abc import Mapping, Sequence +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_openapi_docs.py" +TARGET: Final = "litellm" +TERMINATION_SIGNALS: Final = (signal.SIGTERM, signal.SIGHUP) + +_LINE: Final = re.compile(r"^(?P[A-Z]+) (?P\S+) (?POA\d+)$") +_ADDED_LINE: Final = re.compile(r"^\+(?!\+\+)(?P.*)$", re.MULTILINE) + + +class Violation(NamedTuple): + method: str + path: str + code: str + + +def _run(cmd: Sequence[str], cwd: Path = REPO_ROOT, env: Mapping[str, str] | None = None) -> str: + proc: Final = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, env=env) + if proc.returncode != 0: + sys.stderr.write(proc.stderr) + raise SystemExit(f"{cmd[0]} exited {proc.returncode}") + return proc.stdout + + +def checker_identity(checker: Path = CHECKER) -> Checker: + return Checker("openapi-docs", (sha256_of(checker),)) + + +def parse_violations(out: str) -> tuple[Violation, ...]: + return tuple( + Violation(match.group("method"), match.group("path"), match.group("code")) + for line in out.splitlines() + if (match := _LINE.match(line)) is not None + ) + + +def _check(root: Path, checker: Path) -> tuple[Violation, ...]: + resolved: Final = root.resolve() + env: Final = {**os.environ, "PYTHONPATH": os.pathsep.join((str(resolved), os.environ.get("PYTHONPATH", "")))} + return parse_violations(_run([sys.executable, str(checker)], cwd=resolved, env=env)) + + +def head_violations() -> tuple[Violation, ...]: + return _check(REPO_ROOT, CHECKER) + + +def count_by_rule(violations: Sequence[Violation]) -> Mapping[str, int]: + return MappingProxyType(dict(Counter(v.code for v in violations))) + + +def _exit_on_termination(signum: int, _frame: FrameType | None) -> None: + raise SystemExit(128 + signum) + + +def _install_termination_handlers() -> None: + for termination in TERMINATION_SIGNALS: + if signal.getsignal(termination) == signal.SIG_DFL: + signal.signal(termination, _exit_on_termination) + + +def base_counts(ref: str, repo_root: Path = REPO_ROOT, checker: Path = CHECKER) -> Mapping[str, int]: + """Rule counts at `ref`, measured with the *current* checker over the base tree.""" + _install_termination_handlers() + parent: Final = Path(tempfile.mkdtemp(prefix="oa_base_")) + worktree: Final = parent / "wt" + try: + _run(["git", "worktree", "add", "--detach", str(worktree), ref], cwd=repo_root) + (worktree / "scripts").mkdir(parents=True, exist_ok=True) + base_checker: Final = worktree / "scripts" / "check_openapi_docs.py" + shutil.copy(checker, base_checker) + return count_by_rule(_check(worktree, base_checker)) + finally: + # Teardown must never raise, or it masks the real error when the body failed. + subprocess.run( + ["git", "worktree", "remove", "--force", str(worktree)], + cwd=repo_root, capture_output=True, text=True, + ) + shutil.rmtree(parent, ignore_errors=True) + + +def added_text(diff_text: str) -> str: + return "\n".join(match.group("text") for match in _ADDED_LINE.finditer(diff_text)) + + +def introduced(violations: Sequence[Violation], added: str) -> tuple[Violation, ...]: + """The violations whose route path is quoted on a line this change added. A new + route is registered with its path as a string literal, so this names the endpoint + the change brought in without needing the base document's own violation list.""" + return tuple(v for v in violations if f'"{v.path}"' in added or f"'{v.path}'" in added) + + +def cmd_check(base: str) -> None: + 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)) + if not breaches: + print(f"OK: no OA 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, added_text(diff)) + print(f"FAIL: OA-rule totals grew past their merge-base count (base {base}):") + for breach in breaches: + print(f" {breach.rule}: total {breach.total} over ceiling {breach.ceiling} (this change added {breach.added})") + for violation in sorted(v for v in new if v.code == breach.rule): + print(f" {violation.method} {violation.path}") + print( + "Give every new POST route a request body schema (a typed body parameter, or " + "`openapi_extra={'requestBody': inline_request_body(Model, example)}` when the handler reads " + "`request.body()`), a docstring, and a 200 schema (`response_model=` or `responses={200: {'model': ...}}`), " + "or document an equal number of existing routes; the ceiling is the merge-base count. Run " + "`python scripts/check_openapi_docs.py` to see every finding." + ) + raise SystemExit(1) + + +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( + "--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_check(base_ref) + + +if __name__ == "__main__": + main() diff --git a/tests/unit/test_check_openapi_docs.py b/tests/unit/test_check_openapi_docs.py new file mode 100644 index 00000000000..d7122541416 --- /dev/null +++ b/tests/unit/test_check_openapi_docs.py @@ -0,0 +1,54 @@ +"""Tests for scripts/check_openapi_docs.py, the scan behind scripts/openapi_docs_gate.py.""" + +from typing import Final + +import check_openapi_docs as checker + +_BODY: Final = {"content": {"application/json": {"schema": {"type": "object"}}}} +_RESPONSES: Final = {"200": {"content": {"application/json": {"schema": {"$ref": "#/components/schemas/Out"}}}}} +_DOCUMENTED: Final = {"description": "Does a thing", "requestBody": _BODY, "responses": _RESPONSES} + + +def test_a_fully_documented_post_has_no_findings() -> None: + assert checker.violations({"paths": {"/v1/thing": {"post": _DOCUMENTED}}}) == () + + +def test_each_missing_piece_is_its_own_rule() -> None: + bare: Final = {"summary": "Thing", "responses": {"200": {"description": "Successful Response"}}} + codes: Final = {v.code for v in checker.violations({"paths": {"/v1/thing": {"post": bare}}})} + assert codes == {"OA001", "OA002", "OA003"} + + +def test_only_the_missing_piece_is_reported() -> None: + no_body: Final = {**_DOCUMENTED, "requestBody": {}} + blank_description: Final = {**_DOCUMENTED, "description": " "} + untyped_200: Final = {**_DOCUMENTED, "responses": {"200": {"content": {"application/json": {"schema": {}}}}}} + spec: Final = { + "paths": { + "/a": {"post": no_body}, + "/b": {"post": blank_description}, + "/c": {"post": untyped_200}, + } + } + assert checker.violations(spec) == ( + checker.Violation("POST", "/a", "OA001"), + checker.Violation("POST", "/b", "OA002"), + checker.Violation("POST", "/c", "OA003"), + ) + + +def test_non_post_operations_are_not_gated() -> None: + spec: Final = {"paths": {"/v1/thing": {"get": {}, "delete": {}, "put": {}, "post": _DOCUMENTED}}} + assert checker.violations(spec) == () + + +def test_findings_are_sorted_by_path_so_counts_are_stable() -> None: + spec: Final = {"paths": {"/z": {"post": {}}, "/a": {"post": {}}}} + assert [v.path for v in checker.violations(spec)] == ["/a", "/a", "/a", "/z", "/z", "/z"] + + +def test_main_reads_a_spec_file_and_prints_one_line_per_finding(tmp_path, capsys) -> None: + spec_file: Final = tmp_path / "openapi.json" + spec_file.write_text('{"paths": {"/v1/thing": {"post": {"description": "x", "requestBody": {}}}}}') + assert checker.main([str(spec_file)]) == 0 + assert capsys.readouterr().out.splitlines() == ["POST /v1/thing OA001", "POST /v1/thing OA003"] diff --git a/tests/unit/test_openapi_docs_gate.py b/tests/unit/test_openapi_docs_gate.py new file mode 100644 index 00000000000..fa8a5b1c7e3 --- /dev/null +++ b/tests/unit/test_openapi_docs_gate.py @@ -0,0 +1,60 @@ +"""Tests for scripts/openapi_docs_gate.py. + +The blame rule lives in scripts/lint_base_counts.py and is tested there; what is tested +here is the checker output parse and `introduced`, the diff scan that names the new +endpoint behind a breach. +""" + +from pathlib import Path +from typing import Final + +import openapi_docs_gate as gate + + +def test_editing_the_checker_rekeys_the_base_counts(tmp_path: Path) -> None: + checker: Final = tmp_path / "check.py" + checker.write_text("print('v1')\n") + before: Final = gate.checker_identity(checker).artifact_name("abc123") + checker.write_text("print('v2')\n") + assert gate.checker_identity(checker).artifact_name("abc123") != before + + +def test_parse_violations_skips_anything_that_is_not_a_finding_line() -> None: + out: Final = "Some warning from an import\nPOST /v1/decisions OA001\nPOST /v1/decisions OA003\nnot a line\n" + assert gate.parse_violations(out) == ( + gate.Violation("POST", "/v1/decisions", "OA001"), + gate.Violation("POST", "/v1/decisions", "OA003"), + ) + + +def test_count_by_rule_sums_findings_per_code() -> None: + violations: Final = ( + gate.Violation("POST", "/a", "OA001"), + gate.Violation("POST", "/b", "OA001"), + gate.Violation("POST", "/b", "OA003"), + ) + assert dict(gate.count_by_rule(violations)) == {"OA001": 2, "OA003": 1} + + +def test_introduced_keeps_only_routes_registered_on_added_lines() -> None: + diff: Final = ( + "diff --git a/litellm/proxy/x.py b/litellm/proxy/x.py\n" + "--- a/litellm/proxy/x.py\n" + "+++ b/litellm/proxy/x.py\n" + "@@ -0,0 +3,2 @@\n" + '+@router.post("/v1/new")\n' + "+async def new(request: Request): ...\n" + "@@ -10,1 +20,1 @@\n" + '-@router.post("/v1/removed")\n' + ) + violations: Final = ( + gate.Violation("POST", "/v1/new", "OA001"), + gate.Violation("POST", "/v1/old", "OA001"), + gate.Violation("POST", "/v1/removed", "OA001"), + ) + assert gate.introduced(violations, gate.added_text(diff)) == (gate.Violation("POST", "/v1/new", "OA001"),) + + +def test_introduced_does_not_match_a_path_that_is_only_a_prefix_of_an_added_one() -> None: + added: Final = gate.added_text('+@router.post("/v1/decisions/batch")\n') + assert gate.introduced((gate.Violation("POST", "/v1/decisions", "OA002"),), added) == ()