fix(mcp): OpenAPI tool listing and execution for relative URLs and camelCase - #23238
Conversation
…melCase - Fix case-insensitive tool name matching in _tool_name_matches() so that OpenAPI operationIds (camelCase) match lowercase registered tool names when filtering by allowed_tools - Fix get_base_url() to resolve relative server URLs (e.g. /api/v3) by deriving full base URL from spec_path when OpenAPI spec has relative URLs - Add tests for case-insensitive matching and filter_tools_by_allowed_tools Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes two bugs in the OpenAPI-based MCP server integration: (1) tool listing silently returning zero results when
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py | Adds relative URL resolution in get_base_url() by parsing the domain from spec_path when the OpenAPI servers[0].url is a relative path. Logic is correct; minor style issue with urlparse being imported inside the function body rather than at the module top. |
| litellm/proxy/_experimental/mcp_server/server.py | Makes _tool_name_matches() case-insensitive by lowercasing both the tool name and filter list before comparison. The change correctly fixes OpenAPI camelCase operationId mismatches and also implicitly applies to disallowed_tools. Minor performance overhead from rebuilding the lowercase list on every call. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py | Adds three pure unit tests covering case-insensitive tool name matching. No real network calls are made. Missing a corresponding unit test for the get_base_url relative URL fix. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[get_base_url called] --> B{spec has servers?}
B -- Yes --> C[read servers 0 url]
C --> D{url starts with /\nAND spec_path given?}
D -- Yes --> E{spec_path is http/https?}
E -- Yes --> F[urlparse spec_path\nextract scheme + netloc]
F --> G[return domain + server_url\ne.g. https://petstore3.swagger.io/api/v3]
E -- No --> H[return relative server_url as-is\nInvalidURL still possible for local files]
D -- No --> I[return server_url as-is\nabsolute URL]
B -- No --> J{spec has host?}
J -- Yes --> K[build URL from scheme+host+basePath\nOpenAPI 2.x]
J -- No --> L{spec_path is http/https?}
L -- Yes --> M[strip known suffixes\nreturn derived base URL]
L -- No --> N[return empty string]
subgraph _tool_name_matches fix
P[tool_name + filter_list] --> Q[lowercase both]
Q --> R{tool_name.lower in filter_list_lower?}
R -- Yes --> S[return True]
R -- No --> T[strip server prefix\ncheck unprefixed.lower]
T --> U{match?}
U -- Yes --> S
U -- No --> V[return False]
end
Last reviewed commit: db99fde
| if spec_path.startswith("http://") or spec_path.startswith("https://"): | ||
| # Extract base URL from spec_path (e.g., https://petstore3.swagger.io/api/v3/openapi.json) | ||
| # Combine domain with the relative server URL | ||
| from urllib.parse import urlparse |
There was a problem hiding this comment.
Import inside function body
urlparse is already available in the same urllib.parse namespace used at the top of this file (line 11 imports quote from there). Moving this import to the module level avoids the repeated import resolution on every code path that hits this branch and keeps the import conventions consistent with the rest of the file.
| from urllib.parse import urlparse | |
| from urllib.parse import urlparse, quote |
Or better, just add urlparse to the existing top-level import:
# line 11 — change to:
from urllib.parse import quote, urlparseand remove the inline import entirely.
| # Normalize filter list to lowercase for case-insensitive comparison | ||
| filter_list_lower = [f.lower() for f in filter_list] | ||
|
|
||
| if tool_name.lower() in filter_list_lower: | ||
| return True | ||
|
|
||
| # Check if the unprefixed name is in the list | ||
| # Check if the unprefixed name is in the list (case-insensitive) | ||
| unprefixed_name, _ = split_server_prefix_from_name(tool_name) | ||
| return unprefixed_name in filter_list | ||
| return unprefixed_name.lower() in filter_list_lower |
There was a problem hiding this comment.
filter_list_lower rebuilt on every call
filter_list_lower is reconstructed as a list on every invocation of _tool_name_matches. Since filter_tools_by_allowed_tools calls this function in a loop over all tools, the filter list is lowercased once per tool rather than once per filtering pass. Using a set also changes the in membership test from O(M) to O(1):
| # Normalize filter list to lowercase for case-insensitive comparison | |
| filter_list_lower = [f.lower() for f in filter_list] | |
| if tool_name.lower() in filter_list_lower: | |
| return True | |
| # Check if the unprefixed name is in the list | |
| # Check if the unprefixed name is in the list (case-insensitive) | |
| unprefixed_name, _ = split_server_prefix_from_name(tool_name) | |
| return unprefixed_name in filter_list | |
| return unprefixed_name.lower() in filter_list_lower | |
| filter_list_lower = {f.lower() for f in filter_list} | |
| if tool_name.lower() in filter_list_lower: | |
| return True | |
| # Check if the unprefixed name is in the list (case-insensitive) | |
| unprefixed_name, _ = split_server_prefix_from_name(tool_name) | |
| return unprefixed_name.lower() in filter_list_lower |
A more impactful refactoring would be to compute filter_list_lower once in filter_tools_by_allowed_tools and pass it through, but the set change above at least removes the repeated list construction.
| assert spend_meta["per_server_tool_counts"]["server_a"] == 1 | ||
|
|
||
|
|
||
| def test_tool_name_matches_case_insensitive(): |
There was a problem hiding this comment.
No test for get_base_url relative URL fix
The PR adds test coverage for the case-insensitive matching fix (_tool_name_matches, filter_tools_by_allowed_tools) but does not include a test for the get_base_url relative URL fix — the other half of the changes described in the PR description. Per the project's custom rule, fixes should include passing tests as evidence. A unit test exercising the new branch would look like:
def test_get_base_url_resolves_relative_server_url():
"""Test that get_base_url resolves relative server URLs using the spec_path domain."""
from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import get_base_url
spec = {"servers": [{"url": "/api/v3"}]}
spec_path = "https://petstore3.swagger.io/api/v3/openapi.json"
result = get_base_url(spec, spec_path)
assert result == "https://petstore3.swagger.io/api/v3"
def test_get_base_url_absolute_server_url_unchanged():
"""Test that get_base_url returns absolute server URLs as-is."""
from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import get_base_url
spec = {"servers": [{"url": "https://api.example.com/v1"}]}
result = get_base_url(spec, spec_path=None)
assert result == "https://api.example.com/v1"Restore independent fixes from main that were collaterally removed when PR #23276 (staging_03_10 → main) carried a revert commit: - bedrock: restore output_config pop (PR #23240) - redact_messages: restore dict handling for ModelResponse (PR #23235) - model_checks: restore list() copies to avoid cache mutation (PR #23236) - openapi_to_mcp_generator: restore relative URL handling (PR #23238) - vertex_ai/gemini: restore _LITELLM_INTERNAL_EXTRA_BODY_KEYS check (PR #23131) - openai types: restore extra finish reasons (PR #22138) - completion_extras: restore usage transformation logic Accept main for: model_prices JSONs, credential_endpoints, team_endpoints, object_permission_utils, responses transformation.
…l_fixes fix(mcp): OpenAPI tool listing and execution for relative URLs and camelCase
Restore independent fixes from main that were collaterally removed when PR BerriAI#23276 (staging_03_10 → main) carried a revert commit: - bedrock: restore output_config pop (PR BerriAI#23240) - redact_messages: restore dict handling for ModelResponse (PR BerriAI#23235) - model_checks: restore list() copies to avoid cache mutation (PR BerriAI#23236) - openapi_to_mcp_generator: restore relative URL handling (PR BerriAI#23238) - vertex_ai/gemini: restore _LITELLM_INTERNAL_EXTRA_BODY_KEYS check (PR BerriAI#23131) - openai types: restore extra finish reasons (PR BerriAI#22138) - completion_extras: restore usage transformation logic Accept main for: model_prices JSONs, credential_endpoints, team_endpoints, object_permission_utils, responses transformation.
Summary
Fixes two issues with OpenAPI-based MCP servers (e.g. Petstore, fusion_litellm_mcp):
Issue 1: Tool Listing Failure (Case-Sensitivity Bug)
Problem: Servers with
allowed_toolsconfigured returned 0 tools when the OpenAPI spec used camelCase operationIds (e.g.addPet,updatePet).Root Cause:
addPet→addpet)allowed_toolscontained original camelCase names from the specFix: Modified
_tool_name_matches()inserver.pyto perform case-insensitive comparison.Issue 2: Tool Execution Failure (Relative URL Bug)
Problem: Tool calls failed with
InvalidURL: /api/v3/petfor specs with relative server URLs.Root Cause: Petstore and similar specs have
servers: [{"url": "/api/v3"}]- a relative path. The code returned this directly instead of resolving to a full URL.Fix: Modified
get_base_url()inopenapi_to_mcp_generator.pyto detect relative URLs and derive the full base URL fromspec_path(e.g.https://petstore3.swagger.io/api/v3).Test Coverage
test_tool_name_matches_case_insensitive- Core matching logictest_filter_tools_by_allowed_tools_case_insensitive- End-to-end filtering with camelCasetest_filter_tools_by_allowed_tools_no_filter- Servers without filters return all toolsMade with Cursor