From f85006d860c89a4febb4925c00a751fc20b19be8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 03:10:47 -0700 Subject: [PATCH] fix(orb): stop a conclusion-derived verdict from overwriting a recorded close Both gate_decision writers key the same deterministic row id (gate::#@) 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 JSONbored/loopover#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. --- src/review/parity-wire.ts | 17 +++++++++++++++-- test/unit/parity-wire.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/review/parity-wire.ts b/src/review/parity-wire.ts index c6f03903df..a576c97e4e 100644 --- a/src/review/parity-wire.ts +++ b/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. diff --git a/test/unit/parity-wire.test.ts b/test/unit/parity-wire.test.ts index 13d078c24b..279eb26132 100644 --- a/test/unit/parity-wire.test.ts +++ b/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" });