fix: skill discovery in subdirectories#9583
fix: skill discovery in subdirectories#9583rajgandhi1 wants to merge 10 commits intowarpdotdev:masterfrom
Conversation
|
I'm starting a first review of this pull request. I completed the review and posted feedback on this pull request. Comment I'm re-reviewing this pull request in response to a review request. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I reviewed this pull request and requested human review from: @peicodes. Comment I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR changes project skill discovery to add a second tree-based pass for lazy-loaded provider directories and adds a regression test for a lazy .agents directory.
Concerns
- The new pass still misses the case where the containing subdirectory itself is gitignored/lazy-loaded, because traversal never reaches the nested provider directory.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
I'm checking this implementation PR for association with a likely matching ready issue. Powered by Oz |
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR extends skill-directory discovery to handle gitignored lazy-loaded provider directories and adds regression coverage for lazy .agents discovery under subdirectories.
Concerns
- The new parent-lazy probe checks every lazy ignored directory for provider paths, which can register skills from ignored dependency/cache directories such as
node_modulesortarget. That contradicts the stated bounded-scope behavior and creates a security-sensitive skill discovery expansion.
Verdict
Found: 1 critical, 0 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Adds DEPENDENCY_DIR_NAMES denylist (node_modules, target, vendor, …) to prevent lazy-loaded dependency trees from being probed as workspace roots. Adds a regression test verifying that node_modules/.agents/skills/ is never loaded even when node_modules/ appears in the tree as a lazy-loaded entry. Co-Authored-By: Warp <agent@warp.dev>
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR extends project skill discovery to inspect lazy-loaded gitignored directories and adds regression coverage for skills under ignored subdirectories.
Concerns
- The new Case (b) probing trusts every lazy-loaded directory except a hard-coded basename denylist. That leaves unlisted dependency/cache trees able to provide
.agents/skillsor other provider skill directories that will be auto-loaded.
Verdict
Found: 1 critical, 0 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…wnership check in Pass 2 Case (b)
There was a problem hiding this comment.
Overview
This PR extends project skill discovery to handle lazy-loaded provider directories and lazy-loaded parent directories in the repo metadata tree, with regression coverage for ignored .agents paths.
Concerns
- The new manifest-based ownership heuristic can trust ignored dependency package directories and register skills from untrusted installed/vendor code.
Verdict
Found: 1 critical, 0 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR adds a supplemental lazy-directory pass to skill discovery so gitignored provider roots such as .agents/ can still expose their skills/ subdirectory.
Concerns
- The new pass only probes lazy entries whose own directory name is a known provider root, so it still misses the stated case where the containing subdirectory is gitignored and the provider root is never present in the indexed tree.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| return dir | ||
| .path | ||
| .file_name() | ||
| .is_some_and(|name| provider_root_names_lazy.contains(name)); |
There was a problem hiding this comment.
sub-project/), because .agents/ is not present as a child to probe; either probe provider paths under ignored parent dirs or add the filesystem walk described in the PR body.
… gitignored subdirectories
There was a problem hiding this comment.
Overview
This PR extends project skill discovery to cover gitignored lazy-loaded provider directories and lazy parent directories in the repository metadata tree, with regression tests for both cases.
Concerns
- No blocking correctness or security concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR updates project skill discovery to supplement the repo metadata tree scan by probing lazy-loaded ignored directories for known provider skill paths, with regression tests for ignored provider roots and ignored immediate parent directories.
Concerns
- The targeted probe still misses skills when the lazy-loaded directory is an ancestor above the immediate provider parent, so some gitignored subdirectory layouts remain silently undiscovered.
- No security issues were identified in the changed lines.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR extends project skill discovery so gitignored/lazy-loaded provider directories can still be found by probing the filesystem from lazy nodes in the repo metadata tree.
Concerns
- The new filesystem probing runs synchronously from
find_skill_directories_in_tree, which is called from model-context paths during repository updates; large ignored directories can block the app whileread_dirrecurses. - The
.gitpruning only skips the.gitentry itself and still traverses nested repository working trees, so parent repos can discover skills from vendored/submodule checkouts.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| return; | ||
| } | ||
|
|
||
| let Ok(entries) = std::fs::read_dir(dir) else { |
There was a problem hiding this comment.
read_dir recursion runs from find_skill_directories_in_tree, which is called on model-context repo update paths; probing large ignored directories such as node_modules or target can block the app. Move the filesystem probing to a background task or keep it outside the model read path.
| continue; | ||
| } | ||
| // Prune .git to avoid crossing nested repository boundaries. | ||
| if path.file_name().and_then(|n| n.to_str()) == Some(".git") { |
There was a problem hiding this comment.
.git entry does not prune nested repository roots: the traversal still enters nested-repo/ and can discover nested-repo/.agents/skills before ignoring nested-repo/.git. Skip directories that contain a .git marker before probing or recursing into them.
There was a problem hiding this comment.
Overview
This PR supplements skill discovery with an on-filesystem probe for lazy-loaded repository metadata directories so skills under ignored subdirectories can be discovered.
Concerns
- The new filesystem probe follows symlinked provider paths and child directories, which bypasses the repository metadata layer's existing symlink-dir protection and can cause Warp to read/register skills outside the repository.
- Lazy directories that are themselves nested repository roots are probed before the
.gitguard is applied, so nested-repo skills can be attributed to the parent repo scan.
Verdict
Found: 2 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ) { | ||
| 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() { |
There was a problem hiding this comment.
🚨 [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.
| }; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if !path.is_dir() { |
There was a problem hiding this comment.
🚨 [CRITICAL] This is_dir() follows symlinked child directories, letting the recursive probe leave the repository before looking for provider skill paths; use entry.file_type()/symlink_metadata and skip symlink directories during the filesystem walk.
| 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); |
There was a problem hiding this comment.
probe_lazy_subtrees enters each lazy root before applying the .git guard, so a lazy submodule or nested repository root with .agents/skills is registered by the parent repo scan; skip roots containing .git before calling probe_subtree.
captainsafia
added
the
external-contributor
Warp Dev Github Integration
peicodes
requested review from
kevinyang372
and removed request for
kevinyang372 and
peicodes
|
We can't walk the file tree on skill reads because that's potentially a very expensive operation. The likely solution is to modify indexing. The linked issue has been marked ready to spec. Please include your findings in that issue, and work with us to create a product and tech spec. The discussion around the spec step will help us avoid introducing performance issues or regressions. Once the spec is approved, we can implement. |
3 participants
Skills stored in .agents/skills/ (or other provider paths like .claude/skills/) inside a subdirectory of the repo were silently not discovered when that subdirectory — or its .agents/ folder — was gitignored.
Root cause: The indexed file tree (RepoMetadataModel) is built with IgnoredPathStrategy::IncludeLazy: gitignored directories appear in the tree but with loaded: false and empty children. The find_skill_directories_in_tree traversal never reaches skill dirs nested inside them, so only skills at the repo root (when .agents/ is not gitignored there) were discovered.
Fix: Add find_skill_directories_on_fs which walks the filesystem directly using walkdir, bypassing the indexed tree entirely. It skips .git and short-circuits once a skill directory is found. This is called as a supplemental async pass in scan_repository_for_skills alongside the existing tree-based scan. Skills found by both passes are deduplicated by SkillManager::handle_skills_added, which already uses HashSet for skill paths, making double-registration idempotent.
Testing: Added regression tests for find_skill_directories_on_fs covering root skills, subdirectory skills (the failing scenario from #9486), and .git exclusion.
- Bug fix: "Skills in .agents/skills/ inside subdirectories of a git repo are now discovered correctly, even when the subdirectory or its .agents/ folder is gitignored."