diff --git a/codex-rs/core-skills/src/config_rules.rs b/codex-rs/core-skills/src/config_rules.rs index fe410ae61b..064dd0f654 100644 --- a/codex-rs/core-skills/src/config_rules.rs +++ b/codex-rs/core-skills/src/config_rules.rs @@ -1,5 +1,3 @@ -use std::collections::HashSet; - use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::SkillConfig; @@ -7,11 +5,9 @@ use codex_config::SkillsConfig; pub use codex_skills::SkillConfigRule; pub use codex_skills::SkillConfigRuleSelector; pub use codex_skills::SkillConfigRules; -use codex_utils_absolute_path::AbsolutePathBuf; +pub use codex_skills::resolve_disabled_skill_paths; use tracing::warn; -use crate::SkillMetadata; - pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> SkillConfigRules { let mut entries = Vec::new(); for layer in config_layer_stack.all_layers_low_to_high() { @@ -50,40 +46,6 @@ pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> S SkillConfigRules { entries } } -pub fn resolve_disabled_skill_paths( - skills: &[SkillMetadata], - rules: &SkillConfigRules, -) -> HashSet { - let mut disabled_paths = HashSet::new(); - - for entry in &rules.entries { - match &entry.selector { - SkillConfigRuleSelector::Path(path) => { - if entry.enabled { - disabled_paths.remove(path); - } else { - disabled_paths.insert(path.clone()); - } - } - SkillConfigRuleSelector::Name(name) => { - for path in skills - .iter() - .filter(|skill| skill.name == *name) - .map(|skill| skill.path_to_skills_md.clone()) - { - if entry.enabled { - disabled_paths.remove(&path); - } else { - disabled_paths.insert(path); - } - } - } - } - } - - disabled_paths -} - fn skill_config_rule_selector(entry: &SkillConfig) -> Option { match (entry.path.as_ref(), entry.name.as_deref()) { (Some(path), None) => Some(SkillConfigRuleSelector::Path( @@ -108,3 +70,7 @@ fn skill_config_rule_selector(entry: &SkillConfig) -> Option ConfigLayerEntry { + let config_path = AbsolutePathBuf::try_from(codex_home.path().join(CONFIG_TOML_FILE)) + .expect("absolute config path"); + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: config_path, + profile: None, + }, + toml::from_str(config).expect("valid user config"), + ) +} + +fn stack(codex_home: &TempDir, user: &str, session: &str) -> ConfigLayerStack { + ConfigLayerStack::new( + vec![ + user_layer(codex_home, user), + ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(session).expect("valid session config"), + ), + ], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config stack") +} + +fn path_toggle_config(path: &std::path::Path, enabled: bool) -> String { + format!( + r#"[[skills.config]] +path = "{}" +enabled = {enabled} +"#, + path.display() + ) +} + +#[cfg_attr(windows, ignore)] +#[test] +fn session_flags_can_reenable_user_disabled_path() { + let codex_home = TempDir::new().expect("temp dir"); + let skill_path = codex_home.path().join("skills/demo/SKILL.md"); + + assert_eq!( + skill_config_rules_from_stack(&stack( + &codex_home, + &path_toggle_config(&skill_path, /*enabled*/ false), + &path_toggle_config(&skill_path, /*enabled*/ true), + )), + SkillConfigRules { + entries: vec![SkillConfigRule { + selector: SkillConfigRuleSelector::Path(skill_path.abs()), + enabled: true, + }], + } + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn session_flags_can_disable_user_enabled_path() { + let codex_home = TempDir::new().expect("temp dir"); + let skill_path = codex_home.path().join("skills/demo/SKILL.md"); + + assert_eq!( + skill_config_rules_from_stack(&stack( + &codex_home, + &path_toggle_config(&skill_path, /*enabled*/ true), + &path_toggle_config(&skill_path, /*enabled*/ false), + )), + SkillConfigRules { + entries: vec![SkillConfigRule { + selector: SkillConfigRuleSelector::Path(skill_path.abs()), + enabled: false, + }], + } + ); +} + +#[test] +fn preserves_name_selectors() { + let codex_home = TempDir::new().expect("temp dir"); + + assert_eq!( + skill_config_rules_from_stack(&stack( + &codex_home, + r#" +[[skills.config]] +name = "github:yeet" +enabled = false +"#, + "", + )), + SkillConfigRules { + entries: vec![SkillConfigRule { + selector: SkillConfigRuleSelector::Name("github:yeet".to_string()), + enabled: false, + }], + } + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn preserves_order_across_path_and_name_selectors() { + let codex_home = TempDir::new().expect("temp dir"); + let skill_path = codex_home.path().join("skills/demo/SKILL.md"); + + assert_eq!( + skill_config_rules_from_stack(&stack( + &codex_home, + &path_toggle_config(&skill_path, /*enabled*/ false), + r#" +[[skills.config]] +name = "github:yeet" +enabled = true +"#, + )), + SkillConfigRules { + entries: vec![ + SkillConfigRule { + selector: SkillConfigRuleSelector::Path(skill_path.abs()), + enabled: false, + }, + SkillConfigRule { + selector: SkillConfigRuleSelector::Name("github:yeet".to_string()), + enabled: true, + }, + ], + } + ); +} diff --git a/codex-rs/core-skills/src/service_tests.rs b/codex-rs/core-skills/src/service_tests.rs index 1a27e8034c..3f89ce517a 100644 --- a/codex-rs/core-skills/src/service_tests.rs +++ b/codex-rs/core-skills/src/service_tests.rs @@ -1,7 +1,4 @@ use super::*; -use crate::SkillMetadata; -use crate::config_rules::resolve_disabled_skill_paths; -use crate::config_rules::skill_config_rules_from_stack; use codex_config::CONFIG_TOML_FILE; use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; @@ -81,36 +78,6 @@ fn plugin_skill_root_for_skill_path( } } -fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { - SkillMetadata { - name: name.to_string(), - description: "test".to_string(), - short_description: None, - interface: None, - dependencies: None, - policy: None, - path_to_skills_md: path - .abs() - .canonicalize() - .expect("skill path should canonicalize"), - scope: SkillScope::User, - plugin_id: None, - remote_plugin_id: None, - } -} - -fn write_demo_skill(tempdir: &TempDir) -> PathBuf { - let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); - fs::create_dir_all(skill_path.parent().expect("skill path should have parent")) - .expect("create skill dir"); - fs::write( - &skill_path, - "---\nname: demo-skill\ndescription: demo description\n---\n\n# Body\n", - ) - .expect("write skill"); - skill_path -} - fn user_config_layer(codex_home: &TempDir, config_toml: &str) -> ConfigLayerEntry { let config_path = AbsolutePathBuf::try_from(codex_home.path().join(CONFIG_TOML_FILE)) .expect("user config path should be absolute"); @@ -686,147 +653,6 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() { ); } -#[cfg_attr(windows, ignore)] -#[test] -fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() { - let tempdir = tempfile::tempdir().expect("tempdir"); - let skill_path = write_demo_skill(&tempdir); - let skill = test_skill("demo-skill", skill_path.clone()); - 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, - profile: None, - }, - toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) - .expect("user layer toml"), - ); - let session_layer = ConfigLayerEntry::new( - ConfigLayerSource::SessionFlags, - toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ true)) - .expect("session layer toml"), - ); - let stack = ConfigLayerStack::new( - vec![user_layer, session_layer], - Default::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"); - - let skill_config_rules = skill_config_rules_from_stack(&stack); - assert_eq!( - resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::new() - ); -} - -#[cfg_attr(windows, ignore)] -#[test] -fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill() { - let tempdir = tempfile::tempdir().expect("tempdir"); - let skill_path = write_demo_skill(&tempdir); - let skill = test_skill("demo-skill", skill_path.clone()); - 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, - profile: None, - }, - toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ true)) - .expect("user layer toml"), - ); - let session_layer = ConfigLayerEntry::new( - ConfigLayerSource::SessionFlags, - toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) - .expect("session layer toml"), - ); - let stack = ConfigLayerStack::new( - vec![user_layer, session_layer], - Default::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"); - - let skill_config_rules = skill_config_rules_from_stack(&stack); - assert_eq!( - resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) - ); -} - -#[cfg_attr(windows, ignore)] -#[test] -fn disabled_paths_for_skills_disables_matching_name_selectors() { - let tempdir = tempfile::tempdir().expect("tempdir"); - let skill_path = write_demo_skill(&tempdir); - let skill = test_skill("github:yeet", skill_path.clone()); - 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, - profile: None, - }, - toml::from_str(&name_toggle_config("github:yeet", /*enabled*/ false)) - .expect("user layer toml"), - ); - let stack = ConfigLayerStack::new( - vec![user_layer], - Default::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"); - - let skill_config_rules = skill_config_rules_from_stack(&stack); - assert_eq!( - resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::from([skill_path - .abs() - .canonicalize() - .expect("skill path should canonicalize")]) - ); -} - -#[cfg_attr(windows, ignore)] -#[test] -fn disabled_paths_for_skills_allows_name_selector_to_override_path_selector() { - let tempdir = tempfile::tempdir().expect("tempdir"); - let skill_path = write_demo_skill(&tempdir); - let skill = test_skill("github:yeet", skill_path.clone()); - 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, - profile: None, - }, - toml::from_str(&path_toggle_config(&skill_path, /*enabled*/ false)) - .expect("user layer toml"), - ); - let session_layer = ConfigLayerEntry::new( - ConfigLayerSource::SessionFlags, - toml::from_str(&name_toggle_config("github:yeet", /*enabled*/ true)) - .expect("session layer toml"), - ); - let stack = ConfigLayerStack::new( - vec![user_layer, session_layer], - Default::default(), - ConfigRequirementsToml::default(), - ) - .expect("valid config layer stack"); - - let skill_config_rules = skill_config_rules_from_stack(&stack); - assert_eq!( - resolve_disabled_skill_paths(&[skill], &skill_config_rules), - HashSet::new() - ); -} - #[cfg_attr(windows, ignore)] #[tokio::test] async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index cc3cc948de..925073f00b 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -1,6 +1,7 @@ mod interface; mod model; mod parser; +mod policy; pub use interface::SkillInterfaceAssetPolicy; pub use interface::SkillInterfaceFile; @@ -17,6 +18,7 @@ pub use model::SkillToolDependency; pub use parser::ParsedSkillFrontmatter; pub use parser::SkillParseError; pub use parser::parse_skill_frontmatter_metadata; +pub use policy::resolve_disabled_skill_paths; use codex_utils_absolute_path::AbsolutePathBuf; use include_dir::Dir; diff --git a/codex-rs/skills/src/policy.rs b/codex-rs/skills/src/policy.rs new file mode 100644 index 0000000000..e7972a4588 --- /dev/null +++ b/codex-rs/skills/src/policy.rs @@ -0,0 +1,48 @@ +use std::collections::HashSet; + +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::SkillConfigRuleSelector; +use crate::SkillConfigRules; +use crate::SkillMetadata; + +/// Resolves disabled `SKILL.md` paths by applying configuration rules in order. +/// +/// Later rules override earlier rules for every skill path they select. +pub fn resolve_disabled_skill_paths( + skills: &[SkillMetadata], + rules: &SkillConfigRules, +) -> HashSet { + let mut disabled_paths = HashSet::new(); + + for entry in &rules.entries { + match &entry.selector { + SkillConfigRuleSelector::Path(path) => { + if entry.enabled { + disabled_paths.remove(path); + } else { + disabled_paths.insert(path.clone()); + } + } + SkillConfigRuleSelector::Name(name) => { + for path in skills + .iter() + .filter(|skill| skill.name == *name) + .map(|skill| skill.path_to_skills_md.clone()) + { + if entry.enabled { + disabled_paths.remove(&path); + } else { + disabled_paths.insert(path); + } + } + } + } + } + + disabled_paths +} + +#[cfg(test)] +#[path = "policy_tests.rs"] +mod tests; diff --git a/codex-rs/skills/src/policy_tests.rs b/codex-rs/skills/src/policy_tests.rs new file mode 100644 index 0000000000..da120210cd --- /dev/null +++ b/codex-rs/skills/src/policy_tests.rs @@ -0,0 +1,96 @@ +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; + +use super::resolve_disabled_skill_paths; +use crate::SkillConfigRule; +use crate::SkillConfigRuleSelector; +use crate::SkillConfigRules; +use crate::SkillMetadata; + +fn skill(name: &str, path: AbsolutePathBuf) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: "test".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: path, + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + } +} + +fn test_root() -> AbsolutePathBuf { + AbsolutePathBuf::try_from(std::env::current_dir().expect("current directory")) + .expect("absolute current directory") +} + +#[test] +fn path_rule_disables_selected_path() { + let path = test_root().join("disable-by-path/SKILL.md"); + let rules = SkillConfigRules { + entries: vec![SkillConfigRule { + selector: SkillConfigRuleSelector::Path(path.clone()), + enabled: false, + }], + }; + + assert_eq!( + resolve_disabled_skill_paths(&[], &rules), + [path].into_iter().collect() + ); +} + +#[test] +fn later_name_rule_reenables_path_disabled_skill() { + let path = test_root().join("reenable-by-name/SKILL.md"); + let skills = vec![skill("demo", path.clone())]; + let rules = SkillConfigRules { + entries: vec![ + SkillConfigRule { + selector: SkillConfigRuleSelector::Path(path), + enabled: false, + }, + SkillConfigRule { + selector: SkillConfigRuleSelector::Name("demo".to_string()), + enabled: true, + }, + ], + }; + + assert_eq!( + resolve_disabled_skill_paths(&skills, &rules), + Default::default() + ); +} + +#[test] +fn later_path_rule_reenables_one_skill_disabled_by_name() { + let root = test_root().join("reenable-by-path"); + let first_path = root.join("first/SKILL.md"); + let second_path = root.join("second/SKILL.md"); + let skills = vec![ + skill("demo", first_path.clone()), + skill("demo", second_path.clone()), + ]; + let rules = SkillConfigRules { + entries: vec![ + SkillConfigRule { + selector: SkillConfigRuleSelector::Name("demo".to_string()), + enabled: false, + }, + SkillConfigRule { + selector: SkillConfigRuleSelector::Path(first_path), + enabled: true, + }, + ], + }; + + assert_eq!( + resolve_disabled_skill_paths(&skills, &rules), + [second_path].into_iter().collect() + ); +}