mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
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
This commit is contained in:
@@ -1129,16 +1129,8 @@ impl ThreadRequestProcessor {
|
||||
app_server_client_name: Option<String>,
|
||||
app_server_client_version: Option<String>,
|
||||
) -> 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;
|
||||
|
||||
@@ -1063,16 +1063,8 @@ impl TurnRequestProcessor {
|
||||
app_server_client_name: Option<String>,
|
||||
app_server_client_version: Option<String>,
|
||||
) -> 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"))
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -102,7 +102,6 @@ struct ActiveElicitation {
|
||||
pub(crate) struct ElicitationRequestRouter {
|
||||
requests: Arc<StdMutex<PendingResponders>>,
|
||||
terminal_actions_finished: Arc<tokio::sync::Notify>,
|
||||
auto_deny: Arc<AtomicBool>,
|
||||
full_access_form_input_enabled: Arc<AtomicBool>,
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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");
|
||||
|
||||
|
||||
@@ -810,14 +810,9 @@ impl CodexThread {
|
||||
&self,
|
||||
app_server_client_name: Option<String>,
|
||||
app_server_client_version: Option<String>,
|
||||
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
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -48,11 +48,6 @@ struct GuardianMcpElicitationReviewer {
|
||||
session: std::sync::Weak<Session>,
|
||||
}
|
||||
|
||||
pub(crate) struct McpServerElicitationOutcome {
|
||||
pub(crate) response: Option<ElicitationResponse>,
|
||||
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<ElicitationResponse> {
|
||||
// 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(
|
||||
|
||||
@@ -2382,7 +2382,6 @@ impl Session {
|
||||
&self,
|
||||
app_server_client_name: Option<String>,
|
||||
app_server_client_version: Option<String>,
|
||||
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(())
|
||||
}
|
||||
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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!(
|
||||
|
||||
Reference in New Issue
Block a user