fix(git): correct pull-request size and cycle-time semantics

.github/workflows/scripts/warm-upgrade-sim.sh

@@ -69,6 +69,95 @@ if ! PATH="$WORKDIR/base-venv/bin:$PATH" \
   exit 1
 fi
 
+# A warm installation's git pull requests were settled under the PREVIOUS close
+# contract, which preferred `closed_at`: a request CLOSED, reopened and later
+# MERGED therefore holds the EARLIER close in `closed_on`. The rows below are
+# that state, and the assertion after step 2 is that the upgrade recovered the
+# reported close from the SOURCE rather than copying the settled one — the
+# difference between a 10-hour merge and a 2-hour one, on data no resync has
+# touched. #3362
+seed_git_close_time_warm_state() {
+  echo "=== Seeding the pre-contract git pull-request state ==="
+  source "$REPO_ROOT/src/ingestion/scripts/lib/ch-exec.sh"
+  run_ch <<'SQL'
+-- TWO generations of the same request, the shape bronze actually holds: the
+-- first collection saw it closed, the second saw the merge. The backfill has to
+-- answer from the later one, so a lookup that takes whichever row a part merge
+-- left behind fails here.
+INSERT INTO bronze_github.pull_requests
+    (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at,
+     _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id)
+VALUES
+    ('warm-gh-reopened', 'warm', 'warm', 'closed',
+     '2026-01-01T00:00:00Z', '2026-01-01T02:00:00Z', '',
+     'warm-raw-1', '2026-01-01 03:00:00', '{}', 0),
+    ('warm-gh-reopened', 'warm', 'warm', 'closed',
+     '2026-01-01T00:00:00Z', '2026-01-01T02:00:00Z', '2026-01-01T10:00:00Z',
+     'warm-raw-2', '2026-01-02 00:00:00', '{}', 0);
+-- The pre-#3250 GitLab stream, which only an upgrading installation has, beside
+-- the stream that replaced it. Both name the same request and disagree; the
+-- current stream has to win by declaration, not by extraction time, so the
+-- legacy row here is deliberately the MORE recently extracted of the two.
+CREATE TABLE IF NOT EXISTS bronze_gitlab.merge_requests
+(
+    unique_key Nullable(String), tenant_id Nullable(String), source_id Nullable(String),
+    state Nullable(String), created_at Nullable(String), closed_at Nullable(String),
+    merged_at Nullable(String), _airbyte_raw_id String,
+    _airbyte_extracted_at DateTime64(3), _airbyte_meta String, _airbyte_generation_id UInt32
+)
+ENGINE = MergeTree ORDER BY _airbyte_raw_id;
+INSERT INTO bronze_gitlab.merge_requests
+    (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at,
+     _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id)
+VALUES
+    ('warm-gl-reopened', 'warm', 'warm', 'closed',
+     '2026-01-04T00:00:00Z', '2026-01-04T01:00:00Z', '',
+     'warm-raw-legacy', '2026-01-09 00:00:00', '{}', 0);
+INSERT INTO bronze_gitlab.pull_requests
+    (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at,
+     _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id)
+VALUES
+    ('warm-gl-reopened', 'warm', 'warm', 'merged',
+     '2026-01-04T00:00:00Z', '2026-01-04T01:00:00Z', '2026-01-04T09:00:00Z',
+     'warm-raw-new', '2026-01-05 00:00:00', '{}', 0);
+INSERT INTO silver.class_git_pull_requests
+    (tenant_id, source_id, unique_key, pr_id, state, created_on, closed_on, data_source, _version)
+VALUES
+    ('warm', 'warm', 'warm-gh-reopened', 9001, 'MERGED',
+     '2026-01-01 00:00:00', '2026-01-01 02:00:00', 'insight_github', 1),
+    ('warm', 'warm', 'warm-gl-reopened', 9003, 'MERGED',
+     '2026-01-04 00:00:00', '2026-01-04 01:00:00', 'insight_gitlab', 1),
+    ('warm', 'warm', 'warm-gh-open', 9004, 'OPEN',
+     '2026-01-06 00:00:00', NULL, 'insight_github', 1),
+    ('warm', 'warm', 'warm-bb-recovered', 9002, 'MERGED',
+     '2026-01-05 00:00:00', '2026-01-05 07:00:00', 'insight_bitbucket_cloud', 1);
+SQL
+}
+
+assert_git_close_time_recovered() {
+  echo "=== Asserting the reported close came from the source, not from closed_on ==="
+  source "$REPO_ROOT/src/ingestion/scripts/lib/ch-exec.sh"
+  local got
+  local expected="2026-01-01 10:00:00|2026-01-04 09:00:00|1|1"
+  # ifNull around each part: concat() propagates a NULL, so an unfilled column
+  # would otherwise collapse the whole answer to one '\N' and hide WHICH row
+  # the rollout failed to reach.
+  got="$(printf "SELECT concat(
+      ifNull(toString(maxIf(closed_on_reported, unique_key = 'warm-gh-reopened')), 'NULL'), '|',
+      ifNull(toString(maxIf(closed_on_reported, unique_key = 'warm-gl-reopened')), 'NULL'), '|',
+      toString(countIf(unique_key = 'warm-bb-recovered' AND closed_on_reported IS NULL)), '|',
+      toString(countIf(unique_key = 'warm-gh-open' AND closed_on_reported IS NULL)))
+    FROM silver.class_git_pull_requests
+    WHERE tenant_id = 'warm'" | _ch_http_query | tr -d '\n')"
+  if [[ "${got}" != "${expected}" ]]; then
+    echo "::error title=warm upgrade left a stale close time::Expected '${expected}' — the reopened-then-merged GitHub request reporting its merge from the NEWER bronze generation, the GitLab one reporting the merge the CURRENT stream states rather than the legacy stream's close, and the Bitbucket and still-open rows reporting nothing — but got '${got}'. The rollout must recompute the reported close from bronze under the merged_at-first contract, not copy the settled closed_on — see #3362."
+    exit 1
+  fi
+  echo "=== Reported close recovered from the source, newest generation and current stream ==="
+}
+
+seed_git_close_time_warm_state
+
 echo "=== Step 2: deploy the working tree onto the warm state ==="
 dbt_venv_for "$REPO_ROOT/src/ingestion/scripts/bootstrap-db/pins.env" "$WORKDIR/branch-venv"
 if ! PATH="$WORKDIR/branch-venv/bin:$PATH" \
