-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added a mechanism to extract metadata from MCP tool call response #3339
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @DouweM, quoting you from #3323 (comment), I am pondering over whether it is better to edit the At present, if you take a look at the |
|
Quoting myself (#3339 (comment))
Note that I just did that @DouweM. Need to add more tests to improve coverage. |
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| return structured | ||
| if isinstance(structured, dict) and ( | ||
| (len(structured) == 1 and 'result' in structured) | ||
| or (len(structured) == 2 and 'result' in structured and '_meta' in structured) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the example at https://gofastmcp.com/servers/tools#toolresult-and-metadata, wouldn't the metadata be on result.meta? I don't think we should try to parse it directly from the result.structuredData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you regarding not modifying the structured result.
We may be, though, not thinking of the same metadata.
What you are referring to is a FastMCP tool call result metadata. In addition, the meta is an attribute of FastMCP ToolResult only from version 2.13.1 (according to https://gofastmcp.com/servers/tools#toolresult-and-metadata) while the version currently in use with Pydantic AI is 2.12.4.
What I have been referring to is the _meta in the latest MCP standard https://modelcontextprotocol.io/specification/2025-06-18/basic/index#meta. FastMCP wraps this in TextContent and other types of content too.
If we go with the FastMCP-specific meta then there is a possibility that a MCP server implemented without using that specific version (> 2.13.1) of FastMCP or implemented in a different language will not return the metadata in the expected ToolResult style object.
Having said that, there is a possibility that FastMCP is implementing what the upcoming MCP standard will be, as they seem to typically do. (I haven't dug through this in details.)
In summary:
- for structured content, I think we could go with
metaofToolResultbut I need to upgrade FastMCP for Pydantic AI to 2.13.1 or above; - however, we ought to support
_metaaliasedmetafor each content type.
Regarding (1), is this something I should do by myself?
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I noticed that in my original issue #3323, I had referred to both the FastMCP metadata and the standards _meta. Sorry for the confusion.
Ideally, we should support both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this in commit 39d47b5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see simplified parsing in the latest commit.
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| async def _map_tool_result_part( | ||
| self, part: mcp_types.ContentBlock | ||
| ) -> str | messages.BinaryContent | dict[str, Any] | list[Any]: | ||
| ) -> str | messages.ToolReturn | messages.BinaryContent | dict[str, Any] | list[Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work, because now the tool call could return a list of ToolReturns which is not supported: the tool needs to itself return a ToolReturn object.
I think we should build the list of output contents as we used to, and then if there's result.meta, return a ToolReturn with that metadata + the output content, instead of returning the output content directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but as I mentioned in my comment above, what I have been referring to is the _meta in the latest MCP standard https://modelcontextprotocol.io/specification/2025-06-18/basic/index#meta. This can be present in each content block, it seems.
If we return a single meta, there is no way to know how to merge multiple _meta that may be present in the content blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote the logic in commit 39d47b5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see simplified parsing in the latest commit.
|
@anirbanbasu I'll respond here at the top level with some thoughts because the 3 threads touch on overlapping topics:
Let me know what you think. The fact that there can be metadata at multiple levels in MCP but not currently in Pydantic AI makes this tricky! |
|
@DouweM thanks a lot for your thoughts. Here are my responses.
Yes, good idea. However, to pass the tool-result metadata, one needs to use FastMCP 2.13.1 (as mentioned in https://gofastmcp.com/servers/tools#toolresult-and-metadata) but this version is yet to be released. Thus, while I can parse tool-result metadata using the Related to this, I am attaching tool-result metadata using FastMCP's latest from GitHub in my MCP server template.
Yes, indeed. Thanks for pointing out. That link effectively points to this documentation in a comment, which is the same as the June standard.
Okay, but is it (i.e.,
Yes, that was my mistake. Apologies. I will fix it. If the data is structured data then I can attach the metadata as I am doing in my MCP server template.
Okay, makes sense.
Okay, that's reasonable.
The first option sounds reasonable as a default option even if the dictionary keys seem a bit hard-coded. Instead of your suggested second option, how about letting the |
|
@DouweM with the latest commit (39d47b5), I have attempted to implement 3, 4 and 5 from your message above (#3339 (comment)). Notes.
if isinstance(mapped_part, messages.BinaryContent):
identifier = mapped_part.identifier
return_values.append(f'See file {identifier}')
user_contents.append([f'This is file {identifier}:', mapped_part])
else:
user_contents.append(mapped_part)
|
|
@DouweM, I will let you know once I have completed all your requested changes. Sorry for delay. |
|
@anirbanbasu Please have a look at the failing tests! |
|
@DouweM updated the tests and included some |
| f'The return value of tool {tool_call.tool_name!r} contains invalid nested `ToolReturn` objects. ' | ||
| f'`ToolReturn` should be used directly.' | ||
| ) | ||
| # TODO: Keep updated with the binary parsing in mcp.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # TODO: Keep updated with the binary parsing in mcp.py | |
| # TODO: Keep updated with the binary parsing in `mcp.py` | |
| # or remove comment once https://github.com/pydantic/pydantic-ai/issues/3253 is done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this anymore because I don't construct BinaryContent in that way anymore?
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
|
|
||
| parts_with_metadata = [await self._map_tool_result_part(part) for part in result.content] | ||
| parts_only = [part for part, _ in parts_with_metadata] | ||
| # any_part_has_metadata = any(metadata is not None for _, metadata in parts_with_metadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not using this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's gone. Please see simplified parsing in the latest commit.
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
|
|
||
| # The following branching cannot be tested until FastMCP is updated to version 2.13.1 | ||
| # such that the MCP server can generate ToolResult and result.meta can be specified. | ||
| # TODO: Add tests for the following branching once FastMCP is updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a comment here so we don't lose this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this in once we figure out about FastMCP 2.13.1 upgrade or if I can come up with a test whereby I construct the metadata by hand?
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| # such that the MCP server can generate ToolResult and result.meta can be specified. | ||
| # TODO: Add tests for the following branching once FastMCP is updated. | ||
| if len(parts_metadata) > 0: | ||
| if result.meta is not None and len(result.meta) > 0: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not have any no-covers if we can help it!
Edit: You already pointed out why you did that, never mind :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is this equivalent to if result.meta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have no no-cover but that's because I have not written the tests.
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| # The following branching cannot be tested until FastMCP is updated to version 2.13.1 | ||
| # such that the MCP server can generate ToolResult and result.meta can be specified. | ||
| # TODO: Add tests for the following branching once FastMCP is updated. | ||
| if len(parts_metadata) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, I prefer if parts_metadata over specifically length-checking, unless we want to treat things like None and empty differently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see simplified parsing in the latest commit.
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| else: | ||
| return_metadata = {'content': parts_metadata} # pragma: no cover | ||
| else: | ||
| if result.meta is not None and len(result.meta) > 0: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be elif result.meta:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see simplified parsing in the latest commit.
pydantic_ai_slim/pydantic_ai/mcp.py
Outdated
| if len(resource_result.contents) == 1 | ||
| else [self._get_content(resource) for resource in resource_result.contents] | ||
| ) | ||
| # Check if metadata already exists. If so, merge it with nested the resource metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we dedupe any of this with the above with some helper functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't exist anymore. Please see simplified parsing in the latest commit.
tests/test_mcp.py
Outdated
| assert result == snapshot(32.0) | ||
|
|
||
|
|
||
| async def test_tool_response_metadata(run_context: RunContext[int]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want tests of every combination that we've covered up above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shall write new tests.
|
@anirbanbasu Thanks Anirban, I think the logic is reasonable now, although I'm starting to have doubts about how brittle it will be to consume this metadata from code, as it's not type safe and will require lots of I'm thinking we should add |
|
@DouweM I would agree that packaging the parts metadata in the way we are doing in a dictionary is brittle. Having a metadata in the In addition, I wonder if there is any need to package metadata in one place in the return What do you think? Lastly, is there a plan to upgrade FastMCP to 2.13.1, now that it is an officially released version? |
@anirbanbasu Yep.
Yep that's what I'm hoping we can do this way.
Feel free to update! |
|
@DouweM okay, I will modify the However, we will run into an issue with
It seems that updating it to 2.13.1 is impossible for now due to other dependencies. % uv lock -P fastmcp --dry-run
Resolved 381 packages in 191ms
Update fastmcp v2.12.4 -> v2.12.5This would mean that an exhaustive test set cannot be implemented until we can get 2.13.1. |
@anirbanbasu Hmm, I don't want mcp types bleeding through. Could we use
Ok let's get as far as we can, and then see about fastmcp/coverage. Do you know what dependency causes the issue? We can talk to the fastmcp team if needed |
f70d332 to
adef17f
Compare
|
The branch had too complex conflicts and the discussion above shows that a different, simpler approach is to be taken. Starting afresh. |
|
No tests enabled yet. |
…ntent including resources. chore: Updated tests but have not added tests specific to metadata, hence coverage should fail.
This is now done. The metadata parsing is simpler. I am using
Running fastmcp v2.12.4
├── authlib v1.6.4
│ └── cryptography v46.0.2
│ └── cffi v2.0.0
│ └── pycparser v2.22
├── cyclopts v3.24.0
│ ├── attrs v25.1.0
│ ├── docstring-parser v0.17.0
│ ├── rich v13.9.4
│ │ ├── markdown-it-py v3.0.0
│ │ │ └── mdurl v0.1.2
│ │ └── pygments v2.19.1
│ └── rich-rst v1.3.1
│ ├── docutils v0.22.2
│ └── rich v13.9.4 (*)
├── exceptiongroup v1.2.2
├── httpx v0.28.1
│ ├── anyio v4.8.0
│ │ ├── idna v3.10
│ │ ├── sniffio v1.3.1
│ │ └── typing-extensions v4.12.2
│ ├── certifi v2025.1.31
│ ├── httpcore v1.0.7
│ │ ├── certifi v2025.1.31
│ │ └── h11 v0.14.0
│ ├── idna v3.10
│ └── h2 v4.2.0 (extra: http2)
│ ├── hpack v4.1.0
│ └── hyperframe v6.1.0
├── mcp v1.18.0
│ ├── anyio v4.8.0 (*)
│ ├── httpx v0.28.1 (*)
│ ├── httpx-sse v0.4.0
│ ├── jsonschema v4.25.0
│ │ ├── attrs v25.1.0
│ │ ├── jsonschema-specifications v2025.4.1
│ │ │ └── referencing v0.36.2
│ │ │ ├── attrs v25.1.0
│ │ │ ├── rpds-py v0.26.0
│ │ │ └── typing-extensions v4.12.2
│ │ ├── referencing v0.36.2 (*)
│ │ └── rpds-py v0.26.0
│ ├── pydantic v2.11.7
│ │ ├── annotated-types v0.7.0
│ │ ├── pydantic-core v2.33.2
│ │ │ └── typing-extensions v4.12.2
│ │ ├── typing-extensions v4.12.2
│ │ ├── typing-inspection v0.4.0
│ │ │ └── typing-extensions v4.12.2
│ │ └── email-validator v2.3.0 (extra: email)
│ │ ├── dnspython v2.8.0
│ │ └── idna v3.10
│ ├── pydantic-settings v2.8.1
│ │ ├── pydantic v2.11.7 (*)
│ │ └── python-dotenv v1.1.1
│ ├── python-multipart v0.0.20
│ ├── sse-starlette v2.2.1
│ │ ├── anyio v4.8.0 (*)
│ │ └── starlette v0.45.3
│ │ └── anyio v4.8.0 (*)
│ ├── starlette v0.45.3 (*)
│ ├── uvicorn v0.34.0
│ │ ├── click v8.1.8
│ │ └── h11 v0.14.0
│ ├── python-dotenv v1.1.1 (extra: cli)
│ └── typer v0.16.0 (extra: cli)
│ ├── click v8.1.8
│ ├── rich v13.9.4 (*)
│ ├── shellingham v1.5.4
│ └── typing-extensions v4.12.2
├── openapi-core v0.19.5
│ ├── isodate v0.7.2
│ ├── jsonschema v4.25.0 (*)
│ ├── jsonschema-path v0.3.4
│ │ ├── pathable v0.4.4
│ │ ├── pyyaml v6.0.2
│ │ ├── referencing v0.36.2 (*)
│ │ └── requests v2.32.3
│ │ ├── certifi v2025.1.31
│ │ ├── charset-normalizer v3.4.1
│ │ ├── idna v3.10
│ │ └── urllib3 v2.3.0
│ ├── more-itertools v10.8.0
│ ├── openapi-schema-validator v0.6.3
│ │ ├── jsonschema v4.25.0 (*)
│ │ ├── jsonschema-specifications v2025.4.1 (*)
│ │ └── rfc3339-validator v0.1.4
│ │ └── six v1.17.0
│ ├── openapi-spec-validator v0.7.2
│ │ ├── jsonschema v4.25.0 (*)
│ │ ├── jsonschema-path v0.3.4 (*)
│ │ ├── lazy-object-proxy v1.12.0
│ │ └── openapi-schema-validator v0.6.3 (*)
│ ├── parse v1.20.2
│ ├── typing-extensions v4.12.2
│ └── werkzeug v3.1.1
│ └── markupsafe v2.1.5
├── openapi-pydantic v0.5.1
│ └── pydantic v2.11.7 (*)
├── pydantic[email] v2.11.7 (*)
├── pyperclip v1.9.0
├── python-dotenv v1.1.1
└── rich v13.9.4 (*)
(*) Package tree already displayed
Note that the tests specific to metadata have not been written yet, so 100% coverage should fail. |
…ot be tested until FastMCP can be upgraded to 2.13.1.
Fixes: #3323
TextContentand todict[str, Any]as a_metakey.TextContent.ToolResult.metafrom FastMCP 2.13.1 as documented at https://gofastmcp.com/servers/tools#toolresult-and-metadata.