mesh-delivery-gate: ask a pull request's check again when a config value is held or dropped for it (issue 1826)

mesh-config raises config-changed (hold, drop) naming the pull request; the delivery, or its group's
composed check, is asked again on the gate's own path, with why in the transition. Not the recheck verb.

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

@@ -0,0 +1,146 @@
+package main
+
+import (
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+// A config value held or dropped for a pull request asks its check again (novox/hq ADR 0502 clause 5, issue 1826).
+//
+// The controller's composed check lays the approved values held for a pull request (mesh-config's `held`) when it
+// composes, so a check run before a value was held, or after it was dropped, judged another mesh. mesh-config
+// raises `config-changed` with the action hold or drop and the pull request; this owner then asks that pull
+// request's check, or its group's composed check, again, the way it asks a moved head: its own path, with the
+// reason in the delivery's transition, and not the `recheck` verb, which stays an exception (ADR 0421 §12).
+
+// ConfigChangedEvent is the event this owner follows from mesh-config.
+const ConfigChangedEvent = "mesh-config.config-changed"
+
+// ConfigChange is mesh-config's `config-changed` as far as this owner reads it.
+type ConfigChange struct {
+	Action string `json:"action"`
+	Pull   string `json:"pull,omitempty"`
+	Head   string `json:"head,omitempty"`
+	Key    string `json:"key,omitempty"`
+	Level  struct {
+		Node   string `json:"node,omitempty"`
+		Module string `json:"module,omitempty"`
+	} `json:"level"`
+}
+
+var pullNamed = regexp.MustCompile(`^([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+)#([0-9]+)$`)
+
+// ConfigChanged takes a hold or a drop of a value for a pull request: every live delivery of that pull request has its
+// check asked again. Anything else mesh-config says (a value set, a resolution, the merge's set) asks nothing.
+func (h *Holder) ConfigChanged(c ConfigChange) {
+	if c.Action != "hold" && c.Action != "drop" {
+		return
+	}
+	m := pullNamed.FindStringSubmatch(strings.TrimSpace(c.Pull))
+	if m == nil {
+		return
+	}
+	number, _ := strconv.Atoi(m[3])
+	repository := strings.ToLower(m[1] + "/" + m[2])
+	verb := "held for"
+	if c.Action == "drop" {
+		verb = "dropped from"
+	}
+	why := fmt.Sprintf("a config value was %s this pull request: %s of %s on %s (mesh-config)", verb, c.Key,
+		orWord(c.Level.Module, "a module"), orWord(c.Level.Node, "a node"))
+	h.mu.Lock()
+	var ids []string
+	for id, d := range h.deliveries {
+		if d.Number == number && strings.EqualFold(d.Repository, repository) && d.MergedAs == "" && !d.State.Final() {
+			ids = append(ids, id)
+		}
+	}
+	h.mu.Unlock()
+	for _, id := range ids {
+		h.askAgainForConfig(id, why)
+	}
+}
+
+func orWord(s, word string) string {
+	if s == "" {
+		return word
+	}
+	return s
+}
+
+// askAgainForConfig asks one delivery's check again: a rejected or ready one is proposed again, as any judged again
+// head is; a proposed one has its pending ask replaced. Its group's composed check goes out of date, and is asked
+// once a member is judged again.
+func (h *Holder) askAgainForConfig(id, why string) {
+	h.mu.Lock()
+	d := h.deliveries[id]
+	if d == nil {
+		h.mu.Unlock()
+		return
+	}
+	probe := *d
+	probe.Transitions = append([]Transition(nil), d.Transitions...)
+	_, refused := Apply(&probe, EvRecheck, true, Facts{Now: h.Now(), Why: why, By: byOwner})
+	proposed := d.State == Proposed
+	h.mu.Unlock()
+	switch {
+	case refused == nil:
+		if _, err := h.Recheck(id, why, byOwner); err != nil {
+			h.Logf("[mesh-delivery] %s: its check could not be asked again for a config value: %v", id, err)
+		}
+	case proposed:
+		h.askProposedAgain(id, why)
+	}
+}
+
+// askProposedAgain asks a proposed delivery's check again, its merge check put back to pending first.
+func (h *Holder) askProposedAgain(id, why string) {
+	h.mu.Lock()
+	d := h.deliveries[id]
+	if d == nil || d.State != Proposed {
+		h.mu.Unlock()
+		return
+	}
+	owner, repo, commit, page, number := d.Owner(), d.Repo(), d.Commit, d.HTMLURL, d.Number
+	h.mu.Unlock()
+	if err := h.checkingAgain(owner, repo, number, commit, page, why); err != nil {
+		h.Logf("[mesh-delivery] %s: its check is not asked again for a config value: %v", id, err)
+		return
+	}
+	h.mu.Lock()
+	d = h.deliveries[id]
+	if d == nil || d.State != Proposed {
+		h.mu.Unlock()
+		return
+	}
+	d.Check = nil
+	if g := h.groups[d.Group]; g != nil && g.Check != nil {
+		now := h.Now()
+		g.Check.OutOfDate = fmt.Sprintf("composed check out of date: %s was rechecked (%s)", d.ID, why)
+		g.Check.OutOfDateAt = now
+		g.Updated = now
+		h.keepGroup(g)
+	}
+	m := MemberOf(d)
+	h.mu.Unlock()
+	asked, err := h.Controller.Check("", []Member{m})
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	if err != nil {
+		h.Logf("[mesh-delivery] %s's check cannot be asked again yet (%v); it is asked at the next look", id, err)
+		h.again("a check asked again for a config value that could not be asked")
+		return
+	}
+	d = h.deliveries[id]
+	if d == nil || d.State != Proposed {
+		return
+	}
+	d.CheckAsk = &CheckAsk{ID: asked, At: h.Now(), By: byOwner, Why: why}
+	h.Logf("[mesh-delivery] %s: its check asked again as %s — %s", id, asked, why)
+	d.Owed = append(d.Owed, Effect{Kind: EffectNote, Commit: d.Commit, Since: h.Now(),
+		Line: oneLine(fmt.Sprintf("%s delivery %s: its check asked again by %s as %s: %s", h.Now().UTC().Format("2006-01-02T15:04:05Z07:00"),
+			id, byOwner, asked, why))})
+	h.keep(d)
+}

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

@@ -0,0 +1,145 @@
+package main
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	stdio "git.novox.be/novox/mesh-sdk/go"
+)
+
+// Issue 1826 (novox/hq ADR 0502 clause 5): when mesh-config holds or drops a value for a pull request that is a
+// delivery, its check, or its group's composed check, is asked again, and the transition says why.
+
+func configChanged(action, pull string) ConfigChange {
+	c := ConfigChange{Action: action, Pull: pull, Head: head, Key: "media-gid"}
+	c.Level.Node, c.Level.Module = "ace", "n8n"
+	return c
+}
+
+// 1826/K4: a lone delivery that was ready is proposed again with its merge check pending, its check asked, and the
+// transition says that a config value was held.
+func TestAHeldValueAsksTheReadyDeliverysCheckAgain(t *testing.T) {
+	w := newWorld(t)
+	id := heldGreen(t, w, "novox/app", 7)
+	asked := len(w.ctl.checks)
+	w.h.ConfigChanged(configChanged("hold", "novox/app#7"))
+	if got := w.ctl.checks[asked:]; len(got) != 1 || got[0] != ":"+id {
+		t.Fatalf("asked %v; wanted its own check once", got)
+	}
+	d := w.delivery(id)
+	last := d.Transitions[len(d.Transitions)-1]
+	if d.State != Proposed || last.Event != EvRecheck || !strings.Contains(last.Why, "media-gid") ||
+		!strings.Contains(last.Why, "held") || last.By != byOwner {
+		t.Errorf("state %s, last transition %+v", d.State, last)
+	}
+	if s := w.forge.statuses["novox/app@"+head+" "+ctxGate]; !strings.HasPrefix(s, "pending checking again") {
+		t.Errorf("the merge check says %q: the old verdict would still let it merge", s)
+	}
+}
+
+// 1826/K4: a rejected delivery (the very case: the composed check failed on the missing value) is asked again on a
+// hold; and a drop asks it again too.
+func TestAHeldValueAsksARejectedDeliverysCheckAgainAndADropToo(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr("novox/app", 8, head, "feat/y", "src/a.go"))
+	w.h.Checked(verdict("novox/app", 8, head, "fail"))
+	w.settleAll()
+	id := IDOf("novox/app", head)
+	if w.state(id) != Rejected {
+		t.Fatalf("it is %s", w.state(id))
+	}
+	asked := len(w.ctl.checks)
+	w.h.ConfigChanged(configChanged("hold", "novox/app#8"))
+	if w.state(id) != Proposed || len(w.ctl.checks) != asked+1 {
+		t.Fatalf("a hold left it %s with %v asked", w.state(id), w.ctl.checks[asked:])
+	}
+	w.h.Checked(verdict("novox/app", 8, head, "pass"))
+	w.settleAll()
+	asked = len(w.ctl.checks)
+	w.h.ConfigChanged(configChanged("drop", "novox/app#8"))
+	d := w.delivery(id)
+	if len(w.ctl.checks) != asked+1 || !strings.Contains(d.Transitions[len(d.Transitions)-1].Why, "dropped") {
+		t.Errorf("a drop asked %v; last transition %+v", w.ctl.checks[asked:], d.Transitions[len(d.Transitions)-1])
+	}
+}
+
+// 1826/K4: a delivery whose check is on its way is asked again, its old ask replaced, and says why in its note.
+func TestAHeldValueAsksTheCheckOfAProposedDeliveryAgain(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr("novox/app", 9, head, "feat/z", "src/a.go"))
+	w.settleAll()
+	id := IDOf("novox/app", head)
+	before := w.delivery(id).CheckAsk
+	asked := len(w.ctl.checks)
+	w.h.ConfigChanged(configChanged("hold", "novox/app#9"))
+	d := w.delivery(id)
+	if len(w.ctl.checks) != asked+1 || d.CheckAsk == nil || (before != nil && d.CheckAsk.ID == before.ID) ||
+		!strings.Contains(d.CheckAsk.Why, "held") {
+		t.Errorf("asked %v; ask %+v (was %+v)", w.ctl.checks[asked:], d.CheckAsk, before)
+	}
+	if d.State != Proposed {
+		t.Errorf("it is %s", d.State)
+	}
+}
+
+// 1826/K4: a hold for a member of a group asks the member's check again and then the group's composed check, which
+// the group waits for.
+func TestAHeldValueForAGroupMemberAsksTheComposedCheckAgain(t *testing.T) {
+	w := approvedGroup(t)
+	g := w.h.groups["feat/g"]
+	firstComposed := g.Check.Asked
+	asked := len(w.ctl.checks)
+	w.h.ConfigChanged(configChanged("hold", gCat+"#2"))
+	if got := w.ctl.checks[asked:]; len(got) != 1 || got[0] != ":"+IDOf(gCat, gHk) {
+		t.Fatalf("a hold asked %v; wanted the member's own check", got)
+	}
+	if g = w.h.groups["feat/g"]; g.Check.OutOfDate == "" || !strings.Contains(g.Check.OutOfDate, "rechecked") {
+		t.Fatalf("the composed check stands: %+v", g.Check)
+	}
+	w.later(60e9)
+	w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0))
+	w.settleAll()
+	last := w.ctl.checks[len(w.ctl.checks)-1]
+	if !strings.HasPrefix(last, "feat/g:") || w.h.groups["feat/g"].Check.Asked == firstComposed {
+		t.Errorf("the composed check was not asked again: %v", w.ctl.checks[asked:])
+	}
+}
+
+// 1826/K4: what is not a hold or a drop for a delivery of this owner asks nothing: the merge's set, another action,
+// another pull request, a pull request with no delivery, and a delivery already merged.
+func TestOnlyAHoldOrADropForADeliveryAsksACheckAgain(t *testing.T) {
+	w := newWorld(t)
+	heldGreen(t, w, "novox/app", 7)
+	asked := len(w.ctl.checks)
+	for _, c := range []ConfigChange{configChanged("set", "novox/app#7"), configChanged("resolve", ""),
+		configChanged("hold", "novox/app#99"), configChanged("hold", "novox/other#7"), configChanged("hold", ""),
+		configChanged("hold", "garbage")} {
+		w.h.ConfigChanged(c)
+	}
+	if len(w.ctl.checks) != asked || w.state(IDOf("novox/app", head)) != Ready {
+		t.Errorf("asked %v; it is %s", w.ctl.checks[asked:], w.state(IDOf("novox/app", head)))
+	}
+}
+
+// 1826/K4: the event is heard as mesh-config's config-changed, and it is no recheck verb.
+func TestTheConfigChangedEventIsFollowed(t *testing.T) {
+	w := newWorld(t)
+	heldGreen(t, w, "novox/app", 7)
+	found := false
+	for _, f := range followed {
+		found = found || f == "mesh-config.config-changed"
+	}
+	if !found {
+		t.Fatalf("not followed: %v", followed)
+	}
+	raw, _ := json.Marshal(map[string]any{"action": "hold", "pull": "novox/app#7", "head": head, "key": "media-gid",
+		"level": map[string]any{"level": "module", "node": "ace", "module": "n8n"}})
+	asked := len(w.ctl.checks)
+	if err := handle(w.h, stdio.Envelope{Key: "mesh-config.config-changed", Body: raw}); err != nil {
+		t.Fatal(err)
+	}
+	if len(w.ctl.checks) != asked+1 {
+		t.Errorf("the event asked %v", w.ctl.checks[asked:])
+	}
+}

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

