diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 9abe4ac7f..ba27209d9 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -33,7 +33,6 @@ use codex_plugin::PluginCapabilitySummary; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_plugin::PluginIdError; -use codex_plugin::PluginLoadOutcome; use codex_plugin::PluginTelemetryMetadata; use codex_plugin::app_connector_ids_from_declarations; use codex_protocol::protocol::Product; @@ -81,12 +80,8 @@ enum NonCuratedCacheRefreshMode { ForceReinstall, } -pub fn log_plugin_load_errors(outcome: &PluginLoadOutcome) { - for plugin in outcome - .plugins() - .iter() - .filter(|plugin| plugin.error.is_some()) - { +pub(crate) fn log_plugin_load_errors(plugins: &[LoadedPlugin]) { + for plugin in plugins.iter().filter(|plugin| plugin.error.is_some()) { if let Some(error) = plugin.error.as_deref() { warn!( plugin = plugin.config_name, @@ -110,14 +105,15 @@ struct PluginAppConfig { category: Option, } +/// Load configured plugins without applying auth-dependent runtime policies. #[instrument(level = "trace", skip_all)] -pub async fn load_plugins_from_layer_stack( +pub(crate) async fn load_plugins_from_layer_stack( config_layer_stack: &ConfigLayerStack, extra_plugins: HashMap, store: &PluginStore, restriction_product: Option, prefer_remote_curated_conflicts: bool, -) -> PluginLoadOutcome { +) -> Vec> { let skill_config_rules = skill_config_rules_from_stack(config_layer_stack); load_plugins_from_layer_stack_with_scope( config_layer_stack, @@ -138,7 +134,7 @@ async fn load_plugins_from_layer_stack_with_scope( store: &PluginStore, prefer_remote_curated_conflicts: bool, scope: PluginLoadScope<'_>, -) -> PluginLoadOutcome { +) -> Vec> { let configured_plugins = merge_configured_plugins_with_remote_installed( configured_plugins_from_stack(config_layer_stack), extra_plugins, @@ -167,7 +163,7 @@ async fn load_plugins_from_layer_stack_with_scope( plugins.push(loaded_plugin); } - PluginLoadOutcome::from_plugins(plugins) + plugins } /// Load hooks from enabled plugins without loading their skills, MCP servers, or apps. @@ -177,7 +173,7 @@ pub async fn load_plugin_hooks_from_layer_stack( store: &PluginStore, prefer_remote_curated_conflicts: bool, ) -> PluginHookLoadOutcome { - let outcome = load_plugins_from_layer_stack_with_scope( + let plugins = load_plugins_from_layer_stack_with_scope( config_layer_stack, extra_plugins, store, @@ -186,8 +182,16 @@ pub async fn load_plugin_hooks_from_layer_stack( ) .await; PluginHookLoadOutcome { - hook_sources: outcome.effective_plugin_hook_sources(), - hook_load_warnings: outcome.effective_plugin_hook_warnings(), + hook_sources: plugins + .iter() + .filter(|plugin| plugin.is_active()) + .flat_map(|plugin| plugin.hook_sources.iter().cloned()) + .collect(), + hook_load_warnings: plugins + .iter() + .filter(|plugin| plugin.is_active()) + .flat_map(|plugin| plugin.hook_load_warnings.iter().cloned()) + .collect(), } } diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index ddf1b66fe..133103b78 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -173,9 +173,8 @@ enabled = true ) .await; - let validation_state = |outcome: &PluginLoadOutcome| { - outcome - .plugins() + let validation_state = |plugins: &[LoadedPlugin]| { + plugins .iter() .map(|plugin| { ( @@ -183,22 +182,15 @@ enabled = true plugin.enabled, plugin.root.clone(), plugin.error.clone(), + plugin.hook_sources.clone(), + plugin.hook_load_warnings.clone(), ) }) .collect::>() }; assert_eq!(validation_state(&hooks_only), validation_state(&full)); - assert_eq!( - hooks_only.effective_plugin_hook_sources(), - full.effective_plugin_hook_sources() - ); - assert_eq!( - hooks_only.effective_plugin_hook_warnings(), - full.effective_plugin_hook_warnings() - ); let full_valid = full - .plugins() .iter() .find(|plugin| plugin.config_name == "valid@test") .expect("full load should include valid plugin"); @@ -208,7 +200,6 @@ enabled = true assert!(!full_valid.apps.is_empty()); let hooks_only_valid = hooks_only - .plugins() .iter() .find(|plugin| plugin.config_name == "valid@test") .expect("hooks-only load should include valid plugin"); diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 82cd1dc63..9ed4d82c5 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -1,3 +1,4 @@ +use super::LoadedPlugin; use super::PluginLoadOutcome; use crate::app_mcp_routing::apply_app_mcp_routing_policy; use crate::installed_marketplaces::installed_marketplace_roots_from_layer_stack; @@ -208,23 +209,6 @@ fn featured_plugin_ids_cache_key( } } -fn project_plugin_load_outcome_for_auth( - outcome: PluginLoadOutcome, - auth_mode: Option, -) -> PluginLoadOutcome { - let mut plugins = outcome.plugins().to_vec(); - for plugin in &mut plugins { - let plugin_active = plugin.is_active(); - apply_app_mcp_routing_policy( - &mut plugin.apps, - &mut plugin.mcp_servers, - auth_mode, - plugin_active, - ); - } - PluginLoadOutcome::from_plugins(plugins) -} - #[derive(Debug, Clone, PartialEq, Eq)] pub struct PluginInstallRequest { pub plugin_name: String, @@ -336,8 +320,9 @@ pub struct PluginsManager { featured_plugin_ids_cache: RwLock>, configured_marketplace_upgrade_state: RwLock, non_curated_cache_refresh_state: RwLock, - enabled_outcome_cache: RwLock, - enabled_outcome_load_semaphore: Semaphore, + // Keep the cache auth-independent so auth changes only need to resolve capabilities again. + loaded_plugins_cache: RwLock, + loaded_plugins_load_semaphore: Semaphore, remote_installed_plugins_cache: RwLock>>, remote_installed_plugins_cache_refresh_state: RwLock, global_remote_catalog_cache_refresh_state: RwLock, @@ -347,15 +332,15 @@ pub struct PluginsManager { } #[derive(Clone)] -struct CachedPluginLoadOutcome { +struct LoadedPluginsCacheEntry { key: PluginLoadCacheKey, - outcome: PluginLoadOutcome, + plugins: Vec, } #[derive(Default)] -struct EnabledOutcomeCache { +struct LoadedPluginsCache { generation: u64, - outcome: Option, + entry: Option, } #[derive(Clone, PartialEq, Eq)] @@ -390,8 +375,8 @@ impl PluginsManager { ConfiguredMarketplaceUpgradeState::default(), ), non_curated_cache_refresh_state: RwLock::new(NonCuratedCacheRefreshState::default()), - enabled_outcome_cache: RwLock::new(EnabledOutcomeCache::default()), - enabled_outcome_load_semaphore: Semaphore::new(/*permits*/ 1), + loaded_plugins_cache: RwLock::new(LoadedPluginsCache::default()), + loaded_plugins_load_semaphore: Semaphore::new(/*permits*/ 1), remote_installed_plugins_cache: RwLock::new(None), remote_installed_plugins_cache_refresh_state: RwLock::new( RemoteInstalledPluginsCacheRefreshState::default(), @@ -466,19 +451,19 @@ impl PluginsManager { skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack), remote_plugin_enabled: config.remote_plugin_enabled, }; - if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&cache_key) { - return self.project_plugins_for_auth(outcome); + if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) { + return self.resolve_loaded_plugins_for_auth(plugins); } - let Ok(_load_permit) = self.enabled_outcome_load_semaphore.acquire().await else { + let Ok(_load_permit) = self.loaded_plugins_load_semaphore.acquire().await else { warn!("plugin load semaphore closed"); return PluginLoadOutcome::default(); }; - if !force_reload && let Some(outcome) = self.cached_enabled_outcome(&cache_key) { - return self.project_plugins_for_auth(outcome); + if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) { + return self.resolve_loaded_plugins_for_auth(plugins); } - let cache_generation = self.enabled_outcome_cache_generation(); - let outcome = load_plugins_from_layer_stack( + let cache_generation = self.loaded_plugins_cache_generation(); + let plugins = load_plugins_from_layer_stack( &config.config_layer_stack, self.remote_installed_plugin_configs(), &self.store, @@ -486,17 +471,27 @@ impl PluginsManager { config.remote_plugin_enabled, ) .await; - log_plugin_load_errors(&outcome); - self.cache_enabled_outcome_if_current(cache_generation, cache_key, outcome.clone()); - self.project_plugins_for_auth(outcome) + log_plugin_load_errors(&plugins); + self.cache_loaded_plugins_if_current(cache_generation, cache_key, plugins.clone()); + self.resolve_loaded_plugins_for_auth(plugins) } - fn project_plugins_for_auth(&self, outcome: PluginLoadOutcome) -> PluginLoadOutcome { - project_plugin_load_outcome_for_auth(outcome, self.auth_mode()) + fn resolve_loaded_plugins_for_auth(&self, mut plugins: Vec) -> PluginLoadOutcome { + let auth_mode = self.auth_mode(); + for plugin in &mut plugins { + let plugin_active = plugin.is_active(); + apply_app_mcp_routing_policy( + &mut plugin.apps, + &mut plugin.mcp_servers, + auth_mode, + plugin_active, + ); + } + PluginLoadOutcome::from_plugins(plugins) } pub fn clear_cache(&self) { - self.clear_enabled_outcome_cache(); + self.clear_loaded_plugins_cache(); let mut featured_plugin_ids_cache = match self.featured_plugin_ids_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), @@ -504,13 +499,13 @@ impl PluginsManager { *featured_plugin_ids_cache = None; } - fn clear_enabled_outcome_cache(&self) { - let mut cache = match self.enabled_outcome_cache.write() { + fn clear_loaded_plugins_cache(&self) { + let mut cache = match self.loaded_plugins_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), }; cache.generation = cache.generation.wrapping_add(1); - cache.outcome = None; + cache.entry = None; } /// Load plugins for a config layer stack without touching the plugins cache. @@ -522,7 +517,7 @@ impl PluginsManager { if !config.plugins_enabled { return PluginLoadOutcome::default(); } - let outcome = load_plugins_from_layer_stack( + let plugins = load_plugins_from_layer_stack( config_layer_stack, self.remote_installed_plugin_configs(), &self.store, @@ -530,7 +525,7 @@ impl PluginsManager { config.remote_plugin_enabled, ) .await; - self.project_plugins_for_auth(outcome) + self.resolve_loaded_plugins_for_auth(plugins) } /// Resolve plugin hooks for a config layer stack without loading other plugin capabilities. @@ -562,41 +557,41 @@ impl PluginsManager { .effective_plugin_skill_roots() } - fn cached_enabled_outcome(&self, key: &PluginLoadCacheKey) -> Option { - match self.enabled_outcome_cache.read() { + fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option> { + match self.loaded_plugins_cache.read() { Ok(cache) => cache - .outcome + .entry .as_ref() .filter(|cached| cached.key == *key) - .map(|cached| cached.outcome.clone()), + .map(|cached| cached.plugins.clone()), Err(err) => err .into_inner() - .outcome + .entry .as_ref() .filter(|cached| cached.key == *key) - .map(|cached| cached.outcome.clone()), + .map(|cached| cached.plugins.clone()), } } - fn enabled_outcome_cache_generation(&self) -> u64 { - match self.enabled_outcome_cache.read() { + fn loaded_plugins_cache_generation(&self) -> u64 { + match self.loaded_plugins_cache.read() { Ok(cache) => cache.generation, Err(err) => err.into_inner().generation, } } - fn cache_enabled_outcome_if_current( + fn cache_loaded_plugins_if_current( &self, generation: u64, key: PluginLoadCacheKey, - outcome: PluginLoadOutcome, + plugins: Vec, ) { - let mut cache = match self.enabled_outcome_cache.write() { + let mut cache = match self.loaded_plugins_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), }; if cache.generation == generation { - cache.outcome = Some(CachedPluginLoadOutcome { key, outcome }); + cache.entry = Some(LoadedPluginsCacheEntry { key, plugins }); } } @@ -687,7 +682,7 @@ impl PluginsManager { } *cache = Some(plugins); drop(cache); - self.clear_enabled_outcome_cache(); + self.clear_loaded_plugins_cache(); true } @@ -701,7 +696,7 @@ impl PluginsManager { } *cache = None; drop(cache); - self.clear_enabled_outcome_cache(); + self.clear_loaded_plugins_cache(); true } diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index d6afbfb89..8ca3e0ac8 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -359,7 +359,7 @@ enabled = true } #[tokio::test] -async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() { +async fn plugin_auth_projection_reprojects_cached_plugins_when_auth_changes() { let codex_home = TempDir::new().unwrap(); write_auth_projection_plugin(codex_home.path(), "sample", /*include_app*/ true); write_auth_projection_plugin(codex_home.path(), "docs", /*include_app*/ false); @@ -379,6 +379,27 @@ async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() { chatgpt_outcome.effective_apps(), vec![AppConnectorId("connector_sample".to_string())] ); + assert_eq!( + chatgpt_outcome.capability_summaries(), + &[ + PluginCapabilitySummary { + config_name: "docs@test".to_string(), + display_name: "docs".to_string(), + description: None, + has_skills: false, + mcp_server_names: vec!["docs".to_string()], + app_connector_ids: Vec::new(), + }, + PluginCapabilitySummary { + config_name: "sample@test".to_string(), + display_name: "sample".to_string(), + description: None, + has_skills: false, + mcp_server_names: Vec::new(), + app_connector_ids: vec![AppConnectorId("connector_sample".to_string())], + }, + ] + ); assert!(manager.set_auth_mode(Some(AuthMode::ApiKey))); let api_key_outcome = manager.plugins_for_config(&config).await; @@ -388,6 +409,27 @@ async fn plugin_auth_projection_reprojects_cached_outcome_when_auth_changes() { vec!["docs".to_string(), "sample".to_string()] ); assert!(api_key_outcome.effective_apps().is_empty()); + assert_eq!( + api_key_outcome.capability_summaries(), + &[ + PluginCapabilitySummary { + config_name: "docs@test".to_string(), + display_name: "docs".to_string(), + description: None, + has_skills: false, + mcp_server_names: vec!["docs".to_string()], + app_connector_ids: Vec::new(), + }, + PluginCapabilitySummary { + config_name: "sample@test".to_string(), + display_name: "sample".to_string(), + description: None, + has_skills: false, + mcp_server_names: vec!["sample".to_string()], + app_connector_ids: Vec::new(), + }, + ] + ); } fn write_plugin_with_version( @@ -1781,7 +1823,7 @@ async fn plugin_cache_ignores_unrelated_session_overrides() { } #[test] -fn plugin_cache_invalidation_rejects_stale_load_completion() { +fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() { let codex_home = TempDir::new().unwrap(); let manager = PluginsManager::new(codex_home.path().to_path_buf()); let cache_key = PluginLoadCacheKey { @@ -1789,16 +1831,12 @@ fn plugin_cache_invalidation_rejects_stale_load_completion() { skill_config_rules: SkillConfigRules::default(), remote_plugin_enabled: false, }; - let stale_generation = manager.enabled_outcome_cache_generation(); + let stale_generation = manager.loaded_plugins_cache_generation(); - manager.clear_enabled_outcome_cache(); - manager.cache_enabled_outcome_if_current( - stale_generation, - cache_key.clone(), - PluginLoadOutcome::default(), - ); + manager.clear_loaded_plugins_cache(); + manager.cache_loaded_plugins_if_current(stale_generation, cache_key.clone(), Vec::new()); - assert_eq!(manager.cached_enabled_outcome(&cache_key), None); + assert_eq!(manager.cached_loaded_plugins(&cache_key), None); } #[tokio::test] @@ -4265,7 +4303,7 @@ async fn load_plugins_ignores_project_config_files() { ) .expect("config layer stack should build"); - let outcome = load_plugins_from_layer_stack( + let plugins = load_plugins_from_layer_stack( &stack, std::collections::HashMap::new(), &PluginStore::new(codex_home.path().to_path_buf()), @@ -4274,7 +4312,7 @@ async fn load_plugins_ignores_project_config_files() { ) .await; - assert_eq!(outcome, PluginLoadOutcome::default()); + assert_eq!(plugins, Vec::new()); } #[tokio::test] diff --git a/codex-rs/plugin/src/load_outcome.rs b/codex-rs/plugin/src/load_outcome.rs index 767771d7c..95b98d863 100644 --- a/codex-rs/plugin/src/load_outcome.rs +++ b/codex-rs/plugin/src/load_outcome.rs @@ -83,7 +83,9 @@ pub fn prompt_safe_plugin_description(description: Option<&str>) -> Option { plugins: Vec>,