Say a held group waits for a review and keep each member's status current (issue 1803, round 1)

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

@@ -190,7 +190,7 @@ func (h *Holder) deliveryOf(repository string, number int, commit string) *check
 		c.Check = &v
 	}
 	if g := h.groups[found.Group]; g != nil {
-		state, why := GroupState(g, h.membersOf(g))
+		state, why, _ := h.groupState(g, h.membersOf(g))
 		c.groupSaid = state + ": " + why
 		if g.Check != nil && g.Check.Verdict != "" {
 			c.groupSaid += "; composed check " + g.Check.Verdict + " — " + g.Check.Summary

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

@@ -2217,15 +2217,22 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func(
 		}
 	}
 	// Its state, said when it changes.
-	state, why := GroupState(g, members)
+	state, why, held := h.groupState(g, members)
 	status := groupStatusState(state)
+	if held {
+		status = "pending" // no Approve exists while a member waits for its review (novox/hq ADR 0496 §2)
+	}
 	if state == "delivering" && !(g.Check != nil && (g.Check.Verdict == "pass" || g.Check.Verdict == "warning") &&
 		sameMembers(g.Check.Commits, commitsOf(members)) && g.Check.OutOfDate == "") {
 		// Success there lets the merge tool merge its members: only for a composed check that passed for these heads.
 		status = "pending"
 	}
-	if state != g.Said || status != g.SaidStatus {
-		g.Said, g.SaidStatus = state, status
+	reviewSaid := ""
+	if held {
+		reviewSaid = why
+	}
+	if state != g.Said || status != g.SaidStatus || reviewSaid != g.SaidReview {
+		g.Said, g.SaidStatus, g.SaidReview = state, status, reviewSaid
 		for _, d := range members {
 			g.Owed = append(g.Owed, Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery-group",
 				State: status, Line: clip(fmt.Sprintf("group %s %s: %s", g.ID, state, why), 140),
@@ -2719,6 +2726,29 @@ func GroupState(g *Group, members []*Delivery) (string, string) {
 	return "checking", fmt.Sprintf("%d of %d member(s) ready", count[Ready], n)
 }
 
+// groupState is GroupState, but a group whose members all passed and compose waits for a review while any member the
+// Approve would cover has none at its head: its why names the members (novox/hq ADR 0496 §2, issue 1803), and held
+// says its mesh/delivery-group is pending, not a success. Under the lock.
+func (h *Holder) groupState(g *Group, members []*Delivery) (state, why string, held bool) {
+	state, why = GroupState(g, members)
+	if state != "ready" || g.Closed {
+		return state, why, false
+	}
+	var waiting []string
+	for _, m := range members {
+		if m.MergedAs != "" || m.State.Final() || h.covering(m) != nil {
+			continue
+		}
+		if _, ok := reviewWords(m); !ok {
+			waiting = append(waiting, "at "+shortOf(m.Commit, 12)+" ("+pullSaid(m)+")")
+		}
+	}
+	if len(waiting) == 0 {
+		return state, why, false
+	}
+	return state, waitsForReview + strings.Join(waiting, " and ")[len("at "):], true
+}
+
 // supersededWhy begins why a group whose members all ended is stopped. It is kept short: the forge's status line,
 // "group <id> stopped: <why>", is clipped at 140 characters, and the member's id must fit in it whole.
 const supersededWhy = "superseded, not carried by a later delivery: "

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

@@ -363,6 +363,9 @@ func TestAGroupIsCheckedTogetherAndLetGoOnceEveryMemberIsMerged(t *testing.T) {
 	ctl, cat := IDOf("novox/mesh-controller", ctlHead), IDOf("novox/mesh-catalog", catHead)
 	w.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go"))
 	w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json"))
+	// These heads are not full commit ids, so their description lines cannot name them: the reviews are set as read.
+	w.h.deliveries[ctl].ReviewedBy, w.h.deliveries[ctl].ReviewedAt = "tester@shanks", ctlHead
+	w.h.deliveries[cat].ReviewedBy, w.h.deliveries[cat].ReviewedAt = "tester@shanks", catHead
 	if w.delivery(ctl).Group != "feat/x" || w.delivery(cat).Group != "feat/x" {
 		t.Fatalf("one branch name in two repositories made no group: %q %q", w.delivery(ctl).Group, w.delivery(cat).Group)
 	}

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

@@ -114,46 +114,110 @@ func TestA1803AnApproveAlreadyGivenIsNotAffected(t *testing.T) {
 	}
 }
 
-func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) {
+const gTools, gHt = "novox/mesh-tools", "e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3"
+
+// groupOf is a green group of the given members (repository, number, head, review body), composed green, the operator not
+// answering by himself.
+func groupOf(t *testing.T, members [][4]string) *world {
 	w := newWorld(t)
 	w.op.auto = false
-	a := pr(gCtl, 1, gHc, "feat/g", "internal/a.go")
-	a.Body = "Reviewed-by: reviewer@shanks at " + gHc
-	b := pr(gCat, 2, gHk, "feat/g", "modules/app/module.json")
-	b.Body = "Reviewed-by: pending"
-	w.h.PullUpdated(a)
-	w.h.PullUpdated(b)
-	w.h.Checked(judged(gCtl, 1, gHc, "pass", gTrunk0))
-	w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0))
+	for i, m := range members {
+		p := pr(m[0], i+1, m[2], "feat/g", "src/a.go")
+		p.Body = m[3]
+		w.h.PullUpdated(p)
+	}
+	for i, m := range members {
+		w.h.Checked(judged(m[0], i+1, m[2], "pass", gTrunk0))
+	}
 	w.settleAll()
 	g := w.h.groups["feat/g"]
 	if g == nil || g.Check == nil {
 		t.Fatalf("no composed check was asked: %+v", g)
 	}
-	w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed",
-		Members: []struct {
+	c := CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed"}
+	for _, m := range members {
+		owner, repo, _ := strings.Cut(m[0], "/")
+		c.Members = append(c.Members, struct {
 			Owner  string `json:"owner"`
 			Repo   string `json:"repo"`
 			Commit string `json:"commit"`
 			Trunk  string `json:"trunk,omitempty"`
-		}{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]},
-			{Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gTrunk0[:12]}}})
+		}{Owner: owner, Repo: repo, Commit: m[2], Trunk: gTrunk0[:12]})
+	}
+	w.h.Checked(c)
 	w.settleAll()
