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

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