Skip to content

Commit afc5a74

Browse files
author
HaoLiangXu
committed
fix: preserve CallToolResult.isError flag in MCPToolResult
Addresses review feedback from strands-agents#1943: 1. **Clean split**: Only isError-related changes, no unrelated commits from AgentResult.text (PR strands-agents#1942) or other work. 2. **isError propagation**: MCPToolResult now exposes isError field: - True when MCP server reports CallToolResult.isError (application error) - True when a Python exception occurs during tool execution (protocol error) - False/absent on success Updated docstring clarifies both error sources. 3. **Exception propagation** (maintainer feedback from strands-agents#1670): MCPAgentTool.stream() now passes the original exception via ToolResultEvent(result, exception=e), matching the pattern used by decorated tools. Added _invoke_tool() helper that calls the MCP client internals directly to capture exceptions. Files changed: - mcp_types.py: Added isError: NotRequired[bool], updated docstring - mcp_client.py: Propagated isError in _handle_tool_result and _handle_tool_execution_error - mcp_agent_tool.py: Added _invoke_tool() for exception capture, stream() yields ToolResultEvent(result, exception=...) - Tests: Added isError assertions, exception propagation tests
1 parent ae28397 commit afc5a74

File tree

5 files changed

+137
-28
lines changed

5 files changed

+137
-28
lines changed

src/strands/tools/mcp/mcp_agent_tool.py

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
It allows MCP tools to be seamlessly integrated and used within the agent ecosystem.
66
"""
77

8+
import asyncio
89
import logging
910
from datetime import timedelta
1011
from typing import TYPE_CHECKING, Any
@@ -110,10 +111,22 @@ async def stream(self, tool_use: ToolUse, invocation_state: dict[str, Any], **kw
110111
"""
111112
logger.debug("tool_name=<%s>, tool_use_id=<%s> | streaming", self.tool_name, tool_use["toolUseId"])
112113

113-
result = await self.mcp_client.call_tool_async(
114-
tool_use_id=tool_use["toolUseId"],
115-
name=self.mcp_tool.name, # Use original MCP name for server communication
116-
arguments=tool_use["input"],
117-
read_timeout_seconds=self.timeout,
118-
)
119-
yield ToolResultEvent(result)
114+
result, exception = await self._invoke_tool(tool_use)
115+
yield ToolResultEvent(result, exception=exception)
116+
117+
async def _invoke_tool(self, tool_use: ToolUse) -> tuple[Any, Exception | None]:
118+
"""Invoke the MCP tool and return (result, exception).
119+
120+
Returns both the MCPToolResult and the original exception (if any),
121+
so callers can access the exception via ToolResultEvent.exception —
122+
matching the pattern used by decorated tools.
123+
"""
124+
try:
125+
coro = self.mcp_client._create_call_tool_coroutine(
126+
self.mcp_tool.name, tool_use["input"], self.timeout
127+
)
128+
future = self.mcp_client._invoke_on_background_thread(coro)
129+
call_tool_result = await asyncio.wrap_future(future)
130+
return self.mcp_client._handle_tool_result(tool_use["toolUseId"], call_tool_result), None
131+
except Exception as e:
132+
return self.mcp_client._handle_tool_execution_error(tool_use["toolUseId"], e), e

src/strands/tools/mcp/mcp_client.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,7 @@ def _handle_tool_execution_error(self, tool_use_id: str, exception: Exception) -
673673
status="error",
674674
toolUseId=tool_use_id,
675675
content=[{"text": f"Tool execution failed: {str(exception)}"}],
676+
isError=True,
676677
)
677678

678679
def _handle_tool_result(self, tool_use_id: str, call_tool_result: MCPCallToolResult) -> MCPToolResult:
@@ -703,6 +704,7 @@ def _handle_tool_result(self, tool_use_id: str, call_tool_result: MCPCallToolRes
703704
status=status,
704705
toolUseId=tool_use_id,
705706
content=mapped_contents,
707+
isError=call_tool_result.isError,
706708
)
707709

708710
if call_tool_result.structuredContent:

