From 668444785d74870ca68ff84f5e2163a14d77720d Mon Sep 17 00:00:00 2001 From: jochen Date: Sat, 17 Oct 2026 20:44:54 +0200 Subject: [PATCH] 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. --- merge-check.sh | 9 +- modules/gitea/client.ts | 134 +------ modules/gitea/files/ready-authors | 3 - modules/gitea/merge.ts | 21 +- modules/gitea/module.json | 15 - modules/gitea/siblings.ts | 47 ++- modules/gitea/test/announcing.test.ts | 94 +---- modules/gitea/test/guard.test.ts | 59 +-- modules/gitea/test/no-ready-mark.test.ts | 31 ++ modules/gitea/test/siblings.test.ts | 25 +- modules/gitea/test/tools-account.test.ts | 7 +- modules/gitea/test/webhook.test.ts | 6 +- modules/gitea/tools/index.ts | 6 +- modules/gitea/webhook.ts | 2 +- .../cmd/mesh-delivery/approval.go | 112 ++++-- .../cmd/mesh-delivery/approval_test.go | 34 +- .../cmd/mesh-delivery/bus_test.go | 120 ++---- .../mesh-delivery/cmd/mesh-delivery/events.go | 65 +-- .../cmd/mesh-delivery/events_test.go | 284 ++++++++----- .../cmd/mesh-delivery/fakes_test.go | 38 +- .../mesh-delivery/cmd/mesh-delivery/holder.go | 21 +- .../cmd/mesh-delivery/issue341_test.go | 8 +- .../mesh-delivery/cmd/mesh-delivery/main.go | 31 +- .../cmd/mesh-delivery/manifest_test.go | 49 ++- .../cmd/mesh-delivery/markready_test.go | 143 ------- .../mesh-delivery/cmd/mesh-delivery/model.go | 38 +- .../mesh-delivery/cmd/mesh-delivery/ports.go | 65 +-- .../cmd/mesh-delivery/pullkey_test.go | 16 +- .../mesh-delivery/cmd/mesh-delivery/ready.go | 376 ++++++++---------- .../cmd/mesh-delivery/ready_test.go | 133 ++----- .../cmd/mesh-delivery/timers_test.go | 2 +- .../cmd/mesh-delivery/unchecked.go | 6 +- .../mesh-delivery/unchecked_events_test.go | 2 +- .../mesh-delivery/cmd/mesh-delivery/verbs.go | 59 --- modules/mesh-delivery/module.json | 7 +- 35 files changed, 795 insertions(+), 1273 deletions(-) delete mode 100644 modules/gitea/files/ready-authors create mode 100644 modules/gitea/test/no-ready-mark.test.ts delete mode 100644 modules/mesh-delivery/cmd/mesh-delivery/markready_test.go diff --git a/merge-check.sh b/merge-check.sh index a466457d..45d4a1ec 100755 --- a/merge-check.sh +++ b/merge-check.sh @@ -17,7 +17,8 @@ # toolchain has a C compiler. A requirement of the mesh's own forge the module cache does not hold (a # pinned SDK commit) is taken from its repository's clone beside the check (checks/deps); a module whose # dependencies still cannot be had **fails the check**, by name and why — its tests could not run, and a -# test that could not run is a failure, never a note (novox/hq issue 385). A module written in +# test that could not run is a failure, never a note (novox/hq issue 385). The integration tests (`//go:build +# integration`) run here, with -tags integration, and only here. A module written in # TypeScript is type-checked and tested by # merge-check-typescript.sh, declared above, in the TypeScript toolchain (novox/hq issues 302, 311); # 3. the words (novox/hq ADR 0244): no word the glossary retired for the tools, as copied in @@ -183,8 +184,10 @@ for m in $touched; do fi cgo=0 [ -n "$race" ] && cgo=1 - (cd "modules/$m" && CGO_ENABLED=$cgo GOPRIVATE=git.novox.be go vet ${modfile:+"-modfile=$modfile"} ./... && - CGO_ENABLED=$cgo GOPRIVATE=git.novox.be go test ${modfile:+"-modfile=$modfile"} $race -count=1 ./...) + # The integration tests (a file whose first line is `//go:build integration`: a test that starts a bus, a store or + # another process) run here and nowhere else: the unit suite stays under a second (novox/hq playbook 07). + (cd "modules/$m" && CGO_ENABLED=$cgo GOPRIVATE=git.novox.be go vet -tags integration ${modfile:+"-modfile=$modfile"} ./... && + CGO_ENABLED=$cgo GOPRIVATE=git.novox.be go test -tags integration ${modfile:+"-modfile=$modfile"} $race -count=1 ./...) done if [ -n "$untestable" ]; then echo "the check fails: the tests of$untestable could not run (novox/hq issue 385)" diff --git a/modules/gitea/client.ts b/modules/gitea/client.ts index 536420b8..76e6a285 100644 --- a/modules/gitea/client.ts +++ b/modules/gitea/client.ts @@ -8,7 +8,6 @@ import { ConfiguredToken, MintedToken, type TokenSource } from "./token.js"; import { MAIN_BRANCH, mainRuleBody, protectionBody, type BranchProtection, type ProtectionWanted } from "./protection.js"; import { Refused, checkSentPath, ruleCovers, pathSegment, pathSegments } from "./guard.js"; import { isCommit } from "./checks.js"; -import { TOOLS_ACCOUNT } from "./tools-account.js"; /** A repository's path on the forge, its owner and name each one segment (guard.ts pathSegment): uniform for every * call, whether the names came from a caller or from the forge's own answer. */ @@ -104,92 +103,6 @@ export interface OpenPull { checks: Record; /** The pull request's labels, by name. */ labels?: string[]; - /** Its ready mark (novox/hq issue 722): the label `mesh/ready` and the newest comment "ready at ", the - * head a reviewer approved. Absent without the label; `head` empty when the label names no head in a comment. */ - ready?: ReadyMark; -} - -/** The label that marks a pull request ready to merge at the head a comment names (novox/hq issue 722). */ -export const READY_LABEL = "mesh/ready"; - -/** A comment that names the head a pull request is marked ready at: "ready at ", on a line of its own. */ -const READY_AT = /^\s*ready at ([0-9a-f]{40}|[0-9a-f]{64})\b/im; - -/** A pull request's ready mark: the head it names, who said it and when, and the label's id, to take it off. */ -export interface ReadyMark { - head: string; - by?: string; - at?: string; - label_id: number; -} - -/** **The accounts whose "ready at" comment counts** (novox/hq issue 722): the trusted setting ready-authors, which - * the mesh writes to the file MESH_GITEA_READY_AUTHORS_FILE names as a line `authors= …`. A file - * that cannot be read leaves the mesh's own forge accounts alone, said; never an empty list read as everyone: the - * account the tools act as (novox/hq ADR 0365) and the admin account, whose marks were made before it. */ -export function readyAuthors(file = process.env.MESH_GITEA_READY_AUTHORS_FILE, own = process.env.MESH_GITEA_ADMIN_USER, - read: (f: string) => string = (f) => readFileSync(f, "utf8"), - tools = process.env.MESH_GITEA_TOOLS_USER || TOOLS_ACCOUNT): string[] { - const fallback = [...new Set([tools, own].filter((a): a is string => !!a))]; - if (!file) return fallback; - try { - const line = read(file).split("\n").find((l) => l.startsWith("authors=")); - const authors = (line ?? "").slice("authors=".length).split(/[\s,]+/).filter(Boolean); - if (authors.length) return authors; - console.error(`[gitea] ${file} names no account that may mark a pull request ready; only ${fallback.join(", ") || "none"} may`); - } catch (err) { - console.error(`[gitea] ${file} cannot be read (${err instanceof Error ? err.message : err}); only ` + - `${fallback.join(", ") || "none"} may mark a pull request ready`); - } - return fallback; -} - -/** One event of an issue's or pull request's timeline, as the forge keeps it, oldest first: a comment, a push to a - * pull request's branch (`pull_push`), a label added (body "1") or taken off (body ""), and the rest. */ -export interface TimelineEvent { - id: number; - type: string; - body: string; - created_at?: string; - user?: string; - label?: { id: number; name: string }; -} - -/** The comments that wrote "ready at" without counting, by id: each said once in the log (novox/hq issue 722). */ -const ignoredSaid = new Set(); - -/** **The ready mark a pull request's timeline makes** (novox/hq issue 722), read in the forge's order — every event is - * one row of one table, numbered as it happened, so the order is exact where times share a second. A "ready at" - * comment counts only when its author is one of the accounts given, and only when it was written after the label - * mesh/ready was last added and after the last push to the pull request's branch: a push or the label taken off - * undoes every mark before it, so a mark older than the latest push never counts, even with the label still on. A - * comment by any other account is ignored, and said once. */ -export function readyMarkOf(labelID: number, events: TimelineEvent[], authors: string[], - say: (line: string) => void = (l) => console.error(l)): ReadyMark { - const listed = new Set(authors.map((a) => a.toLowerCase())); - let labelled = false; - let mark: ReadyMark | undefined; - for (const e of [...events].sort((x, y) => x.id - y.id)) { - if (e.type === "pull_push") { - mark = undefined; - } else if (e.type === "label" && e.label?.name === READY_LABEL) { - labelled = e.body === "1"; - mark = undefined; - } else if (e.type === "comment") { - const m = READY_AT.exec(e.body); - if (!m) continue; - if (!listed.has(String(e.user ?? "").toLowerCase())) { - if (!ignoredSaid.has(e.id)) { - ignoredSaid.add(e.id); - say(`[gitea] a "ready at ${m[1].slice(0, 12)}" comment (${e.id}) by ${e.user ?? "an unknown account"} is ignored: ` + - `only ${authors.join(", ") || "no account"} may mark a pull request ready (novox/hq issue 722)`); - } - continue; - } - if (labelled) mark = { head: m[1].toLowerCase(), by: e.user, at: e.created_at, label_id: labelID }; - } - } - return mark ?? { head: "", label_id: labelID }; } /** One status on a head, the newest of its context: its state, what it said, and when it was set. */ @@ -445,7 +358,7 @@ export class GiteaClient { * requires and the contexts its head has a status of, in any state. What mesh-delivery reads to say a pull * request the mesh was never asked to check; asked of the forge, never of what the announcer remembers, so a * fault in the announcer cannot hide itself. Reads only. */ - async openPulls(readyAuthors: string[] = []): Promise { + async openPulls(): Promise { const out: OpenPull[] = []; const rules = new Map(); for (const found of await this.searchOpenPulls()) { @@ -458,13 +371,9 @@ export class GiteaClient { const rule = rules.get(base); const checks = await this.headChecks(found.owner, found.name, pull.head_sha); const labels = found.labels ?? []; - // The ready mark (novox/hq issue 722): its timeline is read, whole, only for a pull request that carries the label. - const label = labels.find((l) => l.name === READY_LABEL); - const ready = label ? readyMarkOf(label.id, await this.timeline(found.owner, found.name, pull.number), readyAuthors) - : undefined; out.push({ owner: found.owner, repo: found.name, number: pull.number, base: pull.base ?? "", head_sha: pull.head_sha, html_url: pull.html_url, required: rule?.enable_status_check ? (rule.status_check_contexts ?? []) : [], - statuses: Object.keys(checks), checks, labels: labels.map((l) => l.name), ...(ready ? { ready } : {}) }); + statuses: Object.keys(checks), checks, labels: labels.map((l) => l.name) }); } return out; } @@ -768,45 +677,6 @@ export class GiteaClient { return this.requestText(`${repoPath(owner, repo)}/pulls/${index}.diff`); } - /** **Whether a pull request carries an approving review at a head** (novox/hq ADR 0382 §6): a review of the forge that - * approves, is not dismissed and was made at that very head; or the mesh's ready mark naming that head (issue 722), which is - * how an agent's approval is read where the forge refuses an agent's approve. Reads only. */ - async approvedAt(owner: string, repo: string, index: number, head: string, authors?: string[]): Promise { - indexOf(index); - const want = head.toLowerCase(); - for (let page = 1; page <= 20; page++) { - const raw = await this.request(`${repoPath(owner, repo)}/pulls/${index}/reviews?page=${page}&limit=50`); - for (const r of raw ?? []) { - if (String(r?.state ?? "").toUpperCase() === "APPROVED" && !r?.dismissed && !r?.stale && - String(r?.commit_id ?? "").toLowerCase() === want) return true; - } - if (!raw || raw.length < 50) break; - } - const labels = await this.request(`${repoPath(owner, repo)}/issues/${index}/labels`); - const label = (labels ?? []).find((l) => l?.name === READY_LABEL); - if (!label) return false; - const ready = readyMarkOf(Number(label.id ?? 0), await this.timeline(owner, repo, index), authors ?? readyAuthors()); - return !!ready && ready.head.toLowerCase() === want; - } - - /** **Every event of an issue's or pull request's timeline**, oldest first, page by page (novox/hq issue 722): a long - * thread hides nothing. Bounded at 100 pages of 50; a longer one is said, and its first 5000 events read. */ - async timeline(owner: string, repo: string, index: number): Promise { - indexOf(index); - const out: TimelineEvent[] = []; - for (let page = 1; page <= 100; page++) { - const raw = await this.request(`${repoPath(owner, repo)}/issues/${index}/timeline?page=${page}&limit=50`); - for (const e of raw ?? []) { - out.push({ id: Number(e?.id ?? 0), type: String(e?.type ?? ""), body: String(e?.body ?? ""), - created_at: e?.created_at, user: e?.user?.login, - ...(e?.label ? { label: { id: Number(e.label.id ?? 0), name: String(e.label.name ?? "") } } : {}) }); - } - if (!raw || raw.length < 50) return out; - } - console.error(`[gitea] ${owner}/${repo}#${index} has more than 5000 timeline events; only the first 5000 are read`); - return out; - } - /** Every comment on an issue or pull request, oldest first. */ async listComments(owner: string, repo: string, index: number): Promise { indexOf(index); diff --git a/modules/gitea/files/ready-authors b/modules/gitea/files/ready-authors deleted file mode 100644 index 794440bf..00000000 --- a/modules/gitea/files/ready-authors +++ /dev/null @@ -1,3 +0,0 @@ -# The accounts whose "ready at " comment marks a pull request ready (novox/hq issue 722), from the setting -# ready-authors, written by the mesh. Replaced on every push; change the setting, never this file. -authors=${config:ready-authors} diff --git a/modules/gitea/merge.ts b/modules/gitea/merge.ts index 9b307b50..09bdcee6 100644 --- a/modules/gitea/merge.ts +++ b/modules/gitea/merge.ts @@ -11,7 +11,7 @@ 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 { groupRefusal, memberKey, namesSiblings, readSiblings, siblingOutOfDate, siblingsFile } from "./siblings.js"; +import { approvedHeadsOf, groupRefusal, memberKey, namesSiblings, notApproved, readSiblings, siblingOutOfDate, siblingsFile } from "./siblings.js"; import { announcementOf, askedWords, outOfDateWords, sameCommit, 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. */ @@ -47,6 +47,10 @@ export function mergeTool(gitea: GiteaClient, given: MergeDeps): ToolDefinition 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)" }, + approved_heads: { type: "array", items: { type: "string" }, description: "the heads the operator's Approve covers, " + + "each owner/repo@, one per member of the group (novox/hq ADR 0421 §9): refused unless this pull " + + "request's repository is named at the head it has now. Required when the pull request's verdict read siblings, " + + "where each sibling's head must be named too; there the Approve takes the place of an approving review" }, }, run: async (args) => { const owner = checkName("owner", args.owner); @@ -82,6 +86,14 @@ export function mergeTool(gitea: GiteaClient, given: MergeDeps): ToolDefinition 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`); } + // **Only a head the operator's Approve covers** (novox/hq ADR 0421 §9): mesh-delivery hands the heads its Approve + // named; a pull request whose head differs is refused, so a push between the Approve and this call merges nothing. + const approved = approvedHeadsOf(args.approved_heads); + if (approved) { + const why = notApproved(approved, `${owner}/${repo}`, number, String(pull.head_sha)); + if (why) throw new Refused(`${owner}/${repo}#${number} is not merged: ${why}. A merge lands the heads the operator ` + + `approved (novox/hq ADR 0421). Nothing was merged`); + } const rule = await gitea.appliedProtection(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). @@ -154,13 +166,12 @@ export function mergeTool(gitea: GiteaClient, given: MergeDeps): ToolDefinition `here, so the group cannot be weighed. Ask its check again at ${twelve(pull.head_sha)}. Nothing was merged`); } if (siblings.length) { - const missing = await groupRefusal({ headChecks: (o, r, sha) => gitea.headChecks(o, r, sha), - approvedAt: (o, r, n, h) => gitea.approvedAt(o, r, n, h) }, - { owner, repo, number, head: String(pull.head_sha) }, siblings); + const missing = await groupRefusal({ headChecks: (o, r, sha) => gitea.headChecks(o, r, sha) }, + { owner, repo, number, head: String(pull.head_sha) }, siblings, approved); if (missing) { throw new Refused(`${owner}/${repo}#${number} is not merged: its verdict was judged beside ${siblings.length} ` + `sibling(s), and ${missing}. A member judged beside its siblings merges once its group passed at the heads that ` + - `verdict read and each of them is approved there (novox/hq ADR 0382). Nothing was merged`); + `verdict read and the operator's Approve covers each of them (novox/hq ADR 0382, 0421). Nothing was merged`); } } await gitea.mergePullRequest(owner, repo, number, method, deleteBranch, pull.head_sha); diff --git a/modules/gitea/module.json b/modules/gitea/module.json index e8c73163..ca4f4618 100644 --- a/modules/gitea/module.json +++ b/modules/gitea/module.json @@ -169,12 +169,6 @@ "description": "The read teams of the agents' forge accounts (ADR 0365 clause 3): each team by name, {\"\": {\"repos\": [\"/\", ...], \"members\": [\"\", ...]}}. The forge gets a team agent-read- that reads only, with exactly those repositories and the accounts agent-; a team taken out of the value is emptied. Nothing else on the forge is touched; an agent account is in no team until it is listed here.", "trusted": true, "default": {} - }, - "ready-authors": { - "type": "string", - "description": "the forge accounts whose comment \"ready at \" marks a pull request ready for mesh-delivery to merge, separated by spaces: the account the forge's tools act as (novox/hq ADR 0365), the admin account for marks made before the tools acted as their own, and the operator's (novox/hq issue 722). A comment by any other account is ignored and said in the forge's log. Trusted: who may have a change merged is the operator's word", - "trusted": true, - "default": "mesh-tools mesh-admin jschoubben" } }, "facts": { @@ -198,14 +192,6 @@ "mode": "0700", "unknown": "program" }, - { - "id": "ready-authors", - "type": "file", - "path": "${dir:mesh-state}/ready-authors", - "mode": "0600", - "trusted": true, - "template": "files/ready-authors" - }, { "id": "forge-patches", "type": "directory", @@ -402,7 +388,6 @@ "MESH_GITEA_TOOLS_USER": "mesh-tools", "MESH_GITEA_STATE_DIR": "${dir:runtime-state}", "MESH_RECEIVES": "${dir:grants}/npm.json", - "MESH_GITEA_READY_AUTHORS_FILE": "${dir:mesh-state}/ready-authors", "MESH_GITEA_HOOK_URL": "http://172.17.0.1:3047", "MESH_GITEA_HOOK_PORT": "3047", "MESH_GITEA_HOOK_HOST": "172.17.0.1" diff --git a/modules/gitea/siblings.ts b/modules/gitea/siblings.ts index 64d05aad..5c970d37 100644 --- a/modules/gitea/siblings.ts +++ b/modules/gitea/siblings.ts @@ -18,6 +18,7 @@ import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { dirname } from "node:path"; import type { CommitStatus, GiteaPull, HeadCheck } from "./client.js"; +import { Refused } from "./guard.js"; import { CHECK_CONTEXT, REPO_CHECK_CONTEXT, clipped } from "./pulls.js"; import { sameCommit, twelve, type OutOfDateWriter } from "./trunk.js"; @@ -250,20 +251,43 @@ export async function weighSiblings(forge: SiblingWeighing, gates: OutOfDateWrit /** What the rule reads beside the pull request being merged. */ export interface GroupForge { headChecks(owner: string, repo: string, sha: string): Promise>; - /** Whether the sibling's head carries an approving review at that head. */ - approvedAt(owner: string, repo: string, number: number, head: string): Promise; +} + +/** The heads the operator's Approve covers (novox/hq ADR 0421 §9, issue 1022), as mesh-delivery hands them to the merge + * tool: `owner/repo@`, one per member, by repository. A malformed entry is refused, never skipped. */ +export function approvedHeadsOf(given: unknown): Map | undefined { + if (given === undefined || given === null) return undefined; + if (!Array.isArray(given)) throw new Refused("approved_heads must be a list of owner/repo@. Nothing was merged"); + const out = new Map(); + for (const entry of given) { + const m = /^([A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+)@([0-9a-f]{40}|[0-9a-f]{64})$/i.exec(String(entry)); + if (!m) throw new Refused(`approved_heads holds ${JSON.stringify(entry)}, which is not owner/repo@. Nothing was merged`); + out.set(m[1].toLowerCase(), m[2].toLowerCase()); + } + return out; +} + +/** Why a pull request's head is not one the Approve covers, in words, or nothing: its repository is not named, or names + * another head. */ +export function notApproved(approved: Map, repository: string, number: number, head: string): string | undefined { + const covered = approved.get(repository.toLowerCase()); + const at = `${repository.split("/")[1]}#${number}@${twelve(head)}`; + if (covered === undefined) return `${at} is not among the heads the Approve covers`; + if (covered !== head.toLowerCase()) return `${at} is not the head the Approve covers (${twelve(covered)})`; + return undefined; } /** * What stops a pull request whose verdict read siblings from merging, in words, or nothing: mesh/delivery-group must be - * a success at its own head and at every sibling head the verdict read, and every sibling head read must carry an - * approving review at that head. Every missing thing is named. + * a success at its own head and at every sibling head the verdict read, and every head read must be one the operator's + * Approve covers (ADR 0421 §9: the Approve is the review; the merge tool takes the approved heads from mesh-delivery). + * Every missing thing is named. */ export async function groupRefusal(forge: GroupForge, self: { owner: string; repo: string; number: number; head: string }, - siblings: SiblingNamed[]): Promise { + siblings: SiblingNamed[], approved: Map | undefined): Promise { const missing: string[] = []; - const members = [{ repository: `${self.owner}/${self.repo}`, number: self.number, head: self.head, sibling: false }, - ...siblings.map((s) => ({ repository: s.repository, number: s.number, head: s.head, sibling: true }))]; + const members = [{ repository: `${self.owner}/${self.repo}`, number: self.number, head: self.head }, + ...siblings.map((s) => ({ repository: s.repository, number: s.number, head: s.head }))]; for (const m of members) { const [owner, repo] = m.repository.split("/"); const at = `${repo}#${m.number}@${twelve(m.head)}`; @@ -275,11 +299,10 @@ export async function groupRefusal(forge: GroupForge, self: { owner: string; rep continue; } if (group?.state !== "success") missing.push(`${GROUP_CONTEXT} is ${group?.state ?? "missing"} at ${at}`); - if (!m.sibling) continue; - try { - if (!(await forge.approvedAt(owner, repo, m.number, m.head))) missing.push(`${at} carries no approving review at that head`); - } catch (err) { - missing.push(`whether ${at} carries an approving review could not be read (${err instanceof Error ? err.message : err})`); + if (!approved) missing.push(`${at} is covered by no Approve: the merge was handed no approved heads`); + else { + const why = notApproved(approved, m.repository, m.number, m.head); + if (why) missing.push(why); } } return missing.length ? missing.join("; ") : undefined; diff --git a/modules/gitea/test/announcing.test.ts b/modules/gitea/test/announcing.test.ts index a7a08c58..375124d7 100644 --- a/modules/gitea/test/announcing.test.ts +++ b/modules/gitea/test/announcing.test.ts @@ -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 " 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((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"]); }); diff --git a/modules/gitea/test/guard.test.ts b/modules/gitea/test/guard.test.ts index fda32d5d..c6643d51 100644 --- a/modules/gitea/test/guard.test.ts +++ b/modules/gitea/test/guard.test.ts @@ -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).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).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).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@/); + 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).merged, true); }); diff --git a/modules/gitea/test/no-ready-mark.test.ts b/modules/gitea/test/no-ready-mark.test.ts new file mode 100644 index 00000000..eb0c1a95 --- /dev/null +++ b/modules/gitea/test/no-ready-mark.test.ts @@ -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)"); +}); diff --git a/modules/gitea/test/siblings.test.ts b/modules/gitea/test/siblings.test.ts index 209e6c14..857b7640 100644 --- a/modules/gitea/test/siblings.test.ts +++ b/modules/gitea/test/siblings.test.ts @@ -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 = {}; - const approved = new Set(); 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"); }); diff --git a/modules/gitea/test/tools-account.test.ts b/modules/gitea/test/tools-account.test.ts index 249ac5ba..cf30a57a 100644 --- a/modules/gitea/test/tools-account.test.ts +++ b/modules/gitea/test/tools-account.test.ts @@ -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); diff --git a/modules/gitea/test/webhook.test.ts b/modules/gitea/test/webhook.test.ts index 1160de34..ea8d54cf 100644 --- a/modules/gitea/test/webhook.test.ts +++ b/modules/gitea/test/webhook.test.ts @@ -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"); }); diff --git a/modules/gitea/tools/index.ts b/modules/gitea/tools/index.ts index 6914c412..37fd2a6f 100644 --- a/modules/gitea/tools/index.ts +++ b/modules/gitea/tools/index.ts @@ -10,7 +10,7 @@ import { registerModuleTools, type ToolDefinition } from "@novox/mesh-sdk/tools"; import { emit } from "@novox/mesh-sdk/events"; -import { GiteaAdmin, GiteaClient, readyAuthors, type BranchProtection } from "../client.js"; +import { GiteaAdmin, GiteaClient, type BranchProtection } from "../client.js"; import { appendNote, run, viewBody, viewComment } from "../delivery.js"; import { containedFrom, isCommit, mergeableUnder, newestByContext } from "../checks.js"; import { @@ -518,9 +518,9 @@ function allGiteaTools(gitea: GiteaClient, admin?: () => GiteaAdmin): ToolDefini }, { name: "gitea_open_pulls", - description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438), and each pull request's labels and ready mark — the label mesh/ready and the head named by the newest comment \"ready at \" that a listed account (the setting ready-authors) wrote after the label was added and after the last push — which mesh-delivery merges on (issue 722). Reads only.", + description: "Every open pull request of every repository, from the forge itself: its head, its base, the statuses the base's protection requires and the ones its head has, each with its state, description and when it was set. What mesh-delivery reads to say a pull request the mesh was never asked to check (novox/hq issue 347), or whose check started and never answered (issue 438). Reads only.", input: {}, - run: async () => ({ pulls: await gitea.openPulls(readyAuthors()) }), + run: async () => ({ pulls: await gitea.openPulls() }), }, { name: "gitea_contains", diff --git a/modules/gitea/webhook.ts b/modules/gitea/webhook.ts index 7cb63671..87a072c4 100644 --- a/modules/gitea/webhook.ts +++ b/modules/gitea/webhook.ts @@ -73,7 +73,7 @@ function login(u: any): string | undefined { /** * The event a forge delivery is: its kind (X-Gitea-Event) and the payload's action, trimmed to ids, repository, refs, - * shas, author, labels and a comment's body (a ready mark is one, ADR 0364). A merge is pull-request.merged, a comment on + * shas, author, labels and a comment's body. A merge is pull-request.merged, a comment on * a pull request pull-request.commented. */ export function eventOf(kind: string, payload: any): ForgeEvent { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval.go b/modules/mesh-delivery/cmd/mesh-delivery/approval.go index 526de606..2b4f2bc0 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/approval.go @@ -11,17 +11,17 @@ import ( "git.novox.be/novox/mesh-sdk/go/asks" ) -// A merge into main waits for the operator's Approve (novox/hq ADR 0398 §3, issue 948). +// A merge into main waits for the operator's Approve (novox/hq ADR 0398 §3, issue 948, ADR 0421). // -// Where a ready mark at the head, a passing check and the group's turn used to call the merge tool at once, this -// owner now asks the operator on the router (the operator channel, as the knowledge base asks for a record): Approve -// or Decline, each option binding the act "merge these heads". It merges only after an Approve whose warrant holds for -// that ask — the same asker, the same ask's digest, an option it offered, in time, and the act that option bound. +// This owner asks the operator on the router (the operator channel, as the knowledge base asks for a record) as soon as +// a pull request's checks pass: Approve or Decline, each option binding the act "merge these heads". It merges only after +// an Approve whose warrant holds for that ask — the same asker, the same ask's digest, an option it offered, in time, and +// the act that option bound. // // - Asked once per head. A group is asked once, for all its members' heads together, by the first member that reaches // this point; the Approve covers every member at those heads. -// - A push after the ask cancels it (the mark is cleared with it): the new head needs a new mark and a new Approve. -// - A Decline merges nothing, is said once on the pull request, and clears the mark. +// - A push after the ask ends it, said once on the pull request: the new head is asked on its own. +// - A Decline merges nothing and is said once on the pull request. // - An ask that ended without a choice (expired, cancelled, refused by the router) is asked again when the router says so. // - The ask, its state and the warrant's words are kept on the delivery's ReadyRun and shown by `show`. // @@ -119,7 +119,7 @@ func repoName(repository string) string { func pullSaid(d *Delivery) string { return fmt.Sprintf("%s#%d", repoName(d.Repository), d.Number) } -// composeMergeAsk is the ask for a delivery, or for a group's members together (in the order they merge). +// composeMergeAsk is the ask for a delivery, or for a group's members together. func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, heads map[string]string, now time.Time) asks.Ask { var a asks.Ask a.ID, a.Who, a.Expires = id, asks.Operator, now.Add(AskLasts) @@ -127,30 +127,32 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, a.About = "delivery." + phone(repoName(d.Repository), 40) + "." + strconv.Itoa(d.Number) a.About = strings.ReplaceAll(a.About, " ", "-") var reviewers, reaches, details []string + unreviewed := "" for _, m := range members { - by, at := "a reviewer", "" - if m.Ready != nil { - if m.Ready.By != "" { - by = m.Ready.By - } - at = m.Ready.MarkedAt + words, ok := reviewWords(m) + if !ok && unreviewed == "" { + unreviewed = pullSaid(m) + " is " + words } - reviewers = append(reviewers, fmt.Sprintf("%s marked %s ready at %s", by, pullSaid(m), shortOf(m.Commit, 12))) + reviewers = append(reviewers, fmt.Sprintf("%s: %s", pullSaid(m), words)) plan := "no delivery plan yet" if m.Plan != nil && m.Plan.Summary != "" { plan = m.Plan.Summary } reaches = append(reaches, plan) - details = append(details, fmt.Sprintf("%s %q at %s: issues: %s; reviewer: %s, marked %s; reaches: %s", - pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), by, at, plan)) + details = append(details, fmt.Sprintf("%s %q at %s: issues: %s; review: %s; reaches: %s", + pullSaid(m), m.Title, shortOf(m.Commit, 12), issuesSaid([]*Delivery{m}), words, plan)) } if group == "" { a.Headline = phone("Merge into main: "+pullSaid(d)+" "+d.Title, asks.HeadlineLength) + summary := fmt.Sprintf("%s %s passed its checks. Approve merges it into main at %s; Decline leaves it unmerged.", + pullSaid(d), d.Title, shortOf(d.Commit, 12)) + if unreviewed != "" { + summary = strings.ToUpper(unreviewed[:1]) + unreviewed[1:] + ". " + summary + } a.Explanation = strings.Join([]string{ - phone(fmt.Sprintf("%s %s passed its checks and was reviewed. Approve merges it into main at %s; Decline leaves it unmerged.", - pullSaid(d), d.Title, shortOf(d.Commit, 12)), asks.SummaryLength), + phone(summary, asks.SummaryLength), phone("Issues: "+issuesSaid(members), asks.PointLength), - phone("Reviewer: "+reviewers[0], asks.PointLength), + phone("Review: "+reviewers[0], asks.PointLength), phone("Reaches: "+reaches[0], asks.PointLength), }, "\n") } else { @@ -159,11 +161,15 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, names = append(names, pullSaid(m)) } a.Headline = phone("Merge into main: group "+group+" ("+strings.Join(names, ", ")+")", asks.HeadlineLength) + summary := fmt.Sprintf("The group %s, %d pull requests, passed its checks. Approve merges all of them into main in one pass, at the heads named; Decline leaves them unmerged.", + group, len(members)) + if unreviewed != "" { + summary = strings.ToUpper(unreviewed[:1]) + unreviewed[1:] + ". " + summary + } a.Explanation = strings.Join([]string{ - phone(fmt.Sprintf("The group %s, %d pull requests, passed its checks and was reviewed. Approve merges all of them into main in their order, at the heads named; Decline leaves them unmerged.", - group, len(members)), asks.SummaryLength), - phone("Members, in order: "+strings.Join(names, ", "), asks.PointLength), + phone(summary, asks.SummaryLength), phone("Issues: "+issuesSaid(members), asks.PointLength), + phone("Review: "+strings.Join(reviewers, "; "), asks.PointLength), phone("Reaches: "+strings.Join(reaches, "; "), asks.PointLength), }, "\n") } @@ -178,8 +184,8 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, }(), "\n") } for _, o := range []struct{ id, label, does string }{ - {OptionApprove, "Approve", "the pull request is merged into main at the head named (a group: all its members, in order)"}, - {OptionDecline, "Decline", "nothing is merged and the ready mark is taken off"}, + {OptionApprove, "Approve", "the pull request is merged into main at the head named (a group: all its members, in one pass)"}, + {OptionDecline, "Decline", "nothing is merged"}, } { binds, _ := asks.ActDigest(mergeAct(id, o.id, heads)) a.Options = append(a.Options, asks.Option{ID: o.id, Label: o.label, Does: o.does, Level: asks.Approve, Binds: binds}) @@ -187,7 +193,7 @@ func composeMergeAsk(id string, d *Delivery, group string, members []*Delivery, return a } -// askMembers are the deliveries one ask covers: the group's members not yet merged, in the group's order, or the delivery. +// askMembers are the deliveries one ask covers: the group's members not yet merged, by id, or the delivery. func (h *Holder) askMembers(d *Delivery) []*Delivery { g := h.groups[d.Group] if d.Group == "" || g == nil { @@ -195,7 +201,9 @@ func (h *Holder) askMembers(d *Delivery) []*Delivery { } var out []*Delivery seen := map[string]bool{} - for _, id := range append(append([]string(nil), g.Order...), g.Members...) { + ids := append([]string(nil), g.Members...) + sort.Strings(ids) + for _, id := range ids { if o := h.deliveries[id]; o != nil && !seen[id] && o.MergedAs == "" && !o.State.Final() { seen[id] = true out = append(out, o) @@ -261,16 +269,20 @@ func (h *Holder) headsHold(d *Delivery, c *ReadyRun) bool { return true } -// endStaleAsks ends every ask whose heads are no longer its group's: a push to any member, marked or not, ends the -// approval (ADR 0398 §3). Under the lock; the cancels run with the look's asks. +// endStaleAsks ends every ask whose heads are no longer its group's: a push to any member ends the approval (ADR 0398 +// §3), and says so once on each pull request it covered (ADR 0421 §6). An ask for a head a newer one replaced ends too. +// A Decline stands, and is not ended. Under the lock; the cancels run with the look's asks. func (h *Holder) endStaleAsks() { for _, d := range h.deliveries { r := d.Ready - if r == nil || r.Merged != nil || d.MergedAs != "" || (r.AskID == "" && r.Approved == nil) || len(r.Heads) == 0 { + if r == nil || r.Merged != nil || d.MergedAs != "" || r.Declined != nil || (r.AskID == "" && r.Approved == nil) || len(r.Heads) == 0 { continue } - if !h.headsHold(d, r) { - h.cancels = append(h.cancels, h.dropAsk(r.AskID, "the heads it covers are no longer the group's heads")) + switch { + case d.State.Final(): + h.cancels = append(h.cancels, h.dropAsk(r.AskID, "its head "+shortOf(d.Commit, 12)+" is no longer the pull request's head, or the pull request closed")) + case !h.headsHold(d, r): + h.cancels = append(h.cancels, h.dropAsk(r.AskID, "a head it covers moved on: the heads it covers are no longer the group's heads")) } } } @@ -307,14 +319,12 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { ask := composeMergeAsk(id, d, group, members, heads, now) taken := []*Delivery{} for _, m := range members { - if m.Ready == nil { - continue - } + h.runOf(m, now) m.Ready.AskID, m.Ready.Ask, m.Ready.Heads, m.Ready.AskedAt, m.Ready.Asks = id, &ask, heads, now, max(m.Ready.Asks, r.Asks) h.keep(m) taken = append(taken, m) } - h.Logf("[mesh-delivery] %s is marked ready and its checks passed: the operator is asked to Approve its merge into main (%s, ask %d)", + h.Logf("[mesh-delivery] %s passed its checks: the operator is asked to Approve its merge into main (%s, ask %d)", d.ID, id, r.Asks) return "the operator's Approve is asked (" + id + ")", func() { if err := h.Operator.Ask(ask); err != nil { @@ -332,19 +342,35 @@ func (h *Holder) approval(d *Delivery, now time.Time) (string, func()) { } } -// dropAsk forgets a pending ask on every delivery it covers and takes it back from the router; under the lock. +// dropAsk forgets a pending ask on every delivery it covers and takes it back from the router; under the lock. What it +// says is said once on each pull request it covered (ADR 0421 §6): the Approve is ended, and why. func (h *Holder) dropAsk(id, why string) func() { + var said []func() + seen := map[string]bool{} + var covered []*Delivery for _, d := range h.deliveries { if d.Ready != nil && d.Ready.AskID == id { - d.Ready.AskID, d.Ready.Ask, d.Ready.Heads = "", nil, nil - if d.Ready.Merged == nil { - d.Ready.Approved, d.Ready.Warrant = nil, "" - } - h.keep(d) + covered = append(covered, d) + } + } + sort.Slice(covered, func(i, j int) bool { return covered[i].ID < covered[j].ID }) + for _, d := range covered { + d.Ready.AskID, d.Ready.Ask, d.Ready.Heads = "", nil, nil + if d.Ready.Merged == nil { + d.Ready.Approved, d.Ready.Warrant = nil, "" + } + h.keep(d) + if k := fmt.Sprintf("%s#%d", d.Repository, d.Number); !seen[k] && d.Number != 0 { + seen[k] = true + said = append(said, h.sayOn(d, "mesh-delivery: the operator's Approve of this pull request's merge into main is ended: "+why+ + ". Nothing is merged on it. The operator is asked again once the checks pass at the new head.")) } } h.Logf("[mesh-delivery] the operator's ask %s is cancelled: %s", id, why) return func() { + for _, s := range said { + s() + } if h.Operator == nil { return } @@ -452,7 +478,7 @@ func (h *Holder) decided(id, state string, w *asks.Warrant) []func() { r.Declined = &now h.Logf("[mesh-delivery] %s: the operator declined its merge into main (%s)", d.ID, r.Warrant) said = append(said, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** The merge of this pull request into main at %s was "+ - "declined: %s. The ready mark is taken off; a new mark, once reviewed, asks again.", d.Commit, r.Warrant))) + "declined: %s. A new head asks the operator again.", d.Commit, r.Warrant))) } h.keep(d) } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go b/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go index 95f2b2cb..e37d3f55 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/approval_test.go @@ -18,9 +18,10 @@ func markedGreen(t *testing.T) *world { w.op.auto = false p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") p.Title = "Fix the apostrophe (issue 948, issue 12)" + p.Body = "Reviewed-by: reviewer@shanks at " + readyHead w.h.PullUpdated(p) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} w.h.SeeOpenPulls() w.settleAll() return w @@ -41,7 +42,7 @@ func TestAMarkedGreenDeliveryIntoMainIsAskedAndNotMergedUntilApproved(t *testing len(a.Options) != 2 || a.Options[0].ID != OptionApprove || a.Options[1].ID != OptionDecline { t.Fatalf("the ask is not the merge ask: %+v", a) } - for _, want := range []string{"issue 948", "issue 12", "reviewer", "builds app"} { + for _, want := range []string{"issue 948", "issue 12", "reviewed by reviewer on shanks", "builds app"} { if !strings.Contains(a.Explanation, want) { t.Fatalf("the ask does not say %q: %s", want, a.Explanation) } @@ -103,7 +104,7 @@ func TestAnAnswerThatIsNotForThisAskMergesNothing(t *testing.T) { } } -func TestADeclinedMergeIsNotMergedSaidOnceAndItsMarkTakenOff(t *testing.T) { +func TestADeclinedMergeIsNotMergedAndSaidOnce(t *testing.T) { w := markedGreen(t) w.op.answer(w.op.open()[0], OptionDecline) for range 3 { @@ -117,18 +118,12 @@ func TestADeclinedMergeIsNotMergedSaidOnceAndItsMarkTakenOff(t *testing.T) { !strings.Contains(w.forge.comments[0], "chose Decline") { t.Fatalf("the Decline is not said once on the pull request, with its warrant: %v", w.forge.comments) } - if len(w.forge.unlabels) == 0 || w.forge.unlabels[0] != "novox/app#7-31" { - t.Fatalf("the mark was not taken off: %v", w.forge.unlabels) - } } -func TestAPushAfterTheAskCancelsItAndTheNewHeadNeedsANewMarkAndApprove(t *testing.T) { +func TestAPushAfterTheAskCancelsItAndTheNewHeadIsAskedOnItsOwnWhenItsChecksPass(t *testing.T) { w := markedGreen(t) old := w.op.open()[0] w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, pushed, "pass", "t1t1t1t1t1t1")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)} // the old mark, on the new head - w.h.SeeOpenPulls() w.settleAll() if len(w.op.cancelled) != 1 || w.op.cancelled[0] != old || len(w.op.open()) != 0 { t.Fatalf("the ask was not cancelled: cancelled %v, open %v", w.op.cancelled, w.op.open()) @@ -139,9 +134,8 @@ func TestAPushAfterTheAskCancelsItAndTheNewHeadNeedsANewMarkAndApprove(t *testin if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready.AskID != "" { t.Fatalf("the cancelled ask is still kept as waiting: %+v", d.Ready) } - // The new head, marked by its reviewer, is asked afresh. - w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, pushed)} - w.h.SeeOpenPulls() + // The new head is asked afresh once its checks pass. + w.h.Checked(judgedAt("novox/app", 7, pushed, "pass", "t1t1t1t1t1t1")) w.settleAll() if open := w.op.open(); len(open) != 1 || open[0] == old { t.Fatalf("the new head was not asked afresh: %v", open) @@ -175,8 +169,8 @@ func TestAGroupIsAskedOnceForAllItsHeadsAndMergedInOrderOnOneApprove(t *testing. w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) w.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1")) w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3")) - w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), - markedPull("novox/mesh-catalog", 2, catHead, catHead)} + w.forge.open = []OpenPull{openPull("novox/mesh-controller", 1, ctlHead), + openPull("novox/mesh-catalog", 2, catHead)} w.h.SeeOpenPulls() w.settleAll() w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"}) @@ -224,8 +218,8 @@ func TestAPushToAnUnmarkedGroupMemberEndsTheGroupsAskAndApprove(t *testing.T) { w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) w.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1")) w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3")) - w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), - markedPull("novox/mesh-catalog", 2, catHead, catHead)} + w.forge.open = []OpenPull{openPull("novox/mesh-controller", 1, ctlHead), + openPull("novox/mesh-catalog", 2, catHead)} w.h.SeeOpenPulls() w.settleAll() w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"}) @@ -236,7 +230,7 @@ func TestAPushToAnUnmarkedGroupMemberEndsTheGroupsAskAndApprove(t *testing.T) { } // The catalog member is pushed; its new head has no mark yet. w.h.PullUpdated(pr("novox/mesh-catalog", 2, catPushed, "feat/x", "modules/app/module.json")) - w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), + w.forge.open = []OpenPull{openPull("novox/mesh-controller", 1, ctlHead), {Owner: "novox", Repo: "mesh-catalog", Number: 2, Base: "main", HeadSHA: catPushed}} w.h.SeeOpenPulls() w.settleAll() @@ -333,8 +327,8 @@ func TestIssue1025AMergeInAGroupCarriesTheGroupAndItsHeads(t *testing.T) { w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) w.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1")) w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3")) - w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), - markedPull("novox/mesh-catalog", 2, catHead, catHead)} + w.forge.open = []OpenPull{openPull("novox/mesh-controller", 1, ctlHead), + openPull("novox/mesh-catalog", 2, catHead)} w.h.SeeOpenPulls() w.settleAll() w.h.Checked(CheckedEvent{Group: "feat/x", ID: "check-1", Verdict: "pass", Summary: "both compose"}) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go index ded238e3..90b8a8b2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/bus_test.go @@ -1,3 +1,5 @@ +//go:build integration + package main import ( @@ -21,9 +23,10 @@ import ( "github.com/nats-io/nats.go/jetstream" ) -// **The acceptance of acting on events** (novox/hq ADR 0406, design 47 milestone 2, ADR 0391 phase 3, issue 975): a -// pull request with a passing verdict gets its `mesh/ready` label event and its `ready at ` comment event, and -// the merge is asked within five seconds of the comment event — with no tick, no poll and no reconcile to find it. +// **The acceptance of acting on events** (novox/hq ADR 0406, design 47 milestone 2, ADR 0391 phase 3, issue 975; ADR +// 0421, issue 1022): a pull request whose verdict passes is asked of the operator within seconds of the verdict's event, +// with no label, comment or mark written by anyone, and the merge is asked within five seconds of the Approve's event — +// with no tick, no poll and no reconcile to find it. // Proved over a real bus: a JetStream server is linked into the test and started for it alone; the module is this // test binary started again as the bundle, speaking MCP on its standard streams to a runtime played here, which does // what the tool runner does for a bundle — its state on the buckets the controller would make from the manifest, its @@ -390,7 +393,7 @@ func (b *testBus) publish(t *testing.T, subject string, body any) time.Time { return time.Now() } -func TestAReadyMarkAndItsApproveAreMergedWithinSecondsOfTheirEventsOverARealBus(t *testing.T) { +func TestAPassingVerdictAsksTheApproveAndItsApproveMergesWithinSecondsOverARealBus(t *testing.T) { b := newTestBus(t) rt := b.start(t) ctx, cancel := context.WithCancel(context.Background()) @@ -402,55 +405,22 @@ func TestAReadyMarkAndItsApproveAreMergedWithinSecondsOfTheirEventsOverARealBus( defer rt.mu.Unlock() return len(rt.subs) >= len(followed)+1 }) + for _, k := range followed { + if strings.Contains(k, "labeled") || strings.Contains(k, "commented") { + t.Fatalf("the module follows %s: a label or a comment starts nothing here (ADR 0421 §4)", k) + } + } // A pull request into main is announced, and the controller's verdict on its head passes. const head = "1111111111111111111111111111111111111111" announced := pr("novox/app", 7, head, "fix/x", "src/a.go") announced.BaseSHA = "t1t1t1t1t1t1" announced.Title = "Fix the apostrophe (issue 975)" + announced.Body = "Reviewed-by: lead@shanks at " + head b.publish(t, "mesh.mod.gitea.event.pull.updated", announced) - b.publish(t, "mesh.seat."+ControllerSeat+".event.checked", judgedAt("novox/app", 7, head, "pass", "t1t1t1t1t1t1")) - rt.until("the module to keep the delivery of the head as ready", 10*time.Second, func() bool { - keys, err := b.kvs["deliveries"].Keys(context.Background()) - if err != nil { - return false - } - for _, k := range keys { - e, err := b.kvs["deliveries"].Get(context.Background(), k) - var d Delivery - if err == nil && json.Unmarshal(e.Value(), &d) == nil && d.State == Ready && d.Commit == head { - return true - } - } - return false - }) - // The pull request's key holds its head, the trunk commit it was asked against and the verdict. - rt.until("the pull request's key to hold head, trunk and verdict", 5*time.Second, func() bool { - e, err := b.kvs["pulls"].Get(context.Background(), "novox.app.7") - var k PullKey - return err == nil && json.Unmarshal(e.Value(), &k) == nil && k == PullKey{Head: head, Trunk: "t1t1t1t1t1t1", Verdict: "pass"} - }) - reads := len(rt.asksOf("gitea.gitea_open_pulls")) + passed := b.publish(t, "mesh.seat."+ControllerSeat+".event.checked", judgedAt("novox/app", 7, head, "pass", "t1t1t1t1t1t1")) - // The reviewer's label and comment: the forge shows the label alone at first, and the mark whole once the comment is - // written. Nothing is asked for the label alone. - rt.mu.Lock() - rt.openPulls = []OpenPull{markedPull("novox/app", 7, head, "")} - rt.mu.Unlock() - b.publish(t, "mesh.mod.gitea.event.forge.pull-request.labeled", labeled(7, ReadyLabel)) - rt.until("the forge's open pull requests to be read for the label", 5*time.Second, func() bool { - return len(rt.asksOf("gitea.gitea_open_pulls")) > reads - }) - rt.mu.Lock() - rt.openPulls = []OpenPull{markedPull("novox/app", 7, head, head)} - nothing := len(rt.operatorAsks) - rt.mu.Unlock() - if nothing != 0 { - t.Fatalf("the operator was asked for a label with no head named") - } - commented := b.publish(t, "mesh.mod.gitea.event.forge.pull-request.commented", commentedReady(7, head)) - - // The operator is asked, within the comment's event, and answers at once (a person's Approve, proven by the router). + // The operator is asked, within seconds of the verdict's event, with no label, comment or mark written by anyone. rt.until("the operator to be asked to Approve the merge", 5*time.Second, func() bool { rt.mu.Lock() defer rt.mu.Unlock() @@ -458,25 +428,39 @@ func TestAReadyMarkAndItsApproveAreMergedWithinSecondsOfTheirEventsOverARealBus( }) rt.mu.Lock() ask := rt.operatorAsks[0] + asked := time.Now() rt.mu.Unlock() + if !strings.Contains(ask.Explanation, "reviewed by lead") { + t.Fatalf("the ask does not name the reviewer: %s", ask.Explanation) + } + t.Logf("the operator was asked %s after the verdict's event", asked.Sub(passed)) + if len(rt.asksOf("gitea.gitea_merge_pull_request")) != 0 { + t.Fatalf("merged before the Approve") + } + // The pull request's key holds its head, the trunk commit it was asked against and the verdict. + rt.until("the pull request's key to hold head, trunk and verdict", 5*time.Second, func() bool { + e, err := b.kvs["pulls"].Get(context.Background(), "novox.app.7") + var k PullKey + return err == nil && json.Unmarshal(e.Value(), &k) == nil && k == PullKey{Head: head, Trunk: "t1t1t1t1t1t1", Verdict: "pass"} + }) + + // The operator approves (a person's Approve, proven by the router): the merge is asked within seconds. opt, _ := ask.Option(OptionApprove) - b.publish(t, "mesh.seat."+asks.Seat+".event.decided."+Module, asks.Warrant{Ask: ask.ID, Asker: Module, + approved := b.publish(t, "mesh.seat."+asks.Seat+".event.decided."+Module, asks.Warrant{Ask: ask.ID, Asker: Module, Outcome: asks.OutcomeChosen, Option: OptionApprove, Label: opt.Label, Level: opt.Level, By: &asks.Person{Who: asks.Operator, Kind: "telegram", Verified: "user id verified"}, At: time.Now(), AskDigest: ask.Digest()}) - rt.until("the merge to be asked of the forge", 5*time.Second, func() bool { return len(rt.asksOf("gitea.gitea_merge_pull_request")) >= 1 }) merges := rt.asksOf("gitea.gitea_merge_pull_request") if len(merges) != 1 || merges[0].Body["head"] != head || merges[0].Body["number"] != float64(7) { - t.Fatalf("the merge was asked %+v; wanted once, for pull request 7 at the marked head", merges) + t.Fatalf("the merge was asked %+v; wanted once, for pull request 7 at the approved head", merges) } - if took := merges[0].At.Sub(commented); took > 5*time.Second { - t.Fatalf("the merge was asked %s after the ready comment's event; wanted within 5s", took) + if got := fmt.Sprint(merges[0].Body["approved_heads"]); got != "[novox/app@"+head+"]" { + t.Fatalf("the merge tool was handed the approved heads %s", got) + } + if took := merges[0].At.Sub(approved); took > 5*time.Second { + t.Fatalf("the merge was asked %s after the Approve's event; wanted within 5s", took) } else { - t.Logf("the merge was asked %s after the ready comment's event", took) - } - // The forge's open pull requests were read for the mark by the events, not by a poll: the label's and the comment's. - if got := len(rt.asksOf("gitea.gitea_open_pulls")) - reads; got < 2 || got > 3 { - t.Fatalf("the forge's open pull requests were read %d times for one mark", got) + t.Logf("the merge was asked %s after the Approve's event", took) } // Quiet follows: nothing asks the forge or the controller again with no event to say so. quiet := len(rt.asked) @@ -488,29 +472,3 @@ func TestAReadyMarkAndItsApproveAreMergedWithinSecondsOfTheirEventsOverARealBus( t.Fatalf("%d asks of the mesh in a quiet second and a half after the merge: something polls", more) } } - -// A ready mark on a pull request whose check nobody asked yet asks it at once, on the ready stream (the check's `ready`). -func TestAReadyMarkAsksTheCheckOfAPullRequestWithNoVerdictAsReady(t *testing.T) { - b := newTestBus(t) - rt := b.start(t) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - rt.deliverEvents(ctx) - rt.until("the module to subscribe", 20*time.Second, func() bool { - rt.mu.Lock() - defer rt.mu.Unlock() - return len(rt.subs) >= len(followed)+1 - }) - const head = "2222222222222222222222222222222222222222" - b.publish(t, "mesh.mod.gitea.event.pull.updated", pr("novox/app", 8, head, "fix/y", "src/a.go")) - rt.mu.Lock() - rt.openPulls = []OpenPull{markedPull("novox/app", 8, head, head)} - rt.mu.Unlock() - b.publish(t, "mesh.mod.gitea.event.forge.pull-request.commented", commentedReady(8, head)) - rt.until("the check to be asked", 5*time.Second, func() bool { return len(rt.asksOf("seat:"+ControllerSeat+".delivery-check")) >= 1 }) - body := rt.asksOf("seat:" + ControllerSeat + ".delivery-check")[0].Body - var members []Member - if err := json.Unmarshal([]byte(fmt.Sprint(body["members"])), &members); err != nil || len(members) != 1 || !members[0].Ready { - t.Fatalf("the check was asked for %v (%v); wanted one member with ready", body["members"], err) - } -} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events.go b/modules/mesh-delivery/cmd/mesh-delivery/events.go index 4528e972..5e49aa41 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/events.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/events.go @@ -1,36 +1,16 @@ package main import ( - "regexp" "slices" - "strconv" - "strings" "git.novox.be/novox/mesh-sdk/go/asks" ) -// Events that start a step (novox/hq ADR 0406 §1, issue 975). +// Events that start a step (novox/hq ADR 0406 §1, issue 975, ADR 0421 issue 1022). // -// This owner acts on events: a ready mark, a verdict, an Approve, a group's turn. The forge's label and comment events -// say that a pull request's ready mark may have changed; they do not say whether it counts. Which accounts' "ready at" -// comments count is the forge module's rule (its trusted setting ready-authors, the gitea module's readyMarkOf), so a -// label added or a "ready at" comment written makes this owner read the forge's open pull requests once, and the mark -// it reads there is the mark (seeMarks). The label taken off needs no read: it only clears. - -// ForgePullEvent is the forge module's `forge.pull-request.labeled`, `.unlabeled` and `.commented` -// (modules/gitea/webhook.ts): the pull request, and the label added or taken off, or the comment written. -type ForgePullEvent struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - Number int `json:"number"` - HeadSHA string `json:"head_sha,omitempty"` - Base string `json:"base,omitempty"` - Labels []string `json:"labels,omitempty"` - Label string `json:"label,omitempty"` - Author string `json:"author,omitempty"` - Comment string `json:"comment,omitempty"` - CommentID int `json:"comment_id,omitempty"` -} +// This owner acts on events: a verdict, a commit status, an Approve. The operator is asked within the event that makes +// the Approve possible (the checks passing), with no label or comment written by anyone: the forge's label and comment +// events start nothing here. // ForgeStatusEvent is the forge module's `forge.status`: a commit's status was set. type ForgeStatusEvent struct { @@ -41,15 +21,7 @@ type ForgeStatusEvent struct { State string `json:"state"` } -// readyAtComment is a comment that names the head a pull request is marked ready at, as the forge module reads it -// (modules/gitea/client.ts READY_AT): "ready at " on a line of its own. -var readyAtComment = regexp.MustCompile(`(?im)^\s*ready at ([0-9a-f]{40}|[0-9a-f]{64})\b`) - -func eventPullKey(e ForgePullEvent) string { - return strings.ToLower(e.Owner+"/"+e.Repo) + "#" + strconv.Itoa(e.Number) -} - -// wantForge says the forge's open pull requests are to be read at the next look: a ready mark may have been made. +// wantForge says the forge's open pull requests are to be read at the next look: a check may have answered. func (h *Holder) wantForge() { h.mu.Lock() h.forgeWanted = true @@ -65,33 +37,6 @@ func (h *Holder) takeForgeWanted() bool { return wanted } -// PullLabeled takes a label added to a pull request: the ready label asks for the mark to be read. -func (h *Holder) PullLabeled(e ForgePullEvent) { - if e.Label == ReadyLabel { - h.wantForge() - } -} - -// PullUnlabeled takes a label taken off a pull request: the ready label clears its mark, with no read. -func (h *Holder) PullUnlabeled(e ForgePullEvent) { - if e.Label != ReadyLabel { - return - } - h.mu.Lock() - defer h.mu.Unlock() - if _, marked := h.marks[eventPullKey(e)]; marked { - delete(h.marks, eventPullKey(e)) - h.Logf("[mesh-delivery] %s's ready label was taken off: its mark is cleared", eventPullKey(e)) - } -} - -// PullCommented takes a comment on a pull request: one that names a head to be ready at asks for the mark to be read. -func (h *Holder) PullCommented(e ForgePullEvent) { - if readyAtComment.MatchString(e.Comment) { - h.wantForge() - } -} - // AskDecided takes the router's `decided` event for an ask this owner made: its warrant is applied to the deliveries // the ask covers, and what is said on their pull requests is said. func (h *Holder) AskDecided(id, state string, w *asks.Warrant) { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/events_test.go b/modules/mesh-delivery/cmd/mesh-delivery/events_test.go index 1025bc00..ef627715 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/events_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/events_test.go @@ -2,19 +2,15 @@ package main import ( "encoding/json" + "strings" "testing" stdio "git.novox.be/novox/mesh-sdk/go" ) -// **A ready mark, a verdict and an Approve are acted on as events** (novox/hq ADR 0406 §1, issue 975): the merge -// is asked within the event that makes it possible, never at the next tick or the next minute. - -const ( - labeledEvent = "gitea.forge.pull-request.labeled" - unlabeledEvent = "gitea.forge.pull-request.unlabeled" - commentedEvent = "gitea.forge.pull-request.commented" -) +// **A verdict and an Approve are acted on as events** (novox/hq ADR 0406 §1, issue 975, ADR 0421 §1, issue 1022): the +// operator is asked within the event that makes the Approve possible, and the merge within the Approve's event, never at +// the next tick or the next minute, and with no label or comment written by anyone. // hear is the runtime handing this owner one event, as handle takes it. func hear(t *testing.T, w *world, key string, body any) { @@ -29,76 +25,40 @@ func hear(t *testing.T, w *world, key string, body any) { w.h.React() // the look the event pokes, taken by the holder's worker } -func labeled(number int, label string) ForgePullEvent { - return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, HeadSHA: readyHead, Base: "main", Label: label, - Labels: []string{label}} -} - -func commentedReady(number int, head string) ForgePullEvent { - return ForgePullEvent{Owner: "novox", Repo: "app", Number: number, Author: "reviewer", CommentID: 5, - Comment: "ready at " + head + "\n\napproved: the tests are the change's"} -} - -// greenAnnounced is a pull request into main, announced and judged green, the operator not answering by himself. -func greenAnnounced(t *testing.T) *world { +// greenAnnounced is a pull request into main, announced, the operator not answering by himself. +func announced(t *testing.T) *world { w := newWorld(t) w.op.auto = false w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) return w } -func TestALabelAndAReadyAtCommentAskTheOperatorAtOnceWithNoTick(t *testing.T) { - w := greenAnnounced(t) - // The label first: the forge reads it without a comment naming a head yet. Nothing is asked. - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")} - hear(t, w, labeledEvent, labeled(7, ReadyLabel)) - if w.op.count() != 0 || len(w.forge.merges) != 0 { - t.Fatalf("a label with no head named was acted on: %d asks, merges %v", w.op.count(), w.forge.merges) +// Issue 1022, design 47 acceptance test 6: the Approve is asked within the event of the checks passing, with no ready +// mark, label or comment written by anyone. +func TestAPassingVerdictAsksTheOperatorAtOnceWithNoMark(t *testing.T) { + w := announced(t) + if w.op.count() != 0 { + t.Fatalf("asked before the checks passed: %d", w.op.count()) } - // The comment naming the head: the forge now reads the mark whole, and the ask is made within this event. - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - hear(t, w, commentedEvent, commentedReady(7, readyHead)) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) if w.op.count() != 1 { - t.Fatalf("the comment that completes the mark asked the operator %d times, wanted once, with no tick", w.op.count()) + t.Fatalf("a pull request whose checks passed asked the operator %d times, wanted once, with no mark", w.op.count()) } - if d := w.delivery(IDOf("novox/app", readyHead)); d.Ready == nil || d.Ready.By != "reviewer" || d.Ready.AskID == "" { - t.Fatalf("the mark is not kept on the delivery: %+v", d.Ready) + if len(w.forge.comments) != 0 || len(w.forge.calls) != 0 { + t.Fatalf("something was written on the forge to ask: %v", w.forge.calls) + } + // Asked once for the head, however often it is looked at again. + hear(t, w, "gitea.forge.status", ForgeStatusEvent{Owner: "novox", Repo: "app", SHA: readyHead, Context: "mesh/merge-gate", State: "success"}) + if w.op.count() != 1 { + t.Fatalf("asked %d times for one head", w.op.count()) } } -func TestACommentThatIsNotAReadyMarkReadsNothingFromTheForge(t *testing.T) { - w := greenAnnounced(t) - before := w.forge.openCalls - c := commentedReady(7, readyHead) - c.Comment = "looks fine to me, though I would not say ready at all" - hear(t, w, commentedEvent, c) - hear(t, w, labeledEvent, labeled(7, "kind/bug")) - if w.forge.openCalls != before { - t.Fatalf("the forge's open pull requests were read %d times for events that are no ready mark", w.forge.openCalls-before) - } -} - -func TestTheReadyLabelTakenOffClearsTheMarkAtOnce(t *testing.T) { - w := greenAnnounced(t) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - hear(t, w, labeledEvent, labeled(7, ReadyLabel)) - w.h.mu.Lock() - marks := len(w.h.marks) - w.h.mu.Unlock() - if marks != 1 { - t.Fatalf("the label did not make a mark: %d", marks) - } - asked := w.op.count() - // The forge no longer shows the label; the event alone says so, and the forge is not read for it. - reads := w.forge.openCalls - w.forge.open = nil - hear(t, w, unlabeledEvent, labeled(7, ReadyLabel)) - w.h.mu.Lock() - marks = len(w.h.marks) - w.h.mu.Unlock() - if marks != 0 || w.forge.openCalls != reads || w.op.count() != asked { - t.Fatalf("the mark was not cleared by the event alone: marks %d, reads %d, asks %d", marks, w.forge.openCalls-reads, w.op.count()-asked) +func TestAFailedVerdictAsksNothing(t *testing.T) { + w := announced(t) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "fail", "t1t1t1t1t1t1")) + if w.op.count() != 0 { + t.Fatalf("a pull request whose checks failed asked the operator %d times", w.op.count()) } } @@ -111,43 +71,82 @@ func approve(w *world) { w.h.React() } -func TestAnApproveHeardAsAnEventAsksTheMergeAtOnce(t *testing.T) { - w := greenAnnounced(t) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - hear(t, w, commentedEvent, commentedReady(7, readyHead)) - open := w.op.open() - if len(open) != 1 || len(w.forge.merges) != 0 { - t.Fatalf("asks %v, merges %v", open, w.forge.merges) +func TestAnApproveHeardAsAnEventMergesAtOnceAtTheApprovedHead(t *testing.T) { + w := announced(t) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + if len(w.op.open()) != 1 || len(w.forge.merges) != 0 { + t.Fatalf("asks %v, merges %v", w.op.open(), w.forge.merges) } approve(w) if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/app#7@"+readyHead { - t.Fatalf("the Approve, heard as an event, merged %v; wanted once at the marked head", w.forge.merges) + t.Fatalf("the Approve, heard as an event, merged %v; wanted once at the approved head", w.forge.merges) + } + if got := w.forge.approvedHeads[0]; len(got) != 1 || got[0] != "novox/app@"+readyHead { + t.Fatalf("the merge tool was not handed the approved heads: %v", got) } } -func TestAPassingVerdictAsksTheMergeAtOnceEvenJustAfterARefusal(t *testing.T) { - w := newWorld(t) - id := IDOf("novox/app", readyHead) - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) - // Another merge moves the trunk: the verdict is out of date and the merge tool refuses, asking the check again. +// Issue 1022 (ADR 0421 §3): a passing verdict that arrives out of date is not asked; its check against the trunk's +// head is, and the Approve is asked on the passing verdict. +func TestAStalePassingVerdictAsksTheCheckAgainBeforeTheOperator(t *testing.T) { + w := announced(t) + // Another merge moved the trunk before the verdict, judged against the trunk before, arrived. w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + if w.op.count() != 0 { + t.Fatalf("the operator was asked for a verdict out of date") + } + if len(w.ctl.checks) != 1 || w.ctl.baseSHAs[0] != "t2t2t2t2t2t2" { + t.Fatalf("its check against the trunk's head was not asked once: %v %v", w.ctl.checks, w.ctl.baseSHAs) + } + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) + if w.op.count() != 1 { + t.Fatalf("the passing verdict at the trunk's head asked the operator %d times, wanted once", w.op.count()) + } + if len(w.ctl.checks) != 1 { + t.Fatalf("its check was asked again: %v", w.ctl.checks) + } +} + +// Issue 1022 (ADR 0421 §3, §4): an ask raised stays when the trunk moves; the Approve is given; the merge waits for the +// fresh verdict and lands on it; no second Approve is asked. +func TestAnAskRaisedStaysThroughATrunkMoveAndTheMergeLandsOnTheFreshVerdict(t *testing.T) { + w := announced(t) + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) + w.h.React() + if w.state(id) != Proposed || w.op.count() != 1 || len(w.op.open()) != 1 { + t.Fatalf("after the trunk moved: %s, asks %d, open %v", w.state(id), w.op.count(), w.op.open()) + } + if len(w.ctl.checks) != 1 { + t.Fatalf("its check against the trunk's head was not asked at once: %v", w.ctl.checks) + } + approve(w) + if len(w.forge.merges) != 0 { + t.Fatalf("merged on a verdict out of date: %v", w.forge.merges) + } + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) + if len(w.forge.merges) != 1 || w.delivery(id).Ready.Merged == nil || w.op.count() != 1 { + t.Fatalf("the passing verdict did not ask the merge at once, on the same Approve: %v %+v asks %d", w.forge.merges, + w.delivery(id).Ready, w.op.count()) + } +} + +// Issue 1022: an Approve whose merge the tool refused is asked again at once on the verdict that decides it. +func TestAPassingVerdictAsksTheMergeAtOnceEvenJustAfterARefusal(t *testing.T) { + w := announced(t) + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) w.forge.refuse = errAskedAgain - w.op.auto = false - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - hear(t, w, commentedEvent, commentedReady(7, readyHead)) approve(w) if len(w.forge.merges) != 1 { - t.Fatalf("the merge tool was asked %v; wanted once to judge it again", w.forge.merges) + t.Fatalf("the merge tool was asked %v; wanted once", w.forge.merges) } - // The verdict against the trunk's head passes: merged within the verdict's own event, no minute waited. w.forge.refuse = nil + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - raw, _ := json.Marshal(judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) - if err := handle(w.h, stdio.Envelope{Key: ControllerSeat + ".checked", Body: raw}); err != nil { - t.Fatal(err) - } - w.h.React() + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) if len(w.forge.merges) != 2 || w.delivery(id).Ready.Merged == nil { t.Fatalf("the passing verdict did not ask the merge at once: %v %+v", w.forge.merges, w.delivery(id).Ready) } @@ -160,3 +159,108 @@ type askedAgainError struct{} func (*askedAgainError) Error() string { return "the forge refused gitea_merge_pull_request: The check of head merged into trunk was asked" } + +// Issue 1022 (ADR 0421 §5): the review record is read and put in the ask; it is never a gate. +func TestTheAskSaysFirstWhenThePullRequestIsNotReviewedAtThisHead(t *testing.T) { + for _, c := range []struct{ name, body, want string }{ + {"no line", "Fixes issue 7.", "not reviewed at this head"}, + {"another head", "Reviewed-by: lead@shanks at " + pushed, "the Reviewed-by line names " + pushed[:12]}, + } { + t.Run(c.name, func(t *testing.T) { + w := newWorld(t) + w.op.auto = false + p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") + p.Body = c.body + w.h.PullUpdated(p) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + if w.op.count() != 1 { + t.Fatalf("an unreviewed pull request must still be asked: %d", w.op.count()) + } + a := w.op.ask(w.op.open()[0]) + first := strings.SplitN(a.Explanation, "\n", 2)[0] + if !strings.Contains(first, "not reviewed at this head") || !strings.Contains(a.Explanation, c.want) { + t.Fatalf("the ask does not say first that it is not reviewed at this head:\n%s", a.Explanation) + } + }) + } +} + +func TestTheAskNamesTheReviewerOfTheHead(t *testing.T) { + w := newWorld(t) + w.op.auto = false + p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") + p.Body = "Fixes issue 7.\n\nReviewed-by: lead@shanks at " + readyHead + "\n" + w.h.PullUpdated(p) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + a := w.op.ask(w.op.open()[0]) + if strings.Contains(a.Explanation, "not reviewed") || !strings.Contains(a.Explanation, "reviewed by lead") { + t.Fatalf("a reviewed head is not said reviewed:\n%s", a.Explanation) + } +} + +// The description corrected without a push is read again. +func TestTheReviewLineCorrectedInTheDescriptionIsReadWithoutAPush(t *testing.T) { + w := newWorld(t) + w.op.auto = false + p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") + w.h.PullUpdated(p) + p.Body = "Reviewed-by: lead@shanks at " + readyHead + w.h.PullUpdated(p) + if d := w.delivery(IDOf("novox/app", readyHead)); d.ReviewedBy != "lead@shanks" || d.ReviewedAt != readyHead { + t.Fatalf("the corrected line is not read: %q %q", d.ReviewedBy, d.ReviewedAt) + } +} + +// Issue 1022 (ADR 0421 §6): a push ends the Approve and says so once on the pull request. +func TestAPushAfterTheAskEndsItAndSaysSoOnceOnThePullRequest(t *testing.T) { + w := announced(t) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + old := w.op.open()[0] + w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) + w.h.React() + w.h.React() + if len(w.op.cancelled) != 1 || w.op.cancelled[0] != old || len(w.op.open()) != 0 { + t.Fatalf("the ask was not cancelled: %v, open %v", w.op.cancelled, w.op.open()) + } + if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "Approve") || + !strings.Contains(w.forge.comments[0], "ended") { + t.Fatalf("the end of the Approve is not said once on the pull request: %v", w.forge.comments) + } + // The new head is asked on its own when its checks pass. + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, pushed, "pass", "t1t1t1t1t1t1")) + if open := w.op.open(); len(open) != 1 || open[0] == old { + t.Fatalf("the new head was not asked afresh: %v", open) + } + if len(w.forge.comments) != 1 { + t.Fatalf("said more than once: %v", w.forge.comments) + } +} + +// Issue 1022 (ADR 0421 §7): a check that failed after the ask merges nothing and is said once and as a stalled line. +func TestACheckThatFailsAfterTheAskIsNotMergedAndIsSaid(t *testing.T) { + w := announced(t) + id := IDOf("novox/app", readyHead) + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + if _, err := w.h.Recheck(id, "a second look", "tester"); err != nil { + t.Fatal(err) + } + hear(t, w, ControllerSeat+".checked", judgedAt("novox/app", 7, readyHead, "fail", "t1t1t1t1t1t1")) + approve(w) + if len(w.forge.merges) != 0 { + t.Fatalf("merged past a failed check: %v", w.forge.merges) + } + w.h.React() + said := 0 + for _, c := range w.forge.comments { + if strings.Contains(c, "not merged") { + said++ + } + } + if said != 1 { + t.Fatalf("the failure is not said once: %v", w.forge.comments) + } + lines := w.h.Stalled() + if len(lines) != 1 || lines[0].ID != id || strings.Contains(lines[0].Says, "marked") { + t.Fatalf("the failure is not a stalled line: %+v", lines) + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go index 1c2a9a94..8e2b9a2a 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/fakes_test.go @@ -110,6 +110,8 @@ type fakeController struct { walkReads int // checkReady is the ready flag of each member of each check asked. checkReady []bool + // baseSHAs is the base head each member of each check asked was sent with. + baseSHAs []string // refuseDeliver refuses every word, as a controller that will not take it does (review of novox/hq issue 753). refuseDeliver bool // onCheck runs as a check is asked, before it is answered: what the forge held at that moment. @@ -182,6 +184,7 @@ func (c *fakeController) Check(group string, ms []Member) (string, error) { c.checks = append(c.checks, group+":"+strings.Join(heads, ",")) for _, m := range ms { c.checkReady = append(c.checkReady, m.Ready) + c.baseSHAs = append(c.baseSHAs, m.BaseSHA) } return id, nil } @@ -410,12 +413,13 @@ type fakeForge struct { open []OpenPull openDown bool openCalls int - // merges, comments and unlabels are what was asked of the forge for ready marks (novox/hq issue 722). - merges, comments, unlabels []string - refuse error - // calls are the labels and comments in the order asked (novox/hq issue 913); labelErr fails a Label. - calls []string - labelErr error + // merges and comments are what was asked of the forge for an approved merge (novox/hq issue 722); approvedHeads the + // heads each merge was handed as approved (ADR 0421 §9). + merges, comments []string + approvedHeads [][]string + refuse error + // calls are the comments in the order asked. + calls []string } func (f *fakeForge) OpenPulls() ([]OpenPull, error) { @@ -690,11 +694,12 @@ func aWalk(id, repo, commit string, waits bool) Walk { return w } -// Merge, Comment and Unlabel record what was asked (novox/hq issue 722); Merge answers refuse when set. -func (f *fakeForge) Merge(owner, repo string, number int, head string) error { +// Merge and Comment record what was asked (novox/hq issue 722); Merge answers refuse when set. +func (f *fakeForge) Merge(owner, repo string, number int, head string, approved []string) error { f.mu.Lock() 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) return f.refuse } @@ -705,20 +710,3 @@ func (f *fakeForge) Comment(owner, repo string, number int, body string) error { f.calls = append(f.calls, "comment "+body) return nil } - -func (f *fakeForge) Label(owner, repo string, number int, name string) error { - f.mu.Lock() - defer f.mu.Unlock() - if f.labelErr != nil { - return f.labelErr - } - f.calls = append(f.calls, fmt.Sprintf("label %s/%s#%d %s", owner, repo, number, name)) - return nil -} - -func (f *fakeForge) Unlabel(owner, repo string, number, label int) error { - f.mu.Lock() - defer f.mu.Unlock() - f.unlabels = append(f.unlabels, fmt.Sprintf("%s/%s#%d-%d", owner, repo, number, label)) - return nil -} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/holder.go b/modules/mesh-delivery/cmd/mesh-delivery/holder.go index 6a69eb1d..04216762 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/holder.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/holder.go @@ -84,10 +84,8 @@ type Holder struct { coverAskedSince map[string]time.Time // tried counts the owed effects Flush tried since this owner started, by how each ended (novox/hq issue 407). tried owedTries - // marks are the open pull requests read with a ready mark, by owner/repo#number (novox/hq issue 722, ready.go). - marks map[string]*marked - // forgeWanted is whether the forge's open pull requests are to be read at the next React: a ready mark may have been - // made (events.go). reactMu keeps one React at a time. + // forgeWanted is whether the forge's open pull requests are to be read at the next React: a check may have + // answered (events.go). reactMu keeps one React at a time. forgeWanted bool // rereadWalks is whether the controller's walks are to be read once more: a merge passed the bound a walk opens // within, and the walks were last read before it (unwalkedPast). @@ -148,7 +146,6 @@ func (h *Holder) init() { h.askTried = map[string]time.Time{} h.walkAsked = map[string]time.Time{} h.unchecked = map[string]unchecked{} - h.marks = map[string]*marked{} h.walks = map[string]seenWalk{} h.orphans = map[string]time.Time{} h.asking = map[string]bool{} @@ -342,6 +339,11 @@ func (h *Holder) PullUpdated(p PullEvent) (isNew bool) { d.BaseSHA = p.BaseSHA h.keep(d) } + // The review record, corrected in the description without a push, is read again (ADR 0421 §5). + if by, at := ReviewIn(p.Body); p.Body != "" && (by != d.ReviewedBy || at != d.ReviewedAt) { + d.ReviewedBy, d.ReviewedAt = by, at + h.keep(d) + } if d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil { d.AskedAgain = &now h.keep(d) @@ -403,6 +405,7 @@ func (h *Holder) freshAnnouncement(p PullEvent, now time.Time, startedOver strin Title: p.Title, Base: p.Base, Branch: p.Head, HTMLURL: p.HTMLURL, CloneURL: p.CloneURL, Paths: p.Paths, PathsTruncated: p.PathsTruncated, Removed: p.Removed, ModuleDirs: p.ModuleDirs, ModuleDirsSaid: p.ModuleDirsSaid, After: AfterIn(p.Body), Issues: IssuesIn(p.Title + "\n" + p.Body), Created: now, BaseSHA: p.BaseSHA} + d.ReviewedBy, d.ReviewedAt = ReviewIn(p.Body) why := fmt.Sprintf("%s#%d's head announced", d.Repository, d.Number) if startedOver != "" { why = fmt.Sprintf("%s; %s", why, startedOver) @@ -526,7 +529,6 @@ func (h *Holder) PullMerged(p PullEvent) { } h.mu.Lock() defer h.mu.Unlock() - delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a merged pull request keeps no mark h.forgetUnchecked(p.Owner, p.Repo, p.Number) var d *Delivery if p.HeadSHA != "" { @@ -689,7 +691,6 @@ func verdictOfState(s string) string { // go of it within the week anyway. func (h *Holder) PullClosed(p PullEvent) { h.mu.Lock() - delete(h.marks, strings.ToLower(p.Owner+"/"+p.Repo)+fmt.Sprintf("#%d", p.Number)) // a closed pull request keeps no mark h.forgetUnchecked(p.Owner, p.Repo, p.Number) for _, d := range h.deliveries { if d.Repository == p.repository() && d.Number == p.Number && !d.State.Final() && d.MergedAs == "" { @@ -828,7 +829,9 @@ func (h *Holder) Checked(c CheckedEvent) { d.Plan = c.Plan } if why := d.outOfDate(); why != "" { - // Kept and shown, and it does not count: the delivery stays proposed (ADR 0323 §7). + // Kept and shown, and it does not count: the delivery stays proposed (ADR 0323 §7). If it passes, its check is + // asked against the trunk's head before the operator is (ADR 0421 §3, askFresh). + d.FreshWanted = d.Check.Passes() h.Logf("[mesh-delivery] %s: its verdict %s is %s", d.ID, c.ID, why) } h.settleAndKeep(d) @@ -1710,7 +1713,7 @@ func (h *Holder) askCheck(d *Delivery, now time.Time) func() { if d.State != Proposed || d.Check != nil || d.CheckAsk != nil || d.MergedAs != "" || d.Number == 0 { return nil } - if now.Sub(d.Since) < checkGrace && d.Ready == nil { // a ready mark makes the check worth asking at once + if now.Sub(d.Since) < checkGrace && d.Ready == nil { // an Approve asked makes the check worth asking at once h.lookAt(d.Since.Add(checkGrace), "a proposed delivery's check, asked once the grace has passed with no verdict") return nil } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go b/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go index 1b6e5352..e335d52f 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/issue341_test.go @@ -215,9 +215,13 @@ func TestAGroupsComposedCheckGoesOutOfDateOnAMergeThatIsNotItsMember(t *testing. t.Fatalf("mesh/delivery-group on %s says %q, not pending with the reason", head, s) } } - if len(w.ctl.checks) != asked { - t.Fatalf("the trunk's move asked a check: %v", w.ctl.checks[asked:]) + // The group's Approve stays; each member whose verdict went out of date has its check asked at once, against the + // trunk's head (ADR 0421 §3, issue 1047): only the member whose trunk moved, and the composed check is asked + // after it. + if got := w.ctl.checks[asked:]; len(got) != 1 || got[0] != ":"+idc { + t.Fatalf("the trunk's move asked %v; wanted the moved member's own check once", got) } + asked++ // The controller's member judged again against the trunk's head: the composed check runs again. w.later(time.Minute) w.h.Checked(judged(ctl, 1, hc, "pass", merge1)) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/main.go b/modules/mesh-delivery/cmd/mesh-delivery/main.go index a2966be8..4379330c 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/main.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/main.go @@ -32,9 +32,8 @@ func (l *listening) set(s string) { l.mu.Lock(); l.now = s; l.mu.Unlock() } func (l *listening) get() string { l.mu.Lock(); defer l.mu.Unlock(); return l.now } // The events this owner follows: the forge's pull requests, and the controller's verdicts and walks. -// The forge's label and comment events say a ready mark changed, its status events that a check answered (events.go). +// The forge's status events say that a check answered (events.go). var followed = []string{"gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", - "gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented", "gitea.forge.status", ControllerSeat + ".checked", ControllerSeat + ".plan-moved"} func main() { @@ -150,20 +149,6 @@ func take(h *Holder, e stdio.Envelope) error { default: h.PullClosed(p) } - case "gitea.forge.pull-request.labeled", "gitea.forge.pull-request.unlabeled", "gitea.forge.pull-request.commented": - var p ForgePullEvent - if err := json.Unmarshal(e.Body, &p); err != nil { - h.Refuse(fmt.Errorf("%s is not readable: %v", e.Key, err)) - return nil - } - switch e.Key { - case "gitea.forge.pull-request.labeled": - h.PullLabeled(p) - case "gitea.forge.pull-request.unlabeled": - h.PullUnlabeled(p) - default: - h.PullCommented(p) - } case "gitea.forge.status": var s ForgeStatusEvent if err := json.Unmarshal(e.Body, &s); err != nil { @@ -404,20 +389,6 @@ func tools(h *Holder, l *listening) []stdio.Tool { "and tried again, given up) and when the last was, walks with no delivery yet, whether its events arrive, when " + "the controller's walks were last read or why they could not be, and the last refusals.", Run: func(map[string]any) (any, error) { return h.Status(l.get()), nil }}, - {Name: seat + "mark-ready", - Description: "A reviewer's word that a delivery's head is approved: the mesh's own forge account puts the " + - "label mesh/ready on its pull request and then writes \"ready at \", the order the forge counts. " + - "Refused when the delivery's commit is no longer its pull request's head. mesh-delivery then merges it " + - "once its checks pass (novox/hq ADR 0364, issue 913).", - Input: map[string]any{"type": "object", "properties": map[string]any{"id": str("the delivery's id (owner/repository@commit): the head reviewed"), - "reviewed-by": str("who reviewed it, as who@node"), "why": str("the review's verdict in a line")}, - "required": []string{"id", "reviewed-by", "why"}}, - Run: func(a map[string]any) (any, error) { - if err := need(a, "id", "reviewed-by", "why"); err != nil { - return nil, err - } - return h.MarkReady(strArg(a, "id"), strArg(a, "reviewed-by"), strArg(a, "why")) - }}, } // A verb that changes a delivery asks for a look at what it makes possible (react.go). for i := range list { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go index 7093e435..4bbe714e 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/manifest_test.go @@ -8,6 +8,7 @@ import ( "sort" "strings" "testing" + "time" ) // The manifest says what the code does: the seat it claims and the verbs it serves, the events it follows @@ -68,7 +69,7 @@ func TestItClaimsTheDeliverySeatAndFollowsWhatItHandles(t *testing.T) { want = append(want, "gitea.gitea_note_append", "gitea.gitea_delivery_view", "gitea.gitea_commit_status", "gitea.gitea_commit_statuses", "gitea.gitea_contains", "gitea.gitea_open_pulls", // The merge on a ready mark (novox/hq issue 722): the merge tool, a comment, a label taken off. - "gitea.gitea_merge_pull_request", "gitea.gitea_add_comment", "gitea.gitea_api") + "gitea.gitea_merge_pull_request", "gitea.gitea_add_comment") // A merge into main asks the operator for the Approve (novox/hq ADR 0398 §3, issue 948), as the knowledge base asks. want = append(want, "seat:operator-channel.notify") if !reflect.DeepEqual(m.Uses, []string{"operator-channel"}) { @@ -128,3 +129,49 @@ func TestAnAnswerIsReadWhateverWrapsIt(t *testing.T) { } } } + +// **No ready mark is left in the module** (novox/hq ADR 0421 §4, issue 1022; how it is checked): no label, comment, verb +// or trusted setting of the mark stands in any of its code or its manifest. Tests are left out: they name the words to +// prove them gone. +func TestNoReadyMarkIsLeftInTheModule(t *testing.T) { + forbidden := []string{"mesh/ready", "ready at", "ready-authors", "mark-ready", "ReadyLabel", "pull-request.labeled", + "pull-request.unlabeled", "pull-request.commented"} + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + files = append(files, filepath.Join("..", "..", "module.json")) + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + raw, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + for _, word := range forbidden { + if strings.Contains(string(raw), word) { + t.Errorf("%s still holds %q: the ready mark is gone (ADR 0421 §4)", f, word) + } + } + } +} + +// A row written while the ready mark stood still reads: its mark's fields are ignored, its ask and merge kept (ADR 0421 +// §4 consequences, issue 1022). +func TestARowWrittenWithTheReadyMarkStillReads(t *testing.T) { + store := newMemStore() + row := `{"id":"novox/app@111111111111","repository":"novox/app","commit":"` + readyHead + `","number":7,"base":"main", +"state":"ready","since":"2026-10-17T10:00:00Z","created":"2026-10-17T10:00:00Z","let_go":"0001-01-01T00:00:00Z", +"ready":{"head":"` + readyHead + `","by":"lead@shanks","marked_at":"2026-10-17T10:01:00Z","seen":"2026-10-17T10:02:00Z", +"last_try":"0001-01-01T00:00:00Z","asked_at":"2026-10-17T10:03:00Z","ask_id":"merge-7-111111111111-1","heads":{"novox/app@111111111111":"` + readyHead + `"}}}` + store.deliveries["novox/app@111111111111"] = []byte(row) + w := newWorldOver(t, store, newFakeController(), newFakeForge(), time.Date(2026, 10, 17, 11, 0, 0, 0, time.UTC)) + d := w.delivery("novox/app@111111111111") + if d.Ready == nil || d.Ready.AskID != "merge-7-111111111111-1" || d.Ready.Head != readyHead { + t.Fatalf("the old row is not read: %+v", d.Ready) + } + if !MemberOf(d).Ready { + t.Fatalf("an ask waiting is a ready member") + } +} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go deleted file mode 100644 index f8e13b52..00000000 --- a/modules/mesh-delivery/cmd/mesh-delivery/markready_test.go +++ /dev/null @@ -1,143 +0,0 @@ -package main - -import ( - "errors" - "strings" - "testing" - "time" -) - -// **A reviewer's word marks the pull request ready in the order the forge counts** (novox/hq issue 913): the label, -// then the comment "ready at ". Written the other way round the mark's head is empty (issue 873). - -const fullHead = "3333333333333333333333333333333333333333" - -// readyDelivery is a delivery of a pull request whose head is fullHead, its check passed, and the forge's open pull. -func readyDelivery(t *testing.T, w *world) string { - t.Helper() - w.h.PullUpdated(pr("novox/app", 7, fullHead, "feat/x", "src/a.go")) - w.h.Checked(verdict("novox/app", 7, fullHead, "pass")) - w.settleAll() - w.forge.open = []OpenPull{{Owner: "novox", Repo: "app", Number: 7, Base: "main", HeadSHA: fullHead}} - return IDOf("novox/app", fullHead) -} - -func TestMarkReadyLabelsThenCommentsTheFullHead(t *testing.T) { - w := newWorld(t) - id := readyDelivery(t, w) - answer, err := w.h.MarkReady(id, "fix-913@shanks", "the change is right") - if err != nil { - t.Fatal(err) - } - if len(w.forge.calls) != 2 || w.forge.calls[0] != "label novox/app#7 mesh/ready" { - t.Fatalf("the forge was asked %q; wanted the label first", w.forge.calls) - } - c := strings.TrimPrefix(w.forge.calls[1], "comment ") - if first, _, _ := strings.Cut(c, "\n"); first != "ready at "+fullHead || !strings.Contains(c, "\n\nReviewed by fix-913@shanks: the change is right.") || - !strings.Contains(c, "mark-ready") { - t.Fatalf("the comment is %q", c) - } - d := w.delivery(id) - if d.Ready == nil || d.Ready.Head != fullHead || d.Ready.By != "fix-913@shanks" || d.Ready.Seen.IsZero() { - t.Fatalf("the mark is not kept: %+v", d.Ready) - } - if !strings.Contains(answer, "marked") { - t.Fatalf("answer %q", answer) - } -} - -func TestMarkReadyRefusesAMovedHeadAndAsksTheForgeNothing(t *testing.T) { - w := newWorld(t) - id := readyDelivery(t, w) - w.forge.open[0].HeadSHA = pushed - _, err := w.h.MarkReady(id, "fix-913@shanks", "ok") - if err == nil || !strings.Contains(err.Error(), "the head is now "+pushed) { - t.Fatalf("a moved head: %v", err) - } - if len(w.forge.calls) != 0 { - t.Fatalf("the forge was asked %q", w.forge.calls) - } -} - -func TestMarkReadyRefusesAnUnknownAndAFinalDelivery(t *testing.T) { - w := newWorld(t) - id := readyDelivery(t, w) - if _, err := w.h.MarkReady("novox/app@nothing", "a@b", "ok"); err == nil || !strings.Contains(err.Error(), "deliveries") { - t.Fatalf("unknown id: %v", err) - } - w.h.mu.Lock() - w.h.deliveries[id].State = Stopped - w.h.mu.Unlock() - if _, err := w.h.MarkReady(id, "a@b", "ok"); err == nil { - t.Fatal("a final delivery was marked") - } - if len(w.forge.calls) != 0 { - t.Fatalf("the forge was asked %q", w.forge.calls) - } -} - -func TestMarkReadyWritesNoCommentWhenTheLabelFails(t *testing.T) { - w := newWorld(t) - id := readyDelivery(t, w) - w.forge.labelErr = errors.New("the forge refused gitea_api: no such label") - _, err := w.h.MarkReady(id, "a@b", "ok") - if err == nil || !strings.Contains(err.Error(), "no such label") { - t.Fatalf("label failure: %v", err) - } - if len(w.forge.comments) != 0 { - t.Fatalf("commented: %v", w.forge.comments) - } - if w.delivery(id).Ready != nil { - t.Fatal("marked though the label failed") - } -} - -func TestClearMarkWithAnEmptyHeadSaysWhatWasWrongOnceItStayedEmpty(t *testing.T) { - w := newWorld(t) - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, "")} - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.comments) != 0 || len(w.forge.unlabels) != 0 { - t.Fatalf("an empty-head mark seen once was cleared: %v", w.forge.comments) - } - w.later(markGrace + time.Second) - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "no \"ready at\" comment by a listed account") || - !strings.Contains(w.forge.comments[0], "the head is now "+readyHead) || - !strings.Contains(w.forge.comments[0], "mesh-delivery.mark-ready") || strings.Contains(w.forge.comments[0], "pushed after it") { - t.Fatalf("comments: %v", w.forge.comments) - } -} - -func TestAMarkReadyJustMadeIsNotClearedByAReadThatPredatesItsComment(t *testing.T) { - w := newWorld(t) - id := readyDelivery(t, w) - if _, err := w.h.MarkReady(id, "a@b", "ok"); err != nil { - t.Fatal(err) - } - w.later(markGrace + time.Second) // the label alone was read long enough ago to count as stale... - w.forge.open = []OpenPull{markedPull("novox/app", 7, fullHead, "")} - w.h.SeeOpenPulls() - w.h.mu.Lock() - w.h.deliveries[id].Ready.Seen = w.now // ...but the mark was made just now - w.h.marks["novox/app#7"].firstEmpty = w.now.Add(-2 * markGrace) - w.h.mu.Unlock() - before := len(w.forge.comments) - w.settleAll() - if len(w.forge.unlabels) != 0 || len(w.forge.comments) != before { - t.Fatalf("a mark just made was cleared: %v %v", w.forge.unlabels, w.forge.comments) - } -} - -func TestClearMarkWithAnOlderHeadKeepsTheOldWording(t *testing.T) { - w := newWorld(t) - w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)} - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "pushed after it") { - t.Fatalf("comments: %v", w.forge.comments) - } -} diff --git a/modules/mesh-delivery/cmd/mesh-delivery/model.go b/modules/mesh-delivery/cmd/mesh-delivery/model.go index ef6631b1..e635616d 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/model.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/model.go @@ -120,8 +120,17 @@ type Delivery struct { LetGo time.Time `json:"let_go"` LetGoWhy string `json:"let_go_why,omitempty"` - // Ready is what this owner did with its pull request's ready mark (novox/hq issue 722, ready.go). + // Ready is what this owner did with the operator's Approve of its merge into main, and with the merge (novox/hq ADR + // 0421, ready.go). The json key is the one the ready mark's record had, so rows written before still read. Ready *ReadyRun `json:"ready,omitempty"` + // ReviewedBy and ReviewedAt are the review record its pull request's description holds: the line + // `Reviewed-by: at ` (novox/hq ADR 0421 §5, issue 1022), as written. Read at every head; never a + // gate: the ask says first when the line is missing or names another head. + ReviewedBy string `json:"reviewed_by,omitempty"` + ReviewedAt string `json:"reviewed_at,omitempty"` + // FreshWanted is whether a passing verdict that arrived out of date is to have its check asked against the + // trunk's head, before the operator is asked (novox/hq ADR 0421 §3, issue 1022). Cleared when it is asked. + FreshWanted bool `json:"fresh_wanted,omitempty"` Transitions []Transition `json:"transitions,omitempty"` // Owed are the forge's and the bus's effects of its transitions not yet done: said only after the @@ -418,6 +427,33 @@ func sameCommit(a, b string) bool { return len(a) >= 7 && strings.HasPrefix(strings.ToLower(b), strings.ToLower(a)) } +// reviewedLine is the review record of a pull request's description (novox/hq ADR 0421 §5, issue 1022): +// `Reviewed-by: at `, on a line of its own, who as who@node. +var reviewedLine = regexp.MustCompile(`(?im)^\s*Reviewed-by:\s*(\S+)\s+at\s+([0-9a-f]{40}|[0-9a-f]{64})\b`) + +// ReviewIn reads the review record of a pull request's description: who reviewed and the full sha named. The last such +// line counts; empty when there is none. +func ReviewIn(body string) (who, sha string) { + for _, m := range reviewedLine.FindAllStringSubmatch(body, -1) { + who, sha = m[1], strings.ToLower(m[2]) + } + return who, sha +} + +// reviewWords is what the delivery's review record says of its head, for the ask: reviewed by whom, or, first, that it +// is not reviewed at this head (never a gate, ADR 0421 §5). ok is whether it is reviewed at this head. +func reviewWords(d *Delivery) (words string, ok bool) { + switch { + case d.ReviewedBy == "": + return "not reviewed at this head: the description holds no Reviewed-by line", false + case !strings.EqualFold(d.ReviewedAt, d.Commit): + return fmt.Sprintf("not reviewed at this head: the Reviewed-by line names %s, the head is %s", + shortOf(d.ReviewedAt, 12), shortOf(d.Commit, 12)), false + } + // The router refuses an @ in an ask's words: who@node is said "who on node". + return "reviewed by " + strings.Replace(d.ReviewedBy, "@", " on ", 1) + " at " + shortOf(d.Commit, 12), true +} + // afterLines are the `after: ` lines of a pull request's description. var afterLines = regexp.MustCompile(`(?im)^\s*after:\s*([A-Za-z0-9_.\-/]+)\s*$`) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ports.go b/modules/mesh-delivery/cmd/mesh-delivery/ports.go index b884cf0b..cdad7701 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ports.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ports.go @@ -32,7 +32,7 @@ type Member struct { ModuleDirsSaid bool `json:"module_dirs_said,omitempty"` CloneURL string `json:"clone_url,omitempty"` After []string `json:"after,omitempty"` - // Ready is whether the pull request carries a ready mark: its check is published on the ready stream (ADR 0406 §3). + // Ready is whether the operator's Approve of its merge is asked or given: its check is published on the ready stream (ADR 0406 §3, 0421 §10). Ready bool `json:"ready,omitempty"` // BaseSHA is the base branch's head the forge last announced the head with (novox/hq ADR 0323): the trunk commit // the controller's verdict names when no build judges the head. @@ -41,7 +41,7 @@ type Member struct { // MemberOf is a delivery as a member. func MemberOf(d *Delivery) Member { - return Member{Ready: d.Ready != nil && d.Ready.Merged == nil, ID: d.ID, Repository: d.Repository, Base: d.Base, Head: d.Commit, Number: d.Number, Paths: d.Paths, + return Member{Ready: d.Ready != nil && d.Ready.Merged == nil && (d.Ready.AskID != "" || d.Ready.Approved != nil), ID: d.ID, Repository: d.Repository, Base: d.Base, Head: d.Commit, Number: d.Number, Paths: d.Paths, PathsTruncated: d.PathsTruncated, Removed: d.Removed, ModuleDirs: d.ModuleDirs, ModuleDirsSaid: d.ModuleDirsSaid, CloneURL: d.CloneURL, After: d.After, BaseSHA: d.BaseSHA} } @@ -94,14 +94,12 @@ type Forge interface { // 347). Reads only. OpenPulls() ([]OpenPull, error) // Merge asks the forge's merge tool to merge a pull request at the head named (gitea_merge_pull_request, novox/hq - // issue 722): the one path every merge takes. Its refusal is the error, in its words. - Merge(owner, repo string, number int, head string) error + // issue 722): the one path every merge takes. approved are the heads the operator's Approve covers, each + // `owner/repo@` (ADR 0421 §9); the tool refuses a pull request whose head differs. Its refusal is the + // error, in its words. + Merge(owner, repo string, number int, head string, approved []string) error // Comment adds a comment to a pull request. Comment(owner, repo string, number int, body string) error - // Label puts a label on a pull request, by the label's name (novox/hq issue 913). - Label(owner, repo string, number int, name string) error - // Unlabel takes a label off a pull request, by the label's id. - Unlabel(owner, repo string, number, label int) error } // ForgeChecks is what the forge says of a commit's checks (the gitea module's gitea_commit_statuses). @@ -403,60 +401,15 @@ func (f toolForge) Statuses(owner, repo string, number int, commit, base string) return &c, nil } -func (f toolForge) Merge(owner, repo string, number int, head string) error { - return f.tool("gitea_merge_pull_request", map[string]any{"owner": owner, "repo": repo, "number": number, "head": head}) +func (f toolForge) Merge(owner, repo string, number int, head string, approved []string) error { + return f.tool("gitea_merge_pull_request", map[string]any{"owner": owner, "repo": repo, "number": number, "head": head, + "approved_heads": approved}) } func (f toolForge) Comment(owner, repo string, number int, body string) error { return f.tool("gitea_add_comment", map[string]any{"owner": owner, "repo": repo, "number": number, "body": body}) } -// Label puts a label on a pull request. The forge's add-labels call takes label ids on every version (names only on -// the newer), so the id is read from the repository's labels by name first; a repository without the label is refused, -// not given one. -func (f toolForge) Label(owner, repo string, number int, name string) error { - id := 0 - for page := 1; page <= 20 && id == 0; page++ { - raw, err := f.ask("gitea.gitea_api", map[string]any{"method": "GET", - "path": fmt.Sprintf("/repos/%s/%s/labels?limit=50&page=%d", owner, repo, page)}) - if err != nil { - return err - } - answer, output, ok := answerOf(raw) - if !ok { - return fmt.Errorf("the forge refused gitea_api: %s", lastLine(output)) - } - var got struct { - Result []struct { - ID int `json:"id"` - Name string `json:"name"` - } `json:"result"` - } - if err := json.Unmarshal(answer, &got); err != nil { - return fmt.Errorf("the forge's labels are not readable: %s", clip(string(answer), 200)) - } - for _, l := range got.Result { - if l.Name == name { - id = l.ID - } - } - if len(got.Result) < 50 { - break - } - } - if id == 0 { - return fmt.Errorf("%s/%s has no label %s", owner, repo, name) - } - return f.tool("gitea_api", map[string]any{"method": "POST", - "path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels", owner, repo, number), - "body": map[string]any{"labels": []int{id}}}) -} - -func (f toolForge) Unlabel(owner, repo string, number, label int) error { - return f.tool("gitea_api", map[string]any{"method": "DELETE", - "path": fmt.Sprintf("/repos/%s/%s/issues/%d/labels/%d", owner, repo, number, label)}) -} - func (f toolForge) OpenPulls() ([]OpenPull, error) { raw, err := f.ask("gitea.gitea_open_pulls", map[string]any{}) if err != nil { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go index 37e951df..413b58a1 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/pullkey_test.go @@ -120,22 +120,26 @@ func TestALostRaceReadsTheKeyAgainAndAppliesTheChangeToWhatItFound(t *testing.T) } } -func TestAPullRequestMarkedReadyHasItsCheckAskedOnTheReadyStream(t *testing.T) { +// A check asked for a head whose Approve is asked is on the ready stream (ADR 0406 §3, ADR 0421 §10); one for a head +// with no Approve is asked as before. +func TestAPullRequestWithItsApproveAskedHasItsCheckAskedOnTheReadyStream(t *testing.T) { w := newWorld(t) + w.op.auto = false w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - w.h.wantForge() + w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) + w.settleAll() + w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) w.settleAll() if len(w.ctl.checkReady) == 0 || !w.ctl.checkReady[0] { - t.Fatalf("the check of a marked pull request was asked with ready %v", w.ctl.checkReady) + t.Fatalf("the check of a pull request whose Approve is asked was asked with ready %v", w.ctl.checkReady) } - // An unmarked one is asked as before. + // One with no Approve asked is asked as before. w2 := newWorld(t) w2.h.PullUpdated(pr("novox/app", 8, pushed, "feat/y", "src/a.go")) w2.later(checkGrace + 1) w2.settleAll() if len(w2.ctl.checkReady) == 0 || w2.ctl.checkReady[0] { - t.Fatalf("the check of an unmarked pull request was asked with ready %v", w2.ctl.checkReady) + t.Fatalf("the check of a pull request with no Approve asked was asked with ready %v", w2.ctl.checkReady) } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready.go b/modules/mesh-delivery/cmd/mesh-delivery/ready.go index 9a67ef3b..ad246038 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready.go @@ -2,69 +2,52 @@ package main import ( "fmt" + "sort" "strings" "time" "git.novox.be/novox/mesh-sdk/go/asks" ) -// Merge on ready (novox/hq issue 722). +// The Approve is asked as soon as the checks pass (novox/hq ADR 0421, issue 1022). // -// The operator, 2026-10-14: "the reviewer task should merge the request or mark the MR as ready to merge in any case". -// Before, a reviewed pull request with green checks waited until a session called the forge's merge tool, and again -// after every check the tool asked because the trunk had moved. Now the independent reviewer marks the pull request -// ready at the head it approved — the forge label `mesh/ready` and a comment "ready at " (approving -// through the forge is the operator's) — and this owner merges it, through the forge's own merge tool -// (gitea_merge_pull_request), the one path every merge takes: +// Before, a reviewer marked a pull request ready (a label and a comment) and only then was the operator's Approve asked +// (issue 722, ADR 0364). The review now finishes before the push (playbook 07) and its record, a line in the pull +// request's description, is given when the pull request is opened, so the mark repeated what the push already meant and +// added a step and a wait. This owner asks the operator's Approve itself, within the event that makes it possible: the +// verdict that passes, the commit status that sets the checks. Then: // -// - only at the head the mark names, and only while that is the pull request's head: a push after the mark clears -// it — the label taken off, and said on the pull request — and the new head needs a mark of its own; -// - only when its delivery is ready here: its check passed against its trunk's head. A verdict out of date is judged -// again by the merge tool, which asks the check against the trunk's head itself (novox/hq ADR 0323 §5); the merge -// is asked again once a minute, and lands on the passing verdict; -// - a group member only at its turn in the group's order, and only once the group's composed check passed for its -// members' heads as they are; +// - the Approve is the only act: nothing is merged before it, and nothing else is needed after it (no label, comment +// or second Approve); it is asked once for a head, and once for a group's heads together; +// - it merges through the forge's merge tool (gitea_merge_pull_request), the one path every merge takes, at the head +// the Approve names, handing the tool the heads the Approve covers (ADR 0421 §9); +// - only when its delivery is ready here: its check passed against its trunk's head. A verdict out of date is asked +// again by this owner at once (askFresh), and an Approve already asked stays through a trunk move (ADR 0421 §3): the +// merge is asked again on the passing verdict, with no second Approve; +// - a group member only once the group's composed check passed for its members' heads as they are; +// - a push ends the Approve and says so once on the pull request (ADR 0421 §6); the new head is asked on its own; // - a check that failed merges nothing: it is said once on the pull request, and as a stalled line, which the -// controller raises as the condition `delivery..stalled`. So is a mark the merge tool refused for an hour. +// controller raises as the condition `delivery..stalled`. So is a merge the tool refused for an hour. // -// What this owner does with a mark is kept on the delivery (Ready), said in its log, and the merge's transitions name -// it as their author. +// The review record is read and put in the ask; it is never a gate (ADR 0421 §5). What this owner does is kept on the +// delivery (Ready), said in its log, and the merge's transitions name the Approve as their warrant. -// ReadyMark is a pull request's ready mark as the forge reads it (the gitea module's gitea_open_pulls): the head the -// newest "ready at" comment names, who wrote it and when, and the label's id. Head is empty when the label is on and -// no comment names a head. -type ReadyMark struct { - Head string `json:"head"` - By string `json:"by,omitempty"` - At string `json:"at,omitempty"` - LabelID int `json:"label_id"` -} - -// ReadyLabel is the label that marks a pull request ready. -const ReadyLabel = "mesh/ready" - -// mergeRetryAfter is how long after the merge tool refused a marked head's merge it is asked again, when nothing -// happened meanwhile that decides whether it may merge (a verdict, an Approve, a group's turn are events, and ask it at +// mergeRetryAfter is how long after the merge tool refused a head's merge it is asked again, when nothing happened +// meanwhile that decides whether it may merge (a verdict, an Approve, a group's composed check are events, and ask it at // once): a check it asked takes minutes, and a refusal costs the forge a few reads. A retry of a bounded step (askMerge). const mergeRetryAfter = time.Minute -// markGrace is how long a mark not complete (the label, no "ready at" comment yet), and one this owner has just -// cleared, are left before they are judged again: the comment follows the label, and a clearing takes a moment. -const markGrace = time.Minute - -// readyRefusedFor is how long the merge tool may refuse a marked head before it is said as stalled: longer than a +// readyRefusedFor is how long the merge tool may refuse an approved head before it is said as stalled: longer than a // check takes (the controller's checkTimeout is 45 minutes), so a check on its way is never said. const readyRefusedFor = time.Hour -// ReadyRun is what this owner did with a delivery's ready mark: kept on the delivery and shown whole by `show`. +// ReadyRun is what this owner did with a delivery's Approve and merge: kept on the delivery and shown whole by `show`. type ReadyRun struct { - // Head, By and MarkedAt are the mark: the head it names, who marked it, when. - Head string `json:"head"` - By string `json:"by,omitempty"` - MarkedAt string `json:"marked_at,omitempty"` - // Seen is when this owner first read the mark. + // Head is the head the run is for. + Head string `json:"head"` + // Seen is when this owner first made the run: the checks passed. Seen time.Time `json:"seen"` - // Waits is what the merge waits on, when it waits: a verdict, the group's turn or its composed check. + // Waits is what the merge waits on, when it waits: a verdict, the group's composed check or the operator's Approve. Waits string `json:"waits,omitempty"` // Tries is how many times the merge was asked; LastTry when, and Answer what the merge tool last answered. Tries int `json:"tries,omitempty"` @@ -78,8 +61,8 @@ type ReadyRun struct { Merged *time.Time `json:"merged,omitempty"` // Failed is why it is not merged and will not be on this head: its check failed. Said once on the pull request. Failed string `json:"failed,omitempty"` - // OtherBase is the branch its pull request merges into when that is not main: never merged here, said once on the - // pull request (novox/hq ADR 0398 §4, issue 948). + // OtherBase is the branch its pull request merges into when that is not main: never asked, never merged here, said + // once on the pull request (novox/hq ADR 0398 §4, issue 948). OtherBase string `json:"other_base,omitempty"` // The operator's Approve of the merge into main (novox/hq ADR 0398 §3, issue 948, approval.go). AskID is the ask @@ -101,120 +84,57 @@ type ReadyRun struct { // MergeBranch is the one branch this owner merges into (novox/hq ADR 0398 §4): a merge into any other is its author's. const MergeBranch = "main" -// mergedBy is who a merge this owner made on a ready mark is said to be by, in its transitions. +// mergedBy is who a merge this owner made on an Approve is said to be by, in its transitions. func (r *ReadyRun) mergedBy() string { if r == nil || r.Merged == nil { return "" } - by := byOwner + ", on the ready mark at " + shortOf(r.Head, 12) - if r.By != "" { - by += " by " + r.By - } + by := byOwner + ", on the operator's Approve at " + shortOf(r.Head, 12) if r.Warrant != "" { by += "; the merge into main approved: " + r.Warrant } return by } -// marked is a pull request read with a ready mark. -type marked struct { - pull OpenPull - // cleared is when the mark was cleared, the label taken off: not cleared again within markGrace. - cleared time.Time - // firstEmpty is when a mark with no head (a label no "ready at" comment follows yet) was first read so: it is a - // mark not yet complete, and is cleared only once it has stayed so for markGrace (novox/hq issue 873). - firstEmpty time.Time -} - // pullKey is a pull request as a key: owner/repo#number. func pullKey(p OpenPull) string { return strings.ToLower(p.Owner+"/"+p.Repo) + fmt.Sprintf("#%d", p.Number) } -// seeMarks keeps the open pull requests that carry a ready mark; under the lock. A pull request no longer open or -// no longer marked drops out. -func (h *Holder) seeMarks(open []OpenPull) { - seen := map[string]*marked{} - for _, p := range open { - if p.Ready == nil || p.HeadSHA == "" { - continue - } - k := pullKey(p) - m := &marked{pull: p} - if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == p.Ready.Head && - was.pull.HeadSHA == p.HeadSHA { - m.cleared = was.cleared - } - if p.Ready.Head == "" { - m.firstEmpty = h.Now() - if was := h.marks[k]; was != nil && was.pull.Ready != nil && was.pull.Ready.Head == "" && - was.pull.HeadSHA == p.HeadSHA && !was.firstEmpty.IsZero() { - m.firstEmpty = was.firstEmpty - } - } - seen[k] = m +// runOf is the delivery's run, made when it has none. +func (h *Holder) runOf(d *Delivery, now time.Time) *ReadyRun { + if d.Ready == nil { + d.Ready = &ReadyRun{Head: d.Commit, Seen: now} + h.keep(d) } - h.marks = seen + return d.Ready } -// sameFull is whether a mark names a head: the whole sha, or — read from an older forge module — at least twelve -// characters of it. -func sameFull(mark, head string) bool { - mark, head = strings.ToLower(mark), strings.ToLower(head) - return len(mark) >= 12 && strings.HasPrefix(head, mark) -} - -// readyAsks works out, under the lock, what each ready mark needs: a mark cleared, a merge asked, a failure said. -// The asking happens outside the lock. +// readyAsks works out, under the lock, what each delivery whose checks passed needs: the operator asked, a merge asked, +// a failure said, an out of date verdict's check asked again. The asking happens outside the lock. func (h *Holder) readyAsks(now time.Time) []func() { var asks []func() h.endStaleAsks() - for k, m := range h.marks { - p, mark := m.pull, *m.pull.Ready - repository := strings.ToLower(p.Owner + "/" + p.Repo) - if !sameFull(mark.Head, p.HeadSHA) { - if mark.Head == "" && now.Sub(m.firstEmpty) < markGrace { - h.lookAt(m.firstEmpty.Add(markGrace), "a ready label whose comment naming the head has not followed yet") - continue // the label first, its comment not yet: a mark not complete, not an old one - } - if d := h.deliveries[IDOf(repository, p.HeadSHA)]; d != nil && d.Ready != nil && - sameFull(d.Ready.Head, p.HeadSHA) && now.Sub(d.Ready.Seen) < markGrace { - h.lookAt(d.Ready.Seen.Add(markGrace), "a mark made through mark-ready, its comment possibly not yet read") - continue // marked through mark-ready a moment ago: this read may predate its comment - } - if !m.cleared.IsZero() && now.Sub(m.cleared) < markGrace { - h.lookAt(m.cleared.Add(markGrace), "a stale ready mark whose clearing may not have taken") - continue - } - m.cleared = now - if old := h.deliveries[IDOf(repository, mark.Head)]; old != nil && old.Ready != nil && old.Ready.AskID != "" { - asks = append(asks, h.dropAsk(old.Ready.AskID, "its head moved on to "+shortOf(p.HeadSHA, 12))) - } - asks = append(asks, h.clearMark(k, p, mark)) - continue + ids := make([]string, 0, len(h.deliveries)) + for id := range h.deliveries { + ids = append(ids, id) + } + sort.Strings(ids) + for _, id := range ids { + d := h.deliveries[id] + if d.Number == 0 || d.State.Final() || d.MergedAs != "" { + continue // not of a pull request, or past its merge } - d := h.deliveries[IDOf(repository, p.HeadSHA)] - if d == nil || d.State.Final() || d.MergedAs != "" { - continue // not announced yet, or past its merge - } - if d.Ready == nil || d.Ready.Head != mark.Head || (d.Ready.Declined != nil && mark.At != d.Ready.MarkedAt) { - d.Ready = &ReadyRun{Head: mark.Head, By: mark.By, MarkedAt: mark.At, Seen: now} + if d.Ready == nil && d.State == Ready { + h.runOf(d, now) h.Poke() // its check, if none is asked, is worth asking now (askCheck) - h.Logf("[mesh-delivery] %s is marked ready at %s by %s: merged once its check passes against its trunk's head", - d.ID, shortOf(mark.Head, 12), orNone(mark.By)) - h.keep(d) + h.Logf("[mesh-delivery] %s: its checks passed; the operator's Approve of its merge into main is asked once it may merge", d.ID) + } + if a := h.askFresh(d, now); a != nil { + asks = append(asks, a) } r := d.Ready - if r.Merged != nil { - continue - } - if r.Declined != nil { - // The operator declined: nothing is merged, and the mark is taken off (said once when it was declined). - if m.cleared.IsZero() || now.Sub(m.cleared) >= markGrace { - m.cleared = now - asks = append(asks, h.unmark(k, p, mark)) - } - h.lookAt(m.cleared.Add(markGrace), "a declined ready mark whose clearing may not have taken") + if r == nil || r.Merged != nil || r.Declined != nil { continue } if d.State == Rejected { @@ -227,22 +147,21 @@ func (h *Holder) readyAsks(now time.Time) []func() { } } r.Waits = "" - h.Logf("[mesh-delivery] %s is marked ready and not merged: %s", d.ID, r.Failed) + h.Logf("[mesh-delivery] %s passed its checks and is not merged: %s", d.ID, r.Failed) h.keep(d) - asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request is marked "+ - "ready at %s, and %s. A new head, marked ready once reviewed, or a recheck that passes, merges it.", - mark.Head, r.Failed))) + asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request's checks passed at "+ + "%s, and now %s. A new head, or a recheck that passes, asks the operator again.", d.Commit, r.Failed))) } continue } - if p.Base != MergeBranch { - if r.OtherBase != p.Base { - r.OtherBase = p.Base - h.Logf("[mesh-delivery] %s is marked ready and not merged: it merges into %s, not %s", d.ID, p.Base, MergeBranch) + if d.Base != MergeBranch { + if r.OtherBase != d.Base { + r.OtherBase = d.Base + h.Logf("[mesh-delivery] %s passed its checks and is not asked: it merges into %s, not %s", d.ID, d.Base, MergeBranch) h.keep(d) - asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request is marked ready "+ - "at %s, and it merges into %s. Only a merge into %s is this owner's; a merge into another branch is its "+ - "author's (novox/hq ADR 0398 §4). The mark stays.", mark.Head, p.Base, MergeBranch))) + asks = append(asks, h.sayOn(d, fmt.Sprintf("mesh-delivery: **not merged.** This pull request's checks passed at "+ + "%s, and it merges into %s. Only a merge into %s is this owner's; a merge into another branch is its "+ + "author's (novox/hq ADR 0398 §4).", d.Commit, d.Base, MergeBranch))) } continue } @@ -252,12 +171,12 @@ func (h *Holder) readyAsks(now time.Time) []func() { if waits != r.Waits { r.Waits = waits if waits != "" { - h.Logf("[mesh-delivery] %s is marked ready and waits: %s", d.ID, waits) + h.Logf("[mesh-delivery] %s waits: %s", d.ID, waits) } h.keep(d) } var approvalAsk func() - if waits == "" && p.Base == MergeBranch { + if waits == "" { waits, approvalAsk = h.approval(d, now) } if waits != r.Waits { @@ -277,14 +196,55 @@ func (h *Holder) readyAsks(now time.Time) []func() { r.Tries++ r.LastTry, r.merging = now, true h.keep(d) - asks = append(asks, h.askMerge(d.ID, p, mark.Head)) + asks = append(asks, h.askMerge(d.ID, r.Heads)) } asks = append(h.cancels, asks...) h.cancels = nil return asks } -// mergeAgainNow lets a marked delivery's merge be asked at once: something that decides whether it may merge happened (a +// askFresh is the ask of a head's check against its trunk's head, when its passing verdict is out of date and either +// arrived so, or has an Approve asked or given for it (novox/hq ADR 0421 §3, issue 1022; issue 1047 for a group's +// member): the Approve stays through a trunk move, and the merge is asked again on the verdict this brings, with no +// second Approve. Nil when nothing needs asking. Under the lock; the ask runs outside it. +func (h *Holder) askFresh(d *Delivery, now time.Time) func() { + if d.State != Proposed || d.outOfDate() == "" || d.AskedAgain != nil || !d.Check.Passes() || d.Base != MergeBranch || + d.Number == 0 || d.TrunkNow == "" { + return nil + } + if !d.FreshWanted && h.covering(d) == nil { + return nil + } + if tried, was := h.askTried[d.ID]; was && now.Sub(tried) < time.Minute { + h.lookAt(tried.Add(time.Minute), "a check against the trunk's head that could not be asked") + return nil + } + h.askTried[d.ID] = now + h.lookAt(now.Add(time.Minute), "a check against the trunk's head that could not be asked") + id, m := d.ID, MemberOf(d) + m.BaseSHA = d.TrunkNow + return func() { + asked, err := h.Controller.Check("", []Member{m}) + h.mu.Lock() + defer h.mu.Unlock() + if err != nil { + h.Logf("[mesh-delivery] %s's check against the trunk's head cannot be asked yet: %v", id, err) + return + } + delete(h.askTried, id) + d := h.deliveries[id] + if d == nil || d.State != Proposed || d.outOfDate() == "" { + return + } + t := h.Now() + d.AskedAgain, d.FreshWanted = &t, false + h.Logf("[mesh-delivery] %s: its verdict is out of date; its check against the trunk's head %s asked as %s", id, + shortOf(d.TrunkNow, 12), asked) + h.keep(d) + } +} + +// mergeAgainNow lets an approved delivery's merge be asked at once: something that decides whether it may merge happened (a // verdict, an Approve, a group's turn), so the wait after a refusal does not hold it. Under the lock. func (h *Holder) mergeAgainNow(d *Delivery) { if d != nil && d.Ready != nil && d.Ready.Merged == nil { @@ -292,12 +252,18 @@ func (h *Holder) mergeAgainNow(d *Delivery) { } } -// readyWaits is what a marked delivery's merge waits on; empty when it is asked now. A ready delivery is asked; so is -// a proposed one whose verdict went out of date and whose check nobody asked again — the merge tool asks it. +// readyWaits is what a delivery's merge waits on; empty when the Approve may be asked and the merge asked. A ready +// delivery is asked. So is a proposed one whose verdict went out of date, when an Approve was asked for it already (it +// stays through a trunk move, ADR 0421 §3) and its check against the trunk's head is not on its way; one without an +// Approve waits for that check first. func (h *Holder) readyWaits(d *Delivery) string { switch { case d.State == Ready: - case d.State == Proposed && d.outOfDate() != "" && d.AskedAgain == nil: + case d.State == Proposed && d.outOfDate() != "" && d.AskedAgain != nil: + return "its check against the trunk's head is asked and has not answered" + case d.State == Proposed && d.outOfDate() != "" && d.Check.Passes() && h.covering(d) != nil: + case d.State == Proposed && d.outOfDate() != "": + return "its verdict is out of date: its check against the trunk's head is asked before the operator is" case d.State == Proposed && d.AskedAgain != nil: return "its check against the trunk's head is asked and has not answered" default: @@ -338,13 +304,26 @@ func (h *Holder) groupTurnToMerge(d *Delivery) string { return "" } -// askMerge asks the forge's merge tool to merge a marked head, and keeps its answer. -func (h *Holder) askMerge(id string, p OpenPull, head string) func() { +// askMerge asks the forge's merge tool to merge a head the operator approved, and keeps its answer. The tool is handed +// the heads the Approve covers, each `owner/repo@` (novox/hq ADR 0421 §9), and refuses a pull request whose +// head differs from the one approved. +func (h *Holder) askMerge(id string, heads map[string]string) func() { return func() { - err := h.Forge.Merge(p.Owner, p.Repo, p.Number, head) + h.mu.Lock() + d := h.deliveries[id] + if d == nil || d.Ready == nil { + h.mu.Unlock() + return + } + owner, repo, number, head := d.Owner(), d.Repo(), d.Number, d.Commit + if sha := heads[id]; sha != "" { + head = sha + } + h.mu.Unlock() + err := h.Forge.Merge(owner, repo, number, head, approvedHeads(heads)) h.mu.Lock() defer h.mu.Unlock() - d := h.deliveries[id] + d = h.deliveries[id] if d == nil || d.Ready == nil { return } @@ -353,9 +332,9 @@ func (h *Holder) askMerge(id string, p OpenPull, head string) func() { if err == nil { // Issue 1025: the announcement is owed, kept with the delivery, so a death right after the merge // still says it once. - d.Owed = append(d.Owed, Effect{Kind: EffectEmit, Event: "merged", Line: string(mustJSON(mergedEvent(d, p, head))), Since: now}) + d.Owed = append(d.Owed, Effect{Kind: EffectEmit, Event: "merged", Line: string(mustJSON(mergedEvent(d, head))), Since: now}) r.Merged, r.Answer, r.RefusedSince = &now, "merged", nil - h.Logf("[mesh-delivery] %s merged on its ready mark at %s, its check passed against its trunk's head", id, + h.Logf("[mesh-delivery] %s merged on the operator's Approve at %s, its check passed against its trunk's head", id, shortOf(head, 12)) h.keep(d) return @@ -365,7 +344,7 @@ func (h *Holder) askMerge(id string, p OpenPull, head string) func() { r.RefusedSince = &now } if answer != r.Answer { - h.Logf("[mesh-delivery] %s is marked ready, and the merge tool did not merge it (asked again in %s): %s", id, + h.Logf("[mesh-delivery] %s is approved, and the merge tool did not merge it (asked again in %s): %s", id, mergeRetryAfter, answer) } h.lookAt(now.Add(mergeRetryAfter), "a merge the forge refused") @@ -374,60 +353,15 @@ func (h *Holder) askMerge(id string, p OpenPull, head string) func() { } } -// clearMark takes a mark off a pull request whose head moved after it, and says so on it. -func (h *Holder) clearMark(k string, p OpenPull, mark ReadyMark) func() { - return func() { - err := h.Forge.Unlabel(p.Owner, p.Repo, p.Number, mark.LabelID) - if err == nil { - err = h.Forge.Comment(p.Owner, p.Repo, p.Number, clearedSaying(mark, p)) - } - if mark.Head == "" { - if err != nil { - h.Logf("[mesh-delivery] %s's ready label has no \"ready at \" comment of a listed account after it, and could not be taken off yet: %v", k, err) - h.again("a ready label that could not be taken off") - return - } - h.Logf("[mesh-delivery] %s's ready label taken off: no \"ready at \" comment of a listed account was written after it; mark-ready writes both in order", k) - } else { - if err != nil { - h.Logf("[mesh-delivery] %s's ready mark at %s names an older head than %s, and could not be cleared yet: %v", - k, shortOf(mark.Head, 12), shortOf(p.HeadSHA, 12), err) - h.again("a stale ready mark that could not be cleared") - return - } - h.Logf("[mesh-delivery] %s's ready mark at %s cleared: its head is now %s", k, shortOf(mark.Head, 12), - shortOf(p.HeadSHA, 12)) - } - h.mu.Lock() - delete(h.marks, k) - h.mu.Unlock() +// approvedHeads are the heads an Approve covers as the merge tool is handed them: `owner/repo@`, sorted. +func approvedHeads(heads map[string]string) []string { + var out []string + for id, sha := range heads { + repository, _, _ := strings.Cut(id, "@") + out = append(out, repository+"@"+sha) } -} - -// unmark takes the mark off a pull request whose merge the operator declined (already said on it). -func (h *Holder) unmark(k string, p OpenPull, mark ReadyMark) func() { - return func() { - if err := h.Forge.Unlabel(p.Owner, p.Repo, p.Number, mark.LabelID); err != nil { - h.Logf("[mesh-delivery] %s's ready mark could not be taken off after the operator's Decline yet: %v", k, err) - h.again("a declined ready mark that could not be taken off") - return - } - h.mu.Lock() - delete(h.marks, k) - h.mu.Unlock() - } -} - -// clearedSaying is what a cleared mark says on its pull request. A mark with no head is a label no "ready at " -// comment of a listed account followed (novox/hq issue 873); one with a head names an older head than the pull's. -func clearedSaying(mark ReadyMark, p OpenPull) string { - if mark.Head == "" { - return "mesh-delivery: the label " + ReadyLabel + " is taken off: no \"ready at\" comment by a listed account " + - "follows both the label's last addition and the last push; the head is now " + p.HeadSHA + ". Review it and " + - "mark that head with `mesh-delivery.mark-ready`, which writes label and comment in the order the forge counts." - } - return fmt.Sprintf("mesh-delivery: the ready mark at %s is cleared: "+ - "the head is now %s, pushed after it. Mark the new head ready once it is reviewed.", mark.Head, p.HeadSHA) + sort.Strings(out) + return out } // sayOn comments on a delivery's pull request. @@ -440,7 +374,7 @@ func (h *Holder) sayOn(d *Delivery, body string) func() { } } -// readyLines are the marked heads that will not merge: a check that failed at once, a merge refused past its bound; +// readyLines are the approved heads that will not merge: a check that failed at once, a merge refused past its bound; // as stalled lines, which the controller raises as conditions. Under the lock. func (h *Holder) readyLines(now time.Time) []StalledLine { var out []StalledLine @@ -452,19 +386,19 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { switch { case r.Failed != "" && d.State == Rejected: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), - Bound: "0s", H2: "none: a new head marked ready, or a recheck that passes — the operator's", - Says: fmt.Sprintf("%s#%d is marked ready at %s and not merged: %s", d.Repository, d.Number, + Bound: "0s", H2: "none: a new head, or a recheck that passes — the operator's", + Says: fmt.Sprintf("%s#%d passed its checks at %s and is not merged: %s", d.Repository, d.Number, shortOf(r.Head, 12), r.Failed)}) case r.Refused != "": out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(r.Seen).Round(time.Second).String(), - Bound: "0s", H2: "none: a new head marked ready, which asks the operator again — the operator's", - Says: fmt.Sprintf("%s#%d is marked ready at %s and not merged: the operator's answer %s", d.Repository, + Bound: "0s", H2: "none: a new head, which asks the operator again — the operator's", + Says: fmt.Sprintf("%s#%d at %s is not merged: the operator's answer %s", d.Repository, d.Number, shortOf(r.Head, 12), r.Refused)}) case r.RefusedSince != nil && now.Sub(*r.RefusedSince) >= readyRefusedFor: out = append(out, StalledLine{ID: d.ID, Number: d.Number, State: d.State, For: now.Sub(*r.RefusedSince).Round(time.Second).String(), Bound: readyRefusedFor.String(), H2: "none: the merge is asked again after each verdict, Approve or turn, and each minute — the operator's", - Says: fmt.Sprintf("%s#%d is marked ready at %s, and the merge tool has refused it since %s: %s", d.Repository, + Says: fmt.Sprintf("%s#%d is approved at %s, and the merge tool has refused it since %s: %s", d.Repository, d.Number, shortOf(r.Head, 12), r.RefusedSince.UTC().Format(time.RFC3339), r.Answer)}) } } @@ -474,13 +408,13 @@ func (h *Holder) readyLines(now time.Time) []StalledLine { // mergedEvent is the module event `merged` (issue 1025): the pull request merged, its head and group, and the ask, // option and heads the operator's Approve was bound to (the warrant the controller reads back from the router). A merge // without an Approve names no ask and no option: no warrant is invented. Under the lock, before anything clears them. -func mergedEvent(d *Delivery, p OpenPull, head string) map[string]any { +func mergedEvent(d *Delivery, head string) map[string]any { r := d.Ready heads := map[string]string{} for k, v := range r.Heads { heads[k] = v } - ev := map[string]any{"owner": p.Owner, "repo": p.Repo, "number": p.Number, "head_sha": head, "group": d.Group, + ev := map[string]any{"owner": d.Owner(), "repo": d.Repo(), "number": d.Number, "head_sha": head, "group": d.Group, "ask_id": "", "option": "", "heads": heads} if r.Approved != nil { ev["ask_id"], ev["option"] = r.AskID, OptionApprove diff --git a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go index 565f08bb..2b705c56 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/ready_test.go @@ -7,20 +7,19 @@ import ( "time" ) -// **A pull request marked ready is merged by this owner** (novox/hq issue 722): before, a reviewed pull request with -// green checks waited until a session called the merge tool, and again after every check the tool asked because the -// trunk had moved. Each test below fails on a holder without the ready mark: nothing merged it. +// **A pull request whose checks passed and whose merge the operator approved is merged by this owner** (novox/hq issues +// 722 and 1022): before, a reviewed pull request with green checks waited until a session called the merge tool, and +// again after every check the tool asked because the trunk had moved. const ( readyHead = "1111111111111111111111111111111111111111" pushed = "2222222222222222222222222222222222222222" ) -// markedPull is an open pull request as gitea_open_pulls reads it, labelled mesh/ready at the head named. -func markedPull(repo string, number int, head, markedAt string) OpenPull { +// openPull is an open pull request into main as gitea_open_pulls reads it. +func openPull(repo string, number int, head string) OpenPull { owner, name, _ := strings.Cut(repo, "/") - return OpenPull{Owner: owner, Repo: name, Number: number, Base: "main", HeadSHA: head, Labels: []string{ReadyLabel}, - Ready: &ReadyMark{Head: markedAt, By: "reviewer", At: "2026-10-14T19:00:00Z", LabelID: 31}} + return OpenPull{Owner: owner, Repo: name, Number: number, Base: "main", HeadSHA: head} } func judgedAt(repo string, number int, sha, gate, trunk string) CheckedEvent { @@ -29,26 +28,26 @@ func judgedAt(repo string, number int, sha, gate, trunk string) CheckedEvent { return c } -func TestAReadyPullRequestMarkedAtItsHeadIsMergedThroughTheMergeTool(t *testing.T) { +func TestAnApprovedPullRequestIsMergedThroughTheMergeTool(t *testing.T) { w := newWorld(t) + w.op.auto = false id := IDOf("novox/app", readyHead) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) w.settleAll() if len(w.forge.merges) != 0 { - t.Fatalf("an unmarked pull request was merged: %v", w.forge.merges) + t.Fatalf("a pull request nobody approved was merged: %v", w.forge.merges) } - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - w.h.SeeOpenPulls() + w.op.answer(w.op.open()[0], OptionApprove) w.settleAll() if len(w.forge.merges) != 1 || w.forge.merges[0] != "novox/app#7@"+readyHead { - t.Fatalf("a ready, marked pull request was merged %v; wanted once, at the marked head", w.forge.merges) + t.Fatalf("an approved pull request was merged %v; wanted once, at the approved head", w.forge.merges) } d := w.delivery(id) - if d.Ready == nil || d.Ready.Merged == nil || d.Ready.By != "reviewer" || d.Ready.Tries != 1 { - t.Fatalf("what was done with the mark is not kept: %+v", d.Ready) + if d.Ready == nil || d.Ready.Merged == nil || d.Ready.Tries != 1 { + t.Fatalf("what was done with the Approve is not kept: %+v", d.Ready) } - // Its merge's transition names who merged it. + // Its merge's transition names the Approve as its warrant. w.h.PullMerged(merged("novox/app", 7, readyHead, "m7m7m7m7m7m7")) w.h.WalkMoved(w.ctl.put(aWalk("plan-7", "novox/app", "m7m7m7m7m7m7", true))) w.settleAll() @@ -59,97 +58,20 @@ func TestAReadyPullRequestMarkedAtItsHeadIsMergedThroughTheMergeTool(t *testing. last = tr } } - if !strings.Contains(last.By, "ready mark at "+readyHead[:12]+" by reviewer") { - t.Fatalf("the merge's transition does not name the mark: %+v", last) + if !strings.Contains(last.By, "the operator's Approve at "+readyHead[:12]) { + t.Fatalf("the merge's transition does not name the Approve: %+v", last) } if len(w.forge.merges) != 1 { t.Fatalf("merged again: %v", w.forge.merges) } } -func TestAPushAfterTheMarkClearsItAndNothingIsMerged(t *testing.T) { - w := newWorld(t) - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.h.PullUpdated(pr("novox/app", 7, pushed, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, pushed, "pass", "t1t1t1t1t1t1")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, pushed, readyHead)} - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.merges) != 0 { - t.Fatalf("a head pushed after the mark was merged: %v", w.forge.merges) - } - if len(w.forge.unlabels) != 1 || w.forge.unlabels[0] != "novox/app#7-31" { - t.Fatalf("the stale mark's label was not taken off: %v", w.forge.unlabels) - } - if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "cleared") || - !strings.Contains(w.forge.comments[0], pushed) { - t.Fatalf("the cleared mark is not said on the pull request: %v", w.forge.comments) - } -} - -func TestAMovedTrunkIsJudgedAgainByTheMergeToolAndMergedOnItsVerdict(t *testing.T) { - w := newWorld(t) - id := IDOf("novox/app", readyHead) - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) - // Another merge moves the trunk: its verdict is out of date, and it goes back to proposed. - w.h.PullMerged(merged("novox/app", 9, "9999999999999999", "t2t2t2t2t2t2")) - if w.state(id) != Proposed { - t.Fatalf("out of date, it is %s", w.state(id)) - } - // The merge tool asks the check against the trunk's head and refuses: nothing merged. - w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: The check of head merged into trunk was asked") - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.merges) != 1 { - t.Fatalf("the merge tool was not asked once to judge it again: %v", w.forge.merges) - } - // The tool's ask is the head announced again; it waits for that verdict, asking nothing. - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.later(5 * time.Minute) - w.settleAll() - if len(w.forge.merges) != 1 || !strings.Contains(w.delivery(id).Ready.Waits, "asked") { - t.Fatalf("asked again while its check was on its way: %v (%+v)", w.forge.merges, w.delivery(id).Ready) - } - // It passes against the trunk's head: merged, without anyone asking. - w.forge.refuse = nil - w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t2t2t2t2t2t2")) - w.settleAll() - if w.state(id) != Ready || len(w.forge.merges) != 2 || w.delivery(id).Ready.Merged == nil { - t.Fatalf("on its passing verdict it was not merged: %s %v %+v", w.state(id), w.forge.merges, w.delivery(id).Ready) - } -} - -func TestAMarkedHeadWhoseCheckFailedIsNotMergedAndIsSaid(t *testing.T) { - w := newWorld(t) - id := IDOf("novox/app", readyHead) - w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) - w.h.Checked(judgedAt("novox/app", 7, readyHead, "fail", "t1t1t1t1t1t1")) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} - w.h.SeeOpenPulls() - w.settleAll() - w.later(10 * time.Minute) - w.h.SeeOpenPulls() - w.settleAll() - if len(w.forge.merges) != 0 { - t.Fatalf("a head whose check failed was merged: %v", w.forge.merges) - } - if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "not merged") { - t.Fatalf("the failure is not said once on the pull request: %v", w.forge.comments) - } - lines := w.h.Stalled() - if len(lines) != 1 || lines[0].ID != id || lines[0].State != Rejected || !strings.Contains(lines[0].Says, "marked ready") { - t.Fatalf("the failure is not a stalled line, which the controller raises as a condition: %+v", lines) - } -} - -func TestAMarkRefusedForAnHourIsSaid(t *testing.T) { +func TestAMergeRefusedForAnHourIsSaid(t *testing.T) { w := newWorld(t) w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: mesh/repo-check is pending") - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} w.h.SeeOpenPulls() w.settleAll() w.later(30 * time.Second) @@ -176,8 +98,8 @@ func TestAGroupMergesItsMarkedMembersInOrderOnlyOnceComposed(t *testing.T) { w.h.PullUpdated(pr("novox/mesh-catalog", 2, catHead, "feat/x", "modules/app/module.json")) w.h.Checked(judgedAt("novox/mesh-controller", 1, ctlHead, "pass", "t1t1t1t1t1t1")) w.h.Checked(judgedAt("novox/mesh-catalog", 2, catHead, "pass", "t3t3t3t3t3t3")) - w.forge.open = []OpenPull{markedPull("novox/mesh-controller", 1, ctlHead, ctlHead), - markedPull("novox/mesh-catalog", 2, catHead, catHead)} + w.forge.open = []OpenPull{openPull("novox/mesh-controller", 1, ctlHead), + openPull("novox/mesh-catalog", 2, catHead)} w.h.SeeOpenPulls() w.settleAll() if len(w.forge.merges) != 0 { @@ -198,15 +120,15 @@ func TestAGroupMergesItsMarkedMembersInOrderOnlyOnceComposed(t *testing.T) { } } -// **A merge into another branch than main is its author's** (novox/hq ADR 0398 §4, issue 948): a ready mark on a pull -// request whose base is not main is never merged by this owner; it says so once on the pull request and leaves the mark. -func TestAMarkedGreenPullRequestIntoAnotherBranchIsNotMergedAndSaidOnce(t *testing.T) { +// **A merge into another branch than main is its author's** (novox/hq ADR 0398 §4, issue 948): a green pull request +// whose base is not main is never asked or merged by this owner; it says so once on the pull request. +func TestAGreenPullRequestIntoAnotherBranchIsNotAskedOrMergedAndSaidOnce(t *testing.T) { w := newWorld(t) p := pr("novox/app", 7, readyHead, "feat/x", "src/a.go") p.Base = "release/x" w.h.PullUpdated(p) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) - open := markedPull("novox/app", 7, readyHead, readyHead) + open := openPull("novox/app", 7, readyHead) open.Base = "release/x" w.forge.open = []OpenPull{open} for range 3 { @@ -214,14 +136,11 @@ func TestAMarkedGreenPullRequestIntoAnotherBranchIsNotMergedAndSaidOnce(t *testi w.settleAll() w.later(2 * time.Minute) } - if len(w.forge.merges) != 0 { - t.Fatalf("a pull request into release/x was merged: %v", w.forge.merges) + if len(w.forge.merges) != 0 || w.op.count() != 0 { + t.Fatalf("a pull request into release/x was merged %v or asked %d", w.forge.merges, w.op.count()) } if len(w.forge.comments) != 1 || !strings.Contains(w.forge.comments[0], "another branch") || !strings.Contains(w.forge.comments[0], "0398") { t.Fatalf("not said exactly once on the pull request: %v", w.forge.comments) } - if len(w.forge.unlabels) != 0 { - t.Fatalf("the mark was taken off: %v", w.forge.unlabels) - } } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go index 4d5d3e5c..281994f2 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/timers_test.go @@ -52,7 +52,7 @@ func TestAMergeTheForgeRefusedIsAskedAgainAfterAMinuteByTheTimerAlone(t *testing w.h.PullUpdated(pr("novox/app", 7, readyHead, "feat/x", "src/a.go")) w.h.Checked(judgedAt("novox/app", 7, readyHead, "pass", "t1t1t1t1t1t1")) w.forge.refuse = errors.New("the forge refused gitea_merge_pull_request: not mergeable yet") - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} w.h.wantForge() w.settleAll() if len(w.forge.merges) != 1 { diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go index 7c4fcb98..92f37462 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked.go @@ -74,9 +74,6 @@ type OpenPull struct { // than that says none: the head is then read by Statuses alone, as before, and a pending check is not told // from an answered one. Checks map[string]HeadCheck `json:"checks,omitempty"` - // Labels are its labels by name, and Ready its ready mark, when it carries one (novox/hq issue 722). - Labels []string `json:"labels,omitempty"` - Ready *ReadyMark `json:"ready,omitempty"` } // HeadCheck is one status on a head as the forge keeps it: pending, success, failure, error or warning. @@ -170,7 +167,7 @@ func askedAgain(was, now map[string]string) bool { } // SeeOpenPulls asks the forge for every open pull request and keeps the heads with a required merge check not -// answered, and the ready marks (seeMarks). A forge that cannot be asked changes nothing: what was seen stands, and nothing is guessed. +// answered. A forge that cannot be asked changes nothing: what was seen stands, and nothing is guessed. func (h *Holder) SeeOpenPulls() error { open, err := h.Forge.OpenPulls() if err != nil { @@ -208,7 +205,6 @@ func (h *Holder) SeeOpenPulls() error { seen[id] = u } h.unchecked = seen - h.seeMarks(open) return nil } diff --git a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go index e052a884..da0f0cbf 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/unchecked_events_test.go @@ -67,7 +67,7 @@ func TestANewlyAnnouncedHeadReadsTheForgeAndAKnownOneDoesNot(t *testing.T) { func TestNothingReadsTheForgeWithoutAnEvent(t *testing.T) { w := newWorld(t) - w.forge.open = []OpenPull{markedPull("novox/app", 7, readyHead, readyHead)} + w.forge.open = []OpenPull{openPull("novox/app", 7, readyHead)} before := w.forge.openCalls for range 6 { w.later(time.Minute) diff --git a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go index 97db5703..14f163f6 100644 --- a/modules/mesh-delivery/cmd/mesh-delivery/verbs.go +++ b/modules/mesh-delivery/cmd/mesh-delivery/verbs.go @@ -331,65 +331,6 @@ func (h *Holder) Recheck(id, why, by string) (string, error) { return fmt.Sprintf("%s is proposed again, its merge check pending; its check was asked (%s)", id, asked), nil } -// MarkReady is a reviewer's word that a delivery's head is approved (novox/hq issue 913): the label mesh/ready on its -// pull request, and only after that call succeeded the comment "ready at ", the order the forge counts (a comment -// written before the label does not count, issue 873). Refused for an unknown, final or merged delivery, and when the -// pull request's head is no longer the delivery's commit. -func (h *Holder) MarkReady(id, reviewedBy, why string) (string, error) { - h.mu.Lock() - d := h.deliveries[id] - if d == nil { - h.mu.Unlock() - return "", fmt.Errorf("no delivery %q; `deliveries` lists them", id) - } - if d.State.Final() || d.MergedAs != "" { - h.mu.Unlock() - return "", fmt.Errorf("%s is %s: a delivery past its merge or final is not marked ready", id, d.State) - } - if d.Number == 0 { - h.mu.Unlock() - return "", fmt.Errorf("%s is not of a pull request: only a pull request is marked ready", id) - } - owner, repo, number, commit := d.Owner(), d.Repo(), d.Number, d.Commit - h.mu.Unlock() - - open, err := h.Forge.OpenPulls() - if err != nil { - return "", fmt.Errorf("%s is not marked: the forge's open pull requests could not be read: %v", id, err) - } - head := "" - for _, p := range open { - if strings.EqualFold(p.Owner, owner) && strings.EqualFold(p.Repo, repo) && p.Number == number { - head = p.HeadSHA - } - } - if head == "" { - return "", fmt.Errorf("%s#%d is not an open pull request on the forge", owner+"/"+repo, number) - } - if !sameCommit(head, commit) { - return "", fmt.Errorf("the head is now %s; review and mark that one", head) - } - if err := h.Forge.Label(owner, repo, number, ReadyLabel); err != nil { - return "", fmt.Errorf("%s is not marked: the label %s could not be put on: %v", id, ReadyLabel, err) - } - body := fmt.Sprintf("ready at %s\n\nReviewed by %s: %s. Marked through mesh-delivery.mark-ready (novox/hq issue 913).", - head, reviewedBy, strings.TrimRight(strings.TrimSpace(why), ".")) - if err := h.Forge.Comment(owner, repo, number, body); err != nil { - return "", fmt.Errorf("%s has the label %s but its comment \"ready at\" could not be written: %v; mark it again", id, ReadyLabel, err) - } - h.mu.Lock() - defer h.mu.Unlock() - now := h.Now() - d.Ready = &ReadyRun{Head: head, By: reviewedBy, MarkedAt: now.UTC().Format(time.RFC3339), Seen: now} - h.Logf("[mesh-delivery] %s is marked ready at %s by %s through mark-ready: %s", id, shortOf(head, 12), reviewedBy, why) - h.keep(d) - waits := h.readyWaits(d) - if waits == "" { - return fmt.Sprintf("%s is marked ready at %s; its merge is asked once the forge's label and comment events reach this owner", id, shortOf(head, 12)), nil - } - return fmt.Sprintf("%s is marked ready at %s; its merge waits: %s", id, shortOf(head, 12), waits), nil -} - // checkingAgain puts a head's merge check statuses — those a branch's protection may require — back to // pending, so the forge refuses the merge until the fresh verdict sets them. The gate always, since every // verdict sets it; the repository's own check only when the head holds one, since a verdict that says none diff --git a/modules/mesh-delivery/module.json b/modules/mesh-delivery/module.json index 7a445a3a..058138ee 100644 --- a/modules/mesh-delivery/module.json +++ b/modules/mesh-delivery/module.json @@ -19,8 +19,7 @@ "release", "stop", "close", - "retire-history", - "mark-ready" + "retire-history" ] } ], @@ -28,9 +27,6 @@ "gitea.pull.updated", "gitea.pull.merged", "gitea.pull.closed", - "gitea.forge.pull-request.labeled", - "gitea.forge.pull-request.unlabeled", - "gitea.forge.pull-request.commented", "gitea.forge.status", "mesh-controller.checked", "mesh-controller.plan-moved" @@ -56,7 +52,6 @@ "gitea.gitea_open_pulls", "gitea.gitea_merge_pull_request", "gitea.gitea_add_comment", - "gitea.gitea_api", "seat:operator-channel.notify" ], "uses": [