diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d5584ab309..eee6d7fdb6 100644 --- a/src/queue/processors.ts +++ b/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 } : {}), diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 76cb317a3b..5342b07486 100644 --- a/src/review/unified-comment.ts +++ b/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"; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 6c269fd29d..18e1f35c97 100644 --- a/src/settings/agent-actions.ts +++ b/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,24 +1022,27 @@ 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 @@ -1045,8 +1050,6 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // 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"), diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts new file mode 100644 index 0000000000..01994c1f7e --- /dev/null +++ b/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"; +} diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts new file mode 100644 index 0000000000..589193d3f3 --- /dev/null +++ b/test/unit/pr-disposition-invariants.test.ts @@ -0,0 +1,179 @@ +import { describe, expect, it } from "vitest"; +import { assessMergeableState, derivePrDisposition, isCommentMergeStateHeld, type PrDispositionInput } from "../../src/settings/pr-disposition"; +import { AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, planAgentMaintenanceActions, type AgentActionPlanInput } from "../../src/settings/agent-actions"; +import { deriveUnifiedStatus, type UnifiedReviewInput } from "../../src/review/unified-comment"; +import type { GateCheckConclusion } from "../../src/rules/advisory"; + +// #8759 (epic #8757): the cross-surface invariant suite. The #8711 incident's root CLASS was four +// surfaces interpreting the raw mergeable_state string independently. These tests pin the shared +// contract: the pure module's own semantics, the planner consuming it verbatim, and the comment +// renderer agreeing with it through the bridge-passed boolean. A regression that re-introduces a +// private raw-string interpretation in any surface shows up here as a cross-surface disagreement. + +const RAW_STATES = ["clean", "dirty", "behind", "unstable", "blocked", "unknown", "", null, undefined] as const; + +function dispositionInput(over: Partial = {}): PrDispositionInput { + return { + mergeableState: "clean", + reviewGood: true, + guardrailHit: false, + migrationCollisionHold: false, + unlinkedIssueMatchHold: false, + advisoryCheckHold: false, + unlinkedIssueMatchCloseWithoutCloseActing: false, + ...over, + }; +} + +describe("assessMergeableState — THE single interpretation point", () => { + it("maps every raw state to its one meaning, case-insensitively; anything unrecognized is indeterminate", () => { + expect(assessMergeableState("clean")).toBe("clean"); + expect(assessMergeableState("CLEAN")).toBe("clean"); + expect(assessMergeableState("dirty")).toBe("conflict"); + expect(assessMergeableState("behind")).toBe("behind"); + expect(assessMergeableState("unstable")).toBe("unstable"); + for (const raw of ["blocked", "unknown", "draft", "", null, undefined]) { + expect(assessMergeableState(raw), `raw=${String(raw)}`).toBe("indeterminate"); + } + }); +}); + +describe("derivePrDisposition — module-level invariants over the full state matrix", () => { + it("wouldMerge implies wouldApprove, for every state x hold combination", () => { + for (const raw of RAW_STATES) { + for (const guardrailHit of [false, true]) { + for (const reviewGood of [false, true]) { + const d = derivePrDisposition(dispositionInput({ mergeableState: raw, guardrailHit, reviewGood })); + if (d.wouldMerge) expect(d.wouldApprove, `state=${String(raw)} guardrail=${guardrailHit}`).toBe(true); + } + } + } + }); + + it("held always suppresses both approve and merge", () => { + for (const raw of RAW_STATES) { + const d = derivePrDisposition(dispositionInput({ mergeableState: raw, migrationCollisionHold: true })); + expect(d.heldForManualReview).toBe(true); + expect(d.wouldApprove).toBe(false); + expect(d.wouldMerge).toBe(false); + } + }); + + it("unstable holds by itself; behind/blocked/unknown never hold and stay approvable; conflict is unapprovable but never a hold", () => { + const unstable = derivePrDisposition(dispositionInput({ mergeableState: "unstable" })); + expect(unstable.heldForManualReview).toBe(true); + expect(unstable.heldForUnstableMergeState).toBe(true); + expect(unstable.wouldApprove).toBe(false); + + for (const raw of ["behind", "blocked", "unknown", undefined]) { + const d = derivePrDisposition(dispositionInput({ mergeableState: raw as string | undefined })); + expect(d.heldForManualReview, `state=${String(raw)}`).toBe(false); + expect(d.wouldApprove, `state=${String(raw)}`).toBe(true); + expect(d.wouldMerge, `state=${String(raw)}`).toBe(false); // merge stays clean-only + } + + const conflict = derivePrDisposition(dispositionInput({ mergeableState: "dirty" })); + expect(conflict.heldForManualReview).toBe(false); // the close path owns conflicts + expect(conflict.wouldApprove).toBe(false); + expect(conflict.wouldMerge).toBe(false); + }); + + it("isCommentMergeStateHeld equals derivePrDisposition(...).commentMergeStateHeld for every raw state (equal by construction, pinned)", () => { + for (const raw of RAW_STATES) { + expect(isCommentMergeStateHeld(raw), `state=${String(raw)}`).toBe( + derivePrDisposition(dispositionInput({ mergeableState: raw })).commentMergeStateHeld, + ); + } + }); +}); + +// ── Cross-surface: the PLANNER's actions must agree with the disposition for every mergeable state ───────── + +function planInput(mergeableState: string | undefined, over: Partial = {}): AgentActionPlanInput { + return { + conclusion: "success" as GateCheckConclusion, + blockerTitles: [], + autonomy: { approve: "auto", merge: "auto", review_state_label: "auto" }, + autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, + slopGateMinScore: 60, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "passed", + pr: { labels: [], ...(mergeableState !== undefined ? { mergeableState } : {}) }, + ...over, + }; +} + +describe("cross-surface: planner actions agree with the shared disposition (#8759)", () => { + it("for every mergeable state on a green PR: approve/merge planned iff the disposition allows them, and the label matches held-ness", () => { + for (const raw of ["clean", "behind", "unstable", "blocked", "unknown", undefined]) { + const d = derivePrDisposition(dispositionInput({ mergeableState: raw })); + const actions = planAgentMaintenanceActions(planInput(raw as string | undefined)); + const classes = actions.map((a) => a.actionClass); + expect(classes.includes("approve"), `approve state=${String(raw)}`).toBe(d.wouldApprove); + expect(classes.includes("merge"), `merge state=${String(raw)}`).toBe(d.wouldMerge); + const stateLabel = actions.find((a) => a.actionClass === "label" && a.labelOp !== "remove"); + if (d.heldForManualReview) { + expect(stateLabel?.label, `label state=${String(raw)}`).toBe(AGENT_LABEL_NEEDS_REVIEW); + } else { + expect(stateLabel?.label, `label state=${String(raw)}`).toBe(AGENT_LABEL_READY); + } + } + }); + + it("RC3 stays planner-private: a terminally-blocked head suppresses ONLY the merge (disposition still wouldMerge), and a new head lifts it", () => { + // Same clean/green state; the only variable is the planner-private mergeBlockedSha/headSha pair. + const blocked = planAgentMaintenanceActions(planInput("clean", { pr: { labels: [], mergeableState: "clean", headSha: "abc", mergeBlockedSha: "abc" } })); + expect(blocked.map((a) => a.actionClass)).not.toContain("merge"); + expect(derivePrDisposition(dispositionInput({ mergeableState: "clean" })).wouldMerge).toBe(true); // not the disposition's business + const unblocked = planAgentMaintenanceActions(planInput("clean", { pr: { labels: [], mergeableState: "clean", headSha: "new", mergeBlockedSha: "abc" } })); + expect(unblocked.map((a) => a.actionClass)).toContain("merge"); + // Absent SHAs (never terminally blocked) also merge — the != null arms. + const absent = planAgentMaintenanceActions(planInput("clean")); + expect(absent.map((a) => a.actionClass)).toContain("merge"); + }); + + it("dirty (conflict) on a green contributor PR: no approve, no merge, close path engaged — matching the disposition's conflict semantics", () => { + const d = derivePrDisposition(dispositionInput({ mergeableState: "dirty" })); + expect(d.wouldApprove).toBe(false); + const actions = planAgentMaintenanceActions(planInput("dirty", { autonomy: { approve: "auto", merge: "auto", close: "auto" } })); + const classes = actions.map((a) => a.actionClass); + expect(classes).not.toContain("approve"); + expect(classes).not.toContain("merge"); + expect(classes).toContain("close"); + }); +}); + +// ── Cross-surface: the COMMENT renderer agrees with the disposition through the bridge boolean ───────────── + +function readyInput(readiness: NonNullable): UnifiedReviewInput { + return { decision: "merge", readiness: { ...readiness, ciState: "passed" } } as UnifiedReviewInput; +} + +describe("cross-surface: deriveUnifiedStatus consumes the bridge-resolved boolean and agrees with the disposition (#8759)", () => { + it("an otherwise-ready status downgrades to held exactly when the shared interpretation says the merge state is held", () => { + for (const raw of ["clean", "dirty", "behind", "unstable", "blocked", "unknown"]) { + const held = isCommentMergeStateHeld(raw); + const status = deriveUnifiedStatus(readyInput({ mergeStateLabel: raw, mergeStateHeld: held } as never)); + expect(status, `state=${raw}`).toBe(held ? "held" : "ready"); + } + }); + + it("the resolved boolean is authoritative over the raw label when both are present", () => { + // A hypothetical future state the raw-string fallback wouldn't hold on: the boolean wins. + const status = deriveUnifiedStatus(readyInput({ mergeStateLabel: "totally-new-state", mergeStateHeld: true } as never)); + expect(status).toBe("held"); + const notHeld = deriveUnifiedStatus(readyInput({ mergeStateLabel: "dirty", mergeStateHeld: false } as never)); + expect(notHeld).toBe("ready"); + }); + + it("LEGACY callers (no resolved boolean) keep the byte-identical pre-#8759 raw-string behavior, which matches the shared interpretation", () => { + for (const raw of ["clean", "dirty", "behind", "unstable", "blocked", "unknown"]) { + const legacy = deriveUnifiedStatus(readyInput({ mergeStateLabel: raw } as never)); + expect(legacy, `state=${raw}`).toBe(isCommentMergeStateHeld(raw) ? "held" : "ready"); + } + }); +}); diff --git a/test/unit/processors-public-comment-merge-facts.test.ts b/test/unit/processors-public-comment-merge-facts.test.ts index 24613be258..714854f6fc 100644 --- a/test/unit/processors-public-comment-merge-facts.test.ts +++ b/test/unit/processors-public-comment-merge-facts.test.ts @@ -57,7 +57,8 @@ describe("derivePublicCommentMergeFacts() — ciState (#4607)", () => { describe("derivePublicCommentMergeFacts() — failing-check projection (#4607)", () => { it("omits the failing keys entirely when nothing is red", () => { const { mergeReadiness } = facts(); - expect(mergeReadiness).toEqual({ ciState: "passed", mergeStateLabel: "clean" }); + // #8759: mergeStateHeld is the bridge-resolved shared interpretation — false for a clean state. + expect(mergeReadiness).toEqual({ ciState: "passed", mergeStateLabel: "clean", mergeStateHeld: false }); }); it("projects name + optional summary/detailsUrl, dropping absent optionals", () => {