fix(openai-router): suppress duplicate output-item envelopes on native passthrough (R6.7c) - #1376
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new StreamAction::Drop and extends StreamingToolHandler to track hosted/native passthrough tool-call output indices; mis-ordered upstream Changes
Sequence Diagram(s)sequenceDiagram
participant Upstream as Upstream Response
participant Handler as StreamingToolHandler
participant MCP as MCP-Dispatch
participant Router as Streaming Router
participant Client as Client
Upstream->>Handler: output_item.added (hosted/native passthrough, index N)
Handler->>Handler: record index N in native_passthrough set
Upstream->>Handler: output_item.done (index N)
Handler->>MCP: check pending_calls for index N
alt pending_calls contains N
Handler->>Router: StreamAction::ExecuteTools { forward_triggering_event: false }
Router->>MCP: trigger tool execution (do not forward done)
else native_passthrough observed
Handler->>Router: StreamAction::Drop
Router->>Router: suppress forwarding (no dispatch)
else normal event
Handler->>Router: Forward event
Router->>Client: send output_item.done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
…ssthrough (R6.7c)
R6.7b tightened the streaming `output_item.done` suppression gate with a
`belongs_to_pending_call` guard so hosted-tool items that the handler
never registered could not have their umbrella dropped permanently
(the tool loop only re-emits umbrellas for items it executed). That
guard is correct for MCP-dispatched calls but silently regresses the
OpenAI cloud "native passthrough" case, where upstream emits a hosted
tool-call item (`image_generation_call`, `web_search_call`,
`code_interpreter_call`, `file_search_call`) end-to-end without our
router wrapping it as `function_call`. Because
`handle_output_item_added` only registers function-call types in
`pending_calls`, `belongs_to_pending_call` is false and the upstream
`output_item.done` — which arrives BEFORE `response.<type>.completed`
on the cloud path — leaked to the wire mis-ordered. That is the gap
R6.5 integration tests surface in the cloud streaming matrix.
Fix: extend the gate with an OR-condition so suppression fires when
EITHER (a) the item is a tracked MCP-dispatched call (R6.7b — kicks
the tool loop via `ExecuteTools { forward_triggering_event: false }`)
OR (b) the item is a known hosted tool-call kind that upstream
announced via `output_item.added` but the MCP-dispatch registry did
NOT capture (R6.7c native passthrough — drops the envelope via a new
`StreamAction::Drop` without running the tool loop). `Drop` is a
distinct variant rather than a reuse of `Buffer` to keep the caller
contract explicit: consume the event, do not forward, do not dispatch.
Implementation is purely state-driven on the handler — no new boolean
threads through from the caller. A new
`native_passthrough_tool_call_indices: HashSet<usize>` records every
output_index whose `output_item.added` carried
`is_tool_call_item_type(ty) && !is_function_call_type(ty)`. The
`output_item.done` gate checks this set alongside the existing
`belongs_to_pending_call` check. The R6.7b MCP-dispatch arm is
unchanged; the new R6.7c arm fires only when the passthrough
`output_item.added` was actually observed, so spurious
`output_item.done` events with no preceding `output_item.added` still
forward rather than being silently swallowed.
Files changed:
- model_gateway/src/routers/openai/mcp/tool_handler.rs: add
`native_passthrough_tool_call_indices` field, record indices in
`handle_output_item_added` for non-function hosted tool-call types,
extend the `OutputItemEvent::DONE` gate with the new R6.7c arm,
add `StreamAction::Drop` variant. New tests cover all four hosted
tool-call types, coexistence with MCP-dispatch on a mixed stream,
dropping while a function_call is in-flight at another index, and
that the `output_item.added` itself still forwards.
- model_gateway/src/routers/openai/responses/streaming.rs: handle
`StreamAction::Drop` in the streaming loop (drop without forwarding
and without setting `tool_calls_detected`).
Refs: R6.5, R6.7, R6.7b
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
model-gateway
a5bb21c to
ced35ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/openai/mcp/tool_handler.rs`:
- Around line 297-313: The native passthrough index set
(native_passthrough_tool_call_indices) is never cleared, so subsequent envelopes
still match is_native_passthrough_item and get StreamAction::Drop, preventing
the later output_item.done from reaching the client; fix by removing the index
from native_passthrough_tool_call_indices as soon as you take the Drop path
(i.e., in the branch that yields StreamAction::Drop where is_tool_call_done &&
is_native_passthrough_item), ensuring you identify and remove the specific
done_output_index (or its usize) from native_passthrough_tool_call_indices
before returning Drop so the later correctly ordered output_item.done will not
be dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 637c34d9-ff65-48e7-8397-68dbeef07344
📒 Files selected for processing (2)
model_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/responses/streaming.rs
![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.
Reviewed commit: a5bb21c67b
ℹ️ 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".
| } else if is_tool_call_done && is_native_passthrough_item { | ||
| // Native passthrough path (R6.7c): drop the upstream | ||
| // umbrella without running the tool loop. The | ||
| // downstream `<type>.completed` event still reaches | ||
| // the client, and upstream re-emits the umbrella in | ||
| // the correct position later in the stream. | ||
| StreamAction::Drop |
There was a problem hiding this comment.
This branch drops every response.output_item.done for a native-passthrough output_index, but the index is only ever inserted (handle_output_item_added) and never removed. In streams where upstream sends an early misordered umbrella and then a later correctly ordered umbrella for the same item (the behavior described in the surrounding comments), the second/final output_item.done is dropped too, so clients never receive a terminal umbrella for that tool item. This breaks the stated ordering contract rather than just suppressing the duplicate early envelope.
Useful? React with 👍 / 👎.
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
model_gateway/src/routers/openai/mcp/tool_handler.rs (1)
296-312:⚠️ Potential issue | 🔴 CriticalNative passthrough index must be removed after drop to allow the correctly-ordered umbrella through.
The
native_passthrough_tool_call_indicesset is only ever inserted into (line 347) and checked withcontains()(line 297), but the index is never removed. According to the docstring (lines 55-56), upstream re-emits a correctly-orderedoutput_item.donelater in the stream. With the current implementation, that second envelope will also matchis_native_passthrough_itemand getStreamAction::Drop, causing the client to never see anyoutput_item.donefor the native-passthrough item.🐛 Proposed fix: remove the index when taking the Drop path
- let is_native_passthrough_item = done_output_index - .is_some_and(|idx| self.native_passthrough_tool_call_indices.contains(&idx)); if is_tool_call_done && belongs_to_pending_call && self.has_complete_calls() { // MCP-dispatch path (R6.7b): suppress the upstream // umbrella event and kick the tool loop — it will emit // its own `output_item.done` at the correct position, // AFTER `response.<type>.completed`. StreamAction::ExecuteTools { forward_triggering_event: false, } - } else if is_tool_call_done && is_native_passthrough_item { + } else if is_tool_call_done + && done_output_index.is_some_and(|idx| { + self.native_passthrough_tool_call_indices.remove(&idx) + }) + { // Native passthrough path (R6.7c): drop the upstream // umbrella without running the tool loop. The // downstream `<type>.completed` event still reaches // the client, and upstream re-emits the umbrella in // the correct position later in the stream. StreamAction::Drop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/mcp/tool_handler.rs` around lines 296 - 312, The native_passthrough index is never removed so the later re-emitted umbrella will also be dropped; in the branch that returns StreamAction::Drop (where is_native_passthrough_item is true), remove the done_output_index from self.native_passthrough_tool_call_indices before returning (i.e. if done_output_index.is_some(), call self.native_passthrough_tool_call_indices.remove(&idx)). This ensures the downstream re-emitted output_item.done is not matched and can pass through.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/openai/mcp/tool_handler.rs`:
- Around line 863-885: Add a test that verifies a mis-ordered first
output_item.done is dropped but a subsequent correctly-ordered output_item.done
is forwarded: instantiate StreamingToolHandler::with_starting_index(0), call
bootstrap_native_hosted_tool_added(...) for an ItemType (e.g.,
ItemType::IMAGE_GENERATION_CALL) with index 0, call
handler.process_event(Some("response.output_item.done"), &first_done) and assert
it yields StreamAction::Drop, then call
handler.process_event(Some("response.output_item.done"), &second_done) with the
identical payload and assert it yields StreamAction::Forward; this will ensure
native_passthrough_tool_call_indices is cleared so the second done is forwarded.
---
Duplicate comments:
In `@model_gateway/src/routers/openai/mcp/tool_handler.rs`:
- Around line 296-312: The native_passthrough index is never removed so the
later re-emitted umbrella will also be dropped; in the branch that returns
StreamAction::Drop (where is_native_passthrough_item is true), remove the
done_output_index from self.native_passthrough_tool_call_indices before
returning (i.e. if done_output_index.is_some(), call
self.native_passthrough_tool_call_indices.remove(&idx)). This ensures the
downstream re-emitted output_item.done is not matched and can pass through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cc60339-ad11-4ce9-bf02-3d6b5c837659
📒 Files selected for processing (2)
model_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/responses/streaming.rs
…umbrella forwards Addresses CodeRabbit and chatgpt-codex-connector P1 feedback on PR #1376. The initial R6.7c implementation only ever inserted into `native_passthrough_tool_call_indices` and checked membership with `contains()`, so every subsequent `output_item.done` at the same output_index was also dropped. That regresses the spec invariant the PR is meant to fix: after upstream emits the correctly-ordered umbrella (after `response.<type>.completed`) the client must see it as the terminal event for the item. With the unbounded membership check the second umbrella also hit the `Drop` arm and the client ended up seeing no terminal `output_item.done` at all. Fix: change the passthrough-match condition from `contains(&idx)` to `remove(&idx)`. The first `output_item.done` for the item atomically removes the index and returns true (Drop fires). The second `output_item.done` finds the index already gone, so `remove` returns false, the R6.7c arm declines, and the envelope forwards through to the client. Added `native_passthrough_second_output_item_done_forwards_after_first_dropped` test to lock the one-shot behaviour: feeds an `output_item.added` plus two identical `output_item.done` events and asserts `Drop` then `Forward`. This test fails against the `contains` form of the gate and passes with the `remove` form. Refs: #1376 review, R6.7c Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3dbc5a474
ℹ️ 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".
| && done_output_index.is_some_and(|idx| { | ||
| self.native_passthrough_tool_call_indices.remove(&idx) | ||
| }) |
There was a problem hiding this comment.
This remove-based gate drops the first native-passthrough response.output_item.done and immediately forgets the index, which assumes the later correctly ordered umbrella will definitely be observed in the same streaming pass. In mixed streams (the coexistence case already modeled in native_passthrough_gate_coexists_with_mcp_dispatch_gate), if a different MCP-dispatched call hits ExecuteTools first, the streaming loop exits to run tools before that later passthrough umbrella is read, so the client can end up seeing no terminal output_item.done for the hosted item at all.
Useful? React with 👍 / 👎.
The `unit-tests` CI job runs `cargo +nightly fmt -- --check`; the R6.7c commits used more generous wrapping than rustfmt's nightly defaults chose. Let rustfmt own the formatting so CI passes — no semantic changes. Refs: #1376 CI Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e08f1443b5
ℹ️ 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".
| } else if is_tool_call_done | ||
| && done_output_index | ||
| .is_some_and(|idx| self.native_passthrough_tool_call_indices.remove(&idx)) | ||
| { |
There was a problem hiding this comment.
This branch drops the first hosted-tool response.output_item.done whenever the index was previously seen in output_item.added, but it does not verify that a replacement terminal umbrella will arrive later. If upstream emits only a single (already correctly ordered) output_item.done for that item, StreamAction::Drop removes the only completion envelope and handle_streaming_with_tool_interception never forwards one, so clients can miss the terminal event for the tool item. The drop condition should depend on observing premature ordering (or otherwise guarantee a later done) rather than unconditionally consuming the first done.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
R6.7b (PR #1371) tightened the streaming
output_item.donesuppression gate in the OpenAI responses router with abelongs_to_pending_callguard so hosted-tool items the handler never registered could not have their umbrella silently dropped (the tool loop only re-emits umbrellas for items it actually dispatched).That guard is correct for the MCP-dispatch path, but it silently regresses the OpenAI cloud "native passthrough" case — where upstream emits a hosted tool-call item (
image_generation_call,web_search_call,code_interpreter_call,file_search_call) end-to-end without our router wrapping it as afunction_call. Becausehandle_output_item_addedonly registersfunction_call/function_tool_callitems inpending_calls,belongs_to_pending_callis always false for native passthrough, and the mis-ordered upstreamoutput_item.done(which the cloud emits BEFOREresponse.<type>.completed) leaked to the wire, breaking the spec invariant thatoutput_item.donemust be the LAST event for a given item.R6.5 integration tests in the cloud streaming matrix surface this gap as an ordering-assertion failure for the native passthrough scenarios.
Solution
Extend the suppression gate with an OR-condition so it fires under EITHER of two conditions:
(a) MCP-dispatched call (R6.7b, unchanged) — the done event's
output_indexmatches a pending call tracked inpending_calls, has a complete name, and the item type is a known tool-call kind. ReturnsStreamAction::ExecuteTools { forward_triggering_event: false }so the tool loop kicks in and emits its own correctly-ordered umbrella.(b) Native passthrough (R6.7c, new) — the done event's
output_indexwas recorded in a newnative_passthrough_tool_call_indices: HashSet<usize>when upstream emittedoutput_item.addedfor a non-function hosted tool-call type, and the item type is still a known tool-call kind. Returns a newStreamAction::Dropso the caller drops the envelope without running the tool loop.The implementation is purely state-driven on the handler itself — no caller-side
native_passthrough: boolneeds to be threaded through. The R6.7b MCP-dispatch arm is completely untouched. The new R6.7c arm only fires when the passthroughoutput_item.addedwas actually observed first, so spuriousoutput_item.doneevents still forward rather than being silently swallowed.A new
StreamAction::Dropvariant is introduced rather than reusingBuffer, so the caller contract is explicit: consume the event, do not forward, do not dispatch.Changes
model_gateway/src/routers/openai/mcp/tool_handler.rsnative_passthrough_tool_call_indices: HashSet<usize>field toStreamingToolHandler.handle_output_item_added, whenis_tool_call_item_type(ty) && !is_function_call_type(ty), record the output_index in that set.OutputItemEvent::DONEgate with the new R6.7c arm that returnsStreamAction::Dropwhenis_tool_call_done && is_native_passthrough_item.StreamAction::Dropvariant with a documentation comment explaining the native passthrough semantics.output_item.addedwas NEVER observed (rename:output_item_done_for_unobserved_hosted_tool_output_index_forwards).output_item_done_dropped_for_native_passthrough_{image_generation,web_search,code_interpreter,file_search}_call— one per hosted tool-call type.native_passthrough_gate_coexists_with_mcp_dispatch_gate— mixed stream locks that the MCP-dispatch arm and the native-passthrough arm fire on the correct items (function_call → ExecuteTools; hosted-tool → Drop).native_passthrough_done_drops_even_when_pending_function_call_complete— verifies the R6.7c arm fires independently ofhas_complete_calls().output_item_added_for_native_passthrough_still_forwards— verifies theoutput_item.addeditself is not accidentally swallowed.model_gateway/src/routers/openai/responses/streaming.rsStreamAction::Dropinhandle_streaming_with_tool_interception: drop the event without forwarding and without settingtool_calls_detected(the tool loop does not run for passthrough items).Test Plan
cargo check -p smg— passes.cargo clippy -p smg --all-targets -- -D warnings— passes with no warnings.cargo test -p smg --lib 'routers::openai::mcp::tool_handler'— 16 tests pass (11 pre-existing + 5 new R6.7c + renamed R6.7b test).cargo test -p smg --lib 'routers::openai'— 57 tests pass across the OpenAI router module; no regressions.Notes / follow-ups
model_gateway/src/routers/openaiwas touched. Protocol types,crates/mcp,crates/protocols, harmony/gRPC routers, andBUILTIN_TOOLSremain untouched.Refs: R6.5, R6.7, R6.7b (#1371)
Summary by CodeRabbit
Bug Fixes
Tests