diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index fb68d88e3f..839aa786b0 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -974,10 +974,9 @@ async fn maybe_request_mcp_tool_approval( ); let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref()); - let approval_required = requires_mcp_tool_approval(annotations); let approval_required = !auto_approved_by_permissions - && (approval_required || approval_mode == AppToolApproval::Prompt); - if !pre_tool_use_allows && !approval_required && !pre_tool_use_asks_user { + && (requires_mcp_tool_approval(annotations) || approval_mode == AppToolApproval::Prompt); + if !approval_required && !pre_tool_use_asks_user { return None; } @@ -1006,21 +1005,27 @@ async fn maybe_request_mcp_tool_approval( } } } - if pre_tool_use_allows { + let must_prompt_user = monitor_reason.is_some() || pre_tool_use_asks_user; + + if auto_approved_by_policy && approval_required && !must_prompt_user { + return None; + } + if pre_tool_use_allows && !must_prompt_user { return None; } let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode); let persistent_approval_key = persistent_mcp_tool_approval_key(invocation, metadata, approval_mode); - if pre_tool_use_permission_decision.is_none() + if !must_prompt_user + && pre_tool_use_permission_decision.is_none() && let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { return Some(McpToolApprovalDecision::Accept); } - if pre_tool_use_permission_decision.is_none() { + if !must_prompt_user && pre_tool_use_permission_decision.is_none() { match run_permission_request_hooks( sess, turn_context, @@ -1052,7 +1057,7 @@ async fn maybe_request_mcp_tool_approval( .features .enabled(Feature::ToolCallMcpElicitation); - if routes_approval_to_guardian(turn_context) && !pre_tool_use_asks_user { + if routes_approval_to_guardian(turn_context) && !must_prompt_user { let review_id = new_guardian_review_id(); let decision = review_approval_request( sess, @@ -1074,9 +1079,14 @@ async fn maybe_request_mcp_tool_approval( return Some(decision); } + let (prompt_session_approval_key, prompt_persistent_approval_key) = if must_prompt_user { + (None, None) + } else { + (session_approval_key, persistent_approval_key) + }; let prompt_options = mcp_tool_approval_prompt_options( - session_approval_key.as_ref(), - persistent_approval_key.as_ref(), + prompt_session_approval_key.as_ref(), + prompt_persistent_approval_key.as_ref(), tool_call_mcp_elicitation_enabled, ); let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); @@ -1101,12 +1111,11 @@ async fn maybe_request_mcp_tool_approval( .as_ref() .map(|rendered_template| rendered_template.question.as_str()), ); - let prompt_reason = match &pre_tool_use_permission_decision { - Some(PreToolUsePermissionDecision::Ask { reason }) => { - reason.as_deref().or(monitor_reason.as_deref()) - } - Some(PreToolUsePermissionDecision::Allow { .. }) | None => monitor_reason.as_deref(), + let pre_tool_use_reason = match &pre_tool_use_permission_decision { + Some(PreToolUsePermissionDecision::Ask { reason }) => reason.as_deref(), + Some(PreToolUsePermissionDecision::Allow { .. }) | None => None, }; + let prompt_reason = monitor_reason.as_deref().or(pre_tool_use_reason); question.question = mcp_tool_approval_question_text(question.question, prompt_reason); if tool_call_mcp_elicitation_enabled { let request_id = rmcp::model::RequestId::String( @@ -1142,8 +1151,8 @@ async fn maybe_request_mcp_tool_approval( sess, turn_context, &decision, - session_approval_key, - persistent_approval_key, + prompt_session_approval_key, + prompt_persistent_approval_key, ) .await; return Some(decision); @@ -1163,8 +1172,8 @@ async fn maybe_request_mcp_tool_approval( sess, turn_context, &decision, - session_approval_key, - persistent_approval_key, + prompt_session_approval_key, + prompt_persistent_approval_key, ) .await; Some(decision) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 524138f017..c8308603fd 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2399,6 +2399,243 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { ); } +#[tokio::test] +async fn pre_tool_use_allow_runs_mcp_after_arc_ok() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/codex/safety/arc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "outcome": "ok", + "short_reason": "", + "rationale": "", + "risk_score": 1, + "risk_level": "low", + "evidence": [], + }))) + .expect(1) + .mount(&server) + .await; + + let (session, mut turn_context) = make_session_and_context().await; + turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth( + codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(), + )); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + turn_context.config = Arc::new(config); + turn_context.record_pre_tool_use_permission_decision( + "call-pretooluse-allow-arc-ok".to_string(), + codex_hooks::PreToolUsePermissionDecision::Allow { + reason: Some("approved by hook".to_string()), + }, + ); + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "dangerous_tool".to_string(), + arguments: Some(serde_json::json!({ "id": 1 })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations(Some(false), Some(true), Some(true))), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + connector_description: Some("Manage events".to_string()), + tool_title: Some("Dangerous Tool".to_string()), + tool_description: Some("Performs a risky action.".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-pretooluse-allow-arc-ok", + &invocation, + "mcp__test__tool", + Some(&metadata), + AppToolApproval::Approve, + ) + .await; + + assert_eq!(decision, None); +} + +#[tokio::test] +async fn pre_tool_use_allow_still_prompts_when_arc_asks_user() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/codex/safety/arc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "outcome": "ask-user", + "short_reason": "needs confirmation", + "rationale": "ARC wants a second review", + "risk_score": 65, + "risk_level": "medium", + "evidence": [{ + "message": "dangerous_tool", + "why": "requires review", + }], + }))) + .expect(1) + .mount(&server) + .await; + + let (session, mut turn_context, _rx_event) = make_session_and_context_with_rx().await; + { + let mut active_turn = session.active_turn.lock().await; + *active_turn = Some(ActiveTurn::default()); + } + { + let turn_context = Arc::get_mut(&mut turn_context).expect("single turn context ref"); + turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth( + codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(), + )); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + turn_context.config = Arc::new(config); + turn_context.record_pre_tool_use_permission_decision( + "call-pretooluse-allow-arc-ask".to_string(), + codex_hooks::PreToolUsePermissionDecision::Allow { + reason: Some("approved by hook".to_string()), + }, + ); + } + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "dangerous_tool".to_string(), + arguments: Some(serde_json::json!({ "id": 1 })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations(Some(false), Some(true), Some(true))), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + connector_description: Some("Manage events".to_string()), + tool_title: Some("Dangerous Tool".to_string()), + tool_description: Some("Performs a risky action.".to_string()), + mcp_app_resource_uri: None, + codex_apps_meta: None, + openai_file_input_params: None, + }; + + let mut approval_task = { + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + tokio::spawn(async move { + maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-pretooluse-allow-arc-ask", + &invocation, + "mcp__test__tool", + Some(&metadata), + AppToolApproval::Approve, + ) + .await + }) + }; + + assert!( + tokio::time::timeout(std::time::Duration::from_millis(200), &mut approval_task) + .await + .is_err(), + "ARC ask-user should still wait for user approval after pre-tool-use allow" + ); + approval_task.abort(); +} + +#[tokio::test] +async fn pre_tool_use_allow_still_blocks_when_arc_steers_model() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/codex/safety/arc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "outcome": "steer-model", + "short_reason": "needs approval", + "rationale": "high-risk action", + "risk_score": 96, + "risk_level": "critical", + "evidence": [{ + "message": "dangerous_tool", + "why": "high-risk action", + }], + }))) + .expect(1) + .mount(&server) + .await; + + let (session, mut turn_context) = make_session_and_context().await; + turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth( + codex_login::CodexAuth::create_dummy_chatgpt_auth_for_testing(), + )); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + turn_context.config = Arc::new(config); + turn_context.record_pre_tool_use_permission_decision( + "call-pretooluse-allow-arc-steer".to_string(), + codex_hooks::PreToolUsePermissionDecision::Allow { + reason: Some("approved by hook".to_string()), + }, + ); + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "dangerous_tool".to_string(), + arguments: Some(serde_json::json!({ "id": 1 })), + }; + let metadata = McpToolApprovalMetadata { + annotations: Some(annotations(Some(false), Some(true), Some(true))), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + connector_description: Some("Manage events".to_string()), + tool_title: Some("Dangerous Tool".to_string()), + tool_description: Some("Performs a risky action.".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-pretooluse-allow-arc-steer", + &invocation, + "mcp__test__tool", + Some(&metadata), + AppToolApproval::Approve, + ) + .await; + + assert_eq!( + decision, + Some(McpToolApprovalDecision::BlockedBySafetyMonitor( + "Tool call was cancelled because of safety risks: high-risk action".to_string(), + )) + ); +} + #[tokio::test] async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { use wiremock::Mock;