diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e5273a6669..acfad3ebe3 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -9,10 +9,7 @@ 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; @@ -20,6 +17,7 @@ use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam; use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template; use crate::session::session::Session; use crate::session::turn_context::TurnContext; +use crate::tools::approval_dispatch::request_automated_approval; use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::PermissionRequestPayload; use crate::turn_metadata::McpTurnMetadataContext; @@ -33,6 +31,7 @@ use codex_app_server_protocol::McpServerElicitationRequest; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_config::types::AppToolApproval; use codex_config::types::ApprovalsReviewer; +use codex_extension_api::ApprovalReviewSource; use codex_features::Feature; use codex_hooks::PermissionRequestDecision; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; @@ -1207,16 +1206,22 @@ async fn maybe_request_mcp_tool_approval( .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( + let automated = request_automated_approval( sess, turn_context, - review_id.clone(), + new_guardian_review_id(), build_guardian_mcp_tool_review_request(call_id, invocation, metadata), + approvals_reviewer, /*retry_reason*/ None, + ApprovalReviewSource::MainTurn, ) .await; - let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await; + let decision = match automated { + Ok(automated) => mcp_tool_approval_decision_from_automated(&automated), + Err(message) => McpToolApprovalDecision::Decline { + message: Some(message), + }, + }; apply_mcp_tool_approval_decision( sess, turn_context, @@ -1382,21 +1387,19 @@ pub(crate) fn build_guardian_mcp_tool_review_request( } } -async fn mcp_tool_approval_decision_from_guardian( - sess: &Session, - review_id: &str, - decision: ReviewDecision, +fn mcp_tool_approval_decision_from_automated( + automated: &crate::tools::approval_dispatch::AutomatedApprovalDecision, ) -> McpToolApprovalDecision { - match decision { + match automated.decision.clone() { 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: Some(automated.denial_message()), }, ReviewDecision::TimedOut => McpToolApprovalDecision::Decline { - message: Some(guardian_timeout_message()), + message: Some(automated.denial_message()), }, 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 723948b7be..bccecc610a 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -5,6 +5,8 @@ use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; +use crate::tools::approval_dispatch::AutomatedApprovalDecision; +use crate::tools::approval_dispatch::AutomatedApprovalSource; use crate::tools::hook_names::HookToolName; use crate::turn_metadata::McpTurnMetadataContext; use codex_config::CONFIG_TOML_FILE; @@ -16,6 +18,12 @@ use codex_config::types::ApprovalsReviewer; use codex_config::types::AppsConfigToml; use codex_config::types::McpServerConfig; use codex_config::types::McpServerToolConfig; +use codex_extension_api::ApprovalReviewContributor; +use codex_extension_api::ApprovalReviewError; +use codex_extension_api::ApprovalReviewInput; +use codex_extension_api::ApprovalReviewOutcome; +use codex_extension_api::ExtensionFuture; +use codex_extension_api::ExtensionRegistryBuilder; use codex_features::Features; use codex_hooks::Hooks; use codex_hooks::HooksConfig; @@ -52,6 +60,21 @@ use tracing::Level; use tracing_subscriber::fmt::format::FmtSpan; use tracing_test::internal::MockWriter; +struct DenyingMcpApprovalReviewContributor { + rationale: String, + seen_reviewer: Arc>>, +} + +impl ApprovalReviewContributor for DenyingMcpApprovalReviewContributor { + fn review<'a>( + &'a self, + input: ApprovalReviewInput<'a>, + ) -> ExtensionFuture<'a, Result> { + *self.seen_reviewer.lock().expect("reviewer lock") = Some(input.reviewer); + Box::pin(async move { Ok(ApprovalReviewOutcome::denied(self.rationale.clone())) }) + } +} + fn annotations( read_only: Option, destructive: Option, @@ -1654,62 +1677,42 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { ); } -#[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 automated_review_decision_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_automated(&AutomatedApprovalDecision { + decision: ReviewDecision::Approved, + denial_message: None, + source: AutomatedApprovalSource::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_automated(&AutomatedApprovalDecision { + decision: ReviewDecision::Denied, + denial_message: Some("extension denied this MCP tool".to_string()), + source: AutomatedApprovalSource::Extension, + }), + McpToolApprovalDecision::Decline { + message: Some("extension denied this MCP tool".to_string()), + } + ); + assert_eq!( + mcp_tool_approval_decision_from_automated(&AutomatedApprovalDecision { + decision: ReviewDecision::TimedOut, + denial_message: None, + source: AutomatedApprovalSource::Extension, + }), + McpToolApprovalDecision::Decline { + message: Some(crate::guardian::guardian_timeout_message()), + } + ); + assert_eq!( + mcp_tool_approval_decision_from_automated(&AutomatedApprovalDecision { + decision: ReviewDecision::Abort, + denial_message: Some("cancelled".to_string()), + source: AutomatedApprovalSource::Guardian, + }), McpToolApprovalDecision::Decline { message: None } ); } @@ -2675,6 +2678,69 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { ); } +#[tokio::test] +async fn approval_review_extension_denies_mcp_tool_with_effective_reviewer() { + let (mut session, mut turn_context) = make_session_and_context().await; + turn_context + .approval_policy + .set(AskForApproval::OnRequest) + .expect("test setup should allow updating approval policy"); + let mut config = (*turn_context.config).clone(); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + turn_context.config = Arc::new(config); + + let rationale = "the connector request exceeds the user's authorization"; + let seen_reviewer = Arc::new(std::sync::Mutex::new(None)); + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions.approval_review_contributor(Arc::new(DenyingMcpApprovalReviewContributor { + rationale: rationale.to_string(), + seen_reviewer: Arc::clone(&seen_reviewer), + })); + session.services.extensions = Arc::new(extensions.build()); + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: "custom_server".to_string(), + tool: "dangerous_tool".to_string(), + arguments: Some(serde_json::json!({ "calendar_id": "primary" })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations(Some(false), Some(true), Some(true))), + connector_id: None, + connector_name: None, + connector_description: None, + plugin_id: None, + tool_title: Some("Dangerous Tool".to_string()), + tool_description: Some("Reads calendar data.".to_string()), + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-extension-deny", + &invocation, + &HookToolName::new("mcp__test__tool"), + Some(&metadata), + AppToolApproval::Auto, + ) + .await; + + assert_eq!( + decision, + Some(McpToolApprovalDecision::Decline { + message: Some(rationale.to_string()), + }) + ); + assert_eq!( + *seen_reviewer.lock().expect("reviewer lock"), + Some(ApprovalsReviewer::AutoReview) + ); +} + #[tokio::test] async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval() { let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await; diff --git a/codex-rs/core/src/tools/approval_dispatch.rs b/codex-rs/core/src/tools/approval_dispatch.rs index da43a83237..428cd7c861 100644 --- a/codex-rs/core/src/tools/approval_dispatch.rs +++ b/codex-rs/core/src/tools/approval_dispatch.rs @@ -37,9 +37,11 @@ pub(crate) struct AutomatedApprovalDecision { impl AutomatedApprovalDecision { pub(crate) fn denial_message(&self) -> String { - self.denial_message - .clone() - .unwrap_or_else(|| match self.source { + self.denial_message.clone().unwrap_or_else(|| { + if self.decision == ReviewDecision::TimedOut { + return guardian_timeout_message(); + } + match self.source { AutomatedApprovalSource::Extension => { "automatic approval reviewer denied the action".to_string() } @@ -47,7 +49,8 @@ impl AutomatedApprovalDecision { "automatic approval review failed".to_string() } AutomatedApprovalSource::Guardian => "Guardian denied this request.".to_string(), - }) + } + }) } }