fix: resolve only the parent when retrying an absolute path

Two findings from reviewing the previous commit, one of them a false pass it
introduced.

The retry canonicalized the whole supplied path, which follows a symlinked
*file*. The project walk records the symlink path rather than its target -- the
reason lexically_normalize is lexical in the first place, stated in its own doc
comment two lines above the code that violated it. So an absolute path naming an
unowned symlink was silently checked as its owned target and exited 0:

    validate ruby/app/models/link_unowned.rb              -> exit 1  (correct)
    validate /tmp/proj/ruby/app/models/link_unowned.rb    -> exit 0  (false pass)
    realpath(link_unowned.rb) = /private/tmp/proj/ruby/app/models/payroll.rb

A false pass on a different file than the caller named, which is the exact
failure class this branch exists to rule out. The retry now resolves the parent
and re-attaches the file name, so the ancestor /var -> /private/var discrepancy
is still fixed without following the leaf. A symlinked *ancestor* is still
resolved, unavoidably -- that is the 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.

Writing the invariant down was not enough to enforce it. There was no symlink
test, so nothing caught the contradiction; there is one now, and it fails if the
whole-path canonicalize is reintroduced.

The same defect survived in codeowners_query::teams_for_files_from_codeowners,
reached from public API as runner::teams_for_files_from_codeowners. It
relativized with relative_to_buf, which passes an unstrippable path through
unchanged, so 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. Fixing validate while leaving the bulk-lookup entry point beside it
would have made the branch's claim narrower than it reads.

That one 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. Note the keys were already the
relativized form, not the caller's spelling, so they were inconsistent depending
on whether strip_prefix happened to succeed; they are now consistently relative.

The retry logic moves to path_utils::resolve_project_relative so both callers
share it rather than growing a second copy.

Adds two positive guards. Every other assertion in the file is that an unowned
file gets reported, which would also hold if normalization mangled a path into
some other unowned path; these pin that a well-owned file still resolves to
itself and passes under `./` and interior `..` spellings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: perryqh

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<HashMap<String, Option<Team>>, 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<PathBuf> = 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();
 

src/path_utils.rs

@@ -38,6 +38,35 @@ pub fn project_relative(root: &Path, path: &Path) -> Option<PathBuf> {
     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<PathBuf> {
+    if let Some(relative) = project_relative(root, path) {
+        return Some(relative);
+    }
+
+    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

src/runner.rs

@@ -153,14 +153,14 @@ impl Runner {
         // 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 `project_relative_path` needs it and that retry can fire for every path
+        // 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<PathBuf> = file_paths
             .iter()
             .filter_map(|file_path| {
-                Self::project_relative_path(&self.run_config.project_root, canonical_root.as_deref(), Path::new(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
@@ -215,28 +215,6 @@ impl Runner {
         RunResult::default()
     }
 
-    /// Reduce a caller-supplied path to project-relative form.
-    ///
-    /// An absolute path only strips if it and the 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.
-    ///
-    /// Hence the retry resolves both sides rather than just the path: fixing only the path
-    /// leaves the mirror-image case — a canonical path against an unresolved root — failing
-    /// exactly as silently. The first attempt uses the root as given, so the common case of
-    /// relative paths costs no syscalls at all.
-    fn project_relative_path(root: &Path, canonical_root: Option<&Path>, path: &Path) -> Option<PathBuf> {
-        if let Some(relative) = crate::path_utils::project_relative(root, path) {
-            return Some(relative);
-        }
-
-        let canonical_path = path.canonicalize().ok()?;
-        crate::path_utils::project_relative(canonical_root.unwrap_or(root), &canonical_path)
-    }
-
     pub fn generate(&self, git_stage: bool) -> RunResult {
         let content = self.ownership.generate_file();
         if let Some(parent) = &self.codeowners_file_path.parent() {

tests/supplied_path_normalization_test.rs

@@ -18,7 +18,10 @@
 //! 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.
+//! 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):
@@ -27,6 +30,7 @@
 //! - 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::*;
@@ -55,6 +59,27 @@ fn fixture_with_an_unowned_file() -> TempDir {
     temp_dir
 }
 
+/// Assert `validate <spelling>` 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<dyn Error>> {
+    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 <spelling>` 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<dyn Error>> {
@@ -159,6 +184,64 @@ fn test_absolute_path_when_only_the_path_is_resolved() {
     );
 }
 
+#[test]
+fn test_owned_file_passes_with_a_dot_slash_prefix() -> Result<(), Box<dyn Error>> {
+    // 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<dyn Error>> {
+    assert_owned_file_passes("ruby/app/payments/../models/payroll.rb")
+}
+
+#[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<dyn Error>> {
     // The other failure mode. `invalid_project`'s `owned_globs` are `**`-leading, so a