feat(tbd_ship): gate auto-merge on the risk tier; unclassified is not low (#63) #73
@@ -125,6 +125,13 @@ by area:
|
|||||||
| Search / Templates | code_search, create_project_from_template |
|
| Search / Templates | code_search, create_project_from_template |
|
||||||
| Composite | tbd_ship (trunk-based ship helper) |
|
| 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).
|
||||||
|
|
||||||
Shared tool-layer hygiene (`internal/tools/tool.go`):
|
Shared tool-layer hygiene (`internal/tools/tool.go`):
|
||||||
- **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so
|
- **Alias normalization** — `repo`↔`name` and `number`↔`index` are reconciled so
|
||||||
a tool works whichever spelling a caller sends (an explicit canonical wins).
|
a tool works whichever spelling a caller sends (an explicit canonical wins).
|
||||||
|
|||||||
@@ -37,7 +37,7 @@ const (
|
|||||||
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
||||||
return registry.ToolDescriptor{
|
return registry.ToolDescriptor{
|
||||||
Name: "tbd_ship",
|
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(`{
|
InputSchema: json.RawMessage(`{
|
||||||
"type":"object",
|
"type":"object",
|
||||||
"properties":{
|
"properties":{
|
||||||
@@ -50,6 +50,7 @@ func (t *TBDShip) Descriptor() registry.ToolDescriptor {
|
|||||||
"branch":{"type":"string","description":"Short-lived branch name. Default derived: tbd/<slug>-<hash>."},
|
"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_title":{"type":"string","description":"PR title. Default: the commit message."},
|
||||||
"pr_body":{"type":"string"},
|
"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."},
|
"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)."}
|
"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"`
|
Branch string `json:"branch"`
|
||||||
PRTitle string `json:"pr_title"`
|
PRTitle string `json:"pr_title"`
|
||||||
PRBody string `json:"pr_body"`
|
PRBody string `json:"pr_body"`
|
||||||
|
Issue int `json:"issue"`
|
||||||
DeleteBranch *bool `json:"delete_branch"`
|
DeleteBranch *bool `json:"delete_branch"`
|
||||||
CITimeoutSec int `json:"ci_timeout_seconds"`
|
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 opened, 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)
|
||||||
|
case riskAmbiguous:
|
||||||
|
return "the issue carries more than one risk/ label — ambiguous classification; PR opened, not merged"
|
||||||
|
case riskUnknown:
|
||||||
|
return "the issue carries an unknown risk/ value — PR opened, not merged"
|
||||||
|
case riskUnreadable:
|
||||||
|
return "could not read the issue's labels — fail-closed; PR opened, 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.
|
// shipGate is the decision produced by evaluateShipGate.
|
||||||
type shipGate struct {
|
type shipGate struct {
|
||||||
merge bool
|
merge bool
|
||||||
@@ -101,9 +168,10 @@ func classifyCI(runs []gitea.WorkflowRun) string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// evaluateShipGate is the load-bearing safety of tbd_ship. It returns merge=true
|
// 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
|
// only when CI is fully green AND the base branch is not review-protected AND the
|
||||||
// other state fails closed to PR-only with an explanatory reason (#40).
|
// change is classified risk/low. Every other state fails closed to PR-only with an
|
||||||
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) shipGate {
|
// explanatory reason (#40, gitea-mcp#63).
|
||||||
|
func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection, risk riskTier) shipGate {
|
||||||
ci := classifyCI(runs)
|
ci := classifyCI(runs)
|
||||||
|
|
||||||
// Review-protected base can't be auto-merged regardless of CI.
|
// Review-protected base can't be auto-merged regardless of CI.
|
||||||
@@ -112,6 +180,11 @@ func evaluateShipGate(runs []gitea.WorkflowRun, bp *gitea.BranchProtection) ship
|
|||||||
"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 opened 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 {
|
switch ci {
|
||||||
case "success":
|
case "success":
|
||||||
return shipGate{true, ci, ""}
|
return shipGate{true, ci, ""}
|
||||||
@@ -230,7 +303,8 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
|
|
||||||
// CI gate on the PR head commit.
|
// CI gate on the PR head commit.
|
||||||
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
|
runs := t.pollRuns(ctx, args.Owner, args.Repo, pr.Head.Sha, args.CITimeoutSec)
|
||||||
gate := evaluateShipGate(runs, bp)
|
risk := t.riskTierOf(ctx, args.Owner, args.Repo, args.Issue)
|
||||||
|
gate := evaluateShipGate(runs, bp, risk)
|
||||||
|
|
||||||
result := map[string]any{
|
result := map[string]any{
|
||||||
"merged": false,
|
"merged": false,
|
||||||
@@ -238,6 +312,7 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
"pr_url": pr.HTMLURL,
|
"pr_url": pr.HTMLURL,
|
||||||
"branch": branch,
|
"branch": branch,
|
||||||
"ci_status": gate.ciStatus,
|
"ci_status": gate.ciStatus,
|
||||||
|
"risk": string(risk),
|
||||||
}
|
}
|
||||||
if !gate.merge {
|
if !gate.merge {
|
||||||
result["reason"] = gate.reason
|
result["reason"] = gate.reason
|
||||||
@@ -261,6 +336,20 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
|
|||||||
return textOK(result)
|
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 (t *TBDShip) riskTierOf(ctx context.Context, owner, repo string, issue int) riskTier {
|
||||||
|
if issue <= 0 {
|
||||||
|
return riskNone
|
||||||
|
}
|
||||||
|
iss, err := t.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
|
// 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
|
// them to reach a terminal state. timeoutSec == 0 is a single snapshot (no
|
||||||
// sleep). A listing error yields no runs → the gate fails closed.
|
// sleep). A listing error yields no runs → the gate fails closed.
|
||||||
|
|||||||
@@ -20,22 +20,31 @@ func TestEvaluateShipGate(t *testing.T) {
|
|||||||
name string
|
name string
|
||||||
runs []gitea.WorkflowRun
|
runs []gitea.WorkflowRun
|
||||||
bp *gitea.BranchProtection
|
bp *gitea.BranchProtection
|
||||||
|
risk riskTier
|
||||||
wantMerge bool
|
wantMerge bool
|
||||||
wantCI string
|
wantCI string
|
||||||
}{
|
}{
|
||||||
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, true, "success"},
|
{"all green → merge", []gitea.WorkflowRun{run("completed", "success")}, open, riskLow, true, "success"},
|
||||||
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, false, "pending"},
|
{"in_progress → no merge", []gitea.WorkflowRun{run("in_progress", "")}, open, riskLow, false, "pending"},
|
||||||
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, false, "pending"},
|
{"queued → no merge", []gitea.WorkflowRun{run("queued", "")}, open, riskLow, false, "pending"},
|
||||||
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, false, "failed"},
|
{"failed → no merge", []gitea.WorkflowRun{run("completed", "failure")}, open, riskLow, false, "failed"},
|
||||||
{"cancelled → no merge (fail-closed)", []gitea.WorkflowRun{run("completed", "cancelled")}, open, 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, false, "none"},
|
{"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, false, "pending"},
|
{"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, false, "success"},
|
{"green but protected → no merge", []gitea.WorkflowRun{run("completed", "success")}, protected, riskLow, false, "success"},
|
||||||
{"no runs + protected → no merge", nil, protected, false, "none"},
|
{"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 {
|
for _, tc := range tests {
|
||||||
t.Run(tc.name, func(t *testing.T) {
|
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 {
|
if g.merge != tc.wantMerge {
|
||||||
t.Errorf("merge = %v, want %v (reason: %q)", g.merge, tc.wantMerge, g.reason)
|
t.Errorf("merge = %v, want %v (reason: %q)", g.merge, tc.wantMerge, g.reason)
|
||||||
}
|
}
|
||||||
@@ -51,3 +60,41 @@ 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)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -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
|
// shipFakeOpts adds protection (requiredApprovals>0 → protected) and a merge
|
||||||
// status code, so tests can drive the review-protected and conflict paths.
|
// 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 {
|
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()
|
t.Helper()
|
||||||
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
p := r.URL.Path
|
p := r.URL.Path
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
switch {
|
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/"):
|
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
|
||||||
if requiredApprovals > 0 {
|
if requiredApprovals > 0 {
|
||||||
_, _ = w.Write([]byte(`{"required_approvals":` + itoa(requiredApprovals) + `}`))
|
_, _ = 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)
|
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, true, res["merged"])
|
||||||
assert.Equal(t, "success", res["ci_status"])
|
assert.Equal(t, "success", res["ci_status"])
|
||||||
@@ -98,7 +108,7 @@ func TestTBDShip_NoCI_FailsClosed(t *testing.T) {
|
|||||||
srv := shipFake(t, `[]`, &merged, &deleted)
|
srv := shipFake(t, `[]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.Equal(t, "none", res["ci_status"])
|
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)
|
srv := shipFake(t, `[{"id":1,"status":"completed","conclusion":"failure","head_sha":"abc"}]`, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.Equal(t, "failed", res["ci_status"])
|
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)
|
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 1, http.StatusOK, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.NotEmpty(t, res["reason"])
|
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)
|
srv := shipFakeOpts(t, `[{"id":1,"status":"completed","conclusion":"success","head_sha":"abc"}]`, 0, http.StatusConflict, &merged, &deleted)
|
||||||
defer srv.Close()
|
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, false, res["merged"])
|
||||||
assert.NotEmpty(t, res["reason"])
|
assert.NotEmpty(t, res["reason"])
|
||||||
@@ -160,6 +170,8 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
|
|||||||
p := r.URL.Path
|
p := r.URL.Path
|
||||||
w.Header().Set("Content-Type", "application/json")
|
w.Header().Set("Content-Type", "application/json")
|
||||||
switch {
|
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/"):
|
case r.Method == http.MethodGet && strings.Contains(p, "/branch_protections/"):
|
||||||
w.WriteHeader(http.StatusNotFound)
|
w.WriteHeader(http.StatusNotFound)
|
||||||
_, _ = w.Write([]byte(`{"message":"not found"}`))
|
_, _ = w.Write([]byte(`{"message":"not found"}`))
|
||||||
@@ -196,7 +208,7 @@ func TestTBDShip_Resume_ExistingBranchAndPR(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
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, true, res["merged"], "resume must merge when CI is green")
|
||||||
assert.Equal(t, float64(7), res["pr_number"], "must reuse the existing PR #7")
|
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())
|
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) {
|
func TestTBDShip_AllowlistRejects(t *testing.T) {
|
||||||
tool := tools.NewTBDShip(gitea.NewClient("http://unused", ""), allowlist.New([]string{"mathias"}))
|
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"}`))
|
_, err := tool.Call(context.Background(), json.RawMessage(`{"owner":"evil","repo":"r","path":"p","content":"c","message":"m"}`))
|
||||||
|
|||||||
Reference in New Issue
Block a user