diff --git a/src/services/public-quality-metrics.ts b/src/services/public-quality-metrics.ts index 2158f54c59..f4bac5b561 100644 --- a/src/services/public-quality-metrics.ts +++ b/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; diff --git a/src/services/review-recap.ts b/src/services/review-recap.ts index 745cb53bf5..ee78b34654 100644 --- a/src/services/review-recap.ts +++ b/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; diff --git a/test/unit/public-quality-metrics.test.ts b/test/unit/public-quality-metrics.test.ts index cf43b172f0..3296676a00 100644 --- a/test/unit/public-quality-metrics.test.ts +++ b/test/unit/public-quality-metrics.test.ts @@ -18,7 +18,14 @@ 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", @@ -26,8 +33,9 @@ function pr( 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); }); });