Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
47 changes: 38 additions & 9 deletions crates/repo_metadata/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -992,22 +992,33 @@ fn descend_allowlist_matches(suffix: &[Component<'_>]) -> bool {
/// a watch on) the directory at `path`.
///
/// Directories inside `.git/` follow the watcher allowlist, force-included
/// paths are always watched even when gitignored, and any other gitignored
/// directory is pruned so we don't register watches on `node_modules`, build
/// output, vendored deps, etc.
/// paths are always watched even when gitignored, directory symlinks are
/// pruned to avoid following trees outside the repository, and any other
/// gitignored directory is pruned so we don't register watches on
/// `node_modules`, build output, vendored deps, etc.
pub fn should_watch_repo_directory(
path: &Path,
repo_root: &Path,
gitignores: &[Gitignore],
force_included_paths: &[PathBuf],
) -> bool {
if is_git_internal_path(path) {
return should_watch_directory_in_git_path(path);
}

// Do not follow directory symlinks while recursively registering watches.
// A repository symlink such as `result -> /nix/store/...` can otherwise
// make the watcher traverse a large tree outside the repository. Keep
// explicitly force-included paths working for project-skill providers,
// which intentionally support symlinked skill directories.
if matches_force_included_path(path, force_included_paths) {
return true;
}

if is_within_symlink(path, repo_root) {
return false;
}

if is_git_internal_path(path) {
return should_watch_directory_in_git_path(path);
}

!matches_gitignores(
path,
path.is_dir(),
Expand All @@ -1016,6 +1027,22 @@ pub fn should_watch_repo_directory(
)
}

/// Returns whether `path` is a symlink or is below one.
///
/// The recursive watcher requires this check to be monotonic: if a symlinked
/// directory is rejected, its descendants must be rejected as well even
/// though their individual paths are not themselves symlinks.
fn is_within_symlink(path: &Path, repo_root: &Path) -> bool {
// A valid path beneath a symlink can only be reached through a directory
// symlink, so avoid a second `metadata` syscall to resolve its target.
path.ancestors()
.take_while(|ancestor| ancestor.starts_with(repo_root))
.any(|ancestor| {
std::fs::symlink_metadata(ancestor)
.is_ok_and(|metadata| metadata.file_type().is_symlink())
})
}

/// Returns the [`WatchFilter`] used by repository file watchers.
///
/// Emit predicate: forwards events for everything outside `.git/` plus the
Expand All @@ -1038,11 +1065,13 @@ pub fn should_watch_repo_directory(
/// over-watch, never to miss events.
#[cfg(feature = "local_fs")]
pub fn repo_watch_filter(
repo_root: PathBuf,
gitignores: Vec<Gitignore>,
force_included_paths: Vec<PathBuf>,
) -> WatchFilter {
let should_watch =
move |path: &Path| should_watch_repo_directory(path, &gitignores, &force_included_paths);
let should_watch = move |path: &Path| {
should_watch_repo_directory(path, &repo_root, &gitignores, &force_included_paths)
};
WatchFilter::with_filter(
Arc::new(should_watch),
Arc::new(|path: &Path| !should_ignore_git_path(path)),
Expand Down
112 changes: 106 additions & 6 deletions crates/repo_metadata/src/entry_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,26 +199,106 @@ fn should_watch_prunes_gitignored_directory() {
let gitignores = vec![gitignore_rooted(&root, "node_modules/\n")];

// Root and non-ignored dirs are watched; the gitignored dir is pruned.
assert!(super::should_watch_repo_directory(&root, &gitignores, &[]));
assert!(super::should_watch_repo_directory(
&root,
&root,
&gitignores,
&[]
));
assert!(super::should_watch_repo_directory(
&root.join("src"),
&root,
&gitignores,
&[]
));
assert!(!super::should_watch_repo_directory(
&root.join("node_modules"),
&root,
&gitignores,
&[]
));
// Descendants of an ignored dir are also pruned (ancestor-aware), which is
// what preserves the watcher's monotonicity invariant.
assert!(!super::should_watch_repo_directory(
&root.join("node_modules/foo"),
&root,
&gitignores,
&[]
));
}

#[cfg(unix)]
#[test]
fn should_watch_prunes_directory_symlinks_and_their_descendants() {
let temp_dir = tempfile::tempdir().unwrap();
let root = dunce::canonicalize(temp_dir.path()).unwrap();
fs::create_dir_all(root.join("target/tree")).unwrap();
std::os::unix::fs::symlink(root.join("target"), root.join("result")).unwrap();

// The symlink and paths reached through it must both be rejected. The
// latter protects the watch filter's ancestor monotonicity invariant.
assert!(!super::should_watch_repo_directory(
&root.join("result"),
&root,
&[],
&[]
));
assert!(!super::should_watch_repo_directory(
&root.join("result/tree"),
&root,
&[],
&[]
));
}

#[cfg(unix)]
#[test]
fn should_watch_ignores_symlinks_above_repo_root() {
let temp_dir = tempfile::tempdir().unwrap();
let real_parent = temp_dir.path().join("real-parent");
let symlinked_parent = temp_dir.path().join("linked-parent");
fs::create_dir_all(real_parent.join("repo/src")).unwrap();
std::os::unix::fs::symlink(&real_parent, &symlinked_parent).unwrap();

let repo_root = symlinked_parent.join("repo");
assert!(super::should_watch_repo_directory(
&repo_root.join("src"),
&repo_root,
&[],
&[]
));
}

#[cfg(unix)]
#[test]
fn should_watch_allows_symlinked_force_included_paths() {
let temp_dir = tempfile::tempdir().unwrap();
let root = dunce::canonicalize(temp_dir.path()).unwrap();
fs::create_dir_all(root.join("targets/skills/linked")).unwrap();
fs::create_dir_all(root.join(".agents/skills")).unwrap();
std::os::unix::fs::symlink(
root.join("targets/skills/linked"),
root.join(".agents/skills/linked"),
)
.unwrap();
let force_included = [std::path::PathBuf::from(".agents/skills")];

// Project-skill providers intentionally support symlinked skill
// directories, so the explicit force-included path remains watchable.
assert!(super::should_watch_repo_directory(
&root.join(".agents/skills/linked"),
&root,
&[],
&force_included
));
assert!(super::should_watch_repo_directory(
&root.join(".agents/skills/linked/SKILL.md"),
&root,
&[],
&force_included
));
}

#[test]
fn should_watch_descends_to_force_included_under_ignored_ancestor() {
let temp_dir = tempfile::tempdir().unwrap();
Expand All @@ -232,22 +312,26 @@ fn should_watch_descends_to_force_included_under_ignored_ancestor() {
// prefix to reach the force-included path, and into its subtree.
assert!(super::should_watch_repo_directory(
&root.join(".agents"),
&root,
&gitignores,
&force_included
));
assert!(super::should_watch_repo_directory(
&root.join(".agents/skills"),
&root,
&gitignores,
&force_included
));
assert!(super::should_watch_repo_directory(
&root.join(".agents/skills/test"),
&root,
&gitignores,
&force_included
));
// A sibling ignored dir that is not force-included is still pruned.
assert!(!super::should_watch_repo_directory(
&root.join(".agents/other"),
&root,
&gitignores,
&force_included
));
Expand All @@ -266,21 +350,25 @@ fn should_watch_handles_nested_ignored_ancestor_with_deeper_force_included() {
// prefix and into it, while pruning the ignored sibling.
assert!(super::should_watch_repo_directory(
&root.join("a"),
&root,
&gitignores,
&force_included
));
assert!(super::should_watch_repo_directory(
&root.join("a/b"),
&root,
&gitignores,
&force_included
));
assert!(super::should_watch_repo_directory(
&root.join("a/b/c"),
&root,
&gitignores,
&force_included
));
assert!(!super::should_watch_repo_directory(
&root.join("a/b/other"),
&root,
&gitignores,
&force_included
));
Expand All @@ -298,13 +386,15 @@ fn should_watch_descends_dir_only_reinclude_negation() {
// `parentdir` itself is not matched by `parentdir/*`, so we descend.
assert!(super::should_watch_repo_directory(
&root.join("parentdir"),
&root,
&gitignores,
&[]
));
// The subdirectory is re-included by the directory-only negation, so it is
// still watched even though `parentdir/*` matched it first.
assert!(super::should_watch_repo_directory(
&root.join("parentdir/sub"),
&root,
&gitignores,
&[]
));
Expand All @@ -320,17 +410,21 @@ fn should_watch_descends_dir_only_reinclude_negation() {

#[test]
fn should_watch_preserves_git_internal_allowlist() {
// No gitignores / force-included paths needed: `.git` handling
// short-circuits and is path-based, mirroring
// `should_watch_directory_in_git_path`.
let repo = std::path::Path::new("/home/user/project");
// No gitignores / force-included paths needed: `.git` handling is
// path-based, mirroring `should_watch_directory_in_git_path`.
let temp_dir = tempfile::tempdir().unwrap();
let repo = dunce::canonicalize(temp_dir.path()).unwrap();
fs::create_dir_all(repo.join(".git/refs/heads")).unwrap();
fs::create_dir_all(repo.join(".git/objects")).unwrap();
assert!(super::should_watch_repo_directory(
&repo.join(".git/refs/heads"),
&repo,
&[],
&[]
));
assert!(!super::should_watch_repo_directory(
&repo.join(".git/objects"),
&repo,
&[],
&[]
));
Expand Down Expand Up @@ -873,10 +967,16 @@ fn gitignore_affects_descend_predicate_but_not_emitted_events() {
// registration, while a tracked dir is still descended into.
assert!(!should_watch_repo_directory(
&node_modules,
&root_path,
&gitignores,
&[]
));
assert!(should_watch_repo_directory(
&src,
&root_path,
&gitignores,
&[]
));
assert!(should_watch_repo_directory(&src, &gitignores, &[]));

// Emit predicate building block (`!should_ignore_git_path`): gitignored,
// non-`.git` paths are NOT suppressed, so their events still flow. Only
Expand Down
6 changes: 3 additions & 3 deletions crates/repo_metadata/src/local_model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ impl LocalRepoMetadataModel {
watcher.update(ctx, |watcher, _ctx| {
std::mem::drop(watcher.register_path(
target_dir,
repo_watch_filter(Vec::new(), Vec::new()),
repo_watch_filter(target_dir.clone(), Vec::new(), Vec::new()),
RecursiveMode::NonRecursive,
));
});
Expand Down Expand Up @@ -971,7 +971,7 @@ impl LocalRepoMetadataModel {
}
std::mem::drop(watcher.register_path(
&watch_path,
repo_watch_filter(gitignores, force_included_paths),
repo_watch_filter(watch_path.clone(), gitignores, force_included_paths),
recursive_mode,
));
});
Expand Down Expand Up @@ -1415,7 +1415,7 @@ impl LocalRepoMetadataModel {
watcher.update(ctx, |watcher, _ctx| {
std::mem::drop(watcher.register_path(
&local_path,
repo_watch_filter(gitignores, force_included_paths),
repo_watch_filter(local_path.clone(), gitignores, force_included_paths),
RecursiveMode::NonRecursive,
));
});
Expand Down
2 changes: 1 addition & 1 deletion crates/repo_metadata/src/watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ impl DirectoryWatcher {

Some(watcher.register_path(
&local_path,
repo_watch_filter(gitignores, force_included_paths),
repo_watch_filter(local_path.clone(), gitignores, force_included_paths),
RecursiveMode::Recursive,
))
})
Expand Down