diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 086e076d4..637fe963e 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -114,6 +114,8 @@ impl SkillsWatcher { .skill_roots_for_config(&skills_input, Some(environment.get_filesystem())) .await .into_iter() + // Plugin roots are invalidated by plugin lifecycle operations. + .filter(|root| root.plugin_id.is_none()) .map(|root| WatchPath { path: root.path.into_path_buf(), recursive: true, diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 279578d75..54eb37168 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -21,6 +21,7 @@ use codex_config::HooksFile; use codex_config::types::McpServerConfig; use codex_config::types::PluginConfig; use codex_config::types::PluginMcpServerConfig; +use codex_core_skills::PluginSkillSnapshots; use codex_core_skills::SkillMetadata; use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::resolve_disabled_skill_paths; @@ -74,6 +75,7 @@ enum PluginLoadScope<'a> { AllCapabilities { restriction_product: Option, skill_config_rules: &'a SkillConfigRules, + plugin_skill_snapshots: Option<&'a PluginSkillSnapshots>, }, HooksOnly, } @@ -115,6 +117,7 @@ pub(crate) async fn load_plugins_from_layer_stack( config_layer_stack: &ConfigLayerStack, extra_plugins: HashMap, store: &PluginStore, + plugin_skill_snapshots: Option<&PluginSkillSnapshots>, restriction_product: Option, prefer_remote_curated_conflicts: bool, ) -> Vec> { @@ -127,6 +130,7 @@ pub(crate) async fn load_plugins_from_layer_stack( PluginLoadScope::AllCapabilities { restriction_product, skill_config_rules: &skill_config_rules, + plugin_skill_snapshots, }, ) .await @@ -745,6 +749,7 @@ async fn load_plugin( PluginLoadScope::AllCapabilities { restriction_product, skill_config_rules, + plugin_skill_snapshots, } => { loaded_plugin.manifest_name = Some(manifest.display_name().to_string()); loaded_plugin.manifest_description = manifest.description.clone(); @@ -755,6 +760,7 @@ async fn load_plugin( &manifest, *restriction_product, skill_config_rules, + *plugin_skill_snapshots, ) .await; let has_enabled_skills = resolved_skills.has_enabled_skills(); @@ -851,10 +857,17 @@ pub async fn load_plugin_skills( manifest: &PluginManifest, restriction_product: Option, skill_config_rules: &SkillConfigRules, + plugin_skill_snapshots: Option<&PluginSkillSnapshots>, ) -> ResolvedPluginSkills { - load_plugin_skill_inventory(plugin_root, plugin_id, manifest, restriction_product) - .await - .resolve(skill_config_rules) + load_plugin_skill_inventory( + plugin_root, + plugin_id, + manifest, + restriction_product, + plugin_skill_snapshots, + ) + .await + .resolve(skill_config_rules) } pub(crate) async fn load_plugin_skill_inventory( @@ -862,6 +875,7 @@ pub(crate) async fn load_plugin_skill_inventory( plugin_id: &PluginId, manifest: &PluginManifest, restriction_product: Option, + plugin_skill_snapshots: Option<&PluginSkillSnapshots>, ) -> PluginSkillInventory { let roots = plugin_skill_roots(plugin_root, &manifest.paths) .into_iter() @@ -874,7 +888,7 @@ pub(crate) async fn load_plugin_skill_inventory( plugin_root: Some(plugin_root.clone()), }) .collect::>(); - let outcome = load_skills_from_roots(roots).await; + let outcome = load_skills_from_roots(roots, plugin_skill_snapshots).await; let had_errors = !outcome.errors.is_empty(); let skills = outcome .skills diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index 133103b78..ee22256fd 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -160,6 +160,7 @@ enabled = true &stack, HashMap::new(), &store, + /*plugin_skill_snapshots*/ None, Some(Product::Codex), /*prefer_remote_curated_conflicts*/ false, ) diff --git a/codex-rs/core-plugins/src/manager.rs b/codex-rs/core-plugins/src/manager.rs index 713a356a0..9d6c09633 100644 --- a/codex-rs/core-plugins/src/manager.rs +++ b/codex-rs/core-plugins/src/manager.rs @@ -61,6 +61,7 @@ use codex_config::set_user_plugin_enabled; use codex_config::types::PluginConfig; use codex_config::types::ToolSuggestDisabledTool; use codex_config::types::ToolSuggestDiscoverableType; +use codex_core_skills::PluginSkillSnapshots; use codex_core_skills::SkillMetadata; use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::skill_config_rules_from_stack; @@ -370,6 +371,7 @@ pub struct PluginsManager { struct LoadedPluginsCacheEntry { key: PluginLoadCacheKey, plugins: Vec, + plugin_skill_snapshots: PluginSkillSnapshots, } #[derive(Default)] @@ -385,6 +387,16 @@ struct PluginLoadCacheKey { remote_plugin_enabled: bool, } +impl PluginLoadCacheKey { + fn from_config(config: &PluginsConfigInput) -> Self { + Self { + configured_plugins: configured_plugins_from_stack(&config.config_layer_stack), + skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack), + remote_plugin_enabled: config.remote_plugin_enabled, + } + } +} + impl PluginsManager { pub fn new(codex_home: PathBuf) -> Self { Self::new_with_options(codex_home, Some(Product::Codex), /*auth_mode*/ None) @@ -470,6 +482,24 @@ impl PluginsManager { .await } + /// Returns skill snapshots parsed while loading the matching plugin cache entry. + pub fn plugin_skill_snapshots_for_config( + &self, + config: &PluginsConfigInput, + ) -> Option { + if !config.plugins_enabled { + return None; + } + let key = PluginLoadCacheKey::from_config(config); + self.loaded_plugins_cache + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .entry + .as_ref() + .filter(|cached| cached.key == key) + .map(|cached| cached.plugin_skill_snapshots.clone()) + } + #[instrument( name = "plugins_for_config", level = "info", @@ -489,11 +519,7 @@ impl PluginsManager { return PluginLoadOutcome::default(); } - let cache_key = PluginLoadCacheKey { - configured_plugins: configured_plugins_from_stack(&config.config_layer_stack), - skill_config_rules: skill_config_rules_from_stack(&config.config_layer_stack), - remote_plugin_enabled: config.remote_plugin_enabled, - }; + let cache_key = PluginLoadCacheKey::from_config(config); if !force_reload && let Some(plugins) = self.cached_loaded_plugins(&cache_key) { return self.resolve_loaded_plugins_for_auth(plugins); } @@ -506,16 +532,23 @@ impl PluginsManager { return self.resolve_loaded_plugins_for_auth(plugins); } let cache_generation = self.loaded_plugins_cache_generation(); + let plugin_skill_snapshots = PluginSkillSnapshots::for_plugin_load(); let plugins = load_plugins_from_layer_stack( &config.config_layer_stack, self.remote_installed_plugin_configs(), &self.store, + Some(&plugin_skill_snapshots), self.restriction_product, config.remote_plugin_enabled, ) .await; log_plugin_load_errors(&plugins); - self.cache_loaded_plugins_if_current(cache_generation, cache_key, plugins.clone()); + self.cache_loaded_plugins_if_current( + cache_generation, + cache_key, + plugins.clone(), + plugin_skill_snapshots, + ); self.resolve_loaded_plugins_for_auth(plugins) } @@ -589,6 +622,7 @@ impl PluginsManager { config_layer_stack, self.remote_installed_plugin_configs(), &self.store, + /*plugin_skill_snapshots*/ None, self.restriction_product, config.remote_plugin_enabled, ) @@ -626,19 +660,13 @@ impl PluginsManager { } fn cached_loaded_plugins(&self, key: &PluginLoadCacheKey) -> Option> { - match self.loaded_plugins_cache.read() { - Ok(cache) => cache - .entry - .as_ref() - .filter(|cached| cached.key == *key) - .map(|cached| cached.plugins.clone()), - Err(err) => err - .into_inner() - .entry - .as_ref() - .filter(|cached| cached.key == *key) - .map(|cached| cached.plugins.clone()), - } + self.loaded_plugins_cache + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .entry + .as_ref() + .filter(|cached| cached.key == *key) + .map(|cached| cached.plugins.clone()) } fn loaded_plugins_cache_generation(&self) -> u64 { @@ -653,13 +681,18 @@ impl PluginsManager { generation: u64, key: PluginLoadCacheKey, plugins: Vec, + plugin_skill_snapshots: PluginSkillSnapshots, ) { let mut cache = match self.loaded_plugins_cache.write() { Ok(cache) => cache, Err(err) => err.into_inner(), }; if cache.generation == generation { - cache.entry = Some(LoadedPluginsCacheEntry { key, plugins }); + cache.entry = Some(LoadedPluginsCacheEntry { + key, + plugins, + plugin_skill_snapshots, + }); } } @@ -1659,6 +1692,7 @@ impl PluginsManager { &codex_core_skills::config_rules::skill_config_rules_from_stack( &config.config_layer_stack, ), + /*plugin_skill_snapshots*/ None, ) .await; let plugin_data_root = self.store.plugin_data_root(&plugin_id); diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index a4b30a199..cc8ee1913 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -37,6 +37,9 @@ use codex_config::McpServerConfig; use codex_config::McpServerOAuthConfig; use codex_config::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; +use codex_core_skills::PluginSkillSnapshots; +use codex_core_skills::SkillsLoadInput; +use codex_core_skills::SkillsService; use codex_core_skills::config_rules::SkillConfigRules; use codex_login::CodexAuth; use codex_plugin::AppDeclaration; @@ -1476,6 +1479,7 @@ async fn load_plugin_skills_dedupes_overlapping_manifest_roots() { &manifest, /*restriction_product*/ None, &SkillConfigRules::default(), + /*plugin_skill_snapshots*/ None, ) .await; @@ -2039,6 +2043,55 @@ async fn plugin_cache_ignores_unrelated_session_overrides() { assert_eq!(second.plugins()[0].mcp_servers.len(), 1); } +#[tokio::test] +async fn skills_service_reuses_skills_parsed_during_plugin_load() { + let codex_home = TempDir::new().unwrap(); + let codex_home_abs = codex_home.path().to_path_buf().abs(); + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join("test/sample/local"); + write_plugin( + codex_home.path().join("plugins/cache/test").as_path(), + "sample/local", + "sample", + ); + let skill_path = plugin_root.join("skills/SKILL.md"); + write_file(&skill_path, "---\nname: search\ndescription: first\n---\n"); + write_file( + &codex_home.path().join(CONFIG_TOML_FILE), + &plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true), + ); + + let config = load_config(codex_home.path(), codex_home.path()).await; + let manager = PluginsManager::new(codex_home.path().to_path_buf()); + let plugin_outcome = manager.plugins_for_config(&config).await; + let plugin_skill_snapshots = manager.plugin_skill_snapshots_for_config(&config); + write_file(&skill_path, "---\nname: search\ndescription: second\n---\n"); + + let skills_input = SkillsLoadInput::new( + codex_home_abs.clone(), + plugin_outcome.effective_plugin_skill_roots(), + config.config_layer_stack.clone(), + /*bundled_skills_enabled*/ false, + ) + .with_plugin_skill_snapshots(plugin_skill_snapshots); + let skills_service = SkillsService::new(codex_home_abs, /*bundled_skills_enabled*/ false); + let cached = skills_service + .snapshot_for_config(&skills_input, /*fs*/ None) + .await; + + assert_eq!( + cached + .outcome() + .skills + .iter() + .map(|skill| skill.description.as_str()) + .collect::>(), + vec!["first"] + ); +} + #[test] fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() { let codex_home = TempDir::new().unwrap(); @@ -2051,7 +2104,12 @@ fn loaded_plugins_cache_invalidation_rejects_stale_load_completion() { let stale_generation = manager.loaded_plugins_cache_generation(); manager.clear_loaded_plugins_cache(); - manager.cache_loaded_plugins_if_current(stale_generation, cache_key.clone(), Vec::new()); + manager.cache_loaded_plugins_if_current( + stale_generation, + cache_key.clone(), + Vec::new(), + PluginSkillSnapshots::for_plugin_load(), + ); assert_eq!(manager.cached_loaded_plugins(&cache_key), None); } @@ -5164,6 +5222,7 @@ async fn load_plugins_ignores_project_config_files() { &stack, std::collections::HashMap::new(), &PluginStore::new(codex_home.path().to_path_buf()), + /*plugin_skill_snapshots*/ None, Some(Product::Codex), /*prefer_remote_curated_conflicts*/ false, ) diff --git a/codex-rs/core-plugins/src/tool_suggest_metadata.rs b/codex-rs/core-plugins/src/tool_suggest_metadata.rs index 2a667405e..310bec865 100644 --- a/codex-rs/core-plugins/src/tool_suggest_metadata.rs +++ b/codex-rs/core-plugins/src/tool_suggest_metadata.rs @@ -210,8 +210,14 @@ async fn load_plugin_metadata( } let manifest = load_plugin_manifest(plugin_root.as_path()) .ok_or_else(|| "missing or invalid plugin.json".to_string())?; - let skill_inventory = - load_plugin_skill_inventory(plugin_root, &plugin_id, &manifest, restriction_product).await; + let skill_inventory = load_plugin_skill_inventory( + plugin_root, + &plugin_id, + &manifest, + restriction_product, + /*plugin_skill_snapshots*/ None, + ) + .await; let mut mcp_server_names = load_plugin_mcp_servers(plugin_root.as_path(), /*auth_mode*/ None) .await diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 31e5a7c53..53ffcddd0 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -6,6 +6,7 @@ mod mention_counts; pub mod model; pub mod remote; pub mod render; +mod root_loader; pub mod service; mod skill_instructions; pub mod system; @@ -29,6 +30,7 @@ pub use render::SkillRenderReport; pub use render::build_available_skills; pub use render::default_skill_metadata_budget; pub use render::render_available_skills_body; +pub use root_loader::PluginSkillSnapshots; pub use service::SkillsLoadInput; pub use service::SkillsService; pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 64861c45e..2a3a3189b 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,6 +1,5 @@ use crate::model::SkillDependencies; use crate::model::SkillError; -use crate::model::SkillFileSystemsByPath; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; @@ -24,7 +23,6 @@ use codex_utils_plugins::PluginSkillRoot; use codex_utils_plugins::plugin_namespace_for_skill_path; use dirs::home_dir; use serde::Deserialize; -use std::collections::HashMap; use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; @@ -161,77 +159,51 @@ pub struct SkillRoot { pub plugin_root: Option, } -pub async fn load_skills_from_roots(roots: I) -> SkillLoadOutcome +pub async fn load_skills_from_roots( + roots: I, + plugin_skill_snapshots: Option<&crate::PluginSkillSnapshots>, +) -> SkillLoadOutcome where I: IntoIterator, { + crate::root_loader::load_and_merge_skill_roots(roots, plugin_skill_snapshots).await +} + +#[derive(Clone)] +pub(crate) struct SkillRootSnapshot { + pub(crate) root: AbsolutePathBuf, + pub(crate) skills: Vec, + pub(crate) errors: Vec, + pub(crate) file_system: Arc, +} + +pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { + let SkillRoot { + path, + scope, + file_system, + plugin_id, + plugin_namespace, + plugin_root, + } = root; + let root = canonicalize_for_skill_identity(file_system.as_ref(), &path).await; let mut outcome = SkillLoadOutcome::default(); - let mut skill_roots: Vec = Vec::new(); - let mut skill_root_by_path: HashMap = HashMap::new(); - let mut file_systems_by_skill_path: HashMap> = - HashMap::new(); - for root in roots { - let fs = root.file_system; - let root_path = canonicalize_for_skill_identity(fs.as_ref(), &root.path).await; - let skills_before_root = outcome.skills.len(); - discover_skills_under_root( - fs.as_ref(), - &root_path, - root.scope, - root.plugin_id.as_deref(), - root.plugin_namespace.as_deref(), - root.plugin_root.as_ref(), - &mut outcome, - ) - .await; - for skill in &outcome.skills[skills_before_root..] { - if !skill_roots.contains(&root_path) { - skill_roots.push(root_path.clone()); - } - skill_root_by_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| root_path.clone()); - file_systems_by_skill_path - .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| Arc::clone(&fs)); - } + discover_skills_under_root( + file_system.as_ref(), + &root, + scope, + plugin_id.as_deref(), + plugin_namespace.as_deref(), + plugin_root.as_ref(), + &mut outcome, + ) + .await; + SkillRootSnapshot { + root, + skills: outcome.skills, + errors: outcome.errors, + file_system, } - - let mut seen: HashSet = HashSet::new(); - outcome - .skills - .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); - let retained_skill_paths: HashSet = outcome - .skills - .iter() - .map(|skill| skill.path_to_skills_md.clone()) - .collect(); - skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); - let used_roots: HashSet = skill_root_by_path.values().cloned().collect(); - skill_roots.retain(|root| used_roots.contains(root)); - file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); - outcome.skill_roots = skill_roots; - outcome.skill_root_by_path = Arc::new(skill_root_by_path); - outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); - - fn scope_rank(scope: SkillScope) -> u8 { - // Higher-priority scopes first (matches root scan order for dedupe). - match scope { - SkillScope::Repo => 0, - SkillScope::User => 1, - SkillScope::System => 2, - SkillScope::Admin => 3, - } - } - - outcome.skills.sort_by(|a, b| { - scope_rank(a.scope) - .cmp(&scope_rank(b.scope)) - .then_with(|| a.name.cmp(&b.name)) - .then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md)) - }); - - outcome } pub(crate) async fn skill_roots( diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 3a6ae9b1c..e22ed33f3 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -128,6 +128,7 @@ async fn load_skills_for_test(config: &TestConfig) -> SkillLoadOutcome { /*home_dir*/ None, ) .await, + /*plugin_skill_snapshots*/ None, ) .await } @@ -315,7 +316,7 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<() Some(&home_folder_abs), ) .await; - let outcome = load_skills_from_roots(roots).await; + let outcome = load_skills_from_roots(roots, /*plugin_skill_snapshots*/ None).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -845,14 +846,17 @@ interface: ); let plugin_root_abs = plugin_root.abs(); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_namespace: None, - plugin_root: Some(plugin_root_abs.clone()), - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("twilio-developer-kit@test".to_string()), + plugin_namespace: None, + plugin_root: Some(plugin_root_abs.clone()), + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -903,14 +907,17 @@ interface: "##, ); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some("twilio-developer-kit@test".to_string()), - plugin_namespace: None, - plugin_root: Some(plugin_root.abs()), - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("twilio-developer-kit@test".to_string()), + plugin_namespace: None, + plugin_root: Some(plugin_root.abs()), + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -1050,14 +1057,17 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { fs::create_dir_all(admin_root.path()).unwrap(); symlink_dir(shared.path(), &admin_root.path().join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: admin_root.path().abs(), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: admin_root.path().abs(), + scope: SkillScope::Admin, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -1133,14 +1143,17 @@ async fn system_scope_ignores_symlinked_subdir() { fs::create_dir_all(&system_root).unwrap(); symlink_dir(shared.path(), &system_root.join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: system_root.abs(), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: system_root.abs(), + scope: SkillScope::System, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( outcome.errors.is_empty(), @@ -1168,14 +1181,17 @@ async fn respects_max_scan_depth_for_user_scope() { ); let skills_root = codex_home.path().join("skills"); - let outcome = load_skills_from_roots([SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: skills_root.abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -1276,14 +1292,17 @@ async fn namespaces_plugin_skills_using_provided_namespace() { ) .unwrap(); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some("sample@test".to_string()), - plugin_namespace: Some("sample".to_string()), - plugin_root: Some(plugin_root.abs()), - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("sample@test".to_string()), + plugin_namespace: Some("sample".to_string()), + plugin_root: Some(plugin_root.abs()), + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -1322,14 +1341,17 @@ async fn plugin_skill_name_length_limit_allows_max_qualified_name() { ) .unwrap(); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some("sample@test".to_string()), - plugin_namespace: Some(plugin_name.clone()), - plugin_root: Some(plugin_root.abs()), - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("sample@test".to_string()), + plugin_namespace: Some(plugin_name.clone()), + plugin_root: Some(plugin_root.abs()), + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( @@ -1368,14 +1390,17 @@ async fn plugin_skill_name_length_limit_rejects_overlong_qualified_name() { ) .unwrap(); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: Some("sample@test".to_string()), - plugin_namespace: Some(plugin_name.clone()), - plugin_root: Some(plugin_root.abs()), - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: plugin_root.join("skills").abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: Some("sample@test".to_string()), + plugin_namespace: Some(plugin_name.clone()), + plugin_root: Some(plugin_root.abs()), + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert_eq!(outcome.skills, Vec::new()); @@ -1807,24 +1832,27 @@ async fn deduplicates_by_path_preferring_first_root() { let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); - let outcome = load_skills_from_roots([ - SkillRoot { - path: root.path().abs(), - scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }, - SkillRoot { - path: root.path().abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }, - ]) + let outcome = load_skills_from_roots( + [ + SkillRoot { + path: root.path().abs(), + scope: SkillScope::Repo, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }, + SkillRoot { + path: root.path().abs(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }, + ], + /*plugin_skill_snapshots*/ None, + ) .await; assert!( diff --git a/codex-rs/core-skills/src/root_loader.rs b/codex-rs/core-skills/src/root_loader.rs new file mode 100644 index 000000000..a31c8a275 --- /dev/null +++ b/codex-rs/core-skills/src/root_loader.rs @@ -0,0 +1,158 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::fmt; +use std::sync::Arc; +use std::sync::Mutex; + +use codex_utils_plugins::PluginSkillRoot; + +use crate::SkillLoadOutcome; +use crate::loader::SkillRoot; +use crate::loader::SkillRootSnapshot; +use crate::loader::load_skill_root; +use crate::model::SkillFileSystemsByPath; + +/// Parsed plugin skill-root snapshots produced by one plugin load. +/// +/// Clones share the same snapshots. The plugins manager stores them with the corresponding loaded +/// plugins and passes a clone to skill loading as an optional preload. +#[derive(Clone)] +pub struct PluginSkillSnapshots { + snapshots_by_root: Arc>>, +} + +impl PluginSkillSnapshots { + /// Creates an empty snapshot collection for a plugin load to populate. + pub fn for_plugin_load() -> Self { + Self { + snapshots_by_root: Arc::new(Mutex::new(HashMap::new())), + } + } +} + +impl fmt::Debug for PluginSkillSnapshots { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("PluginSkillSnapshots") + .finish_non_exhaustive() + } +} + +pub(crate) async fn load_and_merge_skill_roots( + roots: I, + plugin_skill_snapshots: Option<&PluginSkillSnapshots>, +) -> SkillLoadOutcome +where + I: IntoIterator, +{ + let mut root_snapshots = Vec::new(); + for root in roots { + let cache_key = match ( + root.plugin_id.clone(), + root.plugin_namespace.clone(), + root.plugin_root.clone(), + ) { + (Some(plugin_id), Some(plugin_namespace), Some(plugin_root)) => Some(PluginSkillRoot { + path: root.path.clone(), + plugin_id, + plugin_namespace, + plugin_root, + }), + _ => None, + }; + let cached_snapshot = cache_key.as_ref().and_then(|cache_key| { + let plugin_skill_snapshots = plugin_skill_snapshots?; + plugin_skill_snapshots + .snapshots_by_root + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .get(cache_key) + .cloned() + }); + let snapshot = match cached_snapshot { + Some(snapshot) => snapshot, + None => { + let snapshot = load_skill_root(root).await; + if let Some(plugin_skill_snapshots) = plugin_skill_snapshots + && let Some(cache_key) = cache_key + { + plugin_skill_snapshots + .snapshots_by_root + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(cache_key, snapshot.clone()); + } + snapshot + } + }; + root_snapshots.push(snapshot); + } + + merge_skill_root_snapshots(root_snapshots) +} + +fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOutcome { + fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 { + use codex_protocol::protocol::SkillScope; + + // Higher-priority scopes first (matches root scan order for dedupe). + match scope { + SkillScope::Repo => 0, + SkillScope::User => 1, + SkillScope::System => 2, + SkillScope::Admin => 3, + } + } + + let mut outcome = SkillLoadOutcome::default(); + let mut skill_roots = Vec::new(); + let mut skill_root_by_path = HashMap::new(); + let mut file_systems_by_skill_path = HashMap::new(); + + for snapshot in snapshots { + let SkillRootSnapshot { + root, + skills, + errors, + file_system, + } = snapshot; + if !skills.is_empty() && !skill_roots.contains(&root) { + skill_roots.push(root.clone()); + } + for skill in &skills { + skill_root_by_path + .entry(skill.path_to_skills_md.clone()) + .or_insert_with(|| root.clone()); + file_systems_by_skill_path + .entry(skill.path_to_skills_md.clone()) + .or_insert_with(|| Arc::clone(&file_system)); + } + outcome.skills.extend(skills); + outcome.errors.extend(errors); + } + + let mut seen = HashSet::new(); + outcome + .skills + .retain(|skill| seen.insert(skill.path_to_skills_md.clone())); + let retained_skill_paths = outcome + .skills + .iter() + .map(|skill| skill.path_to_skills_md.clone()) + .collect::>(); + skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); + let used_roots = skill_root_by_path.values().cloned().collect::>(); + skill_roots.retain(|root| used_roots.contains(root)); + file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); + outcome.skill_roots = skill_roots; + outcome.skill_root_by_path = Arc::new(skill_root_by_path); + outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); + + outcome.skills.sort_by(|a, b| { + scope_rank(a.scope) + .cmp(&scope_rank(b.scope)) + .then_with(|| a.name.cmp(&b.name)) + .then_with(|| a.path_to_skills_md.cmp(&b.path_to_skills_md)) + }); + + outcome +} diff --git a/codex-rs/core-skills/src/service.rs b/codex-rs/core-skills/src/service.rs index a6a1e30f5..1c956d79b 100644 --- a/codex-rs/core-skills/src/service.rs +++ b/codex-rs/core-skills/src/service.rs @@ -14,6 +14,7 @@ use tracing::instrument; use tracing::warn; use crate::HostSkillsSnapshot; +use crate::PluginSkillSnapshots; use crate::SkillLoadOutcome; use crate::build_implicit_skill_path_indexes; use crate::config_rules::SkillConfigRules; @@ -32,6 +33,7 @@ pub struct SkillsLoadInput { pub effective_skill_roots: Vec, pub config_layer_stack: ConfigLayerStack, pub bundled_skills_enabled: bool, + plugin_skill_snapshots: Option, } impl SkillsLoadInput { @@ -46,8 +48,18 @@ impl SkillsLoadInput { effective_skill_roots, config_layer_stack, bundled_skills_enabled, + plugin_skill_snapshots: None, } } + + /// Attaches plugin skill snapshots parsed during plugin loading, when available. + pub fn with_plugin_skill_snapshots( + mut self, + plugin_skill_snapshots: Option, + ) -> Self { + self.plugin_skill_snapshots = plugin_skill_snapshots; + self + } } /// Owns host skill discovery, immutable snapshots, cache invalidation, and extra roots. @@ -124,7 +136,8 @@ impl SkillsService { } let snapshot = HostSkillsSnapshot::new(Arc::new( - self.build_skill_outcome(roots, &skill_config_rules).await, + self.build_skill_outcome(input, roots, &skill_config_rules) + .await, )); let mut cache = self .cache_by_config @@ -180,7 +193,8 @@ impl SkillsService { } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let snapshot = HostSkillsSnapshot::new(Arc::new( - self.build_skill_outcome(roots, &skill_config_rules).await, + self.build_skill_outcome(input, roots, &skill_config_rules) + .await, )); if use_cwd_cache { let mut cache = self @@ -195,13 +209,13 @@ impl SkillsService { #[instrument(level = "trace", skip_all)] async fn build_skill_outcome( &self, + input: &SkillsLoadInput, roots: Vec, skill_config_rules: &SkillConfigRules, ) -> SkillLoadOutcome { - let outcome = crate::filter_skill_load_outcome_for_product( - load_skills_from_roots(roots).await, - self.restriction_product, - ); + let outcome = load_skills_from_roots(roots, input.plugin_skill_snapshots.as_ref()).await; + let outcome = + crate::filter_skill_load_outcome_for_product(outcome, self.restriction_product); let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); finalize_skill_outcome(outcome, disabled_paths) } diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index e258c0ad9..b56531c90 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -421,7 +421,9 @@ enabled = false let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(&config, effective_skill_roots); + let plugin_skill_snapshots = plugins_manager.plugin_skill_snapshots_for_config(&plugins_input); + let skills_input = skills_load_input_from_config(&config, effective_skill_roots) + .with_plugin_skill_snapshots(plugin_skill_snapshots); let snapshot = skills_service .snapshot_for_config( &skills_input, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index af2d1abeb..b7ceb165b 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -445,7 +445,9 @@ async fn warm_plugins_and_skills_for_session_init( let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(config.as_ref(), effective_skill_roots); + let plugin_skill_snapshots = plugins_manager.plugin_skill_snapshots_for_config(&plugins_input); + let skills_input = skills_load_input_from_config(config.as_ref(), effective_skill_roots) + .with_plugin_skill_snapshots(plugin_skill_snapshots); skills_service .snapshot_for_config(&skills_input, fs) .await diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 0b1cc045c..93faae548 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5050,13 +5050,18 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { turn_environments: Arc::clone(&turn_environments), }; + let plugins_input = per_turn_config.plugins_config_input(); let plugin_outcome = services .plugins_manager - .plugins_for_config(&per_turn_config.plugins_config_input()) + .plugins_for_config(&plugins_input) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); + let plugin_skill_snapshots = services + .plugins_manager + .plugin_skill_snapshots_for_config(&plugins_input); let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); + crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots) + .with_plugin_skill_snapshots(plugin_skill_snapshots); let skill_fs = environment.get_filesystem(); let skills_snapshot = services .skills_service @@ -7099,13 +7104,18 @@ where turn_environments: Arc::clone(&turn_environments), }; + let plugins_input = per_turn_config.plugins_config_input(); let plugin_outcome = services .plugins_manager - .plugins_for_config(&per_turn_config.plugins_config_input()) + .plugins_for_config(&plugins_input) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); + let plugin_skill_snapshots = services + .plugins_manager + .plugin_skill_snapshots_for_config(&plugins_input); let skills_input = - crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); + crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots) + .with_plugin_skill_snapshots(plugin_skill_snapshots); let skill_fs = environment.get_filesystem(); let skills_snapshot = services .skills_service diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 91880bdef..c03bdab7c 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -722,13 +722,19 @@ impl Session { .or(model_info.multi_agent_version) .unwrap_or_else(|| per_turn_config.multi_agent_version_from_features()), }; + let plugins_input = per_turn_config.plugins_config_input(); let plugin_outcome = self .services .plugins_manager - .plugins_for_config(&per_turn_config.plugins_config_input()) + .plugins_for_config(&plugins_input) .await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); - let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots); + let plugin_skill_snapshots = self + .services + .plugins_manager + .plugin_skill_snapshots_for_config(&plugins_input); + let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots) + .with_plugin_skill_snapshots(plugin_skill_snapshots); let fs = primary_turn_environment .map(|turn_environment| turn_environment.environment.get_filesystem()); let skills_snapshot = self diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index 8da0265d0..75234dfbe 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -76,14 +76,17 @@ impl SkillProvider for ExecutorSkillProvider { }; let file_system = environment.get_filesystem(); let outcome = filter_skill_load_outcome_for_product( - load_skills_from_roots([SkillRoot { - path: root_path.clone(), - scope: SkillScope::User, - file_system: Arc::clone(&file_system), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }]) + load_skills_from_roots( + [SkillRoot { + path: root_path.clone(), + scope: SkillScope::User, + file_system: Arc::clone(&file_system), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + ) .await, self.restriction_product, ); diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index bc2a87a74..e4b09e96a 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -188,17 +188,20 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { assert!(!alias_root.as_path().exists()); assert!(!canonical_root.as_path().exists()); - let outcome = load_skills_from_roots([SkillRoot { - path: alias_root.clone(), - scope: SkillScope::User, - file_system: Arc::new(SyntheticFileSystem { - alias_root, - canonical_root: canonical_root.clone(), - }), - plugin_id: None, - plugin_namespace: None, - plugin_root: None, - }]) + let outcome = load_skills_from_roots( + [SkillRoot { + path: alias_root.clone(), + scope: SkillScope::User, + file_system: Arc::new(SyntheticFileSystem { + alias_root, + canonical_root: canonical_root.clone(), + }), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + ) .await; assert_eq!(outcome.errors, Vec::new()); assert_eq!(outcome.skills.len(), 1);