diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e5999a0f93..b89c4b9aa9 100644 --- a/src/queue/processors.ts +++ b/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; + settings: Pick; + unifiedFiles: Awaited>; + 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>, ): 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, diff --git a/test/unit/processors-public-comment-merge-facts.test.ts b/test/unit/processors-public-comment-merge-facts.test.ts new file mode 100644 index 0000000000..e5b01539b5 --- /dev/null +++ b/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; + +function facts(overrides: Partial[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); + }); +});