refactor(disposition): one shared mergeable-state interpretation consumed by planner, comment, and labels (#8759)

The #8711 incident's root CLASS: four surfaces each re-derived their own
meaning for GitHub's raw mergeable_state string (comment {dirty,behind,
unstable}; merge !== clean; approve {dirty}; hold-label none pre-#8758).
#8758 unified the predicates; this removes the class:

- src/settings/pr-disposition.ts (new, pure, dependency-free):
  assessMergeableState — THE single raw-string interpretation point — and
  derivePrDisposition (heldForManualReview / wouldApprove / wouldMerge /
  commentMergeStateHeld), each semantic documented as a contract.
- agent-actions.ts derives the disposition once and consumes its fields for
  heldForManualReview, the approve gate, and canMerge; isConflict routes
  through assessMergeableState. Byte-identical: all 308 existing planner
  tests pass unchanged.
- unified-comment.ts's readiness gains mergeStateHeld — resolved by
  processors.ts via isCommentMergeStateHeld so the self-contained renderer
  consumes a boolean instead of re-deriving meaning (zero-import contract
  intact); legacy callers keep the byte-identical raw-string fallback.
- test/unit/pr-disposition-invariants.test.ts: the cross-surface invariant
  suite — module invariants over the full state matrix, planner-vs-
  disposition agreement per state, renderer-vs-disposition agreement through
  the bridge boolean, and the legacy-fallback equivalence pin.

Author: JSONbored

src/queue/processors.ts

@@ -280,6 +280,7 @@ import {
   type AgentDispositionLabelSettings,
   type PlannedAgentAction,
 } from "../settings/agent-actions";
