fix: report .codeowner files that reference an unregistered team

[sarastrasner]

Problem

README:237-244 documents codeowners validate as ensuring "All referenced teams are valid." .codeowner files were the one ownership surface where the code didn't keep that promise.

DirectoryMapper::entries looks each directory owner up in the team registry and skips the entry when the name doesn't resolve:

let team = team_by_name.get(&directory_codeowner_file.owner);ifletSome(team) = team {
entries.push(Entry{ ...});}

Nothing else reports the unresolved name, so a .codeowner naming a team that was renamed, deleted, or simply typo'd is completely silent:

• generate emits no line for that directory
• the directory instead inherits the nearest ancestor owner
• validate exits 0

The file still looks authoritative, but it's inert — and the ownership it was written to express quietly belongs to whichever team owns the parent directory. Because validate stays green, these accumulate rather than getting caught on the PR that introduced them.

There's also a failure mode where the advice is actively counterproductive: an unresolvable owner drops that directory's line from the generated file, so validate reports CODEOWNERS out of date and points you at codeowners generate — which "fixes" it by erasing the line, making the real problem disappear.

Annotations (invalid_team_annotation) and package ownership (invalid_package_ownership) are already validated against the registry. Directory ownership is the one surface that isn't.

Fix

Adds invalid_directory_ownership as a third check in validate_invalid_team.

The predicate resolves through project.teams_by_name, notproject.teams[].name. That matters: project_builder.rs:306-309 deliberately keys that map by both team.name and team.github_team, so a .codeowner holding the GitHub handle generates a correct CODEOWNERS line. Validating against teams[].name instead would reject a project whose CODEOWNERS is already right — reproducing this PR's own bug in a new form, with no command that fixes it. Using teams_by_name makes the check definitionally identical to the mapper's lookup.

Directory violations get their own InvalidDirectoryTeam variant rather than reusing InvalidTeam, so the message can name the ancestor the directory is now silently inheriting from — the detail a reader needs in order to understand what happened:

- app/services/nested/.codeowner is referencing an invalid team - 'Web3'; this directory is currently inheriting its owner from app/services

The ancestor walk only considers roots whose own owner resolves, so a chain of two bad .codeowner files names the nearest ancestor that actually owns something rather than the nearest one that merely has a file.

category() still returns the same string as InvalidTeam, so one typo'd team name spread across an annotation, a package.yml, and a .codeowner groups under a single headline rather than three. That shared headline is renamed to Found invalid team references, which is accurate for all three surfaces — Found invalid team annotations already spanned package.yml.

An empty or whitespace-only .codeowner is reported too. owner is content.trim() pushed unconditionally with no empty-guard (project_builder.rs:278), so it yields "" and reaches the same silent-inheritance path. Failing is right, but an invalid team - '' gives no hint the file is empty, so it reads is empty and names no team instead.

Scope, deliberately

• generate still doesn't route through the validator, so someone who only runs generate gets a clean-looking file with the bad .codeowner undetected. Left as-is: generate is a writer, and making it validate is a scope change with its own upgrade story.
• The teams[].name vs teams_by_name divergence is pre-existing and remains in the two sibling checks. Bringing them onto teams_by_name would loosen them — # @team @footeam and a package.yml owned by a GitHub handle would start passing where they fail today. That's a behavior change on two surfaces this PR isn't about, on an input form the README doesn't document.

Upgrade impact

Projects whose .codeowner files name a registered team by either its name or its GitHub handle are unaffected. Only genuinely unresolvable names — and empty files — now fail where they previously exited 0.

Diff noise

The Error enum appears heavily rewritten, but every variant except InvalidDirectoryTeam is untouched semantically. With max_width=140, struct_variant_width derives to ~49 chars; the new variant's body is ~85, and rustfmt formats enum variants all-or-nothing, so adding it forces every variant onto multiple lines. I confirmed this is width-driven rather than comment-driven by re-testing with a one-line doc comment and getting the identical result. Roughly 15 of the added lines are that reformat.

Tests

Three integration tests, since the ownership! unit-test macro can't reach validate() (see below).

• invalid-directory-codeowner — a nested .codeowner naming an unregistered team, underneath an ancestor .codeowner naming a real one. That shape matters: ownership resolves cleanly to the ancestor, so no other check fires and the generated CODEOWNERS looks entirely reasonable. Exits 0 with empty output before this change.
• directory-codeowner-github-team — Foo in one .codeowner and @footeam in another, against a single config/teams/foo.yml. Pins the accepted side, so the tolerant behavior can't regress silently. Every pre-existing .codeowner fixture in the repo holds a bare team name, which is exactly why the suite would stay green through an over-strict predicate.
• empty-directory-codeowner — whitespace-only .codeowner, mirroring the invalid-team fixture so exactly one error fires. The generated CODEOWNERS is identical either way, since an unresolvable owner and an absent one both drop the directory's line.

