From 53b067b4e7236c534f7a831f7933bfc689b95430 Mon Sep 17 00:00:00 2001 From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com> Date: Thu, 22 May 2025 09:08:16 -0400 Subject: [PATCH] Make error masking configurable --- docs/servers/resources.mdx | 21 ++++-- docs/servers/tools.mdx | 28 +++++--- src/fastmcp/client/client.py | 7 +- src/fastmcp/prompts/prompt.py | 5 +- src/fastmcp/prompts/prompt_manager.py | 28 ++++++-- src/fastmcp/resources/resource_manager.py | 36 +++++++++-- src/fastmcp/server/server.py | 79 +++++++++++++++-------- src/fastmcp/settings.py | 17 +++++ src/fastmcp/tools/tool_manager.py | 11 +++- tests/client/test_client.py | 61 ++++++++++++++++- tests/contrib/test_bulk_tool_caller.py | 4 +- tests/prompts/test_prompt_manager.py | 4 +- tests/resources/test_resource_manager.py | 73 +++++++++++---------- tests/tools/test_tool_manager.py | 44 +++++++++++-- 14 files changed, 317 insertions(+), 101 deletions(-) diff --git a/docs/servers/resources.mdx b/docs/servers/resources.mdx index 72d736d31..92838ebce 100644 --- a/docs/servers/resources.mdx +++ b/docs/servers/resources.mdx @@ -408,12 +408,20 @@ Templates provide a powerful way to expose parameterized data access points foll ## Error Handling - + If your resource function encounters an error, you can raise a standard Python exception (`ValueError`, `TypeError`, `FileNotFoundError`, custom exceptions, etc.) or a FastMCP `ResourceError`. -For security reasons, most exceptions are wrapped in a generic `ResourceError` before being sent to the client, with internal error details masked. However, if you raise a `ResourceError` directly, its contents **are** included in the response. This allows you to provide informative error messages to the client on an opt-in basis. +By default, all exceptions (including their details) are logged and converted into an MCP error response to be sent back to the client LLM. This helps the LLM understand failures and react appropriately. +If you want to mask internal error details for security reasons, you can: + +1. Use the `mask_error_details=True` parameter when creating your `FastMCP` instance: +```python +mcp = FastMCP(name="SecureServer", mask_error_details=True) +``` + +2. Or use `ResourceError` to explicitly control what error information is sent to clients: ```python from fastmcp import FastMCP from fastmcp.exceptions import ResourceError @@ -423,13 +431,14 @@ mcp = FastMCP(name="DataServer") @mcp.resource("resource://safe-error") def fail_with_details() -> str: """This resource provides detailed error information.""" - # ResourceError contents are sent back to clients + # ResourceError contents are always sent back to clients, + # regardless of mask_error_details setting raise ResourceError("Unable to retrieve data: file not found") @mcp.resource("resource://masked-error") def fail_with_masked_details() -> str: - """This resource masks internal error details.""" - # Other exceptions are converted to ResourceError with generic message + """This resource masks internal error details when mask_error_details=True.""" + # This message would be masked if mask_error_details=True raise ValueError("Sensitive internal file path: /etc/secrets.conf") @mcp.resource("data://{id}") @@ -442,7 +451,7 @@ def get_data_by_id(id: str) -> dict: return {"id": id, "value": "data"} ``` -This error handling pattern applies to both regular resources and resource templates. +When `mask_error_details=True`, only error messages from `ResourceError` will include details, other exceptions will be converted to a generic message. ## Server Behavior diff --git a/docs/servers/tools.mdx b/docs/servers/tools.mdx index 3a850d9cc..879ceab02 100644 --- a/docs/servers/tools.mdx +++ b/docs/servers/tools.mdx @@ -248,13 +248,21 @@ def do_nothing() -> None: ### Error Handling - + If your tool encounters an error, you can raise a standard Python exception (`ValueError`, `TypeError`, `FileNotFoundError`, custom exceptions, etc.) or a FastMCP `ToolError`. -In all cases, the exception is logged and converted into an MCP error response to be sent back to the client LLM. For security reasons, the error message is **not** included in the response by default. However, if you raise a `ToolError`, the contents of the exception **are** included in the response. This allows you to provide informative error messages to the client LLM on an opt-in basis, which can help the LLM understand failures and react appropriately. +By default, all exceptions (including their details) are logged and converted into an MCP error response to be sent back to the client LLM. This helps the LLM understand failures and react appropriately. -```python {2, 10, 14} +If you want to mask internal error details for security reasons, you can: + +1. Use the `mask_error_details=True` parameter when creating your `FastMCP` instance: +```python +mcp = FastMCP(name="SecureServer", mask_error_details=True) +``` + +2. Or use `ToolError` to explicitly control what error information is sent to clients: +```python from fastmcp import FastMCP from fastmcp.exceptions import ToolError @@ -262,16 +270,20 @@ from fastmcp.exceptions import ToolError def divide(a: float, b: float) -> float: """Divide a by b.""" - # Python exceptions raise errors but the contents are not sent to clients + if b == 0: + # Error messages from ToolError are always sent to clients, + # regardless of mask_error_details setting + raise ToolError("Division by zero is not allowed.") + + # If mask_error_details=True, this message would be masked if not isinstance(a, (int, float)) or not isinstance(b, (int, float)): raise TypeError("Both arguments must be numbers.") - - if b == 0: - # ToolError contents are sent back to clients - raise ToolError("Division by zero is not allowed.") + return a / b ``` +When `mask_error_details=True`, only error messages from `ToolError` will include details, other exceptions will be converted to a generic message. + ### Annotations diff --git a/src/fastmcp/client/client.py b/src/fastmcp/client/client.py index 19552ea35..c5446702c 100644 --- a/src/fastmcp/client/client.py +++ b/src/fastmcp/client/client.py @@ -322,7 +322,12 @@ class Client: RuntimeError: If called while the client is not connected. """ if isinstance(uri, str): - uri = AnyUrl(uri) # Ensure AnyUrl + try: + uri = AnyUrl(uri) # Ensure AnyUrl + except Exception as e: + raise ValueError( + f"Provided resource URI is invalid: {str(uri)!r}" + ) from e result = await self.read_resource_mcp(uri) return result.contents diff --git a/src/fastmcp/prompts/prompt.py b/src/fastmcp/prompts/prompt.py index 85eb3bc9d..26698f5d6 100644 --- a/src/fastmcp/prompts/prompt.py +++ b/src/fastmcp/prompts/prompt.py @@ -12,6 +12,7 @@ from mcp.types import Prompt as MCPPrompt from mcp.types import PromptArgument as MCPPromptArgument from pydantic import BaseModel, BeforeValidator, Field, TypeAdapter, validate_call +from fastmcp.exceptions import PromptError from fastmcp.server.dependencies import get_context from fastmcp.utilities.json_schema import compress_schema from fastmcp.utilities.logging import get_logger @@ -199,12 +200,12 @@ class Prompt(BaseModel): ) ) except Exception: - raise ValueError("Could not convert prompt result to message.") + raise PromptError("Could not convert prompt result to message.") return messages except Exception as e: logger.exception(f"Error rendering prompt {self.name}: {e}") - raise ValueError(f"Error rendering prompt {self.name}.") + raise PromptError(f"Error rendering prompt {self.name}.") def __eq__(self, other: object) -> bool: if not isinstance(other, Prompt): diff --git a/src/fastmcp/prompts/prompt_manager.py b/src/fastmcp/prompts/prompt_manager.py index 8102cd364..bfd96b573 100644 --- a/src/fastmcp/prompts/prompt_manager.py +++ b/src/fastmcp/prompts/prompt_manager.py @@ -7,7 +7,7 @@ from typing import TYPE_CHECKING, Any from mcp import GetPromptResult -from fastmcp.exceptions import NotFoundError +from fastmcp.exceptions import NotFoundError, PromptError from fastmcp.prompts.prompt import Prompt, PromptResult from fastmcp.settings import DuplicateBehavior from fastmcp.utilities.logging import get_logger @@ -21,8 +21,13 @@ logger = get_logger(__name__) class PromptManager: """Manages FastMCP prompts.""" - def __init__(self, duplicate_behavior: DuplicateBehavior | None = None): + def __init__( + self, + duplicate_behavior: DuplicateBehavior | None = None, + mask_error_details: bool = False, + ): self._prompts: dict[str, Prompt] = {} + self.mask_error_details = mask_error_details # Default to "warn" if None is provided if duplicate_behavior is None: @@ -85,9 +90,24 @@ class PromptManager: if not prompt: raise NotFoundError(f"Unknown prompt: {name}") - messages = await prompt.render(arguments) + try: + messages = await prompt.render(arguments) + return GetPromptResult(description=prompt.description, messages=messages) - return GetPromptResult(description=prompt.description, messages=messages) + # Pass through PromptErrors as-is + except PromptError as e: + logger.exception(f"Error rendering prompt {name!r}: {e}") + raise e + + # Handle other exceptions + except Exception as e: + logger.exception(f"Error rendering prompt {name!r}: {e}") + if self.mask_error_details: + # Mask internal details + raise PromptError(f"Error rendering prompt {name!r}") + else: + # Include original error details + raise PromptError(f"Error rendering prompt {name!r}: {e}") def has_prompt(self, key: str) -> bool: """Check if a prompt exists.""" diff --git a/src/fastmcp/resources/resource_manager.py b/src/fastmcp/resources/resource_manager.py index 9d8a20d8e..c3b74e5a4 100644 --- a/src/fastmcp/resources/resource_manager.py +++ b/src/fastmcp/resources/resource_manager.py @@ -22,9 +22,22 @@ logger = get_logger(__name__) class ResourceManager: """Manages FastMCP resources.""" - def __init__(self, duplicate_behavior: DuplicateBehavior | None = None): + def __init__( + self, + duplicate_behavior: DuplicateBehavior | None = None, + mask_error_details: bool = False, + ): + """Initialize the ResourceManager. + + Args: + duplicate_behavior: How to handle duplicate resources + (warn, error, replace, ignore) + mask_error_details: Whether to mask error details from exceptions + other than ResourceError + """ self._resources: dict[str, Resource] = {} self._templates: dict[str, ResourceTemplate] = {} + self.mask_error_details = mask_error_details # Default to "warn" if None is provided if duplicate_behavior is None: @@ -35,7 +48,6 @@ class ResourceManager: f"Invalid duplicate_behavior: {duplicate_behavior}. " f"Must be one of: {', '.join(DuplicateBehavior.__args__)}" ) - self.duplicate_behavior = duplicate_behavior def add_resource_or_template_from_fn( @@ -244,12 +256,21 @@ class ResourceManager: uri_str, params=params, ) + # Pass through ResourceErrors as-is except ResourceError as e: logger.error(f"Error creating resource from template: {e}") raise e + # Handle other exceptions except Exception as e: logger.error(f"Error creating resource from template: {e}") - raise ValueError(f"Error creating resource from template: {e}") + if self.mask_error_details: + # Mask internal details + raise ValueError("Error creating resource from template") from e + else: + # Include original error details + raise ValueError( + f"Error creating resource from template: {e}" + ) from e raise NotFoundError(f"Unknown resource: {uri_str}") @@ -265,10 +286,15 @@ class ResourceManager: logger.error(f"Error reading resource {uri!r}: {e}") raise e - # raise other exceptions as ResourceErrors without revealing internal details + # Handle other exceptions except Exception as e: logger.error(f"Error reading resource {uri!r}: {e}") - raise ResourceError(f"Error reading resource {uri!r}") from e + if self.mask_error_details: + # Mask internal details + raise ResourceError(f"Error reading resource {uri!r}") from e + else: + # Include original error details + raise ResourceError(f"Error reading resource {uri!r}: {e}") from e def get_resources(self) -> dict[str, Resource]: """Get all registered resources, keyed by URI.""" diff --git a/src/fastmcp/server/server.py b/src/fastmcp/server/server.py index c3f33fe81..50036ef74 100644 --- a/src/fastmcp/server/server.py +++ b/src/fastmcp/server/server.py @@ -125,6 +125,7 @@ class FastMCP(Generic[LifespanResultT]): on_duplicate_resources: DuplicateBehavior | None = None, on_duplicate_prompts: DuplicateBehavior | None = None, resource_prefix_format: Literal["protocol", "path"] | None = None, + mask_error_details: bool | None = None, **settings: Any, ): if settings: @@ -139,6 +140,10 @@ class FastMCP(Generic[LifespanResultT]): ) self.settings = fastmcp.settings.ServerSettings(**settings) + # If mask_error_details is provided, override the settings value + if mask_error_details is not None: + self.settings.mask_error_details = mask_error_details + self.resource_prefix_format: Literal["protocol", "path"] if resource_prefix_format is None: self.resource_prefix_format = ( @@ -157,11 +162,16 @@ class FastMCP(Generic[LifespanResultT]): self._tool_manager = ToolManager( duplicate_behavior=on_duplicate_tools, serializer=tool_serializer, + mask_error_details=self.settings.mask_error_details, ) self._resource_manager = ResourceManager( - duplicate_behavior=on_duplicate_resources + duplicate_behavior=on_duplicate_resources, + mask_error_details=self.settings.mask_error_details, + ) + self._prompt_manager = PromptManager( + duplicate_behavior=on_duplicate_prompts, + mask_error_details=self.settings.mask_error_details, ) - self._prompt_manager = PromptManager(duplicate_behavior=on_duplicate_prompts) if lifespan is None: self._has_lifespan = False @@ -377,21 +387,30 @@ class FastMCP(Generic[LifespanResultT]): async def _mcp_call_tool( self, key: str, arguments: dict[str, Any] ) -> list[TextContent | ImageContent | EmbeddedResource]: - """Call a tool by name with arguments.""" + """Handle MCP 'callTool' requests. + Args: + key: The name of the tool to call + arguments: Arguments to pass to the tool + + Returns: + List of MCP Content objects containing the tool results + """ + logger.debug("Call tool: %s with %s", key, arguments) + + # Create and use context for the entire call with fastmcp.server.context.Context(fastmcp=self): + # Get tool, checking first from our tools, then from the mounted servers if self._tool_manager.has_tool(key): - result = await self._tool_manager.call_tool(key, arguments) + return await self._tool_manager.call_tool(key, arguments) - else: - for server in self._mounted_servers.values(): - if server.match_tool(key): - new_key = server.strip_tool_prefix(key) - result = await server.server._mcp_call_tool(new_key, arguments) - break - else: - raise NotFoundError(f"Unknown tool: {key}") - return result + # Check mounted servers to see if they have the tool + for server in self._mounted_servers.values(): + if server.match_tool(key): + tool_key = server.strip_tool_prefix(key) + return await server.server._mcp_call_tool(tool_key, arguments) + + raise NotFoundError(f"Unknown tool: {key}") async def _mcp_read_resource(self, uri: AnyUrl | str) -> list[ReadResourceContents]: """ @@ -419,24 +438,30 @@ class FastMCP(Generic[LifespanResultT]): async def _mcp_get_prompt( self, name: str, arguments: dict[str, Any] | None = None ) -> GetPromptResult: - """ - Get a prompt by name with arguments, in the format expected by the low-level - MCP server. + """Handle MCP 'getPrompt' requests. + Args: + name: The name of the prompt to render + arguments: Arguments to pass to the prompt + + Returns: + GetPromptResult containing the rendered prompt messages """ + logger.debug("Get prompt: %s with %s", name, arguments) + + # Create and use context for the entire call with fastmcp.server.context.Context(fastmcp=self): + # Get prompt, checking first from our prompts, then from the mounted servers if self._prompt_manager.has_prompt(name): - prompt_result = await self._prompt_manager.render_prompt( - name, arguments=arguments or {} - ) - return prompt_result - else: - for server in self._mounted_servers.values(): - if server.match_prompt(name): - new_key = server.strip_prompt_prefix(name) - return await server.server._mcp_get_prompt(new_key, arguments) - else: - raise NotFoundError(f"Unknown prompt: {name}") + return await self._prompt_manager.render_prompt(name, arguments) + + # Check mounted servers to see if they have the prompt + for server in self._mounted_servers.values(): + if server.match_prompt(name): + prompt_name = server.strip_prompt_prefix(name) + return await server.server._mcp_get_prompt(prompt_name, arguments) + + raise NotFoundError(f"Unknown prompt: {name}") def add_tool( self, diff --git a/src/fastmcp/settings.py b/src/fastmcp/settings.py index a5c78b3b6..c1dbf5447 100644 --- a/src/fastmcp/settings.py +++ b/src/fastmcp/settings.py @@ -124,6 +124,23 @@ class ServerSettings(BaseSettings): # prompt settings on_duplicate_prompts: DuplicateBehavior = "warn" + # error handling + mask_error_details: Annotated[ + bool, + Field( + default=False, + description=inspect.cleandoc( + """ + If True, error details from user-supplied functions (tool, resource, prompt) + will be masked before being sent to clients. Only error messages from explicitly + raised ToolError, ResourceError, or PromptError will be included in responses. + If False (default), all error details will be included in responses, but prefixed + with appropriate context. + """ + ), + ), + ] = False + dependencies: Annotated[ list[str], Field( diff --git a/src/fastmcp/tools/tool_manager.py b/src/fastmcp/tools/tool_manager.py index 848f07139..c511b18fa 100644 --- a/src/fastmcp/tools/tool_manager.py +++ b/src/fastmcp/tools/tool_manager.py @@ -23,9 +23,11 @@ class ToolManager: self, duplicate_behavior: DuplicateBehavior | None = None, serializer: Callable[[Any], str] | None = None, + mask_error_details: bool = False, ): self._tools: dict[str, Tool] = {} self._serializer = serializer + self.mask_error_details = mask_error_details # Default to "warn" if None is provided if duplicate_behavior is None: @@ -124,7 +126,12 @@ class ToolManager: logger.exception(f"Error calling tool {key!r}: {e}") raise e - # raise other exceptions as ToolErrors without revealing internal details + # Handle other exceptions except Exception as e: logger.exception(f"Error calling tool {key!r}: {e}") - raise ToolError(f"Error calling tool {key!r}") from e + if self.mask_error_details: + # Mask internal details + raise ToolError(f"Error calling tool {key!r}") from e + else: + # Include original error details + raise ToolError(f"Error calling tool {key!r}: {e}") from e diff --git a/tests/client/test_client.py b/tests/client/test_client.py index 62bddf0ab..c4713c326 100644 --- a/tests/client/test_client.py +++ b/tests/client/test_client.py @@ -219,6 +219,13 @@ async def test_get_prompt_mcp(fastmcp_server): assert result.description == "Example greeting prompt." +async def test_read_resource_invalid_uri(fastmcp_server): + """Test reading a resource with an invalid URI.""" + client = Client(transport=FastMCPTransport(fastmcp_server)) + with pytest.raises(ValueError, match="Provided resource URI is invalid"): + await client.read_resource("invalid_uri") + + async def test_read_resource(fastmcp_server): """Test reading a resource with InMemoryClient.""" client = Client(transport=FastMCPTransport(fastmcp_server)) @@ -457,7 +464,7 @@ async def test_tagged_template_functionality(tagged_resources_server): class TestErrorHandling: - async def test_general_tool_exceptions_are_masked(self): + async def test_general_tool_exceptions_are_not_masked_by_default(self): mcp = FastMCP("TestServer") @mcp.tool() @@ -466,6 +473,22 @@ class TestErrorHandling: client = Client(transport=FastMCPTransport(mcp)) + async with client: + result = await client.call_tool_mcp("error_tool", {}) + assert result.isError + assert isinstance(result.content[0], TextContent) + assert "test error" in result.content[0].text + assert "abc" in result.content[0].text + + async def test_general_tool_exceptions_are_masked_when_enabled(self): + mcp = FastMCP("TestServer", mask_error_details=True) + + @mcp.tool() + def error_tool(): + raise ValueError("This is a test error (abc)") + + client = Client(transport=FastMCPTransport(mcp)) + async with client: result = await client.call_tool_mcp("error_tool", {}) assert result.isError @@ -489,7 +512,7 @@ class TestErrorHandling: assert "test error" in result.content[0].text assert "abc" in result.content[0].text - async def test_general_resource_exceptions_are_masked(self): + async def test_general_resource_exceptions_are_not_masked_by_default(self): mcp = FastMCP("TestServer") @mcp.resource(uri="exception://resource") @@ -498,6 +521,22 @@ class TestErrorHandling: client = Client(transport=FastMCPTransport(mcp)) + async with client: + with pytest.raises(Exception) as excinfo: + await client.read_resource(AnyUrl("exception://resource")) + assert "Error reading resource" in str(excinfo.value) + assert "sensitive" in str(excinfo.value) + assert "internal error" in str(excinfo.value) + + async def test_general_resource_exceptions_are_masked_when_enabled(self): + mcp = FastMCP("TestServer", mask_error_details=True) + + @mcp.resource(uri="exception://resource") + async def exception_resource(): + raise ValueError("This is an internal error (sensitive)") + + client = Client(transport=FastMCPTransport(mcp)) + async with client: with pytest.raises(Exception) as excinfo: await client.read_resource(AnyUrl("exception://resource")) @@ -519,7 +558,7 @@ class TestErrorHandling: await client.read_resource(AnyUrl("error://resource")) assert "This is a resource error (xyz)" in str(excinfo.value) - async def test_general_template_exceptions_are_masked(self): + async def test_general_template_exceptions_are_not_masked_by_default(self): mcp = FastMCP("TestServer") @mcp.resource(uri="exception://resource/{id}") @@ -528,6 +567,22 @@ class TestErrorHandling: client = Client(transport=FastMCPTransport(mcp)) + async with client: + with pytest.raises(Exception) as excinfo: + await client.read_resource(AnyUrl("exception://resource/123")) + assert "Error reading resource" in str(excinfo.value) + assert "sensitive" in str(excinfo.value) + assert "internal error" in str(excinfo.value) + + async def test_general_template_exceptions_are_masked_when_enabled(self): + mcp = FastMCP("TestServer", mask_error_details=True) + + @mcp.resource(uri="exception://resource/{id}") + async def exception_resource(id: str): + raise ValueError("This is an internal error (sensitive)") + + client = Client(transport=FastMCPTransport(mcp)) + async with client: with pytest.raises(Exception) as excinfo: await client.read_resource(AnyUrl("exception://resource/123")) diff --git a/tests/contrib/test_bulk_tool_caller.py b/tests/contrib/test_bulk_tool_caller.py index b6f86c927..9b74d87bb 100644 --- a/tests/contrib/test_bulk_tool_caller.py +++ b/tests/contrib/test_bulk_tool_caller.py @@ -27,7 +27,9 @@ async def error_tool(arg1: str) -> dict[str, Any]: def error_tool_result_factory(arg1: str) -> CallToolRequestResult: """Generates the expected error result for error_tool.""" # Mimic the error message format generated by BulkToolCaller when catching ToolException - formatted_error_text = "Error calling tool 'error_tool'" + formatted_error_text = ( + "Error calling tool 'error_tool': Error in tool with arg1: " + arg1 + ) return CallToolRequestResult( isError=True, content=[TextContent(text=formatted_error_text, type="text")], diff --git a/tests/prompts/test_prompt_manager.py b/tests/prompts/test_prompt_manager.py index c887fdca5..d44dde0bf 100644 --- a/tests/prompts/test_prompt_manager.py +++ b/tests/prompts/test_prompt_manager.py @@ -3,7 +3,7 @@ from typing import Annotated import pytest from fastmcp import Context -from fastmcp.exceptions import NotFoundError +from fastmcp.exceptions import NotFoundError, PromptError from fastmcp.prompts import Prompt from fastmcp.prompts.prompt import PromptMessage, TextContent from fastmcp.prompts.prompt_manager import PromptManager @@ -192,7 +192,7 @@ class TestPromptManager: manager = PromptManager() prompt = Prompt.from_function(fn) manager.add_prompt(prompt) - with pytest.raises(ValueError, match="Missing required arguments"): + with pytest.raises(PromptError, match="Missing required arguments"): await manager.render_prompt("fn") async def test_prompt_with_varargs_not_allowed(self): diff --git a/tests/resources/test_resource_manager.py b/tests/resources/test_resource_manager.py index 006911c47..ad3dba908 100644 --- a/tests/resources/test_resource_manager.py +++ b/tests/resources/test_resource_manager.py @@ -563,28 +563,6 @@ class TestResourceErrorHandling: with pytest.raises(ResourceError, match="Specific resource error"): await manager.read_resource("error://resource") - async def test_exception_converted_to_resource_error(self): - """Test that other exceptions are converted to ResourceError.""" - manager = ResourceManager() - - async def buggy_resource(): - """Resource that raises a ValueError.""" - raise ValueError("Internal error details") - - resource = FunctionResource( - uri=AnyUrl("buggy://resource"), - name="buggy_resource", - fn=buggy_resource, - ) - manager.add_resource(resource) - - with pytest.raises(ResourceError) as excinfo: - await manager.read_resource("buggy://resource") - - # Exception message should contain the resource URI but not the internal details - assert "Error reading resource 'buggy://resource'" in str(excinfo.value) - assert "Internal error details" not in str(excinfo.value) - async def test_template_resource_error_passthrough(self): """Test that ResourceErrors from template-generated resources are passed through.""" manager = ResourceManager() @@ -606,21 +584,46 @@ class TestResourceErrorHandling: # The original error message should be included in the ValueError assert "Template error with param test" in str(excinfo.value) - async def test_template_exception_converted_to_resource_error(self): - """Test that other exceptions from template-generated resources are converted.""" + async def test_exception_converted_to_resource_error_with_details(self): + """Test that other exceptions are converted to ResourceError with details by default.""" manager = ResourceManager() - def buggy_template(param: str): - """Template that raises a ValueError.""" - raise ValueError(f"Internal template error with {param}") + async def buggy_resource(): + """Resource that raises a ValueError.""" + raise ValueError("Internal error details") - template = ResourceTemplate.from_function( - fn=buggy_template, - uri_template="buggy://{param}", - name="buggy_template", + resource = FunctionResource( + uri=AnyUrl("buggy://resource"), + name="buggy_resource", + fn=buggy_resource, ) - manager.add_template(template) + manager.add_resource(resource) - # First, the template creation will fail with ValueError - with pytest.raises(ResourceError, match="Error reading resource"): - await manager.read_resource("buggy://test") + with pytest.raises(ResourceError) as excinfo: + await manager.read_resource("buggy://resource") + + # The error message should include the original exception details + assert "Error reading resource 'buggy://resource'" in str(excinfo.value) + assert "Internal error details" in str(excinfo.value) + + async def test_exception_converted_to_masked_resource_error(self): + """Test that other exceptions are masked when enabled.""" + manager = ResourceManager(mask_error_details=True) + + async def buggy_resource(): + """Resource that raises a ValueError.""" + raise ValueError("Internal error details") + + resource = FunctionResource( + uri=AnyUrl("buggy://resource"), + name="buggy_resource", + fn=buggy_resource, + ) + manager.add_resource(resource) + + with pytest.raises(ResourceError) as excinfo: + await manager.read_resource("buggy://resource") + + # The error message should not include the original exception details + assert "Error reading resource 'buggy://resource'" in str(excinfo.value) + assert "Internal error details" not in str(excinfo.value) diff --git a/tests/tools/test_tool_manager.py b/tests/tools/test_tool_manager.py index 7b82e26b1..8a4d6c556 100644 --- a/tests/tools/test_tool_manager.py +++ b/tests/tools/test_tool_manager.py @@ -778,8 +778,8 @@ class TestToolErrorHandling: with pytest.raises(ToolError, match="Specific tool error"): await manager.call_tool("error_tool", {"x": 42}) - async def test_exception_converted_to_tool_error(self): - """Test that other exceptions are converted to ToolError.""" + async def test_exception_converted_to_tool_error_with_details(self): + """Test that other exceptions include details by default.""" manager = ToolManager() def buggy_tool(x: int) -> int: @@ -791,7 +791,24 @@ class TestToolErrorHandling: with pytest.raises(ToolError) as excinfo: await manager.call_tool("buggy_tool", {"x": 42}) - # Exception message should contain the tool name but not the internal details + # Exception message should include the tool name and the internal details + assert "Error calling tool 'buggy_tool'" in str(excinfo.value) + assert "Internal error details" in str(excinfo.value) + + async def test_exception_converted_to_masked_tool_error(self): + """Test that other exceptions are masked when enabled.""" + manager = ToolManager(mask_error_details=True) + + def buggy_tool(x: int) -> int: + """Tool that raises a ValueError.""" + raise ValueError("Internal error details") + + manager.add_tool_from_fn(buggy_tool) + + with pytest.raises(ToolError) as excinfo: + await manager.call_tool("buggy_tool", {"x": 42}) + + # Exception message should only contain the tool name, not the internal details assert "Error calling tool 'buggy_tool'" in str(excinfo.value) assert "Internal error details" not in str(excinfo.value) @@ -808,8 +825,8 @@ class TestToolErrorHandling: with pytest.raises(ToolError, match="Async tool error"): await manager.call_tool("async_error_tool", {"x": 42}) - async def test_async_exception_converted_to_tool_error(self): - """Test that other exceptions from async tools are converted to ToolError.""" + async def test_async_exception_converted_to_tool_error_with_details(self): + """Test that other exceptions from async tools include details by default.""" manager = ToolManager() async def async_buggy_tool(x: int) -> int: @@ -818,6 +835,23 @@ class TestToolErrorHandling: manager.add_tool_from_fn(async_buggy_tool) + with pytest.raises(ToolError) as excinfo: + await manager.call_tool("async_buggy_tool", {"x": 42}) + + # Exception message should include the tool name and the internal details + assert "Error calling tool 'async_buggy_tool'" in str(excinfo.value) + assert "Internal async error details" in str(excinfo.value) + + async def test_async_exception_converted_to_masked_tool_error(self): + """Test that other exceptions from async tools are masked when enabled.""" + manager = ToolManager(mask_error_details=True) + + async def async_buggy_tool(x: int) -> int: + """Async tool that raises a ValueError.""" + raise ValueError("Internal async error details") + + manager.add_tool_from_fn(async_buggy_tool) + with pytest.raises(ToolError) as excinfo: await manager.call_tool("async_buggy_tool", {"x": 42})