From 4a5a67649952e7a657c32d8534c42fdfc8db2448 Mon Sep 17 00:00:00 2001 From: jif Date: Thu, 11 Jun 2026 20:54:52 +0100 Subject: [PATCH] Resolve MCP server registrations through a catalog (#27634) ## Why MCP servers currently come from user config, local plugins, compatibility Apps synthesis, and host extensions. Those sources were composed by mutating a shared map, leaving registration identity, precedence, removal, and provenance implicit in assembly order. Before adding executor-owned MCPs, Codex needs one durable resolution boundary above `McpConnectionManager`. This PR introduces that boundary while preserving current server configuration, policy, and runtime behavior. Executor-scoped registrations and explicit policy layers remain follow-ups. ## What changed - Add typed `McpServerRegistration` inputs and an immutable `ResolvedMcpCatalog` in `codex-mcp`. - Retain each registration's complete `McpServerConfig`, including its environment binding, while recording its source and provenance. - Preserve the existing structural precedence between plugin, config, compatibility, and ordered extension sources. - Resolve equal-precedence actions by contribution order; provenance IDs are used only for diagnostics and cannot affect the winner. - Preserve extension removals and the existing name-scoped `enabled = false` veto. - Report same-tier conflicts with every contender and the final catalog outcome, including whether the winning action registers or removes the server. - Require MCP contributors to provide a stable diagnostic identity. - Derive materialized server maps and plugin ownership from the resolved catalog. `McpConnectionManager`, transport startup, tool calls, and resource routing continue to consume the same effective `McpServerConfig` values. ## Scope This PR does not add new MCP capabilities or change user-visible behavior. It does not add executor plugin discovery, thread-scoped registrations, dynamic refresh generations, or new user/managed policy semantics. ## Verification - Added focused catalog coverage for source precedence, complete configuration preservation, disabled vetoes, plugin ownership, contribution-order tie breaking, removal outcomes, and conflict diagnostics. - Extended hosted Apps coverage for ordered extension removal and Apps-disabled hosts with and without the hosted extension installed. - `cargo check -p codex-mcp --tests -p codex-extension-api -p codex-core` --- codex-rs/codex-mcp/src/catalog.rs | 333 ++++++++++++++++++ codex-rs/codex-mcp/src/catalog_tests.rs | 257 ++++++++++++++ codex-rs/codex-mcp/src/lib.rs | 9 + codex-rs/codex-mcp/src/mcp/mod.rs | 20 +- codex-rs/codex-mcp/src/mcp/mod_tests.rs | 38 +- codex-rs/core/src/config/config_tests.rs | 26 +- codex-rs/core/src/config/mod.rs | 42 ++- codex-rs/core/src/connectors.rs | 5 +- codex-rs/core/src/mcp.rs | 88 +++-- codex-rs/core/src/session/mcp.rs | 6 +- codex-rs/core/src/session/session.rs | 11 +- .../ext/extension-api/src/contributors.rs | 3 + codex-rs/ext/mcp/src/lib.rs | 4 + codex-rs/ext/mcp/tests/hosted_apps_mcp.rs | 24 +- 14 files changed, 745 insertions(+), 121 deletions(-) create mode 100644 codex-rs/codex-mcp/src/catalog.rs create mode 100644 codex-rs/codex-mcp/src/catalog_tests.rs diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs new file mode 100644 index 000000000..68f0a1b5a --- /dev/null +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -0,0 +1,333 @@ +use std::cmp::Reverse; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::collections::HashMap; + +use codex_config::McpServerConfig; + +/// The component that declared an MCP server registration. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum McpServerSource { + Plugin { plugin_id: String }, + Config, + Compatibility { id: String }, + Extension { id: String }, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] +enum RegistrationPrecedence { + Plugin(Reverse), + Config, + Compatibility, + Extension(usize), +} + +impl RegistrationPrecedence { + fn tier(self) -> u8 { + match self { + Self::Plugin(_) => 0, + Self::Config => 1, + Self::Compatibility => 2, + Self::Extension(_) => 3, + } + } +} + +/// One named MCP server declaration before source resolution. +#[derive(Clone, Debug, PartialEq)] +pub struct McpServerRegistration { + name: String, + source: McpServerSource, + config: McpServerConfig, + precedence: RegistrationPrecedence, +} + +impl McpServerRegistration { + pub fn from_config(name: String, config: McpServerConfig) -> Self { + Self::new( + name, + McpServerSource::Config, + config, + RegistrationPrecedence::Config, + ) + } + + pub fn from_plugin( + name: String, + plugin_id: String, + plugin_order: usize, + config: McpServerConfig, + ) -> Self { + Self::new( + name, + McpServerSource::Plugin { plugin_id }, + config, + RegistrationPrecedence::Plugin(Reverse(plugin_order)), + ) + } + + pub fn from_compatibility( + name: String, + id: impl Into, + config: McpServerConfig, + ) -> Self { + Self::new( + name, + McpServerSource::Compatibility { id: id.into() }, + config, + RegistrationPrecedence::Compatibility, + ) + } + + pub fn from_extension( + name: String, + id: impl Into, + contribution_order: usize, + config: McpServerConfig, + ) -> Self { + Self::new( + name, + McpServerSource::Extension { id: id.into() }, + config, + RegistrationPrecedence::Extension(contribution_order), + ) + } + + fn new( + name: String, + source: McpServerSource, + config: McpServerConfig, + precedence: RegistrationPrecedence, + ) -> Self { + Self { + name, + source, + config, + precedence, + } + } +} + +/// One side of an MCP server conflict, including whether it registers or +/// removes the server. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum McpServerConflictAction { + Register(McpServerSource), + Remove(McpServerSource), +} + +/// A same-tier name collision and the final outcome after all precedence is applied. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct McpServerConflict { + pub name: String, + pub outcome: McpServerConflictAction, + pub contenders: Vec, +} + +#[derive(Clone, Debug)] +enum CatalogAction { + Register(Box), + Remove { + name: String, + source: McpServerSource, + precedence: RegistrationPrecedence, + }, +} + +impl CatalogAction { + fn name(&self) -> &str { + match self { + Self::Register(registration) => ®istration.name, + Self::Remove { name, .. } => name, + } + } + + fn precedence(&self) -> RegistrationPrecedence { + match self { + Self::Register(registration) => registration.precedence, + Self::Remove { precedence, .. } => *precedence, + } + } + + fn conflict_action(&self) -> McpServerConflictAction { + match self { + Self::Register(registration) => { + McpServerConflictAction::Register(registration.source.clone()) + } + Self::Remove { source, .. } => McpServerConflictAction::Remove(source.clone()), + } + } +} + +/// Mutable inputs used to produce an immutable resolved catalog. +#[derive(Clone, Debug, Default)] +pub struct McpCatalogBuilder { + actions: Vec, + disabled_server_names: BTreeSet, +} + +impl McpCatalogBuilder { + pub fn register(&mut self, registration: McpServerRegistration) { + self.actions + .push(CatalogAction::Register(Box::new(registration))); + } + + /// Applies the legacy name-scoped disabled veto after source resolution. + pub fn disable(&mut self, name: String) { + self.disabled_server_names.insert(name); + } + + pub fn remove_compatibility(&mut self, name: String, id: impl Into) { + self.actions.push(CatalogAction::Remove { + name, + source: McpServerSource::Compatibility { id: id.into() }, + precedence: RegistrationPrecedence::Compatibility, + }); + } + + pub fn remove_extension( + &mut self, + name: String, + id: impl Into, + contribution_order: usize, + ) { + self.actions.push(CatalogAction::Remove { + name, + source: McpServerSource::Extension { id: id.into() }, + precedence: RegistrationPrecedence::Extension(contribution_order), + }); + } + + pub fn build(mut self) -> ResolvedMcpCatalog { + // Stable sorting makes action order the tie-breaker when precedence is equal. + self.actions.sort_by_key(CatalogAction::precedence); + + let mut winners = BTreeMap::::new(); + let mut actions_by_name_and_tier = BTreeMap::<(String, u8), Vec<&CatalogAction>>::new(); + for action in &self.actions { + winners.insert(action.name().to_string(), action.clone()); + actions_by_name_and_tier + .entry((action.name().to_string(), action.precedence().tier())) + .or_default() + .push(action); + } + + let mut conflicts = Vec::new(); + for ((name, _), actions) in actions_by_name_and_tier { + if actions.len() < 2 { + continue; + } + let Some(outcome) = winners.get(&name).map(CatalogAction::conflict_action) else { + continue; + }; + conflicts.push(McpServerConflict { + name, + outcome, + contenders: actions + .into_iter() + .map(CatalogAction::conflict_action) + .collect(), + }); + } + + let mut disabled_server_names = self.disabled_server_names; + let servers = winners + .into_iter() + .filter_map(|(name, action)| match action { + CatalogAction::Register(registration) => { + let mut registration = *registration; + // Effective disabled winners remain name-scoped vetoes for later overlays. + if !registration.config.enabled || disabled_server_names.contains(&name) { + registration.config.enabled = false; + disabled_server_names.insert(name.clone()); + } + Some(( + name, + ResolvedMcpServer { + source: registration.source, + config: registration.config, + }, + )) + } + CatalogAction::Remove { .. } => None, + }) + .collect(); + + ResolvedMcpCatalog { + actions: self.actions, + disabled_server_names, + servers, + conflicts, + } + } +} + +/// A single winning MCP registration. +#[derive(Clone, Debug, PartialEq)] +pub struct ResolvedMcpServer { + source: McpServerSource, + config: McpServerConfig, +} + +impl ResolvedMcpServer { + pub fn source(&self) -> &McpServerSource { + &self.source + } + + pub fn config(&self) -> &McpServerConfig { + &self.config + } +} + +/// Immutable result of MCP registration resolution. +#[derive(Clone, Debug, Default)] +pub struct ResolvedMcpCatalog { + actions: Vec, + disabled_server_names: BTreeSet, + servers: BTreeMap, + conflicts: Vec, +} + +impl ResolvedMcpCatalog { + pub fn builder() -> McpCatalogBuilder { + McpCatalogBuilder::default() + } + + pub fn to_builder(&self) -> McpCatalogBuilder { + McpCatalogBuilder { + actions: self.actions.clone(), + disabled_server_names: self.disabled_server_names.clone(), + } + } + + pub fn server(&self, name: &str) -> Option<&ResolvedMcpServer> { + self.servers.get(name) + } + + pub fn configured_servers(&self) -> HashMap { + self.servers + .iter() + .map(|(name, server)| (name.clone(), server.config.clone())) + .collect() + } + + pub fn plugin_ids_by_server_name(&self) -> HashMap { + self.servers + .iter() + .filter_map(|(name, server)| match server.source() { + McpServerSource::Plugin { plugin_id } => Some((name.clone(), plugin_id.clone())), + McpServerSource::Config + | McpServerSource::Compatibility { .. } + | McpServerSource::Extension { .. } => None, + }) + .collect() + } + + pub fn conflicts(&self) -> &[McpServerConflict] { + &self.conflicts + } +} + +#[cfg(test)] +#[path = "catalog_tests.rs"] +mod tests; diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs new file mode 100644 index 000000000..6909959a5 --- /dev/null +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -0,0 +1,257 @@ +use std::collections::HashMap; +use std::time::Duration; + +use codex_config::AppToolApproval; +use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; +use codex_config::McpServerConfig; +use codex_config::McpServerToolConfig; +use codex_config::McpServerTransportConfig; +use pretty_assertions::assert_eq; + +use super::McpServerConflict; +use super::McpServerConflictAction; +use super::McpServerRegistration; +use super::McpServerSource; +use super::ResolvedMcpCatalog; + +fn server(url: &str) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: url.to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: true, + supports_parallel_tool_calls: true, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(7)), + tool_timeout_sec: Some(Duration::from_secs(11)), + default_tools_approval_mode: Some(AppToolApproval::Prompt), + enabled_tools: Some(vec!["read".to_string()]), + disabled_tools: Some(vec!["write".to_string()]), + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::from([( + "read".to_string(), + McpServerToolConfig { + approval_mode: Some(AppToolApproval::Approve), + }, + )]), + } +} + +fn plugin_source(plugin_id: &str) -> McpServerSource { + McpServerSource::Plugin { + plugin_id: plugin_id.to_string(), + } +} + +fn compatibility_source(id: &str) -> McpServerSource { + McpServerSource::Compatibility { id: id.to_string() } +} + +fn extension_source(id: &str) -> McpServerSource { + McpServerSource::Extension { id: id.to_string() } +} + +fn register(source: McpServerSource) -> McpServerConflictAction { + McpServerConflictAction::Register(source) +} + +fn remove(source: McpServerSource) -> McpServerConflictAction { + McpServerConflictAction::Remove(source) +} + +#[test] +fn source_precedence_preserves_the_winning_registration() { + let extension = server("https://extension.example/mcp"); + let mut plugin = server("https://plugin.example/mcp"); + plugin.enabled = false; + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_extension( + "docs".to_string(), + "hosted", + /*contribution_order*/ 0, + extension.clone(), + )); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + "plugin@test".to_string(), + /*plugin_order*/ 0, + plugin, + )); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + "other-plugin@test".to_string(), + /*plugin_order*/ 1, + server("https://other-plugin.example/mcp"), + )); + builder.register(McpServerRegistration::from_compatibility( + "docs".to_string(), + "legacy", + server("https://compatibility.example/mcp"), + )); + builder.register(McpServerRegistration::from_config( + "docs".to_string(), + server("https://config.example/mcp"), + )); + + let catalog = builder.build(); + let resolved = catalog.server("docs").expect("resolved server"); + + assert_eq!( + resolved.source(), + &McpServerSource::Extension { + id: "hosted".to_string(), + } + ); + assert_eq!(resolved.config(), &extension); + assert!(catalog.plugin_ids_by_server_name().is_empty()); + assert_eq!( + catalog.conflicts(), + &[McpServerConflict { + name: "docs".to_string(), + outcome: register(extension_source("hosted")), + contenders: vec![ + register(plugin_source("other-plugin@test")), + register(plugin_source("plugin@test")), + ], + }] + ); +} + +#[test] +fn disabled_veto_only_disables_the_winning_registration() { + let extension = server("https://extension.example/mcp"); + let mut expected = extension.clone(); + expected.enabled = false; + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_extension( + "docs".to_string(), + "hosted", + /*contribution_order*/ 0, + extension, + )); + builder.disable("docs".to_string()); + + let actual = builder + .build() + .server("docs") + .expect("resolved server") + .config() + .clone(); + + assert_eq!(actual, expected); +} + +#[test] +fn disabled_winner_remains_a_veto_when_the_catalog_is_extended() { + let mut disabled = server("https://config.example/mcp"); + disabled.enabled = false; + let mut expected = server("https://extension.example/mcp"); + expected.enabled = false; + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_config( + "docs".to_string(), + disabled, + )); + let mut builder = builder.build().to_builder(); + builder.register(McpServerRegistration::from_extension( + "docs".to_string(), + "hosted", + /*contribution_order*/ 0, + server("https://extension.example/mcp"), + )); + + let resolved = builder.build(); + + assert_eq!( + resolved.server("docs"), + Some(&super::ResolvedMcpServer { + source: extension_source("hosted"), + config: expected, + }) + ); +} + +#[test] +fn earlier_plugin_wins_with_an_explicit_conflict() { + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + "alpha@test".to_string(), + /*plugin_order*/ 0, + server("https://alpha.example/mcp"), + )); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + "beta@test".to_string(), + /*plugin_order*/ 1, + server("https://beta.example/mcp"), + )); + + let catalog = builder.build(); + + assert_eq!( + catalog.plugin_ids_by_server_name(), + HashMap::from([("docs".to_string(), "alpha@test".to_string())]) + ); + assert_eq!( + catalog.conflicts(), + &[McpServerConflict { + name: "docs".to_string(), + outcome: register(plugin_source("alpha@test")), + contenders: vec![ + register(plugin_source("beta@test")), + register(plugin_source("alpha@test")), + ], + }] + ); +} + +#[test] +fn equal_precedence_uses_insertion_order_not_source_identity() { + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_compatibility( + "docs".to_string(), + "z-first", + server("https://first.example/mcp"), + )); + builder.register(McpServerRegistration::from_compatibility( + "docs".to_string(), + "a-second", + server("https://second.example/mcp"), + )); + + let catalog = builder.build(); + + assert_eq!( + catalog.server("docs"), + Some(&super::ResolvedMcpServer { + source: compatibility_source("a-second"), + config: server("https://second.example/mcp"), + }) + ); + let mut builder = catalog.to_builder(); + builder.remove_compatibility("docs".to_string(), "remove-last"); + + let catalog = builder.build(); + + assert_eq!(catalog.server("docs"), None); + assert_eq!( + catalog.conflicts(), + &[McpServerConflict { + name: "docs".to_string(), + outcome: remove(compatibility_source("remove-last")), + contenders: vec![ + register(compatibility_source("z-first")), + register(compatibility_source("a-second")), + remove(compatibility_source("remove-last")), + ], + }] + ); +} diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 195e8614e..a270eb4e7 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -11,6 +11,14 @@ pub use runtime::McpRuntimeContext; pub use runtime::SandboxState; pub use tools::ToolInfo; +pub use catalog::McpCatalogBuilder; +pub use catalog::McpServerConflict; +pub use catalog::McpServerConflictAction; +pub use catalog::McpServerRegistration; +pub use catalog::McpServerSource; +pub use catalog::ResolvedMcpCatalog; +pub use catalog::ResolvedMcpServer; + pub use mcp::CODEX_APPS_MCP_SERVER_NAME; pub use mcp::McpConfig; pub use mcp::ToolPluginProvenance; @@ -57,6 +65,7 @@ pub use mcp::qualified_mcp_tool_name_prefix; pub use tools::declared_openai_file_input_param_names; pub(crate) mod auth_elicitation; +mod catalog; pub(crate) mod codex_apps; pub(crate) mod connection_manager; pub(crate) mod elicitation; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index e6b17b14c..eca8a28c5 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -37,6 +37,7 @@ use rmcp::model::ReadResourceResult; use serde_json::Value; use tokio_util::sync::CancellationToken; +use crate::ResolvedMcpCatalog; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; use crate::runtime::McpRuntimeContext; @@ -135,13 +136,8 @@ pub struct McpConfig { pub prefix_mcp_tool_names: bool, /// Client-side elicitation capabilities advertised during MCP initialization. pub client_elicitation_capability: ElicitationCapability, - /// Materialized MCP servers keyed by server name. - /// - /// A host may add compatibility built-ins and extension overlays before - /// calling runtime entry points in this crate. - pub configured_mcp_servers: HashMap, - /// Winning plugin owner for plugin-provided MCP servers, keyed by server name. - pub plugin_ids_by_mcp_server_name: HashMap, + /// Resolved MCP registrations keyed by logical server name. + pub mcp_server_catalog: ResolvedMcpCatalog, /// Plugin metadata used to attribute MCP tools/connectors to plugin display names. pub plugin_capability_summaries: Vec, } @@ -176,6 +172,7 @@ impl ToolPluginProvenance { fn from_config(config: &McpConfig) -> Self { let mut tool_plugin_provenance = Self::default(); + let plugin_ids_by_mcp_server_name = config.mcp_server_catalog.plugin_ids_by_server_name(); for plugin in &config.plugin_capability_summaries { for connector_id in &plugin.app_connector_ids { tool_plugin_provenance @@ -185,7 +182,9 @@ impl ToolPluginProvenance { .push(plugin.display_name.clone()); } - for server_name in &plugin.mcp_server_names { + for server_name in plugin.mcp_server_names.iter().filter(|server_name| { + plugin_ids_by_mcp_server_name.get(*server_name) == Some(&plugin.config_name) + }) { tool_plugin_provenance .plugin_display_names_by_mcp_server_name .entry(server_name.clone()) @@ -206,8 +205,7 @@ impl ToolPluginProvenance { plugin_names.sort_unstable(); plugin_names.dedup(); } - tool_plugin_provenance.plugin_ids_by_mcp_server_name = - config.plugin_ids_by_mcp_server_name.clone(); + tool_plugin_provenance.plugin_ids_by_mcp_server_name = plugin_ids_by_mcp_server_name; tool_plugin_provenance } @@ -218,7 +216,7 @@ pub fn host_owned_codex_apps_enabled(config: &McpConfig, auth: Option<&CodexAuth } pub fn configured_mcp_servers(config: &McpConfig) -> HashMap { - config.configured_mcp_servers.clone() + config.mcp_server_catalog.configured_servers() } pub fn effective_mcp_servers( diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 767f20611..111c810a1 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::McpServerRegistration; use codex_config::Constrained; use codex_config::types::AppToolApproval; use codex_login::CodexAuth; @@ -28,8 +29,7 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig { apps_enabled: false, prefix_mcp_tool_names: true, client_elicitation_capability: ElicitationCapability::default(), - configured_mcp_servers: HashMap::new(), - plugin_ids_by_mcp_server_name: HashMap::new(), + mcp_server_catalog: ResolvedMcpCatalog::default(), plugin_capability_summaries: Vec::new(), } } @@ -122,16 +122,24 @@ fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() { #[test] fn tool_plugin_provenance_collects_app_and_mcp_sources() { let mut config = test_mcp_config(PathBuf::new()); - config.plugin_ids_by_mcp_server_name = - HashMap::from([("alpha".to_string(), "alpha@test".to_string())]); + let mut catalog = ResolvedMcpCatalog::builder(); + catalog.register(McpServerRegistration::from_plugin( + "alpha".to_string(), + "alpha@test".to_string(), + /*plugin_order*/ 0, + codex_apps_mcp_server_config("https://alpha.example", /*apps_mcp_product_sku*/ None), + )); + config.mcp_server_catalog = catalog.build(); config.plugin_capability_summaries = vec![ PluginCapabilitySummary { + config_name: "alpha@test".to_string(), display_name: "alpha-plugin".to_string(), app_connector_ids: vec![AppConnectorId("connector_example".to_string())], mcp_server_names: vec!["alpha".to_string()], ..PluginCapabilitySummary::default() }, PluginCapabilitySummary { + config_name: "beta@test".to_string(), display_name: "beta-plugin".to_string(), app_connector_ids: vec![ AppConnectorId("connector_example".to_string()), @@ -156,10 +164,10 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { vec!["beta-plugin".to_string()], ), ]), - plugin_display_names_by_mcp_server_name: HashMap::from([ - ("alpha".to_string(), vec!["alpha-plugin".to_string()]), - ("beta".to_string(), vec!["beta-plugin".to_string()]), - ]), + plugin_display_names_by_mcp_server_name: HashMap::from([( + "alpha".to_string(), + vec!["alpha-plugin".to_string()], + )]), plugin_ids_by_mcp_server_name: HashMap::from([( "alpha".to_string(), "alpha@test".to_string(), @@ -235,7 +243,8 @@ async fn effective_mcp_servers_preserve_runtime_servers() { config.apps_enabled = true; let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); - config.configured_mcp_servers.insert( + let mut catalog = ResolvedMcpCatalog::builder(); + catalog.register(McpServerRegistration::from_config( "sample".to_string(), McpServerConfig { transport: McpServerTransportConfig::StreamableHttp { @@ -259,8 +268,8 @@ async fn effective_mcp_servers_preserve_runtime_servers() { oauth_resource: None, tools: HashMap::new(), }, - ); - config.configured_mcp_servers.insert( + )); + catalog.register(McpServerRegistration::from_config( "docs".to_string(), McpServerConfig { transport: McpServerTransportConfig::StreamableHttp { @@ -284,14 +293,15 @@ async fn effective_mcp_servers_preserve_runtime_servers() { oauth_resource: None, tools: HashMap::new(), }, - ); - config.configured_mcp_servers.insert( + )); + catalog.register(McpServerRegistration::from_config( CODEX_APPS_MCP_SERVER_NAME.to_string(), codex_apps_mcp_server_config( &config.chatgpt_base_url, config.apps_mcp_product_sku.as_deref(), ), - ); + )); + config.mcp_server_catalog = catalog.build(); let effective = effective_mcp_servers(&config, Some(&auth)); diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index ba266f70c..9072cff3c 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4347,13 +4347,14 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config() .await?; let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); let mcp_config = config.to_mcp_config(&plugins_manager).await; + let configured_servers = mcp_config.mcp_server_catalog.configured_servers(); assert_eq!( - mcp_config.configured_mcp_servers.get("sample"), + configured_servers.get("sample"), Some(&http_mcp("https://sample.example/mcp")) ); assert_eq!( - mcp_config.plugin_ids_by_mcp_server_name, + mcp_config.mcp_server_catalog.plugin_ids_by_server_name(), HashMap::from([("sample".to_string(), "sample@test".to_string())]) ); @@ -4403,12 +4404,18 @@ enabled = true .await?; let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); let mcp_config = config.to_mcp_config(&plugins_manager).await; + let configured_servers = mcp_config.mcp_server_catalog.configured_servers(); assert_eq!( - mcp_config.configured_mcp_servers.get("sample"), + configured_servers.get("sample"), Some(&http_mcp("https://user.example/mcp")) ); - assert!(mcp_config.plugin_ids_by_mcp_server_name.is_empty()); + assert!( + mcp_config + .mcp_server_catalog + .plugin_ids_by_server_name() + .is_empty() + ); Ok(()) } @@ -4465,17 +4472,16 @@ url = "https://sample.example/mcp" .await?; let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); let mcp_config = config.to_mcp_config(&plugins_manager).await; + let configured_servers = mcp_config.mcp_server_catalog.configured_servers(); assert_eq!( - mcp_config - .configured_mcp_servers + configured_servers .get("sample") .map(|server| (server.enabled, server.disabled_reason.clone())), Some((true, None)) ); assert_eq!( - mcp_config - .configured_mcp_servers + configured_servers .get("unlisted") .map(|server| (server.enabled, server.disabled_reason.clone())), Some(( @@ -4538,10 +4544,10 @@ enabled = true .await?; let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); let mcp_config = config.to_mcp_config(&plugins_manager).await; + let configured_servers = mcp_config.mcp_server_catalog.configured_servers(); assert_eq!( - mcp_config - .configured_mcp_servers + configured_servers .get("sample") .map(|server| (server.enabled, server.disabled_reason.clone())), Some(( diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index bb7b0990e..1c3942853 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -69,6 +69,8 @@ use codex_git_utils::resolve_root_git_project_for_trust; use codex_install_context::InstallContext; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; +use codex_mcp::McpServerRegistration; +use codex_mcp::ResolvedMcpCatalog; use codex_memories_read::memory_root; use codex_model_provider_info::LEGACY_OLLAMA_CHAT_PROVIDER_ID; use codex_model_provider_info::ModelProviderInfo; @@ -112,7 +114,6 @@ use serde::Serialize; use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; -use std::collections::hash_map::Entry; use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; @@ -1391,12 +1392,18 @@ impl Config { ) -> McpConfig { let plugins_input = self.plugins_config_input(); let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await; - let mut configured_mcp_servers = self.mcp_servers.get().clone(); - let mut plugin_ids_by_mcp_server_name = HashMap::new(); - for plugin in loaded_plugins + let mut catalog = ResolvedMcpCatalog::builder(); + let empty_mcp_allowlist = self + .config_layer_stack + .requirements() + .mcp_servers + .as_ref() + .filter(|requirements| requirements.value.is_empty()); + for (plugin_order, plugin) in loaded_plugins .plugins() .iter() .filter(|plugin| plugin.is_active()) + .enumerate() { let mut plugin_mcp_servers = plugin.mcp_servers.clone(); filter_plugin_mcp_servers_by_requirements( @@ -1404,22 +1411,22 @@ impl Config { &mut plugin_mcp_servers, self.config_layer_stack.requirements().plugins.as_ref(), ); + filter_mcp_servers_by_requirements(&mut plugin_mcp_servers, empty_mcp_allowlist); for (name, plugin_server) in plugin_mcp_servers { - if let Entry::Vacant(entry) = configured_mcp_servers.entry(name.clone()) { - entry.insert(plugin_server); - plugin_ids_by_mcp_server_name.insert(name, plugin.config_name.clone()); - } + catalog.register(McpServerRegistration::from_plugin( + name, + plugin.config_name.clone(), + plugin_order, + plugin_server, + )); } } - if let Some(mcp_requirements) = self.config_layer_stack.requirements().mcp_servers.as_ref() - && mcp_requirements.value.is_empty() - { - // A present empty allowlist bans configurable MCPs, including plugin MCPs merged - // above. - filter_mcp_servers_by_requirements(&mut configured_mcp_servers, Some(mcp_requirements)); + for (name, server) in self.mcp_servers.get() { + catalog.register(McpServerRegistration::from_config( + name.clone(), + server.clone(), + )); } - plugin_ids_by_mcp_server_name - .retain(|server_name, _| configured_mcp_servers.contains_key(server_name)); McpConfig { chatgpt_base_url: self.chatgpt_base_url.clone(), @@ -1446,8 +1453,7 @@ impl Config { // indicates this should be an empty object. ElicitationCapability::default() }, - configured_mcp_servers, - plugin_ids_by_mcp_server_name, + mcp_server_catalog: catalog.build(), plugin_capability_summaries: loaded_plugins.capability_summaries().to_vec(), } } diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index d8df72855..8ccf51ef5 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -43,6 +43,7 @@ use codex_mcp::codex_apps_tools_cache_key; use codex_mcp::compute_auth_statuses; use codex_mcp::effective_mcp_servers; use codex_mcp::host_owned_codex_apps_enabled; +use codex_mcp::tool_plugin_provenance; const CONNECTORS_READY_TIMEOUT_ON_EMPTY_TOOLS: Duration = Duration::from_secs(30); @@ -251,7 +252,8 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( }); } let cache_key = accessible_connectors_cache_key(config, auth.as_ref()); - let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config).await; + let mcp_config = mcp_manager.runtime_config(config).await; + let tool_plugin_provenance = tool_plugin_provenance(&mcp_config); if !force_refetch && let Some(cached_connectors) = read_cached_accessible_connectors(&cache_key) { let cached_connectors = codex_connectors::filter::filter_disallowed_connectors( @@ -265,7 +267,6 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( }); } - let mcp_config = mcp_manager.runtime_config(config).await; let mut mcp_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); mcp_servers.retain(|name, _| name == CODEX_APPS_MCP_SERVER_NAME); let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(&mcp_config, auth.as_ref()); diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index abd28ec61..5af247b69 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -10,11 +10,12 @@ use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::EffectiveMcpServer; use codex_mcp::McpConfig; -use codex_mcp::ToolPluginProvenance; +use codex_mcp::McpServerRegistration; use codex_mcp::codex_apps_mcp_server_config; use codex_mcp::configured_mcp_servers; use codex_mcp::effective_mcp_servers; -use codex_mcp::tool_plugin_provenance as collect_tool_plugin_provenance; + +const LEGACY_CODEX_APPS_REGISTRATION_ID: &str = "legacy_codex_apps"; #[derive(Clone)] pub struct McpManager { @@ -45,32 +46,53 @@ impl McpManager { /// runtime-only extension overlays. pub async fn runtime_config(&self, config: &Config) -> McpConfig { let mut mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await; - let disabled_server_names = mcp_config - .configured_mcp_servers - .iter() - .filter(|(_, server)| !server.enabled) - .map(|(name, _)| name.clone()) - .collect::>(); + let mut catalog = mcp_config.mcp_server_catalog.to_builder(); if mcp_config.apps_enabled { - mcp_config.configured_mcp_servers.insert( + catalog.register(McpServerRegistration::from_compatibility( CODEX_APPS_MCP_SERVER_NAME.to_string(), + LEGACY_CODEX_APPS_REGISTRATION_ID, codex_apps_mcp_server_config( &mcp_config.chatgpt_base_url, mcp_config.apps_mcp_product_sku.as_deref(), ), - ); + )); } else { - mcp_config - .configured_mcp_servers - .remove(CODEX_APPS_MCP_SERVER_NAME); + catalog.remove_compatibility( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + LEGACY_CODEX_APPS_REGISTRATION_ID, + ); } - let contributions = self.contributions(config).await; - Self::apply_to_configured_servers(&contributions, &mut mcp_config.configured_mcp_servers); - for name in disabled_server_names { - if let Some(server) = mcp_config.configured_mcp_servers.get_mut(&name) { - server.enabled = false; + + let mut contribution_order = 0; + for contributor in self.extensions.mcp_server_contributors() { + for contribution in contributor.contribute(config).await { + match contribution { + McpServerContribution::Set { + name, + config: server_config, + } => catalog.register(McpServerRegistration::from_extension( + name, + contributor.id(), + contribution_order, + *server_config, + )), + McpServerContribution::Remove { name } => { + catalog.remove_extension(name, contributor.id(), contribution_order) + } + } + contribution_order += 1; } } + let catalog = catalog.build(); + for conflict in catalog.conflicts() { + tracing::warn!( + server = conflict.name, + outcome = ?conflict.outcome, + contenders = ?conflict.contenders, + "conflicting MCP server actions; using resolved catalog outcome" + ); + } + mcp_config.mcp_server_catalog = catalog; mcp_config } @@ -95,34 +117,4 @@ impl McpManager { let mcp_config = self.runtime_config(config).await; effective_mcp_servers(&mcp_config, auth) } - - /// Returns provenance for plugin-owned servers in the configured view. - pub async fn tool_plugin_provenance(&self, config: &Config) -> ToolPluginProvenance { - let mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await; - collect_tool_plugin_provenance(&mcp_config) - } - - async fn contributions(&self, config: &Config) -> Vec { - let mut contributions = Vec::new(); - for contributor in self.extensions.mcp_server_contributors() { - contributions.extend(contributor.contribute(config).await); - } - contributions - } - - fn apply_to_configured_servers( - contributions: &[McpServerContribution], - servers: &mut HashMap, - ) { - for contribution in contributions { - match contribution { - McpServerContribution::Set { name, config } => { - servers.insert(name.clone(), config.as_ref().clone()); - } - McpServerContribution::Remove { name } => { - servers.remove(name); - } - } - } - } } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 39333801c..7f52da219 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -294,11 +294,7 @@ impl Session { .mcp_manager .runtime_config(config.as_ref()) .await; - let tool_plugin_provenance = self - .services - .mcp_manager - .tool_plugin_provenance(config.as_ref()) - .await; + let tool_plugin_provenance = codex_mcp::tool_plugin_provenance(&mcp_config); let mcp_servers = effective_mcp_servers_from_configured(mcp_servers, &mcp_config, auth.as_ref()); let host_owned_codex_apps_enabled = diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index da8daf176..cce3e10c7 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -605,16 +605,16 @@ impl Session { let mcp_manager_for_mcp = Arc::clone(&mcp_manager); let auth_and_mcp_fut = async move { let auth = auth_manager_clone.auth().await; - let mcp_servers = mcp_manager_for_mcp - .effective_servers(&config_for_mcp, auth.as_ref()) - .await; + let mcp_config = mcp_manager_for_mcp.runtime_config(&config_for_mcp).await; + let mcp_servers = codex_mcp::effective_mcp_servers(&mcp_config, auth.as_ref()); + let tool_plugin_provenance = codex_mcp::tool_plugin_provenance(&mcp_config); let auth_statuses = compute_auth_statuses( mcp_servers.iter(), config_for_mcp.mcp_oauth_credentials_store_mode, auth.as_ref(), ) .await; - (auth, mcp_servers, auth_statuses) + (auth, mcp_servers, auth_statuses, tool_plugin_provenance) } .instrument(info_span!( "session_init.auth_mcp", @@ -637,7 +637,7 @@ impl Session { let ( thread_persistence_result, state_db_ctx, - (auth, mcp_servers, auth_statuses), + (auth, mcp_servers, auth_statuses, tool_plugin_provenance), plugin_skill_errors, ) = tokio::join!( thread_persistence_fut, @@ -1104,7 +1104,6 @@ impl Session { sess.send_event_raw(event).await; } - let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await; let host_owned_codex_apps_enabled = config .features .apps_enabled_for_auth(auth.as_ref().is_some_and(|auth| auth.uses_codex_backend())); diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index e93088b5d..efc22f5fd 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -48,6 +48,9 @@ pub type ExtensionFuture<'a, T> = Pin + Send + 'a>>; /// Plugin-owned servers and their provenance continue to be resolved by the /// plugin manager until that ownership moves into an extension explicitly. pub trait McpServerContributor: Send + Sync { + /// Stable identity used for registration provenance and conflict diagnostics. + fn id(&self) -> &'static str; + fn contribute<'a>(&'a self, config: &'a C) -> ExtensionFuture<'a, Vec>; } diff --git a/codex-rs/ext/mcp/src/lib.rs b/codex-rs/ext/mcp/src/lib.rs index 8568d5815..3843e0684 100644 --- a/codex-rs/ext/mcp/src/lib.rs +++ b/codex-rs/ext/mcp/src/lib.rs @@ -9,6 +9,10 @@ use codex_mcp::hosted_plugin_runtime_mcp_server_config; struct HostedPluginRuntimeExtension; impl McpServerContributor for HostedPluginRuntimeExtension { + fn id(&self) -> &'static str { + "hosted_plugin_runtime" + } + fn contribute<'a>( &'a self, config: &'a Config, diff --git a/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs b/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs index ea1b330ca..4a3902630 100644 --- a/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs +++ b/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs @@ -104,7 +104,7 @@ async fn legacy_fallback_overwrites_reserved_config_without_an_extension() -> Te } #[tokio::test] -async fn extension_can_remove_legacy_fallback_while_apps_are_enabled() -> TestResult { +async fn later_extension_can_remove_same_name_registration() -> TestResult { let codex_home = tempfile::tempdir()?; let config = ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) @@ -114,6 +114,7 @@ async fn extension_can_remove_legacy_fallback_while_apps_are_enabled() -> TestRe .await?; let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); let mut builder = ExtensionRegistryBuilder::new(); + codex_mcp_extension::install(&mut builder); builder.mcp_server_contributor(Arc::new(RemoveCodexApps)); let manager = McpManager::new_with_extensions( Arc::new(PluginsManager::new(config.codex_home.to_path_buf())), @@ -145,7 +146,7 @@ async fn hosted_apps_mcp_requires_chatgpt_auth() -> TestResult { } #[tokio::test] -async fn disabled_apps_remove_reserved_server_config() -> TestResult { +async fn disabled_apps_remove_reserved_server_config_for_all_hosts() -> TestResult { let codex_home = tempfile::tempdir()?; let config = ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) @@ -159,11 +160,16 @@ async fn disabled_apps_remove_reserved_server_config() -> TestResult { ]) .build() .await?; - let manager = installed_manager(&config); - - let servers = manager.runtime_servers(&config).await; - - assert!(!servers.contains_key(CODEX_APPS_MCP_SERVER_NAME)); + let managers = [ + installed_manager(&config), + McpManager::new(Arc::new(PluginsManager::new( + config.codex_home.to_path_buf(), + ))), + ]; + for manager in managers { + let servers = manager.runtime_servers(&config).await; + assert!(!servers.contains_key(CODEX_APPS_MCP_SERVER_NAME)); + } Ok(()) } @@ -179,6 +185,10 @@ fn installed_manager(config: &Config) -> McpManager { struct RemoveCodexApps; impl McpServerContributor for RemoveCodexApps { + fn id(&self) -> &'static str { + "remove_codex_apps" + } + fn contribute<'a>( &'a self, _config: &'a Config,