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 bbd2099..542a344 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}; @@ -122,11 +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`]. 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(), + executable_name: self.project.executable_name.clone(), + }; + + validator.validate_files(relative_paths) } #[instrument(level = "debug", skip_all)] diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index e7362d8..9bcbd4f 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; @@ -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,18 +35,95 @@ 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(); 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()); + validation_errors.append(&mut self.validate_codeowners_file(file_generator)); + + if validation_errors.is_empty() { + Ok(()) + } else { + Err(Errors(validation_errors)) + } + } + + /// 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. 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 + /// [`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. 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(); + + 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. + // + // 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( + requested + .iter() + .filter(|path| !known.contains(**path)) + .map(|path| Error::FileWithoutOwner { path: path.to_path_buf() }), + ); + + debug!("validate_invalid_team"); + validation_errors.append(&mut self.validate_invalid_team(&files)); + + debug!("validate_file_ownership"); + validation_errors.append(&mut self.validate_file_ownership(&files)); if validation_errors.is_empty() { Ok(()) @@ -57,23 +133,22 @@ impl Validator { } #[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 +183,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() { @@ -128,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 { @@ -143,20 +218,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 new file mode 100644 index 0000000..73d2a23 --- /dev/null +++ b/tests/validate_files_parity_test.rs @@ -0,0 +1,204 @@ +//! Parity between `validate` / `gv` with an explicit file list and the same command with +//! no file list. +//! +//! 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. +//! +//! 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. +//! +//! 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 +//! ``` + +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] +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". + // + // 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); + + 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] +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`. + // + // 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); + + 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] +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'". + // + // 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); + + 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] +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. + // + // 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); + + // 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 rather than byte-for-byte output. The two now + // share a report format, but the no-paths run legitimately reports more (staleness, + // and files outside the supplied list), so only the shared substance is 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 = "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. + // + // 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(()) +} 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"],