openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move skill policy resolution into codex-skills (#36913)

Browse Source 
## What changed

- Move `resolve_disabled_skill_paths` into `codex-skills` and expose it from the crate.
- Re-export the resolver from `core-skills` to preserve existing callers.
- Split config-layer and policy-resolution coverage into focused unit tests, including ordered path and name overrides.

GitOrigin-RevId: 6528ff99285925132bf80eb33eafb1b62118e033
This commit is contained in:
felixxia-oai
2026-08-04 15:37:42 +00:00
committed by copyberry
parent 6a828ca26f
commit 2999b8e831
6 changed files with 298 additions and 213 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core-skills/src/config_rules.rs
+5 -39
View File
@@ -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<AbsolutePathBuf> {
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<SkillConfigRuleSelector> {
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<SkillConfigRuleSele
}
}
}
#[cfg(test)]
#[path = "config_rules_tests.rs"]
mod tests;
codex-rs/core-skills/src/config_rules_tests.rs
+147
View File
@@ -0,0 +1,147 @@
use codex_config::CONFIG_TOML_FILE;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerSource;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirementsToml;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
use super::SkillConfigRule;
use super::SkillConfigRuleSelector;
use super::SkillConfigRules;
use super::skill_config_rules_from_stack;
fn user_layer(codex_home: &TempDir, config: &str) -> 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,
},
],
}
);
}
codex-rs/core-skills/src/service_tests.rs
-174
View File
@@ -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() {
codex-rs/skills/src/lib.rs
+2
View File
@@ -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;
codex-rs/skills/src/policy.rs
+48
View File
@@ -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<AbsolutePathBuf> {
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;
codex-rs/skills/src/policy_tests.rs
+96
View File
@@ -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()
);
}