From f1e1e42fe3d2f7ea16d4aa4ed1551b70d9bae181 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 12:25:37 -0700 Subject: [PATCH] fix(gate): do not lift the manual-review hold while another hold is live #9942 added the release guarded on `manualHoldReason === null`, and stated that no add site could be live at that point. That ternary covers only the guardrail hit, unverified CI, an action_required conclusion and a not-review-good verdict. Every would-MERGE hold -- migration collision, unlinked-issue match, priority eligibility, unlinked-issue close, unstable merge state -- fires on `reviewGood` with a SUCCESS conclusion, which is precisely when manualHoldReason is null. So the release stripped the label while the hold was still standing, and that label is exactly what the executor checks to deny merge and approve: removing it removes the enforcement for a hold nothing has resolved. Verified against the merged planner before changing it -- all five holds planned a removal. The condition is now one named predicate listing every reason that would ADD the label, so a hold added later cannot silently fail to suppress the release. The counterweight is tested too: with nothing holding, the label is still released, so the one-way latch #9942 fixed does not come back. --- src/settings/agent-actions.ts | 28 +++++++++++++++++---- test/unit/agent-actions.test.ts | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) 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); + }); +});