Skip to content

Commit 0042741

Browse files
RossTarrantRossTarrant
committed
feat: Add team reviewer param to update pr tool
1 parent 7d46f8d commit 0042741

8 files changed

Lines changed: 237 additions & 29 deletions

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,7 @@ The following sets of tools are available:
11571157
- `repo`: Repository name (string, required)
11581158
- `reviewers`: GitHub usernames to request reviews from (string[], optional)
11591159
- `state`: New state (string, optional)
1160+
- `team_reviewers`: GitHub team slugs to request reviews from (string[], optional)
11601161
- `title`: New title (string, optional)
11611162

11621163
- **update_pull_request_branch** - Update pull request branch

‎docs/feature-flags.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,8 @@ runtime behavior (such as output formatting) won't appear here.
240240
- `owner`: Repository owner (username or organization) (string, required)
241241
- `pullNumber`: The pull request number (number, required)
242242
- `repo`: Repository name (string, required)
243-
- `reviewers`: GitHub usernames to request reviews from (string[], required)
243+
- `reviewers`: GitHub usernames to request reviews from (string[], optional)
244+
- `team_reviewers`: GitHub team slugs to request reviews from (string[], optional)
244245

245246
- **resolve_review_thread** - Resolve Review Thread
246247
- **Required OAuth Scopes**: `repo`

‎pkg/github/__toolsnaps__/request_pull_request_reviewers.snap‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,19 @@
2626
"type": "string"
2727
},
2828
"type": "array"
29+
},
30+
"team_reviewers": {
31+
"description": "GitHub team slugs to request reviews from",
32+
"items": {
33+
"type": "string"
34+
},
35+
"type": "array"
2936
}
3037
},
3138
"required": [
3239
"owner",
3340
"repo",
34-
"pullNumber",
35-
"reviewers"
41+
"pullNumber"
3642
],
3743
"type": "object"
3844
},

‎pkg/github/__toolsnaps__/update_pull_request.snap‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@
4848
],
4949
"type": "string"
5050
},
51+
"team_reviewers": {
52+
"description": "GitHub team slugs to request reviews from",
53+
"items": {
54+
"type": "string"
55+
},
56+
"type": "array"
57+
},
5158
"title": {
5259
"description": "New title",
5360
"type": "string"

‎pkg/github/granular_tools_test.go‎

Lines changed: 110 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -772,22 +772,118 @@ func TestGranularUpdatePullRequestState(t *testing.T) {
772772
}
773773

774774
func TestGranularRequestPullRequestReviewers(t *testing.T) {
775-
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
776-
PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, &gogithub.PullRequest{Number: gogithub.Ptr(1)}),
777-
}))
778-
deps := BaseDeps{Client: client}
779775
serverTool := GranularRequestPullRequestReviewers(translations.NullTranslationHelper)
780-
handler := serverTool.Handler(deps)
781776

