diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 3b34d3a71..d8c93f73f 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -227,8 +227,10 @@ mod tests { use super::*; use crate::config::ConfigBuilder; use crate::config_loader::ConfigLayerStackOrdering; + use crate::skills::SkillsManager; use codex_protocol::openai_models::ReasoningEffort; use pretty_assertions::assert_eq; + use std::fs; use std::path::PathBuf; use tempfile::TempDir; @@ -470,6 +472,52 @@ writable_roots = ["./sandbox-root"] assert_eq!(session_flags_layer_count(&config), before_layers + 1); } + #[tokio::test] + async fn apply_role_skills_config_disables_skill_for_spawned_agent() { + let (home, mut config) = test_config_with_cli_overrides(Vec::new()).await; + let skill_dir = 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"); + let role_path = write_role_config( + &home, + "skills-role.toml", + &format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + ), + ) + .await; + config.agent_roles.insert( + "custom".to_string(), + AgentRoleConfig { + description: None, + config_file: Some(role_path), + }, + ); + + apply_role_to_config(&mut config, Some("custom")) + .await + .expect("custom role should apply"); + + let skills_manager = SkillsManager::new(home.path().to_path_buf()); + let outcome = skills_manager.skills_for_config(&config); + let skill = outcome + .skills + .iter() + .find(|skill| skill.name == "demo-skill") + .expect("demo skill should be discovered"); + + assert_eq!(outcome.is_skill_enabled(skill), false); + } + #[test] fn spawn_tool_spec_build_deduplicates_user_defined_built_in_roles() { let user_defined_roles = BTreeMap::from([ diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 7d2c6b89f..9f9a56c0a 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock; +use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use toml::Value as TomlValue; @@ -14,6 +15,7 @@ use tracing::warn; use crate::config::Config; use crate::config::types::SkillsConfig; use crate::config_loader::CloudRequirementsLoader; +use crate::config_loader::ConfigLayerStackOrdering; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::skills::SkillLoadOutcome; @@ -172,24 +174,31 @@ fn disabled_paths_from_stack( ) -> HashSet { let mut disabled = HashSet::new(); let mut configs = HashMap::new(); - // Skills config is user-layer only for now; higher-precedence layers are ignored. - let Some(user_layer) = config_layer_stack.get_user_layer() else { - return disabled; - }; - let Some(skills_value) = user_layer.config.get("skills") else { - return disabled; - }; - let skills: SkillsConfig = match skills_value.clone().try_into() { - Ok(skills) => skills, - Err(err) => { - warn!("invalid skills config: {err}"); - return disabled; + for layer in + config_layer_stack.get_layers(ConfigLayerStackOrdering::LowestPrecedenceFirst, true) + { + if !matches!( + layer.name, + ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags + ) { + continue; } - }; - for entry in skills.config { - let path = normalize_override_path(entry.path.as_path()); - configs.insert(path, entry.enabled); + let Some(skills_value) = layer.config.get("skills") else { + continue; + }; + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + continue; + } + }; + + for entry in skills.config { + let path = normalize_override_path(entry.path.as_path()); + configs.insert(path, entry.enabled); + } } for (path, enabled) in configs { @@ -220,6 +229,9 @@ mod tests { use super::*; use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; + use crate::config_loader::ConfigLayerEntry; + use crate::config_loader::ConfigLayerStack; + use crate::config_loader::ConfigRequirementsToml; use pretty_assertions::assert_eq; use std::fs; use std::path::PathBuf; @@ -402,4 +414,83 @@ mod tests { assert_eq!(first, second); } + + #[test] + fn disabled_paths_from_stack_allows_session_flags_to_override_user_layer() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + )) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + assert_eq!(disabled_paths_from_stack(&stack), HashSet::new()); + } + + #[test] + fn disabled_paths_from_stack_allows_session_flags_to_disable_user_enabled_skill() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = true +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + assert_eq!( + disabled_paths_from_stack(&stack), + HashSet::from([skill_path]) + ); + } }