diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 95ea7e6548..2d3ff975f3 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -97,7 +97,6 @@ use codex_app_server_protocol::item_event_to_server_notification; use codex_core::CodexThread; use codex_core::ThreadManager; use codex_features::Feature; -use codex_guardian_v2::StrictReviewReason; use codex_protocol::ThreadId; use codex_protocol::items::CollabAgentTool as CoreCollabAgentTool; use codex_protocol::items::TurnItem as CoreTurnItem; @@ -355,21 +354,16 @@ pub(crate) async fn apply_bespoke_event_handling( ); outgoing.send_server_notification(notification).await; if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress - && match assessment.review_reason { - Some(reason) => matches!( - reason, + && matches!( + assessment.review_reason, + Some( codex_protocol::approvals::GuardianReviewReason::ElevatedRisk | codex_protocol::approvals::GuardianReviewReason::StaleScore | codex_protocol::approvals::GuardianReviewReason::IncompatibleCompaction | codex_protocol::approvals::GuardianReviewReason::ScoringFailure | codex_protocol::approvals::GuardianReviewReason::AuthorizationChanged - ), - // MCP elicitations migrate to per-approval reasons next. - None => conversation - .thread_extension_data() - .remove::() - .is_some(), - } + ) + ) { outgoing .send_server_notification(ServerNotification::StrictReviewRequired( diff --git a/codex-rs/app-server/tests/suite/v2/guardian_code_mode_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_code_mode_tests.rs new file mode 100644 index 0000000000..bd58be5eb3 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/guardian_code_mode_tests.rs @@ -0,0 +1,201 @@ +//! Verifies independent code-mode and nested-tool policy through the app-server API. + +use super::*; +use codex_protocol::openai_models::GuardianModelPolicy; +use codex_protocol::openai_models::GuardianReviewMode; +use pretty_assertions::assert_eq; +use test_case::test_case; + +#[test_case(GuardianReviewMode::Adaptive, GuardianReviewMode::Adaptive, 2, 0; "adaptive cells and cua reuse cached scores")] +#[test_case(GuardianReviewMode::Synchronous, GuardianReviewMode::Adaptive, 1, 2; "adaptive cells retain synchronous nested reviews")] +#[test_case(GuardianReviewMode::Adaptive, GuardianReviewMode::Disabled, 1, 1; "shell and cua do not enable code mode")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_guardian_policy_scores_code_mode_cells( + computer_use: GuardianReviewMode, + code_mode: GuardianReviewMode, + scores_per_cell: usize, + expected_reviews: usize, +) -> Result<()> { + skip_if_no_network!(Ok(())); + let state = Arc::new(MockResponsesState::default()); + let next_cell = Arc::new(Notify::new()); + let continue_parent = Arc::clone(&next_cell); + let listener = TcpListener::bind("127.0.0.1:0").await?; + let responses_url = format!("http://{}", listener.local_addr()?); + let router = Router::new() + .route( + "/v1/responses", + get(luna_websocket).post( + move |State(state): State>, Json(request): Json| { + let next_cell = Arc::clone(&next_cell); + async move { + if request.pointer("/client_metadata/x-openai-subagent") + == Some(&json!("guardian")) + { + return parent_response(State(state), Json(request)) + .await + .into_response(); + } + let index = state.parent_requests.fetch_add(/*val*/ 1, Ordering::SeqCst); + if index == 1 { + next_cell.notified().await; + } + let events = if index == 1 { + let cell_id = request["input"] + .as_array() + .expect("parent input") + .iter() + .find(|item| item["type"] == "custom_tool_call_output" && item["call_id"] == "cell-0") + .and_then(|item| item["output"].as_array()) + .into_iter() + .flatten() + .filter_map(|item| item["text"].as_str()) + .find_map(|text| text.strip_prefix("Script running with cell ID ")) + .and_then(|text| text.lines().next()) + .expect("first cell yielded"); + vec![ + responses::ev_response_created("poll"), + responses::ev_function_call( + "poll-cell", + "wait", + &json!({"cell_id": cell_id, "terminate": true}).to_string(), + ), + responses::ev_completed("poll"), + ] + } else if index < 3 { + let cell = usize::from(index == 2); + let call_id = format!("cell-{cell}"); + let pause = if cell == 0 { + "yield_control(); await new Promise(() => {});" + } else { + "" + }; + vec![ + responses::ev_response_created(&call_id), + responses::ev_custom_tool_call( + &call_id, + "exec", + &format!( + "const message = ['browser', '{cell}'].join('-'); text(await tools.mcp__node_repl__js({{message}})); {pause}" + ), + ), + responses::ev_completed(&call_id), + ] + } else { + vec![ + responses::ev_assistant_message("done", "done"), + responses::ev_completed("done"), + ] + }; + ( + [(header::CONTENT_TYPE, "text/event-stream")], + responses::sse(events), + ) + .into_response() + } + }, + ), + ) + .with_state(Arc::clone(&state)); + let responses_server = tokio::spawn(async move { + let _ = axum::serve(listener, router).await; + }); + let (mcp_url, mcp_server) = + start_mcp_server_with_tools(&["js"], /*sensitive_action*/ None).await?; + let codex_home = TempDir::new()?; + let analytics_server = responses::start_mock_server().await; + mount_analytics_capture(&analytics_server, codex_home.path()).await?; + MockResponsesConfig::new(&responses_url) + .with_model(MODEL) + .with_provider_config("supports_websockets = false") + .with_approval_policy("on-request") + .with_root_config(&format!( + "approvals_reviewer = \"auto_review\"\nchatgpt_base_url = \"{}\"", + analytics_server.uri(), + )) + .with_extra_config(&format!( + "[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[analytics]\nenabled = true" + )) + .enable_feature(Feature::GuardianApproval) + .enable_feature(Feature::CodeModeOnly) + .write(codex_home.path())?; + let config = load_default_config_for_test(&codex_home).await; + let mut model = codex_core::test_support::construct_model_info_offline(MODEL, &config); + model.guardian = Some(GuardianModelPolicy { + computer_use: Some(computer_use), + code_mode: Some(code_mode), + shell: Some(GuardianReviewMode::Adaptive), + ..Default::default() + }); + write_models_cache_with_models(codex_home.path(), vec![model])?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + let thread = app_server + .start_thread(ThreadStartParams::default()) + .await? + .thread; + state.allow_guardian_review.notify_one(); + let id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.id, + input: vec![UserInput::Text { + text: USER_CONTEXT.to_owned(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(id)).await??; + // Publish the first cell's scores before polling it and starting the second + // cell. Polling must not classify or invalidate that score. Keep second-cell + // classifiers pending so its approval must use the thread's latest score. + wait_for_luna_request(&state, scores_per_cell - 1).await?; + for completed_scores in 1..=scores_per_cell { + state.allow_luna.notify_one(); + timeout(TIMEOUT, async { + loop { + let events = captured_analytics_events(&analytics_server).await; + if events + .iter() + .filter(|event| { + event["event_type"] == "codex_guardian_v2_classification" + && matches!( + event["event_params"]["outcome"].as_str(), + Some("success" | "superseded") + ) + }) + .count() + >= completed_scores + { + break; + } + tokio::time::sleep(Duration::from_millis(/*millis*/ 25)).await; + } + }) + .await?; + } + continue_parent.notify_one(); + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.turn.status, TurnStatus::Completed); + wait_for_luna_request(&state, 2 * scores_per_cell - 1).await?; + app_server.shutdown_gracefully().await?; + let requests = state.luna_requests.lock().expect("score requests"); + assert_eq!( + ( + requests.len(), + state.guardian_reviews.load(Ordering::SeqCst) + ), + (2 * scores_per_cell, expected_reviews) + ); + assert!(requests.iter().any(|request| { + request + .to_string() + .contains("const message = ['browser', '0'].join('-')") + })); + mcp_server.abort(); + responses_server.abort(); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/guardian_policy_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_policy_tests.rs new file mode 100644 index 0000000000..20385c64f6 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/guardian_policy_tests.rs @@ -0,0 +1,137 @@ +//! Exercises model policy, legacy config, and live user modes through the app-server API. + +use super::*; +use codex_protocol::openai_models::GuardianModelPolicy; +use codex_protocol::openai_models::GuardianReviewMode; +use pretty_assertions::assert_eq; +use test_case::test_case; + +#[derive(Clone, Copy)] +enum ReviewConstraint { + Ordinary, + Sensitive, +} + +#[derive(Clone, Copy)] +enum UserMode { + User, + Automatic, + FullAccess, +} + +#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::Automatic, false, 1, 0, ReviewConstraint::Ordinary; "model enables async without legacy flag")] +#[test_case(Some(GuardianReviewMode::Synchronous), UserMode::Automatic, true, 0, 1, ReviewConstraint::Ordinary; "model disables async despite legacy flag")] +#[test_case(Some(GuardianReviewMode::Disabled), UserMode::Automatic, true, 0, 0, ReviewConstraint::Ordinary; "model disables ordinary cua review")] +#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::User, true, 0, 0, ReviewConstraint::Ordinary; "user mode never scores")] +#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::FullAccess, true, 0, 0, ReviewConstraint::Ordinary; "full access never scores")] +#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::FullAccess, true, 0, 0, ReviewConstraint::Sensitive; "full access skips sensitive action reviews")] +#[test_case(None, UserMode::Automatic, false, 0, 1, ReviewConstraint::Ordinary; "legacy synchronous config")] +#[test_case(None, UserMode::Automatic, true, 1, 0, ReviewConstraint::Ordinary; "legacy adaptive config")] +#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::Automatic, false, 1, 1, ReviewConstraint::Sensitive; "sensitive elicitation cannot use initial cua allowance")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_guardian_policy_controls_cua( + mode: Option, + user_mode: UserMode, + legacy_enabled: bool, + expected_scores: usize, + expected_reviews: usize, + constraint: ReviewConstraint, +) -> Result<()> { + skip_if_no_network!(Ok(())); + let state = Arc::new(MockResponsesState { + mcp_server_name: Some("node_repl"), + mcp_tool_sequence: Some(&["js"]), + ..Default::default() + }); + let listener = TcpListener::bind("127.0.0.1:0").await?; + let responses_url = format!("http://{}", listener.local_addr()?); + let router = Router::new() + .route("/v1/responses", get(luna_websocket).post(parent_response)) + .with_state(Arc::clone(&state)); + let responses_server = tokio::spawn(async move { + let _ = axum::serve(listener, router).await; + }); + let (mcp_url, mcp_server) = start_mcp_server_with_tools( + &["js"], + matches!(constraint, ReviewConstraint::Sensitive).then_some(/*t*/ true), + ) + .await?; + let codex_home = TempDir::new()?; + MockResponsesConfig::new(&responses_url) + .with_model(MODEL) + .with_provider_config("supports_websockets = false") + .with_approval_policy("on-request") + .with_root_config("approvals_reviewer = \"auto_review\"") + .with_extra_config(&format!( + "[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = {legacy_enabled}" + )) + .enable_feature(Feature::GuardianApproval) + .write(codex_home.path())?; + let config = load_default_config_for_test(&codex_home).await; + let mut model = codex_core::test_support::construct_model_info_offline(MODEL, &config); + // Exercise both directions of precedence over the legacy CUA bit. + model.node_repl_auto_review_required = + mode.is_none() || mode == Some(GuardianReviewMode::Disabled); + model.guardian = mode.map(|computer_use| GuardianModelPolicy { + computer_use: Some(computer_use), + ..Default::default() + }); + write_models_cache_with_models(codex_home.path(), vec![model])?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(TIMEOUT) + .await?; + let started = app_server + .start_thread(ThreadStartParams { + approvals_reviewer: Some(match user_mode { + UserMode::User => ApprovalsReviewer::User, + UserMode::Automatic | UserMode::FullAccess => ApprovalsReviewer::AutoReview, + }), + approval_policy: matches!(user_mode, UserMode::FullAccess) + .then_some(AskForApproval::Never), + sandbox: matches!(user_mode, UserMode::FullAccess) + .then_some(SandboxMode::DangerFullAccess), + ..Default::default() + }) + .await?; + let thread = started.thread; + if expected_scores > 0 { + timeout(TIMEOUT, async { + while state.luna_connections.load(Ordering::SeqCst) == 0 { + tokio::task::yield_now().await; + } + }) + .await?; + } + state.allow_guardian_review.notify_one(); + state.allow_luna.notify_one(); + let id = app_server + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![UserInput::Text { + text: USER_CONTEXT.to_owned(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(id)).await??; + let completed: TurnCompletedNotification = + timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??; + assert_eq!(completed.turn.status, TurnStatus::Completed); + if expected_scores > 0 { + wait_for_luna_request(&state, expected_scores - 1).await?; + } + app_server.shutdown_gracefully().await?; + assert_eq!( + ( + state.luna_requests.lock().expect("score requests").len(), + state.guardian_reviews.load(Ordering::SeqCst), + state.luna_connections.load(Ordering::SeqCst) > 0, + ), + (expected_scores, expected_reviews, expected_scores > 0) + ); + mcp_server.abort(); + responses_server.abort(); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index 507bc18110..e72d7713e1 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -82,6 +82,12 @@ mod history; #[path = "guardian_v2_model_tests.rs"] mod model_tests; +#[path = "guardian_policy_tests.rs"] +mod policy; + +#[path = "guardian_code_mode_tests.rs"] +mod code_mode; + const TIMEOUT: Duration = Duration::from_secs(30); const MODEL: &str = "mock-model"; const REQUIRED_MODEL: &str = "protected-model"; diff --git a/codex-rs/core/src/context/node_repl_review_evidence.rs b/codex-rs/core/src/context/node_repl_review_evidence.rs index 8e4d63e7af..4ead21aaf7 100644 --- a/codex-rs/core/src/context/node_repl_review_evidence.rs +++ b/codex-rs/core/src/context/node_repl_review_evidence.rs @@ -32,7 +32,7 @@ pub enum NodeReplReviewEvidenceMode { pub(crate) fn node_repl_review_evidence_mode(turn: &TurnContext) -> NodeReplReviewEvidenceMode { let features = &turn.config.features; - if turn.model_info().node_repl_auto_review_required + if turn.model_info().computer_use_review_required() || features.enabled(Feature::GuardianEnhancedNodeReplTranscripts) && features.enabled(Feature::GuardianNodeReplTranscriptImages) { diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 3701c86add..acbeba6f84 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -48,6 +48,7 @@ pub(crate) use review::new_guardian_review_id; pub(crate) use review::record_guardian_denial_for_test; #[cfg(test)] pub(crate) use review::review_approval_request; +#[cfg(test)] pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::routes_approval_policy_to_guardian; pub(crate) use review::routes_approval_to_guardian; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 116045bc30..8902d1796b 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -5,9 +5,12 @@ use codex_analytics::GuardianReviewFailureReason; use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewedAction; +#[cfg(test)] use codex_analytics::GuardianV2Event; +#[cfg(test)] use codex_analytics::GuardianV2EventKind; use codex_core_plugins::PluginCommandAttribution; +#[cfg(test)] use codex_extension_api::GuardianV2Enabled; use codex_extension_api::ThreadIdleCause; use codex_features::Feature; @@ -53,6 +56,7 @@ use super::GuardianAssessmentOutcome; use super::GuardianRejectionCircuitBreakerAction; use super::GuardianRejectionCircuitBreakerPolicy; use super::GuardianReviewContext; +#[cfg(test)] use super::approval_request::format_guardian_action_compact; use super::approval_request::format_guardian_action_pretty; use super::approval_request::guardian_assessment_action; @@ -340,6 +344,7 @@ pub(crate) async fn record_guardian_denial_for_test( /// Runs Guardian unless Full Access or an installed extension resolves the review. /// Guardian timeouts, review-session failures, and parse failures all block /// execution, with timeouts surfaced separately from explicit denials. +#[cfg(test)] async fn run_guardian_review( session: Arc, context: GuardianReviewContext, @@ -927,6 +932,7 @@ pub(crate) async fn review_approval_request( review.await } +#[cfg(test)] pub(crate) async fn review_approval_request_with_cancel( session: &Arc, context: impl Into, @@ -1038,7 +1044,7 @@ pub(super) async fn guardian_review_session_config( guardian_reasoning_effort.clone(), guardian_model_info.model_messages.as_ref(), )?; - if turn.model_info().node_repl_auto_review_required { + if turn.model_info().computer_use_review_required() { spawn_config .features .enable(Feature::RetainClientDeveloperMessages) diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index e145d86ef7..09785fd807 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -500,7 +500,7 @@ impl GuardianReviewSessionManager { parent_context .turn() .model_info() - .node_repl_auto_review_required, + .computer_use_review_required(), ) .with_node_repl_policy(&session_config.node_repl_policy); reuse_key.root_authorization_version = root_authorization_version; @@ -613,7 +613,7 @@ impl GuardianReviewSessionManager { .parent_context .turn() .model_info() - .node_repl_auto_review_required, + .computer_use_review_required(), ) .with_node_repl_policy(¶ms.node_repl_policy); next_reuse_key.root_authorization_version = root_authorization_version; @@ -1424,7 +1424,7 @@ async fn ensure_guardian_node_repl_policy( .parent_context .turn() .model_info() - .node_repl_auto_review_required + .computer_use_review_required() || !matches!( ¶ms.request, GuardianApprovalRequest::McpToolCall { server, tool_name, .. } diff --git a/codex-rs/core/src/guardian/runtime.rs b/codex-rs/core/src/guardian/runtime.rs index 093a998a33..097172cd46 100644 --- a/codex-rs/core/src/guardian/runtime.rs +++ b/codex-rs/core/src/guardian/runtime.rs @@ -17,9 +17,9 @@ use crate::session::session::Session; /// This lets the extension return AskUser before invoking that renderer. #[derive(Clone)] pub(crate) struct ReviewAction { - pub(super) action: Result, - pub(super) category: codex_protocol::openai_models::GuardianScope, - pub(super) request: Result, + pub(crate) action: Result, + pub(crate) category: codex_protocol::openai_models::GuardianScope, + pub(crate) request: Result, } impl From for ReviewAction { diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index f4feaafbc2..d4505eace2 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -807,13 +807,15 @@ async fn review_guardian_mcp_elicitation( Some(Value::Bool(true)) ); - if matches!( + let mut review_context = crate::guardian::GuardianReviewContext::from(&turn_context); + let strict_auto_review = matches!( request .elicitation .meta() .and_then(|meta| meta.get(MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY)), Some(Value::Bool(true)) - ) { + ); + let guardian_request = if strict_auto_review { let connector_id = elicitation_connector_id(&request.elicitation); let trusted_guardian_request = if request.server_name == CODEX_APPS_MCP_SERVER_NAME { let Some(call_id) = request @@ -883,109 +885,96 @@ async fn review_guardian_mcp_elicitation( else { return Ok(None); }; - let guardian_request = trusted_guardian_request.unwrap_or(*guardian_request); - let decision = crate::guardian::review_approval_request_with_cancel( - &session, - &turn_context, - crate::guardian::new_guardian_review_id(), - guardian_request, - /*retry_reason*/ None, - crate::guardian::GuardianReviewOptions { - require_guardian: false, - plugin_attribution_override: None, - approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, - external_cancel: Some(cancellation_token), - require_synchronous_review, - }, - ) - .await; - - return Ok(matches!( - decision, - ReviewDecision::Approved - | ReviewDecision::Denied { .. } - | ReviewDecision::TimedOut - | ReviewDecision::Abort - ) - .then(|| { - mcp_elicitation_response_from_guardian_decision(decision, turn_context.model_info()) - })); - } - - let approval_policy = mcp_config.approval_policy.value(); - match approval_policy { - AskForApproval::Never => { - let Some(permission_profile) = - mcp_config.permission_profile_for_server(&request.server_name) - else { + trusted_guardian_request.unwrap_or(*guardian_request).into() + } else { + let approval_policy = mcp_config.approval_policy.value(); + match approval_policy { + AskForApproval::Never => { + let Some(permission_profile) = + mcp_config.permission_profile_for_server(&request.server_name) + else { + return Ok(Some(mcp_elicitation_decline_without_message())); + }; + if codex_mcp::mcp_permission_prompt_is_auto_approved( + approval_policy, + permission_profile, + codex_mcp::McpPermissionPromptAutoApproveContext::default(), + ) && matches!( + &request.elicitation, + Elicitation::Mcp( + rmcp::model::ElicitRequestParams::FormElicitationParams { + requested_schema, + .. + } + ) if requested_schema.properties.is_empty() + ) { + return Ok(Some(ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(serde_json::json!({})), + meta: None, + })); + } return Ok(Some(mcp_elicitation_decline_without_message())); - }; - if codex_mcp::mcp_permission_prompt_is_auto_approved( - approval_policy, - permission_profile, - codex_mcp::McpPermissionPromptAutoApproveContext::default(), - ) && matches!( - &request.elicitation, - Elicitation::Mcp( - rmcp::model::ElicitRequestParams::FormElicitationParams { - requested_schema, - .. - } - ) if requested_schema.properties.is_empty() - ) { - return Ok(Some(ElicitationResponse { - action: ElicitationAction::Accept, - content: Some(serde_json::json!({})), - meta: None, - })); } - return Ok(Some(mcp_elicitation_decline_without_message())); + AskForApproval::Granular(config) if !config.allows_mcp_elicitations() => { + return Ok(Some(mcp_elicitation_decline_without_message())); + } + AskForApproval::OnRequest + | AskForApproval::UnlessTrusted + | AskForApproval::Granular(_) => {} } - AskForApproval::Granular(config) if !config.allows_mcp_elicitations() => { - return Ok(Some(mcp_elicitation_decline_without_message())); - } - AskForApproval::OnRequest | AskForApproval::UnlessTrusted | AskForApproval::Granular(_) => { - } - } - let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers( - &mcp_config.config_layer_stack, - step_settings - .mcp_approvals_reviewer_override - .unwrap_or(mcp_config.approvals_reviewer), - Some(turn_context.model_info().slug.as_str()), - request.server_name.as_str(), - elicitation_connector_id(&request.elicitation), - /*link_id*/ None, - ); - if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { - return Ok(None); - } - - let guardian_request = match guardian_elicitation_review_request(&request, originating_call_id) - { - GuardianElicitationReview::NotRequested => return Ok(None), - GuardianElicitationReview::Decline(reason) => { - warn!( - server_name = %request.server_name, - request_id = %mcp_elicitation_request_id(&request.request_id), - reason, - "declining Guardian MCP elicitation before review" - ); - return Ok(Some(mcp_elicitation_decline_without_message())); + let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers( + &mcp_config.config_layer_stack, + step_settings + .mcp_approvals_reviewer_override + .unwrap_or(mcp_config.approvals_reviewer), + Some(turn_context.model_info().slug.as_str()), + request.server_name.as_str(), + elicitation_connector_id(&request.elicitation), + /*link_id*/ None, + ); + review_context.approval_policy = approval_policy; + review_context.approvals_reviewer = approvals_reviewer; + match guardian_elicitation_review_request(&request, originating_call_id) { + GuardianElicitationReview::NotRequested => return Ok(None), + GuardianElicitationReview::Decline(reason) => { + let Elicitation::Mcp(elicitation) = &request.elicitation else { + return Ok(None); + }; + crate::guardian::ReviewAction { + action: serde_json::to_value(elicitation) + .map(|elicitation| { + serde_json::json!({ + "tool": "mcp_elicitation", + "server": request.server_name, + "request": elicitation, + }) + }) + .map_err(|error| error.to_string()), + category: codex_protocol::openai_models::GuardianScope::for_mcp_server( + &request.server_name, + ), + request: Err(reason.to_owned()), + } + } + GuardianElicitationReview::ApprovalRequest(guardian_request) => { + (*guardian_request).into() + } } - GuardianElicitationReview::ApprovalRequest(guardian_request) => *guardian_request, }; - - let review_id = crate::guardian::new_guardian_review_id(); - let decision = crate::guardian::review_approval_request_with_cancel( - &session, - &turn_context, - review_id, + let declined_reason = guardian_request.request.as_ref().err().cloned(); + let decision = crate::guardian::decide_approval( + session, + review_context, + crate::guardian::new_guardian_review_id(), guardian_request, - /*retry_reason*/ None, + crate::tools::sandboxing::ApprovalRequestReasons { + approval: None, + retry: None, + }, crate::guardian::GuardianReviewOptions { - require_guardian: false, + require_guardian: strict_auto_review, plugin_attribution_override: None, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, external_cancel: Some(cancellation_token), @@ -993,10 +982,15 @@ async fn review_guardian_mcp_elicitation( }, ) .await; - Ok(Some(mcp_elicitation_response_from_guardian_decision( - decision, - turn_context.model_info(), - ))) + Ok(decision.map(|decision| { + if let Some(reason) = declined_reason { + warn!(server_name = %request.server_name, + request_id = %mcp_elicitation_request_id(&request.request_id), reason, + "declining Guardian MCP elicitation before review"); + return mcp_elicitation_decline_without_message(); + } + mcp_elicitation_response_from_guardian_decision(decision, turn_context.model_info()) + })) } fn guardian_elicitation_review_request( diff --git a/codex-rs/core/src/turn_metadata.rs b/codex-rs/core/src/turn_metadata.rs index 92afc9d9e1..5c30fd202f 100644 --- a/codex-rs/core/src/turn_metadata.rs +++ b/codex-rs/core/src/turn_metadata.rs @@ -218,7 +218,7 @@ impl TurnMetadataState { turn_id, sandbox_tags, auto_review_enabled, - node_repl_auto_review_required: model_info.node_repl_auto_review_required, + node_repl_auto_review_required: model_info.computer_use_review_required(), node_repl_disabled: model_info.node_repl_disabled, enriched_workspaces: RwLock::new(None), tool_namespaces_info: RwLock::new(None), diff --git a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs index b385e31e63..c028c736e4 100644 --- a/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs +++ b/codex-rs/core/tests/suite/guardian_mcp_elicitation.rs @@ -11,7 +11,6 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::GuardianAssessmentAction; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::Op; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::user_input::UserInput; use core_test_support::responses; @@ -19,6 +18,7 @@ use core_test_support::skip_if_no_network; use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use serde_json::Value; @@ -72,6 +72,12 @@ for line in sys.stdin: "_meta": {"codex_request_type": "approval_request", "codex_approval_kind": "mcp_tool_call", "tool_name": "write_record", **meta, **invocation_meta}}}) + if len(sys.argv) > 3: + for elicitation in pending_elicitations: + params = elicitation["params"] + params.update(json.loads(sys.argv[3])) + if params.get("mode") == "url": + params.pop("requestedSchema") send(pending_elicitations.pop(0)) continue elif method is None and str(request.get("id", "")).startswith("server-approval"): @@ -224,14 +230,12 @@ async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review( struct AutoApprovingReviewContributor; impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor { - fn fast_decision<'a>( + fn decide<'a>( &'a self, - _session_store: &'a codex_extension_api::ExtensionData, - _thread_store: &'a codex_extension_api::ExtensionData, - _prompt: &'a str, - _extension_metrics: Option>, - ) -> codex_extension_api::ExtensionFuture<'a, Option> { - Box::pin(async { Some(ReviewDecision::Approved) }) + _input: &'a codex_extension_api::ApprovalDecisionInput<'_>, + ) -> codex_extension_api::ExtensionFuture<'a, Option> + { + Box::pin(async { Some(codex_extension_api::ApprovalDecision::Allow) }) } } @@ -653,3 +657,116 @@ async fn node_elicitations_attribute_independent_reviews_without_changing_action test.codex.shutdown_and_wait().await?; Ok(()) } + +#[test_case("form"; "nonempty_form")] +#[test_case("url"; "url")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_review_preserves_unsupported_guardian_elicitations(mode: &str) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter"); + let server = responses::start_mock_server().await; + let form_schema = json!({ + "type": "object", "properties": {"name": {"type": "string"}}, "required": ["name"] + }); + let params = if mode == "url" { + json!({"mode": "url", "url": "https://example.com/approve", "elicitationId": "approval"}) + } else { + json!({"requestedSchema": form_schema}) + }; + let mcp_servers = serde_json::from_value(json!({ + "elicitation": { + "command": if cfg!(windows) { "python" } else { "python3" }, + "args": ["-u", "-c", ELICITATION_SERVER, "{}", "", params.to_string()], + "default_tools_approval_mode": "approve", + } + }))?; + let test = test_codex() + .with_config(move |config| { + config.approvals_reviewer = ApprovalsReviewer::User; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Granular( + codex_protocol::protocol::GranularApprovalConfig { + sandbox_approval: false, + rules: false, + skill_approval: false, + request_permissions: false, + mcp_elicitations: true, + }, + )); + config + .mcp_servers + .set(mcp_servers) + .expect("set MCP fixture"); + }) + .build_with_auto_env(&server) + .await?; + wait_for_mcp_server(&test.codex, "elicitation").await?; + responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "eliciting-tool", + "mcp__elicitation", + "request_approval", + "{}", + ), + responses::ev_completed("parent-tool"), + ]), + ) + .await; + let parent = responses::mount_sse_once( + &server, + responses::sse(vec![responses::ev_completed("parent-complete")]), + ) + .await; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "Run the tool and ask me for its approval.".into(), + text_elements: Vec::new(), + }])) + .await?; + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ElicitationRequest(_) + | EventMsg::GuardianAssessment(_) + | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ElicitationRequest(request) = event else { + panic!("expected a user elicitation, got {event:?}"); + }; + match &request.request { + codex_protocol::approvals::ElicitationRequest::Form { + requested_schema, .. + } => { + assert_eq!(requested_schema, &form_schema); + } + codex_protocol::approvals::ElicitationRequest::Url { url, .. } => { + assert_eq!(url, "https://example.com/approve"); + } + other => panic!("unexpected elicitation: {other:?}"), + } + test.codex + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision: codex_protocol::approvals::ElicitationAction::Decline, + content: None, + meta: None, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert!( + parent + .single_request() + .function_call_output("eliciting-tool") + .to_string() + .contains("decline") + ); + test.codex.shutdown_and_wait().await?; + Ok(()) +}