mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Support enable/disable skill via config/api. (#9328)
In config.toml: ``` [[skills.config]] path = "/Users/xl/.codex/skills/my_skill/SKILL.md" enabled = false ``` API: skills/list, skills/config/write
This commit is contained in:
committed by
GitHub
Unverified
parent
246f506551
commit
ad8bf59cbf
@@ -251,7 +251,8 @@ impl Codex {
|
||||
);
|
||||
}
|
||||
|
||||
let user_instructions = get_user_instructions(&config, Some(&loaded_skills.skills)).await;
|
||||
let enabled_skills = loaded_skills.enabled_skills();
|
||||
let user_instructions = get_user_instructions(&config, Some(&enabled_skills)).await;
|
||||
|
||||
let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack)
|
||||
.await
|
||||
@@ -2264,7 +2265,8 @@ mod handlers {
|
||||
for cwd in cwds {
|
||||
let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await;
|
||||
let errors = super::errors_to_info(&outcome.errors);
|
||||
let skills_metadata = super::skills_to_info(&outcome.skills);
|
||||
let enabled_skills = outcome.enabled_skills();
|
||||
let skills_metadata = super::skills_to_info(&enabled_skills);
|
||||
skills.push(SkillsListEntry {
|
||||
cwd,
|
||||
skills: skills_metadata,
|
||||
|
||||
@@ -9,6 +9,7 @@ use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
use tokio::task;
|
||||
use toml_edit::ArrayOfTables;
|
||||
use toml_edit::DocumentMut;
|
||||
use toml_edit::Item as TomlItem;
|
||||
use toml_edit::Table as TomlTable;
|
||||
@@ -36,6 +37,8 @@ pub enum ConfigEdit {
|
||||
RecordModelMigrationSeen { from: String, to: String },
|
||||
/// Replace the entire `[mcp_servers]` table.
|
||||
ReplaceMcpServers(BTreeMap<String, McpServerConfig>),
|
||||
/// Set or clear a skill config entry under `[[skills.config]]`.
|
||||
SetSkillConfig { path: PathBuf, enabled: bool },
|
||||
/// Set trust_level under `[projects."<path>"]`,
|
||||
/// migrating inline tables to explicit tables.
|
||||
SetProjectTrustLevel { path: PathBuf, level: TrustLevel },
|
||||
@@ -298,6 +301,9 @@ impl ConfigDocument {
|
||||
value(*acknowledged),
|
||||
)),
|
||||
ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)),
|
||||
ConfigEdit::SetSkillConfig { path, enabled } => {
|
||||
Ok(self.set_skill_config(path.as_path(), *enabled))
|
||||
}
|
||||
ConfigEdit::SetPath { segments, value } => Ok(self.insert(segments, value.clone())),
|
||||
ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)),
|
||||
ConfigEdit::SetProjectTrustLevel { path, level } => {
|
||||
@@ -387,6 +393,113 @@ impl ConfigDocument {
|
||||
true
|
||||
}
|
||||
|
||||
fn set_skill_config(&mut self, path: &Path, enabled: bool) -> bool {
|
||||
let normalized_path = normalize_skill_config_path(path);
|
||||
let mut remove_skills_table = false;
|
||||
let mut mutated = false;
|
||||
|
||||
{
|
||||
let root = self.doc.as_table_mut();
|
||||
let skills_item = match root.get_mut("skills") {
|
||||
Some(item) => item,
|
||||
None => {
|
||||
if enabled {
|
||||
return false;
|
||||
}
|
||||
root.insert(
|
||||
"skills",
|
||||
TomlItem::Table(document_helpers::new_implicit_table()),
|
||||
);
|
||||
let Some(item) = root.get_mut("skills") else {
|
||||
return false;
|
||||
};
|
||||
item
|
||||
}
|
||||
};
|
||||
|
||||
if document_helpers::ensure_table_for_write(skills_item).is_none() {
|
||||
if enabled {
|
||||
return false;
|
||||
}
|
||||
*skills_item = TomlItem::Table(document_helpers::new_implicit_table());
|
||||
}
|
||||
let Some(skills_table) = skills_item.as_table_mut() else {
|
||||
return false;
|
||||
};
|
||||
|
||||
let config_item = match skills_table.get_mut("config") {
|
||||
Some(item) => item,
|
||||
None => {
|
||||
if enabled {
|
||||
return false;
|
||||
}
|
||||
skills_table.insert("config", TomlItem::ArrayOfTables(ArrayOfTables::new()));
|
||||
let Some(item) = skills_table.get_mut("config") else {
|
||||
return false;
|
||||
};
|
||||
item
|
||||
}
|
||||
};
|
||||
|
||||
if !matches!(config_item, TomlItem::ArrayOfTables(_)) {
|
||||
if enabled {
|
||||
return false;
|
||||
}
|
||||
*config_item = TomlItem::ArrayOfTables(ArrayOfTables::new());
|
||||
}
|
||||
|
||||
let TomlItem::ArrayOfTables(overrides) = config_item else {
|
||||
return false;
|
||||
};
|
||||
|
||||
let existing_index = overrides.iter().enumerate().find_map(|(idx, table)| {
|
||||
table
|
||||
.get("path")
|
||||
.and_then(|item| item.as_str())
|
||||
.map(Path::new)
|
||||
.map(normalize_skill_config_path)
|
||||
.filter(|value| *value == normalized_path)
|
||||
.map(|_| idx)
|
||||
});
|
||||
|
||||
if enabled {
|
||||
if let Some(index) = existing_index {
|
||||
overrides.remove(index);
|
||||
mutated = true;
|
||||
if overrides.is_empty() {
|
||||
skills_table.remove("config");
|
||||
if skills_table.is_empty() {
|
||||
remove_skills_table = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
} else if let Some(index) = existing_index {
|
||||
for (idx, table) in overrides.iter_mut().enumerate() {
|
||||
if idx == index {
|
||||
table["path"] = value(normalized_path);
|
||||
table["enabled"] = value(false);
|
||||
mutated = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
} else {
|
||||
let mut entry = TomlTable::new();
|
||||
entry.set_implicit(false);
|
||||
entry["path"] = value(normalized_path);
|
||||
entry["enabled"] = value(false);
|
||||
overrides.push(entry);
|
||||
mutated = true;
|
||||
}
|
||||
}
|
||||
|
||||
if remove_skills_table {
|
||||
let root = self.doc.as_table_mut();
|
||||
root.remove("skills");
|
||||
}
|
||||
|
||||
mutated
|
||||
}
|
||||
|
||||
fn scoped_segments(&self, scope: Scope, segments: &[&str]) -> Vec<String> {
|
||||
let resolved: Vec<String> = segments
|
||||
.iter()
|
||||
@@ -494,6 +607,13 @@ impl ConfigDocument {
|
||||
}
|
||||
}
|
||||
|
||||
fn normalize_skill_config_path(path: &Path) -> String {
|
||||
dunce::canonicalize(path)
|
||||
.unwrap_or_else(|_| path.to_path_buf())
|
||||
.to_string_lossy()
|
||||
.to_string()
|
||||
}
|
||||
|
||||
/// Persist edits using a blocking strategy.
|
||||
pub fn apply_blocking(
|
||||
codex_home: &Path,
|
||||
@@ -737,6 +857,54 @@ model_reasoning_effort = "high"
|
||||
assert_eq!(contents, "enabled = true\n");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_skill_config_writes_disabled_entry() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
let codex_home = tmp.path();
|
||||
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.with_edits([ConfigEdit::SetSkillConfig {
|
||||
path: PathBuf::from("/tmp/skills/demo/SKILL.md"),
|
||||
enabled: false,
|
||||
}])
|
||||
.apply_blocking()
|
||||
.expect("persist");
|
||||
|
||||
let contents =
|
||||
std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config");
|
||||
let expected = r#"[[skills.config]]
|
||||
path = "/tmp/skills/demo/SKILL.md"
|
||||
enabled = false
|
||||
"#;
|
||||
assert_eq!(contents, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn set_skill_config_removes_entry_when_enabled() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
let codex_home = tmp.path();
|
||||
std::fs::write(
|
||||
codex_home.join(CONFIG_TOML_FILE),
|
||||
r#"[[skills.config]]
|
||||
path = "/tmp/skills/demo/SKILL.md"
|
||||
enabled = false
|
||||
"#,
|
||||
)
|
||||
.expect("seed config");
|
||||
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.with_edits([ConfigEdit::SetSkillConfig {
|
||||
path: PathBuf::from("/tmp/skills/demo/SKILL.md"),
|
||||
enabled: true,
|
||||
}])
|
||||
.apply_blocking()
|
||||
.expect("persist");
|
||||
|
||||
let contents =
|
||||
std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config");
|
||||
assert_eq!(contents, "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn blocking_set_model_preserves_inline_table_contents() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
|
||||
@@ -13,6 +13,7 @@ use crate::config::types::SandboxWorkspaceWrite;
|
||||
use crate::config::types::ScrollInputMode;
|
||||
use crate::config::types::ShellEnvironmentPolicy;
|
||||
use crate::config::types::ShellEnvironmentPolicyToml;
|
||||
use crate::config::types::SkillsConfig;
|
||||
use crate::config::types::Tui;
|
||||
use crate::config::types::UriBasedFileOpener;
|
||||
use crate::config_loader::ConfigLayerStack;
|
||||
@@ -911,6 +912,9 @@ pub struct ConfigToml {
|
||||
/// Nested tools section for feature toggles
|
||||
pub tools: Option<ToolsToml>,
|
||||
|
||||
/// User-level skill config entries keyed by SKILL.md path.
|
||||
pub skills: Option<SkillsConfig>,
|
||||
|
||||
/// Centralized feature flags (new). Prefer this over individual toggles.
|
||||
#[serde(default)]
|
||||
// Injects known feature keys into the schema and forbids unknown keys.
|
||||
|
||||
@@ -600,6 +600,20 @@ impl Notice {
|
||||
pub(crate) const TABLE_KEY: &'static str = "notice";
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct SkillConfig {
|
||||
pub path: AbsolutePathBuf,
|
||||
pub enabled: bool,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct SkillsConfig {
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub config: Vec<SkillConfig>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct SandboxWorkspaceWrite {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use std::collections::HashSet;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use crate::skills::SkillLoadOutcome;
|
||||
use crate::skills::SkillMetadata;
|
||||
@@ -25,7 +26,8 @@ pub(crate) async fn build_skill_injections(
|
||||
return SkillInjections::default();
|
||||
};
|
||||
|
||||
let mentioned_skills = collect_explicit_skill_mentions(inputs, &outcome.skills);
|
||||
let mentioned_skills =
|
||||
collect_explicit_skill_mentions(inputs, &outcome.skills, &outcome.disabled_paths);
|
||||
if mentioned_skills.is_empty() {
|
||||
return SkillInjections::default();
|
||||
}
|
||||
@@ -46,9 +48,9 @@ pub(crate) async fn build_skill_injections(
|
||||
}
|
||||
Err(err) => {
|
||||
let message = format!(
|
||||
"Failed to load skill {} at {}: {err:#}",
|
||||
skill.name,
|
||||
skill.path.display()
|
||||
"Failed to load skill {name} at {path}: {err:#}",
|
||||
name = skill.name,
|
||||
path = skill.path.display()
|
||||
);
|
||||
result.warnings.push(message);
|
||||
}
|
||||
@@ -61,6 +63,7 @@ pub(crate) async fn build_skill_injections(
|
||||
fn collect_explicit_skill_mentions(
|
||||
inputs: &[UserInput],
|
||||
skills: &[SkillMetadata],
|
||||
disabled_paths: &HashSet<PathBuf>,
|
||||
) -> Vec<SkillMetadata> {
|
||||
let mut selected: Vec<SkillMetadata> = Vec::new();
|
||||
let mut seen: HashSet<String> = HashSet::new();
|
||||
@@ -69,6 +72,7 @@ fn collect_explicit_skill_mentions(
|
||||
if let UserInput::Skill { name, path } = input
|
||||
&& seen.insert(name.clone())
|
||||
&& let Some(skill) = skills.iter().find(|s| s.name == *name && s.path == *path)
|
||||
&& !disabled_paths.contains(&skill.path)
|
||||
{
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
|
||||
+135
-200
@@ -105,13 +105,13 @@ where
|
||||
discover_skills_under_root(&root.path, root.scope, &mut outcome);
|
||||
}
|
||||
|
||||
let mut seen: HashSet<String> = HashSet::new();
|
||||
let mut seen: HashSet<PathBuf> = HashSet::new();
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| seen.insert(skill.name.clone()));
|
||||
.retain(|skill| seen.insert(skill.path.clone()));
|
||||
|
||||
fn scope_rank(scope: SkillScope) -> u8 {
|
||||
// Higher-priority scopes first (matches dedupe priority order).
|
||||
// Higher-priority scopes first (matches root scan order for dedupe).
|
||||
match scope {
|
||||
SkillScope::Repo => 0,
|
||||
SkillScope::User => 1,
|
||||
@@ -445,10 +445,23 @@ fn resolve_asset_path(
|
||||
return None;
|
||||
}
|
||||
|
||||
let mut components = path.components().peekable();
|
||||
while matches!(components.peek(), Some(Component::CurDir)) {
|
||||
components.next();
|
||||
let mut normalized = PathBuf::new();
|
||||
for component in path.components() {
|
||||
match component {
|
||||
Component::CurDir => {}
|
||||
Component::Normal(component) => normalized.push(component),
|
||||
Component::ParentDir => {
|
||||
tracing::warn!("ignoring {field}: icon path must not contain '..'");
|
||||
return None;
|
||||
}
|
||||
_ => {
|
||||
tracing::warn!("ignoring {field}: icon path must be under assets/");
|
||||
return None;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let mut components = normalized.components();
|
||||
match components.next() {
|
||||
Some(Component::Normal(component)) if component == "assets" => {}
|
||||
_ => {
|
||||
@@ -456,12 +469,8 @@ fn resolve_asset_path(
|
||||
return None;
|
||||
}
|
||||
}
|
||||
if components.any(|component| matches!(component, Component::ParentDir)) {
|
||||
tracing::warn!("ignoring {field}: icon path must not contain '..'");
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(skill_dir.join(path))
|
||||
Some(skill_dir.join(normalized))
|
||||
}
|
||||
|
||||
fn sanitize_single_line(raw: &str) -> String {
|
||||
@@ -707,8 +716,8 @@ default_prompt = " default prompt "
|
||||
interface: Some(SkillInterface {
|
||||
display_name: Some("UI Skill".to_string()),
|
||||
short_description: Some("short desc".to_string()),
|
||||
icon_small: Some(normalized_skill_dir.join("./assets/small-400px.png")),
|
||||
icon_large: Some(normalized_skill_dir.join("./assets/large-logo.svg")),
|
||||
icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")),
|
||||
icon_large: Some(normalized_skill_dir.join("assets/large-logo.svg")),
|
||||
brand_color: Some("#3B82F6".to_string()),
|
||||
default_prompt: Some("default prompt".to_string()),
|
||||
}),
|
||||
@@ -753,7 +762,7 @@ icon_large = "./assets/logo.svg"
|
||||
display_name: Some("UI Skill".to_string()),
|
||||
short_description: None,
|
||||
icon_small: Some(normalized_skill_dir.join("assets/icon.png")),
|
||||
icon_large: Some(normalized_skill_dir.join("./assets/logo.svg")),
|
||||
icon_large: Some(normalized_skill_dir.join("assets/logo.svg")),
|
||||
brand_color: None,
|
||||
default_prompt: None,
|
||||
}),
|
||||
@@ -835,7 +844,7 @@ default_prompt = "{too_long}"
|
||||
interface: Some(SkillInterface {
|
||||
display_name: Some("UI Skill".to_string()),
|
||||
short_description: None,
|
||||
icon_small: Some(normalized_skill_dir.join("./assets/small-400px.png")),
|
||||
icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")),
|
||||
icon_large: None,
|
||||
brand_color: None,
|
||||
default_prompt: None,
|
||||
@@ -1376,12 +1385,47 @@ icon_large = "./assets/../logo.svg"
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deduplicates_by_name_preferring_repo_over_user() {
|
||||
async fn deduplicates_by_path_preferring_first_root() {
|
||||
let root = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo");
|
||||
|
||||
let outcome = load_skills_from_roots([
|
||||
SkillRoot {
|
||||
path: root.path().to_path_buf(),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
SkillRoot {
|
||||
path: root.path().to_path_buf(),
|
||||
scope: SkillScope::User,
|
||||
},
|
||||
]);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from repo".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn keeps_duplicate_names_from_repo_and_user() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let repo_dir = tempfile::tempdir().expect("tempdir");
|
||||
mark_as_git_repo(repo_dir.path());
|
||||
|
||||
let _user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user");
|
||||
let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user");
|
||||
let repo_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
@@ -1402,42 +1446,94 @@ icon_large = "./assets/../logo.svg"
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from repo".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&repo_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
vec![
|
||||
SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from repo".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&repo_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from user".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&user_skill_path),
|
||||
scope: SkillScope::User,
|
||||
},
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loads_system_skills_when_present() {
|
||||
async fn keeps_duplicate_names_from_nested_codex_dirs() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let repo_dir = tempfile::tempdir().expect("tempdir");
|
||||
mark_as_git_repo(repo_dir.path());
|
||||
|
||||
let _system_skill_path =
|
||||
write_system_skill(&codex_home, "system", "dupe-skill", "from system");
|
||||
let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user");
|
||||
let nested_dir = repo_dir.path().join("nested/inner");
|
||||
fs::create_dir_all(&nested_dir).unwrap();
|
||||
|
||||
let cfg = make_config(&codex_home).await;
|
||||
let root_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
.join(SKILLS_DIR_NAME),
|
||||
"root",
|
||||
"dupe-skill",
|
||||
"from root",
|
||||
);
|
||||
let nested_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
.join("nested")
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
.join(SKILLS_DIR_NAME),
|
||||
"nested",
|
||||
"dupe-skill",
|
||||
"from nested",
|
||||
);
|
||||
|
||||
let cfg = make_config_for_cwd(&codex_home, nested_dir).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
let root_path =
|
||||
canonicalize_path(&root_skill_path).unwrap_or_else(|_| root_skill_path.clone());
|
||||
let nested_path =
|
||||
canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone());
|
||||
let (first_path, second_path, first_description, second_description) =
|
||||
if root_path <= nested_path {
|
||||
(root_path, nested_path, "from root", "from nested")
|
||||
} else {
|
||||
(nested_path, root_path, "from nested", "from root")
|
||||
};
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from user".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&user_skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
vec![
|
||||
SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: first_description.to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: first_path,
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: second_description.to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: second_path,
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1448,7 +1544,7 @@ icon_large = "./assets/../logo.svg"
|
||||
let repo_dir = outer_dir.path().join("repo");
|
||||
fs::create_dir_all(&repo_dir).unwrap();
|
||||
|
||||
write_skill_at(
|
||||
let _skill_path = write_skill_at(
|
||||
&outer_dir
|
||||
.path()
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
@@ -1457,7 +1553,6 @@ icon_large = "./assets/../logo.svg"
|
||||
"outer-skill",
|
||||
"from outer",
|
||||
);
|
||||
|
||||
mark_as_git_repo(&repo_dir);
|
||||
|
||||
let cfg = make_config_for_cwd(&codex_home, repo_dir).await;
|
||||
@@ -1581,164 +1676,4 @@ icon_large = "./assets/../logo.svg"
|
||||
}
|
||||
assert_eq!(scopes, expected);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deduplicates_by_name_preferring_system_over_admin() {
|
||||
let system_dir = tempfile::tempdir().expect("tempdir");
|
||||
let admin_dir = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
let system_skill_path =
|
||||
write_skill_at(system_dir.path(), "system", "dupe-skill", "from system");
|
||||
let _admin_skill_path =
|
||||
write_skill_at(admin_dir.path(), "admin", "dupe-skill", "from admin");
|
||||
|
||||
let outcome = load_skills_from_roots([
|
||||
SkillRoot {
|
||||
path: system_dir.path().to_path_buf(),
|
||||
scope: SkillScope::System,
|
||||
},
|
||||
SkillRoot {
|
||||
path: admin_dir.path().to_path_buf(),
|
||||
scope: SkillScope::Admin,
|
||||
},
|
||||
]);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from system".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&system_skill_path),
|
||||
scope: SkillScope::System,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deduplicates_by_name_preferring_user_over_system() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let work_dir = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user");
|
||||
let _system_skill_path =
|
||||
write_system_skill(&codex_home, "system", "dupe-skill", "from system");
|
||||
|
||||
let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await;
|
||||
|
||||
let outcome = load_skills(&cfg);
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from user".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&user_skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deduplicates_by_name_preferring_repo_over_system() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let repo_dir = tempfile::tempdir().expect("tempdir");
|
||||
mark_as_git_repo(repo_dir.path());
|
||||
|
||||
let repo_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
.join(SKILLS_DIR_NAME),
|
||||
"repo",
|
||||
"dupe-skill",
|
||||
"from repo",
|
||||
);
|
||||
let _system_skill_path =
|
||||
write_system_skill(&codex_home, "system", "dupe-skill", "from system");
|
||||
|
||||
let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await;
|
||||
|
||||
let outcome = load_skills(&cfg);
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from repo".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: normalized(&repo_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn deduplicates_by_name_preferring_nearest_project_codex_dir() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let repo_dir = tempfile::tempdir().expect("tempdir");
|
||||
mark_as_git_repo(repo_dir.path());
|
||||
|
||||
let nested_dir = repo_dir.path().join("nested/inner");
|
||||
fs::create_dir_all(&nested_dir).unwrap();
|
||||
|
||||
let _root_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
.join(SKILLS_DIR_NAME),
|
||||
"root",
|
||||
"dupe-skill",
|
||||
"from root",
|
||||
);
|
||||
let nested_skill_path = write_skill_at(
|
||||
&repo_dir
|
||||
.path()
|
||||
.join("nested")
|
||||
.join(REPO_ROOT_CONFIG_DIR_NAME)
|
||||
.join(SKILLS_DIR_NAME),
|
||||
"nested",
|
||||
"dupe-skill",
|
||||
"from nested",
|
||||
);
|
||||
|
||||
let cfg = make_config_for_cwd(&codex_home, nested_dir).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
let expected_path =
|
||||
canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone());
|
||||
assert_eq!(
|
||||
vec![SkillMetadata {
|
||||
name: "dupe-skill".to_string(),
|
||||
description: "from nested".to_string(),
|
||||
short_description: None,
|
||||
interface: None,
|
||||
path: expected_path,
|
||||
scope: SkillScope::Repo,
|
||||
}],
|
||||
outcome.skills
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,12 +1,15 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::RwLock;
|
||||
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use toml::Value as TomlValue;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::config::Config;
|
||||
use crate::config::types::SkillsConfig;
|
||||
use crate::config_loader::LoaderOverrides;
|
||||
use crate::config_loader::load_config_layers_state;
|
||||
use crate::skills::SkillLoadOutcome;
|
||||
@@ -44,7 +47,8 @@ impl SkillsManager {
|
||||
}
|
||||
|
||||
let roots = skill_roots_from_layer_stack(&config.config_layer_stack);
|
||||
let outcome = load_skills_from_roots(roots);
|
||||
let mut outcome = load_skills_from_roots(roots);
|
||||
outcome.disabled_paths = disabled_paths_from_stack(&config.config_layer_stack);
|
||||
match self.cache_by_cwd.write() {
|
||||
Ok(mut cache) => {
|
||||
cache.insert(cwd.to_path_buf(), outcome.clone());
|
||||
@@ -100,7 +104,8 @@ impl SkillsManager {
|
||||
};
|
||||
|
||||
let roots = skill_roots_from_layer_stack(&config_layer_stack);
|
||||
let outcome = load_skills_from_roots(roots);
|
||||
let mut outcome = load_skills_from_roots(roots);
|
||||
outcome.disabled_paths = disabled_paths_from_stack(&config_layer_stack);
|
||||
match self.cache_by_cwd.write() {
|
||||
Ok(mut cache) => {
|
||||
cache.insert(cwd.to_path_buf(), outcome.clone());
|
||||
@@ -111,6 +116,51 @@ impl SkillsManager {
|
||||
}
|
||||
outcome
|
||||
}
|
||||
|
||||
pub fn clear_cache(&self) {
|
||||
match self.cache_by_cwd.write() {
|
||||
Ok(mut cache) => cache.clear(),
|
||||
Err(err) => err.into_inner().clear(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn disabled_paths_from_stack(
|
||||
config_layer_stack: &crate::config_loader::ConfigLayerStack,
|
||||
) -> 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 entry in skills.config {
|
||||
let path = normalize_override_path(entry.path.as_path());
|
||||
configs.insert(path, entry.enabled);
|
||||
}
|
||||
|
||||
for (path, enabled) in configs {
|
||||
if !enabled {
|
||||
disabled.insert(path);
|
||||
}
|
||||
}
|
||||
|
||||
disabled
|
||||
}
|
||||
|
||||
fn normalize_override_path(path: &Path) -> PathBuf {
|
||||
dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use std::collections::HashSet;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
@@ -32,4 +33,25 @@ pub struct SkillError {
|
||||
pub struct SkillLoadOutcome {
|
||||
pub skills: Vec<SkillMetadata>,
|
||||
pub errors: Vec<SkillError>,
|
||||
pub disabled_paths: HashSet<PathBuf>,
|
||||
}
|
||||
|
||||
impl SkillLoadOutcome {
|
||||
pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool {
|
||||
!self.disabled_paths.contains(&skill.path)
|
||||
}
|
||||
|
||||
pub fn enabled_skills(&self) -> Vec<SkillMetadata> {
|
||||
self.skills
|
||||
.iter()
|
||||
.filter(|skill| self.is_skill_enabled(skill))
|
||||
.cloned()
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub fn skills_with_enabled(&self) -> impl Iterator<Item = (&SkillMetadata, bool)> {
|
||||
self.skills
|
||||
.iter()
|
||||
.map(|skill| (skill, self.is_skill_enabled(skill)))
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user