fix: report .codeowner files that reference an unregistered team

[sarastrasner]

Problem

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);
if let Some(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, reusing the existing InvalidTeam error.

Reusing InvalidTeam keeps the output shape and exit codes unchanged, which seemed worth preserving given the wrapping code_ownership gem parses these category headlines. The consequence is that a .codeowner violation is reported under Found invalid team annotations, which is now a little loose — though it already covers package.yml. Happy to give it its own category instead if you'd prefer; I avoided it only because it changes output the gem consumes.

Test

New fixture tests/fixtures/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.

Before this change that fixture exits 0 with empty stdout and stderr. After, it reports:

Found invalid team annotations
- app/services/nested/.codeowner is referencing an invalid team - 'Web3'

Verification

• cargo test — all pass (added 1)
• cargo clippy --all-targets --all-features -- -D warnings — clean
• cargo fmt --all -- --check — clean

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 an integration test. Happy to open a separate issue or fix it if useful.

[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.name and team.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.