fix: report .codeowner files that reference an unregistered team

src/ownership/validator.rs

@@ -2,7 +2,7 @@ use crate::project::{Project, ProjectFile};
 use core::fmt;
 use std::collections::HashSet;
 use std::fmt::Display;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
 use error_stack::Context;
@@ -27,10 +27,31 @@ pub struct Validator {
 
 #[derive(Debug)]
 enum Error {
-    InvalidTeam { name: String, path: PathBuf },
-    FileWithoutOwner { path: PathBuf },
-    FileWithMultipleOwners { path: PathBuf, owners: Vec<Owner> },
-    CodeownershipFileIsStale { executable_name: String, diff: String },
+    InvalidTeam {
+        name: String,
+        path: PathBuf,
+    },
+    /// A `.codeowner` naming a team that isn't registered. Distinct from `InvalidTeam` only
+    /// so the message can name the ancestor the directory now silently inherits from, which
+    /// is the part a reader needs in order to understand what happened. `category()`
+    /// deliberately matches `InvalidTeam` so one typo'd team name across an annotation, a
+    /// `package.yml`, and a `.codeowner` still groups under a single headline.
+    InvalidDirectoryTeam {
+        name: String,
+        path: PathBuf,
+        inherits_from: Option<PathBuf>,
+    },
+    FileWithoutOwner {
+        path: PathBuf,
+    },
+    FileWithMultipleOwners {
+        path: PathBuf,
+        owners: Vec<Owner>,
+    },
+    CodeownershipFileIsStale {
+        executable_name: String,
+        diff: String,
+    },
 }
 
 #[derive(Debug)]
@@ -65,6 +86,7 @@ impl Validator {
 
         errors.append(&mut self.invalid_team_annotation(&team_names));
         errors.append(&mut self.invalid_package_ownership(&team_names));
+        errors.append(&mut self.invalid_directory_ownership());
 
         errors
     }
@@ -107,6 +129,41 @@ impl Validator {
             .collect()
     }
 
+    /// `DirectoryMapper::entries` skips unresolvable owners, so the directory silently
+    /// inherits its ancestor's owner and nothing else reports the bad name.
+    ///
+    /// Resolves through `teams_by_name` rather than `teams[].name` so the predicate is
+    /// identical to the mapper's lookup: that map is keyed by both `name` and
+    /// `github_team`, and a `.codeowner` holding either one generates a correct line.
+    fn invalid_directory_ownership(&self) -> Vec<Error> {
+        let resolvable_roots: HashSet<&Path> = self
+            .project
+            .directory_codeowner_files
+            .iter()
+            .filter(|directory_codeowner_file| self.project.teams_by_name.contains_key(&directory_codeowner_file.owner))
+            .filter_map(|directory_codeowner_file| directory_codeowner_file.directory_root())
+            .collect();
+
+        self.project
+            .directory_codeowner_files
+            .iter()
+            .flat_map(|directory_codeowner_file| {
+                if !self.project.teams_by_name.contains_key(&directory_codeowner_file.owner) {
+                    Some(Error::InvalidDirectoryTeam {
+                        name: directory_codeowner_file.owner.clone(),
+                        path: self.project.relative_path(&directory_codeowner_file.path).to_owned(),
+                        inherits_from: directory_codeowner_file
+                            .directory_root()
+                            .and_then(|root| root.ancestors().skip(1).find(|ancestor| resolvable_roots.contains(ancestor)))
+                            .map(|ancestor| self.project.relative_path(ancestor).to_owned()),
+                    })
+                } else {
+                    None
+                }
+            })
+            .collect()
+    }
+
     fn validate_file_ownership(&self) -> Vec<Error> {
         let mut validation_errors = Vec::new();
 
@@ -187,7 +244,8 @@ impl Error {
                 Error::CodeownershipFileIsStale { executable_name, diff: _ } => {
                     format!("CODEOWNERS out of date. Run `{}` to update the CODEOWNERS file", executable_name)
                 }
-                Error::InvalidTeam { name: _, path: _ } => "Found invalid team annotations".to_owned(),
+                Error::InvalidTeam { name: _, path: _ } => "Found invalid team references".to_owned(),
+                Error::InvalidDirectoryTeam { .. } => "Found invalid team references".to_owned(),
             }
     }
 
@@ -213,6 +271,16 @@ impl Error {
             // so that a long diff doesn't bury the actionable headline.
             Error::CodeownershipFileIsStale { .. } => vec![],
             Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)],
+            Error::InvalidDirectoryTeam { name, path, inherits_from } => {
+                let mut message = format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name);
+                if let Some(inherits_from) = inherits_from {
+                    message.push_str(&format!(
+                        "; this directory is currently inheriting its owner from {}",
+                        inherits_from.to_string_lossy()
+                    ));
+                }
+                vec![message]
+            }
         }
     }
 }

tests/directory_codeowner_github_team_test.rs

