Skip to content

feat(chat): chart any aggregate — two stats surfaces + a generic render_chart - #40

Merged
cvetty merged 3 commits into
mainfrom
feat/chat-charts
Jul 31, 2026
Merged

feat(chat): chart any aggregate — two stats surfaces + a generic render_chart#40
cvetty merged 3 commits into
mainfrom
feat/chat-charts

Conversation

@cvetty

@cvetty cvetty commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

The chat assistant can now compute an aggregate over either database and draw it, without ever
retyping a data point.

Three tools' worth of change, in three reviewable commits:

Commit What
0073160 _SCHEMA_DOC: the author paragraph + a fifth rule. A defect already on main — shippable on its own.
fe0289e sql_guard.py, graph_stats_sql.py, charts.py, the registry wiring, the system prompt.
869cb89 echarts.ts, chartSpec.ts, ChartBlock.tsx, the MessageBubble insertion, the eval.

Implements plans/chat-charts-plan.md (rev 5) end to end; §14 of that file is the full
implementation record, including everything below.


What this adds

Two statistics surfaces, one authorizer. The four SQL security layers move out of stats_sql.py
into chat/sql_guard.py, parameterized by a SqlSurface — a label, a frozenset-literal table
allowlist, a deferred db_path, and a per-surface missing-DB message. graph_stats_sql.py is the
second surface, over CodeGraph's own .codegraph/codegraph.db.

That second database is the point: run_project_stats reads whatever the SQLModel engine is bound
to, so code structure — symbols per module, call fan-out, the shape of the codebase — was
unreachable no matter how clever the SQL. Sharing the implementation is itself the security argument:
there is one authorizer to audit, not two that drift.

render_chart, a capability rather than a parameter. A producer mints an opaque per-turn
chart_ref alongside its column names; render_chart takes that ref and names columns of it. The
model never transcribes a value into a chart — a 30-bucket series would otherwise mean 60 numbers
retyped, and one fabricated point is invisible in a rendered chart. A third producer joins by
returning {columns, rows} and calling _mint_chart_ref: one line.

Five kindsline, bar, bar_h, bar_stacked, bar_h_stacked — plus a stat tile. Stacked
kinds take long format (x, series, y), which is what GROUP BY month, kind already returns, so
y stays one column and no dual-axis chart is reachable. A stack adds a dimension, not a second
measure.

Failure modes designed out rather than mitigated

  • One y column makes a dual-axis chart unreachable; a list y is refused, telling the model to
    draw two charts (and that a breakdown is series).
  • A truncated or too-short result never gets a ref at all — the model never sees an affordance it
    would be wrong to use. A chart drawn from a capped result would be a confident lie, since the tool
    cannot know the true total.
  • A bad column returns the available names while the correct numbers stay on screen.
  • Refs are secrets.token_hex and die with the turn, so a stale one returns an expiry error and
    the harness's eliding of older tool results never matters.
  • nodes.docstring / nodes.signature hold source-derived text, but the aggregate-only shape check
    means the graph surface cannot dump them row by row.

The one judgment call worth reviewing carefully

A sparse stacked result and a NULL measure are handled oppositely, on purpose.

GROUP BY x, series omits empty groups — measured on this repo: 14 rows for a 4×5 grid, 6 cells
absent
. An absent cell means zero, so filling it recovers a fact; leaving it out would shift
every segment above it down and silently mislabel them. But a row present with a NULL y is
refused, because a hole makes the bar's total wrong and the total is what a stack is for. On an
unstacked kind that same NULL is neither — it leaves a gap, because there the aggregate itself was
null and zero would fabricate a value.

Densification happens server-side so the frontend never pivots, and the fill is never silent
a caption names the count.


Deviations from the plan

Four. Each surfaced by executing something the plan had only reasoned about.

1. The bundle budget failed by 2.4×, so the chart is code-split.

Tree-shaken ECharts costs +589,214 B raw / +200,582 B gzip, measured — roughly double the
"~100 KB gz" the plan cited, and the 250 KB raw budget was never consistent with a 100 KB gzipped
claim anyway. Rather than move the number, ChartBlock now loads lazily: charts are conditional UI, so
a transcript without one pays nothing.

Before After
Initial JS chunk 2,076,652 B (639,999 gz) 2,090,705 B (645,046 gz) — +13.7 KB raw, +5.0 KB gz
Chart chunk (lazy) 566,578 B (194,000 gz)

2. The plan's chart-to-rows correlation was broken, silently — every chart would have vanished on
reload.

render_chart returns the directive but not the rows (the producer already delivered them, so
echoing would double a 200-row payload), which means the frontend has to find them again by
chart_ref. The plan searched the chart card's own activity group. But the two calls are separate
tool rounds, and turnsFromMessages opens a new group for every assistant row that follows tool
activity — so on replay the producer sits in activityGroups[0] and the chart card in
activityGroups[1], and a per-group search returns null.

Verified by driving the real turns.ts with the persisted row shape: per-group NULL, per-turn
found.
Now flattens the turn, which is what the plan's own prose said ("the producing stats
activity of the same turn"). The same run confirmed the prose placement: narration lands in
segments[2], after the chart's group — picture above, reading below.

3. grid.containLabel is legacy in ECharts 6 and logs a deprecation warning on every render. No
clipping bug — ECharts 6 reserves the label band by default — just console noise. Switched to
grid.outerBounds; measured the longest category label landing at the same x under both.

