fix: normalize the paths callers supply to validate

src/cli.rs

@@ -39,15 +39,19 @@ 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<String>,
     },
 
     #[clap(about = "Chains both `generate` and `validate` commands.", visible_alias = "gv")]
     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<String>,
     },
 

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

@@ -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<PathBuf> {
+    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<PathBuf> {
+    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);
+    }
 }

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<String> = 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<PathBuf> = 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),