validate <files> and for-file don't normalize the paths they're given #145

validate <files>, gv <files> and for-file match each path as it was typed. The only cleanup is stripping the project root from an absolute path, and the CLI canonicalizes the root first. So a path that names the right file in another form (a leading ./, a repeated /, a . or .. segment, or an absolute path through a symlink) is compared against owned_globs, unowned_globs and CODEOWNERS as a different string. Depending on the config, a file that CODEOWNERS assigns is reported as unowned, or a file with no owner is skipped and passes.

Found while working on #135, which works around it only for files matching its new optionally_owned_globs (below). Fixing it changes how every listed file is looked up, so it's left out of that PR.

Reproduction

On main (e64429f), on macOS, where /tmp is a symlink to /private/tmp:

mkdir /tmp/proj && cd /tmp/proj && git init -q
mkdir -p app config/teams
printf 'owned_globs:\n  - "**/*.rb"\n' > config/code_ownership.yml
printf 'name: Valid\ngithub:\n  team: "@org/valid"\n' > config/teams/valid.yml
printf '# @team Valid\n' > app/valid.rb
printf 'puts 1\n' > app/unowned.rb
git add -A && codeowners generate

With owned_globs scoped to directories, as in the README's example, the opposite happens:

printf 'owned_globs:\n  - "{app,config}/**/*.rb"\n' > config/code_ownership.yml
codeowners generate

./app/unowned.rb and the absolute paths don't match {app,config}/**/*.rb, so validate drops them before checking, and for-file ignores the annotation. app//valid.rb and app/./valid.rb do match it, so they're still reported as unowned.

With optionally_owned_globs (#135)

#135 adds optionally_owned_globs. Given an allowed file that CODEOWNERS has no line for, validate <files> puts the path in plain form, without ./, repeated / or . segments, and then checks whether anything names an owner for it. If something does and the form changed, it looks the plain form up in CODEOWNERS again. A path with a .., or an absolute path it can't make project-relative, is reported as unowned, as without the key. On its branch (8f76d20):

printf 'owned_globs:\n  - "**/*.rb"\noptionally_owned_globs:\n  - "**/*"\n' > config/code_ownership.yml
printf '# @team Typo\n' > app/typo.rb   # Typo isn't a team
git add app/typo.rb && codeowners generate

So for files matching optionally_owned_globs, ./, repeated / and . segments now give the same answer as the plain path, and the other forms the same answer as on main. (At f15fff0, which this section first described, the ./, .. and absolute typo.rb paths passed.) Files outside optionally_owned_globs are looked up as on main.

Cause

• Runner::validate_files strips the project root only from an absolute path before matching owned_globs and unowned_globs, and passes the path as typed to the CODEOWNERS lookup.
• teams_for_files_from_codeowners does the same, and Parser::teams_from_files_paths prefixes /, so /./app/valid.rb is matched against CODEOWNERS.
• Args::absolute_project_root canonicalizes the project root, so an absolute path through a symlink never starts with it.

for-file without --from-codeowners copes with ./ because find_file_owners joins the path onto the root first, and strip_prefix compares components, which drops .. It doesn't handle the symlinked path.

Effect on code_ownership

The gem passes paths through as given. bin/codeownership validate <files> hands its arguments to validate. validate --diff takes its paths from git diff --staged --name-only, which prints them without ./ or .., so it isn't affected. CodeOwnership.for_file and teams_for_files_from_codeowners pass theirs to RustCodeOwners.teams_for_files unchanged.

Possible fix

Normalize each path once, before any matching, and use the result for the glob filters and the CODEOWNERS lookup: join a relative path onto the project root, clean up . and .. (cli.rs already uses path_clean's clean()), and if the result doesn't start with the canonical root, canonicalize its parent directory (when it exists) and strip the root from that. Canonicalizing the file itself would follow a symlinked file to its target, which isn't the path CODEOWNERS matches. Results and error messages should keep the path as given. #135's optionally_owned_globs check could then use that result instead of its own plain form and second CODEOWNERS lookup.

Doing this in teams_for_files_from_codeowners would also fix for-file --from-codeowners and the gem's lookups. The gem's TeamFinder.for_file, used with from_codeowners: false, returns nil for any path starting with ./, so it may be worth deciding whether those lookups should accept these forms at all.