From 74f912e1fb4b43da7ef6306cb1de835509d25b57 Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 10:11:26 +0200 Subject: [PATCH] Put the merge check back to pending before a recheck asks it (hq issue 305) A recheck asks of the head the forge already judged, and the forge keeps the newest status of each context on it. Nobody reset mesh/merge-gate or mesh/repo-check, so the old green stood and branch protection would merge on it while the fresh check ran; rechecked heads did turn red. mesh-delivery's recheck now asks the forge's holder to set the gate, and the repository check when the head holds one, to pending ("checking again: ") before it asks the controller; a forge that cannot be told refuses the recheck whole. The gitea holder takes pending, and only pending, on the merge check's statuses by hand: no verdict, so issue 293 still holds. --- modules/gitea/delivery.ts | 11 ++- modules/gitea/test/delivery.test.ts | 4 + modules/gitea/tools/index.ts | 2 +- .../cmd/mesh-delivery/fakes_test.go | 5 + .../mesh-delivery/cmd/mesh-delivery/main.go | 3 +- .../cmd/mesh-delivery/recheck_test.go | 97 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/verbs.go | 51 +++++++++- 7 files changed, 166 insertions(+), 7 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/recheck_test.go diff --git a/modules/gitea/delivery.ts b/modules/gitea/delivery.ts index 9116dfb9..ec667ade 100644 --- a/modules/gitea/delivery.ts +++ b/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, diff --git a/modules/gitea/test/delivery.test.ts b/modules/gitea/test/delivery.test.ts index 5e87f692..623ec204 100644 --- a/modules/gitea/test/delivery.test.ts +++ b/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); }); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index b6ba570b..f18281d8 100644 --- a/modules/gitea/tools/index.ts +++ b/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" }, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index 3ed38658..f27d4707 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/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 diff --git a/modules/mesh-delivery/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go index a718196d..8a14bd1b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/main.go +++ b/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) { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/recheck_test.go b/modules/mesh-delivery/cmd/mesh-delivery/recheck_test.go new file mode 100644 index 00000000..81d618d5 --- /dev/null +++ b/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) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index f3edf4ca..aaf01f0f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/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.