feat(tbd_ship): gate auto-merge on the risk tier; unclassified is not low (#63) (#73)

Co-authored-by: Mathias <mthbqv@gmail.com>

internal/tools/tbd_ship_test.go

@@ -26,11 +26,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) + `}`))
@@ -83,7 +93,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 +108,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 +123,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 +138,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 +152,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 +170,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 +208,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 +216,103 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
 	assert.True(t, merged.Load())
 }
 
+// ---- 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"}`))