diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs index 5d3d473c04..03e0c001c1 100644 --- a/codex-rs/codex-mcp/src/catalog.rs +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -7,6 +7,7 @@ use codex_config::McpServerConfig; use codex_config::McpServerDisabledReason; use codex_config::RequirementSource; use codex_protocol::mcp_policy::EnvironmentMcpPolicy; +use codex_utils_path_uri::PathUri; use crate::CODEX_APPS_MCP_SERVER_NAME; @@ -16,6 +17,7 @@ pub struct McpPluginAttribution { plugin_id: String, display_name: String, agent_plugin: bool, + host_root: Option, } impl McpPluginAttribution { @@ -24,6 +26,7 @@ impl McpPluginAttribution { plugin_id, display_name, agent_plugin: false, + host_root: None, } } @@ -32,9 +35,16 @@ impl McpPluginAttribution { plugin_id, display_name, agent_plugin: true, + host_root: None, } } + /// Records the exact host-discovered plugin root. + pub fn with_host_root(mut self, host_root: PathUri) -> Self { + self.host_root = Some(host_root); + self + } + pub fn plugin_id(&self) -> &str { &self.plugin_id } @@ -46,6 +56,11 @@ impl McpPluginAttribution { pub fn is_agent_plugin(&self) -> bool { self.agent_plugin } + + /// Returns the host-discovered root captured with this server registration. + pub fn host_root(&self) -> Option<&PathUri> { + self.host_root.as_ref() + } } /// The component that declared an MCP server registration. diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs index 5c22336f59..ef41e110a8 100644 --- a/codex-rs/codex-mcp/src/catalog_tests.rs +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -9,6 +9,7 @@ use codex_config::McpServerToolConfig; use codex_config::McpServerTransportConfig; use codex_protocol::mcp_policy::EnvironmentMcpPolicy; use codex_protocol::mcp_policy::PluginMcpRequirements; +use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use crate::CODEX_APPS_MCP_SERVER_NAME; @@ -20,6 +21,7 @@ use super::McpServerConflictAction; use super::McpServerRegistration; use super::McpServerSource; use super::ResolvedMcpCatalog; +use super::ResolvedMcpServer; fn server(url: &str) -> McpServerConfig { McpServerConfig { @@ -85,6 +87,33 @@ fn remove(source: McpServerSource) -> McpServerConflictAction { McpServerConflictAction::Remove(source) } +#[test] +fn plugin_host_root_is_retained_in_catalog_identity() { + let original_root = PathUri::parse("file:///plugins/original").expect("valid plugin root URI"); + let replacement_root = + PathUri::parse("file:///plugins/replacement").expect("valid plugin root URI"); + let catalog_for_root = |root| { + let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_plugin( + "docs".to_string(), + plugin("plugin@test").with_host_root(root), + /*plugin_order*/ 0, + server("https://plugin.example/mcp"), + )); + builder.build() + }; + let original = catalog_for_root(original_root.clone()); + let replacement = catalog_for_root(replacement_root); + + let Some(McpServerSource::Plugin(attribution)) = + original.server("docs").map(ResolvedMcpServer::source) + else { + panic!("expected host-discovered plugin registration"); + }; + assert_eq!(attribution.host_root(), Some(&original_root)); + assert!(!original.has_same_servers(&replacement)); +} + #[test] fn source_precedence_preserves_the_winning_registration() { let extension = server("https://extension.example/mcp"); diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 7485565422..cd9a4f378f 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -30,6 +30,7 @@ use std::sync::atomic::Ordering; use std::time::Duration; use crate::binding::call_tool_result_from_rmcp; +use crate::catalog::McpServerSource; use crate::elicitation::ElicitationRequestManager; use crate::elicitation::ElicitationRequestRouter; use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; @@ -284,14 +285,19 @@ impl McpConnectionSet { .into_iter() .filter(|(_, server)| server.enabled()) { - let is_host_owned_codex_apps = config - .mcp_server_catalog - .server(&server_name) - .is_some_and(|server| { - server - .source() - .is_host_owned_apps(&server_name, server.config()) - }); + let registration = config.mcp_server_catalog.server(&server_name); + let is_host_owned_codex_apps = registration.is_some_and(|server| { + server + .source() + .is_host_owned_apps(&server_name, server.config()) + }); + let host_plugin_root = registration.and_then(|server| match server.source() { + McpServerSource::Plugin(plugin) => plugin.host_root(), + McpServerSource::SelectedPlugin(_) + | McpServerSource::Config + | McpServerSource::Compatibility { .. } + | McpServerSource::Extension { .. } => None, + }); let catalog_item_limit = if is_host_owned_codex_apps { MAX_CODEX_APPS_TOOL_CATALOG_ITEMS } else { @@ -348,6 +354,7 @@ impl McpConnectionSet { let connection_identity = McpServerConnectionIdentity::new( &server_name, &server, + host_plugin_root, store_mode, keyring_backend_kind, &resolved_environment, diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index eb024b173e..f5f7046b42 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -58,6 +58,7 @@ use codex_rmcp_client::InProcessTransportFactory; use codex_rmcp_client::McpAuthState; use codex_rmcp_client::McpLoginRequirement; use codex_rmcp_client::RmcpClient; +use codex_utils_path_uri::PathUri; use futures::FutureExt; use futures::future::BoxFuture; use pretty_assertions::assert_eq; @@ -3854,6 +3855,7 @@ async fn executor_owned_chatgpt_mcp_accepts_only_safe_explicit_authorization() - McpServerConnectionIdentity::new( "fake-first-party", remote_server, + /*host_plugin_root*/ None, OAuthCredentialsStoreMode::File, keyring_backend_kind, &resolved_environment, @@ -4354,6 +4356,7 @@ fn reusable_server_identity( McpServerConnectionIdentity::new( "docs", &server, + /*host_plugin_root*/ None, OAuthCredentialsStoreMode::default(), AuthKeyringBackendKind::default(), &resolved_environment, @@ -4406,16 +4409,29 @@ async fn reconcile_reusable_server( config: McpServerConfig, runtime_context: McpRuntimeContext, ) -> McpConnectionSet { - let (tx_event, _rx_event) = async_channel::unbounded(); let codex_home = tempdir().expect("tempdir"); + reconcile_reusable_server_with_mcp_config( + previous, + config, + runtime_context, + crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()), + ) + .await +} + +async fn reconcile_reusable_server_with_mcp_config( + previous: &McpConnectionSet, + config: McpServerConfig, + runtime_context: McpRuntimeContext, + mcp_config: crate::McpConfig, +) -> McpConnectionSet { + let (tx_event, _rx_event) = async_channel::unbounded(); McpConnectionSet::new( Some(previous), McpPublicationGate::already_published(), McpRuntimeInput { startup_policy: McpStartupPolicy::Eager, - config: Arc::new(crate::mcp::tests::test_mcp_config( - codex_home.path().to_path_buf(), - )), + config: Arc::new(mcp_config), plugins_available: false, ready_selected_capability_roots: Vec::new(), mcp_servers: HashMap::from([( @@ -4738,6 +4754,7 @@ fn connection_identity_uses_effective_authorization_headers() { McpServerConnectionIdentity::new( "docs", &server, + /*host_plugin_root*/ None, OAuthCredentialsStoreMode::File, keyring_backend_kind, &Ok(None), @@ -5118,6 +5135,81 @@ async fn reconciliation_reconnects_when_connection_identity_changes() { assert!(!previous.shares_test_connection_with(&reconciled, "docs")); } +#[tokio::test] +async fn reconciliation_reconnects_when_host_plugin_root_changes() { + let runtime_context = reusable_server_runtime_context(); + let server_config = reusable_server_config("http://127.0.0.1:1"); + let original_root = PathUri::parse("file:///plugins/original").expect("valid plugin root URI"); + let replacement_root = + PathUri::parse("file:///plugins/replacement").expect("valid plugin root URI"); + let mut previous = manager_with_reusable_ready_server( + &server_config, + &runtime_context, + vec![create_test_tool("docs", "search")], + ) + .await; + let server = EffectiveMcpServer::configured(server_config.clone()); + let resolved_environment = runtime_context.resolve_server_environment("docs", &server_config); + let original_identity = McpServerConnectionIdentity::new( + "docs", + &server, + Some(&original_root), + OAuthCredentialsStoreMode::default(), + AuthKeyringBackendKind::default(), + &resolved_environment, + &runtime_context, + /*runtime_auth_provider*/ None, + /*auth*/ None, + /*codex_apps_cache_identity*/ None, + ElicitationCapability::default(), + ClientMcpExtensions::default(), + /*previous_identity*/ None, + ); + Arc::get_mut( + &mut previous + .servers + .get_mut("docs") + .expect("test server should exist") + .connection, + ) + .expect("test server should have one connection owner") + .identity = Some(original_identity); + + let codex_home = tempdir().expect("tempdir"); + let config_for_root = |root| { + let mut config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()); + let mut catalog = crate::ResolvedMcpCatalog::builder(); + catalog.register(crate::McpServerRegistration::from_plugin( + "docs".to_string(), + crate::McpPluginAttribution::new("docs@test".to_string(), "Docs".to_string()) + .with_host_root(root), + /*plugin_order*/ 0, + server_config.clone(), + )); + config.mcp_server_catalog = catalog.build(); + config + }; + + let unchanged = reconcile_reusable_server_with_mcp_config( + &previous, + server_config.clone(), + runtime_context.clone(), + config_for_root(original_root), + ) + .await; + assert!(previous.shares_test_connection_with(&unchanged, "docs")); + + let replacement_config = config_for_root(replacement_root); + let replacement = reconcile_reusable_server_with_mcp_config( + &unchanged, + server_config, + runtime_context, + replacement_config, + ) + .await; + assert!(!unchanged.shares_test_connection_with(&replacement, "docs")); +} + #[tokio::test] async fn connection_identity_distinguishes_accounts_with_the_same_token() -> anyhow::Result<()> { let runtime_context = reusable_server_runtime_context(); @@ -5139,6 +5231,7 @@ async fn connection_identity_distinguishes_accounts_with_the_same_token() -> any McpServerConnectionIdentity::new( "docs", &server, + /*host_plugin_root*/ None, OAuthCredentialsStoreMode::default(), AuthKeyringBackendKind::default(), &Ok(None), @@ -5191,6 +5284,7 @@ async fn connection_identity_distinguishes_agent_account_runtime_and_task() -> a McpServerConnectionIdentity::new( CODEX_APPS_MCP_SERVER_NAME, &server, + /*host_plugin_root*/ None, OAuthCredentialsStoreMode::default(), AuthKeyringBackendKind::default(), &Ok(None), diff --git a/codex-rs/codex-mcp/src/server.rs b/codex-rs/codex-mcp/src/server.rs index 355c41acbc..e1b7276975 100644 --- a/codex-rs/codex-mcp/src/server.rs +++ b/codex-rs/codex-mcp/src/server.rs @@ -17,6 +17,7 @@ use codex_login::CodexAuth; use codex_protocol::mcp::ClientMcpExtensions; use codex_rmcp_client::StoredOAuthCredentialSnapshot; use codex_rmcp_client::StoredOAuthTokens; +use codex_utils_path_uri::PathUri; use rmcp::model::ElicitationCapability; use tracing::warn; @@ -96,6 +97,7 @@ pub(crate) struct McpServerConnectionIdentity { auth: McpServerAuth, transport: McpServerTransportConfig, environment_id: String, + host_plugin_root: Option, oauth_store: Option<(OAuthCredentialsStoreMode, AuthKeyringBackendKind)>, oauth_credentials: Result, String>, pub(crate) oauth_store_was_contended: bool, @@ -115,6 +117,7 @@ impl McpServerConnectionIdentity { pub(crate) fn new( server_name: &str, server: &EffectiveMcpServer, + host_plugin_root: Option<&PathUri>, store_mode: OAuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, resolved_environment: &Result>, String>, @@ -210,6 +213,7 @@ impl McpServerConnectionIdentity { auth: config.auth.clone(), transport: config.transport.clone(), environment_id: config.environment_id.clone(), + host_plugin_root: host_plugin_root.cloned(), oauth_store: stored_oauth_url .is_some() .then_some((store_mode, keyring_backend_kind)), @@ -244,6 +248,7 @@ impl McpServerConnectionIdentity { self.auth == other.auth && self.transport == other.transport && self.environment_id == other.environment_id + && self.host_plugin_root == other.host_plugin_root && self.oauth_store == other.oauth_store && same_resolved_environment(&self.resolved_environment, &other.resolved_environment) && self.local_stdio_fallback_cwd == other.local_stdio_fallback_cwd diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 4cc4bc7e90..ce24e1b6b2 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5533,7 +5533,8 @@ async fn rebuild_preserving_session_layers_refreshes_plugin_derived_mcp_config() .plugin_attributions_by_server_name(), HashMap::from([( "sample".to_string(), - McpPluginAttribution::new("sample@test".to_string(), "sample".to_string()), + McpPluginAttribution::new("sample@test".to_string(), "sample".to_string()) + .with_host_root(PathUri::from_host_native_path(&plugin_root)?), )]) ); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 9c1b419903..a97badf61c 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1702,7 +1702,8 @@ impl Config { plugin.config_name.clone(), plugin.display_name().to_string(), ) - }; + } + .with_host_root(PathUri::from_abs_path(&plugin.root)); for (name, plugin_server) in plugin_mcp_servers { catalog.register(McpServerRegistration::from_plugin( name, diff --git a/codex-rs/core/tests/suite/tool_lifecycle.rs b/codex-rs/core/tests/suite/tool_lifecycle.rs index 58771f6ef7..82007cf657 100644 --- a/codex-rs/core/tests/suite/tool_lifecycle.rs +++ b/codex-rs/core/tests/suite/tool_lifecycle.rs @@ -18,6 +18,7 @@ use codex_extension_api::ToolLifecycleFuture; use codex_extension_api::ToolStartInput; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::models::ContentItem; +use codex_utils_path_uri::PathUri; use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; @@ -285,6 +286,70 @@ async fn tool_start_receives_executed_mcp_call_for_connector( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn tool_start_receives_frozen_host_plugin_root() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_wine_exec!(Ok(()), "requires a native test_stdio_server"); + + let server = responses::start_mock_server().await; + let codex_home = Arc::new(tempfile::tempdir()?); + let plugin_root = super::plugins::write_sample_plugin_manifest_and_config(codex_home.as_ref()); + let server_config = json!({ + "command": super::rmcp_client::remote_aware_stdio_server_bin()?, + "environment_id": super::rmcp_client::remote_aware_environment_id(), + }); + fs::write( + plugin_root.join(".mcp.json"), + serde_json::to_vec(&json!({"mcpServers": {"sample": server_config}}))?, + )?; + + let recorder = Arc::new(ConversationHistoryRecorder::default()); + let mut extensions = ExtensionRegistryBuilder::::new(); + extensions.tool_lifecycle_contributor(recorder.clone()); + let test = test_codex() + .with_home(codex_home) + .with_extensions(Arc::new(extensions.build())) + .with_model_info_override("gpt-5.4", |model| model.supports_search_tool = false) + .build_with_auto_env(&server) + .await?; + wait_for_mcp_server(&test.codex, "sample").await?; + responses::mount_sse_sequence( + &server, + vec![ + responses::sse(vec![ + responses::ev_function_call_with_namespace( + "plugin-lifecycle-call", + "mcp__sample", + "echo", + r#"{"message":"ping"}"#, + ), + responses::ev_completed("first-response"), + ]), + responses::sse(vec![responses::ev_completed("second-response")]), + ], + ) + .await; + test.submit_text_turn("Call the sample echo tool.").await?; + + let histories = recorder + .histories + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let [history] = histories.as_slice() else { + panic!("expected one tool start, got {}", histories.len()); + }; + let expected_source = McpToolSource::Plugin { + id: "sample@test".to_string(), + root: PathUri::from_host_native_path(&plugin_root)?, + }; + assert_eq!( + history.mcp_tool, + Some(("sample".to_string(), None, expected_source)), + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn tool_start_receives_rewritten_payload_and_post_hook_history() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/ext/extension-api/src/contributors/tool_lifecycle.rs b/codex-rs/ext/extension-api/src/contributors/tool_lifecycle.rs index ed489afb78..484202d7f9 100644 --- a/codex-rs/ext/extension-api/src/contributors/tool_lifecycle.rs +++ b/codex-rs/ext/extension-api/src/contributors/tool_lifecycle.rs @@ -5,9 +5,11 @@ use std::sync::Arc; use codex_config::McpServerConfig; use codex_mcp::McpServerSource; use codex_mcp::PreparedMcpCall; +use codex_mcp::ResolvedMcpServer; use codex_tools::ToolCallSource; use codex_tools::ToolName; use codex_tools::ToolPayload; +use codex_utils_path_uri::PathUri; use crate::ConversationHistorySnapshot; use crate::ExtensionData; @@ -47,6 +49,8 @@ pub enum McpToolSource { Plugin { /// Identifier of the plugin that owns this MCP server. id: String, + /// Host-local plugin root captured with the exact server registration. + root: PathUri, }, /// An executor-selected plugin whose root has not been attested by the host. SelectedPlugin, @@ -71,21 +75,24 @@ impl McpToolContext { configured_server: Option<&McpServerConfig>, ) -> Self { let tool = call.tool_info().clone(); + let registration = call.config().mcp_server_catalog.server(call.server_name()); let source = if tool.connector_id.is_some() && call.is_host_owned_apps() { McpToolSource::Connector } else if call.is_selected_plugin_server() { McpToolSource::SelectedPlugin - } else if let Some(id) = call.plugin_id() { - McpToolSource::Plugin { id: id.to_owned() } - } else if call - .config() - .mcp_server_catalog - .server(call.server_name()) - .is_some_and(|server| { - matches!(server.source(), McpServerSource::Config) - && configured_server.is_some_and(|configured| server.config() == configured) - }) + } else if let Some(McpServerSource::Plugin(plugin)) = + registration.map(ResolvedMcpServer::source) + && Some(plugin.plugin_id()) == call.plugin_id() + && let Some(root) = plugin.host_root() { + McpToolSource::Plugin { + id: plugin.plugin_id().to_owned(), + root: root.clone(), + } + } else if registration.is_some_and(|server| { + matches!(server.source(), McpServerSource::Config) + && configured_server.is_some_and(|configured| server.config() == configured) + }) { McpToolSource::Config } else { McpToolSource::Other diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools.rs b/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools.rs index 9c94c21cdc..240ea20b68 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools.rs @@ -68,13 +68,13 @@ pub(crate) async fn trusted_tool_context( ) -> Option { let codex_home = config.codex_home.as_path().canonicalize().ok()?; let plugins = match source { - McpToolSource::Connector | McpToolSource::Plugin { .. } => Some( + McpToolSource::Connector => Some( manager .plugins_manager() .plugins_for_config(&config.plugins_config_input()) .await, ), - McpToolSource::Config => None, + McpToolSource::Config | McpToolSource::Plugin { .. } => None, McpToolSource::SelectedPlugin | McpToolSource::Other => return None, }; @@ -98,18 +98,16 @@ pub(crate) async fn trusted_tool_context( trusted_user_config_source(config, "apps", connector_id, &codex_home)? } } - McpToolSource::Plugin { id: plugin_id } => { - let plugin = plugins.as_ref()?.plugins().iter().find(|plugin| { - plugin.is_active() - && plugin.config_name == plugin_id.as_str() - && plugin.mcp_servers.contains_key(&tool.server_name) - && is_home_owned_plugin_capability( - plugin.root.as_path(), - &codex_home, - PluginCapability::Mcp, - ) - })?; - plugin.root.as_path().display().to_string() + McpToolSource::Plugin { root, .. } => { + let plugin_root = root.to_abs_path().ok()?; + if !is_home_owned_plugin_capability( + plugin_root.as_path(), + &codex_home, + PluginCapability::Mcp, + ) { + return None; + } + plugin_root.as_path().display().to_string() } McpToolSource::Config => { trusted_user_config_source(config, "mcp_servers", &tool.server_name, &codex_home)? diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools_tests.rs index cd00c0bc5a..8cd768999b 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/trusted_tools_tests.rs @@ -149,6 +149,11 @@ async fn trusts_connector_declared_by_home_owned_plugin() -> Result<()> { &mcp, &McpToolSource::Plugin { id: "trusted@test".to_string(), + root: test + .config + .codex_home + .join("plugins/cache/test/trusted/local") + .into(), }, &test.thread_manager, &test.config, @@ -156,6 +161,20 @@ async fn trusts_connector_declared_by_home_owned_plugin() -> Result<()> { .await .expect("home-owned plugin MCP server should be trusted"); assert_eq!(mcp_context.metadata, expected_context(&mcp, &plugin_root)); + // A trusted cached plugin with the same ID must not replace the frozen outside-home root. + assert_eq!( + trusted_tool_context( + &mcp, + &McpToolSource::Plugin { + id: "trusted@test".to_string(), + root: test.config.cwd.clone().into(), + }, + &test.thread_manager, + &test.config, + ) + .await, + None, + ); assert_eq!( trusted_tool_context( &mcp,