From 162ed64c166ce4f2c966021eb66e1d5db94f2cf4 Mon Sep 17 00:00:00 2001 From: Ben Judd Date: Tue, 17 Feb 2026 15:39:19 -0500 Subject: [PATCH 1/2] Allow self-satisfaction of codeowner groups. --- codeowners.toml | 3 + internal/app/app.go | 6 +- internal/app/app_test.go | 7 +- internal/config/config.go | 1 + internal/config/config_test.go | 21 +++++ pkg/codeowners/codeowners.go | 16 ++-- pkg/codeowners/codeowners_test.go | 131 +++++++++++++++++++++++++++++- pkg/codeowners/merger_test.go | 33 +++++++- tools/cli/main_test.go | 10 +-- 9 files changed, 208 insertions(+), 20 deletions(-) diff --git a/codeowners.toml b/codeowners.toml index 7ffca62..594c810 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -15,6 +15,9 @@ disable_smart_dismissal = false # `require_both_branch_reviewers` (default false) requires approval from codeowners defined in BOTH the base branch AND the PR branch # This is useful for ownership handoffs where both the outgoing and incoming teams must approve the change require_both_branch_reviewers = false +# `allow_self_approval` (default false) treats the PR author as satisfying any OR ownership group they belong to +# If the author is listed in an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve +allow_self_approval = false # `enforcement` allows you to specify how the codeowners check should be enforced [enforcement] diff --git a/internal/app/app.go b/internal/app/app.go index c95fbd4..69c0b41 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -167,7 +167,11 @@ func (a *App) Run() (*OutputData, error) { // Set author author := fmt.Sprintf("@%s", a.client.PR().User.GetLogin()) - codeOwners.SetAuthor(author) + authorMode := codeowners.AuthorModeDefault + if conf.AllowSelfApproval { + authorMode = codeowners.AuthorModeSelfApproval + } + codeOwners.SetAuthor(author, authorMode) // Warn about unowned files if !conf.SuppressUnownedWarning { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 4b94038..dd774bb 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -77,14 +77,13 @@ func (m *mockCodeOwners) ApplyApprovals(approvers []codeowners.Slug) { m.appliedApprovals = approvers } -func (m *mockCodeOwners) SetAuthor(author string) { +func (m *mockCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode) { m.author = author - // Remove author from reviewers for _, reviewers := range m.requiredOwners { for i, name := range reviewers.Names { if name.EqualsString(author) { reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) - if len(reviewers.Names) == 0 { + if len(reviewers.Names) == 0 || mode == codeowners.AuthorModeSelfApproval { reviewers.Approved = true } break @@ -95,7 +94,7 @@ func (m *mockCodeOwners) SetAuthor(author string) { for i, name := range reviewers.Names { if name.EqualsString(author) { reviewers.Names = append(reviewers.Names[:i], reviewers.Names[i+1:]...) - if len(reviewers.Names) == 0 { + if len(reviewers.Names) == 0 || mode == codeowners.AuthorModeSelfApproval { reviewers.Approved = true } break diff --git a/internal/config/config.go b/internal/config/config.go index b089b91..537a33b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -19,6 +19,7 @@ type Config struct { DisableSmartDismissal bool `toml:"disable_smart_dismissal"` RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"` SuppressUnownedWarning bool `toml:"suppress_unowned_warning"` + AllowSelfApproval bool `toml:"allow_self_approval"` } type Enforcement struct { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 176b450..cbc26db 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -106,6 +106,23 @@ suppress_unowned_warning = true }, expectedErr: false, }, + { + name: "config with allow_self_approval enabled", + configContent: ` +allow_self_approval = true +`, + path: "testdata/", + expected: &Config{ + MaxReviews: nil, + MinReviews: nil, + UnskippableReviewers: []string{}, + Ignore: []string{}, + Enforcement: &Enforcement{Approval: false, FailCheck: true}, + HighPriorityLabels: []string{}, + AllowSelfApproval: true, + }, + expectedErr: false, + }, { name: "invalid toml", configContent: ` @@ -192,6 +209,10 @@ max_reviews = invalid t.Errorf("SuppressUnownedWarning: expected %v, got %v", tc.expected.SuppressUnownedWarning, got.SuppressUnownedWarning) } + if got.AllowSelfApproval != tc.expected.AllowSelfApproval { + t.Errorf("AllowSelfApproval: expected %v, got %v", tc.expected.AllowSelfApproval, got.AllowSelfApproval) + } + if tc.expected.Enforcement != nil { if got.Enforcement == nil { t.Error("expected Enforcement to be set") diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 2e34a01..95ec9a8 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -9,10 +9,16 @@ import ( "github.com/multimediallc/codeowners-plus/pkg/functional" ) +type AuthorMode int + +const ( + AuthorModeDefault AuthorMode = iota // Author is removed from their groups; group is only auto-satisfied if it becomes empty + AuthorModeSelfApproval // Author auto-satisfies any OR group they belong to +) + // CodeOwners represents a collection of owned files, with reverse lookups for owners and reviewers type CodeOwners interface { - // SetAuthor sets the author of the PR - SetAuthor(author string) + SetAuthor(author string, mode AuthorMode) // FileRequired returns a map of file names to their required reviewers FileRequired() map[string]ReviewerGroups @@ -54,15 +60,13 @@ type ownersMap struct { unownedFiles []string } -func (om *ownersMap) SetAuthor(author string) { +func (om *ownersMap) SetAuthor(author string, mode AuthorMode) { authorSlug := NewSlug(author) for _, reviewers := range om.nameReviewerMap[authorSlug.Normalized()] { - // remove author from the reviewers list (case-insensitive) reviewers.Names = slices.DeleteFunc(reviewers.Names, func(name Slug) bool { return name.Equals(authorSlug) }) - if len(reviewers.Names) == 0 { - // mark the reviewer as approved if they are the author + if len(reviewers.Names) == 0 || mode == AuthorModeSelfApproval { reviewers.Approved = true } } diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index 35e8b0f..a25c3f6 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -214,7 +214,7 @@ func TestSetAuthor(t *testing.T) { if err != nil { t.Errorf("Error getting owners: %v", err) } - owners.SetAuthor("@b-owner") + owners.SetAuthor("@b-owner", AuthorModeDefault) delete(expectedOwners, "@b-owner") // This should be removed by SetAuthor allReviewers := f.RemoveDuplicates(append(owners.AllRequired().Flatten(), owners.AllOptional().Flatten()...)) @@ -247,7 +247,7 @@ func TestSetAuthorCaseInsensitive(t *testing.T) { } // Set author with different casing than what's in the .codeowners file - owners.SetAuthor("@B-OWNER") // .codeowners has @b-owner + owners.SetAuthor("@B-OWNER", AuthorModeDefault) // .codeowners has @b-owner // Verify that @b-owner was removed from reviewers allReviewers := owners.AllRequired().Flatten() @@ -262,7 +262,7 @@ func TestSetAuthorCaseInsensitive(t *testing.T) { if err != nil { t.Errorf("Error getting owners: %v", err) } - owners2.SetAuthor("@b-owner") + owners2.SetAuthor("@b-owner", AuthorModeDefault) allReviewers2 := owners2.AllRequired().Flatten() // Both should have the same result @@ -271,6 +271,131 @@ func TestSetAuthorCaseInsensitive(t *testing.T) { } } +// When the author is in a multi-member OR group (e.g. `*.py @alice @bob`) and self-approval +// is enabled, the author implicitly vouches for their own code — the entire group should be +// satisfied without needing @bob to also approve. +func TestSetAuthorSelfApprovalMultiMemberORGroup(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 0 { + t.Errorf("Expected OR group to be auto-satisfied with self-approval, got %d still required", len(allRequired)) + } +} + +// Contrast to the self-approval case: with default mode, the author is removed from the OR +// group but the remaining members still need to approve. So `*.py @alice @bob` with @alice as +// author means @bob must still review. +func TestSetAuthorDefaultMultiMemberORGroup(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeDefault) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected 1 still-required group, got %d", len(allRequired)) + } + if len(allRequired[0].Names) != 1 || allRequired[0].Names[0].Normalized() != "@bob" { + t.Errorf("Expected remaining required reviewer to be @bob, got %v", OriginalStrings(allRequired[0].Names)) + } +} + +// When the author appears in multiple OR groups across different files, self-approval should +// satisfy all of them — not just the first one encountered. +func TestSetAuthorSelfApprovalMultipleORGroups(t *testing.T) { + orGroup1 := &ReviewerGroup{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false} + orGroup2 := &ReviewerGroup{Names: NewSlugs([]string{"@alice", "@charlie"}), Approved: false} + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file1.py": {orGroup1}, + "file2.py": {orGroup2}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 0 { + t.Errorf("Expected all OR groups containing author to be satisfied, got %d still required", len(allRequired)) + } +} + +// Self-approval only applies to OR groups the author belongs to. If a file also has a separate +// AND group (e.g. `&*.py @security`), that group must still be independently satisfied — +// the author being in one OR group doesn't grant them approval power over unrelated AND groups. +func TestSetAuthorSelfApprovalDoesNotAffectOtherANDGroups(t *testing.T) { + orGroup := &ReviewerGroup{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false} + andGroup := &ReviewerGroup{Names: NewSlugs([]string{"@security"}), Approved: false} + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {orGroup, andGroup}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected 1 still-required AND group, got %d", len(allRequired)) + } + if allRequired[0].Names[0].Normalized() != "@security" { + t.Errorf("Expected @security to still be required, got %v", OriginalStrings(allRequired[0].Names)) + } +} + +// GitHub usernames are case-insensitive. If the PR author is `@ALICE` but the .codeowners +// file lists `@alice`, self-approval should still match and satisfy the group. +func TestSetAuthorSelfApprovalCaseInsensitive(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@ALICE", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 0 { + t.Errorf("Expected case-insensitive self-approval to satisfy the group, got %d still required", len(allRequired)) + } +} + +// When self-approval is enabled but the author isn't listed in any ownership group, +// nothing should change — all groups remain required with their original members. +func TestSetAuthorSelfApprovalAuthorNotInAnyGroup(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@bob", "@charlie"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@outsider", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected group to remain required when author is not a member, got %d", len(allRequired)) + } + if len(allRequired[0].Names) != 2 { + t.Errorf("Expected group members unchanged, got %v", OriginalStrings(allRequired[0].Names)) + } +} + func TestApplyApprovalsCaseInsensitive(t *testing.T) { owners, _, err := setupOwnersMap() if err != nil { diff --git a/pkg/codeowners/merger_test.go b/pkg/codeowners/merger_test.go index 131b213..43a68a1 100644 --- a/pkg/codeowners/merger_test.go +++ b/pkg/codeowners/merger_test.go @@ -324,7 +324,7 @@ func TestMergeCodeOwnersSetAuthor(t *testing.T) { merged := MergeCodeOwners(base, head) // Set author - merged.SetAuthor("@author") + merged.SetAuthor("@author", AuthorModeDefault) // Check that author is removed from reviewer groups or groups are auto-approved allRequired := merged.AllRequired() @@ -337,6 +337,37 @@ func TestMergeCodeOwnersSetAuthor(t *testing.T) { } } +// With require_both_branch_reviewers, ownership rules from base and head are AND'd together. +// Self-approval should only satisfy groups the author belongs to. Here the author is in the +// base branch's OR group but not the head branch's group, so only the base group is satisfied +// and the head's @team-b still requires an independent review. +func TestMergeCodeOwnersSetAuthorSelfApproval(t *testing.T) { + baseRequired := map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@author", "@team-a"}), Approved: false}, + }, + } + headRequired := map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-b"}), Approved: false}, + }, + } + + base := createMockCodeOwners(baseRequired, map[string]ReviewerGroups{}, []string{}) + head := createMockCodeOwners(headRequired, map[string]ReviewerGroups{}, []string{}) + + merged := MergeCodeOwners(base, head) + merged.SetAuthor("@author", AuthorModeSelfApproval) + + allRequired := merged.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected 1 still-required group (head's @team-b), got %d", len(allRequired)) + } + if allRequired[0].Names[0].Normalized() != "@team-b" { + t.Errorf("Expected @team-b to still be required, got %v", OriginalStrings(allRequired[0].Names)) + } +} + // Helper functions // createMockCodeOwners creates a mock CodeOwners object for testing diff --git a/tools/cli/main_test.go b/tools/cli/main_test.go index b5e6adb..fdd7766 100644 --- a/tools/cli/main_test.go +++ b/tools/cli/main_test.go @@ -416,11 +416,11 @@ func (f *fakeCodeOwners) FileRequired() map[string]codeowners.ReviewerGroups { func (f *fakeCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { return f.optional } -func (f *fakeCodeOwners) SetAuthor(author string) {} -func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) UnownedFiles() []string { return nil } -func (f *fakeCodeOwners) ApplyApprovals(approvers []codeowners.Slug) {} +func (f *fakeCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode) {} +func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) UnownedFiles() []string { return nil } +func (f *fakeCodeOwners) ApplyApprovals(approvers []codeowners.Slug) {} func TestJsonTargets(t *testing.T) { owners := &fakeCodeOwners{ From 73c1a42b3c9f77b2a323f47c01b0224ee1e66aa1 Mon Sep 17 00:00:00 2001 From: Ben Judd <14916819+Icantjuddle@users.noreply.github.com> Date: Wed, 18 Feb 2026 10:54:23 -0500 Subject: [PATCH 2/2] Update readme and codecov badge. --- README.md | 6 +++++- codeowners.toml | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 23f3aab..96c80b3 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-83.2%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.0%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -241,6 +241,10 @@ require_both_branch_reviewers = false # Useful when you intentionally have files without codeowners suppress_unowned_warning = true +# `allow_self_approval` (default false) treats the PR author as satisfying any OR ownership group they belong to +# If the PR author is included an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve +allow_self_approval = false + # `enforcement` allows you to specify how the Codeowners Plus check should be enforced [enforcement] # see "Enforcement Options" below for more details diff --git a/codeowners.toml b/codeowners.toml index 594c810..336ac0d 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -16,7 +16,7 @@ disable_smart_dismissal = false # This is useful for ownership handoffs where both the outgoing and incoming teams must approve the change require_both_branch_reviewers = false # `allow_self_approval` (default false) treats the PR author as satisfying any OR ownership group they belong to -# If the author is listed in an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve +# If the PR author is included an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve allow_self_approval = false # `enforcement` allows you to specify how the codeowners check should be enforced