diff --git a/README.md b/README.md index 028f156..9ffa464 100644 --- a/README.md +++ b/README.md @@ -240,10 +240,19 @@ require_both_branch_reviewers = false # Useful when you intentionally have files without codeowners suppress_unowned_warning = true -# `allow_self_approval` (default false) treats the PR author as satisfying any OR ownership group they belong to -# If the PR author is included an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve +# `allow_self_approval` (default false) treats the PR author as satisfying any OR ownership group they are listed in +# If the PR author is included in an OR group (e.g. `*.py @alice @bob`), the group is auto-satisfied without requiring another member to approve +# Note: this matches the author against individually listed logins only; see `self_approval_via_teams` for team membership allow_self_approval = false +# `self_approval_via_teams` (default false) extends `allow_self_approval` to team membership: +# OR groups containing a team the PR author is a member of (e.g. `*.py @org/backend`) are also auto-satisfied +# Requires a token that can read org team members (same requirement as GitHub Teams support) +self_approval_via_teams = false + +# Note: self-approval (both by login and via teams) never satisfies additional-reviewer ("&") rules — +# those represent mandatory extra reviewers and always require an explicit approval from another member + # `disable_review_status_comments` (default false) suppresses review status comments (required/unapproved reviewers). # Optional reviewers are still invited with a CC comment. disable_review_status_comments = false diff --git a/internal/app/app.go b/internal/app/app.go index 642019d..90eac3c 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -168,10 +168,20 @@ func (a *App) Run() (*OutputData, error) { // Set author author := fmt.Sprintf("@%s", a.client.PR().User.GetLogin()) authorMode := codeowners.AuthorModeDefault + var authorTeams []codeowners.Slug if conf.AllowSelfApproval { authorMode = codeowners.AuthorModeSelfApproval + if conf.SelfApprovalViaTeams { + // Resolve the author's team memberships so ownership groups + // containing those teams are satisfied by the author, the same + // way approvals from team members satisfy them. + if err := a.client.InitUserReviewerMap(codeowners.OriginalStrings(codeOwners.AllRequired().Flatten())); err != nil { + return &OutputData{}, fmt.Errorf("InitUserReviewerMap Error: %v", err) + } + authorTeams = a.client.UserReviewers(author) + } } - codeOwners.SetAuthor(author, authorMode) + codeOwners.SetAuthor(author, authorMode, authorTeams...) // Warn about unowned files if !conf.SuppressUnownedWarning { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 0399572..c457d47 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -77,7 +77,7 @@ func (m *mockCodeOwners) ApplyApprovals(approvers []codeowners.Slug) { m.appliedApprovals = approvers } -func (m *mockCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode) { +func (m *mockCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode, authorTeams ...codeowners.Slug) { m.author = author for _, reviewers := range m.requiredOwners { for i, name := range reviewers.Names { @@ -110,6 +110,7 @@ func (m *mockCodeOwners) UnownedFiles() []string { type mockGitHubClient struct { pr *github.PullRequest userReviewerMapError error + userReviewers []codeowners.Slug currentApprovals []*gh.CurrentApproval currentApprovalsError error tokenUser string @@ -145,6 +146,10 @@ func (m *mockGitHubClient) InitUserReviewerMap(owners []string) error { return m.userReviewerMapError } +func (m *mockGitHubClient) UserReviewers(user string) []codeowners.Slug { + return m.userReviewers +} + func (m *mockGitHubClient) GetCurrentReviewerApprovals() ([]*gh.CurrentApproval, error) { return m.currentApprovals, m.currentApprovalsError } diff --git a/internal/config/config.go b/internal/config/config.go index fd1b6e8..c3bcf7e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,6 +20,7 @@ type Config struct { RequireBothBranchReviewers bool `toml:"require_both_branch_reviewers"` SuppressUnownedWarning bool `toml:"suppress_unowned_warning"` AllowSelfApproval bool `toml:"allow_self_approval"` + SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` DisableReviewStatusComments bool `toml:"disable_review_status_comments"` } @@ -47,6 +48,7 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) HighPriorityLabels: []string{}, AdminBypass: &AdminBypass{Enabled: false, AllowedUsers: []string{}}, DetailedReviewers: false, + SelfApprovalViaTeams: false, DisableSmartDismissal: false, RequireBothBranchReviewers: false, DisableReviewStatusComments: false, diff --git a/internal/github/gh.go b/internal/github/gh.go index 112a112..96453ba 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -33,6 +33,7 @@ type Client interface { InitPR(pr_id int) error PR() *github.PullRequest InitUserReviewerMap(reviewers []string) error + UserReviewers(user string) []codeowners.Slug GetTokenUser() (string, error) InitReviews() error AllApprovals() ([]*CurrentApproval, error) @@ -131,6 +132,13 @@ func (gh *GHClient) InitUserReviewerMap(reviewers []string) error { return nil } +// UserReviewers returns the owner slugs (their own login and the teams they +// are a member of) the given user satisfies, based on the map built by +// InitUserReviewerMap. Returns nil if the map has not been initialized. +func (gh *GHClient) UserReviewers(user string) []codeowners.Slug { + return gh.userReviewerMap[strings.ToLower(strings.TrimPrefix(user, "@"))] +} + func (gh *GHClient) GetTokenUser() (string, error) { user, _, err := gh.client.Users.Get(gh.ctx, "") if err != nil { diff --git a/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index 95ec9a8..608eb7b 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -18,7 +18,10 @@ const ( // CodeOwners represents a collection of owned files, with reverse lookups for owners and reviewers type CodeOwners interface { - SetAuthor(author string, mode AuthorMode) + // SetAuthor marks the PR author in the ownership map. In self-approval + // mode, OR groups containing the author (or, when provided, any of the + // author's teams) are treated as satisfied. + SetAuthor(author string, mode AuthorMode, authorTeams ...Slug) // FileRequired returns a map of file names to their required reviewers FileRequired() map[string]ReviewerGroups @@ -60,16 +63,32 @@ type ownersMap struct { unownedFiles []string } -func (om *ownersMap) SetAuthor(author string, mode AuthorMode) { +func (om *ownersMap) SetAuthor(author string, mode AuthorMode, authorTeams ...Slug) { authorSlug := NewSlug(author) for _, reviewers := range om.nameReviewerMap[authorSlug.Normalized()] { reviewers.Names = slices.DeleteFunc(reviewers.Names, func(name Slug) bool { return name.Equals(authorSlug) }) - if len(reviewers.Names) == 0 || mode == AuthorModeSelfApproval { + // Additional ("&") groups represent mandatory extra reviewers and are + // never satisfied by authorship — only by emptiness (the author was + // the sole listed reviewer, so requiring them would deadlock the PR). + if len(reviewers.Names) == 0 || (mode == AuthorModeSelfApproval && !reviewers.Additional) { reviewers.Approved = true } } + if mode == AuthorModeSelfApproval { + // The author also vouches for groups they belong to via team + // membership. Teams are not removed from the group so that other + // team members remain valid reviewers. Additional ("&") groups are + // excluded here as well. + for _, team := range authorTeams { + for _, reviewers := range om.nameReviewerMap[team.Normalized()] { + if !reviewers.Additional { + reviewers.Approved = true + } + } + } + } om.author = author } diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index a25c3f6..b7cafcf 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -39,8 +39,8 @@ func TestInitOwnerTree(t *testing.T) { } expectedAdditionalTests := FileTestCases{ - &reviewerTest{Match: "models*", Reviewer: rgMan.ToReviewerGroup("@devops")}, - &reviewerTest{Match: "**/*test*", Reviewer: rgMan.ToReviewerGroup("@core")}, + &reviewerTest{Match: "models*", Reviewer: rgMan.ToAdditionalReviewerGroup("@devops")}, + &reviewerTest{Match: "**/*test*", Reviewer: rgMan.ToAdditionalReviewerGroup("@core")}, } for i, test := range tree.additionalReviewerTests { @@ -333,6 +333,125 @@ func TestSetAuthorSelfApprovalMultipleORGroups(t *testing.T) { } } +// When self-approval is enabled and the author's team memberships are provided, +// an OR group containing one of those teams (e.g. `*.py @org/backend @bob`) is +// auto-satisfied: the author vouches for their own code as a member of the team. +func TestSetAuthorSelfApprovalViaTeamMembership(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@org/backend", "@bob"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval, NewSlug("@org/backend")) + + allRequired := co.AllRequired() + if len(allRequired) != 0 { + t.Errorf("Expected OR group containing the author's team to be auto-satisfied, got %d still required", len(allRequired)) + } +} + +// Team memberships must not satisfy groups in default mode — other members of +// the team remain valid (and required) reviewers, and the team itself stays in +// the group. +func TestSetAuthorTeamsIgnoredInDefaultMode(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@org/backend", "@bob"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeDefault, NewSlug("@org/backend")) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected 1 still-required group in default mode, got %d", len(allRequired)) + } + if len(allRequired[0].Names) != 2 { + t.Errorf("Expected the team to remain in the group, got %v", OriginalStrings(allRequired[0].Names)) + } +} + +// Team memberships only satisfy groups that actually contain one of the teams. +func TestSetAuthorTeamsDoNotAffectUnrelatedGroups(t *testing.T) { + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {{Names: NewSlugs([]string{"@bob", "@charlie"}), Approved: false}}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval, NewSlug("@org/backend")) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Errorf("Expected group without the author or their teams to remain required, got %d", len(allRequired)) + } +} + +// Additional ("&") groups are mandatory extra reviewers: even when the author +// is listed in one directly, self-approval must not satisfy it — only another +// listed reviewer can. +func TestSetAuthorSelfApprovalDoesNotSatisfyAdditionalGroupWithAuthor(t *testing.T) { + andGroup := &ReviewerGroup{Names: NewSlugs([]string{"@alice", "@bob"}), Approved: false, Additional: true} + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {andGroup}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Fatalf("Expected additional group to remain required despite self-approval, got %d", len(allRequired)) + } + if len(allRequired[0].Names) != 1 || allRequired[0].Names[0].Normalized() != "@bob" { + t.Errorf("Expected remaining required reviewer to be @bob, got %v", OriginalStrings(allRequired[0].Names)) + } +} + +// Same for team membership: an additional group containing the author's team +// must not be satisfied by authorship. +func TestSetAuthorTeamsDoNotSatisfyAdditionalGroups(t *testing.T) { + andGroup := &ReviewerGroup{Names: NewSlugs([]string{"@org/security"}), Approved: false, Additional: true} + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {andGroup}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval, NewSlug("@org/security")) + + allRequired := co.AllRequired() + if len(allRequired) != 1 { + t.Errorf("Expected additional group containing the author's team to remain required, got %d", len(allRequired)) + } +} + +// Deadlock prevention is unchanged: if the author was the only listed +// additional reviewer, the group becomes empty and is satisfied. +func TestSetAuthorEmptyAdditionalGroupStillSatisfied(t *testing.T) { + andGroup := &ReviewerGroup{Names: NewSlugs([]string{"@alice"}), Approved: false, Additional: true} + co := createMockCodeOwners( + map[string]ReviewerGroups{ + "file.py": {andGroup}, + }, + map[string]ReviewerGroups{}, + []string{}, + ) + co.SetAuthor("@alice", AuthorModeSelfApproval) + + allRequired := co.AllRequired() + if len(allRequired) != 0 { + t.Errorf("Expected emptied additional group to be satisfied (deadlock prevention), got %d still required", len(allRequired)) + } +} + // Self-approval only applies to OR groups the author belongs to. If a file also has a separate // AND group (e.g. `&*.py @security`), that group must still be independently satisfied — // the author being in one OR group doesn't grant them approval power over unrelated AND groups. diff --git a/pkg/codeowners/merger.go b/pkg/codeowners/merger.go index a63542c..8b48e34 100644 --- a/pkg/codeowners/merger.go +++ b/pkg/codeowners/merger.go @@ -101,7 +101,11 @@ func mergeReviewerGroups(base ReviewerGroups, head ReviewerGroups) ReviewerGroup func createReviewerGroupKey(rg *ReviewerGroup) string { normalizedNames := f.Map(rg.Names, func(s Slug) string { return s.Normalized() }) slices.Sort(normalizedNames) - return strings.Join(normalizedNames, ",") + key := strings.Join(normalizedNames, ",") + if rg.Additional { + key = "&|" + key + } + return key } // mergeUnownedFiles combines unowned files from both branches, excluding files that have owners diff --git a/pkg/codeowners/reader.go b/pkg/codeowners/reader.go index 2e94319..3c2746a 100644 --- a/pkg/codeowners/reader.go +++ b/pkg/codeowners/reader.go @@ -123,7 +123,11 @@ func Read(path string, reviewerGroupManager ReviewerGroupManager, fileReader Fil match = "**/*" } } - test := &reviewerTest{Match: match, Reviewer: reviewerGroupManager.ToReviewerGroup(owner...)} + reviewer := reviewerGroupManager.ToReviewerGroup(owner...) + if additional { + reviewer = reviewerGroupManager.ToAdditionalReviewerGroup(owner...) + } + test := &reviewerTest{Match: match, Reviewer: reviewer} if additional { rules.AdditionalReviewerTests = append(rules.AdditionalReviewerTests, test) } else if optional { diff --git a/pkg/codeowners/reviewers.go b/pkg/codeowners/reviewers.go index 9ba8804..ddfb123 100644 --- a/pkg/codeowners/reviewers.go +++ b/pkg/codeowners/reviewers.go @@ -12,6 +12,9 @@ import ( type ReviewerGroupManager interface { ToReviewerGroup(names ...string) *ReviewerGroup + // ToAdditionalReviewerGroup creates a group for an additional-reviewer + // ("&") rule. Additional groups are never satisfied by authorship. + ToAdditionalReviewerGroup(names ...string) *ReviewerGroup } func NewReviewerGroupMemo() ReviewerGroupManager { @@ -22,19 +25,35 @@ type ReviewerGroupMemo map[string]*ReviewerGroup // Create a new Reviewers, memoizing the Reviewers so it is only created once func (rgm ReviewerGroupMemo) ToReviewerGroup(names ...string) *ReviewerGroup { + return rgm.toGroup(false, names) +} + +// Create a new additional-reviewer ("&") group, memoized separately from +// owner groups so the Additional flag never leaks between the two roles. +func (rgm ReviewerGroupMemo) ToAdditionalReviewerGroup(names ...string) *ReviewerGroup { + return rgm.toGroup(true, names) +} + +func (rgm ReviewerGroupMemo) toGroup(additional bool, names []string) *ReviewerGroup { key := strings.Join(names, ",") + if additional { + key = "&|" + key + } if item, found := rgm[key]; found { return item } - newReviewers := &ReviewerGroup{NewSlugs(names), false} + newReviewers := &ReviewerGroup{Names: NewSlugs(names), Approved: false, Additional: additional} rgm[key] = newReviewers return newReviewers } -// Represents a group of ReviewerGroup, with a list of names and an approved status +// Represents a group of ReviewerGroup, with a list of names and an approved status. +// Additional marks groups created from "&" rules: they represent mandatory +// extra reviewers and are never satisfied by PR authorship (self-approval). type ReviewerGroup struct { - Names []Slug - Approved bool + Names []Slug + Approved bool + Additional bool } type ReviewerGroups []*ReviewerGroup diff --git a/tools/cli/main_test.go b/tools/cli/main_test.go index fdd7766..4332aea 100644 --- a/tools/cli/main_test.go +++ b/tools/cli/main_test.go @@ -416,11 +416,12 @@ func (f *fakeCodeOwners) FileRequired() map[string]codeowners.ReviewerGroups { func (f *fakeCodeOwners) FileOptional() map[string]codeowners.ReviewerGroups { return f.optional } -func (f *fakeCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode) {} -func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } -func (f *fakeCodeOwners) UnownedFiles() []string { return nil } -func (f *fakeCodeOwners) ApplyApprovals(approvers []codeowners.Slug) {} +func (f *fakeCodeOwners) SetAuthor(author string, mode codeowners.AuthorMode, authorTeams ...codeowners.Slug) { +} +func (f *fakeCodeOwners) AllRequired() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) AllOptional() codeowners.ReviewerGroups { return nil } +func (f *fakeCodeOwners) UnownedFiles() []string { return nil } +func (f *fakeCodeOwners) ApplyApprovals(approvers []codeowners.Slug) {} func TestJsonTargets(t *testing.T) { owners := &fakeCodeOwners{