From e6825203bfda431982c9b9dc3916241fa7f33c2a Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 20 Oct 2025 15:30:16 -0700 Subject: [PATCH] fix: sort CODEOWNERS entries by specificity across all sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensure more specific file patterns override general globs by sorting all entries together instead of grouping by section. Remove section headers from output and handle CODEOWNERS files without headers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/ownership/codeowners_file_parser.rs | 27 ++++++++--- src/ownership/file_generator.rs | 45 +++++++++++++++---- src/ownership/mapper.rs | 1 - src/ownership/mapper/annotated_file_mapper.rs | 4 -- src/ownership/mapper/directory_mapper.rs | 4 -- src/ownership/mapper/package_mapper.rs | 8 ---- src/ownership/mapper/team_gem_mapper.rs | 4 -- src/ownership/mapper/team_glob_mapper.rs | 4 -- src/ownership/mapper/team_yml_mapper.rs | 4 -- .../.github/CODEOWNERS | 3 -- .../.github/CODEOWNERS | 8 ++++ .../fixtures/valid_project/.github/CODEOWNERS | 35 +++++---------- tests/valid_project_test.rs | 27 +++-------- 13 files changed, 85 insertions(+), 89 deletions(-) create mode 100644 tests/fixtures/multiple-directory-owners/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index c40410c..eeff1d5 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -161,6 +161,9 @@ fn codeowner_sections(codeowners_file: &str) -> Result, Box Result { - let section = current_section.as_mut().ok_or(error_message)?; + // If no section header exists, create a default one + if current_section.is_none() { + current_section = Some(TeamOwnership::new("# Owned Files".to_string())); + } + let section = current_section.as_mut().ok_or(error_message)?; let glob = line.split_once(' ').ok_or(error_message)?.0.to_string(); section.globs.push(glob); } @@ -342,10 +349,20 @@ mod tests { /another/owned/path @Foo "}; - let team_ownership = parse_for_team("@Foo".to_string(), codeownership_file); - assert!( - team_ownership - .is_err_and(|e| e.to_string() == "CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file") + let team_ownership = parse_for_team("@Foo".to_string(), codeownership_file)?; + // Now handles files without section headers by creating a default "# Owned Files" section + vecs_match( + &team_ownership, + &vec![ + TeamOwnership { + heading: "# First Section".to_string(), + globs: vec!["/path/to/owned".to_string()], + }, + TeamOwnership { + heading: "# Owned Files".to_string(), + globs: vec!["/another/owned/path".to_string()], + }, + ], ); Ok(()) } diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs index 8878d41..5190e6e 100644 --- a/src/ownership/file_generator.rs +++ b/src/ownership/file_generator.rs @@ -11,17 +11,16 @@ impl FileGenerator { let mut lines: Vec = Vec::new(); lines.append(&mut Self::disclaimer()); + // Collect all entries from all mappers + let mut all_entries: Vec = Vec::new(); for mapper in &self.mappers { - if mapper.entries().is_empty() { - continue; - } - - lines.push(format!("# {}", mapper.name())); - lines.append(&mut Self::to_sorted_lines(&mapper.entries())); - lines.push("".to_owned()); + all_entries.extend(mapper.entries()); } - lines.join("\n") + // Sort all entries together to ensure correct specificity ordering + lines.append(&mut Self::to_sorted_lines(&all_entries)); + + format!("{}\n", lines.join("\n")) } pub fn disclaimer() -> Vec { @@ -200,4 +199,34 @@ mod tests { let sorted = FileGenerator::to_sorted_lines(&entries); assert_eq!(sorted, vec!["/directory/owner-1/** @foo", "/directory/owner_2/** @bar"]); } + + #[test] + fn test_specific_file_overrides_glob_pattern() { + // This tests the case where a specific file should override a general glob pattern. + // In CODEOWNERS, the last matching rule wins, so general patterns must come first. + let entries = vec![ + Entry { + path: "js/packages/zp-constants/src/__generated__/global.ts".to_string(), + github_team: "@Gusto/dev-productivity-web".to_string(), + team_name: "dev-productivity-web".to_string(), + disabled: false, + }, + Entry { + path: "js/packages/zp-constants/**/**".to_string(), + github_team: "@Gusto/dev-productivity-modularity-unowned".to_string(), + team_name: "dev-productivity-modularity-unowned".to_string(), + disabled: false, + }, + ]; + let sorted = FileGenerator::to_sorted_lines(&entries); + // The glob pattern should come FIRST, then the specific file + // This way the specific file rule overrides the general glob + assert_eq!( + sorted, + vec![ + "/js/packages/zp-constants/**/** @Gusto/dev-productivity-modularity-unowned", + "/js/packages/zp-constants/src/__generated__/global.ts @Gusto/dev-productivity-web" + ] + ); + } } diff --git a/src/ownership/mapper.rs b/src/ownership/mapper.rs index d04c4ee..a683ec8 100644 --- a/src/ownership/mapper.rs +++ b/src/ownership/mapper.rs @@ -24,7 +24,6 @@ pub use team_yml_mapper::TeamYmlMapper; use super::Entry; pub trait Mapper { - fn name(&self) -> String; fn entries(&self) -> Vec; fn owner_matchers(&self) -> Vec; } diff --git a/src/ownership/mapper/annotated_file_mapper.rs b/src/ownership/mapper/annotated_file_mapper.rs index ec69230..1ae6a52 100644 --- a/src/ownership/mapper/annotated_file_mapper.rs +++ b/src/ownership/mapper/annotated_file_mapper.rs @@ -61,10 +61,6 @@ impl Mapper for TeamFileMapper { vec![OwnerMatcher::ExactMatches(path_to_team, Source::AnnotatedFile)] } - - fn name(&self) -> String { - "Annotations at the top of file".to_owned() - } } #[cfg(test)] diff --git a/src/ownership/mapper/directory_mapper.rs b/src/ownership/mapper/directory_mapper.rs index 295e842..a028128 100644 --- a/src/ownership/mapper/directory_mapper.rs +++ b/src/ownership/mapper/directory_mapper.rs @@ -55,10 +55,6 @@ impl Mapper for DirectoryMapper { owner_matchers } - - fn name(&self) -> String { - "Owner in .codeowner".to_owned() - } } #[cfg(test)] diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index 351dfd6..155ed8b 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -31,10 +31,6 @@ impl Mapper for RubyPackageMapper { fn owner_matchers(&self) -> Vec { PackageMapper::build(self.project.clone()).owner_matchers(&PackageType::Ruby) } - - fn name(&self) -> String { - "Owner metadata key in package.yml".to_owned() - } } impl JavascriptPackageMapper { @@ -51,10 +47,6 @@ impl Mapper for JavascriptPackageMapper { fn owner_matchers(&self) -> Vec { PackageMapper::build(self.project.clone()).owner_matchers(&PackageType::Javascript) } - - fn name(&self) -> String { - "Owner metadata key in package.json".to_owned() - } } impl PackageMapper { diff --git a/src/ownership/mapper/team_gem_mapper.rs b/src/ownership/mapper/team_gem_mapper.rs index 9d74914..fc6cf5b 100644 --- a/src/ownership/mapper/team_gem_mapper.rs +++ b/src/ownership/mapper/team_gem_mapper.rs @@ -57,10 +57,6 @@ impl Mapper for TeamGemMapper { owner_matchers } - - fn name(&self) -> String { - "Team owned gems".to_owned() - } } #[cfg(test)] diff --git a/src/ownership/mapper/team_glob_mapper.rs b/src/ownership/mapper/team_glob_mapper.rs index be4230b..32b58b8 100644 --- a/src/ownership/mapper/team_glob_mapper.rs +++ b/src/ownership/mapper/team_glob_mapper.rs @@ -49,10 +49,6 @@ impl Mapper for TeamGlobMapper { owner_matchers } - - fn name(&self) -> String { - "Team-specific owned globs".to_owned() - } } #[cfg(test)] diff --git a/src/ownership/mapper/team_yml_mapper.rs b/src/ownership/mapper/team_yml_mapper.rs index c05c417..0c0cd53 100644 --- a/src/ownership/mapper/team_yml_mapper.rs +++ b/src/ownership/mapper/team_yml_mapper.rs @@ -42,10 +42,6 @@ impl Mapper for TeamYmlMapper { vec![OwnerMatcher::ExactMatches(path_to_team, Source::TeamYml)] } - - fn name(&self) -> String { - "Team YML ownership".to_owned() - } } #[cfg(test)] diff --git a/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS b/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS index 0aa5a81..5dc1afa 100644 --- a/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS +++ b/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS @@ -7,11 +7,8 @@ # https://help.github.com/en/articles/about-code-owners -# Owner in .codeowner /app/consumers/**/** @barteam /app/services/**/** @footeam /app/services/exciting/**/** @barteam - -# Team YML ownership /config/teams/bar.yml @barteam /config/teams/foo.yml @footeam diff --git a/tests/fixtures/multiple-directory-owners/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS b/tests/fixtures/multiple-directory-owners/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS new file mode 100644 index 0000000..f8fe621 --- /dev/null +++ b/tests/fixtures/multiple-directory-owners/tests/fixtures/multiple-directory-owners/.github/CODEOWNERS @@ -0,0 +1,8 @@ +# 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 + diff --git a/tests/fixtures/valid_project/.github/CODEOWNERS b/tests/fixtures/valid_project/.github/CODEOWNERS index e0d3d9a..3858147 100644 --- a/tests/fixtures/valid_project/.github/CODEOWNERS +++ b/tests/fixtures/valid_project/.github/CODEOWNERS @@ -7,35 +7,22 @@ # https://help.github.com/en/articles/about-code-owners -# Annotations at the top of file +/config/teams/payments.yml @PaymentsTeam +/config/teams/payroll.yml @PayrollTeam +/config/teams/ux.yml @UX +/gems/payroll_calculator/**/** @PayrollTeam +/gems/pets/**/** @UX +/javascript/packages/PayrollFlow/**/** @PayrollTeam /javascript/packages/PayrollFlow/index.tsx @PayrollTeam +/javascript/packages/items/**/** @PayrollTeam +/javascript/packages/items/(special)/**/** @PaymentsTeam /javascript/packages/list/page-admin.tsx @PaymentsTeam /ruby/app/models/bank_account.rb @PaymentsTeam /ruby/app/models/payroll.rb @PayrollTeam -/ruby/app/views/foos/edit.erb @PayrollTeam -/ruby/app/views/foos/index.html.erb @UX -/ruby/app/views/foos/new.html.erb @PayrollTeam - -# Team-specific owned globs /ruby/app/payments/**/* @PaymentsTeam - -# Owner in .codeowner -/javascript/packages/items/**/** @PayrollTeam -/javascript/packages/items/(special)/**/** @PaymentsTeam /ruby/app/payments/foo/**/** @PayrollTeam /ruby/app/payroll/**/** @PayrollTeam - -# Owner metadata key in package.yml +/ruby/app/views/foos/edit.erb @PayrollTeam +/ruby/app/views/foos/index.html.erb @UX +/ruby/app/views/foos/new.html.erb @PayrollTeam /ruby/packages/payroll_flow/**/** @PayrollTeam - -# Owner metadata key in package.json -/javascript/packages/PayrollFlow/**/** @PayrollTeam - -# Team YML ownership -/config/teams/payments.yml @PaymentsTeam -/config/teams/payroll.yml @PayrollTeam -/config/teams/ux.yml @UX - -# Team owned gems -/gems/payroll_calculator/**/** @PayrollTeam -/gems/pets/**/** @UX diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index ca2aa56..14a31e2 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -299,31 +299,18 @@ fn test_for_team() -> Result<(), Box> { predicate::eq(indoc! {" # Code Ownership Report for `Payroll` Team - ## Annotations at the top of file + ## Owned Files + /config/teams/payroll.yml + /gems/payroll_calculator/**/** + /javascript/packages/PayrollFlow/**/** /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. - - ## Owner in .codeowner /javascript/packages/items/**/** + /ruby/app/models/payroll.rb /ruby/app/payments/foo/**/** /ruby/app/payroll/**/** - - ## Owner metadata key in package.yml + /ruby/app/views/foos/edit.erb + /ruby/app/views/foos/new.html.erb /ruby/packages/payroll_flow/**/** - - ## Owner metadata key in package.json - /javascript/packages/PayrollFlow/**/** - - ## Team YML ownership - /config/teams/payroll.yml - - ## Team owned gems - /gems/payroll_calculator/**/** "}), )?;