diff --git a/src/cli.rs b/src/cli.rs index 1d0a7f9..202114b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -39,7 +39,9 @@ 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 (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.")] files: Vec, }, @@ -47,7 +49,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 (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.")] files: Vec, }, diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index 562bb48..9dff3a7 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -10,15 +10,23 @@ pub(crate) fn teams_for_files_from_codeowners( team_file_globs: &[String], file_paths: &[String], ) -> Result>, String> { + // Normalize the same way `Runner::validate_files` does. This is reached from public API + // (`runner::teams_for_files_from_codeowners`) and had the same defect: + // `relative_to_buf` passes an unstrippable path through unchanged, so an absolute path + // that disagreed with `project_root` about symlinks -- a `/var/...` path against a + // `/private/var/...` root -- was looked up in the CODEOWNERS file *as an absolute path*, + // matched no entry, and came back unowned. + // + // Falls back to the path as given rather than dropping it, because the returned map is + // contracted to hold one entry per input and `team_for_file_from_codeowners` asserts on + // that. A path that cannot be placed inside the project has no owner, which is the + // honest answer for a lookup. + let canonical_root = project_root.canonicalize().ok(); let relative_file_paths: Vec = file_paths .iter() .map(Path::new) .map(|path| { - if path.is_absolute() { - crate::path_utils::relative_to_buf(project_root, path) - } else { - path.to_path_buf() - } + crate::path_utils::resolve_project_relative(project_root, canonical_root.as_deref(), path).unwrap_or_else(|| path.to_path_buf()) }) .collect(); diff --git a/src/path_utils.rs b/src/path_utils.rs index 230b1d1..d58f126 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; /// Return `path` relative to `root` if possible; otherwise return `path` unchanged. pub fn relative_to<'a>(root: &'a Path, path: &'a Path) -> &'a Path { @@ -10,6 +10,103 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// Reduce a caller-supplied `path` to the project-relative form that +/// [`crate::project::Project::relative_path`] produces for walked files. +/// +/// Unlike [`relative_to`], which passes an unstrippable path through unchanged, this +/// reports failure. A path that cannot be placed inside the project is not a path the +/// per-file checks can say anything about, and silently treating it as relative is how +/// `/var/...` came to be compared against project-relative paths and matched nothing. +/// +/// Purely lexical — no filesystem access, so it is safe on a path that no longer exists +/// (a deleted file in a changeset). `.` components are dropped and `..` pops the +/// preceding component, so `./a/b.rb` and `a/c/../b.rb` both reduce to `a/b.rb`. +/// +/// Returns `None` when `path` is absolute and does not lie under `root`, when it escapes +/// `root` via `..`, or when it *is* `root`. The absolute case is not necessarily final: +/// `cli.rs` canonicalizes `--project-root`, so on macOS a root of `/private/var/...` will +/// not strip a caller-supplied `/var/...`. A caller that gets `None` for an absolute path +/// should retry with a canonicalized copy. +pub fn project_relative(root: &Path, path: &Path) -> Option { + let relative = if path.is_absolute() { path.strip_prefix(root).ok()? } else { path }; + + let normalized = lexically_normalize(relative); + if normalized.as_os_str().is_empty() || normalized.starts_with("..") { + return None; + } + + Some(normalized) +} + +/// Like [`project_relative`], but consults the filesystem when the lexical attempt fails. +/// +/// An absolute path only strips if it and `root` agree about symlinks, and there is no +/// guarantee they do. `cli.rs` canonicalizes `--project-root`, but a library caller building +/// its own `RunConfig` (which is how the `code_ownership` gem calls in) does not. So on +/// macOS, where `TMPDIR` lives under `/var`, a symlink to `/private/var`, *either* side can +/// be the unresolved one, and in a symlinked checkout the same is true generally. Resolving +/// only one side leaves the other failing exactly as silently, so the retry resolves both. +/// +/// It resolves the **parent** and re-attaches the file name, rather than canonicalizing the +/// whole path. Canonicalizing the leaf would follow a symlinked *file*, and the project walk +/// records the symlink path rather than its target — so an absolute path naming a symlink +/// would be checked as a different file than the caller asked about, and pass or fail on +/// that file's ownership instead. A symlinked *ancestor* is still resolved, unavoidably: +/// that is the whole point in the `/var` case, and the walk does not follow symlinked +/// directories anyway, so such a path names no walked file under either spelling. +/// +/// `canonical_root` is the resolved `root`, passed in rather than computed so a caller +/// normalizing a whole changeset pays for it once instead of once per path. +pub fn resolve_project_relative(root: &Path, canonical_root: Option<&Path>, path: &Path) -> Option { + if let Some(relative) = project_relative(root, path) { + return Some(relative); + } + + // Only an absolute path can be rescued. A relative path is interpreted against the + // project root by contract -- that is what `--help` promises -- and the lexical pass is + // the whole of that interpretation, so failure means it escapes the root. Retrying would + // resolve it against the process CWD instead, quietly switching interpretation frames: + // the same arguments would then mean different files depending on where the command was + // run from. It also spends a syscall per path to reach that wrong answer. + if !path.is_absolute() { + return None; + } + + let resolved = path.parent()?.canonicalize().ok()?.join(path.file_name()?); + + project_relative(canonical_root.unwrap_or(root), &resolved) +} + +/// Resolve `.` and `..` without touching the filesystem. +/// +/// Deliberately lexical: canonicalizing would also resolve symlinks, and the project walk +/// records the symlink path rather than its target, so resolving here would produce a path +/// that matches no walked file. +fn lexically_normalize(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + // A `..` that cannot pop is retained, so the caller can detect the escape. + // + // A retained `..` must never itself be popped by a later one: `pop()` does + // not distinguish it from a real component, so `../../a` cancelled its own + // escape and came out as `a` -- reporting an out-of-project path as though + // it named a file inside the project. + let escaped = matches!(normalized.components().next_back(), Some(Component::ParentDir)); + if escaped || !normalized.pop() { + normalized.push(Component::ParentDir); + } + } + other => normalized.push(other), + } + } + + normalized +} + #[cfg(test)] mod tests { use super::*; @@ -46,4 +143,78 @@ mod tests { let rel_buf = relative_to_buf(root, path); assert_eq!(rel_ref, rel_buf.as_path()); } + + #[test] + fn project_relative_passes_through_a_plain_relative_path() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_strips_a_leading_dot_slash() { + // `./a.rb` and `a.rb` name the same file, but only one of them used to match a + // walked project file -- the other was silently dropped by the owned_globs filter. + let rel = project_relative(Path::new("/proj"), Path::new("./ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_resolves_interior_parent_dirs() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/services/../app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_strips_the_root_from_an_absolute_path() { + let rel = project_relative(Path::new("/proj"), Path::new("/proj/ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_rejects_an_absolute_path_outside_the_root() { + // The caller retries with the parent resolved; see `resolve_project_relative`. + assert_eq!(project_relative(Path::new("/private/proj"), Path::new("/proj/a.rb")), None); + } + + #[test] + fn project_relative_rejects_a_path_escaping_the_root() { + assert_eq!(project_relative(Path::new("/proj"), Path::new("../outside/a.rb")), None); + } + + #[test] + fn project_relative_rejects_a_path_escaping_via_repeated_parent_dirs() { + // `pop()` does not distinguish a retained `..` from a real component, so this used to + // cancel its own escape and come out as `ruby/app/a.rb` -- an out-of-project path + // silently reported as though it named a file inside the project. + assert_eq!(project_relative(Path::new("/proj"), Path::new("../../ruby/app/a.rb")), None); + assert_eq!(project_relative(Path::new("/proj"), Path::new("../../../a.rb")), None); + } + + #[test] + fn project_relative_rejects_a_path_that_climbs_back_out() { + // Interior `..` still pops normally; the escape only has to survive once it starts. + assert_eq!(project_relative(Path::new("/proj"), Path::new("ruby/../../a.rb")), None); + } + + #[test] + fn project_relative_pops_interior_parent_dirs_without_escaping() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/app/models/../../app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn resolve_project_relative_does_not_touch_the_filesystem_for_a_relative_path() { + // A relative path is project-root-relative by contract, so the lexical pass is the + // whole interpretation. Retrying would resolve it against the process CWD, making + // the same arguments mean different files depending on where the command was run. + assert_eq!( + resolve_project_relative(Path::new("/proj"), Some(Path::new("/proj")), Path::new("../outside/a.rb")), + None + ); + } + + #[test] + fn project_relative_rejects_the_root_itself() { + assert_eq!(project_relative(Path::new("/proj"), Path::new("/proj")), None); + } } diff --git a/src/runner.rs b/src/runner.rs index 5562979..a5c30cf 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -146,26 +146,45 @@ impl Runner { 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 - .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 - }; - - // Mirror the filtering applied by ProjectBuilder when walking the project + // 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. + // + // 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 + .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) }) .collect(); debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { + 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), diff --git a/tests/supplied_path_normalization_test.rs b/tests/supplied_path_normalization_test.rs new file mode 100644 index 0000000..9c215ba --- /dev/null +++ b/tests/supplied_path_normalization_test.rs @@ -0,0 +1,331 @@ +//! Normalization of the paths a caller supplies to `validate` / `generate-and-validate`. +//! +//! A supplied path has to be reduced to the project-relative form the rest of the pipeline +//! speaks. When it is not, what happens depends on the shape of `owned_globs`, and both +//! outcomes are wrong: +//! +//! - **Directory-anchored globs** (`{gems,ruby,...}/**/*.rb`, as in `valid_project`): the +//! mishandled path matches nothing, is dropped before any ownership query runs, and the +//! command exits 0 having checked nothing. Silent, and in the unsafe direction — a hook +//! or CI job reports success on a file it never looked at. +//! - **`**`-leading globs** (`**/*.rb`, as in `invalid_project`): the mishandled path +//! survives the filter and is queried in the caller's spelling, which matches no +//! CODEOWNERS entry, so a perfectly well-owned file is reported unowned. +//! +//! Most tests here use `valid_project` and assert *failure* on a genuinely unowned file, +//! because under anchored globs a dropped path shows up as a pass. They also assert the +//! report names the **normalized** path rather than echoing the caller's spelling — without +//! that, a test cannot tell "checked correctly" from "mishandled and spuriously reported", +//! which is exactly how an earlier draft of this file passed against unfixed code. +//! +//! One test covers the `**`-glob direction, since the symptom there is the opposite. Two +//! more assert an *owned* file still passes under each odd spelling — without those, every +//! assertion here would also hold if normalization mangled one path into some other unowned +//! path. +//! +//! Path forms covered, against both states the project root can be in (resolved or not, +//! since `cli.rs` canonicalizes it but a library caller need not): +//! +//! - `./a/b.rb`, `a/c/../b.rb` -- leading `.`, interior `..` +//! - absolute, root and path agreeing about symlinks +//! - absolute, root resolved and path not +//! - absolute, path resolved and root not (library callers only) +//! - absolute, naming a symlinked *file* -- must check the symlink, not its target +//! - a deleted path, and a path outside the project -- both skipped, deliberately + +use assert_cmd::prelude::*; +use codeowners::runner::{self, RunConfig}; +use predicates::prelude::*; +use std::{error::Error, process::Command}; +use tempfile::TempDir; + +mod common; + +use common::*; + +/// The project-relative form every spelling below must reduce to. +const NORMALIZED: &str = "ruby/app/unowned.rb"; + +/// `valid_project` with a genuinely unowned file added. +/// +/// The fixture ships without one on purpose — `test_validate_with_no_files` requires it to +/// validate cleanly — so the file is injected here rather than committed. Its `owned_globs` +/// are directory-anchored, which is what makes a mishandled path get dropped, so that +/// asserting failure below is a real signal. +fn fixture_with_an_unowned_file() -> TempDir { + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/valid_project")); + std::fs::write(temp_dir.path().join(NORMALIZED), "# nobody owns this\n").expect("failed to write unowned file"); + git_add_all_files(temp_dir.path()); + temp_dir +} + +/// Assert `validate ` succeeds, i.e. the path reached the check *and* resolved to +/// a file that really is owned. +/// +/// The counterpart to `assert_normalizes`: those tests would still pass if normalization +/// mangled a path into some *other* unowned path, and these would not. +fn assert_owned_file_passes(spelling: &str) -> Result<(), Box> { + let temp_dir = fixture_with_an_unowned_file(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(temp_dir.path()) + .arg("--no-cache") + .arg("validate") + .arg(spelling) + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +/// Assert `validate ` reached the ownership check and reported the file under its +/// normalized name. +fn assert_normalizes(spelling_from: impl Fn(&std::path::Path) -> String) -> Result<(), Box> { + let temp_dir = fixture_with_an_unowned_file(); + let project_root = temp_dir.path(); + let spelling = spelling_from(project_root); + + let output = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(&spelling) + .output()?; + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!( + !output.status.success(), + "`validate {spelling}` exited 0 -- the path was dropped before any ownership query ran.\nstdout={stdout}" + ); + assert!( + stdout.contains(NORMALIZED), + "report does not name the normalized path `{NORMALIZED}`.\nstdout={stdout}" + ); + if spelling != NORMALIZED { + assert!( + !stdout.contains(&spelling), + "report echoes the caller's spelling `{spelling}` instead of normalizing it, \ + which means the path reached the query unnormalized.\nstdout={stdout}" + ); + } + + Ok(()) +} + +#[test] +fn test_plain_relative_path() -> Result<(), Box> { + // The control. If this fails, none of the others mean what they claim. + assert_normalizes(|_| NORMALIZED.to_string()) +} + +#[test] +fn test_dot_slash_prefixed_path() -> Result<(), Box> { + assert_normalizes(|_| format!("./{NORMALIZED}")) +} + +#[test] +fn test_path_with_an_interior_parent_dir() -> Result<(), Box> { + assert_normalizes(|_| "ruby/app/models/../unowned.rb".to_string()) +} + +#[test] +fn test_absolute_path_agreeing_with_the_root() -> Result<(), Box> { + assert_normalizes(|root| { + root.canonicalize() + .expect("temp dir should canonicalize") + .join(NORMALIZED) + .to_string_lossy() + .to_string() + }) +} + +#[test] +fn test_absolute_path_when_only_the_root_is_resolved() -> Result<(), Box> { + // `cli.rs` canonicalizes `--project-root`, so on macOS the root becomes + // `/private/var/...` while a caller passing the `TMPDIR` spelling supplies `/var/...`. + // `strip_prefix` then fails, the path stays absolute, and the glob filter drops it. + // Deliberately NOT canonicalized here -- that is the case under test. + assert_normalizes(|root| root.join(NORMALIZED).to_string_lossy().to_string()) +} + +#[test] +fn test_absolute_path_when_only_the_path_is_resolved() { + // The mirror image of the test above, and it has to go through the library API: the CLI + // always canonicalizes `--project-root`, so it cannot produce an unresolved root. A + // library caller can, and does -- the `code_ownership` gem builds its own `RunConfig`. + // + // Worth its own test because resolving only the supplied path fixes the case above + // while leaving this one failing exactly as silently. + let temp_dir = fixture_with_an_unowned_file(); + // Deliberately NOT canonicalized -- that is the point. + let project_root = temp_dir.path().to_path_buf(); + + let canonical_file = project_root.join(NORMALIZED).canonicalize().expect("injected file should exist"); + + let run_config = RunConfig { + project_root: project_root.clone(), + codeowners_file_path: Some(project_root.join(".github/CODEOWNERS")), + config_path: project_root.join("config/code_ownership.yml"), + no_cache: true, + executable_name: None, + }; + + let result = runner::validate(&run_config, vec![canonical_file.to_string_lossy().to_string()]); + + assert!( + result.validation_errors.iter().any(|error| error.contains(NORMALIZED)), + "an unowned file was silently skipped: root={} file={} errors={:?}", + project_root.display(), + canonical_file.display(), + result.validation_errors, + ); +} + +#[test] +fn test_owned_file_passes_with_a_dot_slash_prefix() -> Result<(), Box> { + // A positive guard. Every assertion above is that an *unowned* file gets reported, which + // would also hold if normalization mangled the path into some other unowned path. This + // pins that a well-owned file still resolves to itself and passes. + assert_owned_file_passes("./ruby/app/models/payroll.rb") +} + +#[test] +fn test_owned_file_passes_with_an_interior_parent_dir() -> Result<(), Box> { + assert_owned_file_passes("ruby/app/payments/../models/payroll.rb") +} + +// `std::os::unix::fs::symlink` has no portable equivalent, and this crate ships only +// macOS and Linux artifacts (see .github/workflows/ci.yml), so the test is gated rather +// than made portable -- `cargo test` still compiles everywhere. +#[cfg(unix)] +#[test] +fn test_absolute_path_to_a_symlink_names_the_symlink_not_its_target() { + // Regression guard. The retry used to canonicalize the whole supplied path, which + // follows a symlinked *file*, so an absolute path naming a symlink was checked as its + // target -- a different file than the caller asked about. The retry now resolves only + // the parent and re-attaches the file name, fixing the ancestor `/var` -> + // `/private/var` discrepancy without following the leaf. + // + // Both the symlink and its target are unowned here, and the assertion is on *which path + // the report names* rather than on pass/fail. An earlier version pointed the symlink at + // an owned file and asserted failure, which was fixture-coupled and wrong: reading + // through a symlink sees the target's contents, so once ownership is resolved through + // the mappers rather than by reading CODEOWNERS back, the symlink genuinely inherits the + // target's `@team` annotation and is owned. Naming the path sidesteps that entirely. + // + // The symlink is created after `setup_fixture_repo` because that helper copies with + // `fs::copy`, which would follow it and write a regular file instead. + let temp_dir = fixture_with_an_unowned_file(); + let project_root = temp_dir.path(); + + let link = project_root.join("ruby/app/link_to_unowned.rb"); + std::os::unix::fs::symlink("unowned.rb", &link).expect("failed to create symlink"); + git_add_all_files(project_root); + + // Deliberately NOT canonicalized, so the retry fires. + let absolute = link.to_string_lossy().to_string(); + + let output = Command::cargo_bin("codeowners") + .expect("binary") + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(&absolute) + .output() + .expect("run"); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!( + stdout.contains("link_to_unowned.rb"), + "the report names the symlink's target instead of the symlink the caller asked \ + about, so the retry followed the leaf.\nstdout={stdout}" + ); +} + +#[test] +fn test_owned_file_is_not_spuriously_reported_under_star_star_globs() -> Result<(), Box> { + // The other failure mode. `invalid_project`'s `owned_globs` are `**`-leading, so a + // mishandled path is not dropped by the filter -- it survives, gets queried in the + // caller's spelling, matches no CODEOWNERS entry, and a well-owned file is reported + // unowned. Same cause, opposite symptom, which is why it needs its own fixture. + // + // Uses `generate-and-validate` because that fixture ships an empty CODEOWNERS, so the + // file has to be generated into it before the query can find it. + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/invalid_project")); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + let codeowners_path = project_root.join("tmp/CODEOWNERS"); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--codeowners-file-path") + .arg(&codeowners_path) + .arg("--no-cache") + .arg("generate-and-validate") + .arg("./ruby/app/models/payroll.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_deleted_path_is_skipped() -> Result<(), Box> { + // A changeset that deletes a file lists it, so a deleted path reaches `validate` in + // normal use. A deleted file cannot have an owner, so reporting it as unowned fails a + // commit for removing code. + // + // This is the one case where dropping a supplied path silently is right -- unlike every + // other test here, there is no file left to check. Asserted rather than assumed, + // because "silently skipped" is otherwise exactly the bug this file is about. + // + // Uses `generate-and-validate` so the regenerated CODEOWNERS no longer carries the + // deleted file's stale entry, which would otherwise mask the behavior. + 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); + + let codeowners_path = project_root.join("tmp/CODEOWNERS"); + std::fs::remove_file(project_root.join("ruby/app/models/payroll.rb"))?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--codeowners-file-path") + .arg(&codeowners_path) + .arg("--no-cache") + .arg("generate-and-validate") + .arg("ruby/app/models/payroll.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_path_outside_the_project_is_skipped_not_panicked_on() -> Result<(), Box> { + // A `..` that escapes the root cannot name a project file, so it is dropped rather than + // treated as relative. Pinned mainly so it stays a skip and not a panic -- and, since the + // filesystem retry is now gated to absolute paths, so that it stays a skip regardless of + // the process CWD. + let temp_dir = fixture_with_an_unowned_file(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(temp_dir.path()) + .arg("--no-cache") + .arg("validate") + .arg("../outside/the_project.rb") + .assert() + .success(); + + Ok(()) +} diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index 541b5bf..47f21fe 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -21,12 +21,22 @@ 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 + // 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. run_codeowners( - "valid_project", + "invalid_project", &["validate", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + predicate::str::contains("ruby/app/unowned.rb"), )?; Ok(()) @@ -34,12 +44,14 @@ 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`. run_codeowners( - "valid_project", + "invalid_project", &["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")), + predicate::str::contains("ruby/app/unowned.rb"), )?; Ok(()) @@ -79,7 +91,9 @@ fn test_generate_and_validate_with_owned_files() -> Result<(), Box> { #[test] fn test_generate_and_validate_with_unowned_file() -> Result<(), Box> { - let fixture_root = std::path::Path::new("tests/fixtures/valid_project"); + // `invalid_project` for the same reason as `test_validate_with_unowned_file`: it holds + // a file that genuinely has no owner. + let fixture_root = std::path::Path::new("tests/fixtures/invalid_project"); let temp_dir = setup_fixture_repo(fixture_root); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -96,8 +110,7 @@ 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("Unowned")); + .stdout(predicate::str::contains("ruby/app/unowned.rb")); Ok(()) }