novox/mesh-catalog
18
0
Fork 0
Code Issues Pull Requests 3 Actions Packages Projects Releases Wiki Activity

Hold the Approve ask until a review names the head (issue 1803, ADR 0496)

Browse Source 
Twice an unreviewed pull request was asked and merged on a warning line alone.
This commit is contained in:
jschoubben
2026-10-22 01:25:15 +02:00
parent 4915f69857
commit b4d7a32abf
9 changed files with 237 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go
+3
View File
@@ -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
+7 -12
View File
@@ -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
+3 -1
View File
@@ -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
+1 -1
View File
@@ -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 {
modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go
+5 -1
View File
@@ -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))
}
modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go
+14 -20
View File
@@ -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
+159
View File
@@ -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())
}
}
modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go
+2 -2
View File
@@ -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 == "":
modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go
+43
View File
@@ -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.