Skip to content

Conversation

Kunmeer-SyedMohamedHyder
Copy link
Contributor

Summary

This PR handles the MCP tool output where structured content could never be returned exclusively when use_structured_content=True. The conditional logic checked for content length first, making the structured content branch unreachable when both content types were present.

Before (broken logic):

if len(result.content) == 1:
    tool_output = result.content[0].model_dump_json()
elif server.use_structured_content and result.structuredContent:  # Never reached!
    tool_output = json.dumps(result.structuredContent)

After (fixed logic):

if server.use_structured_content and result.structuredContent:
    tool_output = json.dumps(result.structuredContent)
else:
    # Fall back to regular text content processing
    if len(result.content) == 1:
        tool_output = result.content[0].model_dump_json()

Example usage:

# MCP server with structured content enabled
server = MCPServer(use_structured_content=True)

# When invoke_mcp_tool processes a tool that returns both text and structured content:
# - Text content: "Found 2 users"
# - Structured content: {"users": [{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]}

result = await MCPUtil.invoke_mcp_tool(server, tool, context, input_json)

# Before fix: Would return '{"type":"text","text":"Found 2 users","annotations":null,"meta":null}'
# After fix: Returns '{"users": [{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]}'

Test plan

I've added thorough test coverage to make sure this fix works properly:

  • Created tests for 9 different scenarios to verify structured content takes priority when it should, and falls back correctly when it shouldn't
  • Added specific tests to ensure the core priority logic works as expected and that we didn't break any existing functionality
  • Confirmed that the tests catch the original bug (unreachable code) and validate that our fix actually solves the problem

Issue number

Fixes #1236

Checks

  • I've added new tests (if relevant) - Added comprehensive parameterized tests and individual test functions for all structured content scenarios
  • I've added/updated the relevant documentation - Code comments updated to explain the fix and logic flow
  • I've run make lint and make format - Code follows project formatting standards
  • I've made sure tests pass - All existing tests continue to pass, and new structured content tests validate the fix

@Kunmeer-SyedMohamedHyder Kunmeer-SyedMohamedHyder changed the title use_structured_content given preference when available Fix(mcp): Unreachable structured content branch in invoke_mcp_tool Jul 25, 2025
@seratch seratch requested review from seratch and rm-openai July 26, 2025 01:23
@seratch
Copy link
Member

seratch commented Jul 28, 2025

Could you resolve the lint and typecheck errors?

@Kunmeer-SyedMohamedHyder
Copy link
Contributor Author

Could you resolve the lint and typecheck errors?

Hey @seratch. Fixed them!

image image image

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me; @rm-openai can you do final check before merging it?

@seratch seratch added the bug Something isn't working label Jul 28, 2025
@Kunmeer-SyedMohamedHyder
Copy link
Contributor Author

Kunmeer-SyedMohamedHyder commented Jul 28, 2025

image

Following is the code I ran!

#!/usr/bin/env python3
import asyncio
import json
from typing import Any

from mcp.types import CallToolResult, TextContent, Tool as MCPTool
from agents.run_context import RunContextWrapper
from agents.mcp import MCPServer, MCPUtil


class TestMCPServer(MCPServer):
    def __init__(self, use_structured_content: bool = False):
        super().__init__(use_structured_content=use_structured_content)
        self._server_name = "test_server"
    
    async def cleanup(self) -> None:
        pass
    
    async def connect(self) -> None:
        pass
    
    async def get_prompt(self, name: str, arguments: dict[str, Any] | None = None):
        raise NotImplementedError()
    
    async def list_prompts(self, run_context: RunContextWrapper[Any], agent):
        return []
    
    @property
    def name(self) -> str:
        return self._server_name
    
    async def list_tools(self, run_context: RunContextWrapper[Any], agent) -> list:
        return [MCPTool(name="search_users", description="test", inputSchema={"type": "object"})]
    
    async def call_tool(self, tool_name: str, arguments: dict[str, Any] | None) -> CallToolResult:
        return CallToolResult(
            content=[TextContent(text="Found 2 users", type="text")],
            structuredContent={
                "users": [{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}],
                "total": 2
            }
        )


