From de2a200cd32d152eeece6a433e8b3aa66539afdf Mon Sep 17 00:00:00 2001 From: Daniil Pushin Date: Mon, 6 Jul 2026 17:48:13 +0200 Subject: [PATCH 1/2] feat: extend self-approval to team membership (self_approval_via_teams) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit allow_self_approval currently matches the PR author against individually listed logins only — an OR group like `* @org/backend` is never satisfied by a backend member authoring the PR, even though approvals from team members do satisfy it (issue #49). Add an opt-in `self_approval_via_teams` config option: when enabled (together with allow_self_approval), the author's team memberships are resolved through the existing user-reviewer map and OR groups containing any of those teams are treated as satisfied. Teams are not removed from the groups, so other members remain valid reviewers. Default mode is unchanged; the option defaults to false. Requires a token able to read org team members — the same requirement as the documented GitHub Teams support. Closes #49 Co-Authored-By: Claude Fable 5 --- README.md | 10 ++++-- internal/app/app.go | 12 ++++++- internal/app/app_test.go | 7 +++- internal/config/config.go | 2 ++ internal/github/gh.go | 8 +++++ pkg/codeowners/codeowners.go | 17 +++++++-- pkg/codeowners/codeowners_test.go | 58 +++++++++++++++++++++++++++++++ tools/cli/main_test.go | 11 +++--- 8 files changed, 114 insertions(+), 11 deletions(-) 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{ From 324c9408fb0bcffc117d228f8b5e8cbb7194fd87 Mon Sep 17 00:00:00 2001 From: Daniil Pushin Date: Mon, 6 Jul 2026 18:32:45 +0200 Subject: [PATCH 2/2] fix: self-approval never satisfies additional ("&") reviewer groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Additional-reviewer rules represent mandatory extra reviewers, and the existing test suite documents the intent ("Self-approval only applies to OR groups") — but the implementation satisfied any group containing the author, including "&" groups the author (or now their team) is listed in. Tag groups created from "&" rules with a new ReviewerGroup.Additional flag (memoized separately from owner groups so the flag cannot leak between roles) and skip them in SetAuthor self-approval, for both the author's login and their team memberships. Emptiness satisfaction is unchanged: if the author was the sole listed additional reviewer, the group is still satisfied to avoid deadlocking the PR. The merge key in MergeCodeOwners also includes the flag so owner and additional groups with identical names are not deduplicated into one. Co-Authored-By: Claude Fable 5 --- README.md | 3 ++ pkg/codeowners/codeowners.go | 12 ++++-- pkg/codeowners/codeowners_test.go | 65 ++++++++++++++++++++++++++++++- pkg/codeowners/merger.go | 6 ++- pkg/codeowners/reader.go | 6 ++- pkg/codeowners/reviewers.go | 27 +++++++++++-- 6 files changed, 108 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6e2d0f2..9ffa464 100644 --- a/README.md +++ b/README.md @@ -250,6 +250,9 @@ allow_self_approval = false # 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/pkg/codeowners/codeowners.go b/pkg/codeowners/codeowners.go index afdddb0..608eb7b 100644 --- a/pkg/codeowners/codeowners.go +++ b/pkg/codeowners/codeowners.go @@ -69,17 +69,23 @@ func (om *ownersMap) SetAuthor(author string, mode AuthorMode, authorTeams ...Sl 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. + // team members remain valid reviewers. Additional ("&") groups are + // excluded here as well. for _, team := range authorTeams { for _, reviewers := range om.nameReviewerMap[team.Normalized()] { - reviewers.Approved = true + if !reviewers.Additional { + reviewers.Approved = true + } } } } diff --git a/pkg/codeowners/codeowners_test.go b/pkg/codeowners/codeowners_test.go index 27b5b83..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 { @@ -391,6 +391,67 @@ func TestSetAuthorTeamsDoNotAffectUnrelatedGroups(t *testing.T) { } } +// 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