From 9904f7d1b9070bc22a1bccca0aa2c7ed150f2ec0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 30 Jan 2026 15:50:56 -0700 Subject: [PATCH 01/28] gh pr create: CCR and multiselectwithsearch --- api/queries_pr.go | 170 +++++++- api/queries_pr_test.go | 167 ++++++++ pkg/cmd/issue/create/create.go | 2 +- pkg/cmd/pr/create/create.go | 28 +- pkg/cmd/pr/create/create_test.go | 672 ++++++++++++++++++------------- pkg/cmd/pr/shared/params.go | 42 +- pkg/cmd/pr/shared/state.go | 1 + pkg/cmd/pr/shared/survey.go | 16 +- pkg/cmd/pr/shared/survey_test.go | 8 +- 9 files changed, 793 insertions(+), 313 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index bb5438fb3c9..632e7dd7522 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -615,29 +615,41 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // reviewers are requested in yet another additional mutation - reviewParams := make(map[string]interface{}) - if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { - reviewParams["userIds"] = ids - } - if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { - reviewParams["teamIds"] = ids - } + // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation + userLogins, hasUserLogins := params["userReviewerLogins"].([]string) + teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) - //TODO: How much work to extract this into own method and use for create and edit? - if len(reviewParams) > 0 { - reviewQuery := ` + if hasUserLogins || hasTeamSlugs { + // Use login-based mutation (RequestReviewsByLogin) for github.com + err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, nil, teamSlugs, true) + if err != nil { + return pr, err + } + } else { + // Use ID-based mutation (requestReviews) for GHES compatibility + reviewParams := make(map[string]interface{}) + if ids, ok := params["userReviewerIds"]; ok && !isBlank(ids) { + reviewParams["userIds"] = ids + } + if ids, ok := params["teamReviewerIds"]; ok && !isBlank(ids) { + reviewParams["teamIds"] = ids + } + + //TODO: How much work to extract this into own method and use for create and edit? + if len(reviewParams) > 0 { + reviewQuery := ` mutation PullRequestCreateRequestReviews($input: RequestReviewsInput!) { requestReviews(input: $input) { clientMutationId } }` - reviewParams["pullRequestId"] = pr.ID - reviewParams["union"] = true - variables := map[string]interface{}{ - "input": reviewParams, - } - err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) - if err != nil { - return pr, err + reviewParams["pullRequestId"] = pr.ID + reviewParams["union"] = true + variables := map[string]interface{}{ + "input": reviewParams, + } + err := client.GraphQL(repo.RepoHost(), reviewQuery, variables, &result) + if err != nil { + return pr, err + } } } @@ -1109,6 +1121,126 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, return candidates, moreResults, nil } +// SuggestedReviewerActorsForRepo fetches potential reviewers for a repository. +// Unlike SuggestedReviewerActors, this doesn't require an existing PR - used for gh pr create. +// It combines results from two sources using a cascading quota system: +// - repository collaborators (base quota: 5) +// - organization teams (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 10 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 SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { + type responseData struct { + Repository struct { + // Check for Copilot availability by looking at any open PR's suggested reviewers + PullRequests struct { + Nodes []struct { + SuggestedActors struct { + Nodes []struct { + Reviewer struct { + TypeName string `graphql:"__typename"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 10)"` + } + } `graphql:"pullRequests(first: 1, states: [OPEN])"` + Collaborators 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 { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"organization(login: $owner)"` + } + + variables := map[string]interface{}{ + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) + // Handle the case where the owner is not an organization - the query still returns + // partial data (repository), so we can continue processing. + if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { + return nil, 0, err + } + + // Build candidates using cascading quota logic + seen := make(map[string]bool) + var candidates []ReviewerCandidate + const baseQuota = 5 + + // Check for Copilot availability from open PR's suggested reviewers + for _, pr := range result.Repository.PullRequests.Nodes { + for _, actor := range pr.SuggestedActors.Nodes { + if actor.Reviewer.TypeName == "Bot" && actor.Reviewer.Bot.Login == CopilotReviewerLogin { + candidates = append(candidates, NewReviewerBot(CopilotReviewerLogin)) + seen[CopilotReviewerLogin] = true + break + } + } + } + + // Collaborators + collaboratorsAdded := 0 + for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= baseQuota { + break + } + if c.Login == "" { + continue + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() + for _, t := range result.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } + if t.Slug == "" { + continue + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // 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 +} + func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { var mutation struct { UpdatePullRequestBranch struct { diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 69dc505ca70..4ee312b5d8b 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -362,3 +362,170 @@ func TestSuggestedReviewerActors(t *testing.T) { }) } } + +// mockReviewerResponseForRepo generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// It creates collaborators (c1, c2...) and teams (team1, team2...). +func mockReviewerResponseForRepo(collabs, teams, totalCollabs, totalTeams int) string { + return mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams, false) +} + +// mockReviewerResponseForRepoWithCopilot generates a GraphQL response for SuggestedReviewerActorsForRepo tests. +// If copilotAvailable is true, includes Copilot in the first open PR's suggested reviewers. +func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalTeams int, copilotAvailable bool) string { + var collabNodes, teamNodes []string + + 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)) + } + + pullRequestsJSON := `"pullRequests": {"nodes": []}` + if copilotAvailable { + pullRequestsJSON = `"pullRequests": {"nodes": [{"suggestedReviewerActors": {"nodes": [{"reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}]}}]}` + } + + return fmt.Sprintf(`{ + "data": { + "repository": { + %s, + "collaborators": {"nodes": [%s]}, + "collaboratorsTotalCount": {"totalCount": %d} + }, + "organization": { + "teams": {"nodes": [%s]}, + "teamsTotalCount": {"totalCount": %d} + } + } + }`, pullRequestsJSON, strings.Join(collabNodes, ","), totalCollabs, + strings.Join(teamNodes, ","), totalTeams) +} + +func TestSuggestedReviewerActorsForRepo(t *testing.T) { + tests := []struct { + name string + httpStubs func(*httpmock.Registry) + expectedCount int + expectedLogins []string + expectedMore int + expectError bool + }{ + { + name: "both sources plentiful - 5 each from cascading quota", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(6, 6, 20, 10))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 30, + }, + { + name: "few collaborators - teams fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(2, 10, 2, 15))) + }, + expectedCount: 10, + expectedLogins: []string{"c1", "c2", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8"}, + expectedMore: 17, + }, + { + name: "no collaborators - teams only", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 10, 0, 20))) + }, + expectedCount: 10, + expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 20, + }, + { + name: "personal repo - no organization teams", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(`{ + "data": { + "repository": { + "pullRequests": {"nodes": []}, + "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, + "collaboratorsTotalCount": {"totalCount": 3} + }, + "organization": null + }, + "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + }`)) + }, + expectedCount: 1, + expectedLogins: []string{"c1"}, + expectedMore: 3, + }, + { + name: "empty repo", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepo(0, 0, 0, 0))) + }, + expectedCount: 0, + expectedLogins: []string{}, + expectedMore: 0, + }, + { + name: "copilot available - prepended to candidates", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, true))) + }, + expectedCount: 6, + expectedLogins: []string{"copilot-pull-request-reviewer", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + { + name: "copilot not available - not included", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(mockReviewerResponseForRepoWithCopilot(3, 2, 5, 5, false))) + }, + expectedCount: 5, + expectedLogins: []string{"c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, + expectedMore: 10, + }, + } + + 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 := SuggestedReviewerActorsForRepo(client, repo, "") + 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) + }) + } +} diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index bc38c52b356..e3f2bd5e934 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -312,7 +312,7 @@ func createRun(opts *CreateOptions) (err error) { Repo: baseRepo, State: &tb, } - err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support) + err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil) if err != nil { return } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 5b5d45f9c0a..d29240fe5fd 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -21,6 +21,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -397,11 +398,36 @@ func createRun(opts *CreateOptions) error { client := ctx.Client + // Detect ActorIsAssignable feature to determine if we can use search-based + // reviewer selection (github.com) or need to use traditional ID-based selection (GHES) + issueFeatures, _ := opts.Detector.IssueFeatures() + var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult + if issueFeatures.ActorIsAssignable { + // Create search function for reviewer selection using login-based API + reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { + candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + keys := make([]string, len(candidates)) + labels := make([]string, len(candidates)) + for i, c := range candidates { + keys[i] = c.Login() + labels[i] = c.DisplayName() + } + return prompter.MultiSelectSearchResult{Keys: keys, Labels: labels, MoreResults: moreResults} + } + } + state, err := NewIssueState(*ctx, *opts) if err != nil { return err } + if issueFeatures.ActorIsAssignable { + state.ActorReviewers = true + } + var openURL string if opts.WebMode { @@ -568,7 +594,7 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support) + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support, reviewerSearchFunc) if err != nil { return err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 5ccffd7a8a8..254b545289c 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -434,92 +434,6 @@ func Test_createRun(t *testing.T) { }, expectedErrOut: "", }, - { - name: "dry-run-nontty-with-all-opts", - tty: false, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - "Would have created a Pull Request with:", - `title: TITLE`, - `draft: false`, - `base: trunk`, - `head: feature`, - `labels: bug, todo`, - `reviewers: hubot, monalisa, /core, /robots`, - `assignees: monalisa`, - `milestones: big one.oh`, - `projects: roadmap`, - `maintainerCanModify: false`, - `body:`, - `BODY`, - ``, - }, - expectedErrOut: "", - }, { name: "dry-run-tty-with-default-base", tty: true, @@ -549,98 +463,6 @@ func Test_createRun(t *testing.T) { Dry Running pull request for feature into master in OWNER/REPO - `), - }, - { - name: "dry-run-tty-with-all-opts", - tty: true, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "TITLE" - opts.Body = "BODY" - opts.BaseBranch = "trunk" - opts.HeadBranch = "feature" - opts.Assignees = []string{"monalisa"} - opts.Labels = []string{"bug", "todo"} - opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} - opts.Milestone = "big one.oh" - opts.DryRun = true - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "labels": { - "nodes": [ - { "name": "TODO", "id": "TODOID" }, - { "name": "bug", "id": "BUGID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` - { "data": { "repository": { "milestones": { - "nodes": [ - { "title": "GA", "id": "GAID" }, - { "title": "Big One.oh", "id": "BIGONEID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - mockRetrieveProjects(t, reg) - }, - expectedOutputs: []string{ - `Would have created a Pull Request with:`, - `Title: TITLE`, - `Draft: false`, - `Base: trunk`, - `Head: feature`, - `Labels: bug, todo`, - `Reviewers: hubot, monalisa, /core, /robots`, - `Assignees: monalisa`, - `Milestones: big one.oh`, - `Projects: roadmap`, - `MaintainerCanModify: false`, - `Body:`, - ``, - ` BODY `, - ``, - ``, - }, - expectedErrOut: heredoc.Doc(` - - Dry Running pull request for feature into trunk in OWNER/REPO - `), }, { @@ -1092,9 +914,6 @@ func Test_createRun(t *testing.T) { return func() {} }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) reg.Register( httpmock.GraphQL(`query RepositoryAssignableUsers\b`), httpmock.StringResponse(` @@ -1128,17 +947,6 @@ func Test_createRun(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) mockRetrieveProjects(t, reg) reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), @@ -1171,15 +979,15 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "BIGONEID", inputs["milestoneId"]) })) reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, @@ -1680,7 +1488,7 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, { - name: "fetch org teams non-interactively if reviewer contains any team", + name: "request reviewers by login", setup: func(opts *CreateOptions, t *testing.T) func() { opts.TitleProvided = true opts.BodyProvided = true @@ -1700,78 +1508,399 @@ func Test_createRun(t *testing.T) { } } } }`, func(input map[string]interface{}) {})) reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(` - { "data": { "viewer": { "login": "monalisa" } } } - `)) - reg.Register( - httpmock.GraphQL(`query OrganizationTeamList\b`), - httpmock.StringResponse(` - { "data": { "organization": { "teams": { - "nodes": [ - { "slug": "core", "id": "COREID" }, - { "slug": "robots", "id": "ROBOTID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( - httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` - { "data": { "requestReviews": { + { "data": { "requestReviewsByLogin": { "clientMutationId": "" } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) - assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "", }, - { - name: "do not fetch org teams non-interactively if reviewer does not contain any team", - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.TitleProvided = true - opts.BodyProvided = true - opts.Title = "my title" - opts.Body = "my body" - opts.Reviewers = []string{"hubot", "monalisa"} - opts.HeadBranch = "feature" - return func() {} - }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`mutation PullRequestCreate\b`), - httpmock.GraphQLMutation(` - { "data": { "createPullRequest": { "pullRequest": { - "URL": "https://github.com/OWNER/REPO/pull/12", - "id": "NEWPULLID" - } } } }`, - func(input map[string]interface{}) {})) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID" }, - { "login": "MonaLisa", "id": "MONAID" } - ], - "pageInfo": { "hasNextPage": false } - } } } } + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + branch := "feature" + reg := &httpmock.Registry{} + reg.StubRepoInfoResponse("OWNER", "REPO", "master") + defer reg.Verify(t) + if tt.httpStubs != nil { + tt.httpStubs(reg, t) + } + + pm := &prompter.PrompterMock{} + + if tt.promptStubs != nil { + tt.promptStubs(pm) + } + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + if !tt.customBranchConfig { + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + } + + if tt.cmdStubs != nil { + tt.cmdStubs(cs) + } + + opts := CreateOptions{} + opts.Prompter = pm + + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + ios.SetStdinTTY(tt.tty) + ios.SetStderrTTY(tt.tty) + + browser := &browser.Stub{} + opts.IO = ios + opts.Browser = browser + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + opts.Branch = func() (string, error) { + return branch, nil + } + opts.Finder = shared.NewMockFinder(branch, nil, nil) + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } + cleanSetup := func() {} + if tt.setup != nil { + cleanSetup = tt.setup(&opts, t) + } + defer cleanSetup() + + // All tests in this function use github.com behavior + opts.Detector = &fd.EnabledDetectorMock{} + + if opts.HeadBranch == "" { + cs.Register(`git status --porcelain`, 0, "") + } + + err := createRun(&opts) + output := &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + BrowsedURL: browser.BrowsedURL(), + } + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) + } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) + } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) + assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + } + }) + } +} + +func Test_createRun_GHES(t *testing.T) { + tests := []struct { + name string + setup func(*CreateOptions, *testing.T) func() + cmdStubs func(*run.CommandStubber) + promptStubs func(*prompter.PrompterMock) + httpStubs func(*httpmock.Registry, *testing.T) + expectedOutputs []string + expectedOut string + expectedErrOut string + tty bool + customBranchConfig bool + }{ + { + name: "dry-run-nontty-with-all-opts", + tty: false, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + "Would have created a Pull Request with:", + `title: TITLE`, + `draft: false`, + `base: trunk`, + `head: feature`, + `labels: bug, todo`, + `reviewers: hubot, monalisa, /core, /robots`, + `assignees: monalisa`, + `milestones: big one.oh`, + `maintainerCanModify: false`, + `body:`, + `BODY`, + ``, + }, + expectedErrOut: "", + }, + { + name: "dry-run-tty-with-all-opts", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "TITLE" + opts.Body = "BODY" + opts.BaseBranch = "trunk" + opts.HeadBranch = "feature" + opts.Assignees = []string{"monalisa"} + opts.Labels = []string{"bug", "todo"} + opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Milestone = "big one.oh" + opts.DryRun = true + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID", "name": "" }, + { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "labels": { + "nodes": [ + { "name": "TODO", "id": "TODOID" }, + { "name": "bug", "id": "BUGID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "milestones": { + "nodes": [ + { "title": "GA", "id": "GAID" }, + { "title": "Big One.oh", "id": "BIGONEID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + }, + expectedOutputs: []string{ + `Would have created a Pull Request with:`, + `Title: TITLE`, + `Draft: false`, + `Base: trunk`, + `Head: feature`, + `Labels: bug, todo`, + `Reviewers: hubot, monalisa, /core, /robots`, + `Assignees: monalisa`, + `Milestones: big one.oh`, + `MaintainerCanModify: false`, + `Body:`, + ``, + ` BODY `, + ``, + ``, + }, + expectedErrOut: heredoc.Doc(` + + Dry Running pull request for feature into trunk in OWNER/REPO + + `), + }, + { + name: "fetch org teams non-interactively if reviewer contains any team", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "monalisa", "org/core", "org/robots"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "login": "monalisa" } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationTeamList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "teams": { + "nodes": [ + { "slug": "core", "id": "COREID" }, + { "slug": "robots", "id": "ROBOTID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviews": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"HUBOTID", "MONAID"}, inputs["userIds"]) + assert.Equal(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, + { + name: "do not fetch org teams non-interactively if reviewer does not contain any team", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "monalisa"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableUsers\b`), + httpmock.StringResponse(` + { "data": { "repository": { "assignableUsers": { + "nodes": [ + { "login": "hubot", "id": "HUBOTID" }, + { "login": "MonaLisa", "id": "MONAID" } + ], + "pageInfo": { "hasNextPage": false } + } } } } `)) reg.Register( httpmock.GraphQL(`query UserCurrent\b`), @@ -1951,11 +2080,10 @@ func Test_createRun(t *testing.T) { } opts := CreateOptions{} - opts.Detector = &fd.EnabledDetectorMock{} + opts.Detector = &fd.DisabledDetectorMock{} opts.Prompter = pm ios, _, stdout, stderr := iostreams.Test() - // TODO do i need to bother with this ios.SetStdoutTTY(tt.tty) ios.SetStdinTTY(tt.tty) ios.SetStderrTTY(tt.tty) @@ -1999,23 +2127,17 @@ func Test_createRun(t *testing.T) { err := createRun(&opts) output := &test.CmdOut{ - OutBuf: stdout, - ErrBuf: stderr, - BrowsedURL: browser.BrowsedURL(), + OutBuf: stdout, + ErrBuf: stderr, } - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) - } else { - assert.NoError(t, err) - if tt.expectedOut != "" { - assert.Equal(t, tt.expectedOut, output.String()) - } - if len(tt.expectedOutputs) > 0 { - assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) - } - assert.Equal(t, tt.expectedErrOut, output.Stderr()) - assert.Equal(t, tt.expectedBrowse, output.BrowsedURL) + assert.NoError(t, err) + if tt.expectedOut != "" { + assert.Equal(t, tt.expectedOut, output.String()) + } + if len(tt.expectedOutputs) > 0 { + assert.Equal(t, tt.expectedOutputs, strings.Split(output.String(), "\n")) } + assert.Equal(t, tt.expectedErrOut, output.Stderr()) }) } } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 784b68cf9bc..70990f2bfa7 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -61,11 +61,14 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par return nil } + // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. + needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { input := api.RepoMetadataInput{ - Reviewers: len(tb.Reviewers) > 0, - TeamReviewers: len(tb.Reviewers) > 0 && slices.ContainsFunc(tb.Reviewers, func(r string) bool { + Reviewers: needReviewerIDs, + TeamReviewers: needReviewerIDs && slices.ContainsFunc(tb.Reviewers, func(r string) bool { return strings.ContainsRune(r, '/') }), Assignees: len(tb.Assignees) > 0, @@ -124,17 +127,34 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } } - userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) - } - params["userReviewerIds"] = userReviewerIDs + // When ActorReviewers is true (github.com), pass logins directly for use with + // RequestReviewsByLogin mutation. Otherwise, resolve to IDs for GHES compatibility. + if tb.ActorReviewers { + params["userReviewerLogins"] = userReviewers + // Extract team slugs from org/slug format + teamSlugs := make([]string, len(teamReviewers)) + for i, t := range teamReviewers { + parts := strings.SplitN(t, "/", 2) + if len(parts) == 2 { + teamSlugs[i] = parts[1] + } else { + teamSlugs[i] = t + } + } + params["teamReviewerSlugs"] = teamSlugs + } else { + userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["userReviewerIds"] = userReviewerIDs - teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) - if err != nil { - return fmt.Errorf("could not request reviewer: %w", err) + teamReviewerIDs, err := tb.MetadataResult.TeamsToIDs(teamReviewers) + if err != nil { + return fmt.Errorf("could not request reviewer: %w", err) + } + params["teamReviewerIds"] = teamReviewerIDs } - params["teamReviewerIds"] = teamReviewerIDs return nil } diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index b9f2c029372..0e5c31cdd0a 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -20,6 +20,7 @@ type IssueMetadataState struct { Draft bool ActorAssignees bool + ActorReviewers bool Body string Title string diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index e350671b947..7bcf3a4a7b1 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -154,7 +154,7 @@ type RepoMetadataFetcher interface { RepoMetadataFetch(api.RepoMetadataInput) (*api.RepoMetadataResult, error) } -func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support) error { +func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support, reviewerSearchFunc func(string) prompter.MultiSelectSearchResult) error { isChosen := func(m string) bool { for _, c := range state.Metadata { if m == c { @@ -254,7 +254,19 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface }{} if isChosen("Reviewers") { - if len(reviewers) > 0 { + if reviewerSearchFunc != nil { + // Use search-based selection (github.com with ActorIsAssignable) + selectedReviewers, err := p.MultiSelectWithSearch( + "Reviewers", + "Search reviewers", + state.Reviewers, + []string{}, + reviewerSearchFunc) + if err != nil { + return err + } + values.Reviewers = selectedReviewers + } else if len(reviewers) > 0 { selected, err := p.MultiSelect("Reviewers", state.Reviewers, reviewers) if err != nil { return err diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 7097d0761d4..23ba96ceff7 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -71,7 +71,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { Assignees: []string{"hubot"}, Type: PRMetadata, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -117,7 +117,7 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { Assignees: []string{"hubot"}, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -146,7 +146,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported, nil) require.ErrorContains(t, err, "expected test error") require.True(t, fetcher.projectsV1Requested, "expected projectsV1 to be requested") @@ -167,7 +167,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported, nil) require.ErrorContains(t, err, "expected test error") require.False(t, fetcher.projectsV1Requested, "expected projectsV1 not to be requested") From 04bf86a72c568faf503cc2a88a453be3a921329c 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 02/28] 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 632e7dd7522..6ee7f0c0018 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 { @@ -669,6 +670,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 { @@ -681,16 +696,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()), @@ -702,7 +707,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 { @@ -724,16 +729,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()), @@ -745,7 +740,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 { @@ -770,7 +765,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 78aaa877183b82f5aec3a3a89878b6468330696a 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 03/28] 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 6ee7f0c0018..ccde369355e 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -684,6 +684,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 { @@ -782,23 +792,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{}{ From 620261fea4ebdfa4eb5b8edaa62e8cfc5668274f Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 10 Feb 2026 10:53:30 -0700 Subject: [PATCH 04/28] Remove redundant comments --- pkg/cmd/pr/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index d29240fe5fd..caf79c550c7 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -399,7 +399,7 @@ func createRun(opts *CreateOptions) error { client := ctx.Client // Detect ActorIsAssignable feature to determine if we can use search-based - // reviewer selection (github.com) or need to use traditional ID-based selection (GHES) + // reviewer selection (github.com) or need to use legacy ID-based selection (GHES) issueFeatures, _ := opts.Detector.IssueFeatures() var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult if issueFeatures.ActorIsAssignable { From dd9ab7152b8a628eb43cf36e49d3398b7fe96215 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 10 Feb 2026 10:54:03 -0700 Subject: [PATCH 05/28] Don't swallow error from FD Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/cmd/pr/create/create.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index caf79c550c7..bd6c79f9cc3 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -400,7 +400,10 @@ func createRun(opts *CreateOptions) error { // Detect ActorIsAssignable feature to determine if we can use search-based // reviewer selection (github.com) or need to use legacy ID-based selection (GHES) - issueFeatures, _ := opts.Detector.IssueFeatures() + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return err + } var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult if issueFeatures.ActorIsAssignable { // Create search function for reviewer selection using login-based API From 7373de3e707b59889cb186b617d3158e33db5c2d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 10 Feb 2026 10:58:23 -0700 Subject: [PATCH 06/28] Remove redundant comment --- pkg/cmd/pr/create/create.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index bd6c79f9cc3..631e6943073 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -406,7 +406,6 @@ func createRun(opts *CreateOptions) error { } var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult if issueFeatures.ActorIsAssignable { - // Create search function for reviewer selection using login-based API reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) if err != nil { From cf08f4d51e55cb5ced9812ba51d0303f395183dd Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 10 Feb 2026 11:08:12 -0700 Subject: [PATCH 07/28] Apply suggestion from @BagToad --- pkg/cmd/pr/shared/survey.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 7bcf3a4a7b1..ae84a6ef4fb 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -255,7 +255,6 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface if isChosen("Reviewers") { if reviewerSearchFunc != nil { - // Use search-based selection (github.com with ActorIsAssignable) selectedReviewers, err := p.MultiSelectWithSearch( "Reviewers", "Search reviewers", From db167d31164bc3c18d39a45f2261f79eb047a03a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:15:14 -0700 Subject: [PATCH 08/28] Preserve org/slug format for team reviewer slugs --- pkg/cmd/pr/shared/params.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 70990f2bfa7..c6c5661c8e0 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -131,17 +131,7 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par // RequestReviewsByLogin mutation. Otherwise, resolve to IDs for GHES compatibility. if tb.ActorReviewers { params["userReviewerLogins"] = userReviewers - // Extract team slugs from org/slug format - teamSlugs := make([]string, len(teamReviewers)) - for i, t := range teamReviewers { - parts := strings.SplitN(t, "/", 2) - if len(parts) == 2 { - teamSlugs[i] = parts[1] - } else { - teamSlugs[i] = t - } - } - params["teamReviewerSlugs"] = teamSlugs + params["teamReviewerSlugs"] = teamReviewers } else { userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) if err != nil { From 1209b24e69fe07a8ae4f291325d20a5c790c6b76 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:16:03 -0700 Subject: [PATCH 09/28] Partition bot reviewers separately for RequestReviewsByLogin --- pkg/cmd/pr/shared/params.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index c6c5661c8e0..90e1e6f897d 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -118,10 +118,13 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } var userReviewers []string + var botReviewers []string var teamReviewers []string for _, r := range tb.Reviewers { if strings.ContainsRune(r, '/') { teamReviewers = append(teamReviewers, r) + } else if r == api.CopilotReviewerLogin { + botReviewers = append(botReviewers, r) } else { userReviewers = append(userReviewers, r) } @@ -131,6 +134,9 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par // RequestReviewsByLogin mutation. Otherwise, resolve to IDs for GHES compatibility. if tb.ActorReviewers { params["userReviewerLogins"] = userReviewers + if len(botReviewers) > 0 { + params["botReviewerLogins"] = botReviewers + } params["teamReviewerSlugs"] = teamReviewers } else { userReviewerIDs, err := tb.MetadataResult.MembersToIDs(userReviewers) From ad64d10bf4f59c41581b905f5479f9b561b0ef9a Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:16:23 -0700 Subject: [PATCH 10/28] Wire bot reviewer logins through CreatePullRequest --- api/queries_pr.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index ccde369355e..39036127744 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -618,11 +618,12 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation userLogins, hasUserLogins := params["userReviewerLogins"].([]string) + botLogins, _ := params["botReviewerLogins"].([]string) teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) if hasUserLogins || hasTeamSlugs { // Use login-based mutation (RequestReviewsByLogin) for github.com - err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, nil, teamSlugs, true) + err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, botLogins, teamSlugs, true) if err != nil { return pr, err } From 38661646eef035f08e04e1c4e12777bedf10cddd Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:18:07 -0700 Subject: [PATCH 11/28] Update test assertions to expect org/slug team format --- pkg/cmd/pr/create/create_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 254b545289c..2adf1007b93 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -987,7 +987,7 @@ func Test_createRun(t *testing.T) { `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) - assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) + assert.Equal(t, []interface{}{"/core", "/robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, @@ -1516,7 +1516,7 @@ func Test_createRun(t *testing.T) { `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) - assert.Equal(t, []interface{}{"core", "robots"}, inputs["teamSlugs"]) + assert.Equal(t, []interface{}{"org/core", "org/robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, From e361335e5c1f67154216c80bfaeccdce29966321 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:18:39 -0700 Subject: [PATCH 12/28] Skip reviewer metadata fetch when using search-based selection --- pkg/cmd/pr/shared/survey.go | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index ae84a6ef4fb..6c88a74af45 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -180,10 +180,13 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface state.Metadata = append(state.Metadata, extraFieldsOptions[i]) } - // Retrieve and process data for survey prompts based on the extra fields selected + // Retrieve and process data for survey prompts based on the extra fields selected. + // When search-based reviewer selection is available, skip the expensive assignable-users + // and teams fetch since reviewers are found dynamically via the search function. + useReviewerSearch := reviewerSearchFunc != nil metadataInput := api.RepoMetadataInput{ - Reviewers: isChosen("Reviewers"), - TeamReviewers: isChosen("Reviewers"), + Reviewers: isChosen("Reviewers") && !useReviewerSearch, + TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, Assignees: isChosen("Assignees"), ActorAssignees: isChosen("Assignees") && state.ActorAssignees, Labels: isChosen("Labels"), @@ -197,13 +200,15 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } var reviewers []string - for _, u := range metadataResult.AssignableUsers { - if u.Login() != metadataResult.CurrentLogin { - reviewers = append(reviewers, u.DisplayName()) + if !useReviewerSearch { + for _, u := range metadataResult.AssignableUsers { + if u.Login() != metadataResult.CurrentLogin { + reviewers = append(reviewers, u.DisplayName()) + } + } + for _, t := range metadataResult.Teams { + reviewers = append(reviewers, fmt.Sprintf("%s/%s", baseRepo.RepoOwner(), t.Slug)) } - } - for _, t := range metadataResult.Teams { - reviewers = append(reviewers, fmt.Sprintf("%s/%s", baseRepo.RepoOwner(), t.Slug)) } // Populate the list of selectable assignees and their default selections. From a8655bcda9261f462f6bc37f97b157fd1893c7c3 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 13:55:57 -0700 Subject: [PATCH 13/28] Include bot logins in login-based reviewer mutation guard --- api/queries_pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 39036127744..eb2eb549491 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -618,10 +618,10 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation userLogins, hasUserLogins := params["userReviewerLogins"].([]string) - botLogins, _ := params["botReviewerLogins"].([]string) + botLogins, hasBotLogins := params["botReviewerLogins"].([]string) teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) - if hasUserLogins || hasTeamSlugs { + if hasUserLogins || hasBotLogins || hasTeamSlugs { // Use login-based mutation (RequestReviewsByLogin) for github.com err := RequestReviewsByLogin(client, repo, pr.ID, userLogins, botLogins, teamSlugs, true) if err != nil { From 1cb776384ecd1c2a935b86096fe58ad31b9facfb Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 14:33:13 -0700 Subject: [PATCH 14/28] Normalize /slug team shorthand to org/slug and fix docs --- api/queries_pr.go | 4 ++-- pkg/cmd/pr/create/create_test.go | 2 +- pkg/cmd/pr/shared/params.go | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index eb2eb549491..2ee1d5e5f14 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -764,8 +764,8 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in // 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". +// Bot logins should be passed without the [bot] suffix; it is appended automatically. +// Team slugs must 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. diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 2adf1007b93..a5e30cfcdb3 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -987,7 +987,7 @@ func Test_createRun(t *testing.T) { `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) assert.Equal(t, []interface{}{"hubot", "monalisa"}, inputs["userLogins"]) - assert.Equal(t, []interface{}{"/core", "/robots"}, inputs["teamSlugs"]) + assert.Equal(t, []interface{}{"OWNER/core", "OWNER/robots"}, inputs["teamSlugs"]) assert.Equal(t, true, inputs["union"]) })) }, diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 90e1e6f897d..06f599f0d6f 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -122,6 +122,10 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par var teamReviewers []string for _, r := range tb.Reviewers { if strings.ContainsRune(r, '/') { + // Normalize /slug shorthand to org/slug using the repo owner + if strings.HasPrefix(r, "/") { + r = baseRepo.RepoOwner() + r + } teamReviewers = append(teamReviewers, r) } else if r == api.CopilotReviewerLogin { botReviewers = append(botReviewers, r) From 1d730951d2a13b3b959d939a977a3f128d883d3e Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Feb 2026 14:33:13 -0700 Subject: [PATCH 15/28] Use org/slug format in test fixtures and remove /slug normalization --- pkg/cmd/pr/create/create_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index a5e30cfcdb3..428aca650ed 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -909,7 +909,7 @@ func Test_createRun(t *testing.T) { opts.Assignees = []string{"monalisa"} opts.Labels = []string{"bug", "todo"} opts.Projects = []string{"roadmap"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} opts.Milestone = "big one.oh" return func() {} }, @@ -1648,7 +1648,7 @@ func Test_createRun_GHES(t *testing.T) { opts.HeadBranch = "feature" opts.Assignees = []string{"monalisa"} opts.Labels = []string{"bug", "todo"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} opts.Milestone = "big one.oh" opts.DryRun = true return func() {} @@ -1709,7 +1709,7 @@ func Test_createRun_GHES(t *testing.T) { `base: trunk`, `head: feature`, `labels: bug, todo`, - `reviewers: hubot, monalisa, /core, /robots`, + `reviewers: hubot, monalisa, OWNER/core, OWNER/robots`, `assignees: monalisa`, `milestones: big one.oh`, `maintainerCanModify: false`, @@ -1731,7 +1731,7 @@ func Test_createRun_GHES(t *testing.T) { opts.HeadBranch = "feature" opts.Assignees = []string{"monalisa"} opts.Labels = []string{"bug", "todo"} - opts.Reviewers = []string{"hubot", "monalisa", "/core", "/robots"} + opts.Reviewers = []string{"hubot", "monalisa", "OWNER/core", "OWNER/robots"} opts.Milestone = "big one.oh" opts.DryRun = true return func() {} @@ -1792,7 +1792,7 @@ func Test_createRun_GHES(t *testing.T) { `Base: trunk`, `Head: feature`, `Labels: bug, todo`, - `Reviewers: hubot, monalisa, /core, /robots`, + `Reviewers: hubot, monalisa, OWNER/core, OWNER/robots`, `Assignees: monalisa`, `Milestones: big one.oh`, `MaintainerCanModify: false`, From 72a6e9f3a7c533139a475c482b0e179574b2ff58 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 13 Feb 2026 13:40:59 -0700 Subject: [PATCH 16/28] Move PR review queries from queries_pr.go to queries_pr_review.go Consolidate all review-related types, methods, and functions into queries_pr_review.go for better code organization. The file is now ordered by logical sections: review data types, review status, review requests, reviewer candidates, API operations, and helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr.go | 568 ----------------------------------- api/queries_pr_review.go | 624 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 597 insertions(+), 595 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 2ee1d5e5f14..f1d38397131 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1,12 +1,9 @@ package api import ( - "bytes" - "encoding/json" "fmt" "net/http" "net/url" - "strings" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -301,65 +298,6 @@ type PullRequestFile struct { Deletions int `json:"deletions"` } -type ReviewRequests struct { - Nodes []struct { - RequestedReviewer RequestedReviewer - } -} - -type RequestedReviewer struct { - TypeName string `json:"__typename"` - Login string `json:"login"` - Name string `json:"name"` - Slug string `json:"slug"` - Organization struct { - Login string `json:"login"` - } `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) - } - 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 (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) - } - if r.TypeName == botTypeName && r.Login == CopilotReviewerLogin { - return "Copilot (AI)" - } - if r.Name != "" { - return fmt.Sprintf("%s (%s)", r.Login, r.Name) - } - return r.Login -} - -func (r ReviewRequests) Logins() []string { - logins := make([]string, len(r.Nodes)) - for i, r := range r.Nodes { - logins[i] = r.RequestedReviewer.LoginOrSlug() - } - 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) @@ -383,25 +321,6 @@ func (pr PullRequest) IsOpen() bool { return pr.State == "OPEN" } -type PullRequestReviewStatus struct { - ChangesRequested bool - Approved bool - ReviewRequired bool -} - -func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { - var status PullRequestReviewStatus - switch pr.ReviewDecision { - case "CHANGES_REQUESTED": - status.ChangesRequested = true - case "APPROVED": - status.Approved = true - case "REVIEW_REQUIRED": - status.ReviewRequired = true - } - return status -} - type PullRequestChecksStatus struct { Pending int Failing int @@ -541,18 +460,6 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS } } -func (pr *PullRequest) DisplayableReviews() PullRequestReviews { - published := []PullRequestReview{} - for _, prr := range pr.Reviews.Nodes { - //Dont display pending reviews - //Dont display commenting reviews without top level comment body - if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { - published = append(published, prr) - } - } - return PullRequestReviews{Nodes: published, TotalCount: len(published)} -} - // CreatePullRequest creates a pull request in a GitHub repository func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) { query := ` @@ -671,145 +578,6 @@ 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 -} - -// 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 { - if len(users) == 0 && len(teams) == 0 { - return nil - } - - // The API requires empty arrays instead of null values - if users == nil { - users = []string{} - } - - path := fmt.Sprintf( - "repos/%s/%s/pulls/%d/requested_reviewers", - url.PathEscape(repo.RepoOwner()), - url.PathEscape(repo.RepoName()), - prNumber, - ) - body := struct { - Reviewers []string `json:"reviewers"` - TeamReviewers []string `json:"team_reviewers"` - }{ - Reviewers: users, - TeamReviewers: extractTeamSlugs(teams), - } - buf := &bytes.Buffer{} - if err := json.NewEncoder(buf).Encode(body); err != nil { - return err - } - // The endpoint responds with the updated pull request object; we don't need it here. - return client.REST(repo.RepoHost(), "POST", path, buf, nil) -} - -// 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 - } - - // The API requires empty arrays instead of null values - if users == nil { - users = []string{} - } - - path := fmt.Sprintf( - "repos/%s/%s/pulls/%d/requested_reviewers", - url.PathEscape(repo.RepoOwner()), - url.PathEscape(repo.RepoName()), - prNumber, - ) - body := struct { - Reviewers []string `json:"reviewers"` - TeamReviewers []string `json:"team_reviewers"` - }{ - Reviewers: users, - TeamReviewers: extractTeamSlugs(teams), - } - buf := &bytes.Buffer{} - if err := json.NewEncoder(buf).Encode(body); err != nil { - return err - } - // The endpoint responds with the updated pull request object; we don't need it here. - 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 be passed without the [bot] suffix; it is appended automatically. -// Team slugs must 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:"clientMutationId"` - } `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), - } - - userLoginValues := toGitHubV4Strings(userLogins, "") - input.UserLogins = &userLoginValues - - // Bot logins require the [bot] suffix for the mutation - botLoginValues := toGitHubV4Strings(botLogins, "[bot]") - input.BotLogins = &botLoginValues - - teamSlugValues := toGitHubV4Strings(teamSlugs, "") - input.TeamSlugs = &teamSlugValues - - 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. @@ -902,342 +670,6 @@ 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. -type ReviewerTeam struct { - org string - teamSlug string -} - -// NewReviewerTeam creates a new ReviewerTeam. -func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { - return ReviewerTeam{org: orgName, teamSlug: teamSlug} -} - -func (r ReviewerTeam) DisplayName() string { - return fmt.Sprintf("%s/%s", r.org, r.teamSlug) -} - -func (r ReviewerTeam) Login() string { - return fmt.Sprintf("%s/%s", r.org, r.teamSlug) -} - -func (r ReviewerTeam) Slug() string { - return r.teamSlug -} - -func (r ReviewerTeam) sealedReviewerCandidate() {} - -// SuggestedReviewerActors fetches suggested reviewers for a pull request. -// 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. - // We also fetch unfiltered total counts via aliases for the "X more" display. - 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: 10, query: $query)"` - } `graphql:"... on PullRequest"` - } `graphql:"node(id: $id)"` - Repository struct { - Collaborators 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 { - Nodes []struct { - Slug string - } - } `graphql:"teams(first: 10, query: $query)"` - TeamsTotalCount struct { - TotalCount int - } `graphql:"teamsTotalCount: teams(first: 0)"` - } `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 - } - - // 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 - - // 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 != "" { - login = n.Reviewer.User.Login - candidate = NewReviewerUser(login, n.Reviewer.User.Name) - } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { - login = n.Reviewer.Bot.Login - candidate = NewReviewerBot(login) - } else { - continue - } - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) - suggestionsAdded++ - } - } - - // 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 - } - if !seen[c.Login] { - seen[c.Login] = true - candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) - collaboratorsAdded++ - } - } - - // 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 - } - teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) - if !seen[teamLogin] { - seen[teamLogin] = true - candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) - teamsAdded++ - } - } - - // 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 -} - -// SuggestedReviewerActorsForRepo fetches potential reviewers for a repository. -// Unlike SuggestedReviewerActors, this doesn't require an existing PR - used for gh pr create. -// It combines results from two sources using a cascading quota system: -// - repository collaborators (base quota: 5) -// - organization teams (base quota: 5 + unfilled from collaborators) -// -// This ensures we show up to 10 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 SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { - type responseData struct { - Repository struct { - // Check for Copilot availability by looking at any open PR's suggested reviewers - PullRequests struct { - Nodes []struct { - SuggestedActors struct { - Nodes []struct { - Reviewer struct { - TypeName string `graphql:"__typename"` - Bot struct { - Login string - } `graphql:"... on Bot"` - } - } - } `graphql:"suggestedReviewerActors(first: 10)"` - } - } `graphql:"pullRequests(first: 1, states: [OPEN])"` - Collaborators 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 { - Nodes []struct { - Slug string - } - } `graphql:"teams(first: 10, query: $query)"` - TeamsTotalCount struct { - TotalCount int - } `graphql:"teamsTotalCount: teams(first: 0)"` - } `graphql:"organization(login: $owner)"` - } - - variables := map[string]interface{}{ - "query": githubv4.String(query), - "owner": githubv4.String(repo.RepoOwner()), - "name": githubv4.String(repo.RepoName()), - } - - var result responseData - err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) - // Handle the case where the owner is not an organization - the query still returns - // partial data (repository), so we can continue processing. - if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { - return nil, 0, err - } - - // Build candidates using cascading quota logic - seen := make(map[string]bool) - var candidates []ReviewerCandidate - const baseQuota = 5 - - // Check for Copilot availability from open PR's suggested reviewers - for _, pr := range result.Repository.PullRequests.Nodes { - for _, actor := range pr.SuggestedActors.Nodes { - if actor.Reviewer.TypeName == "Bot" && actor.Reviewer.Bot.Login == CopilotReviewerLogin { - candidates = append(candidates, NewReviewerBot(CopilotReviewerLogin)) - seen[CopilotReviewerLogin] = true - break - } - } - } - - // Collaborators - collaboratorsAdded := 0 - for _, c := range result.Repository.Collaborators.Nodes { - if collaboratorsAdded >= baseQuota { - break - } - if c.Login == "" { - continue - } - if !seen[c.Login] { - seen[c.Login] = true - candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) - collaboratorsAdded++ - } - } - - // Teams: quota = base + unfilled from collaborators - teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) - teamsAdded := 0 - ownerName := repo.RepoOwner() - for _, t := range result.Organization.Teams.Nodes { - if teamsAdded >= teamsQuota { - break - } - if t.Slug == "" { - continue - } - teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) - if !seen[teamLogin] { - seen[teamLogin] = true - candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) - teamsAdded++ - } - } - - // 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 -} - func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { var mutation struct { UpdatePullRequestBranch struct { diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index d5565b54b23..b233ee46ff1 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -1,6 +1,11 @@ package api import ( + "bytes" + "encoding/json" + "fmt" + "net/url" + "strings" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -42,33 +47,6 @@ type PullRequestReview struct { Commit Commit `json:"commit"` } -func AddReview(client *Client, repo ghrepo.Interface, pr *PullRequest, input *PullRequestReviewInput) error { - var mutation struct { - AddPullRequestReview struct { - ClientMutationID string - } `graphql:"addPullRequestReview(input:$input)"` - } - - state := githubv4.PullRequestReviewEventComment - switch input.State { - case ReviewApprove: - state = githubv4.PullRequestReviewEventApprove - case ReviewRequestChanges: - state = githubv4.PullRequestReviewEventRequestChanges - } - - body := githubv4.String(input.Body) - variables := map[string]interface{}{ - "input": githubv4.AddPullRequestReviewInput{ - PullRequestID: pr.ID, - Event: &state, - Body: &body, - }, - } - - return client.Mutate(repo.RepoHost(), "PullRequestReviewAdd", &mutation, variables) -} - func (prr PullRequestReview) Identifier() string { return prr.ID } @@ -115,3 +93,595 @@ func (prr PullRequestReview) Reactions() ReactionGroups { func (prr PullRequestReview) Status() string { return prr.State } + +type PullRequestReviewStatus struct { + ChangesRequested bool + Approved bool + ReviewRequired bool +} + +func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { + var status PullRequestReviewStatus + switch pr.ReviewDecision { + case "CHANGES_REQUESTED": + status.ChangesRequested = true + case "APPROVED": + status.Approved = true + case "REVIEW_REQUIRED": + status.ReviewRequired = true + } + return status +} + +func (pr *PullRequest) DisplayableReviews() PullRequestReviews { + published := []PullRequestReview{} + for _, prr := range pr.Reviews.Nodes { + //Dont display pending reviews + //Dont display commenting reviews without top level comment body + if prr.State != "PENDING" && !(prr.State == "COMMENTED" && prr.Body == "") { + published = append(published, prr) + } + } + return PullRequestReviews{Nodes: published, TotalCount: len(published)} +} + +type ReviewRequests struct { + Nodes []struct { + RequestedReviewer RequestedReviewer + } +} + +type RequestedReviewer struct { + TypeName string `json:"__typename"` + Login string `json:"login"` + Name string `json:"name"` + Slug string `json:"slug"` + Organization struct { + Login string `json:"login"` + } `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) + } + 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 (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) + } + if r.TypeName == botTypeName && r.Login == CopilotReviewerLogin { + return "Copilot (AI)" + } + if r.Name != "" { + return fmt.Sprintf("%s (%s)", r.Login, r.Name) + } + return r.Login +} + +func (r ReviewRequests) Logins() []string { + logins := make([]string, len(r.Nodes)) + for i, r := range r.Nodes { + logins[i] = r.RequestedReviewer.LoginOrSlug() + } + 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 +} + +// 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. +type ReviewerTeam struct { + org string + teamSlug string +} + +// NewReviewerTeam creates a new ReviewerTeam. +func NewReviewerTeam(orgName, teamSlug string) ReviewerTeam { + return ReviewerTeam{org: orgName, teamSlug: teamSlug} +} + +func (r ReviewerTeam) DisplayName() string { + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) +} + +func (r ReviewerTeam) Login() string { + return fmt.Sprintf("%s/%s", r.org, r.teamSlug) +} + +func (r ReviewerTeam) Slug() string { + return r.teamSlug +} + +func (r ReviewerTeam) sealedReviewerCandidate() {} + +func AddReview(client *Client, repo ghrepo.Interface, pr *PullRequest, input *PullRequestReviewInput) error { + var mutation struct { + AddPullRequestReview struct { + ClientMutationID string + } `graphql:"addPullRequestReview(input:$input)"` + } + + state := githubv4.PullRequestReviewEventComment + switch input.State { + case ReviewApprove: + state = githubv4.PullRequestReviewEventApprove + case ReviewRequestChanges: + state = githubv4.PullRequestReviewEventRequestChanges + } + + body := githubv4.String(input.Body) + variables := map[string]interface{}{ + "input": githubv4.AddPullRequestReviewInput{ + PullRequestID: pr.ID, + Event: &state, + Body: &body, + }, + } + + return client.Mutate(repo.RepoHost(), "PullRequestReviewAdd", &mutation, variables) +} + +// 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 + } + + // The API requires empty arrays instead of null values + if users == nil { + users = []string{} + } + + path := fmt.Sprintf( + "repos/%s/%s/pulls/%d/requested_reviewers", + url.PathEscape(repo.RepoOwner()), + url.PathEscape(repo.RepoName()), + prNumber, + ) + body := struct { + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` + }{ + Reviewers: users, + TeamReviewers: extractTeamSlugs(teams), + } + buf := &bytes.Buffer{} + if err := json.NewEncoder(buf).Encode(body); err != nil { + return err + } + // The endpoint responds with the updated pull request object; we don't need it here. + return client.REST(repo.RepoHost(), "POST", path, buf, nil) +} + +// 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 + } + + // The API requires empty arrays instead of null values + if users == nil { + users = []string{} + } + + path := fmt.Sprintf( + "repos/%s/%s/pulls/%d/requested_reviewers", + url.PathEscape(repo.RepoOwner()), + url.PathEscape(repo.RepoName()), + prNumber, + ) + body := struct { + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` + }{ + Reviewers: users, + TeamReviewers: extractTeamSlugs(teams), + } + buf := &bytes.Buffer{} + if err := json.NewEncoder(buf).Encode(body); err != nil { + return err + } + // The endpoint responds with the updated pull request object; we don't need it here. + 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 be passed without the [bot] suffix; it is appended automatically. +// Team slugs must 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:"clientMutationId"` + } `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), + } + + userLoginValues := toGitHubV4Strings(userLogins, "") + input.UserLogins = &userLoginValues + + // Bot logins require the [bot] suffix for the mutation + botLoginValues := toGitHubV4Strings(botLogins, "[bot]") + input.BotLogins = &botLoginValues + + teamSlugValues := toGitHubV4Strings(teamSlugs, "") + input.TeamSlugs = &teamSlugValues + + variables := map[string]interface{}{ + "input": input, + } + + return client.Mutate(repo.RepoHost(), "RequestReviewsByLogin", &mutation, variables) +} + +// SuggestedReviewerActors fetches suggested reviewers for a pull request. +// 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. + // We also fetch unfiltered total counts via aliases for the "X more" display. + 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: 10, query: $query)"` + } `graphql:"... on PullRequest"` + } `graphql:"node(id: $id)"` + Repository struct { + Collaborators 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 { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `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 + } + + // 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 + + // 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 != "" { + login = n.Reviewer.User.Login + candidate = NewReviewerUser(login, n.Reviewer.User.Name) + } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { + login = n.Reviewer.Bot.Login + candidate = NewReviewerBot(login) + } else { + continue + } + if !seen[login] { + seen[login] = true + candidates = append(candidates, candidate) + suggestionsAdded++ + } + } + + // 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 + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // 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 + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // 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 +} + +// SuggestedReviewerActorsForRepo fetches potential reviewers for a repository. +// Unlike SuggestedReviewerActors, this doesn't require an existing PR - used for gh pr create. +// It combines results from two sources using a cascading quota system: +// - repository collaborators (base quota: 5) +// - organization teams (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 10 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 SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { + type responseData struct { + Repository struct { + // Check for Copilot availability by looking at any open PR's suggested reviewers + PullRequests struct { + Nodes []struct { + SuggestedActors struct { + Nodes []struct { + Reviewer struct { + TypeName string `graphql:"__typename"` + Bot struct { + Login string + } `graphql:"... on Bot"` + } + } + } `graphql:"suggestedReviewerActors(first: 10)"` + } + } `graphql:"pullRequests(first: 1, states: [OPEN])"` + Collaborators 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 { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"organization(login: $owner)"` + } + + variables := map[string]interface{}{ + "query": githubv4.String(query), + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + } + + var result responseData + err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) + // Handle the case where the owner is not an organization - the query still returns + // partial data (repository), so we can continue processing. + if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { + return nil, 0, err + } + + // Build candidates using cascading quota logic + seen := make(map[string]bool) + var candidates []ReviewerCandidate + const baseQuota = 5 + + // Check for Copilot availability from open PR's suggested reviewers + for _, pr := range result.Repository.PullRequests.Nodes { + for _, actor := range pr.SuggestedActors.Nodes { + if actor.Reviewer.TypeName == "Bot" && actor.Reviewer.Bot.Login == CopilotReviewerLogin { + candidates = append(candidates, NewReviewerBot(CopilotReviewerLogin)) + seen[CopilotReviewerLogin] = true + break + } + } + } + + // Collaborators + collaboratorsAdded := 0 + for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= baseQuota { + break + } + if c.Login == "" { + continue + } + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ + } + } + + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() + for _, t := range result.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } + if t.Slug == "" { + continue + } + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ + } + } + + // 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 +} + +// 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 +} + +// 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 +} From ceb904413dd8f7478bdc2a7dc1ab0acfd4ab2608 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 13 Feb 2026 13:45:00 -0700 Subject: [PATCH 17/28] Clarify ReviewerCandidate relationship to AssignableActor Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr_review.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index b233ee46ff1..e4deb7124be 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -185,7 +185,10 @@ func (r ReviewRequests) DisplayNames() []string { } // ReviewerCandidate represents a potential reviewer for a pull request. -// This can be a User, Bot, or Team. +// This can be a User, Bot, or Team. It mirrors AssignableActor but adds +// team support (teams can review but not be assigned) and drops the ID method. +// ReviewerUser and ReviewerBot are thin wrappers around AssignableUser and +// AssignableBot that satisfy this interface. type ReviewerCandidate interface { DisplayName() string Login() string From 6341588f90d2b90c1900afab6fac15ea02be7b24 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:56:31 -0700 Subject: [PATCH 18/28] Add TODO requestReviewsByLoginCleanup in CreatePullRequest Add a cleanup TODO comment above the GHES feature detection branch in CreatePullRequest so we can track removing the ID-based reviewer request path once GHES supports requestReviewsByLogin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index f1d38397131..85680f6e2ca 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -523,7 +523,9 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } - // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation + // TODO requestReviewsByLoginCleanup + // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation. + // The ID-based path can be removed once GHES supports requestReviewsByLogin. userLogins, hasUserLogins := params["userReviewerLogins"].([]string) botLogins, hasBotLogins := params["botReviewerLogins"].([]string) teamSlugs, hasTeamSlugs := params["teamReviewerSlugs"].([]string) From 8f62e8116df4243019a41681ec54c66dda2e8f2e Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:56:47 -0700 Subject: [PATCH 19/28] Label Copilot detection in SuggestedReviewerActorsForRepo as a hack Mark the piggyback-on-open-PR technique for detecting Copilot reviewer availability as a HACK, since there is no repo-level API to check Copilot eligibility without a PR context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr_review.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index e4deb7124be..b25b199d1a0 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -557,7 +557,9 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { type responseData struct { Repository struct { - // Check for Copilot availability by looking at any open PR's suggested reviewers + // HACK: There's no repo-level API to check Copilot reviewer eligibility, + // so we piggyback on an open PR's suggestedReviewerActors to detect + // whether Copilot is available as a reviewer for this repository. PullRequests struct { Nodes []struct { SuggestedActors struct { From dd7e44ee0ac308c1fb967d8a971f809f7d94985c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:57:28 -0700 Subject: [PATCH 20/28] Check state.ActorReviewers in MetadataSurvey reviewer search gate Gate search-based reviewer selection on both state.ActorReviewers and the search function being available, consistent with the ActorAssignees pattern used for assignees. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/pr/shared/survey.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 6c88a74af45..0f7ed1a43ea 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -183,7 +183,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface // Retrieve and process data for survey prompts based on the extra fields selected. // When search-based reviewer selection is available, skip the expensive assignable-users // and teams fetch since reviewers are found dynamically via the search function. - useReviewerSearch := reviewerSearchFunc != nil + useReviewerSearch := state.ActorReviewers && reviewerSearchFunc != nil metadataInput := api.RepoMetadataInput{ Reviewers: isChosen("Reviewers") && !useReviewerSearch, TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, From 37776cf2e661666f2fdbf6c905362d2513826d74 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:57:45 -0700 Subject: [PATCH 21/28] Add TODO requestReviewsByLoginCleanup on static reviewer MultiSelect Mark the legacy static MultiSelect reviewer path for cleanup once GHES supports requestReviewsByLogin and search-based selection can be used universally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/pr/shared/survey.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 0f7ed1a43ea..cc66bbe5c4d 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -271,6 +271,9 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } values.Reviewers = selectedReviewers } else if len(reviewers) > 0 { + // TODO requestReviewsByLoginCleanup + // The static MultiSelect path can be removed once GHES supports + // requestReviewsByLogin and search-based selection is always used. selected, err := p.MultiSelect("Reviewers", state.Reviewers, reviewers) if err != nil { return err From 07138b6edf216c1a2cc45a2b3c7cd69d7f4ed09d Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:58:08 -0700 Subject: [PATCH 22/28] Remove /slug team reviewer shorthand normalization Team reviewers must be provided as fully qualified org/teamname. Remove the /slug shorthand that auto-prefixed the repo owner, as this format is not supported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/pr/shared/params.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 06f599f0d6f..90e1e6f897d 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -122,10 +122,6 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par var teamReviewers []string for _, r := range tb.Reviewers { if strings.ContainsRune(r, '/') { - // Normalize /slug shorthand to org/slug using the repo owner - if strings.HasPrefix(r, "/") { - r = baseRepo.RepoOwner() + r - } teamReviewers = append(teamReviewers, r) } else if r == api.CopilotReviewerLogin { botReviewers = append(botReviewers, r) From 49f1bd88004d02f73e8a7d468673decca6904be4 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 10:58:23 -0700 Subject: [PATCH 23/28] Add TODO requestReviewsByLoginCleanup on GHES ID-based reviewer path Mark the GHES ID-resolution branch in AddMetadataToIssueParams for cleanup once GHES supports requestReviewsByLogin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/pr/shared/params.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 90e1e6f897d..d5c168e5f7f 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -130,8 +130,10 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } } + // TODO requestReviewsByLoginCleanup // When ActorReviewers is true (github.com), pass logins directly for use with - // RequestReviewsByLogin mutation. Otherwise, resolve to IDs for GHES compatibility. + // RequestReviewsByLogin mutation. The ID-based else branch can be removed once + // GHES supports requestReviewsByLogin. if tb.ActorReviewers { params["userReviewerLogins"] = userReviewers if len(botReviewers) > 0 { From 08c7a4c207f480c6fa5bbbfcdec202f0a43971f5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 15:13:53 -0700 Subject: [PATCH 24/28] Replace @copilot with Copilot reviewer login in gh pr create Wire CopilotReviewerReplacer into NewIssueState so that `gh pr create --reviewer @copilot` correctly resolves to the copilot-pull-request-reviewer bot login, matching the behavior already implemented in gh pr edit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/cmd/pr/create/create.go | 5 ++++- pkg/cmd/pr/create/create_test.go | 36 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 631e6943073..1f2ce190aac 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -680,9 +680,12 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata return nil, err } + copilotReplacer := shared.NewCopilotReviewerReplacer() + reviewers := copilotReplacer.ReplaceSlice(opts.Reviewers) + state := &shared.IssueMetadataState{ Type: shared.PRMetadata, - Reviewers: opts.Reviewers, + Reviewers: reviewers, Assignees: assignees, Labels: opts.Labels, ProjectTitles: opts.Projects, diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 428aca650ed..d7c7fff8798 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1523,6 +1523,42 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "", }, + { + name: "@copilot reviewer resolves to bot login", + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Reviewers = []string{"hubot", "@copilot"} + opts.HeadBranch = "feature" + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12", + "id": "NEWPULLID" + } } } }`, + func(input map[string]interface{}) {})) + reg.Register( + httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), + httpmock.GraphQLMutation(` + { "data": { "requestReviewsByLogin": { + "clientMutationId": "" + } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) + assert.Equal(t, []interface{}{"hubot"}, inputs["userLogins"]) + assert.Equal(t, []interface{}{"copilot-pull-request-reviewer[bot]"}, inputs["botLogins"]) + assert.Equal(t, true, inputs["union"]) + })) + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 90bfa624c2c4b66053188f6d9382d7f413be258e Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 15:36:48 -0700 Subject: [PATCH 25/28] Exclude current user from suggested reviewers in gh pr create Query the viewer login in SuggestedReviewerActorsForRepo and pre-seed the seen map so the current user is filtered out of collaborator results. You cannot review your own PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr_review.go | 10 +++++++++- api/queries_pr_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index b25b199d1a0..f6ef2a74e39 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -556,6 +556,9 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, // Returns the candidates, a MoreResults count, and an error. func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query string) ([]ReviewerCandidate, int, error) { type responseData struct { + Viewer struct { + Login string + } Repository struct { // HACK: There's no repo-level API to check Copilot reviewer eligibility, // so we piggyback on an open PR's suggestedReviewerActors to detect @@ -610,8 +613,13 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query return nil, 0, err } - // Build candidates using cascading quota logic + // Build candidates using cascading quota logic. + // Pre-seed seen with the current user to exclude them from results + // since you cannot review your own PR. seen := make(map[string]bool) + if result.Viewer.Login != "" { + seen[result.Viewer.Login] = true + } var candidates []ReviewerCandidate const baseQuota = 5 diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 4ee312b5d8b..7b7727f3e7e 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -390,6 +390,7 @@ func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalT return fmt.Sprintf(`{ "data": { + "viewer": {"login": "testuser"}, "repository": { %s, "collaborators": {"nodes": [%s]}, @@ -500,6 +501,36 @@ func TestSuggestedReviewerActorsForRepo(t *testing.T) { expectedLogins: []string{"c1", "c2", "c3", "OWNER/team1", "OWNER/team2"}, expectedMore: 10, }, + { + name: "viewer excluded from collaborators", + httpStubs: func(reg *httpmock.Registry) { + // c1 matches the viewer login "testuser" won't be in this fixture, + // but we can craft a response where the viewer login matches a collaborator. + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActorsForRepo\b`), + httpmock.StringResponse(`{ + "data": { + "viewer": {"login": "c2"}, + "repository": { + "pullRequests": {"nodes": []}, + "collaborators": {"nodes": [ + {"login": "c1", "name": "C1"}, + {"login": "c2", "name": "C2"}, + {"login": "c3", "name": "C3"} + ]}, + "collaboratorsTotalCount": {"totalCount": 3} + }, + "organization": { + "teams": {"nodes": []}, + "teamsTotalCount": {"totalCount": 0} + } + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"c1", "c3"}, + expectedMore: 3, + }, } for _, tt := range tests { From 1bba50b3e01212bc7663ceceeb50c455e9fc94b4 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 15:44:25 -0700 Subject: [PATCH 26/28] Fix duplicate reviewers in gh pr edit by passing logins as defaults Use DefaultLogins instead of Default display names when calling MultiSelectWithSearch for reviewers. The dedup logic in the prompter compares keys (logins) against defaults, so passing display names like 'mxie (Melissa Xie)' prevented deduplication against search result keys like 'mxie'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- 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 31b253d9b6d..1b7c42be3b6 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -317,7 +317,7 @@ func EditFieldsSurvey(p EditPrompter, editable *Editable, editorCommand string) editable.Reviewers.Value, err = p.MultiSelectWithSearch( "Reviewers", "Search reviewers", - editable.Reviewers.Default, + editable.Reviewers.DefaultLogins, // No persistent options - teams are included in search results []string{}, editable.ReviewerSearchFunc) From 24fb7657cd9fd0460ac57ed33a767227073bac95 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 5 Mar 2026 15:47:24 -0700 Subject: [PATCH 27/28] Exclude PR author from reviewer candidates in SuggestedReviewerActors Add author { login } to the SuggestedReviewerActors GraphQL query and pre-seed the seen map with the author login so they are excluded from all sources (suggestions, collaborators, teams). Previously the author was only skipped via the isAuthor flag in the suggestions loop but could still appear as a collaborator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr_review.go | 7 +++++++ api/queries_pr_test.go | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index f6ef2a74e39..3d0132cbddb 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -413,6 +413,9 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, type responseData struct { Node struct { PullRequest struct { + Author struct { + Login string + } SuggestedActors struct { Nodes []struct { IsAuthor bool @@ -472,7 +475,11 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, // 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. + // Pre-seed seen with the PR author since you cannot review your own PR. seen := make(map[string]bool) + if authorLogin := result.Node.PullRequest.Author.Login; authorLogin != "" { + seen[authorLogin] = true + } var candidates []ReviewerCandidate const baseQuota = 5 diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 7b7727f3e7e..8fff120bbac 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -160,7 +160,7 @@ func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams return fmt.Sprintf(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [%s]}}, + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [%s]}}, "repository": { "collaborators": {"nodes": [%s]}, "collaboratorsTotalCount": {"totalCount": %d} @@ -235,7 +235,7 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "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"}} @@ -255,6 +255,34 @@ func TestSuggestedReviewerActors(t *testing.T) { expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"}, expectedMore: 8, }, + { + name: "author excluded from collaborators", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"author": {"login": "theauthor"}, "suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": { + "collaborators": {"nodes": [ + {"login": "theauthor", "name": "The Author"}, + {"login": "c1", "name": "C1"} + ]}, + "collaboratorsTotalCount": {"totalCount": 5} + }, + "organization": { + "teams": {"nodes": []}, + "teamsTotalCount": {"totalCount": 0} + } + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"s1", "c1"}, + expectedMore: 5, + }, { name: "deduplication across sources", httpStubs: func(reg *httpmock.Registry) { @@ -263,7 +291,7 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} ]}}, "repository": { @@ -291,7 +319,7 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, "repository": { @@ -314,7 +342,7 @@ func TestSuggestedReviewerActors(t *testing.T) { httpmock.GraphQL(`query SuggestedReviewerActors\b`), httpmock.StringResponse(`{ "data": { - "node": {"suggestedReviewerActors": {"nodes": [ + "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [ {"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}, {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, From 7382b86c1a5e983bcc1792f2145c659cadc738a0 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 6 Mar 2026 09:47:47 -0700 Subject: [PATCH 28/28] Fetch org teams via repository.owner inline fragment Replace the top-level organization(login: $owner) query with repository.owner { ... on Organization { teams } }. This uses GraphQL inline fragments to conditionally fetch team data only when the repo owner is an Organization, eliminating the need to handle 'Could not resolve to an Organization' errors for personal repos. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_pr_review.go | 66 +++++++++++++++++++++------------------- api/queries_pr_test.go | 58 +++++++++++------------------------ 2 files changed, 52 insertions(+), 72 deletions(-) diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index 3d0132cbddb..a6fa34f9e88 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -407,8 +407,8 @@ func RequestReviewsByLogin(client *Client, repo ghrepo.Interface, prID string, u // 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. + // Organization teams are fetched via repository.owner inline fragment, which + // gracefully returns empty data for personal (User-owned) repos. // We also fetch unfiltered total counts via aliases for the "X more" display. type responseData struct { Node struct { @@ -435,6 +435,19 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } `graphql:"... on PullRequest"` } `graphql:"node(id: $id)"` Repository struct { + Owner struct { + TypeName string `graphql:"__typename"` + Organization struct { + Teams struct { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"... on Organization"` + } Collaborators struct { Nodes []struct { Login string @@ -445,16 +458,6 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, TotalCount int } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` } `graphql:"repository(owner: $owner, name: $name)"` - Organization struct { - Teams struct { - Nodes []struct { - Slug string - } - } `graphql:"teams(first: 10, query: $query)"` - TeamsTotalCount struct { - TotalCount int - } `graphql:"teamsTotalCount: teams(first: 0)"` - } `graphql:"organization(login: $owner)"` } variables := map[string]interface{}{ @@ -466,9 +469,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, 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) { + if err != nil { return nil, 0, err } @@ -531,7 +532,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded) teamsAdded := 0 ownerName := repo.RepoOwner() - for _, t := range result.Organization.Teams.Nodes { + for _, t := range result.Repository.Owner.Organization.Teams.Nodes { if teamsAdded >= teamsQuota { break } @@ -547,7 +548,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) - moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Organization.TeamsTotalCount.TotalCount + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Repository.Owner.Organization.TeamsTotalCount.TotalCount return candidates, moreResults, nil } @@ -584,6 +585,19 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query } `graphql:"suggestedReviewerActors(first: 10)"` } } `graphql:"pullRequests(first: 1, states: [OPEN])"` + Owner struct { + TypeName string `graphql:"__typename"` + Organization struct { + Teams struct { + Nodes []struct { + Slug string + } + } `graphql:"teams(first: 10, query: $query)"` + TeamsTotalCount struct { + TotalCount int + } `graphql:"teamsTotalCount: teams(first: 0)"` + } `graphql:"... on Organization"` + } Collaborators struct { Nodes []struct { Login string @@ -594,16 +608,6 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query TotalCount int } `graphql:"collaboratorsTotalCount: collaborators(first: 0)"` } `graphql:"repository(owner: $owner, name: $name)"` - Organization struct { - Teams struct { - Nodes []struct { - Slug string - } - } `graphql:"teams(first: 10, query: $query)"` - TeamsTotalCount struct { - TotalCount int - } `graphql:"teamsTotalCount: teams(first: 0)"` - } `graphql:"organization(login: $owner)"` } variables := map[string]interface{}{ @@ -614,9 +618,7 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query var result responseData err := client.Query(repo.RepoHost(), "SuggestedReviewerActorsForRepo", &result, variables) - // Handle the case where the owner is not an organization - the query still returns - // partial data (repository), so we can continue processing. - if err != nil && !strings.Contains(err.Error(), errorResolvingOrganization) { + if err != nil { return nil, 0, err } @@ -661,7 +663,7 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query teamsQuota := baseQuota + (baseQuota - collaboratorsAdded) teamsAdded := 0 ownerName := repo.RepoOwner() - for _, t := range result.Organization.Teams.Nodes { + for _, t := range result.Repository.Owner.Organization.Teams.Nodes { if teamsAdded >= teamsQuota { break } @@ -677,7 +679,7 @@ func SuggestedReviewerActorsForRepo(client *Client, repo ghrepo.Interface, query } // MoreResults uses unfiltered total counts (teams will be 0 for personal repos) - moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Organization.TeamsTotalCount.TotalCount + moreResults := result.Repository.CollaboratorsTotalCount.TotalCount + result.Repository.Owner.Organization.TeamsTotalCount.TotalCount return candidates, moreResults, nil } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 8fff120bbac..633b9a8c35f 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -162,16 +162,14 @@ func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams "data": { "node": {"author": {"login": "testauthor"}, "suggestedReviewerActors": {"nodes": [%s]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}}, "collaborators": {"nodes": [%s]}, "collaboratorsTotalCount": {"totalCount": %d} - }, - "organization": { - "teams": {"nodes": [%s]}, - "teamsTotalCount": {"totalCount": %d} } } - }`, strings.Join(suggestionNodes, ","), strings.Join(collabNodes, ","), totalCollabs, - strings.Join(teamNodes, ","), totalTeams) + }`, strings.Join(suggestionNodes, ","), + strings.Join(teamNodes, ","), totalTeams, + strings.Join(collabNodes, ","), totalCollabs) } func TestSuggestedReviewerActors(t *testing.T) { @@ -241,12 +239,9 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 3}}, "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, "collaboratorsTotalCount": {"totalCount": 5} - }, - "organization": { - "teams": {"nodes": [{"slug": "team1"}]}, - "teamsTotalCount": {"totalCount": 3} } } }`)) @@ -271,10 +266,6 @@ func TestSuggestedReviewerActors(t *testing.T) { {"login": "c1", "name": "C1"} ]}, "collaboratorsTotalCount": {"totalCount": 5} - }, - "organization": { - "teams": {"nodes": []}, - "teamsTotalCount": {"totalCount": 0} } } }`)) @@ -295,15 +286,12 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": [{"slug": "team1"}]}, "teamsTotalCount": {"totalCount": 5}}, "collaborators": {"nodes": [ {"login": "shareduser", "name": "Shared"}, {"login": "c1", "name": "C1"} ]}, "collaboratorsTotalCount": {"totalCount": 10} - }, - "organization": { - "teams": {"nodes": [{"slug": "team1"}]}, - "teamsTotalCount": {"totalCount": 5} } } }`)) @@ -323,12 +311,11 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, "repository": { + "owner": {"__typename": "User"}, "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, "collaboratorsTotalCount": {"totalCount": 3} - }, - "organization": null - }, - "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + } + } }`)) }, expectedCount: 2, @@ -347,12 +334,9 @@ func TestSuggestedReviewerActors(t *testing.T) { {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} ]}}, "repository": { + "owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}}, "collaborators": {"nodes": []}, "collaboratorsTotalCount": {"totalCount": 5} - }, - "organization": { - "teams": {"nodes": []}, - "teamsTotalCount": {"totalCount": 0} } } }`)) @@ -421,16 +405,14 @@ func mockReviewerResponseForRepoWithCopilot(collabs, teams, totalCollabs, totalT "viewer": {"login": "testuser"}, "repository": { %s, + "owner": {"__typename": "Organization", "teams": {"nodes": [%s]}, "teamsTotalCount": {"totalCount": %d}}, "collaborators": {"nodes": [%s]}, "collaboratorsTotalCount": {"totalCount": %d} - }, - "organization": { - "teams": {"nodes": [%s]}, - "teamsTotalCount": {"totalCount": %d} } } - }`, pullRequestsJSON, strings.Join(collabNodes, ","), totalCollabs, - strings.Join(teamNodes, ","), totalTeams) + }`, pullRequestsJSON, + strings.Join(teamNodes, ","), totalTeams, + strings.Join(collabNodes, ","), totalCollabs) } func TestSuggestedReviewerActorsForRepo(t *testing.T) { @@ -484,12 +466,11 @@ func TestSuggestedReviewerActorsForRepo(t *testing.T) { "data": { "repository": { "pullRequests": {"nodes": []}, + "owner": {"__typename": "User"}, "collaborators": {"nodes": [{"login": "c1", "name": "C1"}]}, "collaboratorsTotalCount": {"totalCount": 3} - }, - "organization": null - }, - "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + } + } }`)) }, expectedCount: 1, @@ -541,16 +522,13 @@ func TestSuggestedReviewerActorsForRepo(t *testing.T) { "viewer": {"login": "c2"}, "repository": { "pullRequests": {"nodes": []}, + "owner": {"__typename": "Organization", "teams": {"nodes": []}, "teamsTotalCount": {"totalCount": 0}}, "collaborators": {"nodes": [ {"login": "c1", "name": "C1"}, {"login": "c2", "name": "C2"}, {"login": "c3", "name": "C3"} ]}, "collaboratorsTotalCount": {"totalCount": 3} - }, - "organization": { - "teams": {"nodes": []}, - "teamsTotalCount": {"totalCount": 0} } } }`))