Skip to content

fix(review): count owner reopen-then-merge as a real reversal signal #8074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 22, 2026
Merged

fix(review): count owner reopen-then-merge as a real reversal signal #8074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
76 changes: 70 additions & 6 deletions src/review/outcomes-wire.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,43 @@ async function wasMergeRecorded(env: Env, targetId: string): Promise<boolean> {
}
}

// #7985: a bare owner reopen of a bot-closed PR is still ambiguous on its own (could be a genuine
// administrative re-queue rather than "the bot was wrong"), but an owner reopen followed by an approve/merge
// within a short window is unambiguous — the owner looked at it again and decided it was right after all.
// This is exactly the pattern that left the 2026-07-21/22 metagraphed incidents (#7469/#7589/#7591/#7594)
// invisible to reversalRate/the public accuracy metric: every one of that day's maintainer-driven rescues was
// a bot-close reopened and merged by the repo owner within minutes, and the old unconditional owner-reopen
// exclusion recorded nothing for any of them.
const OWNER_REOPEN_PENDING_EVENT_TYPE = "owner_reopen_pending_reversal";
const OWNER_REOPEN_MERGE_WINDOW_MS = 6 * 60 * 60 * 1000;

/** True when this target has an `owner_reopen_pending_reversal` marker (written by the "reopened" branch
* below) within the last `windowMs` — i.e. the repo owner reopened a bot-closed PR recently enough that a
* merge happening NOW plausibly completes that same correction, not an unrelated later action. Fail-safe: a
* read error → false (record nothing rather than a false reversal). */
async function hasRecentOwnerReopenPendingReversal(env: Env, targetKey: string, windowMs: number): Promise<boolean> {
try {
const sinceIso = new Date(Date.now() - windowMs).toISOString();
const row = await env.DB.prepare(
`SELECT 1 AS hit FROM audit_events WHERE target_key = ? AND event_type = ? AND created_at >= ? LIMIT 1`,
)
.bind(targetKey, OWNER_REOPEN_PENDING_EVENT_TYPE, sinceIso)
.first<{ hit: number }>();
return Boolean(row);
} catch {
return false;
}
}

