Repository: cli/cli

Issue:
Decouple arg/URL parsing from PR finder in `pr` commands
Follow-up to #11057

As a follow-up to the temporary fix we made in #11057, we should do a proper fix by decoupling arg parsing from PR finder. @williammartin has already explained what needs to be done in the PR's description and the code.

Since this is a refactoring task, there should be no observable changes. Specifically, The affected/refactored `pr` subcommands (e.g. `pr edit` or `pr view`) must support PR URLs as argument.

### Expected Outcomes

- Existing tests related to affected `pr` commands will still pass

Discussion:

Solution patch:
diff --git a/api/queries_pr.go b/api/queries_pr.go
index 525418a11..1d394e864 100644
--- a/api/queries_pr.go
+++ b/api/queries_pr.go
@@ -85,15 +85,25 @@ type PullRequest struct {
 
 	Assignees      Assignees
 	AssignedActors AssignedActors
-	Labels         Labels
-	ProjectCards   ProjectCards
-	ProjectItems   ProjectItems
-	Milestone      *Milestone
-	Comments       Comments
-	ReactionGroups ReactionGroups
-	Reviews        PullRequestReviews
-	LatestReviews  PullRequestReviews
-	ReviewRequests ReviewRequests
+	// AssignedActorsUsed is a GIGANTIC hack to carry around whether we expected AssignedActors to be requested
+	// on this PR. This is required because the Feature Detection of support for AssignedActors occurs inside the
+	// PR Finder, but knowledge of support is required at the command level. However, we can't easily construct
+	// the feature detector at the command level because it needs knowledge of the BaseRepo, which is only available
+	// inside the PR Finder. This is bad and we should feel bad.
+	//
+	// The right solution is to extract argument parsing from the PR Finder into each command, so that we have access
+	// to the BaseRepo and can construct the feature detector there. This is what happens in the issue commands with
+	// `shared.ParseIssueFromArg`.
+	AssignedActorsUsed bool
+	Labels             Labels
+	ProjectCards       ProjectCards
+	ProjectItems       ProjectItems
+	Milestone          *Milestone
+	Comments           Comments
+	ReactionGroups     ReactionGroups
+	Reviews            PullRequestReviews
+	LatestReviews      PullRequestReviews
+	ReviewRequests     ReviewRequests
 
 	ClosingIssuesReferences ClosingIssuesReferences
 }
diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go
index e959cde2b..b207a96fd 100644
--- a/pkg/cmd/issue/edit/edit.go
+++ b/pkg/cmd/issue/edit/edit.go
@@ -171,8 +171,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
 	cmd.Flags().StringVarP(&opts.Editable.Title.Value, "title", "t", "", "Set the new title.")
 	cmd.Flags().StringVarP(&opts.Editable.Body.Value, "body", "b", "", "Set the new body.")
 	cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file` (use \"-\" to read from standard input)")
-	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself.")
-	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself.")
+	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself, or \"@copilot\" to assign Copilot.")
+	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself, or \"@copilot\" to unassign Copilot.")
 	cmd.Flags().StringSliceVar(&opts.Editable.Labels.Add, "add-label", nil, "Add labels by `name`")
 	cmd.Flags().StringSliceVar(&opts.Editable.Labels.Remove, "remove-label", nil, "Remove labels by `name`")
 	cmd.Flags().StringSliceVar(&opts.Editable.Projects.Add, "add-project", nil, "Add the issue to projects by `title`")
diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go
index c30c4f9bb..5e1f19b18 100644
--- a/pkg/cmd/pr/edit/edit.go
+++ b/pkg/cmd/pr/edit/edit.go
@@ -3,7 +3,6 @@ package edit
 import (
 	"fmt"
 	"net/http"
-	"time"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/v2/api"
@@ -171,8 +170,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
 	cmd.Flags().StringVarP(&opts.Editable.Base.Value, "base", "B", "", "Change the base `branch` for this pull request")
 	cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Add, "add-reviewer", nil, "Add reviewers by their `login`.")
 	cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Remove, "remove-reviewer", nil, "Remove reviewers by their `login`.")
-	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself.")
-	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself.")
+	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself, or \"@copilot\" to assign Copilot.")
+	cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself, or \"@copilot\" to unassign Copilot.")
 	cmd.Flags().StringSliceVar(&opts.Editable.Labels.Add, "add-label", nil, "Add labels by `name`")
 	cmd.Flags().StringSliceVar(&opts.Editable.Labels.Remove, "remove-label", nil, "Remove labels by `name`")
 	cmd.Flags().StringSliceVar(&opts.Editable.Projects.Add, "add-project", nil, "Add the pull request to projects by `title`")
@@ -206,7 +205,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman
 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", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone", "assignees"},
 		Detector: opts.Detector,
 	}
 
@@ -215,27 +214,6 @@ func editRun(opts *EditOptions) error {
 		return err
 	}
 
-	if opts.Detector == nil {
-		baseRepo, err := opts.BaseRepo()
-		if err != nil {
-			return err
-		}
-
-		cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
-		opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
-	}
-
-	issueFeatures, err := opts.Detector.IssueFeatures()
-	if err != nil {
-		return err
-	}
-
-	if issueFeatures.ActorIsAssignable {
-		findOptions.Fields = append(findOptions.Fields, "assignedActors")
-	} else {
-		findOptions.Fields = append(findOptions.Fields, "assignees")
-	}
-
 	pr, repo, err := opts.Finder.Find(findOptions)
 	if err != nil {
 		return err
@@ -247,7 +225,7 @@ func editRun(opts *EditOptions) error {
 	editable.Body.Default = pr.Body
 	editable.Base.Default = pr.BaseRefName
 	editable.Reviewers.Default = pr.ReviewRequests.Logins()
-	if issueFeatures.ActorIsAssignable {
+	if pr.AssignedActorsUsed {
 		editable.Assignees.ActorAssignees = true
 		editable.Assignees.Default = pr.AssignedActors.DisplayNames()
 	} else {
diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go
index b8d23c978..a19c69669 100644
--- a/pkg/cmd/pr/shared/finder.go
+++ b/pkg/cmd/pr/shared/finder.go
@@ -242,6 +242,33 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
 		}
 	}
 
+	// Ok this is super, super horrible so bear with me.
+	// The `assignees` field on a Pull Request exposes users that are assigned. It is also possible for bots to be
+	// assigned, but they only appear under the `assignedActors` field. Ideally, the caller of `Find` would determine
+	// the correct field to use based on the `fd.Detector` that is passed in, but they can't construct a detector
+	// because the BaseRepo is only determined within this function. The more correct solution is to do what I did with
+	// the issue commands and decouple argument parsing from API lookup. See PR #10811 for example.
+	var actorAssigneesUsed bool
+	if fields.Contains("assignees") {
+		if opts.Detector == nil {
+			cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
+			opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost())
+		}
+
+		issueFeatures, err := opts.Detector.IssueFeatures()
+		if err != nil {
+			return nil, nil, fmt.Errorf("error detecting issue features: %v", err)
+		}
+
+		// If actors are assignable on this host then we additionally request the `assignedActors` field.
+		// Note that we don't remove the `assignees` field because some commands (`pr view`) do not display actor
+		// assignees yet, so we have to have both sets of data.
+		if issueFeatures.ActorIsAssignable {
+			fields.Add("assignedActors")
+			actorAssigneesUsed = true
+		}
+	}
+
 	var pr *api.PullRequest
 	if f.prNumber > 0 {
 		// If we have a PR number, let's look it up
@@ -297,6 +324,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
 		})
 	}
 
+	if actorAssigneesUsed {
+		pr.AssignedActorsUsed = true
+	}
+
 	return pr, f.baseRefRepo, g.Wait()
 }
 



Test patch:
diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go
index 3a1e8a548..0c40e3839 100644
--- a/pkg/cmd/pr/edit/edit_test.go
+++ b/pkg/cmd/pr/edit/edit_test.go
@@ -341,9 +341,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "non-interactive",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive: false,
 				Editable: shared.Editable{
@@ -403,9 +405,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "non-interactive skip reviewers",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive: false,
 				Editable: shared.Editable{
@@ -459,9 +463,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "non-interactive remove all reviewers",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive: false,
 				Editable: shared.Editable{
@@ -520,9 +526,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "interactive",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive:     true,
 				Surveyor:        testSurveyor{},
@@ -542,9 +550,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "interactive skip reviewers",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive:     true,
 				Surveyor:        testSurveyor{skipReviewers: true},
@@ -563,9 +573,11 @@ func Test_editRun(t *testing.T) {
 		{
 			name: "interactive remove all reviewers",
 			input: &EditOptions{
+				Detector:    &fd.EnabledDetectorMock{},
 				SelectorArg: "123",
 				Finder: shared.NewMockFinder("123", &api.PullRequest{
-					URL: "https://github.com/OWNER/REPO/pull/123",
+					URL:                "https://github.com/OWNER/REPO/pull/123",
+					AssignedActorsUsed: true,
 				}, ghrepo.New("OWNER", "REPO")),
 				Interactive:     true,
 				Surveyor:        testSurveyor{removeAllReviewers: true},
diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go
index abc754d1a..28c2337d3 100644
--- a/pkg/cmd/pr/shared/finder_test.go
+++ b/pkg/cmd/pr/shared/finder_test.go
@@ -9,6 +9,7 @@ import (
 
 	ghContext "github.com/cli/cli/v2/context"
 	"github.com/cli/cli/v2/git"
+	fd "github.com/cli/cli/v2/internal/featuredetection"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/httpmock"
 	"github.com/stretchr/testify/require"
@@ -705,6 +706,81 @@ func TestFind(t *testing.T) {
 	}
 }
 
+func TestFindAssignableActors(t *testing.T) {
+	t.Run("given actors are not assignable, do nothing special", func(t *testing.T) {
+		reg := &httpmock.Registry{}
+		defer reg.Verify(t)
+
+		// Ensure we never request assignedActors
+		reg.Exclude(t, httpmock.GraphQL(`assignedActors`))
+		reg.Register(
+			httpmock.GraphQL(`query PullRequestByNumber\b`),
+			httpmock.StringResponse(`{"data":{"repository":{
+						"pullRequest":{"number":13}
+					}}}`))
+
+		f := finder{
+			httpClient: func() (*http.Client, error) {
+				return &http.Client{Transport: reg}, nil
+			},
+		}
+
+		pr, _, err := f.Find(FindOptions{
+			Detector: &fd.DisabledDetectorMock{},
+			Fields:   []string{"assignees"},
+			Selector: "https://github.com/cli/cli/pull/123",
+		})
+		require.NoError(t, err)
+
+		require.False(t, pr.AssignedActorsUsed, "expected PR not to have assigned actors used")
+	})
+
+	t.Run("given actors are assignable, request assignedActors and indicate that on the returned PR", func(t *testing.T) {
+		reg := &httpmock.Registry{}
+		defer reg.Verify(t)
+
+		// Ensure that we only respond if assignedActors is requested
+		reg.Register(
+			httpmock.GraphQL(`assignedActors`),
+			httpmock.StringResponse(`{"data":{"repository":{
+						"pullRequest":{
+						    "number":13, 
+							"assignedActors": {
+								"nodes": [
+									{
+										"id": "HUBOTID",
+										"login": "hubot",
+										"__typename": "Bot"
+									},
+									{
+										"id": "MONAID",
+										"login": "MonaLisa",
+										"name": "Mona Display Name",
+										"__typename": "User"
+									}
+								],
+								"totalCount": 2
+							}}
+					}}}`))
+
+		f := finder{
+			httpClient: func() (*http.Client, error) {
+				return &http.Client{Transport: reg}, nil
+			},
+		}
+
+		pr, _, err := f.Find(FindOptions{
+			Detector: &fd.EnabledDetectorMock{},
+			Fields:   []string{"assignees"},
+			Selector: "https://github.com/cli/cli/pull/123",
+		})
+		require.NoError(t, err)
+
+		require.Equal(t, []string{"hubot", "MonaLisa"}, pr.AssignedActors.Logins())
+		require.True(t, pr.AssignedActorsUsed, "expected PR to have assigned actors used")
+	})
+}
+
 func stubBranchConfig(branchConfig git.BranchConfig, err error) func(context.Context, string) (git.BranchConfig, error) {
 	return func(_ context.Context, branch string) (git.BranchConfig, error) {
 		return branchConfig, err