Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 87 additions & 12 deletions app/src/ai/skills/file_watchers/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,44 +5,75 @@ use std::{
};

use ai::skills::{
home_skills_path, read_skills, ParsedSkill, SkillProvider, SKILL_PROVIDER_DEFINITIONS,
ParsedSkill, SKILL_PROVIDER_DEFINITIONS, SkillProvider, home_skills_path, read_skills,
};
use anyhow::Error;
use regex::Regex;
use repo_metadata::{local_model::GetContentsArgs, RepoContent, RepoMetadataModel};
use repo_metadata::{RepoContent, RepoMetadataModel, local_model::GetContentsArgs};
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.
///
/// Returns a list of paths to skill directories (e.g., `/repo/.agents/skills/`, `/repo/sub/.claude/skills/`).
///
/// Two passes are used to handle gitignored provider directories:
///
/// **Pass 1 — loaded skill dirs:** Standard tree traversal collecting directories that
/// end with a known provider skills path (e.g. `.agents/skills`). Gitignored directories
/// are skipped here because they are lazy-loaded with empty children in the tree.
///
/// **Pass 2 — lazy-loaded provider dirs:** Traversal with `include_ignored: true` to find
/// directories that are lazy-loaded (`loaded: false`) and named like a provider root
/// (`.agents`, `.claude`, …). For each one a single `is_dir()` check is made on disk
/// for `{provider_dir}/skills`. This catches the case where `sub-project/.agents/` is
/// gitignored so its children were not indexed, while remaining safe: only directories
/// already registered in the tree are inspected, which prevents dependency trees such as
/// `node_modules` from being reached (their parent is itself lazy-loaded, so their
/// children never appear in the tree at all).
pub fn find_skill_directories_in_tree(
repo_path: &Path,
repo_metadata: &RepoMetadataModel,
ctx: &AppContext,
) -> Vec<PathBuf> {
// Collect provider skills paths (e.g., ".agents/skills", ".claude/skills")
let skill_path_suffixes: Vec<&Path> = SKILL_PROVIDER_DEFINITIONS
let Some(id) = repo_metadata::RepositoryIdentifier::try_local(repo_path) else {
return Vec::new();
};

// Collect provider skills paths (e.g. ".agents/skills", ".claude/skills") and the
// corresponding provider root names (e.g. ".agents", ".claude") for the second pass.
let skill_path_suffixes: Vec<String> = SKILL_PROVIDER_DEFINITIONS
.iter()
.map(|p| p.skills_path.as_path())
.map(|p| p.skills_path.to_string_lossy().into_owned())
.collect();

let provider_root_names: HashSet<String> = 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: find fully-loaded skill directories ───────────────────────────
//
// 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 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 result: Vec<PathBuf> = repo_metadata
.get_repo_contents(&id, args, ctx)
.unwrap_or_default()
.into_iter()
Expand All @@ -52,7 +83,51 @@ pub fn find_skill_directories_in_tree(
RepoContent::Directory(dir) => dir.path.to_local_path_lossy(),
RepoContent::File(f) => f.path.to_local_path_lossy(),
})
.collect()
.collect();

// ── Pass 2: check lazy-loaded provider directories ────────────────────────
//
// Gitignored provider dirs (e.g. `sub-project/.agents/`) are present in the tree
// but have no children because they were lazy-loaded. Re-traverse with
// `include_ignored: true` so we can see them, then do a single `is_dir()` check
// on disk for `{provider_dir}/skills`.
//
// This is safe: only directories already registered in the tree are inspected.
// Dependency subtrees like `node_modules` are themselves lazy-loaded, so their
// children (including any `.agents/` inside them) are never in the tree.
let result_set: HashSet<PathBuf> = result.iter().cloned().collect();
let args_lazy = GetContentsArgs::default()
.include_ignored()
.with_filter(move |content| {
let RepoContent::Directory(dir) = content else {
return false;
};
// Only unloaded provider-root directories need a disk check.
!dir.loaded
Comment thread
rajgandhi1 marked this conversation as resolved.
&& dir
.path
.file_name()
.is_some_and(|name| provider_root_names.contains(name))
});

let lazy_provider_dirs: Vec<PathBuf> = 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();

for provider_dir in lazy_provider_dirs {
let skills_path = provider_dir.join("skills");
if !result_set.contains(&skills_path) && skills_path.is_dir() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 [CRITICAL] is_dir() follows symlinks, so an ignored lazy directory can contain .agents -> ~/.agents and this probe will read/register home skills as project skills; reject provider paths with symlink components or canonicalize and ensure they remain under the lazy root before accepting them.

result.push(skills_path);
}
}

result
}

/// Reads all skills from the given skill directories.
Expand Down
88 changes: 87 additions & 1 deletion app/src/ai/skills/file_watchers/utils_tests.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use repo_metadata::{
DirectoryWatcher, RepoMetadataModel,
entry::{DirectoryEntry, Entry, FileMetadata},
file_tree_store::FileTreeState,
repositories::DetectedRepositories,
DirectoryWatcher, RepoMetadataModel,
};
use virtual_fs::{Stub, VirtualFS};
use warpui::App;
Expand Down Expand Up @@ -724,3 +724,89 @@ fn find_skill_directories_in_tree_empty_repo() {
});
});
}

/// Regression test for warpdotdev/warp#9486.
///
/// 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);
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");
});
});
});
}