diff --git a/src/whygraph/cli/commands/scan.py b/src/whygraph/cli/commands/scan.py index 3907d9e..e62aab7 100644 --- a/src/whygraph/cli/commands/scan.py +++ b/src/whygraph/cli/commands/scan.py @@ -21,6 +21,7 @@ from rich.text import Text from whygraph.scan import ( + AuthorResolver, CodeGraphCrawler, Crawler, GitCrawler, @@ -43,6 +44,7 @@ # Kept in one place so the whole set is trivially swappable (plan Β§10.5). _ICON_STRUCTURAL = "πŸ”Ž" _ICON_PR_ORIGINS = "πŸ”—" +_ICON_AUTHORS = "πŸ‘₯" _ICON_LLM = "🧠" _ICON_CODEGRAPH = "πŸ•Έ" @@ -164,7 +166,9 @@ def scan_cmd( # headers can be numbered against the count of phases that actually run. run_pr_origins = enrich_pr_origins and github_client is not None run_analyze = descriptor is not None - phase_total = 1 + int(run_pr_origins) + int(run_analyze) # Phase 1 always runs + # Author resolution always runs: local-only, no network, no token, so it + # is valid under --no-remote and inside the auto-rescan git hooks. + phase_total = 1 + int(run_pr_origins) + 1 + int(run_analyze) # Phase 1 always runs scan_log_path = db_path.parent / "scan.log" phase_timings: dict[str, float] = {} @@ -247,7 +251,26 @@ def scan_cmd( ok=enricher.error is None, ) - # ── Phase 3 Β· LLM descriptions β€” the slow, token-heavy long pole, + # ── Phase 3 Β· Author identity β€” needs Phase 1's commits AND Phase 2's + # PR-origin rows: an address can appear ONLY in on_default_branch=0 + # commits, so running before Phase 2 would miss that identity. ── + n += 1 + console.rule( + f"{_ICON_AUTHORS} Phase {n}/{phase_total} Β· Author identity", style="cyan" + ) + t0 = time.monotonic() + resolver = AuthorResolver(progress, repository=repository) + ran.append(resolver) + resolver.start() + resolver.join() + phase_timings["Author identity"] = time.monotonic() - t0 + _print_phase_done( + "Author identity", + phase_timings["Author identity"], + ok=resolver.error is None, + ) + + # ── Phase 4 Β· LLM descriptions β€” the slow, token-heavy long pole, # run strictly last and alone. Only ever describes main-walk # commits, so the recovered on_default_branch=0 rows stay lazy. ── if run_analyze: @@ -364,6 +387,7 @@ def _render_results_panel( git = by_name.get("git") github = by_name.get("github") enricher = by_name.get("pr-origins") + resolver = by_name.get("authors") analyzer = by_name.get("analyze") def _timing(title: str) -> str: @@ -392,6 +416,14 @@ def _timing(title: str) -> str: "PR-origin recovery", *_optional_phase_cells(enricher, _timing("PR-origin recovery")), ) + # Author resolution always runs, so this row never renders "β€” skipped" + # in practice; it still goes through _optional_phase_cells so the panel + # keeps its promise of never crashing on an unexpected `ran` list. + grid.add_row( + _ICON_AUTHORS, + "Author identity", + *_optional_phase_cells(resolver, _timing("Author identity")), + ) grid.add_row( _ICON_LLM, "LLM descriptions", diff --git a/src/whygraph/scan/__init__.py b/src/whygraph/scan/__init__.py index c5cecc9..42825ba 100644 --- a/src/whygraph/scan/__init__.py +++ b/src/whygraph/scan/__init__.py @@ -5,19 +5,24 @@ :class:`GitHubCrawler` for GitHub pull requests and issues, :class:`CodeGraphCrawler` which refreshes the CodeGraph index, :class:`AnalyzeCrawler` which describes each commit's diff with an LLM, -and :class:`PROriginEnricher` which recovers a squash-merged PR's original -commits. +:class:`PROriginEnricher` which recovers a squash-merged PR's original +commits, and :class:`AuthorResolver` which collapses raw identities into +one row per human. -The CLI drives them in three ordered phases against the shared SQLite -database: **Phase 1** runs :class:`GitCrawler` and :class:`GitHubCrawler` +The CLI drives them in ordered phases against the shared SQLite database: +**Phase 1** runs :class:`GitCrawler` and :class:`GitHubCrawler` concurrently; **Phase 2** runs :class:`PROriginEnricher` (which needs the -git + PR rows Phase 1 persisted); **Phase 3** runs :class:`AnalyzeCrawler` -last and alone (the slow, token-heavy LLM pass). :class:`CodeGraphCrawler` -is a best-effort background task started before Phase 1 and joined after -Phase 3 β€” it has no data dependency on the DB, so it spans the whole scan. +git + PR rows Phase 1 persisted); **Phase 3** runs :class:`AuthorResolver` +(which needs Phase 2's rows β€” an address can appear *only* in the +``on_default_branch=0`` commits that phase writes); **Phase 4** runs +:class:`AnalyzeCrawler` last and alone (the slow, token-heavy LLM pass). +:class:`CodeGraphCrawler` is a best-effort background task started before +Phase 1 and joined after the last phase β€” it has no data dependency on +the DB, so it spans the whole scan. """ from whygraph.scan.analyze_crawler import AnalyzeCrawler +from whygraph.scan.authors import AuthorResolver from whygraph.scan.codegraph_crawler import CodeGraphCrawler from whygraph.scan.crawler import Crawler from whygraph.scan.git_crawler import GitCrawler @@ -26,6 +31,7 @@ __all__ = [ "AnalyzeCrawler", + "AuthorResolver", "CodeGraphCrawler", "Crawler", "GitCrawler", diff --git a/src/whygraph/scan/authors.py b/src/whygraph/scan/authors.py new file mode 100644 index 0000000..db94d8f --- /dev/null +++ b/src/whygraph/scan/authors.py @@ -0,0 +1,637 @@ +"""AuthorResolver β€” collapse a repository's raw identities into people. + +One human routinely writes commits under several addresses (work laptop, +personal machine, GitHub's web UI) and under more than one display name, +so the obvious ``GROUP BY author_email`` reports the same person several +times while looking authoritative. This crawler rebuilds the ``author`` +table so that one human is one row, and is the producer +:mod:`whygraph.db.models.author` has always named. + +Resolution is **evidence-driven, never heuristic**, and the signals are +ordered by how much authority they carry: + +0. **git's mailmap** (:meth:`Repository.check_mailmap`) β€” a human + explicitly asserting "these contacts are me". It is the only signal + that is a statement of intent rather than an inference, so it runs + first and its *name* output also decides ``primary_name``. It is an + enhancement, not a prerequisite: no mailmap, or no usable git, simply + degrades to the signals below. +A. **GitHub's own triples** β€” ``pull_request.commit_titles`` entries + already carry ``author_login`` / ``author_name`` / ``author_email`` + together, so GitHub is asserting the link and nothing is inferred. +B. **The GitHub noreply parse** β€” ``12345+login@users.noreply.github.com`` + deterministically yields ``login``. Pure string work, so it still + resolves identities on repositories scanned with ``--no-remote``. +C. **Byte-equal emails** β€” implicit: the same address is the same node by + construction. + +What resolution deliberately refuses to do matters as much. It never +merges on display name ("Alex Kim" is not one person globally), never +fuzzy-matches addresses, and never treats a shared or bot address as +evidence β€” because **a false merge is worse than a false split**. +Collapsing two people silently attributes one person's work to another +with no symptom; leaving one person as two rows is merely untidy. + +Two ordering invariants: + +* The crawler runs **after** :class:`~whygraph.scan.pr_origin_enricher.PROriginEnricher`. + An address can appear *only* in the ``on_default_branch=0`` rows that + phase writes, and running earlier would make that identity invisible. +* ``commit_count`` / ``first_seen`` / ``last_seen`` count + ``on_default_branch=1`` commits **only**, matching every other velocity + query β€” a PR-origin commit is the same work as its squash commit, so + counting both would double-count. The flag records "on the default + branch *as of the last scan*", so these are DB truth, not git truth. + +Output is fully deterministic β€” clusters sorted by a stable key, every +JSON array sorted, every tie broken lexicographically β€” so a table +rebuilt on each scan produces no diff noise. +""" + +from __future__ import annotations + +import json +import logging +from collections import Counter, defaultdict +from dataclasses import dataclass, field + +from rich.progress import Progress +from sqlmodel import delete, select + +from whygraph.db import get_session +from whygraph.db.models.author import Author +from whygraph.db.models.commit import Commit as CommitRow +from whygraph.db.models.issue import Issue +from whygraph.db.models.pull_request import PullRequest +from whygraph.services.git import GitError, Repository + +from .crawler import Crawler + +_log = logging.getLogger(__name__) + +_SHARED_EMAILS = frozenset( + { + "noreply@github.com", # GitHub web-UI / merge commits β€” SHARED across users + "action@github.com", + "actions@github.com", + "githubactions@github.com", + } +) +"""Addresses that are not one person. A literal list, never derived. + +An address here is still recorded in a cluster's ``emails`` β€” so nothing +disappears β€” but is NEVER used as merge evidence. Unioning on a shared +address collapses an entire team into a single row, which is exactly the +undetectable failure this module is built to avoid. Deliberately +incomplete (GitLab, Gerrit and enterprise CI have their own): it is a +frozenset literal precisely so extending it is a one-line change. +""" + +_NOREPLY_SUFFIX = "@users.noreply.github.com" + +_Node = tuple[str, str] +"""A namespaced identity node β€” ``("email", …)`` or ``("login", …)``. + +Namespacing is load-bearing: a login and a display name are routinely the +same text, and a bare-string union-find would merge them by coincidence. +Names are never nodes at all (they are cluster attributes), because +same-name is the classic false-merge vector. +""" + + +def _json_list(raw: str | None) -> list: + """Decode a JSON-encoded list column; empty list on anything malformed. + + Mirrors ``pr_origin_enricher.py``'s helper β€” duplicated rather than + imported to keep the scan layer free of an upward dependency on the + MCP server layer. + """ + if not raw: + return [] + try: + parsed = json.loads(raw) + except (TypeError, json.JSONDecodeError): + return [] + return parsed if isinstance(parsed, list) else [] + + +def _login_from_noreply(email: str) -> str | None: + """Signal B β€” the GitHub login behind a ``users.noreply.github.com`` address. + + Handles both forms GitHub has issued: the modern + ``+@users.noreply.github.com`` and the older + ``@users.noreply.github.com``. + + Parameters + ---------- + email : str + Any email address; non-noreply addresses simply return ``None``. + + Returns + ------- + str or None + The parsed login, or ``None`` when the address is not a GitHub + noreply address or its local part yields no usable login. + """ + local = email.strip().lower() + if not local.endswith(_NOREPLY_SUFFIX): + return None + local = local[: -len(_NOREPLY_SUFFIX)] + if "+" in local: + local = local.partition("+")[2] + login = local.strip() + # Reject anything that cannot be a login rather than inventing one β€” + # a wrong login is a merge key, and a merge is the expensive mistake. + if not login or any(ch in login for ch in " \t@<>+"): + return None + return login + + +def _is_bot(email: str, login: str | None) -> bool: + """Whether a contact is a bot rather than a human. + + Recognizes the ``[bot]`` marker GitHub appends to app logins, in + either the login itself or the local part of its noreply address + (``41898282+github-actions[bot]@users.noreply.github.com``). + + Bots are not dropped β€” their commits are real and should stay + countable β€” they just never merge across the bot/human boundary, so a + bot forms its own cluster (:func:`_resolve`). + """ + if login and "[bot]" in login.lower(): + return True + return "[bot]" in email.partition("@")[0].lower() + + +def _format_contact(name: str, email: str) -> str: + """Render a ``Name `` contact for ``git check-mailmap``. + + A blank name is replaced with a placeholder: git needs the + ``Name `` shape, and a name the mailmap does not rewrite is + ignored by the caller anyway (only a *changed* name is treated as an + assertion). + """ + return f"{name.strip() or 'unknown'} <{email}>" + + +def _split_contact(line: str) -> tuple[str, str] | None: + """Parse one ``git check-mailmap`` output line into ``(name, email)``. + + Returns ``None`` for a line that is not in ``Name `` shape, so + unexpected output degrades to "no mapping" rather than to a wrong one. + """ + line = line.strip() + if not line.endswith(">") or "<" not in line: + return None + name, _, rest = line.rpartition("<") + return name.strip(), rest[:-1].strip() + + +def _mailmap_pairs( + repository: Repository, identities: list[tuple[str, str]] +) -> dict[tuple[str, str], tuple[str, str]]: + """Signal 0 β€” map raw ``(name, email)`` contacts to their canonical form. + + One batched :meth:`Repository.check_mailmap` call for every identity + in the repository. + + Parameters + ---------- + repository : Repository + The repository whose mailmap (``./.mailmap``, ``mailmap.file`` or + ``mailmap.blob``) is consulted. + identities : list[tuple[str, str]] + Distinct ``(name, email)`` pairs to canonicalize. + + Returns + ------- + dict[tuple[str, str], tuple[str, str]] + Raw pair β†’ canonical pair, for the pairs git actually returned. + Empty when git is unavailable or fails (logged at DEBUG β€” Signal 0 + is an enhancement, so resolution proceeds on signals A/B/C), and + empty when the output is ragged: the result is positional, so a + short list must not be zipped onto the inputs. + """ + if not identities: + return {} + contacts = [_format_contact(name, email) for name, email in identities] + try: + lines = repository.check_mailmap(contacts) + except GitError as exc: + _log.debug("mailmap unavailable; resolving without Signal 0: %s", exc) + return {} + if len(lines) != len(contacts): + _log.debug( + "check-mailmap returned %d lines for %d contacts; ignoring Signal 0", + len(lines), + len(contacts), + ) + return {} + pairs: dict[tuple[str, str], tuple[str, str]] = {} + for raw, line in zip(identities, lines): + parsed = _split_contact(line) + if parsed is not None: + pairs[raw] = parsed + return pairs + + +class _Union: + """Minimal union–find over :data:`_Node` values. + + Deliberately hand-rolled and tiny (no dependency): the interesting + part of this module is *what may merge*, not the merging itself. + """ + + def __init__(self) -> None: + self._parent: dict[_Node, _Node] = {} + + def add(self, node: _Node) -> None: + """Register ``node`` as its own set if not already known.""" + self._parent.setdefault(node, node) + + def find(self, node: _Node) -> _Node: + """Return the representative of ``node``'s set, path-compressing.""" + self.add(node) + root = node + while self._parent[root] != root: + root = self._parent[root] + while self._parent[node] != root: + self._parent[node], node = root, self._parent[node] + return root + + def union(self, a: _Node, b: _Node) -> None: + """Merge the sets containing ``a`` and ``b``. + + The smaller representative wins so the result does not depend on + insertion order (plan Β§3.6 determinism). + """ + ra, rb = self.find(a), self.find(b) + if ra == rb: + return + lo, hi = (ra, rb) if ra < rb else (rb, ra) + self._parent[hi] = lo + + def nodes(self) -> list[_Node]: + """Every known node, in sorted order.""" + return sorted(self._parent) + + +@dataclass +class _Cluster: + """One resolved identity: its raw values plus its aggregates. + + Attributes + ---------- + emails, logins, names : set[str] + Every raw value the cluster absorbed. ``emails`` may include a + shared address that was recorded but never merged on. + primary_login, primary_name, primary_email : str or None + The display values, chosen deterministically (see + :func:`_resolve`). + first_seen, last_seen : str or None + ISO-8601 bounds over the cluster's default-branch commits. + commit_count, pr_count, issue_count : int + Aggregates; ``commit_count`` counts default-branch commits only. + """ + + emails: set[str] = field(default_factory=set) + logins: set[str] = field(default_factory=set) + names: set[str] = field(default_factory=set) + primary_login: str | None = None + primary_name: str | None = None + primary_email: str | None = None + first_seen: str | None = None + last_seen: str | None = None + commit_count: int = 0 + pr_count: int = 0 + issue_count: int = 0 + + +def _resolve(session, repository: Repository) -> list[_Cluster]: + """Resolve every raw identity in the DB into a sorted list of clusters. + + Reads ``commit``, ``pull_request`` and ``issue``, applies signals + 0/A/B/C (module docstring), then computes each cluster's display + values and aggregates. Purely read-only β€” the caller owns the rewrite. + + Parameters + ---------- + session : sqlmodel.Session + Open session to read through. + repository : Repository + Used only for Signal 0. A repository whose ``check_mailmap`` + raises is tolerated (:func:`_mailmap_pairs`). + + Returns + ------- + list[_Cluster] + Fully populated clusters, ordered by + ``(primary_login, primary_email, min(emails))`` so a rebuild is + byte-identical given identical input. + """ + commit_rows = session.exec( + select( + CommitRow.author_name, + CommitRow.author_email, + CommitRow.authored_at, + CommitRow.on_default_branch, + ) + ).all() + pr_rows = session.exec(select(PullRequest.author, PullRequest.commit_titles)).all() + issue_authors = session.exec(select(Issue.author)).all() + + union = _Union() + names: dict[_Node, set[str]] = defaultdict(set) + # Shared addresses recorded onto the identity that used them without + # ever merging on them (Β§3.4). + extra_emails: dict[_Node, set[str]] = defaultdict(set) + commits_by_email: dict[str, list[tuple[str, str, int]]] = defaultdict(list) + pr_counts: Counter[str] = Counter() + issue_counts: Counter[str] = Counter() + + def _add_email(value: str | None) -> str | None: + email = (value or "").strip() + if not email: + return None + union.add(("email", email)) + return email + + def _add_login(value: str | None) -> str | None: + login = (value or "").strip() + if not login: + return None + union.add(("login", login)) + return login + + # --- raw material ----------------------------------------------------- + identities: set[tuple[str, str]] = set() + for author_name, author_email, authored_at, on_default_branch in commit_rows: + email = _add_email(author_email) + if email is None: + continue + name = (author_name or "").strip() + identities.add((name, email)) + if name: + names[("email", email)].add(name) + commits_by_email[email].append( + (name, authored_at or "", int(on_default_branch or 0)) + ) + + triples: list[tuple[str, str, str]] = [] + for pr_author, commit_titles in pr_rows: + login = _add_login(pr_author) + if login is not None: + # A reviewer who opened PRs but never committed still gets a + # row β€” hence a standalone login node. + pr_counts[login] += 1 + for entry in _json_list(commit_titles): + if not isinstance(entry, dict): + continue # a malformed payload skips its entry, not the PR + triple = ( + (entry.get("author_login") or "").strip(), + (entry.get("author_name") or "").strip(), + (entry.get("author_email") or "").strip(), + ) + triples.append(triple) + _add_login(triple[0]) + email = _add_email(triple[2]) + if email is not None and triple[1]: + identities.add((triple[1], email)) + + for issue_author in issue_authors: + login = _add_login(issue_author) + if login is not None: + issue_counts[login] += 1 + + # --- Signal 0 Β· the mailmap ------------------------------------------ + mailmap = _mailmap_pairs(repository, sorted(identities)) + # email β†’ the proper name the mailmap asserted for it (Β§3.6). + asserted_names: dict[str, str] = {} + for (raw_name, raw_email), (canon_name, canon_email) in sorted(mailmap.items()): + if canon_email and canon_email != raw_email: + union.add(("email", canon_email)) + mergeable = ( + raw_email not in _SHARED_EMAILS + and canon_email not in _SHARED_EMAILS + and _is_bot(raw_email, None) == _is_bot(canon_email, None) + ) + if mergeable: + union.union(("email", raw_email), ("email", canon_email)) + if canon_name and canon_name != raw_name: + asserted_names.setdefault(canon_email or raw_email, canon_name) + asserted_names.setdefault(raw_email, canon_name) + + # --- Signal A Β· GitHub's login ↔ email ↔ name triples ----------------- + for login, name, email in triples: + if email and email not in _SHARED_EMAILS and name: + names[("email", email)].add(name) + elif login and name: + names[("login", login)].add(name) + if not (login and email): + continue + if email in _SHARED_EMAILS: + # Recorded so it does not disappear, but never merge evidence. + extra_emails[("login", login)].add(email) + continue + union.union(("login", login), ("email", email)) + + # --- Signal B Β· the noreply parse ------------------------------------ + for kind, value in union.nodes(): + if kind != "email" or value in _SHARED_EMAILS: + continue + login = _login_from_noreply(value) + if login is None: + continue + union.add(("login", login)) + union.union(("email", value), ("login", login)) + + # --- Signal C is implicit: byte-equal emails are the same node. ------- + + groups: dict[_Node, list[_Node]] = defaultdict(list) + for node in union.nodes(): + groups[union.find(node)].append(node) + + clusters: list[_Cluster] = [] + for members in groups.values(): + cluster = _Cluster() + for node in members: + kind, value = node + if kind == "email": + cluster.emails.add(value) + else: + cluster.logins.add(value) + cluster.names |= names.get(node, set()) + cluster.emails |= extra_emails.get(node, set()) + for email in cluster.emails: + # Names behind a shared address belong to whoever used it, so + # they are NOT attributed here (that would be a false merge in + # all but name). + if email not in _SHARED_EMAILS: + cluster.names |= names.get(("email", email), set()) + clusters.append(cluster) + + # A shared address is an attribute of the identity that used it, so + # the standalone node it also forms is dropped once some cluster has + # claimed it. An *unclaimed* shared address keeps its own row rather + # than disappearing (Β§3.4). + claimed: set[str] = set().union(*extra_emails.values()) if extra_emails else set() + clusters = [ + c + for c in clusters + if c.logins or not (c.emails <= _SHARED_EMAILS and c.emails <= claimed) + ] + + for cluster in clusters: + _populate(cluster, commits_by_email, pr_counts, issue_counts, asserted_names) + + clusters.sort( + key=lambda c: ( + c.primary_login or "", + c.primary_email or "", + min(c.emails) if c.emails else "", + ) + ) + return clusters + + +def _populate( + cluster: _Cluster, + commits_by_email: dict[str, list[tuple[str, str, int]]], + pr_counts: Counter[str], + issue_counts: Counter[str], + asserted_names: dict[str, str], +) -> None: + """Fill in one cluster's aggregates and display values, deterministically. + + ``commit_count`` / ``first_seen`` / ``last_seen`` cover + ``on_default_branch = 1`` commits only. Every "most frequent" choice + breaks ties lexicographically, and ``primary_name`` prefers the + mailmap's proper name over any frequency count β€” it is the string a + human-facing contributors list displays, so a curated name outranks a + tally. + """ + email_commits: Counter[str] = Counter() + name_commits: Counter[str] = Counter() + timestamps: list[str] = [] + for email in cluster.emails: + for name, authored_at, on_default_branch in commits_by_email.get(email, ()): + if on_default_branch != 1: + continue + email_commits[email] += 1 + if name: + name_commits[name] += 1 + if authored_at: + timestamps.append(authored_at) + + cluster.commit_count = sum(email_commits.values()) + cluster.first_seen = min(timestamps) if timestamps else None + cluster.last_seen = max(timestamps) if timestamps else None + cluster.pr_count = sum(pr_counts[login] for login in cluster.logins) + cluster.issue_count = sum(issue_counts[login] for login in cluster.logins) + + # A shared address never becomes anyone's primary β€” it is not theirs. + candidates = {e for e in cluster.emails if e not in _SHARED_EMAILS} + candidates = candidates or cluster.emails + if candidates: + cluster.primary_email = min(candidates, key=lambda e: (-email_commits[e], e)) + + if len(cluster.logins) == 1: + cluster.primary_login = next(iter(cluster.logins)) + elif cluster.logins: + cluster.primary_login = min( + cluster.logins, key=lambda login: (-pr_counts[login], login) + ) + + mailmap_name = asserted_names.get(cluster.primary_email or "") + if mailmap_name is None: + for email in sorted(cluster.emails): + if email in asserted_names: + mailmap_name = asserted_names[email] + break + if mailmap_name is not None: + cluster.primary_name = mailmap_name + cluster.names.add(mailmap_name) + elif name_commits: + cluster.primary_name = min(name_commits, key=lambda n: (-name_commits[n], n)) + elif cluster.names: + cluster.primary_name = min(cluster.names) + + +def _to_author_row(cluster: _Cluster, author_id: int) -> Author: + """Build one ``author`` row from a populated cluster. + + ``emails`` / ``logins`` / ``names`` are ``TEXT`` columns holding + JSON-encoded arrays, not list columns β€” they are serialized here with + ``json.dumps(sorted(...))``, which is also what makes a rebuilt table + byte-identical. + + ``author_id`` is assigned from the cluster's sorted position rather + than left to the database. It keeps a rebuild deterministic by + construction instead of by relying on SQLite reusing rowids after the + ``DELETE``, and it keeps the multi-row insert off SQLAlchemy's + "sentinel column" path, which rejects a batch of rows whose nullable + autoincrement primary key is unset. The value stays ephemeral β€” see + :mod:`whygraph.db.models.author`; nothing may reference it across + scans. + """ + return Author( + id=author_id, + primary_login=cluster.primary_login, + primary_name=cluster.primary_name, + primary_email=cluster.primary_email, + emails=json.dumps(sorted(cluster.emails)), + logins=json.dumps(sorted(cluster.logins)), + names=json.dumps(sorted(cluster.names)), + first_seen=cluster.first_seen, + last_seen=cluster.last_seen, + commit_count=cluster.commit_count, + pr_count=cluster.pr_count, + issue_count=cluster.issue_count, + ) + + +class AuthorResolver(Crawler): + """Rebuild the ``author`` table from commits + GitHub authorship. + + Runs after PR-origin recovery (see the module docstring for why the + ordering is forced by data rather than taste) and rewrites the whole + table in one transaction β€” ``author.id`` is documented as ephemeral, + so a rebuild is the intended shape. + + Best-effort by design: a failure here is logged and reported through + :attr:`Crawler.summary`, leaving :attr:`Crawler.error` ``None`` so a + scan never fails because identity resolution did. The existing table + is left untouched in that case. + + Parameters + ---------- + progress : rich.progress.Progress + Shared Progress instance owned by the orchestrator. + repository : Repository + Used only for Signal 0 (the mailmap). Resolution still works + without a usable git repository. + """ + + def __init__(self, progress: Progress, *, repository: Repository) -> None: + # "authors" is the THREAD name and the key the closing results + # panel looks this crawler up by β€” a mismatch renders "β€” skipped". + super().__init__("authors", progress, total=None) + self._repository = repository + + def work(self) -> None: + try: + with get_session() as session: + clusters = _resolve(session, self._repository) + self.set_total(len(clusters)) + session.exec(delete(Author)) + for index, cluster in enumerate(clusters, start=1): + session.add(_to_author_row(cluster, index)) + self.advance(1) + except Exception as exc: # noqa: BLE001 β€” best-effort phase + _log.warning("author resolution failed; table left as-is: %s", exc) + self.summary = "resolution skipped" + return + + raw = sum(len(c.emails) + len(c.logins) for c in clusters) + noun = "identity" if len(clusters) == 1 else "identities" + self.summary = f"{len(clusters)} {noun} from {raw} raw" diff --git a/src/whygraph/services/git/commands.py b/src/whygraph/services/git/commands.py index 3436039..3a65331 100644 --- a/src/whygraph/services/git/commands.py +++ b/src/whygraph/services/git/commands.py @@ -277,6 +277,38 @@ def parse(self, result: CompletedProcess[str]) -> None: return None +class GitCheckMailmapCmd(ShellCommand[tuple[str, ...]]): + """``git check-mailmap `` β€” canonicalize contacts via the mailmap. + + Each contact is a ``Name `` string; git emits **one line per + contact, in input order**, so a whole repository's identity set costs + a single subprocess. The ``--stdin`` form is deliberately unused: + :meth:`whygraph.core.Shell.run` has no stdin support, and argv + batching sidesteps that entirely. + + Contacts are passed as separate argv tokens β€” no shell is involved, + so a hostile display name cannot inject. + + An unmapped contact is echoed back unchanged, so the worst case is a + no-op rather than a wrong mapping. Note that git canonicalizes the + *name* as well as the email when the mailmap supplies one. + + Parameters + ---------- + contacts : tuple[str, ...] + One or more ``Name `` strings. + """ + + def __init__(self, *contacts: str) -> None: + self.contacts = contacts + + def argv(self) -> list[str]: + return ["git", "check-mailmap", *self.contacts] + + def parse(self, result: CompletedProcess[str]) -> tuple[str, ...]: + return tuple(result.stdout.splitlines()) + + class GitLogCommitCmd(ShellCommand[Commit]): """``git log -1 --shortstat --pretty=format:Commit.LOG_FORMAT ``. diff --git a/src/whygraph/services/git/repository.py b/src/whygraph/services/git/repository.py index 164c6a4..c16eb78 100644 --- a/src/whygraph/services/git/repository.py +++ b/src/whygraph/services/git/repository.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Sequence from functools import cached_property from pathlib import Path @@ -10,6 +11,7 @@ from .blame import BlameHunk from .commands import ( GitBlameCmd, + GitCheckMailmapCmd, GitCurrentBranchCmd, GitDiffCmd, GitDiffTreeFileChangesCmd, @@ -33,6 +35,11 @@ # explicitly so behaviour does not depend on the user's local git config. _BLAME_IGNORE_REVS_FILE = ".git-blame-ignore-revs" +# Contacts per ``git check-mailmap`` invocation. Keeps argv well under +# ARG_MAX on every platform even for a repository with thousands of +# distinct identities; the results are concatenated in input order. +_MAILMAP_CHUNK = 500 + class Repository: """A git repository rooted at a specific working tree on disk. @@ -364,6 +371,55 @@ def fetch_refs(self, refspecs: list[str], *, remote: str | None = None) -> None: f"failed to fetch {len(refspecs)} refspec(s) from {target!r}" ) from exc + def check_mailmap(self, contacts: Sequence[str]) -> tuple[str, ...]: + """Canonicalize ``Name `` contacts through the repo's mailmap. + + Shelling out to ``git check-mailmap`` rather than parsing a file + is deliberate: git honours ``./.mailmap``, ``mailmap.file`` and + ``mailmap.blob`` alike, so a mailmap deliberately kept *outside* + a public repository (the usual arrangement when a contributor + wants their address private) is still applied. A ``./.mailmap`` + parser would silently see nothing. + + Batched via argv β€” one subprocess per :data:`_MAILMAP_CHUNK` + contacts β€” because ``git check-mailmap A B C`` emits one line per + contact **in input order**. Callers therefore zip the result back + onto their inputs positionally, and should treat a ragged result + as a no-op. + + Parameters + ---------- + contacts : Sequence[str] + ``Name `` strings. An empty sequence is a no-op β€” no + ``git`` is invoked. + + Returns + ------- + tuple[str, ...] + One canonicalized ``Name `` line per input contact, in + input order. A contact the mailmap does not mention is echoed + back unchanged β€” no mailmap at all is therefore *not* a + failure, just an identity mapping. + + Raises + ------ + GitError + If ``git`` itself fails β€” the binary is missing, ``root`` is + not a work tree, or the mailmap is malformed. + """ + if not contacts: + return () + lines: list[str] = [] + for start in range(0, len(contacts), _MAILMAP_CHUNK): + chunk = tuple(contacts[start : start + _MAILMAP_CHUNK]) + try: + lines.extend(self._shell.run(GitCheckMailmapCmd(*chunk), cwd=self.root)) + except ShellError as exc: + raise GitError( + f"failed to check-mailmap {len(chunk)} contact(s) at {self.root}" + ) from exc + return tuple(lines) + def commit_metadata(self, ref: str) -> Commit: """Full :class:`Commit` value object for a single commit-ish. diff --git a/tests/test_cli_scan_phases.py b/tests/test_cli_scan_phases.py index 9294ef5..8a2405c 100644 --- a/tests/test_cli_scan_phases.py +++ b/tests/test_cli_scan_phases.py @@ -1,10 +1,12 @@ -"""Tests for ``whygraph scan``'s three-phase orchestration and output. +"""Tests for ``whygraph scan``'s phased orchestration and output. Pins the phase *sequencing* β€” Phase 1 (git + GitHub, concurrent) β†’ Phase 2 -(pr-origins) β†’ Phase 3 (analyze, the LLM long pole, last and alone) β€” plus -the numbered phase headers and the closing results panel. The crawlers are -stubbed with recording stand-ins so no git / GitHub / LLM / CodeGraph work -actually runs; only the orchestrator's ordering and rendering is exercised. +(pr-origins) β†’ Phase 3 (authors, which needs Phase 2's rows) β†’ Phase 4 +(analyze, the LLM long pole, last and alone) β€” plus the numbered phase +headers across every flag combination and the closing results panel. The +crawlers are stubbed with recording stand-ins so no git / GitHub / LLM / +CodeGraph work actually runs; only the orchestrator's ordering and +rendering is exercised. """ from __future__ import annotations @@ -111,11 +113,12 @@ def _patch_crawlers( """Replace every crawler class the CLI touches with a recording stub.""" stubs = { n: _stub(n, order) - for n in ("git", "github", "pr-origins", "analyze", "codegraph") + for n in ("git", "github", "pr-origins", "authors", "analyze", "codegraph") } monkeypatch.setattr(scan_mod, "GitCrawler", stubs["git"]) monkeypatch.setattr(scan_mod, "GitHubCrawler", stubs["github"]) monkeypatch.setattr(scan_mod, "PROriginEnricher", stubs["pr-origins"]) + monkeypatch.setattr(scan_mod, "AuthorResolver", stubs["authors"]) monkeypatch.setattr(scan_mod, "CodeGraphCrawler", stubs["codegraph"]) # AnalyzeCrawler is imported lazily inside scan_cmd β€” patch its source. monkeypatch.setattr("whygraph.scan.AnalyzeCrawler", stubs["analyze"]) @@ -126,7 +129,7 @@ def _idx(order: list[tuple[str, str]], event: tuple[str, str]) -> int: return order.index(event) -def test_three_phases_run_in_order_with_llm_last( +def test_four_phases_run_in_order_with_llm_last( isolated_db: Path, monkeypatch: pytest.MonkeyPatch ) -> None: order: list[tuple[str, str]] = [] @@ -139,14 +142,15 @@ def test_three_phases_run_in_order_with_llm_last( result = CliRunner().invoke(whygraph_main, ["scan"]) assert result.exit_code == 0, result.output - # All five crawlers constructed exactly once. - for name in ("git", "github", "pr-origins", "analyze", "codegraph"): + # All six crawlers constructed exactly once. + for name in ("git", "github", "pr-origins", "authors", "analyze", "codegraph"): assert stubs[name].constructed == 1, name - # Numbered headers for all three phases. - assert "Phase 1/3 Β· Structural crawl" in result.output - assert "Phase 2/3 Β· PR-origin recovery" in result.output - assert "Phase 3/3 Β· LLM descriptions" in result.output + # Numbered headers for all four phases. + assert "Phase 1/4 Β· Structural crawl" in result.output + assert "Phase 2/4 Β· PR-origin recovery" in result.output + assert "Phase 3/4 Β· Author identity" in result.output + assert "Phase 4/4 Β· LLM descriptions" in result.output # CodeGraph is a background task: started first, joined last. assert order[0] == ("start", "codegraph") @@ -158,48 +162,95 @@ def test_three_phases_run_in_order_with_llm_last( # Phase 1 both joined before Phase 2 starts. assert _idx(order, ("join", "git")) < _idx(order, ("start", "pr-origins")) assert _idx(order, ("join", "github")) < _idx(order, ("start", "pr-origins")) - # LLM is strictly last and alone: analyze starts only after pr-origins joins. - assert _idx(order, ("join", "pr-origins")) < _idx(order, ("start", "analyze")) + # Author resolution runs after PR-origin recovery: an address can appear + # only in the on_default_branch=0 rows Phase 2 writes. + assert _idx(order, ("join", "pr-origins")) < _idx(order, ("start", "authors")) + # LLM is strictly last and alone. + assert _idx(order, ("join", "authors")) < _idx(order, ("start", "analyze")) # Closing results panel is present. assert "done in" in result.output -def test_skip_analyze_drops_the_llm_phase( - isolated_db: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - order: list[tuple[str, str]] = [] - stubs = _patch_crawlers(monkeypatch, order) - monkeypatch.setattr( - scan_mod, "_select_github_client", lambda *a, **k: _DummyClient() - ) - - result = CliRunner().invoke(whygraph_main, ["scan", "--skip-analyze"]) - - assert result.exit_code == 0, result.output - # Two phases: structural + pr-origin recovery; no LLM phase. - assert "Phase 1/2 Β· Structural crawl" in result.output - assert "Phase 2/2 Β· PR-origin recovery" in result.output - assert "Β· LLM descriptions" not in result.output - assert stubs["analyze"].constructed == 0 - - -def test_single_phase_when_remote_and_llm_disabled( - isolated_db: Path, monkeypatch: pytest.MonkeyPatch +@pytest.mark.parametrize( + ("flags", "remote", "expected", "absent"), + [ + pytest.param( + [], + True, + [ + "Phase 1/4 Β· Structural crawl", + "Phase 2/4 Β· PR-origin recovery", + "Phase 3/4 Β· Author identity", + "Phase 4/4 Β· LLM descriptions", + ], + [], + id="default", + ), + pytest.param( + ["--skip-analyze"], + True, + [ + "Phase 1/3 Β· Structural crawl", + "Phase 2/3 Β· PR-origin recovery", + "Phase 3/3 Β· Author identity", + ], + ["Β· LLM descriptions"], + id="skip-analyze", + ), + pytest.param( + ["--no-remote"], + False, + [ + "Phase 1/3 Β· Structural crawl", + "Phase 2/3 Β· Author identity", + "Phase 3/3 Β· LLM descriptions", + ], + ["Β· PR-origin recovery"], + id="no-remote", + ), + pytest.param( + ["--no-remote", "--skip-analyze"], + False, + [ + "Phase 1/2 Β· Structural crawl", + "Phase 2/2 Β· Author identity", + ], + ["Β· PR-origin recovery", "Β· LLM descriptions"], + id="no-remote-skip-analyze", # the git-hook path + ), + ], +) +def test_phase_numbering_across_flag_combinations( + isolated_db: Path, + monkeypatch: pytest.MonkeyPatch, + flags: list[str], + remote: bool, + expected: list[str], + absent: list[str], ) -> None: + """``phase_total`` is computed, so every combination is pinned β€” a stale + count mislabels every header (e.g. two "Phase 3/3" rules).""" order: list[tuple[str, str]] = [] stubs = _patch_crawlers(monkeypatch, order) + if remote: + monkeypatch.setattr( + scan_mod, "_select_github_client", lambda *a, **k: _DummyClient() + ) + monkeypatch.setattr("whygraph.analyze.LlmDescriptor", _DummyDescriptor) - result = CliRunner().invoke( - whygraph_main, ["scan", "--no-remote", "--skip-analyze"] - ) + result = CliRunner().invoke(whygraph_main, ["scan", *flags]) assert result.exit_code == 0, result.output - assert "Phase 1/1 Β· Structural crawl" in result.output - assert "Phase 2" not in result.output - assert stubs["github"].constructed == 0 - assert stubs["pr-origins"].constructed == 0 - assert stubs["analyze"].constructed == 0 + for header in expected: + assert header in result.output + for header in absent: + assert header not in result.output + # Author resolution is local-only, so it runs under every combination. + assert stubs["authors"].constructed == 1 + # `n` never exceeds `phase_total`. + total = len(expected) + assert f"Phase {total + 1}/" not in result.output class _Fake: @@ -230,7 +281,7 @@ def test_results_panel_is_defensive_and_total( _Fake("git", summary="5 commits (5 new)"), _Fake("github", summary="2 PRs Β· 1 issues"), _Fake("analyze", error=RuntimeError("boom")), # failed β†’ βœ— - # pr-origins absent from `ran` β†’ "β€” skipped" row + # pr-origins and authors absent from `ran` β†’ "β€” skipped" rows ] codegraph = _Fake("codegraph", warning="CodeGraph refresh skipped β€” no binary") @@ -246,6 +297,7 @@ def test_results_panel_is_defensive_and_total( out = buf.getvalue() assert "βœ—" in out # failed analyze assert "⚠" in out # codegraph warning + assert "Author identity" in out # the row exists even when absent from `ran` assert "skipped" in out # absent pr-origins assert "Scan log" in out # R11: path row retained assert "done in" in out # total elapsed in the title diff --git a/tests/test_mcp_resources.py b/tests/test_mcp_resources.py index 726d84b..a77c8cb 100644 --- a/tests/test_mcp_resources.py +++ b/tests/test_mcp_resources.py @@ -18,7 +18,7 @@ import pytest from whygraph.db import get_session -from whygraph.db.models import Commit, Issue, PRIssueLink, PullRequest +from whygraph.db.models import Author, Commit, Issue, PRIssueLink, PullRequest from whygraph.mcp.errors import WhyGraphError from whygraph.mcp.resources import ( _commit_resource, @@ -372,6 +372,57 @@ def test_repo_overview_populated(whygraph_db_initialized: Path) -> None: ] +def test_top_contributors_ignores_a_populated_author_table( + whygraph_db_initialized: Path, +) -> None: + """``top_contributors`` stays ``Commit``-based even once author identity + resolution has run, so this resource's output does not change under a + scan that populates ``author``.""" + with get_session() as session: + session.add( + _db_commit( + "a" * 40, + authored_at="2026-01-01T00:00:00+00:00", + author_name="Alice", + author_email="alice@example.com", + ) + ) + session.add( + _db_commit( + "b" * 40, + authored_at="2026-02-01T00:00:00+00:00", + author_name="Bob", + author_email="bob@example.com", + ) + ) + # A resolved identity that (wrongly, for this fixture) claims both + # addresses β€” if the resource ever consulted `author`, one row would + # collapse the two contributors and this assertion would fail. + session.add( + Author( + id=1, + primary_login="alice", + primary_name="Alice", + primary_email="alice@example.com", + emails=json.dumps(["alice@example.com", "bob@example.com"]), + logins=json.dumps(["alice"]), + names=json.dumps(["Alice", "Bob"]), + commit_count=2, + ) + ) + + result = _repo_overview_resource() + + assert result["top_contributors"] == [ + { + "author_name": "Alice", + "author_email": "alice@example.com", + "commit_count": 1, + }, + {"author_name": "Bob", "author_email": "bob@example.com", "commit_count": 1}, + ] + + def test_resource_errors_on_uninitialized_db(whygraph_db: Path) -> None: """Every resource raises ``WhyGraphError`` when the DB hasn't been migrated. diff --git a/tests/test_scan_authors.py b/tests/test_scan_authors.py new file mode 100644 index 0000000..23b2c0b --- /dev/null +++ b/tests/test_scan_authors.py @@ -0,0 +1,588 @@ +"""Tests for :mod:`whygraph.scan.authors` (author identity resolution). + +Covers the pure helpers, then resolution against seeded fixtures. The +highest-value cases here are the *negative* ones β€” same display name stays +two rows, a shared address never merges β€” because a false merge silently +attributes one person's work to another and has no symptom, while a false +split is merely untidy. + +``Repository.check_mailmap`` is stubbed throughout rather than writing a +real mailmap into a temp repo: the stub keeps these tests hermetic and +lets the git-unavailable degradation path be exercised by raising. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from rich.progress import Progress +from sqlmodel import func, select + +from whygraph.db import get_session +from whygraph.db.models import Author, Commit, Issue, PullRequest +from whygraph.scan.authors import ( + AuthorResolver, + _is_bot, + _json_list, + _login_from_noreply, + _Union, +) +from whygraph.services.git import GitError + +_MTR = "tsvetoslav.nikolov@mtr-design.com" +_PFIZER = "tsvetoslav.nikolov@pfizer.com" +_NOREPLY = "85567502+cvetty@users.noreply.github.com" +_SHARED = "noreply@github.com" + + +# --- fixtures ---------------------------------------------------------------- + + +def _commit( + sha: str, + *, + name: str, + email: str, + on_default_branch: int = 1, + authored_at: str = "2026-01-01T00:00:00+00:00", +) -> Commit: + return Commit( + sha=sha, + parent_shas="", + author_name=name, + author_email=email, + authored_at=authored_at, + committed_at=authored_at, + subject=sha, + body="", + files_changed=1, + insertions=1, + deletions=0, + scanned_at="2026-01-02T00:00:00+00:00", + on_default_branch=on_default_branch, + ) + + +def _pr( + number: int, + *, + author: str | None, + triples: list[tuple[str, str, str]] | None = None, + commit_titles: str | None = None, +) -> PullRequest: + if commit_titles is None: + commit_titles = json.dumps( + [ + { + "oid": f"oid{number}{i}", + "headline": "work", + "author_login": login, + "author_name": name, + "author_email": email, + } + for i, (login, name, email) in enumerate(triples or []) + ] + ) + return PullRequest( + number=number, + title=f"PR {number}", + state="MERGED", + created_at="2026-01-01T00:00:00+00:00", + updated_at="2026-01-01T00:00:00+00:00", + merged_at="2026-01-02T00:00:00+00:00", + merge_commit_sha=None, + head_sha=f"head{number}", + base_ref="main", + html_url=f"https://example.test/pr/{number}", + labels="[]", + author=author, + fetched_at="2026-01-02T00:00:00+00:00", + commit_titles=commit_titles, + ) + + +def _issue(number: int, *, author: str | None) -> Issue: + return Issue( + number=number, + title=f"Issue {number}", + body="", + state="OPEN", + created_at="2026-01-01T00:00:00+00:00", + updated_at="2026-01-01T00:00:00+00:00", + author=author, + html_url=f"https://example.test/issue/{number}", + labels="[]", + fetched_at="2026-01-02T00:00:00+00:00", + ) + + +class _StubRepo: + """Duck-typed :class:`Repository` β€” only ``check_mailmap`` is called. + + The default instance is an *identity* mailmap (every contact echoed + back), which is what a repository with no mailmap actually produces. + """ + + def __init__( + self, + mapping: dict[str, str] | None = None, + *, + raises: bool = False, + ragged: bool = False, + ) -> None: + self._mapping = mapping or {} + self._raises = raises + self._ragged = ragged + self.calls = 0 + + def check_mailmap(self, contacts) -> tuple[str, ...]: + self.calls += 1 + if self._raises: + raise GitError("git unavailable") + out = [self._mapping.get(_email_of(c), c) for c in contacts] + return tuple(out[:-1]) if self._ragged else tuple(out) + + +def _email_of(contact: str) -> str: + return contact.rpartition("<")[2].rstrip(">") + + +def _resolve(repo: _StubRepo | None = None) -> tuple[AuthorResolver, list[dict]]: + """Run the crawler and return it plus the ``author`` rows it wrote.""" + resolver = AuthorResolver(Progress(), repository=repo or _StubRepo()) + resolver.run() + with get_session() as session: + rows = [a.model_dump() for a in session.exec(select(Author)).all()] + return resolver, rows + + +# --- pure helpers ------------------------------------------------------------ + + +@pytest.mark.parametrize( + ("email", "expected"), + [ + (_NOREPLY, "cvetty"), + ("cvetty@users.noreply.github.com", "cvetty"), # the older form + (_MTR, None), + ("weird+@users.noreply.github.com", None), # empty local part + (_SHARED, None), # different domain entirely + ], +) +def test_login_from_noreply(email: str, expected: str | None) -> None: + assert _login_from_noreply(email) == expected + + +@pytest.mark.parametrize( + ("email", "login", "expected"), + [ + ("x@example.com", "github-actions[bot]", True), + ("41898282+github-actions[bot]@users.noreply.github.com", None, True), + (_MTR, "cvetty", False), + ], +) +def test_is_bot(email: str, login: str | None, expected: bool) -> None: + assert _is_bot(email, login) is expected + + +@pytest.mark.parametrize("raw", [None, "", "{}", "not json", '"a string"']) +def test_json_list_never_raises(raw: str | None) -> None: + assert _json_list(raw) == [] + + +def test_union_is_transitive_idempotent_and_namespaced() -> None: + u = _Union() + u.union(("email", "a"), ("login", "x")) + u.union(("login", "x"), ("email", "b")) + u.union(("email", "a"), ("login", "x")) # idempotent re-union + + assert u.find(("email", "a")) == u.find(("email", "b")) + # A login and an identical display-name/email string are distinct nodes. + u.add(("email", "x")) + assert u.find(("email", "x")) != u.find(("login", "x")) + + +# --- the headline case: this repo's real identities -------------------------- + + +def _seed_real_identities(session) -> None: + """Β§0's four real identities plus Β§3.3's three real GitHub triples.""" + session.add( + _commit( + "d1", name="cvetty", email=_MTR, authored_at="2026-01-01T00:00:00+00:00" + ) + ) + session.add( + _commit( + "d2", name="cvetty", email=_MTR, authored_at="2026-02-01T00:00:00+00:00" + ) + ) + session.add( + _commit( + "d3", name="cvetty", email=_MTR, authored_at="2026-03-01T00:00:00+00:00" + ) + ) + session.add(_commit("n1", name="Tsvetoslav Nikolov", email=_NOREPLY)) + session.add(_commit("n2", name="Tsvetoslav Nikolov", email=_NOREPLY)) + # The @pfizer.com address is visible ONLY in PR-origin rows. + session.add(_commit("o1", name="cvetty", email=_MTR, on_default_branch=0)) + session.add(_commit("o2", name="cvetty", email=_PFIZER, on_default_branch=0)) + session.add( + _pr( + 1, + author="cvetty", + triples=[ + ("cvetty", "cvetty", _MTR), + ("cvetty", "cvetty", _PFIZER), + ("cvetty", "Tsvetoslav Nikolov", _NOREPLY), + ], + ) + ) + session.commit() + + +def test_real_identities_resolve_to_one_row(whygraph_db_initialized: Path) -> None: + """Four raw identities across three emails and two names β€” one human.""" + with get_session() as session: + _seed_real_identities(session) + + resolver, rows = _resolve() + + assert resolver.error is None + assert len(rows) == 1 + row = rows[0] + assert row["primary_login"] == "cvetty" + assert row["primary_email"] == _MTR # 3 default-branch commits vs 2 vs 0 + assert row["primary_name"] == "cvetty" + assert json.loads(row["emails"]) == sorted([_MTR, _PFIZER, _NOREPLY]) + assert json.loads(row["logins"]) == ["cvetty"] + assert json.loads(row["names"]) == ["Tsvetoslav Nikolov", "cvetty"] + assert row["pr_count"] == 1 + assert row["issue_count"] == 0 + + +def test_pr_origin_only_email_lands_in_emails(whygraph_db_initialized: Path) -> None: + """The @pfizer.com address appears only in on_default_branch=0 rows, so + this pins the ordering invariant: resolution must run after Phase 2.""" + with get_session() as session: + _seed_real_identities(session) + + _, rows = _resolve() + + assert _PFIZER in json.loads(rows[0]["emails"]) + + +def test_commit_count_is_the_db_default_branch_count( + whygraph_db_initialized: Path, +) -> None: + """``commit_count`` equals the DB's own default-branch count for the + cluster's emails β€” asserted as an identity, never against a constant, + because ``on_default_branch`` records scan-time state.""" + with get_session() as session: + _seed_real_identities(session) + + _, rows = _resolve() + emails = json.loads(rows[0]["emails"]) + + with get_session() as session: + expected = session.exec( + select(func.count()) + .select_from(Commit) + .where(Commit.on_default_branch == 1) + .where(Commit.author_email.in_(emails)) + ).one() + all_rows = session.exec( + select(func.count()) + .select_from(Commit) + .where(Commit.author_email.in_(emails)) + ).one() + + assert rows[0]["commit_count"] == expected + # PR-origin rows are the same work as their squash commit β€” excluded. + assert rows[0]["commit_count"] < all_rows + + +# --- the false-merge guards -------------------------------------------------- + + +def test_same_display_name_stays_two_rows(whygraph_db_initialized: Path) -> None: + """The most important test here: name is never merge evidence, because + "Alex Kim" is not one person globally.""" + with get_session() as session: + session.add(_commit("a1", name="Alex Kim", email="alex@one.example")) + session.add(_commit("a2", name="Alex Kim", email="alex@two.example")) + session.commit() + + _, rows = _resolve() + + assert len(rows) == 2 + assert {r["primary_email"] for r in rows} == { + "alex@one.example", + "alex@two.example", + } + + +def test_shared_email_does_not_merge_two_logins(whygraph_db_initialized: Path) -> None: + """``noreply@github.com`` is shared across users β€” unioning on it would + collapse the whole team. It is still recorded in both rows.""" + with get_session() as session: + session.add(_commit("s1", name="Alice", email="alice@example.com")) + session.add(_commit("s2", name="Bob", email="bob@example.com")) + session.add(_commit("s3", name="Web Flow", email=_SHARED)) + session.add( + _pr(1, author="alice", triples=[("alice", "Alice", "alice@example.com")]) + ) + session.add(_pr(2, author="bob", triples=[("bob", "Bob", "bob@example.com")])) + # GitHub reports the shared address for both logins. + session.add(_pr(3, author="alice", triples=[("alice", "Alice", _SHARED)])) + session.add(_pr(4, author="bob", triples=[("bob", "Bob", _SHARED)])) + session.commit() + + _, rows = _resolve() + + assert len(rows) == 2 + by_login = {r["primary_login"]: r for r in rows} + assert set(by_login) == {"alice", "bob"} + for row in rows: + assert _SHARED in json.loads(row["emails"]) + # …and it never becomes anyone's primary. + assert row["primary_email"] != _SHARED + + +def test_unclaimed_shared_email_keeps_its_own_row( + whygraph_db_initialized: Path, +) -> None: + """A shared address no login claimed still forms a row β€” it is never + used as merge evidence, but it does not disappear either.""" + with get_session() as session: + session.add(_commit("u1", name="Web Flow", email=_SHARED)) + session.commit() + + _, rows = _resolve() + + assert [json.loads(r["emails"]) for r in rows] == [[_SHARED]] + + +def test_bot_forms_its_own_cluster(whygraph_db_initialized: Path) -> None: + with get_session() as session: + session.add(_commit("h1", name="Alice", email="alice@example.com")) + session.add( + _commit( + "b1", + name="github-actions[bot]", + email="41898282+github-actions[bot]@users.noreply.github.com", + ) + ) + session.commit() + + _, rows = _resolve() + + assert len(rows) == 2 + bot = [r for r in rows if r["primary_login"] == "github-actions[bot]"] + human = [r for r in rows if r["primary_email"] == "alice@example.com"] + assert len(bot) == 1 and len(human) == 1 + assert "alice@example.com" not in json.loads(bot[0]["emails"]) + + +# --- offline / partial GitHub data ------------------------------------------- + + +def test_noreply_parse_works_with_no_pr_rows(whygraph_db_initialized: Path) -> None: + """Signal B is pure string work, so a --no-remote scan still recovers + the login behind a GitHub noreply address.""" + with get_session() as session: + session.add(_commit("nb1", name="Tsvetoslav Nikolov", email=_NOREPLY)) + session.commit() + + _, rows = _resolve() + + assert len(rows) == 1 + assert rows[0]["primary_login"] == "cvetty" + assert json.loads(rows[0]["logins"]) == ["cvetty"] + + +def test_pr_author_who_never_committed_gets_a_row( + whygraph_db_initialized: Path, +) -> None: + with get_session() as session: + session.add(_pr(1, author="reviewer", triples=[])) + session.commit() + + _, rows = _resolve() + + assert len(rows) == 1 + assert rows[0]["primary_login"] == "reviewer" + assert rows[0]["commit_count"] == 0 + assert rows[0]["pr_count"] == 1 + assert rows[0]["first_seen"] is None and rows[0]["last_seen"] is None + + +def test_null_pr_author_produces_no_phantom_row( + whygraph_db_initialized: Path, +) -> None: + """A deleted GitHub account leaves ``author IS NULL`` β€” no row for None.""" + with get_session() as session: + session.add(_commit("g1", name="Alice", email="alice@example.com")) + session.add(_pr(1, author=None, triples=[])) + session.commit() + + resolver, rows = _resolve() + + assert resolver.error is None + assert len(rows) == 1 + assert rows[0]["primary_email"] == "alice@example.com" + + +def test_issue_counts_are_attributed_to_the_login( + whygraph_db_initialized: Path, +) -> None: + """Synthetic by necessity β€” this repo has zero issues scanned.""" + with get_session() as session: + session.add(_commit("i1", name="Alice", email="alice@example.com")) + session.add( + _pr(1, author="alice", triples=[("alice", "Alice", "alice@example.com")]) + ) + session.add(_issue(10, author="alice")) + session.add(_issue(11, author="alice")) + session.add(_issue(12, author="bob")) + session.commit() + + _, rows = _resolve() + + by_login = {r["primary_login"]: r for r in rows} + assert by_login["alice"]["issue_count"] == 2 + assert by_login["bob"]["issue_count"] == 1 + assert by_login["bob"]["commit_count"] == 0 + + +def test_malformed_commit_titles_are_skipped(whygraph_db_initialized: Path) -> None: + with get_session() as session: + session.add(_commit("m1", name="Alice", email="alice@example.com")) + session.add(_pr(1, author="alice", commit_titles="}{ not json")) + session.add(_pr(2, author="alice", commit_titles=json.dumps(["a string", 7]))) + session.add( + _pr(3, author="alice", triples=[("alice", "Alice", "alice@example.com")]) + ) + session.commit() + + resolver, rows = _resolve() + + assert resolver.error is None + assert len(rows) == 1 + assert rows[0]["primary_login"] == "alice" + assert rows[0]["pr_count"] == 3 + + +# --- determinism ------------------------------------------------------------- + + +def test_two_consecutive_runs_are_byte_identical( + whygraph_db_initialized: Path, +) -> None: + with get_session() as session: + _seed_real_identities(session) + session.add(_commit("z1", name="Alice", email="alice@example.com")) + session.add(_commit("z2", name="Bob", email="bob@example.com")) + session.commit() + + _, first = _resolve() + _, second = _resolve() + + assert first == second + assert [r["id"] for r in second] == list(range(1, len(second) + 1)) + + +# --- Signal 0 Β· the mailmap -------------------------------------------------- + + +def test_mailmap_merges_emails_nothing_else_could( + whygraph_db_initialized: Path, +) -> None: + """Two ordinary addresses with no GitHub data and no noreply form β€” + the case only a human-curated mailmap can fix.""" + with get_session() as session: + session.add(_commit("mm1", name="Real Person", email="real@example.com")) + session.add(_commit("mm2", name="rp", email="alias@example.com")) + session.commit() + + repo = _StubRepo({"alias@example.com": "Real Person "}) + _, rows = _resolve(repo) + + assert repo.calls == 1 # ONE subprocess for every identity + assert len(rows) == 1 + assert json.loads(rows[0]["emails"]) == ["alias@example.com", "real@example.com"] + + +def test_mailmap_name_outranks_the_frequency_winner( + whygraph_db_initialized: Path, +) -> None: + """``primary_name`` is what a contributors list displays, so a curated + name beats a tally β€” even when the tally says otherwise.""" + with get_session() as session: + session.add(_commit("f1", name="cvetty", email=_MTR)) + session.add(_commit("f2", name="cvetty", email=_MTR)) + session.add(_commit("f3", name="cvetty", email=_MTR)) + session.commit() + + repo = _StubRepo({_MTR: f"Tsvetoslav Nikolov <{_MTR}>"}) + _, rows = _resolve(repo) + + assert rows[0]["primary_name"] == "Tsvetoslav Nikolov" + assert "cvetty" in json.loads(rows[0]["names"]) + + +def test_mailmap_cannot_defeat_the_shared_address_denylist( + whygraph_db_initialized: Path, +) -> None: + """Even a careless mailmap mapping a shared address must not merge the + identities behind it.""" + with get_session() as session: + session.add(_commit("d1", name="Alice", email="alice@example.com")) + session.add(_commit("d2", name="Web Flow", email=_SHARED)) + session.commit() + + # The mailmap claims the shared address belongs to Alice. + repo = _StubRepo({_SHARED: "Alice "}) + _, rows = _resolve(repo) + + assert len(rows) == 2 + alice = [r for r in rows if r["primary_email"] == "alice@example.com"][0] + assert _SHARED not in json.loads(alice["emails"]) + + +def test_mailmap_failure_degrades_to_signals_abc( + whygraph_db_initialized: Path, +) -> None: + """git absent / not a work tree: resolution completes on A/B/C, sets a + summary, and leaves ``error`` None β€” a scan never fails because + identity resolution did.""" + with get_session() as session: + session.add(_commit("gf1", name="Tsvetoslav Nikolov", email=_NOREPLY)) + session.commit() + + resolver, rows = _resolve(_StubRepo(raises=True)) + + assert resolver.error is None + assert resolver.summary and "identit" in resolver.summary + assert len(rows) == 1 + assert rows[0]["primary_login"] == "cvetty" # Signal B still fired + + +def test_ragged_mailmap_output_is_a_noop(whygraph_db_initialized: Path) -> None: + """``check-mailmap`` output is positional, so a short list must be + discarded rather than mis-zipped onto the inputs.""" + with get_session() as session: + session.add(_commit("r1", name="Real Person", email="real@example.com")) + session.add(_commit("r2", name="rp", email="alias@example.com")) + session.commit() + + repo = _StubRepo( + {"alias@example.com": "Real Person "}, ragged=True + ) + _, rows = _resolve(repo) + + # No merge β€” the mapping was thrown away rather than applied to the + # wrong contact. + assert len(rows) == 2 diff --git a/tests/test_services_git_mailmap.py b/tests/test_services_git_mailmap.py new file mode 100644 index 0000000..f0b3c03 --- /dev/null +++ b/tests/test_services_git_mailmap.py @@ -0,0 +1,129 @@ +"""Tests for ``GitCheckMailmapCmd`` and :meth:`Repository.check_mailmap`. + +The command's argv shape is pinned directly (argv batching, **not** +``--stdin`` β€” ``Shell.run`` has no stdin support), and the repository +method is exercised against a real ``git`` binary with a real +``.mailmap`` on disk, matching ``test_services_git_fetch_metadata.py``. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from subprocess import CompletedProcess + +import pytest + +from whygraph.services.git import GitError, Repository +from whygraph.services.git import repository as repo_mod +from whygraph.services.git.commands import GitCheckMailmapCmd + + +def _git(cwd: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(cwd), *args], + check=True, + capture_output=True, + text=True, + ).stdout + + +def _init(root: Path) -> None: + root.mkdir(parents=True, exist_ok=True) + _git(root, "init", "-q", "-b", "main") + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "Test User") + _git(root, "config", "commit.gpgsign", "false") + + +def _completed(stdout: str) -> CompletedProcess[str]: + return CompletedProcess(args=["git"], returncode=0, stdout=stdout, stderr="") + + +# --- the command ------------------------------------------------------------ + + +def test_argv_batches_contacts_and_never_uses_stdin() -> None: + contacts = ("a ", "b ", "c ") + + argv = GitCheckMailmapCmd(*contacts).argv() + + assert argv == ["git", "check-mailmap", *contacts] + assert "--stdin" not in argv + + +def test_parse_preserves_line_order() -> None: + out = "A \nB \nC \n" + + parsed = GitCheckMailmapCmd().parse(_completed(out)) + + assert parsed == ("A ", "B ", "C ") + + +# --- the repository method -------------------------------------------------- + + +def test_check_mailmap_empty_never_spawns_git( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + def _boom(*_a: object, **_kw: object) -> None: + raise AssertionError("git must not be invoked for an empty contact list") + + repo = Repository(tmp_path) + monkeypatch.setattr(repo._shell, "run", _boom) + + assert repo.check_mailmap([]) == () + + +def test_check_mailmap_canonicalizes_name_and_email(tmp_path: Path) -> None: + """A real mailmap folds two aliases onto one proper contact β€” including + rewriting the display name, which is what makes ``primary_name`` + implementable.""" + root = tmp_path / "repo" + _init(root) + (root / ".mailmap").write_text( + "Real Person \n" + "Real Person <12345+person@users.noreply.github.com>\n" + ) + + out = Repository(root).check_mailmap( + [ + "whatever ", + "x <12345+person@users.noreply.github.com>", + "y ", + ] + ) + + assert out[0] == "Real Person " + assert out[1] == "Real Person " + # An unknown contact passes through untouched β€” worst case is a no-op. + assert out[2] == "y " + + +def test_check_mailmap_chunks_large_input_preserving_order( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """1,200 contacts become 3 invocations of <= 500, concatenated in input + order β€” so argv can never approach ARG_MAX.""" + monkeypatch.setattr(repo_mod, "_MAILMAP_CHUNK", 500) + contacts = [f"n{i} " for i in range(1200)] + calls: list[int] = [] + + def _fake_run(cmd: GitCheckMailmapCmd, **_kw: object) -> tuple[str, ...]: + calls.append(len(cmd.contacts)) + return tuple(cmd.contacts) + + repo = Repository(tmp_path) + monkeypatch.setattr(repo._shell, "run", _fake_run) + + out = repo.check_mailmap(contacts) + + assert calls == [500, 500, 200] + assert list(out) == contacts + + +def test_check_mailmap_git_failure_raises_git_error(tmp_path: Path) -> None: + """A non-git directory surfaces as GitError, not ShellError β€” matching + every sibling method on Repository.""" + with pytest.raises(GitError): + Repository(tmp_path).check_mailmap(["x "])