Report the created or updated pull request when a later step fails

pkg/github/pullrequests.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"net/http"
 	"slices"
+	"strings"
 
 	"github.com/go-viper/mapstructure/v2"
 	"github.com/google/go-github/v92/github"
@@ -877,10 +878,14 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 					TeamReviewers: teamReviewers,
 				}
 
+				// The pull request already exists at this point, so a reviewer failure must still
+				// tell the caller which pull request was created. Otherwise a retry only reports
+				// that a pull request for the head branch already exists.
+				reviewerErrMsg := fmt.Sprintf("pull request #%d was created (%s), but failed to request reviewers", pr.GetNumber(), pr.GetHTMLURL())
 				_, reviewerResp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pr.GetNumber(), reviewersRequest)
 				if err != nil {
 					return ghErrors.NewGitHubAPIErrorResponse(ctx,
-						"failed to request reviewers",
+						reviewerErrMsg,
 						reviewerResp,
 						err,
 					), nil, nil
@@ -896,7 +901,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 					if err != nil {
 						return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
 					}
-					return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to request reviewers", reviewerResp, bodyBytes), nil, nil
+					return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, reviewerErrMsg, reviewerResp, bodyBytes), nil, nil
 				}
 			}
 
@@ -1080,6 +1085,16 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 				return utils.NewToolResultError("No update parameters provided."), nil, nil
 			}
 
+			// The steps below are separate API calls. Once one of them has changed the pull
+			// request, a later failure must say so, or the caller assumes nothing changed.
+			updated := false
+			stepErrMsg := func(step string) string {
+				if updated {
+					return fmt.Sprintf("pull request #%d was updated, but %s%s", pullNumber, strings.ToLower(step[:1]), step[1:])
+				}
+				return step
+			}
+
 			// Handle REST API updates (title, body, state, base, maintainer_can_modify)
 			if restUpdateNeeded {
 				client, err := deps.GetClient(ctx)
@@ -1104,6 +1119,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 					}
 					return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update pull request", resp, bodyBytes), nil, nil
 				}
+				updated = true
 			}
 
 			// Handle draft status changes using GraphQL
@@ -1128,7 +1144,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 					"prNum": githubv4.Int(pullNumber), // #nosec G115 - pull request numbers are always small positive integers
 				})
 				if err != nil {
-					return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find pull request", err), nil, nil
+					return ghErrors.NewGitHubGraphQLErrorResponse(ctx, stepErrMsg("Failed to find pull request"), err), nil, nil
 				}
 
 				currentIsDraft := bool(prQuery.Repository.PullRequest.IsDraft)
@@ -1149,7 +1165,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 							PullRequestID: prQuery.Repository.PullRequest.ID,
 						}, nil)
 						if err != nil {
-							return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to convert pull request to draft", err), nil, nil
+							return ghErrors.NewGitHubGraphQLErrorResponse(ctx, stepErrMsg("Failed to convert pull request to draft"), err), nil, nil
 						}
 					} else {
 						// Mark as ready for review
@@ -1166,9 +1182,10 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 							PullRequestID: prQuery.Repository.PullRequest.ID,
 						}, nil)
 						if err != nil {
-							return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to mark pull request ready for review", err), nil, nil
+							return ghErrors.NewGitHubGraphQLErrorResponse(ctx, stepErrMsg("Failed to mark pull request ready for review"), err), nil, nil
 						}
 					}
+					updated = true
 				}
 			}
 
@@ -1188,7 +1205,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 				_, resp, err := client.PullRequests.RequestReviewers(ctx, owner, repo, pullNumber, reviewersRequest)
 				if err != nil {
 					return ghErrors.NewGitHubAPIErrorResponse(ctx,
-						"failed to request reviewers",
+						stepErrMsg("failed to request reviewers"),
 						resp,
 						err,
 					), nil, nil
@@ -1204,8 +1221,9 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 					if err != nil {
 						return utils.NewToolResultErrorFromErr("failed to read response body", err), nil, nil
 					}
-					return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to request reviewers", resp, bodyBytes), nil, nil
+					return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, stepErrMsg("failed to request reviewers"), resp, bodyBytes), nil, nil
 				}
+				updated = true
 			}
 
 			// Get the final state of the PR to return
