Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/codex-mcp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
13 changes: 13 additions & 0 deletions codex-rs/codex-mcp/src/mcp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
59 changes: 53 additions & 6 deletions codex-rs/codex-mcp/src/mcp_connection_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)),
}
}

Expand All @@ -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,
Expand Down Expand Up @@ -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(),
),
}
}

Expand All @@ -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>,
Expand All @@ -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();
Expand Down
78 changes: 74 additions & 4 deletions codex-rs/codex-mcp/src/mcp_connection_manager_tests.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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![
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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(),
Expand Down
12 changes: 7 additions & 5 deletions codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -2559,11 +2560,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()
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/core/src/codex_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -3701,6 +3702,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(
Expand Down
16 changes: 5 additions & 11 deletions codex-rs/core/src/mcp_skill_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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() {
Expand Down
Loading
Loading