diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 56229b3e87..73decd4504 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1297,6 +1297,16 @@ async fn maybe_request_mcp_tool_approval( policy: McpToolApprovalPolicy, ) -> Option { let turn_context = &step_context.turn; + let turn_state = sess + .active_turn + .lock() + .await + .as_ref() + .map(|active| Arc::clone(&active.turn_state)); + let strict_auto_review = match turn_state { + Some(turn_state) => turn_state.lock().await.strict_auto_review_enabled(), + None => false, + }; let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( &config.config_layer_stack, config.approvals_reviewer, @@ -1304,18 +1314,20 @@ async fn maybe_request_mcp_tool_approval( &invocation.server, metadata.connector_id.as_deref(), ); - if mcp_permission_prompt_is_auto_approved( - config.approval_policy.value(), - &config.permission_profile, - McpPermissionPromptAutoApproveContext { - tool_approval_mode: Some(policy.mode), - }, - ) { + if !strict_auto_review + && mcp_permission_prompt_is_auto_approved( + config.approval_policy.value(), + &config.permission_profile, + McpPermissionPromptAutoApproveContext { + tool_approval_mode: Some(policy.mode), + }, + ) + { return None; } let annotations = metadata.annotations.as_ref(); - if !requires_mcp_tool_approval_for_mode(annotations, policy.mode) { + if !strict_auto_review && !requires_mcp_tool_approval_for_mode(annotations, policy.mode) { return None; } @@ -1326,7 +1338,8 @@ async fn maybe_request_mcp_tool_approval( } else { None }; - if let Some(key) = session_approval_key.as_ref() + if !strict_auto_review + && let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { return Some(ReviewDecision::Approved); @@ -1364,7 +1377,7 @@ async fn maybe_request_mcp_tool_approval( review_context: GuardianReviewContext::from(step_context), call_id: call_id.to_string(), tool_name: invocation_tool_name.clone(), - strict_auto_review: false, + strict_auto_review, approval_reason: None, retry_reason: None, network_approval_context: None, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index f2844830ad..6cd51640dd 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -2628,7 +2628,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { } #[tokio::test] -async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_message() { +async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { let server = start_mock_server().await; let guardian_request_log = mount_sse_once( &server, @@ -2657,7 +2657,7 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes .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())); - config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config.approvals_reviewer = ApprovalsReviewer::User; let config = Arc::new(config); let models_manager = models_manager_with_provider( config.codex_home.to_path_buf(), @@ -2671,6 +2671,13 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes turn_context.auth_manager.clone(), ); + let active_turn = ActiveTurn::default(); + active_turn + .turn_state + .lock() + .await + .enable_strict_auto_review(); + *session.active_turn.lock().await = Some(active_turn); let session = Arc::new(session); let turn_context = Arc::new(turn_context); let invocation = McpInvocation { @@ -2707,7 +2714,7 @@ async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_mes &HookToolName::new("mcp__test__tool"), &metadata, &captured_mcp_config, - McpToolApprovalPolicy::for_server(AppToolApproval::Auto), + McpToolApprovalPolicy::for_server(AppToolApproval::Approve), ) .await; diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index 941c6f3390..6f0208652b 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -196,23 +196,37 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( 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, + response_sequence.push(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"), + ])); + if strict_auto_review { + response_sequence.push(sse(vec![ + ev_response_created("resp-guardian-review"), + ev_assistant_message( + "msg-guardian-review", + &json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "Creating this calendar event is low risk.", + }) + .to_string(), ), - ev_completed("resp-1"), - ]), - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "done"), - ev_completed("resp-2"), - ]), - ]); + ev_completed("resp-guardian-review"), + ])); + } + response_sequence.push(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()) @@ -283,26 +297,28 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( }; assert_eq!(begin.call_id, call_id); - let EventMsg::ElicitationRequest(request) = wait_for_event(&test.codex, |event| { - matches!( - event, - EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_) - ) - }) - .await - else { - panic!("expected apps._default user to route the app approval to the user"); - }; - - test.codex - .submit(Op::ResolveElicitation { - server_name: request.server_name, - request_id: request.id, - decision: ElicitationAction::Accept, - content: None, - meta: None, + if !strict_auto_review { + let EventMsg::ElicitationRequest(request) = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_) + ) }) - .await?; + .await + else { + panic!("expected apps._default user to route the app approval to the user"); + }; + + test.codex + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision: ElicitationAction::Accept, + content: None, + meta: None, + }) + .await?; + } wait_for_event(&test.codex, |event| { matches!(event, EventMsg::TurnComplete(_)) @@ -310,7 +326,10 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( .await; let response_requests = mock.requests(); - assert_eq!(response_requests.len(), 2 + usize::from(strict_auto_review)); + assert_eq!( + response_requests.len(), + 2 + 2 * usize::from(strict_auto_review) + ); let response_body = response_requests[0].body_json(); let turn_id = response_body["client_metadata"]["turn_id"] .as_str() @@ -335,7 +354,7 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( assert_eq!( apps_tool_call .pointer("/params/_meta/x-codex-turn-metadata/user_input_requested_during_turn"), - Some(&json!(true)) + (!strict_auto_review).then_some(&json!(true)) ); Ok(())