Supersede older heads when an announcement or edit fills a delivery made from its verdict, and say truthfully what fills it (issue 1726)

modules/mesh-delivery-gate/.gitignore

@@ -0,0 +1 @@
+/cmd/mesh-delivery-gate/mesh-delivery-gate

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go

@@ -368,19 +368,7 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 			}
 			// A delivery made from its verdict before its head was heard (issue 1726) holds none of what an announcement
 			// carries: filled from this one, so its base is known and the operator is asked.
-			if d.Base == "" && p.Base != "" {
-				d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL
-				d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed
-				d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid
-				d.Issues = IssuesIn(p.Title + "\n" + p.Body)
-				d.Number = p.Number
-				h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base)
-				h.keep(d)
-				if d.Group == "" && !d.State.Final() {
-					h.joinGroup(d)
-				}
-				h.Poke()
-			}
+			h.fillFromAnnouncement(d, p)
 			// The review record, corrected in the description without a push, is read again (ADR 0421 §5).
 			if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) {
 				d.ReviewedBy, d.ReviewedAt = by, at
@@ -413,18 +401,7 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 			h.leaveGroup(old)
 		}
 	}
-	replaced := ""
-	for _, old := range h.deliveries {
-		if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) &&
-			!old.State.Final() && old.MergedAs == "" {
-			old.NewerHead = p.HeadSHA
-			h.settleAndKeep(old)
-			if old.Group != "" {
-				h.leaveGroup(old)
-				replaced = old.ID
-			}
-		}
-	}
+	replaced := h.supersedeOlder(p)
 	d, t, err := h.freshAnnouncement(p, now, startedOver)
 	if err != nil {
 		h.refuse(err)
@@ -444,6 +421,49 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 	return isNew
 }
 
+// supersedeOlder ends the older heads of p's pull request: each is told its newer head, and one in a group leaves it. It
+// returns the id of the one that left a group, whose place the new head takes (takePlaceOf). Under the lock.
+func (h *Holder) supersedeOlder(p PullEvent) (replaced string) {
+	for _, old := range h.deliveries {
+		if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) &&
+			!old.State.Final() && old.MergedAs == "" {
+			old.NewerHead = p.HeadSHA
+			h.settleAndKeep(old)
+			if old.Group != "" {
+				h.leaveGroup(old)
+				replaced = old.ID
+			}
+		}
+	}
+	return replaced
+}
+
+// fillFromAnnouncement fills a delivery made from its check's verdict before its pull request's head was heard (issue
+// 1726: no base, branch, title, URLs or review record) from an announcement or an edit of that pull request, and then
+// does what a first announcement does: the older heads of the pull request are superseded, and the delivery takes
+// their place in a group or joins the group its branch makes. Nothing for a delivery that holds its base. Under the lock.
+func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) {
+	if d.Base != "" || p.Base == "" || d.State.Final() || d.MergedAs != "" {
+		return
+	}
+	d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL
+	d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed
+	d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid
+	d.Issues, d.After = IssuesIn(p.Title+"\n"+p.Body), AfterIn(p.Body)
+	d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body)
+	if p.BaseSHA != "" {
+		d.BaseSHA = p.BaseSHA
+	}
+	d.Number = p.Number
+	h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base)
+	h.keep(d)
+	replaced := h.supersedeOlder(p)
+	if d.Group == "" && !h.takePlaceOf(replaced, d) {
+		h.joinGroup(d)
+	}
+	h.Poke()
+}
+
 // PullEdited takes a pull request's edit (novox/hq issue 1079): its description is read again for the review record, the
 // `after:` lines and the issues it names, exactly as an announcement reads them. An Approve ask still unanswered whose words
 // the new record changes is withdrawn and asked again with it read, without a word on the pull request: nothing was
@@ -473,6 +493,9 @@ func (h *Holder) PullEdited(p PullEvent) {
 		h.pullUpdated(p)
 		return
 	}
+	if d != nil && d.Number == p.Number {
+		h.fillFromAnnouncement(d, p) // a delivery made from its verdict, its head never heard (issue 1726)
+	}
 	if p.Body == "" {
 		return
 	}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go

@@ -81,3 +81,46 @@ func TestADeliveryWithoutItsHeadPastItsBoundIsStalled(t *testing.T) {
 		t.Fatalf("not stalled past its bound: %+v", w.h.Stalled())
 	}
 }
+
+// A head asked, then a push whose verdict is heard before its head: the announcement of the new head supersedes the old
+// one exactly as a first announcement does, so one Approve stands, for the new head.
+func TestAnAnnouncementFillingAVerdictFirstHeadSupersedesTheOlderHeadAndItsAsk(t *testing.T) {
+	w := newWorld(t)
+	w.op.auto = false
+	a, b := IDOf("novox/app", readyHead), IDOf("novox/app", pushed)
+	w.h.PullUpdated(pr("novox/app", 419, readyHead, "fix/x", "src/a.go"))
+	w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("head A: %d asks open; wanted 1", n)
+	}
+	w.h.Checked(judgedAt("novox/app", 419, pushed, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	w.h.PullUpdated(pr("novox/app", 419, pushed, "fix/x", "src/a.go"))
+	w.settleAll()
+	if got := w.delivery(a); got.NewerHead != pushed || got.State != Superseded {
+		t.Fatalf("head A is %s with newer head %q; wanted superseded by %s", got.State, got.NewerHead, pushed)
+	}
+	if got := w.delivery(b); got.Base != "main" || got.Branch != "fix/x" {
+		t.Fatalf("head B not filled: %+v", got)
+	}
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("%d asks open; wanted one, for the new head", n)
+	}
+}
+
+// A description edit is a natural trigger too: it fills a delivery made from its verdict.
+func TestAnEditOfThePullRequestFillsADeliveryMadeFromItsVerdict(t *testing.T) {
+	w, id := verdictFirst(t)
+	p := pr("novox/app", 419, readyHead, "fix/x")
+	p.Title, p.Body = "Fix x", verdictFirstBody
+	w.h.PullEdited(p)
+	w.settleAll()
+	d := w.delivery(id)
+	if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.ReviewedBy != "reviewer@shanks" {
+		t.Fatalf("the edit did not fill the delivery: %+v", d)
+	}
+	if n := len(w.op.open()); n != 1 {
+		t.Fatalf("the operator was asked %d times; wanted once", n)
+	}
+}

modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go

@@ -389,7 +389,7 @@ func (h *Holder) readyLines(now time.Time) []StalledLine {
 		switch {
 		case d.Base == "" && d.Number != 0 && now.Sub(r.Seen) >= headHeardBound:
 			out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(),
-				Bound: headHeardBound.String(), H2: "none: the forge's announcement of the pull request's head, heard again at each look at the open pull requests — the operator's",
+				Bound: headHeardBound.String(), H2: "none: a later announcement of the pull request's head (the merge tool announces it again, the forge's announcer restarts, or a new push) or an edit of its description; a recheck does not fill it — the operator's",
 				Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)})
 		case r.Failed != "" && d.State == Rejected:
 			out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(),