Compare commits

...
9 Commits
Author SHA1 Message Date
mathias d1e501cc64 feat(pr_merge): gate it like tbd_ship — green CI, unprotected base, exactly one risk/low issue (#74) (#76)
CD / Lint / Test / Vet (push) Successful in 15s
CD / Build & Import (push) Successful in 7s
CD / Deploy via GitOps (push) Skipped
Co-authored-by: Mathias <mthbqv@gmail.com>
2026-10-08 06:18:07 +00:00
mathias d3faffc4c4 feat(tbd_ship): gate auto-merge on the risk tier; unclassified is not low (#63) (#73)
CD / Build & Import (push) Successful in 4s
CD / Lint / Test / Vet (push) Successful in 11s
CD / Deploy via GitOps (push) Skipped
Co-authored-by: Mathias <mthbqv@gmail.com>
2026-10-08 06:02:34 +00:00
mathiasandClaude Opus 5.5 dbf78a3b34 ci: move template expressions out of run: shells (signals#47)
CD / Lint / Test / Vet (push) Successful in 12s
CD / Build & Import (push) Successful in 6s
CD / Deploy via GitOps (push) Successful in 5s
"Derive image tags" expanded ${{ github.ref }} into the run: body. A
ref name may contain $(...) and the expansion happens before the shell
parses the script, so it now arrives via env: REF instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 09:33:44 +02:00
mathias c3f8d6c002 ci: run the check job on pull requests (#68) (#69)
CD / Deploy via GitOps (push) Successful in 5s
CD / Lint / Test / Vet (push) Successful in 13s
CD / Build & Import (push) Successful in 6s
Co-authored-by: Mathias <mthbqv@gmail.com>
2026-10-05 07:25:38 +00:00
mathias dc127a05b5 Merge pull request 'fix(tools): reject case-variant arg keys and ref-shaped branch names (#65)' (#66) from fix/65-strict-args-branch-names into main
CD / Lint / Test / Vet (push) Successful in 14s
CD / Build & Import (push) Successful in 6s
CD / Deploy via GitOps (push) Skipped
2026-10-05 05:10:51 +00:00
mathiasandClaude Opus 5.5 cd61239451 perf(tools): linear duplicate-key scan keyed on SimpleFold orbit
PR #66 review round 2: the pairwise EqualFold loop was quadratic, 7s of
CPU for ~90k keys under the 1 MiB body cap. foldKey maps each rune to
the smallest member of its unicode.SimpleFold orbit, equal exactly when
strings.EqualFold is, so the scan is a map lookup again. The scaling
test fails on the previous code (7.1s).

Refs #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 07:05:00 +02:00
mathiasandClaude Opus 5.5 31a3dd3e0e fix(tools): fail closed when the key scan cannot read the input
PR #66 review: json.Decoder.Token() fails on a number outside float64
range (1e999), which json.Unmarshal ignores in an unknown key, so a
decoy key ended the scan and the #65 repro wrote to main. The decoder
now uses UseNumber, and any scan error is a validation error. The
duplicate check folds with strings.EqualFold (as encoding/json does)
instead of ToLower, so long s and the Kelvin sign are caught in nested
objects too.

Refs #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 07:02:27 +02:00
mathiasandClaude Opus 5.5 ae50501ba3 fix(tools): reject case-variant arg keys and ref-shaped branch names
parseArgs now refuses, before any request: two keys equal under
strings.EqualFold in any object (exact duplicates included), and a
top-level key that differs from a schema field only in case. Without
this, encoding/json's case-insensitive last-wins match let
{"branch":"feat/x","Branch":"main"} write to main while a client-side
hook read feat/x.

file_write_branch (branch) and tbd_ship (branch and base) validate the
branch name: ^[A-Za-z0-9][A-Za-z0-9._/-]*$ plus no refs/ prefix, HEAD,
"..", "//", "/.", trailing "/" or ".", or ".lock". refs/heads/main
otherwise created refs/heads/refs/heads/main and wedged API writes to
main. tbd_ship's base is the protection-probe key, so a refs/-prefixed
base would also have read as unprotected.

Closes #65

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 06:58:31 +02:00
mathiasandClaude Opus 5.5 9396b4daaa chore(context): resync derived adapters from canonical AGENT.md
CD / Lint / Test / Vet (push) Successful in 28s
CD / Build & Import (push) Successful in 14s
CD / Deploy via GitOps (push) Successful in 6s
Clears ContextAdapterDrift; canonical changed in local-dev 12f694b/218cf0d.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 22:49:57 +02:00
14 changed files with 1115 additions and 96 deletions
+1 -1
View File
@@ -304,7 +304,7 @@ entries that age well are about *why*, *how to avoid*, and *what to do when*.
|---------|-------|-------|
| **Claude Code, Claude Desktop** | `brain_query` (BM25), `brain_answer` (LLM-synth + sources) MCP tools | `brain_write` MCP tool |
| **Crush, Pi, Antigravity, other MCP-capable** | same MCP server: `ingestion-brain` (via the `mcp__*_brain__*` namespace once authenticated) | same |
| **Anything HTTP-only (curl, scripts)** | `POST https://brain-mcp.d-ma.be/query` with `{"query":"..."}` (auth via `BRAIN_MCP_TOKEN`) | `POST .../write` with `{"content":"...","filename":"..."}` |
| **Anything HTTP-only (curl, scripts)** | Tailnet only: `POST https://koala.tail30472e.ts.net:8443/query` with `{"query":"..."}`, sending `BRAIN_MCP_TOKEN` (not yet enforced on REST, hyperguild#149). The public `brain-mcp.d-ma.be` serves `/mcp` only (infra#517). | `POST .../write` with `{"content":"...","wing":"...","hall":"..."}` |
| **Browser / human inspection** | `https://git.d-ma.be/mathias/hyperguild` → `knowledge/` and `wiki/` markdown files |
- **Scoping — the brain holds no confidential client content.** Nothing on the read path
+10 -1
View File
@@ -8,6 +8,12 @@ name: CD
# (Only applies to branch pushes; tag pushes always trigger.)
paths-ignore:
- 'docs/**'
# gitea-mcp#68: without this a PR never got a CI run, so the risk/medium+
# PR lane merged on local `task check` alone. Only the check job runs for a
# PR -- build and deploy stay push/tag-only (no images, no infra writes,
# no secrets from a PR).
pull_request:
branches: [main]
env:
IMAGE: gitea-mcp
@@ -44,6 +50,7 @@ jobs:
build:
name: Build & Import
needs: check
if: github.event_name == 'push'
runs-on: self-hosted
outputs:
image-tag: ${{ steps.meta.outputs.sha-tag }}
@@ -52,10 +59,12 @@ jobs:
- name: Derive image tags
id: meta
env:
# signals#47: a ref name can carry $(...); pass it via env, not ${{ }}.
REF: ${{ github.ref }}
run: |
SHA=$(git rev-parse --short HEAD)
echo "sha-tag=${SHA}" >> "$GITHUB_OUTPUT"
REF="${{ github.ref }}"
if [[ "$REF" == refs/tags/v* ]]; then
echo "version-tag=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
echo "version=${REF#refs/tags/}" >> "$GITHUB_OUTPUT"
+1 -1
View File
@@ -299,7 +299,7 @@ entries that age well are about *why*, *how to avoid*, and *what to do when*.
|---------|-------|-------|
| **Claude Code, Claude Desktop** | `brain_query` (BM25), `brain_answer` (LLM-synth + sources) MCP tools | `brain_write` MCP tool |
| **Crush, Pi, Antigravity, other MCP-capable** | same MCP server: `ingestion-brain` (via the `mcp__*_brain__*` namespace once authenticated) | same |
| **Anything HTTP-only (curl, scripts)** | `POST https://brain-mcp.d-ma.be/query` with `{"query":"..."}` (auth via `BRAIN_MCP_TOKEN`) | `POST .../write` with `{"content":"...","filename":"..."}` |
| **Anything HTTP-only (curl, scripts)** | Tailnet only: `POST https://koala.tail30472e.ts.net:8443/query` with `{"query":"..."}`, sending `BRAIN_MCP_TOKEN` (not yet enforced on REST, hyperguild#149). The public `brain-mcp.d-ma.be` serves `/mcp` only (infra#517). | `POST .../write` with `{"content":"...","wing":"...","hall":"..."}` |
| **Browser / human inspection** | `https://git.d-ma.be/mathias/hyperguild` → `knowledge/` and `wiki/` markdown files |
- **Scoping — the brain holds no confidential client content.** Nothing on the read path
+16
View File
@@ -125,6 +125,22 @@ by area:
| Search / Templates | code_search, create_project_from_template |
| Composite | tbd_ship (trunk-based ship helper) |
**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
a tool works whichever spelling a caller sends (an explicit canonical wins).
+2
View File
@@ -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 {
+3
View File
@@ -63,6 +63,9 @@ func (t *FileWriteBranch) Call(ctx context.Context, raw json.RawMessage) (json.R
if args.Branch == "" {
return nil, fmt.Errorf("branch is required: %w", gitea.ErrValidation)
}
if err := ValidateBranchName(args.Branch); err != nil {
return nil, err
}
if args.Message == "" {
return nil, fmt.Errorf("message is required: %w", gitea.ErrValidation)
}
+42
View File
@@ -0,0 +1,42 @@
package tools
import (
"fmt"
"strings"
"testing"
"time"
)
// foldKey must agree with strings.EqualFold, which is how encoding/json matches.
func TestFoldKeyAgreesWithEqualFold(t *testing.T) {
pairs := [][2]string{{"branch", "Branch"}, {"sha", "ſha"}, {"k", "K"}, {"ss", "ß"}, {"a", "b"}, {"Σ", "ς"}}
for _, p := range pairs {
if got, want := foldKey(p[0]) == foldKey(p[1]), strings.EqualFold(p[0], p[1]); got != want {
t.Errorf("%q vs %q: foldKey equal=%v, EqualFold=%v", p[0], p[1], got, want)
}
}
}
// The duplicate scan is linear in the number of keys: 90k keys (about the
// 1 MiB body cap) must scan well under a second.
func TestRejectAmbiguousKeysScalesLinearly(t *testing.T) {
var b strings.Builder
b.WriteString("{")
for i := 0; i < 90_000; i++ {
if i > 0 {
b.WriteString(",")
}
fmt.Fprintf(&b, `"k%d":1`, i)
}
b.WriteString("}")
var dst struct {
Owner string `json:"owner"`
}
start := time.Now()
if err := rejectAmbiguousKeys([]byte(b.String()), &dst); err != nil {
t.Fatal(err)
}
if d := time.Since(start); d > 2*time.Second {
t.Errorf("scan of 90k keys took %v", d)
}
}
+54 -13
View File
@@ -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)
}
+347 -45
View File
@@ -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())
})
}
}
+116
View File
@@ -0,0 +1,116 @@
package tools_test
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"sync/atomic"
"testing"
"git.d-ma.be/mathias/gitea-mcp/internal/allowlist"
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
"git.d-ma.be/mathias/gitea-mcp/internal/tools"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// countingServer fails nothing by itself; it records how many requests reached
// it so a test can assert that a rejected call never touched Gitea.
func countingServer(t *testing.T) (*httptest.Server, *atomic.Int32) {
t.Helper()
var n atomic.Int32
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
n.Add(1)
w.WriteHeader(http.StatusTeapot)
}))
t.Cleanup(srv.Close)
return srv, &n
}
// #65 (1): encoding/json matches keys case-insensitively and the last one wins,
// so a client-side check reading `branch` could see a different value from the
// one the server acts on. Any two keys equal under strings.EqualFold, and any
// key that differs from a schema field only in case, are rejected before a
// request is made.
func TestParseArgsRejectsCaseVariantKeys(t *testing.T) {
cases := map[string]string{
"issue repro: branch + Branch": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","branch":"feat/y8","Branch":"main"}`,
"exact duplicate key": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","branch":"main"}`,
"case-only schema mismatch": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","BRANCH":"main"}`,
"nested object duplicate": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"a":1,"A":2}}`,
// PR #66 review: a number the token scanner cannot parse as float64 must
// not make the scan give up and let the duplicate through.
"1e999 decoy before duplicate": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","z":1e999,"branch":"feat/y8","Branch":"main"}`,
"-1e309 decoy before exact dup": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","z":-1e309,"branch":"feat/y8","branch":"main"}`,
// encoding/json folds like strings.EqualFold: long s (U+017F) == s.
"nested long-s duplicate": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"s":1,"\u017f":2}}`,
}
for name, raw := range cases {
t.Run(name, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
_, err := tool.Call(context.Background(), json.RawMessage(raw))
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
}
}
// #65 (2): `refs/heads/main` as a branch makes Gitea create
// refs/heads/refs/heads/main, after which every API write to main 500s.
// Branch names that can create such refs are rejected before any request.
func TestBranchNameValidationRejectsBeforeRequest(t *testing.T) {
bad := []string{
"refs/heads/main", "refs/tags/v1", "a..b", "feat x", "feat\tx", "feat/",
"/feat", "-feat", ".feat", "feat//x", "feat/.x", "feat.lock", "feat.", "HEAD", "feat@{1}", "",
}
for _, b := range bad {
t.Run("file_write_branch "+b, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
args, _ := json.Marshal(map[string]string{
"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
})
_, err := tool.Call(context.Background(), args)
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
if b == "" {
continue // tbd_ship derives a branch when empty
}
t.Run("tbd_ship "+b, func(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
args, _ := json.Marshal(map[string]string{
"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
})
_, err := tool.Call(context.Background(), args)
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
})
}
}
// tbd_ship's base is the branch-protection probe key: `refs/heads/main` would
// read as unprotected, so it is held to the same rule as the head branch.
func TestTBDShipRejectsRefsPrefixedBase(t *testing.T) {
srv, n := countingServer(t)
tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
_, err := tool.Call(context.Background(), json.RawMessage(
`{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","base":"refs/heads/main"}`))
require.Error(t, err)
assert.ErrorIs(t, err, gitea.ErrValidation)
assert.Zero(t, n.Load(), "no request may reach gitea")
}
func TestBranchNameValidationAcceptsOrdinaryNames(t *testing.T) {
for _, b := range []string{"main", "feat/y8", "tbd/fix-abc-1234abcd", "hold/mail-chat-infra-21", "v1.2_x"} {
t.Run(b, func(t *testing.T) {
assert.NoError(t, tools.ValidateBranchName(b))
})
}
}
+126 -16
View File
@@ -37,7 +37,7 @@ const (
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
return registry.ToolDescriptor{
Name: "tbd_ship",
Description: "Trunk-based ship of a single-file change: branch from base, write the file, open a PR, and auto-merge (squash) to base ONLY when the head commit's CI is fully green. Fails closed to PR-only (never merges) when CI is pending, red, or absent, when the base branch requires reviews, or when the merge isn't clean — returning the PR for manual handling with a reason. Set ci_timeout_seconds to poll CI to completion (default 0 = snapshot, which returns pending right after opening the PR). Returns {merged, pr_url, pr_number, ci_status, reason, branch}.",
Description: "Trunk-based ship of a single-file change: branch from base, write the file, open a PR, and auto-merge (squash) to base ONLY when the head commit's CI is fully green AND the named issue carries exactly one risk/ label, risk/low. Fails closed to PR-only (never merges) when CI is pending, red, or absent, when no issue is named or its risk/ label is missing, ambiguous, unknown, risk/medium or risk/high, when the issue cannot be read, when the base branch requires reviews, or when the merge isn't clean — returning the PR for manual handling with a reason. Set ci_timeout_seconds to poll CI to completion (default 0 = snapshot, which returns pending right after opening the PR). Returns {merged, pr_url, pr_number, ci_status, risk, reason, branch}.",
InputSchema: json.RawMessage(`{
"type":"object",
"properties":{
@@ -50,6 +50,7 @@ func (t *TBDShip) Descriptor() registry.ToolDescriptor {
"branch":{"type":"string","description":"Short-lived branch name. Default derived: tbd/<slug>-<hash>."},
"pr_title":{"type":"string","description":"PR title. Default: the commit message."},
"pr_body":{"type":"string"},
"issue":{"type":"integer","minimum":1,"description":"Issue number in the same repo whose risk/ label classifies this change. Auto-merge happens only for exactly one label, risk/low; no issue, none, several, an unknown value, risk/medium or risk/high means PR only, never merged (unclassified is not low)."},
"delete_branch":{"type":"boolean","description":"Delete the branch after a successful merge. Default true."},
"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)."}
},
@@ -68,10 +69,76 @@ type tbdShipArgs struct {
Branch string `json:"branch"`
PRTitle string `json:"pr_title"`
PRBody string `json:"pr_body"`
Issue int `json:"issue"`
DeleteBranch *bool `json:"delete_branch"`
CITimeoutSec int `json:"ci_timeout_seconds"`
}
// riskTier is the change's classification under the lane model (local-dev .context/AGENT.md rule 5): only a change
// whose issue carries exactly one risk/ label, risk/low, may merge on green; everything else is reviewed by a human
// (gitea-mcp#63). The zero value is deliberately NOT riskLow: a forgotten field must never mean "low".
type riskTier string
const (
riskLow riskTier = "low"
riskMedium riskTier = "medium"
riskHigh riskTier = "high"
riskNone riskTier = "none" // no issue named, or the issue has no risk/ label: unclassified is not low
riskAmbiguous riskTier = "ambiguous" // more than one risk/ label
riskUnknown riskTier = "unknown" // risk/<something else>
riskUnreadable riskTier = "unreadable" // the issue lookup failed
)
// classifyRisk reads the tier from an issue's labels. Exact, case-sensitive prefix "risk/"; exactly one such label
// is required, so two labels (even identical ones) are ambiguous rather than "the lower one wins".
func classifyRisk(labels []gitea.Label) riskTier {
var found []string
for _, l := range labels {
if strings.HasPrefix(l.Name, "risk/") {
found = append(found, strings.TrimPrefix(l.Name, "risk/"))
}
}
switch len(found) {
case 0:
return riskNone
case 1:
default:
return riskAmbiguous
}
switch found[0] {
case "low":
return riskLow
case "medium":
return riskMedium
case "high":
return riskHigh
default:
return riskUnknown
}
}
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 left open, not merged", r)
case riskAmbiguous:
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 left open, not merged"
case riskUnreadable:
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:
return unclassifiedReason
default: // the zero value and any tier added later: unclassified is not low
return unclassifiedReason
}
}
// shipGate is the decision produced by evaluateShipGate.
type shipGate struct {
merge bool
@@ -101,29 +168,46 @@ func classifyCI(runs []gitea.WorkflowRun) string {
}
// evaluateShipGate is the load-bearing safety of tbd_ship. It returns merge=true
// only when CI is fully green AND the base branch is not review-protected. Every
// other state fails closed to PR-only with an explanatory reason (#40).
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) shipGate {
// only when CI is fully green AND the base branch is not review-protected AND the
// change is classified risk/low. Every other state fails closed to PR-only with an
// explanatory reason (#40, gitea-mcp#63).
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk riskTier) shipGate {
ci := classifyCI(runs)
// 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".
if risk != riskLow {
return shipGate{false, ci, riskBlockReason(risk)}
}
switch ci {
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,
@@ -160,6 +244,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
if branch == "" {
branch = deriveBranch(args.Message, args.Path, args.Content)
}
// Both names reach a ref: base is also the protection-probe key, so a
// `refs/heads/main` base would read as unprotected (#65).
for _, b := range []string{base, branch} {
if err := ValidateBranchName(b); err != nil {
return nil, err
}
}
deleteBranch := true
if args.DeleteBranch != nil {
deleteBranch = *args.DeleteBranch
@@ -222,8 +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)
gate := evaluateShipGate(runs, bp)
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,
@@ -231,6 +323,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
"pr_url": pr.HTMLURL,
"branch": branch,
"ci_status": gate.ciStatus,
"risk": string(risk),
}
if !gate.merge {
result["reason"] = gate.reason
@@ -238,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)
@@ -254,10 +347,27 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
return textOK(result)
}
// 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 riskTierOf(ctx context.Context, c *gitea.Client, owner, repo string, issue int) riskTier {
if issue <= 0 {
return riskNone
}
iss, err := c.GetIssue(ctx, owner, repo, issue)
if err != nil || iss == nil {
return riskUnreadable
}
return classifyRisk(iss.Labels)
}
// 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
}
@@ -266,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
@@ -297,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
}
+95 -10
View File
@@ -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"
@@ -20,22 +25,31 @@ func TestEvaluateShipGate(t *testing.T) {
name string
runs []gitea.WorkflowRun
bp *gitea.BranchProtection
risk riskTier
wantMerge bool
wantCI string
}{
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, true, "success"},
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, false, "pending"},
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, false, "pending"},
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, false, "failed"},
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, false, "failed"},
{"no runs → no merge (no CI gate)", nil, open, false, "none"},
{"mixed success+queued → no merge", []gitea.WorkflowRun{run("completed", "success"), run("queued", "")}, open, false, "pending"},
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, false, "success"},
{"no runs + protected → no merge", nil, protected, false, "none"},
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskLow, true, "success"},
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, riskLow, false, "pending"},
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, riskLow, false, "pending"},
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, riskLow, false, "failed"},
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, riskLow, false, "failed"},
{"no runs → no merge (no CI gate)", nil, open, riskLow, false, "none"},
{"mixed success+queued → no merge", []gitea.WorkflowRun{run("completed", "success"), run("queued", "")}, open, riskLow, false, "pending"},
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, riskLow, false, "success"},
{"no runs + protected → no merge", nil, protected, riskLow, false, "none"},
// gitea-mcp#63: the tier is an input. Green CI never overrides it; unclassified is not low.
{"green + risk/medium → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskMedium, false, "success"},
{"green + risk/high → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskHigh, false, "success"},
{"green + no risk label → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskNone, false, "success"},
{"green + ambiguous labels → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskAmbiguous, false, "success"},
{"green + unknown value → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskUnknown, false, "success"},
{"green + unreadable issue → no merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskUnreadable, false, "success"},
{"green + the zero tier → no merge (a forgotten field must not mean low)", []gitea.WorkflowRun{run("completed", "success")}, open, riskTier(""), false, "success"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
g := evaluateShipGate(tc.runs, tc.bp)
g := evaluateShipGate(tc.runs, tc.bp, tc.risk)
if g.merge != tc.wantMerge {
t.Errorf("merge = %v, want %v (reason: %q)", g.merge, tc.wantMerge, g.reason)
}
@@ -51,3 +65,74 @@ func TestEvaluateShipGate(t *testing.T) {
})
}
}
func TestClassifyRisk(t *testing.T) {
l := func(names ...string) []gitea.Label {
out := make([]gitea.Label, len(names))
for i, n := range names {
out[i] = gitea.Label{Name: n}
}
return out
}
tests := []struct {
name string
labels []gitea.Label
want riskTier
}{
{"low", l("risk/low"), riskLow},
{"medium", l("risk/medium"), riskMedium},
{"high", l("risk/high"), riskHigh},
{"low among others", l("type/fix", "risk/low", "size/s"), riskLow},
{"none", l("type/fix"), riskNone},
{"no labels", nil, riskNone},
{"two different", l("risk/low", "risk/high"), riskAmbiguous},
{"two identical", l("risk/low", "risk/low"), riskAmbiguous},
{"unknown value", l("risk/extreme"), riskUnknown},
{"empty value", l("risk/"), riskUnknown},
{"wrong case", l("Risk/Low"), riskNone},
{"value case matters: risk/Low is unknown, not low", l("risk/Low"), riskUnknown},
{"trailing space in the value is unknown, not low", l("risk/low "), riskUnknown},
{"leading space in the value is unknown, not low", l("risk/ low"), riskUnknown},
{"prefix only inside a name", l("not-risk/low"), riskNone},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := classifyRisk(tc.labels); got != tc.want {
t.Errorf("classifyRisk(%v) = %q, want %q", tc.labels, got, tc.want)
}
})
}
}
// 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())
}
}
+154 -7
View File
@@ -3,6 +3,7 @@ package tools_test
import (
"context"
"encoding/json"
"io"
"net/http"
"net/http/httptest"
"strings"
@@ -26,11 +27,21 @@ func shipFake(t *testing.T, runsJSON string, merged, deleted *atomic.Bool) *http
// shipFakeOpts adds protection (requiredApprovals>0 → protected) and a merge
// status code, so tests can drive the review-protected and conflict paths.
func shipFakeOpts(t *testing.T, runsJSON string, requiredApprovals, mergeStatus int, merged, deleted *atomic.Bool) *httptest.Server {
t.Helper()
return shipFakeFull(t, runsJSON, requiredApprovals, mergeStatus, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, merged, deleted)
}
// shipFakeFull also serves the issue whose risk/ label classifies the change (gitea-mcp#63): issueLabels is the
// labels JSON array, issueStatus the HTTP status of the issue lookup.
func shipFakeFull(t *testing.T, runsJSON string, requiredApprovals, mergeStatus int, issueLabels string, issueStatus int, merged, deleted *atomic.Bool) *httptest.Server {
t.Helper()
return 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 && strings.Contains(p, "/issues/42"):
w.WriteHeader(issueStatus)
_, _ = w.Write([]byte(`{"number":42,"title":"t","state":"open","labels":` + issueLabels + `}`))
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
if requiredApprovals > 0 {
_, _ = w.Write([]byte(`{"required_approvals":` + itoa(requiredApprovals) + `}`))
@@ -49,10 +60,12 @@ func shipFakeOpts(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(`{}`))
@@ -83,7 +96,7 @@ func TestTBDShip_GreenCI_Merges(t *testing.T) {
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, true, res["merged"])
assert.Equal(t, "success", res["ci_status"])
@@ -98,7 +111,7 @@ func TestTBDShip_NoCI_FailsClosed(t *testing.T) {
srv := shipFake(t, `[]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.Equal(t, "none", res["ci_status"])
@@ -113,7 +126,7 @@ func TestTBDShip_RedCI_FailsClosed(t *testing.T) {
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.Equal(t, "failed", res["ci_status"])
@@ -128,7 +141,7 @@ func TestTBDShip_ReviewProtected_FailsClosed(t *testing.T) {
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 1, http.StatusOK, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.NotEmpty(t, res["reason"])
@@ -142,7 +155,7 @@ func TestTBDShip_MergeConflict_FailsClosed(t *testing.T) {
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 0, http.StatusConflict, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`)
assert.Equal(t, false, res["merged"])
assert.NotEmpty(t, res["reason"])
@@ -160,6 +173,8 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
p := r.URL.Path
w.Header().Set("Content-Type", "application/json")
switch {
case r.Method == http.MethodGet && strings.Contains(p, "/issues/42"):
_, _ = w.Write([]byte(`{"number":42,"title":"t","state":"open","labels":[{"id":1,"name":"risk/low"}]}`))
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
w.WriteHeader(http.StatusNotFound)
_, _ = w.Write([]byte(`{"message":"not found"}`))
@@ -196,7 +211,7 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
}))
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","branch":"`+branch+`"}`)
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42,"branch":"`+branch+`"}`)
assert.Equal(t, true, res["merged"], "resume must merge when CI is green")
assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7")
@@ -204,6 +219,138 @@ 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"}]`
shipArgs = `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x","issue":42}`
)
func TestTBDShip_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"},{"id":3,"name":"size/s"}]`, 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 risk value does not merge", `[{"id":1,"name":"risk/lowish"}]`, false, "unknown"},
{"an empty risk value does not merge", `[{"id":1,"name":"risk/"}]`, false, "unknown"},
{"labels are case-sensitive: RISK/LOW is not risk/low", `[{"id":1,"name":"RISK/LOW"}]`, false, "no risk/"},
{"a label that merely contains risk/low does not count", `[{"id":1,"name":"not-risk/low"}]`, false, "no risk/"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, tc.labels, http.StatusOK, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, shipArgs)
assert.Equal(t, tc.wantMerged, res["merged"])
assert.Equal(t, tc.wantMerged, merged.Load(), "the merge endpoint is called iff the tier allows it")
assert.Equal(t, "success", res["ci_status"], "CI was green: the tier alone decided")
assert.Equal(t, float64(7), res["pr_number"], "the PR is opened either way")
if !tc.wantMerged {
assert.Contains(t, res["reason"], tc.wantInReason)
}
})
}
}
// No issue named → nothing classifies the change → no merge, even on green CI.
func TestTBDShip_NoIssue_Unclassified_NoMerge(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFake(t, shipGreen, &merged, &deleted)
defer srv.Close()
res := callShip(t, srv.URL, `{"owner":"mathias","repo":"myrepo","path":"docs/x.md","content":"hi","message":"add x"}`)
assert.Equal(t, false, res["merged"])
assert.Contains(t, res["reason"], "issue")
assert.False(t, merged.Load(), "an unclassified change must not merge")
assert.Equal(t, float64(7), res["pr_number"])
}
// The issue lookup failing is not a reason to merge: fail closed, PR left open.
func TestTBDShip_IssueLookupFails_FailsClosed(t *testing.T) {
for _, status := range []int{http.StatusInternalServerError, http.StatusNotFound, http.StatusForbidden} {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, status, &merged, &deleted)
res := callShip(t, srv.URL, shipArgs)
srv.Close()
assert.Equal(t, false, res["merged"], "status %d", status)
assert.NotEmpty(t, res["reason"], "status %d", status)
assert.False(t, merged.Load(), "status %d: an unreadable issue must not merge", status)
}
}
// risk/low is necessary, not sufficient: red CI still blocks it, and a review-protected base still blocks it.
func TestTBDShip_RiskLow_DoesNotOverrideTheOtherGates(t *testing.T) {
var merged, deleted atomic.Bool
red := shipFakeFull(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, 0, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, &merged, &deleted)
defer red.Close()
assert.Equal(t, false, callShip(t, red.URL, shipArgs)["merged"])
assert.False(t, merged.Load())
prot := shipFakeFull(t, shipGreen, 1, http.StatusOK, `[{"id":1,"name":"risk/low"}]`, http.StatusOK, &merged, &deleted)
defer prot.Close()
assert.Equal(t, false, callShip(t, prot.URL, shipArgs)["merged"])
assert.False(t, merged.Load())
}
// The decision is visible to the caller.
func TestTBDShip_ReportsTheRiskTier(t *testing.T) {
var merged, deleted atomic.Bool
srv := shipFakeFull(t, shipGreen, 0, http.StatusOK, `[{"id":1,"name":"risk/medium"}]`, http.StatusOK, &merged, &deleted)
defer srv.Close()
assert.Equal(t, "medium", callShip(t, srv.URL, shipArgs)["risk"])
}
func TestTBDShip_AllowlistRejects(t *testing.T) {
tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))
+148 -2
View File
@@ -1,10 +1,14 @@
package tools
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"reflect"
"regexp"
"strings"
"unicode"
"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
"git.d-ma.be/mathias/gitea-mcp/internal/registry"
@@ -22,12 +26,154 @@ func parseArgs(raw json.RawMessage, dst any) error {
if err := json.Unmarshal([]byte("{}"), dst); err != nil {
return err
}
} else if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
return err
} else {
// Reject ambiguity BEFORE alias normalisation: re-marshalling through a
// map would otherwise silently collapse exact duplicates (#65).
if err := rejectAmbiguousKeys(raw, dst); err != nil {
return err
}
if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
return err
}
}
return validateRequiredIdentifiers(dst)
}
// rejectAmbiguousKeys refuses input whose meaning depends on encoding/json's
// case-insensitive, last-key-wins matching (#65). A client-side check that reads
// `branch` must see the value the server acts on, so: no object at any depth may
// carry two keys equal under strings.EqualFold (exact duplicates included), and
// no top-level key may differ from a field of dst only in case.
func rejectAmbiguousKeys(raw json.RawMessage, dst any) error {
dec := json.NewDecoder(bytes.NewReader(raw))
// UseNumber: without it Token() fails on a number outside float64 range,
// which json.Unmarshal ignores in an unknown key — a decoy that would end
// the scan before the duplicate (PR #66 review).
dec.UseNumber()
if err := scanJSONValue(dec); err != nil {
if errors.Is(err, gitea.ErrValidation) {
return err
}
// Fail closed: input the scanner cannot read is input it cannot vouch for.
return fmt.Errorf("arguments are not valid JSON: %v: %w", err, gitea.ErrValidation)
}
var top map[string]json.RawMessage
if err := json.Unmarshal(raw, &top); err != nil {
return nil // not an object; nothing to compare against the schema
}
fields := jsonFieldNames(dst)
for k := range top {
for _, f := range fields {
if k != f && strings.EqualFold(k, f) {
return fmt.Errorf("argument %q does not match field %q exactly (keys are case-sensitive): %w", k, f, gitea.ErrValidation)
}
}
}
return nil
}
// scanJSONValue consumes one JSON value from dec, failing with ErrValidation if
// any object within it has two keys that are equal under strings.EqualFold.
func scanJSONValue(dec *json.Decoder) error {
tok, err := dec.Token()
if err != nil {
return err
}
d, ok := tok.(json.Delim)
if !ok {
return nil
}
switch d {
case '{':
seen := map[string]string{}
for dec.More() {
kt, err := dec.Token()
if err != nil {
return err
}
k, _ := kt.(string)
fk := foldKey(k)
if prev, dup := seen[fk]; dup {
return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation)
}
seen[fk] = k
if err := scanJSONValue(dec); err != nil {
return err
}
}
case '[':
for dec.More() {
if err := scanJSONValue(dec); err != nil {
return err
}
}
}
_, err = dec.Token() // closing delimiter
return err
}
// foldKey maps every rune to the smallest member of its unicode.SimpleFold
// orbit, so foldKey(a) == foldKey(b) exactly when strings.EqualFold(a, b) —
// the folding encoding/json uses (long s == s, Kelvin sign == k). A map keyed
// on it keeps the duplicate scan linear (PR #66 review: a pairwise EqualFold
// loop cost 7s of CPU on ~90k keys).
func foldKey(s string) string {
var b strings.Builder
for _, r := range s {
min := r
for f := unicode.SimpleFold(r); f != r; f = unicode.SimpleFold(f) {
if f < min {
min = f
}
}
b.WriteRune(min)
}
return b.String()
}
// jsonFieldNames returns the json tag names of dst's top-level struct fields.
func jsonFieldNames(dst any) []string {
v := reflect.ValueOf(dst)
if v.Kind() != reflect.Pointer || v.IsNil() || v.Elem().Kind() != reflect.Struct {
return nil
}
t := v.Elem().Type()
names := make([]string, 0, t.NumField())
for i := 0; i < t.NumField(); i++ {
name, _, _ := strings.Cut(t.Field(i).Tag.Get("json"), ",")
if name == "" || name == "-" {
name = t.Field(i).Name
}
names = append(names, name)
}
return names
}
// branchNameRe is the shape the skills mcp-destructive-guard hook enforces.
var branchNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`)
// ValidateBranchName rejects branch names that Gitea would turn into a ref
// other than refs/heads/<name> as the caller meant it. `refs/heads/main` in
// particular creates refs/heads/refs/heads/main, after which every API write to
// main fails with "matches more than one" until the ref is deleted by hand
// (#65). Beyond the shape regexp it applies git's own check-ref-format rules
// that the regexp admits.
func ValidateBranchName(b string) error {
bad := !branchNameRe.MatchString(b) ||
strings.HasPrefix(b, "refs/") ||
b == "HEAD" ||
strings.Contains(b, "..") ||
strings.Contains(b, "//") ||
strings.Contains(b, "/.") ||
strings.HasSuffix(b, "/") ||
strings.HasSuffix(b, ".") ||
strings.HasSuffix(b, ".lock")
if bad {
return fmt.Errorf("invalid branch name %q (want a short name like feat/x, no refs/ prefix): %w", b, gitea.ErrValidation)
}
return nil
}
// validateRequiredIdentifiers rejects an empty repo identifier at the tool layer
// before it reaches the client, where it would build a trailing-empty path
// segment (`/api/v1/repos/{owner}/`) and leak gitea's bare 404 (#37). `repo` and