diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8d0ef51b7a..e9bfd32a0a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -631,6 +631,7 @@ import { import { isCloseHoldOnly, isHoldOnly, + readUntrustworthyRuleCodes, recordPrOutcome, recordReversalSignals, } from "../review/outcomes-wire"; @@ -2131,18 +2132,23 @@ async function resolveLiveMigrationCollisionHold( * downgrades), in order. PURE — the live flag reads happen at the call site (each fail-open), so this composes * only the transforms: * • holdOnly → downgradeMergeToHold (would-MERGE → human HOLD), else passthrough. - * • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt), else passthrough. - * Both off (the common path) returns the plan byte-identically. The breakers don't interfere: the merge - * downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`. + * • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt). + * `untrustworthyRuleCodes` (#7986) is ALWAYS passed to downgradeCloseToHold, even when `closeHoldOnly` is + * false — that function is internally self-gating (a no-op unless something is actually downgradable either + * via the project flag or a per-rule match), so this stays byte-identical to before #7986 whenever the set is + * empty (the default) or nothing matches. Both `holdOnly`/`closeHoldOnly` off AND an empty + * `untrustworthyRuleCodes` (the common path) returns the plan byte-identically. The breakers don't interfere: + * the merge downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`. */ export function applyPrecisionBreakers( planned: PlannedAgentAction[], holdOnly: boolean, closeHoldOnly: boolean, labelSettings: AgentDispositionLabelSettings = {}, + untrustworthyRuleCodes: ReadonlySet = new Set(), ): PlannedAgentAction[] { const afterMerge = holdOnly ? downgradeMergeToHold(planned, true, labelSettings) : planned; - return closeHoldOnly ? downgradeCloseToHold(afterMerge, true, labelSettings) : afterMerge; + return downgradeCloseToHold(afterMerge, closeHoldOnly, labelSettings, untrustworthyRuleCodes); } /** PURE: which precision-breaker directions actually rewrote the plan — i.e. `planned` had a merge/close that @@ -3182,6 +3188,9 @@ async function runAgentMaintenancePlanAndExecute( migrationCollisionLabel: settings.migrationCollisionLabel, pendingClosureLabel: settings.pendingClosureLabel, }, + // #7986: a cheap, cron-refreshed single-row read (readUntrustworthyRuleCodes) — never a fresh aggregate + // query on the hot webhook path. Fail-open (empty set) on any read error, same as isHoldOnly/isCloseHoldOnly. + await readUntrustworthyRuleCodes(env), ); // 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 diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 4a0cc16af7..a2ad3172a2 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -41,6 +41,7 @@ import { } from "./auto-tune"; import { computeGateEval } from "./parity"; import { LOOPOVER_NATIVE_SOURCE } from "./parity-wire"; +import { computeBlendedRuleGateEval, rulesBelowClosePrecisionFloor } from "./rule-gate-eval"; /** 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. */ @@ -219,6 +220,46 @@ export function createFlagStore(env: Env): FlagStore { }; } +// #7986: which deterministic rule codes currently sit below their OWN measured close-precision floor +// (rulesBelowClosePrecisionFloor over computeBlendedRuleGateEval, #7984) — a cheap, cron-refreshed cache of an +// otherwise-expensive fleet-wide aggregate, reusing system_flags (a generic key/value table, not booleans-only +// despite its FlagStore-facing name above) so no schema change is needed. Mirrors the SAME "expensive compute +// on a cron tick, cheap single-row read at decision time" split isHoldOnly/isCloseHoldOnly already use for the +// project-level breaker flags. FAIL-SAFE: a read error, missing row, or unparseable value degrades to an EMPTY +// set — exactly #7986's own "insufficient/unavailable data defaults to keeping the exemption" rule, never the +// opposite direction (a read failure must never spuriously revoke every rule's exemption at once). +const UNTRUSTWORTHY_RULE_CODES_FLAG_KEY = "rule_untrustworthy_codes:global"; + +/** Read the cron-cached set of rule codes currently below their close-precision floor. See this constant's own + * doc comment above for the fail-safe contract. */ +export async function readUntrustworthyRuleCodes(env: Env): Promise> { + try { + const row = await env.DB.prepare("SELECT value FROM system_flags WHERE key = ?") + .bind(UNTRUSTWORTHY_RULE_CODES_FLAG_KEY) + .first<{ value: string }>(); + if (!row?.value) return new Set(); + const parsed: unknown = JSON.parse(row.value); + if (!Array.isArray(parsed)) return new Set(); + return new Set(parsed.filter((code): code is string => typeof code === "string")); + } catch { + return new Set(); + } +} + +/** Write the cron-computed set of rule codes currently below their close-precision floor, replacing whatever + * was cached before (this is a SNAPSHOT, not an append-only log — a code that recovers or that no longer has + * a large enough sample must disappear from the set on the next tick, not linger). Best-effort: a write + * failure is swallowed, matching every other cron-tick cache write in this module — the NEXT tick will retry, + * and until then {@link readUntrustworthyRuleCodes} keeps serving the last successfully-written snapshot. */ +async function writeUntrustworthyRuleCodes(env: Env, codes: readonly string[]): Promise { + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)", + ) + .bind(UNTRUSTWORTHY_RULE_CODES_FLAG_KEY, JSON.stringify([...codes])) + .run() + .catch(() => undefined); +} + // ── review_audit append (the canonical eval/parity store) ─────────────────────────────────────────────────── /** The target_id the gate-decision writer (parity-wire.ts) stamps — `project#pr`. The pr_outcome/reversal rows @@ -766,6 +807,16 @@ export async function runSelfTuneBreaker(env: Env): Promise { await runBreakerPassForReport(flags, plainPass.report, plainPass.engagedHoldonly, plainPass.engagedClosehold, nowMs, ""); await runBreakerPassForReport(flags, minerPass.report, minerPass.engagedHoldonly, minerPass.engagedClosehold, nowMs, "miner_"); + + // #7986: refresh the per-rule track-record cache the concrete-evidence breaker exemption reads + // (readUntrustworthyRuleCodes) -- SAME window, pooled cross-project (a rule's trustworthiness is a + // property of the rule, not of any one repo it happened to trip). Independent of the two passes above: + // a failure here must not prevent (and does not roll back) the merge/close breaker engagement that just + // completed -- computeBlendedRuleGateEval and writeUntrustworthyRuleCodes are both already fail-safe on + // their own, so no extra try/catch is needed beyond this function's own outer one. + const ruleReport = await computeBlendedRuleGateEval(env, { days: BREAKER_EVAL_WINDOW_DAYS, nowMs, source: LOOPOVER_NATIVE_SOURCE }); + const untrustworthyCodes = rulesBelowClosePrecisionFloor(ruleReport.rows).map((row) => row.ruleCode); + await writeUntrustworthyRuleCodes(env, untrustworthyCodes); } catch (error) { console.warn( JSON.stringify({ diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 7a929f7c52..39c26fd941 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -5,7 +5,7 @@ import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor"; import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; -import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; +import { isCloseHoldOnly, isHoldOnly, readUntrustworthyRuleCodes } from "../review/outcomes-wire"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -325,7 +325,14 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // Re-apply the SAME merge/close precision circuit-breakers the live webhook path applies before executing, so // a breaker engaged AFTER staging (an operator halting a runaway auto-merge, or the auto-tuner tripping on a // precision drop) still holds this sticky pending row instead of executing it unmodified. (#2127) - const [holdOnly, closeHoldOnly] = await Promise.all([isHoldOnly(env, pending.repoFullName), isCloseHoldOnly(env, pending.repoFullName)]); + // #7986: the same per-rule track-record read the live webhook path uses -- a staged close backed ONLY by a + // now-untrustworthy code must not slip through just because it was accepted from the approval queue instead + // of the live path. + const [holdOnly, closeHoldOnly, untrustworthyRuleCodes] = await Promise.all([ + isHoldOnly(env, pending.repoFullName), + isCloseHoldOnly(env, pending.repoFullName), + readUntrustworthyRuleCodes(env), + ]); let plan: PlannedAgentAction[] = [pendingActionToPlanned({ actionClass: pending.actionClass, params: liveParams, reason: pending.reason })]; const labelSettings = { manualReviewLabel: settings.manualReviewLabel, @@ -335,7 +342,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de pendingClosureLabel: settings.pendingClosureLabel, }; if (holdOnly) plan = downgradeMergeToHold(plan, true, labelSettings); - if (closeHoldOnly) plan = downgradeCloseToHold(plan, true, labelSettings); + plan = downgradeCloseToHold(plan, closeHoldOnly, labelSettings, untrustworthyRuleCodes); // Re-validate a staged MERGE against the CURRENT linked-issue hard-rule state (#2132). The hard rule is // evaluated fresh on every planning pass and takes precedence over merge (see planAgentMaintenanceActions), diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index c5b6dccfcf..7eb4f80c28 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -165,6 +165,14 @@ export type PlannedAgentAction = { // 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; + // #7986: the specific gate-blocker code(s) (a subset of CONCRETE_EVIDENCE_BLOCKER_CODES) that justify + // `closeConcreteEvidence: true` via the blocker-code path specifically -- empty/absent when the evidence is + // CI-failure/base-conflict/duplicate-link based instead (those are not "rules" with a measurable per-code + // track record the same way a blocker code is, so they are never subject to the per-rule downgrade + // downgradeCloseToHold now also applies). Lets downgradeCloseToHold check a close's justification against a + // live per-rule precision track record, instead of trusting blanket CONCRETE_EVIDENCE_BLOCKER_CODES + // membership forever regardless of that specific code's own real-world accuracy. + closeConcreteEvidenceCodes?: string[]; 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 @@ -216,13 +224,21 @@ const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ "content_lane_deliverable_missing", ]); +/** The specific gate-blocker code(s) that justify concrete evidence via the {@link CONCRETE_EVIDENCE_BLOCKER_CODES} + * path specifically (#7986) — a subset of `input.gateBlockerCodes`, excluding advisory.ts's own + * {@link AI_JUDGMENT_BLOCKER_CODES} (belt-and-suspenders, same guard {@link hasConcreteCloseEvidence} always + * had). Empty when the close's ONLY concrete evidence is CI-failure/base-conflict/duplicate-link based + * (`hasConcreteCloseEvidence` returns true via one of those without ever reaching this) — those are not + * "rules" with a measurable per-code track record the same way a blocker code is, so `downgradeCloseToHold`'s + * per-rule check must never apply to them. */ +function concreteCloseEvidenceCodes(input: AgentActionPlanInput): string[] { + return (input.gateBlockerCodes ?? []).filter((code) => CONCRETE_EVIDENCE_BLOCKER_CODES.has(code) && !AI_JUDGMENT_BLOCKER_CODES.has(code)); +} + /** 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). */ + * base conflict, a deterministic duplicate-PR link, or a gate-blocker code in {@link CONCRETE_EVIDENCE_BLOCKER_CODES} + * (via {@link concreteCloseEvidenceCodes}). 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; // A duplicate-PR link stays concrete evidence even now that the close has its own live staleness recheck @@ -233,7 +249,7 @@ function hasConcreteCloseEvidence(input: AgentActionPlanInput, ciFailed: boolean // itself closes. A duplicate-issue-link, like a base conflict, is still a deterministic, zero-hallucination // fact about the linked-issue graph; it just needs to be re-verified fresh, which it now is. 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)); + return concreteCloseEvidenceCodes(input).length > 0; } export type AgentActionPlanInput = { @@ -554,20 +570,51 @@ export function downgradeMergeToHold(planned: PlannedAgentAction[], holdOnly: bo * 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. * + * #7986: the concrete-evidence exemption above is no longer UNCONDITIONAL. `untrustworthyRuleCodes` — the + * set of blocker codes whose OWN measured close-precision has dropped below its floor over a real sample + * (`rulesBelowClosePrecisionFloor` over `computeBlendedRuleGateEval`, #7984) — makes a close's concrete + * evidence STOP counting as an exemption when EVERY code that justified it (`closeConcreteEvidenceCodes`) is + * in that set. This fires INDEPENDENTLY of `closeHoldOnly` (the PROJECT-level flag): a single systematically + * wrong rule can sit at 0% precision while diluted into an otherwise-healthy project aggregate (exactly the + * class of bug #7984 exists to surface), so this rule-level check must not wait for the project flag to + * engage. A code with an insufficient sample, or one that isn't in the set at all, keeps its exemption -- + * `rulesBelowClosePrecisionFloor`'s own "insufficient sample defaults to keeping the exemption" contract. + * `untrustworthyRuleCodes` defaults to empty (byte-identical to pre-#7986 behavior when omitted). + * * 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 downgradable close - * planned it is also a no-op. Only ever makes the system MORE cautious. + * with `closeHoldOnly` false AND no untrustworthy-rule match, 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, labelSettings: AgentDispositionLabelSettings = {}): PlannedAgentAction[] { - const isHeuristicClose = (action: PlannedAgentAction): boolean => action.actionClass === "close" && action.closeKind === "heuristic" && action.closeConcreteEvidence !== true; - if (!closeHoldOnly || !planned.some(isHeuristicClose)) return planned; +export function downgradeCloseToHold( + planned: PlannedAgentAction[], + closeHoldOnly: boolean, + labelSettings: AgentDispositionLabelSettings = {}, + untrustworthyRuleCodes: ReadonlySet = new Set(), +): PlannedAgentAction[] { + // Reason A (project-level, unchanged from before #7986): no concrete evidence at all, AND the project's + // close-precision breaker has engaged. + const noConcreteEvidenceUnderProjectBreaker = (action: PlannedAgentAction): boolean => + action.actionClass === "close" && action.closeKind === "heuristic" && action.closeConcreteEvidence !== true && closeHoldOnly; + // Reason B (#7986, per-rule, independent of closeHoldOnly): HAS concrete evidence, but every code that + // justified it has its own bad track record. `.every` (not `.some`) so a close backed by a MIX of a + // trustworthy code and an untrustworthy one keeps its exemption via the trustworthy code -- only a close + // whose EVERY justifying code is known-bad loses it. + const everyJustifyingCodeUntrustworthy = (action: PlannedAgentAction): boolean => { + const codes = action.closeConcreteEvidenceCodes ?? []; + return codes.length > 0 && codes.every((code) => untrustworthyRuleCodes.has(code)); + }; + const isDowngradableClose = (action: PlannedAgentAction): boolean => + action.actionClass === "close" && + action.closeKind === "heuristic" && + (noConcreteEvidenceUnderProjectBreaker(action) || (action.closeConcreteEvidence === true && everyJustifyingCodeUntrustworthy(action))); + if (!planned.some(isDowngradableClose)) return planned; const labels = resolveAgentDispositionLabels(labelSettings); - // 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)); + // Drop ONLY the downgradable close(s); a deterministic linked-issue-hard-rule close (if any) is left intact. + const next = planned.filter((action) => !isDowngradableClose(action)); // The dropped close means the PR is held for a person — surface the manual-review label. Idempotent: only add when // absent (e.g. a guarded-but-passing plan may already carry it). NEVER adds a merge/approve. const alreadyNeedsReview = labels.manualReview !== null && next.some((action) => action.actionClass === "label" && action.label === labels.manualReview && action.labelOp !== "remove"); - const droppedClose = planned.find(isHeuristicClose); + const droppedClose = planned.find(isDowngradableClose); if (labels.manualReview !== null && !alreadyNeedsReview) { next.push({ actionClass: "label", @@ -1365,6 +1412,10 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne closeComment: closeMessage(closeReasons), closeKind: "heuristic", closeConcreteEvidence: hasConcreteCloseEvidence(input, ciFailed, isConflict), + // #7986: preserved alongside the collapsed boolean above so downgradeCloseToHold can check this specific + // close's justification against a live per-rule precision track record. Empty when the evidence above + // came from ciFailed/isConflict/linkedDuplicateCount instead of a blocker code. + closeConcreteEvidenceCodes: concreteCloseEvidenceCodes(input), // 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/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index c436e8055f..4c6d03d6ac 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1785,6 +1785,115 @@ describe("closeConcreteEvidence — concrete-evidence exemption from the close-p expect(held.some((a) => a === ambiguousClose)).toBe(false); expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW)).toBe(true); }); + + // #7986: closeConcreteEvidenceCodes preserves the SPECIFIC justifying code(s) alongside the collapsed + // closeConcreteEvidence boolean, so downgradeCloseToHold can check a close's justification against a live + // per-rule precision track record. + it("closeConcreteEvidenceCodes carries the specific blocker code(s) that justified a blocker-code-based concrete close", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", gateBlockerCodes: ["secret_leak"], blockerTitles: ["Possible leaked secret"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: ["secret_leak"] }); + }); + + it("closeConcreteEvidenceCodes is EMPTY for CI-failure-justified concrete evidence -- CI is not a 'rule' with a per-code track record", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "failed", failingCheckNames: ["ci"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: [] }); + }); + + it("closeConcreteEvidenceCodes is EMPTY for a base-conflict-justified concrete evidence", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], mergeableState: "dirty" } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: [] }); + }); + + it("closeConcreteEvidenceCodes is EMPTY for a duplicate-link-justified concrete evidence", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto" }, ciState: "passed", pr: { labels: [], linkedDuplicateCount: 1 } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: [] }); + }); + + it("closeConcreteEvidenceCodes excludes an AI-judgment code even when a real concrete code is also present", () => { + 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({ closeConcreteEvidenceCodes: ["secret_leak"] }); + }); +}); + +describe("downgradeCloseToHold — per-rule track record overrides the blanket exemption (#7986)", () => { + const closeOf = (plan: ReturnType) => plan.find((a) => a.actionClass === "close"); + const concreteClosePlan = (codes: string[]) => + planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "passed", gateBlockerCodes: codes, blockerTitles: codes.map(() => "x"), pr: { labels: [] } })); + const linkedIssueClosePlan = () => + planAgentMaintenanceActions( + input({ + conclusion: "success", + autonomy: { close: "auto", review_state_label: "auto" }, + ciState: "passed", + linkedIssueHardRule: { violated: true, reason: "Linked issue #5 is labeled `maintainer-only` — it is not open for community PRs." }, + linkedIssueVerify: { verifyBeforeClose: false, closeDelaySeconds: 0 }, + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + }), + ); + + it("INCIDENT REPLAY: a concrete-evidence code with 0% measured precision over a real sample loses its exemption, even with closeHoldOnly FALSE", () => { + const plan = concreteClosePlan(["surface_lane_reject"]); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: true, closeConcreteEvidenceCodes: ["surface_lane_reject"] }); + // closeHoldOnly is FALSE here -- the PROJECT aggregate looks fine (this is exactly the #7984 dilution + // scenario: one bad rule hiding inside an otherwise-healthy project). The per-rule track record alone + // must still catch it. + const held = downgradeCloseToHold(plan, false, {}, new Set(["surface_lane_reject"])); + expect(held.some((a) => a.actionClass === "close")).toBe(false); + expect(held.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "add")).toBe(true); + }); + + it("a healthy concrete-evidence rule (not in untrustworthyRuleCodes) is NEVER spuriously held, even with closeHoldOnly true", () => { + const plan = concreteClosePlan(["secret_leak"]); + const held = downgradeCloseToHold(plan, true, {}, new Set(["surface_lane_reject"])); // a DIFFERENT code is untrustworthy + expect(held).toBe(plan); // fully unchanged -- secret_leak is not in the untrustworthy set + expect(held.some((a) => a.actionClass === "close")).toBe(true); + }); + + it("an untrustworthy code with an EMPTY set (the default) never triggers the per-rule path -- byte-identical to pre-#7986 behavior", () => { + const plan = concreteClosePlan(["surface_lane_reject"]); + expect(downgradeCloseToHold(plan, false)).toBe(plan); // no 4th arg at all + expect(downgradeCloseToHold(plan, false, {}, new Set())).toBe(plan); // explicit empty set + }); + + it("a close backed by a MIX of a trustworthy and an untrustworthy code KEEPS its exemption (only ALL-untrustworthy loses it)", () => { + const plan = concreteClosePlan(["secret_leak", "surface_lane_reject"]); + expect(closeOf(plan)?.closeConcreteEvidenceCodes).toEqual(["secret_leak", "surface_lane_reject"]); + const held = downgradeCloseToHold(plan, false, {}, new Set(["surface_lane_reject"])); + expect(held).toBe(plan); // secret_leak alone still justifies the exemption + expect(held.some((a) => a.actionClass === "close")).toBe(true); + }); + + it("downgrades once BOTH justifying codes are untrustworthy", () => { + const plan = concreteClosePlan(["secret_leak", "surface_lane_reject"]); + const held = downgradeCloseToHold(plan, false, {}, new Set(["secret_leak", "surface_lane_reject"])); + expect(held.some((a) => a.actionClass === "close")).toBe(false); + }); + + it("CI-failure-justified concrete evidence (empty closeConcreteEvidenceCodes) is NEVER downgraded via the per-rule path, no matter what's in untrustworthyRuleCodes", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "failed", failingCheckNames: ["ci"], pr: { labels: [] } })); + expect(closeOf(plan)?.closeConcreteEvidenceCodes).toEqual([]); + // A pathological untrustworthyRuleCodes set containing every string imaginable still must not match an + // EMPTY justifying-codes array -- codes.length > 0 is a hard guard, not just an optimization. + const held = downgradeCloseToHold(plan, false, {}, new Set(["ci", ""])); + expect(held).toBe(plan); + expect(held.some((a) => a.actionClass === "close")).toBe(true); + }); + + it("a non-concrete (ambiguous) heuristic close is unaffected by untrustworthyRuleCodes -- it already has no exemption to lose, and the project-level closeHoldOnly path still governs it", () => { + const plan = planAgentMaintenanceActions(input({ conclusion: "failure", autonomy: { close: "auto", review_state_label: "auto" }, ciState: "passed", blockerTitles: ["readiness score too low"], pr: { labels: [] } })); + expect(closeOf(plan)).toMatchObject({ closeConcreteEvidence: false }); + // closeHoldOnly false + no untrustworthy match -> unchanged (Reason A requires closeHoldOnly). + expect(downgradeCloseToHold(plan, false, {}, new Set(["some_code"]))).toBe(plan); + // closeHoldOnly true -> still downgraded via the EXISTING project-level path, untouched by #7986. + const held = downgradeCloseToHold(plan, true, {}, new Set()); + expect(held.some((a) => a.actionClass === "close")).toBe(false); + }); + + it("the deterministic linked-issue-hard-rule close stays exempt from the per-rule path too (it never carries closeConcreteEvidenceCodes)", () => { + const plan = linkedIssueClosePlan(); + const held = downgradeCloseToHold(plan, false, {}, new Set(["anything"])); + expect(held).toBe(plan); + }); }); // #module-cycle-regression: agent-actions.ts imports AI_JUDGMENT_BLOCKER_CODES from rules/advisory.ts, which diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index b363f38d47..ee12f2ddb1 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -5,6 +5,7 @@ import { isCloseHoldOnly, isHoldOnly, parseRevertedPrNumber, + readUntrustworthyRuleCodes, recordPrOutcome, recordReversalSignals, resolveDispositionReason, @@ -622,6 +623,123 @@ describe("isHoldOnly + createFlagStore (system_flags, migration 0054)", () => { }); }); +describe("readUntrustworthyRuleCodes (#7986, same system_flags table)", () => { + it("returns an empty set when nothing has ever been written", async () => { + const env = createTestEnv(); + const codes = await readUntrustworthyRuleCodes(env); + expect(codes.size).toBe(0); + }); + + it("round-trips a written snapshot via runSelfTuneBreaker's own cache write", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', ?, CURRENT_TIMESTAMP)", + ) + .bind(JSON.stringify(["surface_lane_reject", "missing_linked_issue"])) + .run(); + const codes = await readUntrustworthyRuleCodes(env); + expect([...codes].sort()).toEqual(["missing_linked_issue", "surface_lane_reject"]); + }); + + it("degrades to an empty set (fail-open) when the stored value is invalid JSON", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', ?, CURRENT_TIMESTAMP)", + ) + .bind("{not valid json") + .run(); + expect((await readUntrustworthyRuleCodes(env)).size).toBe(0); + }); + + it("degrades to an empty set when the stored value parses but isn't an array", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', ?, CURRENT_TIMESTAMP)", + ) + .bind(JSON.stringify({ not: "an array" })) + .run(); + expect((await readUntrustworthyRuleCodes(env)).size).toBe(0); + }); + + it("filters out any non-string element rather than throwing", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', ?, CURRENT_TIMESTAMP)", + ) + .bind(JSON.stringify(["surface_lane_reject", 42, null, "missing_linked_issue"])) + .run(); + const codes = await readUntrustworthyRuleCodes(env); + expect([...codes].sort()).toEqual(["missing_linked_issue", "surface_lane_reject"]); + }); + + it("fails open (empty set) when the DB read throws", async () => { + const env = { DB: { prepare: () => ({ bind: () => ({ first: async () => { throw new Error("d1 down"); } }) }) } } as unknown as Env; + expect((await readUntrustworthyRuleCodes(env)).size).toBe(0); + }); + + it("treats an empty-string stored value the same as no row at all", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', '', CURRENT_TIMESTAMP)", + ).run(); + expect((await readUntrustworthyRuleCodes(env)).size).toBe(0); + }); +}); + +describe("runSelfTuneBreaker — also refreshes the per-rule track-record cache (#7986)", () => { + it("INCIDENT REPLAY: writes an isolated 0%-precision rule to the cache even while its project's own close-precision aggregate looks healthy", async () => { + const env = createTestEnv(); + const seedClose = async (id: string, ruleCode: string, truth: "closed" | "merged"): Promise => { + await env.DB.prepare( + `INSERT INTO review_audit (id, project, target_id, event_type, decision, summary, source, created_at) VALUES (?, 'metagraphed/metagraphed', ?, 'gate_decision', 'close', ?, 'gittensory-native', ?)`, + ) + .bind(`gd-${id}`, `metagraphed/metagraphed#${id}`, ruleCode, new Date().toISOString()) + .run(); + await env.DB.prepare( + `INSERT INTO review_audit (id, project, target_id, event_type, decision, source, created_at) VALUES (?, 'metagraphed/metagraphed', ?, 'pr_outcome', ?, 'github', ?)`, + ) + .bind(`po-${id}`, `metagraphed/metagraphed#${id}`, truth, new Date().toISOString()) + .run(); + }; + // The buggy rule: 12 closes (clears AUTOTUNE_MIN_DECIDED), every single one later merged -- 0% precision. + for (let i = 1; i <= 12; i++) await seedClose(`bad-${i}`, "surface_lane_reject", "merged"); + // The SAME project's every OTHER close reason: perfectly healthy -- would dilute a project-wide number, + // exactly the scenario #7984/#7986 exist to catch. + for (let i = 1; i <= 20; i++) await seedClose(`good-${i}`, "missing_linked_issue", "closed"); + + await runSelfTuneBreaker(env); + const codes = await readUntrustworthyRuleCodes(env); + expect(codes.has("surface_lane_reject")).toBe(true); + expect(codes.has("missing_linked_issue")).toBe(false); + }); + + it("writes an empty set (not a stale one) once every previously-bad rule recovers on a later tick", async () => { + const env = createTestEnv(); + await env.DB.prepare( + "INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES ('rule_untrustworthy_codes:global', ?, CURRENT_TIMESTAMP)", + ) + .bind(JSON.stringify(["stale_code"])) + .run(); + await runSelfTuneBreaker(env); // no review_audit rows at all -> nothing below any floor + const codes = await readUntrustworthyRuleCodes(env); + expect(codes.has("stale_code")).toBe(false); + }); + + it("swallows a write failure on the rule-code cache without throwing or rolling back the breaker passes that already ran", async () => { + const env = createTestEnv(); + const realPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => { + if (/INSERT OR REPLACE INTO system_flags/i.test(sql)) { + return { + bind: () => ({ run: async () => { throw new Error("d1 down"); } }), + } as unknown as ReturnType; + } + return realPrepare(sql); + }) as typeof env.DB.prepare; + await expect(runSelfTuneBreaker(env)).resolves.toBeUndefined(); + }); +}); + describe("isCloseHoldOnly + createFlagStore.isCloseHoldOnly (closehold:, same system_flags table)", () => { it("isCloseHoldOnly is false with no flags, true once closehold: is set, with per-project isolation, and respects closehold:global", async () => { const env = createTestEnv();