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"}]`