fix(metrics): bucket closed PRs by close time, not their last-updated time

src/services/public-quality-metrics.ts

@@ -10,6 +10,7 @@ import type { GateOutcomeRecord, PullRequestRecord } from "../types";
 import { nowIso } from "../utils/json";
 import { buildGatePrecisionReport, loadGatePrecisionReport, type GatePrecisionReport } from "./gate-precision";
 import { buildSlopOutcomeCalibration, buildRepoOutcomeCalibration, type SlopOutcomeCalibration } from "./outcome-calibration";
+import { closedAtMs } from "./review-recap";
 
 export const PUBLIC_QUALITY_TREND_WEEKS = 8;
 /** Below this per-week gate-block sample the weekly false-positive rate is too noisy to publish. */
@@ -92,6 +93,13 @@ function parseStamp(value: string | null | undefined): number | null {
   return Number.isFinite(parsed) ? parsed : null;
 }
 
+/** Adapt closedAtMs' NaN-means-skip contract to the null-means-skip shape parseStamp already uses at the
+ *  bucketing call site, so a closed PR whose closedAt and updatedAt are both unusable is skipped, not
+ *  bucketed at NaN (#9700). */
+function finiteOrNull(ms: number): number | null {
+  return Number.isFinite(ms) ? ms : null;
+}
+
 /** UTC Monday (YYYY-MM-DD) containing `ms`. */
 export function isoWeekStart(ms: number): string {
   const d = new Date(ms);
@@ -147,7 +155,11 @@ export function buildPublicQualityTrend(
   for (const pr of pullRequests) {
     const terminal = terminalOutcome(pr);
     if (!terminal) continue;
-    const stamp = parseStamp(terminal === "merged" ? pr.mergedAt : pr.updatedAt ?? pr.createdAt);
+    // Closed PRs bucket by their actual close time (closedAtMs), not updatedAt: GitHub bumps updatedAt on
+    // every later comment/label/edit, which would drift a long-closed PR into a recent week and skew the
+    // public merge-ratio trend. closedAtMs already carries the updatedAt fallback for rows written before
+    // closed_at was persisted, so no createdAt fallback is needed (#9700).
+    const stamp = terminal === "merged" ? parseStamp(pr.mergedAt) : finiteOrNull(closedAtMs(pr));
     if (stamp == null) continue;
     const idx = weekBucketIndex(currentStartMs, stamp, weeks);
     if (idx == null) continue;

src/services/review-recap.ts

@@ -61,7 +61,7 @@ type ReviewRecapInputs = {
  *  one, else `updatedAt` (always populated on write — see toPullRequestRecordFromRow) as a fallback, since a
  *  closed PR's last update IS effectively its close time absent a dedicated column. Returns NaN (never
  *  counted as "in window") only when BOTH are missing/unparseable. */
-function closedAtMs(pr: { closedAt?: string | null | undefined; updatedAt?: string | null | undefined }): number {
+export function closedAtMs(pr: { closedAt?: string | null | undefined; updatedAt?: string | null | undefined }): number {
   const closed = pr.closedAt ? Date.parse(pr.closedAt) : Number.NaN;
   if (Number.isFinite(closed)) return closed;
   const updated = pr.updatedAt ? Date.parse(pr.updatedAt) : Number.NaN;

test/unit/public-quality-metrics.test.ts

@@ -18,16 +18,24 @@ const GENERATED = "2026-06-22T12:00:00.000Z";
 function pr(
   number: number,
   outcome: "merged" | "closed" | "open",
-  opts: { mergedAt?: string; updatedAt?: string; createdAt?: string; slopBand?: string; slopRisk?: number } = {},
+  opts: {
+    mergedAt?: string;
+    updatedAt?: string | null;
+    createdAt?: string;
+    closedAt?: string | null;
+    slopBand?: string;
+    slopRisk?: number;
+  } = {},
 ): PullRequestRecord {
   return {
     repoFullName: "owner/repo",
     number,
     title: `PR ${number}`,
     state: outcome === "open" ? "open" : "closed",
     mergedAt: opts.mergedAt ?? (outcome === "merged" ? "2026-06-20T00:00:00.000Z" : null),
-    updatedAt: opts.updatedAt ?? "2026-06-20T00:00:00.000Z",
+    updatedAt: opts.updatedAt === undefined ? "2026-06-20T00:00:00.000Z" : opts.updatedAt,
     createdAt: opts.createdAt ?? "2026-06-01T00:00:00.000Z",
+    ...(opts.closedAt === undefined ? {} : { closedAt: opts.closedAt }),
     slopBand: opts.slopBand,
     slopRisk: opts.slopRisk,
     labels: [],
@@ -137,9 +145,11 @@ describe("buildPublicQualityTrend", () => {
         pr(1, "closed", { updatedAt: "also-not-a-date", createdAt: "still-not-a-date" }),
         pr(2, "merged", { mergedAt: `${currentMonday}T12:00:00.000Z` }),
         {
+          // A closed PR carrying only createdAt no longer counts: closedAtMs falls back to updatedAt (absent
+          // here), never to createdAt, so this PR is skipped rather than bucketed at its open time (#9700).
           repoFullName: "owner/repo",
           number: 3,
-          title: "Closed via createdAt",
+          title: "Closed with only createdAt is skipped",
           state: "closed",
           mergedAt: null,
           labels: [],
@@ -154,7 +164,62 @@ describe("buildPublicQualityTrend", () => {
       gateBlocked: 1,
       gateBlockedThenMerged: 1,
       outcomesMerged: 1,
-      outcomesClosed: 1,
+      outcomesClosed: 0,
+    });
+  });
+
+  describe("closed PRs bucket by close time, not updatedAt (#9700)", () => {
+    // NOW = 2026-06-22 (week of Mon 2026-06-15). "Week 1" here is a week ~8 weeks earlier; "week 8" is current.
+    const weekStart = (offset: number) => isoWeekStart(NOW - offset * 7 * 86_400_000);
+
+    it("counts a PR closed in an early week there, even when updatedAt was bumped into a later week", () => {
+      const earlyWeek = weekStart(7); // oldest bucket in an 8-week window
+      const trend = buildPublicQualityTrend(
+        [],
+        [pr(1, "closed", { closedAt: `${earlyWeek}T10:00:00.000Z`, updatedAt: `${weekStart(0)}T10:00:00.000Z` })],
+        NOW,
+        8,
+      );
+      // The close week gets the outcome; the (later) updatedAt week does not.
+      expect(trend[0]).toMatchObject({ weekStart: earlyWeek, outcomesClosed: 1 });
+      expect(trend[7]).toMatchObject({ weekStart: weekStart(0), outcomesClosed: 0 });
+    });
+
+    it("falls back to updatedAt's week when closedAt is null (rows written before closed_at was persisted)", () => {
+      const updatedWeek = weekStart(3);
+      const trend = buildPublicQualityTrend(
+        [],
+        [pr(1, "closed", { closedAt: null, updatedAt: `${updatedWeek}T10:00:00.000Z` })],
+        NOW,
+        8,
+      );
+      const idx = trend.findIndex((w) => w.weekStart === updatedWeek);
+      expect(trend[idx]).toMatchObject({ outcomesClosed: 1 });
+      expect(trend.reduce((sum, w) => sum + w.outcomesClosed, 0)).toBe(1);
+    });
+
+    it("skips a closed PR whose closedAt and updatedAt are both missing/unparseable", () => {
+      const trend = buildPublicQualityTrend(
+        [],
+        [pr(1, "closed", { closedAt: null, updatedAt: null })],
+        NOW,
+        8,
+      );
+      expect(trend.reduce((sum, w) => sum + w.outcomesClosed, 0)).toBe(0);
+    });
+
+    it("leaves merged-PR bucketing unchanged (still keyed on mergedAt)", () => {
+      const mergeWeek = weekStart(2);
+      const trend = buildPublicQualityTrend(
+        [],
+        // closedAt/updatedAt point at other weeks, but a merged PR must ignore them and bucket by mergedAt.
+        [pr(1, "merged", { mergedAt: `${mergeWeek}T10:00:00.000Z`, closedAt: `${weekStart(6)}T10:00:00.000Z`, updatedAt: `${weekStart(0)}T10:00:00.000Z` })],
+        NOW,
+        8,
+      );
+      const idx = trend.findIndex((w) => w.weekStart === mergeWeek);
+      expect(trend[idx]).toMatchObject({ outcomesMerged: 1 });
+      expect(trend.reduce((sum, w) => sum + w.outcomesMerged, 0)).toBe(1);
     });
   });