fix(gate): an ignored check must not hold the PR via mergeable_state either

src/github/backfill.ts

@@ -2626,7 +2626,7 @@ async function fetchPullRequestChecks(
 // auto-approval). (#fork-action-required) — a THIRD-PARTY app's own COMPLETED action_required verdict (e.g. a
 // security/check tool) is handled separately below and fails closed as a manual-hold signal, not green CI.
 const CI_FAILING_CONCLUSIONS = new Set(["failure", "timed_out", "cancelled", "startup_failure"]);
-const CI_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]);
+export const CI_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]);
 // The bot's OWN check-runs — it posts these (in_progress, then concluded) as PART OF reviewing. They are NOT
 // "CI to wait on": counting them self-deadlocks (the review waits for all CI to finish; these only finish when
 // the very review they're blocking runs → the PR defers forever). Excluded from the CI aggregate entirely.

src/queue/processors.ts

@@ -122,6 +122,7 @@ import {
   isReviewsCacheUpToDate,
   primeDurablePrStateCache,
   refreshPullRequestDetails,
+  CI_PASSING_CONCLUSIONS,
 } from "../github/backfill";
 import {
   contributorRepoStatsFromGittensor,
@@ -2863,6 +2864,9 @@ function buildAgentMaintenancePlanInput(args: {
     // 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,
+    // #9810 follow-up: only the NON-PASSING ignored runs. A maintainer-ignored check that concluded fine is
+    // not an explanation for instability, so it must not license dismissing one.
+    ignoredCheckNonPassing: ciAggregate.ignoredCheckDetails.filter((run) => !CI_PASSING_CONCLUSIONS.has(run.conclusion)),
     ...(blacklistEntry !== null
       ? { blacklistMatch: { matched: true, reason: blacklistEntry.reason } }
       : {}),

src/settings/agent-actions.ts

@@ -422,6 +422,11 @@ 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;
+  // #9810 follow-up: non-passing check-runs the maintainer listed in `gate.ignoredCheckRuns`, as seen on this
+  // head. Used ONLY to decide whether a GitHub "unstable" mergeable_state is fully explained by them -- the
+  // CI aggregate already excludes these from pass/fail, but mergeable_state is GitHub's own computation and
+  // stays unstable while the check exists at all.
+  ignoredCheckNonPassing?: 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
@@ -799,13 +804,23 @@ function mergeUnstableHoldReason(failures: ReadonlyArray<{ name: string }> | und
   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";
+    // #9810 follow-up: the un-itemized case used to say only "a non-required check or status is not passing",
+    // which tells a maintainer nothing they can act on -- not which check, not where to look. GitHub computes
+    // mergeable_state itself and never says why, and our aggregate can legitimately fail to itemize it (a
+    // COMMIT STATUS rather than a check-run, a check from an app whose page we couldn't read, or a run that
+    // appeared after the aggregate was taken). Name that ambiguity and point at the one place the answer
+    // always exists, instead of restating the state.
+    : "mergeable_state is unstable — GitHub reports a non-required check or status as not passing, but this pass could not itemize which one (it may be a commit status rather than a check-run, or it appeared after CI was read). See the PR's own Checks tab for the current list";
 }
 
 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.`;
+  const whereToLook =
+    names.length > 0
+      ? ""
+      : " This pass could not identify which check (it may be a commit status rather than a check-run, or it appeared after CI was read) — the PR's own Checks tab has the current list.";
+  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.${whereToLook} A maintainer can resolve the failing check or review and merge manually. This is an automated maintenance action.`;
 }
 
 /**
@@ -1102,6 +1117,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
     migrationCollisionHold: input.migrationCollisionHold !== undefined,
     unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined,
     advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0,
+    // Deliberately conjoined with "nothing else adverse": an unstable state is only attributed to the ignore
+    // list when our own aggregate found NO failing check of any kind. If some other non-required check is
+    // also red, failingDetails is non-empty and the hold stands -- an ignore must never mask a real failure.
+    unstableExplainedByIgnoredChecks:
+      input.ignoredCheckNonPassing !== undefined && input.ignoredCheckNonPassing.length > 0 && (input.nonRequiredCheckFailures ?? []).length === 0 && input.ciState !== "failed",
     unlinkedIssueMatchCloseWithoutCloseActing: input.unlinkedIssueMatchClose !== undefined && !acting("close"),
   });
   const heldForManualReview = disposition.heldForManualReview;

src/settings/pr-disposition.ts

@@ -61,6 +61,13 @@ export type PrDispositionInput = {
   migrationCollisionHold: boolean;
   unlinkedIssueMatchHold: boolean;
   advisoryCheckHold: boolean;
+  /** #9810 follow-up: GitHub says `unstable`, but the ONLY non-passing check explaining it is one the
+   *  maintainer listed in `gate.ignoredCheckRuns`. LoopOver's own CI aggregate already excludes such a run --
+   *  yet `mergeable_state` is GitHub's computation, not ours, and it stays "unstable" while the check exists
+   *  at all. Without this the ignore was half-effective: the check no longer failed the gate, and the PR was
+   *  held anyway (observed on JSONbored/loopover#9816, reason "mergeable_state is unstable — non-required
+   *  check(s) not passing: Contributor trust"). Set ONLY when nothing else adverse was seen. */
+  unstableExplainedByIgnoredChecks?: boolean | undefined;
   /** 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;
@@ -89,22 +96,25 @@ export type PrDisposition = {
 
 export function derivePrDisposition(input: PrDispositionInput): PrDisposition {
   const mergeable = assessMergeableState(input.mergeableState);
+  // An "unstable" state that ONLY an ignored check explains carries no signal a maintainer asked to act on:
+  // they explicitly declared that check meaningless for this repo. Every other unstable cause still holds.
+  const unstableHolds = mergeable === "unstable" && input.unstableExplainedByIgnoredChecks !== true;
   const heldForManualReview =
     input.guardrailHit ||
     input.migrationCollisionHold ||
     input.unlinkedIssueMatchHold ||
     input.advisoryCheckHold ||
-    mergeable === "unstable" ||
+    unstableHolds ||
     input.unlinkedIssueMatchCloseWithoutCloseActing;
-  const heldForUnstableMergeState = mergeable === "unstable";
+  const heldForUnstableMergeState = unstableHolds;
   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";
+  const commentMergeStateHeld = mergeable === "conflict" || mergeable === "behind" || unstableHolds;
   return { mergeable, heldForManualReview, heldForUnstableMergeState, wouldApprove, wouldMerge, commentMergeStateHeld };
 }
 

test/unit/pr-disposition-invariants.test.ts

@@ -177,3 +177,45 @@ describe("cross-surface: deriveUnifiedStatus consumes the bridge-resolved boolea
     }
   });
 });
+
+describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => {
+  const base = {
+    reviewGood: true, guardrailHit: false, migrationCollisionHold: false, unlinkedIssueMatchHold: false,
+    advisoryCheckHold: false, unlinkedIssueMatchCloseWithoutCloseActing: false,
+  };
+
+  it("REGRESSION: an unstable state the ignore list fully explains no longer holds", () => {
+    // The live half-fix: gate.ignoredCheckRuns removed the check from LoopOver's CI aggregate, but
+    // mergeable_state is GitHub's own computation and stayed "unstable" while the check existed at all —
+    // so JSONbored/loopover#9816 was still held, reason "mergeable_state is unstable — non-required
+    // check(s) not passing: Contributor trust". The ignore was half-effective until this.
+    const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: true });
+    expect(d.heldForManualReview).toBe(false);
+    expect(d.heldForUnstableMergeState).toBe(false);
+    expect(d.commentMergeStateHeld).toBe(false);
+  });
+
+  it("INVARIANT: unstable from ANY other cause still holds — the flag is not a blanket override", () => {
+    const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: false });
+    expect(d.heldForManualReview).toBe(true);
+    expect(d.heldForUnstableMergeState).toBe(true);
+  });
+
+  it("INVARIANT: absent flag behaves exactly as before (byte-identical for every existing caller)", () => {
+    expect(derivePrDisposition({ ...base, mergeableState: "unstable" }).heldForManualReview).toBe(true);
+  });
+
+  it("INVARIANT: the flag never rescues a PR held for a DIFFERENT reason", () => {
+    // An ignored check explaining the instability must not also wave through a guardrail hit.
+    const d = derivePrDisposition({ ...base, guardrailHit: true, mergeableState: "unstable", unstableExplainedByIgnoredChecks: true });
+    expect(d.heldForManualReview).toBe(true);
+    expect(d.wouldMerge).toBe(false);
+  });
+
+  it("a dismissed-unstable PR can actually merge when everything else is clean", () => {
+    // The point of the fix: not merely "not held", but genuinely mergeable again. GitHub still says
+    // unstable, so mergeableState stays the gate on wouldMerge — the PR approves rather than merging.
+    const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: true });
+    expect(d.wouldApprove).toBe(true);
+  });
+});