782-
request := createMCPRequest(map[string]any{
783-
"owner": "owner",
784-
"repo": "repo",
785-
"pullNumber": float64(1),
786-
"reviewers": []string{"user1", "user2"},
787-
})
788-
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
789-
require.NoError(t, err)
790-
assert.False(t, result.IsError)
777+
tests := []struct {
778+
name string
779+
requestArgs map[string]any
780+
expectedBody map[string]any
781+
expectedErrMsg string
782+
}{
783+
{
784+
name: "user reviewers",
785+
requestArgs: map[string]any{
786+
"owner": "owner",
787+
"repo": "repo",
788+
"pullNumber": float64(1),
789+
"reviewers": []string{"user1", "user2"},
790+
},
791+
expectedBody: map[string]any{
792+
"reviewers": []any{"user1", "user2"},
793+
},
794+
},
795+
{
796+
name: "team reviewers",
797+
requestArgs: map[string]any{
798+
"owner": "owner",
799+
"repo": "repo",
800+
"pullNumber": float64(1),
801+
"reviewers": []string{"user1"},
802+
"team_reviewers": []string{"team-slug", "platform"},
803+
},
804+
expectedBody: map[string]any{
805+
"reviewers": []any{"user1"},
806+
"team_reviewers": []any{"team-slug", "platform"},
807+
},
808+
},
809+
{
810+
name: "team reviewers only",
811+
requestArgs: map[string]any{
812+
"owner": "owner",
813+
"repo": "repo",
814+
"pullNumber": float64(1),
815+
"team_reviewers": []string{"platform"},
816+
},
817+
expectedBody: map[string]any{
818+
"team_reviewers": []any{"platform"},
819+
},
820+
},
821+
{
822+
name: "missing reviewers",
823+
requestArgs: map[string]any{
824+
"owner": "owner",
825+
"repo": "repo",
826+
"pullNumber": float64(1),
827+
},
828+
expectedErrMsg: "missing required parameter: reviewers or team_reviewers",
829+
},
830+
{
831+
name: "team reviewer in reviewers",
832+
requestArgs: map[string]any{
833+
"owner": "owner",
834+
"repo": "repo",
835+
"pullNumber": float64(1),
836+
"reviewers": []string{"owner/team-slug"},
837+
},
838+
expectedErrMsg: `invalid reviewer "owner/team-slug": expected GitHub username; use team_reviewers for team slugs`,
839+
},
840+
{
841+
name: "malformed reviewer",
842+
requestArgs: map[string]any{
843+
"owner": "owner",
844+
"repo": "repo",
845+
"pullNumber": float64(1),
846+
"reviewers": []string{"owner/team-slug/extra"},
847+
},
848+
expectedErrMsg: `invalid reviewer "owner/team-slug/extra": expected GitHub username; use team_reviewers for team slugs`,
849+
},
850+
{
851+
name: "invalid team reviewers slug",
852+
requestArgs: map[string]any{
853+
"owner": "owner",
854+
"repo": "repo",
855+
"pullNumber": float64(1),
856+
"team_reviewers": []string{"owner/team-slug"},
857+
},
858+
expectedErrMsg: `invalid team reviewer "owner/team-slug": expected team slug`,
859+
},
860+
}
861+
862+
for _, tc := range tests {
863+
t.Run(tc.name, func(t *testing.T) {
864+
mockedClient := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})
865+
if tc.expectedBody != nil {
866+
mockedClient = MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
867+
PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: expectRequestBody(t, tc.expectedBody).andThen(
868+
mockResponse(t, http.StatusOK, &gogithub.PullRequest{Number: gogithub.Ptr(1)}),
869+
),
870+
})
871+
}
872+
client := mustNewGHClient(t, mockedClient)
873+
deps := BaseDeps{Client: client}
874+
handler := serverTool.Handler(deps)
875+
876+
request := createMCPRequest(tc.requestArgs)
877+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
878+
require.NoError(t, err)
879+
if tc.expectedErrMsg != "" {
880+
require.True(t, result.IsError)
881+
assert.Contains(t, getErrorResult(t, result).Text, tc.expectedErrMsg)
882+
return
883+
}
884+
assert.False(t, result.IsError)
885+
})
886+
}
791887
}
792888

793889
func TestGranularCreatePullRequestReview(t *testing.T) {

‎pkg/github/pullrequests.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,13 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
757757
Type: "string",
758758
},
759759
},
760+
"team_reviewers": {
761+
Type: "array",
762+
Description: "GitHub team slugs to request reviews from",
763+
Items: &jsonschema.Schema{
764+
Type: "string",
765+
},
766+
},
760767
},
761768
Required: []string{"owner", "repo", "pullNumber"},
762769
}
@@ -839,9 +846,17 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
839846
if err != nil {
840847
return utils.NewToolResultError(err.Error()), nil, nil
841848
}
849+
teamReviewers, err := OptionalStringArrayParam(args, "team_reviewers")
850+
if err != nil {
851+
return utils.NewToolResultError(err.Error()), nil, nil
852+
}
853+
reviewersRequest, err := pullRequestReviewersRequest(reviewers, teamReviewers)
854+
if err != nil {
855+
return utils.NewToolResultError(err.Error()), nil, nil
856+
}
842857

