diff --git a/README.md b/README.md index 028f156..6e2d0f2 100644 --- a/README.md +++ b/README.md @@ -240,10 +240,16 @@ 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 + # `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..afdddb0 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,7 +63,7 @@ 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 { @@ -70,6 +73,16 @@ func (om *ownersMap) SetAuthor(author string, mode AuthorMode) { 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. + for _, team := range authorTeams { + for _, reviewers := range om.nameReviewerMap[team.Normalized()] { + reviewers.Approved = true + } + } + } om.author = author } diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index a25c3f6..27b5b83 100644 --- a/pkg/codeowners/codeowners_test.go +++ b/pkg/codeowners/codeowners_test.go @@ -333,6 +333,64 @@ 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)) + } +} + // 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/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{