Expand MCP tool metadata: explicit output schemas and hint annotations#34
Expand MCP tool metadata: explicit output schemas and hint annotations#34aerdasaliko wants to merge 8 commits intogalaxyproject:mainfrom
Conversation
…due to worsened performance
dannon
left a comment
There was a problem hiding this comment.
Thanks for putting this together — adding output schemas and annotations is a worthwhile improvement.
A few things to address before this can move forward:
String concatenation bug (throughout)
Python's implicit string concatenation is biting you in several places where adjacent strings lack a trailing space, producing mangled descriptions. For example in search_tools:
"description": "Tools whose names match the query string, returned as an array."
"Each object contains a tool id..."This concatenates to "...array.Each object..." (no space after the period). The same issue appears in:
run_tooloutputs —"execution.Each"get_tool_panel—"tools.Each"create_historyid —"identifier.This"and"such as:run_tool"get_job_detailsdataset_id —"retrieved.Reusable"upload_file_from_urloutputs —"URL.Each"get_iwc_workflows—"manifest.'trsID'"search_iwc_workflowstrsID —"workflow.Can"list_workflowsitems —"attributes.The"get_workflow_detailsdescription —"steps,inputs"(missing space after comma) and"parameters.An"
Fix is straightforward — add a trailing space before the closing quote on each first string, or wrap in parentheses:
"description": (
"Tools whose names match the query string, returned as an array. "
"Each object contains a tool id..."
),list_history_ids return type change
This PR changes the return type from list[dict[str, str]] to dict[str, Any] and wraps the return value in {"histories": [...]}. That's a behavioral change to existing clients, not just metadata. If we want this change it should be called out explicitly and the existing tests updated.
cancel_workflow_invocation missing description
Every other tool has a description= in the decorator — this one was missed.
upload_file_from_url missing required
The nearly identical upload_file has "required": ["outputs", "jobs", ...] in its schema, but upload_file_from_url doesn't. Looks like an oversight.
Inconsistent idempotentHint
Some read-only tools include idempotentHint: True (e.g. search_tools, get_tool_details) while others that are equally idempotent omit it (e.g. get_user, get_histories, list_history_ids, get_history_contents, get_dataset_details). Worth making this consistent — if a tool is read-only and deterministic given the same server state, it should have idempotentHint: True.
Rebase needed
The main branch has moved since this was opened (fastmcp 3 bump and IWC function fixes). You'll want to rebase on current main to make sure the decorator kwargs work with fastmcp >= 3.0.0.
|
Thanks for the thorough review! I've made the following initial quick fixes:
Regarding As for |
|
Thanks for the feedback. I'm closing this PR as it's become too stale. Feel free to reopen if this work is still needed |
Description
Type of Change