From 33857626be9cd77382ede53e35241954444d27db Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 7 Oct 2026 13:55:34 +0200 Subject: [PATCH] Never set warning on the merge check's statuses: the forge blocks a required one Branch protection requires mesh/merge-gate (and mesh/repo-check on the core repositories) with no admin override, and the forge combines warning as a failure. A note is now a success that says it; a repository without a merge-check.sh is a success where repo-check is not required and a failure for a person where it is; the status tool refuses the merge check's contexts. --- modules/gitea/delivery.ts | 6 +++ modules/gitea/index.ts | 29 +++++++++-- modules/gitea/pulls.ts | 78 +++++++++++++++++++++++++---- modules/gitea/test/delivery.test.ts | 3 ++ modules/gitea/test/pulls.test.ts | 50 ++++++++++++++++-- 5 files changed, 148 insertions(+), 18 deletions(-) diff --git a/modules/gitea/delivery.ts b/modules/gitea/delivery.ts index e626e7d..9116dfb 100644 --- a/modules/gitea/delivery.ts +++ b/modules/gitea/delivery.ts @@ -85,10 +85,16 @@ export function viewComment(comments: T[ const states = new Set(["pending", "success", "error", "failure", "warning"]); +/** The merge check's contexts (pulls.ts): a branch's protection may require them. */ +const mergeCheckContexts = new Set(["mesh/merge-gate", "mesh/repo-check"]); + /** A status mesh-delivery asks for, checked: one of the forge's states, a context of the mesh's own, a * bounded description. */ export function deliveryStatus(context: string, state: string, description: string, target?: string) { if (!/^mesh\/[a-z-]+$/.test(context)) throw new Error(`${context} is not a status of the mesh's own`); + // The merge check's statuses are its verdict's, set when the controller says it, and a branch may require + // them: never set by hand, so nothing passes — or blocks — a merge past the check (novox/hq issue 293). + if (mergeCheckContexts.has(context)) throw new Error(`${context} is the merge check's status, set by its verdict only`); if (!states.has(state)) throw new Error(`${state} is not a status the forge keeps`); let d = String(description ?? "").replace(/\s+/g, " ").trim(); if (d.length > 140) d = d.slice(0, 139) + "…"; diff --git a/modules/gitea/index.ts b/modules/gitea/index.ts index 56d127f..2595300 100644 --- a/modules/gitea/index.ts +++ b/modules/gitea/index.ts @@ -27,7 +27,8 @@ import { emit, on } from "@novox/mesh-sdk/events"; import { GiteaClient, movedSince } from "./client.js"; -import { CHECK_CONTEXT, commentFor, headsToAnnounce, statusesFor, type Announced, type Checked } from "./pulls.js"; +import { CHECK_CONTEXT, REPO_CHECK_CONTEXT, commentFor, headsToAnnounce, statusesFor, type Announced, type Checked, + type RepoCheckFacts } from "./pulls.js"; // Without a way to a token — configured, or mintable with the admin account (token.ts) — there is // nothing to watch; log and stay quiet rather than crash the runtime. With one, the first poll mints @@ -288,17 +289,37 @@ async function setVerdict(client: GiteaClient, event: { body: unknown }): Promis } // Each status links to the pull request, where the delivery's view is kept (novox/hq ADR 0239). let target: string | undefined; + let base: string | undefined; if (c.number) { - target = await client.getPullRequest(c.owner, c.repo, c.number).then((p) => p.html_url || undefined).catch(() => undefined); + const pull = await client.getPullRequest(c.owner, c.repo, c.number).catch(() => undefined); + target = pull?.html_url || undefined; + base = pull?.base || undefined; } - for (const status of statusesFor(c)) { + const facts = await repoCheckFacts(client, c, base ?? c.plan?.base); + for (const status of statusesFor(c, facts)) { await client.setCommitStatus(c.owner, c.repo, c.commit, target ? { ...status, target_url: target } : status); } - const comment = commentFor(c); + const comment = commentFor(c, facts); if (comment && c.number) await client.addComment(c.owner, c.repo, c.number, comment); console.log(`[gitea] ${c.owner}/${c.repo}#${c.number ?? "?"} at ${c.commit.slice(0, 8)}: merge check ${c.verdict}`); } +// **What only the forge knows of a repository check said as a warning** (novox/hq issue 293): whether the +// head holds a merge-check.sh at all, and — when it does not — whether the base branch's protection +// requires mesh/repo-check. Asked only for a warning; unknown is left undefined, which statusesFor reads +// as possibly required: a failure on a status nothing requires blocks nothing, a success on one that is +// required would let an untested repository merge. +async function repoCheckFacts(client: GiteaClient, c: Checked, base: string | undefined): Promise { + if (c["repo-check"]?.verdict !== "warning") return {}; + const defined = await client.holdsFile(c.owner, c.repo, c.commit, "merge-check.sh").catch(() => undefined); + if (defined !== false) return { defined }; + const rules = await client.branchProtections(c.owner, c.repo).catch(() => undefined); + if (!rules) return { defined }; + const rule = rules.find((p) => (p.rule_name ?? p.branch_name) === (base || "main")); + const required = !!rule?.enable_status_check && (rule.status_check_contexts ?? []).includes(REPO_CHECK_CONTEXT); + return { defined, required }; +} + if (gitea) { const client = gitea; // A poll that fails says so once, not once a minute: the same reason repeating (the forge not up diff --git a/modules/gitea/pulls.ts b/modules/gitea/pulls.ts index 09bc783..09f02ae 100644 --- a/modules/gitea/pulls.ts +++ b/modules/gitea/pulls.ts @@ -92,19 +92,72 @@ export function headsToAnnounce(full: string, pulls: GiteaPull[], announced: Ann return pulls.filter((p) => p.state === "open" && !!p.head_sha && announced[`${full}#${p.number}`] !== p.head_sha); } -/** The forge's state for a verdict: an error is the forge's `error`, never a success. */ +// **A required status is success or it blocks** (novox/hq issue 293). The forge combines `warning` as a +// failure, and a branch whose protection requires mesh/merge-gate or mesh/repo-check — with no +// administrator override — will not merge past one. So the controller's `warning`, a note and never a +// question, is set as `success` with the note in its description; only what a person must decide is a +// `failure`, with why. The mapping, verdict by verdict: +// +// pass → success +// warning (a note: rebuild width, a problem +// already so on the base, a script's own) → success, "pass, with a note: …" +// repo-check, no merge-check.sh, not required → success, "no repository check defined" +// repo-check, no merge-check.sh, required (or +// the protection unreadable) → failure: a person adds one, or lifts the requirement +// fail → failure +// error, or no verdict → error — never a success + +/** The forge's state for a verdict: an error is the forge's `error`, never a success; a warning is a note. */ function stateOf(verdict: string): CommitStatus["state"] { - return verdict === "pass" ? "success" : verdict === "warning" ? "warning" : verdict === "fail" ? "failure" : "error"; + return verdict === "pass" || verdict === "warning" ? "success" : verdict === "fail" ? "failure" : "error"; +} + +/** How a verdict is said in a status's description: a warning as the pass with a note it is. */ +function saidAs(verdict: string): string { + return verdict === "warning" ? "pass, with a note" : verdict || "error"; } function described(verdict: string, summary: string, modules?: string[], dependents?: string[]): string { // The forge keeps a short description; the rest is the comment's. - let d = `${verdict || "error"}: ${summary}`; + let d = `${saidAs(verdict)}: ${summary}`; if (modules?.length) d += ` [${modules.join(", ")}${dependents?.length ? ` +${dependents.length} dependent(s)` : ""}]`; + return clipped(d); +} + +function clipped(d: string): string { d = d.replace(/\s+/g, " ").trim(); return d.length > 140 ? d.slice(0, 139) + "…" : d; } +/** What the forge says of a repository's own check beyond the controller's verdict, asked by index.ts only + * when the verdict is a warning: whether the head holds a merge-check.sh, and whether the base branch's + * protection requires mesh/repo-check. Unknown is undefined. */ +export interface RepoCheckFacts { + defined?: boolean; + required?: boolean; +} + +/** The description of a required repository check that is not defined. */ +export const UNDEFINED_REQUIRED = "fail: mesh/repo-check is required here and this head defines no merge-check.sh — " + + "a person adds one, or lifts the requirement from the branch's protection"; + +/** The repository check's status: its verdict, unless the repository defines no check at all — then + * success where the check is not required, and a failure for a person where it is (or may be). */ +function repoStatus(repo: Layer, facts: RepoCheckFacts): CommitStatus { + if (repo.verdict === "warning" && facts.defined === false) { + return facts.required === false + ? { state: "success", context: REPO_CHECK_CONTEXT, description: "no repository check defined" } + : { state: "failure", context: REPO_CHECK_CONTEXT, description: clipped(UNDEFINED_REQUIRED) }; + } + return { state: stateOf(repo.verdict), context: REPO_CHECK_CONTEXT, description: described(repo.verdict, repo.summary) }; +} + +/** Whether the repository check is a failure a person must read: failed, could not run, or required and + * not defined. */ +function repoWrong(repo: Layer | undefined, facts: RepoCheckFacts): boolean { + return !!repo && repoStatus(repo, facts).state !== "success"; +} + /** The forge's status for the gate. */ export function statusFor(c: Checked): CommitStatus { const gate = c.gate ?? { verdict: c.verdict, summary: c.summary }; @@ -117,10 +170,10 @@ export function statusFor(c: Checked): CommitStatus { } /** Every status a verdict sets: the gate's, and the repository's own check's when it was said. */ -export function statusesFor(c: Checked): CommitStatus[] { +export function statusesFor(c: Checked, facts: RepoCheckFacts = {}): CommitStatus[] { const out = [statusFor(c)]; const repo = c["repo-check"]; - if (repo) out.push({ state: stateOf(repo.verdict), context: REPO_CHECK_CONTEXT, description: described(repo.verdict, repo.summary) }); + if (repo) out.push(repoStatus(repo, facts)); return out; } @@ -128,16 +181,21 @@ export function statusesFor(c: Checked): CommitStatus[] { * change that builds something (novox/hq ADR 0238), and why, when the gate is not a pass or the * repository's own check failed or could not run. A repository with no merge-check.sh, touching nothing, * is said by its statuses alone, not by a comment on every push. */ -export function commentFor(c: Checked): string | null { +export function commentFor(c: Checked, facts: RepoCheckFacts = {}): string | null { const gate = c.gate ?? { verdict: c.verdict, summary: c.summary }; const repo = c["repo-check"]; - const repoWrong = !!repo && repo.verdict !== "pass" && repo.verdict !== "warning"; - if (gate.verdict === "pass" && !repoWrong && !builds(c.plan)) return null; + const wrong = repoWrong(repo, facts); + if (gate.verdict === "pass" && !wrong && !builds(c.plan)) return null; const lines = [`**Merge check** at \`${c.commit.slice(0, 8)}\``, ""]; - lines.push(`- \`${CHECK_CONTEXT}\`: **${(gate.verdict || "error").toUpperCase()}** — ${gate.summary}` + + lines.push(`- \`${CHECK_CONTEXT}\`: **${saidAs(gate.verdict).toUpperCase()}** — ${gate.summary}` + (gate.modules?.length ? ` (modules: ${gate.modules.join(", ")}` + (gate.dependents?.length ? `; built after them: ${gate.dependents.join(", ")}` : "") + ")" : "")); - if (repo) lines.push(`- \`${REPO_CHECK_CONTEXT}\`: **${(repo.verdict || "error").toUpperCase()}** — ${repo.summary}`); + if (repo) { + const st = repoStatus(repo, facts); + lines.push(`- \`${REPO_CHECK_CONTEXT}\`: ` + (repo.verdict === "warning" && facts.defined === false + ? `**${st.state === "success" ? "PASS" : "FAIL"}** — ${st.description.replace(/^fail: /, "")}` + : `**${saidAs(repo.verdict).toUpperCase()}** — ${repo.summary}`)); + } if (builds(c.plan)) lines.push("", planText(c.plan!)); const ran = c.on ? `\n\nRun by the build seat on ${c.on} as \`${c.id}\` (\`builds --log ${c.id}\`).` : ""; const report = c.report ? `\n\n\`\`\`\n${c.report.replace(/```/g, "'''")}\n\`\`\`` : ""; diff --git a/modules/gitea/test/delivery.test.ts b/modules/gitea/test/delivery.test.ts index 16ccd8b..5e87f69 100644 --- a/modules/gitea/test/delivery.test.ts +++ b/modules/gitea/test/delivery.test.ts @@ -52,5 +52,8 @@ test("one view per pull request, found by its marker; only the mesh's statuses", assert.ok(s.description.length <= 140 && s.target_url); assert.throws(() => deliveryStatus("ci/other", "success", "x"), /mesh's own/); assert.throws(() => deliveryStatus("mesh/delivery", "green", "x"), /not a status/); + // The merge check's statuses are its verdict's alone: required by a branch, they are never set by hand. + assert.throws(() => deliveryStatus("mesh/merge-gate", "success", "x"), /merge check's status/); + assert.throws(() => deliveryStatus("mesh/repo-check", "warning", "x"), /merge check's status/); assert.equal(deliveryStatus("mesh/delivery", "success", "x", "javascript:alert(1)").target_url, undefined); }); diff --git a/modules/gitea/test/pulls.test.ts b/modules/gitea/test/pulls.test.ts index 45cd521..88399da 100644 --- a/modules/gitea/test/pulls.test.ts +++ b/modules/gitea/test/pulls.test.ts @@ -22,7 +22,10 @@ test("a verdict is the head commit's status; an error is the forge's error, neve const { statusFor, commentFor, CHECK_CONTEXT } = await import("../pulls.ts"); const base = { owner: "novox", repo: "mesh-catalog", number: 7, commit: "0123456789abcdef", id: "build-1", on: "laptop" }; assert.equal(statusFor({ ...base, verdict: "pass", summary: "every machine composes" }).state, "success"); - assert.equal(statusFor({ ...base, verdict: "warning", summary: "a merge rebuilds 14 module(s)" }).state, "warning"); + // A warning is a note, never a blocking state: the forge combines `warning` as a failure (issue 293). + const wide = statusFor({ ...base, verdict: "warning", summary: "a merge rebuilds 14 module(s)" }); + assert.equal(wide.state, "success"); + assert.match(wide.description, /^pass, with a note: a merge rebuilds 14 module/); assert.equal(statusFor({ ...base, verdict: "fail", summary: "x" }).state, "failure"); assert.equal(statusFor({ ...base, verdict: "error", summary: "the check could not run" }).state, "error"); assert.equal(statusFor({ ...base, verdict: "", summary: "" }).state, "error", "no verdict is no pass"); @@ -51,15 +54,54 @@ test("each layer is its own status: the gate with the modules it judged, the rep const quiet = { ...base, verdict: "pass", summary: "the change touches no module of the mesh's graph", gate: { verdict: "pass", summary: "the change touches no module of the mesh's graph" }, "repo-check": { verdict: "warning", summary: "the repository declares no merge-check.sh" } }; - assert.deepEqual(statusesFor(quiet).map((s) => s.state), ["success", "warning"]); - assert.equal(commentFor(quiet), null); + assert.deepEqual(statusesFor(quiet, { defined: false, required: false }).map((s) => [s.state, s.description]), + [["success", "pass: the change touches no module of the mesh's graph"], ["success", "no repository check defined"]]); + assert.equal(commentFor(quiet, { defined: false, required: false }), null); // A repository outside the mesh, touching nothing: the gate alone, a pass. assert.equal(statusesFor({ ...base, verdict: "pass", summary: "x", gate: { verdict: "pass", summary: "x" } }).length, 1); // A controller from before the layers: its verdict is the gate's. assert.deepEqual(statusesFor({ ...base, verdict: "warning", summary: "wide" }).map((s) => [s.context, s.state]), - [[CHECK_CONTEXT, "warning"]]); + [[CHECK_CONTEXT, "success"]]); +}); + +// **A required status is success or it blocks** (novox/hq issue 293): branch protection requires +// mesh/merge-gate (and mesh/repo-check on the core repositories) with no administrator override, and the +// forge combines `warning` as a failure. No verdict ever sets `warning` on either; a note is a success that +// says it; only what a person must decide is a failure, with why. +test("no verdict sets warning on a required check; only a person's decision fails", async () => { + const { statusesFor, commentFor, UNDEFINED_REQUIRED, REPO_CHECK_CONTEXT } = await import("../pulls.ts"); + const base = { owner: "novox", repo: "photos", number: 3, commit: "0123456789abcdef", id: "build-2" }; + const noScript = { verdict: "warning", summary: "the repository declares no merge-check.sh" }; + const quiet = { ...base, verdict: "pass", summary: "x", gate: { verdict: "pass", summary: "x" }, "repo-check": noScript }; + for (const verdict of ["pass", "warning", "fail", "error", ""]) { + for (const facts of [{}, { defined: true }, { defined: false }, { defined: false, required: true }, { defined: false, required: false }]) { + const c = { ...base, verdict, summary: "s", gate: { verdict, summary: "s" }, "repo-check": { verdict, summary: "s" } }; + for (const s of statusesFor(c, facts)) assert.notEqual(s.state, "warning", `${verdict} ${JSON.stringify(facts)} → ${s.context}`); + } + } + // A repository without a merge-check.sh where repo-check is not required: success, said. + assert.deepEqual(statusesFor(quiet, { defined: false, required: false })[1], + { state: "success", context: REPO_CHECK_CONTEXT, description: "no repository check defined" }); + // Where it is required — or the protection could not be read — a person decides: a failure, with why. + for (const facts of [{ defined: false, required: true }, { defined: false }]) { + const s = statusesFor(quiet, facts)[1]; + assert.equal(s.state, "failure"); + assert.ok(s.description.length <= 140 && UNDEFINED_REQUIRED.startsWith(s.description.replace(/…$/, ""))); + assert.match(s.description, /^fail: mesh\/repo-check is required/); + assert.match(commentFor(quiet, facts) ?? "", /mesh\/repo-check`: \*\*FAIL\*\* — mesh\/repo-check is required/); + } + // A script that defines its check and said a warning itself: a note, a success. + const noted = statusesFor({ ...quiet, "repo-check": { verdict: "warning", summary: "2 tests skipped" } }, { defined: true })[1]; + assert.deepEqual([noted.state, noted.description], ["success", "pass, with a note: 2 tests skipped"]); + // The gate's notes — a wide rebuild, a problem already so on the base — are successes that say so. + const already = statusesFor({ ...base, verdict: "warning", summary: "s", + gate: { verdict: "warning", summary: "the module check's problems were all so on main already" } })[0]; + assert.deepEqual([already.state, already.description], + ["success", "pass, with a note: the module check's problems were all so on main already"]); + assert.match(commentFor({ ...base, verdict: "warning", summary: "wide", gate: { verdict: "warning", summary: "wide" } }) ?? "", + /PASS, WITH A NOTE/); }); test("a commit status is set on the commit, under the merge check's context", async () => {