From 28c7815024b4b61a217193da9cc3a76a6182e84f Mon Sep 17 00:00:00 2001 From: kpurdon Date: Wed, 18 Feb 2026 10:02:43 -0700 Subject: [PATCH] feat: default to exact CODEOWNERS matching Previously, directories were considered covered if any parent CODEOWNERS rule matched them. This made it easy to miss adding explicit entries for new subdirectories when a broad parent rule already existed. The default match mode is now "exact", requiring each directory to have its own CODEOWNERS rule. Users can opt out per-directory with `match: coverage` to restore the previous behavior. Co-Authored-By: Claude --- README.md | 30 ++++++++++-- main.go | 54 +++++++++++++++++++--- main_test.go | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index f95f531..c974fb4 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,27 @@ That's it! The action will fail if any configured directories lack CODEOWNERS en ## Configuration +### Match mode + +Controls how CODEOWNERS coverage is checked for each directory: + +| Mode | Behavior | +|------|----------| +| `exact` (default) | Directory must have its own CODEOWNERS rule. Inheritance from a parent rule is not sufficient. | +| `coverage` | Directory only needs to be covered by any CODEOWNERS rule, including inherited parent rules. | + +```yaml +directories: + - path: services + level: 1 + # Default: each services/* must have its own CODEOWNERS entry + - path: internal + level: 1 + match: coverage # Opt out: parent coverage is acceptable +``` + +For example, if CODEOWNERS contains `/apps/ @team-apps`, a subdirectory `apps/my-service/` is covered by inheritance. With the default `exact` mode, this would fail because `apps/my-service/` lacks its own entry. With `coverage` mode, it would pass. + ### Level explained | Level | Behavior | Example | @@ -52,10 +73,13 @@ This matches `applications/a/services`, `applications/b/services`, etc., and che ```yaml directories: - path: services - level: 1 # services/auth/, services/api/, etc. + level: 1 # services/auth/, services/api/, etc. must each have an exact CODEOWNERS entry - path: libs - level: 2 # libs/go/utils/, libs/js/common/, etc. - - path: docs # docs/ itself (level defaults to 0) + level: 2 # libs/go/utils/, libs/js/common/, etc. + - path: internal + level: 1 + match: coverage # parent CODEOWNERS coverage is acceptable + - path: docs # docs/ itself (level defaults to 0) ``` ## GitHub Action diff --git a/main.go b/main.go index 345aacf..9b67214 100644 --- a/main.go +++ b/main.go @@ -18,6 +18,7 @@ type config struct { type dirSpec struct { Path string `yaml:"path"` Level int `yaml:"level"` + Match string `yaml:"match"` } type validationError struct { @@ -114,6 +115,9 @@ func loadConfig(path string) (*config, error) { if d.Level < 0 { return nil, fmt.Errorf("directory %s has invalid level %d (must be >= 0)", d.Path, d.Level) } + if d.Match != "" && d.Match != "exact" && d.Match != "coverage" { + return nil, fmt.Errorf("directory %s has invalid match %q (must be \"exact\" or \"coverage\")", d.Path, d.Match) + } } return &cfg, nil @@ -193,8 +197,13 @@ func validate(specs []dirSpec, ruleset codeowners.Ruleset, configPath string) [] continue } + matchMode := spec.Match + if matchMode == "" { + matchMode = "exact" + } + for _, dir := range matchedDirs { - errs := validateDirectory(dir, spec.Level, ruleset, configPath) + errs := validateDirectory(dir, spec.Level, matchMode, ruleset, configPath) errors = append(errors, errs...) } } @@ -202,7 +211,7 @@ func validate(specs []dirSpec, ruleset codeowners.Ruleset, configPath string) [] return errors } -func validateDirectory(path string, level int, ruleset codeowners.Ruleset, configPath string) []validationError { +func validateDirectory(path string, level int, matchMode string, ruleset codeowners.Ruleset, configPath string) []validationError { var errors []validationError info, err := os.Stat(path) @@ -240,11 +249,24 @@ func validateDirectory(path string, level int, ruleset codeowners.Ruleset, confi } for _, d := range dirsToCheck { - if !hasCodeownersCoverage(ruleset, d) { - errors = append(errors, validationError{ - path: d, - message: fmt.Sprintf("Not covered by CODEOWNERS. Add: /%s/ @your-team", d), - }) + if matchMode == "coverage" { + if !hasCodeownersCoverage(ruleset, d) { + errors = append(errors, validationError{ + path: d, + message: fmt.Sprintf("Not covered by CODEOWNERS. Add: /%s/ @your-team", d), + }) + } + } else { + if !hasExactCodeownersCoverage(ruleset, d) { + msg := fmt.Sprintf("Not covered by CODEOWNERS. Add: /%s/ @your-team", d) + if hasCodeownersCoverage(ruleset, d) { + msg = fmt.Sprintf("Covered by parent CODEOWNERS rule but missing exact entry. Add: /%s/ @your-team", d) + } + errors = append(errors, validationError{ + path: d, + message: msg, + }) + } } } @@ -275,6 +297,24 @@ func getDirsAtLevel(dir string, level int) ([]string, error) { return results, nil } +func hasExactCodeownersCoverage(ruleset codeowners.Ruleset, dir string) bool { + dir = filepath.Clean(dir) + + rule, _ := ruleset.Match(dir + "/file.txt") + if rule == nil || len(rule.Owners) == 0 { + return false + } + + // If the parent matches the same rule pattern, this dir is only covered by inheritance + parent := filepath.Dir(dir) + parentRule, _ := ruleset.Match(parent + "/file.txt") + if parentRule != nil && rule.RawPattern() == parentRule.RawPattern() { + return false + } + + return true +} + func hasCodeownersCoverage(ruleset codeowners.Ruleset, dir string) bool { dir = filepath.Clean(dir) diff --git a/main_test.go b/main_test.go index 22f4e22..3a73ab0 100644 --- a/main_test.go +++ b/main_test.go @@ -51,6 +51,31 @@ func TestLoadConfig(t *testing.T) { wantErr: true, errMsg: "parsing config", }, + { + name: "valid match exact", + content: `directories: + - path: src + match: exact +`, + wantErr: false, + }, + { + name: "valid match coverage", + content: `directories: + - path: src + match: coverage +`, + wantErr: false, + }, + { + name: "invalid match value", + content: `directories: + - path: src + match: invalid +`, + wantErr: true, + errMsg: "invalid match", + }, } for _, tt := range tests { @@ -392,3 +417,104 @@ internal/ @team-c }) } } + +func TestHasExactCodeownersCoverage(t *testing.T) { + content := `/apps/ @team-apps +/apps/autotune/ @team-autotune +/services/foo/ @team-foo +` + ruleset, err := codeowners.ParseFile(strings.NewReader(content)) + if err != nil { + t.Fatalf("parsing CODEOWNERS: %v", err) + } + + tests := []struct { + name string + dir string + want bool + }{ + {"dir with its own rule", "apps/autotune", true}, + {"dir covered only by parent rule", "apps/other", false}, + {"top-level dir with its own rule", "apps", true}, + {"completely uncovered dir", "unknown", false}, + {"dir with own rule no parent", "services/foo", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := hasExactCodeownersCoverage(ruleset, tt.dir) + if got != tt.want { + t.Errorf("hasExactCodeownersCoverage(%q) = %v, want %v", tt.dir, got, tt.want) + } + }) + } +} + +func TestValidateMatchModes(t *testing.T) { + tmpDir := t.TempDir() + + // Create directory structure: apps/ with a subdirectory that has no explicit rule + os.MkdirAll(filepath.Join(tmpDir, "apps", "covered"), 0755) + os.MkdirAll(filepath.Join(tmpDir, "apps", "inherited"), 0755) + os.MkdirAll(filepath.Join(tmpDir, ".github"), 0755) + + // /apps/ covers everything under apps/, but only /apps/covered/ has an explicit rule + os.WriteFile(filepath.Join(tmpDir, ".github", "CODEOWNERS"), []byte(`/apps/ @team-apps +/apps/covered/ @team-covered +`), 0644) + + oldWd, _ := os.Getwd() + os.Chdir(tmpDir) + defer os.Chdir(oldWd) + + ruleset, err := loadCodeowners("") + if err != nil { + t.Fatalf("loading CODEOWNERS: %v", err) + } + + tests := []struct { + name string + specs []dirSpec + wantErrs int + }{ + { + name: "exact mode (default) - inherited coverage fails", + specs: []dirSpec{{Path: "apps", Level: 1}}, + wantErrs: 1, // apps/inherited only has parent coverage + }, + { + name: "exact mode explicit - inherited coverage fails", + specs: []dirSpec{{Path: "apps", Level: 1, Match: "exact"}}, + wantErrs: 1, + }, + { + name: "coverage mode - inherited coverage passes", + specs: []dirSpec{{Path: "apps", Level: 1, Match: "coverage"}}, + wantErrs: 0, // both subdirs are covered (inherited is fine) + }, + { + name: "exact mode level 0 - dir with own rule passes", + specs: []dirSpec{{Path: "apps/covered", Level: 0}}, + wantErrs: 0, + }, + { + name: "exact mode level 0 - dir with only inherited rule fails", + specs: []dirSpec{{Path: "apps/inherited", Level: 0}}, + wantErrs: 1, + }, + { + name: "coverage mode level 0 - dir with only inherited rule passes", + specs: []dirSpec{{Path: "apps/inherited", Level: 0, Match: "coverage"}}, + wantErrs: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errs := validate(tt.specs, ruleset, ".requirecodeowners.yml") + if len(errs) != tt.wantErrs { + t.Errorf("validate() errors = %v, want %d errors", errs, tt.wantErrs) + } + }) + } +}