Fill a delivery made from its verdict from its pull request's announcement, and say what it waits for (issue 1726)

2026-10-21 16:18:43 +02:00
parent 90ecba4e50
commit dd31d991cd
4 changed files with 123 additions and 0 deletions
@@ -366,6 +366,21 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
 				d.BaseSHA = p.BaseSHA
 				h.keep(d)
 			}
+			// 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()
+			}
 			// 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
@@ -0,0 +1,83 @@
+package main
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// novox/hq issue 1726: a delivery made from a check's verdict that arrived before its pull request's head was heard
+// (transition "its verdict arrived before its head was heard", as mesh-controller#419 at a47476252772 was) holds no base,
+// branch, title, URLs or review record. The ready pass skipped it in silence, so no Approve was ever asked, and the
+// announcement of the same head, heard later, filled none of it.
+
+const verdictFirstBody = "Fixes a bug.\n\nReviewed-by: reviewer@shanks at " + readyHead + "\n"
+
+// verdictFirst is the live shape: the verdict heard, the head not, and the delivery ready.
+func verdictFirst(t *testing.T) (*world, string) {
+	w := newWorld(t)
+	w.op.auto = false
+	id := IDOf("novox/app", readyHead)
+	w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	d := w.delivery(id)
+	if d.State != Ready || d.Base != "" || d.Branch != "" || d.Title != "" || d.HTMLURL != "" {
+		t.Fatalf("not the live shape: %s base %q branch %q title %q", d.State, d.Base, d.Branch, d.Title)
+	}
+	return w, id
+}
+
+func TestADeliveryWithoutItsHeadSaysWhatItWaitsForAndAsksNoOne(t *testing.T) {
+	w, id := verdictFirst(t)
+	if n := len(w.op.open()); n != 0 {
+		t.Fatalf("an ask was made for a delivery with no base: %d", n)
+	}
+	d := w.delivery(id)
+	if d.Ready == nil || !strings.Contains(d.Ready.Waits, "announcement") {
+		t.Fatalf("it does not say what it waits for: %+v", d.Ready)
+	}
+	shown, err := w.h.Show(id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if waits, _ := shown.(map[string]any)["waits"].(string); !strings.Contains(waits, "announcement") {
+		t.Fatalf("show waits %q", waits)
+	}
+}
+
+func TestAnAnnouncementFillsADeliveryMadeFromItsVerdictAndTheOperatorIsAsked(t *testing.T) {
+	w, id := verdictFirst(t)
+	p := pr("novox/app", 419, readyHead, "fix/x", "src/a.go")
+	p.Title, p.Body, p.CloneURL = "Fix x", verdictFirstBody, "https://forge.invalid/novox/app.git"
+	w.h.PullUpdated(p)
+	w.settleAll()
+	d := w.delivery(id)
+	if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.HTMLURL == "" || d.CloneURL == "" ||
+		d.ReviewedBy != "reviewer@shanks" || len(d.Paths) != 1 {
+		t.Fatalf("the announcement 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)
+	}
+	if d.Ready.Waits != "" && !strings.Contains(d.Ready.Waits, "Approve") {
+		t.Fatalf("still waits: %q", d.Ready.Waits)
+	}
+}
+
+func TestADeliveryWithoutItsHeadPastItsBoundIsStalled(t *testing.T) {
+	w, id := verdictFirst(t)
+	if s := w.h.Stalled(); len(s) != 0 {
+		t.Fatalf("stalled before its bound: %+v", s)
+	}
+	w.later(headHeardBound + time.Minute)
+	w.settleAll()
+	var found bool
+	for _, l := range w.h.Stalled() {
+		if l.ID == id && strings.Contains(l.Says, "announcement") {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("not stalled past its bound: %+v", w.h.Stalled())
+	}
+}
@@ -41,6 +41,12 @@ const readyRefusedFor time.Duration = 0
 // groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: none (issue 1115).
 const groupRefusedFor time.Duration = 0

+// waitsForHead is what a delivery waits for when its verdict was heard and its pull request's head was not (issue 1726).
+const waitsForHead = "its pull request's announcement: its base, branch and title are not heard, so what it merges into is not known"
+
+// headHeardBound is how long a delivery may wait for its pull request's announcement before it is said as stalled.
+const headHeardBound = 10 * time.Minute
+
 // groupClosed is whether a group is closed: a member reached the trunk. Under the lock.
 func (h *Holder) groupClosed(id string) bool {
 	g := h.groups[id]
@@ -181,6 +187,18 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 			}
 			continue
 		}
+		if d.Base == "" {
+			// A delivery made from its verdict before its head was heard (issue 1726): its base, branch and title are
+			// unknown, so what it merges into is too. It is neither asked nor passed over in silence: it waits for its
+			// pull request's announcement, which fills it (pullUpdated), and past headHeardBound it is stalled.
+			if r.Waits != waitsForHead {
+				r.Waits = waitsForHead
+				h.Logf("[mesh-delivery] %s waits: %s", d.ID, waitsForHead)
+				h.keep(d)
+			}
+			h.lookAt(r.Seen.Add(headHeardBound), "a delivery whose pull request was never announced")
+			continue
+		}
 		if d.Base != MergeBranch {
 			if r.OtherBase != d.Base {
 				r.OtherBase = d.Base
@@ -369,6 +387,10 @@ func (h *Holder) readyLines(now time.Time) []StalledLine {
 			continue
 		}
 		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",
+				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(),
 				Bound: "0s", H2: "none: a new head, or a recheck that passes — the operator's",
@@ -42,6 +42,9 @@ func (h *Holder) waitsFor(d *Delivery) string {
 		if d.MergedAs != "" {
 			return "a walk that answers its merge"
 		}
+		if d.Base == "" && d.Number != 0 {
+			return waitsForHead
+		}
 		return "its merge"
 	case Rejected:
 		return "a new head, or a recheck"