issue 975: ready marks, verdicts and Approves ask the merge as events, not at the next minute

modules/mesh-delivery/cmd/mesh-delivery/approval.go

@@ -442,6 +442,7 @@ func (h *Holder) decided(id, state string, w *asks.Warrant) []func() {
 	for _, d := range covered {
 		r := d.Ready
 		r.Warrant = w.Says()
+		h.mergeAgainNow(d) // the operator chose: the merge is asked at once
 		switch option.ID {
 		case OptionApprove:
 			r.Approved = &now

modules/mesh-delivery/cmd/mesh-delivery/events.go

@@ -0,0 +1,114 @@
+package main
+
+import (
+	"regexp"
+	"strconv"
+	"strings"
+
+	"git.novox.be/novox/mesh-sdk/go/asks"
+)
+
+// Events that start a step (novox/hq ADR 0406 §1, issue 975).
+//
+// This owner acts on events: a ready mark, a verdict, an Approve, a group's turn. The forge's label and comment events
+// say that a pull request's ready mark may have changed; they do not say whether it counts. Which accounts' "ready at"
+// comments count is the forge module's rule (its trusted setting ready-authors, the gitea module's readyMarkOf), so a
+// label added or a "ready at" comment written makes this owner read the forge's open pull requests once, and the mark
+// it reads there is the mark (seeMarks). The label taken off needs no read: it only clears.
+
+// ForgePullEvent is the forge module's `forge.pull-request.labeled`, `.unlabeled` and `.commented`
+// (modules/gitea/webhook.ts): the pull request, and the label added or taken off, or the comment written.
+type ForgePullEvent struct {
+	Owner     string   `json:"owner"`
+	Repo      string   `json:"repo"`
+	Number    int      `json:"number"`
+	HeadSHA   string   `json:"head_sha,omitempty"`
+	Base      string   `json:"base,omitempty"`
+	Labels    []string `json:"labels,omitempty"`
+	Label     string   `json:"label,omitempty"`
+	Author    string   `json:"author,omitempty"`
+	Comment   string   `json:"comment,omitempty"`
+	CommentID int      `json:"comment_id,omitempty"`
+}
+
+// ForgeStatusEvent is the forge module's `forge.status`: a commit's status was set.
+type ForgeStatusEvent struct {
+	Owner   string `json:"owner"`
+	Repo    string `json:"repo"`
+	SHA     string `json:"sha"`
+	Context string `json:"context"`
+	State   string `json:"state"`
+}
+
+// readyAtComment is a comment that names the head a pull request is marked ready at, as the forge module reads it
+// (modules/gitea/client.ts READY_AT): "ready at <full sha>" on a line of its own.
+var readyAtComment = regexp.MustCompile(`(?im)^\s*ready at ([0-9a-f]{40}|[0-9a-f]{64})\b`)
+
+func eventPullKey(e ForgePullEvent) string {
+	return strings.ToLower(e.Owner+"/"+e.Repo) + "#" + strconv.Itoa(e.Number)
+}
+
+// wantForge says the forge's open pull requests are to be read at the next look: a ready mark may have been made.
+func (h *Holder) wantForge() {
+	h.mu.Lock()
+	h.forgeWanted = true
+	h.mu.Unlock()
+}
+
+// takeForgeWanted answers whether the forge's open pull requests are to be read, and takes the want.
+func (h *Holder) takeForgeWanted() bool {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	wanted := h.forgeWanted
+	h.forgeWanted = false
+	return wanted
+}
+
+// PullLabeled takes a label added to a pull request: the ready label asks for the mark to be read.
+func (h *Holder) PullLabeled(e ForgePullEvent) {
+	if e.Label == ReadyLabel {
+		h.wantForge()
+	}
+}
+
+// PullUnlabeled takes a label taken off a pull request: the ready label clears its mark, with no read.
+func (h *Holder) PullUnlabeled(e ForgePullEvent) {
+	if e.Label != ReadyLabel {
+		return
+	}
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	if _, marked := h.marks[eventPullKey(e)]; marked {
+		delete(h.marks, eventPullKey(e))
+		h.Logf("[mesh-delivery] %s's ready label was taken off: its mark is cleared", eventPullKey(e))
+	}
+}
+
+// PullCommented takes a comment on a pull request: one that names a head to be ready at asks for the mark to be read.
+func (h *Holder) PullCommented(e ForgePullEvent) {
+	if readyAtComment.MatchString(e.Comment) {
+		h.wantForge()
+	}
+}
+
+// React takes every step the facts allow, after an event that can change what they are: the forge's open pull requests
+// read when a ready mark may have been made, then the asks of a look (look). Never concurrent with itself.
+func (h *Holder) React() {
+	h.reactMu.Lock()
+	defer h.reactMu.Unlock()
+	if h.takeForgeWanted() {
+		h.SeeOpenPulls()
+	}
+	h.look()
+}
+
+// AskDecided takes the router's `decided` event for an ask this owner made: its warrant is applied to the deliveries
+// the ask covers, and what is said on their pull requests is said.
+func (h *Holder) AskDecided(id, state string, w *asks.Warrant) {
+	h.mu.Lock()
+	said := h.decided(id, state, w)
+	h.mu.Unlock()
+	for _, s := range said {
+		s()
+	}
+}

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

@@ -0,0 +1,160 @@
+package main
+
+import (
+	"encoding/json"
+	"testing"
+
+	stdio "git.novox.be/novox/mesh-sdk/go"
+)
+
+// **A ready mark, a verdict and an Approve are acted on as events** (novox/hq ADR 0406 §1, issue 975): the merge
+// is asked within the event that makes it possible, never at the next tick or the next minute.
+
+const (
+	labeledEvent   = "gitea.forge.pull-request.labeled"
+	unlabeledEvent = "gitea.forge.pull-request.unlabeled"
+	commentedEvent = "gitea.forge.pull-request.commented"
+)
+
+// hear is the runtime handing this owner one event, as handle takes it.
+func hear(t *testing.T, w *world, key string, body any) {
+	t.Helper()
+	raw, err := json.Marshal(body)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := handle(w.h, stdio.Envelope{Key: key, Body: raw}); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func labeled(number int, label string) ForgePullEvent {
+	return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, HeadSHA: readyHead, Base: "main", Label: label,
+		Labels: []string{label}}
+}
+
+func commentedReady(number int, head string) ForgePullEvent {
+	return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, Author: "reviewer", CommentID: 5,
+		Comment: "ready at " + head + "\n\napproved: the tests are the change's"}
+}
+
+// greenAnnounced is a pull request into main, announced and judged green, the operator not answering by himself.
+func greenAnnounced(t *testing.T) *world {
+	w := newWorld(t)
+	w.op.auto = false
+	w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
+	w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
+	return w
+}
+
+func TestALabelAndAReadyAtCommentAskTheOperatorAtOnceWithNoTick(t *testing.T) {
+	w := greenAnnounced(t)
+	// The label first: the forge reads it without a comment naming a head yet. Nothing is asked.
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")}
+	hear(t, w, labeledEvent, labeled(7, ReadyLabel))
+	if w.op.count() != 0 || len(w.forge.merges) != 0 {
+		t.Fatalf("a label with no head named was acted on: %d asks, merges %v", w.op.count(), w.forge.merges)
+	}
+	// The comment naming the head: the forge now reads the mark whole, and the ask is made within this event.
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)}
+	hear(t, w, commentedEvent, commentedReady(7, readyHead))
+	if w.op.count() != 1 {
+		t.Fatalf("the comment that completes the mark asked the operator %d times, wanted once, with no tick", w.op.count())
+	}
+	if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready == nil || d.Ready.By != "reviewer" || d.Ready.AskID == "" {
+		t.Fatalf("the mark is not kept on the delivery: %+v", d.Ready)
+	}
+}
+
+func TestACommentThatIsNotAReadyMarkReadsNothingFromTheForge(t *testing.T) {
+	w := greenAnnounced(t)
+	before := w.forge.openCalls
+	c := commentedReady(7, readyHead)
+	c.Comment = "looks fine to me, though I would not say ready at all"
+	hear(t, w, commentedEvent, c)
+	hear(t, w, labeledEvent, labeled(7, "kind/bug"))
+	if w.forge.openCalls != before {
+		t.Fatalf("the forge's open pull requests were read %d times for events that are no ready mark", w.forge.openCalls-before)
+	}
+}
+
+func TestTheReadyLabelTakenOffClearsTheMarkAtOnce(t *testing.T) {
+	w := greenAnnounced(t)
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)}
+	hear(t, w, labeledEvent, labeled(7, ReadyLabel))
+	w.h.mu.Lock()
+	marks := len(w.h.marks)
+	w.h.mu.Unlock()
+	if marks != 1 {
+		t.Fatalf("the label did not make a mark: %d", marks)
+	}
+	asked := w.op.count()
+	// The forge no longer shows the label; the event alone says so, and the forge is not read for it.
+	reads := w.forge.openCalls
+	w.forge.open = nil
+	hear(t, w, unlabeledEvent, labeled(7, ReadyLabel))
+	w.h.mu.Lock()
+	marks = len(w.h.marks)
+	w.h.mu.Unlock()
+	if marks != 0 || w.forge.openCalls != reads || w.op.count() != asked {
+		t.Fatalf("the mark was not cleared by the event alone: marks %d, reads %d, asks %d", marks, w.forge.openCalls-reads, w.op.count()-asked)
+	}
+}
+
+// approve is the operator's Approve of the one open ask, heard as the router's `decided` event.
+func approve(w *world) {
+	open := w.op.open()
+	w.op.answer(open[0], OptionApprove)
+	state, warrant, _ := w.op.Where(open[0])
+	w.h.AskDecided(open[0], state, warrant)
+	w.h.React()
+}
+
+func TestAnApproveHeardAsAnEventAsksTheMergeAtOnce(t *testing.T) {
+	w := greenAnnounced(t)
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)}
+	hear(t, w, commentedEvent, commentedReady(7, readyHead))
+	open := w.op.open()
+	if len(open) != 1 || len(w.forge.merges) != 0 {
+		t.Fatalf("asks %v, merges %v", open, w.forge.merges)
+	}
+	approve(w)
+	if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/app#7@"+readyHead {
+		t.Fatalf("the Approve, heard as an event, merged %v; wanted once at the marked head", w.forge.merges)
+	}
+}
+
+func TestAPassingVerdictAsksTheMergeAtOnceEvenJustAfterARefusal(t *testing.T) {
+	w := newWorld(t)
+	id := IDOf("novox/app", readyHead)
+	w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
+	w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
+	// Another merge moves the trunk: the verdict is out of date and the merge tool refuses, asking the check again.
+	w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2"))
+	w.forge.refuse = errAskedAgain
+	w.op.auto = false
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)}
+	hear(t, w, commentedEvent, commentedReady(7, readyHead))
+	approve(w)
+	if len(w.forge.merges) != 1 {
+		t.Fatalf("the merge tool was asked %v; wanted once to judge it again", w.forge.merges)
+	}
+	// The verdict against the trunk's head passes: merged within the verdict's own event, no minute waited.
+	w.forge.refuse = nil
+	w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
+	raw, _ := json.Marshal(judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2"))
+	if err := handle(w.h, stdio.Envelope{Key: ControllerSeat + ".checked", Body: raw}); err != nil {
+		t.Fatal(err)
+	}
+	if len(w.forge.merges) != 2 || w.delivery(id).Ready.Merged == nil {
+		t.Fatalf("the passing verdict did not ask the merge at once: %v %+v", w.forge.merges, w.delivery(id).Ready)
+	}
+}
+
+var errAskedAgain = &askedAgainError{}
+
+type askedAgainError struct{}
+
+func (*askedAgainError) Error() string {
+	return "the forge refused gitea_merge_pull_request: The check of head merged into trunk was asked"
+}

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

