Skip to content

Commit a6796f1

Browse files
committed
fix(agent-actions): replace blanket concrete-evidence breaker exemption with per-rule track record
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 01d2b5a commit a6796f1

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
@@ -631,6 +631,7 @@ import {
631631
import {
632632
isCloseHoldOnly,
633633
isHoldOnly,
634+
readUntrustworthyRuleCodes,
634635
recordPrOutcome,
635636
recordReversalSignals,
636637
} from "../review/outcomes-wire";
@@ -2131,18 +2132,23 @@ async function resolveLiveMigrationCollisionHold(
21312132
* downgrades), in order. PURE — the live flag reads happen at the call site (each fail-open), so this composes
21322133
* only the transforms:
21332134
* • holdOnly → downgradeMergeToHold (would-MERGE → human HOLD), else passthrough.
2134-
* • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt), else passthrough.
2135-
* Both off (the common path) returns the plan byte-identically. The breakers don't interfere: the merge
2136-
* downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`.
2135+
* • closeHoldOnly → downgradeCloseToHold (HEURISTIC would-CLOSE → human HOLD; deterministic close exempt).
2136+
* `untrustworthyRuleCodes` (#7986) is ALWAYS passed to downgradeCloseToHold, even when `closeHoldOnly` is
2137+
* false — that function is internally self-gating (a no-op unless something is actually downgradable either
2138+
* via the project flag or a per-rule match), so this stays byte-identical to before #7986 whenever the set is
2139+
* empty (the default) or nothing matches. Both `holdOnly`/`closeHoldOnly` off AND an empty
2140+
* `untrustworthyRuleCodes` (the common path) returns the plan byte-identically. The breakers don't interfere:
2141+
* the merge downgrade only touches `merge`/ready-label, the close downgrade only touches a heuristic `close`.
21372142
*/
21382143
export function applyPrecisionBreakers(
21392144
planned: PlannedAgentAction[],
21402145
holdOnly: boolean,
21412146
closeHoldOnly: boolean,
21422147
labelSettings: AgentDispositionLabelSettings = {},
2148+
untrustworthyRuleCodes: ReadonlySet<string> = new Set(),
21432149
): PlannedAgentAction[] {
21442150
const afterMerge = holdOnly ? downgradeMergeToHold(planned, true, labelSettings) : planned;
2145-
return closeHoldOnly ? downgradeCloseToHold(afterMerge, true, labelSettings) : afterMerge;
2151+
return downgradeCloseToHold(afterMerge, closeHoldOnly, labelSettings, untrustworthyRuleCodes);
21462152
}
21472153

21482154
/** PURE: which precision-breaker directions actually rewrote the plan — i.e. `planned` had a merge/close that
@@ -3182,6 +3188,9 @@ async function runAgentMaintenancePlanAndExecute(
31823188
migrationCollisionLabel: settings.migrationCollisionLabel,
31833189
pendingClosureLabel: settings.pendingClosureLabel,
31843190
},
3191+
// #7986: a cheap, cron-refreshed single-row read (readUntrustworthyRuleCodes) — never a fresh aggregate
3192+
// query on the hot webhook path. Fail-open (empty set) on any read error, same as isHoldOnly/isCloseHoldOnly.
3193+
await readUntrustworthyRuleCodes(env),
31853194
);
31863195
// Observability (#terminal-outcome-audit): a bounded-cardinality counter (direction only — no repo/PR/reason
31873196
// 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
@@ -41,6 +41,7 @@ import {
4141
} from "./auto-tune";
4242
import { computeGateEval } from "./parity";
4343
import { LOOPOVER_NATIVE_SOURCE } from "./parity-wire";
44+
import { computeBlendedRuleGateEval, rulesBelowClosePrecisionFloor } from "./rule-gate-eval";
4445

4546
/** PURE: parse the PR number an "Reverts #N / Reverts owner/repo#N" body refers to (GitHub's revert PRs).
4647
* Mirrors reviewbot runtime.ts parseRevertedPrNumber. Returns undefined when the body isn't a revert. */
@@ -219,6 +220,46 @@ export function createFlagStore(env: Env): FlagStore {
219220
};
220221
}
221222

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

224265
/** 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<void> {
766807

767808
await runBreakerPassForReport(flags, plainPass.report, plainPass.engagedHoldonly, plainPass.engagedClosehold, nowMs, "");
768809
await runBreakerPassForReport(flags, minerPass.report, minerPass.engagedHoldonly, minerPass.engagedClosehold, nowMs, "miner_");
810+
811+
// #7986: refresh the per-rule track-record cache the concrete-evidence breaker exemption reads
812+
// (readUntrustworthyRuleCodes) -- SAME window, pooled cross-project (a rule's trustworthiness is a
813+
// property of the rule, not of any one repo it happened to trip). Independent of the two passes above:
814+
// a failure here must not prevent (and does not roll back) the merge/close breaker engagement that just
815+
// completed -- computeBlendedRuleGateEval and writeUntrustworthyRuleCodes are both already fail-safe on
816+
// their own, so no extra try/catch is needed beyond this function's own outer one.
817+
const ruleReport = await computeBlendedRuleGateEval(env, { days: BREAKER_EVAL_WINDOW_DAYS, nowMs, source: LOOPOVER_NATIVE_SOURCE });
818+
const untrustworthyCodes = rulesBelowClosePrecisionFloor(ruleReport.rows).map((row) => row.ruleCode);
819+
await writeUntrustworthyRuleCodes(env, untrustworthyCodes);
769820
} catch (error) {
770821
console.warn(
771822
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)