diff --git a/README.md b/README.md index d6925d8ab..d9443c35b 100644 --- a/README.md +++ b/README.md @@ -1002,6 +1002,13 @@ Possible options: - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) +- **reply_to_review_comment** - Reply to a review comment + - `body`: Reply text supporting GitHub-flavored Markdown (string, required) + - `comment_id`: Review comment ID from pull_request_read (number, required) + - `owner`: Repository owner (string, required) + - `pull_number`: Pull request number (number, required) + - `repo`: Repository name (string, required) + - **request_copilot_review** - Request Copilot review - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number (number, required) diff --git a/pkg/github/__toolsnaps__/reply_to_review_comment.snap b/pkg/github/__toolsnaps__/reply_to_review_comment.snap new file mode 100644 index 000000000..1bb4af555 --- /dev/null +++ b/pkg/github/__toolsnaps__/reply_to_review_comment.snap @@ -0,0 +1,40 @@ +{ + "annotations": { + "title": "Reply to a review comment", + "readOnlyHint": false + }, + "description": "Reply to a review comment on a pull request. Use this to respond directly within pull request review comment threads, maintaining conversation context at specific code locations.", + "inputSchema": { + "properties": { + "body": { + "description": "Reply text supporting GitHub-flavored Markdown", + "type": "string" + }, + "comment_id": { + "description": "Review comment ID from pull_request_read", + "type": "number" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "pull_number": { + "description": "Pull request number", + "type": "number" + }, + "repo": { + "description": "Repository name", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "pull_number", + "comment_id", + "body" + ], + "type": "object" + }, + "name": "reply_to_review_comment" +} \ No newline at end of file diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 6fb5ed30b..2717e32ab 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1668,6 +1668,98 @@ func RequestCopilotReview(getClient GetClientFn, t translations.TranslationHelpe } } +// ReplyToReviewComment creates a tool to reply to a review comment on a pull request. +func ReplyToReviewComment(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { + return mcp.NewTool("reply_to_review_comment", + mcp.WithDescription(t("TOOL_REPLY_TO_REVIEW_COMMENT_DESCRIPTION", "Reply to a review comment on a pull request. Use this to respond directly within pull request review comment threads, maintaining conversation context at specific code locations.")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_REPLY_TO_REVIEW_COMMENT_USER_TITLE", "Reply to a review comment"), + ReadOnlyHint: ToBoolPtr(false), + }), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("Repository owner"), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("Repository name"), + ), + mcp.WithNumber("pull_number", + mcp.Required(), + mcp.Description("Pull request number"), + ), + mcp.WithNumber("comment_id", + mcp.Required(), + mcp.Description("Review comment ID from pull_request_read"), + ), + mcp.WithString("body", + mcp.Required(), + mcp.Description("Reply text supporting GitHub-flavored Markdown"), + ), + ), + 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, "pull_number") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + commentID, err := RequiredBigInt(request, "comment_id") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + body, err := RequiredParam[string](request, "body") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + client, err := getClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + comment, resp, err := client.PullRequests.CreateCommentInReplyTo(ctx, owner, repo, pullNumber, body, commentID) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to create reply to review comment", + resp, + err, + ), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + responseBody, 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 create reply to review comment: %s", string(responseBody))), nil + } + // Return minimal response with just essential information + minimalResponse := MinimalResponse{ + ID: fmt.Sprintf("%d", comment.GetID()), + URL: comment.GetHTMLURL(), + } + + r, err := json.Marshal(minimalResponse) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return mcp.NewToolResultText(string(r)), 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 6eac5ce83..f54ae897c 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -3105,3 +3105,259 @@ func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mo ), ) } + +func Test_ReplyToReviewComment(t *testing.T) { + // Verify tool definition once + mockClient := github.NewClient(nil) + tool, _ := ReplyToReviewComment(stubGetClientFn(mockClient), translations.NullTranslationHelper) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "reply_to_review_comment", 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, "pull_number") + assert.Contains(t, tool.InputSchema.Properties, "comment_id") + assert.Contains(t, tool.InputSchema.Properties, "body") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pull_number", "comment_id", "body"}) + + // Verify ReadOnlyHint is false for write operation + assert.NotNil(t, tool.Annotations) + assert.NotNil(t, tool.Annotations.ReadOnlyHint) + assert.False(t, *tool.Annotations.ReadOnlyHint) + + // Setup mock comment for success case + mockComment := &github.PullRequestComment{ + ID: github.Ptr(int64(67890)), + Body: github.Ptr("Thanks for the review!"), + HTMLURL: github.Ptr("https://github.com/owner/repo/pull/42#discussion_r67890"), + User: &github.User{ + Login: github.Ptr("testuser"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedReply *github.PullRequestComment + expectedErrMsg string + }{ + { + name: "successful reply creation", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{ + Pattern: "/repos/owner/repo/pulls/42/comments", + Method: http.MethodPost, + }, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusCreated) + responseBody, _ := json.Marshal(mockComment) + _, _ = w.Write(responseBody) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(12345), + "body": "Thanks for the review!", + }, + expectError: false, + expectedReply: mockComment, + }, + { + name: "comment not found", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{ + Pattern: "/repos/owner/repo/pulls/42/comments", + Method: http.MethodPost, + }, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(99999), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "failed to create reply to review comment", + }, + { + name: "permission denied", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{ + Pattern: "/repos/owner/repo/pulls/42/comments", + Method: http.MethodPost, + }, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(12345), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "failed to create reply to review comment", + }, + { + name: "validation failure - unprocessable entity", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.EndpointPattern{ + Pattern: "/repos/owner/repo/pulls/42/comments", + Method: http.MethodPost, + }, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message":"Validation failed","errors":[{"resource":"PullRequestComment","code":"invalid"}]}`)) + }), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(12345), + "body": "Some reply text", + }, + expectError: true, + expectedErrMsg: "failed to create reply to review comment", + }, + { + name: "missing required parameter - owner", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(12345), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "missing required parameter: owner", + }, + { + name: "missing required parameter - repo", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "pull_number": float64(42), + "comment_id": float64(12345), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "missing required parameter: repo", + }, + { + name: "missing required parameter - pull_number", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "comment_id": float64(12345), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "missing required parameter: pull_number", + }, + { + name: "missing required parameter - comment_id", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "missing required parameter: comment_id", + }, + { + name: "missing required parameter - body", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": float64(12345), + }, + expectError: true, + expectedErrMsg: "missing required parameter: body", + }, + { + name: "invalid comment_id type", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pull_number": float64(42), + "comment_id": "not-a-number", + "body": "Reply", + }, + expectError: true, + expectedErrMsg: "comment_id", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Setup client with mock + client := github.NewClient(tc.mockedClient) + _, handler := ReplyToReviewComment(stubGetClientFn(client), translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + + // Verify results + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + + // If no error returned but result contains error + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedErrMsg) + return + } + + require.NoError(t, err) + + // Debug: check if result is an error + if result.IsError { + textContent := getTextResult(t, result) + t.Fatalf("Expected successful result but got error: %s", textContent.Text) + } + + // Parse the result and get the text content if no error + textContent := getTextResult(t, result) + + // Unmarshal and verify the minimal result + var returnedReply MinimalResponse + err = json.Unmarshal([]byte(textContent.Text), &returnedReply) + require.NoError(t, err) + assert.Equal(t, "67890", returnedReply.ID) + assert.Equal(t, tc.expectedReply.GetHTMLURL(), returnedReply.URL) + }) + } +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index a5605ec04..0c756c4a8 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -239,6 +239,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG // Reviews toolsets.NewServerTool(PullRequestReviewWrite(getGQLClient, t)), toolsets.NewServerTool(AddCommentToPendingReview(getGQLClient, t)), + toolsets.NewServerTool(ReplyToReviewComment(getClient, t)), ) codeSecurity := toolsets.NewToolset(ToolsetMetadataCodeSecurity.ID, ToolsetMetadataCodeSecurity.Description). AddReadTools(