diff --git a/apps/backend/internal/github/client.go b/apps/backend/internal/github/client.go index f7c0724cef..94e0dddb36 100644 --- a/apps/backend/internal/github/client.go +++ b/apps/backend/internal/github/client.go @@ -2,6 +2,7 @@ package github import ( "context" + "fmt" "time" ) @@ -13,6 +14,33 @@ type RateLimitFetcher interface { FetchRateLimit(ctx context.Context) error } +type MergeOutcome string + +const ( + MergeOutcomeMerged MergeOutcome = "merged" + MergeOutcomeQueued MergeOutcome = "queued" + mergeStatusFailed = "failed" +) + +type mergeAsyncResponse struct { + Status string `json:"status"` + UUID string `json:"uuid"` + Message string `json:"message"` +} + +func normalizeMergeOutcome(status string) (MergeOutcome, error) { + switch status { + case string(MergeOutcomeMerged): + return MergeOutcomeMerged, nil + case "enqueued", "queued": + return MergeOutcomeQueued, nil + case mergeStatusFailed: + return "", fmt.Errorf("GitHub rejected merge: %s", status) + default: + return "", fmt.Errorf("unexpected GitHub merge status %q", status) + } +} + // Client defines the interface for interacting with the GitHub API. type Client interface { // IsAuthenticated checks if the client is authenticated with GitHub. @@ -91,8 +119,8 @@ type Client interface { // RequestReviewers requests reviews from GitHub users on a pull request. RequestReviewers(ctx context.Context, owner, repo string, number int, reviewers []string) error - // MergePR merges a pull request. mergeMethod is one of "merge", "squash", "rebase". - MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error + // MergePR merges a pull request immediately or queues it according to GitHub policy. + MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) // ListRepoBranches lists branches for a repository. ListRepoBranches(ctx context.Context, owner, repo string) ([]RepoBranch, error) diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 15cd83398b..b65db9ccd7 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -759,7 +759,7 @@ func (c *Controller) httpMergePR(ctx *gin.Context) { ctx.JSON(http.StatusBadRequest, gin.H{"error": "merge_method must be merge, squash, or rebase"}) return } - principal, err := c.service.MergePRForWorkspace( + principal, outcome, err := c.service.MergePRForWorkspace( ctx.Request.Context(), ctx.Query("workspace_id"), currentGitHubUserID(ctx), owner, repo, number, req.MergeMethod, ) @@ -771,6 +771,8 @@ func (c *Controller) httpMergePR(ctx *gin.Context) { var apiErr *GitHubAPIError if errors.As(err, &apiErr) { switch apiErr.StatusCode { + case http.StatusBadRequest: + status = http.StatusBadRequest case http.StatusMethodNotAllowed, http.StatusConflict: status = http.StatusConflict case http.StatusUnauthorized: @@ -784,7 +786,7 @@ func (c *Controller) httpMergePR(ctx *gin.Context) { ctx.JSON(status, gin.H{"error": err.Error()}) return } - ctx.JSON(http.StatusOK, gin.H{"merged": true, "principal": principal}) + ctx.JSON(http.StatusOK, gin.H{"status": outcome, "principal": principal}) } func (c *Controller) httpListPRWatches(ctx *gin.Context) { diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 235db48192..a4258ab4f5 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -52,7 +52,7 @@ func TestValidGitHubCallbackState(t *testing.T) { type stubClient struct { getPRFunc func(ctx context.Context, owner, repo string, number int) (*PR, error) getIssueFunc func(ctx context.Context, owner, repo string, number int) (*Issue, error) - mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) error + mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) getRepoMergeMethodsFn func() (RepoMergeMethods, error) requestReviewersFn func(ctx context.Context, owner, repo string, number int, reviewers []string) error } @@ -132,11 +132,11 @@ func (s *stubClient) RequestReviewers(ctx context.Context, owner, repo string, n } return nil } -func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error { +func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { if s.mergePRFn != nil { return s.mergePRFn(ctx, owner, repo, number, mergeMethod) } - return nil + return MergeOutcomeMerged, nil } func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) { return nil, nil @@ -1780,12 +1780,12 @@ func TestHttpMergePR_Success(t *testing.T) { mergeMethod string } sc := &stubClient{ - mergePRFn: func(_ context.Context, owner, repo string, number int, mergeMethod string) error { + mergePRFn: func(_ context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { called.owner = owner called.repo = repo called.number = number called.mergeMethod = mergeMethod - return nil + return MergeOutcomeMerged, nil }, } router, _ := setupControllerTest(sc) @@ -1799,11 +1799,39 @@ func TestHttpMergePR_Success(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } + var response struct { + Status MergeOutcome `json:"status"` + } + if err := json.NewDecoder(w.Body).Decode(&response); err != nil { + t.Fatalf("decode response: %v", err) + } + if response.Status != MergeOutcomeMerged { + t.Errorf("status = %q, want merged", response.Status) + } if called.owner != "acme" || called.repo != "widget" || called.number != 42 || called.mergeMethod != "squash" { t.Errorf("unexpected MergePR args: %+v", called) } } +func TestHttpMergePR_Queued(t *testing.T) { + router, _ := setupControllerTest(&stubClient{ + mergePRFn: func(context.Context, string, string, int, string) (MergeOutcome, error) { + return MergeOutcomeQueued, nil + }, + }) + + req := httptest.NewRequest(http.MethodPut, "/api/v1/github/prs/acme/widget/42/merge", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), `"status":"queued"`) { + t.Errorf("body = %s, want queued outcome", w.Body.String()) + } +} + func TestHttpMergePR_InvalidMethod(t *testing.T) { router, _ := setupControllerTest(&stubClient{}) @@ -1824,9 +1852,9 @@ func TestHttpMergePR_EmptyBody_ResolvesToAllowedMethod(t *testing.T) { // should resolve to the first allowed method instead. var gotMethod string sc := &stubClient{ - mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { + mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) (MergeOutcome, error) { gotMethod = mergeMethod - return nil + return MergeOutcomeMerged, nil }, getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { // Squash-only repo (the case that surfaced this bug). @@ -1853,9 +1881,9 @@ func TestHttpMergePR_ExplicitMethod_Passthrough(t *testing.T) { var gotMethod string var lookupCalls int sc := &stubClient{ - mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { + mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) (MergeOutcome, error) { gotMethod = mergeMethod - return nil + return MergeOutcomeMerged, nil }, getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { lookupCalls++ @@ -1887,9 +1915,9 @@ func TestHttpMergePR_EmptyBody_LookupFails_FallsBackToGitHubDefault(t *testing.T // than refusing to merge because of an unrelated lookup failure. var gotMethod string sc := &stubClient{ - mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { + mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) (MergeOutcome, error) { gotMethod = mergeMethod - return nil + return MergeOutcomeMerged, nil }, getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { return RepoMergeMethods{}, fmt.Errorf("rate limited") @@ -2402,8 +2430,8 @@ func TestHandleListAccessibleRepos_PreservesAPIErrorStatus(t *testing.T) { func TestHttpMergePR_Conflict(t *testing.T) { sc := &stubClient{ - mergePRFn: func(context.Context, string, string, int, string) error { - return &GitHubAPIError{StatusCode: http.StatusMethodNotAllowed, Endpoint: "/merge", Body: "not mergeable"} + mergePRFn: func(context.Context, string, string, int, string) (MergeOutcome, error) { + return "", &GitHubAPIError{StatusCode: http.StatusMethodNotAllowed, Endpoint: "/merge", Body: "not mergeable"} }, } router, _ := setupControllerTest(sc) diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index c4d65ef782..bcebbe5360 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -901,23 +901,55 @@ func (c *GHClient) RequestReviewers(ctx context.Context, owner, repo string, num return nil } -func (c *GHClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error { - endpoint := fmt.Sprintf("repos/%s/%s/pulls/%d/merge", owner, repo, number) - args := []string{"api", endpoint, "-X", "PUT"} +func (c *GHClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { + endpoint := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async", owner, repo, number) + args := []string{"api", endpoint, "-X", "PUT", "-f", "merge_action=default"} if mergeMethod != "" { args = append(args, "-f", "merge_method="+mergeMethod) } - _, err := c.run(ctx, args...) + out, err := c.run(ctx, args...) + conflictBody := false if err != nil { // Surface status-based errors as GitHubAPIError so httpMergePR can - // translate 405 (not mergeable) / 409 (conflict) to HTTP 409 for - // gh CLI users too, matching the PAT path. + // translate merge rejections for gh CLI users, matching the PAT path. if code, ok := ghMergeStatusCode(err); ok { - return &GitHubAPIError{StatusCode: code, Endpoint: endpoint, Body: err.Error()} + if code != http.StatusConflict { + return "", &GitHubAPIError{StatusCode: code, Endpoint: endpoint, Body: err.Error()} + } + out = err.Error() + conflictBody = true + } else { + return "", fmt.Errorf("merge PR #%d: %w", number, err) } - return fmt.Errorf("merge PR #%d: %w", number, err) } - return nil + var response mergeAsyncResponse + jsonBody := out + if start := strings.Index(jsonBody, "{"); start >= 0 { + if end := strings.LastIndex(jsonBody, "}"); end >= start { + jsonBody = jsonBody[start : end+1] + } + } else if conflictBody { + return "", &GitHubAPIError{StatusCode: http.StatusConflict, Endpoint: endpoint, Body: out} + } + if unmarshalErr := json.Unmarshal([]byte(jsonBody), &response); unmarshalErr != nil { + return "", fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) + } + for response.Status == "pending" { + if response.UUID == "" { + return "", fmt.Errorf("GitHub merge response is pending without a UUID") + } + result, runErr := c.run(ctx, "api", endpoint+"/"+response.UUID) + if runErr != nil { + return "", fmt.Errorf("poll merge PR #%d: %w", number, runErr) + } + if unmarshalErr := json.Unmarshal([]byte(result), &response); unmarshalErr != nil { + return "", fmt.Errorf("decode GitHub merge response: %w", unmarshalErr) + } + } + if response.Status == mergeStatusFailed { + return "", fmt.Errorf("GitHub rejected merge: %s", response.Message) + } + return normalizeMergeOutcome(response.Status) } // ghMergeStatusCode extracts the HTTP status code from a gh CLI merge error. @@ -935,6 +967,10 @@ func ghMergeStatusCode(err error) (int, bool) { return http.StatusForbidden, true } s := err.Error() + if strings.Contains(s, "HTTP 400") || strings.Contains(s, "status: 400") || + strings.Contains(s, "400 Bad Request") { + return http.StatusBadRequest, true + } if strings.Contains(s, "HTTP 405") || strings.Contains(s, "status: 405") || strings.Contains(s, "405 Method Not Allowed") { return http.StatusMethodNotAllowed, true diff --git a/apps/backend/internal/github/gh_client_commands_test.go b/apps/backend/internal/github/gh_client_commands_test.go index f8f5581882..3119c8cc5e 100644 --- a/apps/backend/internal/github/gh_client_commands_test.go +++ b/apps/backend/internal/github/gh_client_commands_test.go @@ -530,21 +530,23 @@ func TestGHClient_SubmitReview(t *testing.T) { func TestGHClient_MergePR(t *testing.T) { t.Run("sends the merge method", func(t *testing.T) { - calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"merged":true}`}) - if err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { + calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"status":"merged"}`}) + if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { t.Fatalf("MergePR: %v", err) } assertGHArgv(t, calls(t), 0, []string{ - "api", "repos/acme/widget/pulls/42/merge", "-X", "PUT", "-f", "merge_method=squash", + "api", "repos/acme/widget/pulls/42/merge-async", "-X", "PUT", + "-f", "merge_action=default", "-f", "merge_method=squash", }) }) t.Run("omits the merge method when unset", func(t *testing.T) { - calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"merged":true}`}) - if err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { + calls := newFakeGH(t, ghResponse{Prefix: "api repos/", Stdout: `{"status":"merged"}`}) + if _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { t.Fatalf("MergePR: %v", err) } assertGHArgv(t, calls(t), 0, []string{ - "api", "repos/acme/widget/pulls/42/merge", "-X", "PUT", + "api", "repos/acme/widget/pulls/42/merge-async", "-X", "PUT", + "-f", "merge_action=default", }) }) } @@ -557,14 +559,14 @@ func TestGHClient_MergePR_StatusIsRecoverable(t *testing.T) { want int }{ {"gh: HTTP 405: Method Not Allowed", http.StatusMethodNotAllowed}, - {"gh: HTTP 409: Conflict", http.StatusConflict}, {"gh: HTTP 404: Not Found", http.StatusNotFound}, {"gh: HTTP 403: Forbidden", http.StatusForbidden}, + {"gh: merge conflict (HTTP 409)", http.StatusConflict}, } for _, tc := range cases { t.Run(http.StatusText(tc.want), func(t *testing.T) { newFakeGH(t, ghResponse{Prefix: "api repos/", Stderr: tc.stderr, Exit: 1}) - err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") var apiErr *GitHubAPIError if !errors.As(err, &apiErr) { t.Fatalf("err = %v, want a *GitHubAPIError", err) @@ -572,16 +574,27 @@ func TestGHClient_MergePR_StatusIsRecoverable(t *testing.T) { if apiErr.StatusCode != tc.want { t.Errorf("status = %d, want %d", apiErr.StatusCode, tc.want) } - if apiErr.Endpoint != "repos/acme/widget/pulls/42/merge" { + if apiErr.Endpoint != "repos/acme/widget/pulls/42/merge-async" { t.Errorf("endpoint = %q", apiErr.Endpoint) } }) } } +func TestGHClient_MergePR_AlreadyQueuedIsIdempotent(t *testing.T) { + newFakeGH(t, + ghResponse{Prefix: "api repos/acme/widget/pulls/42/merge-async/request-1", Stdout: `{"status":"enqueued","uuid":"request-1"}`}, + ghResponse{Prefix: "api repos/", Stderr: `{"status":"pending","uuid":"request-1"} gh: HTTP 409`, Exit: 1}, + ) + outcome, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "squash") + if err != nil || outcome != MergeOutcomeQueued { + t.Fatalf("outcome = %q, err = %v, want queued", outcome, err) + } +} + func TestGHClient_MergePR_UnmappedStatusFallsBackToAPlainWrap(t *testing.T) { newFakeGH(t, ghResponse{Prefix: "api repos/", Stderr: "gh: HTTP 500: server error", Exit: 1}) - err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := NewGHClient().MergePR(context.Background(), "acme", "widget", 42, "merge") if err == nil || !strings.Contains(err.Error(), "merge PR #42") { t.Fatalf("err = %v, want a plain wrap naming the PR", err) } diff --git a/apps/backend/internal/github/gh_client_test.go b/apps/backend/internal/github/gh_client_test.go index 2f962a8b15..a7f96a3788 100644 --- a/apps/backend/internal/github/gh_client_test.go +++ b/apps/backend/internal/github/gh_client_test.go @@ -69,6 +69,7 @@ func TestGhMergeStatusCode(t *testing.T) { {"404 Not Found phrase", errors.New("404 Not Found"), true, 404}, {"status 404", errors.New("request failed (status: 404)"), true, 404}, {"HTTP 403", errors.New("HTTP 403: Forbidden"), true, 403}, + {"HTTP 400", errors.New("HTTP 400: Bad Request"), true, 400}, {"HTTP 405", errors.New("gh: HTTP 405: Method Not Allowed"), true, 405}, {"status 405", errors.New("status: 405"), true, 405}, {"405 phrase", errors.New("405 Method Not Allowed"), true, 405}, diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 86c8006c4c..3852ac249e 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -115,6 +115,7 @@ type MockClient struct { submittedReviews []submittedReview requestedReviews []requestedReviewers mergedPRs []mergedPR + mergeOutcomes map[prKey]MergeOutcome mergeMethods map[repoKey]RepoMergeMethods repositoryDetails map[repoKey]*GitHubRepository gists map[string]mockGist @@ -167,6 +168,7 @@ func NewMockClient() *MockClient { prCommitsFailures: make(map[prKey]int), commitDetails: make(map[commitDetailKey]PRCommitDetail), mergeMethods: make(map[repoKey]RepoMergeMethods), + mergeOutcomes: make(map[prKey]MergeOutcome), repositoryDetails: make(map[repoKey]*GitHubRepository), gists: make(map[string]mockGist), repoFiles: make(map[repoKey][]repoFileEntry), @@ -748,19 +750,32 @@ func (m *MockClient) SetRepoMergeMethods(owner, repo string, methods RepoMergeMe m.mergeMethods[repoKey{owner, repo}] = methods } -func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, mergeMethod string) error { +func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { m.mu.Lock() defer m.mu.Unlock() m.mergedPRs = append(m.mergedPRs, mergedPR{ Owner: owner, Repo: repo, Number: number, MergeMethod: mergeMethod, }) + outcome := m.mergeOutcomes[prKey{owner, repo, number}] + if outcome == "" { + outcome = MergeOutcomeMerged + } + if outcome == MergeOutcomeQueued { + return outcome, nil + } now := time.Now().UTC() if pr, ok := m.prs[prKey{owner, repo, number}]; ok { pr.State = "merged" pr.MergedAt = &now pr.Mergeable = false } - return nil + return outcome, nil +} + +func (m *MockClient) SetMergeOutcome(owner, repo string, number int, outcome MergeOutcome) { + m.mu.Lock() + defer m.mu.Unlock() + m.mergeOutcomes[prKey{owner, repo, number}] = outcome } func (m *MockClient) CreateGist(_ context.Context, in CreateGistInput) (*GistResponse, error) { @@ -1034,6 +1049,7 @@ func (m *MockClient) Reset() { m.submittedReviews = nil m.requestedReviews = nil m.mergedPRs = nil + m.mergeOutcomes = make(map[prKey]MergeOutcome) m.mergeMethods = make(map[repoKey]RepoMergeMethods) m.gists = make(map[string]mockGist) m.deletedGists = nil diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index 1416b1ad06..c84bfaac4b 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -51,6 +51,7 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api.POST("/files", c.addPRFiles) api.POST("/commits", c.addPRCommits) api.PUT("/pr-commits-failures", c.setPRCommitsFailures) + api.PUT("/merge-outcomes", c.setMergeOutcome) api.POST("/commit-details", c.addPRCommitDetail) api.POST("/branches", c.addBranches) api.POST("/repo-files", c.addRepoFiles) @@ -518,6 +519,23 @@ func (c *MockController) setPRCommitsFailures(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"failures": req.Failures}) } +func (c *MockController) setMergeOutcome(ctx *gin.Context) { + var req struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Outcome MergeOutcome `json:"outcome"` + } + if err := ctx.ShouldBindJSON(&req); err != nil || strings.TrimSpace(req.Owner) == "" || + strings.TrimSpace(req.Repo) == "" || req.Number <= 0 || + (req.Outcome != MergeOutcomeMerged && req.Outcome != MergeOutcomeQueued) { + respondInvalidPayload(ctx) + return + } + c.mock.SetMergeOutcome(req.Owner, req.Repo, req.Number, req.Outcome) + ctx.JSON(http.StatusOK, gin.H{"outcome": req.Outcome}) +} + func (c *MockController) addPRCommitDetail(ctx *gin.Context) { var req struct { Owner string `json:"owner"` diff --git a/apps/backend/internal/github/noop_client.go b/apps/backend/internal/github/noop_client.go index b857aa30b9..79bb8bea24 100644 --- a/apps/backend/internal/github/noop_client.go +++ b/apps/backend/internal/github/noop_client.go @@ -125,8 +125,8 @@ func (c *NoopClient) RequestReviewers(context.Context, string, string, int, []st return ErrNoClient } -func (c *NoopClient) MergePR(context.Context, string, string, int, string) error { - return ErrNoClient +func (c *NoopClient) MergePR(context.Context, string, string, int, string) (MergeOutcome, error) { + return "", ErrNoClient } func (c *NoopClient) ListIssues(context.Context, string, string) ([]*Issue, error) { diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 96ce26acd9..2d9ff57ee8 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -728,17 +728,36 @@ func (c *PATClient) RequestReviewers(ctx context.Context, owner, repo string, nu return c.post(ctx, endpoint, jsonBody) } -func (c *PATClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error { - endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge", owner, repo, number) - payload := map[string]string{} +func (c *PATClient) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { + endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d/merge-async", owner, repo, number) + payload := map[string]string{"merge_action": "default"} if mergeMethod != "" { payload["merge_method"] = mergeMethod } jsonBody, err := json.Marshal(payload) if err != nil { - return fmt.Errorf("marshal merge payload: %w", err) + return "", fmt.Errorf("marshal merge payload: %w", err) } - return c.put(ctx, endpoint, jsonBody) + var response mergeAsyncResponse + if err := c.putJSON(ctx, endpoint, jsonBody, &response); err != nil { + var apiErr *GitHubAPIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusConflict || + json.Unmarshal([]byte(apiErr.Body), &response) != nil || response.UUID == "" { + return "", err + } + } + for response.Status == "pending" { + if response.UUID == "" { + return "", fmt.Errorf("GitHub merge response is pending without a UUID") + } + if err := c.get(ctx, endpoint+"/"+response.UUID, &response); err != nil { + return "", err + } + } + if response.Status == mergeStatusFailed { + return "", fmt.Errorf("GitHub rejected merge: %s", response.Message) + } + return normalizeMergeOutcome(response.Status) } func (c *PATClient) CreateGist(ctx context.Context, in CreateGistInput) (*GistResponse, error) { @@ -848,8 +867,18 @@ func (c *PATClient) post(ctx context.Context, endpoint string, body []byte) erro // postJSON sends a POST and decodes the response body into result. // 2xx with no body returns nil; 4xx/5xx returns a *GitHubAPIError. func (c *PATClient) postJSON(ctx context.Context, endpoint string, body []byte, result interface{}) error { + return c.requestJSON(ctx, http.MethodPost, endpoint, body, result) +} + +func (c *PATClient) requestJSON( + ctx context.Context, + method string, + endpoint string, + body []byte, + result interface{}, +) error { u := githubAPIBase + endpoint - req, err := http.NewRequestWithContext(ctx, http.MethodPost, u, bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, method, u, bytes.NewReader(body)) if err != nil { return err } @@ -858,7 +887,7 @@ func (c *PATClient) postJSON(ctx context.Context, endpoint string, body []byte, resp, err := c.httpClient.Do(req) if err != nil { - return fmt.Errorf("request POST %s: %w", endpoint, err) + return fmt.Errorf("request %s %s: %w", method, endpoint, err) } defer func() { _ = resp.Body.Close() }() c.recordRateHeaders(resp, endpoint) @@ -900,27 +929,11 @@ func (c *PATClient) delete(ctx context.Context, endpoint string) error { } func (c *PATClient) put(ctx context.Context, endpoint string, body []byte) error { - u := githubAPIBase + endpoint - req, err := http.NewRequestWithContext(ctx, http.MethodPut, u, bytes.NewReader(body)) - if err != nil { - return err - } - c.setGitHubHeaders(req) - req.Header.Set("Content-Type", "application/json") + return c.putJSON(ctx, endpoint, body, nil) +} - resp, err := c.httpClient.Do(req) - if err != nil { - return fmt.Errorf("request PUT %s: %w", endpoint, err) - } - defer func() { _ = resp.Body.Close() }() - c.recordRateHeaders(resp, endpoint) - - if resp.StatusCode >= 400 { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) - c.maybeMarkRateExhaustedFromBody(endpoint, resp.StatusCode, respBody) - return &GitHubAPIError{StatusCode: resp.StatusCode, Endpoint: endpoint, Body: string(respBody)} - } - return nil +func (c *PATClient) putJSON(ctx context.Context, endpoint string, body []byte, result interface{}) error { + return c.requestJSON(ctx, http.MethodPut, endpoint, body, result) } func (c *PATClient) get(ctx context.Context, endpoint string, result interface{}) error { diff --git a/apps/backend/internal/github/pat_client_writes_test.go b/apps/backend/internal/github/pat_client_writes_test.go index a0f247eb8e..1fc83e22f9 100644 --- a/apps/backend/internal/github/pat_client_writes_test.go +++ b/apps/backend/internal/github/pat_client_writes_test.go @@ -73,31 +73,31 @@ func TestPATClient_SubmitReview_ErrorIncludesStatusAndBody(t *testing.T) { func TestPATClient_MergePR(t *testing.T) { t.Run("sends the merge method", func(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { - return http.StatusOK, `{"merged":true}` + return http.StatusOK, `{"status":"merged"}` }) - if err := c.MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { + if _, err := c.MergePR(context.Background(), "acme", "widget", 42, "squash"); err != nil { t.Fatalf("MergePR: %v", err) } got := (*requests)[0] if got.Method != http.MethodPut { t.Errorf("method = %q, want PUT", got.Method) } - if got.Path != "/repos/acme/widget/pulls/42/merge" { + if got.Path != "/repos/acme/widget/pulls/42/merge-async" { t.Errorf("path = %q", got.Path) } - if got.Body != `{"merge_method":"squash"}` { - t.Errorf("body = %q, want the merge_method payload", got.Body) + if got.Body != `{"merge_action":"default","merge_method":"squash"}` { + t.Errorf("body = %q, want queue-aware merge payload", got.Body) } }) t.Run("omits the merge method when unset", func(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { - return http.StatusOK, `{"merged":true}` + return http.StatusOK, `{"status":"merged"}` }) - if err := c.MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { + if _, err := c.MergePR(context.Background(), "acme", "widget", 42, ""); err != nil { t.Fatalf("MergePR: %v", err) } - if (*requests)[0].Body != `{}` { - t.Errorf("body = %q, want an empty object so GitHub picks its default", + if (*requests)[0].Body != `{"merge_action":"default"}` { + t.Errorf("body = %q, want GitHub to choose direct or queued merge", (*requests)[0].Body) } }) @@ -111,7 +111,7 @@ func TestPATClient_MergePR_StatusIsRecoverable(t *testing.T) { c, _ := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return status, `{"message":"Pull Request is not mergeable"}` }) - err := c.MergePR(context.Background(), "acme", "widget", 42, "merge") + _, err := c.MergePR(context.Background(), "acme", "widget", 42, "merge") var apiErr *GitHubAPIError if !errors.As(err, &apiErr) { t.Fatalf("err = %v, want a *GitHubAPIError", err) @@ -119,7 +119,7 @@ func TestPATClient_MergePR_StatusIsRecoverable(t *testing.T) { if apiErr.StatusCode != status { t.Errorf("status = %d, want %d", apiErr.StatusCode, status) } - if apiErr.Endpoint != "/repos/acme/widget/pulls/42/merge" { + if apiErr.Endpoint != "/repos/acme/widget/pulls/42/merge-async" { t.Errorf("endpoint = %q", apiErr.Endpoint) } if !strings.Contains(apiErr.Body, "not mergeable") { @@ -129,6 +129,19 @@ func TestPATClient_MergePR_StatusIsRecoverable(t *testing.T) { } } +func TestPATClient_MergePR_AlreadyQueuedIsIdempotent(t *testing.T) { + c, _ := newRecordingPATServerFunc(t, func(r *http.Request) (int, string) { + if r.Method == http.MethodGet { + return http.StatusOK, `{"status":"enqueued","uuid":"request-1"}` + } + return http.StatusConflict, `{"status":"pending","uuid":"request-1"}` + }) + outcome, err := c.MergePR(context.Background(), "acme", "widget", 42, "squash") + if err != nil || outcome != MergeOutcomeQueued { + t.Fatalf("outcome = %q, err = %v, want queued", outcome, err) + } +} + func TestPATClient_CreateGist(t *testing.T) { c, requests := newRecordingPATServerFunc(t, func(*http.Request) (int, string) { return http.StatusCreated, `{"id":"abc123","html_url":"https://gist.github.com/abc123", diff --git a/apps/backend/internal/github/service_automation_auth.go b/apps/backend/internal/github/service_automation_auth.go index 7997f71cea..a4f5ed4af2 100644 --- a/apps/backend/internal/github/service_automation_auth.go +++ b/apps/backend/internal/github/service_automation_auth.go @@ -197,7 +197,8 @@ func (s *Service) MergePRForAutomation( if err := requireGitHubCapability(resolved, CapabilityPullRequestWrite); err != nil { return err } - return s.mergePRWithClient( + _, err = s.mergePRWithClient( ctx, resolved.Client, resolved.CacheScope, owner, repo, number, mergeMethod, ) + return err } diff --git a/apps/backend/internal/github/service_pr.go b/apps/backend/internal/github/service_pr.go index 692528f942..8558684880 100644 --- a/apps/backend/internal/github/service_pr.go +++ b/apps/backend/internal/github/service_pr.go @@ -124,9 +124,9 @@ func (s *Service) requestReviewersWithClient( // repo allows. The caller is expected to refresh PR feedback after a // successful merge — the background poller will catch the merged state on // its next pass. -func (s *Service) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error { +func (s *Service) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) (MergeOutcome, error) { if s.client == nil { - return ErrNoClient + return "", ErrNoClient } return s.mergePRWithClient(ctx, s.client, "legacy", owner, repo, number, mergeMethod) } @@ -141,20 +141,21 @@ func (s *Service) MergePRForWorkspace( repo string, number int, mergeMethod string, -) (AuthPrincipal, error) { +) (AuthPrincipal, MergeOutcome, error) { if err := s.ensureRepositoryInWorkspaceScope(ctx, workspaceID, owner, repo); err != nil { - return AuthPrincipal{}, err + return AuthPrincipal{}, "", err } resolved, err := s.resolvePersonalWriteClient(ctx, workspaceID, userID, owner, repo) if err != nil { - return AuthPrincipal{}, err + return AuthPrincipal{}, "", err } if err := requireGitHubCapability(resolved, CapabilityPullRequestWrite); err != nil { - return AuthPrincipal{}, err + return AuthPrincipal{}, "", err } - return resolved.Principal, s.mergePRWithClient( + outcome, err := s.mergePRWithClient( ctx, resolved.Client, resolved.CacheScope, owner, repo, number, mergeMethod, ) + return resolved.Principal, outcome, err } func (s *Service) mergePRWithClient( @@ -165,7 +166,7 @@ func (s *Service) mergePRWithClient( repo string, number int, mergeMethod string, -) error { +) (MergeOutcome, error) { if mergeMethod == "" { // Resolve to an allowed method up-front so we don't rely on GitHub's // "default to merge" behavior, which 405s on repos that disallow diff --git a/apps/web/components/github/pr-merge-button.test.tsx b/apps/web/components/github/pr-merge-button.test.tsx new file mode 100644 index 0000000000..e7257a973b --- /dev/null +++ b/apps/web/components/github/pr-merge-button.test.tsx @@ -0,0 +1,118 @@ +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; +import type { AppState } from "@/lib/state/store"; +import type { TaskPR } from "@/lib/types/github"; +import { PRMergeButton } from "./pr-merge-button"; + +const mergePRMock = vi.hoisted(() => vi.fn()); +const MERGE_BUTTON_TEST_ID = "pr-merge-button"; + +vi.mock("@/lib/api/domains/github-api", () => ({ mergePR: mergePRMock })); +vi.mock("@/hooks/domains/github/use-github-status", () => ({ + useGitHubStatus: () => ({ status: null }), +})); +vi.mock("@/hooks/domains/github/use-repo-merge-methods", () => ({ + useRepoMergeMethods: () => null, +})); + +const taskPR = { + id: "pr-1", + task_id: "task-1", + owner: "acme", + repo: "site", + pr_number: 42, + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "blocked", + review_count: 2, + required_reviews: 2, + pending_review_count: 0, +} as TaskPR; + +function renderButton(onMerged = vi.fn()) { + const initialState = { workspaces: { activeId: "workspace-1" } } as Partial; + render( + + + + + , + ); + return onMerged; +} + +beforeEach(() => mergePRMock.mockReset()); +afterEach(() => cleanup()); + +describe("PRMergeButton", () => { + it.each([ + ["merged", "PR merged"], + ["queued", "PR added to merge queue"], + ] as const)("reports a %s terminal outcome", async (status, message) => { + mergePRMock.mockResolvedValue({ status }); + const onMerged = renderButton(); + + fireEvent.click(screen.getByTestId(MERGE_BUTTON_TEST_ID)); + + expect(await screen.findByText(message)).not.toBeNull(); + expect(onMerged).toHaveBeenCalledOnce(); + expect(screen.queryByTestId(MERGE_BUTTON_TEST_ID)).toBeNull(); + }); + + it("remains retryable after a rejected merge", async () => { + mergePRMock.mockRejectedValueOnce(new Error("merge rejected")).mockResolvedValue({ + status: "queued", + }); + renderButton(); + + fireEvent.click(screen.getByTestId(MERGE_BUTTON_TEST_ID)); + expect(await screen.findByText("merge rejected")).not.toBeNull(); + fireEvent.click(screen.getByTestId(MERGE_BUTTON_TEST_ID)); + + await waitFor(() => expect(mergePRMock).toHaveBeenCalledTimes(2)); + }); + + it("suppresses duplicate clicks while the request is pending", async () => { + let resolve!: (value: { status: "queued" }) => void; + mergePRMock.mockReturnValue(new Promise((done) => (resolve = done))); + renderButton(); + const button = screen.getByTestId(MERGE_BUTTON_TEST_ID); + + fireEvent.click(button); + fireEvent.click(button); + expect(mergePRMock).toHaveBeenCalledOnce(); + + resolve({ status: "queued" }); + expect(await screen.findByText("PR added to merge queue")).not.toBeNull(); + }); + + it("does not apply a delayed acceptance to a different pull request", async () => { + let resolve!: (value: { status: "queued" }) => void; + mergePRMock.mockReturnValue(new Promise((done) => (resolve = done))); + const initialState = { workspaces: { activeId: "workspace-1" } } as Partial; + const view = render( + + + + + , + ); + + fireEvent.click(screen.getByTestId(MERGE_BUTTON_TEST_ID)); + const nextPR = { ...taskPR, id: "pr-2", pr_number: 43 }; + view.rerender( + + + + + , + ); + resolve({ status: "queued" }); + + await screen.findByText("PR added to merge queue"); + expect(screen.getByTestId(MERGE_BUTTON_TEST_ID)).not.toBeNull(); + }); +}); diff --git a/apps/web/components/github/pr-merge-button.tsx b/apps/web/components/github/pr-merge-button.tsx index c42e342674..a5f8c1d3ec 100644 --- a/apps/web/components/github/pr-merge-button.tsx +++ b/apps/web/components/github/pr-merge-button.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useState } from "react"; +import { useState } from "react"; import { IconChevronDown, IconGitMerge } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; import { @@ -16,7 +16,7 @@ import { useRepoMergeMethods } from "@/hooks/domains/github/use-repo-merge-metho import { mergePR } from "@/lib/api/domains/github-api"; import { getGitHubMutationActor } from "@/lib/github-auth"; import type { MergeMethod, TaskPR } from "@/lib/types/github"; -import { isPRReadyToMerge } from "./pr-task-icon"; +import { canAttemptPRMerge } from "./pr-task-icon"; import { useTranslation } from "react-i18next"; function MutationActor({ actor }: { actor: string | null }) { @@ -89,25 +89,20 @@ export function PRMergeButton({ const { t } = useTranslation(); const { toast } = useToast(); const [merging, setMerging] = useState(false); - // After a successful merge we stay hidden until the store catches up to - // state="merged" — otherwise the button briefly re-enables during the async - // refresh window and a double-click would hit the GitHub API again. - const [merged, setMerged] = useState(false); + // After GitHub accepts a direct or queued merge, stay hidden until refreshed + // provider state catches up so a repeat click cannot submit a second request. + const [acceptedFor, setAcceptedFor] = useState(null); const workspaceId = useAppStore((state) => state.workspaces.activeId); const methods = useRepoMergeMethods(workspaceId, taskPR.owner, taskPR.repo); const { status } = useGitHubStatus(workspaceId); const mutationActor = getGitHubMutationActor(status); - // If the same component instance ever renders a different PR (e.g. the user - // switches the active task while the panel/popover stays mounted), the - // sticky `merged` flag from a previous merge would hide the button for an - // unrelated, still-mergeable PR. Reset it whenever the underlying PR id - // changes. - useEffect(() => { - setMerged(false); - }, [taskPR.id]); + // Reset after switching PRs or after GitHub reports an observable lifecycle + // or mergeability transition, including a PR ejected from a merge queue. + const signature = `${taskPR.id}:${taskPR.state}:${taskPR.mergeable_state}`; + const accepted = acceptedFor === signature; - if (merged || !isPRReadyToMerge(taskPR)) return null; + if (accepted || !canAttemptPRMerge(taskPR)) return null; // `methods` may be null on first render, on lookup failure, or after the // 5-minute cache window. We still render the button — clicking with no // method routes through the backend's GetRepoMergeMethods resolver, so @@ -119,11 +114,23 @@ export function PRMergeButton({ const runMerge = async (method?: MergeMethod) => { if (!workspaceId) return; + const submittedSignature = signature; setMerging(true); try { - await mergePR(workspaceId, taskPR.owner, taskPR.repo, taskPR.pr_number, method); - setMerged(true); - toast({ description: t("github:prMerged"), variant: "success" }); + const result = await mergePR( + workspaceId, + taskPR.owner, + taskPR.repo, + taskPR.pr_number, + method, + ); + setAcceptedFor(submittedSignature); + toast({ + description: t( + result.status === "queued" ? "github:prAddedToMergeQueue" : "github:prMerged", + ), + variant: "success", + }); onMerged?.(); } catch (err) { toast({ @@ -144,11 +151,21 @@ export function PRMergeButton({ return ( m !== primary)} + extraMethods={ + taskPR.mergeable_state === "blocked" ? [] : allowed.filter((m) => m !== primary) + } onPickMethod={(m) => void runMerge(m)} /> ); @@ -197,7 +214,7 @@ function MergeButtonShell({