Skip to content

Commit a648537

Browse files
Varun SharmaCopilot
andcommitted
fix: address review comments - remove duplicate logging and add defensive handler test
- Remove logger.exception from base.py Tool.run() to avoid duplicate logging (server.py's defensive handler already logs) - Remove unused logging import from base.py - Remove '# pragma: no cover' from server.py defensive exception handler - Add test_handle_call_tool_defensive_exception_handler to verify the defensive handler returns a generic error and does not leak internal exception details Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5a93a4e commit a648537

3 files changed

Lines changed: 21 additions & 5 deletions

File tree

src/mcp/server/mcpserver/server.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ async def _handle_call_tool(
306306
raise
307307
except ToolError as e:
308308
return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True)
309-
except Exception: # pragma: no cover
309+
except Exception:
310310
logger.exception(f"Error calling tool {params.name}")
311311
return CallToolResult(
312312
content=[TextContent(type="text", text=f"An unexpected error occurred calling tool {params.name}")],

src/mcp/server/mcpserver/tools/base.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import functools
44
import inspect
5-
import logging
65
from collections.abc import Callable
76
from functools import cached_property
87
from typing import TYPE_CHECKING, Any
@@ -16,8 +15,6 @@
1615
from mcp.shared.tool_name_validation import validate_and_warn_tool_name
1716
from mcp.types import Icon, ToolAnnotations
1817

19-
logger = logging.getLogger(__name__)
20-
2118
if TYPE_CHECKING:
2219
from mcp.server.context import LifespanContextT, RequestT
2320
from mcp.server.mcpserver.server import Context
@@ -118,7 +115,6 @@ async def run(
118115
except ToolError:
119116
raise
120117
except Exception as e:
121-
logger.exception(f"Error executing tool {self.name}")
122118
raise ToolError(f"An unexpected error occurred executing tool {self.name}") from e
123119

124120

tests/server/mcpserver/test_server.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,26 @@ async def test_tool_error_passthrough(self):
302302
assert isinstance(content, TextContent)
303303
assert "Intentional tool error" in content.text
304304

305+
async def test_handle_call_tool_defensive_exception_handler(self):
306+
"""Test that _handle_call_tool returns generic error when call_tool raises unexpected Exception."""
307+
mcp = MCPServer()
308+
mcp.add_tool(tool_fn)
309+
310+
original_call_tool = mcp.call_tool
311+
312+
async def patched_call_tool(name: str, arguments: dict[str, Any]) -> Any:
313+
raise RuntimeError("internal db connection string leaked")
314+
315+
async with Client(mcp) as client:
316+
mcp.call_tool = patched_call_tool # type: ignore[assignment]
317+
result = await client.call_tool("tool_fn", {"x": 1, "y": 2})
318+
mcp.call_tool = original_call_tool # type: ignore[assignment]
319+
assert result.is_error is True
320+
content = result.content[0]
321+
assert isinstance(content, TextContent)
322+
assert "unexpected error" in content.text.lower()
323+
assert "internal db connection string leaked" not in content.text
324+
305325
async def test_tool_return_value_conversion(self):
306326
mcp = MCPServer()
307327
mcp.add_tool(tool_fn)

0 commit comments

Comments
 (0)