Merge pull request 'Hold the Approve ask until a review names the head (issue 1803, ADR 0496)' (#432) from fix/1803-hold-ask-without-review into main

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

@@ -519,30 +519,29 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB
 	b.publish(t, "mesh.mod.gitea.event.pull.updated", announced)
 	passed := b.publish(t, "mesh.seat."+ControllerSeat+".event.checked", judgedAt("novox/app", 7, head, "pass", "t1t1t1t1t1t1"))
 
-	// The ask is written into the gate's bucket asks within seconds of the verdict's event, with no label, comment or mark
-	// written by anyone, and nothing is asked of a router.
+	// No review names the head, so nothing is asked (ADR 0496, issue 1803): the delivery waits for a review.
+	rt.until("the delivery to wait for a review", 5*time.Second, func() bool {
+		e, err := b.kvs["pulls"].Get(ctx, "novox.app.7")
+		var k PullKey
+		return err == nil && json.Unmarshal(e.Value(), &k) == nil && k.Verdict == "pass"
+	})
+	time.Sleep(time.Second)
+	if key := onlyAsk(b); key != "" {
+		t.Fatalf("a head with no review was asked: %s", key)
+	}
+	// The description is edited to add the review record (issue 1079): the forge module's edit event carries it, and the
+	// ask is written into the gate's bucket asks within seconds, with no label, comment or mark written by anyone, and
+	// nothing is asked of a router.
+	edit := time.Now()
+	b.publish(t, "mesh.mod.gitea.event.forge.pull-request.edited", map[string]any{"owner": "novox", "repo": "app", "number": 7,
+		"title": announced.Title, "head_sha": head, "base": "main", "body": "Reviewed-by: lead@shanks at " + head})
+	_ = passed
 	var key string
 	rt.until("the ask to be written", 5*time.Second, func() bool { key = onlyAsk(b); return key != "" })
 	ask := askAt(t, b, key)
-	t.Logf("the ask was written %s after the verdict's event", time.Since(passed))
-	if !strings.Contains(ask.Ask.Explanation, "not reviewed at this head") || ask.Ask.ID != sdk.AskID(key) {
-		t.Fatalf("the first ask does not say the record is missing: %s", ask.Ask.Explanation)
-	}
-	// The description is edited to add the review record (issue 1079): the forge module's edit event carries it. The unanswered
-	// ask is taken away and written again, once, with the record read (the heads are the same, so is the key).
-	b.publish(t, "mesh.mod.gitea.event.forge.pull-request.edited", map[string]any{"owner": "novox", "repo": "app", "number": 7,
-		"title": announced.Title, "head_sha": head, "base": "main", "body": "Reviewed-by: lead@shanks at " + head})
-	rt.until("the ask to be written again with the record", 5*time.Second, func() bool {
-		a, ok := askNow(b, key) // the key may be absent for an instant while the ask is taken away and written again
-		return ok && strings.Contains(a.Ask.Explanation, "reviewed by lead on shanks")
-	})
-	// The ask is overwritten, never taken away: the key is there at every revision, and the second asking has an id of its own
-	// (review round 1, G2), which the router would take as new.
-	if h, err := b.kvs["asks"].History(ctx, key); err != nil || len(h) != 2 || h[1].Operation() != jetstream.KeyValuePut {
-		t.Fatalf("the ask's history is %d revisions (%v); wanted written and written again", len(h), err)
-	}
-	if again := askAt(t, b, key); again.Asking != 2 || again.Ask.ID != sdk.AskingID(key, 2) || again.Ask.ID == ask.Ask.ID {
-		t.Fatalf("the second asking is %d under %s, the first was %s", again.Asking, again.Ask.ID, ask.Ask.ID)
+	t.Logf("the ask was written %s after the edit's event", time.Since(edit))
+	if !strings.Contains(ask.Ask.Explanation, "reviewed by lead on shanks") || ask.Ask.ID != sdk.AskID(key) {
+		t.Fatalf("the ask does not say the record: %s", ask.Ask.Explanation)
 	}
 	if len(rt.asksOf("gitea.gitea_merge_pull_request")) != 0 {
 		t.Fatalf("merged before the Approve")

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/effects.go

@@ -131,6 +131,9 @@ func statusOf(d *Delivery) (string, string) {
 	case Proposed, Checked:
 		return "pending", "checking"
 	case Ready:
+		if d.holdsForReview() {
+			return "pending", clip("ready, "+d.Ready.Waits, 140) // novox/hq ADR 0496: no Approve is asked before a review
+		}
 		return "success", "ready: it delivers once merged"
 	case Rejected:
 		return "failure", clip("rejected: "+why, 140)

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

@@ -158,11 +158,11 @@ func (*askedAgainError) Error() string {
 	return "the forge refused gitea_merge_pull_request: novox/app#7 is not merged: its verdict was judged against trunk aaaaaaaaaaaa, and main is now bbbbbbbbbbbb. The check of head cccccccccccc merged into trunk bbbbbbbbbbbb was asked; took 3s. Ask again at cccccccccccc once both statuses pass. Nothing was merged"
 }
 
-// Issue 1022 (ADR 0421 §5): the review record is read and put in the ask; it is never a gate.
-func TestTheAskSaysFirstWhenThePullRequestIsNotReviewedAtThisHead(t *testing.T) {
-	for _, c := range []struct{ name, body, want string }{
-		{"no line", "Fixes issue 7.", "not reviewed at this head"},
-		{"another head", "Reviewed-by: lead@shanks at " + pushed, "the Reviewed-by line names " + pushed[:12]},
+// Issue 1022 (ADR 0421 §5), as ADR 0496 changed it (issue 1803): the review record is read; a head not reviewed is not asked.
+func TestAHeadNotReviewedAtThisHeadIsNotAsked(t *testing.T) {
+	for _, c := range []struct{ name, body string }{
+		{"no line", "Fixes issue 7."},
+		{"another head", "Reviewed-by: lead@shanks at " + pushed},
 	} {
 		t.Run(c.name, func(t *testing.T) {
 			w := newWorld(t)
@@ -171,13 +171,8 @@ func TestTheAskSaysFirstWhenThePullRequestIsNotReviewedAtThisHead(t *testing.T)
 			p.Body = c.body
 			w.h.PullUpdated(p)
 			hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
-			if w.op.count() != 1 {
-				t.Fatalf("an unreviewed pull request must still be asked: %d", w.op.count())
-			}
-			a := w.op.ask(w.op.open()[0])
-			first := strings.SplitN(a.Explanation, "\n", 2)[0]
-			if !strings.Contains(first, "not reviewed at this head") || !strings.Contains(a.Explanation, c.want) {
-				t.Fatalf("the ask does not say first that it is not reviewed at this head:\n%s", a.Explanation)
+			if w.op.count() != 0 {
+				t.Fatalf("an unreviewed pull request was asked: %d", w.op.count())
 			}
 		})
 	}

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

@@ -683,7 +683,9 @@ func (w *world) state(id string) State { return w.delivery(id).State }
 func pr(repo string, number int, sha, branch string, paths ...string) PullEvent {
 	owner, name, _ := strings.Cut(repo, "/")
 	return PullEvent{Owner: owner, Repo: name, Number: number, Base: "main", Head: branch, HeadSHA: sha,
-		HTMLURL: fmt.Sprintf("https://forge.invalid/%s/pulls/%d", repo, number), Paths: paths}
+		HTMLURL: fmt.Sprintf("https://forge.invalid/%s/pulls/%d", repo, number), Paths: paths,
+		// Reviewed at its head, as the Approve is asked only for a reviewed head (novox/hq ADR 0496, issue 1803).
+		Body: "Reviewed-by: tester@shanks at " + sha}
 }
 
 // verdict is the controller's `checked` for a head.

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

@@ -467,7 +467,7 @@ func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) {
 // 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
-// approved, so no Approve ended. An answered ask is left as it is. The record is never a gate (ADR 0421 §5). An edit with no
+// approved, so no Approve ended. An answered ask is left as it is. A record naming the head lifts the hold at once (ADR 0496 §4). An edit with no
 // description, or of a pull request with no open delivery at that head, changes nothing.
 func (h *Holder) PullEdited(p PullEvent) {
 	if p.HeadSHA == "" || p.Number == 0 {
@@ -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

@@ -19,8 +19,12 @@ const (
 
 func TestAPullRequestsHeadIsProposedCheckedAndReady(t *testing.T) {
 	w := newWorld(t)
-	w.h.PullUpdated(pr("novox/app", 7, head, "feat/x", "src/a.go"))
+	p := pr("novox/app", 7, head, "feat/x", "src/a.go")
+	p.Body = "" // an empty description is never read again, so the review set below stays
+	w.h.PullUpdated(p)
 	id := IDOf("novox/app", head)
+	// This head is not a full commit id, so its description line cannot name it: its review is set as read.
+	w.h.deliveries[id].ReviewedBy, w.h.deliveries[id].ReviewedAt = "tester@shanks", head
 	if w.state(id) != Proposed {
 		t.Fatalf("a head is %s", w.state(id))
 	}
@@ -359,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/issue1079_test.go

@@ -10,18 +10,19 @@ import (
 // this head". The forge's edit event is followed; an Approve ask not yet answered is withdrawn and asked again with the
 // record read (the operator agreed, 2026-10-18). The review record is never a gate (ADR 0421 §5).
 
-// askedUnreviewed is a green pull request whose Approve is asked while its description holds no Reviewed-by line.
-func askedUnreviewed(t *testing.T) *world {
+// askedReviewed is a green pull request whose Approve is asked, its review line naming the head by one reviewer.
+func askedReviewed(t *testing.T) *world {
 	w := newWorld(t)
 	w.op.auto = false
 	p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
 	p.Title = "Fix the apostrophe (issue 948)"
+	p.Body = "Reviewed-by: first@shanks at " + readyHead
 	w.h.PullUpdated(p)
 	w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
 	w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)}
 	w.h.SeeOpenPulls()
 	w.settleAll()
-	if open := w.op.open(); len(open) != 1 || !strings.Contains(w.op.ask(open[0]).Explanation, "not reviewed at this head") {
+	if open := w.op.open(); len(open) != 1 || !strings.Contains(w.op.ask(open[0]).Explanation, "reviewed by first on shanks") {
 		t.Fatalf("setup: the ask is %v", w.op.open())
 	}
 	return w
@@ -33,10 +34,10 @@ func edited(body string) PullEvent {
 	return p
 }
 
-func TestAReviewedByLineWrittenAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(t *testing.T) {
-	w := askedUnreviewed(t)
+func TestAReviewedByLineChangedAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(t *testing.T) {
+	w := askedReviewed(t)
 	first := w.op.open()[0]
-	w.h.PullEdited(edited("Reviewed-by: reviewer@shanks at " + readyHead))
+	w.h.PullEdited(edited("Reviewed-by: second@shanks at " + readyHead))
 	w.settleAll()
 	open := w.op.open()
 	// The heads are the same, so the key is: the ask is taken away and written again with the record in its words, which the
@@ -47,8 +48,7 @@ func TestAReviewedByLineWrittenAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(
 	if len(w.op.cancelled) != 0 || w.op.byKey[first].Asking != 2 {
 		t.Fatalf("the first ask was taken away (%v) or not written again as asking 2 (%d)", w.op.cancelled, w.op.byKey[first].Asking)
 	}
-	if e := w.op.ask(open[0]).Explanation; !strings.Contains(e, "reviewed by reviewer on shanks at "+shortOf(readyHead, 12)) ||
-		strings.Contains(e, "not reviewed") {
+	if e := w.op.ask(open[0]).Explanation; !strings.Contains(e, "reviewed by second on shanks at "+shortOf(readyHead, 12)) {
 		t.Fatalf("the new ask does not say the record: %s", e)
 	}
 	if len(w.forge.comments) != 0 {
@@ -63,35 +63,29 @@ func TestAReviewedByLineWrittenAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(
 }
 
 func TestAnEditThatChangesNothingTheAskSaysAsksNothingAndNoEditChangesAnAnsweredAsk(t *testing.T) {
-	w := askedUnreviewed(t)
-	// The same words: no description with a line, then a description without one.
-	w.h.PullEdited(edited("A new sentence in the description."))
+	w := askedReviewed(t)
+	// The same words: a description with the same line and a new sentence.
+	w.h.PullEdited(edited("A new sentence.\nReviewed-by: first@shanks at " + readyHead))
 	w.settleAll()
 	if w.op.count() != 1 || len(w.op.cancelled) != 0 {
 		t.Fatalf("an edit that changes no word of the ask asked again: %d asks, cancelled %v", w.op.count(), w.op.cancelled)
 	}
-	// An edit that brings a line for another head changes the words: the ask is made again, still saying not reviewed.
-	w.h.PullEdited(edited("Reviewed-by: reviewer@shanks at " + strings.Repeat("a", 40)))
-	w.settleAll()
-	if w.op.count() != 2 || len(w.op.open()) != 1 {
-		t.Fatalf("a line for another head: %d asks, open %v", w.op.count(), w.op.open())
-	}
 	// Answered: a later edit changes nothing.
 	w.op.answer(w.op.open()[0], OptionApprove)
 	w.settleAll()
 	asked := w.op.count()
-	w.h.PullEdited(edited("Reviewed-by: reviewer@shanks at " + readyHead))
+	w.h.PullEdited(edited("Reviewed-by: second@shanks at " + readyHead))
 	w.settleAll()
 	if w.op.count() != asked || len(w.op.open()) != 0 {
 		t.Fatalf("an edit after the Approve asked again: %d asks, open %v", w.op.count(), w.op.open())
 	}
-	if d := w.delivery(IDOf("novox/app", readyHead)); d.ReviewedBy != "reviewer@shanks" {
+	if d := w.delivery(IDOf("novox/app", readyHead)); d.ReviewedBy != "second@shanks" {
 		t.Fatalf("the record is still read: %q", d.ReviewedBy)
 	}
 }
 
 func TestAnEditOfAnUnknownOrMergedOrEmptyPullRequestChangesNothing(t *testing.T) {
-	w := askedUnreviewed(t)
+	w := askedReviewed(t)
 	other := pr("novox/other", 3, readyHead, "feat/y")
 	other.Body = "Reviewed-by: a@b at " + readyHead
 	w.h.PullEdited(other) // no delivery: nothing made

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

@@ -0,0 +1,223 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// novox/hq issue 1803, ADR 0496: the Approve is asked only for a head with a review recorded at that head. A missing
+// line, `Reviewed-by: pending` or a line naming another commit holds the ask; the delivery stays ready and says it waits
+// for a review at the head. An edit naming the head asks at once. A group asks when every member is reviewed. An ask
+// already raised is not touched.
+
+const stale1803 = "9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f"
+
+// greenWithBody is a green pull request whose description is body, the operator not answering by himself.
+func greenWithBody(t *testing.T, body string) *world {
+	w := newWorld(t)
+	w.op.auto = false
+	p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
+	p.Title = "Fix the apostrophe (issue 948)"
+	p.Body = body
+	w.h.PullUpdated(p)
+	w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
+	w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)}
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	return w
+}
+
+func (w *world) wantHeld(t *testing.T, label string) {
+	t.Helper()
+	want := "waits for a review at " + shortOf(readyHead, 12)
+	d := w.delivery(IDOf("novox/app", readyHead))
+	if w.op.count() != 0 || len(w.op.open()) != 0 {
+		t.Fatalf("%s: the operator was asked (%d asks)", label, w.op.count())
+	}
+	if d.State != Ready || d.Ready == nil || !strings.Contains(d.Ready.Waits, want) {
+		t.Fatalf("%s: state %s, waits %+v; wanted ready and %q", label, d.State, d.Ready, want)
+	}
+	found := false
+	for k, v := range w.forge.statuses {
+		if strings.HasSuffix(strings.SplitN(k, " ", 2)[1], "mesh/delivery") && strings.Contains(v, want) {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("%s: no mesh/delivery status says %q: %v", label, want, w.forge.statuses)
+	}
+}
+
+func TestA1803PendingReviewLineHoldsTheAsk(t *testing.T) {
+	w := greenWithBody(t, "Reviewed-by: pending")
+	w.wantHeld(t, "1803/AC1 pending")
+	for range 3 {
+		w.later(2 * 60 * 1e9)
+		w.settleAll()
+	}
+	w.wantHeld(t, "1803/AC1 pending after ticks")
+}
+
+func TestA1803MissingReviewLineHoldsTheAsk(t *testing.T) {
+	w := greenWithBody(t, "No review line at all.")
+	w.wantHeld(t, "1803/AC1 missing")
+}
+
+func TestA1803ReviewLineOfAnotherCommitHoldsTheAsk(t *testing.T) {
+	w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+stale1803)
+	w.wantHeld(t, "1803/AC2 stale")
+}
+
+func TestA1803ReviewLineNamingTheHeadIsAsked(t *testing.T) {
+	w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+readyHead)
+	if len(w.op.open()) != 1 {
+		t.Fatalf("1803/AC3: a review at the head is not asked: %v", w.op.open())
+	}
+	if e := w.op.ask(w.op.open()[0]).Explanation; !strings.Contains(e, "reviewed by reviewer on shanks") {
+		t.Fatalf("the ask does not name the review: %s", e)
+	}
+}
+
+func TestA1803AnEditNamingTheHeadRaisesTheAskAtOnce(t *testing.T) {
+	w := greenWithBody(t, "Reviewed-by: pending")
+	w.wantHeld(t, "1803/AC4 before the edit")
+	e := pr("novox/app", 7, readyHead, "feat/x")
+	e.Body = "Reviewed-by: reviewer@shanks at " + readyHead
+	w.h.PullEdited(e)
+	w.settleAll()
+	if len(w.op.open()) != 1 {
+		t.Fatalf("1803/AC4: the edit naming the head raised no ask: %d asks", w.op.count())
+	}
+	// An unanswered ask whose review is taken back is withdrawn: the hold applies again (ADR 0496 §1).
+	e.Body = "Reviewed-by: pending"
+	w.h.PullEdited(e)
+	w.settleAll()
+	if len(w.op.open()) != 0 {
+		t.Fatalf("1803/AC4: an unanswered ask stayed after its review was taken back: open %v", w.op.open())
+	}
+}
+
+func TestA1803AnApproveAlreadyGivenIsNotAffected(t *testing.T) {
+	w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+readyHead)
+	w.op.answer(w.op.open()[0], OptionApprove)
+	w.settleAll()
+	if len(w.forge.merges) != 1 {
+		t.Fatalf("1803/AC6: the approved head was not merged: %v", w.forge.merges)
+	}
+	// A later edit to pending changes nothing of the Approve given.
+	e := pr("novox/app", 7, readyHead, "feat/x")
+	e.Body = "Reviewed-by: pending"
+	w.h.PullEdited(e)
+	w.settleAll()
+	if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready == nil || d.Ready.Approved == nil {
+		t.Fatalf("1803/AC6: the Approve given was lost: %+v", d.Ready)
+	}
+}
+
+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
+	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)
+	}
+	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: 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())
+	}
+	w.wantGroupSays(t, "1803/AC5", gCtl, gHc, "mesh-catalog#2", "mesh-controller#1")
+	e := pr(gCat, 2, gHk, "feat/g")
+	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

@@ -132,7 +132,7 @@ type Delivery struct {
 	Ready *ReadyRun `json:"ready,omitempty"`
 	// ReviewedBy and ReviewedAt are the review record its pull request's description holds: the line
 	// `Reviewed-by: <who> at <full sha>` (novox/hq ADR 0421 §5, issue 1022), as written. Read at every head; never a
-	// gate: the ask says first when the line is missing or names another head.
+	// hold: no Approve is asked until it names the head (novox/hq ADR 0496, issue 1803).
 	ReviewedBy string `json:"reviewed_by,omitempty"`
 	ReviewedAt string `json:"reviewed_at,omitempty"`
 	// FreshWanted is whether a passing verdict that arrived out of date is to have its check asked against the
@@ -499,7 +499,7 @@ func ReviewIn(body string) (who, sha string) {
 }
 
 // reviewWords is what the delivery's review record says of its head, for the ask: reviewed by whom, or, first, that it
-// is not reviewed at this head (never a gate, ADR 0421 §5). ok is whether it is reviewed at this head.
+// is not reviewed at this head (the ask is held then, ADR 0496). ok is whether it is reviewed at this head.
 func reviewWords(d *Delivery) (words string, ok bool) {
 	switch {
 	case d.ReviewedBy == "":
@@ -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

@@ -219,7 +219,14 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 		r.Failed = ""
 		waits := h.readyWaits(d)
 		if waits != r.Waits {
+			wasHeld := d.holdsForReview()
 			r.Waits = waits
+			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})
+			}
 			if waits != "" {
 				h.Logf("[mesh-delivery] %s waits: %s", d.ID, waits)
 			}
@@ -381,12 +388,48 @@ func (h *Holder) readyWaits(d *Delivery) string {
 	default:
 		return "its check has not passed yet (" + string(d.State) + ")"
 	}
+	// ADR 0496 §1, §2: the Approve is asked only for heads with a review recorded at them. An ask already raised or
+	// answered is not touched (covering).
+	if h.covering(d) == nil {
+		if w := h.reviewWaits(d); w != "" {
+			return w
+		}
+	}
 	if d.Group == "" {
 		return ""
 	}
 	return h.groupTurnToMerge(d)
 }
 
+// waitsForReview is how a hold for a review begins, as the delivery's state and its mesh/delivery status say it.
+const waitsForReview = "waits for a review at "
+
+// reviewWaits is why the Approve is not asked yet for want of a review (novox/hq ADR 0496, issue 1803): a delivery
+// whose description holds no review line, `pending` or a line naming another commit, and for a group any member so.
+// Empty when every delivery the ask covers is reviewed at its head. Under the lock.
+func (h *Holder) reviewWaits(d *Delivery) string {
+	members := h.askMembers(d)
+	var waiting []string
+	for _, m := range members {
+		if _, ok := reviewWords(m); ok {
+			continue
+		}
+		if len(members) == 1 {
+			return waitsForReview + shortOf(m.Commit, 12)
+		}
+		waiting = append(waiting, "at "+shortOf(m.Commit, 12)+" ("+pullSaid(m)+")")
+	}
+	if len(waiting) == 0 {
+		return ""
+	}
+	return waitsForReview + strings.Join(waiting, " and ")[len("at "):]
+}
+
+// holdsForReview is whether a delivery's wait is the hold for a review.
+func (d *Delivery) holdsForReview() bool {
+	return d.Ready != nil && strings.HasPrefix(d.Ready.Waits, waitsForReview)
+}
+
 // groupTurnToMerge is why a group member may not merge yet; empty when it may: the group's composed check passed for
 // its members' heads as they are and is not out of date. No member waits for another's merge or delivery (novox/hq
 // ADR 0418 §4): once the composed check passed and the Approve covers the heads, every member is merged in one pass.

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
 		}