fix(pulls): report the real reason when a merge is refused (#1107)
Some checks are pending
goreleaser / goreleaser (push) Waiting to run
goreleaser / release-image (push) Waiting to run

## Problem

`tea pr merge <index>` reports the same misleading error for every refusal:

```
failed to merge PR, is it still open?
```

The PR usually *is* still open — `tea pr <index>` 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 <xiaolunwen@gmail.com>
Reviewed-on: https://gitea.com/gitea/tea/pulls/1107
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Jan Baer <jan.s.baer@googlemail.com>
This commit is contained in:
Jan Baer 2026-09-09 19:15:46 +00:00 committed by Lunny Xiao
parent b2bab268d7
commit 4d09587d4c
2 changed files with 177 additions and 3 deletions

View file

@ -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
}
}

View file

@ -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())
}