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 00f4f86c49..1f3dc7bafa 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,6 +113,7 @@ 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 dda2edd914..1536a0265a 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -222,6 +222,7 @@ impl McpConnectionSet { client_mcp_extensions, auth, auth_manager, + allow_user_interaction, elicitation_reviewer, elicitation_lifecycle, } = input; @@ -258,6 +259,7 @@ impl McpConnectionSet { !previous.servers.is_empty() && previous.elicitation_requests.update( Arc::clone(&config), + allow_user_interaction, elicitation_reviewer.clone(), elicitation_lifecycle.clone(), ) @@ -267,6 +269,7 @@ 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 aadc5d688c..efaaa4ff4a 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -117,6 +117,7 @@ impl McpConnectionSet { approval_policy.value(), permission_profile.get().clone(), ), + /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -912,6 +913,7 @@ 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(), @@ -950,6 +952,7 @@ 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(), @@ -1045,6 +1048,7 @@ 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(), @@ -1089,6 +1093,7 @@ 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, @@ -1180,6 +1185,7 @@ 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(), @@ -1330,6 +1336,7 @@ 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(), @@ -1369,6 +1376,7 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::Never, PermissionProfile::default(), ), + /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, ElicitationRequestRouter::default(), @@ -1382,6 +1390,7 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::OnRequest, PermissionProfile::Disabled ), + /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -1391,6 +1400,7 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() { AskForApproval::Never, PermissionProfile::default() ), + /*allow_user_interaction*/ true, /*reviewer*/ None, /*lifecycle*/ None, )); @@ -1452,6 +1462,7 @@ 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(), @@ -1462,6 +1473,7 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() { AskForApproval::OnRequest, PermissionProfile::default(), ), + /*allow_user_interaction*/ true, /*reviewer*/ None, Some(lifecycle), router.clone(), @@ -1566,6 +1578,7 @@ 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(), @@ -2045,6 +2058,7 @@ 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, }, @@ -2100,6 +2114,7 @@ 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, @@ -2206,6 +2221,7 @@ 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, @@ -4603,6 +4619,7 @@ 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, @@ -4717,6 +4734,7 @@ 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(), @@ -5140,6 +5158,7 @@ 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 } @@ -5150,12 +5169,14 @@ 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, @@ -5204,6 +5225,7 @@ 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!( @@ -5433,6 +5455,7 @@ 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, ); @@ -5907,6 +5930,7 @@ 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, @@ -5965,6 +5989,7 @@ 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, @@ -5999,42 +6024,96 @@ async fn reconciliation_reuses_legacy_stdio_server_when_modern_protocol_is_enabl async fn reconciliation_updates_elicitation_policy_without_restarting_ready_server() { let runtime_context = reusable_server_runtime_context(); let config = reusable_server_config("http://127.0.0.1:1"); - let previous = manager_with_reusable_ready_server( + let mut previous = manager_with_reusable_ready_server( &config, &runtime_context, vec![create_test_tool("docs", "search")], ) .await; - { - let mut authority = previous - .elicitation_requests - .authority - .lock() - .expect("elicitation authority lock"); - let config = Arc::make_mut( - &mut authority - .as_mut() - .expect("test manager should have permission authority") - .config, + 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(), + ); + let (tx_event, events) = async_channel::unbounded(); + let sender = previous.elicitation_requests.make_sender( + "docs".to_string(), + Some(tx_event), + &ClientMcpExtensions::default(), + ); + let elicitation = + codex_rmcp_client::Elicitation::Mcp(ElicitRequestParams::FormElicitationParams { + meta: None, + message: "What should I say?".to_string(), + requested_schema: requested_user_input_schema(), + }); + let response = ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(serde_json::json!({"message": "continue"})), + meta: None, + }; + + for allow_user_interaction in [true, false, true] { + let mcp_config = test_elicitation_config( + "docs", + AskForApproval::OnRequest, + PermissionProfile::default(), ); - config.approval_policy = Constrained::allow_any(AskForApproval::Never); - config.permission_profile = PermissionProfile::Disabled; + 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")); + { + let authority = reconciled + .elicitation_requests + .authority + .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.permission_profile, PermissionProfile::default()); + } + + // A sender captured before reconciliation must observe each ownership update. + let mut pending = sender(NumberOrString::Number(7), elicitation.clone()); + if allow_user_interaction { + assert!(futures::poll!(pending.as_mut()).is_pending()); + let EventMsg::ElicitationRequest(request) = + events.try_recv().expect("user-input event").msg + else { + panic!("expected MCP elicitation"); + }; + let codex_protocol::mcp::RequestId::String(request_id) = request.id else { + panic!("expected Codex-owned string request ID"); + }; + router + .resolve( + "docs".to_string(), + NumberOrString::String(request_id.into()), + response.clone(), + ) + .await + .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!(events.is_empty()); + previous = reconciled; } - - let reconciled = reconcile_reusable_server(&previous, config, runtime_context).await; - - assert!(previous.shares_test_connection_with(&reconciled, "docs")); - let authority = reconciled - .elicitation_requests - .authority - .lock() - .expect("elicitation authority lock"); - let config = &authority - .as_ref() - .expect("reconciled manager should have permission authority") - .config; - assert_eq!(config.approval_policy.value(), AskForApproval::OnRequest); - assert_eq!(config.permission_profile, PermissionProfile::default()); } #[tokio::test] @@ -6220,6 +6299,7 @@ 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")); @@ -6231,6 +6311,7 @@ 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 ec3d3f0d0f..65eeaa724c 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -4,6 +4,8 @@ //! 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. use std::collections::HashMap; use std::sync::Arc; @@ -24,9 +26,11 @@ 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; @@ -44,6 +48,12 @@ 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; @@ -154,11 +164,78 @@ impl ElicitationRequestRouter { .send(response) .map_err(|_| anyhow!("elicitation response receiver closed")) } + + async fn request_user_interaction( + &self, + events: Option>, + authority: &ElicitationAuthority, + 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, + content: None, + meta: None, + }); + }; + let (delivery_context, response_context) = + if matches!(&request, ElicitationRequest::UserVerification { .. }) { + ( + "failed to deliver user-verification request", + "user-verification response channel closed", + ) + } else { + ( + "failed to deliver MCP elicitation request", + "elicitation request channel closed unexpectedly", + ) + }; + let public_request_id = format!( + "codex-mcp-elicitation-{}", + NEXT_ELICITATION_REQUEST_ID.fetch_add(1, Ordering::Relaxed) + ); + let request_key = ( + server_name.clone(), + RequestId::String(public_request_id.clone().into()), + ); + let (tx, rx) = oneshot::channel(); + let _active_elicitation = authority + .lifecycle + .as_ref() + .map(ElicitationLifecycle::start); + self.requests + .lock() + .map_err(|_| anyhow!("elicitation request router unavailable"))? + .insert(request_key.clone(), tx); + let _pending_request = PendingElicitationRequest { + router: self.clone(), + key: request_key, + }; + events + .send(Event { + id: "mcp_elicitation_request".to_string(), + msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { + turn_id: None, + server_name, + id: ProtocolRequestId::String(public_request_id), + request, + }), + }) + .await + .context(delivery_context)?; + rx.await.context(response_context) + } } #[derive(Clone)] pub(crate) struct ElicitationAuthority { pub(crate) config: Arc, + pub(crate) allow_user_interaction: bool, reviewer: Option, lifecycle: Option, } @@ -172,6 +249,7 @@ pub(crate) struct ElicitationRequestManager { impl ElicitationRequestManager { pub(crate) fn new( config: Arc, + allow_user_interaction: bool, reviewer: Option, lifecycle: Option, router: ElicitationRequestRouter, @@ -180,6 +258,7 @@ impl ElicitationRequestManager { router, authority: Arc::new(StdMutex::new(Some(ElicitationAuthority { config, + allow_user_interaction, reviewer, lifecycle, }))), @@ -189,6 +268,7 @@ impl ElicitationRequestManager { pub(crate) fn update( &self, config: Arc, + allow_user_interaction: bool, reviewer: Option, lifecycle: Option, ) -> bool { @@ -197,6 +277,7 @@ impl ElicitationRequestManager { }; *authority = Some(ElicitationAuthority { config, + allow_user_interaction, reviewer, lifecycle, }); @@ -223,6 +304,40 @@ impl ElicitationRequestManager { let server_name = server_name.clone(); let authority = authority.clone(); async move { + let authority = authority + .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, @@ -270,8 +385,7 @@ impl ElicitationRequestManager { }); } - let Ok(Some(authority)) = authority.lock().map(|authority| authority.clone()) - else { + let Some(authority) = authority else { return Ok(ElicitationResponse { action: ElicitationAction::Decline, content: None, @@ -282,7 +396,8 @@ impl ElicitationRequestManager { config, reviewer, lifecycle, - } = authority; + .. + } = &authority; let approval_policy = config.approval_policy.value(); let Some(permission_profile) = config.permission_profile_for_server(&server_name) else { @@ -400,19 +515,6 @@ impl ElicitationRequestManager { } } - let Some(tx_event) = tx_event else { - return Ok(ElicitationResponse { - action: ElicitationAction::Decline, - content: None, - meta: None, - }); - }; - - let public_request_id = format!( - "codex-mcp-elicitation-{}", - NEXT_ELICITATION_REQUEST_ID.fetch_add(1, Ordering::Relaxed) - ); - let routed_request_id = RequestId::String(public_request_id.clone().into()); let request = match elicitation { Elicitation::UserVerification { .. } => { return Ok(ElicitationResponse { @@ -474,32 +576,9 @@ impl ElicitationRequestManager { requested_schema, }, }; - let (tx, rx) = oneshot::channel(); - let _active_elicitation = lifecycle.as_ref().map(ElicitationLifecycle::start); - let request_key = (server_name.clone(), routed_request_id); router - .requests - .lock() - .map_err(|_| anyhow!("elicitation request router unavailable"))? - .insert(request_key.clone(), tx); - let _pending_request = PendingElicitationRequest { - router: router.clone(), - key: request_key, - }; - tx_event - .send(Event { - id: "mcp_elicitation_request".to_string(), - msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { - turn_id: None, - server_name, - id: ProtocolRequestId::String(public_request_id), - request, - }), - }) + .request_user_interaction(tx_event, &authority, server_name, request) .await - .context("failed to deliver MCP elicitation request")?; - rx.await - .context("elicitation request channel closed unexpectedly") } .boxed() }) diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs index 3e9f0867f4..4f2472bff9 100644 --- a/codex-rs/codex-mcp/src/elicitation_tests.rs +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -38,11 +38,17 @@ impl ElicitationReviewer for RecordingReviewer { assert_eq!(request.server_name, "independent-mcp"); self.calls.fetch_add(/*val*/ 1, Relaxed); let active_elicitations = self.active_elicitations.clone(); + let active_during_review = usize::from( + request + .elicitation + .meta() + .is_some_and(|meta| meta.get(STRICT_AUTO_REVIEW_KEY) == Some(&Value::Bool(true))), + ); let response = self.response.clone(); async move { - assert_eq!(active_elicitations.load(Relaxed), 1); + assert_eq!(active_elicitations.load(Relaxed), active_during_review); tokio::task::yield_now().await; - assert_eq!(active_elicitations.load(Relaxed), 1); + assert_eq!(active_elicitations.load(Relaxed), active_during_review); response.map_err(anyhow::Error::msg) } .boxed() @@ -87,6 +93,7 @@ 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(), @@ -113,6 +120,243 @@ 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}); + [ + Elicitation::Mcp(ElicitRequestParams::FormElicitationParams { + meta: meta + .as_ref() + .map(|meta| RequestMetaObject::from(meta.as_object().unwrap().clone())), + message: "Review this request".into(), + requested_schema: serde_json::from_value(schema.clone()).unwrap(), + }), + Elicitation::OpenAiForm { + meta: meta.clone(), + message: "Review this request".into(), + requested_schema: schema.clone(), + }, + Elicitation::OpenAiElicitationForm { + meta, + message: "Review this request".into(), + requested_schema: schema, + }, + ] +} + +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() { + for (strict_auto_review, approval_policy, expected_reviews) in [ + (false, AskForApproval::OnRequest, 1), + (true, AskForApproval::OnRequest, 1), + (true, AskForApproval::Never, 1), + (false, AskForApproval::Never, 0), + ] { + for response in [ + approved_response(), + ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: Some(json!({"approvals_reviewer": "auto_review", "message": "Denied"})), + }, + ] { + let reviewer = RecordingReviewer::new(Ok(Some(response.clone()))); + let (manager, events, sender) = elicitation_fixture( + approval_policy, + PermissionProfile::Disabled, + Some(reviewer.clone()), + ); + disable_user_interaction(&manager); + let [request, _, _] = form_elicitations( + Some(json!({ + "codex_request_type": "approval_request", + "codex_approval_kind": "mcp_tool_call", + "codex_strict_auto_review": strict_auto_review, + "tool_name": "access_browser_origin", + "tool_params": {"origin": "https://example.com"}, + })), + json!({}), + ); + let expected = if expected_reviews == 0 { + ElicitationResponse { + meta: None, + ..approved_response() + } + } else { + response + }; + assert_eq!( + sender(RequestId::Number(7), request).await.unwrap(), + expected + ); + assert_eq!(reviewer.calls.load(Relaxed), expected_reviews); + assert_eq!(reviewer.active_elicitations.load(Relaxed), 0); + assert!(events.is_empty()); + assert!(manager.router.requests.lock().unwrap().is_empty()); + } + } +} + +#[tokio::test] +async fn subagent_without_an_automatic_decision_does_not_prompt() { + 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; + } + 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.active_elicitations.load(Relaxed), 0); + } + assert!(events.is_empty()); + assert!(manager.router.requests.lock().unwrap().is_empty()); + } +} + +#[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); @@ -150,6 +394,7 @@ fn closed_event_channel_immediately_cleans_up_pending_elicitation() { AskForApproval::OnRequest, PermissionProfile::Disabled ), + /*allow_user_interaction*/ true, /*reviewer*/ None, Some(lifecycle), )); @@ -338,6 +583,7 @@ 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(), @@ -368,6 +614,7 @@ 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, )); @@ -391,6 +638,7 @@ 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, )); @@ -403,6 +651,7 @@ 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, )); @@ -434,6 +683,7 @@ 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(), @@ -574,6 +824,18 @@ async fn user_verification_cancels_for_an_event_receiver_without_host_activation async fn user_verification_drops_pending_response_when_the_request_is_cancelled() { let (manager, events, sender) = verification_fixture(AskForApproval::OnRequest, /*reviewer*/ None); + let active_elicitations = Arc::new(AtomicUsize::new(0)); + let registrations = active_elicitations.clone(); + manager + .authority + .lock() + .unwrap() + .as_mut() + .unwrap() + .lifecycle = Some(ElicitationLifecycle::new(move || { + registrations.fetch_add(/*val*/ 1, Relaxed); + LifecycleRegistration(registrations.clone()) + })); let pending = tokio::spawn(sender( RequestId::Number(7), Elicitation::UserVerification { @@ -583,9 +845,11 @@ async fn user_verification_drops_pending_response_when_the_request_is_cancelled( }, )); events.recv().await.unwrap(); + assert_eq!(active_elicitations.load(Relaxed), 1); pending.abort(); assert!(pending.await.unwrap_err().is_cancelled()); assert!(manager.router.requests.lock().unwrap().is_empty()); + assert_eq!(active_elicitations.load(Relaxed), 0); } #[tokio::test] diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 77dc37f1a6..9acb97e52d 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -8,6 +8,7 @@ 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 a65da6e697..0f902be3fb 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -458,6 +458,7 @@ 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, }, @@ -537,6 +538,7 @@ 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 804ae9e38a..17aa9f557b 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -71,7 +71,7 @@ pub enum McpStartupPolicy { LazyWhenCached, } -/// Everything needed to materialize one exact MCP configuration. +/// Configuration and owning-thread state needed to materialize an MCP runtime. pub struct McpRuntimeInput { pub startup_policy: McpStartupPolicy, pub config: Arc, @@ -88,6 +88,8 @@ 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/codex-mcp/src/user_verification_elicitation.rs b/codex-rs/codex-mcp/src/user_verification_elicitation.rs index 5b2890400c..9c9334aa70 100644 --- a/codex-rs/codex-mcp/src/user_verification_elicitation.rs +++ b/codex-rs/codex-mcp/src/user_verification_elicitation.rs @@ -5,14 +5,10 @@ use super::*; pub(super) async fn route( router: ElicitationRequestRouter, events: Option>, - authority: Arc>>, + authority: Option, server_name: String, request: ElicitationRequest, ) -> Result { - let authority = authority - .lock() - .ok() - .and_then(|authority| authority.clone()); let plugin_service = authority.as_ref().is_some_and(|authority| { authority .config @@ -24,42 +20,17 @@ pub(super) async fn route( .is_host_owned_apps(&server_name, server.config()) }) }); - let Some(events) = events.filter(|_| plugin_service && !router.auto_deny()) else { + let Some((events, authority)) = events + .zip(authority) + .filter(|_| plugin_service && !router.auto_deny()) + else { return Ok(ElicitationResponse { action: ElicitationAction::Cancel, content: None, meta: None, }); }; - let id = format!( - "codex-mcp-elicitation-{}", - NEXT_ELICITATION_REQUEST_ID.fetch_add(1, Ordering::Relaxed) - ); - let key = (server_name.clone(), RequestId::String(id.clone().into())); - let (response, receiver) = oneshot::channel(); router - .requests - .lock() - .map_err(|_| anyhow!("elicitation request router unavailable"))? - .insert(key.clone(), response); - let _pending = PendingElicitationRequest { router, key }; - let _active = authority - .as_ref() - .and_then(|authority| authority.lifecycle.as_ref()) - .map(ElicitationLifecycle::start); - events - .send(Event { - id: "mcp_elicitation_request".to_string(), - msg: EventMsg::ElicitationRequest(ElicitationRequestEvent { - turn_id: None, - server_name, - id: ProtocolRequestId::String(id), - request, - }), - }) + .request_user_interaction(Some(events), &authority, server_name, request) .await - .context("failed to deliver user-verification request")?; - receiver - .await - .context("user-verification response channel closed") } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 4156fcfc39..f0a4cc2c92 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -269,6 +269,7 @@ 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 2e6c191852..5e2e369a4e 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -781,7 +781,7 @@ async fn maybe_request_codex_apps_auth_elicitation( url: plan.elicitation.url, elicitation_id: plan.elicitation.elicitation_id, }; - let response = sess + let outcome = match sess .request_mcp_server_elicitation( turn_context, CODEX_APPS_MCP_SERVER_NAME.to_string(), @@ -789,8 +789,33 @@ async fn maybe_request_codex_apps_auth_elicitation( request, ) .await - .response; - if !response + { + 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; + } + }; + if !outcome + .response .as_ref() .is_some_and(|response| response.action == ElicitationAction::Accept) { @@ -1597,6 +1622,10 @@ 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 @@ -1662,9 +1691,13 @@ pub(crate) async fn request_mcp_tool_user_approval( .map(|rendered_template| rendered_template.elicitation_message.as_str()), prompt_options, }); - let outcome = sess + let outcome = match sess .request_mcp_server_elicitation(turn_context, server.clone(), request_id, request) - .await; + .await + { + Ok(outcome) => outcome, + Err(error) => return ReviewDecision::denied(error.to_string()), + }; ( outcome.sent, parse_mcp_tool_approval_elicitation_response(outcome.response, &question_id), diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index ae1d03b484..d9f156c631 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -13,6 +13,9 @@ 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; @@ -30,6 +33,7 @@ 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; @@ -1724,6 +1728,88 @@ async fn codex_apps_auth_elicitation_granular_mcp_disabled_returns_original_resu assert!(rx_event.try_recv().is_err()); } +#[tokio::test] +async fn codex_apps_auth_elicitation_returns_subagent_handoff_and_diagnostics() { + 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; diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 70656904ab..11555def8a 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -556,16 +556,20 @@ impl Session { server_name: String, request_id: RequestId, request: ElicitationRequest, - ) -> McpServerElicitationOutcome { + ) -> anyhow::Result { + anyhow::ensure!( + !turn_context.session_source.is_non_root_agent(), + codex_mcp::MCP_ELICITATION_HANDOFF_MESSAGE + ); if self.services.mcp_runtime.elicitations_auto_deny() { - return McpServerElicitationOutcome { + return Ok(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(); @@ -617,10 +621,10 @@ impl Session { plugin_install_telemetry.tool_name.as_str(), ); } - McpServerElicitationOutcome { + Ok(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 b85bbc2ffe..7d6c5e2cfb 100644 --- a/codex-rs/core/src/session/mcp_runtime.rs +++ b/codex-rs/core/src/session/mcp_runtime.rs @@ -404,6 +404,7 @@ 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 394ef4b064..723f85a882 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -87,6 +87,7 @@ use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSandboxPolicyContext; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::protocol::EnvironmentConfigState; +use codex_protocol::protocol::InternalSessionSource; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::TurnEnvironmentSelections; use codex_protocol::request_permissions::PermissionGrantScope; @@ -652,7 +653,8 @@ async fn request_mcp_server_elicitation_auto_accepts_when_auto_deny_is_enabled() }), }, ) - .await; + .await + .expect("root thread elicitation should be accepted"); assert_eq!( response.response, @@ -666,6 +668,58 @@ 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")] +#[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(); + + 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(), + }, + ), + ) + .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") + ); + } +} + #[tokio::test] async fn interrupting_regular_turn_waiting_on_startup_prewarm_emits_turn_aborted() { let (sess, tc, rx) = make_session_and_context_with_rx().await; 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 cb08cc5e13..a7271f3ec8 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -254,7 +254,8 @@ impl RequestPluginInstallHandler { request_id, request, ) - .await; + .await + .map_err(|error| FunctionCallError::RespondToModel(error.to_string()))?; 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 c6024968fa..219ce0fc7c 100644 --- a/codex-rs/core/tests/suite/mcp_auth_elicitation.rs +++ b/codex-rs/core/tests/suite/mcp_auth_elicitation.rs @@ -1,9 +1,10 @@ -//! Verify MCP auth prompts and elicitation analytics through actual turns. +//! Verify MCP prompts, non-root refusal, and elicitation analytics through actual turns. use anyhow::Result; use codex_analytics::AnalyticsEventsClient; use codex_analytics::AppServerRpcTransport; use codex_app_server_protocol as app; +use codex_core::StartThreadOptions; use codex_core::TurnInputRequest; use codex_core::TurnInputSubmission; use codex_core::config::Constrained; @@ -14,13 +15,19 @@ 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::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ElicitationAction; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; +use codex_protocol::protocol::SessionSource; +use codex_protocol::protocol::SubAgentSource; +use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::user_input::UserInput; use core_test_support::PathExt; use core_test_support::apps_test_server::AppsTestServer; @@ -31,6 +38,8 @@ use core_test_support::apps_test_server::recorded_apps_tool_calls; use core_test_support::apps_test_server::search_capable_apps_builder; use core_test_support::responses; use core_test_support::skip_if_no_network; +use core_test_support::wait_for_event; +use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use serde_json::Value; use serde_json::json; @@ -50,44 +59,63 @@ const CALL_ID: &str = "calendar-elicitation-call"; const PRIVATE_SENTINEL: &str = "synthetic-private-value"; #[derive(Clone, Copy)] -struct AuthFailureResponder(Scenario); +enum AuthFailureFormat { + Text, + Structured, +} + +#[derive(Clone, Copy)] +struct AuthFailureResponder { + scenario: Scenario, + format: AuthFailureFormat, +} + +impl AuthFailureResponder { + fn result(self) -> Value { + let mut response = json!({ + "content": [{ + "type": "text", + "text": PRIVATE_SENTINEL, + }], + "isError": true, + "_meta": { + "_codex_apps": { + "connector_auth_failure": { + "is_auth_failure": true, + "auth_reason": "reauthentication_required", + "connector_id": "calendar", + "link_id": "link_123", + "error_code": "UNAUTHORIZED", + "error_http_status_code": 401, + "error_action": "TRIGGER_REAUTHENTICATION", + }, + }, + }, + }); + if self.scenario == Scenario::AuthMetadataRemoved { + response["_meta"] = json!({"_codex_apps": {"connector_auth_failure": { + "is_auth_failure": true, "connector_id": "calendar", + }}}); + } + if matches!(self.format, AuthFailureFormat::Structured) { + response["structuredContent"] = json!({ + "error": "synthetic-structured-auth-failure", + "details": {"reason": "reauthentication_required", "status": 401}, + }); + } + response + } +} impl Respond for AuthFailureResponder { fn respond(&self, request: &Request) -> ResponseTemplate { let body: Value = serde_json::from_slice(&request.body).expect("tools/call request should be valid JSON"); - let id = body.get("id").cloned().unwrap_or(Value::Null); - - let mut response = json!({ + ResponseTemplate::new(/*status*/ 200).set_body_json(json!({ "jsonrpc": "2.0", - "id": id, - "result": { - "content": [{ - "type": "text", - "text": PRIVATE_SENTINEL, - }], - "isError": true, - "_meta": { - "_codex_apps": { - "connector_auth_failure": { - "is_auth_failure": true, - "auth_reason": "reauthentication_required", - "connector_id": "calendar", - "link_id": "link_123", - "error_code": "UNAUTHORIZED", - "error_http_status_code": 401, - "error_action": "TRIGGER_REAUTHENTICATION", - }, - }, - }, - }, - }); - if self.0 == Scenario::AuthMetadataRemoved { - response["result"]["_meta"] = json!({"_codex_apps": {"connector_auth_failure": { - "is_auth_failure": true, "connector_id": "calendar", - }}}); - } - ResponseTemplate::new(/*status*/ 200).set_body_json(response) + "id": body.get("id").cloned().unwrap_or(Value::Null), + "result": self.result(), + })) } } @@ -141,7 +169,10 @@ async fn actual_turn_elicitation_analytics(scenario: Scenario) -> Result<()> { "method": "tools/call", "params": {"name": "calendar_create_event"}, }))) - .respond_with(AuthFailureResponder(scenario)) + .respond_with(AuthFailureResponder { + scenario, + format: AuthFailureFormat::Text, + }) .with_priority(/*p*/ 1) .mount(&server) .await; @@ -485,3 +516,207 @@ approvals_reviewer = "user" Ok(()) } + +#[derive(Clone, Copy)] +enum SubagentRequestKind { + ToolApproval, + LegacyToolApproval, + 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")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn core_generated_mcp_elicitations_are_root_only( + kind: SubagentRequestKind, + tool_output_token_limit: Option, +) -> Result<()> { + skip_if_no_network!(Ok(())); + let server = responses::start_mock_server().await; + let apps = AppsTestServer::mount_searchable(&server).await?; + if let SubagentRequestKind::ConnectorAuth(format) = kind { + Mock::given(method("POST")) + .and(path_regex("^/api/codex/ps/mcp/?$")) + .and(body_partial_json(json!({ + "method": "tools/call", + "params": {"name": "calendar_create_event"} + }))) + .respond_with(AuthFailureResponder { + scenario: Scenario::DefaultAuth, + format, + }) + .with_priority(/*p*/ 1) + .mount(&server) + .await; + } + let test = search_capable_apps_builder(apps.chatgpt_base_url) + .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) + .expect("enable auth elicitation"); + config + .features + .set_enabled( + Feature::ToolCallMcpElicitation, + !matches!(kind, SubagentRequestKind::LegacyToolApproval), + ) + .expect("configure MCP approval transport"); + let approval_mode = match kind { + SubagentRequestKind::ToolApproval | SubagentRequestKind::LegacyToolApproval => { + "prompt" + } + SubagentRequestKind::ConnectorAuth(_) => "approve", + }; + let user_config = toml::from_str(&format!( + r#"[apps.calendar] +default_tools_approval_mode = "{approval_mode}" +approvals_reviewer = "user" +"# + )) + .expect("apps config"); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&config.codex_home.join("config.toml").abs(), user_config) + .expect("apply apps config"); + }) + .build_with_auto_env(&server) + .await?; + let child = test + .thread_manager + .start_thread(StartThreadOptions { + session_source: Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: test.session_configured.thread_id, + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + })), + environments: Some(vec![test.executor_environment().selection().clone()]), + ..StartThreadOptions::new(test.config.clone()) + }) + .await? + .thread; + wait_for_mcp_server(&child, CODEX_APPS_MCP_SERVER_NAME).await?; + responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "calendar-call", + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL, + &json!({"title": "Lunch", "starts_at": "2026-06-18T12:00:00Z"}).to_string(), + ), + responses::ev_completed("tool-response"), + ]), + ) + .await; + let follow_up = responses::mount_sse_once(&server, responses::sse_completed("done")).await; + child + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "Use [$calendar](app://calendar) to create a calendar event.".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::OnRequest), + permission_profile: Some(PermissionProfile::Disabled), + ..Default::default() + }), + ) + .await?; + let mut completed_status = None; + let mut completed_result = None; + wait_for_event(&child, |event| { + assert!( + !matches!( + event, + EventMsg::ElicitationRequest(_) | EventMsg::RequestUserInput(_) + ), + "child MCP tool calls must not prompt the user" + ); + 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::TurnComplete(_)) + }) + .await; + assert_eq!(completed_status, Some(McpToolCallStatus::Failed)); + assert_eq!( + recorded_apps_tool_calls(&server).await.len(), + usize::from(matches!(kind, SubagentRequestKind::ConnectorAuth(_))) + ); + let output = follow_up + .single_request() + .function_call_output("calendar-call"); + let output_text = match &output["output"] { + Value::String(text) => text.clone(), + Value::Array(items) => items + .iter() + .filter_map(|item| item["text"].as_str()) + .collect::>() + .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 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" + ); + } + assert!( + !output_text.contains("link_123"), + "private auth metadata must not reach the model" + ); + } + child.shutdown_and_wait().await?; + test.codex.shutdown_and_wait().await?; + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mcp_auth_refresh.rs b/codex-rs/core/tests/suite/mcp_auth_refresh.rs index 6a9f3ec62c..64778694e0 100644 --- a/codex-rs/core/tests/suite/mcp_auth_refresh.rs +++ b/codex-rs/core/tests/suite/mcp_auth_refresh.rs @@ -109,6 +109,7 @@ 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 new file mode 100644 index 0000000000..752379a982 --- /dev/null +++ b/codex-rs/core/tests/suite/mcp_subagent_elicitation.rs @@ -0,0 +1,288 @@ +//! MCP requests keep human input on the root and allow automatic approval in subagents. + +use anyhow::Result; +use codex_core::StartThreadOptions; +use codex_core::TurnInputRequest; +use codex_protocol::approvals::ElicitationAction; +use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::SessionSource; +use codex_protocol::protocol::SubAgentSource; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; +use core_test_support::responses; +use core_test_support::responses::ResponsesRequest; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; +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; +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 + +def send(message): + print(json.dumps({"jsonrpc": "2.0", **message}), flush=True) + +elicitation = json.loads(sys.argv[1]) +pending = None +for line in sys.stdin: + request = json.loads(line) + method = request.get("method") + if method == "initialize": + result = {"protocolVersion": request["params"]["protocolVersion"], + "capabilities": {"tools": {}}, + "serverInfo": {"name": "elicitation-test", "version": "1"}} + elif method == "tools/list": + result = {"tools": [{"name": "request_input", "inputSchema": {"type": "object", "properties": {}}, + "annotations": {"readOnlyHint": True}}]} + elif method == "tools/call": + pending = request["id"] + send({"id": "input-request", "method": "elicitation/create", "params": elicitation}) + continue + elif method is None and request.get("id") == "input-request": + send({"id": pending, "result": {"content": [{"type": "text", "text": json.dumps( + request.get("result", request.get("error")))}]}}) + continue + elif method == "resources/list": + result = {"resources": []} + elif method == "resources/templates/list": + result = {"resourceTemplates": []} + elif "id" not in request: + continue + else: + result = {} + send({"id": request["id"], "result": result}) +"#; + +#[derive(Clone, Copy)] +pub(super) enum Caller { + Root, + Subagent, + FullAccessSubagent, +} + +#[derive(Clone, Copy)] +pub(super) enum RequestKind { + BrowserAuth, + Permission, + StrictReview, +} + +#[test_case(Caller::FullAccessSubagent, RequestKind::BrowserAuth; "full_access_cannot_auto_accept_subagent_browser_auth")] +#[test_case(Caller::Root, RequestKind::BrowserAuth; "root_browser_auth_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( + caller: Caller, + request_kind: RequestKind, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter"); + mcp_server_elicitation_scenario(caller, request_kind).await?; + Ok(()) +} + +pub(super) async fn mcp_server_elicitation_scenario( + caller: Caller, + request_kind: RequestKind, +) -> Result> { + let server = responses::start_mock_server().await; + let test = test_codex().build_with_auto_env(&server).await?; + let mut elicitation = json!({ + "mode": "form", + "message": "Provide the requested input.", + "requestedSchema": {"type": "object", "properties": {}} + }); + match request_kind { + RequestKind::BrowserAuth => { + // Browser credentials travel through the browser broker, so the MCP schema is empty. + elicitation["_meta"] = json!({ + "codex_approval_kind": "browser_auth", + "browser_auth_challenge_id": "test_browser_auth_challenge_0123456789", + "origin": "https://example.com", + "reason": "Sign in to continue browsing.", + "fields": [{"id": "password", "label": "Password", "type": "password", "required": true}] + }); + } + RequestKind::Permission | RequestKind::StrictReview => { + elicitation["_meta"] = json!({ + "codex_request_type": "approval_request", + "codex_approval_kind": "mcp_tool_call", + "codex_strict_auto_review": matches!(request_kind, RequestKind::StrictReview), + "codex_sensitive_action": true, + "tool_name": "write_record", + "tool_params": {"value": 42} + }); + } + } + let mut config = test.config.clone(); + config.approvals_reviewer = ApprovalsReviewer::User; + config.mcp_servers.set(serde_json::from_value(json!({ + "elicitation": { + "command": if cfg!(windows) { "python" } else { "python3" }, + "args": ["-u", "-c", SERVER, elicitation.to_string()], + "default_tools_approval_mode": "approve" + } + }))?)?; + let expects_prompt = matches!(caller, Caller::Root); + let session_source = if expects_prompt { + SessionSource::Exec + } else { + SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id: test.session_configured.thread_id, + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + }) + }; + let thread = test + .thread_manager + .start_thread(StartThreadOptions { + session_source: Some(session_source), + environments: Some(vec![test.executor_environment().selection().clone()]), + ..StartThreadOptions::new(config) + }) + .await? + .thread; + wait_for_mcp_server(&thread, "elicitation").await?; + let tool_call = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "input-call", + "mcp__elicitation", + "request_input", + "{}", + ), + responses::ev_completed("tool-response"), + ]), + ) + .await; + let guardian = if matches!(request_kind, RequestKind::StrictReview) { + Some( + responses::mount_sse_once_match( + &server, + body_partial_json(json!({"client_metadata": {"x-openai-subagent": "guardian"}})), + responses::sse(vec![ + responses::ev_assistant_message( + "review-result", + &json!({ + "risk_level": "low", "user_authorization": "high", "outcome": "allow", + "rationale": "The user requested this action." + }) + .to_string(), + ), + responses::ev_completed("guardian-review"), + ]), + ) + .await, + ) + } else { + None + }; + let follow_up = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_assistant_message("done-message", "done"), + responses::ev_completed("done-response"), + ]), + ) + .await; + let full_access = matches!(caller, Caller::FullAccessSubagent); + thread + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "Run the input tool.".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(if full_access { + AskForApproval::Never + } else { + AskForApproval::OnRequest + }), + permission_profile: Some(if full_access { + PermissionProfile::Disabled + } else { + PermissionProfile::read_only() + }), + ..Default::default() + }), + ) + .await?; + let mut prompts = 0; + loop { + let event = wait_for_event(&thread, |event| { + matches!( + event, + EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ElicitationRequest(request) = event else { + break; + }; + assert!( + expects_prompt, + "subagent MCP elicitations must not reach the user" + ); + assert_eq!(prompts, 0, "one tool call must not prompt twice"); + prompts += 1; + assert!( + follow_up.requests().is_empty(), + "the tool must wait for input" + ); + thread + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision: ElicitationAction::Accept, + content: Some(json!({})), + meta: None, + }) + .await?; + } + assert_eq!(prompts, usize::from(expects_prompt)); + let output = follow_up + .single_request() + .function_call_output("input-call"); + let response: Value = serde_json::from_str( + output["output"][1]["text"] + .as_str() + .expect("MCP response must reach the model"), + )?; + let expected = if matches!(request_kind, RequestKind::StrictReview) { + json!({ + "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}) + }; + assert_eq!(response, expected); + let mut requests = tool_call.requests(); + if let Some(guardian) = guardian { + assert!(guardian.single_request().body_contains_text("write_record")); + requests.extend(guardian.requests()); + } + requests.extend(follow_up.requests()); + thread.shutdown_and_wait().await?; + test.codex.shutdown_and_wait().await?; + Ok(requests) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index f6e66f5966..e751f952ec 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -113,6 +113,7 @@ mod mcp_optional_startup_grace; #[cfg(unix)] mod mcp_refresh_cleanup; mod mcp_startup_refresh_http_proxy; +mod mcp_subagent_elicitation; mod mcp_tool_cache; mod mcp_tool_exposure; mod mcp_turn_metadata; diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 27784a3ce7..3ba7a0e247 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -756,6 +756,30 @@ async fn astra_refreshes_plugin_tools_and_skills_in_an_existing_thread() -> Resu Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn subagent_browser_auth_returns_handoff_without_prompting() -> 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; + use super::mcp_subagent_elicitation::RequestKind; + use super::mcp_subagent_elicitation::mcp_server_elicitation_scenario; + + 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.", + &requests, + &ContextSnapshotOptions::default() + .rewrite_known_segments() + .include_request_settings(), + ); + 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); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_checkpoint_migration_request_history() -> Result<()> { skip_if_no_network!(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_handoff.snap new file mode 100644 index 0000000000..41d40d25dd --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__subagent_browser_auth_handoff.snap @@ -0,0 +1,40 @@ +--- +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. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + instructions: + model: "gpt-5.5" + parallel_tool_calls: true + prompt_cache_key: "" + reasoning: {"effort":"medium"} + store: false + stream: true + text: {"verbosity":"low"} + tool_choice: "auto" + tools (10; hash=1d541727c70b02b8): + - function/exec_command: Runs a command in a PTY, returning output or a session ID for ongoing interaction.; args=[cmd, justification, login, max_output_tokens, prefix_rule, sandbox_permissions, shell, tty, workdir, yield_time_ms]; hash=c04108e51a8548c5 + - function/write_stdin: Writes characters to an existing unified exec session and returns recent output.; args=[chars, max_output_tokens, session_id, yield_time_ms]; hash=7fb62d8a3e2df4e2 + - function/list_mcp_resources: Lists resources provided by MCP servers. Resources allow servers to share data that provides cont...; args=[cursor, server]; hash=6792edaff13561bd + - function/list_mcp_resource_templates: Lists resource templates provided by MCP servers. Parameterized resource templates allow servers ...; args=[cursor, server]; hash=60f17c90ef7a4ebb + - function/read_mcp_resource: Read a specific resource from an MCP server given the server name and resource URI.; args=[server, uri]; hash=24246896b39008ae + - function/request_user_input: Request user input for one to three short questions and wait for the response. This tool is only ...; args=[questions]; hash=64a729b25c424fc6 + - custom/apply_patch: The `apply_patch` tool can be used to edit files. This is a FREEFORM tool, so do not wrap the pat...; hash=dea9f12b0111a4ba + - function/view_image: View a local image file from the filesystem when visual inspection is needed. Use this for images...; args=[detail, path]; hash=3b766d5bdec5d50e + - tool_search: # Tool discovery Searches over deferred tool metadata with BM25 and exposes matching tools for t...; args=[limit, query]; hash=cea8b3c4515459b0 + - web_search; hash=f103e327c6870a46 +-- request 1 (turn) -- +00:message/developer: + +01:message/user: + +02:message/user: + Run the input tool. +-- 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] diff --git a/codex-rs/ext/mcp/src/event_stream_tests.rs b/codex-rs/ext/mcp/src/event_stream_tests.rs index 30ffcd5f6d..c533a90211 100644 --- a/codex-rs/ext/mcp/src/event_stream_tests.rs +++ b/codex-rs/ext/mcp/src/event_stream_tests.rs @@ -176,6 +176,7 @@ 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, }; diff --git a/codex-rs/protocol/src/mcp_approval_meta.rs b/codex-rs/protocol/src/mcp_approval_meta.rs index b5323708d8..1ed0f2f3fd 100644 --- a/codex-rs/protocol/src/mcp_approval_meta.rs +++ b/codex-rs/protocol/src/mcp_approval_meta.rs @@ -3,7 +3,10 @@ /// form-forwarding safeguards. pub const APPROVAL_KIND_KEY: &str = "codex_approval_kind"; pub const APPROVAL_KIND_MCP_TOOL_CALL: &str = "mcp_tool_call"; +pub const APPROVAL_KIND_BROWSER_AUTH: &str = "browser_auth"; pub const APPROVAL_KIND_TOOL_SUGGESTION: &str = "tool_suggestion"; +/// Marks requests that need user input even when their form schema is empty. +pub const REQUIRES_USER_INPUT_KEY: &str = "codex_requires_user_input"; pub const REQUEST_TYPE_KEY: &str = "codex_request_type"; pub const REQUEST_TYPE_APPROVAL_REQUEST: &str = "approval_request"; pub const STRICT_AUTO_REVIEW_KEY: &str = "codex_strict_auto_review";