From bb52b0b6b0a658f66d9aada1ce1780947b719c19 Mon Sep 17 00:00:00 2001 From: Ishaan Jaff Date: Fri, 6 Mar 2026 18:02:48 -0800 Subject: [PATCH] fix(mcp): resolve $ref params and path-level params in OpenAPI spec parsing (#22952) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): resolve \$ref params and merge path-level params in OpenAPI tool registration Real-world OpenAPI specs (e.g. GitHub's 11.8 MB official spec) use two patterns that crashed tool registration: 1. \$ref parameters: params defined as {"$ref": "#/components/parameters/foo"} instead of inline objects. Accessing param["name"] on a $ref raises KeyError. Fix: resolve each param against components/parameters before processing. 2. Path-level parameters: params defined on the path object apply to all HTTP methods on that path, but the operation object doesn't include them. GitHub's spec uses this for owner/repo/etc. path params. Fix: merge path-level params with operation-level params (op-level wins when the same name+in combination appears in both). With this fix the full GitHub REST API spec loads successfully: 720 paths → 1079 tools, all with correct parameter schemas. * fix(mcp): resolve \$ref params in OpenAPI preview endpoint (test/tools/list) The _preview_openapi_tools function (called by the UI add-server form to show connection status and available tools) had the same bug as _register_openapi_tools: it accessed param["name"] directly without resolving \$ref parameters or merging path-level parameters from the path item. This caused "Failed to load OpenAPI spec: 'name'" for any spec that uses component-level parameter references (e.g. GitHub's official REST API spec). Apply the same fix: resolve \$ref against components/parameters and merge path-level params (with operation-level taking priority) before building schemas. * refactor(openapi-mcp): extract resolve_operation_params, add tests - Hoist _resolve_ref and _resolve_param_list to module level in openapi_to_mcp_generator.py (were being redefined on every loop iteration) - _resolve_ref now returns None for unresolvable $refs instead of the stub dict, preventing (None, None) from poisoning deduplication - Add resolve_operation_params() as a shared helper that handles both $ref resolution and path-level param merging - Replace duplicated inline logic in mcp_server_manager.py and rest_endpoints.py with calls to resolve_operation_params() - Add TestResolveRef, TestResolveParamList, TestResolveOperationParams test classes covering $ref resolution, path-level merging, collision semantics, unresolvable ref filtering, and a GitHub-style spec fixture --- .../mcp_server/mcp_server_manager.py | 11 +- .../mcp_server/openapi_to_mcp_generator.py | 58 +++++- .../mcp_server/rest_endpoints.py | 7 +- .../test_openapi_to_mcp_generator.py | 194 +++++++++++++++++- 4 files changed, 265 insertions(+), 5 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 7c17da36bb..0b58009fcf 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -385,6 +385,7 @@ class MCPServerManager: ) from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( load_openapi_spec_async, + resolve_operation_params, ) from litellm.proxy._experimental.mcp_server.tool_registry import ( global_mcp_tool_registry, @@ -438,6 +439,7 @@ class MCPServerManager: # Extract and register tools from OpenAPI paths paths = spec.get("paths", {}) + components = spec.get("components", {}) registered_count = 0 verbose_logger.debug(f"Processing {len(paths)} paths from OpenAPI spec") @@ -449,6 +451,11 @@ class MCPServerManager: operation = path_item[method] + # Resolve $ref params and merge path-level params into the operation. + resolved_operation = resolve_operation_params( + operation, path_item, components + ) + # Generate tool name (without prefix initially) operation_id = operation.get( "operationId", f"{method}_{path.replace('/', '_')}" @@ -467,11 +474,11 @@ class MCPServerManager: ) # Build input schema using imported function - input_schema = build_input_schema(operation) + input_schema = build_input_schema(resolved_operation) # Create tool function with headers using imported function tool_func = create_tool_function( - path, method, operation, base_url, headers=headers + path, method, resolved_operation, base_url, headers=headers ) tool_func.__name__ = prefixed_tool_name tool_func.__doc__ = description diff --git a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py index 6ad391a500..5f6cb87b26 100644 --- a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py +++ b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py @@ -7,7 +7,7 @@ import contextvars import json import os from pathlib import PurePosixPath -from typing import Any, Dict, Optional +from typing import Any, Dict, List, Optional from urllib.parse import quote from litellm._logging import verbose_logger @@ -115,6 +115,62 @@ def get_base_url(spec: Dict[str, Any], spec_path: Optional[str] = None) -> str: return "" +def _resolve_ref( + param: Dict[str, Any], component_params: Dict[str, Any] +) -> Optional[Dict[str, Any]]: + """Resolve a single parameter, following a $ref if present. + + Returns the resolved param dict, or None if the $ref target is absent from + components (so callers can skip/filter it rather than propagating a stub + with name=None that would corrupt deduplication). + """ + ref = param.get("$ref", "") + if not ref.startswith("#/components/parameters/"): + return param + return component_params.get(ref.split("/")[-1]) + + +def _resolve_param_list( + raw: List[Dict[str, Any]], component_params: Dict[str, Any] +) -> List[Dict[str, Any]]: + """Resolve $refs in a parameter list, dropping any unresolvable entries.""" + result = [] + for p in raw: + resolved = _resolve_ref(p, component_params) + if resolved is not None and resolved.get("name"): + result.append(resolved) + return result + + +def resolve_operation_params( + operation: Dict[str, Any], + path_item: Dict[str, Any], + components: Dict[str, Any], +) -> Dict[str, Any]: + """Return a copy of *operation* with fully-resolved, merged parameters. + + Handles two common patterns in real-world OpenAPI specs: + + 1. **$ref parameters** — ``{"$ref": "#/components/parameters/per-page"}`` + instead of inline objects. Each ref is resolved against + ``components["parameters"]``; unresolvable refs are silently dropped so + they cannot corrupt the deduplication set with ``(None, None)`` keys. + + 2. **Path-level parameters** — params defined on the path item that apply + to every HTTP method on that path (e.g. ``owner``, ``repo``). They are + merged with the operation-level params; operation-level wins when the + same ``name`` + ``in`` combination appears in both. + """ + component_params = components.get("parameters", {}) + path_level = _resolve_param_list(path_item.get("parameters", []), component_params) + op_level = _resolve_param_list(operation.get("parameters", []), component_params) + op_keys = {(p["name"], p.get("in")) for p in op_level} + merged = [p for p in path_level if (p["name"], p.get("in")) not in op_keys] + op_level + result = dict(operation) + result["parameters"] = merged + return result + + def extract_parameters(operation: Dict[str, Any]) -> tuple: """Extract parameter names from OpenAPI operation.""" path_params = [] diff --git a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py index 16f8f83543..6082e9bd60 100644 --- a/litellm/proxy/_experimental/mcp_server/rest_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/rest_endpoints.py @@ -666,21 +666,26 @@ if MCP_AVAILABLE: from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( build_input_schema, load_openapi_spec_async, + resolve_operation_params, ) try: spec = await load_openapi_spec_async(spec_path) paths = spec.get("paths", {}) + components = spec.get("components", {}) tools: List[dict] = [] for path, path_item in paths.items(): for method in ("get", "post", "put", "patch", "delete"): operation = path_item.get(method) if operation is None: continue + + resolved_op = resolve_operation_params(operation, path_item, components) + op_id = operation.get("operationId", f"{method}_{path}") summary = operation.get("summary", "") description = operation.get("description", summary) - input_schema = build_input_schema(operation) + input_schema = build_input_schema(resolved_op) tools.append( { "name": op_id, diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py index bc93d54830..655e14b965 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_openapi_to_mcp_generator.py @@ -15,10 +15,13 @@ from unittest.mock import AsyncMock, patch import pytest from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( + _resolve_param_list, + _resolve_ref, build_input_schema, create_tool_function, extract_parameters, get_base_url, + resolve_operation_params, ) GET_ASYNC_CLIENT_TARGET = ( @@ -660,6 +663,195 @@ class TestGetBaseUrl: "paths": {} } spec_path = "https://api.example.com/v2/docs/api/openapi.json" - + base_url = get_base_url(spec, spec_path) assert base_url == "https://api.example.com/v2/docs/api" + + +class TestResolveRef: + """Test $ref resolution for individual parameters.""" + + def test_inline_param_returned_unchanged(self): + """Inline params (no $ref) are returned as-is.""" + param = {"name": "owner", "in": "path", "required": True} + component_params: dict = {} + result = _resolve_ref(param, component_params) + assert result is param + + def test_ref_resolved_from_components(self): + """A $ref pointing at components/parameters is resolved correctly.""" + param = {"$ref": "#/components/parameters/per-page"} + component_params = { + "per-page": {"name": "per_page", "in": "query", "schema": {"type": "integer"}} + } + result = _resolve_ref(param, component_params) + assert result == {"name": "per_page", "in": "query", "schema": {"type": "integer"}} + + def test_unresolvable_ref_returns_none(self): + """A $ref whose target is absent from components returns None (not the stub).""" + param = {"$ref": "#/components/parameters/missing-param"} + component_params: dict = {} + result = _resolve_ref(param, component_params) + assert result is None + + def test_non_component_ref_returned_unchanged(self): + """A $ref that doesn't start with #/components/parameters/ is returned as-is.""" + param = {"$ref": "#/definitions/SomeModel"} + result = _resolve_ref(param, {}) + assert result is param + + +class TestResolveParamList: + """Test batch $ref resolution with filtering.""" + + def test_all_inline_params_preserved(self): + """All inline params with names are preserved.""" + raw = [ + {"name": "owner", "in": "path"}, + {"name": "repo", "in": "path"}, + ] + result = _resolve_param_list(raw, {}) + assert len(result) == 2 + assert result[0]["name"] == "owner" + assert result[1]["name"] == "repo" + + def test_refs_resolved(self): + """$ref entries are resolved against component_params.""" + raw = [ + {"$ref": "#/components/parameters/per-page"}, + {"name": "q", "in": "query"}, + ] + component_params = { + "per-page": {"name": "per_page", "in": "query", "schema": {"type": "integer"}} + } + result = _resolve_param_list(raw, component_params) + assert len(result) == 2 + assert result[0]["name"] == "per_page" + assert result[1]["name"] == "q" + + def test_unresolvable_refs_dropped(self): + """Unresolvable $refs are silently dropped — no None or nameless entries.""" + raw = [ + {"$ref": "#/components/parameters/does-not-exist"}, + {"name": "q", "in": "query"}, + ] + result = _resolve_param_list(raw, {}) + assert len(result) == 1 + assert result[0]["name"] == "q" + + def test_nameless_entries_dropped(self): + """Entries that resolve to dicts without a 'name' key are dropped.""" + raw = [{"schema": {"type": "string"}}] # no "name" field + result = _resolve_param_list(raw, {}) + assert result == [] + + +class TestResolveOperationParams: + """Test the shared resolve_operation_params helper.""" + + def test_inline_params_unchanged(self): + """Simple inline operation with no $refs or path-level params is returned unchanged.""" + operation = { + "operationId": "listRepos", + "parameters": [ + {"name": "owner", "in": "path", "required": True}, + {"name": "sort", "in": "query"}, + ], + } + path_item = {"get": operation} + result = resolve_operation_params(operation, path_item, {}) + names = [p["name"] for p in result["parameters"]] + assert names == ["owner", "sort"] + + def test_ref_params_resolved(self): + """$ref parameters in the operation are resolved from components.""" + operation = { + "parameters": [ + {"$ref": "#/components/parameters/per-page"}, + {"name": "q", "in": "query"}, + ] + } + path_item = {"get": operation} + components = { + "parameters": { + "per-page": {"name": "per_page", "in": "query", "schema": {"type": "integer"}} + } + } + result = resolve_operation_params(operation, path_item, components) + names = [p["name"] for p in result["parameters"]] + assert "per_page" in names + assert "q" in names + + def test_path_level_params_merged(self): + """Path-level parameters are merged into the operation parameters.""" + path_level_params = [ + {"name": "owner", "in": "path", "required": True}, + {"name": "repo", "in": "path", "required": True}, + ] + operation = { + "parameters": [{"name": "sort", "in": "query"}] + } + path_item = {"parameters": path_level_params, "get": operation} + result = resolve_operation_params(operation, path_item, {}) + names = [p["name"] for p in result["parameters"]] + assert "owner" in names + assert "repo" in names + assert "sort" in names + + def test_operation_level_wins_on_collision(self): + """When path-level and operation-level define the same name+in, operation wins.""" + path_level_params = [ + {"name": "per_page", "in": "query", "schema": {"type": "integer"}, "default": 30} + ] + operation = { + "parameters": [ + {"name": "per_page", "in": "query", "schema": {"type": "integer"}, "default": 100} + ] + } + path_item = {"parameters": path_level_params, "get": operation} + result = resolve_operation_params(operation, path_item, {}) + per_page_params = [p for p in result["parameters"] if p["name"] == "per_page"] + assert len(per_page_params) == 1 + assert per_page_params[0].get("default") == 100 # operation-level value + + def test_unresolvable_refs_silently_dropped(self): + """Unresolvable $refs are dropped — they don't poison the result with (None, None) keys.""" + operation = { + "parameters": [ + {"$ref": "#/components/parameters/nonexistent"}, + {"name": "q", "in": "query"}, + ] + } + path_item = {"get": operation} + result = resolve_operation_params(operation, path_item, {}) + names = [p["name"] for p in result["parameters"]] + assert names == ["q"] + # Verify no None entries slipped through + assert all(p.get("name") is not None for p in result["parameters"]) + + def test_github_style_spec_structure(self): + """Simulate a GitHub-style spec: path-level owner+repo refs, operation-level query params.""" + component_params = { + "owner": {"name": "owner", "in": "path", "required": True, "schema": {"type": "string"}}, + "repo": {"name": "repo", "in": "path", "required": True, "schema": {"type": "string"}}, + "per-page": {"name": "per_page", "in": "query", "schema": {"type": "integer"}}, + } + path_level_params = [ + {"$ref": "#/components/parameters/owner"}, + {"$ref": "#/components/parameters/repo"}, + ] + operation = { + "operationId": "repos/list-commits", + "parameters": [ + {"$ref": "#/components/parameters/per-page"}, + {"name": "sha", "in": "query"}, + ], + } + path_item = {"parameters": path_level_params, "get": operation} + result = resolve_operation_params(operation, path_item, {"parameters": component_params}) + names = [p["name"] for p in result["parameters"]] + assert "owner" in names + assert "repo" in names + assert "per_page" in names + assert "sha" in names + assert len(names) == 4 # no duplicates