+	return w
+}
+
+func reviewedAt(head string) string { return "Reviewed-by: reviewer@shanks at " + head }
+
+// wantGroupSays checks the group's state, its mesh/delivery-group status and the member's mesh/delivery status.
+func (w *world) wantGroupSays(t *testing.T, label, member, head string, want, notWant string) {
+	t.Helper()
+	g := w.h.groups["feat/g"]
+	state, why, held := w.h.groupState(g, w.h.membersOf(g))
+	if state != "ready" || !held || !strings.Contains(why, want) || (notWant != "" && strings.Contains(why, notWant)) {
+		t.Fatalf("%s: group state %s, why %q, held %v; wanted ready waiting with %q and without %q", label, state, why, held, want, notWant)
+	}
+	for _, c := range []string{"mesh/delivery-group", "mesh/delivery"} {
+		said := w.forge.statuses[member+"@"+head+" "+c]
+		if !strings.HasPrefix(said, "pending") || !strings.Contains(said, want) || (notWant != "" && strings.Contains(said, notWant)) {
+			t.Fatalf("%s: %s on %s says %q; wanted pending with %q and without %q", label, c, member, said, want, notWant)
+		}
+	}
+}
+
+func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) {
+	w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}})
 	if w.op.count() != 0 {
 		t.Fatalf("1803/AC5: a group with an unreviewed member was asked: %d asks", w.op.count())
 	}
-	for _, id := range []string{IDOf(gCtl, gHc), IDOf(gCat, gHk)} {
-		d := w.delivery(id)
-		if d.Ready == nil || !strings.Contains(d.Ready.Waits, "waits for a review at "+shortOf(gHk, 12)) ||
-			!strings.Contains(d.Ready.Waits, "mesh-catalog#2") {
-			t.Fatalf("1803/AC5: %s waits %+v; wanted it to name mesh-catalog#2 and its head", id, d.Ready)
-		}
-	}
+	w.wantGroupSays(t, "1803/AC5", gCtl, gHc, "mesh-catalog#2", "mesh-controller#1")
 	e := pr(gCat, 2, gHk, "feat/g")
-	e.Body = "Reviewed-by: reviewer@shanks at " + gHk
+	e.Body = reviewedAt(gHk)
 	w.h.PullEdited(e)
 	w.settleAll()
 	if len(w.op.open()) != 1 {
 		t.Fatalf("1803/AC5: every member reviewed, yet %d asks", w.op.count())
 	}
