diff --git a/crates/goose/src/agents/platform_extensions/summon.rs b/crates/goose/src/agents/platform_extensions/summon.rs index 4d857717..00c80f70 100644 --- a/crates/goose/src/agents/platform_extensions/summon.rs +++ b/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 { /// Returns discovered skills, skipping any whose names are already in `seen`. fn scan_skills_from_dir(dir: &Path, seen: &mut std::collections::HashSet) -> Vec { 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 } }; - 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 sources } +fn collect_skill_files(dir: &Path, visited_dirs: &mut HashSet) -> Vec { + 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( + dir: &Path, + visited_dirs: &mut HashSet, + 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 { 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 { +/// Collect all files in a skill directory recursively, excluding SKILL.md itself. +fn find_supporting_files(directory: &Path, visited_dirs: &mut HashSet) -> Vec { 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 = 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]