Skip to content

orb(public-stats): bucket closed PRs by closedAt, not by last-edit time #9700

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

buildPublicQualityTrend buckets each PR's terminal outcome into a weekly bucket — src/services/public-quality-metrics.ts:150:

const stamp = parseStamp(terminal === "merged" ? pr.mergedAt : pr.updatedAt ?? pr.createdAt);

The merged arm uses the true terminal event timestamp. The closed arm uses updatedAt, which GitHub bumps on every later comment, label change, or edit — so a PR closed in week 1 that receives a comment in week 8 is counted in week 8's outcomesClosed. That deflates week 1's mergeRatioPct toward 100% and inflates week 8's toward 0%, on a payload exposed publicly (opt-in publicQualityMetrics, :5).

PullRequestRecord.closedAt exists (src/types.ts:692) and is populated on write from payload.closed_at (src/db/repositories.ts:7291, :7449). The in-repo sibling does it correctly — src/services/review-recap.ts:60-69's closedAtMs, documented as "The best-available terminal timestamp for a closed-unmerged PR: closedAt when GitHub's payload carried one, else updatedAt".

The ?? pr.createdAt third fallback in the public-metrics version is also dead: updatedAt is always written on the row.

Requirements

  • Export closedAtMs from src/services/review-recap.ts (it is currently a private helper) with its existing behaviour and doc comment unchanged, so there is one definition of "when did this PR close".
  • src/services/public-quality-metrics.ts:150 uses it for the closed arm: const stamp = terminal === "merged" ? parseStamp(pr.mergedAt) : finiteOrNull(closedAtMs(pr)); — a PR for which closedAtMs returns NaN is skipped, exactly as parseStamp returning null skips today (:151).
  • The ?? pr.createdAt fallback is removed — closedAtMs already carries the updatedAt fallback, and createdAt was never a close time.
  • No change to the gate-outcome bucketing at :139 (outcome.blockedAt ?? outcome.updatedAt) — that reads a different record type with its own timestamp.
  • No change to terminalOutcome, MIN_GATE_TREND_SAMPLE, or mergeRatioPct.

⚠️ Required pattern: closedAtMs at src/services/review-recap.ts:60-69 is the definition to reuse verbatim, including its NaN-means-skip contract. It does NOT satisfy this issue to hand-copy the pr.closedAt ? ... : pr.updatedAt expression into public-quality-metrics.ts (a second copy of the rule this issue exists to unify); to switch to pr.closedAt alone and drop the updatedAt fallback, which would silently drop every PR closed before closed_at started being persisted; or to add a new closedAt column or migration.

Deliverables

  • closedAtMs is exported from src/services/review-recap.ts and imported by src/services/public-quality-metrics.ts; the expression pr.updatedAt ?? pr.createdAt no longer appears in public-quality-metrics.ts (grep-verifiable).
  • Named regression test: a PR with closedAt in week 1 and updatedAt in week 8 is counted in week 1's outcomesClosed, not week 8's.
  • A closed PR with closedAt: null and a valid updatedAt is still counted, in updatedAt's week — the fallback arm.
  • A closed PR with both closedAt and updatedAt missing/unparseable is skipped entirely and does not appear in any bucket.
  • Merged-PR bucketing is unchanged — asserted by an existing-behaviour case in the same test block.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example switching the field without covering the closedAt: null fallback arm — does not resolve this issue.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on src/**. src/services/public-quality-metrics.ts and src/services/review-recap.ts are inside coverage.include. Every arm of closedAtMs must be exercised from the new call site: closedAt present, closedAt absent with updatedAt present, and both unusable.

Expected Outcome

A publicly-exposed weekly merge-ratio trend attributes a closed PR to the week it was actually closed, so later comment activity on an old PR can no longer move historical weeks. The two consumers of "when did this PR close" share one implementation.

Links & Resources

src/services/public-quality-metrics.ts:128-160; src/services/review-recap.ts:60-74; src/types.ts:692; src/db/repositories.ts:7291, :7449.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions