fix(gate): do not lift the manual-review hold while another hold is live

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)

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<AgentActionPlanInput>) =>
+    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<AgentActionPlanInput>]> = [
+    ["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);
+  });
+});