From f33ac830aad5cdee4e52cf7eca0a73686a17b375 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 21 Feb 2026 14:40:13 -0800 Subject: [PATCH] fix: make skills loader tests hermetic with ~/.agents skills (#12474) --- codex-rs/core/src/skills/loader.rs | 82 ++++++++++++++++++------------ 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 1ad231438..6508d4fd1 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -137,7 +137,17 @@ impl fmt::Display for SkillParseError { impl Error for SkillParseError {} pub fn load_skills(config: &Config) -> SkillLoadOutcome { - load_skills_from_roots(skill_roots(config)) + load_skills_with_home_dir(config, home_dir().as_deref()) +} + +fn load_skills_with_home_dir(config: &Config, home_dir: Option<&Path>) -> SkillLoadOutcome { + let mut roots = skill_roots_from_layer_stack_inner(&config.config_layer_stack, home_dir); + roots.extend(repo_agents_skill_roots( + &config.config_layer_stack, + &config.cwd, + )); + dedupe_skill_roots_by_path(&mut roots); + load_skills_from_roots(roots) } pub(crate) struct SkillRoot { @@ -240,6 +250,7 @@ fn skill_roots_from_layer_stack_inner( roots } +#[cfg(test)] fn skill_roots(config: &Config) -> Vec { skill_roots_from_layer_stack_with_agents(&config.config_layer_stack, &config.cwd) } @@ -867,6 +878,11 @@ mod tests { .expect("defaults for test should always succeed") } + fn load_skills_for_test(config: &Config) -> SkillLoadOutcome { + // Keep unit tests hermetic by never scanning the real `$HOME/.agents/skills`. + super::load_skills_with_home_dir(config, None) + } + 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. @@ -1129,7 +1145,7 @@ mod tests { ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1208,7 +1224,7 @@ interface: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1258,7 +1274,7 @@ policy: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1289,7 +1305,7 @@ policy: {} ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1332,7 +1348,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1394,7 +1410,7 @@ permissions: {} ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1452,7 +1468,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1502,7 +1518,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1547,7 +1563,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1595,7 +1611,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1642,7 +1658,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1691,7 +1707,7 @@ permissions: ); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1736,7 +1752,7 @@ permissions: symlink_dir(shared.path(), &codex_home.path().join("skills/shared")); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1773,7 +1789,7 @@ permissions: symlink_file(&shared_skill_path, &skill_dir.join(SKILLS_FILENAME)); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1797,7 +1813,7 @@ permissions: let skill_path = write_skill_at(&cycle_dir, "demo", "cycle-skill", "still loads"); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1875,7 +1891,7 @@ permissions: symlink_dir(shared.path(), &repo_skills_root.join("shared")); let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -1972,7 +1988,7 @@ permissions: 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); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2004,7 +2020,7 @@ permissions: fs::write(&skill_path, contents).unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2038,7 +2054,7 @@ permissions: fs::write(skill_dir.join(SKILLS_FILENAME), contents).unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( @@ -2067,7 +2083,7 @@ permissions: fs::write(invalid_dir.join(SKILLS_FILENAME), "---\nname: bad").unwrap(); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert_eq!(outcome.skills.len(), 0); assert_eq!(outcome.errors.len(), 1); assert!( @@ -2085,7 +2101,7 @@ permissions: write_skill(&codex_home, "max-len", "max-len", &max_desc); let cfg = make_config(&codex_home).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2095,7 +2111,7 @@ permissions: let too_long_desc = "\u{1F4A1}".repeat(MAX_DESCRIPTION_LEN + 1); write_skill(&codex_home, "too-long", "too-long", &too_long_desc); - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert_eq!(outcome.skills.len(), 1); assert_eq!(outcome.errors.len(), 1); assert!( @@ -2117,7 +2133,7 @@ permissions: 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); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2153,7 +2169,7 @@ permissions: ); let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2206,7 +2222,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2258,7 +2274,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2337,7 +2353,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2402,7 +2418,7 @@ permissions: ); let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), @@ -2468,7 +2484,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, repo_dir).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2497,7 +2513,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, file_path).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2538,7 +2554,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -2556,7 +2572,7 @@ permissions: let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - let outcome = load_skills(&cfg); + let outcome = load_skills_for_test(&cfg); assert!( outcome.errors.is_empty(), "unexpected errors: {:?}",