fix(mcp): contain malformed tool results
This commit is contained in:
@@ -418,7 +418,9 @@ class MCPToolWrapper(_MCPWrapperBase):
|
|||||||
logger.warning(
|
logger.warning(
|
||||||
"MCP tool '{}' timed out after {}s", self._name, self._tool_timeout
|
"MCP tool '{}' timed out after {}s", self._name, self._tool_timeout
|
||||||
)
|
)
|
||||||
return f"(MCP tool call timed out after {self._tool_timeout}s)"
|
return ToolResult.error(
|
||||||
|
f"(MCP tool call timed out after {self._tool_timeout}s)"
|
||||||
|
)
|
||||||
except asyncio.CancelledError:
|
except asyncio.CancelledError:
|
||||||
# MCP SDK's anyio cancel scopes can leak CancelledError on timeout/failure.
|
# MCP SDK's anyio cancel scopes can leak CancelledError on timeout/failure.
|
||||||
# Re-raise only if our task was externally cancelled (e.g. /stop).
|
# Re-raise only if our task was externally cancelled (e.g. /stop).
|
||||||
@@ -426,7 +428,7 @@ class MCPToolWrapper(_MCPWrapperBase):
|
|||||||
if task is not None and task.cancelling() > 0:
|
if task is not None and task.cancelling() > 0:
|
||||||
raise
|
raise
|
||||||
logger.warning("MCP tool '{}' was cancelled by server/SDK", self._name)
|
logger.warning("MCP tool '{}' was cancelled by server/SDK", self._name)
|
||||||
return "(MCP tool call was cancelled)"
|
return ToolResult.error("(MCP tool call was cancelled)")
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
if await self._refresh_session_after_termination(
|
if await self._refresh_session_after_termination(
|
||||||
exc,
|
exc,
|
||||||
@@ -451,20 +453,35 @@ class MCPToolWrapper(_MCPWrapperBase):
|
|||||||
self._name,
|
self._name,
|
||||||
type(exc).__name__,
|
type(exc).__name__,
|
||||||
)
|
)
|
||||||
return f"(MCP tool call failed after retry: {type(exc).__name__})"
|
return ToolResult.error(
|
||||||
|
f"(MCP tool call failed after retry: {type(exc).__name__})"
|
||||||
|
)
|
||||||
logger.exception(
|
logger.exception(
|
||||||
"MCP tool '{}' failed: {}: {}",
|
"MCP tool '{}' failed: {}: {}",
|
||||||
self._name,
|
self._name,
|
||||||
type(exc).__name__,
|
type(exc).__name__,
|
||||||
exc,
|
exc,
|
||||||
)
|
)
|
||||||
return f"(MCP tool call failed: {type(exc).__name__})"
|
return ToolResult.error(
|
||||||
|
f"(MCP tool call failed: {type(exc).__name__})"
|
||||||
|
)
|
||||||
else:
|
else:
|
||||||
# Success — extract text and persist any image content as artifacts.
|
# Success — extract text and persist any image content as artifacts.
|
||||||
rendered = self._render_call_result(result.content, kwargs)
|
try:
|
||||||
if getattr(result, "isError", False):
|
rendered = self._render_call_result(result.content, kwargs)
|
||||||
return ToolResult.error(rendered)
|
if getattr(result, "isError", False):
|
||||||
return rendered
|
return ToolResult.error(rendered)
|
||||||
|
return rendered
|
||||||
|
except Exception as exc:
|
||||||
|
logger.exception(
|
||||||
|
"MCP tool '{}' failed while rendering result: {}: {}",
|
||||||
|
self._name,
|
||||||
|
type(exc).__name__,
|
||||||
|
exc,
|
||||||
|
)
|
||||||
|
return ToolResult.error(
|
||||||
|
f"(MCP tool returned malformed content: {type(exc).__name__})"
|
||||||
|
)
|
||||||
|
|
||||||
return "(MCP tool call failed)" # Unreachable, but satisfies type checkers
|
return "(MCP tool call failed)" # Unreachable, but satisfies type checkers
|
||||||
|
|
||||||
|
|||||||
@@ -320,6 +320,35 @@ async def test_execute_wraps_mcp_is_error_result() -> None:
|
|||||||
assert is_tool_error_result(wrapper.name, result)
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_execute_contains_malformed_success_result() -> None:
|
||||||
|
async def call_tool(_name: str, arguments: dict) -> object:
|
||||||
|
return SimpleNamespace(content=None)
|
||||||
|
|
||||||
|
wrapper = _make_wrapper(SimpleNamespace(call_tool=call_tool))
|
||||||
|
|
||||||
|
result = await wrapper.execute()
|
||||||
|
|
||||||
|
assert result == "(MCP tool returned malformed content: TypeError)"
|
||||||
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_registry_adds_retry_hint_to_malformed_mcp_result() -> None:
|
||||||
|
async def call_tool(_name: str, arguments: dict) -> object:
|
||||||
|
return SimpleNamespace(content=None)
|
||||||
|
|
||||||
|
wrapper = _make_wrapper(SimpleNamespace(call_tool=call_tool))
|
||||||
|
registry = ToolRegistry()
|
||||||
|
registry.register(wrapper)
|
||||||
|
|
||||||
|
result = await registry.execute(wrapper.name, {})
|
||||||
|
|
||||||
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
assert "MCP tool returned malformed content" in result
|
||||||
|
assert "Analyze the error above and try a different approach" in result
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_execute_preserves_success_text_that_starts_with_error() -> None:
|
async def test_execute_preserves_success_text_that_starts_with_error() -> None:
|
||||||
async def call_tool(_name: str, arguments: dict) -> object:
|
async def call_tool(_name: str, arguments: dict) -> object:
|
||||||
@@ -401,6 +430,7 @@ async def test_execute_returns_timeout_message() -> None:
|
|||||||
result = await wrapper.execute()
|
result = await wrapper.execute()
|
||||||
|
|
||||||
assert result == "(MCP tool call timed out after 0.01s)"
|
assert result == "(MCP tool call timed out after 0.01s)"
|
||||||
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
@@ -413,6 +443,7 @@ async def test_execute_handles_server_cancelled_error() -> None:
|
|||||||
result = await wrapper.execute()
|
result = await wrapper.execute()
|
||||||
|
|
||||||
assert result == "(MCP tool call was cancelled)"
|
assert result == "(MCP tool call was cancelled)"
|
||||||
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
@@ -444,6 +475,7 @@ async def test_execute_handles_generic_exception() -> None:
|
|||||||
result = await wrapper.execute()
|
result = await wrapper.execute()
|
||||||
|
|
||||||
assert result == "(MCP tool call failed: RuntimeError)"
|
assert result == "(MCP tool call failed: RuntimeError)"
|
||||||
|
assert is_tool_error_result(wrapper.name, result)
|
||||||
|
|
||||||
|
|
||||||
def _make_tool_def(name: str) -> SimpleNamespace:
|
def _make_tool_def(name: str) -> SimpleNamespace:
|
||||||
|
|||||||
Reference in New Issue
Block a user