Skip to content

feat(scan): author identity resolution — one row per human - #39

Merged
cvetty merged 1 commit into
mainfrom
feature/author-identity
Jul 31, 2026
Merged

feat(scan): author identity resolution — one row per human#39
cvetty merged 1 commit into
mainfrom
feature/author-identity

Conversation

@cvetty

@cvetty cvetty commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Populates the long-empty author table, so per-developer queries stop reporting one person several times.

Implements plans/author-identity-plan.md rev 3.

Why

The obvious contributor query on this repo returns four rows that are all the same human — three emails, two display names:

author_name author_email on_default_branch
cvetty tsvetoslav.nikolov@mtr-design.com 1
Tsvetoslav Nikolov 85567502+cvetty@users.noreply.github.com 1
cvetty tsvetoslav.nikolov@mtr-design.com 0
cvetty tsvetoslav.nikolov@pfizer.com 0

Any per-developer chart or ranking built on a bare GROUP BY is wrong, and looks authoritative while being wrong. The author table, both its indexes, and its intended design have shipped since the initial schema — db/models/author.py names whygraph.scan.authors as its producer, and that module simply did not exist. This adds it.

How

Resolution is evidence-driven, never heuristic, in descending order of authority:

  1. git's mailmap — a human explicitly asserting "these contacts are me". Read via git check-mailmap rather than parsing ./.mailmap, so a mailmap kept outside a public repo (mailmap.file / mailmap.blob — what a developer who needs email privacy does, and what this repo does) still applies. It also canonicalizes the name, which is what makes primary_name implementable. Batched via argv: one subprocess for every identity.
  2. GitHub's own triplespull_request.commit_titles entries already carry author_login / author_name / author_email together. On this repo that single signal resolves all three emails and both names with no inference at all.
  3. The noreply parse12345+login@users.noreply.github.comlogin. Pure string work, so it still resolves identities under --no-remote.
  4. Byte-equal emails — implicit; the same address is the same node by construction.

What it 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 (noreply@github.com) or bot address as evidence. A false merge silently attributes one person's work to another and has no symptom; a false split is merely untidy. Union–find runs over namespaced nodes (("email", …) / ("login", …)) so a login and an identical display name cannot merge by coincidence.

Two ordering invariants, both forced by data rather than taste:

  • The phase runs after PR-origin recovery — @pfizer.com appears only in the on_default_branch=0 rows Phase 2 writes.
  • commit_count / first_seen / last_seen cover on_default_branch=1 only, so squashed work is not double-counted.

The phase is local, token-free and millisecond-scale, so it always runs — including under --no-remote and in the auto-rescan git hooks. Output is fully deterministic (clusters sorted, JSON arrays sorted, ties lexicographic), so the rebuilt-per-scan table produces no diff noise.

Nothing reads the table yet — deliberately. author was already in the chat SQL allowlist, so it becomes useful the moment it has rows, and the algorithm's real failure modes are better learned from a second and third repo than from a reader built on day one.

Changes

NEW scan/authors.py AuthorResolver + the resolution algorithm
services/git/commands.py, repository.py GitCheckMailmapCmd + Repository.check_mailmap() (argv-batched, chunked at 500, ShellErrorGitError)
cli/commands/scan.py the 👥 phase between PR-origins and LLM descriptions, plus phase_total and the results-panel row
scan/__init__.py export
NEW tests/test_scan_authors.py, tests/test_services_git_mailmap.py 38 tests
tests/test_cli_scan_phases.py, tests/test_mcp_resources.py phase-numbering matrix; top_contributors regression

No new Alembic revision, no config key, no CLI flag, no new dependency.

Verified on this repo

1|cvetty|cvetty|tsvetoslav.nikolov@mtr-design.com
 |["85567502+cvetty@users.noreply.github.com", "…@mtr-design.com", "…@pfizer.com"]
 |["cvetty"]|["Tsvetoslav Nikolov", "cvetty"]|…|170|38|0

One row — the plan's §3.8 shape. Two consecutive scans produce byte-identical rows. Through the chat SQL surface, grouping via author now returns one developer where the raw author_email grouping still returns two.

commit_count read 170, not the plan's measured 169, because the scan indexed one new commit. The ACs assert an identity against the DB rather than a constant — commit_count == COUNT(*) WHERE on_default_branch = 1 AND author_email IN (emails) evaluates 170 == 170. That drift is the plan's risk 11 behaving as designed, not a regression.

Two deviations from the plan, both forced

  1. _to_author_row assigns author.id explicitly (1..n in sorted cluster order) instead of leaving it to the DB. Not anticipated by the plan, and a real defect: SQLAlchemy's insertmanyvalues path rejects a multi-row insert whose nullable autoincrement PK is unset. This repo resolves to one row, so the live smoke passed — only the multi-row fixtures exposed it. It also makes the determinism guarantee true by construction rather than by relying on SQLite reusing rowids after the DELETE. id stays ephemeral; nothing references it.
  2. The results-panel row uses _optional_phase_cells, not the plan's bare _status_glyph. The plan's reasoning ("resolution always runs, so it is never None") holds for the CLI, but _render_results_panel is also called directly by an existing test with a ran list that omits it — a bare resolver.error would crash there, breaking that function's documented promise that it can never turn a successful crawl into a crash. Renders identically in every real scan.

One design point also needed a clause the plan lacked: a denied shared address is both a cluster attribute and a node, which would leave a phantom singleton row beside the two clusters the shared-email test expects. It is now dropped once some cluster has claimed it, and kept when none has.

Gates

ruff check src/ tests/ scripts/ ✓ · ruff format --check src/ tests/ scripts/ ✓ · pytest832 passed

