From 0d10edef5c96c5265c64f187e2c4cb8d157643dd Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Tue, 7 Jul 2026 10:48:39 -0700 Subject: [PATCH] refactor(approvals): route MCP reviews through coordinator --- codex-rs/core/src/approval_coordinator.rs | 97 ++++++++++-- codex-rs/core/src/mcp_tool_call.rs | 175 +++++++++++----------- codex-rs/core/src/mcp_tool_call_tests.rs | 90 +++++------ codex-rs/core/src/session/mcp.rs | 41 ++--- 4 files changed, 230 insertions(+), 173 deletions(-) diff --git a/codex-rs/core/src/approval_coordinator.rs b/codex-rs/core/src/approval_coordinator.rs index 63bfc191ff..d0b74555b8 100644 --- a/codex-rs/core/src/approval_coordinator.rs +++ b/codex-rs/core/src/approval_coordinator.rs @@ -64,6 +64,13 @@ impl ApprovalReviewer { } } + pub(crate) fn automatic_for_reviewer( + turn: &TurnContext, + reviewer: ApprovalsReviewer, + ) -> Option { + Self::routes_to_guardian(turn, reviewer).then_some(Self::Guardian) + } + fn routes_to_guardian(turn: &TurnContext, reviewer: ApprovalsReviewer) -> bool { matches!( turn.approval_policy.value(), @@ -86,6 +93,16 @@ pub(crate) struct ApprovalResolution { pub(crate) source: ApprovalResolutionSource, } +pub(crate) struct ApprovalUserReview { + pub(crate) decision: ReviewDecision, + pub(crate) output: T, +} + +pub(crate) struct ApprovalEventResolution { + pub(crate) resolution: ApprovalResolution, + pub(crate) user_output: Option, +} + impl ApprovalResolution { pub(crate) fn into_tool_result(self) -> Result { if let Some(rejection) = self.rejection { @@ -193,15 +210,65 @@ impl ApprovalCoordinator { where F: FnOnce() -> Fut, Fut: Future, + { + Self::resolve_event_with_user_output( + session, + turn, + reviewer, + hook_request, + review, + || async { + ApprovalUserReview { + decision: user_review().await, + output: (), + } + }, + ) + .await + .resolution + } + + pub(crate) async fn resolve_automatic_event( + session: &Arc, + turn: &Arc, + reviewer: ApprovalReviewer, + hook_request: Option>, + review: ApprovalReview, + ) -> ApprovalResolution { + debug_assert_eq!(reviewer, ApprovalReviewer::Guardian); + Self::resolve_event_with_user_output(session, turn, reviewer, hook_request, review, || async { + ApprovalUserReview { + decision: ReviewDecision::Denied, + output: (), + } + }) + .await + .resolution + } + + pub(crate) async fn resolve_event_with_user_output( + session: &Arc, + turn: &Arc, + reviewer: ApprovalReviewer, + hook_request: Option>, + review: ApprovalReview, + user_review: F, + ) -> ApprovalEventResolution + where + F: FnOnce() -> Fut, + Fut: Future>, { if let Some(hook_request) = hook_request && let Some(resolution) = Self::resolve_hook(session, turn, hook_request.run_id, hook_request.payload).await { - return resolution; + return ApprovalEventResolution { + resolution, + user_output: None, + }; } - let resolution = match reviewer { + let (resolution, user_output) = match reviewer { ApprovalReviewer::Guardian => { let review_id = new_guardian_review_id(); let decision = review_approval_request( @@ -212,15 +279,27 @@ impl ApprovalCoordinator { review.retry_reason.clone(), ) .await; - Self::normalize_guardian(session, review_id, decision).await + ( + Self::normalize_guardian(session, review_id, decision).await, + None, + ) + } + ApprovalReviewer::User => { + let user_review = user_review().await; + ( + ApprovalResolution { + decision: user_review.decision, + rejection: None, + source: ApprovalResolutionSource::User, + }, + Some(user_review.output), + ) } - ApprovalReviewer::User => ApprovalResolution { - decision: user_review().await, - rejection: None, - source: ApprovalResolutionSource::User, - }, }; - Self::normalize_user_rejection(resolution) + ApprovalEventResolution { + resolution: Self::normalize_user_rejection(resolution), + user_output, + } } async fn resolve_hook( diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index d03f232d12..02963e8728 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -2,18 +2,18 @@ use std::collections::HashMap; use std::time::Duration; use std::time::Instant; +use crate::approval_coordinator::ApprovalAction; +use crate::approval_coordinator::ApprovalCoordinator; +use crate::approval_coordinator::ApprovalHookRequest; +use crate::approval_coordinator::ApprovalResolution; +use crate::approval_coordinator::ApprovalReview; +use crate::approval_coordinator::ApprovalReviewer; +use crate::approval_coordinator::ApprovalUserReview; use crate::config::Config; use crate::config::edit::ConfigEdit; use crate::config::edit::ConfigEditsBuilder; use crate::connectors; -use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianMcpAnnotations; -use crate::guardian::guardian_rejection_message; -use crate::guardian::guardian_timeout_message; -use crate::guardian::new_guardian_review_id; -use crate::guardian::review_approval_request; -use crate::guardian::routes_approval_to_guardian_with_reviewer; -use crate::hook_runtime::run_permission_request_hooks; use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files; use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template; @@ -33,7 +33,6 @@ use codex_connectors::AppToolPolicy; use codex_connectors::AppToolPolicyEvaluator; use codex_connectors::AppToolPolicyInput; use codex_features::Feature; -use codex_hooks::PermissionRequestDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_mcp::McpConnectionManager; @@ -1240,61 +1239,78 @@ async fn maybe_request_mcp_tool_approval( return Some(McpToolApprovalDecision::Accept); } - match run_permission_request_hooks( - sess, - turn_context, - call_id, - PermissionRequestPayload { - tool_name: hook_tool_name.clone(), - tool_input: invocation - .arguments - .clone() - .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), - }, - ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); - } - None => {} - } - let tool_call_mcp_elicitation_enabled = turn_context .config .features .enabled(Feature::ToolCallMcpElicitation); - - if routes_approval_to_guardian_with_reviewer(turn_context, approvals_reviewer) { - let review_id = new_guardian_review_id(); - let decision = review_approval_request( - sess, - turn_context, - review_id.clone(), + let result = ApprovalCoordinator::resolve_event_with_user_output( + sess, + turn_context, + ApprovalReviewer::for_reviewer(turn_context, approvals_reviewer), + Some(ApprovalHookRequest { + run_id: call_id, + payload: PermissionRequestPayload { + tool_name: hook_tool_name.clone(), + tool_input: invocation + .arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, + }), + ApprovalReview::main_turn( build_guardian_mcp_tool_review_request(call_id, invocation, metadata), /*retry_reason*/ None, - ) - .await; - let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await; - apply_mcp_tool_approval_decision( - sess, - turn_context, - &decision, - session_approval_key, - persistent_approval_key, - ) - .await; - return Some(decision); - } + ), + || async { + let output = request_mcp_tool_approval_from_user( + sess, + turn_context, + call_id, + invocation, + metadata, + approval_mode, + tool_call_mcp_elicitation_enabled, + session_approval_key.as_ref(), + persistent_approval_key.as_ref(), + ) + .await; + ApprovalUserReview { + decision: review_decision_for_mcp_tool_approval(&output), + output, + } + }, + ) + .await; + let decision = match result.user_output { + Some(decision) => decision, + None => mcp_tool_approval_decision_from_resolution(result.resolution), + }; + apply_mcp_tool_approval_decision( + sess, + turn_context, + &decision, + session_approval_key, + persistent_approval_key, + ) + .await; + Some(decision) +} +#[allow(clippy::too_many_arguments)] +async fn request_mcp_tool_approval_from_user( + sess: &Arc, + turn_context: &Arc, + call_id: &str, + invocation: &McpInvocation, + metadata: Option<&McpToolApprovalMetadata>, + approval_mode: AppToolApproval, + tool_call_mcp_elicitation_enabled: bool, + session_approval_key: Option<&McpToolApprovalKey>, + persistent_approval_key: Option<&McpToolApprovalKey>, +) -> McpToolApprovalDecision { let prompt_options = mcp_tool_approval_prompt_options( - session_approval_key.as_ref(), - persistent_approval_key.as_ref(), + session_approval_key, + persistent_approval_key, tool_call_mcp_elicitation_enabled, ); let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); @@ -1349,16 +1365,7 @@ async fn maybe_request_mcp_tool_approval( .response, &question_id, ); - let decision = normalize_approval_decision_for_mode(decision, approval_mode); - apply_mcp_tool_approval_decision( - sess, - turn_context, - &decision, - session_approval_key, - persistent_approval_key, - ) - .await; - return Some(decision); + return normalize_approval_decision_for_mode(decision, approval_mode); } let args = RequestUserInputArgs { @@ -1368,19 +1375,10 @@ async fn maybe_request_mcp_tool_approval( let response = sess .request_user_input(turn_context.as_ref(), call_id.to_string(), args) .await; - let decision = normalize_approval_decision_for_mode( + normalize_approval_decision_for_mode( parse_mcp_tool_approval_response(response, &question_id), approval_mode, - ); - apply_mcp_tool_approval_decision( - sess, - turn_context, - &decision, - session_approval_key, - persistent_approval_key, ) - .await; - Some(decision) } pub(crate) fn mcp_approvals_reviewer( @@ -1428,8 +1426,8 @@ pub(crate) fn build_guardian_mcp_tool_review_request( call_id: &str, invocation: &McpInvocation, metadata: Option<&McpToolApprovalMetadata>, -) -> GuardianApprovalRequest { - GuardianApprovalRequest::McpToolCall { +) -> ApprovalAction { + ApprovalAction::McpToolCall { id: call_id.to_string(), server: invocation.server.clone(), tool_name: invocation.tool.clone(), @@ -1452,21 +1450,30 @@ pub(crate) fn build_guardian_mcp_tool_review_request( } } -async fn mcp_tool_approval_decision_from_guardian( - sess: &Session, - review_id: &str, - decision: ReviewDecision, -) -> McpToolApprovalDecision { +fn review_decision_for_mcp_tool_approval(decision: &McpToolApprovalDecision) -> ReviewDecision { match decision { + McpToolApprovalDecision::Accept => ReviewDecision::Approved, + McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember => { + ReviewDecision::ApprovedForSession + } + McpToolApprovalDecision::Decline { .. } => ReviewDecision::Denied, + McpToolApprovalDecision::Cancel => ReviewDecision::Abort, + } +} + +fn mcp_tool_approval_decision_from_resolution( + resolution: ApprovalResolution, +) -> McpToolApprovalDecision { + match resolution.decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept, ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession, ReviewDecision::Denied => McpToolApprovalDecision::Decline { - message: Some(guardian_rejection_message(sess, review_id).await), + message: resolution.rejection, }, ReviewDecision::TimedOut => McpToolApprovalDecision::Decline { - message: Some(guardian_timeout_message()), + message: resolution.rejection, }, ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None }, } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index f68d32ff4e..bf7ffc44e9 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1759,7 +1759,7 @@ fn guardian_mcp_review_request_includes_invocation_metadata() { assert_eq!( request, - GuardianApprovalRequest::McpToolCall { + ApprovalAction::McpToolCall { id: "call-1".to_string(), server: CODEX_APPS_MCP_SERVER_NAME.to_string(), tool_name: "browser_navigate".to_string(), @@ -1804,7 +1804,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { assert_eq!( request, - GuardianApprovalRequest::McpToolCall { + ApprovalAction::McpToolCall { id: "call-1".to_string(), server: "custom_server".to_string(), tool_name: "dangerous_tool".to_string(), @@ -1842,7 +1842,7 @@ fn guardian_mcp_review_request_ignores_untrusted_connected_account_email() { assert_eq!( request, - GuardianApprovalRequest::McpToolCall { + ApprovalAction::McpToolCall { id: "call-1".to_string(), server: "custom_server".to_string(), tool_name: "dangerous_tool".to_string(), @@ -1858,62 +1858,42 @@ fn guardian_mcp_review_request_ignores_untrusted_connected_account_email() { ); } -#[tokio::test(flavor = "current_thread")] -async fn guardian_review_decision_maps_to_mcp_tool_decision() { - let (session, _) = make_session_and_context().await; - let session = Arc::new(session); - +#[test] +fn approval_resolution_maps_to_mcp_tool_decision() { assert_eq!( - mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Approved - ) - .await, + mcp_tool_approval_decision_from_resolution(ApprovalResolution { + decision: ReviewDecision::Approved, + rejection: None, + source: crate::approval_coordinator::ApprovalResolutionSource::Guardian, + }), McpToolApprovalDecision::Accept ); - session.services.guardian_rejections.lock().await.insert( - "review-id".to_string(), - crate::guardian::GuardianRejection { - rationale: "too risky".to_string(), - source: codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, - }, - ); - let denial = mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Denied, - ) - .await; - let McpToolApprovalDecision::Decline { - message: Some(message), - } = denial - else { - panic!("guardian denial should carry a rejection message"); - }; - assert!(message.contains("Reason: too risky")); - assert!(message.contains("The agent must not attempt to achieve the same outcome")); - let timeout = mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::TimedOut, - ) - .await; - let McpToolApprovalDecision::Decline { - message: Some(message), - } = timeout - else { - panic!("guardian timeout should carry a timeout message"); - }; - assert!(message.contains("did not finish before its deadline")); - assert!(!message.contains("unacceptable risk")); assert_eq!( - mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Abort - ) - .await, + mcp_tool_approval_decision_from_resolution(ApprovalResolution { + decision: ReviewDecision::Denied, + rejection: Some("too risky".to_string()), + source: crate::approval_coordinator::ApprovalResolutionSource::Guardian, + }), + McpToolApprovalDecision::Decline { + message: Some("too risky".to_string()), + } + ); + assert_eq!( + mcp_tool_approval_decision_from_resolution(ApprovalResolution { + decision: ReviewDecision::TimedOut, + rejection: Some("review timed out".to_string()), + source: crate::approval_coordinator::ApprovalResolutionSource::Guardian, + }), + McpToolApprovalDecision::Decline { + message: Some("review timed out".to_string()), + } + ); + assert_eq!( + mcp_tool_approval_decision_from_resolution(ApprovalResolution { + decision: ReviewDecision::Abort, + rejection: Some("ignored for abort".to_string()), + source: crate::approval_coordinator::ApprovalResolutionSource::Guardian, + }), McpToolApprovalDecision::Decline { message: None } ); } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 3f5e92eca7..db6961643b 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -34,7 +34,7 @@ const TOOL_SUGGESTION_TOOL_TYPE_KEY: &str = "tool_type"; enum GuardianElicitationReview { NotRequested, Decline(&'static str), - ApprovalRequest(Box), + ApprovalRequest(Box), } struct GuardianMcpElicitationReviewer { @@ -599,12 +599,12 @@ async fn review_guardian_mcp_elicitation( request.server_name.as_str(), elicitation_connector_id(&request.elicitation), ); - if !crate::guardian::routes_approval_to_guardian_with_reviewer( + let Some(reviewer) = crate::approval_coordinator::ApprovalReviewer::automatic_for_reviewer( turn_context.as_ref(), approvals_reviewer, - ) { + ) else { return Ok(None); - } + }; let guardian_request = match guardian_elicitation_review_request(&request) { GuardianElicitationReview::NotRequested => return Ok(None), @@ -620,19 +620,18 @@ async fn review_guardian_mcp_elicitation( GuardianElicitationReview::ApprovalRequest(guardian_request) => *guardian_request, }; - let review_id = crate::guardian::new_guardian_review_id(); - let decision = crate::guardian::review_approval_request( + let resolution = crate::approval_coordinator::ApprovalCoordinator::resolve_automatic_event( &session, &turn_context, - review_id.clone(), - guardian_request, - /*retry_reason*/ None, + reviewer, + /*hook_request*/ None, + crate::approval_coordinator::ApprovalReview::main_turn( + guardian_request, + /*retry_reason*/ None, + ), ) .await; - Ok(Some( - mcp_elicitation_response_from_guardian_decision(session.as_ref(), &review_id, decision) - .await, - )) + Ok(Some(mcp_elicitation_response_from_resolution(resolution))) } fn guardian_elicitation_review_request( @@ -696,7 +695,7 @@ fn guardian_elicitation_review_request( }; GuardianElicitationReview::ApprovalRequest(Box::new( - crate::guardian::GuardianApprovalRequest::McpToolCall { + crate::approval_coordinator::ApprovalAction::McpToolCall { id: format!( "mcp_elicitation:{}:{}", request.server_name, @@ -776,18 +775,10 @@ fn mcp_elicitation_request_id(id: &RequestId) -> String { } } -async fn mcp_elicitation_response_from_guardian_decision( - session: &Session, - review_id: &str, - decision: ReviewDecision, +fn mcp_elicitation_response_from_resolution( + resolution: crate::approval_coordinator::ApprovalResolution, ) -> ElicitationResponse { - let denial_message = match decision { - ReviewDecision::Denied => { - Some(crate::guardian::guardian_rejection_message(session, review_id).await) - } - _ => None, - }; - mcp_elicitation_response_from_guardian_decision_parts(decision, denial_message) + mcp_elicitation_response_from_guardian_decision_parts(resolution.decision, resolution.rejection) } fn mcp_elicitation_response_from_guardian_decision_parts(