fix(gate): never plan an add and a remove of the manual-review label together

#10155 is flapping in production on beta.7: the label is
removed and re-added roughly every 90 seconds -- four cycles in eight minutes,
each pair a GitHub write and a subscriber notification. The audit trail shows
both operations in the SAME pass, two seconds apart.

Three post-plan transforms surface the manual-review hold -- the merge
circuit-breaker, the close circuit-breaker, and the close-audit holdout (#8831).
All three carried the identical idempotency check, which looks for an existing
ADD (`labelOp !== "remove"`) and therefore cannot see a planned REMOVE. When the
planner has already scheduled a release -- section 1b does, whenever nothing IT
knows about still wants a hold -- the transform appended an add next to that
remove, and the executor performed both.

Latent until #10116. `noManualReviewHoldWanted` used to include
`!mergeableStateUnstable`, which suppressed the release on exactly these PRs and
accidentally masked the missing case. Removing that term (correctly -- it was the
latch keeping #10098 stuck) let the release fire and exposed the contradiction.
#10116 did not create this; the label was previously a sticky latch, quieter but
strictly more broken.

The planner cannot fix it from its side. Section 1b's doc calls
noManualReviewHoldWanted "every reason that would ADD this label, in one place";
it is not, and cannot be, because these three run AFTER planning -- #8831 landed
long after that comment was written. Extending the planner's list would just be
one more thing to remember on the next transform.

So one shared withManualReviewHoldLabel() drops a planned release of the same
label before adding, and all three call sites use it. The contradiction becomes
unrepresentable where the add happens. The remove is dropped rather than the add
skipped: a transform only gets there by having just diverted a merge or a close,
so its hold is strictly newer than the release decided before that diversion.

Scoped to this label only -- dropping every label remove would silently defeat
the stale-disposition-label cleanup, which is its own mutation test.

gate.closeAuditHoldoutPct is 20 (the maximum) in the global Orb config, so about
one in five would-close PRs is eligible for the path that triggered this.

Closes #10164

Author: JSONbored

src/review/close-audit-holdout.ts

@@ -31,7 +31,7 @@
 import { DECISION_AUDIT_RUBRIC_VERSION } from "./decision-audit";
 import { recordAuditEvent } from "../db/repositories";
 import { incr } from "../selfhost/metrics";
-import { resolveAgentDispositionLabels, type AgentDispositionLabelSettings, type PlannedAgentAction } from "../settings/agent-actions";
+import { withManualReviewHoldLabel, type AgentDispositionLabelSettings, type PlannedAgentAction } from "../settings/agent-actions";
 import type { DecisionReplayHoldout } from "./decision-replay";
 import { hmacHex } from "../utils/crypto";
 import { errorMessage, nowIso } from "../utils/json";
@@ -106,21 +106,16 @@ export function holdoutEligibleClose(planned: PlannedAgentAction[]): PlannedAgen
  *  downgradeCloseToHold's conversion exactly (drop + idempotent label add, never a merge/approve). */
 export function applyCloseAuditHoldout(planned: PlannedAgentAction[], labelSettings: AgentDispositionLabelSettings = {}): PlannedAgentAction[] {
   const isEligible = (action: PlannedAgentAction): boolean => action.actionClass === "close" && action.closeKind === "heuristic" && action.requiresApproval !== true;
-  const labels = resolveAgentDispositionLabels(labelSettings);
   const next = planned.filter((action) => !isEligible(action));
-  const alreadyNeedsReview = labels.manualReview !== null && next.some((action) => action.actionClass === "label" && action.label === labels.manualReview && action.labelOp !== "remove");
-  if (labels.manualReview !== null && !alreadyNeedsReview) {
-    next.push({
-      actionClass: "label",
-      // Authorized by `close` — the class actually being diverted (#label-scoping, mirrors downgradeCloseToHold).
-      autonomyClass: "close",
-      requiresApproval: false,
-      reason: "close-audit holdout drew this PR — would-close held for human adjudication (#8831)",
-      label: labels.manualReview,
-      labelOp: "add",
-    });
-  }
-  return next;
+  // #10164: via withManualReviewHoldLabel, which also drops a planned RELEASE of this same label. This
+  // transform is where the flap was actually observed -- JSONbored/loopover#10155 cycled the label roughly
+  // every 90 seconds because the planner's release and this add both landed in one plan.
+  return withManualReviewHoldLabel(next, labelSettings, {
+    // Authorized by `close` — the class actually being diverted (#label-scoping, mirrors downgradeCloseToHold).
+    autonomyClass: "close",
+    requiresApproval: false,
+    reason: "close-audit holdout drew this PR — would-close held for human adjudication (#8831)",
+  });
 }
 
 /**

src/settings/agent-actions.ts

@@ -585,29 +585,64 @@ function guardrailHoldReason(changedPaths: string[], hardGuardrailGlobs: string[
  * PURE + idempotent: with `holdOnly` false this returns the plan UNCHANGED (byte-identical, the common path);
  * with it true and no merge planned it is also a no-op. Only ever makes the system MORE cautious.
  */
+/**
+ * Add the manual-review hold label to a plan, dropping any planned REMOVE of that same label (#10164).
+ *
+ * Three post-plan transforms surface this hold -- the merge circuit-breaker, the close circuit-breaker, and
+ * the close-audit holdout (#8831, in review/close-audit-holdout.ts). All three had the same idempotency
+ * check, and all three had the same hole in it: it looks for an existing ADD (`labelOp !== "remove"`) and
+ * therefore does not notice a planned REMOVE. When the planner has already decided to release the label --
+ * which section 1b does whenever nothing it knows about still wants a hold -- the transform appended an add
+ * NEXT TO that remove. One plan, both operations, same label.
+ *
+ * The executor performs both, so the label is removed and re-added every pass, forever. Observed on
+ * JSONbored/loopover#10155 flapping roughly every 90 seconds: four add/remove cycles in eight minutes,
+ * burning GitHub write quota and notifying subscribers each time.
+ *
+ * Section 1b cannot fix this from its side. Its doc calls `noManualReviewHoldWanted` "every reason that would
+ * ADD this label, in one place", but these three run AFTER the planner, so their reasons are not knowable
+ * there -- #8831 in particular was added long after that condition was written. Resolving the contradiction
+ * where the add happens is what makes it structural instead of a list someone must remember to extend.
+ *
+ * The remove is dropped rather than the add skipped: a transform only reaches here because it has just
+ * diverted a merge or a close, so the hold it is surfacing is strictly newer information than the release the
+ * planner decided before that diversion.
+ */
+export function withManualReviewHoldLabel(
+  planned: PlannedAgentAction[],
+  labelSettings: AgentDispositionLabelSettings,
+  action: Omit<PlannedAgentAction, "actionClass" | "label" | "labelOp">,
+): PlannedAgentAction[] {
+  const labels = resolveAgentDispositionLabels(labelSettings);
+  if (labels.manualReview === null) return planned;
+  const isThisLabel = (candidate: PlannedAgentAction): boolean =>
+    candidate.actionClass === "label" && candidate.label === labels.manualReview;
+  // Drop a planned release of the very label we are about to add -- the contradiction this exists to prevent.
+  const next = planned.filter((candidate) => !(isThisLabel(candidate) && candidate.labelOp === "remove"));
+  if (next.some((candidate) => isThisLabel(candidate) && candidate.labelOp !== "remove")) return next; // already adding it
+  next.push({ ...action, actionClass: "label", label: labels.manualReview, labelOp: "add" });
+  return next;
+}
+
 export function downgradeMergeToHold(planned: PlannedAgentAction[], holdOnly: boolean, labelSettings: AgentDispositionLabelSettings = {}): PlannedAgentAction[] {
   if (!holdOnly || !planned.some((action) => action.actionClass === "merge")) return planned;
   const labels = resolveAgentDispositionLabels(labelSettings);
   const next = planned.filter((action) => action.actionClass !== "merge");
   // The dropped merge implies the PR is review-good — re-label it for manual review (replacing a stale
   // ready-to-merge promise) so the held PR is clearly flagged for a person. Idempotent: only add when absent.
-  const alreadyNeedsReview = labels.manualReview !== null && next.some((action) => action.actionClass === "label" && action.label === labels.manualReview && action.labelOp !== "remove");
   const stagedMerge = planned.find((action) => action.actionClass === "merge");
-  if (labels.manualReview !== null && !alreadyNeedsReview) {
-    next.push({
-      actionClass: "label",
-      // Authorized by `merge` (the class actually being downgraded here), NOT `review_state_label` — mirrors
-      // the guardrail-hold label above (#label-scoping) so this hold label posts whenever merge autonomy is
-      // acting, independent of whether the repo has separately opted into the advisory review_state_label class.
-      autonomyClass: "merge",
-      requiresApproval: stagedMerge?.requiresApproval ?? false,
-      reason: "accuracy circuit-breaker engaged (merge precision dropped) — would-merge held for human review",
-      label: labels.manualReview,
-      labelOp: "add",
-    });
-  }
+  // #10164: via withManualReviewHoldLabel so a planned RELEASE of this same label is dropped rather than
+  // fought with, which is what made the label flap every pass.
+  const withHold = withManualReviewHoldLabel(next, labelSettings, {
+    // Authorized by `merge` (the class actually being downgraded here), NOT `review_state_label` — mirrors
+    // the guardrail-hold label above (#label-scoping) so this hold label posts whenever merge autonomy is
+    // acting, independent of whether the repo has separately opted into the advisory review_state_label class.
+    autonomyClass: "merge",
+    requiresApproval: stagedMerge?.requiresApproval ?? false,
+    reason: "accuracy circuit-breaker engaged (merge precision dropped) — would-merge held for human review",
+  });
   // Drop any ready-to-merge label add (the auto-merge it promised is now suppressed).
-  return next.filter((action) => !(labels.readyToMerge !== null && action.actionClass === "label" && action.label === labels.readyToMerge && action.labelOp !== "remove"));
+  return withHold.filter((action) => !(labels.readyToMerge !== null && action.actionClass === "label" && action.label === labels.readyToMerge && action.labelOp !== "remove"));
 }
 
 /**
@@ -684,7 +719,6 @@ export function downgradeCloseToHold(
     isBreakerEligibleClose(action) &&
     (noConcreteEvidenceUnderProjectBreaker(action) || (action.closeConcreteEvidence === true && everyJustifyingCodeUntrustworthy(action)));
   if (!planned.some(isDowngradableClose)) return planned;
-  const labels = resolveAgentDispositionLabels(labelSettings);
   // #9158 (label-close-split-brain, breaker-downgrade half): dropping a close here must ALSO drop any label
   // COUPLED to it -- the anti-abuse label pushed alongside a blacklist/contributor_cap/review_nag/copycat
   // close carries the SAME closeKind and is inseparable metadata on that close (see planContributorCapClose's/
@@ -704,22 +738,16 @@ export function downgradeCloseToHold(
   const next = planned.filter((action) => !isDowngradableClose(action) && !isOrphanedCoupledLabel(action));
   // The dropped close means the PR is held for a person — surface the manual-review label. Idempotent: only add when
   // absent (e.g. a guarded-but-passing plan may already carry it). NEVER adds a merge/approve.
-  const alreadyNeedsReview = labels.manualReview !== null && next.some((action) => action.actionClass === "label" && action.label === labels.manualReview && action.labelOp !== "remove");
   const droppedClose = planned.find(isDowngradableClose);
-  if (labels.manualReview !== null && !alreadyNeedsReview) {
-    next.push({
-      actionClass: "label",
-      // Authorized by `close` (the class actually being downgraded here), NOT `review_state_label` — same
-      // reasoning as downgradeMergeToHold's own manual-review label above (#label-scoping).
-      autonomyClass: "close",
-      requiresApproval: droppedClose?.requiresApproval ?? false,
-      reason: "close-precision circuit-breaker engaged — would-close held for human review",
-      label: labels.manualReview,
-      labelOp: "add",
-    });
-  }
+  // #10164: see withManualReviewHoldLabel — drops a planned release of this label instead of racing it.
   // KEEP the changes-requested label (it correctly states the PR is not mergeable) and every other action.
-  return next;
+  return withManualReviewHoldLabel(next, labelSettings, {
+    // Authorized by `close` (the class actually being downgraded here), NOT `review_state_label` — same
+    // reasoning as downgradeMergeToHold's own manual-review label above (#label-scoping).
+    autonomyClass: "close",
+    requiresApproval: droppedClose?.requiresApproval ?? false,
+    reason: "close-precision circuit-breaker engaged — would-close held for human review",
+  });
 }
 
 function closeMessage(reasons: string[]): string {

test/unit/manual-review-label-flap.test.ts

@@ -0,0 +1,103 @@
+import { describe, expect, it } from "vitest";
+
+import {
+  AGENT_LABEL_NEEDS_REVIEW,
+  downgradeMergeToHold,
+  withManualReviewHoldLabel,
+  type PlannedAgentAction,
+} from "../../src/settings/agent-actions";
+import { applyCloseAuditHoldout } from "../../src/review/close-audit-holdout";
+
+// #10164: a plan must never contain BOTH an add and a remove of the same label.
+//
+// Three post-plan transforms surface the manual-review hold -- the merge circuit-breaker, the close
+// circuit-breaker, and the close-audit holdout (#8831). All three checked idempotency by looking for an
+// existing ADD (`labelOp !== "remove"`), which by construction cannot see a planned REMOVE. So when the
+// planner had already decided to release the label (section 1b, whenever nothing it knows about still wants a
+// hold), the transform appended an add NEXT TO that remove and the executor performed both.
+//
+// Observed in production on JSONbored/loopover#10155: the label cycled roughly every 90 seconds -- four
+// add/remove pairs in eight minutes -- burning write quota and notifying subscribers on each flip. It was
+// latent until #10116 made section 1b's release actually fire for these PRs.
+//
+// The planner cannot prevent this from its side: these run AFTER planning, so their reasons are not knowable
+// to `noManualReviewHoldWanted` (whose comment nonetheless claims to list "every reason that would ADD this
+// label"). Resolving it where the add happens is what makes it structural.
+
+const releasePlanned: PlannedAgentAction = {
+  actionClass: "label",
+  autonomyClass: "merge",
+  requiresApproval: false,
+  reason: 'manual-review hold resolved — clearing the "manual-review" label the bot applied',
+  label: AGENT_LABEL_NEEDS_REVIEW,
+  labelOp: "remove",
+};
+
+const holdLabels = (plan: PlannedAgentAction[]) =>
+  plan.filter((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW);
+
+describe("manual-review label flap (#10164)", () => {
+  it("REGRESSION: the close-audit holdout drops a planned release instead of racing it", () => {
+    // The exact production shape: planner released the label, holdout diverts the close and re-adds it.
+    const planned: PlannedAgentAction[] = [
+      { actionClass: "close", requiresApproval: false, reason: "heuristic close", closeKind: "heuristic" },
+      releasePlanned,
+    ];
+    const out = applyCloseAuditHoldout(planned);
+    const labelOps = holdLabels(out);
+    expect(labelOps).toHaveLength(1);
+    expect(labelOps[0]?.labelOp).toBe("add");
+    expect(out.some((a) => a.actionClass === "close")).toBe(false);
+  });
+
+  it("REGRESSION: the merge circuit-breaker does the same", () => {
+    const planned: PlannedAgentAction[] = [
+      { actionClass: "merge", requiresApproval: false, reason: "green" },
+      releasePlanned,
+    ];
+    const labelOps = holdLabels(downgradeMergeToHold(planned, true));
+    expect(labelOps).toHaveLength(1);
+    expect(labelOps[0]?.labelOp).toBe("add");
+  });
+
+  it("INVARIANT: no transform ever emits both operations for the same label", () => {
+    // Stated over all three rather than per-transform, so a fourth added later is covered by the same rule
+    // the moment it routes through withManualReviewHoldLabel.
+    const withClose: PlannedAgentAction[] = [
+      { actionClass: "close", requiresApproval: false, reason: "heuristic close", closeKind: "heuristic" },
+      releasePlanned,
+    ];
+    const plans = [
+      applyCloseAuditHoldout(withClose),
+      downgradeMergeToHold([{ actionClass: "merge", requiresApproval: false, reason: "green" }, releasePlanned], true),
+      withManualReviewHoldLabel([releasePlanned], {}, { autonomyClass: "close", requiresApproval: false, reason: "any hold" }),
+    ];
+    for (const [i, plan] of plans.entries()) {
+      const ops = new Set(holdLabels(plan).map((a) => a.labelOp));
+      expect(ops.has("add") && ops.has("remove"), `plan ${i} contains both ops`).toBe(false);
+    }
+  });
+
+  it("stays idempotent: an add already in the plan is not duplicated", () => {
+    const alreadyAdding: PlannedAgentAction = {
+      actionClass: "label", autonomyClass: "close", requiresApproval: false,
+      reason: "already held", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add",
+    };
+    expect(holdLabels(withManualReviewHoldLabel([alreadyAdding], {}, { autonomyClass: "close", requiresApproval: false, reason: "second hold" }))).toHaveLength(1);
+  });
+
+  it("leaves OTHER labels' removes alone — only this label's release is dropped", () => {
+    // Over-broad filtering here would silently defeat the stale-disposition-label cleanup.
+    const otherRemove: PlannedAgentAction = {
+      actionClass: "label", autonomyClass: "review_state_label", requiresApproval: false,
+      reason: "stale sibling", label: "ready-to-merge", labelOp: "remove",
+    };
+    const out = withManualReviewHoldLabel([otherRemove, releasePlanned], {}, { autonomyClass: "close", requiresApproval: false, reason: "hold" });
+    expect(out).toContainEqual(otherRemove);
+  });
+
+  it("is a no-op when the repo has no manual-review label configured", () => {
+    const plan: PlannedAgentAction[] = [releasePlanned];
+    expect(withManualReviewHoldLabel(plan, { manualReviewLabel: null }, { autonomyClass: "close", requiresApproval: false, reason: "hold" })).toBe(plan);
+  });
+});