mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
Allow subagents to request MCP elicitation input (#46877)
## Why MCP requests requiring user input previously failed in subagents with guidance to ask the parent agent, blocking browser sign-in, form input, and interactive tool approval in the child thread. ## What changed Remove the root-only MCP elicitation guard and `allow_user_interaction` plumbing so subagents can surface prompts and wait for responses under the existing approval policy and automatic review rules. Connector authentication can now return retry guidance after acceptance or preserve the original failure when declined. Keep legacy tool approvals through `request_user_input` restricted to the root thread. ## Testing Update unit and integration coverage for subagent prompt resolution, form responses, connector authentication acceptance and decline, automatic approval, strict review, and approval-policy updates on reused connections. Update the browser sign-in scenario snapshot to show the subagent continuing after an accepted response. GitOrigin-RevId: c226fc6dfde724ed22b91dc04dd32bbc15245bee
This commit is contained in:
@@ -222,7 +222,6 @@ impl McpConnectionSet {
|
||||
client_mcp_extensions,
|
||||
auth,
|
||||
auth_manager,
|
||||
allow_user_interaction,
|
||||
elicitation_reviewer,
|
||||
elicitation_lifecycle,
|
||||
} = input;
|
||||
@@ -259,7 +258,6 @@ impl McpConnectionSet {
|
||||
!previous.servers.is_empty()
|
||||
&& previous.elicitation_requests.update(
|
||||
Arc::clone(&config),
|
||||
allow_user_interaction,
|
||||
elicitation_reviewer.clone(),
|
||||
elicitation_lifecycle.clone(),
|
||||
)
|
||||
@@ -269,7 +267,6 @@ impl McpConnectionSet {
|
||||
} else {
|
||||
ElicitationRequestManager::new(
|
||||
Arc::clone(&config),
|
||||
allow_user_interaction,
|
||||
elicitation_reviewer,
|
||||
elicitation_lifecycle,
|
||||
elicitation_router,
|
||||
|
||||
@@ -117,7 +117,6 @@ impl McpConnectionSet {
|
||||
approval_policy.value(),
|
||||
permission_profile.get().clone(),
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -913,7 +912,6 @@ fn elicitation_granular_policy_respects_never_and_config() {
|
||||
async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() {
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -952,7 +950,6 @@ async fn disabled_permissions_auto_accept_elicitation_with_empty_form_schema() {
|
||||
async fn disabled_permissions_do_not_auto_accept_elicitation_with_requested_fields() {
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -1048,7 +1045,6 @@ async fn assert_elicitation_declined_with_reviewer_calls(
|
||||
let reviewer = Arc::new(DecliningElicitationReviewer::default());
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config(server_name, approval_policy, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
Some(reviewer.clone()),
|
||||
/*lifecycle*/ None,
|
||||
full_access_form_input_enabled_router(),
|
||||
@@ -1093,7 +1089,6 @@ async fn assert_requested_user_input_is_declined(
|
||||
) {
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config("server", approval_policy, permission_profile),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
router,
|
||||
@@ -1185,7 +1180,6 @@ async fn assert_disabled_permissions_surface_requested_user_input(
|
||||
let reviewer = Arc::new(DecliningElicitationReviewer::default());
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
Some(reviewer.clone()),
|
||||
/*lifecycle*/ None,
|
||||
router.clone(),
|
||||
@@ -1336,7 +1330,6 @@ async fn on_request_approval_forms_remain_with_the_reviewer() {
|
||||
async fn disabled_permissions_decline_user_input_without_an_event_channel() {
|
||||
let manager = ElicitationRequestManager::new(
|
||||
test_elicitation_config("server", AskForApproval::Never, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
full_access_form_input_enabled_router(),
|
||||
@@ -1376,7 +1369,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() {
|
||||
AskForApproval::Never,
|
||||
PermissionProfile::default(),
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -1390,7 +1382,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() {
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::Disabled
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
));
|
||||
@@ -1400,7 +1391,6 @@ async fn concurrent_authority_updates_never_auto_approve_mixed_policy() {
|
||||
AskForApproval::Never,
|
||||
PermissionProfile::default()
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
));
|
||||
@@ -1462,7 +1452,6 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() {
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::default(),
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
Some(lifecycle.clone()),
|
||||
router.clone(),
|
||||
@@ -1473,7 +1462,6 @@ async fn shared_elicitation_router_targets_the_exact_pending_request() {
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::default(),
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
Some(lifecycle),
|
||||
router.clone(),
|
||||
@@ -1578,7 +1566,6 @@ async fn cancelled_elicitation_is_removed_without_affecting_other_pending_reques
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::default(),
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
router.clone(),
|
||||
@@ -2058,7 +2045,6 @@ async fn read_only_apps_discovery_never_uses_a_shared_writable_catalog() -> anyh
|
||||
client_mcp_extensions: ClientMcpExtensions::default(),
|
||||
auth: None,
|
||||
auth_manager: None,
|
||||
allow_user_interaction: true,
|
||||
elicitation_reviewer: None,
|
||||
elicitation_lifecycle: None,
|
||||
},
|
||||
@@ -2114,7 +2100,6 @@ async fn hosted_apps_protocol_mode_is_independent_of_generic_mode() -> anyhow::R
|
||||
/*previous*/ None,
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(config),
|
||||
plugins_available: false,
|
||||
@@ -2221,7 +2206,6 @@ async fn codex_apps_extension_does_not_share_host_owned_tools_cache() -> anyhow:
|
||||
/*previous*/ None,
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(config),
|
||||
plugins_available: false,
|
||||
@@ -4619,7 +4603,6 @@ async fn executor_owned_chatgpt_mcp_accepts_only_safe_explicit_authorization() -
|
||||
/*previous*/ None,
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(runtime_config.clone()),
|
||||
plugins_available: false,
|
||||
@@ -4734,7 +4717,6 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() {
|
||||
/*previous*/ None,
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(crate::mcp::tests::test_mcp_config(
|
||||
codex_home.path().to_path_buf(),
|
||||
@@ -5158,7 +5140,6 @@ async fn reconcile_reusable_server(
|
||||
config,
|
||||
runtime_context,
|
||||
crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()),
|
||||
/*allow_user_interaction*/ true,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -5169,14 +5150,12 @@ async fn reconcile_reusable_server_with_mcp_config(
|
||||
config: McpServerConfig,
|
||||
runtime_context: McpRuntimeContext,
|
||||
mcp_config: crate::McpConfig,
|
||||
allow_user_interaction: bool,
|
||||
) -> McpConnectionSet {
|
||||
let (tx_event, _rx_event) = async_channel::unbounded();
|
||||
McpConnectionSet::new(
|
||||
Some(previous),
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(mcp_config),
|
||||
plugins_available: false,
|
||||
@@ -5225,7 +5204,6 @@ async fn read_only_policy_does_not_reuse_a_writable_connection() {
|
||||
config.clone(),
|
||||
runtime_context.clone(),
|
||||
mcp_config,
|
||||
/*allow_user_interaction*/ true,
|
||||
)
|
||||
.await;
|
||||
assert_eq!(
|
||||
@@ -5455,7 +5433,6 @@ async fn refreshed_catalog_follows_reused_client_without_mutating_old_bindings()
|
||||
config,
|
||||
runtime_context,
|
||||
crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()),
|
||||
/*allow_user_interaction*/ true,
|
||||
)
|
||||
.await,
|
||||
);
|
||||
@@ -5930,7 +5907,6 @@ async fn reconciliation_replaces_connection_when_protocol_mode_changes() {
|
||||
Some(&previous),
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(mcp_config),
|
||||
plugins_available: false,
|
||||
@@ -5989,7 +5965,6 @@ async fn reconciliation_reuses_legacy_stdio_server_when_modern_protocol_is_enabl
|
||||
Some(&previous),
|
||||
McpPublicationGate::already_published(),
|
||||
McpRuntimeInput {
|
||||
allow_user_interaction: true,
|
||||
startup_policy: McpStartupPolicy::Eager,
|
||||
config: Arc::new(mcp_config),
|
||||
plugins_available: false,
|
||||
@@ -6033,7 +6008,6 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv
|
||||
let router = ElicitationRequestRouter::default();
|
||||
previous.elicitation_requests = ElicitationRequestManager::new(
|
||||
test_elicitation_config("docs", AskForApproval::Never, PermissionProfile::Disabled),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
router.clone(),
|
||||
@@ -6056,19 +6030,19 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv
|
||||
meta: None,
|
||||
};
|
||||
|
||||
for allow_user_interaction in [true, false, true] {
|
||||
let mcp_config = test_elicitation_config(
|
||||
"docs",
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::default(),
|
||||
);
|
||||
for approval_policy in [
|
||||
AskForApproval::OnRequest,
|
||||
AskForApproval::Never,
|
||||
AskForApproval::OnRequest,
|
||||
] {
|
||||
let mcp_config =
|
||||
test_elicitation_config("docs", approval_policy, PermissionProfile::default());
|
||||
let reconciled = reconcile_reusable_server_with_mcp_config(
|
||||
&previous,
|
||||
"docs",
|
||||
config.clone(),
|
||||
runtime_context.clone(),
|
||||
mcp_config.as_ref().clone(),
|
||||
allow_user_interaction,
|
||||
)
|
||||
.await;
|
||||
assert!(previous.shares_test_connection_with(&reconciled, "docs"));
|
||||
@@ -6079,13 +6053,13 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv
|
||||
.lock()
|
||||
.expect("elicitation authority lock");
|
||||
let config = &authority.as_ref().expect("elicitation authority").config;
|
||||
assert_eq!(config.approval_policy.value(), AskForApproval::OnRequest);
|
||||
assert_eq!(config.approval_policy.value(), approval_policy);
|
||||
assert_eq!(config.permission_profile, PermissionProfile::default());
|
||||
}
|
||||
|
||||
// A sender captured before reconciliation must observe each ownership update.
|
||||
// A sender captured before reconciliation must observe each policy update.
|
||||
let mut pending = sender(NumberOrString::Number(7), elicitation.clone());
|
||||
if allow_user_interaction {
|
||||
if approval_policy == AskForApproval::OnRequest {
|
||||
assert!(futures::poll!(pending.as_mut()).is_pending());
|
||||
let EventMsg::ElicitationRequest(request) =
|
||||
events.try_recv().expect("user-input event").msg
|
||||
@@ -6105,11 +6079,17 @@ async fn reconciliation_updates_elicitation_policy_without_restarting_ready_serv
|
||||
.expect("user response should resolve the retained sender");
|
||||
assert_eq!(pending.await.expect("elicitation should resolve"), response);
|
||||
} else {
|
||||
let error = pending
|
||||
.now_or_never()
|
||||
.expect("blocked elicitation must not wait for user input")
|
||||
.expect_err("subagent user input must be rejected");
|
||||
assert_eq!(error.to_string(), crate::MCP_ELICITATION_HANDOFF_MESSAGE);
|
||||
assert_eq!(
|
||||
pending
|
||||
.now_or_never()
|
||||
.expect("a policy denial must not wait for user input")
|
||||
.expect("elicitation should receive a response"),
|
||||
ElicitationResponse {
|
||||
action: ElicitationAction::Decline,
|
||||
content: None,
|
||||
meta: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
assert!(events.is_empty());
|
||||
previous = reconciled;
|
||||
@@ -6299,7 +6279,6 @@ async fn reconciliation_reconnects_when_host_plugin_root_changes() {
|
||||
server_config.clone(),
|
||||
runtime_context.clone(),
|
||||
config_for_root(original_root),
|
||||
/*allow_user_interaction*/ true,
|
||||
)
|
||||
.await;
|
||||
assert!(previous.shares_test_connection_with(&unchanged, "docs"));
|
||||
@@ -6311,7 +6290,6 @@ async fn reconciliation_reconnects_when_host_plugin_root_changes() {
|
||||
server_config,
|
||||
runtime_context,
|
||||
replacement_config,
|
||||
/*allow_user_interaction*/ true,
|
||||
)
|
||||
.await;
|
||||
assert!(!unchanged.shares_test_connection_with(&replacement, "docs"));
|
||||
|
||||
@@ -4,8 +4,7 @@
|
||||
//! from the user. It decides whether the request can be automatically accepted,
|
||||
//! must be declined by policy, or should be surfaced as a Codex protocol event
|
||||
//! and later resolved through the stored responder.
|
||||
//! Non-root agents may use automatic decisions, but explicit user-input requests
|
||||
//! are rejected before review and no pending user prompt is registered.
|
||||
//! Explicit MCP elicitation requests can be surfaced by root and non-root agents.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
@@ -26,11 +25,9 @@ use codex_protocol::approvals::ElicitationRequestEvent;
|
||||
use codex_protocol::mcp::ClientMcpExtensions;
|
||||
use codex_protocol::mcp::OPENAI_ELICITATION_EXTENSION_ID;
|
||||
use codex_protocol::mcp::RequestId as ProtocolRequestId;
|
||||
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_BROWSER_AUTH;
|
||||
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY;
|
||||
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_TOOL_SUGGESTION;
|
||||
use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY;
|
||||
use codex_protocol::mcp_approval_meta::REQUIRES_USER_INPUT_KEY;
|
||||
use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::Event;
|
||||
@@ -48,12 +45,6 @@ use tokio::sync::oneshot;
|
||||
static NEXT_ELICITATION_REQUEST_ID: AtomicU64 = AtomicU64::new(0);
|
||||
|
||||
const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval.";
|
||||
/// Recovery guidance for a request that a non-root agent cannot present to the user.
|
||||
pub const MCP_ELICITATION_HANDOFF_MESSAGE: &str = concat!(
|
||||
"MCP server elicitations can only be requested by the root thread. ",
|
||||
"Ask the parent agent to handle this request. ",
|
||||
"Do not retry the blocked action until the parent confirms the blocker is resolved."
|
||||
);
|
||||
|
||||
#[path = "user_verification_elicitation.rs"]
|
||||
mod user_verification_elicitation;
|
||||
@@ -172,10 +163,6 @@ impl ElicitationRequestRouter {
|
||||
server_name: String,
|
||||
request: ElicitationRequest,
|
||||
) -> Result<ElicitationResponse> {
|
||||
anyhow::ensure!(
|
||||
authority.allow_user_interaction,
|
||||
MCP_ELICITATION_HANDOFF_MESSAGE
|
||||
);
|
||||
let Some(events) = events else {
|
||||
return Ok(ElicitationResponse {
|
||||
action: ElicitationAction::Decline,
|
||||
@@ -235,7 +222,6 @@ impl ElicitationRequestRouter {
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct ElicitationAuthority {
|
||||
pub(crate) config: Arc<McpConfig>,
|
||||
pub(crate) allow_user_interaction: bool,
|
||||
reviewer: Option<ElicitationReviewerHandle>,
|
||||
lifecycle: Option<ElicitationLifecycle>,
|
||||
}
|
||||
@@ -249,7 +235,6 @@ pub(crate) struct ElicitationRequestManager {
|
||||
impl ElicitationRequestManager {
|
||||
pub(crate) fn new(
|
||||
config: Arc<McpConfig>,
|
||||
allow_user_interaction: bool,
|
||||
reviewer: Option<ElicitationReviewerHandle>,
|
||||
lifecycle: Option<ElicitationLifecycle>,
|
||||
router: ElicitationRequestRouter,
|
||||
@@ -258,7 +243,6 @@ impl ElicitationRequestManager {
|
||||
router,
|
||||
authority: Arc::new(StdMutex::new(Some(ElicitationAuthority {
|
||||
config,
|
||||
allow_user_interaction,
|
||||
reviewer,
|
||||
lifecycle,
|
||||
}))),
|
||||
@@ -268,7 +252,6 @@ impl ElicitationRequestManager {
|
||||
pub(crate) fn update(
|
||||
&self,
|
||||
config: Arc<McpConfig>,
|
||||
allow_user_interaction: bool,
|
||||
reviewer: Option<ElicitationReviewerHandle>,
|
||||
lifecycle: Option<ElicitationLifecycle>,
|
||||
) -> bool {
|
||||
@@ -277,7 +260,6 @@ impl ElicitationRequestManager {
|
||||
};
|
||||
*authority = Some(ElicitationAuthority {
|
||||
config,
|
||||
allow_user_interaction,
|
||||
reviewer,
|
||||
lifecycle,
|
||||
});
|
||||
@@ -308,36 +290,6 @@ impl ElicitationRequestManager {
|
||||
.lock()
|
||||
.ok()
|
||||
.and_then(|authority| authority.clone());
|
||||
let user_interaction_disabled = authority
|
||||
.as_ref()
|
||||
.is_some_and(|authority| !authority.allow_user_interaction);
|
||||
if user_interaction_disabled {
|
||||
// Browser sign-in can use an empty schema. Explicit user-input
|
||||
// markers must take precedence over automatic approval or review.
|
||||
let requires_user_input = elicitation.meta().is_some_and(|meta| {
|
||||
meta.get(REQUIRES_USER_INPUT_KEY) == Some(&Value::Bool(true))
|
||||
|| meta.get(APPROVAL_KIND_KEY).and_then(Value::as_str)
|
||||
== Some(APPROVAL_KIND_BROWSER_AUTH)
|
||||
}) || match &elicitation {
|
||||
Elicitation::Mcp(
|
||||
rmcp::model::ElicitRequestParams::FormElicitationParams {
|
||||
requested_schema,
|
||||
..
|
||||
},
|
||||
) => !requested_schema.properties.is_empty(),
|
||||
Elicitation::OpenAiForm {
|
||||
requested_schema, ..
|
||||
}
|
||||
| Elicitation::OpenAiElicitationForm {
|
||||
requested_schema, ..
|
||||
} => requested_schema
|
||||
.get("properties")
|
||||
.and_then(Value::as_object)
|
||||
.is_some_and(|properties| !properties.is_empty()),
|
||||
Elicitation::Mcp(_) | Elicitation::UserVerification { .. } => true,
|
||||
};
|
||||
anyhow::ensure!(!requires_user_input, MCP_ELICITATION_HANDOFF_MESSAGE);
|
||||
}
|
||||
if let Elicitation::UserVerification {
|
||||
title,
|
||||
description,
|
||||
|
||||
@@ -93,7 +93,6 @@ fn elicitation_fixture(
|
||||
.insert("another-independent-mcp".to_string(), permission_profile);
|
||||
let manager = ElicitationRequestManager::new(
|
||||
config,
|
||||
/*allow_user_interaction*/ true,
|
||||
reviewer.map(|reviewer| reviewer as Arc<dyn ElicitationReviewer>),
|
||||
lifecycle,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -120,16 +119,6 @@ async fn send_elicitation(sender: &SendElicitation, marker: Option<Value>) -> 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<Value>, properties: Value) -> [Elicitation; 3] {
|
||||
let schema = json!({"type": "object", "properties": properties});
|
||||
[
|
||||
@@ -153,85 +142,8 @@ fn form_elicitations(meta: Option<Value>, properties: Value) -> [Elicitation; 3]
|
||||
]
|
||||
}
|
||||
|
||||
async fn assert_subagent_elicitation_blocked(sender: &SendElicitation, elicitation: Elicitation) {
|
||||
let error = tokio::time::timeout(
|
||||
std::time::Duration::from_secs(1),
|
||||
sender(RequestId::Number(7), elicitation),
|
||||
)
|
||||
.await
|
||||
.expect("blocked elicitation must not wait for user input")
|
||||
.unwrap_err();
|
||||
assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn subagent_human_input_is_rejected_before_automatic_approval() {
|
||||
let mut requests = Vec::new();
|
||||
for meta in [
|
||||
json!({"codex_approval_kind": "browser_auth"}),
|
||||
json!({"codex_approval_kind": "browser_auth", "codex_requires_user_input": true}),
|
||||
json!({"codex_requires_user_input": true}),
|
||||
json!({
|
||||
"codex_approval_kind": "mcp_tool_call",
|
||||
"codex_request_type": "approval_request",
|
||||
"codex_strict_auto_review": true,
|
||||
"codex_requires_user_input": true,
|
||||
}),
|
||||
] {
|
||||
requests.extend(form_elicitations(Some(meta), json!({})));
|
||||
}
|
||||
for meta in [None, Some(json!({"codex_strict_auto_review": true}))] {
|
||||
requests.extend(form_elicitations(
|
||||
meta.clone(),
|
||||
json!({"answer": {"type": "string"}}),
|
||||
));
|
||||
requests.push(Elicitation::Mcp(
|
||||
ElicitRequestParams::UrlElicitationParams {
|
||||
meta: meta.map(|meta| RequestMetaObject::from(meta.as_object().unwrap().clone())),
|
||||
message: "Sign in to continue".into(),
|
||||
url: "https://example.com/auth".into(),
|
||||
elicitation_id: "test-auth".into(),
|
||||
},
|
||||
));
|
||||
}
|
||||
|
||||
for approval_policy in [AskForApproval::OnRequest, AskForApproval::Never] {
|
||||
let reviewer = RecordingReviewer::new(Ok(Some(approved_response())));
|
||||
let (manager, events, sender) = elicitation_fixture(
|
||||
approval_policy,
|
||||
PermissionProfile::Disabled,
|
||||
Some(reviewer.clone()),
|
||||
);
|
||||
// Update after creating the sender so a retained connection observes the restriction.
|
||||
disable_user_interaction(&manager);
|
||||
for elicitation in requests.iter().cloned() {
|
||||
assert_subagent_elicitation_blocked(&sender, elicitation).await;
|
||||
}
|
||||
assert!(events.is_empty());
|
||||
assert!(manager.router.requests.lock().unwrap().is_empty());
|
||||
assert_eq!(reviewer.calls.load(Relaxed), 0);
|
||||
assert_eq!(reviewer.active_elicitations.load(Relaxed), 0);
|
||||
|
||||
let (manager, events, sender) =
|
||||
verification_fixture(approval_policy, Some(reviewer.clone()));
|
||||
disable_user_interaction(&manager);
|
||||
assert_subagent_elicitation_blocked(
|
||||
&sender,
|
||||
Elicitation::UserVerification {
|
||||
title: "Approve purchase".into(),
|
||||
description: "Pay $200".into(),
|
||||
challenge: "AQID".into(),
|
||||
},
|
||||
)
|
||||
.await;
|
||||
assert!(events.is_empty());
|
||||
assert!(manager.router.requests.lock().unwrap().is_empty());
|
||||
assert_eq!(reviewer.calls.load(Relaxed), 0);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn subagent_permission_preserves_automatic_review_decisions() {
|
||||
async fn permission_elicitation_preserves_automatic_review_decisions() {
|
||||
for (strict_auto_review, approval_policy, expected_reviews) in [
|
||||
(false, AskForApproval::OnRequest, 1),
|
||||
(true, AskForApproval::OnRequest, 1),
|
||||
@@ -252,7 +164,6 @@ async fn subagent_permission_preserves_automatic_review_decisions() {
|
||||
PermissionProfile::Disabled,
|
||||
Some(reviewer.clone()),
|
||||
);
|
||||
disable_user_interaction(&manager);
|
||||
let [request, _, _] = form_elicitations(
|
||||
Some(json!({
|
||||
"codex_request_type": "approval_request",
|
||||
@@ -284,23 +195,62 @@ async fn subagent_permission_preserves_automatic_review_decisions() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn subagent_without_an_automatic_decision_does_not_prompt() {
|
||||
async fn elicitation_without_an_automatic_decision_prompts() {
|
||||
for reviewer in [None, Some(RecordingReviewer::new(Ok(None)))] {
|
||||
let (manager, events, sender) = elicitation_fixture(
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::read_only(),
|
||||
reviewer.clone(),
|
||||
);
|
||||
disable_user_interaction(&manager);
|
||||
for elicitation in form_elicitations(/*meta*/ None, json!({})) {
|
||||
assert_subagent_elicitation_blocked(&sender, elicitation).await;
|
||||
let mut requests = Vec::from(form_elicitations(/*meta*/ None, json!({})));
|
||||
requests.extend(form_elicitations(
|
||||
Some(json!({"codex_requires_user_input": true})),
|
||||
json!({"answer": {"type": "string"}}),
|
||||
));
|
||||
requests.push(Elicitation::Mcp(
|
||||
ElicitRequestParams::UrlElicitationParams {
|
||||
meta: None,
|
||||
message: "Sign in to continue".into(),
|
||||
url: "https://example.com/auth".into(),
|
||||
elicitation_id: "test-auth".into(),
|
||||
},
|
||||
));
|
||||
let expected_reviews = requests.len() + 1;
|
||||
for elicitation in requests {
|
||||
let pending = tokio::spawn(sender(RequestId::Number(7), elicitation));
|
||||
let event = tokio::time::timeout(std::time::Duration::from_secs(1), events.recv())
|
||||
.await
|
||||
.expect("elicitation should prompt")
|
||||
.expect("event channel should remain open");
|
||||
let EventMsg::ElicitationRequest(request) = event.msg else {
|
||||
panic!("expected MCP elicitation");
|
||||
};
|
||||
assert!(!pending.is_finished());
|
||||
let ProtocolRequestId::String(request_id) = request.id else {
|
||||
panic!("expected Codex-owned string request ID");
|
||||
};
|
||||
let response = ElicitationResponse {
|
||||
action: ElicitationAction::Accept,
|
||||
content: Some(json!({"answer": "continue"})),
|
||||
meta: None,
|
||||
};
|
||||
manager
|
||||
.router
|
||||
.resolve(
|
||||
request.server_name,
|
||||
RequestId::String(request_id.into()),
|
||||
response.clone(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(pending.await.unwrap().unwrap(), response);
|
||||
}
|
||||
assert_eq!(
|
||||
send_elicitation(&sender, Some(json!(true))).await,
|
||||
strict_auto_review_decline()
|
||||
);
|
||||
if let Some(reviewer) = reviewer {
|
||||
assert_eq!(reviewer.calls.load(Relaxed), 4);
|
||||
assert_eq!(reviewer.calls.load(Relaxed), expected_reviews);
|
||||
assert_eq!(reviewer.active_elicitations.load(Relaxed), 0);
|
||||
}
|
||||
assert!(events.is_empty());
|
||||
@@ -308,55 +258,6 @@ async fn subagent_without_an_automatic_decision_does_not_prompt() {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn final_prompt_guard_rejects_subagents_before_registration() {
|
||||
let (manager, _, _) = verification_fixture(AskForApproval::OnRequest, /*reviewer*/ None);
|
||||
let mut authority = manager.authority.lock().unwrap().clone().unwrap();
|
||||
authority.allow_user_interaction = false;
|
||||
let registrations = Arc::new(AtomicUsize::new(0));
|
||||
let starts = registrations.clone();
|
||||
authority.lifecycle = Some(ElicitationLifecycle::new(move || {
|
||||
starts.fetch_add(/*val*/ 1, Relaxed);
|
||||
}));
|
||||
let (tx, events) = async_channel::bounded(1);
|
||||
for sender in [Some(tx.clone()), None] {
|
||||
let error = manager
|
||||
.router
|
||||
.request_user_interaction(
|
||||
sender,
|
||||
&authority,
|
||||
crate::CODEX_APPS_MCP_SERVER_NAME.into(),
|
||||
ElicitationRequest::Form {
|
||||
meta: None,
|
||||
message: "Confirm the action".into(),
|
||||
requested_schema: json!({"type": "object", "properties": {}}),
|
||||
},
|
||||
)
|
||||
.now_or_never()
|
||||
.expect("a subagent must not wait for user input")
|
||||
.unwrap_err();
|
||||
assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE);
|
||||
}
|
||||
let error = user_verification_elicitation::route(
|
||||
manager.router.clone(),
|
||||
Some(tx),
|
||||
Some(authority),
|
||||
crate::CODEX_APPS_MCP_SERVER_NAME.into(),
|
||||
ElicitationRequest::UserVerification {
|
||||
title: "Approve purchase".into(),
|
||||
description: "Pay $200".into(),
|
||||
challenge: "AQID".into(),
|
||||
},
|
||||
)
|
||||
.now_or_never()
|
||||
.expect("direct verification routing must not wait for a subagent")
|
||||
.unwrap_err();
|
||||
assert_eq!(error.to_string(), MCP_ELICITATION_HANDOFF_MESSAGE);
|
||||
assert_eq!(registrations.load(Relaxed), 0);
|
||||
assert!(events.is_empty());
|
||||
assert!(manager.router.requests.lock().unwrap().is_empty());
|
||||
}
|
||||
|
||||
async fn assert_declined(marker: Value, response: Option<ReviewerResponse>) {
|
||||
let expected_calls = usize::from(marker == Value::Bool(true));
|
||||
let reviewer = response.map(RecordingReviewer::new);
|
||||
@@ -394,7 +295,6 @@ fn closed_event_channel_immediately_cleans_up_pending_elicitation() {
|
||||
AskForApproval::OnRequest,
|
||||
PermissionProfile::Disabled
|
||||
),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
Some(lifecycle),
|
||||
));
|
||||
@@ -583,7 +483,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author
|
||||
|
||||
let manager = ElicitationRequestManager::new(
|
||||
Arc::new(config.clone()),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
@@ -614,7 +513,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author
|
||||
);
|
||||
assert!(manager.update(
|
||||
Arc::new(config.clone()),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
));
|
||||
@@ -638,7 +536,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author
|
||||
);
|
||||
assert!(manager.update(
|
||||
Arc::new(config.clone()),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
));
|
||||
@@ -651,7 +548,6 @@ async fn reused_elicitation_senders_follow_each_servers_latest_permission_author
|
||||
config.set_server_permission_profiles(&servers, std::iter::empty());
|
||||
assert!(manager.update(
|
||||
Arc::new(config.clone()),
|
||||
/*allow_user_interaction*/ true,
|
||||
/*reviewer*/ None,
|
||||
/*lifecycle*/ None,
|
||||
));
|
||||
@@ -683,7 +579,6 @@ fn verification_fixture(
|
||||
Arc::make_mut(&mut config).mcp_server_catalog = catalog.build();
|
||||
let manager = ElicitationRequestManager::new(
|
||||
config,
|
||||
/*allow_user_interaction*/ true,
|
||||
reviewer.map(|reviewer| reviewer as Arc<dyn ElicitationReviewer>),
|
||||
/*lifecycle*/ None,
|
||||
ElicitationRequestRouter::default(),
|
||||
|
||||
@@ -8,7 +8,6 @@ pub use elicitation::ElicitationLifecycle;
|
||||
pub use elicitation::ElicitationReviewRequest;
|
||||
pub use elicitation::ElicitationReviewer;
|
||||
pub use elicitation::ElicitationReviewerHandle;
|
||||
pub use elicitation::MCP_ELICITATION_HANDOFF_MESSAGE;
|
||||
pub use event_stream::McpEventStreamOpener;
|
||||
pub use resource_client::CodexAppsResourceListParams;
|
||||
pub use resource_client::McpEventCatalogSnapshot;
|
||||
|
||||
@@ -458,7 +458,6 @@ pub async fn read_mcp_resource(
|
||||
client_mcp_extensions: ClientMcpExtensions::default(),
|
||||
auth: auth.cloned(),
|
||||
auth_manager: None,
|
||||
allow_user_interaction: true,
|
||||
elicitation_reviewer: None,
|
||||
elicitation_lifecycle: None,
|
||||
},
|
||||
@@ -538,7 +537,6 @@ pub async fn collect_mcp_server_status_snapshot_with_detail(
|
||||
client_mcp_extensions: ClientMcpExtensions::default(),
|
||||
auth: auth.cloned(),
|
||||
auth_manager: None,
|
||||
allow_user_interaction: true,
|
||||
elicitation_reviewer: None,
|
||||
elicitation_lifecycle: None,
|
||||
},
|
||||
|
||||
@@ -89,8 +89,6 @@ pub struct McpRuntimeInput {
|
||||
pub client_mcp_extensions: ClientMcpExtensions,
|
||||
pub auth: Option<CodexAuth>,
|
||||
pub auth_manager: Option<Arc<AuthManager>>,
|
||||
/// Whether the owning thread may prompt the user; automatic decisions remain available.
|
||||
pub allow_user_interaction: bool,
|
||||
pub elicitation_reviewer: Option<ElicitationReviewerHandle>,
|
||||
pub elicitation_lifecycle: Option<ElicitationLifecycle>,
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user