From 93b6d92fb1807858aa87ca5907ad3b0bbf092420 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 10:38:03 -0700 Subject: [PATCH 01/38] Add allow_unowned_files to permit files with no owner code_ownership#140: a repo that assigns owners to only some files had no way to pass `validate` except `unowned_globs: ['**/*']`, and files matched by unowned_globs are never read, so that also switched off their `@team` annotations. With `allow_unowned_files: true`, a file in owned_globs that no mechanism assigns an owner is no longer an error, in both `validate` and `validate `. Every mechanism, file annotations included, still applies to those files, so the annotations keep working. It defaults to false, and nothing else changes: unowned_globs files are still never read, which is what keeps large third-party trees cheap. Making annotations work inside unowned_globs instead would have meant opening every file those globs match. Measured on a synthetic tree of tracked vendored files, that made a cold `generate` about 7x slower at 100k files and 11x at 300k. --- README.md | 5 +- src/config.rs | 21 ++++ src/ownership.rs | 1 + src/ownership/file_owner_resolver.rs | 1 + src/ownership/validator.rs | 5 +- src/project.rs | 2 + src/project_builder.rs | 1 + src/runner.rs | 1 + tests/allow_unowned_files_test.rs | 98 +++++++++++++++++++ .../allow_unowned_files/.github/CODEOWNERS | 14 +++ .../allow_unowned_files/app/annotated.rb | 2 + .../allow_unowned_files/app/unowned.rb | 1 + .../config/code_ownership.yml | 4 + .../allow_unowned_files/config/teams/foo.yml | 4 + 14 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 tests/allow_unowned_files_test.rs create mode 100644 tests/fixtures/allow_unowned_files/.github/CODEOWNERS create mode 100644 tests/fixtures/allow_unowned_files/app/annotated.rb create mode 100644 tests/fixtures/allow_unowned_files/app/unowned.rb create mode 100644 tests/fixtures/allow_unowned_files/config/code_ownership.yml create mode 100644 tests/fixtures/allow_unowned_files/config/teams/foo.yml diff --git a/README.md b/README.md index ebbafcf..904827e 100644 --- a/README.md +++ b/README.md @@ -209,11 +209,12 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`) +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here don't need an owner, and they're never read, so file annotations in them are ignored. Use this for third-party or generated code. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. +- `allow_unowned_files` (default: `false`): When `true`, files in `owned_globs` that no mechanism assigns an owner are not a validation error. Every ownership mechanism, including file annotations, still applies to them. Use this when only some of your code has owners; unlike putting files in `unowned_globs`, their annotations keep working. Example configuration with custom executable name: @@ -238,7 +239,7 @@ By default, cache is stored under `tmp/cache/codeowners` relative to the project 1. Only one mechanism defines ownership for any file. 2. All referenced teams are valid. -3. All files in `owned_globs` are owned, unless matched by `unowned_globs`. +3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `allow_unowned_files` is set. 4. The generated `CODEOWNERS` file is up to date. Exit status is non-zero on errors. diff --git a/src/config.rs b/src/config.rs index 620185f..3674525 100644 --- a/src/config.rs +++ b/src/config.rs @@ -31,6 +31,9 @@ pub struct Config { #[serde(default = "default_codeowners_path")] pub codeowners_path: String, + + #[serde(default)] + pub allow_unowned_files: bool, } #[allow(dead_code)] @@ -168,6 +171,24 @@ mod tests { let config_file = File::open(&config_path)?; let config: Config = serde_yaml::from_reader(config_file)?; assert_eq!(config.executable_name, "codeowners generate"); + assert!(!config.allow_unowned_files); + Ok(()) + } + + #[test] + fn test_parse_config_with_allow_unowned_files() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + allow_unowned_files: true + "}; + fs::write(&config_path, config_str)?; + let config_file = File::open(&config_path)?; + let config: Config = serde_yaml::from_reader(config_file)?; + assert!(config.allow_unowned_files); Ok(()) } diff --git a/src/ownership.rs b/src/ownership.rs index c0422e4..0d0ac2d 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -126,6 +126,7 @@ impl Ownership { mappers: self.codeowners_file_mappers(), }, executable_name: self.project.executable_name.clone(), + allow_unowned_files: self.project.allow_unowned_files, }; validator.validate() diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index cc7f9fa..3276a9d 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -377,6 +377,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + allow_unowned_files: false, } } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 3f5339c..af8ddac 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -22,6 +22,7 @@ pub struct Validator { pub mappers: Vec>, pub file_generator: FileGenerator, pub executable_name: String, + pub allow_unowned_files: bool, } #[derive(Debug)] @@ -172,7 +173,9 @@ impl Validator { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { - validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + if !self.allow_unowned_files { + validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + } } else if owners.len() > 1 { validation_errors.push(Error::FileWithMultipleOwners { path: relative_path, diff --git a/src/project.rs b/src/project.rs index 12b09a6..1176f62 100644 --- a/src/project.rs +++ b/src/project.rs @@ -18,6 +18,7 @@ pub struct Project { pub directory_codeowner_files: Vec, pub teams_by_name: HashMap, pub executable_name: String, + pub allow_unowned_files: bool, } #[derive(Clone, Debug)] @@ -222,6 +223,7 @@ mod tests { directory_codeowner_files: vec![], teams_by_name: HashMap::new(), executable_name: "codeowners generate".to_string(), + allow_unowned_files: false, }; let map = project.vendored_gem_by_name(); diff --git a/src/project_builder.rs b/src/project_builder.rs index e0d06ee..f5ed2a0 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -325,6 +325,7 @@ impl<'a> ProjectBuilder<'a> { directory_codeowner_files: directory_codeowners, teams_by_name, executable_name: self.config.executable_name.clone(), + allow_unowned_files: self.config.allow_unowned_files, }) } } diff --git a/src/runner.rs b/src/runner.rs index 5562979..a72a44e 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -168,6 +168,7 @@ impl Runner { for file_path in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} + Ok(None) if self.config.allow_unowned_files => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } diff --git a/tests/allow_unowned_files_test.rs b/tests/allow_unowned_files_test.rs new file mode 100644 index 0000000..6196d08 --- /dev/null +++ b/tests/allow_unowned_files_test.rs @@ -0,0 +1,98 @@ +use assert_cmd::prelude::*; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/allow_unowned_files"; + +#[test] +fn test_validate_passes_with_unowned_files() -> Result<(), Box> { + run_codeowners("allow_unowned_files", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_generate_and_validate_passes_with_unowned_files() -> Result<(), Box> { + run_codeowners("allow_unowned_files", &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_unowned_file() -> Result<(), Box> { + run_codeowners( + "allow_unowned_files", + &["validate", "app/unowned.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + Ok(()) +} + +#[test] +fn test_annotations_still_assign_owners() -> Result<(), Box> { + run_codeowners( + "allow_unowned_files", + &["for-file", "app/annotated.rb"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {" + Team: Foo + Github Team: @FooTeam + Team YML: config/teams/foo.yml + Description: + - Owner annotation at the top of the file + "}), + )?; + Ok(()) +} + +#[test] +fn test_unowned_file_is_reported_as_unowned() -> Result<(), Box> { + run_codeowners( + "allow_unowned_files", + &["for-file", "app/unowned.rb"], + true, + OutputStream::Stdout, + predicate::str::contains("Team: Unowned"), + )?; + Ok(()) +} + +fn run_without_the_flag(args: &[&str]) -> Result> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\n", + )?; + git_add_all_files(project_root); + let assert = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert(); + Ok(assert) +} + +#[test] +fn test_validate_reports_unowned_files_without_the_flag() -> Result<(), Box> { + run_without_the_flag(&["validate"])? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/unowned.rb")); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_unowned_file_without_the_flag() -> Result<(), Box> { + run_without_the_flag(&["validate", "app/unowned.rb"])? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/unowned.rb")); + Ok(()) +} diff --git a/tests/fixtures/allow_unowned_files/.github/CODEOWNERS b/tests/fixtures/allow_unowned_files/.github/CODEOWNERS new file mode 100644 index 0000000..0667b79 --- /dev/null +++ b/tests/fixtures/allow_unowned_files/.github/CODEOWNERS @@ -0,0 +1,14 @@ +# STOP! - DO NOT EDIT THIS FILE MANUALLY +# This file was automatically generated by "bin/codeownership validate". +# +# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub +# teams. This is useful when developers create Pull Requests since the +# code/file owner is notified. Reference GitHub docs for more details: +# https://help.github.com/en/articles/about-code-owners + + +# Team YML ownership +/config/teams/foo.yml @FooTeam + +# Annotations at the top of file +/app/annotated.rb @FooTeam diff --git a/tests/fixtures/allow_unowned_files/app/annotated.rb b/tests/fixtures/allow_unowned_files/app/annotated.rb new file mode 100644 index 0000000..cfab2e8 --- /dev/null +++ b/tests/fixtures/allow_unowned_files/app/annotated.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated' diff --git a/tests/fixtures/allow_unowned_files/app/unowned.rb b/tests/fixtures/allow_unowned_files/app/unowned.rb new file mode 100644 index 0000000..8eb0b57 --- /dev/null +++ b/tests/fixtures/allow_unowned_files/app/unowned.rb @@ -0,0 +1 @@ +puts 'no owner' diff --git a/tests/fixtures/allow_unowned_files/config/code_ownership.yml b/tests/fixtures/allow_unowned_files/config/code_ownership.yml new file mode 100644 index 0000000..d765f38 --- /dev/null +++ b/tests/fixtures/allow_unowned_files/config/code_ownership.yml @@ -0,0 +1,4 @@ +--- +owned_globs: + - "{app,config}/**/*.rb" +allow_unowned_files: true diff --git a/tests/fixtures/allow_unowned_files/config/teams/foo.yml b/tests/fixtures/allow_unowned_files/config/teams/foo.yml new file mode 100644 index 0000000..3b226c9 --- /dev/null +++ b/tests/fixtures/allow_unowned_files/config/teams/foo.yml @@ -0,0 +1,4 @@ +--- +name: Foo +github: + team: '@FooTeam' From 0b91d2964006a0c3a35d52b6ad0153a51f1ffdaf Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 13:01:06 -0700 Subject: [PATCH 02/38] Take globs in allow_unowned_globs rather than a boolean A boolean could only exempt every file at once, while a glob list can also scope the exemption, e.g. `**/deprecated/**/*`, and `['**/*']` still covers code_ownership#140's case. It matches how owned_globs and unowned_globs are configured. The difference from unowned_globs is that files here are still read, so annotations and every other mechanism still apply; they just don't have to have an owner. The globs are only checked for files that have no owner, using the same glob_match used for owned_globs on every walked file. --- README.md | 4 +- src/config.rs | 11 +- src/ownership.rs | 2 +- src/ownership/file_owner_resolver.rs | 2 +- src/ownership/validator.rs | 5 +- src/project.rs | 4 +- src/project_builder.rs | 4 +- src/runner.rs | 21 +-- tests/allow_unowned_files_test.rs | 98 -------------- tests/allow_unowned_globs_test.rs | 120 ++++++++++++++++++ .../config/code_ownership.yml | 4 - .../.github/CODEOWNERS | 1 + .../app/annotated.rb | 0 .../app/deprecated/annotated_old.rb | 2 + .../app/deprecated/old.rb} | 0 .../config/code_ownership.yml | 5 + .../config/teams/foo.yml | 0 17 files changed, 157 insertions(+), 126 deletions(-) delete mode 100644 tests/allow_unowned_files_test.rs create mode 100644 tests/allow_unowned_globs_test.rs delete mode 100644 tests/fixtures/allow_unowned_files/config/code_ownership.yml rename tests/fixtures/{allow_unowned_files => allow_unowned_globs}/.github/CODEOWNERS (92%) rename tests/fixtures/{allow_unowned_files => allow_unowned_globs}/app/annotated.rb (100%) create mode 100644 tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb rename tests/fixtures/{allow_unowned_files/app/unowned.rb => allow_unowned_globs/app/deprecated/old.rb} (100%) create mode 100644 tests/fixtures/allow_unowned_globs/config/code_ownership.yml rename tests/fixtures/{allow_unowned_files => allow_unowned_globs}/config/teams/foo.yml (100%) diff --git a/README.md b/README.md index 904827e..9a08729 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `allow_unowned_files` (default: `false`): When `true`, files in `owned_globs` that no mechanism assigns an owner are not a validation error. Every ownership mechanism, including file annotations, still applies to them. Use this when only some of your code has owners; unlike putting files in `unowned_globs`, their annotations keep working. +- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, they're still read and every ownership mechanism, including file annotations, still applies to them. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. Example configuration with custom executable name: @@ -239,7 +239,7 @@ By default, cache is stored under `tmp/cache/codeowners` relative to the project 1. Only one mechanism defines ownership for any file. 2. All referenced teams are valid. -3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `allow_unowned_files` is set. +3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `allow_unowned_globs`. 4. The generated `CODEOWNERS` file is up to date. Exit status is non-zero on errors. diff --git a/src/config.rs b/src/config.rs index 3674525..232b48d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -33,7 +33,7 @@ pub struct Config { pub codeowners_path: String, #[serde(default)] - pub allow_unowned_files: bool, + pub allow_unowned_globs: Vec, } #[allow(dead_code)] @@ -171,24 +171,25 @@ mod tests { let config_file = File::open(&config_path)?; let config: Config = serde_yaml::from_reader(config_file)?; assert_eq!(config.executable_name, "codeowners generate"); - assert!(!config.allow_unowned_files); + assert!(config.allow_unowned_globs.is_empty()); Ok(()) } #[test] - fn test_parse_config_with_allow_unowned_files() -> Result<(), Box> { + fn test_parse_config_with_allow_unowned_globs() -> Result<(), Box> { let temp_dir = tempdir()?; let config_path = temp_dir.path().join("config.yml"); let config_str = indoc! {" --- owned_globs: - \"**/*.rb\" - allow_unowned_files: true + allow_unowned_globs: + - \"**/deprecated/**/*\" "}; fs::write(&config_path, config_str)?; let config_file = File::open(&config_path)?; let config: Config = serde_yaml::from_reader(config_file)?; - assert!(config.allow_unowned_files); + assert_eq!(config.allow_unowned_globs, vec!["**/deprecated/**/*"]); Ok(()) } diff --git a/src/ownership.rs b/src/ownership.rs index 0d0ac2d..41c5e42 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -126,7 +126,7 @@ impl Ownership { mappers: self.codeowners_file_mappers(), }, executable_name: self.project.executable_name.clone(), - allow_unowned_files: self.project.allow_unowned_files, + allow_unowned_globs: self.project.allow_unowned_globs.clone(), }; validator.validate() diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 3276a9d..ab6d2ec 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -377,7 +377,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), - allow_unowned_files: false, + allow_unowned_globs: vec![], } } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index af8ddac..70a59c4 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,4 +1,5 @@ use crate::project::{Project, ProjectFile}; +use crate::project_builder::matches_globs; use core::fmt; use std::collections::HashSet; use std::fmt::Display; @@ -22,7 +23,7 @@ pub struct Validator { pub mappers: Vec>, pub file_generator: FileGenerator, pub executable_name: String, - pub allow_unowned_files: bool, + pub allow_unowned_globs: Vec, } #[derive(Debug)] @@ -173,7 +174,7 @@ impl Validator { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { - if !self.allow_unowned_files { + if !matches_globs(&relative_path, &self.allow_unowned_globs) { validation_errors.push(Error::FileWithoutOwner { path: relative_path }) } } else if owners.len() > 1 { diff --git a/src/project.rs b/src/project.rs index 1176f62..3f21b7c 100644 --- a/src/project.rs +++ b/src/project.rs @@ -18,7 +18,7 @@ pub struct Project { pub directory_codeowner_files: Vec, pub teams_by_name: HashMap, pub executable_name: String, - pub allow_unowned_files: bool, + pub allow_unowned_globs: Vec, } #[derive(Clone, Debug)] @@ -223,7 +223,7 @@ mod tests { directory_codeowner_files: vec![], teams_by_name: HashMap::new(), executable_name: "codeowners generate".to_string(), - allow_unowned_files: false, + allow_unowned_globs: vec![], }; let map = project.vendored_gem_by_name(); diff --git a/src/project_builder.rs b/src/project_builder.rs index f5ed2a0..34c4d81 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -325,12 +325,12 @@ impl<'a> ProjectBuilder<'a> { directory_codeowner_files: directory_codeowners, teams_by_name, executable_name: self.config.executable_name.clone(), - allow_unowned_files: self.config.allow_unowned_files, + allow_unowned_globs: self.config.allow_unowned_globs.clone(), }) } } -fn matches_globs(path: &Path, globs: &[String]) -> bool { +pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { match path.to_str() { Some(s) => globs.iter().any(|glob| glob_match(glob, s)), None => false, diff --git a/src/runner.rs b/src/runner.rs index a72a44e..2e7abe0 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -146,21 +146,24 @@ impl Runner { let mut unowned_files = Vec::new(); let mut io_errors = Vec::new(); + let relative_to_root = |file_path: &str| { + let path = Path::new(file_path); + if path.is_absolute() { + path.strip_prefix(&self.run_config.project_root).unwrap_or(path).to_path_buf() + } else { + path.to_path_buf() + } + }; + // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs let filtered_paths: Vec = file_paths .into_iter() .filter(|file_path| { - // Convert to relative path for glob matching - let path = Path::new(file_path); - let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) - } else { - path - }; + let relative_path = relative_to_root(file_path); // Mirror the filtering applied by ProjectBuilder when walking the project - matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) + matches_globs(&relative_path, &self.config.owned_globs) && !matches_globs(&relative_path, &self.config.unowned_globs) }) .collect(); @@ -168,7 +171,7 @@ impl Runner { for file_path in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - Ok(None) if self.config.allow_unowned_files => {} + Ok(None) if matches_globs(&relative_to_root(&file_path), &self.config.allow_unowned_globs) => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } diff --git a/tests/allow_unowned_files_test.rs b/tests/allow_unowned_files_test.rs deleted file mode 100644 index 6196d08..0000000 --- a/tests/allow_unowned_files_test.rs +++ /dev/null @@ -1,98 +0,0 @@ -use assert_cmd::prelude::*; -use indoc::indoc; -use predicates::prelude::*; -use std::{error::Error, fs, path::Path, process::Command}; - -mod common; -use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; - -const FIXTURE: &str = "tests/fixtures/allow_unowned_files"; - -#[test] -fn test_validate_passes_with_unowned_files() -> Result<(), Box> { - run_codeowners("allow_unowned_files", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; - Ok(()) -} - -#[test] -fn test_generate_and_validate_passes_with_unowned_files() -> Result<(), Box> { - run_codeowners("allow_unowned_files", &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; - Ok(()) -} - -#[test] -fn test_validate_files_passes_for_an_unowned_file() -> Result<(), Box> { - run_codeowners( - "allow_unowned_files", - &["validate", "app/unowned.rb"], - true, - OutputStream::Stdout, - predicate::eq(""), - )?; - Ok(()) -} - -#[test] -fn test_annotations_still_assign_owners() -> Result<(), Box> { - run_codeowners( - "allow_unowned_files", - &["for-file", "app/annotated.rb"], - true, - OutputStream::Stdout, - predicate::eq(indoc! {" - Team: Foo - Github Team: @FooTeam - Team YML: config/teams/foo.yml - Description: - - Owner annotation at the top of the file - "}), - )?; - Ok(()) -} - -#[test] -fn test_unowned_file_is_reported_as_unowned() -> Result<(), Box> { - run_codeowners( - "allow_unowned_files", - &["for-file", "app/unowned.rb"], - true, - OutputStream::Stdout, - predicate::str::contains("Team: Unowned"), - )?; - Ok(()) -} - -fn run_without_the_flag(args: &[&str]) -> Result> { - let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); - let project_root = temp_dir.path(); - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\n", - )?; - git_add_all_files(project_root); - let assert = Command::cargo_bin("codeowners")? - .arg("--project-root") - .arg(project_root) - .arg("--no-cache") - .args(args) - .assert(); - Ok(assert) -} - -#[test] -fn test_validate_reports_unowned_files_without_the_flag() -> Result<(), Box> { - run_without_the_flag(&["validate"])? - .failure() - .stdout(predicate::str::contains("Some files are missing ownership")) - .stdout(predicate::str::contains("- app/unowned.rb")); - Ok(()) -} - -#[test] -fn test_validate_files_reports_an_unowned_file_without_the_flag() -> Result<(), Box> { - run_without_the_flag(&["validate", "app/unowned.rb"])? - .failure() - .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("app/unowned.rb")); - Ok(()) -} diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs new file mode 100644 index 0000000..240c950 --- /dev/null +++ b/tests/allow_unowned_globs_test.rs @@ -0,0 +1,120 @@ +use assert_cmd::prelude::*; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; + +const FIXTURE: &str = "tests/fixtures/allow_unowned_globs"; + +#[test] +fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { + run_codeowners("allow_unowned_globs", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_generate_and_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { + run_codeowners("allow_unowned_globs", &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["validate", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + Ok(()) +} + +#[test] +fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["for-file", "app/deprecated/annotated_old.rb"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {" + Team: Foo + Github Team: @FooTeam + Team YML: config/teams/foo.yml + Description: + - Owner annotation at the top of the file + "}), + )?; + Ok(()) +} + +#[test] +fn test_unowned_file_in_an_allowed_glob_is_reported_as_unowned() -> Result<(), Box> { + run_codeowners( + "allow_unowned_globs", + &["for-file", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::str::contains("Team: Unowned"), + )?; + Ok(()) +} + +fn run_on_modified_fixture( + modify: impl FnOnce(&Path) -> std::io::Result<()>, + args: &[&str], +) -> Result> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + modify(project_root)?; + git_add_all_files(project_root); + let assert = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .args(args) + .assert(); + Ok(assert) +} + +fn add_unowned_file_outside_allowed_globs(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/stray.rb"), "puts 'no owner'\n") +} + +#[test] +fn test_validate_reports_unowned_files_outside_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_allowed_globs, &["validate"])? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/stray.rb")) + .stdout(predicate::str::contains("app/deprecated/old.rb").not()); + Ok(()) +} + +#[test] +fn test_validate_files_reports_an_unowned_file_outside_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_allowed_globs, &["validate", "app/stray.rb"])? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/stray.rb")); + Ok(()) +} + +#[test] +fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture( + |project_root| { + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\n", + ) + }, + &["validate"], + )? + .failure() + .stdout(predicate::str::contains("Some files are missing ownership")) + .stdout(predicate::str::contains("- app/deprecated/old.rb")); + Ok(()) +} diff --git a/tests/fixtures/allow_unowned_files/config/code_ownership.yml b/tests/fixtures/allow_unowned_files/config/code_ownership.yml deleted file mode 100644 index d765f38..0000000 --- a/tests/fixtures/allow_unowned_files/config/code_ownership.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -owned_globs: - - "{app,config}/**/*.rb" -allow_unowned_files: true diff --git a/tests/fixtures/allow_unowned_files/.github/CODEOWNERS b/tests/fixtures/allow_unowned_globs/.github/CODEOWNERS similarity index 92% rename from tests/fixtures/allow_unowned_files/.github/CODEOWNERS rename to tests/fixtures/allow_unowned_globs/.github/CODEOWNERS index 0667b79..b117ec9 100644 --- a/tests/fixtures/allow_unowned_files/.github/CODEOWNERS +++ b/tests/fixtures/allow_unowned_globs/.github/CODEOWNERS @@ -12,3 +12,4 @@ # Annotations at the top of file /app/annotated.rb @FooTeam +/app/deprecated/annotated_old.rb @FooTeam diff --git a/tests/fixtures/allow_unowned_files/app/annotated.rb b/tests/fixtures/allow_unowned_globs/app/annotated.rb similarity index 100% rename from tests/fixtures/allow_unowned_files/app/annotated.rb rename to tests/fixtures/allow_unowned_globs/app/annotated.rb diff --git a/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb b/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb new file mode 100644 index 0000000..ca506d0 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated, in an allowed glob' diff --git a/tests/fixtures/allow_unowned_files/app/unowned.rb b/tests/fixtures/allow_unowned_globs/app/deprecated/old.rb similarity index 100% rename from tests/fixtures/allow_unowned_files/app/unowned.rb rename to tests/fixtures/allow_unowned_globs/app/deprecated/old.rb diff --git a/tests/fixtures/allow_unowned_globs/config/code_ownership.yml b/tests/fixtures/allow_unowned_globs/config/code_ownership.yml new file mode 100644 index 0000000..8fd3663 --- /dev/null +++ b/tests/fixtures/allow_unowned_globs/config/code_ownership.yml @@ -0,0 +1,5 @@ +--- +owned_globs: + - "{app,config}/**/*.rb" +allow_unowned_globs: + - "app/deprecated/**/*" diff --git a/tests/fixtures/allow_unowned_files/config/teams/foo.yml b/tests/fixtures/allow_unowned_globs/config/teams/foo.yml similarity index 100% rename from tests/fixtures/allow_unowned_files/config/teams/foo.yml rename to tests/fixtures/allow_unowned_globs/config/teams/foo.yml From 2060252e77552c50ca02c153c2c31dd5104f046c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 13:08:41 -0700 Subject: [PATCH 03/38] Keep glob matching local to the validator and reuse relative_to_buf The validator imported matches_globs from project_builder, which made project_builder's copy crate-public just to share a one-line helper. It now has its own private copy, like runner.rs and file_owner_resolver.rs, and project_builder's copy is private again. validate_files' relative-path closure now calls path_utils::relative_to_buf instead of hand-rolling it. Also pin that `allow_unowned_globs: ['**/*']`, the README's way to allow every unowned file, covers files at the project root, for both `validate` and `validate `. --- src/ownership/validator.rs | 9 ++++++++- src/project_builder.rs | 2 +- src/runner.rs | 9 +-------- tests/allow_unowned_globs_test.rs | 19 +++++++++++++++++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 70a59c4..4d5e1ab 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,11 +1,11 @@ use crate::project::{Project, ProjectFile}; -use crate::project_builder::matches_globs; use core::fmt; use std::collections::HashSet; use std::fmt::Display; use std::path::{Path, PathBuf}; use std::sync::Arc; +use fast_glob::glob_match; use itertools::Itertools; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; @@ -341,6 +341,13 @@ impl Display for Errors { impl core::error::Error for Errors {} +fn matches_globs(path: &Path, globs: &[String]) -> bool { + match path.to_str() { + Some(s) => globs.iter().any(|glob| glob_match(glob, s)), + None => false, + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/project_builder.rs b/src/project_builder.rs index 34c4d81..1b1a52a 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -330,7 +330,7 @@ impl<'a> ProjectBuilder<'a> { } } -pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { +fn matches_globs(path: &Path, globs: &[String]) -> bool { match path.to_str() { Some(s) => globs.iter().any(|glob| glob_match(glob, s)), None => false, diff --git a/src/runner.rs b/src/runner.rs index 2e7abe0..e29f010 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -146,14 +146,7 @@ impl Runner { let mut unowned_files = Vec::new(); let mut io_errors = Vec::new(); - let relative_to_root = |file_path: &str| { - let path = Path::new(file_path); - if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path).to_path_buf() - } else { - path.to_path_buf() - } - }; + let relative_to_root = |file_path: &str| crate::path_utils::relative_to_buf(&self.run_config.project_root, Path::new(file_path)); // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 240c950..1ec16b8 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -118,3 +118,22 @@ fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box .stdout(predicate::str::contains("- app/deprecated/old.rb")); Ok(()) } + +fn allow_every_unowned_file_with_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("root.rb"), "puts 'no owner'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + ) +} + +#[test] +fn test_double_star_glob_allows_unowned_files_at_the_project_root() -> Result<(), Box> { + run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate"])? + .success() + .stdout(predicate::eq("")); + run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate", "root.rb"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} From b694dda422b965ab8b630288e778757215689c27 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 13:17:41 -0700 Subject: [PATCH 04/38] Share one matches_globs in path_utils runner.rs, project_builder.rs and the validator each had an identical private matches_globs. They now all use path_utils::matches_globs, next to relative_to and relative_to_buf, with a unit test that `**/*` matches both root-level and nested paths. --- src/ownership/validator.rs | 9 +-------- src/path_utils.rs | 15 +++++++++++++++ src/project_builder.rs | 10 ++-------- src/runner.rs | 11 ++--------- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 4d5e1ab..cd807bf 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,3 +1,4 @@ +use crate::path_utils::matches_globs; use crate::project::{Project, ProjectFile}; use core::fmt; use std::collections::HashSet; @@ -5,7 +6,6 @@ use std::fmt::Display; use std::path::{Path, PathBuf}; use std::sync::Arc; -use fast_glob::glob_match; use itertools::Itertools; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; @@ -341,13 +341,6 @@ impl Display for Errors { impl core::error::Error for Errors {} -fn matches_globs(path: &Path, globs: &[String]) -> bool { - match path.to_str() { - Some(s) => globs.iter().any(|glob| glob_match(glob, s)), - None => false, - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/path_utils.rs b/src/path_utils.rs index 230b1d1..5fa95ca 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -10,10 +10,25 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// Returns true if `path` matches any of the provided glob patterns. +pub fn matches_globs(path: &Path, globs: &[String]) -> bool { + match path.to_str() { + Some(s) => globs.iter().any(|glob| fast_glob::glob_match(glob, s)), + None => false, + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn matches_globs_double_star_matches_root_level_and_nested_files() { + let globs = vec!["**/*".to_string()]; + assert!(matches_globs(Path::new("root.rb"), &globs)); + assert!(matches_globs(Path::new("app/models/user.rb"), &globs)); + } + #[test] fn relative_to_returns_relative_when_under_root() { let root = Path::new("/a/b"); diff --git a/src/project_builder.rs b/src/project_builder.rs index 1b1a52a..b0223b8 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -5,11 +5,11 @@ use std::{ }; use error_stack::{Report, ResultExt}; -use fast_glob::glob_match; use ignore::{DirEntry, WalkBuilder, WalkParallel, WalkState}; use rayon::iter::{IntoParallelIterator, ParallelIterator}; use tracing::instrument; +use crate::path_utils::matches_globs; use crate::{ cache::Cache, config::Config, @@ -330,13 +330,6 @@ impl<'a> ProjectBuilder<'a> { } } -fn matches_globs(path: &Path, globs: &[String]) -> bool { - match path.to_str() { - Some(s) => globs.iter().any(|glob| glob_match(glob, s)), - None => false, - } -} - pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { let file = File::open(path).change_context(Error::Io)?; let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; @@ -366,6 +359,7 @@ fn javascript_package_owner(path: &Path) -> Result, Report #[cfg(test)] mod tests { use super::*; + use fast_glob::glob_match; const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; diff --git a/src/runner.rs b/src/runner.rs index e29f010..fd2b585 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,8 +1,9 @@ use std::path::{Path, PathBuf}; use std::process::Command; +use crate::path_utils::matches_globs; + use error_stack::{Report, ResultExt}; -use fast_glob::glob_match; use serde::Serialize; use tracing::debug_span; @@ -439,14 +440,6 @@ impl RunResult { } } -/// Returns true if `path` matches any of the provided glob patterns. -fn matches_globs(path: &Path, globs: &[String]) -> bool { - match path.to_str() { - Some(s) => globs.iter().any(|glob| glob_match(glob, s)), - None => false, - } -} - #[cfg(test)] mod tests { use super::*; From 9edb0efb973588dd13ea6a5cdf23f787d9136aa8 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 14:06:55 -0700 Subject: [PATCH 05/38] Test allow_unowned_globs with absolute paths and the checks it keeps Nothing tested that `validate ` matched allow_unowned_globs against the project-relative path. Matching the raw argument instead passed the whole suite, because the only test passed a path that was already relative. A test with an absolute path now covers it. validate_files also relativizes each path once, carrying it through the owned_globs filter, instead of twice per file. Two more tests pin the README's claim that only the missing-owner check is waived: a file in an allowed glob still fails when it has more than one owner or names an invalid team. The validator now reads allow_unowned_globs from its project instead of keeping a copy, matches_globs is pub(crate) so it doesn't become public API, and its imports sit with the other crate imports. --- src/ownership.rs | 1 - src/ownership/validator.rs | 3 +- src/path_utils.rs | 2 +- src/project_builder.rs | 2 +- src/runner.rs | 23 +++++++------- tests/allow_unowned_globs_test.rs | 53 +++++++++++++++++++++++++++++++ 6 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 41c5e42..c0422e4 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -126,7 +126,6 @@ impl Ownership { mappers: self.codeowners_file_mappers(), }, executable_name: self.project.executable_name.clone(), - allow_unowned_globs: self.project.allow_unowned_globs.clone(), }; validator.validate() diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index cd807bf..e796572 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -23,7 +23,6 @@ pub struct Validator { pub mappers: Vec>, pub file_generator: FileGenerator, pub executable_name: String, - pub allow_unowned_globs: Vec, } #[derive(Debug)] @@ -174,7 +173,7 @@ impl Validator { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { - if !matches_globs(&relative_path, &self.allow_unowned_globs) { + if !matches_globs(&relative_path, &self.project.allow_unowned_globs) { validation_errors.push(Error::FileWithoutOwner { path: relative_path }) } } else if owners.len() > 1 { diff --git a/src/path_utils.rs b/src/path_utils.rs index 5fa95ca..6c42be8 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -11,7 +11,7 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { } /// Returns true if `path` matches any of the provided glob patterns. -pub fn matches_globs(path: &Path, globs: &[String]) -> bool { +pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { match path.to_str() { Some(s) => globs.iter().any(|glob| fast_glob::glob_match(glob, s)), None => false, diff --git a/src/project_builder.rs b/src/project_builder.rs index b0223b8..34fc37e 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -9,10 +9,10 @@ use ignore::{DirEntry, WalkBuilder, WalkParallel, WalkState}; use rayon::iter::{IntoParallelIterator, ParallelIterator}; use tracing::instrument; -use crate::path_utils::matches_globs; use crate::{ cache::Cache, config::Config, + path_utils::matches_globs, project::{DirectoryCodeownersFile, Error, Package, PackageType, Project, ProjectFile, Team, VendoredGem, deserializers}, project_file_builder::ProjectFileBuilder, tracked_files, diff --git a/src/runner.rs b/src/runner.rs index fd2b585..b9c5ce4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,8 +1,6 @@ use std::path::{Path, PathBuf}; use std::process::Command; -use crate::path_utils::matches_globs; - use error_stack::{Report, ResultExt}; use serde::Serialize; use tracing::debug_span; @@ -11,6 +9,7 @@ use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, + path_utils::{matches_globs, relative_to_buf}, project_builder::ProjectBuilder, }; @@ -147,25 +146,25 @@ impl Runner { let mut unowned_files = Vec::new(); let mut io_errors = Vec::new(); - let relative_to_root = |file_path: &str| crate::path_utils::relative_to_buf(&self.run_config.project_root, Path::new(file_path)); - // Filter files based on owned_globs and unowned_globs configuration // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec = file_paths + let filtered_paths: Vec<(String, PathBuf)> = file_paths .into_iter() - .filter(|file_path| { - let relative_path = relative_to_root(file_path); - - // Mirror the filtering applied by ProjectBuilder when walking the project - matches_globs(&relative_path, &self.config.owned_globs) && !matches_globs(&relative_path, &self.config.unowned_globs) + .map(|file_path| { + let relative_path = relative_to_buf(&self.run_config.project_root, Path::new(&file_path)); + (file_path, relative_path) + }) + // Mirror the filtering applied by ProjectBuilder when walking the project + .filter(|(_, relative_path)| { + matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) }) .collect(); debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { + for (file_path, relative_path) in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - Ok(None) if matches_globs(&relative_to_root(&file_path), &self.config.allow_unowned_globs) => {} + Ok(None) if matches_globs(&relative_path, &self.config.allow_unowned_globs) => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 1ec16b8..78c877b 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -32,6 +32,26 @@ fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result Ok(()) } +#[test] +fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + let file_absolute_path = project_root.join("app/deprecated/old.rb").canonicalize()?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(file_absolute_path) + .assert() + .success() + .stdout(predicate::eq("")); + Ok(()) +} + #[test] fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box> { run_codeowners( @@ -102,6 +122,39 @@ fn test_validate_files_reports_an_unowned_file_outside_allowed_globs() -> Result Ok(()) } +fn add_a_second_team_owning_the_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("config/teams/bar.yml"), + "---\nname: Bar\ngithub:\n team: '@BarTeam'\nowned_globs:\n - app/deprecated/**/*\n", + ) +} + +#[test] +fn test_validate_reports_multiple_owners_in_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_a_second_team_owning_the_allowed_glob, &["validate"])? + .failure() + .stdout(predicate::str::contains( + "Code ownership should only be defined for each file in one way.", + )) + .stdout(predicate::str::contains("app/deprecated/annotated_old.rb\n owner:")); + Ok(()) +} + +fn add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/typo.rb"), "# @team Typo\nputs 'typo'\n") +} + +#[test] +fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box> { + run_on_modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, &["validate"])? + .failure() + .stdout(predicate::str::contains("Found invalid team references")) + .stdout(predicate::str::contains( + "- app/deprecated/typo.rb is referencing an invalid team - 'Typo'", + )); + Ok(()) +} + #[test] fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { run_on_modified_fixture( From 31cf459261edc62841d7f9eb580646d667d9f785 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 14:08:29 -0700 Subject: [PATCH 06/38] Explain how allow_unowned_globs differs from unowned_globs The README said unowned_globs files are never read, but `for-file` reads the top of the file before ignoring its annotation. It also implied that no ownership mechanism applies to unowned_globs files, when directory, package and team-glob rules still cover them through CODEOWNERS. The real differences are that annotations count and that the other checks still run. It now also says that unowned_globs takes precedence, and how to move off `unowned_globs: ['**/*']`, the setup code_ownership#140 started from, where adding the new key alone changes nothing. The Getting Started example moves a hand-picked app file from unowned_globs to allow_unowned_globs, since unowned_globs is meant for third-party and generated code. --- README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 9a08729..13c7703 100644 --- a/README.md +++ b/README.md @@ -66,8 +66,9 @@ cargo install --git https://github.com/rubyatscale/codeowners-rs codeowners js_package_paths: [] unowned_globs: - db/**/* - - app/services/some_file1.rb - frontend/javascripts/**/__generated__/**/* + allow_unowned_globs: + - app/services/some_file1.rb ``` 2. **Declare Teams** @@ -209,12 +210,12 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here don't need an owner, and they're never read, so file annotations in them are ignored. Use this for third-party or generated code. +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. Use this for third-party or generated code. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, they're still read and every ownership mechanism, including file annotations, still applies to them. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. +- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, file annotations in them still count, and every other check still runs, so a file there can't have more than one owner or name an invalid team. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work. Those files then get the other checks too, which may report problems that were hidden before. Example configuration with custom executable name: From 62ed8d7d31540165f6678943951776162478919d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 28 Sep 2026 14:09:47 -0700 Subject: [PATCH 07/38] Bump version to 0.6.0 A minor bump. Since 0.5.0: - `allow_unowned_globs` (#135) is a new config key. It defaults to `[]`, so existing configs behave as before, but it adds a `pub` field to `Config`, which breaks downstream code that builds a `Config` with a struct literal. - Test git no longer follows a hook's GIT_DIR into this repo (#133). This only affects the test suite. --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9be5280..9b07a78 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -168,7 +168,7 @@ checksum = "b94f61472cee1439c0b966b47e3aca9ae07e45d070759512cd390ea2bebc6675" [[package]] name = "codeowners" -version = "0.5.0" +version = "0.6.0" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index d95b654..b352a71 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.5.0" +version = "0.6.0" edition = "2024" [profile.release] From efc40f5e4c1dcc68c1104e107094fb7b016ed2bb Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 15:01:54 -0700 Subject: [PATCH 08/38] Read allow_unowned_globs with strings_or_nulls serde-saphyr rejects a list item with no value, `~` or `null`, so such an `allow_unowned_globs` entry made the whole config fail to parse. serde_yaml read a bare `-` as "" and kept `~` and `null` as text. Like the other list keys, the field now goes through strings_or_nulls, which reads all three as "", and reads an empty, `~` or `null` list as empty: serde-saphyr 2.0 does that by itself, but 2.1 stops doing it for a Vec. The empty-values config test now covers this key too. --- src/config.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 2c44752..9a09349 100644 --- a/src/config.rs +++ b/src/config.rs @@ -44,7 +44,7 @@ pub struct Config { #[serde(default = "default_codeowners_path", deserialize_with = "crate::yaml::string_or_null")] pub codeowners_path: String, - #[serde(default)] + #[serde(default, deserialize_with = "crate::yaml::strings_or_nulls")] pub allow_unowned_globs: Vec, #[serde(default = "default_yaml_max_nodes")] @@ -325,6 +325,8 @@ mod tests { - ignore_dirs: - + allow_unowned_globs: + - vendored_gems_path: cache_directory: executable_name: @@ -339,6 +341,7 @@ mod tests { &config.team_file_glob, &config.unowned_globs, &config.ignore_dirs, + &config.allow_unowned_globs, ] { assert_eq!(globs, &[""]); } From 5b28431e0551ae9ff96409bfe1bf9a3ab0a002e8 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 16:37:38 -0700 Subject: [PATCH 09/38] Report allowed files that name an owner in validate Given a list of files, `validate` and `gv` check each one only against CODEOWNERS, and allow_unowned_globs excused every allowed file that CODEOWNERS had no line for. That included a file whose annotation names a team that doesn't exist: without the key it is reported as unowned, and a full `validate` reports the invalid team. Now the glob excuses only a file that nothing names an owner for, counting owners as a full `validate` does, so such a file is reported as unowned again, and so is one that CODEOWNERS doesn't list yet. The owner matchers are collected once, and only when an allowed file is missing from CODEOWNERS. --- src/ownership.rs | 21 +++++++++++++++++++ src/runner.rs | 9 +++++++- tests/allow_unowned_globs_test.rs | 34 +++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/ownership.rs b/src/ownership.rs index c0422e4..155b563 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -109,6 +109,21 @@ impl Entry { } } +/// Tells whether anything names an owner for a project-relative path, counting owners as `validate` does, so an annotation +/// or `.codeowner` naming a team that doesn't exist still counts. Building one goes through every project file, so reuse it. +pub(crate) struct OwnerLookup { + owner_matchers: Vec, +} + +impl OwnerLookup { + pub(crate) fn has_owner(&self, relative_path: &Path) -> bool { + let file_owner_finder = FileOwnerFinder { + owner_matchers: &self.owner_matchers, + }; + !file_owner_finder.find(relative_path).is_empty() + } +} + impl Ownership { pub fn build(project: Project) -> Self { Self { @@ -156,6 +171,12 @@ impl Ownership { .collect()) } + pub(crate) fn owner_lookup(&self) -> OwnerLookup { + OwnerLookup { + owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(), + } + } + #[instrument(level = "debug", skip_all)] pub fn for_team(&self, team_name: &str) -> Result, Box> { info!("getting team ownership for {}", team_name); diff --git a/src/runner.rs b/src/runner.rs index b9c5ce4..bdcb0cd 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,3 +1,4 @@ +use std::cell::LazyCell; use std::path::{Path, PathBuf}; use std::process::Command; @@ -160,11 +161,17 @@ impl Runner { }) .collect(); + // Built only if an allowed file needs it, and then once. + let owner_lookup = LazyCell::new(|| self.ownership.owner_lookup()); + debug_span!("per_file_query").in_scope(|| { for (file_path, relative_path) in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - Ok(None) if matches_globs(&relative_path, &self.config.allow_unowned_globs) => {} + // As in `validate`, allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation + // names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without the key. + Ok(None) + if matches_globs(&relative_path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&relative_path) => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 78c877b..3919f80 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -155,6 +155,40 @@ fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box Result<(), Box> { + // A list of files is checked only against CODEOWNERS, which has no line for a team that doesn't exist. + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + &[command, "app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/typo.rb")); + } + Ok(()) +} + +fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("app/deprecated/old.rb"), + "# @team Foo\nputs 'no longer unowned'\n", + ) +} + +#[test] +fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { + run_on_modified_fixture( + annotate_the_unowned_file_without_regenerating_codeowners, + &["validate", "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + Ok(()) +} + #[test] fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { run_on_modified_fixture( From 56c6a0d686654e02b54dc49c3cd1483ac0bdfe46 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 16:40:28 -0700 Subject: [PATCH 10/38] Test that unowned_globs takes precedence over allow_unowned_globs The README says a file matching both is left out, but no test checked it: making allow_unowned_globs win passed the whole suite. The new test gives a file matching both an annotation naming an unknown team and checks that `gv` passes, which it can only do with the file left out. --- tests/allow_unowned_globs_test.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 3919f80..853622e 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -189,6 +189,24 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu Ok(()) } +fn put_the_allowed_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\nunowned_globs:\n - \"app/deprecated/**/*\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n", + ) +} + +#[test] +fn test_unowned_globs_take_precedence_over_allow_unowned_globs() -> Result<(), Box> { + // A file in both is left out, so the unknown team its annotation names doesn't count. `gv`, not `validate`, because + // leaving files out changes CODEOWNERS. + run_on_modified_fixture(put_the_allowed_glob_in_unowned_globs_too, &["gv"])? + .success() + .stdout(predicate::eq("")); + Ok(()) +} + #[test] fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { run_on_modified_fixture( From ded8a7fb4672b8b70fa96d75a16a1006dc86d46e Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 18:30:12 -0700 Subject: [PATCH 11/38] Count a package naming an unknown team in validate The previous commit made allow_unowned_globs excuse only an allowed file that nothing names an owner for, but the owner lookup missed one kind of owner. The package mappers drop a package whose package.yml or package.json names a team that doesn't exist, so a file in it still passed `validate ` and `gv `. Without the key it is reported as unowned, and a full `validate` reports the package.yml or package.json. The lookup now adds a matcher for each such package, so the file is reported as unowned again. A full `validate` is unchanged. --- src/ownership.rs | 21 +++++++++++++++++---- src/runner.rs | 5 +++-- tests/allow_unowned_globs_test.rs | 24 ++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 155b563..33eada4 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -109,8 +109,8 @@ impl Entry { } } -/// Tells whether anything names an owner for a project-relative path, counting owners as `validate` does, so an annotation -/// or `.codeowner` naming a team that doesn't exist still counts. Building one goes through every project file, so reuse it. +/// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation, +/// `.codeowner`, `package.yml` or `package.json` naming one still counts. Building one goes through every project file, so reuse it. pub(crate) struct OwnerLookup { owner_matchers: Vec, } @@ -172,9 +172,22 @@ impl Ownership { } pub(crate) fn owner_lookup(&self) -> OwnerLookup { - OwnerLookup { - owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(), + let mut owner_matchers: Vec = self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(); + // The package mappers drop a package whose owner isn't a team, and `validate` reports its package.yml or package.json + // instead. A list of files isn't checked for that, so here the package still names an owner for its files. + for package in &self.project.packages { + if let Some(package_root) = package.package_root() + && !self.project.teams_by_name.contains_key(&package.owner) + { + let glob = format!("{}/**/**", package_root.to_string_lossy()); + owner_matchers.push(OwnerMatcher::new_glob( + glob.clone(), + package.owner.clone(), + Source::Package(package.path.to_string_lossy().to_string(), glob), + )); + } } + OwnerLookup { owner_matchers } } #[instrument(level = "debug", skip_all)] diff --git a/src/runner.rs b/src/runner.rs index bdcb0cd..8417087 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -168,8 +168,9 @@ impl Runner { for (file_path, relative_path) in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - // As in `validate`, allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation - // names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without the key. + // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or + // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without + // the key. Ok(None) if matches_globs(&relative_path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&relative_path) => {} Ok(None) => unowned_files.push(file_path), diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 853622e..e5ec551 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -170,6 +170,30 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs() -> Result<(), Bo Ok(()) } +fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/typo/app"))?; + fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/typo/app/thing.rb"), "puts 'typo'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/typo/**/*\"\n", + ) +} + +#[test] +fn test_validate_files_reports_a_package_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_package_naming_an_unknown_team_in_an_allowed_glob, + &[command, "packs/typo/app/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("packs/typo/app/thing.rb")); + } + Ok(()) +} + fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { fs::write( project_root.join("app/deprecated/old.rb"), From 351b804e4dc3c3f8cdf94e5d45f93777eaec6f9d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 18:32:57 -0700 Subject: [PATCH 12/38] Test more of how validate treats allowed files Three changes to the code passed every test. The owner lookup could leave out `.codeowner` files, so an allowed file under one naming a team that doesn't exist would pass. It could be given the path as typed rather than the project-relative one, so an absolute path would miss the file's annotation. And `validate ` applies the precedence of unowned_globs with its own filter, where letting allow_unowned_globs win went unnoticed: an annotated file in unowned_globs is never read, but a `.codeowner` there still is. A new test catches each change. The comment on the annotation test no longer says a list of files is checked only against CODEOWNERS, which stopped being true when the lookup was added. --- tests/allow_unowned_globs_test.rs | 62 ++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index e5ec551..797488c 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -157,7 +157,8 @@ fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box Result<(), Box> { - // A list of files is checked only against CODEOWNERS, which has no line for a team that doesn't exist. + // A list of files isn't checked for invalid teams. CODEOWNERS has no line for this file, and its annotation keeps + // allow_unowned_globs from excusing it, so it's reported as unowned. for command in ["validate", "gv"] { run_on_modified_fixture( add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, @@ -170,6 +171,28 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs() -> Result<(), Bo Ok(()) } +#[test] +fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_an_absolute_path() -> Result<(), Box> { + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; + git_add_all_files(project_root); + + let file_absolute_path = project_root.join("app/deprecated/typo.rb").canonicalize()?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(file_absolute_path) + .assert() + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/typo.rb")); + Ok(()) +} + fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; @@ -194,6 +217,24 @@ fn test_validate_files_reports_a_package_naming_an_unknown_team_in_allowed_globs Ok(()) } +fn add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "Typo\n") +} + +#[test] +fn test_validate_files_reports_invalid_directory_teams_in_allowed_globs() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { fs::write( project_root.join("app/deprecated/old.rb"), @@ -231,6 +272,25 @@ fn test_unowned_globs_take_precedence_over_allow_unowned_globs() -> Result<(), B Ok(()) } +fn put_a_codeowner_file_naming_an_unknown_team_in_both_globs(project_root: &Path) -> std::io::Result<()> { + put_the_allowed_glob_in_unowned_globs_too(project_root)?; + add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root) +} + +#[test] +fn test_validate_files_leaves_out_files_in_both_globs() -> Result<(), Box> { + // A `.codeowner` is read even in unowned_globs. Its team has no CODEOWNERS line, so the file passes only if it's left out. + for command in ["validate", "gv"] { + run_on_modified_fixture( + put_a_codeowner_file_naming_an_unknown_team_in_both_globs, + &[command, "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + #[test] fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box> { run_on_modified_fixture( From f15fff0d7193c805b28f449e8c205ed1aec41f4c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Wed, 21 Oct 2026 18:34:49 -0700 Subject: [PATCH 13/38] Say what unowned_globs still reads The README said files matching unowned_globs are left out and that validation doesn't check them. A `.codeowner`, package.yml or package.json among them is still read, though: its owner still goes into CODEOWNERS, and a team it names that doesn't exist is still reported. main behaves the same way; only the sentence is new here. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index dfbd9a3..5c371a9 100644 --- a/README.md +++ b/README.md @@ -212,7 +212,7 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. Use this for third-party or generated code. +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. Use this for third-party or generated code. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) From 013fdd5f39bf750448489ad8a477999d1127e06a Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 00:09:25 -0700 Subject: [PATCH 14/38] Make only absolute paths project-relative in validate Reusing path_utils::relative_to_buf in validate_files changed how it reads a relative path. The helper strips the project root from any path, but the closure it replaced, like the CODEOWNERS lookup, stripped it only from an absolute one. The CLI canonicalizes the root, but a library caller can pass a relative one, as the README's example does. A relative path that starts with that root was then checked against owned_globs with the root cut off, while the CODEOWNERS lookup read it as given and found no line for it, so runner::validate and runner::generate_and_validate reported an owned file as unowned. Before that change the file was skipped. A relative path is already relative to the project root, so it is now left as given and only an absolute one has the root stripped. Such a file is skipped again, and nothing changes when the root is absolute. --- src/runner.rs | 8 +++++++- tests/runner_api.rs | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/runner.rs b/src/runner.rs index 8417087..8f6e8f9 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -152,7 +152,13 @@ impl Runner { let filtered_paths: Vec<(String, PathBuf)> = file_paths .into_iter() .map(|file_path| { - let relative_path = relative_to_buf(&self.run_config.project_root, Path::new(&file_path)); + let path = Path::new(&file_path); + // As in the CODEOWNERS lookup, a relative path is already relative to the project root, even when that root is relative. + let relative_path = if path.is_absolute() { + relative_to_buf(&self.run_config.project_root, path) + } else { + path.to_path_buf() + }; (file_path, relative_path) }) // Mirror the filtering applied by ProjectBuilder when walking the project diff --git a/tests/runner_api.rs b/tests/runner_api.rs index 8c9d4b1..3e5f3f7 100644 --- a/tests/runner_api.rs +++ b/tests/runner_api.rs @@ -187,3 +187,24 @@ javascript_package_paths: let content = std::fs::read_to_string(td.path().join(".github/CODEOWNERS")).unwrap(); assert!(!content.is_empty()); } + +#[test] +fn test_validate_files_takes_relative_paths_as_given_with_relative_root() { + let project_root = Path::new("tests/fixtures/valid_project"); + let run_config = RunConfig { + project_root: project_root.to_path_buf(), + codeowners_file_path: Some(project_root.join(".github/CODEOWNERS").to_path_buf()), + config_path: project_root.join("config/code_ownership.yml").to_path_buf(), + no_cache: true, + executable_name: None, + }; + + // Taken as given, as the CODEOWNERS lookup takes it, this path is outside owned_globs and skipped. Stripping the root for + // the glob check alone would match it as ruby/app/models/payroll.rb and then find no CODEOWNERS line for it. + let result = runner::validate( + &run_config, + vec!["tests/fixtures/valid_project/ruby/app/models/payroll.rb".to_string()], + ); + assert!(result.io_errors.is_empty(), "io: {:?}", result.io_errors); + assert!(result.validation_errors.is_empty(), "val: {:?}", result.validation_errors); +} From d1f7de365476c29adfbf1f27de5f749c90e12dcd Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 00:23:42 -0700 Subject: [PATCH 15/38] Excuse only a plain project-relative path in validate allow_unowned_globs excuses a file in `validate ` and `gv ` only if the owner lookup finds nothing naming an owner for it. The lookup matches only paths in the project's own form, such as app/x.rb, but it was given the path as passed. So `./app/x.rb`, `app/../app/x.rb` or an absolute path not under the canonical root, as through a symlink, found no owner, and the file was excused even when its annotation, `.codeowner` or package names a team that doesn't exist. Without the key it is reported as unowned. The excuse now drops `.` components and repeated separators from the path before matching it against allow_unowned_globs and looking up its owners, so it treats `./app/x.rb` as app/x.rb, and such a file is reported again. A path with a `..`, or an absolute one not under the project root, can't be put in that form without asking the file system, so it is never excused and is reported as without the key. The owned_globs and unowned_globs checks and the CODEOWNERS lookup read the path as before, so nothing changes without the key. --- src/path_utils.rs | 39 ++++++++++++++++++++++++++- src/runner.rs | 9 ++++--- tests/allow_unowned_globs_test.rs | 44 +++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/path_utils.rs b/src/path_utils.rs index 6c42be8..4feb4fd 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; /// Return `path` relative to `root` if possible; otherwise return `path` unchanged. pub fn relative_to<'a>(root: &'a Path, path: &'a Path) -> &'a Path { @@ -10,6 +10,20 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// `path` without `.` components or repeated separators, or `None` if it has a `..`, a root or a prefix. Owners are matched +/// against plain project-relative paths, and no other path can be made into one without asking the file system. +pub(crate) fn plain_relative(path: &Path) -> Option { + let mut plain = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::Normal(name) => plain.push(name), + Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None, + } + } + Some(plain) +} + /// Returns true if `path` matches any of the provided glob patterns. pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { match path.to_str() { @@ -61,4 +75,27 @@ mod tests { let rel_buf = relative_to_buf(root, path); assert_eq!(rel_ref, rel_buf.as_path()); } + + // Compared as strings, as globs see them, since Path equality ignores a repeated separator or a `.` past the start. + fn plain(path: &str) -> Option { + plain_relative(Path::new(path)).map(|plain| plain.to_string_lossy().into_owned()) + } + + #[test] + fn plain_relative_returns_a_plain_path_unchanged() { + assert_eq!(plain("a/b").as_deref(), Some("a/b")); + } + + #[test] + fn plain_relative_drops_dot_components_and_repeated_separators() { + assert_eq!(plain("./a/b").as_deref(), Some("a/b")); + assert_eq!(plain("a/./b").as_deref(), Some("a/b")); + assert_eq!(plain("a//b").as_deref(), Some("a/b")); + } + + #[test] + fn plain_relative_returns_none_for_parent_dirs_and_roots() { + assert_eq!(plain("a/../b"), None); + assert_eq!(plain("/a/b"), None); + } } diff --git a/src/runner.rs b/src/runner.rs index 8f6e8f9..1c0be7d 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -10,7 +10,7 @@ use crate::{ cache::{Cache, Caching, file::GlobalCache, noop::NoopCache}, config::Config, ownership::{FileOwner, Ownership}, - path_utils::{matches_globs, relative_to_buf}, + path_utils::{matches_globs, plain_relative, relative_to_buf}, project_builder::ProjectBuilder, }; @@ -176,9 +176,12 @@ impl Runner { Ok(Some(_)) => {} // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without - // the key. + // the key. The lookup matches plain project-relative paths only, so a path with a `..`, or an absolute one + // not under the project root, is never excused. Ok(None) - if matches_globs(&relative_path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&relative_path) => {} + if plain_relative(&relative_path).is_some_and(|path| { + matches_globs(&path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&path) + }) => {} Ok(None) => unowned_files.push(file_path), Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 797488c..02e6afd 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -193,6 +193,50 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_an_absolute_pa Ok(()) } +#[test] +fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_a_path_with_dot_dot() -> Result<(), Box> { + // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + &[command, "app/deprecated/../deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/../deprecated/typo.rb")); + } + Ok(()) +} + +fn allow_every_file_and_annotate_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + ) +} + +#[test] +fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Box> { + // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`, so these reach the owner lookup. + for command in ["validate", "gv"] { + run_on_modified_fixture( + allow_every_file_and_annotate_one_with_an_unknown_team, + &[command, "./app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("./app/deprecated/typo.rb")); + run_on_modified_fixture( + allow_every_file_and_annotate_one_with_an_unknown_team, + &[command, "./app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; From d4a64055d15ead98be1e244c9817fde9589d97ab Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 00:33:58 -0700 Subject: [PATCH 16/38] Match a package naming an unknown team by its directory The owner lookup behind allow_unowned_globs in `validate ` and `gv ` gave each package naming a team that doesn't exist an unescaped glob of its directory. For a package in `packs/[typo]`, `[typo]` read as a character class, so a file in the package passed although a full `validate` reports its package.yml, while an allowed file with no owner in `packs/t` was reported as unowned. A package.yml at the project root got `/**/**`, which matches nothing, so the files under it passed too. The lookup now keeps the directory of each such package and finds an owner for any path under one. It compares paths a component at a time rather than as globs, so brackets are literal and a package at the project root covers every file. A file in such a package is reported as unowned again, as without the key, and the file in `packs/t` passes. A full `validate` is unchanged. The package test now covers package.json too. --- src/ownership.rs | 31 +++++++------- tests/allow_unowned_globs_test.rs | 67 +++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 19 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 33eada4..e8ff029 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -4,7 +4,7 @@ use mapper::{OwnerMatcher, Source, TeamName}; use std::{ error::Error, fmt::{self, Display}, - path::Path, + path::{Path, PathBuf}, sync::Arc, }; use tracing::{info, instrument}; @@ -113,6 +113,7 @@ impl Entry { /// `.codeowner`, `package.yml` or `package.json` naming one still counts. Building one goes through every project file, so reuse it. pub(crate) struct OwnerLookup { owner_matchers: Vec, + unknown_team_package_roots: Vec, } impl OwnerLookup { @@ -120,7 +121,8 @@ impl OwnerLookup { let file_owner_finder = FileOwnerFinder { owner_matchers: &self.owner_matchers, }; - !file_owner_finder.find(relative_path).is_empty() + self.unknown_team_package_roots.iter().any(|root| relative_path.starts_with(root)) + || !file_owner_finder.find(relative_path).is_empty() } } @@ -172,22 +174,19 @@ impl Ownership { } pub(crate) fn owner_lookup(&self) -> OwnerLookup { - let mut owner_matchers: Vec = self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(); // The package mappers drop a package whose owner isn't a team, and `validate` reports its package.yml or package.json - // instead. A list of files isn't checked for that, so here the package still names an owner for its files. - for package in &self.project.packages { - if let Some(package_root) = package.package_root() - && !self.project.teams_by_name.contains_key(&package.owner) - { - let glob = format!("{}/**/**", package_root.to_string_lossy()); - owner_matchers.push(OwnerMatcher::new_glob( - glob.clone(), - package.owner.clone(), - Source::Package(package.path.to_string_lossy().to_string(), glob), - )); - } + // instead. A list of files isn't checked for that, so here the package still names an owner for its files. Its directory + // is matched as a path prefix, not a glob, so brackets in it are literal and a package at the project root covers every file. + OwnerLookup { + owner_matchers: self.mappers().iter().flat_map(|mapper| mapper.owner_matchers()).collect(), + unknown_team_package_roots: self + .project + .packages + .iter() + .filter(|package| !self.project.teams_by_name.contains_key(&package.owner)) + .filter_map(|package| package.package_root().map(Path::to_path_buf)) + .collect(), } - OwnerLookup { owner_matchers } } #[instrument(level = "debug", skip_all)] diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 02e6afd..8b7b558 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -8,6 +8,8 @@ use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo const FIXTURE: &str = "tests/fixtures/allow_unowned_globs"; +type FixtureChange = fn(&Path) -> std::io::Result<()>; + #[test] fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { run_codeowners("allow_unowned_globs", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; @@ -247,16 +249,75 @@ fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) ) } +fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("frontend/typo/src"))?; + fs::write( + project_root.join("frontend/typo/package.json"), + "{\"metadata\": {\"owner\": \"Typo\"}}\n", + )?; + fs::write(project_root.join("frontend/typo/src/thing.js"), "console.log('typo');\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,frontend}/**/*.{rb,js}\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"frontend/typo/**/*\"\n", + ) +} + #[test] fn test_validate_files_reports_a_package_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { + let packages: [(FixtureChange, &str); 2] = [ + (add_a_package_naming_an_unknown_team_in_an_allowed_glob, "packs/typo/app/thing.rb"), + ( + add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob, + "frontend/typo/src/thing.js", + ), + ]; + for (add_package, file) in packages { + for command in ["validate", "gv"] { + run_on_modified_fixture(add_package, &[command, file])? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(file)); + } + } + Ok(()) +} + +// Read as a glob, `packs/[typo]` would match `packs/t` rather than the package's own directory. +fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/[typo]/app"))?; + fs::write(project_root.join("packs/[typo]/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/[typo]/app/thing.rb"), "puts 'typo'\n")?; + fs::create_dir_all(project_root.join("packs/t"))?; + fs::write(project_root.join("packs/t/stray.rb"), "puts 'no owner'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", + ) +} + +#[test] +fn test_validate_files_reports_a_bracketed_package_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_package_naming_an_unknown_team_in_an_allowed_glob, - &[command, "packs/typo/app/thing.rb"], + add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob, + &[command, "packs/[typo]/app/thing.rb"], )? .failure() .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("packs/typo/app/thing.rb")); + .stdout(predicate::str::contains("packs/[typo]/app/thing.rb")); + } + Ok(()) +} + +#[test] +fn test_validate_files_passes_an_allowed_file_outside_a_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob, + &[command, "packs/t/stray.rb", "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); } Ok(()) } From 2abedac23e871470b15da9d32bd5a9aea0ece869 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 00:44:11 -0700 Subject: [PATCH 17/38] Say the owner lookup sees only files the project reads The comments on the owner lookup behind allow_unowned_globs in `validate ` and `gv ` said that an allowed file is excused only if nothing names an owner for it, and that one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported. But the lookup is built from the project, which leaves out untracked files and anything under ignore_dirs. So an owner named only in such a file, as in a new file's own annotation before it is staged, or in an untracked `.codeowner` or package.yml, doesn't count, and the allowed file is excused, although without the key it is reported as unowned. A full `validate` doesn't read those files either, so it doesn't report such a file, and `validate ` keeps excusing it. Both comments now say that only files the project reads count, as in `validate`. Nothing else changes. --- src/ownership.rs | 4 +++- src/runner.rs | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index e8ff029..5a2ab63 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -110,7 +110,9 @@ impl Entry { } /// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation, -/// `.codeowner`, `package.yml` or `package.json` naming one still counts. Building one goes through every project file, so reuse it. +/// `.codeowner`, `package.yml` or `package.json` naming one still counts. It sees only files the project reads, so an annotated +/// file, `.codeowner` or package file names no owner here if it is untracked or under ignore_dirs, as in `validate`. Building one +/// goes through every project file, so reuse it. pub(crate) struct OwnerLookup { owner_matchers: Vec, unknown_team_package_roots: Vec, diff --git a/src/runner.rs b/src/runner.rs index 1c0be7d..ea7b7a4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -176,8 +176,10 @@ impl Runner { Ok(Some(_)) => {} // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without - // the key. The lookup matches plain project-relative paths only, so a path with a `..`, or an absolute one - // not under the project root, is never excused. + // the key. Only files the project reads count, though, so an annotated file, `.codeowner` or package file + // names no owner here if it is untracked or under ignore_dirs, as in `validate`. The lookup matches plain + // project-relative paths only, so a path with a `..`, or an absolute one not under the project root, is + // never excused. Ok(None) if plain_relative(&relative_path).is_some_and(|path| { matches_globs(&path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&path) From a28ea876bc9912556b0a0387ed1074bc18aefe1b Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 00:54:30 -0700 Subject: [PATCH 18/38] Check allow_unowned_globs in parallel in validate A full `validate` or `gv` matched each file with no owner against allow_unowned_globs in a loop on one thread. Such a file can be tried against every glob in the list, so the time grows with the number of those files times the length of the list. That is small for a few globs, but the README's Getting Started example lists a file by path, and a list of files still to be given owners can run to thousands of paths. On main such a list goes in unowned_globs, which is matched in the parallel walker. The check now runs on rayon's thread pool, like the other per-file checks in the validator. rayon's collect keeps the order of the files, so it reports the same errors in the same order. On a synthetic project of 10,000 files with no owner, each allowed by its path, the best of 15 runs of `validate` took 1.0 s before and takes 0.45 s now. --- src/ownership/validator.rs | 40 ++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index e796572..d5e6341 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -7,6 +7,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use itertools::Itertools; +use rayon::prelude::IntoParallelIterator; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; use similar::{ChangeTag, TextDiff}; @@ -167,24 +168,29 @@ impl Validator { #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] fn validate_file_ownership(&self) -> Vec { - let mut validation_errors = Vec::new(); - - for (file, owners) in self.file_to_owners() { - let relative_path = self.project.relative_path(&file.path).to_owned(); - - if owners.is_empty() { - if !matches_globs(&relative_path, &self.project.allow_unowned_globs) { - validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + let project = &self.project; + + self.file_to_owners() + .into_par_iter() + .filter_map(|(file, owners)| { + let relative_path = project.relative_path(&file.path).to_owned(); + + if owners.is_empty() { + if matches_globs(&relative_path, &project.allow_unowned_globs) { + None + } else { + Some(Error::FileWithoutOwner { path: relative_path }) + } + } else if owners.len() > 1 { + Some(Error::FileWithMultipleOwners { + path: relative_path, + owners, + }) + } else { + None } - } else if owners.len() > 1 { - validation_errors.push(Error::FileWithMultipleOwners { - path: relative_path, - owners, - }) - } - } - - validation_errors + }) + .collect() } #[instrument(name = "validate_codeowners_file", level = "debug", skip_all)] From 2fb71cd4106f92ef33ebdf2cf117706bdb3357ca Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:02:56 -0700 Subject: [PATCH 19/38] Share fixture setup between the allow_unowned_globs tests The two absolute-path tests copied the fixture, staged it and built the codeowners command line themselves, as run_on_modified_fixture does, because that helper takes its arguments before the temp root that an absolute path is built from exists. The file also named its fixture two ways, as a path for setup_fixture_repo and as a bare name for run_codeowners. The fixture is now named once, and run_on_modified_fixture is split into a builder that copies, modifies and stages the fixture and a runner for the command. validate_absolute_path joins them to validate a file by its canonical absolute path, so each absolute-path test is now a single call. The assertions are unchanged, and no test is added or removed. --- tests/allow_unowned_globs_test.rs | 87 ++++++++++++++----------------- 1 file changed, 38 insertions(+), 49 deletions(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 8b7b558..9153540 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -1,31 +1,32 @@ -use assert_cmd::prelude::*; +use assert_cmd::{assert::Assert, prelude::*}; use indoc::indoc; use predicates::prelude::*; -use std::{error::Error, fs, path::Path, process::Command}; +use std::{error::Error, ffi::OsStr, fs, path::Path, process::Command}; +use tempfile::TempDir; mod common; use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; -const FIXTURE: &str = "tests/fixtures/allow_unowned_globs"; +const FIXTURE: &str = "allow_unowned_globs"; type FixtureChange = fn(&Path) -> std::io::Result<()>; #[test] fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { - run_codeowners("allow_unowned_globs", &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; Ok(()) } #[test] fn test_generate_and_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { - run_codeowners("allow_unowned_globs", &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; + run_codeowners(FIXTURE, &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; Ok(()) } #[test] fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { run_codeowners( - "allow_unowned_globs", + FIXTURE, &["validate", "app/deprecated/old.rb"], true, OutputStream::Stdout, @@ -36,19 +37,7 @@ fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result #[test] fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Result<(), Box> { - let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); - let project_root = temp_dir.path(); - git_add_all_files(project_root); - - let file_absolute_path = project_root.join("app/deprecated/old.rb").canonicalize()?; - - Command::cargo_bin("codeowners")? - .arg("--project-root") - .arg(project_root) - .arg("--no-cache") - .arg("validate") - .arg(file_absolute_path) - .assert() + validate_absolute_path(|_| Ok(()), "app/deprecated/old.rb")? .success() .stdout(predicate::eq("")); Ok(()) @@ -57,7 +46,7 @@ fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Resul #[test] fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box> { run_codeowners( - "allow_unowned_globs", + FIXTURE, &["for-file", "app/deprecated/annotated_old.rb"], true, OutputStream::Stdout, @@ -75,7 +64,7 @@ fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box Result<(), Box> { run_codeowners( - "allow_unowned_globs", + FIXTURE, &["for-file", "app/deprecated/old.rb"], true, OutputStream::Stdout, @@ -84,21 +73,31 @@ fn test_unowned_file_in_an_allowed_glob_is_reported_as_unowned() -> Result<(), B Ok(()) } -fn run_on_modified_fixture( - modify: impl FnOnce(&Path) -> std::io::Result<()>, - args: &[&str], -) -> Result> { - let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); - let project_root = temp_dir.path(); - modify(project_root)?; - git_add_all_files(project_root); - let assert = Command::cargo_bin("codeowners")? +fn modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>) -> Result> { + let temp_dir = setup_fixture_repo(&Path::new("tests/fixtures").join(FIXTURE)); + modify(temp_dir.path())?; + git_add_all_files(temp_dir.path()); + Ok(temp_dir) +} + +fn codeowners(project_root: &Path, args: &[impl AsRef]) -> Result> { + Ok(Command::cargo_bin("codeowners")? .arg("--project-root") .arg(project_root) .arg("--no-cache") .args(args) - .assert(); - Ok(assert) + .assert()) +} + +fn run_on_modified_fixture(modify: impl FnOnce(&Path) -> std::io::Result<()>, args: &[&str]) -> Result> { + let temp_dir = modified_fixture(modify)?; + codeowners(temp_dir.path(), args) +} + +fn validate_absolute_path(modify: impl FnOnce(&Path) -> std::io::Result<()>, file: &str) -> Result> { + let temp_dir = modified_fixture(modify)?; + let file_absolute_path = temp_dir.path().join(file).canonicalize()?; + codeowners(temp_dir.path(), &[OsStr::new("validate"), file_absolute_path.as_os_str()]) } fn add_unowned_file_outside_allowed_globs(project_root: &Path) -> std::io::Result<()> { @@ -175,23 +174,13 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs() -> Result<(), Bo #[test] fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_an_absolute_path() -> Result<(), Box> { - let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); - let project_root = temp_dir.path(); - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; - git_add_all_files(project_root); - - let file_absolute_path = project_root.join("app/deprecated/typo.rb").canonicalize()?; - - Command::cargo_bin("codeowners")? - .arg("--project-root") - .arg(project_root) - .arg("--no-cache") - .arg("validate") - .arg(file_absolute_path) - .assert() - .failure() - .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("app/deprecated/typo.rb")); + validate_absolute_path( + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + "app/deprecated/typo.rb", + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/typo.rb")); Ok(()) } From f7d1d52ecec09a410bbfcbde062bdeee12fcd598 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:12:30 -0700 Subject: [PATCH 20/38] Name the allow_unowned_globs tests after what they check Six tests of `validate ` were named as if they reported an invalid team or a package naming one, but a list of files is never checked for invalid teams. Each asserts that an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported under "Unowned files detected:". Beside test_validate_reports_invalid_teams_in_allowed_globs, which does check a full `validate` for invalid teams, the near-identical names misled. They are now named after the allowed file they report. The two `for-file` tests can't be broken by anything allow_unowned_globs changes, since `for-file` never reads the key. They are kept, now named after `for-file`, and a comment says they pin that it reports an allowed file's owner as it would without the key. Another comment says the test that a full `validate` passes relies on the fixture's CODEOWNERS listing app/deprecated/annotated_old.rb, so it also fails, with CODEOWNERS out of date, if annotations in allowed globs stop counting. Only test names and comments change. --- tests/allow_unowned_globs_test.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 9153540..3352f59 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -13,6 +13,8 @@ type FixtureChange = fn(&Path) -> std::io::Result<()>; #[test] fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { + // The fixture's CODEOWNERS lists `app/deprecated/annotated_old.rb`, so if annotations in allowed globs stop + // counting, this also fails, with CODEOWNERS out of date. run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; Ok(()) } @@ -43,8 +45,9 @@ fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Resul Ok(()) } +// The `for-file` tests pin that it reports an allowed file's owner as it would without allow_unowned_globs. #[test] -fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box> { +fn test_for_file_reports_annotations_in_allowed_globs() -> Result<(), Box> { run_codeowners( FIXTURE, &["for-file", "app/deprecated/annotated_old.rb"], @@ -62,7 +65,7 @@ fn test_annotations_still_assign_owners_in_allowed_globs() -> Result<(), Box Result<(), Box> { +fn test_for_file_reports_an_unowned_file_in_an_allowed_glob_as_unowned() -> Result<(), Box> { run_codeowners( FIXTURE, &["for-file", "app/deprecated/old.rb"], @@ -157,7 +160,7 @@ fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team() -> Result<(), Box> { // A list of files isn't checked for invalid teams. CODEOWNERS has no line for this file, and its annotation keeps // allow_unowned_globs from excusing it, so it's reported as unowned. for command in ["validate", "gv"] { @@ -173,7 +176,7 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs() -> Result<(), Bo } #[test] -fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_an_absolute_path() -> Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team_by_absolute_path() -> Result<(), Box> { validate_absolute_path( add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, "app/deprecated/typo.rb", @@ -185,7 +188,7 @@ fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_an_absolute_pa } #[test] -fn test_validate_files_reports_invalid_teams_in_allowed_globs_for_a_path_with_dot_dot() -> Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() -> Result<(), Box> { // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. for command in ["validate", "gv"] { run_on_modified_fixture( @@ -252,7 +255,7 @@ fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_ro } #[test] -fn test_validate_files_reports_a_package_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_team() -> Result<(), Box> { let packages: [(FixtureChange, &str); 2] = [ (add_a_package_naming_an_unknown_team_in_an_allowed_glob, "packs/typo/app/thing.rb"), ( @@ -285,7 +288,7 @@ fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_roo } #[test] -fn test_validate_files_reports_a_bracketed_package_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_in_a_bracketed_package_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob, @@ -316,7 +319,7 @@ fn add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root: } #[test] -fn test_validate_files_reports_invalid_directory_teams_in_allowed_globs() -> Result<(), Box> { +fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, From ce601e20de040714ad758447f1fae2931f87b15c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:27:59 -0700 Subject: [PATCH 21/38] Test more owner sources for allowed files Five changes to the code passed every test. `validate ` could report an allowed file with an owner even when CODEOWNERS lists it, which would fail a pre-commit hook on a commit that edits an annotated file in an allowed glob. The owner lookup could leave out team globs, or packages naming a team that exists, because the only test of an allowed file that CODEOWNERS doesn't list yet gave it an annotation. And no test ran a full `validate` with a package.yml or `.codeowner` naming an unknown team in an allowed glob, so it could stop reporting them there. New tests pass the annotated file and an unowned one to `validate` and `gv`, and run that full `validate` with each. The latter checks for each message, not just the exit code, since the `.codeowner` also gives annotated_old.rb a second owner. It also checks that the package's file, which has no owner, isn't reported as missing ownership. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a team glob and one from a package, not just an annotation. Only tests change. --- tests/allow_unowned_globs_test.rs | 76 ++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 3352f59..f0377dc 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -45,6 +45,22 @@ fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Resul Ok(()) } +#[test] +fn test_validate_files_passes_for_an_annotated_and_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { + // What a pre-commit hook checks when a commit edits both. An allowed file that names an owner is reported only if + // CODEOWNERS doesn't list it, and CODEOWNERS lists the annotated one. + for command in ["validate", "gv"] { + run_codeowners( + FIXTURE, + &[command, "app/deprecated/annotated_old.rb", "app/deprecated/old.rb"], + true, + OutputStream::Stdout, + predicate::eq(""), + )?; + } + Ok(()) +} + // The `for-file` tests pin that it reports an allowed file's owner as it would without allow_unowned_globs. #[test] fn test_for_file_reports_annotations_in_allowed_globs() -> Result<(), Box> { @@ -332,6 +348,27 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno Ok(()) } +#[test] +fn test_validate_reports_a_package_or_codeowner_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { + // The package mappers drop the package, leaving its file with no owner, but the file is allowed. The `.codeowner` + // gives annotated_old.rb a second owner, so `validate` would fail even if the `.codeowner` itself went unreported. + let files: [(FixtureChange, &str); 2] = [ + (add_a_package_naming_an_unknown_team_in_an_allowed_glob, "packs/typo/package.yml"), + ( + add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + "app/deprecated/.codeowner", + ), + ]; + for (add_file, file) in files { + let invalid_team = format!("- {file} is referencing an invalid team - 'Typo'"); + run_on_modified_fixture(add_file, &["validate"])? + .failure() + .stdout(predicate::str::contains(invalid_team)) + .stdout(predicate::str::contains("Some files are missing ownership").not()); + } + Ok(()) +} + fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { fs::write( project_root.join("app/deprecated/old.rb"), @@ -339,15 +376,40 @@ fn annotate_the_unowned_file_without_regenerating_codeowners(project_root: &Path ) } +fn give_the_unowned_file_a_team_glob_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("config/teams/foo.yml"), + "---\nname: Foo\ngithub:\n team: '@FooTeam'\nowned_globs:\n - app/deprecated/old.rb\n", + ) +} + +fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/foo/app"))?; + fs::write(project_root.join("packs/foo/package.yml"), "owner: Foo\n")?; + fs::write(project_root.join("packs/foo/app/thing.rb"), "puts 'foo'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/foo/**/*\"\n", + ) +} + #[test] fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { - run_on_modified_fixture( - annotate_the_unowned_file_without_regenerating_codeowners, - &["validate", "app/deprecated/old.rb"], - )? - .failure() - .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("app/deprecated/old.rb")); + // `validate`, not `gv`, which would add each file to CODEOWNERS. + let owners: [(FixtureChange, &str); 3] = [ + (annotate_the_unowned_file_without_regenerating_codeowners, "app/deprecated/old.rb"), + ( + give_the_unowned_file_a_team_glob_without_regenerating_codeowners, + "app/deprecated/old.rb", + ), + (add_a_package_without_regenerating_codeowners, "packs/foo/app/thing.rb"), + ]; + for (add_owner, file) in owners { + run_on_modified_fixture(add_owner, &["validate", file])? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(file)); + } Ok(()) } From 0fef8a1de5486036a81bff23db0331b38ce4722f Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:34:20 -0700 Subject: [PATCH 22/38] Keep unit tests next to what they test When matches_globs moved from project_builder to path_utils, its test stayed behind, as did test_glob_match, which tests fast_glob itself and needed a new glob_match import in project_builder's tests. The only check of the allow_unowned_globs default went in the test of the executable_name default, so a change to it would fail a test named for another key. Both tests and their OWNED_GLOB now sit with the other matches_globs test in path_utils, calling fast_glob::glob_match by its full path as the code there does, and the import is gone. The default is checked in test_parse_config with most of the others, between executable_name and yaml_max_nodes as in the struct, and the executable_name test is back to what main has. Only tests change, and none is added or removed. --- src/config.rs | 2 +- src/path_utils.rs | 12 ++++++++++++ src/project_builder.rs | 13 ------------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/src/config.rs b/src/config.rs index 9a09349..a8132e0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -176,6 +176,7 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); + assert!(config.allow_unowned_globs.is_empty()); assert_eq!(config.yaml_max_nodes, 20_000); Ok(()) } @@ -254,7 +255,6 @@ mod tests { fs::write(&config_path, config_str)?; let config = Config::load_from_path(&config_path)?; assert_eq!(config.executable_name, "codeowners generate"); - assert!(config.allow_unowned_globs.is_empty()); Ok(()) } diff --git a/src/path_utils.rs b/src/path_utils.rs index 4feb4fd..dc98e00 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -36,6 +36,18 @@ pub(crate) fn matches_globs(path: &Path, globs: &[String]) -> bool { mod tests { use super::*; + const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; + + #[test] + fn test_matches_globs() { + assert!(matches_globs(Path::new("script/.eslintrc.js"), &[OWNED_GLOB.to_string()])); + } + + #[test] + fn test_glob_match() { + assert!(fast_glob::glob_match(OWNED_GLOB, "script/.eslintrc.js")); + } + #[test] fn matches_globs_double_star_matches_root_level_and_nested_files() { let globs = vec!["**/*".to_string()]; diff --git a/src/project_builder.rs b/src/project_builder.rs index 55104a8..c0a3ce2 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -368,19 +368,6 @@ mod tests { use indoc::indoc; use super::*; - use fast_glob::glob_match; - - const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; - - #[test] - fn test_matches_globs() { - assert!(matches_globs(Path::new("script/.eslintrc.js"), &[OWNED_GLOB.to_string()])); - } - - #[test] - fn test_glob_match() { - assert!(glob_match(OWNED_GLOB, "script/.eslintrc.js")); - } #[test] fn test_ruby_package_owner_top_level() { From f314fd8812bc267c080805a4755854815d1d1211 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:37:53 -0700 Subject: [PATCH 23/38] List allow_unowned_globs next to unowned_globs in the README The README's list of configuration keys put allow_unowned_globs after executable_name, with four other keys between it and unowned_globs, though the bullet is written in terms of unowned_globs. It says how the two keys differ, that unowned_globs takes precedence, and when to switch from `unowned_globs: ['**/*']` to the new key, so a reader had to look back up the list to follow it. The bullet now comes directly after unowned_globs. Its text and the rest of the README are unchanged. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5c371a9..a642742 100644 --- a/README.md +++ b/README.md @@ -213,11 +213,11 @@ codeowners gv --no-cache - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) - `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. Use this for third-party or generated code. +- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, file annotations in them still count, and every other check still runs, so a file there can't have more than one owner or name an invalid team. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work. Those files then get the other checks too, which may report problems that were hidden before. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, file annotations in them still count, and every other check still runs, so a file there can't have more than one owner or name an invalid team. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work. Those files then get the other checks too, which may report problems that were hidden before. - `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: From 9b28647291b6530dcc17f89d923d9007fc917203 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 01:51:51 -0700 Subject: [PATCH 24/38] Explain allow_unowned_globs for file lists, migration and old versions The README said a file in allow_unowned_globs can't have more than one owner or name an invalid team, since every other check still runs. That holds for a full `validate` or `gv`, but the README never mentioned what they do given a list of files, as from a git hook. They look each file up in CODEOWNERS and never check for more than one owner, and pass an allowed file with no line only if nothing names an owner for it. So one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Unowned files detected". The advice to replace `unowned_globs: ['**/*']` with the new key left out that CODEOWNERS then gains a line for each annotated file, so until `codeowners gv` runs, `validate` reports it out of date and `validate ` reports those files as unowned. It also left out that the files are then read and checked, which takes longer. The bullet now covers all of this, says to keep third-party and generated code in unowned_globs, and says the key was added in 0.6.0, since older versions ignore it without a warning and still report these files. Only the README changes. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index a642742..bd6ab9b 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ codeowners gv --no-cache - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) - `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. Use this for third-party or generated code. -- `allow_unowned_globs` (default: `[]`): Files in `owned_globs` that match these globs don't need an owner, so leaving them unowned isn't a validation error. Unlike `unowned_globs`, file annotations in them still count, and every other check still runs, so a file there can't have more than one owner or name an invalid team. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work. Those files then get the other checks too, which may report problems that were hidden before. +- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Unowned files detected". Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) From 63251195b399efe62a9bf838286344d02e76f309 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 02:13:59 -0700 Subject: [PATCH 25/38] Match a .codeowner naming an unknown team by its directory too The owner lookup behind allow_unowned_globs in `validate ` and `gv ` matched a `.codeowner` only by the glob the directory mapper builds from its directory. For one at the project root that glob is `/**/**`, which matches nothing, and the mapper escapes brackets but not braces, so `app/deprecated/{legacy}/**/**` matches files in `app/deprecated/legacy` but not in `app/deprecated/{legacy}`. An allowed file under such a `.codeowner` naming a team that doesn't exist therefore passed, although a full `validate` reports the `.codeowner` and, without the key, the file is reported as unowned. The lookup now keeps the directory of each `.codeowner` naming a team that doesn't exist, as it already did for packages, and finds an owner for any path under one. A file under such a `.codeowner` is reported as unowned again, as without the key. Only the nearest `.codeowner` gives a file its owner in `validate`, but the lookup asks only whether anything names an owner. The directory mapper is unchanged, so CODEOWNERS and a full `validate` are too. A test of a package in a directory with braces is added as well. It passed before, but without it no test fails if packages are matched by globs like the directory mapper's. --- src/ownership.rs | 40 ++++++++++------- tests/allow_unowned_globs_test.rs | 75 +++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 5a2ab63..2cf5714 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -109,13 +109,13 @@ impl Entry { } } -/// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation, -/// `.codeowner`, `package.yml` or `package.json` naming one still counts. It sees only files the project reads, so an annotated -/// file, `.codeowner` or package file names no owner here if it is untracked or under ignore_dirs, as in `validate`. Building one -/// goes through every project file, so reuse it. +/// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation naming one +/// still counts for its file, and a `.codeowner`, `package.yml` or `package.json` naming one for every path under its directory. +/// It sees only files the project reads, so an annotated file, `.codeowner` or package file names no owner here if it is +/// untracked or under ignore_dirs, as in `validate`. Building one goes through every project file, so reuse it. pub(crate) struct OwnerLookup { owner_matchers: Vec, - unknown_team_package_roots: Vec, + unknown_team_roots: Vec, } impl OwnerLookup { @@ -123,8 +123,7 @@ impl OwnerLookup { let file_owner_finder = FileOwnerFinder { owner_matchers: &self.owner_matchers, }; - self.unknown_team_package_roots.iter().any(|root| relative_path.starts_with(root)) - || !file_owner_finder.find(relative_path).is_empty() + self.unknown_team_roots.iter().any(|root| relative_path.starts_with(root)) || !file_owner_finder.find(relative_path).is_empty() } } @@ -176,18 +175,25 @@ impl Ownership { } pub(crate) fn owner_lookup(&self) -> OwnerLookup { - // The package mappers drop a package whose owner isn't a team, and `validate` reports its package.yml or package.json - // instead. A list of files isn't checked for that, so here the package still names an owner for its files. Its directory - // is matched as a path prefix, not a glob, so brackets in it are literal and a package at the project root covers every file. + // `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 or in a directory whose path + // has braces, so the directory is matched as a path prefix: brackets and braces are literal and the root covers every file. + 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()).collect(), - unknown_team_package_roots: self - .project - .packages - .iter() - .filter(|package| !self.project.teams_by_name.contains_key(&package.owner)) - .filter_map(|package| package.package_root().map(Path::to_path_buf)) - .collect(), + unknown_team_roots: package_roots.chain(directory_roots).map(Path::to_path_buf).collect(), } } diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index f0377dc..9a9ccb1 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -330,6 +330,31 @@ fn test_validate_files_passes_an_allowed_file_outside_a_package_naming_an_unknow Ok(()) } +// Read as a glob, `packs/{typo}` would match `packs/typo` rather than the package's own directory. +fn add_a_braced_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("packs/{typo}/app"))?; + fs::write(project_root.join("packs/{typo}/package.yml"), "owner: Typo\n")?; + fs::write(project_root.join("packs/{typo}/app/thing.rb"), "puts 'typo'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", + ) +} + +#[test] +fn test_validate_files_reports_an_allowed_file_in_a_braced_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_braced_package_naming_an_unknown_team_in_an_allowed_glob, + &[command, "packs/{typo}/app/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("packs/{typo}/app/thing.rb")); + } + Ok(()) +} + fn add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("app/deprecated/.codeowner"), "Typo\n") } @@ -348,6 +373,56 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno Ok(()) } +// A glob built from the project root, `/**/**`, would match no project-relative path. +fn allow_every_file_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join(".codeowner"), "Typo\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + ) +} + +#[test] +fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + allow_every_file_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root, + &[command, "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/old.rb")); + } + Ok(()) +} + +// Read as a glob, `app/deprecated/{legacy}` would match `app/deprecated/legacy` rather than its own directory. +fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_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")?; + fs::write(project_root.join("app/deprecated/{legacy}/thing.rb"), "puts 'typo'\n") +} + +#[test] +fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + &[command, "app/deprecated/{legacy}/thing.rb"], + )? + .failure() + .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); + run_on_modified_fixture( + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + &[command, "app/deprecated/old.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + #[test] fn test_validate_reports_a_package_or_codeowner_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { // The package mappers drop the package, leaving its file with no owner, but the file is allowed. The `.codeowner` From 27c7579a048171e2bd53ee53bbd29f523b33e272 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 03:03:29 -0700 Subject: [PATCH 26/38] Report allowed files under their own heading in validate Given a list of files, `validate` and `gv` report an allowed file that CODEOWNERS has no line for only if something names an owner for it: a team that doesn't exist, or one CODEOWNERS doesn't list yet. Such a file was still listed under "Unowned files detected:", with no hint, although the config says it may go unowned. With the README's `['**/*']`, every failure for a list of files is of this kind, and in the code_ownership gem's pre-commit hook, which runs `gv` and so regenerates CODEOWNERS first, the cause is almost always a team that doesn't exist. These files now get a heading of their own. It says that they match allow_unowned_globs but name an owner that CODEOWNERS doesn't list, and suggests checking the team name or running the configured executable_name to update CODEOWNERS, as the stale-CODEOWNERS message does. Files outside the globs, and a path with a `..` or an absolute one not under the project root, are still listed under "Unowned files detected:", and both headings can appear in one run. The exit status is unchanged, and the gem only joins these lines into the error it raises. The README names the new heading, and new tests pass both kinds of file at once and check that the hint names the configured executable. --- README.md | 2 +- src/runner.rs | 37 ++++++++++------ tests/allow_unowned_globs_test.rs | 72 +++++++++++++++++++++++++------ 3 files changed, 84 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index bd6ab9b..6fd1f56 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ codeowners gv --no-cache - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) - `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. Use this for third-party or generated code. -- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Unowned files detected". Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. +- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list". Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) diff --git a/src/runner.rs b/src/runner.rs index ea7b7a4..5cb5a91 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -145,6 +145,7 @@ impl Runner { fn validate_files(&self, file_paths: Vec) -> RunResult { let mut unowned_files = Vec::new(); + let mut allowed_files_with_unlisted_owners = Vec::new(); let mut io_errors = Vec::new(); // Filter files based on owned_globs and unowned_globs configuration @@ -175,26 +176,36 @@ impl Runner { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or - // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still unowned, as without - // the key. Only files the project reads count, though, so an annotated file, `.codeowner` or package file - // names no owner here if it is untracked or under ignore_dirs, as in `validate`. The lookup matches plain - // project-relative paths only, so a path with a `..`, or an absolute one not under the project root, is - // never excused. - Ok(None) - if plain_relative(&relative_path).is_some_and(|path| { - matches_globs(&path, &self.config.allow_unowned_globs) && !owner_lookup.has_owner(&path) - }) => {} - Ok(None) => unowned_files.push(file_path), + // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still reported, but not + // as unowned, which the key allows. Only files the project reads count, though, so an annotated file, + // `.codeowner` or package file names no owner here if it is untracked or under ignore_dirs, as in + // `validate`. The lookup matches plain project-relative paths only, so a path with a `..`, or an absolute + // one not under the project root, is never excused and is reported as unowned, as without the key. + Ok(None) => match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.allow_unowned_globs)) { + Some(path) if owner_lookup.has_owner(&path) => allowed_files_with_unlisted_owners.push(file_path), + Some(_) => {} + None => unowned_files.push(file_path), + }, Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } } }); + let mut validation_errors = Vec::new(); if !unowned_files.is_empty() { - let validation_errors = std::iter::once("Unowned files detected:".to_string()) - .chain(unowned_files.into_iter().map(|file| format!(" {}", file))) - .collect(); + validation_errors.push("Unowned files detected:".to_string()); + validation_errors.extend(unowned_files.into_iter().map(|file| format!(" {}", file))); + } + if !allowed_files_with_unlisted_owners.is_empty() { + validation_errors.push(format!( + "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name, or run `{}` to update the CODEOWNERS file:", + self.config.executable_name + )); + validation_errors.extend(allowed_files_with_unlisted_owners.into_iter().map(|file| format!(" {}", file))); + } + if !validation_errors.is_empty() { return RunResult { validation_errors, io_errors, diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 9a9ccb1..95fc707 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -9,6 +9,10 @@ use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo const FIXTURE: &str = "allow_unowned_globs"; +// As printed with the default executable_name. +const UNLISTED_OWNER_HEADING: &str = "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name, or run `codeowners generate` to update the CODEOWNERS file:"; + type FixtureChange = fn(&Path) -> std::io::Result<()>; #[test] @@ -178,15 +182,36 @@ fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box Result<(), Box> { // A list of files isn't checked for invalid teams. CODEOWNERS has no line for this file, and its annotation keeps - // allow_unowned_globs from excusing it, so it's reported as unowned. + // allow_unowned_globs from excusing it, so it's reported, but not as unowned. for command in ["validate", "gv"] { run_on_modified_fixture( add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, &[command, "app/deprecated/typo.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) - .stdout(predicate::str::contains("app/deprecated/typo.rb")); + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains("app/deprecated/typo.rb")) + .stdout(predicate::str::contains("Unowned files detected:").not()); + } + Ok(()) +} + +fn add_an_unowned_file_and_an_allowed_one_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_unowned_file_outside_allowed_globs(project_root)?; + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root) +} + +#[test] +fn test_validate_files_reports_allowed_and_unowned_files_under_separate_headings() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_unowned_file_and_an_allowed_one_naming_an_unknown_team, + &[command, "app/deprecated/typo.rb", "app/stray.rb"], + )? + .failure() + .stdout(predicate::eq(format!( + "Unowned files detected:\n app/stray.rb\n{UNLISTED_OWNER_HEADING}\n app/deprecated/typo.rb\n" + ))); } Ok(()) } @@ -198,14 +223,15 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown "app/deprecated/typo.rb", )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("app/deprecated/typo.rb")); Ok(()) } #[test] fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() -> Result<(), Box> { - // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. + // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. It's reported + // as unowned, as without the key. for command in ["validate", "gv"] { run_on_modified_fixture( add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, @@ -235,7 +261,7 @@ fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), &[command, "./app/deprecated/typo.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("./app/deprecated/typo.rb")); run_on_modified_fixture( allow_every_file_and_annotate_one_with_an_unknown_team, @@ -283,7 +309,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_te for command in ["validate", "gv"] { run_on_modified_fixture(add_package, &[command, file])? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains(file)); } } @@ -311,7 +337,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_bracketed_package_naming_an_ &[command, "packs/[typo]/app/thing.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("packs/[typo]/app/thing.rb")); } Ok(()) @@ -349,7 +375,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_braced_package_naming_an_unk &[command, "packs/{typo}/app/thing.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("packs/{typo}/app/thing.rb")); } Ok(()) @@ -367,7 +393,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno &[command, "app/deprecated/old.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("app/deprecated/old.rb")); } Ok(()) @@ -390,7 +416,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_nam &[command, "app/deprecated/old.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("app/deprecated/old.rb")); } Ok(()) @@ -411,7 +437,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_a &[command, "app/deprecated/{legacy}/thing.rb"], )? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); run_on_modified_fixture( add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, @@ -482,12 +508,32 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu for (add_owner, file) in owners { run_on_modified_fixture(add_owner, &["validate", file])? .failure() - .stdout(predicate::str::contains("Unowned files detected:")) + .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains(file)); } Ok(()) } +fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + annotate_the_unowned_file_without_regenerating_codeowners(project_root)?; + let config_path = project_root.join("config/code_ownership.yml"); + let config = fs::read_to_string(&config_path)?; + fs::write(config_path, config + "executable_name: bin/codeownership validate\n") +} + +#[test] +fn test_validate_files_suggests_the_configured_executable_for_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { + run_on_modified_fixture( + annotate_the_unowned_file_and_name_an_executable, + &["validate", "app/deprecated/old.rb"], + )? + .failure() + .stdout(predicate::str::contains( + "run `bin/codeownership validate` to update the CODEOWNERS file:\n app/deprecated/old.rb\n", + )); + Ok(()) +} + fn put_the_allowed_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; fs::write( From b4188fb5c4dc7c751542f2dad1c7897502cae2c3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 03:05:05 -0700 Subject: [PATCH 27/38] Say the config file is held to the default node limit The README said yaml_max_nodes doesn't apply to the config file itself, which can read as if the config had no limit. It is always parsed with the default of 20,000 nodes, and nothing raises that, so a config whose lists add up to about 20,000 entries fails with a `budget breached` error and, unlike a team file, no hint. A long literal allow_unowned_globs list is the likeliest way to get there, and 0.5.0, which predates #142's limits, read such a config. The sentence now gives the limit. Only the README changes. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 6fd1f56..e3f2163 100644 --- a/README.md +++ b/README.md @@ -218,7 +218,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself, which is always held to the default of 20,000 nodes, about one per list entry. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: From f40c5701a8c956272196cec38a309dfbf51d2e22 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 03:05:38 -0700 Subject: [PATCH 28/38] Say that unowned_globs still reads team files The README's unowned_globs bullet says files matched there are left out of the project, then names `.codeowner`, `package.yml` and `package.json` as the files still read, which reads as the whole list. A team file is another: the project builder matches team_file_glob before owned_globs and unowned_globs, so a team file in unowned_globs still defines its team and keeps its own line in CODEOWNERS. The bullet now says so in a sentence of its own, since the invalid-team check the sentence before it mentions doesn't apply to team files. Only the README changes. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e3f2163..4a2d803 100644 --- a/README.md +++ b/README.md @@ -212,7 +212,7 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. Use this for third-party or generated code. +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. So is a team file, which still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code. - `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list". Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) From 63e878adc3850bdd509a08d6ac3049247823c958 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 04:41:42 -0700 Subject: [PATCH 29/38] Match a .codeowner naming an unknown team only by its directory The owner lookup behind allow_unowned_globs in `validate ` and `gv ` matches the directory of a `.codeowner` naming a team that doesn't exist as a path prefix, but it still took the glob the directory mapper builds from that directory as well. The mapper escapes brackets but not braces, so the glob it builds for one in `app/deprecated/{legacy}` matches files in `app/deprecated/legacy` instead. An allowed file there, which nothing names an owner for, was therefore listed under the heading for files that name an owner CODEOWNERS doesn't list, and the run failed instead of passing. The lookup now leaves out the glob for such a `.codeowner`, as the prefix covers just the files under its directory, so that file passes and a file under the `.codeowner` is still reported. The glob for a `.codeowner` naming a team that exists is kept, since it names an owner for files that CODEOWNERS may not list yet. The directory mapper is unchanged, so CODEOWNERS and a full `validate` are too. The braced test now also passes a file in `app/deprecated/legacy` and fails without the change. The test of an allowed file that CODEOWNERS doesn't list yet covers an owner from a `.codeowner` too, since without it no test fails if the lookup drops every `.codeowner` glob. --- src/ownership.rs | 19 ++++++++++++++++--- tests/allow_unowned_globs_test.rs | 13 ++++++++++--- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index 2cf5714..ad6a3b9 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -177,8 +177,9 @@ 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 or in a directory whose path - // has braces, so the directory is matched as a path prefix: brackets and braces are literal and the root covers every file. + // 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. let package_roots = self .project .packages @@ -192,7 +193,19 @@ impl Ownership { .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()).collect(), + 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(), } } diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 95fc707..c8ce9cb 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -426,7 +426,9 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_nam fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_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")?; - fs::write(project_root.join("app/deprecated/{legacy}/thing.rb"), "puts 'typo'\n") + fs::write(project_root.join("app/deprecated/{legacy}/thing.rb"), "puts 'typo'\n")?; + fs::create_dir_all(project_root.join("app/deprecated/legacy"))?; + fs::write(project_root.join("app/deprecated/legacy/thing.rb"), "puts 'no owner'\n") } #[test] @@ -441,7 +443,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_a .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); run_on_modified_fixture( add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, - &[command, "app/deprecated/old.rb"], + &[command, "app/deprecated/legacy/thing.rb", "app/deprecated/old.rb"], )? .success() .stdout(predicate::eq("")); @@ -484,6 +486,10 @@ fn give_the_unowned_file_a_team_glob_without_regenerating_codeowners(project_roo ) } +fn add_a_codeowner_file_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::write(project_root.join("app/deprecated/.codeowner"), "Foo\n") +} + fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/foo/app"))?; fs::write(project_root.join("packs/foo/package.yml"), "owner: Foo\n")?; @@ -497,12 +503,13 @@ fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io #[test] fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { // `validate`, not `gv`, which would add each file to CODEOWNERS. - let owners: [(FixtureChange, &str); 3] = [ + let owners: [(FixtureChange, &str); 4] = [ (annotate_the_unowned_file_without_regenerating_codeowners, "app/deprecated/old.rb"), ( give_the_unowned_file_a_team_glob_without_regenerating_codeowners, "app/deprecated/old.rb", ), + (add_a_codeowner_file_without_regenerating_codeowners, "app/deprecated/old.rb"), (add_a_package_without_regenerating_codeowners, "packs/foo/app/thing.rb"), ]; for (add_owner, file) in owners { From be2b436d2bedd68bb4116cf16cab8d793675ccf9 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 04:55:21 -0700 Subject: [PATCH 30/38] Look up a non-plain allowed path in CODEOWNERS by its plain form Given a list of files, `validate` and `gv` look each one up in CODEOWNERS by the path as given, but match a file with no line against allow_unowned_globs and the owner lookup by its plain project-relative path. So an allowed file that CODEOWNERS lists, such as an annotated one, missed its line when passed as `./app/x.rb`, `app//x.rb` or `app/./x.rb`, the owner lookup found its owner, and the file was listed under the heading for files that name an owner CODEOWNERS doesn't list. That is false, and neither checking the team name nor regenerating CODEOWNERS made it pass. The `./` form passed before allowed files were matched by their plain path. Such a file is now looked up in CODEOWNERS again by its plain path, and passes if that finds a line. The second lookup is made only when the plain path differs from the one given and the owner lookup has found an owner, so only a file that would otherwise be listed pays for it. A file outside allow_unowned_globs is still looked up only by the path as given, as on main, so nothing changes without the key. A new test passes the annotated file to `validate` and `gv` by each of those paths and fails without the change. The test of a `./` path now allows only `app/deprecated/**/*`, which matches the plain path but not the one given, so it also catches allow_unowned_globs being matched against the path as given, which no test did before. --- src/runner.rs | 12 +++++++++-- tests/allow_unowned_globs_test.rs | 36 ++++++++++++++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 5cb5a91..841ef76 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -170,6 +170,12 @@ impl Runner { // Built only if an allowed file needs it, and then once. let owner_lookup = LazyCell::new(|| self.ownership.owner_lookup()); + let listed_in_codeowners = |path: &Path| { + matches!( + team_for_file_from_codeowners(&self.run_config, &path.to_string_lossy()), + Ok(Some(_)) + ) + }; debug_span!("per_file_query").in_scope(|| { for (file_path, relative_path) in filtered_paths { @@ -182,8 +188,10 @@ impl Runner { // `validate`. The lookup matches plain project-relative paths only, so a path with a `..`, or an absolute // one not under the project root, is never excused and is reported as unowned, as without the key. Ok(None) => match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.allow_unowned_globs)) { - Some(path) if owner_lookup.has_owner(&path) => allowed_files_with_unlisted_owners.push(file_path), - Some(_) => {} + Some(path) if !owner_lookup.has_owner(&path) => {} + // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` made it miss the file's line. + Some(path) if path.as_os_str() != relative_path.as_os_str() && listed_in_codeowners(&path) => {} + Some(_) => allowed_files_with_unlisted_owners.push(file_path), None => unowned_files.push(file_path), }, Err(err) => io_errors.push(format!("{}: {}", file_path, err)), diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index c8ce9cb..bbd57b7 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -244,27 +244,28 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown Ok(()) } -fn allow_every_file_and_annotate_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { +fn own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; fs::write( project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n", ) } #[test] fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Box> { - // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`, so these reach the owner lookup. + // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`. The allowed glob doesn't, so these + // reach the owner lookup only if it's matched against the plain path. for command in ["validate", "gv"] { run_on_modified_fixture( - allow_every_file_and_annotate_one_with_an_unknown_team, + own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team, &[command, "./app/deprecated/typo.rb"], )? .failure() .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) .stdout(predicate::str::contains("./app/deprecated/typo.rb")); run_on_modified_fixture( - allow_every_file_and_annotate_one_with_an_unknown_team, + own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team, &[command, "./app/deprecated/old.rb"], )? .success() @@ -273,6 +274,31 @@ fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Ok(()) } +fn allow_every_file(project_root: &Path) -> std::io::Result<()> { + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", + ) +} + +#[test] +fn test_validate_files_looks_up_an_allowed_file_in_codeowners_by_its_plain_path() -> Result<(), Box> { + // Read as given, none of these paths matches annotated_old.rb's line in CODEOWNERS, and the owner lookup finds its + // annotation. + for command in ["validate", "gv"] { + for file in [ + "./app/deprecated/annotated_old.rb", + "app//deprecated/annotated_old.rb", + "app/./deprecated/annotated_old.rb", + ] { + run_on_modified_fixture(allow_every_file, &[command, file])? + .success() + .stdout(predicate::eq("")); + } + } + Ok(()) +} + fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; From 5c14495a224c918963cea0addb461c9be4a57a4f Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 05:08:09 -0700 Subject: [PATCH 31/38] Don't suggest updating CODEOWNERS that gv has just written Given a list of files, `gv` writes CODEOWNERS before it looks each one up, so an allowed file it lists under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list" is almost always there because its annotation, `.codeowner` or package names a team that doesn't exist. The heading still suggested running the configured executable_name to update CODEOWNERS, which can't help. The default, `codeowners generate`, writes the same file again, and the code_ownership gem names `bin/codeownership validate`, which by default runs `gv` again. For a file under an empty `.codeowner`, "check the team name" didn't say where to look either. Both commands now say to check the team name in each file's annotation, `.codeowner` or package, and only `validate` still suggests running the executable, as its CODEOWNERS may be out of date. The heading's first sentence, which the README quotes, and the exit status are unchanged. The tests now expect the heading each command prints, and a new one runs `gv` with the gem's executable_name and checks that it doesn't suggest updating CODEOWNERS. They fail without the change. --- src/runner.rs | 20 +++++++--- tests/allow_unowned_globs_test.rs | 63 +++++++++++++++++++++++-------- 2 files changed, 62 insertions(+), 21 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 841ef76..84887f8 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -126,7 +126,7 @@ impl Runner { if file_paths.is_empty() { self.validate_all() } else { - self.validate_files(file_paths) + self.validate_files(file_paths, false) } } @@ -143,7 +143,7 @@ impl Runner { } } - fn validate_files(&self, file_paths: Vec) -> RunResult { + fn validate_files(&self, file_paths: Vec, codeowners_generated: bool) -> RunResult { let mut unowned_files = Vec::new(); let mut allowed_files_with_unlisted_owners = Vec::new(); let mut io_errors = Vec::new(); @@ -205,10 +205,16 @@ impl Runner { validation_errors.extend(unowned_files.into_iter().map(|file| format!(" {}", file))); } if !allowed_files_with_unlisted_owners.is_empty() { + // `gv` has just written CODEOWNERS, so updating it again can't help, and only a team that doesn't exist is left out. + let or_update_codeowners = if codeowners_generated { + String::new() + } else { + format!(", or run `{}` to update the CODEOWNERS file", self.config.executable_name) + }; validation_errors.push(format!( "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ - Check the team name, or run `{}` to update the CODEOWNERS file:", - self.config.executable_name + Check the team name in each one's annotation, `.codeowner` or package{}:", + or_update_codeowners )); validation_errors.extend(allowed_files_with_unlisted_owners.into_iter().map(|file| format!(" {}", file))); } @@ -255,7 +261,11 @@ impl Runner { if run_result.has_errors() { return run_result; } - self.validate(file_paths) + if file_paths.is_empty() { + self.validate_all() + } else { + self.validate_files(file_paths, true) + } } fn git_stage(&self) { diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index bbd57b7..6ddc775 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -9,9 +9,17 @@ use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo const FIXTURE: &str = "allow_unowned_globs"; -// As printed with the default executable_name. -const UNLISTED_OWNER_HEADING: &str = "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ - Check the team name, or run `codeowners generate` to update the CODEOWNERS file:"; +// `validate` suggests running the default executable_name, but `gv` has just written CODEOWNERS, so it doesn't. +fn unlisted_owner_heading(command: &str) -> &'static str { + if command == "gv" { + "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name in each one's annotation, `.codeowner` or package:" + } else { + "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + Check the team name in each one's annotation, `.codeowner` or package, \ + or run `codeowners generate` to update the CODEOWNERS file:" + } +} type FixtureChange = fn(&Path) -> std::io::Result<()>; @@ -189,7 +197,7 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown &[command, "app/deprecated/typo.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("app/deprecated/typo.rb")) .stdout(predicate::str::contains("Unowned files detected:").not()); } @@ -210,7 +218,8 @@ fn test_validate_files_reports_allowed_and_unowned_files_under_separate_headings )? .failure() .stdout(predicate::eq(format!( - "Unowned files detected:\n app/stray.rb\n{UNLISTED_OWNER_HEADING}\n app/deprecated/typo.rb\n" + "Unowned files detected:\n app/stray.rb\n{}\n app/deprecated/typo.rb\n", + unlisted_owner_heading(command) ))); } Ok(()) @@ -223,7 +232,7 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown "app/deprecated/typo.rb", )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading("validate"))) .stdout(predicate::str::contains("app/deprecated/typo.rb")); Ok(()) } @@ -262,7 +271,7 @@ fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), &[command, "./app/deprecated/typo.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("./app/deprecated/typo.rb")); run_on_modified_fixture( own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team, @@ -335,7 +344,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_te for command in ["validate", "gv"] { run_on_modified_fixture(add_package, &[command, file])? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains(file)); } } @@ -363,7 +372,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_bracketed_package_naming_an_ &[command, "packs/[typo]/app/thing.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("packs/[typo]/app/thing.rb")); } Ok(()) @@ -401,7 +410,7 @@ fn test_validate_files_reports_an_allowed_file_in_a_braced_package_naming_an_unk &[command, "packs/{typo}/app/thing.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("packs/{typo}/app/thing.rb")); } Ok(()) @@ -419,7 +428,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno &[command, "app/deprecated/old.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("app/deprecated/old.rb")); } Ok(()) @@ -442,7 +451,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_nam &[command, "app/deprecated/old.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("app/deprecated/old.rb")); } Ok(()) @@ -465,7 +474,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_a &[command, "app/deprecated/{legacy}/thing.rb"], )? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); run_on_modified_fixture( add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, @@ -541,19 +550,23 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu for (add_owner, file) in owners { run_on_modified_fixture(add_owner, &["validate", file])? .failure() - .stdout(predicate::str::contains(UNLISTED_OWNER_HEADING)) + .stdout(predicate::str::contains(unlisted_owner_heading("validate"))) .stdout(predicate::str::contains(file)); } Ok(()) } -fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { - annotate_the_unowned_file_without_regenerating_codeowners(project_root)?; +fn name_an_executable(project_root: &Path) -> std::io::Result<()> { let config_path = project_root.join("config/code_ownership.yml"); let config = fs::read_to_string(&config_path)?; fs::write(config_path, config + "executable_name: bin/codeownership validate\n") } +fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + annotate_the_unowned_file_without_regenerating_codeowners(project_root)?; + name_an_executable(project_root) +} + #[test] fn test_validate_files_suggests_the_configured_executable_for_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { run_on_modified_fixture( @@ -567,6 +580,24 @@ fn test_validate_files_suggests_the_configured_executable_for_an_allowed_file_mi Ok(()) } +fn annotate_an_allowed_file_with_an_unknown_team_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; + name_an_executable(project_root) +} + +#[test] +fn test_generate_and_validate_files_does_not_suggest_the_configured_executable() -> Result<(), Box> { + // `gv` has just written CODEOWNERS, so running the executable to update it can't help. + run_on_modified_fixture( + annotate_an_allowed_file_with_an_unknown_team_and_name_an_executable, + &["gv", "app/deprecated/typo.rb"], + )? + .failure() + .stdout(predicate::str::contains(unlisted_owner_heading("gv"))) + .stdout(predicate::str::contains("to update the CODEOWNERS file").not()); + Ok(()) +} + fn put_the_allowed_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; fs::write( From b7d17c22fc28946c8c6ffed77f8230ec1ea41de7 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 05:25:52 -0700 Subject: [PATCH 32/38] Test a package.json owner and a sibling directory for allowed files Two changes to the owner lookup behind allow_unowned_globs in `validate ` and `gv ` passed every test. It could leave out package.json files naming a team that exists, because each one the tests add names a team that doesn't, and those are matched by their directory instead. `validate` would then pass an allowed file in such a package that CODEOWNERS doesn't list yet, rather than list it under the heading for files that name an owner CODEOWNERS doesn't list. It could also match the directory of a package or `.codeowner` naming a team that doesn't exist as a string prefix rather than a component at a time, so a package in `packs/typo` would name an owner for an allowed file with none in `packs/typo2`, which would be listed under that heading instead of passing. The test of an allowed file that CODEOWNERS doesn't list yet now also covers an owner from a package.json, as it did one from a package.yml and a `.codeowner`, and a new test passes the file in `packs/typo2` to `validate` and `gv`. Each fails with its change. Only tests change. --- tests/allow_unowned_globs_test.rs | 43 ++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 6ddc775..36e0f73 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -351,6 +351,30 @@ fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_te Ok(()) } +// `packs/typo` is a string prefix of `packs/typo2` but not a path prefix, so the package names no owner for the file there. +fn add_an_allowed_unowned_file_in_a_sibling_of_a_package_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root)?; + fs::create_dir_all(project_root.join("packs/typo2/app"))?; + fs::write(project_root.join("packs/typo2/app/thing.rb"), "puts 'no owner'\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", + ) +} + +#[test] +fn test_validate_files_passes_an_allowed_file_in_a_sibling_of_a_package_naming_an_unknown_team() -> Result<(), Box> { + for command in ["validate", "gv"] { + run_on_modified_fixture( + add_an_allowed_unowned_file_in_a_sibling_of_a_package_naming_an_unknown_team, + &[command, "packs/typo2/app/thing.rb"], + )? + .success() + .stdout(predicate::eq("")); + } + Ok(()) +} + // Read as a glob, `packs/[typo]` would match `packs/t` rather than the package's own directory. fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/[typo]/app"))?; @@ -535,10 +559,23 @@ fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io ) } +fn add_a_javascript_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { + fs::create_dir_all(project_root.join("frontend/foo/src"))?; + fs::write( + project_root.join("frontend/foo/package.json"), + "{\"metadata\": {\"owner\": \"Foo\"}}\n", + )?; + fs::write(project_root.join("frontend/foo/src/thing.js"), "console.log('foo');\n")?; + fs::write( + project_root.join("config/code_ownership.yml"), + "---\nowned_globs:\n - \"{app,config,frontend}/**/*.{rb,js}\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"frontend/foo/**/*\"\n", + ) +} + #[test] fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { // `validate`, not `gv`, which would add each file to CODEOWNERS. - let owners: [(FixtureChange, &str); 4] = [ + let owners: [(FixtureChange, &str); 5] = [ (annotate_the_unowned_file_without_regenerating_codeowners, "app/deprecated/old.rb"), ( give_the_unowned_file_a_team_glob_without_regenerating_codeowners, @@ -546,6 +583,10 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu ), (add_a_codeowner_file_without_regenerating_codeowners, "app/deprecated/old.rb"), (add_a_package_without_regenerating_codeowners, "packs/foo/app/thing.rb"), + ( + add_a_javascript_package_without_regenerating_codeowners, + "frontend/foo/src/thing.js", + ), ]; for (add_owner, file) in owners { run_on_modified_fixture(add_owner, &["validate", file])? From 1030e89cd30a8092299a914e5f251c86e9e739f4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 05:42:40 -0700 Subject: [PATCH 33/38] Share config writing between the allow_unowned_globs tests Thirteen fixture changes in the allow_unowned_globs tests replaced config/code_ownership.yml with an inline YAML string, each quote and newline escaped, and several were copies of one another: owning `**/*.rb` and allowing `**/*` three times, and owning `{app,config,packs}/**/*.rb` and allowing `packs/**/*` three more. Which globs a test's config owned or allowed had to be picked out of the escapes. A Config struct now takes each key's globs by name, and its write method renders the YAML, leaving out a key with no globs so that unowned_globs keeps its default. Every fixture change that writes the config now uses it, including name_an_executable, which appended its key to the fixture's config. The two that copied allow_every_file now call it. Every fixture a test builds has the same files, byte for byte, as before, and no test is added, removed or changed in what it checks. Only tests change. --- tests/allow_unowned_globs_test.rs | 159 +++++++++++++++++++----------- 1 file changed, 104 insertions(+), 55 deletions(-) diff --git a/tests/allow_unowned_globs_test.rs b/tests/allow_unowned_globs_test.rs index 36e0f73..b888b05 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/allow_unowned_globs_test.rs @@ -131,6 +131,35 @@ fn validate_absolute_path(modify: impl FnOnce(&Path) -> std::io::Result<()>, fil codeowners(temp_dir.path(), &[OsStr::new("validate"), file_absolute_path.as_os_str()]) } +// A config/code_ownership.yml that replaces the fixture's. A key with no globs is left out, so unowned_globs keeps its default. +#[derive(Default)] +struct Config { + owned_globs: &'static [&'static str], + unowned_globs: &'static [&'static str], + allow_unowned_globs: &'static [&'static str], + executable_name: Option<&'static str>, +} + +impl Config { + fn write(self, project_root: &Path) -> std::io::Result<()> { + let mut yaml = String::from("---\n"); + for (key, globs) in [ + ("owned_globs", self.owned_globs), + ("unowned_globs", self.unowned_globs), + ("allow_unowned_globs", self.allow_unowned_globs), + ] { + if !globs.is_empty() { + yaml += &format!("{key}:\n"); + yaml.extend(globs.iter().map(|glob| format!(" - \"{glob}\"\n"))); + } + } + if let Some(executable_name) = self.executable_name { + yaml += &format!("executable_name: {executable_name}\n"); + } + fs::write(project_root.join("config/code_ownership.yml"), yaml) + } +} + fn add_unowned_file_outside_allowed_globs(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("app/stray.rb"), "puts 'no owner'\n") } @@ -255,10 +284,12 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown fn own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n", - ) + Config { + owned_globs: &["**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -284,10 +315,12 @@ fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), } fn allow_every_file(project_root: &Path) -> std::io::Result<()> { - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", - ) + Config { + owned_globs: &["**/*.rb"], + allow_unowned_globs: &["**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -312,10 +345,12 @@ fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; fs::write(project_root.join("packs/typo/app/thing.rb"), "puts 'typo'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/typo/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*", "packs/typo/**/*"], + ..Default::default() + } + .write(project_root) } fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { @@ -325,10 +360,12 @@ fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_ro "{\"metadata\": {\"owner\": \"Typo\"}}\n", )?; fs::write(project_root.join("frontend/typo/src/thing.js"), "console.log('typo');\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,frontend}/**/*.{rb,js}\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"frontend/typo/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], + allow_unowned_globs: &["app/deprecated/**/*", "frontend/typo/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -356,10 +393,12 @@ fn add_an_allowed_unowned_file_in_a_sibling_of_a_package_naming_an_unknown_team( add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root)?; fs::create_dir_all(project_root.join("packs/typo2/app"))?; fs::write(project_root.join("packs/typo2/app/thing.rb"), "puts 'no owner'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -382,10 +421,12 @@ fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_roo fs::write(project_root.join("packs/[typo]/app/thing.rb"), "puts 'typo'\n")?; fs::create_dir_all(project_root.join("packs/t"))?; fs::write(project_root.join("packs/t/stray.rb"), "puts 'no owner'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -420,10 +461,12 @@ fn add_a_braced_package_naming_an_unknown_team_in_an_allowed_glob(project_root: fs::create_dir_all(project_root.join("packs/{typo}/app"))?; fs::write(project_root.join("packs/{typo}/package.yml"), "owner: Typo\n")?; fs::write(project_root.join("packs/{typo}/app/thing.rb"), "puts 'typo'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -461,10 +504,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno // A glob built from the project root, `/**/**`, would match no project-relative path. fn allow_every_file_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join(".codeowner"), "Typo\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", - ) + allow_every_file(project_root) } #[test] @@ -553,10 +593,12 @@ fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io fs::create_dir_all(project_root.join("packs/foo/app"))?; fs::write(project_root.join("packs/foo/package.yml"), "owner: Foo\n")?; fs::write(project_root.join("packs/foo/app/thing.rb"), "puts 'foo'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,packs}/**/*.rb\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"packs/foo/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,packs}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*", "packs/foo/**/*"], + ..Default::default() + } + .write(project_root) } fn add_a_javascript_package_without_regenerating_codeowners(project_root: &Path) -> std::io::Result<()> { @@ -566,10 +608,12 @@ fn add_a_javascript_package_without_regenerating_codeowners(project_root: &Path) "{\"metadata\": {\"owner\": \"Foo\"}}\n", )?; fs::write(project_root.join("frontend/foo/src/thing.js"), "console.log('foo');\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config,frontend}/**/*.{rb,js}\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n - \"frontend/foo/**/*\"\n", - ) + Config { + owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], + allow_unowned_globs: &["app/deprecated/**/*", "frontend/foo/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -598,9 +642,13 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu } fn name_an_executable(project_root: &Path) -> std::io::Result<()> { - let config_path = project_root.join("config/code_ownership.yml"); - let config = fs::read_to_string(&config_path)?; - fs::write(config_path, config + "executable_name: bin/codeownership validate\n") + Config { + owned_globs: &["{app,config}/**/*.rb"], + allow_unowned_globs: &["app/deprecated/**/*"], + executable_name: Some("bin/codeownership validate"), + ..Default::default() + } + .write(project_root) } fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { @@ -641,10 +689,13 @@ fn test_generate_and_validate_files_does_not_suggest_the_configured_executable() fn put_the_allowed_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\nunowned_globs:\n - \"app/deprecated/**/*\"\nallow_unowned_globs:\n - \"app/deprecated/**/*\"\n", - ) + Config { + owned_globs: &["{app,config}/**/*.rb"], + unowned_globs: &["app/deprecated/**/*"], + allow_unowned_globs: &["app/deprecated/**/*"], + ..Default::default() + } + .write(project_root) } #[test] @@ -680,10 +731,11 @@ fn test_validate_files_leaves_out_files_in_both_globs() -> Result<(), Box Result<(), Box> { run_on_modified_fixture( |project_root| { - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"{app,config}/**/*.rb\"\n", - ) + Config { + owned_globs: &["{app,config}/**/*.rb"], + ..Default::default() + } + .write(project_root) }, &["validate"], )? @@ -695,10 +747,7 @@ fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box fn allow_every_unowned_file_with_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("root.rb"), "puts 'no owner'\n")?; - fs::write( - project_root.join("config/code_ownership.yml"), - "---\nowned_globs:\n - \"**/*.rb\"\nallow_unowned_globs:\n - \"**/*\"\n", - ) + allow_every_file(project_root) } #[test] From c11f489e28a76a40ed6d154ba8258a347b24e15e Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 05:51:41 -0700 Subject: [PATCH 34/38] Correct comments on the owner lookup and the config's node limit Two comments on the owner lookup behind allow_unowned_globs in `validate ` and `gv ` said an annotated file, `.codeowner` or package file names no owner there if it is under ignore_dirs, but the project builder prunes only a directory at the project root that ignore_dirs names. With the default ignore_dirs, an annotation naming a team that doesn't exist keeps a file in `app/tmp` from passing, though not one in `tmp`. They now say a top-level directory named in ignore_dirs. The one in validate_files also said a file whose annotation, `.codeowner` or package names a team that doesn't exist, "or that CODEOWNERS doesn't list yet", is still reported, which can be read as a team that CODEOWNERS doesn't list. It now says "or whose owner CODEOWNERS doesn't list yet", as the README does, since an owner that CODEOWNERS lists for other files counts too. The comment where the config is read said that at a few hundred nodes it never needs a higher limit, but a long literal allow_unowned_globs list can run to thousands of entries, and the README says the config is held to the default of 20,000 nodes, about one per list entry. It now says the config's lists can hold about 20,000 entries in all. Only comments change. --- src/config.rs | 2 +- src/ownership.rs | 3 ++- src/runner.rs | 11 ++++++----- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/config.rs b/src/config.rs index a8132e0..c891999 100644 --- a/src/config.rs +++ b/src/config.rs @@ -125,7 +125,7 @@ fn default_path() -> PathBuf { impl Config { pub fn load_from_path(path: &Path) -> std::result::Result { let bytes = std::fs::read(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; - // The config can't raise its own limit, and at a few hundred nodes it never needs to. + // The config can't raise its own limit, so its lists can hold about 20,000 entries in all. let config: Self = crate::yaml::from_slice(&bytes, crate::yaml::DEFAULT_MAX_NODES) .map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e))?; Ok(Self { diff --git a/src/ownership.rs b/src/ownership.rs index ad6a3b9..e076fe5 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -112,7 +112,8 @@ impl Entry { /// Tells whether anything names an owner for a project-relative path, even a team that doesn't exist: an annotation naming one /// still counts for its file, and a `.codeowner`, `package.yml` or `package.json` naming one for every path under its directory. /// It sees only files the project reads, so an annotated file, `.codeowner` or package file names no owner here if it is -/// untracked or under ignore_dirs, as in `validate`. Building one goes through every project file, so reuse it. +/// untracked or under a top-level directory named in ignore_dirs, as in `validate`. Building one goes through every project +/// file, so reuse it. pub(crate) struct OwnerLookup { owner_matchers: Vec, unknown_team_roots: Vec, diff --git a/src/runner.rs b/src/runner.rs index 84887f8..c33425c 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -182,11 +182,12 @@ impl Runner { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or - // package names a team that doesn't exist, or that CODEOWNERS doesn't list yet, is still reported, but not - // as unowned, which the key allows. Only files the project reads count, though, so an annotated file, - // `.codeowner` or package file names no owner here if it is untracked or under ignore_dirs, as in - // `validate`. The lookup matches plain project-relative paths only, so a path with a `..`, or an absolute - // one not under the project root, is never excused and is reported as unowned, as without the key. + // package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported, + // but not as unowned, which the key allows. Only files the project reads count, though, so an annotated + // file, `.codeowner` or package file names no owner here if it is untracked or under a top-level directory + // named in ignore_dirs, as in `validate`. The lookup matches plain project-relative paths only, so a path + // with a `..`, or an absolute one not under the project root, is never excused and is reported as unowned, + // as without the key. Ok(None) => match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.allow_unowned_globs)) { Some(path) if !owner_lookup.has_owner(&path) => {} // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` made it miss the file's line. From 579a86d514fb4c16702f10a77ed4486b6c69860c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 05:59:25 -0700 Subject: [PATCH 35/38] Clarify the README on team files and what the owner lookup reads In the README's unowned_globs bullet, "So is a team file, which still defines its team and keeps its own line in CODEOWNERS" followed "an invalid team in it is still reported", so the elliptical "So is" could be read as saying that a team file is still reported. It now says that a team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. The allow_unowned_globs bullet said that, given a list of files, an allowed file whose annotation, `.codeowner` or package names a team that doesn't exist is reported, but the owner lookup that decides this sees only the files the project reads. An untracked file's own annotation names no owner there, nor does an untracked `.codeowner` or package file, or any of them in a top-level directory named in ignore_dirs, as in a full `validate`. So `validate app/new.rb` passes a new allowed file whose annotation names a team that doesn't exist, and reports it only once the file is staged. A sentence after the one quoting the heading now says so. It says that such a file doesn't count, rather than that only a tracked one does, since outside a git repository no file is left out for being untracked. Only the README changes. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4a2d803..37f5225 100644 --- a/README.md +++ b/README.md @@ -212,8 +212,8 @@ codeowners gv --no-cache - `ruby_package_paths` (default: `['packs/**/*', 'components/**']`) - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) -- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. So is a team file, which still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code. -- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list". Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. +- `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. A team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code. +- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list". As in a full `validate`, an annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) From 60e7ef9dc5092441b255de4b05ef9ee154011469 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 06:07:47 -0700 Subject: [PATCH 36/38] Give executable_name's real default in the README The README said executable_name defaults to `'codeowners'`, but the default, here and on main, is `codeowners generate`. Its example config set `bin/codeownership` for the Ruby gem, while the gem passes `bin/codeownership validate`, which regenerates CODEOWNERS by default. Both the stale-CODEOWNERS message and the new hint for allowed files in `validate ` print this value as the command to run to update CODEOWNERS. The gem's own runs aren't affected, since the value it passes replaces the config's, but a config copied from the example made `codeowners validate` tell users to run `bin/codeownership`, which only prints the gem's usage. The bullet now gives the real default and says the key is the whole command these messages tell you to run, rather than a command name, and the example sets `bin/codeownership validate`. Only the README changes. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 37f5225..ff92889 100644 --- a/README.md +++ b/README.md @@ -217,7 +217,7 @@ codeowners gv --no-cache - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) -- `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. +- `executable_name` (default: `'codeowners generate'`): The command that validation errors tell you to run to update the CODEOWNERS file. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. - `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint to raise this key in the config file in use. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to the config file itself, which is always held to the default of 20,000 nodes, about one per list entry. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: @@ -225,7 +225,7 @@ Example configuration with custom executable name: ```yaml owned_globs: - '{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx}' -executable_name: 'bin/codeownership' # For Ruby gem wrapper +executable_name: 'bin/codeownership validate' # For Ruby gem wrapper ``` See examples in `tests/fixtures/**/config/` for reference setups. From 132bde12e57c4522c0692db4a21d7613707cbfa4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 09:49:07 -0700 Subject: [PATCH 37/38] Rename allow_unowned_globs to optionally_owned_globs `allow_unowned_globs` reads as `allow_` followed by the name of the `unowned_globs` key, as though it switched on or loosened that key. The relationship is the reverse: the new key narrows `owned_globs`, whose files it still reads and checks, and has nothing to do with `unowned_globs`, which takes precedence over it. The `allow_` prefix also reads like a boolean, as rubyatscale/code_ownership#140 proposed with `allow_unowned_files: true`, while the other list keys are noun phrases. `optionally_owned_globs` sits between the two existing keys: files in `owned_globs` must have an owner, files that also match `optionally_owned_globs` may have one, and files in `unowned_globs` are left out. Since the name could be read as adding files beyond `owned_globs`, the README now says it doesn't, and that a file must match both. The key, the `Config` and `Project` fields, the heading that `validate ` prints, the README, the fixture directory, the test file and the test names all use the new name, and prose that called these files "allowed" now calls them optionally owned. 0.6.0 hasn't been released, so nothing reads the old name and no alias is kept. --- README.md | 6 +- src/config.rs | 14 +- src/ownership/file_owner_resolver.rs | 2 +- src/ownership/validator.rs | 2 +- src/project.rs | 4 +- src/project_builder.rs | 2 +- src/runner.rs | 32 ++- .../app/deprecated/annotated_old.rb | 2 - .../.github/CODEOWNERS | 0 .../app/annotated.rb | 0 .../app/deprecated/annotated_old.rb | 2 + .../app/deprecated/old.rb | 0 .../config/code_ownership.yml | 2 +- .../config/teams/foo.yml | 0 ...test.rs => optionally_owned_globs_test.rs} | 236 ++++++++++-------- 15 files changed, 164 insertions(+), 140 deletions(-) delete mode 100644 tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb rename tests/fixtures/{allow_unowned_globs => optionally_owned_globs}/.github/CODEOWNERS (100%) rename tests/fixtures/{allow_unowned_globs => optionally_owned_globs}/app/annotated.rb (100%) create mode 100644 tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb rename tests/fixtures/{allow_unowned_globs => optionally_owned_globs}/app/deprecated/old.rb (100%) rename tests/fixtures/{allow_unowned_globs => optionally_owned_globs}/config/code_ownership.yml (74%) rename tests/fixtures/{allow_unowned_globs => optionally_owned_globs}/config/teams/foo.yml (100%) rename tests/{allow_unowned_globs_test.rs => optionally_owned_globs_test.rs} (68%) diff --git a/README.md b/README.md index ff92889..d99237b 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ cargo install --git https://github.com/rubyatscale/codeowners-rs codeowners unowned_globs: - db/**/* - frontend/javascripts/**/__generated__/**/* - allow_unowned_globs: + optionally_owned_globs: - app/services/some_file1.rb ``` @@ -213,7 +213,7 @@ codeowners gv --no-cache - `js_package_paths` / `javascript_package_paths` (default: `['frontend/**/*']`) - `team_file_glob` (default: `['config/teams/**/*.yml']`) - `unowned_globs` (default: `['frontend/**/node_modules/**/*', 'frontend/**/__generated__/**/*']`): Files matched here are left out of the project: they don't need an owner, validation doesn't check them, and file annotations in them are ignored. A `.codeowner`, `package.yml` or `package.json` among them is still read, though, so its owner still goes into CODEOWNERS and an invalid team in it is still reported. A team file among them is still read too, so it still defines its team and keeps its own line in CODEOWNERS. Use this for third-party or generated code. -- `allow_unowned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An allowed file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list". As in a full `validate`, an annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `allow_unowned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. +- `optionally_owned_globs` (default: `[]`, added in 0.6.0): Files in `owned_globs` that match these globs don't need an owner. It doesn't add to `owned_globs`, so a file must match both. Unlike `unowned_globs`, file annotations in them still count, and a full `validate` or `gv` still runs every other check, so a file there can't have more than one owner or name an invalid team. Given a list of files, as in a git hook, `validate` and `gv` look each one up in CODEOWNERS and don't check for more than one owner. An optionally owned file with no line passes only if nothing names an owner for it, so one whose annotation, `.codeowner` or package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is reported under "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list". As in a full `validate`, an annotation, `.codeowner` or package file doesn't count if the file is untracked or in a top-level directory named in `ignore_dirs`, so a new file's own annotation counts once the file is staged. Use `['**/*']` if only some of your code has owners, or scope it, e.g. `['**/deprecated/**/*']`. `unowned_globs` takes precedence, so a file that matches both is left out. If you use `unowned_globs: ['**/*']` only so that files can go unowned, replace it with `optionally_owned_globs: ['**/*']` to make annotations work, and run `codeowners gv` to add a CODEOWNERS line for each annotated file. The files that `unowned_globs` left out are then read and checked like any other owned file, which takes longer and may turn up problems that were hidden before. Keep third-party and generated code in `unowned_globs`. Versions before 0.6.0 ignore the key without a warning, so they still report unowned files in these globs. - `vendored_gems_path` (default: `'vendored/'`) - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) @@ -243,7 +243,7 @@ By default, cache is stored under `tmp/cache/codeowners` relative to the project 1. Only one mechanism defines ownership for any file. 2. All referenced teams are valid. -3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `allow_unowned_globs`. +3. All files in `owned_globs` are owned, unless matched by `unowned_globs` or `optionally_owned_globs`. 4. The generated `CODEOWNERS` file is up to date. Exit status is non-zero on errors. diff --git a/src/config.rs b/src/config.rs index c891999..23cbba1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -45,7 +45,7 @@ pub struct Config { pub codeowners_path: String, #[serde(default, deserialize_with = "crate::yaml::strings_or_nulls")] - pub allow_unowned_globs: Vec, + pub optionally_owned_globs: Vec, #[serde(default = "default_yaml_max_nodes")] pub yaml_max_nodes: usize, @@ -176,7 +176,7 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); - assert!(config.allow_unowned_globs.is_empty()); + assert!(config.optionally_owned_globs.is_empty()); assert_eq!(config.yaml_max_nodes, 20_000); Ok(()) } @@ -259,19 +259,19 @@ mod tests { } #[test] - fn test_parse_config_with_allow_unowned_globs() -> Result<(), Box> { + fn test_parse_config_with_optionally_owned_globs() -> Result<(), Box> { let temp_dir = tempdir()?; let config_path = temp_dir.path().join("config.yml"); let config_str = indoc! {" --- owned_globs: - \"**/*.rb\" - allow_unowned_globs: + optionally_owned_globs: - \"**/deprecated/**/*\" "}; fs::write(&config_path, config_str)?; let config = Config::load_from_path(&config_path)?; - assert_eq!(config.allow_unowned_globs, vec!["**/deprecated/**/*"]); + assert_eq!(config.optionally_owned_globs, vec!["**/deprecated/**/*"]); Ok(()) } @@ -325,7 +325,7 @@ mod tests { - ignore_dirs: - - allow_unowned_globs: + optionally_owned_globs: - vendored_gems_path: cache_directory: @@ -341,7 +341,7 @@ mod tests { &config.team_file_glob, &config.unowned_globs, &config.ignore_dirs, - &config.allow_unowned_globs, + &config.optionally_owned_globs, ] { assert_eq!(globs, &[""]); } diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 22674ba..23a5fcc 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -385,7 +385,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), - allow_unowned_globs: vec![], + optionally_owned_globs: vec![], yaml_max_nodes: crate::yaml::DEFAULT_MAX_NODES, path: PathBuf::from(crate::config::DEFAULT_PATH), } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index d5e6341..e291755 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -176,7 +176,7 @@ impl Validator { let relative_path = project.relative_path(&file.path).to_owned(); if owners.is_empty() { - if matches_globs(&relative_path, &project.allow_unowned_globs) { + if matches_globs(&relative_path, &project.optionally_owned_globs) { None } else { Some(Error::FileWithoutOwner { path: relative_path }) diff --git a/src/project.rs b/src/project.rs index 7f46238..1740172 100644 --- a/src/project.rs +++ b/src/project.rs @@ -17,7 +17,7 @@ pub struct Project { pub directory_codeowner_files: Vec, pub teams_by_name: HashMap, pub executable_name: String, - pub allow_unowned_globs: Vec, + pub optionally_owned_globs: Vec, } #[derive(Clone, Debug)] @@ -230,7 +230,7 @@ mod tests { directory_codeowner_files: vec![], teams_by_name: HashMap::new(), executable_name: "codeowners generate".to_string(), - allow_unowned_globs: vec![], + optionally_owned_globs: vec![], }; let map = project.vendored_gem_by_name(); diff --git a/src/project_builder.rs b/src/project_builder.rs index c0a3ce2..a3e1b2d 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -327,7 +327,7 @@ impl<'a> ProjectBuilder<'a> { directory_codeowner_files: directory_codeowners, teams_by_name, executable_name: self.config.executable_name.clone(), - allow_unowned_globs: self.config.allow_unowned_globs.clone(), + optionally_owned_globs: self.config.optionally_owned_globs.clone(), }) } } diff --git a/src/runner.rs b/src/runner.rs index c33425c..f4397e5 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -145,7 +145,7 @@ impl Runner { fn validate_files(&self, file_paths: Vec, codeowners_generated: bool) -> RunResult { let mut unowned_files = Vec::new(); - let mut allowed_files_with_unlisted_owners = Vec::new(); + let mut optionally_owned_files_with_unlisted_owners = Vec::new(); let mut io_errors = Vec::new(); // Filter files based on owned_globs and unowned_globs configuration @@ -168,7 +168,7 @@ impl Runner { }) .collect(); - // Built only if an allowed file needs it, and then once. + // Built only if an optionally owned file needs it, and then once. let owner_lookup = LazyCell::new(|| self.ownership.owner_lookup()); let listed_in_codeowners = |path: &Path| { matches!( @@ -181,20 +181,22 @@ impl Runner { for (file_path, relative_path) in filtered_paths { match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} - // allow_unowned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or + // optionally_owned_globs excuses only a file nothing names an owner for. One whose annotation, `.codeowner` or // package names a team that doesn't exist, or whose owner CODEOWNERS doesn't list yet, is still reported, // but not as unowned, which the key allows. Only files the project reads count, though, so an annotated // file, `.codeowner` or package file names no owner here if it is untracked or under a top-level directory // named in ignore_dirs, as in `validate`. The lookup matches plain project-relative paths only, so a path // with a `..`, or an absolute one not under the project root, is never excused and is reported as unowned, // as without the key. - Ok(None) => match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.allow_unowned_globs)) { - Some(path) if !owner_lookup.has_owner(&path) => {} - // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` made it miss the file's line. - Some(path) if path.as_os_str() != relative_path.as_os_str() && listed_in_codeowners(&path) => {} - Some(_) => allowed_files_with_unlisted_owners.push(file_path), - None => unowned_files.push(file_path), - }, + Ok(None) => { + match plain_relative(&relative_path).filter(|path| matches_globs(path, &self.config.optionally_owned_globs)) { + Some(path) if !owner_lookup.has_owner(&path) => {} + // The CODEOWNERS lookup above read the path as given, so a `./`, `//` or inner `.` missed the file's line. + Some(path) if path.as_os_str() != relative_path.as_os_str() && listed_in_codeowners(&path) => {} + Some(_) => optionally_owned_files_with_unlisted_owners.push(file_path), + None => unowned_files.push(file_path), + } + } Err(err) => io_errors.push(format!("{}: {}", file_path, err)), } } @@ -205,7 +207,7 @@ impl Runner { validation_errors.push("Unowned files detected:".to_string()); validation_errors.extend(unowned_files.into_iter().map(|file| format!(" {}", file))); } - if !allowed_files_with_unlisted_owners.is_empty() { + if !optionally_owned_files_with_unlisted_owners.is_empty() { // `gv` has just written CODEOWNERS, so updating it again can't help, and only a team that doesn't exist is left out. let or_update_codeowners = if codeowners_generated { String::new() @@ -213,11 +215,15 @@ impl Runner { format!(", or run `{}` to update the CODEOWNERS file", self.config.executable_name) }; validation_errors.push(format!( - "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ Check the team name in each one's annotation, `.codeowner` or package{}:", or_update_codeowners )); - validation_errors.extend(allowed_files_with_unlisted_owners.into_iter().map(|file| format!(" {}", file))); + validation_errors.extend( + optionally_owned_files_with_unlisted_owners + .into_iter() + .map(|file| format!(" {}", file)), + ); } if !validation_errors.is_empty() { diff --git a/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb b/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb deleted file mode 100644 index ca506d0..0000000 --- a/tests/fixtures/allow_unowned_globs/app/deprecated/annotated_old.rb +++ /dev/null @@ -1,2 +0,0 @@ -# @team Foo -puts 'annotated, in an allowed glob' diff --git a/tests/fixtures/allow_unowned_globs/.github/CODEOWNERS b/tests/fixtures/optionally_owned_globs/.github/CODEOWNERS similarity index 100% rename from tests/fixtures/allow_unowned_globs/.github/CODEOWNERS rename to tests/fixtures/optionally_owned_globs/.github/CODEOWNERS diff --git a/tests/fixtures/allow_unowned_globs/app/annotated.rb b/tests/fixtures/optionally_owned_globs/app/annotated.rb similarity index 100% rename from tests/fixtures/allow_unowned_globs/app/annotated.rb rename to tests/fixtures/optionally_owned_globs/app/annotated.rb diff --git a/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb b/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb new file mode 100644 index 0000000..d150983 --- /dev/null +++ b/tests/fixtures/optionally_owned_globs/app/deprecated/annotated_old.rb @@ -0,0 +1,2 @@ +# @team Foo +puts 'annotated, in an optionally owned glob' diff --git a/tests/fixtures/allow_unowned_globs/app/deprecated/old.rb b/tests/fixtures/optionally_owned_globs/app/deprecated/old.rb similarity index 100% rename from tests/fixtures/allow_unowned_globs/app/deprecated/old.rb rename to tests/fixtures/optionally_owned_globs/app/deprecated/old.rb diff --git a/tests/fixtures/allow_unowned_globs/config/code_ownership.yml b/tests/fixtures/optionally_owned_globs/config/code_ownership.yml similarity index 74% rename from tests/fixtures/allow_unowned_globs/config/code_ownership.yml rename to tests/fixtures/optionally_owned_globs/config/code_ownership.yml index 8fd3663..cb352a7 100644 --- a/tests/fixtures/allow_unowned_globs/config/code_ownership.yml +++ b/tests/fixtures/optionally_owned_globs/config/code_ownership.yml @@ -1,5 +1,5 @@ --- owned_globs: - "{app,config}/**/*.rb" -allow_unowned_globs: +optionally_owned_globs: - "app/deprecated/**/*" diff --git a/tests/fixtures/allow_unowned_globs/config/teams/foo.yml b/tests/fixtures/optionally_owned_globs/config/teams/foo.yml similarity index 100% rename from tests/fixtures/allow_unowned_globs/config/teams/foo.yml rename to tests/fixtures/optionally_owned_globs/config/teams/foo.yml diff --git a/tests/allow_unowned_globs_test.rs b/tests/optionally_owned_globs_test.rs similarity index 68% rename from tests/allow_unowned_globs_test.rs rename to tests/optionally_owned_globs_test.rs index b888b05..c916837 100644 --- a/tests/allow_unowned_globs_test.rs +++ b/tests/optionally_owned_globs_test.rs @@ -7,15 +7,15 @@ use tempfile::TempDir; mod common; use common::{OutputStream, git_add_all_files, run_codeowners, setup_fixture_repo}; -const FIXTURE: &str = "allow_unowned_globs"; +const FIXTURE: &str = "optionally_owned_globs"; // `validate` suggests running the default executable_name, but `gv` has just written CODEOWNERS, so it doesn't. fn unlisted_owner_heading(command: &str) -> &'static str { if command == "gv" { - "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ Check the team name in each one's annotation, `.codeowner` or package:" } else { - "Files matching allow_unowned_globs name an owner that CODEOWNERS doesn't list. \ + "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list. \ Check the team name in each one's annotation, `.codeowner` or package, \ or run `codeowners generate` to update the CODEOWNERS file:" } @@ -24,21 +24,21 @@ fn unlisted_owner_heading(command: &str) -> &'static str { type FixtureChange = fn(&Path) -> std::io::Result<()>; #[test] -fn test_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { - // The fixture's CODEOWNERS lists `app/deprecated/annotated_old.rb`, so if annotations in allowed globs stop +fn test_validate_passes_with_unowned_files_in_optionally_owned_globs() -> Result<(), Box> { + // The fixture's CODEOWNERS lists `app/deprecated/annotated_old.rb`, so if annotations in optionally owned globs stop // counting, this also fails, with CODEOWNERS out of date. run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; Ok(()) } #[test] -fn test_generate_and_validate_passes_with_unowned_files_in_allowed_globs() -> Result<(), Box> { +fn test_generate_and_validate_passes_with_unowned_files_in_optionally_owned_globs() -> Result<(), Box> { run_codeowners(FIXTURE, &["gv"], true, OutputStream::Stdout, predicate::eq(""))?; Ok(()) } #[test] -fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { +fn test_validate_files_passes_for_an_unowned_file_in_an_optionally_owned_glob() -> Result<(), Box> { run_codeowners( FIXTURE, &["validate", "app/deprecated/old.rb"], @@ -50,7 +50,7 @@ fn test_validate_files_passes_for_an_unowned_file_in_an_allowed_glob() -> Result } #[test] -fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Result<(), Box> { +fn test_validate_files_passes_for_an_absolute_path_in_an_optionally_owned_glob() -> Result<(), Box> { validate_absolute_path(|_| Ok(()), "app/deprecated/old.rb")? .success() .stdout(predicate::eq("")); @@ -58,8 +58,8 @@ fn test_validate_files_passes_for_an_absolute_path_in_an_allowed_glob() -> Resul } #[test] -fn test_validate_files_passes_for_an_annotated_and_an_unowned_file_in_an_allowed_glob() -> Result<(), Box> { - // What a pre-commit hook checks when a commit edits both. An allowed file that names an owner is reported only if +fn test_validate_files_passes_for_an_annotated_and_an_unowned_file_in_an_optionally_owned_glob() -> Result<(), Box> { + // What a pre-commit hook checks when a commit edits both. An optionally owned file that names an owner is reported only if // CODEOWNERS doesn't list it, and CODEOWNERS lists the annotated one. for command in ["validate", "gv"] { run_codeowners( @@ -73,9 +73,9 @@ fn test_validate_files_passes_for_an_annotated_and_an_unowned_file_in_an_allowed Ok(()) } -// The `for-file` tests pin that it reports an allowed file's owner as it would without allow_unowned_globs. +// The `for-file` tests pin that it reports an optionally owned file's owner as it would without optionally_owned_globs. #[test] -fn test_for_file_reports_annotations_in_allowed_globs() -> Result<(), Box> { +fn test_for_file_reports_annotations_in_optionally_owned_globs() -> Result<(), Box> { run_codeowners( FIXTURE, &["for-file", "app/deprecated/annotated_old.rb"], @@ -93,7 +93,7 @@ fn test_for_file_reports_annotations_in_allowed_globs() -> Result<(), Box Result<(), Box> { +fn test_for_file_reports_an_unowned_file_in_an_optionally_owned_glob_as_unowned() -> Result<(), Box> { run_codeowners( FIXTURE, &["for-file", "app/deprecated/old.rb"], @@ -136,7 +136,7 @@ fn validate_absolute_path(modify: impl FnOnce(&Path) -> std::io::Result<()>, fil struct Config { owned_globs: &'static [&'static str], unowned_globs: &'static [&'static str], - allow_unowned_globs: &'static [&'static str], + optionally_owned_globs: &'static [&'static str], executable_name: Option<&'static str>, } @@ -146,7 +146,7 @@ impl Config { for (key, globs) in [ ("owned_globs", self.owned_globs), ("unowned_globs", self.unowned_globs), - ("allow_unowned_globs", self.allow_unowned_globs), + ("optionally_owned_globs", self.optionally_owned_globs), ] { if !globs.is_empty() { yaml += &format!("{key}:\n"); @@ -160,13 +160,13 @@ impl Config { } } -fn add_unowned_file_outside_allowed_globs(project_root: &Path) -> std::io::Result<()> { +fn add_unowned_file_outside_optionally_owned_globs(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("app/stray.rb"), "puts 'no owner'\n") } #[test] -fn test_validate_reports_unowned_files_outside_allowed_globs() -> Result<(), Box> { - run_on_modified_fixture(add_unowned_file_outside_allowed_globs, &["validate"])? +fn test_validate_reports_unowned_files_outside_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_optionally_owned_globs, &["validate"])? .failure() .stdout(predicate::str::contains("Some files are missing ownership")) .stdout(predicate::str::contains("- app/stray.rb")) @@ -175,15 +175,15 @@ fn test_validate_reports_unowned_files_outside_allowed_globs() -> Result<(), Box } #[test] -fn test_validate_files_reports_an_unowned_file_outside_allowed_globs() -> Result<(), Box> { - run_on_modified_fixture(add_unowned_file_outside_allowed_globs, &["validate", "app/stray.rb"])? +fn test_validate_files_reports_an_unowned_file_outside_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_unowned_file_outside_optionally_owned_globs, &["validate", "app/stray.rb"])? .failure() .stdout(predicate::str::contains("Unowned files detected:")) .stdout(predicate::str::contains("app/stray.rb")); Ok(()) } -fn add_a_second_team_owning_the_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_second_team_owning_the_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::write( project_root.join("config/teams/bar.yml"), "---\nname: Bar\ngithub:\n team: '@BarTeam'\nowned_globs:\n - app/deprecated/**/*\n", @@ -191,8 +191,8 @@ fn add_a_second_team_owning_the_allowed_glob(project_root: &Path) -> std::io::Re } #[test] -fn test_validate_reports_multiple_owners_in_allowed_globs() -> Result<(), Box> { - run_on_modified_fixture(add_a_second_team_owning_the_allowed_glob, &["validate"])? +fn test_validate_reports_multiple_owners_in_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_a_second_team_owning_the_optionally_owned_glob, &["validate"])? .failure() .stdout(predicate::str::contains( "Code ownership should only be defined for each file in one way.", @@ -201,13 +201,13 @@ fn test_validate_reports_multiple_owners_in_allowed_globs() -> Result<(), Box std::io::Result<()> { +fn add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("app/deprecated/typo.rb"), "# @team Typo\nputs 'typo'\n") } #[test] -fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box> { - run_on_modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, &["validate"])? +fn test_validate_reports_invalid_teams_in_optionally_owned_globs() -> Result<(), Box> { + run_on_modified_fixture(add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, &["validate"])? .failure() .stdout(predicate::str::contains("Found invalid team references")) .stdout(predicate::str::contains( @@ -217,12 +217,12 @@ fn test_validate_reports_invalid_teams_in_allowed_globs() -> Result<(), Box Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team() -> Result<(), Box> { // A list of files isn't checked for invalid teams. CODEOWNERS has no line for this file, and its annotation keeps - // allow_unowned_globs from excusing it, so it's reported, but not as unowned. + // optionally_owned_globs from excusing it, so it's reported, but not as unowned. for command in ["validate", "gv"] { run_on_modified_fixture( - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/typo.rb"], )? .failure() @@ -233,16 +233,16 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown Ok(()) } -fn add_an_unowned_file_and_an_allowed_one_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { - add_unowned_file_outside_allowed_globs(project_root)?; - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root) +fn add_an_unowned_file_and_an_optionally_owned_one_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_unowned_file_outside_optionally_owned_globs(project_root)?; + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root) } #[test] -fn test_validate_files_reports_allowed_and_unowned_files_under_separate_headings() -> Result<(), Box> { +fn test_validate_files_reports_optionally_owned_and_unowned_files_under_separate_headings() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_an_unowned_file_and_an_allowed_one_naming_an_unknown_team, + add_an_unowned_file_and_an_optionally_owned_one_naming_an_unknown_team, &[command, "app/deprecated/typo.rb", "app/stray.rb"], )? .failure() @@ -255,9 +255,10 @@ fn test_validate_files_reports_allowed_and_unowned_files_under_separate_headings } #[test] -fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team_by_absolute_path() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_absolute_path() +-> Result<(), Box> { validate_absolute_path( - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, "app/deprecated/typo.rb", )? .failure() @@ -267,12 +268,13 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown } #[test] -fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_whose_annotation_names_an_unknown_team_by_path_with_dot_dot() +-> Result<(), Box> { // The owner lookup can't see the annotation without resolving the `..`, so the file mustn't be excused. It's reported // as unowned, as without the key. for command in ["validate", "gv"] { run_on_modified_fixture( - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob, + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/../deprecated/typo.rb"], )? .failure() @@ -282,11 +284,11 @@ fn test_validate_files_reports_an_allowed_file_whose_annotation_names_an_unknown Ok(()) } -fn own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; +fn own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; Config { owned_globs: &["**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*"], + optionally_owned_globs: &["app/deprecated/**/*"], ..Default::default() } .write(project_root) @@ -294,18 +296,18 @@ fn own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team(project_ #[test] fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Box> { - // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`. The allowed glob doesn't, so these + // Unlike the fixture's owned_globs, `**/*.rb` matches a path starting with `./`. The optionally owned glob doesn't, so these // reach the owner lookup only if it's matched against the plain path. for command in ["validate", "gv"] { run_on_modified_fixture( - own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team, + own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team, &[command, "./app/deprecated/typo.rb"], )? .failure() .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("./app/deprecated/typo.rb")); run_on_modified_fixture( - own_every_ruby_file_and_annotate_an_allowed_one_with_an_unknown_team, + own_every_ruby_file_and_annotate_an_optionally_owned_one_with_an_unknown_team, &[command, "./app/deprecated/old.rb"], )? .success() @@ -314,17 +316,17 @@ fn test_validate_files_treats_a_dot_slash_path_like_a_plain_one() -> Result<(), Ok(()) } -fn allow_every_file(project_root: &Path) -> std::io::Result<()> { +fn make_every_file_optionally_owned(project_root: &Path) -> std::io::Result<()> { Config { owned_globs: &["**/*.rb"], - allow_unowned_globs: &["**/*"], + optionally_owned_globs: &["**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_looks_up_an_allowed_file_in_codeowners_by_its_plain_path() -> Result<(), Box> { +fn test_validate_files_looks_up_an_optionally_owned_file_in_codeowners_by_its_plain_path() -> Result<(), Box> { // Read as given, none of these paths matches annotated_old.rb's line in CODEOWNERS, and the owner lookup finds its // annotation. for command in ["validate", "gv"] { @@ -333,7 +335,7 @@ fn test_validate_files_looks_up_an_allowed_file_in_codeowners_by_its_plain_path( "app//deprecated/annotated_old.rb", "app/./deprecated/annotated_old.rb", ] { - run_on_modified_fixture(allow_every_file, &[command, file])? + run_on_modified_fixture(make_every_file_optionally_owned, &[command, file])? .success() .stdout(predicate::eq("")); } @@ -341,19 +343,19 @@ fn test_validate_files_looks_up_an_allowed_file_in_codeowners_by_its_plain_path( Ok(()) } -fn add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/typo/app"))?; fs::write(project_root.join("packs/typo/package.yml"), "owner: Typo\n")?; fs::write(project_root.join("packs/typo/app/thing.rb"), "puts 'typo'\n")?; Config { owned_globs: &["{app,config,packs}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*", "packs/typo/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/typo/**/*"], ..Default::default() } .write(project_root) } -fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_javascript_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("frontend/typo/src"))?; fs::write( project_root.join("frontend/typo/package.json"), @@ -362,18 +364,21 @@ fn add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob(project_ro fs::write(project_root.join("frontend/typo/src/thing.js"), "console.log('typo');\n")?; Config { owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], - allow_unowned_globs: &["app/deprecated/**/*", "frontend/typo/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "frontend/typo/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_in_a_package_naming_an_unknown_team() -> Result<(), Box> { let packages: [(FixtureChange, &str); 2] = [ - (add_a_package_naming_an_unknown_team_in_an_allowed_glob, "packs/typo/app/thing.rb"), ( - add_a_javascript_package_naming_an_unknown_team_in_an_allowed_glob, + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob, + "packs/typo/app/thing.rb", + ), + ( + add_a_javascript_package_naming_an_unknown_team_in_an_optionally_owned_glob, "frontend/typo/src/thing.js", ), ]; @@ -389,23 +394,23 @@ fn test_validate_files_reports_an_allowed_file_in_a_package_naming_an_unknown_te } // `packs/typo` is a string prefix of `packs/typo2` but not a path prefix, so the package names no owner for the file there. -fn add_an_allowed_unowned_file_in_a_sibling_of_a_package_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { - add_a_package_naming_an_unknown_team_in_an_allowed_glob(project_root)?; +fn add_an_optionally_owned_file_with_no_owner_in_a_sibling_of_a_package_naming_an_unknown_team(project_root: &Path) -> std::io::Result<()> { + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; fs::create_dir_all(project_root.join("packs/typo2/app"))?; fs::write(project_root.join("packs/typo2/app/thing.rb"), "puts 'no owner'\n")?; Config { owned_globs: &["{app,config,packs}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_passes_an_allowed_file_in_a_sibling_of_a_package_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_passes_an_optionally_owned_file_in_a_sibling_of_a_package_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_an_allowed_unowned_file_in_a_sibling_of_a_package_naming_an_unknown_team, + add_an_optionally_owned_file_with_no_owner_in_a_sibling_of_a_package_naming_an_unknown_team, &[command, "packs/typo2/app/thing.rb"], )? .success() @@ -415,7 +420,7 @@ fn test_validate_files_passes_an_allowed_file_in_a_sibling_of_a_package_naming_a } // Read as a glob, `packs/[typo]` would match `packs/t` rather than the package's own directory. -fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/[typo]/app"))?; fs::write(project_root.join("packs/[typo]/package.yml"), "owner: Typo\n")?; fs::write(project_root.join("packs/[typo]/app/thing.rb"), "puts 'typo'\n")?; @@ -423,17 +428,17 @@ fn add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob(project_roo fs::write(project_root.join("packs/t/stray.rb"), "puts 'no owner'\n")?; Config { owned_globs: &["{app,config,packs}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_reports_an_allowed_file_in_a_bracketed_package_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_in_a_bracketed_package_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob, + add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "packs/[typo]/app/thing.rb"], )? .failure() @@ -444,10 +449,10 @@ fn test_validate_files_reports_an_allowed_file_in_a_bracketed_package_naming_an_ } #[test] -fn test_validate_files_passes_an_allowed_file_outside_a_package_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_passes_an_optionally_owned_file_outside_a_package_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_bracketed_package_naming_an_unknown_team_in_an_allowed_glob, + add_a_bracketed_package_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "packs/t/stray.rb", "app/deprecated/old.rb"], )? .success() @@ -457,23 +462,23 @@ fn test_validate_files_passes_an_allowed_file_outside_a_package_naming_an_unknow } // Read as a glob, `packs/{typo}` would match `packs/typo` rather than the package's own directory. -fn add_a_braced_package_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_braced_package_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::create_dir_all(project_root.join("packs/{typo}/app"))?; fs::write(project_root.join("packs/{typo}/package.yml"), "owner: Typo\n")?; fs::write(project_root.join("packs/{typo}/app/thing.rb"), "puts 'typo'\n")?; Config { owned_globs: &["{app,config,packs}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*", "packs/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_reports_an_allowed_file_in_a_braced_package_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_in_a_braced_package_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_braced_package_naming_an_unknown_team_in_an_allowed_glob, + add_a_braced_package_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "packs/{typo}/app/thing.rb"], )? .failure() @@ -483,15 +488,15 @@ fn test_validate_files_reports_an_allowed_file_in_a_braced_package_naming_an_unk Ok(()) } -fn add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +fn add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("app/deprecated/.codeowner"), "Typo\n") } #[test] -fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/old.rb"], )? .failure() @@ -502,16 +507,19 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_naming_an_unkno } // A glob built from the project root, `/**/**`, would match no project-relative path. -fn allow_every_file_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root(project_root: &Path) -> std::io::Result<()> { +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<()> { fs::write(project_root.join(".codeowner"), "Typo\n")?; - allow_every_file(project_root) + make_every_file_optionally_owned(project_root) } #[test] -fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_under_a_codeowner_at_the_root_naming_an_unknown_team() -> Result<(), Box> +{ for command in ["validate", "gv"] { run_on_modified_fixture( - allow_every_file_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root, + make_every_file_optionally_owned_and_put_a_codeowner_file_naming_an_unknown_team_at_the_project_root, &[command, "app/deprecated/old.rb"], )? .failure() @@ -522,7 +530,7 @@ fn test_validate_files_reports_an_allowed_file_under_a_codeowner_at_the_root_nam } // Read as a glob, `app/deprecated/{legacy}` would match `app/deprecated/legacy` rather than its own directory. -fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root: &Path) -> std::io::Result<()> { +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")?; fs::write(project_root.join("app/deprecated/{legacy}/thing.rb"), "puts 'typo'\n")?; @@ -531,17 +539,17 @@ fn add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project } #[test] -fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_an_unknown_team() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_under_a_braced_codeowner_naming_an_unknown_team() -> Result<(), Box> { for command in ["validate", "gv"] { run_on_modified_fixture( - add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/{legacy}/thing.rb"], )? .failure() .stdout(predicate::str::contains(unlisted_owner_heading(command))) .stdout(predicate::str::contains("app/deprecated/{legacy}/thing.rb")); run_on_modified_fixture( - add_a_braced_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + add_a_braced_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, &[command, "app/deprecated/legacy/thing.rb", "app/deprecated/old.rb"], )? .success() @@ -551,13 +559,16 @@ fn test_validate_files_reports_an_allowed_file_under_a_braced_codeowner_naming_a } #[test] -fn test_validate_reports_a_package_or_codeowner_naming_an_unknown_team_in_allowed_globs() -> Result<(), Box> { - // The package mappers drop the package, leaving its file with no owner, but the file is allowed. The `.codeowner` +fn test_validate_reports_a_package_or_codeowner_naming_an_unknown_team_in_optionally_owned_globs() -> Result<(), Box> { + // The package mappers drop the package, leaving its file with no owner, but the file is optionally owned. The `.codeowner` // gives annotated_old.rb a second owner, so `validate` would fail even if the `.codeowner` itself went unreported. let files: [(FixtureChange, &str); 2] = [ - (add_a_package_naming_an_unknown_team_in_an_allowed_glob, "packs/typo/package.yml"), ( - add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob, + add_a_package_naming_an_unknown_team_in_an_optionally_owned_glob, + "packs/typo/package.yml", + ), + ( + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob, "app/deprecated/.codeowner", ), ]; @@ -595,7 +606,7 @@ fn add_a_package_without_regenerating_codeowners(project_root: &Path) -> std::io fs::write(project_root.join("packs/foo/app/thing.rb"), "puts 'foo'\n")?; Config { owned_globs: &["{app,config,packs}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*", "packs/foo/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "packs/foo/**/*"], ..Default::default() } .write(project_root) @@ -610,14 +621,14 @@ fn add_a_javascript_package_without_regenerating_codeowners(project_root: &Path) fs::write(project_root.join("frontend/foo/src/thing.js"), "console.log('foo');\n")?; Config { owned_globs: &["{app,config,frontend}/**/*.{rb,js}"], - allow_unowned_globs: &["app/deprecated/**/*", "frontend/foo/**/*"], + optionally_owned_globs: &["app/deprecated/**/*", "frontend/foo/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { +fn test_validate_files_reports_an_optionally_owned_file_missing_from_codeowners() -> Result<(), Box> { // `validate`, not `gv`, which would add each file to CODEOWNERS. let owners: [(FixtureChange, &str); 5] = [ (annotate_the_unowned_file_without_regenerating_codeowners, "app/deprecated/old.rb"), @@ -644,7 +655,7 @@ fn test_validate_files_reports_an_allowed_file_missing_from_codeowners() -> Resu fn name_an_executable(project_root: &Path) -> std::io::Result<()> { Config { owned_globs: &["{app,config}/**/*.rb"], - allow_unowned_globs: &["app/deprecated/**/*"], + optionally_owned_globs: &["app/deprecated/**/*"], executable_name: Some("bin/codeownership validate"), ..Default::default() } @@ -657,7 +668,8 @@ fn annotate_the_unowned_file_and_name_an_executable(project_root: &Path) -> std: } #[test] -fn test_validate_files_suggests_the_configured_executable_for_an_allowed_file_missing_from_codeowners() -> Result<(), Box> { +fn test_validate_files_suggests_the_configured_executable_for_an_optionally_owned_file_missing_from_codeowners() +-> Result<(), Box> { run_on_modified_fixture( annotate_the_unowned_file_and_name_an_executable, &["validate", "app/deprecated/old.rb"], @@ -669,8 +681,8 @@ fn test_validate_files_suggests_the_configured_executable_for_an_allowed_file_mi Ok(()) } -fn annotate_an_allowed_file_with_an_unknown_team_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; +fn annotate_an_optionally_owned_file_with_an_unknown_team_and_name_an_executable(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; name_an_executable(project_root) } @@ -678,7 +690,7 @@ fn annotate_an_allowed_file_with_an_unknown_team_and_name_an_executable(project_ fn test_generate_and_validate_files_does_not_suggest_the_configured_executable() -> Result<(), Box> { // `gv` has just written CODEOWNERS, so running the executable to update it can't help. run_on_modified_fixture( - annotate_an_allowed_file_with_an_unknown_team_and_name_an_executable, + annotate_an_optionally_owned_file_with_an_unknown_team_and_name_an_executable, &["gv", "app/deprecated/typo.rb"], )? .failure() @@ -687,30 +699,30 @@ fn test_generate_and_validate_files_does_not_suggest_the_configured_executable() Ok(()) } -fn put_the_allowed_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { - add_an_annotation_naming_an_unknown_team_in_an_allowed_glob(project_root)?; +fn put_the_optionally_owned_glob_in_unowned_globs_too(project_root: &Path) -> std::io::Result<()> { + add_an_annotation_naming_an_unknown_team_in_an_optionally_owned_glob(project_root)?; Config { owned_globs: &["{app,config}/**/*.rb"], unowned_globs: &["app/deprecated/**/*"], - allow_unowned_globs: &["app/deprecated/**/*"], + optionally_owned_globs: &["app/deprecated/**/*"], ..Default::default() } .write(project_root) } #[test] -fn test_unowned_globs_take_precedence_over_allow_unowned_globs() -> Result<(), Box> { +fn test_unowned_globs_take_precedence_over_optionally_owned_globs() -> Result<(), Box> { // A file in both is left out, so the unknown team its annotation names doesn't count. `gv`, not `validate`, because // leaving files out changes CODEOWNERS. - run_on_modified_fixture(put_the_allowed_glob_in_unowned_globs_too, &["gv"])? + run_on_modified_fixture(put_the_optionally_owned_glob_in_unowned_globs_too, &["gv"])? .success() .stdout(predicate::eq("")); Ok(()) } fn put_a_codeowner_file_naming_an_unknown_team_in_both_globs(project_root: &Path) -> std::io::Result<()> { - put_the_allowed_glob_in_unowned_globs_too(project_root)?; - add_a_codeowner_file_naming_an_unknown_team_in_an_allowed_glob(project_root) + put_the_optionally_owned_glob_in_unowned_globs_too(project_root)?; + add_a_codeowner_file_naming_an_unknown_team_in_an_optionally_owned_glob(project_root) } #[test] @@ -728,7 +740,7 @@ fn test_validate_files_leaves_out_files_in_both_globs() -> Result<(), Box Result<(), Box> { +fn test_validate_reports_unowned_files_without_optionally_owned_globs() -> Result<(), Box> { run_on_modified_fixture( |project_root| { Config { @@ -745,18 +757,24 @@ fn test_validate_reports_unowned_files_without_allowed_globs() -> Result<(), Box Ok(()) } -fn allow_every_unowned_file_with_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { +fn make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root(project_root: &Path) -> std::io::Result<()> { fs::write(project_root.join("root.rb"), "puts 'no owner'\n")?; - allow_every_file(project_root) + make_every_file_optionally_owned(project_root) } #[test] fn test_double_star_glob_allows_unowned_files_at_the_project_root() -> Result<(), Box> { - run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate"])? - .success() - .stdout(predicate::eq("")); - run_on_modified_fixture(allow_every_unowned_file_with_one_at_the_project_root, &["validate", "root.rb"])? - .success() - .stdout(predicate::eq("")); + run_on_modified_fixture( + make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root, + &["validate"], + )? + .success() + .stdout(predicate::eq("")); + run_on_modified_fixture( + make_every_file_optionally_owned_with_an_unowned_one_at_the_project_root, + &["validate", "root.rb"], + )? + .success() + .stdout(predicate::eq("")); Ok(()) } From 8f76d206d0baaaf3231021b6eae241e4eb0d6d4e Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Thu, 22 Oct 2026 11:26:19 -0700 Subject: [PATCH 38/38] Stop at the first owner in OwnerLookup::has_owner has_owner only needs to know whether anything names an owner, but it called FileOwnerFinder::find, which runs every owner matcher, clones each matching source into a map of teams and collects the owners, all to check that the list isn't empty. With `optionally_owned_globs: ['**/*']`, every file `validate ` is given that has no line in CODEOWNERS reaches it. find returns an owner exactly when some matcher names a team: a matched directory always survives as the most specific one. So has_owner now asks whether any matcher names a team, and stops at the first. Fresh Eyes reported this as an info-level performance finding. --- src/ownership.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index e076fe5..9cefbd7 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -121,10 +121,12 @@ pub(crate) struct OwnerLookup { impl OwnerLookup { pub(crate) fn has_owner(&self, relative_path: &Path) -> bool { - let file_owner_finder = FileOwnerFinder { - owner_matchers: &self.owner_matchers, - }; - self.unknown_team_roots.iter().any(|root| relative_path.starts_with(root)) || !file_owner_finder.find(relative_path).is_empty() + // FileOwnerFinder::find returns an owner exactly when some matcher names a team, but builds every owner to do it. + self.unknown_team_roots.iter().any(|root| relative_path.starts_with(root)) + || self + .owner_matchers + .iter() + .any(|owner_matcher| owner_matcher.owner_for(relative_path).0.is_some()) } }