diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 08c46226c7..72c8f92e3a 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1286,12 +1286,30 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // // `manualReviewLabelAppliedSha` is the missing provenance. Non-null means the PLANNER applied it, so the // planner may take it back; null means a human applied it (or it predates the column) and it is left - // strictly alone. Guarded on `manualHoldReason === null`, i.e. NO reason wants a hold this pass -- which is - // why this cannot lift a label applied for reason A just because reason B cleared. Deliberately not scoped - // to the recorded head: a rebase that resolves the cause is the single most common way a hold goes stale, - // and refusing to lift it there would leave the exact #9935 case unfixed. - if ( + // strictly alone. Deliberately not scoped to the recorded head: a rebase that resolves the cause is the + // single most common way a hold goes stale, and refusing to lift it there would leave the #9935 case unfixed. + // + // `manualHoldReason === null` alone is NOT "nothing wants a hold". That ternary covers the guardrail hit, + // unverified CI, an action_required conclusion and a not-review-good verdict -- but every one of the + // would-MERGE holds in sections 1c-1g below fires on `reviewGood` with a SUCCESS conclusion, which is + // precisely when manualHoldReason is null. Releasing on it alone therefore stripped the label while a + // migration collision, an unlinked-issue match, a priority-eligibility hold, an unlinked-issue close or an + // unstable merge state was still live -- and that label is the exact thing the executor checks to deny + // merge and approve, so removing it removes the enforcement for the hold that is still standing. + // + // `noManualReviewHoldWanted` is the complete condition: every reason that would ADD this label, in one + // place, so a new hold added below cannot silently fail to suppress the release. + const noManualReviewHoldWanted = manualHoldReason === null && + input.migrationCollisionHold === undefined && + input.unlinkedIssueMatchHold === undefined && + input.priorityEligibilityHold === undefined && + input.unlinkedIssueMatchClose === undefined && + !mergeableStateUnstable && + !heldForManualReview && + !mergeTerminallyBlocked; + if ( + noManualReviewHoldWanted && labels.manualReview !== null && input.manualReviewLabelAppliedSha != null && hasLabel(input.pr.labels, labels.manualReview) diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 36dafd4add..9da7b2fe57 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2806,3 +2806,47 @@ describe("priority-eligibility hold (#9738)", () => { expect(plan).toContain("merge"); }); }); + +describe("manual-review release must not fire while a hold is still live (#9939 follow-up)", () => { + // #9942 added the release guarded on `manualHoldReason === null`, and its comment stated that no add site + // could be live at that point. That ternary only covers the guardrail hit, unverified CI, an + // action_required conclusion and a not-review-good verdict. Every would-MERGE hold fires on `reviewGood` + // with a SUCCESS conclusion -- exactly when manualHoldReason is null -- so the release stripped the label + // while the hold was still standing. That label is what the executor checks to DENY merge and approve. + const held = (over: Partial) => + planAgentMaintenanceActions( + input({ + conclusion: "success", + autonomy: { merge: "auto", review_state_label: "auto" }, + manualReviewLabelAppliedSha: "abc123", + pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" }, + ...over, + }), + ); + + const releases = (actions: PlannedAgentAction[]) => + actions.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove"); + + const liveHolds: Array<[string, Partial]> = [ + ["a migration collision", { migrationCollisionHold: { reason: "collides with main", comment: "rebase" } }], + ["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" } }], + ]; + + for (const [name, over] of liveHolds) { + it(`keeps the label while ${name} is live`, () => { + expect(releases(held(over))).toBe(false); + }); + } + + 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); + }); + + it("still refuses to touch a label with no recorded provenance", () => { + expect(releases(held({ manualReviewLabelAppliedSha: null }))).toBe(false); + }); +});