Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,10 @@ async def my_tool(x: int, ctx: Context) -> str:

The internal layers (`ToolManager.call_tool`, `Tool.run`, `Prompt.render`, `ResourceTemplate.create_resource`, etc.) now require `context` as a positional argument.

### `ResourceManager.get_resource()` and `ResourceTemplate.create_resource()` raise typed exceptions

`ResourceManager.get_resource()` now raises `ResourceNotFoundError` (instead of `ValueError`) when no resource or template matches the URI. `ResourceTemplate.create_resource()` now raises `ResourceError` (instead of `ValueError`) when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`). `MCPServer.read_resource()` continues to raise `ResourceError` and is unaffected.

### Replace `RootModel` by union types with `TypeAdapter` validation

The following union types are no longer `RootModel` subclasses:
Expand Down
9 changes: 9 additions & 0 deletions src/mcp/server/mcpserver/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ class ResourceError(MCPServerError):
"""Error in resource operations."""


class ResourceNotFoundError(ResourceError):
"""Resource does not exist.

Raise this from a resource template handler to signal that the requested
instance does not exist; clients receive ``-32602`` (invalid params) per
SEP-2164.
"""


class ToolError(MCPServerError):
"""Error in tool operations."""

Expand Down
8 changes: 3 additions & 5 deletions src/mcp/server/mcpserver/resources/resource_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from pydantic import AnyUrl

from mcp.server.mcpserver.exceptions import ResourceNotFoundError
from mcp.server.mcpserver.resources.base import Resource
from mcp.server.mcpserver.resources.templates import ResourceTemplate
from mcp.server.mcpserver.utilities.logging import get_logger
Expand Down Expand Up @@ -92,12 +93,9 @@ async def get_resource(self, uri: AnyUrl | str, context: Context[LifespanContext
# Then check templates
for template in self._templates.values():
if params := template.matches(uri_str):
try:
return await template.create_resource(uri_str, params, context=context)
except Exception as e: # pragma: no cover
raise ValueError(f"Error creating resource from template: {e}")
return await template.create_resource(uri_str, params, context=context)

raise ValueError(f"Unknown resource: {uri}")
raise ResourceNotFoundError(f"Unknown resource: {uri}")
Comment thread
claude[bot] marked this conversation as resolved.

def list_resources(self) -> list[Resource]:
"""List all registered resources."""
Expand Down
7 changes: 5 additions & 2 deletions src/mcp/server/mcpserver/resources/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

from pydantic import BaseModel, Field, validate_call

from mcp.server.mcpserver.exceptions import ResourceError
from mcp.server.mcpserver.resources.types import FunctionResource, Resource
from mcp.server.mcpserver.utilities.context_injection import find_context_parameter, inject_context
from mcp.server.mcpserver.utilities.func_metadata import func_metadata
Expand Down Expand Up @@ -104,7 +105,7 @@ async def create_resource(
"""Create a resource from the template with the given parameters.

Raises:
ValueError: If creating the resource fails.
ResourceError: If creating the resource fails.
"""
try:
# Add context to params if needed
Expand All @@ -126,5 +127,7 @@ async def create_resource(
meta=self.meta,
fn=lambda: result, # Capture result in closure
)
except ResourceError:
raise
except Exception as e:
raise ValueError(f"Error creating resource from template: {e}")
raise ResourceError(f"Error creating resource from template: {e}")
Comment thread
claude[bot] marked this conversation as resolved.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We keep leaking info when adding the exception in the message itself. We should drop it out.

This comment is not related to this PR.

16 changes: 10 additions & 6 deletions src/mcp/server/mcpserver/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from mcp.server.lowlevel.server import LifespanResultT, Server
from mcp.server.lowlevel.server import lifespan as default_lifespan
from mcp.server.mcpserver.context import Context
from mcp.server.mcpserver.exceptions import ResourceError
from mcp.server.mcpserver.exceptions import ResourceError, ResourceNotFoundError
from mcp.server.mcpserver.prompts import Prompt, PromptManager
from mcp.server.mcpserver.resources import FunctionResource, Resource, ResourceManager
from mcp.server.mcpserver.tools import Tool, ToolManager
Expand All @@ -44,6 +44,8 @@
from mcp.server.transport_security import TransportSecuritySettings
from mcp.shared.exceptions import MCPError
from mcp.types import (
INTERNAL_ERROR,
INVALID_PARAMS,
Annotations,
BlobResourceContents,
CallToolRequestParams,
Expand Down Expand Up @@ -332,7 +334,12 @@
self, ctx: ServerRequestContext[LifespanResultT], params: ReadResourceRequestParams
) -> ReadResourceResult:
context = Context(request_context=ctx, mcp_server=self)
results = await self.read_resource(params.uri, context)
try:
results = await self.read_resource(params.uri, context)
except ResourceNotFoundError as err:
raise MCPError(code=INVALID_PARAMS, message=str(err), data={"uri": str(params.uri)})

Check warning on line 340 in src/mcp/server/mcpserver/server.py

View check run for this annotation

Claude / Claude Code Review

Nested ctx.read_resource() not-found misattributed to outer URI in data field

Edge case: if a template handler composes via `await ctx.read_resource("dep://missing")` and that *inner* lookup raises `ResourceNotFoundError`, the new `except ResourceError: raise` pass-through (templates.py:130) plus the removed wrap around `get_resource` lets it reach `_handle_read_resource()` unchanged — which then attaches `data={"uri": str(params.uri)}`, i.e. the **outer** requested URI. The client gets the SEP-2164 `-32602` not-found signal with `data.uri` pointing at a resource that act
Comment on lines +339 to +340
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Edge case: if a template handler composes via await ctx.read_resource("dep://missing") and that inner lookup raises ResourceNotFoundError, the new except ResourceError: raise pass-through (templates.py:130) plus the removed wrap around get_resource lets it reach _handle_read_resource() unchanged — which then attaches data={"uri": str(params.uri)}, i.e. the outer requested URI. The client gets the SEP-2164 -32602 not-found signal with data.uri pointing at a resource that actually exists and matched. Niche (only affects template handlers calling ctx.read_resource()), but since data.uri is the spec-defined machine-readable field it might be worth deriving data.uri from the exception (or having handlers catch/re-wrap nested reads).

Extended reasoning...

What the bug is

When an async resource-template handler internally calls await ctx.read_resource(<inner-uri>) and that inner URI does not exist, the resulting ResourceNotFoundError("Unknown resource: <inner-uri>") now propagates unchanged all the way to _handle_read_resource(), which builds the SEP-2164 error envelope using data={"uri": str(params.uri)} — the outer URI the client originally requested. So the client is told (via the spec-defined machine-readable data.uri field) that the outer URI doesn't exist, when in fact the outer template matched fine and only an internal dependency is missing. The human-readable message text still names the inner URI, but data.uri does not.

Code path that triggers it

Context.read_resource() (context.py:117) is a thin wrapper that calls MCPServer.read_resource() directly with no exception handling. After this PR, MCPServer.read_resource() (server.py:446) no longer wraps get_resource() in a try/except, so an inner ResourceNotFoundError from ResourceManager.get_resource() (resource_manager.py:98) propagates straight out of the nested ctx.read_resource() call and into the handler body.

When that exception exits the handler's await result in ResourceTemplate.create_resource(), it hits the new except ResourceError: raise at templates.py:130 — which re-raises it unchanged (since ResourceNotFoundError <: ResourceError). It then propagates through the outer get_resource() (wrap removed) and outer read_resource() (wrap removed) to _handle_read_resource() at server.py:339, which catches ResourceNotFoundError and raises MCPError(code=INVALID_PARAMS, message=str(err), data={"uri": str(params.uri)}).

Why nothing prevents it

Before this PR, the inner failure was double-wrapped (templates.py except Exception → ValueError, then server.py except ValueError → ResourceError("Unknown resource: <outer>")) and surfaced as code 0 with no machine-actionable data.uri. The PR's three changes — removing the server.py wrap around get_resource, adding the except ResourceError: raise pass-through in templates.py, and attaching data={"uri": params.uri} — combine to create this leak. Each change is individually correct for its intended purpose (letting template handlers raise ResourceNotFoundError directly), but together they don't distinguish a ResourceNotFoundError raised by the handler about the requested instance from one that bubbled up from a nested read of a different URI.

Step-by-step proof

  1. Register @mcp.resource("composite://{id}") with an async handler: async def get(id: str, ctx: Context): base = await ctx.read_resource("dep://missing"); return ...
  2. Client sends resources/read with uri="composite://42".
  3. Outer get_resource("composite://42") matches the template; create_resource() calls the handler.
  4. Handler awaits ctx.read_resource("dep://missing")MCPServer.read_resource("dep://missing")get_resource("dep://missing") → raises ResourceNotFoundError("Unknown resource: dep://missing") (no wrap).
  5. Exception exits handler body → templates.py:130 except ResourceError: raise passes it through.
  6. Propagates through outer get_resource/read_resource (no wrap).
  7. _handle_read_resource() catches it → MCPError(code=-32602, message="Unknown resource: dep://missing", data={"uri": "composite://42"}).

Client receives {code: -32602, data: {uri: "composite://42"}} — per SEP-2164, the signal that composite://42 does not exist — when it actually does.

Impact and suggested fix

Niche: only affects async template handlers that compose resources via ctx.read_resource(), which is a supported but uncommon pattern (Context injection into templates is tested in test_resource_with_context). Handler authors can work around it by catching and re-wrapping the inner exception themselves. It's also debatable whether data.uri should echo back the requested URI (reasonable as a request-correlation field) vs. identify the actually-missing URI (the SEP-2164 reading). Options if you want to tighten this: derive data.uri from the caught exception (e.g. attach a .uri attribute to ResourceNotFoundError and prefer it over params.uri when present), or note in ResourceNotFoundError's docstring that handlers composing via ctx.read_resource() should catch and re-raise with the outer URI. Filed as a nit — not blocking.

except ResourceError as err:
raise MCPError(code=INTERNAL_ERROR, message=str(err), data={"uri": str(params.uri)})
Comment on lines +341 to +342
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Minor: template-function exception text now reaches the client (ResourceError(f"Error creating resource from template: {e}")message=str(err)), whereas the resource.read() path three lines below explicitly sanitizes per the inline comment "we should not leak the exception to the client". Both paths execute user-supplied resource functions, so it's worth deciding deliberately whether to align them — e.g. drop the {e} interpolation in templates.py:133 and chain via from e so the inner text is logged server-side only, or accept the leak and relax the static-resource path for consistency.

Extended reasoning...

What changed

Before this PR, a template-function failure was (accidentally) sanitized: templates.py raised ValueError("Error creating resource from template: <inner>"), which server.py:443-445's except ValueError: raise ResourceError(f"Unknown resource: {uri}") caught and replaced with a message that discarded the inner text. After this PR, templates.py:133 raises ResourceError(f"Error creating resource from template: {e}"), which propagates unchanged through resource_manager.get_resource() and MCPServer.read_resource() (the try/except around get_resource was removed), and _handle_read_resource() sends it verbatim via MCPError(code=INTERNAL_ERROR, message=str(err), ...).

Step-by-step trace

  1. @mcp.resource("db://{id}") def get(id): raise RuntimeError("connection string postgres://user:pw@host invalid")
  2. Client sends resources/read with uri="db://42".
  3. ResourceTemplate.create_resource() calls self.fn(id="42")RuntimeError(...) → caught by except Exception as e at templates.py:132 → re-raised as ResourceError("Error creating resource from template: connection string postgres://user:pw@host invalid").
  4. resource_manager.get_resource() (line 96) no longer wraps — propagates as-is.
  5. MCPServer.read_resource() (line 446) no longer has a try/except around get_resource — propagates as-is.
  6. _handle_read_resource() matches except ResourceError as errMCPError(code=INTERNAL_ERROR, message=str(err), data={"uri": ...}).
  7. Client receives {code: -32603, message: "Error creating resource from template: connection string postgres://user:pw@host invalid"}.

Previously step 5 would have caught ValueError and replaced the message with "Unknown resource: db://42", so the client never saw the inner text.

The inconsistency

Within the same read_resource() function, the resource.read() path (server.py:448-454) wraps user-function failures as ResourceError(f"Error reading resource {uri}") with an explicit comment: "If an exception happens when reading the resource, we should not leak the exception to the client." That path covers static FunctionResource instances, where the user's function executes inside resource.read(). For templated resources, the user's function executes inside create_resource() instead — and that path now reaches the client unsanitized. So whether a user's exception text leaks depends purely on whether their @mcp.resource URI has {params} in it, which seems unintentional.

Addressing the counter-argument

It's fair to note that (a) the previous sanitization was an accidental side-effect of the very bug this PR fixes, not a designed feature; (b) _handle_call_tool() already returns str(e) to clients for tool exceptions, so the framework doesn't uniformly hide user exception text; and (c) the new test_read_resource_template_error asserts the "Error creating resource from template" prefix reaches the client, suggesting some intent. Those are all valid — which is why this is a nit, not a blocker. But the test uses a substring match and doesn't assert whether ": backend unavailable" is appended, so it doesn't pin down the policy either way. And the resource.read() comment is in the same function being edited here, so the two paths diverging is at minimum worth a conscious decision rather than falling out of the refactor.

Suggested fix

If sanitization is preferred (matching the existing comment): in templates.py:133, raise ResourceError("Error creating resource from template") from e — the inner exception is still chained for server-side logging via logger.exception, but str(err) no longer includes it. Alternatively, if exposing user-handler exception text is the intended policy (as with tools), consider dropping the sanitization comment/wrapper on the resource.read() path so both behave the same.

Comment on lines +340 to +342
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we have something like MCPError.from something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

only for inbound JSONRPCError and ErrorData, not from an exception.

contents: list[TextResourceContents | BlobResourceContents] = []
for item in results:
if isinstance(item.content, bytes):
Expand Down Expand Up @@ -436,10 +443,7 @@
"""Read a resource by URI."""
if context is None:
context = Context(mcp_server=self)
try:
resource = await self._resource_manager.get_resource(uri, context)
except ValueError:
raise ResourceError(f"Unknown resource: {uri}")
resource = await self._resource_manager.get_resource(uri, context)

try:
content = await resource.read()
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down
3 changes: 2 additions & 1 deletion tests/server/mcpserver/resources/test_resource_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from pydantic import AnyUrl

from mcp.server.mcpserver import Context
from mcp.server.mcpserver.exceptions import ResourceNotFoundError
from mcp.server.mcpserver.resources import FileResource, FunctionResource, ResourceManager, ResourceTemplate


Expand Down Expand Up @@ -114,7 +115,7 @@ def greet(name: str) -> str:
async def test_get_unknown_resource(self):
"""Test getting a non-existent resource."""
manager = ResourceManager()
with pytest.raises(ValueError, match="Unknown resource"):
with pytest.raises(ResourceNotFoundError, match="Unknown resource"):
await manager.get_resource(AnyUrl("unknown://test"), Context())

def test_list_resources(self, temp_file: Path):
Expand Down
3 changes: 2 additions & 1 deletion tests/server/mcpserver/resources/test_resource_template.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from pydantic import BaseModel

from mcp.server.mcpserver import Context, MCPServer
from mcp.server.mcpserver.exceptions import ResourceError
from mcp.server.mcpserver.resources import FunctionResource, ResourceTemplate
from mcp.types import Annotations

Expand Down Expand Up @@ -86,7 +87,7 @@ def failing_func(x: str) -> str:
name="fail",
)

with pytest.raises(ValueError, match="Error creating resource from template"):
with pytest.raises(ResourceError, match="Error creating resource from template"):
await template.create_resource("fail://test", {"x": "test"}, Context())

@pytest.mark.anyio
Expand Down
41 changes: 38 additions & 3 deletions tests/server/mcpserver/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@
from mcp.server.context import ServerRequestContext
from mcp.server.experimental.request_context import Experimental
from mcp.server.mcpserver import Context, MCPServer
from mcp.server.mcpserver.exceptions import ToolError
from mcp.server.mcpserver.exceptions import ResourceNotFoundError, ToolError
from mcp.server.mcpserver.prompts.base import Message, UserMessage
from mcp.server.mcpserver.resources import FileResource, FunctionResource
from mcp.server.mcpserver.utilities.types import Audio, Image
from mcp.server.transport_security import TransportSecuritySettings
from mcp.shared.exceptions import MCPError
from mcp.types import (
INTERNAL_ERROR,
INVALID_PARAMS,
AudioContent,
BlobResourceContents,
Completion,
Expand Down Expand Up @@ -692,13 +694,16 @@ def get_text():
assert result.contents[0].text == "Hello, world!"

async def test_read_unknown_resource(self):
"""Test that reading an unknown resource raises MCPError."""
"""Test that reading an unknown resource returns -32602 with uri in data (SEP-2164)."""
mcp = MCPServer()

async with Client(mcp) as client:
with pytest.raises(MCPError, match="Unknown resource: unknown://missing"):
with pytest.raises(MCPError, match="Unknown resource: unknown://missing") as exc_info:
await client.read_resource("unknown://missing")

assert exc_info.value.error.code == INVALID_PARAMS
assert exc_info.value.error.data == {"uri": "unknown://missing"}

async def test_read_resource_error(self):
"""Test that resource read errors are properly wrapped in MCPError."""
mcp = MCPServer()
Expand All @@ -711,6 +716,36 @@ def failing_resource():
with pytest.raises(MCPError, match="Error reading resource resource://failing"):
await client.read_resource("resource://failing")

async def test_read_resource_template_error(self):
"""Template-creation failure must surface as INTERNAL_ERROR, not INVALID_PARAMS (not-found)."""
mcp = MCPServer()

@mcp.resource("resource://item/{item_id}")
def get_item(item_id: str) -> str:
raise RuntimeError("backend unavailable")

async with Client(mcp) as client:
with pytest.raises(MCPError, match="Error creating resource from template") as exc_info:
await client.read_resource("resource://item/42")

assert exc_info.value.error.code == INTERNAL_ERROR
assert exc_info.value.error.code != INVALID_PARAMS

async def test_read_resource_template_not_found(self):
"""A template handler raising ResourceNotFoundError must surface as INVALID_PARAMS per SEP-2164."""
mcp = MCPServer()

@mcp.resource("resource://users/{user_id}")
def get_user(user_id: str) -> str:
raise ResourceNotFoundError(f"no user {user_id}")

async with Client(mcp) as client:
with pytest.raises(MCPError, match="no user 999") as exc_info:
await client.read_resource("resource://users/999")

assert exc_info.value.error.code == INVALID_PARAMS
assert exc_info.value.error.data == {"uri": "resource://users/999"}

async def test_binary_resource(self):
mcp = MCPServer()

Expand Down
Loading