From 56b82e676cc56ccd550362fc5055c76ba3445849 Mon Sep 17 00:00:00 2001 From: jacobzhou-oai Date: Wed, 5 Aug 2026 04:48:41 +0000 Subject: [PATCH] Enforce Agent Plugin runtime boundaries (#37027) ## What changed - Track Agent Plugin manifests through plugin, skill, and MCP loading so their capabilities use format-specific behavior without changing legacy plugins. - Discover only direct-child skills, exclude app and hook capabilities, isolate MCP data, and reject MCP configuration files that are non-regular or resolve outside the plugin root. - Bound model-visible skill instructions, plugin instructions, MCP descriptions, schemas, individual tools, and the aggregate Agent Plugin MCP tool set. - Stop MCP and OAuth redirects when Agent Plugins send configured or authorization headers, while retaining existing redirect behavior for legacy MCP servers. ## Testing - Add coverage for capability filtering, skill discovery, isolated MCP data and reserved-path expansion, unsafe MCP configuration files, context limits, and redirect handling. GitOrigin-RevId: c9af66b051269f3226628ca280a58d32c808c38f --- codex-rs/Cargo.lock | 1 + .../analytics/src/analytics_client_tests.rs | 1 + codex-rs/app-server/src/request_processors.rs | 1 + .../src/request_processors/mcp_processor.rs | 7 + .../src/request_processors/plugins.rs | 34 ++- .../tests/suite/v2/plugin_install.rs | 2 +- codex-rs/cli/src/mcp_cmd.rs | 3 + codex-rs/codex-mcp/src/catalog.rs | 23 ++ codex-rs/codex-mcp/src/mcp/auth.rs | 14 +- codex-rs/codex-mcp/src/mcp/mod.rs | 9 +- codex-rs/codex-mcp/src/mcp/mod_tests.rs | 3 + codex-rs/codex-mcp/src/rmcp_client.rs | 9 +- codex-rs/codex-mcp/src/server.rs | 18 +- codex-rs/core-plugins/src/loader.rs | 225 ++++++++++++--- codex-rs/core-plugins/src/loader_tests.rs | 178 +++++++++++- codex-rs/core-plugins/src/manager.rs | 52 +++- codex-rs/core-plugins/src/manager_tests.rs | 120 +++++++- codex-rs/core-plugins/src/manifest.rs | 29 +- codex-rs/core-plugins/src/remote.rs | 1 + .../src/script_attribution_tests.rs | 2 + codex-rs/core-plugins/src/store.rs | 23 ++ codex-rs/core-plugins/src/store_tests.rs | 22 ++ .../core-plugins/src/tool_suggest_metadata.rs | 17 +- codex-rs/core-skills/Cargo.toml | 1 + codex-rs/core-skills/src/injection.rs | 20 ++ codex-rs/core-skills/src/injection_tests.rs | 10 + codex-rs/core-skills/src/lib.rs | 6 + codex-rs/core-skills/src/loader.rs | 4 +- codex-rs/core-skills/src/loader_tests.rs | 4 +- codex-rs/core-skills/src/model.rs | 9 + codex-rs/core-skills/src/root_loader.rs | 18 +- codex-rs/core/src/config/mod.rs | 15 +- codex-rs/core/src/mcp_skill_dependencies.rs | 2 + codex-rs/core/src/mcp_tool_exposure.rs | 40 ++- codex-rs/core/src/mcp_tool_exposure_test.rs | 106 +++++++ codex-rs/core/src/plugins/mentions_tests.rs | 1 + codex-rs/core/src/plugins/render.rs | 25 +- codex-rs/core/src/plugins/render_tests.rs | 41 +++ codex-rs/core/src/tools/handlers/mcp.rs | 43 ++- .../src/tools/handlers/mcp_search_tests.rs | 21 ++ codex-rs/core/src/tools/spec_plan.rs | 1 + codex-rs/core/tests/suite/plugins.rs | 261 ++++++++++++++++++ codex-rs/ext/skills/src/render.rs | 4 +- codex-rs/plugin/src/lib.rs | 1 + codex-rs/plugin/src/load_outcome.rs | 9 +- codex-rs/rmcp-client/src/auth_status.rs | 68 ++++- .../rmcp-client/src/http_client_adapter.rs | 30 +- .../src/http_client_adapter_tests.rs | 62 +++++ codex-rs/rmcp-client/src/lib.rs | 1 + codex-rs/rmcp-client/src/oauth_http_client.rs | 110 +++++++- .../rmcp-client/src/perform_oauth_login.rs | 25 +- codex-rs/rmcp-client/src/rmcp_client.rs | 61 +++- .../rmcp-client/tests/mcp_2026_discovery.rs | 2 +- .../tests/mcp_2026_oauth_discovery.rs | 5 + .../tests/streamable_http_oauth_startup.rs | 2 + codex-rs/tools/src/lib.rs | 2 + codex-rs/tools/src/mcp_tool.rs | 25 +- codex-rs/tools/src/mcp_tool_tests.rs | 35 +++ codex-rs/tools/src/responses_api.rs | 20 ++ codex-rs/tools/src/responses_api_tests.rs | 41 +++ codex-rs/tui/src/app/plugin_mentions.rs | 3 + codex-rs/tui/src/bottom_pane/chat_composer.rs | 7 + .../tui/src/chatwidget/tests/plan_mode.rs | 1 + 63 files changed, 1830 insertions(+), 106 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b2382ed8bf..76d2af28e4 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2923,6 +2923,7 @@ dependencies = [ "codex-utils-absolute-path", "codex-utils-path-uri", "codex-utils-plugins", + "codex-utils-string", "dunce", "futures", "pretty_assertions", diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 45d83c362c..47707225bc 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -5379,6 +5379,7 @@ fn sample_plugin_metadata() -> PluginTelemetryMetadata { capability_summary: Some(PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: None, description: None, has_skills: true, mcp_server_names: vec!["mcp-1".to_string(), "mcp-2".to_string()], diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 5d9bec815b..57dbb04279 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -458,6 +458,7 @@ use codex_protocol::protocol::W3cTraceContext; use codex_protocol::protocol::strip_user_message_prefix; use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS; use codex_protocol::user_input::UserInput as CoreInputItem; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::perform_oauth_login_return_url; use codex_rollout::is_persisted_rollout_item; use codex_rollout::state_db::StateDbHandle; diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 0ea24d4226..a6281e2227 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -149,6 +149,11 @@ impl McpRequestProcessor { "No MCP server named '{name}' found." ))); }; + let redirect_mode = if server.is_agent_plugin() { + StreamableHttpRedirectMode::AgentPluginV1 + } else { + StreamableHttpRedirectMode::Legacy + }; let server = server.config(); let (url, http_headers, env_http_headers) = match &server.transport { @@ -176,6 +181,7 @@ impl McpRequestProcessor { &server.transport, Arc::clone(&http_client), codex_rmcp_client::OAuthDiscoveryTimeout::Requested, + redirect_mode, ) .await } else { @@ -199,6 +205,7 @@ impl McpRequestProcessor { mcp_config.mcp_oauth_callback_port, mcp_config.mcp_oauth_callback_url.as_deref(), http_client, + redirect_mode, ) .await .map_err(|err| internal_error(format!("failed to login to MCP server '{name}': {err}")))?; diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 20b283d066..45ff3c947a 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -12,6 +12,7 @@ use codex_core_plugins::OPENAI_CURATED_MARKETPLACE_NAME; use codex_core_plugins::PluginListBackgroundTaskOptions; use codex_core_plugins::is_openai_curated_marketplace_name; use codex_core_plugins::loader::load_configured_plugin_mcp_servers; +use codex_core_plugins::manifest::is_agent_plugin_manifest; use codex_core_plugins::remote::REMOTE_CREATED_BY_ME_MARKETPLACE_NAME; use codex_core_plugins::remote::REMOTE_GLOBAL_MARKETPLACE_NAME; use codex_core_plugins::remote::REMOTE_WORKSPACE_MARKETPLACE_NAME; @@ -32,10 +33,19 @@ use codex_plugin::PluginId; use codex_plugin::PluginTelemetryMetadata; use codex_protocol::auth::AuthMode as DomainAuthMode; use codex_rmcp_client::OAuthDiscoveryTimeout; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::perform_oauth_login_silent; mod search; +fn plugin_redirect_mode(plugin_root: &Path) -> StreamableHttpRedirectMode { + if is_agent_plugin_manifest(plugin_root) { + StreamableHttpRedirectMode::AgentPluginV1 + } else { + StreamableHttpRedirectMode::Legacy + } +} + #[derive(Clone)] pub(crate) struct PluginRequestProcessor { auth_manager: Arc, @@ -1540,8 +1550,14 @@ impl PluginRequestProcessor { ) .await; if !plugin_mcp_servers.is_empty() { - self.start_plugin_mcp_oauth_logins(&config, &result.plugin_id, plugin_mcp_servers) - .await; + let redirect_mode = plugin_redirect_mode(result.installed_path.as_path()); + self.start_plugin_mcp_oauth_logins( + &config, + &result.plugin_id, + plugin_mcp_servers, + redirect_mode, + ) + .await; } let plugin_app_declarations = load_plugin_apps(result.installed_path.as_path()).await; @@ -1717,8 +1733,14 @@ impl PluginRequestProcessor { ) .await; if !plugin_mcp_servers.is_empty() { - self.start_plugin_mcp_oauth_logins(&config, &result.plugin_id, plugin_mcp_servers) - .await; + let redirect_mode = plugin_redirect_mode(result.installed_path.as_path()); + self.start_plugin_mcp_oauth_logins( + &config, + &result.plugin_id, + plugin_mcp_servers, + redirect_mode, + ) + .await; } let is_chatgpt_auth = auth.as_ref().is_some_and(CodexAuth::is_chatgpt_auth); @@ -1869,6 +1891,7 @@ impl PluginRequestProcessor { config: &Config, plugin_id: &PluginId, mut plugin_mcp_servers: HashMap, + redirect_mode: StreamableHttpRedirectMode, ) { let plugin_id = plugin_id.as_key(); config.apply_plugin_mcp_server_requirements(&plugin_id, &mut plugin_mcp_servers); @@ -1900,6 +1923,7 @@ impl PluginRequestProcessor { &server.transport, Arc::clone(&http_client), OAuthDiscoveryTimeout::LOCAL, + redirect_mode, ) .await; let oauth_config = match login_support { @@ -1944,6 +1968,7 @@ impl PluginRequestProcessor { callback_port, callback_url.as_deref(), Arc::clone(&http_client), + redirect_mode, ) .await; @@ -1962,6 +1987,7 @@ impl PluginRequestProcessor { callback_port, callback_url.as_deref(), http_client, + redirect_mode, ) .await } diff --git a/codex-rs/app-server/tests/suite/v2/plugin_install.rs b/codex-rs/app-server/tests/suite/v2/plugin_install.rs index a8e76d28e4..974d7de49b 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_install.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_install.rs @@ -2927,7 +2927,7 @@ fn remote_plugin_bundle_tar_gz_bytes_with_entries( app_manifest: Option<&str>, mcp_config: Option<&str>, ) -> Result> { - let skill = "# Plan Work\n\nTrack work in Linear.\n"; + let skill = "---\nname: plan-work\ndescription: Track work in Linear.\n---\n\n# Plan Work\n"; let encoder = GzEncoder::new(Vec::new(), Compression::default()); let mut tar = tar::Builder::new(encoder); let mut entries = vec![ diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 2ebd737cf7..ec51a8452e 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -32,6 +32,7 @@ use codex_mcp::resolve_oauth_scopes; use codex_mcp::should_retry_without_scopes; use codex_protocol::protocol::McpAuthStatus; use codex_rmcp_client::OAuthDiscoveryTimeout; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::delete_oauth_tokens; use codex_rmcp_client::perform_oauth_login; use codex_utils_cli::CliConfigOverrides; @@ -406,6 +407,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re &transport, Arc::clone(&http_client), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await; match login_support { @@ -511,6 +513,7 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { &server.transport, Arc::clone(&http_client), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await } else { diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs index d60ea843df..274b0eb001 100644 --- a/codex-rs/codex-mcp/src/catalog.rs +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -10,6 +10,7 @@ use codex_config::McpServerConfig; pub struct McpPluginAttribution { plugin_id: String, display_name: String, + agent_plugin: bool, } impl McpPluginAttribution { @@ -17,6 +18,15 @@ impl McpPluginAttribution { Self { plugin_id, display_name, + agent_plugin: false, + } + } + + pub fn agent_plugin(plugin_id: String, display_name: String) -> Self { + Self { + plugin_id, + display_name, + agent_plugin: true, } } @@ -27,6 +37,10 @@ impl McpPluginAttribution { pub fn display_name(&self) -> &str { &self.display_name } + + pub fn is_agent_plugin(&self) -> bool { + self.agent_plugin + } } /// The component that declared an MCP server registration. @@ -46,6 +60,15 @@ pub enum McpServerSource { } impl McpServerSource { + pub fn is_agent_plugin(&self) -> bool { + match self { + Self::Plugin(attribution) | Self::SelectedPlugin(attribution) => { + attribution.is_agent_plugin() + } + Self::Config | Self::Compatibility { .. } | Self::Extension { .. } => false, + } + } + 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. diff --git a/codex-rs/codex-mcp/src/mcp/auth.rs b/codex-rs/codex-mcp/src/mcp/auth.rs index 9e093e8d88..6fac47c293 100644 --- a/codex-rs/codex-mcp/src/mcp/auth.rs +++ b/codex-rs/codex-mcp/src/mcp/auth.rs @@ -12,6 +12,7 @@ use codex_login::CodexAuth; use codex_rmcp_client::McpAuthState; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::OAuthProviderError; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::determine_streamable_http_auth_status; use codex_rmcp_client::discover_streamable_http_oauth; use futures::FutureExt; @@ -61,6 +62,7 @@ pub async fn oauth_login_support( transport: &McpServerTransportConfig, http_client: Arc, discovery_timeout: OAuthDiscoveryTimeout, + redirect_mode: StreamableHttpRedirectMode, ) -> McpOAuthLoginSupport { let Some(mut config) = oauth_login_candidate(transport) else { return McpOAuthLoginSupport::Unsupported; @@ -71,6 +73,7 @@ pub async fn oauth_login_support( config.env_http_headers.clone(), http_client, discovery_timeout, + redirect_mode, ) .await { @@ -108,8 +111,9 @@ pub async fn discover_supported_scopes( transport: &McpServerTransportConfig, http_client: Arc, discovery_timeout: OAuthDiscoveryTimeout, + redirect_mode: StreamableHttpRedirectMode, ) -> Option> { - match oauth_login_support(transport, http_client, discovery_timeout).await { + match oauth_login_support(transport, http_client, discovery_timeout, redirect_mode).await { McpOAuthLoginSupport::Supported(config) => config.discovered_scopes, McpOAuthLoginSupport::Unsupported | McpOAuthLoginSupport::Unknown(_) => None, } @@ -166,6 +170,11 @@ where { let futures = servers.into_iter().map(|(name, server)| { let name = name.clone(); + let redirect_mode = if server.is_agent_plugin() { + StreamableHttpRedirectMode::AgentPluginV1 + } else { + StreamableHttpRedirectMode::Legacy + }; let config = server.config().clone(); let runtime_context = runtime_context.clone(); let has_runtime_auth = matches!(&config.auth, McpServerAuth::ChatGpt) @@ -185,6 +194,7 @@ where keyring_backend_kind, has_runtime_auth, &runtime_context, + redirect_mode, ) .await { @@ -212,6 +222,7 @@ async fn compute_auth_status( keyring_backend_kind: AuthKeyringBackendKind, has_runtime_auth: bool, runtime_context: &McpRuntimeContext, + redirect_mode: StreamableHttpRedirectMode, ) -> Result { if !config.enabled { return Ok(McpAuthState::Unsupported); @@ -256,6 +267,7 @@ async fn compute_auth_status( keyring_backend_kind, http_client, discovery_timeout, + redirect_mode, ) .boxed() .await diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index b9c94326c5..6bddbf24c8 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -321,7 +321,14 @@ pub fn effective_mcp_servers_from_configured( } McpServerAuth::OAuth => {} } - (name, EffectiveMcpServer::configured(server)) + let agent_plugin = config + .mcp_server_catalog + .server(&name) + .is_some_and(|server| server.source().is_agent_plugin()); + ( + name, + EffectiveMcpServer::configured(server).with_agent_plugin(agent_plugin), + ) }) .collect::>(); if !host_owned_codex_apps_enabled(config, auth) { diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index f27319de20..b6f454647b 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -148,6 +148,7 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { PluginCapabilitySummary { config_name: "alpha@test".to_string(), display_name: "alpha-plugin".to_string(), + plugin_namespace: None, app_connector_ids: vec![AppConnectorId("connector_example".to_string())], mcp_server_names: vec!["alpha".to_string()], ..PluginCapabilitySummary::default() @@ -155,6 +156,7 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { PluginCapabilitySummary { config_name: "beta@test".to_string(), display_name: "beta-plugin".to_string(), + plugin_namespace: None, app_connector_ids: vec![ AppConnectorId("connector_example".to_string()), AppConnectorId("connector_gmail".to_string()), @@ -219,6 +221,7 @@ fn selected_mcp_attribution_does_not_join_an_unrelated_local_summary() { PluginCapabilitySummary { config_name: "shared-plugin-id".to_string(), display_name: "Local GitHub".to_string(), + plugin_namespace: None, mcp_server_names: vec!["github".to_string()], ..PluginCapabilitySummary::default() }, diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index ae831c1026..d5e25977c3 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -59,6 +59,7 @@ use codex_rmcp_client::LocalStdioServerLauncher; use codex_rmcp_client::McpProtocolMode; use codex_rmcp_client::RmcpClient; use codex_rmcp_client::StdioServerLauncher; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::ToolWithConnectorId; use codex_rmcp_client::is_authentication_required_error; use futures::future::BoxFuture; @@ -1084,7 +1085,12 @@ async fn make_rmcp_client( Ok(token) => token, Err(error) => return Err(error.into()), }; - RmcpClient::new_streamable_http_client_with_protocol_mode( + let redirect_mode = if server.is_agent_plugin() { + StreamableHttpRedirectMode::AgentPluginV1 + } else { + StreamableHttpRedirectMode::Legacy + }; + RmcpClient::new_streamable_http_client_with_protocol_mode_and_redirect_mode( oauth_credential_name.as_ref(), &url, resolved_bearer_token, @@ -1095,6 +1101,7 @@ async fn make_rmcp_client( http_client, runtime_auth_provider, protocol_mode, + redirect_mode, ) .await .map_err(StartupOutcomeError::from) diff --git a/codex-rs/codex-mcp/src/server.rs b/codex-rs/codex-mcp/src/server.rs index c00e786c06..e12f906e97 100644 --- a/codex-rs/codex-mcp/src/server.rs +++ b/codex-rs/codex-mcp/src/server.rs @@ -24,11 +24,20 @@ use tracing::warn; #[derive(Debug, Clone)] pub struct EffectiveMcpServer { config: McpServerConfig, + agent_plugin: bool, } impl EffectiveMcpServer { pub fn configured(config: McpServerConfig) -> Self { - Self { config } + Self { + config, + agent_plugin: false, + } + } + + pub fn with_agent_plugin(mut self, agent_plugin: bool) -> Self { + self.agent_plugin = agent_plugin; + self } pub fn config(&self) -> &McpServerConfig { @@ -42,6 +51,10 @@ impl EffectiveMcpServer { pub fn required(&self) -> bool { self.config.required } + + pub fn is_agent_plugin(&self) -> bool { + self.agent_plugin + } } pub(crate) fn has_explicit_http_authorization(config: &McpServerConfig) -> bool { @@ -92,6 +105,7 @@ pub(crate) struct McpServerConnectionIdentity { codex_apps_cache_identity: Option<(PathBuf, ConnectorRuntimeContextKey)>, client_elicitation_capability: ElicitationCapability, client_mcp_extensions: ClientMcpExtensions, + agent_plugin: bool, } impl McpServerConnectionIdentity { @@ -165,6 +179,7 @@ impl McpServerConnectionIdentity { codex_apps_cache_identity, client_elicitation_capability, client_mcp_extensions, + agent_plugin: server.is_agent_plugin(), } } @@ -193,6 +208,7 @@ impl McpServerConnectionIdentity { && self.codex_apps_cache_identity == other.codex_apps_cache_identity && self.client_elicitation_capability == other.client_elicitation_capability && self.client_mcp_extensions == other.client_mcp_extensions + && self.agent_plugin == other.agent_plugin } pub(crate) fn oauth_credentials(&self) -> Result<&Option, &String> { diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 471ebcc20f..c1ddf91a94 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -3,10 +3,11 @@ use crate::app_mcp_routing::apps_route_available; use crate::command_migration::migrated_command_skills_root; use crate::is_openai_curated_marketplace_name; use crate::manifest::PluginManifest; +use crate::manifest::PluginManifestFormat; use crate::manifest::PluginManifestHooks; use crate::manifest::PluginManifestMcpServers; use crate::manifest::PluginManifestPaths; -use crate::manifest::load_plugin_manifest; +use crate::manifest::load_plugin_manifest_with_format; use crate::marketplace::MarketplacePluginSource; use crate::marketplace::find_marketplace_plugin; use crate::marketplace::list_marketplaces_with_home; @@ -23,6 +24,7 @@ use crate::store::plugin_version_for_source_with_fallback_manifest; use codex_config::ConfigLayerStack; use codex_config::HooksFile; use codex_config::types::McpServerConfig; +use codex_config::types::McpServerTransportConfig; use codex_config::types::PluginConfig; use codex_config::types::PluginMcpServerConfig; use codex_connectors::parse_plugin_app_config; @@ -33,6 +35,7 @@ use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::LOCAL_FS; +use codex_mcp::parse_agent_plugin_mcp_config; use codex_mcp::parse_plugin_mcp_config; use codex_plugin::AppDeclaration; use codex_plugin::LoadedPlugin; @@ -831,6 +834,7 @@ async fn load_plugin( root, enabled: plugin.enabled, skill_roots: Vec::new(), + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: false, mcp_servers: HashMap::new(), @@ -873,12 +877,19 @@ async fn load_plugin( return loaded_plugin; } - let Some(manifest) = load_plugin_manifest(plugin_root.as_path()) else { + let Some(loaded_manifest) = load_plugin_manifest_with_format(plugin_root.as_path()) else { loaded_plugin.error = Some("missing or invalid plugin.json".to_string()); return loaded_plugin; }; + loaded_plugin.skill_discovery_mode = match loaded_manifest.format { + PluginManifestFormat::Legacy => SkillDiscoveryMode::Recursive, + PluginManifestFormat::AgentPlugin => SkillDiscoveryMode::DirectChildren, + }; + let manifest = loaded_manifest.manifest; let manifest_paths = &manifest.paths; + let plugin_data_root = store.plugin_data_root(&loaded_plugin_id); + let mcp_plugin_data_root = store.mcp_data_root(&loaded_plugin_id, loaded_manifest.format); loaded_plugin.plugin_namespace = Some(manifest.name.clone()); match scope { PluginLoadScope::AllCapabilities { @@ -890,40 +901,51 @@ async fn load_plugin( } => { loaded_plugin.manifest_name = Some(manifest.display_name().to_string()); loaded_plugin.manifest_description = manifest.description.clone(); - loaded_plugin.skill_roots = plugin_skill_roots(&plugin_root, manifest_paths); + loaded_plugin.skill_roots = + plugin_skill_roots(&plugin_root, manifest_paths, loaded_manifest.format); let plugin_identity = PluginIdentity { plugin_id: loaded_plugin_id.as_key(), remote_plugin_id: loaded_plugin.remote_plugin_id.clone(), }; - let resolved_skills = load_plugin_skills_with_identity( + let resolved_skills = load_plugin_skill_inventory( &plugin_root, &plugin_identity, &manifest, + loaded_manifest.format, *restriction_product, - skill_config_rules, *plugin_skill_snapshots, Arc::clone(root_scan_slots), ) - .await; + .await + .resolve(skill_config_rules); let has_enabled_skills = resolved_skills.has_enabled_skills(); loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths; loaded_plugin.has_enabled_skills = has_enabled_skills; - loaded_plugin.mcp_servers = load_plugin_mcp_servers_from_manifest( + loaded_plugin.mcp_servers = load_plugin_mcp_servers_from_manifest_with_format( plugin_root.as_path(), manifest_paths, Some(&plugin.mcp_servers), + Some(mcp_plugin_data_root.as_path()), + loaded_manifest.format, ) .await; - loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await; + if loaded_manifest.format == PluginManifestFormat::Legacy { + loaded_plugin.apps = load_plugin_apps(plugin_root.as_path()).await; + } } PluginLoadScope::HooksOnly => {} } - let (hook_sources, hook_load_warnings) = load_plugin_hooks( - &plugin_root, - &loaded_plugin_id, - &store.plugin_data_root(&loaded_plugin_id), - manifest_paths, - ); + let (hook_sources, hook_load_warnings) = + if loaded_manifest.format == PluginManifestFormat::AgentPlugin { + (Vec::new(), Vec::new()) + } else { + load_plugin_hooks( + &plugin_root, + &loaded_plugin_id, + &plugin_data_root, + manifest_paths, + ) + }; loaded_plugin.hook_sources = hook_sources; loaded_plugin.hook_load_warnings = hook_load_warnings; loaded_plugin @@ -961,7 +983,7 @@ impl PluginSkillInventory { ) } - fn resolve(self, skill_config_rules: &SkillConfigRules) -> ResolvedPluginSkills { + pub(crate) fn resolve(self, skill_config_rules: &SkillConfigRules) -> ResolvedPluginSkills { let disabled_skill_paths = resolve_disabled_skill_paths(&self.skills, skill_config_rules); ResolvedPluginSkills { skills: self.skills, @@ -1031,6 +1053,7 @@ pub(crate) async fn load_plugin_skills_with_identity( plugin_root, plugin_identity, manifest, + PluginManifestFormat::Legacy, restriction_product, plugin_skill_snapshots, root_scan_slots, @@ -1043,11 +1066,16 @@ pub(crate) async fn load_plugin_skill_inventory( plugin_root: &AbsolutePathBuf, plugin_identity: &PluginIdentity, manifest: &PluginManifest, + manifest_format: PluginManifestFormat, restriction_product: Option, plugin_skill_snapshots: Option<&PluginSkillSnapshots>, root_scan_slots: Arc, ) -> PluginSkillInventory { - let roots = plugin_skill_roots(plugin_root, &manifest.paths) + let discovery_mode = match manifest_format { + PluginManifestFormat::Legacy => SkillDiscoveryMode::Recursive, + PluginManifestFormat::AgentPlugin => SkillDiscoveryMode::DirectChildren, + }; + let roots = plugin_skill_roots(plugin_root, &manifest.paths, manifest_format) .into_iter() .map(|path| SkillRoot { path, @@ -1056,7 +1084,7 @@ pub(crate) async fn load_plugin_skill_inventory( plugin_identity: Some(plugin_identity.clone()), plugin_namespace: Some(manifest.name.clone()), plugin_root: Some(plugin_root.clone()), - discovery_mode: SkillDiscoveryMode::Recursive, + discovery_mode, }) .collect::>(); let outcome = load_skills_from_roots(roots, plugin_skill_snapshots, root_scan_slots).await; @@ -1098,15 +1126,18 @@ pub(crate) async fn load_plugin_skill_inventory( fn plugin_skill_roots( plugin_root: &AbsolutePathBuf, manifest_paths: &PluginManifestPaths, + manifest_format: PluginManifestFormat, ) -> Vec { let mut paths = if manifest_paths.skills.is_empty() { default_skill_roots(plugin_root) } else { manifest_paths.skills.clone() }; - let migrated_command_skills = migrated_command_skills_root(plugin_root); - if migrated_command_skills.is_dir() { - paths.push(migrated_command_skills); + if manifest_format == PluginManifestFormat::Legacy { + let migrated_command_skills = migrated_command_skills_root(plugin_root); + if migrated_command_skills.is_dir() { + paths.push(migrated_command_skills); + } } paths.sort_unstable(); paths.dedup(); @@ -1146,8 +1177,11 @@ fn default_mcp_config_paths(plugin_root: &Path) -> Vec { } pub async fn load_plugin_apps(plugin_root: &Path) -> Vec { - if let Some(manifest) = load_plugin_manifest(plugin_root) { - return load_plugin_apps_from_manifest(plugin_root, &manifest.paths).await; + if let Some(loaded_manifest) = load_plugin_manifest_with_format(plugin_root) { + if loaded_manifest.format == PluginManifestFormat::AgentPlugin { + return Vec::new(); + } + return load_plugin_apps_from_manifest(plugin_root, &loaded_manifest.manifest.paths).await; } load_apps_from_paths(plugin_root, default_app_config_paths(plugin_root)).await } @@ -1345,14 +1379,40 @@ pub async fn plugin_capability_summary_from_root( plugin_id: &PluginId, plugin_root: &AbsolutePathBuf, ) -> Option { - let manifest = load_plugin_manifest(plugin_root.as_path())?; + let loaded_manifest = load_plugin_manifest_with_format(plugin_root.as_path())?; + let manifest_format = loaded_manifest.format; + let manifest = loaded_manifest.manifest; + let plugin_identity = PluginIdentity { + plugin_id: plugin_id.as_key(), + remote_plugin_id: None, + }; let manifest_paths = &manifest.paths; - let has_skills = !plugin_skill_roots(plugin_root, manifest_paths).is_empty(); - let mut mcp_server_names = load_plugin_mcp_servers_from_manifest( + let has_skills = match manifest_format { + PluginManifestFormat::Legacy => { + !plugin_skill_roots(plugin_root, manifest_paths, manifest_format).is_empty() + } + PluginManifestFormat::AgentPlugin => { + !load_plugin_skill_inventory( + plugin_root, + &plugin_identity, + &manifest, + manifest_format, + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(1)), + ) + .await + .skills + .is_empty() + } + }; + let mut mcp_server_names = load_plugin_mcp_servers_from_manifest_with_format( plugin_root.as_path(), manifest_paths, /*plugin_policy*/ None, + /*plugin_data_root*/ None, + manifest_format, ) .await .into_keys() @@ -1360,16 +1420,17 @@ pub async fn plugin_capability_summary_from_root( mcp_server_names.sort_unstable(); mcp_server_names.dedup(); - let app_declarations = load_apps_from_paths( - plugin_root.as_path(), - plugin_app_config_paths(plugin_root.as_path(), manifest_paths), - ) - .await; + let app_declarations = if manifest_format == PluginManifestFormat::AgentPlugin { + Vec::new() + } else { + load_plugin_apps_from_manifest(plugin_root.as_path(), manifest_paths).await + }; let app_connector_ids = app_connector_ids_from_declarations(&app_declarations); Some(PluginCapabilitySummary { config_name: plugin_id.as_key(), display_name: plugin_id.plugin_name.clone(), + plugin_namespace: Some(manifest.name.clone()), description: None, has_skills, mcp_server_names, @@ -1426,17 +1487,26 @@ async fn load_declared_plugin_mcp_servers( plugin_root: &Path, plugin_policy: Option<&HashMap>, ) -> HashMap { - let Some(manifest) = load_plugin_manifest(plugin_root) else { + let Some(loaded_manifest) = load_plugin_manifest_with_format(plugin_root) else { return HashMap::new(); }; - load_plugin_mcp_servers_from_manifest(plugin_root, &manifest.paths, plugin_policy).await + load_plugin_mcp_servers_from_manifest_with_format( + plugin_root, + &loaded_manifest.manifest.paths, + plugin_policy, + /*plugin_data_root*/ None, + loaded_manifest.format, + ) + .await } -pub(crate) async fn load_plugin_mcp_servers_from_manifest( +pub(crate) async fn load_plugin_mcp_servers_from_manifest_with_format( plugin_root: &Path, manifest_paths: &PluginManifestPaths, plugin_policy: Option<&HashMap>, + plugin_data_root: Option<&Path>, + manifest_format: PluginManifestFormat, ) -> HashMap { let mut mcp_servers = HashMap::new(); match &manifest_paths.mcp_servers { @@ -1457,7 +1527,13 @@ pub(crate) async fn load_plugin_mcp_servers_from_manifest( } Some(PluginManifestMcpServers::Path(_)) | None => { for mcp_config_path in plugin_mcp_config_paths(plugin_root, manifest_paths) { - let plugin_mcp = load_mcp_servers_from_file(plugin_root, &mcp_config_path).await; + let plugin_mcp = load_mcp_servers_from_file( + plugin_root, + plugin_data_root, + manifest_format, + &mcp_config_path, + ) + .await; for (name, mut config) in plugin_mcp.mcp_servers { if let Some(policy) = plugin_policy.and_then(|policy| policy.get(&name)) { apply_plugin_mcp_server_policy(&mut config, policy); @@ -1480,12 +1556,74 @@ pub(crate) async fn load_plugin_mcp_servers_from_manifest( async fn load_mcp_servers_from_file( plugin_root: &Path, + plugin_data_root: Option<&Path>, + manifest_format: PluginManifestFormat, mcp_config_path: &AbsolutePathBuf, ) -> PluginMcpDiscovery { + let is_agent_plugin_mcp = manifest_format == PluginManifestFormat::AgentPlugin; + if is_agent_plugin_mcp { + match tokio::fs::symlink_metadata(mcp_config_path.as_path()).await { + Ok(metadata) if metadata.file_type().is_symlink() || !metadata.is_file() => { + warn!( + path = %mcp_config_path.display(), + "Agent Plugins MCP config is not a regular file; disabling MCP" + ); + return PluginMcpDiscovery::default(); + } + Ok(_) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + return PluginMcpDiscovery::default(); + } + Err(err) => { + warn!( + path = %mcp_config_path.display(), + "failed to inspect Agent Plugins MCP config; disabling MCP: {err}" + ); + return PluginMcpDiscovery::default(); + } + } + let resolved_root = match tokio::fs::canonicalize(plugin_root).await { + Ok(path) => path, + Err(err) => { + warn!( + plugin = %plugin_root.display(), + "failed to resolve Agent Plugins root; disabling MCP: {err}" + ); + return PluginMcpDiscovery::default(); + } + }; + let resolved_config = match tokio::fs::canonicalize(mcp_config_path.as_path()).await { + Ok(path) => path, + Err(err) => { + warn!( + path = %mcp_config_path.display(), + "failed to resolve Agent Plugins MCP config; disabling MCP: {err}" + ); + return PluginMcpDiscovery::default(); + } + }; + if !resolved_config.starts_with(&resolved_root) { + warn!( + plugin = %plugin_root.display(), + path = %mcp_config_path.display(), + "Agent Plugins MCP config resolves outside the plugin root; disabling MCP" + ); + return PluginMcpDiscovery::default(); + } + } let Ok(contents) = tokio::fs::read_to_string(mcp_config_path.as_path()).await else { return PluginMcpDiscovery::default(); }; - let parsed = match parse_plugin_mcp_config(plugin_root, &contents) { + let fallback_data_root = plugin_root.join(".plugin-data"); + let mut parsed = match if is_agent_plugin_mcp { + parse_agent_plugin_mcp_config( + plugin_root, + plugin_data_root.unwrap_or(&fallback_data_root), + &contents, + ) + } else { + parse_plugin_mcp_config(plugin_root, &contents) + } { Ok(parsed) => parsed, Err(err) => { warn!( @@ -1495,6 +1633,23 @@ async fn load_mcp_servers_from_file( return PluginMcpDiscovery::default(); } }; + if is_agent_plugin_mcp + && let Some(plugin_data_root) = plugin_data_root + && parsed + .servers + .values() + .any(|server| matches!(&server.transport, McpServerTransportConfig::Stdio { .. })) + && let Err(err) = tokio::fs::create_dir_all(plugin_data_root).await + { + warn!( + plugin = %plugin_root.display(), + path = %plugin_data_root.display(), + "failed to create Agent Plugins data directory; disabling stdio MCP servers: {err}" + ); + parsed.servers.retain(|_, server| { + !matches!(&server.transport, McpServerTransportConfig::Stdio { .. }) + }); + } for error in parsed.errors { warn!( plugin = %plugin_root.display(), diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index cfc5c892da..5150a6cec2 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -1,5 +1,6 @@ use super::*; use crate::manifest::load_plugin_manifest; +use crate::manifest::load_plugin_manifest_with_format; use crate::test_support::write_file; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; @@ -7,6 +8,7 @@ use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_plugin::PluginId; +use codex_utils_plugins::AGENT_PLUGIN_SCHEMA_URI; use pretty_assertions::assert_eq; use tempfile::TempDir; @@ -25,6 +27,177 @@ fn user_layer(path: AbsolutePathBuf, config: &str) -> ConfigLayerEntry { ) } +#[tokio::test] +async fn agent_plugin_overlay_apps_are_not_runtime_active() { + let temp_dir = TempDir::new().expect("tempdir"); + let plugin_root = temp_dir.path().join("plugin"); + write_file( + &plugin_root.join("plugin.json"), + &format!(r#"{{"$schema":"{AGENT_PLUGIN_SCHEMA_URI}","name":"plugin"}}"#), + ); + write_file( + &plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"plugin","apps":"./.app.json"}"#, + ); + write_file( + &plugin_root.join(".app.json"), + r#"{"apps":{"example":{"id":"connector_example"}}}"#, + ); + + assert!(load_plugin_apps(&plugin_root).await.is_empty()); +} + +#[cfg(unix)] +#[tokio::test] +async fn agent_plugin_mcp_rejects_config_symlink_outside_plugin_root() { + let temp_dir = TempDir::new().expect("tempdir"); + let plugin_root = temp_dir.path().join("plugin"); + let outside_config = temp_dir.path().join("outside-mcp.json"); + fs::create_dir_all(&plugin_root).expect("create plugin root"); + fs::write( + plugin_root.join("plugin.json"), + format!(r#"{{"$schema":"{AGENT_PLUGIN_SCHEMA_URI}","name":"plugin"}}"#), + ) + .expect("write Agent Plugins manifest"); + fs::write( + &outside_config, + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"outside":{"type":"stdio","command":"echo"}}}"#, + ) + .expect("write outside MCP config"); + std::os::unix::fs::symlink(&outside_config, plugin_root.join("mcp.json")) + .expect("create MCP symlink"); + let config_path = AbsolutePathBuf::from_absolute_path(plugin_root.join("mcp.json")) + .expect("absolute MCP path"); + + let discovered = load_mcp_servers_from_file( + &plugin_root, + /*plugin_data_root*/ None, + PluginManifestFormat::AgentPlugin, + &config_path, + ) + .await; + + assert!(discovered.mcp_servers.is_empty()); +} + +#[tokio::test] +async fn agent_plugin_mcp_rejects_present_nonregular_config() { + let temp_dir = TempDir::new().expect("tempdir"); + let plugin_root = temp_dir.path().join("plugin"); + let config_path = plugin_root.join("mcp.json"); + fs::create_dir_all(&config_path).expect("create nonregular MCP config"); + + let discovered = load_mcp_servers_from_file( + &plugin_root, + /*plugin_data_root*/ None, + PluginManifestFormat::AgentPlugin, + &AbsolutePathBuf::from_absolute_path(config_path).expect("absolute MCP path"), + ) + .await; + + assert!(discovered.mcp_servers.is_empty()); +} + +#[tokio::test] +async fn legacy_manifest_can_point_at_root_mcp_json() { + let temp_dir = TempDir::new().expect("tempdir"); + let plugin_root = temp_dir.path().join("plugin"); + fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create manifest directory"); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"plugin","mcpServers":"./mcp.json"}"#, + ) + .expect("write legacy manifest"); + fs::write( + plugin_root.join("mcp.json"), + r#"{"mcpServers":{"legacy":{"command":"echo"}}}"#, + ) + .expect("write legacy MCP config"); + let manifest = load_plugin_manifest(&plugin_root).expect("load legacy manifest"); + + let discovered = load_plugin_mcp_servers_from_manifest_with_format( + &plugin_root, + &manifest.paths, + /*plugin_policy*/ None, + /*plugin_data_root*/ None, + PluginManifestFormat::Legacy, + ) + .await; + + assert_eq!( + discovered.keys().collect::>(), + vec![&"legacy".to_string()] + ); +} + +#[tokio::test] +async fn installed_agent_plugin_uses_isolated_data_root_for_stdio_mcp() { + let temp_dir = TempDir::new().expect("tempdir"); + let plugin_root = temp_dir.path().join("plugins/cache/c/a-b/local"); + write_file( + &plugin_root.join("plugin.json"), + &format!(r#"{{"$schema":"{AGENT_PLUGIN_SCHEMA_URI}","name":"a-b"}}"#), + ); + write_file( + &plugin_root.join("mcp.json"), + r#"{ + "$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json", + "mcpServers": { + "example": { + "type": "stdio", + "command": "echo" + } + } +}"#, + ); + let stack = ConfigLayerStack::new( + vec![user_layer( + user_config_path(&temp_dir, "config.toml"), + "[plugins.\"a-b@c\"]\nenabled = true\n", + )], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + let store = PluginStore::new(temp_dir.path().to_path_buf()); + + let plugins = load_plugins_from_layer_stack( + &stack, + RemoteInstalledPluginsSnapshot::default(), + &store, + /*plugin_skill_snapshots*/ None, + Some(Product::Codex), + /*remote_global_catalog_active*/ false, + Arc::new(Semaphore::new(MAX_CONCURRENT_ROOT_SCANS)), + ) + .await; + + let expected_data_root = temp_dir + .path() + .join("plugins") + .join("data") + .join("agent-plugins") + .join("6920dd17774030852d11d1b94758fcaae4f894c7b2f36301ed174bc3b33e0743"); + let expected_data_root = AbsolutePathBuf::from_absolute_path(expected_data_root) + .expect("absolute Agent Plugin data root") + .canonicalize() + .expect("canonical Agent Plugin data root"); + let server = plugins + .first() + .and_then(|plugin| plugin.mcp_servers.get("example")) + .expect("Agent plugin stdio MCP server"); + let McpServerTransportConfig::Stdio { env, .. } = &server.transport else { + panic!("expected stdio MCP server"); + }; + assert_eq!( + env.as_ref() + .and_then(|env| env.get("PLUGIN_DATA")) + .map(String::as_str), + expected_data_root.as_path().to_str() + ); + assert!(expected_data_root.as_path().is_dir()); +} + #[test] fn configured_plugins_from_stack_merges_user_layers() { let temp_dir = TempDir::new().expect("tempdir"); @@ -265,7 +438,8 @@ fn write_hook_file(plugin_root: &AbsolutePathBuf, relative_path: &str, event: &s } fn load_sources(plugin_root: &AbsolutePathBuf) -> (Vec, Vec) { - let manifest = load_plugin_manifest(plugin_root.as_path()).expect("manifest"); + let loaded_manifest = + load_plugin_manifest_with_format(plugin_root.as_path()).expect("manifest"); let plugin_data_root = AbsolutePathBuf::try_from( plugin_root .as_path() @@ -278,7 +452,7 @@ fn load_sources(plugin_root: &AbsolutePathBuf) -> (Vec, Vec for PluginCapabilitySummary { Self { config_name: value.id, display_name: value.name, + plugin_namespace: None, description: prompt_safe_plugin_description(value.description.as_deref()), has_skills, mcp_server_names: value.mcp_server_names, @@ -1997,18 +2000,24 @@ impl PluginsManager { "path does not exist or is not a directory".to_string(), )); } - let manifest = + let loaded_manifest = if codex_utils_plugins::find_plugin_manifest_path(source_path.as_path()).is_some() { - load_plugin_manifest(source_path.as_path()) + load_plugin_manifest_with_format(source_path.as_path()) } else { plugin .manifest_fallback .as_ref() .and_then(|fallback| fallback.parse_for_plugin_root(source_path.as_path())) + .map(|manifest| crate::manifest::LoadedPluginManifest { + manifest, + format: PluginManifestFormat::Legacy, + }) } .ok_or_else(|| { MarketplaceError::InvalidPlugin("missing or invalid plugin.json".to_string()) })?; + let manifest_format = loaded_manifest.format; + let manifest = loaded_manifest.manifest; let description = manifest.description.clone(); let marketplace_category = plugin .interface @@ -2022,21 +2031,27 @@ impl PluginsManager { plugin_id: plugin_id.as_key(), remote_plugin_id: self.remote_plugin_id_for(&plugin_id), }; - let resolved_skills = load_plugin_skills_with_identity( + let skill_config_rules = codex_core_skills::config_rules::skill_config_rules_from_stack( + &config.config_layer_stack, + ); + let resolved_skills = load_plugin_skill_inventory( &source_path, &plugin_identity, &manifest, + manifest_format, self.restriction_product, - &codex_core_skills::config_rules::skill_config_rules_from_stack( - &config.config_layer_stack, - ), /*plugin_skill_snapshots*/ None, Arc::clone(&self.skill_root_scan_slots), ) - .await; + .await + .resolve(&skill_config_rules); let plugin_data_root = self.store.plugin_data_root(&plugin_id); - let (hook_sources, _hook_load_warnings) = - load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths); + let (hook_sources, _hook_load_warnings) = if manifest_format == PluginManifestFormat::Legacy + { + load_plugin_hooks(&source_path, &plugin_id, &plugin_data_root, &manifest.paths) + } else { + (Vec::new(), Vec::new()) + }; let hooks = plugin_hook_declarations(&hook_sources) .into_iter() .map(|hook| PluginHookSummary { @@ -2045,15 +2060,22 @@ impl PluginsManager { }) .collect(); let auth_mode = self.auth_mode(); - let mut app_declarations = - load_plugin_apps_from_manifest(source_path.as_path(), &manifest.paths).await; - let mut mcp_servers = load_plugin_mcp_servers_from_manifest( + let mut app_declarations = if manifest_format == PluginManifestFormat::Legacy { + load_plugin_apps_from_manifest(source_path.as_path(), &manifest.paths).await + } else { + Vec::new() + }; + let mcp_data_root = (manifest_format == PluginManifestFormat::AgentPlugin) + .then(|| self.store.mcp_data_root(&plugin_id, manifest_format)); + let mut mcp_servers = load_plugin_mcp_servers_from_manifest_with_format( source_path.as_path(), &manifest.paths, /*plugin_policy*/ None, + mcp_data_root.as_deref(), + manifest_format, ) .await; - if auth_mode.is_some() { + if manifest_format == PluginManifestFormat::Legacy && auth_mode.is_some() { apply_app_mcp_routing_policy( &mut app_declarations, &mut mcp_servers, diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index b48c30bfd7..5182e0f57a 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -52,6 +52,7 @@ use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::HostSkillsService; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; +use codex_utils_plugins::SkillDiscoveryMode; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; @@ -675,6 +676,7 @@ async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() { PluginCapabilitySummary { config_name: "docs@test".to_string(), display_name: "docs".to_string(), + plugin_namespace: Some("docs".to_string()), description: None, has_skills: false, mcp_server_names: vec!["docs".to_string()], @@ -683,6 +685,7 @@ async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() { PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: None, has_skills: false, mcp_server_names: Vec::new(), @@ -705,6 +708,7 @@ async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() { PluginCapabilitySummary { config_name: "docs@test".to_string(), display_name: "docs".to_string(), + plugin_namespace: Some("docs".to_string()), description: None, has_skills: false, mcp_server_names: vec!["docs".to_string()], @@ -713,6 +717,7 @@ async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() { PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: None, has_skills: false, mcp_server_names: vec!["sample".to_string()], @@ -739,7 +744,11 @@ fn write_plugin_with_version( format!(r#"{{"name":"{manifest_name}"{version}}}"#), ) .unwrap(); - fs::write(plugin_root.join("skills/SKILL.md"), "skill").unwrap(); + fs::write( + plugin_root.join("skills/SKILL.md"), + format!("---\nname: {manifest_name}-skill\ndescription: test skill\n---\n\n# Test skill\n"), + ) + .unwrap(); fs::write(plugin_root.join(".mcp.json"), r#"{"mcpServers":{}}"#).unwrap(); } @@ -931,6 +940,7 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { root: AbsolutePathBuf::try_from(plugin_root.clone()).unwrap(), enabled: true, skill_roots: vec![plugin_root.join("skills").abs()], + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: true, mcp_servers: HashMap::from([( @@ -973,6 +983,7 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { &[PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: Some("Plugin that includes the sample MCP server and Skills".to_string(),), has_skills: true, mcp_server_names: vec!["sample".to_string()], @@ -1232,6 +1243,7 @@ async fn installed_plugin_telemetry_metadata_collects_capabilities() { capability_summary: Some(PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: None, has_skills: true, mcp_server_names: Vec::new(), @@ -1264,6 +1276,7 @@ async fn installed_plugin_telemetry_metadata_resolves_persisted_remote_identity( capability_summary: Some(PluginCapabilitySummary { config_name: "linear@openai-curated-remote".to_string(), display_name: "linear".to_string(), + plugin_namespace: Some("linear".to_string()), description: None, has_skills: true, mcp_server_names: Vec::new(), @@ -1317,6 +1330,7 @@ async fn installed_plugin_telemetry_metadata_prefers_remote_snapshot_identity() capability_summary: Some(PluginCapabilitySummary { config_name: "linear@openai-curated-remote".to_string(), display_name: "linear".to_string(), + plugin_namespace: Some("linear".to_string()), description: None, has_skills: true, mcp_server_names: Vec::new(), @@ -1354,6 +1368,7 @@ fn capability_summary_telemetry_metadata_uses_local_identity() { let summary = PluginCapabilitySummary { config_name: "linear@openai-curated-remote".to_string(), display_name: "Linear".to_string(), + plugin_namespace: Some("linear".to_string()), description: Some("Track work".to_string()), has_skills: true, mcp_server_names: vec!["linear".to_string()], @@ -1387,6 +1402,7 @@ fn capability_summary_telemetry_metadata_resolves_persisted_remote_identity() { let summary = PluginCapabilitySummary { config_name: "linear@openai-curated-remote".to_string(), display_name: "Linear".to_string(), + plugin_namespace: Some("linear".to_string()), description: Some("Track work".to_string()), has_skills: true, mcp_server_names: vec!["linear".to_string()], @@ -1772,6 +1788,7 @@ enabled = true &[PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: None, has_skills: true, mcp_server_names: Vec::new(), @@ -1817,6 +1834,7 @@ async fn plugin_telemetry_metadata_uses_default_mcp_config_path() { Some(PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: Some("sample".to_string()), description: None, has_skills: false, mcp_server_names: vec!["sample".to_string()], @@ -1858,6 +1876,7 @@ async fn plugin_capability_summary_uses_manifest_mcp_server_objects() { Some(PluginCapabilitySummary { config_name: "counter-sample@test".to_string(), display_name: "counter-sample".to_string(), + plugin_namespace: Some("counter-sample".to_string()), description: None, has_skills: false, mcp_server_names: vec!["counter".to_string()], @@ -2273,7 +2292,6 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() { interface: None, }; let plugin_id = PluginId::parse("sample@test").expect("plugin id should parse"); - let resolved = load_plugin_skills( &plugin_root, &plugin_id, @@ -2502,6 +2520,7 @@ async fn load_plugins_preserves_disabled_plugins_without_effective_contributions root: AbsolutePathBuf::try_from(plugin_root).unwrap(), enabled: false, skill_roots: Vec::new(), + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: false, mcp_servers: HashMap::new(), @@ -2679,6 +2698,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { root: AbsolutePathBuf::try_from(codex_home.path().join(dir_name)).unwrap(), enabled: true, skill_roots: Vec::new(), + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: false, mcp_servers: HashMap::new(), @@ -2690,6 +2710,12 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { let summary = |config_name: &str, display_name: &str| PluginCapabilitySummary { config_name: config_name.to_string(), display_name: display_name.to_string(), + plugin_namespace: Some( + config_name + .split_once('@') + .map_or(config_name, |(name, _)| name) + .to_string(), + ), description: None, ..PluginCapabilitySummary::default() }; @@ -3985,6 +4011,96 @@ plugins = true ); } +#[tokio::test] +async fn agent_plugin_read_and_tool_suggestions_use_portable_capabilities_only() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + let plugin_root = repo_root.join("agent-plugin"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + write_file( + &repo_root.join(".agents/plugins/marketplace.json"), + r#"{"name":"debug","plugins":[{"name":"agent-plugin","source":"./agent-plugin"}]}"#, + ); + write_file( + &plugin_root.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"agent.tools"}"#, + ); + write_file( + &plugin_root.join("skills/direct/SKILL.md"), + "---\nname: direct\ndescription: Direct skill\n---\n", + ); + write_file( + &plugin_root.join("skills/group/nested/SKILL.md"), + "---\nname: nested\ndescription: Nested skill\n---\n", + ); + write_file( + &plugin_root.join("mcp.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/mcp.schema.json","mcpServers":{"portable":{"type":"stdio","command":"echo"}}}"#, + ); + write_file( + &plugin_root.join(".codex-plugin/plugin.json"), + r#"{"apps":"./.app.json","hooks":"./hooks/hooks.json"}"#, + ); + write_file( + &plugin_root.join(".app.json"), + r#"{"apps":{"legacy":{"id":"connector_legacy"}}}"#, + ); + write_file( + &plugin_root.join("hooks/hooks.json"), + r#"{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"echo legacy"}]}]}}"#, + ); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + "[features]\nplugins = true\n", + ); + + let config = load_config(tmp.path(), &repo_root).await; + let manager = PluginsManager::new(tmp.path().to_path_buf()); + let plugin = manager + .list_marketplaces_for_config( + &config, + &[AbsolutePathBuf::try_from(repo_root).unwrap()], + /*include_openai_curated*/ false, + ) + .unwrap() + .marketplaces + .into_iter() + .find(|marketplace| marketplace.name == "debug") + .unwrap() + .plugins + .into_iter() + .find(|plugin| plugin.name == "agent-plugin") + .unwrap(); + let detail = manager + .read_plugin_detail_for_marketplace_plugin(&config, "debug", plugin.clone()) + .await + .unwrap(); + let suggestion = manager + .tool_suggest_metadata_for_marketplace_plugin( + "debug", + &plugin, + &SkillConfigRules::default(), + ) + .await + .unwrap(); + + assert_eq!( + detail + .skills + .iter() + .map(|skill| skill.name.as_str()) + .collect::>(), + vec!["agent.tools:direct"] + ); + assert_eq!(detail.mcp_server_names, vec!["portable"]); + assert!(detail.apps.is_empty()); + assert!(detail.hooks.is_empty()); + assert!(suggestion.has_skills); + assert_eq!(suggestion.mcp_server_names, vec!["portable"]); + assert!(suggestion.app_connector_ids.is_empty()); +} + #[tokio::test] async fn read_plugin_for_config_does_not_fallback_from_invalid_plugin_manifest() { let tmp = tempfile::tempdir().unwrap(); diff --git a/codex-rs/core-plugins/src/manifest.rs b/codex-rs/core-plugins/src/manifest.rs index 3503a8d80d..40d724734b 100644 --- a/codex-rs/core-plugins/src/manifest.rs +++ b/codex-rs/core-plugins/src/manifest.rs @@ -31,6 +31,17 @@ pub type PluginManifestPaths = codex_plugin::manifest::PluginManifestPaths; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum PluginManifestFormat { + Legacy, + AgentPlugin, +} + +pub(crate) struct LoadedPluginManifest { + pub manifest: PluginManifest, + pub format: PluginManifestFormat, +} + #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase")] struct RawPluginManifest { @@ -143,6 +154,15 @@ enum RawPluginManifestHooks { /// Loads a plugin manifest from the local host filesystem. pub fn load_plugin_manifest(plugin_root: &Path) -> Option { + load_plugin_manifest_with_format(plugin_root).map(|loaded| loaded.manifest) +} + +pub fn is_agent_plugin_manifest(plugin_root: &Path) -> bool { + load_plugin_manifest_with_format(plugin_root) + .is_some_and(|loaded| loaded.format == PluginManifestFormat::AgentPlugin) +} + +pub(crate) fn load_plugin_manifest_with_format(plugin_root: &Path) -> Option { let manifest_path = find_plugin_manifest_path(plugin_root)?; let contents = fs::read_to_string(&manifest_path).ok()?; let is_agent_plugin = manifest_path == plugin_root.join(AGENT_PLUGIN_MANIFEST_RELATIVE_PATH); @@ -162,7 +182,14 @@ pub fn load_plugin_manifest(plugin_root: &Path) -> Option { .as_ref() .map(|(path, contents)| (path.as_path(), contents.as_str())), ) { - Ok(manifest) => Some(manifest), + Ok(manifest) => Some(LoadedPluginManifest { + manifest, + format: if is_agent_plugin { + PluginManifestFormat::AgentPlugin + } else { + PluginManifestFormat::Legacy + }, + }), Err(err) => { tracing::warn!( path = %manifest_path.display(), diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 27de631aea..ed3f4df8d9 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -1524,6 +1524,7 @@ pub async fn resolve_remote_plugin_uninstall_target( let fallback_capability_summary = PluginCapabilitySummary { config_name: plugin_id.as_key(), display_name: plugin.release.display_name, + plugin_namespace: Some(plugin_id.plugin_name.clone()), description: prompt_safe_plugin_description(Some(&plugin.release.description)), has_skills: !plugin.release.skills.is_empty(), mcp_server_names, diff --git a/codex-rs/core-plugins/src/script_attribution_tests.rs b/codex-rs/core-plugins/src/script_attribution_tests.rs index 0455002d25..641806d166 100644 --- a/codex-rs/core-plugins/src/script_attribution_tests.rs +++ b/codex-rs/core-plugins/src/script_attribution_tests.rs @@ -10,6 +10,7 @@ use crate::test_support::write_curated_plugin_sha_with; use crate::test_support::write_openai_api_curated_marketplace; use crate::test_support::write_openai_curated_marketplace; use codex_plugin::PluginLoadOutcome; +use codex_utils_plugins::SkillDiscoveryMode; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::collections::HashSet; @@ -30,6 +31,7 @@ fn loaded_plugin(config_name: &str, root: &Path, enabled: bool) -> LoadedPlugin root: path(root), enabled, skill_roots: Vec::new(), + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: false, mcp_servers: HashMap::new(), diff --git a/codex-rs/core-plugins/src/store.rs b/codex-rs/core-plugins/src/store.rs index 55458a2c2e..fc5d37ce80 100644 --- a/codex-rs/core-plugins/src/store.rs +++ b/codex-rs/core-plugins/src/store.rs @@ -1,5 +1,6 @@ use crate::command_migration::migrate_plugin_commands; use crate::manifest::PluginManifest; +use crate::manifest::PluginManifestFormat; use crate::manifest::load_plugin_manifest; use crate::manifest::parse_plugin_manifest; use codex_plugin::PluginId; @@ -24,6 +25,7 @@ use std::path::PathBuf; pub const DEFAULT_PLUGIN_VERSION: &str = "local"; pub const PLUGINS_CACHE_DIR: &str = "plugins/cache"; pub const PLUGINS_DATA_DIR: &str = "plugins/data"; +const AGENT_PLUGINS_DATA_DIR: &str = "agent-plugins"; const REMOTE_PLUGIN_INSTALL_METADATA_FILE: &str = ".codex-remote-plugin-install.json"; const REMOTE_PLUGIN_INSTALL_METADATA_SCHEMA_VERSION: u8 = 1; const DEFAULT_AGENT_PLUGIN_VERSION: &str = "1.0.0"; @@ -143,6 +145,27 @@ impl PluginStore { )) } + pub(crate) fn agent_plugin_data_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf { + let mut digest = Sha256::new(); + digest.update(plugin_id.marketplace_name.as_bytes()); + digest.update([0]); + digest.update(plugin_id.plugin_name.as_bytes()); + self.data_root + .join(AGENT_PLUGINS_DATA_DIR) + .join(hex_prefix(&digest.finalize(), /*count*/ 32)) + } + + pub(crate) fn mcp_data_root( + &self, + plugin_id: &PluginId, + manifest_format: PluginManifestFormat, + ) -> AbsolutePathBuf { + match manifest_format { + PluginManifestFormat::AgentPlugin => self.agent_plugin_data_root(plugin_id), + PluginManifestFormat::Legacy => self.plugin_data_root(plugin_id), + } + } + pub fn active_plugin_version(&self, plugin_id: &PluginId) -> Option { let mut discovered_versions = fs::read_dir(self.plugin_base_root(plugin_id).as_path()) .ok()? diff --git a/codex-rs/core-plugins/src/store_tests.rs b/codex-rs/core-plugins/src/store_tests.rs index 380ef84736..27b8168fc0 100644 --- a/codex-rs/core-plugins/src/store_tests.rs +++ b/codex-rs/core-plugins/src/store_tests.rs @@ -162,6 +162,28 @@ fn plugin_data_root_derives_path_from_key() { ); } +#[test] +fn agent_plugin_data_root_is_stable_and_unambiguous() { + let tmp = tempdir().unwrap(); + let store = PluginStore::new(tmp.path().to_path_buf()); + let first = PluginId::new("a-b".to_string(), "c".to_string()).unwrap(); + let second = PluginId::new("a".to_string(), "b-c".to_string()).unwrap(); + + let first_root = store.agent_plugin_data_root(&first); + let second_root = store.agent_plugin_data_root(&second); + let expected_parent = tmp.path().join("plugins/data/agent-plugins"); + + assert_ne!(first_root, second_root); + assert_eq!( + first_root.as_path(), + expected_parent.join("6920dd17774030852d11d1b94758fcaae4f894c7b2f36301ed174bc3b33e0743") + ); + assert_eq!( + second_root.as_path(), + expected_parent.join("fa89b988ebbe54a68fdcbeb87fb913a5238d482084a3cee49a86288c2d45fa90") + ); +} + #[test] fn install_with_version_uses_requested_cache_version() { let tmp = tempdir().unwrap(); diff --git a/codex-rs/core-plugins/src/tool_suggest_metadata.rs b/codex-rs/core-plugins/src/tool_suggest_metadata.rs index 0871a7436c..fbedcb5707 100644 --- a/codex-rs/core-plugins/src/tool_suggest_metadata.rs +++ b/codex-rs/core-plugins/src/tool_suggest_metadata.rs @@ -21,7 +21,8 @@ use crate::loader::load_plugin_mcp_servers; use crate::loader::load_plugin_skill_inventory; use crate::manager::ConfiguredMarketplacePlugin; use crate::manager::remote_plugin_install_required_description; -use crate::manifest::load_plugin_manifest; +use crate::manifest::PluginManifestFormat; +use crate::manifest::load_plugin_manifest_with_format; use crate::marketplace::MarketplaceError; use crate::marketplace::MarketplacePluginSource; @@ -53,6 +54,7 @@ struct PluginArtifactIdentity { pub(crate) struct ToolSuggestMetadataFragment { config_name: String, display_name: String, + plugin_namespace: Option, description: Option, mcp_server_names: Vec, app_declarations: Vec, @@ -86,6 +88,7 @@ impl ToolSuggestMetadataFragment { PluginCapabilitySummary { config_name: self.config_name.clone(), display_name: self.display_name.clone(), + plugin_namespace: self.plugin_namespace.clone(), description: self.description.clone(), has_skills: self .skill_inventory @@ -206,6 +209,7 @@ async fn load_plugin_metadata( return Ok(Arc::new(ToolSuggestMetadataFragment { config_name: plugin.id.clone(), display_name: plugin.name.clone(), + plugin_namespace: Some(plugin.name.clone()), description: prompt_safe_plugin_description(Some( &remote_plugin_install_required_description(&plugin.source), )), @@ -217,16 +221,18 @@ async fn load_plugin_metadata( if !plugin_root.as_path().is_dir() { return Err("path does not exist or is not a directory".to_string()); } - let manifest = load_plugin_manifest(plugin_root.as_path()) + let loaded_manifest = load_plugin_manifest_with_format(plugin_root.as_path()) .ok_or_else(|| "missing or invalid plugin.json".to_string())?; let plugin_identity = PluginIdentity { plugin_id: plugin_id.as_key(), remote_plugin_id: None, }; + let manifest = loaded_manifest.manifest; let skill_inventory = load_plugin_skill_inventory( plugin_root, &plugin_identity, &manifest, + loaded_manifest.format, restriction_product, /*plugin_skill_snapshots*/ None, root_scan_slots, @@ -239,11 +245,16 @@ async fn load_plugin_metadata( .collect::>(); mcp_server_names.sort_unstable(); mcp_server_names.dedup(); - let app_declarations = load_plugin_apps(plugin_root.as_path()).await; + let app_declarations = if loaded_manifest.format == PluginManifestFormat::AgentPlugin { + Vec::new() + } else { + load_plugin_apps(plugin_root.as_path()).await + }; Ok(Arc::new(ToolSuggestMetadataFragment { config_name: plugin.id.clone(), display_name: plugin.name.clone(), + plugin_namespace: Some(manifest.name.clone()), description: prompt_safe_plugin_description(manifest.description.as_deref()), mcp_server_names, app_declarations, diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index 61b16ab236..e0acd97111 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -27,6 +27,7 @@ codex-skills = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-path-uri = { workspace = true } codex-utils-plugins = { workspace = true } +codex-utils-string = { workspace = true } dunce = { workspace = true } futures = { workspace = true } serde = { workspace = true, features = ["derive"] } diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 2529526496..6c85b77c80 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -16,6 +16,9 @@ use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL; +use codex_utils_string::take_bytes_at_char_boundary; + +use crate::MAX_SKILL_PROMPT_BYTES; #[derive(Debug, Default)] pub struct SkillInjections { @@ -93,6 +96,18 @@ pub async fn build_skill_injections( let path = PathUri::from_abs_path(&skill.path_to_skills_md); match fs.read_file_text(&path, /*sandbox*/ None).await { Ok(contents) => { + let (contents, truncated) = + if loaded_skills.is_some_and(|outcome| outcome.is_agent_plugin_skill(skill)) { + bounded_skill_prompt_contents(&contents) + } else { + (contents, false) + }; + if truncated { + result.warnings.push(format!( + "Skill `{}` exceeded the main prompt context limit and was truncated.", + skill.name + )); + } emit_skill_injected_metric(otel, skill, "ok"); invocations.push(SkillInvocation { skill_name: skill.name.clone(), @@ -125,6 +140,11 @@ pub async fn build_skill_injections( result } +fn bounded_skill_prompt_contents(contents: &str) -> (String, bool) { + let bounded = take_bytes_at_char_boundary(contents, MAX_SKILL_PROMPT_BYTES); + (bounded.to_string(), bounded.len() < contents.len()) +} + fn normalize_host_skill_path(path: &str) -> String { normalize_skill_path(path).replace('\\', "/") } diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 1b6e14dac3..01cbb65af9 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -21,6 +21,16 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata { } } +#[test] +fn skill_prompt_contents_are_bounded_at_utf8_boundaries() { + let contents = format!("{}é", "a".repeat(MAX_SKILL_PROMPT_BYTES - 1)); + + let (bounded, truncated) = bounded_skill_prompt_contents(&contents); + + assert_eq!(bounded.len(), MAX_SKILL_PROMPT_BYTES - 1); + assert_eq!(truncated, true); +} + fn set<'a>(items: &'a [&'a str]) -> HashSet<&'a str> { items.iter().copied().collect() } diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 71d2ef4584..8a0bb5fc39 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -8,6 +8,12 @@ pub mod remote; mod root_loader; mod skill_instructions; +/// Hard byte limit for one model-visible skill instruction body. +/// +/// Both the legacy explicit-injection path and the skills extension use this +/// limit so a skill cannot bypass context bounds by changing how it is loaded. +pub const MAX_SKILL_PROMPT_BYTES: usize = 8_000; + pub(crate) use invocation_utils::build_implicit_skill_path_indexes; pub use invocation_utils::detect_implicit_skill_invocation_for_command; pub use mention_counts::build_skill_name_counts; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 347d415419..36e4fe5690 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -95,7 +95,7 @@ const SKILLS_FILENAME: &str = "SKILL.md"; const SKILLS_METADATA_DIR: &str = "agents"; const SKILLS_METADATA_FILENAME: &str = "openai.yaml"; const MAX_NAME_LEN: usize = 64; -const MAX_QUALIFIED_NAME_LEN: usize = 128; +const MAX_QUALIFIED_NAME_LEN: usize = MAX_NAME_LEN * 2 + 1; const MAX_DESCRIPTION_LEN: usize = 1024; const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN; const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN; @@ -163,6 +163,7 @@ where #[derive(Clone)] pub(crate) struct SkillRootSnapshot { pub(crate) root: AbsolutePathBuf, + pub(crate) is_agent_plugin: bool, pub(crate) skills: Vec, pub(crate) errors: Vec, pub(crate) file_system: Arc, @@ -175,6 +176,7 @@ pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { load_skills_under_root(&root, &canonical_root, &mut outcome).await; SkillRootSnapshot { root: canonical_root, + is_agent_plugin: root.discovery_mode == SkillDiscoveryMode::DirectChildren, skills: outcome.skills, errors: outcome.errors, file_system: root.file_system, diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 055fa8a830..93ebe41e8a 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -1202,7 +1202,7 @@ async fn keeps_inherited_namespace_when_symlink_target_is_scan_root_ancestor() { #[tokio::test] async fn plugin_skill_name_length_limit_allows_max_qualified_name() { let root = tempfile::tempdir().expect("tempdir"); - let plugin_name = "p".repeat(MAX_NAME_LEN - 1); + let plugin_name = "p".repeat(MAX_NAME_LEN); let skill_name = "s".repeat(MAX_NAME_LEN); let plugin_root = root.path().join("plugins").join(&plugin_name); let frontmatter = format!("name: {skill_name}\ndescription: search sample data"); @@ -1257,7 +1257,7 @@ async fn plugin_skill_name_length_limit_allows_max_qualified_name() { #[tokio::test] async fn plugin_skill_name_length_limit_rejects_overlong_qualified_name() { let root = tempfile::tempdir().expect("tempdir"); - let plugin_name = "p".repeat(MAX_NAME_LEN); + let plugin_name = "p".repeat(MAX_NAME_LEN + 1); let skill_name = "s".repeat(MAX_NAME_LEN); let plugin_root = root.path().join("plugins").join(&plugin_name); let frontmatter = format!("name: {skill_name}\ndescription: search sample data"); diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index f01d972788..789827f7b0 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -28,6 +28,7 @@ pub struct SkillLoadOutcome { pub disabled_paths: HashSet, pub(crate) skill_roots: Vec, pub(crate) skill_root_by_path: Arc>, + pub(crate) agent_plugin_skill_paths: HashSet, pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, pub(crate) implicit_skills_by_scripts_dir: Arc>, pub(crate) implicit_skills_by_doc_path: Arc>, @@ -70,6 +71,11 @@ impl SkillLoadOutcome { self } + pub fn is_agent_plugin_skill(&self, skill: &SkillMetadata) -> bool { + self.agent_plugin_skill_paths + .contains(&skill.path_to_skills_md) + } + /// Returns the discovery root that supplied a loaded skill path. pub fn skill_root_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> { self.skill_root_by_path.get(path) @@ -173,6 +179,9 @@ pub fn filter_skill_load_outcome_for_product( .map(|(path, root)| (path.clone(), root.clone())) .collect(), ); + outcome + .agent_plugin_skill_paths + .retain(|path| retained_paths.contains(path)); let retained_roots: HashSet = outcome.skill_root_by_path.values().cloned().collect(); outcome diff --git a/codex-rs/core-skills/src/root_loader.rs b/codex-rs/core-skills/src/root_loader.rs index e3cc4fb6d6..ca7d059f43 100644 --- a/codex-rs/core-skills/src/root_loader.rs +++ b/codex-rs/core-skills/src/root_loader.rs @@ -150,6 +150,7 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut for snapshot in snapshots { let SkillRootSnapshot { root, + is_agent_plugin, skills, errors, file_system, @@ -158,12 +159,14 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut skill_roots.push(root.clone()); } for skill in &skills { - skill_root_by_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| root.clone()); - file_systems_by_skill_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| Arc::clone(&file_system)); + let path = skill.path_to_skills_md.clone(); + if !skill_root_by_path.contains_key(&path) { + skill_root_by_path.insert(path.clone(), root.clone()); + file_systems_by_skill_path.insert(path.clone(), Arc::clone(&file_system)); + if is_agent_plugin { + outcome.agent_plugin_skill_paths.insert(path); + } + } } outcome.skills.extend(skills); outcome.errors.extend(errors); @@ -182,6 +185,9 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut let used_roots = skill_root_by_path.values().cloned().collect::>(); skill_roots.retain(|root| used_roots.contains(root)); file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); + outcome + .agent_plugin_skill_paths + .retain(|path| retained_skill_paths.contains(path)); outcome.skill_roots = skill_roots; outcome.skill_root_by_path = Arc::new(skill_root_by_path); outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index c013713450..a7784643ce 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1700,10 +1700,17 @@ impl Config { { let mut plugin_mcp_servers = plugin.mcp_servers.clone(); 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(), - ); + let attribution = if plugin.is_agent_plugin() { + McpPluginAttribution::agent_plugin( + plugin.config_name.clone(), + plugin.display_name().to_string(), + ) + } else { + McpPluginAttribution::new( + plugin.config_name.clone(), + plugin.display_name().to_string(), + ) + }; for (name, plugin_server) in plugin_mcp_servers { catalog.register(McpServerRegistration::from_plugin( name, diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index 8029755f21..f07d9778d0 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -12,6 +12,7 @@ use codex_protocol::request_user_input::RequestUserInputQuestion; use codex_protocol::request_user_input::RequestUserInputQuestionOption; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_rmcp_client::OAuthDiscoveryTimeout; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::perform_oauth_login; use tokio_util::sync::CancellationToken; use tracing::warn; @@ -152,6 +153,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( &server_config.transport, Arc::clone(&http_client), discovery_timeout, + StreamableHttpRedirectMode::Legacy, ) .await; let oauth_config = match login_support { diff --git a/codex-rs/core/src/mcp_tool_exposure.rs b/codex-rs/core/src/mcp_tool_exposure.rs index 0991e4cf41..84d14ebcd3 100644 --- a/codex-rs/core/src/mcp_tool_exposure.rs +++ b/codex-rs/core/src/mcp_tool_exposure.rs @@ -11,6 +11,9 @@ use codex_tools::ToolName; use tracing::instrument; use tracing::warn; +const MAX_AGENT_PLUGIN_MCP_SPEC_BYTES: usize = 8_000; +const MAX_AGENT_PLUGIN_MCP_TOTAL_BYTES: usize = 64_000; + use crate::config::Config; use crate::tools::handlers::McpHandler; use crate::tools::registry::ToolRegistry; @@ -20,6 +23,7 @@ pub(crate) fn append_mcp_tools( all_mcp_tools: &[McpToolInfo], config: &Config, apps_enabled: bool, + mcp_server_catalog: &codex_mcp::ResolvedMcpCatalog, search_tool_enabled: bool, registry: &mut ToolRegistry, ) -> HashSet { @@ -35,11 +39,43 @@ pub(crate) fn append_mcp_tools( ToolExposure::Direct }; let mut registered_tools = HashSet::new(); + let mut agent_plugin_bytes = 0usize; for tool in non_app_tools.chain(app_tools) { let tool_name = tool.canonical_tool_name(); - match McpHandler::new(tool.clone()) { + let agent_plugin = mcp_server_catalog + .server(&tool.server_name) + .is_some_and(|server| server.source().is_agent_plugin()); + let handler = if agent_plugin { + McpHandler::new_agent_plugin(tool.clone()) + } else { + McpHandler::new(tool.clone()) + }; + match handler { Ok(handler) => { - if registry.register_external_with_exposure(Arc::new(handler), exposure) { + let fits_agent_budget = if agent_plugin { + handler.model_spec_bytes().is_ok_and(|bytes| { + if bytes > MAX_AGENT_PLUGIN_MCP_SPEC_BYTES { + return false; + } + let next = agent_plugin_bytes.saturating_add(bytes); + if next <= MAX_AGENT_PLUGIN_MCP_TOTAL_BYTES { + agent_plugin_bytes = next; + true + } else { + false + } + }) + } else { + true + }; + let tool_exposure = if fits_agent_budget { + exposure + } else { + ToolExposure::Hidden + }; + if registry.register_external_with_exposure(Arc::new(handler), tool_exposure) + && fits_agent_budget + { registered_tools.insert(tool_name); } } diff --git a/codex-rs/core/src/mcp_tool_exposure_test.rs b/codex-rs/core/src/mcp_tool_exposure_test.rs index 782ea0f9fb..9f709f8dcb 100644 --- a/codex-rs/core/src/mcp_tool_exposure_test.rs +++ b/codex-rs/core/src/mcp_tool_exposure_test.rs @@ -2,6 +2,9 @@ use std::collections::HashMap; use std::sync::Arc; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_mcp::McpPluginAttribution; +use codex_mcp::McpServerRegistration; +use codex_mcp::ResolvedMcpCatalog; use codex_mcp::ToolInfo; use codex_tools::ToolExposure; use codex_tools::ToolName; @@ -75,12 +78,29 @@ fn runtimes_by_name( config: &Config, apps_enabled: bool, search_tool_enabled: bool, +) -> HashMap { + runtimes_by_name_with_catalog( + tools, + config, + apps_enabled, + &ResolvedMcpCatalog::default(), + search_tool_enabled, + ) +} + +fn runtimes_by_name_with_catalog( + tools: &[ToolInfo], + config: &Config, + apps_enabled: bool, + mcp_server_catalog: &ResolvedMcpCatalog, + search_tool_enabled: bool, ) -> HashMap { let mut registry = ToolRegistry::default(); append_mcp_tools( tools, config, apps_enabled, + mcp_server_catalog, search_tool_enabled, &mut registry, ); @@ -90,6 +110,91 @@ fn runtimes_by_name( .collect() } +#[tokio::test] +async fn agent_plugin_budget_hides_only_overflow_agent_tools() { + let codex_home = tempdir().expect("tempdir should succeed"); + std::fs::write( + codex_home.path().join(CONFIG_TOML_FILE), + "[mcp_servers.agent]\ncommand = \"echo\"\n", + ) + .expect("write config"); + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .build() + .await + .expect("config should build"); + let agent_config = config.mcp_servers.get()["agent"].clone(); + let legacy_config = agent_config.clone(); + let mut catalog = ResolvedMcpCatalog::builder(); + catalog.register(McpServerRegistration::from_plugin( + "agent".to_string(), + McpPluginAttribution::agent_plugin("agent@test".to_string(), "Agent".to_string()), + /*plugin_order*/ 0, + agent_config, + )); + catalog.register(McpServerRegistration::from_plugin( + "legacy".to_string(), + McpPluginAttribution::new("legacy@test".to_string(), "Legacy".to_string()), + /*plugin_order*/ 1, + legacy_config, + )); + let catalog = catalog.build(); + let mut tools = (0..40) + .map(|index| { + let name = format!("tool_{index}"); + let mut tool = make_mcp_tool( + "agent", + &name, + "mcp__agent", + &name, + /*connector_id*/ None, + /*connector_name*/ None, + ); + tool.namespace_description = Some("n".repeat(1_000)); + tool.tool.description = Some("d".repeat(1_000).into()); + tool + }) + .collect::>(); + let oversized_name = "x".repeat(MAX_AGENT_PLUGIN_MCP_SPEC_BYTES); + let oversized_agent_tool = make_mcp_tool( + "agent", + "oversized_agent_tool", + "mcp__agent", + &oversized_name, + /*connector_id*/ None, + /*connector_name*/ None, + ); + tools.push(oversized_agent_tool.clone()); + let legacy_tool = make_mcp_tool( + "legacy", + "legacy_tool", + "mcp__legacy", + &oversized_name, + /*connector_id*/ None, + /*connector_name*/ None, + ); + tools.push(legacy_tool.clone()); + + let runtimes = runtimes_by_name_with_catalog( + &tools, &config, /*apps_enabled*/ false, &catalog, /*search_tool_enabled*/ false, + ); + let agent_exposures = tools[..40] + .iter() + .map(|tool| runtimes[&tool.canonical_tool_name()]) + .collect::>(); + + assert!(agent_exposures.contains(&ToolExposure::Direct)); + assert!(agent_exposures.contains(&ToolExposure::Hidden)); + assert_eq!( + runtimes[&oversized_agent_tool.canonical_tool_name()], + ToolExposure::Hidden + ); + assert_eq!( + runtimes[&legacy_tool.canonical_tool_name()], + ToolExposure::Direct + ); +} + fn with_visibility(mut tool: ToolInfo, visibility: &[&str]) -> ToolInfo { tool.tool.meta = Some(MetaObject( serde_json::json!({ "ui": { "visibility": visibility } }) @@ -240,6 +345,7 @@ async fn app_tool_registration_uses_trusted_catalog_metadata_and_preserves_sourc &mcp_tools, &config, /*apps_enabled*/ true, + &ResolvedMcpCatalog::default(), /*search_tool_enabled*/ false, &mut registry, ); diff --git a/codex-rs/core/src/plugins/mentions_tests.rs b/codex-rs/core/src/plugins/mentions_tests.rs index 37c9adb886..8184a41945 100644 --- a/codex-rs/core/src/plugins/mentions_tests.rs +++ b/codex-rs/core/src/plugins/mentions_tests.rs @@ -18,6 +18,7 @@ fn plugin(config_name: &str, display_name: &str) -> PluginCapabilitySummary { PluginCapabilitySummary { config_name: config_name.to_string(), display_name: display_name.to_string(), + plugin_namespace: None, description: None, has_skills: true, mcp_server_names: Vec::new(), diff --git a/codex-rs/core/src/plugins/render.rs b/codex-rs/core/src/plugins/render.rs index 4e7575bd62..38dc949b2f 100644 --- a/codex-rs/core/src/plugins/render.rs +++ b/codex-rs/core/src/plugins/render.rs @@ -3,6 +3,11 @@ use crate::context::AvailablePluginsInstructions; #[cfg(test)] use crate::context::ContextualUserFragment; use crate::plugins::PluginCapabilitySummary; +use codex_utils_string::take_bytes_at_char_boundary; + +const MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES: usize = 4 * 1024; +const TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX: &str = + "\n- Additional plugin capabilities omitted to fit the context limit."; #[cfg(test)] pub(crate) fn render_plugins_section(plugins: &[PluginCapabilitySummary]) -> Option { @@ -20,9 +25,12 @@ pub(crate) fn render_explicit_plugin_instructions( )]; if plugin.has_skills { + let skill_namespace = plugin + .plugin_namespace + .as_deref() + .unwrap_or(plugin.display_name.as_str()); lines.push(format!( - "- Skills from this plugin are prefixed with `{}:`.", - plugin.display_name + "- Skills from this plugin are prefixed with `{skill_namespace}:`." )); } @@ -54,7 +62,18 @@ pub(crate) fn render_explicit_plugin_instructions( lines.push("Use these plugin-associated capabilities to help solve the task.".to_string()); - Some(lines.join("\n")) + Some(bound_explicit_plugin_instructions(lines.join("\n"))) +} + +fn bound_explicit_plugin_instructions(rendered: String) -> String { + if rendered.len() <= MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES { + return rendered; + } + + let max_prefix_bytes = MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES + .saturating_sub(TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX.len()); + let prefix = take_bytes_at_char_boundary(&rendered, max_prefix_bytes); + format!("{prefix}{TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX}") } #[cfg(test)] diff --git a/codex-rs/core/src/plugins/render_tests.rs b/codex-rs/core/src/plugins/render_tests.rs index ee7ea8c81a..e264f8b2b1 100644 --- a/codex-rs/core/src/plugins/render_tests.rs +++ b/codex-rs/core/src/plugins/render_tests.rs @@ -11,6 +11,7 @@ fn render_plugins_section_keeps_plugin_usage_guidance_without_listing_plugins() let rendered = render_plugins_section(&[PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: None, description: Some("inspect sample data".to_string()), has_skills: true, ..PluginCapabilitySummary::default() @@ -21,3 +22,43 @@ fn render_plugins_section_keeps_plugin_usage_guidance_without_listing_plugins() assert_eq!(rendered, expected); } + +#[test] +fn explicit_plugin_instructions_use_manifest_namespace_for_skills() { + let rendered = render_explicit_plugin_instructions( + &PluginCapabilitySummary { + config_name: "acme.tools@test".to_string(), + display_name: "Acme Developer Tools".to_string(), + plugin_namespace: Some("acme.tools".to_string()), + has_skills: true, + ..PluginCapabilitySummary::default() + }, + &[], + &[], + ) + .expect("skill capability should render"); + + assert!(rendered.contains("`acme.tools:`")); + assert!(!rendered.contains("`Acme Developer Tools:`")); +} + +#[test] +fn explicit_plugin_instructions_are_bounded() { + let servers = (0..1_024) + .map(|index| format!("server-{index}")) + .collect::>(); + + let rendered = render_explicit_plugin_instructions( + &PluginCapabilitySummary { + config_name: "sample@test".to_string(), + display_name: "sample".to_string(), + ..PluginCapabilitySummary::default() + }, + &servers, + &[], + ) + .expect("MCP capability should render"); + + assert!(rendered.len() <= MAX_EXPLICIT_PLUGIN_INSTRUCTIONS_BYTES); + assert!(rendered.ends_with(TRUNCATED_PLUGIN_INSTRUCTIONS_SUFFIX)); +} diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index a8f35690b4..7a67372737 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -23,6 +23,7 @@ use codex_tools::ToolName; use codex_tools::ToolSearchInfo; use codex_tools::ToolSearchSourceInfo; use codex_tools::ToolSpec; +use codex_tools::agent_plugin_mcp_tool_to_responses_api_tool; use codex_tools::mcp_tool_to_responses_api_tool; use codex_utils_string::take_bytes_at_char_boundary; use futures::future::BoxFuture; @@ -31,6 +32,7 @@ use serde_json::Value; const LEGACY_MCP_TOOL_NAME_PREFIX: &str = "mcp__"; const MCP_TOOL_NAME_DELIMITER: &str = "__"; +const MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 1_000; const MAX_MCP_NAMESPACE_DESCRIPTION_BYTES: usize = 512 * 1024; pub struct McpHandler { @@ -40,10 +42,38 @@ pub struct McpHandler { impl McpHandler { pub fn new(tool_info: ToolInfo) -> Result { - let spec = create_tool_spec(&tool_info)?; + Self::with_agent_plugin(tool_info, /*agent_plugin*/ false) + } + + pub fn new_agent_plugin(tool_info: ToolInfo) -> Result { + Self::with_agent_plugin(tool_info, /*agent_plugin*/ true) + } + + fn with_agent_plugin( + mut tool_info: ToolInfo, + agent_plugin: bool, + ) -> Result { + if agent_plugin { + tool_info.namespace_description = + tool_info + .namespace_description + .as_deref() + .map(|description| { + take_bytes_at_char_boundary( + description, + MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES, + ) + .to_string() + }); + } + let spec = create_tool_spec(&tool_info, agent_plugin)?; Ok(Self { tool_info, spec }) } + pub(crate) fn model_spec_bytes(&self) -> Result { + serde_json::to_vec(&self.spec).map(|spec| spec.len()) + } + fn hook_tool_name(&self) -> HookToolName { HookToolName::new(ensure_mcp_prefix(&join_tool_name(&self.tool_name()))) } @@ -236,9 +266,16 @@ impl CoreToolRuntime for McpHandler { } } -fn create_tool_spec(tool_info: &ToolInfo) -> Result { +fn create_tool_spec( + tool_info: &ToolInfo, + agent_plugin: bool, +) -> Result { let tool_name = tool_info.canonical_tool_name(); - let tool = mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)?; + let tool = if agent_plugin { + agent_plugin_mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)? + } else { + mcp_tool_to_responses_api_tool(&tool_name, &tool_info.tool)? + }; let description = tool_info .namespace_description .as_deref() diff --git a/codex-rs/core/src/tools/handlers/mcp_search_tests.rs b/codex-rs/core/src/tools/handlers/mcp_search_tests.rs index af8797f02c..d4e8d162ec 100644 --- a/codex-rs/core/src/tools/handlers/mcp_search_tests.rs +++ b/codex-rs/core/src/tools/handlers/mcp_search_tests.rs @@ -89,6 +89,27 @@ fn mcp_namespace_descriptions_are_bounded_at_512_kib() { assert_eq!(namespace.description, expected_description); } +#[test] +fn agent_plugin_namespace_descriptions_use_the_stricter_bound() { + let expected_description = "é".repeat(MAX_AGENT_PLUGIN_MCP_NAMESPACE_DESCRIPTION_BYTES / 2); + let mut info = tool_info(); + info.namespace_description = Some(format!("{expected_description}overflow")); + let handler = McpHandler::new_agent_plugin(info).expect("MCP tool spec should build"); + let search_info = handler.search_info().expect("MCP search info"); + + assert_eq!( + search_info.source_info, + Some(ToolSearchSourceInfo { + name: "Calendar".to_string(), + description: Some(expected_description.clone()), + }) + ); + let LoadableToolSpec::Namespace(namespace) = search_info.entry.output else { + panic!("expected namespace search output"); + }; + assert_eq!(namespace.description, expected_description); +} + fn tool_info() -> ToolInfo { ToolInfo { server_name: "codex-apps".to_string(), diff --git a/codex-rs/core/src/tools/spec_plan.rs b/codex-rs/core/src/tools/spec_plan.rs index 50464f5c1e..a1d98c43a6 100644 --- a/codex-rs/core/src/tools/spec_plan.rs +++ b/codex-rs/core/src/tools/spec_plan.rs @@ -150,6 +150,7 @@ pub(crate) fn build_tool_router( mcp.tools(), &turn_context.config, apps_enabled, + &mcp.config().mcp_server_catalog, search_tool_enabled(turn_context), &mut registry, ); diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 31035b5e42..4fc1d50768 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -31,6 +31,7 @@ use core_test_support::responses::ResponsesRequest; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_response_created; use core_test_support::responses::ev_tool_search_call; use core_test_support::responses::mount_sse_once; @@ -164,6 +165,38 @@ fn write_sample_plugin_skill(plugin_root: std::path::PathBuf) -> std::path::Path skill_dir.join("SKILL.md") } +fn write_agent_plugin_skill_plugin(home: &TempDir) -> std::path::PathBuf { + let plugin_root = home.path().join("plugins/cache/test/acme.tools/local"); + let direct_skill = plugin_root.join("skills/review"); + let nested_skill = plugin_root.join("skills/group/hidden"); + std::fs::create_dir_all(&direct_skill).expect("create direct skill"); + std::fs::create_dir_all(&nested_skill).expect("create nested skill"); + std::fs::write( + plugin_root.join("plugin.json"), + r#"{"$schema":"https://agent-plugins.org/schemas/1.0.0/plugin.schema.json","name":"acme.tools","extensions":{"com.openai":{"interface":{"displayName":"Acme Developer Tools"}}}}"#, + ) + .expect("write Agent Plugin manifest"); + std::fs::write( + direct_skill.join("SKILL.md"), + format!( + "---\nname: review\ndescription: Review code\n---\n\n{}\nAGENT_SKILL_TRUNCATED_TAIL\n", + "x".repeat(9_000) + ), + ) + .expect("write direct skill"); + std::fs::write( + nested_skill.join("SKILL.md"), + "---\nname: hidden\ndescription: Hidden skill\n---\n\nHidden.\n", + ) + .expect("write nested skill"); + std::fs::write( + home.path().join("config.toml"), + "[features]\nplugins = true\n\n[plugins.\"acme.tools@test\"]\nenabled = true\n", + ) + .expect("write Agent Plugin config"); + direct_skill.join("SKILL.md") +} + fn write_plugin_mcp_plugin(home: &TempDir, command: &str) { let plugin_root = write_sample_plugin_manifest_and_config(home); std::fs::write( @@ -485,6 +518,234 @@ async fn capability_sections_render_in_developer_message_in_order() -> Result<() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn agent_plugin_skills_use_shared_catalog_and_direct_child_discovery() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let resp_mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + let codex_home = Arc::new(TempDir::new()?); + let skill_path = std::fs::canonicalize(write_agent_plugin_skill_plugin(codex_home.as_ref()))?; + let test_codex = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_extensions(skills_extensions()) + .build(&server) + .await?; + + test_codex + .codex + .submit(Op::UserInput { + items: vec![UserInput::Skill { + name: "acme.tools:review".into(), + path: skill_path, + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + let warning = wait_for_event(&test_codex.codex, |ev| { + matches!( + ev, + EventMsg::Warning(warning) + if warning.message.contains("main prompt context limit") + ) + }) + .await; + wait_for_event(&test_codex.codex, |ev| { + matches!(ev, EventMsg::TurnComplete(_)) + }) + .await; + + let developer_text = resp_mock + .single_request() + .message_input_texts("developer") + .join("\n"); + assert!(developer_text.contains("acme.tools:review: Review code")); + assert!(!developer_text.contains("acme.tools:hidden")); + let user_text = resp_mock + .single_request() + .message_input_texts("user") + .join("\n"); + assert!(user_text.contains("acme.tools:review")); + assert!(!user_text.contains("AGENT_SKILL_TRUNCATED_TAIL")); + let EventMsg::Warning(warning) = warning else { + unreachable!("wait_for_event matched an Agent skill truncation warning") + }; + assert!(warning.message.contains("acme.tools:review")); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn legacy_plugin_skill_prompt_remains_complete() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let resp_mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + let codex_home = Arc::new(TempDir::new()?); + let skill_path = write_plugin_skill_plugin(codex_home.as_ref()); + let skill_contents = format!( + "---\nname: sample-search\ndescription: inspect sample data\n---\n\n{}\nLEGACY_SKILL_FULL_TAIL\n", + "x".repeat(9_000) + ); + std::fs::write(&skill_path, &skill_contents)?; + let skill_path = std::fs::canonicalize(skill_path)?; + let test_codex = test_codex() + .with_home(codex_home) + .with_extensions(skills_extensions()) + .build(&server) + .await?; + + test_codex + .codex + .submit(Op::UserInput { + items: vec![UserInput::Skill { + name: "sample:sample-search".into(), + path: skill_path, + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + wait_for_event(&test_codex.codex, |ev| { + matches!(ev, EventMsg::TurnComplete(_)) + }) + .await; + + let user_text = resp_mock + .single_request() + .message_input_texts("user") + .join("\n"); + assert!(user_text.contains(&skill_contents)); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn agent_plugin_root_mcp_stdio_tool_round_trip_expands_reserved_paths() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let search_call_id = "search-agent-echo"; + let tool_call_id = "call-agent-echo"; + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_tool_search_call(search_call_id, &serde_json::json!({"query": "echo"})), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_function_call_with_namespace( + tool_call_id, + "mcp__agent", + "echo", + r#"{"message":"ping"}"#, + ), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + let codex_home = Arc::new(TempDir::new()?); + write_agent_plugin_skill_plugin(codex_home.as_ref()); + let plugin_root = codex_home + .path() + .join("plugins/cache/test/acme.tools/local"); + let stdio_server = match stdio_server_bin() { + Ok(path) => path, + Err(err) => { + eprintln!("test_stdio_server binary not available, skipping test: {err}"); + return Ok(()); + } + }; + let stdio_server_name = format!("test_stdio_server{}", std::env::consts::EXE_SUFFIX); + std::fs::copy(stdio_server, plugin_root.join(&stdio_server_name))?; + let mcp_config = serde_json::json!({ + "$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json", + "mcpServers": { + "agent": { + "type": "stdio", + "command": format!("./{stdio_server_name}"), + "env": {"MCP_TEST_VALUE": "${PLUGIN_ROOT}|${PLUGIN_DATA}"} + } + } + }); + std::fs::write( + plugin_root.join("mcp.json"), + serde_json::to_vec_pretty(&mcp_config)?, + )?; + let test_codex = test_codex() + .with_home(Arc::clone(&codex_home)) + .build(&server) + .await?; + wait_for_mcp_server(&test_codex.codex, "agent").await?; + let data_root = std::fs::read_dir(codex_home.path().join("plugins/data/agent-plugins"))? + .next() + .expect("Agent Plugin data root")? + .path() + .canonicalize()?; + let expected_env = format!( + "{}|{}", + plugin_root.canonicalize()?.display(), + data_root.display() + ); + + test_codex + .codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "call the Agent Plugin echo tool".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + let end = wait_for_event(&test_codex.codex, |event| { + matches!(event, EventMsg::McpToolCallEnd(_)) + }) + .await; + wait_for_event(&test_codex.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let EventMsg::McpToolCallEnd(end) = end else { + unreachable!("wait_for_event matched an MCP tool end") + }; + let result = end.result.as_ref().expect("Agent Plugin MCP tool result"); + assert_eq!( + result + .structured_content + .as_ref() + .and_then(|content| content.get("env")) + .and_then(serde_json::Value::as_str), + Some(expected_env.as_str()) + ); + let requests = mock.requests(); + let search_output = requests[1].tool_search_output(search_call_id); + assert!(namespace_child_tool(&search_output, "mcp__agent", "echo").is_some()); + assert!(requests[2].function_call_output(tool_call_id).is_object()); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn agent_turns_route_curated_plugin_skills_after_auth_switch() -> Result<()> { const CHATGPT_CURATED_PLUGIN_SKILL: &str = "chatgpt-plugin:chatgpt-skill"; diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 7b22230b49..05c41e45ee 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -5,6 +5,7 @@ use std::path::Component; use std::path::Path; use std::path::PathBuf; +use codex_core_skills::MAX_SKILL_PROMPT_BYTES; use codex_protocol::protocol::SkillScope; use codex_utils_string::approx_token_count; use codex_utils_string::take_bytes_at_char_boundary; @@ -17,7 +18,6 @@ use crate::fragments::AvailableSkillsInstructions; const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000; const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; -const MAX_MAIN_PROMPT_BYTES: usize = 8_000; const MAX_CATALOG_SKILL_DESCRIPTION_CHARS: usize = 1_024; const TRUNCATED_SKILL_DESCRIPTION_SUFFIX: &str = "..."; const SKILL_DESCRIPTION_TRUNCATION_WARNING_THRESHOLD_CHARS: usize = 100; @@ -1083,7 +1083,7 @@ pub(crate) fn truncate_catalog_skill_description(description: &str) -> Cow<'_, s } pub(crate) fn truncate_main_prompt_contents(contents: &str) -> (String, bool) { - truncate_utf8_to_bytes(contents, MAX_MAIN_PROMPT_BYTES) + truncate_utf8_to_bytes(contents, MAX_SKILL_PROMPT_BYTES) } pub(crate) fn truncate_utf8_to_bytes(contents: &str, max_bytes: usize) -> (String, bool) { diff --git a/codex-rs/plugin/src/lib.rs b/codex-rs/plugin/src/lib.rs index 8a00d8c5ef..452c00b906 100644 --- a/codex-rs/plugin/src/lib.rs +++ b/codex-rs/plugin/src/lib.rs @@ -52,6 +52,7 @@ pub fn app_connector_ids_from_declarations<'a>( pub struct PluginCapabilitySummary { pub config_name: String, pub display_name: String, + pub plugin_namespace: Option, pub description: Option, pub has_skills: bool, pub mcp_server_names: Vec, diff --git a/codex-rs/plugin/src/load_outcome.rs b/codex-rs/plugin/src/load_outcome.rs index 5fd1320c09..0ebb81ad07 100644 --- a/codex-rs/plugin/src/load_outcome.rs +++ b/codex-rs/plugin/src/load_outcome.rs @@ -25,6 +25,7 @@ pub struct LoadedPlugin { pub root: AbsolutePathBuf, pub enabled: bool, pub skill_roots: Vec, + pub skill_discovery_mode: SkillDiscoveryMode, pub disabled_skill_paths: HashSet, pub has_enabled_skills: bool, pub mcp_servers: HashMap, @@ -42,6 +43,10 @@ impl LoadedPlugin { pub fn display_name(&self) -> &str { self.manifest_name.as_deref().unwrap_or(&self.config_name) } + + pub fn is_agent_plugin(&self) -> bool { + self.skill_discovery_mode == SkillDiscoveryMode::DirectChildren + } } fn plugin_capability_summary_from_loaded( @@ -57,6 +62,7 @@ fn plugin_capability_summary_from_loaded( let summary = PluginCapabilitySummary { config_name: plugin.config_name.clone(), display_name: plugin.display_name().to_string(), + plugin_namespace: plugin.plugin_namespace.clone(), description: prompt_safe_plugin_description(plugin.manifest_description.as_deref()), has_skills: plugin.has_enabled_skills, mcp_server_names, @@ -143,7 +149,7 @@ impl PluginLoadOutcome { }, plugin_namespace: plugin_namespace.clone(), plugin_root: plugin.root.clone(), - discovery_mode: SkillDiscoveryMode::Recursive, + discovery_mode: plugin.skill_discovery_mode, }); } } @@ -241,6 +247,7 @@ mod tests { root: test_path(config_name), enabled: true, skill_roots, + skill_discovery_mode: SkillDiscoveryMode::Recursive, disabled_skill_paths: HashSet::new(), has_enabled_skills: true, mcp_servers: HashMap::new(), diff --git a/codex-rs/rmcp-client/src/auth_status.rs b/codex-rs/rmcp-client/src/auth_status.rs index c85b784da4..d29419b410 100644 --- a/codex-rs/rmcp-client/src/auth_status.rs +++ b/codex-rs/rmcp-client/src/auth_status.rs @@ -12,6 +12,7 @@ use rmcp::transport::AuthorizationManager; use rmcp::transport::auth::AuthError; use tracing::debug; +use crate::http_client_adapter::StreamableHttpRedirectMode; use crate::oauth::StoredOAuthTokenStatus; use crate::oauth::oauth_token_status; use crate::oauth_http_client::OAuthHttpClientAdapter; @@ -85,7 +86,9 @@ pub async fn determine_streamable_http_auth_status( keyring_backend_kind: AuthKeyringBackendKind, http_client: Arc, discovery_timeout: OAuthDiscoveryTimeout, + redirect_mode: StreamableHttpRedirectMode, ) -> Result { + let has_configured_headers = has_configured_headers(&http_headers, &env_http_headers); let default_headers = match auth_status_before_discovery( server_name, url, @@ -106,6 +109,8 @@ pub async fn determine_streamable_http_auth_status( default_headers, http_client, discovery_timeout, + has_configured_headers, + redirect_mode, ) .await, ) @@ -193,13 +198,17 @@ pub async fn discover_streamable_http_oauth( env_http_headers: Option>, http_client: Arc, discovery_timeout: OAuthDiscoveryTimeout, + redirect_mode: StreamableHttpRedirectMode, ) -> Result> { + let has_configured_headers = has_configured_headers(&http_headers, &env_http_headers); let default_headers = build_default_headers(http_headers, env_http_headers)?; discover_streamable_http_oauth_with_headers_and_http_client( url, default_headers, http_client, discovery_timeout, + has_configured_headers, + redirect_mode, ) .await } @@ -209,13 +218,24 @@ async fn discover_streamable_http_oauth_with_headers_and_http_client( default_headers: HeaderMap, http_client: Arc, discovery_timeout: OAuthDiscoveryTimeout, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, ) -> Result> { let oauth_http_client = match discovery_timeout { - OAuthDiscoveryTimeout::Requested => { - OAuthHttpClientAdapter::new(http_client, default_headers) - } + OAuthDiscoveryTimeout::Requested => OAuthHttpClientAdapter::new_with_redirect_mode( + http_client, + default_headers, + has_configured_headers, + redirect_mode, + ), OAuthDiscoveryTimeout::Capped(max_timeout) => { - OAuthHttpClientAdapter::new_with_max_timeout(http_client, default_headers, max_timeout) + OAuthHttpClientAdapter::new_with_max_timeout_and_redirect_mode( + http_client, + default_headers, + max_timeout, + has_configured_headers, + redirect_mode, + ) } }; let mut authorization_manager = @@ -224,6 +244,18 @@ async fn discover_streamable_http_oauth_with_headers_and_http_client( discover_streamable_http_oauth_with_manager(&authorization_manager).await } +fn has_configured_headers( + http_headers: &Option>, + env_http_headers: &Option>, +) -> bool { + http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()) + || env_http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()) +} + async fn discover_streamable_http_oauth_with_manager( authorization_manager: &AuthorizationManager, ) -> Result> { @@ -268,6 +300,7 @@ mod tests { use axum::http::header::WWW_AUTHENTICATE; use axum::routing::get; use codex_exec_server::ExecServerError; + use codex_exec_server::HttpRedirectPolicy; use codex_exec_server::HttpRequestParams; use codex_exec_server::HttpRequestResponse; use codex_exec_server::HttpResponseBodyStream; @@ -308,6 +341,7 @@ mod tests { #[derive(Default)] struct RecordingHttpClient { headers: Mutex>>, + redirect_policy: Mutex>, timeout_ms: Mutex>>, } @@ -342,6 +376,11 @@ mod tests { .timeout_ms .lock() .expect("timeout recorder lock should not be poisoned") = Some(params.timeout_ms); + *self + .redirect_policy + .lock() + .expect("redirect policy recorder lock should not be poisoned") = + Some(params.redirect_policy); Box::pin(async { Err(ExecServerError::HttpRequest( "expected discovery request failure".to_string(), @@ -440,6 +479,7 @@ mod tests { AuthKeyringBackendKind::default(), test_http_client(), OAuthDiscoveryTimeout::Requested, + StreamableHttpRedirectMode::Legacy, ) .await .expect("status should compute"); @@ -464,6 +504,7 @@ mod tests { AuthKeyringBackendKind::default(), test_http_client(), OAuthDiscoveryTimeout::Requested, + StreamableHttpRedirectMode::Legacy, ) .await .expect("status should compute"); @@ -518,6 +559,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await; assert_eq!( @@ -560,6 +602,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect_err("cross-origin OAuth discovery redirects must be rejected"); @@ -602,6 +645,7 @@ mod tests { AuthKeyringBackendKind::default(), test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect_err("transient OAuth discovery failures must not become unsupported access"); @@ -632,6 +676,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect("discovery should succeed") @@ -653,6 +698,7 @@ mod tests { /*env_http_headers*/ None, http_client.clone(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await; @@ -679,6 +725,7 @@ mod tests { /*env_http_headers*/ None, http_client.clone(), OAuthDiscoveryTimeout::Requested, + StreamableHttpRedirectMode::Legacy, ) .await; @@ -693,7 +740,7 @@ mod tests { } #[tokio::test] - async fn routed_oauth_discovery_preserves_configured_headers() { + async fn routed_agent_plugin_oauth_discovery_stops_with_configured_headers() { let http_client = Arc::new(RecordingHttpClient::default()); let discovery = discover_streamable_http_oauth( @@ -705,6 +752,7 @@ mod tests { /*env_http_headers*/ None, http_client.clone(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::AgentPluginV1, ) .await; @@ -722,6 +770,13 @@ mod tests { .map(|(_, value)| value.as_str()), Some("configured-value") ); + assert_eq!( + *http_client + .redirect_policy + .lock() + .expect("redirect policy recorder lock should not be poisoned"), + Some(HttpRedirectPolicy::Stop) + ); } #[tokio::test] @@ -774,6 +829,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect("discovery should succeed") @@ -800,6 +856,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect("discovery should succeed") @@ -822,6 +879,7 @@ mod tests { /*env_http_headers*/ None, test_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await .expect("support check should succeed") diff --git a/codex-rs/rmcp-client/src/http_client_adapter.rs b/codex-rs/rmcp-client/src/http_client_adapter.rs index 23220cdf53..bde57a9d99 100644 --- a/codex-rs/rmcp-client/src/http_client_adapter.rs +++ b/codex-rs/rmcp-client/src/http_client_adapter.rs @@ -63,11 +63,19 @@ const HEADER_SESSION_ID: &str = "Mcp-Session-Id"; const NON_JSON_RESPONSE_BODY_PREVIEW_BYTES: usize = 8_192; const LEGACY_HTTP_PREVALIDATION_ERROR_CODE: ErrorCode = ErrorCode(-32000); +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum StreamableHttpRedirectMode { + Legacy, + AgentPluginV1, +} + #[derive(Clone)] pub(crate) struct StreamableHttpClientAdapter { http_client: Arc, default_headers: HeaderMap, auth_provider: Option, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, } #[derive(Debug, thiserror::Error)] @@ -87,13 +95,21 @@ impl StreamableHttpClientAdapter { http_client: Arc, default_headers: HeaderMap, auth_provider: Option, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, ) -> Self { Self { http_client, default_headers, auth_provider, + has_configured_headers, + redirect_mode, } } + + fn redirect_policy(&self, headers: &HeaderMap) -> HttpRedirectPolicy { + mcp_redirect_policy(self.redirect_mode, headers, self.has_configured_headers) + } } impl StreamableHttpClient for StreamableHttpClientAdapter { @@ -150,7 +166,7 @@ impl StreamableHttpClient for StreamableHttpClientAdapter { let redirect_policy = if mcp_method.as_deref() == Some(DiscoverRequestMethod::VALUE) { HttpRedirectPolicy::Stop } else { - mcp_redirect_policy(&headers) + self.redirect_policy(&headers) }; let body = serde_json::to_vec(&message).map_err(StreamableHttpError::Deserialize)?; @@ -348,7 +364,7 @@ impl StreamableHttpClient for StreamableHttpClientAdapter { session.to_string(), StreamableHttpClientAdapterError::Header, )?; - let redirect_policy = mcp_redirect_policy(&headers); + let redirect_policy = self.redirect_policy(&headers); let response = self .http_client @@ -421,7 +437,7 @@ impl StreamableHttpClient for StreamableHttpClientAdapter { StreamableHttpClientAdapterError::Header, )?; } - let redirect_policy = mcp_redirect_policy(&headers); + let redirect_policy = self.redirect_policy(&headers); let (response, body_stream) = self .http_client @@ -646,11 +662,17 @@ fn parse_json_rpc_error(body: &[u8]) -> Option { } } -fn mcp_redirect_policy(headers: &HeaderMap) -> HttpRedirectPolicy { +fn mcp_redirect_policy( + mode: StreamableHttpRedirectMode, + headers: &HeaderMap, + has_configured_headers: bool, +) -> HttpRedirectPolicy { if headers .get(HEADER_MCP_PROTOCOL_VERSION) .and_then(|value| value.to_str().ok()) == Some(ProtocolVersion::V_2026_07_28.as_str()) + || (mode == StreamableHttpRedirectMode::AgentPluginV1 + && (has_configured_headers || headers.contains_key(AUTHORIZATION))) { HttpRedirectPolicy::Stop } else { diff --git a/codex-rs/rmcp-client/src/http_client_adapter_tests.rs b/codex-rs/rmcp-client/src/http_client_adapter_tests.rs index a615beec4e..7452f5b400 100644 --- a/codex-rs/rmcp-client/src/http_client_adapter_tests.rs +++ b/codex-rs/rmcp-client/src/http_client_adapter_tests.rs @@ -1,11 +1,15 @@ use std::io::ErrorKind; +use codex_exec_server::HttpRedirectPolicy; use http::HeaderMap; use http::HeaderValue; +use http::header::AUTHORIZATION; use pretty_assertions::assert_eq; use super::HttpHeader; use super::SseEventSizeLimit; +use super::StreamableHttpRedirectMode; +use super::mcp_redirect_policy; use super::protocol_headers; #[test] @@ -25,6 +29,64 @@ fn protocol_headers_preserve_utf8_values() { ); } +#[test] +fn legacy_configured_headers_follow_redirects() { + assert_eq!( + mcp_redirect_policy( + StreamableHttpRedirectMode::Legacy, + &HeaderMap::new(), + /*has_configured_headers*/ true, + ), + HttpRedirectPolicy::Follow + ); +} + +#[test] +fn agent_plugin_configured_headers_stop_redirects() { + assert_eq!( + mcp_redirect_policy( + StreamableHttpRedirectMode::AgentPluginV1, + &HeaderMap::new(), + /*has_configured_headers*/ true, + ), + HttpRedirectPolicy::Stop + ); +} + +#[test] +fn requests_without_sensitive_headers_follow_redirects() { + assert_eq!( + mcp_redirect_policy( + StreamableHttpRedirectMode::AgentPluginV1, + &HeaderMap::new(), + /*has_configured_headers*/ false, + ), + HttpRedirectPolicy::Follow + ); +} + +#[test] +fn authorization_redirects_depend_on_mode() { + let mut headers = HeaderMap::new(); + headers.insert(AUTHORIZATION, HeaderValue::from_static("Bearer secret")); + assert_eq!( + mcp_redirect_policy( + StreamableHttpRedirectMode::Legacy, + &headers, + /*has_configured_headers*/ false, + ), + HttpRedirectPolicy::Follow + ); + assert_eq!( + mcp_redirect_policy( + StreamableHttpRedirectMode::AgentPluginV1, + &headers, + /*has_configured_headers*/ false, + ), + HttpRedirectPolicy::Stop + ); +} + #[test] fn lf_terminators_reset_the_event_limit() { let mut limit = SseEventSizeLimit::new(Some(8)); diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 0ed0f5d054..80ac068684 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -24,6 +24,7 @@ pub use auth_status::determine_streamable_http_auth_status; pub use auth_status::determine_streamable_http_auth_status_from_credentials; pub use auth_status::discover_streamable_http_oauth; pub use codex_protocol::protocol::McpAuthStatus; +pub use http_client_adapter::StreamableHttpRedirectMode; pub use in_process_transport::InProcessTransportFactory; pub use oauth::StoredOAuthTokens; pub use oauth::WrappedOAuthTokenResponse; diff --git a/codex-rs/rmcp-client/src/oauth_http_client.rs b/codex-rs/rmcp-client/src/oauth_http_client.rs index 4787238c7f..681887c3d7 100644 --- a/codex-rs/rmcp-client/src/oauth_http_client.rs +++ b/codex-rs/rmcp-client/src/oauth_http_client.rs @@ -8,6 +8,7 @@ use codex_exec_server::HttpHeader; use codex_exec_server::HttpRedirectPolicy; use codex_exec_server::HttpRequestParams; use http::HeaderMap; +use http::header::AUTHORIZATION; use oauth2::HttpRequest; use oauth2::HttpResponse; use rmcp::transport::auth::OAuthHttpClient; @@ -17,6 +18,7 @@ use rmcp::transport::auth::OAuthHttpRedirectPolicy; use rmcp::transport::auth::OAuthHttpRequest; use crate::auth_status::OAuthDiscoveryTimeout; +use crate::http_client_adapter::StreamableHttpRedirectMode; const MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES: usize = 1024 * 1024; static NEXT_OAUTH_REQUEST_ID: AtomicU64 = AtomicU64::new(0); @@ -40,26 +42,50 @@ pub(crate) struct OAuthHttpClientAdapter { http_client: Arc, default_headers: HeaderMap, timeout: OAuthDiscoveryTimeout, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, } impl OAuthHttpClientAdapter { + #[cfg(test)] pub(crate) fn new(http_client: Arc, default_headers: HeaderMap) -> Self { Self { http_client, default_headers, timeout: OAuthDiscoveryTimeout::Requested, + has_configured_headers: false, + redirect_mode: StreamableHttpRedirectMode::Legacy, } } - pub(crate) fn new_with_max_timeout( + pub(crate) fn new_with_redirect_mode( + http_client: Arc, + default_headers: HeaderMap, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, + ) -> Self { + Self { + http_client, + default_headers, + timeout: OAuthDiscoveryTimeout::Requested, + has_configured_headers, + redirect_mode, + } + } + + pub(crate) fn new_with_max_timeout_and_redirect_mode( http_client: Arc, default_headers: HeaderMap, max_timeout: Duration, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, ) -> Self { Self { http_client, default_headers, timeout: OAuthDiscoveryTimeout::Capped(max_timeout), + has_configured_headers, + redirect_mode, } } @@ -84,6 +110,12 @@ impl OAuthHttpClientAdapter { headers.remove(name); } headers.extend(parts.headers); + let redirect_policy = oauth_redirect_policy( + self.redirect_mode, + &headers, + self.has_configured_headers, + redirect_policy, + ); let headers = headers .iter() @@ -139,8 +171,84 @@ impl OAuthHttpClientAdapter { } } +fn oauth_redirect_policy( + mode: StreamableHttpRedirectMode, + headers: &HeaderMap, + has_configured_headers: bool, + requested_policy: HttpRedirectPolicy, +) -> HttpRedirectPolicy { + if mode == StreamableHttpRedirectMode::AgentPluginV1 + && (has_configured_headers || headers.contains_key(AUTHORIZATION)) + { + HttpRedirectPolicy::Stop + } else { + requested_policy + } +} + impl OAuthHttpClient for OAuthHttpClientAdapter { fn execute(&self, request: OAuthHttpRequest) -> OAuthHttpClientFuture<'_> { Box::pin(self.execute_request(request.request, request.redirect_policy, request.timeout)) } } + +#[cfg(test)] +mod tests { + use http::HeaderValue; + use pretty_assertions::assert_eq; + + use super::*; + + #[test] + fn agent_plugin_oauth_stops_only_for_sensitive_headers() { + assert_eq!( + policy( + StreamableHttpRedirectMode::AgentPluginV1, + /*has_configured_headers*/ true, + /*has_authorization*/ false, + ), + HttpRedirectPolicy::Stop + ); + assert_eq!( + policy( + StreamableHttpRedirectMode::AgentPluginV1, + /*has_configured_headers*/ false, + /*has_authorization*/ true, + ), + HttpRedirectPolicy::Stop + ); + assert_eq!( + policy( + StreamableHttpRedirectMode::AgentPluginV1, + /*has_configured_headers*/ false, + /*has_authorization*/ false, + ), + HttpRedirectPolicy::Follow + ); + assert_eq!( + policy( + StreamableHttpRedirectMode::Legacy, + /*has_configured_headers*/ true, + /*has_authorization*/ true, + ), + HttpRedirectPolicy::Follow + ); + } + + fn policy( + mode: StreamableHttpRedirectMode, + has_configured_headers: bool, + has_authorization: bool, + ) -> HttpRedirectPolicy { + let mut headers = HeaderMap::new(); + if has_authorization { + headers.insert(AUTHORIZATION, HeaderValue::from_static("Bearer secret")); + } + oauth_redirect_policy( + mode, + &headers, + has_configured_headers, + HttpRedirectPolicy::Follow, + ) + } +} diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index 023d0cf9e9..4178464187 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -27,6 +27,7 @@ use urlencoding::decode; use crate::StoredOAuthTokens; use crate::WrappedOAuthTokenResponse; +use crate::http_client_adapter::StreamableHttpRedirectMode; use crate::oauth::compute_expires_at_millis; use crate::oauth_http_client::OAuthHttpClientAdapter; use crate::save_oauth_tokens; @@ -38,6 +39,7 @@ struct OAuthHttpContext { http_headers: Option>, env_http_headers: Option>, http_client: Arc, + redirect_mode: StreamableHttpRedirectMode, } struct CallbackServerGuard { @@ -109,6 +111,7 @@ pub async fn perform_oauth_login( callback_url, http_client, /*emit_browser_url*/ true, + StreamableHttpRedirectMode::Legacy, ) .await } @@ -127,6 +130,7 @@ pub async fn perform_oauth_login_silent( callback_port: Option, callback_url: Option<&str>, http_client: Arc, + redirect_mode: StreamableHttpRedirectMode, ) -> Result<()> { perform_oauth_login_with_browser_output( server_name, @@ -142,6 +146,7 @@ pub async fn perform_oauth_login_silent( callback_url, http_client, /*emit_browser_url*/ false, + redirect_mode, ) .await } @@ -161,11 +166,13 @@ async fn perform_oauth_login_with_browser_output( callback_url: Option<&str>, http_client: Arc, emit_browser_url: bool, + redirect_mode: StreamableHttpRedirectMode, ) -> Result<()> { let http_context = OAuthHttpContext { http_headers, env_http_headers, http_client, + redirect_mode, }; OauthLoginFlow::new( server_name, @@ -201,11 +208,13 @@ pub async fn perform_oauth_login_return_url( callback_port: Option, callback_url: Option<&str>, http_client: Arc, + redirect_mode: StreamableHttpRedirectMode, ) -> Result { let http_context = OAuthHttpContext { http_headers, env_http_headers, http_client, + redirect_mode, }; let flow = OauthLoginFlow::new( server_name, @@ -515,9 +524,21 @@ impl OauthLoginFlow { http_headers, env_http_headers, http_client, + redirect_mode, } = http_context; + let has_configured_headers = http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()) + || env_http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()); let default_headers = build_default_headers(http_headers, env_http_headers)?; - let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new(http_client, default_headers)); + let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode( + http_client, + default_headers, + has_configured_headers, + redirect_mode, + )); let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect(); let oauth_state = start_authorization( @@ -728,6 +749,7 @@ mod tests { use super::CallbackOutcome; use super::OAuthHttpClientAdapter; use super::OAuthProviderError; + use super::StreamableHttpRedirectMode; use super::append_callback_id_to_redirect_uri; use super::append_query_param; use super::callback_id_from_server_url; @@ -975,6 +997,7 @@ mod tests { /*callback_port*/ None, /*callback_url*/ None, http_client.clone(), + StreamableHttpRedirectMode::Legacy, ) .await .expect_err("OAuth metadata discovery should fail through the supplied client"); diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 74cc903704..2f1501d596 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -70,6 +70,7 @@ use tracing::warn; use crate::elicitation_client_service::ElicitationClientService; use crate::http_client_adapter::StreamableHttpClientAdapter; use crate::http_client_adapter::StreamableHttpClientAdapterError; +use crate::http_client_adapter::StreamableHttpRedirectMode; use crate::in_process_transport::InProcessTransportFactory; use crate::oauth::OAuthPersistor; use crate::oauth::ResolvedOAuthCredentialStore; @@ -139,6 +140,7 @@ enum TransportRecipe { pinned_credential_store: Arc>, http_client: Arc, auth_provider: Option, + redirect_mode: StreamableHttpRedirectMode, }, } @@ -480,6 +482,36 @@ impl RmcpClient { http_client: Arc, auth_provider: Option, protocol_mode: McpProtocolMode, + ) -> Result { + Self::new_streamable_http_client_with_protocol_mode_and_redirect_mode( + server_name, + url, + bearer_token, + http_headers, + env_http_headers, + store_mode, + keyring_backend_kind, + http_client, + auth_provider, + protocol_mode, + StreamableHttpRedirectMode::Legacy, + ) + .await + } + + #[allow(clippy::too_many_arguments)] + pub async fn new_streamable_http_client_with_protocol_mode_and_redirect_mode( + server_name: &str, + url: &str, + bearer_token: Option, + http_headers: Option>, + env_http_headers: Option>, + store_mode: OAuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + http_client: Arc, + auth_provider: Option, + protocol_mode: McpProtocolMode, + redirect_mode: StreamableHttpRedirectMode, ) -> Result { let transport_recipe = TransportRecipe::StreamableHttp { server_name: server_name.to_string(), @@ -492,6 +524,7 @@ impl RmcpClient { pinned_credential_store: Arc::new(OnceLock::new()), http_client, auth_provider, + redirect_mode, }; let transport = Self::create_pending_transport(&transport_recipe).await?; Ok(Self { @@ -914,7 +947,14 @@ impl RmcpClient { pinned_credential_store, http_client, auth_provider, + redirect_mode, } => { + let has_configured_headers = http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()) + || env_http_headers + .as_ref() + .is_some_and(|headers| !headers.is_empty()); let default_headers = build_default_headers(http_headers.clone(), env_http_headers.clone())?; let auth_provider = @@ -976,6 +1016,8 @@ impl RmcpClient { credential_store, default_headers.clone(), Arc::clone(http_client), + has_configured_headers, + *redirect_mode, ) .await { @@ -1007,6 +1049,8 @@ impl RmcpClient { Arc::clone(http_client), default_headers, /*auth_provider*/ None, + has_configured_headers, + *redirect_mode, ), http_config, ); @@ -1026,6 +1070,8 @@ impl RmcpClient { Arc::clone(http_client), default_headers, auth_provider, + has_configured_headers, + *redirect_mode, ), http_config, ); @@ -1333,6 +1379,7 @@ impl RmcpClient { } } +#[allow(clippy::too_many_arguments)] async fn create_oauth_transport_and_runtime( server_name: &str, url: &str, @@ -1340,13 +1387,17 @@ async fn create_oauth_transport_and_runtime( credential_store: ResolvedOAuthCredentialStore, default_headers: HeaderMap, http_client: Arc, + has_configured_headers: bool, + redirect_mode: StreamableHttpRedirectMode, ) -> Result<( StreamableHttpClientTransport>, OAuthPersistor, )> { - let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new( + let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode( http_client.clone(), default_headers.clone(), + has_configured_headers, + redirect_mode, )); let mut manager = AuthorizationManager::new_with_oauth_http_client(url.to_string(), oauth_http_client) @@ -1370,7 +1421,13 @@ async fn create_oauth_transport_and_runtime( }; let auth_client = AuthClient::new( - StreamableHttpClientAdapter::new(http_client, default_headers, /*auth_provider*/ None), + StreamableHttpClientAdapter::new( + http_client, + default_headers, + /*auth_provider*/ None, + has_configured_headers, + redirect_mode, + ), manager, ); let auth_manager = auth_client.auth_manager.clone(); diff --git a/codex-rs/rmcp-client/tests/mcp_2026_discovery.rs b/codex-rs/rmcp-client/tests/mcp_2026_discovery.rs index ed77a2a559..72968a75d3 100644 --- a/codex-rs/rmcp-client/tests/mcp_2026_discovery.rs +++ b/codex-rs/rmcp-client/tests/mcp_2026_discovery.rs @@ -230,7 +230,7 @@ async fn modern_discovery_does_not_follow_redirects_with_sensitive_headers() -> } #[tokio::test] -async fn legacy_mcp_requests_preserve_existing_redirect_behavior() -> anyhow::Result<()> { +async fn legacy_mcp_requests_follow_redirects_with_configured_headers() -> anyhow::Result<()> { let redirect_target = MockServer::start().await; Mock::given(method("POST")) .and(path("/forwarded")) diff --git a/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs b/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs index 8d861b5730..9c7aa54fca 100644 --- a/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs +++ b/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs @@ -6,6 +6,7 @@ use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpOAuthDiscovery; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::discover_streamable_http_oauth; use pretty_assertions::assert_eq; use rmcp::transport::auth::AuthError; @@ -108,6 +109,7 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session( OutboundProxyPolicy::ReqwestDefault, ))), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await; let routed_discovery = discover_streamable_http_oauth( @@ -118,6 +120,7 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session( OutboundProxyPolicy::ReqwestDefault, ))), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await; @@ -197,6 +200,7 @@ async fn oauth_discovery_does_not_invent_support_for_an_unauthenticated_legacy_s OutboundProxyPolicy::ReqwestDefault, ))), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await?; let local_discovery = discover_streamable_http_oauth( @@ -207,6 +211,7 @@ async fn oauth_discovery_does_not_invent_support_for_an_unauthenticated_legacy_s OutboundProxyPolicy::ReqwestDefault, ))), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await?; diff --git a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs index df99ca9bbb..7266cfb817 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs @@ -12,6 +12,7 @@ use codex_rmcp_client::McpLoginRequirement; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::RmcpClient; use codex_rmcp_client::StoredOAuthTokens; +use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::WrappedOAuthTokenResponse; use codex_rmcp_client::determine_streamable_http_auth_status; use codex_rmcp_client::is_authentication_required_error; @@ -297,6 +298,7 @@ async fn auth_status(server_url: &str) -> anyhow::Result { AuthKeyringBackendKind::default(), Environment::default_for_tests().get_http_client(), OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, ) .await } diff --git a/codex-rs/tools/src/lib.rs b/codex-rs/tools/src/lib.rs index de5749afa7..6f90e65c99 100644 --- a/codex-rs/tools/src/lib.rs +++ b/codex-rs/tools/src/lib.rs @@ -38,6 +38,7 @@ pub use json_schema::JsonSchemaType; pub use json_schema::parse_tool_input_schema; pub use json_schema::parse_tool_input_schema_without_compaction; pub use mcp_tool::mcp_call_tool_result_output_schema; +pub use mcp_tool::parse_agent_plugin_mcp_tool; pub use mcp_tool::parse_mcp_tool; pub use request_plugin_install::REQUEST_PLUGIN_INSTALL_APPROVAL_KIND_VALUE; pub use request_plugin_install::REQUEST_PLUGIN_INSTALL_PERSIST_ALWAYS_VALUE; @@ -56,6 +57,7 @@ pub use responses_api::LoadableToolSpec; pub use responses_api::ResponsesApiNamespace; pub use responses_api::ResponsesApiNamespaceTool; pub use responses_api::ResponsesApiTool; +pub use responses_api::agent_plugin_mcp_tool_to_responses_api_tool; pub use responses_api::coalesce_loadable_tool_specs; pub use responses_api::default_namespace_description; pub use responses_api::dynamic_tool_to_responses_api_tool; diff --git a/codex-rs/tools/src/mcp_tool.rs b/codex-rs/tools/src/mcp_tool.rs index 337a8e42ad..4ab24b9cd4 100644 --- a/codex-rs/tools/src/mcp_tool.rs +++ b/codex-rs/tools/src/mcp_tool.rs @@ -1,9 +1,25 @@ use crate::ToolDefinition; use crate::parse_tool_input_schema; +use codex_utils_string::take_bytes_at_char_boundary; use serde_json::Value as JsonValue; use serde_json::json; +const MAX_MCP_TOOL_DESCRIPTION_BYTES: usize = 1_000; + pub fn parse_mcp_tool(tool: &rmcp::model::Tool) -> Result { + parse_mcp_tool_with_description_limit(tool, /*description_limit*/ None) +} + +pub fn parse_agent_plugin_mcp_tool( + tool: &rmcp::model::Tool, +) -> Result { + parse_mcp_tool_with_description_limit(tool, Some(MAX_MCP_TOOL_DESCRIPTION_BYTES)) +} + +fn parse_mcp_tool_with_description_limit( + tool: &rmcp::model::Tool, + description_limit: Option, +) -> Result { let mut serialized_input_schema = serde_json::Value::Object(tool.input_schema.as_ref().clone()); // OpenAI models mandate the "properties" field in the schema. Some MCP @@ -27,7 +43,14 @@ pub fn parse_mcp_tool(tool: &rmcp::model::Tool) -> Result take_bytes_at_char_boundary(description, limit).to_string(), + None => description.to_string(), + }) + .unwrap_or_default(), input_schema, output_schema: Some(mcp_call_tool_result_output_schema( structured_content_schema, diff --git a/codex-rs/tools/src/mcp_tool_tests.rs b/codex-rs/tools/src/mcp_tool_tests.rs index 98f8885a95..5f9b7b3264 100644 --- a/codex-rs/tools/src/mcp_tool_tests.rs +++ b/codex-rs/tools/src/mcp_tool_tests.rs @@ -1,4 +1,5 @@ use super::mcp_call_tool_result_output_schema; +use super::parse_agent_plugin_mcp_tool; use super::parse_mcp_tool; use crate::JsonSchema; use crate::ToolDefinition; @@ -39,6 +40,40 @@ fn parse_mcp_tool_inserts_empty_properties() { ); } +#[test] +fn agent_plugin_mcp_tool_bounds_model_visible_description() { + let description = format!("{}é", "a".repeat(super::MAX_MCP_TOOL_DESCRIPTION_BYTES - 1)); + let tool = mcp_tool( + "bounded_description", + &description, + serde_json::json!({"type": "object"}), + ); + + let parsed = parse_agent_plugin_mcp_tool(&tool).expect("parse Agent Plugin MCP tool"); + + assert_eq!( + parsed.description.len(), + super::MAX_MCP_TOOL_DESCRIPTION_BYTES - 1 + ); +} + +#[test] +fn legacy_mcp_tool_preserves_long_description() { + let description = "a".repeat(super::MAX_MCP_TOOL_DESCRIPTION_BYTES + 100); + let tool = mcp_tool( + "legacy_description", + &description, + serde_json::json!({"type": "object"}), + ); + + assert_eq!( + parse_mcp_tool(&tool) + .expect("parse legacy MCP tool") + .description, + description + ); +} + #[test] fn parse_mcp_tool_preserves_top_level_output_schema() { let mut tool = mcp_tool( diff --git a/codex-rs/tools/src/responses_api.rs b/codex-rs/tools/src/responses_api.rs index e63372ac94..e450dcf35f 100644 --- a/codex-rs/tools/src/responses_api.rs +++ b/codex-rs/tools/src/responses_api.rs @@ -1,6 +1,7 @@ use crate::JsonSchema; use crate::ToolDefinition; use crate::ToolName; +use crate::parse_agent_plugin_mcp_tool; use crate::parse_dynamic_tool; use crate::parse_mcp_tool; use codex_protocol::DEFAULT_FUNCTION_NAMESPACE; @@ -9,6 +10,8 @@ use serde::Deserialize; use serde::Serialize; use serde_json::Value; +const MAX_SERIALIZED_MCP_TOOL_BYTES: usize = 8_000; + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct FreeformTool { pub name: String, @@ -123,6 +126,23 @@ pub fn mcp_tool_to_responses_api_tool( )) } +pub fn agent_plugin_mcp_tool_to_responses_api_tool( + tool_name: &ToolName, + tool: &rmcp::model::Tool, +) -> Result { + let mut tool = tool_definition_to_responses_api_tool( + parse_agent_plugin_mcp_tool(tool)?.renamed(tool_name.name.clone()), + ); + if serde_json::to_vec(&tool)?.len() > MAX_SERIALIZED_MCP_TOOL_BYTES { + tool.parameters = JsonSchema::object( + Default::default(), + /*required*/ None, + Some(true.into()), + ); + } + Ok(tool) +} + pub fn mcp_tool_to_deferred_responses_api_tool( tool_name: &ToolName, tool: &rmcp::model::Tool, diff --git a/codex-rs/tools/src/responses_api_tests.rs b/codex-rs/tools/src/responses_api_tests.rs index f12d078d26..43212060a3 100644 --- a/codex-rs/tools/src/responses_api_tests.rs +++ b/codex-rs/tools/src/responses_api_tests.rs @@ -3,6 +3,7 @@ use super::LoadableToolSpec; use super::ResponsesApiNamespace; use super::ResponsesApiNamespaceTool; use super::ResponsesApiTool; +use super::agent_plugin_mcp_tool_to_responses_api_tool; use super::dynamic_tool_to_responses_api_tool; use super::mcp_tool_to_deferred_responses_api_tool; use super::tool_definition_to_responses_api_tool; @@ -153,6 +154,46 @@ fn mcp_tool_to_deferred_responses_api_tool_sets_defer_loading() { ); } +#[test] +fn agent_plugin_mcp_tool_uses_fallback_for_oversized_schema() { + let properties: serde_json::Map = (0..1_024) + .map(|index| (format!("property_{index}"), json!({"type": "string"}))) + .collect(); + let tool = rmcp::model::Tool::new( + "oversized", + "Large schema", + std::sync::Arc::new(rmcp::model::object(json!({ + "type": "object", + "properties": properties, + }))), + ); + + assert_eq!( + agent_plugin_mcp_tool_to_responses_api_tool(&ToolName::from("oversized"), &tool) + .expect("Agent Plugin MCP tool should use a fallback schema") + .parameters, + JsonSchema::object(BTreeMap::new(), /*required*/ None, Some(true.into())) + ); +} + +#[test] +fn legacy_mcp_tool_accepts_oversized_schema() { + let properties: serde_json::Map = (0..1_024) + .map(|index| (format!("property_{index}"), json!({"type": "string"}))) + .collect(); + let tool = rmcp::model::Tool::new( + "oversized", + "Large legacy schema", + std::sync::Arc::new(rmcp::model::object(json!({ + "type": "object", + "properties": properties, + }))), + ); + + super::mcp_tool_to_responses_api_tool(&ToolName::from("oversized"), &tool) + .expect("legacy MCP conversion must preserve existing acceptance"); +} + #[test] fn loadable_tool_spec_namespace_serializes_with_deferred_child_tools() { let namespace = LoadableToolSpec::Namespace(ResponsesApiNamespace { diff --git a/codex-rs/tui/src/app/plugin_mentions.rs b/codex-rs/tui/src/app/plugin_mentions.rs index dd162af0d6..a71f4fa8d6 100644 --- a/codex-rs/tui/src/app/plugin_mentions.rs +++ b/codex-rs/tui/src/app/plugin_mentions.rs @@ -50,6 +50,7 @@ fn plugin_mention_from_summary( Some(PluginCapabilitySummary { config_name: plugin.id.clone(), display_name: plugin_mention_display_name(&plugin), + plugin_namespace: Some(plugin.name.clone()), description: plugin_mention_description(marketplace_name, &plugin), has_skills: false, mcp_server_names: Vec::new(), @@ -129,6 +130,7 @@ mod tests { PluginCapabilitySummary { config_name: "active@server-marketplace".to_string(), display_name: "active".to_string(), + plugin_namespace: Some("active".to_string()), description: Some("server-marketplace".to_string()), has_skills: false, mcp_server_names: Vec::new(), @@ -137,6 +139,7 @@ mod tests { PluginCapabilitySummary { config_name: "active-shared@server-marketplace".to_string(), display_name: "active-shared".to_string(), + plugin_namespace: Some("active-shared".to_string()), description: Some("server-marketplace".to_string()), has_skills: false, mcp_server_names: Vec::new(), diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index fe3cd178c5..9574b615ed 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -6721,6 +6721,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "Sample Plugin".to_string(), + plugin_namespace: None, description: None, has_skills: true, mcp_server_names: vec!["sample".to_string()], @@ -6773,6 +6774,7 @@ mod tests { PluginCapabilitySummary { config_name: format!("{name}@test"), display_name: name.to_string(), + plugin_namespace: None, description: Some(description.to_string()), has_skills: false, mcp_server_names: vec![name.to_string()], @@ -7445,6 +7447,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "google-calendar@debug".to_string(), display_name: "Google Calendar".to_string(), + plugin_namespace: None, description: Some( "Connect Google Calendar for scheduling, availability, and event management." .to_string(), @@ -7497,6 +7500,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "Sample Plugin".to_string(), + plugin_namespace: None, description: Some( "Plugin that includes the Figma MCP server and Skills for common workflows" .to_string(), @@ -7522,6 +7526,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "Sample Plugin".to_string(), + plugin_namespace: None, description: Some("Plugin with skills and an MCP server".to_string()), has_skills: true, mcp_server_names: vec!["sample".to_string()], @@ -7629,6 +7634,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "google-calendar@debug".to_string(), display_name: "Google Calendar".to_string(), + plugin_namespace: None, description: Some( "Connect Google Calendar for scheduling, availability, and event management." .to_string(), @@ -8197,6 +8203,7 @@ mod tests { composer.set_plugin_mentions(Some(vec![PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "sample".to_string(), + plugin_namespace: None, description: None, has_skills: true, mcp_server_names: vec!["sample".to_string()], diff --git a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs index 98a832335c..5b809dac71 100644 --- a/codex-rs/tui/src/chatwidget/tests/plan_mode.rs +++ b/codex-rs/tui/src/chatwidget/tests/plan_mode.rs @@ -1310,6 +1310,7 @@ async fn submit_user_message_emits_structured_plugin_mentions_from_bindings() { .set_plugin_mentions(Some(vec![codex_plugin::PluginCapabilitySummary { config_name: "sample@test".to_string(), display_name: "Sample Plugin".to_string(), + plugin_namespace: None, description: None, has_skills: true, mcp_server_names: Vec::new(),