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
38 changes: 38 additions & 0 deletions src/review/outcomes-wire.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<void> {
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):
Expand Down Expand Up @@ -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).
Expand Down
118 changes: 118 additions & 0 deletions test/unit/outcomes-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
await createSignalStore(env).recordRuleFired({
ruleId,
targetKey,
outcome: "warning",
occurredAt: new Date().toISOString(),
metadata: { confidence: 0.4 },
});
}

function ownerClose(number = 9, overrides: Record<string, unknown> = {}) {
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();
});
});