From da094008ea1e2c28a2ac69804fa19a795a9b46c5 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:08:26 +0200 Subject: [PATCH 1/8] A group's composed verdict names each member's siblings at the heads it composed, so a sibling's own merge is no move (issue 1049) --- modules/gitea/index.ts | 13 ++++- modules/gitea/siblings.ts | 31 +++++++++++ modules/gitea/test/guard.test.ts | 50 +++++++++++++++++ modules/gitea/test/siblings.test.ts | 55 +++++++++++++++++++ .../cmd/mesh-delivery/fakes_test.go | 5 ++ .../cmd/mesh-delivery/group_test.go | 31 +++++++++++ 6 files changed, 184 insertions(+), 1 deletion(-) diff --git a/modules/gitea/index.ts b/modules/gitea/index.ts index 0d591a56..680769cd 100644 --- a/modules/gitea/index.ts +++ b/modules/gitea/index.ts @@ -46,7 +46,7 @@ import { CHECK_CONTEXT, REPO_CHECK_CONTEXT, commentFor, composedGate, groupGateS repoCheckFacts, reposToLook } from "./pulls.js"; import { GateWriter, clonedAs, type BuildStarted } from "./gate.js"; import { HOOK_PATH, HOOK_PORT, Pace, claimHead, ensureSystemHook, eventsTool, keptSecret, receiver, type ForgeEvent } from "./webhook.js"; -import { keepSiblings, noteVerdict, readSiblings, siblingsFile, siblingsOf, weighSiblings } from "./siblings.js"; +import { keepSiblings, noteComposed, noteVerdict, readSiblings, siblingsFile, siblingsOf, weighSiblings } from "./siblings.js"; import { announcementOf, keepTook, lateVerdictComment, outOfDateWords, sameCommit, tookFile, weighTrunks } from "./trunk.js"; // Without a way to a token — configured, or mintable with the admin account (token.ts) — there is @@ -423,6 +423,9 @@ async function setVerdict(client: GiteaClient, event: { body: unknown }): Promis * writer (gate.ts), judged against the trunk as a member's own verdict is: one that names a trunk commit that is no longer * the trunk's head is out of date when it lands, and said so, pending. */ async function setGroupVerdict(client: GiteaClient, c: Checked): Promise { + // The base branch each member merges into, read with its pull request below: the composed verdict names the siblings of + // each member from it (issue 1049). + const bases = new Map(); for (const m of c.members ?? []) { if (!m.owner || !m.repo || !m.commit || !c.verdict) { console.error(`[gitea] a composed verdict of ${c.group} names a member with no repository, commit or verdict; ignored`); @@ -431,6 +434,7 @@ async function setGroupVerdict(client: GiteaClient, c: Checked): Promise { await gates.verdict(m.owner, m.repo, m.commit, async () => { const pull = m.number ? await client.getPullRequest(m.owner, m.repo, m.number).catch(() => undefined) : undefined; const target = pull?.html_url || undefined; + if (m.number && pull?.base) bases.set(`${m.owner}/${m.repo}#${m.number}`, pull.base); let status = groupGateStatus(c, m); if (m.trunk && pull?.base) { const head = await client.branchHead(m.owner, m.repo, pull.base).catch(() => undefined); @@ -441,6 +445,13 @@ async function setGroupVerdict(client: GiteaClient, c: Checked): Promise { `its merge gate ${status.state}`); }, c.id); } + // The siblings of each member are the other members at the heads and base commits this verdict composed (issue 1049): a + // sibling's own merge of such a head is no move of the member's base. Kept for what the merge tool and the siblings' + // weighing read; nothing is run. + if (c.verdict && c.members?.length) { + const members = c.members.map((m) => ({ ...m, base: m.number ? bases.get(`${m.owner}/${m.repo}#${m.number}`) : undefined })); + if (noteComposed(siblingNamed, members)) keepSiblings(siblingsRecord, siblingNamed); + } } // **This module is the one writer of a head's mesh/merge-gate** (novox/hq issue 446): the announcement's pending words, diff --git a/modules/gitea/siblings.ts b/modules/gitea/siblings.ts index 5c970d37..ae9b6c58 100644 --- a/modules/gitea/siblings.ts +++ b/modules/gitea/siblings.ts @@ -130,6 +130,37 @@ export function noteVerdict(record: SiblingRecord, key: string, head: string, si return true; } +/** One member of a composed verdict, as the group's composed check read it: its pull request, the head it judged, the + * trunk commit that head was merged onto, and the branch the pull request merges into. */ +export interface ComposedMember { + owner: string; + repo: string; + number?: number; + commit: string; + trunk?: string; + base?: string; +} + +/** + * **A group's composed verdict names, for each member, the other members at the heads and base commits it composed** + * (novox/hq ADR 0403 §5, issue 1049): the composed check read every member's head together, so it is the verdict to trust + * for the siblings. Without this, the record held what a member's OWN verdict named, which went stale when a sibling got + * a new head; the sibling's own merge of the head the composed check judged then read as another head's merge, put the + * member's gate back to pending, and the group merged in two passes. A member the verdict cannot name (no pull request + * number, trunk commit or base branch), or a verdict of fewer than two such members, changes nothing. Answers whether + * the record changed. + */ +export function noteComposed(record: SiblingRecord, members: ComposedMember[]): boolean { + const named = members.filter((m) => m.owner && m.repo && m.number && m.commit && m.trunk && m.base); + if (named.length < 2 || named.length !== members.length) return false; + for (const m of named) { + const siblings = named.filter((o) => o !== m).map((o): SiblingNamed => ({ + repository: `${o.owner}/${o.repo}`, number: o.number!, head: o.commit, base: o.base!, base_commit: o.trunk! })); + record[memberKey(m.owner, m.repo, m.number!)] = { head: m.commit, siblings }; + } + return true; +} + // --- what a sibling's move is --------------------------------------------------------------------------------- /** What weighing a sibling reads of the forge. */ diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index c6643d51..a654393a 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -630,3 +630,53 @@ test("gitea_merge_pull_request refuses a head the Approve did not name", async ( assert.equal(state.merged, false); assert.equal(((await merge(["novox/mesh-catalog@" + REVIEWED.toUpperCase()])) as Record).merged, true); }); + +// **A group approved together merges in one pass, on the real merge tool** (novox/hq ADR 0418, issue 1049): the member's +// own verdict named its sibling at an old head; the sibling got a new head, the group's composed verdict passed for both, +// the sibling merged that head, and the member then merges on its first ask: the sibling's own merge is no move of its +// base. The records are the module's own (siblings.ts), the weighing is what the sibling's merge announcement does. +test("a group of two merges in one pass: the sibling's merge of the composed head does not stop the member's merge (issue 1049)", async () => { + const { keepSiblings, siblingsFile, noteComposed, weighSiblings } = await import("../dist/siblings.js"); + const both = ["novox/mesh-catalog@" + REVIEWED, "novox/mesh-host@" + SIBLING_HEAD]; + const OLD_SIBLING = "9".repeat(40), HOST_BASE = "c".repeat(40), HOST_MERGE = "f".repeat(40); + const beside = `head ${REVIEWED.slice(0, 12)} merged into trunk ${TRUNK.slice(0, 12)} beside mesh-host#114@${OLD_SIBLING.slice(0, 8)}`; + state.statuses = [{ context: "mesh/merge-gate", state: "success", + description: `head ${REVIEWED.slice(0, 12)} merged into trunk ${TRUNK.slice(0, 12)} \u2014 pass: group g composed: ok` }, + { context: "mesh/repo-check", state: "success", description: `${beside} \u2014 pass: y` }]; + state.groupStatus = "success"; + state.siblingGroup = "success"; + // The member's own verdict named the sibling at its old head. + const record: Record = { "novox/mesh-catalog#7": { head: REVIEWED, + siblings: [{ repository: "novox/mesh-host", number: 114, head: OLD_SIBLING, base: "main", base_commit: HOST_BASE }] } }; + // The composed verdict then judged both new heads. + noteComposed(record, [{ owner: "novox", repo: "mesh-catalog", number: 7, commit: REVIEWED, trunk: TRUNK.slice(0, 12), base: "main" }, + { owner: "novox", repo: "mesh-host", number: 114, commit: SIBLING_HEAD, trunk: HOST_BASE.slice(0, 12), base: "main" }]); + // The sibling merges its head; its announcement weighs the member's record. The member's gate is written only by what + // that weighing decides, through the writer. + const pulled = { number: 114, title: "", html_url: "", merged: true, state: "closed", head_sha: SIBLING_HEAD, merge_commit_sha: HOST_MERGE }; + const forge = { + async getPullRequest(_o: string, repo: string, n: number) { + return repo === "mesh-host" ? pulled : { number: n, title: "", html_url: "", merged: false, state: "open", head_sha: REVIEWED }; + }, + async branchHead() { return HOST_MERGE; }, + async commitParents() { return [HOST_BASE, SIBLING_HEAD]; }, + }; + const wrote: string[] = []; + const gatesOut = { async outOfDate(_o: string, _r: string, sha: string, decide: (c: any) => any[]) { + const put = decide({ "mesh/merge-gate": { state: "success", description: state.statuses[0].description }, + "mesh/repo-check": { state: "success", description: "x" } }); + for (const s of put) { + wrote.push(`${sha.slice(0, 12)} ${s.context} ${s.state}`); + state.statuses = [{ ...s }, ...state.statuses.filter((x) => x.context !== s.context)]; + } + return put; + } }; + await weighSiblings(forge as any, gatesOut as any, record, "novox/mesh-host", 114); + assert.deepEqual(wrote, [], "the member's statuses were left alone"); + keepSiblings(siblingsFile(stateDir), record); + // The member merges on its first ask, with the one Approve's heads. + const done = (await tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED, + approved_heads: both })) as Record; + assert.equal(done.merged, true); + assert.equal(state.emitted.length, 0, "no check was asked"); +}); diff --git a/modules/gitea/test/siblings.test.ts b/modules/gitea/test/siblings.test.ts index 857b7640..92da1119 100644 --- a/modules/gitea/test/siblings.test.ts +++ b/modules/gitea/test/siblings.test.ts @@ -268,3 +268,58 @@ test("a merged or closed member's entry is dropped, so a sibling's later push ne assert.deepEqual(mine, {}); assert.deepEqual(own.written, []); }); + +// --- the composed verdict names the siblings of each member (issue 1049) -------------------------------------- + +// Issue 1049: the 2026-10-17 23:48 group (mesh-controller #375 beside mesh-catalog #353). #375's own verdict named #353 +// at its old head; #353 then got a new head, and the group's composed verdict passed for both new heads. The record still +// held the old head, so #353's own merge of the NEW head read as "merged otherwise than its head" and put #375's gate back +// to pending, and the group merged in two passes. +const composedMembers = () => [ + { owner: "novox", repo: "mesh-controller", number: 346, commit: MEMBER_HEAD, trunk: "111111111111", base: "main" }, + { owner: "novox", repo: "mesh-host", number: 114, commit: HOST_NEW, trunk: HOST_BASE.slice(0, 12), base: "main" }, +]; +const staleRecord = (): SiblingRecord => ({ "novox/mesh-controller#346": { head: MEMBER_HEAD, siblings: siblingsOf([sibling]) } }); + +test("without the composed verdict's siblings, a sibling's own merge of its new head is a move (the pin of issue 1049)", async () => { + const w = world(); + Object.assign(w.host, { merged: true, state: "closed", merge_commit_sha: MERGE, head_sha: HOST_NEW }); + w.parents = [HOST_BASE, HOST_NEW]; + await weigh(w, staleRecord()); + assert.equal(w.written.length, 2, "the record names the old head: the merge reads as another head's"); +}); + +test("a composed verdict names the siblings of each member at the heads and base commits it composed, so a sibling's own merge of its new head is no move (issue 1049)", async () => { + const { noteComposed } = await import("../dist/siblings.js"); + const record = staleRecord(); + assert.equal(noteComposed(record, composedMembers()), true); + assert.deepEqual(record["novox/mesh-controller#346"], { head: MEMBER_HEAD, + siblings: [{ repository: "novox/mesh-host", number: 114, head: HOST_NEW, base: "main", base_commit: HOST_BASE.slice(0, 12) }] }); + assert.deepEqual(record["novox/mesh-host#114"], { head: HOST_NEW, + siblings: [{ repository: "novox/mesh-controller", number: 346, head: MEMBER_HEAD, base: "main", base_commit: "111111111111" }] }); + const w = world(); + Object.assign(w.host, { merged: true, state: "closed", merge_commit_sha: MERGE, head_sha: HOST_NEW }); + w.parents = [HOST_BASE, HOST_NEW]; + await weigh(w, record); + assert.deepEqual(w.written, [], "its own merge of the head the composed check judged leaves the member's gate alone"); + assert.equal(record["novox/mesh-controller#346"].siblings[0].merged, true); + // A merge of any other head, or onto any other base, is still a move. + const other = world(); + const again: SiblingRecord = {}; + noteComposed(again, composedMembers()); + Object.assign(other.host, { merged: true, state: "closed", merge_commit_sha: MERGE, head_sha: HOST_NEW }); + other.parents = [HOST_MOVED, HOST_NEW]; + await weigh(other, again); + assert.equal(other.written.length, 2); +}); + +test("a composed verdict keeps nothing for a member it cannot name, and a group of one is none (issue 1049)", async () => { + const { noteComposed } = await import("../dist/siblings.js"); + const record: SiblingRecord = {}; + const [a, b] = composedMembers(); + assert.equal(noteComposed(record, [a]), false); + assert.equal(noteComposed(record, [a, { ...b, number: undefined }]), false, "a member with no pull request number"); + assert.equal(noteComposed(record, [a, { ...b, trunk: undefined }]), false, "a member with no trunk commit"); + assert.equal(noteComposed(record, [a, { ...b, base: undefined }]), false, "a member with no base branch"); + assert.deepEqual(record, {}); +}); diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index c40666e8..f48b1a76 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -428,6 +428,8 @@ type fakeForge struct { merges, comments []string approvedHeads [][]string refuse error + // refuseRepo refuses the merges of one repository only ("owner/name"), beside refuse (issue 1049). + refuseRepo map[string]error // calls are the comments in the order asked. calls []string } @@ -711,6 +713,9 @@ func (f *fakeForge) Merge(owner, repo string, number int, head string, approved defer f.mu.Unlock() f.merges = append(f.merges, fmt.Sprintf("%s/%s#%d@%s", owner, repo, number, head)) f.approvedHeads = append(f.approvedHeads, approved) + if err := f.refuseRepo[owner+"/"+repo]; err != nil { + return err + } return f.refuse } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index c9a34734..405c983f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -188,3 +188,34 @@ func TestAMemberWhoseGateIsTheComposedChecksCountsAsPassed(t *testing.T) { t.Fatalf("a member whose repository check failed is ready") } } + +// Issue 1049 (survey question 1, confirmed here): a composed check asked after one member has already merged is asked +// with ALL the members, the merged one at the head it merged, never with the unmerged one alone. The controller needs two +// or more members to compose (mesh-controller delivery.go askGroupCheck), and a merge of a contained head is a no-op +// there. +func TestAComposedCheckAskedAfterOneMemberMergedIsAskedWithAllMembers(t *testing.T) { + w := approvedGroup(t) + w.forge.refuseRepo = map[string]error{gCtl: errAskedAgain} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.h.PullMerged(merged(gCat, 2, gHk, gT1)) + w.settleAll() + if g := w.h.groups["feat/g"]; g == nil || !g.Closed { + t.Fatalf("the group did not close when a member merged: %+v", g) + } + // The unmerged member gets a new head and its own verdict: the group's composed check is due again. + const gHc2 = "e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3e3" + asked := len(w.ctl.checks) + w.h.PullUpdated(pr(gCtl, 1, gHc2, "feat/g", "internal/a.go")) + w.h.Checked(judged(gCtl, 1, gHc2, "pass", gT1)) + w.settleAll() + var composed []string + for _, c := range w.ctl.checks[asked:] { + if len(c) > 7 && c[:7] == "feat/g:" { + composed = append(composed, c) + } + } + if len(composed) != 1 || composed[0] != "feat/g:"+IDOf(gCat, gHk)+","+IDOf(gCtl, gHc2) { + t.Fatalf("the composed check was asked with %v; wanted once, with both members (the merged one at its head)", composed) + } +} From 983e4a64448b4838179a99e0ed79cd12da70fe98 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:09:37 +0200 Subject: [PATCH 2/8] A closed group's member whose merge was refused asks the composed check again at once, and a refusal loop past ten minutes is said (issue 1082) --- .../cmd/mesh-delivery/group_test.go | 66 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 21 ++++++ .../mesh-delivery/cmd/mesh-delivery/ready.go | 19 ++++++ 3 files changed, 106 insertions(+) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index 405c983f..faf97c2d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -3,6 +3,7 @@ package main import ( "encoding/json" "sort" + "strings" "testing" "time" ) @@ -219,3 +220,68 @@ func TestAComposedCheckAskedAfterOneMemberMergedIsAskedWithAllMembers(t *testing t.Fatalf("the composed check was asked with %v; wanted once, with both members (the merged one at its head)", composed) } } + +// Issue 1082 (the 2026-10-17 23:48 group): a closed group's member whose merge the tool refused after its sibling merged +// asks the group's composed check again at once, and merges on its pass with the one Approve (ADR 0418 §4, ADR 0421). +func TestAClosedGroupsMemberWhoseMergeWasRefusedAsksTheComposedCheckAgainAtOnce(t *testing.T) { + w := approvedGroup(t) + w.forge.refuseRepo = map[string]error{gCtl: errAskedAgain} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + // The catalogue member merged, the controller's was refused. + w.h.PullMerged(merged(gCat, 2, gHk, gT1)) + asked := len(w.ctl.checks) + w.settleAll() + var composed []string + for _, c := range w.ctl.checks[asked:] { + if strings.HasPrefix(c, "feat/g:") { + composed = append(composed, c) + } + } + if len(composed) != 1 || composed[0] != "feat/g:"+IDOf(gCat, gHk)+","+IDOf(gCtl, gHc) { + t.Fatalf("the refusal asked the composed check %v; wanted once, at once, with both members", composed) + } + // Not again while that check is on its way. + w.later(2 * time.Minute) + w.settleAll() + if got := w.ctl.checks[asked:]; len(got) != 1 { + t.Fatalf("asked again while the composed check was on its way: %v", got) + } + // It passes; the controller's member merges on the same Approve. + w.forge.refuseRepo = nil + g := w.h.groups["feat/g"] + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed again"}) + w.settleAll() + if d := w.delivery(IDOf(gCtl, gHc)); d.Ready.Merged == nil { + t.Fatalf("the refused member was not merged on the new composed verdict: %+v", d.Ready) + } + if w.op.count() != 1 { + t.Fatalf("a second Approve was asked: %d", w.op.count()) + } +} + +// Issue 1082: a refusal loop is not silent. A merge refused past ten minutes in a closed group (ADR 0418 §3) is a stalled +// line naming the member and the merge tool's last answer, which the controller raises as a condition. +func TestARefusalLoopInAClosedGroupIsSaidPastTenMinutes(t *testing.T) { + w := approvedGroup(t) + w.forge.refuseRepo = map[string]error{gCtl: errAskedAgain} + w.op.answer(w.op.open()[0], OptionApprove) + w.settleAll() + w.h.PullMerged(merged(gCat, 2, gHk, gT1)) + w.settleAll() + // Each composed pass is followed by a refusal again: the loop. + for range 12 { + w.later(time.Minute) + g := w.h.groups["feat/g"] + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed again"}) + w.settleAll() + if lines := w.h.Stalled(); len(lines) != 0 && w.now.Sub(*w.delivery(IDOf(gCtl, gHc)).Ready.RefusedSince) < 10*time.Minute { + t.Fatalf("said before its bound: %+v", lines) + } + } + lines := w.h.Stalled() + if len(lines) != 1 || lines[0].ID != IDOf(gCtl, gHc) || lines[0].Bound != "10m0s" || + !strings.Contains(lines[0].Says, errAskedAgain.Error()) || !strings.Contains(lines[0].Says, "group feat/g") { + t.Fatalf("the loop was not said, naming the member, its group and the tool's answer: %+v", lines) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 8b2ea3a1..a91fbee6 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -1847,6 +1847,16 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func( } } again := g.Check != nil && (!sameMembers(g.Check.Commits, commits) || g.Check.OutOfDate != "" && rejudgedSince(members, g.Check.OutOfDateAt)) + // A closed group's member whose merge the tool refused, after the composed check answered, is judged again at once + // (novox/hq ADR 0418 §4, issue 1082): the refusal is a sibling's merge having moved what the verdict stands on, and + // nothing else asks the composed check again. Not while a check is on its way (its verdict is empty then). + if !again && g.Closed && g.Check != nil && g.Check.Verdict != "" { + if refused := refusedSince(members, g.Check.At); refused != nil { + again = true + h.Logf("[mesh-delivery] group %s: the merge of %s was refused (%s), so its composed check is asked again", g.ID, + refused.ID, refused.Ready.Answer) + } + } if allReady && (g.Check == nil && !g.Closed || again) { ms := membersOfAsk(members) id := g.ID @@ -2426,6 +2436,17 @@ func containsString(xs []string, x string) bool { } // rejudgedSince is whether a member was judged again after a moment: a verdict of its own heard since. +// refusedSince is a member whose merge the tool refused, asked at or after the time given and not merged since: the first in +// the group's order, or nil. +func refusedSince(members []*Delivery, at time.Time) *Delivery { + for _, d := range members { + if r := d.Ready; r != nil && r.Merged == nil && d.MergedAs == "" && r.RefusedSince != nil && !r.LastTry.Before(at) { + return d + } + } + return nil +} + func rejudgedSince(members []*Delivery, at time.Time) bool { for _, d := range members { if d.Check != nil && d.Check.At.After(at) { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 19d92742..fb2de9f7 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -41,6 +41,16 @@ const mergeRetryAfter = time.Minute // check takes (the controller's checkTimeout is 45 minutes), so a check on its way is never said. const readyRefusedFor = time.Hour +// groupRefusedFor is how long the merge tool may refuse a closed group's member before it is said as stalled: the merge +// window of ADR 0418 §3, not the hour of a lone head, because the group's other members are already on the trunk. +const groupRefusedFor = 10 * time.Minute + +// groupClosed is whether a group is closed: a member reached the trunk. Under the lock. +func (h *Holder) groupClosed(id string) bool { + g := h.groups[id] + return g != nil && g.Closed +} + // ReadyRun is what this owner did with a delivery's Approve and merge: kept on the delivery and shown whole by `show`. type ReadyRun struct { // Head is the head the run is for. @@ -407,6 +417,15 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { Bound: "0s", H2: "none: a new head, which asks the operator again — the operator's", Says: fmt.Sprintf("%s#%d at %s is not merged: the operator's answer %s", d.Repository, d.Number, shortOf(r.Head, 12), r.Refused)}) + case r.RefusedSince != nil && d.Group != "" && h.groupClosed(d.Group) && now.Sub(*r.RefusedSince) >= groupRefusedFor: + // A closed group's member (novox/hq ADR 0418 §3, issue 1082): its sibling merged, and its own merge is refused + // again after each composed check. Said after the merge window, with the group and the tool's last answer. + out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, + For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: groupRefusedFor.String(), + H2: "none: the group's composed check is asked again after each refusal, and the merge each minute — the operator's", + Says: fmt.Sprintf("%s#%d is approved at %s with its group %s closed (a sibling merged), and the merge tool has "+ + "refused it since %s: %s", d.Repository, d.Number, shortOf(r.Head, 12), d.Group, + r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) case r.RefusedSince != nil && now.Sub(*r.RefusedSince) >= readyRefusedFor: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: readyRefusedFor.String(), From 814c1a991c4609c46afb22e377881e42a318e90f Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:10:12 +0200 Subject: [PATCH 3/8] A trunk move asks at once the check of a group member whose Approve is not asked yet (issue 1047) --- .../cmd/mesh-delivery/group_test.go | 32 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 4 ++- .../mesh-delivery/cmd/mesh-delivery/ready.go | 5 ++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index faf97c2d..35a91e24 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -285,3 +285,35 @@ func TestARefusalLoopInAClosedGroupIsSaidPastTenMinutes(t *testing.T) { t.Fatalf("the loop was not said, naming the member, its group and the tool's answer: %+v", lines) } } + +// Issue 1047 (the live case of 2026-10-17 23:34, ADR 0395 §1, design 47): a group member that is ready when a third party's +// merge moves its trunk, with no Approve asked yet (the composed check has not passed), has its own check asked at once, +// and the Approve is asked once the group's composed check has passed on it. +func TestATrunkMoveAsksTheCheckOfAGroupMemberWhoseApproveIsNotAskedYet(t *testing.T) { + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr(gCtl, 1, gHc, "feat/g", "internal/a.go")) + w.h.PullUpdated(pr(gCat, 2, gHk, "feat/g", "modules/app/module.json")) + w.h.Checked(judged(gCtl, 1, gHc, "pass", gTrunk0)) + w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0)) + w.settleAll() + if w.op.count() != 0 { + t.Fatalf("the composed check has not passed, yet the operator was asked: %d", w.op.count()) + } + asked := len(w.ctl.checks) + w.h.PullMerged(merged(gCat, 5, "5555555555555555", gT1)) + w.settleAll() + got := w.ctl.checks[asked:] + if len(got) != 1 || got[0] != ":"+IDOf(gCat, gHk) { + t.Fatalf("the trunk's move asked %v; wanted the moved member's own check once", got) + } + if last := w.ctl.baseSHAs[len(w.ctl.baseSHAs)-1]; last != gT1[:12] && last != gT1 { + t.Fatalf("its check was not asked against the trunk's head: %v", w.ctl.baseSHAs) + } + // Not asked again at the next look while it is on its way. + w.later(time.Second) + w.settleAll() + if len(w.ctl.checks[asked:]) != 1 { + t.Fatalf("asked twice: %v", w.ctl.checks[asked:]) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index a91fbee6..4e18c0a2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -616,7 +616,9 @@ func (h *Holder) PullMerged(p PullEvent) { // trunkMoved takes a merge into a trunk as that trunk's move (novox/hq ADR 0323 §7): every other open delivery into // it keeps the trunk's new head, and a ready one whose verdict names the commit before goes back to proposed. A // group's composed check goes out of date when the merge is not one of its members and a member still open merges -// into that trunk (§6); the group then reads checking. Nothing is asked: a recheck or the merge tool judges again. +// into that trunk (§6); the group then reads checking. Nothing is asked here: at the same look askFresh asks the check of +// a group's member, or of a head with an Approve asked or given (issue 1047); a lone head's is judged again by a recheck +// or the merge tool (the rest of ADR 0395 §1 is issue 888). func (h *Holder) trunkMoved(merged *Delivery, base, commit string, now time.Time) { if base == "" || commit == "" { return diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index fb2de9f7..4c7449d3 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -245,7 +245,10 @@ func (h *Holder) askFresh(d *Delivery, now time.Time) func() { d.Number == 0 || d.TrunkNow == "" { return nil } - if !d.FreshWanted && h.covering(d) == nil { + // A group's member too (issue 1047, design 47: a move of the trunk asks every check again): the group's one Approve is + // asked after its composed check, so a member ready before that has no Approve to carry; its check is asked at once + // all the same, and the composed check follows on the new verdict. The rest of ADR 0395 §1 stays with issue 888. + if !d.FreshWanted && h.covering(d) == nil && h.groups[d.Group] == nil { return nil } if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { From 93365be98f3fb06197e1903878247963571d01ad Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:10:51 +0200 Subject: [PATCH 4/8] A new pull request at a commit whose delivery was stopped and unmerged takes that delivery over (issue 1083) --- .../mesh-delivery/cmd/mesh-delivery/holder.go | 33 +++++-- .../cmd/mesh-delivery/issue1083_test.go | 89 +++++++++++++++++++ 2 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 4e18c0a2..749fbabe 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -359,6 +359,12 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { // another pull request with the same head (novox/hq issue 783): started over below, exactly as for a first // announcement, under the same id — never a transition out of Stopped. } + if startedOver != "" { + // The record the new pull request takes over leaves the group it was in (issue 1083): the new one joins its own. + if old := h.deliveries[id]; old != nil && old.Group != "" { + h.leaveGroup(old) + } + } replaced := "" for _, old := range h.deliveries { if old.Repository == p.repository() && old.Number == p.Number && !sameCommit(old.Commit, p.HeadSHA) && @@ -391,14 +397,28 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { } // 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: only when it was stopped solely by an earlier, closed duplicate pull request with -// the same head — still Stopped, ClosedUnmerged, never merged, and under another number (novox/hq issue 783). -// Empty for the ordinary 'announced again' path, or for any other stop (a person's, a group's, its own close). +// 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 +// closed (issue 1083: the new pull request at that commit takes the record over, the key staying repository@commit). +// Empty for the ordinary 'announced again' path, or for the same pull request's own stop. func startedOverFor(d *Delivery, number int) string { - if d != nil && d.State == Stopped && d.ClosedUnmerged && d.MergedAs == "" && d.Number != number { + if d == nil || d.State != Stopped || d.MergedAs != "" || d.Number == number { + return "" + } + if d.ClosedUnmerged { return fmt.Sprintf("#%d with the same head closed unmerged", d.Number) } - return "" + said := fmt.Sprintf("#%d with the same head was stopped", d.Number) + if n := len(d.Transitions); n > 0 && d.Transitions[n-1].To == Stopped { + last := d.Transitions[n-1] + if last.By != "" { + said += " by " + last.By + } + if last.Why != "" { + said += ": " + last.Why + } + } + return said } // freshAnnouncement makes a delivery for p's head exactly as a first announcement would: its own id, proposed. @@ -553,6 +573,9 @@ func (h *Holder) PullMerged(p PullEvent) { // merge in every field but its id: FromForge, and its check the forge's word on the head, so a long-past // merge replayed from the bus's history still falls under the history rule (issue 313) and is not held for // a person (review round 1). + if d.Group != "" { + h.leaveGroup(d) // issue 1083: the record taken over leaves its group + } fresh, t, err := h.freshAnnouncement(p, now, over) if err != nil { h.refuse(err) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go new file mode 100644 index 00000000..194f7146 --- /dev/null +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue1083_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "strings" + "testing" +) + +// novox/hq issue 1083: mesh-controller #375's delivery was stopped by a person, then #375 was closed and the same commit +// opened as #376 from another branch. The delivery is named repository@commit; it was Stopped before its pull request +// closed, so it was not ClosedUnmerged, and #376 got no delivery of its own: its Approve was never asked. A new pull +// request at a commit whose delivery is Stopped and unmerged, under another number, takes that record over (the key stays +// repository@commit); the take-over is said in its transitions, with who stopped it and why. + +const ( + head1083 = "e375e375e375e375" + branch1083 = "feat/backup-before-send" +) + +func TestAPullRequestAtACommitWhoseDeliveryAPersonStoppedTakesItOver(t *testing.T) { + w := newWorld(t) + id := IDOf(catalog783, head1083) + + w.h.PullUpdated(pr(catalog783, 375, head1083, branch1083, "src/a.go")) + w.h.Checked(verdict(catalog783, 375, head1083, "pass")) + if w.state(id) != Ready { + t.Fatalf("its passing verdict left it %s", w.state(id)) + } + if _, err := w.h.Stop(id, "to break the loop", "jochen"); err != nil { + t.Fatal(err) + } + // The person stopped it first; the pull request closed afterwards: a final state is not moved by the close. + w.h.PullClosed(pr(catalog783, 375, head1083, branch1083)) + if d := w.delivery(id); d.State != Stopped || d.ClosedUnmerged { + t.Fatalf("setup: %s, closed unmerged %v; wanted stopped by the person alone", d.State, d.ClosedUnmerged) + } + + w.h.PullUpdated(pr(catalog783, 376, head1083, branch1083+"-2", "src/a.go")) + d := w.delivery(id) + if d.State != Proposed || d.Number != 376 { + t.Fatalf("#376 at the same commit left it %s, Number %d; wanted proposed under #376", d.State, d.Number) + } + why := d.Transitions[len(d.Transitions)-1].Why + if !strings.Contains(why, "#375 with the same head was stopped by jochen: to break the loop") { + t.Fatalf("the take-over is not said, with who stopped it and why: %q", why) + } + w.h.Checked(verdict(catalog783, 376, head1083, "pass")) + if w.state(id) != Ready { + t.Fatalf("a passing verdict for #376 left it %s", w.state(id)) + } +} + +// A stopped record that was merged is not taken over, and the same pull request announced again stays stopped. +func TestAStoppedRecordThatWasMergedIsNotTakenOverByAnotherPullRequest(t *testing.T) { + w := newWorld(t) + id := IDOf(catalog783, head783c+"1") + w.h.PullUpdated(pr(catalog783, 400, head783c+"1", branch1083, "src/a.go")) + w.h.PullMerged(merged(catalog783, 400, head783c+"1", "f400f400f400f400")) + w.h.PullUpdated(pr(catalog783, 401, head783c+"1", branch1083+"-2", "src/a.go")) + if d := w.delivery(id); d.Number != 400 || d.MergedAs == "" { + t.Fatalf("a merged record was taken over: Number %d, merged as %q", d.Number, d.MergedAs) + } +} + +// The take-over leaves the stopped record's group: the new record joins its own group by its branch. +func TestATakenOverRecordLeavesTheGroupItWasStoppedIn(t *testing.T) { + w := newWorld(t) + w.h.PullUpdated(pr(gCtl, 1, gHc, "feat/g", "internal/a.go")) + w.h.PullUpdated(pr(gCat, 2, gHk, "feat/g", "modules/app/module.json")) + id := IDOf(gCtl, gHc) + if _, err := w.h.Stop(id, "no", "jochen"); err != nil { + t.Fatal(err) + } + w.h.PullClosed(pr(gCtl, 1, gHc, "feat/g")) + w.h.PullUpdated(pr(gCtl, 9, gHc, "feat/other", "internal/a.go")) + w.h.mu.Lock() + defer w.h.mu.Unlock() + for _, g := range w.h.groups { + if g.ID == "feat/g" { + for _, m := range g.Members { + if m == id { + t.Fatalf("group feat/g still lists %s, now #9 on feat/other: %v", id, g.Members) + } + } + } + } + if d := w.h.deliveries[id]; d.Number != 9 || d.State != Proposed || d.Group == "feat/g" { + t.Fatalf("taken over: Number %d, %s, group %q", d.Number, d.State, d.Group) + } +} From 6556e614850dd7b9e5286817ca41c5866aa840e2 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:12:40 +0200 Subject: [PATCH 5/8] Read a pull request's review record again when its description is edited, and ask the Approve again if it was unanswered (issue 1079) --- modules/gitea/test/webhook.test.ts | 17 +++ modules/gitea/webhook.ts | 4 +- .../cmd/mesh-delivery/approval.go | 21 ++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 32 ++++++ .../cmd/mesh-delivery/issue1079_test.go | 101 ++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/main.go | 10 +- modules/mesh-delivery/module.json | 1 + 7 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 modules/mesh-delivery/cmd/mesh-delivery/issue1079_test.go 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" ], From e8a74765f8a6f1815b86fbdf9ea47cf6a8453f60 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:18:13 +0200 Subject: [PATCH 6/8] A composed check that lands judged against a trunk that has moved is out of date and asked again (issue 1047, review round 1) --- .../cmd/mesh-delivery/group_test.go | 41 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 13 ++++++ 2 files changed, 54 insertions(+) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index 35a91e24..faa9dbf4 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -316,4 +316,45 @@ func TestATrunkMoveAsksTheCheckOfAGroupMemberWhoseApproveIsNotAskedYet(t *testin if len(w.ctl.checks[asked:]) != 1 { t.Fatalf("asked twice: %v", w.ctl.checks[asked:]) } + // The composed check that was on its way when the trunk moved answers, judged against the old trunk (ADR 0403 §6): it is + // out of date as it lands, so the group does not become ready on it and the operator is not asked. + g := w.h.groups["feat/g"] + w.later(time.Minute) + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed on the old trunk", + Members: []struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Trunk string `json:"trunk,omitempty"` + }{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]}, + {Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gTrunk0[:12]}}}) + w.settleAll() + if w.op.count() != 0 { + t.Fatalf("the operator was asked on a composed pass judged against the old trunk: %d", w.op.count()) + } + // The moved member's fresh verdict arrives: the composed check is asked again, and only its pass asks the operator. + w.later(time.Minute) + composedBefore := len(w.ctl.checks) + w.h.Checked(judged(gCat, 2, gHk, "pass", gT1)) + w.settleAll() + last := w.ctl.checks[len(w.ctl.checks)-1] + if len(w.ctl.checks) == composedBefore || !strings.HasPrefix(last, "feat/g:") { + t.Fatalf("the composed check was not asked again on the fresh verdict: %v", w.ctl.checks[composedBefore:]) + } + if w.op.count() != 0 { + t.Fatalf("the operator was asked before the new composed check passed: %d", w.op.count()) + } + g = w.h.groups["feat/g"] + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed on the new trunk", + Members: []struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Trunk string `json:"trunk,omitempty"` + }{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]}, + {Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gT1[:12]}}}) + w.settleAll() + if w.op.count() != 1 { + t.Fatalf("the operator is asked once, on the new pass: %d", w.op.count()) + } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 386d2729..77f7013b 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -829,6 +829,19 @@ func (h *Holder) Checked(c CheckedEvent) { } } } + // A composed check that was on its way when a trunk moved lands judged against the old trunk: out of date as it lands + // (novox/hq ADR 0403 §6, issue 1047), and asked again once a member is judged again. + if g.Check.OutOfDate == "" { + for id, judged := range g.Check.Trunks { + if d := h.deliveries[id]; d != nil && d.MergedAs == "" && !d.State.Final() && d.TrunkNow != "" && !sameCommit(judged, d.TrunkNow) { + g.Check.OutOfDate = fmt.Sprintf("composed check out of date: %s was judged against trunk %s, the trunk is now %s", + id, shortOf(judged, 12), shortOf(d.TrunkNow, 12)) + g.Check.OutOfDateAt = now + h.Logf("[mesh-delivery] group %s: %s", g.ID, g.Check.OutOfDate) + break + } + } + } g.Updated = now h.keepGroup(g) h.Logf("[mesh-delivery] group %s composed: %s — %s", g.ID, c.Verdict, c.Summary) From ebd29223f37928f0b93fc7ea0e9c38790878664b Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:18:43 +0200 Subject: [PATCH 7/8] Prove over a real bus that an edit of the description asks the Approve again (issue 1079, review round 1) --- .../cmd/mesh-delivery/bus_test.go | 40 +++++++++++++++++-- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go index 34c5b329..87d37458 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go @@ -107,7 +107,9 @@ type runtime struct { // the asks the operator was asked, and the controller's walks and the forge's open pull requests as the test plays // them. operatorAsks []asks.Ask - openPulls []OpenPull + // operatorCancels are the ids of the asks taken back from the router, in order (issue 1079). + operatorCancels []string + openPulls []OpenPull } type askedOfMesh struct { @@ -268,6 +270,17 @@ func (r *runtime) answer(m rpcMessage) { r.operatorAsks = append(r.operatorAsks, a) r.mu.Unlock() } + if strings.Contains(p.Subject, ".accept.cancel.") { + var c struct { + ID string `json:"id"` + } + if err := json.Unmarshal(p.Body, &c); err != nil { + return nil, err + } + r.mu.Lock() + r.operatorCancels = append(r.operatorCancels, c.ID) + r.mu.Unlock() + } return map[string]any{}, nil case "mesh/seat.record": return nil, nil @@ -416,7 +429,7 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB announced := pr("novox/app", 7, head, "fix/x", "src/a.go") announced.BaseSHA = "t1t1t1t1t1t1" announced.Title = "Fix the apostrophe (issue 975)" - announced.Body = "Reviewed-by: lead@shanks at " + head + announced.Body = "The description, with no review line yet." b.publish(t, "mesh.mod.gitea.event.pull.updated", announced) passed := b.publish(t, "mesh.seat."+ControllerSeat+".event.checked", judgedAt("novox/app", 7, head, "pass", "t1t1t1t1t1t1")) @@ -430,10 +443,29 @@ func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealB ask := rt.operatorAsks[0] asked := time.Now() rt.mu.Unlock() - if !strings.Contains(ask.Explanation, "reviewed by lead") { - t.Fatalf("the ask does not name the reviewer: %s", ask.Explanation) + if !strings.Contains(ask.Explanation, "not reviewed at this head") { + t.Fatalf("the first ask does not say the record is missing: %s", ask.Explanation) } t.Logf("the operator was asked %s after the verdict's event", asked.Sub(passed)) + // The description is edited to add the review record (issue 1079): the forge module's edit event carries it. The unanswered + // ask is withdrawn from the router and asked again, once, with the record read; nothing else is asked of the mesh. + first := ask + 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}) + rt.until("the first ask to be withdrawn and the operator asked again", 5*time.Second, func() bool { + rt.mu.Lock() + defer rt.mu.Unlock() + return len(rt.operatorCancels) >= 1 && len(rt.operatorAsks) >= 2 + }) + rt.mu.Lock() + if len(rt.operatorCancels) != 1 || rt.operatorCancels[0] != first.ID || len(rt.operatorAsks) != 2 { + t.Fatalf("withdrawn %v, asked %d; wanted the first ask %s withdrawn once and exactly one new ask", rt.operatorCancels, len(rt.operatorAsks), first.ID) + } + ask = rt.operatorAsks[1] + rt.mu.Unlock() + if ask.ID == first.ID || !strings.Contains(ask.Explanation, "reviewed by lead on shanks") || strings.Contains(ask.Explanation, "not reviewed") { + t.Fatalf("the new ask is not a fresh one naming the reviewer: %s %s", ask.ID, ask.Explanation) + } if len(rt.asksOf("gitea.gitea_merge_pull_request")) != 0 { t.Fatalf("merged before the Approve") } From 7383bc31703eca05b6663e351f844c6568da2d18 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:20:18 +0200 Subject: [PATCH 8/8] A stale composed pass counts members judged since it was asked, whichever verdict lands first (issue 1047, review round 2) --- .../cmd/mesh-delivery/group_test.go | 37 +++++++++++++++++++ .../mesh-delivery/cmd/mesh-delivery/holder.go | 4 +- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go index faa9dbf4..88967366 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/group_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/group_test.go @@ -358,3 +358,40 @@ func TestATrunkMoveAsksTheCheckOfAGroupMemberWhoseApproveIsNotAskedYet(t *testin t.Fatalf("the operator is asked once, on the new pass: %d", w.op.count()) } } + +// Issue 1047 (review round 2): the other order. The moved member's fresh verdict lands before the stale composed pass (one +// build against two): the pass is out of date as it lands, the composed check is asked again, and the operator is asked +// only after that check passes. +func TestAStaleComposedPassLandingAfterTheMovedMembersFreshVerdictIsAskedAgain(t *testing.T) { + w := newWorld(t) + w.op.auto = false + w.h.PullUpdated(pr(gCtl, 1, gHc, "feat/g", "internal/a.go")) + w.h.PullUpdated(pr(gCat, 2, gHk, "feat/g", "modules/app/module.json")) + w.h.Checked(judged(gCtl, 1, gHc, "pass", gTrunk0)) + w.h.Checked(judged(gCat, 2, gHk, "pass", gTrunk0)) + w.settleAll() + g := w.h.groups["feat/g"] + w.later(time.Minute) + w.h.PullMerged(merged(gCat, 5, "5555555555555555", gT1)) + w.settleAll() + w.later(time.Minute) + w.h.Checked(judged(gCat, 2, gHk, "pass", gT1)) // the fresh verdict first + w.settleAll() + w.later(time.Minute) + before := len(w.ctl.checks) + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed on the old trunk", + Members: []struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + Commit string `json:"commit"` + Trunk string `json:"trunk,omitempty"` + }{{Owner: "novox", Repo: "mesh-controller", Commit: gHc, Trunk: gTrunk0[:12]}, + {Owner: "novox", Repo: "mesh-catalog", Commit: gHk, Trunk: gTrunk0[:12]}}}) + w.settleAll() + if len(w.ctl.checks) == before || !strings.HasPrefix(w.ctl.checks[len(w.ctl.checks)-1], "feat/g:") { + t.Fatalf("the composed check was not asked again after the stale pass: %v", w.ctl.checks[before:]) + } + if w.op.count() != 0 { + t.Fatalf("the operator was asked before the new composed check passed: %d", w.op.count()) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 77f7013b..b8fa3fcf 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -836,7 +836,9 @@ func (h *Holder) Checked(c CheckedEvent) { if d := h.deliveries[id]; d != nil && d.MergedAs == "" && !d.State.Final() && d.TrunkNow != "" && !sameCommit(judged, d.TrunkNow) { g.Check.OutOfDate = fmt.Sprintf("composed check out of date: %s was judged against trunk %s, the trunk is now %s", id, shortOf(judged, 12), shortOf(d.TrunkNow, 12)) - g.Check.OutOfDateAt = now + // From when the composed check was asked: a member judged since then, even before this verdict landed, + // counts as judged again. + g.Check.OutOfDateAt = g.Check.At h.Logf("[mesh-delivery] group %s: %s", g.ID, g.Check.OutOfDate) break }