diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index b7b25b223..f828633aa 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -429,10 +429,28 @@ export async function recordPrOutcome( return; const decision = merged ? "merged" : "closed"; + const targetId = reviewAuditTargetId(repoFullName, pr.number); + // The bot's own recordTerminalActionOutcome writes the pr_outcome row first (right after the merge/close + // mutation); GitHub then delivers the `closed` webhook for the SAME action, landing here second. Probe for + // the existing row before touching the counter so loopover_pr_outcomes_total is incremented exactly once per + // real outcome — mirroring recordTerminalActionOutcome's own guard (#10332). + 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 a genuinely-new outcome — a duplicate row is strictly better than a + // lost one (fleet export + computeGateEval both read the LATEST pr_outcome per target). Fail open, like the + // direct path. + console.warn(JSON.stringify({ event: "pr_outcome_webhook_probe_error", message: errorMessage(error).slice(0, 160) })); + } + // Observability (#reviews-dashboard): realized human outcome (merged vs closed) for the Grafana panel + as the // ground truth to compare against the engine's gate verdicts. incr("loopover_pr_outcomes_total", { outcome: decision }); - const targetId = reviewAuditTargetId(repoFullName, pr.number); await appendReviewAudit(env, { project: repoFullName.slice(0, 200), diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 3a2fbd15d..d22ae9ced 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -22,6 +22,7 @@ import { type PlannedAgentAction, } from "../../src/settings/agent-actions"; import { recordAuditEvent } from "../../src/db/repositories"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import type { GitHubPullRequestPayload } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -267,6 +268,46 @@ describe("recordTerminalActionOutcome — webhook-independent ground truth (#882 expect(await reviewAuditRows(env, "pr_outcome")).toHaveLength(1); }); + it("counts loopover_pr_outcomes_total exactly ONCE in the realistic terminal-action-first ordering (#10332)", async () => { + // Production ordering: the bot's own recordTerminalActionOutcome writes first (right after the merge/close + // mutation), THEN GitHub delivers the `closed` webhook and recordPrOutcome runs for the SAME PR. Before the + // fix, the webhook path always incremented the counter, so one real outcome was counted twice. + const env = createTestEnv(); + resetMetrics(); + await recordTerminalActionOutcome(env, "owner/repo", 42, "closed"); + 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" }, + }); + const counter = (await renderMetrics()) + .split("\n") + .find((l) => l.startsWith('loopover_pr_outcomes_total{outcome="closed"}')); + expect(counter).toBe('loopover_pr_outcomes_total{outcome="closed"} 1'); // once total, not twice + expect(await reviewAuditRows(env, "pr_outcome")).toHaveLength(1); // and the row stays deduplicated too + }); + + it("recordPrOutcome fails OPEN when its idempotency probe throws — a lost outcome is worse than a duplicate (#10332)", async () => { + const env = createTestEnv(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const realPrepare = env.DB.prepare.bind(env.DB); + // Fail ONLY the probe SELECT; every subsequent write (appendReviewAudit/audit_events) still runs for real. + vi.spyOn(env.DB, "prepare").mockImplementationOnce(() => { + throw new Error("ledger unavailable"); + }); + await recordPrOutcome(env, "pull_request", { + action: "closed", + repository: { name: "repo", full_name: "owner/repo", owner: { login: "owner" } }, + pull_request: pullRequestPayload({ number: 77 }), + sender: { login: "maintainer", type: "User" }, + }); + (env.DB.prepare as unknown as { mockRestore: () => void }).mockRestore?.(); + void realPrepare; + expect((await reviewAuditRows(env, "pr_outcome"))[0]).toMatchObject({ target_id: "owner/repo#77" }); + expect(warn).toHaveBeenCalled(); + }); + 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);