Skip to content

Commit c02a277

Browse files
authored
fix(agent-actions): replace blanket concrete-evidence breaker exemption with per-rule track record (#8124)
CONCRETE_EVIDENCE_BLOCKER_CODES membership alone made a heuristic close categorically immune to the close-precision circuit breaker, regardless of that specific rule's own measured accuracy. A single systematically wrong rule can sit at 0% precision while diluted into an otherwise- healthy project aggregate, so downgradeCloseToHold now also checks a close's justifying code(s) against a live, cron-refreshed per-rule track record (computeBlendedRuleGateEval) and drops the exemption only when every justifying code is below its own close-precision floor. Insufficient sample size still defaults to keeping the exemption. Closes #7986.
1 parent 073de61 commit c02a277

6 files changed

Lines changed: 367 additions & 22 deletions

File tree

src/queue/processors.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,7 @@ import {
633633
import {
634634
isCloseHoldOnly,
635635
isHoldOnly,
636+
readUntrustworthyRuleCodes,
636637
recordPrOutcome,
637638
recordReversalSignals,
638639
} from "../review/outcomes-wire";
@@ -2133,18 +2134,23 @@ async function resolveLiveMigrationCollisionHold(
21332134
* downgrades), in order. PURE — the live flag reads happen at the call site (each fail-open), so this composes
21342135
* only the transforms:
21352136
* • holdOnly → downgradeMergeToHold (would-MERGE → human HOLD), else passthrough.
2136-
* • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt), else passthrough.
2137-
* Both off (the common path) returns the plan byte-identically. The breakers don't interfere: the merge
2138-
* downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`.
2137+
* • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt).
2138+
* `untrustworthyRuleCodes` (#7986) is ALWAYS passed to downgradeCloseToHold, even when `closeHoldOnly` is
2139+
* false — that function is internally self-gating (a no-op unless something is actually downgradable either
2140+
* via the project flag or a per-rule match), so this stays byte-identical to before #7986 whenever the set is
2141+
* empty (the default) or nothing matches. Both `holdOnly`/`closeHoldOnly` off AND an empty
2142+
* `untrustworthyRuleCodes` (the common path) returns the plan byte-identically. The breakers don't interfere:
2143+
* the merge downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`.
21392144
*/
21402145
export function applyPrecisionBreakers(
21412146
planned: PlannedAgentAction[],
21422147
holdOnly: boolean,
21432148
closeHoldOnly: boolean,
21442149
labelSettings: AgentDispositionLabelSettings = {},
2150+
untrustworthyRuleCodes: ReadonlySet<string> = new Set(),
21452151
): PlannedAgentAction[] {
21462152
const afterMerge = holdOnly ? downgradeMergeToHold(planned, true, labelSettings) : planned;
2147-
return closeHoldOnly ? downgradeCloseToHold(afterMerge, true, labelSettings) : afterMerge;
2153+
return downgradeCloseToHold(afterMerge, closeHoldOnly, labelSettings, untrustworthyRuleCodes);
21482154
}
21492155

21502156
/** PURE: which precision-breaker directions actually rewrote the plan — i.e. `planned` had a merge/close that
@@ -3184,6 +3190,9 @@ async function runAgentMaintenancePlanAndExecute(
31843190
migrationCollisionLabel: settings.migrationCollisionLabel,
31853191
pendingClosureLabel: settings.pendingClosureLabel,
31863192
},
3193+
// #7986: a cheap, cron-refreshed single-row read (readUntrustworthyRuleCodes) — never a fresh aggregate
3194+
// query on the hot webhook path. Fail-open (empty set) on any read error, same as isHoldOnly/isCloseHoldOnly.
3195+
await readUntrustworthyRuleCodes(env),
31873196
);
31883197
// Observability (#terminal-outcome-audit): a bounded-cardinality counter (direction only — no repo/PR/reason
31893198
// text) so an operator can see, at a glance, how much of the plan a breaker is currently rewriting, without

src/review/outcomes-wire.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import {
4646
} from "./auto-tune";
4747
import { computeGateEval } from "./parity";
4848
import { LOOPOVER_NATIVE_SOURCE } from "./parity-wire";
49+
import { computeBlendedRuleGateEval, rulesBelowClosePrecisionFloor } from "./rule-gate-eval";
4950

5051
/** PURE: parse the PR number an "Reverts #N / Reverts owner/repo#N" body refers to (GitHub's revert PRs).
5152
* Mirrors reviewbot runtime.ts parseRevertedPrNumber. Returns undefined when the body isn't a revert. */
@@ -224,6 +225,46 @@ export function createFlagStore(env: Env): FlagStore {
224225
};
225226
}
226227

