diff --git a/crates/repo_metadata/src/entry.rs b/crates/repo_metadata/src/entry.rs index e84b3e475df..35f90c79041 100644 --- a/crates/repo_metadata/src/entry.rs +++ b/crates/repo_metadata/src/entry.rs @@ -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(), @@ -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 @@ -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, force_included_paths: Vec, ) -> 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)), diff --git a/crates/repo_metadata/src/entry_tests.rs b/crates/repo_metadata/src/entry_tests.rs index b862348efff..20979153430 100644 --- a/crates/repo_metadata/src/entry_tests.rs +++ b/crates/repo_metadata/src/entry_tests.rs @@ -199,14 +199,21 @@ 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, &[] )); @@ -214,11 +221,84 @@ fn should_watch_prunes_gitignored_directory() { // 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(); @@ -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 )); @@ -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 )); @@ -298,6 +386,7 @@ 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, &[] )); @@ -305,6 +394,7 @@ fn should_watch_descends_dir_only_reinclude_negation() { // still watched even though `parentdir/*` matched it first. assert!(super::should_watch_repo_directory( &root.join("parentdir/sub"), + &root, &gitignores, &[] )); @@ -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, &[], &[] )); @@ -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 diff --git a/crates/repo_metadata/src/local_model.rs b/crates/repo_metadata/src/local_model.rs index d796346026e..1881cc4a1fb 100644 --- a/crates/repo_metadata/src/local_model.rs +++ b/crates/repo_metadata/src/local_model.rs @@ -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, )); }); @@ -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, )); }); @@ -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, )); }); diff --git a/crates/repo_metadata/src/watcher.rs b/crates/repo_metadata/src/watcher.rs index b945c343b43..bd336844dd6 100644 --- a/crates/repo_metadata/src/watcher.rs +++ b/crates/repo_metadata/src/watcher.rs @@ -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, )) })