Skip to content

fix(mcp): use configured MCP server URL and unwrap taskgroup errors #583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
omkargaikwad23 merged 1 commit into from
Aug 28, 2026
Merged

fix(mcp): use configured MCP server URL and unwrap taskgroup errors #583

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 12 additions & 20 deletions evalbench/generators/models/mcp_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,11 @@ class McpToolsError(Exception):
"""Raised when a tools spec cannot be fetched or parsed."""


def sanitize_url(url: str) -> str:

@wangauone wangauone Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the same function in here https://github.com/GoogleCloudPlatform/evalbench/blob/main/evalbench/generators/models/mcp_tools.py#L93-L109. Do we also want to make some similar changes there as well?

@omkargaikwad23 omkargaikwad23 Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's out of scope for now as those files are related to mcp-readability flow. Maybe I can check with owner of the file.

"""Ensure the URL has a scheme and ends with ``/mcp``.

``/mcp`` is the conventional path where the MCP protocol is exposed.
"""
stripped_url = (url or "").strip()
if stripped_url.startswith(("http://", "https://")):
url_with_scheme = stripped_url
elif "localhost" in stripped_url:

@wangauone wangauone Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will lost the capability to parse the value like localhost:8080 or sqladmin.googleapis.com (value without http prefix)? Just want to make sure this is the expected behavior

@omkargaikwad23 omkargaikwad23 Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. We are assuming the fully qualified URL will be provided for now. We can definitely revisit this and add support for scheme-less URLs in the future if needed

url_with_scheme = f"http://{stripped_url}"
else:
url_with_scheme = f"https://{stripped_url}"

rstripped_url = url_with_scheme.rstrip("/")
if not rstripped_url.endswith("/mcp"):
return f"{rstripped_url}/mcp"
return rstripped_url
def _format_error(e: BaseException) -> str:

@wangauone wangauone Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Ideally, it would be good to have a mcp_client_test.py to do some testing. : ) But I think it is okay to limit the scope of this PR.

@omkargaikwad23 omkargaikwad23 Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sg! I will create a followup PR to add required test files.

"""Unpack ExceptionGroup/TaskGroup leaf errors for error messages."""
if hasattr(e, "exceptions") and getattr(e, "exceptions"):
return f"{e} ({', '.join(map(_format_error, e.exceptions))})"
return f"{type(e).__name__}: {e}" if str(e) else type(e).__name__


def auth_headers(server_config: dict) -> dict | None:
Expand Down Expand Up @@ -73,15 +61,18 @@ def fetch_tools_http(
raw_url: str, headers: dict | None = None, timeout: int = 30
) -> list[mcp_types.Tool]:
"""``tools/list`` over Streamable HTTP."""
url = sanitize_url(raw_url)
url = (raw_url or "").strip()
try:
return anyio.run(
functools.partial(_async_fetch_http, url, headers, timeout)
)
except McpToolsError:
raise
except Exception as e:
raise McpToolsError(f"Failed to fetch tools from {url}: {e}") from e
err = _format_error(e)
raise McpToolsError(
f"Failed to fetch tools from {url}: {err}"
) from e


def fetch_tools_stdio(
Expand All @@ -99,8 +90,9 @@ def fetch_tools_stdio(
except McpToolsError:
raise
except Exception as e:
err = _format_error(e)
raise McpToolsError(
f"Failed to fetch tools from stdio server '{command}': {e}"
f"Failed to fetch tools from stdio server '{command}': {err}"
) from e


Expand Down
Loading