Refactor PR/BEP README tables: split combined cells into sortable columns + fix counting of authors

AGENTS.md

@@ -110,16 +110,35 @@ The CLI is registered via `[project.scripts]` in `pyproject.toml`.
   and leaves the new stats columns as `—`.
 - `pr_number`, `git_ref`, `last_commit`, `last_updated` (build time, UTC)
 - `has_schema_changes`, `build_status` (`success` | `failed`)
-- `authors_count` (unique commit authors from `merge_base..PR_HEAD`)
+- `authors_count` — **seed value only.** Unique contributor *emails*
+  (mailmap-canonical, authors ∪ committers, minus GitHub's web-flow
+  identity) over `merge_base..PR_HEAD`, computed by
+  `count_contributors` in `tools/inject-schema-pr` at **schema-build**
+  time. It goes stale as soon as commits land without a schema rebuild,
+  so renderers prefer `stats.contributors.count` and fall back here only
+  when stats are absent. Do not read it directly.
 - On failure: `error_message`, `error_log`
 - **`stats`** (v2 nested block, populated by `bids-schema collect prs`):
   `_source_head_sha`, `_collected_at`, `_complete`, `_error`;
   `pr_state`, `pr_created_at`, `pr_updated_at`, `review_decision`;
   `commits.{count, first_at, last_at}`;
+  `contributors.{count, authors, committers, by_identity{...}}`;
   `reviews.{approved, changes_requested, commented, dismissed, pending, total, by_author{...}}`;
   `comments.{issue_count, review_thread_count, total, first_at, last_at, by_author{...}}`;
   `review_threads.{total, unresolved, unresolved_active, unresolved_outdated, unresolved_by_author{...}}`.
 
