diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index c863b347..7afb7ff8 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,40 @@ 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 " + f"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 +300,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 9039bc7c..99eaeb34 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): @@ -207,24 +209,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."""