mesh-issues: read merged from the pull request, not the state, so a closed one is withdrawn (hq issue 483)

mesh-delivery says stopped for a pull request closed unmerged too; read as
merged, it was said stuck to the operator for good. Cite ADR 0284 for the
worker rules, which the comments gave as 0283.

modules/mesh-issues/cmd/mesh-issues/delivery.go

@@ -44,13 +44,36 @@ func reached(state string) bool {
 	return state == "delivered" || state == "covered" || state == "retired"
 }
 
-// afterMerge says whether a state is one a delivery reaches only once merged.
-func afterMerge(state string) bool {
-	switch state {
-	case "published", "delivering", "held", "delivered", "failed", "stopped", "covered", "retired":
-		return true
+// Whether a pull request is merged is what the delivery says of the pull request itself — `merged_as` on a
+// transition, the pull request's `merged` in checks — never read from the state: `stopped` is also what a pull
+// request closed without merging becomes (mesh-delivery's `closed` event), and read as merged it was said stuck to
+// the operator for good (novox/hq issue 483).
+
+// withdrawn says whether a fix is a pull request closed without merging: no fix at all, neither awaiting delivery,
+// delivered, nor open.
+func withdrawn(f *Fix) bool { return f != nil && f.State == "stopped" && !f.Merged }
+
+// liveFixes is an issue's fixes less those withdrawn: what its delivery, readiness to verify, the stuck sweep and a
+// stale claim are measured on. fixPullRequests still names the withdrawn ones, so a pull request reopened is heard.
+func liveFixes(is *Issue) []string {
+	var out []string
+	for _, pr := range fixPullRequests(is) {
+		if !withdrawn(fixOf(is, pr)) {
+			out = append(out, pr)
+		}
 	}
-	return false
+	return out
+}
+
+// withdrawnFixes is the pull requests of an issue closed without merging.
+func withdrawnFixes(is *Issue) []string {
+	var out []string
+	for _, pr := range fixPullRequests(is) {
+		if withdrawn(fixOf(is, pr)) {
+			out = append(out, pr)
+		}
+	}
+	return out
 }
 
 func fixOf(is *Issue, pr string) *Fix {
@@ -64,7 +87,7 @@ func fixOf(is *Issue, pr string) *Fix {
 
 // allDelivered says whether an issue names a pull request and every one is delivered.
 func allDelivered(is *Issue) bool {
-	prs := fixPullRequests(is)
+	prs := liveFixes(is)
 	if len(prs) == 0 {
 		return false
 	}
@@ -79,8 +102,11 @@ func allDelivered(is *Issue) bool {
 // DeliveryLine is the derived delivery state of an issue's fix, in words; empty when fixed-by names no pull
 // request.
 func DeliveryLine(is *Issue) string {
-	prs := fixPullRequests(is)
+	prs := liveFixes(is)
 	if len(prs) == 0 {
+		if w := withdrawnFixes(is); len(w) > 0 {
+			return "fix withdrawn: " + strings.Join(w, ", ") + " closed without merging"
+		}
 		return ""
 	}
 	var held, failed, merged, open, unheard []string
@@ -182,7 +208,7 @@ func (t *Tracker) afterFix(n uint64) error {
 	}
 	if allDelivered(is) {
 		if unresolved(is) {
-			prs := fixPullRequests(is)
+			prs := liveFixes(is)
 			sort.Strings(prs)
 			if strings.Join(is.VerifyFor, ",") != strings.Join(prs, ",") {
 				// The note first, kept once by its words; then the claim; then the event. A failure before the claim
@@ -245,7 +271,7 @@ func stillStuck(is *Issue) bool {
 	if is.Status == StatusWontfix {
 		return false
 	}
-	named := fixPullRequests(is)
+	named := liveFixes(is)
 	for _, f := range is.Fixes {
 		if f.StuckSaid && contains(named, f.PullRequest) && !contains(is.Delivered, f.PullRequest) {
 			return true
@@ -296,7 +322,7 @@ func (t *Tracker) OnTransition(e stdio.Envelope) error {
 	if at.IsZero() {
 		at = t.now()
 	}
-	merged := body.MergedAs != "" || afterMerge(body.To)
+	merged := body.MergedAs != ""
 	changed, err := t.recordFix(prKey(body.Repository, body.Number), body.To, body.Why, merged, at, 0)
 	if err != nil {
 		return err
@@ -315,6 +341,8 @@ type Delivery struct {
 	Why   string
 	// Since is when the delivery entered its state, as it says; zero when it does not.
 	Since time.Time
+	// Merged is the pull request's own merged flag, as the forge says it.
+	Merged bool
 }
 
 // DeliveryState asks mesh-delivery for one pull request's delivery.
@@ -333,11 +361,14 @@ func busDeliveryState(owner string) DeliveryState {
 				Waits string    `json:"waits"`
 				Since time.Time `json:"since"`
 			} `json:"delivery"`
+			Pull struct {
+				Merged bool `json:"merged"`
+			} `json:"pull"`
 		}
 		if err := json.Unmarshal(raw, &out); err != nil {
 			return Delivery{}, fmt.Errorf("mesh-delivery's checks answered no delivery: %.200s", raw)
 		}
-		return Delivery{State: out.Delivery.State, Why: out.Delivery.Waits, Since: out.Delivery.Since.UTC()}, nil
+		return Delivery{State: out.Delivery.State, Why: out.Delivery.Waits, Since: out.Delivery.Since.UTC(), Merged: out.Pull.Merged}, nil
 	}
 }
 
@@ -386,7 +417,7 @@ func (t *Tracker) OnMoved(e stdio.Envelope) error {
 			if since.IsZero() {
 				since = t.now()
 			}
-			if _, err := t.recordFix(pr, state, d.Why, afterMerge(state), since, body.Number); err != nil {
+			if _, err := t.recordFix(pr, state, d.Why, d.Merged, since, body.Number); err != nil {
 				return err
 			}
 		}
@@ -452,7 +483,7 @@ func (t *Tracker) Sweep() (int, error) {
 		if err := t.clearStuck(is); err != nil {
 			t.Logf("[mesh-issues] the stuck fix of %d could not be cleared on the operator's channel: %v", n, err)
 		}
-		named := fixPullRequests(is)
+		named := liveFixes(is)
 		for _, f := range is.Fixes {
 			if !contains(named, f.PullRequest) || contains(is.Delivered, f.PullRequest) || !f.Merged || f.MergedAt == nil ||
 				reached(f.State) || f.StuckSaid || now.Sub(*f.MergedAt) < bound {