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
37 changes: 36 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-82.8%25-brightgreen)
![Coverage](https://img.shields.io/badge/Coverage-83.2%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 @@ -232,6 +232,11 @@ detailed_reviewers = true
# `disable_smart_dismissal` (default false) means the codeowners will not dismiss stale reviews
disable_smart_dismissal = true

# `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 review
require_both_branch_reviewers = false

# `enforcement` allows you to specify how the Codeowners Plus check should be enforced
[enforcement]
# see "Enforcement Options" below for more details
Expand Down Expand Up @@ -308,6 +313,36 @@ Codeowners Plus automatically detects and validates the bypass approval, immedia

The bypass text is case-insensitive, so "codeowners bypass", "Codeowners Bypass", or "CODEOWNERS BYPASS" all work.

#### Require Both Branch Reviewers (Ownership Handoffs)

The `require_both_branch_reviewers` feature enables self-service ownership transfers by requiring approval from codeowners defined in **BOTH** the base branch and the PR branch. This creates an AND relationship between ownership rules from both branches.

**Use Case:** When Team A wants to transfer ownership of files to Team B, they can submit a PR that modifies the `.codeowners` file. With `require_both_branch_reviewers` enabled, the PR will require approval from **both** Team A (base branch owners) and Team B (PR branch owners), ensuring both parties agree to the handoff.

`codeowners.toml`:
```toml
# `require_both_branch_reviewers` (default false) requires approval from BOTH base and PR branch codeowners
require_both_branch_reviewers = true
```

**Example Ownership Handoff:**

Base branch `.codeowners`:
```
*.py @backend-team
```

PR branch `.codeowners` (modified in the PR):
```
*.py @data-team
```

Result with `require_both_branch_reviewers = true`:
- Any `.py` file changes require approval from **both** `@backend-team` AND `@data-team`
- This ensures the outgoing team (`@backend-team`) and incoming team (`@data-team`) both approve the ownership transfer

**Note:** The `require_both_branch_reviewers` setting is read from the base branch's `codeowners.toml` for security. PR authors cannot enable this feature for their own PRs.

### Quiet Mode

Using the `quiet` input on the action will change the behavior in a couple ways:
Expand Down
3 changes: 3 additions & 0 deletions codeowners.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ high_priority_labels = ["P0"]
detailed_reviewers = false
# `disable_smart_dismissal` (default false) means the codeowners will not dismiss stale reviews
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

# `enforcement` allows you to specify how the codeowners check should be enforced
[enforcement]
Expand Down
36 changes: 30 additions & 6 deletions internal/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ func (a *App) Run() (*OutputData, error) {

// Create file reader for base ref to prevent PR authors from modifying config or .codeowners
// This ensures the security policy comes from the protected branch, not the PR branch
fileReader := git.NewGitRefFileReader(a.client.PR().Base.GetSHA(), a.config.RepoDir)
baseFileReader := git.NewGitRefFileReader(a.client.PR().Base.GetSHA(), a.config.RepoDir)
a.printDebug("Using base ref %s for codeowners.toml and .codeowners files\n", a.client.PR().Base.GetSHA())

// Read config from base ref
conf, err := owners.ReadConfig(a.config.RepoDir, fileReader)
conf, err := owners.ReadConfig(a.config.RepoDir, baseFileReader)
if err != nil {
a.printWarn("Error reading codeowners.toml - using default config\n")
}
Expand All @@ -134,10 +134,34 @@ func (a *App) Run() (*OutputData, error) {
}
a.gitDiff = gitDiff

// Initialize codeowners using base ref file reader (same reader as config)
codeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), fileReader, a.config.WarningBuffer)
if err != nil {
return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err)
// Initialize codeowners
var codeOwners codeowners.CodeOwners
if conf.RequireBothBranchReviewers {
// Require both branch reviewers mode: read .codeowners from BOTH base and head, then merge
a.printDebug("Require both branch reviewers mode enabled - reading .codeowners from both base and head refs\n")

// Create base codeowners from base ref
baseCodeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), baseFileReader, a.config.WarningBuffer)
if err != nil {
return &OutputData{}, fmt.Errorf("NewCodeOwners (base) Error: %v", err)
}

