From f29179b30d5fd9c7daefc4feb7c6bf51492e39db Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 6 Nov 2025 21:06:13 -0500 Subject: [PATCH 1/3] fix: handle Pydantic TextContent objects in MCP response parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP SDK returns Pydantic TextContent objects, not plain dicts. The response_parser.py was using .get() method which only works on dicts, causing AttributeError: 'TextContent' object has no attribute 'get'. This change adds a helper function that handles both dict and Pydantic object access patterns, ensuring compatibility with both the MCP SDK's actual return types and the test suite's mock dicts. Tests added for: - Pydantic TextContent objects - Mixed dict and Pydantic content - Empty Pydantic text content handling Fixes regression introduced in v1.0.3 where parsing logic was added that assumed dict objects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/utils/response_parser.py | 27 ++++++++++++---- tests/test_response_parser.py | 51 +++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/adcp/utils/response_parser.py b/src/adcp/utils/response_parser.py index ad86929..f9c4beb 100644 --- a/src/adcp/utils/response_parser.py +++ b/src/adcp/utils/response_parser.py @@ -13,17 +13,20 @@ T = TypeVar("T", bound=BaseModel) -def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) -> T: +def parse_mcp_content(content: list[dict[str, Any] | Any], response_type: type[T]) -> T: """ Parse MCP content array into structured response type. MCP tools return content as a list of content items: [{"type": "text", "text": "..."}, {"type": "resource", ...}] + Content items may be either plain dicts or Pydantic objects (TextContent, etc.) + depending on the MCP SDK version and usage pattern. + For AdCP, we expect JSON data in text content items. Args: - content: MCP content array + content: MCP content array (list of dicts or Pydantic objects) response_type: Expected Pydantic model type Returns: @@ -35,10 +38,19 @@ def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) -> if not content: raise ValueError("Empty MCP content array") + # Helper to get field value from dict or object + def get_field(item: Any, field: str, default: Any = None) -> Any: + """Get field from dict or Pydantic object.""" + if isinstance(item, dict): + return item.get(field, default) + return getattr(item, field, default) + # Look for text content items that might contain JSON for item in content: - if item.get("type") == "text": - text = item.get("text", "") + item_type = get_field(item, "type") + + if item_type == "text": + text = get_field(item, "text", "") if not text: continue @@ -55,7 +67,7 @@ def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) -> f"MCP content doesn't match expected schema {response_type.__name__}: {e}" ) raise ValueError(f"MCP response doesn't match expected schema: {e}") from e - elif item.get("type") == "resource": + elif item_type == "resource": # Resource content might have structured data try: return response_type.model_validate(item) @@ -69,9 +81,12 @@ def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) -> if len(content_preview) > 500: content_preview = content_preview[:500] + "..." + # Extract types for error message + content_types = [get_field(item, "type") for item in content] + raise ValueError( f"No valid {response_type.__name__} data found in MCP content. " - f"Content types: {[item.get('type') for item in content]}. " + f"Content types: {content_types}. " f"Content preview:\n{content_preview}" ) diff --git a/tests/test_response_parser.py b/tests/test_response_parser.py index 8b6658a..d03897b 100644 --- a/tests/test_response_parser.py +++ b/tests/test_response_parser.py @@ -18,6 +18,13 @@ class SampleResponse(BaseModel): items: list[str] = Field(default_factory=list) +class MockTextContent(BaseModel): + """Mock MCP TextContent object for testing.""" + + type: str = "text" + text: str + + class TestParseMCPContent: """Tests for parse_mcp_content function.""" @@ -92,6 +99,50 @@ def test_empty_text_content_skipped(self): result = parse_mcp_content(content, SampleResponse) assert result.message == "Found" + def test_parse_pydantic_text_content(self): + """Test parsing MCP Pydantic TextContent objects.""" + content = [ + MockTextContent( + type="text", + text=json.dumps({"message": "Pydantic", "count": 99, "items": ["x", "y"]}), + ) + ] + + result = parse_mcp_content(content, SampleResponse) + + assert isinstance(result, SampleResponse) + assert result.message == "Pydantic" + assert result.count == 99 + assert result.items == ["x", "y"] + + def test_parse_mixed_dict_and_pydantic_content(self): + """Test parsing MCP content with both dicts and Pydantic objects.""" + content = [ + {"type": "text", "text": "Not JSON"}, + MockTextContent( + type="text", + text=json.dumps({"message": "Mixed", "count": 15}), + ), + ] + + result = parse_mcp_content(content, SampleResponse) + + assert result.message == "Mixed" + assert result.count == 15 + + def test_parse_pydantic_empty_text_skipped(self): + """Test that empty Pydantic text content is skipped.""" + content = [ + MockTextContent(type="text", text=""), + MockTextContent( + type="text", + text=json.dumps({"message": "Skip", "count": 7}), + ), + ] + + result = parse_mcp_content(content, SampleResponse) + assert result.message == "Skip" + class TestParseJSONOrText: """Tests for parse_json_or_text function.""" From 7d2a521389ef35560012ae0fa124db4b6e575d31 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 6 Nov 2025 21:12:31 -0500 Subject: [PATCH 2/3] refactor: move MCP serialization to protocol boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This refactor addresses the architectural issue identified in code review: the response parser was handling both parsing AND protocol translation, violating single responsibility principle. Changes: 1. Added _serialize_mcp_content() to MCPAdapter to convert Pydantic objects to dicts at the protocol boundary 2. Simplified parse_mcp_content() to only handle plain dicts 3. Removed get_field() helper function (no longer needed) 4. Removed Pydantic mock tests from response_parser tests 5. Added serialization tests to protocol adapter tests Benefits: - Clean separation: adapters translate, parsers parse - Simpler type signatures: list[dict[str, Any]] instead of | Any - Tests use plain dicts (no MCP SDK dependency in parser tests) - More obvious code (removed clever helper function) - Follows adapter pattern correctly The fix maintains backward compatibility while establishing proper architectural boundaries for future MCP content types (images, resources). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 41 ++++++++++++++++++++-- src/adcp/utils/response_parser.py | 28 +++++----------- tests/test_protocols.py | 56 +++++++++++++++++++++++++++++++ tests/test_response_parser.py | 51 ---------------------------- 4 files changed, 103 insertions(+), 73 deletions(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index fa61a20..8529890 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -186,6 +186,40 @@ async def _get_session(self) -> ClientSession: else: raise ValueError(f"Unsupported transport scheme: {parsed.scheme}") + def _serialize_mcp_content(self, content: list[Any]) -> list[dict[str, Any]]: + """ + Convert MCP SDK content objects to plain dicts. + + The MCP SDK returns Pydantic objects (TextContent, ImageContent, etc.) + but the rest of the ADCP client expects protocol-agnostic dicts. + This method handles the translation at the protocol boundary. + + Args: + content: List of MCP content items (may be dicts or Pydantic objects) + + Returns: + List of plain dicts representing the content + """ + result = [] + for item in content: + # Already a dict, pass through + if isinstance(item, dict): + result.append(item) + # Pydantic v2 model with model_dump() + elif hasattr(item, "model_dump"): + result.append(item.model_dump()) + # Pydantic v1 model with dict() + elif hasattr(item, "dict") and callable(item.dict): + result.append(item.dict()) # type: ignore[attr-defined] + # Fallback: try to access __dict__ + elif hasattr(item, "__dict__"): + result.append(dict(item.__dict__)) + # Last resort: serialize as unknown type + else: + logger.warning(f"Unknown MCP content type: {type(item)}, serializing as string") + result.append({"type": "unknown", "data": str(item)}) + return result + async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskResult[Any]: """Call a tool using MCP protocol.""" start_time = time.time() if self.agent_config.debug else None @@ -205,12 +239,15 @@ 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) + # Serialize MCP SDK types to plain dicts at protocol boundary + serialized_content = self._serialize_mcp_content(result.content) + if self.agent_config.debug and start_time: duration_ms = (time.time() - start_time) * 1000 debug_info = DebugInfo( request=debug_request, response={ - "content": result.content, + "content": serialized_content, "is_error": result.isError if hasattr(result, "isError") else False, }, duration_ms=duration_ms, @@ -220,7 +257,7 @@ async def _call_mcp_tool(self, tool_name: str, params: dict[str, Any]) -> TaskRe # For AdCP, we expect the data in the content return TaskResult[Any]( status=TaskStatus.COMPLETED, - data=result.content, + data=serialized_content, success=True, debug_info=debug_info, ) diff --git a/src/adcp/utils/response_parser.py b/src/adcp/utils/response_parser.py index f9c4beb..cd60541 100644 --- a/src/adcp/utils/response_parser.py +++ b/src/adcp/utils/response_parser.py @@ -13,20 +13,20 @@ T = TypeVar("T", bound=BaseModel) -def parse_mcp_content(content: list[dict[str, Any] | Any], response_type: type[T]) -> T: +def parse_mcp_content(content: list[dict[str, Any]], response_type: type[T]) -> T: """ Parse MCP content array into structured response type. MCP tools return content as a list of content items: [{"type": "text", "text": "..."}, {"type": "resource", ...}] - Content items may be either plain dicts or Pydantic objects (TextContent, etc.) - depending on the MCP SDK version and usage pattern. + The MCP adapter is responsible for serializing MCP SDK Pydantic objects + to plain dicts before calling this function. For AdCP, we expect JSON data in text content items. Args: - content: MCP content array (list of dicts or Pydantic objects) + content: MCP content array (list of plain dicts) response_type: Expected Pydantic model type Returns: @@ -38,19 +38,10 @@ def parse_mcp_content(content: list[dict[str, Any] | Any], response_type: type[T if not content: raise ValueError("Empty MCP content array") - # Helper to get field value from dict or object - def get_field(item: Any, field: str, default: Any = None) -> Any: - """Get field from dict or Pydantic object.""" - if isinstance(item, dict): - return item.get(field, default) - return getattr(item, field, default) - # Look for text content items that might contain JSON for item in content: - item_type = get_field(item, "type") - - if item_type == "text": - text = get_field(item, "text", "") + if item.get("type") == "text": + text = item.get("text", "") if not text: continue @@ -67,7 +58,7 @@ def get_field(item: Any, field: str, default: Any = None) -> Any: f"MCP content doesn't match expected schema {response_type.__name__}: {e}" ) raise ValueError(f"MCP response doesn't match expected schema: {e}") from e - elif item_type == "resource": + elif item.get("type") == "resource": # Resource content might have structured data try: return response_type.model_validate(item) @@ -81,12 +72,9 @@ def get_field(item: Any, field: str, default: Any = None) -> Any: if len(content_preview) > 500: content_preview = content_preview[:500] + "..." - # Extract types for error message - content_types = [get_field(item, "type") for item in content] - raise ValueError( f"No valid {response_type.__name__} data found in MCP content. " - f"Content types: {content_types}. " + f"Content types: {[item.get('type') for item in content]}. " f"Content preview:\n{content_preview}" ) diff --git a/tests/test_protocols.py b/tests/test_protocols.py index 7ffd8c7..b10e801 100644 --- a/tests/test_protocols.py +++ b/tests/test_protocols.py @@ -240,3 +240,59 @@ async def test_close_session(self, mcp_config): mock_exit_stack.aclose.assert_called_once() assert adapter._exit_stack is None assert adapter._session is None + + def test_serialize_mcp_content_with_dicts(self, mcp_config): + """Test serializing MCP content that's already dicts.""" + adapter = MCPAdapter(mcp_config) + + content = [ + {"type": "text", "text": "Hello"}, + {"type": "resource", "uri": "file://test.txt"}, + ] + + result = adapter._serialize_mcp_content(content) + + assert result == content # Pass through unchanged + assert len(result) == 2 + + def test_serialize_mcp_content_with_pydantic_v2(self, mcp_config): + """Test serializing MCP content with Pydantic v2 objects.""" + from pydantic import BaseModel + + adapter = MCPAdapter(mcp_config) + + class MockTextContent(BaseModel): + type: str + text: str + + content = [ + MockTextContent(type="text", text="Pydantic v2"), + ] + + result = adapter._serialize_mcp_content(content) + + assert len(result) == 1 + assert result[0] == {"type": "text", "text": "Pydantic v2"} + assert isinstance(result[0], dict) + + def test_serialize_mcp_content_mixed(self, mcp_config): + """Test serializing mixed MCP content (dicts and Pydantic objects).""" + from pydantic import BaseModel + + adapter = MCPAdapter(mcp_config) + + class MockTextContent(BaseModel): + type: str + text: str + + content = [ + {"type": "text", "text": "Plain dict"}, + MockTextContent(type="text", text="Pydantic object"), + ] + + result = adapter._serialize_mcp_content(content) + + assert len(result) == 2 + assert result[0] == {"type": "text", "text": "Plain dict"} + assert result[1] == {"type": "text", "text": "Pydantic object"} + assert all(isinstance(item, dict) for item in result) diff --git a/tests/test_response_parser.py b/tests/test_response_parser.py index d03897b..8b6658a 100644 --- a/tests/test_response_parser.py +++ b/tests/test_response_parser.py @@ -18,13 +18,6 @@ class SampleResponse(BaseModel): items: list[str] = Field(default_factory=list) -class MockTextContent(BaseModel): - """Mock MCP TextContent object for testing.""" - - type: str = "text" - text: str - - class TestParseMCPContent: """Tests for parse_mcp_content function.""" @@ -99,50 +92,6 @@ def test_empty_text_content_skipped(self): result = parse_mcp_content(content, SampleResponse) assert result.message == "Found" - def test_parse_pydantic_text_content(self): - """Test parsing MCP Pydantic TextContent objects.""" - content = [ - MockTextContent( - type="text", - text=json.dumps({"message": "Pydantic", "count": 99, "items": ["x", "y"]}), - ) - ] - - result = parse_mcp_content(content, SampleResponse) - - assert isinstance(result, SampleResponse) - assert result.message == "Pydantic" - assert result.count == 99 - assert result.items == ["x", "y"] - - def test_parse_mixed_dict_and_pydantic_content(self): - """Test parsing MCP content with both dicts and Pydantic objects.""" - content = [ - {"type": "text", "text": "Not JSON"}, - MockTextContent( - type="text", - text=json.dumps({"message": "Mixed", "count": 15}), - ), - ] - - result = parse_mcp_content(content, SampleResponse) - - assert result.message == "Mixed" - assert result.count == 15 - - def test_parse_pydantic_empty_text_skipped(self): - """Test that empty Pydantic text content is skipped.""" - content = [ - MockTextContent(type="text", text=""), - MockTextContent( - type="text", - text=json.dumps({"message": "Skip", "count": 7}), - ), - ] - - result = parse_mcp_content(content, SampleResponse) - assert result.message == "Skip" - class TestParseJSONOrText: """Tests for parse_json_or_text function.""" From c87ca286260e2b30490633adf928bfe704d355f1 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Thu, 6 Nov 2025 21:22:23 -0500 Subject: [PATCH 3/3] fix: remove unused type ignore comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mypy now correctly infers types after hasattr and callable checks, making the type: ignore[attr-defined] comment unnecessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/protocols/mcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/adcp/protocols/mcp.py b/src/adcp/protocols/mcp.py index 8529890..4fc6ce2 100644 --- a/src/adcp/protocols/mcp.py +++ b/src/adcp/protocols/mcp.py @@ -210,7 +210,7 @@ def _serialize_mcp_content(self, content: list[Any]) -> list[dict[str, Any]]: result.append(item.model_dump()) # Pydantic v1 model with dict() elif hasattr(item, "dict") and callable(item.dict): - result.append(item.dict()) # type: ignore[attr-defined] + result.append(item.dict()) # Fallback: try to access __dict__ elif hasattr(item, "__dict__"): result.append(dict(item.__dict__))