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.

Author: dylanpulver

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,

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
 			}