From 320c6b511a1117b112296eabe3cb9600b7639491 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:29:21 -0700 Subject: [PATCH 1/7] fix(agent-actions): close hard-blocked contributor PRs in auto mode VPS evidence (system_flags) showed closehold: engaged for all three review-active repos since 2026-06-28, silently downgrading every heuristic close to a human hold regardless of autonomy.close=auto. Three compounding, generic self-host engine bugs: - The close-precision circuit breaker treated every heuristic close identically, including ones backed by concrete, non-judgment evidence (a committed secret, red CI, a base conflict, a deterministic linked-issue duplicate, a dual-model AI consensus defect). Added closeConcreteEvidence, round-tripped through the approval-queue staging path, and scoped downgradeCloseToHold to exempt it - a hard blocker now closes even while the breaker is engaged, matching every other deterministic close kind. - The self-tune breaker's auto-clear tick only reconsidered projects present in the current gate-eval report, but a project whose closes are 100% suppressed stops producing new decided samples for that action class and can drop out of the report entirely - stranding its flag engaged forever regardless of cooldown. Widened the auto-clear candidate set to the union of the eval report and every currently-engaged per-project flag. - The eval read across every review_audit source, including the frozen, no-longer-written 'reviewbot' source from before the gittensory-native convergence. A dead system's historical close predictions could permanently anchor a live repo's measured close precision. Scoped the tick to source='gittensory-native'. Also adds a bounded-cardinality gittensory_precision_breaker_downgrades_total counter so an engaged breaker is visible without querying review_audit directly. --- src/queue/processors.ts | 23 +++++ src/review/outcomes-wire.ts | 76 +++++++++++--- src/selfhost/metrics.ts | 1 + src/services/agent-action-executor.ts | 2 + src/settings/agent-actions.ts | 57 ++++++++++- src/types.ts | 6 ++ test/unit/agent-action-executor.test.ts | 14 +++ test/unit/agent-actions.test.ts | 85 +++++++++++++++- test/unit/outcomes-wire.test.ts | 112 +++++++++++++++++++++ test/unit/precision-breakers-chain.test.ts | 35 ++++++- 10 files changed, 384 insertions(+), 27 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 778887f68f..0563221df8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1850,6 +1850,22 @@ 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"> { + const directions: Array<"merge" | "close"> = []; + if (planned.some((action) => action.actionClass === "merge") && !breakerOnPlan.some((action) => action.actionClass === "merge")) { + directions.push("merge"); + } + if (planned.some((action) => action.actionClass === "close") && !breakerOnPlan.some((action) => action.actionClass === "close")) { + 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 +2374,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..7b6aa9885a 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,41 @@ 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; + if (prefix === "holdonly") holdonly.push(project); + else if (prefix === "closehold") 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 +484,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 +509,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 +551,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..aecb4c0d82 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -96,6 +96,14 @@ 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 a single fuzzy score or an unconfirmed AI + // verdict. 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. 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 +112,37 @@ export type PlannedAgentAction = { dismissStaleApproval?: boolean; }; +// Gate-blocker codes backed by CONCRETE, non-judgment evidence: a committed secret, a deterministic +// linked-issue-overlap duplicate, a rule-based content/surface-lane or manifest/pre-merge rejection, or a +// dual-model AI CONSENSUS (both independent reviewers agree) — as opposed to a single fuzzy score or an +// unconfirmed/ambiguous verdict. `ai_review_split` is deliberately excluded: the two reviewers DISAGREED, +// which is exactly the ambiguous case the close-precision breaker exists to catch. 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. +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", + "ai_consensus_defect", +]); + +/** 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. */ +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)); +} + export type AgentActionPlanInput = { conclusion: GateCheckConclusion; blockerTitles: string[]; @@ -293,12 +332,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 +764,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..d5f71d9d32 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: "leaked secret", 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..12c33f9d3d 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -794,10 +794,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 +816,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 +879,77 @@ 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 concrete evidence — both reviewers independently agreed", () => { + 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: true }); + }); + + it("a SPLIT AI review (ai_review_split) is deliberately NOT concrete — the reviewers disagreed, stays ambiguous", () => { + 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("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); + }); + + it("downgradeCloseToHold still downgrades a non-concrete heuristic close alongside a KEPT concrete one", () => { + const concreteClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "secret leaked", closeKind: "heuristic", closeConcreteEvidence: true }; + const ambiguousClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "verdict failed", closeKind: "heuristic", closeConcreteEvidence: false }; + const held = downgradeCloseToHold([concreteClose, ambiguousClose], true); + // Both are `closeKind: "heuristic"`, but only the non-concrete one gets swept (+ its replacement label) — + // this can't happen in a real plan (the planner emits at most one close), but proves the predicate + // discriminates on closeConcreteEvidence alone, not on closeKind or array position. + 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); + }); +}); + 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..37238ee150 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,36 @@ 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: "leaked secret", 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([]); + }); +}); From 7ffe49d3fb54553ad3b0764da2245df498cf45e5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:41:00 -0700 Subject: [PATCH 2/7] fix(agent-actions): remove AI-judgment exemption and fix downgrade-metric miscount Gate review (round 2) found two reachable defects in the prior commit: - ai_consensus_defect was wrongly classified as closeConcreteEvidence, letting a green PR with only an AI-derived blocker bypass the close-precision breaker it exists to check. Two independent models agreeing is still a judgment call, not deterministic evidence - removed it from CONCRETE_EVIDENCE_BLOCKER_CODES, and hardened hasConcreteCloseEvidence to also exclude anything in advisory.ts's own AI_JUDGMENT_BLOCKER_CODES so this can't silently regress again. - precisionBreakerDowngradeDirections detected a close downgrade by checking whether any close action remained in the post-breaker plan, so a plan with a kept deterministic close plus a dropped heuristic close recorded no downgrade. Switched to reference-identity survival checks (does this specific planned action still exist in the breaker's output), which correctly handles multiple close actions. Also reuses advisory.ts's/pre-merge-checks.ts's existing exported code constants for two of the nine concrete-evidence codes instead of retyping them, and adds a source-text parity test guarding the remaining seven against silent producer-side drift. --- src/queue/processors.ts | 16 ++++--- src/settings/agent-actions.ts | 47 +++++++++++++------- test/unit/agent-actions.test.ts | 50 +++++++++++++++++++--- test/unit/precision-breakers-chain.test.ts | 15 +++++++ 4 files changed, 99 insertions(+), 29 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0563221df8..d025d53f2f 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1856,13 +1856,17 @@ export function applyPrecisionBreakers( * 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") && !breakerOnPlan.some((action) => action.actionClass === "merge")) { - directions.push("merge"); - } - if (planned.some((action) => action.actionClass === "close") && !breakerOnPlan.some((action) => action.actionClass === "close")) { - directions.push("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; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index aecb4c0d82..3b9ea5cff2 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,5 +1,6 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; -import type { GateCheckConclusion } from "../rules/advisory"; +import { AI_JUDGMENT_BLOCKER_CODES, DUPLICATE_ONLY_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory"; +import { PRE_MERGE_CHECK_BLOCKING_CODE } from "../review/pre-merge-checks"; 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"; @@ -98,11 +99,14 @@ export type PlannedAgentAction = { 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 a single fuzzy score or an unconfirmed AI - // verdict. 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. Absent/false ⇒ the close stays subject to the - // breaker like any other heuristic close (the conservative default). + // 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 @@ -113,34 +117,45 @@ export type PlannedAgentAction = { }; // Gate-blocker codes backed by CONCRETE, non-judgment evidence: a committed secret, a deterministic -// linked-issue-overlap duplicate, a rule-based content/surface-lane or manifest/pre-merge rejection, or a -// dual-model AI CONSENSUS (both independent reviewers agree) — as opposed to a single fuzzy score or an -// unconfirmed/ambiguous verdict. `ai_review_split` is deliberately excluded: the two reviewers DISAGREED, -// which is exactly the ambiguous case the close-precision breaker exists to catch. Kept here (not in +// 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. +// self-host engine vocabulary (src/rules/advisory.ts), not specific to any one repository. Two entries reuse +// advisory.ts's own exported code constants (DUPLICATE_ONLY_BLOCKER_CODES, PRE_MERGE_CHECK_BLOCKING_CODE) +// rather than retyping their literals; the rest have no single canonical export to import (each is either a +// module-private const or a raw literal duplicated across several unrelated producer files), so a source-text +// parity test in the test file below guards against drift for all nine instead of a broader cross-module +// refactor. const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ "secret_leak", - "duplicate_pr_risk", + ...DUPLICATE_ONLY_BLOCKER_CODES, "surface_lane_reject", "manifest_missing_tests", "manifest_linked_issue_required", - "pre_merge_check_required", + PRE_MERGE_CHECK_BLOCKING_CODE, "lockfile_tamper_risk", "missing_linked_issue", "self_authored_linked_issue", - "ai_consensus_defect", ]); /** 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. */ + * 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)); + return (input.gateBlockerCodes ?? []).some((code) => CONCRETE_EVIDENCE_BLOCKER_CODES.has(code) && !AI_JUDGMENT_BLOCKER_CODES.has(code)); } export type AgentActionPlanInput = { diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 12c33f9d3d..6fdb075aaa 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1,3 +1,4 @@ +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"; @@ -907,16 +908,21 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: true }); }); - it("a dual-model AI CONSENSUS defect (ai_consensus_defect) is concrete evidence — both reviewers independently agreed", () => { + 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: true }); + expect(closeOf(plan)).toMatchObject({ closeKind: "heuristic", closeConcreteEvidence: false }); }); - it("a SPLIT AI review (ai_review_split) is deliberately NOT concrete — the reviewers disagreed, stays ambiguous", () => { + 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 }); @@ -937,19 +943,49 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW)).toBe(false); }); - it("downgradeCloseToHold still downgrades a non-concrete heuristic close alongside a KEPT concrete one", () => { + // 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: "secret leaked", closeKind: "heuristic", closeConcreteEvidence: true }; const ambiguousClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "verdict failed", closeKind: "heuristic", closeConcreteEvidence: false }; const held = downgradeCloseToHold([concreteClose, ambiguousClose], true); - // Both are `closeKind: "heuristic"`, but only the non-concrete one gets swept (+ its replacement label) — - // this can't happen in a real plan (the planner emits at most one close), but proves the predicate - // discriminates on closeConcreteEvidence alone, not on closeKind or array position. 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); }); }); +// #hard-blockers-not-ai-judgment parity guard (nit): CONCRETE_EVIDENCE_BLOCKER_CODES hand-types 7 of its 9 +// literals (the other 2 -- duplicate_pr_risk, pre_merge_check_required -- are imported directly from +// advisory.ts/pre-merge-checks.ts, so a rename there is a compile error, not silent drift). Each of these 7 +// producer files defines its code as either a module-private const or a raw literal duplicated across several +// unrelated call sites, so there is no single canonical export worth centralizing around (see the doc comment +// on CONCRETE_EVIDENCE_BLOCKER_CODES). 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: "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: "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" }, + ]; + + it.each(HAND_TYPED_CODES_AND_PRODUCERS)("$code still appears as a literal in its producer ($file)", ({ code, file }) => { + const source = readFileSync(file, "utf8"); + expect(source).toContain(`"${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/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index 37238ee150..0246d59def 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/test/unit/precision-breakers-chain.test.ts @@ -74,4 +74,19 @@ describe("precisionBreakerDowngradeDirections — bounded-cardinality breaker-do 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"]); + }); }); From e702486e7b38402c370693125795261d513bd24f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 17:54:45 -0700 Subject: [PATCH 3/7] fix(agent-actions): avoid a real module-load cycle in the code-parity fix npm run test:coverage (and CI's validate/validate-code) failed with "DUPLICATE_ONLY_BLOCKER_CODES is not iterable": agent-actions.ts sits inside a genuine module-load cycle (scoring/model.ts -> db/repositories.ts -> agent-actions.ts -> rules/advisory.ts -> scoring/preview.ts -> scoring/model.ts), and eagerly spreading another module's export into a top-level array literal reads it before that module has necessarily finished initializing on the cycle's first pass. Reverted the two CONCRETE_EVIDENCE_BLOCKER_CODES entries back to plain string literals and extended the parity test to cover all nine codes instead of seven. AI_JUDGMENT_BLOCKER_CODES stays imported - it is only read inside a function body, not at module-eval time, so it isn't exposed to the same hazard. --- src/settings/agent-actions.ts | 22 ++++++++++++---------- test/unit/agent-actions.test.ts | 19 +++++++++++-------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 3b9ea5cff2..0b94a81af5 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,6 +1,5 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; -import { AI_JUDGMENT_BLOCKER_CODES, DUPLICATE_ONLY_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory"; -import { PRE_MERGE_CHECK_BLOCKING_CODE } from "../review/pre-merge-checks"; +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"; @@ -127,19 +126,22 @@ export type PlannedAgentAction = { // 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. Two entries reuse -// advisory.ts's own exported code constants (DUPLICATE_ONLY_BLOCKER_CODES, PRE_MERGE_CHECK_BLOCKING_CODE) -// rather than retyping their literals; the rest have no single canonical export to import (each is either a -// module-private const or a raw literal duplicated across several unrelated producer files), so a source-text -// parity test in the test file below guards against drift for all nine instead of a broader cross-module -// refactor. +// 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_ONLY_BLOCKER_CODES, + "duplicate_pr_risk", "surface_lane_reject", "manifest_missing_tests", "manifest_linked_issue_required", - PRE_MERGE_CHECK_BLOCKING_CODE, + "pre_merge_check_required", "lockfile_tamper_risk", "missing_linked_issue", "self_authored_linked_issue", diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 6fdb075aaa..41c0eb007d 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -961,20 +961,23 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p }); }); -// #hard-blockers-not-ai-judgment parity guard (nit): CONCRETE_EVIDENCE_BLOCKER_CODES hand-types 7 of its 9 -// literals (the other 2 -- duplicate_pr_risk, pre_merge_check_required -- are imported directly from -// advisory.ts/pre-merge-checks.ts, so a rename there is a compile error, not silent drift). Each of these 7 -// producer files defines its code as either a module-private const or a raw literal duplicated across several -// unrelated call sites, so there is no single canonical export worth centralizing around (see the doc comment -// on CONCRETE_EVIDENCE_BLOCKER_CODES). 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. +// #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" }, From 7afa9464c2b992e7f3afebc7a54b71b302fc7892 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:22:00 -0700 Subject: [PATCH 4/7] test(queue): cover the live precision-breaker downgrade call site codecov/patch flagged src/queue/processors.ts's incr() call inside runAgentMaintenancePlanAndExecute as uncovered: precisionBreakerDowngradeDirections and applyPrecisionBreakers were already exercised as pure functions in precision-breakers-chain.test.ts, but nothing drove the LIVE integration path (a real webhook, a real engaged breaker, a real withheld mutation) through this exact call site. Added an end-to-end regression alongside the existing convergence-chain test, seeding a holdonly breaker flag before the CI-completion re-review step and asserting both the withheld merge and the resulting counter. --- test/unit/queue.test.ts | 70 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 7718636117..c670ade10b 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: "installation-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 From 44d750080c89b9fa375c4aac9811aed4e335343a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:37:38 -0700 Subject: [PATCH 5/7] fix(selfhost): simplify a redundant, unreachable branch in listEngagedProjectScopes codecov/patch flagged an else-if arm as a partial branch: the query's own WHERE clause (key LIKE 'holdonly:%' OR 'closehold:%') guarantees prefix can only ever be "holdonly" or "closehold", so the second === check could never see any other value. Collapsed to a plain else -- same behavior, no unreachable branch left to cover. --- src/review/outcomes-wire.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 7b6aa9885a..2066635894 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -125,8 +125,10 @@ async function listEngagedProjectScopes(env: Env): Promise<{ holdonly: string[]; 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 (prefix === "holdonly") holdonly.push(project); - else if (prefix === "closehold") closehold.push(project); + else closehold.push(project); } return { holdonly, closehold }; } catch (error) { From 400e03d96e64b148024357a8c29010e8dbf454e4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:52:28 -0700 Subject: [PATCH 6/7] fix(agent-actions): reword test fixtures flagged by the secret scanner, address gate nits The gate's deterministic secret scanner flagged reason: "leaked secret" / "secret leaked" test fixtures as a possible generic_secret_assignment hit - no real credential, just wording that matched the heuristic's shape. Reworded to "hard blocker", which the tests don't depend on semantically. Also addresses the round-3 review nits: - Added a dedicated regression test that forces agent-actions.ts and scoring/model.ts to load together in the same module graph, so a reintroduced eager cross-module read on the documented load cycle fails a test directly instead of only being caught incidentally by unrelated suites. - Tightened the CONCRETE_EVIDENCE_BLOCKER_CODES parity test to require the actual producer assignment shape (code: "..." or a SOME_CONST = "..." export) rather than the bare literal appearing anywhere in the file, which a stale comment could have satisfied. - Documented the maintenance path in listEngagedProjectScopes's plain else: if the WHERE clause ever grows a third prefix, it must go back to an explicit branch with its own test, not stay silently bucketed as closehold. --- src/review/outcomes-wire.ts | 3 ++ test/unit/agent-action-executor.test.ts | 2 +- test/unit/agent-actions.test.ts | 32 ++++++++++++++++++++-- test/unit/precision-breakers-chain.test.ts | 2 +- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 2066635894..ce6647cfd0 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -127,6 +127,9 @@ async function listEngagedProjectScopes(env: Env): Promise<{ holdonly: string[]; 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); } diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index d5f71d9d32..001697eee9 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -268,7 +268,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { }); 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: "leaked secret", closeComment: "closing", closeKind: "heuristic", closeConcreteEvidence: true }; + 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 }); diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 41c0eb007d..bf986f83b2 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -3,6 +3,11 @@ 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 { @@ -952,7 +957,7 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p // 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: "secret leaked", closeKind: "heuristic", closeConcreteEvidence: true }; + 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); @@ -961,6 +966,22 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p }); }); +// #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 @@ -983,9 +1004,14 @@ describe("CONCRETE_EVIDENCE_BLOCKER_CODES parity — hand-typed literals still m { code: "self_authored_linked_issue", file: "src/rules/advisory.ts" }, ]; - it.each(HAND_TYPED_CODES_AND_PRODUCERS)("$code still appears as a literal in its producer ($file)", ({ code, file }) => { + // 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).toContain(`"${code}"`); + expect(source).toMatch(CODE_ASSIGNMENT_PATTERN(code)); }); }); diff --git a/test/unit/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index 0246d59def..a5af5829d2 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/test/unit/precision-breakers-chain.test.ts @@ -65,7 +65,7 @@ describe("precisionBreakerDowngradeDirections — bounded-cardinality breaker-do }); 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: "leaked secret", closeKind: "heuristic", closeConcreteEvidence: true }; + 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([]); }); From ad04158bdf27715fa946698c9c599d45a7795475 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 3 Jul 2026 19:05:55 -0700 Subject: [PATCH 7/7] test(queue): avoid a scanner-shaped token value in the new breaker regression The deterministic secret scanner's generic_secret_assignment pattern matches any `token: "..."` (or api_key/secret/password/etc.) assignment whose value is 16+ characters and doesn't look like a known placeholder string. The new holdonly-breaker test's hand-rolled fetch stub used "installation-token" (18 chars, no placeholder markers) in an ADDED line, which the scanner flags regardless of the hundreds of pre-existing, unchanged occurrences of that same literal elsewhere in this file. Shortened to a value under the length threshold; the token's actual content was never asserted on. --- test/unit/queue.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c670ade10b..b7acd013f1 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -19523,7 +19523,7 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri 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: "installation-token" }); + 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([]);