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