diff --git a/README.md b/README.md index 64303639..48bf019d 100644 --- a/README.md +++ b/README.md @@ -272,7 +272,9 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `body`: Review comment text (string, optional) - `event`: Review action ('APPROVE', 'REQUEST_CHANGES', 'COMMENT') (string, required) - `commitId`: SHA of commit to review (string, optional) - - `comments`: Line-specific comments array of objects, each object with path (string), position (number), and body (string) (array, optional) + - `comments`: Line-specific comments array of objects to place comments on pull request changes (array, optional) + - For inline comments: provide `path`, `position` (or `line`), and `body` + - For multi-line comments: provide `path`, `start_line`, `line`, optional `side`/`start_side`, and `body` - **create_pull_request** - Create a new pull request diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index c02336ca..a43d5b88 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -593,7 +593,7 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe map[string]interface{}{ "type": "object", "additionalProperties": false, - "required": []string{"path", "position", "body"}, + "required": []string{"path", "body"}, "properties": map[string]interface{}{ "path": map[string]interface{}{ "type": "string", @@ -601,7 +601,23 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe }, "position": map[string]interface{}{ "type": "number", - "description": "line number in the file", + "description": "position of the comment in the diff", + }, + "line": map[string]interface{}{ + "type": "number", + "description": "line number in the file to comment on. For multi-line comments, the end of the line range", + }, + "side": map[string]interface{}{ + "type": "string", + "description": "The side of the diff on which the line resides. For multi-line comments, this is the side for the end of the line range. (LEFT or RIGHT)", + }, + "start_line": map[string]interface{}{ + "type": "number", + "description": "The first line of the range to which the comment refers. Required for multi-line comments.", + }, + "start_side": map[string]interface{}{ + "type": "string", + "description": "The side of the diff on which the start line resides for multi-line comments. (LEFT or RIGHT)", }, "body": map[string]interface{}{ "type": "string", @@ -610,7 +626,7 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe }, }, ), - mcp.Description("Line-specific comments array of objects, each object with path (string), position (number), and body (string)"), + mcp.Description("Line-specific comments array of objects to place comments on pull request changes. Requires path and body. For line comments use line or position. For multi-line comments use start_line and line with optional side parameters."), ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ -661,7 +677,7 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe for _, c := range commentsObj { commentMap, ok := c.(map[string]interface{}) if !ok { - return mcp.NewToolResultError("each comment must be an object with path, position, and body"), nil + return mcp.NewToolResultError("each comment must be an object with path and body"), nil } path, ok := commentMap["path"].(string) @@ -669,22 +685,47 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe return mcp.NewToolResultError("each comment must have a path"), nil } - positionFloat, ok := commentMap["position"].(float64) - if !ok { - return mcp.NewToolResultError("each comment must have a position"), nil - } - position := int(positionFloat) - body, ok := commentMap["body"].(string) if !ok || body == "" { return mcp.NewToolResultError("each comment must have a body"), nil } - comments = append(comments, &github.DraftReviewComment{ - Path: github.Ptr(path), - Position: github.Ptr(position), - Body: github.Ptr(body), - }) + _, hasPosition := commentMap["position"].(float64) + _, hasLine := commentMap["line"].(float64) + _, hasSide := commentMap["side"].(string) + _, hasStartLine := commentMap["start_line"].(float64) + _, hasStartSide := commentMap["start_side"].(string) + + switch { + case !hasPosition && !hasLine: + return mcp.NewToolResultError("each comment must have either position or line"), nil + case hasPosition && (hasLine || hasSide || hasStartLine || hasStartSide): + return mcp.NewToolResultError("position cannot be combined with line, side, start_line, or start_side"), nil + case hasStartSide && !hasSide: + return mcp.NewToolResultError("if start_side is provided, side must also be provided"), nil + } + + comment := &github.DraftReviewComment{ + Path: github.Ptr(path), + Body: github.Ptr(body), + } + + if positionFloat, ok := commentMap["position"].(float64); ok { + comment.Position = github.Ptr(int(positionFloat)) + } else if lineFloat, ok := commentMap["line"].(float64); ok { + comment.Line = github.Ptr(int(lineFloat)) + } + if side, ok := commentMap["side"].(string); ok { + comment.Side = github.Ptr(side) + } + if startLineFloat, ok := commentMap["start_line"].(float64); ok { + comment.StartLine = github.Ptr(int(startLineFloat)) + } + if startSide, ok := commentMap["start_side"].(string); ok { + comment.StartSide = github.Ptr(startSide) + } + + comments = append(comments, comment) } reviewRequest.Comments = comments diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index b666e8a8..9e2e9f47 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -1167,7 +1167,152 @@ func Test_CreatePullRequestReview(t *testing.T) { }, }, expectError: false, - expectedErrMsg: "each comment must have a position", + expectedErrMsg: "each comment must have either position or line", + }, + { + name: "successful review creation with line parameter", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PostReposPullsReviewsByOwnerByRepoByPullNumber, + expectRequestBody(t, map[string]interface{}{ + "body": "Code review comments", + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "line": float64(42), + "body": "Consider adding a comment here", + }, + }, + }).andThen( + mockResponse(t, http.StatusOK, mockReview), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "body": "Code review comments", + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "line": float64(42), + "body": "Consider adding a comment here", + }, + }, + }, + expectError: false, + expectedReview: mockReview, + }, + { + name: "successful review creation with multi-line comment", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PostReposPullsReviewsByOwnerByRepoByPullNumber, + expectRequestBody(t, map[string]interface{}{ + "body": "Multi-line comment review", + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "start_line": float64(10), + "line": float64(15), + "side": "RIGHT", + "body": "This entire block needs refactoring", + }, + }, + }).andThen( + mockResponse(t, http.StatusOK, mockReview), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "body": "Multi-line comment review", + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "start_line": float64(10), + "line": float64(15), + "side": "RIGHT", + "body": "This entire block needs refactoring", + }, + }, + }, + expectError: false, + expectedReview: mockReview, + }, + { + name: "invalid multi-line comment - missing line parameter", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "start_line": float64(10), + // missing line parameter + "body": "Invalid multi-line comment", + }, + }, + }, + expectError: false, + expectedErrMsg: "each comment must have either position or line", // Updated error message + }, + { + name: "invalid comment - mixing position with line parameters", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.PostReposPullsReviewsByOwnerByRepoByPullNumber, + mockReview, + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "position": float64(5), + "line": float64(42), + "body": "Invalid parameter combination", + }, + }, + }, + expectError: false, + expectedErrMsg: "position cannot be combined with line, side, start_line, or start_side", + }, + { + name: "invalid multi-line comment - missing side parameter", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "event": "COMMENT", + "comments": []interface{}{ + map[string]interface{}{ + "path": "main.go", + "start_line": float64(10), + "line": float64(15), + "start_side": "LEFT", + // missing side parameter + "body": "Invalid multi-line comment", + }, + }, + }, + expectError: false, + expectedErrMsg: "if start_side is provided, side must also be provided", }, { name: "review creation fails",