Python: fix: preserve reasoning with hosted MCP calls#6139
Conversation
|
@he-yufeng You currently have 38 open PRs. As maintainers, this is a lot for us to keep on top of. Can you please hold off on opening new PRs until you've gotten some of the existing ones merged (comments addressed, checks passing)? Thank you. |
db8a064 to
e1ac412
Compare
|
Thanks, understood. I closed six superseded Agent Framework PRs to reduce the queue: #5945, #6033, #6128, #6110, #5809, and #6011. I’ll avoid opening new PRs in this repo while the current queue is being reviewed, and will focus on keeping existing PRs deduplicated, rebased, and with checks/comments addressed. |
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
| # without its required following item"), so the reasoning branch always drops. | ||
| # without its required following item"). Hosted MCP calls are the exception: if we keep | ||
| # the mcp_call item without storage, keep its paired reasoning item too. | ||
| has_hosted_mcp_call = any(content.type == "mcp_server_tool_call" for content in message.contents) |
There was a problem hiding this comment.
Should retention be per-pair rather than per-message? has_hosted_mcp_call keeps every text_reasoning in the message as soon as any mcp_call is present, but the API needs each reasoning item immediately followed by its required item. Shapes like [reasoning, mcp_call, reasoning] (trailing reasoning) or [reasoning_1, reasoning_2, mcp_call] emit a reasoning item not immediately followed by a valid one -> the exact "reasoning was provided without its required following item" this fix targets. coalesce doesn't reorder, and plain text is appended last, so neither rescues these. Wondering if the gate should check the next surviving content is the mcp_call, not just "mcp_call somewhere in the message".
| storage_on = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=True) | ||
| assert "reasoning" not in [item.get("type") for item in storage_on] | ||
|
|
||
| storage_off = client._prepare_messages_for_openai(messages, request_uses_service_side_storage=False) |
There was a problem hiding this comment.
Are we covering the orderings where reasoning isn't adjacent to the mcp_call? This pins only [reasoning, mcp_call], the one case least likely to break. Could we add [reasoning_1, reasoning_2, mcp_call] (all kept, in order) and a non-adjacent / trailing-reasoning shape, since that's where the message-level gate could emit an orphan reasoning item?
Summary
text_reasoningitems when a storage-off replay also keeps a hosted MCPmcp_callCloses #6074.
To verify
PYTHONPATH=python\packages\core;python\packages\openai python -m pytest python\packages\openai\tests\openai\test_openai_chat_client.py -q -k "keeps_reasoning_with_mcp_call or serializes_mcp_server_tool_call or coalesces_mcp_call or strips_mcp_items" --basetemp .tmp\pytest-6074-2PYTHONPATH=python\packages\core;python\packages\openai python -m pytest python\packages\openai\tests\openai\test_openai_chat_client.py -q -k "prepare_messages_for_openai" --basetemp .tmp\pytest-6074-preparepython -m py_compile python\packages\openai\agent_framework_openai\_chat_client.py python\packages\openai\tests\openai\test_openai_chat_client.pypython -m ruff check python\packages\openai\agent_framework_openai\_chat_client.py python\packages\openai\tests\openai\test_openai_chat_client.pygit diff --check