diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index f5e9fce59ee..5930fe43559 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -2652,19 +2652,17 @@ impl CodexMessageProcessor { }; let skills_manager = self.conversation_manager.skills_manager(); - let data = cwds - .into_iter() - .map(|cwd| { - let outcome = skills_manager.skills_for_cwd_with_options(&cwd, force_reload); - let errors = errors_to_info(&outcome.errors); - let skills = skills_to_info(&outcome.skills); - codex_app_server_protocol::SkillsListEntry { - cwd, - skills, - errors, - } - }) - .collect(); + let mut data = Vec::new(); + for cwd in cwds { + let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; + let errors = errors_to_info(&outcome.errors); + let skills = skills_to_info(&outcome.skills); + data.push(codex_app_server_protocol::SkillsListEntry { + cwd, + skills, + errors, + }); + } self.outgoing .send_response(request_id, SkillsListResponse { data }) .await; diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e853a2e3843..bb4a45a75df 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -218,10 +218,11 @@ impl Codex { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); - let loaded_skills = config - .features - .enabled(Feature::Skills) - .then(|| skills_manager.skills_for_cwd(&config.cwd)); + let loaded_skills = if config.features.enabled(Feature::Skills) { + Some(skills_manager.skills_for_config(&config)) + } else { + None + }; if let Some(outcome) = &loaded_skills { for err in &outcome.errors { @@ -1987,18 +1988,18 @@ mod handlers { }; let skills = if sess.enabled(Feature::Skills) { let skills_manager = &sess.services.skills_manager; - cwds.into_iter() - .map(|cwd| { - let outcome = skills_manager.skills_for_cwd_with_options(&cwd, force_reload); - let errors = super::errors_to_info(&outcome.errors); - let skills = super::skills_to_info(&outcome.skills); - SkillsListEntry { - cwd, - skills, - errors, - } - }) - .collect() + let mut entries = Vec::new(); + 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 = super::skills_to_info(&outcome.skills); + entries.push(SkillsListEntry { + cwd, + skills, + errors, + }); + } + entries } else { cwds.into_iter() .map(|cwd| SkillsListEntry { @@ -2252,11 +2253,16 @@ pub(crate) async fn run_task( }); sess.send_event(&turn_context, event).await; - let skills_outcome = sess.enabled(Feature::Skills).then(|| { - sess.services - .skills_manager - .skills_for_cwd(&turn_context.cwd) - }); + let skills_outcome = if sess.enabled(Feature::Skills) { + Some( + sess.services + .skills_manager + .skills_for_cwd(&turn_context.cwd, false) + .await, + ) + } else { + None + }; let SkillInjections { items: skill_items, diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index bce13fbb057..35aeaec7f85 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -1,9 +1,10 @@ use crate::config::Config; -use crate::git_info::resolve_root_git_project_for_trust; +use crate::config_loader::ConfigLayerStack; use crate::skills::model::SkillError; use crate::skills::model::SkillLoadOutcome; use crate::skills::model::SkillMetadata; use crate::skills::system::system_cache_root_dir; +use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::SkillScope; use dunce::canonicalize as normalize_path; use serde::Deserialize; @@ -32,8 +33,6 @@ struct SkillFrontmatterMetadata { const SKILLS_FILENAME: &str = "SKILL.md"; const SKILLS_DIR_NAME: &str = "skills"; -const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex"; -const ADMIN_SKILLS_ROOT: &str = "/etc/codex/skills"; const MAX_NAME_LEN: usize = 64; const MAX_DESCRIPTION_LEN: usize = 1024; const MAX_SHORT_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; @@ -88,86 +87,81 @@ where .skills .retain(|skill| seen.insert(skill.name.clone())); - outcome - .skills - .sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path))); + fn scope_rank(scope: SkillScope) -> u8 { + // Higher-priority scopes first (matches dedupe priority order). + match scope { + SkillScope::Repo => 0, + SkillScope::User => 1, + SkillScope::System => 2, + SkillScope::Admin => 3, + } + } - outcome -} + outcome.skills.sort_by(|a, b| { + scope_rank(a.scope) + .cmp(&scope_rank(b.scope)) + .then_with(|| a.name.cmp(&b.name)) + .then_with(|| a.path.cmp(&b.path)) + }); -pub(crate) fn user_skills_root(codex_home: &Path) -> SkillRoot { - SkillRoot { - path: codex_home.join(SKILLS_DIR_NAME), - scope: SkillScope::User, - } + outcome } -pub(crate) fn system_skills_root(codex_home: &Path) -> SkillRoot { - SkillRoot { - path: system_cache_root_dir(codex_home), - scope: SkillScope::System, - } -} +fn skill_roots_from_layer_stack_inner(config_layer_stack: &ConfigLayerStack) -> Vec { + let mut roots = Vec::new(); -pub(crate) fn admin_skills_root() -> SkillRoot { - SkillRoot { - path: PathBuf::from(ADMIN_SKILLS_ROOT), - scope: SkillScope::Admin, - } -} + for layer in config_layer_stack.layers_high_to_low() { + let Some(config_folder) = layer.config_folder() else { + continue; + }; -pub(crate) fn repo_skills_root(cwd: &Path) -> Option { - let base = if cwd.is_dir() { cwd } else { cwd.parent()? }; - let base = normalize_path(base).unwrap_or_else(|_| base.to_path_buf()); - - let repo_root = - resolve_root_git_project_for_trust(&base).map(|root| normalize_path(&root).unwrap_or(root)); - - let scope = SkillScope::Repo; - if let Some(repo_root) = repo_root.as_deref() { - for dir in base.ancestors() { - let skills_root = dir.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); - if skills_root.is_dir() { - return Some(SkillRoot { - path: skills_root, - scope, + match &layer.name { + ConfigLayerSource::Project { .. } => { + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::Repo, }); } + ConfigLayerSource::User { .. } => { + // `$CODEX_HOME/skills` (user-installed skills). + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::User, + }); - if dir == repo_root { - break; + // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a + // special case (not a config layer). + roots.push(SkillRoot { + path: system_cache_root_dir(config_folder.as_path()), + scope: SkillScope::System, + }); } + ConfigLayerSource::System { .. } => { + // The system config layer lives under `/etc/codex/` on Unix, so treat + // `/etc/codex/skills` as admin-scoped skills. + roots.push(SkillRoot { + path: config_folder.as_path().join(SKILLS_DIR_NAME), + scope: SkillScope::Admin, + }); + } + ConfigLayerSource::Mdm { .. } + | ConfigLayerSource::SessionFlags + | ConfigLayerSource::LegacyManagedConfigTomlFromFile { .. } + | ConfigLayerSource::LegacyManagedConfigTomlFromMdm => {} } - return None; - } - - let skills_root = base.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); - skills_root.is_dir().then_some(SkillRoot { - path: skills_root, - scope, - }) -} - -pub(crate) fn skill_roots_for_cwd(codex_home: &Path, cwd: &Path) -> Vec { - let mut roots = Vec::new(); - - if let Some(repo_root) = repo_skills_root(cwd) { - roots.push(repo_root); - } - - // Load order matters: we dedupe by name, keeping the first occurrence. - // Priority order: repo, user, system, then admin. - roots.push(user_skills_root(codex_home)); - roots.push(system_skills_root(codex_home)); - if cfg!(unix) { - roots.push(admin_skills_root()); } roots } fn skill_roots(config: &Config) -> Vec { - skill_roots_for_cwd(&config.codex_home, &config.cwd) + skill_roots_from_layer_stack_inner(&config.config_layer_stack) +} + +pub(crate) fn skill_roots_from_layer_stack( + config_layer_stack: &ConfigLayerStack, +) -> Vec { + skill_roots_from_layer_stack_inner(config_layer_stack) } fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut SkillLoadOutcome) { @@ -318,21 +312,91 @@ fn extract_frontmatter(contents: &str) -> Option { 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::ConfigRequirements; use codex_protocol::protocol::SkillScope; + use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::path::Path; - use std::process::Command; use tempfile::TempDir; + use toml::Value as TomlValue; + + const REPO_ROOT_CONFIG_DIR_NAME: &str = ".codex"; async fn make_config(codex_home: &TempDir) -> Config { - let mut config = ConfigBuilder::default() + make_config_for_cwd(codex_home, codex_home.path().to_path_buf()).await + } + + async fn make_config_for_cwd(codex_home: &TempDir, cwd: PathBuf) -> Config { + let harness_overrides = ConfigOverrides { + cwd: Some(cwd), + ..Default::default() + }; + + ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(harness_overrides) .build() .await - .expect("defaults for test should always succeed"); + .expect("defaults for test should always succeed") + } + + fn mark_as_git_repo(dir: &Path) { + // Config/project-root discovery only checks for the presence of `.git` (file or dir), + // so we can avoid shelling out to `git init` in tests. + fs::write(dir.join(".git"), "gitdir: fake\n").unwrap(); + } + + fn normalized(path: &Path) -> PathBuf { + normalize_path(path).unwrap_or_else(|_| path.to_path_buf()) + } - config.cwd = codex_home.path().to_path_buf(); - config + #[test] + fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() + -> anyhow::Result<()> { + let tmp = tempfile::tempdir()?; + + let system_folder = tmp.path().join("etc/codex"); + let user_folder = tmp.path().join("home/codex"); + fs::create_dir_all(&system_folder)?; + fs::create_dir_all(&user_folder)?; + + // The file path doesn't need to exist; it's only used to derive the config folder. + let system_file = AbsolutePathBuf::from_absolute_path(system_folder.join("config.toml"))?; + let user_file = AbsolutePathBuf::from_absolute_path(user_folder.join("config.toml"))?; + + let layers = vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { file: system_file }, + TomlValue::Table(toml::map::Map::new()), + ), + ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + TomlValue::Table(toml::map::Map::new()), + ), + ]; + let stack = ConfigLayerStack::new(layers, ConfigRequirements::default())?; + + let got = skill_roots_from_layer_stack(&stack) + .into_iter() + .map(|root| (root.scope, root.path)) + .collect::>(); + + assert_eq!( + got, + vec![ + (SkillScope::User, user_folder.join("skills")), + ( + SkillScope::System, + user_folder.join("skills").join(".system") + ), + (SkillScope::Admin, system_folder.join("skills")), + ] + ); + + Ok(()) } fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { @@ -368,7 +432,7 @@ mod tests { #[tokio::test] async fn loads_valid_skill() { let codex_home = tempfile::tempdir().expect("tempdir"); - write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); + let skill_path = write_skill(&codex_home, "demo", "demo-skill", "does things\ncarefully"); let cfg = make_config(&codex_home).await; let outcome = load_skills(&cfg); @@ -377,15 +441,15 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - let skill = &outcome.skills[0]; - assert_eq!(skill.name, "demo-skill"); - assert_eq!(skill.description, "does things carefully"); - assert_eq!(skill.short_description, None); - let path_str = skill.path.to_string_lossy().replace('\\', "/"); - assert!( - path_str.ends_with("skills/demo/SKILL.md"), - "unexpected path {path_str}" + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "demo-skill".to_string(), + description: "does things carefully".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::User, + }] ); } @@ -395,7 +459,8 @@ mod tests { let skill_dir = codex_home.path().join("skills/demo"); fs::create_dir_all(&skill_dir).unwrap(); let contents = "---\nname: demo-skill\ndescription: long description\nmetadata:\n short-description: short summary\n---\n\n# Body\n"; - fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); + let skill_path = skill_dir.join(SKILLS_FILENAME); + fs::write(&skill_path, contents).unwrap(); let cfg = make_config(&codex_home).await; let outcome = load_skills(&cfg); @@ -404,10 +469,15 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); assert_eq!( - outcome.skills[0].short_description, - Some("short summary".to_string()) + outcome.skills, + vec![SkillMetadata { + name: "demo-skill".to_string(), + description: "long description".to_string(), + short_description: Some("short summary".to_string()), + path: normalized(&skill_path), + scope: SkillScope::User, + }] ); } @@ -493,22 +563,14 @@ mod tests { async fn loads_skills_from_repo_root() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); - - let status = Command::new("git") - .arg("init") - .current_dir(repo_dir.path()) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); + mark_as_git_repo(repo_dir.path()); let skills_root = repo_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) .join(SKILLS_DIR_NAME); - write_skill_at(&skills_root, "repo", "repo-skill", "from repo"); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = repo_dir.path().to_path_buf(); - let repo_root = normalize_path(&skills_root).unwrap_or_else(|_| skills_root.clone()); + let skill_path = write_skill_at(&skills_root, "repo", "repo-skill", "from repo"); + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -516,28 +578,28 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - let skill = &outcome.skills[0]; - assert_eq!(skill.name, "repo-skill"); - assert!(skill.path.starts_with(&repo_root)); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); } #[tokio::test] - async fn loads_skills_from_nearest_codex_dir_under_repo_root() { + async fn loads_skills_from_all_codex_dirs_under_project_root() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); - - let status = Command::new("git") - .arg("init") - .current_dir(repo_dir.path()) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); + mark_as_git_repo(repo_dir.path()); let nested_dir = repo_dir.path().join("nested/inner"); fs::create_dir_all(&nested_dir).unwrap(); - write_skill_at( + let root_skill_path = write_skill_at( &repo_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -546,7 +608,7 @@ mod tests { "root-skill", "from root", ); - write_skill_at( + let nested_skill_path = write_skill_at( &repo_dir .path() .join("nested") @@ -557,8 +619,7 @@ mod tests { "from nested", ); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = nested_dir; + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; let outcome = load_skills(&cfg); assert!( @@ -566,8 +627,25 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "nested-skill"); + assert_eq!( + outcome.skills, + vec![ + SkillMetadata { + name: "nested-skill".to_string(), + description: "from nested".to_string(), + short_description: None, + path: normalized(&nested_skill_path), + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "root-skill".to_string(), + description: "from root".to_string(), + short_description: None, + path: normalized(&root_skill_path), + scope: SkillScope::Repo, + }, + ] + ); } #[tokio::test] @@ -575,7 +653,7 @@ mod tests { let codex_home = tempfile::tempdir().expect("tempdir"); let work_dir = tempfile::tempdir().expect("tempdir"); - write_skill_at( + let skill_path = write_skill_at( &work_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -585,8 +663,7 @@ mod tests { "from cwd", ); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = work_dir.path().to_path_buf(); + let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -594,25 +671,26 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "local-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "local-skill".to_string(), + description: "from cwd".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); } #[tokio::test] async fn deduplicates_by_name_preferring_repo_over_user() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); - let status = Command::new("git") - .arg("init") - .current_dir(repo_dir.path()) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); - - write_skill(&codex_home, "user", "dupe-skill", "from user"); - write_skill_at( + let _user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); + let repo_skill_path = write_skill_at( &repo_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -622,8 +700,7 @@ mod tests { "from repo", ); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = repo_dir.path().to_path_buf(); + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -631,17 +708,25 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "dupe-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + path: normalized(&repo_skill_path), + scope: SkillScope::Repo, + }] + ); } #[tokio::test] async fn loads_system_skills_when_present() { let codex_home = tempfile::tempdir().expect("tempdir"); - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); - write_skill(&codex_home, "user", "dupe-skill", "from user"); + 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 cfg = make_config(&codex_home).await; let outcome = load_skills(&cfg); @@ -650,9 +735,16 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].description, "from user"); - assert_eq!(outcome.skills[0].scope, SkillScope::User); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from user".to_string(), + short_description: None, + path: normalized(&user_skill_path), + scope: SkillScope::User, + }] + ); } #[tokio::test] @@ -672,15 +764,9 @@ mod tests { "from outer", ); - let status = Command::new("git") - .arg("init") - .current_dir(&repo_dir) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); + mark_as_git_repo(&repo_dir); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = repo_dir; + let cfg = make_config_for_cwd(&codex_home, repo_dir).await; let outcome = load_skills(&cfg); assert!( @@ -695,15 +781,9 @@ mod tests { async fn loads_skills_when_cwd_is_file_in_repo() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); - let status = Command::new("git") - .arg("init") - .current_dir(repo_dir.path()) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); - - write_skill_at( + let skill_path = write_skill_at( &repo_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -715,8 +795,7 @@ mod tests { let file_path = repo_dir.path().join("some-file.txt"); fs::write(&file_path, "contents").unwrap(); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = file_path; + let cfg = make_config_for_cwd(&codex_home, file_path).await; let outcome = load_skills(&cfg); assert!( @@ -724,9 +803,16 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "repo-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "repo-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); } #[tokio::test] @@ -746,8 +832,7 @@ mod tests { "from outer", ); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = nested_dir; + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; let outcome = load_skills(&cfg); assert!( @@ -763,10 +848,9 @@ mod tests { let codex_home = tempfile::tempdir().expect("tempdir"); let work_dir = tempfile::tempdir().expect("tempdir"); - write_system_skill(&codex_home, "system", "system-skill", "from system"); + let skill_path = write_system_skill(&codex_home, "system", "system-skill", "from system"); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = work_dir.path().to_path_buf(); + let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -774,9 +858,16 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "system-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::System); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "system-skill".to_string(), + description: "from system".to_string(), + short_description: None, + path: normalized(&skill_path), + scope: SkillScope::System, + }] + ); } #[tokio::test] @@ -800,8 +891,10 @@ mod tests { let system_dir = tempfile::tempdir().expect("tempdir"); let admin_dir = tempfile::tempdir().expect("tempdir"); - write_skill_at(system_dir.path(), "system", "dupe-skill", "from system"); - write_skill_at(admin_dir.path(), "admin", "dupe-skill", "from admin"); + 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 { @@ -819,9 +912,16 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "dupe-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::System); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from system".to_string(), + short_description: None, + path: normalized(&system_skill_path), + scope: SkillScope::System, + }] + ); } #[tokio::test] @@ -829,11 +929,11 @@ mod tests { let codex_home = tempfile::tempdir().expect("tempdir"); let work_dir = tempfile::tempdir().expect("tempdir"); - write_skill(&codex_home, "user", "dupe-skill", "from user"); - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); + 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 mut cfg = make_config(&codex_home).await; - cfg.cwd = work_dir.path().to_path_buf(); + let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -841,24 +941,25 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "dupe-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::User); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from user".to_string(), + short_description: 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 status = Command::new("git") - .arg("init") - .current_dir(repo_dir.path()) - .status() - .expect("git init"); - assert!(status.success(), "git init failed"); - - write_skill_at( + let repo_skill_path = write_skill_at( &repo_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -867,10 +968,10 @@ mod tests { "dupe-skill", "from repo", ); - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); + let _system_skill_path = + write_system_skill(&codex_home, "system", "dupe-skill", "from system"); - let mut cfg = make_config(&codex_home).await; - cfg.cwd = repo_dir.path().to_path_buf(); + let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; let outcome = load_skills(&cfg); assert!( @@ -878,8 +979,66 @@ mod tests { "unexpected errors: {:?}", outcome.errors ); - assert_eq!(outcome.skills.len(), 1); - assert_eq!(outcome.skills[0].name, "dupe-skill"); - assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: 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 = + normalize_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, + path: expected_path, + scope: SkillScope::Repo, + }], + outcome.skills + ); } } diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 8cc93d05bc2..bff928a5657 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -3,10 +3,17 @@ use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; +use codex_utils_absolute_path::AbsolutePathBuf; +use toml::Value as TomlValue; + +use crate::config::Config; +use crate::config_loader::LoaderOverrides; +use crate::config_loader::load_config_layers_state; use crate::skills::SkillLoadOutcome; use crate::skills::loader::load_skills_from_roots; -use crate::skills::loader::skill_roots_for_cwd; +use crate::skills::loader::skill_roots_from_layer_stack; use crate::skills::system::install_system_skills; + pub struct SkillsManager { codex_home: PathBuf, cache_by_cwd: RwLock>, @@ -24,11 +31,32 @@ impl SkillsManager { } } - pub fn skills_for_cwd(&self, cwd: &Path) -> SkillLoadOutcome { - self.skills_for_cwd_with_options(cwd, false) + /// Load skills for an already-constructed [`Config`], avoiding any additional config-layer + /// loading. This also seeds the per-cwd cache for subsequent lookups. + pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { + let cwd = &config.cwd; + let cached = match self.cache_by_cwd.read() { + Ok(cache) => cache.get(cwd).cloned(), + Err(err) => err.into_inner().get(cwd).cloned(), + }; + if let Some(outcome) = cached { + return outcome; + } + + let roots = skill_roots_from_layer_stack(&config.config_layer_stack); + let outcome = load_skills_from_roots(roots); + match self.cache_by_cwd.write() { + Ok(mut cache) => { + cache.insert(cwd.to_path_buf(), outcome.clone()); + } + Err(err) => { + err.into_inner().insert(cwd.to_path_buf(), outcome.clone()); + } + } + outcome } - pub fn skills_for_cwd_with_options(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome { + pub async fn skills_for_cwd(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome { let cached = match self.cache_by_cwd.read() { Ok(cache) => cache.get(cwd).cloned(), Err(err) => err.into_inner().get(cwd).cloned(), @@ -37,7 +65,41 @@ impl SkillsManager { return outcome; } - let roots = skill_roots_for_cwd(&self.codex_home, cwd); + let cwd_abs = match AbsolutePathBuf::try_from(cwd) { + Ok(cwd_abs) => cwd_abs, + Err(err) => { + return SkillLoadOutcome { + errors: vec![crate::skills::model::SkillError { + path: cwd.to_path_buf(), + message: err.to_string(), + }], + ..Default::default() + }; + } + }; + + let cli_overrides: Vec<(String, TomlValue)> = Vec::new(); + let config_layer_stack = match load_config_layers_state( + &self.codex_home, + Some(cwd_abs), + &cli_overrides, + LoaderOverrides::default(), + ) + .await + { + Ok(config_layer_stack) => config_layer_stack, + Err(err) => { + return SkillLoadOutcome { + errors: vec![crate::skills::model::SkillError { + path: cwd.to_path_buf(), + message: err.to_string(), + }], + ..Default::default() + }; + } + }; + + let roots = skill_roots_from_layer_stack(&config_layer_stack); let outcome = load_skills_from_roots(roots); match self.cache_by_cwd.write() { Ok(mut cache) => { @@ -50,3 +112,52 @@ impl SkillsManager { outcome } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::ConfigBuilder; + use crate::config::ConfigOverrides; + use pretty_assertions::assert_eq; + use std::fs; + use tempfile::TempDir; + + fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { + let skill_dir = codex_home.path().join("skills").join(dir); + fs::create_dir_all(&skill_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skill_dir.join("SKILL.md"), content).unwrap(); + } + + #[tokio::test] + async fn skills_for_config_seeds_cache_by_cwd() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + + let cfg = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf()); + + write_user_skill(&codex_home, "a", "skill-a", "from a"); + let outcome1 = skills_manager.skills_for_config(&cfg); + assert!( + outcome1.skills.iter().any(|s| s.name == "skill-a"), + "expected skill-a to be discovered" + ); + + // Write a new skill after the first call; the second call should hit the cache and not + // reflect the new file. + write_user_skill(&codex_home, "b", "skill-b", "from b"); + let outcome2 = skills_manager.skills_for_config(&cfg); + assert_eq!(outcome2.errors, outcome1.errors); + assert_eq!(outcome2.skills, outcome1.skills); + } +}