diff --git a/scripts/backfill-pr-outcomes-core.ts b/scripts/backfill-pr-outcomes-core.ts new file mode 100644 index 0000000000..5125e1bfed --- /dev/null +++ b/scripts/backfill-pr-outcomes-core.ts @@ -0,0 +1,82 @@ +// Backfill missing pr_outcome rows (#8823) — PURE planning core. +// +// The webhook-only pr_outcome writer lost ground truth whenever a `pull_request.closed` delivery was never +// processed. Because the fleet export inner-joins gate_decision to pr_outcome, every affected PR fell out of +// calibration entirely — and since the losses skew toward the gate's MISTAKES (a superseded close is by +// definition a wrong close), their absence biased published accuracy upward. +// +// recordTerminalActionOutcome (src/review/outcomes-wire.ts) stops the bleeding going forward; this closes the +// historical hole. The completed terminal action in audit_events IS the ground truth: `agent.action.merge` +// means the bot merged it, `agent.action.close` means the bot closed it. Both are authoritative — GitHub +// rejects a merge the actor may not perform, and a completed close is the repository's realized decision. +// +// PURE: decides WHAT to write from rows the caller supplies. The thin IO wrapper reads/writes the ledger. + +/** A completed terminal action, as read from audit_events. */ +export type TerminalActionRow = { + targetKey: string; // "owner/repo#123" + eventType: string; // agent.action.merge | agent.action.close + createdAt: string; +}; + +export type BackfillPlanEntry = { + targetKey: string; + project: string; + pullNumber: number; + decision: "merged" | "closed"; + createdAt: string; +}; + +export type BackfillPlan = { + entries: BackfillPlanEntry[]; + skipped: { alreadyRecorded: number; unparseable: number; unknownAction: number }; +}; + +/** Split "owner/repo#123" into its parts; null when the shape isn't exactly that. */ +export function parseTargetKey(targetKey: string): { project: string; pullNumber: number } | null { + const hash = targetKey.lastIndexOf("#"); + if (hash <= 0 || hash === targetKey.length - 1) return null; + const project = targetKey.slice(0, hash); + // Reject the legacy `project:pull_request:owner/repo#123` shape outright: those rows predate the + // convergence cutover and the fleet export deliberately excludes them (source != 'gittensory-native'). + if (!/^[^/:\s]+\/[^/:\s]+$/.test(project)) return null; + const pullNumber = Number(targetKey.slice(hash + 1)); + if (!Number.isInteger(pullNumber) || pullNumber <= 0) return null; + return { project, pullNumber }; +} + +/** + * Plan the backfill: one pr_outcome per terminal-action target that has none. + * + * When a target somehow carries BOTH a merge and a close action (a close that later got superseded and + * re-merged under the same number is not possible on GitHub, but a retry storm could record both), the + * LATEST action wins — it is the one whose effect survived. + */ +export function planPrOutcomeBackfill(actions: TerminalActionRow[], alreadyRecorded: ReadonlySet): BackfillPlan { + const skipped = { alreadyRecorded: 0, unparseable: 0, unknownAction: 0 }; + const latest = new Map(); + + for (const action of actions) { + const decision = action.eventType === "agent.action.merge" ? "merged" : action.eventType === "agent.action.close" ? "closed" : null; + if (decision === null) { + skipped.unknownAction += 1; + continue; + } + if (alreadyRecorded.has(action.targetKey)) { + skipped.alreadyRecorded += 1; + continue; + } + const parsed = parseTargetKey(action.targetKey); + if (parsed === null) { + skipped.unparseable += 1; + continue; + } + const existing = latest.get(action.targetKey); + if (existing !== undefined && existing.createdAt >= action.createdAt) continue; + latest.set(action.targetKey, { targetKey: action.targetKey, project: parsed.project, pullNumber: parsed.pullNumber, decision, createdAt: action.createdAt }); + } + + // Deterministic order so a dry run and the real run report identically. + const entries = [...latest.values()].sort((a, b) => a.createdAt.localeCompare(b.createdAt) || a.targetKey.localeCompare(b.targetKey)); + return { entries, skipped }; +} diff --git a/scripts/backfill-pr-outcomes.ts b/scripts/backfill-pr-outcomes.ts new file mode 100644 index 0000000000..9c0ea089f7 --- /dev/null +++ b/scripts/backfill-pr-outcomes.ts @@ -0,0 +1,61 @@ +#!/usr/bin/env node +// Backfill missing pr_outcome rows (#8823) — thin IO wrapper around backfill-pr-outcomes-core.ts. +// +// Runs INSIDE a self-hosted instance (it talks to the local ledger, not the cloud): reads completed terminal +// actions from audit_events, subtracts targets that already have a pr_outcome, and writes the missing rows. +// Idempotent — re-running writes nothing new. +// +// node --experimental-strip-types scripts/backfill-pr-outcomes.ts --dry-run +// node --experimental-strip-types scripts/backfill-pr-outcomes.ts --apply +// +// After applying, rewind orb_export_cursor so the corrected rows re-export to fleet calibration: +// UPDATE orb_export_cursor SET last_exported_at = ''; +import { planPrOutcomeBackfill, type TerminalActionRow } from "./backfill-pr-outcomes-core.js"; + +type Row = Record; +type Db = { + all: (sql: string, binds?: unknown[]) => Promise; + run: (sql: string, binds?: unknown[]) => Promise; +}; + +const TERMINAL_ACTIONS_SQL = ` + SELECT target_key AS "targetKey", event_type AS "eventType", created_at AS "createdAt" + FROM audit_events + WHERE event_type IN ('agent.action.merge', 'agent.action.close') + AND outcome IN ('success', 'completed') + ORDER BY created_at ASC`; + +const RECORDED_SQL = `SELECT DISTINCT target_id AS "targetId" FROM review_audit WHERE event_type = 'pr_outcome'`; + +export async function runBackfill(db: Db, apply: boolean, log: (line: string) => void = console.log): Promise { + const actionRows = (await db.all(TERMINAL_ACTIONS_SQL)) as unknown as TerminalActionRow[]; + const recordedRows = await db.all(RECORDED_SQL); + const recorded = new Set(recordedRows.map((r) => String(r.targetId))); + + const plan = planPrOutcomeBackfill(actionRows, recorded); + log( + `backfill-pr-outcomes: ${actionRows.length} terminal action(s), ${recorded.size} already recorded -> ` + + `${plan.entries.length} to write (skipped: ${plan.skipped.alreadyRecorded} recorded, ` + + `${plan.skipped.unparseable} unparseable, ${plan.skipped.unknownAction} non-terminal)`, + ); + if (!apply) { + for (const entry of plan.entries.slice(0, 20)) log(` would write ${entry.targetKey} -> ${entry.decision}`); + if (plan.entries.length > 20) log(` ... and ${plan.entries.length - 20} more`); + log("backfill-pr-outcomes: DRY RUN — pass --apply to write"); + return plan.entries.length; + } + + let written = 0; + for (const entry of plan.entries) { + // review_audit is the store the fleet export and computeGateEval read; the audit_events mirror carries the + // same provenance the live writer stamps, tagged so a backfilled row is distinguishable from a live one. + await db.run( + `INSERT INTO review_audit (id, project, target_id, event_type, decision, source, created_at) + VALUES (?, ?, ?, 'pr_outcome', ?, 'gittensory-native', ?)`, + [`backfill-${entry.targetKey}-${entry.createdAt}`.slice(0, 190), entry.project.slice(0, 200), entry.targetKey, entry.decision, entry.createdAt], + ); + written += 1; + } + log(`backfill-pr-outcomes: wrote ${written} pr_outcome row(s)`); + return written; +} diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 135f033682..b7b25b2236 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -346,6 +346,61 @@ export async function resolveDispositionReason( } } +/** + * Record the realized outcome of a terminal action THIS bot just completed, without waiting for GitHub to + * deliver the matching `pull_request.closed` webhook (#8823). + * + * recordPrOutcome below is webhook-only, so a delivery this instance never processes — a dropped brokered + * relay event, a restart mid-delivery — loses that PR's ground truth PERMANENTLY: the fleet export inner-joins + * gate_decision to pr_outcome, so an outcome-less PR vanishes from calibration entirely, contributing to + * neither numerator nor denominator. Measured on the live self-host, ~55% of bot closes after 2026-07-18 had + * no pr_outcome row while the PRs were verifiably closed on GitHub, and because the dropped rows skew toward + * the gate's MISTAKES (a superseded close is by definition a wrong close), their absence biased published + * accuracy upward. + * + * Idempotent: skips when a pr_outcome row already exists for the target, so the webhook path (whichever wins + * the race) never produces a duplicate. Best-effort — a bookkeeping failure must never fail the action that + * already succeeded against GitHub. + */ +export async function recordTerminalActionOutcome( + env: Env, + repoFullName: string, + pullNumber: number, + decision: "merged" | "closed", +): Promise { + const targetId = reviewAuditTargetId(repoFullName, pullNumber); + try { + const existing = await env.DB.prepare( + "SELECT 1 AS x FROM review_audit WHERE target_id = ? AND event_type = 'pr_outcome' LIMIT 1", + ) + .bind(targetId) + .first<{ x: number }>(); + if (existing) return; + } catch (error) { + // An unreadable ledger must not suppress the write — a duplicate row is strictly better than a lost + // outcome (the fleet export and computeGateEval both read the LATEST pr_outcome per target). + console.warn(JSON.stringify({ event: "pr_outcome_direct_probe_error", message: errorMessage(error).slice(0, 160) })); + } + + incr("loopover_pr_outcomes_total", { outcome: decision }); + await appendReviewAudit(env, { + project: repoFullName.slice(0, 200), + targetId, + eventType: "pr_outcome", + decision, + }); + await recordAuditEvent(env, { + eventType: "pr_outcome", + actor: null, + targetKey: targetId, + outcome: "completed", + detail: decision, + metadata: { repoFullName, pullNumber, merged: decision === "merged", botWasActor: true, source: "terminal_action" }, + }).catch((error) => + console.warn(JSON.stringify({ event: "pr_outcome_direct_audit_error", message: errorMessage(error).slice(0, 160) })), + ); +} + export async function recordPrOutcome( env: Env, eventName: string, diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 10b20f9b8c..1ce034cfd4 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -17,7 +17,7 @@ import { import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; -import { resolveDispositionReason } from "../review/outcomes-wire"; +import { recordTerminalActionOutcome, resolveDispositionReason } from "../review/outcomes-wire"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestState, fetchLiveReviewThreadBlockers, refreshInstallationHealthForInstallation } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; @@ -1049,6 +1049,10 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: // expectedHeadSha == ctx.headSha, so its behavior is unchanged; the fallback covers any unpinned plan. const mergeSha = action.expectedHeadSha ?? ctx.headSha; await mergePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, { mergeMethod: action.mergeMethod ?? "squash", ...(mergeSha ? { sha: mergeSha } : {}) }); + // #8823: record ground truth from the action we just completed rather than depending on the inbound + // `pull_request.closed` webhook — a delivery this instance never processes used to lose the outcome + // permanently, dropping the PR out of fleet calibration entirely. Idempotent against the webhook path. + await recordTerminalActionOutcome(env, ctx.repoFullName, ctx.pullNumber, "merged"); return; } case "close": @@ -1057,6 +1061,9 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action: // instead of stacking a duplicate "why we closed you" every failed cycle. if (action.closeComment) await createOrUpdateCloseExplanationComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.closeComment, action.closeKind); await closePullRequest(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber); + // #8823: see the merge case — the bot's own close is authoritative ground truth and must not depend on + // a webhook round-trip the instance might never see. + await recordTerminalActionOutcome(env, ctx.repoFullName, ctx.pullNumber, "closed"); return; case "update_branch": { // update_branch does NOT need the accept-flow-level "unpinned → deny" gate that #2377/#2422 added for diff --git a/test/unit/backfill-pr-outcomes-core.test.ts b/test/unit/backfill-pr-outcomes-core.test.ts new file mode 100644 index 0000000000..71540e930d --- /dev/null +++ b/test/unit/backfill-pr-outcomes-core.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from "vitest"; +import { parseTargetKey, planPrOutcomeBackfill, type TerminalActionRow } from "../../scripts/backfill-pr-outcomes-core"; +import { runBackfill } from "../../scripts/backfill-pr-outcomes"; + +// #8823: the historical half of the lost-ground-truth fix. The planning core decides WHAT to backfill from +// completed terminal actions; a wrong decision here would inject fabricated ground truth into calibration, +// so every skip arm is asserted explicitly. +describe("parseTargetKey", () => { + it("accepts the canonical owner/repo#N shape", () => { + expect(parseTargetKey("JSONbored/loopover#8428")).toEqual({ project: "JSONbored/loopover", pullNumber: 8428 }); + }); + + it("rejects the LEGACY project:pull_request:owner/repo#N shape the fleet export already excludes", () => { + expect(parseTargetKey("JSONbored/awesome-claude:pull_request:JSONbored/awesome-claude#2516")).toBeNull(); + }); + + it("rejects malformed keys rather than guessing", () => { + expect(parseTargetKey("no-hash")).toBeNull(); + expect(parseTargetKey("#123")).toBeNull(); // no project + expect(parseTargetKey("owner/repo#")).toBeNull(); // no number + expect(parseTargetKey("owner/repo#abc")).toBeNull(); // non-numeric + expect(parseTargetKey("owner/repo#0")).toBeNull(); // PR numbers are 1-based + expect(parseTargetKey("ownerrepo#12")).toBeNull(); // missing the slash + }); +}); + +describe("planPrOutcomeBackfill (#8823)", () => { + const row = (targetKey: string, eventType: string, createdAt: string): TerminalActionRow => ({ targetKey, eventType, createdAt }); + + it("maps merge → merged and close → closed for targets with no recorded outcome", () => { + const plan = planPrOutcomeBackfill( + [row("o/r#1", "agent.action.merge", "2026-07-20T00:00:00Z"), row("o/r#2", "agent.action.close", "2026-07-21T00:00:00Z")], + new Set(), + ); + expect(plan.entries).toEqual([ + { targetKey: "o/r#1", project: "o/r", pullNumber: 1, decision: "merged", createdAt: "2026-07-20T00:00:00Z" }, + { targetKey: "o/r#2", project: "o/r", pullNumber: 2, decision: "closed", createdAt: "2026-07-21T00:00:00Z" }, + ]); + }); + + it("NEVER re-writes a target that already has a pr_outcome (the webhook got through)", () => { + const plan = planPrOutcomeBackfill([row("o/r#1", "agent.action.close", "2026-07-20T00:00:00Z")], new Set(["o/r#1"])); + expect(plan.entries).toEqual([]); + expect(plan.skipped.alreadyRecorded).toBe(1); + }); + + it("skips legacy/unparseable target keys and non-terminal action types instead of fabricating an outcome", () => { + const plan = planPrOutcomeBackfill( + [ + row("o/r:pull_request:o/r#5", "agent.action.close", "2026-07-20T00:00:00Z"), + row("o/r#6", "agent.action.label", "2026-07-20T00:00:00Z"), + ], + new Set(), + ); + expect(plan.entries).toEqual([]); + expect(plan.skipped).toEqual({ alreadyRecorded: 0, unparseable: 1, unknownAction: 1 }); + }); + + it("when a target has BOTH actions the LATEST wins — the effect that survived — regardless of input order", () => { + const late = row("o/r#9", "agent.action.merge", "2026-07-22T00:00:00Z"); + const early = row("o/r#9", "agent.action.close", "2026-07-20T00:00:00Z"); + expect(planPrOutcomeBackfill([early, late], new Set()).entries[0]).toMatchObject({ decision: "merged" }); + expect(planPrOutcomeBackfill([late, early], new Set()).entries[0]).toMatchObject({ decision: "merged" }); + // Equal timestamps keep the first-seen row rather than flapping on input order. + const tie = planPrOutcomeBackfill([row("o/r#9", "agent.action.close", "2026-07-22T00:00:00Z"), late], new Set()); + expect(tie.entries[0]).toMatchObject({ decision: "closed" }); + }); + + it("emits a deterministic (createdAt, then targetKey) order so a dry run matches the real run", () => { + const plan = planPrOutcomeBackfill( + [ + row("o/r#3", "agent.action.close", "2026-07-22T00:00:00Z"), + row("o/r#1", "agent.action.close", "2026-07-20T00:00:00Z"), + row("o/r#2", "agent.action.close", "2026-07-22T00:00:00Z"), + ], + new Set(), + ); + expect(plan.entries.map((e) => e.targetKey)).toEqual(["o/r#1", "o/r#2", "o/r#3"]); + }); + + it("returns an empty plan (never throws) for no input at all", () => { + expect(planPrOutcomeBackfill([], new Set())).toEqual({ entries: [], skipped: { alreadyRecorded: 0, unparseable: 0, unknownAction: 0 } }); + }); +}); + +describe("runBackfill IO wrapper (#8823)", () => { + function fakeDb(actions: Array>, recorded: string[]) { + const writes: unknown[][] = []; + return { + writes, + db: { + all: async (sql: string) => (sql.includes("audit_events") ? actions : recorded.map((targetId) => ({ targetId }))), + run: async (_sql: string, binds?: unknown[]) => { + writes.push(binds ?? []); + return undefined; + }, + }, + }; + } + + it("DRY RUN reports the plan and writes nothing", async () => { + const { db, writes } = fakeDb( + [{ targetKey: "o/r#1", eventType: "agent.action.close", createdAt: "2026-07-20T00:00:00Z" }], + [], + ); + const lines: string[] = []; + expect(await runBackfill(db, false, (l) => lines.push(l))).toBe(1); + expect(writes).toHaveLength(0); + expect(lines.join("\n")).toContain("DRY RUN"); + expect(lines.join("\n")).toContain("would write o/r#1 -> closed"); + }); + + it("--apply writes exactly the planned rows and is idempotent on a second run", async () => { + const actions = [ + { targetKey: "o/r#1", eventType: "agent.action.close", createdAt: "2026-07-20T00:00:00Z" }, + { targetKey: "o/r#2", eventType: "agent.action.merge", createdAt: "2026-07-21T00:00:00Z" }, + ]; + const first = fakeDb(actions, []); + expect(await runBackfill(first.db, true, () => undefined)).toBe(2); + expect(first.writes.map((w) => [w[2], w[3]])).toEqual([ + ["o/r#1", "closed"], + ["o/r#2", "merged"], + ]); + // Re-run with those targets now recorded -> nothing to do. + const second = fakeDb(actions, ["o/r#1", "o/r#2"]); + expect(await runBackfill(second.db, true, () => undefined)).toBe(0); + expect(second.writes).toHaveLength(0); + }); + + it("truncates the dry-run listing past 20 entries instead of flooding the operator", async () => { + const actions = Array.from({ length: 25 }, (_, i) => ({ + targetKey: `o/r#${i + 1}`, + eventType: "agent.action.close", + createdAt: `2026-07-${String((i % 9) + 1).padStart(2, "0")}T00:00:00Z`, + })); + const { db } = fakeDb(actions, []); + const lines: string[] = []; + expect(await runBackfill(db, false, (l) => lines.push(l))).toBe(25); + expect(lines.join("\n")).toContain("and 5 more"); + }); +}); diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 45982b2a70..3a2fbd15de 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -10,6 +10,7 @@ import { readUntrustworthyRuleCodes, recordPrOutcome, recordReversalSignals, + recordTerminalActionOutcome, resolveDispositionReason, runSelfTuneBreaker, } from "../../src/review/outcomes-wire"; @@ -239,6 +240,63 @@ describe("recordPrOutcome — realized merge/close ground truth", () => { // ── 2) reversal_reopened — a contributor reopened a bot-CLOSED PR ────────────────────────────────────────────── +describe("recordTerminalActionOutcome — webhook-independent ground truth (#8823)", () => { + it("writes the pr_outcome the bot's own action realized, without any webhook", async () => { + const env = createTestEnv(); + await recordTerminalActionOutcome(env, "owner/repo", 42, "closed"); + const rows = await reviewAuditRows(env, "pr_outcome"); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ project: "owner/repo", target_id: "owner/repo#42", decision: "closed" }); + }); + + it("records a merge the same way", async () => { + const env = createTestEnv(); + await recordTerminalActionOutcome(env, "owner/repo", 7, "merged"); + expect((await reviewAuditRows(env, "pr_outcome"))[0]).toMatchObject({ target_id: "owner/repo#7", decision: "merged" }); + }); + + it("is IDEMPOTENT against the webhook path — whichever wins the race, exactly one row survives", async () => { + const env = createTestEnv(); + await recordPrOutcome(env, "pull_request", { + action: "closed", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number: 42 }), + sender: { login: "loopover[bot]", type: "Bot" }, + }); + await recordTerminalActionOutcome(env, "owner/repo", 42, "closed"); + expect(await reviewAuditRows(env, "pr_outcome")).toHaveLength(1); + }); + + it("a failing audit_events mirror never breaks the canonical review_audit write", async () => { + const env = createTestEnv(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + // Fail ONLY the audit_events mirror insert; the review_audit write and the probe still run for real. + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + if (sql.includes("audit_events") && sql.trim().toUpperCase().startsWith("INSERT")) throw new Error("mirror down"); + return realPrepare(sql); + }); + await recordTerminalActionOutcome(env, "owner/repo", 55, "merged"); + (env.DB.prepare as unknown as { mockRestore: () => void }).mockRestore(); + expect((await reviewAuditRows(env, "pr_outcome"))[0]).toMatchObject({ target_id: "owner/repo#55", decision: "merged" }); + expect(warn).toHaveBeenCalled(); + }); + + it("still writes when the idempotency probe itself fails — a lost outcome is worse than a duplicate", async () => { + const env = createTestEnv(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementationOnce(() => { + throw new Error("ledger unavailable"); + }); + await recordTerminalActionOutcome(env, "owner/repo", 99, "closed"); + (env.DB.prepare as unknown as { mockRestore: () => void }).mockRestore?.(); + void realPrepare; + expect((await reviewAuditRows(env, "pr_outcome"))[0]).toMatchObject({ target_id: "owner/repo#99" }); + expect(warn).toHaveBeenCalled(); + }); +}); + describe("recordReversalSignals — reversal_reopened", () => { it("writes reversal_reopened (review_audit + audit_events) when a contributor reopens a bot-CLOSED PR", async () => { const env = createTestEnv();