228+
// #7986: which deterministic rule codes currently sit below their OWN measured close-precision floor
229+
// (rulesBelowClosePrecisionFloor over computeBlendedRuleGateEval, #7984) — a cheap, cron-refreshed cache of an
230+
// otherwise-expensive fleet-wide aggregate, reusing system_flags (a generic key/value table, not booleans-only
231+
// despite its FlagStore-facing name above) so no schema change is needed. Mirrors the SAME "expensive compute
232+
// on a cron tick, cheap single-row read at decision time" split isHoldOnly/isCloseHoldOnly already use for the
233+
// project-level breaker flags. FAIL-SAFE: a read error, missing row, or unparseable value degrades to an EMPTY
234+
// set — exactly #7986's own "insufficient/unavailable data defaults to keeping the exemption" rule, never the
235+
// opposite direction (a read failure must never spuriously revoke every rule's exemption at once).
236+
const UNTRUSTWORTHY_RULE_CODES_FLAG_KEY = "rule_untrustworthy_codes:global";
237+
238+
/** Read the cron-cached set of rule codes currently below their close-precision floor. See this constant's own
239+
* doc comment above for the fail-safe contract. */
240+
export async function readUntrustworthyRuleCodes(env: Env): Promise<ReadonlySet<string>> {
241+
try {
242+
const row = await env.DB.prepare("SELECT value FROM system_flags WHERE key = ?")
243+
.bind(UNTRUSTWORTHY_RULE_CODES_FLAG_KEY)
244+
.first<{ value: string }>();
245+
if (!row?.value) return new Set();
246+
const parsed: unknown = JSON.parse(row.value);
247+
if (!Array.isArray(parsed)) return new Set();
248+
return new Set(parsed.filter((code): code is string => typeof code === "string"));
249+
} catch {
250+
return new Set();
251+
}
252+
}
253+
254+
/** Write the cron-computed set of rule codes currently below their close-precision floor, replacing whatever
255+
* was cached before (this is a SNAPSHOT, not an append-only log — a code that recovers or that no longer has
256+
* a large enough sample must disappear from the set on the next tick, not linger). Best-effort: a write
257+
* failure is swallowed, matching every other cron-tick cache write in this module — the NEXT tick will retry,
258+
* and until then {@link readUntrustworthyRuleCodes} keeps serving the last successfully-written snapshot. */
259+
async function writeUntrustworthyRuleCodes(env: Env, codes: readonly string[]): Promise<void> {
260+
await env.DB.prepare(
261+
"INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)",
262+
)
263+
.bind(UNTRUSTWORTHY_RULE_CODES_FLAG_KEY, JSON.stringify([...codes]))
264+
.run()
265+
.catch(() => undefined);
266+
}
267+
227268
// ── review_audit append (the canonical eval/parity store) ───────────────────────────────────────────────────
228269

