Split PR review creation, commenting and submission, and deletion #381

williammartin · 2025-05-07T15:54:14Z

Description

Relates to: #343

@toby @SamMorrowDrums before I go any further cleaning up the new and old tools, handling edge cases and adding unit, I want to get your feedback on this approach, since there have been quite a few pain points along the way.

The original issue suggested removing comments from create_pull_request_review and depending on add_pull_request_review_comment, however, add_pull_request_review_comment uses the REST API which is only able to create a single comment and only if a review is not already in progress.

Thus, we need to lean on GraphQL here to:

Create a pending review (mvp_create_pending_pull_request_review)
Add a comment to that pending review (mvp_add_pull_request_review_comment_to_pending_review)
Submit the pending review (mvp_submit_pull_request_review)

The implicit requirement here is that a user may only have one pending review at a time, so if we get their most recent review on a PR, and it is pending, we can add a comment to that. This does not allow commenting on other reviews and it does not allow commenting on already submitted reviews, which could be further enhancements in the same or in different tools.

There is an additional challenge since we are now using GraphQL which is that for the most part, queries and mutations operate on GraphQL IDs (node_id in the REST API). So for example, when we create a pending review, there is a reviewId that is required for commenting. Within a single session, it would be easy to return the id in the textContent but this:

Leaks the API implementation (maybe not a big deal?)
Would require new sessions to request the Id in some way in order to provide it the next time

For now, I've decided with these very explicit tools (e.g. mvp_add_pull_request_review_comment_to_pending_review) to allow the model to be API implementation agnostic, and that the tool will internally do the lookup. For example, since we know there can only be one review for the user, knowing the owner, repo and pr number seems to be enough to look up the most recent review. The downside is that it's a bit more restrictive and requires more internal API lookups than an alternative which might be to expose a get_pending_review and allow the model to figure it out. If you asked me I would ship this and wait for feedback on when it sucks or should be enhanced later but I don't have much confidence in my intuition here.

Cheers.

Currently passes e2e tests:

➜  github-mcp-server git:(0ca07aa) ✗ GOMAXPROCS=1 GITHUB_MCP_SERVER_E2E_TOKEN=$(gh auth token) go test -v -count=1 --tags e2e ./e2e

=== RUN   TestGetMe
=== PAUSE TestGetMe
=== RUN   TestToolsets
=== PAUSE TestToolsets
=== RUN   TestTags
=== PAUSE TestTags
=== RUN   TestPullRequestReview
=== PAUSE TestPullRequestReview
=== CONT  TestGetMe
    e2e_test.go:49: Building Docker image for e2e tests...
    e2e_test.go:123: Starting Stdio MCP client...
--- PASS: TestGetMe (3.33s)
=== CONT  TestPullRequestReview
    e2e_test.go:123: Starting Stdio MCP client...
    e2e_test.go:384: Getting current user...
    e2e_test.go:413: Creating repository williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:437: Creating branch in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:454: Creating commit with new file in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:482: Creating pull request in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:497: Creating pending review for pull request in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:518: Adding review comment to pull request in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:534: Submitting review for pull request in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:548: Getting reviews for pull request in williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
    e2e_test.go:422: Deleting repository williammartin/github-mcp-server-e2e-TestPullRequestReview-1746631692347...
--- PASS: TestPullRequestReview (9.26s)
=== CONT  TestTags
    e2e_test.go:123: Starting Stdio MCP client...
    e2e_test.go:246: Getting current user...
    e2e_test.go:275: Creating repository williammartin/github-mcp-server-e2e-TestTags-1746631701637...
    e2e_test.go:292: Creating tag williammartin/github-mcp-server-e2e-TestTags-1746631701637:v0.0.1...
    e2e_test.go:322: Listing tags for williammartin/github-mcp-server-e2e-TestTags-1746631701637...
    e2e_test.go:355: Getting tag williammartin/github-mcp-server-e2e-TestTags-1746631701637:v0.0.1...
    e2e_test.go:284: Deleting repository williammartin/github-mcp-server-e2e-TestTags-1746631701637...
--- PASS: TestTags (3.75s)
=== CONT  TestToolsets
    e2e_test.go:123: Starting Stdio MCP client...
--- PASS: TestToolsets (0.17s)
PASS
ok      github.com/github/github-mcp-server/e2e 16.767s

toby · 2025-05-07T17:03:58Z

Thank you for this @williammartin! I would say that yes, this is worth shipping and getting feedback from. It's better to be a bit more limited (and have a more complex backend api dance) than to break on launch because of JSON schema conflicts imho.

williammartin · 2025-05-07T17:54:02Z

Ok thanks, I'll move it forward to a place where I think it's acceptable and then ask for any final comments.

