From 6aa88f9946c4e369f272388c576d1f32932a81d5 Mon Sep 17 00:00:00 2001 From: jameswt-oai Date: Mon, 9 Nov 2026 23:35:26 +0000 Subject: [PATCH] Remove the Xcode 26.4 MCP elicitation compatibility workaround (#60488) ## What changed Route MCP elicitations through the standard handling paths for Xcode 26.4 clients. Remove the client-specific auto-deny flag and the synthetic acceptance of session elicitation requests without dispatching them to the client. Simplify `request_mcp_server_elicitation` to return the optional response directly, and remove telemetry guards that depended on whether the workaround bypassed dispatch. GitOrigin-RevId: ab540dcd67f8fa6d16c69bc7629f436e4b5aa73a --- .../request_processors/thread_processor.rs | 21 +------- .../src/request_processors/turn_processor.rs | 21 +------- .../codex-mcp/src/connection_manager_tests.rs | 24 ++------- codex-rs/codex-mcp/src/elicitation.rs | 17 ------- codex-rs/codex-mcp/src/elicitation_tests.rs | 8 --- codex-rs/codex-mcp/src/runtime.rs | 8 --- .../src/user_verification_elicitation.rs | 5 +- codex-rs/core/src/codex_delegate_tests.rs | 6 +-- codex-rs/core/src/codex_thread.rs | 7 +-- codex-rs/core/src/mcp_tool_call.rs | 28 ++++------- codex-rs/core/src/session/mcp.rs | 23 +-------- codex-rs/core/src/session/mod.rs | 4 -- codex-rs/core/src/session/tests.rs | 49 +------------------ .../tools/handlers/request_plugin_install.rs | 47 +++++++++--------- .../core/tests/suite/multi_agent_resume.rs | 6 +-- 15 files changed, 45 insertions(+), 229 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/thread_processor.rs b/codex-rs/app-server/src/request_processors/thread_processor.rs index b515ad6d16..734563f903 100644 --- a/codex-rs/app-server/src/request_processors/thread_processor.rs +++ b/codex-rs/app-server/src/request_processors/thread_processor.rs @@ -1129,16 +1129,8 @@ impl ThreadRequestProcessor { app_server_client_name: Option, app_server_client_version: Option, ) -> Result<(), JSONRPCErrorError> { - let mcp_elicitations_auto_deny = xcode_26_4_mcp_elicitations_auto_deny( - app_server_client_name.as_deref(), - app_server_client_version.as_deref(), - ); thread - .set_app_server_client_info( - app_server_client_name, - app_server_client_version, - mcp_elicitations_auto_deny, - ) + .set_app_server_client_info(app_server_client_name, app_server_client_version) .await .map_err(|err| internal_error(format!("failed to set app server client info: {err}"))) } @@ -6182,17 +6174,6 @@ impl ThreadRequestProcessor { } } -fn xcode_26_4_mcp_elicitations_auto_deny( - client_name: Option<&str>, - client_version: Option<&str>, -) -> bool { - // Xcode 26.4 shipped before app-server MCP elicitation requests were - // client-visible. Keep elicitations auto-denied for that client line. - // TODO: Remove this compatibility hack once Xcode 26.4 ages out. - client_name == Some("Xcode") - && client_version.is_some_and(|version| version.starts_with("26.4")) -} - const THREAD_TURNS_DEFAULT_LIMIT: usize = 25; const THREAD_TURNS_MAX_LIMIT: usize = 100; const THREAD_ITEMS_DEFAULT_LIMIT: usize = 25; diff --git a/codex-rs/app-server/src/request_processors/turn_processor.rs b/codex-rs/app-server/src/request_processors/turn_processor.rs index 8fc7afa981..121a3f87d3 100644 --- a/codex-rs/app-server/src/request_processors/turn_processor.rs +++ b/codex-rs/app-server/src/request_processors/turn_processor.rs @@ -1063,16 +1063,8 @@ impl TurnRequestProcessor { app_server_client_name: Option, app_server_client_version: Option, ) -> Result<(), JSONRPCErrorError> { - let mcp_elicitations_auto_deny = xcode_26_4_mcp_elicitations_auto_deny( - app_server_client_name.as_deref(), - app_server_client_version.as_deref(), - ); thread - .set_app_server_client_info( - app_server_client_name, - app_server_client_version, - mcp_elicitations_auto_deny, - ) + .set_app_server_client_info(app_server_client_name, app_server_client_version) .await .map_err(|err| internal_error(format!("failed to set app server client info: {err}"))) } @@ -1562,14 +1554,3 @@ impl TurnRequestProcessor { .await } } - -fn xcode_26_4_mcp_elicitations_auto_deny( - client_name: Option<&str>, - client_version: Option<&str>, -) -> bool { - // Xcode 26.4 shipped before app-server MCP elicitation requests were - // client-visible. Keep elicitations auto-denied for that client line. - // TODO: Remove this compatibility hack once Xcode 26.4 ages out. - client_name == Some("Xcode") - && client_version.is_some_and(|version| version.starts_with("26.4")) -} diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index f324ebd293..ca4edee537 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1197,13 +1197,12 @@ async fn assert_elicitation_declined_with_reviewer_calls( async fn assert_requested_user_input_is_declined( approval_policy: AskForApproval, permission_profile: PermissionProfile, - router: ElicitationRequestRouter, ) { let manager = ElicitationRequestManager::new( test_elicitation_config("server", approval_policy, permission_profile), /*reviewer*/ None, /*lifecycle*/ None, - router, + full_access_form_input_enabled_router(), ); let (tx_event, rx_event) = async_channel::bounded(1); let sender = manager.make_sender( @@ -1240,18 +1239,6 @@ async fn assert_requested_user_input_is_declined( assert!(rx_event.try_recv().is_err()); } -#[tokio::test] -async fn disabled_permissions_do_not_surface_user_input_when_auto_denied() { - let router = full_access_form_input_enabled_router(); - router.set_auto_deny(/*auto_deny*/ true); - assert_requested_user_input_is_declined( - AskForApproval::Never, - PermissionProfile::Disabled, - router, - ) - .await; -} - #[tokio::test] async fn plugin_tool_suggestion_elicitations_are_declined_before_review() { assert_elicitation_declined_with_reviewer_calls( @@ -1393,12 +1380,8 @@ async fn disabled_permissions_decline_requested_user_input_with_approval_metadat #[tokio::test] async fn restricted_never_policy_does_not_surface_requested_user_input() { - assert_requested_user_input_is_declined( - AskForApproval::Never, - PermissionProfile::default(), - full_access_form_input_enabled_router(), - ) - .await; + assert_requested_user_input_is_declined(AskForApproval::Never, PermissionProfile::default()) + .await; } #[tokio::test] @@ -1412,7 +1395,6 @@ async fn granular_policy_does_not_surface_requested_user_input() { mcp_elicitations: false, }), PermissionProfile::Disabled, - full_access_form_input_enabled_router(), ) .await; } diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index 4f4e3ec72d..46d1ad84fd 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -102,7 +102,6 @@ struct ActiveElicitation { pub(crate) struct ElicitationRequestRouter { requests: Arc>, terminal_actions_finished: Arc, - auto_deny: Arc, full_access_form_input_enabled: Arc, } @@ -242,14 +241,6 @@ impl ElicitationRequestRouter { } } - pub(crate) fn auto_deny(&self) -> bool { - self.auto_deny.load(Ordering::Relaxed) - } - - pub(crate) fn set_auto_deny(&self, auto_deny: bool) { - self.auto_deny.store(auto_deny, Ordering::Relaxed); - } - pub(crate) fn full_access_form_input_enabled(&self) -> bool { self.full_access_form_input_enabled.load(Ordering::Acquire) } @@ -480,14 +471,6 @@ impl ElicitationRequestManager { ) .await; } - if router.auto_deny() { - return Ok(ElicitationResponse { - action: ElicitationAction::Decline, - content: None, - meta: None, - }); - } - if elicitation .meta() .and_then(|meta| meta.get(APPROVAL_KIND_KEY)) diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs index 7cd097192f..da44a5938e 100644 --- a/codex-rs/codex-mcp/src/elicitation_tests.rs +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -676,14 +676,6 @@ async fn strict_auto_review_respects_explicit_elicitation_denials() { ); } } - manager.router.set_auto_deny(/*auto_deny*/ true); - assert_eq!( - send_elicitation(&sender, Some(json!(true))).await, - ElicitationResponse { - meta: None, - ..strict_auto_review_decline() - }, - ); assert_eq!( ( reviewer.calls.load(Relaxed), diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 1ebf390e59..166f675a66 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -709,14 +709,6 @@ impl McpRuntime { }) } - pub fn elicitations_auto_deny(&self) -> bool { - self.elicitation_router.auto_deny() - } - - pub fn set_elicitations_auto_deny(&self, auto_deny: bool) { - self.elicitation_router.set_auto_deny(auto_deny); - } - pub fn enable_full_access_form_input(&self) { self.elicitation_router.enable_full_access_form_input(); } diff --git a/codex-rs/codex-mcp/src/user_verification_elicitation.rs b/codex-rs/codex-mcp/src/user_verification_elicitation.rs index b42d026d54..6302dfbd73 100644 --- a/codex-rs/codex-mcp/src/user_verification_elicitation.rs +++ b/codex-rs/codex-mcp/src/user_verification_elicitation.rs @@ -20,10 +20,7 @@ pub(super) async fn route( .is_host_owned_apps(&server_name, server.config()) }) }); - let Some((events, authority)) = events - .zip(authority) - .filter(|_| plugin_service && !router.auto_deny()) - else { + let Some((events, authority)) = events.zip(authority).filter(|_| plugin_service) else { return Ok(ElicitationResponse { action: ElicitationAction::Cancel, content: None, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 26e780e3eb..ab7e698585 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -262,11 +262,7 @@ async fn delegate_start_analytics_honors_child_opt_out_with_enabled_parent() { .services .analytics_events_client = client.clone(); parent_session - .set_app_server_client_info( - Some("codex-test".to_string()), - Some("1.0.0".to_string()), - /*mcp_elicitations_auto_deny*/ false, - ) + .set_app_server_client_info(Some("codex-test".to_string()), Some("1.0.0".to_string())) .await .expect("set parent client metadata"); diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 18719552e2..af15c8d5a0 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -810,14 +810,9 @@ impl CodexThread { &self, app_server_client_name: Option, app_server_client_version: Option, - mcp_elicitations_auto_deny: bool, ) -> ConstraintResult<()> { self.session - .set_app_server_client_info( - app_server_client_name, - app_server_client_version, - mcp_elicitations_auto_deny, - ) + .set_app_server_client_info(app_server_client_name, app_server_client_version) .await } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index a7094d26c3..47f0c82015 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -952,7 +952,7 @@ async fn maybe_request_codex_apps_auth_elicitation( url: plan.elicitation.url, elicitation_id: plan.elicitation.elicitation_id, }; - let outcome = sess + let response = sess .request_mcp_server_elicitation( turn_context, CODEX_APPS_MCP_SERVER_NAME.to_string(), @@ -960,11 +960,7 @@ async fn maybe_request_codex_apps_auth_elicitation( request, ) .await; - if !outcome - .response - .as_ref() - .is_some_and(|response| response.action == ElicitationAction::Accept) - { + if !response.is_some_and(|response| response.action == ElicitationAction::Accept) { return result; } @@ -1866,7 +1862,7 @@ pub(crate) async fn request_mcp_tool_user_approval( .as_ref() .map(|rendered_template| rendered_template.question.as_str()), ); - let (request_dispatched, decision, cancellation_reason) = if tool_call_mcp_elicitation_enabled { + let (decision, cancellation_reason) = if tool_call_mcp_elicitation_enabled { let metadata = sess .mcp_tool_approval_metadata(server, id) .map(|context| context.metadata) @@ -1896,12 +1892,12 @@ pub(crate) async fn request_mcp_tool_user_approval( .map(|rendered_template| rendered_template.elicitation_message.as_str()), prompt_options, }); - let outcome = sess + let response = sess .request_mcp_server_elicitation(turn_context, server.clone(), request_id, request) .await; // Missing replies are interruptions. A received Cancel needs provenance // from its source; RPC success alone does not establish a user action. - let cancellation_reason = match outcome.response.as_ref() { + let cancellation_reason = match response.as_ref() { None => Some(McpApprovalCancellationReason::Interrupted), Some(response) if response.action == ElicitationAction::Cancel => response .meta @@ -1911,8 +1907,7 @@ pub(crate) async fn request_mcp_tool_user_approval( Some(_) => None, }; ( - outcome.sent, - parse_mcp_tool_approval_elicitation_response(outcome.response, &question_id), + parse_mcp_tool_approval_elicitation_response(response, &question_id), cancellation_reason, ) } else { @@ -1951,7 +1946,6 @@ pub(crate) async fn request_mcp_tool_user_approval( .then_some(McpApprovalCancellationReason::Interrupted) }); ( - true, parse_mcp_tool_approval_response( response.map(|accepted| accepted.response), &question_id, @@ -1960,12 +1954,10 @@ pub(crate) async fn request_mcp_tool_user_approval( ) }; let decision = normalize_approval_decision_for_mode(decision, *approval_mode); - if request_dispatched - && matches!( - decision, - ReviewDecision::Denied { .. } | ReviewDecision::TimedOut | ReviewDecision::Abort - ) - { + if matches!( + decision, + ReviewDecision::Denied { .. } | ReviewDecision::TimedOut | ReviewDecision::Abort + ) { track_mcp_tool_call_elicitation(sess, turn_context, call_id, ElicitationType::Approval); } ApprovalResolution { diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index e8cb500650..82aa3b8802 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -48,11 +48,6 @@ struct GuardianMcpElicitationReviewer { session: std::sync::Weak, } -pub(crate) struct McpServerElicitationOutcome { - pub(crate) response: Option, - pub(crate) sent: bool, -} - #[derive(Debug, PartialEq, Eq)] struct PluginInstallElicitationTelemetryMetadata { tool_type: String, @@ -748,18 +743,7 @@ impl Session { server_name: String, request_id: RequestId, request: ElicitationRequest, - ) -> McpServerElicitationOutcome { - if self.services.mcp_runtime.elicitations_auto_deny() { - return McpServerElicitationOutcome { - response: Some(ElicitationResponse { - action: codex_rmcp_client::ElicitationAction::Accept, - content: Some(serde_json::json!({})), - meta: None, - }), - sent: false, - }; - } - + ) -> Option { // This callback carries a request ID, not a reliable tool invocation owner. // Do not report its human wait as ordinary tool execution. if turn_context @@ -818,10 +802,7 @@ impl Session { plugin_install_telemetry.tool_name.as_str(), ); } - McpServerElicitationOutcome { - response: rx_response.await.ok(), - sent: true, - } + rx_response.await.ok() } #[expect( diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 171e0a4e1d..32084e62c7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2382,7 +2382,6 @@ impl Session { &self, app_server_client_name: Option, app_server_client_version: Option, - mcp_elicitations_auto_deny: bool, ) -> ConstraintResult<()> { self.update_settings(SessionSettingsUpdate { app_server_client_name, @@ -2390,9 +2389,6 @@ impl Session { ..Default::default() }) .await?; - self.services - .mcp_runtime - .set_elicitations_auto_deny(mcp_elicitations_auto_deny); Ok(()) } diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 8faddfd4bb..7f96937e30 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -683,50 +683,6 @@ 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( - 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 - .set_elicitations_auto_deny(/*auto_deny*/ true); - - let response = session - .request_mcp_server_elicitation( - turn_context.as_ref(), - "codex_apps".to_string(), - RequestId::String("request-1".into()), - ElicitationRequest::Form { - meta: None, - message: "Allow this request?".to_string(), - requested_schema: json!({ - "type": "object", - "properties": {}, - }), - }, - ) - .await; - - assert_eq!( - response.response, - Some(ElicitationResponse { - action: ElicitationAction::Accept, - content: Some(json!({})), - meta: None, - }) - ); - assert!(!response.sent); - assert!(rx.try_recv().is_err()); -} - #[test_case(SessionSource::Exec, true; "root")] #[test_case(SessionSource::Exec, false; "capture_off")] #[test_case(SessionSource::SubAgent(SubAgentSource::Review), true; "subagent")] @@ -818,12 +774,11 @@ async fn request_mcp_server_elicitation_waits_for_user_response( ) .await .expect("user response should resolve the elicitation"); - let outcome = tokio::time::timeout(Duration::from_secs(1), pending) + let received = 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_eq!(received, Some(response)); assert!(!*paused.borrow()); assert!(rx.try_recv().is_err()); } 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 335584ab4c..a79fbaa983 100644 --- a/codex-rs/core/src/tools/handlers/request_plugin_install.rs +++ b/codex-rs/core/src/tools/handlers/request_plugin_install.rs @@ -251,7 +251,7 @@ impl RequestPluginInstallHandler { let request = build_request_plugin_install_elicitation_request(suggest_reason, &tool, &suggestion_id); let request_id = RequestId::String(suggestion_id.into()); - let elicitation = session + let response = session .request_mcp_server_elicitation( turn.as_ref(), CODEX_APPS_MCP_SERVER_NAME.to_string(), @@ -259,7 +259,6 @@ impl RequestPluginInstallHandler { request, ) .await; - let response = elicitation.response; if let Some(response) = response.as_ref() { maybe_persist_disabled_install_request(&session, &turn, &tool, response).await; } @@ -281,29 +280,27 @@ impl RequestPluginInstallHandler { .await; } - if elicitation.sent { - let tool_type = match tool_type { - DiscoverableToolType::Connector => "connector", - DiscoverableToolType::Plugin => "plugin", - }; - let response_action = match response.as_ref().map(|response| &response.action) { - Some(ElicitationAction::Accept) => "accept", - Some(ElicitationAction::Decline) => "decline", - Some(ElicitationAction::Cancel) => "cancel", - Some(_) => "unknown", - None => "unavailable", - }; - step_context - .session_telemetry - .record_plugin_install_suggestion( - tool_type, - tool.id(), - tool.name(), - response_action, - user_confirmed, - completed, - ); - } + let telemetry_tool_type = match tool_type { + DiscoverableToolType::Connector => "connector", + DiscoverableToolType::Plugin => "plugin", + }; + let response_action = match response.as_ref().map(|response| &response.action) { + Some(ElicitationAction::Accept) => "accept", + Some(ElicitationAction::Decline) => "decline", + Some(ElicitationAction::Cancel) => "cancel", + Some(_) => "unknown", + None => "unavailable", + }; + step_context + .session_telemetry + .record_plugin_install_suggestion( + telemetry_tool_type, + tool.id(), + tool.name(), + response_action, + user_confirmed, + completed, + ); let content = serde_json::to_string(&RequestPluginInstallResult { completed, diff --git a/codex-rs/core/tests/suite/multi_agent_resume.rs b/codex-rs/core/tests/suite/multi_agent_resume.rs index 09c317d8f7..4e074544db 100644 --- a/codex-rs/core/tests/suite/multi_agent_resume.rs +++ b/codex-rs/core/tests/suite/multi_agent_resume.rs @@ -485,11 +485,7 @@ async fn cold_root_resume_restores_agent_identity_and_role_on_followup() -> Resu let resumed = resume_builder.restart(&server, &initial).await?; resumed .codex - .set_app_server_client_info( - Some("resume-test".to_string()), - Some("1.0.0".to_string()), - /*mcp_elicitations_auto_deny*/ false, - ) + .set_app_server_client_info(Some("resume-test".to_string()), Some("1.0.0".to_string())) .await?; drop(initial); assert_eq!(