From 4188798e3da057f96930885fe47b8ec5f022fe9a Mon Sep 17 00:00:00 2001 From: Andriy Polanski Date: Wed, 22 Jul 2026 22:55:44 +0000 Subject: [PATCH] feat(review): wire remaining configured gate blockers into signal tracking (#8104) Record RuleFiredEvent for every isConfiguredGateBlocker finding except linked_issue_scope_mismatch (#8101), and emit reversed HumanOverrideEvent on contributor reopen and owner reopen-then-merge when prior fires exist. Co-authored-by: Cursor --- src/queue/processors.ts | 6 + src/review/outcomes-wire.ts | 34 ++++ src/rules/advisory.ts | 63 +++++++ .../configured-gate-blocker-signals.test.ts | 167 ++++++++++++++++++ test/unit/outcomes-wire.test.ts | 116 +++++++++++- 5 files changed, 385 insertions(+), 1 deletion(-) create mode 100644 test/unit/configured-gate-blocker-signals.test.ts diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8d0ef51b7a..618fcf425c 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -195,6 +195,7 @@ import { buildIssueAdvisory, buildPullRequestAdvisory, evaluateGateCheck, + recordConfiguredGateBlockerSignals, resolveAiReviewLowConfidenceHold, } from "../rules/advisory"; import { hasValidationNote, isTestPath } from "../signals/test-evidence"; @@ -10283,6 +10284,11 @@ async function maybePublishPrPublicSurface( let evaluation = shouldEvaluateGate ? evaluateGateCheck(advisory, gatePolicy) : undefined; + // #8104: record RuleFiredEvent for every configured gate blocker except linked_issue_scope_mismatch + // (#8101). Same advisory+policy as evaluateGateCheck above so the filter stays in lock-step. + if (evaluation) { + await recordConfiguredGateBlockerSignals(env, advisory, gatePolicy, repoFullName, pr.number); + } // Deterministic content/registry surface lane (#1255) — flag-gated + per-repo allowlist, byte-identical when // off (evaluateWithSurfaceLane returns the generic evaluation unchanged and resolves no files). A metagraphed // registry-submission PR's surface verdict OVERRIDES the generic gate; the helper preserves a generic HARD diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 4a0cc16af7..8af7d8f92b 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -24,10 +24,15 @@ // once a repo's merge precision actually drops below the floor over a real sample. import { recordAuditEvent } from "../db/repositories"; +import { createSignalStore } from "./signal-tracking-wire"; import { tryEnqueueDecisionPackRebuild } from "../services/decision-pack"; import { incr } from "../selfhost/metrics"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import type { GitHubWebhookPayload } from "../types"; +import { + CONFIGURED_GATE_BLOCKER_SIGNAL_CODES, + CONFIGURED_GATE_BLOCKER_SIGNAL_LOOKBACK_MS, +} from "../rules/advisory"; import { errorMessage, nowIso } from "../utils/json"; import { applyAutoTune, @@ -447,6 +452,33 @@ async function hasRecentOwnerReopenPendingReversal(env: Env, targetKey: string, } } +// #8104: when a reversal is recorded for a target that any configured-gate-blocker rule (except +// linked_issue_scope_mismatch — #8101 owns that one) previously fired against, the human undoing of the bot +// action IS the human judgment on those findings. Fixed 30-day lookback; candidate codes come from +// CONFIGURED_GATE_BLOCKER_SIGNAL_CODES so the list cannot silently drift from isConfiguredGateBlocker. +// Callers attach `.catch(() => undefined)`: a SignalStore failure (including a queryRuleHistory read error, +// which deliberately propagates) must never affect whether the underlying reversal itself is recorded. +async function recordConfiguredGateBlockerOverrides(env: Env, targetId: string): Promise { + const store = createSignalStore(env); + const sinceMs = Date.now() - CONFIGURED_GATE_BLOCKER_SIGNAL_LOOKBACK_MS; + await Promise.all( + CONFIGURED_GATE_BLOCKER_SIGNAL_CODES.map(async (ruleId) => { + try { + const history = await store.queryRuleHistory(ruleId, sinceMs); + if (!history.fired.some((event) => event.targetKey === targetId)) return; + await store.recordHumanOverride({ + ruleId, + targetKey: targetId, + verdict: "reversed", + occurredAt: nowIso(), + }); + } catch { + // Fail-open per code: one SignalStore reject must not skip the rest of the candidate list. + } + }), + ); +} + /** * Record a REVERSAL — a human overriding a loopover auto-action — into the eval/audit stores (the * ground-truth accuracy signal). Mirrors reviewbot recordReversalSignals (runtime.ts ~157/274): @@ -510,6 +542,7 @@ export async function recordReversalSignals( detail: `Bot-closed PR #${pr.number} reopened by a contributor.`, metadata: { repoFullName, pullNumber: pr.number }, }).catch(() => undefined); + await recordConfiguredGateBlockerOverrides(env, targetId).catch(() => undefined); // #8104 return; } @@ -533,6 +566,7 @@ export async function recordReversalSignals( detail: `Bot-closed PR #${pr.number} reopened and merged by the repo owner.`, metadata: { repoFullName, pullNumber: pr.number }, }).catch(() => undefined); + await recordConfiguredGateBlockerOverrides(env, targetId).catch(() => undefined); // #8104 } const reverted = parseRevertedPrNumber(pr.body); if (!reverted) return; diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index e608bd300f..0ca64eb47e 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -37,6 +37,7 @@ import { nowIso } from "../utils/json"; import { LOOPOVER_GATE_CHECK_NAME } from "../review/check-names"; import { CLA_CHECK_UNRESOLVED_CODE, CLA_CONSENT_MISSING_CODE } from "../review/cla-check"; import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; +import { createSignalStore } from "../review/signal-tracking-wire"; import { labelMatchesPattern } from "../scoring/preview"; export type GateCheckConclusion = "success" | "failure" | "action_required" | "neutral" | "skipped"; @@ -164,6 +165,29 @@ export type GateCheckEvaluation = { // `ai_review_inconclusive` is deliberately EXCLUDED — that is a "could not review" HOLD, not a false defect. export const AI_JUDGMENT_BLOCKER_CODES = new Set(["ai_consensus_defect", "ai_review_split"]); +/** + * Every finding code `isConfiguredGateBlocker` can return true for, EXCEPT `linked_issue_scope_mismatch` + * (#8104). That one code is wired by #8101 at its own upstream push / reversal sites — including it here + * would double-count fired/reversed history. Keep this list in sync with `isConfiguredGateBlocker`'s body. + */ +export const CONFIGURED_GATE_BLOCKER_SIGNAL_CODES: readonly string[] = Object.freeze([ + "missing_linked_issue", + "duplicate_pr_risk", + ...AI_JUDGMENT_BLOCKER_CODES, + REVIEW_THREAD_BLOCKER_CODE, + "secret_leak", + "pre_merge_check_required", + "manifest_missing_tests", + "manifest_linked_issue_required", + "self_authored_linked_issue", + "content_lane_deliverable_missing", + "lockfile_tamper_risk", + CLA_CONSENT_MISSING_CODE, +]); + +/** Fixed lookback for reversal→HumanOverrideEvent pairing (#8104) — 30 days in milliseconds. */ +export const CONFIGURED_GATE_BLOCKER_SIGNAL_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; + /** True when the gate FAILED *solely* because of AI-judgment blockers (every blocker is an AI-judgment code). * An empty blocker list is NOT an AI-judgment-only failure. PURE. */ export function isAiJudgmentOnlyFailure(evaluation: GateCheckEvaluation): boolean { @@ -612,6 +636,10 @@ function evaluateGateCheckCore(advisoryResult: Advisory, policy: GateCheckPolicy // pass/fail. Readiness/quality stays advisory-only. const effective = applyMergeReadinessGate(policy); const configuredBlockers = advisoryResult.findings.filter((finding) => isConfiguredGateBlocker(finding, effective)); + // #8104: every configured blocker except linked_issue_scope_mismatch (#8101) records a RuleFiredEvent in + // the shared calibration module. evaluateGateCheckCore stays sync/pure (engine parity twin); the env-bearing + // caller awaits {@link recordConfiguredGateBlockerSignals} with the same advisory+policy so this filter and + // the recording loop stay in lock-step. const qualityWarning = buildQualityGateWarning(effective); const slopBlocker = buildSlopGateBlocker(effective); const blockers = [...configuredBlockers, ...(slopBlocker ? [slopBlocker] : [])]; @@ -1025,6 +1053,41 @@ function isConfiguredGateBlocker(finding: AdvisoryFinding, policy: GateCheckPoli return false; } +/** + * Record a {@link RuleFiredEvent} for every finding that `isConfiguredGateBlocker` would put into + * `configuredBlockers`, excluding `linked_issue_scope_mismatch` (#8104 / complements #8101). Call from the + * env-bearing gate path immediately after {@link evaluateGateCheck} with the SAME advisory + policy so the + * filter matches `evaluateGateCheckCore`'s own. Best-effort: a SignalStore failure never throws and never + * affects the gate verdict. + */ +export async function recordConfiguredGateBlockerSignals( + env: Env, + advisoryResult: Advisory, + policy: GateCheckPolicy, + repoFullName: string, + prNumber: number, +): Promise { + const effective = applyMergeReadinessGate(policy); + const configuredBlockers = advisoryResult.findings.filter((finding) => isConfiguredGateBlocker(finding, effective)); + const store = createSignalStore(env); + const targetKey = `${repoFullName}#${prNumber}`; + const occurredAt = nowIso(); + await Promise.all( + configuredBlockers.map((finding) => { + if (finding.code === "linked_issue_scope_mismatch") return Promise.resolve(); + return store + .recordRuleFired({ + ruleId: finding.code, + targetKey, + outcome: finding.severity ?? "blocker", + occurredAt, + ...(finding.confidence !== undefined ? { metadata: { confidence: finding.confidence } } : {}), + }) + .catch(() => undefined); + }), + ); +} + function buildQualityGateWarning(policy: GateCheckPolicy): AdvisoryFinding | null { if (gateMode(policy.qualityGateMode) === "off") return null; const score = normalizeScore(policy.readinessScore); diff --git a/test/unit/configured-gate-blocker-signals.test.ts b/test/unit/configured-gate-blocker-signals.test.ts new file mode 100644 index 0000000000..912e3ba90c --- /dev/null +++ b/test/unit/configured-gate-blocker-signals.test.ts @@ -0,0 +1,167 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + recordConfiguredGateBlockerSignals, + type GateCheckPolicy, +} from "../../src/rules/advisory"; +import * as signalTrackingWire from "../../src/review/signal-tracking-wire"; +import { createSignalStore } from "../../src/review/signal-tracking-wire"; +import type { Advisory, AdvisoryFinding } from "../../src/types"; +import { createTestEnv } from "../helpers/d1"; + +function finding(over: Partial & Pick): AdvisoryFinding { + return { + title: over.title ?? over.code, + severity: over.severity ?? "warning", + detail: over.detail ?? `${over.code} detail`, + action: over.action ?? "fix it", + ...over, + }; +} + +function advisory(findings: AdvisoryFinding[]): Advisory { + return { + id: "advisory-8104", + targetType: "pull_request", + targetKey: "owner/repo#7", + repoFullName: "owner/repo", + pullNumber: 7, + headSha: "abc", + conclusion: "neutral", + severity: "warning", + title: "advisory", + summary: `${findings.length} finding(s)`, + findings, + generatedAt: "2026-07-22T00:00:00.000Z", + }; +} + +const blockAi: GateCheckPolicy = { aiReviewGateMode: "block" }; +const blockLinked: GateCheckPolicy = { linkedIssueGateMode: "block" }; +const blockSatisfaction: GateCheckPolicy = { linkedIssueSatisfactionGateMode: "block" }; + +describe("recordConfiguredGateBlockerSignals (#8104)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("records a fired signal for ai_consensus_defect when it is a configured gate blocker", async () => { + const env = createTestEnv(); + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "ai_consensus_defect", confidence: 0.95 })]), + blockAi, + "owner/repo", + 7, + ); + const history = await createSignalStore(env).queryRuleHistory("ai_consensus_defect", 0); + expect(history.fired).toHaveLength(1); + expect(history.fired[0]).toMatchObject({ + ruleId: "ai_consensus_defect", + targetKey: "owner/repo#7", + outcome: "warning", + metadata: { confidence: 0.95 }, + }); + }); + + it("records a fired signal for ai_review_split when it is a configured gate blocker", async () => { + const env = createTestEnv(); + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "ai_review_split", severity: "critical" })]), + blockAi, + "owner/repo", + 7, + ); + const history = await createSignalStore(env).queryRuleHistory("ai_review_split", 0); + expect(history.fired).toHaveLength(1); + expect(history.fired[0]).toMatchObject({ + ruleId: "ai_review_split", + targetKey: "owner/repo#7", + outcome: "critical", + }); + expect(history.fired[0]?.metadata).toBeUndefined(); + }); + + it("records a fired signal for a deterministic code (secret_leak)", async () => { + const env = createTestEnv(); + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "secret_leak", severity: "critical" })]), + {}, + "owner/repo", + 7, + ); + const history = await createSignalStore(env).queryRuleHistory("secret_leak", 0); + expect(history.fired).toHaveLength(1); + expect(history.fired[0]).toMatchObject({ + ruleId: "secret_leak", + targetKey: "owner/repo#7", + outcome: "critical", + }); + }); + + it("records a fired signal for missing_linked_issue when linkedIssueGateMode is block", async () => { + const env = createTestEnv(); + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "missing_linked_issue" })]), + blockLinked, + "owner/repo", + 7, + ); + expect((await createSignalStore(env).queryRuleHistory("missing_linked_issue", 0)).fired).toHaveLength(1); + }); + + it("records NO fired signal for linked_issue_scope_mismatch even when it is a configured blocker (#8101 owns it)", async () => { + const env = createTestEnv(); + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "linked_issue_scope_mismatch" }), finding({ code: "secret_leak", severity: "critical" })]), + blockSatisfaction, + "owner/repo", + 7, + ); + expect((await createSignalStore(env).queryRuleHistory("linked_issue_scope_mismatch", 0)).fired).toEqual([]); + expect((await createSignalStore(env).queryRuleHistory("secret_leak", 0)).fired).toHaveLength(1); + }); + + it("records NO fired signal when isConfiguredGateBlocker returns false", async () => { + const env = createTestEnv(); + // missing_linked_issue defaults to advisory — not a configured blocker. + await recordConfiguredGateBlockerSignals( + env, + advisory([finding({ code: "missing_linked_issue" })]), + { linkedIssueGateMode: "advisory" }, + "owner/repo", + 7, + ); + expect((await createSignalStore(env).queryRuleHistory("missing_linked_issue", 0)).fired).toEqual([]); + }); + + it("uses outcome 'blocker' when finding.severity is missing (nullish coalescing arm)", async () => { + const env = createTestEnv(); + const noSeverity = finding({ code: "secret_leak" }); + delete (noSeverity as { severity?: AdvisoryFinding["severity"] }).severity; + await recordConfiguredGateBlockerSignals(env, advisory([noSeverity]), {}, "owner/repo", 7); + expect((await createSignalStore(env).queryRuleHistory("secret_leak", 0)).fired[0]?.outcome).toBe("blocker"); + }); + + it("degrades silently when the SignalStore write rejects: nothing throws", async () => { + vi.spyOn(signalTrackingWire, "createSignalStore").mockReturnValue({ + recordRuleFired: async () => { + throw new Error("signal store down"); + }, + recordHumanOverride: async () => undefined, + queryRuleHistory: async () => ({ fired: [], overrides: [] }), + }); + await expect( + recordConfiguredGateBlockerSignals( + createTestEnv(), + advisory([finding({ code: "secret_leak", severity: "critical" })]), + {}, + "owner/repo", + 7, + ), + ).resolves.toBeUndefined(); + }); +}); diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index b363f38d47..8e89818a6a 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { processJob } from "../../src/queue/processors"; import { createFlagStore, @@ -21,6 +21,8 @@ import { recordAuditEvent } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import type { GitHubPullRequestPayload } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; +import * as signalTrackingWire from "../../src/review/signal-tracking-wire"; +import { createSignalStore } from "../../src/review/signal-tracking-wire"; // ── helpers ──────────────────────────────────────────────────────────────────────────────────────────────── @@ -1318,3 +1320,115 @@ describe("resolveDispositionReason (enriched Discord reason)", () => { ).toBe("fallback"); }); }); + +// ── #8104: remaining configured-gate-blocker reversal-override wiring ──────────────────────────────────────── + +describe("recordReversalSignals — configured gate blocker overrides (#8104)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + async function seedFired(env: Env, ruleId: string, targetKey: string): Promise { + await createSignalStore(env).recordRuleFired({ + ruleId, + targetKey, + outcome: "warning", + occurredAt: new Date().toISOString(), + }); + } + + function contributorReopen(number = 7) { + return { + action: "reopened", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number, state: "open" }), + sender: { login: "contributor", type: "User" }, + }; + } + + it("records 'reversed' overrides for every non-excluded code that previously fired on the target", async () => { + const env = createTestEnv(); + await seedBotAction(env, "owner/repo#7", "close"); + await seedFired(env, "ai_consensus_defect", "owner/repo#7"); + await seedFired(env, "secret_leak", "owner/repo#7"); + await seedFired(env, "ai_review_split", "owner/repo#99"); // different target — ignored + + await recordReversalSignals(env, "pull_request", contributorReopen()); + + const defect = await createSignalStore(env).queryRuleHistory("ai_consensus_defect", 0); + const secret = await createSignalStore(env).queryRuleHistory("secret_leak", 0); + const split = await createSignalStore(env).queryRuleHistory("ai_review_split", 0); + expect(defect.overrides).toHaveLength(1); + expect(defect.overrides[0]).toMatchObject({ + ruleId: "ai_consensus_defect", + targetKey: "owner/repo#7", + verdict: "reversed", + }); + expect(secret.overrides).toHaveLength(1); + expect(secret.overrides[0]).toMatchObject({ ruleId: "secret_leak", targetKey: "owner/repo#7", verdict: "reversed" }); + expect(split.overrides).toEqual([]); + }); + + it("records 'reversed' overrides on the owner reopen-then-merge path when prior fires exist", async () => { + const env = createTestEnv(); + await seedBotAction(env, "owner/repo#7", "close"); + await seedFired(env, "missing_linked_issue", "owner/repo#7"); + await seedFired(env, "duplicate_pr_risk", "owner/repo#7"); + + await recordReversalSignals(env, "pull_request", { + action: "reopened", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number: 7, state: "open" }), + sender: { login: "owner", type: "User" }, + }); + expect((await createSignalStore(env).queryRuleHistory("missing_linked_issue", 0)).overrides).toEqual([]); + + await recordReversalSignals(env, "pull_request", { + action: "closed", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number: 7, state: "closed", merged_at: new Date().toISOString() }), + sender: { login: "owner", type: "User" }, + }); + + expect((await createSignalStore(env).queryRuleHistory("missing_linked_issue", 0)).overrides).toHaveLength(1); + expect((await createSignalStore(env).queryRuleHistory("duplicate_pr_risk", 0)).overrides).toHaveLength(1); + }); + + it("records NO override when the reversal target has no prior fired event", async () => { + const env = createTestEnv(); + await seedBotAction(env, "owner/repo#7", "close"); + await seedFired(env, "secret_leak", "owner/repo#99"); + + await recordReversalSignals(env, "pull_request", contributorReopen()); + + expect((await createSignalStore(env).queryRuleHistory("secret_leak", 0)).overrides).toEqual([]); + expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(1); + }); + + it("never records a linked_issue_scope_mismatch override from the #8104 loop (owned by #8101)", async () => { + const env = createTestEnv(); + await seedBotAction(env, "owner/repo#7", "close"); + await seedFired(env, "linked_issue_scope_mismatch", "owner/repo#7"); + await seedFired(env, "secret_leak", "owner/repo#7"); + + await recordReversalSignals(env, "pull_request", contributorReopen()); + + expect((await createSignalStore(env).queryRuleHistory("linked_issue_scope_mismatch", 0)).overrides).toEqual([]); + expect((await createSignalStore(env).queryRuleHistory("secret_leak", 0)).overrides).toHaveLength(1); + }); + + it("degrades silently when the SignalStore read rejects: the reversal itself still records", async () => { + const env = createTestEnv(); + await seedBotAction(env, "owner/repo#7", "close"); + vi.spyOn(signalTrackingWire, "createSignalStore").mockReturnValue({ + recordRuleFired: async () => undefined, + recordHumanOverride: async () => undefined, + queryRuleHistory: async () => { + throw new Error("signal store down"); + }, + }); + + await expect(recordReversalSignals(env, "pull_request", contributorReopen())).resolves.toBeUndefined(); + expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(1); + }); +});