From 90f7f87c2ac180d3087659c5d229849b76f6e42f Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 1 Nov 2026 18:06:27 +0100 Subject: [PATCH] Say a failed check first in the merge ask's headline, since a merge is not gated on the check and the headline is the phone's only warning (issue 1166) Claude-Session: https://claude.ai/code/session_01NcbgfJ9Gum47zkFKWRaovL --- .../cmd/mesh-delivery-gate/approval.go | 14 ++++++++++++-- .../cmd/mesh-delivery-gate/issue1348_test.go | 12 ++++++++++++ .../cmd/mesh-delivery-gate/ready_test.go | 7 ++++++- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go index c635afc6..77d9805f 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -169,6 +169,16 @@ func approveCaveat(c checkSaid) string { return "" } +// mergeHeadline opens the ask's headline: "Merge into main: ", and "Check failed - merge into main: " once the check failed, +// since a merge is not gated on the check and the headline is the phone's only warning (issue 1166). A passed or still +// running check keeps the plain headline. The prefix leads, so phone's cut of a long headline never drops it. +func mergeHeadline(c checkSaid) string { + if c.Key == checkSaidFail { + return "Check failed \u2014 merge into main: " + } + return "Merge into main: " +} + // composeMergeAsk is the ask for a delivery, or for a group's members together. // check is the state of the check at the heads it covers (checkAt): the ask says "passed its check" only when it did, and // otherwise that Approve merges before the check ends, or although it failed (issue 1733). @@ -193,7 +203,7 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), words, touches)) } if group == "" { - a.Headline = phone("Merge into main: "+pullSaid(d)+" "+d.Title, asks.HeadlineLength) + a.Headline = phone(mergeHeadline(check)+pullSaid(d)+" "+d.Title, asks.HeadlineLength) summary := fmt.Sprintf("%s %s%s. Approve merges it into main at %s%s; Decline leaves it unmerged.", pullSaid(d), d.Title, checkClause(check, " "), shortOf(d.Commit, 12), approveCaveat(check)) if unreviewed != "" { @@ -210,7 +220,7 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, for _, m := range members { names = append(names, pullSaid(m)) } - a.Headline = phone("Merge into main: group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength) + a.Headline = phone(mergeHeadline(check)+"group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength) caveat := approveCaveat(check) if caveat != "" { caveat = "," + caveat diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go index 4555603b..98a9cac5 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1348_test.go @@ -3,6 +3,7 @@ package main import ( "strings" "testing" + "time" "git.novox.be/novox/mesh-sdk/go/asks" sdk "git.novox.be/novox/mesh-sdk/go/delivery" @@ -42,6 +43,17 @@ func TestAReadyGroupWritesOneAskKeyWithTodaysWordsAndCallsNoRouter(t *testing.T) !strings.Contains(ask.Ask.Details, "mesh-catalog#2") || len(ask.Ask.Options) != 2 || ask.Ask.Who != asks.Operator { t.Fatalf("the ask is not today's merge ask: %+v", ask.Ask) } + // A group's headline says a failed check first (issue 1166); a passed or a running one keeps the plain words. + members := []*Delivery{{Repository: "novox/mesh-controller", Number: 1}, {Repository: "novox/mesh-catalog", Number: 2}} + for _, c := range []struct{ key, head string }{ + {checkSaidFail, "Check failed \u2014 merge into main: group feat"}, + {checkSaidPass, "Merge into main: group feat"}, + {"running", "Merge into main: group feat"}, + } { + if h := composeMergeAsk("id", members[0], "feat/g", members, nil, checkSaid{Key: c.key, Words: "w"}, time.Time{}).Headline; !strings.HasPrefix(h, c.head) { + t.Fatalf("a %s check gives the headline %q, wanted it to start %q", c.key, h, c.head) + } + } // The act the Approve binds is the one the teller checks and the controller verifies: this ask's router id, the option, and // the heads as `owner/repo@` to the whole commit. approve, _ := ask.Ask.Option(OptionApprove) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go index c8354af9..bfd839a2 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go @@ -80,8 +80,9 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { after time.Duration asks, open int want, not []string + head string // the ask's headline starts with it }{ - {name: "gated, passed", gated: true, state: "pass", asks: 1, + {name: "gated, passed", gated: true, state: "pass", asks: 1, head: "Merge into main: app#7 Fix it", want: []string{"app#7 Fix it passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends", "no verdict yet"}}, {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}}, @@ -98,6 +99,7 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { {name: "ungated, passed, its verdict gone", state: "pass", then: "gone", asks: 1, open: -1, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet"}, not: []string{"anyway"}}, {name: "ungated, failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 1, + head: "Check failed \u2014 merge into main: app#7 Fix it", want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed", "anyway"}}, } { t.Run(row.name, func(t *testing.T) { @@ -143,6 +145,9 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { } return } + if h := v.op.ask(open[0]).Headline; row.head != "" && !strings.HasPrefix(h, row.head) { + t.Fatalf("the headline is %q; wanted it to start %q", h, row.head) + } e := v.op.ask(open[0]).Explanation for _, w := range row.want { if !strings.Contains(e, w) {