Merge pull request 'Put the merge check back to pending before a recheck asks it (hq issue 305)' (#118) from fix/recheck-resets-the-merge-check into main

modules/gitea/delivery.ts

@@ -92,10 +92,15 @@ const mergeCheckContexts = new Set(["mesh/merge-gate", "mesh/repo-check"]);
  *  bounded description. */
 export function deliveryStatus(context: string, state: string, description: string, target?: string) {
   if (!/^mesh\/[a-z-]+$/.test(context)) throw new Error(`${context} is not a status of the mesh's own`);
-  // The merge check's statuses are its verdict's, set when the controller says it, and a branch may require
-  // them: never set by hand, so nothing passes — or blocks — a merge past the check (novox/hq issue 293).
-  if (mergeCheckContexts.has(context)) throw new Error(`${context} is the merge check's status, set by its verdict only`);
   if (!states.has(state)) throw new Error(`${state} is not a status the forge keeps`);
+  // The merge check's statuses are its verdict's, set when the controller says it, and a branch may require
+  // them: never set to a verdict by hand, so nothing passes — or fails — a merge past the check (novox/hq
+  // issue 293). Pending is no verdict: it is what this holder says on every new head until the verdict comes,
+  // and what a recheck of the same head says, so the old verdict does not let it merge while the fresh check
+  // runs.
+  if (mergeCheckContexts.has(context) && state !== "pending") {
+    throw new Error(`${context} is the merge check's status, set by its verdict only (pending, for a check asked again, is the one state it takes by hand)`);
+  }
   let d = String(description ?? "").replace(/\s+/g, " ").trim();
   if (d.length > 140) d = d.slice(0, 139) + "…";
   return { state: state as "pending" | "success" | "error" | "failure" | "warning", context, description: d,

modules/gitea/test/delivery.test.ts

@@ -55,5 +55,9 @@ test("one view per pull request, found by its marker; only the mesh's statuses",
   // The merge check's statuses are its verdict's alone: required by a branch, they are never set by hand.
   assert.throws(() => deliveryStatus("mesh/merge-gate", "success", "x"), /merge check's status/);
   assert.throws(() => deliveryStatus("mesh/repo-check", "warning", "x"), /merge check's status/);
+  assert.throws(() => deliveryStatus("mesh/merge-gate", "failure", "x"), /merge check's status/);
+  // Pending is no verdict: a recheck says it so the old verdict does not let the head merge meanwhile.
+  assert.equal(deliveryStatus("mesh/merge-gate", "pending", "checking again: main moved on").state, "pending");
+  assert.equal(deliveryStatus("mesh/repo-check", "pending", "checking again: main moved on").context, "mesh/repo-check");
   assert.equal(deliveryStatus("mesh/delivery", "success", "x", "javascript:alert(1)").target_url, undefined);
 });

modules/gitea/tools/index.ts

@@ -494,7 +494,7 @@ export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] {
     },
     {
       name: "gitea_commit_status",
-      description: "Set one of the mesh's statuses on a commit (mesh/delivery, mesh/delivery-group): pending, success, error, failure or warning, a short description, and the page it links to.",
+      description: "Set one of the mesh's statuses on a commit (mesh/delivery, mesh/delivery-group): pending, success, error, failure or warning, a short description, and the page it links to. The merge check's statuses (mesh/merge-gate, mesh/repo-check) take pending only — a recheck putting them back until the fresh verdict sets them.",
       input: {
         owner: { type: "string", description: "the repository owner" },
         repo: { type: "string", description: "the repository name" },

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

@@ -93,6 +93,8 @@ type fakeController struct {
 	stops    []string
 	plans    int
 	down     bool
+	// onCheck runs as a check is asked, before it is answered: what the forge held at that moment.
+	onCheck func()
 }
 
 func newFakeController() *fakeController {
@@ -130,6 +132,9 @@ func (c *fakeController) Check(group string, ms []Member) (string, error) {
 	if c.down {
 		return "", errors.New("down")
 	}
+	if c.onCheck != nil {
+		c.onCheck()
+	}
 	c.asked++
 	id := fmt.Sprintf("check-%d", c.asked)
 	var heads []string

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

@@ -247,7 +247,8 @@ func tools(h *Holder, l *listening) []stdio.Tool {
 			Description: "Every delivery held past its state's bound, with the transition the table lets healer H2 take.",
 			Run:         func(map[string]any) (any, error) { return h.Stalled(), nil }},
 		{Name: seat + "recheck",
-			Description: "Check a rejected or ready delivery again: proposed again, and its check asked. With why.",
+			Description: "Check a rejected or ready delivery again: its merge check statuses put back to pending on the forge, " +
+				"so nothing merges on the old verdict; proposed again, and its check asked. With why.",
 			Input: map[string]any{"type": "object", "properties": map[string]any{"id": str("the delivery's id"),
 				"why": str("why, kept with the transition")}, "required": []string{"id", "why"}},
 			Run: func(a map[string]any) (any, error) {

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

@@ -0,0 +1,97 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// A recheck puts the merge check's statuses back to pending before it asks the check: the forge keeps the
+// newest status of each context on the head, and a recheck asks of that same head, so the old verdict would
+// stand — and branch protection would let the pull request merge on it — until the fresh one lands.
+
+// heldGreen is a head whose merge check passed, as the forge's holder set it from the verdict.
+func heldGreen(t *testing.T, w *world, repo string, number int) string {
+	t.Helper()
+	w.h.PullUpdated(pr(repo, number, head, "feat/x", "src/a.go"))
+	owner, name, _ := strings.Cut(repo, "/")
+	_ = w.forge.Status(owner, name, head, ctxGate, "success", "pass: the gate said pass", "")
+	_ = w.forge.Status(owner, name, head, ctxRepo, "success", "pass: merge-check.sh passed", "")
+	w.forge.required = []string{ctxGate, ctxRepo}
+	w.h.Checked(verdict(repo, number, head, "pass"))
+	w.settleAll()
+	id := IDOf(repo, head)
+	if s := w.state(id); s != Ready {
+		t.Fatalf("a passing verdict left it %s", s)
+	}
+	return id
+}
+
+func TestARecheckPutsTheMergeCheckBackToPendingBeforeItAsks(t *testing.T) {
+	w := newWorld(t)
+	id := heldGreen(t, w, "novox/app", 7)
+	at := "novox/app@" + head + " "
+	var whenAsked map[string]string
+	w.ctl.onCheck = func() {
+		w.forge.mu.Lock()
+		defer w.forge.mu.Unlock()
+		whenAsked = map[string]string{ctxGate: w.forge.statuses[at+ctxGate], ctxRepo: w.forge.statuses[at+ctxRepo]}
+	}
+	if _, err := w.h.Recheck(id, "main moved on", "a person"); err != nil {
+		t.Fatal(err)
+	}
+	if len(w.ctl.checks) != 1 {
+		t.Fatalf("its check was asked %v", w.ctl.checks)
+	}
+	for _, c := range []string{ctxGate, ctxRepo} {
+		if !strings.HasPrefix(whenAsked[c], "pending checking again: main moved on") {
+			t.Fatalf("as its check was asked, the forge held %s %q: the old verdict still lets it merge", c, whenAsked[c])
+		}
+	}
+	checks, err := w.forge.Statuses("novox", "app", 0, head, "main")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if checks.Merge.Mergeable == nil || *checks.Merge.Mergeable {
+		t.Fatalf("rechecked, the forge would still merge it: %+v", checks.Merge)
+	}
+	if s := w.state(id); s != Proposed {
+		t.Fatalf("rechecked, it is %s", s)
+	}
+}
+
+// Only the statuses the head holds are put back: a verdict that says no repository check would leave a
+// pending one standing for ever.
+func TestARecheckLeavesAStatusTheHeadNeverHadAlone(t *testing.T) {
+	w := newWorld(t)
+	w.h.PullUpdated(pr("novox/app", 8, head, "feat/y", "src/a.go"))
+	_ = w.forge.Status("novox", "app", head, ctxGate, "failure", "fail: the gate said fail", "")
+	w.h.Checked(verdict("novox/app", 8, head, "fail"))
+	w.settleAll()
+	id := IDOf("novox/app", head)
+	if _, err := w.h.Recheck(id, "flaky", "a person"); err != nil {
+		t.Fatal(err)
+	}
+	if s := w.forge.statuses["novox/app@"+head+" "+ctxGate]; !strings.HasPrefix(s, "pending checking again: flaky") {
+		t.Fatalf("the gate is %q", s)
+	}
+	if s, set := w.forge.statuses["novox/app@"+head+" "+ctxRepo]; set {
+		t.Fatalf("a repository check the head never had was set: %q", s)
+	}
+}
+
+// A forge that cannot be told refuses the recheck whole: nothing is asked on a stale verdict, and the
+// delivery stays where it stood, for the person to ask again.
+func TestARecheckTheForgeCannotTakeIsRefused(t *testing.T) {
+	w := newWorld(t)
+	id := heldGreen(t, w, "novox/app", 9)
+	w.forge.down = true
+	if _, err := w.h.Recheck(id, "main moved on", "a person"); err == nil {
+		t.Fatal("a recheck whose statuses could not be put back was taken")
+	}
+	if len(w.ctl.checks) != 0 {
+		t.Fatalf("its check was asked on a stale verdict: %v", w.ctl.checks)
+	}
+	if s := w.state(id); s != Ready {
+		t.Fatalf("refused, it moved to %s", s)
+	}
+}

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

@@ -191,6 +191,12 @@ func (h *Holder) Stalled() []StalledLine {
 }
 
 // Recheck is the `recheck` verb: a rejected or ready delivery proposed again, and its check asked again.
+//
+// A recheck asks of the head the forge already judged, and the forge keeps the newest status of each
+// context on it: left alone, the old verdict stands, and a branch that requires it lets the pull request merge
+// on it while the fresh check runs. So the merge check's statuses are put back to pending first — by the
+// forge's holder, which keeps them — and only then is the check asked. A forge that cannot be told refuses the
+// recheck whole: the delivery stays where it stood, and nothing is asked on a stale verdict.
 func (h *Holder) Recheck(id, why, by string) (string, error) {
 	h.mu.Lock()
 	d := h.deliveries[id]
@@ -198,10 +204,24 @@ func (h *Holder) Recheck(id, why, by string) (string, error) {
 		h.mu.Unlock()
 		return "", fmt.Errorf("no delivery %q", id)
 	}
+	// Whether the table takes it, tried on a copy: nothing is said on the forge for a recheck it refuses.
+	probe := *d
+	probe.Transitions = append([]Transition(nil), d.Transitions...)
+	if _, err := Apply(&probe, EvRecheck, true, Facts{Now: h.Now(), Why: why, By: by}); err != nil {
+		h.mu.Unlock()
+		return "", err
+	}
+	owner, repo, commit, page := d.Owner(), d.Repo(), d.Commit, d.HTMLURL
+	h.mu.Unlock()
+	if err := h.checkingAgain(owner, repo, commit, page, why); err != nil {
+		return "", fmt.Errorf("%s is not rechecked: %v; its merge check must say pending before it is asked again, "+
+			"so nothing merges on the old verdict — ask again once the forge answers", id, err)
+	}
+	h.mu.Lock()
 	t, err := Apply(d, EvRecheck, true, Facts{Now: h.Now(), Why: why, By: by})
 	if err != nil {
 		h.mu.Unlock()
-		return "", err
+		return "", fmt.Errorf("%v (its merge check says pending; the forge's next announcement asks it)", err)
 	}
 	d.Check = nil
 	h.moved(d, []Transition{t})
@@ -212,7 +232,34 @@ func (h *Holder) Recheck(id, why, by string) (string, error) {
 		return "", fmt.Errorf("%s is proposed again and its check could not be asked yet (%v); the forge's next "+
 			"announcement asks it", id, err)
 	}
-	return fmt.Sprintf("%s is proposed again; its check was asked (%s)", id, asked), nil
+	return fmt.Sprintf("%s is proposed again, its merge check pending; its check was asked (%s)", id, asked), 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
+// would leave a pending one standing for ever. Asked of the forge's holder, which keeps them; it takes
+// pending on them and nothing else, a verdict being the controller's alone (novox/hq issue 293).
+func (h *Holder) checkingAgain(owner, repo, commit, page, why string) error {
+	if h.Forge == nil {
+		return errors.New("no forge")
+	}
+	contexts := []string{ctxGate, ctxRepo}
+	if held, err := h.Forge.Statuses(owner, repo, 0, commit, ""); err == nil {
+		contexts = []string{ctxGate}
+		for _, s := range held.Statuses {
+			if s.Context == ctxRepo {
+				contexts = append(contexts, ctxRepo)
+			}
+		}
+	}
+	line := clip(oneLine("checking again: "+why), 140)
+	for _, c := range contexts {
+		if err := h.Forge.Status(owner, repo, commit, c, "pending", line, page); err != nil {
+			return fmt.Errorf("the forge did not set %s pending on %s: %w", c, shortOf(commit, 8), err)
+		}
+	}
+	return nil
 }
 
 // Release is the `release` verb: a held delivery goes on, on a person's word.