diff --git a/.github/scripts/check_agent_server_rest_api_breakage.py b/.github/scripts/check_agent_server_rest_api_breakage.py index 39fc8ea255..0671ef098b 100644 --- a/.github/scripts/check_agent_server_rest_api_breakage.py +++ b/.github/scripts/check_agent_server_rest_api_breakage.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 -"""REST API breakage detection for openhands-agent-server. +"""REST API breakage detection for openhands-agent-server using oasdiff. This script compares the current OpenAPI schema for the agent-server REST API against -the previous published version on PyPI. +the previous published version on PyPI, using oasdiff for breaking change detection. Policies enforced (mirrors the SDK's Griffe checks, but for REST): @@ -14,13 +14,6 @@ - If a breaking REST change is detected, the current version must be at least a MINOR bump compared to the previous release. -The breakage detection currently focuses on compatibility rules that are robust to -OpenAPI generation changes: -- Removed operations -- New required parameters -- Request bodies that became required -- New required fields in JSON request bodies (best-effort) - If the previous release schema can't be fetched (e.g., network / PyPI issues), the script emits a warning and exits successfully to avoid flaky CI. """ @@ -33,8 +26,6 @@ import tempfile import tomllib import urllib.request -from collections.abc import Iterable -from dataclasses import dataclass from pathlib import Path from packaging import version as pkg_version @@ -45,24 +36,6 @@ PYPI_DISTRIBUTION = "openhands-agent-server" -_HTTP_METHODS = ( - "get", - "post", - "put", - "patch", - "delete", - "options", - "head", - "trace", -) - - -@dataclass(frozen=True, slots=True) -class OperationKey: - method: str - path: str - - def _read_version_from_pyproject(pyproject: Path) -> str: data = tomllib.loads(pyproject.read_text()) try: @@ -186,103 +159,43 @@ def _generate_openapi_for_version(version: str) -> dict | None: return None -def _iter_operations(schema: dict) -> Iterable[tuple[OperationKey, dict]]: - paths: dict = schema.get("paths", {}) - for path, path_item in paths.items(): - if not isinstance(path_item, dict): - continue - for method in _HTTP_METHODS: - operation = path_item.get(method) - if isinstance(operation, dict): - yield OperationKey(method=method, path=path), operation - - -def _required_parameters(operation: dict) -> set[tuple[str, str]]: - required: set[tuple[str, str]] = set() - for param in operation.get("parameters", []) or []: - if not isinstance(param, dict): - continue - if not param.get("required"): - continue - name = param.get("name") - location = param.get("in") - if isinstance(name, str) and isinstance(location, str): - required.add((name, location)) - return required - - -def _resolve_ref(schema: dict, spec: dict, *, max_depth: int = 50) -> dict: - current = schema - seen: set[str] = set() - depth = 0 - - while isinstance(current, dict) and "$ref" in current: - ref = current["$ref"] - if not isinstance(ref, str) or not ref.startswith("#/"): - return current - if ref in seen or depth >= max_depth: - return current - - seen.add(ref) - depth += 1 - - target: object = spec - for part in ref.removeprefix("#/").split("/"): - if not isinstance(target, dict) or part not in target: - return current - target = target[part] - if not isinstance(target, dict): - return current - current = target - - return current - - -def _required_json_fields(operation: dict, spec: dict) -> set[str]: - request_body = operation.get("requestBody") or {} - if not isinstance(request_body, dict): - return set() +def _run_oasdiff_breakage_check( + prev_spec: Path, cur_spec: Path +) -> tuple[list[dict], int]: + """Run oasdiff breaking check between two OpenAPI specs. - content = request_body.get("content") or {} - if not isinstance(content, dict): - return set() - - json_content = content.get("application/json") - if not isinstance(json_content, dict): - return set() - - schema = json_content.get("schema") - if not isinstance(schema, dict): - return set() - - return _required_json_fields_from_schema(schema, spec) - - -def _required_json_fields_from_schema(schema: dict, spec: dict) -> set[str]: - resolved = _resolve_ref(schema, spec) - - if "allOf" in resolved and isinstance(resolved["allOf"], list): - required: set[str] = set() - for item in resolved["allOf"]: - if isinstance(item, dict): - required |= _required_json_fields_from_schema(item, spec) - return required - - if resolved.get("type") != "object": - return set() - - required = resolved.get("required") - if not isinstance(required, list): - return set() - - return {field for field in required if isinstance(field, str)} + Returns (list of breaking changes, exit code from oasdiff). + """ + try: + result = subprocess.run( + [ + "oasdiff", + "breaking", + "-f", + "json", + "--fail-on", + "ERR", + str(prev_spec), + str(cur_spec), + ], + capture_output=True, + text=True, + ) + except FileNotFoundError: + print( + "::warning title=oasdiff not found::" + "Please install oasdiff: https://github.com/oasdiff/oasdiff" + ) + return [], 0 + breaking_changes = [] + if result.stdout: + try: + breaking_changes = json.loads(result.stdout) + except json.JSONDecodeError: + pass -def _is_request_body_required(operation: dict) -> bool: - request_body = operation.get("requestBody") - if not isinstance(request_body, dict): - return False - return bool(request_body.get("required")) + return breaking_changes, result.returncode def _is_minor_or_major_bump(current: str, previous: str) -> bool: @@ -293,58 +206,6 @@ def _is_minor_or_major_bump(current: str, previous: str) -> bool: return (cur.major, cur.minor) != (prev.major, prev.minor) -def _compute_breakages( - prev_schema: dict, current_schema: dict -) -> tuple[list[str], list[OperationKey]]: - prev_ops = dict(_iter_operations(prev_schema)) - cur_ops = dict(_iter_operations(current_schema)) - - removed = set(prev_ops).difference(cur_ops) - - undeprecated_removals: list[OperationKey] = [] - for key in sorted(removed, key=lambda k: (k.path, k.method)): - if not prev_ops[key].get("deprecated"): - undeprecated_removals.append(key) - - breaking_reasons: list[str] = [] - - if removed: - breaking_reasons.append(f"Removed operations: {len(removed)}") - - for key, prev_op in prev_ops.items(): - cur_op = cur_ops.get(key) - if cur_op is None: - continue - - new_required_params = _required_parameters(cur_op) - _required_parameters( - prev_op - ) - if new_required_params: - formatted = ", ".join( - sorted(f"{n}({loc})" for n, loc in new_required_params) - ) - breaking_reasons.append( - f"{key.method.upper()} {key.path}: new required params: {formatted}" - ) - - if _is_request_body_required(cur_op) and not _is_request_body_required(prev_op): - breaking_reasons.append( - f"{key.method.upper()} {key.path}: request body became required" - ) - - prev_required_fields = _required_json_fields(prev_op, prev_schema) - cur_required_fields = _required_json_fields(cur_op, current_schema) - new_required_fields = cur_required_fields - prev_required_fields - if new_required_fields: - formatted = ", ".join(sorted(new_required_fields)) - breaking_reasons.append( - f"{key.method.upper()} {key.path}: " - f"new required JSON fields: {formatted}" - ) - - return breaking_reasons, undeprecated_removals - - def main() -> int: current_version = _read_version_from_pyproject(AGENT_SERVER_PYPROJECT) prev_version = _get_previous_version(PYPI_DISTRIBUTION, current_version) @@ -362,34 +223,81 @@ def main() -> int: current_schema = _generate_current_openapi() - breaking_reasons, undeprecated_removals = _compute_breakages( - prev_schema, current_schema - ) + with tempfile.TemporaryDirectory(prefix="oasdiff-specs-") as tmp: + tmp_path = Path(tmp) + prev_spec_file = tmp_path / "prev_spec.json" + cur_spec_file = tmp_path / "cur_spec.json" + + prev_spec_file.write_text(json.dumps(prev_schema, indent=2)) + cur_spec_file.write_text(json.dumps(current_schema, indent=2)) + + breaking_changes, exit_code = _run_oasdiff_breakage_check( + prev_spec_file, cur_spec_file + ) + + if not breaking_changes: + if exit_code == 0: + print("No breaking changes detected.") + else: + print( + f"oasdiff returned exit code {exit_code} but no breaking changes " + "in JSON format. There may be warnings only." + ) + return 0 + + removed_operations = [] + other_breakage = [] + + for change in breaking_changes: + change_id = change.get("id", "") + text = change.get("text", "") + details = change.get("details", {}) + + if "removed" in change_id.lower() and "operation" in change_id.lower(): + path = details.get("path", "") + method = details.get("method", "") + operation_id = details.get("operationId", "") + deprecated = details.get("deprecated", False) + + removed_operations.append( + { + "path": path, + "method": method, + "operationId": operation_id, + "deprecated": deprecated, + } + ) + else: + other_breakage.append(text) + + undeprecated_removals = [ + op for op in removed_operations if not op.get("deprecated", False) + ] if undeprecated_removals: - for key in undeprecated_removals: + for op in undeprecated_removals: print( - "::error " - f"title={PYPI_DISTRIBUTION} REST API::Removed {key.method.upper()} " - f"{key.path} without prior deprecation (deprecated=true)." + f"::error " + f"title={PYPI_DISTRIBUTION} REST API::Removed {op['method'].upper()} " + f"{op['path']} without prior deprecation (deprecated=true)." ) - breaking = bool(breaking_reasons) + has_breaking = bool(breaking_changes) - if breaking and not _is_minor_or_major_bump(current_version, prev_version): + if has_breaking and not _is_minor_or_major_bump(current_version, prev_version): print( "::error " f"title={PYPI_DISTRIBUTION} REST API::Breaking REST API change detected " f"without MINOR version bump ({prev_version} -> {current_version})." ) - if breaking: - print("Breaking REST API changes detected compared to previous release:") - for reason in breaking_reasons: - print(f"- {reason}") + if has_breaking: + print("\nBreaking REST API changes detected compared to previous release:") + for text in breaking_changes: + print(f"- {text.get('text', str(text))}") errors = bool(undeprecated_removals) or ( - breaking and not _is_minor_or_major_bump(current_version, prev_version) + has_breaking and not _is_minor_or_major_bump(current_version, prev_version) ) return 1 if errors else 0 diff --git a/.github/workflows/agent-server-rest-api-breakage.yml b/.github/workflows/agent-server-rest-api-breakage.yml index 040a2ec594..f3cf41444e 100644 --- a/.github/workflows/agent-server-rest-api-breakage.yml +++ b/.github/workflows/agent-server-rest-api-breakage.yml @@ -26,6 +26,11 @@ jobs: - name: Install workspace deps (dev) run: uv sync --frozen --group dev + - name: Install oasdiff + run: | + curl -L https://raw.githubusercontent.com/oasdiff/oasdiff/main/install.sh | sh -s -- -b /usr/local/bin + oasdiff --version + - name: Run agent server REST API breakage check id: api_breakage # Release PRs (rel-*) must block on breakage. diff --git a/openhands-agent-server/AGENTS.md b/openhands-agent-server/AGENTS.md index 74dac9f129..0473a1f579 100644 --- a/openhands-agent-server/AGENTS.md +++ b/openhands-agent-server/AGENTS.md @@ -43,7 +43,7 @@ Removing an endpoint is a breaking change. ### CI enforcement The workflow `Agent server REST API breakage checks` compares the current OpenAPI -schema against the previous `openhands-agent-server` release on PyPI. +schema against the previous `openhands-agent-server` release on PyPI using [oasdiff](https://github.com/oasdiff/oasdiff). It currently enforces: - No removal of operations (path + method) unless they were already marked