mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Auto-approve MCP server elicitations in Full Access mode (#17164)
Currently, when a MCP server sends an elicitation to Codex running in Full Access (`sandbox_policy: DangerFullAccess` + `approval_policy: Never`), the elicitations are auto-cancelled. This PR updates the automatic handling of MCP elicitations to be consistent with other approvals in full-access, where they are auto-approved. Because MCP elicitations may actually require user input, this mechanism is limited to empty form elicitations. ## Changeset - Add policy helper shared with existing MCP tool call approval auto-approve - Update `ElicitationRequestManager` to auto-approve elicitations in full access when `can_auto_accept_elicitation` is true. - Add tests Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
dcbc91fd39
commit
01537f0bd2
@@ -22,6 +22,7 @@ pub use mcp::configured_mcp_servers;
|
||||
pub use mcp::discover_supported_scopes;
|
||||
pub use mcp::effective_mcp_servers;
|
||||
pub use mcp::group_tools_by_server;
|
||||
pub use mcp::mcp_permission_prompt_is_auto_approved;
|
||||
pub use mcp::oauth_login_support;
|
||||
pub use mcp::qualified_mcp_tool_name_prefix;
|
||||
pub use mcp::resolve_oauth_scopes;
|
||||
|
||||
@@ -82,6 +82,19 @@ pub fn qualified_mcp_tool_name_prefix(server_name: &str) -> String {
|
||||
))
|
||||
}
|
||||
|
||||
/// Returns true when MCP permission prompts should resolve as approved instead
|
||||
/// of being shown to the user.
|
||||
pub fn mcp_permission_prompt_is_auto_approved(
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
) -> bool {
|
||||
approval_policy == AskForApproval::Never
|
||||
&& matches!(
|
||||
sandbox_policy,
|
||||
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
|
||||
)
|
||||
}
|
||||
|
||||
/// MCP runtime settings derived from `codex_core::config::Config`.
|
||||
///
|
||||
/// This struct should contain only long-lived configuration values that the
|
||||
|
||||
@@ -25,6 +25,7 @@ use crate::mcp::McpConfig;
|
||||
use crate::mcp::ToolPluginProvenance;
|
||||
use crate::mcp::configured_mcp_servers;
|
||||
use crate::mcp::effective_mcp_servers;
|
||||
use crate::mcp::mcp_permission_prompt_is_auto_approved;
|
||||
use crate::mcp::sanitize_responses_api_tool_name;
|
||||
use crate::mcp::tool_plugin_provenance;
|
||||
use anyhow::Context;
|
||||
@@ -243,17 +244,31 @@ fn elicitation_is_rejected_by_policy(approval_policy: AskForApproval) -> bool {
|
||||
}
|
||||
}
|
||||
|
||||
fn can_auto_accept_elicitation(elicitation: &CreateElicitationRequestParams) -> bool {
|
||||
match elicitation {
|
||||
CreateElicitationRequestParams::FormElicitationParams {
|
||||
requested_schema, ..
|
||||
} => {
|
||||
// Auto-accept confirm/approval elicitations without schema requirements.
|
||||
requested_schema.properties.is_empty()
|
||||
}
|
||||
CreateElicitationRequestParams::UrlElicitationParams { .. } => false,
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct ElicitationRequestManager {
|
||||
requests: Arc<Mutex<ResponderMap>>,
|
||||
approval_policy: Arc<StdMutex<AskForApproval>>,
|
||||
sandbox_policy: Arc<StdMutex<SandboxPolicy>>,
|
||||
}
|
||||
|
||||
impl ElicitationRequestManager {
|
||||
fn new(approval_policy: AskForApproval) -> Self {
|
||||
fn new(approval_policy: AskForApproval, sandbox_policy: SandboxPolicy) -> Self {
|
||||
Self {
|
||||
requests: Arc::new(Mutex::new(HashMap::new())),
|
||||
approval_policy: Arc::new(StdMutex::new(approval_policy)),
|
||||
sandbox_policy: Arc::new(StdMutex::new(sandbox_policy)),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -275,16 +290,33 @@ impl ElicitationRequestManager {
|
||||
fn make_sender(&self, server_name: String, tx_event: Sender<Event>) -> SendElicitation {
|
||||
let elicitation_requests = self.requests.clone();
|
||||
let approval_policy = self.approval_policy.clone();
|
||||
let sandbox_policy = self.sandbox_policy.clone();
|
||||
Box::new(move |id, elicitation| {
|
||||
let elicitation_requests = elicitation_requests.clone();
|
||||
let tx_event = tx_event.clone();
|
||||
let server_name = server_name.clone();
|
||||
let approval_policy = approval_policy.clone();
|
||||
let sandbox_policy = sandbox_policy.clone();
|
||||
async move {
|
||||
if approval_policy
|
||||
let approval_policy = approval_policy
|
||||
.lock()
|
||||
.is_ok_and(|policy| elicitation_is_rejected_by_policy(*policy))
|
||||
.map(|policy| *policy)
|
||||
.unwrap_or(AskForApproval::Never);
|
||||
let sandbox_policy = sandbox_policy
|
||||
.lock()
|
||||
.map(|policy| policy.clone())
|
||||
.unwrap_or_else(|_| SandboxPolicy::new_read_only_policy());
|
||||
if mcp_permission_prompt_is_auto_approved(approval_policy, &sandbox_policy)
|
||||
&& can_auto_accept_elicitation(&elicitation)
|
||||
{
|
||||
return Ok(ElicitationResponse {
|
||||
action: ElicitationAction::Accept,
|
||||
content: Some(serde_json::json!({})),
|
||||
meta: None,
|
||||
});
|
||||
}
|
||||
|
||||
if elicitation_is_rejected_by_policy(approval_policy) {
|
||||
return Ok(ElicitationResponse {
|
||||
action: ElicitationAction::Decline,
|
||||
content: None,
|
||||
@@ -604,11 +636,17 @@ impl McpConnectionManager {
|
||||
tool_plugin_provenance(config)
|
||||
}
|
||||
|
||||
pub fn new_uninitialized(approval_policy: &Constrained<AskForApproval>) -> Self {
|
||||
pub fn new_uninitialized(
|
||||
approval_policy: &Constrained<AskForApproval>,
|
||||
sandbox_policy: &Constrained<SandboxPolicy>,
|
||||
) -> Self {
|
||||
Self {
|
||||
clients: HashMap::new(),
|
||||
server_origins: HashMap::new(),
|
||||
elicitation_requests: ElicitationRequestManager::new(approval_policy.value()),
|
||||
elicitation_requests: ElicitationRequestManager::new(
|
||||
approval_policy.value(),
|
||||
sandbox_policy.get().clone(),
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -626,6 +664,12 @@ impl McpConnectionManager {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn set_sandbox_policy(&self, sandbox_policy: &SandboxPolicy) {
|
||||
if let Ok(mut policy) = self.elicitation_requests.sandbox_policy.lock() {
|
||||
*policy = sandbox_policy.clone();
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::new_ret_no_self, clippy::too_many_arguments)]
|
||||
pub async fn new(
|
||||
mcp_servers: &HashMap<String, McpServerConfig>,
|
||||
@@ -643,7 +687,10 @@ impl McpConnectionManager {
|
||||
let mut clients = HashMap::new();
|
||||
let mut server_origins = HashMap::new();
|
||||
let mut join_set = JoinSet::new();
|
||||
let elicitation_requests = ElicitationRequestManager::new(approval_policy.value());
|
||||
let elicitation_requests = ElicitationRequestManager::new(
|
||||
approval_policy.value(),
|
||||
initial_sandbox_state.sandbox_policy.clone(),
|
||||
);
|
||||
let tool_plugin_provenance = Arc::new(tool_plugin_provenance);
|
||||
let startup_submit_id = submit_id.clone();
|
||||
let mcp_servers = mcp_servers.clone();
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
use super::*;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::McpAuthStatus;
|
||||
use pretty_assertions::assert_eq;
|
||||
use rmcp::model::JsonObject;
|
||||
use rmcp::model::NumberOrString;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
use tempfile::tempdir;
|
||||
@@ -97,6 +99,70 @@ fn elicitation_granular_policy_respects_never_and_config() {
|
||||
)));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn full_access_auto_accepts_elicitation_with_empty_form_schema() {
|
||||
let manager =
|
||||
ElicitationRequestManager::new(AskForApproval::Never, SandboxPolicy::DangerFullAccess);
|
||||
let (tx_event, _rx_event) = async_channel::bounded(1);
|
||||
let sender = manager.make_sender("server".to_string(), tx_event);
|
||||
|
||||
let response = sender(
|
||||
NumberOrString::Number(1),
|
||||
CreateElicitationRequestParams::FormElicitationParams {
|
||||
meta: None,
|
||||
message: "Confirm?".to_string(),
|
||||
requested_schema: rmcp::model::ElicitationSchema::builder()
|
||||
.build()
|
||||
.expect("schema should build"),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect("elicitation should auto accept");
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
ElicitationResponse {
|
||||
action: ElicitationAction::Accept,
|
||||
content: Some(serde_json::json!({})),
|
||||
meta: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn full_access_does_not_auto_accept_elicitation_with_requested_fields() {
|
||||
let manager =
|
||||
ElicitationRequestManager::new(AskForApproval::Never, SandboxPolicy::DangerFullAccess);
|
||||
let (tx_event, _rx_event) = async_channel::bounded(1);
|
||||
let sender = manager.make_sender("server".to_string(), tx_event);
|
||||
|
||||
let response = sender(
|
||||
NumberOrString::Number(1),
|
||||
CreateElicitationRequestParams::FormElicitationParams {
|
||||
meta: None,
|
||||
message: "What should I say?".to_string(),
|
||||
requested_schema: rmcp::model::ElicitationSchema::builder()
|
||||
.required_property(
|
||||
"message",
|
||||
rmcp::model::PrimitiveSchema::String(rmcp::model::StringSchema::new()),
|
||||
)
|
||||
.build()
|
||||
.expect("schema should build"),
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect("elicitation should auto decline");
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
ElicitationResponse {
|
||||
action: ElicitationAction::Decline,
|
||||
content: None,
|
||||
meta: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_qualify_tools_short_non_duplicated_names() {
|
||||
let tools = vec![
|
||||
@@ -409,7 +475,8 @@ async fn list_all_tools_uses_startup_snapshot_while_client_is_pending() {
|
||||
.boxed()
|
||||
.shared();
|
||||
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy);
|
||||
let sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy());
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy, &sandbox_policy);
|
||||
manager.clients.insert(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
AsyncManagedClient {
|
||||
@@ -434,7 +501,8 @@ async fn list_all_tools_blocks_while_client_is_pending_without_startup_snapshot(
|
||||
.boxed()
|
||||
.shared();
|
||||
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy);
|
||||
let sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy());
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy, &sandbox_policy);
|
||||
manager.clients.insert(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
AsyncManagedClient {
|
||||
@@ -456,7 +524,8 @@ async fn list_all_tools_does_not_block_when_startup_snapshot_cache_hit_is_empty(
|
||||
.boxed()
|
||||
.shared();
|
||||
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy);
|
||||
let sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy());
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy, &sandbox_policy);
|
||||
manager.clients.insert(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
AsyncManagedClient {
|
||||
@@ -487,7 +556,8 @@ async fn list_all_tools_uses_startup_snapshot_when_client_startup_fails() {
|
||||
.boxed()
|
||||
.shared();
|
||||
let approval_policy = Constrained::allow_any(AskForApproval::OnFailure);
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy);
|
||||
let sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy());
|
||||
let mut manager = McpConnectionManager::new_uninitialized(&approval_policy, &sandbox_policy);
|
||||
let startup_complete = Arc::new(std::sync::atomic::AtomicBool::new(true));
|
||||
manager.clients.insert(
|
||||
CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
|
||||
@@ -1982,6 +1982,7 @@ impl Session {
|
||||
// setup is straightforward enough and performs well.
|
||||
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::new_uninitialized(
|
||||
&config.permissions.approval_policy,
|
||||
&config.permissions.sandbox_policy,
|
||||
))),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
@@ -2560,11 +2561,12 @@ impl Session {
|
||||
sandbox_policy_changed: bool,
|
||||
) -> Arc<TurnContext> {
|
||||
let per_turn_config = Self::build_per_turn_config(&session_configuration);
|
||||
self.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.set_approval_policy(&session_configuration.approval_policy);
|
||||
{
|
||||
let mcp_connection_manager = self.services.mcp_connection_manager.read().await;
|
||||
mcp_connection_manager.set_approval_policy(&session_configuration.approval_policy);
|
||||
mcp_connection_manager
|
||||
.set_sandbox_policy(per_turn_config.permissions.sandbox_policy.get());
|
||||
}
|
||||
|
||||
if sandbox_policy_changed {
|
||||
self.refresh_managed_network_proxy_for_current_sandbox_policy()
|
||||
|
||||
@@ -2859,6 +2859,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
let services = SessionServices {
|
||||
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::new_uninitialized(
|
||||
&config.permissions.approval_policy,
|
||||
&config.permissions.sandbox_policy,
|
||||
))),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
@@ -3702,6 +3703,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
let services = SessionServices {
|
||||
mcp_connection_manager: Arc::new(RwLock::new(McpConnectionManager::new_uninitialized(
|
||||
&config.permissions.approval_policy,
|
||||
&config.permissions.sandbox_policy,
|
||||
))),
|
||||
mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()),
|
||||
unified_exec_manager: UnifiedExecProcessManager::new(
|
||||
|
||||
@@ -7,8 +7,6 @@ use codex_config::McpServerTransportConfig;
|
||||
use codex_config::load_global_mcp_servers;
|
||||
use codex_login::default_client::is_first_party_originator;
|
||||
use codex_login::default_client::originator;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::request_user_input::RequestUserInputArgs;
|
||||
use codex_protocol::request_user_input::RequestUserInputQuestion;
|
||||
use codex_protocol::request_user_input::RequestUserInputQuestionOption;
|
||||
@@ -22,6 +20,7 @@ use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::skills::model::SkillToolDependency;
|
||||
use codex_mcp::McpOAuthLoginSupport;
|
||||
use codex_mcp::mcp_permission_prompt_is_auto_approved;
|
||||
use codex_mcp::oauth_login_support;
|
||||
use codex_mcp::resolve_oauth_scopes;
|
||||
use codex_mcp::should_retry_without_scopes;
|
||||
@@ -218,7 +217,10 @@ async fn should_install_mcp_dependencies(
|
||||
missing: &HashMap<String, McpServerConfig>,
|
||||
cancellation_token: &CancellationToken,
|
||||
) -> bool {
|
||||
if is_full_access_mode(turn_context) {
|
||||
if mcp_permission_prompt_is_auto_approved(
|
||||
turn_context.approval_policy.value(),
|
||||
turn_context.sandbox_policy.get(),
|
||||
) {
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -305,14 +307,6 @@ fn format_missing_mcp_dependencies(missing: &HashMap<String, McpServerConfig>) -
|
||||
names.join(", ")
|
||||
}
|
||||
|
||||
fn is_full_access_mode(turn_context: &TurnContext) -> bool {
|
||||
matches!(turn_context.approval_policy.value(), AskForApproval::Never)
|
||||
&& matches!(
|
||||
turn_context.sandbox_policy.get(),
|
||||
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
|
||||
)
|
||||
}
|
||||
|
||||
fn canonical_mcp_key(transport: &str, identifier: &str, fallback: &str) -> String {
|
||||
let identifier = identifier.trim();
|
||||
if identifier.is_empty() {
|
||||
|
||||
@@ -34,16 +34,15 @@ use codex_analytics::build_track_events_context;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_features::Feature;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_mcp::mcp_permission_prompt_is_auto_approved;
|
||||
use codex_otel::sanitize_metric_tag_value;
|
||||
use codex_protocol::mcp::CallToolResult;
|
||||
use codex_protocol::openai_models::InputModality;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::McpInvocation;
|
||||
use codex_protocol::protocol::McpToolCallBeginEvent;
|
||||
use codex_protocol::protocol::McpToolCallEndEvent;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::request_user_input::RequestUserInputAnswer;
|
||||
use codex_protocol::request_user_input::RequestUserInputArgs;
|
||||
use codex_protocol::request_user_input::RequestUserInputQuestion;
|
||||
@@ -695,7 +694,10 @@ async fn maybe_request_mcp_tool_approval(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalDecision> {
|
||||
if is_full_access_mode(turn_context) {
|
||||
if mcp_permission_prompt_is_auto_approved(
|
||||
turn_context.approval_policy.value(),
|
||||
turn_context.sandbox_policy.get(),
|
||||
) {
|
||||
return None;
|
||||
}
|
||||
|
||||
@@ -956,14 +958,6 @@ async fn mcp_tool_approval_decision_from_guardian(
|
||||
}
|
||||
}
|
||||
|
||||
fn is_full_access_mode(turn_context: &TurnContext) -> bool {
|
||||
matches!(turn_context.approval_policy.value(), AskForApproval::Never)
|
||||
&& matches!(
|
||||
turn_context.sandbox_policy.get(),
|
||||
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
|
||||
)
|
||||
}
|
||||
|
||||
fn mcp_tool_approval_callsite_mode(
|
||||
approval_mode: AppToolApproval,
|
||||
_turn_context: &TurnContext,
|
||||
|
||||
@@ -12,6 +12,8 @@ use codex_config::types::ApprovalsReviewer;
|
||||
use codex_config::types::AppsConfigToml;
|
||||
use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::McpServerToolConfig;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
|
||||
Reference in New Issue
Block a user