fix: Distinguish missing and empty plugin products (#15263)
Treat [] as no product allowed, empty as all products allowed.
This commit is contained in:
parent
a3e59e9e85
commit
35f8b87a5b
4 changed files with 183 additions and 29 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<Product>,
|
||||
pub products: Option<Vec<Product>>,
|
||||
}
|
||||
|
||||
#[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<Product>,
|
||||
products: Option<Vec<Product>>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
|
|
|
|||
|
|
@ -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`"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue