From 93cbf6b3f003e65d43fffe63cccf89c64a7df6ba Mon Sep 17 00:00:00 2001 From: Giles Odigwe <79032838+giles17@users.noreply.github.com> Date: Wed, 10 Jun 2026 05:51:09 -0700 Subject: [PATCH] Python: Parse MCP CallToolResult.structuredContent field to prevent tool results returning None (#6421) * Parse structuredContent from MCP CallToolResult (#3313) The _parse_tool_result_from_mcp method only iterated over the content field from CallToolResult, ignoring the structuredContent field entirely. MCP servers that return JSON data via structuredContent (e.g., Power BI MCP) appeared to return None. Add handling for structuredContent: when present, serialize it as JSON text and append it to the result list. This preserves the data for the LLM while maintaining backward compatibility with existing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Python: Parse MCP CallToolResult.structuredContent field to prevent tool results returning None Fixes #3313 * Address review feedback: add default=str to json.dumps and remove .checkpoints/ - Add default=str to json.dumps for structuredContent serialization so non-JSON-serializable values (e.g. bytes) degrade gracefully instead of raising TypeError - Remove all .checkpoints/ runtime artifacts from the repository - Add **/.checkpoints/ to .gitignore to prevent future accidental commits - Add test for non-serializable structuredContent values Fixes #3313 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for #3313: Python: MCP CallToolResult.structuredContent field is not parsed, causing tool results to return None --------- Co-authored-by: Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .gitignore | 1 + python/packages/core/agent_framework/_mcp.py | 3 + .../packages/core/agent_framework/_skills.py | 10 +- python/packages/core/tests/core/test_mcp.py | 63 ++++++ .../core/tests/core/test_mcp_observability.py | 5 +- .../core/tests/core/test_mcp_skills.py | 190 ++++++++---------- .../mem0/tests/test_mem0_context_provider.py | 7 +- .../02-agents/harness/console/agent_runner.py | 4 +- .../samples/02-agents/harness/console/app.py | 10 +- .../harness/console/commands/todo_handler.py | 4 +- .../console/components/scroll_panel.py | 2 +- .../console/observers/planning_models.py | 3 +- 12 files changed, 158 insertions(+), 144 deletions(-) diff --git a/.gitignore b/.gitignore index 9cb714813a..e6b9efb40e 100644 --- a/.gitignore +++ b/.gitignore @@ -206,6 +206,7 @@ temp*/ .temp/ # AI +**/.checkpoints/ .claude/ .omc/ .omx/ diff --git a/python/packages/core/agent_framework/_mcp.py b/python/packages/core/agent_framework/_mcp.py index ccb0be3b70..2fc79e85a5 100644 --- a/python/packages/core/agent_framework/_mcp.py +++ b/python/packages/core/agent_framework/_mcp.py @@ -577,6 +577,9 @@ class MCPTool: case _: result.append(Content.from_text(str(item))) + if mcp_type.structuredContent is not None: + result.append(Content.from_text(json.dumps(mcp_type.structuredContent, default=str))) + if not result: result.append(Content.from_text("null")) return result diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 97afe66cea..91bdb61914 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -3516,9 +3516,7 @@ class MCPSkill(Skill): result = await self._client.read_resource(_mcp_any_url(self._skill_md_uri)) text = _mcp_join_text(result) if not text: - raise ValueError( - f"The MCP server returned no text content for SKILL.md resource '{self._skill_md_uri}'." - ) + raise ValueError(f"The MCP server returned no text content for SKILL.md resource '{self._skill_md_uri}'.") self._content = text return text @@ -3572,11 +3570,7 @@ class MCPSkill(Skill): or ``None`` if the name is unsafe. """ normalized = name.replace("\\", "/") - if ( - normalized.startswith("/") - or "://" in normalized - or any(seg == ".." for seg in normalized.split("/")) - ): + if normalized.startswith("/") or "://" in normalized or any(seg == ".." for seg in normalized.split("/")): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None return normalized diff --git a/python/packages/core/tests/core/test_mcp.py b/python/packages/core/tests/core/test_mcp.py index ce69e9766b..a40c1c9b54 100644 --- a/python/packages/core/tests/core/test_mcp.py +++ b/python/packages/core/tests/core/test_mcp.py @@ -342,6 +342,69 @@ def test_parse_tool_result_from_mcp_resource_link_text_resource_and_unknown(): assert result[1].text == "Embedded result" +def test_parse_tool_result_from_mcp_structured_content_only(): + """Test that structuredContent is parsed when content list is empty.""" + mcp_result = types.CallToolResult( + content=[], + structuredContent={"Tables": [{"Name": "Sales", "Columns": ["Amount", "Date"]}]}, + ) + result = _HELPER_MCP_TOOL._parse_tool_result_from_mcp(mcp_result) + + assert isinstance(result, list) + assert len(result) == 1 + assert result[0].type == "text" + parsed = json.loads(result[0].text) + assert parsed == {"Tables": [{"Name": "Sales", "Columns": ["Amount", "Date"]}]} + + +def test_parse_tool_result_from_mcp_structured_content_with_text(): + """Test that structuredContent is appended alongside regular content items.""" + mcp_result = types.CallToolResult( + content=[types.TextContent(type="text", text="Summary")], + structuredContent={"data": [1, 2, 3]}, + ) + result = _HELPER_MCP_TOOL._parse_tool_result_from_mcp(mcp_result) + + assert isinstance(result, list) + assert len(result) == 2 + assert result[0].type == "text" + assert result[0].text == "Summary" + assert result[1].type == "text" + parsed = json.loads(result[1].text) + assert parsed == {"data": [1, 2, 3]} + + +def test_parse_tool_result_from_mcp_structured_content_none(): + """Test that None structuredContent does not affect results.""" + mcp_result = types.CallToolResult( + content=[types.TextContent(type="text", text="Hello")], + structuredContent=None, + ) + result = _HELPER_MCP_TOOL._parse_tool_result_from_mcp(mcp_result) + + assert isinstance(result, list) + assert len(result) == 1 + assert result[0].type == "text" + assert result[0].text == "Hello" + + +def test_parse_tool_result_from_mcp_structured_content_non_serializable(): + """Test that non-JSON-serializable values in structuredContent degrade gracefully.""" + mcp_result = types.CallToolResult( + content=[], + structuredContent={"data": b"raw bytes", "count": 42}, + ) + result = _HELPER_MCP_TOOL._parse_tool_result_from_mcp(mcp_result) + + assert isinstance(result, list) + assert len(result) == 1 + assert result[0].type == "text" + parsed = json.loads(result[0].text) + assert parsed["count"] == 42 + # bytes should be converted to string representation via default=str + assert "raw bytes" in parsed["data"] + + def test_mcp_content_types_to_ai_content_text(): """Test conversion of MCP text content to AI content.""" mcp_content = types.TextContent(type="text", text="Sample text") diff --git a/python/packages/core/tests/core/test_mcp_observability.py b/python/packages/core/tests/core/test_mcp_observability.py index 226e976120..e32bce1b02 100644 --- a/python/packages/core/tests/core/test_mcp_observability.py +++ b/python/packages/core/tests/core/test_mcp_observability.py @@ -76,6 +76,7 @@ def _make_call_tool_result(text: str = "result", is_error: bool = False) -> Mock result = Mock() result.isError = is_error result.content = [types.TextContent(type="text", text=text)] + result.structuredContent = None return result @@ -281,9 +282,7 @@ async def test_mcp_prompts_get_creates_client_span(span_exporter: InMemorySpanEx async def test_mcp_prompts_get_mcp_error_sets_error_type(span_exporter: InMemorySpanExporter): """When session.get_prompt() raises McpError, the span should have error.type and ERROR status.""" tool = _make_connected_mcp_tool() - tool.session.get_prompt = AsyncMock( - side_effect=McpError(ErrorData(code=-32602, message="prompt not found")) - ) + tool.session.get_prompt = AsyncMock(side_effect=McpError(ErrorData(code=-32602, message="prompt not found"))) span_exporter.clear() with pytest.raises(ToolExecutionException): diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index 3e7c67662a..74993997d0 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -35,26 +35,22 @@ description: Convert between common units. Body content here. """ -SAMPLE_SKILL_INDEX = json.dumps( - { - "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", - "skills": [ - { - "name": "unit-converter", - "type": "skill-md", - "description": "Convert between common units.", - "url": "skill://unit-converter/SKILL.md", - } - ], - } -) +SAMPLE_SKILL_INDEX = json.dumps({ + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "name": "unit-converter", + "type": "skill-md", + "description": "Convert between common units.", + "url": "skill://unit-converter/SKILL.md", + } + ], +}) def _make_text_result(text: str, uri: str = "skill://test") -> ReadResourceResult: """Create a ReadResourceResult with a single TextResourceContents.""" - return ReadResourceResult( - contents=[TextResourceContents(uri=AnyUrl(uri), text=text, mimeType="text/markdown")] - ) + return ReadResourceResult(contents=[TextResourceContents(uri=AnyUrl(uri), text=text, mimeType="text/markdown")]) def _make_blob_result( @@ -230,12 +226,10 @@ class TestMCPSkill: @pytest.mark.asyncio async def test_get_resource_text(self) -> None: - client = _make_client( - **{ - "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), - "skill://unit-converter/references/checklist.md": _make_text_result("- check thing 1\n- check thing 2"), - } - ) + client = _make_client(**{ + "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), + "skill://unit-converter/references/checklist.md": _make_text_result("- check thing 1\n- check thing 2"), + }) from agent_framework import SkillFrontmatter fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") @@ -249,12 +243,10 @@ class TestMCPSkill: @pytest.mark.asyncio async def test_get_resource_binary(self) -> None: data = bytes([0x01, 0x02, 0x03, 0x04]) - client = _make_client( - **{ - "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), - "skill://unit-converter/assets/icon.bin": _make_blob_result(data), - } - ) + client = _make_client(**{ + "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), + "skill://unit-converter/assets/icon.bin": _make_blob_result(data), + }) from agent_framework import SkillFrontmatter fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") @@ -345,12 +337,10 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_index_based_discovery_returns_skill(self) -> None: - client = _make_client( - **{ - "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), - "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), - } - ) + client = _make_client(**{ + "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), + "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), + }) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -373,9 +363,7 @@ class TestMCPSkillsSource: async def test_does_not_read_skill_md_during_discovery(self) -> None: # Index points to a skill, but SKILL.md is not registered on the server. # Discovery should succeed because it only reads the index. - client = _make_client( - **{"skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json")} - ) + client = _make_client(**{"skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -384,19 +372,17 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_invalid_name_is_skipped(self) -> None: - index_json = json.dumps( - { - "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", - "skills": [ - { - "name": "UnitConverter", # Invalid: uppercase - "type": "skill-md", - "description": "Convert between common units.", - "url": "skill://UnitConverter/SKILL.md", - } - ], - } - ) + index_json = json.dumps({ + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "name": "UnitConverter", # Invalid: uppercase + "type": "skill-md", + "description": "Convert between common units.", + "url": "skill://UnitConverter/SKILL.md", + } + ], + }) client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -404,18 +390,16 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_missing_required_fields_is_skipped(self) -> None: - index_json = json.dumps( - { - "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", - "skills": [ - { - "name": "unit-converter", - "type": "skill-md", - # Missing description and url - } - ], - } - ) + index_json = json.dumps({ + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "name": "unit-converter", + "type": "skill-md", + # Missing description and url + } + ], + }) client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -423,19 +407,17 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_unsupported_type_is_skipped(self) -> None: - index_json = json.dumps( - { - "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", - "skills": [ - { - "name": "some-skill", - "type": "archive", - "description": "Packaged skill.", - "url": "skill://some-skill.tar.gz", - } - ], - } - ) + index_json = json.dumps({ + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "name": "some-skill", + "type": "archive", + "description": "Packaged skill.", + "url": "skill://some-skill.tar.gz", + } + ], + }) client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -443,18 +425,16 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_template_type_is_skipped(self) -> None: - index_json = json.dumps( - { - "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", - "skills": [ - { - "type": "mcp-resource-template", - "description": "Per-product documentation skill", - "url": "skill://docs/{product}/SKILL.md", - } - ], - } - ) + index_json = json.dumps({ + "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", + "skills": [ + { + "type": "mcp-resource-template", + "description": "Per-product documentation skill", + "url": "skill://docs/{product}/SKILL.md", + } + ], + }) client = _make_client(**{"skill://index.json": _make_text_result(index_json, uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() @@ -462,31 +442,25 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_empty_index_returns_empty(self) -> None: - client = _make_client( - **{"skill://index.json": _make_text_result('{"skills": []}', uri="skill://index.json")} - ) + client = _make_client(**{"skill://index.json": _make_text_result('{"skills": []}', uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() assert skills == [] @pytest.mark.asyncio async def test_malformed_index_json_returns_empty(self) -> None: - client = _make_client( - **{"skill://index.json": _make_text_result("not valid json", uri="skill://index.json")} - ) + client = _make_client(**{"skill://index.json": _make_text_result("not valid json", uri="skill://index.json")}) source = MCPSkillsSource(client=client) skills = await source.get_skills() assert skills == [] @pytest.mark.asyncio async def test_sibling_text_resource(self) -> None: - client = _make_client( - **{ - "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), - "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), - "skill://unit-converter/references/checklist.md": _make_text_result("- check thing 1\n- check thing 2"), - } - ) + client = _make_client(**{ + "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), + "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), + "skill://unit-converter/references/checklist.md": _make_text_result("- check thing 1\n- check thing 2"), + }) source = MCPSkillsSource(client=client) skill = (await source.get_skills())[0] resource = await skill.get_resource("references/checklist.md") @@ -497,13 +471,11 @@ class TestMCPSkillsSource: @pytest.mark.asyncio async def test_sibling_binary_resource(self) -> None: data = bytes([0x01, 0x02, 0x03, 0x04]) - client = _make_client( - **{ - "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), - "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), - "skill://unit-converter/assets/icon.bin": _make_blob_result(data), - } - ) + client = _make_client(**{ + "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), + "skill://unit-converter/SKILL.md": _make_text_result(SAMPLE_SKILL_MD), + "skill://unit-converter/assets/icon.bin": _make_blob_result(data), + }) source = MCPSkillsSource(client=client) skill = (await source.get_skills())[0] resource = await skill.get_resource("assets/icon.bin") @@ -649,9 +621,7 @@ class TestMCPSkillsSourceErrorCodeBranching: from agent_framework import SkillFrontmatter client = AsyncMock() - client.read_resource = AsyncMock( - side_effect=McpError(error=ErrorData(code=0, message="Handler error")) - ) + client.read_resource = AsyncMock(side_effect=McpError(error=ErrorData(code=0, message="Handler error"))) fm = SkillFrontmatter(name="test-skill", description="Test.") skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://test/SKILL.md", client=client) with pytest.raises(McpError): diff --git a/python/packages/mem0/tests/test_mem0_context_provider.py b/python/packages/mem0/tests/test_mem0_context_provider.py index a047af1638..d5c9e2fe67 100644 --- a/python/packages/mem0/tests/test_mem0_context_provider.py +++ b/python/packages/mem0/tests/test_mem0_context_provider.py @@ -198,12 +198,7 @@ class TestBeforeRun: """OSS client with all scoping parameters passes them as isolated concurrent kwargs.""" mock_oss_mem0_client.search.return_value = [] - provider = Mem0ContextProvider( - source_id="mem0", - mem0_client=mock_oss_mem0_client, - user_id="u1", - agent_id="a1" - ) + provider = Mem0ContextProvider(source_id="mem0", mem0_client=mock_oss_mem0_client, user_id="u1", agent_id="a1") mock_context = MagicMock(spec=SessionContext) mock_msg = MagicMock() diff --git a/python/samples/02-agents/harness/console/agent_runner.py b/python/samples/02-agents/harness/console/agent_runner.py index 3b7c685dbd..743ad1e132 100644 --- a/python/samples/02-agents/harness/console/agent_runner.py +++ b/python/samples/02-agents/harness/console/agent_runner.py @@ -313,9 +313,7 @@ class HarnessAgentRunner: """ actions: list[FollowUpAction] = [] for observer in self._observers: - observer_actions = await observer.on_stream_complete( - self._ux, self._agent, session - ) + observer_actions = await observer.on_stream_complete(self._ux, self._agent, session) if observer_actions: actions.extend(observer_actions) return actions diff --git a/python/samples/02-agents/harness/console/app.py b/python/samples/02-agents/harness/console/app.py index c56360c661..e2260eb200 100644 --- a/python/samples/02-agents/harness/console/app.py +++ b/python/samples/02-agents/harness/console/app.py @@ -182,18 +182,12 @@ class HarnessApp(App[None]): if command_handlers is None: from .commands import build_default_command_handlers - self._command_handlers = build_default_command_handlers( - agent, mode_colors=mode_colors - ) + self._command_handlers = build_default_command_handlers(agent, mode_colors=mode_colors) else: self._command_handlers = command_handlers # Compute help text from command handlers - help_parts = [ - h.get_help_text() - for h in self._command_handlers - if h.get_help_text() is not None - ] + help_parts = [h.get_help_text() for h in self._command_handlers if h.get_help_text() is not None] help_text = ", ".join(help_parts) if help_parts else None # State and driver diff --git a/python/samples/02-agents/harness/console/commands/todo_handler.py b/python/samples/02-agents/harness/console/commands/todo_handler.py index 73703e6db3..e32ffd3f6a 100644 --- a/python/samples/02-agents/harness/console/commands/todo_handler.py +++ b/python/samples/02-agents/harness/console/commands/todo_handler.py @@ -45,9 +45,7 @@ class TodoCommandHandler(CommandHandler): ux.append_info_line("TodoProvider is not available.") return True - todos = await self._todo_provider.store.load_items( - session, source_id=self._todo_provider.source_id - ) + todos = await self._todo_provider.store.load_items(session, source_id=self._todo_provider.source_id) if not todos: ux.append_info_line("No todos yet.") diff --git a/python/samples/02-agents/harness/console/components/scroll_panel.py b/python/samples/02-agents/harness/console/components/scroll_panel.py index a9cf15a774..35b478b54b 100644 --- a/python/samples/02-agents/harness/console/components/scroll_panel.py +++ b/python/samples/02-agents/harness/console/components/scroll_panel.py @@ -72,7 +72,7 @@ class HarnessScrollPanel(RichLog): # Truncate lines back to where streaming started if len(self.lines) > self._streaming_line_start: - del self.lines[self._streaming_line_start:] + del self.lines[self._streaming_line_start :] from textual.geometry import Size self.virtual_size = Size(self._widest_line_width, len(self.lines)) diff --git a/python/samples/02-agents/harness/console/observers/planning_models.py b/python/samples/02-agents/harness/console/observers/planning_models.py index 9b4a92e575..d4c425b078 100644 --- a/python/samples/02-agents/harness/console/observers/planning_models.py +++ b/python/samples/02-agents/harness/console/observers/planning_models.py @@ -41,8 +41,7 @@ class PlanningQuestion(BaseModel): choices: list[str] | None = Field( default=None, description=( - "For clarifications, this has a list of options that the user can " - "choose from. null for approvals." + "For clarifications, this has a list of options that the user can choose from. null for approvals." ), )