From 35f8b87a5b396ac9780fa0100cf6fb1af5a5e282 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Thu, 19 Mar 2026 20:02:40 -0700 Subject: [PATCH] fix: Distinguish missing and empty plugin products (#15263) Treat [] as no product allowed, empty as all products allowed. --- codex-rs/core/src/plugins/manager.rs | 18 ++-- codex-rs/core/src/plugins/manager_tests.rs | 86 +++++++++++++++-- codex-rs/core/src/plugins/marketplace.rs | 15 +-- .../core/src/plugins/marketplace_tests.rs | 93 +++++++++++++++++-- 4 files changed, 183 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index 936dc48fd..aabe77812 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -500,11 +500,14 @@ impl PluginsManager { *stored_client = Some(analytics_events_client); } - fn restriction_product_matches(&self, products: &[Product]) -> bool { - products.is_empty() - || self + fn restriction_product_matches(&self, products: Option<&[Product]>) -> bool { + match products { + None => true, + Some([]) => false, + Some(products) => self .restriction_product - .is_some_and(|product| product.matches_product_restriction(products)) + .is_some_and(|product| product.matches_product_restriction(products)), + } } pub fn plugins_for_config(&self, config: &Config) -> PluginLoadOutcome { @@ -830,7 +833,8 @@ impl PluginsManager { .get(&plugin_key) .map(|plugin| plugin.enabled); let installed_version = self.store.active_plugin_version(&plugin_id); - let product_allowed = self.restriction_product_matches(&plugin.policy.products); + let product_allowed = + self.restriction_product_matches(plugin.policy.products.as_deref()); local_plugins.push(( plugin_name, plugin_id, @@ -991,7 +995,7 @@ impl PluginsManager { if !seen_plugin_keys.insert(plugin_key.clone()) { return None; } - if !self.restriction_product_matches(&plugin.policy.products) { + if !self.restriction_product_matches(plugin.policy.products.as_deref()) { return None; } @@ -1041,7 +1045,7 @@ impl PluginsManager { marketplace_name, }); }; - if !self.restriction_product_matches(&plugin.policy.products) { + if !self.restriction_product_matches(plugin.policy.products.as_deref()) { return Err(MarketplaceError::PluginNotFound { plugin_name: request.plugin_name.clone(), marketplace_name, diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index 49a63eee4..6f474c747 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -976,7 +976,7 @@ enabled = false policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: true, @@ -992,7 +992,7 @@ enabled = false policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: true, @@ -1043,6 +1043,80 @@ enabled = true assert_eq!(marketplaces, Vec::new()); } +#[tokio::test] +async fn list_marketplaces_excludes_plugins_with_explicit_empty_products() { + let tmp = tempfile::tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + fs::write( + repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "debug", + "plugins": [ + { + "name": "disabled-plugin", + "source": { + "source": "local", + "path": "./disabled-plugin" + }, + "policy": { + "products": [] + } + }, + { + "name": "default-plugin", + "source": { + "source": "local", + "path": "./default-plugin" + } + } + ] +}"#, + ) + .unwrap(); + write_file( + &tmp.path().join(CONFIG_TOML_FILE), + r#"[features] +plugins = true +"#, + ); + + let config = load_config(tmp.path(), &repo_root).await; + let marketplaces = PluginsManager::new(tmp.path().to_path_buf()) + .list_marketplaces_for_config(&config, &[AbsolutePathBuf::try_from(repo_root).unwrap()]) + .unwrap(); + + let marketplace = marketplaces + .into_iter() + .find(|marketplace| { + marketplace.path + == AbsolutePathBuf::try_from( + tmp.path().join("repo/.agents/plugins/marketplace.json"), + ) + .unwrap() + }) + .expect("expected repo marketplace entry"); + assert_eq!( + marketplace.plugins, + vec![ConfiguredMarketplacePlugin { + id: "default-plugin@debug".to_string(), + name: "default-plugin".to_string(), + source: MarketplacePluginSource::Local { + path: AbsolutePathBuf::try_from(tmp.path().join("repo/default-plugin")).unwrap(), + }, + policy: MarketplacePluginPolicy { + installation: MarketplacePluginInstallPolicy::Available, + authentication: MarketplacePluginAuthPolicy::OnInstall, + products: None, + }, + interface: None, + installed: false, + enabled: false, + }] + ); +} + #[tokio::test] async fn read_plugin_for_config_returns_plugins_disabled_when_feature_disabled() { let tmp = tempfile::tempdir().unwrap(); @@ -1177,7 +1251,7 @@ plugins = true policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: false, @@ -1280,7 +1354,7 @@ enabled = false policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: false, @@ -1309,7 +1383,7 @@ enabled = false policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: false, @@ -1391,7 +1465,7 @@ enabled = true policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, installed: false, diff --git a/codex-rs/core/src/plugins/marketplace.rs b/codex-rs/core/src/plugins/marketplace.rs index 4c3564ee7..17b37f8cc 100644 --- a/codex-rs/core/src/plugins/marketplace.rs +++ b/codex-rs/core/src/plugins/marketplace.rs @@ -57,7 +57,7 @@ pub struct MarketplacePluginPolicy { pub authentication: MarketplacePluginAuthPolicy, // TODO: Surface or enforce product gating at the Codex/plugin consumer boundary instead of // only carrying it through core marketplace metadata. - pub products: Vec, + pub products: Option>, } #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize)] @@ -169,9 +169,13 @@ pub fn resolve_marketplace_plugin( .. } = plugin; let install_policy = policy.installation; - let product_allowed = policy.products.is_empty() - || restriction_product - .is_some_and(|product| product.matches_product_restriction(&policy.products)); + let product_allowed = match policy.products.as_deref() { + None => true, + Some([]) => false, + Some(products) => { + restriction_product.is_some_and(|product| product.matches_product_restriction(products)) + } + }; if install_policy == MarketplacePluginInstallPolicy::NotAvailable || !product_allowed { return Err(MarketplaceError::PluginNotAvailable { plugin_name: name, @@ -432,8 +436,7 @@ struct RawMarketplaceManifestPluginPolicy { installation: MarketplacePluginInstallPolicy, #[serde(default)] authentication: MarketplacePluginAuthPolicy, - #[serde(default)] - products: Vec, + products: Option>, } #[derive(Debug, Deserialize)] diff --git a/codex-rs/core/src/plugins/marketplace_tests.rs b/codex-rs/core/src/plugins/marketplace_tests.rs index d15b628e3..faf60250a 100644 --- a/codex-rs/core/src/plugins/marketplace_tests.rs +++ b/codex-rs/core/src/plugins/marketplace_tests.rs @@ -150,7 +150,7 @@ fn list_marketplaces_returns_home_and_repo_marketplaces() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }, @@ -162,7 +162,7 @@ fn list_marketplaces_returns_home_and_repo_marketplaces() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }, @@ -183,7 +183,7 @@ fn list_marketplaces_returns_home_and_repo_marketplaces() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }, @@ -195,7 +195,7 @@ fn list_marketplaces_returns_home_and_repo_marketplaces() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }, @@ -271,7 +271,7 @@ fn list_marketplaces_keeps_distinct_entries_for_same_name() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }], @@ -288,7 +288,7 @@ fn list_marketplaces_keeps_distinct_entries_for_same_name() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }], @@ -359,7 +359,7 @@ fn list_marketplaces_dedupes_multiple_roots_in_same_repo() { policy: MarketplacePluginPolicy { installation: MarketplacePluginInstallPolicy::Available, authentication: MarketplacePluginAuthPolicy::OnInstall, - products: vec![], + products: None, }, interface: None, }], @@ -522,7 +522,7 @@ fn list_marketplaces_resolves_plugin_interface_paths_to_absolute() { ); assert_eq!( marketplaces[0].plugins[0].policy.products, - vec![Product::Codex, Product::Chatgpt, Product::Atlas] + Some(vec![Product::Codex, Product::Chatgpt, Product::Atlas]) ); assert_eq!( marketplaces[0].plugins[0].interface, @@ -587,7 +587,7 @@ fn list_marketplaces_ignores_legacy_top_level_policy_fields() { marketplaces[0].plugins[0].policy.authentication, MarketplacePluginAuthPolicy::OnInstall ); - assert_eq!(marketplaces[0].plugins[0].policy.products, Vec::new()); + assert_eq!(marketplaces[0].plugins[0].policy.products, None); } #[test] @@ -661,7 +661,7 @@ fn list_marketplaces_ignores_plugin_interface_assets_without_dot_slash() { marketplaces[0].plugins[0].policy.authentication, MarketplacePluginAuthPolicy::OnInstall ); - assert_eq!(marketplaces[0].plugins[0].policy.products, Vec::new()); + assert_eq!(marketplaces[0].plugins[0].policy.products, None); } #[test] @@ -784,3 +784,76 @@ fn resolve_marketplace_plugin_rejects_disallowed_product() { "plugin `chatgpt-plugin` is not available for install in marketplace `codex-curated`" ); } + +#[test] +fn resolve_marketplace_plugin_allows_missing_products_field() { + let tmp = tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + fs::write( + repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "codex-curated", + "plugins": [ + { + "name": "default-plugin", + "source": { + "source": "local", + "path": "./plugin" + }, + "policy": {} + } + ] +}"#, + ) + .unwrap(); + + let resolved = resolve_marketplace_plugin( + &AbsolutePathBuf::try_from(repo_root.join(".agents/plugins/marketplace.json")).unwrap(), + "default-plugin", + Some(Product::Codex), + ) + .unwrap(); + + assert_eq!(resolved.plugin_id.as_key(), "default-plugin@codex-curated"); +} + +#[test] +fn resolve_marketplace_plugin_rejects_explicit_empty_products() { + let tmp = tempdir().unwrap(); + let repo_root = tmp.path().join("repo"); + fs::create_dir_all(repo_root.join(".git")).unwrap(); + fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap(); + fs::write( + repo_root.join(".agents/plugins/marketplace.json"), + r#"{ + "name": "codex-curated", + "plugins": [ + { + "name": "disabled-plugin", + "source": { + "source": "local", + "path": "./plugin" + }, + "policy": { + "products": [] + } + } + ] +}"#, + ) + .unwrap(); + + let err = resolve_marketplace_plugin( + &AbsolutePathBuf::try_from(repo_root.join(".agents/plugins/marketplace.json")).unwrap(), + "disabled-plugin", + Some(Product::Codex), + ) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "plugin `disabled-plugin` is not available for install in marketplace `codex-curated`" + ); +}