[ResponsesAPI] Add GPTOSS MCP tool streaming #30301
Conversation
Signed-off-by: Daniel Salib <danielsalib@meta.com>
Signed-off-by: Daniel Salib <danielsalib@meta.com>
qandrew
requested review from
DarkLight1337,
NickLucche,
aarnphm,
chaunceyjiang and
robertgshaw2-redhat
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for streaming events related to Multi-Channel Protocol (MCP) tools within the OpenAI Responses API. The changes include adding new MCP-specific event types and modifying the Harmony streaming event processing to correctly handle and differentiate between traditional function calls, legacy code interpreter calls, and the new MCP tool calls. New tests were added to verify the streaming event sequence for MCP tool calls and to ensure multi-turn conversations with MCP tools function as expected. Review comments highlight issues in the new tests, specifically that assertions for event types are too broad or incorrect for MCP events, and question the logic for handling delta events in streaming. Additionally, a change in message processing for multi-turn conversations, which now includes 'analysis' channel messages, requires confirmation of its intended behavioral impact.
|
|
||
| stack_of_event_types = [] | ||
| async for event in stream_response: | ||
| assert "mcp_call" in event.type |
There was a problem hiding this comment.
This assertion assert "mcp_call" in event.type is too broad. Not all event types in the stream will contain "mcp_call" (e.g., response.created, response.completed, response.reasoning_text.delta). This assertion will likely cause the test to fail for valid non-MCP events. Consider making this assertion more specific to actual MCP-related event types or removing it if it's not strictly necessary for all events.
| elif event.type.endswith("delta"): | ||
| if stack_of_event_types[-1] == event.type: | ||
| continue | ||
| stack_of_event_types.append(event.type) |
There was a problem hiding this comment.
The logic for handling delta events might be flawed. If multiple delta events of the same type occur consecutively, only the first one is pushed to the stack, and subsequent ones are skipped (continue). However, pairs_of_event_types maps delta to done events, implying that each done event should correspond to a delta event. If intermediate delta events are skipped, the final done event might pop an incorrect delta event from the stack, leading to an assertion failure or an incorrect len(stack_of_event_types) == 0 at the end. Consider pushing all delta events or refining the stack logic to correctly match done events with their corresponding delta streams.
| code_interpreter_events_seen = False | ||
|
|
||
| async for event in stream_response: | ||
| if "code_interpreter" in event.type: |
There was a problem hiding this comment.
The assertion assert "code_interpreter" in event.type is incorrect. The event types for MCP tools are like response.mcp_call.in_progress, response.mcp_call_arguments.delta, etc., which do not contain "code_interpreter". This assertion will always be false for the new MCP event types.
| assert not code_interpreter_events_seen, ( | ||
| "Should not see code_interpreter events when using MCP type" | ||
| ) |
There was a problem hiding this comment.
Given the previous assertion on line 891, this assertion assert not code_interpreter_events_seen seems contradictory or indicates a misunderstanding of the event types. If the intention is to ensure that legacy code_interpreter events are not seen when using the new mcp type, the check on line 891 should be removed or modified to specifically look for legacy event types, and code_interpreter_events_seen should only be set if a legacy event is encountered. As it stands, the test logic is confusing.
| for msg in recent_turn_msgs: | ||
| assert isinstance(msg, OpenAIHarmonyMessage) | ||
| if msg.channel != "analysis": | ||
| prev_msgs.append(msg) | ||
| prev_msgs.append(msg) |
There was a problem hiding this comment.
The removal of the if msg.channel != "analysis": condition means that messages on the "analysis" channel are now included in prev_msgs for subsequent turns. Previously, these messages were filtered out. If "analysis" channel messages are internal reasoning or tool outputs that should not directly influence the model's next turn, this change could lead to unintended behavioral shifts in multi-turn conversations. Please confirm if this is the desired behavior and if including "analysis" messages in the conversational context is appropriate.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not sent_output_item_added: | ||
| sent_output_item_added = True | ||
| current_item_id = f"tool_{random_uuid()}" | ||
| yield _increment_sequence_number_and_return( | ||
| ResponseOutputItemAddedEvent( |
There was a problem hiding this comment.
Function tool calls now satisfy the broad commentary/analysis branch and fall through to the non-MCP path, which always yields code_interpreter_call delta/progress events even when the recipient namespace is functions.*. Because _is_mcp_tool_by_namespace returns False for functions.*, streaming a normal function call will generate an in-progress code interpreter item before the actual function-call events are emitted, causing clients to see the wrong tool type and duplicated tool call completions for every function invocation.
Useful? React with 👍 / 👎.
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
1 similar comment
|
Hi @qandrew, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
qandrew
force-pushed
the
pr3-mcp-streaming
branch
from
12b8473 to
2aa50bd
Compare
2 participants
Purpose
This change enables streaming support for MCP tools when using GPT OSS. It extends the harmony utilities and response serving infrastructure to handle tool streaming, allowing tool calls and their results to be incrementally streamed back to clients rather than returned as a single batch.
taken over from #30192, builds on top of #30054
Test Plan
Test Result