Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions codeowners.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
[enforcement]
Expand Down
6 changes: 5 additions & 1 deletion internal/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 3 additions & 4 deletions internal/app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: `
Expand Down Expand Up @@ -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")
Expand Down
16 changes: 10 additions & 6 deletions pkg/codeowners/codeowners.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}
Expand Down
131 changes: 128 additions & 3 deletions pkg/codeowners/codeowners_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()...))
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand All @@ -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 {
Expand Down
33 changes: 32 additions & 1 deletion pkg/codeowners/merger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
Expand Down
10 changes: 5 additions & 5 deletions tools/cli/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading