Ask the Approve as soon as the checks pass, with no ready mark (issue 1022)

The review finishes before the push and its record is a Reviewed-by line in the
description, so the mark repeated what the push meant. mesh-delivery asks the
operator when a pull request's checks pass, keeps an ask through a trunk move
by asking the check again at once, and hands the merge tool the approved heads,
which replace the forge review of each sibling.
This commit is contained in:
2026-10-17 20:44:54 +02:00
parent 5e7567e5a0
commit 668444785d
35 changed files with 795 additions and 1273 deletions
+10 -84
View File
@@ -145,16 +145,11 @@ test("a merged pull request whose branch was deleted is announced with its branc
assert.equal(headBranchOf(undefined), undefined);
});
// **A pull request marked ready says the head it is marked at** (novox/hq issue 722): the label mesh/ready and the
// newest comment "ready at <full head sha>" by a listed account, written after the label was added and after the last
// push. Read from the pull request's whole timeline, page by page; without the label nothing is read.
// **An open pull request is read from the forge with no ready mark** (novox/hq ADR 0421 §4, issue 1022): its head, base,
// required contexts and statuses, and no timeline.
const head = "c".repeat(40), older = "d".repeat(40);
const ev = (id: number, type: string, body: string, user = "mesh-admin", label?: { id: number; name: string }) =>
({ id, type, body, user: { login: user }, created_at: `2026-10-14T10:${String(id % 60).padStart(2, "0")}:00Z`,
...(label ? { label } : {}) });
const readyLabel = { id: 31, name: "mesh/ready" };
function forgeWith(timeline: unknown[]) {
function forgeWith() {
const asked: string[] = [];
const server = createServer((req, res) => {
const url = req.url ?? "";
@@ -162,8 +157,7 @@ function forgeWith(timeline: unknown[]) {
const send = (v: unknown) => res.end(JSON.stringify(v));
if (url.startsWith("/api/v1/repos/issues/search")) {
return send(url.includes("page=1") ? [
{ number: 7, repository: { owner: "novox", name: "hq", full_name: "novox/hq" },
labels: [{ id: 31, name: "mesh/ready" }, { id: 2, name: "bug" }] },
{ number: 7, repository: { owner: "novox", name: "hq", full_name: "novox/hq" }, labels: [{ id: 2, name: "bug" }] },
{ number: 8, repository: { owner: "novox", name: "hq", full_name: "novox/hq" }, labels: [] },
] : []);
}
@@ -173,11 +167,6 @@ function forgeWith(timeline: unknown[]) {
if (url.startsWith("/api/v1/repos/novox/hq/pulls/8")) {
return send({ number: 8, state: "open", base: { ref: "main" }, head: { ref: "y", sha: older }, html_url: "h" });
}
const t = /\/issues\/7\/timeline\?page=(\d+)&limit=50/.exec(url);
if (t) {
const page = Number(t[1]);
return send(timeline.slice((page - 1) * 50, page * 50));
}
if (url.includes("/branch_protections")) return send([]);
if (url.includes("/status")) return send({ statuses: [] });
res.statusCode = 404;
@@ -186,80 +175,17 @@ function forgeWith(timeline: unknown[]) {
return { server, asked };
}
async function readOpen(timeline: unknown[], authors = ["mesh-admin", "jschoubben"]) {
test("an open pull request is read with its head and statuses, and no ready mark nor timeline", async () => {
const { GiteaClient } = await import("../client.ts");
const { server, asked } = forgeWith(timeline);
const { server, asked } = forgeWith();
await new Promise<void>((r) => server.listen(0, r));
try {
const client = new GiteaClient(`http://127.0.0.1:${(server.address() as any).port}`, "t");
return { open: await client.openPulls(authors), asked };
const open = await client.openPulls();
assert.deepEqual(open.map((p) => [p.number, p.head_sha]), [[7, head], [8, older]]);
assert.equal((open[0] as any).ready, undefined);
assert.ok(!asked.some((u) => u.includes("/timeline")), "no timeline is read");
} finally {
server.close();
}
}
test("a pull request's ready mark is read: the label and the head the newest listed ready comment names", async () => {
const { open, asked } = await readOpen([
ev(1, "pull_push", JSON.stringify({ commit_ids: [head] })),
ev(2, "label", "1", "mesh-admin", readyLabel),
ev(3, "comment", `ready at ${older}`),
ev(4, "comment", `Reviewed: approve.\nready at ${head}\n`),
ev(5, "comment", "thanks", "jschoubben"),
]);
assert.deepEqual(open[0].labels, ["mesh/ready", "bug"]);
assert.deepEqual(open[0].ready, { head, by: "mesh-admin", at: "2026-10-14T10:04:00Z", label_id: 31 });
assert.equal(open[1].ready, undefined, "no label, no mark");
assert.ok(!asked.some((u) => u.includes("/issues/8/")), "no timeline read without the label");
});
test("a ready comment by an account not listed is ignored, and said", async () => {
const { readyMarkOf } = await import("../client.ts");
const said: string[] = [];
const mark = readyMarkOf(31, [
ev(1, "pull_push", "{}") as any, ev(2, "label", "1", "mesh-admin", readyLabel) as any,
{ ...ev(3, "comment", `ready at ${head}`, "outsider"), user: "outsider" },
].map((e: any) => ({ ...e, user: typeof e.user === "string" ? e.user : e.user.login })), ["mesh-admin"], (l) => said.push(l));
assert.equal(mark.head, "", "an outsider's mark counts");
assert.equal(said.length, 1);
assert.match(said[0], /by outsider is ignored/);
});
test("a ready comment older than the latest push, or than the label, never counts", async () => {
const pushedAfter = await readOpen([
ev(1, "label", "1", "mesh-admin", readyLabel),
ev(2, "comment", `ready at ${head}`),
ev(3, "pull_push", JSON.stringify({ commit_ids: [head] })),
]);
assert.equal(pushedAfter.open[0].ready?.head, "", "a mark written before the latest push counts, the label still on");
const beforeLabel = await readOpen([
ev(1, "pull_push", "{}"),
ev(2, "comment", `ready at ${head}`),
ev(3, "label", "1", "mesh-admin", readyLabel),
]);
assert.equal(beforeLabel.open[0].ready?.head, "", "a mark written before the label was added counts");
const takenOff = await readOpen([
ev(1, "label", "1", "mesh-admin", readyLabel),
ev(2, "comment", `ready at ${head}`),
ev(3, "label", "", "mesh-admin", readyLabel),
]);
assert.equal(takenOff.open[0].ready?.head, "", "a mark whose label was taken off since counts");
});
test("a long thread hides nothing: every page of the timeline is read", async () => {
const events: unknown[] = [ev(1, "pull_push", "{}"), ev(2, "label", "1", "mesh-admin", readyLabel),
ev(3, "comment", `ready at ${older}`)];
for (let id = 4; id < 130; id++) events.push(ev(id, "comment", "a long discussion", "jschoubben"));
events.push(ev(130, "comment", `ready at ${head}`, "jschoubben"));
const { open, asked } = await readOpen(events);
assert.equal(open[0].ready?.head, head, "the newest mark, on the third page, is not read");
assert.equal(asked.filter((u) => u.includes("/issues/7/timeline")).length, 3);
});
test("the accounts that may mark are the setting's, and only the mesh's own when it cannot be read", async () => {
const { readyAuthors } = await import("../client.ts");
assert.deepEqual(readyAuthors("f", "mesh-admin", () => "# x\nauthors=mesh-admin jschoubben\n"), ["mesh-admin", "jschoubben"]);
// The tools' account and the admin's, whose marks were made before ADR 0365 (issue 469).
assert.deepEqual(readyAuthors("f", "mesh-admin", () => { throw new Error("gone"); }, "mesh-tools"), ["mesh-tools", "mesh-admin"]);
assert.deepEqual(readyAuthors("f", "mesh-admin", () => "authors=\n", "mesh-tools"), ["mesh-tools", "mesh-admin"]);
assert.deepEqual(readyAuthors(undefined, "mesh-admin", undefined, "mesh-tools"), ["mesh-tools", "mesh-admin"]);
});
+32 -27
View File
@@ -566,12 +566,16 @@ test("no tool's caller text becomes another path or another user: `..` and `?sud
});
// **A member judged beside its siblings merges only with its group passed and reviewed** (novox/hq ADR 0382 §6, issue 867):
// the verdict names the sibling; mesh/delivery-group must be a success at the member's head and the sibling's head that
// verdict read, and the sibling's head must carry an approving review. A verdict that read none merges as before.
test("gitea_merge_pull_request merges a verdict that read siblings only with its group passed and each sibling head reviewed", async () => {
// **A member judged beside its siblings merges only with its group passed and the Approve covering each head** (novox/hq
// ADR 0382 §6 as ADR 0421 §9 changes it, issues 867 and 1022): the verdict names the sibling; mesh/delivery-group must
// be a success at the member's head and the sibling's head that verdict read, and mesh-delivery hands the heads the
// operator's Approve covers: every head read must be among them. No forge review is asked. A verdict that read none
// merges as before.
test("gitea_merge_pull_request merges a verdict that read siblings only with its group passed and each head approved", async () => {
const { keepSiblings, siblingsFile } = await import("../dist/siblings.js");
const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED });
const both = ["novox/mesh-catalog@" + REVIEWED, "novox/mesh-host@" + SIBLING_HEAD];
const merge = (approved_heads?: string[]) => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7,
head: REVIEWED, ...(approved_heads ? { approved_heads } : {}) });
const beside = `head ${REVIEWED.slice(0, 12)} merged into trunk ${TRUNK.slice(0, 12)} beside mesh-host#114@${SIBLING_HEAD.slice(0, 8)}`;
const named = (siblings: unknown[]) => keepSiblings(siblingsFile(stateDir),
{ "novox/mesh-catalog#7": { head: REVIEWED, siblings: siblings as any } });
@@ -579,31 +583,31 @@ test("gitea_merge_pull_request merges a verdict that read siblings only with its
{ context: "mesh/repo-check", state: "success", description: `${beside} — pass: y` }];
named([{ repository: "novox/mesh-host", number: 114, head: SIBLING_HEAD, base: "main", base_commit: "c".repeat(40) }]);
// Nothing passed: every missing thing is named, nothing merged.
// Nothing passed and no approved heads handed: every missing thing is named, nothing merged.
await assert.rejects(merge(), (err: Error) => {
assert.match(err.message, /is not merged: its verdict was judged beside 1 sibling\(s\), and mesh\/delivery-group is missing at mesh-catalog#7@abc1234def56; mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb carries no approving review at that head/);
assert.match(err.message, /is not merged: its verdict was judged beside 1 sibling\(s\), and mesh\/delivery-group is missing at mesh-catalog#7@abc1234def56; mesh-catalog#7@abc1234def56 is covered by no Approve: the merge was handed no approved heads; mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb is covered by no Approve/);
return true;
});
assert.equal(state.merged, false);
// The group passed at the member's head only: the sibling's head still lacks it.
state.groupStatus = "success";
await assert.rejects(merge(), /mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb carries no approving review/);
// Passed at both heads, but the only approval is at another head, or dismissed: not an approval at the head read.
await assert.rejects(merge(both), /mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb\. A member judged beside/);
// Passed at both heads, but the Approve covers the sibling at another head, or does not name it: not an approved head.
state.siblingGroup = "success";
state.reviews = [{ state: "APPROVED", commit_id: "d".repeat(40) }, { state: "APPROVED", commit_id: SIBLING_HEAD, dismissed: true },
{ state: "COMMENT", commit_id: SIBLING_HEAD }];
await assert.rejects(merge(), /judged beside 1 sibling\(s\), and mesh-host#114@bbbbbbbbbbbb carries no approving review at that head\. A member judged beside/);
await assert.rejects(merge(["novox/mesh-catalog@" + REVIEWED, "novox/mesh-host@" + "d".repeat(40)]),
/mesh-host#114@bbbbbbbbbbbb is not the head the Approve covers \(dddddddddddd\)\. A member judged beside/);
await assert.rejects(merge(["novox/mesh-catalog@" + REVIEWED]), /mesh-host#114@bbbbbbbbbbbb is not among the heads the Approve covers/);
assert.equal(state.merged, false);
// Passed and approved at the very heads read: it merges.
state.reviews.push({ state: "APPROVED", commit_id: SIBLING_HEAD });
assert.equal(((await merge()) as Record<string, unknown>).merged, true);
// Passed and approved at the very heads read, with no forge review of the sibling: it merges.
assert.deepEqual(state.reviews, [], "no review is asked of the forge");
assert.equal(((await merge(both)) as Record<string, unknown>).merged, true);
// A status that says it read siblings, with no record of them, is no verdict to merge on.
state.merged = false;
keepSiblings(siblingsFile(stateDir), {});
await assert.rejects(merge(), /its verdict read siblings, and which ones is not held here/);
await assert.rejects(merge(both), /its verdict read siblings, and which ones is not held here/);
assert.equal(state.merged, false);
// A verdict that read none (an older controller, or a group of one) merges exactly as before.
@@ -612,16 +616,17 @@ test("gitea_merge_pull_request merges a verdict that read siblings only with its
assert.equal(((await merge()) as Record<string, unknown>).merged, true);
});
test("a verdict put out of date by a sibling's move is refused in its own words, and asked again once", async () => {
const merge = () => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED });
const words = "out of date: judged beside mesh-host#114 at bbbbbbbbbbbb, it is now at dddddddddddd; judged again on a recheck or when it is merged";
state.statuses = judgedAt(TRUNK).map((s) => ({ ...s, state: "pending", description: words }));
await assert.rejects(merge(), (err: Error) => {
assert.match(err.message, /its verdict was put out of date by a sibling's move \(out of date: judged beside mesh-host#114 at bbbbbbbbbbbb, it is now at dddddddddddd/);
assert.match(err.message, /was asked/);
assert.match(err.message, /never merges by itself/);
return true;
});
// **The merge lands only the head the Approve named** (novox/hq ADR 0421 §9, issue 1022): a pull request whose head
// differs from the approved one, or whose repository the Approve does not name, is refused with nothing merged.
test("gitea_merge_pull_request refuses a head the Approve did not name", async () => {
state.merged = false;
state.statuses = judgedAt(TRUNK);
const merge = (approved_heads: unknown) => tool("gitea_merge_pull_request").run({ owner: "novox", repo: "mesh-catalog", number: 7,
head: REVIEWED, approved_heads });
await assert.rejects(merge(["novox/mesh-catalog@" + "e".repeat(40)]), /is not the head the Approve covers \(eeeeeeeeeeee\)/);
await assert.rejects(merge(["novox/mesh-host@" + REVIEWED]), /is not among the heads the Approve covers/);
await assert.rejects(merge(["novox/mesh-catalog@abc123"]), /which is not owner\/repo@<full sha>/);
await assert.rejects(merge("novox/mesh-catalog@" + REVIEWED), /must be a list/);
assert.equal(state.merged, false);
assert.equal(state.emitted.length, 1, "the check is asked, once");
assert.equal(((await merge(["novox/mesh-catalog@" + REVIEWED.toUpperCase()])) as Record<string, unknown>).merged, true);
});
+31
View File
@@ -0,0 +1,31 @@
import { test } from "node:test";
import assert from "node:assert/strict";
import { readdirSync, readFileSync, statSync } from "node:fs";
import { join } from "node:path";
// **No ready mark is left in the forge module** (novox/hq ADR 0421 §4, issue 1022; how it is checked): no label, comment
// reading, trusted setting, file or merge rule of the mark stands in any of its sources, its manifest or its files. Tests
// are left out: they name the words to prove them gone.
const root = new URL("..", import.meta.url).pathname;
const FORBIDDEN = ["mesh/ready", "ready at", "ready-authors", "READY_AUTHORS", "readyAuthors", "readyMarkOf", "READY_LABEL", "approvedAt("];
function sources(dir: string): string[] {
const out: string[] = [];
for (const name of readdirSync(dir)) {
if (name === "node_modules" || name === "dist" || name === "test") continue;
const path = join(dir, name);
if (statSync(path).isDirectory()) out.push(...sources(path));
else if (/\.(ts|json|go)$/.test(name) || !name.includes(".")) out.push(path);
}
return out;
}
test("no ready mark, trusted ready-authors setting or approval-by-mark rule is left in the module", () => {
const found: string[] = [];
for (const file of sources(root)) {
if (/package-lock|go\.sum/.test(file)) continue;
const text = readFileSync(file, "utf8");
for (const word of FORBIDDEN) if (text.includes(word)) found.push(`${file.slice(root.length)} holds "${word}"`);
}
assert.deepEqual(found, [], "the ready mark is gone (ADR 0421 §4)");
});
+13 -12
View File
@@ -10,7 +10,7 @@ import { mkdtempSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { besideWords, groupRefusal, keepSiblings, namesSiblings, noteVerdict, readSiblings, siblingOutOfDate, siblingsFile,
import { approvedHeadsOf, besideWords, groupRefusal, keepSiblings, namesSiblings, noteVerdict, readSiblings, siblingOutOfDate, siblingsFile,
siblingsOf, siblingStatuses, weighSiblings, type SiblingRecord } from "../dist/siblings.js";
import { weighTrunks } from "../dist/trunk.js";
import { commentFor, statusesFor } from "../dist/pulls.js";
@@ -224,27 +224,28 @@ test("a trunk that moved tells the siblings' weighing; one first seen does not",
// --- the merge rule -------------------------------------------------------------------------------------------
test("the group rule names every missing thing: the group status at each head read, and each sibling head's review", async () => {
test("the group rule names every missing thing: the group status at each head read, and each head the Approve does not cover", async () => {
const self = { owner: "novox", repo: "mesh-controller", number: 346, head: MEMBER_HEAD };
const statuses: Record<string, string> = {};
const approved = new Set<string>();
const forge = {
headChecks: async (_o: string, r: string, sha: string) => (statuses[`${r}@${sha}`] ? { "mesh/delivery-group": { state: statuses[`${r}@${sha}`] } } : {}),
approvedAt: async (_o: string, r: string, n: number, head: string) => approved.has(`${r}#${n}@${head}`),
};
const siblings = siblingsOf([sibling]);
const none = await groupRefusal(forge, self, siblings);
assert.match(none!, /mesh\/delivery-group is missing at mesh-controller#346@aaaaaaaaaaaa; mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb carries no approving review at that head/);
const none = await groupRefusal(forge, self, siblings, undefined);
assert.match(none!, /mesh\/delivery-group is missing at mesh-controller#346@aaaaaaaaaaaa; mesh-controller#346@aaaaaaaaaaaa is covered by no Approve: the merge was handed no approved heads; mesh\/delivery-group is missing at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb is covered by no Approve/);
statuses[`mesh-controller@${MEMBER_HEAD}`] = "success";
statuses[`mesh-host@${HOST_HEAD}`] = "pending";
assert.match((await groupRefusal(forge, self, siblings))!, /^mesh\/delivery-group is pending at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb carries no approving review/);
const approved = approvedHeadsOf([`novox/mesh-controller@${MEMBER_HEAD}`])!;
assert.match((await groupRefusal(forge, self, siblings, approved))!, /^mesh\/delivery-group is pending at mesh-host#114@bbbbbbbbbbbb; mesh-host#114@bbbbbbbbbbbb is not among the heads the Approve covers$/);
statuses[`mesh-host@${HOST_HEAD}`] = "success";
assert.match((await groupRefusal(forge, self, siblings))!, /^mesh-host#114@bbbbbbbbbbbb carries no approving review at that head$/);
approved.add(`mesh-host#114@${HOST_HEAD}`);
assert.equal(await groupRefusal(forge, self, siblings), undefined);
assert.match((await groupRefusal(forge, self, siblings, approved))!, /^mesh-host#114@bbbbbbbbbbbb is not among the heads the Approve covers$/);
approved.set("novox/mesh-host", "c".repeat(40));
assert.match((await groupRefusal(forge, self, siblings, approved))!, /^mesh-host#114@bbbbbbbbbbbb is not the head the Approve covers \(cccccccccccc\)$/);
approved.set("novox/mesh-host", HOST_HEAD.toLowerCase());
assert.equal(await groupRefusal(forge, self, siblings, approved), undefined);
// A read that fails is named, never read as a pass.
const broken = { ...forge, approvedAt: async () => { throw new Error("the forge is away"); } };
assert.match((await groupRefusal(broken, self, siblings))!, /could not be read \(the forge is away\)/);
const broken = { headChecks: async () => { throw new Error("the forge is away"); } };
assert.match((await groupRefusal(broken, self, siblings, approved))!, /could not be read \(the forge is away\)/);
assert.equal(besideWords(siblings), "beside mesh-host#114@bbbbbbbb");
});
+1 -6
View File
@@ -282,15 +282,10 @@ test("the tools never act as the administrator: MESH_GITEA_TOOLS_USER naming it
assert.throws(() => MintedToken.fromEnv(forge.url, { ...env, MESH_GITEA_TOOLS_USER: ADMIN }), /names the admin account/);
});
test("the manifest hands the tools their account, and the merge-ready authors name it rather than the administrator", async () => {
test("the manifest hands the tools their account, and the Go forge bundle holds no administrator password", async () => {
const manifest = JSON.parse(await readFile(new URL("../module.json", import.meta.url), "utf8"));
const code = manifest.build.artifacts.find((a: any) => a.name === "code");
assert.equal(code.env.MESH_GITEA_TOOLS_USER, TOOLS_ACCOUNT);
const authors = String(manifest.config["ready-authors"].default).split(/\s+/);
assert.ok(authors.includes(TOOLS_ACCOUNT));
// Kept for now (review of issue 469): a pull request the admin account marked ready before the tools acted as
// their own keeps counting as ready after the delivery.
assert.ok(authors.includes(ADMIN), "marks made by the admin account before ADR 0365 keep counting");
// The Go forge bundle reads the token the tools' account holds, never the administrator's password.
const forgeBundle = manifest.build.artifacts.find((a: any) => a.name === "forge");
assert.ok(forgeBundle.env.MESH_GITEA_TOKEN_FILE);
+3 -3
View File
@@ -73,12 +73,12 @@ test("each forge event is named in the module's stream and trimmed", async () =>
head: { sha: "a".repeat(40) }, base: { ref: "main" } }, repository: { name: "hq", owner: { login: "novox" } } });
assert.equal(merged.name, "forge.pull-request.merged");
assert.equal(merged.body.merge_commit_sha, "c".repeat(40));
assert.equal(eventOf("pull_request", { action: "label_updated", pull_request: { number: 3 }, label: { name: "mesh/ready" } }).name,
assert.equal(eventOf("pull_request", { action: "label_updated", pull_request: { number: 3 }, label: { name: "kind/bug" } }).name,
"forge.pull-request.labeled");
const comment = eventOf("issue_comment", { action: "created", is_pull: true, issue: { number: 3 },
comment: { id: 9, body: "ready at abc", user: { login: "jo" } } });
comment: { id: 9, body: "looks fine to me", user: { login: "jo" } } });
assert.equal(comment.name, "forge.pull-request.commented");
assert.equal(comment.body.comment, "ready at abc");
assert.equal(comment.body.comment, "looks fine to me");
assert.equal(eventOf("push", { ref: "refs/heads/main", after: "d".repeat(40) }).name, "forge.push");
assert.equal(eventOf("pull_request_review_approved", {}).name, "forge.other");
});