src/strands/tools/mcp/mcp_types.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,10 @@ class MCPToolResult(ToolResult):
5555
that provides structured results beyond the standard text/image/document content.
5656
5757
Attributes:
58+
isError: Flag indicating whether this result represents an error.
59+
Set to True when the MCP tool reports a failure via CallToolResult.isError
60+
(application-level error) or when a Python exception occurs during tool
61+
execution (protocol/client error). Set to False or omitted on success.
5862
structuredContent: Optional JSON object containing structured data returned
5963
by the MCP tool. This allows MCP tools to return complex data structures
6064
that can be processed programmatically by agents or other tools.
@@ -63,5 +67,6 @@ class MCPToolResult(ToolResult):
6367
performance metrics, or business-specific tracking information).
6468
"""
6569

70+
isError: NotRequired[bool]
6671
structuredContent: NotRequired[dict[str, Any]]
6772
metadata: NotRequired[dict[str, Any]]

tests/strands/tools/mcp/test_mcp_agent_tool.py

Lines changed: 106 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
from datetime import timedelta
2-
from unittest.mock import MagicMock
2+
from unittest.mock import MagicMock, patch
33

44
import pytest
55
from mcp.types import Tool as MCPTool
@@ -8,16 +8,6 @@
88
from strands.types._events import ToolResultEvent
99

1010

11-
@pytest.fixture
12-
def mock_mcp_tool():
13-
mock_tool = MagicMock(spec=MCPTool)
14-
mock_tool.name = "test_tool"
15-
mock_tool.description = "A test tool"
16-
mock_tool.inputSchema = {"type": "object", "properties": {}}
17-
mock_tool.outputSchema = None # MCP tools can have optional outputSchema
18-
return mock_tool
19-
20-
2111
@pytest.fixture
2212
def mock_mcp_client():
2313
mock_server = MagicMock(spec=MCPClient)
@@ -26,9 +16,32 @@ def mock_mcp_client():
2616
"toolUseId": "test-123",
2717
"content": [{"text": "Success result"}],
2818
}
19+
# Mock internal methods used by MCPAgentTool.stream()
20+
mock_server._create_call_tool_coroutine.return_value = MagicMock()
21+
mock_server._handle_tool_result.return_value = {
22+
"status": "success",
23+
"toolUseId": "test-123",
24+
"content": [{"text": "Success result"}],
25+
}
26+
mock_server._handle_tool_execution_error.return_value = {
27+
"status": "error",
28+
"toolUseId": "test-123",
29+
"content": [{"text": "error"}],
30+
"isError": True,
31+
}
2932
return mock_server
3033

3134

35+
@pytest.fixture
36+
def mock_mcp_tool():
37+
mock_tool = MagicMock(spec=MCPTool)
38+
mock_tool.name = "test_tool"
39+
mock_tool.description = "A test tool"
40+
mock_tool.inputSchema = {"type": "object", "properties": {}}
41+
mock_tool.outputSchema = None # MCP tools can have optional outputSchema
42+
return mock_tool
43+
44+
3245
@pytest.fixture
3346
def mcp_agent_tool(mock_mcp_tool, mock_mcp_client):
3447
return MCPAgentTool(mock_mcp_tool, mock_mcp_client)
@@ -84,13 +97,25 @@ def test_tool_spec_without_output_schema(mock_mcp_tool, mock_mcp_client):
8497
async def test_stream(mcp_agent_tool, mock_mcp_client, alist):
8598
tool_use = {"toolUseId": "test-123", "name": "test_tool", "input": {"param": "value"}}
8699

87-
tru_events = await alist(mcp_agent_tool.stream(tool_use, {}))
88-
exp_events = [ToolResultEvent(mock_mcp_client.call_tool_async.return_value)]
100+
mock_result = mock_mcp_client._handle_tool_result.return_value
101+
102+
with patch("asyncio.wrap_future") as mock_wrap_future:
103+
# Make wrap_future return a coroutine that resolves to the mock call_tool result
104+
async def mock_awaitable(_):
105+
return MagicMock() # call_tool_result (raw MCP response)
89106

90-
assert tru_events == exp_events
91-
mock_mcp_client.call_tool_async.assert_called_once_with(
92-
tool_use_id="test-123", name="test_tool", arguments={"param": "value"}, read_timeout_seconds=None
107+
mock_wrap_future.side_effect = mock_awaitable
108+
109+
tru_events = await alist(mcp_agent_tool.stream(tool_use, {}))
110+
111+
assert len(tru_events) == 1
112+
event = tru_events[0]
113+
assert event.exception is None
114+
assert event.tool_result == mock_result
115+
mock_mcp_client._create_call_tool_coroutine.assert_called_once_with(
116+
"test_tool", {"param": "value"}, None
93117
)
118+
mock_mcp_client._handle_tool_result.assert_called_once()
94119

95120

96121
def test_timeout_initialization(mock_mcp_tool, mock_mcp_client):
@@ -110,10 +135,70 @@ async def test_stream_with_timeout(mock_mcp_tool, mock_mcp_client, alist):
110135
agent_tool = MCPAgentTool(mock_mcp_tool, mock_mcp_client, timeout=timeout)
111136
tool_use = {"toolUseId": "test-456", "name": "test_tool", "input": {"param": "value"}}
112137

113-
tru_events = await alist(agent_tool.stream(tool_use, {}))
114-
exp_events = [ToolResultEvent(mock_mcp_client.call_tool_async.return_value)]
138+
mock_result = mock_mcp_client._handle_tool_result.return_value
139+
140+
with patch("asyncio.wrap_future") as mock_wrap_future:
141+
142+
async def mock_awaitable(_):
143+
return MagicMock()
115144

116-
assert tru_events == exp_events
117-
mock_mcp_client.call_tool_async.assert_called_once_with(
118-
tool_use_id="test-456", name="test_tool", arguments={"param": "value"}, read_timeout_seconds=timeout
145+
mock_wrap_future.side_effect = mock_awaitable
146+
147+
tru_events = await alist(agent_tool.stream(tool_use, {}))
148+
149+
assert len(tru_events) == 1
150+
assert tru_events[0].exception is None
151+
assert tru_events[0].tool_result == mock_result
152+
mock_mcp_client._create_call_tool_coroutine.assert_called_once_with(
153+
"test_tool", {"param": "value"}, timeout
119154
)
155+
156+
157+
@pytest.mark.asyncio
158+
async def test_stream_propagates_exception(mock_mcp_tool, mock_mcp_client, alist):
159+
"""Test that stream() passes the original exception via ToolResultEvent.exception.
160+
161+
This ensures parity with decorated tools, where the exception is accessible
162+
via event.exception for debugging and conditional handling.
163+
"""
164+
agent_tool = MCPAgentTool(mock_mcp_tool, mock_mcp_client)
165+
tool_use = {"toolUseId": "test-123", "name": "test_tool", "input": {"param": "value"}}
166+
167+
test_exception = RuntimeError("MCP server connection failed")
168+
with patch("asyncio.wrap_future", side_effect=test_exception):
169+
mock_error_result = {
170+
"status": "error", "toolUseId": "test-123",
171+
"content": [{"text": "Tool execution failed: MCP server connection failed"}],
172+
"isError": True,
173+
}
174+
mock_mcp_client._handle_tool_execution_error.return_value = mock_error_result
175+
176+
tru_events = await alist(agent_tool.stream(tool_use, {}))
177+
178+
assert len(tru_events) == 1
179+
event = tru_events[0]
180+
assert event.exception is test_exception
181+
assert event.tool_result == mock_error_result
182+
mock_mcp_client._handle_tool_execution_error.assert_called_once_with("test-123", test_exception)
183+
184+
185+
@pytest.mark.asyncio
186+
async def test_stream_no_exception_on_success(mock_mcp_tool, mock_mcp_client, alist):
187+
"""Test that stream() sets exception=None on successful execution."""
188+
agent_tool = MCPAgentTool(mock_mcp_tool, mock_mcp_client)
189+
tool_use = {"toolUseId": "test-123", "name": "test_tool", "input": {"param": "value"}}
190+
191+
mock_result = mock_mcp_client._handle_tool_result.return_value
192+
193+
with patch("asyncio.wrap_future") as mock_wrap_future:
194+
195+
async def mock_awaitable(_):
196+
return MagicMock()
197+
198+
mock_wrap_future.side_effect = mock_awaitable
199+
200+
tru_events = await alist(agent_tool.stream(tool_use, {}))
201+
202+
assert len(tru_events) == 1
203+
assert tru_events[0].exception is None
204+
assert tru_events[0].tool_result == mock_result

tests/strands/tools/mcp/test_mcp_client.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ def test_call_tool_sync_status(mock_transport, mock_session, is_error, expected_
128128

129129
assert result["status"] == expected_status
130130
assert result["toolUseId"] == "test-123"
131+
assert result["isError"] == is_error
131132
assert len(result["content"]) == 1
132133
assert result["content"][0]["text"] == "Test message"
133134
# No structured content should be present when not provided by MCP
@@ -176,6 +177,7 @@ def test_call_tool_sync_exception(mock_transport, mock_session):
176177

177178
assert result["status"] == "error"
178179
assert result["toolUseId"] == "test-123"
180+
assert result["isError"] is True
179181
assert len(result["content"]) == 1
180182
assert "Test exception" in result["content"][0]["text"]
181183

@@ -214,6 +216,7 @@ async def mock_awaitable():
214216

215217
assert result["status"] == expected_status
216218
assert result["toolUseId"] == "test-123"
219+
assert result["isError"] == is_error
217220
assert len(result["content"]) == 1
218221
assert result["content"][0]["text"] == "Test message"
219222

@@ -241,6 +244,7 @@ async def test_call_tool_async_exception(mock_transport, mock_session):
241244

242245
assert result["status"] == "error"
243246
assert result["toolUseId"] == "test-123"
247+
assert result["isError"] is True
244248
assert len(result["content"]) == 1
245249
assert "Test exception" in result["content"][0]["text"]
246250

0 commit comments

Comments
 (0)