From c6e0000a630d7a5f75d70acdc5b320466e61b7de Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 2 Nov 2026 14:54:04 -0800 Subject: [PATCH 1/4] Escape globs built from paths and fix them at the project root Each package and `.codeowner` becomes a `/**/**` glob and each annotated file its own path. `validate` matches files against them, and they're written to CODEOWNERS, where `for-file --from-codeowners`, `validate ` and code_ownership's `for_file` match them again. The package mappers escaped nothing and the `.codeowner` and annotation mappers escaped only `[` and `]`, so `packs/[ab]` claimed `packs/a`, `{legacy}` meant `legacy`, and in `validate` a top-level `!legacy` was a negated glob that gave its team every file outside `legacy`. A package or `.codeowner` at the project root got `/**/**`, which matches no project-relative path, and the CODEOWNERS line `//**/**`. One escaper now puts a backslash before `\`, `*`, `?`, `[`, `]`, `{`, `}` and `!` in every glob built from a path, team files and vendored gems included. It leaves letters alone, since fast_glob reads `\b`, `\n`, `\r` and `\t` as control characters. The project root's glob is `**/**`, which CODEOWNERS gets as `/**/**`, and `for-file` describes a package by the glob `validate` matches. With the root matching, a file in `app/` matches both a root `.codeowner` and `app/.codeowner`, and `validate` kept whichever came first from the parallel walk, since `Source::len` counted separators and gave both 0. It now counts components, which ranks the root below every directory. The root's `.codeowner` is shown as `.codeowner` rather than `/.codeowner`, and a `.codeowner` naming an invalid team below it is reported as inheriting its owner from the project root rather than from an empty name. When `validate` drops nested packages, a root package encloses every other one, as for any nested package; a test now pins that. Repos with such names, or with a `.codeowner` or package at the root, get a different CODEOWNERS, and `validate` reports it out of date until they regenerate it. A root `.codeowner` or package now also owns files in `validate` and CODEOWNERS, as `for-file` already reported, so a file that another mechanism gives a different team is reported as having more than one owner. Fixes #146. --- src/common_test.rs | 95 ++++++ src/ownership/file_owner_finder.rs | 14 + src/ownership/file_owner_resolver.rs | 37 ++- src/ownership/mapper.rs | 7 +- src/ownership/mapper/annotated_file_mapper.rs | 11 +- src/ownership/mapper/directory_mapper.rs | 124 +++++++- src/ownership/mapper/escaper.rs | 95 +++++- src/ownership/mapper/package_mapper.rs | 129 +++++++- src/ownership/mapper/team_gem_mapper.rs | 43 ++- src/ownership/mapper/team_yml_mapper.rs | 24 +- src/ownership/validator.rs | 8 +- tests/path_globs_test.rs | 290 ++++++++++++++++++ 12 files changed, 849 insertions(+), 28 deletions(-) create mode 100644 tests/path_globs_test.rs diff --git a/src/common_test.rs b/src/common_test.rs index fdb0ffa..8c5ac5b 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -185,6 +185,40 @@ pub mod tests { ) } + pub fn build_ownership_with_directory_codeowners_with_glob_metacharacters() -> Result> { + ownership!( + TestProjectFile { + relative_path: "app/{old}/.codeowner".to_owned(), + content: "Bar\n".to_owned(), + }, + TestProjectFile { + relative_path: "!legacy/.codeowner".to_owned(), + content: "Foo\n".to_owned(), + } + ) + } + + #[cfg(unix)] + pub fn build_ownership_with_directory_codeowner_with_an_asterisk() -> Result> { + ownership!(TestProjectFile { + relative_path: "app/*/.codeowner".to_owned(), + content: "Baz\n".to_owned(), + }) + } + + pub fn build_ownership_with_directory_codeowners_at_the_project_root() -> Result> { + ownership!( + TestProjectFile { + relative_path: ".codeowner".to_owned(), + content: "Bam\n".to_owned(), + }, + TestProjectFile { + relative_path: "app/.codeowner".to_owned(), + content: "Bar\n".to_owned(), + } + ) + } + pub fn build_ownership_with_all_mappers() -> Result> { ownership!( TestProjectFile { @@ -249,6 +283,10 @@ pub mod tests { relative_path: "packs/[admin]/comp.ts".to_owned(), content: "// @team Bar\n".to_owned(), }, + TestProjectFile { + relative_path: "packs/{admin}/comp.ts".to_owned(), + content: "// @team Foo\n".to_owned(), + }, TestProjectFile { relative_path: "packs/bar/comp.rb".to_owned(), content: "# @team Bar\n".to_owned(), @@ -307,6 +345,28 @@ pub mod tests { let test_config = TestConfig::new(temp_dir.path().to_path_buf(), vec![]); build_ownership(test_config) } + + pub fn build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters() -> Result> { + let temp_dir = tempdir()?; + + let test_config = TestConfig { + team_names: vec![], + ..TestConfig::new( + temp_dir.path().to_path_buf(), + vec![ + TestProjectFile { + relative_path: "config/teams/[ops].yml".to_owned(), + content: "name: Ops\ngithub:\n team: \"@Ops\"\nruby:\n owned_gems:\n - \"{taco}\"\n".to_owned(), + }, + TestProjectFile { + relative_path: "gems/{taco}/sauce.rb".to_owned(), + content: "class Taco::Sauce\nend\n".to_owned(), + }, + ], + ) + }; + build_ownership(test_config) + } pub fn build_ownership_with_package_codeowners() -> Result> { ownership!( TestProjectFile { @@ -328,6 +388,41 @@ pub mod tests { ) } + pub fn build_ownership_with_package_codeowners_with_glob_metacharacters() -> Result> { + ownership!( + TestProjectFile { + relative_path: "packs/[ab]/package.yml".to_owned(), + content: "owner: Foo\n".to_owned(), + }, + TestProjectFile { + relative_path: "packs/{legacy}/package.yml".to_owned(), + content: "owner: Bar\n".to_owned(), + } + ) + } + + pub fn build_ownership_with_package_codeowners_at_the_project_root() -> Result> { + let temp_dir = tempdir()?; + + let test_config = TestConfig { + code_ownership_config_yml: format!("{DEFAULT_CODE_OWNERSHIP_YML}ruby_package_paths:\n - \"**/*\"\n"), + ..TestConfig::new( + temp_dir.path().to_path_buf(), + vec![ + TestProjectFile { + relative_path: "package.yml".to_owned(), + content: "owner: Foo\n".to_owned(), + }, + TestProjectFile { + relative_path: "packs/inner/package.yml".to_owned(), + content: "owner: Bar\n".to_owned(), + }, + ], + ) + }; + build_ownership(test_config) + } + pub fn vecs_match(a: &Vec, b: &Vec) { // First check lengths match assert_eq!(a.len(), b.len(), "Vectors have different lengths"); diff --git a/src/ownership/file_owner_finder.rs b/src/ownership/file_owner_finder.rs index e89de74..34dff4c 100644 --- a/src/ownership/file_owner_finder.rs +++ b/src/ownership/file_owner_finder.rs @@ -97,4 +97,18 @@ mod tests { Some((&team_name_longest, &source_longest)) ); } + + #[test] + fn test_directory_overrider_ranks_the_project_root_below_a_top_level_directory() { + // The `.codeowner` files come from a parallel walk, so either can be processed first. + let (root_team, root) = ("root_team".to_string(), Source::Directory("".to_string())); + let (app_team, app) = ("app_team".to_string(), Source::Directory("app".to_string())); + for order in [[(&root_team, &root), (&app_team, &app)], [(&app_team, &app), (&root_team, &root)]] { + let mut directory_overrider = DirectoryOverrider::default(); + for (team_name, source) in order { + directory_overrider.process(team_name, source); + } + assert_eq!(directory_overrider.specific_directory_owner(), Some((&app_team, &app))); + } + } } diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 23a5fcc..fb6ea51 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -10,7 +10,10 @@ use glob::glob; use crate::{config::Config, project::Team, project_file_builder::build_project_file_without_cache, yaml::NodeLimit}; -use super::{FileOwner, mapper::Source}; +use super::{ + FileOwner, + mapper::{Source, directory_glob}, +}; pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) -> Result, String> { let absolute_file_path = if file_path.is_absolute() { @@ -278,7 +281,7 @@ fn nearest_package_owner( Ok(owner) => { if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { let package_path = parent_rel.join("package.yml"); - let package_glob = format!("{rel_str}/**/**"); + let package_glob = directory_glob(rel_str); return Some(( team.name.clone(), Source::Package(package_path.to_string_lossy().to_string(), package_glob), @@ -300,7 +303,7 @@ fn nearest_package_owner( && let Some(team) = teams_by_name.get(&owner) { let package_path = parent_rel.join("package.json"); - let package_glob = format!("{rel_str}/**/**"); + let package_glob = directory_glob(rel_str); return Some(( team.name.clone(), Source::Package(package_path.to_string_lossy().to_string(), package_glob), @@ -584,6 +587,34 @@ mod tests { } } + #[test] + fn test_nearest_package_owner_describes_a_package_by_the_glob_validate_matches() { + let td = tempdir().unwrap(); + let project_root = td.path(); + let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + + let ruby_pkg = project_root.join("packs/{payroll}"); + std::fs::create_dir_all(&ruby_pkg).unwrap(); + std::fs::write(ruby_pkg.join("package.yml"), "---\nowner: Payroll\n").unwrap(); + let js_pkg = project_root.join("frontend/[flow]"); + std::fs::create_dir_all(&js_pkg).unwrap(); + std::fs::write(js_pkg.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap(); + + let mut tbn: HashMap = HashMap::new(); + for name in ["Payroll", "UX"] { + let t = team_named(name); + tbn.insert(t.name.clone(), t); + } + + for (file, package_path, glob) in [ + ("packs/{payroll}/a.rb", "packs/{payroll}/package.yml", "packs/\\{payroll\\}/**/**"), + ("frontend/[flow]/a.ts", "frontend/[flow]/package.json", "frontend/\\[flow\\]/**/**"), + ] { + let (_, source) = nearest_package_owner(project_root, Path::new(file), &config, &tbn).unwrap(); + assert_eq!(source, Source::Package(package_path.to_string(), glob.to_string())); + } + } + #[test] fn test_vendored_gem_owner() { let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index 867e620..a77a71d 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -15,6 +15,7 @@ mod team_yml_mapper; pub use annotated_file_mapper::TeamFileMapper; pub use directory_mapper::DirectoryMapper; +pub use escaper::directory_glob; pub use package_mapper::JavascriptPackageMapper; pub use package_mapper::RubyPackageMapper; pub use team_gem_mapper::TeamGemMapper; @@ -43,7 +44,7 @@ pub enum Source { impl Display for Source { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - Source::Directory(path) => write!(f, "Owner specified in `{}/.codeowner`", path), + Source::Directory(path) => write!(f, "Owner specified in `{}`", Path::new(path).join(".codeowner").display()), Source::AnnotatedFile => write!(f, "Owner annotation at the top of the file"), Source::TeamGem => write!(f, "Owner specified in Team YML's `owned_gems`"), Source::TeamGlob(glob) => write!(f, "Owner specified in Team YML as an owned_glob `{}`", glob), @@ -58,7 +59,8 @@ impl Display for Source { impl Source { pub fn len(&self) -> usize { match self { - Source::Directory(path) => path.matches('/').count(), + // Counting components rather than separators ranks the project root, "", below a top-level directory. + Source::Directory(path) => Path::new(path).components().count(), _ => 0, } } @@ -200,6 +202,7 @@ mod tests { Source::Directory("packs/bam".to_string()).to_string(), "Owner specified in `packs/bam/.codeowner`" ); + assert_eq!(Source::Directory("".to_string()).to_string(), "Owner specified in `.codeowner`"); assert_eq!(Source::AnnotatedFile.to_string(), "Owner annotation at the top of the file"); assert_eq!(Source::TeamGem.to_string(), "Owner specified in Team YML's `owned_gems`"); assert_eq!( diff --git a/src/ownership/mapper/annotated_file_mapper.rs b/src/ownership/mapper/annotated_file_mapper.rs index ec69230..cbc61c2 100644 --- a/src/ownership/mapper/annotated_file_mapper.rs +++ b/src/ownership/mapper/annotated_file_mapper.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use std::sync::Arc; use super::Entry; -use super::escaper::escape_brackets; +use super::escaper::escape_glob; use super::{Mapper, OwnerMatcher}; use crate::ownership::mapper::Source; use crate::project::Project; @@ -31,7 +31,7 @@ impl Mapper for TeamFileMapper { let relative_path = self.project.relative_path(&owned_file.path); entries.push(Entry { - path: escape_brackets(&relative_path.to_string_lossy()), + path: escape_glob(&relative_path.to_string_lossy()), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -87,6 +87,12 @@ mod tests { team_name: "Bar".to_owned(), disabled: false, }, + Entry { + path: "packs/\\{admin\\}/comp.ts".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, Entry { path: "packs/jscomponents/comp.ts".to_owned(), github_team: "@Foo".to_owned(), @@ -142,6 +148,7 @@ mod tests { let expected_owner_matchers = vec![OwnerMatcher::ExactMatches( HashMap::from([ (PathBuf::from("packs/[admin]/comp.ts"), "Bar".to_owned()), + (PathBuf::from("packs/{admin}/comp.ts"), "Foo".to_owned()), (PathBuf::from("packs/bar/comp.rb"), "Bar".to_owned()), (PathBuf::from("packs/jscomponents/comp.ts"), "Foo".to_owned()), (PathBuf::from("packs/jscomponents/comp-colon.ts"), "Foo".to_owned()), diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 295e842..446dabc 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use super::escaper::escape_brackets; +use super::escaper::directory_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::Project; @@ -28,7 +28,7 @@ impl Mapper for DirectoryMapper { let team = team_by_name.get(&directory_codeowner_file.owner); if let Some(team) = team { entries.push(Entry { - path: format!("{}/**/**", escape_brackets(&dir_root)), + path: directory_glob(&dir_root), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -44,10 +44,7 @@ impl Mapper for DirectoryMapper { for file in &self.project.directory_codeowner_files { owner_matchers.push(OwnerMatcher::new_glob( - format!( - "{}/**/**", - escape_brackets(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()) - ), + directory_glob(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()), file.owner.to_owned(), Source::Directory(file.directory_root().map(|p| p.to_string_lossy().to_string()).unwrap_or_default()), )); @@ -65,8 +62,12 @@ impl Mapper for DirectoryMapper { mod tests { use std::error::Error; + #[cfg(unix)] + use crate::common_test::tests::build_ownership_with_directory_codeowner_with_an_asterisk; use crate::common_test::tests::{ - build_ownership_with_directory_codeowners, build_ownership_with_directory_codeowners_with_brackets, vecs_match, + build_ownership_with_directory_codeowners, build_ownership_with_directory_codeowners_at_the_project_root, + build_ownership_with_directory_codeowners_with_brackets, build_ownership_with_directory_codeowners_with_glob_metacharacters, + vecs_match, }; use super::*; @@ -172,4 +173,113 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_with_glob_metacharacters()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "app/\\{old\\}/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + Entry { + path: "\\!legacy/**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_with_glob_metacharacters()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob( + "app/\\{old\\}/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/{old}".to_owned()), + ), + OwnerMatcher::new_glob( + "\\!legacy/**/**".to_owned(), + "Foo".to_owned(), + Source::Directory("!legacy".to_owned()), + ), + ], + ); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn test_entries_and_owner_matchers_with_an_asterisk() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowner_with_an_asterisk()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![Entry { + path: "app/\\*/**/**".to_owned(), + github_team: "@Baz".to_owned(), + team_name: "Baz".to_owned(), + disabled: false, + }], + ); + vecs_match( + &mapper.owner_matchers(), + &vec![OwnerMatcher::new_glob( + "app/\\*/**/**".to_owned(), + "Baz".to_owned(), + Source::Directory("app/*".to_owned()), + )], + ); + Ok(()) + } + + #[test] + fn test_entries_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_at_the_project_root()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "**/**".to_owned(), + github_team: "@Bam".to_owned(), + team_name: "Bam".to_owned(), + disabled: false, + }, + Entry { + path: "app/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_at_the_project_root()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob("**/**".to_owned(), "Bam".to_owned(), Source::Directory("".to_owned())), + OwnerMatcher::new_glob("app/**/**".to_owned(), "Bar".to_owned(), Source::Directory("app".to_owned())), + ], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/escaper.rs b/src/ownership/mapper/escaper.rs index bc1e36d..ca09a25 100644 --- a/src/ownership/mapper/escaper.rs +++ b/src/ownership/mapper/escaper.rs @@ -1,3 +1,94 @@ -pub fn escape_brackets(path: &str) -> String { - path.replace("[", "\\[").replace("]", "\\]") +/// `path` with a `\` before each character that fast_glob reads as a pattern, so a glob built from it matches it literally. `!` is +/// one, since it negates a glob it starts. Nothing else is escaped, since fast_glob reads `\b`, `\n`, `\r` and `\t` as control +/// characters. +pub fn escape_glob(path: &str) -> String { + let mut escaped = String::with_capacity(path.len()); + for c in path.chars() { + if matches!(c, '\\' | '*' | '?' | '[' | ']' | '{' | '}' | '!') { + escaped.push('\\'); + } + escaped.push(c); + } + escaped +} + +/// The glob for every file under `directory`, a project-relative path. That's `**/**` for the project root, the empty path, which +/// CODEOWNERS gets as `/**/**`. +pub fn directory_glob(directory: &str) -> String { + if directory.is_empty() { + "**/**".to_owned() + } else { + format!("{}/**/**", escape_glob(directory)) + } +} + +#[cfg(test)] +mod tests { + use fast_glob::glob_match; + + use super::*; + + #[test] + fn test_escape_glob_escapes_each_metacharacter() { + assert_eq!(escape_glob("a*b"), "a\\*b"); + assert_eq!(escape_glob("a?b"), "a\\?b"); + assert_eq!(escape_glob("[ab]"), "\\[ab\\]"); + assert_eq!(escape_glob("{a,b}"), "\\{a,b\\}"); + assert_eq!(escape_glob("!a"), "\\!a"); + assert_eq!(escape_glob("a!"), "a\\!"); + assert_eq!(escape_glob("a\\b"), "a\\\\b"); + } + + #[test] + fn test_escape_glob_escapes_a_backslash_once() { + assert_eq!(escape_glob("a\\[b"), "a\\\\\\[b"); + assert_eq!(escape_glob("a\\\\b"), "a\\\\\\\\b"); + } + + #[test] + fn test_escape_glob_leaves_other_characters_alone() { + let path = "app/b n r t/(x)/a,b-c^d/e.rb"; + assert_eq!(escape_glob(path), path); + } + + #[test] + fn test_directory_glob() { + assert_eq!(directory_glob("packs/[ab]"), "packs/\\[ab\\]/**/**"); + assert_eq!(directory_glob(""), "**/**"); + assert!(glob_match(directory_glob(""), "x.rb")); + assert!(glob_match(directory_glob(""), "app/x.rb")); + } + + #[test] + fn test_directory_glob_matches_only_its_own_directory() { + // Each name, with directories its unescaped glob would also match. + let names: &[(&str, &[&str])] = &[ + ("[ab]", &["a", "b"]), + ("{a,b}", &["a", "b"]), + ("{legacy}", &["legacy"]), + ("a*", &["a", "ab"]), + ("a?", &["ab"]), + ("!x", &["legacy"]), + ("a\\b", &[]), + ("a]", &[]), + ("a}", &[]), + ("(x)", &[]), + ("a,b", &[]), + ]; + for (name, siblings) in names { + let glob = directory_glob(name); + assert!(glob_match(&glob, format!("{name}/f.rb")), "{glob} should match {name}/f.rb"); + assert!(glob_match(&glob, format!("{name}/x/f.rb")), "{glob} should match {name}/x/f.rb"); + for sibling in *siblings { + assert!( + glob_match(format!("{name}/**/**"), format!("{sibling}/f.rb")), + "the unescaped glob for {name} should match {sibling}/f.rb" + ); + assert!( + !glob_match(&glob, format!("{sibling}/f.rb")), + "{glob} shouldn't match {sibling}/f.rb" + ); + } + } + } } diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index 1ab5d34..bf98a04 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use super::escaper::directory_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::{Package, PackageType, Project}; @@ -75,7 +76,7 @@ impl PackageMapper { if let Some(team) = team { entries.push(Entry { - path: format!("{}/**/**", package_root), + path: directory_glob(&package_root), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -105,9 +106,9 @@ impl PackageMapper { if let Some(team) = team { owner_matchers.push(OwnerMatcher::new_glob( - format!("{}/**/**", package_root), + directory_glob(&package_root), team.name.to_owned(), - Source::Package(package.path.to_string_lossy().to_string(), format!("{}/**/**", package_root)), + Source::Package(package.path.to_string_lossy().to_string(), directory_glob(&package_root)), )); } } @@ -139,7 +140,11 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> { mod tests { use super::*; use crate::{ - common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_package_codeowners, vecs_match}, + common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_package_codeowners, + build_ownership_with_package_codeowners_at_the_project_root, build_ownership_with_package_codeowners_with_glob_metacharacters, + vecs_match, + }, ownership::mapper::RubyPackageMapper, project::{Package, PackageType}, }; @@ -180,6 +185,36 @@ mod tests { assert_eq!(package_paths, vec!["packs/a/package.yml", "packs/c/package.yml"]); } + #[test] + fn test_remove_nested_packages_with_a_package_at_the_project_root() { + let packages = [ + Package { + path: Path::new("packs/a/package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_a".to_owned(), + }, + Package { + path: Path::new("package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_root".to_owned(), + }, + Package { + path: Path::new("packs/b/package.yml").to_owned(), + package_type: PackageType::Ruby, + owner: "owner_b".to_owned(), + }, + ]; + + let packages = packages.iter().collect_vec(); + + let package_paths = super::remove_nested_packages(&packages) + .iter() + .map(|package| package.path.to_str().unwrap()) + .collect_vec(); + + assert_eq!(package_paths, vec!["package.yml"]); + } + #[test] fn test_entries() -> Result<(), Box> { let ownership = build_ownership_with_all_mappers()?; @@ -217,4 +252,90 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_with_glob_metacharacters()?; + let mapper = RubyPackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "packs/\\[ab\\]/**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + Entry { + path: "packs/\\{legacy\\}/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_with_glob_metacharacters()?; + let mapper = PackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(&PackageType::Ruby), + &vec![ + OwnerMatcher::new_glob( + "packs/\\[ab\\]/**/**".to_owned(), + "Foo".to_owned(), + Source::Package("packs/[ab]/package.yml".to_owned(), "packs/\\[ab\\]/**/**".to_owned()), + ), + OwnerMatcher::new_glob( + "packs/\\{legacy\\}/**/**".to_owned(), + "Bar".to_owned(), + Source::Package("packs/{legacy}/package.yml".to_owned(), "packs/\\{legacy\\}/**/**".to_owned()), + ), + ], + ); + Ok(()) + } + + #[test] + fn test_entries_at_the_project_root() -> Result<(), Box> { + let ownership = build_ownership_with_package_codeowners_at_the_project_root()?; + let mapper = RubyPackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![ + Entry { + path: "**/**".to_owned(), + github_team: "@Foo".to_owned(), + team_name: "Foo".to_owned(), + disabled: false, + }, + Entry { + path: "packs/inner/**/**".to_owned(), + github_team: "@Bar".to_owned(), + team_name: "Bar".to_owned(), + disabled: false, + }, + ], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_at_the_project_root() -> Result<(), Box> { + // The root package encloses the other, so only it is matched, as for any nested package. + let ownership = build_ownership_with_package_codeowners_at_the_project_root()?; + let mapper = PackageMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(&PackageType::Ruby), + &vec![OwnerMatcher::new_glob( + "**/**".to_owned(), + "Foo".to_owned(), + Source::Package("package.yml".to_owned(), "**/**".to_owned()), + )], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/team_gem_mapper.rs b/src/ownership/mapper/team_gem_mapper.rs index 9d74914..ab1554a 100644 --- a/src/ownership/mapper/team_gem_mapper.rs +++ b/src/ownership/mapper/team_gem_mapper.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use super::escaper::escape_glob; use super::{Entry, Source}; use super::{Mapper, OwnerMatcher}; use crate::project::Project; @@ -24,8 +25,9 @@ impl Mapper for TeamGemMapper { let vendored_gem = vendored_gem_by_name.get(owned_gem); if let Some(vendored_gem) = vendored_gem { + let gem_path = escape_glob(&self.project.relative_path(&vendored_gem.path).to_string_lossy()); entries.push(Entry { - path: format!("{}/**/**", self.project.relative_path(&vendored_gem.path).to_string_lossy()), + path: format!("{gem_path}/**/**"), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -46,8 +48,9 @@ impl Mapper for TeamGemMapper { let vendored_gem = vendored_gem_by_name.get(owned_gem); if let Some(vendored_gem) = vendored_gem { + let gem_path = escape_glob(&self.project.relative_path(&vendored_gem.path).to_string_lossy()); owner_matchers.push(OwnerMatcher::new_glob( - format!("{}/**/*", self.project.relative_path(&vendored_gem.path).to_string_lossy()), + format!("{gem_path}/**/*"), team.name.clone(), Source::TeamGem, )); @@ -67,7 +70,10 @@ impl Mapper for TeamGemMapper { mod tests { use std::error::Error; - use crate::common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_team_gem_codeowners, vecs_match}; + use crate::common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_team_gem_codeowners, + build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters, vecs_match, + }; use super::*; #[test] @@ -100,4 +106,35 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamGemMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![Entry { + path: "gems/\\{taco\\}/**/**".to_owned(), + github_team: "@Ops".to_owned(), + team_name: "Ops".to_owned(), + disabled: false, + }], + ); + Ok(()) + } + + #[test] + fn test_owner_matchers_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamGemMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![OwnerMatcher::new_glob( + "gems/\\{taco\\}/**/*".to_owned(), + "Ops".to_owned(), + Source::TeamGem, + )], + ); + Ok(()) + } } diff --git a/src/ownership/mapper/team_yml_mapper.rs b/src/ownership/mapper/team_yml_mapper.rs index c05c417..ac78a88 100644 --- a/src/ownership/mapper/team_yml_mapper.rs +++ b/src/ownership/mapper/team_yml_mapper.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use std::sync::Arc; use super::Entry; +use super::escaper::escape_glob; use super::{Mapper, OwnerMatcher}; use crate::ownership::mapper::Source; use crate::project::Project; @@ -23,7 +24,7 @@ impl Mapper for TeamYmlMapper { for team in &self.project.teams { entries.push(Entry { - path: self.project.relative_path(&team.path).to_string_lossy().to_string(), + path: escape_glob(&self.project.relative_path(&team.path).to_string_lossy()), github_team: team.github_team.to_owned(), team_name: team.name.to_owned(), disabled: team.avoid_ownership, @@ -52,7 +53,10 @@ impl Mapper for TeamYmlMapper { mod tests { use std::error::Error; - use crate::common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_team_yml_codeowners, vecs_match}; + use crate::common_test::tests::{ + build_ownership_with_all_mappers, build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters, + build_ownership_with_team_yml_codeowners, vecs_match, + }; use super::*; #[test] @@ -109,4 +113,20 @@ mod tests { ); Ok(()) } + + #[test] + fn test_entries_with_glob_metacharacters() -> Result<(), Box> { + let ownership = build_ownership_with_team_yml_and_gem_codeowners_with_glob_metacharacters()?; + let mapper = TeamYmlMapper::build(ownership.project.clone()); + vecs_match( + &mapper.entries(), + &vec![Entry { + path: "config/teams/\\[ops\\].yml".to_owned(), + github_team: "@Ops".to_owned(), + team_name: "Ops".to_owned(), + disabled: false, + }], + ); + Ok(()) + } } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index e291755..3952046 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -292,10 +292,12 @@ impl Error { format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name) }; if let Some(inherits_from) = inherits_from { - message.push_str(&format!( - "; this directory is currently inheriting its owner from {}", + let inherits_from = if inherits_from.as_os_str().is_empty() { + "the project root".into() + } else { inherits_from.to_string_lossy() - )); + }; + message.push_str(&format!("; this directory is currently inheriting its owner from {inherits_from}")); } vec![message] } diff --git a/tests/path_globs_test.rs b/tests/path_globs_test.rs new file mode 100644 index 0000000..f551ee5 --- /dev/null +++ b/tests/path_globs_test.rs @@ -0,0 +1,290 @@ +//! The globs built from a package's or `.codeowner`'s directory, or an annotated file's path, are matched by `validate` and written +//! to CODEOWNERS, where `for-file --from-codeowners` and `validate ` match them again. + +use assert_cmd::{assert::Assert, prelude::*}; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; +use tempfile::TempDir; + +mod common; +use common::{git_add_all_files, init_git_repo}; + +// The default ruby_package_paths don't match the project root. +const RUBY_PACKAGES_ANYWHERE: &str = "owned_globs:\n - \"**/*.rb\"\nruby_package_paths:\n - \"**/*\"\n"; + +// A git repo of `files`, teams Foo and Bar and a config owning `**/*.rb`, any of which `files` can replace, with CODEOWNERS +// generated. +fn generated_project(files: &[(&str, &str)]) -> Result> { + let temp_dir = tempfile::tempdir()?; + let project_root = temp_dir.path(); + let defaults = [ + ("config/code_ownership.yml", "owned_globs:\n - \"**/*.rb\"\n"), + ("config/teams/foo.yml", "name: Foo\ngithub:\n team: \"@org/foo\"\n"), + ("config/teams/bar.yml", "name: Bar\ngithub:\n team: \"@org/bar\"\n"), + ]; + for (path, contents) in defaults.iter().chain(files) { + let path = project_root.join(path); + fs::create_dir_all(path.parent().ok_or("a file needs a parent directory")?)?; + fs::write(path, contents)?; + } + init_git_repo(project_root); + git_add_all_files(project_root); + codeowners(project_root, &["generate"])?.success(); + Ok(temp_dir) +} + +fn codeowners(project_root: &Path, args: &[&str]) -> Result> { + Ok(Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert()) +} + +// CODEOWNERS without its comments and blank lines. +fn codeowners_rules(project_root: &Path) -> Result, Box> { + Ok(fs::read_to_string(project_root.join(".github/CODEOWNERS"))? + .lines() + .filter(|line| !line.is_empty() && !line.starts_with('#')) + .map(str::to_owned) + .collect()) +} + +fn assert_owner(project_root: &Path, file: &str, team: &str) -> Result<(), Box> { + for args in [["for-file", file].as_slice(), &["for-file", "--from-codeowners", file]] { + codeowners(project_root, args)? + .success() + .stdout(predicate::str::starts_with(format!("Team: {team}\n"))); + } + Ok(()) +} + +// The project from #146: each name with glob metacharacters has a sibling that its glob would match instead if unescaped. +fn project_with_glob_metacharacters_in_names() -> Result> { + generated_project(&[ + ("packs/[ab]/package.yml", "owner: Foo\n"), + ("packs/{legacy}/package.yml", "owner: Foo\n"), + ("packs/legacy/package.yml", "owner: Bar\n"), + ("app/{old}/.codeowner", "Foo\n"), + ("app/old/.codeowner", "Bar\n"), + ("lib/{v2}.rb", "# @team Foo\n"), + ("packs/[ab]/x.rb", "puts 1\n"), + ("packs/a/y.rb", "puts 1\n"), + ("packs/{legacy}/x.rb", "puts 1\n"), + ("packs/legacy/z.rb", "puts 1\n"), + ("app/{old}/x.rb", "puts 1\n"), + ("app/old/z.rb", "puts 1\n"), + ("lib/v2.rb", "puts 1\n"), + ]) +} + +const OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES: [(&str, &str); 8] = [ + ("packs/[ab]/x.rb", "Foo"), + ("packs/a/y.rb", "Unowned"), + ("packs/{legacy}/x.rb", "Foo"), + ("packs/legacy/z.rb", "Bar"), + ("app/{old}/x.rb", "Foo"), + ("app/old/z.rb", "Bar"), + ("lib/{v2}.rb", "Foo"), + ("lib/v2.rb", "Unowned"), +]; + +#[test] +fn test_generate_escapes_glob_metacharacters_in_names() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/app/\\{old\\}/**/** @org/foo", + "/app/old/**/** @org/bar", + "/packs/\\[ab\\]/**/** @org/foo", + "/packs/\\{legacy\\}/**/** @org/foo", + "/packs/legacy/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + "/lib/\\{v2\\}.rb @org/foo", + ] + ); + Ok(()) +} + +#[test] +fn test_for_file_agrees_with_codeowners_for_names_with_glob_metacharacters() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + for (file, team) in OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["crosscheck-owners"])? + .success() + .stdout(predicate::str::contains( + "Success! All files match between CODEOWNERS and for-file command.", + )); + Ok(()) +} + +#[test] +fn test_for_file_describes_a_package_by_the_glob_validate_matches() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + codeowners(project.path(), &["for-file", "packs/{legacy}/x.rb"])? + .success() + .stdout(predicate::eq(indoc! {" + Team: Foo + Github Team: @org/foo + Team YML: config/teams/foo.yml + Description: + - Owner defined in `packs/{legacy}/package.yml` with implicitly owned glob: `packs/\\{legacy\\}/**/**` + "})); + Ok(()) +} + +#[test] +fn test_validate_reads_glob_metacharacters_in_names_literally() -> Result<(), Box> { + let project = project_with_glob_metacharacters_in_names()?; + codeowners(project.path(), &["validate"])?.failure().stdout(predicate::eq(indoc! {" + + Some files are missing ownership + - lib/v2.rb + - packs/a/y.rb + + "})); + let files = OWNERS_OF_FILES_WITH_GLOB_METACHARACTERS_IN_NAMES.map(|(file, _)| file); + codeowners(project.path(), &[["validate"].as_slice(), &files].concat())? + .failure() + .stdout(predicate::eq("Unowned files detected:\n packs/a/y.rb\n lib/v2.rb\n")); + Ok(()) +} + +#[test] +fn test_validate_reads_a_leading_exclamation_mark_in_a_directory_name_literally() -> Result<(), Box> { + // Unescaped, `!legacy/**/**` would match every file not under `legacy`, so `validate` would give app/x.rb an owner. + let project = generated_project(&[ + ("!legacy/.codeowner", "Foo\n"), + ("!legacy/x.rb", "puts 1\n"), + ("app/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/\\!legacy/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + codeowners(project.path(), &["validate"])? + .failure() + .stdout(predicate::eq("\nSome files are missing ownership\n- app/x.rb\n\n")); + Ok(()) +} + +#[test] +fn test_a_codeowner_file_at_the_project_root_owns_every_file() -> Result<(), Box> { + let project = generated_project(&[(".codeowner", "Foo\n"), ("app/x.rb", "puts 1\n")])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + assert_owner(project.path(), "app/x.rb", "Foo")?; + codeowners(project.path(), &["for-file", "app/x.rb"])? + .success() + .stdout(predicate::str::ends_with("Description:\n- Owner specified in `.codeowner`\n")); + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + codeowners(project.path(), &["validate", "app/x.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_package_at_the_project_root_owns_every_file() -> Result<(), Box> { + let project = generated_project(&[ + ("config/code_ownership.yml", RUBY_PACKAGES_ANYWHERE), + ("package.yml", "owner: Foo\n"), + ("app/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + assert_owner(project.path(), "app/x.rb", "Foo")?; + codeowners(project.path(), &["for-file", "app/x.rb"])? + .success() + .stdout(predicate::str::ends_with( + "Description:\n- Owner defined in `package.yml` with implicitly owned glob: `**/**`\n", + )); + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + codeowners(project.path(), &["validate", "app/x.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_codeowner_file_in_a_directory_overrides_one_at_the_project_root() -> Result<(), Box> { + // If `validate` took the root's owner for annotated.rb, the file would have two. + let project = generated_project(&[ + (".codeowner", "Foo\n"), + ("app/.codeowner", "Bar\n"), + ("app/x.rb", "puts 1\n"), + ("app/annotated.rb", "# @team Bar\n"), + ("lib/x.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/app/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + "/app/annotated.rb @org/bar", + ] + ); + for (file, team) in [("app/x.rb", "Bar"), ("app/annotated.rb", "Bar"), ("lib/x.rb", "Foo")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_a_package_nested_in_one_at_the_project_root_owns_its_files() -> Result<(), Box> { + let project = generated_project(&[ + ("config/code_ownership.yml", RUBY_PACKAGES_ANYWHERE), + ("package.yml", "owner: Foo\n"), + ("packs/inner/package.yml", "owner: Bar\n"), + ("app/x.rb", "puts 1\n"), + ("packs/inner/y.rb", "puts 1\n"), + ])?; + assert_eq!( + codeowners_rules(project.path())?, + [ + "/**/** @org/foo", + "/packs/inner/**/** @org/bar", + "/config/teams/bar.yml @org/bar", + "/config/teams/foo.yml @org/foo", + ] + ); + for (file, team) in [("app/x.rb", "Foo"), ("packs/inner/y.rb", "Bar")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + +#[test] +fn test_validate_names_the_project_root_as_what_a_codeowner_file_with_an_invalid_team_inherits_from() -> Result<(), Box> { + let project = generated_project(&[(".codeowner", "Foo\n"), ("app/.codeowner", "Typo\n"), ("app/x.rb", "puts 1\n")])?; + codeowners(project.path(), &["validate"])?.failure().stdout(predicate::str::contains( + "- app/.codeowner is referencing an invalid team - 'Typo'; this directory is currently inheriting its owner from the project root\n", + )); + Ok(()) +} From 5fa2a805648790c4d3ac3533a4d6b1c6d0f387c3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 2 Nov 2026 14:55:08 -0800 Subject: [PATCH 2/4] Let the owner lookup match a .codeowner by its glob `validate ` excuses an optionally owned file only if nothing names an owner for it. Its owner lookup matched the directory of a `.codeowner` naming a team that doesn't exist by path prefix, because the directory mapper's glob missed one at the project root and read braces as alternatives. Since the previous commit that glob matches just the directory, and the mapper builds a matcher for every `.codeowner` whatever team it names, so the lookup now uses those matchers. An empty `.codeowner`, whose owner is "", still names an owner there, and a test now pins that. A package naming a team that doesn't exist is still matched by path prefix, since the package mappers leave it out. Refs #146. --- src/ownership.rs | 29 +++++----------------------- tests/optionally_owned_globs_test.rs | 23 ++++++++++++++++++++-- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 9cefbd7..ba34b9a 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -179,37 +179,18 @@ impl Ownership { pub(crate) fn owner_lookup(&self) -> OwnerLookup { // `validate` reports a package.yml, package.json or `.codeowner` naming a team that doesn't exist, but a list of files - // isn't checked for that, so here each still names an owner for the files under its directory. The package mappers drop - // such a package, and the directory mapper's glob misses a `.codeowner` at the project root and reads braces as alternatives, - // matching `app/legacy` for one in `app/{legacy}`. So the directory is matched as a path prefix, which replaces that glob - // for such a `.codeowner`: brackets and braces are literal and the root covers every file. + // isn't checked for that, so here each still names an owner for the files under its directory. The directory mapper + // matches a `.codeowner` whatever team it names, but the package mappers drop such a package, so its directory is matched + // as a path prefix. let package_roots = self .project .packages .iter() .filter(|package| !self.project.teams_by_name.contains_key(&package.owner)) .filter_map(|package| package.package_root()); - let directory_roots = self - .project - .directory_codeowner_files - .iter() - .filter(|directory_codeowner_file| !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner)) - .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root()); OwnerLookup { - owner_matchers: self - .mappers() - .iter() - .flat_map(|mapper| mapper.owner_matchers()) - .filter(|owner_matcher| match owner_matcher { - OwnerMatcher::Glob { - source: Source::Directory(_), - team_name, - .. - } => self.project.teams_by_name.contains_key(team_name), - _ => true, - }) - .collect(), - unknown_team_roots: package_roots.chain(directory_roots).map(Path::to_path_buf).collect(), + owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(), + unknown_team_roots: package_roots.map(Path::to_path_buf).collect(), } } diff --git a/tests/optionally_owned_globs_test.rs b/tests/optionally_owned_globs_test.rs index c916837..583f0b8 100644 --- a/tests/optionally_owned_globs_test.rs +++ b/tests/optionally_owned_globs_test.rs @@ -506,7 +506,26 @@ fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_naming Ok(()) } -// A glob built from the project root, `/**/**`, would match no project-relative path. +// `validate` reports an empty `.codeowner` like one naming a team that doesn't exist. +fn add_an_empty_codeowner_file_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "") +} + +#[test] +fn test_validate_files_reports_an_optionally_owned_file_under_an_empty_codeowner() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_empty_codeowner_file_in_an_optionally_owned_glob, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading(command))) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + +// The project root's glob is `**/**`, since `/**/**` would be `/**/**` there and match no project-relative path. fn make_every_file_optionally_owned_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root( project_root: &Path, ) -> std::io::Result<()> { @@ -529,7 +548,7 @@ fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_at_the Ok(()) } -// Read as a glob, `app/deprecated/{legacy}` would match `app/deprecated/legacy` rather than its own directory. +// Unescaped, `app/deprecated/{legacy}/**/**` would match files in `app/deprecated/legacy` rather than its own directory. fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("app/deprecated/{legacy}"))?; fs::write(project_root.join("app/deprecated/{legacy}/.codeowner"), "Typo\n")?; From 3038f762279c32a5d9d54cc9c03c7f27db885e99 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 2 Nov 2026 16:24:18 -0800 Subject: [PATCH 3/4] Count a team once in validate however its ownership names it `validate` counts each distinct team name its matchers report for a file as an owner. The team glob mapper reported a team's GitHub team, and the `.codeowner` mapper the name as written, which can also be the GitHub team, while the other mappers report the team's name. So a file that one team reached two such ways got two owners, though `for-file` and CODEOWNERS give it one. On main that happens wherever a team's owned globs cover a file the team also owns another way, as for `frontend/apps/public/index.tsx` in the javascript_only_project fixture, or where a `.codeowner` names its team by GitHub team and another mapper names it by name. Now that the root's glob matches every file, a repo that passes today would also fail wherever a root `.codeowner` or package meets an owned glob of the same team, or a root `.codeowner` naming the team by GitHub team meets an annotation or package naming it by name. Both mappers now report the team's name, and keep a name that matches no team as written. A multiple-owner report names a team glob's team by name too. Refs #146. --- src/common_test.rs | 13 ++++++++++ src/ownership/mapper.rs | 2 ++ src/ownership/mapper/directory_mapper.rs | 32 +++++++++++++++++++++++- src/ownership/mapper/team_glob_mapper.rs | 6 ++--- tests/path_globs_test.rs | 20 +++++++++++++++ 5 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/common_test.rs b/src/common_test.rs index 8c5ac5b..c330937 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -219,6 +219,19 @@ pub mod tests { ) } + pub fn build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team() -> Result> { + ownership!( + TestProjectFile { + relative_path: "app/by_handle/.codeowner".to_owned(), + content: "@Bar\n".to_owned(), + }, + TestProjectFile { + relative_path: "app/typo/.codeowner".to_owned(), + content: "Typo\n".to_owned(), + } + ) + } + pub fn build_ownership_with_all_mappers() -> Result> { ownership!( TestProjectFile { diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index a77a71d..b566b2e 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -27,6 +27,8 @@ use super::Entry; pub trait Mapper { fn name(&self) -> String; fn entries(&self) -> Vec; + /// A matcher names a team that exists by its `name`, even where a file names it by GitHub team, since `validate` counts + /// each name matching a file as a separate owner. fn owner_matchers(&self) -> Vec; } pub type TeamName = String; diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 446dabc..6dcdaf5 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -43,9 +43,16 @@ impl Mapper for DirectoryMapper { let mut owner_matchers = Vec::new(); for file in &self.project.directory_codeowner_files { + // A `.codeowner` can name its team by GitHub team. + let team_name = self + .project + .teams_by_name + .get(&file.owner) + .map(|team| team.name.clone()) + .unwrap_or_else(|| file.owner.clone()); owner_matchers.push(OwnerMatcher::new_glob( directory_glob(&file.directory_root().map(|p| p.to_string_lossy()).unwrap_or_default()), - file.owner.to_owned(), + team_name, Source::Directory(file.directory_root().map(|p| p.to_string_lossy().to_string()).unwrap_or_default()), )); } @@ -66,6 +73,7 @@ mod tests { use crate::common_test::tests::build_ownership_with_directory_codeowner_with_an_asterisk; use crate::common_test::tests::{ build_ownership_with_directory_codeowners, build_ownership_with_directory_codeowners_at_the_project_root, + build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team, build_ownership_with_directory_codeowners_with_brackets, build_ownership_with_directory_codeowners_with_glob_metacharacters, vecs_match, }; @@ -174,6 +182,28 @@ mod tests { Ok(()) } + #[test] + fn test_owner_matchers_name_a_team_by_its_name() -> Result<(), Box> { + let ownership = build_ownership_with_directory_codeowners_naming_a_github_team_and_an_unknown_team()?; + let mapper = DirectoryMapper::build(ownership.project.clone()); + vecs_match( + &mapper.owner_matchers(), + &vec![ + OwnerMatcher::new_glob( + "app/by_handle/**/**".to_owned(), + "Bar".to_owned(), + Source::Directory("app/by_handle".to_owned()), + ), + OwnerMatcher::new_glob( + "app/typo/**/**".to_owned(), + "Typo".to_owned(), + Source::Directory("app/typo".to_owned()), + ), + ], + ); + Ok(()) + } + #[test] fn test_entries_with_glob_metacharacters() -> Result<(), Box> { let ownership = build_ownership_with_directory_codeowners_with_glob_metacharacters()?; diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index be4230b..3be6505 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -41,7 +41,7 @@ impl Mapper for TeamGlobMapper { owner_matchers.push(OwnerMatcher::new_glob_with_candidate_subtracted_globs( owned_glob.clone(), &team_subtracted_globs, - team.github_team.clone(), + team.name.clone(), Source::TeamGlob(owned_glob.clone()), )) } @@ -90,7 +90,7 @@ mod tests { &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( "packs/bar/**".to_owned(), &[], - "@Baz".to_owned(), + "Baz".to_owned(), Source::TeamGlob("packs/bar/**".to_owned()), )], ); @@ -107,7 +107,7 @@ mod tests { &vec![OwnerMatcher::new_glob_with_candidate_subtracted_globs( "packs/bar/**".to_owned(), &["packs/bar/excluded/**".to_owned()], - "@Baz".to_owned(), + "Baz".to_owned(), Source::TeamGlob("packs/bar/**".to_owned()), )], ); diff --git a/tests/path_globs_test.rs b/tests/path_globs_test.rs index f551ee5..2abc86d 100644 --- a/tests/path_globs_test.rs +++ b/tests/path_globs_test.rs @@ -255,6 +255,26 @@ fn test_a_codeowner_file_in_a_directory_overrides_one_at_the_project_root() -> R Ok(()) } +#[test] +fn test_validate_counts_a_team_reached_two_ways_as_one_owner() -> Result<(), Box> { + // If `validate` counted a team glob's or a `.codeowner`'s team by its GitHub team, each file would have two owners. + let project = generated_project(&[ + ( + "config/teams/foo.yml", + "name: Foo\ngithub:\n team: \"@org/foo\"\nowned_globs:\n - \"lib/**\"\n", + ), + (".codeowner", "Foo\n"), + ("lib/x.rb", "puts 1\n"), + ("app/.codeowner", "@org/bar\n"), + ("app/annotated.rb", "# @team Bar\n"), + ])?; + for (file, team) in [("lib/x.rb", "Foo"), ("app/annotated.rb", "Bar")] { + assert_owner(project.path(), file, team)?; + } + codeowners(project.path(), &["validate"])?.success().stdout(predicate::eq("")); + Ok(()) +} + #[test] fn test_a_package_nested_in_one_at_the_project_root_owns_its_files() -> Result<(), Box> { let project = generated_project(&[ From 5f8f07db27c276e0625d61fd0828b3cad2f90cb2 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 2 Nov 2026 16:17:11 -0800 Subject: [PATCH 4/4] Cover a braced team file and a root package.json in tests Escaping only brackets in a team file's path, as the `.codeowner` and annotation mappers did before, passed every test, though the CODEOWNERS lookup would then read the line for `config/teams/{dev}.yml` as `config/teams/dev.yml`. Nor did any test cover how `for-file` describes a `package.json` at the project root, the JavaScript side of `nearest_package_owner`. Add both cases. Refs #146. --- src/common_test.rs | 4 ++++ src/ownership/file_owner_resolver.rs | 4 +++- src/ownership/mapper/team_yml_mapper.rs | 20 ++++++++++++++------ 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/common_test.rs b/src/common_test.rs index c330937..511e5ab 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -371,6 +371,10 @@ pub mod tests { relative_path: "config/teams/[ops].yml".to_owned(), content: "name: Ops\ngithub:\n team: \"@Ops\"\nruby:\n owned_gems:\n - \"{taco}\"\n".to_owned(), }, + TestProjectFile { + relative_path: "config/teams/{dev}.yml".to_owned(), + content: "name: Dev\ngithub:\n team: \"@Dev\"\n".to_owned(), + }, TestProjectFile { relative_path: "gems/{taco}/sauce.rb".to_owned(), content: "class Taco::Sauce\nend\n".to_owned(), diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index fb6ea51..bc2efe4 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -591,7 +591,7 @@ mod tests { fn test_nearest_package_owner_describes_a_package_by_the_glob_validate_matches() { let td = tempdir().unwrap(); let project_root = td.path(); - let config = build_config_for_temp("frontend/**/*", "packs/**/*", "vendored"); + let config = build_config_for_temp("**/*", "packs/**/*", "vendored"); let ruby_pkg = project_root.join("packs/{payroll}"); std::fs::create_dir_all(&ruby_pkg).unwrap(); @@ -599,6 +599,7 @@ mod tests { let js_pkg = project_root.join("frontend/[flow]"); std::fs::create_dir_all(&js_pkg).unwrap(); std::fs::write(js_pkg.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap(); + std::fs::write(project_root.join("package.json"), r#"{"metadata": {"owner": "UX"}}"#).unwrap(); let mut tbn: HashMap = HashMap::new(); for name in ["Payroll", "UX"] { @@ -609,6 +610,7 @@ mod tests { for (file, package_path, glob) in [ ("packs/{payroll}/a.rb", "packs/{payroll}/package.yml", "packs/\\{payroll\\}/**/**"), ("frontend/[flow]/a.ts", "frontend/[flow]/package.json", "frontend/\\[flow\\]/**/**"), + ("app/a.ts", "package.json", "**/**"), ] { let (_, source) = nearest_package_owner(project_root, Path::new(file), &config, &tbn).unwrap(); assert_eq!(source, Source::Package(package_path.to_string(), glob.to_string())); diff --git a/src/ownership/mapper/team_yml_mapper.rs b/src/ownership/mapper/team_yml_mapper.rs index ac78a88..83bd096 100644 --- a/src/ownership/mapper/team_yml_mapper.rs +++ b/src/ownership/mapper/team_yml_mapper.rs @@ -120,12 +120,20 @@ mod tests { let mapper = TeamYmlMapper::build(ownership.project.clone()); vecs_match( &mapper.entries(), - &vec![Entry { - path: "config/teams/\\[ops\\].yml".to_owned(), - github_team: "@Ops".to_owned(), - team_name: "Ops".to_owned(), - disabled: false, - }], + &vec![ + Entry { + path: "config/teams/\\[ops\\].yml".to_owned(), + github_team: "@Ops".to_owned(), + team_name: "Ops".to_owned(), + disabled: false, + }, + Entry { + path: "config/teams/\\{dev\\}.yml".to_owned(), + github_team: "@Dev".to_owned(), + team_name: "Dev".to_owned(), + disabled: false, + }, + ], ); Ok(()) }