From 0318381762e336686b281d8fd9e4c11769a0dfc0 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 17 Jun 2026 16:01:06 +0100 Subject: [PATCH] Replace SkillsManager with SkillsService (#28705) ## Why Host skill discovery was still exposed as a manager even though it is a process-owned service shared by sessions, the app-server catalog, and file-watcher invalidation. The skills extension also consumed an ad hoc loaded-skills wrapper instead of a named immutable snapshot. ## What changed - replace `SkillsManager` with concrete `SkillsService` - make the service cache and return immutable `HostSkillsSnapshot` values - migrate the skills extension host provider to the snapshot boundary - migrate app-server catalog, watcher, and invalidation paths to the service This keeps the service limited to host discovery, caching, roots, and invalidation. Catalog rendering and invocation remain extension responsibilities for the next stacked change. --- codex-rs/app-server/src/message_processor.rs | 2 +- .../request_processors/account_processor.rs | 2 +- .../request_processors/catalog_processor.rs | 13 +-- .../request_processors/config_processor.rs | 2 +- .../external_agent_config_processor.rs | 2 +- .../src/request_processors/plugins.rs | 4 +- codex-rs/app-server/src/skills_watcher.rs | 12 +-- codex-rs/core-api/src/lib.rs | 2 +- codex-rs/core-skills/src/lib.rs | 8 +- codex-rs/core-skills/src/model.rs | 8 +- .../src/{manager.rs => service.rs} | 60 ++++++----- .../{manager_tests.rs => service_tests.rs} | 99 ++++++++++--------- codex-rs/core/src/agent/role_tests.rs | 11 ++- codex-rs/core/src/codex_delegate.rs | 2 +- codex-rs/core/src/config/mod.rs | 2 +- codex-rs/core/src/guardian/tests.rs | 2 +- codex-rs/core/src/lib.rs | 4 +- codex-rs/core/src/session/mod.rs | 16 +-- codex-rs/core/src/session/review.rs | 7 +- codex-rs/core/src/session/session.rs | 14 +-- codex-rs/core/src/session/tests.rs | 65 ++++++------ .../core/src/session/tests/guardian_tests.rs | 4 +- codex-rs/core/src/session/turn.rs | 2 +- codex-rs/core/src/session/turn_context.rs | 28 +++--- codex-rs/core/src/skills.rs | 6 +- codex-rs/core/src/state/service.rs | 4 +- codex-rs/core/src/thread_manager.rs | 18 ++-- codex-rs/ext/skills/src/extension.rs | 23 ++--- codex-rs/ext/skills/src/provider.rs | 6 +- codex-rs/ext/skills/src/provider/host.rs | 35 +++---- codex-rs/ext/skills/src/tools/mod.rs | 2 +- codex-rs/ext/skills/src/tools/read.rs | 2 +- .../tests/executor_file_system_authority.rs | 6 +- codex-rs/ext/skills/tests/skills_extension.rs | 6 +- 34 files changed, 244 insertions(+), 235 deletions(-) rename codex-rs/core-skills/src/{manager.rs => service.rs} (85%) rename codex-rs/core-skills/src/{manager_tests.rs => service_tests.rs} (91%) diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 3c3600008..985160985 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -379,7 +379,7 @@ impl MessageProcessor { thread_manager .plugins_manager() .set_analytics_events_client(analytics_events_client.clone()); - let skills_watcher = SkillsWatcher::new(thread_manager.skills_manager(), outgoing.clone()); + let skills_watcher = SkillsWatcher::new(thread_manager.skills_service(), outgoing.clone()); let pending_thread_unloads = Arc::new(Mutex::new(HashSet::new())); let thread_watch_manager = diff --git a/codex-rs/app-server/src/request_processors/account_processor.rs b/codex-rs/app-server/src/request_processors/account_processor.rs index 1fa0961dd..5d234f700 100644 --- a/codex-rs/app-server/src/request_processors/account_processor.rs +++ b/codex-rs/app-server/src/request_processors/account_processor.rs @@ -219,7 +219,7 @@ impl AccountRequestProcessor { ) { tokio::spawn(async move { thread_manager.plugins_manager().clear_cache(); - thread_manager.skills_manager().clear_cache(); + thread_manager.skills_service().clear_cache(); if thread_manager.list_thread_ids().await.is_empty() { return; } diff --git a/codex-rs/app-server/src/request_processors/catalog_processor.rs b/codex-rs/app-server/src/request_processors/catalog_processor.rs index 9c0cc1c8b..f61d0ed4b 100644 --- a/codex-rs/app-server/src/request_processors/catalog_processor.rs +++ b/codex-rs/app-server/src/request_processors/catalog_processor.rs @@ -511,7 +511,7 @@ impl CatalogRequestProcessor { let workspace_codex_plugins_enabled = self .workspace_codex_plugins_enabled(&config, auth.as_ref()) .await; - let skills_manager = self.thread_manager.skills_manager(); + let skills_service = self.thread_manager.skills_service(); let plugins_manager = self.thread_manager.plugins_manager(); let fs = self .thread_manager @@ -523,7 +523,7 @@ impl CatalogRequestProcessor { let config = &config; let fs = fs.clone(); let plugins_manager = &plugins_manager; - let skills_manager = &skills_manager; + let skills_service = &skills_service; async move { let (cwd_abs, config_layer_stack) = match self.resolve_cwd_config(&cwd).await { Ok(resolved) => resolved, @@ -559,9 +559,10 @@ impl CatalogRequestProcessor { config_layer_stack, config.bundled_skills_enabled(), ); - let outcome = skills_manager - .skills_for_cwd(&skills_input, force_reload, fs) + let snapshot = skills_service + .snapshot_for_cwd(&skills_input, force_reload, fs) .await; + let outcome = snapshot.outcome(); let errors = errors_to_info(&outcome.errors); let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths); ( @@ -590,7 +591,7 @@ impl CatalogRequestProcessor { self.skills_watcher .register_runtime_extra_roots(&extra_roots); self.thread_manager - .skills_manager() + .skills_service() .set_extra_roots(extra_roots); self.outgoing .send_server_notification(ServerNotification::SkillsChanged( @@ -703,7 +704,7 @@ impl CatalogRequestProcessor { .await .map(|()| { self.thread_manager.plugins_manager().clear_cache(); - self.thread_manager.skills_manager().clear_cache(); + self.thread_manager.skills_service().clear_cache(); SkillsConfigWriteResponse { effective_enabled: enabled, } diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 837018107..15c3b7c86 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -169,7 +169,7 @@ impl ConfigRequestProcessor { pub(crate) async fn handle_config_mutation(&self) { self.thread_manager.plugins_manager().clear_cache(); - self.thread_manager.skills_manager().clear_cache(); + self.thread_manager.skills_service().clear_cache(); } async fn handle_config_mutation_result( diff --git a/codex-rs/app-server/src/request_processors/external_agent_config_processor.rs b/codex-rs/app-server/src/request_processors/external_agent_config_processor.rs index 1a9002dd1..b3cdb0e21 100644 --- a/codex-rs/app-server/src/request_processors/external_agent_config_processor.rs +++ b/codex-rs/app-server/src/request_processors/external_agent_config_processor.rs @@ -319,7 +319,7 @@ impl ExternalAgentConfigRequestProcessor { completed_item_results.extend(background_item_results); if has_plugin_imports { thread_manager.plugins_manager().clear_cache(); - thread_manager.skills_manager().clear_cache(); + thread_manager.skills_service().clear_cache(); } send_completed_import_notification( &outgoing, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 609d89459..d2cd08b63 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -474,7 +474,7 @@ impl PluginRequestProcessor { ) { tokio::spawn(async move { thread_manager.plugins_manager().clear_cache(); - thread_manager.skills_manager().clear_cache(); + thread_manager.skills_service().clear_cache(); if thread_manager.list_thread_ids().await.is_empty() { return; } @@ -484,7 +484,7 @@ impl PluginRequestProcessor { fn clear_plugin_related_caches(&self) { self.thread_manager.plugins_manager().clear_cache(); - self.thread_manager.skills_manager().clear_cache(); + self.thread_manager.skills_service().clear_cache(); } async fn load_latest_config( diff --git a/codex-rs/app-server/src/skills_watcher.rs b/codex-rs/app-server/src/skills_watcher.rs index 57edb89c7..086e076d4 100644 --- a/codex-rs/app-server/src/skills_watcher.rs +++ b/codex-rs/app-server/src/skills_watcher.rs @@ -8,7 +8,7 @@ use codex_app_server_protocol::SkillsChangedNotification; use codex_core::ThreadManager; use codex_core::config::Config; use codex_core::skills::SkillsLoadInput; -use codex_core::skills::SkillsManager; +use codex_core::skills::SkillsService; use codex_file_watcher::FileWatcher; use codex_file_watcher::FileWatcherSubscriber; use codex_file_watcher::Receiver; @@ -35,7 +35,7 @@ pub(crate) struct SkillsWatcher { impl SkillsWatcher { pub(crate) fn new( - skills_manager: Arc, + skills_service: Arc, outgoing: Arc, ) -> Arc { let file_watcher = match FileWatcher::new() { @@ -48,7 +48,7 @@ impl SkillsWatcher { let (subscriber, rx) = file_watcher.add_subscriber(); let shutdown_token = CancellationToken::new(); let shutdown_drop_guard = shutdown_token.clone().drop_guard(); - Self::spawn_event_loop(rx, skills_manager, outgoing, shutdown_token.child_token()); + Self::spawn_event_loop(rx, skills_service, outgoing, shutdown_token.child_token()); Arc::new(Self { subscriber, runtime_extra_roots_registration: Mutex::new(WatchRegistration::default()), @@ -110,7 +110,7 @@ impl SkillsWatcher { config.bundled_skills_enabled(), ); let roots = thread_manager - .skills_manager() + .skills_service() .skill_roots_for_config(&skills_input, Some(environment.get_filesystem())) .await .into_iter() @@ -124,7 +124,7 @@ impl SkillsWatcher { fn spawn_event_loop( rx: Receiver, - skills_manager: Arc, + skills_service: Arc, outgoing: Arc, shutdown_token: CancellationToken, ) { @@ -142,7 +142,7 @@ impl SkillsWatcher { if event.is_none() { break; } - skills_manager.clear_cache(); + skills_service.clear_cache(); outgoing .send_server_notification(ServerNotification::SkillsChanged( SkillsChangedNotification {}, diff --git a/codex-rs/core-api/src/lib.rs b/codex-rs/core-api/src/lib.rs index df4e6ea8a..aac0e2168 100644 --- a/codex-rs/core-api/src/lib.rs +++ b/codex-rs/core-api/src/lib.rs @@ -45,7 +45,7 @@ pub use codex_core::config::ThreadStoreConfig; pub use codex_core::config::find_codex_home; pub use codex_core::init_state_db; pub use codex_core::resolve_installation_id; -pub use codex_core::skills::SkillsManager; +pub use codex_core::skills::SkillsService; pub use codex_core::thread_store_from_config; pub use codex_exec_server::EnvironmentManager; pub use codex_exec_server::ExecServerError; diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 0390302af..31e5a7c53 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -2,20 +2,18 @@ pub mod config_rules; pub mod injection; pub(crate) mod invocation_utils; pub mod loader; -pub mod manager; mod mention_counts; pub mod model; pub mod remote; pub mod render; +pub mod service; mod skill_instructions; pub mod system; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; pub use invocation_utils::detect_implicit_skill_invocation_for_command; -pub use manager::SkillsLoadInput; -pub use manager::SkillsManager; pub use mention_counts::build_skill_name_counts; -pub use model::HostLoadedSkills; +pub use model::HostSkillsSnapshot; pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; @@ -31,4 +29,6 @@ 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 service::SkillsLoadInput; +pub use service::SkillsService; pub use skill_instructions::SkillInstructions; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 3abb30c96..6afbde6cc 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -132,14 +132,14 @@ impl SkillLoadOutcome { } } -/// Host-loaded skills for one turn, including the filesystem mapping needed to -/// read skill bodies through the environment that loaded them. +/// Immutable snapshot of host-owned skills and the filesystem mapping needed +/// to read each skill through the environment that discovered it. #[derive(Debug, Clone)] -pub struct HostLoadedSkills { +pub struct HostSkillsSnapshot { outcome: Arc, } -impl HostLoadedSkills { +impl HostSkillsSnapshot { pub fn new(outcome: Arc) -> Self { Self { outcome } } diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/service.rs similarity index 85% rename from codex-rs/core-skills/src/manager.rs rename to codex-rs/core-skills/src/service.rs index f8c5bfd04..29d34c380 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/service.rs @@ -13,6 +13,7 @@ use tracing::info; use tracing::instrument; use tracing::warn; +use crate::HostSkillsSnapshot; use crate::SkillLoadOutcome; use crate::build_implicit_skill_path_indexes; use crate::config_rules::SkillConfigRules; @@ -49,15 +50,18 @@ impl SkillsLoadInput { } } -pub struct SkillsManager { +/// Owns host skill discovery, immutable snapshots, cache invalidation, and extra roots. +/// +/// Source-specific model exposure remains the responsibility of the skills extension. +pub struct SkillsService { codex_home: AbsolutePathBuf, restriction_product: Option, extra_roots: RwLock>, - cache_by_cwd: RwLock>, - cache_by_config: RwLock>, + cache_by_cwd: RwLock>, + cache_by_config: RwLock>, } -impl SkillsManager { +impl SkillsService { pub fn new(codex_home: AbsolutePathBuf, bundled_skills_enabled: bool) -> Self { Self::new_with_restriction_product(codex_home, bundled_skills_enabled, Some(Product::Codex)) } @@ -67,7 +71,7 @@ impl SkillsManager { bundled_skills_enabled: bool, restriction_product: Option, ) -> Self { - let manager = Self { + let service = Self { codex_home, restriction_product, extra_roots: RwLock::new(Vec::new()), @@ -77,11 +81,11 @@ impl SkillsManager { if !bundled_skills_enabled { // The loader caches bundled skills under `skills/.system`. Clearing that directory is // best-effort cleanup; root selection still enforces the config even if removal fails. - uninstall_system_skills(&manager.codex_home); - } else if let Err(err) = install_system_skills(&manager.codex_home) { + uninstall_system_skills(&service.codex_home); + } else if let Err(err) = install_system_skills(&service.codex_home) { tracing::error!("failed to install system skills: {err}"); } - manager + service } pub fn set_extra_roots(&self, extra_roots: Vec) { @@ -102,25 +106,27 @@ impl SkillsManager { /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen /// to share a directory. #[instrument(level = "trace", skip_all)] - pub async fn skills_for_config( + pub async fn snapshot_for_config( &self, input: &SkillsLoadInput, fs: Option>, - ) -> SkillLoadOutcome { + ) -> HostSkillsSnapshot { let roots = self.skill_roots_for_config(input, fs).await; let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); let cache_key = config_skills_cache_key(&roots, &skill_config_rules); - if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { - return outcome; + if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key) { + return snapshot; } - let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; + let snapshot = HostSkillsSnapshot::new(Arc::new( + self.build_skill_outcome(roots, &skill_config_rules).await, + )); let mut cache = self .cache_by_config .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(cache_key, outcome.clone()); - outcome + cache.insert(cache_key, snapshot.clone()); + snapshot } pub async fn skill_roots_for_config( @@ -142,18 +148,18 @@ impl SkillsManager { roots } - pub async fn skills_for_cwd( + pub async fn snapshot_for_cwd( &self, input: &SkillsLoadInput, force_reload: bool, fs: Option>, - ) -> SkillLoadOutcome { + ) -> HostSkillsSnapshot { let use_cwd_cache = fs.is_some(); if use_cwd_cache && !force_reload - && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) + && let Some(snapshot) = self.cached_snapshot_for_cwd(&input.cwd) { - return outcome; + return snapshot; } let mut roots = skill_roots( @@ -168,15 +174,17 @@ impl SkillsManager { roots.retain(|root| root.scope != SkillScope::System); } let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); - let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; + let snapshot = HostSkillsSnapshot::new(Arc::new( + self.build_skill_outcome(roots, &skill_config_rules).await, + )); if use_cwd_cache { let mut cache = self .cache_by_cwd .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(input.cwd.clone(), outcome.clone()); + cache.insert(input.cwd.clone(), snapshot.clone()); } - outcome + snapshot } #[instrument(level = "trace", skip_all)] @@ -216,17 +224,17 @@ impl SkillsManager { info!("skills cache cleared ({cleared} entries)"); } - fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option { + fn cached_snapshot_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option { match self.cache_by_cwd.read() { Ok(cache) => cache.get(cwd).cloned(), Err(err) => err.into_inner().get(cwd).cloned(), } } - fn cached_outcome_for_config( + fn cached_snapshot_for_config( &self, cache_key: &ConfigSkillsCacheKey, - ) -> Option { + ) -> Option { match self.cache_by_config.read() { Ok(cache) => cache.get(cache_key).cloned(), Err(err) => err.into_inner().get(cache_key).cloned(), @@ -303,5 +311,5 @@ fn finalize_skill_outcome( } #[cfg(test)] -#[path = "manager_tests.rs"] +#[path = "service_tests.rs"] mod tests; diff --git a/codex-rs/core-skills/src/manager_tests.rs b/codex-rs/core-skills/src/service_tests.rs similarity index 91% rename from codex-rs/core-skills/src/manager_tests.rs rename to codex-rs/core-skills/src/service_tests.rs index 713b94d7b..bc05ebf35 100644 --- a/codex-rs/core-skills/src/manager_tests.rs +++ b/codex-rs/core-skills/src/service_tests.rs @@ -159,7 +159,7 @@ enabled = {enabled} } async fn skills_for_config_with_stack( - skills_manager: &SkillsManager, + skills_service: &SkillsService, cwd: &TempDir, config_layer_stack: &ConfigLayerStack, effective_skill_roots: &[PluginSkillRoot], @@ -170,9 +170,11 @@ async fn skills_for_config_with_stack( config_layer_stack.clone(), bundled_skills_enabled_from_stack(config_layer_stack), ); - skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS))) + skills_service + .snapshot_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS))) .await + .outcome() + .clone() } #[test] @@ -183,7 +185,7 @@ fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() { fs::write(stale_system_skill_dir.join("SKILL.md"), "# stale\n") .expect("write stale system skill"); - let _skills_manager = SkillsManager::new( + let _skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ false, ); @@ -199,14 +201,14 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { let codex_home = tempfile::tempdir().expect("tempdir"); let cwd = tempfile::tempdir().expect("tempdir"); let config_layer_stack = config_stack(&codex_home, ""); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); write_user_skill(&codex_home, "a", "skill-a", "from a"); let outcome1 = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!( outcome1.skills.iter().any(|s| s.name == "skill-a"), "expected skill-a to be discovered" @@ -216,7 +218,7 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { // entry because the effective skill config is unchanged. write_user_skill(&codex_home, "b", "skill-b", "from b"); let outcome2 = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert_eq!(outcome2.errors, outcome1.errors); assert_eq!(outcome2.skills, outcome1.skills); } @@ -227,7 +229,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { let cwd = tempfile::tempdir().expect("tempdir"); let extra_root = tempfile::tempdir().expect("tempdir"); let config_layer_stack = config_stack(&codex_home, ""); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); @@ -238,13 +240,14 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), ); - let empty_outcome = skills_manager - .skills_for_cwd( + let empty_snapshot = skills_service + .snapshot_for_cwd( &skills_input, /*force_reload*/ false, Some(Arc::clone(&LOCAL_FS)), ) .await; + let empty_outcome = empty_snapshot.outcome(); assert!( empty_outcome .skills @@ -260,15 +263,16 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { "---\nname: runtime-skill\ndescription: runtime skill\n---\n\n# Body\n", ) .expect("write skill"); - skills_manager.set_extra_roots(vec![extra_skills_root.abs()]); + skills_service.set_extra_roots(vec![extra_skills_root.abs()]); - let runtime_outcome = skills_manager - .skills_for_cwd( + let runtime_snapshot = skills_service + .snapshot_for_cwd( &skills_input, /*force_reload*/ false, Some(Arc::clone(&LOCAL_FS)), ) .await; + let runtime_outcome = runtime_snapshot.outcome(); assert!( runtime_outcome .skills @@ -276,14 +280,15 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { .any(|skill| skill.name == "runtime-skill") ); - skills_manager.set_extra_roots(vec![extra_root.path().join("missing-skills").abs()]); - let replaced_outcome = skills_manager - .skills_for_cwd( + skills_service.set_extra_roots(vec![extra_root.path().join("missing-skills").abs()]); + let replaced_snapshot = skills_service + .snapshot_for_cwd( &skills_input, /*force_reload*/ false, Some(Arc::clone(&LOCAL_FS)), ) .await; + let replaced_outcome = replaced_snapshot.outcome(); assert_eq!(replaced_outcome.errors, Vec::new()); assert!( replaced_outcome @@ -299,13 +304,13 @@ async fn set_extra_roots_applies_to_config_loads_and_empty_clears() { let cwd = tempfile::tempdir().expect("tempdir"); let extra_root = tempfile::tempdir().expect("tempdir"); let config_layer_stack = config_stack(&codex_home, ""); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); let empty_outcome = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!( empty_outcome .skills @@ -321,10 +326,10 @@ async fn set_extra_roots_applies_to_config_loads_and_empty_clears() { "---\nname: runtime-skill\ndescription: runtime skill\n---\n\n# Body\n", ) .expect("write skill"); - skills_manager.set_extra_roots(vec![extra_skills_root.abs()]); + skills_service.set_extra_roots(vec![extra_skills_root.abs()]); let runtime_outcome = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!( runtime_outcome .skills @@ -332,9 +337,9 @@ async fn set_extra_roots_applies_to_config_loads_and_empty_clears() { .any(|skill| skill.name == "runtime-skill") ); - skills_manager.set_extra_roots(Vec::new()); + skills_service.set_extra_roots(Vec::new()); let cleared_outcome = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!( cleared_outcome .skills @@ -360,13 +365,13 @@ async fn skills_for_config_disables_plugin_skills_by_name() { &name_toggle_config("sample:sample-search", /*enabled*/ false), ); let plugin_skill_root = plugin_skill_root_for_skill_path(&skill_path, "test-plugin@test"); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); let outcome = skills_for_config_with_stack( - &skills_manager, + &skills_service, &cwd, &config_layer_stack, &[plugin_skill_root], @@ -427,18 +432,19 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() { config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), ); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let outcome = skills_manager - .skills_for_cwd( + let snapshot = skills_service + .snapshot_for_cwd( &skills_input, /*force_reload*/ true, Some(Arc::clone(&LOCAL_FS)), ) .await; + let outcome = snapshot.outcome(); assert!( outcome.errors.is_empty(), @@ -490,14 +496,15 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() { config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), ); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let outcome = skills_manager - .skills_for_cwd(&skills_input, /*force_reload*/ true, /*fs*/ None) + let snapshot = skills_service + .snapshot_for_cwd(&skills_input, /*force_reload*/ true, /*fs*/ None) .await; + let outcome = snapshot.outcome(); assert!( outcome.errors.is_empty(), @@ -525,7 +532,7 @@ async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { ) .expect("write bundled skill"); let config_layer_stack = config_stack(&codex_home, "[skills.bundled]\nenabled = false\n"); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ false, ); @@ -540,7 +547,7 @@ async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() { .expect("rewrite bundled skill"); let outcome = - skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; assert!( outcome .skills @@ -560,24 +567,25 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { let codex_home = tempfile::tempdir().expect("tempdir"); let cwd = tempfile::tempdir().expect("tempdir"); let config_layer_stack = config_stack(&codex_home, ""); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); - let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await; + let _ = skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; let base_input = SkillsLoadInput::new( cwd.path().abs(), Vec::new(), config_layer_stack.clone(), bundled_skills_enabled_from_stack(&config_layer_stack), ); - let outcome_a = skills_manager - .skills_for_cwd( + let snapshot_a = skills_service + .snapshot_for_cwd( &base_input, /*force_reload*/ false, Some(Arc::clone(&LOCAL_FS)), ) .await; + let outcome_a = snapshot_a.outcome(); assert!( outcome_a .skills @@ -587,13 +595,14 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { write_user_skill(&codex_home, "late", "late-skill", "added after cache"); - let outcome_b = skills_manager - .skills_for_cwd( + let snapshot_b = skills_service + .snapshot_for_cwd( &base_input, /*force_reload*/ false, Some(Arc::clone(&LOCAL_FS)), ) .await; + let outcome_b = snapshot_b.outcome(); assert!( outcome_b .skills @@ -601,13 +610,14 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { .all(|skill| skill.name != "late-skill") ); - let outcome_reloaded = skills_manager - .skills_for_cwd( + let snapshot_reloaded = skills_service + .snapshot_for_cwd( &base_input, /*force_reload*/ true, Some(Arc::clone(&LOCAL_FS)), ) .await; + let outcome_reloaded = snapshot_reloaded.outcome(); assert!( outcome_reloaded .skills @@ -775,7 +785,7 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() let parent_stack = config_stack(&codex_home, &disabled_skill_config); let child_stack = config_stack_with_session_flags(&codex_home, &disabled_skill_config, &enabled_skill_config); - let skills_manager = SkillsManager::new( + let skills_service = SkillsService::new( codex_home.path().abs(), /*bundled_skills_enabled*/ true, ); @@ -786,13 +796,14 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() bundled_skills_enabled_from_stack(&parent_stack), ); - let parent_outcome = skills_manager - .skills_for_cwd( + let parent_snapshot = skills_service + .snapshot_for_cwd( &parent_input, /*force_reload*/ true, Some(Arc::clone(&LOCAL_FS)), ) .await; + let parent_outcome = parent_snapshot.outcome(); let parent_skill = parent_outcome .skills .iter() @@ -801,7 +812,7 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() assert_eq!(parent_outcome.is_skill_enabled(parent_skill), false); let child_outcome = - skills_for_config_with_stack(&skills_manager, &cwd, &child_stack, &[]).await; + skills_for_config_with_stack(&skills_service, &cwd, &child_stack, &[]).await; let child_skill = child_outcome .skills .iter() diff --git a/codex-rs/core/src/agent/role_tests.rs b/codex-rs/core/src/agent/role_tests.rs index 5461323a3..e258c0ad9 100644 --- a/codex-rs/core/src/agent/role_tests.rs +++ b/codex-rs/core/src/agent/role_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::SkillsManager; +use crate::SkillsService; use crate::config::ConfigBuilder; use crate::skills_load_input_from_config; use codex_config::ConfigLayerStackOrdering; @@ -416,18 +416,19 @@ enabled = false .expect("custom role should apply"); let plugins_manager = Arc::new(PluginsManager::new(home.path().to_path_buf())); - let skills_manager = - SkillsManager::new(home.path().abs(), /*bundled_skills_enabled*/ true); + let skills_service = + SkillsService::new(home.path().abs(), /*bundled_skills_enabled*/ true); 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 outcome = skills_manager - .skills_for_config( + let snapshot = skills_service + .snapshot_for_config( &skills_input, Some(Arc::clone(&codex_exec_server::LOCAL_FS)), ) .await; + let outcome = snapshot.outcome(); let skill = outcome .skills .iter() diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index d31dc138b..16398a2f2 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -94,7 +94,7 @@ pub(crate) async fn run_codex_thread_interactive( .services .turn_environments .environment_manager(), - skills_manager: Arc::clone(&parent_session.services.skills_manager), + skills_service: Arc::clone(&parent_session.services.skills_service), plugins_manager: Arc::clone(&parent_session.services.plugins_manager), mcp_manager: Arc::clone(&parent_session.services.mcp_manager), extensions: Arc::clone(&parent_session.services.extensions), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 3a6120b9f..72d3d45c9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -3874,7 +3874,7 @@ impl Config { } pub fn bundled_skills_enabled(&self) -> bool { - crate::manager::bundled_skills_enabled_from_stack(&self.config_layer_stack) + crate::skills::service::bundled_skills_enabled_from_stack(&self.config_layer_stack) } } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index fefe15cb7..b19cd9d35 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1678,7 +1678,7 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot() "---\nname: {GUARDIAN_SKILL_NAME}\ndescription: Guardian skill injection probe.\n---\n\n{GUARDIAN_SKILL_BODY_PROBE}\n" ), )?; - session.services.skills_manager.clear_cache(); + session.services.skills_service.clear_cache(); turn.config = Arc::clone(&config); turn.provider = create_model_provider(config.model_provider.clone(), turn.auth_manager.clone()); let session = Arc::new(session); diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 073c06ced..0641d7226 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -87,16 +87,14 @@ mod session_prefix; mod session_startup_prewarm; pub mod skills; pub(crate) use skills::SkillInjections; -pub(crate) use skills::SkillLoadOutcome; pub(crate) use skills::SkillMetadata; -pub(crate) use skills::SkillsManager; +pub(crate) use skills::SkillsService; pub(crate) use skills::build_available_skills; pub(crate) use skills::build_skill_injections; pub(crate) use skills::build_skill_name_counts; pub(crate) use skills::collect_explicit_skill_mentions; pub(crate) use skills::default_skill_metadata_budget; pub(crate) use skills::injection; -pub(crate) use skills::manager; pub(crate) use skills::maybe_emit_implicit_skill_invocation; pub(crate) use skills::skills_load_input_from_config; mod stream_events_utils; diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index d61de71bc..01d6246ce 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -291,11 +291,9 @@ pub(crate) struct PreviousTurnSettings { pub(crate) realtime_active: Option, } -#[cfg(test)] -use crate::SkillLoadOutcome; #[cfg(test)] use crate::SkillMetadata; -use crate::SkillsManager; +use crate::SkillsService; use crate::agents_md::load_project_instructions; use crate::exec_policy::ExecPolicyUpdateError; use crate::guardian::GuardianReviewSessionManager; @@ -304,6 +302,8 @@ use crate::network_policy_decision::execpolicy_network_rule_amendment; use crate::rollout::map_session_init_error; use crate::session_startup_prewarm::SessionStartupPrewarmHandle; use crate::shell; +#[cfg(test)] +use crate::skills::SkillLoadOutcome; use crate::state::AutoCompactWindowSnapshot; use crate::state::PendingRequestPermissions; use crate::state::SessionServices; @@ -409,7 +409,7 @@ pub(crate) struct CodexSpawnArgs { pub(crate) auth_manager: Arc, pub(crate) models_manager: SharedModelsManager, pub(crate) environment_manager: Arc, - pub(crate) skills_manager: Arc, + pub(crate) skills_service: Arc, pub(crate) plugins_manager: Arc, pub(crate) mcp_manager: Arc, pub(crate) extensions: Arc>, @@ -495,7 +495,7 @@ impl Codex { auth_manager, models_manager, environment_manager, - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions, @@ -651,7 +651,7 @@ impl Codex { agent_status_tx.clone(), conversation_history, session_source_clone, - skills_manager, + skills_service, plugins_manager, mcp_manager.clone(), extensions, @@ -1507,7 +1507,7 @@ impl Session { (previous_config, new_config, config) }; self.emit_config_changed_contributors(previous_config.as_ref(), new_config.as_ref()); - self.services.skills_manager.clear_cache(); + self.services.skills_service.clear_cache(); self.services.plugins_manager.clear_cache(); let environments = self.services.turn_environments.snapshot().await; let hooks = build_hooks_for_config( @@ -2969,7 +2969,7 @@ impl Session { } if turn_context.config.include_skill_instructions { let available_skills = build_available_skills( - &turn_context.turn_skills.outcome, + turn_context.turn_skills.snapshot.outcome(), default_skill_metadata_budget(turn_context.model_info.context_window), SkillRenderSideEffects::ThreadStart { session_telemetry: &self.services.session_telemetry, diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 0296d9e44..bbdfc3f98 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -1,5 +1,4 @@ use super::*; -use codex_core_skills::HostLoadedSkills; use std::sync::atomic::AtomicBool; /// Spawn a review thread using the given prompt. @@ -100,9 +99,7 @@ pub(super) async fn spawn_review_thread( let extension_data = Arc::new(codex_extension_api::ExtensionData::new( review_turn_id.clone(), )); - extension_data.insert(HostLoadedSkills::new( - parent_turn_context.turn_skills.outcome.clone(), - )); + extension_data.insert(parent_turn_context.turn_skills.snapshot.clone()); let review_turn_context = TurnContext { sub_id: review_turn_id.clone(), @@ -138,7 +135,7 @@ pub(super) async fn spawn_review_thread( dynamic_tools: parent_turn_context.dynamic_tools.clone(), turn_metadata_state, extension_data, - turn_skills: TurnSkillsContext::new(parent_turn_context.turn_skills.outcome.clone()), + turn_skills: TurnSkillsContext::new(parent_turn_context.turn_skills.snapshot.clone()), turn_timing_state: Arc::new(TurnTimingState::default()), terminal_error: Arc::new(Mutex::new(None)), server_model_warning_emitted: AtomicBool::new(false), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 578152814..c4bc064a4 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -438,7 +438,7 @@ pub(crate) struct AppServerClientMetadata { async fn warm_plugins_and_skills_for_session_init( config: Arc, plugins_manager: Arc, - skills_manager: Arc, + skills_service: Arc, turn_environments: &TurnEnvironmentSnapshot, ) -> Vec { let fs = turn_environments.primary_filesystem(); @@ -446,10 +446,12 @@ async fn warm_plugins_and_skills_for_session_init( 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); - skills_manager - .skills_for_config(&skills_input, fs) + skills_service + .snapshot_for_config(&skills_input, fs) .await + .outcome() .errors + .clone() } impl Session { @@ -477,7 +479,7 @@ impl Session { agent_status: watch::Sender, initial_history: InitialHistory, session_source: SessionSource, - skills_manager: Arc, + skills_service: Arc, plugins_manager: Arc, mcp_manager: Arc, extensions: Arc>, @@ -828,7 +830,7 @@ impl Session { let plugin_skill_errors = warm_plugins_and_skills_for_session_init( Arc::clone(&config), Arc::clone(&plugins_manager), - Arc::clone(&skills_manager), + Arc::clone(&skills_service), &resolved_environments, ) .instrument(info_span!( @@ -999,7 +1001,7 @@ impl Session { guardian_rejections: Mutex::new(HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), - skills_manager, + skills_service, plugins_manager: Arc::clone(&plugins_manager), mcp_manager: Arc::clone(&mcp_manager), extensions, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index a26fb6599..84c4a3888 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -24,6 +24,7 @@ use codex_config::RequirementSource; use codex_config::Sourced; use codex_config::loader::project_trust_key; use codex_config::types::ToolSuggestDisabledTool; +use codex_core_skills::HostSkillsSnapshot; use core_test_support::test_codex::local_selections; use codex_features::Feature; @@ -4434,15 +4435,16 @@ async fn new_default_turn_uses_config_aware_skills_for_role_overrides() { .default_environment() .map(|environment| environment.get_filesystem()) .unwrap_or_else(|| std::sync::Arc::clone(&codex_exec_server::LOCAL_FS)); - let parent_outcome = session + let parent_snapshot = session .services - .skills_manager - .skills_for_cwd( + .skills_service + .snapshot_for_cwd( &crate::skills_load_input_from_config(&parent_config, Vec::new()), /*force_reload*/ true, Some(Arc::clone(&skill_fs)), ) .await; + let parent_outcome = parent_snapshot.outcome(); let parent_skill = parent_outcome .skills .iter() @@ -4488,13 +4490,18 @@ enabled = false .await; let child_skill = child_turn .turn_skills - .outcome + .snapshot + .outcome() .skills .iter() .find(|skill| skill.name == "demo-skill") .expect("demo skill should be discovered"); assert_eq!( - child_turn.turn_skills.outcome.is_skill_enabled(child_skill), + child_turn + .turn_skills + .snapshot + .outcome() + .is_skill_enabled(child_skill), false ); } @@ -4825,7 +4832,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -4842,7 +4849,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { agent_status_tx, InitialHistory::New, SessionSource::Exec, - skills_manager, + skills_service, plugins_manager, mcp_manager, Arc::new(codex_extension_api::ExtensionRegistryBuilder::new().build()), @@ -4961,7 +4968,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { ); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -5000,7 +5007,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions: Arc::new(codex_extension_api::ExtensionRegistryBuilder::new().build()), @@ -5045,12 +5052,10 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { let skills_input = crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); let skill_fs = environment.get_filesystem(); - let skills_outcome = Arc::new( - services - .skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) - .await, - ); + let skills_snapshot = services + .skills_service + .snapshot_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .await; let turn_context = Session::make_turn_context( thread_id, SessionId::from(thread_id), @@ -5069,7 +5074,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { resolved_turn_environments, session_configuration.cwd().clone(), "turn_id".to_string(), - skills_outcome, + skills_snapshot, ); let session = Session { @@ -5173,7 +5178,7 @@ async fn make_session_with_config_and_rx( let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -5191,7 +5196,7 @@ async fn make_session_with_config_and_rx( agent_status_tx, InitialHistory::New, SessionSource::Exec, - skills_manager, + skills_service, plugins_manager, mcp_manager, Arc::new(codex_extension_api::ExtensionRegistryBuilder::new().build()), @@ -5277,7 +5282,7 @@ async fn make_session_with_history_source_and_agent_control_and_rx( let (agent_status_tx, _agent_status_rx) = watch::channel(AgentStatus::PendingInit); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -5295,7 +5300,7 @@ async fn make_session_with_history_source_and_agent_control_and_rx( agent_status_tx, initial_history, session_source, - skills_manager, + skills_service, plugins_manager, mcp_manager, Arc::new(codex_extension_api::ExtensionRegistryBuilder::new().build()), @@ -7006,7 +7011,7 @@ where ); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -7045,7 +7050,7 @@ where guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions: Arc::new(codex_extension_api::ExtensionRegistryBuilder::new().build()), @@ -7090,12 +7095,10 @@ where let skills_input = crate::skills_load_input_from_config(&per_turn_config, effective_skill_roots); let skill_fs = environment.get_filesystem(); - let skills_outcome = Arc::new( - services - .skills_manager - .skills_for_config(&skills_input, Some(Arc::clone(&skill_fs))) - .await, - ); + let skills_snapshot = services + .skills_service + .snapshot_for_config(&skills_input, Some(Arc::clone(&skill_fs))) + .await; let turn_context = Arc::new(Session::make_turn_context( thread_id, SessionId::from(thread_id), @@ -7114,7 +7117,7 @@ where resolved_turn_environments, session_configuration.cwd().clone(), "turn_id".to_string(), - skills_outcome, + skills_snapshot, )); let session = Arc::new(Session { @@ -7781,7 +7784,7 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget() }, ]; turn_context.model_info.context_window = Some(100); - turn_context.turn_skills = TurnSkillsContext::new(Arc::new(outcome)); + turn_context.turn_skills = TurnSkillsContext::new(HostSkillsSnapshot::new(Arc::new(outcome))); let initial_context = session.build_initial_context(&turn_context).await; let developer_texts = developer_input_texts(&initial_context); @@ -7932,7 +7935,7 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil }, ]; turn_context.model_info.context_window = Some(100); - turn_context.turn_skills = TurnSkillsContext::new(Arc::new(outcome)); + turn_context.turn_skills = TurnSkillsContext::new(HostSkillsSnapshot::new(Arc::new(outcome))); let _ = session.build_initial_context(&turn_context).await; let warning_event = timeout(Duration::from_secs(1), rx.recv()) diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 43cd91a99..b9727a040 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -694,7 +694,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { config.model_provider.clone(), ); let plugins_manager = Arc::new(PluginsManager::new(config.codex_home.to_path_buf())); - let skills_manager = Arc::new(SkillsManager::new( + let skills_service = Arc::new(SkillsService::new( config.codex_home.clone(), /*bundled_skills_enabled*/ true, )); @@ -711,7 +711,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() { auth_manager, models_manager, environment_manager: Arc::new(EnvironmentManager::default_for_tests()), - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions: codex_extension_api::empty_extension_registry(), diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 7da157cf7..4beef792b 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -528,7 +528,7 @@ async fn build_skills_and_plugins( } else { Vec::new() }; - let skills_outcome = turn_context.turn_skills.outcome.as_ref(); + let skills_outcome = turn_context.turn_skills.snapshot.outcome(); let connector_slug_counts = build_connector_slug_counts(&available_connectors); let extension_injection_items = build_extension_turn_input_items(sess, turn_context, &user_input, cancellation_token) diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index b76c9384f..cd01f92b9 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -1,9 +1,8 @@ use super::*; -use crate::SkillLoadOutcome; use crate::agents_md::LoadedAgentsMd; use crate::environment_selection::TurnEnvironmentSnapshot; use crate::shell_snapshot::ShellSnapshotFile; -use codex_core_skills::HostLoadedSkills; +use codex_core_skills::HostSkillsSnapshot; use codex_file_system::FileSystemSandboxContext; use codex_model_provider::SharedModelProvider; use codex_model_provider::create_model_provider; @@ -26,14 +25,14 @@ use tracing::instrument; #[derive(Clone, Debug)] pub(crate) struct TurnSkillsContext { - pub(crate) outcome: Arc, + pub(crate) snapshot: HostSkillsSnapshot, pub(crate) implicit_invocation_seen_skills: Arc>>, } impl TurnSkillsContext { - pub(crate) fn new(outcome: Arc) -> Self { + pub(crate) fn new(snapshot: HostSkillsSnapshot) -> Self { Self { - outcome, + snapshot, implicit_invocation_seen_skills: Arc::new(Mutex::new(HashSet::new())), } } @@ -488,7 +487,7 @@ impl Session { environments: TurnEnvironmentSnapshot, cwd: AbsolutePathBuf, sub_id: String, - skills_outcome: Arc, + skills_snapshot: HostSkillsSnapshot, ) -> TurnContext { let reasoning_effort = session_configuration.collaboration_mode.reasoning_effort(); let reasoning_summary = session_configuration @@ -531,7 +530,7 @@ impl Session { )); let (current_date, timezone) = local_time_context(); let extension_data = Arc::new(codex_extension_api::ExtensionData::new(sub_id.clone())); - extension_data.insert(HostLoadedSkills::new(Arc::clone(&skills_outcome))); + extension_data.insert(skills_snapshot.clone()); TurnContext { sub_id, trace_id: current_span_trace_id(), @@ -569,7 +568,7 @@ impl Session { dynamic_tools: session_configuration.dynamic_tools.clone(), turn_metadata_state, extension_data, - turn_skills: TurnSkillsContext::new(skills_outcome), + turn_skills: TurnSkillsContext::new(skills_snapshot), turn_timing_state: Arc::new(TurnTimingState::default()), terminal_error: Arc::new(Mutex::new(None)), server_model_warning_emitted: AtomicBool::new(false), @@ -726,12 +725,11 @@ impl Session { let skills_input = skills_load_input_from_config(&per_turn_config, effective_skill_roots); let fs = primary_turn_environment .map(|turn_environment| turn_environment.environment.get_filesystem()); - let skills_outcome = Arc::new( - self.services - .skills_manager - .skills_for_config(&skills_input, fs) - .await, - ); + let skills_snapshot = self + .services + .skills_service + .snapshot_for_config(&skills_input, fs) + .await; let mut turn_context: TurnContext = Self::make_turn_context( self.thread_id(), self.session_id(), @@ -759,7 +757,7 @@ impl Session { turn_environments, cwd, sub_id, - skills_outcome, + skills_snapshot, ); turn_context.realtime_active = self.conversation.running_state().await.is_some(); diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index b11f79260..2c1ba8245 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -14,7 +14,7 @@ pub use codex_core_skills::SkillMetadata; pub use codex_core_skills::SkillPolicy; pub use codex_core_skills::SkillRenderReport; pub use codex_core_skills::SkillsLoadInput; -pub use codex_core_skills::SkillsManager; +pub use codex_core_skills::SkillsService; pub use codex_core_skills::build_available_skills; pub use codex_core_skills::build_skill_name_counts; pub use codex_core_skills::config_rules; @@ -26,11 +26,11 @@ pub use codex_core_skills::injection::SkillInjections; pub use codex_core_skills::injection::build_skill_injections; pub use codex_core_skills::injection::collect_explicit_skill_mentions; pub use codex_core_skills::loader; -pub use codex_core_skills::manager; pub use codex_core_skills::model; pub use codex_core_skills::remote; pub use codex_core_skills::render; pub use codex_core_skills::render::SkillRenderSideEffects; +pub use codex_core_skills::service; pub use codex_core_skills::system; pub(crate) fn skills_load_input_from_config( @@ -52,7 +52,7 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( workdir: &AbsolutePathBuf, ) { let Some(candidate) = detect_implicit_skill_invocation_for_command( - turn_context.turn_skills.outcome.as_ref(), + turn_context.turn_skills.snapshot.outcome(), command, workdir, ) else { diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 00380c6d6..9724d8b3a 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::sync::Arc; -use crate::SkillsManager; +use crate::SkillsService; use crate::agent::AgentControl; use crate::attestation::AttestationProvider; use crate::client::ModelClient; @@ -61,7 +61,7 @@ pub(crate) struct SessionServices { pub(crate) guardian_rejections: Mutex>, pub(crate) guardian_rejection_circuit_breaker: Mutex, pub(crate) runtime_handle: Handle, - pub(crate) skills_manager: Arc, + pub(crate) skills_service: Arc, pub(crate) plugins_manager: Arc, pub(crate) mcp_manager: Arc, pub(crate) extensions: Arc>, diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 2adb713a0..63335c012 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -1,4 +1,4 @@ -use crate::SkillsManager; +use crate::SkillsService; use crate::agent::AgentControl; use crate::attestation::AttestationProvider; use crate::codex_thread::CodexThread; @@ -207,7 +207,7 @@ pub(crate) struct ThreadManagerState { auth_manager: Arc, models_manager: SharedModelsManager, environment_manager: Arc, - skills_manager: Arc, + skills_service: Arc, plugins_manager: Arc, mcp_manager: Arc, extensions: Arc>, @@ -281,7 +281,7 @@ impl ThreadManager { Arc::clone(&plugins_manager), Arc::clone(&extensions), )); - let skills_manager = Arc::new(SkillsManager::new_with_restriction_product( + let skills_service = Arc::new(SkillsService::new_with_restriction_product( codex_home, config.bundled_skills_enabled(), restriction_product, @@ -292,7 +292,7 @@ impl ThreadManager { thread_created_tx, models_manager: build_models_manager(config, auth_manager.clone()), environment_manager, - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions, @@ -373,7 +373,7 @@ impl ThreadManager { auth_manager.get_api_auth_mode(), )); let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager))); - let skills_manager = Arc::new(SkillsManager::new_with_restriction_product( + let skills_service = Arc::new(SkillsService::new_with_restriction_product( skills_codex_home, /*bundled_skills_enabled*/ true, restriction_product, @@ -395,7 +395,7 @@ impl ThreadManager { models_manager: create_model_provider(provider, Some(auth_manager.clone())) .models_manager(codex_home, /*config_model_catalog*/ None), environment_manager, - skills_manager, + skills_service, plugins_manager, mcp_manager, extensions: empty_extension_registry(), @@ -424,8 +424,8 @@ impl ThreadManager { self.state.auth_manager.clone() } - pub fn skills_manager(&self) -> Arc { - self.state.skills_manager.clone() + pub fn skills_service(&self) -> Arc { + self.state.skills_service.clone() } pub fn plugins_manager(&self) -> Arc { @@ -1406,7 +1406,7 @@ impl ThreadManagerState { auth_manager, models_manager: Arc::clone(&self.models_manager), environment_manager: Arc::clone(&self.environment_manager), - skills_manager: Arc::clone(&self.skills_manager), + skills_service: Arc::clone(&self.skills_service), plugins_manager: Arc::clone(&self.plugins_manager), mcp_manager: Arc::clone(&self.mcp_manager), extensions: Arc::clone(&self.extensions), diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index bf4d86d36..c82ce6642 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use codex_core_skills::HostLoadedSkills; +use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_exec_server::LOCAL_ENVIRONMENT_ID; use codex_extension_api::ConfigContributor; @@ -121,7 +121,7 @@ where SkillListQuery { turn_id: thread_store.level_id().to_string(), executor_roots: thread_state.selected_roots().to_vec(), - host: None, + host_snapshot: None, include_host_skills: false, include_bundled_skills: config.bundled_skills_enabled, include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), @@ -184,11 +184,11 @@ where }; let config = thread_state.config(); - let host_loaded_skills = turn_store.get::(); + let host_snapshot = turn_store.get::(); let query = SkillListQuery { turn_id: input.turn_id.clone(), executor_roots: thread_state.selected_roots().to_vec(), - host: host_loaded_skills.clone(), + host_snapshot: host_snapshot.clone(), include_host_skills: true, include_bundled_skills: config.bundled_skills_enabled, include_orchestrator_skills: thread_state.orchestrator_skills_enabled(), @@ -217,12 +217,7 @@ where let mut injected_host_skill_prompts = InjectedHostSkillPrompts::default(); for entry in &selected_entries { match self - .read_main_prompt( - entry, - host_loaded_skills.clone(), - session_store, - &thread_state, - ) + .read_main_prompt(entry, host_snapshot.clone(), session_store, &thread_state) .await { Ok(read_result) => { @@ -259,12 +254,12 @@ where } } - if let Some(host_loaded_skills) = &host_loaded_skills { + if let Some(host_snapshot) = &host_snapshot { for entry in selected_entries .iter() .filter(|entry| entry.authority.kind != SkillSourceKind::Host) { - for host_skill in host_loaded_skills + for host_skill in host_snapshot .outcome() .skills .iter() @@ -319,7 +314,7 @@ impl SkillsExtension { async fn read_main_prompt( &self, entry: &SkillCatalogEntry, - host_loaded_skills: Option>, + host_snapshot: Option>, session_store: &ExtensionData, thread_state: &SkillsThreadState, ) -> Result { @@ -330,7 +325,7 @@ impl SkillsExtension { authority: entry.authority.clone(), package: entry.id.clone(), resource: entry.main_prompt.clone(), - host: host_loaded_skills, + host_snapshot, mcp_resources: session_store.get::(), }, ) diff --git a/codex-rs/ext/skills/src/provider.rs b/codex-rs/ext/skills/src/provider.rs index 329c0c581..405592e93 100644 --- a/codex-rs/ext/skills/src/provider.rs +++ b/codex-rs/ext/skills/src/provider.rs @@ -6,7 +6,7 @@ mod executor; mod host; mod orchestrator; -use codex_core_skills::HostLoadedSkills; +use codex_core_skills::HostSkillsSnapshot; use codex_mcp::McpResourceClient; use codex_protocol::capabilities::SelectedCapabilityRoot; @@ -26,7 +26,7 @@ pub use orchestrator::OrchestratorSkillProvider; pub struct SkillListQuery { pub turn_id: String, pub executor_roots: Vec, - pub host: Option>, + pub host_snapshot: Option>, pub include_host_skills: bool, pub include_bundled_skills: bool, pub include_orchestrator_skills: bool, @@ -38,7 +38,7 @@ pub struct SkillReadRequest { pub authority: SkillAuthority, pub package: SkillPackageId, pub resource: SkillResourceId, - pub host: Option>, + pub host_snapshot: Option>, pub mcp_resources: Option>, } diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 63d0d4588..2cee69421 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -18,12 +18,10 @@ use crate::provider::SkillSearchRequest; const HOST_AUTHORITY_ID: &str = "host"; -/// Host-owned skill provider backed by the already-loaded turn skills. +/// Host-owned skill provider backed by an immutable service snapshot. /// -/// The provider intentionally does not reload or cache host skills. Core owns -/// skill loading, including plugin roots, runtime extra roots, and the primary -/// environment filesystem. This adapter only maps that loaded outcome into the -/// skills-extension catalog/read contract. +/// Discovery and caching belong to `SkillsService`; this provider only maps a +/// snapshot into the authority-aware catalog/read contract. #[derive(Clone, Default)] pub struct HostSkillProvider; @@ -36,24 +34,24 @@ impl HostSkillProvider { impl SkillProvider for HostSkillProvider { fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { Box::pin(async move { - let Some(host_loaded_skills) = query.host else { + let Some(host_snapshot) = query.host_snapshot else { return Err(SkillProviderError::new( - "host skill provider requires loaded host skills", + "host skill provider requires a host skills snapshot", )); }; - Ok(catalog_from_outcome(host_loaded_skills.outcome())) + Ok(catalog_from_outcome(host_snapshot.outcome())) }) } fn read(&self, request: SkillReadRequest) -> SkillProviderFuture<'_, SkillReadResult> { Box::pin(async move { - let Some(host_loaded_skills) = request.host else { + let Some(host_snapshot) = request.host_snapshot else { return Err(SkillProviderError::new( - "host skill provider requires loaded host skills", + "host skill provider requires a host skills snapshot", )); }; - let Some(skill) = host_loaded_skills.outcome().skills.iter().find(|skill| { + let Some(skill) = host_snapshot.outcome().skills.iter().find(|skill| { let skill_path = skill.path_to_skills_md.to_string_lossy(); skill_path == request.resource.as_str() || skill_path.replace('\\', "/") == request.resource.as_str() @@ -64,15 +62,12 @@ impl SkillProvider for HostSkillProvider { ))); }; - let contents = host_loaded_skills - .read_skill_text(skill) - .await - .map_err(|err| { - SkillProviderError::new(format!( - "failed to read host skill resource {}: {err}", - request.resource.as_str() - )) - })?; + let contents = host_snapshot.read_skill_text(skill).await.map_err(|err| { + SkillProviderError::new(format!( + "failed to read host skill resource {}: {err}", + request.resource.as_str() + )) + })?; Ok(SkillReadResult { resource: request.resource, diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index a1898b1a2..0d05064af 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -68,7 +68,7 @@ impl SkillToolContext { self.providers.list_orchestrator_for_turn(SkillListQuery { turn_id: turn_id.to_string(), executor_roots: Vec::new(), - host: None, + host_snapshot: None, include_host_skills: false, include_bundled_skills: false, include_orchestrator_skills: true, diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index 358842aa1..64587cab9 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -82,7 +82,7 @@ impl ToolExecutor for ReadTool { authority, package: SkillPackageId(args.package), resource: requested_resource.clone(), - host: None, + host_snapshot: None, mcp_resources: self.context.mcp_resources.clone(), }, ) 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 f3d17e352..1eff53c3b 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use codex_core_skills::HostLoadedSkills; +use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::CopyOptions; @@ -208,7 +208,7 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { skill.path_to_skills_md, canonical_root.join("skill/SKILL.md") ); - let loaded = HostLoadedSkills::new(Arc::new(outcome)); + let loaded = HostSkillsSnapshot::new(Arc::new(outcome)); assert_eq!( loaded.read_skill_text(&skill).await.expect("skill body"), SKILL_CONTENTS @@ -242,7 +242,7 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { }, }) .collect(), - host: None, + host_snapshot: None, include_host_skills: false, include_bundled_skills: true, include_orchestrator_skills: false, diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index bcf058d6b..cd6f46d9c 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -4,7 +4,7 @@ use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use codex_core_skills::HostLoadedSkills; +use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::SKILLS_HOW_TO_USE_WITH_ABSOLUTE_PATHS; use codex_core_skills::SKILLS_INTRO_WITH_ABSOLUTE_PATHS; use codex_core_skills::SkillLoadOutcome; @@ -52,7 +52,7 @@ const DEMO_SKILL_CONTENTS: &str = "---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n\nUse the demo skill.\n"; #[tokio::test] -async fn installed_extension_uses_host_loaded_skills() -> TestResult { +async fn installed_extension_uses_host_service_snapshot() -> TestResult { let codex_home = test_codex_home(); let skill_path = codex_home.join("skills").join("demo").join("SKILL.md"); std::fs::create_dir_all( @@ -97,7 +97,7 @@ async fn installed_extension_uses_host_loaded_skills() -> TestResult { let loaded_skills = Arc::new(outcome); let skill_prompt_path = skill_path_string.replace('\\', "/"); let turn_store = ExtensionData::new("turn-1"); - turn_store.insert(HostLoadedSkills::new(Arc::clone(&loaded_skills))); + turn_store.insert(HostSkillsSnapshot::new(Arc::clone(&loaded_skills))); let fragments = registry.turn_input_contributors()[0] .contribute(