// Create head file reader and codeowners from head ref
headFileReader := git.NewGitRefFileReader(a.client.PR().Head.GetSHA(), a.config.RepoDir)
headCodeOwners, err := codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), headFileReader, a.config.WarningBuffer)
if err != nil {
return &OutputData{}, fmt.Errorf("NewCodeOwners (head) Error: %v", err)
}

// Merge base and head codeowners using AND logic
codeOwners = codeowners.MergeCodeOwners(baseCodeOwners, headCodeOwners)
a.printDebug("Merged ownership rules from base and head refs\n")
} else {
// Standard mode: read .codeowners only from base ref
codeOwners, err = codeowners.New(a.config.RepoDir, gitDiff.AllChanges(), baseFileReader, a.config.WarningBuffer)
if err != nil {
return &OutputData{}, fmt.Errorf("NewCodeOwners Error: %v", err)
}
}
a.codeowners = codeOwners

Expand Down
37 changes: 20 additions & 17 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,16 @@ import (
)

type Config struct {
MaxReviews *int `toml:"max_reviews"`
MinReviews *int `toml:"min_reviews"`
UnskippableReviewers []string `toml:"unskippable_reviewers"`
Ignore []string `toml:"ignore"`
Enforcement *Enforcement `toml:"enforcement"`
HighPriorityLabels []string `toml:"high_priority_labels"`
AdminBypass *AdminBypass `toml:"admin_bypass"`
DetailedReviewers bool `toml:"detailed_reviewers"`
DisableSmartDismissal bool `toml:"disable_smart_dismissal"`
MaxReviews *int `toml:"max_reviews"`
MinReviews *int `toml:"min_reviews"`
UnskippableReviewers []string `toml:"unskippable_reviewers"`
Ignore []string `toml:"ignore"`
Enforcement *Enforcement `toml:"enforcement"`
HighPriorityLabels []string `toml:"high_priority_labels"`
AdminBypass *AdminBypass `toml:"admin_bypass"`
DetailedReviewers bool `toml:"detailed_reviewers"`
DisableSmartDismissal bool `toml:"disable_smart_dismissal"`
RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"`
}

type Enforcement struct {
Expand All @@ -35,14 +36,16 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error)
}

defaultConfig := &Config{
MaxReviews: nil,
MinReviews: nil,
UnskippableReviewers: []string{},
Ignore: []string{},
Enforcement: &Enforcement{Approval: false, FailCheck: true},
HighPriorityLabels: []string{},
AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}},
DetailedReviewers: false,
MaxReviews: nil,
MinReviews: nil,
UnskippableReviewers: []string{},
Ignore: []string{},
Enforcement: &Enforcement{Approval: false, FailCheck: true},
HighPriorityLabels: []string{},
AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}},
DetailedReviewers: false,
DisableSmartDismissal: false,
RequireBothBranchReviewers: false,
}