Populates the long-empty `author` table, so per-developer queries stop
reporting one person three times. On this repo the obvious contributor
query returns four rows — three emails, two display names, one human.

Resolution is evidence-driven, never heuristic, in descending authority:
git's mailmap (a human asserting identity, read via `git check-mailmap`
so a mailmap kept outside the repo still applies), GitHub's own
login/name/email triples from `pull_request.commit_titles`, a
deterministic parse of GitHub noreply addresses, and byte-equal emails.
It never merges on display name, never fuzzy-matches, and never treats a
shared or bot address as evidence — a false merge silently attributes one
person's work to another, while a false split is merely untidy.

The phase runs after PR-origin recovery: one address appears only in the
on_default_branch=0 rows Phase 2 writes. It is local, token-free and
millisecond-scale, so it always runs — including under --no-remote and in
the auto-rescan git hooks. Output is fully deterministic, so the
rebuilt-per-scan table produces no diff noise.

Nothing reads the table yet; `author` was already in the chat SQL
allowlist, so it becomes useful the moment it has rows.

Implements plans/author-identity-plan.md rev 3.
@cvetty cvetty self-assigned this Jul 31, 2026
@cvetty cvetty added the enhancement New feature or request label Jul 31, 2026
@cvetty
cvetty merged commit bcbde74 into main Jul 31, 2026
2 checks passed
cvetty added a commit that referenced this pull request Jul 31, 2026
…er_chart (#40)

* fix(chat): _SCHEMA_DOC — author table is populated, add identity rule 5

PR #39 populated the `author` table but left the stats schema doc telling the
model the opposite ("POPULATED BY A SEPARATE SCAN STEP THAT OFTEN HAS NOT RUN"),
steering every contributor question at raw git identities — which split one human
into several rows.

Replace that paragraph with the real shape, and add a fifth rule routing
per-developer grouping through the table. Both SQL forms in the rule were
executed against the live DB before being written down:

- all-time: SUM(commit_count) GROUP BY id — the bare column read is refused by
  _check_shape, since reading a pre-computed column is not an aggregate.
- time-sliced: instr(a.emails, '"' || c.author_email || '"') > 0. json_each is
  denied by the authorizer, and a LIKE match treats '_' as a wildcard, so it can
  attribute one person's commits to another.

Defect fix on main, independent of the charts work.

* feat(chat): two stats surfaces + a generic render_chart tool (backend)

Extract the four SQL security layers into chat/sql_guard.py, parameterized by a
SqlSurface (label, frozenset-literal table allowlist, deferred db_path, per-surface
missing-DB message). Behaviour-preserving: test_chat_stats_sql.py passes with one
line changed — the runaway-query test now patches sql_guard._TIMEOUT_SEC, since
layer 4 is where the deadline is read from.

Add graph_stats_sql.py: the same fence over CodeGraph's own .codegraph/codegraph.db,
which run_project_stats structurally cannot reach. Allowlist is {nodes, edges,
files} as a literal — CodeGraph's schema is upstream and a derived allowlist would
widen itself on a version bump.

Add charts.py: validates a chart directive against the rows it describes, resolving
column names to indices. The model names columns, never values. Five kinds; stacked
kinds take long format (x, series, y) so y stays one column and no dual-axis chart
is reachable. Sparse stacked results densify to 0 server-side (GROUP BY omits empty
groups, so an absent cell means zero) but a present NULL y is refused — a hole makes
the bar's total a lie.

Wire two specs (15 -> 17) and a per-turn chart_ref map onto ToolRegistry. Refs are
secrets.token_hex, minted only for results worth charting, and die with the turn.
render_chart returns the directive but never the rows.

New tests: sql_guard surface isolation (12), graph surface incl. byte-identical-DB
after every hostile query (29), charts rules (41), registry wiring (16).

* feat(serve): render charts in the chat transcript (frontend + eval)

ChartBlock.tsx implements the visual spec as one pure buildOption(); chartSpec.ts
is the parse + correlation boundary (never throws — a truncated payload renders as
no chart beside numbers that are still correct); echarts.ts is the only file that
reaches into the library, registering BarChart, LineChart, Grid, Tooltip, Legend
and CanvasRenderer (PNG export does not work under the SVG renderer).

Three deviations from the plan, each forced by a measurement:

- Code-split the chart. Tree-shaken ECharts costs +575 KB raw / +196 KB gzip,
  measured — about 2x the ~100 KB gz the plan cited, so the flat 250 KB budget
  failed. Charts are conditional UI, so ChartBlock loads on first use: the initial
  chunk grows 13.7 KB raw / 5 KB gzip and the 566 KB lands in its own chunk.
- parseChart searches the whole turn, not the activity group. render_chart and the
  stats call that minted its chart_ref are separate tool rounds, and a new round
  opens a new group — always on replay. The plan's per-group search returns NULL
  there, i.e. the chart would silently vanish on reload. Verified both paths by
  driving the real turns.ts.
- grid.outerBounds instead of containLabel, which is legacy in ECharts 6 and warns
  on every render. Measured equivalent: the longest label lands at the same x.

Verified without a browser: every kind rendered through ECharts SSR (legend only on
stacked, max-value label, <=12 x ticks at 30 rows), and the tooltip formatter proven
to touch no markup sink — an <img onerror> series name survives as literal text.

Eval gains graph-stats / identity / chart cases and a second kind of check: whether
a tool DESCRIPTION was obeyed, read off the emitted arguments (author-table join,
no json_each, no LIKE on emails, render_chart naming columns, a breakdown becoming
one stacked chart).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant