From 91f58cf3d16c5b5d7ec2a1e15bcf22ab66d1754c Mon Sep 17 00:00:00 2001 From: Saba-Fatima Date: Mon, 29 Sep 2025 16:59:49 +1300 Subject: [PATCH 1/2] feat: add update_pull_request_review tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a new MCP tool `update_pull_request_review` that allows updating an existing pull request review (body, event, commit SHA). This avoids dismissing + recreating reviews and keeps PR timelines clean without “dismissed” markers. Highly suitable for agents/bots that need to refresh their reviews on new commits without spamming the PR timeline. --- README.md | 9 ++ .../update_pull_request_review.snap | 52 +++++++ pkg/github/pullrequests.go | 132 ++++++++++++++++++ pkg/github/pullrequests_test.go | 120 ++++++++++++++++ pkg/github/tools.go | 1 + 5 files changed, 314 insertions(+) create mode 100644 pkg/github/__toolsnaps__/update_pull_request_review.snap diff --git a/README.md b/README.md index 3b0cd861f..729b19a24 100644 --- a/README.md +++ b/README.md @@ -812,6 +812,15 @@ The following sets of tools are available (all are on by default): - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) +- **update_pull_request_review** - Update pull request review + - `body`: New body text for the review (string, optional) + - `commitID`: SHA of commit to update the review against (string, optional) + - `event`: New review state (string, optional) + - `owner`: Repository owner (string, required) + - `pullNumber`: Pull request number (number, required) + - `repo`: Repository name (string, required) + - `reviewID`: The ID of the review to update (number, required) +
diff --git a/pkg/github/__toolsnaps__/update_pull_request_review.snap b/pkg/github/__toolsnaps__/update_pull_request_review.snap new file mode 100644 index 000000000..f02ce4a13 --- /dev/null +++ b/pkg/github/__toolsnaps__/update_pull_request_review.snap @@ -0,0 +1,52 @@ +{ + "annotations": { + "title": "Update pull request review", + "readOnlyHint": false + }, + "description": "Update an existing pull request review (body, event, commit ID).", + "inputSchema": { + "properties": { + "body": { + "description": "New body text for the review", + "type": "string" + }, + "commitID": { + "description": "SHA of commit to update the review against", + "type": "string" + }, + "event": { + "description": "New review state", + "enum": [ + "COMMENT", + "APPROVE", + "REQUEST_CHANGES" + ], + "type": "string" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "pullNumber": { + "description": "Pull request number", + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "reviewID": { + "description": "The ID of the review to update", + "type": "number" + } + }, + "required": [ + "owner", + "repo", + "pullNumber", + "reviewID" + ], + "type": "object" + }, + "name": "update_pull_request_review" +} \ No newline at end of file diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 8289a4e48..621a95714 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "bytes" "github.com/go-viper/mapstructure/v2" "github.com/google/go-github/v74/github" @@ -1870,6 +1871,137 @@ func RequestCopilotReview(getClient GetClientFn, t translations.TranslationHelpe } } + +// UpdatePullRequestReview provides a tool to update an existing pull request review. +// Use this when you want to modify your own BOT review (body, event, or commit SHA) +// instead of dismissing and recreating a new one. +// This keeps the PR timeline clean (no dismissed markers). +func UpdatePullRequestReview(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { + return mcp.NewTool( + "update_pull_request_review", + mcp.WithDescription(t( + "TOOL_UPDATE_PULL_REQUEST_REVIEW_DESCRIPTION", + "Update an existing pull request review (body, event, commit ID).", + )), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_UPDATE_PULL_REQUEST_REVIEW_USER_TITLE", "Update pull request review"), + ReadOnlyHint: ToBoolPtr(false), + }), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("Repository owner"), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("Repository name"), + ), + mcp.WithNumber("pullNumber", + mcp.Required(), + mcp.Description("Pull request number"), + ), + mcp.WithNumber("reviewID", + mcp.Required(), + mcp.Description("The ID of the review to update"), + ), + mcp.WithString("body", + mcp.Description("New body text for the review"), + ), + mcp.WithString("event", + mcp.Description("New review state"), + mcp.Enum("COMMENT", "APPROVE", "REQUEST_CHANGES"), + ), + mcp.WithString("commitID", + mcp.Description("SHA of commit to update the review against"), + ), + ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + owner, err := RequiredParam[string](request, "owner") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + repo, err := RequiredParam[string](request, "repo") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + pullNumber, err := RequiredInt(request, "pullNumber") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + reviewID, err := RequiredInt(request, "reviewID") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + body, _ := OptionalParam[string](request, "body") + event, _ := OptionalParam[string](request, "event") + commitID, _ := OptionalParam[string](request, "commitID") + + client, err := getClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + // Build request body + reqBody := map[string]any{} + if body != "" { + reqBody["body"] = body + } + if event != "" { + reqBody["event"] = event + } + if commitID != "" { + reqBody["commit_id"] = commitID + } + + // Encode JSON + jsonBody, _ := json.Marshal(reqBody) + + // Construct API path + url := fmt.Sprintf("repos/%s/%s/pulls/%d/reviews/%d", owner, repo, pullNumber, reviewID) + + // Create PATCH request + req, err := client.NewRequest("PATCH", url, bytes.NewReader(jsonBody)) + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to build request: %v", err)), nil + } + + // Execute request + review := new(github.PullRequestReview) + resp, err := client.Do(ctx, req, review) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update pull request review", + resp, + err, + ), nil + } + defer func() { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + }() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + return mcp.NewToolResultError(fmt.Sprintf("failed to update review: %s", string(bodyBytes))), nil + } + + // Minimal response back to client + type minimal struct { + ReviewID int64 `json:"reviewID"` + State string `json:"state"` + Body string `json:"body,omitempty"` + URL string `json:"url,omitempty"` + } + out := minimal{ + ReviewID: int64(reviewID), + State: review.GetState(), + Body: review.GetBody(), + URL: review.GetHTMLURL(), + } + b, _ := json.Marshal(out) + return mcp.NewToolResultText(string(b)), nil + } +} + // newGQLString like takes something that approximates a string (of which there are many types in shurcooL/githubv4) // and constructs a pointer to it, or nil if the string is empty. This is extremely useful because when we parse // params from the MCP request, we need to convert them to types that are pointers of type def strings and it's diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 18fc8d87d..83238a370 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -6,6 +6,8 @@ import ( "net/http" "testing" "time" + "net/http/httptest" + "net/url" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" @@ -2910,3 +2912,121 @@ func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mo ), ) } + +func TestUpdatePullRequestReview(t *testing.T) { + // Snapshot test for tool definition + mockClient := github.NewClient(nil) + tool, _ := UpdatePullRequestReview(stubGetClientFn(mockClient), translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "update_pull_request_review", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "pullNumber") + assert.Contains(t, tool.InputSchema.Properties, "reviewID") + // body, event, commitID are optional + assert.Subset(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber", "reviewID"}) + + t.Run("successful update", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/pulls/42/reviews/201", func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPatch, r.Method) + + _ = json.NewEncoder(w).Encode(&github.PullRequestReview{ + ID: github.Ptr(int64(201)), + State: github.Ptr("COMMENT"), + Body: github.Ptr("updated body"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42#review-201"), + }) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + getClient := func(ctx context.Context) (*github.Client, error) { + c := github.NewClient(nil) + c.BaseURL = mustParseURL(srv.URL + "/") + return c, nil + } + + _, handler := UpdatePullRequestReview(getClient, translations.NullTranslationHelper) + req := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "reviewID": float64(201), + "body": "updated body", + "event": "COMMENT", + }) + + result, err := handler(context.Background(), req) + require.NoError(t, err) + require.False(t, result.IsError) + + text := getTextResult(t, result) + assert.Contains(t, text.Text, `"state":"COMMENT"`) + assert.Contains(t, text.Text, `"body":"updated body"`) + }) + + t.Run("API error", func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/pulls/42/reviews/202", func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPatch, r.Method) + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"Bad request"}`)) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + getClient := func(ctx context.Context) (*github.Client, error) { + c := github.NewClient(nil) + c.BaseURL = mustParseURL(srv.URL + "/") + return c, nil + } + + _, handler := UpdatePullRequestReview(getClient, translations.NullTranslationHelper) + req := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "reviewID": float64(202), + "body": "new", + }) + + result, err := handler(context.Background(), req) + require.NoError(t, err) + require.True(t, result.IsError) + + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, "failed to update pull request review") + }) + + t.Run("missing required parameter", func(t *testing.T) { + client := github.NewClient(nil) + _, handler := UpdatePullRequestReview(stubGetClientFn(client), translations.NullTranslationHelper) + + // missing reviewID + req := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + // no reviewID + }) + + result, err := handler(context.Background(), req) + require.NoError(t, err) + require.True(t, result.IsError) + + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, "missing required parameter") + }) +} + +// Helper for parsing test server URLs +func mustParseURL(raw string) *url.URL { + u, err := url.Parse(raw) + if err != nil { + panic(err) + } + return u +} \ No newline at end of file diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 2de9c23ca..2684c2e38 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -106,6 +106,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG toolsets.NewServerTool(AddCommentToPendingReview(getGQLClient, t)), toolsets.NewServerTool(SubmitPendingPullRequestReview(getGQLClient, t)), toolsets.NewServerTool(DeletePendingPullRequestReview(getGQLClient, t)), + toolsets.NewServerTool(UpdatePullRequestReview(getClient, t)), ) codeSecurity := toolsets.NewToolset("code_security", "Code security related tools, such as GitHub Code Scanning"). AddReadTools( From 6c3a74471bb414931001dfeeca3713cdb41ac431 Mon Sep 17 00:00:00 2001 From: Saba-Fatima Date: Mon, 29 Sep 2025 17:31:47 +1300 Subject: [PATCH 2/2] fixed linting issue --- pkg/github/pullrequests_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 83238a370..ebf16f9f3 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -4,10 +4,10 @@ import ( "context" "encoding/json" "net/http" + "net/http/httptest" + "net/url" "testing" "time" - "net/http/httptest" - "net/url" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" @@ -2943,7 +2943,7 @@ func TestUpdatePullRequestReview(t *testing.T) { srv := httptest.NewServer(mux) defer srv.Close() - getClient := func(ctx context.Context) (*github.Client, error) { + getClient := func(_ context.Context) (*github.Client, error) { c := github.NewClient(nil) c.BaseURL = mustParseURL(srv.URL + "/") return c, nil @@ -2978,7 +2978,7 @@ func TestUpdatePullRequestReview(t *testing.T) { srv := httptest.NewServer(mux) defer srv.Close() - getClient := func(ctx context.Context) (*github.Client, error) { + getClient := func(_ context.Context) (*github.Client, error) { c := github.NewClient(nil) c.BaseURL = mustParseURL(srv.URL + "/") return c, nil @@ -3029,4 +3029,4 @@ func mustParseURL(raw string) *url.URL { panic(err) } return u -} \ No newline at end of file +}