Skip to content

feat: add reviewers parameter to UpdatePullRequest #285

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.

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
- `state`: New state ('open' or 'closed') (string, optional)
- `base`: New base branch name (string, optional)
- `maintainer_can_modify`: Allow maintainer edits (boolean, optional)
- `reviewers`: GitHub usernames to request reviews from (string[], optional)

### Repositories

Expand Down
101 changes: 91 additions & 10 deletions pkg/github/pullrequests.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu
mcp.WithBoolean("maintainer_can_modify",
mcp.Description("Allow maintainer edits"),
),
mcp.WithArray("reviewers",
mcp.Description("GitHub usernames to request reviews from"),
mcp.Items(map[string]interface{}{
"type": "string",
}),
),
),
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
owner, err := requiredParam[string](request, "owner")
Expand Down Expand Up @@ -165,26 +171,101 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu
updateNeeded = true
}

if !updateNeeded {
return mcp.NewToolResultError("No update parameters provided."), nil
// Handle reviewers separately
var reviewers []string
if reviewersArr, ok := request.Params.Arguments["reviewers"].([]interface{}); ok && len(reviewersArr) > 0 {
for _, reviewer := range reviewersArr {
if reviewerStr, ok := reviewer.(string); ok {
reviewers = append(reviewers, reviewerStr)
}
}
}

// Create the GitHub client
client, err := getClient(ctx)
if err != nil {
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update)
if err != nil {
return nil, fmt.Errorf("failed to update pull request: %w", err)

var pr *github.PullRequest
var resp *http.Response

// First, update the PR if needed
if updateNeeded {
var ghResp *github.Response
pr, ghResp, err = client.PullRequests.Edit(ctx, owner, repo, pullNumber, update)
if err != nil {
return nil, fmt.Errorf("failed to update pull request: %w", err)
}
resp = ghResp.Response
defer func() {
if resp != nil && resp.Body != nil {
_ = resp.Body.Close()
}
}()

if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
return mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil
}
} else {
// If no update needed, just get the current PR
var ghResp *github.Response
pr, ghResp, err = client.PullRequests.Get(ctx, owner, repo, pullNumber)
if err != nil {
return nil, fmt.Errorf("failed to get pull request: %w", err)
}
resp = ghResp.Response
defer func() {
if resp != nil && resp.Body != nil {
_ = resp.Body.Close()
}
}()

if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %s", string(body))), nil
}
}
defer func() { _ = resp.Body.Close() }()

if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
// Add reviewers if specified
if len(reviewers) > 0 {
reviewersRequest := github.ReviewersRequest{
Reviewers: reviewers,
}

// Use the direct result of RequestReviewers which includes the requested reviewers
updatedPR, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, reviewersRequest)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
return nil, fmt.Errorf("failed to request reviewers: %w", err)
}
return mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil
defer func() {
if resp != nil && resp.Body != nil {
_ = resp.Body.Close()
}
}()

if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %w", err)
}
return mcp.NewToolResultError(fmt.Sprintf("failed to request reviewers: %s", string(body))), nil
}

// Use the updated PR with reviewers
pr = updatedPR
}

// If no updates and no reviewers, return error
if !updateNeeded && len(reviewers) == 0 {
return mcp.NewToolResultError("No update parameters provided"), nil
}

