From af0c16298d57062f55412b63fb815280d97741ad Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 01:30:39 -0700 Subject: [PATCH] fix(review): hold PRs whose review can't complete instead of "safe to merge" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When preflight cannot complete the review (the repo's review lane is unknown/inactive), the disposition previously still rendered an otherwise-ready PR as approve/merge — the preflight hold only fed the advisory readiness score, never the merge/close disposition. An incomplete review must never read as "safe to merge". Thread `preflightHeld` (preflight.status === "hold") through the comment bridge into `deriveUnifiedStatus`, which downgrades an otherwise-`ready` status to a manual-review hold. Applied last and only to a would-be-`ready` status, so it can only ever downgrade, never approve. A gate `merge` verdict carrying advisory blockers stays authoritative-ready by design — the gate already weighed those; tightening that lives in the gate's confidence/approval bars, not this renderer. Also correct the preflight-hold validation-component message to describe the real cause (review lane unavailable) rather than a nonexistent blocker. Advances #1936. Refs #2002. --- src/queue/processors.ts | 3 +++ src/review/unified-comment-bridge.ts | 4 ++++ src/review/unified-comment.ts | 9 +++++++++ src/signals/engine.ts | 2 +- test/unit/signals-coverage.test.ts | 2 +- test/unit/unified-comment-bridge.test.ts | 13 +++++++++++++ test/unit/unified-comment.test.ts | 12 ++++++++++++ 7 files changed, 43 insertions(+), 2 deletions(-) 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");