// Use filesystem reader if none provided
Expand Down
24 changes: 24 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,26 @@ unskippable_reviewers = ["@user1"]
},
expectedErr: false,
},
{
name: "config with require_both_branch_reviewers enabled",
configContent: `
require_both_branch_reviewers = true
max_reviews = 2
`,
path: "testdata/",
expected: &Config{
MaxReviews: intPtr(2),
MinReviews: nil,
UnskippableReviewers: []string{},
Ignore: []string{},
Enforcement: &Enforcement{Approval: false, FailCheck: true},
HighPriorityLabels: []string{},
DetailedReviewers: false,
DisableSmartDismissal: false,
RequireBothBranchReviewers: true,
},
expectedErr: false,
},
{
name: "invalid toml",
configContent: `
Expand Down Expand Up @@ -147,6 +167,10 @@ max_reviews = invalid
t.Errorf("Ignore: expected %v, got %v", tc.expected.Ignore, got.Ignore)
}

if got.RequireBothBranchReviewers != tc.expected.RequireBothBranchReviewers {
t.Errorf("RequireBothBranchReviewers: expected %v, got %v", tc.expected.RequireBothBranchReviewers, got.RequireBothBranchReviewers)
}

if tc.expected.Enforcement != nil {
if got.Enforcement == nil {
t.Error("expected Enforcement to be set")
Expand Down
152 changes: 152 additions & 0 deletions pkg/codeowners/merger.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
package codeowners

import (
"slices"
"strings"

f "github.com/multimediallc/codeowners-plus/pkg/functional"
)

// MergeCodeOwners combines two CodeOwners objects using AND logic.
// The result requires satisfaction of ownership rules from BOTH base and head.
// This is useful for ownership handoffs where both outgoing and incoming teams must approve.
func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners {
// Get file ownership maps from both branches
baseRequired := base.FileRequired()
headRequired := head.FileRequired()
baseOptional := base.FileOptional()
headOptional := head.FileOptional()

// Get all unique file names from both branches
allFiles := getAllFileNames(baseRequired, headRequired, baseOptional, headOptional)

// Build merged ownership map
mergedFileToOwner := make(map[string]fileOwners)
for _, file := range allFiles {
baseReq := baseRequired[file]
headReq := headRequired[file]
baseOpt := baseOptional[file]
headOpt := headOptional[file]

// Merge required reviewers (AND logic - both must be satisfied)
mergedRequired := mergeReviewerGroups(baseReq, headReq)

// Merge optional reviewers (both are CC'd)
mergedOptional := mergeReviewerGroups(baseOpt, headOpt)

mergedFileToOwner[file] = fileOwners{
requiredReviewers: mergedRequired,
optionalReviewers: mergedOptional,
}
}

// Merge unowned files
mergedUnowned := mergeUnownedFiles(base.UnownedFiles(), head.UnownedFiles(), allFiles)

// Build nameReviewerMap for approval tracking
nameReviewerMap := buildNameReviewerMap(mergedFileToOwner)

return &ownersMap{
author: "",
fileToOwner: mergedFileToOwner,
nameReviewerMap: nameReviewerMap,
unownedFiles: mergedUnowned,
}
}

// getAllFileNames returns a deduplicated, sorted list of all file names from multiple maps
func getAllFileNames(maps ...map[string]ReviewerGroups) []string {
fileSet := make(map[string]bool)
for _, m := range maps {
for file := range m {
fileSet[file] = true
}
}

files := make([]string, 0, len(fileSet))
for file := range fileSet {
files = append(files, file)
}

// Sort for deterministic output
slices.Sort(files)
return files
}

// mergeReviewerGroups combines two ReviewerGroups using AND logic.
func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroups {
// Combine both groups
combined := make([]*ReviewerGroup, 0, len(base)+len(head))
seen := make(map[string]bool)

// add reviewer group with deduplication
addGroups := func(rgs ReviewerGroups) {
for _, rg := range rgs {
key := createReviewerGroupKey(rg)
if seen[key] {
continue
}
combined = append(combined, rg)
seen[key] = true
}
}

addGroups(base)
addGroups(head)

return combined
}

// createReviewerGroupKey creates a unique key for a ReviewerGroup based on its normalized names
func createReviewerGroupKey(rg *ReviewerGroup) string {
normalizedNames := f.Map(rg.Names, func(s Slug) string { return s.Normalized() })
slices.Sort(normalizedNames)
return strings.Join(normalizedNames, ",")
}

// mergeUnownedFiles combines unowned files from both branches, excluding files that have owners
func mergeUnownedFiles(baseUnowned []string, headUnowned []string, filesWithOwners []string) []string {
// Create a set of files that have owners
ownedSet := f.NewSet[string]()
for _, file := range filesWithOwners {
ownedSet.Add(file)
}

// Combine unowned files from both branches
unownedSet := f.NewSet[string]()
addToUnowned := func(filenames []string) {
for _, file := range filenames {
if !ownedSet.Contains(file) {
unownedSet.Add(file)
}
}
}
addToUnowned(baseUnowned)
addToUnowned(headUnowned)

// Convert to sorted slice
unowned := unownedSet.Items()
slices.Sort(unowned)
return unowned
}

// buildNameReviewerMap creates a reverse lookup from normalized reviewer names to their ReviewerGroups
func buildNameReviewerMap(fileToOwner map[string]fileOwners) map[string]ReviewerGroups {
nameReviewerMap := make(map[string]ReviewerGroups)

addReviewerGroups := func(rgs ReviewerGroups) {
for _, rg := range rgs {
for _, name := range rg.Names {
normalizedName := name.Normalized()
nameReviewerMap[normalizedName] = append(nameReviewerMap[normalizedName], rg)
}
}
}

for _, owners := range fileToOwner {
addReviewerGroups(owners.requiredReviewers)
addReviewerGroups(owners.optionalReviewers)
}

return nameReviewerMap
}
Loading