From a32b7af3fd8a27114f6f91bb73c423b42e283ade Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 23:50:25 +0200 Subject: [PATCH] Ask a refused merge again when its checks turn green on the forge (issue 1144) --- .../mesh-delivery/cmd/mesh-delivery/events.go | 31 ++++++- .../mesh-delivery/cmd/mesh-delivery/ready.go | 3 +- .../cmd/mesh-delivery/ready_test.go | 84 +++++++++++++++++++ .../cmd/mesh-delivery/timers_test.go | 1 + 4 files changed, 116 insertions(+), 3 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events.go b/modules/mesh-delivery/cmd/mesh-delivery/events.go index 5e49aa41..9de213c3 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/events.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/events.go @@ -2,6 +2,7 @@ package main import ( "slices" + "strings" "git.novox.be/novox/mesh-sdk/go/asks" ) @@ -51,8 +52,34 @@ func (h *Holder) AskDecided(id, state string, w *asks.Warrant) { // CommitStatus takes a commit status set on the forge: one of the merge checks (mesh/merge-gate, mesh/repo-check) says a // check started, answered, or was put back, so the forge's open pull requests are read to see which heads still wait on // one (SeeOpenPulls). A status of any other context shows nothing the merge needs. +// +// A merge check turning success for the head an approved, ready, not-yet-merged delivery's merge was refused at is +// itself an event that decides whether it may merge (issue 1144): the merge tool may have refused it on a check still +// pending, which this status answers. The merge is asked again at once (mergeAgainNow), under the lock, so the next +// look asks it; no timer does this (issue 1115). A status of another context, another commit or a state other than +// success changes nothing. func (h *Holder) CommitStatus(e ForgeStatusEvent) { - if slices.Contains(mergeChecks, e.Context) { - h.wantForge() + if !slices.Contains(mergeChecks, e.Context) { + return + } + h.wantForge() + if e.State != "success" { + return + } + h.mu.Lock() + defer h.mu.Unlock() + repository := strings.ToLower(e.Owner + "/" + e.Repo) + for _, d := range h.deliveries { + if !strings.EqualFold(d.Repository, repository) || d.Ready == nil || d.Ready.Merged != nil || d.MergedAs != "" || + d.State.Final() { + continue + } + head := d.Commit + if sha := d.Ready.Heads[d.ID]; sha != "" { + head = sha + } + if sameCommit(head, e.SHA) { + h.mergeAgainNow(d) // its approved head's check turned success on the forge: the merge is asked again at once + } } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 358d213c..8417ba16 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -427,7 +427,8 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { case r.RefusedSince != nil && now.Sub(*r.RefusedSince) >= readyRefusedFor: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: readyRefusedFor.String(), - H2: "none: the merge is asked again after each verdict, Approve or composed verdict, and each minute — the operator's", + H2: "none: the merge is asked again on a verdict, an Approve, a group's composed check or a merge check's " + + "status turning success — the operator's", Says: fmt.Sprintf("%s#%d is approved at %s, and the merge tool has refused it since %s: %s", d.Repository, d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go index c84a9286..0f09e359 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go @@ -67,6 +67,8 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { } } +// 1144/AC3: a refused merge is not asked again by any timer; only an event (here, mergeAgainNow called directly, as a +// verdict, an Approve or a composed check does) asks it again. func TestAMergeRefusedIsSaidAtOnceAndAskedAgainOnlyOnAnEvent(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) @@ -93,6 +95,88 @@ func TestAMergeRefusedIsSaidAtOnceAndAskedAgainOnlyOnAnEvent(t *testing.T) { } } +// 1144/AC1: a refused merge, approved and ready, is asked again at once when the forge's forge.status says one of +// the merge checks (mesh/merge-gate, mesh/repo-check) is success for the commit the merge was refused at — the +// delivery's approved head — and merges once the tool accepts (novox/hq mesh-catalog#366: the verdict reached this +// owner before the forge wrote the statuses, the merge asked then was refused on a pending check, and nothing asked +// it again when the statuses turned success). +func TestAMergeRefusedIsAskedAgainWhenItsCheckTurnsSuccessOnTheForge(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: mesh/repo-check (pending)") + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 1 { + t.Fatalf("not asked once, refused: %v", w.forge.merges) + } + w.forge.refuse = nil + w.h.CommitStatus(ForgeStatusEvent{Owner: "novox", Repo: "app", SHA: readyHead, Context: "mesh/repo-check", State: "success"}) + w.settleAll() + if len(w.forge.merges) != 2 { + t.Fatalf("not asked again when its check turned success: %v", w.forge.merges) + } + if w.delivery(IDOf("novox/app", readyHead)).Ready.Merged == nil { + t.Fatalf("it did not merge once the tool accepted") + } +} + +// 1144/AC2: a status of another context, of another commit, or a state other than success changes nothing for a +// refused merge: it is not asked again. +func TestAMergeRefusedIsNotAskedAgainOnAStatusThatDoesNotMatch(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: mesh/repo-check (pending)") + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + if len(w.forge.merges) != 1 { + t.Fatalf("not asked once, refused: %v", w.forge.merges) + } + w.forge.refuse = nil + for _, e := range []ForgeStatusEvent{ + {Owner: "novox", Repo: "app", SHA: readyHead, Context: "some/other-check", State: "success"}, // another context + {Owner: "novox", Repo: "app", SHA: pushed, Context: "mesh/repo-check", State: "success"}, // another commit + {Owner: "novox", Repo: "app", SHA: readyHead, Context: "mesh/repo-check", State: "pending"}, // another state + } { + w.h.CommitStatus(e) + w.settleAll() + } + if len(w.forge.merges) != 1 { + t.Fatalf("asked again on a status that should change nothing: %v", w.forge.merges) + } + if w.delivery(IDOf("novox/app", readyHead)).Ready.Merged != nil { + t.Fatalf("merged on a status that should change nothing") + } +} + +// 1144/AC4: the stalled line for a refused approved head names the events that ask the merge again, and no longer +// says "each minute" (removed by issue 1115; false since then — there is no timer). +func TestTheStalledLineForARefusedApprovedHeadNamesTheEventsThatAskTheMergeAgain(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: mesh/repo-check (pending)") + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} + w.h.SeeOpenPulls() + w.settleAll() + lines := w.h.Stalled() + if len(lines) != 1 { + t.Fatalf("wanted one stalled line, got %+v", lines) + } + h2 := lines[0].H2 + if strings.Contains(h2, "each minute") { + t.Fatalf("the stalled line still says 'each minute', false since issue 1115: %q", h2) + } + for _, want := range []string{"verdict", "Approve", "composed check", "status"} { + if !strings.Contains(h2, want) { + t.Fatalf("the stalled line does not name %q as asking the merge again: %q", want, h2) + } + } +} + func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing.T) { w := newWorld(t) w.op.auto = false diff --git a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go index 648c88cd..370d1127 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go @@ -55,6 +55,7 @@ func TestAProposedDeliveryWhoseHeadHoldsAPassingVerdictAtTheForgeAdoptsItAndAsks } } +// 1144/AC3: no timer asks a refused merge again; only an event does (here, mergeAgainNow called directly). func TestAMergeTheForgeRefusedIsAskedAgainOnlyOnAnEventWithNoTimer(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go"))