From a2076b35ca305e1fc793d3f0934baae83c92d255 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:22:47 +0000 Subject: [PATCH] fix(settings): thread unstableExplainedByIgnoredChecks into isCommentMergeStateHeld isCommentMergeStateHeld re-derived unstable-holds from the raw mergeable_state string alone, so it stayed true even when the planner's derivePrDisposition had already stopped holding an unstable state fully explained by an ignored check. The comment surface's only production caller now resolves the same formula buildPrDispositionInput uses from its own live-CI inputs and passes it through, restoring the "equal by construction" invariant the module's JSDoc claims. --- src/queue/processors.ts | 24 +++++-- src/settings/pr-disposition.ts | 16 ++++- test/unit/pr-disposition-invariants.test.ts | 36 ++++++++-- ...cessors-public-comment-merge-facts.test.ts | 67 +++++++++++++++++++ 4 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b4bdec09eb..9b03e5afe3 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2163,7 +2163,12 @@ export function derivePublicCommentMergeFacts(args: { // #8683: the live per-repo admin verdict for the author, resolved by the caller (isPerTenantAdmin) so this // pure function can match the planner's owner-OR-admin close-eligibility formula without an async fetch. authorIsAdmin: boolean; - liveCi: Pick; + liveCi: Pick & { + // #10055: optional so existing fixtures/callers that predate the ignored-checks resolution keep + // typechecking unchanged; the one production caller always passes the full LiveCiAggregate, which + // always has this field. Absent/empty behaves exactly like "nothing was ignored". + ignoredCheckDetails?: LiveCiAggregate["ignoredCheckDetails"] | undefined; + }; settings: Pick; unifiedFiles: Awaited>; repoFullName: string; @@ -2180,13 +2185,22 @@ export function derivePublicCommentMergeFacts(args: { // Non-required-but-red checks (#4414-class advisory holds): surfaced so a flagged check is never silently // invisible, but never folded into failingChecks/failingDetails -- those two drive ciState/close. const nonRequiredFailingDetails = publicCheckFailureDetails(args.liveCi.nonRequiredFailingDetails); + // #10055: the SAME resolution agent-actions.ts's buildPrDispositionInput computes at its + // `unstableExplainedByIgnoredChecks` field -- only the NON-PASSING ignored runs count as an explanation, + // and only when nothing else non-required is also red and CI itself did not fail. Recomputed here (not + // threaded as a pre-resolved boolean) because this is the only production caller of the state-only + // helper and must read this surface's own live-CI inputs rather than a second, possibly-stale source. + const ignoredCheckNonPassing = (args.liveCi.ignoredCheckDetails ?? []).filter((run) => !CI_PASSING_CONCLUSIONS.has(run.conclusion)); + const unstableExplainedByIgnoredChecks = + ignoredCheckNonPassing.length > 0 && nonRequiredFailingDetails.length === 0 && args.liveCi.ciState !== "failed"; 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) } : {}), + // #8759/#10055: the SHARED interpretation of the merge state (pr-disposition.ts) — the same one the + // disposition planner reads, now including the ignored-checks resolution — 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, unstableExplainedByIgnoredChecks) } : {}), ...(failingDetails.length > 0 ? { failingChecks: failingDetails.map((detail) => detail.name) } : {}), ...(failingDetails.length > 0 ? { failingDetails } : {}), ...(nonRequiredFailingDetails.length > 0 ? { nonRequiredFailingDetails } : {}), diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index c4a0c35423..2261948840 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -165,8 +165,18 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { /** 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 { + * uses. Equal by construction to derivePrDisposition(...).commentMergeStateHeld -- including the + * `unstableExplainedByIgnoredChecks` term (#10055): a state-only interpretation of "unstable" went stale + * the moment that flag was added to PrDispositionInput, since a caller that never threads it through still + * held a PR the planner had already stopped holding. `unstableExplainedByIgnoredChecks` defaults to + * `undefined` so every existing caller that cannot resolve it keeps today's behaviour byte-identical. */ +export function isCommentMergeStateHeld( + state: string | null | undefined, + unstableExplainedByIgnoredChecks?: boolean | undefined, +): boolean { const mergeable = assessMergeableState(state); - return mergeable === "conflict" || mergeable === "behind" || mergeable === "unstable"; + // Identical to derivePrDisposition's own `unstableHolds` term (line 132) by design -- the two must never + // be able to drift back apart. + const unstableHolds = mergeable === "unstable" && unstableExplainedByIgnoredChecks !== true; + return mergeable === "conflict" || mergeable === "behind" || unstableHolds; } diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index f992eea97c..d94ad50e44 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -79,13 +79,28 @@ describe("derivePrDisposition — module-level invariants over the full state ma expect(conflict.wouldMerge).toBe(false); }); - it("isCommentMergeStateHeld equals derivePrDisposition(...).commentMergeStateHeld for every raw state (equal by construction, pinned)", () => { + it("isCommentMergeStateHeld equals derivePrDisposition(...).commentMergeStateHeld for every raw state x every unstableExplainedByIgnoredChecks value (equal by construction, pinned, #10055)", () => { + // #10055: the loop used to iterate only the raw state, so it only ever exercised the arm where + // unstableExplainedByIgnoredChecks is absent -- the one arm the two functions still agreed on. A + // helper that dropped the flag entirely could pass this test forever. Iterating all three flag values + // for every state is what makes "equal by construction" (the JSDoc's own claim) an actual proof. for (const raw of RAW_STATES) { - expect(isCommentMergeStateHeld(raw), `state=${String(raw)}`).toBe( - derivePrDisposition(dispositionInput({ mergeableState: raw })).commentMergeStateHeld, - ); + for (const flag of [undefined, false, true] as const) { + expect(isCommentMergeStateHeld(raw, flag), `state=${String(raw)} flag=${String(flag)}`).toBe( + derivePrDisposition(dispositionInput({ mergeableState: raw, unstableExplainedByIgnoredChecks: flag })).commentMergeStateHeld, + ); + } } }); + + it("REGRESSION (#10055): an unstable state fully explained by the ignore list is not comment-held either", () => { + // The exact pair that disagreed before this fix: isCommentMergeStateHeld ignored the flag entirely and + // returned true, while the planner's derivePrDisposition had already stopped holding. + expect(isCommentMergeStateHeld("unstable", true)).toBe(false); + expect( + derivePrDisposition(dispositionInput({ mergeableState: "unstable", unstableExplainedByIgnoredChecks: true })).commentMergeStateHeld, + ).toBe(false); + }); }); // ── Cross-surface: the PLANNER's actions must agree with the disposition for every mergeable state ───────── @@ -217,6 +232,19 @@ describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: true }); expect(d.wouldApprove).toBe(true); }); + + it("deriveUnifiedStatus (unified-comment.ts's ready→held branch) agrees with the resolved boolean on both arms (#10055)", () => { + // An otherwise-"ready" status must stay ready once the ignore list explains the instability... + const stillReady = deriveUnifiedStatus( + readyInput({ mergeStateLabel: "unstable", mergeStateHeld: isCommentMergeStateHeld("unstable", true) } as never), + ); + expect(stillReady).toBe("ready"); + // ...and must still downgrade to held when the same raw label is NOT explained by an ignored check. + const stillHeld = deriveUnifiedStatus( + readyInput({ mergeStateLabel: "unstable", mergeStateHeld: isCommentMergeStateHeld("unstable", false) } as never), + ); + expect(stillHeld).toBe("held"); + }); }); describe("guardrail hold released by a clean escalated review (#9808 second half)", () => { diff --git a/test/unit/processors-public-comment-merge-facts.test.ts b/test/unit/processors-public-comment-merge-facts.test.ts index 714854f6fc..84958b97b4 100644 --- a/test/unit/processors-public-comment-merge-facts.test.ts +++ b/test/unit/processors-public-comment-merge-facts.test.ts @@ -101,6 +101,73 @@ describe("derivePublicCommentMergeFacts() — failing-check projection (#4607)", }); }); +describe("derivePublicCommentMergeFacts() — mergeStateHeld honours the ignored-checks resolution (#10055)", () => { + it("does NOT hold an unstable state fully explained by an ignored, non-passing check", () => { + const { mergeReadiness } = facts({ + liveMergeState: "unstable", + mergeableState: "unstable", + liveCi: { + ciState: "passed", + failingDetails: [], + nonRequiredFailingDetails: [], + ignoredCheckDetails: [{ name: "Contributor trust", appSlug: "some-app", conclusion: "failure" }], + }, + }); + expect(mergeReadiness.mergeStateHeld).toBe(false); + }); + + it("still holds an unstable state when the ignored check passed (nothing explains the instability)", () => { + const { mergeReadiness } = facts({ + liveMergeState: "unstable", + mergeableState: "unstable", + liveCi: { + ciState: "passed", + failingDetails: [], + nonRequiredFailingDetails: [], + ignoredCheckDetails: [{ name: "Contributor trust", appSlug: "some-app", conclusion: "success" }], + }, + }); + expect(mergeReadiness.mergeStateHeld).toBe(true); + }); + + it("still holds an unstable state when some OTHER non-required check is also red", () => { + const { mergeReadiness } = facts({ + liveMergeState: "unstable", + mergeableState: "unstable", + liveCi: { + ciState: "passed", + failingDetails: [], + nonRequiredFailingDetails: [{ name: "advisory-scan", summary: "1 note" }], + ignoredCheckDetails: [{ name: "Contributor trust", appSlug: "some-app", conclusion: "failure" }], + }, + }); + expect(mergeReadiness.mergeStateHeld).toBe(true); + }); + + it("still holds an unstable state when CI itself failed, even with a non-passing ignored check", () => { + const { mergeReadiness } = facts({ + liveMergeState: "unstable", + mergeableState: "unstable", + liveCi: { + ciState: "failed", + failingDetails: [{ name: "codecov/patch" }], + nonRequiredFailingDetails: [], + ignoredCheckDetails: [{ name: "Contributor trust", appSlug: "some-app", conclusion: "failure" }], + }, + }); + expect(mergeReadiness.mergeStateHeld).toBe(true); + }); + + it("has no ignored checks to explain anything when ignoredCheckDetails is omitted (byte-identical to before #10055)", () => { + const { mergeReadiness } = facts({ + liveMergeState: "unstable", + mergeableState: "unstable", + liveCi: { ciState: "passed", failingDetails: [], nonRequiredFailingDetails: [] }, + }); + expect(mergeReadiness.mergeStateHeld).toBe(true); + }); +}); + describe("derivePublicCommentMergeFacts() — heldForReview (#guarded-hold-comment, #4607)", () => { it("holds a PR whose diff touches a hard-guardrail path, and does not hold one that doesn't", () => { expect(facts({ unifiedFiles: [GUARDED_FILE] }).heldForReview).toBe(true);