From 250046bb994984512d85d00d9a5b3776c196c0b8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 08:08:24 -0700 Subject: [PATCH] fix(selfhost): give verdict the same state/merged_at precedence status has (#3511) The Grafana reporting exporter's verdict CASE switched purely on advisories.conclusion, unlike the status CASE right above it (which checks the PR's own terminal state first). Live advisories.conclusion is stuck at neutral/action_required for every PR (a separate, pre-existing gap -- persistAdvisory has zero callers), so verdict was mathematically forced to 'manual' for every merged/closed PR on the maintainer dashboard. Mirror status's precedence into verdict in both the Postgres-source and SQLite-source blocks: check state/merged_at first, fall back to advisories.conclusion only for a PR that's still open. Closes #3511 --- scripts/export-grafana-reporting-db.sh | 33 +++++++++----- test/unit/selfhost-grafana-reporting.test.ts | 48 ++++++++++++++++++++ 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/scripts/export-grafana-reporting-db.sh b/scripts/export-grafana-reporting-db.sh index 702f615647..74304dfb7d 100644 --- a/scripts/export-grafana-reporting-db.sh +++ b/scripts/export-grafana-reporting-db.sh @@ -223,12 +223,17 @@ current_pull_requests AS ( WHEN a.conclusion IS NOT NULL THEN 'commented' ELSE 'manual' END AS status, - CASE a.conclusion - WHEN 'success' THEN 'merge' - WHEN 'failure' THEN 'close' - WHEN 'action_required' THEN 'manual' - WHEN 'neutral' THEN 'manual' - WHEN 'skipped' THEN 'ignore' + -- 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). + 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' ELSE NULL END AS verdict, p.title AS title, @@ -378,12 +383,16 @@ current_pull_requests AS ( WHEN a.conclusion IS NOT NULL THEN 'commented' ELSE 'manual' END AS status, - CASE a.conclusion - WHEN 'success' THEN 'merge' - WHEN 'failure' THEN 'close' - WHEN 'action_required' THEN 'manual' - WHEN 'neutral' THEN 'manual' - WHEN 'skipped' THEN 'ignore' + -- 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). + 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' ELSE NULL END AS verdict, p.title AS title, diff --git a/test/unit/selfhost-grafana-reporting.test.ts b/test/unit/selfhost-grafana-reporting.test.ts index be160dc3e7..212bf630ad 100644 --- a/test/unit/selfhost-grafana-reporting.test.ts +++ b/test/unit/selfhost-grafana-reporting.test.ts @@ -263,6 +263,54 @@ 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", () => { + 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', 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 + ); + INSERT INTO advisories (repo_full_name, pull_number, conclusion, updated_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'); + `); + + 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=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. + expect(sqlite(outDb, "SELECT status || '|' || verdict FROM review_targets WHERE repo='JSONbored/gittensory' AND number=2003;")).toBe("manual|manual"); + }); + it("falls back to legacy review_targets when the current PR cache is absent", () => { const root = tmpRoot(); const appDb = join(root, "app.sqlite");