diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 98df3536c0..a4f84e0ca3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5850,6 +5850,9 @@ async function maybePublishPrPublicSurface( mergeReadiness, heldForReview, neverClosed, + // A preflight HOLD (e.g. the review lane is unavailable → the review is incomplete) must never render as + // "safe to merge"; the renderer downgrades an otherwise-ready status to a manual-review hold. (#2002) + preflightHeld: preflight.status === "hold", extraCollapsibles: buildPublicSafeCollapsibles({ repo, pr, diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 2b1ee6142b..bf22ec9396 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -275,6 +275,9 @@ export type UnifiedCommentBridgeArgs = { /** 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; + /** Preflight is holding this PR (e.g. the review lane is unavailable) — an otherwise-ready comment then renders + * "held", never "safe to merge". (#2002) */ + preflightHeld?: boolean | undefined; /** Public freshness marker for the posted/updated review comment. Defaults to the current publish time. */ reviewedAt?: string | number | Date | undefined; }; @@ -390,6 +393,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ...(extraCollapsibles !== undefined ? { extraCollapsibles } : {}), ...(args.heldForReview ? { heldForReview: true } : {}), ...(args.neverClosed ? { neverClosed: true } : {}), + ...(args.preflightHeld ? { preflightHeld: 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 8ace750ac7..a2e73bb14d 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -209,6 +209,9 @@ export interface UnifiedCommentContext { /** 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; + /** Preflight is HOLDING this PR (e.g. the review lane is unavailable so the review is incomplete) — an + * otherwise-ready status must then render as "held" (manual review), never "safe to merge". (#2002) */ + preflightHeld?: boolean; /** Public freshness marker for the posted/updated review comment. Rendered as UTC when provided. */ reviewedAt?: string | number | Date | undefined; } @@ -277,6 +280,12 @@ 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"; + // A PREFLIGHT HOLD means the review is INCOMPLETE (e.g. the review lane is unavailable) — it otherwise only lands + // in the advisory readiness score, so an otherwise-ready status would still read "safe to merge" on an + // unfinished review. Downgrade it to a manual-review hold. Applied only to an otherwise-`ready` status, so it can + // only ever DOWNGRADE, never approve. (#2002) — NOTE: a gate `merge` verdict WITH advisory blockers stays + // authoritative-ready by design (the gate already weighed those); tightening THAT is the gate's confidence/bar. + if (status === "ready" && ctx.preflightHeld) return "held"; // Held-vs-closed disposition parity (#8/#9): owner/automation-bot authors may be exempt from auto-close, so a // close verdict on those authors is rendered as held. Guardrail holds are handled above only for otherwise-ready // PRs; they must not downgrade a blocker/close verdict to manual review. diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 386d493f83..939f852b3e 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -4746,7 +4746,7 @@ function validationComponent(pr: PullRequestRecord, preflight: PreflightResult): const missingTests = findingCodes.some((code) => /missing.*test|test.*missing|no_test/i.test(code)); const explicitValidation = hasValidationNote(pr.body ?? ""); if (preflight.status === "hold") { - return { score: 5, evidence: "Preflight is holding this PR; address the blocker before review.", action: "Fix the blocker." }; + return { score: 5, evidence: "Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.", action: "Await review-lane availability." }; } if (missingTests) { // A body validation note is an UNBACKED claim when no test files accompany the change. Cap it just above the diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 86d20cfe6a..be44013728 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1431,7 +1431,7 @@ describe("signal coverage edge cases", () => { expect(comment).toContain("> | Linked issue | ✅ No-issue rationale | PR body explains why no issue is linked. | No action. |"); expect(comment).toContain("> | Change scope | ❌ 8/20 | High review scope from cached public metadata (size label size:L; draft PR; no linked issue context). | Add a concise scope and risk note. |"); - expect(comment).toContain("> | Validation posture | ❌ 5/25 | Preflight is holding this PR; address the blocker before review. | Fix the blocker. |"); + expect(comment).toContain("> | Validation posture | ❌ 5/25 | Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review. | Await review-lane availability. |"); expect(comment).toContain("> | Contributor workload | ✅ 10/10 | Author activity: 29 registered-repo PR(s), 20 merged, 6 issue(s). | No action. |"); expect(comment).toContain("> | Gate result | ⚠️ Not blocking | Advisory; not blocking this PR. | No action. |"); expect(comment).toContain("[JSONbored](https://github.com/JSONbored)"); diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 55840c88cd..7e333cf585 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -339,6 +339,19 @@ describe("buildUnifiedCommentBody", () => { expect(held).not.toContain("> [!TIP]"); }); + it("preflightHeld renders a passing PR as HELD (incomplete review), never 'safe to merge' (#2002)", () => { + const args = { gate: gate({ conclusion: "success" }), panelRows, readinessTotal: 90, changedFiles: 2, mergeReadiness: { ciState: "passed" as const }, footerMarkdown: footer }; + // Without the preflight hold, a success+green PR is the green approve/merge recommendation. + const ready = buildUnifiedCommentBody(args); + expect(ready).toContain("Suggested Action - Approve/Merge"); + // With a preflight hold (e.g. the review lane is unavailable → the review is incomplete), the SAME PR renders + // held (WARNING), never safe-to-merge — the incomplete review can't recommend a merge. + const held = buildUnifiedCommentBody({ ...args, preflightHeld: true }); + expect(held).toContain("> [!WARNING]"); + expect(held).toContain("Suggested Action - Manual Review"); + expect(held).not.toContain("> [!TIP]"); + }); + it("neverClosed renders a gate-failure (close) PR as HELD when CI is green, not reject/close (#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 reject/close recommendation. diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 8d2d3f7a1e..2a6729caa0 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -48,6 +48,18 @@ describe("deriveUnifiedStatus", () => { expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } })).toBe("ready"); }); + it("never renders 'safe to merge' on an incomplete review — a preflight hold downgrades a gate merge verdict (#2002)", () => { + // A preflight HOLD means the review is incomplete (e.g. the review lane is unavailable). A gate `merge` decision + // sets `ready` and the hold otherwise only lands in the advisory readiness score — so this downgrade catches it, + // and an unfinished review can never read as approve/merge. + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } }, { preflightHeld: true })).toBe("held"); + // Regression: a clean merge with no hold STILL renders ready — the downgrade only ever downgrades, never approves. + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } })).toBe("ready"); + // A gate `merge` WITH advisory blockers stays authoritative-ready by design (the gate already weighed them); + // tightening that lives in the gate's confidence/approval bars, not this renderer. See the authoritative-merge test. + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" }, blockers: ["minor"] })).toBe("ready"); + }); + it("blocked for a close verdict or consensus blockers", () => { expect(deriveUnifiedStatus({ ...base, decision: "close" })).toBe("blocked"); expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "unverified" } })).toBe("blocked");