+	if said := w.forge.statuses[gCtl+"@"+gHc+" mesh/delivery-group"]; !strings.HasPrefix(said, "success") {
+		t.Fatalf("1803/AC5: the group's status once asked is %q", said)
+	}
+}
+
+func TestA1803TheWaitingWordsFollowTheMembersStillWaiting(t *testing.T) {
+	w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}, {gTools, "", gHt, "Reviewed-by: pending"}})
+	w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-catalog#2", "")
+	w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-tools#3", "")
+	e := pr(gCat, 2, gHk, "feat/g")
+	e.Body = reviewedAt(gHk)
+	w.h.PullEdited(e)
+	w.settleAll()
+	if w.op.count() != 0 {
+		t.Fatalf("1803/AC7: a member still waits, yet %d asks", w.op.count())
+	}
+	// Only the other member is named now, on the group and on every member's own status.
+	w.wantGroupSays(t, "1803/AC7 one waiting", gCtl, gHc, "mesh-tools#3", "mesh-catalog#2")
+	w.wantGroupSays(t, "1803/AC7 one waiting", gCat, gHk, "mesh-tools#3", "mesh-catalog#2")
+}
+
+func TestA1803APushAfterAnApprovingReviewHoldsTheNewHead(t *testing.T) {
+	w := greenWithBody(t, reviewedAt(readyHead))
+	if len(w.op.open()) != 1 {
+		t.Fatalf("setup: the reviewed head is not asked")
+	}
+	// A new head on the same pull request whose description still names the old commit.
+	const pushed2 = "7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a"
+	p := pr("novox/app", 7, pushed2, "feat/x", "src/a.go")
+	p.Body = reviewedAt(readyHead)
+	w.h.PullUpdated(p)
+	w.h.Checked(judgedAt("novox/app", 7, pushed2, "pass", "t1t1t1t1t1t1"))
+	w.settleAll()
+	d := w.delivery(IDOf("novox/app", pushed2))
+	if d.Ready == nil || !strings.Contains(d.Ready.Waits, waitsForReview+shortOf(pushed2, 12)) {
+		t.Fatalf("1803/AC8: the new head waits %+v; wanted it to wait for a review at %s", d.Ready, shortOf(pushed2, 12))
+	}
+	if len(w.op.open()) != 0 {
+		t.Fatalf("1803/AC8: the new head was asked on the old review: %v", w.op.open())
+	}
 }

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

@@ -1019,7 +1019,10 @@ type Group struct {
 	Hold *GroupHold `json:"hold,omitempty"`
 	// SaidStatus is the mesh/delivery-group status last written for it: a change of the status a state takes is
 	// written too, though the state did not change (novox/hq issue 902).
-	SaidStatus string   `json:"said_status,omitempty"`
+	SaidStatus string `json:"said_status,omitempty"`
+	// SaidReview is the wait for a review last said of the group while it was held for one (ADR 0496 §2): a change of
+	// the members still waiting is said too, though the state did not change.
+	SaidReview string   `json:"said_review,omitempty"`
 	Owed       []Effect `json:"owed,omitempty"`
 }
 

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

@@ -221,8 +221,8 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 		if waits != r.Waits {
 			wasHeld := d.holdsForReview()
 			r.Waits = waits
-			if wasHeld != d.holdsForReview() && d.shownOnItsPullRequest() {
-				// The status and the view say it waits for a review, and say no more once it is asked.
+			if (wasHeld || d.holdsForReview()) && d.shownOnItsPullRequest() {
+				// The status and the view say what it waits for, again whenever the words change while held, and no more once asked.
 				state, desc := statusOf(d)
 				d.owe(Effect{Kind: EffectView, Since: now})
 				d.owe(Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery", State: state, Line: desc, Since: now})

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

@@ -134,14 +134,14 @@ func (h *Holder) Show(id string) (any, error) {
 			out["reads"] = d.strandedEndedSaid() // stopped, and nothing can carry what it waited for (issue 1581)
 		}
 		if g := h.groups[d.Group]; g != nil {
-			state, why := GroupState(g, h.membersOf(g))
+			state, why, _ := h.groupState(g, h.membersOf(g))
 			out["group"] = map[string]any{"group": g, "state": state, "why": why}
 		}
 		return out, nil
 	}
 	if g := h.groups[id]; g != nil {
 		members := h.membersOf(g)
-		state, why := GroupState(g, members)
+		state, why, _ := h.groupState(g, members)
 		return map[string]any{"group": g, "state": state, "why": why, "members": members}, nil
 	}
 	return nil, fmt.Errorf("no delivery or group %q: `deliveries` lists them", id)
@@ -184,7 +184,7 @@ func (h *Holder) Groups(all bool) []GroupLine {
 		if len(members) == 0 {
 			continue
 		}
-		state, why := GroupState(g, members)
+		state, why, _ := h.groupState(g, members)
 		if !all && (state == "delivered" || state == "failed" || state == "stopped") {
 			continue
 		}