diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index a59745e2df..662bdefa60 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -25,6 +25,7 @@ import { recordAuditEvent } from "../db/repositories"; import { createSignalStore } from "./signal-tracking-wire"; +import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; import { tryEnqueueDecisionPackRebuild } from "../services/decision-pack"; import { incr } from "../selfhost/metrics"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; @@ -505,6 +506,30 @@ async function recordLinkedIssueScopeMismatchOverride(env: Env, targetId: string }); } +// #8123 (implements #8106's decision): the repo OWNER closing (not merging) a PR that was held for a +// low-confidence AI judgment is the explicit "the automated call was right" signal — the confirmed-side +// mirror of the reversal hooks above, and the first non-inferred positive confirmation in this system. +// "Held via aiReviewLowConfidenceHold" is detected the same way #8101 detects its rule: a recorded +// rule_fired event for either AI-judgment code (ai_consensus_defect / ai_review_split) against this target +// within the fixed lookback (#8104's own 30-day constant). Scoped to those two codes only — the other two +// hold kinds carry no ruleId-equivalent to key on (see the issue's Boundaries). Callers attach +// `.catch(() => undefined)`: a SignalStore failure must never affect the underlying PR-close handling. +const AI_JUDGMENT_CONFIRMATION_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; + +async function recordAiJudgmentHoldConfirmations(env: Env, targetId: string): Promise { + const store = createSignalStore(env); + for (const code of AI_JUDGMENT_BLOCKER_CODES) { + const history = await store.queryRuleHistory(code, Date.now() - AI_JUDGMENT_CONFIRMATION_LOOKBACK_MS); + if (!history.fired.some((event) => event.targetKey === targetId)) continue; + await store.recordHumanOverride({ + ruleId: code, + targetKey: targetId, + verdict: "confirmed", + occurredAt: nowIso(), + }); + } +} + /** * 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): @@ -573,6 +598,19 @@ export async function recordReversalSignals( return; } + // #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). + 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); + } + return; + } + // A merge — either it completes an owner's earlier rescue of a bot-closed PR (#7985), or it's a "Reverts // #N" PR undoing a DIFFERENT bot-merged PR. Both can apply to the SAME merge (a rescue is never also a // revert of itself — they key off different target PRs — so there is no double-counting risk). diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 36602d5708..5f42df71cd 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -1521,3 +1521,121 @@ describe("recordReversalSignals — linked_issue_scope_mismatch override (#8101) vi.restoreAllMocks(); }); }); + +// ── #8123: owner-close of an AI-judgment-held PR → "confirmed" override ───────────────────────────────────── + +describe("recordReversalSignals — aiReviewLowConfidenceHold confirmation (#8123)", () => { + async function seedAiFired(env: Env, ruleId: string, targetKey = "owner/repo#9"): Promise { + await createSignalStore(env).recordRuleFired({ + ruleId, + targetKey, + outcome: "warning", + occurredAt: new Date().toISOString(), + metadata: { confidence: 0.4 }, + }); + } + + 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("records a 'confirmed' override for the AI ruleId that fired when the OWNER closes the held PR without merging", async () => { + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect"); + + await recordReversalSignals(env, "pull_request", ownerClose()); + + const overrides = await overridesFor(env, "ai_consensus_defect"); + expect(overrides).toHaveLength(1); + expect(overrides[0]).toMatchObject({ ruleId: "ai_consensus_defect", targetKey: "owner/repo#9", verdict: "confirmed" }); + }); + + it("confirms every AI-judgment code that fired for the target, in the same close", async () => { + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect"); + await seedAiFired(env, "ai_review_split"); + + await recordReversalSignals(env, "pull_request", ownerClose()); + + expect(await overridesFor(env, "ai_consensus_defect")).toHaveLength(1); + expect(await overridesFor(env, "ai_review_split")).toHaveLength(1); + }); + + it("records nothing when a CONTRIBUTOR closes the same held PR — owner-only, like the reversal side", async () => { + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect"); + + await recordReversalSignals(env, "pull_request", ownerClose(9, { sender: { login: "contributor", type: "User" } })); + + 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 () => { + const env = createTestEnv(); + await seedAiFired(env, "secret_leak"); // a non-AI code firing does not make this an AI hold + + await recordReversalSignals(env, "pull_request", ownerClose()); + + 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); + }); + + it("records nothing for a fired event on a DIFFERENT target", async () => { + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect", "owner/repo#999"); + + await recordReversalSignals(env, "pull_request", ownerClose()); + + expect(await overridesFor(env, "ai_consensus_defect")).toHaveLength(0); + }); + + it("does not treat an owner MERGE-close as a confirmation (the merged path is the reversal side's business)", async () => { + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect"); + + await recordReversalSignals( + env, + "pull_request", + ownerClose(9, { pull_request: pullRequestPayload({ number: 9, state: "closed", merged_at: "2026-07-22T23:00:00Z" }) }), + ); + + expect(await overridesFor(env, "ai_consensus_defect")).toHaveLength(0); + }); + + it("records nothing when the sender is missing or the repo full name has an empty owner segment", async () => { + // Both degenerate-webhook arms of the owner check: a payload with no sender at all, and a repository + // full_name whose owner segment is empty — neither can ever equal a real owner login, so neither may + // count as an owner confirmation. + const env = createTestEnv(); + await seedAiFired(env, "ai_consensus_defect"); + await recordReversalSignals(env, "pull_request", ownerClose(9, { sender: undefined })); + expect(await overridesFor(env, "ai_consensus_defect")).toEqual([]); + await seedAiFired(env, "ai_consensus_defect", "/repo#9"); + await recordReversalSignals(env, "pull_request", ownerClose(9, { repository: { name: "repo", full_name: "/repo", owner: { login: "" } }, sender: { login: "", type: "User" } })); + expect(await overridesFor(env, "ai_consensus_defect")).toEqual([]); + }); + + it("degrades silently when the SignalStore rejects — the close handling itself never throws", async () => { + const env = createTestEnv(); + 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", ownerClose())).resolves.toBeUndefined(); + }); +});