diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs index 68f0a1b5a..3a752c82b 100644 --- a/codex-rs/codex-mcp/src/catalog.rs +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -5,18 +5,58 @@ use std::collections::HashMap; use codex_config::McpServerConfig; +/// Plugin identity retained with an MCP registration for tool attribution. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct McpPluginAttribution { + plugin_id: String, + display_name: String, +} + +impl McpPluginAttribution { + pub fn new(plugin_id: String, display_name: String) -> Self { + Self { + plugin_id, + display_name, + } + } + + pub fn plugin_id(&self) -> &str { + &self.plugin_id + } + + pub fn display_name(&self) -> &str { + &self.display_name + } +} + /// The component that declared an MCP server registration. #[derive(Clone, Debug, PartialEq, Eq)] pub enum McpServerSource { - Plugin { plugin_id: String }, + /// A plugin discovered through the process-wide legacy plugin manager. + Plugin(McpPluginAttribution), + /// A plugin explicitly selected for this thread through a capability root. + SelectedPlugin(McpPluginAttribution), Config, - Compatibility { id: String }, - Extension { id: String }, + Compatibility { + id: String, + }, + Extension { + id: String, + }, +} + +impl McpServerSource { + fn disabled_registration_is_name_veto(&self) -> bool { + // A selected package's policy applies to its registration, not to a higher runtime source + // that happens to use the same logical server name. + !matches!(self, Self::SelectedPlugin(_)) + } } #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] enum RegistrationPrecedence { Plugin(Reverse), + SelectedPlugin(Reverse), Config, Compatibility, Extension(usize), @@ -26,9 +66,10 @@ impl RegistrationPrecedence { fn tier(self) -> u8 { match self { Self::Plugin(_) => 0, - Self::Config => 1, - Self::Compatibility => 2, - Self::Extension(_) => 3, + Self::SelectedPlugin(_) => 1, + Self::Config => 2, + Self::Compatibility => 3, + Self::Extension(_) => 4, } } } @@ -54,18 +95,33 @@ impl McpServerRegistration { pub fn from_plugin( name: String, - plugin_id: String, + attribution: McpPluginAttribution, plugin_order: usize, config: McpServerConfig, ) -> Self { Self::new( name, - McpServerSource::Plugin { plugin_id }, + McpServerSource::Plugin(attribution), config, RegistrationPrecedence::Plugin(Reverse(plugin_order)), ) } + /// Registers a thread-selected plugin above discovered plugins and below config. + pub fn from_selected_plugin( + name: String, + attribution: McpPluginAttribution, + selection_order: usize, + config: McpServerConfig, + ) -> Self { + Self::new( + name, + McpServerSource::SelectedPlugin(attribution), + config, + RegistrationPrecedence::SelectedPlugin(Reverse(selection_order)), + ) + } + pub fn from_compatibility( name: String, id: impl Into, @@ -236,10 +292,14 @@ impl McpCatalogBuilder { .filter_map(|(name, action)| match action { CatalogAction::Register(registration) => { let mut registration = *registration; - // Effective disabled winners remain name-scoped vetoes for later overlays. + let persist_disabled_name = + registration.source.disabled_registration_is_name_veto(); if !registration.config.enabled || disabled_server_names.contains(&name) { registration.config.enabled = false; - disabled_server_names.insert(name.clone()); + if persist_disabled_name { + // Preserve legacy disabled winners across later runtime overlays. + disabled_server_names.insert(name.clone()); + } } Some(( name, @@ -311,11 +371,15 @@ impl ResolvedMcpCatalog { .collect() } - pub fn plugin_ids_by_server_name(&self) -> HashMap { + /// Returns package attribution for each winning plugin-owned server. + pub fn plugin_attributions_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::Plugin(attribution) + | McpServerSource::SelectedPlugin(attribution) => { + Some((name.clone(), attribution.clone())) + } McpServerSource::Config | McpServerSource::Compatibility { .. } | McpServerSource::Extension { .. } => None, @@ -323,6 +387,13 @@ impl ResolvedMcpCatalog { .collect() } + /// Returns the names of winning servers supplied by thread-selected plugins. + pub(crate) fn selected_plugin_server_names(&self) -> impl Iterator { + self.servers.iter().filter_map(|(name, server)| { + matches!(server.source(), McpServerSource::SelectedPlugin(_)).then_some(name.as_str()) + }) + } + pub fn conflicts(&self) -> &[McpServerConflict] { &self.conflicts } diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs index 6909959a5..696876cc4 100644 --- a/codex-rs/codex-mcp/src/catalog_tests.rs +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -8,6 +8,7 @@ use codex_config::McpServerToolConfig; use codex_config::McpServerTransportConfig; use pretty_assertions::assert_eq; +use super::McpPluginAttribution; use super::McpServerConflict; use super::McpServerConflictAction; use super::McpServerRegistration; @@ -44,10 +45,16 @@ fn server(url: &str) -> McpServerConfig { } } +fn plugin(plugin_id: &str) -> McpPluginAttribution { + McpPluginAttribution::new(plugin_id.to_string(), plugin_id.to_string()) +} + fn plugin_source(plugin_id: &str) -> McpServerSource { - McpServerSource::Plugin { - plugin_id: plugin_id.to_string(), - } + McpServerSource::Plugin(plugin(plugin_id)) +} + +fn selected_plugin_source(plugin_id: &str) -> McpServerSource { + McpServerSource::SelectedPlugin(plugin(plugin_id)) } fn compatibility_source(id: &str) -> McpServerSource { @@ -69,8 +76,8 @@ fn remove(source: McpServerSource) -> McpServerConflictAction { #[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 plugin_server = server("https://plugin.example/mcp"); + plugin_server.enabled = false; let mut builder = ResolvedMcpCatalog::builder(); builder.register(McpServerRegistration::from_extension( "docs".to_string(), @@ -80,13 +87,13 @@ fn source_precedence_preserves_the_winning_registration() { )); builder.register(McpServerRegistration::from_plugin( "docs".to_string(), - "plugin@test".to_string(), + plugin("plugin@test"), /*plugin_order*/ 0, - plugin, + plugin_server, )); builder.register(McpServerRegistration::from_plugin( "docs".to_string(), - "other-plugin@test".to_string(), + plugin("other-plugin@test"), /*plugin_order*/ 1, server("https://other-plugin.example/mcp"), )); @@ -110,7 +117,7 @@ fn source_precedence_preserves_the_winning_registration() { } ); assert_eq!(resolved.config(), &extension); - assert!(catalog.plugin_ids_by_server_name().is_empty()); + assert!(catalog.plugin_attributions_by_server_name().is_empty()); assert_eq!( catalog.conflicts(), &[McpServerConflict { @@ -178,18 +185,50 @@ fn disabled_winner_remains_a_veto_when_the_catalog_is_extended() { ); } +#[test] +fn disabled_discovered_plugin_remains_a_veto_for_runtime_overlays() { + let mut disabled = server("https://plugin.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_plugin( + "docs".to_string(), + plugin("plugin@test"), + /*plugin_order*/ 0, + 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("alpha@test"), /*plugin_order*/ 0, server("https://alpha.example/mcp"), )); builder.register(McpServerRegistration::from_plugin( "docs".to_string(), - "beta@test".to_string(), + plugin("beta@test"), /*plugin_order*/ 1, server("https://beta.example/mcp"), )); @@ -197,8 +236,8 @@ fn earlier_plugin_wins_with_an_explicit_conflict() { let catalog = builder.build(); assert_eq!( - catalog.plugin_ids_by_server_name(), - HashMap::from([("docs".to_string(), "alpha@test".to_string())]) + catalog.plugin_attributions_by_server_name(), + HashMap::from([("docs".to_string(), plugin("alpha@test"))]) ); assert_eq!( catalog.conflicts(), @@ -213,6 +252,105 @@ fn earlier_plugin_wins_with_an_explicit_conflict() { ); } +#[test] +fn selected_plugins_override_discovered_plugins_but_not_config() { + let selected = server("https://selected-alpha.example/mcp"); + let mut discovered = server("https://local.example/mcp"); + discovered.enabled = false; + discovered.default_tools_approval_mode = Some(AppToolApproval::Auto); + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + plugin("local@test"), + /*plugin_order*/ 0, + discovered, + )); + builder.register(McpServerRegistration::from_selected_plugin( + "docs".to_string(), + plugin("selected-beta"), + /*selection_order*/ 1, + server("https://selected-beta.example/mcp"), + )); + builder.register(McpServerRegistration::from_selected_plugin( + "docs".to_string(), + plugin("selected-alpha"), + /*selection_order*/ 0, + selected.clone(), + )); + + let catalog = builder.build(); + + assert_eq!( + catalog.server("docs"), + Some(&super::ResolvedMcpServer { + source: selected_plugin_source("selected-alpha"), + config: selected, + }) + ); + assert_eq!( + catalog.plugin_attributions_by_server_name(), + HashMap::from([("docs".to_string(), plugin("selected-alpha"))]) + ); + assert_eq!( + catalog.conflicts(), + &[McpServerConflict { + name: "docs".to_string(), + outcome: register(selected_plugin_source("selected-alpha")), + contenders: vec![ + register(selected_plugin_source("selected-beta")), + register(selected_plugin_source("selected-alpha")), + ], + }] + ); + + let mut builder = catalog.to_builder(); + let configured = server("https://config.example/mcp"); + builder.register(McpServerRegistration::from_config( + "docs".to_string(), + configured.clone(), + )); + let catalog = builder.build(); + + assert_eq!( + catalog.server("docs"), + Some(&super::ResolvedMcpServer { + source: McpServerSource::Config, + config: configured, + }) + ); +} + +#[test] +fn disabled_selected_plugin_does_not_veto_runtime_overlays() { + let mut disabled = server("https://selected.example/mcp"); + disabled.enabled = false; + let extension = server("https://extension.example/mcp"); + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_selected_plugin( + "docs".to_string(), + plugin("selected"), + /*selection_order*/ 0, + disabled, + )); + let mut builder = builder.build().to_builder(); + builder.register(McpServerRegistration::from_extension( + "docs".to_string(), + "hosted", + /*contribution_order*/ 0, + extension.clone(), + )); + + let resolved = builder.build(); + + assert_eq!( + resolved.server("docs"), + Some(&super::ResolvedMcpServer { + source: extension_source("hosted"), + config: extension, + }) + ); +} + #[test] fn equal_precedence_uses_insertion_order_not_source_identity() { let mut builder = ResolvedMcpCatalog::builder(); diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index b7e318a39..359648656 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -381,6 +381,22 @@ impl McpConnectionManager { .plugin_id_for_mcp_server_name(server_name) } + pub fn is_selected_plugin_mcp_server(&self, server_name: &str) -> bool { + self.tool_plugin_provenance + .is_selected_plugin_mcp_server(server_name) + } + + pub fn tool_approval_mode( + &self, + server_name: &str, + tool_name: &str, + ) -> codex_config::AppToolApproval { + self.server_metadata + .get(server_name) + .map(|metadata| metadata.tool_approval_mode(tool_name)) + .unwrap_or_default() + } + pub fn is_host_owned_codex_apps_server(&self, server_name: &str) -> bool { self.host_owned_codex_apps_enabled && server_name == CODEX_APPS_MCP_SERVER_NAME } diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 508598d34..bfbc09c47 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -12,14 +12,18 @@ use crate::elicitation::elicitation_is_rejected_by_policy; use crate::rmcp_client::AsyncManagedClient; use crate::rmcp_client::ManagedClient; use crate::rmcp_client::StartupOutcomeError; +use crate::server::EffectiveMcpServer; +use crate::server::McpServerMetadata; use crate::server::McpServerOrigin; use crate::tools::ToolFilter; use crate::tools::ToolInfo; use crate::tools::filter_tools; use crate::tools::normalize_tools_for_model_with_prefix; use crate::tools::tool_with_model_visible_input_schema; +use codex_config::AppToolApproval; use codex_config::Constrained; use codex_config::McpServerConfig; +use codex_config::McpServerToolConfig; use codex_config::types::AuthKeyringBackendKind; use codex_exec_server::EnvironmentManager; use codex_protocol::ToolName; @@ -1128,6 +1132,8 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { "https://docs.example".to_string(), )), supports_parallel_tool_calls: true, + default_tools_approval_mode: None, + tool_approval_modes: HashMap::new(), }, ); manager.clients.insert( @@ -1150,6 +1156,28 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { assert_eq!(tool.server_origin.as_deref(), Some("https://docs.example")); } +#[test] +fn server_metadata_preserves_tool_approval_policy() { + let mut config = crate::codex_apps_mcp_server_config( + "https://docs.example", + /*apps_mcp_product_sku*/ None, + ); + config.default_tools_approval_mode = Some(AppToolApproval::Prompt); + config.tools.insert( + "search".to_string(), + McpServerToolConfig { + approval_mode: Some(AppToolApproval::Approve), + }, + ); + let metadata = McpServerMetadata::from(&EffectiveMcpServer::configured(config)); + + assert_eq!(metadata.tool_approval_mode("read"), AppToolApproval::Prompt); + assert_eq!( + metadata.tool_approval_mode("search"), + AppToolApproval::Approve + ); +} + #[tokio::test] async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index e50c24412..598aaeac8 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -12,6 +12,7 @@ pub use runtime::SandboxState; pub use tools::ToolInfo; pub use catalog::McpCatalogBuilder; +pub use catalog::McpPluginAttribution; pub use catalog::McpServerConflict; pub use catalog::McpServerConflictAction; pub use catalog::McpServerRegistration; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 1a0f900a2..7a585cb5d 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -12,6 +12,7 @@ pub use auth::should_retry_without_scopes; pub(crate) mod auth; use std::collections::HashMap; +use std::collections::HashSet; use std::env; use std::path::PathBuf; use std::time::Duration; @@ -141,7 +142,8 @@ pub struct McpConfig { pub client_elicitation_capability: ElicitationCapability, /// 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. + /// Plugin metadata used to attribute connector tools to plugin display names. + /// MCP registrations retain their own package attribution in the catalog. pub plugin_capability_summaries: Vec, } @@ -150,6 +152,7 @@ pub struct ToolPluginProvenance { plugin_display_names_by_connector_id: HashMap>, plugin_display_names_by_mcp_server_name: HashMap>, plugin_ids_by_mcp_server_name: HashMap, + selected_plugin_mcp_server_names: HashSet, } impl ToolPluginProvenance { @@ -173,9 +176,12 @@ impl ToolPluginProvenance { .map(String::as_str) } + pub(crate) fn is_selected_plugin_mcp_server(&self, server_name: &str) -> bool { + self.selected_plugin_mcp_server_names.contains(server_name) + } + 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 @@ -184,18 +190,31 @@ impl ToolPluginProvenance { .or_default() .push(plugin.display_name.clone()); } - - 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()) - .or_default() - .push(plugin.display_name.clone()); - } } + for (server_name, attribution) in config + .mcp_server_catalog + .plugin_attributions_by_server_name() + { + tool_plugin_provenance + .plugin_display_names_by_mcp_server_name + .insert( + server_name.clone(), + vec![attribution.display_name().to_string()], + ); + tool_plugin_provenance + .plugin_ids_by_mcp_server_name + .insert(server_name, attribution.plugin_id().to_string()); + } + tool_plugin_provenance + .selected_plugin_mcp_server_names + .extend( + config + .mcp_server_catalog + .selected_plugin_server_names() + .map(str::to_string), + ); + for plugin_names in tool_plugin_provenance .plugin_display_names_by_connector_id .values_mut() @@ -208,8 +227,6 @@ impl ToolPluginProvenance { plugin_names.sort_unstable(); plugin_names.dedup(); } - tool_plugin_provenance.plugin_ids_by_mcp_server_name = plugin_ids_by_mcp_server_name; - tool_plugin_provenance } } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index ecaccd484..31c31960c 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::McpPluginAttribution; use crate::McpServerRegistration; use codex_config::Constrained; use codex_config::types::AppToolApproval; @@ -13,6 +14,7 @@ use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; use pretty_assertions::assert_eq; use std::collections::HashMap; +use std::collections::HashSet; use std::path::PathBuf; fn test_mcp_config(codex_home: PathBuf) -> McpConfig { @@ -127,7 +129,7 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { let mut catalog = ResolvedMcpCatalog::builder(); catalog.register(McpServerRegistration::from_plugin( "alpha".to_string(), - "alpha@test".to_string(), + McpPluginAttribution::new("alpha@test".to_string(), "alpha-plugin".to_string()), /*plugin_order*/ 0, codex_apps_mcp_server_config("https://alpha.example", /*apps_mcp_product_sku*/ None), )); @@ -174,6 +176,7 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { "alpha".to_string(), "alpha@test".to_string(), )]), + selected_plugin_mcp_server_names: HashSet::new(), } ); assert_eq!( @@ -183,6 +186,47 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { assert_eq!(provenance.plugin_id_for_mcp_server_name("beta"), None); } +#[test] +fn selected_mcp_attribution_does_not_join_an_unrelated_local_summary() { + let mut config = test_mcp_config(PathBuf::new()); + let mut catalog = ResolvedMcpCatalog::builder(); + catalog.register(McpServerRegistration::from_selected_plugin( + "github".to_string(), + McpPluginAttribution::new( + "shared-plugin-id".to_string(), + "Executor GitHub".to_string(), + ), + /*selection_order*/ 0, + codex_apps_mcp_server_config("https://github.example", /*apps_mcp_product_sku*/ None), + )); + config.mcp_server_catalog = catalog.build(); + config.plugin_capability_summaries = vec![PluginCapabilitySummary { + config_name: "shared-plugin-id".to_string(), + display_name: "Local GitHub".to_string(), + mcp_server_names: vec!["github".to_string()], + ..PluginCapabilitySummary::default() + }]; + + let provenance = tool_plugin_provenance(&config); + + assert_eq!( + provenance, + ToolPluginProvenance { + plugin_display_names_by_connector_id: HashMap::new(), + plugin_display_names_by_mcp_server_name: HashMap::from([( + "github".to_string(), + vec!["Executor GitHub".to_string()], + )]), + plugin_ids_by_mcp_server_name: HashMap::from([( + "github".to_string(), + "shared-plugin-id".to_string(), + )]), + selected_plugin_mcp_server_names: HashSet::from(["github".to_string()]), + } + ); + assert!(provenance.is_selected_plugin_mcp_server("github")); +} + #[test] fn codex_apps_mcp_url_for_base_url_keeps_existing_paths() { assert_eq!( diff --git a/codex-rs/codex-mcp/src/server.rs b/codex-rs/codex-mcp/src/server.rs index d8fb5a11f..90952cbfa 100644 --- a/codex-rs/codex-mcp/src/server.rs +++ b/codex-rs/codex-mcp/src/server.rs @@ -1,3 +1,6 @@ +use std::collections::HashMap; + +use codex_config::AppToolApproval; use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; @@ -75,6 +78,18 @@ pub(crate) struct McpServerMetadata { pub pollutes_memory: bool, pub origin: Option, pub supports_parallel_tool_calls: bool, + pub default_tools_approval_mode: Option, + pub tool_approval_modes: HashMap, +} + +impl McpServerMetadata { + pub fn tool_approval_mode(&self, tool_name: &str) -> AppToolApproval { + self.tool_approval_modes + .get(tool_name) + .copied() + .or(self.default_tools_approval_mode) + .unwrap_or_default() + } } impl From<&EffectiveMcpServer> for McpServerMetadata { @@ -84,6 +99,16 @@ impl From<&EffectiveMcpServer> for McpServerMetadata { pollutes_memory: true, origin: McpServerOrigin::from_transport(&config.transport), supports_parallel_tool_calls: config.supports_parallel_tool_calls, + default_tools_approval_mode: config.default_tools_approval_mode, + tool_approval_modes: config + .tools + .iter() + .filter_map(|(name, config)| { + config + .approval_mode + .map(|approval_mode| (name.clone(), approval_mode)) + }) + .collect(), }, } } diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index ff2f7ef24..183756051 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4357,8 +4357,13 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config() Some(&http_mcp("https://sample.example/mcp")) ); assert_eq!( - mcp_config.mcp_server_catalog.plugin_ids_by_server_name(), - HashMap::from([("sample".to_string(), "sample@test".to_string())]) + mcp_config + .mcp_server_catalog + .plugin_attributions_by_server_name(), + HashMap::from([( + "sample".to_string(), + McpPluginAttribution::new("sample@test".to_string(), "sample".to_string()), + )]) ); Ok(()) @@ -4416,7 +4421,7 @@ enabled = true assert!( mcp_config .mcp_server_catalog - .plugin_ids_by_server_name() + .plugin_attributions_by_server_name() .is_empty() ); @@ -4424,7 +4429,7 @@ enabled = true } #[tokio::test] -async fn to_mcp_config_applies_plugin_mcp_cloud_config_bundle() -> anyhow::Result<()> { +async fn selected_plugin_wins_after_discovered_plugin_requirements() -> anyhow::Result<()> { let codex_home = TempDir::new()?; let plugin_root = codex_home .path() @@ -4497,6 +4502,36 @@ url = "https://sample.example/mcp" }) )) ); + + let selected = http_mcp("https://selected.example/mcp"); + let mcp_config = config + .to_mcp_config_with_plugin_registrations( + &plugins_manager, + [McpServerRegistration::from_selected_plugin( + "unlisted".to_string(), + McpPluginAttribution::new( + "selected-root".to_string(), + "Selected Plugin".to_string(), + ), + /*selection_order*/ 0, + selected.clone(), + )], + ) + .await; + + assert_eq!( + mcp_config + .mcp_server_catalog + .server("unlisted") + .map(|server| (server.source().clone(), server.config().clone())), + Some(( + codex_mcp::McpServerSource::SelectedPlugin(McpPluginAttribution::new( + "selected-root".to_string(), + "Selected Plugin".to_string(), + )), + selected, + )) + ); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6cf7b9a18..25da826b2 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -70,6 +70,7 @@ 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::McpPluginAttribution; use codex_mcp::McpServerRegistration; use codex_mcp::ResolvedMcpCatalog; use codex_memories_read::memory_root; @@ -1396,19 +1397,45 @@ impl Config { ) } - pub async fn to_mcp_config( + /// Applies managed MCP requirements to servers supplied by one plugin. + pub fn apply_plugin_mcp_server_requirements( &self, - plugins_manager: &codex_core_plugins::PluginsManager, - ) -> McpConfig { - let plugins_input = self.plugins_config_input(); - let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await; - let mut catalog = ResolvedMcpCatalog::builder(); + plugin_id: &str, + mcp_servers: &mut HashMap, + ) { + filter_plugin_mcp_servers_by_requirements( + plugin_id, + mcp_servers, + self.config_layer_stack.requirements().plugins.as_ref(), + ); let empty_mcp_allowlist = self .config_layer_stack .requirements() .mcp_servers .as_ref() .filter(|requirements| requirements.value.is_empty()); + filter_mcp_servers_by_requirements(mcp_servers, empty_mcp_allowlist); + } + + pub async fn to_mcp_config( + &self, + plugins_manager: &codex_core_plugins::PluginsManager, + ) -> McpConfig { + self.to_mcp_config_with_plugin_registrations( + plugins_manager, + std::iter::empty::(), + ) + .await + } + + pub(crate) async fn to_mcp_config_with_plugin_registrations( + &self, + plugins_manager: &codex_core_plugins::PluginsManager, + additional_plugin_registrations: impl IntoIterator, + ) -> McpConfig { + let plugins_input = self.plugins_config_input(); + let loaded_plugins = plugins_manager.plugins_for_config(&plugins_input).await; + let mut catalog = ResolvedMcpCatalog::builder(); for (plugin_order, plugin) in loaded_plugins .plugins() .iter() @@ -1416,21 +1443,23 @@ impl Config { .enumerate() { let mut plugin_mcp_servers = plugin.mcp_servers.clone(); - filter_plugin_mcp_servers_by_requirements( - &plugin.config_name, - &mut plugin_mcp_servers, - self.config_layer_stack.requirements().plugins.as_ref(), + self.apply_plugin_mcp_server_requirements(&plugin.config_name, &mut plugin_mcp_servers); + let attribution = McpPluginAttribution::new( + plugin.config_name.clone(), + plugin.display_name().to_string(), ); - filter_mcp_servers_by_requirements(&mut plugin_mcp_servers, empty_mcp_allowlist); for (name, plugin_server) in plugin_mcp_servers { catalog.register(McpServerRegistration::from_plugin( name, - plugin.config_name.clone(), + attribution.clone(), plugin_order, plugin_server, )); } } + for registration in additional_plugin_registrations { + catalog.register(registration); + } for (name, server) in self.mcp_servers.get() { catalog.register(McpServerRegistration::from_config( name.clone(), diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index 4ff56fb62..90e6b01e7 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -12,6 +12,7 @@ use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::EffectiveMcpServer; use codex_mcp::McpConfig; +use codex_mcp::McpPluginAttribution; use codex_mcp::McpServerRegistration; use codex_mcp::codex_apps_mcp_server_config; use codex_mcp::configured_mcp_servers; @@ -19,6 +20,20 @@ use codex_mcp::effective_mcp_servers; const LEGACY_CODEX_APPS_REGISTRATION_ID: &str = "legacy_codex_apps"; +enum OrderedMcpOverlay { + Set { + contributor_id: &'static str, + contribution_order: usize, + name: String, + config: Box, + }, + Remove { + contributor_id: &'static str, + contribution_order: usize, + name: String, + }, +} + #[derive(Clone)] pub struct McpManager { plugins_manager: Arc, @@ -65,7 +80,58 @@ impl McpManager { config: &Config, thread_init: Option<&ExtensionDataInit>, ) -> McpConfig { - let mut mcp_config = config.to_mcp_config(self.plugins_manager.as_ref()).await; + let context = match thread_init { + Some(thread_init) => McpServerContributionContext::for_thread(config, thread_init), + None => McpServerContributionContext::global(config), + }; + let mut selected_plugin_registrations = Vec::new(); + let mut overlays = Vec::new(); + // A contributor can emit multiple ordered actions, so order each action globally rather + // than enumerating contributors. + let mut contribution_order = 0; + for contributor in self.extensions.mcp_server_contributors() { + for contribution in contributor.contribute(context).await { + match contribution { + McpServerContribution::Set { name, config } => { + overlays.push(OrderedMcpOverlay::Set { + contributor_id: contributor.id(), + contribution_order, + name, + config, + }); + } + McpServerContribution::SelectedPlugin { + name, + plugin_id, + plugin_display_name, + selection_order, + config, + } => selected_plugin_registrations.push( + McpServerRegistration::from_selected_plugin( + name, + McpPluginAttribution::new(plugin_id, plugin_display_name), + selection_order, + *config, + ), + ), + McpServerContribution::Remove { name } => { + overlays.push(OrderedMcpOverlay::Remove { + contributor_id: contributor.id(), + contribution_order, + name, + }); + } + } + contribution_order += 1; + } + } + + let mut mcp_config = config + .to_mcp_config_with_plugin_registrations( + self.plugins_manager.as_ref(), + selected_plugin_registrations, + ) + .await; let mut catalog = mcp_config.mcp_server_catalog.to_builder(); if mcp_config.apps_enabled { catalog.register(McpServerRegistration::from_compatibility( @@ -83,28 +149,24 @@ impl McpManager { ); } - let context = match thread_init { - Some(thread_init) => McpServerContributionContext::for_thread(config, thread_init), - None => McpServerContributionContext::global(config), - }; - let mut contribution_order = 0; - for contributor in self.extensions.mcp_server_contributors() { - for contribution in contributor.contribute(context).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; + for overlay in overlays { + match overlay { + OrderedMcpOverlay::Set { + contributor_id, + contribution_order, + name, + config, + } => catalog.register(McpServerRegistration::from_extension( + name, + contributor_id, + contribution_order, + *config, + )), + OrderedMcpOverlay::Remove { + contributor_id, + contribution_order, + name, + } => catalog.remove_extension(name, contributor_id, contribution_order), } } let catalog = catalog.build(); diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e3f7d5faf..e63a96e37 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -166,6 +166,15 @@ pub(crate) async fn handle_mcp_tool_call( }; let approval_mode = if server == CODEX_APPS_MCP_SERVER_NAME { app_tool_policy.approval + } else if let Some(approval_mode) = { + // Selected-plugin registrations are absent from config.toml and the legacy plugin manager, + // so their resolved catalog entry is the authoritative source for tool approval policy. + let manager = sess.services.mcp_connection_manager.load(); + manager + .is_selected_plugin_mcp_server(&server) + .then(|| manager.tool_approval_mode(&server, &tool_name)) + } { + approval_mode } else { custom_mcp_tool_approval_mode(sess.as_ref(), turn_context.as_ref(), &server, &tool_name) .await @@ -1168,8 +1177,16 @@ async fn maybe_request_mcp_tool_approval( } let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode); - let persistent_approval_key = - persistent_mcp_tool_approval_key(invocation, metadata, approval_mode); + let persistent_approval_key = if sess + .services + .mcp_connection_manager + .load() + .is_selected_plugin_mcp_server(&invocation.server) + { + None + } else { + persistent_mcp_tool_approval_key(invocation, metadata, approval_mode) + }; if let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 06a6374e7..a5670b053 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -448,8 +448,12 @@ async fn start_thread_seeds_extension_data_for_mcp_and_lifecycle_contributors() &selected_root.location; server.environment_id = environment_id.clone(); server.enabled = false; - vec![codex_extension_api::McpServerContribution::Set { - name: selected_root.id, + let plugin_id = selected_root.id; + vec![codex_extension_api::McpServerContribution::SelectedPlugin { + name: plugin_id.clone(), + plugin_display_name: plugin_id.clone(), + plugin_id, + selection_order: 0, config: Box::new(server), }] }) diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index 6edaf1c02..3a0fee49a 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -49,8 +49,8 @@ pub type ExtensionFuture<'a, T> = Pin + Send + 'a>>; /// Thread-scoped resolution exposes the host-seeded thread inputs; global /// resolution exposes none and must not imply a local fallback. Thread inputs /// are frozen for the runtime and do not include lifecycle-contributor state. -/// Plugin-owned servers and their provenance continue to be resolved by the -/// plugin manager until that ownership moves into an extension explicitly. +/// Auto-discovered plugin servers are resolved by the plugin manager. A +/// thread-selected plugin contribution must carry its own package provenance. pub trait McpServerContributor: Send + Sync { /// Stable identity used for registration provenance and conflict diagnostics. fn id(&self) -> &'static str; diff --git a/codex-rs/ext/extension-api/src/contributors/mcp.rs b/codex-rs/ext/extension-api/src/contributors/mcp.rs index 399cc0009..91e9cb459 100644 --- a/codex-rs/ext/extension-api/src/contributors/mcp.rs +++ b/codex-rs/ext/extension-api/src/contributors/mcp.rs @@ -58,6 +58,14 @@ pub enum McpServerContribution { name: String, config: Box, }, + /// Registers a server declared by a plugin selected for this thread. + SelectedPlugin { + name: String, + plugin_id: String, + plugin_display_name: String, + selection_order: usize, + config: Box, + }, /// Removes a named MCP server. Remove { name: String }, } diff --git a/codex-rs/plugin/src/load_outcome.rs b/codex-rs/plugin/src/load_outcome.rs index 5d731a8b5..767771d7c 100644 --- a/codex-rs/plugin/src/load_outcome.rs +++ b/codex-rs/plugin/src/load_outcome.rs @@ -34,6 +34,10 @@ impl LoadedPlugin { pub fn is_active(&self) -> bool { self.enabled && self.error.is_none() } + + pub fn display_name(&self) -> &str { + self.manifest_name.as_deref().unwrap_or(&self.config_name) + } } fn plugin_capability_summary_from_loaded( @@ -48,10 +52,7 @@ fn plugin_capability_summary_from_loaded( let summary = PluginCapabilitySummary { config_name: plugin.config_name.clone(), - display_name: plugin - .manifest_name - .clone() - .unwrap_or_else(|| plugin.config_name.clone()), + display_name: plugin.display_name().to_string(), description: prompt_safe_plugin_description(plugin.manifest_description.as_deref()), has_skills: plugin.has_enabled_skills, mcp_server_names,