Skip to content

Commit 31471a4

Browse files
ashwin-antClaudejuruen
authored
Add line parameter support to create_pull_request_review tool (#118)
* Add line parameter support to create_pull_request_review tool - Updated schema to make path and body the only required fields - Added line parameter as alternative to position for inline comments - Updated handler to accept either position or line based on GitHub API spec - Added new test case that verifies line parameter works properly - Updated error messages for better validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * Expand PR review API with multi-line comment support - Added new parameters: line, side, start_line, start_side - Added proper validation for multi-line comment parameters - Improved validation logic to handle parameter combinations - Added test cases for regular and multi-line comments - Updated schema documentation for better tool discoverability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> * gofmt --------- Co-authored-by: Claude <[email protected]> Co-authored-by: Javier Uruen Val <[email protected]>
1 parent c77ea94 commit 31471a4

File tree

3 files changed

+205
-17
lines changed

3 files changed

+205
-17
lines changed

‎README.md

+3-1
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,9 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description
272272
- `body`: Review comment text (string, optional)
273273
- `event`: Review action ('APPROVE', 'REQUEST_CHANGES', 'COMMENT') (string, required)
274274
- `commitId`: SHA of commit to review (string, optional)
275-
- `comments`: Line-specific comments array of objects, each object with path (string), position (number), and body (string) (array, optional)
275+
- `comments`: Line-specific comments array of objects to place comments on pull request changes (array, optional)
276+
- For inline comments: provide `path`, `position` (or `line`), and `body`
277+
- For multi-line comments: provide `path`, `start_line`, `line`, optional `side`/`start_side`, and `body`
276278

277279
- **create_pull_request** - Create a new pull request
278280

‎pkg/github/pullrequests.go

+56-15
Original file line numberDiff line numberDiff line change
@@ -593,15 +593,31 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe
593593
map[string]interface{}{
594594
"type": "object",
595595
"additionalProperties": false,
596-
"required": []string{"path", "position", "body"},
596+
"required": []string{"path", "body"},
597597
"properties": map[string]interface{}{
598598
"path": map[string]interface{}{
599599
"type": "string",
600600
"description": "path to the file",
601601
},
602602
"position": map[string]interface{}{
603603
"type": "number",
604-
"description": "line number in the file",
604+
"description": "position of the comment in the diff",
605+
},
606+
"line": map[string]interface{}{
607+
"type": "number",
608+
"description": "line number in the file to comment on. For multi-line comments, the end of the line range",
609+
},
610+
"side": map[string]interface{}{
611+
"type": "string",
612+
"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)",
613+
},
614+
"start_line": map[string]interface{}{
615+
"type": "number",
616+
"description": "The first line of the range to which the comment refers. Required for multi-line comments.",
617+
},
618+
"start_side": map[string]interface{}{
619+
"type": "string",
620+
"description": "The side of the diff on which the start line resides for multi-line comments. (LEFT or RIGHT)",
605621
},
606622
"body": map[string]interface{}{
607623
"type": "string",
@@ -610,7 +626,7 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe
610626
},
611627
},
612628
),
613-
mcp.Description("Line-specific comments array of objects, each object with path (string), position (number), and body (string)"),
629+
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."),
614630
),
615631
),
616632
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@ -661,30 +677,55 @@ func createPullRequestReview(client *github.Client, t translations.TranslationHe
661677
for _, c := range commentsObj {
662678
commentMap, ok := c.(map[string]interface{})
663679
if !ok {
664-
return mcp.NewToolResultError("each comment must be an object with path, position, and body"), nil
680+
return mcp.NewToolResultError("each comment must be an object with path and body"), nil
665681
}
666682

667683
path, ok := commentMap["path"].(string)
668684
if !ok || path == "" {
669685
return mcp.NewToolResultError("each comment must have a path"), nil
670686
}
671687

672-
positionFloat, ok := commentMap["position"].(float64)
673-
if !ok {
674-
return mcp.NewToolResultError("each comment must have a position"), nil
675-
}
676-
position := int(positionFloat)
677-
678688
body, ok := commentMap["body"].(string)
679689
if !ok || body == "" {
680690
return mcp.NewToolResultError("each comment must have a body"), nil
681691
}
682692

683-
comments = append(comments, &github.DraftReviewComment{
684-
Path: github.Ptr(path),
685-
Position: github.Ptr(position),
686-
Body: github.Ptr(body),
687-
})
693+
_, hasPosition := commentMap["position"].(float64)
694+
_, hasLine := commentMap["line"].(float64)
695+
_, hasSide := commentMap["side"].(string)
696+
_, hasStartLine := commentMap["start_line"].(float64)
697+
_, hasStartSide := commentMap["start_side"].(string)
698+
699+
switch {
700+
case !hasPosition && !hasLine:
701+
return mcp.NewToolResultError("each comment must have either position or line"), nil
702+
case hasPosition && (hasLine || hasSide || hasStartLine || hasStartSide):
703+
return mcp.NewToolResultError("position cannot be combined with line, side, start_line, or start_side"), nil
704+
case hasStartSide && !hasSide:
705+
return mcp.NewToolResultError("if start_side is provided, side must also be provided"), nil
706+
}
707+
708+
comment := &github.DraftReviewComment{
709+
Path: github.Ptr(path),
710+
Body: github.Ptr(body),
711+
}
712+
713+
if positionFloat, ok := commentMap["position"].(float64); ok {
714+
comment.Position = github.Ptr(int(positionFloat))
715+
} else if lineFloat, ok := commentMap["line"].(float64); ok {
716+
comment.Line = github.Ptr(int(lineFloat))
717+
}
718+
if side, ok := commentMap["side"].(string); ok {
719+
comment.Side = github.Ptr(side)
720+
}
721+
if startLineFloat, ok := commentMap["start_line"].(float64); ok {
722+
comment.StartLine = github.Ptr(int(startLineFloat))
723+
}
724+
if startSide, ok := commentMap["start_side"].(string); ok {
725+
comment.StartSide = github.Ptr(startSide)
726+
}
727+
728+
comments = append(comments, comment)
688729
}
689730

690731
reviewRequest.Comments = comments

‎pkg/github/pullrequests_test.go

+146-1
Original file line numberDiff line numberDiff line change
@@ -1167,7 +1167,152 @@ func Test_CreatePullRequestReview(t *testing.T) {
11671167
},
11681168
},
11691169
expectError: false,
1170-
expectedErrMsg: "each comment must have a position",
1170+
expectedErrMsg: "each comment must have either position or line",
1171+
},
1172+
{
1173+
name: "successful review creation with line parameter",
1174+
mockedClient: mock.NewMockedHTTPClient(
1175+
mock.WithRequestMatchHandler(
1176+
mock.PostReposPullsReviewsByOwnerByRepoByPullNumber,
1177+
expectRequestBody(t, map[string]interface{}{
1178+
"body": "Code review comments",
1179+
"event": "COMMENT",
1180+
"comments": []interface{}{
1181+
map[string]interface{}{
1182+
"path": "main.go",
1183+
"line": float64(42),
1184+
"body": "Consider adding a comment here",
1185+
},
1186+
},
1187+
}).andThen(
1188+
mockResponse(t, http.StatusOK, mockReview),
1189+
),
1190+
),
1191+
),
1192+
requestArgs: map[string]interface{}{
1193+
"owner": "owner",
1194+
"repo": "repo",
1195+
"pullNumber": float64(42),
1196+
"body": "Code review comments",
1197+
"event": "COMMENT",
1198+
"comments": []interface{}{
1199+
map[string]interface{}{
1200+
"path": "main.go",
1201+
"line": float64(42),
1202+
"body": "Consider adding a comment here",
1203+
},
1204+
},
1205+
},
1206+
expectError: false,
1207+
expectedReview: mockReview,
1208+
},
1209+
{
1210+
name: "successful review creation with multi-line comment",
1211+
mockedClient: mock.NewMockedHTTPClient(
1212+
mock.WithRequestMatchHandler(
1213+
mock.PostReposPullsReviewsByOwnerByRepoByPullNumber,
1214+
expectRequestBody(t, map[string]interface{}{
1215+
"body": "Multi-line comment review",
1216+
"event": "COMMENT",
1217+
"comments": []interface{}{
1218+
map[string]interface{}{
1219+
"path": "main.go",
1220+
"start_line": float64(10),
1221+
"line": float64(15),
1222+
"side": "RIGHT",
1223+
"body": "This entire block needs refactoring",
1224+
},
1225+
},
1226+
}).andThen(
1227+
mockResponse(t, http.StatusOK, mockReview),
1228+
),
1229+
),
1230+
),
1231+
requestArgs: map[string]interface{}{
1232+
"owner": "owner",
1233+
"repo": "repo",
1234+
"pullNumber": float64(42),
1235+
"body": "Multi-line comment review",
1236+
"event": "COMMENT",
1237+
"comments": []interface{}{
1238+
map[string]interface{}{
1239+
"path": "main.go",
1240+
"start_line": float64(10),
1241+
"line": float64(15),
1242+
"side": "RIGHT",
1243+
"body": "This entire block needs refactoring",
1244+
},
1245+
},
1246+
},
1247+
expectError: false,
1248+
expectedReview: mockReview,
1249+
},
1250+
{
1251+
name: "invalid multi-line comment - missing line parameter",
1252+
mockedClient: mock.NewMockedHTTPClient(),
1253+
requestArgs: map[string]interface{}{
1254+
"owner": "owner",
1255+
"repo": "repo",
1256+
"pullNumber": float64(42),
1257+
"event": "COMMENT",
1258+
"comments": []interface{}{
1259+
map[string]interface{}{
1260+
"path": "main.go",
1261+
"start_line": float64(10),
1262+
// missing line parameter
1263+
"body": "Invalid multi-line comment",
1264+
},
1265+
},
1266+
},
1267+
expectError: false,
1268+
expectedErrMsg: "each comment must have either position or line", // Updated error message
1269+
},
1270+
{
1271+
name: "invalid comment - mixing position with line parameters",
1272+
mockedClient: mock.NewMockedHTTPClient(
1273+
mock.WithRequestMatch(
1274+
mock.PostReposPullsReviewsByOwnerByRepoByPullNumber,
1275+
mockReview,
1276+
),
1277+
),
1278+
requestArgs: map[string]interface{}{
1279+
"owner": "owner",
1280+
"repo": "repo",
1281+
"pullNumber": float64(42),
1282+
"event": "COMMENT",
1283+
"comments": []interface{}{
1284+
map[string]interface{}{
1285+
"path": "main.go",
1286+
"position": float64(5),
1287+
"line": float64(42),
1288+
"body": "Invalid parameter combination",
1289+
},
1290+
},
1291+
},
1292+
expectError: false,
1293+
expectedErrMsg: "position cannot be combined with line, side, start_line, or start_side",
1294+
},
1295+
{
1296+
name: "invalid multi-line comment - missing side parameter",
1297+
mockedClient: mock.NewMockedHTTPClient(),
1298+
requestArgs: map[string]interface{}{
1299+
"owner": "owner",
1300+
"repo": "repo",
1301+
"pullNumber": float64(42),
1302+
"event": "COMMENT",
1303+
"comments": []interface{}{
1304+
map[string]interface{}{
1305+
"path": "main.go",
1306+
"start_line": float64(10),
1307+
"line": float64(15),
1308+
"start_side": "LEFT",
1309+
// missing side parameter
1310+
"body": "Invalid multi-line comment",
1311+
},
1312+
},
1313+
},
1314+
expectError: false,
1315+
expectedErrMsg: "if start_side is provided, side must also be provided",
11711316
},
11721317
{
11731318
name: "review creation fails",

0 commit comments

Comments
 (0)