fix(mcp): use configured MCP server URL and unwrap taskgroup errors - #583
fix(mcp): use configured MCP server URL and unwrap taskgroup errors#583omkargaikwad23 wants to merge 1 commit into
Conversation
…up errors Do not alter or append /mcp to the configured MCP server URL, allowing endpoints with explicit paths (e.g. /mcp/instance_manage) to function as intended. Add _format_error to unpack ExceptionGroup/TaskGroup leaf errors for actionable error reporting.
|
/gcbrun |
|
/gcbrun |
omkargaikwad23
changed the title
wangauone
left a comment
wangauone
left a comment
There was a problem hiding this comment.
LGTM with some minor comments
| """Raised when a tools spec cannot be fetched or parsed.""" | ||
|
|
||
|
|
||
| def sanitize_url(url: str) -> str: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| if not rstripped_url.endswith("/mcp"): | ||
| return f"{rstripped_url}/mcp" | ||
| return rstripped_url | ||
| def _format_error(e: BaseException) -> str: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sg! I will create a followup PR to add required test files.
| stripped_url = (url or "").strip() | ||
| if stripped_url.startswith(("http://", "https://")): | ||
| url_with_scheme = stripped_url | ||
| elif "localhost" in stripped_url: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
3 participants
Description
mcp_client.pythat forced an/mcpsuffix onto every endpoint. Configured URLs are now trusted directly (with whitespace trimmed), allowing endpoints with custom paths (e.g.https://<host>/mcp/<service>orhttps://<host>/api/v1/mcp) to work without being broken by redundant path appending._format_errorhelper inmcp_client.pyto extract leaf errors from nestedExceptionGroup/TaskGroupexceptions, replacing the generic"unhandled errors in a TaskGroup (1 sub-exception)"with actionable error causes in reports and UI views.Testing