From a66e0e9c4b2978121ed1cd4242f7f62dd027423f Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Mon, 18 May 2026 15:33:33 -0700 Subject: [PATCH] Include plugin id in plugin MCP tool metadata (#23353) Adding the id of the plugin that contains the MCP (if any) so we can apply filters at plugin level. ## Summary - carry the plugin owner into MCP runtime provenance - attach `plugin_id` to outbound plugin-backed MCP tool-call `_meta` - avoid misattributing user-configured MCP servers that shadow plugin server names ## Testing - `just fmt` - `just fix -p codex-mcp` - `just fix -p codex-core` - `cargo test -p codex-mcp` - `cargo test -p codex-core plugin_mcp_tool_call_request_meta_includes_plugin_id` - `cargo test -p codex-core to_mcp_config_omits_plugin_id_when_user_server_shadows_plugin_mcp` - `cargo test -p codex-core rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config` - `git diff --check` ## Notes - Attempted `cargo test -p codex-core`; it aborted in `agent::control::tests::resume_agent_from_rollout_skips_descendants_when_parent_resume_fails` with a stack overflow before the full suite completed. --- codex-rs/codex-mcp/src/connection_manager.rs | 8 +++ codex-rs/codex-mcp/src/mcp/mod.rs | 17 ++++-- codex-rs/codex-mcp/src/mcp/mod_tests.rs | 18 ++++++- codex-rs/core/src/config/config_tests.rs | 57 ++++++++++++++++++++ codex-rs/core/src/config/mod.rs | 10 +++- codex-rs/core/src/mcp_tool_call.rs | 21 +++++--- codex-rs/core/src/mcp_tool_call_tests.rs | 37 +++++++++++++ 7 files changed, 155 insertions(+), 13 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index e8dc36b9b..8144f5b22 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -71,6 +71,7 @@ use tracing::warn; pub struct McpConnectionManager { clients: HashMap, server_metadata: HashMap, + tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, elicitation_requests: ElicitationRequestManager, startup_cancellation_token: CancellationToken, @@ -91,6 +92,7 @@ impl McpConnectionManager { Self { clients: HashMap::new(), server_metadata: HashMap::new(), + tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, elicitation_requests: ElicitationRequestManager::new( approval_policy.value(), @@ -136,6 +138,11 @@ impl McpConnectionManager { .is_none_or(|metadata| metadata.pollutes_memory) } + pub fn plugin_id_for_mcp_server_name(&self, server_name: &str) -> Option<&str> { + self.tool_plugin_provenance + .plugin_id_for_mcp_server_name(server_name) + } + 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 } @@ -283,6 +290,7 @@ impl McpConnectionManager { let manager = Self { clients, server_metadata, + tool_plugin_provenance, host_owned_codex_apps_enabled, elicitation_requests: elicitation_requests.clone(), startup_cancellation_token: cancel_token.clone(), diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 2ce03c931..3d1969de0 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -138,6 +138,8 @@ pub struct McpConfig { /// /// Runtime-only additions are merged later by [`effective_mcp_servers`]. 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, /// Plugin metadata used to attribute MCP tools/connectors to plugin display names. pub plugin_capability_summaries: Vec, } @@ -146,6 +148,7 @@ pub struct McpConfig { 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, } impl ToolPluginProvenance { @@ -163,9 +166,15 @@ impl ToolPluginProvenance { .unwrap_or(&[]) } - fn from_capability_summaries(capability_summaries: &[PluginCapabilitySummary]) -> Self { + pub fn plugin_id_for_mcp_server_name(&self, server_name: &str) -> Option<&str> { + self.plugin_ids_by_mcp_server_name + .get(server_name) + .map(String::as_str) + } + + fn from_config(config: &McpConfig) -> Self { let mut tool_plugin_provenance = Self::default(); - for plugin in capability_summaries { + for plugin in &config.plugin_capability_summaries { for connector_id in &plugin.app_connector_ids { tool_plugin_provenance .plugin_display_names_by_connector_id @@ -195,6 +204,8 @@ 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 } @@ -244,7 +255,7 @@ pub fn effective_mcp_servers_from_configured( } pub fn tool_plugin_provenance(config: &McpConfig) -> ToolPluginProvenance { - ToolPluginProvenance::from_capability_summaries(&config.plugin_capability_summaries) + ToolPluginProvenance::from_config(config) } pub async fn read_mcp_resource( diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 99622d7f0..7566bf9bd 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -30,6 +30,7 @@ fn test_mcp_config(codex_home: PathBuf) -> McpConfig { apps_enabled: false, client_elicitation_capability: ElicitationCapability::default(), configured_mcp_servers: HashMap::new(), + plugin_ids_by_mcp_server_name: HashMap::new(), plugin_capability_summaries: Vec::new(), } } @@ -124,7 +125,10 @@ fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() { #[test] fn tool_plugin_provenance_collects_app_and_mcp_sources() { - let provenance = ToolPluginProvenance::from_capability_summaries(&[ + let mut config = test_mcp_config(PathBuf::new()); + config.plugin_ids_by_mcp_server_name = + HashMap::from([("alpha".to_string(), "alpha@test".to_string())]); + config.plugin_capability_summaries = vec![ PluginCapabilitySummary { display_name: "alpha-plugin".to_string(), app_connector_ids: vec![AppConnectorId("connector_example".to_string())], @@ -140,7 +144,8 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { mcp_server_names: vec!["beta".to_string()], ..PluginCapabilitySummary::default() }, - ]); + ]; + let provenance = tool_plugin_provenance(&config); assert_eq!( provenance, @@ -159,8 +164,17 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { ("alpha".to_string(), vec!["alpha-plugin".to_string()]), ("beta".to_string(), vec!["beta-plugin".to_string()]), ]), + plugin_ids_by_mcp_server_name: HashMap::from([( + "alpha".to_string(), + "alpha@test".to_string(), + )]), } ); + assert_eq!( + provenance.plugin_id_for_mcp_server_name("alpha"), + Some("alpha@test") + ); + assert_eq!(provenance.plugin_id_for_mcp_server_name("beta"), None); } #[test] diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 21d291b30..50b03b505 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -3849,6 +3849,63 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config() mcp_config.configured_mcp_servers.get("sample"), Some(&http_mcp("https://sample.example/mcp")) ); + assert_eq!( + mcp_config.plugin_ids_by_mcp_server_name, + HashMap::from([("sample".to_string(), "sample@test".to_string())]) + ); + + Ok(()) +} + +#[tokio::test] +async fn to_mcp_config_omits_plugin_id_when_user_server_shadows_plugin_mcp() -> anyhow::Result<()> { + let codex_home = TempDir::new()?; + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); + std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?; + std::fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample"}"#, + )?; + std::fs::write( + plugin_root.join(".mcp.json"), + r#"{ + "mcpServers": { + "sample": { + "type": "http", + "url": "https://plugin.example/mcp" + } + } +}"#, + )?; + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + r#" +[features] +plugins = true + +[mcp_servers.sample] +url = "https://user.example/mcp" + +[plugins."sample@test"] +enabled = true +"#, + )?; + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .build() + .await?; + let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf()); + let mcp_config = config.to_mcp_config(&plugins_manager).await; + + assert_eq!( + mcp_config.configured_mcp_servers.get("sample"), + Some(&http_mcp("https://user.example/mcp")) + ); + assert!(mcp_config.plugin_ids_by_mcp_server_name.is_empty()); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6eb66c454..583c2a42b 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -107,6 +107,7 @@ 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; @@ -1243,6 +1244,7 @@ impl Config { 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 .plugins() .iter() @@ -1255,7 +1257,10 @@ impl Config { self.config_layer_stack.requirements().plugins.as_ref(), ); for (name, plugin_server) in plugin_mcp_servers { - configured_mcp_servers.entry(name).or_insert(plugin_server); + 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()); + } } } if let Some(mcp_requirements) = self.config_layer_stack.requirements().mcp_servers.as_ref() @@ -1265,6 +1270,8 @@ impl Config { // above. filter_mcp_servers_by_requirements(&mut configured_mcp_servers, Some(mcp_requirements)); } + 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(), @@ -1292,6 +1299,7 @@ impl Config { ElicitationCapability::default() }, configured_mcp_servers, + plugin_ids_by_mcp_server_name, plugin_capability_summaries: loaded_plugins.capability_summaries().to_vec(), } } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 8f6e3afaf..902ede574 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -974,6 +974,7 @@ pub(crate) struct McpToolApprovalMetadata { connector_id: Option, connector_name: Option, connector_description: Option, + plugin_id: Option, tool_title: Option, tool_description: Option, mcp_app_resource_uri: Option, @@ -983,6 +984,7 @@ pub(crate) struct McpToolApprovalMetadata { const MCP_TOOL_OPENAI_OUTPUT_TEMPLATE_META_KEY: &str = "openai/outputTemplate"; const MCP_TOOL_UI_RESOURCE_URI_META_KEY: &str = "ui/resourceUri"; +const MCP_TOOL_PLUGIN_ID_META_KEY: &str = "plugin_id"; const MCP_TOOL_THREAD_ID_META_KEY: &str = "threadId"; async fn custom_mcp_tool_approval_mode( @@ -1068,6 +1070,12 @@ fn build_mcp_tool_call_request_meta( serde_json::Value::Object(codex_apps_meta), ); } + if let Some(plugin_id) = metadata.and_then(|metadata| metadata.plugin_id.as_ref()) { + request_meta.insert( + MCP_TOOL_PLUGIN_ID_META_KEY.to_string(), + serde_json::Value::String(plugin_id.clone()), + ); + } (!request_meta.is_empty()).then_some(serde_json::Value::Object(request_meta)) } @@ -1480,13 +1488,11 @@ pub(crate) async fn lookup_mcp_tool_metadata( server: &str, tool_name: &str, ) -> Option { - let tools = sess - .services - .mcp_connection_manager - .read() - .await - .list_all_tools() - .await; + let manager = sess.services.mcp_connection_manager.read().await; + let plugin_id = manager + .plugin_id_for_mcp_server_name(server) + .map(str::to_string); + let tools = manager.list_all_tools().await; let tool_info = tools .into_iter() .find(|tool_info| tool_info.server_name == server && tool_info.tool.name == tool_name)?; @@ -1519,6 +1525,7 @@ pub(crate) async fn lookup_mcp_tool_metadata( connector_id: tool_info.connector_id, connector_name: tool_info.connector_name, connector_description, + plugin_id, tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), mcp_app_resource_uri: get_mcp_app_resource_uri(tool_info.tool.meta.as_deref()), diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index d8b6326cd..25f19c87d 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -76,6 +76,7 @@ fn approval_metadata( connector_id: connector_id.map(str::to_string), connector_name: connector_name.map(str::to_string), connector_description: connector_description.map(str::to_string), + plugin_id: None, tool_title: tool_title.map(str::to_string), tool_description: tool_description.map(str::to_string), mcp_app_resource_uri: None, @@ -1099,6 +1100,29 @@ async fn mcp_tool_call_request_meta_includes_turn_started_at_unix_ms() { ); } +#[tokio::test] +async fn plugin_mcp_tool_call_request_meta_includes_plugin_id() { + let (_, turn_context) = make_session_and_context().await; + let expected_turn_metadata = turn_context + .turn_metadata_state + .current_meta_value_for_mcp_request(mcp_turn_metadata_context(&turn_context)) + .expect("turn metadata"); + let mut metadata = approval_metadata( + /*connector_id*/ None, /*connector_name*/ None, + /*connector_description*/ None, /*tool_title*/ None, + /*tool_description*/ None, + ); + metadata.plugin_id = Some("sample@test".to_string()); + + assert_eq!( + build_mcp_tool_call_request_meta(&turn_context, "sample", "call-plugin", Some(&metadata),), + Some(serde_json::json!({ + crate::X_CODEX_TURN_METADATA_HEADER: expected_turn_metadata, + MCP_TOOL_PLUGIN_ID_META_KEY: "sample@test", + })) + ); +} + #[tokio::test] async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps_meta() { let (_, turn_context) = make_session_and_context().await; @@ -1111,6 +1135,7 @@ async fn codex_apps_tool_call_request_meta_includes_turn_metadata_and_codex_apps connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), connector_description: Some("Manage events".to_string()), + plugin_id: None, tool_title: Some("Create Event".to_string()), tool_description: Some("Create a calendar event.".to_string()), mcp_app_resource_uri: None, @@ -1573,6 +1598,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: None, tool_description: None, mcp_app_resource_uri: None, @@ -2290,6 +2316,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, mcp_app_resource_uri: None, @@ -2363,6 +2390,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, mcp_app_resource_uri: None, @@ -2419,6 +2447,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Create entities".to_string()), tool_description: None, mcp_app_resource_uri: None, @@ -2554,6 +2583,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Create entities".to_string()), tool_description: None, mcp_app_resource_uri: None, @@ -2640,6 +2670,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Reads calendar data.".to_string()), mcp_app_resource_uri: None, @@ -2693,6 +2724,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Read Only Tool".to_string()), tool_description: None, mcp_app_resource_uri: None, @@ -2772,6 +2804,7 @@ async fn approve_mode_skips_arc_interrupt_for_model() { connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), connector_description: Some("Manage events".to_string()), + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), mcp_app_resource_uri: None, @@ -2839,6 +2872,7 @@ async fn custom_approve_mode_skips_arc_interrupt_for_model() { connector_id: None, connector_name: None, connector_description: None, + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), mcp_app_resource_uri: None, @@ -2906,6 +2940,7 @@ async fn approve_mode_skips_arc_interrupt_without_annotations() { connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), connector_description: Some("Manage events".to_string()), + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), mcp_app_resource_uri: None, @@ -2978,6 +3013,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() { connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), connector_description: Some("Manage events".to_string()), + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), mcp_app_resource_uri: None, @@ -3046,6 +3082,7 @@ async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() { connector_id: Some("calendar".to_string()), connector_name: Some("Calendar".to_string()), connector_description: Some("Manage events".to_string()), + plugin_id: None, tool_title: Some("Dangerous Tool".to_string()), tool_description: Some("Performs a risky action.".to_string()), mcp_app_resource_uri: None,