fix(growth-report): address dsblank re-review (URL builder + sweep)

- admin_api_url: accept http+https on-prem bases (reject only malformed
  scheme/empty host) and preserve the base path prefix (e.g. /clientlib)
  instead of dropping it (#1, #2)
- migrate_users: mirror the http+https check; print the actually-built
  request URL rather than a naively-concatenated one (#1, #2)
- growth-report build(): surface a bad-URL ValueError with its own
  message instead of the generic "needs admin key" text (#3)
- _iter_buckets: replace the per-bucket rescan with a created_at-sorted
  sweep pointer; behavior-preserving (#4)
- _fetch_service_accounts: reuse the validated admin_api_url builder (#5a)
- _short_api_error: drop the over-broad "csp" redaction marker (#5b)
- churn_series: use `is not None` consistently for created/deleted (#5c)
- leaderboard bar values: normalize via _lb_value/_num so fractional
  em_score renders as a clean int (#5d)

Adds coverage for the URL layer (http, path prefix, malformed base/host),
the sweep's mid-series deletion drop-out, and _num/_lb_value rounding.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: LeoRoccoBreedt

cometx/cli/admin_growth_report.py

@@ -26,7 +26,6 @@
 import datetime
 import os
 import re
-from urllib.parse import urlparse
 
 from cometx.cli.admin_growth_render import build_html, write_html
 from cometx.cli.admin_growth_users import (
@@ -49,7 +48,7 @@
     workspace_churn_series,
     workspace_org_totals,
 )
-from cometx.utils import fetch_chargeback_report, format_time_key
+from cometx.utils import admin_api_url, fetch_chargeback_report, format_time_key
 
 # `build_html` is re-exported here so the growth-report module is the single
 # import surface (tests + callers import it from here). Declaring it in
@@ -91,6 +90,19 @@ def _num(value):
     return value
 
 
+def _lb_value(value):
+    """Normalize a leaderboard bar value the same way workspace rows are
+    rendered: round a fractional metric (e.g. `em_score`, which folds in
+    `data_logged_mb`) to a clean integer via `_num(round(...))`. Passes
+    `None` through unchanged so a metric a user lacks stays absent rather
+    than becoming 0."""
+    if value is None:
+        return None
+    if isinstance(value, float):
+        return _num(round(value))
+    return value
+
+
 def _window_growth(created_ms, window):
     """Growth over the analysis window from a collection of creation timestamps
     (epoch ms): `new_in` = items created within `[window.start, window.end]`,
@@ -173,11 +185,12 @@ def _fetch_service_accounts(api) -> "set[str] | None":
     labeled regex heuristic instead of crashing or silently reporting a
     zero split under the admin_api label."""
     try:
-        parsed = urlparse(api.config["comet.url_override"])
-        base = "%s://%s" % (parsed.scheme, parsed.netloc)
-        while base.endswith("/"):
-            base = base[:-1]
-        url = base + "/api/admin/service-accounts"
+        # Reuse the shared validated URL builder so this endpoint honors the
+        # same scheme/host/path-prefix handling as fetch_chargeback_report
+        # (the two admin endpoints were previously built inconsistently).
+        url = admin_api_url(
+            api.config["comet.url_override"], "/api/admin/service-accounts"
+        )
         response = api._client.get(
             url, headers={"Authorization": api.api_key}, params={}
         )
@@ -207,13 +220,17 @@ def _short_api_error(exc):
     # onward -- verbose SDK/HTTP errors dump headers, cookies, body, and CSP.
     lowered = text.lower()
     cut = len(text)
+    # NB: markers are matched as substrings anywhere in the text, so each must
+    # be specific enough not to fire on an incidental hostname/message word.
+    # "content-security-policy" already covers the CSP header; a bare "csp"
+    # (3 chars) would truncate errors that merely happen to contain those
+    # letters, so it is intentionally not listed.
     for marker in (
         "headers:",
         "header:",
         "cookie",
         "body:",
         "content-security-policy",
-        "csp",
         "set-cookie",
     ):
         idx = lowered.find(marker)
@@ -344,6 +361,13 @@ def build(self, workspaces):
         print("Fetching chargeback report (admin API)...")
         try:
             chargeback = fetch_chargeback_report(self.api)
+        except ValueError as exc:
+            # A ValueError here is a configuration/URL problem (e.g. a
+            # malformed --host or url_override), not an auth failure. Surface
+            # it as-is rather than asserting the API key isn't admin.
+            raise GrowthReportError(
+                f"growth-report could not reach the chargeback endpoint: {exc}"
+            ) from exc
         except Exception as exc:
             raise GrowthReportError(
                 "growth-report requires an admin API key: the chargeback "
@@ -976,7 +1000,10 @@ def emit_ws(slug, label, totals, hint):
                         self._leaderboard_chart(
                             f"chart-lb-user-{key}-top",
                             f"Top {n} users by {label}",
-                            [{"label": u.username, "value": value_fn(u)} for u in top],
+                            [
+                                {"label": u.username, "value": _lb_value(value_fn(u))}
+                                for u in top
+                            ],
                             hint,
                         )
                     )
@@ -987,7 +1014,7 @@ def emit_ws(slug, label, totals, hint):
                             f"chart-lb-user-{key}-bottom",
                             f"Bottom {n} users by {label}",
                             [
-                                {"label": u.username, "value": value_fn(u)}
+                                {"label": u.username, "value": _lb_value(value_fn(u))}
                                 for u in bottom
                             ],
                             hint,

cometx/cli/admin_growth_users.py

@@ -347,24 +347,36 @@ def _iter_buckets(users: "list[UserRecord]", units: str, now_ms: int):
     yet deleted) as of `bucket_end_ms`. `bucket_end_ms` is capped at
     `now_ms` so the current/last bucket reflects "as of now" rather than
     the theoretical end of an in-progress period."""
-    created_times = [u.created_at for u in users if u.created_at is not None]
-    if not created_times:
+    # Sort the eligible (non-suspended, dated) users by `created_at` once so
+    # each bucket can *admit* newly-created users via a sweep pointer instead
+    # of rescanning the whole list. This turns the created-side filter from
+    # O(buckets x users) into a single pass; the only per-bucket work left is
+    # dropping users whose deletion has passed (cheap unless there are many
+    # deletions). Buckets are yielded in increasing time order, and
+    # `created_at` only ever admits (never removes) as time advances, so the
+    # sweep is safe.
+    dated = sorted(
+        (u for u in users if not u.suspended and u.created_at is not None),
+        key=lambda u: u.created_at,
+    )
+    if not dated:
         return
-    earliest_ms = min(created_times)
+    earliest_ms = dated[0].created_at
     if earliest_ms > now_ms:
         earliest_ms = now_ms
 
+    admitted: "list[UserRecord]" = []
+    ptr = 0
+    n = len(dated)
     for key in _bucket_keys(earliest_ms, now_ms, units):
         next_key = get_next_time_key(key, units)
         next_start_ms = _dt_to_ms(parse_time_key(next_key, units))
         bucket_end_ms = min(next_start_ms - 1, now_ms)
+        while ptr < n and dated[ptr].created_at <= bucket_end_ms:
+            admitted.append(dated[ptr])
+            ptr += 1
         existing = [
-            u
-            for u in users
-            if not u.suspended
-            and u.created_at is not None
-            and u.created_at <= bucket_end_ms
-            and (u.deleted_at is None or u.deleted_at >= bucket_end_ms)
+            u for u in admitted if u.deleted_at is None or u.deleted_at >= bucket_end_ms
         ]
         yield key, bucket_end_ms, existing
 
@@ -520,7 +532,7 @@ def churn_series(
     the chargeback snapshot are visible, so hard-deleted accounts are not
     counted -- `deleted` reflects soft-deletes only. Returns `None` when no
     user has a known `created_at`."""
-    created = [u.created_at for u in users if u.created_at]
+    created = [u.created_at for u in users if u.created_at is not None]
     if not created:
         return None
     earliest_ms = min(created)
@@ -530,10 +542,10 @@ def churn_series(
     added_counts: dict = {}
     deleted_counts: dict = {}
     for u in users:
-        if u.created_at:
+        if u.created_at is not None:
             k = format_time_key(_ms_to_dt(u.created_at), units)
             added_counts[k] = added_counts.get(k, 0) + 1
-        if u.deleted_at:
+        if u.deleted_at is not None:
             k = format_time_key(_ms_to_dt(u.deleted_at), units)
             deleted_counts[k] = deleted_counts.get(k, 0) + 1
 

cometx/cli/migrate_users.py

@@ -137,14 +137,16 @@ class _RequestsClient:
     """
 
     def get(self, url, headers=None, params=None):
-        # Defensive scheme check at the request boundary: callers derive
-        # `url` from operator-supplied --url/--source-url, and we only ever
-        # talk to a Comet server over https. Reject anything else rather than
-        # handing an arbitrary value to requests.get.
+        # Defensive sanity check at the request boundary: callers derive
+        # `url` from operator-supplied --url/--source-url. Reject only
+        # clearly-malformed values (no host, or a non-http(s) scheme) rather
+        # than handing an arbitrary value to requests.get. On-prem Comet
+        # servers are reachable over plain http (see MIGRATIONS.md), so http
+        # is allowed; this is not an SSRF denylist.
         parsed = urllib.parse.urlparse(url)
-        if parsed.scheme != "https" or not parsed.netloc:
+        if parsed.scheme not in ("http", "https") or not parsed.netloc:
             raise ValueError(
-                "Request URL must be an https:// URL with a host; got %r." % url
+                "Request URL must be an http(s):// URL with a host; got %r." % url
             )
         resp = requests.get(url, headers=headers, params=params, timeout=30)
         resp.raise_for_status()
@@ -161,9 +163,12 @@ def __init__(self, url, key):
 
 
 def _fetch_chargeback_report(server_url, source_api_key):
-    from cometx.utils import fetch_chargeback_report
+    from cometx.utils import admin_api_url, fetch_chargeback_report
 
-    url = f"{server_url}/api/admin/chargeback/report"
+    # Build the printed URL the same way fetch_chargeback_report builds the
+    # one it requests (preserving any path prefix), so the debug line can't
+    # drift from the URL actually hit.
+    url = admin_api_url(server_url, "/api/admin/chargeback/report")
     print(f"Fetching chargeback report from {url}...")
     api = _ApiShim(server_url, source_api_key)
     return fetch_chargeback_report(api, host=server_url)

cometx/utils.py

@@ -387,29 +387,40 @@ def remove_extra_slashes(path):
         return ""
 
 
+def admin_api_url(base, path):
+    """Join an operator-supplied server base with an admin API `path`.
+
+    Validates that `base` is a well-formed http(s) URL with a host, then
+    preserves its scheme, host, AND any path prefix (e.g. `/clientlib`) that
+    on-prem deployments sit behind -- only clearly-malformed values (no
+    scheme, non-http(s) scheme, or empty host) are rejected. This is a
+    boundary sanity check, not an SSRF control: it intentionally does not
+    denylist private/loopback hosts, since operators legitimately point these
+    admin commands at internal addresses.
+    """
+    parsed = urlparse(base)
+    if parsed.scheme not in ("http", "https") or not parsed.netloc:
+        raise ValueError(
+            "Comet server URL must be an http(s):// URL with a host; got %r." % base
+        )
+    prefix = parsed.path.rstrip("/")
+    return "%s://%s%s%s" % (parsed.scheme, parsed.netloc, prefix, path)
+
+
 def fetch_chargeback_report(api, host=None, report_month=None):
     """Fetch the admin chargeback report JSON.
 
     Single source for the `/api/admin/chargeback/report` call used by the
     chargeback-report action, migrate-users, and growth-report. `host`
     overrides the base URL derived from `api.config["comet.url_override"]`;
-    `report_month` (YYYY-MM) adds `?reportMonth=`.
+    `report_month` (YYYY-MM) adds `?reportMonth=`. The base's path prefix (if
+    any) is preserved -- see `admin_api_url`.
     """
     if host is not None:
         base = host
     else:
         base = api.config["comet.url_override"]
-    # Require a well-formed https base before issuing the request. The base
-    # comes from operator-supplied --host/--source-url or the configured
-    # override; enforce a scheme so a malformed value can't be sent verbatim.
-    parsed = urlparse(base)
-    if parsed.scheme != "https" or not parsed.netloc:
-        raise ValueError(
-            "Chargeback server URL must be an https:// URL with a host; "
-            "got %r." % base
-        )
-    base = "%s://%s" % (parsed.scheme, parsed.netloc)
-    url = base + "/api/admin/chargeback/report"
+    url = admin_api_url(base, "/api/admin/chargeback/report")
     # Pass reportMonth as a query param so it's URL-encoded rather than
     # interpolated raw into the URL.
     params = {"reportMonth": report_month} if report_month else {}

tests/unit/test_admin_growth_report.py

@@ -1280,3 +1280,25 @@ def test_short_api_error_condenses_verbose_sdk_error():
     assert _short_api_error("plain boom") == "plain boom"
     long = "x" * 300
     assert len(_short_api_error(long)) <= 160
+
+
+def test_num_renders_integral_floats_as_int():
+    from cometx.cli.admin_growth_report import _num
+
+    assert _num(3.0) == 3
+    assert isinstance(_num(3.0), int)
+    assert _num(3.5) == 3.5
+    assert _num(7) == 7
+
+
+def test_lb_value_rounds_fractional_metric_and_passes_none():
+    # #5d: leaderboard bar values must render like workspace rows -- an
+    # em_score of 12.7 (data_logged_mb folded in) shows as 13, not 12.7,
+    # and a metric a user lacks (None) stays absent rather than becoming 0.
+    from cometx.cli.admin_growth_report import _lb_value
+
+    assert _lb_value(12.7) == 13
+    assert isinstance(_lb_value(12.7), int)
+    assert _lb_value(4.0) == 4
+    assert _lb_value(None) is None
+    assert _lb_value(9) == 9

tests/unit/test_admin_growth_users.py

@@ -117,6 +117,48 @@ def test_active_series_buckets_total_and_active():
     assert pts[-1]["values"]["total"] == 2
 
 
+def test_active_series_sweep_drops_user_after_deletion():
+    # Guards the _iter_buckets sweep refactor: a user created early and
+    # deleted mid-span must be counted in buckets before their deletion and
+    # excluded from buckets after it (the anti-monotone deletion case the
+    # created-sorted sweep pointer must still honor).
+    from cometx.cli.admin_growth_users import active_series, parse_users
+
+    month = 31 * 86400 * 1000
+    start = NOW - 4 * month
+    cb = {
+        "workspaces": [],
+        "users": {
+            "licensedUsers": [
+                {
+                    "username": "steady",
+                    "email": "s@x",
+                    "createdAt": start,
+                    "lastUsedAt": NOW,
+                    "deletedAt": None,
+                    "suspended": False,
+                },
+                {
+                    "username": "leaver",
+                    "email": "l@x",
+                    "createdAt": start,
+                    "lastUsedAt": NOW - 3 * month,
+                    # deleted ~2 months before now
+                    "deletedAt": NOW - 2 * month,
+                    "suspended": False,
+                },
+            ]
+        },
+    }
+    pts = active_series(
+        parse_users(cb), units="month", now_ms=NOW, active_window_days=30
+    )
+    totals = [p["values"]["total"] for p in pts]
+    # both present in the first bucket; only 'steady' survives to the last
+    assert totals[0] == 2
+    assert totals[-1] == 1
+
+
 def test_capability_series_none_when_no_capability_fields():
     from cometx.cli.admin_growth_users import capability_series, parse_users