Merge pull request 'gitea: never set warning on the merge check's statuses; a required one blocks (hq issue 293)' (#102) from fix/no-warning-on-a-required-check into main

modules/gitea/delivery.ts

@@ -85,10 +85,16 @@ export function viewComment<T extends { id: number; body: string }>(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) + "…";

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<RepoCheckFacts> {
+  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

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\`\`\`` : "";

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);
 });

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 () => {