feat(responses): interrupt approval-required MCP tool calls - #1174
Conversation
📝 WalkthroughWalkthroughIntroduces approval-preserving tool execution: adds Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Router as Router (tool_loop)
participant Session as McpToolSession
participant Orchestrator as McpOrchestrator
participant Approval as Approval/Eval
Client->>Router: request (with tools)
Router->>Session: execute_tool_result(input)
Session->>Orchestrator: execute_tool_resolved_result(input, server_key, server_label, ctx)
Orchestrator->>Approval: evaluate execution / approval rules
alt Approval Required
Approval-->>Orchestrator: PendingApproval
Orchestrator-->>Session: ToolExecutionResult::PendingApproval
Session-->>Router: PendingApproval
Router->>Router: build_approval_response(mcp_approval_request + retained items)
Router-->>Client: response (status: completed, mcp_approval_request item)
else Executed
Approval-->>Orchestrator: Executed(output)
Orchestrator-->>Session: ToolExecutionResult::Executed
Session-->>Router: Executed
Router-->>Client: response containing mcp_call output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for MCP tools to require interactive approval before execution, allowing the tool loop to interrupt and return an mcp_approval_request. Key changes include the addition of a ToolExecutionResult enum to distinguish between executed and pending states, updates to the McpOrchestrator and McpToolSession to preserve this state, and an expansion of the RequireApproval protocol to support granular rules based on tool names and read-only status. Feedback focuses on ensuring that pending approvals are not incorrectly recorded as failures in metrics and that tool names in approval requests are properly escaped and use model-facing aliases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a806806730
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1097-1131: The metrics currently treat a PendingApproval result as
a failed execution because succeeded is computed only for
ToolExecutionResult::Executed and record_call_end is always called; update the
logic so that when ApprovalExecutionResult::PendingApproval produces a
ToolExecutionResult::PendingApproval you do not call
self.metrics.record_call_end (or call a new
self.metrics.record_call_pending_approval) and only invoke record_call_end when
execution actually completed (i.e., when producing
ToolExecutionResult::Executed); refer to the PendingApproval construction in
this block, record_call_end usage, and note that record_approval_requested is
already emitted deeper in execute_tool_with_approval_raw_internal if you prefer
the skip-to-avoid-double-counting approach.
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 842-858: The approval request is using the orchestrator-resolved
internal name (pending.approval_request.tool_name) instead of the exposed name,
causing client-visible internal names and mismatches with function_call.name and
call.arguments; update the call to build_mcp_approval_request_item to pass
&pending.tool_name (the exposed name set by McpToolSession::execute_tool_result)
instead of &pending.approval_request.tool_name so the approval item and
subsequent build_approval_response use the externally visible tool name.
In `@model_gateway/src/routers/openai/responses/non_streaming.rs`:
- Around line 79-94: The approval_mode detection currently treats any
RequireApproval::Rules as Interactive; change it so only explicit string modes
drive Interactive sessions until rule-filter semantics are implemented: update
the predicate that inspects original_body.tools / ResponseTool::Mcp to only
return ApprovalMode::Interactive when mcp_tool.require_approval.as_ref() matches
Some(RequireApproval::Mode(mode)) and mode != RequireApprovalMode::Never; leave
Rules variants as non-interactive (PolicyOnly) for now, or alternatively
implement a helper like evaluate_require_approval_rules(&rules) and call it from
this predicate if you prefer to fully evaluate Rules before choosing
Interactive.
🪄 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: b56c7dd0-b5a7-4751-86c6-9520e851dfa8
📒 Files selected for processing (9)
crates/mcp/src/core/mod.rscrates/mcp/src/core/orchestrator.rscrates/mcp/src/core/session.rscrates/mcp/src/lib.rscrates/protocols/src/responses.rse2e_test/responses/test_tools_call.pymodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/tests/api/responses_api_test.rs
a806806 to
63966a3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63966a3415
ℹ️ 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".
63966a3 to
ba17a0c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba17a0c54c
ℹ️ 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".
ba17a0c to
8173c59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8173c59a9b
ℹ️ 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".
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 (1)
crates/mcp/src/core/session.rs (1)
95-103:⚠️ Potential issue | 🔴 CriticalDon't replace the caller's tenant with
TenantContext::default().Every per-tool context now comes from
request_ctx_for(), so this constructor value is whatexecute_tool_with_approval_raw_internal()passes intoApprovalParams.tenant_ctx. Hardcoding the default tenant will run approval policy/audit under the wrong tenant for every non-default request.Proposed fix
pub fn new( orchestrator: &'a McpOrchestrator, mcp_servers: Vec<McpServerBinding>, request_id: impl Into<String>, + tenant_ctx: TenantContext, ) -> Self { let request_id = request_id.into(); - let tenant_ctx = TenantContext::default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mcp/src/core/session.rs` around lines 95 - 103, The constructor currently overwrites the caller's tenant by using TenantContext::default(); change the signature of McpSession::new to accept a TenantContext (e.g., tenant_ctx: TenantContext) and assign that to tenant_ctx instead of TenantContext::default(), and update call sites (such as where execute_tool_with_approval_raw_internal() / request_ctx_for() create sessions) to pass the caller's request context so ApprovalParams.tenant_ctx remains the original request tenant rather than the default.
🤖 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/session.rs`:
- Around line 251-258: The PendingApproval branch updates pending.tool_name but
not the nested pending.approval_request.tool_name, leaving the embedded approval
request with the old pre-remap name; in the ToolExecutionResult::PendingApproval
arm (where you already set pending.tool_name = invoked_name), also assign
pending.approval_request.tool_name = invoked_name so the
exposed/collision-suffixed name stays in sync with the embedded
mcp_approval_request; ensure you reference the
ToolExecutionResult::PendingApproval variant, the pending variable, its
tool_name field, and the nested approval_request.tool_name when making this
change.
In `@e2e_test/responses/test_tools_call.py`:
- Around line 220-225: The test is over-constraining the sequence by requiring a
deepwiki mcp_call before approval items; instead, relax the assertions to only
check presence regardless of order: use resp.output and output_types to assert
that "mcp_call" and "mcp_approval_request" exist, assert that any item in
resp.output has type "mcp_call" with server_label == "deepwiki" (i.e., ensure
deepwiki_calls exists) without asserting positional ordering, and apply the same
relaxation to the other affected blocks referenced (the checks around lines
242-254 and 570-591) so the test only verifies presence of required call types
and labels rather than strict ordering.
---
Outside diff comments:
In `@crates/mcp/src/core/session.rs`:
- Around line 95-103: The constructor currently overwrites the caller's tenant
by using TenantContext::default(); change the signature of McpSession::new to
accept a TenantContext (e.g., tenant_ctx: TenantContext) and assign that to
tenant_ctx instead of TenantContext::default(), and update call sites (such as
where execute_tool_with_approval_raw_internal() / request_ctx_for() create
sessions) to pass the caller's request context so ApprovalParams.tenant_ctx
remains the original request tenant rather than the default.
🪄 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: 0038f9ec-d427-4de5-98fe-e0c846404aaa
📒 Files selected for processing (9)
crates/mcp/src/core/mod.rscrates/mcp/src/core/orchestrator.rscrates/mcp/src/core/session.rscrates/mcp/src/lib.rscrates/protocols/src/responses.rse2e_test/responses/test_tools_call.pymodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/tests/api/responses_api_test.rs
8173c59 to
3968cb7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3968cb7cfa
ℹ️ 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".
3968cb7 to
7ff04eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ff04eb852
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/openai/mcp/tool_loop.rs (1)
776-862:⚠️ Potential issue | 🟡 MinorConfirm: parallel MCP function_calls after the approval-triggering call are intentionally dropped in this iteration.
The current implementation processes
function_callssequentially in a loop and early-returns viabuild_approval_response(...)when the first call triggersPendingApproval. Remaining calls in that iteration are skipped, never executed, and never recorded or surfaced to the client.This is intentional scope for v1 ("one approval per request"), consistent with the design noted in PR
#1174. However, this is incomplete work:test_mcp_approval_required_interrupts_and_resumesine2e_test/responses/test_tools_call.py(line 580) includes an explicit TODO to extend approval-resumption testing in a follow-up PR. The session layer supports concurrent execution with approval state preservation (execute_tool_resultswithbuffered()incrates/mcp/src/core/session.rs), but the non-streaming tool loop does not leverage it.If parallel MCP calls with approval are expected in production, either pre-execute non-approval calls before the early return, or emit subsequent calls as additional output items so the client is aware of them. If this is deferred, document the limitation explicitly in a code comment or tracked issue.
♻️ Duplicate comments (1)
crates/protocols/src/responses.rs (1)
83-112:⚠️ Potential issue | 🟠 MajorReject filtered
require_approvalobjects until execution actually honors them.
RequireApproval::Rulesnow deserializes successfully, but the runtime path still only upgradesRequireApproval::Mode(RequireApprovalMode::Always)to interactive approval. Requests like{"never":{"tool_names":["foo"]}}will therefore round-trip here and then execute withPolicyOnly, silently dropping the caller’s approval policy.🛠️ Suggested guard until the follow-up lands
fn validate_response_tools(tools: &[ResponseTool]) -> Result<(), ValidationError> { // MCP server_label must be present and unique (case-insensitive). let mut seen_mcp_labels: HashSet<String> = HashSet::new(); for (idx, tool) in tools.iter().enumerate() { if let ResponseTool::Mcp(mcp) = tool { + if matches!(mcp.require_approval, Some(RequireApproval::Rules(_))) { + let mut e = ValidationError::new("unsupported_require_approval"); + e.message = Some( + "Object-form 'require_approval' is not supported yet; use \"always\" or \"never\"." + .into(), + ); + return Err(e); + } + let raw_label = mcp.server_label.as_str(); if raw_label.is_empty() { let mut e = ValidationError::new("missing_required_parameter"); e.message = Some( format!("Missing required parameter: 'tools[{idx}].server_label'.").into(),Based on learnings,
RequireApprovalintentionally only supported string forms until the object-form filtering behavior was implemented.🤖 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 83 - 112, The new object-form `RequireApprovalRules` must be rejected at deserialization until execution honors filtering: change RequireApproval's deserialization to fail when the `Rules` (RequireApprovalRules) shape is encountered so callers cannot round-trip an unsupported policy. Implement a custom Deserialize for the RequireApproval enum (or a deserialize_with for the enum) that successfully parses the string-mode forms into RequireApproval::Mode(RequireApprovalMode::...) but returns a clear error when an object matching RequireApprovalRules is provided; reference the types RequireApproval, RequireApprovalMode, and RequireApprovalRules to locate and update the deserialization logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/protocols/src/responses.rs`:
- Around line 83-112: The new object-form `RequireApprovalRules` must be
rejected at deserialization until execution honors filtering: change
RequireApproval's deserialization to fail when the `Rules`
(RequireApprovalRules) shape is encountered so callers cannot round-trip an
unsupported policy. Implement a custom Deserialize for the RequireApproval enum
(or a deserialize_with for the enum) that successfully parses the string-mode
forms into RequireApproval::Mode(RequireApprovalMode::...) but returns a clear
error when an object matching RequireApprovalRules is provided; reference the
types RequireApproval, RequireApprovalMode, and RequireApprovalRules to locate
and update the deserialization logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1206f3eb-50eb-468d-a634-05b5d903c20a
📒 Files selected for processing (9)
crates/mcp/src/core/mod.rscrates/mcp/src/core/orchestrator.rscrates/mcp/src/core/session.rscrates/mcp/src/lib.rscrates/protocols/src/responses.rse2e_test/responses/test_tools_call.pymodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/tests/api/responses_api_test.rs
7ff04eb to
66ffeeb
Compare
Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
…ector Both have been the consistent primary authors of recent PRs in these subsystems: - @zhoug9127 (Daisy): #1168, #1149, #1065, #1061, #976 — mcp + data_connector - @zhaowenzi (Ziwen): #1174, #1163, #1123 — mcp Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
66ffeeb to
6774fb3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6774fb3e4b
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/protocols/src/responses.rs (1)
83-112:⚠️ Potential issue | 🟠 MajorDo not accept rule-form approvals until they are enforced.
RequireApproval::Rulesnow deserializes valid object-form requests, but the runtime path still only treatsRequireApproval::Mode(RequireApprovalMode::Always)as interactive (crates/mcp/src/core/session.rs:309-314);Rulesfalls through as policy-only. A client sending{"always": ...}can therefore request approval while the tool executes without an approval interruption. Please either rejectRulesduring request validation for now, or evaluate/map it conservatively before execution.Based on learnings,
RequireApprovalintentionally only supported string forms until object-form filtering behavior was implemented.🤖 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 83 - 112, The PR added an object-form variant RequireApproval::Rules but the runtime approval check still only treats RequireApproval::Mode(RequireApprovalMode::Always) as interactive, so a client can send {"always": ...} and bypass interactive approval; fix by either (A) rejecting Rules at request/validation time (return a validation error when RequireApproval::Rules is present) or (B) conservatively mapping Rules to interactive behavior before execution (treat RequireApproval::Rules the same as RequireApproval::Mode(RequireApprovalMode::Always) in the session approval decision path). Locate the enum RequireApproval and the runtime code that matches on RequireApproval in the session approval check (the session approval decision path in core session code) and implement one of these two fixes consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/responses/test_tools_call.py`:
- Around line 561-583: The test named
test_mcp_approval_required_interrupts_and_resumes only asserts the interruption
and does not perform the resume flow; either rename it to reflect
interruption-only behavior (e.g., test_mcp_approval_required_interrupts_only) or
implement the continuation: after
assert_mcp_approval_interruption_non_streaming(resp) call the
approval/continuation API flow using api_client (the same client used earlier)
to send the approval token/continuation request for
DEEPWIKI_MCP_TOOL/BRAVE_MCP_TOOL_REQUIRE_APPROVAL_ALWAYS, then assert the
resumed response emits the expected mcp_call and final assistant output
(reuse/assert with the existing helper expectations such as checking for
mcp_call events and final assistant text) so the test name matches its behavior.
---
Duplicate comments:
In `@crates/protocols/src/responses.rs`:
- Around line 83-112: The PR added an object-form variant RequireApproval::Rules
but the runtime approval check still only treats
RequireApproval::Mode(RequireApprovalMode::Always) as interactive, so a client
can send {"always": ...} and bypass interactive approval; fix by either (A)
rejecting Rules at request/validation time (return a validation error when
RequireApproval::Rules is present) or (B) conservatively mapping Rules to
interactive behavior before execution (treat RequireApproval::Rules the same as
RequireApproval::Mode(RequireApprovalMode::Always) in the session approval
decision path). Locate the enum RequireApproval and the runtime code that
matches on RequireApproval in the session approval check (the session approval
decision path in core session code) and implement one of these two fixes
consistently.
🪄 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: 50e52621-63d1-4c1b-b659-834980bebe89
📒 Files selected for processing (9)
crates/mcp/src/core/mod.rscrates/mcp/src/core/orchestrator.rscrates/mcp/src/core/session.rscrates/mcp/src/lib.rscrates/protocols/src/responses.rse2e_test/responses/test_tools_call.pymodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/tests/api/responses_api_test.rs
Description
Problem
SMG's Responses API MCP flow did not expose approval-required interruptions in a way that matches OpenAI. Even when a tool required approval, the router continued to emit
mcp_calloutput instead of stopping at an approval request. The protocol layer also could not round-trip OpenAI's richerrequire_approvalshape.Solution
This PR adds the protocol and MCP core support needed for approval-aware execution, then wires the OpenAI non-streaming Responses path to stop on pending approval and emit
mcp_approval_requestinstead of continuing tomcp_call.Current scope note: the router interruption behavior in this PR is intentionally limited to explicit
require_approval: \"always\". Object-formrequire_approvalrules are supported at the protocol round-trip level, but their runtime semantics are left to a follow-up PR. The non-streaming interruption behavior is also scoped per tool call rather than blanket-enabling interactive approval for the whole request.Changes
require_approvalincrates/protocols/src/responses.rsto support both string and object formsrequire_approvalToolExecutionResultand configurable session approval moderequire_approval: \"always\"mcp_approval_requestoutput instead of emittingmcp_callrequire_approvalrule semantics for a follow-up PRTest Plan
In another shell, register an OpenAI worker without inlining secrets in the command:
Then send a mixed-tool Responses request where one MCP tool should execute immediately and another should interrupt for approval:
Expected behavior for this PR:
{ "status": "completed", "output": [ { "type": "mcp_list_tools", "server_label": "deepwiki" }, { "type": "mcp_list_tools", "server_label": "openai-developer-docs" }, { "type": "mcp_call", "id": "mcp_...", "server_label": "deepwiki", "name": "ask_question", "arguments": "{...}", "output": ... }, { "type": "mcp_approval_request", "id": "mcpr_...", "server_label": "openai-developer-docs", "name": "search_openai_docs", "arguments": "{...}" } ] }The response should allow the
deepwikitool to emitmcp_call, then stop atopenai-developer-docs'smcp_approval_requestwithout emitting anopenai-developer-docsmcp_callyet.This PR does not yet validate object-form
require_approvalrules at runtime; that behavior is deferred to a follow-up PR.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Tests