diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 778887f68f..d025d53f2f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1850,6 +1850,26 @@ export function applyPrecisionBreakers( return closeHoldOnly ? downgradeCloseToHold(afterMerge, true) : afterMerge; } +/** PURE: which precision-breaker directions actually rewrote the plan — i.e. `planned` had a merge/close that + * `breakerOnPlan` (the post-{@link applyPrecisionBreakers} result) no longer has. Extracted from the call site + * so the bounded-cardinality observability counter (#terminal-outcome-audit) is unit-tested directly, the same + * way applyPrecisionBreakers itself is. Returns at most one entry per direction, in a stable merge-then-close + * order; empty on the common (not-engaged, or nothing downgraded) path. */ +export function precisionBreakerDowngradeDirections(planned: PlannedAgentAction[], breakerOnPlan: PlannedAgentAction[]): Array<"merge" | "close"> { + // Reference identity, not "is the class still present anywhere in the array": downgradeMergeToHold / + // downgradeCloseToHold both filter() the input (preserving object identity for every KEPT action) and only + // ever push brand-new label actions, so a specific planned action survives iff the SAME object reference is + // still in breakerOnPlan. A coarse `!breakerOnPlan.some(actionClass === "close")` check would miss a downgrade + // when a plan carries TWO close actions and only one (the heuristic one) is dropped — the surviving + // deterministic close keeps that check from ever firing even though the breaker did rewrite the plan (gate + // review finding, round 2). + const kept = new Set(breakerOnPlan); + const directions: Array<"merge" | "close"> = []; + if (planned.some((action) => action.actionClass === "merge" && !kept.has(action))) directions.push("merge"); + if (planned.some((action) => action.actionClass === "close" && !kept.has(action))) directions.push("close"); + return directions; +} + /** * Historical compatibility helper for callers/tests that still need to know whether branch-protection contexts * were readable. The disposition planner no longer uses this to soften red CI: any visible completed red @@ -2358,6 +2378,13 @@ async function runAgentMaintenancePlanAndExecute( await isHoldOnly(env, repoFullName), await isCloseHoldOnly(env, repoFullName), ); + // Observability (#terminal-outcome-audit): a bounded-cardinality counter (direction only — no repo/PR/reason + // text) so an operator can see, at a glance, how much of the plan a breaker is currently rewriting, without + // re-deriving it from individual PR audit rows. Fires only when the breaker actually changed something — + // the common (not-engaged) path increments nothing, matching every other breaker log in this codebase. + for (const direction of precisionBreakerDowngradeDirections(planned, breakerOnPlan)) { + incr("gittensory_precision_breaker_downgrades_total", { direction }); + } if (breakerOnPlan.length === 0) return; // #2552 (gate review finding, round 2): force a fresh rebase + CI recheck when the base has advanced within diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 98538b7e9c..ce6647cfd0 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -39,6 +39,7 @@ import { maybeAutoClearHoldOnly, } from "./auto-tune"; import { computeGateEval } from "./parity"; +import { GITTENSORY_NATIVE_SOURCE } from "./parity-wire"; /** PURE: parse the PR number an "Reverts #N / Reverts owner/repo#N" body refers to (GitHub's revert PRs). * Mirrors reviewbot runtime.ts parseRevertedPrNumber. Returns undefined when the body isn't a revert. */ @@ -104,6 +105,46 @@ export async function isCloseHoldOnly( } } +/** Every project currently holding a PER-PROJECT (not `:global`) `holdonly:`/`closehold:` flag. Used ONLY to + * widen the auto-clear tick's candidate set beyond `report.rows` (#autoclear-deadlock) — the eval report only + * contains a project once it has a fresh DECIDED sample in the window, but a breaker that is suppressing every + * merge/close for a project stops that project from producing new decided samples at all, so a project with no + * OTHER (e.g. merge-side) activity can silently never reappear in `report.rows` and its stuck flag would never + * be reconsidered. `:global` is deliberately excluded here (mirrors {@link shouldAutoClear}: a human-set global + * freeze is never auto-cleared, so it must never enter an auto-clear candidate set). Fail-open (empty) on a DB + * error, matching every other flag read in this module. */ +async function listEngagedProjectScopes(env: Env): Promise<{ holdonly: string[]; closehold: string[] }> { + try { + const res = await env.DB.prepare( + "SELECT key, value FROM system_flags WHERE key LIKE 'holdonly:%' OR key LIKE 'closehold:%'", + ).all<{ key: string; value: string }>(); + const holdonly: string[] = []; + const closehold: string[] = []; + for (const row of res.results ?? []) { + if (!flagTruthy(row.value)) continue; + const [prefix, ...rest] = row.key.split(":"); + const project = rest.join(":"); + if (!project || project === "global") continue; + // The SQL WHERE clause above only ever matches a "holdonly:" or "closehold:" key, so prefix can never be + // anything else here — a plain else (not another === check) so there is no unreachable branch to cover. + // If the WHERE clause ever grows a third prefix, this must go back to an explicit `else if (prefix === + // "closehold")` (with a new branch/test for the resulting default case) so an unrecognized prefix is + // never silently miscategorized as closehold. + if (prefix === "holdonly") holdonly.push(project); + else closehold.push(project); + } + return { holdonly, closehold }; + } catch (error) { + console.warn( + JSON.stringify({ + ev: "flags_read_error", + message: errorMessage(error).slice(0, 120), + }), + ); + return { holdonly: [], closehold: [] }; + } +} + /** A live FlagStore over system_flags for the circuit-breaker (applyAutoTune / maybeAutoClearHoldOnly + * applyCloseAutoTune / maybeAutoClearCloseHoldOnly). */ export function createFlagStore(env: Env): FlagStore { @@ -448,7 +489,14 @@ const BREAKER_EVAL_WINDOW_DAYS = 90; /** * One precision-circuit-breaker tick, run on the scheduled (selftune) cron. Reads the gate-eval confusion - * matrix over gittensory's OWN recorded pr_outcome/gate_decision rows, then engages/clears BOTH breakers: + * matrix over gittensory's OWN recorded pr_outcome/gate_decision rows -- SCOPED to `source: 'gittensory-native'` + * (#autoclear-deadlock / stale-source): review_audit can also carry historical `gate_decision` rows from the + * pre-convergence reviewbot engine (source='reviewbot'), which stopped running once a repo converged and so + * never grows. Reading across ALL sources (the pre-fix behavior) let a permanently-frozen legacy prediction set + * dominate a project's measured precision forever, with no way for it to ever reflect the LIVE gate's actual + * behavior -- exactly the scenario that leaves a breaker stuck: precision can never "recover" against data that + * never changes. Scoping to the live source makes the loop honest: it judges (and can only re-engage on) what + * THIS instance's own gate has actually predicted. It then engages/clears BOTH breakers: * • MERGE: ENGAGES holdonly: for any repo whose merge precision dropped below the floor over a real * sample (applyAutoTune) — the would-MERGE → HOLD downgrade then kicks in on the next merge path; AUTO-CLEARS * an auto-engaged breaker once its cooldown elapsed AND precision recovered (maybeAutoClearHoldOnly). @@ -466,6 +514,7 @@ export async function runSelfTuneBreaker(env: Env): Promise { const report: GateEvalReport = await computeGateEval(env, { days: BREAKER_EVAL_WINDOW_DAYS, nowMs, + source: GITTENSORY_NATIVE_SOURCE, }); const flags = createFlagStore(env); const engaged = await applyAutoTune(flags, report); @@ -507,22 +556,22 @@ export async function runSelfTuneBreaker(env: Env): Promise { }), ); } - // Auto-clear any auto-engaged breaker (merge AND close) that has cooled down + recovered (one per repo in the report). - for (const row of report.rows) { - if (await maybeAutoClearHoldOnly(flags, report, row.project, nowMs)) { - console.log( - JSON.stringify({ ev: "breaker_auto_cleared", project: row.project }), - ); + // Auto-clear any auto-engaged breaker (merge AND close) that has cooled down + recovered. Candidates are the + // UNION of report.rows (projects with a fresh decided sample) and every project currently holding a + // per-project flag (#autoclear-deadlock) — a project whose breaker is suppressing 100% of its merges/closes + // stops producing new decided samples for THAT action class and can drop out of report.rows entirely, which + // would otherwise strand its flag engaged forever regardless of how long the cooldown has elapsed. + const engagedScopes = await listEngagedProjectScopes(env); + const mergeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedScopes.holdonly]); + const closeClearCandidates = new Set([...report.rows.map((row) => row.project), ...engagedScopes.closehold]); + for (const project of mergeClearCandidates) { + if (await maybeAutoClearHoldOnly(flags, report, project, nowMs)) { + console.log(JSON.stringify({ ev: "breaker_auto_cleared", project })); } - if ( - await maybeAutoClearCloseHoldOnly(flags, report, row.project, nowMs) - ) { - console.log( - JSON.stringify({ - ev: "close_breaker_auto_cleared", - project: row.project, - }), - ); + } + for (const project of closeClearCandidates) { + if (await maybeAutoClearCloseHoldOnly(flags, report, project, nowMs)) { + console.log(JSON.stringify({ ev: "close_breaker_auto_cleared", project })); } } } catch (error) { diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index a79c5252c3..f883ecb755 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -112,6 +112,7 @@ const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["gittensory_regate_ai_skipped_current_total", { help: "Regate requests skipped because AI state is current.", type: "counter" }], ["gittensory_public_surface_publish_skipped_current_total", { help: "Public surface publishes skipped because state is current.", type: "counter" }], ["gittensory_gate_decisions_total", { help: "Gate decisions by conclusion.", type: "counter" }], + ["gittensory_precision_breaker_downgrades_total", { help: "Would-merge/would-close actions downgraded to a human hold by an accuracy circuit-breaker, by breaker direction.", type: "counter" }], ["gittensory_reviews_published_total", { help: "Published review comments.", type: "counter" }], ["gittensory_github_branch_protection_permission_denied_total", { help: "GitHub branch-protection reads denied by permissions.", type: "counter" }], ["gittensory_github_pr_files_fetch_total", { help: "GitHub pull-request file fetch attempts.", type: "counter" }], diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index a2e3ae6328..29af65a231 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -676,6 +676,8 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara // Round-trip the CI dependency separately from closeKind: closeKind is intentionally broad (gate-verdict / // duplicate / slop / CI) for the close-precision breaker, but only red-CI closes need the live-CI guard. ...(action.closeRequiresCiState !== undefined ? { closeRequiresCiState: action.closeRequiresCiState } : {}), + // Round-trip the concrete-evidence tag so the breaker's exemption still applies when a staged close accepts. + ...(action.closeConcreteEvidence !== undefined ? { closeConcreteEvidence: action.closeConcreteEvidence } : {}), }; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 603972b949..0b94a81af5 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,5 +1,5 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; -import type { GateCheckConclusion } from "../rules/advisory"; +import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory"; import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy"; import { isGuardrailHit } from "../signals/change-guardrail"; import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules"; @@ -96,6 +96,17 @@ export type PlannedAgentAction = { // ALWAYS set for a heuristic close (never omitted) -- see the field's doc comment on AgentPendingActionParams // in types.ts for why the tri-state (rather than an optional "failed") matters (#2478). closeRequiresCiState?: "failed" | "not_required"; + // For a "heuristic" close: true when the close is backed by CONCRETE, non-judgment evidence — a committed + // secret, a failing/red CI run, a base conflict, a deterministic linked-issue-overlap duplicate, or a + // rule-based lane/manifest/pre-merge rejection — rather than any AI/model-derived verdict or a fuzzy score. + // The close-precision circuit-breaker (downgradeCloseToHold) EXEMPTS a concrete-evidence close: it only + // exists to catch the class of error where a heuristic call turned out to be wrong, and a committed secret + // or a red CI run is not a plausible false positive. An AI verdict — even a dual-model CONSENSUS — is + // deliberately NOT concrete: two models agreeing is still a judgment call, not deterministic evidence, and a + // systematically wrong AI-driven close is exactly the failure mode this breaker exists to catch (gate review + // finding, round 2 — an AI-only blocker must not bypass its own precision safety net). Absent/false ⇒ the + // close stays subject to the breaker like any other heuristic close (the conservative default). + closeConcreteEvidence?: boolean; expectedHeadSha?: string; // For an `approve` action: retract the bot's own prior approval instead of posting a new one — a later commit // no longer qualifies for approval, but the PR isn't merging or closing this pass, so the stale APPROVE @@ -104,6 +115,51 @@ export type PlannedAgentAction = { dismissStaleApproval?: boolean; }; +// Gate-blocker codes backed by CONCRETE, non-judgment evidence: a committed secret, a deterministic +// linked-issue-overlap duplicate, or a rule-based content/surface-lane or manifest/pre-merge rejection — every +// entry here is produced by an exact match / regex / deterministic rule, never by a model's output. NO AI- or +// model-derived code belongs in this set, no matter how the verdict was reached (including a dual-model +// CONSENSUS): the close-precision breaker exists specifically to catch a systematically-wrong AI/heuristic +// judgment, and an AI-only blocker that could bypass its own precision safety net would defeat the point (gate +// review finding, round 2 — `ai_consensus_defect` was wrongly included here and has been removed; both +// `ai_consensus_defect` and `ai_review_split` stay fully subject to the breaker, defended below by explicitly +// excluding advisory.ts's own AI_JUDGMENT_BLOCKER_CODES so this can't silently regress). Kept here (not in +// rules/advisory.ts) because "which findings are trustworthy enough to survive the breaker" is a +// disposition-planning concern, not a gate-evaluation one — the set of finding codes is itself generic +// self-host engine vocabulary (src/rules/advisory.ts), not specific to any one repository. Every entry is a +// plain string literal, deliberately NOT imported from its producer's own exported constant (even where one +// exists, e.g. advisory.ts's DUPLICATE_ONLY_BLOCKER_CODES / pre-merge-checks.ts's PRE_MERGE_CHECK_BLOCKING_CODE): +// this module sits inside a real module-load cycle +// (scoring/model.ts -> db/repositories.ts -> agent-actions.ts -> advisory.ts -> scoring/preview.ts -> +// scoring/model.ts), and spreading/reading another module's export INTO A TOP-LEVEL ARRAY LITERAL evaluates it +// eagerly at module-load time, before that module has necessarily finished initializing on this cycle's first +// pass -- confirmed by a real "X is not iterable" failure when that was tried. A plain literal has no such +// hazard. A source-text parity test in the test file below guards all nine against producer-side drift instead. +const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ + "secret_leak", + "duplicate_pr_risk", + "surface_lane_reject", + "manifest_missing_tests", + "manifest_linked_issue_required", + "pre_merge_check_required", + "lockfile_tamper_risk", + "missing_linked_issue", + "self_authored_linked_issue", +]); + +/** True when a would-CLOSE is justified by at least one piece of concrete, non-judgment evidence: red CI, a + * base conflict, a deterministic duplicate-PR link, or a gate-blocker code in {@link CONCRETE_EVIDENCE_BLOCKER_CODES}. + * Mixed blockers (one concrete + one ambiguous) still count as concrete — the concrete signal alone already + * justifies the close regardless of what else is present. Defensively excludes advisory.ts's own + * {@link AI_JUDGMENT_BLOCKER_CODES} even though none should ever land in CONCRETE_EVIDENCE_BLOCKER_CODES — a + * belt-and-suspenders guard against exactly the regression a gate review already caught once (`ai_consensus_defect` + * wrongly classified as concrete). */ +function hasConcreteCloseEvidence(input: AgentActionPlanInput, ciFailed: boolean, isConflict: boolean): boolean { + if (ciFailed || isConflict) return true; + if ((input.pr.linkedDuplicateCount ?? 0) > 0) return true; + return (input.gateBlockerCodes ?? []).some((code) => CONCRETE_EVIDENCE_BLOCKER_CODES.has(code) && !AI_JUDGMENT_BLOCKER_CODES.has(code)); +} + export type AgentActionPlanInput = { conclusion: GateCheckConclusion; blockerTitles: string[]; @@ -293,12 +349,18 @@ export function downgradeMergeToHold(planned: PlannedAgentAction[], holdOnly: bo * would be incoherent. So a plan whose only close is the deterministic one is returned UNCHANGED. A plan with a * heuristic close gets it dropped; a deterministic close present alongside is KEPT. * + * It ALSO exempts a heuristic close carrying `closeConcreteEvidence: true` (red CI, a base conflict, a + * committed secret, a deterministic duplicate, or another code in {@link CONCRETE_EVIDENCE_BLOCKER_CODES}): + * the breaker exists to catch a heuristic call that turned out to be WRONG, and concrete evidence is not the + * class of error it is watching for. A heuristic close with no concrete evidence (an unconfirmed AI verdict, + * a bare gate-verdict=failure, or a slop-score threshold) stays fully subject to the breaker. + * * The existing changes-requested label is KEPT (it correctly says the PR is not mergeable). PURE + idempotent: - * with `closeHoldOnly` false this returns the plan UNCHANGED (the common path); with no HEURISTIC close planned - * it is also a no-op. Only ever makes the system MORE cautious. + * with `closeHoldOnly` false this returns the plan UNCHANGED (the common path); with no downgradable close + * planned it is also a no-op. Only ever makes the system MORE cautious. */ export function downgradeCloseToHold(planned: PlannedAgentAction[], closeHoldOnly: boolean): PlannedAgentAction[] { - const isHeuristicClose = (action: PlannedAgentAction): boolean => action.actionClass === "close" && action.closeKind === "heuristic"; + const isHeuristicClose = (action: PlannedAgentAction): boolean => action.actionClass === "close" && action.closeKind === "heuristic" && action.closeConcreteEvidence !== true; if (!closeHoldOnly || !planned.some(isHeuristicClose)) return planned; // Drop ONLY the heuristic close(s); a deterministic linked-issue-hard-rule close (if any) is left intact. const next = planned.filter((action) => !isHeuristicClose(action)); @@ -719,14 +781,16 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne 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) closeReasons.push("the review gate is not satisfied"); - // Tagged "heuristic": a verdict-driven close (gate-verdict / duplicate / slop / CI). This is the ONLY close - // the close-precision breaker downgrades to a hold when close precision has dropped. + // Tagged "heuristic": a verdict-driven close (gate-verdict / duplicate / slop / CI). The close-precision + // breaker downgrades this to a hold when close precision has dropped — UNLESS it is also backed by concrete, + // non-judgment evidence (see closeConcreteEvidence's doc comment), in which case the breaker leaves it alone. actions.push({ actionClass: "close", requiresApproval: approval("close"), reason: closeReasons.join("; "), closeComment: closeMessage(closeReasons), closeKind: "heuristic", + closeConcreteEvidence: hasConcreteCloseEvidence(input, ciFailed, isConflict), // Pin like merge/approve (#2452): lets the accept-time supersede check detect a force-push after staging; // the executor's own step-6 live-CI re-check (#2128) separately covers the CI-driven reason above. ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), diff --git a/src/types.ts b/src/types.ts index 78be7887fb..75125ecb10 100644 --- a/src/types.ts +++ b/src/types.ts @@ -978,6 +978,12 @@ export type AgentPendingActionParams = { // ALWAYS set (to "failed" or "not_required") for a freshly planned heuristic close (#2478) -- never omitted -- // so `undefined` unambiguously means a LEGACY row staged before this field existed, not "not CI-driven". closeRequiresCiState?: "failed" | "not_required"; + // Persisted so the close-precision breaker's concrete-evidence exemption (see + // PlannedAgentAction.closeConcreteEvidence) still applies correctly when a staged heuristic close is later + // accepted -- without this, EVERY staged close would silently fall back to "not concrete" at accept-time and + // stay wrongly subject to the breaker even when it was planned from red CI, a conflict, a committed secret, + // or another concrete signal. + closeConcreteEvidence?: boolean; expectedHeadSha?: string; // For an `approve` action: retract the bot's own stale approval instead of posting a new one (see // PlannedAgentAction.dismissStaleApproval). Must round-trip through staging like every other action-specific diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index f601240d73..001697eee9 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -267,6 +267,20 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(closePullRequest).not.toHaveBeenCalled(); }); + it("REGRESSION (#hard-blockers-not-ai-judgment): closeConcreteEvidence round-trips through the persist/replay round trip so a staged concrete-evidence close still bypasses the close-precision breaker at accept-time", () => { + const concreteClose: PlannedAgentAction = { actionClass: "close", requiresApproval: true, reason: "hard blocker", closeComment: "closing", closeKind: "heuristic", closeConcreteEvidence: true }; + const persisted = actionParams(concreteClose); + expect(persisted.closeConcreteEvidence).toBe(true); + const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: concreteClose.reason }); + expect(replayed.closeConcreteEvidence).toBe(true); + }); + + it("closeConcreteEvidence is omitted from persisted params when absent on the planned action (no stray key)", () => { + const ambiguousClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "verdict failed", closeComment: "closing", closeKind: "heuristic" }; + const persisted = actionParams(ambiguousClose); + expect(persisted).not.toHaveProperty("closeConcreteEvidence"); + }); + it("LIVE non-CI heuristic close proceeds when live CI is passing because the close reason is independent of CI", async () => { const env = createTestEnv({}); // "not_required", not omitted: the planner always tags a fresh heuristic close explicitly (#2478). diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index cc253b41a7..bf986f83b2 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1,7 +1,13 @@ +import { readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; import { AGENT_LABEL_CHANGES, AGENT_LABEL_MIGRATION_COLLISION, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, DEFAULT_REVIEW_NAG_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import type { GateCheckConclusion } from "../../src/rules/advisory"; +// #module-cycle-regression: forces the SAME module-load cycle that broke once (scoring/model.ts -> +// db/repositories.ts -> agent-actions.ts -> rules/advisory.ts -> scoring/preview.ts -> scoring/model.ts) to +// actually manifest in this test file's own module graph, not just incidentally in other suites. Importing +// agent-actions.ts alone (above) never exercises the OTHER direction of the cycle -- this import does. +import { DEFAULT_ISSUE_DISCOVERY_SHARE } from "../../src/scoring/model"; function input(overrides: Partial & { conclusion: GateCheckConclusion }): AgentActionPlanInput { return { @@ -794,10 +800,13 @@ describe("downgradeMergeToHold — accuracy circuit-breaker (#self-improve / GAP }); describe("downgradeCloseToHold — close-precision circuit-breaker (#close-precision-breaker)", () => { - // A REAL heuristic would-close plan from the planner: red CI on a contributor PR → changes-requested label + - // a heuristic close. + // A REAL heuristic would-close plan from the planner, backed by NO concrete evidence: a bare gate-verdict + // failure with no red CI, no conflict, no duplicate, and no gate-blocker code the breaker trusts (see + // CONCRETE_EVIDENCE_BLOCKER_CODES) — an unconfirmed/ambiguous verdict, exactly the class of close the + // breaker exists to catch. (Deliberately NOT CI-driven: a red-CI close is concrete evidence and now EXEMPT — + // see the closeConcreteEvidence describe block below.) const heuristicClosePlan = () => - planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "failed", failingCheckNames: ["codecov/patch"], blockerTitles: ["x"], pr: { labels: [] } })); + planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "passed", blockerTitles: ["readiness score too low"], pr: { labels: [] } })); // A REAL deterministic linked-issue-hard-rule close (the exempt kind). const linkedIssueClosePlan = () => planAgentMaintenanceActions( @@ -813,8 +822,9 @@ describe("downgradeCloseToHold — close-precision circuit-breaker (#close-preci it("a real heuristic would-CLOSE plan drops the close + adds needs-human-review + KEEPS changes-requested", () => { const plan = heuristicClosePlan(); - // sanity: the planner really would heuristically close, with a changes-requested label. - expect(plan.some((a) => a.actionClass === "close" && a.closeKind === "heuristic")).toBe(true); + // sanity: the planner really would heuristically close, with a changes-requested label, and the close + // carries NO concrete evidence (so it stays subject to the breaker below). + expect(plan.some((a) => a.actionClass === "close" && a.closeKind === "heuristic" && a.closeConcreteEvidence === false)).toBe(true); expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_CHANGES)).toBe(true); const held = downgradeCloseToHold(plan, true); expect(held.some((a) => a.actionClass === "close")).toBe(false); // the would-close is downgraded... @@ -875,6 +885,136 @@ describe("downgradeCloseToHold — close-precision circuit-breaker (#close-preci }); }); +describe("closeConcreteEvidence — concrete-evidence exemption from the close-precision breaker (#hard-blockers-not-ai-judgment)", () => { + const closeOf = (plan: ReturnType) => plan.find((a) => a.actionClass === "close"); + + it("red CI (ciFailed) is concrete evidence — planned with closeConcreteEvidence: true", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "failed", failingCheckNames: ["codecov/patch"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); + }); + + it("a base conflict (isConflict) is concrete evidence even with ciState passed (the isConflict OR-arm, ciFailed false)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], mergeableState: "dirty" } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); + }); + + it("a deterministic linked-issue-overlap duplicate (linkedDuplicateCount > 0) is concrete evidence", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1 } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); + }); + + it("linkedDuplicateCount absent (nullish ?? 0) does NOT count as concrete on its own", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + }); + + it("a committed secret (secret_leak) is concrete evidence via gateBlockerCodes", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["secret_leak"], blockerTitles: ["Possible leaked secret"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); + }); + + it("a dual-model AI CONSENSUS defect (ai_consensus_defect) is deliberately NOT concrete — two models agreeing is still a judgment call, not deterministic evidence (gate review finding, round 2)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["ai_consensus_defect"], blockerTitles: ["AI review found a defect"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + }); + + it("a SPLIT AI review (ai_review_split) is also NOT concrete — the reviewers disagreed, an even more ambiguous case than consensus", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["ai_review_split"], blockerTitles: ["AI reviewers disagreed"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + }); + + it("a would-close justified ONLY by AI verdicts (consensus + split together) is still not concrete — no deterministic signal present", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["ai_consensus_defect", "ai_review_split"], blockerTitles: ["x", "y"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + }); + + it("an unrecognized/unknown gate-blocker code stays NOT concrete (fail-safe: only explicitly classified codes are trusted)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["some_future_code"], blockerTitles: ["x"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); + }); + + it("a mix of one concrete + one non-concrete blocker code is still concrete (the concrete signal alone is sufficient)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["ai_review_split", "secret_leak"], blockerTitles: ["x", "y"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); + }); + + it("the close-precision breaker EXEMPTS a concrete-evidence close even while engaged (the actual bug this fixes)", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "failed", failingCheckNames: ["ci"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true }); + const held = downgradeCloseToHold(plan, true); + // Unlike a non-concrete heuristic close, this one SURVIVES the breaker unchanged. + expect(held).toBe(plan); + expect(held.some((a) => a.actionClass === "close")).toBe(true); + expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW)).toBe(false); + }); + + // Defensive/API-contract test on downgradeCloseToHold's PREDICATE itself, not a claim about what the live + // planner emits: planAgentMaintenanceActions's disposition branch is an if/else-if chain + // (flagForLinkedIssue / willCloseForLinkedIssue / canMerge / willClose are mutually exclusive), so it can + // never plan two `close` actions in one pass — see the real single-close planner-path tests above and in + // the closeConcreteEvidence describe block below for the actual planner contract. This synthetic two-close + // input exists purely to prove downgradeCloseToHold discriminates on closeConcreteEvidence alone (not on + // closeKind or array position), the same "kept deterministic + dropped heuristic" shape that + // precisionBreakerDowngradeDirections (test/unit/precision-breakers-chain.test.ts) must also get right. + it("downgradeCloseToHold's predicate discriminates on closeConcreteEvidence alone: a non-concrete heuristic close is downgraded even alongside a KEPT concrete one", () => { + const concreteClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "hard blocker", closeKind: "heuristic", closeConcreteEvidence: true }; + const ambiguousClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "verdict failed", closeKind: "heuristic", closeConcreteEvidence: false }; + const held = downgradeCloseToHold([concreteClose, ambiguousClose], true); + expect(held.some((a) => a === concreteClose)).toBe(true); + expect(held.some((a) => a === ambiguousClose)).toBe(false); + expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW)).toBe(true); + }); +}); + +// #module-cycle-regression: agent-actions.ts imports AI_JUDGMENT_BLOCKER_CODES from rules/advisory.ts, which +// sits inside a real module-load cycle (scoring/model.ts -> db/repositories.ts -> agent-actions.ts -> +// rules/advisory.ts -> scoring/preview.ts -> scoring/model.ts) -- exactly the cycle a top-level array-literal +// spread of another module's export previously broke with a genuine "X is not iterable" failure. This test +// (combined with the scoring/model.ts import at the top of this file, which forces BOTH directions of the +// cycle into this file's own module graph) proves the import stays safe: it is only ever read inside a +// function body (hasConcreteCloseEvidence), never at module-eval time, so it resolves correctly regardless of +// which side of the cycle initializes first. +describe("module-load cycle safety (#module-cycle-regression)", () => { + it("agent-actions.ts and scoring/model.ts load together without throwing, and the AI-judgment exclusion actually works", () => { + expect(DEFAULT_ISSUE_DISCOVERY_SHARE).toBe(0.5); + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["ai_consensus_defect"], blockerTitles: ["x"], pr: { labels: [] } })); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeConcreteEvidence: false }); + }); +}); + +// #hard-blockers-not-ai-judgment parity guard (nit): CONCRETE_EVIDENCE_BLOCKER_CODES hand-types all 9 of its +// literals rather than importing any of them from their producers, even where a producer DOES export a +// reusable constant (advisory.ts's DUPLICATE_ONLY_BLOCKER_CODES, pre-merge-checks.ts's +// PRE_MERGE_CHECK_BLOCKING_CODE) -- see the doc comment on CONCRETE_EVIDENCE_BLOCKER_CODES for why: this module +// sits inside a real module-load cycle, and an eager top-level import of another module's export broke with a +// genuine "X is not iterable" failure the first time it was tried. This test reads the real producer source +// text and asserts each literal still appears there, so a future rename/removal at the producer fails this +// test immediately instead of silently turning a "concrete evidence" code into a permanently-unreachable Set +// entry. +describe("CONCRETE_EVIDENCE_BLOCKER_CODES parity — hand-typed literals still match their producers", () => { + const HAND_TYPED_CODES_AND_PRODUCERS: Array<{ code: string; file: string }> = [ + { code: "secret_leak", file: "src/review/safety.ts" }, + { code: "duplicate_pr_risk", file: "src/rules/advisory.ts" }, + { code: "surface_lane_reject", file: "src/review/content-lane-wire.ts" }, + { code: "manifest_missing_tests", file: "src/signals/focus-manifest.ts" }, + { code: "manifest_linked_issue_required", file: "src/signals/focus-manifest.ts" }, + { code: "pre_merge_check_required", file: "src/review/pre-merge-checks.ts" }, + { code: "lockfile_tamper_risk", file: "src/review/lockfile-tamper.ts" }, + { code: "missing_linked_issue", file: "src/rules/advisory.ts" }, + { code: "self_authored_linked_issue", file: "src/rules/advisory.ts" }, + ]; + + // Requires the actual producer shape (a `code: "..."` finding property, or a `SOME_CONST = "..."` exported + // code constant) immediately before the literal -- not just the bare string anywhere in the file, which a + // stale comment mentioning the code (with no real producer left) could satisfy just as easily. + const CODE_ASSIGNMENT_PATTERN = (code: string) => new RegExp(`(?:code:\\s*|=\\s*)"${code}"`); + + it.each(HAND_TYPED_CODES_AND_PRODUCERS)("$code is still produced (not merely mentioned) in its producer ($file)", ({ code, file }) => { + const source = readFileSync(file, "utf8"); + expect(source).toMatch(CODE_ASSIGNMENT_PATTERN(code)); + }); +}); + describe("contributor blacklist short-circuit (#1425)", () => { const blacklisted = (extra: Partial = {}) => // #label-scoping: the blacklist label rides on `close` autonomy, not `label` — no `label: "auto"` needed. diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 3405184565..73a33986ba 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -731,6 +731,118 @@ describe("runSelfTuneBreaker — reads recorded pr_outcome ground truth + engage await expect(runSelfTuneBreaker(env)).resolves.toBeUndefined(); warn.mockRestore(); }); + + // Seed a gate_decision/pr_outcome pair under an ARBITRARY source (e.g. the pre-convergence 'reviewbot' + // engine), independent of the gittensory-native-only seedDecisionAndOutcome helper above. + async function seedDecisionAndOutcomeForSource(env: Env, project: string, pr: number, pred: "merge" | "close", truth: "merged" | "closed", source: string): Promise { + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at) VALUES (?, ?, ?, 'gate_decision', ?, ?, ?, NULL, CURRENT_TIMESTAMP)", + ) + .bind(`gd:${source}:${project}#${pr}`, project, `${project}#${pr}`, pred, source, `sha${pr}`) + .run(); + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at) VALUES (?, ?, ?, 'pr_outcome', ?, ?, NULL, NULL, CURRENT_TIMESTAMP)", + ) + .bind(`po:${source}:${project}#${pr}`, project, `${project}#${pr}`, truth, source) + .run(); + } + + it("#autoclear-deadlock (stale-source): a FROZEN legacy 'reviewbot' close-precision failure does NOT engage the LIVE close breaker — the tick is scoped to source='gittensory-native'", async () => { + const env = createTestEnv(); + // 12 would-CLOSE predictions from the pre-convergence 'reviewbot' engine, 33% precision — would trip the + // floor if read, but this source stopped writing long ago and must not drive the LIVE self-host breaker. + for (let i = 0; i < 4; i += 1) await seedDecisionAndOutcomeForSource(env, "owner/repo", i, "close", "closed", "reviewbot"); + for (let i = 4; i < 12; i += 1) await seedDecisionAndOutcomeForSource(env, "owner/repo", i, "close", "merged", "reviewbot"); + + await runSelfTuneBreaker(env); + + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(false); + }); + + it("#autoclear-deadlock: a per-project closehold flag with NO fresh gittensory-native decided sample (report.rows empty for it) still auto-clears once the cooldown elapses", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + // Engage the CLOSE breaker directly (as the auto-tuner would have) and backdate past the 24h cooldown. + await flags.setFlag("closehold:owner/repo", true); + await env.DB.prepare("UPDATE system_flags SET updated_at = datetime('now', '-2 days') WHERE key = 'closehold:owner/repo'").run(); + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(true); + // No gittensory-native gate_decision/pr_outcome rows are seeded at all for this project — pre-fix, the + // auto-clear loop only walked report.rows and would never reconsider a project with zero decided samples, + // stranding the flag engaged forever regardless of the cooldown. + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + await runSelfTuneBreaker(env); + log.mockRestore(); + + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(false); + }); + + it("#autoclear-deadlock: does NOT auto-clear a stranded closehold flag before its cooldown has elapsed", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + await flags.setFlag("closehold:owner/repo", true); // freshly engaged (updated_at = now) — still within cooldown + await runSelfTuneBreaker(env); + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(true); + }); + + it("#autoclear-deadlock: a human-set GLOBAL closehold flag is never entered into the widened auto-clear candidate set", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + await flags.setFlag("closehold:global", true); + await env.DB.prepare("UPDATE system_flags SET updated_at = datetime('now', '-2 days') WHERE key = 'closehold:global'").run(); + await runSelfTuneBreaker(env); + // The global scope stays a human-only clear — the cooldown-elapsed widening must never touch it. + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(true); + }); + + it("#autoclear-deadlock: the merge-side holdonly flag gets the same widened-candidate auto-clear treatment", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + await flags.setFlag("holdonly:owner/repo", true); + await env.DB.prepare("UPDATE system_flags SET updated_at = datetime('now', '-2 days') WHERE key = 'holdonly:owner/repo'").run(); + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + await runSelfTuneBreaker(env); + log.mockRestore(); + expect(await isHoldOnly(env, "owner/repo")).toBe(false); + }); + + it("#autoclear-deadlock: a holdonly/closehold row with a falsy value is excluded from the widened candidate set (flagTruthy false arm)", async () => { + const env = createTestEnv(); + // A stray row exists but is NOT truthy — must not be treated as an engaged breaker (would otherwise call + // maybeAutoClear* for a project that was never really engaged). + await env.DB.prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('closehold:owner/repo', '0', CURRENT_TIMESTAMP)").run(); + await expect(runSelfTuneBreaker(env)).resolves.toBeUndefined(); + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(false); + }); + + it("#autoclear-deadlock: tolerates an all() result with no `results` array when scanning for engaged scopes (the ?? [] fallback arm)", async () => { + const env = createTestEnv(); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/SELECT key, value FROM system_flags WHERE key LIKE/i.test(sql)) { + return { all: async () => ({}) } as unknown as ReturnType; + } + return realPrepare(sql); + }) as typeof env.DB.prepare; + await expect(runSelfTuneBreaker(env)).resolves.toBeUndefined(); + }); + + it("#autoclear-deadlock: fails safe (empty candidate widening) when the engaged-scopes scan throws, without breaking the tick", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + await flags.setFlag("closehold:owner/repo", true); + await env.DB.prepare("UPDATE system_flags SET updated_at = datetime('now', '-2 days') WHERE key = 'closehold:owner/repo'").run(); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/SELECT key, value FROM system_flags WHERE key LIKE/i.test(sql)) throw new Error("d1 down"); + return realPrepare(sql); + }) as typeof env.DB.prepare; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + await expect(runSelfTuneBreaker(env)).resolves.toBeUndefined(); + warn.mockRestore(); + // The scan failed, so the widened candidate set fell back to report.rows alone (empty here) — the flag, + // with no fresh decided sample either, is correctly left engaged rather than incorrectly cleared. + expect(await isCloseHoldOnly(env, "owner/repo")).toBe(true); + }); }); // ── integration: the PR-closed webhook records pr_outcome through processJob ──────────────────────────────────── diff --git a/test/unit/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index df1f18a478..a5af5829d2 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/test/unit/precision-breakers-chain.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { applyPrecisionBreakers } from "../../src/queue/processors"; +import { applyPrecisionBreakers, precisionBreakerDowngradeDirections } from "../../src/queue/processors"; import { AGENT_LABEL_CHANGES, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, type PlannedAgentAction } from "../../src/settings/agent-actions"; // The processors chaining at maybeRunAgentMaintenance: @@ -42,3 +42,51 @@ describe("applyPrecisionBreakers — chaining the merge + close precision breake expect(out.filter((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW)).toHaveLength(1); }); }); + +describe("precisionBreakerDowngradeDirections — bounded-cardinality breaker-downgrade observability (#terminal-outcome-audit)", () => { + it("empty when neither breaker is engaged (the common, byte-identical path)", () => { + const planned = [readyLabel, mergeAction]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, false, false))).toEqual([]); + }); + + it("['merge'] when the merge breaker dropped a would-merge", () => { + const planned = [readyLabel, mergeAction]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, true, false))).toEqual(["merge"]); + }); + + it("['close'] when the close breaker dropped a heuristic would-close", () => { + const planned = [changesLabel, heuristicClose]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, false, true))).toEqual(["close"]); + }); + + it("['merge', 'close'] (stable order) when both breakers downgrade in the same pass", () => { + const planned = [readyLabel, mergeAction, changesLabel, heuristicClose]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, true, true))).toEqual(["merge", "close"]); + }); + + it("empty when closeHoldOnly is engaged but the only close present is concrete-evidence-exempt (not actually downgraded)", () => { + const concreteClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "hard blocker", closeKind: "heuristic", closeConcreteEvidence: true }; + const planned = [concreteClose]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, false, true))).toEqual([]); + }); + + it("empty when holdOnly is engaged but no merge was ever planned (nothing to downgrade)", () => { + const planned = [changesLabel]; + expect(precisionBreakerDowngradeDirections(planned, applyPrecisionBreakers(planned, true, true))).toEqual([]); + }); + + // REGRESSION (gate review finding, round 2): a plan can carry TWO close actions — a KEPT deterministic close + // (e.g. linked-issue-hard-rule, per downgradeCloseToHold's own "when BOTH a heuristic and a deterministic + // close are present, drops ONLY the heuristic one" contract) alongside a DROPPED heuristic one. A coarse + // `!breakerOnPlan.some(actionClass === "close")` check would never fire here, since the surviving + // deterministic close keeps a "close" action in breakerOnPlan even though the breaker DID rewrite the plan. + it("['close'] when a KEPT deterministic close and a DROPPED heuristic close are both present in the same plan", () => { + const deterministicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "ineligible issue", closeKind: "linked-issue-hard-rule" }; + const planned = [deterministicClose, heuristicClose]; + const breakerOnPlan = applyPrecisionBreakers(planned, false, true); + // Sanity: the deterministic close really does survive alongside the dropped heuristic one. + expect(breakerOnPlan.some((a) => a.actionClass === "close" && a.closeKind === "linked-issue-hard-rule")).toBe(true); + expect(breakerOnPlan.some((a) => a.actionClass === "close" && a.closeKind === "heuristic")).toBe(false); + expect(precisionBreakerDowngradeDirections(planned, breakerOnPlan)).toEqual(["close"]); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 7718636117..b7acd013f1 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7,6 +7,7 @@ import * as rateLimitModule from "../../src/github/rate-limit"; import * as repositoriesModule from "../../src/db/repositories"; import * as repositorySettingsModule from "../../src/settings/repository-settings"; import * as sentryModule from "../../src/selfhost/sentry"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { listCollisionEdges, createAgentRun, @@ -19505,6 +19506,75 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri expect(mergeAudit?.n).toBeGreaterThanOrEqual(1); }); + // #terminal-outcome-audit: end-to-end proof that the LIVE runAgentMaintenancePlanAndExecute call site (not just + // the extracted pure precisionBreakerDowngradeDirections/applyPrecisionBreakers unit tests) actually increments + // gittensory_precision_breaker_downgrades_total when an engaged accuracy circuit-breaker rewrites a real plan. + it("REGRESSION (#terminal-outcome-audit): an engaged holdonly breaker withholds a real would-merge AND increments the downgrade counter", async () => { + // Mirrors the "#selfhost-backlog-convergence" chain test above (same two-step CI-pending-then-green shape, + // the proven way this suite reaches a REAL merge attempt): a plain "opened" webhook with CI already green + // never reaches the merge decision in this harness; the check_suite.completed re-review path does. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_REPOS: REPO }); + await setupAutoActionRepo(env, { autonomy: { merge: "auto", approve: "auto" }, linkedIssueGateMode: "off" }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); + const seen = { closed: false, merged: false }; + let ciState: "pending" | "passed" = "pending"; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url === "https://api.github.com/graphql") return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + if (url.includes("/access_tokens")) return Response.json({ token: "test-token" }); + if (url.includes("/pulls/65/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/65/reviews")) return Response.json([]); + if (url.includes("/pulls/65/commits")) return Response.json([]); + if (url.endsWith("/pulls/65/merge") && method === "PUT") { seen.merged = true; return Response.json({ merged: true }); } + if (url.endsWith("/pulls/65")) return Response.json({ number: 65, state: "open", user: { login: "contributor" }, head: { sha: "conv65" }, mergeable_state: "clean" }); + if (url.includes("/commits/conv65/check-runs")) { + return ciState === "pending" + ? Response.json({ total_count: 1, check_runs: [{ name: "CI", status: "in_progress", conclusion: null, app: { slug: "github-actions" } }] }) + : Response.json({ total_count: 1, check_runs: [{ name: "CI", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + } + if (url.includes("/commits/conv65/status")) return Response.json({ state: ciState === "pending" ? "pending" : "success", statuses: [] }); + if (url.includes("/issues/65/labels")) return Response.json([]); + if (url.includes("/issues/65/comments")) return Response.json([]); + return Response.json({}); + }); + + // Step 1: a synchronize webhook while CI is still running — establishes the PR, no merge yet. + await processJob(env, { + type: "github-webhook", + deliveryId: "conv-holdonly-1", + eventName: "pull_request", + payload: prPayload({ number: 65, head: { sha: "conv65" }, body: "Closes #1", action: "synchronize" }), + }); + expect(seen.merged).toBe(false); + + // Engage the merge-precision breaker for this exact repo BEFORE CI resolves — mirrors how runSelfTuneBreaker + // (or a human) would set it via system_flags ahead of the next re-review. + await env.DB.prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('holdonly:JSONbored/gittensory', '1', CURRENT_TIMESTAMP)").run(); + resetMetrics(); + + // Step 2: CI finishes; a check_suite.completed webhook re-triggers the pipeline — without the breaker this + // would merge exactly like the sibling convergence-chain test above; the engaged breaker withholds it instead. + ciState = "passed"; + await processJob(env, { + type: "github-webhook", + deliveryId: "conv-holdonly-2", + eventName: "check_suite", + payload: { + action: "completed", + installation: { id: INSTALLATION_ID, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, + check_suite: { head_sha: "conv65", conclusion: "success", pull_requests: [{ number: 65 }] }, + } as never, + }); + + expect(seen.merged).toBe(false); + const mergeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.merge'").first<{ n: number }>(); + expect(mergeAudit?.n).toBe(0); + expect(await renderMetrics()).toContain('gittensory_precision_breaker_downgrades_total{direction="merge"} 1'); + }); + it("REGRESSION: closeOwnerAuthors=false (default) protects an owner-authored blocked PR from the general heuristic-close path", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await setupAutoActionRepo(env, { autonomy: { close: "auto" } }); // closeOwnerAuthors defaults false