Uh oh!
There was an error while loading. Please reload this page.
fix(mcp): support MCP 2.0 tool schemas - #420
Conversation
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. I verified the fix against real installs of mcp 1.28.1 and 2.0.0, and reading the schema through the serialization alias works on both, with no pydantic serializer warnings. The diagnosis is right and the mechanism is the one I'd have picked.
My concern is scope rather than correctness. MCP 2.0 applied the same rename a second time in this file: result.structuredContent at lines 330-331 became structured_content. That one sits on the tool call path and it fails quietly. The AttributeError gets wrapped into a RuntimeError, handed to the model as a tool result string, and reported to the dashboard as agent_tool_complete. A run can finish "successfully" with the model having worked around a tool that never ran.
Second thing worth a look: the test swapped the 1.x fixture for a 2.x one rather than adding to it. If you change line 207 to the 2.x-only tool.input_schema, tests/test_mcp still reports 53 passed, while every user on the pinned floor breaks.
Details inline.
| "name": prefixed_name, | ||
| "description": tool.description or "", | ||
| "input_schema": tool.inputSchema, | ||
| "input_schema": tool.model_dump(by_alias=True)["inputSchema"], |
There was a problem hiding this comment.
Confirmed this returns the schema on both 1.28.1 and 2.0.0.
The same rename hits result.structuredContent at lines 330-331, which MCP 2.0 renamed to structured_content. That path fails quietly: the AttributeError is caught at line 353, re-raised as a RuntimeError, and mcp_toolset.py turns it into a tool result the model reads as an ordinary tool failure. max_retries=0 there, so there is no retry, and the dashboard records it as agent_tool_complete rather than an error. Only structured-output tools reach it, since not response_text short-circuits whenever there is text content, which is why it survives a smoke test.
One helper covering both sites would be easier to keep in sync than two separate idioms:
_MISSING=object()
def_mcp_field(model: Any, current_name: str, legacy_name: str) ->Any:
"""Read a model field renamed between MCP 1.x and 2.x."""value=getattr(model, current_name, _MISSING)
ifvalueis_MISSING:
value=getattr(model, legacy_name)
returnvalueTwo things I ran into while testing that are worth knowing. model_dump is fine here on Tool (about 1.8 microseconds, once per tool at connect) but not at the call_tool site, where CallToolResult carries the content payload up to max_chars, so dumping it per call copies the whole thing. And the shorter getattr(m, new, None) or getattr(m, legacy) is subtly wrong for this field, because structured_content is legitimately None on most results and would fall through to the 1.x name on the common path.
There was a problem hiding this comment.
Both points landed. The structuredContent read at lines 330-331 now goes through a shared _mcp_field(model, current_name, legacy_name) helper, exactly as you sketched — sentinel-based, with the legacy getattr raising AttributeError when neither name exists (same behavior as the pre-fix code on the call_tool site, where the existing wrap at line 353 still applies). The line-207 read moved to the same helper. I verified against real installs of mcp 1.28.1 and 2.0.0: Tool.inputSchema/input_schema and CallToolResult.structuredContent/structured_content are the two rename pairs, and on 2.0.0 all three aliases (alias, validation_alias, serialization_alias) hold the camelCase name.
On your two testing notes: model_dump is kept out of the call_tool site for the reason you give (the payload copy up to max_chars per call) — and the discovery site no longer uses it either, since the helper covers both. The or-chain trap is exactly why the sentinel is there: structured_content being legitimately None on most 2.x results would have fallen through to the legacy name on the common path.
| """Requirement: tool discovery accepts MCP 2.x snake-case model fields.""" | ||
| from pydantic import BaseModel, Field | ||
| class MCP2Tool(BaseModel): |
There was a problem hiding this comment.
The 2.x fixture replaced the 1.x one rather than joining it, so nothing now covers the version the lockfile actually pins. I checked by setting line 207 to the 2.x-only tool.input_schema: tests/test_mcp still reports 53 passed, even though that breaks every user on mcp 1.28.1.
mcp is a hard dependency, so mcp.types.Tool imports freely here, and Tool.model_validate({"name": ..., "inputSchema": ...}) works on both majors. Parametrizing over the real type plus this stand-in would pin both directions, and it keeps working when the floor eventually moves.
Worth noting the stand-in is faithful today. I checked alias, validation_alias and serialization_alias on real mcp 2.0.0 and all three are inputSchema. The gap is that nothing would tell you if that ever stopped being true.
There was a problem hiding this comment.
Fixed. test_connect_server_mocked is now parametrized over both shapes: a real mcp.types.Tool (the 1.28.1 shape the lockfile pins, built via Tool.model_validate({"name": ..., "inputSchema": ...})) and the MCP-2.x stand-in, with ids mcp1-real/mcp2-standin. A new test_call_tool_structured_content_both_shapes does the same for the call_tool structured-content branch against a real CallToolResult and a 2.x-shaped stand-in. I mutation-checked all four one-name-only reads (each site × each single name) and confirmed each fails exactly the opposite parametrization — so a 2.x-only line 207 now fails mcp1-real instead of passing 53 green.
On the drift risk you flagged: fair — nothing pins the stand-in to a real 2.x install today. For now the parametrization over the real 1.x type pins one direction against the actual SDK, and the 2.x direction rests on the alias check I ran manually. A guard asserting the stand-in's aliases against a real 2.x install would need 2.x in the tree, which the floor doesn't allow yet — leaving that for when the floor moves.
| mock_tool.description = "Search the web" | ||
| mock_tool.inputSchema = {"type": "object", "properties": {"query": {"type": "string"}}} | ||
| """Requirement: tool discovery accepts MCP 2.x snake-case model fields.""" | ||
| from pydantic import BaseModel, Field |
There was a problem hiding this comment.
Minor: the other function-local imports in this file exist because the module has to be imported under the MCP_SDK_AVAILABLE patch. pydantic has no such constraint, so this one reads as an exception to a pattern that otherwise carries a signal. Moving it and the stand-in to module scope would also let a call_tool test reuse the model.
There was a problem hiding this comment.
Done. The pydantic import and both stand-ins (MCP2Tool, MCP2CallToolResult) are at module scope now, and the new call_tool test reuses the models. You're right that the other function-local imports carry a signal (the MCP_SDK_AVAILABLE patch) that pydantic doesn't — the local import read as an exception to that pattern.
| Conductor now reads the aliased model representation, preserving compatibility | ||
| with both MCP 1.x and 2.x. |
There was a problem hiding this comment.
Discovery is compatible after this change. Tool execution is not, because of the structuredContent read at mcp/manager.py:330-331. Someone who hits that will read this entry and rule out MCP 2.0 as the cause, which is the debugging session the fix is meant to prevent.
If the second site lands in this PR, this sentence becomes true as written and you can ignore the suggestion. Otherwise it needs narrowing:
| Conductor now reads the aliased model representation, preserving compatibility | |
| with both MCP 1.x and 2.x. | |
| Conductor now reads the aliased model representation, so tool discovery works | |
| under both MCP 1.x and 2.x. Tool calls whose results carry only structured | |
| content are still affected on 2.x. |
There was a problem hiding this comment.
The second site landed in this PR, so the entry now covers the tool-call path as well as discovery rather than the narrowed wording. It names both renames (inputSchema/input_schema and structuredContent/structured_content) and the quiet-failure mode you described, so someone hitting either can no longer rule out MCP 2.0 as the cause.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #420 +/- ##
=======================================
Coverage ? 91.90% =======================================
Files ? 144 Lines ? 23260 Branches ? 0 =======================================
Hits ? 21378 Misses ? 1882 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…results MCP 2.0 applied the same snake_case rename a second time in this file: result.structuredContent at the call_tool site became structured_content. That path failed quietly — the AttributeError was wrapped into a RuntimeError the model read as an ordinary tool failure, with no retry and no error surfacing on the dashboard. Only structured-output tools reached it, since any text content short-circuits the branch. Both rename sites now share one helper that tries the 2.x field name and falls back to the 1.x one via a sentinel (a plain "or" chain is wrong here: structured_content is legitimately None on most 2.x results and would fall through to the legacy name on the common path). model_dump is deliberately not used at the call_tool site — CallToolResult carries the content payload up to max_chars, so dumping per call copies the whole thing. Tests now pin both directions instead of only the 2.x shape: the connect_server test is parametrized over a real mcp.types.Tool (the 1.28.1 shape the lockfile pins) and an MCP-2.x stand-in, and a new call_tool test covers the structured-content branch against both the real CallToolResult and a 2.x-shaped stand-in. Each one-name-only read was mutation-checked to fail the opposite parametrization. The pydantic stand-ins moved to module scope; unlike the module's other local imports, pydantic has no MCP_SDK_AVAILABLE constraint. The changelog entry now covers the tool-call path as well as discovery.
db5531c to
0ba4db4CompareJason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
Resolves the CHANGELOG conflict: microsoft#420's MCP 2.0 entry and this branch's OpenAI provider entries both landed in the previously-empty [Unreleased] section, so git could not merge them. Both are kept, with the two Fixed items combined under a single heading. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Testing
uv run pytest -q tests/test_mcp/test_manager.py tests/test_providers/test_pydantic_ai_mcp.pymake checkFixes#419