From a7d2b3b55740b5a7d701da853a28dda03c1dc6c3 Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Wed, 30 Sep 2026 04:08:05 +0000 Subject: [PATCH] fix: find pending pull request reviews through REST Use the REST reviews endpoint to locate the caller-visible pending review and retain GraphQL for comment, submit, and delete mutations. This supports GitHub App installation tokens, for which viewerLatestReview excludes pending reviews. Add pagination coverage and update both consolidated and granular review tools without changing their schemas. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f5bfbcc5-06e4-4487-910f-6a2ca7b5bd7b --- pkg/github/granular_tools_test.go | 48 +----- pkg/github/pullrequests.go | 232 ++++++++----------------- pkg/github/pullrequests_granular.go | 18 +- pkg/github/pullrequests_test.go | 252 ++++++++++++---------------- 4 files changed, 193 insertions(+), 357 deletions(-) diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 425f954ef9..fe0928d5dd 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -1653,49 +1653,6 @@ func TestGranularUpdatePullRequestDraftState(t *testing.T) { func TestGranularAddPullRequestReviewComment(t *testing.T) { mockedClient := githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Login githubv4.String - } - }{}, - nil, - githubv4mock.DataResponse(map[string]any{ - "viewer": map[string]any{"login": "testuser"}, - }), - ), - githubv4mock.NewQueryMatcher( - 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)"` - }{}, - map[string]any{ - "author": githubv4.String("testuser"), - "owner": githubv4.String("owner"), - "name": githubv4.String("repo"), - "prNum": githubv4.Int(1), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "pullRequest": map[string]any{ - "reviews": map[string]any{ - "nodes": []map[string]any{ - {"id": "PRR_123", "state": "PENDING", "url": "https://github.com/owner/repo/pull/1#pullrequestreview-123"}, - }, - }, - }, - }, - }), - ), githubv4mock.NewMutationMatcher( struct { AddPullRequestReviewThread struct { @@ -1721,7 +1678,10 @@ func TestGranularAddPullRequestReviewComment(t *testing.T) { ), ) gqlClient := githubv4.NewClient(mockedClient) - deps := BaseDeps{GQLClient: gqlClient} + deps := BaseDeps{ + Client: mockPendingReviewClient(t, "PRR_123"), + GQLClient: gqlClient, + } serverTool := GranularAddPullRequestReviewComment(translations.NullTranslationHelper) handler := serverTool.Handler(deps) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 5cee8b3231..43518fbc59 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1876,31 +1876,38 @@ Available methods: return utils.NewToolResultError(err.Error()), nil, nil } - // Given our owner, repo and PR number, lookup the GQL ID of the PR. - client, err := deps.GetGQLClient(ctx) + gqlClient, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub GQL client: %v", err)), nil, nil } switch params.Method { case "create": - result, err := CreatePullRequestReview(ctx, client, params) + result, err := CreatePullRequestReview(ctx, gqlClient, params) return result, nil, err case "submit_pending": - result, err := SubmitPendingPullRequestReview(ctx, client, params) + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub client: %v", err)), nil, nil + } + result, err := SubmitPendingPullRequestReview(ctx, restClient, gqlClient, params) return result, nil, err case "delete_pending": - result, err := DeletePendingPullRequestReview(ctx, client, params) + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to get GitHub client: %v", err)), nil, nil + } + result, err := DeletePendingPullRequestReview(ctx, restClient, gqlClient, params) return result, nil, err case "resolve_thread": if !withResolutionReason { - result, err := ResolveReviewThread(ctx, client, params.ThreadID, true) + result, err := ResolveReviewThread(ctx, gqlClient, params.ThreadID, true) return result, nil, err } - result, err := ResolveReviewThreadWithReason(ctx, client, params.ThreadID, params.ResolutionReason, true) + result, err := ResolveReviewThreadWithReason(ctx, gqlClient, params.ThreadID, params.ResolutionReason, true) return result, nil, err case "unresolve_thread": - result, err := ResolveReviewThread(ctx, client, params.ThreadID, false) + result, err := ResolveReviewThread(ctx, gqlClient, params.ThreadID, false) return result, nil, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", params.Method)), nil, nil @@ -1988,58 +1995,43 @@ func CreatePullRequestReview(ctx context.Context, client *githubv4.Client, param return utils.NewToolResultText("pull request review submitted successfully"), nil } -func SubmitPendingPullRequestReview(ctx context.Context, client *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) { - // First we'll get the current user - var getViewerQuery struct { - Viewer struct { - Login githubv4.String +// GetPendingPullRequestReviewID returns the node ID of the viewer's pending review, +// or a tool error result otherwise. GitHub only includes pending reviews for their +// author in the REST response, including when the viewer is a GitHub App. +func GetPendingPullRequestReviewID(ctx context.Context, client *github.Client, owner, repo string, pullNumber int32) (githubv4.ID, *mcp.CallToolResult) { + opts := &github.ListOptions{PerPage: 100} + for { + reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, int(pullNumber), opts) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + if err != nil { + return nil, ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list pull request reviews", resp, err) } - } - - if err := client.Query(ctx, &getViewerQuery, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", - err, - ), nil - } - - 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)"` - } - vars := map[string]any{ - "author": githubv4.String(getViewerQuery.Viewer.Login), - "owner": githubv4.String(params.Owner), - "name": githubv4.String(params.Repo), - "prNum": githubv4.Int(params.PullNumber), - } + for _, review := range reviews { + if review.GetState() != "PENDING" { + continue + } + if review.GetNodeID() == "" { + return nil, utils.NewToolResultError("Pending review did not include a node ID") + } + return githubv4.ID(review.GetNodeID()), nil + } - if err := client.Query(ctx, &getLatestReviewForViewerQuery, vars); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", - err, - ), nil + if resp == nil || resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage } - // Validate there is one review and the state is pending - if len(getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes) == 0 { - return utils.NewToolResultError("No pending review found for the viewer"), nil - } + return nil, utils.NewToolResultError("No pending review found for the viewer") +} - review := getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes[0] - if review.State != githubv4.PullRequestReviewStatePending { - errText := fmt.Sprintf("The latest review, found at %s is not pending", review.URL) - return utils.NewToolResultError(errText), nil +func SubmitPendingPullRequestReview(ctx context.Context, restClient *github.Client, gqlClient *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) { + reviewID, errResult := GetPendingPullRequestReviewID(ctx, restClient, params.Owner, params.Repo, params.PullNumber) + if errResult != nil { + return errResult, nil } // Prepare the mutation @@ -2051,11 +2043,11 @@ func SubmitPendingPullRequestReview(ctx context.Context, client *githubv4.Client } `graphql:"submitPullRequestReview(input: $input)"` } - if err := client.Mutate( + if err := gqlClient.Mutate( ctx, &submitPullRequestReviewMutation, githubv4.SubmitPullRequestReviewInput{ - PullRequestReviewID: &review.ID, + PullRequestReviewID: &reviewID, Event: githubv4.PullRequestReviewEvent(params.Event), Body: newGQLStringlikePtr[githubv4.String](¶ms.Body), }, @@ -2073,58 +2065,10 @@ func SubmitPendingPullRequestReview(ctx context.Context, client *githubv4.Client return utils.NewToolResultText("pending pull request review successfully submitted"), nil } -func DeletePendingPullRequestReview(ctx context.Context, client *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) { - // First we'll get the current user - var getViewerQuery struct { - Viewer struct { - Login githubv4.String - } - } - - if err := client.Query(ctx, &getViewerQuery, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", - err, - ), nil - } - - 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)"` - } - - vars := map[string]any{ - "author": githubv4.String(getViewerQuery.Viewer.Login), - "owner": githubv4.String(params.Owner), - "name": githubv4.String(params.Repo), - "prNum": githubv4.Int(params.PullNumber), - } - - if err := client.Query(ctx, &getLatestReviewForViewerQuery, vars); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", - err, - ), nil - } - - // Validate there is one review and the state is pending - if len(getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes) == 0 { - return utils.NewToolResultError("No pending review found for the viewer"), nil - } - - review := getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes[0] - if review.State != githubv4.PullRequestReviewStatePending { - errText := fmt.Sprintf("The latest review, found at %s is not pending", review.URL) - return utils.NewToolResultError(errText), nil +func DeletePendingPullRequestReview(ctx context.Context, restClient *github.Client, gqlClient *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) { + reviewID, errResult := GetPendingPullRequestReviewID(ctx, restClient, params.Owner, params.Repo, params.PullNumber) + if errResult != nil { + return errResult, nil } // Prepare the mutation @@ -2136,11 +2080,11 @@ func DeletePendingPullRequestReview(ctx context.Context, client *githubv4.Client } `graphql:"deletePullRequestReview(input: $input)"` } - if err := client.Mutate( + if err := gqlClient.Mutate( ctx, &deletePullRequestReviewMutation, githubv4.DeletePullRequestReviewInput{ - PullRequestReviewID: &review.ID, + PullRequestReviewID: &reviewID, }, nil, ); err != nil { @@ -2234,58 +2178,10 @@ type AddCommentToPendingReviewParams struct { } // AddCommentToPendingReviewCall adds a review comment to the viewer's pending pull request review. -func AddCommentToPendingReviewCall(ctx context.Context, client *githubv4.Client, params AddCommentToPendingReviewParams) (*mcp.CallToolResult, error) { - // Get the current user - var getViewerQuery struct { - Viewer struct { - Login githubv4.String - } - } - - if err := client.Query(ctx, &getViewerQuery, nil); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get current user", - err, - ), nil - } - - 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)"` - } - - vars := map[string]any{ - "author": githubv4.String(getViewerQuery.Viewer.Login), - "owner": githubv4.String(params.Owner), - "name": githubv4.String(params.Repo), - "prNum": githubv4.Int(params.PullNumber), - } - - if err := client.Query(ctx, &getLatestReviewForViewerQuery, vars); err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, - "failed to get latest review for current user", - err, - ), nil - } - - // Validate there is one review and the state is pending - if len(getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes) == 0 { - return utils.NewToolResultError("No pending review found for the viewer"), nil - } - - review := getLatestReviewForViewerQuery.Repository.PullRequest.Reviews.Nodes[0] - if review.State != githubv4.PullRequestReviewStatePending { - errText := fmt.Sprintf("The latest review, found at %s is not pending", review.URL) - return utils.NewToolResultError(errText), nil +func AddCommentToPendingReviewCall(ctx context.Context, restClient *github.Client, gqlClient *githubv4.Client, params AddCommentToPendingReviewParams) (*mcp.CallToolResult, error) { + reviewID, errResult := GetPendingPullRequestReviewID(ctx, restClient, params.Owner, params.Repo, params.PullNumber) + if errResult != nil { + return errResult, nil } // Create a new review thread comment on the review. @@ -2297,7 +2193,7 @@ func AddCommentToPendingReviewCall(ctx context.Context, client *githubv4.Client, } `graphql:"addPullRequestReviewThread(input: $input)"` } - if err := client.Mutate( + if err := gqlClient.Mutate( ctx, &addPullRequestReviewThreadMutation, githubv4.AddPullRequestReviewThreadInput{ @@ -2308,7 +2204,7 @@ func AddCommentToPendingReviewCall(ctx context.Context, client *githubv4.Client, Side: newGQLStringlikePtr[githubv4.DiffSide](params.Side), StartLine: newGQLIntPtr(params.StartLine), StartSide: newGQLStringlikePtr[githubv4.DiffSide](params.StartSide), - PullRequestReviewID: &review.ID, + PullRequestReviewID: &reviewID, }, nil, ); err != nil { @@ -2435,10 +2331,14 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S } startSide, _ := OptionalParam[string](args, "startSide") - client, err := deps.GetGQLClient(ctx) + gqlClient, err := deps.GetGQLClient(ctx) if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub GQL client", err), nil, nil } + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } var linePtr, startLinePtr *int32 if line != 0 { @@ -2457,7 +2357,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S startSidePtr = &startSide } - result, err := AddCommentToPendingReviewCall(ctx, client, AddCommentToPendingReviewParams{ + result, err := AddCommentToPendingReviewCall(ctx, restClient, gqlClient, AddCommentToPendingReviewParams{ Owner: owner, Repo: repo, PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers diff --git a/pkg/github/pullrequests_granular.go b/pkg/github/pullrequests_granular.go index e670da9a34..f8b6653ea5 100644 --- a/pkg/github/pullrequests_granular.go +++ b/pkg/github/pullrequests_granular.go @@ -489,8 +489,12 @@ func GranularSubmitPendingPullRequestReview(t translations.TranslationHelperFunc if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil } + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } - result, err := SubmitPendingPullRequestReview(ctx, gqlClient, PullRequestReviewWriteParams{ + result, err := SubmitPendingPullRequestReview(ctx, restClient, gqlClient, PullRequestReviewWriteParams{ Owner: owner, Repo: repo, PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers @@ -546,8 +550,12 @@ func GranularDeletePendingPullRequestReview(t translations.TranslationHelperFunc if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil } + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } - result, err := DeletePendingPullRequestReview(ctx, gqlClient, PullRequestReviewWriteParams{ + result, err := DeletePendingPullRequestReview(ctx, restClient, gqlClient, PullRequestReviewWriteParams{ Owner: owner, Repo: repo, PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers @@ -630,6 +638,10 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub GraphQL client", err), nil, nil } + restClient, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } // Convert optional int params to *int32 for the helper var linePtr, startLinePtr *int32 @@ -651,7 +663,7 @@ func GranularAddPullRequestReviewComment(t translations.TranslationHelperFunc) i startSidePtr = &startSide } - result, err := AddCommentToPendingReviewCall(ctx, gqlClient, AddCommentToPendingReviewParams{ + result, err := AddCommentToPendingReviewCall(ctx, restClient, gqlClient, AddCommentToPendingReviewParams{ Owner: owner, Repo: repo, PullNumber: int32(pullNumber), // #nosec G115 - PR numbers are always small positive integers diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index c0e392aea6..84a6942a3d 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" "github.com/google/jsonschema-go/jsonschema" @@ -3681,21 +3682,6 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { "startSide": "RIGHT", }, mockedClient: githubv4mock.NewMockedHTTPClient( - viewerQuery("williammartin"), - getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ - author: "williammartin", - owner: "owner", - repo: "repo", - prNum: 42, - - reviews: []getLatestPendingReviewQueryReview{ - { - id: "PR_kwDODKw3uc6WYN1T", - state: "PENDING", - url: "https://github.com/owner/repo/pull/42", - }, - }, - }), githubv4mock.NewMutationMatcher( struct { AddPullRequestReviewThread struct { @@ -3740,21 +3726,6 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { "startSide": "RIGHT", }, mockedClient: githubv4mock.NewMockedHTTPClient( - viewerQuery("williammartin"), - getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ - author: "williammartin", - owner: "owner", - repo: "repo", - prNum: 42, - - reviews: []getLatestPendingReviewQueryReview{ - { - id: "PR_kwDODKw3uc6WYN1T", - state: "PENDING", - url: "https://github.com/owner/repo/pull/42", - }, - }, - }), githubv4mock.NewMutationMatcher( struct { AddPullRequestReviewThread struct { @@ -3821,21 +3792,6 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { "side": "RIGHT", }, mockedClient: githubv4mock.NewMockedHTTPClient( - viewerQuery("williammartin"), - getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ - author: "williammartin", - owner: "owner", - repo: "repo", - prNum: 42, - - reviews: []getLatestPendingReviewQueryReview{ - { - id: "PR_kwDODKw3uc6WYN1T", - state: "PENDING", - url: "https://github.com/owner/repo/pull/42", - }, - }, - }), githubv4mock.NewMutationMatcher( struct { AddPullRequestReviewThread struct { @@ -3877,6 +3833,7 @@ func TestAddPullRequestReviewCommentToPendingReview(t *testing.T) { client := githubv4.NewClient(tc.mockedClient) serverTool := AddCommentToPendingReview(translations.NullTranslationHelper) deps := BaseDeps{ + Client: mockPendingReviewClient(t, "PR_kwDODKw3uc6WYN1T"), GQLClient: client, } handler := serverTool.Handler(deps) @@ -3939,21 +3896,6 @@ func TestSubmitPendingPullRequestReview(t *testing.T) { "body": "This is a test review", }, mockedClient: githubv4mock.NewMockedHTTPClient( - viewerQuery("williammartin"), - getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ - author: "williammartin", - owner: "owner", - repo: "repo", - prNum: 42, - - reviews: []getLatestPendingReviewQueryReview{ - { - id: "PR_kwDODKw3uc6WYN1T", - state: "PENDING", - url: "https://github.com/owner/repo/pull/42", - }, - }, - }), githubv4mock.NewMutationMatcher( struct { SubmitPullRequestReview struct { @@ -3982,6 +3924,7 @@ func TestSubmitPendingPullRequestReview(t *testing.T) { client := githubv4.NewClient(tc.mockedClient) serverTool := PullRequestReviewWrite(translations.NullTranslationHelper) deps := BaseDeps{ + Client: mockPendingReviewClient(t, "PR_kwDODKw3uc6WYN1T"), GQLClient: client, } handler := serverTool.Handler(deps) @@ -4040,21 +3983,6 @@ func TestDeletePendingPullRequestReview(t *testing.T) { "pullNumber": float64(42), }, mockedClient: githubv4mock.NewMockedHTTPClient( - viewerQuery("williammartin"), - getLatestPendingReviewQuery(getLatestPendingReviewQueryParams{ - author: "williammartin", - owner: "owner", - repo: "repo", - prNum: 42, - - reviews: []getLatestPendingReviewQueryReview{ - { - id: "PR_kwDODKw3uc6WYN1T", - state: "PENDING", - url: "https://github.com/owner/repo/pull/42", - }, - }, - }), githubv4mock.NewMutationMatcher( struct { DeletePullRequestReview struct { @@ -4081,6 +4009,7 @@ func TestDeletePendingPullRequestReview(t *testing.T) { client := githubv4.NewClient(tc.mockedClient) serverTool := PullRequestReviewWrite(translations.NullTranslationHelper) deps := BaseDeps{ + Client: mockPendingReviewClient(t, "PR_kwDODKw3uc6WYN1T"), GQLClient: client, } handler := serverTool.Handler(deps) @@ -4106,6 +4035,102 @@ func TestDeletePendingPullRequestReview(t *testing.T) { } } +func TestPendingPullRequestReviewLookupErrors(t *testing.T) { + t.Parallel() + + tools := []struct { + name string + tool inventory.ServerTool + method string + }{ + {"comment", AddCommentToPendingReview(translations.NullTranslationHelper), ""}, + {"submit", PullRequestReviewWrite(translations.NullTranslationHelper), "submit_pending"}, + {"delete", PullRequestReviewWrite(translations.NullTranslationHelper), "delete_pending"}, + {"granular comment", GranularAddPullRequestReviewComment(translations.NullTranslationHelper), ""}, + {"granular submit", GranularSubmitPendingPullRequestReview(translations.NullTranslationHelper), ""}, + {"granular delete", GranularDeletePendingPullRequestReview(translations.NullTranslationHelper), ""}, + } + + tests := []struct { + name string + reviews []*github.PullRequestReview + status int + expectedError string + }{ + {name: "no review", status: http.StatusOK, expectedError: "No pending review found for the viewer"}, + { + name: "only submitted reviews", + status: http.StatusOK, + reviews: []*github.PullRequestReview{ + {NodeID: github.Ptr("PRR_123"), State: github.Ptr("COMMENTED")}, + }, + expectedError: "No pending review found for the viewer", + }, + { + name: "pending review without node ID", + status: http.StatusOK, + reviews: []*github.PullRequestReview{ + {State: github.Ptr("PENDING")}, + }, + expectedError: "Pending review did not include a node ID", + }, + {name: "REST failure", status: http.StatusInternalServerError, expectedError: "failed to list pull request reviews"}, + } + + for _, tool := range tools { + for _, tc := range tests { + t.Run(tool.name+"/"+tc.name, func(t *testing.T) { + t.Parallel() + + restClient, err := github.NewClient(github.WithHTTPClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, tc.status, tc.reviews), + }))) + require.NoError(t, err) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient()) + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + args := map[string]any{ + "owner": "owner", "repo": "repo", "pullNumber": float64(42), + "event": "COMMENT", "body": "Review comment", + "path": "file.go", "subjectType": "FILE", + } + if tool.method != "" { + args["method"] = tool.method + } + request := createMCPRequest(args) + result, err := tool.tool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedError) + }) + } + } +} + +func TestGetPendingPullRequestReviewIDPaginates(t *testing.T) { + t.Parallel() + + client, err := github.NewClient(github.WithHTTPClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _ = json.NewEncoder(w).Encode([]*github.PullRequestReview{ + {NodeID: github.Ptr("PRR_pending"), State: github.Ptr("PENDING")}, + }) + return + } + w.Header().Set("Link", `; rel="next"`) + _ = json.NewEncoder(w).Encode([]*github.PullRequestReview{ + {NodeID: github.Ptr("PRR_submitted"), State: github.Ptr("COMMENTED")}, + }) + }, + }))) + require.NoError(t, err) + + reviewID, errResult := GetPendingPullRequestReviewID(context.Background(), client, "owner", "repo", 42) + require.Nil(t, errResult) + assert.Equal(t, githubv4.ID("PRR_pending"), reviewID) +} + func TestGetPullRequestDiff(t *testing.T) { t.Parallel() @@ -4252,76 +4277,15 @@ index 5d6e7b2..8a4f5c3 100644 } } -func viewerQuery(login string) githubv4mock.Matcher { - return githubv4mock.NewQueryMatcher( - struct { - Viewer struct { - Login githubv4.String - } `graphql:"viewer"` - }{}, - map[string]any{}, - githubv4mock.DataResponse(map[string]any{ - "viewer": map[string]any{ - "login": login, - }, +func mockPendingReviewClient(t *testing.T, nodeID string) *github.Client { + t.Helper() + client, err := github.NewClient(github.WithHTTPClient(MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsReviewsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, []*github.PullRequestReview{ + {NodeID: github.Ptr(nodeID), State: github.Ptr("PENDING")}, }), - ) -} - -type getLatestPendingReviewQueryReview struct { - id string - state string - url string -} - -type getLatestPendingReviewQueryParams struct { - author string - owner string - repo string - prNum int32 - - reviews []getLatestPendingReviewQueryReview -} - -func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mock.Matcher { - return githubv4mock.NewQueryMatcher( - 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)"` - }{}, - map[string]any{ - "author": githubv4.String(p.author), - "owner": githubv4.String(p.owner), - "name": githubv4.String(p.repo), - "prNum": githubv4.Int(p.prNum), - }, - githubv4mock.DataResponse( - map[string]any{ - "repository": map[string]any{ - "pullRequest": map[string]any{ - "reviews": map[string]any{ - "nodes": []any{ - map[string]any{ - "id": p.reviews[0].id, - "state": p.reviews[0].state, - "url": p.reviews[0].url, - }, - }, - }, - }, - }, - }, - ), - ) + }))) + require.NoError(t, err) + return client } func TestAddReplyToPullRequestComment(t *testing.T) {