From c09b89b02ed470683fdf48ab888f0ad756699c2b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 23 Jun 2026 08:37:22 -0700 Subject: [PATCH] feat(gate): re-align planner to canonical close-or-merge policy (minimize manual) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Match reviewbot's decideNonContentGate, tuned to the operator's minimize-manual goal: - A CONTRIBUTOR PR that is not review-good (gate blockers / red OR unverified CI) OR conflicts with base is CLOSED one-shot (taopedia model — resolve + open a fresh PR), citing the concrete reasons. Closes even on a guarded path: the guardrail withholds a GOOD PR for review, it never rescues a bad one. - A review-good PR (gate passes + CI green) on a protected path is APPROVED but HELD for the owner's manual safety review (never auto-merged). - OWNER / automation PRs are NEVER closed — held with request-changes. - request-changes only fires as a fallback when not closing (owner, or close not at an acting level). - Manual review is now the rare exception. 3425 unit tests pass. --- src/settings/agent-actions.ts | 121 ++++++++++++++++++-------------- test/unit/agent-actions.test.ts | 57 +++++++-------- 2 files changed, 91 insertions(+), 87 deletions(-) diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index c86204a947..7c3c25b72d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -86,8 +86,6 @@ export type AgentActionPlanInput = { }; }; -const isBlocking = (conclusion: GateCheckConclusion): boolean => conclusion === "failure" || conclusion === "action_required"; - function hasLabel(labels: string[], name: string): boolean { return labels.some((label) => label.toLowerCase() === name.toLowerCase()); } @@ -124,70 +122,79 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // Settle-before-decide: never approve / merge / close on a half-finished CI run. if (input.ciState === "pending") return actions; - const blocking = isBlocking(input.conclusion); const gatePassing = input.conclusion === "success"; - // A changed path matching a hard guardrail forces manual review: suppress the irreversible dispositions - // (merge / close) AND the auto-approve that could later satisfy a merge. label + request_changes still run. - // Fail SAFE on UNKNOWN paths: 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/close a PR whose diff we don't know. Repos with no guardrails stay permissive. + // A changed path matching a hard guardrail forces manual review (suppresses auto-MERGE / auto-approve). + // 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.) const guardrailHit = input.hardGuardrailGlobs.length > 0 && (input.changedPaths.length === 0 || changedPathsHittingGuardrail(input.changedPaths, input.hardGuardrailGlobs).length > 0); - // Auto-merge-ready ONLY when the gate passes AND CI is green AND no guarded path is touched. A red, pending, - // or unverified CI is never approved/merged. - const readyToMerge = gatePassing && ciPassed && !guardrailHit; - const ciReason = ciFailed ? `CI is failing${failingCheckNames.length ? ` (${failingCheckNames.join(", ")})` : ""}` : ""; - - // 1) label — a blocking gate OR a red CI → changes-requested. A gate-passing PR that is not yet - // auto-mergeable (guarded path, or CI not green/unverified) → needs-human-review (labeling it - // `ready-to-merge` would promise an auto-merge that never happens). Only a gate-passing, CI-green, - // non-guarded PR gets `ready-to-merge`. Idempotent: skip if the PR already carries the chosen label. + // 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. + const ciUnverified = input.ciState === "unverified"; + const reviewGood = gatePassing && ciPassed; + const isContributor = !input.authorIsOwner && !input.authorIsAutomationBot; + const mergeableClean = input.pr.mergeableState === "clean"; + const isConflict = input.pr.mergeableState === "dirty"; // conflicts with base — can't merge as-is + // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan + // 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); + const ciReason = ciFailed + ? `CI is failing${failingCheckNames.length ? ` (${failingCheckNames.join(", ")})` : ""}` + : ciUnverified + ? "CI could not be verified" + : ""; + + // 1) label — ready-to-merge (review-good, unguarded) / needs-human-review (review-good but guarded) / + // changes-requested (not review-good → will be closed for a contributor, held for the owner). Idempotent. if (acting("label")) { - const label = blocking || ciFailed ? AGENT_LABEL_CHANGES : readyToMerge ? AGENT_LABEL_READY : AGENT_LABEL_NEEDS_REVIEW; - const reason = ciFailed - ? `verdict=${input.conclusion}; ${ciReason}` - : !blocking && guardrailHit - ? `verdict=${input.conclusion}; guarded path forces human review` - : !blocking && !ciPassed - ? `verdict=${input.conclusion}; CI not green yet — held for human` - : `verdict=${input.conclusion}`; + const label = !reviewGood ? AGENT_LABEL_CHANGES : guardrailHit ? AGENT_LABEL_NEEDS_REVIEW : AGENT_LABEL_READY; + const reason = !reviewGood + ? `verdict=${input.conclusion}${ciReason ? `; ${ciReason}` : ""}` + : guardrailHit + ? `verdict=${input.conclusion}; guarded path → owner safety review` + : `verdict=${input.conclusion}; CI green`; if (!hasLabel(input.pr.labels, label)) { actions.push({ actionClass: "label", requiresApproval: approval("label"), reason, label }); } } - // 2) review — approve XOR request-changes, never re-post the same state. A red CI forces request-changes - // (citing the failing checks) and is NEVER approved; approve fires only when the gate passes AND CI is green - // AND no guarded path is touched. - if ((blocking || ciFailed) && acting("request_changes") && input.pr.reviewDecision !== "CHANGES_REQUESTED") { - const lines = ciFailed ? [ciReason, ...input.blockerTitles] : [...input.blockerTitles]; - const summary = lines.length ? lines.map((line) => `- ${line}`).join("\n") : "- The Gittensory Gate is not satisfied."; - const reason = ciFailed ? `CI failing${input.blockerTitles.length ? ` + ${input.blockerTitles.length} blocker(s)` : ""}` : `${input.blockerTitles.length || 1} blocker(s)`; - actions.push({ - actionClass: "request_changes", - requiresApproval: approval("request_changes"), - reason, - reviewBody: `Gittensory requests changes — ${ciFailed ? "CI is not green" : "the gate is not yet satisfied"}:\n\n${summary}`, - }); - } else if (readyToMerge && acting("approve") && input.pr.reviewDecision !== "APPROVED") { + // 2) review — APPROVE a review-good PR (even on a guarded path: it's correct; the owner just merges it). A + // not-good OWNER/automation PR is HELD with request-changes (the maintainer sees what to fix). A not-good + // CONTRIBUTOR PR is CLOSED below and the close comment carries the reasoning — no redundant request-changes. + if (reviewGood && acting("approve") && input.pr.reviewDecision !== "APPROVED") { actions.push({ actionClass: "approve", requiresApproval: approval("approve"), - reason: "gate passed, CI green", + reason: wouldMergeButGuarded ? "gate passed, CI green (held for owner — guarded path)" : "gate passed, CI green", reviewBody: "Gittensory approves — the gate is satisfied and CI is green.", }); + } else if (!reviewGood && !willClose && acting("request_changes") && input.pr.reviewDecision !== "CHANGES_REQUESTED") { + const lines = [ciReason, ...input.blockerTitles].filter(Boolean); + const summary = lines.length ? lines.map((line) => `- ${line}`).join("\n") : "- The Gittensory Gate is not satisfied."; + actions.push({ + actionClass: "request_changes", + requiresApproval: approval("request_changes"), + reason: ciFailed ? `CI failing${input.blockerTitles.length ? ` + ${input.blockerTitles.length} blocker(s)` : ""}` : `${input.blockerTitles.length || 1} blocker(s)`, + reviewBody: `Gittensory requests changes — ${ciFailed ? "CI is not green" : ciUnverified ? "CI could not be verified" : "the gate is not yet satisfied"}:\n\n${summary}`, + }); } - // 3) disposition — merge a clean, approved, CI-green PR; otherwise close clear noise OR a red-CI PR (citing - // the failing checks). Owner + maintainer-automation PRs are NEVER closed (a red-CI owner PR is held via the - // request_changes above, left open for the maintainer). Mutually exclusive with merge. - const mergeableClean = input.pr.mergeableState === "clean"; - // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan - // the merge; it can't complete for this commit. A new commit makes the live head differ from mergeBlockedSha, - // so this only suppresses the genuinely-stuck merge — the PR falls through to needs-human-review. - const mergeTerminallyBlocked = input.pr.mergeBlockedSha != null && input.pr.headSha != null && input.pr.mergeBlockedSha === input.pr.headSha; - const canMerge = readyToMerge && acting("merge") && mergeableClean && approvalsSatisfied && !mergeTerminallyBlocked; + // 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. if (canMerge) { actions.push({ actionClass: "merge", @@ -195,15 +202,21 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne reason: `gate passed, CI green, mergeable, ${autoMaintain.requireApprovals} approval(s) satisfied`, mergeMethod: autoMaintain.mergeMethod, }); - } else if (acting("close") && (ciFailed || !gatePassing) && !guardrailHit && !input.authorIsOwner && !input.authorIsAutomationBot) { + } 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. const closeReasons: string[] = []; - if (ciFailed) closeReasons.push(ciReason); + if (ciFailed || ciUnverified) closeReasons.push(ciReason); + if (isConflict) closeReasons.push("conflicts with the base branch — resolve and open a fresh PR"); + for (const blockerTitle of input.blockerTitles) closeReasons.push(blockerTitle); if (input.pr.slopRisk != null && input.pr.slopRisk >= slopGateMinScore) closeReasons.push(`slop score ${input.pr.slopRisk} ≥ ${slopGateMinScore}`); if ((input.pr.linkedDuplicateCount ?? 0) > 0) closeReasons.push("duplicate of another open PR"); - if (closeReasons.length > 0) { - actions.push({ actionClass: "close", requiresApproval: approval("close"), reason: closeReasons.join("; "), closeComment: closeMessage(closeReasons) }); - } + 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 + // (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 b5626b23ea..d7a27a35a6 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -81,18 +81,19 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(classes(planAgentMaintenanceActions({ conclusion: "success", blockerTitles: [], autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], mergeableState: "clean" } }))).not.toContain("merge"); // no slopGateMinScore → defaults to 60 → slopRisk 70 counts as noise and closes expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 70 } }))).toContain("close"); - // ...and slopRisk 50 is below the default → no close - expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 50 } }))).not.toContain("close"); + // ...and slopRisk 50 (below the slop default) STILL closes — a failing-gate contributor PR is closed one-shot + // regardless of slop; the slop score only adds a close reason (minimize-manual: merge-or-close). + expect(classes(planAgentMaintenanceActions({ conclusion: "failure", blockerTitles: ["x"], autonomy: { close: "auto" }, changedPaths: [], hardGuardrailGlobs: [], authorIsOwner: false, authorIsAutomationBot: false, ciState: "passed", pr: { labels: [], slopRisk: 50 } }))).toContain("close"); }); - it("closes clear noise (high slop or duplicate) on a non-passing verdict, and never closes a passing PR", () => { - // high slop + it("closes any non-passing contributor PR (citing noise when present), and never closes a passing PR", () => { + // high slop — closes, slop cited expect(classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], slopGateMinScore: 60, pr: { labels: [], slopRisk: 80 } })))).toContain("close"); - // duplicate + // duplicate — closes, duplicate cited expect(classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], pr: { labels: [], linkedDuplicateCount: 2 } })))).toContain("close"); - // no noise → no close - expect(classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], pr: { labels: [], slopRisk: 10 } })))).not.toContain("close"); - // passing verdict is never closed even with noise present + // no slop/duplicate noise → STILL closes (the gate failure alone is enough — minimize-manual: merge-or-close) + expect(classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], pr: { labels: [], slopRisk: 10 } })))).toContain("close"); + // a review-good (passing + CI green) PR is NEVER closed, even with high slop present expect(classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto" }, pr: { labels: [], slopRisk: 90 } })))).not.toContain("close"); }); @@ -126,14 +127,15 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(plan).not.toContain("merge"); }); - it("does NOT auto-close a noisy failing PR that touches a guarded path", () => { + it("DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)", () => { 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 auto-approve a passing PR that touches a guarded path (so it can't later satisfy a merge)", () => { - const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" }, ...guarded, pr: { labels: [] } }))); - expect(plan).not.toContain("approve"); + it("APPROVES a passing PR on a guarded path (pre-cleared for the owner) but never auto-merges it", () => { + 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("merge"); }); it("still labels a guarded PR (the reversible action is unaffected — it just falls to a human)", () => { @@ -169,23 +171,6 @@ describe("planAgentMaintenanceActions (#778)", () => { // A clean, non-guarded passing PR keeps the `ready-to-merge` label (the auto-merge it promises happens). expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_READY); }); - - it("fails SAFE on UNKNOWN paths: with guardrails configured but no changed-file set, suppress merge/close/approve", () => { - // The changed-file cache can be empty (fresh PR before backfill) or stale; we cannot prove the PR - // doesn't touch a guarded path, so it must fall through to a human (label/request_changes still run). - const merge = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto", label: "auto" }, changedPaths: [], hardGuardrailGlobs: ["src/scoring/**", "scripts/**"], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); - expect(merge).not.toContain("merge"); - expect(merge).toContain("label"); - const close = classes(planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, blockerTitles: ["x"], changedPaths: [], hardGuardrailGlobs: ["src/scoring/**"], pr: { labels: [], slopRisk: 95 } }))); - expect(close).not.toContain("close"); - const approve = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { approve: "auto" }, changedPaths: [], hardGuardrailGlobs: ["src/scoring/**"], pr: { labels: [] } }))); - expect(approve).not.toContain("approve"); - }); - - it("stays permissive on empty paths when the repo has NO guardrails configured (opted out)", () => { - const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { merge: "auto" }, changedPaths: [], hardGuardrailGlobs: [], pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))); - expect(plan).toContain("merge"); - }); }); describe("owner-PR guard: never auto-close the repo owner's own PRs", () => { @@ -252,13 +237,19 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto", approve: "auto", merge: "auto", close: "auto" }, ciState: "pending", pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } }))).toEqual([]); }); - it("HOLDS (needs-human-review, no merge/close/approve) a gate-passing PR whose CI is unverified", () => { + it("CLOSES a contributor's gate-passing PR whose CI is UNVERIFIED (fork workflows awaiting approval → green can't be confirmed)", () => { const plan = planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { label: "auto", approve: "auto", merge: "auto", close: "auto" }, ciState: "unverified", pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" } })); const cls = classes(plan); expect(cls).not.toContain("merge"); - expect(cls).not.toContain("close"); expect(cls).not.toContain("approve"); - expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_NEEDS_REVIEW); + expect(cls).toContain("close"); + expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_CHANGES); + }); + + it("NEVER closes the OWNER's unverified-CI PR — held, left open", () => { + const plan = classes(planAgentMaintenanceActions(input({ conclusion: "success", autonomy: { close: "auto", request_changes: "auto" }, ciState: "unverified", authorIsOwner: true, pr: { labels: [] } }))); + expect(plan).not.toContain("close"); + expect(plan).toContain("request_changes"); }); it("merges the same clean+approved PR on green CI but NOT on red CI", () => {