From cffa2f7f260bd81ff97a274c68fc80ba04b80bfd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 22 Jul 2026 12:14:14 -0700 Subject: [PATCH] fix(review): count owner reopen-then-merge as a real reversal signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7985. A bare owner reopen of a bot-closed PR stayed excluded from reversalRate (still ambiguous — could be an administrative re-queue), but an owner reopen followed by an approve/merge within 6h is unambiguous: the owner looked again and decided the bot was wrong. Every one of the 2026-07-21/22 metagraphed false-positive incidents (#7469/#7589/#7591/#7594) was exactly this pattern, and the old unconditional owner-reopen exclusion recorded nothing for any of them — part of why the accuracy metric stayed misleadingly high that day. Record a time-bounded owner_reopen_pending_reversal marker on reopen, then promote it to a real reversal_reopened event if a merge follows within OWNER_REOPEN_MERGE_WINDOW_MS. A bot reopening itself still never counts. --- src/review/outcomes-wire.ts | 76 ++++++++++++++++++++++++++++--- test/unit/outcomes-wire.test.ts | 81 +++++++++++++++++++++++++++++++-- 2 files changed, 148 insertions(+), 9 deletions(-) diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index b7fd72828f..4a0cc16af7 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -419,12 +419,43 @@ async function wasMergeRecorded(env: Env, targetId: string): Promise { } } +// #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 { + 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 @@ -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, @@ -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); diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 88a3e42d13..b363f38d47 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -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: { @@ -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: { @@ -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 () => {