mirror of
https://github.com/tiennm99/litellm.git
synced 2026-08-02 16:21:54 +00:00
fix(mcp): resolve $ref params and path-level params in OpenAPI spec parsing (#22952)
* 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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 = []
|
||||
|
||||
@@ -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,
|
||||
|
||||
+193
-1
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user