@@ -399,8 +399,9 @@ type fakeForge struct {
 	containsDown bool
 	containsAsks []string
 	// open is what OpenPulls answers; openDown a forge that cannot say.
-	open     []OpenPull
-	openDown bool
+	open      []OpenPull
+	openDown  bool
+	openCalls int
 	// merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722).
 	merges, comments, unlabels []string
 	refuse                     error
@@ -412,6 +413,7 @@ type fakeForge struct {
 func (f *fakeForge) OpenPulls() ([]OpenPull, error) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
+	f.openCalls++
 	if f.openDown {
 		return nil, errors.New("the forge is away")
 	}

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

@@ -84,6 +84,10 @@ type Holder struct {
 	tried owedTries
 	// marks are the open pull requests read with a ready mark, by owner/repo#number (novox/hq issue 722, ready.go).
 	marks map[string]*marked
+	// forgeWanted is whether the forge's open pull requests are to be read at the next React: a ready mark may have been
+	// made (events.go). reactMu keeps one React at a time.
+	forgeWanted bool
+	reactMu     sync.Mutex
 }
 
 // owedTries is how many owed effects were tried since this owner started, and when the last was: three numbers
@@ -509,6 +513,7 @@ func (h *Holder) PullMerged(p PullEvent) {
 	}
 	h.mu.Lock()
 	defer h.mu.Unlock()
+	delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a merged pull request keeps no mark
 	var d *Delivery
 	if p.HeadSHA != "" {
 		d = h.deliveries[IDOf(p.repository(), p.HeadSHA)]
@@ -667,6 +672,7 @@ func verdictOfState(s string) string {
 // go of it within the week anyway.
 func (h *Holder) PullClosed(p PullEvent) {
 	h.mu.Lock()
+	delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a closed pull request keeps no mark
 	for _, d := range h.deliveries {
 		if d.Repository == p.repository() && d.Number == p.Number && !d.State.Final() && d.MergedAs == "" {
 			d.ClosedUnmerged = true
@@ -736,6 +742,9 @@ func (h *Holder) Checked(c CheckedEvent) {
 			return
 		}
 		g.Check.Verdict, g.Check.Summary = c.Verdict, c.Summary
+		for _, m := range h.membersOf(g) {
+			h.mergeAgainNow(m) // the group's composed check answered: its members' merges are asked at once
+		}
 		// Each member's head and the trunk commit it was merged into (novox/hq ADR 0323 §6).
 		for _, m := range c.Members {
 			for id := range g.Check.Commits {
@@ -796,6 +805,7 @@ func (h *Holder) Checked(c CheckedEvent) {
 		v.Gate = "error"
 	}
 	d.Check, d.AskedAgain = v, nil
+	h.mergeAgainNow(d) // a verdict taken: the merge is asked at once, not a minute after a refusal
 	if c.Plan != nil {
 		d.Plan = c.Plan
 	}
@@ -815,6 +825,11 @@ func (h *Holder) WalkMoved(w Walk) {
 }
 
 func (h *Holder) walkMoved(w Walk) {
+	for _, d := range h.deliveries {
+		if d.Group != "" {
+			h.mergeAgainNow(d) // a walk moved: a group member's turn to merge may have come, asked at once
+		}
+	}
 	if w.Release != nil {
 		h.creditBacklog(w)
 		return
@@ -1055,9 +1070,12 @@ func (h *Holder) Reconcile() error {
 
 // --- the tick: groups, turns, adoption --------------------------------------------------------------------
 
-// Tick takes every step the facts allow that needs another's word: a group's order and composed check, a
+// Tick is React, for the tick that still runs beside the events.
+func (h *Holder) Tick() { h.React() }
+
+// look takes every step the facts allow that needs another's word: a group's order and composed check, a
 // delivery's turn, a walk with no delivery made one. The asking happens outside the lock.
-func (h *Holder) Tick() {
+func (h *Holder) look() {
 	for _, ask := range h.plan() {
 		ask()
 	}

modules/mesh-delivery/cmd/mesh-delivery/main.go

@@ -31,7 +31,9 @@ func (l *listening) set(s string) { l.mu.Lock(); l.now = s; l.mu.Unlock() }
 func (l *listening) get() string  { l.mu.Lock(); defer l.mu.Unlock(); return l.now }
 
 // The events this owner follows: the forge's pull requests, and the controller's verdicts and walks.
+// The forge's label and comment events say a ready mark changed (events.go).
 var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed",
+	"gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented",
 	ControllerSeat + ".checked", ControllerSeat + ".plan-moved"}
 
 func main() {
@@ -104,9 +106,15 @@ func run(h *Holder, l *listening) {
 	logf("[mesh-delivery] listening: %s", strings.Join(followed, ", "))
 }
 
-// handle takes one event. An event that cannot be read is said and acknowledged: read again it would fail
-// again, and the controller's walks are read back by comparison anyway.
+// handle takes one event, then takes every step the event makes possible (React). An event that cannot be read is
+// said and acknowledged: read again it would fail again, and the controller's walks are read back by comparison anyway.
 func handle(h *Holder, e stdio.Envelope) error {
+	taken := take(h, e)
+	h.React()
+	return taken
+}
+
+func take(h *Holder, e stdio.Envelope) error {
 	switch e.Key {
 	case "gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed":
 		var p PullEvent
@@ -122,6 +130,20 @@ func handle(h *Holder, e stdio.Envelope) error {
 		default:
 			h.PullClosed(p)
 		}
+	case "gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented":
+		var p ForgePullEvent
+		if err := json.Unmarshal(e.Body, &p); err != nil {
+			h.Refuse(fmt.Errorf("%s is not readable: %v", e.Key, err))
+			return nil
+		}
+		switch e.Key {
+		case "gitea.forge.pull-request.labeled":
+			h.PullLabeled(p)
+		case "gitea.forge.pull-request.unlabeled":
+			h.PullUnlabeled(p)
+		default:
+			h.PullCommented(p)
+		}
 	case ControllerSeat + ".checked":
 		var c CheckedEvent
 		if err := json.Unmarshal(e.Body, &c); err != nil {

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

@@ -65,6 +65,8 @@ type ReadyRun struct {
 	Tries   int       `json:"tries,omitempty"`
 	LastTry time.Time `json:"last_try"`
 	Answer  string    `json:"answer,omitempty"`
+	// merging is whether a merge ask is on its way, outside the lock: not asked twice meanwhile. Not kept.
+	merging bool
 	// RefusedSince is when the merge tool began refusing it without a break.
 	RefusedSince *time.Time `json:"refused_since,omitempty"`
 	// Merged is when the merge tool merged it.
@@ -255,11 +257,11 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 		if approvalAsk != nil {
 			asks = append(asks, approvalAsk)
 		}
-		if waits != "" || (!r.LastTry.IsZero() && now.Sub(r.LastTry) < readyTryEvery) {
+		if waits != "" || r.merging || (!r.LastTry.IsZero() && now.Sub(r.LastTry) < readyTryEvery) {
 			continue
 		}
 		r.Tries++
-		r.LastTry = now
+		r.LastTry, r.merging = now, true
 		h.keep(d)
 		asks = append(asks, h.askMerge(d.ID, p, mark.Head))
 	}
@@ -268,6 +270,14 @@ func (h *Holder) readyAsks(now time.Time) []func() {
 	return asks
 }
 
+// mergeAgainNow lets a marked delivery's merge be asked at once: something that decides whether it may merge happened (a
+// verdict, an Approve, a group's turn), so the wait after a refusal does not hold it. Under the lock.
+func (h *Holder) mergeAgainNow(d *Delivery) {
+	if d != nil && d.Ready != nil && d.Ready.Merged == nil {
+		d.Ready.LastTry = time.Time{}
+	}
+}
+
 // readyWaits is what a marked delivery's merge waits on; empty when it is asked now. A ready delivery is asked; so is
 // a proposed one whose verdict went out of date and whose check nobody asked again — the merge tool asks it.
 func (h *Holder) readyWaits(d *Delivery) string {
@@ -325,6 +335,7 @@ func (h *Holder) askMerge(id string, p OpenPull, head string) func() {
 			return
 		}
 		r, now := d.Ready, h.Now()
+		r.merging = false
 		if err == nil {
 			r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil
 			h.Logf("[mesh-delivery] %s merged on its ready mark at %s, its check passed against its trunk's head", id,

modules/mesh-delivery/module.json

@@ -28,6 +28,9 @@
     "gitea.pull.updated",
     "gitea.pull.merged",
     "gitea.pull.closed",
+    "gitea.forge.pull-request.labeled",
+    "gitea.forge.pull-request.unlabeled",
+    "gitea.forge.pull-request.commented",
     "mesh-controller.checked",
     "mesh-controller.plan-moved"
   ],