SamMorrowDrums · 2025-05-07T22:40:03Z

I think the direction sounds great. I'd love to know how well models navigate it.

We could have simple approve_pr, reject_pr tools that only do top level comment, when a full review is not required but I expect the would cause more confusion not less.

I can't wait to try this out.

SamMorrowDrums · 2025-05-08T02:30:39Z

@williammartin I think we need a cancel draft pull request review too, so there's a way to get past the fact you can only have one at a time issue.

williammartin

This is a test review of a tool in this PR itself.

williammartin · 2025-05-09T14:58:28Z

@toby @SamMorrowDrums please play around with the changes in this PR. The delete review tool has been added. I haven't written unit tests yet, but you should look at the e2e tests to understand the flow.

williammartin · 2025-05-09T14:58:59Z

internal/ghmcp/server.go

@@ -87,11 +89,21 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
 		return ghClient, nil // closing over client
 	}

+	getGQLClient := func(_ context.Context) (*githubv4.Client, error) {


TODO: Make enterprise ready.

And TODO add headers to these requests somehow too if possible, for same source identification purposes.

williammartin · 2025-05-09T15:00:17Z

pkg/github/server.go

+
+type constrainableInt32 int32
+
+func (ci *constrainableInt32) Constrain(param any) error {


Ok, I'm not sure if this is way overengineered or not, but it facilitates constraining param values: https://github.com/github/github-mcp-server/pull/381/files#diff-c2e2a7250ef597d08ee5d429159bc89c4b862a7ed6cbcf30b8231a3ec599d190R1081

I will get an error calling this as a lib right? Because the type and interface are not exported so I cannot provide them.

I don't need to call it I suppose, just a thought that I would need to do that if I need to add similar functionality.

To your question, I think it makes sense - we want to avoid integer wrapping, and sanitising user input is generally a good thing. It is a little heavy handed perhaps, but I expect we will not touch it often. I've not had to touch any of these functions much, they largely just do what you'd expect, so I don't mind it - it's not a huge footprint.

I suppose one comment might be, why not allow the caller to pass the min and max, in case they wish to constrain a value futher? Then indeed you could lose the interface, and have a generic function with numeric argument with a min and max check based on user provided value?

🤔 hard to decide. I do get the desire for extending the type directly.

I'm just going to remove it in favour of ParseInt32. It's simpler, and if we later discover we want this behaviour, we have evidence for it.

I have some ideas around parsing all args into structs that I think would be nice to explore first and are more general as a solution.

Thanks for the food for thought!

williammartin · 2025-05-09T15:00:43Z

pkg/github/pullrequests.go

@@ -75,8 +76,123 @@ func GetPullRequest(getClient GetClientFn, t translations.TranslationHelperFunc)
 		}
 }

