Add updating draft state to update_pull_request tool
#774
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds draft state management capability to the update_pull_request tool, allowing users to convert pull requests between draft and ready-for-review states using the GitHub GraphQL API.
- Adds a new
draftparameter to the tool's input schema - Implements GraphQL mutations for converting between draft and ready-for-review states
- Updates tests and documentation to reflect the new functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/tools.go | Updates tool registration to include GraphQL client parameter |
| pkg/github/pullrequests.go | Implements draft state management logic using GraphQL mutations |
| pkg/github/pullrequests_test.go | Updates tests to include the new draft parameter and GraphQL client |
| pkg/github/toolsnaps/update_pull_request.snap | Adds draft parameter to tool schema snapshot |
| README.md | Documents the new draft parameter in the tool description |
| return mcp.NewToolResultError("No update parameters provided."), nil | ||
| } | ||
|
|
||
| if restUpdateNeeded { |
There was a problem hiding this comment.
[nitpick] The conditional block structure creates an unbalanced indentation flow. Consider extracting the REST API update logic into a separate function to improve readability and maintainability.
Note: See the diff below for a potential fix:
@@ -308,28 +308,10 @@
}
if restUpdateNeeded {
- client, err := getClient(ctx)
+ result, err := updatePullRequest(ctx, getClient, owner, repo, pullNumber, update)
if err != nil {
- return nil, fmt.Errorf("failed to get GitHub client: %w", err)
+ return result, err
}
-
- _, resp, err := client.PullRequests.Edit(ctx, owner, repo, pullNumber, update)
- if err != nil {
- return ghErrors.NewGitHubAPIErrorResponse(ctx,
- "failed to update pull request",
- resp,
- err,
- ), nil
- }
- defer func() { _ = resp.Body.Close() }()
-
- if resp.StatusCode != http.StatusOK {
- body, err := io.ReadAll(resp.Body)
- if err != nil {
- return nil, fmt.Errorf("failed to read response body: %w", err)
- }
- return mcp.NewToolResultError(fmt.Sprintf("failed to update pull request: %s", string(body))), nil
- }
}
if draftProvided {
Copilot uses AI. Check for mistakes.
| client, err := getClient(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
| return nil, err |
There was a problem hiding this comment.
The REST client is retrieved twice in this function - once for the conditional update and again at the end. Consider reusing the client instance or restructuring the code to avoid duplicate client retrieval.
| return nil, err | |
| finalPR, resp, err := client.REST.PullRequests.Get(ctx, owner, repo, pullNumber) | |
| if err != nil { | |
| return ghErrors.NewGitHubAPIErrorResponse(ctx, "Failed to get pull request", resp, err), nil |
Copilot uses AI. Check for mistakes.
Closes: #501 and #423
This is a copy of the comment in issue 501:
I've had a look at implementing this as an extension of the
update_pull_requesttool (using GraphQL). I've chosen to use a newdraftparameter as a boolean for this for a few reasons: