diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 5c5962b125..cef3a4c764 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -123,19 +123,19 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne if (input.ciState === "pending") return actions; const gatePassing = input.conclusion === "success"; - // A changed path matching a hard guardrail forces manual review (suppresses auto-MERGE / auto-approve). + // A changed path matching a hard guardrail forces manual review (suppresses auto-MERGE / auto-approve / auto-close). // Fail SAFE on UNKNOWN paths (#1062): when guardrails are configured but the changed-file set is empty (cache // not yet / no longer populated), we cannot prove the PR doesn't touch a guarded path, so treat it as a hit — - // never auto-merge a PR whose diff we don't know. Repos with no guardrails configured stay permissive. (The - // gate verdict + CI — reviewGood — is computed from the real diff upstream, so a genuinely bad PR still closes.) + // never auto-merge, auto-approve, or auto-close a PR whose diff we don't know. Repos with no guardrails + // configured stay permissive. const guardrailHit = input.hardGuardrailGlobs.length > 0 && (input.changedPaths.length === 0 || changedPathsHittingGuardrail(input.changedPaths, input.hardGuardrailGlobs).length > 0); // Canonical (reviewbot non-content-gate) policy, tuned to the operator's minimize-manual goal: merge-or-close // with high accuracy; manual review is the RARE exception. A PR is "review-good" when the gate passes AND CI is // green — that's the only thing that earns an auto-merge or an approve. Everything else, for a CONTRIBUTOR, is a - // one-shot CLOSE (taopedia model: resolve + open a fresh PR). The guardrail is handled SEPARATELY: it converts a - // would-MERGE into a manual hold (owner safety review), but it NEVER rescues a red/blocked PR from closure. + // one-shot CLOSE (taopedia model: resolve + open a fresh PR). The guardrail is handled SEPARATELY: it converts + // every would-approve/would-merge/would-close disposition into a manual hold (owner safety review). const ciUnverified = input.ciState === "unverified"; const reviewGood = gatePassing && ciPassed; const isContributor = !input.authorIsOwner && !input.authorIsAutomationBot; @@ -145,12 +145,12 @@ 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-merged, NOT closed). - const wouldMergeButGuarded = reviewGood && mergeableClean && guardrailHit; - // A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts. request-changes is then - // redundant (the close comment carries the reasoning); it only fires as a FALLBACK when we are NOT closing — - // an owner/automation PR (never closed) or a repo where `close` isn't at an acting autonomy level. - const willClose = isContributor && acting("close") && (!reviewGood || isConflict); + // 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); const ciReason = ciFailed ? `CI is failing${failingCheckNames.length ? ` (${failingCheckNames.join(", ")})` : ""}` : ciUnverified @@ -171,24 +171,25 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne } } - // 2) review — APPROVE a review-good PR (even on a guarded path: it's correct; the owner just merges it). The - // bot NEVER posts a formal CHANGES_REQUESTED review: a blocking review counts against required approvals and - // STRANDS a PR when it later goes green (a stale request-changes keeps it un-mergeable forever). A not-good - // CONTRIBUTOR PR is CLOSED below; a not-good OWNER/automation PR is HELD via the needs-human label + the - // (non-blocking) unified review comment — never a formal request-changes. (#no-request-changes) Either - // merge/approve, or close, with the rare manual hold left open + commented, never blocked. - if (reviewGood && acting("approve") && input.pr.reviewDecision !== "APPROVED") { + // 2) review — APPROVE a review-good PR only when it is NOT on a guarded path; a guarded PR falls through to the + // owner's manual safety review (never auto-approved). The bot NEVER posts a formal CHANGES_REQUESTED review: a + // blocking review counts against required approvals and STRANDS a PR when it later goes green (a stale + // request-changes keeps it un-mergeable forever). A not-good CONTRIBUTOR PR is CLOSED below; a not-good + // OWNER/automation PR is HELD via the needs-human label + the (non-blocking) unified review comment — never a + // formal request-changes. (#no-request-changes) Either merge/approve, or close, with the rare manual hold left + // open + commented, never blocked. + if (reviewGood && !guardrailHit && acting("approve") && input.pr.reviewDecision !== "APPROVED") { actions.push({ actionClass: "approve", requiresApproval: approval("approve"), - reason: wouldMergeButGuarded ? "gate passed, CI green (held for owner — guarded path)" : "gate passed, CI green", + reason: "gate passed, CI green", reviewBody: "Gittensory approves — the gate is satisfied and CI is green.", }); } // 3) disposition — MERGE (review-good, unguarded, mergeable, approvals) / CLOSE (not-good OR conflicting - // CONTRIBUTOR PR, one-shot) / MANUAL (review-good-but-guarded, or any not-good OWNER/automation PR — held, - // never closed). Mutually exclusive. + // CONTRIBUTOR PR, one-shot) / MANUAL (guarded, or any not-good OWNER/automation PR — held, never closed). + // Mutually exclusive. if (canMerge) { actions.push({ actionClass: "merge", @@ -198,8 +199,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne }); } else if (willClose) { // Contributor PR that is NOT review-good (gate blockers / red / unverified CI) OR conflicts with base → - // CLOSE one-shot. Closes EVEN on a guarded path: a guardrail withholds a GOOD PR for review, it never - // rescues a bad/red/conflicting one. Cite the concrete reasons. + // CLOSE one-shot when no hard guardrail requires manual review. Cite the concrete reasons. const closeReasons: string[] = []; if (ciFailed || ciUnverified) closeReasons.push(ciReason); if (isConflict) closeReasons.push("conflicts with the base branch — resolve and open a fresh PR"); @@ -209,7 +209,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne if (closeReasons.length === 0) closeReasons.push("the review gate is not satisfied"); actions.push({ actionClass: "close", requiresApproval: approval("close"), reason: closeReasons.join("; "), closeComment: closeMessage(closeReasons) }); } - // else: review-good-but-guarded → manual (approved + needs-human label above); not-good OWNER/automation → held + // else: guarded → manual (needs-human/changes label above); not-good OWNER/automation → held // (request-changes above); review-good-but-not-yet-mergeable → held briefly (rebase/approve resolves it next pass). return actions; diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 1796fc66ff..6076e2afff 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -130,14 +130,14 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(plan).not.toContain("merge"); }); - it("DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)", () => { + it("does NOT auto-close a failing PR on a guarded path", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], ...guarded, pr: { labels: [], slopRisk: 95 } }))); - expect(plan).toContain("close"); + expect(plan).not.toContain("close"); }); - it("APPROVES a passing PR on a guarded path (pre-cleared for the owner) but never auto-merges it", () => { + it("does NOT approve or auto-merge a passing PR on a guarded path", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto", merge: "auto" }, ...guarded, pr: { labels: [], mergeableState: "clean" } }))); - expect(plan).toContain("approve"); + expect(plan).not.toContain("approve"); expect(plan).not.toContain("merge"); });