feat(chat): chart any aggregate — two stats surfaces + a generic render_chart - #40
Merged
Conversation
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
0073160_SCHEMA_DOC: theauthorparagraph + a fifth rule. A defect already onmain— shippable on its own.fe0289esql_guard.py,graph_stats_sql.py,charts.py, the registry wiring, the system prompt.869cb89echarts.ts,chartSpec.ts,ChartBlock.tsx, theMessageBubbleinsertion, the eval.Implements
plans/chat-charts-plan.md(rev 5) end to end; §14 of that file is the fullimplementation record, including everything below.
What this adds
Two statistics surfaces, one authorizer. The four SQL security layers move out of
stats_sql.pyinto
chat/sql_guard.py, parameterized by aSqlSurface— a label, a frozenset-literal tableallowlist, a deferred
db_path, and a per-surface missing-DB message.graph_stats_sql.pyis thesecond surface, over CodeGraph's own
.codegraph/codegraph.db.That second database is the point:
run_project_statsreads whatever the SQLModel engine is boundto, 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-turnchart_refalongside its column names;render_charttakes that ref and names columns of it. Themodel 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 kinds —
line,bar,bar_h,bar_stacked,bar_h_stacked— plus a stat tile. Stackedkinds take long format
(x, series, y), which is whatGROUP BY month, kindalready returns, soystays one column and no dual-axis chart is reachable. A stack adds a dimension, not a secondmeasure.
Failure modes designed out rather than mitigated
ycolumn makes a dual-axis chart unreachable; a listyis refused, telling the model todraw two charts (and that a breakdown is
series).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.
secrets.token_hexand die with the turn, so a stale one returns an expiry error andthe harness's eliding of older tool results never matters.
nodes.docstring/nodes.signaturehold source-derived text, but the aggregate-only shape checkmeans 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, seriesomits empty groups — measured on this repo: 14 rows for a 4×5 grid, 6 cellsabsent. 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
yisrefused, 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,
ChartBlocknow loads lazily: charts are conditional UI, soa transcript without one pays nothing.
2. The plan's chart-to-rows correlation was broken, silently — every chart would have vanished on
reload.
render_chartreturns the directive but not the rows (the producer already delivered them, soechoing 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 separatetool rounds, and
turnsFromMessagesopens a new group for every assistant row that follows toolactivity — so on replay the producer sits in
activityGroups[0]and the chart card inactivityGroups[1], and a per-group search returnsnull.Verified by driving the real
turns.tswith the persisted row shape: per-groupNULL, per-turnfound. Now flattens the turn, which is what the plan's own prose said ("the producing statsactivity 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.containLabelis legacy in ECharts 6 and logs a deprecation warning on every render. Noclipping 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.
markPointneedsMarkPointComponent, which the registration list deliberately excludes, sothe max-bar label would have been silently dropped. Uses a per-datum
labelinstead — same visualresult, no added component.
Verification
All three CI gates green.
ruff check,ruff format --check, and 931 tests (was 833).New:
test_chat_sql_guard.py12 ·test_chat_graph_stats.py29 ·test_chat_charts.py41 · 16 charting cases in
test_chat_tools.py.The whole pipeline against the two live databases, through
ToolRegistry.dispatch:every per-x total equals the raw rows' sum. That last assertion is what separates "recovered a fact"
from "invented one".
run_graph_stats→bar_hoverfiles.node_count.[['cvetty', 171]]. One row, so no ref is minted — on a solo repo thehonest 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.
pie/seriesonbar/ stackedwithout
series→layer:"chart"; a forged ref →layer:"ref"; a missing CodeGraph index →layer:"connection".Security. Both databases are byte-identical (SHA-256) after every hostile query —
DROP,DELETE,UPDATE,INSERT,ATTACH,PRAGMA,WITH RECURSIVE,load_extension/readfile/writefile, a denied table smuggled through a subquery, and a non-aggregateSELECT. The absence ofan exception is not the proof; the bytes are.
The renderer introduced one HTML sink — ECharts renders a
tooltip.formatterstring as HTML, andour 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 bydriving it through a recording DOM shim whose
innerHTML/insertAdjacentHTMLsetters log everywrite: a series name of
<img src=x onerror=alert(1)>comes back as literal text, with zeromarkup sinks touched. Swatch colours resolve from the palette by index, so even the one styled
attribute is not data-derived.
_ALLOWED_TABLESis a frozenset literal on both surfaces, never derived fromsqlite_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, andrationale_cachestay denied, asserted by name.No browser or working chat provider in the authoring environment (
[chat].providerisanthropic; that key is revoked locally). Substitutes that caught more than a screenshot would have:line,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.
turns.tsdriven with both the persisted row shape and SSE frames — reload behaviour without areload.
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:323patches_TIMEOUT_SEC, which moved tosql_guardwith layer 4 — sopatching 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_SECre-export instats_sqlwould have made the editunnecessary and silently ineffective, which is worse.
Two other test edits are the
FOUR→FIVE REQUIRED RULEScount, in the step-0 commit.Also:
_SCHEMA_DOC'scommittable describedauthor_emailas "the reliable contributoridentity", 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:
opaque background, and "render it and look at it" for collisions and overflow, are pixel judgments
no SSR run substitutes for.
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-configuredfrom a shell with a working provider.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/CSVexport, and charting model-derived numbers — the one chart that could show a number no database
produced.
package.jsongains exactly two dependencies (echarts6.1.0,echarts-for-react3.0.6); nobackend dependency change.
npm auditis unchanged at 2 pre-existing dev-only findings (vite/esbuild)— neither comes from echarts.