From e83b7841b0789d6f8b3252092d44e51a6e0e54ba Mon Sep 17 00:00:00 2001 From: xl-openai Date: Thu, 18 Jun 2026 16:45:58 -0700 Subject: [PATCH] [codex] Reuse parsed plugin skills during session startup (#28844) ## Summary - Preserve raw plugin skill-root snapshots in the matching loaded-plugin cache entry, keyed by the effective plugin root identity including namespace. - Pass those snapshots through `SkillsLoadInput` as an optional preload, so session startup reuses plugin parsing while ordinary skill loads pass `None`. - Keep plugin skill loading cohesive: the existing loaders accept the optional snapshots directly, and uncached or marketplace-detail paths do not create a cache. ## Why Plugin discovery already parses plugin skills to determine available capabilities. Cold session startup then scanned and parsed the same roots again while building the skills snapshot. This solves the same duplicate-work problem as #28623 while keeping ownership narrow: `PluginsManager` creates and owns `PluginSkillSnapshots` only for its loaded-plugin cache entry; `SkillsService` consumes an optional clone. Entry replacement or clearing naturally drops the snapshots, with no separate generation, capacity policy, or watcher coupling. ## Validation - `cargo clippy -p codex-core-skills --all-targets -- -D warnings` - `just test -p codex-core-plugins skills_service_reuses_skills_parsed_during_plugin_load` - `just test -p codex-core-skills namespaces_plugin_skills_using_provided_namespace` - `just fmt` --- codex-rs/app-server/src/skills_watcher.rs | 2 + codex-rs/core-plugins/src/loader.rs | 22 +- codex-rs/core-plugins/src/loader_tests.rs | 1 + codex-rs/core-plugins/src/manager.rs | 74 +++++-- codex-rs/core-plugins/src/manager_tests.rs | 61 +++++- .../core-plugins/src/tool_suggest_metadata.rs | 10 +- codex-rs/core-skills/src/lib.rs | 2 + codex-rs/core-skills/src/loader.rs | 108 ++++------ codex-rs/core-skills/src/loader_tests.rs | 194 ++++++++++-------- codex-rs/core-skills/src/root_loader.rs | 158 ++++++++++++++ codex-rs/core-skills/src/service.rs | 26 ++- codex-rs/core/src/agent/role_tests.rs | 4 +- codex-rs/core/src/session/session.rs | 4 +- codex-rs/core/src/session/tests.rs | 18 +- codex-rs/core/src/session/turn_context.rs | 10 +- codex-rs/ext/skills/src/provider/executor.rs | 19 +- .../tests/executor_file_system_authority.rs | 25 ++- 17 files changed, 527 insertions(+), 211 deletions(-) create mode 100644 codex-rs/core-skills/src/root_loader.rs 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);