Conversation
… policy Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
…ity logic Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughCentralizes MCP visibility and interception: adds McpToolSession predicates, threads user-function name context through routers/handlers, applies session-driven visibility filtering across streaming and non‑streaming paths, refactors mcp_list_tools emission/deduplication, and introduces per-output visibility state in OpenAI MCP streaming. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as Router (gRPC/OpenAI)
participant Session as McpToolSession
participant Handler as Tool Handler
participant Emitter as Event Emitter
Client->>Router: Send request with tool calls
Router->>Router: collect_user_function_names
Router->>Session: should_intercept_function_call(tool_name, user_function_names)?
alt intercept (MCP)
Session-->>Router: true
Router->>Session: should_hide_output_item_json?(item)
alt hidden
Session-->>Router: true
Router->>Emitter: skip emission / redact
else visible
Session-->>Router: false
Router->>Handler: resolve visibility, allocate indices
Handler->>Emitter: emit output_item_added / progress events
Emitter->>Client: stream events
end
else not intercepted (user function)
Session-->>Router: false
Router->>Handler: route to user function
end
Router->>Session: should_emit_streaming_mcp_list_tools?(server_label)
alt emit
Session-->>Router: true
Router->>Emitter: emit_visible_mcp_list_tools_sequence
Emitter->>Client: mcp_list_tools events
else skip
Session-->>Router: false
end
Router->>Router: apply retain_client_visible_* to final response
Router->>Client: send filtered final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 implements comprehensive visibility filtering and redaction for MCP (Model Context Protocol) tools across both gRPC and OpenAI-compatible endpoints, covering streaming and non-streaming paths. It introduces logic to distinguish between internal MCP tools, which are hidden from clients, and user-defined or builtin tools. Key changes include the addition of visibility tracking in the StreamingToolHandler, utility functions for filtering response outputs, and logic to handle mixed-mode turns where both MCP and user-defined tools are present. The review feedback identifies critical edge cases where executed MCP results could be lost or duplicated in final responses, specifically when reaching tool limits or transitioning between MCP and user-driven iterations.
![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: 8e1386d5b4
ℹ️ 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".
github-actions
Bot
added
documentation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough review of all 17 changed files across the MCP visibility/interception overhaul. The changes are consistent across all four response paths (OpenAI streaming/non-streaming, gRPC regular, gRPC harmony) and the new should_intercept_function_call / should_emit_streaming_mcp_list_tools / OutputVisibilityState abstractions are well-designed.
Summary: 0 🔴 Important, 1 🟡 Nit, 0 🟣 Pre-existing
Key things verified:
- Mixed MCP + user-function partitioning logic is correct across all paths (the old
has_exposed_tool→ newshould_intercept_function_callgives user-defined functions proper precedence on name collisions) - Streaming visibility probe items are constructed with the fields
should_hide_output_item_jsonactually inspects mcp_list_toolsdedup key change from label-only to label+tools is backwards-compatible with stored conversation chains (extraction runs against stored output items at load time)restore_original_toolsnow receives the session for proper redaction in passthrough/mixed modeOutputVisibilityStatestate machine correctly prevents hidden→visible transitions and logs a warning on the reverse- Test coverage for the new visibility state machine, mixed-mode reconciliation, and hidden-event dropping
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs (1)
291-315:⚠️ Potential issue | 🟠 MajorFilter
response.outputbefore returning mixed MCP/function results.This mixed path builds executed MCP calls as
FunctionToolCalloutput items, then only filtersresponse.tools. Internal MCP call names/results can still leak throughresponse.output; the max-tool path already applies the missing output filter.🛡️ Proposed fix
if mcp_tracking.total_calls() > 0 { inject_mcp_metadata( &mut response, &mcp_tracking, &session, &user_function_names, ); } + retain_client_visible_output_items( + &mut response.output, + &session, + &user_function_names, + ); retain_client_visible_response_tools( &mut response, &session, &user_function_names, );Based on learnings, mixed function+MCP paths can place internal MCP
FunctionToolCallitems directly inresponse.output, so pre-existing output items must be filtered before returning client-visible responses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs` around lines 291 - 315, The mixed MCP+function result path must also filter response.output to avoid leaking internal MCP FunctionToolCall items: after building the response (after build_tool_response and after inject_mcp_metadata if present) iterate and retain only client-visible output items, removing any FunctionToolCall entries whose tool name is not in user_function_names or is an internal MCP tool (use the same predicate/visibility logic as retain_client_visible_response_tools); implement this by applying that predicate to response.output (e.g., response.output.retain(|item| is_client_visible(item, &session, &user_function_names))) before returning the response.model_gateway/src/routers/grpc/regular/responses/non_streaming.rs (1)
275-302:⚠️ Potential issue | 🔴 CriticalDon’t skip MCP output reconciliation on the user-function-only exit.
If a prior iteration executed MCP calls and this iteration returns only user function calls, this early return drops
state.mcp_call_itemsfrom the final response and persisted output. Inject the accumulated MCP items before returning.🐛 Proposed fix
})?; + if state.total_calls > 0 { + let tool_items = std::mem::take(&mut state.mcp_call_items); + session.inject_client_visible_mcp_output_items( + &mut responses_response.output, + tool_items, + &user_function_names, + ); + } retain_client_visible_response_tools( &mut responses_response, &session,Based on learnings,
inject_client_visible_mcp_output_itemsmust apply the visibility policy to both injected MCP items and pre-existing output in mixed function+MCP early-exit paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/responses/non_streaming.rs` around lines 275 - 302, Early-return when mcp_tool_calls.is_empty() drops previously accumulated MCP items (state.mcp_call_items); before returning the converted ResponsesResponse from conversions::chat_to_responses and after calling retain_client_visible_response_tools, call inject_client_visible_mcp_output_items to inject and reconcile state.mcp_call_items into the responses_response (ensuring the visibility policy is applied to both injected MCP items and any pre-existing output items) so the final returned/persisted response includes MCP outputs from prior iterations.model_gateway/src/routers/grpc/regular/responses/streaming.rs (1)
591-610:⚠️ Potential issue | 🔴 CriticalMove tool-call visibility before streaming chunk emission.
convert_and_accumulate_streamemits chat tool-call chunks throughemitter.process_chunkbefore this partition runs, so hidden MCP calls can already leak asfunction_callevents, and user function calls can be emitted again later. The converter needs the session/user-function context and must buffer or suppress tool-call deltas until visibility is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs` around lines 591 - 610, The converter currently emits tool-call chunks inside convert_and_accumulate_stream (via emitter.process_chunk) before visibility is determined, causing MCP/function calls to leak; change the flow so tool-call deltas are buffered and not emitted until visibility is known: have convert_and_accumulate_stream either (A) accept session and user_function_names and suppress/buffer any tool-call deltas (do not call emitter.process_chunk for those) while accumulating, or (B) split into a first pass that accumulates the raw streamed events (without emitting) then call extract_all_tool_calls_from_chat and session.should_intercept_function_call to partition into mcp_tool_calls vs function_tool_calls, and only afterwards emit non-tool chunks and the appropriate visible tool-call events via emitter.process_chunk and tx. Ensure references: convert_and_accumulate_stream, extract_all_tool_calls_from_chat, session.should_intercept_function_call, and emitter.process_chunk are updated so tool-call emission is conditional on the partition result.model_gateway/src/routers/openai/mcp/tool_loop.rs (1)
1105-1153:⚠️ Potential issue | 🟠 MajorDrop original MCP
function_callitems in incomplete responses.This path prefixes incomplete/executed MCP items but leaves the original intercepted MCP
function_callplaceholders inoutput_array. That can duplicate visible MCP calls and leak hidden/internal placeholders whenmax_tool_callsis exceeded.Proposed fix
// Convert only MCP-intercepted function_call items in output to mcp_call format. if let Some(output_array) = obj.get_mut("output").and_then(|v| v.as_array_mut()) { // Find any function_call items and convert them to mcp_call (incomplete) let mut incomplete_items = Vec::new(); - for item in output_array.iter() { + let mut retained_items = Vec::with_capacity(output_array.len()); + for item in output_array.drain(..) { let item_type = item.get("type").and_then(|t| t.as_str()); if item_type.is_some_and(is_function_call_type) { let tool_name = item.get("name").and_then(|v| v.as_str()).unwrap_or(""); if !session.should_intercept_function_call(tool_name, &user_function_names) { // Non-intercepted calls (user tools, unknown names, collisions) must remain // function_call output items. - continue; + retained_items.push(item); + continue; } let args = item .get("arguments") .and_then(|v| v.as_str()) .unwrap_or("{}"); @@ ); incomplete_items.push(mcp_call_item); + continue; } + + retained_items.push(item); } // Add mcp_list_tools and executed mcp_call items at the beginning if state.total_calls > 0 || !incomplete_items.is_empty() { let mut prefix = visible_mcp_list_tools_items(session, &list_tools_bindings); @@ prefix.extend( incomplete_items.into_iter().filter(|item| { !session.should_hide_output_item_json(item, &user_function_names) }), ); - output_array.splice(0..0, prefix); + prefix.extend(retained_items); + *output_array = prefix; + } else { + *output_array = retained_items; } }🤖 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_loop.rs` around lines 1105 - 1153, The current code collects incomplete MCP mcp_call items but leaves the original intercepted `function_call` entries in `output_array`, causing duplicates/leaks; change the logic in the block that iterates `output_array` so that when you detect an intercepted function call (using `is_function_call_type` + `session.should_intercept_function_call`) you record that entry for removal (e.g., collect its index or mark it) instead of only pushing a new `mcp_call_item` into `incomplete_items`, then after building `prefix` remove or replace those original entries from `output_array` (or reconstruct `output_array` without them) before splicing in `prefix`; keep the same helpers (`build_mcp_call_item`, `visible_mcp_list_tools_items`, `state.mcp_call_items`, `session.should_hide_output_item_json`) so hidden/internal placeholders are not left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/mcp-internal-servers.md`:
- Around line 19-23: The doc incorrectly states that `internal: true` applies
only to servers declared under `servers:`; update the wording to clarify that
the session's internal-server set also includes statically registered internal
servers (e.g., those in `static_servers`) so streaming and final redaction
covers both declared (`config.servers`) and static registrations; reference
`McpOrchestrator::internal_server_names()` behavior when explaining that
internal server lists are derived from both `static_servers` and
`config.servers`.
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 786-795: retained_output_items currently only hides items via
session.should_hide_output_item_json but still returns MCP-intercepted
function_call placeholders; update retained_output_items to also filter out any
output objects that represent function calls intended for MCP interception by
excluding items where item is an object with "type" == "function_call" and the
function "name" is an MCP placeholder (e.g. starts_with "mcp_" or is in a
session-tracked intercepted set). Use the existing session reference (e.g. call
a new helper like session.is_intercepted_mcp_function(name) if needed) alongside
session.should_hide_output_item_json and user_function_names so the function
returns only visible user/non-MCP calls.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs`:
- Around line 291-315: The mixed MCP+function result path must also filter
response.output to avoid leaking internal MCP FunctionToolCall items: after
building the response (after build_tool_response and after inject_mcp_metadata
if present) iterate and retain only client-visible output items, removing any
FunctionToolCall entries whose tool name is not in user_function_names or is an
internal MCP tool (use the same predicate/visibility logic as
retain_client_visible_response_tools); implement this by applying that predicate
to response.output (e.g., response.output.retain(|item| is_client_visible(item,
&session, &user_function_names))) before returning the response.
In `@model_gateway/src/routers/grpc/regular/responses/non_streaming.rs`:
- Around line 275-302: Early-return when mcp_tool_calls.is_empty() drops
previously accumulated MCP items (state.mcp_call_items); before returning the
converted ResponsesResponse from conversions::chat_to_responses and after
calling retain_client_visible_response_tools, call
inject_client_visible_mcp_output_items to inject and reconcile
state.mcp_call_items into the responses_response (ensuring the visibility policy
is applied to both injected MCP items and any pre-existing output items) so the
final returned/persisted response includes MCP outputs from prior iterations.
In `@model_gateway/src/routers/grpc/regular/responses/streaming.rs`:
- Around line 591-610: The converter currently emits tool-call chunks inside
convert_and_accumulate_stream (via emitter.process_chunk) before visibility is
determined, causing MCP/function calls to leak; change the flow so tool-call
deltas are buffered and not emitted until visibility is known: have
convert_and_accumulate_stream either (A) accept session and user_function_names
and suppress/buffer any tool-call deltas (do not call emitter.process_chunk for
those) while accumulating, or (B) split into a first pass that accumulates the
raw streamed events (without emitting) then call
extract_all_tool_calls_from_chat and session.should_intercept_function_call to
partition into mcp_tool_calls vs function_tool_calls, and only afterwards emit
non-tool chunks and the appropriate visible tool-call events via
emitter.process_chunk and tx. Ensure references: convert_and_accumulate_stream,
extract_all_tool_calls_from_chat, session.should_intercept_function_call, and
emitter.process_chunk are updated so tool-call emission is conditional on the
partition result.
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 1105-1153: The current code collects incomplete MCP mcp_call items
but leaves the original intercepted `function_call` entries in `output_array`,
causing duplicates/leaks; change the logic in the block that iterates
`output_array` so that when you detect an intercepted function call (using
`is_function_call_type` + `session.should_intercept_function_call`) you record
that entry for removal (e.g., collect its index or mark it) instead of only
pushing a new `mcp_call_item` into `incomplete_items`, then after building
`prefix` remove or replace those original entries from `output_array` (or
reconstruct `output_array` without them) before splicing in `prefix`; keep the
same helpers (`build_mcp_call_item`, `visible_mcp_list_tools_items`,
`state.mcp_call_items`, `session.should_hide_output_item_json`) so
hidden/internal placeholders are not left behind.
🪄 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: 909d32c3-50fe-466b-9cc6-833a6490c92b
📒 Files selected for processing (17)
crates/mcp/src/core/config.rscrates/mcp/src/core/session.rsdocs/reference/mcp-internal-servers.mdmodel_gateway/src/routers/grpc/common/responses/mod.rsmodel_gateway/src/routers/grpc/common/responses/streaming.rsmodel_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsmodel_gateway/src/routers/grpc/regular/responses/non_streaming.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rsmodel_gateway/src/routers/openai/context.rsmodel_gateway/src/routers/openai/mcp/mod.rsmodel_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/src/routers/openai/responses/streaming.rs
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
|
Hi @zhoug9127, 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24fb73d626
ℹ️ 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".
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
mergify
Bot
removed
the
needs-rebase
mergify
Bot
added
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c685e10a2
ℹ️ 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".
1c685e1 to
87ff9b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/grpc/common/responses/utils.rs`:
- Around line 399-410: emit_visible_mcp_list_tools_sequence currently iterates
session.mcp_servers() and may emit duplicate mcp_list_tools for bindings sharing
the same binding.label; change the function to deduplicate by binding.label
(e.g., track seen labels in a HashSet) so that for each unique label you call
session.list_tools_for_server(binding.server_key) and
emitter.emit_mcp_list_tools_sequence(&binding.label, &tools_for_server, tx) only
once, still honoring
session.should_emit_streaming_mcp_list_tools(&binding.label) before emitting;
keep all referenced symbols (emit_visible_mcp_list_tools_sequence,
session.mcp_servers, session.should_emit_streaming_mcp_list_tools,
session.list_tools_for_server, emitter.emit_mcp_list_tools_sequence,
binding.label, binding.server_key) unchanged.
In `@model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs`:
- Around line 322-326: The branch that calls
retain_client_visible_response_tools(&mut response, &session,
&user_function_names) does not filter already-populated response.output and
therefore leaks FunctionToolCall entries added by build_tool_response; update
the same visibility filtering to also remove or replace hidden/internal
FunctionToolCall items from response.output (not just response.tools) — locate
the build_tool_response usage and ensure the logic in
retain_client_visible_response_tools (or immediately after it) applies the
visibility predicate to response.output entries (matching on FunctionToolCall)
so internal tool names/outputs are dropped or redacted for the given session and
user_function_names.
In `@model_gateway/src/routers/openai/mcp/tool_handler.rs`:
- Around line 211-227: process_event currently calls ensure_output_index()
directly when handling response.output_item.done which bypasses visibility-aware
logic; change that branch to call
resolve_output_index_for_forwarding(upstream_index) (or check
visibility_state(upstream_index) before calling ensure_output_index()) so hidden
items do not consume a mapped index. Update the response.output_item.done
handling in process_event to use resolve_output_index_for_forwarding (or guard
with OutputVisibilityState::Visible) and preserve the existing behavior of
find_call_mut and assigned_output_index mutation when a mapping is returned.
In `@model_gateway/src/routers/openai/responses/streaming.rs`:
- Around line 1584-1637: The test currently doesn't exercise
reconcile_mixed_mode_final_response's retain-by-call_id branch because the
placeholder items lack a "call_id" and state.conversation_history is empty;
update the test to add a matching "call_id" on the MCP placeholder item (e.g.,
"call_id": "cid1") and populate state.conversation_history with a function_call
entry whose call_id is the same "cid1" so the retain closure has to check and
remove the executed placeholder; keep the rest of the setup (user_function_names
and assertions) the same so the final assertions still verify that the executed
MCP entry is removed and the user function_call remains.
🪄 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: 823728a6-851d-4232-b509-d428129781f5
📒 Files selected for processing (11)
model_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsmodel_gateway/src/routers/grpc/regular/responses/non_streaming.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rsmodel_gateway/src/routers/openai/mcp/mod.rsmodel_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87ff9b5cf6
ℹ️ 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".
ad9fcad to
1e14be5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e14be5c4b
ℹ️ 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".
…onciliation Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
1e14be5 to
5f30737
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
model_gateway/src/routers/openai/mcp/tool_handler.rs (1)
280-284:⚠️ Potential issue | 🟠 Major
output_item.donestill commits hidden items to a visible index.
handle_streaming_with_tool_interception()callsprocess_event()before the hide/redaction gate inmodel_gateway/src/routers/openai/responses/streaming.rs, so thisresolve_output_index_for_forwarding()call still allocates and marksUnknown -> Visiblefor a hiddenDONE-only item. The event is then dropped later, butnext_indexhas already advanced, so the next visible item can start at 1/2/... again.Please defer the
Unknown -> Visibletransition out of this branch until the forwarding path has decided the item is client-visible, or only reuse an existing mapping here.🤖 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 280 - 284, OutputItemEvent::DONE currently calls resolve_output_index_for_forwarding which creates a new Unknown->Visible mapping even for items that will later be hidden; inside handle_streaming_with_tool_interception (which calls process_event before the hide/redaction gate) this advances next_index incorrectly. Change the DONE branch in tool_handler.rs to avoid creating a new mapping: either call a non-creating lookup (e.g., get_existing_output_index_for_forwarding) or check if a mapping exists before calling resolve_output_index_for_forwarding, and only perform the Unknown->Visible transition when the forwarding path decides the item is client-visible (defer the transition until after forward/visibility decision). Ensure you reference OutputItemEvent::DONE and resolve_output_index_for_forwarding when locating the code to modify.
🤖 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/grpc/common/responses/utils.rs`:
- Around line 267-324: has_custom_tool currently only checks top-level
ResponseTool::Custom, missing Custom tools nested inside
ResponseTool::Namespace; update has_custom_tool(tools: &[ResponseTool],
custom_name: &str) to also traverse ResponseTool::Namespace variants
(recursively or by iterating the namespace's inner tools) and return true if any
nested ResponseTool::Custom has the matching name (leave callers like
is_tool_choice_compatible and has_allowed_tool_reference unchanged so
ResponsesToolChoice::Custom and AllowedTools see namespaced customs).
---
Duplicate comments:
In `@model_gateway/src/routers/openai/mcp/tool_handler.rs`:
- Around line 280-284: OutputItemEvent::DONE currently calls
resolve_output_index_for_forwarding which creates a new Unknown->Visible mapping
even for items that will later be hidden; inside
handle_streaming_with_tool_interception (which calls process_event before the
hide/redaction gate) this advances next_index incorrectly. Change the DONE
branch in tool_handler.rs to avoid creating a new mapping: either call a
non-creating lookup (e.g., get_existing_output_index_for_forwarding) or check if
a mapping exists before calling resolve_output_index_for_forwarding, and only
perform the Unknown->Visible transition when the forwarding path decides the
item is client-visible (defer the transition until after forward/visibility
decision). Ensure you reference OutputItemEvent::DONE and
resolve_output_index_for_forwarding when locating the code to modify.
🪄 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: 191e939c-79aa-4605-b717-f9bf85d07b66
📒 Files selected for processing (12)
model_gateway/src/routers/common/mcp_utils.rsmodel_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/responses/non_streaming.rsmodel_gateway/src/routers/grpc/harmony/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsmodel_gateway/src/routers/grpc/regular/responses/non_streaming.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rsmodel_gateway/src/routers/openai/mcp/mod.rsmodel_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f30737939
ℹ️ 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".
…nciliation Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e00e7c45a0
ℹ️ 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".
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f3c990ecf
ℹ️ 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".
| let classification_session = ctx.session.or(redaction_session); | ||
|
|
||
| let is_mcp_call_arguments_event = matches!( | ||
| resolved_event_type, | ||
| FunctionCallEvent::ARGUMENTS_DELTA | FunctionCallEvent::ARGUMENTS_DONE |
There was a problem hiding this comment.
When mixed-mode streaming switches to passthrough (ctx.session is None), this still falls back to redaction_session for MCP argument classification, so later client-owned MCP calls are treated as intercepted. In that state, forward_streaming_event drops response.function_call_arguments.delta and remaps *.done to mcp_call_arguments.*, while the corresponding output items remain function_call; this can remove arguments the client needs and produce inconsistent event types/IDs for the same call. Classification for argument remapping should stay tied to active interception, and use redaction-only logic only for hide/drop decisions.
Useful? React with 👍 / 👎.
Signed-off-by: Daisy Zhou <zhoug9127@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dec1dc95e
ℹ️ 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".
| let mut seen_labels = HashSet::new(); | ||
| for binding in session.mcp_servers() { | ||
| if !seen_labels.insert(binding.label.clone()) { | ||
| continue; |
There was a problem hiding this comment.
The new seen_labels gate drops later bindings that reuse an earlier server_label, so streaming now emits only the first server’s mcp_list_tools block for that label. McpToolSession allows multiple bindings to share a label (for example different server_key entries), and those tools can still be exposed/called, so this dedupe makes the streamed metadata incomplete and can desync clients that rely on mcp_list_tools to discover callable tools. Deduping should use a binding-level key (e.g. label+server_key/tools) rather than label alone.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
Streaming MCP interception had a mixed-batch gap: when a streamed turn contained both user function calls and MCP-intercepted calls, final
response.completed/persisted output could omit already-executed MCP output items. That creates client state mismatch and replay risk.In addition, output visibility/index behavior relied on subtle ordering assumptions with limited local guardrails/tests.
Solution
function_callplaceholders and injecting executed MCP output items beforeresponse.completedemission and persistence.resolve_output_index_for_forwardingside effects and interception vs redaction context usage.mark_output_hiddenis called after an index is already visible.output_item.donehandling,Changes
model_gateway/src/routers/openai/responses/streaming.rssend_final_response_eventand persisted final response path when interception is disabled but MCP calls executed.model_gateway/src/routers/openai/mcp/tool_handler.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/mcp/mod.rsmodel_gateway/src/routers/grpc/regular/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsTest Plan
Reproducible verification run:
cargo test -p smg --lib routers::openai::mcp::tool_handler::tests::cargo test -p smg --lib routers::openai::responses::streaming::tests::cargo test -p smg --lib routers::openai::mcp::tool_loop::tests::cargo test -p smg --test api_tests responsescargo test -p smg --test api_tests mcppre-commit run --all-filesExpected/observed: all commands pass.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit