Repository navigation
ci: gate undocumented POST routes in the proxy OpenAPI document #45843
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,6 @@ | |
| ReadOnly, | ||
| Required, | ||
| TypedDict, | ||
| ) | ||
|
|
||
| from litellm._uuid import uuid | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, ``<METHOD> <path> <code>``, 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" | ||
|
Comment on lines
+44
to
+45
Contributor
There was a problem hiding this comment.
Accept the route's documented success response instead of requiring a response it never returns. |
||
|
|
||
|
|
||
| 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()) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<method>[A-Z]+) (?P<path>\S+) (?P<code>OA\d+)$") | ||
| _ADDED_LINE: Final = re.compile(r"^\+(?!\+\+)(?P<text>.*)$", 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() |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_json_schemaonly acceptsapplication/json, soOA001rejects a fully documented multipart or form request. The proxy already uses this pattern increate_file, withUploadFile = File(...)andForm(...)parameters. Adding another documented upload route increases the count and fails lint even though Swagger shows its request schema.Check schemas under the supported content types rather than requiring JSON.