diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 3cfd4d01e..d689ea904 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -71,12 +71,7 @@ pub fn mcp_permission_prompt_is_auto_approved( permission_profile: &PermissionProfile, context: McpPermissionPromptAutoApproveContext, ) -> bool { - if matches!( - approval_policy, - AskForApproval::OnRequest | AskForApproval::Granular(_) - ) && context.approvals_reviewer == Some(ApprovalsReviewer::AutoReview) - && context.tool_approval_mode == Some(AppToolApproval::Approve) - { + if context.tool_approval_mode == Some(AppToolApproval::Approve) { return true; } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index fa5cbf1f7..94ddd43b3 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -74,16 +74,11 @@ fn mcp_prompt_auto_approval_honors_unrestricted_managed_profiles() { } #[test] -fn mcp_prompt_auto_approval_honors_auto_review_approved_tools() { - assert!(mcp_permission_prompt_is_auto_approved( +fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() { + for approval_policy in [ + AskForApproval::UnlessTrusted, + AskForApproval::OnFailure, AskForApproval::OnRequest, - &PermissionProfile::read_only(), - McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - tool_approval_mode: Some(AppToolApproval::Approve), - }, - )); - assert!(mcp_permission_prompt_is_auto_approved( AskForApproval::Granular(GranularApprovalConfig { sandbox_approval: true, rules: true, @@ -91,34 +86,36 @@ fn mcp_prompt_auto_approval_honors_auto_review_approved_tools() { request_permissions: true, mcp_elicitations: true, }), + AskForApproval::Never, + ] { + assert!(mcp_permission_prompt_is_auto_approved( + approval_policy, + &PermissionProfile::read_only(), + McpPermissionPromptAutoApproveContext { + approvals_reviewer: Some(ApprovalsReviewer::User), + tool_approval_mode: Some(AppToolApproval::Approve), + }, + )); + } + + assert!(!mcp_permission_prompt_is_auto_approved( + AskForApproval::OnRequest, &PermissionProfile::read_only(), McpPermissionPromptAutoApproveContext { approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - tool_approval_mode: Some(AppToolApproval::Approve), + tool_approval_mode: Some(AppToolApproval::Auto), }, )); +} + +#[test] +fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() { assert!(!mcp_permission_prompt_is_auto_approved( AskForApproval::OnRequest, &PermissionProfile::read_only(), McpPermissionPromptAutoApproveContext { approvals_reviewer: Some(ApprovalsReviewer::User), - tool_approval_mode: Some(AppToolApproval::Approve), - }, - )); - assert!(!mcp_permission_prompt_is_auto_approved( - AskForApproval::OnFailure, - &PermissionProfile::read_only(), - McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - tool_approval_mode: Some(AppToolApproval::Approve), - }, - )); - assert!(!mcp_permission_prompt_is_auto_approved( - AskForApproval::UnlessTrusted, - &PermissionProfile::read_only(), - McpPermissionPromptAutoApproveContext { - approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - tool_approval_mode: Some(AppToolApproval::Approve), + tool_approval_mode: Some(AppToolApproval::Auto), }, )); } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 524138f01..d83307e80 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -18,6 +18,7 @@ use codex_hooks::HooksConfig; use codex_model_provider::create_model_provider; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::GranularApprovalConfig; use core_test_support::PathExt; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -2328,7 +2329,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval } #[tokio::test] -async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { +async fn approve_mode_skips_arc_interrupt_for_model() { use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -2349,7 +2350,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { "why": "high-risk action", }], }))) - .expect(1) + .expect(0) .mount(&server) .await; @@ -2391,16 +2392,11 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { ) .await; - assert_eq!( - decision, - Some(McpToolApprovalDecision::BlockedBySafetyMonitor( - "Tool call was cancelled because of safety risks: high-risk action".to_string(), - )) - ); + assert_eq!(decision, None); } #[tokio::test] -async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { +async fn custom_approve_mode_skips_arc_interrupt_for_model() { use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -2421,7 +2417,7 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { "why": "high-risk action", }], }))) - .expect(1) + .expect(0) .mount(&server) .await; @@ -2463,16 +2459,11 @@ async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() { ) .await; - assert_eq!( - decision, - Some(McpToolApprovalDecision::BlockedBySafetyMonitor( - "Tool call was cancelled because of safety risks: high-risk action".to_string(), - )) - ); + assert_eq!(decision, None); } #[tokio::test] -async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { +async fn approve_mode_skips_arc_interrupt_without_annotations() { use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -2493,7 +2484,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { "why": "high-risk action", }], }))) - .expect(1) + .expect(0) .mount(&server) .await; @@ -2535,12 +2526,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { ) .await; - assert_eq!( - decision, - Some(McpToolApprovalDecision::BlockedBySafetyMonitor( - "Tool call was cancelled because of safety risks: high-risk action".to_string(), - )) - ); + assert_eq!(decision, None); } #[tokio::test] @@ -2622,7 +2608,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { } #[tokio::test] -async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() { +async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() { use wiremock::Mock; use wiremock::ResponseTemplate; use wiremock::matchers::method; @@ -2652,33 +2638,6 @@ async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() .mount(&server) .await; - let (mut 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(), - )); - turn_context - .approval_policy - .set(AskForApproval::OnRequest) - .expect("test setup should allow updating approval policy"); - let mut config = (*turn_context.config).clone(); - config.chatgpt_base_url = server.uri(); - config.model_provider.base_url = Some(format!("{}/v1", server.uri())); - config.approvals_reviewer = ApprovalsReviewer::AutoReview; - let config = Arc::new(config); - let models_manager = models_manager_with_provider( - config.codex_home.to_path_buf(), - Arc::clone(&session.services.auth_manager), - config.model_provider.clone(), - ); - session.services.models_manager = models_manager; - turn_context.config = Arc::clone(&config); - turn_context.provider = create_model_provider( - config.model_provider.clone(), - turn_context.auth_manager.clone(), - ); - - 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(), @@ -2696,16 +2655,57 @@ async fn approve_mode_skips_arc_and_guardian_when_guardian_reviewer_is_enabled() openai_file_input_params: None, }; - let decision = maybe_request_mcp_tool_approval( - &session, - &turn_context, - "call-3", - &invocation, - "mcp__test__tool", - Some(&metadata), - AppToolApproval::Approve, - ) - .await; + for approval_policy in [ + AskForApproval::UnlessTrusted, + AskForApproval::OnFailure, + AskForApproval::OnRequest, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + AskForApproval::Never, + ] { + let (mut 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(), + )); + turn_context + .approval_policy + .set(approval_policy) + .expect("test setup should allow updating approval policy"); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + config.model_provider.base_url = Some(format!("{}/v1", server.uri())); + config.approvals_reviewer = ApprovalsReviewer::User; + let config = Arc::new(config); + let models_manager = models_manager_with_provider( + config.codex_home.to_path_buf(), + Arc::clone(&session.services.auth_manager), + config.model_provider.clone(), + ); + session.services.models_manager = models_manager; + turn_context.config = Arc::clone(&config); + turn_context.provider = create_model_provider( + config.model_provider.clone(), + turn_context.auth_manager.clone(), + ); - assert_eq!(decision, None); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-3", + &invocation, + "mcp__test__tool", + Some(&metadata), + AppToolApproval::Approve, + ) + .await; + + assert_eq!(decision, None); + } }