diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 547e378e7e..8b8b97820d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2913,6 +2913,13 @@ async function maybePublishPrPublicSurface( unifiedFiles.map((file) => file.path), await loadHardGuardrailGlobs(env, repoFullName), ); + // 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, profile, detection, queueHealth, collisions, preflight, settings, gate: commentGate, duplicateWinnerEnabled }); // Visual before/after capture (visual-capture port). Fires ONLY when (1) the global flag + per-repo // cutover gate both allow it (screenshotsAllowed) AND (2) the PR touches WEB-VISIBLE files (isVisualPath @@ -2959,6 +2966,7 @@ async function maybePublishPrPublicSurface( ...(aiReview?.reviewerCount !== undefined ? { reviewerCount: aiReview.reviewerCount } : {}), mergeReadiness, heldForReview, + neverClosed, extraCollapsibles: buildPublicSafeCollapsibles({ repo, pr, diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index f00bf4a4ba..67374593ec 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -221,6 +221,9 @@ export type UnifiedCommentBridgeArgs = { /** The disposition holds this PR for owner review because its diff touches a hard-guardrail path — so an * otherwise-ready comment renders "held for review" instead of "safe to merge". (#guarded-hold-comment) */ heldForReview?: boolean | undefined; + /** The author is the repo owner or a protected automation bot — never auto-closed, so a gate "close" verdict + * renders as "held" rather than "Closed" (#8/#9). */ + neverClosed?: boolean | undefined; }; /** @@ -321,6 +324,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ...(args.reRunLabel !== undefined ? { reRunLabel: args.reRunLabel } : {}), ...(extraCollapsibles !== undefined ? { extraCollapsibles } : {}), ...(args.heldForReview ? { heldForReview: true } : {}), + ...(args.neverClosed ? { neverClosed: true } : {}), }); // Prepend the marker verbatim (matching the legacy body, which leads with the marker then a blank line) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 59ad968b05..65de582e96 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -206,6 +206,9 @@ export interface UnifiedCommentContext { /** The host's disposition holds this PR for owner review (its diff touches a hard-guardrail path), so an * otherwise-ready status renders as "held for review" instead of "safe to merge". (#guarded-hold-comment) */ heldForReview?: boolean; + /** The PR's author is the repo owner or a protected automation bot — the disposition NEVER auto-closes them, + * so a gate "close" verdict renders as "held", not "Closed" (#8/#9). */ + neverClosed?: boolean; } const STATUS_META: Record = { @@ -269,6 +272,15 @@ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedComme // PR that won't actually merge). Applied LAST so it only ever downgrades an otherwise-ready status — a real // CI / merge-state / gate block above still wins. (#guarded-hold-comment) if (status === "ready" && ctx.heldForReview) return "held"; + // Held-vs-closed disposition parity (#8/#9): a gate "close" verdict does NOT always close the PR. An + // owner/automation-bot author is NEVER auto-closed, and a guarded-path PR is held for owner review UNLESS a + // RED required check forces the close (ciState "failed" mirrors the disposition's redVerifiedRequiredCi). Render + // those as "held" so the headline matches the action (#4220 class); a genuine contributor close — red required + // CI, or a non-guarded block — still headlines "Closed". + if (input.decision === "close") { + if (ctx.neverClosed) return "held"; + if (ctx.heldForReview && input.readiness?.ciState !== "failed") return "held"; + } return status; } diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 974e02d7eb..f1fb319410 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -229,6 +229,18 @@ describe("buildUnifiedCommentBody", () => { expect(held).toContain("Held for maintainer review"); expect(held).not.toContain("> [!TIP]"); }); + + it("neverClosed renders a gate-failure (close) PR as HELD, not 'Closed' (#8/#9)", () => { + const args = { gate: gate({ conclusion: "failure" }), panelRows, readinessTotal: 40, changedFiles: 2, mergeReadiness: { ciState: "passed" as const }, footerMarkdown: footer }; + // A contributor close → the red "Closed" headline. + const closed = buildUnifiedCommentBody(args); + expect(closed).toContain("Closed"); + // The SAME verdict on an owner / automation-bot PR (never auto-closed) renders held, not Closed. + const held = buildUnifiedCommentBody({ ...args, neverClosed: true }); + expect(held).toContain("> [!WARNING]"); + expect(held).toContain("Held for maintainer review"); + expect(held).not.toContain("Closed"); + }); }); // ── Reconciliation invariant (#1016): comment-verdict ↔ gate-conclusion alignment ────────────────── diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index a4ff2949f7..0d58902d3c 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -72,12 +72,26 @@ describe("deriveUnifiedStatus", () => { it("a guarded-path hold downgrades a would-be-ready PR to held — never 'safe to merge' (#guarded-hold-comment)", () => { // A clean+green PR that touches a hard-guardrail path is HELD for owner review, so the comment says held. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } }, { heldForReview: true })).toBe("held"); - // It only downgrades an otherwise-ready status — a real close/blocked verdict still wins over the hold. - expect(deriveUnifiedStatus({ ...base, decision: "close" }, { heldForReview: true })).toBe("blocked"); + // A guarded close with RED required CI still closes (the red check overrides the guardrail hold), so the + // headline stays "blocked"/Closed. The held-vs-closed nuance for non-red guarded closes is covered below. + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { heldForReview: true })).toBe("blocked"); // Without the hold flag, the same clean+green PR is ready. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } }, { heldForReview: false })).toBe("ready"); }); + it("renders a non-closing disposition as held, not Closed (#8/#9)", () => { + // #9: an owner / automation-bot author is NEVER auto-closed → a gate "close" verdict renders held, even on red CI. + expect(deriveUnifiedStatus({ ...base, decision: "close" }, { neverClosed: true })).toBe("held"); + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { neverClosed: true })).toBe("held"); + // #8: a guarded-path close is the disposition's HOLD (owner review) unless a red required check forces it. + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "passed" } }, { heldForReview: true })).toBe("held"); + expect(deriveUnifiedStatus({ ...base, decision: "close" }, { heldForReview: true })).toBe("held"); // CI not yet reported → held + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { heldForReview: true })).toBe("blocked"); // red required CI → real close + // A genuine contributor close (no guard, not owner/bot) still headlines Closed/blocked. + expect(deriveUnifiedStatus({ ...base, decision: "close" })).toBe("blocked"); + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } })).toBe("blocked"); + }); + it("honors an explicit host status override", () => { expect(deriveUnifiedStatus({ ...base, decision: "close" }, { statusOverride: "ready" })).toBe("ready"); });