Content-Length: 17401 | pFad | http://github.com/github/github-mcp-server/pull/285.patch

thub.com From 3234755fa4c67a28011716576dff4664d38f6bb6 Mon Sep 17 00:00:00 2001 From: MayorFaj Date: Tue, 15 Apr 2025 23:25:47 +0100 Subject: [PATCH 1/4] feat: add reviewers parameter to UpdatePullRequest and update tests --- README.md | 1 + pkg/github/pullrequests.go | 101 ++++++++++++++++++++++++++++---- pkg/github/pullrequests_test.go | 100 ++++++++++++++++++++++++++++++- 3 files changed, 190 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 288d7548b..98784b54b 100644 --- a/README.md +++ b/README.md @@ -327,6 +327,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 diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 2be249c8a..1c8b35aaa 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -103,6 +103,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") @@ -157,26 +163,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) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index bb3726249..3a064a399 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -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 @@ -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 @@ -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", @@ -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 { @@ -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) + } }) } } From 5c85a0940da7292f3c753edb34219437c8b4c3b4 Mon Sep 17 00:00:00 2001 From: MayorFaj <127399119+MayorFaj@users.noreply.github.com> Date: Wed, 25 Jun 2025 21:15:17 +0100 Subject: [PATCH 2/4] Update pullrequests.go --- pkg/github/pullrequests.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 6fb63fae3..caf4ef568 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -313,7 +313,6 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) } -<<<<<<< feat/259/assign-reviewers var pr *github.PullRequest var resp *http.Response @@ -360,7 +359,7 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu } return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %s", string(body))), nil } -======= + pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, @@ -368,7 +367,6 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu resp, err, ), nil ->>>>>>> main } // Add reviewers if specified From b09f5895e7c842fbd3c4ececc56d4882041191b2 Mon Sep 17 00:00:00 2001 From: MayorFaj Date: Wed, 25 Jun 2025 21:47:25 +0100 Subject: [PATCH 3/4] feat: enhance update pull request functionality with reviewers support --- .../__toolsnaps__/update_pull_request.snap | 7 ++++ pkg/github/pullrequests.go | 36 +++++++++---------- pkg/github/pullrequests_test.go | 2 +- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/pkg/github/__toolsnaps__/update_pull_request.snap b/pkg/github/__toolsnaps__/update_pull_request.snap index 765983afd..621299e43 100644 --- a/pkg/github/__toolsnaps__/update_pull_request.snap +++ b/pkg/github/__toolsnaps__/update_pull_request.snap @@ -30,6 +30,13 @@ "description": "Repository name", "type": "string" }, + "reviewers": { + "description": "GitHub usernames to request reviews from", + "items": { + "type": "string" + }, + "type": "array" + }, "state": { "description": "New state", "enum": [ diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index caf4ef568..f5be0b381 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -299,13 +299,9 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu } // 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) - } - } + reviewers, err := OptionalStringArrayParam(request, "reviewers") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil } // Create the GitHub client @@ -322,7 +318,11 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu 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) + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update pull request", + ghResp, + err, + ), nil } resp = ghResp.Response defer func() { @@ -343,7 +343,11 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu 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) + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to get pull request", + ghResp, + err, + ), nil } resp = ghResp.Response defer func() { @@ -359,14 +363,6 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu } return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %s", string(body))), nil } - - pr, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to update pull request", - resp, - err, - ), nil } // Add reviewers if specified @@ -378,7 +374,11 @@ func UpdatePullRequest(getClient GetClientFn, t translations.TranslationHelperFu // 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 request reviewers: %w", err) + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to request reviewers", + resp, + err, + ), nil } defer func() { if resp != nil && resp.Body != nil { diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 892fe1599..cd66460f6 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -390,7 +390,7 @@ func Test_UpdatePullRequest(t *testing.T) { } // Check reviewers if they exist in the expected PR - if tc.expectedPR.RequestedReviewers != nil && len(tc.expectedPR.RequestedReviewers) > 0 { + if len(tc.expectedPR.RequestedReviewers) > 0 { assert.NotNil(t, returnedPR.RequestedReviewers) assert.Equal(t, len(tc.expectedPR.RequestedReviewers), len(returnedPR.RequestedReviewers)) From 6f21c3fd3feef033f093195933dd843ad826712d Mon Sep 17 00:00:00 2001 From: MayorFaj Date: Mon, 30 Jun 2025 22:04:52 +0100 Subject: [PATCH 4/4] update README to clarify optional reviewers parameter in API documentation- go run ./cmd/github-mcp-server generate-docs --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 39572e5f3..3b4089518 100644 --- a/README.md +++ b/README.md @@ -794,9 +794,9 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number to update (number, required) - `repo`: Repository name (string, required) + - `reviewers`: GitHub usernames to request reviews from (string[], optional) - `state`: New state (string, optional) - `title`: New title (string, optional) - - `reviewers`: GitHub usernames to request reviews from (string[], optional) - **update_pull_request_branch** - Update pull request branch - `expectedHeadSha`: The expected SHA of the pull request's HEAD ref (string, optional)

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!