From d1e501cc64ec6e3417ea63a2a6240401af330ba5 Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 8 Oct 2026 06:18:07 +0000 Subject: [PATCH] =?UTF-8?q?feat(pr=5Fmerge):=20gate=20it=20like=20tbd=5Fsh?= =?UTF-8?q?ip=20=E2=80=94=20green=20CI,=20unprotected=20base,=20exactly=20?= =?UTF-8?q?one=20risk/low=20issue=20(#74)=20(#76)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mathias --- docs/HOW_GITEA_MCP_WORKS.md | 21 +- internal/gitea/pulls.go | 2 + internal/tools/pr_merge.go | 67 +++- internal/tools/pr_merge_test.go | 392 ++++++++++++++++++++--- internal/tools/tbd_ship.go | 54 ++-- internal/tools/tbd_ship_internal_test.go | 38 +++ internal/tools/tbd_ship_test.go | 40 ++- 7 files changed, 529 insertions(+), 85 deletions(-) diff --git a/docs/HOW_GITEA_MCP_WORKS.md b/docs/HOW_GITEA_MCP_WORKS.md index ecc886c..1149221 100644 --- a/docs/HOW_GITEA_MCP_WORKS.md +++ b/docs/HOW_GITEA_MCP_WORKS.md @@ -125,12 +125,21 @@ 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. +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 +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/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 10d5f4b..fcad724 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,13 +71,47 @@ 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) + } + // 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 := evaluateMergeGate(pr.Head.Sha, 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, - Body: args.Body, + Do: style, + Title: args.Title, + Body: args.Body, + HeadCommitID: pr.Head.Sha, }); 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..41ca4ad 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,380 @@ 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 + emptyHeadSha bool // the PR reports head.sha == "" + prState string // default "open" + prDraft bool +} + +type prMergeFake struct { + *httptest.Server + 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 } + +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 + } + 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 + 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":"` + 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: + 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"): + 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) + _, _ = 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 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"})) + 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"]) } +// 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) { - 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()) +} + +// ---- 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 b8e4998..282312f 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,14 +189,25 @@ 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"} } } +// 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, @@ -302,9 +313,9 @@ 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) - gate := evaluateShipGate(runs, bp, risk) + 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 := evaluateMergeGate(pr.Head.Sha, runs, bp, risk) result := map[string]any{ "merged": false, @@ -320,9 +331,9 @@ 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 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 +350,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 +364,10 @@ 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 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 } @@ -362,7 +376,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 +407,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 } 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 215160d..90771d3 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" @@ -59,10 +60,12 @@ 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"): + body, _ := io.ReadAll(r.Body) + shipLastMergeBody.Store(string(body)) merged.Store(true) w.WriteHeader(mergeStatus) _, _ = w.Write([]byte(`{}`)) @@ -216,6 +219,41 @@ 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 + 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"}]`