229270
/** The target_id the gate-decision writer (parity-wire.ts) stamps — `project#pr`. The pr_outcome/reversal rows
@@ -828,6 +869,16 @@ export async function runSelfTuneBreaker(env: Env): Promise<void> {
828869

829870
await runBreakerPassForReport(flags, plainPass.report, plainPass.engagedHoldonly, plainPass.engagedClosehold, nowMs, "");
830871
await runBreakerPassForReport(flags, minerPass.report, minerPass.engagedHoldonly, minerPass.engagedClosehold, nowMs, "miner_");
872+
873+
// #7986: refresh the per-rule track-record cache the concrete-evidence breaker exemption reads
874+
// (readUntrustworthyRuleCodes) -- SAME window, pooled cross-project (a rule's trustworthiness is a
875+
// property of the rule, not of any one repo it happened to trip). Independent of the two passes above:
876+
// a failure here must not prevent (and does not roll back) the merge/close breaker engagement that just
877+
// completed -- computeBlendedRuleGateEval and writeUntrustworthyRuleCodes are both already fail-safe on
878+
// their own, so no extra try/catch is needed beyond this function's own outer one.
879+
const ruleReport = await computeBlendedRuleGateEval(env, { days: BREAKER_EVAL_WINDOW_DAYS, nowMs, source: LOOPOVER_NATIVE_SOURCE });
880+
const untrustworthyCodes = rulesBelowClosePrecisionFloor(ruleReport.rows).map((row) => row.ruleCode);
881+
await writeUntrustworthyRuleCodes(env, untrustworthyCodes);
831882
} catch (error) {
832883
console.warn(
833884
JSON.stringify({

src/services/agent-approval-queue.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/
55
import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor";
66
import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions";
77
import { findBlacklistEntry } from "../settings/contributor-blacklist";
8-
import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire";
8+
import { isCloseHoldOnly, isHoldOnly, readUntrustworthyRuleCodes } from "../review/outcomes-wire";
99
import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, mergeRequiredCiContexts } from "../github/backfill";
1010
import { githubRateLimitAdmissionKeyForToken } from "../github/client";
1111
import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types";
@@ -325,7 +325,14 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
325325
// Re-apply the SAME merge/close precision circuit-breakers the live webhook path applies before executing, so
326326
// a breaker engaged AFTER staging (an operator halting a runaway auto-merge, or the auto-tuner tripping on a
327327
// precision drop) still holds this sticky pending row instead of executing it unmodified. (#2127)
328-
const [holdOnly, closeHoldOnly] = await Promise.all([isHoldOnly(env, pending.repoFullName), isCloseHoldOnly(env, pending.repoFullName)]);
328+
// #7986: the same per-rule track-record read the live webhook path uses -- a staged close backed ONLY by a
329+
// now-untrustworthy code must not slip through just because it was accepted from the approval queue instead
330+
// of the live path.
331+
const [holdOnly, closeHoldOnly, untrustworthyRuleCodes] = await Promise.all([
332+
isHoldOnly(env, pending.repoFullName),
333+
isCloseHoldOnly(env, pending.repoFullName),
334+
readUntrustworthyRuleCodes(env),
335+
]);
329336
let plan: PlannedAgentAction[] = [pendingActionToPlanned({ actionClass: pending.actionClass, params: liveParams, reason: pending.reason })];
330337
const labelSettings = {
331338
manualReviewLabel: settings.manualReviewLabel,
@@ -335,7 +342,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
335342
pendingClosureLabel: settings.pendingClosureLabel,
336343
};
337344
if (holdOnly) plan = downgradeMergeToHold(plan, true, labelSettings);
338-
if (closeHoldOnly) plan = downgradeCloseToHold(plan, true, labelSettings);
345+
plan = downgradeCloseToHold(plan, closeHoldOnly, labelSettings, untrustworthyRuleCodes);
339346

340347
// Re-validate a staged MERGE against the CURRENT linked-issue hard-rule state (#2132). The hard rule is
341348
// evaluated fresh on every planning pass and takes precedence over merge (see planAgentMaintenanceActions),

0 commit comments

Comments
 (0)