fix(disposition): hold loudly on an unstable mergeable state instead of approving into a silent stall

src/queue/processors.ts

@@ -2525,6 +2525,11 @@ function buildAgentMaintenancePlanInput(args: {
     // aggregate's field is always an array, [] when none); the planner applies its own length>0 gate, matching
     // how failingCheckNames above is likewise threaded unconditionally.
     advisoryCheckHold: ciAggregate.advisoryHoldDetails,
+    // #8758: non-required red checks (neither branch-protection-required nor declared advisory). They never gate
+    // ciState, but GitHub folds them into mergeable_state "unstable" — the state whose merge-suppression used to
+    // be silent (#8711). Threaded so the planner's unstable hold can NAME the culprit check(s) in its
+    // reason/comment; the hold itself keys on pr.mergeableState, so an empty list still holds with generic wording.
+    nonRequiredCheckFailures: ciAggregate.nonRequiredFailingDetails,
     ...(blacklistEntry !== null
       ? { blacklistMatch: { matched: true, reason: blacklistEntry.reason } }
       : {}),

src/services/agent-action-executor.ts

@@ -436,14 +436,23 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
     // and is left as a no-op here, matching closeRequiresMergeableState's own "false ⇒ skip" scoping above.
     const requiresLiveDuplicateRecheck =
       action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresDuplicateStillOpen === true && action.duplicateWinnerPrNumber !== undefined;
-    if (requiresLiveCiRecheck || requiresLiveMergeableRecheck || requiresLiveThreadRecheck || requiresLiveDuplicateRecheck) {
+    // #8758: an APPROVE is likewise planned from the planning-pass snapshot, and the planner never approves a
+    // "dirty" (about-to-close conflict) or "unstable" (merge self-suppresses, unstable hold) mergeable state —
+    // but nothing re-verified that right before posting the formal review. A check flipping red in the window
+    // between planning and actuation used to yield exactly #8711's incoherence: an approval claiming "gate
+    // satisfied and CI green" on a PR the same plan's merge arm would refuse. Same live signal the #3863
+    // conflict recheck already uses; one extra GET per approve (approves fire at most once per head).
+    const requiresLiveApproveMergeableRecheck = action.actionClass === "approve";
+    if (requiresLiveCiRecheck || requiresLiveMergeableRecheck || requiresLiveThreadRecheck || requiresLiveDuplicateRecheck || requiresLiveApproveMergeableRecheck) {
       const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined);
       const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId);
       const [liveCi, liveMergeableState, liveThreadBlockers, liveWinnerState] = await Promise.all([
         requiresLiveCiRecheck
           ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, ctx.requiredCiContexts ?? null, admissionKey, ctx.advisoryCheckRuns ?? null)
           : Promise.resolve(undefined),
-        requiresLiveMergeableRecheck ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined),
+        requiresLiveMergeableRecheck || requiresLiveApproveMergeableRecheck
+          ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey)
+          : Promise.resolve(undefined),
         requiresLiveThreadRecheck ? fetchLiveReviewThreadBlockers(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined),
         requiresLiveDuplicateRecheck
           ? fetchLivePullRequestState(env, ctx.repoFullName, action.duplicateWinnerPrNumber!, ciToken, admissionKey).catch(() => undefined)
@@ -485,7 +494,15 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
         requiresLiveDuplicateRecheck && liveWinnerState !== undefined && liveWinnerState !== "open"
           ? `duplicate-cluster winner #${action.duplicateWinnerPrNumber} is no longer open`
           : null;
-      const staleReason = ciStaleReason ?? mergeableStaleReason ?? threadStaleReason ?? duplicateStaleReason;
+      // #8758: only a CONFIRMED bad state suppresses the approve — a failed/ambiguous live read (undefined,
+      // "unknown", null) fails OPEN, matching the planner's own posture (it approves those states too: an
+      // approval is reversible and a transient fetch hiccup must not strand approval-required repos). Only the
+      // two states the planner itself refuses to approve ("dirty", "unstable") deny here.
+      const approveMergeableStaleReason =
+        requiresLiveApproveMergeableRecheck && (liveMergeableState === "dirty" || liveMergeableState === "unstable")
+          ? `live mergeable_state is now "${liveMergeableState}" — the planner never approves this state`
+          : null;
+      const staleReason = ciStaleReason ?? mergeableStaleReason ?? threadStaleReason ?? duplicateStaleReason ?? approveMergeableStaleReason;
       if (staleReason) {
         await audit("denied", `${staleReason} — action not executed`);
         continue;

src/settings/agent-actions.ts

@@ -407,6 +407,15 @@ export type AgentActionPlanInput = {
   // held-for-review state so a maintainer can act on the signal their installed app raised. Each entry names the
   // triggering check/app/conclusion so the hold reason (and the manual-review label's comment) is actionable.
   advisoryCheckHold?: ReadonlyArray<{ name: string; appSlug: string; conclusion: string }> | undefined;
+  // Non-required failing checks/statuses (#8758, the #8711 silent-stall fix). The CI aggregate's
+  // nonRequiredFailingDetails: red checks that are neither branch-protection-required nor declared advisory —
+  // they never feed ciState or a close, but GitHub folds them into mergeable_state "unstable", which suppresses
+  // the merge below (mergeableClean requires "clean"). Pre-#8758 nothing else accounted for that state: approve
+  // still fired ("gate satisfied, CI green"), the ready-to-merge label landed, and the only record of WHY the
+  // merge never came was an internal audit row. Threaded here purely to NAME the culprit check(s) in the
+  // unstable hold's reason/comment; the hold itself keys on pr.mergeableState, not on this list (an unstable
+  // state whose source the aggregate couldn't itemize still holds, with generic wording).
+  nonRequiredCheckFailures?: ReadonlyArray<{ name: string; summary?: string | undefined; detailsUrl?: string | undefined }> | undefined;
   // Unlinked-issue guardrail (#unlinked-issue-guardrail, credibility-gate-farming defense). The trigger
   // (runAgentMaintenancePlanAndExecute) has already run the deterministic pre-filter + AI verification for a
   // PR that links NO issue -- this input is already the resolved "yes, hold this merge" verdict (or absent,
@@ -728,6 +737,24 @@ function advisoryHoldComment(holds: ReadonlyArray<{ name: string; appSlug: strin
   return `Held for manual review: a maintainer-configured advisory check-run reported a non-passing result — ${parts.join("; ")}. This does not block CI, but a maintainer should review it. This is an automated maintenance action.`;
 }
 
+// #8758: name the non-required check(s) behind a GitHub "unstable" mergeable state, for the hold reason (audit)
+// and the public comment. Check names are already public on the PR's own Checks tab, so interpolating them is
+// safe; summaries/URLs are deliberately NOT interpolated (same public-safe-comment posture as advisoryHoldComment).
+// Falls back to generic wording when the CI aggregate couldn't itemize the source (the hold keys on
+// mergeable_state itself, never on this list being non-empty).
+function mergeUnstableHoldReason(failures: ReadonlyArray<{ name: string }> | undefined): string {
+  const names = (failures ?? []).map((f) => `"${f.name}"`);
+  return names.length > 0
+    ? `mergeable_state is unstable — non-required check(s) not passing: ${names.join("; ")}`
+    : "mergeable_state is unstable — a non-required check or status is not passing";
+}
+
+function mergeUnstableHoldComment(failures: ReadonlyArray<{ name: string }> | undefined): string {
+  const names = (failures ?? []).map((f) => `\`${f.name}\``);
+  const culprit = names.length > 0 ? ` — ${names.join(", ")} —` : "";
+  return `Held for manual review: the gate and required CI are green, but GitHub reports this pull request's mergeable state as \`unstable\` because a non-required check or status${culprit} is not passing, so LoopOver will not auto-merge. A maintainer can resolve the failing check or review and merge manually. This is an automated maintenance action.`;
+}
+
 /**
  * Plan best-effort assignment of the PR's opening contributor (#3182), independent of merge/close/CI outcome.
  * MUST run before the CI-pending settle-before-decide return below (#assign-before-ci-pending) — a PR that has
@@ -993,11 +1020,23 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // would silently MERGE straight through the escalation instead of being held. When `close` IS acting, the
   // dedicated close branch below handles it and this term is redundant (harmless: both paths agree the PR
   // must not silently merge).
+  // Unstable mergeable state (#8758, the #8711 silent-stall fix): GitHub reports "unstable" when every REQUIRED
+  // check is green but some non-required check/status is not — exactly the state where canMerge below
+  // self-suppresses (mergeableClean requires "clean") while, pre-#8758, nothing else held, labeled, or explained.
+  // Folding it into heldForManualReview downgrades the would-approve/would-merge into the SAME loud
+  // held-for-review disposition every other merge-suppressing hold gets: no approve claiming "gate satisfied",
+  // no ready-to-merge label, and a manual-review label + comment naming the culprit check. Deliberately ONLY
+  // "unstable": "dirty" is the close path (isConflict), "behind" belongs to the rebase rail, and
+  // "blocked"/"unknown"/absent stay approvable (the approval itself can be the unblocking act — see the approve
+  // block's own doc comment — and a transient null must not spray hold labels). Every consumer that acts on this
+  // flag is conjoined with reviewGood, so a red-CI/failed-gate PR's close is never softened by this term.
+  const mergeableStateUnstable = input.pr.mergeableState === "unstable";
   const heldForManualReview =
     guardrailHit ||
     input.migrationCollisionHold !== undefined ||
     input.unlinkedIssueMatchHold !== undefined ||
     (input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0) ||
+    mergeableStateUnstable ||
     (input.unlinkedIssueMatchClose !== undefined && !acting("close"));
   const labels = resolveAgentDispositionLabels(input);
   // Canonical (reviewbot non-content-gate) policy, tuned to the operator's minimize-manual goal: merge-or-close
@@ -1156,6 +1195,22 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
     });
   }
 
+  // 1f) unstable-mergeable-state manual-review fallback (#8758): mirrors 1d. When the dedicated
+  // review_state_label class isn't acting but merge is, a would-merge suppressed ONLY by GitHub's "unstable"
+  // mergeable state must still surface a visible hold — label + comment naming the non-required check —
+  // instead of the silent stall #8711 hit (approve posted, merge never planned, nothing on the PR saying why).
+  if (reviewGood && mergeableStateUnstable && !acting("review_state_label") && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) {
+    actions.push({
+      actionClass: "label",
+      autonomyClass: "merge",
+      requiresApproval: approval("merge"),
+      reason: `verdict=${conclusion}; ${mergeUnstableHoldReason(input.nonRequiredCheckFailures)}`,
+      label: labels.manualReview,
+      labelOp: "add",
+      comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)),
+    });
+  }
+
   // 2) review_state_label (#label-scoping) — ready-to-merge (review-good, unguarded) / manual-review
   // (review-good but guarded) / changes-requested (not review-good → will be closed for a contributor, held for
   // the owner). A pending linked-issue hard-rule close (flag OR close pass) forces the changes-requested label
@@ -1189,9 +1244,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
                 ? `verdict=${conclusion}; ${input.unlinkedIssueMatchClose.reason}`
                 : input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0
                   ? `verdict=${conclusion}; ${advisoryHoldReason(input.advisoryCheckHold)}`
-                  : heldForManualReview
-                    ? `verdict=${conclusion}; ${guardrailReason}`
-                    : `verdict=${conclusion}; CI green`;
+                  : mergeableStateUnstable
+                    ? `verdict=${conclusion}; ${mergeUnstableHoldReason(input.nonRequiredCheckFailures)}`
+                    : heldForManualReview
+                      ? `verdict=${conclusion}; ${guardrailReason}`
+                      : `verdict=${conclusion}; CI green`;
     if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) {
       actions.push({
         actionClass: "label",
@@ -1212,7 +1269,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
               ? { comment: sanitizePublicComment(input.unlinkedIssueMatchClose.comment) }
               : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0
                 ? { comment: sanitizePublicComment(advisoryHoldComment(input.advisoryCheckHold)) }
-                : {}),
+                : !linkedIssueCloseInFlight && !unlinkedIssueMatchViolated && reviewGood && mergeableStateUnstable
+                  ? { comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)) }
+                  : {}),
       });
     }
     // Stale disposition-label cleanup (#stale-disposition-label-cleanup): the review-state labels below
@@ -1285,7 +1344,10 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // Never APPROVE a base-conflicting PR: it is closed below (willClose on isConflict), so a "LoopOver approves —
   // safe to merge" review on a PR we're about to close is incoherent (and a stale approval strands the PR if it
   // later goes green). A `behind`/`blocked` PR is fine to approve (it is rebased pre-review or the approval clears
-  // the block); only a hard `dirty` conflict is excluded here. (#ready-needs-mergeable, the #4220 report) */
+  // the block); only a hard `dirty` conflict is excluded here. (#ready-needs-mergeable, the #4220 report)
+  // An `unstable` PR is excluded too, via heldForManualReview's mergeableStateUnstable term (#8758): the merge
+  // below would self-suppress on it, and approve firing while merge silently never comes was exactly #8711's
+  // "approved, labeled ready, never merged, nobody told" incident. */
   if (reviewGood && !heldForManualReview && !linkedIssueCloseInFlight && !isConflict && acting("approve") && input.pr.reviewDecision !== "APPROVED" && !alreadyApprovedThisHead) {
     actions.push({
       actionClass: "approve",

test/unit/agent-action-executor.test.ts

@@ -35,16 +35,18 @@ vi.mock("../../src/github/app", async (importOriginal) => ({
 }));
 // The actuation-time live CI re-check (#2128) defaults to "still passing" so the existing merge tests stay
 // deterministic; individual tests below override this to exercise the staleness-denial path.
-// The actuation-time live mergeable-state re-check (#3863) defaults to "dirty" (conflict still present) so no
-// existing test needs to override it; the tests below explicitly set it to exercise the staleness-denial path.
+// The actuation-time live mergeable-state re-check defaults to "clean": since #8758, EVERY approve action
+// consults this fetch (denied on live "dirty"/"unstable"), so a non-clean default would wrongly deny the many
+// approve-executing tests that never think about mergeable state. The #3863 conflict-close tests all set their
+// state explicitly (mockResolvedValueOnce) and never relied on the old "dirty" default.
 // The actuation-time live review-thread re-check (#review-thread-staleness) defaults to a single still-unresolved
 // blocker for the same reason -- individual tests below override it to exercise the staleness-denial path.
 // The actuation-time live duplicate-winner-still-open re-check (#dup-winner-staleness) defaults to "open" (the
 // named winning sibling is still open, i.e. the duplicate justification still holds) for the same reason.
 vi.mock("../../src/github/backfill", async (importOriginal) => ({
   ...(await importOriginal<typeof import("../../src/github/backfill")>()),
   fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], advisoryHoldDetails: [], ciCompletenessWarning: null })),
-  fetchLivePullRequestMergeState: vi.fn(async () => "dirty" as const),
+  fetchLivePullRequestMergeState: vi.fn(async () => "clean" as const),
   fetchLiveReviewThreadBlockers: vi.fn(async () => [{ title: "still unresolved", scannerFinding: false }]),
   fetchLivePullRequestState: vi.fn(async () => "open" as const),
   refreshInstallationHealthForInstallation: vi.fn(async () => null),
@@ -592,6 +594,34 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     expect(fetchLivePullRequestMergeState).not.toHaveBeenCalled();
   });
 
+  it("REGRESSION (#8758): an approve is DENIED when the live mergeable_state is unstable — no approval claiming green on a PR the merge arm refuses", async () => {
+    const env = createTestEnv({});
+    const approve: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "gate passed, CI green", reviewBody: "LoopOver approves." };
+    vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("unstable");
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [approve]);
+    expect(outcomes[0]?.outcome).toBe("denied");
+    expect(outcomes[0]?.detail).toContain("unstable");
+    expect(createPullRequestReview).not.toHaveBeenCalled();
+  });
+
+  it("REGRESSION (#8758): an approve is DENIED when the live mergeable_state is dirty (about to be conflict-closed)", async () => {
+    const env = createTestEnv({});
+    const approve: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "gate passed, CI green", reviewBody: "LoopOver approves." };
+    vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty");
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [approve]);
+    expect(outcomes[0]?.outcome).toBe("denied");
+    expect(createPullRequestReview).not.toHaveBeenCalled();
+  });
+
+  it("an approve FAILS OPEN (still executes) when the live mergeable-state read is ambiguous — a transient fetch hiccup must not strand approval-required repos (#8758)", async () => {
+    const env = createTestEnv({});
+    const approve: PlannedAgentAction = { actionClass: "approve", requiresApproval: false, reason: "gate passed, CI green", reviewBody: "LoopOver approves." };
+    vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("unknown");
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [approve]);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(createPullRequestReview).toHaveBeenCalled();
+  });
+
   it("REGRESSION (#3863): closeRequiresMergeableState round-trips through the persist/replay round trip so a staged conflict close still re-checks live mergeable-state", async () => {
     const env = createTestEnv({});
     const conflictClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "conflicts with the base branch", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true };