@@ -77,4 +166,6 @@ if ! PATH="$WORKDIR/branch-venv/bin:$PATH" \
   exit 1
 fi
 
+assert_git_close_time_recovered
+
 echo "=== Warm upgrade OK: every relation the gold build reads is covered by a migration or heal ==="

src/backend/services/analytics/src/domain/metric_definitions/passport.rs

@@ -19,6 +19,10 @@ const HEADER: &str = "\
 Generated from `registry.yaml` by `analytics passports`. Do not edit by hand —
 regenerate and commit. A drift test (`metric_definitions::passport`) fails when
 this file and the registry disagree.
+
+`median(x)` is the textbook median: on an even sample the two middle values are
+averaged, so the answer need not be a value any observation took. A percentile
+is an order statistic instead, and always answers with one that did.
 ";
 
 /// Render the passport document from the builtin registry. Deterministic:

src/backend/services/analytics/src/domain/metric_definitions/passports.md

@@ -4,6 +4,10 @@ Generated from `registry.yaml` by `analytics passports`. Do not edit by hand —
 regenerate and commit. A drift test (`metric_definitions::passport`) fails when
 this file and the registry disagree.
 
+`median(x)` is the textbook median: on an even sample the two middle values are
+averaged, so the answer need not be a value any observation took. A percentile
+is an order statistic instead, and always answers with one that did.
+
 ## ci.runs — CI runs
 
 - Source: ci (ci_metric_observations)
@@ -450,7 +454,7 @@ this file and the registry disagree.
 - Reads: pr_change_size
 - Formula: median(pr_change_size)
 - Shape: integer, lower_is_better, unit lines
-- Notes: Median diff size of authored pull requests (lines added plus removed). Smaller requests are easier to review. Sources that do not report line counts contribute no values.
+- Notes: Median diff size of authored pull requests (lines added plus removed), dated by the day the request was OPENED in UTC and counted whatever state the request reached. Smaller requests are easier to review. A source that never reported line counts contributes no value; a request whose counts were reported as zero — a rename or a mode change — contributes a zero, which is an observed diff of no lines rather than an absence.
 
 ## git.pr_commits — Commits per PR
 
@@ -465,16 +469,16 @@ this file and the registry disagree.
 - Source: git (git_metric_observations)
 - Reads: pr_cycle_hours
 - Formula: median(pr_cycle_hours)
-- Shape: decimal, lower_is_better, unit h
-- Notes: Median hours from opening a pull request to merging it, over requests merged in the period.
+- Shape: decimal, neutral, unit h
+- Notes: Median hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Mostly waiting — for a reviewer, for a build, for someone to press merge — so it describes the path a change travels rather than the person who opened it. A source that does not report a merge time contributes no duration.
 
 ## git.pr_cycle_time_p75_h — PR cycle time (p75)
 
 - Source: git (git_metric_observations)
 - Reads: pr_cycle_hours
 - Formula: p75(pr_cycle_hours)
