refactor(queue): extract the public-comment merge-facts derivation from maybePublishPrPublicSurface (#4607) (#5787)

`maybePublishPrPublicSurface` is 2,600+ lines, and #4607's stated cost is that "most cross-cutting
review-pipeline concerns live as unnamed inline blocks in one function rather than named,
independently-testable steps". This lifts one such block into a named step.

`derivePublicCommentMergeFacts` derives the five merge/disposition values the public PR comment
renders -- ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed -- from the live CI
aggregate, the live merge-state refresh, the repo settings and the changed files. Its output IS
buildUnifiedCommentBody's input contract for those fields, so the seam is a real step rather than an
arbitrary cut. It is pure: the `incr()` metrics emit that sat in the middle of the block stays at the
call site, since it reads the gate conclusion the derivation never sees.

These flags exist so the COMMENT agrees with the ACTION the disposition planner takes -- heldForReview
for a guardrail-touching diff (#guarded-hold-comment), neverClosed for an owner/protected-automation
author (#8/#9). Until now they were only reachable by standing up a whole webhook delivery: the
renderer's own suite passes them in as hand-written literals, and the queue suites exercise them
transitively. They now have direct table tests, including the empty-changed-file-list fail-safe (an
unresolved file list HOLDS for review rather than claiming safe-to-merge), which had no test at all.

No behavior change: the 882 tests in the existing queue suites pass unchanged.

Author: glorydavid03023

src/queue/processors.ts

@@ -1900,6 +1900,71 @@ export async function regatePullRequest(
   }
 }
 
+/** The merge/disposition facts the public PR comment renders, derived from the live CI + merge-state refresh
+ *  (#4607). Extracted out of `maybePublishPrPublicSurface`'s inline body: it is pure, and its output IS the
+ *  renderer's input contract (`buildUnifiedCommentBody`'s ciState/mergeStateLabel/mergeReadiness/heldForReview/
+ *  neverClosed arguments), so the seam is a real step rather than an arbitrary cut. */
+export type PublicCommentMergeFacts = {
+  ciState: MergeReadiness["ciState"];
+  mergeStateLabel: string | undefined;
+  mergeReadiness: MergeReadiness;
+  heldForReview: boolean;
+  neverClosed: boolean;
+};
+
+/** Public-safe projection of one failing check: name + short reason only, dropping absent optionals so the
+ *  rendered chip never carries an `undefined` summary/url. */
+function publicCheckFailureDetails(details: LiveCiAggregate["failingDetails"]): CheckFailureDetail[] {
+  return details.map((detail) => ({
+    name: detail.name,
+    ...(detail.summary ? { summary: detail.summary } : {}),
+    ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}),
+  }));
+}
+
+/**
+ * Derive the public comment's merge-readiness + disposition flags (#4607). Pure: no D1, no GitHub client, no
+ * metrics — the caller keeps the `incr()` emit, which reads the gate conclusion this function never sees.
+ *
+ * The two flags exist so the COMMENT agrees with the ACTION the disposition planner will take:
+ * - `heldForReview` — a clean, green PR whose diff touches a hard-guardrail path is held for owner review by
+ *   `planAgentMaintenanceActions`, never auto-merged, so the comment must not headline "safe to merge"
+ *   (#guarded-hold-comment). Uses the same shared `isGuardrailHit` the planner uses, not a second copy.
+ * - `neverClosed` — the disposition never auto-closes a repo-owner or protected-automation PR, so a gate
+ *   "close" verdict on one must headline "held", not "Closed" (#8/#9).
+ */
+export function derivePublicCommentMergeFacts(args: {
+  liveMergeState: string | undefined;
+  mergeableState: string | null | undefined;
+  authorLogin: string | null | undefined;
+  liveCi: Pick<LiveCiAggregate, "ciState" | "failingDetails" | "nonRequiredFailingDetails">;
+  settings: Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants">;
+  unifiedFiles: Awaited<ReturnType<typeof listPullRequestFiles>>;
+  repoFullName: string;
+}): PublicCommentMergeFacts {
+  const mergeStateLabel = args.liveMergeState ?? args.mergeableState ?? undefined; // fail-safe to the stored value
+  const ciState: MergeReadiness["ciState"] =
+    args.liveCi.ciState === "passed" ? "passed" : args.liveCi.ciState === "failed" ? "failed" : "unverified";
+  // Per-failed-check WHY (codecov %/test/lint reason) from each check-run output or commit-status description.
+  const failingDetails = publicCheckFailureDetails(args.liveCi.failingDetails);
+  // Non-required-but-red checks (#4414-class advisory holds): surfaced so a flagged check is never silently
+  // invisible, but never folded into failingChecks/failingDetails -- those two drive ciState/close.
+  const nonRequiredFailingDetails = publicCheckFailureDetails(args.liveCi.nonRequiredFailingDetails);
+  const mergeReadiness: MergeReadiness = {
+    ciState,
+    ...(mergeStateLabel ? { mergeStateLabel } : {}),
+    ...(failingDetails.length > 0 ? { failingChecks: failingDetails.map((detail) => detail.name) } : {}),
+    ...(failingDetails.length > 0 ? { failingDetails } : {}),
+    ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}),
+  };
+  const heldForReview = isGuardrailHit(changedPathsForGuardrail(args.unifiedFiles), resolveHardGuardrailGlobs(args.settings));
+  const repoOwner = args.repoFullName.includes("/") ? args.repoFullName.slice(0, args.repoFullName.indexOf("/")) : "";
+  const authorLogin = args.authorLogin ?? "";
+  const neverClosed =
+    (authorLogin.length > 0 && authorLogin.toLowerCase() === repoOwner.toLowerCase()) || isProtectedAutomationAuthor(args.authorLogin);
+  return { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed };
+}
+
 export function changedPathsForGuardrail(
   files: Awaited<ReturnType<typeof listPullRequestFiles>>,
 ): string[] {
@@ -9833,40 +9898,15 @@ async function maybePublishPrPublicSurface(
       // The stored pr.mergeableState lags GitHub's async recompute, and the gate's own check/review publication can
       // also advance mergeability after readiness ran, so refresh at this post-publish boundary.
       const liveMergeState = await refreshLiveMergeState(env, repoFullName, webhook.liveFacts, pr.number, token, admissionKey).catch(() => undefined);
-      const mergeStateLabel = liveMergeState ?? pr.mergeableState; // fail-safe to the stored value
-      const ciState: MergeReadiness["ciState"] =
-        liveCi.ciState === "passed"
-          ? "passed"
-          : liveCi.ciState === "failed"
-            ? "failed"
-            : "unverified";
-      // Per-failed-check WHY (codecov %/test/lint reason) from each check-run output or commit-status
-      // description — capped + public-safe (name + short reason only). The renderer lists these under the CI chip.
-      const failingDetails: CheckFailureDetail[] = liveCi.failingDetails.map(
-        (detail) => ({
-          name: detail.name,
-          ...(detail.summary ? { summary: detail.summary } : {}),
-          ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}),
-        }),
-      );
-      // Non-required-but-red checks (#4414-class advisory holds): surfaced so a flagged check is never silently
-      // invisible, but never folded into failingChecks/failingDetails -- those two drive ciState/close.
-      const nonRequiredFailingDetails: CheckFailureDetail[] = liveCi.nonRequiredFailingDetails.map(
-        (detail) => ({
-          name: detail.name,
-          ...(detail.summary ? { summary: detail.summary } : {}),
-          ...(detail.detailsUrl ? { detailsUrl: detail.detailsUrl } : {}),
-        }),
-      );
-      const mergeReadiness: MergeReadiness = {
-        ciState,
-        ...(mergeStateLabel ? { mergeStateLabel } : {}),
-        ...(failingDetails.length > 0
-          ? { failingChecks: failingDetails.map((detail) => detail.name) }
-          : {}),
-        ...(failingDetails.length > 0 ? { failingDetails } : {}),
-        ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}),
-      };
+      const { ciState, mergeStateLabel, mergeReadiness, heldForReview, neverClosed } = derivePublicCommentMergeFacts({
+        liveMergeState,
+        mergeableState: pr.mergeableState,
+        authorLogin: pr.authorLogin,
+        liveCi,
+        settings,
+        unifiedFiles,
+        repoFullName,
+      });
       // The public comment must match the authoritative Gate check-run conclusion.
       const commentGate = gateEvaluation;
       // Observability (#reviews-dashboard): record the would-be gate verdict so the Grafana panel shows the
