diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 4b01d8e74a..ef3a25aa11 100644 --- a/src/settings/agent-actions.ts +++ b/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 && diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index dc09510cc3..4c0653273f 100644 --- a/test/unit/agent-actions.test.ts +++ b/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");