Skip to content

review(outcomes-wire): guard recordPrOutcome's webhook path against double-counting loopover_pr_outcomes_total #10332

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/review/outcomes-wire.ts writes a pr_outcome row two independent ways, both best-effort: directly
after the bot's own merge/close mutation (recordTerminalActionOutcome, called from
agent-action-executor.ts), and from the inbound pull_request.closed webhook (recordPrOutcome, called
from queue/processors.ts). pr_outcome is realized ground truth the fleet calibration export and
computeGateEval both read (the LATEST row per target), so a duplicate row for the same PR is tolerated by
every downstream reader — but it is NOT tolerated by the loopover_pr_outcomes_total Prometheus counter
both paths independently increment.

recordTerminalActionOutcome (the direct-action path) already guards against double-counting:

export async function recordTerminalActionOutcome(env, repoFullName, pullNumber, decision) {
  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; // <-- skips the write AND the incr() below when a row already exists
  } catch (error) {
    console.warn(/* ... */);
  }
  incr("loopover_pr_outcomes_total", { outcome: decision });
  await appendReviewAudit(env, { /* ... */ });
  // ...
}

recordPrOutcome (the webhook path) has no equivalent probe — it always writes and always increments:

export async function recordPrOutcome(env, eventName, payload) {
  // ...
  const decision = merged ? "merged" : "closed";
  incr("loopover_pr_outcomes_total", { outcome: decision }); // <-- unconditional, no existence check
  const targetId = reviewAuditTargetId(repoFullName, pr.number);
  await appendReviewAudit(env, { /* ... */ });
  // ...
}

This gap is already PARTIALLY known: src/review/submitter-reputation.ts documents "recordPrOutcome's
webhook path has no existence check ... so a redelivered closed webhook can double-insert" and every
downstream SQL consumer defends itself with a LATEST_PR_OUTCOME_FILTER that reads only the most recent
row per target — so the duplicate ROW is harmless to every real consumer. But that defense does nothing for
the incr("loopover_pr_outcomes_total", ...) call itself: in the realistic production ordering (the bot's
own recordTerminalActionOutcome write happens first, right after the merge/close mutation; the webhook's
recordPrOutcome write happens second, once GitHub delivers the closed event for the same action), the
counter is incremented TWICE for one real PR outcome — the webhook path has no way to know a row already
exists, so it always adds its own increment on top. test/unit/outcomes-wire.test.ts's existing "whichever
wins the race" test (around line 258) only exercises the webhook-first ordering, not this realistic
terminal-action-first ordering, so the double-increment is not currently caught by any test.

Requirements

  • recordPrOutcome must probe for an existing pr_outcome row for the same target BEFORE incrementing
    loopover_pr_outcomes_total or writing a new row, using the same query shape
    recordTerminalActionOutcome already uses, and skip both the metric increment and the write when a row
    already exists — mirroring recordTerminalActionOutcome's existing guard exactly.
  • The existing "must not suppress a legitimate write on a read error" fail-open behavior
    (recordTerminalActionOutcome's catch block, which logs and proceeds rather than blocking the write on
    an unreadable ledger) must be mirrored in recordPrOutcome too — a probe failure must never silently drop
    a genuinely-new outcome.
  • Must not change recordTerminalActionOutcome's own logic.
  • Must not change the existing self-close guard in recordPrOutcome (the senderLogin === authorLogin
    early return) or any other decision logic in this function — this issue is scoped to the missing
    existence-check + metric double-count only.

Deliverables

  • recordPrOutcome probes for an existing pr_outcome row for the same targetId before writing,
    matching recordTerminalActionOutcome's existing query shape, and returns early (skipping both the
    incr() call and the appendReviewAudit/recordAuditEvent writes) when one already exists.
  • A probe-read failure inside recordPrOutcome logs a warning and proceeds with the write (fail-open),
    matching recordTerminalActionOutcome's existing catch-and-proceed behavior — never silently drops a
    genuinely-new outcome.
  • A new regression test covering the REALISTIC ordering this bug actually occurs in production
    (recordTerminalActionOutcome writes first for a PR, THEN recordPrOutcome is called for the same
    PR via the webhook path) asserts loopover_pr_outcomes_total is incremented exactly once total across
    both calls, not twice — this is the missing case; the existing "whichever wins the race" test at line
    258 (webhook-first ordering) must continue to pass unmodified alongside it.

All three Deliverables are required in the same PR.

Test Coverage Requirements

This repo's Codecov patch gate requires 99%+ patch coverage on every changed line and branch under
src/**. src/review/outcomes-wire.ts is inside src/**. The new test (Deliverable 3) must assert on the
actual metric value (not just that appendReviewAudit/the DB row is deduplicated, which was already
partially covered) — that is the concrete, previously-uncovered defect this issue exists to fix.

Expected Outcome

loopover_pr_outcomes_total counts each real PR outcome exactly once, regardless of which of the two write
paths (direct terminal-action write or webhook delivery) reaches the ledger first — matching the dedup
guarantee recordTerminalActionOutcome already provides on its own side, closing the metric-side gap
submitter-reputation.ts's own comments already flagged as a known row-level issue but did not fully close.

Links & Resources

  • src/review/outcomes-wire.tsrecordTerminalActionOutcome (around lines 365-402, the already-correct
    pattern to mirror) and recordPrOutcome (around lines 404-442, the function to fix).
  • src/review/submitter-reputation.ts (around lines 26-29, 313-315) — the existing documentation of the
    known row-level double-insert gap and its LATEST_PR_OUTCOME_FILTER defense (row-level only; does not
    cover the metric).
  • test/unit/outcomes-wire.test.ts (around line 258) — the existing "whichever wins the race" test to
    extend with the missing terminal-action-first ordering case.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions