mesh-delivery: mark-ready labels then comments the head in the order the forge counts; an empty-head clear says why (issue 913, issue 873)

2026-10-16 14:33:50 +02:00
parent 3db85a0dde
commit 741b729ad4
7 changed files with 273 additions and 9 deletions
@@ -404,6 +404,9 @@ type fakeForge struct {
 	// merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722).
 	merges, comments, unlabels []string
 	refuse                     error
+	// calls are the labels and comments in the order asked (novox/hq issue 913); labelErr fails a Label.
+	calls    []string
+	labelErr error
 }

 func (f *fakeForge) OpenPulls() ([]OpenPull, error) {
@@ -670,6 +673,17 @@ func (f *fakeForge) Comment(owner, repo string, number int, body string) error {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 	f.comments = append(f.comments, fmt.Sprintf("%s/%s#%d: %s", owner, repo, number, body))
+	f.calls = append(f.calls, "comment "+body)
+	return nil
+}
+
+func (f *fakeForge) Label(owner, repo string, number int, name string) error {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if f.labelErr != nil {
+		return f.labelErr
+	}
+	f.calls = append(f.calls, fmt.Sprintf("label %s/%s#%d %s", owner, repo, number, name))
 	return nil
 }

@@ -352,5 +352,19 @@ func tools(h *Holder, l *listening) []stdio.Tool {
 				"and tried again, given up) and when the last was, walks with no delivery yet, whether its events arrive, when " +
 				"the controller's walks were last read or why they could not be, and the last refusals.",
 			Run: func(map[string]any) (any, error) { return h.Status(l.get()), nil }},
+		{Name: seat + "mark-ready",
+			Description: "A reviewer's word that a delivery's head is approved: the mesh's own forge account puts the " +
+				"label mesh/ready on its pull request and then writes \"ready at <head>\", the order the forge counts. " +
+				"Refused when the delivery's commit is no longer its pull request's head. mesh-delivery then merges it " +
+				"once its checks pass (novox/hq ADR 0364, issue 913).",
+			Input: map[string]any{"type": "object", "properties": map[string]any{"id": str("the delivery's id (owner/repository@commit): the head reviewed"),
+				"reviewed-by": str("who reviewed it, as who@node"), "why": str("the review's verdict in a line")},
+				"required": []string{"id", "reviewed-by", "why"}},
+			Run: func(a map[string]any) (any, error) {
+				if err := need(a, "id", "reviewed-by", "why"); err != nil {
+					return nil, err
+				}
+				return h.MarkReady(strArg(a, "id"), strArg(a, "reviewed-by"), strArg(a, "why"))
+			}},
 	}
 }
