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)
This commit is contained in:
+12
-1
@@ -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<void> {
|
||||
// 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<string, string>();
|
||||
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<void> {
|
||||
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<void> {
|
||||
`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,
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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<string, unknown>).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<string, any> = { "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<string, unknown>;
|
||||
assert.equal(done.merged, true);
|
||||
assert.equal(state.emitted.length, 0, "no check was asked");
|
||||
});
|
||||
|
||||
@@ -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, {});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user