diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 59d3e4b0f..7f156507d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -819,33 +819,11 @@ function advisoryHoldComment(holds: ReadonlyArray<{ name: string; appSlug: strin return `Held for manual review: a maintainer-configured advisory check-run reported a non-passing result — ${parts.join("; ")}. This does not block CI, but a maintainer should review it. This is an automated maintenance action.`; } -// #8758: name the non-required check(s) behind a GitHub "unstable" mergeable state, for the hold reason (audit) -// and the public comment. Check names are already public on the PR's own Checks tab, so interpolating them is -// safe; summaries/URLs are deliberately NOT interpolated (same public-safe-comment posture as advisoryHoldComment). -// Falls back to generic wording when the CI aggregate couldn't itemize the source (the hold keys on -// mergeable_state itself, never on this list being non-empty). -function mergeUnstableHoldReason(failures: ReadonlyArray<{ name: string }> | undefined): string { - const names = (failures ?? []).map((f) => `"${f.name}"`); - return names.length > 0 - ? `mergeable_state is unstable — non-required check(s) not passing: ${names.join("; ")}` - // #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(", ")} —` : ""; - 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.`; -} +// #10116: `mergeUnstableHoldReason` and `mergeUnstableHoldComment` lived here. Both existed only to word the +// manual-review hold that a GitHub "unstable" mergeable state used to trigger; with no such hold there is no +// action left to hang either string on (there is no comment-only AgentActionClass — a comment always rides a +// label/approve/merge). `nonRequiredCheckFailures` stays on the planner input: the ignored-check +// reconciliation still reads it, and it is what #9810's `unstableExplainedByIgnoredChecks` is computed from. /** * Plan best-effort assignment of the PR's opening contributor (#3182), independent of merge/close/CI outcome. @@ -1366,7 +1344,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne input.priorityEligibilityHold === undefined && input.screenshotEvidenceHold === undefined && input.unlinkedIssueMatchClose === undefined && - !mergeableStateUnstable && + // #10116: `!mergeableStateUnstable` deliberately REMOVED. An unstable state no longer applies this label, + // so it must not block lifting one either — leaving it here would strand the label on any PR whose CI + // happened to be red when an unrelated hold cleared. That is the latch #10098 was stuck in. !heldForManualReview && !mergeTerminallyBlocked; if ( @@ -1472,21 +1452,26 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } - // 1f) unstable-mergeable-state manual-review fallback (#8758): mirrors 1d. When the dedicated - // review_state_label class isn't acting but merge is, a would-merge suppressed ONLY by GitHub's "unstable" - // mergeable state must still surface a visible hold — label + comment naming the non-required check — - // instead of the silent stall #8711 hit (approve posted, merge never planned, nothing on the PR saying why). - if (reviewGood && mergeableStateUnstable && !acting("review_state_label") && labels.manualReview !== null && acting("merge") && !hasLabelOrPlanned(input.pr.labels, actions, labels.manualReview)) { - actions.push({ - actionClass: "label", - autonomyClass: "merge", - requiresApproval: approval("merge"), - reason: `verdict=${conclusion}; ${mergeUnstableHoldReason(input.nonRequiredCheckFailures)}`, - label: labels.manualReview, - labelOp: "add", - comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)), - }); - } + // 1f) REMOVED (#10116). This was the unstable-mergeable-state manual-review fallback (#8758): when + // review_state_label was off but merge was acting, a would-merge suppressed only by GitHub's "unstable" + // state got the manual-review label plus a comment naming the non-required check. + // + // It was the dominant source of spurious holds. `reviewGood` means the GATE passed and REQUIRED CI passed, + // and `unstable` is GitHub's verdict on everything else — so the two are true together on any PR whose + // non-required checks are red OR STILL RUNNING. That is most PRs, most of the time, and each one got an + // ENFORCEMENT label: the executor denies merge and approve while it is present, and merge-train.ts evicts a + // labelled sibling from the train entirely, so the PR was skipped rather than queued. Worse, section 1b's + // release condition also tested this state, which made the label a LATCH — applied while CI was red, and + // then not liftable *because* CI was red. Observed on JSONbored/loopover#10098, stuck until a human + // removed the label by hand. + // + // Nothing replaces it, and that is deliberate. #8758's concern was the silent stall #8711 hit: approve + // posted, merge never planned, nothing on the PR saying why. All three legs stay closed without summoning + // anyone — `wouldApprove` conjoins `!unstableHolds` so no approval is posted, section 2 chooses NO + // disposition label rather than `ready-to-merge`, and the unified comment still refuses "safe to merge" via + // `mergeStateHeld`. Which checks are unhappy is on the PR's own Checks tab, where it is always current; + // restating it in a bot comment on every in-flight PR is noise, and the state resolves itself on the next + // push without anyone having read it. // 2) review_state_label (#label-scoping) — ready-to-merge (review-good, unguarded) / manual-review // (review-good but guarded) / changes-requested (not review-good → will be closed for a contributor, held for @@ -1506,7 +1491,17 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? labels.migrationCollision : heldForManualReview || mergeTerminallyBlocked ? labels.manualReview - : labels.readyToMerge; + : // #10116: an unstable merge state gets NEITHER disposition label. It no longer summons a human + // (see derivePrDisposition), but it must not claim `ready-to-merge` either — our own aggregate + // can read CI green while GitHub still reports unstable for a non-required check (#9810), and + // promising "ready" on a PR the merge then self-suppresses on is precisely #8711's "approved, + // labeled ready, never merged, nobody told". No label is the honest state: the PR is not held + // for a person and not ready to merge, it is waiting on checks. The unified comment still + // refuses "safe to merge" (via `mergeStateHeld`) and the Checks tab names the culprits, so + // nothing about this state is hidden from the contributor. + mergeableStateUnstable + ? null + : labels.readyToMerge; const reason = linkedIssueCloseInFlight ? `linked-issue hard rule: ${linkedIssueHardRule?.reason ?? "ineligible linked issue"}` : unlinkedIssueMatchViolated @@ -1521,18 +1516,23 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? `verdict=${conclusion}; ${input.unlinkedIssueMatchClose.reason}` : input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0 ? `verdict=${conclusion}; ${advisoryHoldReason(input.advisoryCheckHold)}` - : mergeableStateUnstable - ? `verdict=${conclusion}; ${mergeUnstableHoldReason(input.nonRequiredCheckFailures)}` - : heldForManualReview - ? `verdict=${conclusion}; ${guardrailReason}` - : mergeTerminallyBlocked - ? // #9012: the ONE place a terminal merge block becomes visible to a human. Everything - // else about this PR reads healthy — gate passing, CI green, bot-approved — so - // without naming the block here the label says "needs review" with no reason and the - // maintainer has nothing to act on. mergeBlockedReason is executor-written and - // already length-capped at persist time (280 chars). - `verdict=${conclusion}; CI green, but a prior merge attempt failed and is held: ${input.pr.mergeBlockedReason ?? "reason unrecorded"}` - : `verdict=${conclusion}; CI green`; + : // #10116: the `mergeableStateUnstable` arm that used to sit HERE is gone. It PRECEDED + // `heldForManualReview`, so once an unstable state stopped being a hold of its own, the + // only way to reach it with a label attached was a PR held for some OTHER reason that + // happened to also be unstable — and it would then have recorded "mergeable_state is + // unstable" as the cause of a label the guardrail (or a migration collision, or a + // terminal merge block) actually caused. A hold reason lands in the ledger and drives + // #9729's per-path clearance; misattributing it is worse than the noise it replaced. + heldForManualReview + ? `verdict=${conclusion}; ${guardrailReason}` + : mergeTerminallyBlocked + ? // #9012: the ONE place a terminal merge block becomes visible to a human. Everything + // else about this PR reads healthy — gate passing, CI green, bot-approved — so + // without naming the block here the label says "needs review" with no reason and the + // maintainer has nothing to act on. mergeBlockedReason is executor-written and + // already length-capped at persist time (280 chars). + `verdict=${conclusion}; CI green, but a prior merge attempt failed and is held: ${input.pr.mergeBlockedReason ?? "reason unrecorded"}` + : `verdict=${conclusion}; CI green`; if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) { actions.push({ actionClass: "label", @@ -1563,9 +1563,12 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // hasLabelOrPlanned and the contributor would get a bare label with no reason. input.screenshotEvidenceHold !== undefined ? { comment: sanitizePublicComment(input.screenshotEvidenceHold.comment) } - : mergeableStateUnstable - ? { comment: sanitizePublicComment(mergeUnstableHoldComment(input.nonRequiredCheckFailures)) } - : {} + : // #10116: the trailing `mergeableStateUnstable` arm is gone with the label that carried + // it. A comment here only reaches the PR when the ternary above chose a label, and an + // unstable state now chooses none — so the arm was reachable only on a PR held for an + // unrelated reason, where it would have replaced that hold's own message with one + // about checks. + {} : {}), }); } @@ -1640,9 +1643,12 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // safe to merge" review on a PR we're about to close is incoherent (and a stale approval strands the PR if it // later goes green). A `behind`/`blocked` PR is fine to approve (it is rebased pre-review or the approval clears // the block); only a hard `dirty` conflict is excluded here. (#ready-needs-mergeable, the #4220 report) - // 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. */ + // An `unstable` PR is excluded too, now via wouldApprove's OWN `!unstableHolds` term (#10116 — it used to + // ride on heldForManualReview, until an unstable state stopped being a hold): 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. #10116 dropped the manual-review LABEL for this state + // but deliberately kept this suppression — the two answer different questions ("does a human need to act", + // no; "may we say this is good to merge", not while GitHub disagrees). */ // #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) { diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index c4a0c3542..4ec203ad3 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -14,7 +14,7 @@ // // 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); +// resolves (assessment "conflict" → the close path owns it; "unstable" → CI itself owns it, #10116); // • "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); @@ -29,8 +29,11 @@ export type MergeableAssessment = | "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. */ + /** Required checks green but a non-required check/status is not, OR checks are still running — GitHub + * reports the same string for both. Merge self-suppresses, so approve must too, or the PR lands in + * #8711's "approved, labeled ready, never merged, nobody told" stall. It is NOT a manual hold (#10116): + * the state is visible on the PR's own Checks tab and resolves itself on the next push, so no maintainer + * is summoned for something CI has already answered. */ | "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. */ @@ -108,9 +111,11 @@ export type PrDisposition = { * suppressor is the unstable mergeable state — that is GitHub's computation, not one of our declared hold * inputs, and it is reported by `heldForUnstableMergeState` instead. */ heldBy: MergeHoldInput[]; - /** 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". */ + /** True when the ONLY thing suppressing a would-merge is the unstable mergeable state. Since #10116 this + * is REPORTING, not a hold: the planner reads it to choose NO disposition label (neither `manual-review` + * nor `ready-to-merge`), and the comment surface reads it to refuse the "safe to merge" claim. It is a + * separate field from `heldForManualReview` precisely so those two can differ — "not ready" and "a human + * must act" were conflated here, and the conflation is what labelled every in-flight PR. */ 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, @@ -149,9 +154,29 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { // automatically -- restatement is the thing that table exists to remove. Order is the table's own // declaration order, which makes the recorded cause deterministic for identical inputs. const heldBy = MERGE_HOLD_INPUT_KEYS.filter((key) => input[key] === true && releasedHolds[key] !== true); - const heldForManualReview = heldBy.length > 0 || unstableHolds; + // #10116: an unstable merge state NO LONGER summons a human. GitHub reports `unstable` for checks that are + // failing OR still running, so this term was labelling `manual-review` on every PR whose CI had not finished + // -- the dominant cause of the held-with-`reason_code: success` records on the production Orb, and pure + // noise on a repo meant to run autonomously. Neither case wants a maintainer: a pending PR is simply not + // ready, and a failing one has already been answered by CI, so the contributor fixes CI rather than a human + // reviewing something that cannot merge. + // + // This does NOT make an unstable PR mergeable. `wouldMerge` requires `mergeable === "clean"` independently, + // and `reviewGood` requires green CI, so both remain false. What changes is only whether a human is summoned. + const heldForManualReview = heldBy.length > 0; + // Still reported, and still true: the comment surface reads it to refuse the "safe to merge" claim (#8758). + // Saying "this is not ready" is useful; escalating it to a maintainer is not. const heldForUnstableMergeState = unstableHolds; - const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict"; + // `!unstableHolds` is now EXPLICIT rather than riding on heldForManualReview. It used to be implied by the + // unstable term above, and dropping that term without restating it here would let approve fire on an + // unstable PR while merge self-suppressed -- exactly #8711's "approved, labeled ready, never merged, nobody + // told" incident. `reviewGood` alone does not cover it: our own CI aggregate can read passed while GitHub + // still reports unstable for a non-required check. + // + // Keyed on `unstableHolds`, NOT on `mergeable === "unstable"`. The difference is #9810: an instability + // explained solely by a check the maintainer declared meaningless must stay APPROVABLE, and a blanket + // mergeable-state test would silently un-fix that. Caught by its own test when I first wrote it too broadly. + const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict" && !unstableHolds; 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" diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 5234bb2a8..0f3d86f45 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -755,38 +755,44 @@ describe("planAgentMaintenanceActions (#778)", () => { pr: { labels: [], mergeableState: "unstable", reviewDecision: "APPROVED" as const }, }; - it("the #8711 regression: does NOT approve, does NOT merge, and surfaces a manual-review label + comment naming the check (review_state_label not acting)", () => { + it("REGRESSION (#10116): the #8711 protections hold with NO label at all, on the MERGE-autonomy path", () => { + // One of TWO sites #8758 labelled this state from. This is the fallback authorized by merge autonomy, + // for one-shot repos that configure manualReviewLabel without the ready/changes disposition labels; + // the next test covers section 2's disposition ternary. Removing only one would have left half the + // repos exactly as stuck, so each removal is pinned independently. + // + // What #8758 actually existed to prevent is asserted and unchanged: no approve, no merge. The stall it + // feared -- approve posted, merge never planned, nothing saying why -- cannot happen without the approve. const plan = planAgentMaintenanceActions(input({ ...unstable, autonomy: { approve: "auto", merge: "auto" } })); expect(classes(plan)).not.toContain("approve"); expect(classes(plan)).not.toContain("merge"); - const label = plan.find((a) => a.actionClass === "label"); - expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); - expect(label?.reason).toContain("unstable"); - expect(label?.reason).toContain("Third-Party Trust Check"); - expect(label?.comment).toContain("unstable"); - expect(label?.comment).toContain("Third-Party Trust Check"); - expect(label?.comment).toContain("will not auto-merge"); + expect(plan.find((a) => a.actionClass === "label" && a.labelOp !== "remove")).toBeUndefined(); }); - it("labels manual-review (never ready-to-merge) with the unstable reason + comment when review_state_label IS acting", () => { - const plan = planAgentMaintenanceActions(input({ ...unstable, autonomy: { review_state_label: "auto", merge: "auto" } })); - const label = plan.find((a) => a.actionClass === "label"); - expect(label?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); - expect(label?.label).not.toBe(AGENT_LABEL_READY); - expect(label?.reason).toContain("unstable"); - expect(label?.reason).toContain("Third-Party Trust Check"); - expect(label?.comment).toContain("Third-Party Trust Check"); + it("REGRESSION (#10116): applies NEITHER disposition label when review_state_label IS acting", () => { + // The other site. `manual-review` summoned a maintainer for a PR whose checks were merely unfinished + // AND evicted it from the merge train (merge-train.ts filters on heldForManualReview) -- the behaviour + // that made the repo look non-autonomous. `ready-to-merge` would be the opposite error: promising a + // merge that self-suppresses. Neither is right, so neither is emitted. + const plan = planAgentMaintenanceActions(input({ ...unstable, autonomy: { review_state_label: "auto", merge: "auto", approve: "auto" } })); + const label = plan.find((a) => a.actionClass === "label" && a.labelOp !== "remove"); + expect(label?.label).toBeUndefined(); expect(classes(plan)).not.toContain("merge"); + expect(classes(plan)).not.toContain("approve"); }); - it("still holds with GENERIC wording when the CI aggregate could not itemize the culprit (empty/absent failures list)", () => { + it("REGRESSION (#10116): behaves identically when the CI aggregate could not itemize the culprit", () => { + // #8758 branched on whether `nonRequiredCheckFailures` named the check, to pick specific vs generic + // wording for the hold comment. With no hold and no comment that branch is gone -- and the reason to + // keep this case is that the un-itemized path must not be the one that still labels. It is the MORE + // common shape in production (a commit status rather than a check-run, or a run that appeared after CI + // was read), so a fallback surviving only here would look fixed in tests and stay broken on the Orb. const plan = planAgentMaintenanceActions( input({ conclusion: "success", ciState: "passed", autonomy: { approve: "auto", merge: "auto" }, pr: { labels: [], mergeableState: "unstable", reviewDecision: "APPROVED" } }), ); expect(classes(plan)).not.toContain("approve"); expect(classes(plan)).not.toContain("merge"); - const label = plan.find((a) => a.actionClass === "label"); - expect(label?.comment).toContain("a non-required check or status"); + expect(plan.find((a) => a.actionClass === "label" && a.labelOp !== "remove")).toBeUndefined(); }); it("never CLOSES on unstable — the hold only downgrades a would-merge, close semantics untouched", () => { @@ -2878,7 +2884,10 @@ describe("manual-review release must not fire while a hold is still live (#9939 ["an unlinked-issue match hold", { unlinkedIssueMatchHold: { reason: "matches an unlinked issue", comment: "c" } }], ["a priority-eligibility hold", { priorityEligibilityHold: { reason: "outside the priority window", comment: "c" } }], ["an unlinked-issue close", { unlinkedIssueMatchClose: { reason: "unlinked close", comment: "c" } }], - ["an unstable merge state", { pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "unstable" } }], + // "an unstable merge state" deliberately REMOVED (#10116) -- it is no longer a manual-review hold, so it + // must not block the release either. Leaving it here would strand the label on any PR whose CI happened + // to be red when an unrelated hold cleared, which is the one-way latch #9942 fixed. Its release behaviour + // is asserted directly below instead. ]; for (const [name, over] of liveHolds) { @@ -2887,6 +2896,13 @@ describe("manual-review release must not fire while a hold is still live (#9939 }); } + it("REGRESSION (#10116): RELEASES the label when the only thing left is an unstable merge state", () => { + // The stuck-PR case from production (#10098): CI finished red, the label stayed, the PR left the merge + // train, and it needed a human to strip the label by hand. An unstable state no longer wants the label, + // so the planner takes back the one it applied. + expect(releases(held({ pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "unstable" } }))).toBe(true); + }); + it("still releases when nothing at all holds the PR — the #9935 case this must not regress", () => { // The counterweight: making the guard too broad would silently restore the one-way latch #9942 fixed. expect(releases(held({}))).toBe(true); diff --git a/test/unit/hold-cause.test.ts b/test/unit/hold-cause.test.ts index db0ce6d79..56ea65041 100644 --- a/test/unit/hold-cause.test.ts +++ b/test/unit/hold-cause.test.ts @@ -64,12 +64,19 @@ describe("derivePrDisposition heldBy (#9991)", () => { } }); - it("stays empty when only the unstable mergeable state suppresses the merge", () => { - // That is GitHub's computation, not one of our declared inputs, and is reported separately. + it("REGRESSION (#10116): an unstable mergeable state is reported separately and holds NOTHING", () => { + // That is GitHub's computation, not one of our declared inputs. It used to set heldForManualReview + // anyway, which put the manual-review label -- an ENFORCEMENT label the executor and the merge train both + // read -- on every PR whose checks were red OR still running. `heldBy` stayed empty throughout, which is + // how those holds reached the ledger as reason "success" with no cause: 518 of them on the production Orb. + // The two fields now agree, and the state is carried by heldForUnstableMergeState alone. const disposition = derivePrDisposition(baseInput({ mergeableState: "unstable" })); expect(disposition.heldBy).toEqual([]); expect(disposition.heldForUnstableMergeState).toBe(true); - expect(disposition.heldForManualReview).toBe(true); + expect(disposition.heldForManualReview).toBe(false); + // ...and it is still not mergeable, and still not approvable. Dropping the hold must not promote it. + expect(disposition.wouldMerge).toBe(false); + expect(disposition.wouldApprove).toBe(false); }); }); diff --git a/test/unit/pr-disposition-invariants.test.ts b/test/unit/pr-disposition-invariants.test.ts index f992eea97..25538deea 100644 --- a/test/unit/pr-disposition-invariants.test.ts +++ b/test/unit/pr-disposition-invariants.test.ts @@ -60,12 +60,23 @@ describe("derivePrDisposition — module-level invariants over the full state ma } }); - it("unstable holds by itself; behind/blocked/unknown never hold and stay approvable; conflict is unapprovable but never a hold", () => { + it("REGRESSION (#10116): unstable no longer summons a human, but is still unapprovable and unmergeable", () => { + // GitHub reports `unstable` for checks that are failing OR still running. Treating that as a manual-review + // hold labelled every PR whose CI had not finished and pulled it out of the merge train, which is the + // opposite of autonomous. Neither case wants a maintainer: a pending PR is not ready, and a failing one + // has already been answered by CI. const unstable = derivePrDisposition(dispositionInput({ mergeableState: "unstable" })); - expect(unstable.heldForManualReview).toBe(true); + expect(unstable.heldForManualReview).toBe(false); + // Still reported -- the comment surface reads it to refuse the "safe to merge" claim (#8758). expect(unstable.heldForUnstableMergeState).toBe(true); + // #8711 stays closed. `wouldApprove` now excludes unstable EXPLICITLY rather than via the hold term: our + // aggregate can read CI green while GitHub still reports unstable for a non-required check (#9810), so + // dropping the hold without this would approve a PR the merge then self-suppresses on. expect(unstable.wouldApprove).toBe(false); + expect(unstable.wouldMerge).toBe(false); + }); + it("behind/blocked/unknown never hold and stay approvable; conflict is unapprovable but never a hold", () => { 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); @@ -119,6 +130,10 @@ describe("cross-surface: planner actions agree with the shared disposition (#875 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 if (d.heldForUnstableMergeState) { + // #10116: unstable earns NEITHER label. Not held for a person, and not ready to merge either -- + // claiming ready here is #8711 ("approved, labeled ready, never merged, nobody told"). + expect(stateLabel?.label, `label state=${String(raw)}`).toBeUndefined(); } else { expect(stateLabel?.label, `label state=${String(raw)}`).toBe(AGENT_LABEL_READY); } @@ -194,14 +209,19 @@ describe("unstable explained only by an IGNORED check (#9810 follow-up)", () => expect(d.commentMergeStateHeld).toBe(false); }); - it("INVARIANT: unstable from ANY other cause still holds — the flag is not a blanket override", () => { + it("INVARIANT: unstable from ANY other cause is still REPORTED — the flag is not a blanket override", () => { + // #10116 changed what an unstable state COSTS (no manual-review hold), not whether it is detected. The + // flag's job is still to say "this instability is explained by a check the maintainer declared + // meaningless", and that distinction has to survive — it is what still separates an approvable unstable + // PR from an unapprovable one. const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: false }); - expect(d.heldForManualReview).toBe(true); expect(d.heldForUnstableMergeState).toBe(true); + expect(d.wouldApprove).toBe(false); }); - it("INVARIANT: absent flag behaves exactly as before (byte-identical for every existing caller)", () => { - expect(derivePrDisposition({ ...base, mergeableState: "unstable" }).heldForManualReview).toBe(true); + it("INVARIANT: an explained-by-ignored-checks unstable state is not even reported as unstable", () => { + const d = derivePrDisposition({ ...base, mergeableState: "unstable", unstableExplainedByIgnoredChecks: true }); + expect(d.heldForUnstableMergeState).toBe(false); }); it("INVARIANT: the flag never rescues a PR held for a DIFFERENT reason", () => { @@ -246,7 +266,9 @@ describe("guardrail hold released by a clean escalated review (#9808 second half { migrationCollisionHold: true }, { unlinkedIssueMatchHold: true }, { advisoryCheckHold: true }, - { mergeableState: "unstable" }, + // `{ mergeableState: "unstable" }` deliberately REMOVED (#10116): an unstable state is no longer a + // manual-review hold at all, so it is not one of the "other holds" this invariant is about. Its own + // behaviour -- reported, unapprovable, unmergeable, but never a human summons -- has a dedicated test. ]) { const d = derivePrDisposition({ ...base, guardrailEscalationCleared: true, ...extra }); expect(d.heldForManualReview, JSON.stringify(extra)).toBe(true);