diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index cef3a4c764..919068e204 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -145,12 +145,13 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // the merge; it can't complete for this commit. A new commit makes the live head differ from mergeBlockedSha. const mergeTerminallyBlocked = input.pr.mergeBlockedSha != null && input.pr.headSha != null && input.pr.mergeBlockedSha === input.pr.headSha; const canMerge = reviewGood && !guardrailHit && acting("merge") && mergeableClean && approvalsSatisfied && !mergeTerminallyBlocked; - // A good PR on a guarded path → held for the owner's manual safety review (NOT auto-approved, auto-merged, or auto-closed). - // A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts, unless a hard guardrail - // requires manual review. request-changes is then redundant (the close comment carries the reasoning); it only - // fires as a FALLBACK when we are NOT closing — a guarded PR, an owner/automation PR (never closed), or a repo - // where `close` isn't at an acting autonomy level. - const willClose = !guardrailHit && isContributor && acting("close") && (!reviewGood || isConflict); + // A GOOD PR on a guarded path → held for the owner's manual safety review (NOT auto-approved or auto-merged). + // But a CONTRIBUTOR PR that is NOT review-good (gate blockers / red / unverified CI) OR conflicts is CLOSED + // one-shot REGARDLESS of the guardrail. Spec: "guarded + would-merge → hold; otherwise → closure." The + // guardrail exists to stop auto-MERGING/APPROVING crucial-path changes without owner review (see canMerge / + // approve) — it must NOT keep a rejected PR open: closing rejects bad changes and merges nothing, so it is + // always safe. Owner/automation PRs are still never closed (isContributor gates that). (#close-bad-guarded) + const willClose = isContributor && acting("close") && (!reviewGood || isConflict); const ciReason = ciFailed ? `CI is failing${failingCheckNames.length ? ` (${failingCheckNames.join(", ")})` : ""}` : ciUnverified diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 6076e2afff..b3ab4e8678 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -130,9 +130,11 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(plan).not.toContain("merge"); }); - it("does NOT auto-close a failing PR on a guarded path", () => { + it("DOES auto-close a failing contributor PR on a guarded path (the guard blocks auto-merge, NOT rejection)", () => { + // Spec: guarded + would-merge → hold; otherwise → closure. Closing a bad PR merges nothing, so the + // hard-guardrail (which exists to stop auto-MERGING crucial paths) must not keep a rejected PR open. const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], ...guarded, pr: { labels: [], slopRisk: 95 } }))); - expect(plan).not.toContain("close"); + expect(plan).toContain("close"); }); it("does NOT approve or auto-merge a passing PR on a guarded path", () => {