async def test_structured_content_fix():
    print("Testing MCP Structured Content Fix (Issue #1236)")
    
    run_context = RunContextWrapper(context=None)
    tool = MCPTool(name="search_users", description="test", inputSchema={"type": "object"})
    
    # Test 1: use_structured_content=False (returns text content)
    print("\nTest 1: use_structured_content=False")
    server_text = TestMCPServer(use_structured_content=False)
    result = await MCPUtil.invoke_mcp_tool(server_text, tool, run_context, "{}")
    parsed = json.loads(result)
    print(f"Result: {result}")
    print(f"PASS: Returns text content" if "text" in parsed else "FAIL: Expected text content")
    
    # Test 2: use_structured_content=True (THE FIX - returns structured content)
    print("\nTest 2: use_structured_content=True (THE FIX)")
    print("Before fix: Would return text content (unreachable path)")
    print("After fix: Returns structured content exclusively")
    
    server_structured = TestMCPServer(use_structured_content=True)
    result_structured = await MCPUtil.invoke_mcp_tool(server_structured, tool, run_context, "{}")
    parsed_structured = json.loads(result_structured)
    print(f"Result: {result_structured}")
    
    if "users" in parsed_structured and "text" not in parsed_structured:
        print(f"PASS: Returns structured content exclusively")
        print(f"Found {len(parsed_structured['users'])} users")
        print("FIX CONFIRMED: Previously unreachable code now works!")
    else:
        print("FAIL: Expected structured content or found text mixing")
    
    # Test 3: Fallback when no structured content
    print("\nTest 3: Fallback when no structured content")
    
    class FallbackServer(TestMCPServer):
        async def call_tool(self, tool_name: str, arguments: dict[str, Any] | None):
            return CallToolResult(
                content=[TextContent(text="No structured data", type="text")],
                structuredContent=None
            )
    
    server_fallback = FallbackServer(use_structured_content=True)
    result_fallback = await MCPUtil.invoke_mcp_tool(server_fallback, tool, run_context, "{}")
    parsed_fallback = json.loads(result_fallback)
    print(f"Result: {result_fallback}")
    print("PASS: Fallback to text content works" if parsed_fallback.get("text") == "No structured data" else "FAIL: Fallback behavior not working")


if __name__ == "__main__":
    asyncio.run(test_structured_content_fix())

Did a quick test @seratch!! BTW thanks to GPT! 😄

@seratch
Copy link
Member

seratch commented Jul 29, 2025

@rm-openai can you take a look at this before making next release?

@rm-openai rm-openai merged commit fb68e77 into openai:main Aug 1, 2025
6 checks passed
vcshih pushed a commit to veris-ai/openai-agents-python that referenced this pull request Aug 15, 2025
…penai#1250)

### Summary

This PR handles the MCP tool output where structured content could never
be returned exclusively when `use_structured_content=True`. The
conditional logic checked for content length first, making the
structured content branch unreachable when both content types were
present.

**Before (broken logic):**
```python
if len(result.content) == 1:
    tool_output = result.content[0].model_dump_json()
elif server.use_structured_content and result.structuredContent:  # Never reached!
    tool_output = json.dumps(result.structuredContent)
```

**After (fixed logic):**
```python
if server.use_structured_content and result.structuredContent:
    tool_output = json.dumps(result.structuredContent)
else:
    # Fall back to regular text content processing
    if len(result.content) == 1:
        tool_output = result.content[0].model_dump_json()
```

**Example usage:**
```python
# MCP server with structured content enabled
server = MCPServer(use_structured_content=True)

# When invoke_mcp_tool processes a tool that returns both text and structured content:
# - Text content: "Found 2 users"
# - Structured content: {"users": [{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]}

result = await MCPUtil.invoke_mcp_tool(server, tool, context, input_json)

# Before fix: Would return '{"type":"text","text":"Found 2 users","annotations":null,"meta":null}'
# After fix: Returns '{"users": [{"id": 1, "name": "Alice"}, {"id": 2, "name": "Bob"}]}'
```

### Test plan

I've added thorough test coverage to make sure this fix works properly:
- Created tests for 9 different scenarios to verify structured content
takes priority when it should, and falls back correctly when it
shouldn't
- Added specific tests to ensure the core priority logic works as
expected and that we didn't break any existing functionality
- Confirmed that the tests catch the original bug (unreachable code) and
validate that our fix actually solves the problem

### Issue number

Fixes openai#1236

### Checks

- [x] I've added new tests (if relevant) - Added comprehensive
parameterized tests and individual test functions for all structured
content scenarios
- [x] I've added/updated the relevant documentation - Code comments
updated to explain the fix and logic flow
- [x] I've run `make lint` and `make format` - Code follows project
formatting standards
- [x] I've made sure tests pass - All existing tests continue to pass,
and new structured content tests validate the fix

---------

Co-authored-by: SyedMohamedHyder <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature:mcp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invoke_mcp_tool behavior clarification for use_structured_content
4 participants