From bf6bb3d09491c1b790fba74699c8f0c2cd293bd6 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 11 Nov 2026 14:44:17 +0100 Subject: [PATCH] The gate names the issues linked in the issue tracker and the forge's author, and reads no description (milestone 47/G, W11.5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0659 §4 kept three description reads that shape words only until they move to the bus: the issue numbers the ask names, the author a stalled line names and the notice that after: lines are not read. The ask now names the issues linked to each member's pull request, read once per head through the seat issue-tracker's list (a tracker that does not answer is said, never waited on); the author is the forge's pull request record's, from the seat git's events; the after: notice is gone. The gate's forms of a pull request and a delivery hold no description field. Claude-Session: https://claude.ai/code/session_01JG73KEq4zoDfM9WtYpcQgZ (Joined from ddd93a78.) Builds: design 47 G §50, §143 (GATE15) Claude-Session: https://claude.ai/code/session_01JG73KEq4zoDfM9WtYpcQgZ --- modules/mesh-delivery-gate/README.md | 5 +- .../cmd/mesh-delivery-gate/adr505_test.go | 20 ++-- .../cmd/mesh-delivery-gate/approval.go | 25 ++--- .../cmd/mesh-delivery-gate/bus_test.go | 2 - .../cmd/mesh-delivery-gate/events_test.go | 83 ++++---------- .../cmd/mesh-delivery-gate/holder.go | 45 +++----- .../cmd/mesh-delivery-gate/holder_test.go | 71 ++++++++---- .../cmd/mesh-delivery-gate/issue1114_test.go | 3 - .../cmd/mesh-delivery-gate/issue1726_test.go | 6 +- .../cmd/mesh-delivery-gate/issuelinks.go | 106 ++++++++++++++++++ .../cmd/mesh-delivery-gate/main.go | 1 + .../cmd/mesh-delivery-gate/manifest_test.go | 3 +- .../cmd/mesh-delivery-gate/model.go | 55 +++------ .../mesh-delivery-gate/onestatus_bus_test.go | 1 - .../cmd/mesh-delivery-gate/ready.go | 23 ---- modules/mesh-delivery-gate/module.json | 3 +- 16 files changed, 234 insertions(+), 218 deletions(-) create mode 100644 modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issuelinks.go diff --git a/modules/mesh-delivery-gate/README.md b/modules/mesh-delivery-gate/README.md index a7ed14ada..726019984 100644 --- a/modules/mesh-delivery-gate/README.md +++ b/modules/mesh-delivery-gate/README.md @@ -33,8 +33,9 @@ names every refusal and rejection at once, and the pull request is told at once; `refusal-stall-floor-minutes`, 10). Every other refusal is stalled at once. - a delivery that needs its author's new head (its check failed after its Approve: a conflict, a failed check, a rejected group) is stalled only once no new head came for the setting `author-new-head-minutes` (120). Its line says - `needs: new-head` and names its author: the branch, and the session a `Pushed-by: ` line in the pull - request's description names. + `needs: new-head` and names its author: the branch, and who opened the pull request as the forge's record says + (the seat git's events). The ask names the issues linked to each pull request in the issue tracker (the seat + issue-tracker's `list`); nothing is read from a description. ## A merge the forge account cannot carry out is not asked (issue 1945) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go index 3848448a7..b3c0355a0 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/adr505_test.go @@ -131,15 +131,13 @@ func TestAnyOtherRefusalIsStalledAtOnce(t *testing.T) { } // rejectedBeforeItsAnswer is a delivery approved and then rejected: its check failed after the ask (as in events_test.go), -// its pull request's description naming who pushed it when pushedBy is given. -func rejectedBeforeItsAnswer(t *testing.T, pushedBy string) (*world, string) { +// its pull request opened by author as the forge's record says, when given. +func rejectedBeforeItsAnswer(t *testing.T, author string) (*world, string) { t.Helper() w := newWorld(t) w.op.auto = false p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") - if pushedBy != "" { - p.Body += "\nPushed-by: " + pushedBy - } + p.Author = author w.h.PullUpdated(p) w.settleAll() id := IDOf("novox/app", readyHead) @@ -170,7 +168,7 @@ func rejectedBeforeItsAnswer(t *testing.T, pushedBy string) (*world, string) { // with no new head, in words naming its author (its branch, and the session that pushed it) and needing a new head — // never "stop it", never "the operator's". func TestARejectedDeliveryIsItsAuthorsForTwoHours(t *testing.T) { - w, id := rejectedBeforeItsAnswer(t, "quiet-deliveries@shanks") + w, id := rejectedBeforeItsAnswer(t, "quiet-deliveries") said := false for _, c := range w.forge.comments { said = said || strings.Contains(c, "not merged") @@ -188,7 +186,7 @@ func TestARejectedDeliveryIsItsAuthorsForTwoHours(t *testing.T) { t.Fatalf("not the operator's after two hours with no new head: %+v", got) } l := got[0] - if l.Needs != NeedsNewHead || l.Author != "the author of feat/x (quiet-deliveries on shanks)" || l.Branch != "feat/x" { + if l.Needs != NeedsNewHead || l.Author != "the author of feat/x (quiet-deliveries)" || l.Branch != "feat/x" { t.Fatalf("the line does not name its author and the new head it needs: %+v", l) } for _, never := range []string{"operator's", "stop it"} { @@ -196,13 +194,13 @@ func TestARejectedDeliveryIsItsAuthorsForTwoHours(t *testing.T) { t.Fatalf("the line says %q: %+v", never, l) } } - if !strings.Contains(l.Says, "needs a new head from the author of feat/x (quiet-deliveries on shanks)") { + if !strings.Contains(l.Says, "needs a new head from the author of feat/x (quiet-deliveries)") { t.Fatalf("the words do not say a new head is needed from its author: %q", l.Says) } } -// ADR 0505 §5: with no Pushed-by line, the branch alone names its author. -func TestARejectedDeliveryWithNoPushedByLineIsNamedByItsBranch(t *testing.T) { +// ADR 0505 §5: with no author in the forge's record, the branch alone names its author. +func TestARejectedDeliveryWithNoAuthorIsNamedByItsBranch(t *testing.T) { w, id := rejectedBeforeItsAnswer(t, "") w.later(2*time.Hour + time.Minute) if got := linesFor(w, id); len(got) != 1 || got[0].Author != "the author of feat/x" { @@ -212,7 +210,7 @@ func TestARejectedDeliveryWithNoPushedByLineIsNamedByItsBranch(t *testing.T) { // ADR 0505 §5: a new head within the two hours ends it: the rejected head is never the operator's. func TestANewHeadWithinTwoHoursKeepsTheRejectedHeadFromTheOperator(t *testing.T) { - w, id := rejectedBeforeItsAnswer(t, "quiet-deliveries@shanks") + w, id := rejectedBeforeItsAnswer(t, "quiet-deliveries") w.later(time.Hour) w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) w.settleAll() 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 f23058fa5..e322d10c4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/approval.go @@ -98,26 +98,14 @@ func phone(s string, n int) string { return s } -var issueNamed = regexp.MustCompile(`(?i)\bissues?\s*#?(\d+)`) - -// IssuesIn reads the issues a pull request's title and body name ("issue 948"), ascending, once each. -func IssuesIn(text string) []int { - seen := map[int]bool{} - var out []int - for _, m := range issueNamed.FindAllStringSubmatch(text, -1) { - if n, err := strconv.Atoi(m[1]); err == nil && !seen[n] { - seen[n] = true - out = append(out, n) - } - } - sort.Ints(out) - return out -} - +// issuesSaid is the issues linked to the deliveries' pull requests in the issue tracker, as the ask names them. func issuesSaid(ds []*Delivery) string { seen := map[int]bool{} var out []string for _, d := range ds { + if d.IssuesUnread != "" { + out = append(out, pullSaid(d)+"'s unread (the issue tracker did not answer)") + } for _, n := range d.Issues { if !seen[n] { seen[n] = true @@ -126,7 +114,7 @@ func issuesSaid(ds []*Delivery) string { } } if len(out) == 0 { - return "none named" + return "none linked" } return strings.Join(out, ", ") } @@ -440,6 +428,9 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { if waits := h.writeWaits(d, members, now); waits != "" { return waits, nil // asked only once the forge account can carry the merge out (writes.go, issue 1945) } + if read := h.issuesToRead(members); read != nil { + return "the issues linked to its pull request are read from the issue tracker", read + } group := "" if len(members) > 1 { group = d.Group 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 d52c1e910..3ebee8272 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 @@ -562,7 +562,6 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB announced := pr("novox/app", 7, head, "fix/x", "src/a.go") announced.BaseSHA = "a1a1a1a1a1a1" announced.Title = "Fix the apostrophe (issue 975)" - announced.Body = "The description, with no review line yet." b.publish(t, "mesh.seat.git.event.pull.updated", seatUpdated(announced)) rt.until("the pull request's key to hold its head", eventWithin, func() bool { e, err := b.kvs["pulls"].Get(ctx, "novox.app.7") @@ -754,7 +753,6 @@ func TestAGroupOfTwoWithGreenChecksGetsOneAskAndOnItsApproveBothMergeOneAfterThe for _, p := range []PullEvent{pr("novox/mesh-controller", 1, ctlHead, "feat/x", "internal/a.go"), pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")} { p.BaseSHA = "a1a1a1a1a1a1" - p.Body = "Reviewed-by: lead@shanks at " + p.HeadSHA b.publish(t, "mesh.seat.git.event.pull.updated", seatUpdated(p)) } // Both members pass at the checker, whose one verdict covers both heads: the group is asked once, no composed check is asked 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 c71ad614a..7e3192cdb 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 @@ -2,6 +2,7 @@ package main import ( gitseat "git.novox.be/novox/mesh-delivery/contracts/go/seats/git" + "reflect" "encoding/json" "strings" @@ -120,29 +121,31 @@ 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" } -// 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. +// The gate reads nothing from a pull request's description (novox/hq ADR 0493 §6, ADR 0659, which ends ADR 0496): its form +// of a pull request and its delivery hold no description, no review line, no pushed-by and no after: line, so no wait can +// read one; a pull request 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) - w.op.auto = false - p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") - p.Body = c.body - w.h.PullUpdated(p) - hear(t, w, shimCheckedKey, judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) - 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)) + for _, form := range []any{PullEvent{}, Delivery{}} { + ty := reflect.TypeOf(form) + for i := 0; i < ty.NumField(); i++ { + name := strings.ToLower(ty.Field(i).Name) + for _, never := range []string{"body", "description", "pushed", "after", "review"} { + if strings.Contains(name, never) { + t.Errorf("%s holds the field %s, a description's", ty.Name(), ty.Field(i).Name) + } } - if e := w.op.ask(open[0]).Explanation; strings.Contains(strings.ToLower(e), "review") { - t.Fatalf("the ask names a review:\n%s", e) - } - }) + } + } + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) + hear(t, w, shimCheckedKey, judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + 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) } } @@ -170,41 +173,3 @@ 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.askWaits(d), w.h.approvedWaits(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/holder.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go index a427ebdad..d2f1b5ed4 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/holder.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "slices" "sort" "strings" "sync" @@ -45,6 +44,8 @@ type Holder struct { Consumer *StepConsumer // SaySeat says an event of the seat mesh-delivery (recordevents.go): heads-replaced here; nil says none. SaySeat SeatSay + // Links reads the issues linked to a pull request from the issue tracker (issuelinks.go); nil reads none. + Links IssueLinks // Pulls keeps the pull requests' keys on the bus (pullkey.go); nil keeps none. Pulls PullKeys // Say says a condition to the operator, and Conditions keeps which the gate raised (conditions.go); nil says none. @@ -297,7 +298,6 @@ type PullEvent struct { Repo string `json:"repo"` Number int `json:"number"` Title string `json:"title,omitempty"` - Body string `json:"body,omitempty"` Base string `json:"base"` Head string `json:"head"` HeadSHA string `json:"head_sha"` @@ -357,14 +357,9 @@ 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) - // So is the line naming who pushed it (ADR 0505 §5). - if by := PushedIn(p.Body); p.Body != "" && by != d.PushedBy { - d.PushedBy = by - h.keep(d) - } - // An `after:` line written into the description later is told about too (novox/hq ADR 0419 §2). - if after := AfterIn(p.Body); p.Body != "" && !slices.Equal(after, d.After) { - d.After = after + // So is who opened it, from the forge's pull request record (ADR 0505 §5, ADR 0659 §4). + if p.Author != "" && p.Author != d.Author { + d.Author = p.Author h.keep(d) } if d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil { @@ -449,8 +444,9 @@ func (h *Holder) fillFromAnnouncement(d *Delivery, p PullEvent) { d.Title, d.Base, d.Branch, d.HTMLURL, d.CloneURL = p.Title, p.Base, p.Head, p.HTMLURL, p.CloneURL 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.PushedBy = PushedIn(p.Body) + if p.Author != "" { + d.Author = p.Author + } if p.BaseSHA != "" { d.BaseSHA, d.BaseSHAAt = p.BaseSHA, h.Now() } @@ -485,9 +481,9 @@ func (h *Holder) retarget(d *Delivery, p PullEvent) { h.keep(d) } -// 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. +// PullEdited takes a pull request's edit (novox/hq issue 1079): a delivery made from its verdict is filled from it, and a +// record stopped under another, closed pull request at this head is taken over; nothing of its description is read +// (novox/hq ADR 0659), and nothing here holds or withdraws an ask. func (h *Holder) PullEdited(p PullEvent) { if p.HeadSHA == "" || p.Number == 0 { return @@ -515,20 +511,6 @@ func (h *Holder) PullEdited(p PullEvent) { if d != nil && d.Number == p.Number { h.fillFromAnnouncement(d, p) // a delivery made from its verdict, its head never heard (issue 1726) } - if p.Body == "" { - return - } - if d == nil || d.Number != p.Number || d.State.Final() || d.MergedAs != "" { - return - } - d.PushedBy = PushedIn(p.Body) - d.After = AfterIn(p.Body) - title := p.Title - if title == "" { - title = d.Title - } - d.Issues = IssuesIn(title + "\n" + p.Body) - h.keep(d) } // withOther is others with p put in, replacing an earlier announcement of the same pull request. @@ -581,11 +563,10 @@ func (h *Holder) freshAnnouncement(p PullEvent, now time.Time, startedOver strin d := &Delivery{ID: IDOf(p.repository(), p.HeadSHA), Repository: p.repository(), Commit: p.HeadSHA, Number: p.Number, Title: p.Title, Base: p.Base, Branch: p.Head, HTMLURL: p.HTMLURL, CloneURL: p.CloneURL, Paths: p.Paths, PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid, - After: AfterIn(p.Body), Issues: IssuesIn(p.Title + "\n" + p.Body), Created: now, BaseSHA: p.BaseSHA} + Author: p.Author, Created: now, BaseSHA: p.BaseSHA} if p.BaseSHA != "" { d.BaseSHAAt = now } - d.PushedBy = PushedIn(p.Body) why := fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number) if startedOver != "" { why = fmt.Sprintf("%s; %s", why, startedOver) @@ -756,7 +737,7 @@ func (h *Holder) pullMerged(p PullEvent, late bool) { d = &Delivery{ID: IDOf(p.repository(), head), Repository: p.repository(), Commit: head, Number: p.Number, Title: p.Title, Base: p.Base, Branch: p.Head, HTMLURL: p.HTMLURL, CloneURL: p.CloneURL, Paths: p.Paths, PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid, - After: AfterIn(p.Body), Issues: IssuesIn(p.Title + "\n" + p.Body), Created: now, FromForge: true} + Author: p.Author, Created: now, FromForge: true} // Its check, as the forge holds it on the head: the verdict the pull request merged with. if s := p.HeadChecks[ctxCheck]; s != "" { d.Check = verdictOfCheck(s, "", now) 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 73ec828ae..c369104d7 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 @@ -2,8 +2,12 @@ package main import ( "encoding/json" + "errors" seat "git.novox.be/novox/mesh-delivery/contracts/go/seats/meshdelivery" + "sort" + "strconv" "strings" + "sync" "testing" "time" @@ -263,13 +267,16 @@ func TestAMergedGroupEndsMergedWithSourceMainMoved(t *testing.T) { } } -// ADR 0419 §2, issue 978: a description's `after:` line is not read, and says so once on its pull request. The -// composed check is asked as for any other group: no order is asked of the controller first, and none gates it. -func TestAnAfterLineIsNotReadAndSaidOnceAndNothingWaitsForAnOrder(t *testing.T) { +// W11.5 (novox/hq ADR 0659 §4, ADR 0493 §4): the issues an ask names are the issue tracker's links of each member's pull +// request, read through the seat issue-tracker, never the title; a tracker that does not answer is said, and holds nothing; +// the author is the forge's pull request record's. ADR 0419 §2's after: notice is gone with the description. +func TestTheAskNamesTheIssuesLinkedInTheIssueTracker(t *testing.T) { w := newWorld(t) w.op.auto = false + links := &fakeLinks{linked: map[string][]int{"novox/a#1": {948, 12}}, failing: map[string]bool{"novox/b#2": true}} + w.h.Links = links pa := pr("novox/a", 1, "a1a1a1a1a1a1a1", "feat/z") - pa.Body = "Goes after the other.\n\nafter: novox/b\n" + pa.Title, pa.Author = "Fix issue 77", "quiet-deliveries" w.h.PullUpdated(pa) w.h.PullUpdated(pr("novox/b", 2, "b2b2b2b2b2b2b2", "feat/z")) w.h.Checked(verdict("novox/a", 1, "a1a1a1a1a1a1a1", "pass")) @@ -277,29 +284,49 @@ func TestAnAfterLineIsNotReadAndSaidOnceAndNothingWaitsForAnOrder(t *testing.T) w.settleAll() w.later(time.Minute) w.settleAll() - said := 0 + open := w.op.open() + if len(open) != 1 { + t.Fatalf("the group with an unread tracker is not asked: %v", open) + } + e := w.op.ask(open[0]).Explanation + if !strings.Contains(e, "Issues: issue 12, issue 948, b#2's unread (the issue tracker did not answer)") || strings.Contains(e, "77") { + t.Fatalf("the ask's issues are not the links:\n%s", e) + } + if d := w.delivery(IDOf("novox/a", "a1a1a1a1a1a1a1")); d.Author != "quiet-deliveries" || authorOf(d) != "the author of feat/z (quiet-deliveries)" { + t.Fatalf("the author is %q, said %q", d.Author, authorOf(d)) + } + if links.asked["novox/a#1"] != 1 { + t.Fatalf("the links of novox/a#1 were read %d times at one head, want once", links.asked["novox/a#1"]) + } for _, c := range w.forge.comments { - if strings.Contains(c, "not read") && strings.Contains(c, "novox/a#1") { - said++ + if strings.Contains(c, "after:") { + t.Fatalf("an after: notice is said: %q", c) } } - if said != 1 { - t.Fatalf("the after: line is not said once on its pull request: %v", w.forge.comments) +} + +// fakeLinks is the issue tracker's links of pull requests, by `#`. +type fakeLinks struct { + mu sync.Mutex + linked map[string][]int + failing map[string]bool + asked map[string]int +} + +func (f *fakeLinks) Linked(repository string, number int) ([]int, error) { + f.mu.Lock() + defer f.mu.Unlock() + key := repository + "#" + strconv.Itoa(number) + if f.asked == nil { + f.asked = map[string]int{} } - // A new head of the same pull request, with the line still there, is not told again. - pa2 := pr("novox/a", 1, "a9a9a9a9a9a9a9", "feat/z") - pa2.Body = pa.Body - w.h.PullUpdated(pa2) - w.settleAll() - said = 0 - for _, c := range w.forge.comments { - if strings.Contains(c, "not read") { - said++ - } - } - if said != 1 { - t.Fatalf("said again for a new head: %v", w.forge.comments) + f.asked[key]++ + if f.failing[key] { + return nil, errors.New("no responders") } + out := append([]int(nil), f.linked[key]...) + sort.Ints(out) + return out, nil } // Stalled: a delivery past its state's bound is listed with what H2 may do; close takes only that. Only a proposed 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 427a8f01b..8f5416c20 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 @@ -18,7 +18,6 @@ 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 = "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) @@ -56,7 +55,6 @@ func TestAnEditOfAPullRequestWhoseSharedHeadsRecordIsStoppedUnderAnotherTakesItO t.Fatalf("setup: %s", d.State) } e := pr(gCtl, 379, head1114, "feat/chain-2") - e.Body = "Fixes issue 379." w.h.PullEdited(e) d := w.delivery(id) if d.Number != 379 || d.State != Proposed || len(d.Paths) == 0 { @@ -77,7 +75,6 @@ func TestAMergedRecordIsNeverTakenOverByTheOtherPullRequestOfItsHead(t *testing. w.h.PullMerged(merged(gCtl, 378, head1114, "f378f378f378f378")) w.h.PullClosed(pr(gCtl, 378, head1114, "feat/chain")) e := pr(gCtl, 379, head1114, "feat/chain-2") - e.Body = "text" w.h.PullEdited(e) if d := w.delivery(id); d.Number != 378 || d.MergedAs == "" { t.Fatalf("a merged record was taken over: #%d merged as %q", d.Number, d.MergedAs) 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 120653b6d..99861eaa7 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 @@ -11,8 +11,6 @@ import ( // branch, title, URLs or review record. The ready pass skipped it in silence, so no Approve was ever asked, and the // announcement of the same head, heard later, filled none of it. -const verdictFirstBody = "Fixes a bug.\n\nReviewed-by: reviewer@shanks at " + readyHead + "\n" - // verdictFirst is the live shape: the verdict heard, the head not, and the delivery ready. func verdictFirst(t *testing.T) (*world, string) { w := newWorld(t) @@ -48,7 +46,7 @@ func TestADeliveryWithoutItsHeadSaysWhatItWaitsForAndAsksNoOne(t *testing.T) { func TestAnAnnouncementFillsADeliveryMadeFromItsVerdictAndTheOperatorIsAsked(t *testing.T) { w, id := verdictFirst(t) p := pr("novox/app", 419, readyHead, "fix/x", "src/a.go") - p.Title, p.Body, p.CloneURL = "Fix x", verdictFirstBody, "https://forge.invalid/novox/app.git" + p.Title, p.CloneURL = "Fix x", "https://forge.invalid/novox/app.git" w.h.PullUpdated(p) w.settleAll() d := w.delivery(id) @@ -112,7 +110,7 @@ func TestAnAnnouncementFillingAVerdictFirstHeadSupersedesTheOlderHeadAndItsAsk(t func TestAnEditOfThePullRequestFillsADeliveryMadeFromItsVerdict(t *testing.T) { w, id := verdictFirst(t) p := pr("novox/app", 419, readyHead, "fix/x") - p.Title, p.Body = "Fix x", verdictFirstBody + p.Title = "Fix x" w.h.PullEdited(p) w.settleAll() d := w.delivery(id) diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issuelinks.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issuelinks.go new file mode 100644 index 000000000..e30287678 --- /dev/null +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/issuelinks.go @@ -0,0 +1,106 @@ +package main + +// The issues an ask names, from the issue tracker's links (novox/hq ADR 0493 §4, ADR 0659 §4): a pull request is linked to +// the issue it fixes by a tool call of the seat issue-tracker, never matched from its title or description, and the gate +// reads those links through the seat's verb `list`. Words only: an ask is never held for them; a read that fails is said in +// the ask as the tracker unread, and read again at the pull request's next head. + +import ( + "encoding/json" + "fmt" + "sort" + "strconv" +) + +// TrackerListVerb is the seat issue-tracker's verb the gate reads a pull request's issues with (the manifest invokes it). +const TrackerListVerb = "seat:issue-tracker.list" + +// IssueLinks reads the issues linked to a pull request in the issue tracker. +type IssueLinks interface { + // Linked is the issues linked to the pull request number of repository (owner/name), ascending. + Linked(repository string, number int) ([]int, error) +} + +// seatIssues is the issue tracker through its seat. +type seatIssues struct{ ask asking } + +func (s seatIssues) Linked(repository string, number int) ([]int, error) { + // A pull request is linked as `#`, lower-case, as the tracker keeps it. + target := "pull-request:" + repoName(repository) + "#" + strconv.Itoa(number) + raw, err := s.ask(TrackerListVerb, map[string]any{"linked": target, "limit": 200}) + if err != nil { + return nil, err + } + answer, output, ok := answerOf(raw) + if !ok { + return nil, fmt.Errorf("the seat issue-tracker refused list: %s", lastLine(output)) + } + var listed struct { + Rows []struct { + Number int `json:"number"` + } `json:"rows"` + Error string `json:"error"` + } + if err := json.Unmarshal(answer, &listed); err != nil { + return nil, fmt.Errorf("the seat issue-tracker's list is not readable: %s", clip(string(answer), 200)) + } + if listed.Error != "" { + return nil, fmt.Errorf("the seat issue-tracker could not list %s: %s", target, listed.Error) + } + out := make([]int, 0, len(listed.Rows)) + for _, r := range listed.Rows { + out = append(out, r.Number) + } + sort.Ints(out) + return out, nil +} + +// issuesToRead is the read of the linked issues of every member not read at its head yet, done outside the lock; nil when +// all are read or no issue tracker is reached. Under the lock. +func (h *Holder) issuesToRead(members []*Delivery) func() { + if h.Links == nil { + return nil + } + type read struct { + id, repository, head, why string + number int + issues []int + } + var reads []read + for _, m := range members { + if m.Number != 0 && m.IssuesRead != m.Commit { + reads = append(reads, read{id: m.ID, repository: m.Repository, head: m.Commit, number: m.Number}) + } + } + if len(reads) == 0 { + return nil + } + for _, m := range members { + if m.issuesReading == m.Commit && m.IssuesRead != m.Commit { + return func() {} // read already: its end looks again + } + } + for _, m := range members { + m.issuesReading = m.Commit + } + return func() { + for i := range reads { + r := &reads[i] + issues, err := h.Links.Linked(r.repository, r.number) + r.issues = issues + if err != nil { + r.why = err.Error() + h.Logf("[mesh-delivery] %s: the issues linked to its pull request cannot be read: %v; its ask says so", r.id, err) + } + } + h.mu.Lock() + for _, r := range reads { + if d := h.deliveries[r.id]; d != nil && d.Commit == r.head { + d.Issues, d.IssuesRead, d.IssuesUnread = r.issues, r.head, r.why + h.keep(d) + } + } + h.mu.Unlock() + h.lookAt(h.Now(), "an ask whose issues were read from the issue tracker") + } +} 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 bf165028f..5c47895e1 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/main.go @@ -61,6 +61,7 @@ func main() { Settings: settings, Store: kvStore{}, Forge: seatForge{ask: stdio.Ask}, + Links: seatIssues{ask: stdio.Ask}, Emit: func(event string, body any) error { return stdio.Emit(event, body) }, Logf: logf, Operator: kvOperator{}, diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/manifest_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/manifest_test.go index dcc245082..fc3194694 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/manifest_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/manifest_test.go @@ -144,7 +144,8 @@ func TestItClaimsTheDeliverySeatAndFollowsWhatItHandles(t *testing.T) { // merge state through the one statuses tool the seat does not carry yet (seatForge.mergeState); the forge's writes are the // bridge's (ADR 0437 §8). It uses operator-channel only to say its conditions (ADR 0437 §9, issue 1275): the teller asks // (ADR 0437 §6). - want := []string{"seat:git.pull", "seat:git.statuses", "seat:git.open-pulls", "seat:git.contains", "seat:mesh-hand-acts.hand-act"} + want := []string{"seat:git.pull", "seat:git.statuses", "seat:git.open-pulls", "seat:git.contains", "seat:mesh-hand-acts.hand-act", + TrackerListVerb} if !reflect.DeepEqual(m.Uses, []string{"operator-channel"}) { t.Fatalf("uses %v: operator-channel, for its conditions alone", m.Uses) } 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 6ff3e41e3..340bd7dac 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/model.go @@ -3,7 +3,6 @@ package main import ( "encoding/json" "fmt" - "regexp" "sort" "strings" "time" @@ -32,15 +31,16 @@ type Delivery struct { Removed []string `json:"removed,omitempty"` ModuleDirs []string `json:"module_dirs,omitempty"` ModuleDirsSaid bool `json:"module_dirs_said,omitempty"` - // After are the repositories its pull request's description names in `after: ` lines. They are not read - // (novox/hq ADR 0419 §2): kept only to say once on the pull request that the line is not read. AfterSaid is whether - // that was said, for this pull request. - After []string `json:"after,omitempty"` - AfterSaid bool `json:"after_said,omitempty"` // Comments is how many comments this owner owed on the pull request at this head, which numbers the next. Comments int `json:"comments,omitempty"` - // Issues are the issues its title and body name ("issue 948"), shown to the operator who is asked to approve its merge. - Issues []int `json:"issues,omitempty"` + // Issues are the issues linked to its pull request in the issue tracker (novox/hq ADR 0493 §4, ADR 0659 §4), shown to the + // operator who is asked to approve its merge; IssuesRead is the head they were read at, IssuesUnread why the issue + // tracker could not say them. Words only: nothing waits on them. + Issues []int `json:"issues,omitempty"` + IssuesRead string `json:"issues_read,omitempty"` + IssuesUnread string `json:"issues_unread,omitempty"` + // issuesReading is a read of them in flight, so a look meanwhile does not ask it again; not kept. + issuesReading string // Group is the delivery group it is in: its head branch name. Empty for a lone delivery. Group string `json:"group,omitempty"` @@ -99,9 +99,9 @@ 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"` - // 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"` + // Author is who opened its pull request, as the forge's pull request record says (the seat git's events): named with + // its branch when a new head is needed (novox/hq ADR 0505 §5, ADR 0659 §4). + Author string `json:"author,omitempty"` // FreshWanted is whether a passing verdict that arrived out of date is to have its check asked against the // trunk's head, before the operator is asked (novox/hq ADR 0421 §3, issue 1022). Cleared when it is asked. FreshWanted bool `json:"fresh_wanted,omitempty"` @@ -284,44 +284,19 @@ func sameCommit(a, b string) bool { return len(a) >= 7 && strings.HasPrefix(strings.ToLower(b), strings.ToLower(a)) } -// 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*$`) - -// PushedIn reads it: the last such line, or "" when there is none. -func PushedIn(body string) string { - who := "" - for _, m := range pushedLine.FindAllStringSubmatch(body, -1) { - who = m[1] - } - return who -} - -// authorOf is a delivery's author as the operator reads it: "the author of ", and the session that pushed it -// when its description names one ("who on node": the router refuses an @ in its words). +// authorOf is a delivery's author as the operator reads it: "the author of ", and who opened its pull request as +// the forge's record says (the router refuses an @ in its words). func authorOf(d *Delivery) string { who := "the author of " + d.Branch if d.Branch == "" { who = "its author" } - if d.PushedBy != "" { - who += " (" + strings.Replace(d.PushedBy, "@", " on ", 1) + ")" + if d.Author != "" { + who += " (" + strings.Replace(d.Author, "@", " on ", 1) + ")" } return who } -// afterLines are the `after: ` lines of a pull request's description. -var afterLines = regexp.MustCompile(`(?im)^\s*after:\s*([A-Za-z0-9_.\-/]+)\s*$`) - -// AfterIn reads them. -func AfterIn(body string) []string { - var out []string - for _, m := range afterLines.FindAllStringSubmatch(body, -1) { - out = append(out, strings.TrimSuffix(m[1], ".git")) - } - sort.Strings(out) - return out -} - // Group is a delivery group: two or more deliveries sharing a head branch name, one level only. type Group struct { ID string `json:"id"` diff --git a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus_bus_test.go b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus_bus_test.go index d5dfaa0c0..ae815a941 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus_bus_test.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/onestatus_bus_test.go @@ -39,7 +39,6 @@ func TestADeliveryOnMeshCheckGoesReadyAskedAndMergedOverARealBus(t *testing.T) { Statuses: []string{}, Checks: map[string]HeadCheck{}}} rt.mu.Unlock() announced := pr("novox/app", 8, head, "fix/y", "src/b.go") - announced.Body = "Reviewed-by: lead@shanks at " + head b.publish(t, "mesh.seat.git.event.pull.updated", seatUpdated(announced)) // The checker's verdict at the head, then the forge's word that its bridge set mesh/check. 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 649f99a27..7ce09b063 100644 --- a/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go +++ b/modules/mesh-delivery-gate/cmd/mesh-delivery-gate/ready.go @@ -212,9 +212,6 @@ func (h *Holder) readyAsks(now time.Time) []func() { h.Logf("[mesh-delivery] %s is ready (%s); the operator's Approve of its merge into main is asked once it may merge", d.ID, h.checkAt([]*Delivery{d}).Words) } - if a := h.sayAfterLine(d); a != nil { - asks = append(asks, a) - } r := d.Ready if r == nil || r.Merged != nil || r.declined() { continue @@ -315,26 +312,6 @@ func (h *Holder) readyAsks(now time.Time) []func() { return asks } -// sayAfterLine tells a pull request, once, that its description holds an `after: ` line, which is not read -// (novox/hq ADR 0419 §2): the order of a delivery comes from the module graph alone, and no refusal asks for the line. -// Once per pull request, not per head. Under the lock; the comment is said outside it. -func (h *Holder) sayAfterLine(d *Delivery) func() { - if len(d.After) == 0 || d.AfterSaid { - return nil - } - d.AfterSaid = true - for _, o := range h.deliveries { - if o != d && o.Number == d.Number && o.Repository == d.Repository && o.AfterSaid { - h.keep(d) - return nil // said on this pull request for an earlier head - } - } - h.keep(d) - h.Logf("[mesh-delivery] %s: its description holds an after: line (%s), which is not read", d.ID, strings.Join(d.After, ", ")) - return h.sayOn(d, "mesh-delivery: this description holds an `after:` line ("+strings.Join(d.After, ", ")+"). It is not read: "+ - "the order of a delivery comes from the module graph alone, and nothing asks for the line (novox/hq ADR 0419).") -} - // mergeAgainNow lets an approved delivery's merge be asked at once: something that decides whether it may merge happened (a // verdict, an Approve, a group's composed check), so the wait after a refusal does not hold it. Under the lock. func (h *Holder) mergeAgainNow(d *Delivery) { diff --git a/modules/mesh-delivery-gate/module.json b/modules/mesh-delivery-gate/module.json index e462c942a..9677d8801 100644 --- a/modules/mesh-delivery-gate/module.json +++ b/modules/mesh-delivery-gate/module.json @@ -294,7 +294,8 @@ "seat:git.statuses", "seat:git.open-pulls", "seat:git.contains", - "seat:mesh-hand-acts.hand-act" + "seat:mesh-hand-acts.hand-act", + "seat:issue-tracker.list" ], "state": [ "deliveries",