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 cbd99faf1..00f2b66e8 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -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 { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval_test.go index f6763408a..6496ecf3c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval_test.go @@ -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) } diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go index 5f2d8edd1..a9a2847a8 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/bus_test.go @@ -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") diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go index d6489b2ce..ea17f3684 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/checks.go @@ -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 diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go index aee203cda..dcd1762fb 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/effects.go @@ -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) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go index 7941cd821..45a65698c 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/events_test.go @@ -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) + } + } +} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go index f1d36f37b..9b1e1f2a1 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/fakes_test.go @@ -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. diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index 6a60b0850..36c28b901 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -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). diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go index 3de163f4d..e62fe9aa4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder_test.go @@ -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"} { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go deleted file mode 100644 index 919bf4b45..000000000 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1079_test.go +++ /dev/null @@ -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)) - } -} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1114_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1114_test.go index 6da2cec4c..427a8f01b 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1114_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1114_test.go @@ -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 { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go index ef8efd07e..120653b6d 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1726_test.go @@ -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 { diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go deleted file mode 100644 index 7b9660521..000000000 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issue1803_test.go +++ /dev/null @@ -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()) - } -} diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go index e7f684b9c..6d86d423a 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go @@ -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)) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go index c55432500..5ad6a565f 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -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: at ` (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: ` (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: at `, 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: ` (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: ` 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). 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 d02dd1f4f..442593248 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -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. diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go index 076895920..7abc79052 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/verbs.go @@ -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 } diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index 736144095..0efb57769 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -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 }