From 2230d64464488d8847197722fdca09d90095c705 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 12 Aug 2026 02:54:40 +0000 Subject: [PATCH] Route MCP tool calls through shared approval handling (#38108) ## What changed - Represent MCP tool calls as approval actions and route them through the session-level approval flow for permission hooks, reviewer selection, rejection handling, and resolution telemetry. - Preserve MCP-specific user prompts, session and persistent approval choices, and the approval policy and reviewer captured for the tool call. ## Testing - Add coverage for permission hooks resolving MCP calls before user or automatic review, persistence choices, captured policy routing, and MCP turn metadata with strict automatic review enabled. GitOrigin-RevId: 8e134b3d17a19dc62b3b56076d2cd05a54c9f638 --- codex-rs/core/src/mcp_tool_call.rs | 209 +++++++++++------- codex-rs/core/src/mcp_tool_call_tests.rs | 130 ++++++++++- codex-rs/core/src/tools/approvals.rs | 89 +++++++- codex-rs/core/src/tools/handlers/mcp.rs | 5 +- codex-rs/core/src/tools/mod.rs | 1 + codex-rs/core/tests/suite/hooks_mcp.rs | 201 ++++++++++++++++- .../core/tests/suite/mcp_turn_metadata.rs | 113 +++++++--- 7 files changed, 618 insertions(+), 130 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index f0080580b9..01c1e2bcbb 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -8,17 +8,17 @@ use crate::config::edit::ConfigEditsBuilder; use crate::connectors; use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianMcpAnnotations; -use crate::guardian::new_guardian_review_id; -use crate::guardian::review_approval_request; -use crate::hook_runtime::run_permission_request_hooks; +use crate::guardian::GuardianReviewContext; 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; use crate::session::session::Session; use crate::session::step_context::StepContext; use crate::session::turn_context::TurnContext; +use crate::tools::ApprovalContext; use crate::tools::hook_names::HookToolName; -use crate::tools::sandboxing::PermissionRequestPayload; +use crate::tools::sandboxing::ApprovalAction; +use crate::tools::sandboxing::ToolError; use crate::turn_metadata::McpTurnMetadataContext; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; @@ -31,12 +31,12 @@ 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::McpPermissionPromptAutoApproveContext; use codex_mcp::PreparedMcpCall; use codex_mcp::SandboxState; +use codex_mcp::ToolInfo; use codex_mcp::auth_elicitation_completed_result; use codex_mcp::build_auth_elicitation_plan; use codex_mcp::mcp_permission_prompt_is_auto_approved; @@ -72,6 +72,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::ElicitationAction; use codex_rmcp_client::ElicitationResponse; use codex_rollout::state_db; +use codex_tools::ToolName; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::truncate_text; @@ -112,12 +113,14 @@ pub(crate) async fn handle_mcp_tool_call( sess: Arc, step_context: &Arc, call_id: String, - server: String, - tool_name: String, + tool_info: &ToolInfo, hook_tool_name: HookToolName, + invocation_tool_name: ToolName, arguments: String, ) -> HandledMcpToolCall { let turn_context = &step_context.turn; + let server = tool_info.server_name.clone(); + let tool_name = tool_info.tool.name.to_string(); // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON // is not. let arguments_value = if arguments.trim().is_empty() { @@ -233,6 +236,7 @@ pub(crate) async fn handle_mcp_tool_call( step_context, &call_id, &invocation, + &invocation_tool_name, &hook_tool_name, &metadata, prepared_call.config(), @@ -1282,14 +1286,13 @@ struct McpToolApprovalKey { } fn mcp_tool_approval_prompt_options( - session_approval_key: Option<&McpToolApprovalKey>, - persistent_approval_key: Option<&McpToolApprovalKey>, + allow_session_remember: bool, + allow_persistent_approval: bool, tool_call_mcp_elicitation_enabled: bool, ) -> McpToolApprovalPromptOptions { McpToolApprovalPromptOptions { - allow_session_remember: session_approval_key.is_some(), - allow_persistent_approval: tool_call_mcp_elicitation_enabled - && persistent_approval_key.is_some(), + allow_session_remember, + allow_persistent_approval: tool_call_mcp_elicitation_enabled && allow_persistent_approval, } } @@ -1299,6 +1302,7 @@ async fn maybe_request_mcp_tool_approval( step_context: &Arc, call_id: &str, invocation: &McpInvocation, + invocation_tool_name: &ToolName, hook_tool_name: &HookToolName, metadata: &McpToolApprovalMetadata, config: &codex_mcp::McpConfig, @@ -1340,93 +1344,133 @@ async fn maybe_request_mcp_tool_approval( return Some(ReviewDecision::Approved); } - 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())), + let action = ApprovalAction::McpToolCall { + id: call_id.to_string(), + server: invocation.server.clone(), + tool_name: invocation.tool.clone(), + arguments: invocation.arguments.clone(), + connector_id: metadata.connector_id.clone(), + connector_name: metadata.connector_name.clone(), + connector_description: metadata.connector_description.clone(), + connected_account_email: (invocation.server == CODEX_APPS_MCP_SERVER_NAME) + .then(|| metadata.connected_account_email.clone()) + .flatten(), + tool_title: metadata.tool_title.clone(), + tool_description: metadata.tool_description.clone(), + annotations: metadata + .annotations + .as_ref() + .map(|annotations| GuardianMcpAnnotations { + destructive_hint: annotations.destructive_hint, + open_world_hint: annotations.open_world_hint, + read_only_hint: annotations.read_only_hint, + }), + hook_tool_name: hook_tool_name.clone(), + approval_policy: config.approval_policy.value(), + reviewer: approvals_reviewer, + approval_mode: policy.mode, + allow_session_remember: session_approval_key.is_some(), + allow_persistent_approval: persistent_approval_key.is_some(), + }; + let approval_context = ApprovalContext { + review_context: GuardianReviewContext::from(step_context), + call_id: call_id.to_string(), + tool_name: invocation_tool_name.clone(), + strict_auto_review: false, + approval_reason: None, + retry_reason: None, + network_approval_context: None, + }; + Some( + match sess.request_approval(action, approval_context).await { + Ok(decision) => decision, + Err(ToolError::Rejected(rejection)) => ReviewDecision::denied(rejection), + Err(ToolError::Codex(_)) => ReviewDecision::Abort, }, ) - .await - { - Some(PermissionRequestDecision::Allow) => { - return Some(ReviewDecision::Approved); - } - Some(PermissionRequestDecision::Deny { message }) => { - return Some(ReviewDecision::denied(message)); - } - None => {} - } +} + +pub(crate) async fn request_mcp_tool_user_approval( + sess: &Session, + turn_context: &TurnContext, + call_id: &str, + action: &ApprovalAction, +) -> ReviewDecision { + let ApprovalAction::McpToolCall { + id, + server, + tool_name, + arguments, + connector_id, + connector_name, + connector_description, + connected_account_email, + tool_title, + tool_description, + approval_mode, + allow_session_remember, + allow_persistent_approval, + .. + } = action + else { + unreachable!("only MCP actions can request MCP tool approval"); + }; let tool_call_mcp_elicitation_enabled = turn_context .config .features .enabled(Feature::ToolCallMcpElicitation); - - if matches!( - config.approval_policy.value(), - AskForApproval::OnRequest | AskForApproval::Granular(_) - ) && approvals_reviewer == ApprovalsReviewer::AutoReview - { - let review_id = new_guardian_review_id(); - let decision = review_approval_request( - sess, - step_context, - review_id.clone(), - build_guardian_mcp_tool_review_request(call_id, invocation, Some(metadata)), - Default::default(), - ) - .await; - return Some(match decision { - ReviewDecision::Abort => ReviewDecision::denied("user rejected MCP tool call"), - decision => decision, - }); - } - let prompt_options = mcp_tool_approval_prompt_options( - session_approval_key.as_ref(), - persistent_approval_key.as_ref(), + *allow_session_remember, + *allow_persistent_approval, tool_call_mcp_elicitation_enabled, ); - let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); + let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{id}"); let rendered_template = render_mcp_tool_approval_template( - &invocation.server, - metadata.connector_id.as_deref(), - metadata.connector_name.as_deref(), - metadata.tool_title.as_deref(), - invocation.arguments.as_ref(), + server, + connector_id.as_deref(), + connector_name.as_deref(), + tool_title.as_deref(), + arguments.as_ref(), ); let tool_params_display = rendered_template .as_ref() .map(|rendered_template| rendered_template.tool_params_display.clone()) - .or_else(|| build_mcp_tool_approval_display_params(invocation.arguments.as_ref())); + .or_else(|| build_mcp_tool_approval_display_params(arguments.as_ref())); let question = build_mcp_tool_approval_question( question_id.clone(), - &invocation.server, - &invocation.tool, - metadata.connector_name.as_deref(), + server, + tool_name, + connector_name.as_deref(), prompt_options, rendered_template .as_ref() .map(|rendered_template| rendered_template.question.as_str()), ); if tool_call_mcp_elicitation_enabled { - let request_id = rmcp::model::RequestId::String( - format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}").into(), - ); + let metadata = McpToolApprovalMetadata { + annotations: None, + connector_id: connector_id.clone(), + link_id: None, + connector_name: connector_name.clone(), + connector_description: connector_description.clone(), + connected_account_email: connected_account_email.clone(), + plugin_id: None, + tool_title: tool_title.clone(), + tool_description: tool_description.clone(), + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_optional_fields: None, + }; + let request_id = rmcp::model::RequestId::String(question_id.clone().into()); let request = build_mcp_tool_approval_elicitation_request(McpToolApprovalElicitationRequest { - server: &invocation.server, - metadata: Some(metadata), + server, + metadata: Some(&metadata), tool_params: rendered_template .as_ref() .and_then(|rendered_template| rendered_template.tool_params.as_ref()) - .or(invocation.arguments.as_ref()), + .or(arguments.as_ref()), tool_params_display: tool_params_display.as_deref(), question, message_override: rendered_template @@ -1435,18 +1479,12 @@ async fn maybe_request_mcp_tool_approval( prompt_options, }); let decision = parse_mcp_tool_approval_elicitation_response( - sess.request_mcp_server_elicitation( - turn_context.as_ref(), - invocation.server.clone(), - request_id, - request, - ) - .await - .response, + sess.request_mcp_server_elicitation(turn_context, server.clone(), request_id, request) + .await + .response, &question_id, ); - let decision = normalize_approval_decision_for_mode(decision, policy.mode); - return Some(decision); + return normalize_approval_decision_for_mode(decision, *approval_mode); } let args = RequestUserInputArgs { @@ -1455,13 +1493,12 @@ async fn maybe_request_mcp_tool_approval( auto_resolution_ms: None, }; let response = sess - .request_user_input(turn_context.as_ref(), call_id.to_string(), args) + .request_user_input(turn_context, 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), - policy.mode, - ); - Some(decision) + *approval_mode, + ) } pub(crate) fn mcp_approvals_reviewer( diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 54f3fea0c0..427a8d2efc 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -738,20 +738,13 @@ fn trusted_codex_apps_tool_question_offers_always_allow() { #[test] fn codex_apps_tool_question_without_elicitation_omits_always_allow() { - let session_key = McpToolApprovalKey { - server: CODEX_APPS_MCP_SERVER_NAME.to_string(), - connector_id: Some("calendar".to_string()), - tool_name: "run_action".to_string(), - }; - let persistent_key = session_key.clone(); let question = build_mcp_tool_approval_question( "q".to_string(), CODEX_APPS_MCP_SERVER_NAME, "run_action", Some("Calendar"), mcp_tool_approval_prompt_options( - Some(&session_key), - Some(&persistent_key), + /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, /*tool_call_mcp_elicitation_enabled*/ false, ), /*question_override*/ None, @@ -2331,6 +2324,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { &StepContext::for_test(Arc::clone(&turn_context)), "call-1", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), @@ -2409,6 +2403,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { &StepContext::for_test(Arc::clone(&turn_context)), "call-guardian", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), @@ -2469,6 +2464,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { &StepContext::for_test(Arc::clone(&turn_context)), "call-mcp-hook", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), @@ -2537,6 +2533,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { &StepContext::for_test(Arc::clone(&turn_context)), "call-mcp-hook-no-metadata", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), @@ -2620,6 +2617,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { &StepContext::for_test(Arc::clone(&turn_context)), "call-mcp-remembered", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__memory__create_entities"), &metadata, &approval_config(&turn_context), @@ -2635,7 +2633,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { } #[tokio::test] -async fn guardian_mode_mcp_denial_returns_rationale_message() { +async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_message() { let server = start_mock_server().await; let guardian_request_log = mount_sse_once( &server, @@ -2660,7 +2658,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { Arc::make_mut(&mut turn_context.config) .permissions .approval_policy - .set(AskForApproval::OnRequest) + .set(AskForApproval::UnlessTrusted) .expect("test setup should allow updating approval policy"); let mut config = (*turn_context.config).clone(); config.model_provider.base_url = Some(format!("{}/v1", server.uri())); @@ -2699,15 +2697,21 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { codex_apps_meta: None, openai_file_input_optional_fields: None, }; + let mut captured_mcp_config = approval_config(&turn_context); + captured_mcp_config + .approval_policy + .set(AskForApproval::OnRequest) + .expect("captured MCP policy should allow updating approval policy"); let decision = maybe_request_mcp_tool_approval( &session, &StepContext::for_test(Arc::clone(&turn_context)), "call-guardian-deny", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, - &approval_config(&turn_context), + &captured_mcp_config, McpToolApprovalPolicy::for_server(AppToolApproval::Auto), ) .await; @@ -2723,6 +2727,107 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { ); } +#[tokio::test] +async fn session_approval_preserves_mcp_session_persistence_choice() { + assert_mcp_user_approval_persistence( + MCP_TOOL_APPROVAL_PERSIST_SESSION, + ReviewDecision::ApprovedForSession, + ) + .await; +} + +#[tokio::test] +async fn session_approval_preserves_mcp_always_persistence_choice() { + assert_mcp_user_approval_persistence( + MCP_TOOL_APPROVAL_PERSIST_ALWAYS, + ReviewDecision::ApprovedMcpPolicyAmendment, + ) + .await; +} + +async fn assert_mcp_user_approval_persistence( + persistence: &'static str, + expected_decision: ReviewDecision, +) { + let (session, turn_context, rx_event) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let invocation = McpInvocation { + server: "memory".to_string(), + tool: "create_entities".to_string(), + arguments: Some(serde_json::json!({ "entities": [] })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations( + Some(false), + Some(true), + /*open_world*/ None, + )), + connector_id: None, + link_id: None, + connector_name: None, + connector_description: None, + connected_account_email: None, + plugin_id: None, + tool_title: Some("Create entities".to_string()), + tool_description: None, + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_optional_fields: None, + }; + + let approval_task = tokio::spawn({ + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + async move { + maybe_request_mcp_tool_approval( + &session, + &StepContext::for_test(Arc::clone(&turn_context)), + "call-mcp-persist", + &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), + &HookToolName::new("mcp__memory__create_entities"), + &metadata, + &approval_config(&turn_context), + McpToolApprovalPolicy::for_server(AppToolApproval::Auto), + ) + .await + } + }); + + let request = loop { + let event = tokio::time::timeout(std::time::Duration::from_secs(1), rx_event.recv()) + .await + .expect("MCP approval elicitation timed out") + .expect("expected MCP approval elicitation event"); + if let EventMsg::ElicitationRequest(request) = event.msg { + break request; + } + }; + assert_eq!(request.server_name, "memory"); + session + .resolve_elicitation( + "memory".to_string(), + rmcp::model::RequestId::String("mcp_tool_call_approval_call-mcp-persist".into()), + ElicitationResponse { + action: ElicitationAction::Accept, + content: None, + meta: Some(serde_json::json!({ + MCP_TOOL_APPROVAL_PERSIST_KEY: persistence, + })), + }, + ) + .await + .expect("MCP approval elicitation should resolve"); + + assert_eq!( + tokio::time::timeout(std::time::Duration::from_secs(1), approval_task) + .await + .expect("MCP approval task timed out") + .expect("MCP approval task failed"), + Some(expected_decision), + ); +} + #[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; @@ -2763,6 +2868,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval &StepContext::for_test(Arc::clone(&turn_context)), "call-prompt", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), @@ -2826,6 +2932,7 @@ async fn full_access_mode_skips_mcp_tool_approval_for_all_approval_modes() { &StepContext::for_test(Arc::clone(&turn_context)), "call-2", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), @@ -2917,6 +3024,7 @@ async fn approve_mode_skips_guardian_in_every_permission_mode() { &StepContext::for_test(Arc::clone(&turn_context)), "call-3", &invocation, + &ToolName::namespaced(&invocation.server, &invocation.tool), &HookToolName::new("mcp__test__tool"), &metadata, &approval_config(&turn_context), diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 8d47817e35..05b687a6f7 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -5,8 +5,9 @@ use crate::guardian::GuardianReviewContext; 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::guardian::routes_approval_policy_to_guardian; use crate::hook_runtime::run_permission_request_hooks; +use crate::mcp_tool_call::request_mcp_tool_user_approval; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; use crate::session::turn_context::TurnContext; @@ -19,14 +20,17 @@ use crate::tools::sandboxing::ApprovalRequestReasons; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::with_cached_approval; +use codex_config::types::AppToolApproval; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::approvals::ExecPolicyAmendment; #[cfg(unix)] use codex_protocol::approvals::GuardianCommandSource; use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::error::CodexErr; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; @@ -95,6 +99,25 @@ pub(crate) enum ApprovalAction { changes: Arc>, permissions_preapproved: bool, }, + McpToolCall { + id: String, + server: String, + tool_name: String, + arguments: Option, + connector_id: Option, + connector_name: Option, + connector_description: Option, + connected_account_email: Option, + tool_title: Option, + tool_description: Option, + annotations: Option, + hook_tool_name: HookToolName, + approval_policy: AskForApproval, + reviewer: ApprovalsReviewer, + approval_mode: AppToolApproval, + allow_session_remember: bool, + allow_persistent_approval: bool, + }, } #[derive(Clone, Debug, Eq, Hash, PartialEq, serde::Serialize)] @@ -127,6 +150,16 @@ impl ApprovalAction { tool_name: HookToolName::apply_patch(), tool_input: serde_json::json!({ "command": patch }), }, + Self::McpToolCall { + hook_tool_name, + arguments, + .. + } => PermissionRequestPayload { + tool_name: hook_tool_name.clone(), + tool_input: arguments + .clone() + .unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())), + }, } } @@ -164,6 +197,7 @@ impl ApprovalAction { })], #[cfg(unix)] Self::Execve { .. } => Vec::new(), + Self::McpToolCall { .. } => Vec::new(), Self::ApplyPatch { environment_id, files, @@ -252,6 +286,32 @@ impl ApprovalAction { .collect::>>()?, patch, }, + Self::McpToolCall { + id, + server, + tool_name, + arguments, + connector_id, + connector_name, + connector_description, + connected_account_email, + tool_title, + tool_description, + annotations, + .. + } => crate::guardian::GuardianApprovalRequest::McpToolCall { + id, + server, + tool_name, + arguments, + connector_id, + connector_name, + connector_description, + connected_account_email, + tool_title, + tool_description, + annotations, + }, }) } } @@ -286,7 +346,11 @@ enum ApprovalReviewer { impl ApprovalReviewer { fn for_turn(turn: &TurnContext) -> Self { - if routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) { + Self::for_policy(turn.approval_policy(), turn.config.approvals_reviewer) + } + + fn for_policy(approval_policy: AskForApproval, reviewer: ApprovalsReviewer) -> Self { + if routes_approval_policy_to_guardian(approval_policy, reviewer) { Self::Guardian } else { Self::User @@ -343,6 +407,7 @@ impl Session { action: ApprovalAction, ctx: ApprovalContext, ) -> Result { + let is_mcp_tool_call = matches!(&action, ApprovalAction::McpToolCall { .. }); let permission_request_run_id = match &action { #[cfg(unix)] ApprovalAction::Execve { approval_id, .. } => approval_id.clone(), @@ -372,6 +437,9 @@ impl Session { None => self.request_reviewer_approval(action, &ctx).await, }; record_resolution(&ctx, &resolution); + if is_mcp_tool_call && resolution.decision == ReviewDecision::ApprovedMcpPolicyAmendment { + return Ok(resolution.decision); + } resolution.into_tool_result() } @@ -382,6 +450,13 @@ impl Session { ) -> ApprovalResolution { let reviewer = if ctx.strict_auto_review { ApprovalReviewer::Guardian + } else if let ApprovalAction::McpToolCall { + approval_policy, + reviewer, + .. + } = &action + { + ApprovalReviewer::for_policy(*approval_policy, *reviewer) } else { ApprovalReviewer::for_turn(ctx.review_context.turn()) }; @@ -465,6 +540,7 @@ impl Session { #[cfg(unix)] ApprovalAction::Execve { .. } => unreachable!("matched command approval"), ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"), + ApprovalAction::McpToolCall { .. } => unreachable!("matched command approval"), }; let reason = ctx .retry_reason @@ -555,6 +631,15 @@ impl Session { ) .await } + ApprovalAction::McpToolCall { .. } => { + request_mcp_tool_user_approval( + self, + ctx.review_context.turn(), + &ctx.call_id, + action, + ) + .await + } } } } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 893fd0bae3..664096d788 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -166,6 +166,7 @@ impl McpHandler { session, step_context, call_id, + tool_name, payload, .. } = invocation; @@ -185,9 +186,9 @@ impl McpHandler { Arc::clone(&session), &step_context, call_id.clone(), - self.tool_info.server_name.clone(), - self.tool_info.tool.name.to_string(), + &self.tool_info, self.hook_tool_name(), + tool_name, payload, ) .await; diff --git a/codex-rs/core/src/tools/mod.rs b/codex-rs/core/src/tools/mod.rs index 314c6ed1b9..3ca459a83e 100644 --- a/codex-rs/core/src/tools/mod.rs +++ b/codex-rs/core/src/tools/mod.rs @@ -21,6 +21,7 @@ mod tool_namespaces_info; use std::borrow::Cow; use crate::session::turn_context::TurnContext; +pub(crate) use approvals::ApprovalContext; use codex_features::Feature; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::openai_models::ToolMode; diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 7423bc0678..9ea3b10c37 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -6,10 +6,13 @@ use std::time::Duration; use anyhow::Context; use anyhow::Result; use codex_config::types::AppToolApproval; +use codex_config::types::ApprovalsReviewer; use codex_config::types::McpServerConfig; use codex_config::types::McpServerTransportConfig; use codex_core::config::Config; use codex_features::Feature; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; use core_test_support::hooks::trust_discovered_hooks; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -20,6 +23,7 @@ use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; use core_test_support::stdio_server_bin; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_mcp_server; @@ -27,6 +31,9 @@ use pretty_assertions::assert_eq; use serde_json::Value; use serde_json::json; +use super::rmcp_client::remote_aware_environment_id; +use super::rmcp_client::remote_aware_stdio_server_bin; + const RMCP_SERVER: &str = "rmcp"; const RMCP_PREFIXED_NAMESPACE: &str = "mcp__rmcp"; const RMCP_UNPREFIXED_NAMESPACE: &str = "rmcp"; @@ -34,6 +41,12 @@ const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo"; const RMCP_HOOK_MATCHER: &str = RMCP_ECHO_TOOL_NAME; const RMCP_ECHO_MESSAGE: &str = "hook e2e ping"; +#[derive(Clone, Copy)] +enum PermissionRequestHookOutcome { + Allow, + Deny(&'static str), +} + fn enable_mcp_tool_name_features(config: &mut Config, prefix_mcp_tool_names: bool) { if !prefix_mcp_tool_names { let _ = config.features.enable(Feature::NonPrefixedMcpToolNames); @@ -170,6 +183,56 @@ print(json.dumps({{ Ok(()) } +fn write_permission_request_hook(home: &Path, outcome: PermissionRequestHookOutcome) -> Result<()> { + let script_path = home.join("permission_request_hook.py"); + let log_path = home.join("permission_request_hook_log.jsonl"); + let decision = match outcome { + PermissionRequestHookOutcome::Allow => json!({ "behavior": "allow" }), + PermissionRequestHookOutcome::Deny(message) => { + json!({ "behavior": "deny", "message": message }) + } + }; + let hook_output = json!({ + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": decision, + } + }); + let python_output_literal = serde_json::to_string(&hook_output.to_string()) + .context("serialize MCP permission request hook output")?; + let script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) + +with Path(r"{log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") + +print({python_output_literal}) +"#, + log_path = log_path.display(), + ); + let hooks = json!({ + "hooks": { + "PermissionRequest": [{ + "matcher": RMCP_HOOK_MATCHER, + "hooks": [{ + "type": "command", + "command": format!("python3 {}", script_path.display()), + "statusMessage": "running MCP permission request hook", + }] + }] + } + }); + + fs::write(&script_path, script).context("write MCP permission request hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()) + .context("write MCP permission request hooks")?; + Ok(()) +} + fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { fs::read_to_string(home.join(log_name)) .with_context(|| format!("read {log_name}"))? @@ -179,7 +242,12 @@ fn read_hook_inputs(home: &Path, log_name: &str) -> Result> { .collect() } -fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: AppToolApproval) { +fn insert_rmcp_test_server( + config: &mut Config, + command: String, + approval_mode: AppToolApproval, + environment_id: String, +) { let mut servers = config.mcp_servers.get().clone(); servers.insert( RMCP_SERVER.to_string(), @@ -192,7 +260,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: env_vars: Vec::new(), cwd: None, }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + environment_id, enabled: true, required: false, supports_parallel_tool_calls: false, @@ -223,7 +291,134 @@ fn enable_hooks_and_rmcp_server( ) { trust_discovered_hooks(config); enable_mcp_tool_name_features(config, prefix_mcp_tool_names); - insert_rmcp_test_server(config, rmcp_test_server_bin, approval_mode); + insert_rmcp_test_server( + config, + rmcp_test_server_bin, + approval_mode, + codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + ); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_allows_mcp_tool_without_user_or_guardian_review() -> Result<()> { + run_mcp_permission_request_hook_test(PermissionRequestHookOutcome::Allow).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_denies_mcp_tool_without_user_or_guardian_review() -> Result<()> { + run_mcp_permission_request_hook_test(PermissionRequestHookOutcome::Deny( + "MCP tool access denied by the integration-test hook", + )) + .await +} + +async fn run_mcp_permission_request_hook_test(outcome: PermissionRequestHookOutcome) -> Result<()> { + skip_if_wine_exec!( + Ok(()), + "requires a Windows test_stdio_server in the Wine-exec environment" + ); + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = match outcome { + PermissionRequestHookOutcome::Allow => "permissionrequest-rmcp-allow", + PermissionRequestHookOutcome::Deny(_) => "permissionrequest-rmcp-deny", + }; + let arguments = json!({ "message": RMCP_ECHO_MESSAGE }).to_string(); + let rmcp_test_server_bin = remote_aware_stdio_server_bin()?; + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + write_permission_request_hook(home, outcome) + .expect("failed to write MCP permission request hook fixture"); + }) + .with_config(move |config| { + trust_discovered_hooks(config); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + insert_rmcp_test_server( + config, + rmcp_test_server_bin, + AppToolApproval::Prompt, + remote_aware_environment_id(), + ); + }); + let test = builder.build_with_remote_and_local_env(&server).await?; + wait_for_mcp_server(&test.codex, RMCP_SERVER).await?; + + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-mcp-permission-hook-1"), + ev_function_call_with_namespace( + call_id, + RMCP_PREFIXED_NAMESPACE, + "echo", + &arguments, + ), + ev_completed("resp-mcp-permission-hook-1"), + ]), + sse(vec![ + ev_response_created("resp-mcp-permission-hook-2"), + ev_assistant_message("msg-mcp-permission-hook", "done"), + ev_completed("resp-mcp-permission-hook-2"), + ]), + ], + ) + .await; + + test.submit_turn_with_approval_and_permission_profile( + "call the rmcp echo tool with the MCP permission request hook", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + assert!( + requests.iter().all(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"].as_str() != Some("guardian") + }), + "a permission request hook should resolve MCP approval before Guardian review", + ); + + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("MCP tool output should be a string"); + match outcome { + PermissionRequestHookOutcome::Allow => assert!( + output.contains(&format!("ECHOING: {RMCP_ECHO_MESSAGE}")), + "an allowed MCP tool should execute", + ), + PermissionRequestHookOutcome::Deny(message) => assert!( + output.contains(message), + "a denied MCP tool should surface the hook's rejection message", + ), + } + + let hook_inputs = + read_hook_inputs(test.codex_home_path(), "permission_request_hook_log.jsonl")?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + json!({ + "hook_event_name": hook_inputs[0]["hook_event_name"], + "tool_name": hook_inputs[0]["tool_name"], + "tool_input": hook_inputs[0]["tool_input"], + }), + json!({ + "hook_event_name": "PermissionRequest", + "tool_name": RMCP_ECHO_TOOL_NAME, + "tool_input": { "message": RMCP_ECHO_MESSAGE }, + }), + ); + assert!( + hook_inputs[0].get("tool_use_id").is_none(), + "PermissionRequest input should not include a tool_use_id", + ); + + Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index d6c6474487..e1cd35e240 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -14,11 +14,15 @@ use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; use codex_protocol::items::McpToolCallStatus; use codex_protocol::items::TurnItem; +use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ElicitationAction; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::request_permissions::PermissionGrantScope; +use codex_protocol::request_permissions::RequestPermissionProfile; +use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputAnswer; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; @@ -156,7 +160,11 @@ async fn wait_for_mcp_tool_call_item( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> Result<()> { +#[test_case(false; "without strict auto review")] +#[test_case(true; "with strict auto review")] +async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( + strict_auto_review: bool, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -166,36 +174,65 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R "title": "Lunch", "starts_at": "2026-03-10T12:00:00Z" }))?; - let mock = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-1"), - ev_function_call_with_namespace( - call_id, - SEARCH_CALENDAR_NAMESPACE, - SEARCH_CALENDAR_CREATE_TOOL, - &calendar_args, - ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "done"), - ev_completed("resp-2"), - ]), - ], - ) - .await; + let mut response_sequence = Vec::new(); + if strict_auto_review { + let requested_permissions = RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }; + let request_permissions_args = json!({ + "reason": "Enable strict auto review before the MCP approval", + "permissions": requested_permissions, + }); + response_sequence.push(sse(vec![ + ev_response_created("resp-permissions"), + ev_function_call( + "calendar-strict-permissions", + "request_permissions", + &serde_json::to_string(&request_permissions_args)?, + ), + ev_completed("resp-permissions"), + ])); + } + response_sequence.extend([ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call_with_namespace( + call_id, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL, + &calendar_args, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + ]); + let mock = mount_sse_sequence(&server, response_sequence).await; let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone()) - .with_config(|config| { - // Use the opposite global reviewer so this route must come from apps._default. - config.approvals_reviewer = ApprovalsReviewer::AutoReview; + .with_config(move |config| { + // The permission grant needs user review before strict review applies to the turn. + config.approvals_reviewer = if strict_auto_review { + ApprovalsReviewer::User + } else { + ApprovalsReviewer::AutoReview + }; config .features .enable(Feature::ToolCallMcpElicitation) .expect("test config should allow feature update"); + if strict_auto_review { + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("test config should allow feature update"); + } set_default_app_approval_mode_and_reviewer( config, AppToolApproval::Prompt, @@ -213,6 +250,30 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R ) .await?; + if strict_auto_review { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::RequestPermissions(request) = event else { + panic!("expected permission request before MCP approval, received {event:?}"); + }; + assert_eq!(request.call_id, "calendar-strict-permissions"); + test.codex + .submit(Op::RequestPermissionsResponse { + id: request.call_id, + response: RequestPermissionsResponse { + permissions: request.permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + }) + .await?; + } + let EventMsg::McpToolCallBegin(begin) = wait_for_event(&test.codex, |event| { matches!(event, EventMsg::McpToolCallBegin(_)) }) @@ -248,7 +309,7 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R }) .await; - assert_eq!(mock.requests().len(), 2); + assert_eq!(mock.requests().len(), 2 + usize::from(strict_auto_review)); let apps_tool_call = recorded_apps_tool_call_by_call_id(&server, call_id).await; assert_eq!(