From 67f84994634dde67e29ee7d0b016ee4eda59ed72 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 23 Jun 2026 18:38:26 -0700 Subject: [PATCH] fix(gate): stop fork false-close + neutral silent-stuck (#harm-stop) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit THE dominant contributor-flood failure (robustness audit). Two disposition bugs killed/stranded good PRs: 1. FORK FALSE-CLOSE: ciState='unverified' (a fork whose Actions await maintainer approval, or unreadable check-runs) made reviewGood=false, so a CONTRIBUTOR with a passing gate was one-shot CLOSED citing 'CI could not be verified' — and re-closed every push. FIX: willClose now fires ONLY on a REAL adverse signal — a confirmed gate FAILURE, a red required CI (ciFailed), or a base CONFLICT. Unverified / not-yet-mergeable is HELD for review, never killed. (Owner/automation still never close; guarded paths still held.) 2. NEUTRAL SILENT-STUCK: a non-confirmed contributor PR with any advisory blocker gets conclusion='neutral', and the planner returned [] immediately — no label, no disposition — so the PR looked 'unreviewed forever' (the metagraphed #1551/#1554 class). FIX: only SKIPPED short-circuits; a NEUTRAL gate now FLOWS to the disposition so the PR is surfaced with a label + held (never silently undecided). Neutral is NOT auto-merged (that trust/policy decision is deferred, not bundled here). Full suite green (3589). Updated agent-actions + queue tests that asserted the old (harmful) close/silent behavior. --- src/settings/agent-actions.ts | 17 +++++++++++++---- test/unit/agent-actions.test.ts | 25 ++++++++++++++++--------- test/unit/queue.test.ts | 7 +++++-- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 45dcc0c41f..5fb755d5a9 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -170,8 +170,10 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const acting = (actionClass: AgentActionClass) => isActingAutonomyLevel(level(actionClass)); const approval = (actionClass: AgentActionClass) => autonomyRequiresApproval(level(actionClass)); - // App/infra-neutral verdicts (not evaluated yet) never drive an action. - if (input.conclusion === "neutral" || input.conclusion === "skipped") return actions; + // Only a SKIPPED gate (genuinely not evaluated) drives no action. A NEUTRAL gate (advisory-only blockers on a + // non-confirmed contributor, or eval-not-ready) is gate-NON-BLOCKING: it flows to the disposition so the PR is + // merged (clean+green) or HELD with a label — never left silently undecided. (#harm-stop neutral-silent-stuck) + if (input.conclusion === "skipped") return actions; // CI state over ALL of the PR's checks (required OR not — codecov/patch included) — reviewbot's ci_red // parity. A red CI is NEVER approved/merged and is itself a close-worthy signal (non-owner); while CI is @@ -181,6 +183,9 @@ 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; + // Only SUCCESS earns the review-good auto-merge. A NEUTRAL gate flows (no longer silently returns []) but is + // NOT auto-merged — it falls through to a HELD + labeled state for review. (Auto-merging neutral / non-confirmed + // contributor PRs is a separate trust/policy decision, deliberately NOT bundled into the harm-stop.) (#harm-stop) const gatePassing = input.conclusion === "success"; // 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 @@ -214,7 +219,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // hallucinated reject on a crucial PR must NOT auto-close a good change (the #1528 near-miss); the owner // verifies and closes/merges. The BULK (non-guarded) contributor PRs still auto-close one-shot on a bad // verdict / conflict — only the small crucial set is held. Owner/automation PRs are never closed regardless. - const willClose = !guardrailHit && isContributor && acting("close") && (!reviewGood || isConflict); + // CLOSE a contributor PR ONLY on a REAL adverse signal — a confirmed gate FAILURE, a red required CI, or a base + // CONFLICT. NEVER close merely because CI is UNVERIFIED (a fork whose Actions await approval, or unreadable + // checks) or otherwise not-yet-mergeable — those are HELD for review, not killed (#harm-stop fork-false-close). + // Owner/automation PRs are never closed (isContributor); guarded paths are held (guardrailHit). + const willClose = !guardrailHit && isContributor && acting("close") && (input.conclusion === "failure" || ciFailed || isConflict); // Linked-issue HARD-RULE close (#linked-issue-hard-rules). A DETERMINISTIC verdict about the LINKED ISSUE // (owner-assigned / missing point-label / maintainer-only) — NOT an AI verdict, so there is no hallucination // to guard against: this close fires REGARDLESS of `guardrailHit`. It still only ever closes a CONTRIBUTOR @@ -336,7 +345,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // Contributor PR that is NOT review-good (gate blockers / red / unverified CI) OR conflicts with base → // CLOSE one-shot when no hard guardrail requires manual review. Cite the concrete reasons. const closeReasons: string[] = []; - if (ciFailed || ciUnverified) closeReasons.push(ciReason); + if (ciFailed) 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}`); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 0ac0b4a46d..33b46bffbb 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -22,9 +22,14 @@ function input(overrides: Partial & { conclusion: GateChec const classes = (actions: ReturnType) => actions.map((a) => a.actionClass); describe("planAgentMaintenanceActions (#778)", () => { - it("plans nothing for a not-yet-evaluated verdict (neutral / skipped)", () => { - expect(planAgentMaintenanceActions(input({ conclusion: "neutral", autonomy: { merge: "auto", label: "auto", close: "auto" } }))).toEqual([]); + it("plans nothing for SKIPPED; a NEUTRAL verdict FLOWS (advisory non-blocking, never silently undecided)", () => { + // skipped = genuinely not evaluated → no action. expect(planAgentMaintenanceActions(input({ conclusion: "skipped", autonomy: { approve: "auto" } }))).toEqual([]); + // neutral = advisory-only blockers → NON-blocking: flows to the disposition, earns a label (clean+green here), + // and is NEVER left silently undecided or auto-closed. (#harm-stop neutral-silent-stuck) + const neutral = classes(planAgentMaintenanceActions(input({ conclusion: "neutral", autonomy: { merge: "auto", label: "auto", close: "auto" } }))); + expect(neutral).not.toEqual([]); + expect(neutral).not.toContain("close"); }); it("plans nothing when every class is at a non-acting level", () => { @@ -50,10 +55,12 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(noClose).not.toContain("request_changes"); }); - it("never emits request_changes even for an action_required verdict (merge-or-close, never block)", () => { + it("an action_required verdict is HELD — never request_changes, never closed (awaiting action ≠ failure)", () => { const plan = classes(planAgentMaintenanceActions(input({ conclusion: "action_required", autonomy: { request_changes: "auto", close: "auto", label: "auto" }, blockerTitles: [] }))); expect(plan).not.toContain("request_changes"); - expect(plan).toContain("close"); // contributor + not review-good → close + // awaiting-action (e.g. a fork's CI awaiting approval) → HELD + labeled, NOT a one-shot close. (#harm-stop) + expect(plan).not.toContain("close"); + expect(plan).toContain("label"); }); it("approves a passing verdict and never re-approves; a failing one closes (never approves, never requests changes)", () => { @@ -278,13 +285,13 @@ 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("CLOSES a contributor's gate-passing PR whose CI is UNVERIFIED (fork workflows awaiting approval → green can't be confirmed)", () => { + it("HOLDS a contributor's gate-passing PR whose CI is UNVERIFIED — NEVER closes it (fork workflows awaiting approval) (#harm-stop)", () => { 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("approve"); - expect(cls).toContain("close"); - expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_CHANGES); + expect(cls).not.toContain("merge"); // can't merge — green not confirmed + expect(cls).not.toContain("approve"); // can't approve — green not confirmed + expect(cls).not.toContain("close"); // NEVER close on unverified CI — held for review, not killed + expect(cls).toContain("label"); // labeled (held), never silently stuck }); it("NEVER closes the OWNER's unverified-CI PR — held (no blocking request_changes), left open", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ca289d5e95..0abd081fd8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1222,8 +1222,11 @@ describe("queue processors", () => { }, }); - const count = await env.DB.prepare("select count(*) as n from audit_events where event_type like 'agent.action.%'").first<{ n: number }>(); - expect(count?.n).toBe(0); + // A non-confirmed contributor (neutral/advisory gate) is no longer left SILENT — the bot may surface it with a + // label so it's visible, but it takes NO TERMINAL action (never auto-merge/close/approve a non-confirmed or + // not-review-good PR). (#harm-stop: neutral flows to held+labeled instead of an empty plan.) + const terminal = await env.DB.prepare("select count(*) as n from audit_events where event_type in ('agent.action.merge','agent.action.close','agent.action.approve')").first<{ n: number }>(); + expect(terminal?.n).toBe(0); }); it("auto-maintain (#778): skips a closed PR even on an agent-configured repo", async () => {