From 89aa4a9765b1918418ffa112655592696eb3ff5b Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Thu, 24 Sep 2026 17:34:39 -0600 Subject: [PATCH 1/2] fix: write CODEOWNERS annotations after glob-based sections GitHub applies the last matching CODEOWNERS line, but the generator wrote the file-annotation section first. A file that a team glob excludes via unowned_globs and an annotation assigns to another team is valid, and for-file resolves it to the annotated team, yet the broader team glob came later in CODEOWNERS and won on GitHub (and in --from-codeowners lookups). Write the annotations section last. Resolution and validation keep their existing mapper order; only the generated file changes. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/ownership.rs | 22 +++++++++- tests/annotation_in_unowned_team_glob_test.rs | 44 +++++++++++++++++++ tests/executable_name_config_test.rs | 12 ++--- .../.github/CODEOWNERS | 18 ++++++++ .../config/code_ownership.yml | 4 ++ .../config/teams/alpha.yml | 7 +++ .../config/teams/beta.yml | 3 ++ .../ruby/app/services/alpha_owned.rb | 2 + .../ruby/app/services/beta_owned.rb | 4 ++ .../custom_codeowners_path/docs/CODEOWNERS | 6 +-- .../expected/CODEOWNERS | 6 +-- .../fixtures/valid_project/.github/CODEOWNERS | 18 ++++---- .../.github/CODEOWNERS | 24 +++++----- tests/invalid_project_test.rs | 12 ++--- tests/valid_project_test.rs | 12 ++--- 15 files changed, 147 insertions(+), 47 deletions(-) create mode 100644 tests/annotation_in_unowned_team_glob_test.rs create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/.github/CODEOWNERS create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/config/code_ownership.yml create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/config/teams/alpha.yml create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/config/teams/beta.yml create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/ruby/app/services/alpha_owned.rb create mode 100644 tests/fixtures/annotation_in_unowned_team_glob/ruby/app/services/beta_owned.rb diff --git a/src/ownership.rs b/src/ownership.rs index bbd2099..c0422e4 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -122,7 +122,9 @@ impl Ownership { let validator = Validator { project: self.project.clone(), mappers: self.mappers(), - file_generator: FileGenerator { mappers: self.mappers() }, + file_generator: FileGenerator { + mappers: self.codeowners_file_mappers(), + }, executable_name: self.project.executable_name.clone(), }; @@ -166,7 +168,9 @@ impl Ownership { #[instrument(level = "debug", skip_all)] pub fn generate_file(&self) -> String { info!("generating codeowners file"); - let file_generator = FileGenerator { mappers: self.mappers() }; + let file_generator = FileGenerator { + mappers: self.codeowners_file_mappers(), + }; file_generator.generate_file() } @@ -182,6 +186,20 @@ impl Ownership { Box::new(TeamGemMapper::build(self.project.clone())), ] } + + // GitHub applies the last matching CODEOWNERS line, so annotations are written last: an annotated + // file can still match a broader team glob that excludes it through unowned_globs. + fn codeowners_file_mappers(&self) -> Vec> { + vec![ + Box::new(TeamGlobMapper::build(self.project.clone())), + Box::new(DirectoryMapper::build(self.project.clone())), + Box::new(RubyPackageMapper::build(self.project.clone())), + Box::new(JavascriptPackageMapper::build(self.project.clone())), + Box::new(TeamYmlMapper::build(self.project.clone())), + Box::new(TeamGemMapper::build(self.project.clone())), + Box::new(TeamFileMapper::build(self.project.clone())), + ] + } } #[cfg(test)] diff --git a/tests/annotation_in_unowned_team_glob_test.rs b/tests/annotation_in_unowned_team_glob_test.rs new file mode 100644 index 0000000..b064625 --- /dev/null +++ b/tests/annotation_in_unowned_team_glob_test.rs @@ -0,0 +1,44 @@ +use indoc::indoc; +use predicates::prelude::*; +use std::error::Error; + +mod common; +use common::OutputStream; +use common::run_codeowners; + +// Alpha owns ruby/app/**/* but excludes beta_owned.rb through unowned_globs, and Beta owns that file +// through an annotation. GitHub applies the last matching CODEOWNERS line, so the annotation line must +// come after Alpha's broader glob for GitHub to agree with for-file. +const FIXTURE: &str = "annotation_in_unowned_team_glob"; + +#[test] +fn test_validate_accepts_generated_order() -> Result<(), Box> { + run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; + Ok(()) +} + +#[test] +fn test_codeowners_file_agrees_with_for_file() -> Result<(), Box> { + run_codeowners( + FIXTURE, + &["crosscheck-owners"], + true, + OutputStream::Stdout, + predicate::eq(indoc! {" + Success! All files match between CODEOWNERS and for-file command. + "}), + )?; + Ok(()) +} + +#[test] +fn test_for_file_from_codeowners_returns_annotated_owner() -> Result<(), Box> { + run_codeowners( + FIXTURE, + &["for-file", "--from-codeowners", "ruby/app/services/beta_owned.rb"], + true, + OutputStream::Stdout, + predicate::str::contains("Team: Beta"), + )?; + Ok(()) +} diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index 4dc135d..3ad64b5 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -45,15 +45,15 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box -# Outdated content to trigger validation error -/app/old.rb @FooTeam + - +# Annotations at the top of file - +/app/foo.rb @FooTeam - + +# Team-specific owned globs +/ruby/app/payments/** @PaymentTeam + +# Team YML ownership +/config/teams/foo.yml @FooTeam +/config/teams/payments.yml @PaymentTeam + + + +# Annotations at the top of file + +/app/foo.rb @FooTeam CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file @@ -74,11 +74,11 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box> { +# code/file owner is notified. Reference GitHub docs for more details: +# https://help.github.com/en/articles/about-code-owners + - +# Annotations at the top of file - +/gems/payroll_calculator/calculator.rb @PaymentTeam - +/ruby/app/models/bank_account.rb @PaymentTeam - +/ruby/app/models/payroll.rb @PayrollTeam - +/ruby/app/services/multi_owned.rb @PaymentTeam - + +# Team-specific owned globs +/ruby/app/payments/**/* @PaymentTeam + @@ -44,6 +38,12 @@ fn test_validate() -> Result<(), Box> { + +# Team owned gems +/gems/payroll_calculator/**/** @PayrollTeam + + + +# Annotations at the top of file + +/gems/payroll_calculator/calculator.rb @PaymentTeam + +/ruby/app/models/bank_account.rb @PaymentTeam + +/ruby/app/models/payroll.rb @PayrollTeam + +/ruby/app/services/multi_owned.rb @PaymentTeam CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index f207fac..33d40b0 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -299,12 +299,6 @@ fn test_for_team() -> Result<(), Box> { predicate::eq(indoc! {" # Code Ownership Report for `Payroll` Team - ## Annotations at the top of file - /javascript/packages/PayrollFlow/index.tsx - /ruby/app/models/payroll.rb - /ruby/app/views/foos/edit.erb - /ruby/app/views/foos/new.html.erb - ## Team-specific owned globs This team owns nothing in this category. @@ -324,6 +318,12 @@ fn test_for_team() -> Result<(), Box> { ## Team owned gems /gems/payroll_calculator/**/** + + ## Annotations at the top of file + /javascript/packages/PayrollFlow/index.tsx + /ruby/app/models/payroll.rb + /ruby/app/views/foos/edit.erb + /ruby/app/views/foos/new.html.erb "}), )?; From 314ba94822cecff30d409e4b70e2e757b457de44 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Fri, 25 Sep 2026 13:16:37 -0600 Subject: [PATCH 2/2] test: regenerate CODEOWNERS before checking annotation order The crosscheck and for-file tests read the committed fixture CODEOWNERS, which is already in the fixed order, so they passed without the generator change. Regenerate into the temp copy first so all three tests fail on the old order. --- tests/annotation_in_unowned_team_glob_test.rs | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/tests/annotation_in_unowned_team_glob_test.rs b/tests/annotation_in_unowned_team_glob_test.rs index b064625..3ae70b5 100644 --- a/tests/annotation_in_unowned_team_glob_test.rs +++ b/tests/annotation_in_unowned_team_glob_test.rs @@ -1,16 +1,37 @@ +use assert_cmd::prelude::*; use indoc::indoc; use predicates::prelude::*; use std::error::Error; +use std::path::Path; +use std::process::Command; +use tempfile::TempDir; mod common; use common::OutputStream; +use common::git_add_all_files; use common::run_codeowners; +use common::setup_fixture_repo; // Alpha owns ruby/app/**/* but excludes beta_owned.rb through unowned_globs, and Beta owns that file // through an annotation. GitHub applies the last matching CODEOWNERS line, so the annotation line must // come after Alpha's broader glob for GitHub to agree with for-file. const FIXTURE: &str = "annotation_in_unowned_team_glob"; +// Copies the fixture and regenerates its CODEOWNERS, so assertions exercise the generator rather than +// the committed file. +fn fixture_with_generated_codeowners() -> Result> { + let temp_dir = setup_fixture_repo(&Path::new("tests/fixtures").join(FIXTURE)); + git_add_all_files(temp_dir.path()); + codeowners(temp_dir.path(), &["generate"])?.assert().success(); + Ok(temp_dir) +} + +fn codeowners(project_root: &Path, args: &[&str]) -> Result> { + let mut cmd = Command::cargo_bin("codeowners")?; + cmd.arg("--project-root").arg(project_root).arg("--no-cache").args(args); + Ok(cmd) +} + #[test] fn test_validate_accepts_generated_order() -> Result<(), Box> { run_codeowners(FIXTURE, &["validate"], true, OutputStream::Stdout, predicate::eq(""))?; @@ -18,27 +39,26 @@ fn test_validate_accepts_generated_order() -> Result<(), Box> { } #[test] -fn test_codeowners_file_agrees_with_for_file() -> Result<(), Box> { - run_codeowners( - FIXTURE, - &["crosscheck-owners"], - true, - OutputStream::Stdout, - predicate::eq(indoc! {" +fn test_generated_codeowners_agrees_with_for_file() -> Result<(), Box> { + let temp_dir = fixture_with_generated_codeowners()?; + codeowners(temp_dir.path(), &["crosscheck-owners"])? + .assert() + .success() + .stdout(predicate::eq(indoc! {" Success! All files match between CODEOWNERS and for-file command. - "}), - )?; + "})); Ok(()) } #[test] -fn test_for_file_from_codeowners_returns_annotated_owner() -> Result<(), Box> { - run_codeowners( - FIXTURE, +fn test_for_file_from_generated_codeowners_returns_annotated_owner() -> Result<(), Box> { + let temp_dir = fixture_with_generated_codeowners()?; + codeowners( + temp_dir.path(), &["for-file", "--from-codeowners", "ruby/app/services/beta_owned.rb"], - true, - OutputStream::Stdout, - predicate::str::contains("Team: Beta"), - )?; + )? + .assert() + .success() + .stdout(predicate::str::contains("Team: Beta")); Ok(()) }