diff --git a/codex-rs/app-server/src/effective_plugin_change_tests.rs b/codex-rs/app-server/src/effective_plugin_change_tests.rs index c7acea8a0c..4ea5e9edf0 100644 --- a/codex-rs/app-server/src/effective_plugin_change_tests.rs +++ b/codex-rs/app-server/src/effective_plugin_change_tests.rs @@ -14,7 +14,6 @@ fn only_workspace_listed_materializations_are_eligible() { scope, discoverability, authenticated_account_id: Some("account-123".to_string()), - capabilities: Default::default(), } }; diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index a0aa76b889..38278a68eb 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -57,7 +57,9 @@ use crate::remote::RecommendedPluginsMode; use crate::remote::RemoteInstalledPlugin; use crate::remote::RemoteInstalledPluginBundleSyncError; use crate::remote::RemoteInstalledPluginBundleSyncOutcome; +use crate::remote::RemotePluginCapabilities; use crate::remote::RemotePluginCatalogError; +use crate::remote::RemotePluginChange; use crate::remote::RemotePluginMaterialization; use crate::remote::RemotePluginScope; use crate::remote::RemotePluginServiceConfig; @@ -1502,12 +1504,12 @@ impl PluginsManager { Ok((outcome, effective_plugins_changed)) => { tracing::info!( materialized_remote_plugins = ?outcome.materialized_remote_plugins, - removed_plugins = ?outcome.removed_plugins, + changed_plugins = ?outcome.changed_plugins, failed_remote_plugin_ids = ?outcome.failed_remote_plugin_ids, failed_materialization_remote_plugin_ids = ?outcome.failed_materialization_remote_plugin_ids, "completed remote installed plugin bundle sync" ); - if (effective_plugins_changed || outcome.changed_local_cache()) + if (effective_plugins_changed || !outcome.changed_plugins.is_empty()) && let Some(on_effective_plugins_changed) = on_effective_plugins_changed { on_effective_plugins_changed(EffectivePluginsChange { @@ -1578,13 +1580,72 @@ impl PluginsManager { generation, committed: false, }; + let previous_enabled = { + let cache = match self.remote_installed_plugins_cache.read() { + Ok(cache) => cache, + Err(err) => err.into_inner(), + }; + cache.plugins.as_ref().map(|plugins| { + plugins + .iter() + .filter_map(|plugin| { + PluginId::new(plugin.name.clone(), plugin.marketplace_name.clone()) + .ok() + .map(|plugin_id| (plugin_id, plugin.enabled)) + }) + .collect::>() + }) + }; + let previous_plugin_ids = previous_enabled + .iter() + .flat_map(HashMap::keys) + .cloned() + .collect::>(); let result = crate::remote::sync_remote_installed_plugin_bundles_once_with_snapshot( self.codex_home.clone(), &remote_plugin_service_config(config), auth, + &previous_plugin_ids, ) .await?; - let outcome = result.outcome; + let mut outcome = result.outcome; + // The generation fence keeps this comparison on the snapshot replaced by this pass. + // In a known snapshot, absence means inactive: cached reinstalls need the same hints + // as re-enablement, without becoming materializations or triggering hook-trust writes. + if let Some(previous_enabled) = previous_enabled { + for plugin in &result.installed_plugins { + let plugin_id = PluginId::new(plugin.name.clone(), plugin.marketplace_name.clone()) + .map_err(|err| RemotePluginCatalogError::UnexpectedResponse(err.to_string()))?; + if previous_enabled + .get(&plugin_id) + .copied() + .unwrap_or_default() + == plugin.enabled + { + continue; + } + let plugin_key = plugin_id.as_key(); + if outcome + .changed_plugins + .iter() + .any(|change| change.plugin_id == plugin_key) + || self.store.active_plugin_root(&plugin_id).is_none() + { + continue; + } + let mut capabilities = RemotePluginCapabilities::default(); + capabilities + .include_active_bundle(&self.store, &plugin_id) + .await; + outcome.changed_plugins.push(RemotePluginChange { + plugin_id: plugin_key, + capabilities, + }); + } + } + outcome + .changed_plugins + .sort_unstable_by(|left, right| left.plugin_id.cmp(&right.plugin_id)); let Some(effective_plugins_changed) = self.write_remote_installed_plugins_cache_snapshot( generation, result.installed_plugins, @@ -1594,7 +1655,7 @@ impl PluginsManager { return Err(RemoteInstalledPluginBundleSyncError::Superseded); }; reconciliation.committed = true; - if !effective_plugins_changed && outcome.changed_local_cache() { + if !effective_plugins_changed && !outcome.changed_plugins.is_empty() { self.clear_loaded_plugins_cache(); } Ok((outcome, effective_plugins_changed)) diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 8648b0ff24..008ada15f4 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -7090,7 +7090,6 @@ fn remote_installed_plugins_cache_refresh_coalesces_materializations() { scope: crate::remote::RemotePluginScope::Workspace, discoverability: Some(crate::remote::RemotePluginShareDiscoverability::Listed), authenticated_account_id: Some("account-123".to_string()), - capabilities: Default::default(), }; let change = |name: &str| EffectivePluginsChange { materialized_remote_plugins: vec![materialization(name)], @@ -7206,6 +7205,121 @@ remote_plugin = true server.verify().await; } +#[tokio::test] +async fn reconcile_remote_installed_plugins_reports_cached_state_changes() { + let codex_home = TempDir::new().unwrap(); + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + "[features]\nplugins = true\n", + ); + write_cached_plugin( + codex_home.path(), + REMOTE_WORKSPACE_MARKETPLACE_NAME, + "linear", + ); + let plugin_root = codex_home + .path() + .join("plugins/cache/workspace-directory/linear/local"); + write_file( + &plugin_root.join(".mcp.json"), + r#"{"mcpServers":{"example":{"command":"unused"}}}"#, + ); + write_file( + &plugin_root.join("hooks/hooks.json"), + r#"{"hooks":{"UserPromptSubmit":[{"hooks":[{"type":"command","command":"echo hook"}]}]}}"#, + ); + let server = MockServer::start().await; + let mut config = load_config(codex_home.path(), codex_home.path()).await; + config.chatgpt_base_url = format!("{}/backend-api", server.uri()); + let auth_manager = test_auth_manager(Some(AuthMode::Chatgpt)); + let auth = auth_manager.auth_cached().expect("test ChatGPT auth"); + let manager = test_plugins_manager_with_auth_manager( + codex_home.path().to_path_buf(), + Some(Product::Codex), + auth_manager, + ); + let change = RemotePluginChange { + plugin_id: "linear@workspace-directory".to_string(), + capabilities: RemotePluginCapabilities { + has_mcps: true, + has_hooks: true, + has_skills: true, + ..Default::default() + }, + }; + // None represents an uninstall. Reinstalling its retained bundle must report an + // activation even though the known previous snapshot has no entry for the plugin. + for (enabled, changes) in [ + (Some(true), Vec::new()), + (Some(false), vec![change.clone()]), + (Some(false), Vec::new()), + (Some(true), vec![change.clone()]), + (None, vec![change.clone()]), + (Some(true), vec![change.clone()]), + (Some(true), Vec::new()), + ] { + if enabled.is_none() { + // Fail cleanup before it reaches the bundle; removal hints must survive. + write_file( + &codex_home + .path() + .join("plugins/cache/openai-curated-remote"), + "not a directory", + ); + } + let plugins = enabled + .into_iter() + .map(|enabled| { + serde_json::json!({ + "id": "plugins~Plugin_linear", + "name": "linear", + "scope": "WORKSPACE", + "discoverability": "LISTED", + "installation_policy": "AVAILABLE", + "authentication_policy": "ON_USE", + "release": { + "version": "local", + "display_name": "Linear", + "description": "Test plugin", + "interface": {}, + }, + "enabled": enabled, + }) + }) + .collect::>(); + // No download URL: every installed pass must reuse the cached version. + Mock::given(method("GET")) + .and(path("/backend-api/ps/plugins/installed")) + .and(query_param("includeDownloadUrls", "true")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "plugins": plugins, + "pagination": { "next_page_token": null }, + }))) + .expect(1) + .mount(&server) + .await; + assert_eq!( + manager + .reconcile_remote_installed_plugins(&config, Some(&auth)) + .await + .expect("reconcile cached plugin state"), + RemoteInstalledPluginBundleSyncOutcome { + changed_plugins: changes, + ..Default::default() + } + ); + assert!(plugin_root.exists()); + if enabled.is_none() { + assert_eq!( + manager.plugins_for_config(&config).await, + PluginLoadOutcome::default() + ); + } + server.verify().await; + server.reset().await; + } +} + #[tokio::test] async fn reconcile_remote_installed_plugins_rejects_incomplete_snapshot_without_cleanup() { 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 e2a05866bc..68e5595630 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -60,8 +60,8 @@ pub use plugin_capabilities::RemotePluginCapabilities; pub use remote_installed_plugin_sync::RemoteInstalledPluginBundleSyncError; pub use remote_installed_plugin_sync::RemoteInstalledPluginBundleSyncOutcome; pub use remote_installed_plugin_sync::RemotePluginCacheMutationGuard; +pub use remote_installed_plugin_sync::RemotePluginChange; pub use remote_installed_plugin_sync::RemotePluginMaterialization; -pub use remote_installed_plugin_sync::RemotePluginRemoval; pub use remote_installed_plugin_sync::mark_remote_plugin_cache_mutation_in_flight; pub(crate) use remote_installed_plugin_sync::remote_installed_plugin_bundle_sync_gate; pub use remote_installed_plugin_sync::sync_remote_installed_plugin_bundles_once; diff --git a/codex-rs/core-plugins/src/remote/plugin_capabilities.rs b/codex-rs/core-plugins/src/remote/plugin_capabilities.rs index 3028abd74d..4a47c0ffa6 100644 --- a/codex-rs/core-plugins/src/remote/plugin_capabilities.rs +++ b/codex-rs/core-plugins/src/remote/plugin_capabilities.rs @@ -13,7 +13,7 @@ use crate::store::PluginStore; use codex_hooks::plugin_hook_declarations; use codex_plugin::PluginId; -/// Runtime categories affected by a bundle change, before runtime policy filtering. +/// Runtime categories affected by a plugin change, before runtime policy filtering. #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct RemotePluginCapabilities { pub has_mcps: bool, @@ -23,7 +23,7 @@ pub struct RemotePluginCapabilities { } impl RemotePluginCapabilities { - pub(super) async fn include_active_bundle( + pub(crate) async fn include_active_bundle( &mut self, store: &PluginStore, plugin_id: &PluginId, diff --git a/codex-rs/core-plugins/src/remote/plugin_capabilities_tests.rs b/codex-rs/core-plugins/src/remote/plugin_capabilities_tests.rs index 48a28b4e50..77ede1bb43 100644 --- a/codex-rs/core-plugins/src/remote/plugin_capabilities_tests.rs +++ b/codex-rs/core-plugins/src/remote/plugin_capabilities_tests.rs @@ -2,7 +2,7 @@ use super::*; use crate::remote::RemoteInstalledPluginBundleSyncOutcome; -use crate::remote::RemotePluginRemoval; +use crate::remote::RemotePluginChange; use crate::remote::RemotePluginServiceConfig; use crate::remote::sync_remote_installed_plugin_bundles_once; use crate::test_support::write_file; @@ -141,7 +141,7 @@ async fn capabilities_union_cached_versions_and_sync_reports_removal() -> anyhow ) .await?, RemoteInstalledPluginBundleSyncOutcome { - removed_plugins: vec![RemotePluginRemoval { + changed_plugins: vec![RemotePluginChange { plugin_id: plugin_id.as_key(), capabilities: RemotePluginCapabilities { has_apps: true, diff --git a/codex-rs/core-plugins/src/remote/remote_installed_plugin_sync.rs b/codex-rs/core-plugins/src/remote/remote_installed_plugin_sync.rs index 04ea8e3888..380271b77f 100644 --- a/codex-rs/core-plugins/src/remote/remote_installed_plugin_sync.rs +++ b/codex-rs/core-plugins/src/remote/remote_installed_plugin_sync.rs @@ -45,21 +45,22 @@ pub struct RemotePluginMaterialization { pub scope: RemotePluginScope, pub discoverability: Option, pub authenticated_account_id: Option, - /// Runtime categories declared by either the old or new bundle. - pub capabilities: RemotePluginCapabilities, } -/// A removed cache entry and the runtime categories declared by its previous bundle. +/// A local plugin and the runtime categories affected by its bundle or installed-state change. #[derive(Debug, Clone, PartialEq, Eq)] -pub struct RemotePluginRemoval { +pub struct RemotePluginChange { pub plugin_id: String, pub capabilities: RemotePluginCapabilities, } #[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct RemoteInstalledPluginBundleSyncOutcome { + /// Internal provenance for materialization-owned hook trust, not runtime change reporting. pub materialized_remote_plugins: Vec, - pub removed_plugins: Vec, + /// Affected plugins with capabilities from either side of the change, including removals. + /// Installed-state removals do not depend on cache cleanup succeeding. + pub changed_plugins: Vec, pub failed_remote_plugin_ids: Vec, /// Failures that leave an otherwise valid installed plugin unavailable locally. pub failed_materialization_remote_plugin_ids: Vec, @@ -70,12 +71,6 @@ pub(crate) struct RemoteInstalledPluginBundleSyncResult { pub(crate) installed_plugins: Vec, } -impl RemoteInstalledPluginBundleSyncOutcome { - pub fn changed_local_cache(&self) -> bool { - !self.materialized_remote_plugins.is_empty() || !self.removed_plugins.is_empty() - } -} - #[derive(Debug, thiserror::Error)] pub enum RemoteInstalledPluginBundleSyncError { #[error("{0}")] @@ -123,8 +118,13 @@ pub async fn sync_remote_installed_plugin_bundles_once( config: &RemotePluginServiceConfig, auth: Option<&CodexAuth>, ) -> Result { - let result = - sync_remote_installed_plugin_bundles_once_with_snapshot(codex_home, config, auth).await?; + let result = sync_remote_installed_plugin_bundles_once_with_snapshot( + codex_home, + config, + auth, + /*previous_plugin_ids*/ &[], + ) + .await?; Ok(result.outcome) } @@ -132,6 +132,7 @@ pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot( codex_home: PathBuf, config: &RemotePluginServiceConfig, auth: Option<&CodexAuth>, + previous_plugin_ids: &[PluginId], ) -> Result { let auth = ensure_chatgpt_auth(auth)?; let authenticated_account_id = auth.get_account_id(); @@ -162,6 +163,29 @@ pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot( validated_installed_plugins.push((installed_plugin, cached_plugin, plugin_id)); } let store = PluginStore::try_new(codex_home.clone())?; + let installed_plugin_ids = validated_installed_plugins + .iter() + .map(|(_, _, plugin_id)| plugin_id.as_key()) + .collect::>(); + let mut changed_plugins = BTreeMap::new(); + // Metadata publication removes these plugins even when cleanup fails or partially deletes + // a bundle. Capture cached capabilities first so consumers can still invalidate runtimes; + // plugins that were never available locally have no previous bundle to invalidate. + for plugin_id in previous_plugin_ids { + let key = plugin_id.as_key(); + if installed_plugin_ids.contains(&key) || store.active_plugin_root(plugin_id).is_none() { + continue; + } + let mut capabilities = RemotePluginCapabilities::default(); + capabilities.include_active_bundle(&store, plugin_id).await; + changed_plugins.insert( + key.clone(), + RemotePluginChange { + plugin_id: key, + capabilities, + }, + ); + } let mut installed_plugin_names_by_marketplace = BTreeMap::>::from_iter([ (REMOTE_GLOBAL_MARKETPLACE_NAME.to_string(), BTreeSet::new()), @@ -261,6 +285,13 @@ pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot( Ok(result) => { let plugin_id = result.plugin_id; capabilities.include_active_bundle(&store, &plugin_id).await; + changed_plugins.insert( + plugin_id.as_key(), + RemotePluginChange { + plugin_id: plugin_id.as_key(), + capabilities, + }, + ); materialized_remote_plugins.insert( plugin_id.as_key(), RemotePluginMaterialization { @@ -268,7 +299,6 @@ pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot( scope, discoverability, authenticated_account_id: authenticated_account_id.clone(), - capabilities, }, ); } @@ -292,22 +322,26 @@ pub(crate) async fn sync_remote_installed_plugin_bundles_once_with_snapshot( .cmp(&right.marketplace_name) .then_with(|| left.id.cmp(&right.id)) }); - let mut removed_plugins = Vec::new(); + let mut removed_cache_plugins = Vec::new(); if let Err(err) = remove_stale_remote_plugin_caches( &store, &installed_plugin_names_by_marketplace, - &mut removed_plugins, + &mut removed_cache_plugins, ) .await { warn!(error = %err, "failed to remove stale remote plugin cache entries"); } - removed_plugins.sort_unstable_by(|left, right| left.plugin_id.cmp(&right.plugin_id)); + for plugin in removed_cache_plugins { + changed_plugins + .entry(plugin.plugin_id.clone()) + .or_insert(plugin); + } Ok(RemoteInstalledPluginBundleSyncResult { outcome: RemoteInstalledPluginBundleSyncOutcome { materialized_remote_plugins: materialized_remote_plugins.into_values().collect(), - removed_plugins, + changed_plugins: changed_plugins.into_values().collect(), failed_remote_plugin_ids: failed_remote_plugin_ids.into_iter().collect(), failed_materialization_remote_plugin_ids: failed_materialization_remote_plugin_ids .into_iter() @@ -358,7 +392,7 @@ impl Drop for RemotePluginCacheMutationGuard { async fn remove_stale_remote_plugin_caches( store: &PluginStore, installed_plugin_names_by_marketplace: &BTreeMap>, - removed_plugins: &mut Vec, + removed_plugins: &mut Vec, ) -> Result<(), String> { let codex_home = store.codex_home().as_path(); for marketplace_name in [ @@ -428,7 +462,7 @@ async fn remove_stale_remote_plugin_caches( let plugin_key = plugin_id .map(|plugin_id| plugin_id.as_key()) .unwrap_or_else(|_| format!("{plugin_name}@{marketplace_name}")); - removed_plugins.push(RemotePluginRemoval { + removed_plugins.push(RemotePluginChange { plugin_id: plugin_key, capabilities, }); @@ -707,9 +741,9 @@ mod tests { outcome, RemoteInstalledPluginBundleSyncOutcome { materialized_remote_plugins: Vec::new(), - removed_plugins: removed_cache_plugin_ids + changed_plugins: removed_cache_plugin_ids .into_iter() - .map(|plugin_id| RemotePluginRemoval { + .map(|plugin_id| RemotePluginChange { plugin_id, capabilities: RemotePluginCapabilities::default(), }) @@ -814,7 +848,7 @@ mod tests { ) .await .expect("cleanup while install is guarded"); - assert_eq!(removed, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(cached_manifest.is_file()); drop(guard); @@ -826,7 +860,7 @@ mod tests { ) .await .expect("cleanup while second install guard is still active"); - assert_eq!(removed, Vec::::new()); + assert_eq!(removed, Vec::::new()); assert!(cached_manifest.is_file()); drop(second_guard); @@ -840,7 +874,7 @@ mod tests { .expect("cleanup after install guard is dropped"); assert_eq!( removed, - vec![RemotePluginRemoval { + vec![RemotePluginChange { plugin_id: "linear@openai-curated-remote".to_string(), capabilities: RemotePluginCapabilities::default(), }] @@ -935,7 +969,7 @@ mod tests { "created-by-me-plugin@created-by-me-remote", "private-plugin@workspace-shared-with-me-private", ] - .map(|plugin_id| RemotePluginRemoval { + .map(|plugin_id| RemotePluginChange { plugin_id: plugin_id.to_string(), capabilities: RemotePluginCapabilities::default(), })