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.

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

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.

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 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

Split PR review creation, commenting, submission and deletion

f345fcb

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

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

williammartin May 9, 2025

williammartin May 9, 2025

williammartin May 9, 2025 •

edited

Loading

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


		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

williammartin May 9, 2025

Choose a reason for hiding this comment

williammartin May 9, 2025

Choose a reason for hiding this comment

williammartin May 9, 2025

Choose a reason for hiding this comment

williammartin May 9, 2025 • edited Loading

Choose a reason for hiding this comment

williammartin May 9, 2025

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

williammartin May 9, 2025

Choose a reason for hiding this comment

williammartin commented May 7, 2025 •

edited

Loading

williammartin May 9, 2025 •

edited

Loading