/**
* 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):
* • REOPEN of a bot-CLOSED PR by a CONTRIBUTOR → `reversal_reopened` (the high-value case). Reopens by the
* repo OWNER (administrative re-queue) or by a BOT are NOT contributor disputes and are skipped, so the
* reversal signal isn't inflated.
* • REOPEN of a bot-CLOSED PR by a CONTRIBUTOR → `reversal_reopened` (the high-value case).
* • REOPEN of a bot-CLOSED PR by the repo OWNER, followed by an approve/merge within
* OWNER_REOPEN_MERGE_WINDOW_MS → also `reversal_reopened` (#7985): unlike a bare owner reopen (still
* ambiguous — could be a genuine administrative re-queue), an owner reopen the owner then actually merges
* is an unambiguous "the bot was wrong" signal. A bot reopening itself is never a human disagreement
* signal and stays excluded unconditionally.
* • a merged "Reverts #N" PR (a bot-MERGED PR a human reverted) → `reversal_reverted` against PR #N.
*
* Writes to BOTH review_audit (what ops.ts joins for reversalRate/calibration) and audit_events (the general
Expand All @@ -441,16 +472,30 @@ export async function recordReversalSignals(
if (!pr?.number || !repoFullName) return;
const project = repoFullName.slice(0, 200);

// A bot-CLOSED PR REOPENED by a contributor — the genuine "human disagreed with this close" signal.
// A bot-CLOSED PR REOPENED by a human — the genuine "disagreed with this close" signal.
if (payload.action === "reopened") {
const ownerLogin = (repoFullName.split("/")[0] || "").toLowerCase();
const senderLogin = (payload.sender?.login || "").toLowerCase();
const senderIsOwner =
!!ownerLogin && !!senderLogin && ownerLogin === senderLogin;
const senderIsBot = payload.sender?.type === "Bot";
if (senderIsBot || senderIsOwner) return; // administrative / bot reopen — not a contributor dispute
if (senderIsBot) return; // a bot reopening itself is never a human disagreement signal
const targetId = reviewAuditTargetId(repoFullName, pr.number);
if (!(await lastBotActionWasClose(env, targetId))) return; // only a bot-CLOSED PR reopening is a reversal
if (senderIsOwner) {
// #7985: record a time-bounded marker rather than an immediate reversal — the "closed"+merged branch
// below promotes it to a real reversal_reopened only if a merge follows within the window, the same
// "genuine correction, not noise" bar a bare reopen doesn't clear on its own.
await recordAuditEvent(env, {
eventType: OWNER_REOPEN_PENDING_EVENT_TYPE,
actor: payload.sender?.login ?? null,
targetKey: targetId,
outcome: "completed",
detail: `Bot-closed PR #${pr.number} reopened by the repo owner.`,
metadata: { repoFullName, pullNumber: pr.number },
}).catch(() => undefined);
return;
}
await appendReviewAudit(env, {
project,
targetId,
Expand All @@ -468,8 +513,27 @@ export async function recordReversalSignals(
return;
}

// A merged "Reverts #N" PR — a bot-MERGED PR that a human reverted.
// 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).
if (payload.action === "closed" && Boolean(pr.merged_at)) {
const targetId = reviewAuditTargetId(repoFullName, pr.number);
if (await hasRecentOwnerReopenPendingReversal(env, targetId, OWNER_REOPEN_MERGE_WINDOW_MS)) {
await appendReviewAudit(env, {
project,
targetId,
eventType: "reversal_reopened",
summary: `Bot-closed PR #${pr.number} reopened and merged by the repo owner.`,
});
await recordAuditEvent(env, {
eventType: "reversal_reopened",
actor: payload.sender?.login ?? null,
targetKey: targetId,
outcome: "completed",
detail: `Bot-closed PR #${pr.number} reopened and merged by the repo owner.`,
metadata: { repoFullName, pullNumber: pr.number },
}).catch(() => undefined);
}
const reverted = parseRevertedPrNumber(pr.body);
if (!reverted) return;
const revertedTargetKey = reviewAuditTargetId(repoFullName, reverted);
Expand Down
81 changes: 78 additions & 3 deletions test/unit/outcomes-wire.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,11 @@ describe("recordReversalSignals — reversal_reopened", () => {
expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(0);
});

it("does NOT record an OWNER reopen or a BOT reopen (administrative / not a contributor dispute)", async () => {
it("does NOT immediately record an OWNER reopen (still ambiguous on its own — #7985), and never records a BOT reopen at all", async () => {
const env = createTestEnv();
await seedBotAction(env, "owner/repo#7", "close");
// Owner reopen — administrative re-queue, not a dispute.
// Owner reopen — a bare reopen alone stays ambiguous (could be an administrative re-queue); it only
// becomes a reversal if a merge follows within the window (see the describe block below).
await recordReversalSignals(env, "pull_request", {
action: "reopened",
repository: {
Expand All @@ -289,7 +290,11 @@ describe("recordReversalSignals — reversal_reopened", () => {
pull_request: pullRequestPayload({ number: 7, state: "open" }),
sender: { login: "owner", type: "User" },
});
// Bot reopen — not a human dispute.
expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(0);
// ...but it DOES record the time-bounded pending marker the merge branch will look for.
expect(await auditEventRows(env, "owner_reopen_pending_reversal")).toHaveLength(1);

// Bot reopen — not a human dispute, no marker at all.
await recordReversalSignals(env, "pull_request", {
action: "reopened",
repository: {
Expand All @@ -301,6 +306,76 @@ describe("recordReversalSignals — reversal_reopened", () => {
sender: { login: "some-bot[bot]", type: "Bot" },
});
expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(0);
expect(await auditEventRows(env, "owner_reopen_pending_reversal")).toHaveLength(1); // unchanged
});

describe("owner reopen + merge within the window (#7985)", () => {
it("promotes an owner's reopen-then-merge of a bot-closed PR to a real reversal_reopened", async () => {
const env = createTestEnv();
await seedBotAction(env, "owner/repo#7", "close");
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 reviewAuditRows(env, "reversal_reopened")).toHaveLength(0); // not yet — no merge seen
await recordReversalSignals(env, "pull_request", {
action: "closed",
repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } },
pull_request: pullRequestPayload({ number: 7, merged_at: "2026-06-20T00:00:00.000Z" }),
sender: { login: "owner", type: "User" },
});
const eval_ = await reviewAuditRows(env, "reversal_reopened");
expect(eval_).toHaveLength(1);
expect(eval_[0]).toMatchObject({ project: "owner/repo", target_id: "owner/repo#7" });
expect(await auditEventRows(env, "reversal_reopened")).toHaveLength(1);
});

it("does NOT record a reversal for a plain merge with no preceding owner-reopen marker", async () => {
const env = createTestEnv();
await recordReversalSignals(env, "pull_request", {
action: "closed",
repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } },
pull_request: pullRequestPayload({ number: 7, merged_at: "2026-06-20T00:00:00.000Z" }),
sender: { login: "owner", type: "User" },
});
expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(0);
});

it("does NOT record a reversal when the owner-reopen marker is older than the merge window (stale rescue signal)", async () => {
const env = createTestEnv();
await seedBotAction(env, "owner/repo#7", "close");
// Seed a marker far enough in the past that it's outside OWNER_REOPEN_MERGE_WINDOW_MS by construction,
// bypassing recordReversalSignals' own (real-clock) write path so the test isn't time-flaky.
await recordAuditEvent(env, {
eventType: "owner_reopen_pending_reversal",
actor: "owner",
targetKey: "owner/repo#7",
outcome: "completed",
detail: "Bot-closed PR #7 reopened by the repo owner.",
createdAt: "2020-01-01T00:00:00.000Z",
});
await recordReversalSignals(env, "pull_request", {
action: "closed",
repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } },
pull_request: pullRequestPayload({ number: 7, merged_at: "2026-06-20T00:00:00.000Z" }),
sender: { login: "owner", type: "User" },
});
expect(await reviewAuditRows(env, "reversal_reopened")).toHaveLength(0);
});

it("does NOT record a reversal when the owner reopens a PR whose last bot action was NOT a close", async () => {
const env = createTestEnv();
await seedBotAction(env, "owner/repo#7", "approve");
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 auditEventRows(env, "owner_reopen_pending_reversal")).toHaveLength(0);
});
});

it("still records reversal_reopened when the bot close was logged with the legacy 'success' outcome", async () => {
Expand Down