mirror of
https://github.com/openai/codex.git
synced 2026-11-07 20:30:33 +00:00
Preserve plugin caches across display metadata refreshes (#46309)
## Why Renewed image URLs and other display metadata changes unnecessarily invalidate loaded plugins and MCP and skill caches, even when installed plugin behavior is unchanged. ## What changed - Compare installed plugin metadata by identity, version, enablement, policy, and availability before invalidating derived caches. Continue storing the full updated payload so display consumers receive fresh metadata. - Preserve invalidation when behavioral metadata changes or reconciliation requires an effective plugin refresh. - Export `remote_catalog_metadata_eq` to compare catalogs independently of display metadata and plugin display order, while retaining marketplace order significance. ## Testing Add regression tests for display-only updates, behavioral changes, catalog ordering, and preservation of loaded skills and tool suggestions. Add an app-server integration test verifying that image URL renewals and badge updates preserve live MCP sessions and cached skill resources, while an authentication policy change invalidates resource caches. GitOrigin-RevId: 22b9ba1234a9f850201c6890e03d1ef899a56c54
This commit is contained in:
@@ -25,6 +25,7 @@ mod recommended_plugin_install;
|
||||
pub mod remote;
|
||||
pub mod remote_bundle;
|
||||
pub mod remote_legacy;
|
||||
mod remote_metadata;
|
||||
mod remote_plugin_id_resolver;
|
||||
mod script_attribution;
|
||||
mod skill_snapshots;
|
||||
@@ -105,6 +106,7 @@ pub use provider::ResolvedExecutorPlugin;
|
||||
pub use recommended_plugin_install::hydrate_selected_recommended_plugin_install_metadata;
|
||||
pub use remote::RecommendedPlugin;
|
||||
pub use remote::RecommendedPluginsMode;
|
||||
pub use remote_metadata::remote_catalog_metadata_eq;
|
||||
pub use script_attribution::PluginCommandAttribution;
|
||||
pub use script_attribution::TrustedPluginRoots;
|
||||
pub use script_attribution::command_script_arguments;
|
||||
|
||||
@@ -1417,10 +1417,16 @@ impl PluginsManager {
|
||||
}
|
||||
return Some(needs_effective_plugins_refresh);
|
||||
}
|
||||
let metadata_changed = cache.plugins.as_ref().is_none_or(|previous| {
|
||||
!crate::remote_metadata::installed_plugin_metadata_eq(previous, &plugins)
|
||||
});
|
||||
cache.plugins = Some(plugins);
|
||||
drop(cache);
|
||||
self.clear_loaded_plugins_cache();
|
||||
Some(true)
|
||||
let changed = needs_effective_plugins_refresh || metadata_changed;
|
||||
if changed {
|
||||
self.clear_loaded_plugins_cache();
|
||||
}
|
||||
Some(changed)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -78,6 +78,9 @@ const MAX_CAPABILITY_SUMMARY_DESCRIPTION_LEN: usize = 1024;
|
||||
#[path = "marketplace_policy/curated_loading_tests.rs"]
|
||||
mod curated_marketplace_policy;
|
||||
|
||||
#[path = "remote_metadata_cache_tests.rs"]
|
||||
mod remote_metadata_cache;
|
||||
|
||||
fn unrestricted_config_layer_stack() -> ConfigLayerStack {
|
||||
ConfigLayerStack::default()
|
||||
}
|
||||
@@ -7270,7 +7273,10 @@ remote_plugin = true
|
||||
/*on_effective_plugins_changed*/ None,
|
||||
);
|
||||
|
||||
tokio::time::sleep(Duration::from_millis(400)).await;
|
||||
let _guard = first_manager
|
||||
.acquire_remote_installed_plugin_sync_guard()
|
||||
.await
|
||||
.expect("background bundle sync should finish");
|
||||
server.verify().await;
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
//! Compares the metadata that determines installed-plugin behavior.
|
||||
//!
|
||||
//! Display metadata (including image URLs and store capability badges) is excluded. Callers must
|
||||
//! still publish the unmodified payload so display consumers receive fresh metadata.
|
||||
|
||||
use crate::remote::RemoteInstalledPlugin;
|
||||
use crate::remote::RemoteMarketplace;
|
||||
|
||||
pub(crate) fn installed_plugin_metadata_eq(
|
||||
previous: &[RemoteInstalledPlugin],
|
||||
current: &[RemoteInstalledPlugin],
|
||||
) -> bool {
|
||||
previous.len() == current.len()
|
||||
&& previous.iter().zip(current).all(|(previous, current)| {
|
||||
previous.marketplace_name == current.marketplace_name
|
||||
&& previous.id == current.id
|
||||
&& previous.name == current.name
|
||||
&& previous.canonical_app_id == current.canonical_app_id
|
||||
&& previous.version == current.version
|
||||
&& previous.installed_at == current.installed_at
|
||||
&& previous.enabled == current.enabled
|
||||
&& previous.install_policy == current.install_policy
|
||||
&& previous.install_policy_source == current.install_policy_source
|
||||
&& previous.must_show_installation_interstitial
|
||||
== current.must_show_installation_interstitial
|
||||
&& previous.auth_policy == current.auth_policy
|
||||
&& previous.availability == current.availability
|
||||
&& previous.disabled_reason == current.disabled_reason
|
||||
&& previous.eligible_plan_types == current.eligible_plan_types
|
||||
})
|
||||
}
|
||||
|
||||
/// Compare installed catalog identity, versions, enablement, policy and availability.
|
||||
///
|
||||
/// Plugin display order is ignored. Marketplace ordering remains significant.
|
||||
/// Callers must still store the unmodified current catalog for display consumers.
|
||||
pub fn remote_catalog_metadata_eq(
|
||||
previous: &[RemoteMarketplace],
|
||||
current: &[RemoteMarketplace],
|
||||
) -> bool {
|
||||
previous.len() == current.len()
|
||||
&& previous.iter().zip(current).all(|(previous, current)| {
|
||||
if previous.name != current.name || previous.plugins.len() != current.plugins.len() {
|
||||
return false;
|
||||
}
|
||||
// Core sorts this view by display name, which can change independently of behavior.
|
||||
let mut previous = previous.plugins.iter().collect::<Vec<_>>();
|
||||
let mut current = current.plugins.iter().collect::<Vec<_>>();
|
||||
previous.sort_unstable_by(|a, b| a.remote_plugin_id.cmp(&b.remote_plugin_id));
|
||||
current.sort_unstable_by(|a, b| a.remote_plugin_id.cmp(&b.remote_plugin_id));
|
||||
previous
|
||||
.into_iter()
|
||||
.zip(current)
|
||||
.all(|(previous, current)| {
|
||||
previous.id == current.id
|
||||
&& previous.remote_plugin_id == current.remote_plugin_id
|
||||
&& previous.name == current.name
|
||||
&& previous.version == current.version
|
||||
&& previous.local_version == current.local_version
|
||||
&& previous.installed == current.installed
|
||||
&& previous.installed_at == current.installed_at
|
||||
&& previous.enabled == current.enabled
|
||||
&& previous.install_policy == current.install_policy
|
||||
&& previous.install_policy_source == current.install_policy_source
|
||||
&& previous.must_show_installation_interstitial
|
||||
== current.must_show_installation_interstitial
|
||||
&& previous.auth_policy == current.auth_policy
|
||||
&& previous.availability == current.availability
|
||||
&& previous.disabled_reason == current.disabled_reason
|
||||
&& previous.eligible_plan_types == current.eligible_plan_types
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "remote_metadata_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,135 @@
|
||||
//! Metadata publication must preserve derived caches until runtime inputs change.
|
||||
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[tokio::test]
|
||||
async fn display_refresh_preserves_loaded_skills_and_tool_suggestions() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
let marketplace_root = codex_home
|
||||
.path()
|
||||
.join("plugins/cache/openai-curated-remote");
|
||||
write_plugin(&marketplace_root, "sample/local", "sample");
|
||||
let plugin_root = marketplace_root.join("sample/local");
|
||||
write_file(
|
||||
&codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"[features]
|
||||
plugins = true
|
||||
remote_plugin = true
|
||||
|
||||
[plugins."sample@openai-curated-remote"]
|
||||
enabled = true
|
||||
"#,
|
||||
);
|
||||
let config = load_config(codex_home.path(), codex_home.path()).await;
|
||||
let manager = test_plugins_manager_with_options(
|
||||
codex_home.path().to_path_buf(),
|
||||
Some(Product::Codex),
|
||||
Some(AuthMode::Chatgpt),
|
||||
);
|
||||
let mut remote = remote_installed_plugin("sample");
|
||||
remote.interface = Some(
|
||||
serde_json::from_value(serde_json::json!({
|
||||
"capabilities": [], "screenshots": [], "screenshotUrls": [],
|
||||
"logoUrl": "https://files.openai.com/logo.png?sig=old"
|
||||
}))
|
||||
.unwrap(),
|
||||
);
|
||||
assert!(manager.write_remote_installed_plugins_cache(vec![remote.clone()]));
|
||||
|
||||
let loaded = manager.plugins_for_config(&config).await;
|
||||
let snapshots = manager.plugin_skill_snapshots_for_config(&config).unwrap();
|
||||
let roots = loaded.effective_plugin_skill_roots();
|
||||
assert_eq!(roots.len(), 1);
|
||||
assert_eq!(snapshots.get(&roots[0]).unwrap().skills.len(), 1);
|
||||
let loaded_generation = manager.loaded_plugins_cache_generation();
|
||||
let plugin = ConfiguredMarketplacePlugin {
|
||||
id: "sample@openai-curated-remote".to_string(),
|
||||
name: "sample".to_string(),
|
||||
local_version: None,
|
||||
installed_version: None,
|
||||
source: MarketplacePluginSource::Local {
|
||||
path: plugin_root.abs(),
|
||||
},
|
||||
policy: MarketplacePluginPolicy {
|
||||
installation: MarketplacePluginInstallPolicy::Available,
|
||||
authentication: MarketplacePluginAuthPolicy::OnUse,
|
||||
products: None,
|
||||
},
|
||||
interface: None,
|
||||
keywords: Vec::new(),
|
||||
manifest_fallback: None,
|
||||
installed: true,
|
||||
enabled: true,
|
||||
};
|
||||
let suggestions = manager
|
||||
.tool_suggest_metadata_cache
|
||||
.metadata_for_plugin(
|
||||
REMOTE_GLOBAL_MARKETPLACE_NAME,
|
||||
&plugin,
|
||||
manager.restriction_product,
|
||||
manager.skill_root_loader.as_ref(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
suggestions
|
||||
.project(&SkillConfigRules::default(), manager.auth_mode())
|
||||
.has_skills
|
||||
);
|
||||
|
||||
let interface = remote.interface.as_mut().unwrap();
|
||||
interface.logo_url = Some("https://files.openai.com/logo.png?sig=new".to_string());
|
||||
interface.display_name = Some("Updated display name".to_string());
|
||||
interface.capabilities = vec!["Updated store badge".to_string()];
|
||||
assert!(!manager.write_remote_installed_plugins_cache(vec![remote.clone()]));
|
||||
assert_eq!(
|
||||
manager
|
||||
.remote_installed_plugins_cache
|
||||
.read()
|
||||
.unwrap()
|
||||
.plugins,
|
||||
Some(vec![remote.clone()])
|
||||
);
|
||||
assert_eq!(manager.loaded_plugins_cache_generation(), loaded_generation);
|
||||
assert_eq!(
|
||||
manager.plugin_skill_snapshots_for_config(&config),
|
||||
Some(snapshots.clone())
|
||||
);
|
||||
assert_eq!(manager.plugins_for_config(&config).await, loaded);
|
||||
let refreshed_suggestions = manager
|
||||
.tool_suggest_metadata_cache
|
||||
.metadata_for_plugin(
|
||||
REMOTE_GLOBAL_MARKETPLACE_NAME,
|
||||
&plugin,
|
||||
manager.restriction_product,
|
||||
manager.skill_root_loader.as_ref(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(Arc::ptr_eq(&suggestions, &refreshed_suggestions));
|
||||
|
||||
remote.version = Some("2.0.0".to_string());
|
||||
assert!(manager.write_remote_installed_plugins_cache(vec![remote]));
|
||||
assert_eq!(
|
||||
manager.loaded_plugins_cache_generation(),
|
||||
loaded_generation + 1
|
||||
);
|
||||
assert_eq!(manager.plugin_skill_snapshots_for_config(&config), None);
|
||||
manager.plugins_for_config(&config).await;
|
||||
assert_ne!(
|
||||
manager.plugin_skill_snapshots_for_config(&config),
|
||||
Some(snapshots)
|
||||
);
|
||||
let changed_suggestions = manager
|
||||
.tool_suggest_metadata_cache
|
||||
.metadata_for_plugin(
|
||||
REMOTE_GLOBAL_MARKETPLACE_NAME,
|
||||
&plugin,
|
||||
manager.restriction_product,
|
||||
manager.skill_root_loader.as_ref(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!Arc::ptr_eq(&suggestions, &changed_suggestions));
|
||||
}
|
||||
@@ -0,0 +1,181 @@
|
||||
//! Regression coverage for the fields that affect plugin behavior.
|
||||
|
||||
use super::*;
|
||||
use crate::remote::REMOTE_GLOBAL_MARKETPLACE_NAME;
|
||||
use crate::remote::group_remote_installed_plugins_by_marketplaces;
|
||||
use codex_app_server_protocol::PluginAuthPolicy;
|
||||
use codex_app_server_protocol::PluginAvailability;
|
||||
use codex_app_server_protocol::PluginInstallPolicy;
|
||||
use serde_json::json;
|
||||
|
||||
fn image_url(renewal: &str) -> String {
|
||||
format!("https://files.openai.com/plugins/icon.png?sig={renewal}")
|
||||
}
|
||||
|
||||
fn plugin(renewal: &str) -> RemoteInstalledPlugin {
|
||||
RemoteInstalledPlugin {
|
||||
marketplace_name: REMOTE_GLOBAL_MARKETPLACE_NAME.to_string(),
|
||||
id: "plugin-test".to_string(),
|
||||
version: Some("1.0.0".to_string()),
|
||||
name: "test".to_string(),
|
||||
canonical_app_id: None,
|
||||
installed_at: None,
|
||||
enabled: true,
|
||||
install_policy: PluginInstallPolicy::Available,
|
||||
install_policy_source: None,
|
||||
must_show_installation_interstitial: None,
|
||||
auth_policy: PluginAuthPolicy::OnUse,
|
||||
availability: PluginAvailability::Available,
|
||||
disabled_reason: None,
|
||||
eligible_plan_types: None,
|
||||
interface: Some(
|
||||
serde_json::from_value(json!({
|
||||
"capabilities": [], "screenshots": [],
|
||||
"logoUrl": image_url(renewal), "logoUrlDark": image_url(renewal),
|
||||
"composerIconUrl": image_url(renewal), "screenshotUrls": [image_url(renewal)]
|
||||
}))
|
||||
.unwrap(),
|
||||
),
|
||||
keywords: vec![],
|
||||
}
|
||||
}
|
||||
|
||||
fn catalog(plugins: &[RemoteInstalledPlugin]) -> Vec<RemoteMarketplace> {
|
||||
group_remote_installed_plugins_by_marketplaces(plugins, &[REMOTE_GLOBAL_MARKETPLACE_NAME])
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn behavioral_metadata_changes_still_invalidate() {
|
||||
let changes: &[fn(&mut RemoteInstalledPlugin)] = &[
|
||||
|plugin| plugin.id.push_str("-other"),
|
||||
|plugin| plugin.name.push_str("-other"),
|
||||
|plugin| plugin.version = Some("2.0.0".to_string()),
|
||||
|plugin| {
|
||||
plugin.installed_at =
|
||||
chrono::DateTime::from_timestamp(/*secs*/ 1, /*nsecs*/ 0)
|
||||
},
|
||||
|plugin| plugin.enabled = false,
|
||||
|plugin| plugin.install_policy = PluginInstallPolicy::NotAvailable,
|
||||
|plugin| {
|
||||
plugin.install_policy_source =
|
||||
Some(codex_app_server_protocol::PluginInstallPolicySource::WorkspaceSetting)
|
||||
},
|
||||
|plugin| plugin.must_show_installation_interstitial = Some(true),
|
||||
|plugin| plugin.auth_policy = PluginAuthPolicy::OnInstall,
|
||||
|plugin| plugin.availability = PluginAvailability::DisabledByAdmin,
|
||||
|plugin| plugin.eligible_plan_types = Some(vec!["enterprise".to_string()]),
|
||||
];
|
||||
for change in changes {
|
||||
let previous = [plugin("old")];
|
||||
let mut current = [plugin("new")];
|
||||
change(&mut current[0]);
|
||||
assert!(!installed_plugin_metadata_eq(&previous, ¤t));
|
||||
assert!(!remote_catalog_metadata_eq(
|
||||
&catalog(&previous),
|
||||
&catalog(¤t)
|
||||
));
|
||||
}
|
||||
let previous = [plugin("old")];
|
||||
let mut current = [plugin("new")];
|
||||
current[0].canonical_app_id = Some("new-app".to_string());
|
||||
assert!(!installed_plugin_metadata_eq(&previous, ¤t));
|
||||
assert!(!installed_plugin_metadata_eq(&previous, &[]));
|
||||
assert!(!remote_catalog_metadata_eq(&catalog(&previous), &[]));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn display_metadata_does_not_invalidate() {
|
||||
let previous = [plugin("old")];
|
||||
let mut current = [plugin("new")];
|
||||
current[0].keywords.push("new keyword".to_string());
|
||||
let interface = current[0].interface.as_mut().unwrap();
|
||||
interface.logo_url = Some("https://another-host.test/different-image.svg".to_string());
|
||||
interface.display_name = Some("New display name".to_string());
|
||||
interface.short_description = Some("New description".to_string());
|
||||
interface.brand_color = Some("#123456".to_string());
|
||||
interface.default_prompt = Some(vec!["New starter prompt".to_string()]);
|
||||
interface.website_url = Some("https://another-host.test".to_string());
|
||||
assert_ne!(previous, current);
|
||||
assert!(installed_plugin_metadata_eq(&previous, ¤t));
|
||||
let mut current_catalog = catalog(¤t);
|
||||
current_catalog[0].display_name = "New marketplace title".to_string();
|
||||
assert!(remote_catalog_metadata_eq(
|
||||
&catalog(&previous),
|
||||
¤t_catalog
|
||||
));
|
||||
|
||||
current[0].interface = None;
|
||||
assert!(installed_plugin_metadata_eq(&previous, ¤t));
|
||||
assert!(remote_catalog_metadata_eq(
|
||||
&catalog(&previous),
|
||||
&catalog(¤t)
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn installed_ordering_remains_significant() {
|
||||
let mut other = plugin("old");
|
||||
other.id.push_str("-other");
|
||||
other.name.push_str("-other");
|
||||
let previous = [plugin("old"), other];
|
||||
let mut current = previous.clone();
|
||||
current.reverse();
|
||||
assert!(!installed_plugin_metadata_eq(&previous, ¤t));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn store_badge_changes_do_not_invalidate() {
|
||||
let mut previous = [plugin("old")];
|
||||
previous[0].interface.as_mut().unwrap().capabilities =
|
||||
vec!["tools".to_string(), "skills".to_string()];
|
||||
for badges in [vec!["skills", "tools"], vec!["updated badge"], vec![]] {
|
||||
let mut current = previous.clone();
|
||||
current[0].interface.as_mut().unwrap().capabilities =
|
||||
badges.into_iter().map(str::to_string).collect();
|
||||
assert_ne!(previous, current);
|
||||
assert!(installed_plugin_metadata_eq(&previous, ¤t));
|
||||
assert!(remote_catalog_metadata_eq(
|
||||
&catalog(&previous),
|
||||
&catalog(¤t)
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn catalog_display_reordering_does_not_invalidate() {
|
||||
let mut first = plugin("old");
|
||||
first.interface.as_mut().unwrap().display_name = Some("Alpha".to_string());
|
||||
let mut second = plugin("old");
|
||||
second.id.push_str("-second");
|
||||
second.name.push_str("-second");
|
||||
second.interface.as_mut().unwrap().display_name = Some("Beta".to_string());
|
||||
let previous = [first, second];
|
||||
let mut current = previous.clone();
|
||||
current[0].interface.as_mut().unwrap().display_name = Some("Zulu".to_string());
|
||||
let previous_catalog = catalog(&previous);
|
||||
let current_catalog = catalog(¤t);
|
||||
assert_ne!(
|
||||
previous_catalog[0].plugins[0].id,
|
||||
current_catalog[0].plugins[0].id
|
||||
);
|
||||
assert!(installed_plugin_metadata_eq(&previous, ¤t));
|
||||
assert!(remote_catalog_metadata_eq(
|
||||
&previous_catalog,
|
||||
¤t_catalog
|
||||
));
|
||||
|
||||
current[0].version = Some("2.0.0".to_string());
|
||||
assert!(!remote_catalog_metadata_eq(
|
||||
&previous_catalog,
|
||||
&catalog(¤t)
|
||||
));
|
||||
assert!(!remote_catalog_metadata_eq(
|
||||
&previous_catalog,
|
||||
&catalog(¤t[1..])
|
||||
));
|
||||
current[0] = current[1].clone();
|
||||
assert!(!remote_catalog_metadata_eq(
|
||||
&previous_catalog,
|
||||
&catalog(¤t)
|
||||
));
|
||||
}
|
||||
Reference in New Issue
Block a user