From af1fc2dbff641e78298c272c4b45c9fec5c33898 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Tue, 15 Sep 2026 17:59:35 +0000 Subject: [PATCH] Honor canonical plugin disables for shared connectors (#45755) ## Why Disabling a connector's canonical plugin could leave its tools available when another enabled plugin contributed the same connector. ## What changed Retain `canonical_app_id` in the remote installed-plugin cache and use ownership metadata for the current account when building connector snapshots. A disabled canonical owner now excludes its connector even when another plugin contributes it or the owner's bundle is absent from the host. Match owners by plugin name and marketplace, and ignore ownership metadata when the cache no longer matches the current authentication. Build these snapshots through `PluginsManager` when the plugins feature is enabled; otherwise use an empty snapshot. ## Testing Add manager coverage for combined local and canonical exclusions, marketplace matching, and authentication changes. Add an integration test showing that disabling a noncanonical contributor preserves shared calendar tools, disabling the canonical owner hides them, and clearing the exclusion restores them. GitOrigin-RevId: ee2981d1277825fefb671dd7d1078cd7ea5c0ea5 --- codex-rs/connectors/src/snapshot.rs | 15 +- codex-rs/connectors/src/snapshot_tests.rs | 26 +++- codex-rs/core-plugins/src/manager.rs | 36 +++++ codex-rs/core-plugins/src/manager_tests.rs | 50 ++++++ codex-rs/core-plugins/src/remote.rs | 4 + codex-rs/core-plugins/src/remote_tests.rs | 1 + codex-rs/core/src/mcp.rs | 13 +- .../suite/canonical_plugin_connectors.rs | 142 ++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 1 + 9 files changed, 275 insertions(+), 13 deletions(-) create mode 100644 codex-rs/core/tests/suite/canonical_plugin_connectors.rs diff --git a/codex-rs/connectors/src/snapshot.rs b/codex-rs/connectors/src/snapshot.rs index 7011a74d3c..4129f03988 100644 --- a/codex-rs/connectors/src/snapshot.rs +++ b/codex-rs/connectors/src/snapshot.rs @@ -79,10 +79,11 @@ pub struct ConnectorSnapshot { impl ConnectorSnapshot { /// Builds the final selection from all plugin sources, preserving contribution order. - /// Pass host and selected sources together so any enabled shared owner keeps a connector. + /// An enabled contributor preserves a shared connector unless its canonical owner is disabled. pub fn from_plugin_sources( sources: impl IntoIterator, disabled_plugin_ids: &[String], + canonical_disabled_connector_ids: HashSet, ) -> Self { let mut connector_ids = Vec::new(); let mut seen_connector_ids = HashSet::new(); @@ -95,6 +96,9 @@ impl ConnectorSnapshot { continue; } for connector_id in source.connector_ids() { + if canonical_disabled_connector_ids.contains(&connector_id.0) { + continue; + } if seen_connector_ids.insert(connector_id.0.clone()) { connector_ids.push(connector_id.clone()); } @@ -109,6 +113,7 @@ impl ConnectorSnapshot { plugin_names.dedup(); } disabled_connector_ids.retain(|id| !seen_connector_ids.contains(id)); + disabled_connector_ids.extend(canonical_disabled_connector_ids); Self { connector_ids, @@ -119,7 +124,11 @@ impl ConnectorSnapshot { /// Adapts the current host plugin summaries to the connector-owned snapshot. pub fn from_plugin_capability_summaries(summaries: &[PluginCapabilitySummary]) -> Self { - Self::from_plugin_sources(summaries.iter().map(PluginConnectorSource::from), &[]) + Self::from_plugin_sources( + summaries.iter().map(PluginConnectorSource::from), + &[], + HashSet::new(), + ) } /// Returns the connector IDs in source contribution order. @@ -127,7 +136,7 @@ impl ConnectorSnapshot { &self.connector_ids } - /// Connector tools excluded because no enabled plugin still contributes them. + /// Connector tools explicitly excluded or excluded by all contributing plugins. pub fn disabled_connector_ids(&self) -> &HashSet { &self.disabled_connector_ids } diff --git a/codex-rs/connectors/src/snapshot_tests.rs b/codex-rs/connectors/src/snapshot_tests.rs index e4f0bc2714..e6eb2b6b07 100644 --- a/codex-rs/connectors/src/snapshot_tests.rs +++ b/codex-rs/connectors/src/snapshot_tests.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use codex_plugin::AppConnectorId; use pretty_assertions::assert_eq; @@ -15,7 +17,11 @@ fn snapshot_merges_sources_in_order_and_dedupes_provenance() { source("selected-b", "Alpha", &["calendar"]), ]; - let merged = ConnectorSnapshot::from_plugin_sources(host.into_iter().chain(selected), &[]); + let merged = ConnectorSnapshot::from_plugin_sources( + host.into_iter().chain(selected), + &[], + HashSet::new(), + ); assert_eq!( merged.connector_ids(), @@ -40,13 +46,18 @@ fn disabled_plugins_preserve_shared_connectors() { source("alpha", "Alpha", &["exclusive", "shared"]), source("beta", "Beta", &["other", "shared"]), ]; - let filtered = ConnectorSnapshot::from_plugin_sources(sources.clone(), &["alpha".to_string()]); + let filtered = ConnectorSnapshot::from_plugin_sources( + sources.clone(), + &["alpha".to_string()], + HashSet::new(), + ); let expected = ConnectorSnapshot { - disabled_connector_ids: std::collections::HashSet::from(["exclusive".to_string()]), + disabled_connector_ids: HashSet::from(["exclusive".to_string()]), ..ConnectorSnapshot::from_plugin_sources( [source("beta", "Beta", &["other", "shared"])], &[], + HashSet::new(), ) }; assert_eq!(filtered, expected); @@ -54,13 +65,18 @@ fn disabled_plugins_preserve_shared_connectors() { ConnectorSnapshot::from_plugin_sources( sources.iter().rev().cloned(), &["alpha".to_string()], + HashSet::new(), ), expected ); assert_eq!( - ConnectorSnapshot::from_plugin_sources(sources, &["alpha".to_string(), "beta".to_string()],), + ConnectorSnapshot::from_plugin_sources( + sources, + &["alpha".to_string(), "beta".to_string()], + HashSet::new(), + ), ConnectorSnapshot { - disabled_connector_ids: std::collections::HashSet::from([ + disabled_connector_ids: HashSet::from([ "exclusive".to_string(), "other".to_string(), "shared".to_string(), diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 69269e67ed..ab56913d94 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -99,6 +99,8 @@ use codex_config::skill_config_rules_from_stack; use codex_config::types::PluginConfig; use codex_config::types::ToolSuggestDisabledTool; use codex_config::types::ToolSuggestDiscoverableType; +use codex_connectors::ConnectorSnapshot; +use codex_connectors::PluginConnectorSource; use codex_hooks::plugin_hook_declarations; use codex_http_client::HttpClientFactory; use codex_login::AuthManager; @@ -1024,6 +1026,40 @@ impl PluginsManager { } } + /// Applies plugin exclusions and canonical ownership from the current account's installed cache. + /// A canonical owner's bundle need not be installed on this host. + pub fn connector_snapshot( + &self, + sources: impl IntoIterator, + disabled_plugin_ids: &[String], + ) -> ConnectorSnapshot { + let mut canonical_app_ids = HashSet::new(); + if !disabled_plugin_ids.is_empty() { + let cache = match self.remote_installed_plugins_cache.read() { + Ok(cache) => cache, + Err(err) => err.into_inner(), + }; + if self.remote_installed_plugins_cache_matches_current_auth(&cache) { + canonical_app_ids = cache + .plugins + .as_deref() + .unwrap_or_default() + .iter() + .filter_map(|plugin| { + let app_id = plugin.canonical_app_id.as_ref()?; + let plugin_id = + PluginId::new(plugin.name.clone(), plugin.marketplace_name.clone()) + .ok()?; + disabled_plugin_ids + .contains(&plugin_id.as_key()) + .then(|| app_id.clone()) + }) + .collect(); + } + } + ConnectorSnapshot::from_plugin_sources(sources, disabled_plugin_ids, canonical_app_ids) + } + fn remote_installed_plugin_configs(&self) -> HashMap { let cache = match self.remote_installed_plugins_cache.read() { Ok(cache) => cache, diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 50d3598399..26930a88ba 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -864,6 +864,7 @@ fn remote_installed_plugin_in_marketplace( marketplace_name: &str, ) -> RemoteInstalledPlugin { RemoteInstalledPlugin { + canonical_app_id: None, marketplace_name: marketplace_name.to_string(), id: format!("plugins~Plugin_{name}"), version: None, @@ -3102,6 +3103,55 @@ enabled = true } } +#[tokio::test] +async fn connector_snapshot_combines_plugin_exclusions_with_current_account_ownership() { + let codex_home = TempDir::new().unwrap(); + let auth_manager = test_auth_manager(Some(AuthMode::Chatgpt)); + let manager = test_plugins_manager_with_auth_manager( + codex_home.path().to_path_buf(), + Some(Product::Codex), + Arc::clone(&auth_manager), + ); + let sources = [PluginConnectorSource::from_connector_ids( + "local@test", + "Local", + [AppConnectorId("local-connector".to_string())], + )]; + let disabled = vec![ + "linear@openai-curated-remote".to_string(), + "local@test".to_string(), + ]; + let local_exclusion = HashSet::from(["local-connector".to_string()]); + assert_eq!( + manager + .connector_snapshot(sources.clone(), &disabled) + .disabled_connector_ids(), + &local_exclusion, + ); + let mut plugin = remote_installed_linear_plugin(); + plugin.canonical_app_id = Some("linear".to_string()); + manager.write_remote_installed_plugins_cache(vec![plugin]); + assert_eq!( + manager + .connector_snapshot(sources.clone(), &disabled) + .disabled_connector_ids(), + &HashSet::from(["linear".to_string(), "local-connector".to_string()]), + ); + assert!( + manager + .connector_snapshot(sources.clone(), &["linear@another-marketplace".to_string()]) + .disabled_connector_ids() + .is_empty() + ); + set_test_auth_mode(&auth_manager, Some(AuthMode::ApiKey)).await; + assert_eq!( + manager + .connector_snapshot(sources, &disabled) + .disabled_connector_ids(), + &local_exclusion, + ); +} + #[test] fn loaded_plugins_cache_evicts_least_recently_used_configuration() { let codex_home = TempDir::new().unwrap(); diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 18cd4ff967..86ba6960f4 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -230,6 +230,7 @@ pub struct RemoteInstalledPlugin { pub id: String, pub version: Option, pub name: String, + pub canonical_app_id: Option, pub installed_at: Option>, pub enabled: bool, pub install_policy: PluginInstallPolicy, @@ -682,6 +683,8 @@ impl RemotePluginInstallPolicySource { struct RemotePluginDirectoryItem { id: String, name: String, + #[serde(default)] + canonical_app_id: Option, scope: RemotePluginScope, #[serde(default)] discoverability: Option, @@ -1879,6 +1882,7 @@ fn remote_installed_plugin_to_cache_entry( id: plugin.id.clone(), version: plugin.release.version.clone(), name: plugin.name.clone(), + canonical_app_id: plugin.canonical_app_id.clone(), installed_at: installed_plugin.installed_at, enabled: installed_plugin.enabled, install_policy: plugin.installation_policy, diff --git a/codex-rs/core-plugins/src/remote_tests.rs b/codex-rs/core-plugins/src/remote_tests.rs index 1d8bc682ea..1dbc7b8b37 100644 --- a/codex-rs/core-plugins/src/remote_tests.rs +++ b/codex-rs/core-plugins/src/remote_tests.rs @@ -596,6 +596,7 @@ fn workspace_share_context_preserves_publish_capability() { fn directory_plugin(id: &str, name: &str) -> RemotePluginDirectoryItem { RemotePluginDirectoryItem { + canonical_app_id: None, id: id.to_string(), name: name.to_string(), scope: RemotePluginScope::Global, diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index 8b7617f035..9766373ee8 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -6,7 +6,6 @@ use crate::environment_selection::ThreadEnvironments; use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; use codex_config::McpServerConfig; use codex_connectors::ConnectorRuntimeManager; -use codex_connectors::ConnectorSnapshot; use codex_connectors::PluginConnectorSource; use codex_core_plugins::PluginsManager; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; @@ -273,10 +272,14 @@ impl McpManager { .capability_summaries() .iter() .map(PluginConnectorSource::from); - let connector_snapshot = ConnectorSnapshot::from_plugin_sources( - host_plugin_connector_sources.chain(selected_plugin_connector_sources), - disabled_plugin_ids, - ); + let connector_snapshot = if config.features.enabled(Feature::Plugins) { + self.plugins_manager.connector_snapshot( + host_plugin_connector_sources.chain(selected_plugin_connector_sources), + disabled_plugin_ids, + ) + } else { + Default::default() + }; let loaded_plugins = loaded_plugins.without_plugins(disabled_plugin_ids); let plugins_available = selected_plugin_available || !loaded_plugins.capability_summaries().is_empty(); diff --git a/codex-rs/core/tests/suite/canonical_plugin_connectors.rs b/codex-rs/core/tests/suite/canonical_plugin_connectors.rs new file mode 100644 index 0000000000..7ff9977907 --- /dev/null +++ b/codex-rs/core/tests/suite/canonical_plugin_connectors.rs @@ -0,0 +1,142 @@ +//! Canonical connector ownership overrides other plugins' shared contributions. + +use std::sync::Arc; + +use anyhow::Result; +use codex_core::TurnInputRequest; +use codex_features::Feature; +use codex_login::CodexAuth; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::ev_tool_search_call; +use core_test_support::responses::mount_sse_sequence; +use core_test_support::responses::namespace_child_tool; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use wiremock::Mock; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn canonical_plugin_disable_overrides_shared_connector_and_can_be_cleared() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let apps = AppsTestServer::mount_with_connector_name(&server, "Google Calendar").await?; + Mock::given(method("GET")) + .and(path("/ps/plugins/installed")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "plugins": [{ + "id": "plugins~Plugin_calendar", + "name": "calendar", + "scope": "GLOBAL", + "status": "ENABLED", + "installation_policy": "AVAILABLE", + "authentication_policy": "ON_USE", + "canonical_app_id": "calendar", + "release": { + "version": "local", + "display_name": "Calendar", + "description": "Calendar connector", + "interface": {}, + }, + "enabled": true, + }], + "pagination": {"next_page_token": null}, + }))) + .mount(&server) + .await; + let home = Arc::new(TempDir::new()?); + std::fs::write( + home.path().join("config.toml"), + "[features]\nplugins = true\nremote_plugin = true\n[plugins.\"shared@test\"]\nenabled = true\n", + )?; + for (marketplace, name) in [("test", "shared"), ("openai-curated-remote", "calendar")] { + let root = home + .path() + .join(format!("plugins/cache/{marketplace}/{name}/local")); + std::fs::create_dir_all(root.join(".codex-plugin"))?; + std::fs::write( + root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{name}"}}"#), + )?; + std::fs::write( + root.join(".app.json"), + r#"{"apps":{"calendar":{"id":"calendar"}}}"#, + )?; + } + let mut builder = test_codex() + .with_home(home) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(move |config| { + config.features.enable(Feature::Apps).unwrap(); + config.chatgpt_base_url = apps.chatgpt_base_url; + }); + let test = builder.build_with_auto_env(&server).await?; + let manager = test.thread_manager.plugins_manager(); + let auth = test.thread_manager.auth_manager().auth().await; + manager + .reconcile_remote_installed_plugins(&test.config.plugins_config_input(), auth.as_ref()) + .await?; + for (phase, (disabled, visible)) in [ + (vec!["shared@test"], true), + (vec!["calendar@openai-curated-remote"], false), + (vec![], true), + ] + .into_iter() + .enumerate() + { + let call_id = format!("calendar-search-{phase}"); + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("search"), + ev_tool_search_call( + &call_id, + &serde_json::json!({"query":"create calendar event"}), + ), + ev_completed("search"), + ]), + sse(vec![ev_response_created("done"), ev_completed("done")]), + ], + ) + .await; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "Find the calendar tool.".to_string(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + disabled_plugin_ids: Some(disabled.into_iter().map(str::to_string).collect()), + ..Default::default() + }), + ) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + assert_eq!( + namespace_child_tool( + &mock.requests()[1].tool_search_output(&call_id), + "mcp__codex_apps__google_calendar", + SEARCH_CALENDAR_CREATE_TOOL, + ) + .is_some(), + visible + ); + } + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 67cfcb47eb..6e08c580dc 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -74,6 +74,7 @@ mod fork_thread; mod git_enrichment; mod guardian_authorization; // Uses the same command-approval harness as guardian_review below. +mod canonical_plugin_connectors; #[cfg(not(target_os = "windows"))] mod guardian_context_budget; mod guardian_history;