[perryqh]

Updated to address the review. This branch was stacked on #126, which was closed as superseded by #148, so it now merges main and targets it. Perry's original description is kept at the end. Its notes on path normalization and skipping deleted files describe #126 and no longer apply.

Summary

validate <files> and gv <files> now resolve ownership through the mappers, as validate and gv with no files do, rather than by reading CODEOWNERS back. Reading it back hid two defects:

• gv <a file owned two ways> exited 0, because generating CODEOWNERS gives the file to one of its owners. It's still the case on main: codeowners gv ruby/app/services/multi_owned.rb in invalid_project exits 0 with empty output.
• A file annotated with a team that doesn't exist was reported as unowned, rather than as naming an invalid team.

What a list of files now checks

• Each listed file has exactly one owner, and its annotation names a team that exists. An optionally owned file with no owner is excused, as in a full validate.
• Each package and .codeowner whose directory contains a listed path names a team that exists. Scoping them means one bad one elsewhere doesn't fail every scoped run. A listed package.yml or .codeowner selects itself.
• Under validate, but not gv, which has just written it, CODEOWNERS gives each listed file its owner. The whole-file staleness check can't be scoped, and without this a file annotated and staged without regenerating CODEOWNERS passed. That's the gem's bin/codeownership validate --diff --skip-autocorrect. Such files are reported under the same "CODEOWNERS out of date" heading as a full validate.

Behavior changes

• Unowned files are reported under "Some files are missing ownership", the format a full validate uses, instead of "Unowned files detected:". Paths are still shown as given, per Normalize the paths validate, gv and for-file are given #148.
• Add optionally_owned_globs to permit files with no owner #135's "Files matching optionally_owned_globs name an owner that CODEOWNERS doesn't list" heading is gone. An annotation, package or .codeowner naming a team that doesn't exist is reported as an invalid team reference, and an owner missing from CODEOWNERS as out of date.
• An untracked file in a directory with a .codeowner is owned, so validate <file> agrees with for-file <file>. An untracked file's own annotation still doesn't count, as in validate.
• A path that doesn't exist is still checked, per Normalize the paths validate, gv and for-file are given #148. A deleted file that only its annotation owned is now reported as missing ownership under validate too, as it already was under gv on main. The gem drops files that don't exist before calling in, so its users aren't affected.

Commits since the review

• 08fd38a merges main. Normalize the paths validate, gv and for-file are given #148's path handling replaces fix: normalize the paths callers supply to validate #126's, fix: write CODEOWNERS annotations after glob-based sections #130's generator mappers are kept, fix: report .codeowner files that reference an unregistered team #116's .codeowner check is scoped like packages, and Add optionally_owned_globs to permit files with no owner #135's handling is reconciled as above.
• 80e255c adds the per-path CODEOWNERS check.
• aa30f3d replaces the differential test's two hardcoded strings with a byte-for-byte comparison of gv with every tracked file and gv with none, across every fixture.
• 2e944f3 updates the README.
• 596c038 and bbb498a fix comments and the perf README's span names, time the CODEOWNERS lookup inside per_file_query, and reword the message when CODEOWNERS gives a file no known team.
• cb1ae1d and 4329638 tidy the tests.

Test plan

• cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings and cargo test pass (328 passed, the 3 existing ignores).
• tests/validate_files_parity_test.rs:
  • On every fixture whose project builds, gv with every tracked file, leaving out what .ignore hides, prints the same output and exits the same way as gv with no files. Dropping the scoped .codeowner check fails it.
  • On every such fixture, once CODEOWNERS is generated, validate with every tracked file reports nothing stale.
• tests/validate_files_test.rs covers a file missing from CODEOWNERS, one CODEOWNERS gives another team, and a team with do_not_add_to_codeowners_file.
• Validator unit tests cover the scoping of packages and .codeowner files, including whole-component prefixes and the project root.
• Mutation-checked: turning off the CODEOWNERS check fails 4 tests, and comparing teams that don't exist fails 8.

Not in this PR

• A list of files isn't checked against .ignore, which leaves files out of the project walk, so a listed file it hides is reported as unowned. main does the same.
• A change to a team file or .codeowner can change the owner of files outside the list, which a scoped run doesn't see. gv regenerates CODEOWNERS, so it writes the change but only checks the listed files.
• The perf numbers below predate the merge and weren't re-measured. The CODEOWNERS check adds one parse of CODEOWNERS per run, which main's validate_files already does.

Original description

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

[dduugg]

Thanks for this, and for how much care went into it. The write-up made the false pass easy to understand and reproduce, and the differential test, the measured perf numbers, and the unit tests pinning component-wise starts_with and the root-level package are the kind of detail that makes a change like this reviewable.

The core change is right, and the bug is still live on main: codeowners gv ruby/app/services/multi_owned.rb in invalid_project exits 0 with empty output there, and this branch reports the dual ownership. Resolving supplied paths through the mappers is the correct fix, and scoping the package check to packages that contain a supplied path is a good call.

Two things need to happen before it can land. Both are detailed inline.

1. A regression in validate <paths>. Removing the CODEOWNERS read-back also removes the only per-path staleness signal, so a file added without regenerating now passes. There's a repro and a suggested per-path check on runner.rs.
2. A rebase onto main. #126 is now closed as superseded by #148, so this needs retargeting. Main has changed most of what this touches since 99d25cd, and some of it went the other way from what this branch inherited from #126. The inline notes cover #148's path handling, #116's directory check, and #130's generator mappers. Three more have no line to attach to:
   • optionally_owned_globs (#135). Main's validate_file_ownership already excuses an optionally owned file with no owner, so reusing it here gets that for free. Main's validate_files also has the "names an owner CODEOWNERS doesn't list" category and the codeowners_generated flag that drops the "run X to update" hint under gv. Those need a decision rather than a conflict resolution.
   • README. The validate [files...] line and the optionally_owned_globs paragraph say that given files, validate and gv "look each one up in CODEOWNERS and don't check for more than one owner", which is the behavior this changes.
   • Wording. The move from "Unowned files detected:" to "Some files are missing ownership" is reasonable, but main now has many more exact-output assertions on the old wording (path_normalization_test.rs, optionally_owned_globs_test.rs, path_globs_test.rs).

A few points in the description have gone stale: #116 has landed, #124 is closed, and the gem now pins v0.5.0 rather than v0.3.3. Running the gem's suite against this before bumping the pin is still a good idea.