From 5518f8e41760c80902b874e07beb0fe0785d8104 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 14 Jul 2026 02:49:18 -0700 Subject: [PATCH] fix(review): treat an unstable GitHub mergeable_state as a held verdict in the PR panel A PR whose mergeable_state is "unstable" (e.g. a non-required third-party check reporting non-success) was rendered "approve/merge recommended" / "safe to merge" even though the disposition planner already withholds the merge for any non-"clean" mergeable_state -- directly contradicting the comment on the same PR. deriveUnifiedStatus only downgraded ready->held for dirty/behind; unstable was a gap, not a deliberate exclusion (unlike unknown and blocked, which are already documented as intentionally non-downgrading). --- src/review/unified-comment.ts | 12 ++++++++---- test/unit/unified-comment.test.ts | 8 +++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index c13b2abddb..38d611fc46 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -340,12 +340,16 @@ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedComme return "held"; } // Merge-state readiness follows the same rule: do not claim "safe to merge" while GitHub says the branch is - // dirty/behind, but keep the comment in a held/advisory tone instead of turning readiness into a blocker. - // Other states — clean, a not-yet-computed `unknown`, or a `blocked` that the bot's own pending approval will clear — do not downgrade. - // (#ready-needs-mergeable) + // dirty/behind/unstable, but keep the comment in a held/advisory tone instead of turning readiness into a + // blocker. `unstable` (#pr-5288-confusing-verdict) covers a non-required check reporting non-success (e.g. a + // third-party App's own check) — exactly the state agentHoldAuditDetail (processors.ts) already treats as a + // real merge-withhold reason (`mergeableState !== "clean"`), so without this the comment could say "safe to + // merge" on the SAME PR the disposition planner is actively holding, which is the contradiction #5288 reported. + // Other states — clean, a not-yet-computed `unknown`, or a `blocked` that the bot's own pending approval will + // clear — do not downgrade. (#ready-needs-mergeable) if (status === "ready" && input.readiness?.mergeStateLabel) { const mergeState = input.readiness.mergeStateLabel.toLowerCase(); - if (mergeState === "dirty" || mergeState === "behind") return "held"; + if (mergeState === "dirty" || mergeState === "behind" || mergeState === "unstable") return "held"; } // Guarded-hold gate — a clean + green PR whose diff touches a hard-guardrail path (CI config, the review // engine, visuals) is HELD for owner review by the disposition, never auto-merged. The comment must then say diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 17468ac966..102dc4f125 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -67,11 +67,17 @@ describe("deriveUnifiedStatus", () => { expect(deriveUnifiedStatus({ ...base, recommendations: [], blockers: ["leaks a secret"] })).toBe("blocked"); }); - it("a non-mergeable merge state is advisory — dirty/behind hold, but never block a merge verdict (#4220)", () => { + it("a non-mergeable merge state is advisory — dirty/behind/unstable hold, but never block a merge verdict (#4220, #pr-5288-confusing-verdict)", () => { // The reported bug: green CI + merge verdict but a `dirty` base conflict rendered "safe to merge". expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "dirty" } })).toBe("held"); expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "DIRTY" } })).toBe("held"); // case-insensitive expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "behind" } })).toBe("held"); + // The PR #5288 bug: green CI + merge verdict but a non-required third-party check (e.g. a Superagent + // "Contributor trust" ACTION_REQUIRED) leaves GitHub's mergeable_state "unstable" -- the disposition planner + // (agentHoldAuditDetail, processors.ts) already withholds the merge for ANY non-"clean" state, so the comment + // must not say "safe to merge" here either, or it directly contradicts the bot's own held merge action. + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "unstable" } })).toBe("held"); + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "UNSTABLE" } })).toBe("held"); // case-insensitive // A clean (or not-yet-computed / pending-bot-approval) merge state still renders ready. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "clean" } })).toBe("ready"); expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed", mergeStateLabel: "unknown" } })).toBe("ready");