Repository: cli/cli
Pull request #11057 (2025-06-02 13:04:17)

Fix pr edit when URL is provided
## Description

Fixes https://github.com/cli/cli/issues/11055

The fundamental issue as described by @babakks [here](https://github.com/cli/cli/issues/11055#issuecomment-2930105787) is that with the work done to support adding copilot as an issue assignee, `pr edit` started doing feature detection. However, when the argument provided to `pr edit` is a URL, there is a bit of a "race" between determining the base repo and constructing the feature detector.

When doing the `projectsV1` deprecation work, I encountered this as well and took a shortcut for `pr` commands by pushing feature detection into the finder itself. The more correct solution is to do what I did with the `issue` commands and decouple argument parsing from API lookup. See https://github.com/cli/cli/pull/10811 for example. We **should** do this.

However, our team is about to have a number of days offsite together, and I am keen to get a patch release for this going, especially since it will take some time to roll out into actions images. Therefore this PR takes the shortest path I can think. It is very horrible, and that's a good thing because we won't let this sit around after our offsite.

It pushes the feature detection for actor assignees into the finder and exposes the fact that the server did or didn't support it on the `PullRequest` struct itself 🤮 .

### E2E Tests

```
--- PASS: TestPullRequests (0.00s)
    --- PASS: TestPullRequests/pr-checkout-by-number (8.69s)
    --- PASS: TestPullRequests/pr-view (7.47s)
    --- PASS: TestPullRequests/pr-view-status-respects-simple-pushdefault (10.32s)
    --- PASS: TestPullRequests/pr-view-status-respects-remote-pushdefault (17.32s)
    --- PASS: TestPullRequests/pr-view-status-respects-push-destination (8.85s)
    --- PASS: TestPullRequests/pr-view-status-respects-branch-pushremote (17.09s)
    --- PASS: TestPullRequests/pr-view-same-org-fork (15.42s)
    --- PASS: TestPullRequests/pr-view-outside-repo (8.11s)
    --- PASS: TestPullRequests/pr-status-respects-cross-org (16.48s)
    --- PASS: TestPullRequests/pr-merge-rebase-strategy (12.88s)
    --- PASS: TestPullRequests/pr-merge-merge-strategy (11.27s)
    --- PASS: TestPullRequests/pr-list (7.52s)
    --- PASS: TestPullRequests/pr-create-without-upstream-config (8.06s)
    --- PASS: TestPullRequests/pr-create-with-metadata (9.79s)
    --- PASS: TestPullRequests/pr-create-respects-user-colon-branch-syntax (16.24s)
    --- PASS: TestPullRequests/pr-create-respects-simple-pushdefault (8.77s)
    --- PASS: TestPullRequests/pr-create-respects-remote-pushdefault (17.35s)
    --- PASS: TestPullRequests/pr-create-respects-push-destination (17.32s)
    --- PASS: TestPullRequests/pr-create-respects-branch-pushremote (18.46s)
    --- PASS: TestPullRequests/pr-create-remote-ref-with-branch-name-slash (17.07s)
    --- PASS: TestPullRequests/pr-create-push-default-upstream-no-merge-ref (8.28s)
    --- SKIP: TestPullRequests/pr-create-push-default-upstream-no-merge-ref-fork (0.01s)
    --- PASS: TestPullRequests/pr-create-no-local-repo (7.70s)
    --- PASS: TestPullRequests/pr-create-guesses-remote-from-sha (17.94s)
    --- PASS: TestPullRequests/pr-create-guesses-remote-from-sha-with-branch-name-slash (18.09s)
    --- PASS: TestPullRequests/pr-create-from-manual-merge-base (10.50s)
    --- PASS: TestPullRequests/pr-create-from-issue-develop-base (13.17s)
    --- PASS: TestPullRequests/pr-create-edit-with-project (19.56s)
    --- PASS: TestPullRequests/pr-create-basic (8.28s)
    --- PASS: TestPullRequests/pr-comment-new (9.38s)
    --- PASS: TestPullRequests/pr-comment-edit-last-without-comments-errors (9.32s)
    --- PASS: TestPullRequests/pr-comment-edit-last-without-comments-creates (9.36s)
    --- PASS: TestPullRequests/pr-comment-edit-last-with-comments (11.13s)
    --- PASS: TestPullRequests/pr-checkout (8.65s)
    --- PASS: TestPullRequests/pr-checkout-with-url-from-fork (18.82s)
PASS
```


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/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go
index c30c4f9bb..867ee9a4f 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"
@@ -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