+// CreatePullRequest creates a tool to create a new pull request.
+func CreatePullRequest(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) {


This is just moved from below to a more sensible location in the file, putting the review tools together in one place.

williammartin · 2025-05-09T15:01:52Z

pkg/github/tools.go

@@ -65,10 +67,16 @@ func InitToolsets(passedToolsets []string, readOnly bool, getClient GetClientFn,
 		AddWriteTools(
 			toolsets.NewServerTool(MergePullRequest(getClient, t)),
 			toolsets.NewServerTool(UpdatePullRequestBranch(getClient, t)),
-			toolsets.NewServerTool(CreatePullRequestReview(getClient, t)),
 			toolsets.NewServerTool(CreatePullRequest(getClient, t)),
 			toolsets.NewServerTool(UpdatePullRequest(getClient, t)),
 			toolsets.NewServerTool(AddPullRequestReviewComment(getClient, t)),


TODO: decide what to do with this standalone tool. I think it can just be removed in favour of create pending + add comment.

I think so, the one-shot version will get confusing otherwise.

williammartin · 2025-05-09T15:05:19Z

pkg/github/pullrequests.go

+			mcp.WithString("subjectType",
 				mcp.Required(),
-				mcp.Description("Branch to merge into"),
+				mcp.Description("The level at which the comment is targeted"),
+				mcp.Enum("FILE", "LINE"),
 			),
-			mcp.WithBoolean("draft",
-				mcp.Description("Create as draft PR"),
+			mcp.WithNumber("line",
+				mcp.Description("The line of the blob in the pull request diff that the comment applies to. For multi-line comments, the last line of the range"),
 			),
-			mcp.WithBoolean("maintainer_can_modify",
-				mcp.Description("Allow maintainer edits"),
+			mcp.WithString("side",
+				mcp.Description("The side of the diff to comment on"),
+				mcp.Enum("LEFT", "RIGHT"),
+			),
+			mcp.WithNumber("startLine",
+				mcp.Description("For multi-line comments, the first line of the range that the comment applies to"),
+			),
+			mcp.WithString("startSide",
+				mcp.Description("For multi-line comments, the starting side of the diff that the comment applies to"),
+				mcp.Enum("LEFT", "RIGHT"),


It's not really clear to me whether this actually works with Gemini, but it matches the other existing Tool AddPullRequestReviewComment that doesn't seem to cause gemini to barf.

williammartin

this is a test from gemini model

williammartin

this is a test from gemini model

williammartin

this is a test from gemini model

williammartin

this is a test from gemini model

williammartin · 2025-05-09T15:28:52Z

Using gemini 2.5 Pro (Preview) in VS Code is basically unusable due to 500s (where the backend seems to be rate-limited). However, on the rare chance I got it to work I saw:

Comparatively, on main all I ever see is:

I tried this multiple times so my assumption is that it relates to the schema.

williammartin · 2025-05-09T15:40:10Z

pkg/github/pullrequests.go

+			line, err := OptionalParam[constrainableInt32](request, "line")
 			if err != nil {
 				return mcp.NewToolResultError(err.Error()), nil
 			}

-			maintainerCanModify, err := OptionalParam[bool](request, "maintainer_can_modify")
+			side, err := OptionalParam[string](request, "side")
 			if err != nil {
 				return mcp.NewToolResultError(err.Error()), nil
 			}

-			newPR := &github.NewPullRequest{
-				Title: github.Ptr(title),
-				Head:  github.Ptr(head),
-				Base:  github.Ptr(base),
+			startLine, err := OptionalParam[constrainableInt32](request, "startLine")
+			if err != nil {
+				return mcp.NewToolResultError(err.Error()), nil
 			}

-			if body != "" {
-				newPR.Body = github.Ptr(body)
+			startSide, err := OptionalParam[string](request, "startSide")
+			if err != nil {
+				return mcp.NewToolResultError(err.Error()), nil
 			}

-			newPR.Draft = github.Ptr(draft)
-			newPR.MaintainerCanModify = github.Ptr(maintainerCanModify)
+			client, err := getGQLClient(ctx)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get GitHub GQL client: %w", err)
+			}


TODO: A bit more e2e validation (and probably a manual check) of these working as expected

williammartin · 2025-05-12T14:21:59Z

pkg/github/tools.go

+			toolsets.NewServerTool(CreateAndSubmitPullRequestReview(getGQLClient, t)),
+			toolsets.NewServerTool(CreatePendingPullRequestReview(getGQLClient, t)),
+			toolsets.NewServerTool(AddPullRequestReviewCommentToPendingReview(getGQLClient, t)),
+			toolsets.NewServerTool(SubmitPendingPullRequestReview(getGQLClient, t)),
+			toolsets.NewServerTool(DeletePendingPullRequestReview(getGQLClient, t)),


TODO: update README with new tools.

SamMorrowDrums

Pretty big as anticipated, but in general I like it a lot. I had a play with descriptions. I will clone it down and try them out too.

SamMorrowDrums · 2025-05-13T14:33:05Z

internal/ghmcp/server.go

@@ -87,11 +89,21 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) {
 		return ghClient, nil // closing over client
 	}

+	getGQLClient := func(_ context.Context) (*githubv4.Client, error) {


And TODO add headers to these requests somehow too if possible, for same source identification purposes.

SamMorrowDrums · 2025-05-13T14:40:05Z

pkg/github/server.go

+
+type constrainableInt32 int32
+
+func (ci *constrainableInt32) Constrain(param any) error {


I will get an error calling this as a lib right? Because the type and interface are not exported so I cannot provide them.

I don't need to call it I suppose, just a thought that I would need to do that if I need to add similar functionality.

To your question, I think it makes sense - we want to avoid integer wrapping, and sanitising user input is generally a good thing. It is a little heavy handed perhaps, but I expect we will not touch it often. I've not had to touch any of these functions much, they largely just do what you'd expect, so I don't mind it - it's not a huge footprint.

SamMorrowDrums · 2025-05-13T14:40:51Z

pkg/github/server.go

+
+type constrainableInt32 int32
+
+func (ci *constrainableInt32) Constrain(param any) error {


I suppose one comment might be, why not allow the caller to pass the min and max, in case they wish to constrain a value futher? Then indeed you could lose the interface, and have a generic function with numeric argument with a min and max check based on user provided value?

🤔 hard to decide. I do get the desire for extending the type directly.

pkg/github/server.go

SamMorrowDrums · 2025-05-13T14:47:03Z

pkg/github/tools.go

@@ -65,10 +67,16 @@ func InitToolsets(passedToolsets []string, readOnly bool, getClient GetClientFn,
 		AddWriteTools(
 			toolsets.NewServerTool(MergePullRequest(getClient, t)),
 			toolsets.NewServerTool(UpdatePullRequestBranch(getClient, t)),
-			toolsets.NewServerTool(CreatePullRequestReview(getClient, t)),
 			toolsets.NewServerTool(CreatePullRequest(getClient, t)),
 			toolsets.NewServerTool(UpdatePullRequest(getClient, t)),
 			toolsets.NewServerTool(AddPullRequestReviewComment(getClient, t)),


I think so, the one-shot version will get confusing otherwise.

pkg/github/pullrequests.go

SamMorrowDrums

This is a test of the PR review feature. I've reviewed the mcp.WithDescription blocks in the pullrequests.go file and suggested improvements to make them more informative for LLMs by:

Adding more context about what the tools actually return
Using more descriptive language about the functionality
Including clear explanations of parameters and their effects
Highlighting relationships between related operations

These improvements will help LLMs better understand the purpose and capabilities of each GitHub tool when generating responses.

williammartin linked an issue May 7, 2025 that may be closed by this pull request

Remove comments from create_pull_request_review #343

Open

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch from 8ef3228 to 85e18a9 Compare May 7, 2025 15:56

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch 2 times, most recently from 9419821 to a233900 Compare May 9, 2025 14:53

williammartin commented May 9, 2025

View reviewed changes

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch from a233900 to f345fcb Compare May 9, 2025 15:01

williammartin commented May 9, 2025

View reviewed changes

williammartin changed the title ~~Split PR review creation, commenting and submission~~ Split PR review creation, commenting and submission, and deletion May 9, 2025

williammartin commented May 9, 2025

View reviewed changes

williammartin commented May 12, 2025

View reviewed changes

SamMorrowDrums reviewed May 13, 2025

View reviewed changes

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch 2 times, most recently from 283c92d to e76b575 Compare May 13, 2025 15:45

Split PR review creation, commenting, submission and deletion

192a38a

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch from e76b575 to 192a38a Compare May 13, 2025 15:47

SamMorrowDrums mentioned this pull request May 14, 2025

Summary of Recent Issues (May 2025) #402

Closed

WIP

f0c9d52

williammartin force-pushed the 343-remove-comments-from-create_pull_request_review branch from d39cee2 to f0c9d52 Compare May 15, 2025 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split PR review creation, commenting and submission, and deletion #381

Split PR review creation, commenting and submission, and deletion #381

williammartin commented May 7, 2025 •

edited

Loading

toby commented May 7, 2025

williammartin commented May 7, 2025

SamMorrowDrums commented May 7, 2025

SamMorrowDrums commented May 8, 2025

williammartin left a comment

williammartin commented May 9, 2025

williammartin May 9, 2025

SamMorrowDrums May 13, 2025

williammartin May 9, 2025

SamMorrowDrums May 13, 2025

SamMorrowDrums May 13, 2025

williammartin May 13, 2025 •

edited

Loading

williammartin May 9, 2025

williammartin May 9, 2025 •

edited

Loading

SamMorrowDrums May 13, 2025

williammartin May 9, 2025

williammartin left a comment

williammartin left a comment

williammartin left a comment

williammartin left a comment

williammartin commented May 9, 2025

williammartin May 9, 2025

williammartin May 12, 2025

SamMorrowDrums left a comment

SamMorrowDrums May 13, 2025

SamMorrowDrums May 13, 2025

SamMorrowDrums May 13, 2025

SamMorrowDrums May 13, 2025

SamMorrowDrums left a comment


		type constrainableInt32 int32

		func (ci *constrainableInt32) Constrain(param any) error {

Split PR review creation, commenting and submission, and deletion #381

Are you sure you want to change the base?

Split PR review creation, commenting and submission, and deletion #381

Conversation

williammartin commented May 7, 2025 • edited Loading

Description

toby commented May 7, 2025

williammartin commented May 7, 2025

SamMorrowDrums commented May 7, 2025

SamMorrowDrums commented May 8, 2025

williammartin left a comment

Choose a reason for hiding this comment

williammartin commented May 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin May 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin left a comment

Choose a reason for hiding this comment

williammartin left a comment

Choose a reason for hiding this comment

williammartin left a comment

Choose a reason for hiding this comment

williammartin left a comment

Choose a reason for hiding this comment

williammartin commented May 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamMorrowDrums left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamMorrowDrums left a comment

Choose a reason for hiding this comment

williammartin commented May 7, 2025 •

edited

Loading

williammartin May 13, 2025 •

edited

Loading

williammartin May 9, 2025 •

edited

Loading