Skip to content

Commit ce6df32

Browse files
ashwin-antclaude
ashwin-ant
and
claude
committed
Unify PR review comment tools into a single consolidated tool
The separate AddPullRequestReviewComment and ReplyToPullRequestReviewComment tools have been merged into a single tool that handles both creating new comments and replying to existing ones. This approach simplifies the API and provides a more consistent interface for users. - Made commit_id and path optional when using in_reply_to for replies - Updated the tests to verify both comment and reply functionality - Removed the separate ReplyToPullRequestReviewComment tool - Fixed test expectations to match how errors are returned 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent bab5c0e commit ce6df32

3 files changed

Lines changed: 128 additions & 215 deletions

File tree

‎pkg/github/pullrequests.go‎

Lines changed: 62 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -665,12 +665,10 @@ func AddPullRequestReviewComment(getClient GetClientFn, t translations.Translati
665665
mcp.Description("The text of the review comment"),
666666
),
667667
mcp.WithString("commit_id",
668-
mcp.Required(),
669-
mcp.Description("The SHA of the commit to comment on"),
668+
mcp.Description("The SHA of the commit to comment on. Required unless in_reply_to is specified."),
670669
),
671670
mcp.WithString("path",
672-
mcp.Required(),
673-
mcp.Description("The relative path to the file that necessitates a comment"),
671+
mcp.Description("The relative path to the file that necessitates a comment. Required unless in_reply_to is specified."),
674672
),
675673
mcp.WithString("subject_type",
676674
mcp.Description("The level at which the comment is targeted, 'line' or 'file'"),
@@ -691,7 +689,7 @@ func AddPullRequestReviewComment(getClient GetClientFn, t translations.Translati
691689
mcp.Enum("LEFT", "RIGHT"),
692690
),
693691
mcp.WithNumber("in_reply_to",
694-
mcp.Description("The ID of the review comment to reply to. When specified, all parameters other than body are ignored"),
692+
mcp.Description("The ID of the review comment to reply to. When specified, only body is required and all other parameters are ignored"),
695693
),
696694
),
697695
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@ -711,161 +709,103 @@ func AddPullRequestReviewComment(getClient GetClientFn, t translations.Translati
711709
if err != nil {
712710
return mcp.NewToolResultError(err.Error()), nil
713711
}
714-
commitID, err := requiredParam[string](request, "commit_id")
715-
if err != nil {
716-
return mcp.NewToolResultError(err.Error()), nil
717-
}
718-
path, err := requiredParam[string](request, "path")
719-
if err != nil {
720-
return mcp.NewToolResultError(err.Error()), nil
721-
}
722712

723-
comment := &github.PullRequestComment{
724-
Body: github.Ptr(body),
725-
CommitID: github.Ptr(commitID),
726-
Path: github.Ptr(path),
713+
client, err := getClient(ctx)
714+
if err != nil {
715+
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
727716
}
728717

729-
// Check for in_reply_to parameter which takes precedence
718+
// Check if this is a reply to an existing comment
730719
if replyToFloat, ok := request.Params.Arguments["in_reply_to"].(float64); ok {
731-
comment.InReplyTo = github.Ptr(int64(replyToFloat))
732-
} else {
733-
// Handle subject_type parameter
734-
subjectType, err := OptionalParam[string](request, "subject_type")
720+
// Use the specialized method for reply comments due to inconsistency in underlying go-github library: https://github.com/google/go-github/pull/950
721+
commentID := int64(replyToFloat)
722+
createdReply, resp, err := client.PullRequests.CreateCommentInReplyTo(ctx, owner, repo, pullNumber, body, commentID)
735723
if err != nil {
736-
return mcp.NewToolResultError(err.Error()), nil
724+
return nil, fmt.Errorf("failed to reply to pull request comment: %w", err)
737725
}
738-
if subjectType != "file" {
739-
// Handle line or position-based comments
740-
// No action needed if subjectType is "file"
741-
line, lineExists := request.Params.Arguments["line"].(float64)
742-
startLine, startLineExists := request.Params.Arguments["start_line"].(float64)
743-
side, sideExists := request.Params.Arguments["side"].(string)
744-
startSide, startSideExists := request.Params.Arguments["start_side"].(string)
745-
746-
if subjectType != "file" && !lineExists {
747-
return mcp.NewToolResultError("line parameter is required unless using subject_type:file or in_reply_to"), nil
748-
}
726+
defer func() { _ = resp.Body.Close() }()
749727

750-
if lineExists {
751-
comment.Line = github.Ptr(int(line))
752-
}
753-
if sideExists {
754-
comment.Side = github.Ptr(side)
755-
}
756-
if startLineExists {
757-
comment.StartLine = github.Ptr(int(startLine))
758-
}
759-
if startSideExists {
760-
comment.StartSide = github.Ptr(startSide)
761-
}
762-
763-
// Validate multi-line comment parameters
764-
if startLineExists && !lineExists {
765-
return mcp.NewToolResultError("if start_line is provided, line must also be provided"), nil
766-
}
767-
if startSideExists && !sideExists {
768-
return mcp.NewToolResultError("if start_side is provided, side must also be provided"), nil
728+
if resp.StatusCode != http.StatusCreated {
729+
respBody, err := io.ReadAll(resp.Body)
730+
if err != nil {
731+
return nil, fmt.Errorf("failed to read response body: %w", err)
769732
}
733+
return mcp.NewToolResultError(fmt.Sprintf("failed to reply to pull request comment: %s", string(respBody))), nil
770734
}
771-
}
772-
773-
client, err := getClient(ctx)
774-
if err != nil {
775-
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
776-
}
777-
778-
createdComment, resp, err := client.PullRequests.CreateComment(ctx, owner, repo, pullNumber, comment)
779-
if err != nil {
780-
return nil, fmt.Errorf("failed to create pull request comment: %w", err)
781-
}
782-
defer func() { _ = resp.Body.Close() }()
783735

784-
if resp.StatusCode != http.StatusCreated {
785-
body, err := io.ReadAll(resp.Body)
736+
r, err := json.Marshal(createdReply)
786737
if err != nil {
787-
return nil, fmt.Errorf("failed to read response body: %w", err)
738+
return nil, fmt.Errorf("failed to marshal response: %w", err)
788739
}
789-
return mcp.NewToolResultError(fmt.Sprintf("failed to create pull request comment: %s", string(body))), nil
790-
}
791740

792-
r, err := json.Marshal(createdComment)
793-
if err != nil {
794-
return nil, fmt.Errorf("failed to marshal response: %w", err)
741+
return mcp.NewToolResultText(string(r)), nil
795742
}
796743

797-
return mcp.NewToolResultText(string(r)), nil
798-
}
799-
}
800-
801-
// ReplyToPullRequestReviewComment creates a tool to reply to an existing review comment on a pull request.
802-
func ReplyToPullRequestReviewComment(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool,
803-
handler server.ToolHandlerFunc) {
804-
return mcp.NewTool("reply_to_pull_request_review_comment",
805-
mcp.WithDescription(t("TOOL_REPLY_TO_PULL_REQUEST_REVIEW_COMMENT_DESCRIPTION", "Reply to an existing review comment on a pull request")),
806-
mcp.WithString("owner",
807-
mcp.Required(),
808-
mcp.Description("Repository owner"),
809-
),
810-
mcp.WithString("repo",
811-
mcp.Required(),
812-
mcp.Description("Repository name"),
813-
),
814-
mcp.WithNumber("pull_number",
815-
mcp.Required(),
816-
mcp.Description("Pull request number"),
817-
),
818-
mcp.WithNumber("comment_id",
819-
mcp.Required(),
820-
mcp.Description("The unique identifier of the comment to reply to"),
821-
),
822-
mcp.WithString("body",
823-
mcp.Required(),
824-
mcp.Description("The text of the reply comment"),
825-
),
826-
),
827-
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
828-
owner, err := requiredParam[string](request, "owner")
829-
if err != nil {
830-
return mcp.NewToolResultError(err.Error()), nil
831-
}
832-
repo, err := requiredParam[string](request, "repo")
744+
// This is a new comment, not a reply
745+
// Verify required parameters for a new comment
746+
commitID, err := requiredParam[string](request, "commit_id")
833747
if err != nil {
834748
return mcp.NewToolResultError(err.Error()), nil
835749
}
836-
pullNumber, err := RequiredInt(request, "pull_number")
750+
path, err := requiredParam[string](request, "path")
837751
if err != nil {
838752
return mcp.NewToolResultError(err.Error()), nil
839753
}
840-
commentID, err := RequiredInt(request, "comment_id")
841-
if err != nil {
842-
return mcp.NewToolResultError(err.Error()), nil
754+
755+
comment := &github.PullRequestComment{
756+
Body: github.Ptr(body),
757+
CommitID: github.Ptr(commitID),
758+
Path: github.Ptr(path),
843759
}
844-
body, err := requiredParam[string](request, "body")
760+
761+
subjectType, err := OptionalParam[string](request, "subject_type")
845762
if err != nil {
846763
return mcp.NewToolResultError(err.Error()), nil
847764
}
765+
if subjectType != "file" {
766+
line, lineExists := request.Params.Arguments["line"].(float64)
767+
startLine, startLineExists := request.Params.Arguments["start_line"].(float64)
768+
side, sideExists := request.Params.Arguments["side"].(string)
769+
startSide, startSideExists := request.Params.Arguments["start_side"].(string)
848770

849-
client, err := getClient(ctx)
850-
if err != nil {
851-
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
771+
if !lineExists {
772+
return mcp.NewToolResultError("line parameter is required unless using subject_type:file"), nil
773+
}
774+
775+
comment.Line = github.Ptr(int(line))
776+
if sideExists {
777+
comment.Side = github.Ptr(side)
778+
}
779+
if startLineExists {
780+
comment.StartLine = github.Ptr(int(startLine))
781+
}
782+
if startSideExists {
783+
comment.StartSide = github.Ptr(startSide)
784+
}
785+
786+
if startLineExists && !lineExists {
787+
return mcp.NewToolResultError("if start_line is provided, line must also be provided"), nil
788+
}
789+
if startSideExists && !sideExists {
790+
return mcp.NewToolResultError("if start_side is provided, side must also be provided"), nil
791+
}
852792
}
853793

854-
createdReply, resp, err := client.PullRequests.CreateCommentInReplyTo(ctx, owner, repo, pullNumber, body, int64(commentID))
794+
createdComment, resp, err := client.PullRequests.CreateComment(ctx, owner, repo, pullNumber, comment)
855795
if err != nil {
856-
return nil, fmt.Errorf("failed to reply to pull request comment: %w", err)
796+
return nil, fmt.Errorf("failed to create pull request comment: %w", err)
857797
}
858798
defer func() { _ = resp.Body.Close() }()
859799

860800
if resp.StatusCode != http.StatusCreated {
861-
body, err := io.ReadAll(resp.Body)
801+
respBody, err := io.ReadAll(resp.Body)
862802
if err != nil {
863803
return nil, fmt.Errorf("failed to read response body: %w", err)
864804
}
865-
return mcp.NewToolResultError(fmt.Sprintf("failed to reply to pull request comment: %s", string(body))), nil
805+
return mcp.NewToolResultError(fmt.Sprintf("failed to create pull request comment: %s", string(respBody))), nil
866806
}
867807

868-
r, err := json.Marshal(createdReply)
808+
r, err := json.Marshal(createdComment)
869809
if err != nil {
870810
return nil, fmt.Errorf("failed to marshal response: %w", err)
871811
}

0 commit comments

Comments
 (0)