fix(review): never approve/merge red CI — close-on-red + CI trigger + status legend + clickable visual table

src/github/backfill.ts

@@ -1902,9 +1902,84 @@
     checkRuns.push(...(result.data.check_runs ?? []));
     if (!hasNextPage(result.link)) break;
   }
   return { check_runs: checkRuns };
 }
 
+const CI_FAILING_CONCLUSIONS = new Set(["failure", "timed_out", "cancelled", "action_required", "startup_failure"]);
+const CI_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]);
+
+export type LiveCiAggregate = {
+  ciState: "passed" | "failed" | "pending" | "unverified";
+  failingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>;
+};
+
+/**
+ * Fetch the head SHA's LIVE CI aggregate over BOTH GitHub Check-runs AND classic commit-statuses. This is the
+ * reviewbot `getAllChecksState` parity that the converged auto-maintain path needs: codecov (codecov/patch,
+ * codecov/project) and many other tools post a classic COMMIT-STATUS, not a check-run — fetching only
+ * `/check-runs` (what the backfill sync does) misses them entirely, which is why a red codecov was reported as
+ * "CI green". We aggregate ANY failing check/status → "failed"; else any still-running → "pending"; else any
+ * present → "passed"; none at all → "unverified". The disposition layer NEVER approves/merges unless "passed",
+ * and closes (non-owner) / holds (owner) on "failed". Best-effort: a fetch error degrades that source to empty.
+ */
+export async function fetchLiveCiAggregate(env: Env, repoFullName: string, headSha: string | null | undefined, token: string | undefined): Promise<LiveCiAggregate> {
+  if (!headSha) return { ciState: "unverified", failingDetails: [] };
+  const failingDetails: LiveCiAggregate["failingDetails"] = [];
+  let total = 0;
+  let anyPending = false;
+
+  // 1) Check-runs (GitHub Actions jobs, CodeQL, app checks).
+  for (let page = 1; page <= PR_DETAIL_MAX_PAGES; page += 1) {
+    const result = await githubJsonWithHeaders<{ check_runs?: Array<GitHubCheckRunPayload & { output?: { title?: unknown; summary?: unknown } }> }>(
+      env,
+      repoFullName,
+      `/commits/${headSha}/check-runs?per_page=100&page=${page}`,
+      token,
+    ).catch(() => undefined);
+    if (!result) break;
+    for (const run of result.data.check_runs ?? []) {
+      total += 1;
+      const conclusion = (run.conclusion ?? "").toLowerCase();
+      const status = (run.status ?? "").toLowerCase();
+      if (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false) {
+        const summary = [run.output?.title, run.output?.summary].find((value): value is string => typeof value === "string" && value.trim().length > 0)?.trim().slice(0, 200);
+        failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) });
+      } else if (conclusion ? CI_PASSING_CONCLUSIONS.has(conclusion) : status === "completed") {
+        // concluded and not failing → passing
+      } else {
+        anyPending = true; // queued / in_progress / not yet concluded
+      }
+    }
+    if (!hasNextPage(result.link)) break;
+  }
+
+  // 2) Classic commit-statuses (codecov/patch, codecov/project, and any other status-API context). The
+  // combined endpoint returns the LATEST status per context, so a context that flipped red→green is counted
+  // once at its current state.
+  const statusResult = await githubJsonWithHeaders<{ statuses?: Array<{ context?: string | null; state?: string | null; description?: string | null; target_url?: string | null }> }>(
+    env,
+    repoFullName,
+    `/commits/${headSha}/status?per_page=100`,
+    token,
+  ).catch(() => undefined);
+  for (const ctx of statusResult?.data.statuses ?? []) {
+    total += 1;
+    const state = (ctx.state ?? "").toLowerCase();
+    const name = ctx.context ?? "status";
+    if (state === "failure" || state === "error") {
+      const summary = typeof ctx.description === "string" ? ctx.description.trim().slice(0, 200) : "";
+      failingDetails.push({ name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) });
+    } else if (state === "success") {
+      // passing
+    } else {
+      anyPending = true; // pending
+    }
+  }
+
+  const ciState: LiveCiAggregate["ciState"] = failingDetails.length > 0 ? "failed" : anyPending ? "pending" : total > 0 ? "passed" : "unverified";
+  return { ciState, failingDetails };
+}
+
 async function fetchPullRequestDetailsFromGraphQl(
   env: Env,
   repoFullName: string,

src/review/unified-comment-bridge.ts

@@ -218,36 +218,42 @@
    *  Public-safe: only URLs + route paths — no private terms. Default OFF (the processor passes this only
    *  when screenshotsAllowed + the PR touches web-visible files). */
   beforeAfter?: CaptureRoute[] | undefined;
 };
 
 /**
- * Build the "Visual preview" collapsible from the before/after capture routes — a markdown table of image
- * cells pointing at the public /gittensory/shot URLs. Uses GitHub markdown image syntax `![](url)` rather
- * than raw `<img>` tags ON PURPOSE: the unified renderer's `details()` HTML-escapes a collapsible body (a
- * security control so caller text can't inject structure-changing HTML), which would turn a literal `<img>`
- * into inert `&lt;img&gt;` text — markdown image syntax has no angle brackets, so it survives the escape and
- * still renders as an image. Public-safe by construction: every cell is a route path or a shot URL (no
- * private rubric/scoring terms). Returns null when nothing is renderable (no route has any shot URL), so the
- * section is omitted entirely rather than showing an empty table.
+ * Build the "Visual preview" collapsible from the before/after capture routes — a clean table whose cells are
+ * CLICKABLE THUMBNAILS: a small `<img>` (GitHub caps it to the column width) wrapped in an `<a href>` to the
+ * SAME full-resolution shot, so a click opens the screenshot full-size. One row per route per viewport
+ * (desktop / mobile), with the route path as the caption and a before (production) vs after (this PR's preview)
+ * column. Emitted as TRUSTED raw HTML (`rawHtml: true`) so the `<a>/<img>` survive — public-safe by
+ * construction: every value is a first-party minted /gittensory/shot URL or a route path (no private rubric /
+ * scoring terms), and a stray `"` in a URL is neutralized so it can't break out of the attribute. Returns null
+ * when nothing is renderable (no route has any shot URL), so the section is omitted rather than shown empty.
  */
 export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedCollapsible | null {
-  const rows = routes
-    .filter((route) => route.beforeUrl || route.afterUrl || route.beforeUrlMobile || route.afterUrlMobile)
-    .map((route) => {
-      // Escape `(`/`)`/`]` in the URL so a crafted shot URL can't break out of the markdown image token; the
-      // URLs are first-party (we mint them), but this keeps the cell robust regardless.
-      const cell = (url: string | undefined): string => (url ? `![preview](${url.replace(/[()\]]/g, encodeURIComponent)})` : "—");
-      return `| \`${route.path.replace(/\|/g, "\\|")}\` | ${cell(route.beforeUrl)} | ${cell(route.afterUrl)} |`;
-    });
+  const attr = (value: string): string => value.replace(/"/g, "%22");
+  const alt = (value: string): string => value.replace(/"/g, "'");
+  const cell = (url: string | undefined, label: string): string =>
+    url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${alt(label)}" src="${attr(url)}"></a>` : "—";
+  const rows: string[] = [];
+  for (const route of routes) {
+    const path = `\`${route.path.replace(/\|/g, "\\|")}\``;
+    if (route.beforeUrl || route.afterUrl) {
+      rows.push(`| ${path} | desktop | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} |`);
+    }
+    if (route.beforeUrlMobile || route.afterUrlMobile) {
+      rows.push(`| ${path} | mobile | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)`)} |`);
+    }
+  }
   if (rows.length === 0) return null;
   const body = [
-    "| Route | Before (production) | After (this PR's preview) |",
-    "| --- | --- | --- |",
+    "| Route | Viewport | Before (production) | After (this PR's preview) |",
+    "| --- | --- | --- | --- |",
     ...rows,
     "",
-    "_Before = production · After = this PR's preview deploy._",
+    "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy._",
   ].join("\n");
-  return { title: "Visual preview", body };
+  return { title: "Visual preview", body, rawHtml: true };
 }
 
 /**

src/review/unified-comment.ts

@@ -179,9 +179,12 @@
 }
 
 /** A collapsed section (gittensory side: signal definitions, contributor next steps, …). */
 export interface UnifiedCollapsible {
   title: string;
   body: string;
+  /** When true the body is TRUSTED raw HTML and is NOT angle-escaped — used only by the visual before/after
+   *  table (a table of `<a href><img>` clickable thumbnails the bridge builds from first-party shot URLs). */
+  rawHtml?: boolean;
 }
 
 /** The host (gittensory) side: brand, readiness score, signals, sections, re-run, footer. */
@@ -214,6 +217,11 @@
 /** Derive the single unified status from reviewbot's decision/recs/CI + the host override. */
 export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedCommentContext = {}): UnifiedCommentStatus {
   if (ctx.statusOverride) return ctx.statusOverride;
+  // A failing CI is NEVER "safe to merge": a red CI downgrades any otherwise-ready/merge verdict to blocked (the
+  // disposition layer then closes it for a non-owner author / holds it open for the owner). This runs BEFORE the
+  // explicit-verdict switch so an optimistic gate "merge" can't render a green "safe to merge" headline over a
+  // red CI — the exact bug where a PR with a failing codecov/patch showed "Approved — safe to merge".
+  if (input.readiness?.ciState === "failed") return "blocked";
   // An explicit gate verdict is authoritative — it already weighed the reviewers + guardrails.
   switch (input.decision) {
     case "merge":
@@ -231,7 +239,6 @@
   const recs = input.recommendations ?? [];
   const hasConsensusBlocker = input.consensusBlocker ?? (input.blockers ?? []).length > 0;
   if (recs.includes("close") || hasConsensusBlocker) return "blocked";
-  if (input.readiness?.ciState === "failed") return "held";
   if (recs.length === 0) return "advisory";
   if ((input.failedCount ?? 0) > 0 || recs.some((r) => r !== "merge")) return "held";
   return "ready";
@@ -361,6 +368,13 @@
   return `<details><summary><b>${safeTitle}</b>${safeSub}</summary>\n\n${escapePublicHtmlAngles(body)}\n</details>`;
 }
 
+/** Like details(), but the body is TRUSTED raw HTML and is NOT angle-escaped. Used only for the visual
+ *  before/after table, whose body is built solely from first-party minted shot URLs + route paths (see
+ *  buildBeforeAfterCollapsible). The title is still escaped. */
+function detailsRaw(title: string, body: string): string {
+  return `<details><summary><b>${escapePublicHtmlAngles(title)}</b></summary>\n\n${body}\n</details>`;
+}
+
 /** Wrap the assembled body in a GitHub alert blockquote — this is the full-comment colored sidebar. */
 function asAlert(alert: string, inner: string): string {
   const quoted = inner
@@ -406,10 +420,15 @@
   const nits = dedupeLines(input.nits ?? []);
   if (nits.length) blocks.push(details("Nits", bullets(nits), `${nits.length} non-blocking`));
   for (const c of ctx.extraCollapsibles ?? []) {
-    if (c.body.trim()) blocks.push(details(c.title, c.body.trim()));
+    if (c.body.trim()) blocks.push(c.rawHtml ? detailsRaw(c.title, c.body.trim()) : details(c.title, c.body.trim()));
   }
 
   if (ctx.reRunLabel) blocks.push(`- [ ] ${ctx.reRunLabel}`);
+  // Color-coded status legend (key) — a quiet footer mapping each headline color/icon to its meaning, so a
+  // reader can tell at a glance what "this PR's status" means. Squares are the SAME ones used in the headline.
+  blocks.push(
+    `<sub>${STATUS_META.ready.square} Safe / merged · ${STATUS_META.advisory.square} Advisory · ${STATUS_META.held.square} Held for review · ${STATUS_META.blocked.square} Blocked / closed</sub>`,
+  );
   if (ctx.footerMarkdown?.trim()) blocks.push(`---\n${ctx.footerMarkdown.trim()}`);
 
   return asAlert(meta.alert, blocks.join("\n\n"));