diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 7e174db9ff..4b3225e221 100644 --- a/pkg/github/copilot.go +++ b/pkg/github/copilot.go @@ -3,6 +3,7 @@ package github import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -900,7 +901,7 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server ) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to request copilot review", + copilotReviewErrMsg(ctx, client, "failed to request copilot review", owner, repo, pullNumber, resp, err), resp, err, ), nil, nil @@ -920,6 +921,39 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server }) } +// copilotReviewErrMsg disambiguates the bare 404 this endpoint returns when the +// caller lacks write access, which is otherwise indistinguishable from a missing +// repository or pull request. Authoring the pull request does not grant write +// access, so fork contributors are refused here even though the website offers +// them a Copilot review. +// https://docs.github.com/en/pull-requests/reference/pull-request-reviews#requesting-and-requiring-reviews +func copilotReviewErrMsg(ctx context.Context, client *github.Client, base, owner, repo string, pullNumber int, resp *github.Response, err error) string { + if resp == nil || (resp.StatusCode != http.StatusNotFound && resp.StatusCode != http.StatusForbidden) { + return base + } + + // Rate limiting is also reported as 403, and the read below would be refused + // for the same reason, so leave the caller with the rate limit message. + var rateLimitErr *github.RateLimitError + var abuseErr *github.AbuseRateLimitError + if errors.As(err, &rateLimitErr) || errors.As(err, &abuseErr) { + return base + } + + repository, _, repoErr := client.Repositories.Get(ctx, owner, repo) + switch { + case repoErr != nil: + return fmt.Sprintf("%s. %s/%s could not be read with the current credentials, so it may not exist or the credentials may not reach it. "+ + "Lacking write access is refused with the same status.", base, owner, repo) + case !repository.GetPermissions().GetPush(): + return fmt.Sprintf("%s. The authenticated user has no write access to %s/%s, and GitHub requires write access to request a reviewer, even from the author of the pull request. "+ + "Request the Copilot review from the pull request page on the GitHub website instead, or ask someone with write access to request it.", base, owner, repo) + default: + return fmt.Sprintf("%s. The authenticated user has write access to %s/%s, so check that pull request #%d exists there and that Copilot code review is available for the repository. "+ + "Copilot code review is not available on GitHub Enterprise Server.", base, owner, repo, pullNumber) + } +} + func AssignCodingAgentPrompt(t translations.TranslationHelperFunc) inventory.ServerPrompt { return inventory.NewServerPrompt( ToolsetMetadataIssues, diff --git a/pkg/github/copilot_test.go b/pkg/github/copilot_test.go index 63c0cc8784..c90de4804b 100644 --- a/pkg/github/copilot_test.go +++ b/pkg/github/copilot_test.go @@ -5,8 +5,10 @@ import ( "encoding/json" "fmt" "net/http" + "strconv" "strings" "testing" + "time" "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" @@ -886,11 +888,12 @@ func Test_RequestCopilotReview(t *testing.T) { } tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedErrMsg string + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedErrMsg string + unexpectedErrMsg string }{ { name: "successful request", @@ -927,6 +930,116 @@ func Test_RequestCopilotReview(t *testing.T) { expectError: true, expectedErrMsg: "failed to request copilot review", }, + { + name: "pull request author without write access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, &github.Repository{ + Name: github.Ptr("repo"), + Permissions: &github.RepositoryPermissions{ + Pull: github.Ptr(true), + Push: github.Ptr(false), + }, + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + }, + expectError: true, + expectedErrMsg: "The authenticated user has no write access to owner/repo", + }, + { + name: "forbidden is explained the same way as not found", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusForbidden, map[string]any{"message": "Forbidden"}), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, &github.Repository{ + Name: github.Ptr("repo"), + Permissions: &github.RepositoryPermissions{ + Pull: github.Ptr(true), + Push: github.Ptr(false), + }, + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + }, + expectError: true, + expectedErrMsg: "The authenticated user has no write access to owner/repo", + }, + { + name: "write access present points at the pull request instead", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), + GetReposByOwnerByRepo: mockResponse(t, http.StatusOK, &github.Repository{ + Name: github.Ptr("repo"), + Permissions: &github.RepositoryPermissions{ + Pull: github.Ptr(true), + Push: github.Ptr(true), + }, + }), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(999), + }, + expectError: true, + expectedErrMsg: "check that pull request #999 exists there", + }, + { + name: "unreadable repository", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), + GetReposByOwnerByRepo: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + }, + expectError: true, + expectedErrMsg: "owner/repo could not be read with the current credentials", + }, + { + name: "server error is not explained as a permission problem", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusInternalServerError, map[string]any{"message": "Internal Server Error"}), + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + }, + expectError: true, + expectedErrMsg: "failed to request copilot review", + unexpectedErrMsg: "write access", + }, + { + name: "rate limited forbidden is not explained as a permission problem", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10)) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "API rate limit exceeded"}`)) + }, + GetReposByOwnerByRepo: func(_ http.ResponseWriter, _ *http.Request) { + t.Error("repository should not be read while rate limited") + }, + }), + requestArgs: map[string]any{ + "owner": "owner", + "repo": "repo", + "pullNumber": float64(1), + }, + expectError: true, + expectedErrMsg: "rate limit exceeded", + unexpectedErrMsg: "write access", + }, } for _, tc := range tests { @@ -949,6 +1062,9 @@ func Test_RequestCopilotReview(t *testing.T) { require.True(t, result.IsError) errorContent := getErrorResult(t, result) assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + if tc.unexpectedErrMsg != "" { + assert.NotContains(t, errorContent.Text, tc.unexpectedErrMsg) + } return }