From fcf10478e63f2cc6f1162616ccc78a7db94e723c Mon Sep 17 00:00:00 2001 From: starr-openai Date: Mon, 20 Apr 2026 13:59:25 -0700 Subject: [PATCH] Add sub-agent MCP startup modes Add a multi-agent config knob that lets sub-agents either start fresh MCP clients, inherit a parent-owned MCP manager for thread-spawned children, or disable MCP startup entirely. Non-owning sessions skip refresh, policy mutation, and skill dependency MCP installation paths. Co-authored-by: Codex --- codex-rs/core/config.schema.json | 28 ++++ codex-rs/core/src/agent/control.rs | 48 ++++++ codex-rs/core/src/agent/control_tests.rs | 152 ++++++++++++++++++ codex-rs/core/src/codex_delegate.rs | 19 +++ codex-rs/core/src/codex_delegate_tests.rs | 39 +++++ codex-rs/core/src/config/config_tests.rs | 11 ++ codex-rs/core/src/config/mod.rs | 8 + codex-rs/core/src/mcp_skill_dependencies.rs | 88 ++++++++++ codex-rs/core/src/session/mcp.rs | 14 ++ codex-rs/core/src/session/mod.rs | 11 ++ codex-rs/core/src/session/session.rs | 93 ++++++----- codex-rs/core/src/session/tests.rs | 64 ++++++++ .../core/src/session/tests/guardian_tests.rs | 1 + codex-rs/core/src/session/turn_context.rs | 4 + codex-rs/core/src/state/mod.rs | 1 + codex-rs/core/src/state/service.rs | 22 +++ codex-rs/core/src/thread_manager.rs | 11 ++ codex-rs/features/src/feature_configs.rs | 14 ++ codex-rs/features/src/lib.rs | 1 + codex-rs/features/src/tests.rs | 3 + 20 files changed, 593 insertions(+), 39 deletions(-) diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index f0fbe89948..71a7cdfed5 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1074,6 +1074,9 @@ "hide_spawn_agent_metadata": { "type": "boolean" }, + "subagent_mcp_mode": { + "$ref": "#/definitions/SubagentMcpMode" + }, "usage_hint_enabled": { "type": "boolean" }, @@ -1834,6 +1837,31 @@ }, "type": "object" }, + "SubagentMcpMode": { + "oneOf": [ + { + "description": "Sub-agents create their own MCP clients and server processes.", + "enum": [ + "fresh" + ], + "type": "string" + }, + { + "description": "Sub-agents reuse the parent thread's MCP connection manager without owning refresh or policy state; parentless sub-agents skip MCP startup.", + "enum": [ + "inherit_parent" + ], + "type": "string" + }, + { + "description": "Sub-agents skip MCP startup and ignore configured MCP servers.", + "enum": [ + "disabled" + ], + "type": "string" + } + ] + }, "ToolSuggestConfig": { "additionalProperties": false, "properties": { diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index 984b92e7bb..cc96d516e5 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -8,6 +8,7 @@ use crate::codex_thread::ThreadConfigSnapshot; use crate::find_archived_thread_path_by_id_str; use crate::find_thread_path_by_id_str; use crate::rollout::RolloutRecorder; +use crate::session::McpStartupMode; use crate::session::emit_subagent_session_started; use crate::session_prefix::format_subagent_context_line; use crate::session_prefix::format_subagent_notification_message; @@ -15,6 +16,7 @@ use crate::shell_snapshot::ShellSnapshot; use crate::thread_manager::ThreadManagerState; use crate::thread_rollout_truncation::truncate_rollout_to_last_n_fork_turns; use codex_features::Feature; +use codex_features::SubagentMcpMode; use codex_protocol::AgentPath; use codex_protocol::ThreadId; use codex_protocol::error::CodexErr; @@ -192,6 +194,9 @@ impl AgentControl { let inherited_exec_policy = self .inherited_exec_policy_for_source(&state, session_source.as_ref(), &config) .await; + let mcp_startup_mode = self + .mcp_startup_mode_for_source(&state, session_source.as_ref(), &config) + .await; let (session_source, mut agent_metadata) = match session_source { Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { parent_thread_id, @@ -225,6 +230,7 @@ impl AgentControl { &options, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, ) .await? } @@ -238,6 +244,7 @@ impl AgentControl { /*metrics_service_name*/ None, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, ) .await? } @@ -332,6 +339,7 @@ impl AgentControl { options: &SpawnAgentOptions, inherited_shell_snapshot: Option>, inherited_exec_policy: Option>, + mcp_startup_mode: McpStartupMode, ) -> CodexResult { if options.fork_parent_spawn_call_id.is_none() { return Err(CodexErr::Fatal( @@ -397,6 +405,7 @@ impl AgentControl { /*persist_extended_history*/ false, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, ) .await } @@ -525,6 +534,9 @@ impl AgentControl { let inherited_exec_policy = self .inherited_exec_policy_for_source(&state, Some(&session_source), &config) .await; + let mcp_startup_mode = self + .mcp_startup_mode_for_source(&state, Some(&session_source), &config) + .await; let rollout_path = match find_thread_path_by_id_str(config.codex_home.as_path(), &thread_id.to_string()) .await? @@ -546,6 +558,7 @@ impl AgentControl { session_source, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, ) .await?; let mut agent_metadata = agent_metadata; @@ -1056,6 +1069,41 @@ impl AgentControl { )) } + async fn mcp_startup_mode_for_source( + &self, + state: &Arc, + session_source: Option<&SessionSource>, + child_config: &crate::config::Config, + ) -> McpStartupMode { + let Some(SessionSource::SubAgent(subagent_source)) = session_source else { + return McpStartupMode::Fresh; + }; + + match ( + child_config.multi_agent_v2.subagent_mcp_mode, + subagent_source, + ) { + (SubagentMcpMode::Fresh, _) => McpStartupMode::Fresh, + (SubagentMcpMode::Disabled, _) => McpStartupMode::Disabled, + ( + SubagentMcpMode::InheritParent, + SubAgentSource::ThreadSpawn { + parent_thread_id, .. + }, + ) => state + .get_thread(*parent_thread_id) + .await + .ok() + .map(|parent_thread| { + McpStartupMode::Inherit(Arc::clone( + &parent_thread.codex.session.services.mcp_connection_manager, + )) + }) + .unwrap_or(McpStartupMode::Disabled), + (SubagentMcpMode::InheritParent, _) => McpStartupMode::Disabled, + } + } + async fn open_thread_spawn_children( &self, parent_thread_id: ThreadId, diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index b444d8c67c..60abff045f 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -6,8 +6,10 @@ use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigBuilder; use crate::contextual_user_message::SUBAGENT_NOTIFICATION_OPEN_TAG; +use crate::state::McpConnectionManagerMode; use assert_matches::assert_matches; use codex_features::Feature; +use codex_features::SubagentMcpMode; use codex_login::CodexAuth; use codex_protocol::AgentPath; use codex_protocol::config_types::ModeKind; @@ -592,6 +594,156 @@ async fn spawn_agent_creates_thread_and_sends_prompt() { assert_eq!(captured, Some(expected)); } +#[tokio::test] +async fn subagent_mcp_mode_does_not_affect_root_thread_startup() { + let harness = AgentControlHarness::new().await; + let mut config = harness.config.clone(); + config.multi_agent_v2.subagent_mcp_mode = SubagentMcpMode::Disabled; + + let thread_id = harness + .control + .spawn_agent( + config, + text_input("root task"), + /*session_source*/ None, + ) + .await + .expect("spawn_agent should succeed"); + let thread = harness + .manager + .get_thread(thread_id) + .await + .expect("thread should be registered"); + + assert_eq!( + thread.codex.session.services.mcp_connection_manager_mode, + McpConnectionManagerMode::Owned + ); +} + +#[tokio::test] +async fn spawn_agent_can_inherit_parent_mcp_connection_manager() { + let harness = AgentControlHarness::new().await; + let (parent_thread_id, parent_thread) = harness.start_thread().await; + let mut config = harness.config.clone(); + config.multi_agent_v2.subagent_mcp_mode = SubagentMcpMode::InheritParent; + + let child_thread_id = harness + .control + .spawn_agent( + config, + text_input("child task"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id, + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + })), + ) + .await + .expect("spawn_agent should succeed"); + let child_thread = harness + .manager + .get_thread(child_thread_id) + .await + .expect("child thread should be registered"); + + assert!(Arc::ptr_eq( + &parent_thread.codex.session.services.mcp_connection_manager, + &child_thread.codex.session.services.mcp_connection_manager, + )); + assert_eq!( + parent_thread + .codex + .session + .services + .mcp_connection_manager_mode, + McpConnectionManagerMode::Owned + ); + assert_eq!( + child_thread + .codex + .session + .services + .mcp_connection_manager_mode, + McpConnectionManagerMode::Inherited + ); +} + +#[tokio::test] +async fn spawn_agent_disabled_mcp_mode_does_not_inherit_parent_manager() { + let harness = AgentControlHarness::new().await; + let (parent_thread_id, parent_thread) = harness.start_thread().await; + let mut config = harness.config.clone(); + config.multi_agent_v2.subagent_mcp_mode = SubagentMcpMode::Disabled; + + let child_thread_id = harness + .control + .spawn_agent( + config, + text_input("child task"), + Some(SessionSource::SubAgent(SubAgentSource::ThreadSpawn { + parent_thread_id, + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + })), + ) + .await + .expect("spawn_agent should succeed"); + let child_thread = harness + .manager + .get_thread(child_thread_id) + .await + .expect("child thread should be registered"); + + assert!(!Arc::ptr_eq( + &parent_thread.codex.session.services.mcp_connection_manager, + &child_thread.codex.session.services.mcp_connection_manager, + )); + assert_eq!( + child_thread + .codex + .session + .services + .mcp_connection_manager_mode, + McpConnectionManagerMode::Disabled + ); +} + +#[tokio::test] +async fn parentless_subagent_source_disables_mcp_when_inherit_parent_is_requested() { + let harness = AgentControlHarness::new().await; + let mut config = harness.config.clone(); + config.multi_agent_v2.subagent_mcp_mode = SubagentMcpMode::InheritParent; + + let child_thread_id = harness + .control + .spawn_agent( + config, + text_input("memory consolidation task"), + Some(SessionSource::SubAgent(SubAgentSource::MemoryConsolidation)), + ) + .await + .expect("spawn_agent should succeed"); + let child_thread = harness + .manager + .get_thread(child_thread_id) + .await + .expect("child thread should be registered"); + + assert_eq!( + child_thread + .codex + .session + .services + .mcp_connection_manager_mode, + McpConnectionManagerMode::Disabled + ); +} + #[tokio::test] async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { let harness = AgentControlHarness::new().await; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 5d67223b33..c193ee9a40 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -44,6 +44,7 @@ use crate::mcp_tool_call::lookup_mcp_tool_metadata; use crate::session::Codex; use crate::session::CodexSpawnArgs; use crate::session::CodexSpawnOk; +use crate::session::McpStartupMode; use crate::session::SUBMISSION_CHANNEL_CAPACITY; use crate::session::emit_subagent_session_started; use crate::session::session::Session; @@ -74,6 +75,8 @@ pub(crate) async fn run_codex_thread_interactive( ) -> Result { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_ops, rx_ops) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); + let mcp_startup_mode = + mcp_startup_mode_for_subagent_source(&config, &parent_session, &subagent_source); let CodexSpawnOk { codex, .. } = Box::pin(Codex::spawn(CodexSpawnArgs { config, @@ -95,6 +98,7 @@ pub(crate) async fn run_codex_thread_interactive( inherited_shell_snapshot: None, user_shell_override: None, inherited_exec_policy: Some(Arc::clone(&parent_session.services.exec_policy)), + mcp_startup_mode, parent_trace: None, analytics_events_client: Some(parent_session.services.analytics_events_client.clone()), })) @@ -153,6 +157,21 @@ pub(crate) async fn run_codex_thread_interactive( }) } +fn mcp_startup_mode_for_subagent_source( + config: &Config, + parent_session: &Arc, + subagent_source: &SubAgentSource, +) -> McpStartupMode { + match (config.multi_agent_v2.subagent_mcp_mode, subagent_source) { + (codex_features::SubagentMcpMode::Fresh, _) => McpStartupMode::Fresh, + (codex_features::SubagentMcpMode::Disabled, _) => McpStartupMode::Disabled, + (codex_features::SubagentMcpMode::InheritParent, SubAgentSource::ThreadSpawn { .. }) => { + McpStartupMode::Inherit(Arc::clone(&parent_session.services.mcp_connection_manager)) + } + (codex_features::SubagentMcpMode::InheritParent, _) => McpStartupMode::Disabled, + } +} + /// Convenience wrapper for one-time use with an initial prompt. /// /// Internally calls the interactive variant, then immediately submits the provided input. diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index d26460a255..c110718168 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -2,6 +2,8 @@ use super::*; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX; use async_channel::bounded; +use codex_features::SubagentMcpMode; +use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::ResponseItem; @@ -32,6 +34,43 @@ use tokio::sync::Mutex; use tokio::sync::watch; use tokio::time::timeout; +#[tokio::test] +async fn delegate_mcp_inherit_parent_only_applies_to_thread_spawn_sources() { + let (parent_session, _parent_ctx, _rx_evt) = + crate::session::tests::make_session_and_context_with_rx().await; + let mut config = (*parent_session.get_config().await).clone(); + config.multi_agent_v2.subagent_mcp_mode = SubagentMcpMode::InheritParent; + + let thread_spawn_mode = mcp_startup_mode_for_subagent_source( + &config, + &parent_session, + &SubAgentSource::ThreadSpawn { + parent_thread_id: ThreadId::new(), + depth: 1, + agent_path: None, + agent_nickname: None, + agent_role: None, + }, + ); + let review_mode = + mcp_startup_mode_for_subagent_source(&config, &parent_session, &SubAgentSource::Review); + let other_mode = mcp_startup_mode_for_subagent_source( + &config, + &parent_session, + &SubAgentSource::Other("guardian".to_string()), + ); + + match thread_spawn_mode { + McpStartupMode::Inherit(manager) => assert!(Arc::ptr_eq( + &manager, + &parent_session.services.mcp_connection_manager + )), + _ => panic!("thread-spawned delegates should inherit the parent MCP manager"), + } + assert!(matches!(review_mode, McpStartupMode::Disabled)); + assert!(matches!(other_mode, McpStartupMode::Disabled)); +} + #[tokio::test] async fn forward_events_cancelled_while_send_blocked_shuts_down_delegate() { let (tx_events, rx_events) = bounded(1); diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 6e721b1238..03a3e3793b 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -6466,6 +6466,7 @@ enabled = true usage_hint_enabled = false usage_hint_text = "Custom delegation guidance." hide_spawn_agent_metadata = true +subagent_mcp_mode = "inherit_parent" "#, )?; @@ -6482,6 +6483,10 @@ hide_spawn_agent_metadata = true Some("Custom delegation guidance.") ); assert!(config.multi_agent_v2.hide_spawn_agent_metadata); + assert_eq!( + config.multi_agent_v2.subagent_mcp_mode, + codex_features::SubagentMcpMode::InheritParent + ); Ok(()) } @@ -6497,11 +6502,13 @@ async fn profile_multi_agent_v2_config_overrides_base() -> std::io::Result<()> { usage_hint_enabled = true usage_hint_text = "base hint" hide_spawn_agent_metadata = true +subagent_mcp_mode = "disabled" [profiles.no_hint.features.multi_agent_v2] usage_hint_enabled = false usage_hint_text = "profile hint" hide_spawn_agent_metadata = false +subagent_mcp_mode = "inherit_parent" "#, )?; @@ -6517,6 +6524,10 @@ hide_spawn_agent_metadata = false Some("profile hint") ); assert!(!config.multi_agent_v2.hide_spawn_agent_metadata); + assert_eq!( + config.multi_agent_v2.subagent_mcp_mode, + codex_features::SubagentMcpMode::InheritParent + ); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6320bff2e3..139e541376 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -56,6 +56,7 @@ use codex_features::FeatureToml; use codex_features::Features; use codex_features::FeaturesToml; use codex_features::MultiAgentV2ConfigToml; +use codex_features::SubagentMcpMode; use codex_git_utils::resolve_root_git_project_for_trust; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; @@ -602,6 +603,7 @@ pub struct MultiAgentV2Config { pub usage_hint_enabled: bool, pub usage_hint_text: Option, pub hide_spawn_agent_metadata: bool, + pub subagent_mcp_mode: SubagentMcpMode, } impl Default for MultiAgentV2Config { @@ -610,6 +612,7 @@ impl Default for MultiAgentV2Config { usage_hint_enabled: true, usage_hint_text: None, hide_spawn_agent_metadata: false, + subagent_mcp_mode: SubagentMcpMode::Fresh, } } } @@ -1399,11 +1402,16 @@ fn resolve_multi_agent_v2_config( .and_then(|config| config.hide_spawn_agent_metadata) .or_else(|| base.and_then(|config| config.hide_spawn_agent_metadata)) .unwrap_or(default.hide_spawn_agent_metadata); + let subagent_mcp_mode = profile + .and_then(|config| config.subagent_mcp_mode) + .or_else(|| base.and_then(|config| config.subagent_mcp_mode)) + .unwrap_or(default.subagent_mcp_mode); MultiAgentV2Config { usage_hint_enabled, usage_hint_text, hide_spawn_agent_metadata, + subagent_mcp_mode, } } diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index c711d1a158..6aa64a6bbe 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -13,6 +13,7 @@ use codex_protocol::request_user_input::RequestUserInputQuestionOption; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::perform_oauth_login; use tokio_util::sync::CancellationToken; +use tracing::debug; use tracing::warn; use crate::SkillMetadata; @@ -35,6 +36,18 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies( cancellation_token: &CancellationToken, mentioned_skills: &[SkillMetadata], ) { + if !sess + .services + .mcp_connection_manager_mode + .can_install_mcp_dependencies() + { + debug!( + mode = ?sess.services.mcp_connection_manager_mode, + "skipping skill MCP dependency prompt for non-owning session" + ); + return; + } + let originator_value = originator().value; if !is_first_party_originator(originator_value.as_str()) { // Only support first-party clients for now. @@ -78,6 +91,18 @@ pub(crate) async fn maybe_install_mcp_dependencies( config: &crate::config::Config, mentioned_skills: &[SkillMetadata], ) { + if !sess + .services + .mcp_connection_manager_mode + .can_install_mcp_dependencies() + { + debug!( + mode = ?sess.services.mcp_connection_manager_mode, + "skipping skill MCP dependency install for non-owning session" + ); + return; + } + if mentioned_skills.is_empty() || !config .features @@ -468,3 +493,66 @@ fn collect_missing_mcp_dependencies( missing } + +#[cfg(test)] +mod tests { + use super::*; + use crate::skills::model::SkillDependencies; + use crate::state::McpConnectionManagerMode; + use codex_protocol::protocol::SkillScope; + use codex_utils_absolute_path::test_support::PathBufExt; + use codex_utils_absolute_path::test_support::test_path_buf; + + fn skill_with_mcp_dependency() -> SkillMetadata { + SkillMetadata { + name: "needs-mcp".to_string(), + description: "test skill".to_string(), + short_description: None, + interface: None, + dependencies: Some(SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "mcp".to_string(), + value: "dep-server".to_string(), + description: None, + transport: Some("streamable_http".to_string()), + command: None, + url: Some("https://example.invalid/mcp".to_string()), + }], + }), + policy: None, + path_to_skills_md: test_path_buf("/tmp/needs-mcp/SKILL.md").abs(), + scope: SkillScope::User, + } + } + + #[tokio::test] + async fn non_owning_sessions_skip_skill_mcp_dependency_install() { + for mode in [ + McpConnectionManagerMode::Inherited, + McpConnectionManagerMode::Disabled, + ] { + let (mut session, turn_context) = + crate::session::tests::make_session_and_context().await; + session.services.mcp_connection_manager_mode = mode; + std::fs::create_dir_all(&turn_context.config.codex_home) + .expect("create temp codex home"); + let config_path = turn_context.config.codex_home.join("config.toml"); + assert!(!config_path.exists()); + let old_token = session.mcp_startup_cancellation_token().await; + + maybe_install_mcp_dependencies( + &session, + &turn_context, + turn_context.config.as_ref(), + &[skill_with_mcp_dependency()], + ) + .await; + + assert!( + !config_path.exists(), + "non-owning sessions must not write global MCP config in {mode:?} mode" + ); + assert!(!old_token.is_cancelled()); + } + } +} diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 56628f0543..352a9c9efc 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -227,6 +227,13 @@ impl Session { let Some(refresh_config) = refresh_config else { return; }; + if !self.services.mcp_connection_manager_mode.allows_refresh() { + debug!( + mode = ?self.services.mcp_connection_manager_mode, + "skipping MCP server refresh for non-owning session" + ); + return; + } let McpServerRefreshConfig { mcp_servers, @@ -261,6 +268,13 @@ impl Session { mcp_servers: HashMap, store_mode: OAuthCredentialsStoreMode, ) { + if !self.services.mcp_connection_manager_mode.allows_refresh() { + debug!( + mode = ?self.services.mcp_connection_manager_mode, + "skipping immediate MCP server refresh for non-owning session" + ); + return; + } self.refresh_mcp_servers_inner(turn_context, mcp_servers, store_mode) .await; } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index fd2623f07b..da454ad04a 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -387,11 +387,20 @@ pub(crate) struct CodexSpawnArgs { pub(crate) metrics_service_name: Option, pub(crate) inherited_shell_snapshot: Option>, pub(crate) inherited_exec_policy: Option>, + pub(crate) mcp_startup_mode: McpStartupMode, pub(crate) user_shell_override: Option, pub(crate) parent_trace: Option, pub(crate) analytics_events_client: Option, } +#[derive(Clone, Default)] +pub(crate) enum McpStartupMode { + #[default] + Fresh, + Inherit(Arc>), + Disabled, +} + pub(crate) const INITIAL_SUBMIT_ID: &str = ""; pub(crate) const SUBMISSION_CHANNEL_CAPACITY: usize = 512; const CYBER_VERIFY_URL: &str = "https://chatgpt.com/cyber"; @@ -441,6 +450,7 @@ impl Codex { inherited_shell_snapshot, user_shell_override, inherited_exec_policy, + mcp_startup_mode, parent_trace: _, analytics_events_client, } = args; @@ -638,6 +648,7 @@ impl Codex { skills_watcher, agent_control, environment, + mcp_startup_mode, analytics_events_client, ) .await diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 766ac79ec1..75d48a2575 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1,4 +1,5 @@ use super::*; +use crate::state::McpConnectionManagerMode; /// Context for an initialized model agent /// @@ -223,6 +224,7 @@ impl Session { skills_watcher: Arc, agent_control: AgentControl, environment: Option>, + mcp_startup_mode: McpStartupMode, analytics_events_client: Option, ) -> anyhow::Result> { debug!( @@ -626,10 +628,20 @@ impl Session { // before any MCP-related events. It is reasonable to consider // changing this to use Option or OnceCell, though the current // 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_connection_manager: match &mcp_startup_mode { + McpStartupMode::Inherit(manager) => Arc::clone(manager), + McpStartupMode::Fresh | McpStartupMode::Disabled => { + Arc::new(RwLock::new(McpConnectionManager::new_uninitialized( + &config.permissions.approval_policy, + &config.permissions.sandbox_policy, + ))) + } + }, + mcp_connection_manager_mode: match &mcp_startup_mode { + McpStartupMode::Fresh => McpConnectionManagerMode::Owned, + McpStartupMode::Inherit(_) => McpConnectionManagerMode::Inherited, + McpStartupMode::Disabled => McpConnectionManagerMode::Disabled, + }, mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), unified_exec_manager: UnifiedExecProcessManager::new( config.background_terminal_max_timeout, @@ -756,43 +768,46 @@ impl Session { required_mcp_servers.sort(); let enabled_mcp_server_count = mcp_servers.values().filter(|server| server.enabled).count(); let required_mcp_server_count = required_mcp_servers.len(); - let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await; - { - let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; - cancel_guard.cancel(); - *cancel_guard = CancellationToken::new(); - } - let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( - &mcp_servers, - config.mcp_oauth_credentials_store_mode, - auth_statuses.clone(), - &session_configuration.approval_policy, - INITIAL_SUBMIT_ID.to_owned(), - tx_event.clone(), - session_configuration.sandbox_policy.get().clone(), - config.codex_home.to_path_buf(), - codex_apps_tools_cache_key(auth), - tool_plugin_provenance, - ) - .instrument(info_span!( - "session_init.mcp_manager_init", - otel.name = "session_init.mcp_manager_init", - session_init.enabled_mcp_server_count = enabled_mcp_server_count, - session_init.required_mcp_server_count = required_mcp_server_count, - )) - .await; - { - let mut manager_guard = sess.services.mcp_connection_manager.write().await; - *manager_guard = mcp_connection_manager; - } - { - let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; - if cancel_guard.is_cancelled() { - cancel_token.cancel(); + if matches!(mcp_startup_mode, McpStartupMode::Fresh) { + let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await; + { + let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; + cancel_guard.cancel(); + *cancel_guard = CancellationToken::new(); + } + let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( + &mcp_servers, + config.mcp_oauth_credentials_store_mode, + auth_statuses.clone(), + &session_configuration.approval_policy, + INITIAL_SUBMIT_ID.to_owned(), + tx_event.clone(), + session_configuration.sandbox_policy.get().clone(), + config.codex_home.to_path_buf(), + codex_apps_tools_cache_key(auth), + tool_plugin_provenance, + ) + .instrument(info_span!( + "session_init.mcp_manager_init", + otel.name = "session_init.mcp_manager_init", + session_init.enabled_mcp_server_count = enabled_mcp_server_count, + session_init.required_mcp_server_count = required_mcp_server_count, + )) + .await; + { + let mut manager_guard = sess.services.mcp_connection_manager.write().await; + *manager_guard = mcp_connection_manager; + } + { + let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; + if cancel_guard.is_cancelled() { + cancel_token.cancel(); + } + *cancel_guard = cancel_token; } - *cancel_guard = cancel_token; } - if !required_mcp_servers.is_empty() { + if !required_mcp_servers.is_empty() && !matches!(mcp_startup_mode, McpStartupMode::Disabled) + { let failures = sess .services .mcp_connection_manager diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index d2e0d5dcb3..a3c663e8ad 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -39,6 +39,7 @@ use tracing::Span; use crate::RolloutRecorderParams; use crate::rollout::policy::EventPersistenceMode; use crate::rollout::recorder::RolloutRecorder; +use crate::state::McpConnectionManagerMode; use crate::state::TaskKind; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; @@ -2806,6 +2807,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { .await .expect("create environment"), )), + McpStartupMode::Fresh, /*analytics_events_client*/ None, ) .await; @@ -2913,6 +2915,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { &config.permissions.approval_policy, &config.permissions.sandbox_policy, ))), + mcp_connection_manager_mode: McpConnectionManagerMode::Owned, mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), unified_exec_manager: UnifiedExecProcessManager::new( config.background_terminal_max_timeout, @@ -3128,6 +3131,7 @@ async fn make_session_with_config_and_rx( .await .expect("create environment"), )), + McpStartupMode::Fresh, /*analytics_events_client*/ None, ) .await?; @@ -3876,6 +3880,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( &config.permissions.approval_policy, &config.permissions.sandbox_policy, ))), + mcp_connection_manager_mode: McpConnectionManagerMode::Owned, mcp_startup_cancellation_token: Mutex::new(CancellationToken::new()), unified_exec_manager: UnifiedExecProcessManager::new( config.background_terminal_max_timeout, @@ -4076,6 +4081,65 @@ async fn refresh_mcp_servers_is_deferred_until_next_turn() { assert!(!new_token.is_cancelled()); } +#[tokio::test] +async fn inherited_mcp_manager_sessions_skip_refresh() { + let (mut session, turn_context) = make_session_and_context().await; + session.services.mcp_connection_manager_mode = McpConnectionManagerMode::Inherited; + let old_token = session.mcp_startup_cancellation_token().await; + assert!(!old_token.is_cancelled()); + + let mcp_oauth_credentials_store_mode = + serde_json::to_value(OAuthCredentialsStoreMode::Auto).expect("serialize store mode"); + let refresh_config = McpServerRefreshConfig { + mcp_servers: json!({}), + mcp_oauth_credentials_store_mode, + }; + { + let mut guard = session.pending_mcp_server_refresh_config.lock().await; + *guard = Some(refresh_config); + } + + session + .refresh_mcp_servers_if_requested(&turn_context) + .await; + + assert!(!old_token.is_cancelled()); + assert!( + session + .pending_mcp_server_refresh_config + .lock() + .await + .is_none() + ); + let new_token = session.mcp_startup_cancellation_token().await; + assert!(!new_token.is_cancelled()); +} + +#[tokio::test] +async fn non_owning_mcp_manager_sessions_skip_immediate_refresh() { + for mode in [ + McpConnectionManagerMode::Inherited, + McpConnectionManagerMode::Disabled, + ] { + let (mut session, turn_context) = make_session_and_context().await; + session.services.mcp_connection_manager_mode = mode; + let old_token = session.mcp_startup_cancellation_token().await; + assert!(!old_token.is_cancelled()); + + session + .refresh_mcp_servers_now( + &turn_context, + HashMap::new(), + OAuthCredentialsStoreMode::Auto, + ) + .await; + + assert!(!old_token.is_cancelled()); + let new_token = session.mcp_startup_cancellation_token().await; + assert!(!new_token.is_cancelled()); + } +} + #[tokio::test] async fn record_model_warning_appends_user_message() { let (mut session, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 9ccd6eb4e0..dbb3deb358 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -453,6 +453,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { metrics_service_name: None, inherited_shell_snapshot: None, inherited_exec_policy: Some(Arc::new(parent_exec_policy)), + mcp_startup_mode: crate::session::McpStartupMode::Fresh, user_shell_override: None, parent_trace: None, analytics_events_client: None, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 2969d7efad..baff0cb584 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -513,6 +513,10 @@ impl Session { final_output_json_schema: Option>, ) -> Arc { let per_turn_config = Self::build_per_turn_config(&session_configuration); + if self + .services + .mcp_connection_manager_mode + .owns_policy_state() { let mcp_connection_manager = self.services.mcp_connection_manager.read().await; mcp_connection_manager.set_approval_policy(&session_configuration.approval_policy); diff --git a/codex-rs/core/src/state/mod.rs b/codex-rs/core/src/state/mod.rs index f3ebc7225d..aa0aa3f407 100644 --- a/codex-rs/core/src/state/mod.rs +++ b/codex-rs/core/src/state/mod.rs @@ -2,6 +2,7 @@ mod service; mod session; mod turn; +pub(crate) use service::McpConnectionManagerMode; pub(crate) use service::SessionServices; pub(crate) use session::SessionState; pub(crate) use turn::ActiveTurn; diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 5db38f7b72..be9e293e0a 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -33,6 +33,7 @@ use tokio_util::sync::CancellationToken; pub(crate) struct SessionServices { pub(crate) mcp_connection_manager: Arc>, + pub(crate) mcp_connection_manager_mode: McpConnectionManagerMode, pub(crate) mcp_startup_cancellation_token: Mutex, pub(crate) unified_exec_manager: UnifiedExecProcessManager, #[cfg_attr(not(unix), allow(dead_code))] @@ -66,3 +67,24 @@ pub(crate) struct SessionServices { pub(crate) code_mode_service: CodeModeService, pub(crate) environment: Option>, } + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum McpConnectionManagerMode { + Owned, + Inherited, + Disabled, +} + +impl McpConnectionManagerMode { + pub(crate) fn allows_refresh(self) -> bool { + matches!(self, Self::Owned) + } + + pub(crate) fn owns_policy_state(self) -> bool { + matches!(self, Self::Owned | Self::Disabled) + } + + pub(crate) fn can_install_mcp_dependencies(self) -> bool { + matches!(self, Self::Owned) + } +} diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 03bf768ac5..eff9abc2a8 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -11,6 +11,7 @@ use crate::session::Codex; use crate::session::CodexSpawnArgs; use crate::session::CodexSpawnOk; use crate::session::INITIAL_SUBMIT_ID; +use crate::session::McpStartupMode; use crate::shell_snapshot::ShellSnapshot; use crate::skills_watcher::SkillsWatcher; use crate::skills_watcher::SkillsWatcherEvent; @@ -773,6 +774,7 @@ impl ThreadManagerState { /*metrics_service_name*/ None, /*inherited_shell_snapshot*/ None, /*inherited_exec_policy*/ None, + McpStartupMode::Fresh, )) .await } @@ -787,6 +789,7 @@ impl ThreadManagerState { metrics_service_name: Option, inherited_shell_snapshot: Option>, inherited_exec_policy: Option>, + mcp_startup_mode: McpStartupMode, ) -> CodexResult { Box::pin(self.spawn_thread_with_source( config, @@ -799,6 +802,7 @@ impl ThreadManagerState { metrics_service_name, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, /*parent_trace*/ None, /*user_shell_override*/ None, )) @@ -813,6 +817,7 @@ impl ThreadManagerState { session_source: SessionSource, inherited_shell_snapshot: Option>, inherited_exec_policy: Option>, + mcp_startup_mode: McpStartupMode, ) -> CodexResult { let initial_history = RolloutRecorder::get_rollout_history(&rollout_path).await?; Box::pin(self.spawn_thread_with_source( @@ -826,6 +831,7 @@ impl ThreadManagerState { /*metrics_service_name*/ None, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, /*parent_trace*/ None, /*user_shell_override*/ None, )) @@ -842,6 +848,7 @@ impl ThreadManagerState { persist_extended_history: bool, inherited_shell_snapshot: Option>, inherited_exec_policy: Option>, + mcp_startup_mode: McpStartupMode, ) -> CodexResult { Box::pin(self.spawn_thread_with_source( config, @@ -854,6 +861,7 @@ impl ThreadManagerState { /*metrics_service_name*/ None, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, /*parent_trace*/ None, /*user_shell_override*/ None, )) @@ -885,6 +893,7 @@ impl ThreadManagerState { metrics_service_name, /*inherited_shell_snapshot*/ None, /*inherited_exec_policy*/ None, + McpStartupMode::Fresh, parent_trace, user_shell_override, )) @@ -904,6 +913,7 @@ impl ThreadManagerState { metrics_service_name: Option, inherited_shell_snapshot: Option>, inherited_exec_policy: Option>, + mcp_startup_mode: McpStartupMode, parent_trace: Option, user_shell_override: Option, ) -> CodexResult { @@ -944,6 +954,7 @@ impl ThreadManagerState { metrics_service_name, inherited_shell_snapshot, inherited_exec_policy, + mcp_startup_mode, user_shell_override, parent_trace, analytics_events_client: self.analytics_events_client.clone(), diff --git a/codex-rs/features/src/feature_configs.rs b/codex-rs/features/src/feature_configs.rs index 0db4e4e82e..e53700da9d 100644 --- a/codex-rs/features/src/feature_configs.rs +++ b/codex-rs/features/src/feature_configs.rs @@ -14,6 +14,8 @@ pub struct MultiAgentV2ConfigToml { pub usage_hint_text: Option, #[serde(skip_serializing_if = "Option::is_none")] pub hide_spawn_agent_metadata: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub subagent_mcp_mode: Option, } impl FeatureConfig for MultiAgentV2ConfigToml { @@ -21,3 +23,15 @@ impl FeatureConfig for MultiAgentV2ConfigToml { self.enabled } } + +#[derive(Serialize, Deserialize, Debug, Clone, Copy, Default, PartialEq, Eq, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum SubagentMcpMode { + /// Sub-agents create their own MCP clients and server processes. + #[default] + Fresh, + /// Sub-agents reuse the parent thread's MCP connection manager without owning refresh or policy state; parentless sub-agents skip MCP startup. + InheritParent, + /// Sub-agents skip MCP startup and ignore configured MCP servers. + Disabled, +} diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 9a8862c277..5aefd4b9d7 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -17,6 +17,7 @@ use toml::Table; mod feature_configs; mod legacy; pub use feature_configs::MultiAgentV2ConfigToml; +pub use feature_configs::SubagentMcpMode; use legacy::LegacyFeatureToggles; pub use legacy::legacy_feature_keys; diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index a561d0ee3f..0fd7152e7a 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -350,6 +350,7 @@ enabled = true usage_hint_enabled = false usage_hint_text = "Custom delegation guidance." hide_spawn_agent_metadata = true +subagent_mcp_mode = "inherit_parent" "#, ) .expect("features table should deserialize"); @@ -365,6 +366,7 @@ hide_spawn_agent_metadata = true usage_hint_enabled: Some(false), usage_hint_text: Some("Custom delegation guidance.".to_string()), hide_spawn_agent_metadata: Some(true), + subagent_mcp_mode: Some(crate::SubagentMcpMode::InheritParent), })) ); } @@ -396,6 +398,7 @@ usage_hint_enabled = false usage_hint_enabled: Some(false), usage_hint_text: None, hide_spawn_agent_metadata: None, + subagent_mcp_mode: None, })) ); }