diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go index 1e08b31c..e57a47b0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -411,6 +411,9 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { } } check := h.checkAt(members) + if !h.checkGates() { + check = unansweredWords(check) // asked with no verdict only past its bound (verdictWaits) + } ask := composeMergeAsk(sdk.AskingID(id, asking), d, group, members, heads, check, now) taken := []*Delivery{} var reads []memberRead diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go index 1bec05c3..2001a2a0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/helpers_test.go @@ -1,6 +1,11 @@ package main -import "time" +import ( + "encoding/json" + "time" + + sdk "git.novox.be/novox/mesh-sdk/go/delivery" +) var t0 = time.Date(2026, 10, 6, 12, 0, 0, 0, time.UTC) @@ -27,3 +32,12 @@ func statusOn(w *world, repo, commit string) string { defer w.forge.mu.Unlock() return w.forge.statuses[repo+"@"+commit+" mesh/delivery"] } + +// checkerSays puts the checker's verdict at these heads together into its verdicts bucket, as captured on the bus +// (mesh-delivery-checker, key `.`). +func checkerSays(w *world, group, state, failed string, heads ...sdk.Head) { + hash, _ := sdk.HeadsHash(heads) + value, _ := json.Marshal(map[string]any{"v": 1, "group": group, "heads": hash, "state": state, + "failed": map[string]string{"words": failed}}) + w.h.CheckerVerdictChanged(group+"."+hash, value) +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go index aee84748..fdc97fd3 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -446,9 +446,62 @@ func (h *Holder) ungatedWaits(d *Delivery) string { } } } + if h.covering(d) == nil { + return h.verdictWaits(d) + } return "" } +// askCheckBound is how long an Approve waits for the check's verdict at its heads while the check gates no merge: past it, +// the operator is asked anyway, and the ask says the check has not answered. The checker's own step bound is 30 minutes +// and a check runs several steps; an hour covers a whole check. +const askCheckBound = time.Hour + +// waitsCheckBeforeAsking is how the wait for the check's verdict begins, as `show` and the delivery's wait say it. +const waitsCheckBeforeAsking = "waits its check before asking the operator's Approve: " + +// verdictWaits is why an Approve is not asked yet while the check gates no merge: the check at the heads the ask would cover +// has no verdict (the operator, 2026-10-27 23:54: "if you don't want me to approve it, don't send the approval request"). +// A pass or a failure is asked (a failure in its words: the operator may still merge it), and so is a check that has not +// answered within askCheckBound of the heads becoming ready, which the ask says. For a group the check is the one of all its +// heads together; for a placement change it ends only once every node it places answered (ADR 0559 §5). Under the lock. +func (h *Holder) verdictWaits(d *Delivery) string { + members := h.askMembers(d) + c := h.checkAt(members) + if c.Key == checkSaidPass || c.Key == checkSaidFail { + return "" + } + until := h.readySince(d, members).Add(askCheckBound) + if !h.Now().Before(until) { + return "" + } + h.lookAt(until, "an Approve that waits its check, asked anyway at its bound") + return waitsCheckBeforeAsking + c.Words + "; asked anyway at " + until.UTC().Format("15:04") + " UTC if it has not answered" +} + +// readySince is when the latest of the heads an ask would cover became ready: the bound of verdictWaits counts from it. +func (h *Holder) readySince(d *Delivery, members []*Delivery) time.Time { + since := h.Now() + if d.Ready != nil { + since = d.Ready.Seen + } + for _, m := range members { + if m.Ready != nil && m.Ready.Seen.After(since) { + since = m.Ready.Seen + } + } + return since +} + +// unansweredWords is the check's words in an ask made past askCheckBound with no verdict: it says the check has not answered. +func unansweredWords(c checkSaid) checkSaid { + if c.Key == checkSaidPass || c.Key == checkSaidFail { + return c + } + c.Words += ", and has given none in " + strconv.Itoa(int(askCheckBound.Minutes())) + " minutes, so the operator is asked anyway" + return c +} + // waitsForReview is how a hold for a review begins, as the delivery's state and its mesh/delivery status say it. const waitsForReview = "waits for a review at " diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go index 6e139b14..0249ec4c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready_test.go @@ -1,7 +1,6 @@ package main import ( - "encoding/json" "errors" "sort" "strings" @@ -70,27 +69,32 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { } // 1733: the Approve's ask says the check's state at the heads it names, read from the checker's verdict there: "passed - // its check" only when it passed. While the check gates no merge, the ask is made before any verdict, says that Approve - // merges before the check ends, and is asked again in new words once the check passes or fails; a running check is no - // new word. + // its check" only when it passed. While the check gates no merge, the ask waits for the check's verdict (the operator, + // 2026-10-27 23:54: "if you don't want me to approve it, don't send the approval request"): a pass or a failure is asked, + // the failure in its words; with no verdict nothing is asked and the delivery says it waits its check, until the bound, + // past which it is asked anyway, saying so, and asked again in new words once the check passes. The verdict is the + // checker's verdicts bucket value as captured on the bus (mesh-delivery-checker, `.`). for _, row := range []struct { - name, state, failed string - gated bool - asks int - want, not []string + name, state, failed, then string + gated bool + after time.Duration + asks int + want, not []string }{ {name: "gated, passed", gated: true, state: "pass", asks: 1, want: []string{"app#7 Fix it passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends", "no verdict yet"}}, - {name: "ungated, no verdict", asks: 1, - want: []string{"its check has no verdict yet", "Approve merges it into main at " + readyHead[:12] + " now, before its check ends"}, - not: []string{"passed"}}, - {name: "ungated, running", state: "running", asks: 1, - want: []string{"its check has no verdict yet", "before its check ends"}, not: []string{"passed"}}, - {name: "ungated, then passed", state: "pass", asks: 2, + {name: "ungated, no verdict", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet; asked anyway at "}}, + {name: "ungated, running", state: "running", asks: 0, want: []string{waitsCheckBeforeAsking + "its check has no verdict yet: it is running"}}, + {name: "ungated, running past its bound", state: "running", after: askCheckBound, asks: 1, + want: []string{"its check has no verdict yet: it is running", "has given none in 60 minutes, so the operator is asked anyway", + "Approve merges it into main at " + readyHead[:12] + " now, before its check ends"}, not: []string{"passed"}}, + {name: "ungated, past its bound, then passed", state: "running", after: askCheckBound, then: "pass", asks: 2, + want: []string{"passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends", "anyway"}}, + {name: "ungated, passed", state: "pass", asks: 1, want: []string{"passed its check. Approve merges it into main at " + readyHead[:12] + ";"}, not: []string{"before its check ends"}}, - {name: "ungated, then failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 2, - want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed"}}, + {name: "ungated, failed", state: "fail", failed: "TestThing: wanted 1, got 2", asks: 1, + want: []string{"its check failed: TestThing: wanted 1, got 2", "although its check failed"}, not: []string{"passed", "anyway"}}, } { t.Run(row.name, func(t *testing.T) { v := newWorld(t) @@ -101,23 +105,35 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") p.Title = "Fix it" v.h.PullUpdated(p) - put := func() { - hash, _ := sdk.HeadsHash([]sdk.Head{headOf("novox/app", 7, readyHead)}) - value, _ := json.Marshal(map[string]any{"v": 1, "group": "g", "heads": hash, "state": row.state, - "failed": map[string]string{"words": row.failed}}) - v.h.CheckerVerdictChanged("g."+hash, value) - } + head := headOf("novox/app", 7, readyHead) if row.gated { v.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) } v.settleAll() if !row.gated && row.state != "" { - put() + checkerSays(v, "g", row.state, row.failed, head) + v.settleAll() + } + if row.after > 0 { + v.later(row.after) + v.settleAll() + } + if row.then != "" { + checkerSays(v, "g", row.then, row.failed, head) v.settleAll() } open := v.op.open() - if len(open) != 1 || v.op.count() != row.asks { - t.Fatalf("open asks %v, asked %d times; wanted one open, asked %d times", open, v.op.count(), row.asks) + if v.op.count() != row.asks || len(open) != min(row.asks, 1) { + t.Fatalf("open asks %v, asked %d times; wanted asked %d times", open, v.op.count(), row.asks) + } + if row.asks == 0 { + d := v.delivery(IDOf("novox/app", readyHead)) + for _, w := range row.want { + if d.Ready == nil || !strings.Contains(d.Ready.Waits, w) { + t.Fatalf("the delivery's wait does not say %q: %+v", w, d.Ready) + } + } + return } e := v.op.ask(open[0]).Explanation for _, w := range row.want { @@ -146,8 +162,8 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { u.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) u.h.PullUpdated(pr("novox/app", 8, other, "feat/y", "src/b.go")) u.settleAll() - if u.op.count() != 2 || u.state(IDOf("novox/app", readyHead)) != Ready { - t.Fatalf("ungated, two reviewed pull requests with no verdict: %d asks, %s", u.op.count(), u.state(IDOf("novox/app", readyHead))) + if u.op.count() != 0 || u.state(IDOf("novox/app", readyHead)) != Ready { + t.Fatalf("ungated, two reviewed pull requests with no verdict: %d asks, %s; wanted ready and not asked", u.op.count(), u.state(IDOf("novox/app", readyHead))) } if gates, _ := u.h.Status("listening")["check-gates-merge"].(bool); gates { t.Fatalf("delivery_status does not say the check gates no merge: %v", u.h.Status("listening")) @@ -157,8 +173,12 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { t.Fatalf("show does not say the check gates no merge: %v", shown) } u.h.Checked(judgedAt("novox/app", 8, other, "fail", "t1t1t1t1t1t1")) + checkerSays(u, "feat/y", "fail", "TestB", headOf("novox/app", 8, other)) u.h.PullMerged(merged("novox/app", 7, readyHead, "m7m7m7m7m7m7")) u.settleAll() + if u.op.count() != 1 { + t.Fatalf("ungated, a failed verdict is asked once (the check gates no merge): %d asks", u.op.count()) + } d = u.delivery(IDOf("novox/app", other)) if d.State != Ready || d.Check == nil || d.Check.Passes() { t.Fatalf("ungated, after a failed verdict and a trunk's move: %s, verdict %+v", d.State, d.Check) @@ -323,13 +343,21 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. u.h.PullUpdated(pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go")) u.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) u.settleAll() - if u.op.count() != 1 { - t.Fatalf("ungated, the group is asked %d times; wanted once", u.op.count()) + group := []sdk.Head{headOf("novox/mesh-controller", 1, ctlHead), headOf("novox/mesh-catalog", 2, catHead)} + checkerSays(u, "feat/x", "running", "", group...) + u.settleAll() + // The group's check is the one of its heads together: while it runs, nothing is asked and each member says it waits. + if d := u.delivery(IDOf("novox/mesh-catalog", catHead)); u.op.count() != 0 || d.Ready == nil || + !strings.Contains(d.Ready.Waits, waitsCheckBeforeAsking+"its check has no verdict yet: it is running") { + t.Fatalf("ungated, the group is asked %d times while its check runs; wanted none, and the wait said: %+v", u.op.count(), d.Ready) } - // 1733: the group's ask claims no pass it has not had. - if e := u.op.ask(u.op.open()[0]).Explanation; strings.Contains(e, "passed") || - !strings.Contains(e, "2 pull requests: its check has no verdict yet. Approve merges all of them into main in one pass, at the heads named, now, before its check ends;") { - t.Fatalf("ungated, the group's ask does not say its check has no verdict yet:\n%s", e) + checkerSays(u, "feat/x", "pass", "", group...) + u.settleAll() + if u.op.count() != 1 { + t.Fatalf("ungated, the group is asked %d times once its check passed; wanted once", u.op.count()) + } + if e := u.op.ask(u.op.open()[0]).Explanation; !strings.Contains(e, "2 pull requests, passed its check. Approve merges all of them into main in one pass, at the heads named;") { + t.Fatalf("ungated, the group's ask does not say its check passed:\n%s", e) } u.op.answer(u.op.open()[0], OptionApprove) u.settleAll() diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index 06e16ec7..ef5d1b80 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -99,7 +99,7 @@ }, "check-gates-merge": { "type": "boolean", - "description": "Whether mesh/check gates the merge. True (the default): the operator's Approve is asked, and a merge handed, only on a passing verdict judged against the trunk's head, and a trunk's move puts a verdict out of date (ADR 0447, ADR 0505 §4). False (the operator, 2026-10-25 22:56: \"run the check locally only, but not as a gate, it loses us too much time\"; developers run the unit and integration levels locally before a pull request): a pull request reviewed at its head is ready and its Approve asked at once, with no verdict waited on; no merge is refused for a check judged on an older trunk; a verdict that comes is shown on the pull request and the delivery, never waited on; a group still merges all its members in one pass. Set it false together with the gitea module's check-gates-merge, which lets its merge tool merge an approved head without a passing mesh/check.", + "description": "Whether mesh/check gates the merge. True (the default): the operator's Approve is asked, and a merge handed, only on a passing verdict judged against the trunk's head, and a trunk's move puts a verdict out of date (ADR 0447, ADR 0505 §4). False (the operator, 2026-10-25 22:56: \"run the check locally only, but not as a gate, it loses us too much time\"; developers run the unit and integration levels locally before a pull request): a pull request reviewed at its head is ready, and its Approve is asked once the check at its heads (a group: of all its heads together) has a verdict, a pass or a failure said in the ask, or after 60 minutes with none, said so (the operator, 2026-10-27 23:54: \"if you don't want me to approve it, don't send the approval request\"); an approved merge waits for no verdict; no merge is refused for a check judged on an older trunk; a later verdict is shown on the pull request and the delivery, and asks a pending Approve again in its words; a group still merges all its members in one pass. Set it false together with the gitea module's check-gates-merge, which lets its merge tool merge an approved head without a passing mesh/check.", "trusted": false, "default": true }