From 4309c4d65680cd00d4709289c1ad32b954f4dd6e Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:04:50 +0900 Subject: [PATCH] fix(review): guard recordPrOutcome's webhook path against double-counting 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 --- src/review/outcomes-wire.ts | 20 +++++++++++++++- test/unit/outcomes-wire.test.ts | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index b7b25b2236..f828633aa0 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 3a2fbd15de..d22ae9cede 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);