Skip to content

Fix: gh pr create, only fetch teams when reviewers contain a team #11361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 23, 2025
Merged

Fix: gh pr create, only fetch teams when reviewers contain a team #11361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
449920b
Add TeamReviewers flag to RepoMetadataInput
BagToad Jul 22, 2025
df317d4
FIX: conditionally fetching team reviewers
BagToad Jul 22, 2025
e5feda3
Refactor error assertion in Test_RepoMetadataTeams
BagToad Jul 22, 2025
addee16
Refactor and improve RepoMetadata teams test
BagToad Jul 23, 2025
5a6cac3
Add tests for reviewer team handling in PR creation
BagToad Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/queries_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,7 @@ type RepoMetadataInput struct {
Assignees bool
ActorAssignees bool
Reviewers bool
TeamReviewers bool
Labels bool
ProjectsV1 bool
ProjectsV2 bool
Expand Down Expand Up @@ -964,7 +965,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
}
}

if input.Reviewers {
if input.Reviewers && input.TeamReviewers {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question:

  1. what conditions will users still encounter the GraphQL: Resource not accessible by integration (organization.teams) error?

    • GitHub CLI OAuth App?
    • GitHub Actions automatic token?

  2. why is it important for both Reviewers and TeamReviewers to be set for retrieving organization teams?

Copy link
Member Author

@BagToad BagToad Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think it'll happen when someone gives us a team (contains a slash), but their auth token does not have perms to fetch teams.

  2. It's technically not important to make this function but it eliminates a situation where the caller might set TeamReviewers to true while Reviewers is false, causing only teams to be fetched and populated into the reviewer metadata. I don't know of a case where we actually want that to happen, so I figure we should guard against callers misusing this field until we know that to be a valid use case by coupling the two fields together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insight!

For my first question, I was unsure which gh authentication methods include organization team permissions, so I reset my gh auth session and confirmed the read:org scope associated with GitHub CLI OAuth app is good:

$ gh auth status --hostname github.com
github.com
  ✓ Logged in to github.com account andyfeller (keyring)
  - Active account: true
  - Git operations protocol: https
  - Token: gho_************************************
  - Token scopes: 'gist', 'read:org', 'repo', 'workflow'

$ QUERY='                             
query {
  organization(login:"cli"){
    teams(first:10, privacy:VISIBLE){
      nodes{
        slug
      }
    }
  }
}'

$ gh api graphql -f query="$QUERY"
{
  "data": {
    "organization": {
      "teams": {
        "nodes": [
          {
            "slug": "code-reviewers"
          },
          {
            "slug": "codespaces"
          },
          {
            "slug": "deployment-reviewers"
          },
          {
            "slug": "package-security"
          },
          {
            "slug": "tuf-root-reviewers"
          },
          {
            "slug": "webhooks"
          }
        ]
      }
    }
  }
}

g.Go(func() error {
teams, err := OrganizationTeams(client, repo)
// TODO: better detection of non-org repos
Expand Down
50 changes: 44 additions & 6 deletions api/queries_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,13 @@ func Test_RepoMetadata(t *testing.T) {

repo, _ := ghrepo.FromFullName("OWNER/REPO")
input := RepoMetadataInput{
Assignees: true,
Reviewers: true,
Labels: true,
ProjectsV1: true,
ProjectsV2: true,
Milestones: true,
Assignees: true,
Reviewers: true,
TeamReviewers: true,
Labels: true,
ProjectsV1: true,
ProjectsV2: true,
Milestones: true,
}

http.Register(
Expand Down Expand Up @@ -213,6 +214,43 @@ func Test_RepoMetadata(t *testing.T) {
}
}

// Test that RepoMetadata only fetches teams if the input specifies it
func Test_RepoMetadata_TeamsAreConditionallyFetched(t *testing.T) {
http := &httpmock.Registry{}
client := newTestClient(http)
repo, _ := ghrepo.FromFullName("OWNER/REPO")
input := RepoMetadataInput{
Reviewers: true,
TeamReviewers: false, // Do not fetch teams
}

http.Register(
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "assignableUsers": {
"nodes": [
{ "login": "hubot", "id": "HUBOTID" },
{ "login": "MonaLisa", "id": "MONAID" }
],
"pageInfo": { "hasNextPage": false }
} } } }
`))

http.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`
{ "data": { "viewer": { "login": "monalisa" } } }
`))

http.Exclude(
t,
httpmock.GraphQL(`query OrganizationTeamList\b`),
)

Comment on lines +245 to +249
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: thank you for considering the idea ❤️

_, err := RepoMetadata(client, repo, input)
require.NoError(t, err)
}

func Test_ProjectNamesToPaths(t *testing.T) {
t.Run("when projectsV1 is supported, requests them", func(t *testing.T) {
http := &httpmock.Registry{}
Expand Down
127 changes: 119 additions & 8 deletions pkg/cmd/pr/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,14 +1314,6 @@ func Test_createRun(t *testing.T) {
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`query OrganizationTeamList\b`),
httpmock.StringResponse(`
{ "data": { "organization": { "teams": {
"nodes": [],
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`),
httpmock.GraphQLMutation(`
Expand Down Expand Up @@ -1662,6 +1654,125 @@ func Test_createRun(t *testing.T) {
},
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
},
{
name: "if reviewer contains any team, fetch teams",
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"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: I appreciate having the org added rather than leaving it clipped as in other tests

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: "if reviewer does NOT contain any team, do NOT fetch teams",
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`),
httpmock.StringResponse(`
{ "data": { "viewer": { "login": "monalisa" } } }
`))
reg.Exclude(
t,
httpmock.GraphQL(`query OrganizationTeamList\b`),
)
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.NotEqual(t, []interface{}{"COREID", "ROBOTID"}, inputs["teamIds"])
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) {
Expand Down
11 changes: 10 additions & 1 deletion pkg/cmd/pr/shared/editable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,16 @@ func FieldsToEditSurvey(p EditPrompter, editable *Editable) error {

func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) error {
input := api.RepoMetadataInput{
Reviewers: editable.Reviewers.Edited,
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,
Assignees: editable.Assignees.Edited,
ActorAssignees: editable.Assignees.ActorAssignees,
Labels: editable.Labels.Edited,
Expand Down
6 changes: 5 additions & 1 deletion pkg/cmd/pr/shared/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package shared
import (
"fmt"
"net/url"
"slices"
"strings"

"github.com/cli/cli/v2/api"
Expand Down Expand Up @@ -63,7 +64,10 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par
// 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,
Reviewers: len(tb.Reviewers) > 0,
TeamReviewers: len(tb.Reviewers) > 0 && slices.ContainsFunc(tb.Reviewers, func(r string) bool {
return strings.ContainsRune(r, '/')
}),
Comment on lines +67 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: do we have any tests that should make sure that OrganizationTeams query is excluded when no teams are specified as reviewers?

I see tests like this that include teams:

cli/pkg/cmd/pr/create/create_test.go

Lines 412 to 479 in e5feda3

{
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)
},

I only see 1 test that has only human users:

cli/pkg/cmd/pr/create/create_test.go

Lines 1300 to 1382 in e5feda3

{
name: "recover",
tty: true,
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": "jillValentine", "id": "JILLID", "name": "Jill Valentine" }
],
"pageInfo": { "hasNextPage": false }
} } } }
`))
reg.Register(
httpmock.GraphQL(`mutation PullRequestCreateRequestReviews\b`),
httpmock.GraphQLMutation(`
{ "data": { "requestReviews": {
"clientMutationId": ""
} } }
`, func(inputs map[string]interface{}) {
assert.Equal(t, []interface{}{"JILLID"}, inputs["userIds"])
}))
reg.Register(
httpmock.GraphQL(`mutation PullRequestCreate\b`),
httpmock.GraphQLMutation(`
{ "data": { "createPullRequest": { "pullRequest": {
"URL": "https://github.com/OWNER/REPO/pull/12"
} } } }
`, func(input map[string]interface{}) {
assert.Equal(t, "recovered title", input["title"].(string))
assert.Equal(t, "recovered body", input["body"].(string))
}))
},
cmdStubs: func(cs *run.CommandStubber) {
cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "")
},
promptStubs: func(pm *prompter.PrompterMock) {
pm.InputFunc = func(p, d string) (string, error) {
if p == "Title (required)" {
return d, nil
} else {
return "", prompter.NoSuchPromptErr(p)
}
}
pm.MarkdownEditorFunc = func(p, d string, ba bool) (string, error) {
if p == "Body" {
return d, nil
} else {
return "", prompter.NoSuchPromptErr(p)
}
}
pm.SelectFunc = func(p, _ string, opts []string) (int, error) {
if p == "What's next?" {
return 0, nil
} else {
return -1, prompter.NoSuchPromptErr(p)
}
}
},
setup: func(opts *CreateOptions, t *testing.T) func() {
tmpfile, err := os.CreateTemp(t.TempDir(), "testrecover*")
assert.NoError(t, err)
state := shared.IssueMetadataState{
Title: "recovered title",
Body: "recovered body",
Reviewers: []string{"jillValentine"},
}
data, err := json.Marshal(state)
assert.NoError(t, err)
_, err = tmpfile.Write(data)
assert.NoError(t, err)
opts.RecoverFile = tmpfile.Name()
opts.HeadBranch = "feature"
return func() { tmpfile.Close() }
},
expectedOut: "https://github.com/OWNER/REPO/pull/12\n",
expectedErrOut: "\nCreating pull request for feature into master in OWNER/REPO\n\n",
},

If so, then maybe we .Exclude(...) that query from this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test added in the PR removes the OrganizationTeams query mock while setting TeamReviewers to false. This would fail if the OrganizationTeams query actually runs because it won't find a mock for that query.

cli/api/queries_repo_test.go

Lines 217 to 247 in e5feda3

func Test_RepoMetadataTeams(t *testing.T) {
// Test that RepoMetadata only fetches teams if the input specifies it
http := &httpmock.Registry{}
client := newTestClient(http)
repo, _ := ghrepo.FromFullName("OWNER/REPO")
input := RepoMetadataInput{
Reviewers: true,
TeamReviewers: false,
}
http.Register(
httpmock.GraphQL(`query RepositoryAssignableUsers\b`),
httpmock.StringResponse(`
{ "data": { "repository": { "assignableUsers": {
"nodes": [
{ "login": "hubot", "id": "HUBOTID" },
{ "login": "MonaLisa", "id": "MONAID" }
],
"pageInfo": { "hasNextPage": false }
} } } }
`))
http.Register(
httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`
{ "data": { "viewer": { "login": "monalisa" } } }
`))
_, err := RepoMetadata(client, repo, input)
require.NoError(t, err)
}

Copy link
Member

@andyfeller andyfeller Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🤔 I guess I was thinking we have a stronger assertion setup to ensure the query was explicitly not called at all close to the gh pr create command since it conditionally includes team reviewers whereas gh pr edit always does:

cli/api/queries_repo_test.go

Lines 287 to 300 in b2348f8

t.Run("when projectsV1 is not supported, does not request them", func(t *testing.T) {
http := &httpmock.Registry{}
client := newTestClient(http)
repo, _ := ghrepo.FromFullName("OWNER/REPO")
http.Exclude(
t,
httpmock.GraphQL(`query RepositoryProjectList\b`),
)
http.Exclude(
t,
httpmock.GraphQL(`query OrganizationProjectList\b`),
)

Copy link
Member Author

@BagToad BagToad Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I spent some time firming up these tests, and I think they're well in the direction you mentioned. I ensured there are tests at the metadata fetcher level and higher up in the pr create tests directly. This should cover all the bases.

addee16
5a6cac3

Assignees: len(tb.Assignees) > 0,
ActorAssignees: tb.ActorAssignees,
Labels: len(tb.Labels) > 0,
Expand Down
Loading