From 83996f897e248d9b7a15f5755930339741458d34 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 21 Aug 2026 09:13:44 -0500 Subject: [PATCH 1/3] test: expose the validate/gv parity gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passing file paths swaps validate_all() for validate_files() (runner.rs:124). validate_all runs three checks — validate_invalid_team, validate_file_ownership, validate_codeowners_file (validator.rs:40-57). validate_files runs none of them; it only asks whether each path resolves to a team when reading the CODEOWNERS file. gv regenerates before validating, which cures staleness by construction, but not the other two. Worse, regenerating writes a dual-owned file into CODEOWNERS under one of its owners, so the per-path check then sees an owner and passes. Regenerating conceals that defect rather than exposing it. Five tests, all failing, all #[ignore]d so the suite stays green: - gv exits 0 with no output, twice over — once for annotation vs .codeowner, once for annotation vs owned_gems. They travel through different mappers, so a fix could catch one and miss the other. - gv fails, but reports "unowned" instead of naming the nonexistent team, sending the developer after the wrong problem. - gv with every owned path disagrees with gv with no paths about which defects exist. This is the general form, and needs no knowledge of what the fixture contains. - validate exits 0 having never checked the file. cli.rs canonicalizes --project-root, so a /var/... path fails strip_prefix against a /private/var/... root, stays absolute, and is then dropped by the owned_globs filter. Silent, and it fails in the unsafe direction. The last one is unrelated to the parity gap and predates it — it dates to the owned_globs filter added by #89 for #88. No production code changes. Run with: cargo test --test validate_files_parity_test -- --ignored Co-Authored-By: Claude Fable 5 --- tests/validate_files_parity_test.rs | 205 ++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 tests/validate_files_parity_test.rs diff --git a/tests/validate_files_parity_test.rs b/tests/validate_files_parity_test.rs new file mode 100644 index 0000000..174948e --- /dev/null +++ b/tests/validate_files_parity_test.rs @@ -0,0 +1,205 @@ +//! Parity between `validate` / `gv` with an explicit file list and the same command with +//! no file list. +//! +//! Passing paths swaps `validate_all()` for `validate_files()` (`runner.rs:124`). +//! `validate_all` runs three checks -- `validate_invalid_team`, `validate_file_ownership`, +//! `validate_codeowners_file` (`validator.rs:40-57`). `validate_files` runs none of them; +//! it only asks whether each path resolves to a team when reading the CODEOWNERS file. +//! +//! `gv ` regenerates before validating, which cures staleness by construction, but +//! not the other two. Worse, regenerating makes a dual-owned file *appear* owned, so the +//! per-path check waves it through. +//! +//! EVERY TEST IN THIS FILE CURRENTLY FAILS, so all are `#[ignore]`d to keep the suite green. +//! They assert the behavior we want and exist to document the gap. Run them with: +//! +//! ```sh +//! cargo test --test validate_files_parity_test -- --ignored +//! ``` +//! +//! Remove the `#[ignore]` attributes as each is fixed. + +use assert_cmd::prelude::*; +use predicates::prelude::*; +use std::{error::Error, process::Command}; + +mod common; + +use common::*; + +/// The `invalid_project` fixture carries one defect of each class. Full `validate` reports +/// all of them; see `tests/invalid_project_test.rs`. +const FIXTURE: &str = "tests/fixtures/invalid_project"; + +#[test] +#[ignore = "documents the validate/gv parity gap; remove when fixed"] +fn test_gv_with_paths_detects_dual_ownership_via_codeowner_file() -> Result<(), Box> { + // `ruby/app/services/multi_owned.rb` is owned twice: a `@team Payments` annotation and + // `ruby/app/services/.codeowner` naming Payroll. Full `gv` reports "Code ownership + // should only be defined for each file in one way". + // + // BUG: `gv` regenerates first, which writes the file into CODEOWNERS as @PaymentTeam. + // The per-path check then finds an owner and exits 0. A false pass -- the commit is + // waved through with genuinely ambiguous ownership. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("gv") + .arg("ruby/app/services/multi_owned.rb") + .assert() + .failure() + .stdout(predicate::str::contains("multi_owned.rb").and(predicate::str::contains("one way"))); + + Ok(()) +} + +#[test] +#[ignore = "documents the validate/gv parity gap; remove when fixed"] +fn test_gv_with_paths_detects_dual_ownership_via_owned_gems() -> Result<(), Box> { + // Same class, different source: `gems/payroll_calculator/calculator.rb` has a + // `@team Payments` annotation while Payroll claims it through `owned_gems`. + // + // BUG: same false pass. Included separately because the two travel through different + // mappers, so a fix could plausibly catch one and miss the other. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("gv") + .arg("gems/payroll_calculator/calculator.rb") + .assert() + .failure() + .stdout(predicate::str::contains("calculator.rb").and(predicate::str::contains("one way"))); + + Ok(()) +} + +#[test] +#[ignore = "documents the validate/gv parity gap; remove when fixed"] +fn test_gv_with_paths_names_the_invalid_team() -> Result<(), Box> { + // `ruby/app/models/blockchain.rb` is annotated `@team Web3`, which is not a team. Full + // `gv` reports "is referencing an invalid team - 'Web3'". + // + // BUG: this one does fail, but for the wrong reason. An invalid team yields no owner, so + // the file is absent from the generated CODEOWNERS and gets reported as merely "unowned". + // The actual fault -- a typo'd team name -- is never named, so the developer goes looking + // for missing ownership instead of fixing the annotation. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("gv") + .arg("ruby/app/models/blockchain.rb") + .assert() + .failure() + .stdout(predicate::str::contains("Web3")); + + Ok(()) +} + +#[test] +#[ignore = "documents the validate/gv parity gap; remove when fixed"] +fn test_gv_with_every_path_matches_gv_with_no_paths() -> Result<(), Box> { + // The differential check: handing over every owned file should be equivalent to handing + // over none. This is the general form of the three tests above -- it needs no knowledge + // of which defects the fixture contains, so it keeps working as fixtures change. + // + // BUG: the no-paths run reports dual ownership and the invalid team; the all-paths run + // reports neither. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + // owned_globs for this fixture is `**/*.{rb,tsx}`. + let tracked = Command::new("git").arg("ls-files").current_dir(project_root).output()?; + let owned_files: Vec = String::from_utf8(tracked.stdout)? + .lines() + .filter(|line| line.ends_with(".rb") || line.ends_with(".tsx")) + .map(str::to_string) + .collect(); + assert!(!owned_files.is_empty(), "fixture should contain owned files"); + + let no_paths = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("gv") + .output()?; + + let all_paths = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("gv") + .args(&owned_files) + .output()?; + + // Compare the defects each run found, not byte-for-byte output: the two use different + // report formats, and only the substance is being claimed here. + let no_paths_out = String::from_utf8_lossy(&no_paths.stdout); + let all_paths_out = String::from_utf8_lossy(&all_paths.stdout); + + for defect in ["one way", "Web3"] { + assert_eq!( + no_paths_out.contains(defect), + all_paths_out.contains(defect), + "`gv` with no paths and `gv` with every path disagree about {:?}.\n\ + \n--- no paths (exit {:?}) ---\n{}\n--- every path (exit {:?}) ---\n{}", + defect, + no_paths.status.code(), + no_paths_out, + all_paths.status.code(), + all_paths_out, + ); + } + + Ok(()) +} + +#[test] +#[ignore = "documents the validate/gv parity gap; remove when fixed"] +fn test_validate_does_not_silently_skip_absolute_paths() -> Result<(), Box> { + // Unrelated to the parity gap above, and the most dangerous of the set because it is + // completely silent. + // + // `cli.rs` canonicalizes `--project-root`. On macOS the temp dir is under `/var`, which + // canonicalizes to `/private/var`, so a caller-supplied `/var/...` path fails + // `strip_prefix`, stays absolute, and is then rejected by the `owned_globs` filter -- + // dropped before any ownership query runs. Exit 0, no output, file never checked. + // + // valid_project is used here because its owned_globs are directory-anchored + // (`{gems,config,javascript,ruby,components}/**`). With a `**`-leading glob the same path + // survives the filter and is reported spuriously unowned instead, so the symptom is + // config-dependent while the cause is the same. + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/valid_project")); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + // Deliberately NOT canonicalized -- that is the bug. + let absolute = project_root.join("ruby/app/unowned.rb"); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(absolute.to_str().unwrap()) + .assert() + .failure() + .stdout(predicate::str::contains("unowned.rb")); + + Ok(()) +} From df20d4b0083db7b2ff43fa0ac1bddcf8ccd46952 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 21 Aug 2026 10:39:29 -0500 Subject: [PATCH 2/3] fix: run the real ownership checks when paths are supplied MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_files answered only "does this path have an owner in the CODEOWNERS file". That could not see the two defects validate_all catches per file: - A file owned two ways. Generation picks one winner and writes it, so reading CODEOWNERS back finds an owner and passes. `gv ` exited 0 with empty output — regenerating first concealed the defect instead of exposing it. - An annotation naming a nonexistent team. That yields no owner, so the file was absent from the generated CODEOWNERS and reported as merely "unowned", sending the developer after missing ownership rather than a typo'd team. Now ownership for supplied paths is resolved through the mappers, the same way the whole-project run does. Validator gains a scoped entry point that runs validate_invalid_team and validate_file_ownership over just the named files, so a caller pays O(changed files) rather than O(repo). Both checks were already per-file — validate_file_ownership iterates file_to_owners(), which is a par_iter over project.files — so scoping them is a filter, not a rewrite. The mappers are built either way, by the project build both paths already pay. Package ownership is checked in full regardless of the path list. Packages are orders of magnitude fewer than files, and skipping them would leave a second blind spot. Staleness is still not checked for a supplied path list, and cannot be: it compares the whole generated file against the whole on-disk one. `gv ` makes it moot by regenerating first. A team file or .codeowner change can therefore still alter ownership of files outside the changeset without being caught — that gap wants an escalation path, which this commit does not add. One behavior change worth noting: unowned files supplied by path now report as "Some files are missing ownership", the same wording the whole-project run uses, rather than "Unowned files detected:". Same defect, same words, whether or not paths are passed — which is the point. Three test assertions updated for the new wording, and test_validate_only_checks_codeowners_file is renamed, since it documented the very behavior this removes. Absolute paths now render project-relative rather than as the caller wrote them, because the validator reports relative paths. Four of the five parity tests from the previous commit now pass and are un-ignored. The fifth stays ignored: non-canonical absolute paths are still dropped by the owned_globs filter before any check runs, which is a separate pre-existing bug. Co-Authored-By: Claude Fable 5 --- src/ownership.rs | 18 ++++++- src/ownership/validator.rs | 83 +++++++++++++++++++++++------ src/runner.rs | 74 +++++++++++-------------- tests/validate_files_parity_test.rs | 57 ++++++++++---------- tests/validate_files_test.rs | 24 +++++---- 5 files changed, 159 insertions(+), 97 deletions(-) diff --git a/src/ownership.rs b/src/ownership.rs index bbd2099..c964444 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -4,7 +4,7 @@ use mapper::{OwnerMatcher, Source, TeamName}; use std::{ error::Error, fmt::{self, Display}, - path::Path, + path::{Path, PathBuf}, sync::Arc, }; use tracing::{info, instrument}; @@ -129,6 +129,22 @@ impl Ownership { validator.validate() } + /// Like [`Ownership::validate`], but restricted to the supplied project-relative + /// paths. Skips the staleness check, which cannot be scoped — see + /// [`Validator::validate_files`]. + #[instrument(name = "ownership_validate_files", level = "debug", skip_all)] + pub fn validate_files(&self, relative_paths: &[PathBuf]) -> Result<(), ValidatorErrors> { + info!("validating file ownership for {} supplied paths", relative_paths.len()); + let validator = Validator { + project: self.project.clone(), + mappers: self.mappers(), + file_generator: FileGenerator { mappers: self.mappers() }, + executable_name: self.project.executable_name.clone(), + }; + + validator.validate_files(relative_paths) + } + #[instrument(level = "debug", skip_all)] pub fn for_file(&self, file_path: &Path) -> Result, ValidatorErrors> { info!("getting file ownership for {}", file_path.display()); diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index e7362d8..8b75b8c 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -2,7 +2,7 @@ use crate::project::{Project, ProjectFile}; use core::fmt; use std::collections::HashSet; use std::fmt::Display; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use itertools::Itertools; @@ -39,12 +39,13 @@ impl Validator { #[instrument(name = "validator_validate", level = "debug", skip_all)] pub fn validate(&self) -> Result<(), Errors> { let mut validation_errors = Vec::new(); + let files: Vec<&ProjectFile> = self.project.files.iter().collect(); debug!("validate_invalid_team"); - validation_errors.append(&mut self.validate_invalid_team()); + validation_errors.append(&mut self.validate_invalid_team(&files)); debug!("validate_file_ownership"); - validation_errors.append(&mut self.validate_file_ownership()); + validation_errors.append(&mut self.validate_file_ownership(&files)); debug!("validate_codeowners_file"); validation_errors.append(&mut self.validate_codeowners_file()); @@ -56,24 +57,77 @@ impl Validator { } } + /// Validation restricted to `relative_paths`. + /// + /// Runs the same per-file checks as [`Validator::validate`] — invalid team + /// annotations and file ownership — over just the named files, so a caller with a + /// changeset pays O(changed files) rather than O(repo). Ownership is resolved + /// through the mappers, exactly as the whole-project run does, so a file owned two + /// ways is reported rather than silently resolving to whichever owner happened to + /// win in the generated CODEOWNERS. + /// + /// The staleness check is deliberately absent: it compares the entire generated + /// file against the entire on-disk one and cannot be scoped. `generate_and_validate` + /// makes it moot by regenerating first; a caller that needs it on its own must run + /// [`Validator::validate`]. + /// + /// Package ownership is checked in full regardless of the path list — packages are + /// orders of magnitude fewer than files, and skipping them would leave a second + /// blind spot. + #[instrument(name = "validate_scoped", level = "debug", skip_all)] + pub fn validate_files(&self, relative_paths: &[PathBuf]) -> Result<(), Errors> { + let requested: HashSet<&Path> = relative_paths.iter().map(PathBuf::as_path).collect(); + + let files: Vec<&ProjectFile> = self + .project + .files + .iter() + .filter(|file| requested.contains(self.project.relative_path(&file.path))) + .collect(); + + let mut validation_errors = Vec::new(); + + // A requested path the project never walked cannot be attributed to a team. + // Report it as unowned, which is what a whole-project run says about any file + // it can't attribute. + let known: HashSet<&Path> = files.iter().map(|file| self.project.relative_path(&file.path)).collect(); + validation_errors.extend( + relative_paths + .iter() + .filter(|path| !known.contains(path.as_path())) + .map(|path| Error::FileWithoutOwner { path: path.clone() }), + ); + + debug!("validate_invalid_team (scoped)"); + validation_errors.append(&mut self.validate_invalid_team(&files)); + + debug!("validate_file_ownership (scoped)"); + validation_errors.append(&mut self.validate_file_ownership(&files)); + + if validation_errors.is_empty() { + Ok(()) + } else { + Err(Errors(validation_errors)) + } + } + #[instrument(name = "validate_invalid_team", level = "debug", skip_all)] - fn validate_invalid_team(&self) -> Vec { + fn validate_invalid_team(&self, files: &[&ProjectFile]) -> Vec { debug!("validating project"); let mut errors: Vec = Vec::new(); let team_names: HashSet<&TeamName> = self.project.teams.iter().map(|team| &team.name).collect(); - errors.append(&mut self.invalid_team_annotation(&team_names)); + errors.append(&mut self.invalid_team_annotation(&team_names, files)); errors.append(&mut self.invalid_package_ownership(&team_names)); errors } - fn invalid_team_annotation(&self, team_names: &HashSet<&String>) -> Vec { + fn invalid_team_annotation(&self, team_names: &HashSet<&String>, files: &[&ProjectFile]) -> Vec { let project = self.project.clone(); - self.project - .files + files .par_iter() .flat_map(|file| { if let Some(owner) = &file.owner @@ -108,10 +162,10 @@ impl Validator { } #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] - fn validate_file_ownership(&self) -> Vec { + fn validate_file_ownership(&self, files: &[&ProjectFile]) -> Vec { let mut validation_errors = Vec::new(); - for (file, owners) in self.file_to_owners() { + for (file, owners) in self.file_to_owners(files) { let relative_path = self.project.relative_path(&file.path).to_owned(); if owners.is_empty() { @@ -143,20 +197,19 @@ impl Validator { } #[instrument(name = "file_to_owners", level = "debug", skip_all)] - fn file_to_owners(&self) -> Vec<(&ProjectFile, Vec)> { + fn file_to_owners<'a>(&'a self, files: &[&'a ProjectFile]) -> Vec<(&'a ProjectFile, Vec)> { let owner_matchers: Vec = self.mappers.iter().flat_map(|mapper| mapper.owner_matchers()).collect(); let file_owner_finder = FileOwnerFinder { owner_matchers: &owner_matchers, }; let project = self.project.clone(); - self.project - .files + files .par_iter() - .filter_map(|project_file| { + .map(|project_file| { let relative_path = project.relative_path(&project_file.path); let owners = file_owner_finder.find(relative_path); - Some((project_file, owners)) + (*project_file, owners) }) .collect() } diff --git a/src/runner.rs b/src/runner.rs index 5562979..2c619a9 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -142,58 +142,48 @@ impl Runner { } } + /// Validate just the supplied paths. + /// + /// This resolves ownership through the mappers, the same way [`Runner::validate_all`] + /// does, rather than by reading the generated CODEOWNERS back. Reading it back could + /// only ever answer "does this path have an owner" — it could not see a file owned two + /// ways (generation picks one winner, so the file looks owned) nor name an annotation + /// referencing a nonexistent team (which yields no owner, so the file merely looked + /// unowned). + /// + /// Staleness is not checked here; it is a property of the whole CODEOWNERS file. + /// `generate_and_validate` makes it moot by regenerating first. fn validate_files(&self, file_paths: Vec) -> RunResult { - let mut unowned_files = Vec::new(); - let mut io_errors = Vec::new(); - - // Filter files based on owned_globs and unowned_globs configuration - // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec = file_paths + // Mirror the filtering applied by ProjectBuilder when walking the project, so a + // path the project would never have considered is not reported as unowned. + // + // Relativize with the same helper the project uses, so these paths match the + // form `Project::relative_path` produces. + let relative_paths: Vec = file_paths .into_iter() - .filter(|file_path| { - // Convert to relative path for glob matching - let path = Path::new(file_path); - let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) - } else { - path - }; + .filter_map(|file_path| { + let relative_path = crate::path_utils::relative_to(&self.run_config.project_root, Path::new(&file_path)); - // Mirror the filtering applied by ProjectBuilder when walking the project - matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) + if matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) { + Some(relative_path.to_path_buf()) + } else { + None + } }) .collect(); - debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { - match team_for_file_from_codeowners(&self.run_config, &file_path) { - Ok(Some(_)) => {} - Ok(None) => unowned_files.push(file_path), - Err(err) => io_errors.push(format!("{}: {}", file_path, err)), - } - } - }); - - if !unowned_files.is_empty() { - let validation_errors = std::iter::once("Unowned files detected:".to_string()) - .chain(unowned_files.into_iter().map(|file| format!(" {}", file))) - .collect(); - - return RunResult { - validation_errors, - io_errors, - ..Default::default() - }; + if relative_paths.is_empty() { + return RunResult::default(); } - if !io_errors.is_empty() { - return RunResult { - io_errors, + match self.ownership.validate_files(&relative_paths) { + Ok(_) => RunResult::default(), + Err(err) => RunResult { + info_messages: err.info_messages(), + validation_errors: vec![format!("{}", err)], ..Default::default() - }; + }, } - - RunResult::default() } pub fn generate(&self, git_stage: bool) -> RunResult { diff --git a/tests/validate_files_parity_test.rs b/tests/validate_files_parity_test.rs index 174948e..73d2a23 100644 --- a/tests/validate_files_parity_test.rs +++ b/tests/validate_files_parity_test.rs @@ -1,23 +1,25 @@ //! Parity between `validate` / `gv` with an explicit file list and the same command with //! no file list. //! -//! Passing paths swaps `validate_all()` for `validate_files()` (`runner.rs:124`). -//! `validate_all` runs three checks -- `validate_invalid_team`, `validate_file_ownership`, -//! `validate_codeowners_file` (`validator.rs:40-57`). `validate_files` runs none of them; -//! it only asks whether each path resolves to a team when reading the CODEOWNERS file. +//! Passing paths routes through `validate_files()` instead of `validate_all()` +//! (`runner.rs:124`). Both resolve ownership through the mappers, so both catch an +//! invalid team annotation and a file owned two ways; `validate_files` simply scopes the +//! per-file checks to the supplied paths. //! -//! `gv ` regenerates before validating, which cures staleness by construction, but -//! not the other two. Worse, regenerating makes a dual-owned file *appear* owned, so the -//! per-path check waves it through. +//! Only the staleness check differs, and unavoidably so: it compares the whole generated +//! CODEOWNERS against the whole on-disk one, so it cannot be scoped to a subset. +//! `gv ` makes it moot by regenerating first. //! -//! EVERY TEST IN THIS FILE CURRENTLY FAILS, so all are `#[ignore]`d to keep the suite green. -//! They assert the behavior we want and exist to document the gap. Run them with: +//! These previously all failed. `validate_files` used to answer only "does this path have +//! an owner in the CODEOWNERS file", which could not see a file owned two ways — generation +//! picks one winner, so the file looked owned and the command exited 0. They are kept as +//! regression guards against reintroducing that shortcut. +//! +//! One test remains `#[ignore]`d for a separate, still-unfixed bug. Run it with: //! //! ```sh //! cargo test --test validate_files_parity_test -- --ignored //! ``` -//! -//! Remove the `#[ignore]` attributes as each is fixed. use assert_cmd::prelude::*; use predicates::prelude::*; @@ -32,15 +34,14 @@ use common::*; const FIXTURE: &str = "tests/fixtures/invalid_project"; #[test] -#[ignore = "documents the validate/gv parity gap; remove when fixed"] fn test_gv_with_paths_detects_dual_ownership_via_codeowner_file() -> Result<(), Box> { // `ruby/app/services/multi_owned.rb` is owned twice: a `@team Payments` annotation and // `ruby/app/services/.codeowner` naming Payroll. Full `gv` reports "Code ownership // should only be defined for each file in one way". // - // BUG: `gv` regenerates first, which writes the file into CODEOWNERS as @PaymentTeam. - // The per-path check then finds an owner and exits 0. A false pass -- the commit is - // waved through with genuinely ambiguous ownership. + // Regression guard. This used to exit 0 with empty output: `gv` regenerates first, + // writing the file into CODEOWNERS under @PaymentTeam, so a check that read CODEOWNERS + // back found an owner and passed. Regeneration concealed the defect. let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -59,13 +60,12 @@ fn test_gv_with_paths_detects_dual_ownership_via_codeowner_file() -> Result<(), } #[test] -#[ignore = "documents the validate/gv parity gap; remove when fixed"] fn test_gv_with_paths_detects_dual_ownership_via_owned_gems() -> Result<(), Box> { // Same class, different source: `gems/payroll_calculator/calculator.rb` has a // `@team Payments` annotation while Payroll claims it through `owned_gems`. // - // BUG: same false pass. Included separately because the two travel through different - // mappers, so a fix could plausibly catch one and miss the other. + // Regression guard, same false pass. Kept separate because the two travel through + // different mappers, so a regression could reappear in one and not the other. let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -84,15 +84,13 @@ fn test_gv_with_paths_detects_dual_ownership_via_owned_gems() -> Result<(), Box< } #[test] -#[ignore = "documents the validate/gv parity gap; remove when fixed"] fn test_gv_with_paths_names_the_invalid_team() -> Result<(), Box> { // `ruby/app/models/blockchain.rb` is annotated `@team Web3`, which is not a team. Full // `gv` reports "is referencing an invalid team - 'Web3'". // - // BUG: this one does fail, but for the wrong reason. An invalid team yields no owner, so - // the file is absent from the generated CODEOWNERS and gets reported as merely "unowned". - // The actual fault -- a typo'd team name -- is never named, so the developer goes looking - // for missing ownership instead of fixing the annotation. + // Regression guard. This used to fail, but for the wrong reason: an invalid team yields + // no owner, so the file was absent from the generated CODEOWNERS and reported as merely + // "unowned", sending the developer after missing ownership instead of a typo'd team. let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -111,14 +109,14 @@ fn test_gv_with_paths_names_the_invalid_team() -> Result<(), Box> { } #[test] -#[ignore = "documents the validate/gv parity gap; remove when fixed"] fn test_gv_with_every_path_matches_gv_with_no_paths() -> Result<(), Box> { // The differential check: handing over every owned file should be equivalent to handing // over none. This is the general form of the three tests above -- it needs no knowledge // of which defects the fixture contains, so it keeps working as fixtures change. // - // BUG: the no-paths run reports dual ownership and the invalid team; the all-paths run - // reports neither. + // Regression guard, and the most valuable of the set: it needs no knowledge of the + // fixture's contents, so it keeps working as fixtures change. The all-paths run used to + // report neither the dual ownership nor the invalid team. let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -147,8 +145,9 @@ fn test_gv_with_every_path_matches_gv_with_no_paths() -> Result<(), Box Result<(), Box Result<(), Box> { // Unrelated to the parity gap above, and the most dangerous of the set because it is // completely silent. diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index 541b5bf..c2dee1c 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -26,7 +26,9 @@ fn test_validate_with_unowned_file() -> Result<(), Box> { &["validate", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + // Same wording a whole-project `validate` uses for an unattributable file -- + // supplying paths no longer produces a separate "Unowned files detected:" format. + predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("missing ownership")), )?; Ok(()) @@ -39,7 +41,9 @@ fn test_validate_with_mixed_files() -> Result<(), Box> { &["validate", "ruby/app/models/payroll.rb", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + // Same wording a whole-project `validate` uses for an unattributable file -- + // supplying paths no longer produces a separate "Unowned files detected:" format. + predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("missing ownership")), )?; Ok(()) @@ -97,7 +101,7 @@ fn test_generate_and_validate_with_unowned_file() -> Result<(), Box> .assert() .failure() .stdout(predicate::str::contains("ruby/app/unowned.rb")) - .stdout(predicate::str::contains("Unowned")); + .stdout(predicate::str::contains("missing ownership")); Ok(()) } @@ -124,14 +128,14 @@ fn test_validate_with_absolute_path() -> Result<(), Box> { } #[test] -fn test_validate_only_checks_codeowners_file() -> Result<(), Box> { - // This test demonstrates that `validate` with files only checks the CODEOWNERS file - // It does NOT check file annotations or other ownership sources +fn test_validate_with_paths_resolves_ownership_through_mappers() -> Result<(), Box> { + // Ownership for a supplied path is resolved through the mappers, not by reading the + // generated CODEOWNERS back. This test used to assert the opposite -- that `validate` + // with files consulted only the CODEOWNERS file and ignored annotations -- which is + // exactly the weakness that let a dual-owned file pass. // - // If a file has an annotation but is missing from CODEOWNERS, `validate` will report it as unowned - // This is why `generate-and-validate` should be used for accuracy - - // ruby/app/models/bank_account.rb has @team Payments annotation and is in CODEOWNERS + // ruby/app/models/bank_account.rb has a @team Payments annotation and is in CODEOWNERS, + // so it is owned exactly once and validates cleanly either way. run_codeowners( "valid_project", &["validate", "ruby/app/models/bank_account.rb"], From da9e047e4f7e431113e8bd37ff579b9400c0927c Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Fri, 21 Aug 2026 10:58:15 -0500 Subject: [PATCH 3/3] fix: address review of the scoped validation path Four things, all found by reviewing the previous commit rather than by a test failing. Deduplicate absent paths. The missing-path branch iterated the raw argument list, so `validate ghost.rb ghost.rb` reported the file twice. It now iterates the deduped set. Paths the project does know about were already deduped for free, by going through project.files, so only this branch was inconsistent. Correct the complexity claim. The previous commit said the scoped path costs O(changed files) rather than O(repo). That is wrong: file_to_owners builds the owner matchers from every mapper, and TeamFileMapper::owner_matchers enumerates every annotated file in the project. The real cost is a fixed O(repo) term plus O(supplied paths x matchers), against O(repo x matchers) for the whole-project run. The saving is in the variable term, which dominates on a large repo with a small changeset -- but it is not free, and it is still unmeasured. Make the unused FileGenerator impossible rather than merely unused. The scoped constructor built one and never used it, since nothing there generates. The generator is now a parameter of Validator::validate instead of a field, so the scoped path cannot be handed one. Also documents why the whole-project path builds mappers twice: FileGenerator owns them and Box is not Clone. Rename the scoped span to validator_validate_scoped, paralleling validator_validate, and drop the "(scoped)" suffixes from the debug! lines. The per-check spans are shared between both paths, so a profile distinguishes them by parent span; #121 added those span names precisely to stop distinct work collapsing into one bucket, and that reasoning applies here too. Help text for `validate ` now states that staleness is not checked and points at running without files or using generate-and-validate. It previously advertised "fast mode for git hooks" with no hint it checks less. This is where the mental model forms; a note on stdout was rejected because successful runs are silent by contract and pre-commit hooks depend on that. Co-Authored-By: Claude Fable 5 --- src/cli.rs | 9 +++++-- src/ownership.rs | 12 ++++++---- src/ownership/validator.rs | 49 +++++++++++++++++++++++++++----------- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 1d0a7f9..b735c7d 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -39,7 +39,10 @@ enum Command { visible_alias = "v" )] Validate { - #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")] + #[arg(help = "Optional list of files to validate ownership for (for git hooks). Checks ownership of \ + just these files. Does NOT check whether the CODEOWNERS file itself is up to date -- \ + that is a property of the whole file. Run without files, or use generate-and-validate, \ + to catch a stale CODEOWNERS.")] files: Vec, }, @@ -47,7 +50,9 @@ enum Command { GenerateAndValidate { #[arg(long, short, default_value = "false", help = "Skip staging the CODEOWNERS file")] skip_stage: bool, - #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")] + #[arg(help = "Optional list of files to validate ownership for (for git hooks). Checks ownership of \ + just these files. Staleness is covered regardless, since the CODEOWNERS file is \ + regenerated first.")] files: Vec, }, diff --git a/src/ownership.rs b/src/ownership.rs index c964444..542a344 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -122,23 +122,27 @@ impl Ownership { let validator = Validator { project: self.project.clone(), mappers: self.mappers(), - file_generator: FileGenerator { mappers: self.mappers() }, executable_name: self.project.executable_name.clone(), }; - validator.validate() + // A second set of mappers, because FileGenerator owns rather than borrows them + // and `Box` is not Clone. Construction is trivial (each `build` just + // stores an Arc); the O(repo) work happens in `owner_matchers`/`entries`. + let file_generator = FileGenerator { mappers: self.mappers() }; + + validator.validate(&file_generator) } /// Like [`Ownership::validate`], but restricted to the supplied project-relative /// paths. Skips the staleness check, which cannot be scoped — see - /// [`Validator::validate_files`]. + /// [`Validator::validate_files`]. Builds no `FileGenerator`, since nothing here + /// generates. #[instrument(name = "ownership_validate_files", level = "debug", skip_all)] pub fn validate_files(&self, relative_paths: &[PathBuf]) -> Result<(), ValidatorErrors> { info!("validating file ownership for {} supplied paths", relative_paths.len()); let validator = Validator { project: self.project.clone(), mappers: self.mappers(), - file_generator: FileGenerator { mappers: self.mappers() }, executable_name: self.project.executable_name.clone(), }; diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 8b75b8c..9bcbd4f 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -20,7 +20,6 @@ use super::mapper::{Mapper, OwnerMatcher, TeamName}; pub struct Validator { pub project: Arc, pub mappers: Vec>, - pub file_generator: FileGenerator, pub executable_name: String, } @@ -36,8 +35,13 @@ enum Error { pub struct Errors(Vec); impl Validator { + /// Whole-project validation. + /// + /// The `FileGenerator` is a parameter rather than a field so that + /// [`Validator::validate_files`], which cannot check staleness, is structurally + /// incapable of being handed one it would never use. #[instrument(name = "validator_validate", level = "debug", skip_all)] - pub fn validate(&self) -> Result<(), Errors> { + pub fn validate(&self, file_generator: &FileGenerator) -> Result<(), Errors> { let mut validation_errors = Vec::new(); let files: Vec<&ProjectFile> = self.project.files.iter().collect(); @@ -48,7 +52,7 @@ impl Validator { validation_errors.append(&mut self.validate_file_ownership(&files)); debug!("validate_codeowners_file"); - validation_errors.append(&mut self.validate_codeowners_file()); + validation_errors.append(&mut self.validate_codeowners_file(file_generator)); if validation_errors.is_empty() { Ok(()) @@ -60,12 +64,19 @@ impl Validator { /// Validation restricted to `relative_paths`. /// /// Runs the same per-file checks as [`Validator::validate`] — invalid team - /// annotations and file ownership — over just the named files, so a caller with a - /// changeset pays O(changed files) rather than O(repo). Ownership is resolved + /// annotations and file ownership — over just the named files. Ownership is resolved /// through the mappers, exactly as the whole-project run does, so a file owned two /// ways is reported rather than silently resolving to whichever owner happened to /// win in the generated CODEOWNERS. /// + /// This scopes the *per-file* work, not all of it. Building the owner matchers is + /// still O(repo): `TeamFileMapper::owner_matchers` enumerates every annotated file + /// in the project. So the cost is a fixed O(repo) term plus a variable + /// O(supplied paths × matchers) term, where the whole-project run pays + /// O(repo × matchers) for the latter. The saving is in the variable term, which + /// dominates on a large repo with a small changeset — but it is not free, and it has + /// not been measured. + /// /// The staleness check is deliberately absent: it compares the entire generated /// file against the entire on-disk one and cannot be scoped. `generate_and_validate` /// makes it moot by regenerating first; a caller that needs it on its own must run @@ -73,8 +84,14 @@ impl Validator { /// /// Package ownership is checked in full regardless of the path list — packages are /// orders of magnitude fewer than files, and skipping them would leave a second - /// blind spot. - #[instrument(name = "validate_scoped", level = "debug", skip_all)] + /// blind spot. A consequence worth knowing: validating one file can fail over a + /// package that file has nothing to do with. + /// + /// The per-check spans (`validate_invalid_team`, `validate_file_ownership`) are + /// shared with the whole-project run, so a profile tells the two apart by parent + /// span — `validator_validate_scoped` here, `validator_validate` there — not by the + /// child span name. + #[instrument(name = "validator_validate_scoped", level = "debug", skip_all)] pub fn validate_files(&self, relative_paths: &[PathBuf]) -> Result<(), Errors> { let requested: HashSet<&Path> = relative_paths.iter().map(PathBuf::as_path).collect(); @@ -90,18 +107,22 @@ impl Validator { // A requested path the project never walked cannot be attributed to a team. // Report it as unowned, which is what a whole-project run says about any file // it can't attribute. + // + // Iterate the deduped set rather than the raw argument list: the same path + // supplied twice is one defect, not two. Paths the project *does* know about are + // deduped for free by going through `project.files`. let known: HashSet<&Path> = files.iter().map(|file| self.project.relative_path(&file.path)).collect(); validation_errors.extend( - relative_paths + requested .iter() - .filter(|path| !known.contains(path.as_path())) - .map(|path| Error::FileWithoutOwner { path: path.clone() }), + .filter(|path| !known.contains(**path)) + .map(|path| Error::FileWithoutOwner { path: path.to_path_buf() }), ); - debug!("validate_invalid_team (scoped)"); + debug!("validate_invalid_team"); validation_errors.append(&mut self.validate_invalid_team(&files)); - debug!("validate_file_ownership (scoped)"); + debug!("validate_file_ownership"); validation_errors.append(&mut self.validate_file_ownership(&files)); if validation_errors.is_empty() { @@ -182,8 +203,8 @@ impl Validator { } #[instrument(name = "validate_codeowners_file", level = "debug", skip_all)] - fn validate_codeowners_file(&self) -> Vec { - let generated_file = self.file_generator.generate_file(); + fn validate_codeowners_file(&self, file_generator: &FileGenerator) -> Vec { + let generated_file = file_generator.generate_file(); let current_file = self.project.get_codeowners_file().unwrap_or_default(); if generated_file == current_file {