From 4d09587d4cb53dd3d7becee4514118993b01f7e0 Mon Sep 17 00:00:00 2001 From: Jan Baer Date: Wed, 9 Sep 2026 19:15:46 +0000 Subject: [PATCH] fix(pulls): report the real reason when a merge is refused (#1107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `tea pr merge ` reports the same misleading error for every refusal: ``` failed to merge PR, is it still open? ``` The PR usually *is* still open — `tea pr ` shows it as open and lists `Conflicting files` — so the message sends users looking in the wrong direction. ## Root cause Gitea answers an unmergeable PR with a 405 and a body naming the actual cause. The SDK's `MergePullRequest` is built on `getStatusCode`, which returns only the status code and never calls `statusCodeToErr`, so the body is discarded. tea receives `success=false, err=nil` with no server explanation to pass on, and fell back to guessing that the PR might be closed. ## Changes - Derive the refusal reason from the pull request when a merge fails: already merged, closed, draft, or not mergeable. - When the PR looks mergeable but was refused anyway, name the conditions tea cannot observe (required status checks, requested reviews, branch protection) instead of guessing. - Include the PR index in the error. - Add table-driven tests for every reason, plus the case where the follow-up PR lookup fails. The extra API call happens only on the failure path. Fixes #1022 --------- Co-authored-by: Lunny Xiao Reviewed-on: https://gitea.com/gitea/tea/pulls/1107 Reviewed-by: Lunny Xiao Co-authored-by: Jan Baer --- modules/task/pull_merge.go | 34 +++++++- modules/task/pull_merge_test.go | 146 ++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 modules/task/pull_merge_test.go diff --git a/modules/task/pull_merge.go b/modules/task/pull_merge.go index 171f5d2b..9930c902 100644 --- a/modules/task/pull_merge.go +++ b/modules/task/pull_merge.go @@ -19,8 +19,36 @@ func PullMerge(requestCtx stdctx.Context, login *config.Login, repoOwner, repoNa if err != nil { return err } - if !success { - return fmt.Errorf("failed to merge PR, is it still open?") + if success { + return nil + } + return fmt.Errorf("failed to merge PR #%d: %s", index, + mergeFailureReason(requestCtx, client, repoOwner, repoName, index)) +} + +// mergeFailureReason returns why merging was refused. The SDK reports refusal as +// success=false and discards Gitea's explanatory body, so the reason has to be +// re-derived from the PR. Costs one API call, on the failure path only. +func mergeFailureReason(requestCtx stdctx.Context, client *gitea.Client, repoOwner, repoName string, index int64) string { + // Fallback naming the conditions tea cannot observe, used when the PR looks + // mergeable but the merge was refused anyway. + const refused = "the server refused the merge; check required status checks, requested reviews, or branch protection rules" + + pr, _, err := client.PullRequests.GetPullRequest(requestCtx, repoOwner, repoName, index) + if err != nil || pr == nil { + return refused + } + + switch { + case pr.HasMerged: + return "it has already been merged" + case pr.State == gitea.StateClosed: + return "it is closed" + case pr.Draft: + return "it is a draft; mark it ready for review first" + case !pr.Mergeable: + return "it has conflicting files or is otherwise not mergeable" + default: + return refused } - return nil } diff --git a/modules/task/pull_merge_test.go b/modules/task/pull_merge_test.go new file mode 100644 index 00000000..789a7f91 --- /dev/null +++ b/modules/task/pull_merge_test.go @@ -0,0 +1,146 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package task + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + gitea "gitea.dev/sdk" + + "gitea.dev/tea/modules/config" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mergeTestServer answers the merge POST with mergeStatus and the PR GET with +// prJSON, or a 404 if prJSON is empty. +func mergeTestServer(t *testing.T, prJSON string, mergeStatus int) *httptest.Server { + t.Helper() + + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/merge"): + w.WriteHeader(mergeStatus) + // Gitea explains itself here; the SDK discards it. + _, _ = w.Write([]byte(`{"message":"Please try again later"}`)) + case r.Method == http.MethodGet: + if prJSON == "" { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"pull request does not exist"}`)) + return + } + _, _ = w.Write([]byte(prJSON)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + } + })) +} + +func pullJSON(state string, merged, draft, mergeable bool) string { + return fmt.Sprintf( + `{"number":3,"state":%q,"merged":%t,"draft":%t,"mergeable":%t,"head":{"sha":"abc123"}}`, + state, merged, draft, mergeable) +} + +func TestPullMerge(t *testing.T) { + tests := []struct { + name string + pr string + mergeStatus int + wantErr string + }{ + { + name: "success", + pr: pullJSON("open", false, false, true), + mergeStatus: http.StatusOK, + }, + { + name: "created is also success", + pr: pullJSON("open", false, false, true), + mergeStatus: http.StatusCreated, + }, + { + // gitea/tea#1022: an open PR with conflicts was reported as + // possibly not open. + name: "conflicting files", + pr: pullJSON("open", false, false, false), + mergeStatus: http.StatusMethodNotAllowed, + wantErr: "failed to merge PR #3: it has conflicting files or is otherwise not mergeable", + }, + { + name: "already merged", + pr: pullJSON("closed", true, false, false), + mergeStatus: http.StatusMethodNotAllowed, + wantErr: "failed to merge PR #3: it has already been merged", + }, + { + name: "closed", + pr: pullJSON("closed", false, false, false), + mergeStatus: http.StatusMethodNotAllowed, + wantErr: "failed to merge PR #3: it is closed", + }, + { + name: "draft", + pr: pullJSON("open", false, true, false), + mergeStatus: http.StatusMethodNotAllowed, + wantErr: "failed to merge PR #3: it is a draft; mark it ready for review first", + }, + { + // Open and mergeable, yet refused. + name: "refused while mergeable", + pr: pullJSON("open", false, false, true), + mergeStatus: http.StatusMethodNotAllowed, + wantErr: "failed to merge PR #3: the server refused the merge; check required status checks, requested reviews, or branch protection rules", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := mergeTestServer(t, tt.pr, tt.mergeStatus) + defer server.Close() + + err := PullMerge(t.Context(), &config.Login{ + Name: "test", + URL: server.URL, + Token: "secret-token", + VersionCheck: false, + }, "owner", "repo", 3, gitea.MergePullRequestOption{Style: gitea.MergeStyleMerge}) + + if tt.wantErr == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + }) + } +} + +// A refusal must still explain itself when the follow-up PR lookup fails. +func TestPullMergeReasonUnavailable(t *testing.T) { + server := mergeTestServer(t, "", http.StatusMethodNotAllowed) + defer server.Close() + + err := PullMerge(t.Context(), &config.Login{ + Name: "test", + URL: server.URL, + Token: "secret-token", + VersionCheck: false, + }, "owner", "repo", 3, gitea.MergePullRequestOption{ + Style: gitea.MergeStyleMerge, + // Set so the SDK skips its own pre-merge PR lookup. + HeadCommitId: "abc123", + }) + + require.Error(t, err) + assert.Equal(t, "failed to merge PR #3: the server refused the merge; check required status checks, requested reviews, or branch protection rules", err.Error()) +}