feat: extend skills/list to support additional roots. (#10835)

Add an optional perCwdExtraUserRoots

codex-rs/core/src/skills/manager.rs

@@ -4,6 +4,7 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::sync::RwLock;
 
+use codex_protocol::protocol::SkillScope;
 use codex_utils_absolute_path::AbsolutePathBuf;
 use toml::Value as TomlValue;
 use tracing::info;
@@ -15,6 +16,7 @@ use crate::config_loader::CloudRequirementsLoader;
 use crate::config_loader::LoaderOverrides;
 use crate::config_loader::load_config_layers_state;
 use crate::skills::SkillLoadOutcome;
+use crate::skills::loader::SkillRoot;
 use crate::skills::loader::load_skills_from_roots;
 use crate::skills::loader::skill_roots_from_layer_stack_with_agents;
 use crate::skills::system::install_system_skills;
@@ -40,11 +42,7 @@ impl SkillsManager {
     /// 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 {
+        if let Some(outcome) = self.cached_outcome_for_cwd(cwd) {
             return outcome;
         }
 
@@ -61,14 +59,25 @@ impl SkillsManager {
     }
 
     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(),
-        };
-        if !force_reload && let Some(outcome) = cached {
+        if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) {
             return outcome;
         }
 
+        self.skills_for_cwd_with_extra_user_roots(cwd, force_reload, &[])
+            .await
+    }
+
+    pub async fn skills_for_cwd_with_extra_user_roots(
+        &self,
+        cwd: &Path,
+        force_reload: bool,
+        extra_user_roots: &[PathBuf],
+    ) -> SkillLoadOutcome {
+        if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) {
+            return outcome;
+        }
+        let normalized_extra_user_roots = normalize_extra_user_roots(extra_user_roots);
+
         let cwd_abs = match AbsolutePathBuf::try_from(cwd) {
             Ok(cwd_abs) => cwd_abs,
             Err(err) => {
@@ -104,7 +113,16 @@ impl SkillsManager {
             }
         };
 
-        let roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd);
+        let mut roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd);
+        roots.extend(
+            normalized_extra_user_roots
+                .iter()
+                .cloned()
+                .map(|path| SkillRoot {
+                    path,
+                    scope: SkillScope::User,
+                }),
+        );
         let mut outcome = load_skills_from_roots(roots);
         outcome.disabled_paths = disabled_paths_from_stack(&config_layer_stack);
         let mut cache = match self.cache_by_cwd.write() {
@@ -124,6 +142,13 @@ impl SkillsManager {
         cache.clear();
         info!("skills cache cleared ({cleared} entries)");
     }
+
+    fn cached_outcome_for_cwd(&self, cwd: &Path) -> Option<SkillLoadOutcome> {
+        match self.cache_by_cwd.read() {
+            Ok(cache) => cache.get(cwd).cloned(),
+            Err(err) => err.into_inner().get(cwd).cloned(),
+        }
+    }
 }
 
 fn disabled_paths_from_stack(
@@ -164,6 +189,16 @@ fn normalize_override_path(path: &Path) -> PathBuf {
     dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf())
 }
 
+fn normalize_extra_user_roots(extra_user_roots: &[PathBuf]) -> Vec<PathBuf> {
+    let mut normalized: Vec<PathBuf> = extra_user_roots
+        .iter()
+        .map(|path| dunce::canonicalize(path).unwrap_or_else(|_| path.clone()))
+        .collect();
+    normalized.sort_unstable();
+    normalized.dedup();
+    normalized
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -171,6 +206,7 @@ mod tests {
     use crate::config::ConfigOverrides;
     use pretty_assertions::assert_eq;
     use std::fs;
+    use std::path::PathBuf;
     use tempfile::TempDir;
 
     fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) {
@@ -211,4 +247,143 @@ mod tests {
         assert_eq!(outcome2.errors, outcome1.errors);
         assert_eq!(outcome2.skills, outcome1.skills);
     }
+
+    #[tokio::test]
+    async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() {
+        let codex_home = tempfile::tempdir().expect("tempdir");
+        let cwd = tempfile::tempdir().expect("tempdir");
+        let extra_root = tempfile::tempdir().expect("tempdir");
+
+        let config = 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());
+        let _ = skills_manager.skills_for_config(&config);
+
+        write_user_skill(&extra_root, "x", "extra-skill", "from extra root");
+        let extra_root_path = extra_root.path().to_path_buf();
+        let outcome_with_extra = skills_manager
+            .skills_for_cwd_with_extra_user_roots(
+                cwd.path(),
+                true,
+                std::slice::from_ref(&extra_root_path),
+            )
+            .await;
+        assert!(
+            outcome_with_extra
+                .skills
+                .iter()
+                .any(|skill| skill.name == "extra-skill")
+        );
+
+        // The cwd-only API returns the current cached entry for this cwd, even when that entry
+        // was produced with extra roots.
+        let outcome_without_extra = skills_manager.skills_for_cwd(cwd.path(), false).await;
+        assert_eq!(outcome_without_extra.skills, outcome_with_extra.skills);
+        assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors);
+    }
+
+    #[tokio::test]
+    async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
+        let codex_home = tempfile::tempdir().expect("tempdir");
+        let cwd = tempfile::tempdir().expect("tempdir");
+        let extra_root_a = tempfile::tempdir().expect("tempdir");
+        let extra_root_b = tempfile::tempdir().expect("tempdir");
+
+        let config = 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());
+        let _ = skills_manager.skills_for_config(&config);
+
+        write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a");
+        write_user_skill(&extra_root_b, "x", "extra-skill-b", "from extra root b");
+
+        let extra_root_a_path = extra_root_a.path().to_path_buf();
+        let outcome_a = skills_manager
+            .skills_for_cwd_with_extra_user_roots(
+                cwd.path(),
+                true,
+                std::slice::from_ref(&extra_root_a_path),
+            )
+            .await;
+        assert!(
+            outcome_a
+                .skills
+                .iter()
+                .any(|skill| skill.name == "extra-skill-a")
+        );
+        assert!(
+            outcome_a
+                .skills
+                .iter()
+                .all(|skill| skill.name != "extra-skill-b")
+        );
+
+        let extra_root_b_path = extra_root_b.path().to_path_buf();
+        let outcome_b = skills_manager
+            .skills_for_cwd_with_extra_user_roots(
+                cwd.path(),
+                false,
+                std::slice::from_ref(&extra_root_b_path),
+            )
+            .await;
+        assert!(
+            outcome_b
+                .skills
+                .iter()
+                .any(|skill| skill.name == "extra-skill-a")
+        );
+        assert!(
+            outcome_b
+                .skills
+                .iter()
+                .all(|skill| skill.name != "extra-skill-b")
+        );
+
+        let outcome_reloaded = skills_manager
+            .skills_for_cwd_with_extra_user_roots(
+                cwd.path(),
+                true,
+                std::slice::from_ref(&extra_root_b_path),
+            )
+            .await;
+        assert!(
+            outcome_reloaded
+                .skills
+                .iter()
+                .any(|skill| skill.name == "extra-skill-b")
+        );
+        assert!(
+            outcome_reloaded
+                .skills
+                .iter()
+                .all(|skill| skill.name != "extra-skill-a")
+        );
+    }
+
+    #[test]
+    fn normalize_extra_user_roots_is_stable_for_equivalent_inputs() {
+        let a = PathBuf::from("/tmp/a");
+        let b = PathBuf::from("/tmp/b");
+
+        let first = normalize_extra_user_roots(&[a.clone(), b.clone(), a.clone()]);
+        let second = normalize_extra_user_roots(&[b, a]);
+
+        assert_eq!(first, second);
+    }
 }