Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ import {
import {
isCloseHoldOnly,
isHoldOnly,
readUntrustworthyRuleCodes,
recordPrOutcome,
recordReversalSignals,
} from "../review/outcomes-wire";
Expand Down Expand Up @@ -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<string> = 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
Expand Down Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions src/review/outcomes-wire.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -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<ReadonlySet<string>> {
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<void> {
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
Expand Down Expand Up @@ -766,6 +807,16 @@ export async function runSelfTuneBreaker(env: Env): Promise<void> {

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({
Expand Down
13 changes: 10 additions & 3 deletions src/services/agent-approval-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down
Loading