fix(orb): stop a conclusion-derived verdict from overwriting a recorded close (#8826)

Both gate_decision writers key the same deterministic row id
(gate:<source>:<project>#<pr>@<sha>) with ON CONFLICT DO UPDATE, so the
last writer wins. One caller records the ACTUAL disposition the bot
acted on; the other derives the verdict from the gate-check conclusion
alone, where success maps to merge.

On a PR the bot closed for a downstream reason (CI failure, policy) the
conclusion-only writer runs last and clobbers the real close with a
merge. On #5861 the close landed at 20:23:31 and the
contradicting merge verdict was written at 20:23:44 -- 13 seconds after
the PR was already closed.

Fleet calibration reads the latest gate_decision as the gate's
prediction, so every such row is scored as a merge prediction that
ended closed: a false positive that never happened. Measured on the
live self-host, 59 rows carry a verdict timestamped after the close
action it contradicts, out of 210 in that class -- biasing published
accuracy DOWNWARD, opposite to the reversal under-counting in #8823.

Advances #8825 (the recording half; scoring policy closes as their own
class is the remaining part).

The DO UPDATE now skips when the incoming write is conclusion-derived
and the stored decision is already 'close'. A close is a terminal
action that already happened and no later conclusion can un-close it.
An explicit action still replaces it, and non-close rows keep
latest-finalize-wins.

Author: JSONbored

src/review/parity-wire.ts

@@ -163,16 +163,29 @@ export async function recordNativeGateDecision(
   const targetId = `${project}#${input.pullNumber}`;
   const summary = input.reasonCode ? input.reasonCode.slice(0, 200) : null;
   const minerAuthored = input.minerAuthored === true ? 1 : 0;
+  // #8825: whether this verdict is the ACTUAL disposition the bot acted on (`input.action` supplied by the
+  // disposition-aware caller) or merely DERIVED from the gate-check conclusion. Both callers write the same
+  // deterministic row id below, so without this distinction the conclusion-derived write clobbers the real one.
+  const derivedFromConclusion = input.action === undefined ? 1 : 0;
   try {
     // Deterministic id per (source, project, pr, sha): a re-run at the SAME commit REPLACES its prior decision
     // (the latest finalize wins), while a new commit gets its own row. event_type/source default in the schema
     // but are written explicitly for clarity.
+    //
+    // #8825 — the DO UPDATE is guarded so a conclusion-derived verdict can never overwrite a recorded `close`.
+    // A gate conclusion of "success" maps to `merge` (nativeGateActionFromConclusion), and the conclusion-only
+    // caller runs AFTER the disposition-aware one on a PR the bot closed for a downstream reason (CI failure,
+    // policy). That wrote `merge` over the real `close`, sometimes seconds after the PR was already closed --
+    // measured on the live self-host, 59 rows recorded a verdict timestamped AFTER the close action it
+    // contradicted, and calibration scored every one as a merge prediction that ended closed. A close is a
+    // terminal action that already happened; no later conclusion can un-close it, so the older row wins.
     await env.DB.prepare(
       `INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, miner_authored, created_at)
        VALUES (?, ?, ?, 'gate_decision', ?, ?, ?, ?, ?, ?)
-       ON CONFLICT(id) DO UPDATE SET decision = excluded.decision, summary = excluded.summary, miner_authored = excluded.miner_authored, created_at = excluded.created_at`,
+       ON CONFLICT(id) DO UPDATE SET decision = excluded.decision, summary = excluded.summary, miner_authored = excluded.miner_authored, created_at = excluded.created_at
+       WHERE NOT (? = 1 AND review_audit.decision = 'close')`,
     )
-      .bind(`gate:${LOOPOVER_NATIVE_SOURCE}:${targetId}@${input.headSha}`, project, targetId, action, LOOPOVER_NATIVE_SOURCE, input.headSha, summary, minerAuthored, nowIso())
+      .bind(`gate:${LOOPOVER_NATIVE_SOURCE}:${targetId}@${input.headSha}`, project, targetId, action, LOOPOVER_NATIVE_SOURCE, input.headSha, summary, minerAuthored, nowIso(), derivedFromConclusion)
       .run();
   } catch (error) {
     // Telemetry must never break finalization.

test/unit/parity-wire.test.ts

@@ -147,6 +147,38 @@ describe("recordNativeGateDecision — flag-gated SHADOW recording into review_a
     expect(rows[0]).toMatchObject({ miner_authored: 1 });
   });
 
+  it("#8825: a conclusion-derived verdict NEVER overwrites a recorded close (the terminal action already happened)", async () => {
+    const env = createTestEnv({ LOOPOVER_REVIEW_PARITY_AUDIT: "true" });
+    // The disposition-aware caller records the real action: the bot closed this PR (CI failure / policy).
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", action: "close", reasonCode: "ci_failing" });
+    // The conclusion-only caller then finalizes with a "success" conclusion, which maps to merge. Before this
+    // fix that clobbered the close and calibration scored the PR as a merge prediction that ended closed.
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", reasonCode: "success" });
+
+    const rows = await rawAll(env, "SELECT * FROM review_audit");
+    expect(rows).toHaveLength(1);
+    expect(rows[0]).toMatchObject({ decision: "close", summary: "ci_failing" });
+  });
+
+  it("#8825: an EXPLICIT action still replaces a recorded close — only conclusion-derived writes are blocked", async () => {
+    const env = createTestEnv({ LOOPOVER_REVIEW_PARITY_AUDIT: "true" });
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", action: "close", reasonCode: "ci_failing" });
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", action: "merge", reasonCode: "recovered" });
+
+    const rows = await rawAll(env, "SELECT * FROM review_audit");
+    expect(rows[0]).toMatchObject({ decision: "merge", summary: "recovered" });
+  });
+
+  it("#8825: a conclusion-derived verdict still updates a non-close row (hold/merge stay latest-wins)", async () => {
+    const env = createTestEnv({ LOOPOVER_REVIEW_PARITY_AUDIT: "true" });
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "failure", reasonCode: "guardrail_hold" });
+    await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", reasonCode: "success" });
+
+    const rows = await rawAll(env, "SELECT * FROM review_audit");
+    expect(rows).toHaveLength(1);
+    expect(rows[0]).toMatchObject({ decision: "merge", summary: "success" });
+  });
+
   it("a re-run at the SAME commit REPLACES the prior decision (latest finalize wins, no duplicate)", async () => {
     const env = createTestEnv({ LOOPOVER_REVIEW_PARITY_AUDIT: "true" });
     await recordNativeGateDecision(env, { project: "owner/repo", pullNumber: 7, headSha: "abc123", conclusion: "success", reasonCode: "all_clear" });