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 bbd2099..8a6ed99 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`], 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, 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(), + executable_name: self.project.executable_name.clone(), + }; + + 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 e7362d8..f235036 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -1,8 +1,8 @@ -use crate::project::{Project, ProjectFile}; +use crate::project::{Package, 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,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()); + validation_errors.append(&mut self.validate_invalid_team(&files, &packages)); debug!("validate_file_ownership"); - validation_errors.append(&mut self.validate_file_ownership()); + 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(()) @@ -56,24 +63,122 @@ impl Validator { } } + /// 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. 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 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 + .files + .iter() + .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(); + + // 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"); + validation_errors.append(&mut self.validate_invalid_team(&files, &packages)); + + debug!("validate_file_ownership"); + validation_errors.append(&mut self.validate_file_ownership(&requested_paths)); + + if validation_errors.is_empty() { + Ok(()) + } else { + Err(Errors(validation_errors)) + } + } + + /// 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) -> 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)); - errors.append(&mut self.invalid_package_ownership(&team_names)); + errors.append(&mut self.invalid_team_annotation(&team_names, files)); + errors.append(&mut self.invalid_package_ownership(&team_names, packages)); 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 @@ -90,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) { @@ -108,17 +212,17 @@ impl Validator { } #[instrument(name = "validate_file_ownership", level = "debug", skip_all)] - fn validate_file_ownership(&self) -> Vec { + fn validate_file_ownership(&self, relative_paths: &[&Path]) -> Vec { let mut validation_errors = Vec::new(); - for (file, owners) in self.file_to_owners() { - 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, }) } @@ -128,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 { @@ -142,22 +246,22 @@ impl Validator { } } - #[instrument(name = "file_to_owners", level = "debug", skip_all)] - fn file_to_owners(&self) -> Vec<(&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(); - self.project - .files + relative_paths .par_iter() - .filter_map(|project_file| { - let relative_path = project.relative_path(&project_file.path); - let owners = file_owner_finder.find(relative_path); - Some((project_file, owners)) - }) + .map(|relative_path| (*relative_path, file_owner_finder.find(relative_path))) .collect() } } @@ -266,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 a5c30cf..5645f7f 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -142,77 +142,76 @@ 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. + // 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. 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. - .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. .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(); - 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(); + // 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(); - return RunResult { - validation_errors, - io_errors, - ..Default::default() - }; + // 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(); } - if !io_errors.is_empty() { - return RunResult { - io_errors, + 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 { + 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..1ec073f --- /dev/null +++ b/tests/validate_files_parity_test.rs @@ -0,0 +1,331 @@ +//! 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. +//! +//! 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. +//! +//! 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. +//! +//! Normalization of the supplied paths themselves is covered separately, in +//! `supplied_path_normalization_test.rs`. + +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(()) +} + +/// Rewrite the fixture's one package manifest to name a team that does not exist, and +/// return the repo. Used by the package-scoping tests below. +fn fixture_with_an_invalid_package_owner() -> (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] +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. + // + // 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); + + 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("ruby/app/orphan_new.rb") + .assert() + .failure() + .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 47f21fe..9f2e745 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -21,22 +21,21 @@ 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"], 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(()) @@ -44,14 +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, - predicate::str::contains("ruby/app/unowned.rb"), + 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(()) @@ -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"],