From 87468f40db49324d61eac3c91e55d050aa04212c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Mon, 29 Sep 2025 13:50:11 -0600 Subject: [PATCH 1/9] Refactor PR reviewer editing to use REST API and optimize team fetch Switches pull request reviewer add/remove operations from GraphQL to the REST API, enabling separate add and remove calls for reviewers and teams. Refactors reviewer editing logic to avoid fetching organization teams unless required for interactive editing, improving performance for non-interactive flows. Updates tests and supporting code to reflect the new reviewer management and metadata fetching behavior. --- api/queries_pr.go | 60 ++++++-- pkg/cmd/pr/edit/edit.go | 84 +++++++++--- pkg/cmd/pr/edit/edit_test.go | 249 ++++++++++++++++++++++++++-------- pkg/cmd/pr/shared/editable.go | 55 ++------ 4 files changed, 325 insertions(+), 123 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index b3373a903..0d0fed03d 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1,6 +1,8 @@ package api import ( + "bytes" + "encoding/json" "fmt" "net/http" "net/url" @@ -629,17 +631,55 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } -func UpdatePullRequestReviews(client *Client, repo ghrepo.Interface, params githubv4.RequestReviewsInput) error { - var mutation struct { - RequestReviews struct { - PullRequest struct { - ID string - } - } `graphql:"requestReviews(input: $input)"` +// AddPullRequestReviews updates the requested reviewers on a pull request using the REST API. +func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { + if len(users) == 0 && len(teams) == 0 { + return nil } - variables := map[string]interface{}{"input": params} - err := client.Mutate(repo.RepoHost(), "PullRequestUpdateRequestReviews", &mutation, variables) - return err + + path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber) + body := struct { + Reviewers []string `json:"reviewers,omitempty"` + TeamReviewers []string `json:"team_reviewers,omitempty"` + }{ + Reviewers: users, + TeamReviewers: 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. +func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { + if len(users) == 0 && len(teams) == 0 { + return nil + } + + if users == nil { + users = []string{} + } + if teams == nil { + teams = []string{} + } + + path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber) + body := struct { + Reviewers []string `json:"reviewers"` + TeamReviewers []string `json:"team_reviewers"` + }{ + Reviewers: users, + TeamReviewers: 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) } func UpdatePullRequestBranch(client *Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestBranchInput) error { diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index b196546cd..de65f1c72 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,6 +3,8 @@ package edit import ( "fmt" "net/http" + "slices" + "strings" "time" "github.com/MakeNowJust/heredoc" @@ -13,6 +15,7 @@ import ( shared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/set" "github.com/shurcooL/githubv4" "github.com/spf13/cobra" "golang.org/x/sync/errgroup" @@ -237,7 +240,7 @@ func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, - Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, + Fields: []string{"id", "author", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, Detector: opts.Detector, } @@ -298,6 +301,18 @@ func editRun(opts *EditOptions) error { } if opts.Interactive { + // Remove PR author from reviewer options + // as it is not a valid option for a reviewer. + // The REST API will return an error if we + // attempt to add the PR author as a reviewer. + // However, the GraphQL API will silently ignore it. + if editable.Reviewers.Edited { + s := set.NewStringSet() + s.AddValues(editable.Reviewers.Options) + s.Remove(pr.Author.Login) + editable.Reviewers.Options = s.ToSlice() + } + editorCommand, err := opts.EditorRetriever.Retrieve() if err != nil { return err @@ -309,7 +324,7 @@ func editRun(opts *EditOptions) error { } opts.IO.StartProgressIndicator() - err = updatePullRequest(httpClient, repo, pr.ID, editable) + err = updatePullRequest(httpClient, repo, pr.ID, pr.Number, editable) opts.IO.StopProgressIndicator() if err != nil { return err @@ -320,36 +335,71 @@ func editRun(opts *EditOptions) error { return nil } -func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error { +func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, number int, editable shared.Editable) error { var wg errgroup.Group wg.Go(func() error { return shared.UpdateIssue(httpClient, repo, id, true, editable) }) if editable.Reviewers.Edited { wg.Go(func() error { - return updatePullRequestReviews(httpClient, repo, id, editable) + return updatePullRequestReviews(httpClient, repo, number, editable) }) } return wg.Wait() } -func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error { - userIds, teamIds, err := editable.ReviewerIds() - if err != nil { - return err - } - if userIds == nil && teamIds == nil { +func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, number int, editable shared.Editable) error { + if !editable.Reviewers.Edited { return nil } - union := githubv4.Boolean(false) - reviewsRequestParams := githubv4.RequestReviewsInput{ - PullRequestID: id, - Union: &union, - UserIDs: ghIds(userIds), - TeamIDs: ghIds(teamIds), + + // Rebuild the Value slice from non-interactive flag input. + if len(editable.Reviewers.Add) != 0 || len(editable.Reviewers.Remove) != 0 { + s := set.NewStringSet() + s.AddValues(editable.Reviewers.Add) + s.AddValues(editable.Reviewers.Default) + s.RemoveValues(editable.Reviewers.Remove) + editable.Reviewers.Value = s.ToSlice() } + + var addUsers []string + var addTeams []string + for _, r := range editable.Reviewers.Value { + if strings.ContainsRune(r, '/') { + teamSlug := strings.Split(r, "/")[1] + addTeams = append(addTeams, teamSlug) + } else { + addUsers = append(addUsers, r) + } + } + + // Reviewers in Default but not in the Value have been removed interactively. + var toRemove []string + for _, r := range editable.Reviewers.Default { + if !slices.Contains(editable.Reviewers.Value, r) { + toRemove = append(toRemove, r) + } + } + var removeUsers []string + var removeTeams []string + for _, r := range toRemove { + if strings.ContainsRune(r, '/') { + teamSlug := strings.Split(r, "/")[1] + removeTeams = append(removeTeams, teamSlug) + } else { + removeUsers = append(removeUsers, r) + } + } + client := api.NewClientFromHTTP(httpClient) - return api.UpdatePullRequestReviews(client, repo, reviewsRequestParams) + wg := errgroup.Group{} + wg.Go(func() error { + return api.AddPullRequestReviews(client, repo, number, addUsers, addTeams) + }) + wg.Go(func() error { + return api.RemovePullRequestReviews(client, repo, number, removeUsers, removeTeams) + }) + return wg.Wait() } type Surveyor interface { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index bb468a307..803f506bc 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -354,7 +354,7 @@ func Test_editRun(t *testing.T) { tests := []struct { name string input *EditOptions - httpStubs func(*httpmock.Registry) + httpStubs func(*httpmock.Registry, *testing.T) stdout string stderr string }{ @@ -411,11 +411,11 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, false) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) - mockPullRequestReviewersUpdate(reg) + mockPullRequestAddReviewers(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -469,8 +469,8 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, true) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) @@ -483,8 +483,19 @@ func Test_editRun(t *testing.T) { input: &EditOptions{ Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", - Finder: shared.NewMockFinder("123", &api.PullRequest{ + Finder: shared.NewMockFinder("123", &api.PullRequest{ // include existing reviewers so removal logic triggers URL: "https://github.com/OWNER/REPO/pull/123", + ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{ + {RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "external", Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "hubot"}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "dependabot"}}, + }}, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -501,8 +512,9 @@ func Test_editRun(t *testing.T) { Edited: true, }, Reviewers: shared.EditableSlice{ - Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"}, - Edited: true, + Default: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"}, + Remove: []string{"OWNER/core", "OWNER/external", "monalisa", "hubot", "dependabot"}, + Edited: true, }, Assignees: shared.EditableAssignees{ EditableSlice: shared.EditableSlice{ @@ -530,16 +542,107 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, false) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestReviewersUpdate(reg) + mockPullRequestRemoveReviewers(reg) mockPullRequestUpdateLabels(reg) mockPullRequestUpdateActorAssignees(reg) mockProjectV2ItemUpdate(reg) }, stdout: "https://github.com/OWNER/REPO/pull/123\n", }, + // Conditional team fetching cases + { + name: "non-interactive add only user reviewers skips team fetch", + input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Reviewers: shared.EditableSlice{Add: []string{"monalisa", "hubot"}, Edited: true}, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + // reviewers only (users), no team reviewers fetched + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // explicitly assert that no OrganizationTeamList query occurs + reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) + mockPullRequestUpdate(reg) + mockPullRequestAddReviewers(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, + { + name: "non-interactive add contains team reviewers skips team fetch", + input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Reviewers: shared.EditableSlice{Add: []string{"monalisa", "OWNER/core"}, Edited: true}, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + // reviewer add includes team but non-interactive Add/Remove provided -> no team fetch + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) + mockPullRequestUpdate(reg) + mockPullRequestAddReviewers(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, + { + name: "non-interactive reviewers remove contains team skips team fetch", + input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123", ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{ + {RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}}, + }}}, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Reviewers: shared.EditableSlice{Remove: []string{"monalisa", "OWNER/core"}, Edited: true}, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) + mockPullRequestUpdate(reg) + mockPullRequestRemoveReviewers(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, + { + name: "non-interactive mutate reviewers with no change to existing team reviewers skips team fetch", + input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")), + Interactive: false, + Editable: shared.Editable{ + Reviewers: shared.EditableSlice{Add: []string{"monalisa"}, Remove: []string{"hubot"}, Default: []string{"OWNER/core"}, Edited: true}, + }, + Fetcher: testFetcher{}, + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + // reviewers only (users), no team reviewers fetched + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // explicitly assert that no OrganizationTeamList query occurs + reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) + mockPullRequestUpdate(reg) + mockPullRequestAddReviewers(reg) + }, + stdout: "https://github.com/OWNER/REPO/pull/123\n", + }, { name: "interactive", input: &EditOptions{ @@ -576,11 +679,11 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, false) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) - mockPullRequestReviewersUpdate(reg) + mockPullRequestAddReviewers(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) }, @@ -620,8 +723,9 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, true) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + // interactive but reviewers not chosen; need everything except reviewers/teams + mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) @@ -634,8 +738,19 @@ func Test_editRun(t *testing.T) { input: &EditOptions{ Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", - Finder: shared.NewMockFinder("123", &api.PullRequest{ + Finder: shared.NewMockFinder("123", &api.PullRequest{ // include existing reviewers URL: "https://github.com/OWNER/REPO/pull/123", + ReviewRequests: api.ReviewRequests{Nodes: []struct{ RequestedReviewer api.RequestedReviewer }{ + {RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "core", Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "Team", Slug: "external", Organization: struct { + Login string `json:"login"` + }{Login: "OWNER"}}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "monalisa"}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "hubot"}}, + {RequestedReviewer: api.RequestedReviewer{TypeName: "User", Login: "dependabot"}}, + }}, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{ @@ -665,10 +780,10 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry) { - mockRepoMetadata(reg, false) + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) - mockPullRequestReviewersUpdate(reg) + mockPullRequestRemoveReviewers(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) mockProjectV2ItemUpdate(reg) @@ -712,7 +827,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry) { + httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` @@ -759,7 +874,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry) { + httpStubs: func(reg *httpmock.Registry, t *testing.T) { // Notice there is no call to mockReplaceActorsForAssignable() // and no GraphQL call to RepositoryAssignableActors below. reg.Register( @@ -787,7 +902,7 @@ func Test_editRun(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - tt.httpStubs(reg) + tt.httpStubs(reg, t) httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } @@ -804,10 +919,21 @@ func Test_editRun(t *testing.T) { } } -func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` +type mockRepoMetadataOptions struct { + reviewers bool + teamReviewers bool // reviewers must also be true for this to have an effect. + assignees bool + labels bool + projects bool // includes both legacy (v1) and v2 + milestones bool +} + +func mockRepoMetadata(reg *httpmock.Registry, opt mockRepoMetadataOptions) { + // Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ActorAssignees enabled. + if opt.reviewers || opt.assignees { + reg.Register( + httpmock.GraphQL(`query RepositoryAssignableActors\b`), + httpmock.StringResponse(` { "data": { "repository": { "suggestedActors": { "nodes": [ { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, @@ -816,9 +942,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryLabelList\b`), - httpmock.StringResponse(` + } + if opt.labels { + reg.Register( + httpmock.GraphQL(`query RepositoryLabelList\b`), + httpmock.StringResponse(` { "data": { "repository": { "labels": { "nodes": [ { "name": "feature", "id": "FEATUREID" }, @@ -829,9 +957,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryMilestoneList\b`), - httpmock.StringResponse(` + } + if opt.milestones { + reg.Register( + httpmock.GraphQL(`query RepositoryMilestoneList\b`), + httpmock.StringResponse(` { "data": { "repository": { "milestones": { "nodes": [ { "title": "GA", "id": "GAID" }, @@ -840,9 +970,11 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryProjectList\b`), - httpmock.StringResponse(` + } + if opt.projects { + reg.Register( + httpmock.GraphQL(`query RepositoryProjectList\b`), + httpmock.StringResponse(` { "data": { "repository": { "projects": { "nodes": [ { "name": "Cleanup", "id": "CLEANUPID" }, @@ -851,9 +983,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationProjectList\b`), - httpmock.StringResponse(` + reg.Register( + httpmock.GraphQL(`query OrganizationProjectList\b`), + httpmock.StringResponse(` { "data": { "organization": { "projects": { "nodes": [ { "name": "Triage", "id": "TRIAGEID" } @@ -861,9 +993,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryProjectV2List\b`), - httpmock.StringResponse(` + reg.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` { "data": { "repository": { "projectsV2": { "nodes": [ { "title": "CleanupV2", "id": "CLEANUPV2ID" }, @@ -872,9 +1004,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query OrganizationProjectV2List\b`), - httpmock.StringResponse(` + reg.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` { "data": { "organization": { "projectsV2": { "nodes": [ { "title": "TriageV2", "id": "TRIAGEV2ID" } @@ -882,9 +1014,9 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - reg.Register( - httpmock.GraphQL(`query UserProjectV2List\b`), - httpmock.StringResponse(` + reg.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` { "data": { "viewer": { "projectsV2": { "nodes": [ { "title": "MonalisaV2", "id": "MONALISAV2ID" } @@ -892,7 +1024,8 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) - if !skipReviewers { + } + if opt.teamReviewers && opt.reviewers { // teams only relevant if reviewers edited reg.Register( httpmock.GraphQL(`query OrganizationTeamList\b`), httpmock.StringResponse(` @@ -904,11 +1037,13 @@ func mockRepoMetadata(reg *httpmock.Registry, skipReviewers bool) { "pageInfo": { "hasNextPage": false } } } } } `)) + } + if opt.reviewers { // Current user fetched only when reviewers requested reg.Register( httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(` - { "data": { "viewer": { "login": "monalisa" } } } - `)) + { "data": { "viewer": { "login": "monalisa" } } } + `)) } } @@ -927,9 +1062,15 @@ func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) { ) } -func mockPullRequestReviewersUpdate(reg *httpmock.Registry) { +func mockPullRequestAddReviewers(reg *httpmock.Registry) { reg.Register( - httpmock.GraphQL(`mutation PullRequestUpdateRequestReviews\b`), + httpmock.REST("POST", "repos/OWNER/REPO/pulls/0/requested_reviewers"), + httpmock.StringResponse(`{}`)) +} + +func mockPullRequestRemoveReviewers(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/pulls/0/requested_reviewers"), httpmock.StringResponse(`{}`)) } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 9adbeb47c..7d0805a80 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -2,7 +2,6 @@ package shared import ( "fmt" - "strings" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" @@ -78,37 +77,6 @@ func (e Editable) BodyValue() *string { return &e.Body.Value } -func (e Editable) ReviewerIds() (*[]string, *[]string, error) { - if !e.Reviewers.Edited { - return nil, nil, nil - } - if len(e.Reviewers.Add) != 0 || len(e.Reviewers.Remove) != 0 { - s := set.NewStringSet() - s.AddValues(e.Reviewers.Default) - s.AddValues(e.Reviewers.Add) - s.RemoveValues(e.Reviewers.Remove) - e.Reviewers.Value = s.ToSlice() - } - var userReviewers []string - var teamReviewers []string - for _, r := range e.Reviewers.Value { - if strings.ContainsRune(r, '/') { - teamReviewers = append(teamReviewers, r) - } else { - userReviewers = append(userReviewers, r) - } - } - userIds, err := e.Metadata.MembersToIDs(userReviewers) - if err != nil { - return nil, nil, err - } - teamIds, err := e.Metadata.TeamsToIDs(teamReviewers) - if err != nil { - return nil, nil, err - } - return &userIds, &teamIds, nil -} - func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]string, error) { if !e.Assignees.Edited { return nil, nil @@ -428,17 +396,20 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error { } func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) error { + // Determine whether to fetch organization teams. + // Interactive reviewer editing (Edited true, but no Add/Remove slices) still needs + // team data for selection UI. For non-interactive flows, we never need to fetch teams. + teamReviewers := false + if editable.Reviewers.Edited { + // This is likely an interactive flow since edited is set but no mutations to + // Add/Remove slices, so we need to load the teams. + if len(editable.Reviewers.Add) == 0 && len(editable.Reviewers.Remove) == 0 { + teamReviewers = true + } + } input := api.RepoMetadataInput{ - Reviewers: editable.Reviewers.Edited, - // TeamReviewers is always true if Reviewers is true because - // this is the existing `pr edit` behavior. This means - // always fetch teams. - // TODO: evaluate whether this can follow the same logic as - // `pr create` to conditionally fetch teams if a reviewer contains - // a slash. - // See https://github.com/cli/cli/blob/449920b40fc8a5015d1578ca10a301aa385a1914/pkg/cmd/pr/shared/params.go#L67-L71 - // See https://github.com/cli/cli/issues/11360 - TeamReviewers: editable.Reviewers.Edited, + Reviewers: editable.Reviewers.Edited, + TeamReviewers: teamReviewers, Assignees: editable.Assignees.Edited, ActorAssignees: editable.Assignees.ActorAssignees, Labels: editable.Labels.Edited, From 7094a55eec08fc5224ed4b1926684dbb81cf214e Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 1 Oct 2025 15:47:39 -0600 Subject: [PATCH 2/9] Remove unused ghIds function and githubv4 import Deleted the unused ghIds helper function and the associated githubv4 import from edit.go to clean up the codebase. --- pkg/cmd/pr/edit/edit.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index de65f1c72..2c7f25018 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -16,7 +16,6 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/set" - "github.com/shurcooL/githubv4" "github.com/spf13/cobra" "golang.org/x/sync/errgroup" ) @@ -440,14 +439,3 @@ type editorRetriever struct { func (e editorRetriever) Retrieve() (string, error) { return cmdutil.DetermineEditor(e.config) } - -func ghIds(s *[]string) *[]githubv4.ID { - if s == nil { - return nil - } - ids := make([]githubv4.ID, len(*s)) - for i, v := range *s { - ids[i] = v - } - return &ids -} From 57fce1dc3aa1044dd8872061e58b2f6bd9ef4bf3 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 1 Oct 2025 15:55:12 -0600 Subject: [PATCH 3/9] Escape repo owner and name in PR reviewer API paths Updated AddPullRequestReviews and RemovePullRequestReviews to use url.PathEscape for repo owner and name in API paths. This ensures correct handling of special characters in repository identifiers. --- api/queries_pr.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 0d0fed03d..60e6834ac 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -637,7 +637,12 @@ func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, return nil } - path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber) + 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,omitempty"` TeamReviewers []string `json:"team_reviewers,omitempty"` @@ -666,7 +671,12 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in teams = []string{} } - path := fmt.Sprintf("repos/%s/%s/pulls/%d/requested_reviewers", repo.RepoOwner(), repo.RepoName(), prNumber) + 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"` From 848faf81155e72c70ffc28fb7dbb71f3157ace5c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 1 Oct 2025 16:02:05 -0600 Subject: [PATCH 4/9] Refactor reviewer partitioning in PR edit command Extracted logic for splitting reviewer identifiers into users and teams into a new helper function, partitionUsersAndTeams. Updated updatePullRequestReviews to use this function for both adding and removing reviewers, improving code clarity and maintainability. Also clarified comments regarding PR author handling. --- api/queries_pr.go | 2 +- pkg/cmd/pr/edit/edit.go | 45 ++++++++++++++++++----------------------- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 60e6834ac..010a915e2 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -631,7 +631,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } -// AddPullRequestReviews updates the requested reviewers on a pull request using the REST API. +// AddPullRequestReviews adds the given user and team reviewers to a pull request using the REST API. func AddPullRequestReviews(client *Client, repo ghrepo.Interface, prNumber int, users, teams []string) error { if len(users) == 0 && len(teams) == 0 { return nil diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 2c7f25018..3e00bfab9 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -300,11 +300,8 @@ func editRun(opts *EditOptions) error { } if opts.Interactive { - // Remove PR author from reviewer options - // as it is not a valid option for a reviewer. - // The REST API will return an error if we - // attempt to add the PR author as a reviewer. - // However, the GraphQL API will silently ignore it. + // Remove PR author from reviewer options; + // REST API errors if author is included (GraphQL silently ignores). if editable.Reviewers.Edited { s := set.NewStringSet() s.AddValues(editable.Reviewers.Options) @@ -361,16 +358,7 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, nu editable.Reviewers.Value = s.ToSlice() } - var addUsers []string - var addTeams []string - for _, r := range editable.Reviewers.Value { - if strings.ContainsRune(r, '/') { - teamSlug := strings.Split(r, "/")[1] - addTeams = append(addTeams, teamSlug) - } else { - addUsers = append(addUsers, r) - } - } + addUsers, addTeams := partitionUsersAndTeams(editable.Reviewers.Value) // Reviewers in Default but not in the Value have been removed interactively. var toRemove []string @@ -379,16 +367,7 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, nu toRemove = append(toRemove, r) } } - var removeUsers []string - var removeTeams []string - for _, r := range toRemove { - if strings.ContainsRune(r, '/') { - teamSlug := strings.Split(r, "/")[1] - removeTeams = append(removeTeams, teamSlug) - } else { - removeUsers = append(removeUsers, r) - } - } + removeUsers, removeTeams := partitionUsersAndTeams(toRemove) client := api.NewClientFromHTTP(httpClient) wg := errgroup.Group{} @@ -439,3 +418,19 @@ type editorRetriever struct { func (e editorRetriever) Retrieve() (string, error) { return cmdutil.DetermineEditor(e.config) } + +// partitionUsersAndTeams splits reviewer identifiers into user logins and team slugs. +// Team identifiers are in the form "org/slug"; only the slug portion is returned for teams. +func partitionUsersAndTeams(values []string) (users []string, teams []string) { + for _, v := range values { + if strings.ContainsRune(v, '/') { + parts := strings.SplitN(v, "/", 2) + if len(parts) == 2 && parts[1] != "" { + teams = append(teams, parts[1]) + } + } else if v != "" { + users = append(users, v) + } + } + return +} From 52bb1dec30b33ab3ef51d06c799aa8b6ef91434b Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 1 Oct 2025 16:09:33 -0600 Subject: [PATCH 5/9] Fix typo in error message for required flags Corrected '--tile' to '--title' in the error message shown when required flags are missing in non-interactive mode. --- pkg/cmd/pr/edit/edit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 3e00bfab9..e593b791a 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -172,7 +172,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } if opts.Interactive && !opts.IO.CanPrompt() { - return cmdutil.FlagErrorf("--tile, --body, --reviewer, --assignee, --label, --project, or --milestone required when not running interactively") + return cmdutil.FlagErrorf("--title, --body, --reviewer, --assignee, --label, --project, or --milestone required when not running interactively") } if runF != nil { From 66fae72872c27037e15df92ae7a12da7ceb718f5 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 1 Oct 2025 16:12:06 -0600 Subject: [PATCH 6/9] Remove default empty slices in RemovePullRequestReviews Eliminates unnecessary initialization of users and teams to empty slices in RemovePullRequestReviews. Also updates the request body struct to use 'omitempty' for reviewers and team_reviewers, ensuring empty fields are omitted from the JSON payload. --- api/queries_pr.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 010a915e2..4262738c3 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -664,13 +664,6 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in return nil } - if users == nil { - users = []string{} - } - if teams == nil { - teams = []string{} - } - path := fmt.Sprintf( "repos/%s/%s/pulls/%d/requested_reviewers", url.PathEscape(repo.RepoOwner()), @@ -678,8 +671,8 @@ func RemovePullRequestReviews(client *Client, repo ghrepo.Interface, prNumber in prNumber, ) body := struct { - Reviewers []string `json:"reviewers"` - TeamReviewers []string `json:"team_reviewers"` + Reviewers []string `json:"reviewers,omitempty"` + TeamReviewers []string `json:"team_reviewers,omitempty"` }{ Reviewers: users, TeamReviewers: teams, From bf728893fa2348b2b2c200db237498cfe9adb036 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 2 Oct 2025 09:54:01 -0600 Subject: [PATCH 7/9] Fix argument order in httpStubs test functions Swaps the argument order of the httpStubs functions in edit_test.go to match the expected (t *testing.T, reg *httpmock.Registry) signature. This improves consistency and prevents potential confusion or errors when calling these test helpers. --- pkg/cmd/pr/edit/edit_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 803f506bc..5ed4d4fdd 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -354,7 +354,7 @@ func Test_editRun(t *testing.T) { tests := []struct { name string input *EditOptions - httpStubs func(*httpmock.Registry, *testing.T) + httpStubs func(*testing.T, *httpmock.Registry) stdout string stderr string }{ @@ -411,7 +411,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) @@ -469,7 +469,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) @@ -542,7 +542,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestRemoveReviewers(reg) @@ -565,7 +565,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { // reviewers only (users), no team reviewers fetched mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) // explicitly assert that no OrganizationTeamList query occurs @@ -587,7 +587,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { // reviewer add includes team but non-interactive Add/Remove provided -> no team fetch mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) @@ -613,7 +613,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) @@ -633,7 +633,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { // reviewers only (users), no team reviewers fetched mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) // explicitly assert that no OrganizationTeamList query occurs @@ -679,7 +679,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) @@ -723,7 +723,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { // interactive but reviewers not chosen; need everything except reviewers/teams mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) @@ -780,7 +780,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true, teamReviewers: true, assignees: true, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestRemoveReviewers(reg) @@ -827,7 +827,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`query RepositoryAssignableActors\b`), httpmock.StringResponse(` @@ -874,7 +874,7 @@ func Test_editRun(t *testing.T) { }, Fetcher: testFetcher{}, }, - httpStubs: func(reg *httpmock.Registry, t *testing.T) { + httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Notice there is no call to mockReplaceActorsForAssignable() // and no GraphQL call to RepositoryAssignableActors below. reg.Register( @@ -902,7 +902,7 @@ func Test_editRun(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - tt.httpStubs(reg, t) + tt.httpStubs(t, reg) httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } From 8d8594609896f662e345a8b4f67e39fb685e5287 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 2 Oct 2025 09:55:45 -0600 Subject: [PATCH 8/9] Apply suggestion from @babakks Co-authored-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 5ed4d4fdd..0fa74d030 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -590,6 +590,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { // reviewer add includes team but non-interactive Add/Remove provided -> no team fetch mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) mockPullRequestAddReviewers(reg) From 43e834a6919d2d1e1fd5904e0ba0058b0ef0aaeb Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 2 Oct 2025 09:55:58 -0600 Subject: [PATCH 9/9] Apply suggestion from @babakks Co-authored-by: Babak K. Shandiz --- pkg/cmd/pr/edit/edit_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 0fa74d030..47ce30f22 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -616,6 +616,7 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: true}) + // explicitly assert that no OrganizationTeamList query occurs reg.Exclude(t, httpmock.GraphQL(`query OrganizationTeamList\b`)) mockPullRequestUpdate(reg) mockPullRequestRemoveReviewers(reg)