fix(review): guard recordPrOutcome's webhook path against double-counting (#10346)

recordPrOutcome (the inbound pull_request.closed webhook path) always incremented
loopover_pr_outcomes_total and wrote a pr_outcome row, with no existence check.
recordTerminalActionOutcome (the bot's own direct-action path) already probes for
an existing pr_outcome row and skips both the metric increment and the write when
one exists.

In the realistic production ordering the direct write lands first (right after the
merge/close mutation), then GitHub delivers the closed webhook for the same action,
so the counter was incremented twice for one real PR outcome. The duplicate ROW is
harmless (every downstream reader takes the LATEST row per target), but the counter
had no such defense.

Mirror recordTerminalActionOutcome's guard exactly: probe for an existing pr_outcome
row before incrementing or writing, and return early when one exists. A probe-read
failure logs and proceeds (fail-open) so a genuinely-new outcome is never dropped.
No other decision logic in recordPrOutcome changes.

Closes #10332

Author: shin-core

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),

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);