fix: recursively discover nested skills under configured roots (#8082)

Signed-off-by: jamestotah <135163520+jamestotah@users.noreply.github.com>
Signed-off-by: Douwe Osinga <douwe@squareup.com>
Co-authored-by: Douwe Osinga <douwe@squareup.com>

crates/goose/src/agents/platform_extensions/summon.rs

@@ -26,7 +26,7 @@ use rmcp::model::{
     ServerCapabilities, ServerNotification, Tool,
 };
 use serde::Deserialize;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicU32, AtomicU64, Ordering};
 use std::sync::Arc;
@@ -214,22 +214,11 @@ fn parse_agent_content(content: &str, path: PathBuf) -> Option<Source> {
 /// Returns discovered skills, skipping any whose names are already in `seen`.
 fn scan_skills_from_dir(dir: &Path, seen: &mut std::collections::HashSet<String>) -> Vec<Source> {
     let mut sources = Vec::new();
-    let entries = match std::fs::read_dir(dir) {
-        Ok(e) => e,
-        Err(_) => return sources,
-    };
-
-    for entry in entries.flatten() {
-        let skill_dir = entry.path();
-        if !skill_dir.is_dir() {
+    let mut visited_dirs = HashSet::new();
+    for skill_file in collect_skill_files(dir, &mut visited_dirs) {
+        let Some(skill_dir) = skill_file.parent() else {
             continue;
-        }
-
-        let skill_file = skill_dir.join("SKILL.md");
-        if !skill_file.exists() {
-            continue;
-        }
-
+        };
         let content = match std::fs::read_to_string(&skill_file) {
             Ok(c) => c,
             Err(e) => {
@@ -238,9 +227,11 @@ fn scan_skills_from_dir(dir: &Path, seen: &mut std::collections::HashSet<String>
             }
         };
 
-        if let Some(mut source) = parse_skill_content(&content, skill_dir.clone()) {
+        if let Some(mut source) = parse_skill_content(&content, skill_dir.to_path_buf()) {
             if !seen.contains(&source.name) {
-                source.supporting_files = find_supporting_files(&skill_dir, &skill_file);
+                let mut visited_support_dirs = HashSet::new();
+                source.supporting_files =
+                    find_supporting_files(skill_dir, &mut visited_support_dirs);
                 seen.insert(source.name.clone());
                 sources.push(source);
             }
@@ -249,6 +240,66 @@ fn scan_skills_from_dir(dir: &Path, seen: &mut std::collections::HashSet<String>
     sources
 }
 
+fn collect_skill_files(dir: &Path, visited_dirs: &mut HashSet<PathBuf>) -> Vec<PathBuf> {
+    let mut skill_files = Vec::new();
+
+    walk_files_recursively(
+        dir,
+        visited_dirs,
+        &mut |path| !should_skip_skill_walk_dir(path),
+        &mut |path| {
+            if path.file_name().and_then(|name| name.to_str()) == Some("SKILL.md") {
+                skill_files.push(path.to_path_buf());
+            }
+        },
+    );
+
+    skill_files
+}
+
+fn should_skip_skill_walk_dir(path: &Path) -> bool {
+    matches!(
+        path.file_name().and_then(|name| name.to_str()),
+        Some(".git") | Some(".hg") | Some(".svn")
+    )
+}
+
+fn walk_files_recursively<F, G>(
+    dir: &Path,
+    visited_dirs: &mut HashSet<PathBuf>,
+    should_descend: &mut G,
+    visit_file: &mut F,
+) where
+    F: FnMut(&Path),
+    G: FnMut(&Path) -> bool,
+{
+    let canonical_dir = match std::fs::canonicalize(dir) {
+        Ok(path) => path,
+        Err(_) => return,
+    };
+
+    if !visited_dirs.insert(canonical_dir) {
+        return;
+    }
+
+    let entries = match std::fs::read_dir(dir) {
+        Ok(e) => e,
+        Err(_) => return,
+    };
+
+    for entry in entries.flatten() {
+        let path = entry.path();
+
+        if path.is_dir() {
+            if should_descend(&path) {
+                walk_files_recursively(&path, visited_dirs, should_descend, visit_file);
+            }
+        } else if path.is_file() {
+            visit_file(&path);
+        }
+    }
+}
+
 fn scan_recipes_from_dir(
     dir: &Path,
     kind: SourceKind,
@@ -427,29 +478,26 @@ fn discover_filesystem_sources(working_dir: &Path) -> Vec<Source> {
     sources
 }
 
-/// Collect all files in a skill directory (excluding SKILL.md itself),
-/// recursing one level into subdirectories.
-fn find_supporting_files(directory: &Path, skill_file: &Path) -> Vec<PathBuf> {
+/// Collect all files in a skill directory recursively, excluding SKILL.md itself.
+fn find_supporting_files(directory: &Path, visited_dirs: &mut HashSet<PathBuf>) -> Vec<PathBuf> {
     let mut files = Vec::new();
-    let entries = match std::fs::read_dir(directory) {
-        Ok(e) => e,
-        Err(_) => return files,
-    };
-    for entry in entries.flatten() {
-        let path = entry.path();
-        if path.is_file() && path != skill_file {
-            files.push(path);
-        } else if path.is_dir() {
-            if let Ok(sub_entries) = std::fs::read_dir(&path) {
-                for sub_entry in sub_entries.flatten() {
-                    let sub_path = sub_entry.path();
-                    if sub_path.is_file() {
-                        files.push(sub_path);
-                    }
-                }
+
+    walk_files_recursively(
+        directory,
+        visited_dirs,
+        &mut |path| !should_skip_skill_walk_dir(path) && !path.join("SKILL.md").is_file(),
+        &mut |path| {
+            let is_skill_md = path
+                .file_name()
+                .and_then(|n| n.to_str())
+                .map(|n| n == "SKILL.md")
+                .unwrap_or(false);
+            if !is_skill_md {
+                files.push(path.to_path_buf());
             }
-        }
-    }
+        },
+    );
+
     files
 }
 
@@ -2050,22 +2098,26 @@ You review code."#;
         let temp_dir = TempDir::new().unwrap();
 
         let skill_dir = temp_dir.path().join(".goose/skills/my-skill");
-        fs::create_dir_all(&skill_dir).unwrap();
+        fs::create_dir_all(skill_dir.join("templates/nested")).unwrap();
         fs::write(
             skill_dir.join("SKILL.md"),
             "---\nname: my-skill\ndescription: A skill with scripts\n---\nRun check_all.sh",
         )
         .unwrap();
         fs::write(skill_dir.join("myscript.sh"), "#!/bin/bash\necho ok").unwrap();
-        fs::create_dir(skill_dir.join("templates")).unwrap();
         fs::write(skill_dir.join("templates/report.txt"), "template content").unwrap();
+        fs::write(
+            skill_dir.join("templates/nested/checklist.txt"),
+            "nested template content",
+        )
+        .unwrap();
 
         let client = SummonClient::new(create_test_context()).unwrap();
         let sources = client.discover_filesystem_sources(temp_dir.path());
 
         let skill = sources.iter().find(|s| s.name == "my-skill").unwrap();
         assert_eq!(skill.path, skill_dir);
-        assert_eq!(skill.supporting_files.len(), 2);
+        assert_eq!(skill.supporting_files.len(), 3);
 
         let file_names: Vec<String> = skill
             .supporting_files
@@ -2074,6 +2126,167 @@ You review code."#;
             .collect();
         assert!(file_names.contains(&"myscript.sh".to_string()));
         assert!(file_names.contains(&"report.txt".to_string()));
+        assert!(file_names.contains(&"checklist.txt".to_string()));
+    }
+
+    #[tokio::test]
+    async fn test_nested_claude_catalog_skills_discovered() {
+        let temp_dir = TempDir::new().unwrap();
+
+        let root_skill_file = temp_dir.path().join(".claude/skills/SKILL.md");
+        fs::create_dir_all(root_skill_file.parent().unwrap()).unwrap();
+        fs::write(
+            &root_skill_file,
+            "---\nname: root-skill\ndescription: Root level skill\n---\nRoot content",
+        )
+        .unwrap();
+
+        let nested_skill_dir = temp_dir.path().join(".claude/skills/catalog/internal/ai");
+        fs::create_dir_all(&nested_skill_dir).unwrap();
+        fs::write(
+            nested_skill_dir.join("SKILL.md"),
+            "---\nname: nested-skill\ndescription: Nested catalog skill\n---\nNested content",
+        )
+        .unwrap();
+
+        let client = SummonClient::new(create_test_context()).unwrap();
+        let sources = client.discover_filesystem_sources(temp_dir.path());
+
+        let root_skill = sources.iter().find(|s| s.name == "root-skill").unwrap();
+        assert_eq!(root_skill.path, temp_dir.path().join(".claude/skills"));
+
+        let nested_skill = sources.iter().find(|s| s.name == "nested-skill").unwrap();
+        assert_eq!(nested_skill.path, nested_skill_dir);
+    }
+
+    #[tokio::test]
+    async fn test_root_skill_supporting_files_exclude_nested_skill_subtrees() {
+        let temp_dir = TempDir::new().unwrap();
+
+        let root_skill_dir = temp_dir.path().join(".claude/skills");
+        fs::create_dir_all(&root_skill_dir).unwrap();
+        fs::write(
+            root_skill_dir.join("SKILL.md"),
+            "---\nname: root-skill\ndescription: Root level skill\n---\nRoot content",
+        )
+        .unwrap();
+        fs::write(root_skill_dir.join("README.md"), "root readme").unwrap();
+
+        let nested_skill_dir = root_skill_dir.join("catalog/internal/ai");
+        fs::create_dir_all(&nested_skill_dir).unwrap();
+        fs::write(
+            nested_skill_dir.join("SKILL.md"),
+            "---\nname: nested-skill\ndescription: Nested catalog skill\n---\nNested content",
+        )
+        .unwrap();
+        fs::write(nested_skill_dir.join("notes.md"), "nested notes").unwrap();
+
+        let client = SummonClient::new(create_test_context()).unwrap();
+        let sources = client.discover_filesystem_sources(temp_dir.path());
+
+        let root_skill = sources.iter().find(|s| s.name == "root-skill").unwrap();
+        assert!(root_skill
+            .supporting_files
+            .contains(&root_skill_dir.join("README.md")));
+        assert!(!root_skill
+            .supporting_files
+            .contains(&nested_skill_dir.join("SKILL.md")));
+        assert!(!root_skill
+            .supporting_files
+            .contains(&nested_skill_dir.join("notes.md")));
+    }
+
+    #[tokio::test]
+    async fn test_skill_discovery_preserves_dot_prefixed_paths() {
+        let temp_dir = TempDir::new().unwrap();
+
+        let dot_skill_dir = temp_dir.path().join(".claude/skills/.team");
+        fs::create_dir_all(&dot_skill_dir).unwrap();
+        fs::write(
+            dot_skill_dir.join("SKILL.md"),
+            "---\nname: team-skill\ndescription: Dot skill\n---\nTeam content",
+        )
+        .unwrap();
+        fs::write(dot_skill_dir.join(".env.example"), "EXAMPLE=1").unwrap();
+
+        let git_skill_dir = temp_dir.path().join(".claude/skills/.git/hidden-skill");
+        fs::create_dir_all(&git_skill_dir).unwrap();
+        fs::write(
+            git_skill_dir.join("SKILL.md"),
+            "---\nname: hidden-git-skill\ndescription: Hidden git skill\n---\nHidden content",
+        )
+        .unwrap();
+
+        let client = SummonClient::new(create_test_context()).unwrap();
+        let sources = client.discover_filesystem_sources(temp_dir.path());
+
+        let dot_skill = sources.iter().find(|s| s.name == "team-skill").unwrap();
+        assert_eq!(dot_skill.path, dot_skill_dir);
+        assert!(dot_skill
+            .supporting_files
+            .contains(&dot_skill_dir.join(".env.example")));
+        assert!(!sources.iter().any(|s| s.name == "hidden-git-skill"));
+    }
+
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_symlinked_skill_directory_is_discovered() {
+        let temp_dir = TempDir::new().unwrap();
+
+        let shared_skill_dir = temp_dir.path().join("shared-skills/ai");
+        fs::create_dir_all(&shared_skill_dir).unwrap();
+        fs::write(
+            shared_skill_dir.join("SKILL.md"),
+            "---\nname: shared-ai\ndescription: Shared skill\n---\nShared content",
+        )
+        .unwrap();
+        fs::write(shared_skill_dir.join("notes.md"), "shared notes").unwrap();
+
+        let linked_catalog_dir = temp_dir.path().join(".claude/skills/catalog/internal");
+        fs::create_dir_all(&linked_catalog_dir).unwrap();
+        std::os::unix::fs::symlink(&shared_skill_dir, linked_catalog_dir.join("ai")).unwrap();
+
+        let client = SummonClient::new(create_test_context()).unwrap();
+        let sources = client.discover_filesystem_sources(temp_dir.path());
+
+        let skill = sources.iter().find(|s| s.name == "shared-ai").unwrap();
+        assert_eq!(skill.path, linked_catalog_dir.join("ai"));
+        assert!(skill
+            .supporting_files
+            .contains(&linked_catalog_dir.join("ai/notes.md")));
+    }
+
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_skill_discovery_avoids_symlink_cycles() {
+        let temp_dir = TempDir::new().unwrap();
+
+        let skill_dir = temp_dir.path().join(".goose/skills/my-skill");
+        fs::create_dir_all(skill_dir.join("refs")).unwrap();
+        fs::write(
+            skill_dir.join("SKILL.md"),
+            "---\nname: my-skill\ndescription: A skill with a loop\n---\nLoop safe",
+        )
+        .unwrap();
+        fs::write(skill_dir.join("refs/guide.md"), "guide content").unwrap();
+        std::os::unix::fs::symlink(&skill_dir, skill_dir.join("refs/loop")).unwrap();
+
+        let client = SummonClient::new(create_test_context()).unwrap();
+        let sources = client.discover_filesystem_sources(temp_dir.path());
+
+        let skill = sources.iter().find(|s| s.name == "my-skill").unwrap();
+        assert_eq!(skill.path, skill_dir);
+        assert!(skill
+            .supporting_files
+            .contains(&skill_dir.join("refs/guide.md")));
+        assert_eq!(
+            skill
+                .supporting_files
+                .iter()
+                .filter(|path| *path == &skill_dir.join("refs/guide.md"))
+                .count(),
+            1
+        );
     }
 
     #[tokio::test]