From f40d813f58644bf3b27fa063f129fffc6c56494a Mon Sep 17 00:00:00 2001 From: Chris Hipple Date: Wed, 12 Aug 2026 10:15:48 -0400 Subject: [PATCH 1/2] feat: remove stale review requests (remove_stale_review_requests) Codeowners Plus only ever added review requests, so when a push changed which files a PR touches, the requests made for the old owner set stayed on the PR while the review status comment updated to the new, smaller one. Add an opt-in `remove_stale_review_requests` setting which drops the review requests the action made for owners the current diff no longer involves. The owner set is taken pre-approval and includes optional reviewers, so an owner who already approved keeps their review request instead of having it pulled out from under them. Only requests whose most recent `review_requested` timeline event was performed by the token user are removed, so reviewers somebody added by hand survive a push. Telling the two apart needs a resolvable token user, so removal is skipped with a warning when it cannot be read (e.g. GITHUB_TOKEN), and in quiet mode. The timeline is only fetched when there is at least one candidate, so an ordinary run costs no extra API calls. Co-Authored-By: Claude Opus 5 --- README.md | 37 +++- internal/app/app.go | 56 +++++++ internal/app/app_test.go | 216 ++++++++++++++++++++++++ internal/config/config.go | 2 + internal/config/config_test.go | 21 +++ internal/github/gh.go | 121 ++++++++++++++ internal/github/gh_test.go | 297 +++++++++++++++++++++++++++++++++ 7 files changed, 748 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 90857ad..ed061e1 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.6%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) @@ -21,6 +21,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better - [.codeowners File Spec](#codeowners-file-spec) - [Advanced Configuration](#advanced-configuration) - [Enforcement Options](#enforcement-options) + - [Review Request Removal](#review-request-removal) - [Quiet Mode](#quiet-mode) - [CLI Tool](#cli-tool) - [Contributing](#contributing) @@ -254,6 +255,11 @@ self_approval_via_teams = false # Optional reviewers are still invited with a CC comment. disable_review_status_comments = false +# `remove_stale_review_requests` (default false) removes the review requests Codeowners Plus made +# for owners the pull request no longer involves. +# Requires a token whose user can be resolved - see "Review Request Removal" below +remove_stale_review_requests = false + # `enforcement` allows you to specify how the Codeowners Plus check should be enforced [enforcement] # see "Enforcement Options" below for more details @@ -360,12 +366,39 @@ Result with `require_both_branch_reviewers = true`: **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. +### Review Request Removal + +Codeowners Plus requests reviews from the owners of the changed files every time it runs. When a +push changes which files the pull request touches, some of those owners stop being required - for +example when the author reverts the changes which pulled a team in. By default those review +requests stay on the pull request, so its requested reviewers can end up listing more teams than the +review status comment does. + +Opt in to cleaning them up with `remove_stale_review_requests`: + +`codeowners.toml`: +```toml +# `remove_stale_review_requests` (default false) removes the review requests Codeowners Plus made +# for owners the pull request no longer involves +remove_stale_review_requests = true +``` + +Only review requests made by the token owner are removed. Reviewers added by anybody else stay +requested, even when they do not own any of the changed files, so manually added reviewers survive a +push. Owners which are still required, and owners which are optional reviewers of the changed +files, are never removed. + +Telling the two apart requires resolving the token's user, so this needs a token which can read its +own user - a PAT (the same requirement as [GitHub Teams Support](#github-teams-support)). With a +token whose user cannot be resolved, such as `GITHUB_TOKEN`, removal is skipped with a warning. +Removal is also skipped in [Quiet Mode](#quiet-mode). + ### Quiet Mode Using the `quiet` input on the action will change the behavior in a couple ways: * **No Comments:** The action will **not** post the review status comment (listing required/unapproved reviewers) or the optional reviewer "cc" comment to the Pull Request. -* **No Review Requests:** The action will **not** automatically request reviews from required owners who have not yet approved via the GitHub API. +* **No Review Requests:** The action will **not** automatically request reviews from required owners who have not yet approved via the GitHub API, and will **not** remove its own stale review requests even when `remove_stale_review_requests` is enabled. #### Use Cases diff --git a/internal/app/app.go b/internal/app/app.go index d962024..18db541 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -258,6 +258,12 @@ func (a *App) processApprovalsAndReviewers() (bool, string, []string, error) { return false, message, nil, err } + // Drop review requests for owners the PR no longer needs + err = a.removeStaleReviewRequests(slices.Concat(allRequiredOwnerNames, allOptionalReviewerNames)) + if err != nil { + return false, message, nil, err + } + // Request reviews from required owners err = a.requestReviews() if err != nil { @@ -529,6 +535,56 @@ func (a *App) processApprovals(ghApprovals []*gh.CurrentApproval) (int, error) { return len(ghApprovals) - len(approvalsToDismiss), nil } +// removeStaleReviewRequests drops the review requests this action made for +// owners the PR no longer involves - for example when the author reverts the +// changes which pulled a team in. currentOwners is every owner of the current +// diff (required and optional), and only review requests made by the token user +// are removed, so reviewers somebody added by hand are left alone. +// +// This is opt-in via the remove_stale_review_requests config setting. +func (a *App) removeStaleReviewRequests(currentOwners []codeowners.Slug) error { + if a.config.Quiet { + return nil + } + if !a.Conf.RemoveStaleReviewRequests { + a.printDebug("Skipping stale review request removal (not enabled in config).\n") + return nil + } + + requestedReviewers, err := a.client.GetRequestedReviewers() + if err != nil { + return fmt.Errorf("GetRequestedReviewers Error: %v", err) + } + staleCandidates := codeowners.FilterOutNames(requestedReviewers, currentOwners) + if len(staleCandidates) == 0 { + return nil + } + a.printDebug("Requested Reviewers no longer owning changed files: %s\n", codeowners.OriginalStrings(staleCandidates)) + + // Only remove the requests we made ourselves - anything a human requested + // stays, even when they do not own any of the changed files. + selfRequested, err := a.client.GetSelfRequestedReviewers() + if err != nil { + a.printWarn("WARNING: Error finding review requests to remove: %v\n", err) + return nil + } + staleReviewers := f.Filtered(staleCandidates, func(reviewer codeowners.Slug) bool { + return codeowners.ContainsSlug(selfRequested, reviewer) + }) + + if len(staleReviewers) == 0 { + a.printDebug("No stale review requests to remove.\n") + return nil + } + + a.printDebug("Removing Review Requests from: %s\n", codeowners.OriginalStrings(staleReviewers)) + if err := a.client.RemoveReviewers(codeowners.OriginalStrings(staleReviewers)); err != nil { + return fmt.Errorf("RemoveReviewers Error: %v", err) + } + + return nil +} + func (a *App) requestReviews() error { if a.config.Quiet { return nil diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 30b487a..2273778 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "io" + "slices" "strings" "testing" "time" @@ -117,10 +118,15 @@ type mockGitHubClient struct { tokenUserError error currentlyRequested []codeowners.Slug currentlyRequestedError error + requestedReviewers []codeowners.Slug + requestedReviewersError error + selfRequestedReviewers []codeowners.Slug + selfRequestedError error alreadyReviewed []codeowners.Slug alreadyReviewedError error dismissError error requestReviewersError error + removeReviewersError error warningBuffer io.Writer infoBuffer io.Writer comments []*github.IssueComment @@ -132,6 +138,8 @@ type mockGitHubClient struct { AddCommentCalled bool AddCommentInput string RequestReviewersCalled bool + RemoveReviewersCalled bool + RemoveReviewersInput []string FindExistingCommentCalled bool FindExistingCommentInput string UpdateCommentCalled bool @@ -171,6 +179,14 @@ func (m *mockGitHubClient) GetCurrentlyRequested() ([]codeowners.Slug, error) { return m.currentlyRequested, m.currentlyRequestedError } +func (m *mockGitHubClient) GetRequestedReviewers() ([]codeowners.Slug, error) { + return m.requestedReviewers, m.requestedReviewersError +} + +func (m *mockGitHubClient) GetSelfRequestedReviewers() ([]codeowners.Slug, error) { + return m.selfRequestedReviewers, m.selfRequestedError +} + func (m *mockGitHubClient) GetAlreadyReviewed() ([]codeowners.Slug, error) { return m.alreadyReviewed, m.alreadyReviewedError } @@ -184,6 +200,12 @@ func (m *mockGitHubClient) RequestReviewers(reviewers []string) error { return m.requestReviewersError } +func (m *mockGitHubClient) RemoveReviewers(reviewers []string) error { + m.RemoveReviewersCalled = true + m.RemoveReviewersInput = reviewers + return m.removeReviewersError +} + func (m *mockGitHubClient) CheckApprovals(fileReviewers map[string][]string, approvals []*gh.CurrentApproval, diff git.Diff) ([]codeowners.Slug, []*gh.CurrentApproval) { // Simple mock implementation - approve all reviewers var approvers []codeowners.Slug @@ -265,6 +287,8 @@ func (m *mockGitHubClient) ResetGHClientTracking() { m.AddCommentCalled = false m.AddCommentInput = "" m.RequestReviewersCalled = false + m.RemoveReviewersCalled = false + m.RemoveReviewersInput = nil } func (m *mockGitHubClient) IsSubstringInComments(substring string, since *time.Time) (bool, error) { @@ -758,6 +782,198 @@ func TestRequestReviews(t *testing.T) { } } +func TestRemoveStaleReviewRequests(t *testing.T) { + tt := []struct { + name string + quiet bool + removalEnabled bool + currentOwners []codeowners.Slug + requestedReviewers []codeowners.Slug + requestedReviewersError error + selfRequestedReviewers []codeowners.Slug + selfRequestedError error + removeReviewersError error + expectedShouldCall bool + expectedRemoved []string + expectError bool + }{ + { + name: "short circuits when not enabled in config", + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + expectedShouldCall: false, + }, + { + name: "short circuits in quiet mode", + quiet: true, + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + expectedShouldCall: false, + }, + { + name: "no requested reviewers", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: []codeowners.Slug{}, + expectedShouldCall: false, + }, + { + name: "all requested reviewers still own changed files", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1", "@user1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team1", "@user1"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team1", "@user1"}), + expectedShouldCall: false, + }, + { + name: "removes stale requests made by the action", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team1", "@org/team2", "@org/team3"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team1", "@org/team2", "@org/team3"}), + expectedShouldCall: true, + expectedRemoved: []string{"@org/team2", "@org/team3"}, + }, + { + name: "leaves stale requests made by somebody else", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team2", "@user2"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + expectedShouldCall: true, + expectedRemoved: []string{"@org/team2"}, + }, + { + name: "keeps optional owners which are no longer required", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1", "@org/optional"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/optional"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/optional"}), + expectedShouldCall: false, + }, + { + name: "matches owners case insensitively", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@Org/Team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team1"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team1"}), + expectedShouldCall: false, + }, + { + name: "skips removal when self requested lookup fails", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + selfRequestedError: fmt.Errorf("token user unavailable"), + expectedShouldCall: false, + }, + { + name: "errors when requested reviewers cannot be read", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewersError: fmt.Errorf("no PR"), + expectedShouldCall: false, + expectError: true, + }, + { + name: "errors when removal fails", + removalEnabled: true, + currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), + requestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), + removeReviewersError: fmt.Errorf("api error"), + expectedShouldCall: true, + expectedRemoved: []string{"@org/team2"}, + expectError: true, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + app, mockClient := setupAppForTest(t, tc.quiet) + mockClient.ResetGHClientTracking() + + app.Conf.RemoveStaleReviewRequests = tc.removalEnabled + mockClient.requestedReviewers = tc.requestedReviewers + mockClient.requestedReviewersError = tc.requestedReviewersError + mockClient.selfRequestedReviewers = tc.selfRequestedReviewers + mockClient.selfRequestedError = tc.selfRequestedError + mockClient.removeReviewersError = tc.removeReviewersError + + err := app.removeStaleReviewRequests(tc.currentOwners) + + if tc.expectError && err == nil { + t.Error("Expected an error during removeStaleReviewRequests, but got nil") + } + if !tc.expectError && err != nil { + t.Errorf("Unexpected error during removeStaleReviewRequests: %v", err) + } + + if mockClient.RemoveReviewersCalled != tc.expectedShouldCall { + t.Errorf("Expected mockClient.RemoveReviewersCalled to be %t, but got %t", tc.expectedShouldCall, mockClient.RemoveReviewersCalled) + } + if !slices.Equal(mockClient.RemoveReviewersInput, tc.expectedRemoved) { + t.Errorf("Expected removed reviewers %v, got %v", tc.expectedRemoved, mockClient.RemoveReviewersInput) + } + }) + } +} + +// Removal runs against every owner of the current diff, not just the owners +// still waiting to approve, so an owner who already approved keeps their review +// request instead of having it pulled out from under them. +func TestProcessApprovalsAndReviewersRemovesOnlyStaleRequests(t *testing.T) { + mockGH := &mockGitHubClient{ + currentApprovals: []*gh.CurrentApproval{ + {GHLogin: codeowners.NewSlug("@user1")}, + }, + requestedReviewers: codeowners.NewSlugs([]string{"@user1", "@org/team1", "@user3", "@org/stale"}), + selfRequestedReviewers: codeowners.NewSlugs([]string{"@user1", "@org/team1", "@user3", "@org/stale"}), + } + + mockOwners := &mockCodeOwners{ + requiredOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@org/team1"})}, + }, + optionalOwners: codeowners.ReviewerGroups{ + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user3"})}, + }, + fileRequiredMap: map[string]codeowners.ReviewerGroups{ + "file1.go": { + &codeowners.ReviewerGroup{Names: codeowners.NewSlugs([]string{"@user1"})}, + }, + }, + } + + app := &App{ + config: &Config{ + InfoBuffer: io.Discard, + WarningBuffer: io.Discard, + }, + client: mockGH, + codeowners: mockOwners, + gitDiff: mockGitDiff{changes: []string{"file1.go"}}, + Conf: &owners.Config{ + Enforcement: &owners.Enforcement{Approval: false, FailCheck: true}, + AdminBypass: &owners.AdminBypass{Enabled: false, AllowedUsers: []string{}}, + RemoveStaleReviewRequests: true, + }, + } + + if _, _, _, err := app.processApprovalsAndReviewers(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := []string{"@org/stale"} + if !slices.Equal(mockGH.RemoveReviewersInput, expected) { + t.Errorf("expected removed reviewers %v, got %v", expected, mockGH.RemoveReviewersInput) + } +} + func TestProcessApprovalsAndReviewers(t *testing.T) { maxReviews := 2 minReviews := 2 diff --git a/internal/config/config.go b/internal/config/config.go index c3bcf7e..9b4c587 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,7 @@ type Config struct { AllowSelfApproval bool `toml:"allow_self_approval"` SelfApprovalViaTeams bool `toml:"self_approval_via_teams"` DisableReviewStatusComments bool `toml:"disable_review_status_comments"` + RemoveStaleReviewRequests bool `toml:"remove_stale_review_requests"` } type Enforcement struct { @@ -52,6 +53,7 @@ func ReadConfig(path string, fileReader codeowners.FileReader) (*Config, error) DisableSmartDismissal: false, RequireBothBranchReviewers: false, DisableReviewStatusComments: false, + RemoveStaleReviewRequests: false, } // Use filesystem reader if none provided diff --git a/internal/config/config_test.go b/internal/config/config_test.go index f09c117..d8e497a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -125,6 +125,23 @@ allow_self_approval = true }, expectedErr: false, }, + { + name: "config with remove_stale_review_requests enabled", + configContent: ` +remove_stale_review_requests = true +`, + path: "testdata/", + expected: &Config{ + MaxReviews: nil, + MinReviews: nil, + UnskippableReviewers: []string{}, + Ignore: []string{}, + Enforcement: &Enforcement{Approval: false, FailCheck: true}, + HighPriorityLabels: []string{}, + RemoveStaleReviewRequests: true, + }, + expectedErr: false, + }, { name: "invalid toml", configContent: ` @@ -215,6 +232,10 @@ max_reviews = invalid t.Errorf("AllowSelfApproval: expected %v, got %v", tc.expected.AllowSelfApproval, got.AllowSelfApproval) } + if got.RemoveStaleReviewRequests != tc.expected.RemoveStaleReviewRequests { + t.Errorf("RemoveStaleReviewRequests: expected %v, got %v", tc.expected.RemoveStaleReviewRequests, got.RemoveStaleReviewRequests) + } + if tc.expected.Enforcement != nil { if got.Enforcement == nil { t.Error("expected Enforcement to be set") diff --git a/internal/github/gh.go b/internal/github/gh.go index 8803d66..a827499 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -41,8 +41,11 @@ type Client interface { GetCurrentReviewerApprovals() ([]*CurrentApproval, error) GetAlreadyReviewed() ([]codeowners.Slug, error) GetCurrentlyRequested() ([]codeowners.Slug, error) + GetRequestedReviewers() ([]codeowners.Slug, error) + GetSelfRequestedReviewers() ([]codeowners.Slug, error) DismissStaleReviews(staleApprovals []*CurrentApproval) error RequestReviewers(reviewers []string) error + RemoveReviewers(reviewers []string) error ApprovePR() error InitComments() error AddComment(comment string) error @@ -365,6 +368,105 @@ func currentlyRequested(pr *github.PullRequest, owner string, userReviewerMap gh return slices.Collect(maps.Values(seen)) } +// GetRequestedReviewers returns every reviewer currently requested on the PR, +// including ones which are not code owners. Unlike GetCurrentlyRequested, the +// results are not mapped through the user reviewer map, so reviewers who no +// longer own any of the changed files are still reported. +func (gh *GHClient) GetRequestedReviewers() ([]codeowners.Slug, error) { + if gh.pr == nil { + return nil, &NoPRError{} + } + return requestedReviewers(gh.pr, gh.owner), nil +} + +// requestedReviewers returns the reviewers requested on the PR as owner slugs - +// users as "@login" and teams as "@owner/team-slug". +func requestedReviewers(pr *github.PullRequest, owner string) []codeowners.Slug { + users := f.Map(pr.RequestedReviewers, func(user *github.User) codeowners.Slug { + return codeowners.NewSlug(fmt.Sprintf("@%s", user.GetLogin())) + }) + teams := f.Map(pr.RequestedTeams, func(team *github.Team) codeowners.Slug { + return codeowners.NewSlug(fmt.Sprintf("@%s/%s", owner, team.GetSlug())) + }) + return slices.Concat(users, teams) +} + +// GetSelfRequestedReviewers returns the reviewers currently requested on the PR +// whose latest review request was made by the user the client token belongs to. +// Reviewers added by anybody else - a human picking an extra reviewer, GitHub +// CODEOWNERS - are left out, so callers can clean up their own review requests +// without touching someone else's. +func (gh *GHClient) GetSelfRequestedReviewers() ([]codeowners.Slug, error) { + if gh.pr == nil { + return nil, &NoPRError{} + } + tokenUser, err := gh.GetTokenUser() + if err != nil { + return nil, fmt.Errorf("GetTokenUser Error: %w", err) + } + if tokenUser == "" { + // Without a login there is no way to tell our own requests apart from + // everybody else's, and an empty login would match every actorless event + return nil, fmt.Errorf("GetTokenUser Error: token user has no login") + } + timeline, err := gh.listTimeline() + if err != nil { + return nil, err + } + return selfRequestedReviewers(gh.pr, gh.owner, timeline, tokenUser), nil +} + +func (gh *GHClient) listTimeline() ([]*github.Timeline, error) { + allEvents := make([]*github.Timeline, 0) + listTimeline := func(page int) (*github.Response, error) { + listOptions := &github.ListOptions{PerPage: 100, Page: page} + events, res, err := gh.client.Issues.ListIssueTimeline(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), listOptions) + if err != nil { + return nil, err + } + defer func() { + _ = res.Body.Close() + }() + allEvents = append(allEvents, events...) + return res, err + } + if err := walkPaginatedApi(listTimeline); err != nil { + return nil, err + } + return allEvents, nil +} + +// selfRequestedReviewers filters the currently requested reviewers down to those +// whose most recent review request event was performed by tokenUser. +func selfRequestedReviewers(pr *github.PullRequest, owner string, timeline []*github.Timeline, tokenUser string) []codeowners.Slug { + tokenUserSlug := codeowners.NewSlug(tokenUser) + // The timeline is in chronological order, so the last request event for a + // reviewer is the one which put them in the PR's requested reviewers. + requesters := make(map[string]string) + for _, event := range timeline { + var reviewer codeowners.Slug + switch { + case event.Reviewer != nil: + reviewer = codeowners.NewSlug(fmt.Sprintf("@%s", event.Reviewer.GetLogin())) + case event.RequestedTeam != nil: + reviewer = codeowners.NewSlug(fmt.Sprintf("@%s/%s", owner, event.RequestedTeam.GetSlug())) + default: + continue + } + switch event.GetEvent() { + case "review_requested": + requesters[reviewer.Normalized()] = event.GetActor().GetLogin() + case "review_request_removed": + delete(requesters, reviewer.Normalized()) + } + } + + return f.Filtered(requestedReviewers(pr, owner), func(reviewer codeowners.Slug) bool { + requester, found := requesters[reviewer.Normalized()] + return found && tokenUserSlug.EqualsString(requester) + }) +} + func (gh *GHClient) DismissStaleReviews(staleApprovals []*CurrentApproval) error { if gh.pr == nil { return &NoPRError{} @@ -402,6 +504,25 @@ func (gh *GHClient) RequestReviewers(reviewers []string) error { return err } +func (gh *GHClient) RemoveReviewers(reviewers []string) error { + if gh.pr == nil { + return &NoPRError{} + } + if len(reviewers) == 0 { + return nil + } + indvidualReviewers, teamReviewers := splitReviewers(reviewers) + reviewersRequest := github.ReviewersRequest{Reviewers: indvidualReviewers, TeamReviewers: teamReviewers} + res, err := gh.client.PullRequests.RemoveReviewers(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), reviewersRequest) + if err != nil { + return err + } + defer func() { + _ = res.Body.Close() + }() + return err +} + func splitReviewers(reviewers []string) ([]string, []string) { indvidualReviewers := make([]string, 0, len(reviewers)) teamReviewers := make([]string, 0, len(reviewers)) diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index d43fab4..fda3b02 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "slices" "testing" "time" @@ -411,6 +412,26 @@ func TestNilPRErr(t *testing.T) { return gh.RequestReviewers([]string{}) }, }, + { + name: "RemoveReviewers", + testFn: func() error { + return gh.RemoveReviewers([]string{}) + }, + }, + { + name: "GetRequestedReviewers", + testFn: func() error { + _, err := gh.GetRequestedReviewers() + return err + }, + }, + { + name: "GetSelfRequestedReviewers", + testFn: func() error { + _, err := gh.GetSelfRequestedReviewers() + return err + }, + }, { name: "AddComment", testFn: func() error { @@ -840,6 +861,282 @@ func TestRequestReviewersFailure(t *testing.T) { } } +func TestRemoveReviewersSuccess(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{Number: github.Ptr(123)} + + reviewers := []string{"@reviewer1", "@org/team1"} + + // Mock the GitHub API endpoint + mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/requested_reviewers", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + t.Errorf("expected method DELETE, got %s", r.Method) + } + + // Validate the request payload + var req github.ReviewersRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("failed to decode request body: %v", err) + } + if len(req.Reviewers) != 1 || req.Reviewers[0] != "reviewer1" { + t.Errorf("expected reviewers [reviewer1], got %v", req.Reviewers) + } + if len(req.TeamReviewers) != 1 || req.TeamReviewers[0] != "team1" { + t.Errorf("expected team reviewers [team1], got %v", req.TeamReviewers) + } + + w.WriteHeader(http.StatusOK) + }) + + err := gh.RemoveReviewers(reviewers) + if err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestRemoveReviewersNoReviewers(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{Number: github.Ptr(123)} + + mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/requested_reviewers", func(w http.ResponseWriter, r *http.Request) { + t.Error("expected no request to be made for an empty reviewer list") + }) + + if err := gh.RemoveReviewers([]string{}); err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestRemoveReviewersFailure(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{Number: github.Ptr(123)} + + reviewers := []string{"@reviewer1", "@org/team1"} + + // Mock the GitHub API to simulate an error + mux.HandleFunc("/repos/test-owner/test-repo/pulls/123/requested_reviewers", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + }) + + err := gh.RemoveReviewers(reviewers) + if err == nil { + t.Error("expected an error, got nil") + } +} + +func TestGetRequestedReviewers(t *testing.T) { + c, err := NewClient("test-owner", "test-repo", "test-token") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + gh := c.(*GHClient) + gh.pr = &github.PullRequest{ + Number: github.Ptr(123), + RequestedReviewers: []*github.User{ + {Login: github.Ptr("user1")}, + }, + RequestedTeams: []*github.Team{ + {Slug: github.Ptr("team1")}, + {Slug: github.Ptr("team2")}, + }, + } + + requested, err := gh.GetRequestedReviewers() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := []string{"@user1", "@test-owner/team1", "@test-owner/team2"} + if !slices.Equal(codeowners.OriginalStrings(requested), expected) { + t.Errorf("expected %v, got %v", expected, codeowners.OriginalStrings(requested)) + } +} + +func TestSelfRequestedReviewers(t *testing.T) { + requested := func(actor string, reviewer *github.User, team *github.Team) *github.Timeline { + return &github.Timeline{ + Event: github.Ptr("review_requested"), + Actor: &github.User{Login: github.Ptr(actor)}, + Reviewer: reviewer, + RequestedTeam: team, + } + } + removed := func(actor string, reviewer *github.User, team *github.Team) *github.Timeline { + return &github.Timeline{ + Event: github.Ptr("review_request_removed"), + Actor: &github.User{Login: github.Ptr(actor)}, + Reviewer: reviewer, + RequestedTeam: team, + } + } + user := func(login string) *github.User { return &github.User{Login: github.Ptr(login)} } + team := func(slug string) *github.Team { return &github.Team{Slug: github.Ptr(slug)} } + + tt := []struct { + name string + pr *github.PullRequest + timeline []*github.Timeline + expected []string + }{ + { + name: "requests made by the token user", + pr: &github.PullRequest{ + RequestedReviewers: []*github.User{user("user1")}, + RequestedTeams: []*github.Team{team("team1")}, + }, + timeline: []*github.Timeline{ + requested("bot-user", user("user1"), nil), + requested("bot-user", nil, team("team1")), + }, + expected: []string{"@user1", "@test-owner/team1"}, + }, + { + name: "requests made by somebody else are excluded", + pr: &github.PullRequest{ + RequestedReviewers: []*github.User{user("user1")}, + RequestedTeams: []*github.Team{team("team1")}, + }, + timeline: []*github.Timeline{ + requested("human", user("user1"), nil), + requested("bot-user", nil, team("team1")), + }, + expected: []string{"@test-owner/team1"}, + }, + { + name: "the most recent request wins", + pr: &github.PullRequest{ + RequestedTeams: []*github.Team{team("team1"), team("team2")}, + }, + timeline: []*github.Timeline{ + requested("human", nil, team("team1")), + requested("bot-user", nil, team("team2")), + requested("bot-user", nil, team("team1")), + removed("human", nil, team("team2")), + requested("human", nil, team("team2")), + }, + expected: []string{"@test-owner/team1"}, + }, + { + name: "reviewers no longer requested on the PR are excluded", + pr: &github.PullRequest{ + RequestedTeams: []*github.Team{team("team1")}, + }, + timeline: []*github.Timeline{ + requested("bot-user", nil, team("team1")), + requested("bot-user", nil, team("team2")), + }, + expected: []string{"@test-owner/team1"}, + }, + { + name: "reviewers without a request event are excluded", + pr: &github.PullRequest{ + RequestedTeams: []*github.Team{team("team1")}, + }, + timeline: []*github.Timeline{ + {Event: github.Ptr("commented"), Actor: user("bot-user")}, + }, + expected: []string{}, + }, + { + name: "actor matching is case insensitive", + pr: &github.PullRequest{ + RequestedTeams: []*github.Team{team("team1")}, + }, + timeline: []*github.Timeline{ + requested("Bot-User", nil, team("team1")), + }, + expected: []string{"@test-owner/team1"}, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + result := selfRequestedReviewers(tc.pr, "test-owner", tc.timeline, "bot-user") + if !slices.Equal(codeowners.OriginalStrings(result), tc.expected) { + t.Errorf("expected %v, got %v", tc.expected, codeowners.OriginalStrings(result)) + } + }) + } +} + +func TestGetSelfRequestedReviewers(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{ + Number: github.Ptr(123), + RequestedReviewers: []*github.User{{Login: github.Ptr("user1")}}, + RequestedTeams: []*github.Team{{Slug: github.Ptr("team1")}}, + } + + mux.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(&github.User{Login: github.Ptr("bot-user")}) + }) + mux.HandleFunc("/repos/test-owner/test-repo/issues/123/timeline", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode([]*github.Timeline{ + { + Event: github.Ptr("review_requested"), + Actor: &github.User{Login: github.Ptr("human")}, + Reviewer: &github.User{Login: github.Ptr("user1")}, + }, + { + Event: github.Ptr("review_requested"), + Actor: &github.User{Login: github.Ptr("bot-user")}, + RequestedTeam: &github.Team{Slug: github.Ptr("team1")}, + }, + }) + }) + + requested, err := gh.GetSelfRequestedReviewers() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expected := []string{"@test-owner/team1"} + if !slices.Equal(codeowners.OriginalStrings(requested), expected) { + t.Errorf("expected %v, got %v", expected, codeowners.OriginalStrings(requested)) + } +} + +func TestGetSelfRequestedReviewersTokenUserFailure(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{Number: github.Ptr(123)} + + mux.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Resource not accessible by integration", http.StatusForbidden) + }) + + if _, err := gh.GetSelfRequestedReviewers(); err == nil { + t.Error("expected an error, got nil") + } +} + +func TestGetSelfRequestedReviewersTimelineFailure(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + gh.pr = &github.PullRequest{Number: github.Ptr(123)} + + mux.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(&github.User{Login: github.Ptr("bot-user")}) + }) + mux.HandleFunc("/repos/test-owner/test-repo/issues/123/timeline", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Internal Server Error", http.StatusInternalServerError) + }) + + if _, err := gh.GetSelfRequestedReviewers(); err == nil { + t.Error("expected an error, got nil") + } +} + func TestApprovePRSuccess(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close() From a1a25aa9cf988f1ba0b7483bfd660014c49fa729 Mon Sep 17 00:00:00 2001 From: Chris Hipple Date: Wed, 12 Aug 2026 10:31:19 -0400 Subject: [PATCH 2/2] fix: address PRism review feedback - Downgrade a RemoveReviewers failure to a warning. Removal is an opt-in cosmetic cleanup, so a transient GitHub API error should not fail the check. This also makes the function consistent: both of its network calls now warn and continue, leaving only the nil-PR guard fatal. - Cache the token user on GHClient so GetTokenUser costs at most one GET /user per run. It can now be called twice, from processTokenOwnerApproval and GetSelfRequestedReviewers. - Fix the `indvidualReviewers` typo in RemoveReviewers along with the two pre-existing occurrences in RequestReviewers and splitReviewers. NewClient now uses named struct fields - adding tokenUser to a ten-field positional literal was asking for a silently misassigned value. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- internal/app/app.go | 3 ++- internal/app/app_test.go | 5 +++-- internal/github/gh.go | 39 ++++++++++++++++++++------------------ internal/github/gh_test.go | 26 +++++++++++++++++++++++++ 5 files changed, 53 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index ed061e1..e37cc9e 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.4%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/internal/app/app.go b/internal/app/app.go index 18db541..b763273 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -579,7 +579,8 @@ func (a *App) removeStaleReviewRequests(currentOwners []codeowners.Slug) error { a.printDebug("Removing Review Requests from: %s\n", codeowners.OriginalStrings(staleReviewers)) if err := a.client.RemoveReviewers(codeowners.OriginalStrings(staleReviewers)); err != nil { - return fmt.Errorf("RemoveReviewers Error: %v", err) + // Removal is cosmetic - a transient API error should not fail the check + a.printWarn("WARNING: Error removing stale review requests: %v\n", err) } return nil diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 2273778..dd8ef5c 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -879,7 +879,8 @@ func TestRemoveStaleReviewRequests(t *testing.T) { expectError: true, }, { - name: "errors when removal fails", + // Removal is cosmetic, so a failed cleanup must not fail the check + name: "warns and continues when removal fails", removalEnabled: true, currentOwners: codeowners.NewSlugs([]string{"@org/team1"}), requestedReviewers: codeowners.NewSlugs([]string{"@org/team2"}), @@ -887,7 +888,7 @@ func TestRemoveStaleReviewRequests(t *testing.T) { removeReviewersError: fmt.Errorf("api error"), expectedShouldCall: true, expectedRemoved: []string{"@org/team2"}, - expectError: true, + expectError: false, }, } diff --git a/internal/github/gh.go b/internal/github/gh.go index a827499..2d2a134 100644 --- a/internal/github/gh.go +++ b/internal/github/gh.go @@ -68,6 +68,7 @@ type GHClient struct { userReviewerMap ghUserReviewerMap comments []*github.IssueComment reviews []*github.PullRequestReview + tokenUser string warningBuffer io.Writer infoBuffer io.Writer } @@ -78,16 +79,12 @@ func NewClient(owner, repo, token string) (Client, error) { return nil, err } return &GHClient{ - context.Background(), - owner, - repo, - client, - nil, - nil, - nil, - nil, - io.Discard, - io.Discard, + ctx: context.Background(), + owner: owner, + repo: repo, + client: client, + warningBuffer: io.Discard, + infoBuffer: io.Discard, }, nil } @@ -145,12 +142,18 @@ func (gh *GHClient) UserReviewers(user string) []codeowners.Slug { return gh.userReviewerMap[strings.ToLower(strings.TrimPrefix(user, "@"))] } +// GetTokenUser returns the login of the user the client token belongs to, +// caching it so repeat callers do not each cost a GET /user. func (gh *GHClient) GetTokenUser() (string, error) { + if gh.tokenUser != "" { + return gh.tokenUser, nil + } user, _, err := gh.client.Users.Get(gh.ctx, "") if err != nil { return "", err } - return user.GetLogin(), nil + gh.tokenUser = user.GetLogin() + return gh.tokenUser, nil } func (gh *GHClient) InitReviews() error { @@ -492,8 +495,8 @@ func (gh *GHClient) RequestReviewers(reviewers []string) error { if len(reviewers) == 0 { return nil } - indvidualReviewers, teamReviewers := splitReviewers(reviewers) - reviewersRequest := github.ReviewersRequest{Reviewers: indvidualReviewers, TeamReviewers: teamReviewers} + individualReviewers, teamReviewers := splitReviewers(reviewers) + reviewersRequest := github.ReviewersRequest{Reviewers: individualReviewers, TeamReviewers: teamReviewers} _, res, err := gh.client.PullRequests.RequestReviewers(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), reviewersRequest) if err != nil { return err @@ -511,8 +514,8 @@ func (gh *GHClient) RemoveReviewers(reviewers []string) error { if len(reviewers) == 0 { return nil } - indvidualReviewers, teamReviewers := splitReviewers(reviewers) - reviewersRequest := github.ReviewersRequest{Reviewers: indvidualReviewers, TeamReviewers: teamReviewers} + individualReviewers, teamReviewers := splitReviewers(reviewers) + reviewersRequest := github.ReviewersRequest{Reviewers: individualReviewers, TeamReviewers: teamReviewers} res, err := gh.client.PullRequests.RemoveReviewers(gh.ctx, gh.owner, gh.repo, gh.pr.GetNumber(), reviewersRequest) if err != nil { return err @@ -524,7 +527,7 @@ func (gh *GHClient) RemoveReviewers(reviewers []string) error { } func splitReviewers(reviewers []string) ([]string, []string) { - indvidualReviewers := make([]string, 0, len(reviewers)) + individualReviewers := make([]string, 0, len(reviewers)) teamReviewers := make([]string, 0, len(reviewers)) for _, reviewer := range reviewers { reviewerString := reviewer[1:] // trim the @ @@ -532,10 +535,10 @@ func splitReviewers(reviewers []string) ([]string, []string) { split := strings.SplitN(reviewerString, "/", 2) teamReviewers = append(teamReviewers, split[1]) } else { - indvidualReviewers = append(indvidualReviewers, reviewerString) + individualReviewers = append(individualReviewers, reviewerString) } } - return indvidualReviewers, teamReviewers + return individualReviewers, teamReviewers } func (gh *GHClient) ApprovePR() error { diff --git a/internal/github/gh_test.go b/internal/github/gh_test.go index fda3b02..33e0c19 100644 --- a/internal/github/gh_test.go +++ b/internal/github/gh_test.go @@ -693,6 +693,32 @@ func TestGetTokenUserSuccess(t *testing.T) { } } +func TestGetTokenUserCached(t *testing.T) { + mux, server, gh := mockServerAndClient(t) + defer server.Close() + + calls := 0 + mux.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(&github.User{Login: github.Ptr("test-user")}) + }) + + for range 3 { + user, err := gh.GetTokenUser() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if user != "test-user" { + t.Errorf("expected user 'test-user', got '%s'", user) + } + } + + if calls != 1 { + t.Errorf("expected 1 call to GET /user, got %d", calls) + } +} + func TestGetTokenUserFailure(t *testing.T) { mux, server, gh := mockServerAndClient(t) defer server.Close()