From 698e35456975ee013e3dc13543de1d94a665c30f Mon Sep 17 00:00:00 2001 From: Dylan Pulver Date: Wed, 19 Aug 2026 16:52:57 +0300 Subject: [PATCH 1/2] fix(copilot): explain review request denials instead of forwarding a bare 404 The review request endpoint requires write access to the repository, and GitHub refuses a caller without it with 404 Not Found rather than a permission error. Authoring the pull request does not grant that access, so a fork contributor can be offered a Copilot review by the web UI and still be refused by request_copilot_review, with nothing in the tool result to say why. On 403 or 404 the tool now reads the repository once so it can name the cause. A caller without write access is told so directly and pointed at the web UI. When the repository cannot be read at all, or when write access is present, the message says so and points at the likelier cause. --- pkg/github/copilot.go | 34 +++++++++++- pkg/github/copilot_test.go | 106 +++++++++++++++++++++++++++++++++++-- 2 files changed, 134 insertions(+), 6 deletions(-) diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 7e174db9ff..218ccc9fe0 100644 --- a/pkg/github/copilot.go +++ b/pkg/github/copilot.go @@ -900,7 +900,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), resp, err, ), nil, nil @@ -920,6 +920,38 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server }) } +// copilotReviewErrMsg explains the opaque failures of the review request +// endpoint used by request_copilot_review. +// +// Requesting a reviewer needs write access to the repository. Being the author +// of the pull request does not grant it, which is why a fork contributor can be +// offered a Copilot review by the web UI and still be refused by the API. See +// https://docs.github.com/en/pull-requests/reference/pull-request-reviews#requesting-and-requiring-reviews +// +// The endpoint is documented to answer a caller who is not a collaborator with +// 403 or 422, but in practice it answers with 404 Not Found, which on its own +// is indistinguishable from a repository or pull request that does not exist. +// Reading the repository tells the two apart, and only runs once the request +// has already failed. +func copilotReviewErrMsg(ctx context.Context, client *github.Client, base, owner, repo string, pullNumber int, resp *github.Response) string { + if resp == nil || (resp.StatusCode != http.StatusNotFound && resp.StatusCode != http.StatusForbidden) { + 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 either it does not exist or the credentials cannot reach it. "+ + "GitHub refuses this endpoint the same way when the authenticated user has no write access to the repository.", 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..ef4ef05184 100644 --- a/pkg/github/copilot_test.go +++ b/pkg/github/copilot_test.go @@ -886,11 +886,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 +928,98 @@ func Test_RequestCopilotReview(t *testing.T) { expectError: true, expectedErrMsg: "failed to request copilot review", }, + { + // The author of a cross-fork pull request has no write access on the + // upstream repository, so GitHub refuses the review request with a 404 + // that says nothing about permissions. + 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", + }, + { + // A failure that carries no permission signal keeps the original message. + 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", + }, } for _, tc := range tests { @@ -949,6 +1042,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 } From 631703fc36574108fe5b154d4a05168526d1bed5 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Wed, 19 Aug 2026 16:51:04 +0200 Subject: [PATCH 2/2] fix(copilot): keep rate limit denials out of the review permission hint A 403 carrying X-RateLimit-Remaining: 0, or a secondary rate limit documentation URL, reached copilotReviewErrMsg as a rate limit error. It was explained as a missing repository or missing write access, and the repository read it triggered was refused for the same reason, so the caller paid an extra call to be told the wrong thing. Return the base message for both rate limit error types so the rate limit text stands on its own, and trim the helper and its tests to the comments the code cannot state. Co-authored-by: Dylan Pulver Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pkg/github/copilot.go | 36 +++++++++++++++++++----------------- pkg/github/copilot_test.go | 28 ++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/pkg/github/copilot.go b/pkg/github/copilot.go index 218ccc9fe0..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, - copilotReviewErrMsg(ctx, client, "failed to request copilot review", owner, repo, pullNumber, resp), + copilotReviewErrMsg(ctx, client, "failed to request copilot review", owner, repo, pullNumber, resp, err), resp, err, ), nil, nil @@ -920,31 +921,32 @@ func RequestCopilotReview(t translations.TranslationHelperFunc) inventory.Server }) } -// copilotReviewErrMsg explains the opaque failures of the review request -// endpoint used by request_copilot_review. -// -// Requesting a reviewer needs write access to the repository. Being the author -// of the pull request does not grant it, which is why a fork contributor can be -// offered a Copilot review by the web UI and still be refused by the API. See +// 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 -// -// The endpoint is documented to answer a caller who is not a collaborator with -// 403 or 422, but in practice it answers with 404 Not Found, which on its own -// is indistinguishable from a repository or pull request that does not exist. -// Reading the repository tells the two apart, and only runs once the request -// has already failed. -func copilotReviewErrMsg(ctx context.Context, client *github.Client, base, owner, repo string, pullNumber int, resp *github.Response) string { +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 either it does not exist or the credentials cannot reach it. "+ - "GitHub refuses this endpoint the same way when the authenticated user has no write access to the repository.", base, owner, repo) + 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. "+ + 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. "+ diff --git a/pkg/github/copilot_test.go b/pkg/github/copilot_test.go index ef4ef05184..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" @@ -929,9 +931,6 @@ func Test_RequestCopilotReview(t *testing.T) { expectedErrMsg: "failed to request copilot review", }, { - // The author of a cross-fork pull request has no write access on the - // upstream repository, so GitHub refuses the review request with a 404 - // that says nothing about permissions. name: "pull request author without write access", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusNotFound, map[string]any{"message": "Not Found"}), @@ -1006,7 +1005,6 @@ func Test_RequestCopilotReview(t *testing.T) { expectedErrMsg: "owner/repo could not be read with the current credentials", }, { - // A failure that carries no permission signal keeps the original message. 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"}), @@ -1020,6 +1018,28 @@ func Test_RequestCopilotReview(t *testing.T) { 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 {