diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs index 03e0c001c1..14189c8ab3 100644 --- a/codex-rs/codex-mcp/src/catalog.rs +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -10,6 +10,7 @@ use codex_protocol::mcp_policy::EnvironmentMcpPolicy; use codex_utils_path_uri::PathUri; use crate::CODEX_APPS_MCP_SERVER_NAME; +use crate::McpProtocolMode; /// Plugin identity retained with an MCP registration for tool attribution. #[derive(Clone, Debug, PartialEq, Eq)] @@ -137,6 +138,7 @@ pub struct McpServerRegistration { name: String, source: McpServerSource, config: McpServerConfig, + protocol_mode: Option, precedence: RegistrationPrecedence, } @@ -209,6 +211,12 @@ impl McpServerRegistration { ) } + /// Overrides the protocol for this registration if it wins HTTP server resolution. + pub fn with_protocol_mode(mut self, protocol_mode: McpProtocolMode) -> Self { + self.protocol_mode = Some(protocol_mode); + self + } + /// Registers the controller-owned Apps server contributed by a host extension. pub fn from_hosted_apps( id: impl Into, @@ -237,6 +245,7 @@ impl McpServerRegistration { name, source, config, + protocol_mode: None, precedence, } } @@ -471,6 +480,7 @@ impl McpCatalogBuilder { ResolvedMcpServer { source: registration.source, config: registration.config, + protocol_mode: registration.protocol_mode, }, )) } @@ -492,6 +502,7 @@ impl McpCatalogBuilder { pub struct ResolvedMcpServer { source: McpServerSource, config: McpServerConfig, + protocol_mode: Option, } impl ResolvedMcpServer { @@ -502,6 +513,10 @@ impl ResolvedMcpServer { pub fn config(&self) -> &McpServerConfig { &self.config } + + pub fn protocol_mode(&self) -> Option { + self.protocol_mode + } } /// Immutable result of MCP registration resolution. @@ -547,8 +562,8 @@ impl ResolvedMcpCatalog { pub fn with_materialized_servers(&self, servers: HashMap) -> Self { let mut builder = Self::builder(); for (name, config) in servers { - let source = self - .server(&name) + let previous = self.server(&name); + let source = previous .map(|server| server.source.clone()) .unwrap_or(McpServerSource::Config); let precedence = match &source { @@ -560,7 +575,9 @@ impl ResolvedMcpCatalog { McpServerSource::Compatibility { .. } => RegistrationPrecedence::Compatibility, McpServerSource::Extension { .. } => RegistrationPrecedence::Extension(0), }; - builder.register(McpServerRegistration::new(name, source, config, precedence)); + let mut registration = McpServerRegistration::new(name, source, config, precedence); + registration.protocol_mode = previous.and_then(ResolvedMcpServer::protocol_mode); + builder.register(registration); } builder.build() } diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs index 896b2d4522..39f4d11008 100644 --- a/codex-rs/codex-mcp/src/catalog_tests.rs +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -13,6 +13,7 @@ use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; use crate::CODEX_APPS_MCP_SERVER_NAME; +use crate::McpProtocolMode; use super::McpEnvironmentAuthority; use super::McpPluginAttribution; @@ -224,6 +225,7 @@ fn disabled_winner_remains_a_veto_when_the_catalog_is_extended() { Some(&super::ResolvedMcpServer { source: extension_source("hosted"), config: expected, + protocol_mode: None, }) ); } @@ -256,6 +258,7 @@ fn disabled_discovered_plugin_remains_a_veto_for_runtime_overlays() { Some(&super::ResolvedMcpServer { source: extension_source("hosted"), config: expected, + protocol_mode: None, }) ); } @@ -328,6 +331,7 @@ fn selected_plugins_override_discovered_plugins_but_not_config() { Some(&super::ResolvedMcpServer { source: selected_plugin_source("selected-alpha"), config: selected, + protocol_mode: None, }) ); assert_eq!( @@ -354,6 +358,7 @@ fn selected_plugins_override_discovered_plugins_but_not_config() { Some(&super::ResolvedMcpServer { source: selected_plugin_source("selected-alpha"), config: refreshed, + protocol_mode: None, }) ); @@ -370,6 +375,7 @@ fn selected_plugins_override_discovered_plugins_but_not_config() { Some(&super::ResolvedMcpServer { source: McpServerSource::Config, config: configured, + protocol_mode: None, }) ); } @@ -401,6 +407,7 @@ fn disabled_selected_plugin_does_not_veto_runtime_overlays() { Some(&super::ResolvedMcpServer { source: extension_source("hosted"), config: extension, + protocol_mode: None, }) ); } @@ -426,6 +433,7 @@ fn equal_precedence_uses_insertion_order_not_source_identity() { Some(&super::ResolvedMcpServer { source: compatibility_source("a-second"), config: server("https://second.example/mcp"), + protocol_mode: None, }) ); let mut builder = catalog.to_builder(); @@ -448,6 +456,59 @@ fn equal_precedence_uses_insertion_order_not_source_identity() { ); } +#[test] +fn extension_protocol_mode_follows_the_winner_through_materialization() { + let config = server("https://apps.example/mcp"); + let mut builder = ResolvedMcpCatalog::builder(); + builder.register( + McpServerRegistration::from_extension( + "apps".to_string(), + "loser", + /*contribution_order*/ 0, + config.clone(), + ) + .with_protocol_mode(McpProtocolMode::V20260728), + ); + builder.register(McpServerRegistration::from_extension( + "apps".to_string(), + "winner", + /*contribution_order*/ 1, + config.clone(), + )); + let without_override = builder.build(); + assert_eq!( + without_override + .server("apps") + .and_then(ResolvedMcpServer::protocol_mode), + None + ); + + let mut builder = ResolvedMcpCatalog::builder(); + builder.register( + McpServerRegistration::from_extension( + "apps".to_string(), + "winner", + /*contribution_order*/ 1, + config, + ) + .with_protocol_mode(McpProtocolMode::Legacy), + ); + let with_override = builder.build(); + assert!(!without_override.has_same_servers(&with_override)); + + let refreshed = server("https://refreshed.example/mcp"); + let materialized = with_override + .with_materialized_servers(HashMap::from([("apps".to_string(), refreshed.clone())])); + assert_eq!( + materialized.server("apps"), + Some(&ResolvedMcpServer { + source: extension_source("winner"), + config: refreshed, + protocol_mode: Some(McpProtocolMode::Legacy), + }) + ); +} + #[test] fn environment_policy_exempts_only_explicitly_host_owned_apps() { let policy = EnvironmentMcpPolicy { diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index b95a678134..ffa238bb87 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -314,12 +314,17 @@ impl McpConnectionSet { }; let metadata = McpServerMetadata::from(&server); let configured_config = server.config().clone(); - let protocol_mode = if is_host_owned_codex_apps - && matches!( - &configured_config.transport, - McpServerTransportConfig::StreamableHttp { .. } - ) { - host_owned_apps_protocol_mode + let protocol_mode = if matches!( + &configured_config.transport, + McpServerTransportConfig::StreamableHttp { .. } + ) { + registration + .and_then(crate::ResolvedMcpServer::protocol_mode) + .unwrap_or(if is_host_owned_codex_apps { + host_owned_apps_protocol_mode + } else { + default_protocol_mode + }) } else { default_protocol_mode }; diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index f8f8a468a8..502f379026 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -2099,65 +2099,81 @@ async fn codex_apps_extension_does_not_share_host_owned_tools_cache() -> anyhow: let server_config: McpServerConfig = serde_json::from_value(serde_json::json!({ "url": "http://127.0.0.1:1" }))?; - let mut config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()); - config.host_owned_apps_protocol_mode = crate::McpProtocolMode::V20260728; - let mut catalog = crate::ResolvedMcpCatalog::builder(); - catalog.register(crate::McpServerRegistration::from_extension( - CODEX_APPS_MCP_SERVER_NAME.to_string(), - "test-extension", - /*contribution_order*/ 0, - server_config.clone(), - )); - config.mcp_server_catalog = catalog.build(); + for (hosted_mode, extension_mode, expected_mode) in [ + ( + crate::McpProtocolMode::V20260728, + None, + crate::McpProtocolMode::Legacy, + ), + ( + crate::McpProtocolMode::Legacy, + Some(crate::McpProtocolMode::V20260728), + crate::McpProtocolMode::V20260728, + ), + ] { + let mut config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()); + config.host_owned_apps_protocol_mode = hosted_mode; + let mut registration = crate::McpServerRegistration::from_extension( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + "test-extension", + /*contribution_order*/ 0, + server_config.clone(), + ); + if let Some(mode) = extension_mode { + registration = registration.with_protocol_mode(mode); + } + let mut catalog = crate::ResolvedMcpCatalog::builder(); + catalog.register(registration); + config.mcp_server_catalog = catalog.build(); - let startup_cancellation_token = CancellationToken::new(); - startup_cancellation_token.cancel(); - let manager = McpConnectionSet::new( - /*previous*/ None, - McpPublicationGate::already_published(), - McpRuntimeInput { - startup_policy: McpStartupPolicy::Eager, - config: Arc::new(config), - plugins_available: false, - ready_selected_capability_roots: Vec::new(), - mcp_servers: HashMap::from([( - CODEX_APPS_MCP_SERVER_NAME.to_string(), - EffectiveMcpServer::configured(server_config), - )]), - submit_id: "cache-ownership-test".to_string(), - tx_event: None, - startup_cancellation_token, - runtime_context: McpRuntimeContext::new( - Arc::new(environment_manager_without_environments()), - codex_home.path().to_path_buf(), - ), - codex_apps_tools_cache, - tool_catalog_cache: McpToolCatalogCache::default(), - codex_apps_tools_cache_key: cache_key, - client_mcp_extensions: ClientMcpExtensions::default(), - auth: None, - auth_manager: None, - elicitation_reviewer: None, - elicitation_lifecycle: None, - }, - ElicitationRequestRouter::default(), - ) - .await; + let startup_cancellation_token = CancellationToken::new(); + startup_cancellation_token.cancel(); + let manager = McpConnectionSet::new( + /*previous*/ None, + McpPublicationGate::already_published(), + McpRuntimeInput { + startup_policy: McpStartupPolicy::Eager, + config: Arc::new(config), + plugins_available: false, + ready_selected_capability_roots: Vec::new(), + mcp_servers: HashMap::from([( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + EffectiveMcpServer::configured(server_config.clone()), + )]), + submit_id: "cache-ownership-test".to_string(), + tx_event: None, + startup_cancellation_token, + runtime_context: McpRuntimeContext::new( + Arc::new(environment_manager_without_environments()), + codex_home.path().to_path_buf(), + ), + codex_apps_tools_cache: codex_apps_tools_cache.clone(), + tool_catalog_cache: McpToolCatalogCache::default(), + codex_apps_tools_cache_key: cache_key.clone(), + client_mcp_extensions: ClientMcpExtensions::default(), + auth: None, + auth_manager: None, + elicitation_reviewer: None, + elicitation_lifecycle: None, + }, + ElicitationRequestRouter::default(), + ) + .await; - let client = manager.test_client(CODEX_APPS_MCP_SERVER_NAME); - assert_eq!( - manager.servers[CODEX_APPS_MCP_SERVER_NAME].protocol_mode, - crate::McpProtocolMode::Legacy, - "an ordinary extension named codex_apps must not inherit the hosted protocol default" - ); - assert!( - client.codex_apps_tools_cache_context.is_none(), - "an extension must not receive the host-owned Apps cache" - ); - assert!( - !client.has_cached_tools(), - "an extension must not expose cached host-owned Apps tools" - ); + let client = manager.test_client(CODEX_APPS_MCP_SERVER_NAME); + assert_eq!( + manager.servers[CODEX_APPS_MCP_SERVER_NAME].protocol_mode, expected_mode, + "an ordinary extension must use its own mode, not the hosted protocol default" + ); + assert!( + client.codex_apps_tools_cache_context.is_none(), + "an extension must not receive the host-owned Apps cache" + ); + assert!( + !client.has_cached_tools(), + "an extension must not expose cached host-owned Apps tools" + ); + } Ok(()) } diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index 193fc946c9..2376ea62b5 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -179,6 +179,21 @@ impl McpManager { ), ))); } + McpServerContribution::SetWithProtocolMode { + name, + config, + protocol_mode, + } => { + overlays.push(OrderedMcpOverlay::Set(Box::new( + McpServerRegistration::from_extension( + name, + contributor.id(), + contribution_order, + *config, + ) + .with_protocol_mode(protocol_mode), + ))); + } McpServerContribution::HostedApps { config } => { overlays.push(OrderedMcpOverlay::Set(Box::new( McpServerRegistration::from_hosted_apps( diff --git a/codex-rs/core/tests/suite/mcp_extension_protocol.rs b/codex-rs/core/tests/suite/mcp_extension_protocol.rs new file mode 100644 index 0000000000..93d69096b1 --- /dev/null +++ b/codex-rs/core/tests/suite/mcp_extension_protocol.rs @@ -0,0 +1,151 @@ +//! Verifies that an extension can choose MCP protocol mode for its own HTTP server. + +use std::collections::HashMap; +use std::sync::Arc; + +use codex_config::McpServerConfig; +use codex_core::config::Config; +use codex_extension_api::ExtensionFuture; +use codex_extension_api::ExtensionRegistryBuilder; +use codex_extension_api::McpProtocolMode; +use codex_extension_api::McpServerContribution; +use codex_extension_api::McpServerContributionContext; +use codex_extension_api::McpServerContributor; +use codex_features::Feature; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_mcp_server; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use wiremock::MockServer; + +struct AppsExtensionEndpoint { + url: String, + protocol_mode: McpProtocolMode, +} + +impl McpServerContributor for AppsExtensionEndpoint { + fn id(&self) -> &'static str { + "apps_protocol_test" + } + + fn contribute<'a>( + &'a self, + _context: McpServerContributionContext<'a, Config>, + ) -> ExtensionFuture<'a, Vec> { + Box::pin(async move { + vec![McpServerContribution::SetWithProtocolMode { + name: "extension_apps".to_string(), + config: Box::new( + serde_json::from_value(json!({ "url": self.url })) + .expect("extension Apps MCP config"), + ), + protocol_mode: self.protocol_mode, + }] + }) + } +} + +async fn mcp_methods(server: &MockServer) -> Vec { + server + .received_requests() + .await + .expect("mock server should capture MCP startup requests") + .into_iter() + .filter(|request| request.url.path() == "/api/codex/ps/mcp") + .filter_map(|request| { + let body: Value = serde_json::from_slice(&request.body).ok()?; + body.get("method")?.as_str().map(str::to_string) + }) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn extension_protocol_mode_does_not_change_other_http_servers() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + for (generic_mode, extension_mode) in [ + (McpProtocolMode::Legacy, McpProtocolMode::V20260728), + (McpProtocolMode::V20260728, McpProtocolMode::Legacy), + ] { + let responses_server = responses::start_mock_server().await; + let extension_server = responses::start_mock_server().await; + let extension_url = format!( + "{}/api/codex/ps/mcp", + AppsTestServer::mount(&extension_server) + .await? + .chatgpt_base_url + ); + let third_party_server = responses::start_mock_server().await; + let third_party_url = format!( + "{}/api/codex/ps/mcp", + AppsTestServer::mount(&third_party_server) + .await? + .chatgpt_base_url + ); + let mut extensions = ExtensionRegistryBuilder::new(); + extensions.mcp_server_contributor(Arc::new(AppsExtensionEndpoint { + url: extension_url, + protocol_mode: extension_mode, + })); + + let fixture = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_config(move |config| { + config + .features + .disable(Feature::Apps) + .expect("test config should disable hosted Apps"); + if generic_mode == McpProtocolMode::V20260728 { + config + .features + .enable(Feature::Mcp20260728) + .expect("test config should enable generic MCP protocol"); + } else { + config + .features + .disable(Feature::Mcp20260728) + .expect("test config should disable generic MCP protocol"); + } + let third_party: McpServerConfig = + serde_json::from_value(json!({ "url": third_party_url })) + .expect("third-party MCP config"); + config + .mcp_servers + .set(HashMap::from([("third_party".to_string(), third_party)])) + .expect("test config should accept MCP server"); + }) + .build_with_auto_env(&responses_server) + .await?; + + wait_for_mcp_server(&fixture.codex, "extension_apps").await?; + let legacy = vec!["initialize", "notifications/initialized", "tools/list"]; + let modern = vec![ + "server/discover", + "initialize", + "notifications/initialized", + "tools/list", + ]; + assert_eq!( + mcp_methods(&extension_server).await, + if extension_mode == McpProtocolMode::V20260728 { + modern.clone() + } else { + legacy.clone() + } + ); + assert_eq!( + mcp_methods(&third_party_server).await, + if generic_mode == McpProtocolMode::V20260728 { + modern + } else { + legacy + } + ); + } + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 1fcc376522..b266997ed4 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -100,6 +100,7 @@ mod json_result; mod live_cli; mod mcp_auth_elicitation; mod mcp_auth_refresh; +mod mcp_extension_protocol; mod mcp_optional_startup_grace; #[cfg(unix)] mod mcp_refresh_cleanup; diff --git a/codex-rs/ext/extension-api/src/contributors/mcp.rs b/codex-rs/ext/extension-api/src/contributors/mcp.rs index 6c36433e43..aa3afdc41a 100644 --- a/codex-rs/ext/extension-api/src/contributors/mcp.rs +++ b/codex-rs/ext/extension-api/src/contributors/mcp.rs @@ -135,6 +135,14 @@ pub enum McpServerContribution { name: String, config: Box, }, + /// Adds an ordinary extension-owned server with its own HTTP protocol mode. + /// The mode applies only if this registration wins server resolution; it + /// does not grant controller-owned Apps cache or environment authority. + SetWithProtocolMode { + name: String, + config: Box, + protocol_mode: crate::McpProtocolMode, + }, /// Registers the controller-owned Apps server under its reserved name. HostedApps { config: Box }, /// Registers a server declared by a plugin selected for this thread. diff --git a/codex-rs/ext/extension-api/src/lib.rs b/codex-rs/ext/extension-api/src/lib.rs index 0fab8a01a6..3daadfe87a 100644 --- a/codex-rs/ext/extension-api/src/lib.rs +++ b/codex-rs/ext/extension-api/src/lib.rs @@ -21,6 +21,7 @@ pub use capabilities::NoopResponseItemInjector; pub use capabilities::ResponseItemInjectionFuture; pub use capabilities::ResponseItemInjector; pub use codex_context_fragments::ContextualUserFragment; +pub use codex_mcp::McpProtocolMode; pub use codex_mcp::ToolInfo as McpToolInfo; pub use codex_protocol::models::ContentItemKind; pub use codex_protocol::models::ResponseItem; diff --git a/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs b/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs index 3ab8077d70..7b68b9c1a1 100644 --- a/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs +++ b/codex-rs/ext/mcp/tests/executor_plugin_mcp.rs @@ -367,6 +367,7 @@ async fn selected_plugin_contributions( }), McpServerContribution::SelectedPluginPackage { .. } => None, McpServerContribution::Set { .. } + | McpServerContribution::SetWithProtocolMode { .. } | McpServerContribution::HostedApps { .. } | McpServerContribution::Remove { .. } => { panic!("expected selected plugin contribution")