diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b4bdec09e..9b03e5afe 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 c4a0c3542..226194884 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 f992eea97..d94ad50e4 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 714854f6f..84958b97b 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);