fix(review): clear stale disposition labels once the underlying hold resolves

src/settings/agent-actions.ts

@@ -767,6 +767,22 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
     : ciUnverified
       ? "CI could not be verified"
       : "";
+  // Hoisted above section 1 (#stale-disposition-label-cleanup) so the review_state_label sibling-label
+  // cleanup below can tell whether the owner/automation "not reviewGood" fallback hold (below, #1089) still
+  // wants `labels.manualReview` this same pass, even when this ternary's own choice picks a different label
+  // (e.g. ciUnverified: reviewGood is false, so the ternary below picks changesRequested, but the fallback
+  // still separately wants manualReview) — without this, the cleanup would remove a label the fallback is
+  // about to re-add later in this same pass. See its own doc comment at the (former) point of use below.
+  const manualHoldReason =
+    guardrailHit
+      ? `verdict=${conclusion}; ${guardrailReason}`
+      : ciUnverified
+        ? "CI could not be verified"
+        : conclusion === "action_required"
+          ? "review requires maintainer action"
+          : !reviewGood && !willClose && (!closeEligible || acting("close"))
+            ? `verdict=${conclusion}${ciReason ? `; ${ciReason}` : ""}`
+            : null;
 
   // 1) manual-review label — a configurable, single-purpose label for guardrail holds. This is intentionally
   // separate from review_state_label so a one-shot repo can opt into `manual-review` without also enabling the
@@ -890,6 +906,29 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
               : {}),
       });
     }
+    // Stale disposition-label cleanup (#stale-disposition-label-cleanup): the four states above
+    // (readyToMerge/manualReview/migrationCollision/changesRequested) are mutually exclusive — a PR should
+    // carry exactly one. But the ternary above only ever ADDS the current one; a label from a PRIOR pass
+    // (e.g. changesRequested while CI was red) never got removed once the PR became healthy again, so it sat
+    // on the PR forever alongside whatever the bot added next. Clear every OTHER configured sibling that is
+    // still live on the PR. `manualReview` is excluded from this pass's removal when `manualHoldReason` is
+    // non-null even though it isn't this ternary's own `label` choice (e.g. ciUnverified: reviewGood is false
+    // so the ternary picks changesRequested here, but the separate owner/automation fallback below still
+    // independently wants manualReview and may re-add it later in this SAME pass) — removing it here would
+    // race against that later add.
+    const dispositionLabelSiblings = [labels.readyToMerge, labels.manualReview, labels.migrationCollision, labels.changesRequested];
+    for (const stale of dispositionLabelSiblings) {
+      if (stale === null || stale === label || !hasLabel(input.pr.labels, stale)) continue;
+      if (stale === labels.manualReview && manualHoldReason !== null) continue;
+      actions.push({
+        actionClass: "label",
+        autonomyClass: "review_state_label",
+        requiresApproval: approval("review_state_label"),
+        reason: `disposition resolved — clearing the stale "${stale}" label`,
+        label: stale,
+        labelOp: "remove",
+      });
+    }
     // Flag-then-close double-check, Pass 1: add the pending-closure label + a warning comment citing the specific
     // rule and the verification window. The label's presence is the state that, persisting to the next pass with
     // the violation still present, triggers the close. Idempotent (the flag only fires when the label is absent).
@@ -1076,16 +1115,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
   // below) so a repo with every relevant class off stays fully quiescent — see "plans nothing when every class is
   // at a non-acting level". For the owner/admin/automation-bot branch (`!closeEligible`) this is unchanged: those
   // authors are never close-eligible regardless of the `close` autonomy dial, so the hold must still surface.
-  const manualHoldReason =
-    guardrailHit
-      ? `verdict=${conclusion}; ${guardrailReason}`
-      : ciUnverified
-        ? "CI could not be verified"
-        : conclusion === "action_required"
-          ? "review requires maintainer action"
-          : !reviewGood && !willClose && (!closeEligible || acting("close"))
-            ? `verdict=${conclusion}${ciReason ? `; ${ciReason}` : ""}`
-            : null;
+  // (manualHoldReason itself is now computed earlier, above section 1 — see its doc comment there.)
   if (
     manualHoldReason !== null &&
     labels.manualReview !== null &&

test/unit/agent-actions.test.ts

@@ -120,6 +120,66 @@ describe("planAgentMaintenanceActions (#778)", () => {
     ]);
   });
 
