Split PR review creation, commenting, submission and deletion #410
Conversation
f15b891 to
3d58333
Compare
| @@ -0,0 +1,235 @@ | |||
| // githubv4mock package provides a mock GraphQL server used for testing queries produced via | |||
There was a problem hiding this comment.
This whole package should have its own tests but I want to get feedback on it before the extra effort.
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) | ||
| }), | ||
| name: "failure to get pull request", |
There was a problem hiding this comment.
I didn't do these GQL error cases in all the other tests intentionally yet, I just wanted to get a feel for whether it made sense. Every handler follows the same pattern on if err := ... ; err != nil
| @@ -157,7 +164,7 @@ func Test_OptionalStringParam(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func Test_RequiredNumberParam(t *testing.T) { | |||
| func Test_RequiredInt(t *testing.T) { | |||
There was a problem hiding this comment.
These names just didn't match the actual functions.
e2e/e2e_test.go
Outdated
There was a problem hiding this comment.
I'm starting to feel the tension in the repetition in this file now, which is a good thing because it means we probably have sufficient examples to start teasing out shared abstractions, asserting the MCP result isn't an error and has text content etc etc.
Would prefer not to address in this PR since it's about behaviour rather than maintenance.
| // Construct our REST client | ||
| restClient := gogithub.NewClient(nil).WithAuthToken(cfg.Token) | ||
| restClient.UserAgent = fmt.Sprintf("github-mcp-server/%s", cfg.Version) | ||
| restClient.BaseURL = apiHost.baseRESTURL |
There was a problem hiding this comment.
I chose to go away from WithEnterpriseURLS because there are hosts that are not enterprise but require different URLs e.g. review labs or development environments.
Sadly, githubv4 doesn't offer quite the same options, so I commented below why that is. It also doesn't export the internal graphql.Client so we can't just set it.
There was a problem hiding this comment.
That's a shame! Might need to propose exporting it.
internal/ghmcp/server.go
Outdated
| return apiHost{}, fmt.Errorf("could not parse host as URL: %s", s) | ||
| } | ||
|
|
||
| if url.Scheme == "" { |
There was a problem hiding this comment.
I'd be happy to loosen this later, but it is currently the expectation of the REST client.
3d58333 to
249064b
Compare
249064b to
1c1082e
Compare
| @@ -75,8 +77,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) { | |||
There was a problem hiding this comment.
This was just moved to a more natural place in the file.
1c1082e to
6273564
Compare
| var getLatestReviewForViewerQuery struct { | ||
| Repository struct { | ||
| PullRequest struct { | ||
| Reviews struct { | ||
| Nodes []struct { | ||
| ID githubv4.ID | ||
| State githubv4.PullRequestReviewState | ||
| URL githubv4.URI | ||
| } | ||
| } `graphql:"reviews(first: 1, author: $author)"` | ||
| } `graphql:"pullRequest(number: $prNum)"` | ||
| } `graphql:"repository(owner: $owner, name: $name)"` |
There was a problem hiding this comment.
Could we DRY this out?
There was a problem hiding this comment.
We could because a number of tools share it, but I'm keen to let it sit and soak in for a bit before choosing the wrong abstraction.
There was a problem hiding this comment.
Author note: this came from GPT-4.1
| var getLatestReviewForViewerQuery struct { | ||
| Repository struct { | ||
| PullRequest struct { | ||
| Reviews struct { | ||
| Nodes []struct { | ||
| ID githubv4.ID | ||
| State githubv4.PullRequestReviewState | ||
| URL githubv4.URI | ||
| } | ||
| } `graphql:"reviews(first: 1, author: $author)"` | ||
| } `graphql:"pullRequest(number: $prNum)"` | ||
| } `graphql:"repository(owner: $owner, name: $name)"` |
There was a problem hiding this comment.
Could we DRY this out?
There was a problem hiding this comment.
Author note: this came from Gemini 2.5
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the PR review workflow by splitting existing REST-based review/comment tools into finer-grained GraphQL-backed operations, adds diff retrieval support, and introduces host-specific URL parsing for GitHub API endpoints.
- Extracts separate GraphQL tools: create pending reviews, add comments to pending reviews, submit/delete pending reviews, and atomic create-and-submit.
- Adds
get_pull_request_diffand end-to-end mocks, including a local GraphQL mock server for testing. - Introduces
parseAPIHostand custom transports to support dotcom, GHEC, and GHES URL patterns.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third-party/github.com/shurcooL/graphql/LICENSE | Added MIT license file for the shurcooL/graphql dependency |
| third-party/github.com/shurcooL/githubv4/LICENSE | Added MIT license file for the shurcooL/githubv4 dependency |
| third-party-licenses.*.md | Registered new GraphQL libraries in platform-specific license docs |
| pkg/github/tools.go | Updated InitToolsets signature to accept a GraphQL client and registered new review tools |
| pkg/github/server_test.go | Added stubGetGQLClientFn; renamed tests for integer params |
| pkg/github/helper_test.go | Enhanced mockResponse to handle raw string bodies |
| internal/githubv4mock/* | Introduced a local GraphQL mock server, query builder, and equality checks for tests |
| internal/ghmcp/server.go | Added parseAPIHost, custom HTTP transports, and GraphQL client setup |
| go.mod | Bumped and added githubv4, graphql, and oauth2 dependencies |
Comments suppressed due to low confidence (5)
pkg/github/server_test.go:167
- [nitpick] The test name
Test_RequiredIntis inconsistent withTest_OptionalIntParam. Consider renaming toTest_RequiredIntParamto match the naming convention.
func Test_RequiredInt(t *testing.T) {
internal/githubv4mock/query.go:1
- Typo in comment:
Thsshould beThe.
// Ths contents of this file are taken from https://github.com/shurcooL/graphql/blob/ed46e5a4646634fc16cb07c3b8db389542cc8847/query.go
internal/ghmcp/server.go:335
- There are no tests covering
parseAPIHost. Adding unit tests for dotcom, GHEC, and GHES cases would help ensure correctness.
func parseAPIHost(s string) (apiHost, error) {
third-party-licenses.windows.md:20
- The indentation of this new list item does not align with existing entries (two spaces before the dash). Please adjust to match the surrounding style.
- [github.com/shurcooL/githubv4](https://pkg.go.dev/github.com/shurcooL/githubv4) ([MIT](https://github.com/shurcooL/githubv4/blob/48295856cce7/LICENSE))
third-party-licenses.linux.md:20
- This list item’s indentation differs from the rest of the file; align the dash under the others (two spaces) for consistency.
- [github.com/shurcooL/graphql](https://pkg.go.dev/github.com/shurcooL/graphql) ([MIT](https://github.com/shurcooL/graphql/blob/ed46e5a46466/LICENSE))
6273564 to
1089f84
Compare
| // Ths contents of this file are taken from https://github.com/shurcooL/graphql/blob/ed46e5a4646634fc16cb07c3b8db389542cc8847/query.go | ||
| // because they are not exported by the module, and we would like to use them in building the githubv4mock test utility. | ||
| // |
There was a problem hiding this comment.
I feel like eventually we might want a mock library to exist in standalone form, so we could pull this out, but it's OK to coexist here for now
| return mcp.NewTool("create_pull_request", | ||
| mcp.WithDescription(t("TOOL_CREATE_PULL_REQUEST_DESCRIPTION", "Create a new pull request in a GitHub repository.")), | ||
| mcp.WithToolAnnotation(mcp.ToolAnnotation{ | ||
| Title: t("TOOL_CREATE_PULL_REQUEST_USER_TITLE", "Open new pull request"), | ||
| ReadOnlyHint: toBoolPtr(false), | ||
| }), | ||
| mcp.WithString("owner", | ||
| mcp.Required(), | ||
| mcp.Description("Repository owner"), | ||
| ), | ||
| mcp.WithString("repo", | ||
| mcp.Required(), | ||
| mcp.Description("Repository name"), | ||
| ), | ||
| mcp.WithString("title", | ||
| mcp.Required(), | ||
| mcp.Description("PR title"), | ||
| ), | ||
| mcp.WithString("body", | ||
| mcp.Description("PR description"), | ||
| ), | ||
| mcp.WithString("head", | ||
| mcp.Required(), | ||
| mcp.Description("Branch containing changes"), | ||
| ), | ||
| mcp.WithString("base", | ||
| mcp.Required(), | ||
| mcp.Description("Branch to merge into"), | ||
| ), | ||
| mcp.WithBoolean("draft", | ||
| mcp.Description("Create as draft PR"), | ||
| ), | ||
| mcp.WithBoolean("maintainer_can_modify", | ||
| mcp.Description("Allow maintainer edits"), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
I hope the GO SDK makes these definitions and parsing much closer to standard JSON parsing into structs as discussed!
1089f84 to
1de146b
Compare
|
@SamMorrowDrums @williammartin Don't you think that removed |
Description
Fixes #343
This is a very large PR as a result of needing to:
Previously, PR review submission and commenting was provided by:
add_pull_request_review_commentcreate_pull_request_reviewHowever, Gemini experienced issues with the schema for comments on
create_pull_request_review. The original desire was to remove comments from that schema and require users to useadd_pull_request_review_commentbut as it turns out, this tool isn't that useful because it was backed by the REST API which only allows the addition of a single comment and then submits the review.As a result, this PR has introduced a number of new tools backed by the GraphQL API:
create_pending_pull_request_reviewadd_pull_request_review_comment_to_pending_reviewsubmit_pending_pull_request_reviewA tool was added to create and submit a review without comments:
create_and_submit_pull_request_reviewA tool was added to clean up pending reviews which were previously not a valid state:
delete_pending_pull_request_reviewFinally, in order to help the LLM determine line numbers for pull requests we added:
get_pull_request_diffIt's important to note that these tools are focused on working with a review that the caller owns. It does not allow adding arbitrary comments to review threads.
Example:
Created: https://github.com/github/github-mcp-server/pull/410/files#r2095483869
This worked with both GPT-4.1 and Gemini.
E2E Tests
github.com
GHES
GHEC