-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: drop hosted MCP calls when reasoning is stripped #6210
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 |
|---|---|---|
|
|
@@ -1492,6 +1492,9 @@ def _prepare_message_for_openai( | |
| # marker, since the server-stored items would otherwise duplicate the inline ones. Without | ||
| # storage, standalone reasoning items are invalid per the API ("reasoning was provided | ||
| # without its required following item"), so the reasoning branch always drops. | ||
| drops_reasoning_without_storage = not request_uses_service_side_storage and any( | ||
| content.type == "text_reasoning" for content in message.contents | ||
| ) | ||
|
Comment on lines
+1495
to
+1497
|
||
| for content in message.contents: | ||
| match content.type: | ||
| case "text_reasoning": | ||
|
|
@@ -1546,7 +1549,10 @@ def _prepare_message_for_openai( | |
| # server-side `id`, so under continuation it would duplicate | ||
| # the prior response's items (#3295). Drop the call here; the | ||
| # orphan result is dropped by the coalesce step that follows. | ||
| if request_uses_service_side_storage: | ||
| # | ||
| # Without storage, a reasoning + hosted-MCP pair cannot be replayed | ||
| # partially: reasoning is stripped above, and a bare mcp_call is rejected. | ||
| if request_uses_service_side_storage or drops_reasoning_without_storage: | ||
|
Comment on lines
+1552
to
+1555
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. Should the same drop apply to |
||
| continue | ||
| prepared_mcp = self._prepare_content_for_openai( | ||
| message.role, | ||
|
|
||
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.
Flag is message-scoped, so it only fires when reasoning and the mcp_call share one Message. For responses parsed by this client that always holds (streaming updates coalesce into one assistant Message, non-streaming builds one), so live traffic is fine. But if a replayed or checkpointed history ever splits a turn so reasoning sits in a separate message, this is False for the mcp_call's message and the bare call survives, the exact state the API rejects. Worth a no-storage test with reasoning and the call in separate messages to pin the contract, or a note that the split can't happen?