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");