From 665db63680c7ae6a77a43df9f12f354c0e343598 Mon Sep 17 00:00:00 2001 From: Adam Perry Date: Fri, 26 Jun 2026 19:34:16 +0000 Subject: [PATCH] core-skills: cache snapshots before root discovery --- codex-rs/core-skills/src/loader.rs | 4 +- codex-rs/core-skills/src/service.rs | 120 ++++++++++++++++------ codex-rs/core-skills/src/service_tests.rs | 38 +++++++ 3 files changed, 129 insertions(+), 33 deletions(-) diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index c9ace542046f..220942b76817 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -407,7 +407,9 @@ async fn repo_agents_skill_roots( roots } -fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec { +pub(crate) fn project_root_markers_from_stack( + config_layer_stack: &ConfigLayerStack, +) -> Vec { let mut merged = TomlValue::Table(toml::map::Map::new()); for layer in config_layer_stack.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, diff --git a/codex-rs/core-skills/src/service.rs b/codex-rs/core-skills/src/service.rs index b4e0d7396c2e..976db698c9c5 100644 --- a/codex-rs/core-skills/src/service.rs +++ b/codex-rs/core-skills/src/service.rs @@ -3,7 +3,9 @@ use std::collections::HashSet; use std::sync::Arc; use std::sync::RwLock; +use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; +use codex_config::ConfigLayerStackOrdering; use codex_exec_server::ExecutorFileSystem; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -22,6 +24,7 @@ use crate::config_rules::resolve_disabled_skill_paths; use crate::config_rules::skill_config_rules_from_stack; use crate::loader::SkillRoot; use crate::loader::load_skills_from_roots; +use crate::loader::project_root_markers_from_stack; use crate::loader::skill_roots; use crate::system::install_system_skills; use crate::system::uninstall_system_skills; @@ -70,7 +73,7 @@ pub struct SkillsService { restriction_product: Option, extra_roots: RwLock>, cache_by_cwd: RwLock>, - cache_by_config: RwLock>, + cache_by_config: RwLock>, } impl SkillsService { @@ -88,7 +91,7 @@ impl SkillsService { restriction_product, extra_roots: RwLock::new(Vec::new()), cache_by_cwd: RwLock::new(HashMap::new()), - cache_by_config: RwLock::new(HashMap::new()), + cache_by_config: RwLock::new(Vec::new()), }; if !bundled_skills_enabled { // The loader caches bundled skills under `skills/.system`. Clearing that directory is @@ -128,22 +131,36 @@ impl SkillsService { input: &SkillsLoadInput, fs: Option>, ) -> 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(snapshot) = self.cached_snapshot_for_config(&cache_key) { + let extra_roots = self.extra_roots(); + let cache_key = config_skills_cache_key(input, &extra_roots, skill_config_rules); + if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key, fs.as_ref()) { return snapshot; } + let roots = self + .skill_roots_for_config_with_extra_roots(input, fs.clone(), extra_roots) + .await; let snapshot = HostSkillsSnapshot::new(Arc::new( - self.build_skill_outcome(input, roots, &skill_config_rules) + self.build_skill_outcome(input, roots, &cache_key.skill_config_rules) .await, )); let mut cache = self .cache_by_config .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(cache_key, snapshot.clone()); + if let Some(entry) = cache + .iter_mut() + .find(|entry| entry.matches(&cache_key, fs.as_ref())) + { + entry.snapshot = snapshot.clone(); + } else { + cache.push(ConfigSkillsCacheEntry { + key: cache_key, + file_system: fs, + snapshot: snapshot.clone(), + }); + } snapshot } @@ -151,13 +168,23 @@ impl SkillsService { &self, input: &SkillsLoadInput, fs: Option>, + ) -> Vec { + self.skill_roots_for_config_with_extra_roots(input, fs, self.extra_roots()) + .await + } + + async fn skill_roots_for_config_with_extra_roots( + &self, + input: &SkillsLoadInput, + fs: Option>, + extra_roots: Vec, ) -> Vec { let mut roots = skill_roots( fs, &input.config_layer_stack, &input.cwd, input.effective_skill_roots.clone(), - self.extra_roots(), + extra_roots, ) .await; if !input.bundled_skills_enabled { @@ -253,11 +280,16 @@ impl SkillsService { fn cached_snapshot_for_config( &self, cache_key: &ConfigSkillsCacheKey, + file_system: Option<&Arc>, ) -> Option { - match self.cache_by_config.read() { - Ok(cache) => cache.get(cache_key).cloned(), - Err(err) => err.into_inner().get(cache_key).cloned(), - } + let cache = self + .cache_by_config + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache + .iter() + .find(|entry| entry.matches(cache_key, file_system)) + .map(|entry| entry.snapshot.clone()) } fn extra_roots(&self) -> Vec { @@ -268,9 +300,35 @@ impl SkillsService { } } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct ConfigSkillsCacheEntry { + key: ConfigSkillsCacheKey, + file_system: Option>, + snapshot: HostSkillsSnapshot, +} + +impl ConfigSkillsCacheEntry { + fn matches( + &self, + key: &ConfigSkillsCacheKey, + file_system: Option<&Arc>, + ) -> bool { + self.key == *key + && match (&self.file_system, file_system) { + (Some(cached), Some(requested)) => Arc::ptr_eq(cached, requested), + (None, None) => true, + (Some(_), None) | (None, Some(_)) => false, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] struct ConfigSkillsCacheKey { - roots: Vec<(AbsolutePathBuf, u8, Option, Option)>, + cwd: AbsolutePathBuf, + effective_skill_roots: Vec, + config_layer_sources: Vec, + project_root_markers: Vec, + extra_roots: Vec, + bundled_skills_enabled: bool, skill_config_rules: SkillConfigRules, } @@ -297,28 +355,26 @@ pub fn bundled_skills_enabled_from_stack( } fn config_skills_cache_key( - roots: &[SkillRoot], - skill_config_rules: &SkillConfigRules, + input: &SkillsLoadInput, + extra_roots: &[AbsolutePathBuf], + skill_config_rules: SkillConfigRules, ) -> ConfigSkillsCacheKey { ConfigSkillsCacheKey { - roots: roots + cwd: input.cwd.clone(), + effective_skill_roots: input.effective_skill_roots.clone(), + config_layer_sources: input + .config_layer_stack + .get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) .iter() - .map(|root| { - let scope_rank = match root.scope { - SkillScope::Repo => 0, - SkillScope::User => 1, - SkillScope::System => 2, - SkillScope::Admin => 3, - }; - ( - root.path.clone(), - scope_rank, - root.plugin_id.clone(), - root.plugin_namespace.clone(), - ) - }) + .map(|layer| layer.name.clone()) .collect(), - skill_config_rules: skill_config_rules.clone(), + project_root_markers: project_root_markers_from_stack(&input.config_layer_stack), + extra_roots: extra_roots.to_vec(), + bundled_skills_enabled: input.bundled_skills_enabled, + skill_config_rules, } } diff --git a/codex-rs/core-skills/src/service_tests.rs b/codex-rs/core-skills/src/service_tests.rs index 6026975097e6..42dda2ba9a70 100644 --- a/codex-rs/core-skills/src/service_tests.rs +++ b/codex-rs/core-skills/src/service_tests.rs @@ -228,6 +228,44 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { assert_eq!(outcome2.skills, outcome1.skills); } +#[tokio::test] +async fn skills_for_config_keeps_repo_root_discovery_with_snapshot_cache_lifecycle() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let config_layer_stack = config_stack(&codex_home, ""); + let skills_service = SkillsService::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ true, + ); + + let outcome1 = + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; + let repo_skill_dir = cwd.path().join(".agents/skills/repo"); + fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir"); + fs::write( + repo_skill_dir.join("SKILL.md"), + "---\nname: repo-skill\ndescription: repo description\n---\n\n# Body\n", + ) + .expect("write repo skill"); + + let outcome2 = + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; + assert_eq!(outcome2.errors, outcome1.errors); + assert_eq!(outcome2.skills, outcome1.skills); + + skills_service.clear_cache(); + + let outcome3 = + skills_for_config_with_stack(&skills_service, &cwd, &config_layer_stack, &[]).await; + assert!( + outcome3 + .skills + .iter() + .any(|skill| skill.name == "repo-skill"), + "expected repo-skill after clearing the snapshot cache" + ); +} + #[tokio::test] async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() { let codex_home = tempfile::tempdir().expect("tempdir");