+  describe("stale disposition-label cleanup (#stale-disposition-label-cleanup)", () => {
+    it("clears a stale changes-requested label once the PR becomes healthy again", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, pr: { labels: [AGENT_LABEL_CHANGES] } }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_READY }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_CHANGES, labelOp: "remove" }));
+    });
+
+    it("clears a stale manual-review label once the guardrail no longer hits", () => {
+      // No guardrailHit this pass (changedPaths/hardGuardrailGlobs empty, the default) — a prior pass's
+      // manual-review hold has resolved, so it must not linger once the PR is genuinely ready-to-merge.
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, pr: { labels: [AGENT_LABEL_NEEDS_REVIEW] } }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_READY }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "remove" }));
+    });
+
+    it("clears a stale ready-to-merge label when the PR newly becomes guarded", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", review_state_label: "auto" }, changedPaths: ["src/settings/agent-actions.ts"], hardGuardrailGlobs: ["src/settings/**"], pr: { labels: [AGENT_LABEL_READY], mergeableState: "clean" } }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "add" }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_READY, labelOp: "remove" }));
+    });
+
+    it("clears every stale sibling at once when more than one lingers from prior passes", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, pr: { labels: [AGENT_LABEL_CHANGES, AGENT_LABEL_MIGRATION_COLLISION] } }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_READY }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_CHANGES, labelOp: "remove" }));
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_MIGRATION_COLLISION, labelOp: "remove" }));
+    });
+
+    it("is idempotent — no remove actions when the PR already carries only the correct label", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, pr: { labels: [AGENT_LABEL_READY] } }));
+      expect(plan.filter((a) => a.actionClass === "label" && a.labelOp === "remove")).toEqual([]);
+    });
+
+    it("never emits a remove for a sibling label that is configured OFF (null)", () => {
+      // readyToMergeLabel disabled entirely: nothing gets ADDED for the healthy verdict, but a stale
+      // changes-requested label from before must still be cleared -- disabling the "success" label must not
+      // suppress cleanup of a now-wrong one.
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, readyToMergeLabel: null, pr: { labels: [AGENT_LABEL_CHANGES] } }));
+      expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_READY)).toBe(false);
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_CHANGES, labelOp: "remove" }));
+    });
+
+    it("does NOT clear manual-review when the owner/automation CI-unverified fallback still wants it, even though this ternary picked changes-requested", () => {
+      // ciUnverified makes reviewGood false, so the review_state_label ternary picks changes-requested here --
+      // but the separate manualHoldReason fallback (ciUnverified branch) still independently wants
+      // manual-review and may re-add it later in this same pass; the cleanup must not race against that.
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, ciState: "unverified", pr: { labels: [AGENT_LABEL_NEEDS_REVIEW] } }));
+      expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove")).toBe(false);
+      expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_CHANGES }));
+    });
+
+    it("never touches the pending-closure label, which has its own dedicated clear path", () => {
+      const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { review_state_label: "auto" }, linkedIssueHardRule: { violated: false, reason: null }, pr: { labels: [AGENT_LABEL_PENDING_CLOSURE] } }));
+      expect(plan.some((a) => a.label === AGENT_LABEL_PENDING_CLOSURE && a.labelOp === "remove")).toBe(true);
+      // exactly one remove for it (from the existing dedicated clearLinkedIssueFlag path), not a duplicate from
+      // the new sibling-cleanup loop (which does not include pendingClosure in its sibling set at all).
+      expect(plan.filter((a) => a.label === AGENT_LABEL_PENDING_CLOSURE && a.labelOp === "remove")).toHaveLength(1);
+    });
+  });
+
   it("approves a passing verdict and never re-approves; a failing one closes (never approves, never requests changes)", () => {
     expect(classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" } })))).toContain("approve");
     expect(classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" }, pr: { labels: [], reviewDecision: "APPROVED" } })))).not.toContain("approve");