fix(merge gate): an unknown head commit never merges; pr_merge refuses closed and draft PRs (review of PR #76)

Review found a bypass: a PR reporting an empty head sha made the runs query lose its head_sha filter (the repo's 50
most recent runs of any commit were classified) and dropped head_commit_id (omitempty), so the merge was unbound.
Both tools now go through evaluateMergeGate, which refuses without a head sha, and pollRuns makes no query without
one. pr_merge also reports merged:false for a PR that is not open or is a draft (Gitea refuses closed ones itself;
the gate no longer depends on that). Tests: unit (gate, counting-server pollRuns with positive control), end to end
for both tools, CI scoped to head_sha, closed/merged/draft. The two caller-wiring mutants are equivalent to the
pollRuns guard and survive by design.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM

internal/tools/pr_merge.go

@@ -77,13 +77,21 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
 	if err != nil {
 		return nil, fmt.Errorf("read PR #%d: %w", args.Number, err)
 	}
+	// Gitea refuses closed and merged PRs itself; the gate does not depend on that.
+	if pr.State != "open" || pr.Draft {
+		reason := fmt.Sprintf("the PR is not open (state %q) — nothing to merge", pr.State)
+		if pr.State == "open" {
+			reason = "the PR is a draft — not merged"
+		}
+		return textOK(map[string]any{"merged": false, "pr_number": args.Number, "reason": reason})
+	}
 	bp, err := t.c.GetBranchProtection(ctx, args.Owner, args.Repo, pr.Base.Ref)
 	if err != nil {
 		return nil, fmt.Errorf("branch protection probe: %w", err)
 	}
 	runs := pollRuns(ctx, t.c, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
 	risk := riskTierOf(ctx, t.c, args.Owner, args.Repo, args.Issue)
-	gate := evaluateShipGate(runs, bp, risk)
+	gate := evaluateMergeGate(pr.Head.Sha, runs, bp, risk)
 
 	result := map[string]any{
 		"merged":    false,

internal/tools/pr_merge_test.go

@@ -34,6 +34,9 @@ type prMergeFakeOpts struct {
 	protectionStatus  int // 0 → 404 (unprotected); 200 with required approvals when requiredApprovals>0
 	requiredApprovals int
 	mergeStatus       int
+	emptyHeadSha      bool   // the PR reports head.sha == ""
+	prState           string // default "open"
+	prDraft           bool
 }
 
 type prMergeFake struct {
@@ -41,6 +44,7 @@ type prMergeFake struct {
 	mu         sync.Mutex
 	mergeCalls int
 	mergeBody  []byte
+	runsQuery  string // raw query of the last workflow-runs request
 }
 
 func (f *prMergeFake) merged() bool { f.mu.Lock(); defer f.mu.Unlock(); return f.mergeCalls > 0 }
@@ -62,6 +66,13 @@ func newPRMergeFake(t *testing.T, o prMergeFakeOpts) *prMergeFake {
 	if o.mergeStatus == 0 {
 		o.mergeStatus = http.StatusNoContent
 	}
+	if o.prState == "" {
+		o.prState = "open"
+	}
+	sha := "abc"
+	if o.emptyHeadSha {
+		sha = ""
+	}
 	f := &prMergeFake{}
 	f.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		p := r.URL.Path
@@ -69,7 +80,7 @@ func newPRMergeFake(t *testing.T, o prMergeFakeOpts) *prMergeFake {
 		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"}}`))
+			_, _ = w.Write([]byte(`{"number":7,"title":"t","state":"` + o.prState + `","draft":` + boolJSON(o.prDraft) + `,"html_url":"http://x/pulls/7","head":{"ref":"feat","sha":"` + sha + `"},"base":{"ref":"main"}}`))
 		case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
 			switch {
 			case o.protectionStatus != 0:
@@ -82,6 +93,9 @@ func newPRMergeFake(t *testing.T, o prMergeFakeOpts) *prMergeFake {
 				_, _ = w.Write([]byte(`{"message":"not found"}`))
 			}
 		case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
+			f.mu.Lock()
+			f.runsQuery = r.URL.RawQuery
+			f.mu.Unlock()
 			_, _ = 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)
@@ -103,6 +117,13 @@ func newPRMergeFake(t *testing.T, o prMergeFakeOpts) *prMergeFake {
 	return f
 }
 
+func boolJSON(b bool) string {
+	if b {
+		return "true"
+	}
+	return "false"
+}
+
 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"}))
@@ -327,3 +348,49 @@ func TestPRMerge_TheTierIsReadFromTheNamedIssueOnly(t *testing.T) {
 	assert.Equal(t, "high", res["risk"])
 	assert.False(t, f.merged())
 }
+
+// ---- review of PR #76 ----
+
+// An empty head sha used to disable BOTH the CI scoping (the runs query lost its head_sha filter, so the repo's most
+// recent runs of any commit were classified) and the merge binding (head_commit_id is omitempty). Fail closed.
+func TestPRMerge_AnEmptyHeadShaFailsClosed(t *testing.T) {
+	f := newPRMergeFake(t, prMergeFakeOpts{emptyHeadSha: true})
+
+	res, err := callMerge(t, f, mergeArgs)
+
+	require.NoError(t, err)
+	assert.Equal(t, false, res["merged"])
+	assert.Contains(t, res["reason"], "head commit")
+	assert.False(t, f.merged(), "an unknown head commit must never merge")
+	assert.Empty(t, f.runsQuery, "no unscoped CI query may be made when the head sha is unknown")
+}
+
+// CI is read for the PR's head commit only: the query names it.
+func TestPRMerge_CIIsScopedToTheHeadCommit(t *testing.T) {
+	f := newPRMergeFake(t, prMergeFakeOpts{})
+
+	_, err := callMerge(t, f, mergeArgs)
+
+	require.NoError(t, err)
+	assert.Contains(t, f.runsQuery, "head_sha=abc")
+}
+
+// Defence in depth: Gitea refuses closed/merged PRs itself, but the gate should not depend on that.
+func TestPRMerge_ClosedOrDraftPRsAreNotMerged(t *testing.T) {
+	for name, o := range map[string]prMergeFakeOpts{
+		"closed": {prState: "closed"},
+		"merged": {prState: "merged"},
+		"draft":  {prDraft: true},
+	} {
+		t.Run(name, func(t *testing.T) {
+			f := newPRMergeFake(t, o)
+
+			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())
+		})
+	}
+}

internal/tools/tbd_ship.go

@@ -197,6 +197,17 @@ func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk
 	}
 }
 
+// evaluateMergeGate is what both merge tools call. It adds one precondition to evaluateShipGate: the PR's head commit
+// must be known. CI is read for that commit and the merge is bound to it (head_commit_id); with an empty sha the runs
+// query would lose its filter (the repo's most recent runs of ANY commit would be classified) and the binding would be
+// dropped, so an unknown head commit never merges.
+func evaluateMergeGate(headSHA string, runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk riskTier) shipGate {
+	if headSHA == "" {
+		return shipGate{false, "none", "the PR's head commit is unknown — fail-closed (CI cannot be scoped and the merge cannot be bound to it); PR left open, not merged"}
+	}
+	return evaluateShipGate(runs, bp, risk)
+}
+
 var shipSlugRe = regexp.MustCompile(`[^a-z0-9]+`)
 
 // deriveBranch builds a deterministic short-lived branch name from the change,
@@ -304,7 +315,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
 	// CI gate on the PR head commit.
 	runs := pollRuns(ctx, t.c, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
 	risk := riskTierOf(ctx, t.c, args.Owner, args.Repo, args.Issue)
-	gate := evaluateShipGate(runs, bp, risk)
+	gate := evaluateMergeGate(pr.Head.Sha, runs, bp, risk)
 
 	result := map[string]any{
 		"merged":    false,
@@ -354,6 +365,9 @@ func riskTierOf(ctx context.Context, c *gitea.Client, owner, repo string, issue
 // them to reach a terminal state. timeoutSec == 0 is a single snapshot (no
 // sleep). A listing error yields no runs → the gate fails closed.
 func pollRuns(ctx context.Context, c *gitea.Client, owner, repo, headSHA string, timeoutSec int) []gitea.WorkflowRun {
+	if headSHA == "" {
+		return nil // never query without the commit filter: Gitea would return the repo's latest runs of any commit
+	}
 	if timeoutSec < 0 {
 		timeoutSec = 0
 	}

internal/tools/tbd_ship_internal_test.go

@@ -1,6 +1,11 @@
 package tools
 
 import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
 	"testing"
 
 	"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
@@ -98,3 +103,36 @@ func TestClassifyRisk(t *testing.T) {
 		})
 	}
 }
+
+// The merge gate needs a head commit to scope CI to and to bind the merge to: without one nothing merges.
+func TestEvaluateMergeGate_UnknownHeadCommitNeverMerges(t *testing.T) {
+	green := []gitea.WorkflowRun{{Status: "completed", Conclusion: "success"}}
+	g := evaluateMergeGate("", green, &gitea.BranchProtection{}, riskLow)
+	if g.merge {
+		t.Fatalf("merge = true for an empty head sha (reason %q)", g.reason)
+	}
+	if !strings.Contains(g.reason, "head commit") {
+		t.Errorf("reason %q should name the head commit", g.reason)
+	}
+	if ok := evaluateMergeGate("abc", green, &gitea.BranchProtection{}, riskLow); !ok.merge {
+		t.Errorf("a known head sha with green CI and risk/low must still merge, got %q", ok.reason)
+	}
+}
+
+func TestPollRuns_NoShaMakesNoQuery(t *testing.T) {
+	var requests atomic.Int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests.Add(1)
+		_, _ = w.Write([]byte(`{"total_count":1,"workflow_runs":[{"id":1,"status":"completed","conclusion":"success"}]}`))
+	}))
+	defer srv.Close()
+	c := gitea.NewClient(srv.URL, "tok")
+
+	if runs := pollRuns(context.Background(), c, "o", "r", "", 0); len(runs) != 0 || requests.Load() != 0 {
+		t.Errorf("an empty sha must make no request and return no runs, got %d runs after %d requests", len(runs), requests.Load())
+	}
+	// positive control: with a sha the same server is asked, so the zero above means something
+	if runs := pollRuns(context.Background(), c, "o", "r", "abc", 0); len(runs) != 1 || requests.Load() != 1 {
+		t.Errorf("control: with a sha expected 1 run after 1 request, got %d runs after %d requests", len(runs), requests.Load())
+	}
+}

internal/tools/tbd_ship_test.go

@@ -60,7 +60,7 @@ func shipFakeFull(t *testing.T, runsJSON string, requiredApprovals, mergeStatus
 			_, _ = w.Write([]byte(`{"content":{"path":"x","sha":"s"},"commit":{"sha":"c"}}`))
 		case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls"):
 			w.WriteHeader(http.StatusCreated)
-			_, _ = w.Write([]byte(`{"number":7,"title":"t","html_url":"http://x/pulls/7","state":"open","head":{"ref":"tbd/x","sha":"abc"},"base":{"ref":"main"}}`))
+			_, _ = w.Write([]byte(`{"number":7,"title":"t","html_url":"http://x/pulls/7","state":"open","head":{"ref":"tbd/x","sha":"` + shipPRHeadSha + `"},"base":{"ref":"main"}}`))
 		case r.Method == http.MethodGet && strings.Contains(p, "/actions/runs"):
 			_, _ = w.Write([]byte(`{"total_count":0,"workflow_runs":` + runsJSON + `}`))
 		case r.Method == http.MethodPost && strings.HasSuffix(p, "/pulls/7/merge"):
@@ -219,9 +219,27 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
 	assert.True(t, merged.Load())
 }
 
+// shipPRHeadSha is the head sha the fake reports for the PR it opens (tests may blank it, then restore it).
+var shipPRHeadSha = "abc"
+
 // shipLastMergeBody is the body of the most recent merge request the fake received.
 var shipLastMergeBody atomic.Value
 
+// An empty head sha used to unscope the CI query and drop the merge binding (review of PR #76). Fail closed.
+func TestTBDShip_AnEmptyHeadShaFailsClosed(t *testing.T) {
+	shipPRHeadSha = ""
+	t.Cleanup(func() { shipPRHeadSha = "abc" })
+	var merged, deleted atomic.Bool
+	srv := shipFake(t, shipGreen, &merged, &deleted)
+	defer srv.Close()
+
+	res := callShip(t, srv.URL, shipArgs)
+
+	assert.Equal(t, false, res["merged"])
+	assert.Contains(t, res["reason"], "head commit")
+	assert.False(t, merged.Load(), "an unknown head commit must never merge")
+}
+
 // TOCTOU: the merge is bound to the head commit whose CI the gate evaluated (gitea-mcp#74).
 func TestTBDShip_TheMergeIsBoundToTheCommitTheGateChecked(t *testing.T) {
 	var merged, deleted atomic.Bool