+import { isCommentMergeStateHeld } from "../settings/pr-disposition";
 import { isAutoCloseExempt } from "../settings/auto-close-exempt";
 import {
   isSkipAutomationBotPullRequestsEnabledGlobally,
@@ -2063,6 +2064,10 @@ export function derivePublicCommentMergeFacts(args: {
   const mergeReadiness: MergeReadiness = {
     ciState,
     ...(mergeStateLabel ? { mergeStateLabel } : {}),
+    // #8759: the SHARED interpretation of the merge state (pr-disposition.ts) — the same one the
+    // disposition planner reads — resolved here so the self-contained renderer consumes a boolean
+    // instead of re-deriving meaning from the raw string (the #8711 four-surfaces-disagree class).
+    ...(mergeStateLabel ? { mergeStateHeld: isCommentMergeStateHeld(mergeStateLabel) } : {}),
     ...(failingDetails.length > 0 ? { failingChecks: failingDetails.map((detail) => detail.name) } : {}),
     ...(failingDetails.length > 0 ? { failingDetails } : {}),
     ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}),

src/review/unified-comment.ts

@@ -89,6 +89,11 @@ export interface CheckFailureDetail {
  *  Canonical home (#288): was duplicated identically in the awesome-claude + metagraphed agents. */
 export interface MergeReadiness {
   mergeStateLabel?: string;
+  /** #8759: the SHARED interpretation of mergeStateLabel, resolved by the bridge via
+   *  pr-disposition.ts's isCommentMergeStateHeld so this self-contained file never re-derives meaning
+   *  from the raw string. When present it is authoritative; absent (older callers) the legacy raw-string
+   *  check below applies, byte-identical to the pre-#8759 behavior. */
+  mergeStateHeld?: boolean;
   ciState: "passed" | "failed" | "unverified";
   failingChecks?: string[];
   failingDetails?: CheckFailureDetail[];
@@ -378,7 +383,10 @@ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedComme
   // 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) {
+  if (status === "ready" && input.readiness?.mergeStateHeld !== undefined) {
+    // #8759: the bridge resolved the shared interpretation (pr-disposition.ts) — authoritative when present.
+    if (input.readiness.mergeStateHeld) return "held";
+  } else if (status === "ready" && input.readiness?.mergeStateLabel) {
     const mergeState = input.readiness.mergeStateLabel.toLowerCase();
     if (mergeState === "dirty" || mergeState === "behind" || mergeState === "unstable") return "held";
   }

src/settings/agent-actions.ts

@@ -1,6 +1,7 @@
 import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types";
 import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory";
 import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy";
+import { assessMergeableState, derivePrDisposition } from "./pr-disposition";
 import { changedPathsHittingGuardrail, isGuardrailHit } from "../signals/change-guardrail";
 import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules";
 import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings";
@@ -966,7 +967,8 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // The gate verdict is authoritative. Green CI is still required for merge/approve, but it does not rewrite an AI
   // or review-thread blocker into success once the gate has classified it as blocking.
   const conclusion: GateCheckConclusion = input.conclusion;
-  const isConflict = input.pr.mergeableState === "dirty"; // conflicts with base — can't merge as-is
+  // #8759: the raw mergeable_state string is interpreted ONLY by assessMergeableState — one shared meaning.
+  const isConflict = assessMergeableState(input.pr.mergeableState) === "conflict"; // conflicts with base — can't merge as-is
   // True when an unresolved GitHub review thread is (at least one of) this close's justifications -- the SAME
   // staleness class as isConflict above (#3863), just triggered by a contributor clicking "Resolve conversation"
   // on GitHub instead of the base branch becoming mergeable again. A mixed blocker set (thread + something else)
@@ -1020,33 +1022,34 @@ 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"));
+  // #8759: the hold/approve/merge core now comes from the SHARED disposition module — the same
+  // derivation the unified comment's bridge reads — so the four surfaces can never again disagree on
+  // what a raw mergeable_state means (#8711's root class). The unstable-hold semantics are #8758's,
+  // unchanged (see derivePrDisposition's own doc + the MergeableAssessment contract): "dirty" stays the
+  // close path, "behind" stays the rebase rail's, "blocked"/"unknown" stay approvable, "unstable" holds
+  // loudly. The disposition's wouldApprove/wouldMerge feed the approve/merge gates below, still
+  // conjoined with the planner-private terms (autonomy, idempotency, approvals, terminal-block) that
+  // are not disposition. reviewGood is computed here (moved up from beside canMerge — same formula,
+  // gate passes AND CI green) because the disposition needs it.
+  const reviewGood = gatePassing && ciPassed;
+  const disposition = derivePrDisposition({
+    mergeableState: input.pr.mergeableState,
+    reviewGood,
+    guardrailHit,
+    migrationCollisionHold: input.migrationCollisionHold !== undefined,
+    unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined,
+    advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0,
+    unlinkedIssueMatchCloseWithoutCloseActing: input.unlinkedIssueMatchClose !== undefined && !acting("close"),
+  });
+  const heldForManualReview = disposition.heldForManualReview;
+  const mergeableStateUnstable = disposition.heldForUnstableMergeState;
   const labels = resolveAgentDispositionLabels(input);
   // Canonical (reviewbot non-content-gate) policy, tuned to the operator's minimize-manual goal: merge-or-close
   // with high accuracy; manual review is the RARE exception. A PR is "review-good" when the gate passes AND CI is
   // green — that's the only thing that earns an auto-merge or an approve. Everything else, for a CONTRIBUTOR, is a
   // one-shot CLOSE (taopedia model: resolve + open a fresh PR). The guardrail is handled SEPARATELY: it converts
   // would-approve/would-merge dispositions into a manual hold.
   const ciUnverified = input.ciState === "unverified";
-  const reviewGood = gatePassing && ciPassed;
-  const mergeableClean = input.pr.mergeableState === "clean";
   // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan
   // the merge; it can't complete for this commit. A new commit makes the live head differ from mergeBlockedSha.
   const mergeTerminallyBlocked = input.pr.mergeBlockedSha != null && input.pr.headSha != null && input.pr.mergeBlockedSha === input.pr.headSha;
@@ -1055,7 +1058,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // reviewDecision to APPROVED, so reviewDecision alone can't dedup). A new commit makes the heads differ →
   // approve may fire again. Absent approved-head SHA (never approved by the bot) ⇒ not idempotent-skipped.
   const alreadyApprovedThisHead = input.pr.approvedHeadSha != null && input.pr.headSha != null && input.pr.approvedHeadSha === input.pr.headSha;
-  const canMerge = reviewGood && !heldForManualReview && acting("merge") && mergeableClean && approvalsSatisfied && !mergeTerminallyBlocked;
+  // #8759: disposition.wouldMerge = reviewGood && !held && exactly-clean — the shared core; the terms
+  // conjoined here (autonomy, approvals, terminal-block) are planner-private state, not disposition.
+  const canMerge = disposition.wouldMerge && acting("merge") && approvalsSatisfied && !mergeTerminallyBlocked;
   // CLOSE a contributor PR ONLY on a REAL adverse signal — a confirmed gate FAILURE, red CI, or a base
   // CONFLICT. NEVER close merely because CI is UNVERIFIED (a fork whose Actions await approval, or unreadable
   // checks) or otherwise not-yet-mergeable — those are HELD for review, not killed (#harm-stop fork-false-close).
@@ -1348,7 +1353,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // 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) {
+  // #8759: disposition.wouldApprove = reviewGood && !held && not-a-conflict — the shared core the executor's
+  // live recheck mirrors; the terms conjoined here are planner-private (close-in-flight, autonomy, idempotency).
+  if (disposition.wouldApprove && !linkedIssueCloseInFlight && acting("approve") && input.pr.reviewDecision !== "APPROVED" && !alreadyApprovedThisHead) {
     actions.push({
       actionClass: "approve",
       requiresApproval: approval("approve"),

src/settings/pr-disposition.ts

@@ -0,0 +1,118 @@
+// Shared PR-disposition core (#8759, epic #8757). The #8711 incident's root CLASS was four surfaces —
+// the disposition planner (agent-actions.ts), the executor's live rechecks (agent-action-executor.ts),
+// the unified comment's status derivation (unified-comment.ts, via the bridge), and the review-state
+// labels — each re-deriving its own meaning for GitHub's raw `mergeable_state` string, with four
+// different subsets treated as "bad" (comment {dirty,behind,unstable}; merge !== "clean"; approve
+// {dirty}; hold-label ∅ pre-#8758). #8758 unified the PREDICATES; this module removes the CLASS by
+// giving every surface ONE place the raw string is interpreted and ONE shared held/approve/merge
+// assessment derived from it.
+//
+// PURE AND DEPENDENCY-FREE by design: agent-actions.ts (the planner), processors.ts (the comment
+// bridge's caller), and the executor all import from here; this file imports nothing of theirs, so it
+// can never participate in a cycle. The self-contained unified-comment.ts still receives plain data
+// (the bridge passes the RESOLVED assessment, never an import), preserving its zero-import contract.
+//
+// INVARIANT CONTRACT (pinned by test/unit/pr-disposition-invariants.test.ts): for any input state,
+//   • approve is never allowed while the state is one merge would refuse for a reason no other rail
+//     resolves (assessment "conflict" → the close path owns it; "unstable" → the manual hold owns it);
+//   • "behind" never holds (the rebase rail owns it) and stays approvable;
+//   • "blocked"/"unknown"/absent stay approvable (the bot's own approval can be the unblocking act,
+//     and a transient null must not spray hold labels);
+//   • merge requires exactly "clean" — the strictest predicate, unchanged since before #8758.
+
+/** Every meaning the raw GitHub `mergeable_state` string carries for the disposition surfaces. This is
+ *  THE single interpretation point — no other module may compare the raw string against a literal. */
+export type MergeableAssessment =
+  /** Safe to merge right now (the only state `canMerge` accepts). */
+  | "clean"
+  /** Hard base conflict — the CLOSE path's business (`isConflict`), never approve, never hold-label. */
+  | "conflict"
+  /** Behind the base — the rebase rail's business; approvable, never a manual hold. */
+  | "behind"
+  /** Required checks green but a non-required check/status is not (#8711/#8758): merge self-suppresses,
+   *  so the PR must be HELD loudly (manual-review label + comment) and never approved into a stall. */
+  | "unstable"
+  /** blocked / unknown / null / anything else: not mergeable YET, but approvable — the missing piece may
+   *  be the bot's own approval (blocked) or a transient computation (unknown). Never a hold. */
+  | "indeterminate";
+
+export function assessMergeableState(state: string | null | undefined): MergeableAssessment {
+  switch ((state ?? "").toLowerCase()) {
+    case "clean":
+      return "clean";
+    case "dirty":
+      return "conflict";
+    case "behind":
+      return "behind";
+    case "unstable":
+      return "unstable";
+    default:
+      return "indeterminate";
+  }
+}
+
+/** The hold inputs every surface must agree on. Each field mirrors the planner input of the same name —
+ *  the caller (planner or processors.ts) resolves them once and both surfaces read the same values. */
+export type PrDispositionInput = {
+  mergeableState: string | null | undefined;
+  /** Gate conclusion success/neutral AND required CI passed — the only thing that earns approve/merge. */
+  reviewGood: boolean;
+  guardrailHit: boolean;
+  migrationCollisionHold: boolean;
+  unlinkedIssueMatchHold: boolean;
+  advisoryCheckHold: boolean;
+  /** A confirmed repeat unlinked-issue-match while `close` autonomy is NOT acting (the planner's own
+   *  fold-into-hold escape hatch — see agent-actions.ts's heldForManualReview doc). */
+  unlinkedIssueMatchCloseWithoutCloseActing: boolean;
+};
+
+export type PrDisposition = {
+  mergeable: MergeableAssessment;
+  /** The SAME formula agent-actions.ts's heldForManualReview computes — one definition, two readers. */
+  heldForManualReview: boolean;
+  /** True when the ONLY thing suppressing a would-merge is the unstable mergeable state (#8758's loud
+   *  hold): the planner uses it to attach the check-naming comment; the comment surface uses it to
+   *  downgrade "safe to merge". */
+  heldForUnstableMergeState: boolean;
+  /** reviewGood && not held && not the close path's conflict — the approve gate's shared core. The
+   *  planner still conjoins its own idempotency/autonomy terms (reviewDecision, approvedHeadSha,
+   *  acting("approve")) — those are planner-private state, not disposition. */
+  wouldApprove: boolean;
+  /** reviewGood && not held && exactly-clean — the merge gate's shared core. The planner still conjoins
+   *  approvalsSatisfied / mergeTerminallyBlocked / acting("merge") — planner-private state. */
+  wouldMerge: boolean;
+  /** The comment surface's readiness downgrade: an otherwise-"ready" status must render held for any
+   *  state in this set (conflict/behind/unstable — never claim "safe to merge" while GitHub disagrees),
+   *  mirroring deriveUnifiedStatus's historical {dirty, behind, unstable} set exactly. */
+  commentMergeStateHeld: boolean;
+};
+
+export function derivePrDisposition(input: PrDispositionInput): PrDisposition {
+  const mergeable = assessMergeableState(input.mergeableState);
+  const heldForManualReview =
+    input.guardrailHit ||
+    input.migrationCollisionHold ||
+    input.unlinkedIssueMatchHold ||
+    input.advisoryCheckHold ||
+    mergeable === "unstable" ||
+    input.unlinkedIssueMatchCloseWithoutCloseActing;
+  const heldForUnstableMergeState = mergeable === "unstable";
+  const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict";
+  const wouldMerge = input.reviewGood && !heldForManualReview && mergeable === "clean";
+  // The comment's historical downgrade set, byte-identical to deriveUnifiedStatus's own
+  // {dirty, behind, unstable} check (#ready-needs-mergeable / #pr-5288-confusing-verdict): "behind"
+  // downgrades the COMMENT's "safe to merge" claim (the rebase hasn't happened yet) even though it never
+  // holds the PLANNER (the rebase rail acts) — a deliberate, documented asymmetry, not drift: the two
+  // surfaces answer different questions ("is it safe to claim mergeable NOW" vs "should a human step in").
+  const commentMergeStateHeld = mergeable === "conflict" || mergeable === "behind" || mergeable === "unstable";
+  return { mergeable, heldForManualReview, heldForUnstableMergeState, wouldApprove, wouldMerge, commentMergeStateHeld };
+}
+
+/** The comment surface's merge-state downgrade as a standalone predicate (#8759): the bridge
+ *  (unified-comment-bridge.ts) resolves it and passes the BOOLEAN into the self-contained renderer, so
+ *  unified-comment.ts keeps its zero-import contract while reading the same interpretation the planner
+ *  uses. Equal by construction to derivePrDisposition(...).commentMergeStateHeld. */
+export function isCommentMergeStateHeld(state: string | null | undefined): boolean {
+  const mergeable = assessMergeableState(state);
+  return mergeable === "conflict" || mergeable === "behind" || mergeable === "unstable";
+}