-- Shape: decimal, lower_is_better, unit h
-- Notes: 75th percentile of hours from opening a pull request to merging it, over requests merged in the period.
+- Shape: decimal, neutral, unit h
+- Notes: 75th percentile of hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Reads the same waiting as the median and describes the same path, so it too is not a statement about the person who opened the request.
 
 ## git.first_review_time_h — Time to first review
 

src/backend/services/analytics/src/domain/metric_definitions/registry.yaml

@@ -1690,7 +1690,7 @@ metrics:
       - distribution
     label: PR size
     description: Typical diff size per pull request
-    explanation: Median diff size of authored pull requests (lines added plus removed). Smaller requests are easier to review. Sources that do not report line counts contribute no values.
+    explanation: Median diff size of authored pull requests (lines added plus removed), dated by the day the request was OPENED in UTC and counted whatever state the request reached. Smaller requests are easier to review. A source that never reported line counts contributes no value; a request whose counts were reported as zero — a rename or a mode change — contributes a zero, which is an observed diff of no lines rather than an absence.
     unit: lines
     format: integer
     direction: lower_is_better
@@ -1739,10 +1739,10 @@ metrics:
     label: PR cycle time
     short_label: PR cycle
     description: Typical hours from open to merge
-    explanation: Median hours from opening a pull request to merging it, over requests merged in the period.
+    explanation: Median hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Mostly waiting — for a reviewer, for a build, for someone to press merge — so it describes the path a change travels rather than the person who opened it. A source that does not report a merge time contributes no duration.
     unit: h
     format: decimal
-    direction: lower_is_better
+    direction: neutral
     entity_type: person
     computation: median
     peer_cohort_key: org_unit
@@ -1764,10 +1764,10 @@ metrics:
     label: PR cycle time (p75)
     short_label: Cycle p75
     description: Slow-end hours from open to merge
-    explanation: 75th percentile of hours from opening a pull request to merging it, over requests merged in the period.
+    explanation: 75th percentile of hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Reads the same waiting as the median and describes the same path, so it too is not a statement about the person who opened the request.
     unit: h
     format: decimal
-    direction: lower_is_better
+    direction: neutral
     entity_type: person
     computation: !percentile
       q: 0.75

src/backend/services/analytics/src/domain/metric_results/compiler.rs

@@ -51,6 +51,20 @@ pub(crate) const ACCOUNT_ASSIGNMENT_RELATION: &str = "identity.account_assignmen
 /// nobody and never falls through to the email map.
 const EXCLUDED_PERSON_ID: &str = "ffffffff-ffff-ffff-ffff-ffffffffffff";
 
+/// The median a reader means: the average of the two middle values on an even
+/// sample. `quantileExact` takes an index instead, so it answers with the upper
+/// middle — a person with two observations is reported at the slower of them,
+/// and on a `lower_is_better` metric that always reads unfavourably.
+/// `quantileExactInclusive` interpolates to the textbook definition and stays
+/// exact, so nothing is approximated by a sketch. A percentile keeps
+/// `quantileExact`: unlike the median it has no second definition to match.
+/// #3362
+const MEDIAN_AGGREGATE: &str = "quantileExactInclusiveIf";
+
+/// `MEDIAN_AGGREGATE` with the `OrNull` combinator, for the arms that must
+/// answer an empty window with null rather than the type's default.
+const MEDIAN_AGGREGATE_OR_NULL: &str = "quantileExactInclusiveIfOrNull";
+
 /// Columns a resolved observation subquery re-exposes to the query above it.
 /// `entity_id` is absent: the subquery replaces it with the canonical person id,
 /// so every outer clause reads unchanged.