@@ -9875,27 +9915,6 @@ async function maybePublishPrPublicSurface(
         repo: repoFullName,
         conclusion: commentGate.conclusion,
       });
-      // Guarded-hold (#guarded-hold-comment): a clean+green PR whose diff touches a hard-guardrail path is HELD
-      // for owner review by the disposition (planAgentMaintenanceActions), never auto-merged — so the comment
-      // must render "held for review", not "✅ safe to merge". Compute the SAME guardrail-hit the disposition uses
-      // (shared isGuardrailHit) and thread it so the signal and the action agree (the #4220 class, clean variant).
-      const commentHardGuardrailGlobs = resolveHardGuardrailGlobs(settings);
-      const heldForReview = isGuardrailHit(
-        changedPathsForGuardrail(unifiedFiles),
-        commentHardGuardrailGlobs,
-      );
-      // Held-vs-closed parity (#8/#9): the disposition NEVER auto-closes an owner / automation-bot PR, so a gate
-      // "close" verdict on one must headline "held", not "Closed". Compute the same author classification the
-      // planner uses (repo-owner login match + protected automation author) and thread it to the comment.
-      const commentRepoOwner = repoFullName.includes("/")
-        ? repoFullName.slice(0, repoFullName.indexOf("/"))
-        : "";
-      const commentAuthorLogin = pr.authorLogin ?? "";
-      const neverClosed =
-        (commentAuthorLogin.length > 0 &&
-          commentAuthorLogin.toLowerCase() ===
-            commentRepoOwner.toLowerCase()) ||
-        isProtectedAutomationAuthor(pr.authorLogin);
       const { rows, readinessTotal } = buildPublicPrPanelSignalRows({
         repo,
         pr,

test/unit/processors-public-comment-merge-facts.test.ts

@@ -0,0 +1,150 @@
+import { describe, expect, it } from "vitest";
+
+import { derivePublicCommentMergeFacts } from "../../src/queue/processors";
+import type { PullRequestFileRecord, RepositorySettings } from "../../src/types";
+
+// `src/rules/**` is one of the ENGINE_DECISION_GUARDRAIL_GLOBS defaults (src/review/guardrail-config.ts), so a
+// diff touching it is a hard-guardrail hit; README.md is not guarded by any default glob.
+const GUARDED_FILE = { path: "src/rules/advisory.ts" } as PullRequestFileRecord;
+const UNGUARDED_FILE = { path: "README.md" } as PullRequestFileRecord;
+
+const NO_GUARDRAIL_OVERRIDES = {
+  hardGuardrailGlobs: [],
+  hardGuardrailGlobsOverridesInvariants: false,
+} as Pick<RepositorySettings, "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants">;
+
+function facts(overrides: Partial<Parameters<typeof derivePublicCommentMergeFacts>[0]> = {}) {
+  return derivePublicCommentMergeFacts({
+    liveMergeState: "clean",
+    mergeableState: "dirty",
+    authorLogin: "contributor",
+    liveCi: { ciState: "passed", failingDetails: [], nonRequiredFailingDetails: [] },
+    settings: NO_GUARDRAIL_OVERRIDES,
+    unifiedFiles: [UNGUARDED_FILE],
+    repoFullName: "acme/widgets",
+    ...overrides,
+  });
+}
+
+describe("derivePublicCommentMergeFacts() — mergeStateLabel (#4607)", () => {
+  it("prefers the live merge state, falls back to the stored one, and omits the label when neither is known", () => {
+    expect(facts({ liveMergeState: "clean", mergeableState: "dirty" }).mergeStateLabel).toBe("clean");
+    // The live refresh can fail (it is a `.catch(() => undefined)` at the call site) — fail safe to the stored value.
+    expect(facts({ liveMergeState: undefined, mergeableState: "dirty" }).mergeStateLabel).toBe("dirty");
+    const unknown = facts({ liveMergeState: undefined, mergeableState: null });
+    expect(unknown.mergeStateLabel).toBeUndefined();
+    expect(unknown.mergeReadiness).not.toHaveProperty("mergeStateLabel");
+  });
+});
+
+describe("derivePublicCommentMergeFacts() — ciState (#4607)", () => {
+  it("passes through passed/failed and collapses everything else to unverified", () => {
+    expect(facts({ liveCi: { ciState: "passed", failingDetails: [], nonRequiredFailingDetails: [] } }).ciState).toBe("passed");
+    expect(facts({ liveCi: { ciState: "failed", failingDetails: [], nonRequiredFailingDetails: [] } }).ciState).toBe("failed");
+    // Both "pending" and "unverified" mean "we cannot claim green" — the comment must never imply a pass.
+    expect(facts({ liveCi: { ciState: "pending", failingDetails: [], nonRequiredFailingDetails: [] } }).ciState).toBe("unverified");
+    expect(facts({ liveCi: { ciState: "unverified", failingDetails: [], nonRequiredFailingDetails: [] } }).ciState).toBe("unverified");
+  });
+});
+
+describe("derivePublicCommentMergeFacts() — failing-check projection (#4607)", () => {
+  it("omits the failing keys entirely when nothing is red", () => {
+    const { mergeReadiness } = facts();
+    expect(mergeReadiness).toEqual({ ciState: "passed", mergeStateLabel: "clean" });
+  });
+
+  it("projects name + optional summary/detailsUrl, dropping absent optionals", () => {
+    const { mergeReadiness } = facts({
+      liveCi: {
+        ciState: "failed",
+        failingDetails: [
+          { name: "codecov/patch", summary: "77% of diff hit", detailsUrl: "https://ci.example/1" },
+          { name: "lint" },
+        ],
+        nonRequiredFailingDetails: [],
+      },
+    });
+    expect(mergeReadiness.failingChecks).toEqual(["codecov/patch", "lint"]);
+    expect(mergeReadiness.failingDetails).toEqual([
+      { name: "codecov/patch", summary: "77% of diff hit", detailsUrl: "https://ci.example/1" },
+      { name: "lint" },
+    ]);
+    // An absent summary/detailsUrl must be OMITTED, never rendered as an `undefined` chip.
+    expect(mergeReadiness.failingDetails?.[1]).not.toHaveProperty("summary");
+    expect(mergeReadiness.failingDetails?.[1]).not.toHaveProperty("detailsUrl");
+    expect(mergeReadiness).not.toHaveProperty("nonRequiredFailingDetails");
+  });
+
+  it("keeps non-required red checks visible WITHOUT folding them into failingChecks (#4414-class)", () => {
+    const { mergeReadiness, ciState } = facts({
+      liveCi: {
+        ciState: "passed",
+        failingDetails: [],
+        nonRequiredFailingDetails: [{ name: "advisory-scan", summary: "1 note", detailsUrl: "https://ci.example/2" }],
+      },
+    });
+    // The whole point: a non-required red check is surfaced, but it must not turn the PR red or drive close.
+    expect(ciState).toBe("passed");
+    expect(mergeReadiness).not.toHaveProperty("failingChecks");
+    expect(mergeReadiness).not.toHaveProperty("failingDetails");
+    expect(mergeReadiness.nonRequiredFailingDetails).toEqual([
+      { name: "advisory-scan", summary: "1 note", detailsUrl: "https://ci.example/2" },
+    ]);
+  });
+});
+
+describe("derivePublicCommentMergeFacts() — heldForReview (#guarded-hold-comment, #4607)", () => {
+  it("holds a PR whose diff touches a hard-guardrail path, and does not hold one that doesn't", () => {
+    expect(facts({ unifiedFiles: [GUARDED_FILE] }).heldForReview).toBe(true);
+    expect(facts({ unifiedFiles: [UNGUARDED_FILE] }).heldForReview).toBe(false);
+    expect(facts({ unifiedFiles: [UNGUARDED_FILE, GUARDED_FILE] }).heldForReview).toBe(true);
+  });
+
+  it("fails SAFE: an empty changed-file list holds for review rather than claiming safe-to-merge", () => {
+    // isGuardrailHit (src/signals/change-guardrail.ts) treats "no known changed paths" as a hit — the file list
+    // may simply not have resolved, and a false "safe to merge" on an unguarded-looking diff is the dangerous
+    // direction. Pinned here because the extraction makes this fail-safe reachable in a unit test for the first
+    // time; previously it could only be hit by standing up a whole webhook delivery.
+    expect(facts({ unifiedFiles: [] }).heldForReview).toBe(true);
+  });
+
+  it("honours a repo that overrides the invariant guardrail globs away", () => {
+    expect(
+      facts({
+        unifiedFiles: [GUARDED_FILE],
+        settings: { hardGuardrailGlobs: [], hardGuardrailGlobsOverridesInvariants: true } as Pick<
+          RepositorySettings,
+          "hardGuardrailGlobs" | "hardGuardrailGlobsOverridesInvariants"
+        >,
+      }).heldForReview,
+    ).toBe(false);
+  });
+});
+
+describe("derivePublicCommentMergeFacts() — neverClosed (#8/#9, #4607)", () => {
+  it("is true for the repo owner, case-insensitively", () => {
+    expect(facts({ repoFullName: "acme/widgets", authorLogin: "acme" }).neverClosed).toBe(true);
+    expect(facts({ repoFullName: "Acme/widgets", authorLogin: "aCmE" }).neverClosed).toBe(true);
+  });
+
+  it("is true for a protected automation author who is NOT the owner", () => {
+    expect(facts({ authorLogin: "dependabot[bot]" }).neverClosed).toBe(true);
+    expect(facts({ authorLogin: "github-actions[bot]" }).neverClosed).toBe(true);
+  });
+
+  it("is false for an ordinary contributor", () => {
+    expect(facts({ authorLogin: "contributor" }).neverClosed).toBe(false);
+  });
+
+  it("is false — never accidentally true — when the author login is missing", () => {
+    // Guard against the empty-string trap: a malformed repoFullName yields an empty owner, and "" === "" would
+    // otherwise make an author-less PR look like the repo owner and become un-closable.
+    expect(facts({ authorLogin: null, repoFullName: "no-slash-name" }).neverClosed).toBe(false);
+    expect(facts({ authorLogin: undefined, repoFullName: "acme/widgets" }).neverClosed).toBe(false);
+    expect(facts({ authorLogin: "", repoFullName: "no-slash-name" }).neverClosed).toBe(false);
+  });
+
+  it("treats a repoFullName with no owner segment as having no owner", () => {
+    expect(facts({ repoFullName: "no-slash-name", authorLogin: "contributor" }).neverClosed).toBe(false);
+  });
+});