From 879a1db54c9adeb77968ddd57e7958dd50b46d50 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 3acb8b1249a04190b25034bb46172dd5959b5ae9 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 | 25 +++++---- 5 files changed, 154 insertions(+), 103 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 a5c30cf..1932424 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -142,15 +142,22 @@ 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(); - // Normalize before anything else. A caller-supplied path has to be reduced to the - // project-relative form the rest of the pipeline speaks, or it silently matches - // nothing: `./ruby/app/x.rb`, and an absolute path that disagrees with the root - // about symlinks, were both dropped by the glob filter below, and the run then - // exited 0 having checked nothing -- a false pass in the unsafe direction. + // project-relative form the rest of the pipeline speaks, or the per-file checks + // below match nothing and the run exits 0 having checked nothing -- a false pass in + // the unsafe direction. // // The canonical root is resolved once rather than per path, since only the retry // inside `resolve_project_relative` needs it and that retry can fire for every path @@ -162,57 +169,28 @@ impl Runner { .filter_map(|file_path| { crate::path_utils::resolve_project_relative(&self.run_config.project_root, canonical_root.as_deref(), Path::new(file_path)) }) - // A path that no longer exists is dropped rather than reported. Changesets - // delete files routinely and `git diff --name-only` lists them, so reporting a - // deleted file as unowned fails a commit for removing code -- and a deleted - // file cannot have an owner. The wrapping `code_ownership` gem already filters - // its list by `File.exist?` before calling in; doing it here too covers callers - // that use the library directly. - // - // `unwrap_or(true)` because only a definite "this is not there" earns a silent - // skip. If the answer is unknown -- a permissions error, a broken symlink -- - // keep the path and let the check report it, because a visible error is - // investigable and a silent pass is not. + // A path that no longer exists is dropped rather than reported: a deleted file + // cannot have an owner, and changesets delete files routinely. .filter(|relative_path| self.run_config.project_root.join(relative_path).try_exists().unwrap_or(true)) - // Mirror the filtering applied by ProjectBuilder when walking the project. + // Mirror the filtering applied by ProjectBuilder when walking the project, so a + // path the project would never have considered is not reported as unowned. .filter(|relative_path| { matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) }) .collect(); - debug_span!("per_file_query").in_scope(|| { - for relative_path in relative_paths { - // Query with the normalized path rather than the caller's spelling, which - // made the query re-derive it using the same broken `strip_prefix`. - let file_path = relative_path.to_string_lossy().to_string(); - 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 47f21fe..bbe53b1 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -36,7 +36,9 @@ fn test_validate_with_unowned_file() -> Result<(), Box> { &["validate", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb"), + // 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(()) @@ -51,7 +53,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"), + // 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(()) @@ -110,7 +114,8 @@ fn test_generate_and_validate_with_unowned_file() -> Result<(), Box> .arg("ruby/app/unowned.rb") .assert() .failure() - .stdout(predicate::str::contains("ruby/app/unowned.rb")); + .stdout(predicate::str::contains("ruby/app/unowned.rb")) + .stdout(predicate::str::contains("missing ownership")); Ok(()) } @@ -137,14 +142,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 a32b81a83414689e0292f54571b9f9047cb09b2b Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Mon, 24 Aug 2026 15:03:14 -0500 Subject: [PATCH 3/3] fix: address review of the scoped validation path Three review passes over the scoped path, each of which found a silent false pass in the one before it. Grouped by theme rather than by pass, since the passes are not interesting on their own. Scope the package check. validate_invalid_team ran invalid_package_ownership over every package regardless of the path list, so `validate ` exited 1 over a package that file had nothing to do with. Since the gem's --diff mode feeds a changeset in, one pre-existing bad package owner would block every commit in the repo until someone fixed it. It was inconsistent too: the empty-path early return keyed off the glob-filtered list, so whether the unrelated error surfaced depended on whether some supplied path happened to match owned_globs -- `validate foo.rb` failed while `validate README.md` passed, same repo, same bad package. Packages are now selected by containing at least one supplied path. That needs two path lists, because a manifest must be able to select its own package while not being eligible to be reported unowned: owned_paths for the per-file checks, supplied_paths for package selection. Selecting on the wider list means editing a package.yml to name a nonexistent team is caught by the commit that does it, not only by a later commit that happens to touch a file inside that package. The early return is now a pure optimization. Normalize the paths callers supply. `./ruby/app/x.rb` exited 0 having checked nothing: it never matched a walked project file, and the owned_globs filter then dropped it. Same shape as the absolute-path bug that was #[ignore]d as pre-existing, and the same cause -- paths were compared without being reduced to the form Project::relative_path produces. path_utils::project_relative resolves `.` and `..` lexically and reports failure rather than passing an unstrippable path through. Lexically, not by canonicalizing: the walk records symlink paths rather than their targets, so resolving symlinks would match nothing. An absolute path only strips if it and the root agree about symlinks, and both sides can disagree. cli.rs canonicalizes --project-root but a library caller building its own RunConfig does not -- which is how the gem calls in -- so resolving only the path leaves the mirror-image case failing exactly as silently: root /var/..., path /private/var/..., dropped before any check runs. The retry resolves both sides. The first attempt uses the root as given, so relative paths cost no syscalls, and the root is resolved once per run. Skip paths that no longer exist. `validate ` exited 1 with "missing ownership". Changesets delete files routinely and git lists them, so this failed commits for removing code. A deleted file cannot have an owner. The gem already filters by File.exist? before calling in, so this matches what its callers see and extends it to direct library callers. Only a definite "not there" skips: try_exists().unwrap_or(true) keeps a path whose status is unknown, because a visible error is investigable and a silent pass is not. Resolve unwalked paths through the mappers instead of assuming them unowned. The walk only records git-tracked files, so a brand-new unstaged file was reported "Some files are missing ownership" even when a .codeowner in its directory owned it -- while for-file on the same path, resolving through the mappers, correctly named the team. Two commands in one binary disagreeing about one path, failing in the direction that rejects a commit for lacking an owner it has. That simplified rather than complicated: ownership resolution never needed ProjectFile, only the relative path, which is what the matchers key on. file_to_owners becomes path_to_owners and the separate "unwalked, therefore unowned" branch disappears instead of growing a special case. A genuinely unowned untracked file is still reported, which a test pins. The annotation check still runs only over walked files, since an untracked file's annotation has not been parsed -- narrower gap, left deliberately. Smaller things: deduplicate a path supplied twice, which was reported twice; make the unused FileGenerator structurally impossible in the scoped path by passing it to validate() rather than holding it as a field; name the scoped span validator_validate_scoped to parallel validator_validate; and rewrite the --help text for `files`, which promised "fast mode for git hooks" with no hint that it checks less. Three tests asserted on valid_project/ruby/app/unowned.rb, which does not exist -- valid_project has to validate cleanly, so it has no unowned file. They passed only because a nonexistent path was reported as unowned, meaning they covered typo handling while claiming to cover unowned files. Repointed at invalid_project, which has a real one. Adds unit coverage for Validator::validate_files, which was reachable only end-to-end through the binary, and asserts two things the scoping predicate silently depends on: starts_with is component-wise, so ruby/packages/foo must not select ruby/packages/foobar, and a root-level manifest has an empty relative root that prefixes every path. Measured, replacing the complexity claim the first pass corrected but left unverified. On a large monorepo (~130k files, ~18k-line CODEOWNERS; codeowners-perf, best of 3 warm): validation drops from 933ms whole-project to 28ms for one path and 56ms for 2000 -- near-flat in path count, the variable term collapsing as predicted. Wall clock only improves 3.0s to 2.0s, because the ~1.9s project build is the fixed term and is paid either way. So the files param is worth about a second at that scale, not an order of magnitude, which is the number the "should the fast path exist at all" question needed. Co-Authored-By: Claude Fable 5 --- src/cli.rs | 16 +- src/ownership.rs | 18 +- src/ownership/validator.rs | 320 +++++++++++++++++++++++----- src/runner.rs | 49 +++-- tests/validate_files_parity_test.rs | 187 +++++++++++++--- tests/validate_files_test.rs | 28 +-- 6 files changed, 496 insertions(+), 122 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 202114b..b1c9787 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -39,9 +39,12 @@ enum Command { visible_alias = "v" )] Validate { - #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks). Paths are \ - resolved relative to the project root; ones that no longer exist are skipped, so a \ - changeset that deletes files is not reported as unowned.")] + #[arg(help = "Optional list of files to validate ownership for (for git hooks). Checks ownership of \ + just these files, and of packages containing them. Paths are resolved relative to the \ + project root, and ones that no longer exist are skipped, so a changeset that deletes \ + files is not reported as unowned. 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, }, @@ -49,9 +52,10 @@ 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). Paths are \ - resolved relative to the project root; ones that no longer exist are skipped, so a \ - changeset that deletes files is not reported as unowned.")] + #[arg(help = "Optional list of files to validate ownership for (for git hooks). Checks ownership of \ + just these files, and of packages containing them. Paths are resolved relative to the \ + project root, and ones that no longer exist are skipped. 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..8a6ed99 100644 --- a/src/ownership.rs +++ b/src/ownership.rs @@ -122,27 +122,31 @@ 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`], which also documents the two path lists. 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()); + pub fn validate_files(&self, owned_paths: &[PathBuf], supplied_paths: &[PathBuf]) -> Result<(), ValidatorErrors> { + info!("validating file ownership for {} supplied paths", supplied_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) + validator.validate_files(owned_paths, supplied_paths) } #[instrument(level = "debug", skip_all)] diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 8b75b8c..f235036 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,4 +1,4 @@ -use crate::project::{Project, ProjectFile}; +use crate::project::{Package, Project, ProjectFile}; use core::fmt; use std::collections::HashSet; use std::fmt::Display; @@ -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,19 +35,26 @@ 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(); + let packages: Vec<&Package> = self.project.packages.iter().collect(); + let relative_paths: Vec<&Path> = files.iter().map(|file| self.project.relative_path(&file.path)).collect(); debug!("validate_invalid_team"); - validation_errors.append(&mut self.validate_invalid_team(&files)); + validation_errors.append(&mut self.validate_invalid_team(&files, &packages)); debug!("validate_file_ownership"); - validation_errors.append(&mut self.validate_file_ownership(&files)); + validation_errors.append(&mut self.validate_file_ownership(&relative_paths)); 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(()) @@ -57,26 +63,52 @@ impl Validator { } } - /// Validation restricted to `relative_paths`. + /// Validation restricted to the supplied 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. + /// + /// Measured on a large monorepo (~130k files, ~18k-line CODEOWNERS; `codeowners-perf`, + /// best of 3 warm): the variable term is what collapses — validation drops from 933ms + /// whole-project to 28ms for one path and 56ms for 2000, so it is near-flat in the + /// number of paths. Wall clock only improves 3.0s to 2.0s, because the ~1.9s project + /// build is the fixed term and is paid either way. Scoping is worth about a second on + /// a repo that size, not an order of magnitude. + /// /// 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(); + /// Package ownership is scoped too, to packages containing at least one supplied + /// path. Checking every package would mean validating one file can fail over a + /// package that file has nothing to do with — and since the gem's `--diff` mode feeds + /// a changeset in, a single pre-existing bad package owner would block every commit in + /// the repo until it was fixed. + /// + /// Hence the two path lists. `owned_paths` are the paths the project walk would have + /// considered, and are what the per-file checks run over. `supplied_paths` is + /// everything the caller named, including paths the walk skips — which is what puts a + /// package in scope, so that editing a `package.yml` into naming a nonexistent team is + /// caught by the commit that does it, not merely by a later commit that happens to + /// touch a file inside that package. + /// + /// 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, owned_paths: &[PathBuf], supplied_paths: &[PathBuf]) -> Result<(), Errors> { + let requested: HashSet<&Path> = owned_paths.iter().map(PathBuf::as_path).collect(); let files: Vec<&ProjectFile> = self .project @@ -85,24 +117,29 @@ impl Validator { .filter(|file| requested.contains(self.project.relative_path(&file.path))) .collect(); + let packages: Vec<&Package> = self + .project + .packages + .iter() + .filter(|package| self.package_contains_any(package, supplied_paths)) + .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() }), - ); + // Every requested path goes to the matchers, including ones the walk never + // recorded. An untracked file is the case that matters: it is absent from + // `project.files`, but the matchers can still attribute it -- a new file in a + // directory with a `.codeowner` is owned the moment it exists. Reporting such a + // path as unowned instead put `validate` at odds with `for-file` on the same path. + // + // Iterating the deduped set also means a path supplied twice is one defect. + let requested_paths: Vec<&Path> = requested.iter().copied().collect(); - debug!("validate_invalid_team (scoped)"); - validation_errors.append(&mut self.validate_invalid_team(&files)); + debug!("validate_invalid_team"); + validation_errors.append(&mut self.validate_invalid_team(&files, &packages)); - debug!("validate_file_ownership (scoped)"); - validation_errors.append(&mut self.validate_file_ownership(&files)); + debug!("validate_file_ownership"); + validation_errors.append(&mut self.validate_file_ownership(&requested_paths)); if validation_errors.is_empty() { Ok(()) @@ -111,15 +148,29 @@ impl Validator { } } + /// Whether any of `supplied_paths` lies inside `package`. + /// + /// The manifest itself counts, since it sits at the package root and so is prefixed by + /// it. A package at the project root has an empty relative root, which every path is + /// prefixed by — correctly, since it owns the whole tree. + fn package_contains_any(&self, package: &Package, supplied_paths: &[PathBuf]) -> bool { + let Some(package_root) = package.package_root() else { + return false; + }; + let package_root = self.project.relative_path(package_root); + + supplied_paths.iter().any(|path| path.starts_with(package_root)) + } + #[instrument(name = "validate_invalid_team", level = "debug", skip_all)] - fn validate_invalid_team(&self, files: &[&ProjectFile]) -> Vec { + fn validate_invalid_team(&self, files: &[&ProjectFile], packages: &[&Package]) -> 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, files)); - errors.append(&mut self.invalid_package_ownership(&team_names)); + errors.append(&mut self.invalid_package_ownership(&team_names, packages)); errors } @@ -144,9 +195,8 @@ impl Validator { .collect() } - fn invalid_package_ownership(&self, team_names: &HashSet<&String>) -> Vec { - self.project - .packages + fn invalid_package_ownership(&self, team_names: &HashSet<&String>, packages: &[&Package]) -> Vec { + packages .iter() .flat_map(|package| { if !team_names.contains(&package.owner) { @@ -162,17 +212,17 @@ impl Validator { } #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] - fn validate_file_ownership(&self, files: &[&ProjectFile]) -> Vec { + fn validate_file_ownership(&self, relative_paths: &[&Path]) -> Vec { let mut validation_errors = Vec::new(); - for (file, owners) in self.file_to_owners(files) { - let relative_path = self.project.relative_path(&file.path).to_owned(); - + for (relative_path, owners) in self.path_to_owners(relative_paths) { if owners.is_empty() { - validation_errors.push(Error::FileWithoutOwner { path: relative_path }) + validation_errors.push(Error::FileWithoutOwner { + path: relative_path.to_owned(), + }) } else if owners.len() > 1 { validation_errors.push(Error::FileWithMultipleOwners { - path: relative_path, + path: relative_path.to_owned(), owners, }) } @@ -182,8 +232,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 { @@ -196,21 +246,22 @@ impl Validator { } } - #[instrument(name = "file_to_owners", level = "debug", skip_all)] - fn file_to_owners<'a>(&'a self, files: &[&'a ProjectFile]) -> Vec<(&'a ProjectFile, Vec)> { + /// Resolve ownership for project-relative paths. + /// + /// Keyed on paths rather than `ProjectFile`s because that is all the matchers consume, + /// and because it lets a scoped run ask about a path the walk never recorded — an + /// untracked file, say. Answering those from the matchers is what makes + /// `validate ` agree with `for-file `; assuming they were unowned did not. + #[instrument(name = "path_to_owners", level = "debug", skip_all)] + fn path_to_owners<'a>(&self, relative_paths: &[&'a Path]) -> Vec<(&'a Path, 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(); - files + relative_paths .par_iter() - .map(|project_file| { - let relative_path = project.relative_path(&project_file.path); - let owners = file_owner_finder.find(relative_path); - (*project_file, owners) - }) + .map(|relative_path| (*relative_path, file_owner_finder.find(relative_path))) .collect() } } @@ -319,7 +370,174 @@ impl core::error::Error for Errors {} #[cfg(test)] mod tests { use super::*; + use crate::project::{PackageType, Team}; use indoc::indoc; + use std::collections::HashMap; + + const ROOT: &str = "/proj"; + + /// A validator over a synthetic project with no mappers. + /// + /// No mappers means no file resolves to an owner, so every file that makes it into + /// scope is reported as unowned. That is the point: it makes the *scoping* visible + /// without any ownership rules to reason about. `validate_files` is otherwise covered + /// only end-to-end through the binary, which cannot isolate the predicate. + fn validator(files: &[&str], packages: &[(&str, &str)], teams: &[&str]) -> Validator { + let project = Project { + base_path: PathBuf::from(ROOT), + files: files + .iter() + .map(|path| ProjectFile { + owner: None, + path: PathBuf::from(ROOT).join(path), + }) + .collect(), + packages: packages + .iter() + .map(|(path, owner)| Package { + path: PathBuf::from(ROOT).join(path), + package_type: PackageType::Ruby, + owner: (*owner).to_string(), + }) + .collect(), + vendored_gems: vec![], + teams: teams + .iter() + .map(|name| Team { + name: (*name).to_string(), + ..Default::default() + }) + .collect(), + codeowners_file_path: PathBuf::from(".github/CODEOWNERS"), + directory_codeowner_files: vec![], + teams_by_name: HashMap::new(), + executable_name: "codeowners".to_string(), + }; + + Validator { + project: Arc::new(project), + mappers: vec![], + executable_name: "codeowners".to_string(), + } + } + + fn paths(paths: &[&str]) -> Vec { + paths.iter().map(PathBuf::from).collect() + } + + #[test] + fn validate_files_reports_only_the_supplied_files() { + let validator = validator(&["ruby/a.rb", "ruby/b.rb"], &[], &[]); + + let errors = validator + .validate_files(&paths(&["ruby/a.rb"]), &paths(&["ruby/a.rb"])) + .expect_err("unowned file should be an error"); + let report = format!("{}", errors); + + assert!(report.contains("ruby/a.rb"), "{report}"); + assert!(!report.contains("ruby/b.rb"), "an unsupplied file leaked into scope: {report}"); + } + + #[test] + fn validate_files_reports_a_path_supplied_twice_once() { + let validator = validator(&[], &[], &[]); + + let errors = validator + .validate_files( + &paths(&["ruby/ghost.rb", "ruby/ghost.rb"]), + &paths(&["ruby/ghost.rb", "ruby/ghost.rb"]), + ) + .expect_err("an unwalked path should be an error"); + + assert_eq!(errors.0.len(), 1, "{errors:?}"); + } + + #[test] + fn validate_files_skips_a_package_containing_no_supplied_path() { + // The blast-radius case: one bad package owner elsewhere in the repo must not fail + // a run scoped to an unrelated file. + let validator = validator(&["ruby/app/a.rb"], &[("ruby/packages/foo/package.yml", "NoSuchTeam")], &["Payroll"]); + + let errors = validator + .validate_files(&paths(&["ruby/app/a.rb"]), &paths(&["ruby/app/a.rb"])) + .expect_err("the unowned file is still an error"); + let report = format!("{}", errors); + + assert!(!report.contains("NoSuchTeam"), "unrelated package leaked into scope: {report}"); + } + + #[test] + fn validate_files_reports_a_package_containing_a_supplied_path() { + let validator = validator( + &["ruby/packages/foo/app/a.rb"], + &[("ruby/packages/foo/package.yml", "NoSuchTeam")], + &["Payroll"], + ); + + let supplied = paths(&["ruby/packages/foo/app/a.rb"]); + let errors = validator + .validate_files(&supplied, &supplied) + .expect_err("bad package owner is an error"); + let report = format!("{}", errors); + + assert!(report.contains("NoSuchTeam"), "{report}"); + assert!(report.contains("ruby/packages/foo/package.yml"), "{report}"); + } + + #[test] + fn validate_files_reports_a_package_whose_manifest_is_itself_supplied() { + // A manifest does not match owned_globs, so it never appears in `owned_paths` -- + // it reaches the package check through `supplied_paths` only. Without this, editing + // a manifest to name a nonexistent team would not be caught by the commit doing it. + let validator = validator(&[], &[("ruby/packages/foo/package.yml", "NoSuchTeam")], &["Payroll"]); + + let errors = validator + .validate_files(&[], &paths(&["ruby/packages/foo/package.yml"])) + .expect_err("bad package owner is an error"); + let report = format!("{}", errors); + + assert!(report.contains("NoSuchTeam"), "{report}"); + assert!( + !report.contains("missing ownership"), + "a manifest is not eligible to be reported unowned: {report}" + ); + } + + #[test] + fn validate_files_does_not_select_a_sibling_package_by_name_prefix() { + // `starts_with` is component-wise, so `ruby/packages/foo` must not swallow + // `ruby/packages/foobar`. A plain string prefix check would. + let validator = validator(&[], &[("ruby/packages/foo/package.yml", "NoSuchTeam")], &["Payroll"]); + + assert!( + validator.validate_files(&[], &paths(&["ruby/packages/foobar/app/a.rb"])).is_ok(), + "a sibling package sharing a name prefix was selected" + ); + } + + #[test] + fn validate_files_selects_a_package_at_the_project_root() { + // A root-level manifest has an empty relative package root, and every path is + // prefixed by the empty path -- correctly, since it owns the whole tree. Asserted + // because the scoping predicate silently depends on it. + let validator = validator(&[], &[("package.yml", "NoSuchTeam")], &["Payroll"]); + + let errors = validator + .validate_files(&[], &paths(&["ruby/app/anything.rb"])) + .expect_err("a root-level package owns every path"); + + assert!(format!("{}", errors).contains("NoSuchTeam")); + } + + #[test] + fn validate_files_accepts_a_valid_package_owner() { + let validator = validator(&[], &[("ruby/packages/foo/package.yml", "Payroll")], &["Payroll"]); + + assert!( + validator.validate_files(&[], &paths(&["ruby/packages/foo/package.yml"])).is_ok(), + "a package owned by a real team is not an error" + ); + } #[test] fn test_codeowners_diff_reports_added_and_removed_lines() { diff --git a/src/runner.rs b/src/runner.rs index 1932424..5645f7f 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -154,39 +154,60 @@ impl Runner { /// 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 { - // Normalize before anything else. A caller-supplied path has to be reduced to the - // project-relative form the rest of the pipeline speaks, or the per-file checks - // below match nothing and the run exits 0 having checked nothing -- a false pass in - // the unsafe direction. + // Normalize before anything else. `./app/x.rb`, `app/x.rb` and an absolute path to + // the same file all have to reduce to the form `Project::relative_path` produces, + // or the per-file checks below match nothing and the run exits 0 having checked + // nothing -- a false pass in the unsafe direction. See + // `path_utils::resolve_project_relative` for why an absolute path needs both sides + // resolved, and why only its parent is. + // + // A path that no longer exists is dropped rather than reported: changesets delete + // files routinely, and a deleted file cannot have an owner, so reporting it as + // unowned would fail a commit for removing code. // // The canonical root is resolved once rather than per path, since only the retry // inside `resolve_project_relative` needs it and that retry can fire for every path // when a caller passes an absolute list. let canonical_root = self.run_config.project_root.canonicalize().ok(); - let relative_paths: Vec = file_paths + let supplied_paths: Vec = file_paths .iter() .filter_map(|file_path| { crate::path_utils::resolve_project_relative(&self.run_config.project_root, canonical_root.as_deref(), Path::new(file_path)) }) - // A path that no longer exists is dropped rather than reported: a deleted file - // cannot have an owner, and changesets delete files routinely. - .filter(|relative_path| self.run_config.project_root.join(relative_path).try_exists().unwrap_or(true)) - // Mirror the filtering applied by ProjectBuilder when walking the project, so a - // path the project would never have considered is not reported as unowned. .filter(|relative_path| { - matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) + // `unwrap_or(true)` on purpose: only a definite "this is not there" earns a + // silent skip. If the answer is unknown -- a permissions error, a bad + // symlink -- keep the path and let the checks report it, because a visible + // error is investigable and a silent pass is not. + self.run_config.project_root.join(relative_path).try_exists().unwrap_or(true) }) .collect(); - if relative_paths.is_empty() { + // Mirror the filtering ProjectBuilder applies when walking, so a path the project + // would never have considered is not reported as unowned. This is narrower than + // `supplied_paths`: a supplied package.yml or README.md is not itself an ownership + // defect, but it does put its package in scope for the check below. + let owned_paths: Vec = supplied_paths + .iter() + .filter(|path| matches_globs(path, &self.config.owned_globs) && !matches_globs(path, &self.config.unowned_globs)) + .cloned() + .collect(); + + // Purely an optimization -- it skips building the mappers. With no paths in scope + // the validator finds no files and no packages and returns Ok regardless, so this + // is not a semantic special case. It used to be one: when the early return keyed + // off the glob-filtered list, whether an unrelated package error surfaced depended + // on whether some supplied path happened to match owned_globs. + if supplied_paths.is_empty() { return RunResult::default(); } - match self.ownership.validate_files(&relative_paths) { + match self.ownership.validate_files(&owned_paths, &supplied_paths) { Ok(_) => RunResult::default(), + // No `info_messages`: the only error carrying one is the stale-CODEOWNERS diff, + // which a scoped run cannot produce. Err(err) => RunResult { - info_messages: err.info_messages(), validation_errors: vec![format!("{}", err)], ..Default::default() }, diff --git a/tests/validate_files_parity_test.rs b/tests/validate_files_parity_test.rs index 73d2a23..1ec073f 100644 --- a/tests/validate_files_parity_test.rs +++ b/tests/validate_files_parity_test.rs @@ -6,20 +6,21 @@ //! invalid team annotation and a file owned two ways; `validate_files` simply scopes the //! per-file checks to the supplied paths. //! -//! 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. +//! Two things differ, both deliberately. The staleness check is unavoidable: it compares +//! the whole generated CODEOWNERS against the whole on-disk one, so it cannot be scoped to +//! a subset, and `gv ` makes it moot by regenerating first. The package check is +//! scoped to packages containing a supplied path, so that one bad package owner elsewhere +//! in the repo does not fail every scoped run; the `*_invalid_package*` tests pin both +//! halves of that, and the `*_untracked_*` pair pins that a path the walk never recorded is +//! asked about rather than assumed unowned. //! -//! 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. +//! The `gv_*` tests 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 -//! ``` +//! Normalization of the supplied paths themselves is covered separately, in +//! `supplied_path_normalization_test.rs`. use assert_cmd::prelude::*; use predicates::prelude::*; @@ -168,37 +169,163 @@ fn test_gv_with_every_path_matches_gv_with_no_paths() -> Result<(), Box (tempfile::TempDir, std::path::PathBuf) { + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path().to_path_buf(); + + let manifest = project_root.join("ruby/packages/payroll_flow/package.yml"); + assert!(manifest.exists(), "fixture should contain a package manifest"); + std::fs::write(&manifest, "owner: NoSuchTeam\n").expect("failed to write package manifest"); + + // A file inside that package, so the in-scope case has something to name. + let inside = project_root.join("ruby/packages/payroll_flow/app/thing.rb"); + std::fs::create_dir_all(inside.parent().unwrap()).expect("failed to create package dir"); + std::fs::write(&inside, "# inside the badly-owned package\n").expect("failed to write file"); + + git_add_all_files(&project_root); + (temp_dir, project_root) +} + #[test] -#[ignore = "separate pre-existing bug: owned_globs filter drops non-canonical absolute paths"] -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. +fn test_validate_with_paths_ignores_an_unrelated_invalid_package() -> Result<(), Box> { + // The package check is scoped to packages containing a supplied path. Checking every + // package meant validating one file could fail over a package that file had nothing to + // do with -- and since the gem's `--diff` mode feeds a changeset in, one pre-existing + // bad package owner would block every commit in the repo until someone fixed it. + let (_temp_dir, project_root) = fixture_with_an_invalid_package_owner(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(&project_root) + .arg("--no-cache") + .arg("validate") + .arg("ruby/app/models/bank_account.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_validate_with_paths_reports_an_invalid_package_containing_a_supplied_path() -> Result<(), Box> { + // The other half of the scoping: in scope means reported. Without this, scoping the + // package check would just be a blind spot. + let (_temp_dir, project_root) = fixture_with_an_invalid_package_owner(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(&project_root) + .arg("--no-cache") + .arg("validate") + .arg("ruby/packages/payroll_flow/app/thing.rb") + .assert() + .failure() + .stdout(predicate::str::contains("package.yml").and(predicate::str::contains("NoSuchTeam"))); + + Ok(()) +} + +#[test] +fn test_validate_with_paths_reports_a_supplied_package_manifest() -> Result<(), Box> { + // Supplying the manifest itself has to work, or the scoped check would never catch a + // bad package owner at the moment it is introduced -- only later, via some unrelated + // commit that happened to touch a file inside that package. The manifest does not match + // `owned_globs`, so it is not eligible to be reported as unowned; it is in scope purely + // as a package selector. + let (_temp_dir, project_root) = fixture_with_an_invalid_package_owner(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(&project_root) + .arg("--no-cache") + .arg("validate") + .arg("ruby/packages/payroll_flow/package.yml") + .assert() + .failure() + .stdout( + predicate::str::contains("package.yml") + .and(predicate::str::contains("NoSuchTeam")) + .and(predicate::str::contains("missing ownership").not()), + ); + + Ok(()) +} + +#[test] +fn test_validate_with_no_paths_still_reports_every_invalid_package() -> Result<(), Box> { + // Scoping applies only to the scoped run. The whole-project run must keep reporting + // every bad package, including the one the tests above deliberately do not name. + let (_temp_dir, project_root) = fixture_with_an_invalid_package_owner(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(&project_root) + .arg("--no-cache") + .arg("validate") + .assert() + .failure() + .stdout(predicate::str::contains("package.yml").and(predicate::str::contains("NoSuchTeam"))); + + Ok(()) +} + +#[test] +fn test_validate_attributes_an_untracked_file_through_the_mappers() -> Result<(), Box> { + // A brand-new file, not yet staged, in a directory carrying a `.codeowner`. It is + // absent from `project.files` (the walk only records git-tracked files), but it is + // genuinely owned -- `.codeowner` owns the directory, so the file is owned the moment + // it exists. // - // 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")); + // This used to report "Some files are missing ownership": paths the walk had not + // recorded were assumed unowned rather than asked about. That put two commands in the + // same binary at odds on the same path, since `for-file` resolves through the mappers + // and correctly answered Payroll. It also failed in the annoying direction -- a + // pre-commit hook rejecting a file for lacking an owner it does have. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + // `ruby/app/services/.codeowner` names Payroll. Written after `git add`, so untracked. + let untracked = project_root.join("ruby/app/services/brand_new.rb"); + std::fs::write(&untracked, "# no annotation; owned by the directory\n")?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg("ruby/app/services/brand_new.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_validate_still_reports_an_untracked_file_with_no_owner() -> Result<(), Box> { + // The other side of the test above: resolving unwalked paths through the mappers must + // not turn into "unwalked paths always pass". `ruby/app/` has no `.codeowner`, so a new + // file there is genuinely unowned and must still be reported. + let temp_dir = setup_fixture_repo(std::path::Path::new(FIXTURE)); 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"); + let untracked = project_root.join("ruby/app/orphan_new.rb"); + std::fs::write(&untracked, "# nobody owns this\n")?; Command::cargo_bin("codeowners")? .arg("--project-root") .arg(project_root) .arg("--no-cache") .arg("validate") - .arg(absolute.to_str().unwrap()) + .arg("ruby/app/orphan_new.rb") .assert() .failure() - .stdout(predicate::str::contains("unowned.rb")); + .stdout(predicate::str::contains("orphan_new.rb").and(predicate::str::contains("missing ownership"))); Ok(()) } diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index bbe53b1..9f2e745 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -21,16 +21,13 @@ fn test_validate_with_owned_files() -> Result<(), Box> { #[test] fn test_validate_with_unowned_file() -> Result<(), Box> { - // `invalid_project`, not `valid_project`: this needs a file that genuinely has no + // `invalid_project`, not `valid_project`: this test needs a file that genuinely has no // owner, and `valid_project/ruby/app/unowned.rb` does not exist -- by design, since - // `test_validate_with_no_files` requires that fixture to validate cleanly. Pointed at - // the nonexistent path, this test passed only because a nonexistent path was reported - // as unowned, so it was really covering typo handling while claiming to cover unowned - // files. Now that a path which no longer exists is skipped, that accident is gone. - // `invalid_project/ruby/app/unowned.rb` is a real file with no owner. - // - // Asserts the path and the exit status, not the category wording, so it stays valid - // however the report is phrased. + // `test_validate_with_no_files` requires that fixture to validate cleanly. Asserted + // against the nonexistent path, this test used to pass only because a nonexistent path + // was reported as unowned, so it was really covering typo handling while claiming to + // cover unowned files. `invalid_project/ruby/app/unowned.rb` is a real file with no + // owner. run_codeowners( "invalid_project", &["validate", "ruby/app/unowned.rb"], @@ -46,16 +43,19 @@ fn test_validate_with_unowned_file() -> Result<(), Box> { #[test] fn test_validate_with_mixed_files() -> Result<(), Box> { - // One owned file and one genuinely unowned one; see `test_validate_with_unowned_file` - // for why this uses `invalid_project`. + // One owned file and one genuinely unowned one; see the note in + // `test_validate_with_unowned_file` for why this uses `invalid_project`. The scoping + // matters here too: `invalid_project` also holds a dual-owned file and an invalid team + // annotation, and neither is named below, so neither should be reported. run_codeowners( "invalid_project", &["validate", "ruby/app/models/payroll.rb", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - // 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")), + predicate::str::contains("ruby/app/unowned.rb") + .and(predicate::str::contains("missing ownership")) + .and(predicate::str::contains("multi_owned.rb").not()) + .and(predicate::str::contains("Web3").not()), )?; Ok(())