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 9e0f7e96..1e08b31c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -148,8 +148,32 @@ func touchesSaid(d *Delivery) string { return strings.Join(d.Check.Modules, ", ") } +// checkClause is the check's state as the ask's summary says it after the pull request's or the group's name: sep and +// "passed its check", or ": its check has no verdict yet: it is running (since 21:05 UTC)" and the like. +func checkClause(c checkSaid, sep string) string { + if c.passed() { + return sep + c.Words + } + return ": " + c.Words +} + +// approveCaveat is what an Approve does beyond the merge while the check has not passed: it merges before the check ends, +// or although it failed (the check gates no merge, check-gates-merge false). Empty once it passed. +func approveCaveat(c checkSaid) string { + switch c.Key { + case checkSaidPass: + return "" + case checkSaidFail: + return " although its check failed" + } + return " now, before its check ends" +} + // composeMergeAsk is the ask for a delivery, or for a group's members together. -func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, heads map[string]string, now time.Time) asks.Ask { +// check is the state of the check at the heads it covers (checkAt): the ask says "passed its check" only when it did, and +// otherwise that Approve merges before the check ends, or although it failed (issue 1733). +func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, heads map[string]string, check checkSaid, + now time.Time) asks.Ask { var a asks.Ask a.ID, a.Who, a.Expires = id, asks.Operator, now.Add(AskLasts) a.OnExpiry = "nothing is merged; the ask is made again" @@ -170,8 +194,8 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, } if group == "" { a.Headline = phone("Merge into main: "+pullSaid(d)+" "+d.Title, asks.HeadlineLength) - summary := fmt.Sprintf("%s %s passed its checks. Approve merges it into main at %s; Decline leaves it unmerged.", - pullSaid(d), d.Title, shortOf(d.Commit, 12)) + summary := fmt.Sprintf("%s %s%s. Approve merges it into main at %s%s; Decline leaves it unmerged.", + pullSaid(d), d.Title, checkClause(check, " "), shortOf(d.Commit, 12), approveCaveat(check)) if unreviewed != "" { summary = strings.ToUpper(unreviewed[:1]) + unreviewed[1:] + ". " + summary } @@ -187,8 +211,12 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, names = append(names, pullSaid(m)) } a.Headline = phone("Merge into main: group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength) - summary := fmt.Sprintf("The group %s, %d pull requests, passed its checks. Approve merges all of them into main in one pass, at the heads named; Decline leaves them unmerged.", - group, len(members)) + caveat := approveCaveat(check) + if caveat != "" { + caveat = "," + caveat + } + summary := fmt.Sprintf("The group %s, %d pull requests%s. Approve merges all of them into main in one pass, at the heads named%s; Decline leaves them unmerged.", + group, len(members), checkClause(check, ", "), caveat) if unreviewed != "" { summary = strings.ToUpper(unreviewed[:1]) + unreviewed[1:] + ". " + summary } @@ -310,6 +338,10 @@ func (h *Holder) endStaleAsks() { h.cancels = append(h.cancels, h.dropAsk(r.AskID, "its head "+shortOf(d.Commit, 12)+" is no longer the pull request's head, or the pull request closed")) case !h.headsHold(d, r): h.cancels = append(h.cancels, h.dropAsk(r.AskID, "a head it covers moved on: the heads it covers are no longer the group's heads")) + case r.Approved == nil && r.AskID != "" && r.AskCheck != "" && !h.checkGates() && h.checkMoved(d, r.AskCheck): + // The check gates no merge, so the ask was made before its verdict: once the check passes or fails (or a pass it + // named is gone), the ask is asked again in words that say so (issue 1733). Running and not started say the same. + h.cancels = append(h.cancels, h.withdrawAsk(r.AskID, "the check at its heads is now: "+h.checkAt(h.askMembers(d)).Words)) case r.Approved == nil && r.AskID != "" && !r.AskedAt.IsZero() && !h.Now().Before(r.AskedAt.Add(AskLasts)): // The ask lapsed at the router with nobody's answer, and nothing in the gate's inputs says so: it is taken away and // written again, which the teller asks afresh (ADR 0441 §1). @@ -318,6 +350,16 @@ func (h *Holder) endStaleAsks() { } } +// checkMoved is whether the check at the heads of d's ask reached a verdict its pending ask does not say: it passed or failed +// since, or the pass it said is gone. Under the lock. +func (h *Holder) checkMoved(d *Delivery, said string) bool { + now := h.checkAt(h.askMembers(d)).Key + if now == said { + return false + } + return now == checkSaidPass || now == checkSaidFail || said == checkSaidPass || said == checkSaidFail +} + // approval is what a delivery that may merge waits on for the operator: empty once its head is approved, and the ask // to make when none is made yet. Under the lock. func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { @@ -368,19 +410,20 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { asking = max(asking, m.Ready.Asking+1) } } - ask := composeMergeAsk(sdk.AskingID(id, asking), d, group, members, heads, now) + check := h.checkAt(members) + ask := composeMergeAsk(sdk.AskingID(id, asking), d, group, members, heads, check, now) taken := []*Delivery{} var reads []memberRead for _, m := range members { h.runOf(m, now) m.Ready.AskID, m.Ready.Ask, m.Ready.Heads, m.Ready.AskedAt, m.Ready.Asks = id, &ask, heads, now, max(m.Ready.Asks, r.Asks) - m.Ready.AskedKey, m.Ready.Asking = id, asking + m.Ready.AskedKey, m.Ready.Asking, m.Ready.AskCheck = id, asking, check.Key h.keep(m) taken = append(taken, m) reads = append(reads, readOf(m, m.Commit)) } - h.Logf("[mesh-delivery] %s passed its checks: the operator is asked to Approve its merge into main (%s, ask %d)", - d.ID, id, r.Asks) + h.Logf("[mesh-delivery] %s (%s): the operator is asked to Approve its merge into main (%s, ask %d)", + d.ID, check.Words, id, r.Asks) gid := "" if group != "" { gid = d.Group @@ -433,7 +476,7 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { h.mu.Lock() defer h.mu.Unlock() undo() - h.Logf("[mesh-delivery] %s passed its checks and the operator is not asked: %s", d.ID, says) + h.Logf("[mesh-delivery] %s (%s): the operator is not asked: %s", d.ID, check.Words, says) h.lookAt(h.Now(), "an ask not made: the forge account cannot write a repository it covers") return } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go index b38e721f..a3055a32 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/merges.go @@ -287,7 +287,7 @@ func (h *Holder) ForgeMerged(f sdk.Forge) { r, now := d.Ready, h.Now() d.Owed = append(d.Owed, Effect{Kind: EffectEmit, Event: "merged", Line: string(mustJSON(mergedEvent(d, f.Head))), Since: now}) r.Merged, r.Answer, r.RefusedSince, r.merging, r.MergeCommit = &now, "merged", nil, false, f.Merge - h.Logf("[mesh-delivery] %s merged on the operator's Approve at %s, its check passed against its trunk's head", d.ID, shortOf(f.Head, 12)) + h.Logf("[mesh-delivery] %s merged on the operator's Approve at %s (%s)", d.ID, shortOf(f.Head, 12), h.checkAt(h.askMembers(d)).Words) h.keep(d) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go index 10151f1f..76386e98 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus.go @@ -302,3 +302,72 @@ func checkerRefusal(id string) error { "its trunk or its group, and nothing is asked of it here (novox/hq ADR 0447). Read where its check stands with the "+ "checker's checker_pull tool", id) } + +// The states of the check at an ask's heads, as the ask says them (issue 1733). +const ( + checkSaidPass = "pass" + checkSaidRunning = "running" + checkSaidNone = "none" + checkSaidFail = "fail" +) + +// checkSaid is the state of the check at the heads an ask covers, and its words for the ask: what the checker's verdict at +// exactly those heads says, never what the gate would like it to say (issue 1733). Key is one of the checkSaid states. +type checkSaid struct { + Key string + Words string +} + +// passed is whether the check at the heads passed. +func (c checkSaid) passed() bool { return c.Key == checkSaidPass } + +// checkAt reads the check at these members' heads: the checker's verdict for exactly those heads (its verdicts bucket, +// keyed by their hash), and, while none is held, the forge's mesh/check at a member's head (a failure or a pending one +// there). "passed its check" is said only for a passing verdict at these heads. Under the lock. +func (h *Holder) checkAt(members []*Delivery) checkSaid { + v, hash, ok := h.checkerAt(members) + if ok { + switch v.State { + case checkerPass: + return checkSaid{Key: checkSaidPass, Words: "passed its check"} + case checkerRunning: + words := "its check has no verdict yet: it is running" + if since, held := h.runningSince[hash]; held && !since.IsZero() { + words += " (since " + since.UTC().Format("15:04") + " UTC)" + } + return checkSaid{Key: checkSaidRunning, Words: words} + default: + why := firstOf(oneLine(v.Failed.Test), oneLine(v.Failed.Words), v.State) + if v.Failed.Test != "" && v.Failed.Words != "" { + why = oneLine(v.Failed.Test) + ": " + oneLine(v.Failed.Words) + } + return checkSaid{Key: checkSaidFail, Words: "its check failed: " + clip(why, 120)} + } + } + for _, m := range members { + switch m.CheckState { + case "failure", "error": + why := "mesh/check is " + m.CheckState + " at " + shortOf(m.Commit, 12) + if m.Check != nil && m.Check.Summary != "" { + why = oneLine(m.Check.Summary) + } + return checkSaid{Key: checkSaidFail, Words: "its check failed: " + clip(why, 120)} + } + } + succeeded := 0 + for _, m := range members { + switch m.CheckState { + case "pending": + return checkSaid{Key: checkSaidRunning, Words: "its check has no verdict yet: it is running"} + case "success": + succeeded++ + } + } + switch { + case succeeded > 0 && len(members) == 1: + return checkSaid{Key: checkSaidPass, Words: "passed its check"} // its head's mesh/check is the checker's verdict at it + case succeeded == len(members) && succeeded > 0: + return checkSaid{Key: checkSaidNone, Words: "its members' own checks passed; the check of their heads together has not answered"} + } + return checkSaid{Key: checkSaidNone, Words: "its check has no verdict yet"} +} 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 4b96bc11..aee84748 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -115,6 +115,10 @@ type ReadyRun struct { Heads map[string]string `json:"heads,omitempty"` AskedAt time.Time `json:"asked_at"` Asks int `json:"asks,omitempty"` + // AskCheck is the state of the check at the heads the pending ask names, as its words said it (checkAt: pass, running, + // none or fail; issue 1733): while the check gates no merge, an ask whose check reached a verdict since is asked again + // in new words, so it never keeps a claim the check no longer holds. + AskCheck string `json:"ask_check,omitempty"` // Approved and Declined are when the operator chose, Warrant what the router's warrant says of who and how; Refused // why an answer was not taken (it did not hold to the ask). Approved *time.Time `json:"approved,omitempty"` @@ -186,7 +190,8 @@ func (h *Holder) readyAsks(now time.Time) []func() { if d.Ready == nil && d.State == Ready { h.runOf(d, now) h.Poke() // its check, if none is asked, is worth asking now (askCheck) - h.Logf("[mesh-delivery] %s: its checks passed; the operator's Approve of its merge into main is asked once it may merge", d.ID) + h.Logf("[mesh-delivery] %s is ready (%s); the operator's Approve of its merge into main is asked once it may merge", + d.ID, h.checkAt([]*Delivery{d}).Words) } if a := h.sayAfterLine(d); a != nil { asks = append(asks, a) @@ -227,11 +232,12 @@ func (h *Holder) readyAsks(now time.Time) []func() { if d.Base != MergeBranch { if r.OtherBase != d.Base { r.OtherBase = d.Base - h.Logf("[mesh-delivery] %s passed its checks and is not asked: it merges into %s, not %s", d.ID, d.Base, MergeBranch) + check := h.checkAt([]*Delivery{d}).Words // ready need not mean passed: the check may gate no merge (issue 1733) + h.Logf("[mesh-delivery] %s (%s) is not asked: it merges into %s, not %s", d.ID, check, d.Base, MergeBranch) h.keep(d) - asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request's checks passed at "+ - "%s, and it merges into %s. Only a merge into %s is this owner's; a merge into another branch is its "+ - "author's (novox/hq ADR 0398 §4).", d.Commit, d.Base, MergeBranch))) + asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request is at %s (%s), "+ + "and it merges into %s. Only a merge into %s is this owner's; a merge into another branch is its "+ + "author's (novox/hq ADR 0398 §4).", d.Commit, check, d.Base, MergeBranch))) } continue } @@ -524,13 +530,14 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { case d.Base == "" && d.Number != 0 && now.Sub(r.Seen) >= headHeardBound: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), Bound: headHeardBound.String(), H2: "none: a later announcement of the pull request's head (the merge tool announces it again, the forge's announcer restarts, or a new push) or an edit of its description; a recheck does not fill it — the operator's", - Says: fmt.Sprintf("%s#%d passed its checks at %s and waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), waitsForHead)}) + Says: fmt.Sprintf("%s#%d at %s (%s) waits for %s", d.Repository, d.Number, shortOf(r.Head, 12), + h.checkAt([]*Delivery{d}).Words, waitsForHead)}) case r.CannotWrite != "" && r.Approved == nil && r.CannotWriteSince != nil: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.CannotWriteSince).Round(time.Second).String(), Bound: "0s", H2: "none: the forge account given write on the repository (gitea_grant_write; the gitea module grants it itself once a module of it is registered), which asks the operator then", - Says: fmt.Sprintf("%s#%d passed its checks at %s and the operator is not asked for its Approve: %s", - d.Repository, d.Number, shortOf(r.Head, 12), r.CannotWrite)}) + Says: fmt.Sprintf("%s#%d at %s (%s): the operator is not asked for its Approve: %s", + d.Repository, d.Number, shortOf(r.Head, 12), h.checkAt([]*Delivery{d}).Words, r.CannotWrite)}) case r.Failed != "" && d.State == Rejected: // Its cure is its author's new head: listed here at once (`show`), told on the pull request at once, and the // operator's only once no new head came within the author's bound (novox/hq ADR 0505 §5). 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 e15cc1e1..6e139b14 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,11 +1,15 @@ package main import ( + "encoding/json" "errors" "sort" "strings" "testing" "time" + + "git.novox.be/novox/mesh-sdk/go/asks" + sdk "git.novox.be/novox/mesh-sdk/go/delivery" ) // **A pull request whose checks passed and whose merge the operator approved is merged by this owner** (novox/hq issues @@ -65,6 +69,73 @@ func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { t.Fatalf("merged again: %v", w.forge.merges) } + // 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. + for _, row := range []struct { + name, state, failed string + gated bool + 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, + 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"}}, + } { + t.Run(row.name, func(t *testing.T) { + v := newWorld(t) + if !row.gated { + ungated(v) + } + v.op.auto = false + 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) + } + if row.gated { + v.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + } + v.settleAll() + if !row.gated && row.state != "" { + put() + 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) + } + e := v.op.ask(open[0]).Explanation + for _, w := range row.want { + if !strings.Contains(e, w) { + t.Fatalf("the ask does not say %q:\n%s", w, e) + } + } + for _, w := range row.not { + if strings.Contains(e, w) { + t.Fatalf("the ask says %q:\n%s", w, e) + } + } + if err := asks.Glance(e); err != nil { + t.Fatal(err) + } + }) + } + // check-gates-merge false (the operator, 2026-10-25 22:56): a reviewed pull request is asked with no verdict, another // merge moving its trunk puts it out of nothing, a failed verdict is shown on it and holds nothing, and the gate's // status and show say the mode. @@ -255,6 +326,11 @@ func TestAGroupMergesEveryMemberInOnePassOnlyOnceComposedAndApproved(t *testing. if u.op.count() != 1 { t.Fatalf("ungated, the group is asked %d times; wanted once", u.op.count()) } + // 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) + } u.op.answer(u.op.open()[0], OptionApprove) u.settleAll() if len(u.forge.merges) != 2 {