From b4d7a32abffcd1d5cf93fcee5502fa506ac885b5 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 01:25:15 +0200 Subject: [PATCH 1/3] Hold the Approve ask until a review names the head (issue 1803, ADR 0496) Twice an unreviewed pull request was asked and merged on a warning line alone. --- .../cmd/mesh-delivery-gate/effects.go | 3 + .../cmd/mesh-delivery-gate/events_test.go | 19 +-- .../cmd/mesh-delivery-gate/fakes_test.go | 4 +- .../cmd/mesh-delivery-gate/holder.go | 2 +- .../cmd/mesh-delivery-gate/holder_test.go | 6 +- .../cmd/mesh-delivery-gate/issue1079_test.go | 34 ++-- .../cmd/mesh-delivery-gate/issue1803_test.go | 159 ++++++++++++++++++ .../cmd/mesh-delivery-gate/model.go | 4 +- .../cmd/mesh-delivery-gate/ready.go | 43 +++++ 9 files changed, 237 insertions(+), 37 deletions(-) create mode 100644 modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go index e765003a..0856a618 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go +++ b/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) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go index fcea942e..90e2d95c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go +++ b/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()) } }) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go index d7c47f8e..c5627690 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go +++ b/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. diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index 35599fb2..193e4e68 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/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 { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go index 92ad64d2..f9857f42 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go +++ b/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)) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go index bc1cacd6..919bf4b4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go +++ b/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 diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go new file mode 100644 index 00000000..b2abe5d5 --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go @@ -0,0 +1,159 @@ +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) + } +} + +func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) { + 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)) + 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 { + 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]}}}) + w.settleAll() + 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) + } + } + e := pr(gCat, 2, gHk, "feat/g") + e.Body = "Reviewed-by: reviewer@shanks at " + 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()) + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index c2d9b806..77e0f19f 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/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: at ` (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 == "": diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index f2809277..8b461fd3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/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 it waits for a review, and say no more once it is 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. -- 2.54.0 From 09d5360bce5abcbd4769df97f4d5ab628b46292e Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 01:27:18 +0200 Subject: [PATCH 2/3] Follow the hold in the bus test: a head with no review is asked only once an edit names it (issue 1803) --- .../cmd/mesh-delivery-gate/bus_test.go | 41 +++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go index a4ad1cf6..76f46626 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go +++ b/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") -- 2.54.0 From c4fd31aa7e975302aa79374c51c2eb1cb06ec897 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 01:37:49 +0200 Subject: [PATCH 3/3] Say a held group waits for a review and keep each member's status current (issue 1803, round 1) --- .../cmd/mesh-delivery-gate/checks.go | 2 +- .../cmd/mesh-delivery-gate/holder.go | 36 +++++- .../cmd/mesh-delivery-gate/holder_test.go | 3 + .../cmd/mesh-delivery-gate/issue1803_test.go | 106 ++++++++++++++---- .../cmd/mesh-delivery-gate/model.go | 5 +- .../cmd/mesh-delivery-gate/ready.go | 4 +- .../cmd/mesh-delivery-gate/verbs.go | 6 +- 7 files changed, 131 insertions(+), 31 deletions(-) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go index a631a5dd..fab99083 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go +++ b/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 diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index 193e4e68..ef4b0045 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/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 stopped: ", is clipped at 140 characters, and the member's id must fit in it whole. const supersededWhy = "superseded, not carried by a later delivery: " diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go index f9857f42..6204e918 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go +++ b/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) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go index b2abe5d5..7b966052 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go +++ b/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()) + } } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index 77e0f19f..9425bfbe 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/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"` } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index 8b461fd3..77737660 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/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}) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go index 8e6c747c..3545cfcd 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/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 } -- 2.54.0