@@ -0,0 +1,22 @@
+use predicates::prelude::*;
+use std::error::Error;
+
+mod common;
+use common::OutputStream;
+use common::run_codeowners;
+
+/// `teams_by_name` is keyed by both `name` and `github_team`, so a `.codeowner` holding
+/// either form generates a correct line. Validation has to accept both, or it fails a
+/// project whose CODEOWNERS is already right — with no command that fixes it.
+#[test]
+fn test_validate_accepts_directory_codeowner_by_name_or_github_team() -> Result<(), Box<dyn Error>> {
+    run_codeowners(
+        "directory-codeowner-github-team",
+        &["validate"],
+        true,
+        OutputStream::Stdout,
+        predicate::str::contains("invalid team").not(),
+    )?;
+
+    Ok(())
+}

tests/fixtures/directory-codeowner-github-team/.github/CODEOWNERS

@@ -0,0 +1,15 @@
+# STOP! - DO NOT EDIT THIS FILE MANUALLY
+# This file was automatically generated by "bin/codeownership validate".
+#
+# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
+# teams. This is useful when developers create Pull Requests since the
+# code/file owner is notified. Reference GitHub docs for more details:
+# https://help.github.com/en/articles/about-code-owners
+
+
+# Owner in .codeowner
+/app/by_handle/**/** @footeam
+/app/by_name/**/** @footeam
+
+# Team YML ownership
+/config/teams/foo.yml @footeam

tests/fixtures/directory-codeowner-github-team/app/by_handle/.codeowner

@@ -0,0 +1 @@
+@footeam

tests/fixtures/directory-codeowner-github-team/app/by_handle/handled.rb

@@ -0,0 +1,2 @@
+class Handled
+end

tests/fixtures/directory-codeowner-github-team/app/by_name/.codeowner

@@ -0,0 +1 @@
+Foo

tests/fixtures/directory-codeowner-github-team/app/by_name/named.rb

@@ -0,0 +1,2 @@
+class Named
+end

tests/fixtures/directory-codeowner-github-team/config/code_ownership.yml

@@ -0,0 +1,10 @@
+---
+owned_globs:
+  - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}"
+unowned_globs:
+  - config/code_ownership.yml
+javascript_package_paths:
+  - javascript/packages/**
+vendored_gems_path: gems
+team_file_glob:
+  - config/teams/**/*.yml

tests/fixtures/directory-codeowner-github-team/config/teams/foo.yml

@@ -0,0 +1,5 @@
+name: Foo
+github:
+  team: "@footeam"
+  members:
+    - fooer

tests/fixtures/invalid-directory-codeowner/.github/CODEOWNERS

@@ -0,0 +1,14 @@
+# STOP! - DO NOT EDIT THIS FILE MANUALLY
+# This file was automatically generated by "bin/codeownership validate".
+#
+# CODEOWNERS is used for GitHub to suggest code/file owners to various GitHub
+# teams. This is useful when developers create Pull Requests since the
+# code/file owner is notified. Reference GitHub docs for more details:
+# https://help.github.com/en/articles/about-code-owners
+
+
+# Owner in .codeowner
+/app/services/**/** @footeam
+
+# Team YML ownership
+/config/teams/foo.yml @footeam

tests/fixtures/invalid-directory-codeowner/app/services/.codeowner

@@ -0,0 +1 @@
+Foo

tests/fixtures/invalid-directory-codeowner/app/services/nested/.codeowner

@@ -0,0 +1 @@
+Web3

tests/fixtures/invalid-directory-codeowner/app/services/nested/nested_file.rb

@@ -0,0 +1,2 @@
+class NestedFile
+end

tests/fixtures/invalid-directory-codeowner/config/code_ownership.yml

@@ -0,0 +1,10 @@
+---
+owned_globs:
+  - "{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}"
+unowned_globs:
+  - config/code_ownership.yml
+javascript_package_paths:
+  - javascript/packages/**
+vendored_gems_path: gems
+team_file_glob:
+  - config/teams/**/*.yml

tests/fixtures/invalid-directory-codeowner/config/teams/foo.yml

@@ -0,0 +1,5 @@
+name: Foo
+github:
+  team: "@footeam"
+  members:
+    - fooer

tests/invalid_directory_codeowner_test.rs

@@ -0,0 +1,26 @@
+use indoc::indoc;
+use predicates::prelude::*;
+use std::error::Error;
+
+mod common;
+use common::OutputStream;
+use common::run_codeowners;
+
+/// A nested `.codeowner` naming an unregistered team, under one naming a real team:
+/// ownership falls through to the ancestor, so nothing else reports the bad name. The
+/// message names that ancestor, since the silent inheritance is the surprising part.
+#[test]
+fn test_validate_reports_directory_codeowner_with_invalid_team() -> Result<(), Box<dyn Error>> {
+    run_codeowners(
+        "invalid-directory-codeowner",
+        &["validate"],
+        false,
+        OutputStream::Stdout,
+        predicate::str::contains(indoc! {"
+            Found invalid team references
+            - app/services/nested/.codeowner is referencing an invalid team - 'Web3'; this directory is currently inheriting its owner from app/services
+        "}),
+    )?;
+
+    Ok(())
+}

tests/invalid_project_test.rs

@@ -61,7 +61,7 @@ fn test_validate() -> Result<(), Box<dyn Error>> {
      owner: Payroll
       - Owner specified in `ruby/app/services/.codeowner`
 
-    Found invalid team annotations
+    Found invalid team references
     - ruby/app/models/blockchain.rb is referencing an invalid team - 'Web3'
 
     Some files are missing ownership