r, err := json.Marshal(pr)
Expand Down
100 changes: 98 additions & 2 deletions pkg/github/pullrequests_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ func Test_UpdatePullRequest(t *testing.T) {
assert.Contains(t, tool.InputSchema.Properties, "state")
assert.Contains(t, tool.InputSchema.Properties, "base")
assert.Contains(t, tool.InputSchema.Properties, "maintainer_can_modify")
assert.Contains(t, tool.InputSchema.Properties, "reviewers")
assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"})

// Setup mock PR for success case
Expand All @@ -162,6 +163,23 @@ func Test_UpdatePullRequest(t *testing.T) {
State: github.Ptr("closed"), // State updated
}

// Mock PR for when there are no updates but we still need a response
mockNoUpdatePR := &github.PullRequest{
Number: github.Ptr(42),
Title: github.Ptr("Test PR"),
State: github.Ptr("open"),
}

mockPRWithReviewers := &github.PullRequest{
Number: github.Ptr(42),
Title: github.Ptr("Test PR"),
State: github.Ptr("open"),
RequestedReviewers: []*github.User{
{Login: github.Ptr("reviewer1")},
{Login: github.Ptr("reviewer2")},
},
}

tests := []struct {
name string
mockedClient *http.Client
Expand Down Expand Up @@ -220,8 +238,40 @@ func Test_UpdatePullRequest(t *testing.T) {
expectedPR: mockClosedPR,
},
{
name: "no update parameters provided",
mockedClient: mock.NewMockedHTTPClient(), // No API call expected
name: "successful PR update with reviewers",
mockedClient: mock.NewMockedHTTPClient(
mock.WithRequestMatch(
mock.GetReposPullsByOwnerByRepoByPullNumber,
&github.PullRequest{
Number: github.Ptr(42),
Title: github.Ptr("Test PR"),
State: github.Ptr("open"),
},
),
// Mock for RequestReviewers call, returning the PR with reviewers
mock.WithRequestMatch(
mock.PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber,
mockPRWithReviewers,
),
),
requestArgs: map[string]interface{}{
"owner": "owner",
"repo": "repo",
"pullNumber": float64(42),
"reviewers": []interface{}{"reviewer1", "reviewer2"},
},
expectError: false,
expectedPR: mockPRWithReviewers,
},
{
name: "no update parameters provided",
mockedClient: mock.NewMockedHTTPClient(
// Mock a response for the GET PR request in case of no updates
mock.WithRequestMatch(
mock.GetReposPullsByOwnerByRepoByPullNumber,
mockNoUpdatePR,
),
),
requestArgs: map[string]interface{}{
"owner": "owner",
"repo": "repo",
Expand Down Expand Up @@ -251,6 +301,32 @@ func Test_UpdatePullRequest(t *testing.T) {
expectError: true,
expectedErrMsg: "failed to update pull request",
},
{
name: "request reviewers fails",
mockedClient: mock.NewMockedHTTPClient(
// First it gets the PR (no fields to update)
mock.WithRequestMatch(
mock.GetReposPullsByOwnerByRepoByPullNumber,
mockNoUpdatePR,
),
// Then reviewer request fails
mock.WithRequestMatchHandler(
mock.PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber,
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusUnprocessableEntity)
_, _ = w.Write([]byte(`{"message": "Invalid reviewers"}`))
}),
),
),
requestArgs: map[string]interface{}{
"owner": "owner",
"repo": "repo",
"pullNumber": float64(42),
"reviewers": []interface{}{"invalid-user"},
},
expectError: true,
expectedErrMsg: "failed to request reviewers",
},
}

for _, tc := range tests {
Expand Down Expand Up @@ -304,6 +380,26 @@ func Test_UpdatePullRequest(t *testing.T) {
if tc.expectedPR.MaintainerCanModify != nil {
assert.Equal(t, *tc.expectedPR.MaintainerCanModify, *returnedPR.MaintainerCanModify)
}

// Check reviewers if they exist in the expected PR
if tc.expectedPR.RequestedReviewers != nil && len(tc.expectedPR.RequestedReviewers) > 0 {
assert.NotNil(t, returnedPR.RequestedReviewers)
assert.Equal(t, len(tc.expectedPR.RequestedReviewers), len(returnedPR.RequestedReviewers))

// Create maps of reviewer logins for easy comparison
expectedReviewers := make(map[string]bool)
for _, reviewer := range tc.expectedPR.RequestedReviewers {
expectedReviewers[*reviewer.Login] = true
}

actualReviewers := make(map[string]bool)
for _, reviewer := range returnedPR.RequestedReviewers {
actualReviewers[*reviewer.Login] = true
}

// Compare the maps
assert.Equal(t, expectedReviewers, actualReviewers)
}
})
}
}
Expand Down