From f024597d2bb60f69afa392999514665b8482801c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 09:09:28 -0700 Subject: [PATCH] fix(selfhost): source an open PR's Grafana verdict from review_audit, not dead advisories The exporter's still-open-PR verdict was joined against advisories, which only ever holds a PRE-gate rules-severity summary (always neutral/action_required in practice) -- never the gate's own actual merge/close/hold decision. That decision already has a live, correct, continuously-populated home: review_audit's gittensory-native gate_decision rows (written unconditionally on self-host by recordNativeGateDecision). Point the exporter there instead so a still-open PR's verdict reflects the gate's real, current state instead of an eternal placeholder. Verified against live production Postgres: a still-open PR now shows its actual hold/merge/close decision instead of always 'manual', while merged/closed PRs (the majority, from PR #3512) are unaffected. Closes #3511 --- scripts/export-grafana-reporting-db.sh | 94 ++++++------ test/unit/selfhost-grafana-reporting.test.ts | 151 ++++++++++++++----- 2 files changed, 164 insertions(+), 81 deletions(-) diff --git a/scripts/export-grafana-reporting-db.sh b/scripts/export-grafana-reporting-db.sh index 74304dfb7d..bd5b661129 100644 --- a/scripts/export-grafana-reporting-db.sh +++ b/scripts/export-grafana-reporting-db.sh @@ -195,21 +195,20 @@ if pg_enabled; then fi fi - if pg_table_exists "pull_requests" && pg_table_exists "advisories"; then + if pg_table_exists "pull_requests" && pg_table_exists "review_audit"; then PR_CSV="$(csv_temp_file "pull-requests")" pg_copy_csv " -WITH latest_advisories AS ( +WITH latest_gate_decisions AS ( SELECT - repo_full_name, - pull_number, - conclusion, - updated_at, + target_id, + decision, + created_at, ROW_NUMBER() OVER ( - PARTITION BY repo_full_name, pull_number - ORDER BY updated_at DESC, id DESC + PARTITION BY target_id + ORDER BY created_at DESC, id DESC ) AS rn - FROM advisories - WHERE pull_number IS NOT NULL + FROM review_audit + WHERE event_type = 'gate_decision' AND source = 'gittensory-native' ), current_pull_requests AS ( SELECT @@ -219,34 +218,37 @@ current_pull_requests AS ( CASE WHEN lower(p.state) = 'closed' AND p.merged_at IS NOT NULL THEN 'merged' WHEN lower(p.state) = 'closed' THEN 'closed' - WHEN a.conclusion IN ('failure', 'action_required', 'neutral') THEN 'manual' - WHEN a.conclusion IS NOT NULL THEN 'commented' + WHEN g.decision = 'hold' THEN 'manual' + WHEN g.decision = 'close' THEN 'manual' + WHEN g.decision = 'merge' THEN 'commented' ELSE 'manual' END AS status, -- The terminal PR outcome (state/merged_at) is the source of truth and takes precedence, exactly like - -- status above -- otherwise a merged/closed PR whose advisories.conclusion happens to read - -- neutral/action_required (the only two values GitHub Checks reports for the gate's own check run today) - -- reports verdict='manual' forever, even though the PR is long since merged/closed (#3511 dashboard bug). + -- status above. For a still-open PR, the live gate's own last recorded decision is the real signal: + -- review_audit (source='gittensory-native', written by recordNativeGateDecision) is the ONLY writer of + -- gate_decision rows and is ALWAYS ON for a self-host instance. The older advisories table never carried + -- this signal at all -- it only ever holds a PRE-gate rules-severity summary (buildPullRequestAdvisory), + -- which is unrelated to the gate's own merge/close/hold decision and reads neutral/action_required for + -- essentially every PR, which is why a still-open PR's verdict was an eternal 'manual' placeholder + -- regardless of the gate's actual decision (#3511 follow-up). CASE WHEN lower(p.state) = 'closed' AND p.merged_at IS NOT NULL THEN 'merge' WHEN lower(p.state) = 'closed' THEN 'close' - WHEN a.conclusion = 'success' THEN 'merge' - WHEN a.conclusion = 'failure' THEN 'close' - WHEN a.conclusion IN ('action_required', 'neutral') THEN 'manual' - WHEN a.conclusion = 'skipped' THEN 'ignore' + WHEN g.decision = 'merge' THEN 'merge' + WHEN g.decision = 'close' THEN 'close' + WHEN g.decision = 'hold' THEN 'manual' ELSE NULL END AS verdict, p.title AS title, p.created_at AS created_at, CASE - WHEN a.updated_at IS NOT NULL AND a.updated_at > p.updated_at THEN a.updated_at + WHEN g.created_at IS NOT NULL AND g.created_at > p.updated_at THEN g.created_at ELSE p.updated_at END AS updated_at FROM pull_requests p - LEFT JOIN latest_advisories a - ON a.repo_full_name = p.repo_full_name - AND a.pull_number = p.number - AND a.rn = 1 + LEFT JOIN latest_gate_decisions g + ON g.target_id = p.repo_full_name || '#' || p.number + AND g.rn = 1 ) SELECT repo, @@ -355,21 +357,20 @@ if ! source_table_exists "pull_requests" && fi fi -if source_table_exists "pull_requests" && source_table_exists "advisories"; then +if source_table_exists "pull_requests" && source_table_exists "review_audit"; then sqlite3 -cmd ".timeout 5000" "$APP_DB" " ATTACH '$TMP_DB_SQL' AS report; -WITH latest_advisories AS ( +WITH latest_gate_decisions AS ( SELECT - repo_full_name, - pull_number, - conclusion, - updated_at, + target_id, + decision, + created_at, ROW_NUMBER() OVER ( - PARTITION BY repo_full_name, pull_number - ORDER BY updated_at DESC, rowid DESC + PARTITION BY target_id + ORDER BY created_at DESC, rowid DESC ) AS rn - FROM main.advisories - WHERE pull_number IS NOT NULL + FROM main.review_audit + WHERE event_type = 'gate_decision' AND source = 'gittensory-native' ), current_pull_requests AS ( SELECT @@ -379,33 +380,32 @@ current_pull_requests AS ( CASE WHEN lower(p.state) = 'closed' AND p.merged_at IS NOT NULL THEN 'merged' WHEN lower(p.state) = 'closed' THEN 'closed' - WHEN a.conclusion IN ('failure', 'action_required', 'neutral') THEN 'manual' - WHEN a.conclusion IS NOT NULL THEN 'commented' + WHEN g.decision = 'hold' THEN 'manual' + WHEN g.decision = 'close' THEN 'manual' + WHEN g.decision = 'merge' THEN 'commented' ELSE 'manual' END AS status, -- Mirrors status's precedence above (see the matching comment in the Postgres-source block): the terminal - -- PR outcome wins over advisories.conclusion, or a merged/closed PR reports verdict='manual' forever - -- (#3511). + -- PR outcome wins, and a still-open PR's verdict comes from review_audit's live gate_decision rows, not the + -- (pre-gate, always neutral/action_required) advisories table (#3511 follow-up). CASE WHEN lower(p.state) = 'closed' AND p.merged_at IS NOT NULL THEN 'merge' WHEN lower(p.state) = 'closed' THEN 'close' - WHEN a.conclusion = 'success' THEN 'merge' - WHEN a.conclusion = 'failure' THEN 'close' - WHEN a.conclusion IN ('action_required', 'neutral') THEN 'manual' - WHEN a.conclusion = 'skipped' THEN 'ignore' + WHEN g.decision = 'merge' THEN 'merge' + WHEN g.decision = 'close' THEN 'close' + WHEN g.decision = 'hold' THEN 'manual' ELSE NULL END AS verdict, p.title AS title, p.created_at AS created_at, CASE - WHEN a.updated_at IS NOT NULL AND a.updated_at > p.updated_at THEN a.updated_at + WHEN g.created_at IS NOT NULL AND g.created_at > p.updated_at THEN g.created_at ELSE p.updated_at END AS updated_at FROM main.pull_requests p - LEFT JOIN latest_advisories a - ON a.repo_full_name = p.repo_full_name - AND a.pull_number = p.number - AND a.rn = 1 + LEFT JOIN latest_gate_decisions g + ON g.target_id = p.repo_full_name || '#' || p.number + AND g.rn = 1 ) INSERT INTO report.review_targets ( repo, diff --git a/test/unit/selfhost-grafana-reporting.test.ts b/test/unit/selfhost-grafana-reporting.test.ts index 212bf630ad..9d8199bad0 100644 --- a/test/unit/selfhost-grafana-reporting.test.ts +++ b/test/unit/selfhost-grafana-reporting.test.ts @@ -71,7 +71,7 @@ case "$args" in echo 'unexpected psql meta-command copy' >&2 exit 9 ;; - *"information_schema.tables"*"pull_requests"*|*"information_schema.tables"*"advisories"*|*"information_schema.tables"*"review_targets"*|*"information_schema.tables"*"ai_usage_events"*) + *"information_schema.tables"*"pull_requests"*|*"information_schema.tables"*"advisories"*|*"information_schema.tables"*"review_targets"*|*"information_schema.tables"*"ai_usage_events"*|*"information_schema.tables"*"review_audit"*) printf '1\\n' ;; *"information_schema.columns"*"ai_usage_events"*"estimated_neurons"*|\ @@ -180,7 +180,7 @@ function failingCopyPsql(root: string): string { `#!/bin/sh args="$*" case "$args" in - *"information_schema.tables"*"pull_requests"*|*"information_schema.tables"*"advisories"*|*"information_schema.tables"*"review_targets"*|*"information_schema.tables"*"ai_usage_events"*) + *"information_schema.tables"*"pull_requests"*|*"information_schema.tables"*"advisories"*|*"information_schema.tables"*"review_targets"*|*"information_schema.tables"*"ai_usage_events"*|*"information_schema.tables"*"review_audit"*) printf '1\\n' ;; *"information_schema.columns"*"ai_usage_events"*"estimated_neurons"*) @@ -234,26 +234,27 @@ esac ('JSONbored/gittensory', 1690, 'fresh advisory PR', 'open', 'JSONbored', NULL, '2026-06-28T21:00:00Z', '2026-06-28T21:39:58Z'), ('JSONbored/gittensory', 1691, 'fresh merged PR', 'closed', 'tmimmanuel', '2026-06-28T21:46:51Z', '2026-06-28T21:30:00Z', '2026-06-28T21:47:36Z'); - CREATE TABLE advisories ( - repo_full_name TEXT NOT NULL, - pull_number INTEGER, - conclusion TEXT NOT NULL, - updated_at TEXT NOT NULL + CREATE TABLE review_audit ( + id TEXT NOT NULL, + target_id TEXT NOT NULL, + event_type TEXT NOT NULL, + decision TEXT, + source TEXT NOT NULL, + created_at TEXT NOT NULL ); - INSERT INTO advisories (repo_full_name, pull_number, conclusion, updated_at) + INSERT INTO review_audit (id, target_id, event_type, decision, source, created_at) VALUES - ('JSONbored/gittensory', 1690, 'failure', '2026-06-28T21:25:00Z'), - ('JSONbored/gittensory', 1690, 'neutral', '2026-06-28T21:40:00Z'), - ('JSONbored/gittensory', 1691, 'success', '2026-06-28T21:47:40Z'); + ('g1', 'JSONbored/gittensory#1690', 'gate_decision', 'close', 'gittensory-native', '2026-06-28T21:25:00Z'), + ('g2', 'JSONbored/gittensory#1690', 'gate_decision', 'hold', 'gittensory-native', '2026-06-28T21:40:00Z'), + ('g3', 'JSONbored/gittensory#1691', 'gate_decision', 'merge', 'gittensory-native', '2026-06-28T21:47:40Z'); `); runExporter(root, appDb, outDb); expect(sqlite(outDb, "PRAGMA quick_check;")).toBe("ok"); expect(sqlite(outDb, "SELECT count(*) FROM review_targets;")).toBe("3"); - // Latest advisory conclusion for #1690 is 'neutral' -- counted as 'manual' (matches gateHeld's held-for-review - // definition in src/signals/engine.ts, not 'commented'/'comment') so the dashboard's manual-review panel - // reflects the same held state the live app itself surfaces via the configured manual-review label. + // #1690's latest gate_decision is 'hold' (superseding the earlier 'close') -- the ROW_NUMBER "latest wins" + // ordering is exercised here, not just a single-row join. expect(sqlite(outDb, "SELECT submitter || '|' || status || '|' || verdict || '|' || updated_at FROM review_targets WHERE repo='JSONbored/gittensory' AND number=1690;")).toBe( "JSONbored|manual|manual|2026-06-28T21:40:00Z", ); @@ -263,7 +264,7 @@ esac expect(sqlite(outDb, "SELECT title FROM review_targets WHERE repo='JSONbored/gittensory' AND number=1049;")).toBe("historical PR"); }); - it("REGRESSION (#3511 dashboard bug): a merged/closed PR reports its OWN verdict, not 'manual', even though advisories.conclusion is stuck at neutral/action_required", () => { + it("REGRESSION (#3511 dashboard bug): a merged/closed PR reports its OWN verdict, not 'manual', regardless of what review_audit's live gate_decision says", () => { const root = tmpRoot(); const appDb = join(root, "app.sqlite"); const outDb = join(root, "reporting.sqlite"); @@ -280,24 +281,27 @@ esac ); INSERT INTO pull_requests (repo_full_name, number, title, state, author_login, merged_at, created_at, updated_at) VALUES - ('JSONbored/gittensory', 2001, 'merged despite a stuck neutral advisory', 'closed', 'JSONbored', '2026-07-05T09:43:12Z', '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), - ('JSONbored/gittensory', 2002, 'closed (not merged) despite a stuck action_required advisory', 'closed', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), - ('JSONbored/gittensory', 2003, 'still open, genuinely held for manual review', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'); - - CREATE TABLE advisories ( - repo_full_name TEXT NOT NULL, - pull_number INTEGER, - conclusion TEXT NOT NULL, - updated_at TEXT NOT NULL + ('JSONbored/gittensory', 2001, 'merged despite a stale held gate_decision', 'closed', 'JSONbored', '2026-07-05T09:43:12Z', '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), + ('JSONbored/gittensory', 2002, 'closed (not merged) despite a stale held gate_decision', 'closed', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), + ('JSONbored/gittensory', 2003, 'still open, genuinely held for manual review', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), + ('JSONbored/gittensory', 2004, 'still open, no gate_decision recorded yet', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'); + + CREATE TABLE review_audit ( + id TEXT NOT NULL, + target_id TEXT NOT NULL, + event_type TEXT NOT NULL, + decision TEXT, + source TEXT NOT NULL, + created_at TEXT NOT NULL ); - INSERT INTO advisories (repo_full_name, pull_number, conclusion, updated_at) + INSERT INTO review_audit (id, target_id, event_type, decision, source, created_at) VALUES - -- Reproduces the live production data: the gate's own advisories.conclusion is stuck at - -- neutral/action_required for every PR (a separate, unrelated pipeline gap) -- a merged/closed PR must - -- not be forced through those branches into verdict='manual' just because this column never resolved. - ('JSONbored/gittensory', 2001, 'neutral', '2026-07-05T09:31:00Z'), - ('JSONbored/gittensory', 2002, 'action_required', '2026-07-05T09:31:00Z'), - ('JSONbored/gittensory', 2003, 'neutral', '2026-07-05T09:31:00Z'); + -- Reproduces the reported production shape: the gate held these PRs (a live, real 'hold' decision) at + -- some point in the past -- a merged/closed PR must not be forced into verdict='manual' forever just + -- because ITS OWN last-recorded live decision, before the terminal outcome, happened to be a hold. + ('g1', 'JSONbored/gittensory#2001', 'gate_decision', 'hold', 'gittensory-native', '2026-07-05T09:31:00Z'), + ('g2', 'JSONbored/gittensory#2002', 'gate_decision', 'hold', 'gittensory-native', '2026-07-05T09:31:00Z'), + ('g3', 'JSONbored/gittensory#2003', 'gate_decision', 'hold', 'gittensory-native', '2026-07-05T09:31:00Z'); `); runExporter(root, appDb, outDb); @@ -305,10 +309,89 @@ esac expect(sqlite(outDb, "PRAGMA quick_check;")).toBe("ok"); expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=2001;")).toBe("merged|merge"); expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=2002;")).toBe("closed|close"); - // A genuinely still-open PR is unaffected by the fix -- it has no terminal state/merged_at to take - // precedence, so it still falls through to the (separately broken, out of scope here) advisories.conclusion - // mapping, exactly as before. + // A genuinely still-open PR reflects its live gate_decision verbatim (#3511 follow-up: this is the real fix + // -- a still-open PR now reports the gate's ACTUAL current decision instead of an eternal placeholder). expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=2003;")).toBe("manual|manual"); + // No gate_decision row at all (e.g. a brand-new PR the gate hasn't evaluated yet) fails safe: status='manual' + // (something needs a look), verdict=NULL (no real signal to report, never fabricated). + expect(sqlite(outDb, "SELECT status || '|' || (verdict IS NULL) FROM review_targets WHERE repo='JSONbored/gittensory' AND number=2004;")).toBe("manual|1"); + }); + + it("a still-open PR whose live gate_decision is 'merge' or 'close' reports that verdict directly, with a distinct status from a genuine 'hold'", () => { + const root = tmpRoot(); + const appDb = join(root, "app.sqlite"); + const outDb = join(root, "reporting.sqlite"); + sqlite(appDb, ` + CREATE TABLE pull_requests ( + repo_full_name TEXT NOT NULL, + number INTEGER NOT NULL, + title TEXT NOT NULL, + state TEXT NOT NULL, + author_login TEXT, + merged_at TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL + ); + INSERT INTO pull_requests (repo_full_name, number, title, state, author_login, merged_at, created_at, updated_at) + VALUES + ('JSONbored/gittensory', 3001, 'gate says merge, GitHub has not caught up yet', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'), + ('JSONbored/gittensory', 3002, 'gate says close, GitHub has not caught up yet', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'); + + CREATE TABLE review_audit ( + id TEXT NOT NULL, + target_id TEXT NOT NULL, + event_type TEXT NOT NULL, + decision TEXT, + source TEXT NOT NULL, + created_at TEXT NOT NULL + ); + INSERT INTO review_audit (id, target_id, event_type, decision, source, created_at) + VALUES + ('g1', 'JSONbored/gittensory#3001', 'gate_decision', 'merge', 'gittensory-native', '2026-07-05T09:31:00Z'), + ('g2', 'JSONbored/gittensory#3002', 'gate_decision', 'close', 'gittensory-native', '2026-07-05T09:31:00Z'); + `); + + runExporter(root, appDb, outDb); + + expect(sqlite(outDb, "PRAGMA quick_check;")).toBe("ok"); + expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=3001;")).toBe("commented|merge"); + expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=3002;")).toBe("manual|close"); + }); + + it("ignores a gate_decision row from another source (e.g. 'reviewbot', the parity harness's authoritative side) so a shadow-comparison row never masquerades as the live verdict", () => { + const root = tmpRoot(); + const appDb = join(root, "app.sqlite"); + const outDb = join(root, "reporting.sqlite"); + sqlite(appDb, ` + CREATE TABLE pull_requests ( + repo_full_name TEXT NOT NULL, + number INTEGER NOT NULL, + title TEXT NOT NULL, + state TEXT NOT NULL, + author_login TEXT, + merged_at TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL + ); + INSERT INTO pull_requests (repo_full_name, number, title, state, author_login, merged_at, created_at, updated_at) + VALUES ('JSONbored/gittensory', 4001, 'only a reviewbot shadow row exists', 'open', 'JSONbored', NULL, '2026-07-05T09:30:00Z', '2026-07-05T09:43:12Z'); + + CREATE TABLE review_audit ( + id TEXT NOT NULL, + target_id TEXT NOT NULL, + event_type TEXT NOT NULL, + decision TEXT, + source TEXT NOT NULL, + created_at TEXT NOT NULL + ); + INSERT INTO review_audit (id, target_id, event_type, decision, source, created_at) + VALUES ('g1', 'JSONbored/gittensory#4001', 'gate_decision', 'merge', 'reviewbot', '2026-07-05T09:31:00Z'); + `); + + runExporter(root, appDb, outDb); + + expect(sqlite(outDb, "PRAGMA quick_check;")).toBe("ok"); + expect(sqlite(outDb, "SELECT status || '|' || (verdict IS NULL) FROM review_targets WHERE repo='JSONbored/gittensory' AND number=4001;")).toBe("manual|1"); }); it("falls back to legacy review_targets when the current PR cache is absent", () => {