feat: skill disable respect config layer (#13027)
This commit is contained in:
jif-oai
GitHub
parent
2b38b4e03b
commit
d33f4b54ac
2 changed files with 155 additions and 16 deletions
|
|
@ -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([
|
||||
|
|
|
|||
|
|
@ -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<PathBuf> {
|
||||
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])
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue