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

[perryqh]

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: — five 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. Four of the five tests now pass and are un-ignored.
3. fix: — review follow-ups: deduplicate a path supplied twice (it was reported twice), correct the complexity claim below, make the unused FileGenerator structurally impossible in the scoped path rather than merely unused, give the scoped span a distinct name, and rewrite the --help text for files.

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.

To be precise about the cost, since an earlier revision of this description overstated it: this scopes the per-file work, not all of it. Building the owner matchers stays O(repo) — TeamFileMapper::owner_matchers enumerates every annotated file. So the scoped path costs a fixed O(repo) term plus O(supplied paths × matchers), against O(repo × matchers) for the whole-project run. The saving is in the variable term, which dominates on a large repo with a small changeset, but it is not free and it has not been measured — gv with no paths still isn't a perf-harness case.

Verified scoping is real, not accidental whole-repo validation — gv ruby/app/services/multi_owned.rb on the invalid_project fixture reports only that file's dual ownership, and stays silent about calculator.rb, blockchain.rb, and unowned.rb. A cleanly-owned file still exits 0.

One behavior change

Unowned files supplied by path now 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. Three test assertions were updated. The wrapping code_ownership gem already parses this format, since validate with no paths has always produced it.

Absolute paths now render project-relative rather than as the caller wrote them.

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 an escalation path (run the full check when team files are in the changeset), which this PR does not add.

The fifth test stays #[ignore]d: non-canonical absolute paths are dropped by the owned_globs filter before any check runs. Separate pre-existing bug, detailed below. Run it with:

cargo test --test validate_files_parity_test -- --ignored

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 runs none of them. It only asks 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 are 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 says 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.

Separately: absolute paths are silently skipped

validate <non-canonical absolute path> exits 0 having never checked the file:

codeowners --project-root /var/folders/.../tmp --no-cache validate \
  /var/folders/.../tmp/ruby/app/unowned.rb
code=0
stdout=""

cli.rs canonicalizes --project-root, so a caller-supplied /var/... path fails strip_prefix against the canonical /private/var/... root, stays absolute, and is then rejected by the owned_globs filter — dropped before any ownership query runs. Easy to hit on macOS (TMPDIR) and with symlinked checkouts.

Unrelated to the parity gap and older: it dates to the owned_globs filter added by #89 for #88. The symptom is config-dependent — with a **-leading owned_globs the same path survives the filter and is reported spuriously unowned instead. Both wrong, same cause.

What this deliberately does not decide

This takes the option that needed no product decision: make the scoped check correct. It is a strict improvement under any subsequent choice about whether the fast path should exist at all. Still open, and better argued with numbers than in the abstract:

• Drop the files param. gv <paths> already pays for the full project build, so running the real validate_all() costs only the validator passes — not a second build. If that is within noise, the param buys nothing.
• Add escalation (--fail-fast-and-retry): run the full check when team files, .codeowner, or CODEOWNERS are in the changeset; otherwise the scoped check. Closes the staleness blind spot above.

Unmeasured, because gv with no paths is not currently a case in the perf harness — worth adding before deciding.

The --help text for files is updated here: it used to promise "fast mode for git hooks" with no hint it skips the staleness check, and now says so outright and points at generate-and-validate. That is deliberately the only place the caveat surfaces — a note on stdout was rejected because successful runs are silent by contract and pre-commit hooks depend on that.

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.

Two things reviewed and knowingly not addressed:

• invalid_package_ownership runs unscoped. Validating one file can therefore fail over a package that file has nothing to do with. Documented in the doc comment; defensible, since packages are orders of magnitude fewer than files and skipping them would be a second blind spot — but it is a behavior change and it has no test.
• No unit test on Validator::validate_files. Coverage is integration-only through the binary, so the scoping predicate is not exercised in isolation.

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