- Notifications
You must be signed in to change notification settings - Fork1k
Split PR review creation, commenting and submission, and deletion#381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Split PR review creation, commenting and submission, and deletion#381
Uh oh!
There was an error while loading.Please reload this page.
Conversation
8ef3228 to85e18a9CompareThank 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. |
Ok thanks, I'll move it forward to a place where I think it's acceptable and then ask for any final comments. |
I think the direction sounds great. I'd love to know how well models navigate it. We could have simple I can't wait to try this out. |
@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. |
9419821 toa233900CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a test review of a tool in this PR itself.
@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. |
| @@ -87,11 +89,21 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { | |||
| return ghClient, nil // closing over client | |||
| } | |||
| getGQLClient := func(_ context.Context) (*githubv4.Client, error) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TODO: Make enterprise ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And TODO add headers to these requests somehow too if possible, for same source identification purposes.
pkg/github/server.go Outdated
| type constrainableInt32 int32 | ||
| func (ci *constrainableInt32) Constrain(param any) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
williammartinMay 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm just going to remove it in favour ofParseInt32. 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!
| @@ -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) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is just moved from below to a more sensible location in the file, putting the review tools together in one place.
a233900 tof345fcbComparepkg/github/tools.go Outdated
| @@ -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)), | |||
williammartinMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TODO: decide what to do with this standalone tool. I think it can just be removed in favour of create pending + add comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think so, the one-shot version will get confusing otherwise.
pkg/github/pullrequests.go Outdated
| 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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's not really clear to me whether this actually works with Gemini, but it matches the other existing ToolAddPullRequestReviewComment that doesn't seem to cause gemini to barf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a test from gemini model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a test from gemini model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a test from gemini model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is a test from gemini model
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  I tried this multiple times so my assumption is that it relates to the schema. |
pkg/github/pullrequests.go Outdated
| 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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TODO: A bit more e2e validation (and probably a manual check) of these working as expected
| 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)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TODO: update README with new tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| @@ -87,11 +89,21 @@ func NewMCPServer(cfg MCPServerConfig) (*server.MCPServer, error) { | |||
| return ghClient, nil // closing over client | |||
| } | |||
| getGQLClient := func(_ context.Context) (*githubv4.Client, error) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And TODO add headers to these requests somehow too if possible, for same source identification purposes.
pkg/github/server.go Outdated
| type constrainableInt32 int32 | ||
| func (ci *constrainableInt32) Constrain(param any) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
pkg/github/server.go Outdated
| type constrainableInt32 int32 | ||
| func (ci *constrainableInt32) Constrain(param any) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
pkg/github/tools.go Outdated
| @@ -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)), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think so, the one-shot version will get confusing otherwise.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
283c92d toe76b575Comparee76b575 to192a38aCompared39cee2 tof0c9d52CompareThis has become pretty unwieldy as we were exploring the design space. I believe it is now ready to PR properly so I'm going to close this one in favour of a clean slate. |
Uh oh!
There was an error while loading.Please reload this page.
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
commentsfromcreate_pull_request_reviewand depending onadd_pull_request_review_comment, however,add_pull_request_review_commentuses 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:
mvp_create_pending_pull_request_review)mvp_add_pull_request_review_comment_to_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_idin the REST API). So for example, when we create a pending review, there is areviewIdthat is required for commenting. Within a single session, it would be easy to return theidin thetextContentbut this: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 theowner,repoandprnumber 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 aget_pending_reviewand 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: