-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: fix: preserve reasoning with hosted MCP calls #6139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5603,6 +5603,42 @@ def test_prepare_messages_for_openai_serializes_mcp_server_tool_call_as_mcp_call | |
| assert "output" not in item or item["output"] is None | ||
|
|
||
|
|
||
| def test_prepare_messages_for_openai_keeps_reasoning_with_mcp_call_without_storage() -> None: | ||
| """Hosted MCP calls returned by reasoning models need their paired reasoning item.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| messages = [ | ||
| Message( | ||
| role="assistant", | ||
| contents=[ | ||
| Content.from_text_reasoning( | ||
| id="rs_abc123", | ||
| text="I need to query the hosted MCP server.", | ||
| additional_properties={"status": "completed"}, | ||
| ), | ||
| Content.from_mcp_server_tool_call( | ||
| call_id="mcp_abc123", | ||
| tool_name="search", | ||
| server_name="api_specs", | ||
| arguments='{"q": "cats"}', | ||
| ), | ||
| ], | ||
| ), | ||
| ] | ||
|
|
||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we covering the orderings where reasoning isn't adjacent to the mcp_call? This pins only |
||
| types = [item.get("type") for item in storage_off] | ||
| assert types == ["reasoning", "mcp_call"] | ||
|
|
||
| reasoning = storage_off[0] | ||
| assert reasoning["id"] == "rs_abc123" | ||
| assert reasoning["status"] == "completed" | ||
| assert reasoning["summary"] == [{"type": "summary_text", "text": "I need to query the hosted MCP server."}] | ||
|
|
||
|
|
||
| def test_prepare_messages_for_openai_coalesces_mcp_call_and_result_into_single_item() -> None: | ||
| """An mcp_server_tool_call followed by an mcp_server_tool_result with the | ||
| same call_id (in same or separate Messages) must produce ONE mcp_call | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should retention be per-pair rather than per-message?
has_hosted_mcp_callkeeps 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".