diff --git a/app/src/ai/skills/file_watchers/skill_watcher.rs b/app/src/ai/skills/file_watchers/skill_watcher.rs index a762495dc..9e3ebd73f 100644 --- a/app/src/ai/skills/file_watchers/skill_watcher.rs +++ b/app/src/ai/skills/file_watchers/skill_watcher.rs @@ -9,7 +9,8 @@ use super::{ }, utils::{ find_skill_directories_in_tree, is_home_provider_path, is_home_skill_directory, - is_skill_file, read_skills_from_directories, + is_skill_file, probe_lazy_subtrees, read_skills_from_directories, + scan_skill_directories_in_tree, }, }; use watcher::{BulkFilesystemWatcherEvent, HomeDirectoryWatcher, HomeDirectoryWatcherEvent}; @@ -257,13 +258,27 @@ impl SkillWatcher { /// This is called when RepositoryMetadataEvent::RepositoryUpdated fires. fn scan_repository_for_skills(&mut self, repo_path: &Path, ctx: &mut ModelContext) { let repo_metadata = RepoMetadataModel::as_ref(ctx); - - // Find all skill directories in the tree - let skill_dirs = find_skill_directories_in_tree(repo_path, repo_metadata, ctx); - if skill_dirs.is_empty() { + let scan = scan_skill_directories_in_tree(repo_path, repo_metadata, ctx); + if scan.found.is_empty() && scan.lazy_dirs.is_empty() { return; } - Self::spawn_read_skills_from_directories(skill_dirs, ctx); + let found = scan.found; + let lazy_dirs = scan.lazy_dirs; + ctx.spawn( + async move { + let mut skill_dirs = found; + skill_dirs.extend(probe_lazy_subtrees(lazy_dirs)); + read_skills_from_directories(skill_dirs) + }, + move |me, skills, ctx| { + if !skills.is_empty() { + me.register_symlink_watches(&skills, ctx); + let _ = me + .watcher_event_tx + .try_send(SkillWatcherEvent::SkillsAdded { skills }); + } + }, + ); } fn spawn_read_skills_from_directories( @@ -471,17 +486,19 @@ impl SkillWatcher { let mut queued_project_directory_creations_to_requeue: Vec = Vec::new(); let mut skill_dirs_to_read: HashSet = HashSet::new(); + let mut all_lazy_dirs: Vec = Vec::new(); for (repo_path, queued_project_directory_creations) in queued_by_repo_path { - // Find all skill directories in the repository let repo_metadata = RepoMetadataModel::as_ref(ctx); - let skill_dirs = find_skill_directories_in_tree(&repo_path, repo_metadata, ctx); - if skill_dirs.is_empty() { + let scan = scan_skill_directories_in_tree(&repo_path, repo_metadata, ctx); + if scan.found.is_empty() && scan.lazy_dirs.is_empty() { continue; } + all_lazy_dirs.extend(scan.lazy_dirs); for queued_project_directory_creation in queued_project_directory_creations { - let relevant_skill_dirs = skill_dirs + let relevant_skill_dirs = scan + .found .iter() .filter(|skill_dir| { // If the skill_dir is the child of the new directory, we need to read it again @@ -511,7 +528,11 @@ impl SkillWatcher { } ctx.spawn( - async move { read_skills_from_directories(skill_dirs_to_read) }, + async move { + let mut dirs_to_read: Vec = skill_dirs_to_read.into_iter().collect(); + dirs_to_read.extend(probe_lazy_subtrees(all_lazy_dirs)); + read_skills_from_directories(dirs_to_read) + }, move |me, skills, ctx| { if !skills.is_empty() { me.register_symlink_watches(&skills, ctx); diff --git a/app/src/ai/skills/file_watchers/utils.rs b/app/src/ai/skills/file_watchers/utils.rs index 4d664d485..97a9d8946 100644 --- a/app/src/ai/skills/file_watchers/utils.rs +++ b/app/src/ai/skills/file_watchers/utils.rs @@ -14,45 +14,174 @@ use warpui::AppContext; use crate::warp_managed_paths_watcher::warp_managed_skill_dirs; -/// Finds all skill directories in a repository by querying the RepoMetadataModel tree. +/// Max directory depth walked below a lazy node when searching for provider skill dirs. +const MAX_LAZY_WALK_DEPTH: usize = 3; + +/// Result of the fast (in-memory) phase of skill directory discovery. +pub struct SkillDirectoryScan { + /// Skill directories found by tree traversal — safe to use on the model path. + pub found: Vec, + /// Lazy-loaded dirs whose subtrees need recursive filesystem probing (Case b). + /// Pass these to [`probe_lazy_subtrees`] off the model path. + pub lazy_dirs: Vec, +} + +/// Fast, in-memory phase: queries the repo tree and returns discovered skill dirs +/// plus any lazy ancestor dirs that still need probing. /// -/// Returns a list of paths to skill directories (e.g., `/repo/.agents/skills/`, `/repo/sub/.claude/skills/`). -pub fn find_skill_directories_in_tree( +/// - **Pass 1:** fully-loaded dirs whose path ends with a known provider skills suffix. +/// - **Pass 2 Case (a):** lazy dirs named like a provider root (`.agents`, `.claude`, …) +/// — probed inline with a single `is_dir()` since they have a known structure. +/// - **Pass 2 Case (b):** all other lazy dirs are returned in `lazy_dirs` for the +/// caller to probe off the model path via [`probe_lazy_subtrees`]. +pub fn scan_skill_directories_in_tree( repo_path: &Path, repo_metadata: &RepoMetadataModel, ctx: &AppContext, -) -> Vec { - // Collect provider skills paths (e.g., ".agents/skills", ".claude/skills") - let skill_path_suffixes: Vec<&Path> = SKILL_PROVIDER_DEFINITIONS +) -> SkillDirectoryScan { + let Some(id) = repo_metadata::RepositoryIdentifier::try_local(repo_path) else { + return SkillDirectoryScan { + found: Vec::new(), + lazy_dirs: Vec::new(), + }; + }; + + let skill_path_suffixes: Vec = SKILL_PROVIDER_DEFINITIONS .iter() - .map(|p| p.skills_path.as_path()) + .map(|p| p.skills_path.to_string_lossy().into_owned()) .collect(); - // Filter during traversal: only collect directories that end with a skill provider path. - // The filter rejects files and non-matching directories, avoiding intermediate allocations. + let provider_root_names: HashSet = SKILL_PROVIDER_DEFINITIONS + .iter() + .filter_map(|p| { + p.skills_path + .parent() + .and_then(Path::file_name) + .and_then(|n| n.to_str()) + .map(str::to_owned) + }) + .collect(); + + // Pass 1: loaded dirs whose path ends with a known provider skills suffix. + let suffixes_1 = skill_path_suffixes.clone(); let args = GetContentsArgs::default().with_filter(move |content| { let RepoContent::Directory(dir) = content else { return false; }; - skill_path_suffixes + suffixes_1 .iter() - .any(|suffix| dir.path.ends_with(&suffix.to_string_lossy())) + .any(|suffix| dir.path.ends_with(suffix.as_str())) }); - let Some(id) = repo_metadata::RepositoryIdentifier::try_local(repo_path) else { - return Vec::new(); - }; - repo_metadata + let mut found: Vec = repo_metadata .get_repo_contents(&id, args, ctx) .unwrap_or_default() .into_iter() - // Only directories should reach this iterator due to the GetContentsArgs::filter. - // Keep the File arm for exhaustive matching in case RepoContent grows new variants. .map(|content| match content { RepoContent::Directory(dir) => dir.path.to_local_path_lossy(), RepoContent::File(f) => f.path.to_local_path_lossy(), }) - .collect() + .collect(); + + // Pass 2: collect all lazy-loaded dirs. + let args_lazy = GetContentsArgs::default() + .include_ignored() + .with_filter(move |content| { + let RepoContent::Directory(dir) = content else { + return false; + }; + !dir.loaded + }); + + let all_lazy: Vec = repo_metadata + .get_repo_contents(&id, args_lazy, ctx) + .unwrap_or_default() + .into_iter() + .map(|content| match content { + RepoContent::Directory(dir) => dir.path.to_local_path_lossy(), + RepoContent::File(f) => f.path.to_local_path_lossy(), + }) + .collect(); + + let mut found_set: HashSet = found.iter().cloned().collect(); + let mut lazy_dirs: Vec = Vec::new(); + + for lazy_dir in all_lazy { + let dir_name = lazy_dir.file_name().and_then(|n| n.to_str()).unwrap_or(""); + if provider_root_names.contains(dir_name) { + // Case (a): lazy dir is a provider root — single cheap probe for {dir}/skills. + let skills_path = lazy_dir.join("skills"); + if !found_set.contains(&skills_path) && skills_path.is_dir() { + found_set.insert(skills_path.clone()); + found.push(skills_path); + } + } else { + // Case (b): lazy dir may be an ancestor — defer to probe_lazy_subtrees. + lazy_dirs.push(lazy_dir); + } + } + + SkillDirectoryScan { found, lazy_dirs } +} + +/// Walks each dir up to `MAX_LAZY_WALK_DEPTH` levels, probing for provider skill dirs. +/// Skips subdirectories that contain a `.git` entry (nested repository roots). +/// Intended to run off the model path (e.g. inside `ctx.spawn`). +pub fn probe_lazy_subtrees(lazy_dirs: impl IntoIterator) -> Vec { + let mut result = Vec::new(); + let mut result_set = HashSet::new(); + for dir in lazy_dirs { + probe_subtree(&dir, 0, &mut result_set, &mut result); + } + result +} + +fn probe_subtree( + dir: &Path, + depth: usize, + result_set: &mut HashSet, + result: &mut Vec, +) { + for provider in SKILL_PROVIDER_DEFINITIONS.iter() { + let skills_path = dir.join(&provider.skills_path); + if !result_set.contains(&skills_path) && skills_path.is_dir() { + result_set.insert(skills_path.clone()); + result.push(skills_path); + } + } + + if depth >= MAX_LAZY_WALK_DEPTH { + return; + } + + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if !path.is_dir() { + continue; + } + // Skip nested repository roots — their skills belong to their own scan. + if path.join(".git").exists() { + continue; + } + probe_subtree(&path, depth + 1, result_set, result); + } +} + +/// Convenience wrapper that runs both phases synchronously. +/// Use [`scan_skill_directories_in_tree`] + [`probe_lazy_subtrees`] when calling +/// from a model context so the filesystem walk runs off the model path. +pub fn find_skill_directories_in_tree( + repo_path: &Path, + repo_metadata: &RepoMetadataModel, + ctx: &AppContext, +) -> Vec { + let scan = scan_skill_directories_in_tree(repo_path, repo_metadata, ctx); + let mut found = scan.found; + found.extend(probe_lazy_subtrees(scan.lazy_dirs)); + found } /// Reads all skills from the given skill directories. @@ -70,22 +199,19 @@ pub fn is_skill_file(path: &Path) -> bool { } static SKILL_PROVIDER_PATHS: LazyLock> = LazyLock::new(|| { - // Collect the skill provider paths from the definitions SKILL_PROVIDER_DEFINITIONS .iter() .map(|p| p.skills_path.to_string_lossy().to_string()) .collect() }); -// Pattern: {prefix}/{provider_path}/{skill-name}/SKILL.md -// where provider_path is 2 parts (e.g., ".agents/skills") and skill-name is 1 part +// Matches {prefix}/{provider_path}/{skill-name}/SKILL.md (provider_path is 2 components). #[cfg(not(target_os = "windows"))] static SKILL_FILE_PATTERN: LazyLock = LazyLock::new(|| { Regex::new(r"(.+)/([^/]+/[^/]+)/[^/]+/SKILL\.md$") .expect("Failed to compile skill file pattern") }); -// On windows, the path separator is \ #[cfg(target_os = "windows")] static SKILL_FILE_PATTERN: LazyLock = LazyLock::new(|| { Regex::new(r"(.+)\\([^\\]+\\[^\\]+)\\[^\\]+\\SKILL\.md$") @@ -120,19 +246,13 @@ pub fn extract_skill_parent_directory(path: &Path) -> Result { Err(anyhow::anyhow!("Not a skill path: {}", path.display())) } -/// Check if this path is a skill directory under a home directory provider path -/// E.g. ~/.agents/skills/skill-name +/// Returns true if `path` is a skill directory under a home provider path +/// (e.g. `~/.agents/skills/skill-name`). pub fn is_home_skill_directory(path: &Path) -> bool { - let parent_directory = path.parent(); - if let Some(parent_directory) = parent_directory { - is_home_provider_path(parent_directory) - } else { - false - } + path.parent().is_some_and(is_home_provider_path) } -/// Check if this path is a home directory provider path -/// E.g. ~/.agents/skills +/// Returns true if `path` is a home provider skills path (e.g. `~/.agents/skills`). pub fn is_home_provider_path(path: &Path) -> bool { SKILL_PROVIDER_DEFINITIONS.iter().any(|provider| { if provider.provider == SkillProvider::Warp { diff --git a/app/src/ai/skills/file_watchers/utils_tests.rs b/app/src/ai/skills/file_watchers/utils_tests.rs index 0efbeca29..d9804524d 100644 --- a/app/src/ai/skills/file_watchers/utils_tests.rs +++ b/app/src/ai/skills/file_watchers/utils_tests.rs @@ -724,3 +724,224 @@ fn find_skill_directories_in_tree_empty_repo() { }); }); } + +/// Regression test for warpdotdev/warp#9486 — provider root is lazy-loaded. +/// +/// Skills inside a gitignored `.agents/` directory were not discovered because +/// the indexed file tree represents gitignored directories as lazy-loaded entries +/// with empty children. The fix performs a targeted `is_dir()` check for +/// `{lazy_provider_dir}/skills` on disk for every unloaded provider directory +/// found in the tree, without a broad filesystem walk. +/// +/// This test constructs a tree where `sub-project/.agents/` is present but +/// `loaded: false` (as it would be when gitignored), while the real +/// `sub-project/.agents/skills/` directory exists on disk. +#[test] +fn find_skill_directories_in_tree_finds_skills_in_lazy_loaded_provider_dir() { + VirtualFS::test("find_lazy_loaded_skills", |dirs, mut vfs| { + let repo = dirs.tests().join("repo"); + + // Create the actual filesystem structure: sub-project/.agents/skills/sub-skill/SKILL.md + vfs.mkdir("repo/sub-project/.agents/skills/sub-skill") + .with_files(vec![Stub::FileWithContent( + "repo/sub-project/.agents/skills/sub-skill/SKILL.md", + "---\nname: sub-skill\ndescription: test\n---\n# sub-skill", + )]); + + // Build a tree that mirrors what the indexer produces when .agents/ is gitignored: + // sub-project/ is fully loaded, but .agents/ inside it is lazy-loaded (loaded: false, + // children: []) because it is gitignored. The skills/ subdirectory is absent from + // the tree, which is exactly the scenario that caused the original bug. + let agents_dir = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local( + &repo.join("sub-project/.agents"), + ) + .unwrap(), + children: vec![], // lazy-loaded: children not indexed + ignored: true, + loaded: false, + }); + let sub_project = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local( + &repo.join("sub-project"), + ) + .unwrap(), + children: vec![agents_dir], + ignored: false, + loaded: true, + }); + let root = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local(&repo).unwrap(), + children: vec![sub_project], + ignored: false, + loaded: true, + }); + + App::test((), |mut app| async move { + let watcher = app.add_singleton_model(DirectoryWatcher::new); + app.add_singleton_model(|_| DetectedRepositories::default()); + let repo_handle = watcher.update(&mut app, |w, ctx| { + w.add_directory( + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(), + ctx, + ) + .unwrap() + }); + let state = FileTreeState::new(root, vec![], Some(repo_handle)); + + let model_handle = app.add_singleton_model(RepoMetadataModel::new); + model_handle.update(&mut app, |model, ctx| { + let key = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(); + model.insert_test_state(key, state, ctx); + }); + + model_handle.read(&app, |model, ctx| { + let skill_dirs = find_skill_directories_in_tree(&repo, model, ctx); + assert_eq!( + skill_dirs.len(), + 1, + "expected lazy-loaded .agents/ skills dir" + ); + assert!(skill_dirs.contains(&repo.join("sub-project/.agents/skills"))); + + let skills = read_skills_from_directories(skill_dirs); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "sub-skill"); + }); + }); + }); +} + +/// Pass 2 Case (b): lazy dir is a direct parent of the provider root. +#[test] +fn find_skill_directories_in_tree_finds_skills_when_parent_dir_is_lazy_loaded() { + VirtualFS::test("find_lazy_parent_skills", |dirs, mut vfs| { + let repo = dirs.tests().join("repo"); + + vfs.mkdir("repo/sub-project/.agents/skills/sub-skill") + .with_files(vec![Stub::FileWithContent( + "repo/sub-project/.agents/skills/sub-skill/SKILL.md", + "---\nname: sub-skill\ndescription: test\n---\n# sub-skill", + )]); + + // sub-project/ is lazy-loaded (ignored: true, loaded: false, no children), + // so .agents/ is entirely absent from the tree. + let sub_project = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local( + &repo.join("sub-project"), + ) + .unwrap(), + children: vec![], + ignored: true, + loaded: false, + }); + let root = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local(&repo).unwrap(), + children: vec![sub_project], + ignored: false, + loaded: true, + }); + + App::test((), |mut app| async move { + let watcher = app.add_singleton_model(DirectoryWatcher::new); + app.add_singleton_model(|_| DetectedRepositories::default()); + let repo_handle = watcher.update(&mut app, |w, ctx| { + w.add_directory( + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(), + ctx, + ) + .unwrap() + }); + let state = FileTreeState::new(root, vec![], Some(repo_handle)); + + let model_handle = app.add_singleton_model(RepoMetadataModel::new); + model_handle.update(&mut app, |model, ctx| { + let key = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(); + model.insert_test_state(key, state, ctx); + }); + + model_handle.read(&app, |model, ctx| { + let skill_dirs = find_skill_directories_in_tree(&repo, model, ctx); + assert_eq!(skill_dirs.len(), 1); + assert!(skill_dirs.contains(&repo.join("sub-project/.agents/skills"))); + + let skills = read_skills_from_directories(skill_dirs); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "sub-skill"); + }); + }); + }); +} + +/// Pass 2 Case (b): lazy dir is a deeper ancestor of the provider root +/// (e.g. `packages/` is lazy, skills live at `packages/frontend/.agents/skills/`). +#[test] +fn find_skill_directories_in_tree_finds_skills_in_nested_lazy_ancestor() { + VirtualFS::test("find_nested_lazy_ancestor_skills", |dirs, mut vfs| { + let repo = dirs.tests().join("repo"); + + vfs.mkdir("repo/packages/frontend/.agents/skills/nested-skill") + .with_files(vec![Stub::FileWithContent( + "repo/packages/frontend/.agents/skills/nested-skill/SKILL.md", + "---\nname: nested-skill\ndescription: test\n---\n# nested-skill", + )]); + + // packages/ is lazy-loaded — frontend/ and .agents/ are absent from the tree. + let packages = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local( + &repo.join("packages"), + ) + .unwrap(), + children: vec![], + ignored: true, + loaded: false, + }); + let root = Entry::Directory(DirectoryEntry { + path: warp_util::standardized_path::StandardizedPath::try_from_local(&repo).unwrap(), + children: vec![packages], + ignored: false, + loaded: true, + }); + + App::test((), |mut app| async move { + let watcher = app.add_singleton_model(DirectoryWatcher::new); + app.add_singleton_model(|_| DetectedRepositories::default()); + let repo_handle = watcher.update(&mut app, |w, ctx| { + w.add_directory( + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(), + ctx, + ) + .unwrap() + }); + let state = FileTreeState::new(root, vec![], Some(repo_handle)); + + let model_handle = app.add_singleton_model(RepoMetadataModel::new); + model_handle.update(&mut app, |model, ctx| { + let key = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized(&repo) + .unwrap(); + model.insert_test_state(key, state, ctx); + }); + + model_handle.read(&app, |model, ctx| { + let skill_dirs = find_skill_directories_in_tree(&repo, model, ctx); + assert_eq!(skill_dirs.len(), 1); + assert!( + skill_dirs.contains(&repo.join("packages/frontend/.agents/skills")), + "expected packages/frontend/.agents/skills, got: {skill_dirs:?}" + ); + + let skills = read_skills_from_directories(skill_dirs); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "nested-skill"); + }); + }); + }); +}