@@ -0,0 +1,115 @@
+package main
+
+import (
+	"errors"
+	"strings"
+	"testing"
+)
+
+// **A reviewer's word marks the pull request ready in the order the forge counts** (novox/hq issue 913): the label,
+// then the comment "ready at <full sha>". Written the other way round the mark's head is empty (issue 873).
+
+const fullHead = "3333333333333333333333333333333333333333"
+
+// readyDelivery is a delivery of a pull request whose head is fullHead, its check passed, and the forge's open pull.
+func readyDelivery(t *testing.T, w *world) string {
+	t.Helper()
+	w.h.PullUpdated(pr("novox/app", 7, fullHead, "feat/x", "src/a.go"))
+	w.h.Checked(verdict("novox/app", 7, fullHead, "pass"))
+	w.settleAll()
+	w.forge.open = []OpenPull{{Owner: "novox", Repo: "app", Number: 7, Base: "main", HeadSHA: fullHead}}
+	return IDOf("novox/app", fullHead)
+}
+
+func TestMarkReadyLabelsThenCommentsTheFullHead(t *testing.T) {
+	w := newWorld(t)
+	id := readyDelivery(t, w)
+	answer, err := w.h.MarkReady(id, "fix-913@shanks", "the change is right")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(w.forge.calls) != 2 || w.forge.calls[0] != "label novox/app#7 mesh/ready" {
+		t.Fatalf("the forge was asked %q; wanted the label first", w.forge.calls)
+	}
+	c := strings.TrimPrefix(w.forge.calls[1], "comment ")
+	if first, _, _ := strings.Cut(c, "\n"); first != "ready at "+fullHead || !strings.Contains(c, "\n\nReviewed by fix-913@shanks: the change is right.") ||
+		!strings.Contains(c, "mark-ready") {
+		t.Fatalf("the comment is %q", c)
+	}
+	d := w.delivery(id)
+	if d.Ready == nil || d.Ready.Head != fullHead || d.Ready.By != "fix-913@shanks" || d.Ready.Seen.IsZero() {
+		t.Fatalf("the mark is not kept: %+v", d.Ready)
+	}
+	if !strings.Contains(answer, "marked") {
+		t.Fatalf("answer %q", answer)
+	}
+}
+
+func TestMarkReadyRefusesAMovedHeadAndAsksTheForgeNothing(t *testing.T) {
+	w := newWorld(t)
+	id := readyDelivery(t, w)
+	w.forge.open[0].HeadSHA = pushed
+	_, err := w.h.MarkReady(id, "fix-913@shanks", "ok")
+	if err == nil || !strings.Contains(err.Error(), "the head is now "+pushed) {
+		t.Fatalf("a moved head: %v", err)
+	}
+	if len(w.forge.calls) != 0 {
+		t.Fatalf("the forge was asked %q", w.forge.calls)
+	}
+}
+
+func TestMarkReadyRefusesAnUnknownAndAFinalDelivery(t *testing.T) {
+	w := newWorld(t)
+	id := readyDelivery(t, w)
+	if _, err := w.h.MarkReady("novox/app@nothing", "a@b", "ok"); err == nil || !strings.Contains(err.Error(), "deliveries") {
+		t.Fatalf("unknown id: %v", err)
+	}
+	w.h.mu.Lock()
+	w.h.deliveries[id].State = Stopped
+	w.h.mu.Unlock()
+	if _, err := w.h.MarkReady(id, "a@b", "ok"); err == nil {
+		t.Fatal("a final delivery was marked")
+	}
+	if len(w.forge.calls) != 0 {
+		t.Fatalf("the forge was asked %q", w.forge.calls)
+	}
+}
+
+func TestMarkReadyWritesNoCommentWhenTheLabelFails(t *testing.T) {
+	w := newWorld(t)
+	id := readyDelivery(t, w)
+	w.forge.labelErr = errors.New("the forge refused gitea_api: no such label")
+	_, err := w.h.MarkReady(id, "a@b", "ok")
+	if err == nil || !strings.Contains(err.Error(), "no such label") {
+		t.Fatalf("label failure: %v", err)
+	}
+	if len(w.forge.comments) != 0 {
+		t.Fatalf("commented: %v", w.forge.comments)
+	}
+	if w.delivery(id).Ready != nil {
+		t.Fatal("marked though the label failed")
+	}
+}
+
+func TestClearMarkWithAnEmptyHeadSaysWhatWasWrong(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")}
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at <head>\" comment by a listed account") ||
+		!strings.Contains(w.forge.comments[0], "mesh-delivery.mark-ready") || strings.Contains(w.forge.comments[0], "pushed after it") {
+		t.Fatalf("comments: %v", w.forge.comments)
+	}
+}
+
+func TestClearMarkWithAnOlderHeadKeepsTheOldWording(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go"))
+	w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)}
+	w.h.SeeOpenPulls()
+	w.settleAll()
+	if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "pushed after it") {
+		t.Fatalf("comments: %v", w.forge.comments)
+	}
+}
@@ -96,6 +96,8 @@ type Forge interface {
 	Merge(owner, repo string, number int, head string) error
 	// Comment adds a comment to a pull request.
 	Comment(owner, repo string, number int, body string) error
+	// Label puts a label on a pull request, by the label's name (novox/hq issue 913).
+	Label(owner, repo string, number int, name string) error
 	// Unlabel takes a label off a pull request, by the label's id.
 	Unlabel(owner, repo string, number, label int) error
 }
