From b4219ee22366a5b2ce8aff5c6201f7faeb60d71b Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 8 Jan 2026 14:09:46 -0800 Subject: [PATCH 1/9] WIP --- README.md | 42 +++ internal/app/app.go | 37 ++- internal/config/config.go | 19 +- internal/config/config_test.go | 24 ++ pkg/codeowners/merger.go | 162 ++++++++++ pkg/codeowners/merger_test.go | 527 +++++++++++++++++++++++++++++++++ 6 files changed, 797 insertions(+), 14 deletions(-) create mode 100644 pkg/codeowners/merger.go create mode 100644 pkg/codeowners/merger_test.go diff --git a/README.md b/README.md index 5174f42..672375a 100644 --- a/README.md +++ b/README.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 +# `sum_owners` (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. +sum_owners = 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,43 @@ 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. +#### Sum Owners (Ownership Handoffs) + +The `sum_owners` 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 `sum_owners` 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 +# `sum_owners` (default false) requires approval from BOTH base and PR branch codeowners +sum_owners = true +``` + +**Example Ownership Handoff:** + +Base branch `.codeowners`: +``` +*.py @backend-team +``` + +PR branch `.codeowners` (modified in the PR): +``` +*.py @data-team +``` + +Result with `sum_owners = 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 + +**Edge Cases:** + +- **New files** (only in PR branch): Only PR branch rules apply +- **Deleted files** (only in base branch): Only base branch rules apply (last owners must approve deletion) +- **Duplicate owners** (same owner in both branches): Only one approval needed (deduplication) +- **No rules in one branch**: Only uses rules from the other branch + +**Note:** The `sum_owners` 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/internal/app/app.go b/internal/app/app.go index f982c34..dc8a892 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,35 @@ 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.SumOwners { + // Sum owners mode: read .codeowners from BOTH base and head, then merge + a.printDebug("Sum owners 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 + reviewerGroupManager := codeowners.NewReviewerGroupMemo() + codeOwners = codeowners.MergeCodeOwners(baseCodeOwners, headCodeOwners, reviewerGroupManager) + 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..9874059 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -17,6 +17,7 @@ type Config struct { AdminBypass *AdminBypass `toml:"admin_bypass"` DetailedReviewers bool `toml:"detailed_reviewers"` DisableSmartDismissal bool `toml:"disable_smart_dismissal"` + SumOwners bool `toml:"sum_owners"` } 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, + SumOwners: false, } // Use filesystem reader if none provided diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 68a40e2..fb02831 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 sum_owners enabled", + configContent: ` +sum_owners = 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, + SumOwners: 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.SumOwners != tc.expected.SumOwners { + t.Errorf("SumOwners: expected %v, got %v", tc.expected.SumOwners, got.SumOwners) + } + 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..0c6e23b --- /dev/null +++ b/pkg/codeowners/merger.go @@ -0,0 +1,162 @@ +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. +// +// The reviewerGroupManager is used to deduplicate ReviewerGroup objects, ensuring +// that approval tracking works correctly. +func MergeCodeOwners(base CodeOwners, head CodeOwners, reviewerGroupManager ReviewerGroupManager) 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, reviewerGroupManager) + + // Merge optional reviewers (both are CC'd) + mergedOptional := mergeReviewerGroups(baseOpt, headOpt, reviewerGroupManager) + + 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. +// Uses reviewerGroupManager to deduplicate identical groups. +func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups, reviewerGroupManager ReviewerGroupManager) ReviewerGroups { + // Combine both groups + combined := make([]*ReviewerGroup, 0, len(base)+len(head)) + seen := make(map[string]bool) + for _, rg := range base { + key := createReviewerGroupKey(rg) + combined = append(combined, rg) + seen[key] = true + } + // add unseen reviewerGroups + for _, rg := range head { + key := createReviewerGroupKey(rg) + if !seen[key] { + combined = append(combined, rg) + } + } + 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]() + for _, file := range baseUnowned { + if !ownedSet.Contains(file) { + unownedSet.Add(file) + } + } + for _, file := range headUnowned { + if !ownedSet.Contains(file) { + unownedSet.Add(file) + } + } + + // 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) + + for _, owners := range fileToOwner { + // Process required reviewers + for _, rg := range owners.requiredReviewers { + for _, name := range rg.Names { + normalizedName := name.Normalized() + nameReviewerMap[normalizedName] = append(nameReviewerMap[normalizedName], rg) + } + } + + // Process optional reviewers + for _, rg := range owners.optionalReviewers { + for _, name := range rg.Names { + normalizedName := name.Normalized() + nameReviewerMap[normalizedName] = append(nameReviewerMap[normalizedName], rg) + } + } + } + + // Deduplicate ReviewerGroups in nameReviewerMap + for name, groups := range nameReviewerMap { + nameReviewerMap[name] = f.RemoveDuplicates(groups) + } + + return nameReviewerMap +} diff --git a/pkg/codeowners/merger_test.go b/pkg/codeowners/merger_test.go new file mode 100644 index 0000000..4a61646 --- /dev/null +++ b/pkg/codeowners/merger_test.go @@ -0,0 +1,527 @@ +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) + + // Create ReviewerGroupManager for deduplication + reviewerGroupManager := NewReviewerGroupMemo() + + // Merge + merged := MergeCodeOwners(base, head, reviewerGroupManager) + + // 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{}) + + reviewerGroupManager := NewReviewerGroupMemo() + merged := MergeCodeOwners(base, head, reviewerGroupManager) + + // 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{}) + + reviewerGroupManager := NewReviewerGroupMemo() + merged := MergeCodeOwners(base, head, reviewerGroupManager) + + // 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) + } + }) + } +} From 328ab85dbdef639eff8aa27c9d650fc01ffebf23 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 8 Jan 2026 14:30:58 -0800 Subject: [PATCH 2/9] Rename to more verbose but clear functionality --- README.md | 24 ++++++++++++------------ codeowners.toml | 3 +++ internal/app/app.go | 6 +++--- internal/config/config.go | 12 ++++++------ internal/config/config_test.go | 26 +++++++++++++------------- test_project/codeowners.toml | 2 ++ 6 files changed, 39 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 672375a..2b92e78 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,10 +232,10 @@ detailed_reviewers = true # `disable_smart_dismissal` (default false) means the codeowners will not dismiss stale reviews disable_smart_dismissal = true -# `sum_owners` (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. -sum_owners = 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 review +require_both_branch_reviewers = false # `enforcement` allows you to specify how the Codeowners Plus check should be enforced [enforcement] @@ -313,16 +313,16 @@ 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. -#### Sum Owners (Ownership Handoffs) +#### Require Both Branch Reviewers (Ownership Handoffs) -The `sum_owners` 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. +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 `sum_owners` 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. +**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 -# `sum_owners` (default false) requires approval from BOTH base and PR branch codeowners -sum_owners = true +# `require_both_branch_reviewers` (default false) requires approval from BOTH base and PR branch codeowners +require_both_branch_reviewers = true ``` **Example Ownership Handoff:** @@ -337,7 +337,7 @@ PR branch `.codeowners` (modified in the PR): *.py @data-team ``` -Result with `sum_owners = true`: +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 @@ -348,7 +348,7 @@ Result with `sum_owners = true`: - **Duplicate owners** (same owner in both branches): Only one approval needed (deduplication) - **No rules in one branch**: Only uses rules from the other branch -**Note:** The `sum_owners` setting is read from the base branch's `codeowners.toml` for security. PR authors cannot enable this feature for their own PRs. +**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 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 dc8a892..d2fdcb9 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -136,9 +136,9 @@ func (a *App) Run() (*OutputData, error) { // Initialize codeowners var codeOwners codeowners.CodeOwners - if conf.SumOwners { - // Sum owners mode: read .codeowners from BOTH base and head, then merge - a.printDebug("Sum owners mode enabled - reading .codeowners from both base and head refs\n") + 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) diff --git a/internal/config/config.go b/internal/config/config.go index 9874059..eb2edfd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,9 +15,9 @@ type Config struct { 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"` - SumOwners bool `toml:"sum_owners"` + DetailedReviewers bool `toml:"detailed_reviewers"` + DisableSmartDismissal bool `toml:"disable_smart_dismissal"` + RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"` } type Enforcement struct { @@ -43,9 +43,9 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) Enforcement: &Enforcement{Approval: false, FailCheck: true}, HighPriorityLabels: []string{}, AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}}, - DetailedReviewers: false, - DisableSmartDismissal: false, - SumOwners: false, + 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 fb02831..7f9cd9d 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -70,22 +70,22 @@ unskippable_reviewers = ["@user1"] expectedErr: false, }, { - name: "config with sum_owners enabled", + name: "config with require_both_branch_reviewers enabled", configContent: ` -sum_owners = true +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, - SumOwners: true, + 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, }, @@ -167,8 +167,8 @@ max_reviews = invalid t.Errorf("Ignore: expected %v, got %v", tc.expected.Ignore, got.Ignore) } - if got.SumOwners != tc.expected.SumOwners { - t.Errorf("SumOwners: expected %v, got %v", tc.expected.SumOwners, got.SumOwners) + if got.RequireBothBranchReviewers != tc.expected.RequireBothBranchReviewers { + t.Errorf("RequireBothBranchReviewers: expected %v, got %v", tc.expected.RequireBothBranchReviewers, got.RequireBothBranchReviewers) } if tc.expected.Enforcement != nil { 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 From f87a1821af7f202eaf3443ea2a3434846e94cbef Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 8 Jan 2026 14:34:03 -0800 Subject: [PATCH 3/9] Clean up --- internal/app/app.go | 3 +-- pkg/codeowners/merger.go | 12 ++++-------- pkg/codeowners/merger_test.go | 11 +++-------- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/internal/app/app.go b/internal/app/app.go index d2fdcb9..4ad050e 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -154,8 +154,7 @@ func (a *App) Run() (*OutputData, error) { } // Merge base and head codeowners using AND logic - reviewerGroupManager := codeowners.NewReviewerGroupMemo() - codeOwners = codeowners.MergeCodeOwners(baseCodeOwners, headCodeOwners, reviewerGroupManager) + 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 diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index 0c6e23b..400a078 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -10,10 +10,7 @@ import ( // 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. -// -// The reviewerGroupManager is used to deduplicate ReviewerGroup objects, ensuring -// that approval tracking works correctly. -func MergeCodeOwners(base CodeOwners, head CodeOwners, reviewerGroupManager ReviewerGroupManager) CodeOwners { +func MergeCodeOwners(base CodeOwners, head CodeOwners) CodeOwners { // Get file ownership maps from both branches baseRequired := base.FileRequired() headRequired := head.FileRequired() @@ -32,10 +29,10 @@ func MergeCodeOwners(base CodeOwners, head CodeOwners, reviewerGroupManager Revi headOpt := headOptional[file] // Merge required reviewers (AND logic - both must be satisfied) - mergedRequired := mergeReviewerGroups(baseReq, headReq, reviewerGroupManager) + mergedRequired := mergeReviewerGroups(baseReq, headReq) // Merge optional reviewers (both are CC'd) - mergedOptional := mergeReviewerGroups(baseOpt, headOpt, reviewerGroupManager) + mergedOptional := mergeReviewerGroups(baseOpt, headOpt) mergedFileToOwner[file] = fileOwners{ requiredReviewers: mergedRequired, @@ -77,8 +74,7 @@ func getAllFileNames(maps ...map[string]ReviewerGroups) []string { } // mergeReviewerGroups combines two ReviewerGroups using AND logic. -// Uses reviewerGroupManager to deduplicate identical groups. -func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups, reviewerGroupManager ReviewerGroupManager) ReviewerGroups { +func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroups { // Combine both groups combined := make([]*ReviewerGroup, 0, len(base)+len(head)) seen := make(map[string]bool) diff --git a/pkg/codeowners/merger_test.go b/pkg/codeowners/merger_test.go index 4a61646..131b213 100644 --- a/pkg/codeowners/merger_test.go +++ b/pkg/codeowners/merger_test.go @@ -208,11 +208,8 @@ func TestMergeCodeOwners(t *testing.T) { base := createMockCodeOwners(tc.baseRequired, tc.baseOptional, tc.baseUnowned) head := createMockCodeOwners(tc.headRequired, tc.headOptional, tc.headUnowned) - // Create ReviewerGroupManager for deduplication - reviewerGroupManager := NewReviewerGroupMemo() - // Merge - merged := MergeCodeOwners(base, head, reviewerGroupManager) + merged := MergeCodeOwners(base, head) // Verify required reviewers mergedRequired := merged.FileRequired() @@ -273,8 +270,7 @@ func TestMergeCodeOwnersApprovalTracking(t *testing.T) { base := createMockCodeOwners(baseRequired, map[string]ReviewerGroups{}, []string{}) head := createMockCodeOwners(headRequired, map[string]ReviewerGroups{}, []string{}) - reviewerGroupManager := NewReviewerGroupMemo() - merged := MergeCodeOwners(base, head, reviewerGroupManager) + merged := MergeCodeOwners(base, head) // Initially, both team-a and team-b should be required (unapproved) allRequired := merged.AllRequired() @@ -325,8 +321,7 @@ func TestMergeCodeOwnersSetAuthor(t *testing.T) { base := createMockCodeOwners(baseRequired, map[string]ReviewerGroups{}, []string{}) head := createMockCodeOwners(headRequired, map[string]ReviewerGroups{}, []string{}) - reviewerGroupManager := NewReviewerGroupMemo() - merged := MergeCodeOwners(base, head, reviewerGroupManager) + merged := MergeCodeOwners(base, head) // Set author merged.SetAuthor("@author") From 9cec58839a5a6289a3e552a8db732674477d54df Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 8 Jan 2026 14:37:28 -0800 Subject: [PATCH 4/9] gofmt --- internal/config/config.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index eb2edfd..046366b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,13 +8,13 @@ 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"` + 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"` @@ -36,13 +36,13 @@ 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{}}, + 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, From 340c9399a6e994d10bd986025312cc02970e9dfa Mon Sep 17 00:00:00 2001 From: BakerNet Date: Thu, 8 Jan 2026 16:57:31 -0800 Subject: [PATCH 5/9] Remove low value edge case section --- README.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/README.md b/README.md index 2b92e78..aea9373 100644 --- a/README.md +++ b/README.md @@ -341,13 +341,6 @@ 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 -**Edge Cases:** - -- **New files** (only in PR branch): Only PR branch rules apply -- **Deleted files** (only in base branch): Only base branch rules apply (last owners must approve deletion) -- **Duplicate owners** (same owner in both branches): Only one approval needed (deduplication) -- **No rules in one branch**: Only uses rules from the other branch - **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 From 099f2eb94ff5e4e3e7a111fe79a0c13d518e7367 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Mon, 12 Jan 2026 15:02:49 -0800 Subject: [PATCH 6/9] review comment --- pkg/codeowners/merger.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index 400a078..07a7d53 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -78,18 +78,25 @@ func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroup // Combine both groups combined := make([]*ReviewerGroup, 0, len(base)+len(head)) seen := make(map[string]bool) - for _, rg := range base { + + // add reviewer group with deduplication + addGroup := func(rg *ReviewerGroup, checkSeen bool) { key := createReviewerGroupKey(rg) + if checkSeen && seen[key] { + return + } combined = append(combined, rg) seen[key] = true } - // add unseen reviewerGroups + + for _, rg := range base { + addGroup(rg, false) + } + for _, rg := range head { - key := createReviewerGroupKey(rg) - if !seen[key] { - combined = append(combined, rg) - } + addGroup(rg, true) } + return combined } From ff3c885b1f5a9c1b84154a6cbee79fbe717445c8 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Mon, 12 Jan 2026 15:04:31 -0800 Subject: [PATCH 7/9] covbadge --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index aea9373..b17c4b8 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.3%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) From 70fd66a63dbe21b863de62a96196aebf0a40ed95 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Mon, 12 Jan 2026 15:06:25 -0800 Subject: [PATCH 8/9] Remove unnecessary conditional --- pkg/codeowners/merger.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index 07a7d53..0bb214e 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -80,9 +80,9 @@ func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroup seen := make(map[string]bool) // add reviewer group with deduplication - addGroup := func(rg *ReviewerGroup, checkSeen bool) { + addGroup := func(rg *ReviewerGroup) { key := createReviewerGroupKey(rg) - if checkSeen && seen[key] { + if seen[key] { return } combined = append(combined, rg) @@ -90,11 +90,11 @@ func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroup } for _, rg := range base { - addGroup(rg, false) + addGroup(rg) } for _, rg := range head { - addGroup(rg, true) + addGroup(rg) } return combined From ba51bde5b94ba8e9a0c715c486c72ed7050ee023 Mon Sep 17 00:00:00 2001 From: BakerNet Date: Mon, 12 Jan 2026 15:24:11 -0800 Subject: [PATCH 9/9] More DRY --- README.md | 2 +- pkg/codeowners/merger.go | 57 ++++++++++++++++------------------------ 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index b17c4b8..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-83.3%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) diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index 0bb214e..a63542c 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -80,22 +80,19 @@ func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroup seen := make(map[string]bool) // add reviewer group with deduplication - addGroup := func(rg *ReviewerGroup) { - key := createReviewerGroupKey(rg) - if seen[key] { - return + addGroups := func(rgs ReviewerGroups) { + for _, rg := range rgs { + key := createReviewerGroupKey(rg) + if seen[key] { + continue + } + combined = append(combined, rg) + seen[key] = true } - combined = append(combined, rg) - seen[key] = true - } - - for _, rg := range base { - addGroup(rg) } - for _, rg := range head { - addGroup(rg) - } + addGroups(base) + addGroups(head) return combined } @@ -117,16 +114,15 @@ func mergeUnownedFiles(baseUnowned []string, headUnowned []string, filesWithOwne // Combine unowned files from both branches unownedSet := f.NewSet[string]() - for _, file := range baseUnowned { - if !ownedSet.Contains(file) { - unownedSet.Add(file) - } - } - for _, file := range headUnowned { - if !ownedSet.Contains(file) { - unownedSet.Add(file) + 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() @@ -138,17 +134,8 @@ func mergeUnownedFiles(baseUnowned []string, headUnowned []string, filesWithOwne func buildNameReviewerMap(fileToOwner map[string]fileOwners) map[string]ReviewerGroups { nameReviewerMap := make(map[string]ReviewerGroups) - for _, owners := range fileToOwner { - // Process required reviewers - for _, rg := range owners.requiredReviewers { - for _, name := range rg.Names { - normalizedName := name.Normalized() - nameReviewerMap[normalizedName] = append(nameReviewerMap[normalizedName], rg) - } - } - - // Process optional reviewers - for _, rg := range owners.optionalReviewers { + addReviewerGroups := func(rgs ReviewerGroups) { + for _, rg := range rgs { for _, name := range rg.Names { normalizedName := name.Normalized() nameReviewerMap[normalizedName] = append(nameReviewerMap[normalizedName], rg) @@ -156,9 +143,9 @@ func buildNameReviewerMap(fileToOwner map[string]fileOwners) map[string]Reviewer } } - // Deduplicate ReviewerGroups in nameReviewerMap - for name, groups := range nameReviewerMap { - nameReviewerMap[name] = f.RemoveDuplicates(groups) + for _, owners := range fileToOwner { + addReviewerGroups(owners.requiredReviewers) + addReviewerGroups(owners.optionalReviewers) } return nameReviewerMap