@@ -1216,7 +1234,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
 
 			finalPR, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber)
 			if err != nil {
-				return ghErrors.NewGitHubAPIErrorResponse(ctx, "Failed to get pull request", resp, err), nil, nil
+				return ghErrors.NewGitHubAPIErrorResponse(ctx, stepErrMsg("Failed to get pull request"), resp, err), nil, nil
 			}
 			defer func() {
 				if resp != nil && resp.Body != nil {

pkg/github/pullrequests_partial_failure_test.go

@@ -0,0 +1,207 @@
+package github
+
+import (
+	"context"
+	"maps"
+	"net/http"
+	"testing"
+
+	"github.com/github/github-mcp-server/v2/internal/githubv4mock"
+	"github.com/github/github-mcp-server/v2/pkg/translations"
+	"github.com/google/go-github/v92/github"
+	"github.com/shurcooL/githubv4"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func reviewerRequestRejected(w http.ResponseWriter, _ *http.Request) {
+	w.WriteHeader(http.StatusUnprocessableEntity)
+	_, _ = w.Write([]byte(`{"message":"Review cannot be requested from pull request author."}`))
+}
+
+func Test_CreatePullRequest_ReviewerFailureNamesCreatedPullRequest(t *testing.T) {
+	createdPR := &github.PullRequest{
+		ID:      new(int64(4242)),
+		Number:  new(42),
+		HTMLURL: new("https://github.com/owner/repo/pull/42"),
+	}
+	client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
+		PostReposPullsByOwnerByRepo:                               mockResponse(t, http.StatusCreated, createdPR),
+		PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: reviewerRequestRejected,
+	}))
+	deps := BaseDeps{Client: client}
+	serverTool := CreatePullRequest(translations.NullTranslationHelper)
+	handler := serverTool.Handler(deps)
+
+	request := createMCPRequest(map[string]any{
+		"owner":     "owner",
+		"repo":      "repo",
+		"title":     "Test PR",
+		"head":      "feature-branch",
+		"base":      "main",
+		"reviewers": []any{"testuser"},
+	})
+	result, err := handler(ContextWithDeps(context.Background(), deps), &request)
+	require.NoError(t, err)
+	require.True(t, result.IsError, "the reviewer request failed, so the call is still an error")
+
+	text := getErrorResult(t, result).Text
+	// The pull request exists now; a retry would only fail with "already exists", so the
+	// error has to carry the number and URL.
+	assert.Contains(t, text, "pull request #42 was created (https://github.com/owner/repo/pull/42), but failed to request reviewers")
+	assert.Contains(t, text, "Review cannot be requested from pull request author.")
+}
+
+func Test_UpdatePullRequest_LaterStepFailureReportsEarlierChanges(t *testing.T) {
+	updatedPR := &github.PullRequest{
+		ID:      new(int64(4242)),
+		Number:  new(42),
+		Title:   new("Changed title"),
+		HTMLURL: new("https://github.com/owner/repo/pull/42"),
+	}
+
+	draftQuery := func(isDraft bool) githubv4mock.Matcher {
+		return githubv4mock.NewQueryMatcher(
+			struct {
+				Repository struct {
+					PullRequest struct {
+						ID      githubv4.ID
+						IsDraft githubv4.Boolean
+					} `graphql:"pullRequest(number: $prNum)"`
+				} `graphql:"repository(owner: $owner, name: $repo)"`
+			}{},
+			map[string]any{
+				"owner": githubv4.String("owner"),
+				"repo":  githubv4.String("repo"),
+				"prNum": githubv4.Int(42),
+			},
+			githubv4mock.DataResponse(map[string]any{
+				"repository": map[string]any{
+					"pullRequest": map[string]any{"id": "PR_42", "isDraft": isDraft},
+				},
+			}),
+		)
+	}
+	convertToDraft := func(resp githubv4mock.GQLResponse) githubv4mock.Matcher {
+		return githubv4mock.NewMutationMatcher(
+			struct {
+				ConvertPullRequestToDraft struct {
+					PullRequest struct {
+						ID      githubv4.ID
+						IsDraft githubv4.Boolean
+					}
+				} `graphql:"convertPullRequestToDraft(input: $input)"`
+			}{},
+			githubv4.ConvertPullRequestToDraftInput{PullRequestID: "PR_42"},
+			nil,
+			resp,
+		)
+	}
+	convertedToDraft := githubv4mock.DataResponse(map[string]any{
+		"convertPullRequestToDraft": map[string]any{
+			"pullRequest": map[string]any{"id": "PR_42", "isDraft": true},
+		},
+	})
+
+	tests := []struct {
+		name          string
+		restHandlers  map[string]http.HandlerFunc
+		gqlMatchers   []githubv4mock.Matcher
+		args          map[string]any
+		expectedErr   string
+		notExpectText string
+	}{
+		{
+			name: "reviewers rejected after the title was changed",
+			restHandlers: map[string]http.HandlerFunc{
+				PatchReposPullsByOwnerByRepoByPullNumber:                  mockResponse(t, http.StatusOK, updatedPR),
+				PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: reviewerRequestRejected,
+			},
+			args:        map[string]any{"title": "Changed title", "reviewers": []any{"testuser"}},
+			expectedErr: "pull request #42 was updated, but failed to request reviewers",
+		},
+		{
+			name: "reviewers rejected after the draft state was changed",
+			restHandlers: map[string]http.HandlerFunc{
+				PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: reviewerRequestRejected,
+			},
+			gqlMatchers: []githubv4mock.Matcher{draftQuery(false), convertToDraft(convertedToDraft)},
+			args:        map[string]any{"draft": true, "reviewers": []any{"testuser"}},
+			expectedErr: "pull request #42 was updated, but failed to request reviewers",
+		},
+		{
+			name: "draft conversion fails after the title was changed",
+			restHandlers: map[string]http.HandlerFunc{
+				PatchReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, updatedPR),
+			},
+			gqlMatchers: []githubv4mock.Matcher{draftQuery(false), convertToDraft(githubv4mock.ErrorResponse("conversion failed"))},
+			args:        map[string]any{"title": "Changed title", "draft": true},
+			expectedErr: "pull request #42 was updated, but failed to convert pull request to draft",
+		},
+		{
+			name: "final read fails after the reviewers were requested",
+			restHandlers: map[string]http.HandlerFunc{
+				PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusCreated, updatedPR),
+				GetReposPullsByOwnerByRepoByPullNumber: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(http.StatusInternalServerError)
+					_, _ = w.Write([]byte(`{"message":"Server Error"}`))
+				},
+			},
+			args:        map[string]any{"reviewers": []any{"testuser"}},
+			expectedErr: "pull request #42 was updated, but failed to get pull request",
+		},
+		{
+			name: "reviewers rejected when nothing else was requested",
+			restHandlers: map[string]http.HandlerFunc{
+				PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber: reviewerRequestRejected,
+			},
+			args:          map[string]any{"reviewers": []any{"testuser"}},
+			expectedErr:   "failed to request reviewers",
+			notExpectText: "was updated",
+		},
+		{
+			name:          "draft conversion fails when nothing else was requested",
+			restHandlers:  map[string]http.HandlerFunc{},
+			gqlMatchers:   []githubv4mock.Matcher{draftQuery(false), convertToDraft(githubv4mock.ErrorResponse("conversion failed"))},
+			args:          map[string]any{"draft": true},
+			expectedErr:   "Failed to convert pull request to draft",
+			notExpectText: "was updated",
+		},
+		{
+			name: "title change rejected",
+			restHandlers: map[string]http.HandlerFunc{
+				PatchReposPullsByOwnerByRepoByPullNumber: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(http.StatusUnprocessableEntity)
+					_, _ = w.Write([]byte(`{"message":"Validation Failed"}`))
+				},
+			},
+			args:          map[string]any{"title": "Changed title", "reviewers": []any{"testuser"}},
+			expectedErr:   "failed to update pull request",
+			notExpectText: "was updated",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			deps := BaseDeps{
+				Client:    mustNewGHClient(t, MockHTTPClientWithHandlers(tc.restHandlers)),
+				GQLClient: githubv4.NewClient(githubv4mock.NewMockedHTTPClient(tc.gqlMatchers...)),
+			}
+			serverTool := UpdatePullRequest(translations.NullTranslationHelper)
+			handler := serverTool.Handler(deps)
+
+			args := map[string]any{"owner": "owner", "repo": "repo", "pullNumber": float64(42)}
+			maps.Copy(args, tc.args)
+			request := createMCPRequest(args)
+			result, err := handler(ContextWithDeps(context.Background(), deps), &request)
+			require.NoError(t, err)
+			require.True(t, result.IsError)
+
+			text := getErrorResult(t, result).Text
+			assert.Contains(t, text, tc.expectedErr)
+			if tc.notExpectText != "" {
+				assert.NotContains(t, text, tc.notExpectText)
+			}
+		})
+	}
+}