diff --git a/README.md b/README.md index 5174f42..aea9373 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-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) @@ -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 @@ -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: diff --git a/codeowners.toml b/codeowners.toml index c495377..7ffca62 100644 --- a/codeowners.toml +++ b/codeowners.toml @@ -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] diff --git a/internal/app/app.go b/internal/app/app.go index f982c34..4ad050e 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -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") } @@ -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 diff --git a/internal/config/config.go b/internal/config/config.go index 0514246..046366b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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 { @@ -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 diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 68a40e2..7f9cd9d 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -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: ` @@ -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") diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go new file mode 100644 index 0000000..a63542c --- /dev/null +++ b/pkg/codeowners/merger.go @@ -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 +} diff --git a/pkg/codeowners/merger_test.go b/pkg/codeowners/merger_test.go new file mode 100644 index 0000000..131b213 --- /dev/null +++ b/pkg/codeowners/merger_test.go @@ -0,0 +1,522 @@ +package codeowners + +import ( + "strings" + "testing" +) + +func TestMergeCodeOwners(t *testing.T) { + tt := []struct { + name string + baseRequired map[string]ReviewerGroups + headRequired map[string]ReviewerGroups + baseOptional map[string]ReviewerGroups + headOptional map[string]ReviewerGroups + baseUnowned []string + headUnowned []string + expectedRequired map[string][]string // file -> list of reviewer group strings + expectedOptional map[string][]string // file -> list of reviewer group strings + expectedUnowned []string + }{ + { + name: "basic merge with different owners", + baseRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + }, + }, + headRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-b"}), Approved: false}, + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "file.py": {"@team-a", "@team-b"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + { + name: "file only in base (deleted file)", + baseRequired: map[string]ReviewerGroups{ + "deleted.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + }, + }, + headRequired: map[string]ReviewerGroups{}, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "deleted.py": {"@team-a"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + { + name: "file only in head (new file)", + baseRequired: map[string]ReviewerGroups{}, + headRequired: map[string]ReviewerGroups{ + "new.py": { + {Names: NewSlugs([]string{"@team-b"}), Approved: false}, + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "new.py": {"@team-b"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + { + name: "duplicate reviewers across branches", + baseRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + }, + }, + headRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "file.py": {"@team-a"}, // Only one @team-a due to deduplication + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + { + name: "multiple reviewers in each branch", + baseRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + {Names: NewSlugs([]string{"@security"}), Approved: false}, + }, + }, + headRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-b"}), Approved: false}, + {Names: NewSlugs([]string{"@compliance"}), Approved: false}, + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "file.py": {"@team-a", "@security", "@team-b", "@compliance"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + { + name: "optional reviewers merged", + baseRequired: map[string]ReviewerGroups{}, + headRequired: map[string]ReviewerGroups{}, + baseOptional: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@junior-dev"}), Approved: false}, + }, + }, + headOptional: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@intern"}), Approved: false}, + }, + }, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{}, + expectedOptional: map[string][]string{ + "file.py": {"@junior-dev", "@intern"}, + }, + expectedUnowned: []string{}, + }, + { + name: "unowned files merged", + baseRequired: map[string]ReviewerGroups{}, + headRequired: map[string]ReviewerGroups{}, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{"unowned1.txt"}, + headUnowned: []string{"unowned2.txt"}, + expectedRequired: map[string][]string{}, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{"unowned1.txt", "unowned2.txt"}, + }, + { + name: "unowned file in one branch, owned in another", + baseRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-a"}), Approved: false}, + }, + }, + headRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@team-b"}), Approved: false}, + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{"file.py"}, // Base says unowned, but we have explicit rules + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "file.py": {"@team-a", "@team-b"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, // Should not be in unowned since it has owners + }, + { + name: "OR groups within each branch", + baseRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@user1", "@user2"}), Approved: false}, // OR group + }, + }, + headRequired: map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@user3", "@user4"}), Approved: false}, // OR group + }, + }, + baseOptional: map[string]ReviewerGroups{}, + headOptional: map[string]ReviewerGroups{}, + baseUnowned: []string{}, + headUnowned: []string{}, + expectedRequired: map[string][]string{ + "file.py": {"@user1 or @user2", "@user3 or @user4"}, + }, + expectedOptional: map[string][]string{}, + expectedUnowned: []string{}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + // Create mock CodeOwners objects + base := createMockCodeOwners(tc.baseRequired, tc.baseOptional, tc.baseUnowned) + head := createMockCodeOwners(tc.headRequired, tc.headOptional, tc.headUnowned) + + // Merge + merged := MergeCodeOwners(base, head) + + // Verify required reviewers + mergedRequired := merged.FileRequired() + if len(tc.expectedRequired) == 0 && len(mergedRequired) > 0 { + t.Errorf("expected no required reviewers, got %v", mergedRequired) + } + for file, expectedGroups := range tc.expectedRequired { + actualGroups, found := mergedRequired[file] + if !found { + t.Errorf("file %s not found in merged required reviewers", file) + continue + } + if !reviewerGroupsMatch(actualGroups, expectedGroups) { + t.Errorf("file %s: expected reviewers %v, got %v", + file, expectedGroups, reviewerGroupsToStrings(actualGroups)) + } + } + + // Verify optional reviewers + mergedOptional := merged.FileOptional() + if len(tc.expectedOptional) == 0 && len(mergedOptional) > 0 { + t.Errorf("expected no optional reviewers, got %v", mergedOptional) + } + for file, expectedGroups := range tc.expectedOptional { + actualGroups, found := mergedOptional[file] + if !found { + t.Errorf("file %s not found in merged optional reviewers", file) + continue + } + if !reviewerGroupsMatch(actualGroups, expectedGroups) { + t.Errorf("file %s: expected optional reviewers %v, got %v", + file, expectedGroups, reviewerGroupsToStrings(actualGroups)) + } + } + + // Verify unowned files + mergedUnowned := merged.UnownedFiles() + if !stringSlicesEqual(mergedUnowned, tc.expectedUnowned) { + t.Errorf("unowned files: expected %v, got %v", tc.expectedUnowned, mergedUnowned) + } + }) + } +} + +func TestMergeCodeOwnersApprovalTracking(t *testing.T) { + // Test that approval tracking works correctly after merging + baseRequired := map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@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) + + // Initially, both team-a and team-b should be required (unapproved) + allRequired := merged.AllRequired() + if len(allRequired) != 2 { + t.Fatalf("expected 2 required reviewer groups initially, got %d", len(allRequired)) + } + + // Apply approval from team-a + merged.ApplyApprovals(NewSlugs([]string{"@team-a"})) + + // After team-a approves, only team-b should be in AllRequired (since AllRequired filters approved) + allRequired = merged.AllRequired() + if len(allRequired) != 1 { + t.Logf("AllRequired contents after team-a approval:") + for i, rg := range allRequired { + t.Logf(" %d: %v (approved: %v)", i, rg.ToCommentString(), rg.Approved) + } + t.Fatalf("expected 1 required reviewer group after team-a approval, got %d", len(allRequired)) + } + + if allRequired[0].ToCommentString() != "@team-b" { + t.Errorf("expected @team-b to be the remaining required reviewer, got %s", allRequired[0].ToCommentString()) + } + + // Apply approval from team-b + merged.ApplyApprovals(NewSlugs([]string{"@team-b"})) + + // After both approve, AllRequired should return empty (all approved) + allRequired = merged.AllRequired() + if len(allRequired) != 0 { + t.Errorf("expected 0 required reviewer groups after both approvals, got %d", len(allRequired)) + } +} + +func TestMergeCodeOwnersSetAuthor(t *testing.T) { + // Test that SetAuthor works correctly after merging + baseRequired := map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@author", "@team-a"}), Approved: false}, + }, + } + headRequired := map[string]ReviewerGroups{ + "file.py": { + {Names: NewSlugs([]string{"@author", "@team-b"}), Approved: false}, + }, + } + + base := createMockCodeOwners(baseRequired, map[string]ReviewerGroups{}, []string{}) + head := createMockCodeOwners(headRequired, map[string]ReviewerGroups{}, []string{}) + + merged := MergeCodeOwners(base, head) + + // Set author + merged.SetAuthor("@author") + + // Check that author is removed from reviewer groups or groups are auto-approved + allRequired := merged.AllRequired() + for _, rg := range allRequired { + for _, name := range rg.Names { + if name.Original() == "@author" { + t.Errorf("author should be removed from reviewer groups, but found in %v", rg.Names) + } + } + } +} + +// Helper functions + +// createMockCodeOwners creates a mock CodeOwners object for testing +func createMockCodeOwners(required, optional map[string]ReviewerGroups, unowned []string) CodeOwners { + fileToOwner := make(map[string]fileOwners) + for file, reviewers := range required { + fileToOwner[file] = fileOwners{ + requiredReviewers: reviewers, + optionalReviewers: optional[file], + } + } + for file, reviewers := range optional { + if _, exists := fileToOwner[file]; !exists { + fileToOwner[file] = fileOwners{ + requiredReviewers: nil, + optionalReviewers: reviewers, + } + } + } + + nameReviewerMap := buildNameReviewerMap(fileToOwner) + + return &ownersMap{ + author: "", + fileToOwner: fileToOwner, + nameReviewerMap: nameReviewerMap, + unownedFiles: unowned, + } +} + +// reviewerGroupsToStrings converts ReviewerGroups to strings for comparison +func reviewerGroupsToStrings(groups ReviewerGroups) []string { + result := make([]string, len(groups)) + for i, rg := range groups { + result[i] = rg.ToCommentString() + } + return result +} + +// reviewerGroupsMatch checks if actual ReviewerGroups match expected string representations +func reviewerGroupsMatch(actual ReviewerGroups, expected []string) bool { + if len(actual) != len(expected) { + return false + } + + actualStrings := reviewerGroupsToStrings(actual) + + // Sort both for comparison + actualSet := make(map[string]bool) + for _, s := range actualStrings { + actualSet[s] = true + } + + for _, exp := range expected { + if !actualSet[exp] { + return false + } + } + + return true +} + +// stringSlicesEqual checks if two string slices are equal (order-independent) +func stringSlicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + + aMap := make(map[string]bool) + for _, s := range a { + aMap[s] = true + } + + for _, s := range b { + if !aMap[s] { + return false + } + } + + return true +} + +func TestGetAllFileNames(t *testing.T) { + map1 := map[string]ReviewerGroups{ + "file1.py": {}, + "file2.py": {}, + } + map2 := map[string]ReviewerGroups{ + "file2.py": {}, // duplicate + "file3.py": {}, + } + map3 := map[string]ReviewerGroups{ + "file4.py": {}, + } + + result := getAllFileNames(map1, map2, map3) + + expected := []string{"file1.py", "file2.py", "file3.py", "file4.py"} + if !stringSlicesEqual(result, expected) { + t.Errorf("expected %v, got %v", expected, result) + } + + // Verify sorted + expectedSorted := strings.Join(expected, ",") + actualSorted := strings.Join(result, ",") + if expectedSorted != actualSorted { + t.Errorf("result is not sorted: expected %s, got %s", expectedSorted, actualSorted) + } +} + +func TestCreateReviewerGroupKey(t *testing.T) { + tt := []struct { + name string + rg *ReviewerGroup + expected string + }{ + { + name: "single reviewer", + rg: &ReviewerGroup{Names: NewSlugs([]string{"@team-a"})}, + expected: "@team-a", + }, + { + name: "multiple reviewers sorted", + rg: &ReviewerGroup{Names: NewSlugs([]string{"@team-b", "@team-a"})}, + expected: "@team-a,@team-b", // Should be sorted + }, + { + name: "case insensitive normalization", + rg: &ReviewerGroup{Names: NewSlugs([]string{"@Team-A"})}, + expected: "@team-a", + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := createReviewerGroupKey(tc.rg) + if result != tc.expected { + t.Errorf("expected %s, got %s", tc.expected, result) + } + }) + } +} + +func TestMergeUnownedFiles(t *testing.T) { + tt := []struct { + name string + baseUnowned []string + headUnowned []string + filesWithOwners []string + expectedUnowned []string + }{ + { + name: "no overlap", + baseUnowned: []string{"unowned1.txt"}, + headUnowned: []string{"unowned2.txt"}, + filesWithOwners: []string{}, + expectedUnowned: []string{"unowned1.txt", "unowned2.txt"}, + }, + { + name: "files with owners excluded", + baseUnowned: []string{"unowned1.txt", "owned.txt"}, + headUnowned: []string{"unowned2.txt"}, + filesWithOwners: []string{"owned.txt"}, + expectedUnowned: []string{"unowned1.txt", "unowned2.txt"}, + }, + { + name: "duplicate unowned", + baseUnowned: []string{"unowned.txt"}, + headUnowned: []string{"unowned.txt"}, + filesWithOwners: []string{}, + expectedUnowned: []string{"unowned.txt"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := mergeUnownedFiles(tc.baseUnowned, tc.headUnowned, tc.filesWithOwners) + if !stringSlicesEqual(result, tc.expectedUnowned) { + t.Errorf("expected %v, got %v", tc.expectedUnowned, result) + } + }) + } +} diff --git a/test_project/codeowners.toml b/test_project/codeowners.toml index d2cc458..834a843 100644 --- a/test_project/codeowners.toml +++ b/test_project/codeowners.toml @@ -5,6 +5,8 @@ unskippable_reviewers = ["@user1", "@user2"] ignore = ["ignored"] +require_both_branch_reviewers = false + [enforcement] approval = false fail_check = true