From d7ea0161f2aff2dd5cc60e71d839afaf0d6d890f Mon Sep 17 00:00:00 2001 From: Sara Strasner Date: Thu, 13 Aug 2026 10:08:57 -0400 Subject: [PATCH 1/2] fix: report .codeowner files that reference an unregistered team `DirectoryMapper::entries` looks each directory owner up in the team registry and skips the entry when the name does not resolve. Nothing else reports the name, so a typo'd or renamed team in a `.codeowner` is completely silent: the directory inherits the nearest ancestor owner, `generate` emits no line for it, and `validate` exits 0. That makes the file inert while still looking authoritative, and the ownership it was written to express quietly belongs to whichever team owns the parent directory. Annotations and package ownership are already validated against the registry; this extends the same check to directory ownership, reusing the existing `InvalidTeam` error so output and exit codes are unchanged in shape. Co-Authored-By: Claude Sonnet 4.6 --- src/ownership/validator.rs | 15 +++++++++++ .../.github/CODEOWNERS | 14 +++++++++++ .../app/services/.codeowner | 1 + .../app/services/nested/.codeowner | 1 + .../app/services/nested/nested_file.rb | 2 ++ .../config/code_ownership.yml | 10 ++++++++ .../config/teams/foo.yml | 5 ++++ tests/invalid_directory_codeowner_test.rs | 25 +++++++++++++++++++ 8 files changed, 73 insertions(+) create mode 100644 tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS create mode 100644 tests/fixtures/invalid-directory-codeowner/app/services/.codeowner create mode 100644 tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner create mode 100644 tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb create mode 100644 tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml create mode 100644 tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml create mode 100644 tests/invalid_directory_codeowner_test.rs diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 664de3a..39509d9 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -65,6 +65,7 @@ impl Validator { errors.append(&mut self.invalid_team_annotation(&team_names)); errors.append(&mut self.invalid_package_ownership(&team_names)); + errors.append(&mut self.invalid_directory_ownership(&team_names)); errors } @@ -107,6 +108,20 @@ impl Validator { .collect() } + /// `DirectoryMapper::entries` skips unresolvable owners, so the directory silently + /// inherits its ancestor's owner and nothing else reports the bad name. + fn invalid_directory_ownership(&self, team_names: &HashSet<&TeamName>) -> Vec { + self.project + .directory_codeowner_files + .iter() + .filter(|directory_codeowner_file| !team_names.contains(&directory_codeowner_file.owner)) + .map(|directory_codeowner_file| Error::InvalidTeam { + name: directory_codeowner_file.owner.clone(), + path: self.project.relative_path(&directory_codeowner_file.path).to_owned(), + }) + .collect() + } + fn validate_file_ownership(&self) -> Vec { let mut validation_errors = Vec::new(); diff --git a/tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS b/tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS new file mode 100644 index 0000000..fed28cf --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/.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 + + +# Owner in .codeowner +/app/services/**/** @footeam + +# Team YML ownership +/config/teams/foo.yml @footeam diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner b/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner new file mode 100644 index 0000000..bc56c4d --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/.codeowner @@ -0,0 +1 @@ +Foo diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner b/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner new file mode 100644 index 0000000..c2075a5 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner @@ -0,0 +1 @@ +Web3 diff --git a/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb b/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb new file mode 100644 index 0000000..d19bcc1 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb @@ -0,0 +1,2 @@ +class NestedFile +end diff --git a/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml b/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml new file mode 100644 index 0000000..c76f028 --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml @@ -0,0 +1,10 @@ +--- +owned_globs: + - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}" +unowned_globs: + - config/code_ownership.yml +javascript_package_paths: + - javascript/packages/** +vendored_gems_path: gems +team_file_glob: + - config/teams/**/*.yml \ No newline at end of file diff --git a/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml b/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml new file mode 100644 index 0000000..7c3977c --- /dev/null +++ b/tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml @@ -0,0 +1,5 @@ +name: Foo +github: + team: "@footeam" + members: + - fooer diff --git a/tests/invalid_directory_codeowner_test.rs b/tests/invalid_directory_codeowner_test.rs new file mode 100644 index 0000000..260db32 --- /dev/null +++ b/tests/invalid_directory_codeowner_test.rs @@ -0,0 +1,25 @@ +use indoc::indoc; +use predicates::prelude::*; +use std::error::Error; + +mod common; +use common::OutputStream; +use common::run_codeowners; + +/// A nested `.codeowner` naming an unregistered team, under one naming a real team: +/// ownership falls through to the ancestor, so nothing else reports the bad name. +#[test] +fn test_validate_reports_directory_codeowner_with_invalid_team() -> Result<(), Box> { + run_codeowners( + "invalid-directory-codeowner", + &["validate"], + false, + OutputStream::Stdout, + predicate::str::contains(indoc! {" + Found invalid team annotations + - app/services/nested/.codeowner is referencing an invalid team - 'Web3' + "}), + )?; + + Ok(()) +} From 2898e216c023f31af50772e36f58b5fe35780750 Mon Sep 17 00:00:00 2001 From: Sara Strasner Date: Mon, 17 Aug 2026 14:17:38 -0400 Subject: [PATCH 2/2] fix: resolve directory owners through teams_by_name, not teams[].name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit README documents `validate` as ensuring "All referenced teams are valid," and `.codeowner` was the one ownership surface where the code didn't keep that promise. The previous commit closed the gap but used the wrong registry. `validate_invalid_team` built its predicate from `project.teams[].name`, while the resolution path goes through `project.teams_by_name` — a map deliberately keyed by both `team.name` and `team.github_team`. A `.codeowner` holding the GitHub handle therefore generated a correct CODEOWNERS line and was simultaneously rejected by validation, reproducing in a new form exactly the trap this work set out to remove: validate red, generate correct, no command that fixes it. Resolving through `teams_by_name` makes the predicate definitionally identical to the mapper's lookup. Upgrade impact: projects whose `.codeowner` files name a registered team by either its name or its GitHub handle are unaffected. Only genuinely unresolvable names now fail, where they previously exited 0. Also splits the directory case into its own error variant so the message can name the ancestor the directory is now silently inheriting from, which is the detail a reader needs to understand what happened. `category()` still matches `InvalidTeam`, so one typo'd team name spread across an annotation, a `package.yml`, and a `.codeowner` groups under a single headline rather than three. That shared headline is renamed to "Found invalid team references", which is accurate for all three surfaces; the earlier claim that the wrapping `code_ownership` gem parses these strings does not hold — it delegates to `::RustCodeOwners.validate` without inspecting output, and `category()` has exactly one consumer, the grouping in `Display for Errors`. Adds a positive test pinning both accepted `.codeowner` forms, so the tolerant side can't regress silently: every pre-existing fixture holds a bare team name, which is why the suite stayed green through the stricter predicate. Co-Authored-By: Claude Sonnet 4.6 --- src/ownership/validator.rs | 77 ++++++++++++++++--- tests/directory_codeowner_github_team_test.rs | 22 ++++++ .../.github/CODEOWNERS | 15 ++++ .../app/by_handle/.codeowner | 1 + .../app/by_handle/handled.rb | 2 + .../app/by_name/.codeowner | 1 + .../app/by_name/named.rb | 2 + .../config/code_ownership.yml | 10 +++ .../config/teams/foo.yml | 5 ++ tests/invalid_directory_codeowner_test.rs | 7 +- tests/invalid_project_test.rs | 2 +- 11 files changed, 128 insertions(+), 16 deletions(-) create mode 100644 tests/directory_codeowner_github_team_test.rs create mode 100644 tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS create mode 100644 tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner create mode 100644 tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb create mode 100644 tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner create mode 100644 tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb create mode 100644 tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml create mode 100644 tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 39509d9..e3888c9 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -2,7 +2,7 @@ use crate::project::{Project, ProjectFile}; use core::fmt; use std::collections::HashSet; use std::fmt::Display; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use error_stack::Context; @@ -27,10 +27,31 @@ pub struct Validator { #[derive(Debug)] enum Error { - InvalidTeam { name: String, path: PathBuf }, - FileWithoutOwner { path: PathBuf }, - FileWithMultipleOwners { path: PathBuf, owners: Vec }, - CodeownershipFileIsStale { executable_name: String, diff: String }, + InvalidTeam { + name: String, + path: PathBuf, + }, + /// A `.codeowner` naming a team that isn't registered. Distinct from `InvalidTeam` only + /// so the message can name the ancestor the directory now silently inherits from, which + /// is the part a reader needs in order to understand what happened. `category()` + /// deliberately matches `InvalidTeam` so one typo'd team name across an annotation, a + /// `package.yml`, and a `.codeowner` still groups under a single headline. + InvalidDirectoryTeam { + name: String, + path: PathBuf, + inherits_from: Option, + }, + FileWithoutOwner { + path: PathBuf, + }, + FileWithMultipleOwners { + path: PathBuf, + owners: Vec, + }, + CodeownershipFileIsStale { + executable_name: String, + diff: String, + }, } #[derive(Debug)] @@ -65,7 +86,7 @@ impl Validator { errors.append(&mut self.invalid_team_annotation(&team_names)); errors.append(&mut self.invalid_package_ownership(&team_names)); - errors.append(&mut self.invalid_directory_ownership(&team_names)); + errors.append(&mut self.invalid_directory_ownership()); errors } @@ -110,14 +131,35 @@ impl Validator { /// `DirectoryMapper::entries` skips unresolvable owners, so the directory silently /// inherits its ancestor's owner and nothing else reports the bad name. - fn invalid_directory_ownership(&self, team_names: &HashSet<&TeamName>) -> Vec { + /// + /// Resolves through `teams_by_name` rather than `teams[].name` so the predicate is + /// identical to the mapper's lookup: that map is keyed by both `name` and + /// `github_team`, and a `.codeowner` holding either one generates a correct line. + fn invalid_directory_ownership(&self) -> Vec { + let resolvable_roots: HashSet<&Path> = 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()) + .collect(); + self.project .directory_codeowner_files .iter() - .filter(|directory_codeowner_file| !team_names.contains(&directory_codeowner_file.owner)) - .map(|directory_codeowner_file| Error::InvalidTeam { - name: directory_codeowner_file.owner.clone(), - path: self.project.relative_path(&directory_codeowner_file.path).to_owned(), + .flat_map(|directory_codeowner_file| { + if !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner) { + Some(Error::InvalidDirectoryTeam { + name: directory_codeowner_file.owner.clone(), + path: self.project.relative_path(&directory_codeowner_file.path).to_owned(), + inherits_from: directory_codeowner_file + .directory_root() + .and_then(|root| root.ancestors().skip(1).find(|ancestor| resolvable_roots.contains(ancestor))) + .map(|ancestor| self.project.relative_path(ancestor).to_owned()), + }) + } else { + None + } }) .collect() } @@ -202,7 +244,8 @@ impl Error { Error::CodeownershipFileIsStale { executable_name, diff: _ } => { format!("CODEOWNERS out of date. Run `{}` to update the CODEOWNERS file", executable_name) } - Error::InvalidTeam { name: _, path: _ } => "Found invalid team annotations".to_owned(), + Error::InvalidTeam { name: _, path: _ } => "Found invalid team references".to_owned(), + Error::InvalidDirectoryTeam { .. } => "Found invalid team references".to_owned(), } } @@ -228,6 +271,16 @@ impl Error { // so that a long diff doesn't bury the actionable headline. Error::CodeownershipFileIsStale { .. } => vec![], Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)], + Error::InvalidDirectoryTeam { name, path, inherits_from } => { + let mut message = format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name); + if let Some(inherits_from) = inherits_from { + message.push_str(&format!( + "; this directory is currently inheriting its owner from {}", + inherits_from.to_string_lossy() + )); + } + vec![message] + } } } } diff --git a/tests/directory_codeowner_github_team_test.rs b/tests/directory_codeowner_github_team_test.rs new file mode 100644 index 0000000..4cbca9b --- /dev/null +++ b/tests/directory_codeowner_github_team_test.rs @@ -0,0 +1,22 @@ +use predicates::prelude::*; +use std::error::Error; + +mod common; +use common::OutputStream; +use common::run_codeowners; + +/// `teams_by_name` is keyed by both `name` and `github_team`, so a `.codeowner` holding +/// either form generates a correct line. Validation has to accept both, or it fails a +/// project whose CODEOWNERS is already right — with no command that fixes it. +#[test] +fn test_validate_accepts_directory_codeowner_by_name_or_github_team() -> Result<(), Box> { + run_codeowners( + "directory-codeowner-github-team", + &["validate"], + true, + OutputStream::Stdout, + predicate::str::contains("invalid team").not(), + )?; + + Ok(()) +} diff --git a/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS b/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS new file mode 100644 index 0000000..a36866e --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS @@ -0,0 +1,15 @@ +# 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 + + +# Owner in .codeowner +/app/by_handle/**/** @footeam +/app/by_name/**/** @footeam + +# Team YML ownership +/config/teams/foo.yml @footeam diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner b/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner new file mode 100644 index 0000000..d556bd1 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner @@ -0,0 +1 @@ +@footeam diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb b/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb new file mode 100644 index 0000000..083e45e --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb @@ -0,0 +1,2 @@ +class Handled +end diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner b/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner new file mode 100644 index 0000000..bc56c4d --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner @@ -0,0 +1 @@ +Foo diff --git a/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb b/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb new file mode 100644 index 0000000..018c0f7 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb @@ -0,0 +1,2 @@ +class Named +end diff --git a/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml b/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml new file mode 100644 index 0000000..c76f028 --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml @@ -0,0 +1,10 @@ +--- +owned_globs: + - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}" +unowned_globs: + - config/code_ownership.yml +javascript_package_paths: + - javascript/packages/** +vendored_gems_path: gems +team_file_glob: + - config/teams/**/*.yml \ No newline at end of file diff --git a/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml b/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml new file mode 100644 index 0000000..7c3977c --- /dev/null +++ b/tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml @@ -0,0 +1,5 @@ +name: Foo +github: + team: "@footeam" + members: + - fooer diff --git a/tests/invalid_directory_codeowner_test.rs b/tests/invalid_directory_codeowner_test.rs index 260db32..f3d1d69 100644 --- a/tests/invalid_directory_codeowner_test.rs +++ b/tests/invalid_directory_codeowner_test.rs @@ -7,7 +7,8 @@ use common::OutputStream; use common::run_codeowners; /// A nested `.codeowner` naming an unregistered team, under one naming a real team: -/// ownership falls through to the ancestor, so nothing else reports the bad name. +/// ownership falls through to the ancestor, so nothing else reports the bad name. The +/// message names that ancestor, since the silent inheritance is the surprising part. #[test] fn test_validate_reports_directory_codeowner_with_invalid_team() -> Result<(), Box> { run_codeowners( @@ -16,8 +17,8 @@ fn test_validate_reports_directory_codeowner_with_invalid_team() -> Result<(), B false, OutputStream::Stdout, predicate::str::contains(indoc! {" - Found invalid team annotations - - app/services/nested/.codeowner is referencing an invalid team - 'Web3' + Found invalid team references + - app/services/nested/.codeowner is referencing an invalid team - 'Web3'; this directory is currently inheriting its owner from app/services "}), )?; diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index 884c5df..3c00a7a 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -61,7 +61,7 @@ fn test_validate() -> Result<(), Box> { owner: Payroll - Owner specified in `ruby/app/services/.codeowner` - Found invalid team annotations + Found invalid team references - ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3' Some files are missing ownership