843858
// If no updates, no draft change, and no reviewers, return error early
844-
if !restUpdateNeeded && !draftProvided && len(reviewers) == 0 {
859+
if !restUpdateNeeded && !draftProvided && len(reviewersRequest.Reviewers) == 0 && len(reviewersRequest.TeamReviewers) == 0 {
845860
return utils.NewToolResultError("No update parameters provided."), nil, nil
846861
}
847862

@@ -938,16 +953,12 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
938953
}
939954

940955
// Handle reviewer requests
941-
if len(reviewers) > 0 {
956+
if len(reviewersRequest.Reviewers) > 0 || len(reviewersRequest.TeamReviewers) > 0 {
942957
client, err := deps.GetClient(ctx)
943958
if err != nil {
944959
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
945960
}
946961

947-
reviewersRequest := github.ReviewersRequest{
948-
Reviewers: reviewers,
949-
}
950-
951962
_, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, reviewersRequest)
952963
if err != nil {
953964
return ghErrors.NewGitHubAPIErrorResponse(ctx,

‎pkg/github/pullrequests_granular.go‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,13 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i
300300
Description: "GitHub usernames to request reviews from",
301301
Items: &jsonschema.Schema{Type: "string"},
302302
},
303+
"team_reviewers": {
304+
Type: "array",
305+
Description: "GitHub team slugs to request reviews from",
306+
Items: &jsonschema.Schema{Type: "string"},
307+
},
303308
},
304-
Required: []string{"owner", "repo", "pullNumber", "reviewers"},
309+
Required: []string{"owner", "repo", "pullNumber"},
305310
},
306311
},
307312
[]scopes.Scope{scopes.Repo},
@@ -322,16 +327,24 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i
322327
if err != nil {
323328
return utils.NewToolResultError(err.Error()), nil, nil
324329
}
325-
if len(reviewers) == 0 {
326-
return utils.NewToolResultError("missing required parameter: reviewers"), nil, nil
330+
teamReviewers, err := OptionalStringArrayParam(args, "team_reviewers")
331+
if err != nil {
332+
return utils.NewToolResultError(err.Error()), nil, nil
333+
}
334+
reviewersRequest, err := pullRequestReviewersRequest(reviewers, teamReviewers)
335+
if err != nil {
336+
return utils.NewToolResultError(err.Error()), nil, nil
337+
}
338+
if len(reviewersRequest.Reviewers) == 0 && len(reviewersRequest.TeamReviewers) == 0 {
339+
return utils.NewToolResultError("missing required parameter: reviewers or team_reviewers"), nil, nil
327340
}
328341

329342
client, err := deps.GetClient(ctx)
330343
if err != nil {
331344
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
332345
}
333346

334-
pr, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, gogithub.ReviewersRequest{Reviewers: reviewers})
347+
pr, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, reviewersRequest)
335348
if err != nil {
336349
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to request reviewers", resp, err), nil, nil
337350
}
@@ -351,6 +364,29 @@ func GranularRequestPullRequestReviewers(t translations.TranslationHelperFunc) i
351364
return st
352365
}
353366

