From e81c32e0bfd5f9b2f9bf77d7221d4a4669e3c320 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 15 Aug 2025 20:54:41 -0500 Subject: [PATCH 01/10] overrides fixture --- .../.github/CODEOWNERS | 35 +++++++++++++++++++ .../config/code_ownership.yml | 10 ++++++ .../config/teams/brewers.yml | 5 +++ .../config/teams/cubs.yml | 3 ++ .../config/teams/giants.yml | 5 +++ .../config/teams/rockies.yml | 5 +++ .../components/datepicker/package.json | 5 +++ .../components/datepicker/src/picks/dp.tsx | 0 .../packages/components/list/package.json | 5 +++ .../packages/components/list/src/item.tsx | 0 .../components/textfield/package.json | 5 +++ .../components/textfield/src/field.tsx | 0 .../components/textfield/src/fields/small.tsx | 0 .../packs/games/app/services/stats.rb | 0 .../packs/games/package.yml | 1 + .../packs/locations/app/services/capacity.rb | 0 .../packs/locations/package.yml | 1 + .../packs/schedule/app/services/date.rb | 0 .../packs/schedule/package.yml | 1 + .../ruby/app/brewers/services/play.rb | 9 +++++ .../ruby/app/cubs/services/models/.codeowner | 1 + .../ruby/app/cubs/services/models/db/price.rb | 2 ++ .../app/cubs/services/models/entertainment.rb | 0 .../ruby/app/cubs/services/play.rb | 5 +++ .../ruby/app/giants/services/play.rb | 0 .../ruby/app/rockies/services/play.rb | 0 .../cache/codeowners/project-file-cache.json | 1 + 27 files changed, 99 insertions(+) create mode 100644 tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS create mode 100644 tests/fixtures/valid_project_with_overrides/config/code_ownership.yml create mode 100644 tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml create mode 100644 tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml create mode 100644 tests/fixtures/valid_project_with_overrides/config/teams/giants.yml create mode 100644 tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/package.json create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/package.json create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/package.json create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx create mode 100644 tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx create mode 100644 tests/fixtures/valid_project_with_overrides/packs/games/app/services/stats.rb create mode 100644 tests/fixtures/valid_project_with_overrides/packs/games/package.yml create mode 100644 tests/fixtures/valid_project_with_overrides/packs/locations/app/services/capacity.rb create mode 100644 tests/fixtures/valid_project_with_overrides/packs/locations/package.yml create mode 100644 tests/fixtures/valid_project_with_overrides/packs/schedule/app/services/date.rb create mode 100644 tests/fixtures/valid_project_with_overrides/packs/schedule/package.yml create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/.codeowner create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/entertainment.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/giants/services/play.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/rockies/services/play.rb 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 new file mode 100644 index 0000000..ea6866b --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/.github/CODEOWNERS @@ -0,0 +1,35 @@ +# 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 +/ruby/app/cubs/services/play.rb @CubsTeam + +# Team-specific owned globs +/ruby/app/brewers/**/* @BrewersTeam +/ruby/app/giants/**/* @GiantsTeam +/ruby/app/rockies/**/* @RockiesTeam + +# Owner in .codeowner +/ruby/app/cubs/services/models/**/** @RockiesTeam + +# Owner metadata key in package.yml +/packs/games/**/** @RockiesTeam +/packs/locations/**/** @GiantsTeam +/packs/schedule/**/** @BrewersTeam + +# Owner metadata key in package.json +/frontend/packages/components/datepicker/**/** @RockiesTeam +/frontend/packages/components/list/**/** @BrewersTeam +/frontend/packages/components/textfield/**/** @GiantsTeam + +# Team YML ownership +/config/teams/brewers.yml @BrewersTeam +/config/teams/cubs.yml @CubsTeam +/config/teams/giants.yml @GiantsTeam +/config/teams/rockies.yml @RockiesTeam diff --git a/tests/fixtures/valid_project_with_overrides/config/code_ownership.yml b/tests/fixtures/valid_project_with_overrides/config/code_ownership.yml new file mode 100644 index 0000000..b0fc127 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/config/code_ownership.yml @@ -0,0 +1,10 @@ +owned_globs: + - "{gems,config,frontend,ruby,components,packs}/**/*.{rb,tsx}" +ruby_package_paths: + - packs/**/* +javascript_package_paths: + - frontend/packages/** +team_file_glob: + - config/teams/**/*.yml +unbuilt_gems_path: gems +unowned_globs: diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml new file mode 100644 index 0000000..125858a --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/config/teams/brewers.yml @@ -0,0 +1,5 @@ +name: Brewers +github: + team: '@BrewersTeam' +owned_globs: + - 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 new file mode 100644 index 0000000..6e1efea --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/config/teams/cubs.yml @@ -0,0 +1,3 @@ +name: Cubs +github: + team: '@CubsTeam' diff --git a/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml new file mode 100644 index 0000000..cb78d2d --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/config/teams/giants.yml @@ -0,0 +1,5 @@ +name: Giants +github: + team: '@GiantsTeam' +owned_globs: + - 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 new file mode 100644 index 0000000..342a72b --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/config/teams/rockies.yml @@ -0,0 +1,5 @@ +name: Rockies +github: + team: '@RockiesTeam' +owned_globs: + - ruby/app/rockies/**/* \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/package.json b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/package.json new file mode 100644 index 0000000..968cf7a --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/package.json @@ -0,0 +1,5 @@ +{ + "metadata": { + "owner": "Rockies" + } +} diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/package.json b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/package.json new file mode 100644 index 0000000..b9aa9ce --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/package.json @@ -0,0 +1,5 @@ +{ + "metadata": { + "owner": "Brewers" + } +} diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/package.json b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/package.json new file mode 100644 index 0000000..e10d6bc --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/package.json @@ -0,0 +1,5 @@ +{ + "metadata": { + "owner": "Giants" + } +} diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/packs/games/app/services/stats.rb b/tests/fixtures/valid_project_with_overrides/packs/games/app/services/stats.rb new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/packs/games/package.yml b/tests/fixtures/valid_project_with_overrides/packs/games/package.yml new file mode 100644 index 0000000..a72f2f3 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/packs/games/package.yml @@ -0,0 +1 @@ +owner: Rockies \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/packs/locations/app/services/capacity.rb b/tests/fixtures/valid_project_with_overrides/packs/locations/app/services/capacity.rb new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/packs/locations/package.yml b/tests/fixtures/valid_project_with_overrides/packs/locations/package.yml new file mode 100644 index 0000000..b7d6c55 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/packs/locations/package.yml @@ -0,0 +1 @@ +owner: Giants \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/packs/schedule/app/services/date.rb b/tests/fixtures/valid_project_with_overrides/packs/schedule/app/services/date.rb new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/packs/schedule/package.yml b/tests/fixtures/valid_project_with_overrides/packs/schedule/package.yml new file mode 100644 index 0000000..27e4d1a --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/packs/schedule/package.yml @@ -0,0 +1 @@ +owner: Brewers \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb new file mode 100644 index 0000000..37c3af8 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/services/play.rb @@ -0,0 +1,9 @@ +class Play + def initialize(team) + @team = team + end + + def play + puts "Playing #{@team}!" + end +end \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/.codeowner b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/.codeowner new file mode 100644 index 0000000..00089bd --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/.codeowner @@ -0,0 +1 @@ +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 new file mode 100644 index 0000000..d8b330a --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/db/price.rb @@ -0,0 +1,2 @@ + +class Price; end \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/entertainment.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/models/entertainment.rb new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb new file mode 100644 index 0000000..c4c169b --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/services/play.rb @@ -0,0 +1,5 @@ +# @team Cubs + +class Play + +end \ No newline at end of file diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/giants/services/play.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/giants/services/play.rb new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/rockies/services/play.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/rockies/services/play.rb new file mode 100644 index 0000000..e69de29 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 27dc33c07eaa0a95508ca24252a730378c840bcc Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 07:05:14 -0500 Subject: [PATCH 02/10] 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 9aabccda14d4dbe5c7e8cfbcf38fc7d987a86657 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 12:07:59 -0500 Subject: [PATCH 03/10] adding verify compare for file --- prompts/owner-overrides.md | 31 +++++++ src/cli.rs | 4 + src/lib.rs | 1 + src/runner.rs | 10 ++- src/verify.rs | 90 +++++++++++++++++++ .../cache/codeowners/project-file-cache.json | 1 - tests/valid_project_test.rs | 16 ++++ 7 files changed, 151 insertions(+), 2 deletions(-) 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/cli.rs b/src/cli.rs index f0e45ec..ab7c6ba 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -46,6 +46,9 @@ enum Command { #[clap(about = "Delete the cache file.", visible_alias = "d")] DeleteCache, + + #[clap(about = "Compare the CODEOWNERS file to the for-file command.")] + VerifyCompareForFile, } /// A CLI to validate and generate Github's CODEOWNERS file. @@ -113,6 +116,7 @@ pub fn cli() -> Result { Command::ForFile { name, fast: _ } => runner::for_file(&run_config, &name), Command::ForTeam { name } => runner::for_team(&run_config, &name), Command::DeleteCache => runner::delete_cache(&run_config), + Command::VerifyCompareForFile => runner::verify_compare_for_file(&run_config), }; Ok(runner_result) diff --git a/src/lib.rs b/src/lib.rs index e769652..b77a23c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,3 +6,4 @@ pub(crate) mod project; pub mod project_builder; pub mod project_file_builder; pub mod runner; +pub mod verify; diff --git a/src/runner.rs b/src/runner.rs index b1d71d0..758e439 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -132,6 +132,10 @@ pub fn delete_cache(run_config: &RunConfig) -> RunResult { run_with_runner(run_config, |runner| runner.delete_cache()) } +pub fn verify_compare_for_file(run_config: &RunConfig) -> RunResult { + run_with_runner(run_config, |runner| runner.verify_compare_for_file()) +} + pub type Runnable = fn(Runner) -> RunResult; pub fn run_with_runner(run_config: &RunConfig, runnable: F) -> RunResult @@ -172,7 +176,7 @@ impl fmt::Display for Error { } } -fn config_from_path(path: &PathBuf) -> Result { +pub(crate) fn config_from_path(path: &PathBuf) -> Result { let config_file = File::open(path) .change_context(Error::Io(format!("Can't open config file: {}", &path.to_string_lossy()))) .attach_printable(format!("Can't open config file: {}", &path.to_string_lossy()))?; @@ -299,6 +303,10 @@ impl Runner { }, } } + + pub fn verify_compare_for_file(&self) -> RunResult { + crate::verify::verify_compare_for_file(&self.run_config, &self.cache) + } } #[cfg(test)] 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 diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index 790f9f2..a629cf1 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -36,6 +36,22 @@ fn test_generate() -> Result<(), Box> { Ok(()) } +#[test] +fn test_verify_compare_for_file() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/valid_project") + .arg("--no-cache") + .arg("verify-compare-for-file") + .assert() + .success() + .stdout(predicate::eq(indoc! {" + Success! All files match between CODEOWNERS and for-file command. + "})); + + Ok(()) +} + #[test] fn test_for_file() -> Result<(), Box> { Command::cargo_bin("codeowners")? From 988f272520bcfd062349ef78b69afce0fbe3db47 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 12:13:31 -0500 Subject: [PATCH 04/10] valid project with overrides test --- .../config/teams/cubs.yml | 1 + .../ruby/app/brewers/lib/util.rb | 3 + tests/valid_project_with_overrides_test.rs | 33 +++++++++ .../verify_compare_for_file_mismatch_test.rs | 68 +++++++++++++++++++ 4 files changed, 105 insertions(+) create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/brewers/lib/util.rb create mode 100644 tests/valid_project_with_overrides_test.rs create mode 100644 tests/verify_compare_for_file_mismatch_test.rs 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' + diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/lib/util.rb b/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/lib/util.rb new file mode 100644 index 0000000..3c743d3 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/brewers/lib/util.rb @@ -0,0 +1,3 @@ +class Util + +end \ No newline at end of file diff --git a/tests/valid_project_with_overrides_test.rs b/tests/valid_project_with_overrides_test.rs new file mode 100644 index 0000000..1a06529 --- /dev/null +++ b/tests/valid_project_with_overrides_test.rs @@ -0,0 +1,33 @@ +use assert_cmd::prelude::*; +use indoc::indoc; +use predicates::prelude::predicate; +use std::{error::Error, process::Command}; + +#[test] +fn test_validate() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/valid_project_with_overrides") + .arg("--no-cache") + .arg("validate") + .assert() + .success(); + + Ok(()) +} + +#[test] +fn test_verify_compare_for_file() -> Result<(), Box> { + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg("tests/fixtures/valid_project_with_overrides") + .arg("--no-cache") + .arg("verify-compare-for-file") + .assert() + .success() + .stdout(predicate::eq(indoc! {" + Success! All files match between CODEOWNERS and for-file command. + "})); + + Ok(()) +} diff --git a/tests/verify_compare_for_file_mismatch_test.rs b/tests/verify_compare_for_file_mismatch_test.rs new file mode 100644 index 0000000..1b75a33 --- /dev/null +++ b/tests/verify_compare_for_file_mismatch_test.rs @@ -0,0 +1,68 @@ +use assert_cmd::prelude::*; +use indoc::indoc; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path, process::Command}; + +mod common; +use common::setup_fixture_repo; + +const FIXTURE: &str = "tests/fixtures/valid_project"; + +#[test] +fn test_verify_compare_for_file_reports_team_mismatch() -> Result<(), Box> { + // Arrange: copy fixture to temp dir and change a single CODEOWNERS mapping + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + let codeowners_path = project_root.join(".github/CODEOWNERS"); + + let original = fs::read_to_string(&codeowners_path)?; + // Change payroll.rb ownership from @PayrollTeam to @PaymentsTeam to induce a mismatch + let modified = original.replace( + "/ruby/app/models/payroll.rb @PayrollTeam", + "/ruby/app/models/payroll.rb @PaymentsTeam", + ); + fs::write(&codeowners_path, modified)?; + + // Act + Assert + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("verify-compare-for-file") + .assert() + .failure() + .stdout(predicate::str::contains(indoc! {"- ruby/app/models/payroll.rb: CODEOWNERS=Payments fast=Payroll"})); + + Ok(()) +} + +#[test] +fn test_verify_compare_for_file_reports_unowned_mismatch() -> Result<(), Box> { + // Arrange: copy fixture to temp dir and remove a CODEOWNERS rule for an owned file + let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); + let project_root = temp_dir.path(); + let codeowners_path = project_root.join(".github/CODEOWNERS"); + + // Remove the explicit mapping for bank_account.rb so CODEOWNERS reports Unowned + let original = fs::read_to_string(&codeowners_path)?; + let modified: String = original + .lines() + .filter(|line| !line.trim().starts_with("/ruby/app/models/bank_account.rb ")) + .map(|l| format!("{}\n", l)) + .collect(); + fs::write(&codeowners_path, modified)?; + + // Act + Assert + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("verify-compare-for-file") + .assert() + .failure() + .stdout(predicate::str::contains("- ruby/app/models/bank_account.rb: CODEOWNERS=Unowned fast=Payments")); + + Ok(()) +} + + From 23ba0f52d7a096ba8dea0bb861a7d6ef5f7bbd0e Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sat, 16 Aug 2025 18:22:28 -0500 Subject: [PATCH 05/10] from codeowners only --- ai-prompts/overrides.md | 48 ------------------- src/cli.rs | 4 +- src/runner.rs | 29 ++++++++++- .../verify_compare_for_file_mismatch_test.rs | 10 ++-- 4 files changed, 36 insertions(+), 55 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 diff --git a/src/cli.rs b/src/cli.rs index ab7c6ba..ecefcde 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -16,7 +16,7 @@ enum Command { default_value = "false", help = "Find the owner from the CODEOWNERS file and just return the team name and yml path" )] - fast: bool, + from_codeowners: bool, name: String, }, @@ -113,7 +113,7 @@ pub fn cli() -> Result { Command::Validate => runner::validate(&run_config, vec![]), Command::Generate { skip_stage } => runner::generate(&run_config, !skip_stage), Command::GenerateAndValidate { skip_stage } => runner::generate_and_validate(&run_config, vec![], !skip_stage), - Command::ForFile { name, fast: _ } => runner::for_file(&run_config, &name), + Command::ForFile { name, from_codeowners } => runner::for_file(&run_config, &name, from_codeowners), Command::ForTeam { name } => runner::for_team(&run_config, &name), Command::DeleteCache => runner::delete_cache(&run_config), Command::VerifyCompareForFile => runner::verify_compare_for_file(&run_config), diff --git a/src/runner.rs b/src/runner.rs index 758e439..c8496d7 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -36,10 +36,37 @@ pub struct Runner { cache: Cache, } -pub fn for_file(run_config: &RunConfig, file_path: &str) -> RunResult { +pub fn for_file(run_config: &RunConfig, file_path: &str, from_codeowners: bool) -> RunResult { + if from_codeowners { + return for_file_codeowners_only(run_config, file_path); + } for_file_optimized(run_config, file_path) } +fn for_file_codeowners_only(run_config: &RunConfig, file_path: &str) -> RunResult { + match team_for_file_from_codeowners(run_config, file_path) { + Ok(Some(team)) => { + let relative_team_path = team + .path + .strip_prefix(&run_config.project_root) + .unwrap_or(team.path.as_path()) + .to_string_lossy() + .to_string(); + RunResult { + info_messages: vec![format!( + "Team: {}\nGithub Team: {}\nTeam YML: {}\nDescription:\n- Owner inferred from codeowners file", + team.name, team.github_team, relative_team_path + )], + ..Default::default() + } + } + Ok(None) => RunResult::default(), + Err(err) => RunResult { + io_errors: vec![err.to_string()], + ..Default::default() + }, + } +} pub fn team_for_file_from_codeowners(run_config: &RunConfig, file_path: &str) -> Result, Error> { let config = config_from_path(&run_config.config_path)?; let relative_file_path = Path::new(file_path) diff --git a/tests/verify_compare_for_file_mismatch_test.rs b/tests/verify_compare_for_file_mismatch_test.rs index 1b75a33..b725382 100644 --- a/tests/verify_compare_for_file_mismatch_test.rs +++ b/tests/verify_compare_for_file_mismatch_test.rs @@ -31,7 +31,9 @@ fn test_verify_compare_for_file_reports_team_mismatch() -> Result<(), Box Result<(), Box Date: Sat, 16 Aug 2025 18:25:00 -0500 Subject: [PATCH 06/10] 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 ++- .../components/datepicker/src/picks/dp.tsx | 3 ++ .../packages/components/list/src/item.tsx | 3 ++ .../components/textfield/src/field.tsx | 3 ++ .../components/textfield/src/fields/small.tsx | 3 ++ .../gems/apollo/lib/apollo.rb | 6 ++++ .../gems/ivy/lib/ivy.rb | 6 ++++ .../gems/lager/lib/lager.rb | 6 ++++ .../gems/summit/lib/summit.rb | 6 ++++ .../ruby/app/cubs/.codeowner | 3 ++ .../ruby/app/cubs/services/models/db/price.rb | 5 ++- 16 files changed, 81 insertions(+), 35 deletions(-) delete mode 100644 prompts/owner-overrides.md create mode 100644 tests/fixtures/valid_project_with_overrides/gems/apollo/lib/apollo.rb create mode 100644 tests/fixtures/valid_project_with_overrides/gems/ivy/lib/ivy.rb create mode 100644 tests/fixtures/valid_project_with_overrides/gems/lager/lib/lager.rb create mode 100644 tests/fixtures/valid_project_with_overrides/gems/summit/lib/summit.rb create mode 100644 tests/fixtures/valid_project_with_overrides/ruby/app/cubs/.codeowner 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/frontend/packages/components/datepicker/src/picks/dp.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx index e69de29..5fad919 100644 --- a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/datepicker/src/picks/dp.tsx @@ -0,0 +1,3 @@ +// @team Rockies +export const DP = () => null; + diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx index e69de29..1a2f5cd 100644 --- a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/list/src/item.tsx @@ -0,0 +1,3 @@ +// @team Brewers +export const Item = () => null; + diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx index e69de29..1ebf50c 100644 --- a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/field.tsx @@ -0,0 +1,3 @@ +// @team Giants +export const Field = () => null; + diff --git a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx index e69de29..965ced4 100644 --- a/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx +++ b/tests/fixtures/valid_project_with_overrides/frontend/packages/components/textfield/src/fields/small.tsx @@ -0,0 +1,3 @@ +// @team Giants +export const Small = () => null; + diff --git a/tests/fixtures/valid_project_with_overrides/gems/apollo/lib/apollo.rb b/tests/fixtures/valid_project_with_overrides/gems/apollo/lib/apollo.rb new file mode 100644 index 0000000..ce16cf8 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/gems/apollo/lib/apollo.rb @@ -0,0 +1,6 @@ +# @team Giants + +module Apollo +end + + diff --git a/tests/fixtures/valid_project_with_overrides/gems/ivy/lib/ivy.rb b/tests/fixtures/valid_project_with_overrides/gems/ivy/lib/ivy.rb new file mode 100644 index 0000000..8902daa --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/gems/ivy/lib/ivy.rb @@ -0,0 +1,6 @@ +# @team Cubs + +module Ivy +end + + diff --git a/tests/fixtures/valid_project_with_overrides/gems/lager/lib/lager.rb b/tests/fixtures/valid_project_with_overrides/gems/lager/lib/lager.rb new file mode 100644 index 0000000..75cd552 --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/gems/lager/lib/lager.rb @@ -0,0 +1,6 @@ +# @team Brewers + +module Lager +end + + diff --git a/tests/fixtures/valid_project_with_overrides/gems/summit/lib/summit.rb b/tests/fixtures/valid_project_with_overrides/gems/summit/lib/summit.rb new file mode 100644 index 0000000..cfdbfdd --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/gems/summit/lib/summit.rb @@ -0,0 +1,6 @@ +# @team Rockies + +module Summit +end + + diff --git a/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/.codeowner b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/.codeowner new file mode 100644 index 0000000..959006a --- /dev/null +++ b/tests/fixtures/valid_project_with_overrides/ruby/app/cubs/.codeowner @@ -0,0 +1,3 @@ +Cubs + + 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 a859a54dc663ee964900a905580fa13af5205450 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 07:12:25 -0500 Subject: [PATCH 07/10] ignoring overrides as they are not yet implemented --- src/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli.rs b/src/cli.rs index ecefcde..f9baf8b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -47,7 +47,7 @@ enum Command { #[clap(about = "Delete the cache file.", visible_alias = "d")] DeleteCache, - #[clap(about = "Compare the CODEOWNERS file to the for-file command.")] + #[clap(about = "Compare the CODEOWNERS file to the for-file command.", hide = true)] VerifyCompareForFile, } From 1ecbb76b2e4dcd6ac3c5a9dead54f86f3cbe64ee Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 07:40:39 -0500 Subject: [PATCH 08/10] renaming verify to crosscheck --- src/cli.rs | 4 ++-- src/{verify.rs => crosscheck.rs} | 6 +++--- src/lib.rs | 2 +- src/runner.rs | 8 ++++---- tests/valid_project_test.rs | 4 ++-- tests/valid_project_with_overrides_test.rs | 6 ++++-- tests/verify_compare_for_file_mismatch_test.rs | 8 ++++---- 7 files changed, 20 insertions(+), 18 deletions(-) rename src/{verify.rs => crosscheck.rs} (92%) diff --git a/src/cli.rs b/src/cli.rs index f9baf8b..4b6343e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -48,7 +48,7 @@ enum Command { DeleteCache, #[clap(about = "Compare the CODEOWNERS file to the for-file command.", hide = true)] - VerifyCompareForFile, + CrosscheckOwners, } /// A CLI to validate and generate Github's CODEOWNERS file. @@ -116,7 +116,7 @@ pub fn cli() -> Result { Command::ForFile { name, from_codeowners } => runner::for_file(&run_config, &name, from_codeowners), Command::ForTeam { name } => runner::for_team(&run_config, &name), Command::DeleteCache => runner::delete_cache(&run_config), - Command::VerifyCompareForFile => runner::verify_compare_for_file(&run_config), + Command::CrosscheckOwners => runner::crosscheck_owners(&run_config), }; Ok(runner_result) diff --git a/src/verify.rs b/src/crosscheck.rs similarity index 92% rename from src/verify.rs rename to src/crosscheck.rs index 204e3d2..02eba9b 100644 --- a/src/verify.rs +++ b/src/crosscheck.rs @@ -9,8 +9,8 @@ use crate::{ 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) { +pub fn crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> RunResult { + match do_crosscheck_owners(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() @@ -26,7 +26,7 @@ pub fn verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> RunResu } } -fn do_verify_compare_for_file(run_config: &RunConfig, cache: &Cache) -> Result, String> { +fn do_crosscheck_owners(run_config: &RunConfig, cache: &Cache) -> Result, String> { let config = load_config(run_config)?; let project = build_project(&config, run_config, cache)?; diff --git a/src/lib.rs b/src/lib.rs index b77a23c..cd7309f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,9 +1,9 @@ pub mod cache; pub(crate) mod common_test; pub mod config; +pub mod crosscheck; pub mod ownership; pub(crate) mod project; pub mod project_builder; pub mod project_file_builder; pub mod runner; -pub mod verify; diff --git a/src/runner.rs b/src/runner.rs index c8496d7..345f83f 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -159,8 +159,8 @@ pub fn delete_cache(run_config: &RunConfig) -> RunResult { run_with_runner(run_config, |runner| runner.delete_cache()) } -pub fn verify_compare_for_file(run_config: &RunConfig) -> RunResult { - run_with_runner(run_config, |runner| runner.verify_compare_for_file()) +pub fn crosscheck_owners(run_config: &RunConfig) -> RunResult { + run_with_runner(run_config, |runner| runner.crosscheck_owners()) } pub type Runnable = fn(Runner) -> RunResult; @@ -331,8 +331,8 @@ impl Runner { } } - pub fn verify_compare_for_file(&self) -> RunResult { - crate::verify::verify_compare_for_file(&self.run_config, &self.cache) + pub fn crosscheck_owners(&self) -> RunResult { + crate::crosscheck::crosscheck_owners(&self.run_config, &self.cache) } } diff --git a/tests/valid_project_test.rs b/tests/valid_project_test.rs index a629cf1..795bff9 100644 --- a/tests/valid_project_test.rs +++ b/tests/valid_project_test.rs @@ -37,12 +37,12 @@ fn test_generate() -> Result<(), Box> { } #[test] -fn test_verify_compare_for_file() -> Result<(), Box> { +fn test_crosscheck_owners() -> Result<(), Box> { Command::cargo_bin("codeowners")? .arg("--project-root") .arg("tests/fixtures/valid_project") .arg("--no-cache") - .arg("verify-compare-for-file") + .arg("crosscheck-owners") .assert() .success() .stdout(predicate::eq(indoc! {" diff --git a/tests/valid_project_with_overrides_test.rs b/tests/valid_project_with_overrides_test.rs index 1a06529..6d92fb4 100644 --- a/tests/valid_project_with_overrides_test.rs +++ b/tests/valid_project_with_overrides_test.rs @@ -4,6 +4,7 @@ use predicates::prelude::predicate; use std::{error::Error, process::Command}; #[test] +#[ignore] fn test_validate() -> Result<(), Box> { Command::cargo_bin("codeowners")? .arg("--project-root") @@ -17,12 +18,13 @@ fn test_validate() -> Result<(), Box> { } #[test] -fn test_verify_compare_for_file() -> Result<(), Box> { +#[ignore] +fn test_crosscheck_owners() -> Result<(), Box> { Command::cargo_bin("codeowners")? .arg("--project-root") .arg("tests/fixtures/valid_project_with_overrides") .arg("--no-cache") - .arg("verify-compare-for-file") + .arg("crosscheck-owners") .assert() .success() .stdout(predicate::eq(indoc! {" diff --git a/tests/verify_compare_for_file_mismatch_test.rs b/tests/verify_compare_for_file_mismatch_test.rs index b725382..c9252a3 100644 --- a/tests/verify_compare_for_file_mismatch_test.rs +++ b/tests/verify_compare_for_file_mismatch_test.rs @@ -9,7 +9,7 @@ use common::setup_fixture_repo; const FIXTURE: &str = "tests/fixtures/valid_project"; #[test] -fn test_verify_compare_for_file_reports_team_mismatch() -> Result<(), Box> { +fn test_crosscheck_owners_reports_team_mismatch() -> Result<(), Box> { // Arrange: copy fixture to temp dir and change a single CODEOWNERS mapping let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); let project_root = temp_dir.path(); @@ -28,7 +28,7 @@ fn test_verify_compare_for_file_reports_team_mismatch() -> Result<(), Box Result<(), Box Result<(), Box> { +fn test_crosscheck_owners_reports_unowned_mismatch() -> Result<(), Box> { // Arrange: copy fixture to temp dir and remove a CODEOWNERS rule for an owned file let temp_dir = setup_fixture_repo(Path::new(FIXTURE)); let project_root = temp_dir.path(); @@ -59,7 +59,7 @@ fn test_verify_compare_for_file_reports_unowned_mismatch() -> Result<(), Box Date: Sun, 17 Aug 2025 07:54:35 -0500 Subject: [PATCH 09/10] updating toolchain --- Cargo.lock | 2 +- Cargo.toml | 2 +- rust-toolchain.toml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 88a434e..45a55d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,7 +179,7 @@ checksum = "1462739cb27611015575c0c11df5df7601141071f07518d56fcc1be504cbec97" [[package]] name = "codeowners" -version = "0.2.7" +version = "0.2.8" dependencies = [ "assert_cmd", "clap", diff --git a/Cargo.toml b/Cargo.toml index 7b14a05..2132df5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "codeowners" -version = "0.2.7" +version = "0.2.8" edition = "2024" [profile.release] diff --git a/rust-toolchain.toml b/rust-toolchain.toml index a1e0e05..ff1a27d 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.86.0" +channel = "1.89.0" components = ["clippy", "rustfmt"] targets = ["x86_64-apple-darwin", "aarch64-apple-darwin", "x86_64-unknown-linux-gnu"] From fc7f4a5c50bbda0e52d52afbd67639d0216244ab Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Sun, 17 Aug 2025 07:55:34 -0500 Subject: [PATCH 10/10] linting updates --- src/cache/file.rs | 26 ++++++------- src/ownership/file_generator.rs | 16 ++++---- src/ownership/for_file_fast.rs | 51 +++++++++++++------------- src/ownership/mapper/package_mapper.rs | 8 ++-- src/ownership/validator.rs | 14 +++---- src/project_builder.rs | 23 ++++++------ 6 files changed, 67 insertions(+), 71 deletions(-) diff --git a/src/cache/file.rs b/src/cache/file.rs index 36fc52d..ba76126 100644 --- a/src/cache/file.rs +++ b/src/cache/file.rs @@ -21,26 +21,24 @@ const DEFAULT_CACHE_CAPACITY: usize = 10000; impl Caching for GlobalCache { fn get_file_owner(&self, path: &Path) -> Result, Error> { - if let Some(cache_mutex) = self.file_owner_cache.as_ref() { - if let Ok(cache) = cache_mutex.lock() { - if let Some(cached_entry) = cache.get(path) { - let timestamp = get_file_timestamp(path)?; - if cached_entry.timestamp == timestamp { - return Ok(Some(cached_entry.clone())); - } - } + if let Some(cache_mutex) = self.file_owner_cache.as_ref() + && let Ok(cache) = cache_mutex.lock() + && let Some(cached_entry) = cache.get(path) + { + let timestamp = get_file_timestamp(path)?; + if cached_entry.timestamp == timestamp { + return Ok(Some(cached_entry.clone())); } } Ok(None) } fn write_file_owner(&self, path: &Path, owner: Option) { - if let Some(cache_mutex) = self.file_owner_cache.as_ref() { - if let Ok(mut cache) = cache_mutex.lock() { - if let Ok(timestamp) = get_file_timestamp(path) { - cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner }); - } - } + if let Some(cache_mutex) = self.file_owner_cache.as_ref() + && let Ok(mut cache) = cache_mutex.lock() + && let Ok(timestamp) = get_file_timestamp(path) + { + cache.insert(path.to_path_buf(), FileOwnerCacheEntry { timestamp, owner }); } } diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs index 6d8a34d..8878d41 100644 --- a/src/ownership/file_generator.rs +++ b/src/ownership/file_generator.rs @@ -49,15 +49,15 @@ impl FileGenerator { } pub fn compare_lines(a: &String, b: &String) -> Ordering { - if let Some((prefix, _)) = a.split_once("**") { - if b.starts_with(prefix) { - return Ordering::Less; - } + if let Some((prefix, _)) = a.split_once("**") + && b.starts_with(prefix) + { + return Ordering::Less; } - if let Some((prefix, _)) = b.split_once("**") { - if a.starts_with(prefix) { - return Ordering::Greater; - } + if let Some((prefix, _)) = b.split_once("**") + && a.starts_with(prefix) + { + return Ordering::Greater; } a.cmp(b) } diff --git a/src/ownership/for_file_fast.rs b/src/ownership/for_file_fast.rs index 081cb8a..51d9877 100644 --- a/src/ownership/for_file_fast.rs +++ b/src/ownership/for_file_fast.rs @@ -32,10 +32,11 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) if let Some(rel_str) = relative_file_path.to_str() { let is_config_owned = glob_list_matches(rel_str, &config.owned_globs); let is_config_unowned = glob_list_matches(rel_str, &config.unowned_globs); - if is_config_owned && !is_config_unowned { - if let Some(team) = teams_by_name.get(&team_name) { - sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile); - } + if is_config_owned + && !is_config_unowned + && let Some(team) = teams_by_name.get(&team_name) + { + sources_by_team.entry(team.name.clone()).or_default().push(Source::TeamFile); } } } @@ -194,32 +195,30 @@ fn nearest_package_owner( if let Some(rel_str) = parent_rel.to_str() { if glob_list_matches(rel_str, &config.ruby_package_paths) { let pkg_yml = current.join("package.yml"); - if pkg_yml.exists() { - if let Ok(owner) = read_ruby_package_owner(&pkg_yml) { - if let Some(team) = teams_by_name.get(&owner) { - let package_path = parent_rel.join("package.yml"); - let package_glob = format!("{rel_str}/**/**"); - return Some(( - team.name.clone(), - Source::Package(package_path.to_string_lossy().to_string(), package_glob), - )); - } - } + if pkg_yml.exists() + && let Ok(owner) = read_ruby_package_owner(&pkg_yml) + && let Some(team) = teams_by_name.get(&owner) + { + let package_path = parent_rel.join("package.yml"); + let package_glob = format!("{rel_str}/**/**"); + return Some(( + team.name.clone(), + Source::Package(package_path.to_string_lossy().to_string(), package_glob), + )); } } if glob_list_matches(rel_str, &config.javascript_package_paths) { let pkg_json = current.join("package.json"); - if pkg_json.exists() { - if let Ok(owner) = read_js_package_owner(&pkg_json) { - if let Some(team) = teams_by_name.get(&owner) { - let package_path = parent_rel.join("package.json"); - let package_glob = format!("{rel_str}/**/**"); - return Some(( - team.name.clone(), - Source::Package(package_path.to_string_lossy().to_string(), package_glob), - )); - } - } + if pkg_json.exists() + && let Ok(owner) = read_js_package_owner(&pkg_json) + && let Some(team) = teams_by_name.get(&owner) + { + let package_path = parent_rel.join("package.json"); + let package_glob = format!("{rel_str}/**/**"); + return Some(( + team.name.clone(), + Source::Package(package_path.to_string_lossy().to_string(), package_glob), + )); } } } diff --git a/src/ownership/mapper/package_mapper.rs b/src/ownership/mapper/package_mapper.rs index 5d7dc76..351dfd6 100644 --- a/src/ownership/mapper/package_mapper.rs +++ b/src/ownership/mapper/package_mapper.rs @@ -122,10 +122,10 @@ fn remove_nested_packages<'a>(packages: &'a [&'a Package]) -> Vec<&'a Package> { for package in packages.iter().sorted_by_key(|package| package.package_root()) { if let Some(last_package) = top_level_packages.last() { - if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) { - if !current_root.starts_with(last_root) { - top_level_packages.push(package); - } + if let (Some(current_root), Some(last_root)) = (package.package_root(), last_package.package_root()) + && !current_root.starts_with(last_root) + { + top_level_packages.push(package); } } else { top_level_packages.push(package); diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 6612e73..dd89f66 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -74,13 +74,13 @@ impl Validator { .files .par_iter() .flat_map(|file| { - if let Some(owner) = &file.owner { - if !team_names.contains(owner) { - return Some(Error::InvalidTeam { - name: owner.clone(), - path: project.relative_path(&file.path).to_owned(), - }); - } + if let Some(owner) = &file.owner + && !team_names.contains(owner) + { + return Some(Error::InvalidTeam { + name: owner.clone(), + path: project.relative_path(&file.path).to_owned(), + }); } None diff --git a/src/project_builder.rs b/src/project_builder.rs index 3fecf05..9560333 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -61,14 +61,13 @@ impl<'a> ProjectBuilder<'a> { builder.filter_entry(move |entry: &DirEntry| { let path = entry.path(); let file_name = entry.file_name().to_str().unwrap_or(""); - if let Some(ft) = entry.file_type() { - if ft.is_dir() { - if let Ok(rel) = path.strip_prefix(&base_path) { - if rel.components().count() == 1 && ignore_dirs.iter().any(|d| *d == file_name) { - return false; - } - } - } + if let Some(ft) = entry.file_type() + && ft.is_dir() + && let Ok(rel) = path.strip_prefix(&base_path) + && rel.components().count() == 1 + && ignore_dirs.iter().any(|d| *d == file_name) + { + return false; } true @@ -92,10 +91,10 @@ impl<'a> ProjectBuilder<'a> { let _ = tx.send(entry_type); } Err(report) => { - if let Ok(mut slot) = error_holder.lock() { - if slot.is_none() { - *slot = Some(report); - } + if let Ok(mut slot) = error_holder.lock() + && slot.is_none() + { + *slot = Some(report); } } }