fix: run the real ownership checks when validate/gv are given paths

[perryqh]

Stacked on #126. That PR fixes path normalization — a set of bugs that exist on main independently of this one — and was split out so each can be reviewed on its own. Review #126 first; this PR's diff is against its branch. main is the eventual merge target.

Summary

validate <paths> and gv <paths> were not equivalent to the same commands with no paths, and one of the gaps was a silent false pass: gv <dual-owned file> exited 0 with empty output.

Three commits:

1. test: — failing tests documenting the gap.
2. fix: — ownership for supplied paths is now resolved through the mappers, the same way the whole-project run does.
3. fix: — review, described under Review below. Three passes squashed into one: each found a silent false pass in the one before it.

The fix is cheap because two of the three checks were already per-file: validate_file_ownership iterates file_to_owners(), a par_iter over project.files, and validate_invalid_team's file half does the same. Scoping them to the supplied paths is a filter, not a rewrite. The old code was doing more I/O (re-reading and re-parsing CODEOWNERS per path) to learn less.

Why

Passing file paths swaps validate_all() for validate_files() (runner.rs:124):

pub fn validate(&self, file_paths: Vec<String>) -> RunResult {
    if file_paths.is_empty() { self.validate_all() } else { self.validate_files(file_paths) }
}

validate_all → Validator::validate runs three checks (validator.rs:40-57):

1. validate_invalid_team — annotation or package naming a nonexistent team
2. validate_file_ownership — file owned two ways
3. validate_codeowners_file — CODEOWNERS is stale

validate_files ran none of them. It only asked whether each path resolves to a team when reading the CODEOWNERS file.

gv <paths> regenerates before validating, so check 3 is moot by construction. Checks 1 and 2 were still skipped — generate() only calls generate_file() and writes the string (ownership.rs:167); it validates nothing.

The false pass

gv ruby/app/services/multi_owned.rb, a file owned by both a @team Payments annotation and ruby/app/services/.codeowner naming Payroll:

code=0
stdout=""

gv with no paths reports "Code ownership should only be defined for each file in one way."

The mechanism is the concerning part: regenerating writes the file into CODEOWNERS under one of its two owners, so the per-path check finds an owner and passes. Regenerating conceals the defect rather than exposing it. Reproduces independently through owned_gems on gems/payroll_calculator/calculator.rb, so it is not one mapper misbehaving.

The differential

gv with every owned path vs. gv with none, same repo:

	no paths	every path
dual ownership (2 files)	reported	missed
invalid team Web3	reported	missed
unowned files	reported	reported

This is the general form of the targeted tests — it needs no knowledge of which defects the fixture holds, so it keeps working as fixtures change. It is also the check requested back in December when #89 landed, which was never added.

Wrong diagnosis, not just a miss

gv ruby/app/models/blockchain.rb (annotated @team Web3, not a team) exits 1 — but said Unowned files detected: ruby/app/models/blockchain.rb. An invalid team yields no owner, so the file is simply absent from the generated CODEOWNERS and reads as unowned. The developer goes looking for missing ownership instead of fixing a typo.

Review

Three passes' worth, grouped by theme rather than by which pass found what. Everything here came from reading the code rather than from a test failing. (The normalization findings from those passes live in #126.)

The package check was unscoped, and inconsistently so

validate_invalid_team ran invalid_package_ownership over every package regardless of the path list. An earlier pass called this "defensible"; it is not:

validate <clean file unrelated to the bad package>  →  exit 1
  "ruby/packages/payroll_flow/package.yml is referencing an invalid team - 'NoSuchTeam'"

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. It was also inconsistent: the empty-path early return keyed off the glob-filtered list, so whether the unrelated package error surfaced depended on whether some supplied path happened to match owned_globs — validate foo.rb failed while validate README.md passed, same repo, same bad package.

Packages are now selected by containing at least one supplied path. That needs two path lists, because a manifest must be able to select its own package while not being eligible to be reported unowned: owned_paths for the per-file checks, supplied_paths for package selection. Selecting on the wider list means editing a package.yml to name a nonexistent team is caught by the commit that does it, not only by some later commit that happens to touch a file inside that package. The early return is now a pure optimization.

An unwalked path was assumed unowned rather than asked about

The walk only records git-tracked files, so a brand-new unstaged file was reported "Some files are missing ownership" even when a .codeowner in its directory owned it. for-file on the same path, resolving through the mappers, correctly named the team — two commands in one binary disagreeing about one path, failing in the direction that rejects a commit for lacking an owner it has.

Every requested path now goes to the matchers. This simplified rather than complicated: ownership resolution never needed ProjectFile, only the relative path, which is what the matchers key on. file_to_owners becomes path_to_owners and the separate "unwalked, therefore unowned" branch disappears instead of growing a special case. A genuinely unowned untracked file is still reported, which a second test pins — resolving unwalked paths must not degrade into passing them.

The annotation check still runs only over walked files, since an untracked file's annotation has not been parsed. Narrower gap, left alone deliberately.

Smaller things

Deduplicate a path supplied twice, which was reported twice. Make the unused FileGenerator structurally impossible in the scoped path by passing it to validate() rather than holding it as a field. Name the scoped span validator_validate_scoped to parallel validator_validate.

Adds unit coverage for Validator::validate_files, which was reachable only end-to-end through the binary, and asserts two things the scoping predicate silently depends on but which were previously only claimed in a doc comment: starts_with is component-wise, so ruby/packages/foo must not select ruby/packages/foobar; and a root-level manifest has an empty relative root that prefixes every path.

Measured

An earlier pass corrected the complexity claim but left it unverified, and asserted gv with no paths was not a perf-harness case — it is (validate_all, gv). Numbers from codeowners-perf, best of 3 warm runs, against a large monorepo (~130k tracked files, ~18k-line CODEOWNERS):

case	wall	project_build	validation
validate_all	3018 ms	1968 ms	933 ms
validate_files_1	2034 ms	1895 ms	28 ms
validate_files_100	2028 ms	1885 ms	32 ms
validate_files_1000	2044 ms	1889 ms	42 ms
validate_files_2000	2068 ms	1898 ms	56 ms

Re-measured after the path_to_owners refactor — 933 to 945 ms whole-project, 28 to 32 ms at one path, 56 to 54 ms at two thousand — so it is perf-neutral and these numbers stand.

The variable term collapses exactly as predicted — validation goes from 933 ms to 28 ms, and is near-flat in path count (28 ms at one path, 56 ms at two thousand). But wall clock only improves 3.0 s → 2.0 s, because the ~1.9 s project build is the fixed O(repo) term and is paid either way.

So the files param is worth about a second on a repo that size, not an order of magnitude. That is the number the "should the fast path exist at all" question needed:

• Drop the files param. Costs ~1 s per invocation at that scale — one third of wall clock, not a second project build. Defensible either way now, but it is a real second in a pre-commit hook.
• Add escalation (run the full check when team files, .codeowner, or CODEOWNERS are in the changeset). Closes the staleness blind spot below, and now has a price attached: ~1 s when it fires.

Behavior changes

Unowned files supplied by path report as "Some files are missing ownership" — the wording the whole-project run uses — rather than "Unowned files detected:". Same defect, same words, whether or not paths are passed, which is the point.

This is safe, though not for the reason an earlier revision of this description gave. The gem does not parse CLI stdout at all: it links codeowners-rs as a magnus native extension and consumes RunResult.validation_errors directly, joining them into a RuntimeError. Its own spec already asserts /Some files are missing ownership/ for the no-paths case, so this aligns the <paths> branch with what the gem already expects. No public consumer greps "Unowned files detected". Note the gem pins tag = "v0.3.3", so nothing changes for its callers until that is bumped.

Also: a bad package owner elsewhere in the repo no longer fails a scoped run, and an untracked file is attributed through the mappers rather than assumed unowned, so validate <path> now agrees with for-file <path>.

Taken together with #126, this PR has changed validate <paths> behavior in several ways — deleted paths, untracked paths, package scope, output wording, path forms. Each is individually tested, but the aggregate is a meaningfully different command than the gem was pinned against at v0.3.3. Worth running the gem's own suite against this stack before bumping that tag, not after.

Still open

Staleness is not checked for a supplied path list and cannot be — it compares the whole generated file against the whole on-disk one. gv <paths> makes it moot by regenerating first, but a team file or .codeowner change can still alter ownership of files outside the changeset without being caught. That gap wants the escalation path priced above, which this PR does not add.

One follow-up left alone: #116 overlaps the invalid-team reporting this now runs on supplied paths. If #116 lands first there may be a conflict in validate_invalid_team; if this lands first, #116's fix applies to the scoped path for free.

Consequence for #124

#124 optimized reading the CODEOWNERS file back per path. This deletes that code path, so #124 is obsolete rather than parked — it can be closed.

🤖 Generated with Claude Code