From c3d8401604028f762a170e4797d583b7a50243f3 Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 8 Oct 2026 08:11:40 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(pr=5Fmerge):=20gate=20it=20like=20tbd?= =?UTF-8?q?=5Fship=20=E2=80=94=20green=20CI,=20unprotected=20base,=20exact?= =?UTF-8?q?ly=20one=20risk/low=20issue=20(gitea-mcp#74)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM --- docs/HOW_GITEA_MCP_WORKS.md | 19 +- internal/tools/pr_merge.go | 52 +++++- internal/tools/pr_merge_test.go | 312 +++++++++++++++++++++++++++----- internal/tools/tbd_ship.go | 36 ++-- 4 files changed, 340 insertions(+), 79 deletions(-) diff --git a/docs/HOW_GITEA_MCP_WORKS.md b/docs/HOW_GITEA_MCP_WORKS.md index ecc886c..d92ccbb 100644 --- a/docs/HOW_GITEA_MCP_WORKS.md +++ b/docs/HOW_GITEA_MCP_WORKS.md @@ -125,12 +125,19 @@ by area: | Search / Templates | code_search, create_project_from_template | | Composite | tbd_ship (trunk-based ship helper) | -**`tbd_ship` merge gate (gitea-mcp#63).** A PR is squash-merged only when ALL of: the head commit's CI is fully green, -the base branch is not review-protected, and the issue named in `issue` carries **exactly one** `risk/` label, `risk/low`. -No `issue`, no `risk/` label, several, an unknown value (`risk/lowish`), `risk/medium`, `risk/high`, or an issue that -cannot be read all fail closed: the PR is opened, `merged:false`, with a `reason` and the `risk` tier in the result. -Unclassified is not low. Limit: the gate enforces the tier of the issue the caller names; it cannot prove that issue is -the one the change belongs to, so the link is the caller's claim (the convention in local-dev rule 5 still applies). +**Merge gate: `tbd_ship` (gitea-mcp#63) and `pr_merge` (gitea-mcp#74).** The two merge tools share one gate +(`evaluateShipGate`). A PR is merged only when ALL of: the head commit's CI is fully green, the base branch is not +review-protected, and the issue named in `issue` carries **exactly one** `risk/` label, `risk/low`. No `issue`, no +`risk/` label, several, an unknown value (`risk/lowish`), `risk/medium`, `risk/high`, or an issue that cannot be read +all fail closed: nothing is merged, the result is `merged:false` with a `reason`, the `ci_status` and the `risk` tier. +Unclassified is not low. Above `risk/low` a human reviews and merges in the Gitea UI (which this gate does not touch). +`pr_merge` additionally fails with an error, merging nothing, when the PR or the base's branch-protection state cannot +be read. A merge conflict is still an error. + +Limits: the gate enforces the tier of the issue the caller names; it cannot prove that issue is the one the change +belongs to, so the link is the caller's claim (local-dev rule 5's convention still applies). Not gated: merging in the +Gitea UI, and `file_write_branch` committing directly to a branch (`main` included), which is how `risk/low` work +reaches trunk. Platform-level enforcement for those is branch protection on `main` (local-dev#27 option 2). Shared tool-layer hygiene (`internal/tools/tool.go`): - **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so diff --git a/internal/tools/pr_merge.go b/internal/tools/pr_merge.go index 10d5f4b..86f8d8a 100644 --- a/internal/tools/pr_merge.go +++ b/internal/tools/pr_merge.go @@ -10,6 +10,9 @@ import ( "git.d-ma.be/mathias/gitea-mcp/internal/registry" ) +// PRMerge merges an existing PR behind the same gate as tbd_ship (gitea-mcp#74, #63): CI fully green, base not +// review-protected, and the issue named in `issue` classified exactly risk/low. Above risk/low a human reviews and +// merges in the Gitea UI; this tool then merges nothing and says why. type PRMerge struct { c *gitea.Client a *allowlist.Allowlist @@ -22,16 +25,18 @@ func NewPRMerge(c *gitea.Client, a *allowlist.Allowlist) *PRMerge { func (t *PRMerge) Descriptor() registry.ToolDescriptor { return registry.ToolDescriptor{ Name: "pr_merge", - Description: "Merge a pull request. style: merge (default), squash, or rebase.", + Description: "Merge a pull request, gated like tbd_ship: it merges ONLY when the head commit's CI is fully green AND the base branch is not review-protected AND the issue named in `issue` carries exactly one risk/ label, risk/low. Otherwise it merges nothing and returns merged:false with a reason and the risk tier (no issue named, no or several risk/ labels, an unknown value, risk/medium, risk/high, an unreadable issue, CI pending/red/absent, a review-protected base). Above risk/low a human reviews and merges in the Gitea UI. style: merge (default), squash, or rebase. Set ci_timeout_seconds to poll CI to completion (default 0 = single snapshot). Returns {merged, pr_number, ci_status, risk, reason}.", InputSchema: json.RawMessage(`{ "type":"object", "properties":{ "owner":{"type":"string"}, "repo":{"type":"string"}, "number":{"type":"integer","minimum":1}, + "issue":{"type":"integer","minimum":1,"description":"Issue number in the same repo whose risk/ label classifies this change. Merge happens only for exactly one label, risk/low; no issue, none, several, an unknown value, risk/medium or risk/high means nothing is merged (unclassified is not low)."}, "style":{"type":"string","enum":["merge","squash","rebase"]}, "merge_message_title":{"type":"string"}, - "merge_message_field":{"type":"string"} + "merge_message_field":{"type":"string"}, + "ci_timeout_seconds":{"type":"integer","minimum":0,"maximum":600,"description":"Poll CI up to this long for the head commit's runs to complete. Default 0 (single snapshot)."} }, "required":["owner","repo","number"] }`), @@ -39,12 +44,14 @@ func (t *PRMerge) Descriptor() registry.ToolDescriptor { } type prMergeArgs struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - Style string `json:"style"` - Title string `json:"merge_message_title"` - Body string `json:"merge_message_field"` + Owner string `json:"owner"` + Repo string `json:"repo"` + Number int `json:"number"` + Issue int `json:"issue"` + Style string `json:"style"` + Title string `json:"merge_message_title"` + Body string `json:"merge_message_field"` + CITimeoutSec int `json:"ci_timeout_seconds"` } func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessage, error) { @@ -64,6 +71,31 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag style = "merge" } + // The gate's inputs. An unreadable PR or branch-protection probe is an error: the gate cannot be evaluated, so + // nothing is merged. An unreadable issue is a tier (unreadable), not an error: it is reported and not merged. + pr, err := t.c.GetPullRequest(ctx, args.Owner, args.Repo, args.Number) + if err != nil { + return nil, fmt.Errorf("read PR #%d: %w", args.Number, err) + } + 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) + + result := map[string]any{ + "merged": false, + "pr_number": args.Number, + "ci_status": gate.ciStatus, + "risk": string(risk), + } + if !gate.merge { + result["reason"] = gate.reason + return textOK(result) + } + if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Number, gitea.MergePRArgs{ Do: style, Title: args.Title, @@ -71,6 +103,6 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag }); err != nil { return nil, err } - - return textOK(map[string]any{"merged": true}) + result["merged"] = true + return textOK(result) } diff --git a/internal/tools/pr_merge_test.go b/internal/tools/pr_merge_test.go index a2e8662..c6b01a6 100644 --- a/internal/tools/pr_merge_test.go +++ b/internal/tools/pr_merge_test.go @@ -6,6 +6,8 @@ import ( "io" "net/http" "net/http/httptest" + "strings" + "sync" "testing" "git.d-ma.be/mathias/gitea-mcp/internal/allowlist" @@ -15,80 +17,300 @@ import ( "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) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "/api/v1/repos/owner/repo/pulls/7/merge", r.URL.Path) - w.WriteHeader(http.StatusNoContent) - })) - defer srv.Close() + f := newPRMergeFake(t, prMergeFakeOpts{}) + + res, err := callMerge(t, f, mergeArgs) - tool := tools.NewPRMerge(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"owner"})) - out, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"owner","name":"repo","index":7}`)) require.NoError(t, err) - - var result map[string]any - require.NoError(t, json.Unmarshal(out, &result)) - assert.Equal(t, true, result["merged"]) + 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 TestPRMergeDefaultsToMergeStyle(t *testing.T) { - var captured []byte - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - var err error - captured, err = io.ReadAll(r.Body) - require.NoError(t, err) - w.WriteHeader(http.StatusNoContent) - })) - defer srv.Close() - - tool := tools.NewPRMerge(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"owner"})) - _, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"owner","name":"repo","index":7}`)) +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(captured, &payload)) + 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) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusConflict) - _, _ = w.Write([]byte(`{"message":"merge conflict"}`)) - })) - defer srv.Close() + f := newPRMergeFake(t, prMergeFakeOpts{mergeStatus: http.StatusConflict}) + + _, err := callMerge(t, f, mergeArgs) - tool := tools.NewPRMerge(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"owner"})) - _, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"owner","name":"repo","index":7}`)) 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. +// #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}`, - `{"owner":"owner","repo":"repo","index":7}`, + `{"owner":"owner","repo":"repo","number":7,"issue":42}`, + `{"owner":"owner","repo":"repo","index":7,"issue":42}`, + `{"owner":"owner","name":"repo","index":7,"issue":42}`, } { - var gotPath string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotPath = r.URL.Path - w.WriteHeader(http.StatusNoContent) - })) - tool := tools.NewPRMerge(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"owner"})) - _, err := tool.Call(context.Background(), json.RawMessage(args)) + f := newPRMergeFake(t, prMergeFakeOpts{}) + res, err := callMerge(t, f, args) require.NoError(t, err, args) - assert.Equal(t, "/api/v1/repos/owner/repo/pulls/7/merge", gotPath, args) - srv.Close() + 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()) +} diff --git a/internal/tools/tbd_ship.go b/internal/tools/tbd_ship.go index b8e4998..bd0f2f8 100644 --- a/internal/tools/tbd_ship.go +++ b/internal/tools/tbd_ship.go @@ -117,19 +117,19 @@ func classifyRisk(labels []gitea.Label) riskTier { } } -const unclassifiedReason = "no risk/ label on the named issue (or no issue named) — unclassified is not low; PR opened, not merged" +const unclassifiedReason = "no risk/ label on the named issue (or no issue named) — unclassified is not low; PR left open, not merged" // riskBlockReason explains why a tier other than riskLow blocks the merge. func riskBlockReason(r riskTier) string { switch r { case riskMedium, riskHigh: - return fmt.Sprintf("the issue is risk/%s — above risk/low a human reviews the change; PR opened, not merged", r) + return fmt.Sprintf("the issue is risk/%s — above risk/low a human reviews the change; PR left open, not merged", r) case riskAmbiguous: - return "the issue carries more than one risk/ label — ambiguous classification; PR opened, not merged" + return "the issue carries more than one risk/ label — ambiguous classification; PR left open, not merged" case riskUnknown: - return "the issue carries an unknown risk/ value — PR opened, not merged" + return "the issue carries an unknown risk/ value — PR left open, not merged" case riskUnreadable: - return "could not read the issue's labels — fail-closed; PR opened, not merged" + return "could not read the issue's labels — fail-closed; PR left open, not merged" case riskLow: return "" // never asked: only a non-low tier needs explaining case riskNone: @@ -177,7 +177,7 @@ func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk // Review-protected base can't be auto-merged regardless of CI. if bp != nil && bp.Protected && bp.RequiredApprovals > 0 { return shipGate{false, ci, fmt.Sprintf( - "base branch requires %d approving review(s) — auto-merge disabled; PR opened for manual merge", bp.RequiredApprovals)} + "base branch requires %d approving review(s) — auto-merge disabled; PR left open for manual merge", bp.RequiredApprovals)} } // Only risk/low merges on green. Checked before CI so an unclassified change never reads as "waiting for CI". @@ -189,11 +189,11 @@ func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk case "success": return shipGate{true, ci, ""} case "pending": - return shipGate{false, ci, "CI is still running for the head commit — PR opened; re-invoke once checks complete, or merge manually"} + return shipGate{false, ci, "CI is still running for the head commit — PR left open; re-invoke once checks complete, or merge manually"} case "failed": - return shipGate{false, ci, "CI failed for the head commit — PR opened, not merged"} + return shipGate{false, ci, "CI failed for the head commit — PR left open, not merged"} default: // none - return shipGate{false, ci, "no CI runs found for the head commit — fail-closed: a change with no CI gate is never auto-merged to trunk; PR opened for manual merge"} + return shipGate{false, ci, "no CI runs found for the head commit — fail-closed: a change with no CI gate is never auto-merged to trunk; PR left open for manual merge"} } } @@ -302,8 +302,8 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag } // CI gate on the PR head commit. - runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec) - risk := t.riskTierOf(ctx, args.Owner, args.Repo, args.Issue) + 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) result := map[string]any{ @@ -322,7 +322,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag // Green + unprotected → squash-merge, then delete the short-lived branch. if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, pr.Number, gitea.MergePRArgs{Do: "squash"}); err != nil { if errors.Is(err, gitea.ErrConflict) { - result["reason"] = "base moved and the merge is not clean — PR opened, not merged; resolve the conflict and merge manually" + result["reason"] = "base moved and the merge is not clean — PR left open, not merged; resolve the conflict and merge manually" return textOK(result) } return nil, fmt.Errorf("merge PR #%d: %w", pr.Number, err) @@ -339,11 +339,11 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag // riskTierOf classifies the change from the named issue. No issue, or an issue that cannot be read, never yields // riskLow. The link between the PR and the issue is the caller's claim: the gate enforces the tier of the issue it // is given, it cannot prove that issue is the one the change belongs to (stated limit, gitea-mcp#63). -func (t *TBDShip) riskTierOf(ctx context.Context, owner, repo string, issue int) riskTier { +func riskTierOf(ctx context.Context, c *gitea.Client, owner, repo string, issue int) riskTier { if issue <= 0 { return riskNone } - iss, err := t.c.GetIssue(ctx, owner, repo, issue) + iss, err := c.GetIssue(ctx, owner, repo, issue) if err != nil || iss == nil { return riskUnreadable } @@ -353,7 +353,7 @@ func (t *TBDShip) riskTierOf(ctx context.Context, owner, repo string, issue int) // pollRuns lists the head commit's workflow runs, polling up to timeoutSec for // 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 (t *TBDShip) pollRuns(ctx context.Context, owner, repo, headSHA string, timeoutSec int) []gitea.WorkflowRun { +func pollRuns(ctx context.Context, c *gitea.Client, owner, repo, headSHA string, timeoutSec int) []gitea.WorkflowRun { if timeoutSec < 0 { timeoutSec = 0 } @@ -362,7 +362,7 @@ func (t *TBDShip) pollRuns(ctx context.Context, owner, repo, headSHA string, tim } deadline := time.Now().Add(time.Duration(timeoutSec) * time.Second) for { - runs := t.listRuns(ctx, owner, repo, headSHA) + runs := listRuns(ctx, c, owner, repo, headSHA) switch classifyCI(runs) { case "success", "failed": return runs // terminal — no point waiting @@ -393,8 +393,8 @@ func (t *TBDShip) findOpenPR(ctx context.Context, owner, repo, branch string) (* return nil, fmt.Errorf("no open PR found for head %s: %w", branch, gitea.ErrNotFound) } -func (t *TBDShip) listRuns(ctx context.Context, owner, repo, headSHA string) []gitea.WorkflowRun { - resp, err := t.c.ListWorkflowRuns(ctx, owner, repo, gitea.ListWorkflowRunsArgs{HeadSHA: headSHA, Limit: 50}) +func listRuns(ctx context.Context, c *gitea.Client, owner, repo, headSHA string) []gitea.WorkflowRun { + resp, err := c.ListWorkflowRuns(ctx, owner, repo, gitea.ListWorkflowRunsArgs{HeadSHA: headSHA, Limit: 50}) if err != nil || resp == nil { return nil } -- 2.54.0 From 6efd30915991561cf0ecc5ab2a183b91dfec3190 Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 8 Oct 2026 08:12:45 +0200 Subject: [PATCH 2/3] fix(merge gate): bind pr_merge and tbd_ship merges to the head commit the gate checked (TOCTOU) The gate evaluates CI for one head sha; a push to the PR branch between that check and the merge would have merged unchecked code. MergePRArgs now carries head_commit_id (documented in Gitea 1.27.1's MergePullRequestOption) and both tools send the sha they evaluated. Tests for both tools; each removal flip-tested red. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01SZkhi49hfeSqBp5aZJNXsM --- docs/HOW_GITEA_MCP_WORKS.md | 2 ++ internal/gitea/pulls.go | 2 ++ internal/tools/pr_merge.go | 7 ++++--- internal/tools/pr_merge_test.go | 13 +++++++++++++ internal/tools/tbd_ship.go | 2 +- internal/tools/tbd_ship_test.go | 20 ++++++++++++++++++++ 6 files changed, 42 insertions(+), 4 deletions(-) diff --git a/docs/HOW_GITEA_MCP_WORKS.md b/docs/HOW_GITEA_MCP_WORKS.md index d92ccbb..1149221 100644 --- a/docs/HOW_GITEA_MCP_WORKS.md +++ b/docs/HOW_GITEA_MCP_WORKS.md @@ -133,6 +133,8 @@ all fail closed: nothing is merged, the result is `merged:false` with a `reason` Unclassified is not low. Above `risk/low` a human reviews and merges in the Gitea UI (which this gate does not touch). `pr_merge` additionally fails with an error, merging nothing, when the PR or the base's branch-protection state cannot be read. A merge conflict is still an error. +Both tools bind the merge to the head commit whose CI the gate evaluated (`head_commit_id`), so a push to the PR +branch after the check makes Gitea refuse the merge instead of merging unchecked code. Limits: the gate enforces the tier of the issue the caller names; it cannot prove that issue is the one the change belongs to, so the link is the caller's claim (local-dev rule 5's convention still applies). Not gated: merging in the diff --git a/internal/gitea/pulls.go b/internal/gitea/pulls.go index a889c63..ca76163 100644 --- a/internal/gitea/pulls.go +++ b/internal/gitea/pulls.go @@ -109,6 +109,8 @@ type MergePRArgs struct { Do string `json:"Do"` Title string `json:"merge_message_title,omitempty"` Body string `json:"merge_message_field,omitempty"` + // HeadCommitID binds the merge to the commit whose CI was checked: if the PR branch moved, Gitea refuses. + HeadCommitID string `json:"head_commit_id,omitempty"` } func (c *Client) MergePullRequest(ctx context.Context, owner, repo string, index int, args MergePRArgs) error { diff --git a/internal/tools/pr_merge.go b/internal/tools/pr_merge.go index 86f8d8a..49803fb 100644 --- a/internal/tools/pr_merge.go +++ b/internal/tools/pr_merge.go @@ -97,9 +97,10 @@ func (t *PRMerge) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag } if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, args.Number, gitea.MergePRArgs{ - Do: style, - Title: args.Title, - Body: args.Body, + Do: style, + Title: args.Title, + Body: args.Body, + HeadCommitID: pr.Head.Sha, }); err != nil { return nil, err } diff --git a/internal/tools/pr_merge_test.go b/internal/tools/pr_merge_test.go index c6b01a6..757f55a 100644 --- a/internal/tools/pr_merge_test.go +++ b/internal/tools/pr_merge_test.go @@ -145,6 +145,19 @@ func TestPRMergeDefaultsToMergeStyleAndPassesStyleThrough(t *testing.T) { assert.Equal(t, "squash", payload["Do"]) } +// TOCTOU: the gate read CI for ONE head commit. The merge must be bound to that commit, so a push to the PR branch +// after the check makes Gitea refuse the merge instead of merging code nobody checked. +func TestPRMerge_TheMergeIsBoundToTheCommitTheGateChecked(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, "abc", payload["head_commit_id"], "the head sha the CI check used") +} + // A merge conflict is still an error, as before the gate existed. func TestPRMergeConflictReturnsError(t *testing.T) { f := newPRMergeFake(t, prMergeFakeOpts{mergeStatus: http.StatusConflict}) diff --git a/internal/tools/tbd_ship.go b/internal/tools/tbd_ship.go index bd0f2f8..17226ba 100644 --- a/internal/tools/tbd_ship.go +++ b/internal/tools/tbd_ship.go @@ -320,7 +320,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag } // Green + unprotected → squash-merge, then delete the short-lived branch. - if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, pr.Number, gitea.MergePRArgs{Do: "squash"}); err != nil { + if err := t.c.MergePullRequest(ctx, args.Owner, args.Repo, pr.Number, gitea.MergePRArgs{Do: "squash", HeadCommitID: pr.Head.Sha}); err != nil { if errors.Is(err, gitea.ErrConflict) { result["reason"] = "base moved and the merge is not clean — PR left open, not merged; resolve the conflict and merge manually" return textOK(result) diff --git a/internal/tools/tbd_ship_test.go b/internal/tools/tbd_ship_test.go index 215160d..a569d68 100644 --- a/internal/tools/tbd_ship_test.go +++ b/internal/tools/tbd_ship_test.go @@ -3,6 +3,7 @@ package tools_test import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" "strings" @@ -63,6 +64,8 @@ func shipFakeFull(t *testing.T, runsJSON string, requiredApprovals, mergeStatus 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"): + body, _ := io.ReadAll(r.Body) + shipLastMergeBody.Store(string(body)) merged.Store(true) w.WriteHeader(mergeStatus) _, _ = w.Write([]byte(`{}`)) @@ -216,6 +219,23 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) { assert.True(t, merged.Load()) } +// shipLastMergeBody is the body of the most recent merge request the fake received. +var shipLastMergeBody atomic.Value + +// 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 + srv := shipFake(t, shipGreen, &merged, &deleted) + defer srv.Close() + + res := callShip(t, srv.URL, shipArgs) + + require.Equal(t, true, res["merged"]) + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(shipLastMergeBody.Load().(string)), &payload)) + assert.Equal(t, "abc", payload["head_commit_id"], "the head sha the CI check used") +} + // ---- gitea-mcp#63: the risk tier is an input to the gate. Only exactly one risk/low label merges. ---- const ( shipGreen = `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]` -- 2.54.0 From 3955758c249c4c52c055e6b325bb2873512f3dfd Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 8 Oct 2026 08:15:49 +0200 Subject: [PATCH 3/3] 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 -- 2.54.0