@@ -898,7 +912,7 @@ fn grouped_value_expr(def: &MetricDefinition) -> String {
             )
         }
         ComputationSpec::Median { .. } => {
-            "quantileExactIf(0.5)(value, value IS NOT NULL)".to_owned()
+            format!("{MEDIAN_AGGREGATE}(0.5)(value, value IS NOT NULL)")
         }
         ComputationSpec::Percentile { q, .. } => {
             format!("quantileExactIf({q})(value, value IS NOT NULL)")
@@ -945,7 +959,7 @@ fn grouped_value_expr_within(
         }
         ComputationSpec::Median { .. } => {
             let window = window_term(window, params);
-            format!("quantileExactIf(0.5)(value, value IS NOT NULL{window})")
+            format!("{MEDIAN_AGGREGATE}(0.5)(value, value IS NOT NULL{window})")
         }
         ComputationSpec::Percentile { q, .. } => {
             let window = window_term(window, params);
@@ -1349,13 +1363,17 @@ fn push_peer_stat_selects(selects: &mut String, item_index: usize) {
     let aliases = peer_aliases(item_index);
     let observed = format!("peer.{value} IS NOT NULL");
     let pool = format!("uniqExactIf(peer.entity_id, {observed})");
-    let quantiles = format!("quantilesExactIf(0.25, 0.5, 0.75)(peer.{value}, {observed})");
+    // p25 and p75 are order statistics and keep `quantilesExact`; the peer
+    // median answers the same question as the period one and so reads the same
+    // textbook definition, or a cohort of an even size disagrees with itself.
+    let quantiles = format!("quantilesExactIf(0.25, 0.75)(peer.{value}, {observed})");
+    let median_expr = format!("{MEDIAN_AGGREGATE}(0.5)(peer.{value}, {observed})");
     let _ = write!(
         selects,
         ",
             if({pool} >= {min_peer_n}, toNullable({quantiles}[1]), NULL) AS {p25},
-            if({pool} >= {min_peer_n}, toNullable({quantiles}[2]), NULL) AS {median},
-            if({pool} >= {min_peer_n}, toNullable({quantiles}[3]), NULL) AS {p75},
+            if({pool} >= {min_peer_n}, toNullable({median_expr}), NULL) AS {median},
+            if({pool} >= {min_peer_n}, toNullable({quantiles}[2]), NULL) AS {p75},
             if({pool} >= {min_peer_n}, minIfOrNull(peer.{value}, {observed}), NULL) AS {min},
             if({pool} >= {min_peer_n}, maxIfOrNull(peer.{value}, {observed}), NULL) AS {max},
             toUInt64({pool}) AS {n}",
@@ -1449,7 +1467,7 @@ fn item_value_expr(
             params.push(value.measure_key.clone());
             let window = window_term(window, params);
             format!(
-                "quantileExactIfOrNull(0.5)(value, source_key = ? AND measure_key = ? AND value IS NOT NULL{window})"
+                "{MEDIAN_AGGREGATE_OR_NULL}(0.5)(value, source_key = ? AND measure_key = ? AND value IS NOT NULL{window})"
             )
         }
         ComputationSpec::Percentile { value, q } => {
@@ -3321,14 +3339,22 @@ mod tests {
             )));
             assert!(query.sql.contains(&format!("AS m{item}_target")));
         }
-        // Quartiles come from one `quantilesExactIf` per item (single sort),
-        // not three separate `quantileExactIf` calls.
+        // The two order statistics come from one `quantilesExactIf` per item
+        // (single sort), not two separate `quantileExactIf` calls, and the peer
+        // median reads the same textbook aggregate as every other median view.
         for item in 0..2 {
             assert!(query.sql.contains(&format!(
-                "quantilesExactIf(0.25, 0.5, 0.75)(peer.m{item}, peer.m{item} IS NOT NULL)"
+                "quantilesExactIf(0.25, 0.75)(peer.m{item}, peer.m{item} IS NOT NULL)"
+            )));
+            assert!(query.sql.contains(&format!(
+                "quantileExactInclusiveIf(0.5)(peer.m{item}, peer.m{item} IS NOT NULL)"
             )));
         }
         assert!(!query.sql.contains("quantileExactIf(0.25)"));
+        assert!(
+            !query.sql.contains("quantilesExactIf(0.25, 0.5, 0.75)"),
+            "the peer median must not come back as the middle order statistic"
+        );
         // The cohort relation is canonical-grained (one row per person and
         // cohort_key, contested membership already dropped), so the pool reads
         // it straight. A collapse here would repair a broken input silently.
@@ -3419,9 +3445,10 @@ mod tests {
         ] {
             assert!(
                 query.sql.contains(
-                    "quantileExactIfOrNull(0.5)(value, source_key = ? AND measure_key = ?"
+                    "quantileExactInclusiveIfOrNull(0.5)(value, source_key = ? AND measure_key = ?"
                 ),
-                "median must batch as an OrNull quantile column"
+                "median must batch as an OrNull quantile column, interpolating both \
+                 middle values on an even sample"
             );
             assert_eq!(query.sql.matches('?').count(), query.params.len());
         }
@@ -3439,13 +3466,13 @@ mod tests {
         );
         assert!(
             ts.sql
-                .contains("quantileExactIf(0.5)(value, value IS NOT NULL)")
+                .contains("quantileExactInclusiveIf(0.5)(value, value IS NOT NULL)")
         );
         assert!(ts.sql.contains("GROUP BY GROUPING SETS"));
         let bd = compile_breakdown_query(&median_metric(), &request(), &["source".to_owned()], &[]);
         assert!(
             bd.sql
-                .contains("quantileExactIf(0.5)(value, value IS NOT NULL)")
+                .contains("quantileExactInclusiveIf(0.5)(value, value IS NOT NULL)")
         );
     }