refactor(approvals): route MCP reviews through coordinator

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> {
+        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<T> {
+    pub(crate) decision: ReviewDecision,
+    pub(crate) output: T,
+}
+
+pub(crate) struct ApprovalEventResolution<T> {
+    pub(crate) resolution: ApprovalResolution,
+    pub(crate) user_output: Option<T>,
+}
+
 impl ApprovalResolution {
     pub(crate) fn into_tool_result(self) -> Result<ReviewDecision, ToolError> {
         if let Some(rejection) = self.rejection {
@@ -193,15 +210,65 @@ impl ApprovalCoordinator {
     where
         F: FnOnce() -> Fut,
         Fut: Future<Output = ReviewDecision>,
+    {
+        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<Session>,
+        turn: &Arc<TurnContext>,
+        reviewer: ApprovalReviewer,
+        hook_request: Option<ApprovalHookRequest<'_>>,
+        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<T, F, Fut>(
+        session: &Arc<Session>,
+        turn: &Arc<TurnContext>,
+        reviewer: ApprovalReviewer,
+        hook_request: Option<ApprovalHookRequest<'_>>,
+        review: ApprovalReview,
+        user_review: F,
+    ) -> ApprovalEventResolution<T>
+    where
+        F: FnOnce() -> Fut,
+        Fut: Future<Output = ApprovalUserReview<T>>,
     {
         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(

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<Session>,
+    turn_context: &Arc<TurnContext>,
+    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 },
     }

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 }
     );
 }

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<crate::guardian::GuardianApprovalRequest>),
+    ApprovalRequest(Box<crate::approval_coordinator::ApprovalAction>),
 }
 
 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(