From 434506f9b12e517fce9d327cc45fe928234b8b4e Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 12 Oct 2026 17:46:39 +0200 Subject: [PATCH] Serve the merge tool beside the one writer, and close the review's gaps The merge tool's pending bypassed the merge check's one writer from the tools' bundle (issue 456); it is now served beside it. A verdict no build judges names the announced base head, so the merge tool no longer refuses it for ever. A late verdict keeps its account, a trunk is read again in each head's turn so a fresh verdict is not put out of date, and a delivered member counts when a rebased member's group is composed again (novox/hq ADR 0323). --- modules/gitea/gate.ts | 6 +- modules/gitea/index.ts | 12 +- modules/gitea/merge.ts | 183 ++++++++++++++++++ modules/gitea/test/guard.test.ts | 28 ++- modules/gitea/test/token.test.ts | 3 +- modules/gitea/test/trunk.test.ts | 24 ++- modules/gitea/tools/index.ts | 169 +--------------- modules/gitea/trunk.ts | 22 ++- .../mesh-delivery/cmd/mesh-delivery/holder.go | 3 +- .../cmd/mesh-delivery/issue341_test.go | 53 +++++ 10 files changed, 317 insertions(+), 186 deletions(-) create mode 100644 modules/gitea/merge.ts diff --git a/modules/gitea/gate.ts b/modules/gitea/gate.ts index 326fb192..f92510bf 100644 --- a/modules/gitea/gate.ts +++ b/modules/gitea/gate.ts @@ -184,12 +184,12 @@ export class GateWriter { * opens no check — nothing is asked — so a verdict of a check asked before it keeps its weight. Answers what it wrote; * a write that fails fails the caller, and the move is weighed again on the next pass. */ - outOfDate(owner: string, repo: string, sha: string, decide: (checks: Record) => CommitStatus[], - target?: string): Promise { + outOfDate(owner: string, repo: string, sha: string, + decide: (checks: Record) => CommitStatus[] | Promise, target?: string): Promise { const h = this.head(owner, repo, sha); const written: CommitStatus[] = []; return this.turn(h, async () => { - for (const s of decide(await this.forge.headChecks(owner, repo, sha))) { + for (const s of await decide(await this.forge.headChecks(owner, repo, sha))) { await this.forge.setCommitStatus(owner, repo, sha, target ? { ...s, target_url: target } : s); written.push(s); } diff --git a/modules/gitea/index.ts b/modules/gitea/index.ts index 0c022a5c..ab0cfffe 100644 --- a/modules/gitea/index.ts +++ b/modules/gitea/index.ts @@ -22,8 +22,8 @@ // request into it go back to pending with the reason. Nothing is run for it. A verdict that lands judged against a trunk // commit that is no longer the trunk's head is said out of date in place of its pass or failure. // -// Serves gitea_commit_status from this bundle, not the tools', so a recheck's pending reaches that one writer -// (novox/hq issue 456). +// Serves gitea_commit_status and gitea_merge_pull_request from this bundle, not the tools', so a recheck's pending and +// the merge tool's ask of a check reach that one writer (novox/hq issue 456, ADR 0323). // // Every pull request the forge holds is announced, whatever its repository: the controller holds the // module graph and decides what is checked — a repository is never asked to opt in. @@ -39,11 +39,12 @@ import { emit, on } from "@novox/mesh-sdk/events"; import { registerModuleTools } from "@novox/mesh-sdk/tools"; import { commitStatusTool } from "./delivery.js"; +import { mergeTool } from "./merge.js"; import { GiteaClient, movedSince } from "./client.js"; import { CHECK_CONTEXT, REPO_CHECK_CONTEXT, commentFor, headsToAnnounce, statusesFor, type Announced, type Checked, type RepoCheckFacts, reposToLook } from "./pulls.js"; import { GateWriter, clonedAs, type BuildStarted } from "./gate.js"; -import { announcementOf, keepTook, outOfDateWords, sameCommit, tookFile, weighTrunks } from "./trunk.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 // nothing to watch; log and stay quiet rather than crash the runtime. With one, the first poll mints @@ -342,6 +343,9 @@ async function setVerdict(client: GiteaClient, event: { body: unknown }): Promis await client.setCommitStatus(c.owner, c.repo, c.commit, { state: "pending", context, description, ...(target ? { target_url: target } : {}) }); } + // Its account is kept and shown all the same (ADR 0323 §7): what it judged, and that it is out of date. + const said = commentFor(c, await repoCheckFacts(client, c, trunk)); + if (c.number) await client.addComment(c.owner, c.repo, c.number, lateVerdictComment(c, said, description)); console.log(`[gitea] ${c.owner}/${c.repo}#${c.number ?? "?"} at ${c.commit.slice(0, 8)}: merge check ${c.verdict}, ` + `${description}`); return; @@ -388,7 +392,7 @@ const gates = new GateWriter({ registerModuleTools("gitea", () => (gitea ? [commitStatusTool({ setCommitStatus: (owner, repo, sha, status) => (gitea as GiteaClient).setCommitStatus(owner, repo, sha, status), gates, -})] : [])); +}), mergeTool(gitea, { gates })] : [])); // **A build machine took an ask** (the build seat's `started`, novox/hq ADR 0157): said on the head it checks, when // the head is an open pull request's this module announced. Any other build — a branch, a merge's — says nothing. diff --git a/modules/gitea/merge.ts b/modules/gitea/merge.ts new file mode 100644 index 00000000..05ee77a2 --- /dev/null +++ b/modules/gitea/merge.ts @@ -0,0 +1,183 @@ +// The forge's merge tool, `gitea_merge_pull_request` (novox/hq ADR 0266, issue 381, ADR 0323). +// +// **Served from the watcher's bundle, beside the merge check's one writer** (index.ts, gate.ts; novox/hq issue 456): +// when a verdict is judged against an earlier trunk commit than the trunk's head, the tool asks the head's check and +// puts its statuses back to pending through that writer, so where the check runs is said and no second check is asked. +// Each bundle the runtime serves is its own process; from the tools' bundle the pending bypassed the writer's chain. + +import { emit } from "@novox/mesh-sdk/events"; +import type { ToolDefinition } from "@novox/mesh-sdk/tools"; +import type { CommitStatus, GiteaClient } from "./client.js"; +import { mergeableUnder, newestByContext } from "./checks.js"; +import { MERGE_METHODS, Refused, checkName } from "./guard.js"; +import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "./pulls.js"; +import { announcementOf, askedWords, outOfDateWords, tookFile, tookWords, twelve, verdictAtTrunk } from "./trunk.js"; + +/** A head named for a merge: at least 12 hex characters of its sha (novox/hq issue 381), up to a sha-256 commit's 64. */ +const REVIEWED_HEAD = /^[0-9a-f]{12,64}$/; + +/** What the merge tool reaches beside the forge: the merge check's one writer, how an event is said, the module's + * state directory, and the clock. */ +export interface MergeDeps { + gates: { rechecked(owner: string, repo: string, sha: string, status: CommitStatus): Promise }; + emit?: (type: string, body: unknown) => Promise; + stateDir?: string; + now?: () => Date; +} + +export function mergeTool(gitea: GiteaClient, given: MergeDeps): ToolDefinition { + const deps = { ...given, emit: given.emit ?? emit, stateDir: given.stateDir ?? process.env.MESH_GITEA_STATE_DIR, + now: given.now ?? (() => new Date()) }; + return { + name: "gitea_merge_pull_request", + description: "Merge a pull request at the head you name — the commit you reviewed — when that is still its head " + + "and every status its base branch's protection requires succeeded there (mesh/merge-gate, mesh/repo-check), " + + "judged against the trunk's head as it is now, optionally deleting the source branch afterwards. Refused " + + "otherwise, with nothing merged: no tool merges past a check, and none merges a head that moved after it was " + + "reviewed. A verdict judged against an earlier trunk commit is refused, naming both, and the head's check against " + + "the trunk's head is asked once, with how long the last check of the repository took: ask again at the same head " + + "once it passes — the tool never merges by itself.", + input: { + owner: { type: "string", description: "the repository owner" }, + repo: { type: "string", description: "the repository name" }, + number: { type: "number", description: "the PR number" }, + head: { type: "string", description: "the commit you reviewed, as its sha or at least its first 12 characters: " + + "refused unless it is the pull request's head" }, + method: { type: "string", description: "merge (default) | fast-forward-only | squash | rebase | rebase-merge. " + + "Only merge and fast-forward-only can be read back to prove the head you named is what landed" }, + delete_branch: { type: "boolean", description: "delete the source branch after merge (default true)" }, + }, + run: async (args) => { + const owner = checkName("owner", args.owner); + const repo = checkName("name", args.repo); + const number = Number(args.number); + const method = args.method ? String(args.method) : "merge"; + if (!MERGE_METHODS.has(method)) throw new Refused(`${method} is not a merge method: ${[...MERGE_METHODS].join(", ")}`); + const deleteBranch = args.delete_branch === undefined ? true : Boolean(args.delete_branch); + // **Only a head whose required statuses all succeeded** (novox/hq ADR 0266): read here rather than left + // to the forge, whose administrator — the account this tool's token is minted for — may merge past a + // rule that lets it. A rule that cannot be read is no rule passed. + // **Only the head that was reviewed** (novox/hq issue 381): mesh-controller#197 was reviewed at 065cfa0d, + // force-pushed to 3ced96f, checked green there, and merged at 3ced96f, because this tool merged whatever the + // head was and nothing told it which head the review had approved. The caller names it; a head that moved + // since is refused, and the forge is handed the same head, so a push between this read and the merge is + // refused by the forge too. + const named = args.head === undefined ? "" : String(args.head).trim().toLowerCase(); + if (!named) { + throw new Refused(`${owner}/${repo}#${number} is not merged: name the head you reviewed (head), so that only ` + + `that commit can land (novox/hq issue 381). Nothing was merged`); + } + // Twelve characters, not seven: an author who force-pushes can make a commit whose first seven match the + // reviewed one in seconds, and the review would then merge it. Twelve cannot be matched that way. + if (!REVIEWED_HEAD.test(named)) { + throw new Refused(`${owner}/${repo}#${number} is not merged: head ${named} does not name one commit — give its ` + + `sha, or at least its first 12 characters. Nothing was merged`); + } + const pull = await gitea.getPullRequest(owner, repo, number); + if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); + if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); + if (!pull.head_sha.toLowerCase().startsWith(named)) { + throw new Refused(`${owner}/${repo}#${number} is not merged: its head is ${pull.head_sha.slice(0, 12)}, not the ` + + `${named} you named. If you reviewed an earlier head, review the one it has now, then name it. Nothing was merged`); + } + const rule = await gitea.branchProtection(owner, repo, pull.base); + // Only into a branch whose protection requires the checks: a pull request whose base was moved to a + // branch nothing guards would merge past every check (novox/hq ADR 0266). + if (!rule || !rule.enable_status_check || !(rule.status_check_contexts ?? []).filter(Boolean).length) { + throw new Refused(`${owner}/${repo}#${number} merges into ${pull.base}, which no protection rule requiring ` + + `the checks guards: a tool merges only into such a branch (novox/hq ADR 0266). Nothing was merged`); + } + // **The verdict must be judged against the trunk's head** (novox/hq ADR 0323 §5, issue 341): a check judges the + // head merged into a named trunk commit, and a merge of another trunk is not the one that check judged. Compared + // here at the moment the merge is asked, not left to the statuses the trunk's move resets a pass later. When they + // differ nothing is merged, and the head's check against the trunk's head is asked once — the announcement a new + // head makes — never again while one is on its way. The caller asks again at the reviewed head once it passes. + const trunkHead = await gitea.branchHead(owner, repo, pull.base); + const checks = await gitea.headChecks(owner, repo, pull.head_sha); + const at = verdictAtTrunk(checks, trunkHead); + if (!at.current) { + const judged = at.judged ? `was judged against trunk ${twelve(at.judged)}` : "names no trunk commit it was judged against"; + const took = tookWords(tookFile(deps.stateDir), `${owner}/${repo}`); + const head = twelve(pull.head_sha), now = twelve(trunkHead); + if (at.onItsWay) { + throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + + `${now}. Its check is already asked and not yet answered (${checks[CHECK_CONTEXT]?.description}), so it is not ` + + `asked again; ${took}. Ask again at ${head} once both statuses pass: this tool never merges by itself ` + + `(novox/hq ADR 0323). A check that never answers is asked again with mesh-delivery.recheck. Nothing was merged`); + } + // Pending first, so a second ask finds the check on its way; then the head is announced as a new head is. An + // announcement that cannot be made puts the statuses back to out of date, so the next ask asks again rather + // than wait for ever on a check nobody asked. + const contexts = [CHECK_CONTEXT, ...(checks[REPO_CHECK_CONTEXT] ? [REPO_CHECK_CONTEXT] : [])]; + // Through the module's one writer of the merge check (gate.ts, novox/hq issue 456): the pending opens a new + // check of the head, so where it runs is said ("building on …") and its verdict is not weighed as older. + const say = async (description: string) => { + for (const context of contexts) { + await deps.gates.rechecked(owner, repo, String(pull.head_sha), { state: "pending", context, description, + ...(pull.html_url ? { target_url: pull.html_url } : {}) }); + } + }; + await say(askedWords(deps.now(), trunkHead)); + try { + const cloneURL = (await gitea.getRepo(owner, repo)).clone_url; + await deps.emit("pull.updated", await announcementOf(gitea, owner, repo, cloneURL, pull)); + } catch (err) { + await say(outOfDateWords(at.judged, trunkHead)).catch(() => {}); + throw new Error(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + + `${now}, and its check could not be asked: ${err instanceof Error ? err.message : err}. Nothing was merged`); + } + throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + + `${now}. The check of head ${head} merged into trunk ${now} was asked; ${took}. Ask again at ${head} once both ` + + `statuses pass: this tool never merges by itself (novox/hq ADR 0323). In a busy repository another merge may ` + + `move the trunk again first; this is then refused and asked once more. A check that never answers is asked ` + + `again with mesh-delivery.recheck. Nothing was merged`); + } + const statuses = newestByContext((await gitea.combinedStatus(owner, repo, pull.head_sha)).statuses); + const verdict = mergeableUnder(pull.base, rule, statuses); + if (verdict.mergeable !== true) { + throw new Refused(`${owner}/${repo}#${number} is not merged: ${verdict.says}. A pull request merges once ` + + `its checks succeed; no tool merges past them (novox/hq ADR 0266)`); + } + await gitea.mergePullRequest(owner, repo, number, method, deleteBranch, pull.head_sha); + // The merge commit only exists now; answered so the caller can follow what is built from it. + // pull.merged is the events entrypoint's to announce (index.ts), once, within its poll. + // From here the merge has happened. Nothing below may read as if it had not: a read-back that fails or cannot + // be done is said beside merged: true, and only a proven other head is an error — one that says the merge happened. + const answer: Record = { merged: true, number, method, head: pull.head_sha, deleted_branch: deleteBranch }; + let landed: string | undefined; + try { + const merged = await gitea.getPullRequest(owner, repo, number); + answer.merge_commit_sha = merged.merge_commit_sha; + // What landed, read back from the commit the merge made rather than from the forge's record of the pull + // request, which is the very thing a race would leave behind. A merge commit's second parent is the head it + // joined; a fast-forward lands the head itself. A squash or a rebase makes new commits that name no head. + if (!merged.merge_commit_sha) { + answer.read_back = "the forge named no merge commit, so what landed cannot be read back"; + } else if (method === "merge") { + const parents = await gitea.commitParents(owner, repo, merged.merge_commit_sha); + if (parents.length === 2) landed = parents[1]; + else answer.read_back = `the merge commit has ${parents.length} parent(s), not two: what landed cannot be read back`; + } else if (method === "fast-forward-only") { + // That the forge names the head itself as a fast-forward's merge commit is assumed, not seen on this forge: + // a different sha is said, not raised as another head having landed. + if (merged.merge_commit_sha === pull.head_sha) landed = merged.merge_commit_sha; + else answer.read_back = `the forge named ${merged.merge_commit_sha.slice(0, 12)} as this fast-forward's commit, ` + + `not the head: that the head landed could not be confirmed`; + } else { + answer.read_back = `a ${method} makes new commits that name no head: what landed cannot be read back`; + } + } catch (err) { + answer.read_back = `the merge happened, but reading back what landed failed: ${(err as Error).message}`; + } + // Loud, not a field beside merged: true that a caller reading only that would miss. The merge cannot be taken + // back by this tool; what must not happen is that it reads as the merge that was asked for. + if (landed !== undefined && landed !== pull.head_sha) { + throw new Error(`${owner}/${repo}#${number} WAS merged (${answer.merge_commit_sha}), but the forge merged ` + + `${landed.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} it was handed and you reviewed: a commit nobody ` + + `reviewed has landed. Read it before anything is built from it (novox/hq issue 381)`); + } + if (landed !== undefined) answer.landed = landed; + return answer; + }, + }; +} diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index 158b1e1e..f14b50ec 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -14,6 +14,7 @@ import { join } from "node:path"; import { GiteaClient } from "../dist/client.js"; import { ConfiguredToken } from "../dist/token.js"; import { getGiteaTools } from "../dist/tools/index.js"; +import { mergeTool } from "../dist/merge.js"; import { commitStatusTool } from "../dist/delivery.js"; import { GateWriter } from "../dist/gate.js"; import { judgeApiCall, judgeProtectionChange, ruleCovers, routedPath } from "../dist/guard.js"; @@ -126,12 +127,18 @@ const { port } = server.address() as { port: number }; const client = new GiteaClient(`http://127.0.0.1:${port}`, new ConfiguredToken("t")); // gitea_commit_status is served from the watcher's bundle, beside the merge check's one writer (novox/hq issue 456). const stateDir = mkdtempSync(join(tmpdir(), "gitea-tools-")); -const tools = [...getGiteaTools(client, { stateDir, emit: async (type, body) => { state.emitted.push({ type, body }); }, - now: () => new Date(2026, 9, 12, 15, 1) }), commitStatusTool({ - setCommitStatus: (o, r, sha, s) => client.setCommitStatus(o, r, sha, s), - gates: new GateWriter({ setCommitStatus: (o, r, sha, s) => client.setCommitStatus(o, r, sha, s), - gateOf: async () => undefined, gateHistory: async () => [] }), -})]; +// The merge check's one writer, as the watcher's bundle holds it: what the merge tool's pending goes through. +const writer = new GateWriter({ setCommitStatus: (o, r, sha, s) => client.setCommitStatus(o, r, sha, s), + gateOf: async () => undefined, gateHistory: async () => [], headChecks: async () => ({}) }); +const throughTheWriter: string[] = []; +const gates = { rechecked: (o: string, r: string, sha: string, s: any) => { + throughTheWriter.push(`${sha.slice(0, 12)} ${s.context} ${s.state}`); + return writer.rechecked(o, r, sha, s); +} }; +const tools = [...getGiteaTools(client), commitStatusTool({ + setCommitStatus: (o, r, sha, s) => client.setCommitStatus(o, r, sha, s), gates: writer, +}), mergeTool(client, { gates, stateDir, emit: async (type, body) => { state.emitted.push({ type, body }); }, + now: () => new Date(2026, 9, 12, 15, 1) })]; const tool = (name: string) => tools.find((t) => t.name === name)!; const writes = () => state.sent.map((s) => `${s.method} ${s.path}`); @@ -254,13 +261,17 @@ test("gitea_merge_pull_request refuses a verdict judged against an earlier trunk assert.equal(state.emitted[0].body.head_sha, REVIEWED); assert.equal(state.emitted[0].body.base, "main"); assert.deepEqual(state.emitted[0].body.paths, ["modules/gitea/x.ts"]); + // The announcement names the trunk's head, for a verdict no build judges (ADR 0323 §3). + assert.equal(state.emitted[0].body.base_sha, MOVED); + // Its pending went through the merge check's one writer (novox/hq issue 456), not around it. + assert.deepEqual(throughTheWriter.splice(0), ["abc1234def56 mesh/merge-gate pending", "abc1234def56 mesh/repo-check pending"]); const gate = state.statuses.find((s) => s.context === "mesh/merge-gate")!; assert.equal(gate.state, "pending"); assert.match(gate.description!, /^waiting in the build queue since 15:01: asked by the merge tool against trunk 3c4d5e6f7a8b/); assert.equal(state.statuses.find((s) => s.context === "mesh/repo-check")!.state, "pending"); // Asked again while that check is on its way: refused, and nothing is asked a second time. - await assert.rejects(merge(), /already asked and not yet answered .*not asked again; the last check of novox\/mesh-catalog took 4m31s/); + await assert.rejects(merge(), /already asked and not yet answered .*not asked again; the last check of novox\/mesh-catalog took 4m31s.*mesh-delivery\.recheck/); assert.equal(state.emitted.length, 1, "no second check while one is on its way"); state.statuses = [{ ...gate, description: "building on ace since 15:02" }, ...state.statuses.slice(1)]; await assert.rejects(merge(), /not asked again/); @@ -281,8 +292,7 @@ test("gitea_merge_pull_request refuses a verdict judged against an earlier trunk // An ask that cannot be announced puts the statuses back to out of date, so the next ask asks again. state.statuses = judgedAt(TRUNK); - const failing = getGiteaTools(client, { stateDir, emit: async () => { throw new Error("the bus is away"); } }) - .find((t) => t.name === "gitea_merge_pull_request")!; + const failing = mergeTool(client, { gates, stateDir, emit: async () => { throw new Error("the bus is away"); } }); await assert.rejects(failing.run({ owner: "novox", repo: "mesh-catalog", number: 7, head: REVIEWED }), /its check could not be asked: the bus is away\. Nothing was merged/); assert.match(state.statuses.find((s) => s.context === "mesh/merge-gate")!.description!, /^out of date: /); diff --git a/modules/gitea/test/token.test.ts b/modules/gitea/test/token.test.ts index 3bbfc5e9..750c8122 100644 --- a/modules/gitea/test/token.test.ts +++ b/modules/gitea/test/token.test.ts @@ -414,7 +414,8 @@ test("the tools register once there is a way to a token, and the first call mint [ "gitea_list_repos", "gitea_create_repo", "gitea_delete_repo", "gitea_list_issues", "gitea_get_issue", "gitea_create_issue", "gitea_close_issue", "gitea_add_comment", - "gitea_list_pull_requests", "gitea_get_pull_request", "gitea_create_pull_request", "gitea_merge_pull_request", + // gitea_merge_pull_request is served from the watcher's bundle, beside the merge check's one writer (ADR 0323). + "gitea_list_pull_requests", "gitea_get_pull_request", "gitea_create_pull_request", "gitea_close_pull_request", "gitea_reopen_pull_request", "gitea_update_pull_request", diff --git a/modules/gitea/test/trunk.test.ts b/modules/gitea/test/trunk.test.ts index dfd8f839..7e7fdd4b 100644 --- a/modules/gitea/test/trunk.test.ts +++ b/modules/gitea/test/trunk.test.ts @@ -6,7 +6,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { checkOnItsWay, outOfDateStatuses, outOfDateWords, trunkNamed, verdictAtTrunk, weighTrunks } from "../dist/trunk.js"; +import { checkOnItsWay, lateVerdictComment, outOfDateStatuses, outOfDateWords, trunkNamed, verdictAtTrunk, weighTrunks } from "../dist/trunk.js"; import { GateWriter } from "../dist/gate.js"; import { statusesFor, commentFor } from "../dist/pulls.js"; @@ -151,3 +151,25 @@ test("the trunk's move takes its turn after a verdict in flight, and weighs what assert.deepEqual(await moved, [], "a verdict judged against the trunk's head is not put out of date"); assert.equal(statuses.get(HEAD)!["mesh/merge-gate"].state, "success"); }); + +test("a verdict judged against a trunk newer than the pass's first read is not put out of date", async () => { + const NEWER = "ffffffffffffffffffffffffffffffffffffffff"; + const heads: Record = { main: TRUNK }; + const { forge, statuses } = aForge([{ number: 1, base: "main", head: HEAD }], heads); + const trunks: Record = { "novox/mesh-host:main": TRUNK }; + // The pass reads main at MOVED; by the head's turn the trunk moved again, and the head's check answered for it. + let reads = 0; + const moving = { ...forge, branchHead: async () => (reads++ === 0 ? MOVED : NEWER) }; + statuses.set(HEAD, { "mesh/merge-gate": { state: "success", description: `head x merged into trunk ${NEWER.slice(0, 12)} — pass` } }); + await weighTrunks(moving, new GateWriter(moving), trunks, "novox", "mesh-host"); + assert.equal(statuses.get(HEAD)!["mesh/merge-gate"].state, "success", "a fresh verdict was put out of date"); +}); + +test("a verdict that landed out of date keeps its account on the pull request", () => { + const words = outOfDateWords(TRUNK, MOVED); + const own = lateVerdictComment({ commit: HEAD, trunk: TRUNK.slice(0, 12), verdict: "fail" }, "**Merge check** at `dfb1415a`\n\nthe report", words); + assert.match(own, /the report/); + assert.match(own, /\*\*Out of date when it landed\*\*: out of date: judged against trunk a2c2c35bbbbb/); + const bare = lateVerdictComment({ commit: HEAD, trunk: TRUNK.slice(0, 12), verdict: "pass" }, null, words); + assert.match(bare, /head dfb1415aaaaa merged into trunk a2c2c35bbbbb, pass/); +}); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 68a00834..201b08d0 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -13,8 +13,6 @@ import { emit } from "@novox/mesh-sdk/events"; import { GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; -import { CHECK_CONTEXT, REPO_CHECK_CONTEXT } from "../pulls.js"; -import { announcementOf, askedWords, outOfDateWords, tookFile, tookWords, twelve, verdictAtTrunk } from "../trunk.js"; import { MERGE_METHODS, Refused, checkName, judgeApiCall, judgeContentsWrite, judgeDelete, judgeProtectionChange, pathSegments, routedPath, sentQuery, ruleCovers, @@ -61,19 +59,13 @@ function namesChecked(tools: ToolDefinition[]): ToolDefinition[] { })); } -/** What the tools reach beside the forge: how an event is said, the module's state directory, and the clock. */ -export interface ToolDeps { - emit?: (type: string, body: unknown) => Promise; - stateDir?: string; - now?: () => Date; +export function getGiteaTools(gitea: GiteaClient): ToolDefinition[] { + return namesChecked(allGiteaTools(gitea)); } -export function getGiteaTools(gitea: GiteaClient, deps: ToolDeps = {}): ToolDefinition[] { - return namesChecked(allGiteaTools(gitea, { emit: deps.emit ?? emit, stateDir: deps.stateDir ?? process.env.MESH_GITEA_STATE_DIR, - now: deps.now ?? (() => new Date()) })); -} - -function allGiteaTools(gitea: GiteaClient, deps: Required> & ToolDeps): ToolDefinition[] { +// gitea_merge_pull_request is served from the watcher's bundle (merge.ts, index.ts), beside the merge check's one +// writer (novox/hq issue 456, ADR 0323). +function allGiteaTools(gitea: GiteaClient): ToolDefinition[] { return [ // ---- Repositories ---- { @@ -273,155 +265,6 @@ function allGiteaTools(gitea: GiteaClient, deps: Required { - const owner = String(args.owner); - const repo = String(args.repo); - const number = Number(args.number); - const method = args.method ? String(args.method) : "merge"; - if (!MERGE_METHODS.has(method)) throw new Refused(`${method} is not a merge method: ${[...MERGE_METHODS].join(", ")}`); - const deleteBranch = args.delete_branch === undefined ? true : Boolean(args.delete_branch); - // **Only a head whose required statuses all succeeded** (novox/hq ADR 0266): read here rather than left - // to the forge, whose administrator — the account this tool's token is minted for — may merge past a - // rule that lets it. A rule that cannot be read is no rule passed. - // **Only the head that was reviewed** (novox/hq issue 381): mesh-controller#197 was reviewed at 065cfa0d, - // force-pushed to 3ced96f, checked green there, and merged at 3ced96f, because this tool merged whatever the - // head was and nothing told it which head the review had approved. The caller names it; a head that moved - // since is refused, and the forge is handed the same head, so a push between this read and the merge is - // refused by the forge too. - const named = args.head === undefined ? "" : String(args.head).trim().toLowerCase(); - if (!named) { - throw new Refused(`${owner}/${repo}#${number} is not merged: name the head you reviewed (head), so that only ` + - `that commit can land (novox/hq issue 381). Nothing was merged`); - } - // Twelve characters, not seven: an author who force-pushes can make a commit whose first seven match the - // reviewed one in seconds, and the review would then merge it. Twelve cannot be matched that way. - if (!REVIEWED_HEAD.test(named)) { - throw new Refused(`${owner}/${repo}#${number} is not merged: head ${named} does not name one commit — give its ` + - `sha, or at least its first 12 characters. Nothing was merged`); - } - const pull = await gitea.getPullRequest(owner, repo, number); - if (pull.merged) throw new Refused(`${owner}/${repo}#${number} is already merged`); - if (!pull.head_sha || !pull.base) throw new Refused(`${owner}/${repo}#${number} names no head commit or base branch`); - if (!pull.head_sha.toLowerCase().startsWith(named)) { - throw new Refused(`${owner}/${repo}#${number} is not merged: its head is ${pull.head_sha.slice(0, 12)}, not the ` + - `${named} you named. If you reviewed an earlier head, review the one it has now, then name it. Nothing was merged`); - } - const rule = await gitea.branchProtection(owner, repo, pull.base); - // Only into a branch whose protection requires the checks: a pull request whose base was moved to a - // branch nothing guards would merge past every check (novox/hq ADR 0266). - if (!rule || !rule.enable_status_check || !(rule.status_check_contexts ?? []).filter(Boolean).length) { - throw new Refused(`${owner}/${repo}#${number} merges into ${pull.base}, which no protection rule requiring ` + - `the checks guards: a tool merges only into such a branch (novox/hq ADR 0266). Nothing was merged`); - } - // **The verdict must be judged against the trunk's head** (novox/hq ADR 0323 §5, issue 341): a check judges the - // head merged into a named trunk commit, and a merge of another trunk is not the one that check judged. Compared - // here at the moment the merge is asked, not left to the statuses the trunk's move resets a pass later. When they - // differ nothing is merged, and the head's check against the trunk's head is asked once — the announcement a new - // head makes — never again while one is on its way. The caller asks again at the reviewed head once it passes. - const trunkHead = await gitea.branchHead(owner, repo, pull.base); - const checks = await gitea.headChecks(owner, repo, pull.head_sha); - const at = verdictAtTrunk(checks, trunkHead); - if (!at.current) { - const judged = at.judged ? `was judged against trunk ${twelve(at.judged)}` : "names no trunk commit it was judged against"; - const took = tookWords(tookFile(deps.stateDir), `${owner}/${repo}`); - const head = twelve(pull.head_sha), now = twelve(trunkHead); - if (at.onItsWay) { - throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + - `${now}. Its check is already asked and not yet answered (${checks[CHECK_CONTEXT]?.description}), so it is not ` + - `asked again; ${took}. Ask again at ${head} once both statuses pass: this tool never merges by itself ` + - `(novox/hq ADR 0323). Nothing was merged`); - } - // Pending first, so a second ask finds the check on its way; then the head is announced as a new head is. An - // announcement that cannot be made puts the statuses back to out of date, so the next ask asks again rather - // than wait for ever on a check nobody asked. - const contexts = [CHECK_CONTEXT, ...(checks[REPO_CHECK_CONTEXT] ? [REPO_CHECK_CONTEXT] : [])]; - const say = async (description: string) => { - for (const context of contexts) { - await gitea.setCommitStatus(owner, repo, pull.head_sha, { state: "pending", context, description, - ...(pull.html_url ? { target_url: pull.html_url } : {}) }); - } - }; - await say(askedWords(deps.now(), trunkHead)); - try { - const cloneURL = (await gitea.getRepo(owner, repo)).clone_url; - await deps.emit("pull.updated", await announcementOf(gitea, owner, repo, cloneURL, pull)); - } catch (err) { - await say(outOfDateWords(at.judged, trunkHead)).catch(() => {}); - throw new Error(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + - `${now}, and its check could not be asked: ${err instanceof Error ? err.message : err}. Nothing was merged`); - } - throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict ${judged}, and ${pull.base} is now ` + - `${now}. The check of head ${head} merged into trunk ${now} was asked; ${took}. Ask again at ${head} once both ` + - `statuses pass: this tool never merges by itself (novox/hq ADR 0323). Nothing was merged`); - } - const statuses = newestByContext((await gitea.combinedStatus(owner, repo, pull.head_sha)).statuses); - const verdict = mergeableUnder(pull.base, rule, statuses); - if (verdict.mergeable !== true) { - throw new Refused(`${owner}/${repo}#${number} is not merged: ${verdict.says}. A pull request merges once ` + - `its checks succeed; no tool merges past them (novox/hq ADR 0266)`); - } - await gitea.mergePullRequest(owner, repo, number, method, deleteBranch, pull.head_sha); - // The merge commit only exists now; answered so the caller can follow what is built from it. - // pull.merged is the events entrypoint's to announce (index.ts), once, within its poll. - // From here the merge has happened. Nothing below may read as if it had not: a read-back that fails or cannot - // be done is said beside merged: true, and only a proven other head is an error — one that says the merge happened. - const answer: Record = { merged: true, number, method, head: pull.head_sha, deleted_branch: deleteBranch }; - let landed: string | undefined; - try { - const merged = await gitea.getPullRequest(owner, repo, number); - answer.merge_commit_sha = merged.merge_commit_sha; - // What landed, read back from the commit the merge made rather than from the forge's record of the pull - // request, which is the very thing a race would leave behind. A merge commit's second parent is the head it - // joined; a fast-forward lands the head itself. A squash or a rebase makes new commits that name no head. - if (!merged.merge_commit_sha) { - answer.read_back = "the forge named no merge commit, so what landed cannot be read back"; - } else if (method === "merge") { - const parents = await gitea.commitParents(owner, repo, merged.merge_commit_sha); - if (parents.length === 2) landed = parents[1]; - else answer.read_back = `the merge commit has ${parents.length} parent(s), not two: what landed cannot be read back`; - } else if (method === "fast-forward-only") { - // That the forge names the head itself as a fast-forward's merge commit is assumed, not seen on this forge: - // a different sha is said, not raised as another head having landed. - if (merged.merge_commit_sha === pull.head_sha) landed = merged.merge_commit_sha; - else answer.read_back = `the forge named ${merged.merge_commit_sha.slice(0, 12)} as this fast-forward's commit, ` + - `not the head: that the head landed could not be confirmed`; - } else { - answer.read_back = `a ${method} makes new commits that name no head: what landed cannot be read back`; - } - } catch (err) { - answer.read_back = `the merge happened, but reading back what landed failed: ${(err as Error).message}`; - } - // Loud, not a field beside merged: true that a caller reading only that would miss. The merge cannot be taken - // back by this tool; what must not happen is that it reads as the merge that was asked for. - if (landed !== undefined && landed !== pull.head_sha) { - throw new Error(`${owner}/${repo}#${number} WAS merged (${answer.merge_commit_sha}), but the forge merged ` + - `${landed.slice(0, 12)}, not the ${pull.head_sha.slice(0, 12)} it was handed and you reviewed: a commit nobody ` + - `reviewed has landed. Read it before anything is built from it (novox/hq issue 381)`); - } - if (landed !== undefined) answer.landed = landed; - return answer; - }, - }, - { name: "gitea_close_pull_request", description: "Close a pull request without merging it — one whose work landed elsewhere, or was abandoned.", @@ -768,7 +611,7 @@ function allGiteaTools(gitea: GiteaClient, deps: Required { try { - return getGiteaTools(GiteaClient.fromEnv(env), { stateDir: env.MESH_GITEA_STATE_DIR }); + return getGiteaTools(GiteaClient.fromEnv(env)); } catch (err) { console.log(`[gitea] no tools — ${err instanceof Error ? err.message : String(err)}`); return []; diff --git a/modules/gitea/trunk.ts b/modules/gitea/trunk.ts index 311c9758..1f0b1a6a 100644 --- a/modules/gitea/trunk.ts +++ b/modules/gitea/trunk.ts @@ -100,6 +100,14 @@ export function askedWords(at: Date, trunkHead: string): string { `against trunk ${twelve(trunkHead)}`; } +/** The comment a verdict that landed out of date leaves (ADR 0323 §7: kept and shown): its own account when it has + * one, else what it judged and said, and that it is out of date. */ +export function lateVerdictComment(c: { commit: string; trunk?: string; verdict: string }, account: string | null, + outOfDate: string): string { + const own = account ?? `**Merge check** at \`${c.commit.slice(0, 8)}\`: ${judgedWords(c.commit, c.trunk ?? "")}, ${c.verdict}`; + return `${own}\n\n**Out of date when it landed**: ${outOfDate}`; +} + // --- a trunk's move, weighed -------------------------------------------------------------------------------------- /** What weighing a trunk's move reads of the forge. */ @@ -110,8 +118,8 @@ export interface TrunkForge { /** What it writes with: the merge check's one writer (gate.ts), which decides in the head's own turn. */ export interface OutOfDateWriter { - outOfDate(owner: string, repo: string, sha: string, decide: (checks: Record) => CommitStatus[], - target?: string): Promise; + outOfDate(owner: string, repo: string, sha: string, + decide: (checks: Record) => CommitStatus[] | Promise, target?: string): Promise; } /** @@ -135,8 +143,10 @@ export async function weighTrunks(forge: TrunkForge, gates: OutOfDateWriter, tru if (before) { let put = 0; for (const pull of open.filter((p) => p.base === base)) { - const written = await gates.outOfDate(owner, name, String(pull.head_sha), (checks) => outOfDateStatuses(checks, head), - pull.html_url || undefined); + // The trunk is read again in the head's own turn: a verdict judged against a head newer than the pass's first + // read — the trunk moved again meanwhile, and a check answered — is not put out of date. + const written = await gates.outOfDate(owner, name, String(pull.head_sha), + async (checks) => outOfDateStatuses(checks, await forge.branchHead(owner, name, base)), pull.html_url || undefined); if (written.length) put++; } console.log(`[gitea] ${owner}/${name}'s ${base} moved from ${twelve(before)} to ${twelve(head)}: ${put} open pull ` + @@ -216,10 +226,14 @@ export async function announcementOf(client: GiteaClient, owner: string, name: s } catch (err) { console.error(`[gitea] ${owner}/${name}#${pull.number}: what the head holds could not be read — ${err instanceof Error ? err.message : err}`); } + // The base branch's head as the head is announced: the trunk commit a verdict no build judges is said against + // (novox/hq ADR 0323 §3). Not said when it cannot be read; that verdict then names none. + const baseSHA = pull.base ? await client.branchHead(owner, name, String(pull.base)).catch(() => undefined) : undefined; return { owner, repo: name, number: pull.number, + ...(baseSHA ? { base_sha: baseSHA } : {}), title: pull.title, body: pull.body, base: pull.base, diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 664efd9f..585c9b64 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -1569,7 +1569,8 @@ func (h *Holder) planGroup(g *Group, members []*Delivery, now time.Time) []func( // was judged again after it went out of date: a recheck, or the merge tool's ask. allReady := true for _, d := range members { - if d.State != Ready && !(g.Closed && d.MergedAs != "" && !d.State.Final()) { + // In a closed group a member on the trunk counts — merged and on its way, or already delivered (reached). + if d.State != Ready && !(g.Closed && d.MergedAs != "" && (!d.State.Final() || d.State.Reached())) { allReady = false } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go index 4624d487..311dc194 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go @@ -385,3 +385,56 @@ func TestAnOutOfDateDeliveryAskedAgainIsBoundFromTheAsk(t *testing.T) { t.Fatalf("judged again it is %s, asked again %v", d.State, d.AskedAgain) } } + +// The same, with the first member already delivered when the second is rebased: a delivered member counts for the +// composed check of the new heads, which is asked, and the rebased member is never held (review of ADR 0323 §6). +func TestARebasedMemberAfterAnotherWasDeliveredIsComposedAgain(t *testing.T) { + const ctl, cat = "novox/mesh-controller", "novox/mesh-catalog" + const hc, hk, hc2 = "c1c1c1c1c1c1c1c1", "k1k1k1k1k1k1k1k1", "c2c2c2c2c2c2c2c2" + idk, idc2 := IDOf(cat, hk), IDOf(ctl, hc2) + w := newWorld(t) + w.h.PullUpdated(pr(ctl, 1, hc, "feat/g")) + w.h.PullUpdated(pr(cat, 2, hk, "feat/g")) + w.h.Checked(judged(ctl, 1, hc, "pass", trunk0)) + w.h.Checked(judged(cat, 2, hk, "pass", trunk0)) + w.settleAll() + g := w.h.groups["feat/g"] + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed"}) + w.settleAll() + + // The catalogue's member merges and its walk is done: delivered. + mk := merged(cat, 2, hk, merge1) + mk.Head = "feat/g" + w.h.PullMerged(mk) + walk := w.ctl.put(aWalk("plan-cat", cat, merge1, true)) + w.h.WalkMoved(walk) + w.settleAll() + walk = w.ctl.walk("plan-cat") + walk.State = walkDone + w.h.WalkMoved(w.ctl.put(walk)) + w.settleAll() + if s := w.state(idk); s != Delivered { + t.Fatalf("the catalogue's member is %s, not delivered", s) + } + + // The controller's member is rebased, and judged. + w.h.PullUpdated(pr(ctl, 1, hc2, "feat/g")) + w.h.Checked(judged(ctl, 1, hc2, "pass", trunk0)) + w.settleAll() + asked := w.ctl.checks[len(w.ctl.checks)-1] + if !strings.HasPrefix(asked, "feat/g:") || !strings.Contains(asked, idc2) { + t.Fatalf("the composed check was not asked again for the new heads: %v", w.ctl.checks) + } + if state, why := GroupState(g, w.h.membersOf(g)); state != "checking" { + t.Fatalf("the group reads %s (%s), not checking", state, why) + } + w.h.Checked(CheckedEvent{Group: "feat/g", ID: g.Check.Asked, Verdict: "pass", Summary: "composed again"}) + mc := merged(ctl, 1, hc2, "e0e0e0e0e0e0e0e0") + mc.Head = "feat/g" + w.h.PullMerged(mc) + w.h.WalkMoved(w.ctl.put(aWalk("plan-ctl", ctl, "e0e0e0e0e0e0e0e0", true))) + w.settleAll() + if d := w.delivery(idc2); d.State == Held { + t.Fatalf("the rebased member is held: %s", d.HeldWhy) + } +}