From 35829373f348075cb6828ea60db5206c70dabc83 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 8 Jun 2026 11:21:22 -0700 Subject: [PATCH 1/2] feat: show CODEOWNERS diff when validate detects a stale file `validate` previously reported only "CODEOWNERS out of date" with no indication of what differed, forcing developers to re-run the tool locally to discover the delta. The stale-file error now includes a line-oriented diff between the on-disk CODEOWNERS file and the freshly generated one (git-style `-`/`+` prefixes, changed lines only), so CI output explains what is out of date. --- Cargo.lock | 7 ++ Cargo.toml | 1 + src/ownership/validator.rs | 92 ++++++++++++++++---- tests/executable_name_config_test.rs | 33 ++++++- tests/invalid_project_test.rs | 30 +++++++ tests/run_config_executable_override_test.rs | 5 +- 6 files changed, 148 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6dfce14..f944b14 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -203,6 +203,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "similar", "tempfile", "tracing", "tracing-subscriber", @@ -868,6 +869,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "similar" +version = "2.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbbb5d9659141646ae647b42fe094daf6c6192d1620870b449d9557f748b2daa" + [[package]] name = "smallvec" version = "1.10.0" diff --git a/Cargo.toml b/Cargo.toml index 13916f7..dd093c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ regex = "1.11.1" serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.143" serde_yaml = "0.9.34" +similar = "2.6.0" tempfile = "3.21.0" tracing = "0.1.41" tracing-subscriber = { version = "0.3.19", features = ["env-filter"] } diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 16916c5..1ff1f79 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -9,6 +9,7 @@ use error_stack::Context; use itertools::Itertools; use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; +use similar::{ChangeTag, TextDiff}; use tracing::debug; use tracing::instrument; @@ -29,7 +30,7 @@ enum Error { InvalidTeam { name: String, path: PathBuf }, FileWithoutOwner { path: PathBuf }, FileWithMultipleOwners { path: PathBuf, owners: Vec }, - CodeownershipFileIsStale { executable_name: String }, + CodeownershipFileIsStale { executable_name: String, diff: String }, } #[derive(Debug)] @@ -127,20 +128,15 @@ impl Validator { fn validate_codeowners_file(&self) -> Vec { let generated_file = self.file_generator.generate_file(); + let current_file = self.project.get_codeowners_file().unwrap_or_default(); - match self.project.get_codeowners_file() { - Ok(current_file) => { - if generated_file != current_file { - vec![Error::CodeownershipFileIsStale { - executable_name: self.executable_name.to_string(), - }] - } else { - vec![] - } - } - Err(_) => vec![Error::CodeownershipFileIsStale { + if generated_file == current_file { + vec![] + } else { + vec![Error::CodeownershipFileIsStale { executable_name: self.executable_name.to_string(), - }], + diff: codeowners_diff(¤t_file, &generated_file), + }] } } @@ -163,12 +159,32 @@ impl Validator { } } +/// Builds a line-oriented diff between the current (on-disk) CODEOWNERS file and the +/// freshly generated one, so that validation failures explain *what* is out of date +/// rather than just *that* it is. Only changed lines are emitted: removals (present +/// on disk but no longer expected) are prefixed with `-` and additions (expected but +/// missing) are prefixed with `+`. +fn codeowners_diff(current: &str, generated: &str) -> String { + let diff = TextDiff::from_lines(current, generated); + + diff.iter_all_changes() + .filter_map(|change| { + let line = change.value().trim_end_matches('\n'); + match change.tag() { + ChangeTag::Delete => Some(format!("-{line}")), + ChangeTag::Insert => Some(format!("+{line}")), + ChangeTag::Equal => None, + } + }) + .join("\n") +} + impl Error { pub fn category(&self) -> String { match self { Error::FileWithoutOwner { path: _ } => "Some files are missing ownership".to_owned(), Error::FileWithMultipleOwners { path: _, owners: _ } => "Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways".to_owned(), - Error::CodeownershipFileIsStale { executable_name } => { + 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(), @@ -192,7 +208,13 @@ impl Error { vec![messages.join("\n")] } - Error::CodeownershipFileIsStale { executable_name: _ } => vec![], + Error::CodeownershipFileIsStale { executable_name: _, diff } => { + if diff.is_empty() { + vec![] + } else { + vec![format!("The following changes are required (- current, + expected):\n{diff}")] + } + } Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)], } } @@ -221,3 +243,43 @@ impl Display for Errors { } impl Context for Errors {} + +#[cfg(test)] +mod tests { + use super::*; + use indoc::indoc; + + #[test] + fn test_codeowners_diff_reports_added_and_removed_lines() { + let current = indoc! {" + # Team A + /app/a.rb @TeamA + /app/old.rb @TeamA + "}; + let generated = indoc! {" + # Team A + /app/a.rb @TeamA + /app/b.rb @TeamB + "}; + + let diff = codeowners_diff(current, generated); + + assert_eq!(diff, "-/app/old.rb @TeamA\n+/app/b.rb @TeamB"); + } + + #[test] + fn test_codeowners_diff_against_empty_file_is_all_additions() { + let generated = "# Team A\n/app/a.rb @TeamA\n"; + + let diff = codeowners_diff("", generated); + + assert_eq!(diff, "+# Team A\n+/app/a.rb @TeamA"); + } + + #[test] + fn test_codeowners_diff_is_empty_when_identical() { + let file = "# Team A\n/app/a.rb @TeamA\n"; + + assert_eq!(codeowners_diff(file, file), ""); + } +} diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index 5832eb8..5c11c59 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -40,11 +40,26 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box &["validate"], false, OutputStream::Stdout, - predicate::eq(indoc! {" + predicate::eq(indoc! {r#" CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file + The following changes are required (- current, + expected): + -# This file was automatically generated by "codeowners". + +# This file was automatically generated by "bin/codeownership validate". + -# 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 - "}), + "#}), )?; Ok(()) } @@ -57,11 +72,21 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box> { predicate::eq(indoc! {" CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file + The following changes are required (- current, + expected): + +# 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 + + + +# 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 + + + +# Owner in .codeowner + +/ruby/app/services/**/** @PayrollTeam + + + +# Owner metadata key in package.yml + +/ruby/packages/payroll_flow/**/** @PayrollTeam + + + +# Team YML ownership + +/config/teams/payments.yml @PaymentTeam + +/config/teams/payroll.yml @PayrollTeam + + + +# Team owned gems + +/gems/payroll_calculator/**/** @PayrollTeam Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways diff --git a/tests/run_config_executable_override_test.rs b/tests/run_config_executable_override_test.rs index 29074b5..f76180c 100644 --- a/tests/run_config_executable_override_test.rs +++ b/tests/run_config_executable_override_test.rs @@ -29,8 +29,11 @@ fn test_run_config_executable_path_overrides_config_file() -> Result<(), Box Date: Mon, 8 Jun 2026 11:39:00 -0700 Subject: [PATCH 2/2] test: fix stale disclaimer header in validate fixtures The custom_executable_name and default_executable_name CODEOWNERS fixtures carried an outdated disclaimer header (`generated by "codeowners"`) that no longer matches the generator's output (`generated by "bin/codeownership validate"`). That mismatch showed up as noise in the validate diff. Update the fixture headers to match the generated disclaimer so the diff only reflects real ownership drift, and restore the stricter executable-override assertion now that the diff no longer contains "bin/codeownership". --- tests/executable_name_config_test.rs | 4 ---- tests/fixtures/custom_executable_name/.github/CODEOWNERS | 2 +- tests/fixtures/default_executable_name/.github/CODEOWNERS | 2 +- tests/run_config_executable_override_test.rs | 5 +---- 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index 5c11c59..e71da1d 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -44,8 +44,6 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file The following changes are required (- current, + expected): - -# This file was automatically generated by "codeowners". - +# This file was automatically generated by "bin/codeownership validate". -# Outdated content to trigger validation error -/app/old.rb @FooTeam + @@ -76,8 +74,6 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box