4. markPoint needs MarkPointComponent, which the registration list deliberately excludes, so
the max-bar label would have been silently dropped. Uses a per-datum label instead — same visual
result, no added component.


Verification

All three CI gates green. ruff check, ruff format --check, and 931 tests (was 833).

New: test_chat_sql_guard.py 12 · test_chat_graph_stats.py 29 · test_chat_charts.py
41 · 16 charting cases in test_chat_tools.py.

The whole pipeline against the two live databases, through ToolRegistry.dispatch:

  • The canonical stacked case reproduces exactly — 14 rows, 4×5 grid, 6 zero-filled cells, and
    every per-x total equals the raw rows' sum. That last assertion is what separates "recovered a fact"
    from "invented one".
  • run_graph_statsbar_h over files.node_count.
  • Rule 5's all-time form → [['cvetty', 171]]. One row, so no ref is minted — on a solo repo the
    honest output is a sentence, not a bar chart of one person outworking themselves. That is the feature
    working, not failing; evaluate it against a multi-contributor repo.
  • Every failure mode degrades with the right layer: bad column / pie / series on bar / stacked
    without serieslayer:"chart"; a forged ref → layer:"ref"; a missing CodeGraph index →
    layer:"connection".

Security. Both databases are byte-identical (SHA-256) after every hostile queryDROP,
DELETE, UPDATE, INSERT, ATTACH, PRAGMA, WITH RECURSIVE, load_extension/readfile/
writefile, a denied table smuggled through a subquery, and a non-aggregate SELECT. The absence of
an exception is not the proof; the bytes are.

The renderer introduced one HTML sink — ECharts renders a tooltip.formatter string as HTML, and
our labels are repo content (a commit subject, a symbol name, an author login). The formatter is a
function returning a DOM element built with textContent, so no HTML is ever parsed. Proved by
driving it through a recording DOM shim whose innerHTML/insertAdjacentHTML setters log every
write: a series name of <img src=x onerror=alert(1)> comes back as literal text, with zero
markup sinks touched. Swatch colours resolve from the palette by index, so even the one styled
attribute is not data-derived.

_ALLOWED_TABLES is a frozenset literal on both surfaces, never derived from sqlite_master
CodeGraph's schema is upstream and a derived allowlist would widen itself on a version bump. A new
upstream table is inert until someone edits the literal. chat_message, chat_session, and
rationale_cache stay denied, asserted by name.

No browser or working chat provider in the authoring environment ([chat].provider is
anthropic; that key is revoked locally). Substitutes that caught more than a screenshot would have:

  • Every kind rendered through real ECharts SSRline, bar, bar_h, bar_stacked,
    bar_h_stacked, and a 30-bucket case. Legend on stacked only, max-value label present, x ticks
    ≤ 12 at 30 rows. ECharts throws on a malformed option and warns on an unregistered component, which
    is how deviations 3 and 4 were found.
  • Label clipping measured off the SVG, comparing three grid configs.
  • turns.ts driven with both the persisted row shape and SSE frames — reload behaviour without a
    reload.
  • The eval's new obedience checks exercised offline on synthetic tool-call arguments. The row
    tuple grew from 7 to 8 fields, so a live run would otherwise have crashed in _report.

Reviewer notes

One existing assertion changed inside the security refactor, and it is worth a look:
test_chat_stats_sql.py:323 patches _TIMEOUT_SEC, which moved to sql_guard with layer 4 — so
patching a re-export could not reach the code under test. It is a patch target following its
constant, not a behaviour change, but the plan's rule ("if a single test needs editing, the refactor is
wrong") does not distinguish those. The honest statement: every behavioural assertion passed
unedited across the extraction.
A _TIMEOUT_SEC re-export in stats_sql would have made the edit
unnecessary and silently ineffective, which is worse.

Two other test edits are the FOURFIVE REQUIRED RULES count, in the step-0 commit.

Also: _SCHEMA_DOC's commit table described author_email as "the reliable contributor
identity", which rule 5 now contradicts outright. Fixed in the same commit.

What still needs a human

Nothing here blocks review, but these acceptance criteria are unverified:

  1. The manual smoke (11 steps, plus the series-cap case). Needs a browser. The PNG's 2× scale and
    opaque background, and "render it and look at it" for collisions and overflow, are pixel judgments
    no SSR run substitutes for.
  2. The live-provider eval matrix. Everything measurable about the script is verified; nothing
    about model behaviour is — and that is where the real open question now lives: does the model
    reach for the right one of two stats tools, and does it ask for one stacked chart rather than
    two? Run with --all-configured from a shell with a working provider.
  3. Pane-resize reflow, and how the lazy-load fallback reads mid-stream — both touched by
    deviation 1.

Not in scope, deliberately

Pie/donut (the design reference routes part-to-whole to a stacked bar), grouped bars,
100 %-normalized stacking, get_coverage_stats (needs a layering decision of its own), SVG/PDF/CSV
export, and charting model-derived numbers — the one chart that could show a number no database
produced.

package.json gains exactly two dependencies (echarts 6.1.0, echarts-for-react 3.0.6); no
backend dependency change. npm audit is unchanged at 2 pre-existing dev-only findings (vite/esbuild)
— neither comes from echarts.

cvetty added 3 commits July 31, 2026 15:22
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.
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).
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).
@cvetty
cvetty merged commit 7e1bab1 into main Jul 31, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant