From ad2ce64725941a8ea563a92620fe6e8dd8afb07a Mon Sep 17 00:00:00 2001 From: Elliott Fouts Date: Thu, 22 May 2025 01:59:32 -0400 Subject: [PATCH 1/3] feat: Add mark_pr_ready_for_review tool This commit introduces a new tool that allows changing a GitHub pull request from draft state to ready for review. The implementation uses GitHub's GraphQL API with the 'markPullRequestReadyForReview' mutation, as the REST API doesn't support this state transition for existing draft PRs. Also fixes the GraphQL query by correcting 'Draft' to 'IsDraft' field name, ensuring proper detection of a PR's draft status. The changes include: - New tool implementation in the pull_requests toolset - Comprehensive tests - GraphQL query field correction --- pkg/github/pullrequests.go | 84 ++++++++++++ pkg/github/pullrequests_test.go | 225 ++++++++++++++++++++++++++++++++ pkg/github/tools.go | 1 + 3 files changed, 310 insertions(+) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index d47ab696..a557659e 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1607,3 +1607,87 @@ func newGQLIntPtr(i *int32) *githubv4.Int { gi := githubv4.Int(*i) return &gi } + +// MarkPullRequestReadyForReview creates a tool to mark a draft pull request as ready for review. +// This uses the GraphQL API because the REST API does not support changing a PR from draft to ready-for-review. +func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { + return mcp.NewTool("mark_pr_ready_for_review", + mcp.WithDescription(t("TOOL_MARK_PR_READY_FOR_REVIEW_DESCRIPTION", "Mark a draft pull request as ready for review. Use this to change a pull request from draft state to ready for review.")), + mcp.WithToolAnnotation(mcp.ToolAnnotation{ + Title: t("TOOL_MARK_PR_READY_FOR_REVIEW_USER_TITLE", "Mark pull request ready for review"), + ReadOnlyHint: toBoolPtr(false), + }), + mcp.WithString("owner", + mcp.Required(), + mcp.Description("Repository owner"), + ), + mcp.WithString("repo", + mcp.Required(), + mcp.Description("Repository name"), + ), + mcp.WithNumber("pullNumber", + mcp.Required(), + mcp.Description("Pull request number"), + ), + ), + func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var params struct { + Owner string + Repo string + PullNumber int32 + } + if err := mapstructure.Decode(request.Params.Arguments, ¶ms); err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + // Get the GraphQL client + client, err := getGQLClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub GraphQL client: %w", err) + } + + // First, we need to get the GraphQL ID of the pull request + var getPullRequestQuery struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + variables := map[string]any{ + "owner": githubv4.String(params.Owner), + "repo": githubv4.String(params.Repo), + "prNum": githubv4.Int(params.PullNumber), + } + + if err := client.Query(ctx, &getPullRequestQuery, variables); err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %v", err)), nil + } + + // Check if the PR is already in non-draft state + if !getPullRequestQuery.Repository.PullRequest.IsDraft { + return mcp.NewToolResultText("Pull request is already marked as ready for review"), nil + } + + // Now we can mark the PR as ready for review using the GraphQL mutation + var markReadyForReviewMutation struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID // We don't need this, but a selector is required or GQL complains + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + } + + input := githubv4.MarkPullRequestReadyForReviewInput{ + PullRequestID: getPullRequestQuery.Repository.PullRequest.ID, + } + + if err := client.Mutate(ctx, &markReadyForReviewMutation, input, nil); err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to mark pull request as ready for review: %v", err)), nil + } + + return mcp.NewToolResultText("Pull request successfully marked as ready for review"), nil + } +} diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index cdbccc28..ff346de4 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2291,3 +2291,228 @@ func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mo ), ) } + +func TestMarkPullRequestReadyForReview(t *testing.T) { + t.Parallel() + + // Verify tool definition once + mockClient := githubv4.NewClient(nil) + tool, _ := MarkPullRequestReadyForReview(stubGetGQLClientFn(mockClient), translations.NullTranslationHelper) + + assert.Equal(t, "mark_pr_ready_for_review", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "pullNumber") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"}) + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectToolError bool + expectedToolErrMsg string + prIsDraft bool + }{ + { + name: "successful mark ready for review", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": true, + }, + }, + }, + ), + ), + githubv4mock.NewMutationMatcher( + struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + }{}, + githubv4.MarkPullRequestReadyForReviewInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.DataResponse(map[string]any{}), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectToolError: false, + prIsDraft: true, + }, + { + name: "PR already ready for review", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": false, + }, + }, + }, + ), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectToolError: false, + prIsDraft: false, + }, + { + name: "failure to get pull request", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.ErrorResponse("expected test failure"), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectToolError: true, + expectedToolErrMsg: "failed to get pull request: expected test failure", + }, + { + name: "failure to mark ready for review", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": true, + }, + }, + }, + ), + ), + githubv4mock.NewMutationMatcher( + struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + }{}, + githubv4.MarkPullRequestReadyForReviewInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.ErrorResponse("expected test failure"), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectToolError: true, + expectedToolErrMsg: "failed to mark pull request as ready for review: expected test failure", + prIsDraft: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Setup client with mock + client := githubv4.NewClient(tc.mockedClient) + _, handler := MarkPullRequestReadyForReview(stubGetGQLClientFn(client), translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + require.NoError(t, err) + + textContent := getTextResult(t, result) + + if tc.expectToolError { + require.True(t, result.IsError) + assert.Contains(t, textContent.Text, tc.expectedToolErrMsg) + return + } + + // Check for the appropriate success message + if tc.prIsDraft { + require.Equal(t, "Pull request successfully marked as ready for review", textContent.Text) + } else { + require.Equal(t, "Pull request is already marked as ready for review", textContent.Text) + } + }) + } +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index ab052817..55632939 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -72,6 +72,7 @@ func InitToolsets(passedToolsets []string, readOnly bool, getClient GetClientFn, toolsets.NewServerTool(UpdatePullRequestBranch(getClient, t)), toolsets.NewServerTool(CreatePullRequest(getClient, t)), toolsets.NewServerTool(UpdatePullRequest(getClient, t)), + toolsets.NewServerTool(MarkPullRequestReadyForReview(getGQLClient, t)), toolsets.NewServerTool(RequestCopilotReview(getClient, t)), // Reviews From c9cda908d31cbf3522ec3f9d8459bf05faba71bf Mon Sep 17 00:00:00 2001 From: Elliott Fouts Date: Thu, 22 May 2025 02:30:03 -0400 Subject: [PATCH 2/3] refactor: address code style nitpicks in MarkPullRequestReadyForReview Based on code review feedback, made the following improvements: - Rename 'variables' to 'vars' for consistency with other GraphQL functions - Improve comment precision for GraphQL schema requirements These changes enhance code consistency and documentation clarity while maintaining full functionality. All tests, linting, and build checks pass. --- pkg/github/pullrequests.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index a557659e..a800b1b9 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1643,7 +1643,7 @@ func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.T // Get the GraphQL client client, err := getGQLClient(ctx) if err != nil { - return nil, fmt.Errorf("failed to get GitHub GraphQL client: %w", err) + return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GraphQL client: %v", err)), nil } // First, we need to get the GraphQL ID of the pull request @@ -1656,13 +1656,13 @@ func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.T } `graphql:"repository(owner: $owner, name: $repo)"` } - variables := map[string]any{ + vars := map[string]any{ "owner": githubv4.String(params.Owner), "repo": githubv4.String(params.Repo), "prNum": githubv4.Int(params.PullNumber), } - if err := client.Query(ctx, &getPullRequestQuery, variables); err != nil { + if err := client.Query(ctx, &getPullRequestQuery, vars); err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %v", err)), nil } @@ -1675,7 +1675,7 @@ func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.T var markReadyForReviewMutation struct { MarkPullRequestReadyForReview struct { PullRequest struct { - ID githubv4.ID // We don't need this, but a selector is required or GQL complains + ID githubv4.ID // Required by GraphQL schema, but not used in response } } `graphql:"markPullRequestReadyForReview(input: $input)"` } From fcbc2566db3e56fb6d684237c8ec058dd9fb73f1 Mon Sep 17 00:00:00 2001 From: Elliott Fouts Date: Fri, 23 May 2025 12:11:34 -0400 Subject: [PATCH 3/3] feat: convert mark_pr_ready_for_review to set_pr_status tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace mark_pr_ready_for_review with set_pr_status tool - Add bidirectional PR status changes (draft ↔ ready_for_review) - Use enum parameter for status: "draft", "ready_for_review" - Implement GraphQL mutations for both directions - Add comprehensive test suite with 8 test scenarios - Remove deprecated MarkPullRequestReadyForReview function Addresses user feedback to provide enum-based PR status management with support for setting PRs to both draft and ready-for-review states. --- pkg/github/pullrequests.go | 90 ++++++++++---- pkg/github/pullrequests_test.go | 207 ++++++++++++++++++++++++++++---- pkg/github/tools.go | 2 +- 3 files changed, 250 insertions(+), 49 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index a800b1b9..e9e2ed50 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1608,13 +1608,13 @@ func newGQLIntPtr(i *int32) *githubv4.Int { return &gi } -// MarkPullRequestReadyForReview creates a tool to mark a draft pull request as ready for review. -// This uses the GraphQL API because the REST API does not support changing a PR from draft to ready-for-review. -func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { - return mcp.NewTool("mark_pr_ready_for_review", - mcp.WithDescription(t("TOOL_MARK_PR_READY_FOR_REVIEW_DESCRIPTION", "Mark a draft pull request as ready for review. Use this to change a pull request from draft state to ready for review.")), +// SetPRStatus creates a tool to set pull request status between draft and ready-for-review states. +// This uses the GraphQL API because the REST API does not support changing PR draft status. +func SetPRStatus(getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { + return mcp.NewTool("set_pr_status", + mcp.WithDescription(t("TOOL_SET_PR_STATUS_DESCRIPTION", "Set pull request status between draft and ready-for-review states. Use this to change a pull request from draft to ready-for-review or vice versa.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ - Title: t("TOOL_MARK_PR_READY_FOR_REVIEW_USER_TITLE", "Mark pull request ready for review"), + Title: t("TOOL_SET_PR_STATUS_USER_TITLE", "Set pull request status"), ReadOnlyHint: toBoolPtr(false), }), mcp.WithString("owner", @@ -1629,24 +1629,35 @@ func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.T mcp.Required(), mcp.Description("Pull request number"), ), + mcp.WithString("status", + mcp.Required(), + mcp.Description("Target status for the pull request"), + mcp.Enum("draft", "ready_for_review"), + ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { var params struct { Owner string Repo string PullNumber int32 + Status string } if err := mapstructure.Decode(request.Params.Arguments, ¶ms); err != nil { return mcp.NewToolResultError(err.Error()), nil } + // Validate status parameter + if params.Status != "draft" && params.Status != "ready_for_review" { + return mcp.NewToolResultError("status must be either 'draft' or 'ready_for_review'"), nil + } + // Get the GraphQL client client, err := getGQLClient(ctx) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub GraphQL client: %v", err)), nil } - // First, we need to get the GraphQL ID of the pull request + // First, we need to get the GraphQL ID of the pull request and its current status var getPullRequestQuery struct { Repository struct { PullRequest struct { @@ -1666,28 +1677,57 @@ func MarkPullRequestReadyForReview(getGQLClient GetGQLClientFn, t translations.T return mcp.NewToolResultError(fmt.Sprintf("failed to get pull request: %v", err)), nil } - // Check if the PR is already in non-draft state - if !getPullRequestQuery.Repository.PullRequest.IsDraft { - return mcp.NewToolResultText("Pull request is already marked as ready for review"), nil - } + currentIsDraft := bool(getPullRequestQuery.Repository.PullRequest.IsDraft) + targetIsDraft := params.Status == "draft" - // Now we can mark the PR as ready for review using the GraphQL mutation - var markReadyForReviewMutation struct { - MarkPullRequestReadyForReview struct { - PullRequest struct { - ID githubv4.ID // Required by GraphQL schema, but not used in response - } - } `graphql:"markPullRequestReadyForReview(input: $input)"` + // Check if the PR is already in the target state + if currentIsDraft == targetIsDraft { + if targetIsDraft { + return mcp.NewToolResultText("Pull request is already in draft state"), nil + } else { + return mcp.NewToolResultText("Pull request is already marked as ready for review"), nil + } } - input := githubv4.MarkPullRequestReadyForReviewInput{ - PullRequestID: getPullRequestQuery.Repository.PullRequest.ID, - } + // Perform the appropriate mutation based on target status + if targetIsDraft { + // Convert to draft + var convertToDraftMutation struct { + ConvertPullRequestToDraft struct { + PullRequest struct { + ID githubv4.ID // Required by GraphQL schema, but not used in response + } + } `graphql:"convertPullRequestToDraft(input: $input)"` + } - if err := client.Mutate(ctx, &markReadyForReviewMutation, input, nil); err != nil { - return mcp.NewToolResultError(fmt.Sprintf("failed to mark pull request as ready for review: %v", err)), nil - } + input := githubv4.ConvertPullRequestToDraftInput{ + PullRequestID: getPullRequestQuery.Repository.PullRequest.ID, + } + + if err := client.Mutate(ctx, &convertToDraftMutation, input, nil); err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to convert pull request to draft: %v", err)), nil + } - return mcp.NewToolResultText("Pull request successfully marked as ready for review"), nil + return mcp.NewToolResultText("Pull request successfully converted to draft"), nil + } else { + // Mark as ready for review + var markReadyForReviewMutation struct { + MarkPullRequestReadyForReview struct { + PullRequest struct { + ID githubv4.ID // Required by GraphQL schema, but not used in response + } + } `graphql:"markPullRequestReadyForReview(input: $input)"` + } + + input := githubv4.MarkPullRequestReadyForReviewInput{ + PullRequestID: getPullRequestQuery.Repository.PullRequest.ID, + } + + if err := client.Mutate(ctx, &markReadyForReviewMutation, input, nil); err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to mark pull request as ready for review: %v", err)), nil + } + + return mcp.NewToolResultText("Pull request successfully marked as ready for review"), nil + } } } diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index ff346de4..fb3b95dc 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2292,19 +2292,26 @@ func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mo ) } -func TestMarkPullRequestReadyForReview(t *testing.T) { +func TestSetPRStatus(t *testing.T) { t.Parallel() // Verify tool definition once mockClient := githubv4.NewClient(nil) - tool, _ := MarkPullRequestReadyForReview(stubGetGQLClientFn(mockClient), translations.NullTranslationHelper) + tool, _ := SetPRStatus(stubGetGQLClientFn(mockClient), translations.NullTranslationHelper) - assert.Equal(t, "mark_pr_ready_for_review", tool.Name) + assert.Equal(t, "set_pr_status", tool.Name) assert.NotEmpty(t, tool.Description) assert.Contains(t, tool.InputSchema.Properties, "owner") assert.Contains(t, tool.InputSchema.Properties, "repo") assert.Contains(t, tool.InputSchema.Properties, "pullNumber") - assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"}) + assert.Contains(t, tool.InputSchema.Properties, "status") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber", "status"}) + + // Verify enum validation for status parameter + statusProperty := tool.InputSchema.Properties["status"].(map[string]any) + assert.Contains(t, statusProperty, "enum") + enumValues := statusProperty["enum"].([]string) + assert.ElementsMatch(t, enumValues, []string{"draft", "ready_for_review"}) tests := []struct { name string @@ -2312,10 +2319,10 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { requestArgs map[string]any expectToolError bool expectedToolErrMsg string - prIsDraft bool + expectedMessage string }{ { - name: "successful mark ready for review", + name: "successful draft to ready conversion", mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { @@ -2361,12 +2368,13 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { "owner": "owner", "repo": "repo", "pullNumber": float64(42), + "status": "ready_for_review", }, expectToolError: false, - prIsDraft: true, + expectedMessage: "Pull request successfully marked as ready for review", }, { - name: "PR already ready for review", + name: "successful ready to draft conversion", mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { @@ -2393,17 +2401,32 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { }, ), ), + githubv4mock.NewMutationMatcher( + struct { + ConvertPullRequestToDraft struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"convertPullRequestToDraft(input: $input)"` + }{}, + githubv4.ConvertPullRequestToDraftInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.DataResponse(map[string]any{}), + ), ), requestArgs: map[string]any{ "owner": "owner", "repo": "repo", "pullNumber": float64(42), + "status": "draft", }, expectToolError: false, - prIsDraft: false, + expectedMessage: "Pull request successfully converted to draft", }, { - name: "failure to get pull request", + name: "no change - already draft", mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { @@ -2419,19 +2442,108 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { "repo": githubv4.String("repo"), "prNum": githubv4.Int(42), }, - githubv4mock.ErrorResponse("expected test failure"), + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": true, + }, + }, + }, + ), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "status": "draft", + }, + expectToolError: false, + expectedMessage: "Pull request is already in draft state", + }, + { + name: "no change - already ready", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": false, + }, + }, + }, + ), ), ), requestArgs: map[string]any{ "owner": "owner", "repo": "repo", "pullNumber": float64(42), + "status": "ready_for_review", + }, + expectToolError: false, + expectedMessage: "Pull request is already marked as ready for review", + }, + { + name: "invalid status enum", + mockedClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "status": "invalid_status", }, expectToolError: true, - expectedToolErrMsg: "failed to get pull request: expected test failure", + expectedToolErrMsg: "status must be either 'draft' or 'ready_for_review'", }, { - name: "failure to mark ready for review", + name: "GraphQL query failure", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(999), + }, + githubv4mock.ErrorResponse("pull request not found"), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(999), + "status": "ready_for_review", + }, + expectToolError: true, + expectedToolErrMsg: "failed to get pull request: pull request not found", + }, + { + name: "GraphQL mutation failure - mark ready", mockedClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { @@ -2470,17 +2582,69 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), }, nil, - githubv4mock.ErrorResponse("expected test failure"), + githubv4mock.ErrorResponse("insufficient permissions"), ), ), requestArgs: map[string]any{ "owner": "owner", "repo": "repo", "pullNumber": float64(42), + "status": "ready_for_review", }, expectToolError: true, - expectedToolErrMsg: "failed to mark pull request as ready for review: expected test failure", - prIsDraft: true, + expectedToolErrMsg: "failed to mark pull request as ready for review: insufficient permissions", + }, + { + name: "GraphQL mutation failure - convert to draft", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + PullRequest struct { + ID githubv4.ID + IsDraft githubv4.Boolean + } `graphql:"pullRequest(number: $prNum)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "prNum": githubv4.Int(42), + }, + githubv4mock.DataResponse( + map[string]any{ + "repository": map[string]any{ + "pullRequest": map[string]any{ + "id": "PR_kwDODKw3uc6WYN1T", + "isDraft": false, + }, + }, + }, + ), + ), + githubv4mock.NewMutationMatcher( + struct { + ConvertPullRequestToDraft struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"convertPullRequestToDraft(input: $input)"` + }{}, + githubv4.ConvertPullRequestToDraftInput{ + PullRequestID: githubv4.ID("PR_kwDODKw3uc6WYN1T"), + }, + nil, + githubv4mock.ErrorResponse("insufficient permissions"), + ), + ), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + "status": "draft", + }, + expectToolError: true, + expectedToolErrMsg: "failed to convert pull request to draft: insufficient permissions", }, } @@ -2490,7 +2654,7 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { // Setup client with mock client := githubv4.NewClient(tc.mockedClient) - _, handler := MarkPullRequestReadyForReview(stubGetGQLClientFn(client), translations.NullTranslationHelper) + _, handler := SetPRStatus(stubGetGQLClientFn(client), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) @@ -2507,12 +2671,9 @@ func TestMarkPullRequestReadyForReview(t *testing.T) { return } - // Check for the appropriate success message - if tc.prIsDraft { - require.Equal(t, "Pull request successfully marked as ready for review", textContent.Text) - } else { - require.Equal(t, "Pull request is already marked as ready for review", textContent.Text) - } + // Verify success message + require.False(t, result.IsError) + assert.Equal(t, tc.expectedMessage, textContent.Text) }) } } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 55632939..312a5edc 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -72,7 +72,7 @@ func InitToolsets(passedToolsets []string, readOnly bool, getClient GetClientFn, toolsets.NewServerTool(UpdatePullRequestBranch(getClient, t)), toolsets.NewServerTool(CreatePullRequest(getClient, t)), toolsets.NewServerTool(UpdatePullRequest(getClient, t)), - toolsets.NewServerTool(MarkPullRequestReadyForReview(getGQLClient, t)), + toolsets.NewServerTool(SetPRStatus(getGQLClient, t)), toolsets.NewServerTool(RequestCopilotReview(getClient, t)), // Reviews