diff --git a/src/review/close-audit-holdout.ts b/src/review/close-audit-holdout.ts index 8c2ed9b0f..cffbaa853 100644 --- a/src/review/close-audit-holdout.ts +++ b/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)", + }); } /** diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 7f156507d..ca9fad11d 100644 --- a/src/settings/agent-actions.ts +++ b/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[] { + 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 { diff --git a/test/unit/manual-review-label-flap.test.ts b/test/unit/manual-review-label-flap.test.ts new file mode 100644 index 000000000..edbb9a5bc --- /dev/null +++ b/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); + }); +});