Conversation
Signed-off-by: Ziwen Zhao <me@zhaoziwen.com.cn>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
mcp
![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 a mechanism to continue tool execution after user approval without re-triggering the approval process. Key changes include adding continuation methods to the MCP orchestrator and session, updating protocol validation to allow approval-only requests, and modifying the model gateway's tool loop to handle resumed executions. Feedback was provided regarding a bug in the tool loop where tools were being incorrectly stripped from the resumed payload, which would prevent the model from making subsequent tool calls.
| &base_payload, | ||
| &state.conversation_history, | ||
| &state.original_input, | ||
| &json!([]), |
There was a problem hiding this comment.
The tools are being stripped from the resumed payload by passing an empty array to build_resume_payload. This will prevent the model from calling any further tools in subsequent turns of the tool loop if the original request included them. It should use &tools_json instead, which contains the tools from the initial payload.
| &json!([]), | |
| &tools_json, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| obj.remove("tools"); | ||
| obj.remove("tool_choice"); |
There was a problem hiding this comment.
🟡 Nit: These removals affect ALL callers of build_resume_payload, not just the approval continuation path. Before this PR, tool_choice from the original request was preserved in every resume payload sent to the LLM during the normal tool loop (line 1116). Now it's unconditionally stripped.
This is likely the correct behavior (you don't want tool_choice: "required" forcing infinite tool calls in the loop), but it's a broader behavioral change than the approval continuation feature. Worth a comment or a dedicated test to document the intent.
| fn payload_input(payload: &Value) -> Option<ResponseInput> { | ||
| payload | ||
| .get("input") | ||
| .cloned() | ||
| .and_then(|input| serde_json::from_value(input).ok()) | ||
| } |
There was a problem hiding this comment.
🟡 Nit: Both .ok() calls in this function and in extract_approved_tool_continuation (line 656) silently swallow deserialization failures. If the stored payload or approval request arguments are malformed, the approved tool execution is silently skipped with no log output — the user approves a tool but nothing happens.
Consider adding warn! logging on the error paths, similar to deserialize_output_items_from_array in history.rs which logs "Failed to deserialize item". Something like:
fn payload_input(payload: &Value) -> Option<ResponseInput> {
payload
.get("input")
.cloned()
.and_then(|input| serde_json::from_value(input)
.map_err(|e| warn!("Failed to deserialize payload input for approval continuation: {e}"))
.ok())
}
| arr.iter() | ||
| .filter(|item| match item.get("type").and_then(|v| v.as_str()) { | ||
| Some(ItemType::MCP_LIST_TOOLS) | Some(ItemType::MCP_CALL) => false, | ||
| Some("mcp_approval_request") => keep_approval_requests, |
There was a problem hiding this comment.
🟡 Nit: "mcp_approval_request" is a raw string here, while MCP_LIST_TOOLS and MCP_CALL on the line above use ItemType constants. Same for "mcp_approval_response" on line 243. Consider adding ItemType::MCP_APPROVAL_REQUEST / ItemType::MCP_APPROVAL_RESPONSE constants for consistency and to avoid drift if the serde rename ever changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/mcp/src/core/orchestrator.rs`:
- Around line 1064-1070: Change the visibility of the approval-bypass API so it
is crate-private: locate the function continue_tool_resolved_execution and
replace its public visibility (pub) with pub(crate) so only internal callers
(e.g., the session wrapper) can invoke the approval-bypass primitive; ensure no
other external code relies on the public signature and run tests/compilation
after the change.
In `@crates/protocols/src/responses.rs`:
- Around line 1105-1114: The is_approval_only_continuation predicate currently
treats MCP approval responses with an empty approval_request_id as valid; update
the check inside the items.iter().all(...) (where
ResponseInputOutputItem::McpApprovalResponse { .. } is matched) to ensure the
embedded approval_request_id is present and non-empty (e.g., require Some(id) &&
!id.is_empty()), so that approval-only continuations only pass when a resolvable
approval_request_id exists; keep the other conditions (request.stream and
request.previous_response_id) unchanged.
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 635-642: The code only finds MCP approval responses with approve
== true; instead, locate the matching
ResponseInputOutputItem::McpApprovalResponse regardless of its approve value
(replace the current find_map that yields approval_response_id with a find that
returns the full item or its id and approve flag), then in execute_tool_loop
branch on the approve boolean: if approve == true continue as before, but if
approve == false convert the denial locally into an upstream-safe completed
response (e.g., create a function_call_output/completed ResponseOutput that
strips any mcp_approval_response items) and return/post that instead of
forwarding the MCP state. Update references to
ResponseInputOutputItem::McpApprovalResponse, approval_response_id,
execute_tool_loop and mcp_approval_response in your changes.
In `@model_gateway/src/routers/openai/responses/history.rs`:
- Around line 233-263: deserialize_input_items_from_array currently only filters
out "mcp_approval_response" but must also exclude all local mcp_* items to
prevent replaying them upstream; update the iterator filter in
deserialize_input_items_from_array to mirror the logic used in
deserialize_output_items_from_array (exclude ItemType::MCP_LIST_TOOLS,
ItemType::MCP_CALL and any "mcp_approval_request"/"mcp_approval_response"
types), i.e., check item.get("type").and_then(|v| v.as_str()) and return false
for those local MCP types so they are not deserialized into the returned
Vec<ResponseInputOutputItem>.
🪄 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: b3fa1ec4-b779-41b4-aa74-6b71face47d4
📒 Files selected for processing (9)
crates/mcp/src/core/orchestrator.rscrates/mcp/src/core/session.rscrates/protocols/src/responses.rse2e_test/responses/test_tools_call.pymodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/history.rsmodel_gateway/tests/api/responses_api_test.rsmodel_gateway/tests/common/mock_worker.rsmodel_gateway/tests/spec/responses.rs
| /// Continue a previously approved resolved tool call without re-entering approval. | ||
| pub async fn continue_tool_resolved_execution( | ||
| &self, | ||
| input: ToolExecutionInput, | ||
| server_key: &str, | ||
| server_label: &str, | ||
| ) -> ToolExecutionOutput { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find direct call sites of the approval-bypassing continuation primitive.
rg -nP '\bcontinue_tool_resolved_execution\s*\(' --type=rust -C2Repository: lightseekorg/smg
Length of output: 836
Keep the approval-bypass primitive crate-private.
This method intentionally skips approval; exposing it as pub makes the low-level bypass available to any downstream caller. Restrict visibility to pub(crate) since the only call site is from crates/mcp/src/core/session.rs (the session-level continuation API wrapper).
Proposed visibility change
- pub async fn continue_tool_resolved_execution(
+ pub(crate) async fn continue_tool_resolved_execution(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/mcp/src/core/orchestrator.rs` around lines 1064 - 1070, Change the
visibility of the approval-bypass API so it is crate-private: locate the
function continue_tool_resolved_execution and replace its public visibility
(pub) with pub(crate) so only internal callers (e.g., the session wrapper) can
invoke the approval-bypass primitive; ensure no other external code relies on
the public signature and run tests/compilation after the change.
| let is_approval_only_continuation = !request.stream.unwrap_or(false) | ||
| && request | ||
| .previous_response_id | ||
| .as_ref() | ||
| .is_some_and(|id| !id.is_empty()) | ||
| && items | ||
| .iter() | ||
| .all(|item| matches!(item, ResponseInputOutputItem::McpApprovalResponse { .. })); | ||
|
|
||
| if !has_valid_input && !is_approval_only_continuation { |
There was a problem hiding this comment.
Require a resolvable approval request id for approval-only continuations.
This exemption currently treats {"type":"mcp_approval_response","approval_request_id":""} as a valid approval-only continuation when previous_response_id is present, so malformed requests can bypass the user-message guard and fail later in the resume path.
🛡️ Proposed fix
let is_approval_only_continuation = !request.stream.unwrap_or(false)
&& request
.previous_response_id
.as_ref()
- .is_some_and(|id| !id.is_empty())
- && items
- .iter()
- .all(|item| matches!(item, ResponseInputOutputItem::McpApprovalResponse { .. }));
+ .is_some_and(|id| !id.trim().is_empty())
+ && items.iter().all(|item| {
+ matches!(
+ item,
+ ResponseInputOutputItem::McpApprovalResponse {
+ approval_request_id,
+ ..
+ } if !approval_request_id.trim().is_empty()
+ )
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/protocols/src/responses.rs` around lines 1105 - 1114, The
is_approval_only_continuation predicate currently treats MCP approval responses
with an empty approval_request_id as valid; update the check inside the
items.iter().all(...) (where ResponseInputOutputItem::McpApprovalResponse { .. }
is matched) to ensure the embedded approval_request_id is present and non-empty
(e.g., require Some(id) && !id.is_empty()), so that approval-only continuations
only pass when a resolvable approval_request_id exists; keep the other
conditions (request.stream and request.previous_response_id) unchanged.
| let approval_response_id = current_items.iter().rev().find_map(|item| match item { | ||
| ResponseInputOutputItem::McpApprovalResponse { | ||
| approval_request_id, | ||
| approve, | ||
| .. | ||
| } if *approve => Some(approval_request_id.clone()), | ||
| _ => None, | ||
| })?; |
There was a problem hiding this comment.
Handle rejected approval responses locally.
Line 640 only recognizes approve: true. For approve: false, continuation detection returns None, so execute_tool_loop falls through and posts the current payload upstream with the local mcp_approval_response still present. Denials should be converted locally into an upstream-safe function_call_output/completed response with approval items stripped, instead of being forwarded as MCP approval protocol state.
🐛 Suggested direction
- } if *approve => Some(approval_request_id.clone()),
+ } => Some((approval_request_id.clone(), *approve)),Then branch on approve after locating the matching approval request:
+// approve == true: execute via continue_tool_execution(...)
+// approve == false: record a synthetic function_call_output such as
+// "Tool call denied by user", build the resume payload with approval items
+// stripped, and do not call the MCP server.🤖 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 635 - 642,
The code only finds MCP approval responses with approve == true; instead, locate
the matching ResponseInputOutputItem::McpApprovalResponse regardless of its
approve value (replace the current find_map that yields approval_response_id
with a find that returns the full item or its id and approve flag), then in
execute_tool_loop branch on the approve boolean: if approve == true continue as
before, but if approve == false convert the denial locally into an upstream-safe
completed response (e.g., create a function_call_output/completed ResponseOutput
that strips any mcp_approval_response items) and return/post that instead of
forwarding the MCP state. Update references to
ResponseInputOutputItem::McpApprovalResponse, approval_response_id,
execute_tool_loop and mcp_approval_response in your changes.
| fn deserialize_input_items_from_array(array: &Value) -> Vec<ResponseInputOutputItem> { | ||
| array | ||
| .as_array() | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter(|item| { | ||
| item.get("type").and_then(|v| v.as_str()) != Some("mcp_approval_response") | ||
| }) | ||
| .filter_map(|item| { | ||
| serde_json::from_value::<ResponseInputOutputItem>(item.clone()) | ||
| .map_err(|e| warn!("Failed to deserialize item: {}. Item: {}", e, item)) | ||
| .ok() | ||
| }) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default() | ||
| } | ||
|
|
||
| fn deserialize_output_items_from_array( | ||
| array: &Value, | ||
| keep_approval_requests: bool, | ||
| ) -> Vec<ResponseInputOutputItem> { | ||
| array | ||
| .as_array() | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter(|item| match item.get("type").and_then(|v| v.as_str()) { | ||
| Some(ItemType::MCP_LIST_TOOLS) | Some(ItemType::MCP_CALL) => false, | ||
| Some("mcp_approval_request") => keep_approval_requests, | ||
| _ => true, | ||
| }) |
There was a problem hiding this comment.
Filter all local MCP items from stored input replay.
Line 238 only drops mcp_approval_response; a later previous_response_id turn can still replay stored-input mcp_approval_request, mcp_call, or mcp_list_tools items. This breaks the “do not resend local mcp_* items upstream” invariant after an approval continuation.
Suggested fix
+fn is_local_mcp_history_item_type(item_type: &str) -> bool {
+ matches!(
+ item_type,
+ ItemType::MCP_LIST_TOOLS
+ | ItemType::MCP_CALL
+ | "mcp_approval_request"
+ | "mcp_approval_response"
+ )
+}
+
fn deserialize_input_items_from_array(array: &Value) -> Vec<ResponseInputOutputItem> {
array
.as_array()
.map(|arr| {
arr.iter()
.filter(|item| {
- item.get("type").and_then(|v| v.as_str()) != Some("mcp_approval_response")
+ !item
+ .get("type")
+ .and_then(|v| v.as_str())
+ .is_some_and(is_local_mcp_history_item_type)
})
.filter_map(|item| {
serde_json::from_value::<ResponseInputOutputItem>(item.clone())
.map_err(|e| warn!("Failed to deserialize item: {}. Item: {}", e, item))
.ok()
@@
.as_array()
.map(|arr| {
arr.iter()
.filter(|item| match item.get("type").and_then(|v| v.as_str()) {
- Some(ItemType::MCP_LIST_TOOLS) | Some(ItemType::MCP_CALL) => false,
Some("mcp_approval_request") => keep_approval_requests,
+ Some(item_type) if is_local_mcp_history_item_type(item_type) => false,
_ => true,
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/openai/responses/history.rs` around lines 233 -
263, deserialize_input_items_from_array currently only filters out
"mcp_approval_response" but must also exclude all local mcp_* items to prevent
replaying them upstream; update the iterator filter in
deserialize_input_items_from_array to mirror the logic used in
deserialize_output_items_from_array (exclude ItemType::MCP_LIST_TOOLS,
ItemType::MCP_CALL and any "mcp_approval_request"/"mcp_approval_response"
types), i.e., check item.get("type").and_then(|v| v.as_str()) and return false
for those local MCP types so they are not deserialized into the returned
Vec<ResponseInputOutputItem>.
1 participant
Summary
/v1/responsesnon-streaming requests by resuming the approved tool locally and replaying only upstream-safe contextprevious_response_id, and sanitize replayed history so laterprevious_response_idturns do not resend localmcp_*items upstreamsmgbehavior against OpenAI, including 6-turnprevious_response_idflows forrequire_approval: \"always\"and\"never\"Validation
pre-commit run --all-filescargo test --package smg --test api_tests test_non_streaming_mcp_approval_continuation_uses_previous_response_id -- --nocapturecargo test --package smg --test spec_test test_validate_input_items_structure -- --nocapturesmg, including:previous_response_idfollow-up flows confirming the model retains the original MCP result for bothrequire_approval: \"always\"and\"never\"Notes
Summary by CodeRabbit
Release Notes
New Features
Tests