diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 2678094b02f..d30e5a3eafa 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1201,6 +1201,7 @@ impl Session { // todo(aibrahim): store this state somewhere else so we don't need to mut config let config = session_configuration.original_config_do_not_use.clone(); let mut per_turn_config = (*config).clone(); + per_turn_config.cwd = session_configuration.cwd.clone(); per_turn_config.model_reasoning_effort = session_configuration.collaboration_mode.reasoning_effort(); per_turn_config.model_reasoning_summary = session_configuration.model_reasoning_summary; @@ -2355,8 +2356,7 @@ impl Session { let skills_outcome = Arc::new( self.services .skills_manager - .skills_for_cwd(&session_configuration.cwd, false) - .await, + .skills_for_config(&per_turn_config), ); let mut turn_context: TurnContext = Self::make_turn_context( Some(Arc::clone(&self.services.auth_manager)), diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index cdf18de942f..fd1bb576b40 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -2064,6 +2064,82 @@ async fn session_configuration_apply_preserves_split_file_system_policy_on_cwd_o ); } +#[cfg_attr(windows, ignore)] +#[tokio::test] +async fn new_default_turn_uses_config_aware_skills_for_role_overrides() { + let (session, _turn_context) = make_session_and_context().await; + let parent_config = session.get_config().await; + let codex_home = parent_config.codex_home.clone(); + let skill_dir = codex_home.join("skills").join("demo"); + std::fs::create_dir_all(&skill_dir).expect("create skill dir"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write( + &skill_path, + "---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n", + ) + .expect("write skill"); + + let parent_outcome = session + .services + .skills_manager + .skills_for_cwd(&parent_config.cwd, true) + .await; + let parent_skill = parent_outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + assert_eq!(parent_outcome.is_skill_enabled(parent_skill), true); + + let role_path = codex_home.join("skills-role.toml"); + std::fs::write( + &role_path, + format!( + r#"developer_instructions = "Stay focused" + +[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + ), + ) + .expect("write role config"); + + let mut child_config = (*parent_config).clone(); + child_config.agent_roles.insert( + "custom".to_string(), + crate::config::AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + crate::agent::role::apply_role_to_config(&mut child_config, Some("custom")) + .await + .expect("custom role should apply"); + + { + let mut state = session.state.lock().await; + state.session_configuration.original_config_do_not_use = Arc::new(child_config); + } + + let child_turn = session + .new_default_turn_with_sub_id("role-skill-turn".to_string()) + .await; + let child_skill = child_turn + .turn_skills + .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), + false + ); +} + #[tokio::test] async fn session_configuration_apply_rederives_legacy_file_system_policy_on_cwd_update() { let mut session_configuration = make_session_configuration_for_tests().await; diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 3a824d25fce..b0da181872f 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -31,6 +31,7 @@ pub struct SkillsManager { codex_home: PathBuf, plugins_manager: Arc, cache_by_cwd: RwLock>, + cache_by_config: RwLock>, } impl SkillsManager { @@ -43,6 +44,7 @@ impl SkillsManager { codex_home, plugins_manager, cache_by_cwd: RwLock::new(HashMap::new()), + cache_by_config: RwLock::new(HashMap::new()), }; if !bundled_skills_enabled { // The loader caches bundled skills under `skills/.system`. Clearing that directory is @@ -55,21 +57,25 @@ impl SkillsManager { } /// Load skills for an already-constructed [`Config`], avoiding any additional config-layer - /// loading. This also seeds the per-cwd cache for subsequent lookups. + /// loading. + /// + /// This path uses a cache keyed by the effective skill-relevant config state rather than just + /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen + /// to share a directory. pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { - let cwd = &config.cwd; - if let Some(outcome) = self.cached_outcome_for_cwd(cwd) { + let roots = self.skill_roots_for_config(config); + let cache_key = config_skills_cache_key(&roots, &config.config_layer_stack); + if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { return outcome; } - let roots = self.skill_roots_for_config(config); let outcome = finalize_skill_outcome(load_skills_from_roots(roots), &config.config_layer_stack); - let mut cache = match self.cache_by_cwd.write() { - Ok(cache) => cache, - Err(err) => err.into_inner(), - }; - cache.insert(cwd.to_path_buf(), outcome.clone()); + let mut cache = self + .cache_by_config + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(cache_key, outcome.clone()); outcome } @@ -163,21 +169,34 @@ impl SkillsManager { ); let outcome = load_skills_from_roots(roots); let outcome = finalize_skill_outcome(outcome, &config_layer_stack); - let mut cache = match self.cache_by_cwd.write() { - Ok(cache) => cache, - Err(err) => err.into_inner(), - }; + let mut cache = self + .cache_by_cwd + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); cache.insert(cwd.to_path_buf(), outcome.clone()); outcome } pub fn clear_cache(&self) { - let mut cache = match self.cache_by_cwd.write() { - Ok(cache) => cache, - Err(err) => err.into_inner(), + let cleared_cwd = { + let mut cache = self + .cache_by_cwd + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let cleared = cache.len(); + cache.clear(); + cleared }; - let cleared = cache.len(); - cache.clear(); + let cleared_config = { + let mut cache = self + .cache_by_config + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let cleared = cache.len(); + cache.clear(); + cleared + }; + let cleared = cleared_cwd + cleared_config; info!("skills cache cleared ({cleared} entries)"); } @@ -187,6 +206,22 @@ impl SkillsManager { Err(err) => err.into_inner().get(cwd).cloned(), } } + + fn cached_outcome_for_config( + &self, + cache_key: &ConfigSkillsCacheKey, + ) -> Option { + match self.cache_by_config.read() { + Ok(cache) => cache.get(cache_key).cloned(), + Err(err) => err.into_inner().get(cache_key).cloned(), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct ConfigSkillsCacheKey { + roots: Vec<(PathBuf, u8)>, + disabled_paths: Vec, } pub(crate) fn bundled_skills_enabled_from_stack( @@ -214,7 +249,6 @@ pub(crate) fn bundled_skills_enabled_from_stack( fn disabled_paths_from_stack( config_layer_stack: &crate::config_loader::ConfigLayerStack, ) -> HashSet { - let mut disabled = HashSet::new(); let mut configs = HashMap::new(); for layer in config_layer_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true) @@ -243,13 +277,36 @@ fn disabled_paths_from_stack( } } - for (path, enabled) in configs { - if !enabled { - disabled.insert(path); - } - } + configs + .into_iter() + .filter_map(|(path, enabled)| (!enabled).then_some(path)) + .collect() +} - disabled +fn config_skills_cache_key( + roots: &[SkillRoot], + config_layer_stack: &crate::config_loader::ConfigLayerStack, +) -> ConfigSkillsCacheKey { + let mut disabled_paths: Vec = disabled_paths_from_stack(config_layer_stack) + .into_iter() + .collect(); + disabled_paths.sort_unstable(); + + ConfigSkillsCacheKey { + roots: roots + .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) + }) + .collect(), + disabled_paths, + } } fn finalize_skill_outcome( diff --git a/codex-rs/core/src/skills/manager_tests.rs b/codex-rs/core/src/skills/manager_tests.rs index f9d6dc2c5fb..98ad9627bdc 100644 --- a/codex-rs/core/src/skills/manager_tests.rs +++ b/codex-rs/core/src/skills/manager_tests.rs @@ -36,7 +36,7 @@ fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() { } #[tokio::test] -async fn skills_for_config_seeds_cache_by_cwd() { +async fn skills_for_config_reuses_cache_for_same_effective_config() { let codex_home = tempfile::tempdir().expect("tempdir"); let cwd = tempfile::tempdir().expect("tempdir"); @@ -60,8 +60,8 @@ async fn skills_for_config_seeds_cache_by_cwd() { "expected skill-a to be discovered" ); - // Write a new skill after the first call; the second call should hit the cache and not - // reflect the new file. + // Write a new skill after the first call; the second call should reuse the config-aware cache + // entry because the effective skill config is unchanged. write_user_skill(&codex_home, "b", "skill-b", "from b"); let outcome2 = skills_manager.skills_for_config(&cfg); assert_eq!(outcome2.errors, outcome1.errors); @@ -354,3 +354,82 @@ enabled = false HashSet::from([skill_path]) ); } + +#[cfg_attr(windows, ignore)] +#[tokio::test] +async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let skill_dir = codex_home.path().join("skills").join("demo"); + fs::create_dir_all(&skill_dir).expect("create skill dir"); + let skill_path = skill_dir.join("SKILL.md"); + fs::write( + &skill_path, + "---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n", + ) + .expect("write skill"); + fs::write( + codex_home.path().join(crate::config::CONFIG_TOML_FILE), + format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + ), + ) + .expect("write config"); + + let parent_config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("load parent config"); + let role_path = codex_home.path().join("enable-role.toml"); + fs::write( + &role_path, + format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + ), + ) + .expect("write role config"); + let mut child_config = parent_config.clone(); + child_config.agent_roles.insert( + "custom".to_string(), + crate::config::AgentRoleConfig { + description: None, + config_file: Some(role_path), + nickname_candidates: None, + }, + ); + crate::agent::role::apply_role_to_config(&mut child_config, Some("custom")) + .await + .expect("custom role should apply"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true); + + let parent_outcome = skills_manager.skills_for_cwd(cwd.path(), true).await; + let parent_skill = parent_outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + assert_eq!(parent_outcome.is_skill_enabled(parent_skill), false); + + let child_outcome = skills_manager.skills_for_config(&child_config); + let child_skill = child_outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + assert_eq!(child_outcome.is_skill_enabled(child_skill), true); +}