diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index e3ef053815..135f033682 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -34,6 +34,7 @@ import type { GitHubWebhookPayload } from "../types"; import { CONFIGURED_GATE_BLOCKER_SIGNAL_CODES, CONFIGURED_GATE_BLOCKER_SIGNAL_LOOKBACK_MS, + GATE_SCORE_SIGNAL_CODES, } from "../rules/advisory"; import { errorMessage, nowIso } from "../utils/json"; import { @@ -572,6 +573,51 @@ async function recordAiJudgmentHoldConfirmations(env: Env, targetId: string): Pr } } +// #8761: implicit terminal-disposition confirmations for every OTHER rule — the "confirmed" mirror the +// non-AI codes never had (pre-#8761 only reversals existed for them, structurally biasing their measured +// precision toward 0 as data accumulated). Terminal-event mapping, documented per #8761's Requirements: +// the repo OWNER closing a PR WITHOUT merging it is the one human disposition CONSISTENT WITH an adverse +// finding's verdict — the same signal #8123 already treats as confirmation for the two AI-judgment codes. +// Deliberately EXCLUDED from this mapping: +// • AI_JUDGMENT_BLOCKER_CODES — recordAiJudgmentHoldConfirmations above owns them (the explicit, +// untagged confirmation; #8123's established semantics stay byte-identical). +// • GATE_SCORE_SIGNAL_CODES — score signals fire on PASS outcomes too (above/below threshold alike, so +// the corpus can replay thresholds), so "owner closed ⇒ the rule was right" is incoherent for them; +// their ground truth comes from threshold replays, never terminal dispositions. +// • a merge-despite-advisory-finding — weak evidence AGAINST the finding (a reversal-shaped signal), +// out of #8761's scope by design. +// Idempotent: any existing override (either verdict) for the (ruleId, target) pair suppresses the write — +// a pair stays single-verdict, so a reopen-recorded reversal is never followed by a contradicting implicit +// confirmation. Every write is tagged metadata.basis = "implicit_terminal_disposition" so downstream +// consumers can weight (or exclude) implicit evidence separately from #8123's explicit kind. Fail-open per +// code; callers attach `.catch(() => undefined)` like every writer in this file. +const IMPLICIT_CONFIRMATION_BASIS = "implicit_terminal_disposition"; +const IMPLICIT_CONFIRMATION_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; + +async function recordImplicitTerminalConfirmations(env: Env, targetId: string): Promise { + const store = createSignalStore(env); + const excluded = new Set([...AI_JUDGMENT_BLOCKER_CODES, ...GATE_SCORE_SIGNAL_CODES]); + const codes = [...CONFIGURED_GATE_BLOCKER_SIGNAL_CODES.filter((code) => !excluded.has(code)), LINKED_ISSUE_SCOPE_MISMATCH_RULE_ID]; + await Promise.all( + codes.map(async (ruleId) => { + try { + const history = await store.queryRuleHistory(ruleId, Date.now() - IMPLICIT_CONFIRMATION_LOOKBACK_MS); + if (!history.fired.some((event) => event.targetKey === targetId)) return; + if (history.overrides.some((event) => event.targetKey === targetId)) return; // single-verdict pair + await store.recordHumanOverride({ + ruleId, + targetKey: targetId, + verdict: "confirmed", + occurredAt: nowIso(), + metadata: { basis: IMPLICIT_CONFIRMATION_BASIS }, + }); + } 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): @@ -643,12 +689,16 @@ export async function recordReversalSignals( // #8123: the OWNER closing a PR WITHOUT merging it — when that PR was held for a low-confidence AI // judgment, the owner's close is the explicit confirmation the finding was right ("confirmed" override). // A contributor's own close is not a confirmation signal and records nothing (mirrors the reversal side's - // owner-vs-contributor distinction above). + // owner-vs-contributor distinction above). #8761 extends the same terminal disposition to every other + // fired rule as an IMPLICIT confirmation (basis-tagged, idempotent — see + // recordImplicitTerminalConfirmations' own doc comment for the mapping and its exclusions). if (payload.action === "closed" && !pr.merged_at) { const ownerLogin = (repoFullName.split("/")[0] || "").toLowerCase(); const senderLogin = (payload.sender?.login || "").toLowerCase(); if (!!ownerLogin && !!senderLogin && ownerLogin === senderLogin) { - await recordAiJudgmentHoldConfirmations(env, reviewAuditTargetId(repoFullName, pr.number)).catch(() => undefined); + const targetId = reviewAuditTargetId(repoFullName, pr.number); + await recordAiJudgmentHoldConfirmations(env, targetId).catch(() => undefined); + await recordImplicitTerminalConfirmations(env, targetId).catch(() => undefined); // #8761 } return; } diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index cce3eaeffe..2e760b121e 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -197,6 +197,9 @@ export const CONFIGURED_GATE_BLOCKER_SIGNAL_CODES: readonly string[] = Object.fr "manifest_linked_issue_required", "self_authored_linked_issue", "content_lane_deliverable_missing", + // #8761 drift fix: backtest_regression (#8138) was added to the gate switch but never to this list, so its + // reversals silently never recorded — exactly the drift this list's own doc comment promises against. + "backtest_regression", "lockfile_tamper_risk", CLA_CONSENT_MISSING_CODE, ...GATE_SCORE_SIGNAL_CODES, diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 8cbde1b201..45982b2a70 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as signalTrackingWire from "../../src/review/signal-tracking-wire"; import { createSignalStore } from "../../src/review/signal-tracking-wire"; import { processJob } from "../../src/queue/processors"; @@ -1698,7 +1698,7 @@ describe("recordReversalSignals — aiReviewLowConfidenceHold confirmation (#812 expect(await overridesFor(env, "ai_consensus_defect")).toHaveLength(0); }); - it("records nothing when the PR was not AI-judgment-held (no matching fired event, other rules ignored)", async () => { + it("records no AI-code override when the PR was not AI-judgment-held; a fired non-AI code gets #8761's IMPLICIT confirmation instead", async () => { const env = createTestEnv(); await seedAiFired(env, "secret_leak"); // a non-AI code firing does not make this an AI hold @@ -1706,7 +1706,11 @@ describe("recordReversalSignals — aiReviewLowConfidenceHold confirmation (#812 expect(await overridesFor(env, "ai_consensus_defect")).toHaveLength(0); expect(await overridesFor(env, "ai_review_split")).toHaveLength(0); - expect(await overridesFor(env, "secret_leak")).toHaveLength(0); + // #8761: the owner's terminal close IS consistent with the fired secret_leak verdict — implicit confirm. + const secretOverrides = await overridesFor(env, "secret_leak"); + expect(secretOverrides).toHaveLength(1); + expect(secretOverrides[0]).toMatchObject({ verdict: "confirmed" }); + expect((secretOverrides[0]!.metadata as { basis: string }).basis).toBe("implicit_terminal_disposition"); }); it("records nothing for a fired event on a DIFFERENT target", async () => { @@ -1757,3 +1761,119 @@ describe("recordReversalSignals — aiReviewLowConfidenceHold confirmation (#812 await expect(recordReversalSignals(env, "pull_request", ownerClose())).resolves.toBeUndefined(); }); }); + +// ── #8761: implicit terminal-disposition confirmations for the non-AI rules ───────────────────────────────── + +describe("recordReversalSignals — implicit terminal confirmations (#8761)", () => { + // The preceding describe's last test spies createSignalStore and (deliberately) never restores inside its + // own block — restore here so these tests always exercise the real D1-backed store. + beforeEach(() => { + vi.restoreAllMocks(); + }); + + async function seedFired(env: Env, ruleId: string, targetKey = "owner/repo#9"): Promise { + await createSignalStore(env).recordRuleFired({ ruleId, targetKey, outcome: "blocker", occurredAt: new Date().toISOString() }); + } + + function ownerClose(number = 9, overrides: Record = {}) { + return { + action: "closed", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number, state: "closed", merged_at: null }), + sender: { login: "owner", type: "User" }, + ...overrides, + }; + } + + async function overridesFor(env: Env, ruleId: string) { + return (await createSignalStore(env).queryRuleHistory(ruleId, 0)).overrides; + } + + it("confirms a fired non-AI rule on an owner close-without-merge, tagged basis=implicit_terminal_disposition", async () => { + const env = createTestEnv(); + await seedFired(env, "missing_linked_issue"); + await recordReversalSignals(env, "pull_request", ownerClose()); + const overrides = await overridesFor(env, "missing_linked_issue"); + expect(overrides).toHaveLength(1); + expect(overrides[0]).toMatchObject({ ruleId: "missing_linked_issue", targetKey: "owner/repo#9", verdict: "confirmed" }); + expect((overrides[0]!.metadata as { basis: string }).basis).toBe("implicit_terminal_disposition"); + }); + + it("includes linked_issue_scope_mismatch — its first-ever confirmed side (#8101 wired reversals only)", async () => { + const env = createTestEnv(); + await seedFired(env, "linked_issue_scope_mismatch"); + await recordReversalSignals(env, "pull_request", ownerClose()); + const overrides = await overridesFor(env, "linked_issue_scope_mismatch"); + expect(overrides).toHaveLength(1); + expect(overrides[0]).toMatchObject({ verdict: "confirmed" }); + }); + + it("IDEMPOTENT: an existing override (either verdict) suppresses the implicit confirmation — a pair stays single-verdict", async () => { + const env = createTestEnv(); + await seedFired(env, "missing_linked_issue"); + await createSignalStore(env).recordHumanOverride({ + ruleId: "missing_linked_issue", + targetKey: "owner/repo#9", + verdict: "reversed", + occurredAt: new Date().toISOString(), + }); + await recordReversalSignals(env, "pull_request", ownerClose()); + const overrides = await overridesFor(env, "missing_linked_issue"); + expect(overrides).toHaveLength(1); + expect(overrides[0]).toMatchObject({ verdict: "reversed" }); + }); + + it("IDEMPOTENT: a second owner close records nothing new — the first confirmation is the pair's only verdict", async () => { + const env = createTestEnv(); + await seedFired(env, "duplicate_pr_risk"); + await recordReversalSignals(env, "pull_request", ownerClose()); + await recordReversalSignals(env, "pull_request", ownerClose()); + expect(await overridesFor(env, "duplicate_pr_risk")).toHaveLength(1); + }); + + it("EXCLUDES the score-signal codes — they fire on pass outcomes too, so close-confirmation is incoherent for them", async () => { + const env = createTestEnv(); + await seedFired(env, "slop_gate_score"); + await seedFired(env, "quality_gate_score"); + await recordReversalSignals(env, "pull_request", ownerClose()); + expect(await overridesFor(env, "slop_gate_score")).toHaveLength(0); + expect(await overridesFor(env, "quality_gate_score")).toHaveLength(0); + }); + + it("leaves the AI-judgment codes to their existing explicit writer — exactly one confirmation, never a basis tag", async () => { + const env = createTestEnv(); + await createSignalStore(env).recordRuleFired({ + ruleId: "ai_consensus_defect", + targetKey: "owner/repo#9", + outcome: "warning", + occurredAt: new Date().toISOString(), + metadata: { confidence: 0.4 }, + }); + await recordReversalSignals(env, "pull_request", ownerClose()); + const overrides = await overridesFor(env, "ai_consensus_defect"); + expect(overrides).toHaveLength(1); + expect(overrides[0]!.metadata?.basis).toBeUndefined(); + }); + + it("covers backtest_regression via the drift-fixed signal-codes list (#8761's list fix)", async () => { + const env = createTestEnv(); + await seedFired(env, "backtest_regression"); + await recordReversalSignals(env, "pull_request", ownerClose()); + expect(await overridesFor(env, "backtest_regression")).toHaveLength(1); + }); + + it("records nothing for a fired non-AI rule when a CONTRIBUTOR closes — owner-only, same guard as #8123", async () => { + const env = createTestEnv(); + await seedFired(env, "missing_linked_issue"); + await recordReversalSignals(env, "pull_request", ownerClose(9, { sender: { login: "contributor", type: "User" } })); + expect(await overridesFor(env, "missing_linked_issue")).toHaveLength(0); + }); + + it("degrades silently when createSignalStore itself throws — the close handling never throws (both confirm writers' outer catch)", async () => { + const env = createTestEnv(); + vi.spyOn(signalTrackingWire, "createSignalStore").mockImplementation(() => { + throw new Error("store construction failed"); + }); + await expect(recordReversalSignals(env, "pull_request", ownerClose())).resolves.toBeUndefined(); + }); +});