pr_merge was an independent merge path with no CI, protection or tier check. It now reads the PR, the base's branch protection and the head commit's CI runs, takes the tier from the issue named in `issue`, and runs the same evaluateShipGate as tbd_ship (helpers shared, not copied). A refusal is a result (merged:false, reason, ci_status, risk); an unreadable PR or protection probe is an error; a merge conflict is still an error. Above risk/low a human merges in the UI. Tests: tier table, no-issue, issue lookup failures, CI and protection not overridden by risk/low, read-side failures, tier read from the named issue only; nine wiring mutants flip-tested red. Shared reason text now says "PR left open" (the PR already exists for pr_merge). Behaviour change: pr_merge without `issue`, or on a PR with no green CI, merges nothing. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM
317 lines
12 KiB
Go
317 lines
12 KiB
Go
package tools_test
|
|
|
|
import (
|
|
"context"
|
|
"encoding/json"
|
|
"io"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"sync"
|
|
"testing"
|
|
|
|
"git.d-ma.be/mathias/gitea-mcp/internal/allowlist"
|
|
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
|
"git.d-ma.be/mathias/gitea-mcp/internal/tools"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// pr_merge is gated exactly like tbd_ship (gitea-mcp#74, #63): it merges only when the head commit's CI is fully
|
|
// green, the base is not review-protected AND the issue named in `issue` carries exactly one risk/ label, risk/low.
|
|
// The fake serves the whole read side the gate needs and records whether (and how) the merge endpoint was hit.
|
|
|
|
const (
|
|
greenRuns = `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`
|
|
lowLabels = `[{"id":1,"name":"risk/low"}]`
|
|
)
|
|
|
|
type prMergeFakeOpts struct {
|
|
runs string // workflow_runs array for the head commit
|
|
issueLabels string // labels array of issue 42
|
|
issueStatus int
|
|
prStatus int // GET pulls/7
|
|
protectionStatus int // 0 → 404 (unprotected); 200 with required approvals when requiredApprovals>0
|
|
requiredApprovals int
|
|
mergeStatus int
|
|
}
|
|
|
|
type prMergeFake struct {
|
|
*httptest.Server
|
|
mu sync.Mutex
|
|
mergeCalls int
|
|
mergeBody []byte
|
|
}
|
|
|
|
func (f *prMergeFake) merged() bool { f.mu.Lock(); defer f.mu.Unlock(); return f.mergeCalls > 0 }
|
|
|
|
func newPRMergeFake(t *testing.T, o prMergeFakeOpts) *prMergeFake {
|
|
t.Helper()
|
|
if o.runs == "" {
|
|
o.runs = greenRuns
|
|
}
|
|
if o.issueLabels == "" {
|
|
o.issueLabels = lowLabels
|
|
}
|
|
if o.issueStatus == 0 {
|
|
o.issueStatus = http.StatusOK
|
|
}
|
|
if o.prStatus == 0 {
|
|
o.prStatus = http.StatusOK
|
|
}
|
|
if o.mergeStatus == 0 {
|
|
o.mergeStatus = http.StatusNoContent
|
|
}
|
|
f := &prMergeFake{}
|
|
f.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
|
p := r.URL.Path
|
|
w.Header().Set("Content-Type", "application/json")
|
|
switch {
|
|
case r.Method == http.MethodGet && p == "/api/v1/repos/owner/repo/pulls/7":
|
|
w.WriteHeader(o.prStatus)
|
|
_, _ = w.Write([]byte(`{"number":7,"title":"t","state":"open","html_url":"http://x/pulls/7","head":{"ref":"feat","sha":"abc"},"base":{"ref":"main"}}`))
|
|
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
|
|
switch {
|
|
case o.protectionStatus != 0:
|
|
w.WriteHeader(o.protectionStatus)
|
|
_, _ = w.Write([]byte(`{"message":"boom"}`))
|
|
case o.requiredApprovals > 0:
|
|
_, _ = w.Write([]byte(`{"required_approvals":` + itoa(o.requiredApprovals) + `}`))
|
|
default:
|
|
w.WriteHeader(http.StatusNotFound)
|
|
_, _ = w.Write([]byte(`{"message":"not found"}`))
|
|
}
|
|
case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
|
|
_, _ = w.Write([]byte(`{"total_count":0,"workflow_runs":` + o.runs + `}`))
|
|
case r.Method == http.MethodGet && p == "/api/v1/repos/owner/repo/issues/42":
|
|
w.WriteHeader(o.issueStatus)
|
|
_, _ = w.Write([]byte(`{"number":42,"title":"t","state":"open","labels":` + o.issueLabels + `}`))
|
|
case r.Method == http.MethodPost && p == "/api/v1/repos/owner/repo/pulls/7/merge":
|
|
body, _ := io.ReadAll(r.Body)
|
|
f.mu.Lock()
|
|
f.mergeCalls++
|
|
f.mergeBody = body
|
|
f.mu.Unlock()
|
|
w.WriteHeader(o.mergeStatus)
|
|
_, _ = w.Write([]byte(`{"message":"merge conflict"}`))
|
|
default:
|
|
t.Errorf("unexpected request: %s %s", r.Method, p)
|
|
w.WriteHeader(http.StatusNotFound)
|
|
}
|
|
}))
|
|
t.Cleanup(f.Close)
|
|
return f
|
|
}
|
|
|
|
func callMerge(t *testing.T, f *prMergeFake, args string) (map[string]any, error) {
|
|
t.Helper()
|
|
tool := tools.NewPRMerge(gitea.NewClient(f.URL, "tok"), allowlist.New([]string{"owner"}))
|
|
out, err := tool.Call(context.Background(), json.RawMessage(args))
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
var res map[string]any
|
|
require.NoError(t, json.Unmarshal(out, &res))
|
|
return res, nil
|
|
}
|
|
|
|
const mergeArgs = `{"owner":"owner","repo":"repo","number":7,"issue":42}`
|
|
|
|
func TestPRMergeSuccess(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{})
|
|
|
|
res, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.NoError(t, err)
|
|
assert.Equal(t, true, res["merged"])
|
|
assert.Equal(t, "success", res["ci_status"])
|
|
assert.Equal(t, "low", res["risk"])
|
|
assert.Equal(t, float64(7), res["pr_number"])
|
|
assert.True(t, f.merged())
|
|
}
|
|
|
|
func TestPRMergeDefaultsToMergeStyleAndPassesStyleThrough(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{})
|
|
_, err := callMerge(t, f, mergeArgs)
|
|
require.NoError(t, err)
|
|
var payload map[string]any
|
|
require.NoError(t, json.Unmarshal(f.mergeBody, &payload))
|
|
assert.Equal(t, "merge", payload["Do"])
|
|
|
|
f2 := newPRMergeFake(t, prMergeFakeOpts{})
|
|
_, err = callMerge(t, f2, `{"owner":"owner","repo":"repo","number":7,"issue":42,"style":"squash"}`)
|
|
require.NoError(t, err)
|
|
require.NoError(t, json.Unmarshal(f2.mergeBody, &payload))
|
|
assert.Equal(t, "squash", payload["Do"])
|
|
}
|
|
|
|
// A merge conflict is still an error, as before the gate existed.
|
|
func TestPRMergeConflictReturnsError(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{mergeStatus: http.StatusConflict})
|
|
|
|
_, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.Error(t, err)
|
|
assert.ErrorIs(t, err, gitea.ErrConflict)
|
|
assert.True(t, f.merged(), "the merge was attempted: the gate had passed")
|
|
}
|
|
|
|
// #45: pr_merge advertises the canonical `number` (was `index`); `index` stays an accepted alias via the shim.
|
|
func TestPRMergeNumberCanonical(t *testing.T) {
|
|
sch := string(tools.NewPRMerge(gitea.NewClient("http://unused", ""), allowlist.New([]string{"owner"})).Descriptor().InputSchema)
|
|
assert.Contains(t, sch, `"number":`, "pr_merge must advertise number")
|
|
assert.NotContains(t, sch, `"index":`, "pr_merge must not advertise index")
|
|
|
|
for _, args := range []string{
|
|
`{"owner":"owner","repo":"repo","number":7,"issue":42}`,
|
|
`{"owner":"owner","repo":"repo","index":7,"issue":42}`,
|
|
`{"owner":"owner","name":"repo","index":7,"issue":42}`,
|
|
} {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{})
|
|
res, err := callMerge(t, f, args)
|
|
require.NoError(t, err, args)
|
|
assert.Equal(t, true, res["merged"], args)
|
|
}
|
|
}
|
|
|
|
// gitea-mcp#74: the descriptor tells the caller about the gate.
|
|
func TestPRMergeDescriptorAdvertisesTheGate(t *testing.T) {
|
|
d := tools.NewPRMerge(gitea.NewClient("http://unused", ""), allowlist.New([]string{"owner"})).Descriptor()
|
|
assert.Contains(t, string(d.InputSchema), `"issue":`)
|
|
assert.Contains(t, string(d.InputSchema), `"ci_timeout_seconds":`)
|
|
assert.Contains(t, d.Description, "risk/low")
|
|
assert.Contains(t, d.Description, "merged:false")
|
|
}
|
|
|
|
func TestPRMergeAllowlistRejects(t *testing.T) {
|
|
tool := tools.NewPRMerge(gitea.NewClient("http://unused", ""), allowlist.New([]string{"allowed"}))
|
|
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","name":"repo","index":1}`))
|
|
require.Error(t, err)
|
|
}
|
|
|
|
// ---- the tier: only exactly one risk/low merges ----
|
|
func TestPRMerge_RiskTier_OnlyLowMerges(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
labels string
|
|
wantMerged bool
|
|
wantInReason string
|
|
}{
|
|
{"risk/low merges", `[{"id":1,"name":"risk/low"}]`, true, ""},
|
|
{"risk/low among other labels merges", `[{"id":2,"name":"type/fix"},{"id":1,"name":"risk/low"}]`, true, ""},
|
|
{"risk/medium does not merge", `[{"id":1,"name":"risk/medium"}]`, false, "risk/medium"},
|
|
{"risk/high does not merge", `[{"id":1,"name":"risk/high"}]`, false, "risk/high"},
|
|
{"no risk label is unclassified, not low", `[{"id":2,"name":"type/fix"}]`, false, "no risk/"},
|
|
{"no labels at all", `[]`, false, "no risk/"},
|
|
{"two risk labels are ambiguous", `[{"id":1,"name":"risk/low"},{"id":2,"name":"risk/high"}]`, false, "more than one"},
|
|
{"two identical risk/low labels are still ambiguous", `[{"id":1,"name":"risk/low"},{"id":2,"name":"risk/low"}]`, false, "more than one"},
|
|
{"an unknown value does not merge", `[{"id":1,"name":"risk/lowish"}]`, false, "unknown"},
|
|
{"the value is case-sensitive: risk/Low", `[{"id":1,"name":"risk/Low"}]`, false, "unknown"},
|
|
{"trailing space in the value", `[{"id":1,"name":"risk/low "}]`, false, "unknown"},
|
|
{"RISK/LOW is not a risk/ label", `[{"id":1,"name":"RISK/LOW"}]`, false, "no risk/"},
|
|
}
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{issueLabels: tc.labels})
|
|
|
|
res, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.NoError(t, err, "a refusal is a result, not an error")
|
|
assert.Equal(t, tc.wantMerged, res["merged"])
|
|
assert.Equal(t, tc.wantMerged, f.merged(), "the merge endpoint is called iff the tier allows it")
|
|
assert.Equal(t, "success", res["ci_status"], "CI was green: the tier alone decided")
|
|
if !tc.wantMerged {
|
|
assert.Contains(t, res["reason"], tc.wantInReason)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestPRMerge_NoIssue_Unclassified_NoMerge(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{})
|
|
|
|
res, err := callMerge(t, f, `{"owner":"owner","repo":"repo","number":7}`)
|
|
|
|
require.NoError(t, err)
|
|
assert.Equal(t, false, res["merged"])
|
|
assert.Equal(t, "none", res["risk"])
|
|
assert.Contains(t, res["reason"], "issue")
|
|
assert.False(t, f.merged(), "an unclassified change must not merge")
|
|
}
|
|
|
|
func TestPRMerge_IssueLookupFails_FailsClosed(t *testing.T) {
|
|
for _, status := range []int{http.StatusInternalServerError, http.StatusNotFound, http.StatusForbidden} {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{issueStatus: status})
|
|
|
|
res, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.NoError(t, err, "status %d", status)
|
|
assert.Equal(t, false, res["merged"], "status %d", status)
|
|
assert.Equal(t, "unreadable", res["risk"], "status %d", status)
|
|
assert.False(t, f.merged(), "status %d: an unreadable issue must not merge", status)
|
|
}
|
|
}
|
|
|
|
// ---- risk/low is necessary, not sufficient: CI and branch protection still gate ----
|
|
func TestPRMerge_RiskLow_DoesNotOverrideCI(t *testing.T) {
|
|
for name, runs := range map[string]string{
|
|
"red": `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`,
|
|
"cancelled": `[{"id":1,"status":"completed","conclusion":"cancelled","head_sha":"abc"}]`,
|
|
"in_progress": `[{"id":1,"status":"in_progress","conclusion":"","head_sha":"abc"}]`,
|
|
"no runs": `[]`,
|
|
"mixed": `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"},{"id":2,"status":"queued","conclusion":"","head_sha":"abc"}]`,
|
|
} {
|
|
t.Run(name, func(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{runs: runs})
|
|
|
|
res, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.NoError(t, err)
|
|
assert.Equal(t, false, res["merged"])
|
|
assert.NotEmpty(t, res["reason"])
|
|
assert.False(t, f.merged(), "risk/low must not override CI that is not fully green")
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestPRMerge_RiskLow_DoesNotOverrideAReviewProtectedBase(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{requiredApprovals: 1})
|
|
|
|
res, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.NoError(t, err)
|
|
assert.Equal(t, false, res["merged"])
|
|
assert.Contains(t, res["reason"], "review")
|
|
assert.False(t, f.merged())
|
|
}
|
|
|
|
// An unreadable PR or an unreadable branch-protection probe is an error, never a merge.
|
|
func TestPRMerge_ReadSideFailures_AreErrorsAndNeverMerge(t *testing.T) {
|
|
for name, o := range map[string]prMergeFakeOpts{
|
|
"PR lookup 500": {prStatus: http.StatusInternalServerError},
|
|
"PR lookup 404": {prStatus: http.StatusNotFound},
|
|
"protection probe 500": {protectionStatus: http.StatusInternalServerError},
|
|
} {
|
|
t.Run(name, func(t *testing.T) {
|
|
f := newPRMergeFake(t, o)
|
|
|
|
_, err := callMerge(t, f, mergeArgs)
|
|
|
|
require.Error(t, err)
|
|
assert.False(t, f.merged(), "must not merge when the gate cannot be evaluated")
|
|
})
|
|
}
|
|
}
|
|
|
|
// The classification comes from the issue the caller names, not from `number`: a PR number that is also an issue
|
|
// must not classify itself by accident. (The gate trusts the named issue: documented limit.)
|
|
func TestPRMerge_TheTierIsReadFromTheNamedIssueOnly(t *testing.T) {
|
|
f := newPRMergeFake(t, prMergeFakeOpts{issueLabels: `[{"id":1,"name":"risk/high"}]`})
|
|
|
|
res, err := callMerge(t, f, `{"owner":"owner","repo":"repo","number":7,"issue":42}`)
|
|
|
|
require.NoError(t, err)
|
|
assert.Equal(t, "high", res["risk"])
|
|
assert.False(t, f.merged())
|
|
}
|