From fcd47bb1eaaa35edf1bf73d5c2ce40612605b5b6 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 15 Aug 2025 20:54:41 -0500 Subject: [PATCH 1/9] overrides fixture --- .../.github/CODEOWNERS | 18 ------------------ .../config/teams/brewers.yml | 8 +------- .../config/teams/cubs.yml | 6 ------ .../config/teams/giants.yml | 5 +---- .../config/teams/rockies.yml | 5 +---- .../ruby/app/cubs/services/models/db/price.rb | 5 +---- .../cache/codeowners/project-file-cache.json | 1 + 7 files changed, 5 insertions(+), 43 deletions(-) create mode 100644 tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json diff --git a/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS b/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS index 8ae8814..ea6866b 100644 --- a/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS +++ b/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS @@ -8,26 +8,14 @@ # Annotations at the top of file -/frontend/packages/components/datepicker/src/picks/dp.tsx @RockiesTeam -/frontend/packages/components/list/src/item.tsx @BrewersTeam -/frontend/packages/components/textfield/src/field.tsx @GiantsTeam -/frontend/packages/components/textfield/src/fields/small.tsx @GiantsTeam -/gems/apollo/lib/apollo.rb @GiantsTeam -/gems/ivy/lib/ivy.rb @CubsTeam -/gems/lager/lib/lager.rb @BrewersTeam -/gems/summit/lib/summit.rb @RockiesTeam -/ruby/app/cubs/services/models/db/price.rb @BrewersTeam /ruby/app/cubs/services/play.rb @CubsTeam # Team-specific owned globs -/frontend/packages/components/** @BrewersTeam /ruby/app/brewers/**/* @BrewersTeam -/ruby/app/cubs/**/* @CubsTeam /ruby/app/giants/**/* @GiantsTeam /ruby/app/rockies/**/* @RockiesTeam # Owner in .codeowner -/ruby/app/cubs/**/** @CubsTeam /ruby/app/cubs/services/models/**/** @RockiesTeam # Owner metadata key in package.yml @@ -45,9 +33,3 @@ /config/teams/cubs.yml @CubsTeam /config/teams/giants.yml @GiantsTeam /config/teams/rockies.yml @RockiesTeam - -# Team owned gems -/gems/apollo/**/** @GiantsTeam -/gems/ivy/**/** @CubsTeam -/gems/lager/**/** @BrewersTeam -/gems/summit/**/** @RockiesTeam diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml index 72a3654..125858a 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml @@ -2,10 +2,4 @@ name: Brewers github: team: '@BrewersTeam' owned_globs: - - ruby/app/brewers/**/* - - frontend/packages/components/** -subtracted_globs: - - frontend/packages/components/textfield/** -ruby: - owned_gems: - - lager \ No newline at end of file + - ruby/app/brewers/**/* \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml index 81797e5..6e1efea 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml @@ -1,9 +1,3 @@ name: Cubs github: team: '@CubsTeam' -owned_globs: - - ruby/app/cubs/**/* -ruby: - owned_gems: - - ivy - diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml index 22a8b9f..cb78d2d 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml @@ -2,7 +2,4 @@ name: Giants github: team: '@GiantsTeam' owned_globs: - - ruby/app/giants/**/* -ruby: - owned_gems: - - apollo \ No newline at end of file + - ruby/app/giants/**/* \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml b/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml index 52cb132..342a72b 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml @@ -2,7 +2,4 @@ name: Rockies github: team: '@RockiesTeam' owned_globs: - - ruby/app/rockies/**/* -ruby: - owned_gems: - - summit \ No newline at end of file + - ruby/app/rockies/**/* \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb index 5c15cca..d8b330a 100644 --- a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb @@ -1,5 +1,2 @@ -# @team Brewers - -class Price -end +class Price; end \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json b/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json new file mode 100644 index 0000000..cf6c1aa --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json @@ -0,0 +1 @@ +{"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb":{"timestamp":1755309011,"owner":"Cubs"},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/entertainment.rb":{"timestamp":1755309127,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb":{"timestamp":1755309218,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx":{"timestamp":1755307665,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/giants/services/play.rb":{"timestamp":1755307039,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb":{"timestamp":1755308422,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/schedule/app/services/date.rb":{"timestamp":1755307389,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/locations/app/services/capacity.rb":{"timestamp":1755307417,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/games/app/services/stats.rb":{"timestamp":1755307271,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx":{"timestamp":1755307786,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/rockies/services/play.rb":{"timestamp":1755306979,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx":{"timestamp":1755307706,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx":{"timestamp":1755307647,"owner":null}} \ No newline at end of file From 53f264eeb17f3d2b0944cd38d365d60fcc3e538b Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 07:05:14 -0500 Subject: [PATCH 2/9] overrides prompt --- ai-prompts/overrides.md | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 ai-prompts/overrides.md diff --git a/ai-prompts/overrides.md b/ai-prompts/overrides.md new file mode 100644 index 0000000..9aabf9d --- /dev/null +++ b/ai-prompts/overrides.md @@ -0,0 +1,48 @@ +# Owner overrides + +## Context + +Today, if more than one mapper claims ownership of a file, we return an error. There are limited exceptions for directory ownership where multiple directories may apply, and we conceptually pick the nearest directory. + +We have seen frequent confusion and repeated requests for a way to explicitly override ownership. + +## Proposal + +Allow overrides, resolving conflicts by choosing the most specific claim. + +Priority (most specific to least specific) +Ownership for a file is determined by the closest applicable claim: + +File annotations (inline annotations in the file) +Directory ownership (the nearest ancestor directory claim). If directory ownership is declared above a package file, it would be lower priority +Package ownership (package.yml, package.json) +Team glob patterns +If multiple teams match at the same priority level (e.g., multiple team glob patterns match the same path), this remains an error. + +## Tooling to reduce confusion + +Update the for-file command to list all matching teams in descending priority, so users can see which owner would win and why. + +### generate-and-validate AND for-file + +Both places need to be updated. There should be tests in place that verify consistency. **I can likely compare the src/parser.rs derived team to the for-file team** in tests locally and against a large repo. + +## The details + +### gv +- implement the new errors +- sort a file's "owners" by priority when writing to CODEOWNERS use the most specific...I think this means we can't have redundancy + +### for-file +- same errors? Look for reuse +- descriptive results +- gem should have a verbose option + +### "New" errors + +1. FileWithMultipleOwners is still a thing. Probably only possible if more than one team file claims ownership. Reason being is that other "claims" can be prioritized by proximity to the file +1. RedundantOwnership. **Looks like redundancy is already OK**. Maybe? Would we want avoid files getting owned by the same team multiple ways? It could potentially not be a problem with a really good `for-file`, but ... It's important to keep in mind that it's better to start more restrictive and then relax constraints than the other way around. + +### Questions +1. Why is owner's source a vec? +Looks like every claim for the same team goes in there. As an example, you can add a matching file annotation and there will be no error and for-file will show all sources \ No newline at end of file From 593e939ca7e717d1172e29cab943c4b1b1a96d8e Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 12:07:59 -0500 Subject: [PATCH 3/9] adding verify compare for file --- prompts/owner-overrides.md | 31 +++++++ src/lib.rs | 1 + src/verify.rs | 90 +++++++++++++++++++ .../cache/codeowners/project-file-cache.json | 1 - 4 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 prompts/owner-overrides.md create mode 100644 src/verify.rs delete mode 100644 tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json diff --git a/prompts/owner-overrides.md b/prompts/owner-overrides.md new file mode 100644 index 0000000..26bb239 --- /dev/null +++ b/prompts/owner-overrides.md @@ -0,0 +1,31 @@ +# Feature Request + +## Owner overrides + +### Context + +Today, if more than one mapper claims ownership of a file, we return an error. There are limited exceptions for directory ownership where multiple directories may apply, and we conceptually pick the nearest directory. + +We have seen frequent confusion and repeated requests for a way to explicitly override ownership. + +### Proposal + +Allow overrides, resolving conflicts by choosing the most specific claim. + +#### Priority (most specific to least specific) + +Ownership for a file is determined by the _closest_ applicable claim: + +1. File annotations (inline annotations in the file) +1. *Directory ownership (the nearest ancestor directory claim) +1. Package ownership (`package.yml`, `package.json`) +1. Team glob patterns + +If multiple teams match at the same priority level (e.g., multiple team glob patterns match the same path), this remains an error. + +#### Tooling to reduce confusion + +Update the `for-file` command to list all matching teams in descending priority, so users can see which owner would win and why. + + +* If directory ownership is declared _above_ a package file, it would be lower priority \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index cd7309f..7b8650c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,3 +7,4 @@ pub(crate) mod project; pub mod project_builder; pub mod project_file_builder; pub mod runner; +pub mod verify; diff --git a/src/verify.rs b/src/verify.rs new file mode 100644 index 0000000..204e3d2 --- /dev/null +++ b/src/verify.rs @@ -0,0 +1,90 @@ +use std::path::Path; + +use crate::{ + cache::Cache, + config::Config, + ownership::for_file_fast::find_file_owners, + project::Project, + project_builder::ProjectBuilder, + runner::{RunConfig, RunResult, config_from_path, team_for_file_from_codeowners}, +}; + +pub fn verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> RunResult { + match do_verify_compare_for_file(run_config, cache) { + Ok(mismatches) if mismatches.is_empty() => RunResult { + info_messages: vec!["Success! All files match between CODEOWNERS and for-file command.".to_string()], + ..Default::default() + }, + Ok(mismatches) => RunResult { + validation_errors: mismatches, + ..Default::default() + }, + Err(err) => RunResult { + io_errors: vec![err], + ..Default::default() + }, + } +} + +fn do_verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> Result, String> { + let config = load_config(run_config)?; + let project = build_project(&config, run_config, cache)?; + + let mut mismatches: Vec = Vec::new(); + for file in &project.files { + let (codeowners_team, fast_display) = owners_for_file(&file.path, run_config, &config)?; + let codeowners_display = codeowners_team.clone().unwrap_or_else(|| "Unowned".to_string()); + if !is_match(codeowners_team.as_deref(), &fast_display) { + mismatches.push(format_mismatch(&project, &file.path, &codeowners_display, &fast_display)); + } + } + + Ok(mismatches) +} + +fn load_config(run_config: &RunConfig) -> Result { + config_from_path(&run_config.config_path).map_err(|e| e.to_string()) +} + +fn build_project(config: &Config, run_config: &RunConfig, cache: &Cache) -> Result { + let mut project_builder = ProjectBuilder::new( + config, + run_config.project_root.clone(), + run_config.codeowners_file_path.clone(), + cache, + ); + project_builder.build().map_err(|e| e.to_string()) +} + +fn owners_for_file(path: &Path, run_config: &RunConfig, config: &Config) -> Result<(Option, String), String> { + let file_path_str = path.to_string_lossy().to_string(); + + let codeowners_team = team_for_file_from_codeowners(run_config, &file_path_str) + .map_err(|e| e.to_string())? + .map(|t| t.name); + + let fast_owners = find_file_owners(&run_config.project_root, config, Path::new(&file_path_str))?; + let fast_display = match fast_owners.len() { + 0 => "Unowned".to_string(), + 1 => fast_owners[0].team.name.clone(), + _ => { + let names: Vec = fast_owners.into_iter().map(|fo| fo.team.name).collect(); + format!("Multiple: {}", names.join(", ")) + } + }; + + Ok((codeowners_team, fast_display)) +} + +fn is_match(codeowners_team: Option<&str>, fast_display: &str) -> bool { + match (codeowners_team, fast_display) { + (None, "Unowned") => true, + (Some(t), fd) if fd == t => true, + _ => false, + } +} + +fn format_mismatch(project: &Project, file_path: &Path, codeowners_display: &str, fast_display: &str) -> String { + let rel = project.relative_path(file_path).to_string_lossy().to_string(); + format!("- {}: CODEOWNERS={} fast={}", rel, codeowners_display, fast_display) +} diff --git a/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json b/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json deleted file mode 100644 index cf6c1aa..0000000 --- a/tests/fixtures/valid_project_with_overrides/tmp/cache/codeowners/project-file-cache.json +++ /dev/null @@ -1 +0,0 @@ -{"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb":{"timestamp":1755309011,"owner":"Cubs"},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/entertainment.rb":{"timestamp":1755309127,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb":{"timestamp":1755309218,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx":{"timestamp":1755307665,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/giants/services/play.rb":{"timestamp":1755307039,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb":{"timestamp":1755308422,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/schedule/app/services/date.rb":{"timestamp":1755307389,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/locations/app/services/capacity.rb":{"timestamp":1755307417,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/packs/games/app/services/stats.rb":{"timestamp":1755307271,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx":{"timestamp":1755307786,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/ruby/app/rockies/services/play.rb":{"timestamp":1755306979,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx":{"timestamp":1755307706,"owner":null},"/Users/perryhertler/workspace/rubyatscale/codeowners-rs/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx":{"timestamp":1755307647,"owner":null}} \ No newline at end of file From 620c82fd6f979813eabbda7db4a14d801d2f398b Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 12:13:31 -0500 Subject: [PATCH 4/9] valid project with overrides test --- .../fixtures/valid_project_with_overrides/config/teams/cubs.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml index 6e1efea..1ee2812 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml @@ -1,3 +1,4 @@ name: Cubs github: team: '@CubsTeam' + From a84afff4e03f69d84ec073a4c6344f1eefffbcd3 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 18:22:28 -0500 Subject: [PATCH 5/9] from codeowners only --- ai-prompts/overrides.md | 48 ----------------------------------------- 1 file changed, 48 deletions(-) delete mode 100644 ai-prompts/overrides.md diff --git a/ai-prompts/overrides.md b/ai-prompts/overrides.md deleted file mode 100644 index 9aabf9d..0000000 --- a/ai-prompts/overrides.md +++ /dev/null @@ -1,48 +0,0 @@ -# Owner overrides - -## Context - -Today, if more than one mapper claims ownership of a file, we return an error. There are limited exceptions for directory ownership where multiple directories may apply, and we conceptually pick the nearest directory. - -We have seen frequent confusion and repeated requests for a way to explicitly override ownership. - -## Proposal - -Allow overrides, resolving conflicts by choosing the most specific claim. - -Priority (most specific to least specific) -Ownership for a file is determined by the closest applicable claim: - -File annotations (inline annotations in the file) -Directory ownership (the nearest ancestor directory claim). If directory ownership is declared above a package file, it would be lower priority -Package ownership (package.yml, package.json) -Team glob patterns -If multiple teams match at the same priority level (e.g., multiple team glob patterns match the same path), this remains an error. - -## Tooling to reduce confusion - -Update the for-file command to list all matching teams in descending priority, so users can see which owner would win and why. - -### generate-and-validate AND for-file - -Both places need to be updated. There should be tests in place that verify consistency. **I can likely compare the src/parser.rs derived team to the for-file team** in tests locally and against a large repo. - -## The details - -### gv -- implement the new errors -- sort a file's "owners" by priority when writing to CODEOWNERS use the most specific...I think this means we can't have redundancy - -### for-file -- same errors? Look for reuse -- descriptive results -- gem should have a verbose option - -### "New" errors - -1. FileWithMultipleOwners is still a thing. Probably only possible if more than one team file claims ownership. Reason being is that other "claims" can be prioritized by proximity to the file -1. RedundantOwnership. **Looks like redundancy is already OK**. Maybe? Would we want avoid files getting owned by the same team multiple ways? It could potentially not be a problem with a really good `for-file`, but ... It's important to keep in mind that it's better to start more restrictive and then relax constraints than the other way around. - -### Questions -1. Why is owner's source a vec? -Looks like every claim for the same team goes in there. As an example, you can add a matching file annotation and there will be no error and for-file will show all sources \ No newline at end of file From e7898aa1fe2678e5a7ebef963404c5946a452309 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 18:25:00 -0500 Subject: [PATCH 6/9] updating fixtures for overrides --- prompts/owner-overrides.md | 31 ------------------- .../.github/CODEOWNERS | 18 +++++++++++ .../config/teams/brewers.yml | 8 ++++- .../config/teams/cubs.yml | 5 +++ .../config/teams/giants.yml | 5 ++- .../config/teams/rockies.yml | 5 ++- .../ruby/app/cubs/services/models/db/price.rb | 5 ++- 7 files changed, 42 insertions(+), 35 deletions(-) delete mode 100644 prompts/owner-overrides.md diff --git a/prompts/owner-overrides.md b/prompts/owner-overrides.md deleted file mode 100644 index 26bb239..0000000 --- a/prompts/owner-overrides.md +++ /dev/null @@ -1,31 +0,0 @@ -# Feature Request - -## Owner overrides - -### Context - -Today, if more than one mapper claims ownership of a file, we return an error. There are limited exceptions for directory ownership where multiple directories may apply, and we conceptually pick the nearest directory. - -We have seen frequent confusion and repeated requests for a way to explicitly override ownership. - -### Proposal - -Allow overrides, resolving conflicts by choosing the most specific claim. - -#### Priority (most specific to least specific) - -Ownership for a file is determined by the _closest_ applicable claim: - -1. File annotations (inline annotations in the file) -1. *Directory ownership (the nearest ancestor directory claim) -1. Package ownership (`package.yml`, `package.json`) -1. Team glob patterns - -If multiple teams match at the same priority level (e.g., multiple team glob patterns match the same path), this remains an error. - -#### Tooling to reduce confusion - -Update the `for-file` command to list all matching teams in descending priority, so users can see which owner would win and why. - - -* If directory ownership is declared _above_ a package file, it would be lower priority \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS b/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS index ea6866b..8ae8814 100644 --- a/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS +++ b/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS @@ -8,14 +8,26 @@ # Annotations at the top of file +/frontend/packages/components/datepicker/src/picks/dp.tsx @RockiesTeam +/frontend/packages/components/list/src/item.tsx @BrewersTeam +/frontend/packages/components/textfield/src/field.tsx @GiantsTeam +/frontend/packages/components/textfield/src/fields/small.tsx @GiantsTeam +/gems/apollo/lib/apollo.rb @GiantsTeam +/gems/ivy/lib/ivy.rb @CubsTeam +/gems/lager/lib/lager.rb @BrewersTeam +/gems/summit/lib/summit.rb @RockiesTeam +/ruby/app/cubs/services/models/db/price.rb @BrewersTeam /ruby/app/cubs/services/play.rb @CubsTeam # Team-specific owned globs +/frontend/packages/components/** @BrewersTeam /ruby/app/brewers/**/* @BrewersTeam +/ruby/app/cubs/**/* @CubsTeam /ruby/app/giants/**/* @GiantsTeam /ruby/app/rockies/**/* @RockiesTeam # Owner in .codeowner +/ruby/app/cubs/**/** @CubsTeam /ruby/app/cubs/services/models/**/** @RockiesTeam # Owner metadata key in package.yml @@ -33,3 +45,9 @@ /config/teams/cubs.yml @CubsTeam /config/teams/giants.yml @GiantsTeam /config/teams/rockies.yml @RockiesTeam + +# Team owned gems +/gems/apollo/**/** @GiantsTeam +/gems/ivy/**/** @CubsTeam +/gems/lager/**/** @BrewersTeam +/gems/summit/**/** @RockiesTeam diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml index 125858a..72a3654 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml @@ -2,4 +2,10 @@ name: Brewers github: team: '@BrewersTeam' owned_globs: - - ruby/app/brewers/**/* \ No newline at end of file + - ruby/app/brewers/**/* + - frontend/packages/components/** +subtracted_globs: + - frontend/packages/components/textfield/** +ruby: + owned_gems: + - lager \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml index 1ee2812..81797e5 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml @@ -1,4 +1,9 @@ name: Cubs github: team: '@CubsTeam' +owned_globs: + - ruby/app/cubs/**/* +ruby: + owned_gems: + - ivy diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml index cb78d2d..22a8b9f 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml @@ -2,4 +2,7 @@ name: Giants github: team: '@GiantsTeam' owned_globs: - - ruby/app/giants/**/* \ No newline at end of file + - ruby/app/giants/**/* +ruby: + owned_gems: + - apollo \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml b/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml index 342a72b..52cb132 100644 --- a/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml +++ b/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml @@ -2,4 +2,7 @@ name: Rockies github: team: '@RockiesTeam' owned_globs: - - ruby/app/rockies/**/* \ No newline at end of file + - ruby/app/rockies/**/* +ruby: + owned_gems: + - summit \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb index d8b330a..5c15cca 100644 --- a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb @@ -1,2 +1,5 @@ +# @team Brewers + +class Price +end -class Price; end \ No newline at end of file From 3a99a87975d121c82d57adea09c395d2c293fbc2 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 07:40:39 -0500 Subject: [PATCH 7/9] renaming verify to crosscheck --- src/lib.rs | 1 - src/verify.rs | 90 --------------------------------------------------- 2 files changed, 91 deletions(-) delete mode 100644 src/verify.rs diff --git a/src/lib.rs b/src/lib.rs index 7b8650c..cd7309f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,4 +7,3 @@ pub(crate) mod project; pub mod project_builder; pub mod project_file_builder; pub mod runner; -pub mod verify; diff --git a/src/verify.rs b/src/verify.rs deleted file mode 100644 index 204e3d2..0000000 --- a/src/verify.rs +++ /dev/null @@ -1,90 +0,0 @@ -use std::path::Path; - -use crate::{ - cache::Cache, - config::Config, - ownership::for_file_fast::find_file_owners, - project::Project, - project_builder::ProjectBuilder, - runner::{RunConfig, RunResult, config_from_path, team_for_file_from_codeowners}, -}; - -pub fn verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> RunResult { - match do_verify_compare_for_file(run_config, cache) { - Ok(mismatches) if mismatches.is_empty() => RunResult { - info_messages: vec!["Success! All files match between CODEOWNERS and for-file command.".to_string()], - ..Default::default() - }, - Ok(mismatches) => RunResult { - validation_errors: mismatches, - ..Default::default() - }, - Err(err) => RunResult { - io_errors: vec![err], - ..Default::default() - }, - } -} - -fn do_verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> Result, String> { - let config = load_config(run_config)?; - let project = build_project(&config, run_config, cache)?; - - let mut mismatches: Vec = Vec::new(); - for file in &project.files { - let (codeowners_team, fast_display) = owners_for_file(&file.path, run_config, &config)?; - let codeowners_display = codeowners_team.clone().unwrap_or_else(|| "Unowned".to_string()); - if !is_match(codeowners_team.as_deref(), &fast_display) { - mismatches.push(format_mismatch(&project, &file.path, &codeowners_display, &fast_display)); - } - } - - Ok(mismatches) -} - -fn load_config(run_config: &RunConfig) -> Result { - config_from_path(&run_config.config_path).map_err(|e| e.to_string()) -} - -fn build_project(config: &Config, run_config: &RunConfig, cache: &Cache) -> Result { - let mut project_builder = ProjectBuilder::new( - config, - run_config.project_root.clone(), - run_config.codeowners_file_path.clone(), - cache, - ); - project_builder.build().map_err(|e| e.to_string()) -} - -fn owners_for_file(path: &Path, run_config: &RunConfig, config: &Config) -> Result<(Option, String), String> { - let file_path_str = path.to_string_lossy().to_string(); - - let codeowners_team = team_for_file_from_codeowners(run_config, &file_path_str) - .map_err(|e| e.to_string())? - .map(|t| t.name); - - let fast_owners = find_file_owners(&run_config.project_root, config, Path::new(&file_path_str))?; - let fast_display = match fast_owners.len() { - 0 => "Unowned".to_string(), - 1 => fast_owners[0].team.name.clone(), - _ => { - let names: Vec = fast_owners.into_iter().map(|fo| fo.team.name).collect(); - format!("Multiple: {}", names.join(", ")) - } - }; - - Ok((codeowners_team, fast_display)) -} - -fn is_match(codeowners_team: Option<&str>, fast_display: &str) -> bool { - match (codeowners_team, fast_display) { - (None, "Unowned") => true, - (Some(t), fd) if fd == t => true, - _ => false, - } -} - -fn format_mismatch(project: &Project, file_path: &Path, codeowners_display: &str, fast_display: &str) -> String { - let rel = project.relative_path(file_path).to_string_lossy().to_string(); - format!("- {}: CODEOWNERS={} fast={}", rel, codeowners_display, fast_display) -} From 40e7ae6439f90c522ec715cebe6b9a9720577b80 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 08:46:35 -0500 Subject: [PATCH 8/9] allow colon after @team --- src/common_test.rs | 10 +- src/ownership/mapper/team_file_mapper.rs | 14 +- src/ownership/tests.rs | 219 ------------------ src/project_file_builder.rs | 34 ++- .../fixtures/valid_project/.github/CODEOWNERS | 1 + .../javascript/packages/list/page-admin.tsx | 1 + .../valid_project/ruby/app/models/payroll.rb | 2 +- tests/invalid_project_test.rs | 1 - 8 files changed, 53 insertions(+), 229 deletions(-) delete mode 100644 src/ownership/tests.rs create mode 100644 tests/fixtures/valid_project/javascript/packages/list/page-admin.tsx diff --git a/src/common_test.rs b/src/common_test.rs index 2e9db19..2dea43f 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -246,13 +246,21 @@ pub mod tests { relative_path: "packs/jscomponents/comp.ts".to_owned(), content: "// @team Foo\n".to_owned(), }, + TestProjectFile { + relative_path: "packs/jscomponents/comp-colon.ts".to_owned(), + content: "// @team: FooColon\n".to_owned(), + }, TestProjectFile { relative_path: "packs/[admin]/comp.ts".to_owned(), content: "// @team Bar\n".to_owned(), }, TestProjectFile { relative_path: "packs/bar/comp.rb".to_owned(), - content: "// @team Bar\n".to_owned(), + content: "# @team Bar\n".to_owned(), + }, + TestProjectFile { + relative_path: "packs/bar/comp-colon.rb".to_owned(), + content: "# @team: BarColon\n".to_owned(), }, ], ); diff --git a/src/ownership/mapper/team_file_mapper.rs b/src/ownership/mapper/team_file_mapper.rs index 3bd8e77..f82d688 100644 --- a/src/ownership/mapper/team_file_mapper.rs +++ b/src/ownership/mapper/team_file_mapper.rs @@ -50,12 +50,12 @@ impl Mapper for TeamFileMapper { for owned_file in &self.project.files { if let Some(ref owner) = owned_file.owner { - let team = team_by_name.get(owner); - - if let Some(team) = team { - let relative_path = self.project.relative_path(&owned_file.path); - path_to_team.insert(relative_path.to_owned(), team.name.clone()); - } + let relative_path = self.project.relative_path(&owned_file.path); + let team_name = team_by_name + .get(owner) + .map(|team| team.name.clone()) + .unwrap_or_else(|| owner.clone()); + path_to_team.insert(relative_path.to_owned(), team_name); } } @@ -114,6 +114,8 @@ mod tests { (PathBuf::from("packs/[admin]/comp.ts"), "Bar".to_owned()), (PathBuf::from("packs/bar/comp.rb"), "Bar".to_owned()), (PathBuf::from("packs/jscomponents/comp.ts"), "Foo".to_owned()), + (PathBuf::from("packs/jscomponents/comp-colon.ts"), "FooColon".to_owned()), + (PathBuf::from("packs/bar/comp-colon.rb"), "BarColon".to_owned()), ]), Source::TeamFile, )]; diff --git a/src/ownership/tests.rs b/src/ownership/tests.rs deleted file mode 100644 index 67d17dd..0000000 --- a/src/ownership/tests.rs +++ /dev/null @@ -1,219 +0,0 @@ -use std::{error::Error, path::Path}; - -use crate::{ - common_test::tests::{build_ownership_with_all_mappers, build_ownership_with_directory_codeowners}, - project::{Package, PackageType, Project, ProjectFile, Team, VendoredGem}, -}; - -use super::Ownership; -use pretty_assertions::assert_eq; - -fn build_payroll_team() -> Team { - Team { - path: Path::new("config/teams/payroll.yml").to_owned(), - name: "Payroll".to_owned(), - github_team: "@Payroll-Eng".to_owned(), - owned_globs: vec![], - avoid_ownership: false, - owned_gems: vec![], - } -} - -fn build_payroll_team_with_owned_gem() -> Team { - Team { - path: Path::new("config/teams/payroll.yml").to_owned(), - name: "Payroll".to_owned(), - github_team: "@Payroll-Eng".to_owned(), - owned_globs: vec![], - avoid_ownership: false, - owned_gems: vec!["payroll_calculator".to_owned()], - } -} - -fn build_annotated_file() -> ProjectFile { - ProjectFile { - owner: Some("Payroll".to_owned()), - path: Path::new("packs/payroll/services/runner.rb").to_owned(), - } -} - -fn build_unannotated_file() -> ProjectFile { - ProjectFile { - owner: None, - path: Path::new("packs/payroll/services/runner_helper.rb").to_owned(), - } -} - -fn build_project_with_annotated_file() -> Project { - Project { - base_path: Path::new("").to_owned(), - files: vec![build_annotated_file(), build_unannotated_file()], - packages: vec![], - teams: vec![build_payroll_team()], - vendored_gems: vec![], - codeowners_file: "".to_owned(), - directory_codeowner_files: vec![], - } -} - -fn build_payroll_team_with_owned_glob() -> Team { - Team { - path: Path::new("config/teams/payroll.yml").to_owned(), - name: "Payroll".to_owned(), - github_team: "@Payroll-Eng".to_owned(), - owned_globs: vec!["packs/payroll/**".to_owned()], - avoid_ownership: false, - owned_gems: vec![], - } -} - -fn build_project_with_team_specific_owned_globs() -> Project { - Project { - base_path: Path::new("").to_owned(), - files: vec![build_unannotated_file()], - packages: vec![], - teams: vec![build_payroll_team_with_owned_glob()], - vendored_gems: vec![], - codeowners_file: "".to_owned(), - directory_codeowner_files: vec![], - } -} - -fn build_project_with_packages() -> Project { - Project { - base_path: Path::new("").to_owned(), - files: vec![build_unannotated_file()], - packages: vec![ - Package { - path: Path::new("packs/payroll_package/package.yml").to_owned(), - package_type: PackageType::Ruby, - owner: "Payroll".to_owned(), - }, - Package { - path: Path::new("frontend/payroll_flow/package.json").to_owned(), - package_type: PackageType::Javascript, - owner: "Payroll".to_owned(), - }, - ], - teams: vec![build_payroll_team()], - vendored_gems: vec![], - codeowners_file: "".to_owned(), - directory_codeowner_files: vec![], - } -} - -fn build_project_with_team_owned_gems() -> Project { - Project { - base_path: Path::new("").to_owned(), - files: vec![build_unannotated_file()], - packages: vec![], - teams: vec![build_payroll_team_with_owned_gem()], - vendored_gems: vec![VendoredGem { - path: Path::new("components/payroll_calculator").to_owned(), - name: "payroll_calculator".to_owned(), - }], - codeowners_file: "".to_owned(), - directory_codeowner_files: vec![], - } -} - -#[test] -fn test_annotations_at_the_top_of_file() { - let ownership = Ownership::build(build_project_with_annotated_file()); - - assert_eq!( - ownership.generate_file(), - with_disclaimer(vec![ - "# Annotations at the top of file", - "/packs/payroll/services/runner.rb @Payroll-Eng", - "", - "# Team YML ownership", - "/config/teams/payroll.yml @Payroll-Eng", - "", - ]) - .join("\n") - ) -} - -#[test] -fn test_team_specific_owned_globs() { - let ownership = Ownership::build(build_project_with_team_specific_owned_globs()); - - assert_eq!( - ownership.generate_file(), - with_disclaimer(vec![ - "# Team-specific owned globs", - "/packs/payroll/** @Payroll-Eng", - "", - "# Team YML ownership", - "/config/teams/payroll.yml @Payroll-Eng", - "", - ]) - .join("\n") - ) -} - -#[test] -fn test_owner_metadata_in_package() { - let ownership = Ownership::build(build_project_with_packages()); - - assert_eq!( - ownership.generate_file(), - with_disclaimer(vec![ - "# Owner metadata key in package.yml", - "/packs/payroll_package/**/** @Payroll-Eng", - "", - "# Owner metadata key in package.json", - "/frontend/payroll_flow/**/** @Payroll-Eng", - "", - "# Team YML ownership", - "/config/teams/payroll.yml @Payroll-Eng", - "", - ]) - .join("\n") - ) -} - -#[test] -fn test_team_owned_gems() { - let ownership = Ownership::build(build_project_with_team_owned_gems()); - - assert_eq!( - ownership.generate_file(), - with_disclaimer(vec![ - "# Team YML ownership", - "/config/teams/payroll.yml @Payroll-Eng", - "", - "# Team owned gems", - "/components/payroll_calculator/**/** @Payroll-Eng", - "" - ]) - .join("\n") - ) -} - -#[test] -fn test_file_with_multiple_directory_owners() -> Result<(), Box> { - let ownership = build_ownership_with_directory_codeowners()?; - let result = ownership.validate(); - assert_eq!(&result.ok(), &Some(())); - Ok(()) -} - -#[test] -fn test_all_mappers() -> Result<(), Box> { - let ownership = build_ownership_with_all_mappers()?; - let result = ownership.validate(); - assert_eq!(&result.ok(), &Some(())); - Ok(()) -} - -fn with_disclaimer(lines: Vec<&str>) -> Vec { - let mut buffer: Vec = Vec::new(); - let mut disclaimer = crate::ownership::file_generator::FileGenerator::disclaimer(); - - buffer.append(&mut disclaimer); - buffer.append(&mut lines.iter().map(|l| l.to_string()).collect()); - - buffer -} diff --git a/src/project_file_builder.rs b/src/project_file_builder.rs index 9a8e79d..05269bb 100644 --- a/src/project_file_builder.rs +++ b/src/project_file_builder.rs @@ -13,7 +13,7 @@ pub struct ProjectFileBuilder<'a> { } lazy_static! { - static ref TEAM_REGEX: Regex = Regex::new(r#"^(?:#|//) @team (.*)$"#).expect("error compiling regular expression"); + static ref TEAM_REGEX: Regex = Regex::new(r#"^(?:#|//) @team:? (.*)$"#).expect("error compiling regular expression"); } impl<'a> ProjectFileBuilder<'a> { @@ -73,3 +73,35 @@ pub(crate) fn build_project_file_without_cache(path: &PathBuf) -> ProjectFile { ProjectFile { path: path.clone(), owner } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_team_regex() { + let owner = TEAM_REGEX + .captures("// @team Foo") + .and_then(|cap| cap.get(1)) + .map(|m| m.as_str().to_string()); + assert_eq!(owner, Some("Foo".to_string())); + + let owner = TEAM_REGEX + .captures("// @team Foo") + .and_then(|cap| cap.get(1)) + .map(|m| m.as_str().to_string()); + assert_eq!(owner, Some("Foo".to_string())); + + let owner = TEAM_REGEX + .captures("// @team: Foo") + .and_then(|cap| cap.get(1)) + .map(|m| m.as_str().to_string()); + assert_eq!(owner, Some("Foo".to_string())); + + let owner = TEAM_REGEX + .captures("# @team: Foo") + .and_then(|cap| cap.get(1)) + .map(|m| m.as_str().to_string()); + assert_eq!(owner, Some("Foo".to_string())); + } +} diff --git a/tests/fixtures/valid_project/.github/CODEOWNERS b/tests/fixtures/valid_project/.github/CODEOWNERS index 6df553a..f39bb1f 100644 --- a/tests/fixtures/valid_project/.github/CODEOWNERS +++ b/tests/fixtures/valid_project/.github/CODEOWNERS @@ -9,6 +9,7 @@ # Annotations at the top of file /javascript/packages/PayrollFlow/index.tsx @PayrollTeam +/javascript/packages/list/page-admin.tsx @PaymentsTeam /ruby/app/models/bank_account.rb @PaymentsTeam /ruby/app/models/payroll.rb @PayrollTeam diff --git a/tests/fixtures/valid_project/javascript/packages/list/page-admin.tsx b/tests/fixtures/valid_project/javascript/packages/list/page-admin.tsx new file mode 100644 index 0000000..3d3deef --- /dev/null +++ b/tests/fixtures/valid_project/javascript/packages/list/page-admin.tsx @@ -0,0 +1 @@ +// @team: Payments \ No newline at end of file diff --git a/tests/fixtures/valid_project/ruby/app/models/payroll.rb b/tests/fixtures/valid_project/ruby/app/models/payroll.rb index ac9c4ed..0e6bba8 100644 --- a/tests/fixtures/valid_project/ruby/app/models/payroll.rb +++ b/tests/fixtures/valid_project/ruby/app/models/payroll.rb @@ -1,3 +1,3 @@ -# @team Payroll +# @team: Payroll class Payroll; end diff --git a/tests/invalid_project_test.rs b/tests/invalid_project_test.rs index 824fa63..4a23f83 100644 --- a/tests/invalid_project_test.rs +++ b/tests/invalid_project_test.rs @@ -34,7 +34,6 @@ fn test_validate() -> Result<(), Box> { - ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3' Some files are missing ownership - - ruby/app/models/blockchain.rb - ruby/app/unowned.rb "})); From 9c7ea9aac8ef39d0de713126c02a8cf63c4684fa Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 08:51:30 -0500 Subject: [PATCH 9/9] 0.2.9 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 45a55d2..010d6e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,7 +179,7 @@ checksum = "1462739cb27611015575c0c11df5df7601141071f07518d56fcc1be504cbec97" [[package]] name = "codeowners" -version = "0.2.8" +version = "0.2.9" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index 2132df5..00aa843 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.2.8" +version = "0.2.9" edition = "2024" [profile.release]