Move skill config rule resolution into codex-config (#37466)

## What changed

- Define skill config selectors, ordered rules, and layer-stack parsing in `codex-config`.
- Resolve disabled skill paths from generic skill name/path pairs, so configuration logic no longer depends on `SkillMetadata`.
- Update plugin and host skill consumers to use the `codex-config` API and consolidate the rule tests with the skill config tests.

GitOrigin-RevId: 3fe67869708df2652befe28d58cdeba933256f84
This commit is contained in:
felixxia-oai
2026-08-07 16:20:38 +00:00
committed by copyberry
parent 3b366654f1
commit b3278e96cb
19 changed files with 231 additions and 274 deletions
-4
View File
@@ -2863,7 +2863,6 @@ dependencies = [
"codex-app-server-protocol",
"codex-config",
"codex-connectors",
"codex-core-skills",
"codex-exec-server",
"codex-exec-server-test-support",
"codex-git-utils",
@@ -2912,7 +2911,6 @@ name = "codex-core-skills"
version = "0.0.0"
dependencies = [
"codex-analytics",
"codex-config",
"codex-context-fragments",
"codex-exec-server",
"codex-otel",
@@ -2921,8 +2919,6 @@ dependencies = [
"codex-utils-path-uri",
"codex-utils-string",
"pretty_assertions",
"tempfile",
"toml 0.9.11+spec-1.1.0",
"tracing",
]
+4
View File
@@ -153,7 +153,11 @@ pub use requirements_layers::compose_requirements;
pub use shell_environment_policy::validate_shell_environment_policy_filter_config;
pub use skills_config::BundledSkillsConfig;
pub use skills_config::SkillConfig;
pub use skills_config::SkillConfigRule;
pub use skills_config::SkillConfigRuleSelector;
pub use skills_config::SkillConfigRules;
pub use skills_config::SkillsConfig;
pub use skills_config::skill_config_rules_from_stack;
pub use state::ConfigLayerEntry;
pub use state::ConfigLayerStack;
pub use state::ConfigLoadOptions;
+131
View File
@@ -1,9 +1,14 @@
//! Skill-related configuration types shared across crates.
use std::collections::HashSet;
use crate::ConfigLayerSource;
use crate::ConfigLayerStack;
use codex_utils_absolute_path::AbsolutePathBuf;
use schemars::JsonSchema;
use serde::Deserialize;
use serde::Serialize;
use tracing::warn;
const fn default_enabled() -> bool {
true
@@ -55,3 +60,129 @@ impl TryFrom<toml::Value> for SkillsConfig {
SkillsConfig::deserialize(value)
}
}
/// Selects configured skills by their name or canonical document path.
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub enum SkillConfigRuleSelector {
Name(String),
Path(AbsolutePathBuf),
}
/// Enables or disables every skill matched by its selector.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct SkillConfigRule {
pub selector: SkillConfigRuleSelector,
pub enabled: bool,
}
/// Ordered effective skill enablement rules from configuration layers.
#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)]
pub struct SkillConfigRules {
pub entries: Vec<SkillConfigRule>,
}
impl SkillConfigRules {
/// Applies rules in order; later rules override earlier rules for matching skills.
///
/// Explicit path selectors remain effective even when no current skill matches.
pub fn resolve_disabled_paths<'a>(
&self,
skills: impl IntoIterator<Item = (&'a str, &'a AbsolutePathBuf)> + Clone,
) -> HashSet<AbsolutePathBuf> {
let mut disabled_paths = HashSet::new();
for entry in &self.entries {
match &entry.selector {
SkillConfigRuleSelector::Path(path) => {
if entry.enabled {
disabled_paths.remove(path);
} else {
disabled_paths.insert(path.clone());
}
}
SkillConfigRuleSelector::Name(name) => {
for (skill_name, path) in skills.clone() {
if skill_name != name {
continue;
}
if entry.enabled {
disabled_paths.remove(path);
} else {
disabled_paths.insert(path.clone());
}
}
}
}
}
disabled_paths
}
}
/// Resolves skill enablement rules from user and session configuration layers.
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() {
if !matches!(
layer.name,
ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags
) {
continue;
}
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 Some(selector) = skill_config_rule_selector(&entry) else {
continue;
};
// Preserve layer order so a later name selector can override an earlier path selector
// for the same loaded skill.
entries.retain(|entry: &SkillConfigRule| entry.selector != selector);
entries.push(SkillConfigRule {
selector,
enabled: entry.enabled,
});
}
}
SkillConfigRules { entries }
}
fn skill_config_rule_selector(entry: &SkillConfig) -> Option<SkillConfigRuleSelector> {
match (entry.path.as_ref(), entry.name.as_deref()) {
(Some(path), None) => Some(SkillConfigRuleSelector::Path(
path.canonicalize().unwrap_or_else(|_| path.clone()),
)),
(None, Some(name)) => {
let name = name.trim();
if name.is_empty() {
warn!("ignoring empty skills.config name override");
None
} else {
Some(SkillConfigRuleSelector::Name(name.to_string()))
}
}
(Some(_), Some(_)) => {
warn!("ignoring skills.config entry with both path and name selectors");
None
}
(None, None) => {
warn!("ignoring skills.config entry without a path or name selector");
None
}
}
}
#[cfg(test)]
#[path = "skills_config_tests.rs"]
mod tests;
@@ -1,8 +1,8 @@
use codex_config::CONFIG_TOML_FILE;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerSource;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirementsToml;
use crate::CONFIG_TOML_FILE;
use crate::ConfigLayerEntry;
use crate::ConfigLayerSource;
use crate::ConfigLayerStack;
use crate::ConfigRequirementsToml;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use pretty_assertions::assert_eq;
@@ -145,3 +145,68 @@ enabled = true
}
);
}
#[test]
fn path_rule_disables_selected_path() {
let codex_home = TempDir::new().expect("temp dir");
let path = codex_home.path().join("disable-by-path/SKILL.md").abs();
let rules = SkillConfigRules {
entries: vec![SkillConfigRule {
selector: SkillConfigRuleSelector::Path(path.clone()),
enabled: false,
}],
};
assert_eq!(
rules.resolve_disabled_paths(std::iter::empty()),
[path].into_iter().collect()
);
}
#[test]
fn later_name_rule_reenables_path_disabled_skill() {
let codex_home = TempDir::new().expect("temp dir");
let path = codex_home.path().join("reenable-by-name/SKILL.md").abs();
let rules = SkillConfigRules {
entries: vec![
SkillConfigRule {
selector: SkillConfigRuleSelector::Path(path.clone()),
enabled: false,
},
SkillConfigRule {
selector: SkillConfigRuleSelector::Name("demo".to_string()),
enabled: true,
},
],
};
assert_eq!(
rules.resolve_disabled_paths([("demo", &path)]),
Default::default()
);
}
#[test]
fn later_path_rule_reenables_one_skill_disabled_by_name() {
let codex_home = TempDir::new().expect("temp dir");
let root = codex_home.path().join("reenable-by-path");
let first_path = root.join("first/SKILL.md").abs();
let second_path = root.join("second/SKILL.md").abs();
let rules = SkillConfigRules {
entries: vec![
SkillConfigRule {
selector: SkillConfigRuleSelector::Name("demo".to_string()),
enabled: false,
},
SkillConfigRule {
selector: SkillConfigRuleSelector::Path(first_path.clone()),
enabled: true,
},
],
};
assert_eq!(
rules.resolve_disabled_paths([("demo", &first_path), ("demo", &second_path)]),
[second_path].into_iter().collect()
);
}
-1
View File
@@ -18,7 +18,6 @@ codex-analytics = { workspace = true }
codex-app-server-protocol = { workspace = true }
codex-config = { workspace = true }
codex-connectors = { workspace = true }
codex-core-skills = { workspace = true }
codex-exec-server = { workspace = true }
codex-git-utils = { workspace = true }
codex-hooks = { workspace = true }
+1 -1
View File
@@ -1,7 +1,7 @@
use anyhow::Context;
use codex_app_server_protocol::PluginAvailability;
use codex_app_server_protocol::PluginInstallPolicy;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_config::skill_config_rules_from_stack;
use codex_login::CodexAuth;
use codex_plugin::PluginId;
use std::collections::HashSet;
+12 -5
View File
@@ -22,14 +22,14 @@ use crate::store::plugin_version_for_source;
use crate::store::plugin_version_for_source_with_fallback_manifest;
use codex_config::ConfigLayerStack;
use codex_config::HooksFile;
use codex_config::SkillConfigRules;
use codex_config::skill_config_rules_from_stack;
use codex_config::types::McpServerConfig;
use codex_config::types::McpServerTransportConfig;
use codex_config::types::PluginConfig;
use codex_config::types::PluginMcpServerConfig;
use codex_connectors::parse_plugin_app_config;
use codex_connectors::parse_plugin_app_config_value;
use codex_core_skills::config_rules::resolve_disabled_skill_paths;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_mcp::parse_agent_plugin_mcp_config;
use codex_mcp::parse_plugin_mcp_config;
use codex_plugin::AppDeclaration;
@@ -41,7 +41,6 @@ use codex_plugin::PluginIdError;
use codex_plugin::app_connector_ids_from_declarations;
use codex_protocol::auth::AuthMode;
use codex_protocol::protocol::Product;
use codex_skills::SkillConfigRules;
use codex_skills::SkillMetadata;
use codex_skills::SkillRootLoadRequest;
use codex_skills::SkillRootLoader;
@@ -976,12 +975,20 @@ impl PluginSkillInventory {
pub(crate) fn has_enabled_skills(&self, skill_config_rules: &SkillConfigRules) -> bool {
contains_enabled_skill(
&self.skills,
&resolve_disabled_skill_paths(&self.skills, skill_config_rules),
&skill_config_rules.resolve_disabled_paths(
self.skills
.iter()
.map(|skill| (skill.name.as_str(), &skill.path_to_skills_md)),
),
)
}
pub(crate) fn resolve(self, skill_config_rules: &SkillConfigRules) -> ResolvedPluginSkills {
let disabled_skill_paths = resolve_disabled_skill_paths(&self.skills, skill_config_rules);
let disabled_skill_paths = skill_config_rules.resolve_disabled_paths(
self.skills
.iter()
.map(|skill| (skill.name.as_str(), &skill.path_to_skills_md)),
);
ResolvedPluginSkills {
skills: self.skills,
disabled_skill_paths,
+3 -5
View File
@@ -70,12 +70,13 @@ use crate::tool_suggest_metadata::ToolSuggestMetadataCache;
use codex_analytics::AnalyticsEventsClient;
use codex_analytics::PluginInstallSource;
use codex_config::ConfigLayerStack;
use codex_config::SkillConfigRules;
use codex_config::clear_user_plugin;
use codex_config::set_user_plugin_enabled;
use codex_config::skill_config_rules_from_stack;
use codex_config::types::PluginConfig;
use codex_config::types::ToolSuggestDisabledTool;
use codex_config::types::ToolSuggestDiscoverableType;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_hooks::plugin_hook_declarations;
use codex_http_client::HttpClientFactory;
use codex_login::AuthManager;
@@ -91,7 +92,6 @@ use codex_plugin::prompt_safe_plugin_description;
use codex_protocol::auth::AuthMode;
use codex_protocol::protocol::HookEventName;
use codex_protocol::protocol::Product;
use codex_skills::SkillConfigRules;
use codex_skills::SkillMetadata;
use codex_skills::SkillRootLoader;
use codex_skills::SkillRootSnapshots;
@@ -2055,9 +2055,7 @@ impl PluginsManager {
plugin_id: plugin_id.as_key(),
remote_plugin_id: self.remote_plugin_id_for(&plugin_id),
};
let skill_config_rules = codex_core_skills::config_rules::skill_config_rules_from_stack(
&config.config_layer_stack,
);
let skill_config_rules = skill_config_rules_from_stack(&config.config_layer_stack);
let resolved_skills = load_plugin_skill_inventory(
&source_path,
&plugin_identity,
+1 -1
View File
@@ -41,6 +41,7 @@ use codex_config::McpServerOAuthConfig;
use codex_config::McpServerToolConfig;
use codex_config::RequirementSource;
use codex_config::RequirementsLayerEntry;
use codex_config::SkillConfigRules;
use codex_config::compose_requirements;
use codex_config::types::McpServerTransportConfig;
use codex_login::CodexAuth;
@@ -49,7 +50,6 @@ use codex_plugin::PluginId;
use codex_protocol::auth::AuthMode;
use codex_protocol::protocol::HookEventName;
use codex_protocol::protocol::Product;
use codex_skills::SkillConfigRules;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_plugins::SkillDiscoveryMode;
@@ -2,6 +2,7 @@ use std::collections::HashMap;
use std::sync::Arc;
use std::sync::RwLock;
use codex_config::SkillConfigRules;
use codex_plugin::AppDeclaration;
use codex_plugin::PluginCapabilitySummary;
use codex_plugin::PluginId;
@@ -10,7 +11,6 @@ use codex_plugin::app_connector_ids_from_declarations;
use codex_plugin::prompt_safe_plugin_description;
use codex_protocol::auth::AuthMode;
use codex_protocol::protocol::Product;
use codex_skills::SkillConfigRules;
use codex_skills::SkillRootLoader;
use codex_utils_plugins::PluginIdentity;
use codex_utils_plugins::PluginSkillRoot;
-3
View File
@@ -14,7 +14,6 @@ workspace = true
[dependencies]
codex-analytics = { workspace = true }
codex-config = { workspace = true }
codex-context-fragments = { workspace = true }
codex-exec-server = { workspace = true }
codex-otel = { workspace = true }
@@ -26,5 +25,3 @@ tracing = { workspace = true }
[dev-dependencies]
pretty_assertions = { workspace = true }
tempfile = { workspace = true }
toml = { workspace = true }
-76
View File
@@ -1,76 +0,0 @@
use codex_config::ConfigLayerSource;
use codex_config::ConfigLayerStack;
use codex_config::SkillConfig;
use codex_config::SkillsConfig;
pub use codex_skills::SkillConfigRule;
pub use codex_skills::SkillConfigRuleSelector;
pub use codex_skills::SkillConfigRules;
pub use codex_skills::resolve_disabled_skill_paths;
use tracing::warn;
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() {
if !matches!(
layer.name,
ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags
) {
continue;
}
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 Some(selector) = skill_config_rule_selector(&entry) else {
continue;
};
// Preserve layer order so a later name selector can override an earlier path selector
// for the same loaded skill.
entries.retain(|entry: &SkillConfigRule| entry.selector != selector);
entries.push(SkillConfigRule {
selector,
enabled: entry.enabled,
});
}
}
SkillConfigRules { entries }
}
fn skill_config_rule_selector(entry: &SkillConfig) -> Option<SkillConfigRuleSelector> {
match (entry.path.as_ref(), entry.name.as_deref()) {
(Some(path), None) => Some(SkillConfigRuleSelector::Path(
path.canonicalize().unwrap_or_else(|_| path.clone()),
)),
(None, Some(name)) => {
let name = name.trim();
if name.is_empty() {
warn!("ignoring empty skills.config name override");
None
} else {
Some(SkillConfigRuleSelector::Name(name.to_string()))
}
}
(Some(_), Some(_)) => {
warn!("ignoring skills.config entry with both path and name selectors");
None
}
(None, None) => {
warn!("ignoring skills.config entry without a path or name selector");
None
}
}
}
#[cfg(test)]
#[path = "config_rules_tests.rs"]
mod tests;
-1
View File
@@ -1,4 +1,3 @@
pub mod config_rules;
pub mod injection;
pub(crate) mod invocation_utils;
pub mod model;
-1
View File
@@ -16,7 +16,6 @@ use tokio::sync::Mutex;
pub use codex_core_skills::SkillError;
pub use codex_core_skills::SkillLoadOutcome;
pub use codex_core_skills::build_skill_name_counts;
pub use codex_core_skills::config_rules;
pub use codex_core_skills::detect_implicit_skill_invocation_for_command;
pub use codex_core_skills::injection;
pub use codex_core_skills::injection::SkillInjections;
+8 -4
View File
@@ -6,6 +6,8 @@ use std::sync::RwLock;
use std::sync::Weak;
use codex_config::ConfigLayerStack;
use codex_config::SkillConfigRules;
use codex_config::skill_config_rules_from_stack;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_protocol::protocol::Product;
@@ -21,9 +23,6 @@ use tracing::warn;
use codex_config::SkillsConfig;
use codex_core_skills::SkillLoadOutcome;
use codex_core_skills::config_rules::SkillConfigRules;
use codex_core_skills::config_rules::resolve_disabled_skill_paths;
use codex_core_skills::config_rules::skill_config_rules_from_stack;
use codex_skills::LoadedSkills;
use codex_skills::SkillLoadFuture;
use codex_skills::SkillRootLoadRequest;
@@ -294,7 +293,12 @@ impl HostSkillsService {
input.plugin_skill_snapshots.as_ref(),
)
.await;
let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules);
let disabled_paths = skill_config_rules.resolve_disabled_paths(
outcome
.skills
.iter()
.map(|skill| (skill.name.as_str(), &skill.path_to_skills_md)),
);
outcome.with_disabled_paths(disabled_paths)
}
-5
View File
@@ -5,7 +5,6 @@ mod mentions;
mod model;
mod name_counts;
mod parser;
mod policy;
mod selection;
pub use interface::SkillInterfaceAssetPolicy;
@@ -30,9 +29,6 @@ pub use mentions::normalize_skill_path;
pub use mentions::plugin_config_name_from_path;
pub use mentions::tool_kind_for_path;
pub use model::EnvironmentSkillMetadata;
pub use model::SkillConfigRule;
pub use model::SkillConfigRuleSelector;
pub use model::SkillConfigRules;
pub use model::SkillDependencies;
pub use model::SkillInterface;
pub use model::SkillMetadata;
@@ -42,7 +38,6 @@ pub use name_counts::build_skill_name_counts;
pub use parser::ParsedSkillFrontmatter;
pub use parser::SkillParseError;
pub use parser::parse_skill_frontmatter_metadata;
pub use policy::resolve_disabled_skill_paths;
pub use selection::ExplicitSkillLookup;
pub use selection::collect_explicit_skill_mentions;
-17
View File
@@ -92,23 +92,6 @@ pub struct SkillToolDependency {
pub url: Option<String>,
}
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
pub enum SkillConfigRuleSelector {
Name(String),
Path(AbsolutePathBuf),
}
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct SkillConfigRule {
pub selector: SkillConfigRuleSelector,
pub enabled: bool,
}
#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)]
pub struct SkillConfigRules {
pub entries: Vec<SkillConfigRule>,
}
fn matches_product_restriction(
policy: Option<&SkillPolicy>,
restriction_product: Option<Product>,
-48
View File
@@ -1,48 +0,0 @@
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;
-96
View File
@@ -1,96 +0,0 @@
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()
);
}