Skip to content

Conversation

fennb
Copy link

@fennb fennb commented Sep 29, 2025

Addresses #1783.

Realised that @Kludex started working on this (#3020) at pretty much the same time/before I had a chance to push, which probably invalidates this work, but creating a PR anyway in case some part of it is useful!

"""
async with self: # Ensure server is running
result = await self._client.read_resource(AnyUrl(uri))
return result.contents
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the same logic we have in the ResourceLink handling below, to turn this into Pydantic AI native types?

            resource_result: mcp_types.ReadResourceResult = await self._client.read_resource(part.uri)
            return (
                self._get_content(resource_result.contents[0])
                if len(resource_result.contents) == 1
                else [self._get_content(resource) for resource in resource_result.contents]
            )

We can then also use this method there!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, that would mean we'd lose the meta field (as pointed out in #2288), as we don't currently have metadata on BinaryContent, let alone on str.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kludex What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I was thinking of doing this (and had exactly the same thought) but was trying to keep the initial PR as minimal as possible.

Using this approach would definitely justify proxying the method (rather than just exposing ClientSession directly like @Kludex's PR).

I think converting the types would make it more consistent with how the rest of the system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fennb Yeah I'm inclined to convert the objects, despite us losing the metadata, and if the user needs it, they can use the client directly.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to lose the metadata for now.

Copy link
Author

Choose a reason for hiding this comment

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

My most recent updates means that read_resource() now (effectively) returns str | BinaryContent (as discussed).

In this particular context, could we map Resource.meta to BinaryContent.vendor_metadata? If this is only being used in an MCPServer.read_resource() context, or would this cause issues?

Also, slightly crazy idea, but to solve losing meta on str results, what about something like:

class TextContent(str):
     meta: dict[str, Any] | None

    def __new__(cls, value, meta: dict[str, Any] | None = None):
        instance = str.__new__(cls, value)
        instance.meta = meta
        return instance

So it still is a str, it just has bonus extra bits? (I have no intuition what real-world issues this might cause).

result = await self._client.list_resources()
return result.resources

async def list_resource_templates(self) -> list[mcp_types.ResourceTemplate]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kludex You suggested having these methods return types of our own, but I'm not super convinced it's worth it to redefine this:

class ResourceTemplate(BaseMetadata):
    """A template description for resources available on the server."""

    uriTemplate: str
    """
    A URI template (according to RFC 6570) that can be used to construct resource
    URIs.
    """
    description: str | None = None
    """A human-readable description of what this template is for."""
    mimeType: str | None = None
    """
    The MIME type for all resources that match this template. This should only be
    included if all resources matching this template have the same type.
    """
    annotations: Annotations | None = None
    meta: dict[str, Any] | None = Field(alias="_meta", default=None)
    """
    See [MCP specification](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/47339c03c143bb4ec01a26e721a1b8fe66634ebe/docs/specification/draft/basic/index.mdx#general-fields)
    for notes on _meta usage.
    """
    model_config = ConfigDict(extra="allow")

And Annotations that it references:

class Annotations(BaseModel):
    audience: list[Role] | None = None
    priority: Annotated[float, Field(ge=0.0, le=1.0)] | None = None
    model_config = ConfigDict(extra="allow"

And Role that that references:

Role = Literal["user", "assistant"]

... instead of just letting the user use MCP types directly for certain things. Although I suppose in that case, instructing users to just use client directly to get access to MCP stuff directly would suffice as well.

There's also the fact that list_tools already returns list[mcp_types.Tool].

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I'm fine with this. @fennb Can you define our own matching types please, as dataclasses with snake_case fields rather than camelCase?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think it's one or the other, right? Either just expose client directly and let users use the native mcp types or convert the types and expose directly on MCPServer?

The place I've left it (which is kind of a mix) is probably "wrong" now that I reflect on it.

Happy to take this further and try to add the native type conversion (or @Kludex can run with it) if we think this is the right path.

Copy link
Author

Choose a reason for hiding this comment

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

Hah, we submitted comments at the same time @DouweM - just saw your most recent comment. I'll run with this this direction and push an update.

Copy link
Member

Choose a reason for hiding this comment

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

If we are not redefining the types, I don't think we should expose those methods. I think exposing the client would be enough.


Is it really worth defining those types here, tho? 👀

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Kludex If we'll have read_resource that returns our types, which would be useful, I think we should also have list_resources, which should then also return our types

Copy link
Author

Choose a reason for hiding this comment

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

Is it really worth defining those types here, tho?

Great question.

@Kludex If we'll have read_resource that returns our types, which would be useful, I think we should also have list_resources, which should then also return our types

Yeah, having had a bit more of a chance to look at/think about this, I think this is where I landed too. All public MCPServer methods should return native Pydantic AI types, which is (sort of) why MCPServer wraps mcp's ClientSession in the first place.

I'll get going on implementing this and you guys can let me know what you think.

Side note 1: list_tools(self) -> list[mcp_types.Tool] is an oddity and I suspect shouldn't exist (or shouldn't be public) - MCPServer.get_tools() is the only thing that uses it and is probably the right "public" method for users to use. Do we want to do anything here?

Side note 2: As part of investigating things, I looked into the origins of the ResourceLink / _get_content() functionality mentioned above: #2094 (comment) - I'm not sure this behaviour is quite right (though I absolutely understand why the decision was made that way). The MCP spec says that tools can return both Resource Links and Embedded Resources, but they're not the same thing (see here).

At the moment, Pydantic transparently fetches resources from the link and returns them inline to the model as part of the tool call result. But surely that's not the intended use given the fact there's already embedded resources as a separate type?

ie: Imagine a hypothetical tool called find_relevant_documents(query: str, limit: int = 50) that returned 50 resource links to documents (each linked document being 1MB) so that a user can click on the one they want to load/select it/whatever. At the moment, Pydantic AI would try to fetch all the docs immediately and include them in context transparently, which seems... wrong.

An approach would be to leave them as links and then it's up to the user to prompt the agent appropriately in telling it what to do with them (ie: display them verbatim for the actual client to handle or whatever). This is less controversial if the ResourceLink.uri is https://some.server.com/path/file.ext but weirder if it's document://legal/archive/blah.txt (or whatever). I don't really know what an agent would do in that case. Arguably, ResourceLinks maybe shouldn't be used this way, but I think blindly fetching them may not be the right default?

Either way, I've gotten off topic and can separately lodge the above as a separate issue if you think it's worth it. I don't know how much changing this will break existing users expectations or not.

Copy link
Collaborator

@DouweM DouweM Oct 3, 2025

Choose a reason for hiding this comment

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

list_tools(self) -> list[mcp_types.Tool] is an oddity and I suspect shouldn't exist (or shouldn't be public) - MCPServer.get_tools() is the only thing that uses it and is probably the right "public" method for users to use. Do we want to do anything here?

I agree, but we can't change that as it'd be a breaking change and we're on v1 now so can't make those until v2.


I agree the resource link behavior is probably incorrect. I tried to get clarification on that in modelcontextprotocol/modelcontextprotocol#872 (reply in thread), but didn't hear back. It would be easier if the model had a read_resource tool available to it, so it can choose which to load, but that'd need to be provided by the user. Maybe MCPServer should have a flag to include that as a tool, and if provided, we wouldn't auto-read resource links?

A separate issue would be great!

Copy link
Member

Choose a reason for hiding this comment

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

I didn't even think about list_tools because it's called internally anyway, but yeah, I agree it should return the PydanticAI types.

As for the new methods, I think they should return the PydanticAI types - where are we on this?

Copy link
Author

Choose a reason for hiding this comment

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

Good timing (and sorry for slow follow up). Just pushed updates. This is, admittedly, a fair amount of boilerplate to not achieve a massive amount (at this point), but I can see some future reasons related to my commentary on ResourceLink above that this will become more important (more on this in the other issue which I'll create shortly).

@DouweM DouweM self-assigned this Sep 30, 2025
@DouweM DouweM requested a review from Kludex September 30, 2025 20:54
description=mcp_resource.description,
mime_type=mcp_resource.mimeType,
size=mcp_resource.size,
annotations=(
Copy link
Author

Choose a reason for hiding this comment

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

To my frustration, there is no support for annotations in mcp.server.fastmcp.server.FastMCP.resource: https://github.com/modelcontextprotocol/python-sdk/blob/main/src/mcp/server/fastmcp/server.py#L480

This is what we use for testing in mcp_server.py, which means I couldn't actually write an integration test (at least using that pattern) to test it all end to end.

Point me in the right direction if there's something else I should do.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@fennb That's annoying, so they support it on their client but not their server :/

@Kludex Any ideas?

Copy link
Author

Choose a reason for hiding this comment

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

@DouweM @Kludex It is indeed annoying. Rather than having to drop down to the low level server for this (or all) tests, I decided to fix the issue upstream: modelcontextprotocol/python-sdk#1468

This is, admittedly, getting pretty yak shave-y, but may as well make things better for everyone?

@phemmer
Copy link
Contributor

phemmer commented Oct 12, 2025

I was just looking at this as I need resources support, and noticed this doesn't provide any way to determine whether the server even supports resources. The only way to tell with this code would be to attempt to make a resource call (e.g. read_resource) and wait for a failure.

I think this should be better exposed as I would like my application to switch to alternate behavior if the MCP server does not support resources. And would like to do so without having to make a call and see if it fails, and then differentiate whether that failure was because of lack of support, or some other reason.

The MCP spec says that the resources capability is used to advertise resources support. However from looking at the code, only serverInfo is captured from the initialize result, and the rest of the initialize response (which includes resources support) is thrown away.

https://github.com/fennb/pydantic-ai/blob/b8424d80eeeb46aef3ad8ae49ae29b1ef820c7af/pydantic_ai_slim/pydantic_ai/mcp.py#L375-L378

@phemmer
Copy link
Contributor

phemmer commented Oct 12, 2025

Another thing I noticed going through the code, is that for EmbeddedResource handling, the metadata is being thrown away. I need to know the URI of the resource.

I'm not sure if addressing this is even in scope of this PR, as ResourceLink handling is also bad, as it automatically fetches the content instead of providing the link. But issue #3099 addresses this. So I'm not sure if this EmbeddedResource metadata loss issue goes here, there, or in a new issue entirely. But figured it would be at least worth mentioning here.

@fennb
Copy link
Author

fennb commented Oct 12, 2025

The MCP spec says that the resources capability is used to advertise resources support. However from looking at the code, only serverInfo is captured from the initialize result, and the rest of the initialize response (which includes resources support) is thrown away.

That's a great point (and not something I was looking at in my changes). Capabilities presumably hasn't been an issue up until this point because MCP was only used for tools in Pydantic AI and it was assumed you wouldn't use an MCP server unless it had tool capabilities.

I'm not quite sure the right way to support this. The upstream mcp library has types for each capability, but replicating/proxying these as native types seems quite verbose.

There is an existing pattern, which is how server_info() is handled (as you point out @phemmer), where the details are stored/cached at initalize time and exposed like so:

@property
    def server_info(self) -> mcp_types.Implementation:
        """Access the information send by the MCP server during initialization."""
        if getattr(self, '_server_info', None) is None:
            raise AttributeError(
                f'The `{self.__class__.__name__}.server_info` is only instantiated after initialization.'
            )
        return self._server_info

We could follow this pattern and just return mcp SDK types. Alternatively, since we don't support listChanged anyway, could we maybe just make them a set of ServerCapabilities literals?

Maybe something like:

ServerCapabilities = Literal[
      "experimental",
      "logging",
      "prompts",
      "resources",
      "tools",
      "completions"
  ]

# MCPServer
@property
def capabilities() -> set[ServerCapabilities]:
    ...

Meh, I guess that's kind ugly: if "resources" in server.capabilities(): ... - perhaps the way mcp already works is superior, so we'd end up with something more like: if server.capabilities.resources: ...? I'm not familiar enough to know what would be more idiomatic for Pydantic.

I don't want this PR to drag on any longer than it already has, but this seems like a reasonable/incremental change overall?

What do you think @DouweM / @Kludex ?

@fennb
Copy link
Author

fennb commented Oct 12, 2025

I'm not sure if addressing this is even in scope of this PR, as ResourceLink handling is also bad, as it automatically fetches the content instead of providing the link. But issue #3099 addresses this.

Yep yep - do you mind moving this topic across to #3099 ?

@DouweM
Copy link
Collaborator

DouweM commented Oct 13, 2025

So I'm not sure if this EmbeddedResource metadata loss issue goes here, there, or in a new issue entirely.

@phemmer We have an issue for that on already: #2288. Input on what the better behavior would be is much appreciated!

@DouweM
Copy link
Collaborator

DouweM commented Oct 13, 2025

Meh, I guess that's kind ugly: if "resources" in server.capabilities(): ...

@fennb I don't mind it! @Kludex You?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants