fix(copilot): explain review request denials instead of forwarding a bare 404

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,

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
  }