@@ -407,6 +409,47 @@ func (f toolForge) Comment(owner, repo string, number int, body string) error {
 	return f.tool("gitea_add_comment", map[string]any{"owner": owner, "repo": repo, "number": number, "body": body})
 }

+// Label puts a label on a pull request. The forge's add-labels call takes label ids on every version (names only on
+// the newer), so the id is read from the repository's labels by name first; a repository without the label is refused,
+// not given one.
+func (f toolForge) Label(owner, repo string, number int, name string) error {
+	id := 0
+	for page := 1; page <= 20 && id == 0; page++ {
+		raw, err := f.ask("gitea.gitea_api", map[string]any{"method": "GET",
+			"path": fmt.Sprintf("/repos/%s/%s/labels?limit=50&page=%d", owner, repo, page)})
+		if err != nil {
+			return err
+		}
+		answer, output, ok := answerOf(raw)
+		if !ok {
+			return fmt.Errorf("the forge refused gitea_api: %s", lastLine(output))
+		}
+		var got struct {
+			Result []struct {
+				ID   int    `json:"id"`
+				Name string `json:"name"`
+			} `json:"result"`
+		}
+		if err := json.Unmarshal(answer, &got); err != nil {
+			return fmt.Errorf("the forge's labels are not readable: %s", clip(string(answer), 200))
+		}
+		for _, l := range got.Result {
+			if l.Name == name {
+				id = l.ID
+			}
+		}
+		if len(got.Result) < 50 {
+			break
+		}
+	}
+	if id == 0 {
+		return fmt.Errorf("%s/%s has no label %s", owner, repo, name)
+	}
+	return f.tool("gitea_api", map[string]any{"method": "POST",
+		"path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels", owner, repo, number),
+		"body": map[string]any{"labels": []int{id}}})
+}
+
 func (f toolForge) Unlabel(owner, repo string, number, label int) error {
 	return f.tool("gitea_api", map[string]any{"method": "DELETE",
 		"path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels/%d", owner, repo, number, label)})
@@ -270,22 +270,40 @@ func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() {
 	return func() {
 		err := h.Forge.Unlabel(p.Owner, p.Repo, p.Number, mark.LabelID)
 		if err == nil {
-			err = h.Forge.Comment(p.Owner, p.Repo, p.Number, fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+
-				"the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA))
+			err = h.Forge.Comment(p.Owner, p.Repo, p.Number, clearedSaying(mark, p))
 		}
-		if err != nil {
-			h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v",
-				k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err)
-			return
+		if mark.Head == "" {
+			if err != nil {
+				h.Logf("[mesh-delivery] %s's ready label has no \"ready at <head>\" comment of a listed account after it, and could not be taken off yet: %v", k, err)
+				return
+			}
+			h.Logf("[mesh-delivery] %s's ready label taken off: no \"ready at <head>\" comment of a listed account was written after it; mark-ready writes both in order", k)
+		} else {
+			if err != nil {
+				h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v",
+					k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err)
+				return
+			}
+			h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12),
+				shortOf(p.HeadSHA, 12))
 		}
-		h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12),
-			shortOf(p.HeadSHA, 12))
 		h.mu.Lock()
 		delete(h.marks, k)
 		h.mu.Unlock()
 	}
 }

