Read a pull request's review record again when its description is edited, and ask the Approve again if it was unanswered (issue 1079)
This commit is contained in:
@@ -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);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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 ?? {};
|
||||
|
||||
@@ -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{}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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"
|
||||
],
|
||||
|
||||
Reference in New Issue
Block a user