Still untested, agreed as non-blocking: multiple invalid directory owners in one tree, a root-level .codeowner, and the generate interaction.

Verification

• cargo test — 26 test binaries, 0 failed (added 3)
• cargo clippy --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean

On Rust 1.97.1, after rebasing onto 99d25cd.

Downstream impact

Against the note that validate_files consumers are insulated — Gusto/web is on the other path. Its CI runs bin/codeownership validate with no file arguments, which runner.rs:119-125 routes to validate_all, so it takes the full repo-wide behavior.

I ran this branch against that monorepo: across 1,772 .codeowner files there is exactly one violation, and it's a true positive of precisely the kind this PR describes — a renamed team leaving an inert .codeowner, above an app with zero CODEOWNERS lines, with validate exiting 0. That's now fixed and merged upstream (Gusto/web#33177), so whenever a release is cut, web takes it green.

Unrelated wrinkle, noted in passing

I first tried this as a unit test and hit something you may want to know about: the ownership! macro in src/common_test.rs binds tempdir() to a local that drops at the end of the macro block, so the temp project directory is deleted before the caller ever uses the returned Ownership. Any test that touches the filesystem afterwards — get_codeowners_file(), for instance — silently sees an empty repo and reports a spurious stale-CODEOWNERS diff.

Nothing currently depends on it, because every existing caller only reads the already-built in-memory Project. It does mean Ownership::validate() can't be unit-tested through that helper today, which is why this PR uses integration tests. Out of scope here.

[dduugg]

Thanks for this, and especially for the writeup. The description does the hard part of the review for the reader: it explains not just what's broken but why it stays invisible, and the note about generate "fixing" the problem by erasing the line is the detail that makes the bug worth prioritizing. The fixture shape you chose (invalid nested under a valid ancestor) is the right one, since it's the only shape where nothing else fires.

The premise holds up under checking. On main, the new fixture exits 0 with empty output, and for-file app/services/nested/nested_file.rb reports Team: Foo sourced from app/services/.codeowner, so the nested file really is inert while looking authoritative. DirectoryMapper::entries drops the entry at directory_mapper.rs:29 while owner_matchers keeps the unresolvable name, which is exactly why no existing check catches it. Full suite, clippy with -D warnings, and fmt --check are all clean on the branch, and the new test does fail without the validator change (code=0, empty stdout).

One thing worth fixing before merge

The new check is stricter than the resolver it mirrors, so it fails projects whose CODEOWNERS is already correct.

validate_invalid_team builds team_names from project.teams[].name (validator.rs:64), but the resolution path goes through project.teams_by_name (directory_mapper.rs:28), and project_builder.rs:306-309 deliberately populates that map with two keys per team, team.nameandteam.github_team. So a .codeowner holding the GitHub handle resolves fine for generation but is now rejected by validation.

With config/teams/foo.yml (name: Foo, github.team: "@footeam") and a .codeowner containing @footeam:

generate -> /app/services/nested/**/** @footeam # correct, CODEOWNERS not stale
validate -> app/services/nested/.codeowner is referencing an invalid team - '@footeam'
EXIT=1 # on main: EXIT=0

This is the same trap the description sets out to remove, in a new form: validate fails, generate produces a correct file, so there's no command that fixes it.

Two qualifiers on severity, in fairness. The handle form is undocumented (the README documents .codeowner content as TeamName), so this is undocumented-but-functional rather than a documented contract. And the check only fires on the full-repo path, since Ownership::validate() is reached from validate_all at runner.rs:128 but not from validate_files, so consumers validating scoped to changed files are unaffected. Still, it's a config that works on main and starts failing CI on upgrade, and the fix is one line:

.filter(|f| !self.project.teams_by_name.contains_key(&f.owner))

That makes the predicate definitionally identical to the mapper's lookup and lets team_names drop out of the signature. file_owner_resolver.rs:151 uses the tolerant map too, so the validator is the sole outlier here.

Worth noting this divergence is pre-existing rather than something you introduced: all three mappers resolve via teams_by_name while all three validator checks use teams[].name, and on main an annotation # @team @footeam likewise generates a valid line and simultaneously reports an invalid team. You mirrored the established pattern faithfully. So either fix is reasonable: tighten it here, or leave it consistent with the siblings and fix all three together. I'd lean toward fixing it here, since .codeowner is the surface where a green generate most strongly implies "this is fine."

A companion positive test would catch this, and there's precedent for asking: 6d44c0f added both a reproduction and a test pinning the accepted side, "so the scoped nature of this fix doesn't regress by accident later." Every .codeowner fixture in the repo currently holds a bare team name, which is why the suite stays green through this. Asserting that both a valid team name and a valid @github_team validate clean would close it.

Smaller notes

• Empty or whitespace-only .codeowner now reports is referencing an invalid team - ''.owner is content.trim() (project_builder.rs:278), so it yields "". Newly failing, and reasonable to flag since a placeholder or stray file is a real case, but the message gives no hint the file is empty.
• The message omits the surprising part, the silent inheritance.validator.rs:230 is shared by all three sources, so it can't mention inheritance as written. If you want message precision and headline stability both, a separate variant whose messages() arm reads something like app/services/nested/.codeowner names an unknown team 'Web3'; this directory is currently inheriting its owner from app/services, with category() returning the same string as InvalidTeam, gets there for the cost of one variant. Optional, but the inherited-owner detail is the thing a user needs in order to understand what happened.
• generate still ignores this error class entirely, since it doesn't route through the validator. Someone who only runs generate gets a clean-looking file with the bad .codeowner undetected. Probably correct as-is, but it's untested and unremarked; worth a line if intentional.
• Style, take it or leave it: the two siblings use a single .flat_map(|x| if .. { Some(..) } else { None }), while this one splits into .filter(..).map(..). Equivalent, arguably more readable, but it's the one divergence in a file that's otherwise uniform across these three functions. Borrowing and allocations match invalid_package_ownership exactly, no gratuitous clones. The doc comment is accurate, and I don't think a shared helper is warranted: only two of the three functions actually twin up, since invalid_team_annotation's owner is an Option.
• Coverage gaps that read as follow-ups rather than blockers: multiple invalid directory owners in one tree, empty .codeowner, a root-level .codeowner, and the generate interaction. Case sensitivity is fine as-is; a lowercase foo against a registered Foo is correctly reported.

On reusing InvalidTeam

Agreed, keep it. But the premise you justified it with doesn't appear to hold, which frees you up if you'd rather do something else.

There's no evidence the gem parses these category headlines. It delegates wholesale to ::RustCodeOwners.validate / generate_and_validate without parsing output, and an org-wide search for "Found invalid team annotations" turns up hits only in this repo. Within the crate, Error::category() has exactly one consumer, the grouping in Display for Errors at validator.rs:256, so there's no hidden parsing path. The compiled code_ownership.bundle can't be indexed, but that native extension is this crate. #108 is the closest precedent, and it says the gem raised the diff as part of the error, which isn't the same claim.

So a new category likely wouldn't have broken anything. I'd still keep the shared one, for a better reason than output stability: Found invalid team annotations already spans package.yml, so in practice it means "invalid team reference in an ownership declaration," and someone who typo'd one team name across an annotation, a package.yml, and a .codeowner wants one grouped list rather than three headlines saying the same thing.

If the looseness bothers you, renaming validator.rs:205 to Found invalid team references is accurate for all three surfaces and is one line plus two test updates. The only caveat is that the gem surfaces this text in raised errors, so it's still user-visible output and consumer repos may have log expectations around it. Low risk, not zero.

On the ownership! macro note

Your description of this is accurate, and it's a real bug rather than a theoretical one. tempdir() is bound inside the macro's block at common_test.rs:22 and drops at block end, and get_codeowners_file (project.rs:173-180) does a live fs::read_to_string guarded by .exists(), so it silently returns "" instead of erroring. validate_codeowners_file then consumes that at validator.rs:146. Probing it directly: base_path no longer exists after the block, and validate() returns Err on an otherwise-clean empty project. Existing tests pass only because none of them call validate() through the macro, exactly as you said. Worth its own issue, and correctly kept out of this PR.

Housekeeping

• No version bump needed. Bumps happen when a release is cut rather than per change: #104 and #109 are standalone Bump version to 0.3.x PRs, and #110 bundled one explicitly "for release." There's no CHANGELOG to update.
• I wouldn't gate this behind a flag. #108 was about output placement, routing the diff to info_messages so the actionable headline wasn't buried, and it kept exit 1, so I don't read it as precedent for gating new failure surfaces. 6d44c0f and 7bd9093 both shipped validate/generate behavior changes directly.
• Consider leading the commit message with the framing that README:237-244 already promises validate ensures "All referenced teams are valid," and .codeowner was the one surface where the code didn't match that promise. That makes this legibly a bug fix rather than a new failure surface, and a one-line note on upgrade impact makes it discoverable in git log, which is how 6d44c0f and 7bd9093 are written.
• If this came from a real report, linking the upstream issue the way 6d44c0f references rubyatscale/code_ownership#149 would help.

[sarastrasner]

Replying to your review (#116 (review)). Description updated to match the code as it now stands.

Rebased onto 99d25cd — main had moved four commits (1.97.1 toolchain bump, perf harness, RUSTSEC deps). Only conflict was the new #[instrument] attributes in validator.rs; both sides kept. On 1.97.1: 26 test binaries pass, clippy --all-targets --all-features -- -D warnings clean, fmt --check clean.

The blocker

Fixed in 2aacc54. The predicate is now self.project.teams_by_name.contains_key(..), definitionally the same lookup DirectoryMapper::entries does, and team_names drops out of the signature entirely.

Companion positive test added, since you were right that nothing would have caught this: tests/fixtures/directory-codeowner-github-team holds Foo in app/by_name/.codeowner and @footeam in app/by_handle/.codeowner against a single config/teams/foo.yml, and asserts validate exits 0 with no invalid team in the output. Every pre-existing .codeowner fixture holds a bare name, which is exactly why the suite stayed green through the stricter predicate.

On the "tighten here vs. fix all three together" fork — tightened here only, and deliberately. Bringing the two siblings onto teams_by_name would loosen them: # @team @footeam and a package.yml owned by a GitHub handle would start passing where they fail today. That's a behavior change on two surfaces this PR isn't about, on an input form the README doesn't document, and it deserves to be argued on its own rather than smuggled in behind a directory-ownership fix. Leaving the divergence you identified in place and visible.

The inheritance detail

Implemented as you sketched it. New InvalidDirectoryTeam variant carrying inherits_from: Option<PathBuf>, category() returning the same string as InvalidTeam so grouping is unchanged:

Found invalid team references
- app/services/nested/.codeowner is referencing an invalid team - 'Web3'; this directory is currently inheriting its owner from app/services

One wrinkle worth flagging: the ancestor walk only considers roots whose own owner resolves (resolvable_roots), so a chain of two bad .codeowner files names the nearest ancestor that actually owns something rather than the nearest one that merely has a file. The fixture pins the whole string, clause included.

Style note taken too — it's .flat_map(|x| if .. { Some(..) } else { None }) now, uniform with the other two.

Empty .codeowner

Fixed in 7ce7060, message only. owner is content.trim() pushed unconditionally with no empty-guard (project_builder.rs:278), so an empty file yields "" and reaches the same silent-inheritance path. Failing is right — the file names no team, so the directory quietly belongs to its ancestor — but you're correct that an invalid team - '' doesn't say so. It now reads:

- app/services/nested/.codeowner is empty and names no team; this directory is currently inheriting its owner from app/services

New fixture empty-directory-codeowner, mirroring the invalid-team one so exactly one error fires — the generated CODEOWNERS is identical either way, since an unresolvable owner and an absent one both drop the directory's line. No change to which projects fail, only to what they're told.

The rename

Went with Found invalid team references and the two test updates. Reached the same conclusion you did on the premise independently: the gem delegates to ::RustCodeOwners.validate / generate_and_validate without inspecting output, and category() has exactly one consumer, the grouping at validator.rs:256. Your caveat stands, though — it's user-visible text in a raised error, so a consumer could have log expectations around it. Low risk, not zero, and a one-line revert if you'd rather not take it.

Left alone

• generate still doesn't route through the validator. Intentional. generate is a writer; making it validate is a scope change with its own upgrade story, and it isn't one this PR should decide.
• Remaining coverage gaps — multiple invalid directory owners in one tree, and a root-level .codeowner. Both untested, both agreed as non-blocking.
• The ownership! macro tempdir drop, which is why this PR uses an integration test rather than a unit test. Out of scope here.

Housekeeping

• No version bump, no CHANGELOG — confirmed against Bump version to 0.3.2 #104 / Bump version to 0.3.3 #109 / fix: don't let git ignore rules override tracked-file inclusion #110.
• 2aacc54's message now leads with the README:237-244 promise (validate ensures "All referenced teams are valid") and carries an explicit upgrade-impact paragraph, matching the shape of 6d44c0f and 7bd9093.
• No upstream issue to link — this came out of running the branch against Gusto/web, not a report.
• Downstream is clear. The single true positive across web's 1,772 .codeowner files — a renamed team leaving an inert file above an app with zero CODEOWNERS lines — is fixed and merged (Gusto/web#33177, 2026-08-18). Whenever a release is cut, web takes it green.