diff --git a/codex-rs/app-server/src/request_processors/apps_processor/installed.rs b/codex-rs/app-server/src/request_processors/apps_processor/installed.rs index 1f3dc7bafa..00f4f86c49 100644 --- a/codex-rs/app-server/src/request_processors/apps_processor/installed.rs +++ b/codex-rs/app-server/src/request_processors/apps_processor/installed.rs @@ -113,7 +113,6 @@ impl AppsRequestProcessor { client_mcp_extensions: ClientMcpExtensions::default(), auth: auth.clone(), auth_manager: codex_apps_auth_manager, - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 1536a0265a..dda2edd914 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -222,7 +222,6 @@ impl McpConnectionSet { client_mcp_extensions, auth, auth_manager, - allow_user_interaction, elicitation_reviewer, elicitation_lifecycle, } = input; @@ -259,7 +258,6 @@ impl McpConnectionSet { !previous.servers.is_empty() && previous.elicitation_requests.update( Arc::clone(&config), - allow_user_interaction, elicitation_reviewer.clone(), elicitation_lifecycle.clone(), ) @@ -269,7 +267,6 @@ impl McpConnectionSet { } else { ElicitationRequestManager::new( Arc::clone(&config), - allow_user_interaction, elicitation_reviewer, elicitation_lifecycle, elicitation_router, diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index efaaa4ff4a..3d8875caf3 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -117,7 +117,6 @@ impl McpConnectionSet { approval_policy.value(), permission_profile.get().clone(), ), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -913,7 +912,6 @@ fn elicitation_granular_policy_respects_never_and_config() { async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() { let manager = ElicitationRequestManager::new( test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -952,7 +950,6 @@ async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() { async fn disabled_permissions_do_not_auto_accept_elicitation_with_requested_fields() { let manager = ElicitationRequestManager::new( test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -1048,7 +1045,6 @@ async fn assert_elicitation_declined_with_reviewer_calls( let reviewer = Arc::new(DecliningElicitationReviewer::default()); let manager = ElicitationRequestManager::new( test_elicitation_config(server_name, approval_policy, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, Some(reviewer.clone()), /*lifecycle*/ None, full_access_form_input_enabled_router(), @@ -1093,7 +1089,6 @@ async fn assert_requested_user_input_is_declined( ) { let manager = ElicitationRequestManager::new( test_elicitation_config("server", approval_policy, permission_profile), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, router, @@ -1185,7 +1180,6 @@ async fn assert_disabled_permissions_surface_requested_user_input( let reviewer = Arc::new(DecliningElicitationReviewer::default()); let manager = ElicitationRequestManager::new( test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, Some(reviewer.clone()), /*lifecycle*/ None, router.clone(), @@ -1336,7 +1330,6 @@ async fn on_request_approval_forms_remain_with_the_reviewer() { async fn disabled_permissions_decline_user_input_without_an_event_channel() { let manager = ElicitationRequestManager::new( test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, full_access_form_input_enabled_router(), @@ -1376,7 +1369,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::Never, PermissionProfile::default(), ), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -1390,7 +1382,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::OnRequest, PermissionProfile::Disabled ), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -1400,7 +1391,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::Never, PermissionProfile::default() ), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -1462,7 +1452,6 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() { AskForApproval::OnRequest, PermissionProfile::default(), ), - /*allow_user_interaction*/ true, /*reviewer*/ None, Some(lifecycle.clone()), router.clone(), @@ -1473,7 +1462,6 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() { AskForApproval::OnRequest, PermissionProfile::default(), ), - /*allow_user_interaction*/ true, /*reviewer*/ None, Some(lifecycle), router.clone(), @@ -1578,7 +1566,6 @@ async fn cancelled_elicitation_is_removed_without_affecting_other_pending_reques AskForApproval::OnRequest, PermissionProfile::default(), ), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, router.clone(), @@ -2058,7 +2045,6 @@ async fn read_only_apps_discovery_never_uses_a_shared_writable_catalog() -> anyh client_mcp_extensions: ClientMcpExtensions::default(), auth: None, auth_manager: None, - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }, @@ -2114,7 +2100,6 @@ async fn hosted_apps_protocol_mode_is_independent_of_generic_mode() -> anyhow::R /*previous*/ None, McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(config), plugins_available: false, @@ -2221,7 +2206,6 @@ async fn codex_apps_extension_does_not_share_host_owned_tools_cache() -> anyhow: /*previous*/ None, McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(config), plugins_available: false, @@ -4619,7 +4603,6 @@ async fn executor_owned_chatgpt_mcp_accepts_only_safe_explicit_authorization() - /*previous*/ None, McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(runtime_config.clone()), plugins_available: false, @@ -4734,7 +4717,6 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { /*previous*/ None, McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(crate::mcp::tests::test_mcp_config( codex_home.path().to_path_buf(), @@ -5158,7 +5140,6 @@ async fn reconcile_reusable_server( config, runtime_context, crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()), - /*allow_user_interaction*/ true, ) .await } @@ -5169,14 +5150,12 @@ async fn reconcile_reusable_server_with_mcp_config( config: McpServerConfig, runtime_context: McpRuntimeContext, mcp_config: crate::McpConfig, - allow_user_interaction: bool, ) -> McpConnectionSet { let (tx_event, _rx_event) = async_channel::unbounded(); McpConnectionSet::new( Some(previous), McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction, startup_policy: McpStartupPolicy::Eager, config: Arc::new(mcp_config), plugins_available: false, @@ -5225,7 +5204,6 @@ async fn read_only_policy_does_not_reuse_a_writable_connection() { config.clone(), runtime_context.clone(), mcp_config, - /*allow_user_interaction*/ true, ) .await; assert_eq!( @@ -5455,7 +5433,6 @@ async fn refreshed_catalog_follows_reused_client_without_mutating_old_bindings() config, runtime_context, crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()), - /*allow_user_interaction*/ true, ) .await, ); @@ -5930,7 +5907,6 @@ async fn reconciliation_replaces_connection_when_protocol_mode_changes() { Some(&previous), McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(mcp_config), plugins_available: false, @@ -5989,7 +5965,6 @@ async fn reconciliation_reuses_legacy_stdio_server_when_modern_protocol_is_enabl Some(&previous), McpPublicationGate::already_published(), McpRuntimeInput { - allow_user_interaction: true, startup_policy: McpStartupPolicy::Eager, config: Arc::new(mcp_config), plugins_available: false, @@ -6033,7 +6008,6 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv let router = ElicitationRequestRouter::default(); previous.elicitation_requests = ElicitationRequestManager::new( test_elicitation_config("docs", AskForApproval::Never, PermissionProfile::Disabled), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, router.clone(), @@ -6056,19 +6030,19 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv meta: None, }; - for allow_user_interaction in [true, false, true] { - let mcp_config = test_elicitation_config( - "docs", - AskForApproval::OnRequest, - PermissionProfile::default(), - ); + for approval_policy in [ + AskForApproval::OnRequest, + AskForApproval::Never, + AskForApproval::OnRequest, + ] { + let mcp_config = + test_elicitation_config("docs", approval_policy, PermissionProfile::default()); let reconciled = reconcile_reusable_server_with_mcp_config( &previous, "docs", config.clone(), runtime_context.clone(), mcp_config.as_ref().clone(), - allow_user_interaction, ) .await; assert!(previous.shares_test_connection_with(&reconciled, "docs")); @@ -6079,13 +6053,13 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv .lock() .expect("elicitation authority lock"); let config = &authority.as_ref().expect("elicitation authority").config; - assert_eq!(config.approval_policy.value(), AskForApproval::OnRequest); + assert_eq!(config.approval_policy.value(), approval_policy); assert_eq!(config.permission_profile, PermissionProfile::default()); } - // A sender captured before reconciliation must observe each ownership update. + // A sender captured before reconciliation must observe each policy update. let mut pending = sender(NumberOrString::Number(7), elicitation.clone()); - if allow_user_interaction { + if approval_policy == AskForApproval::OnRequest { assert!(futures::poll!(pending.as_mut()).is_pending()); let EventMsg::ElicitationRequest(request) = events.try_recv().expect("user-input event").msg @@ -6105,11 +6079,17 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv .expect("user response should resolve the retained sender"); assert_eq!(pending.await.expect("elicitation should resolve"), response); } else { - let error = pending - .now_or_never() - .expect("blocked elicitation must not wait for user input") - .expect_err("subagent user input must be rejected"); - assert_eq!(error.to_string(), crate::MCP_ELICITATION_HANDOFF_MESSAGE); + assert_eq!( + pending + .now_or_never() + .expect("a policy denial must not wait for user input") + .expect("elicitation should receive a response"), + ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: None, + } + ); } assert!(events.is_empty()); previous = reconciled; @@ -6299,7 +6279,6 @@ async fn reconciliation_reconnects_when_host_plugin_root_changes() { server_config.clone(), runtime_context.clone(), config_for_root(original_root), - /*allow_user_interaction*/ true, ) .await; assert!(previous.shares_test_connection_with(&unchanged, "docs")); @@ -6311,7 +6290,6 @@ async fn reconciliation_reconnects_when_host_plugin_root_changes() { server_config, runtime_context, replacement_config, - /*allow_user_interaction*/ true, ) .await; assert!(!unchanged.shares_test_connection_with(&replacement, "docs")); diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index 65eeaa724c..c27e6ce54a 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -4,8 +4,7 @@ //! from the user. It decides whether the request can be automatically accepted, //! must be declined by policy, or should be surfaced as a Codex protocol event //! and later resolved through the stored responder. -//! Non-root agents may use automatic decisions, but explicit user-input requests -//! are rejected before review and no pending user prompt is registered. +//! Explicit MCP elicitation requests can be surfaced by root and non-root agents. use std::collections::HashMap; use std::sync::Arc; @@ -26,11 +25,9 @@ use codex_protocol::approvals::ElicitationRequestEvent; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::mcp::OPENAI_ELICITATION_EXTENSION_ID; use codex_protocol::mcp::RequestId as ProtocolRequestId; -use codex_protocol::mcp_approval_meta::APPROVAL_KIND_BROWSER_AUTH; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY; use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION; use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY; -use codex_protocol::mcp_approval_meta::REQUIRES_USER_INPUT_KEY; use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; @@ -48,12 +45,6 @@ use tokio::sync::oneshot; static NEXT_ELICITATION_REQUEST_ID: AtomicU64 = AtomicU64::new(0); const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval."; -/// Recovery guidance for a request that a non-root agent cannot present to the user. -pub const MCP_ELICITATION_HANDOFF_MESSAGE: &str = concat!( - "MCP server elicitations can only be requested by the root thread. ", - "Ask the parent agent to handle this request. ", - "Do not retry the blocked action until the parent confirms the blocker is resolved." -); #[path = "user_verification_elicitation.rs"] mod user_verification_elicitation; @@ -172,10 +163,6 @@ impl ElicitationRequestRouter { server_name: String, request: ElicitationRequest, ) -> Result { - anyhow::ensure!( - authority.allow_user_interaction, - MCP_ELICITATION_HANDOFF_MESSAGE - ); let Some(events) = events else { return Ok(ElicitationResponse { action: ElicitationAction::Decline, @@ -235,7 +222,6 @@ impl ElicitationRequestRouter { #[derive(Clone)] pub(crate) struct ElicitationAuthority { pub(crate) config: Arc, - pub(crate) allow_user_interaction: bool, reviewer: Option, lifecycle: Option, } @@ -249,7 +235,6 @@ pub(crate) struct ElicitationRequestManager { impl ElicitationRequestManager { pub(crate) fn new( config: Arc, - allow_user_interaction: bool, reviewer: Option, lifecycle: Option, router: ElicitationRequestRouter, @@ -258,7 +243,6 @@ impl ElicitationRequestManager { router, authority: Arc::new(StdMutex::new(Some(ElicitationAuthority { config, - allow_user_interaction, reviewer, lifecycle, }))), @@ -268,7 +252,6 @@ impl ElicitationRequestManager { pub(crate) fn update( &self, config: Arc, - allow_user_interaction: bool, reviewer: Option, lifecycle: Option, ) -> bool { @@ -277,7 +260,6 @@ impl ElicitationRequestManager { }; *authority = Some(ElicitationAuthority { config, - allow_user_interaction, reviewer, lifecycle, }); @@ -308,36 +290,6 @@ impl ElicitationRequestManager { .lock() .ok() .and_then(|authority| authority.clone()); - let user_interaction_disabled = authority - .as_ref() - .is_some_and(|authority| !authority.allow_user_interaction); - if user_interaction_disabled { - // Browser sign-in can use an empty schema. Explicit user-input - // markers must take precedence over automatic approval or review. - let requires_user_input = elicitation.meta().is_some_and(|meta| { - meta.get(REQUIRES_USER_INPUT_KEY) == Some(&Value::Bool(true)) - || meta.get(APPROVAL_KIND_KEY).and_then(Value::as_str) - == Some(APPROVAL_KIND_BROWSER_AUTH) - }) || match &elicitation { - Elicitation::Mcp( - rmcp::model::ElicitRequestParams::FormElicitationParams { - requested_schema, - .. - }, - ) => !requested_schema.properties.is_empty(), - Elicitation::OpenAiForm { - requested_schema, .. - } - | Elicitation::OpenAiElicitationForm { - requested_schema, .. - } => requested_schema - .get("properties") - .and_then(Value::as_object) - .is_some_and(|properties| !properties.is_empty()), - Elicitation::Mcp(_) | Elicitation::UserVerification { .. } => true, - }; - anyhow::ensure!(!requires_user_input, MCP_ELICITATION_HANDOFF_MESSAGE); - } if let Elicitation::UserVerification { title, description, diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs index 4f2472bff9..8c6a31183d 100644 --- a/codex-rs/codex-mcp/src/elicitation_tests.rs +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -93,7 +93,6 @@ fn elicitation_fixture( .insert("another-independent-mcp".to_string(), permission_profile); let manager = ElicitationRequestManager::new( config, - /*allow_user_interaction*/ true, reviewer.map(|reviewer| reviewer as Arc), lifecycle, ElicitationRequestRouter::default(), @@ -120,16 +119,6 @@ async fn send_elicitation(sender: &SendElicitation, marker: Option) -> El .expect("elicitation must receive a terminal response") } -fn disable_user_interaction(manager: &ElicitationRequestManager) { - let authority = manager.authority.lock().unwrap().clone().unwrap(); - assert!(manager.update( - authority.config, - /*allow_user_interaction*/ false, - authority.reviewer, - authority.lifecycle, - )); -} - fn form_elicitations(meta: Option, properties: Value) -> [Elicitation; 3] { let schema = json!({"type": "object", "properties": properties}); [ @@ -153,85 +142,8 @@ fn form_elicitations(meta: Option, properties: Value) -> [Elicitation; 3] ] } -async fn assert_subagent_elicitation_blocked(sender: &SendElicitation, elicitation: Elicitation) { - let error = tokio::time::timeout( - std::time::Duration::from_secs(1), - sender(RequestId::Number(7), elicitation), - ) - .await - .expect("blocked elicitation must not wait for user input") - .unwrap_err(); - assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE); -} - #[tokio::test] -async fn subagent_human_input_is_rejected_before_automatic_approval() { - let mut requests = Vec::new(); - for meta in [ - json!({"codex_approval_kind": "browser_auth"}), - json!({"codex_approval_kind": "browser_auth", "codex_requires_user_input": true}), - json!({"codex_requires_user_input": true}), - json!({ - "codex_approval_kind": "mcp_tool_call", - "codex_request_type": "approval_request", - "codex_strict_auto_review": true, - "codex_requires_user_input": true, - }), - ] { - requests.extend(form_elicitations(Some(meta), json!({}))); - } - for meta in [None, Some(json!({"codex_strict_auto_review": true}))] { - requests.extend(form_elicitations( - meta.clone(), - json!({"answer": {"type": "string"}}), - )); - requests.push(Elicitation::Mcp( - ElicitRequestParams::UrlElicitationParams { - meta: meta.map(|meta| RequestMetaObject::from(meta.as_object().unwrap().clone())), - message: "Sign in to continue".into(), - url: "https://example.com/auth".into(), - elicitation_id: "test-auth".into(), - }, - )); - } - - for approval_policy in [AskForApproval::OnRequest, AskForApproval::Never] { - let reviewer = RecordingReviewer::new(Ok(Some(approved_response()))); - let (manager, events, sender) = elicitation_fixture( - approval_policy, - PermissionProfile::Disabled, - Some(reviewer.clone()), - ); - // Update after creating the sender so a retained connection observes the restriction. - disable_user_interaction(&manager); - for elicitation in requests.iter().cloned() { - assert_subagent_elicitation_blocked(&sender, elicitation).await; - } - assert!(events.is_empty()); - assert!(manager.router.requests.lock().unwrap().is_empty()); - assert_eq!(reviewer.calls.load(Relaxed), 0); - assert_eq!(reviewer.active_elicitations.load(Relaxed), 0); - - let (manager, events, sender) = - verification_fixture(approval_policy, Some(reviewer.clone())); - disable_user_interaction(&manager); - assert_subagent_elicitation_blocked( - &sender, - Elicitation::UserVerification { - title: "Approve purchase".into(), - description: "Pay $200".into(), - challenge: "AQID".into(), - }, - ) - .await; - assert!(events.is_empty()); - assert!(manager.router.requests.lock().unwrap().is_empty()); - assert_eq!(reviewer.calls.load(Relaxed), 0); - } -} - -#[tokio::test] -async fn subagent_permission_preserves_automatic_review_decisions() { +async fn permission_elicitation_preserves_automatic_review_decisions() { for (strict_auto_review, approval_policy, expected_reviews) in [ (false, AskForApproval::OnRequest, 1), (true, AskForApproval::OnRequest, 1), @@ -252,7 +164,6 @@ async fn subagent_permission_preserves_automatic_review_decisions() { PermissionProfile::Disabled, Some(reviewer.clone()), ); - disable_user_interaction(&manager); let [request, _, _] = form_elicitations( Some(json!({ "codex_request_type": "approval_request", @@ -284,23 +195,62 @@ async fn subagent_permission_preserves_automatic_review_decisions() { } #[tokio::test] -async fn subagent_without_an_automatic_decision_does_not_prompt() { +async fn elicitation_without_an_automatic_decision_prompts() { for reviewer in [None, Some(RecordingReviewer::new(Ok(None)))] { let (manager, events, sender) = elicitation_fixture( AskForApproval::OnRequest, PermissionProfile::read_only(), reviewer.clone(), ); - disable_user_interaction(&manager); - for elicitation in form_elicitations(/*meta*/ None, json!({})) { - assert_subagent_elicitation_blocked(&sender, elicitation).await; + let mut requests = Vec::from(form_elicitations(/*meta*/ None, json!({}))); + requests.extend(form_elicitations( + Some(json!({"codex_requires_user_input": true})), + json!({"answer": {"type": "string"}}), + )); + requests.push(Elicitation::Mcp( + ElicitRequestParams::UrlElicitationParams { + meta: None, + message: "Sign in to continue".into(), + url: "https://example.com/auth".into(), + elicitation_id: "test-auth".into(), + }, + )); + let expected_reviews = requests.len() + 1; + for elicitation in requests { + let pending = tokio::spawn(sender(RequestId::Number(7), elicitation)); + let event = tokio::time::timeout(std::time::Duration::from_secs(1), events.recv()) + .await + .expect("elicitation should prompt") + .expect("event channel should remain open"); + let EventMsg::ElicitationRequest(request) = event.msg else { + panic!("expected MCP elicitation"); + }; + assert!(!pending.is_finished()); + let ProtocolRequestId::String(request_id) = request.id else { + panic!("expected Codex-owned string request ID"); + }; + let response = ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(json!({"answer": "continue"})), + meta: None, + }; + manager + .router + .resolve( + request.server_name, + RequestId::String(request_id.into()), + response.clone(), + ) + .await + .unwrap(); + assert_eq!(pending.await.unwrap().unwrap(), response); } assert_eq!( send_elicitation(&sender, Some(json!(true))).await, strict_auto_review_decline() ); if let Some(reviewer) = reviewer { - assert_eq!(reviewer.calls.load(Relaxed), 4); + assert_eq!(reviewer.calls.load(Relaxed), expected_reviews); assert_eq!(reviewer.active_elicitations.load(Relaxed), 0); } assert!(events.is_empty()); @@ -308,55 +258,6 @@ async fn subagent_without_an_automatic_decision_does_not_prompt() { } } -#[test] -fn final_prompt_guard_rejects_subagents_before_registration() { - let (manager, _, _) = verification_fixture(AskForApproval::OnRequest, /*reviewer*/ None); - let mut authority = manager.authority.lock().unwrap().clone().unwrap(); - authority.allow_user_interaction = false; - let registrations = Arc::new(AtomicUsize::new(0)); - let starts = registrations.clone(); - authority.lifecycle = Some(ElicitationLifecycle::new(move || { - starts.fetch_add(/*val*/ 1, Relaxed); - })); - let (tx, events) = async_channel::bounded(1); - for sender in [Some(tx.clone()), None] { - let error = manager - .router - .request_user_interaction( - sender, - &authority, - crate::CODEX_APPS_MCP_SERVER_NAME.into(), - ElicitationRequest::Form { - meta: None, - message: "Confirm the action".into(), - requested_schema: json!({"type": "object", "properties": {}}), - }, - ) - .now_or_never() - .expect("a subagent must not wait for user input") - .unwrap_err(); - assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE); - } - let error = user_verification_elicitation::route( - manager.router.clone(), - Some(tx), - Some(authority), - crate::CODEX_APPS_MCP_SERVER_NAME.into(), - ElicitationRequest::UserVerification { - title: "Approve purchase".into(), - description: "Pay $200".into(), - challenge: "AQID".into(), - }, - ) - .now_or_never() - .expect("direct verification routing must not wait for a subagent") - .unwrap_err(); - assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE); - assert_eq!(registrations.load(Relaxed), 0); - assert!(events.is_empty()); - assert!(manager.router.requests.lock().unwrap().is_empty()); -} - async fn assert_declined(marker: Value, response: Option) { let expected_calls = usize::from(marker == Value::Bool(true)); let reviewer = response.map(RecordingReviewer::new); @@ -394,7 +295,6 @@ fn closed_event_channel_immediately_cleans_up_pending_elicitation() { AskForApproval::OnRequest, PermissionProfile::Disabled ), - /*allow_user_interaction*/ true, /*reviewer*/ None, Some(lifecycle), )); @@ -583,7 +483,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author let manager = ElicitationRequestManager::new( Arc::new(config.clone()), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -614,7 +513,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author ); assert!(manager.update( Arc::new(config.clone()), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -638,7 +536,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author ); assert!(manager.update( Arc::new(config.clone()), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -651,7 +548,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author config.set_server_permission_profiles(&servers, std::iter::empty()); assert!(manager.update( Arc::new(config.clone()), - /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -683,7 +579,6 @@ fn verification_fixture( Arc::make_mut(&mut config).mcp_server_catalog = catalog.build(); let manager = ElicitationRequestManager::new( config, - /*allow_user_interaction*/ true, reviewer.map(|reviewer| reviewer as Arc), /*lifecycle*/ None, ElicitationRequestRouter::default(), diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 7dda112ab1..6eca43bcb3 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -8,7 +8,6 @@ pub use elicitation::ElicitationLifecycle; pub use elicitation::ElicitationReviewRequest; pub use elicitation::ElicitationReviewer; pub use elicitation::ElicitationReviewerHandle; -pub use elicitation::MCP_ELICITATION_HANDOFF_MESSAGE; pub use event_stream::McpEventStreamOpener; pub use resource_client::CodexAppsResourceListParams; pub use resource_client::McpEventCatalogSnapshot; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 0f902be3fb..a65da6e697 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -458,7 +458,6 @@ pub async fn read_mcp_resource( client_mcp_extensions: ClientMcpExtensions::default(), auth: auth.cloned(), auth_manager: None, - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }, @@ -538,7 +537,6 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( client_mcp_extensions: ClientMcpExtensions::default(), auth: auth.cloned(), auth_manager: None, - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }, diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 588c9e0edc..f23bb7db5e 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -89,8 +89,6 @@ pub struct McpRuntimeInput { pub client_mcp_extensions: ClientMcpExtensions, pub auth: Option, pub auth_manager: Option>, - /// Whether the owning thread may prompt the user; automatic decisions remain available. - pub allow_user_interaction: bool, pub elicitation_reviewer: Option, pub elicitation_lifecycle: Option, } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index f0a4cc2c92..4156fcfc39 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -269,7 +269,6 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( client_mcp_extensions: ClientMcpExtensions::default(), auth: auth.clone(), auth_manager: codex_apps_auth_manager, - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 5e2e369a4e..9c32660abb 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -781,39 +781,14 @@ async fn maybe_request_codex_apps_auth_elicitation( url: plan.elicitation.url, elicitation_id: plan.elicitation.elicitation_id, }; - let outcome = match sess + let outcome = sess .request_mcp_server_elicitation( turn_context, CODEX_APPS_MCP_SERVER_NAME.to_string(), request_id, request, ) - .await - { - Ok(outcome) => outcome, - Err(error) => { - let mut result = result; - // Direct output prefers structured content. Preserve its diagnostic as - // text so both output paths show it alongside the recovery guidance. - if let Some(structured_content) = result.structured_content.take() { - result.content.push(serde_json::json!({ - "type": "text", - "text": structured_content.to_string(), - })); - } - result.content.insert( - /*index*/ 0, - serde_json::json!({ - "type": "text", - "text": format!( - "Authentication for {} could not be completed. {error}", - plan.auth_failure.connector_name - ), - }), - ); - return result; - } - }; + .await; if !outcome .response .as_ref() @@ -1622,10 +1597,6 @@ pub(crate) async fn request_mcp_tool_user_approval( ); } - if turn_context.session_source.is_non_root_agent() { - return ReviewDecision::denied(codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE); - } - let tool_call_mcp_elicitation_enabled = turn_context .config .features @@ -1691,18 +1662,21 @@ pub(crate) async fn request_mcp_tool_user_approval( .map(|rendered_template| rendered_template.elicitation_message.as_str()), prompt_options, }); - let outcome = match sess + let outcome = sess .request_mcp_server_elicitation(turn_context, server.clone(), request_id, request) - .await - { - Ok(outcome) => outcome, - Err(error) => return ReviewDecision::denied(error.to_string()), - }; + .await; ( outcome.sent, parse_mcp_tool_approval_elicitation_response(outcome.response, &question_id), ) } else { + if turn_context.session_source.is_non_root_agent() { + return ReviewDecision::denied(concat!( + "MCP tool approval via request_user_input requires the root thread. ", + "Ask the parent agent to handle this request. ", + "Do not retry the blocked action until the parent confirms the blocker is resolved." + )); + } let args = RequestUserInputArgs { questions: vec![question], is_blocking: true, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 8fa73d4098..34bd4b7816 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -13,9 +13,6 @@ use crate::session::tests::update_turn_settings_for_test; use crate::session::turn_context::TurnEnvironment; use crate::state::ActiveTurn; use crate::test_support::models_manager_with_provider; -use crate::tools::context::McpToolOutput; -use crate::tools::context::ToolOutput; -use crate::tools::context::ToolPayload; use crate::tools::hook_names::HookToolName; use crate::turn_metadata::ExecutionMetadata; use codex_app_server_protocol as app_server_protocol; @@ -33,7 +30,6 @@ use codex_hooks::HooksConfig; use codex_model_provider::create_model_provider; use codex_protocol::ResponseItemId; use codex_protocol::models::PermissionProfile; -use codex_protocol::models::ResponseInputItem; use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EnvironmentConfig; @@ -1725,91 +1721,17 @@ async fn codex_apps_auth_elicitation_granular_mcp_disabled_returns_original_resu assert!(rx_event.try_recv().is_err()); } +#[test_case::test_case(SessionSource::Exec; "root")] +#[test_case::test_case(SessionSource::SubAgent(SubAgentSource::Review); "subagent")] +#[test_case::test_case(SessionSource::Internal(InternalSessionSource::Guardian); "guardian")] #[tokio::test] -async fn codex_apps_auth_elicitation_returns_subagent_handoff_and_diagnostics() { +async fn codex_apps_auth_elicitation_enabled_by_default_requests_elicitation( + source: SessionSource, +) { let (session, mut turn_context, rx_event) = make_session_and_context_with_rx().await; Arc::get_mut(&mut turn_context) .expect("single turn context ref") - .session_source = SessionSource::SubAgent(SubAgentSource::Review); - let structured_content = serde_json::json!({ - "error": "reauthentication_required", "status": 401 - }); - let mut result = codex_apps_auth_failure_result(); - result.structured_content = Some(structured_content.clone()); - let diagnostic = format!( - "Connector reauthentication required: {}", - "diagnostic detail ".repeat(/*n*/ 500) - ); - result.content[0]["text"] = serde_json::json!(diagnostic); - let metadata = codex_apps_auth_failure_metadata(); - let returned = tokio::time::timeout( - std::time::Duration::from_secs(1), - maybe_request_codex_apps_auth_elicitation( - &session, - &turn_context, - turn_context.approval_policy(), - "call_123", - CODEX_APPS_MCP_SERVER_NAME, - Some(&metadata), - result.clone(), - ), - ) - .await - .expect("subagent auth must not wait for user input"); - - assert_eq!(returned.meta, result.meta); - assert_eq!(returned.is_error, Some(true)); - assert!(rx_event.try_recv().is_err()); - - let output = McpToolOutput { - result: returned, - tool_input: serde_json::json!({}), - result_metadata_capture_allowed: true, - wall_time: std::time::Duration::ZERO, - original_image_detail_supported: false, - truncation_policy: TruncationPolicy::Tokens(256), - }; - let payload = ToolPayload::Function { - arguments: "{}".to_string(), - }; - let code_mode = output.code_mode_result(&payload); - assert_eq!(code_mode["isError"], serde_json::json!(true)); - assert!(code_mode.get("_meta").is_none()); - let code_text = code_mode["content"] - .as_array() - .expect("MCP content") - .iter() - .map(|item| item["text"].as_str().expect("auth diagnostic text")) - .collect::>() - .join("\n"); - assert!(code_text.contains("Authentication for Google Calendar could not be completed.")); - assert!(code_text.contains(codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE)); - assert!(code_text.contains(&diagnostic)); - assert!(code_text.contains(&structured_content.to_string())); - assert_eq!(output.tool_result_metadata(), result.meta.as_ref()); - let mut expected_hook = code_mode; - expected_hook["_meta"] = result.meta.expect("original auth metadata"); - assert_eq!( - output.post_tool_use_response("call_123", &payload), - Some(expected_hook) - ); - - let ResponseInputItem::FunctionCallOutput { - output: truncated, .. - } = output.to_response_item("call_123", &payload) - else { - panic!("expected FunctionCallOutput"); - }; - let truncated_text = truncated.body.to_text().expect("truncated auth diagnostic"); - assert!(truncated_text.contains(codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE)); - assert!(truncated_text.contains("truncated")); - assert!(!truncated_text.contains(&diagnostic)); - assert_eq!(truncated.success, Some(false)); -} - -#[tokio::test] -async fn codex_apps_auth_elicitation_enabled_by_default_requests_elicitation() { - let (session, turn_context, rx_event) = make_session_and_context_with_rx().await; + .session_source = source; *session.active_turn.lock().await = Some(ActiveTurn::default()); let result = codex_apps_auth_failure_result(); let metadata = codex_apps_auth_failure_metadata(); diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index baf4cb5777..036e90afc4 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -554,20 +554,16 @@ impl Session { server_name: String, request_id: RequestId, request: ElicitationRequest, - ) -> anyhow::Result { - anyhow::ensure!( - !turn_context.session_source.is_non_root_agent(), - codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE - ); + ) -> McpServerElicitationOutcome { if self.services.mcp_runtime.elicitations_auto_deny() { - return Ok(McpServerElicitationOutcome { + return McpServerElicitationOutcome { response: Some(ElicitationResponse { action: codex_rmcp_client::ElicitationAction::Accept, content: Some(serde_json::json!({})), meta: None, }), sent: false, - }); + }; } let _elicitation = self.services.elicitations.register(); @@ -619,10 +615,10 @@ impl Session { plugin_install_telemetry.tool_name.as_str(), ); } - Ok(McpServerElicitationOutcome { + McpServerElicitationOutcome { response: rx_response.await.ok(), sent: true, - }) + } } #[expect( diff --git a/codex-rs/core/src/session/mcp_runtime.rs b/codex-rs/core/src/session/mcp_runtime.rs index 9c0d291cc5..2bbee2d5ed 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -374,7 +374,6 @@ impl Session { client_mcp_extensions: self.services.client_mcp_extensions.for_mcp_servers(), auth, auth_manager: Some(Arc::clone(&self.services.auth_manager)), - allow_user_interaction: !desired.session_source.is_non_root_agent(), elicitation_reviewer, elicitation_lifecycle: Some(self.mcp_elicitation_lifecycle()), } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8417461e78..5370f5c24d 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -638,9 +638,17 @@ async fn regular_turn_emits_turn_started_with_trace_id_without_waiting_for_start sess.abort_all_tasks(TurnAbortReason::Interrupted).await; } +#[test_case(SessionSource::Exec; "root")] +#[test_case(SessionSource::SubAgent(SubAgentSource::Review); "subagent")] +#[test_case(SessionSource::Internal(InternalSessionSource::Guardian); "guardian")] #[tokio::test] -async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled() { - let (session, turn_context, rx) = make_session_and_context_with_rx().await; +async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled( + source: SessionSource, +) { + let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; + Arc::get_mut(&mut turn_context) + .expect("turn context should not be shared") + .session_source = source; session .services .mcp_runtime @@ -660,8 +668,7 @@ async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled() }), }, ) - .await - .expect("root thread elicitation should be accepted"); + .await; assert_eq!( response.response, @@ -675,56 +682,78 @@ async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled() assert!(rx.try_recv().is_err()); } -#[test_case(false; "interactive")] -#[test_case(true; "auto_accept")] +#[test_case(SessionSource::Exec; "root")] +#[test_case(SessionSource::SubAgent(SubAgentSource::Review); "subagent")] +#[test_case(SessionSource::Internal(InternalSessionSource::Guardian); "guardian")] #[tokio::test] -async fn request_mcp_server_elicitation_rejects_non_root_threads(auto_deny: bool) { - for source in [ - SessionSource::SubAgent(SubAgentSource::Review), - SessionSource::Internal(InternalSessionSource::Guardian), - ] { - let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; - Arc::get_mut(&mut turn_context) - .expect("turn context should not be shared") - .session_source = source; - *session.active_turn.lock().await = Some(ActiveTurn::default()); - session - .services - .mcp_runtime - .set_elicitations_auto_deny(auto_deny); - let paused = session.subscribe_elicitation_pause_state(); +async fn request_mcp_server_elicitation_waits_for_user_response(source: SessionSource) { + let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; + Arc::get_mut(&mut turn_context) + .expect("turn context should not be shared") + .session_source = source; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + let paused = session.subscribe_elicitation_pause_state(); + let request = ElicitationRequest::Url { + meta: None, + message: "Connect this app to continue.".to_string(), + url: "https://example.com/connect".to_string(), + elicitation_id: "connect-1".to_string(), + }; + let pending = tokio::spawn({ + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + let request = request.clone(); + async move { + session + .request_mcp_server_elicitation( + &turn_context, + "codex_apps".to_string(), + RequestId::String("request-1".into()), + request, + ) + .await + } + }); + let event = tokio::time::timeout(Duration::from_secs(1), rx.recv()) + .await + .expect("elicitation event should arrive") + .expect("event channel should remain open"); + let EventMsg::ElicitationRequest(event) = event.msg else { + panic!("expected MCP elicitation"); + }; + assert_eq!( + event, + codex_protocol::approvals::ElicitationRequestEvent { + turn_id: Some(turn_context.sub_id.clone()), + server_name: "codex_apps".to_string(), + id: codex_protocol::mcp::RequestId::String("request-1".to_string()), + request, + } + ); + assert!(*paused.borrow()); + assert!(!pending.is_finished()); - let Err(error) = tokio::time::timeout( - Duration::from_secs(1), - session.request_mcp_server_elicitation( - turn_context.as_ref(), - "codex_apps".to_string(), - RequestId::String("request-1".into()), - ElicitationRequest::Url { - meta: None, - message: "Connect this app to continue.".to_string(), - url: "https://example.com/connect".to_string(), - elicitation_id: "connect-1".to_string(), - }, - ), + let response = ElicitationResponse { + action: ElicitationAction::Accept, + content: None, + meta: None, + }; + session + .resolve_elicitation( + "codex_apps".to_string(), + RequestId::String("request-1".into()), + response.clone(), ) .await - .expect("non-root elicitation must not wait for user input") else { - panic!("non-root elicitation must be rejected"); - }; - - assert_eq!( - error.to_string(), - codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE - ); - assert!(rx.try_recv().is_err()); - assert!(!*paused.borrow()); - assert!( - !paused - .has_changed() - .expect("elicitation service should remain available") - ); - } + .expect("user response should resolve the elicitation"); + let outcome = tokio::time::timeout(Duration::from_secs(1), pending) + .await + .expect("elicitation should finish") + .expect("elicitation task should succeed"); + assert_eq!(outcome.response, Some(response)); + assert!(outcome.sent); + assert!(!*paused.borrow()); + assert!(rx.try_recv().is_err()); } #[tokio::test] diff --git a/codex-rs/core/src/tools/handlers/request_plugin_install.rs b/codex-rs/core/src/tools/handlers/request_plugin_install.rs index a7271f3ec8..cb08cc5e13 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -254,8 +254,7 @@ impl RequestPluginInstallHandler { request_id, request, ) - .await - .map_err(|error| FunctionCallError::RespondToModel(error.to_string()))?; + .await; let response = elicitation.response; if let Some(response) = response.as_ref() { maybe_persist_disabled_install_request(&session, &turn, &tool, response).await; diff --git a/codex-rs/core/tests/suite/mcp_auth_elicitation.rs b/codex-rs/core/tests/suite/mcp_auth_elicitation.rs index 219ce0fc7c..c324d05fd2 100644 --- a/codex-rs/core/tests/suite/mcp_auth_elicitation.rs +++ b/codex-rs/core/tests/suite/mcp_auth_elicitation.rs @@ -1,4 +1,4 @@ -//! Verify MCP prompts, non-root refusal, and elicitation analytics through actual turns. +//! Verify MCP prompts and elicitation analytics through actual turns. use anyhow::Result; use codex_analytics::AnalyticsEventsClient; @@ -15,11 +15,11 @@ use codex_extension_api::ToolLifecycleFuture; use codex_features::Feature; use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; -use codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE; use codex_protocol::approvals::ElicitationRequest; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::items::McpToolCallStatus; use codex_protocol::items::TurnItem; +use codex_protocol::mcp::CallToolResult; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ElicitationAction; @@ -524,15 +524,15 @@ enum SubagentRequestKind { ConnectorAuth(AuthFailureFormat), } -#[test_case(SubagentRequestKind::ToolApproval, None; "child_tool_approval_is_rejected_before_execution")] -#[test_case(SubagentRequestKind::LegacyToolApproval, None; "child_legacy_tool_approval_is_rejected_before_execution")] -#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Text), None; "child_connector_auth_preserves_text_diagnostics_and_requests_handoff")] -#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Structured), None; "child_connector_auth_preserves_structured_diagnostics_and_requests_handoff")] -#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Structured), Some(1); "child_connector_auth_structured_diagnostics_respect_output_limit")] +#[test_case(SubagentRequestKind::ToolApproval, ElicitationAction::Accept; "child_tool_approval_waits_for_user_before_execution")] +#[test_case(SubagentRequestKind::LegacyToolApproval, ElicitationAction::Accept; "child_legacy_tool_approval_is_rejected_before_execution")] +#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Text), ElicitationAction::Accept; "child_connector_auth_accepts_text_failure")] +#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Structured), ElicitationAction::Accept; "child_connector_auth_accepts_structured_failure")] +#[test_case(SubagentRequestKind::ConnectorAuth(AuthFailureFormat::Structured), ElicitationAction::Decline; "child_declined_connector_auth_preserves_original_failure")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn core_generated_mcp_elicitations_are_root_only( +async fn core_generated_mcp_elicitations_support_subagents( kind: SubagentRequestKind, - tool_output_token_limit: Option, + decision: ElicitationAction, ) -> Result<()> { skip_if_no_network!(Ok(())); let server = responses::start_mock_server().await; @@ -556,7 +556,6 @@ async fn core_generated_mcp_elicitations_are_root_only( .with_config(move |config| { config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.approvals_reviewer = ApprovalsReviewer::User; - config.tool_output_token_limit = tool_output_token_limit; config .features .enable(Feature::AuthElicitation) @@ -633,28 +632,72 @@ approvals_reviewer = "user" .await?; let mut completed_status = None; let mut completed_result = None; - wait_for_event(&child, |event| { - assert!( - !matches!( + let mut prompts = 0; + loop { + let event = wait_for_event(&child, |event| { + assert!( + !matches!(event, EventMsg::RequestUserInput(_)), + "subagents must not fall back to request_user_input" + ); + if let EventMsg::ItemCompleted(event) = event + && let TurnItem::McpToolCall(item) = &event.item + && item.id == "calendar-call" + { + completed_status = Some(item.status); + completed_result = item.result.clone(); + } + matches!( event, - EventMsg::ElicitationRequest(_) | EventMsg::RequestUserInput(_) - ), - "child MCP tool calls must not prompt the user" + EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ElicitationRequest(request) = event else { + break; + }; + assert_eq!(prompts, 0, "one tool call must not prompt twice"); + prompts += 1; + assert_eq!(request.server_name, CODEX_APPS_MCP_SERVER_NAME); + assert!( + follow_up.requests().is_empty(), + "the tool must wait for input" ); - if let EventMsg::ItemCompleted(event) = event - && let TurnItem::McpToolCall(item) = &event.item - && item.id == "calendar-call" - { - completed_status = Some(item.status); - completed_result = item.result.clone(); + match kind { + SubagentRequestKind::ToolApproval => { + assert!(matches!(request.request, ElicitationRequest::Form { .. })); + assert!(recorded_apps_tool_calls(&server).await.is_empty()); + } + SubagentRequestKind::ConnectorAuth(_) => { + assert!(matches!(request.request, ElicitationRequest::Url { .. })); + assert_eq!(recorded_apps_tool_calls(&server).await.len(), 1); + } + SubagentRequestKind::LegacyToolApproval => { + panic!("legacy subagent approval must be rejected before prompting"); + } } - matches!(event, EventMsg::TurnComplete(_)) - }) - .await; - assert_eq!(completed_status, Some(McpToolCallStatus::Failed)); + child + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision, + content: None, + meta: None, + }) + .await?; + } + let expects_prompt = !matches!(kind, SubagentRequestKind::LegacyToolApproval); + assert_eq!(prompts, usize::from(expects_prompt)); + assert_eq!( + completed_status, + Some(if matches!(kind, SubagentRequestKind::ToolApproval) { + McpToolCallStatus::Completed + } else { + McpToolCallStatus::Failed + }) + ); assert_eq!( recorded_apps_tool_calls(&server).await.len(), - usize::from(matches!(kind, SubagentRequestKind::ConnectorAuth(_))) + usize::from(expects_prompt) ); let output = follow_up .single_request() @@ -668,49 +711,33 @@ approvals_reviewer = "user" .join("\n"), other => panic!("unexpected MCP output: {other}"), }; - assert_eq!( - output_text.contains(MCP_ELICITATION_HANDOFF_MESSAGE), - tool_output_token_limit.is_none(), - "handoff guidance must use the ordinary tool-output limit" - ); - if tool_output_token_limit.is_some() { - assert!(output_text.contains("truncated") || output_text.contains("omitted")); + if matches!(kind, SubagentRequestKind::LegacyToolApproval) { + assert!(output_text.contains("parent agent")); } if let SubagentRequestKind::ConnectorAuth(format) = kind { - let original = AuthFailureResponder { - scenario: Scenario::DefaultAuth, - format, - } - .result(); - let completed_result = completed_result.expect("completed auth failure result"); - assert_eq!(completed_result.is_error, Some(true)); - assert_eq!(completed_result.meta, Some(original["_meta"].clone())); - assert_eq!(completed_result.structured_content, None); - - let mut expected_content = vec![json!({ - "type": "text", - "text": format!( - "Authentication for Calendar could not be completed. {MCP_ELICITATION_HANDOFF_MESSAGE}" - ), - })]; - expected_content.extend( - original["content"] - .as_array() - .expect("original content") - .clone(), - ); - if let Some(structured_content) = original.get("structuredContent") { - expected_content.push(json!({"type": "text", "text": structured_content.to_string()})); - } - assert_eq!(completed_result.content, expected_content); - for content in &expected_content { - let text = content["text"].as_str().expect("auth diagnostic text"); - assert_eq!( - output_text.contains(text), - tool_output_token_limit.is_none(), - "auth diagnostics and guidance must share the ordinary tool-output limit" - ); - } + let original: CallToolResult = serde_json::from_value( + AuthFailureResponder { + scenario: Scenario::DefaultAuth, + format, + } + .result(), + )?; + let expected_result = if decision == ElicitationAction::Accept { + let text = + "Authentication for Calendar was requested and accepted. Retry this tool call now."; + assert!(output_text.contains(text)); + assert!(!output_text.contains(PRIVATE_SENTINEL)); + CallToolResult { + content: vec![json!({"type": "text", "text": text})], + structured_content: None, + is_error: Some(true), + meta: original.meta, + } + } else { + assert!(output_text.contains("synthetic-structured-auth-failure")); + original + }; + assert_eq!(completed_result, Some(expected_result)); assert!( !output_text.contains("link_123"), "private auth metadata must not reach the model" diff --git a/codex-rs/core/tests/suite/mcp_auth_refresh.rs b/codex-rs/core/tests/suite/mcp_auth_refresh.rs index 64778694e0..6a9f3ec62c 100644 --- a/codex-rs/core/tests/suite/mcp_auth_refresh.rs +++ b/codex-rs/core/tests/suite/mcp_auth_refresh.rs @@ -109,7 +109,6 @@ async fn hosted_plugin_runtime_ps_mcp_tool_calls_use_current_auth_manager_token( client_mcp_extensions: ClientMcpExtensions::default(), auth: Some(expected_auth.clone()), auth_manager: Some(Arc::clone(&auth_manager)), - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, }) diff --git a/codex-rs/core/tests/suite/mcp_subagent_elicitation.rs b/codex-rs/core/tests/suite/mcp_subagent_elicitation.rs index 752379a982..e63a97f9a0 100644 --- a/codex-rs/core/tests/suite/mcp_subagent_elicitation.rs +++ b/codex-rs/core/tests/suite/mcp_subagent_elicitation.rs @@ -1,4 +1,4 @@ -//! MCP requests keep human input on the root and allow automatic approval in subagents. +//! MCP server elicitations reach subagents and preserve automatic approval and review. use anyhow::Result; use codex_core::StartThreadOptions; @@ -26,8 +26,6 @@ use serde_json::json; use test_case::test_case; use wiremock::matchers::body_partial_json; -const ROOT_ONLY_MESSAGE: &str = codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE; - const SERVER: &str = r#" import json import sys @@ -76,16 +74,19 @@ pub(super) enum Caller { #[derive(Clone, Copy)] pub(super) enum RequestKind { BrowserAuth, + FormInput, Permission, StrictReview, } -#[test_case(Caller::FullAccessSubagent, RequestKind::BrowserAuth; "full_access_cannot_auto_accept_subagent_browser_auth")] +#[test_case(Caller::FullAccessSubagent, RequestKind::BrowserAuth; "full_access_subagent_preserves_automatic_approval")] #[test_case(Caller::Root, RequestKind::BrowserAuth; "root_browser_auth_remains_interactive")] +#[test_case(Caller::Subagent, RequestKind::FormInput; "subagent_receives_form_input")] +#[test_case(Caller::Subagent, RequestKind::Permission; "subagent_permission_remains_interactive")] #[test_case(Caller::FullAccessSubagent, RequestKind::Permission; "full_access_subagent_automatically_approves_permission")] #[test_case(Caller::Subagent, RequestKind::StrictReview; "subagent_keeps_strict_automatic_safety_review")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn mcp_server_elicitations_keep_user_interaction_on_root( +async fn mcp_server_elicitations_follow_approval_policy( caller: Caller, request_kind: RequestKind, ) -> Result<()> { @@ -117,6 +118,9 @@ pub(super) async fn mcp_server_elicitation_scenario( "fields": [{"id": "password", "label": "Password", "type": "password", "required": true}] }); } + RequestKind::FormInput => { + elicitation["requestedSchema"]["properties"] = json!({"answer": {"type": "string"}}); + } RequestKind::Permission | RequestKind::StrictReview => { elicitation["_meta"] = json!({ "codex_request_type": "approval_request", @@ -137,8 +141,9 @@ pub(super) async fn mcp_server_elicitation_scenario( "default_tools_approval_mode": "approve" } }))?)?; - let expects_prompt = matches!(caller, Caller::Root); - let session_source = if expects_prompt { + let full_access = matches!(caller, Caller::FullAccessSubagent); + let expects_prompt = !full_access && !matches!(request_kind, RequestKind::StrictReview); + let session_source = if matches!(caller, Caller::Root) { SessionSource::Exec } else { SessionSource::SubAgent(SubAgentSource::ThreadSpawn { @@ -202,7 +207,6 @@ pub(super) async fn mcp_server_elicitation_scenario( ]), ) .await; - let full_access = matches!(caller, Caller::FullAccessSubagent); thread .start_or_steer_turn( TurnInputRequest::user_input(vec![UserInput::Text { @@ -224,6 +228,11 @@ pub(super) async fn mcp_server_elicitation_scenario( }), ) .await?; + let response_content = if matches!(request_kind, RequestKind::FormInput) { + json!({"answer": "continue"}) + } else { + json!({}) + }; let mut prompts = 0; loop { let event = wait_for_event(&thread, |event| { @@ -238,7 +247,7 @@ pub(super) async fn mcp_server_elicitation_scenario( }; assert!( expects_prompt, - "subagent MCP elicitations must not reach the user" + "automatically handled elicitations must not prompt the user" ); assert_eq!(prompts, 0, "one tool call must not prompt twice"); prompts += 1; @@ -251,7 +260,7 @@ pub(super) async fn mcp_server_elicitation_scenario( server_name: request.server_name, request_id: request.id, decision: ElicitationAction::Accept, - content: Some(json!({})), + content: Some(response_content.clone()), meta: None, }) .await?; @@ -270,10 +279,8 @@ pub(super) async fn mcp_server_elicitation_scenario( "action": "accept", "content": {}, "_meta": {"approvals_reviewer": "auto_review"} }) - } else if expects_prompt || full_access && matches!(request_kind, RequestKind::Permission) { - json!({"action": "accept", "content": {}}) } else { - json!({"code": -32603, "message": ROOT_ONLY_MESSAGE}) + json!({"action": "accept", "content": response_content}) }; assert_eq!(response, expected); let mut requests = tool_call.requests(); diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index e3c10d13a8..866e7a4de0 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -888,7 +888,7 @@ async fn astra_refreshes_plugin_tools_and_skills_in_an_existing_thread() -> Resu } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn subagent_browser_auth_returns_handoff_without_prompting() -> Result<()> { +async fn subagent_browser_auth_resolves_user_prompt() -> Result<()> { skip_if_no_network!(Ok(())); skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter"); use super::mcp_subagent_elicitation::Caller; @@ -898,7 +898,7 @@ async fn subagent_browser_auth_returns_handoff_without_prompting() -> Result<()> let requests = mcp_server_elicitation_scenario(Caller::Subagent, RequestKind::BrowserAuth).await?; let snapshot = context_snapshot::format_request_history_snapshot( - "An MCP browser sign-in request fails in a subagent without prompting the user; the next model request contains guidance to ask the parent.", + "A subagent waits for an MCP browser sign-in prompt, then receives the accepted response and continues.", &requests, &ContextSnapshotOptions::default() .rewrite_known_segments() @@ -907,7 +907,7 @@ async fn subagent_browser_auth_returns_handoff_without_prompting() -> Result<()> let snapshot = regex_lite::Regex::new(r"Wall time: [0-9]+(?:\.[0-9]+)? seconds")? .replace_all(&snapshot, "Wall time: seconds") .into_owned(); - insta::assert_snapshot!("subagent_browser_auth_handoff", snapshot); + insta::assert_snapshot!("subagent_browser_auth", snapshot); Ok(()) } diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth_handoff.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth.snap similarity index 87% rename from codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth_handoff.snap rename to codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth.snap index 41d40d25dd..2700ea2efa 100644 --- a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth_handoff.snap +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth.snap @@ -2,7 +2,7 @@ source: core/tests/suite/scenarios.rs expression: snapshot --- -Scenario: An MCP browser sign-in request fails in a subagent without prompting the user; the next model request contains guidance to ask the parent. +Scenario: A subagent waits for an MCP browser sign-in prompt, then receives the accepted response and continues. ## Window 1 Settings: @@ -37,4 +37,4 @@ Settings: -- request 2 (turn) -- 03:function_call/mcp__elicitation.request_input:{} 04:function_call_output:Wall time: seconds - Output: | {"code": -32603, "message": "MCP server elicitations can only be requested by the root thread. Ask the p... until the parent confirms the blocker is resolved."} [hash=096a35c5742e4b6f] + Output: | {"action": "accept", "content": {}} diff --git a/codex-rs/ext/mcp/src/event_stream_tests.rs b/codex-rs/ext/mcp/src/event_stream_tests.rs index c533a90211..30ffcd5f6d 100644 --- a/codex-rs/ext/mcp/src/event_stream_tests.rs +++ b/codex-rs/ext/mcp/src/event_stream_tests.rs @@ -176,7 +176,6 @@ impl Fixture { client_mcp_extensions: Default::default(), auth: Some(auth_value.clone()), auth_manager: Some(Arc::clone(&auth)), - allow_user_interaction: true, elicitation_reviewer: None, elicitation_lifecycle: None, };