diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 00b32510e8..89d991d2a6 100644 --- a/src/github/backfill.ts +++ b/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. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9b99ea7c68..92f50bbec8 100644 --- a/src/queue/processors.ts +++ b/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 } } : {}), diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 839269ca91..3742ab895e 100644 --- a/src/settings/agent-actions.ts +++ b/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; diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index 01994c1f7e..10e227e71e 100644 --- a/src/settings/pr-disposition.ts +++ b/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,14 +96,17 @@ 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 @@ -104,7 +114,7 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { // 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 }; } diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index 589193d3f3..a8d63b5d4f 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/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); + }); +});