+`contributors` is the authoritative contributor count, refreshed every
+collection cycle. It counts distinct *people* across both the author and
+the committer of every commit, keyed on GitHub login where GitHub
+resolved the commit email to an account and on the lowercased email
+otherwise; a second pass folds bare-email identities into a login when
+another commit tied that address to an account. GitHub's own web-flow
+identity (`noreply@github.com`) and `[bot]` logins are excluded, while
+per-user `…@users.noreply.github.com` addresses are kept. Keying on the
+*name* — what `git shortlog` groups by — miscounts anyone who has
+committed under two spellings, and ignoring the committer misses anyone
+who merely landed a patch.
+
 Per-author records under `reviews.by_author` also carry `last_state`
 (the reviewer's most recent submission state) and `effective_state`
 (the state of their most recent non-COMMENTED, non-DISMISSED

bids_schema/collect/github.py

@@ -74,7 +74,11 @@ def _build_preflight_query(pr_numbers: list[int]) -> str:
       state createdAt updatedAt reviewDecision headRefOid
       commits(first:100, after:$cCur) {
         totalCount pageInfo{hasNextPage endCursor}
-        nodes{commit{committedDate authoredDate author{user{login}}}}
+        nodes{commit{
+          committedDate authoredDate
+          author{name email user{login}}
+          committer{name email user{login}}
+        }}
       }
       reviews(first:100, after:$rCur) {
         totalCount pageInfo{hasNextPage endCursor}
@@ -379,13 +383,32 @@ def _paginate_pr(pr_number: int) -> dict:
     }
 
 
+def _extract_actor(actor: dict | None) -> dict:
+    """Flatten a GraphQL ``GitActor`` into ``{name, email, login}``.
+
+    ``user`` is null when GitHub cannot match the commit's email to an
+    account (unregistered address, or a co-author trailer) — the raw
+    name/email are what identity resolution falls back to.
+    """
+    actor = actor or {}
+    return {
+        "name":  actor.get("name"),
+        "email": actor.get("email"),
+        "login": ((actor.get("user") or {}).get("login")),
+    }
+
+
 def _extract_commit(node: dict) -> dict:
     commit = node.get("commit") or {}
-    author_user = ((commit.get("author") or {}).get("user") or {})
+    author = _extract_actor(commit.get("author"))
+    committer = _extract_actor(commit.get("committer"))
     return {
         "authoredDate": commit.get("authoredDate"),
         "committedDate": commit.get("committedDate"),
-        "login": author_user.get("login"),
+        # Kept for backwards compatibility: the *authoring* login.
+        "login": author["login"],
+        "author": author,
+        "committer": committer,
     }
 
 
@@ -472,6 +495,89 @@ def _effective_state(states_by_time: list[tuple[str, str]]) -> str | None:
     return None
 
 
+#: GitHub's own machine identity on web-UI commits (merges, "Update branch",
+#: squash-from-UI). Note this is NOT the per-user noreply form, which lives on
+#: ``users.noreply.github.com`` and belongs to a real person.
+WEB_FLOW_EMAIL = "noreply@github.com"
+
+
+def _is_machine_identity(actor: dict) -> bool:
+    """True for GitHub's own commit identity and for App/bot accounts."""
+    login = (actor.get("login") or "").strip().lower()
+    if login.endswith("[bot]") or login == "web-flow":
+        return True
+    return (actor.get("email") or "").strip().lower() == WEB_FLOW_EMAIL
+
+
+def resolve_contributors(commits: list[dict]) -> dict:
+    """Count the distinct *people* behind a PR's commits.
+
+    Both roles count: someone who lands another person's patch (rebase,
+    squash-merge, applying a suggestion) contributed to the branch even
+    though ``git shortlog`` — which only reads the author field — never
+    shows them.
+
+    Identity is keyed on the GitHub login when GitHub resolved the commit
+    email to an account, and on the lowercased email otherwise. Keying on
+    the *name* (what ``git shortlog`` groups by) miscounts anyone who has
+    committed under two spellings: bids-specification PR #2307 has both
+    "Chris Markiewicz" and "Christopher J. Markiewicz" on one address, so
+    a name-keyed count reports six contributors where there are five.
+
+    A second pass folds bare-email identities into a login whenever some
+    other commit tied that same address to an account, so one person split
+    across resolved and unresolved commits still counts once.
+    """
+    actors: list[tuple[dict, str]] = []
+    for commit in commits:
+        actors.append((commit.get("author") or {}, "authored"))
+        actors.append((commit.get("committer") or {}, "committed"))
+
+    # Pass 1 — learn every email GitHub itself tied to an account.
+    email_to_login: dict[str, str] = {}
+    for actor, _role in actors:
+        if _is_machine_identity(actor):
+            continue
+        login = (actor.get("login") or "").strip()
+        email = (actor.get("email") or "").strip().lower()
+        if login and email:
+            email_to_login.setdefault(email, login.lower())
+
+    # Pass 2 — assign each actor a stable key and tally the two roles.
+    by_identity: dict[str, dict] = {}
+    for actor, role in actors:
+        if _is_machine_identity(actor):
+            continue
+        login = (actor.get("login") or "").strip()
+        email = (actor.get("email") or "").strip().lower()
+        name = (actor.get("name") or "").strip()
+        if login:
+            key = login.lower()
+        elif email:
+            key = email_to_login.get(email, email)
+        elif name:
+            key = name.lower()
+        else:
+            continue
+
+        record = by_identity.setdefault(key, {
+            "name": name or None, "login": login or None, "email": email or None,
+            "authored": 0, "committed": 0,
+        })
+        record[role] += 1
+        # Keep the most informative label we have seen for this person.
+        record["name"] = record["name"] or (name or None)
+        record["login"] = record["login"] or (login or None)
+        record["email"] = record["email"] or (email or None)
+
+    return {
+        "count":      len(by_identity),
+        "authors":    sum(1 for r in by_identity.values() if r["authored"]),
+        "committers": sum(1 for r in by_identity.values() if r["committed"]),
+        "by_identity": by_identity,
+    }
+
+
 def derive_stats(fetched: dict, source_head_sha: str | None) -> dict:
     """Turn the paginated raw response into the final ``stats`` block dict."""
     commits = fetched["commits"]
@@ -575,6 +681,8 @@ def derive_stats(fetched: dict, source_head_sha: str | None) -> dict:
             "last_at":  commit_last,
         },
 
+        "contributors": resolve_contributors(commits),
+
         "reviews": {
             "approved":          state_counts.get("APPROVED", 0),
             "changes_requested": state_counts.get("CHANGES_REQUESTED", 0),

bids_schema/render/bep_readme.py

@@ -14,10 +14,12 @@
 from bids_schema.render import formatters as fmt
 
 TABLE_HEADER = (
-    "| BEP # | Title | Google Doc | PR # | Authors | Build | "
-    "Reviews | Comments | Unresolved | BEP registered | Doc registered | Actions |\n"
-    "| ----- | ----- | ---------- | ---- | ------- | ----- | "
-    "------- | -------- | ---------- | -------------- | -------------- | ------- |"
+    "| BEP # | Title | Google Doc | PR # | # Authors | # Commenters | Build | "
+    "Reviews | Unresolved | Comments | First comment | Last comment | "
+    "BEP registered | Doc registered | Actions |\n"
+    "| ----- | ----- | ---------- | ---- | --------- | ------------ | ----- | "
+    "------- | ---------- | -------- | ------------- | ------------ | "
+    "-------------- | -------------- | ------- |"
 )
 
 FOOTER_DEPRECATION_NOTE = (
@@ -47,7 +49,7 @@ def _format_bep_row(bep_number: str, metadata: dict, base_dir: Path | None) -> s
     doc_link = f"[Doc]({google_doc})" if google_doc else "—"
     pr_link = f"[{pr_number}]({fmt.pr_url(pr_number)})" if pr_number else "—"
 
-    authors_count = str(pr_metadata.get("authors_count", 0)) if pr_metadata else "—"
+    authors_cell = fmt.format_contributors(pr_metadata) if pr_metadata else "—"
     build_cell = fmt.format_build_cell(pr_metadata) if pr_metadata else fmt.format_build_indicator("unknown")
 
     bep_registered_cell = fmt.format_date(metadata.get("bep_registered"))
@@ -61,8 +63,10 @@ def _format_bep_row(bep_number: str, metadata: dict, base_dir: Path | None) -> s
     )
 
     return (
-        f"| {bep_display} | {title} | {doc_link} | {pr_link} | {authors_count} | "
-        f"{build_cell} | {cells['reviews']} | {cells['comments']} | {cells['unresolved']} | "
+        f"| {bep_display} | {title} | {doc_link} | {pr_link} | {authors_cell} | "
+        f"{cells['commenters']} | {build_cell} | "
+        f"{cells['reviews']} | {cells['unresolved']} | "
+        f"{cells['comments_count']} | {cells['comments_first']} | {cells['comments_last']} | "
         f"{bep_registered_cell} | {doc_registered_cell} | {actions} |"
     )
 
@@ -110,10 +114,18 @@ def render(bep_records: list[tuple[str, dict]], base_dir: Path | None = None) ->
 
     body_lines.extend([
         "",
-        "Column legend: **Reviews** = `approved✅ / changes_requested❌ / commented💬`; "
+        "Column legend: **# Authors** = distinct people behind the linked PR\u2019s commits, "
+        "counting both the author and the committer of each commit and keyed on GitHub "
+        "account rather than on name; **# Commenters** = distinct accounts that commented "
+        "on that PR. **Reviews** = submitted reviews as "
+        "`approved✅ / changes_requested❌ / commented💬`, zero-valued components omitted "
+        "(so `1✅/27💬`, not `1✅/0❌/27💬`); "
         "**Unresolved** = count of unresolved inline review threads (bolded if > 0); "
+        "**Comments** / **First comment** / **Last comment** = issue comments plus review threads "
+        "on the linked PR, count and dates split so each can be sorted independently; "
         "**BEP registered** = date the BEP entry was first added to `bids-website:data/beps/beps.yml`; "
-        "**Doc registered** = date a `google_doc` URL was first attached to that entry.",
+        "**Doc registered** = date a `google_doc` URL was first attached to that entry. "
+        "See [`PRs/README.md`](../PRs/) for the full per-PR commit statistics.",
         "",
         FOOTER_DEPRECATION_NOTE,
         "",

bids_schema/render/formatters.py

@@ -51,34 +51,39 @@ def format_date(iso: str | None) -> str:
     return dt.strftime("%Y-%m-%d")
 
 
-def format_date_window(first: str | None, last: str | None) -> str:
-    """Render two ISO timestamps as ``YYYY-MM-DD → YYYY-MM-DD``. ``—`` if both missing."""
-    if not first and not last:
-        return "—"
-    return f"{format_date(first)} → {format_date(last)}"
+#: ``stats.reviews`` keys rendered by :func:`format_reviews`, in display order.
+REVIEW_COMPONENTS = (
+    ("approved",          "✅"),
+    ("changes_requested", "❌"),
+    ("commented",         "💬"),
+)
 
 
 def format_reviews(reviews: dict | None) -> str:
     """Render the aggregate review histogram as ``3✅/2❌/7💬``.
 
-    Emoji legend intentionally minimal so the column stays narrow. ``—``
-    if the record has no stats block at all.
+    Zero-valued components are omitted so the cell stays scannable —
+    ``{approved: 1, changes_requested: 0, commented: 27}`` renders as
+    ``1✅/27💬`` rather than ``1✅/0❌/27💬``. ``—`` if the record has no
+    stats block at all, ``0`` if every component is zero.
     """
     if not reviews:
         return "—"
-    approved = reviews.get("approved", 0)
-    changes = reviews.get("changes_requested", 0)
-    commented = reviews.get("commented", 0)
-    if approved == changes == commented == 0:
+    parts = [
+        f"{reviews.get(key, 0)}{emoji}"
+        for key, emoji in REVIEW_COMPONENTS
+        if reviews.get(key, 0)
+    ]
+    if not parts:
         return "0"
-    return f"{approved}✅/{changes}❌/{commented}💬"
+    return "/".join(parts)
 
 
-def format_activity_span(count: int | None, first: str | None, last: str | None) -> str:
-    """Render ``<count> (<first> → <last>)``. ``—`` if count is falsy/absent."""
-    if not count:
+def format_count(count: int | None) -> str:
+    """Render a bare count for its own sortable column. ``None`` → ``—``, ``0`` → ``0``."""
+    if count is None:
         return "—"
-    return f"{count} ({format_date(first)} → {format_date(last)})"
+    return str(count)
 
 
 def format_unresolved(n: int | None) -> str:
@@ -138,34 +143,75 @@ def stale_marker(stats: dict) -> str:
 # --- Aggregated helpers used by both renderers ---------------------------
 
 
+#: Keys returned by :func:`format_stats_cells`. Counts and dates are kept in
+#: separate cells so each gets its own sortable column in the rendered table
+#: (a combined ``47 (2020-01-16 → 2026-05-08)`` cell sorts as a string, which
+#: is useless for every ordering one actually wants).
+STATS_CELL_KEYS = (
+    "reviews",
+    "unresolved",
+    "pr_created",
+    "commits_count", "commits_first", "commits_last",
+    "comments_count", "comments_first", "comments_last",
+    "commenters",
+)
+
+
 def format_stats_cells(stats: dict) -> dict[str, str]:
-    """Return the four stats-derived table cells (reviews / comments /
-    unresolved / commit window) from a PR ``stats`` sub-block.
+    """Return the stats-derived table cells from a PR ``stats`` sub-block.
 
-    Every value defaults to ``—`` if the block is missing or absent —
-    so v1 records or PRs whose stats haven't been collected yet render
-    consistently in both READMEs.
+    Keys are :data:`STATS_CELL_KEYS`. Every value defaults to ``—`` if the
+    block is missing or absent — so v1 records or PRs whose stats haven't
+    been collected yet render consistently in both READMEs.
+
+    ``pr_created`` is the PR's own creation date, which is **not** the same
+    as ``commits_first``: a force-push (rebase, squash, branch recreation)
+    replaces the branch's commits, so on an old PR the earliest surviving
+    commit can post-date the PR — and its comments — by years. Showing both
+    makes that discrepancy legible instead of looking like a bug.
     """
     if not stats:
-        return {"reviews": "—", "comments": "—", "unresolved": "—", "commit_window": "—"}
+        return dict.fromkeys(STATS_CELL_KEYS, "—")
     comments = stats.get("comments") or {}
     threads = stats.get("review_threads") or {}
     commits = stats.get("commits") or {}
     return {
-        "reviews":       format_reviews(stats.get("reviews")),
-        "comments":      format_activity_span(
-                             comments.get("total"),
-                             comments.get("first_at"),
-                             comments.get("last_at"),
-                         ),
-        "unresolved":    format_unresolved(threads.get("unresolved")),
-        "commit_window": format_date_window(
-                             commits.get("first_at"),
-                             commits.get("last_at"),
-                         ),
+        "reviews":        format_reviews(stats.get("reviews")),
+        "unresolved":     format_unresolved(threads.get("unresolved")),
+        "pr_created":     format_date(stats.get("pr_created_at")),
+        "commits_count":  format_count(commits.get("count")),
+        "commits_first":  format_date(commits.get("first_at")),
+        "commits_last":   format_date(commits.get("last_at")),
+        "comments_count": format_count(comments.get("total")),
+        "comments_first": format_date(comments.get("first_at")),
+        "comments_last":  format_date(comments.get("last_at")),
+        "commenters":     format_count(len(comments["by_author"])
+                                       if "by_author" in comments else None),
     }
 
 
+def format_contributors(record: dict) -> str:
+    """The ``# Authors`` cell: how many distinct people are behind the commits.
+
+    Prefers ``stats.contributors.count``, which the collector recomputes on
+    every cycle from both the author and committer of every commit, keyed on
+    GitHub login / email.
+
+    Falls back to the record's top-level ``authors_count``, which is a
+    ``git shortlog -sn | wc -l`` taken once when the schema was *built*. That
+    value goes stale the moment new commits land without a schema rebuild —
+    PR #2307 sat at ``2`` from May while the branch grew to five
+    contributors — and it counts only authors, grouped by name. So it is a
+    seed value, never preferred over collected stats.
+    """
+    contributors = (stats_of(record).get("contributors") or {})
+    count = contributors.get("count")
+    if count is not None:
+        return format_count(count)
+    fallback = record.get("authors_count")
+    return format_count(fallback) if fallback else "—"
+
+
 def format_build_cell(record: dict) -> str:
     """Build indicator + optional stale marker for a record's ``stats`` block."""
     return f"{format_build_indicator(build_status_of(record))}{stale_marker(stats_of(record))}"