From 3955758c249c4c52c055e6b325bb2873512f3dfd Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 8 Oct 2026 08:15:49 +0200 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM --- internal/tools/pr_merge.go | 10 +++- internal/tools/pr_merge_test.go | 69 +++++++++++++++++++++++- internal/tools/tbd_ship.go | 16 +++++- internal/tools/tbd_ship_internal_test.go | 38 +++++++++++++ internal/tools/tbd_ship_test.go | 20 ++++++- 5 files changed, 149 insertions(+), 4 deletions(-) diff --git a/internal/tools/pr_merge.go b/internal/tools/pr_merge.go index 49803fb..fcad724 100644 --- a/internal/tools/pr_merge.go +++ b/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, diff --git a/internal/tools/pr_merge_test.go b/internal/tools/pr_merge_test.go index 757f55a..41ca4ad 100644 --- a/internal/tools/pr_merge_test.go +++ b/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()) + }) + } +} diff --git a/internal/tools/tbd_ship.go b/internal/tools/tbd_ship.go index 17226ba..282312f 100644 --- a/internal/tools/tbd_ship.go +++ b/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 } diff --git a/internal/tools/tbd_ship_internal_test.go b/internal/tools/tbd_ship_internal_test.go index ef729e9..952eb18 100644 --- a/internal/tools/tbd_ship_internal_test.go +++ b/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()) + } +} diff --git a/internal/tools/tbd_ship_test.go b/internal/tools/tbd_ship_test.go index a569d68..90771d3 100644 --- a/internal/tools/tbd_ship_test.go +++ b/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