Merge pull request 'The gate part reads no pull request description: no review-line wait before the Approve (ADR 0659)' (#630) from fix/gate-reads-no-description into main

This commit was merged in pull request #630.
This commit is contained in:
2026-11-07 22:55:00 +00:00
18 changed files with 87 additions and 560 deletions
@@ -190,30 +190,20 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery,
a.OnExpiry = "nothing is merged; the ask is made again"
a.About = "delivery." + phone(repoName(d.Repository), 40) + "." + strconv.Itoa(d.Number)
a.About = strings.ReplaceAll(a.About, " ", "-")
var reviewers, reaches, details []string
unreviewed := ""
var reaches, details []string
for _, m := range members {
words, ok := reviewWords(m)
if !ok && unreviewed == "" {
unreviewed = pullSaid(m) + " is " + words
}
reviewers = append(reviewers, fmt.Sprintf("%s: %s", pullSaid(m), words))
touches := touchesSaid(m)
reaches = append(reaches, touches)
details = append(details, fmt.Sprintf("%s %q at %s: issues: %s; review: %s; touches: %s",
pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), words, touches))
details = append(details, fmt.Sprintf("%s %q at %s: issues: %s; touches: %s",
pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), touches))
}
if group == "" {
a.Headline = phone(mergeHeadline(check)+pullSaid(d)+" "+d.Title, occ.HeadlineLength)
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
}
a.Explanation = strings.Join([]string{
phone(summary, occ.SummaryLength),
phone("Issues: "+issuesSaid(members), occ.PointLength),
phone("Review: "+reviewers[0], occ.PointLength),
phone("Touches: "+reaches[0], occ.PointLength),
}, "\n")
} else {
@@ -228,13 +218,9 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery,
}
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
}
a.Explanation = strings.Join([]string{
phone(summary, occ.SummaryLength),
phone("Issues: "+issuesSaid(members), occ.PointLength),
phone("Review: "+strings.Join(reviewers, "; "), occ.PointLength),
phone("Touches: "+strings.Join(reaches, "; "), occ.PointLength),
}, "\n")
}
@@ -471,7 +457,7 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) {
}
r.Asks++
// Each asking at these heads has its own router id (mdc.AskingID): the router keeps an ask by its id and drops a second ask
// under it as a redelivery, so an ask written again at unchanged heads (a lapse, a status or the review record changing) is
// under it as a redelivery, so an ask written again at unchanged heads (a lapse, or a status changing) is
// the next asking, with a new id (review round 1, G2; issue 1370).
asking := 1
for _, m := range members {
@@ -21,7 +21,6 @@ func markedGreen(t *testing.T) *world {
w.op.auto = false
p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
p.Title = "Fix the apostrophe (issue 948, issue 12)"
p.Body = "Reviewed-by: reviewer@shanks at " + readyHead
w.h.PullUpdated(p)
w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)}
@@ -45,7 +44,7 @@ func TestAMarkedGreenDeliveryIntoMainIsAskedAndNotMergedUntilApproved(t *testing
len(a.Options) != 2 || a.Options[0].ID != OptionApprove || a.Options[1].ID != OptionDecline {
t.Fatalf("the ask is not the merge ask: %+v", a)
}
for _, want := range []string{"issue 948", "issue 12", "reviewed by reviewer on shanks", "Touches: "} {
for _, want := range []string{"issue 948", "issue 12", "Touches: "} {
if !strings.Contains(a.Explanation, want) {
t.Fatalf("the ask does not say %q: %s", want, a.Explanation)
}
@@ -526,31 +526,18 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB
})
b.checkPasses(t, rt, "fix/x", "a1a1a1a1a1a1", judgedPull{"novox/app", 7, head})
// No review names the head, so nothing is asked (ADR 0496, issue 1803): the delivery waits for a review.
rt.until("the delivery to wait for a review", eventWithin, func() bool {
e, err := b.kvs["pulls"].Get(ctx, "novox.app.7")
var k PullKey
return err == nil && json.Unmarshal(e.Value(), &k) == nil && k.Verdict == "pass"
})
time.Sleep(time.Second)
if key := onlyAsk(b); key != "" {
t.Fatalf("a head with no review was asked: %s", key)
}
// The description is edited to add the review record (issue 1079): the forge module's edit event carries it, and the
// ask is written into the gate's bucket asks within seconds, with no label, comment or mark written by anyone, and
// nothing is asked of a router.
// The description holds no review line and nothing is waited for: the ask is written into the gate's bucket asks within
// seconds of the verdict, with no label, comment or mark written by anyone (the gate reads no description, ADR 0493 §6, ADR 0659).
edit := time.Now()
b.publish(t, "mesh.mod.gitea.event.forge.pull-request.edited", map[string]any{"owner": "novox", "repo": "app", "number": 7,
"title": announced.Title, "head_sha": head, "base": "main", "body": "Reviewed-by: lead@shanks at " + head})
var key string
rt.until("the ask to be written", eventWithin, func() bool { key = onlyAsk(b); return key != "" })
ask, err := askOf(askAt(t, b, key))
if err != nil {
t.Fatal(err)
}
t.Logf("the ask was written %s after the edit's event", time.Since(edit))
if !strings.Contains(ask.Explanation, "reviewed by lead on shanks") || ask.ID != mdc.AskID(key) {
t.Fatalf("the ask does not say the record: %s", ask.Explanation)
t.Logf("the ask was written %s after the verdict", time.Since(edit))
if strings.Contains(strings.ToLower(ask.Explanation), "review") || ask.ID != mdc.AskID(key) {
t.Fatalf("the ask names a review or has the wrong id: %s", ask.Explanation)
}
if len(rt.asksOf("gitea.gitea_merge_pull_request")) != 0 {
t.Fatalf("merged before the Approve")
@@ -189,7 +189,7 @@ func (h *Holder) deliveryOf(repository string, number int, commit string) *check
c.Check = &v
}
if g := h.groups[found.Group]; g != nil {
state, why, _ := h.groupState(g, h.membersOf(g))
state, why := GroupState(g, h.membersOf(g))
c.groupSaid = state + ": " + why
if g.Check != nil && g.Check.Verdict != "" {
c.groupSaid += "; composed check " + g.Check.Verdict + " — " + g.Check.Summary
@@ -88,9 +88,6 @@ func statusOf(d *Delivery) (string, string) {
case Proposed:
return "pending", "checking"
case Ready:
if d.holdsForReview() {
return "pending", clip("ready, "+d.Ready.Waits, 140) // novox/hq ADR 0496: no Approve is asked before a review
}
return "success", "ready: it is merged on the Approve"
case Rejected:
return "failure", clip("rejected: "+why, 140)
@@ -118,11 +118,13 @@ func (*askedAgainError) Error() string {
return "the forge refused gitea_merge_pull_request: novox/app#7 is not merged: its verdict was judged against trunk aaaaaaaaaaaa, and main is now bbbbbbbbbbbb. The check of head cccccccccccc merged into trunk bbbbbbbbbbbb was asked; took 3s. Ask again at cccccccccccc once both statuses pass. Nothing was merged"
}
// Issue 1022 (ADR 0421 §5), as ADR 0496 changed it (issue 1803): the review record is read; a head not reviewed is not asked.
func TestAHeadNotReviewedAtThisHeadIsNotAsked(t *testing.T) {
// The gate reads nothing from a pull request's description (novox/hq ADR 0493 §6, ADR 0659, which ends ADR 0496): a pull request whose description
// holds no review line, or one naming another head, is asked once its other waits are met, and the ask names no review.
func TestAPullRequestIsAskedWhateverItsDescriptionSays(t *testing.T) {
for _, c := range []struct{ name, body string }{
{"no line", "Fixes issue 7."},
{"another head", "Reviewed-by: lead@shanks at " + pushed},
{"empty", ""},
} {
t.Run(c.name, func(t *testing.T) {
w := newWorld(t)
@@ -131,39 +133,17 @@ func TestAHeadNotReviewedAtThisHeadIsNotAsked(t *testing.T) {
p.Body = c.body
w.h.PullUpdated(p)
hear(t, w, shimCheckedKey, judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
if w.op.count() != 0 {
t.Fatalf("an unreviewed pull request was asked: %d", w.op.count())
open := w.op.open()
if len(open) != 1 {
t.Fatalf("an unreviewed pull request with its other waits met was not asked: %d", len(open))
}
if e := w.op.ask(open[0]).Explanation; strings.Contains(strings.ToLower(e), "review") {
t.Fatalf("the ask names a review:\n%s", e)
}
})
}
}
func TestTheAskNamesTheReviewerOfTheHead(t *testing.T) {
w := newWorld(t)
w.op.auto = false
p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
p.Body = "Fixes issue 7.\n\nReviewed-by: lead@shanks at " + readyHead + "\n"
w.h.PullUpdated(p)
hear(t, w, shimCheckedKey, judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
a := w.op.ask(w.op.open()[0])
if strings.Contains(a.Explanation, "not reviewed") || !strings.Contains(a.Explanation, "reviewed by lead") {
t.Fatalf("a reviewed head is not said reviewed:\n%s", a.Explanation)
}
}
// The description corrected without a push is read again.
func TestTheReviewLineCorrectedInTheDescriptionIsReadWithoutAPush(t *testing.T) {
w := newWorld(t)
w.op.auto = false
p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
w.h.PullUpdated(p)
p.Body = "Reviewed-by: lead@shanks at " + readyHead
w.h.PullUpdated(p)
if d := w.delivery(IDOf("novox/app", readyHead)); d.ReviewedBy != "lead@shanks" || d.ReviewedAt != readyHead {
t.Fatalf("the corrected line is not read: %q %q", d.ReviewedBy, d.ReviewedAt)
}
}
// Issue 1022 (ADR 0421 §6): a push ends the Approve and says so once on the pull request.
func TestAPushAfterTheAskEndsItAndSaysSoOnceOnThePullRequest(t *testing.T) {
w := announced(t)
@@ -188,3 +168,41 @@ func TestAPushAfterTheAskEndsItAndSaysSoOnceOnThePullRequest(t *testing.T) {
t.Fatalf("said more than once: %v", w.forge.comments)
}
}
// ADR 0659 clause 6: no wait of ready.go reads a description field. A delivery and a group whose descriptions differ only
// (no line, a Reviewed-by line naming the head, another head, garbage, an after: and a Pushed-by line) answer the same
// readyWaits, ungatedWaits and group state.
func TestNoWaitReadsADescription(t *testing.T) {
heads := map[string]string{"novox/a": "a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1a1", "novox/b": "b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2b2"}
bodies := []string{"", "Fixes issue 7.", "Reviewed-by: lead@shanks at %s", "Reviewed-by: lead@shanks at " + pushed,
"\x00garbage: Reviewed-by pending\nafter: novox/zzz\nPushed-by: x@y"}
var first []string
for i, body := range bodies {
w := newWorld(t)
w.op.auto = false
for n, repo := range []string{"novox/a", "novox/b"} {
p := pr(repo, n+1, heads[repo], "feat/y", "x")
p.Body = strings.ReplaceAll(body, "%s", heads[repo])
w.h.PullUpdated(p)
w.h.Checked(verdict(repo, n+1, heads[repo], "pass"))
}
w.h.Checked(CheckedEvent{Group: "feat/y", ID: "check-1", Verdict: "pass", Summary: "compose"})
w.settleAll()
g := w.h.groups["feat/y"]
state, why := GroupState(g, w.h.membersOf(g))
ds := []*Delivery{w.delivery(IDOf("novox/a", heads["novox/a"])), w.delivery(IDOf("novox/b", heads["novox/b"]))}
w.h.mu.Lock()
got := []string{state, why}
for _, d := range ds {
got = append(got, w.h.readyWaits(d), w.h.ungatedWaits(d))
}
w.h.mu.Unlock()
if i == 0 {
first = got
continue
}
if strings.Join(got, "|") != strings.Join(first, "|") {
t.Fatalf("description %q changes the waits:\n got %q\nwant %q", body, got, first)
}
}
}
@@ -370,9 +370,7 @@ func (w *world) state(id string) State { return w.delivery(id).State }
func pr(repo string, number int, sha, branch string, paths ...string) PullEvent {
owner, name, _ := strings.Cut(repo, "/")
return PullEvent{Owner: owner, Repo: name, Number: number, Base: "main", Head: branch, HeadSHA: sha,
HTMLURL: fmt.Sprintf("https://forge.invalid/%s/pulls/%d", repo, number), Paths: paths,
// Reviewed at its head, as the Approve is asked only for a reviewed head (novox/hq ADR 0496, issue 1803).
Body: "Reviewed-by: tester@shanks at " + sha}
HTMLURL: fmt.Sprintf("https://forge.invalid/%s/pulls/%d", repo, number), Paths: paths}
}
// verdict is the controller's `checked` for a head.
@@ -351,11 +351,6 @@ func (h *Holder) pullUpdated(p PullEvent) (isNew bool) {
h.fillFromAnnouncement(d, p)
// A pull request retargeted to another base (review of issue 1422): what it merges into is the forge's word now.
h.retarget(d, p)
// The review record, corrected in the description without a push, is read again (ADR 0421 §5).
if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) {
d.ReviewedBy, d.ReviewedAt = by, at
h.keep(d)
}
// So is the line naming who pushed it (ADR 0505 §5).
if by := PushedIn(p.Body); p.Body != "" && by != d.PushedBy {
d.PushedBy = by
@@ -426,7 +421,7 @@ func (h *Holder) supersedeOlder(p PullEvent) (replaced string) {
}
// fillFromAnnouncement fills a delivery made from its check's verdict before its pull request's head was heard (issue
// 1726: no base, branch, title, URLs or review record) from an announcement or an edit of that pull request, and then
// 1726: no base, branch, title or URLs) from an announcement or an edit of that pull request, and then
// does what a first announcement does: the older heads of the pull request are superseded, and the delivery takes
// their place in a group or joins the group its branch makes. Nothing for a delivery that holds its base. Under the lock.
func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) {
@@ -437,7 +432,6 @@ func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) {
d.Paths, d.PathsTruncated, d.Removed = p.Paths, p.PathsTruncated, p.Removed
d.ModuleDirs, d.ModuleDirsSaid = p.ModuleDirs, p.ModuleDirsSaid
d.Issues, d.After = IssuesIn(p.Title+"\n"+p.Body), AfterIn(p.Body)
d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body)
d.PushedBy = PushedIn(p.Body)
if p.BaseSHA != "" {
d.BaseSHA, d.BaseSHAAt = p.BaseSHA, h.Now()
@@ -473,11 +467,9 @@ func (h *Holder) retarget(d *Delivery, p PullEvent) {
h.keep(d)
}
// PullEdited takes a pull request's edit (novox/hq issue 1079): its description is read again for the review record, the
// `after:` lines and the issues it names, exactly as an announcement reads them. An Approve ask still unanswered whose words
// the new record changes is withdrawn and asked again with it read, without a word on the pull request: nothing was
// approved, so no Approve ended. An answered ask is left as it is. A record naming the head lifts the hold at once (ADR 0496 §4). An edit with no
// description, or of a pull request with no open delivery at that head, changes nothing.
// PullEdited takes a pull request's edit (novox/hq issue 1079): its `after:` lines, the `Pushed-by` line and the issues it names
// are read again, exactly as an announcement reads them; nothing here holds or withdraws an ask. An edit with no description,
// or of a pull request with no open delivery at that head, changes nothing.
func (h *Holder) PullEdited(p PullEvent) {
if p.HeadSHA == "" || p.Number == 0 {
return
@@ -511,8 +503,6 @@ func (h *Holder) PullEdited(p PullEvent) {
if d == nil || d.Number != p.Number || d.State.Final() || d.MergedAs != "" {
return
}
before, _ := reviewWords(d)
d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body)
d.PushedBy = PushedIn(p.Body)
d.After = AfterIn(p.Body)
title := p.Title
@@ -521,12 +511,6 @@ func (h *Holder) PullEdited(p PullEvent) {
}
d.Issues = IssuesIn(title + "\n" + p.Body)
h.keep(d)
if after, _ := reviewWords(d); after == before {
return
}
if r := h.covering(d); r != nil && r.AskID != "" && r.Approved == nil && !r.declined() {
h.cancels = append(h.cancels, h.withdrawAsk(r.AskID, d.ID+"'s review record changed: "+shortOf(d.ReviewedAt, 12)))
}
}
// withOther is others with p put in, replacing an earlier announcement of the same pull request.
@@ -583,7 +567,6 @@ func (h *Holder) freshAnnouncement(p PullEvent, now time.Time, startedOver strin
if p.BaseSHA != "" {
d.BaseSHAAt = now
}
d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body)
d.PushedBy = PushedIn(p.Body)
why := fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number)
if startedOver != "" {
@@ -896,7 +879,7 @@ func (h *Holder) PullClosed(p PullEvent) {
h.leaveGroup(d)
}
// Another open pull request carries this head (issue 1114): the record starts over for it at once, as a first
// announcement of it would, so it gets its check, its review record and its Approve ask.
// announcement of it would, so it gets its check and its Approve ask.
if len(d.Others) > 0 {
next := d.Others[0]
over = &next
@@ -1057,24 +1040,17 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func(
// (issue 1318).
h.composeFromChecker(g, members, now)
// Its state, said when it changes.
state, why, held := h.groupState(g, members)
state, why := GroupState(g, members)
status := groupStatusState(state)
if held {
status = "pending" // no Approve exists while a member waits for its review (novox/hq ADR 0496 §2)
}
if state == GroupMerging && !(g.Check != nil && (g.Check.Verdict == "pass" || g.Check.Verdict == "warning") &&
sameMembers(g.Check.Commits, commitsOf(members)) && g.Check.OutOfDate == "") {
// Success there lets the merge tool merge its members: only for a composed check that passed for these heads.
status = "pending"
}
reviewSaid := ""
if held {
reviewSaid = why
}
saidMembers := strings.Join(sortedIDs(members), " ")
if state != g.Said || status != g.SaidStatus || reviewSaid != g.SaidReview || why != g.SaidWhy ||
if state != g.Said || status != g.SaidStatus || why != g.SaidWhy ||
saidMembers != g.SaidMembers {
g.Said, g.SaidStatus, g.SaidReview, g.SaidWhy, g.SaidMembers = state, status, reviewSaid, why, saidMembers
g.Said, g.SaidStatus, g.SaidWhy, g.SaidMembers = state, status, why, saidMembers
for _, d := range members {
g.Owed = append(g.Owed, Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery-group",
State: status, Line: clip(fmt.Sprintf("group %s %s: %s", g.ID, state, why), 140),
@@ -1160,29 +1136,6 @@ func heldWords(members []*Delivery) string {
return strings.Join(parts, "; ")
}
// groupState is GroupState, but a group whose members all passed and compose waits for a review while any member the
// Approve would cover has none at its head: its why names the members (novox/hq ADR 0496 §2, issue 1803), and held
// says its mesh/delivery-group is pending, not a success. Under the lock.
func (h *Holder) groupState(g *Group, members []*Delivery) (state, why string, held bool) {
state, why = GroupState(g, members)
if state != GroupReady || g.Closed {
return state, why, false
}
var waiting []string
for _, m := range members {
if m.MergedAs != "" || m.State.Final() || h.covering(m) != nil {
continue
}
if _, ok := reviewWords(m); !ok {
waiting = append(waiting, "at "+shortOf(m.Commit, 12)+" ("+pullSaid(m)+")")
}
}
if len(waiting) == 0 {
return state, why, false
}
return state, waitsForReview + strings.Join(waiting, " and ")[len("at "):], true
}
// groupStatusState is the forge status mesh/delivery-group takes for a group's state. A delivering group passed its composed check for these very heads (GroupState reads checking while it is out of
// date), so it is a success too: its members after the first merge only once mesh/delivery-group is one at their
// heads (novox/hq ADR 0382), and a pending one there would hold them for ever (novox/hq issue 902).
@@ -21,11 +21,8 @@ const (
func TestAPullRequestsHeadIsProposedAndReady(t *testing.T) {
w := newWorld(t)
p := pr("novox/app", 7, head, "feat/x", "src/a.go")
p.Body = "" // an empty description is never read again, so the review set below stays
w.h.PullUpdated(p)
id := IDOf("novox/app", head)
// This head is not a full commit id, so its description line cannot name it: its review is set as read.
w.h.deliveries[id].ReviewedBy, w.h.deliveries[id].ReviewedAt = "tester@shanks", head
if w.state(id) != Proposed {
t.Fatalf("a head is %s", w.state(id))
}
@@ -215,6 +212,7 @@ func TestAMergeWithoutAPassingCheckIsHeld(t *testing.T) {
// A group merged whole ends merged, member by member and as a group, and source.main holds its heads (ADR 0539 §17, R6-C2).
func TestAMergedGroupEndsMergedWithSourceMainMoved(t *testing.T) {
w := newWorld(t)
w.op.auto = false // the merges here are the forge's, not the gate's on an Approve
heads := map[string]string{"novox/a": "a1a1a1a1a1a1a1", "novox/b": "b2b2b2b2b2b2b2"}
ids := map[string]string{}
for i, repo := range []string{"novox/a", "novox/b"} {
@@ -1,97 +0,0 @@
package main
import (
"strings"
"testing"
)
// novox/hq issue 1079: mesh-catalog #354's description was edited ten seconds after it was opened to add its
// `Reviewed-by` line, but the line was read only when the head was announced, so the Approve's ask said "not reviewed at
// this head". The forge's edit event is followed; an Approve ask not yet answered is withdrawn and asked again with the
// record read (the operator agreed, 2026-10-18). The review record is never a gate (ADR 0421 §5).
// askedReviewed is a green pull request whose Approve is asked, its review line naming the head by one reviewer.
func askedReviewed(t *testing.T) *world {
w := newWorld(t)
w.op.auto = false
p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
p.Title = "Fix the apostrophe (issue 948)"
p.Body = "Reviewed-by: first@shanks at " + readyHead
w.h.PullUpdated(p)
w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)}
w.h.SeeOpenPulls()
w.settleAll()
if open := w.op.open(); len(open) != 1 || !strings.Contains(w.op.ask(open[0]).Explanation, "reviewed by first on shanks") {
t.Fatalf("setup: the ask is %v", w.op.open())
}
return w
}
func edited(body string) PullEvent {
p := pr("novox/app", 7, readyHead, "feat/x")
p.Body = body
return p
}
func TestAReviewedByLineChangedAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(t *testing.T) {
w := askedReviewed(t)
first := w.op.open()[0]
w.h.PullEdited(edited("Reviewed-by: second@shanks at " + readyHead))
w.settleAll()
open := w.op.open()
// The heads are the same, so the key is: the ask is taken away and written again with the record in its words, which the
// teller reads as the ask to withdraw and ask afresh.
if len(open) != 1 || open[0] != first || w.op.count() != 2 {
t.Fatalf("after the edit the open asks are %v (first %s), %d asked; wanted the first withdrawn and written again once", open, first, w.op.count())
}
if len(w.op.cancelled) != 0 || w.op.byKey[first].Asking != 2 {
t.Fatalf("the first ask was taken away (%v) or not written again as asking 2 (%d)", w.op.cancelled, w.op.byKey[first].Asking)
}
if e := w.op.ask(open[0]).Explanation; !strings.Contains(e, "reviewed by second on shanks at "+shortOf(readyHead, 12)) {
t.Fatalf("the new ask does not say the record: %s", e)
}
if len(w.forge.comments) != 0 {
t.Fatalf("a withdrawn ask is not an ended Approve, so nothing is said on the pull request: %v", w.forge.comments)
}
// The new ask is the one the Approve answers.
w.op.answer(open[0], OptionApprove)
w.settleAll()
if len(w.forge.merges) != 1 {
t.Fatalf("the new ask's Approve did not merge: %v", w.forge.merges)
}
}
func TestAnEditThatChangesNothingTheAskSaysAsksNothingAndNoEditChangesAnAnsweredAsk(t *testing.T) {
w := askedReviewed(t)
// The same words: a description with the same line and a new sentence.
w.h.PullEdited(edited("A new sentence.\nReviewed-by: first@shanks at " + readyHead))
w.settleAll()
if w.op.count() != 1 || len(w.op.cancelled) != 0 {
t.Fatalf("an edit that changes no word of the ask asked again: %d asks, cancelled %v", w.op.count(), w.op.cancelled)
}
// Answered: a later edit changes nothing.
w.op.answer(w.op.open()[0], OptionApprove)
w.settleAll()
asked := w.op.count()
w.h.PullEdited(edited("Reviewed-by: second@shanks at " + readyHead))
w.settleAll()
if w.op.count() != asked || len(w.op.open()) != 0 {
t.Fatalf("an edit after the Approve asked again: %d asks, open %v", w.op.count(), w.op.open())
}
if d := w.delivery(IDOf("novox/app", readyHead)); d.ReviewedBy != "second@shanks" {
t.Fatalf("the record is still read: %q", d.ReviewedBy)
}
}
func TestAnEditOfAnUnknownOrMergedOrEmptyPullRequestChangesNothing(t *testing.T) {
w := askedReviewed(t)
other := pr("novox/other", 3, readyHead, "feat/y")
other.Body = "Reviewed-by: a@b at " + readyHead
w.h.PullEdited(other) // no delivery: nothing made
w.h.PullEdited(edited(""))
w.settleAll()
if w.op.count() != 1 || len(w.h.deliveries) != 1 {
t.Fatalf("asks %d, deliveries %d", w.op.count(), len(w.h.deliveries))
}
}
@@ -1,7 +1,6 @@
package main
import (
"strings"
"testing"
)
@@ -19,7 +18,7 @@ func TestClosingThePullRequestOfASharedHeadStartsTheOtherOnesDeliveryOver(t *tes
id := IDOf(gCtl, head1114)
w.h.PullUpdated(pr(gCtl, 378, head1114, "feat/chain", "internal/a.go"))
b := pr(gCtl, 379, head1114, "feat/chain-2", "internal/a.go")
b.Body = "Reviewed-by: reviewer@shanks at " + head1114
b.Body = "Fixes issue 379."
w.h.PullUpdated(b) // announced while #378 is live: the same delivery heard again
if d := w.delivery(id); d.Number != 378 {
t.Fatalf("setup: the record is #%d, wanted #378 still", d.Number)
@@ -29,14 +28,10 @@ func TestClosingThePullRequestOfASharedHeadStartsTheOtherOnesDeliveryOver(t *tes
if d.Number != 379 || d.State.Final() || d.ClosedUnmerged {
t.Fatalf("after #378 closed the record is #%d %s (closed %v); wanted #379, proposed", d.Number, d.State, d.ClosedUnmerged)
}
if d.ReviewedBy == "" {
t.Fatalf("#379's review record was not read: %+v", d)
}
w.h.Checked(verdict(gCtl, 379, head1114, "pass"))
w.settleAll()
if open := w.op.open(); w.state(id) != Ready || len(open) != 1 ||
!strings.Contains(w.op.ask(open[0]).Explanation, "reviewed by reviewer") {
t.Fatalf("#379's delivery is %s with asks %v; wanted ready and one Approve ask with its review record", w.state(id), w.op.open())
if open := w.op.open(); w.state(id) != Ready || len(open) != 1 {
t.Fatalf("#379's delivery is %s with asks %v; wanted ready and one Approve ask", w.state(id), w.op.open())
}
}
@@ -61,12 +56,12 @@ func TestAnEditOfAPullRequestWhoseSharedHeadsRecordIsStoppedUnderAnotherTakesItO
t.Fatalf("setup: %s", d.State)
}
e := pr(gCtl, 379, head1114, "feat/chain-2")
e.Body = "Reviewed-by: reviewer@shanks at " + head1114
e.Body = "Fixes issue 379."
w.h.PullEdited(e)
d := w.delivery(id)
if d.Number != 379 || d.State != Proposed || d.ReviewedBy == "" || len(d.Paths) == 0 {
t.Fatalf("after #379's edit the record is #%d %s reviewed %q paths %v; wanted #379 proposed, read, with the paths", d.Number,
d.State, d.ReviewedBy, d.Paths)
if d.Number != 379 || d.State != Proposed || len(d.Paths) == 0 {
t.Fatalf("after #379's edit the record is #%d %s paths %v; wanted #379 proposed, read, with the paths", d.Number,
d.State, d.Paths)
}
w.h.PullEdited(e) // an edit heard again changes nothing
if again := w.delivery(id); again.Number != 379 || again.State != Proposed {
@@ -52,8 +52,7 @@ func TestAnAnnouncementFillsADeliveryMadeFromItsVerdictAndTheOperatorIsAsked(t *
w.h.PullUpdated(p)
w.settleAll()
d := w.delivery(id)
if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.HTMLURL == "" || d.CloneURL == "" ||
d.ReviewedBy != "reviewer@shanks" || len(d.Paths) != 1 {
if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.HTMLURL == "" || d.CloneURL == "" || len(d.Paths) != 1 {
t.Fatalf("the announcement did not fill the delivery: %+v", d)
}
if n := len(w.op.open()); n != 1 {
@@ -117,7 +116,7 @@ func TestAnEditOfThePullRequestFillsADeliveryMadeFromItsVerdict(t *testing.T) {
w.h.PullEdited(p)
w.settleAll()
d := w.delivery(id)
if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" || d.ReviewedBy != "reviewer@shanks" {
if d.Base != "main" || d.Branch != "fix/x" || d.Title != "Fix x" {
t.Fatalf("the edit did not fill the delivery: %+v", d)
}
if n := len(w.op.open()); n != 1 {
@@ -1,223 +0,0 @@
package main
import (
"strings"
"testing"
)
// novox/hq issue 1803, ADR 0496: the Approve is asked only for a head with a review recorded at that head. A missing
// line, `Reviewed-by: pending` or a line naming another commit holds the ask; the delivery stays ready and says it waits
// for a review at the head. An edit naming the head asks at once. A group asks when every member is reviewed. An ask
// already raised is not touched.
const stale1803 = "9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f9f"
// greenWithBody is a green pull request whose description is body, the operator not answering by himself.
func greenWithBody(t *testing.T, body string) *world {
w := newWorld(t)
w.op.auto = false
p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go")
p.Title = "Fix the apostrophe (issue 948)"
p.Body = body
w.h.PullUpdated(p)
w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1"))
w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)}
w.h.SeeOpenPulls()
w.settleAll()
return w
}
func (w *world) wantHeld(t *testing.T, label string) {
t.Helper()
want := "waits for a review at " + shortOf(readyHead, 12)
d := w.delivery(IDOf("novox/app", readyHead))
if w.op.count() != 0 || len(w.op.open()) != 0 {
t.Fatalf("%s: the operator was asked (%d asks)", label, w.op.count())
}
if d.State != Ready || d.Ready == nil || !strings.Contains(d.Ready.Waits, want) {
t.Fatalf("%s: state %s, waits %+v; wanted ready and %q", label, d.State, d.Ready, want)
}
found := false
for k, v := range w.forge.statuses {
if strings.HasSuffix(strings.SplitN(k, " ", 2)[1], "mesh/delivery") && strings.Contains(v, want) {
found = true
}
}
if !found {
t.Fatalf("%s: no mesh/delivery status says %q: %v", label, want, w.forge.statuses)
}
}
func TestA1803PendingReviewLineHoldsTheAsk(t *testing.T) {
w := greenWithBody(t, "Reviewed-by: pending")
w.wantHeld(t, "1803/AC1 pending")
for range 3 {
w.later(2 * 60 * 1e9)
w.settleAll()
}
w.wantHeld(t, "1803/AC1 pending after ticks")
}
func TestA1803MissingReviewLineHoldsTheAsk(t *testing.T) {
w := greenWithBody(t, "No review line at all.")
w.wantHeld(t, "1803/AC1 missing")
}
func TestA1803ReviewLineOfAnotherCommitHoldsTheAsk(t *testing.T) {
w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+stale1803)
w.wantHeld(t, "1803/AC2 stale")
}
func TestA1803ReviewLineNamingTheHeadIsAsked(t *testing.T) {
w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+readyHead)
if len(w.op.open()) != 1 {
t.Fatalf("1803/AC3: a review at the head is not asked: %v", w.op.open())
}
if e := w.op.ask(w.op.open()[0]).Explanation; !strings.Contains(e, "reviewed by reviewer on shanks") {
t.Fatalf("the ask does not name the review: %s", e)
}
}
func TestA1803AnEditNamingTheHeadRaisesTheAskAtOnce(t *testing.T) {
w := greenWithBody(t, "Reviewed-by: pending")
w.wantHeld(t, "1803/AC4 before the edit")
e := pr("novox/app", 7, readyHead, "feat/x")
e.Body = "Reviewed-by: reviewer@shanks at " + readyHead
w.h.PullEdited(e)
w.settleAll()
if len(w.op.open()) != 1 {
t.Fatalf("1803/AC4: the edit naming the head raised no ask: %d asks", w.op.count())
}
// An unanswered ask whose review is taken back is withdrawn: the hold applies again (ADR 0496 §1).
e.Body = "Reviewed-by: pending"
w.h.PullEdited(e)
w.settleAll()
if len(w.op.open()) != 0 {
t.Fatalf("1803/AC4: an unanswered ask stayed after its review was taken back: open %v", w.op.open())
}
}
func TestA1803AnApproveAlreadyGivenIsNotAffected(t *testing.T) {
w := greenWithBody(t, "Reviewed-by: reviewer@shanks at "+readyHead)
w.op.answer(w.op.open()[0], OptionApprove)
w.settleAll()
if len(w.forge.merges) != 1 {
t.Fatalf("1803/AC6: the approved head was not merged: %v", w.forge.merges)
}
// A later edit to pending changes nothing of the Approve given.
e := pr("novox/app", 7, readyHead, "feat/x")
e.Body = "Reviewed-by: pending"
w.h.PullEdited(e)
w.settleAll()
if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready == nil || d.Ready.Approved == nil {
t.Fatalf("1803/AC6: the Approve given was lost: %+v", d.Ready)
}
}
const gTools, gHt = "novox/mesh-tools", "e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3"
// groupOf is a green group of the given members (repository, number, head, review body), composed green, the operator not
// answering by himself.
func groupOf(t *testing.T, members [][4]string) *world {
w := newWorld(t)
w.op.auto = false
for i, m := range members {
p := pr(m[0], i+1, m[2], "feat/g", "src/a.go")
p.Body = m[3]
w.h.PullUpdated(p)
}
for i, m := range members {
w.h.Checked(judged(m[0], i+1, m[2], "pass", gTrunk0))
}
w.settleAll()
g := w.h.groups["feat/g"]
if g == nil || g.Check == nil {
t.Fatalf("no composed check was asked: %+v", g)
}
c := CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed"}
for _, m := range members {
owner, repo, _ := strings.Cut(m[0], "/")
c.Members = append(c.Members, struct {
Owner string `json:"owner"`
Repo string `json:"repo"`
Commit string `json:"commit"`
Trunk string `json:"trunk,omitempty"`
}{Owner: owner, Repo: repo, Commit: m[2], Trunk: gTrunk0[:12]})
}
w.h.Checked(c)
w.settleAll()
return w
}
func reviewedAt(head string) string { return "Reviewed-by: reviewer@shanks at " + head }
// wantGroupSays checks the group's state, its mesh/delivery-group status and the member's mesh/delivery status.
func (w *world) wantGroupSays(t *testing.T, label, member, head string, want, notWant string) {
t.Helper()
g := w.h.groups["feat/g"]
state, why, held := w.h.groupState(g, w.h.membersOf(g))
if state != "ready" || !held || !strings.Contains(why, want) || (notWant != "" && strings.Contains(why, notWant)) {
t.Fatalf("%s: group state %s, why %q, held %v; wanted ready waiting with %q and without %q", label, state, why, held, want, notWant)
}
for _, c := range []string{"mesh/delivery-group", "mesh/delivery"} {
said := w.forge.statuses[member+"@"+head+" "+c]
if !strings.HasPrefix(said, "pending") || !strings.Contains(said, want) || (notWant != "" && strings.Contains(said, notWant)) {
t.Fatalf("%s: %s on %s says %q; wanted pending with %q and without %q", label, c, member, said, want, notWant)
}
}
}
func TestA1803AGroupIsAskedOnlyWhenEveryMemberIsReviewedAtItsHead(t *testing.T) {
w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}})
if w.op.count() != 0 {
t.Fatalf("1803/AC5: a group with an unreviewed member was asked: %d asks", w.op.count())
}
w.wantGroupSays(t, "1803/AC5", gCtl, gHc, "mesh-catalog#2", "mesh-controller#1")
e := pr(gCat, 2, gHk, "feat/g")
e.Body = reviewedAt(gHk)
w.h.PullEdited(e)
w.settleAll()
if len(w.op.open()) != 1 {
t.Fatalf("1803/AC5: every member reviewed, yet %d asks", w.op.count())
}
if said := w.forge.statuses[gCtl+"@"+gHc+" mesh/delivery-group"]; !strings.HasPrefix(said, "success") {
t.Fatalf("1803/AC5: the group's status once asked is %q", said)
}
}
func TestA1803TheWaitingWordsFollowTheMembersStillWaiting(t *testing.T) {
w := groupOf(t, [][4]string{{gCtl, "", gHc, reviewedAt(gHc)}, {gCat, "", gHk, "Reviewed-by: pending"}, {gTools, "", gHt, "Reviewed-by: pending"}})
w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-catalog#2", "")
w.wantGroupSays(t, "1803/AC7 two waiting", gCtl, gHc, "mesh-tools#3", "")
e := pr(gCat, 2, gHk, "feat/g")
e.Body = reviewedAt(gHk)
w.h.PullEdited(e)
w.settleAll()
if w.op.count() != 0 {
t.Fatalf("1803/AC7: a member still waits, yet %d asks", w.op.count())
}
// Only the other member is named now, on the group and on every member's own status.
w.wantGroupSays(t, "1803/AC7 one waiting", gCtl, gHc, "mesh-tools#3", "mesh-catalog#2")
w.wantGroupSays(t, "1803/AC7 one waiting", gCat, gHk, "mesh-tools#3", "mesh-catalog#2")
}
func TestA1803APushAfterAnApprovingReviewHoldsTheNewHead(t *testing.T) {
w := greenWithBody(t, reviewedAt(readyHead))
if len(w.op.open()) != 1 {
t.Fatalf("setup: the reviewed head is not asked")
}
// A new head on the same pull request whose description still names the old commit.
const pushed2 = "7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a7a"
p := pr("novox/app", 7, pushed2, "feat/x", "src/a.go")
p.Body = reviewedAt(readyHead)
w.h.PullUpdated(p)
w.h.Checked(judgedAt("novox/app", 7, pushed2, "pass", "t1t1t1t1t1t1"))
w.settleAll()
d := w.delivery(IDOf("novox/app", pushed2))
if d.Ready == nil || !strings.Contains(d.Ready.Waits, waitsForReview+shortOf(pushed2, 12)) {
t.Fatalf("1803/AC8: the new head waits %+v; wanted it to wait for a review at %s", d.Ready, shortOf(pushed2, 12))
}
if len(w.op.open()) != 0 {
t.Fatalf("1803/AC8: the new head was asked on the old review: %v", w.op.open())
}
}
@@ -208,7 +208,7 @@ func take(h *Holder, e stdio.Envelope) error {
h.PullClosed(p)
}
case "gitea.forge.pull-request.edited":
// The description edited after a head was announced (issue 1079): the review record in it is read again.
// The description edited after a head was announced (issue 1079): the lines in it are read again.
var p PullEvent
if err := json.Unmarshal(e.Body, &p); err != nil {
h.Refuse(fmt.Errorf("%s is not readable: %v", e.Key, err))
@@ -96,11 +96,6 @@ type Delivery struct {
// Ready is what this owner did with the operator's Approve of its merge into main, and with the merge (novox/hq ADR
// 0421, ready.go). The json key is the one the ready mark's record had, so rows written before still read.
Ready *ReadyRun `json:"ready,omitempty"`
// ReviewedBy and ReviewedAt are the review record its pull request's description holds: the line
// `Reviewed-by: <who> at <full sha>` (novox/hq ADR 0421 §5, issue 1022), as written. Read at every head; never a
// hold: no Approve is asked until it names the head (novox/hq ADR 0496, issue 1803).
ReviewedBy string `json:"reviewed_by,omitempty"`
ReviewedAt string `json:"reviewed_at,omitempty"`
// PushedBy is the session that pushed its head, as its pull request's description names it in a line
// `Pushed-by: <who@node>` (playbook 07): named with its branch when a new head is needed (novox/hq ADR 0505 §5).
PushedBy string `json:"pushed_by,omitempty"`
@@ -276,19 +271,6 @@ func sameCommit(a, b string) bool {
return len(a) >= 7 && strings.HasPrefix(strings.ToLower(b), strings.ToLower(a))
}
// reviewedLine is the review record of a pull request's description (novox/hq ADR 0421 §5, issue 1022):
// `Reviewed-by: <who> at <full sha>`, on a line of its own, who as who@node.
var reviewedLine = regexp.MustCompile(`(?im)^\s*Reviewed-by:\s*(\S+)\s+at\s+([0-9a-f]{40}|[0-9a-f]{64})\b`)
// ReviewIn reads the review record of a pull request's description: who reviewed and the full sha named. The last such
// line counts; empty when there is none.
func ReviewIn(body string) (who, sha string) {
for _, m := range reviewedLine.FindAllStringSubmatch(body, -1) {
who, sha = m[1], strings.ToLower(m[2])
}
return who, sha
}
// pushedLine is the line naming the session that pushed a pull request's head: `Pushed-by: <who@node>` (ADR 0505 §5).
var pushedLine = regexp.MustCompile(`(?im)^\s*Pushed-by:\s*([A-Za-z0-9_.\-]+(?:@[A-Za-z0-9_.\-]+)?)\s*$`)
@@ -314,20 +296,6 @@ func authorOf(d *Delivery) string {
return who
}
// reviewWords is what the delivery's review record says of its head, for the ask: reviewed by whom, or, first, that it
// is not reviewed at this head (the ask is held then, ADR 0496). ok is whether it is reviewed at this head.
func reviewWords(d *Delivery) (words string, ok bool) {
switch {
case d.ReviewedBy == "":
return "not reviewed at this head: the description holds no Reviewed-by line", false
case !strings.EqualFold(d.ReviewedAt, d.Commit):
return fmt.Sprintf("not reviewed at this head: the Reviewed-by line names %s, the head is %s",
shortOf(d.ReviewedAt, 12), shortOf(d.Commit, 12)), false
}
// The router refuses an @ in an ask's words: who@node is said "who on node".
return "reviewed by " + strings.Replace(d.ReviewedBy, "@", " on ", 1) + " at " + shortOf(d.Commit, 12), true
}
// afterLines are the `after: <repository>` lines of a pull request's description.
var afterLines = regexp.MustCompile(`(?im)^\s*after:\s*([A-Za-z0-9_.\-/]+)\s*$`)
@@ -364,9 +332,6 @@ type Group struct {
// SaidStatus is the mesh/delivery-group status last written for it: a change of the status a state takes is
// written too, though the state did not change (novox/hq issue 902).
SaidStatus string `json:"said_status,omitempty"`
// SaidReview is the wait for a review last said of the group while it was held for one (ADR 0496 §2): a change of
// the members still waiting is said too, though the state did not change.
SaidReview string `json:"said_review,omitempty"`
// SaidWhy and SaidMembers are the why and the member ids (each names its head) last said: a member joining, leaving or
// pushing, or a count in the why moving, is said too, though the state did not change (2026-10-30: a group of four
// said "3 of 3 member(s) ready" for ten minutes, and the member that joined had no mesh/delivery-group at all).
@@ -31,7 +31,7 @@ import (
// - a check that failed merges nothing: it is said once on the pull request, and as a stalled line, which the
// gate says to the operator as a condition (conditions.go). So is a merge the tool refused for an hour.
//
// The review record is read and put in the ask; it is never a gate (ADR 0421 §5). What this owner does is kept on the
// No part reads a pull request's description to gate anything (novox/hq ADR 0493 §6, ADR 0659). What this owner does is kept on the
// delivery (Ready), said in its log, and the merge's transitions name the Approve as their warrant.
// readyRefusedFor is how long the merge tool may refuse an approved head before it is said as stalled: none (issue 1115) —
@@ -266,14 +266,7 @@ func (h *Holder) readyAsks(now time.Time) []func() {
r.Failed = ""
waits := h.readyWaits(d)
if waits != r.Waits {
wasHeld := d.holdsForReview()
r.Waits = waits
if (wasHeld || d.holdsForReview()) && d.shownOnItsPullRequest() {
// The status and the view say what it waits for, again whenever the words change while held, and no more once asked.
state, desc := statusOf(d)
d.owe(Effect{Kind: EffectView, Since: now})
d.owe(Effect{Kind: EffectStatus, Commit: d.Commit, Context: "mesh/delivery", State: state, Line: desc, Since: now})
}
if waits != "" {
h.Logf("[mesh-delivery] %s waits: %s", d.ID, waits)
}
@@ -438,13 +431,6 @@ func (h *Holder) readyWaits(d *Delivery) string {
default:
return "its check has not passed yet (" + string(d.State) + ")"
}
// ADR 0496 §1, §2: the Approve is asked only for heads with a review recorded at them. An ask already raised or
// answered is not touched (covering).
if h.covering(d) == nil {
if w := h.reviewWaits(d); w != "" {
return w
}
}
// On mesh/check, the checker's verdict at exactly the heads the ask covers (issue 1302, ADR 0447 §2).
if w := h.checkerWaits(d); w != "" {
return w
@@ -455,18 +441,13 @@ func (h *Holder) readyWaits(d *Delivery) string {
return h.groupTurnToMerge(d)
}
// ungatedWaits is readyWaits while mesh/check gates no merge (check-gates-merge false): a ready delivery waits only for its
// review at its head, and a group member for every member to be ready; no verdict, composed check or trunk is waited on,
// ungatedWaits is readyWaits while mesh/check gates no merge (check-gates-merge false): a ready delivery waits for nothing but
// a group member for every member to be ready; no verdict, composed check or trunk is waited on,
// so the Approve is asked at once.
func (h *Holder) ungatedWaits(d *Delivery) string {
if d.State != Ready {
return "it is " + string(d.State) + ", not ready yet"
}
if h.covering(d) == nil {
if w := h.reviewWaits(d); w != "" {
return w
}
}
if g := h.groups[d.Group]; d.Group != "" && g != nil {
for _, m := range h.membersOf(g) {
if m.State != Ready && m.State != Merged {
@@ -475,7 +456,7 @@ func (h *Holder) ungatedWaits(d *Delivery) string {
}
}
// No verdict is waited on (the operator, 2026-11-07: "we aren't running any checks anymore, we're delivering"): while
// the check gates no merge, the Approve is asked once the delivery is reviewed and ready; a verdict that comes later
// the check gates no merge, the Approve is asked once the delivery is ready; a verdict that comes later
// is shown on the pull request and the delivery, as before.
return ""
}
@@ -496,35 +477,6 @@ func (h *Holder) verdictWaits(d *Delivery) string {
return waitsCheckBeforeAsking + c.Words
}
// 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 "
// reviewWaits is why the Approve is not asked yet for want of a review (novox/hq ADR 0496, issue 1803): a delivery
// whose description holds no review line, `pending` or a line naming another commit, and for a group any member so.
// Empty when every delivery the ask covers is reviewed at its head. Under the lock.
func (h *Holder) reviewWaits(d *Delivery) string {
members := h.askMembers(d)
var waiting []string
for _, m := range members {
if _, ok := reviewWords(m); ok {
continue
}
if len(members) == 1 {
return waitsForReview + shortOf(m.Commit, 12)
}
waiting = append(waiting, "at "+shortOf(m.Commit, 12)+" ("+pullSaid(m)+")")
}
if len(waiting) == 0 {
return ""
}
return waitsForReview + strings.Join(waiting, " and ")[len("at "):]
}
// holdsForReview is whether a delivery's wait is the hold for a review.
func (d *Delivery) holdsForReview() bool {
return d.Ready != nil && strings.HasPrefix(d.Ready.Waits, waitsForReview)
}
// groupTurnToMerge is why a group member may not merge yet; empty when it may: the group's composed check passed for
// its members' heads as they are and is not out of date. No member waits for another's merge or delivery (novox/hq
// ADR 0418 §4): once the composed check passed and the Approve covers the heads, every member is merged in one pass.
@@ -87,14 +87,14 @@ func (h *Holder) Show(id string) (any, error) {
out["machines"] = m
}
if g := h.groups[d.Group]; g != nil {
state, why, _ := h.groupState(g, h.membersOf(g))
state, why := GroupState(g, h.membersOf(g))
out["group"] = map[string]any{"group": g, "state": state, "why": why}
}
return out, nil
}
if g := h.groups[id]; g != nil {
members := h.membersOf(g)
state, why, _ := h.groupState(g, members)
state, why := GroupState(g, members)
return map[string]any{"group": g, "state": state, "why": why, "members": members, "check-gates-merge": h.checkGates()}, nil
}
return nil, fmt.Errorf("no delivery or group %q: `deliveries` lists them", id)
@@ -138,7 +138,7 @@ func (h *Holder) Groups(all bool) []GroupLine {
if len(members) == 0 {
continue
}
state, why, _ := h.groupState(g, members)
state, why := GroupState(g, members)
if !all && (state == GroupMerged || state == GroupClosed) {
continue
}
+1 -1
View File
@@ -102,7 +102,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 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.",
"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 ready pull request (no review line is waited for, ADR 0659) has 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
}