367+
func pullRequestReviewersRequest(reviewers, teamReviewers []string) (gogithub.ReviewersRequest, error) {
368+
request := gogithub.ReviewersRequest{
369+
Reviewers: make([]string, 0, len(reviewers)),
370+
TeamReviewers: make([]string, 0, len(teamReviewers)),
371+
}
372+
373+
for _, reviewer := range reviewers {
374+
if reviewer == "" || strings.Contains(reviewer, "/") {
375+
return gogithub.ReviewersRequest{}, fmt.Errorf("invalid reviewer %q: expected GitHub username; use team_reviewers for team slugs", reviewer)
376+
}
377+
request.Reviewers = append(request.Reviewers, reviewer)
378+
}
379+
380+
for _, teamReviewer := range teamReviewers {
381+
if teamReviewer == "" || strings.Contains(teamReviewer, "/") {
382+
return gogithub.ReviewersRequest{}, fmt.Errorf("invalid team reviewer %q: expected team slug", teamReviewer)
383+
}
384+
request.TeamReviewers = append(request.TeamReviewers, teamReviewer)
385+
}
386+
387+
return request, nil
388+
}
389+
354390
// GranularCreatePullRequestReview creates a tool to create a PR review.
355391
func GranularCreatePullRequestReview(t translations.TranslationHelperFunc) inventory.ServerTool {
356392
st := NewTool(

‎pkg/github/pullrequests_test.go‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ func Test_UpdatePullRequest(t *testing.T) {
157157
assert.Contains(t, schema.Properties, "base")
158158
assert.Contains(t, schema.Properties, "maintainer_can_modify")
159159
assert.Contains(t, schema.Properties, "reviewers")
160+
assert.Contains(t, schema.Properties, "team_reviewers")
160161
assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "pullNumber"})
161162

162163
// Setup mock PR for success case
@@ -245,8 +246,12 @@ func Test_UpdatePullRequest(t *testing.T) {
245246
{
246247
name: "successful PR update with reviewers",
247248
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
248-
PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRWithReviewers),
249-
GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRWithReviewers),
249+
PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
250+
"reviewers": []any{"reviewer1", "reviewer2"},
251+
}).andThen(
252+
mockResponse(t, http.StatusOK, mockPRWithReviewers),
253+
),
254+
GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRWithReviewers),
250255
}),
251256
requestArgs: map[string]any{
252257
"owner": "owner",
@@ -257,6 +262,51 @@ func Test_UpdatePullRequest(t *testing.T) {
257262
expectError: false,
258263
expectedPR: mockPRWithReviewers,
259264
},
265+
{
266+
name: "successful PR update with team reviewers",
267+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
268+
PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: expectRequestBody(t, map[string]any{
269+
"reviewers": []any{"reviewer1"},
270+
"team_reviewers": []any{"team-slug", "platform"},
271+
}).andThen(
272+
mockResponse(t, http.StatusOK, mockPRWithReviewers),
273+
),
274+
GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPRWithReviewers),
275+
}),
276+
requestArgs: map[string]any{
277+
"owner": "owner",
278+
"repo": "repo",
279+
"pullNumber": float64(42),
280+
"reviewers": []any{"reviewer1"},
281+
"team_reviewers": []any{"team-slug", "platform"},
282+
},
283+
expectError: false,
284+
expectedPR: mockPRWithReviewers,
285+
},
286+
{
287+
name: "team reviewer in reviewers",
288+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
289+
requestArgs: map[string]any{
290+
"owner": "owner",
291+
"repo": "repo",
292+
"pullNumber": float64(42),
293+
"reviewers": []any{"owner/team-slug"},
294+
},
295+
expectError: false,
296+
expectedErrMsg: `invalid reviewer "owner/team-slug": expected GitHub username; use team_reviewers for team slugs`,
297+
},
298+
{
299+
name: "invalid team reviewer slug",
300+
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}),
301+
requestArgs: map[string]any{
302+
"owner": "owner",
303+
"repo": "repo",
304+
"pullNumber": float64(42),
305+
"team_reviewers": []any{"owner/team-slug"},
306+
},
307+
expectError: false,
308+
expectedErrMsg: `invalid team reviewer "owner/team-slug": expected team slug`,
309+
},
260310
{
261311
name: "successful PR update (title only)",
262312
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{

0 commit comments

Comments
 (0)