+// clearedSaying is what a cleared mark says on its pull request. A mark with no head is a label no "ready at <head>"
+// comment of a listed account followed (novox/hq issue 873); one with a head names an older head than the pull's.
+func clearedSaying(mark ReadyMark, p OpenPull) string {
+	if mark.Head == "" {
+		return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at <head>\" comment by a listed " +
+			"account was written after it. Mark it with `mesh-delivery.mark-ready`, which writes both in the order the forge counts."
+	}
+	return fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+
+		"the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA)
+}
+
 // sayOn comments on a delivery's pull request.
 func (h *Holder) sayOn(d *Delivery, body string) func() {
 	owner, repo, number, id := d.Owner(), d.Repo(), d.Number, d.ID
@@ -331,6 +331,65 @@ func (h *Holder) Recheck(id, why, by string) (string, error) {
 	return fmt.Sprintf("%s is proposed again, its merge check pending; its check was asked (%s)", id, asked), nil
 }

+// MarkReady is a reviewer's word that a delivery's head is approved (novox/hq issue 913): the label mesh/ready on its
+// pull request, and only after that call succeeded the comment "ready at <head>", the order the forge counts (a comment
+// written before the label does not count, issue 873). Refused for an unknown, final or merged delivery, and when the
+// pull request's head is no longer the delivery's commit.
+func (h *Holder) MarkReady(id, reviewedBy, why string) (string, error) {
+	h.mu.Lock()
+	d := h.deliveries[id]
+	if d == nil {
+		h.mu.Unlock()
+		return "", fmt.Errorf("no delivery %q; `deliveries` lists them", id)
+	}
+	if d.State.Final() || d.MergedAs != "" {
+		h.mu.Unlock()
+		return "", fmt.Errorf("%s is %s: a delivery past its merge or final is not marked ready", id, d.State)
+	}
+	if d.Number == 0 {
+		h.mu.Unlock()
+		return "", fmt.Errorf("%s is not of a pull request: only a pull request is marked ready", id)
+	}
+	owner, repo, number, commit := d.Owner(), d.Repo(), d.Number, d.Commit
+	h.mu.Unlock()
+
+	open, err := h.Forge.OpenPulls()
+	if err != nil {
+		return "", fmt.Errorf("%s is not marked: the forge's open pull requests could not be read: %v", id, err)
+	}
+	head := ""
+	for _, p := range open {
+		if strings.EqualFold(p.Owner, owner) && strings.EqualFold(p.Repo, repo) && p.Number == number {
+			head = p.HeadSHA
+		}
+	}
+	if head == "" {
+		return "", fmt.Errorf("%s#%d is not an open pull request on the forge", owner+"/"+repo, number)
+	}
+	if !sameCommit(head, commit) {
+		return "", fmt.Errorf("the head is now %s; review and mark that one", head)
+	}
+	if err := h.Forge.Label(owner, repo, number, ReadyLabel); err != nil {
+		return "", fmt.Errorf("%s is not marked: the label %s could not be put on: %v", id, ReadyLabel, err)
+	}
+	body := fmt.Sprintf("ready at %s\n\nReviewed by %s: %s. Marked through mesh-delivery.mark-ready (novox/hq issue 913).",
+		head, reviewedBy, strings.TrimRight(strings.TrimSpace(why), "."))
+	if err := h.Forge.Comment(owner, repo, number, body); err != nil {
+		return "", fmt.Errorf("%s has the label %s but its comment \"ready at\" could not be written: %v; mark it again", id, ReadyLabel, err)
+	}
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	now := h.Now()
+	d.Ready = &ReadyRun{Head: head, By: reviewedBy, MarkedAt: now.UTC().Format(time.RFC3339), Seen: now}
+	h.Logf("[mesh-delivery] %s is marked ready at %s by %s through mark-ready: %s", id, shortOf(head, 12), reviewedBy, why)
+	h.keep(d)
+	waits := h.readyWaits(d)
+	if waits == "" {
+		return fmt.Sprintf("%s is marked ready at %s; its merge is asked once the next tick reads the mark", id, shortOf(head, 12)), nil
+	}
+	return fmt.Sprintf("%s is marked ready at %s; its merge waits: %s", id, shortOf(head, 12), waits), nil
+}
+
 // checkingAgain puts a head's merge check statuses — those a branch's protection may require — back to
 // pending, so the forge refuses the merge until the fresh verdict sets them. The gate always, since every
 // verdict sets it; the repository's own check only when the head holds one, since a verdict that says none
@@ -19,7 +19,8 @@
        "release",
        "stop",
        "close",
-        "retire-history"
+        "retire-history",
+        "mark-ready"
      ]
    }
  ],