From 13f836c8eb181a5d93ef31139eb843d3b6d0821c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 11 Nov 2025 07:26:54 -0500 Subject: [PATCH 1/3] fix: handle MCP error responses without structuredContent gracefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an MCP tool returns an error (isError=True), it may only populate the content field with an error message and leave structuredContent as None. The SDK now handles this case gracefully by: 1. Checking isError flag before requiring structuredContent 2. Extracting error message from content field for error responses 3. Returning a proper TaskResult with success=False and the error message 4. Only requiring structuredContent for successful responses (isError=False) This allows the CLI to display agent-provided error messages instead of raising SDK validation errors. Updated tests to cover both cases: - Error responses without structuredContent (now valid) - Success responses without structuredContent (still invalid) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 53 +++++++++++++++++++++++++++------------ tests/test_protocols.py | 28 ++++++++++++++++++--- 2 files changed, 62 insertions(+), 19 deletions(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index c863b34..c050c0b 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -245,24 +245,12 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe # Call the tool using MCP client session result = await session.call_tool(tool_name, params) - # This SDK requires MCP tools to return structuredContent - # The content field may contain human-readable messages but the actual - # response data must be in structuredContent - if not hasattr(result, "structuredContent") or result.structuredContent is None: - raise ValueError( - f"MCP tool {tool_name} did not return structuredContent. " - f"This SDK requires MCP tools to provide structured responses. " - f"Got content: {result.content if hasattr(result, 'content') else 'none'}" - ) + # Check if this is an error response + is_error = hasattr(result, "isError") and result.isError - # Extract the structured data (required) - data_to_return = result.structuredContent - - # Extract human-readable message from content (optional) - # This is typically a status message like "Found 42 creative formats" + # Extract human-readable message from content message_text = None if hasattr(result, "content") and result.content: - # Serialize content using the same method used for backward compatibility serialized_content = self._serialize_mcp_content(result.content) if isinstance(serialized_content, list): for item in serialized_content: @@ -271,6 +259,39 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe message_text = item["text"] break + # Handle error responses + if is_error: + # For error responses, structuredContent is optional + # Use the error message from content as the error + error_message = message_text or "Tool execution failed" + if self.agent_config.debug and start_time: + duration_ms = (time.time() - start_time) * 1000 + debug_info = DebugInfo( + request=debug_request, + response={ + "error": error_message, + "is_error": True, + }, + duration_ms=duration_ms, + ) + return TaskResult[Any]( + status=TaskStatus.FAILED, + error=error_message, + success=False, + debug_info=debug_info, + ) + + # For successful responses, structuredContent is required + if not hasattr(result, "structuredContent") or result.structuredContent is None: + raise ValueError( + f"MCP tool {tool_name} did not return structuredContent. " + f"This SDK requires MCP tools to provide structured responses for successful calls. " + f"Got content: {result.content if hasattr(result, 'content') else 'none'}" + ) + + # Extract the structured data (required for success) + data_to_return = result.structuredContent + if self.agent_config.debug and start_time: duration_ms = (time.time() - start_time) * 1000 debug_info = DebugInfo( @@ -278,7 +299,7 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe response={ "data": data_to_return, "message": message_text, - "is_error": result.isError if hasattr(result, "isError") else False, + "is_error": False, }, duration_ms=duration_ms, ) diff --git a/tests/test_protocols.py b/tests/test_protocols.py index 9039bc7..04b3f75 100644 --- a/tests/test_protocols.py +++ b/tests/test_protocols.py @@ -207,24 +207,46 @@ async def test_call_tool_with_structured_content(self, mcp_config): @pytest.mark.asyncio async def test_call_tool_missing_structured_content(self, mcp_config): - """Test tool call fails when structuredContent is missing.""" + """Test tool call fails when structuredContent is missing on successful response.""" adapter = MCPAdapter(mcp_config) mock_session = AsyncMock() mock_result = MagicMock() - # Mock MCP result WITHOUT structuredContent (invalid for AdCP) + # Mock MCP result WITHOUT structuredContent and isError=False (invalid) mock_result.content = [{"type": "text", "text": "Success"}] mock_result.structuredContent = None + mock_result.isError = False mock_session.call_tool.return_value = mock_result with patch.object(adapter, "_get_session", return_value=mock_session): result = await adapter._call_mcp_tool("get_products", {"brief": "test"}) - # Verify error handling for missing structuredContent + # Verify error handling for missing structuredContent on success assert result.success is False assert result.status == TaskStatus.FAILED assert "did not return structuredContent" in result.error + @pytest.mark.asyncio + async def test_call_tool_error_without_structured_content(self, mcp_config): + """Test tool call handles error responses without structuredContent gracefully.""" + adapter = MCPAdapter(mcp_config) + + mock_session = AsyncMock() + mock_result = MagicMock() + # Mock MCP error response WITHOUT structuredContent (valid for errors) + mock_result.content = [{"type": "text", "text": "brand_manifest must provide brand information"}] + mock_result.structuredContent = None + mock_result.isError = True + mock_session.call_tool.return_value = mock_result + + with patch.object(adapter, "_get_session", return_value=mock_session): + result = await adapter._call_mcp_tool("get_products", {"brief": "test"}) + + # Verify error is handled gracefully + assert result.success is False + assert result.status == TaskStatus.FAILED + assert result.error == "brand_manifest must provide brand information" + @pytest.mark.asyncio async def test_call_tool_error(self, mcp_config): """Test tool call error via MCP.""" From 047cf849342bd81ef37badf6534aa6e2e3a0fc04 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 11 Nov 2025 08:39:48 -0500 Subject: [PATCH 2/3] fix: split long error message to satisfy line length limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the structuredContent error message across multiple lines to keep each line under 100 characters for linter compliance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index c050c0b..7afb7ff 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -285,7 +285,8 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe if not hasattr(result, "structuredContent") or result.structuredContent is None: raise ValueError( f"MCP tool {tool_name} did not return structuredContent. " - f"This SDK requires MCP tools to provide structured responses for successful calls. " + f"This SDK requires MCP tools to provide structured responses " + f"for successful calls. " f"Got content: {result.content if hasattr(result, 'content') else 'none'}" ) From 60a06772a6e09c289c170d829e03b04a8ca5eb85 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 11 Nov 2025 08:43:50 -0500 Subject: [PATCH 3/3] fix: explicitly set isError=False in test mocks for successful responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MagicMock objects return truthy MagicMock instances for any attribute access, so mock_result.isError was returning a truthy value even when not explicitly set. This caused the code to incorrectly treat successful test responses as errors. Now explicitly setting isError=False for all successful response mocks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- tests/test_protocols.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_protocols.py b/tests/test_protocols.py index 04b3f75..99eaeb3 100644 --- a/tests/test_protocols.py +++ b/tests/test_protocols.py @@ -164,6 +164,7 @@ async def test_call_tool_success(self, mcp_config): # Mock MCP result with structuredContent (required for AdCP) mock_result.content = [{"type": "text", "text": "Success"}] mock_result.structuredContent = {"products": [{"id": "prod1"}]} + mock_result.isError = False mock_session.call_tool.return_value = mock_result with patch.object(adapter, "_get_session", return_value=mock_session): @@ -193,6 +194,7 @@ async def test_call_tool_with_structured_content(self, mcp_config): # Mock MCP result with structuredContent (preferred over content) mock_result.content = [{"type": "text", "text": "Found 42 creative formats"}] mock_result.structuredContent = {"formats": [{"id": "format1"}, {"id": "format2"}]} + mock_result.isError = False mock_session.call_tool.return_value = mock_result with patch.object(adapter, "_get_session", return_value=mock_session):