From 738b82ddab008e824a962351a7cf18ac2a44058a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 27 Jan 2026 22:21:22 -0700 Subject: [PATCH 01/13] Add CCR and reviewer MultiSelectWithSearch Enables Copilot to be requested as a pull request reviewer, supporting both interactive and non-interactive flows. Introduces new reviewer search functionality, updates reviewer partitioning to distinguish users, bots, and teams, and adds a GraphQL mutation for reviewer management on github.com. Updates help text, tests, and internal APIs to support Copilot reviewer login and display names, while maintaining compatibility with GitHub Enterprise Server. --- api/queries_pr.go | 305 ++++++++++++++++++++++++++++++- api/queries_repo.go | 8 +- api/query_builder.go | 4 + pkg/cmd/pr/edit/edit.go | 141 +++++++++++--- pkg/cmd/pr/edit/edit_test.go | 172 +++++++++++++---- pkg/cmd/pr/shared/editable.go | 58 ++++-- pkg/cmd/pr/shared/params.go | 17 +- pkg/cmd/pr/shared/params_test.go | 31 ++++ 8 files changed, 658 insertions(+), 78 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index c806d6bb7..f006c529c 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -323,6 +324,19 @@ func (r RequestedReviewer) LoginOrSlug() string { return r.Login } +// DisplayName returns a user-friendly name for the reviewer. +// For Copilot bot, returns "Copilot (AI)". For teams, returns "org/slug". +// For users, returns login (could be extended to show name if available). +func (r RequestedReviewer) DisplayName() string { + if r.TypeName == teamTypeName { + return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) + } + if r.TypeName == "Bot" && r.Login == CopilotReviewerLogin { + return "Copilot (AI)" + } + return r.Login +} + const teamTypeName = "Team" func (r ReviewRequests) Logins() []string { @@ -333,6 +347,15 @@ func (r ReviewRequests) Logins() []string { return logins } +// DisplayNames returns user-friendly display names for all requested reviewers. +func (r ReviewRequests) DisplayNames() []string { + names := make([]string, len(r.Nodes)) + for i, r := range r.Nodes { + names[i] = r.RequestedReviewer.DisplayName() + } + return names +} + func (pr PullRequest) HeadLabel() string { if pr.IsCrossRepository { return fmt.Sprintf("%s:%s", pr.HeadRepositoryOwner.Login, pr.HeadRefName) @@ -632,6 +655,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } // AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. +// Team identifiers can be in "org/slug" format. func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { if len(users) == 0 && len(teams) == 0 { return nil @@ -641,8 +665,15 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, if users == nil { users = []string{} } - if teams == nil { - teams = []string{} + + // Extract just the slug from org/slug format + teamSlugs := make([]string, 0, len(teams)) + for _, t := range teams { + if idx := strings.Index(t, "/"); idx >= 0 { + teamSlugs = append(teamSlugs, t[idx+1:]) + } else if t != "" { + teamSlugs = append(teamSlugs, t) + } } path := fmt.Sprintf( @@ -656,7 +687,7 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, TeamReviewers []string `json:"team_reviewers"` }{ Reviewers: users, - TeamReviewers: teams, + TeamReviewers: teamSlugs, } buf := &bytes.Buffer{} if err := json.NewEncoder(buf).Encode(body); err != nil { @@ -667,6 +698,7 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, } // RemovePullRequestReviews removes requested reviewers from a pull request using the REST API. +// Team identifiers can be in "org/slug" format. func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { if len(users) == 0 && len(teams) == 0 { return nil @@ -676,8 +708,15 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in if users == nil { users = []string{} } - if teams == nil { - teams = []string{} + + // Extract just the slug from org/slug format + teamSlugs := make([]string, 0, len(teams)) + for _, t := range teams { + if idx := strings.Index(t, "/"); idx >= 0 { + teamSlugs = append(teamSlugs, t[idx+1:]) + } else if t != "" { + teamSlugs = append(teamSlugs, t) + } } path := fmt.Sprintf( @@ -691,7 +730,7 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in TeamReviewers []string `json:"team_reviewers"` }{ Reviewers: users, - TeamReviewers: teams, + TeamReviewers: teamSlugs, } buf := &bytes.Buffer{} if err := json.NewEncoder(buf).Encode(body); err != nil { @@ -701,6 +740,70 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in return client.REST(repo.RepoHost(), "DELETE", path, buf, nil) } +// RequestReviewsByLogin sets requested reviewers on a pull request using the GraphQL mutation. +// This mutation replaces existing reviewers with the provided set unless union is true. +// Only available on github.com, not GHES. +// Bot logins should include the [bot] suffix (e.g., "copilot-pull-request-reviewer[bot]"). +// Team slugs should be in the format "org/team-slug". +// When union is false (replace mode), passing empty slices will remove all reviewers. +func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, userLogins, botLogins, teamSlugs []string, union bool) error { + // In union mode (additive), nothing to do if all lists are empty. + // In replace mode, we may still need to call the mutation to clear reviewers. + if union && len(userLogins) == 0 && len(botLogins) == 0 && len(teamSlugs) == 0 { + return nil + } + + var mutation struct { + RequestReviewsByLogin struct { + ClientMutationID string + } `graphql:"requestReviewsByLogin(input: $input)"` + } + + type RequestReviewsByLoginInput struct { + PullRequestID githubv4.ID `json:"pullRequestId"` + UserLogins *[]githubv4.String `json:"userLogins,omitempty"` + BotLogins *[]githubv4.String `json:"botLogins,omitempty"` + TeamSlugs *[]githubv4.String `json:"teamSlugs,omitempty"` + Union githubv4.Boolean `json:"union"` + } + + input := RequestReviewsByLoginInput{ + PullRequestID: githubv4.ID(prID), + Union: githubv4.Boolean(union), + } + + if len(userLogins) > 0 { + logins := make([]githubv4.String, len(userLogins)) + for i, l := range userLogins { + logins[i] = githubv4.String(l) + } + input.UserLogins = &logins + } + + if len(botLogins) > 0 { + logins := make([]githubv4.String, len(botLogins)) + for i, l := range botLogins { + // Bot logins require the [bot] suffix for the mutation + logins[i] = githubv4.String(l + "[bot]") + } + input.BotLogins = &logins + } + + if len(teamSlugs) > 0 { + slugs := make([]githubv4.String, len(teamSlugs)) + for i, s := range teamSlugs { + slugs[i] = githubv4.String(s) + } + input.TeamSlugs = &slugs + } + + variables := map[string]interface{}{ + "input": input, + } + + return client.Mutate(repo.RepoHost(), "RequestReviewsByLogin", &mutation, variables) +} + // SuggestedAssignableActors fetches up to 10 suggested actors for a specific assignable // (Issue or PullRequest) node ID. `assignableID` is the GraphQL node ID for the Issue/PR. // Returns the actors, the total count of available assignees in the repo, and an error. @@ -793,6 +896,196 @@ func SuggestedAssignableActors(client *Client, repo ghrepo.Interface, assignable return actors, availableAssigneesCount, nil } +// ReviewerCandidate represents a potential reviewer for a pull request. +// This can be a User, Bot, or Team. +type ReviewerCandidate interface { + DisplayName() string + Login() string + + sealedReviewerCandidate() +} + +// ReviewerUser is a user who can review a pull request. +type ReviewerUser struct { + AssignableUser +} + +func NewReviewerUser(login, name string) ReviewerUser { + return ReviewerUser{ + AssignableUser: NewAssignableUser("", login, name), + } +} + +func (r ReviewerUser) sealedReviewerCandidate() {} + +// ReviewerBot is a bot who can review a pull request. +type ReviewerBot struct { + AssignableBot +} + +func NewReviewerBot(login string) ReviewerBot { + return ReviewerBot{ + AssignableBot: NewAssignableBot("", login), + } +} + +func (b ReviewerBot) DisplayName() string { + if b.login == CopilotReviewerLogin { + return fmt.Sprintf("%s (AI)", CopilotActorName) + } + return b.Login() +} + +func (r ReviewerBot) sealedReviewerCandidate() {} + +// ReviewerTeam is a team that can review a pull request. +// The slug is stored as "org/team-slug" format. +type ReviewerTeam struct { + slug string +} + +// NewReviewerTeam creates a new ReviewerTeam with the full "org/slug" format. +func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { + return ReviewerTeam{slug: fmt.Sprintf("%s/%s", orgName, teamSlug)} +} + +func (r ReviewerTeam) DisplayName() string { + return r.slug +} + +func (r ReviewerTeam) Login() string { + return r.slug +} + +func (r ReviewerTeam) Slug() string { + return r.slug +} + +func (r ReviewerTeam) sealedReviewerCandidate() {} + +// SuggestedReviewerActors fetches suggested reviewers for a pull request. +// It combines results from: +// - suggestedReviewerActors (10 max) - suggested based on activity +// - repository collaborators (10 max) - all collaborators +// - organization teams (10 max for org repos) - all teams (if owner is an org) +// Results are returned in that order with duplicates removed. +// Returns the candidates, a MoreResults count, and an error. +func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) { + // Use a single query that includes organization.teams - if the owner is not an org, + // we'll get a "Could not resolve to an Organization" error which we handle gracefully. + type responseData struct { + Node struct { + PullRequest struct { + SuggestedActors struct { + Nodes []struct { + IsAuthor bool + IsCommenter bool + Reviewer struct { + TypeName string `graphql:"__typename"` + User struct { + Login string + Name string + } `graphql:"... on User"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 5, query: $query)"` + } `graphql:"... on PullRequest"` + } `graphql:"node(id: $id)"` + Repository struct { + Collaborators struct { + TotalCount int + Nodes []struct { + Login string + Name string + } + } `graphql:"collaborators(first: 5, query: $query)"` + } `graphql:"repository(owner: $owner, name: $name)"` + Organization struct { + Teams struct { + TotalCount int + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 5, query: $query)"` + } `graphql:"organization(login: $owner)"` + } + + variables := map[string]interface{}{ + "id": githubv4.ID(prID), + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActors", &result, variables) + // Handle the case where the owner is not an organization - the query still returns + // partial data (repository, node), so we can continue processing. + if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { + return nil, 0, err + } + + ownerName := repo.RepoOwner() + seen := make(map[string]bool) + var candidates []ReviewerCandidate + + // Add suggested reviewers first (excluding author) + for _, n := range result.Node.PullRequest.SuggestedActors.Nodes { + if n.IsAuthor { + continue + } + var candidate ReviewerCandidate + if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" { + candidate = NewReviewerUser(n.Reviewer.User.Login, n.Reviewer.User.Name) + } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { + candidate = NewReviewerBot(n.Reviewer.Bot.Login) + } else { + continue + } + + login := candidate.Login() + if !seen[login] { + seen[login] = true + candidates = append(candidates, candidate) + } + } + + // Add collaborators (deduped against suggested) + for _, c := range result.Repository.Collaborators.Nodes { + if c.Login == "" { + continue + } + candidate := NewReviewerUser(c.Login, c.Name) + login := candidate.Login() + if !seen[login] { + seen[login] = true + candidates = append(candidates, candidate) + } + } + + // Add teams (will be empty if owner is not an org) + for _, t := range result.Organization.Teams.Nodes { + if t.Slug == "" { + continue + } + candidate := NewReviewerTeam(ownerName, t.Slug) + login := candidate.Login() + if !seen[login] { + seen[login] = true + candidates = append(candidates, candidate) + } + } + + // MoreResults is the sum of collaborators and teams total counts + // (teams will be 0 for personal repos) + moreResults := result.Repository.Collaborators.TotalCount + result.Organization.Teams.TotalCount + + return candidates, moreResults, nil +} + func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { var mutation struct { UpdatePullRequestBranch struct { diff --git a/api/queries_repo.go b/api/queries_repo.go index 4dadcbad3..1e244df58 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1082,8 +1082,10 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) // Expected login for Copilot when retrieved as an Actor // This is returned from assignable actors and issue/pr assigned actors. -// We use this to check if the actor is Copilot. -const CopilotActorLogin = "copilot-swe-agent" +const CopilotAssigneeLogin = "copilot-swe-agent" + +// Expected login for Copilot when retrieved as a Pull Request Reviewer. +const CopilotReviewerLogin = "copilot-pull-request-reviewer" const CopilotActorName = "Copilot" type AssignableActor interface { @@ -1144,7 +1146,7 @@ func NewAssignableBot(id, login string) AssignableBot { } func (b AssignableBot) DisplayName() string { - if b.login == CopilotActorLogin { + if b.login == CopilotAssigneeLogin { return fmt.Sprintf("%s (AI)", CopilotActorName) } return b.Login() diff --git a/api/query_builder.go b/api/query_builder.go index a2432673b..c36fee736 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -94,12 +94,16 @@ var issueClosedByPullRequestsReferences = shortenQuery(` } `) +// prReviewRequests includes ...on Bot to support Copilot as a reviewer on github.com. +// On GHES, Bot is not part of the RequestedReviewer union, but the fragment is +// silently ignored (verified on GHES 3.19). var prReviewRequests = shortenQuery(` reviewRequests(first: 100) { nodes { requestedReviewer { __typename, ...on User{login}, + ...on Bot{login}, ...on Team{ organization{login} name, diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index d9082496a..3753373ba 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -69,13 +69,15 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman - %[1]s@me%[1]s: assign or unassign yourself - %[1]s@copilot%[1]s: assign or unassign Copilot (not supported on GitHub Enterprise Server) - The %[1]s--add-reviewer%[1]s and %[1]s--remove-reviewer%[1]s flags do not support - these special values. + The %[1]s--add-reviewer%[1]s and %[1]s--remove-reviewer%[1]s flags support + the following special value: + - %[1]s@copilot%[1]s: request or remove review from Copilot (not supported on GitHub Enterprise Server) `, "`"), Example: heredoc.Doc(` $ gh pr edit 23 --title "I found a bug" --body "Nothing works" $ gh pr edit 23 --add-label "bug,help wanted" --remove-label "core" $ gh pr edit 23 --add-reviewer monalisa,hubot --remove-reviewer myorg/team-name + $ gh pr edit 23 --add-reviewer "@copilot" $ gh pr edit 23 --add-assignee "@me" --remove-assignee monalisa,hubot $ gh pr edit 23 --add-assignee "@copilot" $ gh pr edit 23 --add-project "Roadmap" --remove-project v1,v2 @@ -188,8 +190,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Editable.Body.Value, "body", "b", "", "Set the new body.") cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file` (use \"-\" to read from standard input)") cmd.Flags().StringVarP(&opts.Editable.Base.Value, "base", "B", "", "Change the base `branch` for this pull request") - cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Add, "add-reviewer", nil, "Add reviewers by their `login`.") - cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Remove, "remove-reviewer", nil, "Remove reviewers by their `login`.") + cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Add, "add-reviewer", nil, "Add reviewers by their `login`. Use \"@copilot\" to request review from Copilot.") + cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Remove, "remove-reviewer", nil, "Remove reviewers by their `login`. Use \"@copilot\" to remove review request from Copilot.") cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself, or \"@copilot\" to assign Copilot.") cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself, or \"@copilot\" to unassign Copilot.") cmd.Flags().StringSliceVar(&opts.Editable.Labels.Add, "add-label", nil, "Add labels by `name`") @@ -265,7 +267,8 @@ func editRun(opts *EditOptions) error { editable.Title.Default = pr.Title editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName - editable.Reviewers.Default = pr.ReviewRequests.Logins() + editable.Reviewers.Default = pr.ReviewRequests.DisplayNames() + editable.Reviewers.DefaultLogins = pr.ReviewRequests.Logins() if issueFeatures.ActorIsAssignable { editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() @@ -294,9 +297,10 @@ func editRun(opts *EditOptions) error { apiClient := api.NewClientFromHTTP(httpClient) // Wire up search functions for assignees and reviewers. - // TODO KW: Wire up reviewer search func if/when it exists. + // Only enabled on github.com (ActorIsAssignable is false on GHES). if issueFeatures.ActorIsAssignable { editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, repo, &editable, pr.ID) + editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) } opts.IO.StartProgressIndicator() @@ -389,6 +393,51 @@ func assigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, editable * return searchFunc } +// reviewerSearchFunc is intended to be an arg for MultiSelectWithSearch +// to return potential reviewer candidates (users, bots, and teams). +// It also updates the editable's metadata for later ID resolution. +func reviewerSearchFunc(apiClient *api.Client, repo ghrepo.Interface, editable *shared.Editable, prID string) func(string) prompter.MultiSelectSearchResult { + searchFunc := func(input string) prompter.MultiSelectSearchResult { + candidates, moreResults, err := api.SuggestedReviewerActors( + apiClient, + repo, + prID, + input) + if err != nil { + return prompter.MultiSelectSearchResult{ + Keys: nil, + Labels: nil, + MoreResults: 0, + Err: err, + } + } + + keys := make([]string, 0, len(candidates)) + labels := make([]string, 0, len(candidates)) + + for _, c := range candidates { + keys = append(keys, c.Login()) + labels = append(labels, c.DisplayName()) + + // Update the teams metadata in the editable struct + // so that updating the PR later can resolve the team ID. + if team, ok := c.(api.ReviewerTeam); ok { + editable.Metadata.Teams = append(editable.Metadata.Teams, api.OrgTeam{ + ID: "", // ID not needed for REST API reviewer mutations + Slug: team.Slug(), + }) + } + } + return prompter.MultiSelectSearchResult{ + Keys: keys, + Labels: labels, + MoreResults: moreResults, + Err: nil, + } + } + return searchFunc +} + func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, number int, editable shared.Editable) error { var wg errgroup.Group wg.Go(func() error { @@ -396,44 +445,82 @@ func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string }) if editable.Reviewers.Edited { wg.Go(func() error { - return updatePullRequestReviews(httpClient, repo, number, editable) + return updatePullRequestReviews(httpClient, repo, id, number, editable) }) } return wg.Wait() } -func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, number int, editable shared.Editable) error { +func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, prID string, number int, editable shared.Editable) error { if !editable.Reviewers.Edited { return nil } + client := api.NewClientFromHTTP(httpClient) + // Rebuild the Value slice from non-interactive flag input. if len(editable.Reviewers.Add) != 0 || len(editable.Reviewers.Remove) != 0 { + add := editable.Reviewers.Add + remove := editable.Reviewers.Remove + + // Replace @copilot with the Copilot reviewer login (only on github.com). + // Also use DefaultLogins (not Default display names) for computing the set. + var defaultLogins []string + if editable.Assignees.ActorAssignees { + copilotReplacer := shared.NewCopilotReviewerReplacer() + add = copilotReplacer.ReplaceSlice(add) + remove = copilotReplacer.ReplaceSlice(remove) + defaultLogins = editable.Reviewers.DefaultLogins + } else { + // On GHES, Default already contains logins (no display name distinction) + defaultLogins = editable.Reviewers.Default + } + s := set.NewStringSet() - s.AddValues(editable.Reviewers.Add) - s.AddValues(editable.Reviewers.Default) - s.RemoveValues(editable.Reviewers.Remove) + s.AddValues(add) + s.AddValues(defaultLogins) + s.RemoveValues(remove) editable.Reviewers.Value = s.ToSlice() } - addUsers, addTeams := partitionUsersAndTeams(editable.Reviewers.Value) + // On github.com, use the new GraphQL mutation which supports bots. + // On GHES, fall back to REST API. + if editable.Assignees.ActorAssignees { + return updatePullRequestReviewsGraphQL(client, repo, prID, editable) + } + return updatePullRequestReviewsREST(client, repo, number, editable) +} - // Reviewers in Default but not in the Value have been removed interactively. +// updatePullRequestReviewsGraphQL uses the RequestReviewsByLogin mutation. +// This mutation replaces the entire reviewer set (union: false). +func updatePullRequestReviewsGraphQL(client *api.Client, repo ghrepo.Interface, prID string, editable shared.Editable) error { + users, bots, teams := partitionReviewersByType(editable.Reviewers.Value) + return api.RequestReviewsByLogin(client, repo, prID, users, bots, teams, false) +} + +// updatePullRequestReviewsREST uses the REST API to add/remove reviewers. +// This is the legacy path for GHES compatibility. +func updatePullRequestReviewsREST(client *api.Client, repo ghrepo.Interface, number int, editable shared.Editable) error { + addUsers, addBots, addTeams := partitionReviewersByType(editable.Reviewers.Value) + // REST API doesn't distinguish bots from users, so we need to combine them. + allAddUsers := append(addUsers, addBots...) + + // Reviewers in Default but not in Value have been removed interactively. var toRemove []string for _, r := range editable.Reviewers.Default { if !slices.Contains(editable.Reviewers.Value, r) { toRemove = append(toRemove, r) } } - removeUsers, removeTeams := partitionUsersAndTeams(toRemove) + removeUsers, removeBots, removeTeams := partitionReviewersByType(toRemove) + allRemoveUsers := append(removeUsers, removeBots...) - client := api.NewClientFromHTTP(httpClient) wg := errgroup.Group{} wg.Go(func() error { - return api.AddPullRequestReviews(client, repo, number, addUsers, addTeams) + return api.AddPullRequestReviews(client, repo, number, allAddUsers, addTeams) }) wg.Go(func() error { - return api.RemovePullRequestReviews(client, repo, number, removeUsers, removeTeams) + return api.RemovePullRequestReviews(client, repo, number, allRemoveUsers, removeTeams) }) return wg.Wait() } @@ -477,16 +564,20 @@ func (e editorRetriever) Retrieve() (string, error) { return cmdutil.DetermineEditor(e.config) } -// partitionUsersAndTeams splits reviewer identifiers into user logins and team slugs. -// Team identifiers are in the form "org/slug"; only the slug portion is returned for teams. -func partitionUsersAndTeams(values []string) (users []string, teams []string) { +// partitionReviewersByType splits reviewer identifiers into users, bots, and teams. +// Team identifiers are in the form "org/slug" and are returned as-is. +// Bot logins (currently only Copilot) are identified and returned separately. +func partitionReviewersByType(values []string) (users []string, bots []string, teams []string) { for _, v := range values { + if v == "" { + continue + } if strings.ContainsRune(v, '/') { - parts := strings.SplitN(v, "/", 2) - if len(parts) == 2 && parts[1] != "" { - teams = append(teams, parts[1]) - } - } else if v != "" { + // Team: org/slug format, pass as-is + teams = append(teams, v) + } else if v == api.CopilotReviewerLogin { + bots = append(bots, v) + } else { users = append(users, v) } } diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 56b868a36..ddb166317 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -150,10 +150,10 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ SelectorArg: "23", Editable: shared.Editable{ - Reviewers: shared.EditableSlice{ + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{ Add: []string{"monalisa", "owner/core"}, Edited: true, - }, + }}, }, }, wantsErr: false, @@ -164,10 +164,10 @@ func TestNewCmdEdit(t *testing.T) { output: EditOptions{ SelectorArg: "23", Editable: shared.Editable{ - Reviewers: shared.EditableSlice{ + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{ Remove: []string{"monalisa", "owner/core"}, Edited: true, - }, + }}, }, }, wantsErr: false, @@ -381,11 +381,11 @@ func Test_editRun(t *testing.T) { Value: "base-branch-name", Edited: true, }, - Reviewers: shared.EditableSlice{ + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{ Add: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot"}, Remove: []string{"dependabot"}, Edited: true, - }, + }}, Assignees: shared.EditableAssignees{ EditableSlice: shared.EditableSlice{ Add: []string{"monalisa", "hubot"}, @@ -413,10 +413,12 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + // Non-interactive with Add/Remove doesn't need reviewers/assignees metadata + // REST API accepts logins and team slugs directly + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) - mockPullRequestAddReviewers(reg) + mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -512,11 +514,11 @@ func Test_editRun(t *testing.T) { Value: "base-branch-name", Edited: true, }, - Reviewers: shared.EditableSlice{ + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{ Default: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"}, Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"}, Edited: true, - }, + }}, Assignees: shared.EditableAssignees{ EditableSlice: shared.EditableSlice{ Add: []string{"monalisa", "hubot"}, @@ -544,9 +546,10 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + // Non-interactive with Remove doesn't need reviewers metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestRemoveReviewers(reg) + mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) mockPullRequestUpdateActorAssignees(reg) mockProjectV2ItemUpdate(reg) @@ -562,17 +565,17 @@ func Test_editRun(t *testing.T) { Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ - Reviewers: shared.EditableSlice{Add: []string{"monalisa", "hubot"}, Edited: true}, + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{Add: []string{"monalisa", "hubot"}, Edited: true}}, }, Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - // reviewers only (users), no team reviewers fetched - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // Non-interactive with Add/Remove doesn't need reviewer metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{}) // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) - mockPullRequestAddReviewers(reg) + mockRequestReviewsByLogin(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, @@ -584,17 +587,17 @@ func Test_editRun(t *testing.T) { Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ - Reviewers: shared.EditableSlice{Add: []string{"monalisa", "OWNER/core"}, Edited: true}, + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{Add: []string{"monalisa", "OWNER/core"}, Edited: true}}, }, Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - // reviewer add includes team but non-interactive Add/Remove provided -> no team fetch - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // Non-interactive with Add/Remove doesn't need reviewer metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{}) // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) - mockPullRequestAddReviewers(reg) + mockRequestReviewsByLogin(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, @@ -611,16 +614,17 @@ func Test_editRun(t *testing.T) { }}}, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ - Reviewers: shared.EditableSlice{Remove: []string{"monalisa", "OWNER/core"}, Edited: true}, + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{Remove: []string{"monalisa", "OWNER/core"}, Edited: true}}, }, Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // Non-interactive with Add/Remove doesn't need reviewer metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{}) // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) - mockPullRequestRemoveReviewers(reg) + mockRequestReviewsByLogin(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, @@ -632,17 +636,17 @@ func Test_editRun(t *testing.T) { Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ - Reviewers: shared.EditableSlice{Add: []string{"monalisa"}, Remove: []string{"hubot"}, Default: []string{"OWNER/core"}, Edited: true}, + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{Add: []string{"monalisa"}, Remove: []string{"hubot"}, Default: []string{"OWNER/core"}, Edited: true}}, }, Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - // reviewers only (users), no team reviewers fetched - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // Non-interactive with Add/Remove doesn't need reviewer metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{}) // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) - mockPullRequestAddReviewers(reg) + mockRequestReviewsByLogin(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, @@ -671,6 +675,16 @@ func Test_editRun(t *testing.T) { e.Body.Value = "new body" e.Reviewers.Value = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external"} e.Assignees.Value = []string{"monalisa", "hubot"} + // Populate metadata to simulate what searchFunc would do during prompting + e.Metadata.AssignableActors = []api.AssignableActor{ + api.NewAssignableBot("HUBOTID", "hubot"), + api.NewAssignableUser("MONAID", "monalisa", "Mona Display Name"), + } + // Populate team metadata for reviewer search + e.Metadata.Teams = []api.OrgTeam{ + {ID: "COREID", Slug: "core"}, + {ID: "EXTERNALID", Slug: "external"}, + } e.Labels.Value = []string{"feature", "TODO", "bug"} e.Labels.Add = []string{"feature", "TODO", "bug"} e.Labels.Remove = []string{"docs"} @@ -683,10 +697,12 @@ func Test_editRun(t *testing.T) { EditorRetriever: testEditorRetriever{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) + // With search functions enabled, we don't fetch reviewers/assignees metadata + // (searchFunc handles dynamic fetching, metadata populated in test mock) + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) - mockPullRequestAddReviewers(reg) + mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -779,6 +795,16 @@ func Test_editRun(t *testing.T) { e.Body.Value = "new body" e.Reviewers.Remove = []string{"monalisa", "hubot", "OWNER/core", "OWNER/external", "dependabot"} e.Assignees.Value = []string{"monalisa", "hubot"} + // Populate metadata to simulate what searchFunc would do during prompting + e.Metadata.AssignableActors = []api.AssignableActor{ + api.NewAssignableBot("HUBOTID", "hubot"), + api.NewAssignableUser("MONAID", "monalisa", "Mona Display Name"), + } + // Populate team metadata for reviewer search + e.Metadata.Teams = []api.OrgTeam{ + {ID: "COREID", Slug: "core"}, + {ID: "EXTERNALID", Slug: "external"}, + } e.Labels.Value = []string{"feature", "TODO", "bug"} e.Labels.Add = []string{"feature", "TODO", "bug"} e.Labels.Remove = []string{"docs"} @@ -791,9 +817,10 @@ func Test_editRun(t *testing.T) { EditorRetriever: testEditorRetriever{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) + // With search functions enabled, we don't fetch reviewers/assignees metadata + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestRemoveReviewers(reg) + mockRequestReviewsByLogin(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -945,6 +972,78 @@ func Test_editRun(t *testing.T) { }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + { + name: "interactive GHES uses legacy reviewer flow without search", + input: &EditOptions{ + Detector: &fd.DisabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{ + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "octocat"}}, + }}, + }, ghrepo.New("OWNER", "REPO")), + Interactive: true, + Surveyor: testSurveyor{ + fieldsToEdit: func(e *shared.Editable) error { + e.Reviewers.Edited = true + return nil + }, + editFields: func(e *shared.Editable, _ string) error { + // Verify GHES uses legacy flow: ReviewerSearchFunc should be nil + require.Nil(t, e.ReviewerSearchFunc) + // Verify options are populated from fetched metadata + require.Contains(t, e.Reviewers.Options, "monalisa") + require.Contains(t, e.Reviewers.Options, "hubot") + require.Contains(t, e.Reviewers.Options, "OWNER/core") + + e.Reviewers.Value = []string{"monalisa", "OWNER/core"} + return nil + }, + }, + Fetcher: testFetcher{}, + EditorRetriever: testEditorRetriever{}, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + // GHES should NOT use the new SuggestedReviewerActors query + reg.Exclude(t, httpmock.GraphQL(`query SuggestedReviewerActors\b`)) + // GHES should use legacy metadata fetch for reviewers (AssignableUsers, not Actors) + reg.Exclude(t, httpmock.GraphQL(`query RepositoryAssignableActors\b`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "monalisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + // GHES should fetch teams for interactive reviewer editing + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "external", "id": "EXTERNALID" }, + { "slug": "core", "id": "COREID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + // Current user fetched for reviewers + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + mockPullRequestUpdate(reg) + mockPullRequestAddReviewers(reg) + mockPullRequestRemoveReviewers(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, { name: "non-interactive projects v1 unsupported doesn't fetch v1 metadata", input: &EditOptions{ @@ -1188,6 +1287,17 @@ func mockPullRequestRemoveReviewers(reg *httpmock.Registry) { httpmock.StringResponse(`{}`)) } +// mockRequestReviewsByLogin mocks the RequestReviewsByLogin GraphQL mutation +// used on github.com when ActorAssignees is enabled. +func mockRequestReviewsByLogin(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } }`, + func(inputs map[string]interface{}) {}), + ) +} + func mockPullRequestUpdateLabels(reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation LabelAdd\b`), diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 7e6e77219..732f16a4b 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -14,8 +14,8 @@ type Editable struct { Title EditableString Body EditableString Base EditableString - Reviewers EditableSlice - ReviewerSearchFunc func(string) ([]string, []string, error) + Reviewers EditableReviewers + ReviewerSearchFunc func(string) prompter.MultiSelectSearchResult Assignees EditableAssignees AssigneeSearchFunc func(string) prompter.MultiSelectSearchResult Labels EditableSlice @@ -49,6 +49,13 @@ type EditableAssignees struct { DefaultLogins []string // For disambiguating actors from display names } +// EditableReviewers is a special case of EditableSlice. +// It tracks both display names (for UI) and logins (for API mutations). +type EditableReviewers struct { + EditableSlice + DefaultLogins []string // Logins for computing add/remove sets +} + // ProjectsV2 mutations require a mapping of an item ID to a project ID. // Keep that map along with standard EditableSlice data. type EditableProjects struct { @@ -268,6 +275,13 @@ func (ea *EditableAssignees) clone() EditableAssignees { } } +func (er *EditableReviewers) clone() EditableReviewers { + return EditableReviewers{ + EditableSlice: er.EditableSlice.clone(), + DefaultLogins: er.DefaultLogins, + } +} + func (ep *EditableProjects) clone() EditableProjects { return EditableProjects{ EditableSlice: ep.EditableSlice.clone(), @@ -299,10 +313,24 @@ func EditFieldsSurvey(p EditPrompter, editable *Editable, editorCommand string) } } if editable.Reviewers.Edited { - editable.Reviewers.Value, err = multiSelectSurvey( - p, "Reviewers", editable.Reviewers.Default, editable.Reviewers.Options) - if err != nil { - return err + if editable.ReviewerSearchFunc != nil { + editable.Reviewers.Options = []string{} + editable.Reviewers.Value, err = p.MultiSelectWithSearch( + "Reviewers", + "Search reviewers", + editable.Reviewers.Default, + // No persistent options - teams are included in search results + []string{}, + editable.ReviewerSearchFunc) + if err != nil { + return err + } + } else { + editable.Reviewers.Value, err = multiSelectSurvey( + p, "Reviewers", editable.Reviewers.Default, editable.Reviewers.Options) + if err != nil { + return err + } } } if editable.Assignees.Edited { @@ -415,16 +443,24 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error { } func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, projectV1Support gh.ProjectsV1Support) error { - // Determine whether to fetch organization teams. + // Determine whether to fetch organization teams and reviewers. // Interactive reviewer editing (Edited true, but no Add/Remove slices) still needs - // team data for selection UI. For non-interactive flows, we never need to fetch teams. + // team data for selection UI. For non-interactive flows, we never need to fetch teams + // as the REST API accepts team slugs directly. + // If we have a search func, we don't need to fetch teams/reviewers since we + // assume that will be done dynamically in the prompting flow. teamReviewers := false + fetchReviewers := false if editable.Reviewers.Edited { // This is likely an interactive flow since edited is set but no mutations to - // Add/Remove slices, so we need to load the teams. - if len(editable.Reviewers.Add) == 0 && len(editable.Reviewers.Remove) == 0 { + // Add/Remove slices, so we need to load the teams and reviewers. + // However, if we have a search func, skip fetching as it will be done dynamically. + if len(editable.Reviewers.Add) == 0 && len(editable.Reviewers.Remove) == 0 && editable.ReviewerSearchFunc == nil { teamReviewers = true + fetchReviewers = true } + // Note: Non-interactive flows (with Add/Remove) don't need to fetch reviewers/teams + // because the REST API accepts logins and team slugs directly. } fetchAssignees := false @@ -444,7 +480,7 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, } input := api.RepoMetadataInput{ - Reviewers: editable.Reviewers.Edited, + Reviewers: fetchReviewers, TeamReviewers: teamReviewers, Assignees: fetchAssignees, ActorAssignees: editable.Assignees.ActorAssignees, diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index e0ea9f105..784b68cf9 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -295,11 +295,24 @@ func (r *MeReplacer) ReplaceSlice(handles []string) ([]string, error) { // Login is generally needed for API calls; name is used when launching web browser. type CopilotReplacer struct { returnLogin bool + // copilotLogin is the login to use when replacing @copilot. + // Different Copilot features use different bot logins. + copilotLogin string } +// NewCopilotReplacer creates a replacer for assignee @copilot references. func NewCopilotReplacer(returnLogin bool) *CopilotReplacer { return &CopilotReplacer{ - returnLogin: returnLogin, + returnLogin: returnLogin, + copilotLogin: api.CopilotAssigneeLogin, + } +} + +// NewCopilotReviewerReplacer creates a replacer for reviewer @copilot references. +func NewCopilotReviewerReplacer() *CopilotReplacer { + return &CopilotReplacer{ + returnLogin: true, + copilotLogin: api.CopilotReviewerLogin, } } @@ -308,7 +321,7 @@ func (r *CopilotReplacer) replace(handle string) string { return handle } if r.returnLogin { - return api.CopilotActorLogin + return r.copilotLogin } return api.CopilotActorName } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 024965630..ddb7a1b2f 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -334,6 +334,37 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { } } +func TestCopilotReviewerReplacer_ReplaceSlice(t *testing.T) { + tests := []struct { + name string + handles []string + want []string + }{ + { + name: "replaces @copilot with reviewer login", + handles: []string{"monalisa", "@copilot", "hubot"}, + want: []string{"monalisa", "copilot-pull-request-reviewer", "hubot"}, + }, + { + name: "handles @copilot case-insensitively", + handles: []string{"@Copilot", "user", "@CoPiLoT"}, + want: []string{"copilot-pull-request-reviewer", "user", "copilot-pull-request-reviewer"}, + }, + { + name: "handles no @copilot mentions", + handles: []string{"monalisa", "user", "hubot"}, + want: []string{"monalisa", "user", "hubot"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := NewCopilotReviewerReplacer() + got := r.ReplaceSlice(tt.handles) + require.Equal(t, tt.want, got) + }) + } +} + func Test_QueryHasStateClause(t *testing.T) { tests := []struct { searchQuery string From 846d6619b7913c905eb82d5b63000173fbf37e0e Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 28 Jan 2026 10:50:54 -0700 Subject: [PATCH 02/13] Remove redundant comment --- pkg/cmd/pr/edit/edit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 3753373ba..b91798dae 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -297,7 +297,7 @@ func editRun(opts *EditOptions) error { apiClient := api.NewClientFromHTTP(httpClient) // Wire up search functions for assignees and reviewers. - // Only enabled on github.com (ActorIsAssignable is false on GHES). + // Only enabled on github.com. if issueFeatures.ActorIsAssignable { editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, repo, &editable, pr.ID) editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) From 7f8ca2ca812a3aa678a6e2a74ec1a29e70e99249 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 29 Jan 2026 10:23:18 -0700 Subject: [PATCH 03/13] Fix return proper slug only for ReviewerTeam --- api/queries_pr.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index f006c529c..a84582c42 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -939,26 +939,26 @@ func (b ReviewerBot) DisplayName() string { func (r ReviewerBot) sealedReviewerCandidate() {} // ReviewerTeam is a team that can review a pull request. -// The slug is stored as "org/team-slug" format. type ReviewerTeam struct { - slug string + org string + teamSlug string } -// NewReviewerTeam creates a new ReviewerTeam with the full "org/slug" format. +// NewReviewerTeam creates a new ReviewerTeam. func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { - return ReviewerTeam{slug: fmt.Sprintf("%s/%s", orgName, teamSlug)} + return ReviewerTeam{org: orgName, teamSlug: teamSlug} } func (r ReviewerTeam) DisplayName() string { - return r.slug + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) } func (r ReviewerTeam) Login() string { - return r.slug + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) } func (r ReviewerTeam) Slug() string { - return r.slug + return r.teamSlug } func (r ReviewerTeam) sealedReviewerCandidate() {} From d643d5386e25c7f7c9d4f716a1c4356262a251f6 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 29 Jan 2026 10:37:11 -0700 Subject: [PATCH 04/13] fix(pr edit): send empty slices to clear reviewers in replace mode Always send explicit lists for userLogins, botLogins, and teamSlugs in RequestReviewsByLogin mutation, even when empty. Previously, empty slices were omitted due to omitempty JSON behavior and len > 0 checks, which prevented clearing all reviewers when using replace mode. Empty slices are harmless no-ops in union mode, so we now send them unconditionally for simpler logic. Add test to verify the mutation receives empty slices when all reviewers are removed. --- api/queries_pr.go | 32 +++++++++------------ pkg/cmd/pr/edit/edit_test.go | 55 ++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 19 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index a84582c42..aa584cf5d 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -772,30 +772,24 @@ func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, u Union: githubv4.Boolean(union), } - if len(userLogins) > 0 { - logins := make([]githubv4.String, len(userLogins)) - for i, l := range userLogins { - logins[i] = githubv4.String(l) - } - input.UserLogins = &logins + userLoginValues := make([]githubv4.String, len(userLogins)) + for i, l := range userLogins { + userLoginValues[i] = githubv4.String(l) } + input.UserLogins = &userLoginValues - if len(botLogins) > 0 { - logins := make([]githubv4.String, len(botLogins)) - for i, l := range botLogins { - // Bot logins require the [bot] suffix for the mutation - logins[i] = githubv4.String(l + "[bot]") - } - input.BotLogins = &logins + botLoginValues := make([]githubv4.String, len(botLogins)) + for i, l := range botLogins { + // Bot logins require the [bot] suffix for the mutation + botLoginValues[i] = githubv4.String(l + "[bot]") } + input.BotLogins = &botLoginValues - if len(teamSlugs) > 0 { - slugs := make([]githubv4.String, len(teamSlugs)) - for i, s := range teamSlugs { - slugs[i] = githubv4.String(s) - } - input.TeamSlugs = &slugs + teamSlugValues := make([]githubv4.String, len(teamSlugs)) + for i, s := range teamSlugs { + teamSlugValues[i] = githubv4.String(s) } + input.TeamSlugs = &teamSlugValues variables := map[string]interface{}{ "input": input, diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index ddb166317..e38fac3ca 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -556,6 +556,61 @@ func Test_editRun(t *testing.T) { }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + { + name: "remove all reviewers sends empty slices to mutation", + input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + ReviewRequests: api.ReviewRequests{ + Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{ + { + RequestedReviewer: api.RequestedReviewer{ + TypeName: "Team", + Slug: "core", + Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}, + }, + }, + { + RequestedReviewer: api.RequestedReviewer{ + TypeName: "User", + Login: "monalisa", + }, + }, + }, + }, + }, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Reviewers: shared.EditableReviewers{EditableSlice: shared.EditableSlice{ + Default: []string{"OWNER/core", "monalisa"}, + Remove: []string{"OWNER/core", "monalisa"}, + Edited: true, + }}, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + mockRepoMetadata(reg, mockRepoMetadataOptions{}) + mockPullRequestUpdate(reg) + reg.Register( + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } }`, + func(inputs map[string]interface{}) { + // Verify that empty slices are sent to properly clear all reviewer types + require.Equal(t, []interface{}{}, inputs["userLogins"], "userLogins should be an empty slice") + require.Equal(t, []interface{}{}, inputs["botLogins"], "botLogins should be an empty slice") + require.Equal(t, []interface{}{}, inputs["teamSlugs"], "teamSlugs should be an empty slice") + require.Equal(t, false, inputs["union"], "union should be false for replace mode") + }), + ) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, // Conditional team fetching cases { name: "non-interactive add only user reviewers skips team fetch", From 2d191e5ba0304934fa22ad376d9553ee45021fa0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 29 Jan 2026 11:30:08 -0700 Subject: [PATCH 05/13] Implement cascading quota for reviewer suggestions Each source (suggestions, collaborators, teams) has base quota of 5. Unfilled slots cascade to later sources, allowing up to 15 total. Adds unit tests with HTTP mocks. --- api/queries_pr.go | 74 +++++++++------ api/queries_pr_test.go | 199 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 247 insertions(+), 26 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index aa584cf5d..fbed0611f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -958,13 +958,16 @@ func (r ReviewerTeam) Slug() string { func (r ReviewerTeam) sealedReviewerCandidate() {} // SuggestedReviewerActors fetches suggested reviewers for a pull request. -// It combines results from: -// - suggestedReviewerActors (10 max) - suggested based on activity -// - repository collaborators (10 max) - all collaborators -// - organization teams (10 max for org repos) - all teams (if owner is an org) -// Results are returned in that order with duplicates removed. +// It combines results from three sources using a cascading quota system: +// - suggestedReviewerActors - suggested based on PR activity (base quota: 5) +// - repository collaborators - all collaborators (base quota: 5 + unfilled from suggestions) +// - organization teams - all teams for org repos (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 15 total candidates, with each source filling any +// unfilled quota from the previous source. Results are deduplicated. // Returns the candidates, a MoreResults count, and an error. func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) { + // Fetch 10 from each source to allow cascading quota to fill from available results. // Use a single query that includes organization.teams - if the owner is not an org, // we'll get a "Could not resolve to an Organization" error which we handle gracefully. type responseData struct { @@ -985,7 +988,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } `graphql:"... on Bot"` } } - } `graphql:"suggestedReviewerActors(first: 5, query: $query)"` + } `graphql:"suggestedReviewerActors(first: 10, query: $query)"` } `graphql:"... on PullRequest"` } `graphql:"node(id: $id)"` Repository struct { @@ -995,7 +998,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, Login string Name string } - } `graphql:"collaborators(first: 5, query: $query)"` + } `graphql:"collaborators(first: 10, query: $query)"` } `graphql:"repository(owner: $owner, name: $name)"` Organization struct { Teams struct { @@ -1003,7 +1006,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, Nodes []struct { Slug string } - } `graphql:"teams(first: 5, query: $query)"` + } `graphql:"teams(first: 10, query: $query)"` } `graphql:"organization(login: $owner)"` } @@ -1022,54 +1025,73 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, return nil, 0, err } - ownerName := repo.RepoOwner() + // Build candidates using cascading quota logic: + // Each source has a base quota of 5, plus any unfilled quota from previous sources. + // This ensures we show up to 15 total candidates, filling gaps when earlier sources have fewer. seen := make(map[string]bool) var candidates []ReviewerCandidate + const baseQuota = 5 - // Add suggested reviewers first (excluding author) + // Suggested reviewers (excluding author) + suggestionsAdded := 0 for _, n := range result.Node.PullRequest.SuggestedActors.Nodes { + if suggestionsAdded >= baseQuota { + break + } if n.IsAuthor { continue } var candidate ReviewerCandidate + var login string if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" { - candidate = NewReviewerUser(n.Reviewer.User.Login, n.Reviewer.User.Name) + login = n.Reviewer.User.Login + candidate = NewReviewerUser(login, n.Reviewer.User.Name) } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { - candidate = NewReviewerBot(n.Reviewer.Bot.Login) + login = n.Reviewer.Bot.Login + candidate = NewReviewerBot(login) } else { continue } - - login := candidate.Login() if !seen[login] { seen[login] = true candidates = append(candidates, candidate) + suggestionsAdded++ } } - // Add collaborators (deduped against suggested) + // Collaborators: quota = base + unfilled from suggestions + collaboratorsQuota := baseQuota + (baseQuota - suggestionsAdded) + collaboratorsAdded := 0 for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= collaboratorsQuota { + break + } if c.Login == "" { continue } - candidate := NewReviewerUser(c.Login, c.Name) - login := candidate.Login() - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ } } - // Add teams (will be empty if owner is not an org) + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() for _, t := range result.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } if t.Slug == "" { continue } - candidate := NewReviewerTeam(ownerName, t.Slug) - login := candidate.Login() - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ } } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 0e2646048..cea2c201b 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -2,6 +2,8 @@ package api import ( "encoding/json" + "fmt" + "strings" "testing" "github.com/cli/cli/v2/internal/ghrepo" @@ -136,3 +138,200 @@ func Test_Logins(t *testing.T) { }) } } + +// mockReviewerResponse generates a GraphQL response for SuggestedReviewerActors tests. +// It creates suggestions (s1, s2...), collaborators (c1, c2...), and teams (team1, team2...). +// totalCollabs and totalTeams set the TotalCount fields (for "more results" calculation). +func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams int) string { + var suggestionNodes, collabNodes, teamNodes []string + + for i := 1; i <= suggestions; i++ { + suggestionNodes = append(suggestionNodes, + fmt.Sprintf(`{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s%d", "name": "S%d"}}`, i, i)) + } + for i := 1; i <= collabs; i++ { + collabNodes = append(collabNodes, + fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i)) + } + for i := 1; i <= teams; i++ { + teamNodes = append(teamNodes, + fmt.Sprintf(`{"slug": "team%d"}`, i)) + } + + return fmt.Sprintf(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [%s]}}, + "repository": {"collaborators": {"totalCount": %d, "nodes": [%s]}}, + "organization": {"teams": {"totalCount": %d, "nodes": [%s]}} + } + }`, strings.Join(suggestionNodes, ","), totalCollabs, strings.Join(collabNodes, ","), + totalTeams, strings.Join(teamNodes, ",")) +} + +func TestSuggestedReviewerActors(t *testing.T) { + tests := []struct { + name string + httpStubs func(*httpmock.Registry) + expectedCount int + expectedLogins []string + expectedMore int + expectError bool + }{ + { + name: "all sources plentiful - 5 each from cascading quota", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(6, 6, 6, 20, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "s3", "s4", "s5", "c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 30, + }, + { + name: "few suggestions - collaborators fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(2, 10, 6, 50, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "c4", "c5", "c6", "c7", "c8", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 60, + }, + { + name: "few suggestions and collaborators - teams fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(2, 3, 10, 3, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 13, + }, + { + name: "no suggestions or collaborators - teams only", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(0, 0, 10, 0, 10))) + }, + expectedCount: 10, // max 15, but only 10 teams available + expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 10, + }, + { + name: "author excluded from suggestions", + httpStubs: func(reg *httpmock.Registry) { + // Custom response with author flag + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": true, "reviewer": {"__typename": "User", "login": "author", "name": "Author"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}} + ]}}, + "repository": {"collaborators": {"totalCount": 5, "nodes": [{"login": "c1", "name": "C1"}]}}, + "organization": {"teams": {"totalCount": 3, "nodes": [{"slug": "team1"}]}} + } + }`)) + }, + expectedCount: 4, + expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"}, + expectedMore: 8, + }, + { + name: "deduplication across sources", + httpStubs: func(reg *httpmock.Registry) { + // Custom response with duplicate user + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} + ]}}, + "repository": {"collaborators": {"totalCount": 10, "nodes": [ + {"login": "shareduser", "name": "Shared"}, + {"login": "c1", "name": "C1"} + ]}}, + "organization": {"teams": {"totalCount": 5, "nodes": [{"slug": "team1"}]}} + } + }`)) + }, + expectedCount: 3, + expectedLogins: []string{"shareduser", "c1", "OWNER/team1"}, + expectedMore: 15, + }, + { + name: "personal repo - no organization teams", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": {"collaborators": {"totalCount": 3, "nodes": [{"login": "c1", "name": "C1"}]}}, + "organization": null + }, + "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"s1", "c1"}, + expectedMore: 3, + }, + { + name: "bot reviewer included", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": {"collaborators": {"totalCount": 5, "nodes": []}}, + "organization": {"teams": {"totalCount": 0, "nodes": []}} + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"copilot-pull-request-reviewer", "s1"}, + expectedMore: 5, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + + client := newTestClient(reg) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + candidates, moreResults, err := SuggestedReviewerActors(client, repo, "PR_123", "") + if tt.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch") + assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch") + + logins := make([]string, len(candidates)) + for i, c := range candidates { + logins[i] = c.Login() + } + assert.Equal(t, tt.expectedLogins, logins) + }) + } +} From 484526da77ae13c25dcd7d0048e3edd6ac4364a2 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 29 Jan 2026 12:45:58 -0700 Subject: [PATCH 06/13] Include name in reviewer display for existing review requests Fetch name field in reviewRequests GraphQL query and show as 'login (Name)'. --- api/queries_pr.go | 5 ++++- api/query_builder.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index fbed0611f..aaa559e34 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -326,7 +326,7 @@ func (r RequestedReviewer) LoginOrSlug() string { // DisplayName returns a user-friendly name for the reviewer. // For Copilot bot, returns "Copilot (AI)". For teams, returns "org/slug". -// For users, returns login (could be extended to show name if available). +// For users, returns "login (Name)" if name is available, otherwise just login. func (r RequestedReviewer) DisplayName() string { if r.TypeName == teamTypeName { return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) @@ -334,6 +334,9 @@ func (r RequestedReviewer) DisplayName() string { if r.TypeName == "Bot" && r.Login == CopilotReviewerLogin { return "Copilot (AI)" } + if r.Name != "" { + return fmt.Sprintf("%s (%s)", r.Login, r.Name) + } return r.Login } diff --git a/api/query_builder.go b/api/query_builder.go index c36fee736..766c2b4aa 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -102,7 +102,7 @@ var prReviewRequests = shortenQuery(` nodes { requestedReviewer { __typename, - ...on User{login}, + ...on User{login,name}, ...on Bot{login}, ...on Team{ organization{login} From c8b14098033d1cce2581410df5f58e2dd2022d85 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 29 Jan 2026 12:54:04 -0700 Subject: [PATCH 07/13] Use unfiltered totalCount for reviewer 'more results' display Query aliased fields without search filter to get stable counts. --- api/queries_pr.go | 18 +++++++------ api/queries_pr_test.go | 57 +++++++++++++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 22 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index aaa559e34..bb5438fb3 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -973,6 +973,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, // Fetch 10 from each source to allow cascading quota to fill from available results. // Use a single query that includes organization.teams - if the owner is not an org, // we'll get a "Could not resolve to an Organization" error which we handle gracefully. + // We also fetch unfiltered total counts via aliases for the "X more" display. type responseData struct { Node struct { PullRequest struct { @@ -996,20 +997,24 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } `graphql:"node(id: $id)"` Repository struct { Collaborators struct { - TotalCount int - Nodes []struct { + Nodes []struct { Login string Name string } } `graphql:"collaborators(first: 10, query: $query)"` + CollaboratorsTotalCount struct { + TotalCount int + } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` } `graphql:"repository(owner: $owner, name: $name)"` Organization struct { Teams struct { - TotalCount int - Nodes []struct { + Nodes []struct { Slug string } } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` } `graphql:"organization(login: $owner)"` } @@ -1098,9 +1103,8 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } } - // MoreResults is the sum of collaborators and teams total counts - // (teams will be 0 for personal repos) - moreResults := result.Repository.Collaborators.TotalCount + result.Organization.Teams.TotalCount + // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Organization.TeamsTotalCount.TotalCount return candidates, moreResults, nil } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index cea2c201b..69dc505ca 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -141,7 +141,7 @@ func Test_Logins(t *testing.T) { // mockReviewerResponse generates a GraphQL response for SuggestedReviewerActors tests. // It creates suggestions (s1, s2...), collaborators (c1, c2...), and teams (team1, team2...). -// totalCollabs and totalTeams set the TotalCount fields (for "more results" calculation). +// totalCollabs and totalTeams set the unfiltered TotalCount fields (for "more results" calculation). func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams int) string { var suggestionNodes, collabNodes, teamNodes []string @@ -161,11 +161,17 @@ func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams return fmt.Sprintf(`{ "data": { "node": {"suggestedReviewerActors": {"nodes": [%s]}}, - "repository": {"collaborators": {"totalCount": %d, "nodes": [%s]}}, - "organization": {"teams": {"totalCount": %d, "nodes": [%s]}} + "repository": { + "collaborators": {"nodes": [%s]}, + "collaboratorsTotalCount": {"totalCount": %d} + }, + "organization": { + "teams": {"nodes": [%s]}, + "teamsTotalCount": {"totalCount": %d} + } } - }`, strings.Join(suggestionNodes, ","), totalCollabs, strings.Join(collabNodes, ","), - totalTeams, strings.Join(teamNodes, ",")) + }`, strings.Join(suggestionNodes, ","), strings.Join(collabNodes, ","), totalCollabs, + strings.Join(teamNodes, ","), totalTeams) } func TestSuggestedReviewerActors(t *testing.T) { @@ -234,8 +240,14 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}} ]}}, - "repository": {"collaborators": {"totalCount": 5, "nodes": [{"login": "c1", "name": "C1"}]}}, - "organization": {"teams": {"totalCount": 3, "nodes": [{"slug": "team1"}]}} + "repository": { + "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, + "collaboratorsTotalCount": {"totalCount": 5} + }, + "organization": { + "teams": {"nodes": [{"slug": "team1"}]}, + "teamsTotalCount": {"totalCount": 3} + } } }`)) }, @@ -254,11 +266,17 @@ func TestSuggestedReviewerActors(t *testing.T) { "node": {"suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} ]}}, - "repository": {"collaborators": {"totalCount": 10, "nodes": [ - {"login": "shareduser", "name": "Shared"}, - {"login": "c1", "name": "C1"} - ]}}, - "organization": {"teams": {"totalCount": 5, "nodes": [{"slug": "team1"}]}} + "repository": { + "collaborators": {"nodes": [ + {"login": "shareduser", "name": "Shared"}, + {"login": "c1", "name": "C1"} + ]}, + "collaboratorsTotalCount": {"totalCount": 10} + }, + "organization": { + "teams": {"nodes": [{"slug": "team1"}]}, + "teamsTotalCount": {"totalCount": 5} + } } }`)) }, @@ -276,7 +294,10 @@ func TestSuggestedReviewerActors(t *testing.T) { "node": {"suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, - "repository": {"collaborators": {"totalCount": 3, "nodes": [{"login": "c1", "name": "C1"}]}}, + "repository": { + "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, + "collaboratorsTotalCount": {"totalCount": 3} + }, "organization": null }, "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] @@ -297,8 +318,14 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, - "repository": {"collaborators": {"totalCount": 5, "nodes": []}}, - "organization": {"teams": {"totalCount": 0, "nodes": []}} + "repository": { + "collaborators": {"nodes": []}, + "collaboratorsTotalCount": {"totalCount": 5} + }, + "organization": { + "teams": {"nodes": []}, + "teamsTotalCount": {"totalCount": 0} + } } }`)) }, From 7303c4448319df392085be667d44074c2ac2659b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:17:25 -0700 Subject: [PATCH 08/13] Clarify Copilot assignee comment Reword the comment for CopilotAssigneeLogin to indicate it refers to Copilot when retrieved as an assignee. This updates wording from the previous 'Actor/assignable actors' phrasing for clarity; no code behavior changed. --- api/queries_repo.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 1e244df58..d8ffa191d 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1080,8 +1080,7 @@ func RepoProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) return projects, nil } -// Expected login for Copilot when retrieved as an Actor -// This is returned from assignable actors and issue/pr assigned actors. +// Expected login for Copilot when retrieved as an assignee const CopilotAssigneeLogin = "copilot-swe-agent" // Expected login for Copilot when retrieved as a Pull Request Reviewer. From 4569aae0e4a3a254509602e1e7d64d793c1cf199 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:20:30 -0700 Subject: [PATCH 09/13] Clarify EditableReviewers comment Remove a redundant struct-level comment and update the DefaultLogins field comment in pkg/cmd/pr/shared/editable.go to more accurately describe its purpose: used to disambiguate actors from display names rather than to compute add/remove sets. --- pkg/cmd/pr/shared/editable.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 732f16a4b..3e92444be 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -50,10 +50,9 @@ type EditableAssignees struct { } // EditableReviewers is a special case of EditableSlice. -// It tracks both display names (for UI) and logins (for API mutations). type EditableReviewers struct { EditableSlice - DefaultLogins []string // Logins for computing add/remove sets + DefaultLogins []string // For disambiguating actors from display names } // ProjectsV2 mutations require a mapping of an item ID to a project ID. From aac223ab71e96f366c8fc8829c39dbbd71554e97 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:29:11 -0700 Subject: [PATCH 10/13] Clarify assignee/reviewer fetching behavior Add clarifying comment in pkg/cmd/pr/edit/edit.go to note that missing assignee/reviewer search functions trigger a downstream fallback to legacy fetching. In pkg/cmd/pr/shared/editable.go remove a redundant line and add a TODO urging migration of non-interactive assignee updates to use the new logins input with ReplaceActorsForAssignable to avoid unnecessary fetching. These are comment and doc changes to clarify intent and future improvements. --- pkg/cmd/pr/edit/edit.go | 3 ++- pkg/cmd/pr/shared/editable.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index b91798dae..d7ae05236 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -297,7 +297,8 @@ func editRun(opts *EditOptions) error { apiClient := api.NewClientFromHTTP(httpClient) // Wire up search functions for assignees and reviewers. - // Only enabled on github.com. + // When these aren't wired up, it triggers a downstream fallback + // to legacy reviewer/assignee fetching. if issueFeatures.ActorIsAssignable { editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, repo, &editable, pr.ID) editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 3e92444be..9c6629f5e 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -465,7 +465,6 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, fetchAssignees := false if editable.Assignees.Edited { // Similar as above, this is likely an interactive flow if no Add/Remove slices are set. - // The addition here is that we also check for an assignee search func. // If we have a search func, we don't need to fetch assignees since we // assume that will be done dynamically in the prompting flow. if len(editable.Assignees.Add) == 0 && len(editable.Assignees.Remove) == 0 && editable.AssigneeSearchFunc == nil { @@ -473,6 +472,8 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, } // However, if we have Add/Remove operations (non-interactive flow), // we do need to fetch the assignees. + // TODO: KW noninteractive assignees need to migrate to directly use + // new logins input with ReplaceActorsForAssignable to prevent fetching. if len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0 { fetchAssignees = true } From a9a0486c7082c5898e2bd79a860e6b17d57f4d95 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:33:22 -0700 Subject: [PATCH 11/13] Clarify comment on reviewer API behavior Update comment in FetchOptions to specify that the APIs used for both GHES and GitHub.com accept user logins and team slugs directly, clarifying why non-interactive flows with Add/Remove don't need to fetch reviewers/teams. Comment-only change; no functional modifications. --- pkg/cmd/pr/shared/editable.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 9c6629f5e..31b253d9b 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -459,7 +459,7 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, fetchReviewers = true } // Note: Non-interactive flows (with Add/Remove) don't need to fetch reviewers/teams - // because the REST API accepts logins and team slugs directly. + // because the APIs in use for both GHES and GitHub.com accept user logins and team slugs directly. } fetchAssignees := false From ebf932a043d26fd8b6a22ee7c2ed6dfa12b0be17 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 4 Feb 2026 15:04:20 -0700 Subject: [PATCH 12/13] Address PR review comments Address PR review comments: code consistency and DRY improvements - Add botTypeName const for consistency with teamTypeName - Create extractTeamSlugs helper using strings.SplitN to simplify team slug extraction logic - Replace duplicate code in AddPullRequestReviews and RemovePullRequestReviews with extractTeamSlugs helper - Fix ClientMutationId naming with explicit graphql tag for consistency with other mutations in the codebase --- api/queries_pr.go | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index bb5438fb3..90a9567b0 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -317,6 +317,9 @@ type RequestedReviewer struct { } `json:"organization"` } +const teamTypeName = "Team" +const botTypeName = "Bot" + func (r RequestedReviewer) LoginOrSlug() string { if r.TypeName == teamTypeName { return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) @@ -331,7 +334,7 @@ func (r RequestedReviewer) DisplayName() string { if r.TypeName == teamTypeName { return fmt.Sprintf("%s/%s", r.Organization.Login, r.Slug) } - if r.TypeName == "Bot" && r.Login == CopilotReviewerLogin { + if r.TypeName == botTypeName && r.Login == CopilotReviewerLogin { return "Copilot (AI)" } if r.Name != "" { @@ -340,8 +343,6 @@ func (r RequestedReviewer) DisplayName() string { return r.Login } -const teamTypeName = "Team" - func (r ReviewRequests) Logins() []string { logins := make([]string, len(r.Nodes)) for i, r := range r.Nodes { @@ -657,6 +658,20 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } +// extractTeamSlugs extracts just the slug portion from team identifiers. +// Team identifiers can be in "org/slug" format; this returns just the slug. +func extractTeamSlugs(teams []string) []string { + slugs := make([]string, 0, len(teams)) + for _, t := range teams { + if t == "" { + continue + } + s := strings.SplitN(t, "/", 2) + slugs = append(slugs, s[len(s)-1]) + } + return slugs +} + // AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. // Team identifiers can be in "org/slug" format. func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { @@ -669,16 +684,6 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users = []string{} } - // Extract just the slug from org/slug format - teamSlugs := make([]string, 0, len(teams)) - for _, t := range teams { - if idx := strings.Index(t, "/"); idx >= 0 { - teamSlugs = append(teamSlugs, t[idx+1:]) - } else if t != "" { - teamSlugs = append(teamSlugs, t) - } - } - path := fmt.Sprintf( "repos/%s/%s/pulls/%d/requested_reviewers", url.PathEscape(repo.RepoOwner()), @@ -690,7 +695,7 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, TeamReviewers []string `json:"team_reviewers"` }{ Reviewers: users, - TeamReviewers: teamSlugs, + TeamReviewers: extractTeamSlugs(teams), } buf := &bytes.Buffer{} if err := json.NewEncoder(buf).Encode(body); err != nil { @@ -712,16 +717,6 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in users = []string{} } - // Extract just the slug from org/slug format - teamSlugs := make([]string, 0, len(teams)) - for _, t := range teams { - if idx := strings.Index(t, "/"); idx >= 0 { - teamSlugs = append(teamSlugs, t[idx+1:]) - } else if t != "" { - teamSlugs = append(teamSlugs, t) - } - } - path := fmt.Sprintf( "repos/%s/%s/pulls/%d/requested_reviewers", url.PathEscape(repo.RepoOwner()), @@ -733,7 +728,7 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in TeamReviewers []string `json:"team_reviewers"` }{ Reviewers: users, - TeamReviewers: teamSlugs, + TeamReviewers: extractTeamSlugs(teams), } buf := &bytes.Buffer{} if err := json.NewEncoder(buf).Encode(body); err != nil { @@ -758,7 +753,7 @@ func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, u var mutation struct { RequestReviewsByLogin struct { - ClientMutationID string + ClientMutationId string `graphql:"clientMutationId"` } `graphql:"requestReviewsByLogin(input: $input)"` } From c0febc1ac864a8beca58f19b59ed41ebd9e0b7ac Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 4 Feb 2026 15:18:49 -0700 Subject: [PATCH 13/13] Add toGitHubV4Strings helper to reduce code duplication --- api/queries_pr.go | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 90a9567b0..1bc5ddb55 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -672,6 +672,16 @@ func extractTeamSlugs(teams []string) []string { return slugs } +// toGitHubV4Strings converts a string slice to a githubv4.String slice, +// optionally appending a suffix to each element. +func toGitHubV4Strings(strs []string, suffix string) []githubv4.String { + result := make([]githubv4.String, len(strs)) + for i, s := range strs { + result[i] = githubv4.String(s + suffix) + } + return result +} + // AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. // Team identifiers can be in "org/slug" format. func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { @@ -770,23 +780,14 @@ func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, u Union: githubv4.Boolean(union), } - userLoginValues := make([]githubv4.String, len(userLogins)) - for i, l := range userLogins { - userLoginValues[i] = githubv4.String(l) - } + userLoginValues := toGitHubV4Strings(userLogins, "") input.UserLogins = &userLoginValues - botLoginValues := make([]githubv4.String, len(botLogins)) - for i, l := range botLogins { - // Bot logins require the [bot] suffix for the mutation - botLoginValues[i] = githubv4.String(l + "[bot]") - } + // Bot logins require the [bot] suffix for the mutation + botLoginValues := toGitHubV4Strings(botLogins, "[bot]") input.BotLogins = &botLoginValues - teamSlugValues := make([]githubv4.String, len(teamSlugs)) - for i, s := range teamSlugs { - teamSlugValues[i] = githubv4.String(s) - } + teamSlugValues := toGitHubV4Strings(teamSlugs, "") input.TeamSlugs = &teamSlugValues variables := map[string]interface{}{