fix(review): treat an unstable GitHub mergeable_state as a held verdict in the PR panel

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

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");