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) + } +}