From dd31d991cd52d87ec5fe0dfb18301881c9c09318 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 16:18:43 +0200 Subject: [PATCH 1/2] Fill a delivery made from its verdict from its pull request's announcement, and say what it waits for (issue 1726) --- .../cmd/mesh-delivery-gate/holder.go | 15 ++++ .../cmd/mesh-delivery-gate/issue1726_test.go | 83 +++++++++++++++++++ .../cmd/mesh-delivery-gate/ready.go | 22 +++++ .../cmd/mesh-delivery-gate/verbs.go | 3 + 4 files changed, 123 insertions(+) create mode 100644 modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go 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 95550c73..5cc6775a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -366,6 +366,21 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { d.BaseSHA = p.BaseSHA h.keep(d) } + // A delivery made from its verdict before its head was heard (issue 1726) holds none of what an announcement + // carries: filled from this one, so its base is known and the operator is asked. + if d.Base == "" && p.Base != "" { + d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL + d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed + d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid + d.Issues = IssuesIn(p.Title + "\n" + p.Body) + d.Number = p.Number + h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base) + h.keep(d) + if d.Group == "" && !d.State.Final() { + h.joinGroup(d) + } + h.Poke() + } // The review record, corrected in the description without a push, is read again (ADR 0421 §5). if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) { d.ReviewedBy, d.ReviewedAt = by, at diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go new file mode 100644 index 00000000..6e93883f --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go @@ -0,0 +1,83 @@ +package main + +import ( + "strings" + "testing" + "time" +) + +// novox/hq issue 1726: a delivery made from a check's verdict that arrived before its pull request's head was heard +// (transition "its verdict arrived before its head was heard", as mesh-controller#419 at a47476252772 was) holds no base, +// branch, title, URLs or review record. The ready pass skipped it in silence, so no Approve was ever asked, and the +// announcement of the same head, heard later, filled none of it. + +const verdictFirstBody = "Fixes a bug.\n\nReviewed-by: reviewer@shanks at " + readyHead + "\n" + +// verdictFirst is the live shape: the verdict heard, the head not, and the delivery ready. +func verdictFirst(t *testing.T) (*world, string) { + w := newWorld(t) + w.op.auto = false + id := IDOf("novox/app", readyHead) + w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1")) + w.settleAll() + d := w.delivery(id) + if d.State != Ready || d.Base != "" || d.Branch != "" || d.Title != "" || d.HTMLURL != "" { + t.Fatalf("not the live shape: %s base %q branch %q title %q", d.State, d.Base, d.Branch, d.Title) + } + return w, id +} + +func TestADeliveryWithoutItsHeadSaysWhatItWaitsForAndAsksNoOne(t *testing.T) { + w, id := verdictFirst(t) + if n := len(w.op.open()); n != 0 { + t.Fatalf("an ask was made for a delivery with no base: %d", n) + } + d := w.delivery(id) + if d.Ready == nil || !strings.Contains(d.Ready.Waits, "announcement") { + t.Fatalf("it does not say what it waits for: %+v", d.Ready) + } + shown, err := w.h.Show(id) + if err != nil { + t.Fatal(err) + } + if waits, _ := shown.(map[string]any)["waits"].(string); !strings.Contains(waits, "announcement") { + t.Fatalf("show waits %q", waits) + } +} + +func TestAnAnnouncementFillsADeliveryMadeFromItsVerdictAndTheOperatorIsAsked(t *testing.T) { + w, id := verdictFirst(t) + p := pr("novox/app", 419, readyHead, "fix/x", "src/a.go") + p.Title, p.Body, p.CloneURL = "Fix x", verdictFirstBody, "https://forge.invalid/novox/app.git" + w.h.PullUpdated(p) + w.settleAll() + d := w.delivery(id) + if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.HTMLURL == "" || d.CloneURL == "" || + d.ReviewedBy != "reviewer@shanks" || len(d.Paths) != 1 { + t.Fatalf("the announcement did not fill the delivery: %+v", d) + } + if n := len(w.op.open()); n != 1 { + t.Fatalf("the operator was asked %d times; wanted once", n) + } + if d.Ready.Waits != "" && !strings.Contains(d.Ready.Waits, "Approve") { + t.Fatalf("still waits: %q", d.Ready.Waits) + } +} + +func TestADeliveryWithoutItsHeadPastItsBoundIsStalled(t *testing.T) { + w, id := verdictFirst(t) + if s := w.h.Stalled(); len(s) != 0 { + t.Fatalf("stalled before its bound: %+v", s) + } + w.later(headHeardBound + time.Minute) + w.settleAll() + var found bool + for _, l := range w.h.Stalled() { + if l.ID == id && strings.Contains(l.Says, "announcement") { + found = true + } + } + if !found { + t.Fatalf("not stalled past its bound: %+v", w.h.Stalled()) + } +} 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 0a60efc2..e460b592 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -41,6 +41,12 @@ const readyRefusedFor time.Duration = 0 // groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: none (issue 1115). const groupRefusedFor time.Duration = 0 +// waitsForHead is what a delivery waits for when its verdict was heard and its pull request's head was not (issue 1726). +const waitsForHead = "its pull request's announcement: its base, branch and title are not heard, so what it merges into is not known" + +// headHeardBound is how long a delivery may wait for its pull request's announcement before it is said as stalled. +const headHeardBound = 10 * time.Minute + // groupClosed is whether a group is closed: a member reached the trunk. Under the lock. func (h *Holder) groupClosed(id string) bool { g := h.groups[id] @@ -181,6 +187,18 @@ func (h *Holder) readyAsks(now time.Time) []func() { } continue } + if d.Base == "" { + // A delivery made from its verdict before its head was heard (issue 1726): its base, branch and title are + // unknown, so what it merges into is too. It is neither asked nor passed over in silence: it waits for its + // pull request's announcement, which fills it (pullUpdated), and past headHeardBound it is stalled. + if r.Waits != waitsForHead { + r.Waits = waitsForHead + h.Logf("[mesh-delivery] %s waits: %s", d.ID, waitsForHead) + h.keep(d) + } + h.lookAt(r.Seen.Add(headHeardBound), "a delivery whose pull request was never announced") + continue + } if d.Base != MergeBranch { if r.OtherBase != d.Base { r.OtherBase = d.Base @@ -369,6 +387,10 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { continue } switch { + case d.Base == "" && d.Number != 0 && now.Sub(r.Seen) >= headHeardBound: + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), + Bound: headHeardBound.String(), H2: "none: the forge's announcement of the pull request's head, heard again at each look at the open pull requests — the operator's", + Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)}) case r.Failed != "" && d.State == Rejected: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), Bound: "0s", H2: "none: a new head, or a recheck that passes — the operator's", 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 7179b032..8e6c747c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go @@ -42,6 +42,9 @@ func (h *Holder) waitsFor(d *Delivery) string { if d.MergedAs != "" { return "a walk that answers its merge" } + if d.Base == "" && d.Number != 0 { + return waitsForHead + } return "its merge" case Rejected: return "a new head, or a recheck" From 4b9464f3ceeba7685fa451ba535ece58ddd1a8dd Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 17:06:51 +0200 Subject: [PATCH 2/2] Supersede older heads when an announcement or edit fills a delivery made from its verdict, and say truthfully what fills it (issue 1726) --- modules/mesh-delivery-gate/.gitignore | 1 + .../cmd/mesh-delivery-gate/holder.go | 73 ++++++++++++------- .../cmd/mesh-delivery-gate/issue1726_test.go | 43 +++++++++++ .../cmd/mesh-delivery-gate/ready.go | 2 +- 4 files changed, 93 insertions(+), 26 deletions(-) create mode 100644 modules/mesh-delivery-gate/.gitignore diff --git a/modules/mesh-delivery-gate/.gitignore b/modules/mesh-delivery-gate/.gitignore new file mode 100644 index 00000000..03570e7b --- /dev/null +++ b/modules/mesh-delivery-gate/.gitignore @@ -0,0 +1 @@ +/cmd/mesh-delivery-gate/mesh-delivery-gate 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 5cc6775a..627ffabe 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -368,19 +368,7 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { } // A delivery made from its verdict before its head was heard (issue 1726) holds none of what an announcement // carries: filled from this one, so its base is known and the operator is asked. - if d.Base == "" && p.Base != "" { - d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL - d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed - d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid - d.Issues = IssuesIn(p.Title + "\n" + p.Body) - d.Number = p.Number - h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base) - h.keep(d) - if d.Group == "" && !d.State.Final() { - h.joinGroup(d) - } - h.Poke() - } + h.fillFromAnnouncement(d, p) // The review record, corrected in the description without a push, is read again (ADR 0421 §5). if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) { d.ReviewedBy, d.ReviewedAt = by, at @@ -413,18 +401,7 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { h.leaveGroup(old) } } - replaced := "" - for _, old := range h.deliveries { - if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) && - !old.State.Final() && old.MergedAs == "" { - old.NewerHead = p.HeadSHA - h.settleAndKeep(old) - if old.Group != "" { - h.leaveGroup(old) - replaced = old.ID - } - } - } + replaced := h.supersedeOlder(p) d, t, err := h.freshAnnouncement(p, now, startedOver) if err != nil { h.refuse(err) @@ -444,6 +421,49 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) { return isNew } +// supersedeOlder ends the older heads of p's pull request: each is told its newer head, and one in a group leaves it. It +// returns the id of the one that left a group, whose place the new head takes (takePlaceOf). Under the lock. +func (h *Holder) supersedeOlder(p PullEvent) (replaced string) { + for _, old := range h.deliveries { + if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) && + !old.State.Final() && old.MergedAs == "" { + old.NewerHead = p.HeadSHA + h.settleAndKeep(old) + if old.Group != "" { + h.leaveGroup(old) + replaced = old.ID + } + } + } + return replaced +} + +// fillFromAnnouncement fills a delivery made from its check's verdict before its pull request's head was heard (issue +// 1726: no base, branch, title, URLs or review record) from an announcement or an edit of that pull request, and then +// does what a first announcement does: the older heads of the pull request are superseded, and the delivery takes +// their place in a group or joins the group its branch makes. Nothing for a delivery that holds its base. Under the lock. +func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) { + if d.Base != "" || p.Base == "" || d.State.Final() || d.MergedAs != "" { + return + } + d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL + d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed + d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid + d.Issues, d.After = IssuesIn(p.Title+"\n"+p.Body), AfterIn(p.Body) + d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body) + if p.BaseSHA != "" { + d.BaseSHA = p.BaseSHA + } + d.Number = p.Number + h.Logf("[mesh-delivery] %s: its pull request's announcement filled its base (%s), branch and title", d.ID, p.Base) + h.keep(d) + replaced := h.supersedeOlder(p) + if d.Group == "" && !h.takePlaceOf(replaced, d) { + h.joinGroup(d) + } + h.Poke() +} + // 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 @@ -473,6 +493,9 @@ func (h *Holder) PullEdited(p PullEvent) { h.pullUpdated(p) return } + if d != nil && d.Number == p.Number { + h.fillFromAnnouncement(d, p) // a delivery made from its verdict, its head never heard (issue 1726) + } if p.Body == "" { return } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go index 6e93883f..9d4ce895 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go @@ -81,3 +81,46 @@ func TestADeliveryWithoutItsHeadPastItsBoundIsStalled(t *testing.T) { t.Fatalf("not stalled past its bound: %+v", w.h.Stalled()) } } + +// A head asked, then a push whose verdict is heard before its head: the announcement of the new head supersedes the old +// one exactly as a first announcement does, so one Approve stands, for the new head. +func TestAnAnnouncementFillingAVerdictFirstHeadSupersedesTheOlderHeadAndItsAsk(t *testing.T) { + w := newWorld(t) + w.op.auto = false + a, b := IDOf("novox/app", readyHead), IDOf("novox/app", pushed) + w.h.PullUpdated(pr("novox/app", 419, readyHead, "fix/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 419, readyHead, "pass", "t1t1t1t1t1t1")) + w.settleAll() + if n := len(w.op.open()); n != 1 { + t.Fatalf("head A: %d asks open; wanted 1", n) + } + w.h.Checked(judgedAt("novox/app", 419, pushed, "pass", "t1t1t1t1t1t1")) + w.settleAll() + w.h.PullUpdated(pr("novox/app", 419, pushed, "fix/x", "src/a.go")) + w.settleAll() + if got := w.delivery(a); got.NewerHead != pushed || got.State != Superseded { + t.Fatalf("head A is %s with newer head %q; wanted superseded by %s", got.State, got.NewerHead, pushed) + } + if got := w.delivery(b); got.Base != "main" || got.Branch != "fix/x" { + t.Fatalf("head B not filled: %+v", got) + } + if n := len(w.op.open()); n != 1 { + t.Fatalf("%d asks open; wanted one, for the new head", n) + } +} + +// A description edit is a natural trigger too: it fills a delivery made from its verdict. +func TestAnEditOfThePullRequestFillsADeliveryMadeFromItsVerdict(t *testing.T) { + w, id := verdictFirst(t) + p := pr("novox/app", 419, readyHead, "fix/x") + p.Title, p.Body = "Fix x", verdictFirstBody + w.h.PullEdited(p) + w.settleAll() + d := w.delivery(id) + if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.ReviewedBy != "reviewer@shanks" { + t.Fatalf("the edit did not fill the delivery: %+v", d) + } + if n := len(w.op.open()); n != 1 { + t.Fatalf("the operator was asked %d times; wanted once", n) + } +} 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 e460b592..ffccdb18 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -389,7 +389,7 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { switch { case d.Base == "" && d.Number != 0 && now.Sub(r.Seen) >= headHeardBound: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), - Bound: headHeardBound.String(), H2: "none: the forge's announcement of the pull request's head, heard again at each look at the open pull requests — the operator's", + Bound: headHeardBound.String(), H2: "none: a later announcement of the pull request's head (the merge tool announces it again, the forge's announcer restarts, or a new push) or an edit of its description; a recheck does not fill it — the operator's", Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)}) case r.Failed != "" && d.State == Rejected: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(),