diff --git a/modules/gitea/test/webhook.test.ts b/modules/gitea/test/webhook.test.ts index ea8d54cf..cf2450c7 100644 --- a/modules/gitea/test/webhook.test.ts +++ b/modules/gitea/test/webhook.test.ts @@ -333,3 +333,20 @@ test("the polls look every 30 s until events are heard, every five minutes while pace.heard(); assert.equal(pace.every(), 300_000, "the next delivery"); }); + +// Issue 1079: a pull request's edit event carries its description, so mesh-delivery reads a Reviewed-by line written after +// the head was announced. Other pull request events do not carry it (they are many, and the description is long). +test("a pull request's edit event carries its description, and no other pull request event does (issue 1079)", async () => { + const { eventOf } = await import("../webhook.ts"); + const pull = { number: 3, title: "t", body: "Reviewed-by: kb@g14 at " + "a".repeat(40), head: { ref: "feat/x", sha: "a".repeat(40) }, + base: { ref: "main" } }; + const repository = { name: "hq", owner: { login: "novox" } }; + const edited = eventOf("pull_request", { action: "edited", pull_request: pull, repository }); + assert.equal(edited.name, "forge.pull-request.edited"); + assert.equal(edited.body.body, pull.body); + assert.equal(edited.body.head_sha, "a".repeat(40)); + assert.equal(edited.body.number, 3); + for (const action of ["opened", "synchronized", "reopened"]) { + assert.equal(eventOf("pull_request", { action, pull_request: pull, repository }).body.body, undefined, action); + } +}); diff --git a/modules/gitea/webhook.ts b/modules/gitea/webhook.ts index 87a072c4..b0034e94 100644 --- a/modules/gitea/webhook.ts +++ b/modules/gitea/webhook.ts @@ -112,7 +112,9 @@ function forgeEventOf(said: string, payload: any): ForgeEvent { base: str(pull.base?.ref), merged: !!pull.merged, merge_commit_sha: str(pull.merge_commit_sha), merged_at: str(pull.merged_at), html_url: str(pull.html_url), labels: Array.isArray(pull.labels) ? pull.labels.map((l: any) => str(l?.name)).filter(Boolean) : undefined, - label: str(payload.label?.name) } }; + label: str(payload.label?.name), + // An edit carries the description (issue 1079): mesh-delivery reads the review record in it again. + ...(action === "edited" ? { body: str(pull.body) } : {}) } }; } if (kind === "issue_comment") { const issue = payload.issue ?? {}; diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval.go b/modules/mesh-delivery/cmd/mesh-delivery/approval.go index 2b4f2bc0..524bdcaa 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/approval.go @@ -380,6 +380,27 @@ func (h *Holder) dropAsk(id, why string) func() { } } +// withdrawAsk takes back an ask nobody answered, on every delivery it covers, so the next look asks again with what is +// known now (issue 1079). Unlike dropAsk it ends no Approve and says nothing on a pull request: there was none. Under the +// lock; the router's cancel runs with the look's asks. +func (h *Holder) withdrawAsk(id, why string) func() { + for _, d := range h.deliveries { + if d.Ready != nil && d.Ready.AskID == id && d.Ready.Approved == nil { + d.Ready.AskID, d.Ready.Ask, d.Ready.Heads = "", nil, nil + h.keep(d) + } + } + h.Logf("[mesh-delivery] the operator's ask %s is withdrawn to be asked again: %s", id, why) + return func() { + if h.Operator == nil { + return + } + if err := h.Operator.Cancel(id); err != nil { + h.Logf("[mesh-delivery] the operator's ask %s could not be withdrawn (%v)", id, err) + } + } +} + // pendingAsks are the asks waiting for an answer, by id; under the lock. func (h *Holder) pendingAsks() []string { seen := map[string]bool{} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 749fbabe..386d2729 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -396,6 +396,38 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { return isNew } +// 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. The record is never a gate (ADR 0421 §5). 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 || p.Body == "" { + return + } + h.mu.Lock() + defer h.mu.Unlock() + d := h.deliveries[IDOf(p.repository(), p.HeadSHA)] + if d == nil || d.Number != p.Number || d.State.Final() || d.MergedAs != "" { + return + } + before, _ := reviewWords(d) + d.ReviewedBy, d.ReviewedAt = ReviewIn(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) + if after, _ := reviewWords(d); after == before { + return + } + if r := h.covering(d); r != nil && r.AskID != "" && r.Approved == nil && r.Declined == nil { + h.cancels = append(h.cancels, h.withdrawAsk(r.AskID, d.ID+"'s review record changed: "+shortOf(d.ReviewedAt, 12))) + } +} + // startedOverFor is why a delivery already kept at a head's id is started over for a pull request's number rather // than taken as announced again: it is Stopped, never merged, and under another number. Stopped by an earlier, closed // duplicate pull request with the same head (novox/hq issue 783), or by a person or a group before its pull request diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1079_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1079_test.go new file mode 100644 index 00000000..c5cb6cec --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue1079_test.go @@ -0,0 +1,101 @@ +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). + +// askedUnreviewed is a green pull request whose Approve is asked while its description holds no Reviewed-by line. +func askedUnreviewed(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)" + 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, "not reviewed at this head") { + 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 TestAReviewedByLineWrittenAfterTheAskWithdrawsTheUnansweredAskAndAsksAgain(t *testing.T) { + w := askedUnreviewed(t) + first := w.op.open()[0] + w.h.PullEdited(edited("Reviewed-by: reviewer@shanks at " + readyHead)) + w.settleAll() + open := w.op.open() + 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 one new", open, first, w.op.count()) + } + if len(w.op.cancelled) != 1 || w.op.cancelled[0] != first { + t.Fatalf("the first ask was not withdrawn: %v", w.op.cancelled) + } + if e := w.op.ask(open[0]).Explanation; !strings.Contains(e, "reviewed by reviewer on shanks at "+shortOf(readyHead, 12)) || + strings.Contains(e, "not reviewed") { + 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 := askedUnreviewed(t) + // The same words: no description with a line, then a description without one. + w.h.PullEdited(edited("A new sentence in the description.")) + 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) + } + // An edit that brings a line for another head changes the words: the ask is made again, still saying not reviewed. + w.h.PullEdited(edited("Reviewed-by: reviewer@shanks at " + strings.Repeat("a", 40))) + w.settleAll() + if w.op.count() != 2 || len(w.op.open()) != 1 { + t.Fatalf("a line for another head: %d asks, open %v", w.op.count(), w.op.open()) + } + // 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: reviewer@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 != "reviewer@shanks" { + t.Fatalf("the record is still read: %q", d.ReviewedBy) + } +} + +func TestAnEditOfAnUnknownOrMergedOrEmptyPullRequestChangesNothing(t *testing.T) { + w := askedUnreviewed(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/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go index 2574d401..68bb0f56 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/main.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/main.go @@ -34,7 +34,7 @@ func (l *listening) get() string { l.mu.Lock(); defer l.mu.Unlock(); return l.n // The events this owner follows: the forge's pull requests, and the controller's verdicts and walks. // The forge's status events say that a check answered (events.go). var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", - "gitea.forge.status", ControllerSeat + ".checked", ControllerSeat + ".plan-moved"} + "gitea.forge.status", "gitea.forge.pull-request.edited", ControllerSeat + ".checked", ControllerSeat + ".plan-moved"} func main() { operator := &asker.Client{Module: Module, State: stdio.State("asked")} @@ -149,6 +149,14 @@ func take(h *Holder, e stdio.Envelope) error { default: 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. + var p PullEvent + if err := json.Unmarshal(e.Body, &p); err != nil { + h.Refuse(fmt.Errorf("%s is not readable: %v", e.Key, err)) + return nil + } + h.PullEdited(p) case "gitea.forge.status": var s ForgeStatusEvent if err := json.Unmarshal(e.Body, &s); err != nil { diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json index 4359918c..5f2f180d 100644 --- a/modules/mesh-delivery/module.json +++ b/modules/mesh-delivery/module.json @@ -28,6 +28,7 @@ "gitea.pull.merged", "gitea.pull.closed", "gitea.forge.status", + "gitea.forge.pull-request.edited", "mesh-controller.checked", "mesh-controller.plan-moved" ],