@@ -34,7 +34,7 @@ func (l *listening) get() string  { l.mu.Lock(); defer l.mu.Unlock(); return l.n
 // The forge's status events say that a check answered (events.go).
 var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed",
 	"gitea.forge.status", "gitea.forge.pull-request.edited", "gitea.forge.pull-request.closed", ControllerSeat + ".checked", ControllerSeat + ".plan-moved",
-	forgePattern}
+	forgePattern, ConfigChangedEvent}
 
 // forgePattern is the forge bridge's messages on FORGE (novox/hq ADR 0437 §8): the gate follows a merge and the refusal of one.
 const forgePattern = "mesh-delivery-forge-bridge.forge.**"
@@ -187,6 +187,13 @@ func take(h *Holder, e stdio.Envelope) error {
 		}
 		h.Checked(c)
 		h.KeepVerdict(c)
+	case ConfigChangedEvent:
+		var c ConfigChange
+		if err := json.Unmarshal(e.Body, &c); err != nil {
+			h.Refuse(fmt.Errorf("a config change is not readable: %v", err))
+			return nil
+		}
+		h.ConfigChanged(c)
 	case ControllerSeat + ".plan-moved":
 		var w Walk
 		if err := json.Unmarshal(e.Body, &w); err != nil {

modules/mesh-delivery-gate/module.json

@@ -32,7 +32,8 @@
     "gitea.forge.pull-request.closed",
     "mesh-controller.checked",
     "mesh-controller.plan-moved",
-    "mesh-delivery-forge-bridge.forge.**"
+    "mesh-delivery-forge-bridge.forge.**",
+    "mesh-config.config-changed"
   ],
   "emits": [
     "transition",