From 00731606ae1276d13a0b9c66076c4bfd5bb45ef5 Mon Sep 17 00:00:00 2001 From: cvetty Date: Fri, 31 Jul 2026 15:22:13 +0300 Subject: [PATCH 1/3] =?UTF-8?q?fix(chat):=20=5FSCHEMA=5FDOC=20=E2=80=94=20?= =?UTF-8?q?author=20table=20is=20populated,=20add=20identity=20rule=205?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/whygraph/chat/stats_sql.py | 65 ++++++++++++++++++++++++++++++---- tests/test_chat_stats_sql.py | 37 +++++++++++++++++-- tests/test_chat_tools.py | 2 +- 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/src/whygraph/chat/stats_sql.py b/src/whygraph/chat/stats_sql.py index 5479be7..96c9e98 100644 --- a/src/whygraph/chat/stats_sql.py +++ b/src/whygraph/chat/stats_sql.py @@ -369,7 +369,7 @@ def _guard() -> int: get_commit / get_pr / get_area_history / find_changes handle those, and they follow rename chains and git blame, which raw SQL here does NOT. -=== FOUR REQUIRED RULES (each of these silently corrupts results) === +=== FIVE REQUIRED RULES (each of these silently corrupts results) === 1. ALWAYS filter `on_default_branch = 1` on the commit table. Rows with 0 are PR-origin commits recovered from squash merges; counting them double-counts @@ -390,6 +390,44 @@ def _guard() -> int: 4. A merged PR has state = 'closed', NOT 'merged'. Count merges with `merged_at IS NOT NULL`. `state` is only 'open' or 'closed'. +5. NEVER GROUP DEVELOPERS BY `commit.author_name` OR `author_email`. Those + are raw git identities and one human routinely has several — a work + email, a GitHub noreply address, a second machine. Grouping by either + reports one person as two or three, and the number looks authoritative. + The `author` table has already resolved this, one row per human. + + For an ALL-TIME ranking, do not join at all — the counts are already + columns. This tool still requires an aggregate, so wrap them in SUM() and + group by the author's id (one row per group, so SUM is an identity): + + SELECT COALESCE(primary_login, primary_name, primary_email) AS developer, + SUM(commit_count) AS commits, SUM(pr_count) AS prs + FROM author GROUP BY id ORDER BY commits DESC + + (commit_count is already default-branch-only — do NOT also apply rule 1.) + Selecting commit_count bare, without SUM and GROUP BY, is REFUSED as a + non-aggregate query. + + For anything TIME-SLICED ("lately", per month, since a date), join + commit to author on the emails array with instr(): + + SELECT COALESCE(a.primary_login, a.primary_name, a.primary_email) + AS developer, + COUNT(*) AS commits + FROM "commit" c + JOIN author a ON instr(a.emails, '"' || c.author_email || '"') > 0 + WHERE c.on_default_branch = 1 + AND c.authored_at >= date('now', '-90 days') + GROUP BY a.id ORDER BY commits DESC + + Use instr() exactly as written. Do NOT use json_each (not permitted here) + and do NOT use LIKE for this match: '_' is a LIKE wildcard and appears in + ordinary addresses, so a LIKE join can attribute one person's commits to + a different person. The surrounding double quotes matter — they stop a + short address matching a longer one. + + Never present a commit count as a measure of productivity. + === TABLES === commit — one row per scanned commit (first-parent walk of the default branch) @@ -431,9 +469,18 @@ def _guard() -> int: pr_issue_link — pr_number, issue_number, link_kind ('closes') -author — resolved contributor identities. POPULATED BY A SEPARATE SCAN STEP - THAT OFTEN HAS NOT RUN (0 rows here). Prefer commit.author_name / - commit.author_email for contributor stats; treat this table as optional. +author — resolved contributor identities, ONE ROW PER HUMAN. Built by the + scan's author phase from evidence only (git mailmap, GitHub's own + login/name/email triples, noreply parsing, byte-equal emails) — never from + a display name, so two people who share a name are never merged. + id INTEGER PK · primary_login TEXT NULL (GitHub login; NULL if they never + appeared in a PR/issue) · primary_name, primary_email TEXT + emails, logins, names TEXT -- JSON ARRAYS of every known value, sorted + commit_count INTEGER -- default-branch commits only (rule 1 already applied) + pr_count, issue_count INTEGER · first_seen, last_seen TEXT + Rebuilt from scratch each scan, so it is current or absent, never stale. + If it is EMPTY the author phase has not run — fall back to + commit.author_email and say the identities are unresolved. If a table returns nothing, that source has not been scanned — say so rather than reporting zero as a finding.\ @@ -446,9 +493,15 @@ def _guard() -> int: field whose meaning or value domain is non-obvious carries a note, and every value domain quoted was **measured on this repository** rather than assumed. -The four rules exist because each one is a silent-corruption trap that was hit +The five rules exist because each one is a silent-corruption trap that was hit in practice while probing the schema. They are guarded by a test — they must not -be paraphrased or trimmed to save tokens. +be paraphrased or trimmed to save tokens. Rule 5's ``instr`` join is quoted +verbatim on purpose: the two obvious alternatives are both wrong here. +``json_each`` is denied by the authorizer (a table-valued function registers as a +table read, and it is not in :data:`_ALLOWED_TABLES`), and a ``LIKE`` match +against the JSON array treats ``_`` as a single-character wildcard — so one +person's commits can be attributed to a different person whose address differs +only at that position. That false merge is undetectable from the output. """ diff --git a/tests/test_chat_stats_sql.py b/tests/test_chat_stats_sql.py index 20e5a47..b8d2550 100644 --- a/tests/test_chat_stats_sql.py +++ b/tests/test_chat_stats_sql.py @@ -395,7 +395,7 @@ def test_a_missing_database_is_an_error_result(tmp_path: Path) -> None: # --------------------------------------------------------------------------- -def test_schema_doc_carries_all_four_silent_corruption_rules() -> None: +def test_schema_doc_carries_all_five_silent_corruption_rules() -> None: """The rules cannot be paraphrased or trimmed to save tokens. Each guards a trap that produces a *plausible* wrong number, which no other @@ -413,6 +413,9 @@ def test_schema_doc_carries_all_four_silent_corruption_rules() -> None: # Rule 4 — a merged PR is 'closed'. assert "merged_at IS NOT NULL" in doc assert "NOT 'merged'" in doc + # Rule 5 — raw git identities split one human across several rows. + assert "NEVER GROUP DEVELOPERS BY" in doc + assert "one row per human" in doc # The aggregate-only contract and the redirection away from record lookups. assert "Aggregates only" in doc for tool in ("get_commit", "get_area_history", "find_changes"): @@ -432,7 +435,35 @@ def test_schema_doc_documents_the_non_obvious_value_domains() -> None: assert "NOT words" in doc # refactor_score is a heuristic, not a quality score. assert "Not a quality measure" in doc - # The author table is usually empty. - assert "OFTEN HAS NOT RUN" in doc + # The author table resolves identity, and its list columns are JSON. + assert "ONE ROW PER HUMAN" in doc + assert "JSON ARRAYS" in doc # llm_description's provenance, which is why it beats subject/body. assert "DIFF ALONE" in doc + + +def test_schema_doc_routes_developer_grouping_through_the_author_table() -> None: + """Rule 5, and the two join forms it must never teach. + + Both alternatives look right and are not: ``json_each`` is refused by the + authorizer, and a ``LIKE`` match against ``author.emails`` treats ``_`` as a + wildcard, so it can attribute one person's commits to another. That is the + one failure mode invisible in the output, so the wording is a contract — the + eval greps the emitted SQL for this exact form. + """ + doc = stats_sql._SCHEMA_DOC + assert "author" in doc + # The all-time form must aggregate: reading `commit_count` bare is refused + # by `_check_shape`, so the rule ships the SUM/GROUP BY wrapper. + assert "SUM(commit_count) AS commits" in doc + assert "FROM author GROUP BY id" in doc + # The time-sliced form, verbatim. + assert "instr(a.emails, '\"' || c.author_email || '\"') > 0" in doc + # Neither wrong form may appear as advice. + assert "lower(author_name)" not in doc + assert "json_each" in doc and "Do NOT use json_each" in doc + assert "do NOT use LIKE for this match" in doc + # `author.commit_count` already excludes on_default_branch = 0. + assert "do NOT also apply rule 1" in doc + # Rule 1 is not a caveat on this path, and the count is not a scoreboard. + assert "Never present a commit count as a measure of productivity" in doc diff --git a/tests/test_chat_tools.py b/tests/test_chat_tools.py index 6b6367f..988d253 100644 --- a/tests/test_chat_tools.py +++ b/tests/test_chat_tools.py @@ -207,7 +207,7 @@ def test_the_stats_spec_ships_the_annotated_schema() -> None: spec = next(s for s in TOOL_SPECS if s.name == "run_project_stats") assert spec.description == _SCHEMA_DOC - assert "FOUR REQUIRED RULES" in spec.description + assert "FIVE REQUIRED RULES" in spec.description assert "=== TABLES ===" in spec.description From fe0289e2ef44e6ece7676dd3b0c577dec9e7ebea Mon Sep 17 00:00:00 2001 From: cvetty Date: Fri, 31 Jul 2026 15:39:02 +0300 Subject: [PATCH 2/3] feat(chat): two stats surfaces + a generic render_chart tool (backend) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/whygraph/chat/charts.py | 413 +++++++++++++++++++ src/whygraph/chat/graph_stats_sql.py | 211 ++++++++++ src/whygraph/chat/prompts/system.md | 23 ++ src/whygraph/chat/sql_guard.py | 393 ++++++++++++++++++ src/whygraph/chat/stats_sql.py | 357 +++------------- src/whygraph/chat/tools.py | 219 +++++++++- tests/test_chat_charts.py | 593 +++++++++++++++++++++++++++ tests/test_chat_graph_stats.py | 391 ++++++++++++++++++ tests/test_chat_harness.py | 4 +- tests/test_chat_sql_guard.py | 201 +++++++++ tests/test_chat_stats_sql.py | 9 +- tests/test_chat_tools.py | 264 +++++++++++- 12 files changed, 2755 insertions(+), 323 deletions(-) create mode 100644 src/whygraph/chat/charts.py create mode 100644 src/whygraph/chat/graph_stats_sql.py create mode 100644 src/whygraph/chat/sql_guard.py create mode 100644 tests/test_chat_charts.py create mode 100644 tests/test_chat_graph_stats.py create mode 100644 tests/test_chat_sql_guard.py diff --git a/src/whygraph/chat/charts.py b/src/whygraph/chat/charts.py new file mode 100644 index 0000000..72c459c --- /dev/null +++ b/src/whygraph/chat/charts.py @@ -0,0 +1,413 @@ +"""Validate a chart directive **against the result it describes**. + +The model never transcribes a value into a chart. It names *columns* of an +aggregate it has already computed, and this module resolves those names to +indices against the producer's own ``columns`` / ``rows``. A 30-bucket series +would otherwise mean 60 numbers retyped, and one fabricated point is invisible +in a rendered chart. + +That makes column names the failure mode (the dominant one reported for +spec-generating chart agents), so **every refusal names both the offending value +and what was available** — the message is the defense, not decoration, because a +tool error is something the model can read and correct inside the same turn. + +This module knows nothing about SQL and nothing about ECharts. Any producer that +returns ``{columns, rows}`` can be charted, and any renderer can consume the +result — which is what makes a third producer a one-line addition. + +Notes +----- +**Stacked kinds take long format, not a list ``y``.** ``bar_stacked`` adds a third +column name, ``series``, so rows are ``(x, series, y)`` — exactly what +``GROUP BY month, kind`` already returns. ``y`` stays **one column, never a +list**, so the one-measure rule that makes a dual-axis chart unreachable needs no +exception: a stack adds a *dimension*, not a second *measure*. + +**The two null-ish cases are handled oppositely, and that is the crux.** A cell +*absent* from a stacked result is filled with **0**: ``GROUP BY`` omits empty +groups, so an absent ``(x, series)`` pair means that category genuinely had none, +and filling it *recovers* a fact. A row *present* with ``y = None`` is +**refused**: a hole makes the bar's total wrong, and on a stack the total is the +whole point. On an unstacked kind a ``None`` ``y`` is neither — it leaves a gap, +because there the aggregate itself was null and zero would fabricate a value. +""" + +from __future__ import annotations + +CHART_KINDS = frozenset({"line", "bar", "bar_h", "bar_stacked", "bar_h_stacked"}) +"""The closed set of drawable kinds. The registration list *is* the feature list.""" + +STACKED_KINDS = frozenset({"bar_stacked", "bar_h_stacked"}) +"""The kinds that require ``series`` and forbid a ``None`` ``y``. See rules 8-10.""" + +MIN_ROWS_BY_KIND = { + "bar": 2, + "bar_h": 2, + "line": 3, + "bar_stacked": 2, + "bar_h_stacked": 2, +} +"""Minimum plottable rows, per kind. + +A two-bar comparison is legitimate — "these two developers" is a real question — +but a two-point line is a degenerate trend that should be a sentence. The +cataloged anti-pattern is the ONE-bar chart, not the two-bar one. + +For stacked kinds this counts **distinct ``x`` values**, not rows: long format +means one ``x`` spans several rows, so ``len(rows)`` would let a one-bar chart +through whenever it happened to have two series. +""" + +MIN_CHART_ROWS = min(MIN_ROWS_BY_KIND.values()) +"""The ref-minting floor (2). + +A producer withholds ``chart_ref`` below this, so the model never sees an +affordance it would be wrong to use. Kind-specific minimums are enforced here. +""" + +MAX_SERIES = 6 +"""Hard cap on distinct ``series`` values. + +Six is the largest set that stays readable stacked *and* is validated as a +palette against the chat surface. Exceeding it **refuses**; the server never +folds an "other" bucket, because that would put a number on screen that no +emitted query produced — the one property this whole design exists to protect. +""" + +MAX_X_TICKS = 12 +"""The renderer's x-axis label budget. + +Recorded here rather than only in the component so the chart contract lives in +one file: 200 category labels collide, and every value stays readable in the +table view regardless. Consumed by the frontend, not by :func:`validate_chart`. +""" + +_MAX_TITLE = 80 +_MAX_Y_LABEL = 40 + + +class ChartNotAllowed(ValueError): + """A directive that cannot be honoured, with a model-readable reason. + + The message is the contract: it names the offending value and the columns + that *were* available, so the model can correct the call rather than losing + the turn. + """ + + +def _available(columns: list[str]) -> str: + """Render the producer's column names for a refusal message.""" + return ", ".join(repr(column) for column in columns) or "(none)" + + +def _resolve(name: object, role: str, columns: list[str]) -> int: + """Resolve one column name to its index, or refuse naming what was available.""" + if not isinstance(name, str) or not name: + raise ChartNotAllowed( + f"{role} must be a column name (a string); got {name!r}. " + f"Available columns: {_available(columns)}." + ) + if name not in columns: + raise ChartNotAllowed( + f"unknown {role} column {name!r} — this result has columns " + f"{_available(columns)}. Name a column of the result you charted, " + "not a value from it." + ) + return columns.index(name) + + +def validate_chart( + *, + kind: object, + title: object, + x: object, + y: object, + series: object = None, + y_label: object = None, + columns: list[str], + rows: list[list], +) -> dict: + """Normalize a chart directive, resolving column names to indices. + + Parameters + ---------- + kind : str + One of :data:`CHART_KINDS`. + title : str + Non-empty, at most 80 characters. Required because an unstacked chart has + no legend, making the title its only identity channel. + x : str + Column name for the category / time axis. + y : str + Column name for the measure. **One column, never a list.** + series : str, optional + Required for :data:`STACKED_KINDS` and refused for every other kind: the + column whose values become the stack segments. + y_label : str, optional + Axis caption, at most 40 characters. + columns : list of str + The producer's column names. + rows : list of list + The producer's rows, positional against ``columns``. + + Returns + ------- + dict + ``{"kind", "title", "x", "y", "x_index", "y_index", "null_rows"}``, plus + ``"y_label"`` when given, plus for stacked kinds ``{"series", + "series_index", "series_values", "x_values", "cells", "filled_cells"}``. + + ``series_values`` is the stack and colour-slot order — descending by + series total, ties lexicographic — so a segment keeps its colour across + re-renders and across the live-to-persisted swap. ``cells`` is the + **densified** ``x`` × ``series`` grid with absent combinations filled + with 0, and ``filled_cells`` counts them for the caption, so the fill is + never silent. + + Raises + ------ + ChartNotAllowed + For any directive that cannot be honoured. The rows are unaffected and + still on screen, so this degrades to correct numbers rather than a failed + turn. + """ + # Rule 1 — a closed set of kinds. + if kind not in CHART_KINDS: + raise ChartNotAllowed( + f"unknown chart kind {kind!r} — use one of " + f"{', '.join(sorted(CHART_KINDS))}." + ) + stacked = kind in STACKED_KINDS + + # Rule 2 — the title is the only identity channel on an unstacked chart. + if not isinstance(title, str) or not title.strip(): + raise ChartNotAllowed("title is required and must be a non-empty string.") + title = title.strip() + if len(title) > _MAX_TITLE: + raise ChartNotAllowed( + f"title is {len(title)} characters; keep it to {_MAX_TITLE} or fewer." + ) + + # Rule 7 — the optional axis caption. + if y_label is not None: + if not isinstance(y_label, str): + raise ChartNotAllowed(f"y_label must be a string; got {y_label!r}.") + y_label = y_label.strip() + if len(y_label) > _MAX_Y_LABEL: + raise ChartNotAllowed( + f"y_label is {len(y_label)} characters; keep it to " + f"{_MAX_Y_LABEL} or fewer." + ) + + # Rule 4a — one measure, structurally. Checked before any value is read, so + # a list `y` cannot reach the numeric scan below. + if isinstance(y, (list, tuple)): + raise ChartNotAllowed( + "y must be ONE column, not a list. Two measures means two charts — " + "call render_chart twice. If you meant to break one measure down by " + "a category, that is `series` on a stacked kind, not a second y." + ) + + # Rule 3 — both axes are grounded in the producer's own output. + x_index = _resolve(x, "x", columns) + y_index = _resolve(y, "y", columns) + + # Rule 4b — a chart of a column against itself is not a chart. + if x == y: + raise ChartNotAllowed( + f"x and y are both {x!r}. Plotting a column against itself is not a " + f"chart; pick two different columns from {_available(columns)}." + ) + + # Rule 8 — `series` is required by exactly the stacked kinds. + series_index: int | None = None + if stacked: + if series is None: + unstacked = "bar" if kind == "bar_stacked" else "bar_h" + raise ChartNotAllowed( + f"{kind} requires `series` — the column whose values become the " + "stack segments, which means your SQL must GROUP BY both that " + f"column and {x!r}. With no breakdown dimension this is just a " + f"{unstacked}; use that kind instead. Available columns: " + f"{_available(columns)}." + ) + series_index = _resolve(series, "series", columns) + if series in (x, y): + raise ChartNotAllowed( + f"series {series!r} is already used as " + f"{'x' if series == x else 'y'}. The stack segments must be a " + "third, different column." + ) + elif series is not None: + raise ChartNotAllowed( + f"`series` is only valid on a stacked kind " + f"({', '.join(sorted(STACKED_KINDS))}), not on {kind!r}. Drawing " + f"{kind!r} while ignoring your breakdown would answer a different " + "question than you asked — pick the stacked kind, or drop `series`." + ) + + # Rule 5 — the measure must be numeric. `bool` is excluded explicitly + # because `isinstance(True, int)` is True in Python. + null_rows = 0 + for row in rows: + value = row[y_index] + if value is None: + null_rows += 1 + continue + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise ChartNotAllowed( + f"column {y!r} is not numeric — found {value!r}. A chart needs a " + "measure; pick the counted or summed column, or swap x and y if " + "the labels are on the wrong axis." + ) + + # Rule 10 — a present-but-NULL y on a stacked kind is refused, not zeroed. + # This is the opposite of the absent-cell case below, deliberately: a hole + # inside a bar makes its total a lie, and the total is what a stack is for. + if stacked and null_rows: + raise ChartNotAllowed( + f"{null_rows} row(s) have no value for {y!r}, and a stacked chart " + "cannot show a hole — every segment feeds a total. Wrap the measure " + "in COALESCE(..., 0) if zero is the honest value, or filter those " + "rows out. (An unstacked chart would draw them as gaps instead.)" + ) + + result: dict = { + "kind": kind, + "title": title, + "x": x, + "y": y, + "x_index": x_index, + "y_index": y_index, + "null_rows": null_rows, + } + if y_label: + result["y_label"] = y_label + + if stacked: + # `series_index` is set: rule 8 refused above if `series` was missing. + stack = _build_stack( + rows=rows, + x_index=x_index, + y_index=y_index, + series_index=series_index, + series_name=series, + ) + # Rule 9 — cardinality. Refuse; never fold a bucket the SQL did not emit. + if len(stack["series_values"]) > MAX_SERIES: + raise ChartNotAllowed( + f"{len(stack['series_values'])} distinct {series!r} values " + f"exceeds the {MAX_SERIES}-series limit — more segments than " + "that is an unreadable smear. Fold the tail into an 'other' " + "bucket in your SQL with a CASE, or filter to the top " + f"{MAX_SERIES}. Do that in the query so every plotted number " + "still comes from it." + ) + result.update(stack) + plottable = len(stack["x_values"]) + else: + plottable = len(rows) - null_rows + + # Rule 6 — enough to be worth drawing. Counts distinct x on a stacked kind. + minimum = MIN_ROWS_BY_KIND[kind] + if plottable < minimum: + unit = f"distinct {x!r} value(s)" if stacked else "plottable row(s)" + advice = ( + "A two-point line is a degenerate trend — use `bar` to compare 2 values." + if kind == "line" + else "A single value is not a chart; just say the number." + ) + raise ChartNotAllowed( + f"{kind} needs at least {minimum} {unit}; this result has " + f"{plottable}. {advice}" + ) + + return result + + +def _build_stack( + *, + rows: list[list], + x_index: int, + y_index: int, + series_index: int, + series_name: object, +) -> dict: + """Densify a long-format result into an ``x`` × ``series`` grid. + + ``GROUP BY x, series`` omits empty groups, so a real result is sparse — on + this repository the canonical stacked query returns 14 rows for a 4x5 grid. + Leaving those cells out would shift later segments down and silently mislabel + them, so the grid is filled here, **server-side**: one pivot implementation + means the chart and the table view cannot disagree. + + Returns + ------- + dict + ``{"series", "series_index", "series_values", "x_values", "cells", + "filled_cells"}``. ``x_values`` preserves the result's own row order, so + the query's ``ORDER BY`` is the axis order. + + Raises + ------ + ChartNotAllowed + If one ``(x, series)`` pair appears twice — which means the query did not + group by both columns. Summing them silently, or taking the last, would + be a decision the emitted SQL never made. + """ + x_values: list[str] = [] + series_totals: dict[str, float] = {} + cells: dict[str, dict[str, float]] = {} + + for row in rows: + x_value = str(row[x_index]) + series_value = str(row[series_index]) + value = row[y_index] + if x_value not in cells: + x_values.append(x_value) + cells[x_value] = {} + if series_value in cells[x_value]: + raise ChartNotAllowed( + f"({x_value!r}, {series_value!r}) appears more than once, so the " + "result is not one row per (x, series) pair. GROUP BY both " + f"columns — including {series_name!r} — in your SQL." + ) + cells[x_value][series_value] = value + series_totals[series_value] = series_totals.get(series_value, 0) + value + + # Descending by total, ties broken lexicographically. The tie-break is what + # makes colour assignment reproducible: first-appearance order would let a + # segment change colour between two renders of the same data. + series_values = sorted(series_totals, key=lambda name: (-series_totals[name], name)) + + filled_cells = 0 + for x_value in x_values: + row_cells = cells[x_value] + for series_value in series_values: + if series_value not in row_cells: + # Absent from a GROUP BY result *means* zero, so filling it + # recovers a fact rather than inventing one. Counted, and + # captioned by the renderer, so the fill is never invisible. + row_cells[series_value] = 0 + filled_cells += 1 + + return { + "series": series_name, + "series_index": series_index, + "series_values": series_values, + "x_values": x_values, + "cells": cells, + "filled_cells": filled_cells, + } + + +__all__ = [ + "CHART_KINDS", + "MAX_SERIES", + "MAX_X_TICKS", + "MIN_CHART_ROWS", + "MIN_ROWS_BY_KIND", + "STACKED_KINDS", + "ChartNotAllowed", + "validate_chart", +] diff --git a/src/whygraph/chat/graph_stats_sql.py b/src/whygraph/chat/graph_stats_sql.py new file mode 100644 index 0000000..af9995d --- /dev/null +++ b/src/whygraph/chat/graph_stats_sql.py @@ -0,0 +1,211 @@ +"""The CodeGraph statistics surface — a table allowlist and a schema doc. + +The second aggregate surface, and the reason :mod:`whygraph.chat.sql_guard` exists. +``run_project_stats`` reads whatever the SQLModel engine is bound to — the +WhyGraph database — so *code structure* was unreachable from it no matter how +clever the SQL: CodeGraph writes a **separate file**, ``.codegraph/codegraph.db``. +Questions like "which modules hold the most functions", "which files are largest", +"what is the distribution of symbol kinds" had no tool at all. + +Both surfaces run through the identical fence — same authorizer, same action +allowlist, same aggregate-only shape check, same read-only connection, same row +cap and deadline. Sharing the implementation *is* the security argument: there is +one authorizer to audit, not two that drift. + +What is genuinely different is this module's two constants. The database belongs +to `CodeGraph `_ upstream and can gain +tables on a version bump, so :data:`_ALLOWED_TABLES` is a frozenset **literal** +and the authorizer denies every table it does not recognize — a new upstream table +is inert until someone edits that literal. + +Chat-only — never registered with MCP. +""" + +from __future__ import annotations + +import logging +import sqlite3 +from pathlib import Path + +from whygraph.core import get_config +from whygraph.mcp.targets import repo_root +from whygraph.services.codegraph import CODEGRAPH_DB_RELPATH + +from . import sql_guard +from .sql_guard import SqlNotAllowed, SqlSurface, run_aggregate_query + +_log = logging.getLogger(__name__) + +_MAX_ROWS = sql_guard._MAX_ROWS +"""Re-export of the shared row cap — see :mod:`whygraph.chat.stats_sql`.""" + +_ALLOWED_TABLES = frozenset({"nodes", "edges", "files"}) +"""The read allowlist — **the** security boundary of this module. + +A literal, **never** derived from ``sqlite_master``. CodeGraph's schema is +upstream (``colbymchenry/codegraph``) and can gain tables on a version bump; a +derived allowlist would widen silently on the next `codegraph` release, which is +the whole failure mode this constant exists to prevent. + +Denied by default, because the authorizer refuses what it does not recognize: +``nodes_fts`` and its four shadow tables, ``name_segment_vocab``, +``project_metadata``, ``schema_versions``, ``unresolved_refs``. +""" + +_NO_CODEGRAPH = "CodeGraph index unavailable — run `whygraph scan`" +"""Refusal text when the index is absent. + +Deliberately the same sentence the other CodeGraph-backed tools use +(``tools.py``): the WhyGraph and file tools still work without an index, so a +missing index degrades the conversation rather than ending it. +""" + + +def _codegraph_db_path() -> Path: + """Resolve ``/.codegraph/codegraph.db``. + + Mirrors ``tools.py``'s ``_open_graph`` so the two cannot disagree about which + index the assistant is reading: a ``codegraph_db`` entry in ``whygraph.toml`` + wins, otherwise the project-relative default. Resolved **per call** — config + is memoized per process and root discovery walks up from ``cwd``. + """ + configured = get_config().codegraph_db + return configured if configured is not None else repo_root() / CODEGRAPH_DB_RELPATH + + +_SURFACE = SqlSurface( + label="CodeGraph", + allowed_tables=_ALLOWED_TABLES, + db_path=_codegraph_db_path, + # No `{db_path}` here: a missing index is a "run the scan" problem, not a + # path problem, and the path is an implementation detail of the container + # layout the shim mounts. + missing_db_message=_NO_CODEGRAPH, +) +"""This module's binding of the shared guard.""" + + +def _connect(db_path: Path, denials: list[str] | None = None) -> sqlite3.Connection: + """Open the CodeGraph DB read-only with the authorizer already installed. + + A thin binding of :meth:`sql_guard.SqlSurface.connect` to this surface, kept + as a module-level name so the layer-2 tests can drive the authorizer with the + shape check out of the way — mirroring ``stats_sql._connect``. + """ + return _SURFACE.connect(db_path, denials) + + +def run_graph_query(sql: str, *, db_path: Path | None = None) -> dict: + """Execute one aggregate query against CodeGraph's index and return its rows. + + Parameters + ---------- + sql : str + A single ``SELECT`` / ``WITH`` statement that aggregates. + db_path : Path, optional + The database to read. Defaults to :func:`_codegraph_db_path`. + + Returns + ------- + dict + See :func:`sql_guard.run_aggregate_query` — ``{"sql", "columns", "rows", + "row_count", "truncated"}`` on success, ``{"error", "layer"}`` when a + layer refused. A missing index is ``{"error": _NO_CODEGRAPH, "layer": + "connection"}``: it degrades, it never raises. + """ + return run_aggregate_query(sql, _SURFACE, db_path=db_path) + + +_GRAPH_SCHEMA_DOC = """\ +Run a read-only aggregate SQL query over the CODE GRAPH — the structure of the +codebase as it stands right now. How many functions per module, which files hold +the most symbols, the distribution of symbol kinds, call fan-in and fan-out. +Aggregates only: the query MUST use COUNT/SUM/AVG/MIN/MAX or GROUP BY, and +returns at most 200 rows. + +This is a DIFFERENT DATABASE from run_project_stats. It has no history in it — +nothing here changes over time. Anything about WHEN something happened, who +changed it, or how it evolved belongs to run_project_stats. + +For one symbol's callers, callees, or source, use search_symbols / get_symbol / +get_area_outline — they return readable, related detail that a bare count cannot. + +=== FIVE REQUIRED RULES (each of these silently corrupts results) === + +1. `nodes` is NOT one row per definition. It also holds `import` and `variable` + rows, and imports OUTNUMBER functions. For "how much code is here", filter to + real definitions: + kind IN ('function','method','class','interface','route','constant', + 'type_alias','component') + Without that filter every count is dominated by import statements. + +2. `kind = 'file'` rows are files-as-nodes, not code. Counting them alongside + functions double-counts the file. Use the `files` table for per-file facts. + +3. `file_path` is repo-relative ('src/whygraph/chat/tools.py'). To group by + directory, take a fixed number of leading segments — e.g. + substr(file_path, 1, instr(file_path, '/') - 1) + for the top level — and CHECK THE RESULT: a path with no '/' at the expected + offset yields a truncated fragment that looks like a real module. Drop or + floor tiny buckets rather than plotting a fragment. Remember '_' is a LIKE + wildcard and appears in almost every path here, so escape it or use instr(). + +4. `edges.source` and `edges.target` are `nodes.id`, not names. Join to `nodes` + for anything readable. kind='contains' is structural nesting (a class + containing its methods); kind='calls' is the call graph. Fan-out is counting + edges by source, fan-in by target. + +5. TESTS ARE CODE TOO, and here they are the largest module. Grouping symbols by + directory puts `tests` on top. That is truthful and usually not what the + asker meant, so when you answer "how big is the codebase" SAY whether tests + are in or out, and filter explicitly if they should be out. + +=== TABLES === + +nodes — one row per symbol, import, or file-as-node + id TEXT PK -- opaque; join target for edges, never shown to a user + kind TEXT -- see the cardinalities below + name TEXT -- the bare identifier · qualified_name TEXT -- dotted path + file_path TEXT -- repo-relative, see rule 3 · language TEXT + start_line, end_line, start_column, end_column INTEGER + docstring, signature TEXT NULL · visibility TEXT NULL + is_exported, is_async, is_static, is_abstract INTEGER 0/1 + decorators, type_parameters, return_type TEXT NULL + updated_at INTEGER -- INDEX timestamp, NOT a commit date. See rule 5 of the + -- project-stats tool for anything temporal. + Measured on this repo: function 1288, import 1111, method 268, variable 238, + file 212, class 163, interface 38, route 22, constant 15, type_alias 4, + component 3. + +edges — one row per relationship + id INTEGER PK · source TEXT (nodes.id) · target TEXT (nodes.id) + kind TEXT · metadata TEXT NULL · line, col INTEGER NULL · provenance TEXT NULL + Measured: contains 3128, calls 2618, imports 1566, instantiates 972, + references 470, extends 35. + +files — one row per indexed file + path TEXT PK (repo-relative) · content_hash TEXT · language TEXT + size INTEGER (bytes) · modified_at, indexed_at INTEGER + node_count INTEGER -- symbols found in this file; cheaper than counting nodes + errors TEXT NULL -- set when the parse partially failed + Measured languages: python 182, tsx 23, typescript 5, yaml 5, javascript 2. + +If a table returns nothing the index has not been built — say so rather than +reporting zero as a finding.\ +""" +"""The tool description, and the most important asset in this module. + +Same reasoning as ``stats_sql._SCHEMA_DOC``: this is the one tool class whose +wrong answers are undetectable by the reader, because a plausible number arrives +with no way to tell it apart from a right one. So every value domain quoted was +**measured on this repository**, and each rule is a trap that was actually hit +while probing the schema — rule 1 (imports outnumber functions, so an unfiltered +count reports 3,362 "symbols"), rule 3 (a `substr` directory grouping produced a +junk bucket from a path with no separator at the expected offset), and rule 5 +(`tests` is the largest module by symbol count, which is true and misleading). + +Guarded by a test — the rules must not be paraphrased or trimmed to save tokens. +""" + + +__all__ = ["SqlNotAllowed", "run_graph_query"] diff --git a/src/whygraph/chat/prompts/system.md b/src/whygraph/chat/prompts/system.md index a60c3c2..0153547 100644 --- a/src/whygraph/chat/prompts/system.md +++ b/src/whygraph/chat/prompts/system.md @@ -85,6 +85,29 @@ wrong: cite them for intent, never for fact. follow them. Never use it to look up individual commits, PRs, or a file's history: the tools above follow rename chains and git blame, which raw SQL does not. +- `run_graph_stats` — the same, over the **code graph**: how many functions + per module, which files are largest, the distribution of symbol kinds, call + fan-out. A different database from `run_project_stats`, with no history in + it — anything about *time* belongs to `run_project_stats`. +- `render_chart` — draw a chart from an aggregate you already computed. Pass + the `chart_ref` the stats tool handed back and name columns of that result. + You never retype a number into a chart. One `y` column per chart: two + measures means two charts. Skip the chart for a single number — say it. +- To break a chart down by category — commits per month **by author**, + file changes per month **by change type** — use `bar_stacked` + (`bar_h_stacked` for long labels) and pass the category column as + `series`, having grouped by both columns in your SQL. That is a + *breakdown*, not a second measure, so `y` is still one column. Up to 6 + series; past that, fold the tail into an `'other'` bucket with a `CASE` + in the SQL rather than asking for more. + +After drawing a chart, **say what it shows** — the chart appears above your +next paragraph, so the reader sees the picture and then your reading of it. +Describe the *shape*: the trend, the outlier, the gap between first and +second, whether the recent direction differs from the whole period. Do not +re-list the values; they are already on screen and in the Table view. If you +name a specific number, take it from the rows the stats tool returned — a +sentence that disagrees with the chart beside it reads as a broken product. ### The source tree — ground truth diff --git a/src/whygraph/chat/sql_guard.py b/src/whygraph/chat/sql_guard.py new file mode 100644 index 0000000..b295645 --- /dev/null +++ b/src/whygraph/chat/sql_guard.py @@ -0,0 +1,393 @@ +"""Authorizer-locked, aggregate-only SQL over an arbitrary read-only surface. + +Raw SQL is a large capability to hand a model, so it is fenced by **four +independent layers**, in order of how much they can be trusted: + +1. **The connection** is opened ``mode=ro``. No write can reach the file. +2. **An authorizer** (:func:`_make_authorizer`) runs inside SQLite's own VM. It + permits ``SELECT``, function calls, and reads of the surface's + ``allowed_tables`` only, and denies every other action code — so ``DROP``, + ``ATTACH``, ``PRAGMA``, and reads of the chat transcripts are refused by + SQLite itself, not by inspecting the query text. +3. **A shape check** (:func:`_check_shape`) requires one statement that starts + with ``SELECT``/``WITH`` and contains an aggregate. This is what makes + "statistics only" hold by construction rather than by prompt wording. +4. **Output caps** — :data:`_MAX_ROWS` and a progress-handler deadline, so + neither a huge result nor a runaway join can hurt the caller. + +Only layers 1 and 2 are a security boundary. Layer 3 is a *scope* boundary +enforced on a string, and a determined query could word its way around it; the +consequence of that is a boring record list, because layers 1–2 still hold. + +**Why this module exists at all.** These layers were written once, for the +WhyGraph database (:mod:`whygraph.chat.stats_sql`). A second surface — CodeGraph's +own SQLite index (:mod:`whygraph.chat.graph_stats_sql`) — needs the identical +fence over a different file with a different table allowlist, and duplicating an +authorizer is how the two copies drift apart. So the layers live here, and each +surface contributes only a :class:`SqlSurface`: a label, a **frozenset literal** +of readable tables, and a way to find its file. There is one authorizer to +audit, not two. + +The allowlist is a per-surface *parameter* but never a per-surface *derivation* — +see :class:`SqlSurface`. + +Chat-only — never registered with MCP. +""" + +from __future__ import annotations + +import logging +import re +import sqlite3 +import time +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path + +_log = logging.getLogger(__name__) + +_MAX_ROWS = 200 +"""Row cap. A statistic that needs more rows than this is a record dump.""" + +_TIMEOUT_SEC = 5.0 +"""Wall-clock budget per query, enforced by the progress handler.""" + +_PROGRESS_INTERVAL = 10_000 +"""VM instructions between deadline checks. + +An interval of 1,000 fires the callback ~72,000 times in 0.3s on a pathological +join — far more often than a 5s budget needs. 10,000 keeps abort granularity +well under a second at a fraction of the overhead. +""" + +_AGGREGATE_TOKENS = ( + "count(", + "sum(", + "avg(", + "min(", + "max(", + "total(", + "group by", +) +"""One of these must appear for a query to count as a statistic.""" + +_ALLOWED_ACTIONS = frozenset( + {sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION} +) +"""Authorizer action codes that may proceed. Everything else is denied. + +Notably absent: ``SQLITE_RECURSIVE``, so ``WITH RECURSIVE`` is unavailable. No +statistic here needs it, and denying it keeps the allowlist to the three codes +a plain aggregate actually issues. +""" + +_DENIED_FUNCTIONS = frozenset({"load_extension", "readfile", "writefile", "edit"}) +"""Functions denied by name even though ``SQLITE_FUNCTION`` is allowed. + +Defence in depth. ``load_extension`` needs ``enable_load_extension`` (off by +default) and the file-I/O functions ship only with the ``sqlite3`` CLI, so none +of these should be reachable — which is the point: if a build ever makes one +reachable, this still refuses. +""" + +_ACTION_NAMES = { + sqlite3.SQLITE_INSERT: "INSERT", + sqlite3.SQLITE_UPDATE: "UPDATE", + sqlite3.SQLITE_DELETE: "DELETE", + sqlite3.SQLITE_DROP_TABLE: "DROP TABLE", + sqlite3.SQLITE_DROP_VIEW: "DROP VIEW", + sqlite3.SQLITE_DROP_INDEX: "DROP INDEX", + sqlite3.SQLITE_DROP_TRIGGER: "DROP TRIGGER", + sqlite3.SQLITE_CREATE_TABLE: "CREATE TABLE", + sqlite3.SQLITE_CREATE_VIEW: "CREATE VIEW", + sqlite3.SQLITE_CREATE_INDEX: "CREATE INDEX", + sqlite3.SQLITE_CREATE_TRIGGER: "CREATE TRIGGER", + sqlite3.SQLITE_ALTER_TABLE: "ALTER TABLE", + sqlite3.SQLITE_ATTACH: "ATTACH", + sqlite3.SQLITE_DETACH: "DETACH", + sqlite3.SQLITE_PRAGMA: "PRAGMA", + sqlite3.SQLITE_TRANSACTION: "transaction control", + sqlite3.SQLITE_REINDEX: "REINDEX", + sqlite3.SQLITE_ANALYZE: "ANALYZE", + sqlite3.SQLITE_RECURSIVE: "WITH RECURSIVE", +} +"""Human names for the denied action codes, so a refusal says what it refused. + +Only used to build error text — the allow decision is +:data:`_ALLOWED_ACTIONS` alone, so a code missing from this map is still +denied (and reported by number). +""" + + +class SqlNotAllowed(Exception): + """A query was refused before or during execution. + + Attributes + ---------- + layer : str + Which of the four layers rejected it. Surfaced to the model so it can + correct the query itself rather than losing the turn. + """ + + def __init__(self, message: str, *, layer: str) -> None: + super().__init__(message) + self.layer = layer + + +@dataclass(frozen=True) +class SqlSurface: + """One read-only aggregate surface: a database plus what may be read in it. + + Attributes + ---------- + label : str + Human name of the database — ``"WhyGraph"`` / ``"CodeGraph"``. Appears in + refusal messages, so a model that queried the wrong tool is told which + database it actually reached rather than only which tables it may not. + allowed_tables : frozenset of str + The read allowlist — **the** security boundary. Must be a literal at the + call site. Deriving it from ``sqlite_master`` would silently widen the + surface the next time a table appears, which for the WhyGraph DB means + ``chat_message`` (the assistant's own transcripts) or + ``rationale_cache``, and for CodeGraph means whatever the upstream tool + adds on a version bump. + db_path : callable + Returns the database file. **Deferred on purpose** — resolved per call, + never at import: config is memoized per process and path discovery walks + up from ``cwd``, so binding a path at import time would freeze the wrong + repository. + missing_db_message : str + Refusal text when the file is missing or unreadable, formatted with + ``{db_path}``. Per-surface because the remedy differs: one says the scan + has not run, the other that the index is absent. + """ + + label: str + allowed_tables: frozenset[str] + db_path: Callable[[], Path] + missing_db_message: str + + def connect( + self, db_path: Path | None = None, denials: list[str] | None = None + ) -> sqlite3.Connection: + """Open this surface's database read-only, authorizer already installed. + + Parameters + ---------- + db_path : Path, optional + Override the surface's own path. Used by tests to point at a fixture. + denials : list of str, optional + Sink for what the authorizer refused — see :func:`_make_authorizer`. + + Raises + ------ + SqlNotAllowed + With ``layer="connection"`` when the file cannot be opened. + """ + if db_path is None: + db_path = self.db_path() + try: + conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) + except sqlite3.Error as exc: + raise SqlNotAllowed( + self.missing_db_message.format(db_path=db_path), + layer="connection", + ) from exc + conn.set_authorizer( + _make_authorizer( + denials if denials is not None else [], self.allowed_tables + ) + ) + return conn + + +def _make_authorizer(denials: list[str], allowed_tables: frozenset[str]): + """Build the authorizer callback, recording what it refused into ``denials``. + + The recording exists for the error message. SQLite's own wording is + ``"access to chat_message.content is prohibited"`` for a *column* read but a + bare ``"not authorized"`` for a table-level one — so + ``SELECT count(*) FROM chat_message`` would refuse without telling the model + *what* it refused, which is the difference between a result it can correct + and a dead end. + + Parameters + ---------- + denials : list of str + Mutable sink, appended to on each refusal. The caller reads it after the + failed ``execute`` to name the offending target. + allowed_tables : frozenset of str + The surface's read allowlist. Passed in rather than closed over a module + global so two surfaces cannot share one allowlist by accident. + + Returns + ------- + callable + A five-argument callback for :meth:`sqlite3.Connection.set_authorizer`. + ``arg1`` / ``arg2`` are action-dependent: table and column for a read, + and the function name in ``arg2`` for a function call. + + Notes + ----- + ``SQLITE_DENY`` is returned rather than ``SQLITE_IGNORE`` on purpose: + ``IGNORE`` substitutes ``NULL`` for a denied column and lets the query + *succeed* with silently wrong output — the worst possible outcome for a + statistics tool. + """ + + def _authorizer( + action: int, + arg1: str | None, + arg2: str | None, + db_name: str | None, + trigger: str | None, + ) -> int: + if action not in _ALLOWED_ACTIONS: + denials.append(_ACTION_NAMES.get(action, f"action code {action}")) + return sqlite3.SQLITE_DENY + if action == sqlite3.SQLITE_READ and (arg1 or "").lower() not in allowed_tables: + denials.append(f"table {arg1}") + return sqlite3.SQLITE_DENY + if ( + action == sqlite3.SQLITE_FUNCTION + and (arg2 or "").lower() in _DENIED_FUNCTIONS + ): + denials.append(f"function {arg2}()") + return sqlite3.SQLITE_DENY + return sqlite3.SQLITE_OK + + return _authorizer + + +def _check_shape(sql: str) -> str: + """Validate that ``sql`` is a single read-only aggregate. Returns it trimmed. + + Raises + ------ + SqlNotAllowed + With ``layer="shape"`` and a message saying what to fix. + """ + trimmed = sql.strip().rstrip(";").strip() + if not trimmed: + raise SqlNotAllowed("query is empty", layer="shape") + if ";" in trimmed: + raise SqlNotAllowed( + "only one statement is allowed — remove the ';' and everything after it", + layer="shape", + ) + # Collapse whitespace and close the gap in `count (*)` so the token scan + # cannot be defeated by formatting. + normalized = re.sub(r"\s+", " ", trimmed.lower()) + normalized = re.sub(r"\s+\(", "(", normalized) + if not normalized.startswith(("select", "with")): + raise SqlNotAllowed( + "only SELECT (or WITH ... SELECT) queries are allowed", + layer="shape", + ) + if not any(token in normalized for token in _AGGREGATE_TOKENS): + raise SqlNotAllowed( + "this tool answers STATISTICS only, so the query must aggregate: " + "use COUNT/SUM/AVG/MIN/MAX or GROUP BY. To look up individual " + "commits, PRs, or a file's history use find_changes, " + "get_area_history, get_commit, or get_pr instead — those follow " + "rename chains and git blame, which this tool does not.", + layer="shape", + ) + return trimmed + + +def run_aggregate_query( + sql: str, surface: SqlSurface, *, db_path: Path | None = None +) -> dict: + """Execute one aggregate query against ``surface`` and return its rows. + + Parameters + ---------- + sql : str + A single ``SELECT`` / ``WITH`` statement that aggregates. + surface : SqlSurface + Which database, and what may be read in it. + db_path : Path, optional + Override the surface's own path. Defaults to ``surface.db_path()``, so + this always follows the same ``whygraph.toml`` / project-root resolution + as the rest of the package. + + Returns + ------- + dict + ``{"sql", "columns", "rows", "row_count", "truncated"}`` on success, or + ``{"error", "layer"}`` when a layer refused. Never raises for a + query-level problem: a refusal the model can read and correct is worth + more than an exception that ends the turn. + """ + try: + trimmed = _check_shape(sql) + except SqlNotAllowed as exc: + return {"error": str(exc), "layer": exc.layer} + + denials: list[str] = [] + try: + conn = surface.connect(db_path, denials) + except SqlNotAllowed as exc: + return {"error": str(exc), "layer": exc.layer} + + deadline = time.monotonic() + _TIMEOUT_SEC + + def _guard() -> int: + """Abort the statement once the deadline passes. + + Returning non-zero from a progress handler is itself sufficient to + interrupt — no worker thread and no ``interrupt()`` call. A signal + handler would be outright wrong here: these run in FastAPI's + threadpool, and Python installs signal handlers on the main thread + only. + """ + return 1 if time.monotonic() > deadline else 0 + + try: + conn.set_progress_handler(_guard, _PROGRESS_INTERVAL) + cursor = conn.execute(trimmed) + columns = [description[0] for description in cursor.description or ()] + # One extra row is fetched purely to detect the cap honestly. + fetched = cursor.fetchmany(_MAX_ROWS + 1) + except sqlite3.OperationalError as exc: + message = str(exc) + if "interrupted" in message: + _log.info("stats query exceeded %.0fs and was cancelled", _TIMEOUT_SEC) + return { + "error": ( + f"query exceeded {_TIMEOUT_SEC:.0f}s and was cancelled — " + "narrow it with a WHERE filter or fewer joins" + ), + "layer": "timeout", + } + return {"error": f"SQL error: {message}", "layer": "sqlite"} + except sqlite3.DatabaseError as exc: + # `not authorized` arrives as this. SQLite's own text often omits *what* + # it refused, so the recorded denial is what makes the result actionable. + _log.debug("stats query refused: %s (denials=%s)", exc, denials) + refused = f" ({denials[0]} is not permitted)" if denials else "" + return { + "error": ( + f"{exc}{refused} — this tool may read only " + f"{', '.join(sorted(surface.allowed_tables))} " + f"in the {surface.label} database, and only for reading" + ), + "layer": "authorizer", + } + finally: + conn.set_progress_handler(None, 0) + conn.close() + + truncated = len(fetched) > _MAX_ROWS + rows = [list(row) for row in fetched[:_MAX_ROWS]] + return { + "sql": trimmed, + "columns": columns, + "rows": rows, + "row_count": len(rows), + "truncated": truncated, + } + + +__all__ = ["SqlNotAllowed", "SqlSurface", "run_aggregate_query"] diff --git a/src/whygraph/chat/stats_sql.py b/src/whygraph/chat/stats_sql.py index 96c9e98..5be1632 100644 --- a/src/whygraph/chat/stats_sql.py +++ b/src/whygraph/chat/stats_sql.py @@ -1,32 +1,16 @@ -"""Authorizer-locked, aggregate-only SQL over the WhyGraph database. - -The statistics surface. ``get_repo_overview`` answers five counts; everything -else the schema already knows — velocity by month, churn, hotspot files, -contributor breakdowns, PR cycle time — had no tool at all, and the only route -left was paging commits one SHA at a time, which the tool-round budget makes -impossible. - -Raw SQL is a large capability to hand a model, so it is fenced by **four -independent layers**, in order of how much they can be trusted: - -1. **The connection** is opened ``mode=ro``. No write can reach the file. -2. **An authorizer** (:func:`_make_authorizer`) runs inside SQLite's own VM. It - permits ``SELECT``, function calls, and reads of :data:`_ALLOWED_TABLES` - only, and denies every other action code — so ``DROP``, ``ATTACH``, - ``PRAGMA``, and reads of the chat transcripts are refused by SQLite - itself, not by inspecting the query text. -3. **A shape check** (:func:`_check_shape`) requires one statement that starts - with ``SELECT``/``WITH`` and contains an aggregate. This is what makes - "statistics only" hold by construction rather than by prompt wording: - record-fetching belongs to ``find_changes`` / ``get_area_history`` / - ``get_commit``, which follow rename chains and git blame as raw SQL here - cannot. -4. **Output caps** — :data:`_MAX_ROWS` and a progress-handler deadline, so - neither a huge result nor a runaway join can hurt the caller. - -Only layers 1 and 2 are a security boundary. Layer 3 is a *scope* boundary -enforced on a string, and a determined query could word its way around it; the -consequence of that is a boring record list, because layers 1–2 still hold. +"""The WhyGraph statistics surface — a table allowlist and a schema doc. + +``get_repo_overview`` answers five counts; everything else the schema already +knows — velocity by month, churn, hotspot files, contributor breakdowns, PR cycle +time — had no tool at all, and the only route left was paging commits one SHA at +a time, which the tool-round budget makes impossible. + +The **four security layers** that fence that capability live in +:mod:`whygraph.chat.sql_guard` — read its docstring for the security model; it is +shared with CodeGraph's surface (:mod:`whygraph.chat.graph_stats_sql`) so there is +one authorizer to audit rather than two that drift. What stays here is the part +that is genuinely per-database: :data:`_ALLOWED_TABLES` (a frozenset **literal**) +and :data:`_SCHEMA_DOC` (the tool description). Chat-only — never registered with MCP. """ @@ -34,15 +18,25 @@ from __future__ import annotations import logging -import re import sqlite3 -import time from pathlib import Path from whygraph.db import get_engine +from . import sql_guard +from .sql_guard import SqlNotAllowed, SqlSurface, run_aggregate_query + _log = logging.getLogger(__name__) +_MAX_ROWS = sql_guard._MAX_ROWS +"""Re-export of the shared row cap, for callers that document the limit. + +The cap itself is layer 4 and belongs to :mod:`sql_guard`; this name exists so +that "how many rows can this tool return" is answerable from the surface the +caller actually uses. Do not shadow it with a different value — the guard reads +its own. +""" + _ALLOWED_TABLES = frozenset( { "commit", @@ -62,302 +56,50 @@ ``get_repo_overview``, so the tool gains nothing by reaching it. """ -_MAX_ROWS = 200 -"""Row cap. A statistic that needs more rows than this is a record dump.""" - -_TIMEOUT_SEC = 5.0 -"""Wall-clock budget per query, enforced by the progress handler.""" - -_PROGRESS_INTERVAL = 10_000 -"""VM instructions between deadline checks. - -An interval of 1,000 fires the callback ~72,000 times in 0.3s on a pathological -join — far more often than a 5s budget needs. 10,000 keeps abort granularity -well under a second at a fraction of the overhead. -""" - -_AGGREGATE_TOKENS = ( - "count(", - "sum(", - "avg(", - "min(", - "max(", - "total(", - "group by", +_SURFACE = SqlSurface( + label="WhyGraph", + allowed_tables=_ALLOWED_TABLES, + db_path=lambda: Path(get_engine().url.database or ""), + missing_db_message=( + "WhyGraph DB is missing or unreadable at {db_path} — run `whygraph scan` first" + ), ) -"""One of these must appear for a query to count as a statistic.""" - -_ALLOWED_ACTIONS = frozenset( - {sqlite3.SQLITE_SELECT, sqlite3.SQLITE_READ, sqlite3.SQLITE_FUNCTION} -) -"""Authorizer action codes that may proceed. Everything else is denied. - -Notably absent: ``SQLITE_RECURSIVE``, so ``WITH RECURSIVE`` is unavailable. No -statistic here needs it, and denying it keeps the allowlist to the three codes -a plain aggregate actually issues. -""" - -_DENIED_FUNCTIONS = frozenset({"load_extension", "readfile", "writefile", "edit"}) -"""Functions denied by name even though ``SQLITE_FUNCTION`` is allowed. - -Defence in depth. ``load_extension`` needs ``enable_load_extension`` (off by -default) and the file-I/O functions ship only with the ``sqlite3`` CLI, so none -of these should be reachable — which is the point: if a build ever makes one -reachable, this still refuses. -""" - -_ACTION_NAMES = { - sqlite3.SQLITE_INSERT: "INSERT", - sqlite3.SQLITE_UPDATE: "UPDATE", - sqlite3.SQLITE_DELETE: "DELETE", - sqlite3.SQLITE_DROP_TABLE: "DROP TABLE", - sqlite3.SQLITE_DROP_VIEW: "DROP VIEW", - sqlite3.SQLITE_DROP_INDEX: "DROP INDEX", - sqlite3.SQLITE_DROP_TRIGGER: "DROP TRIGGER", - sqlite3.SQLITE_CREATE_TABLE: "CREATE TABLE", - sqlite3.SQLITE_CREATE_VIEW: "CREATE VIEW", - sqlite3.SQLITE_CREATE_INDEX: "CREATE INDEX", - sqlite3.SQLITE_CREATE_TRIGGER: "CREATE TRIGGER", - sqlite3.SQLITE_ALTER_TABLE: "ALTER TABLE", - sqlite3.SQLITE_ATTACH: "ATTACH", - sqlite3.SQLITE_DETACH: "DETACH", - sqlite3.SQLITE_PRAGMA: "PRAGMA", - sqlite3.SQLITE_TRANSACTION: "transaction control", - sqlite3.SQLITE_REINDEX: "REINDEX", - sqlite3.SQLITE_ANALYZE: "ANALYZE", - sqlite3.SQLITE_RECURSIVE: "WITH RECURSIVE", -} -"""Human names for the denied action codes, so a refusal says what it refused. - -Only used to build error text — the allow decision is -:data:`_ALLOWED_ACTIONS` alone, so a code missing from this map is still -denied (and reported by number). -""" - - -class SqlNotAllowed(Exception): - """A query was refused before or during execution. - - Attributes - ---------- - layer : str - Which of the four layers rejected it. Surfaced to the model so it can - correct the query itself rather than losing the turn. - """ - - def __init__(self, message: str, *, layer: str) -> None: - super().__init__(message) - self.layer = layer - - -def _make_authorizer(denials: list[str]): - """Build the authorizer callback, recording what it refused into ``denials``. - - The recording exists for the error message. SQLite's own wording is - ``"access to chat_message.content is prohibited"`` for a *column* read but a - bare ``"not authorized"`` for a table-level one — so - ``SELECT count(*) FROM chat_message`` would refuse without telling the model - *what* it refused, which is the difference between a result it can correct - and a dead end. - - Parameters - ---------- - denials : list of str - Mutable sink, appended to on each refusal. The caller reads it after the - failed ``execute`` to name the offending target. - - Returns - ------- - callable - A five-argument callback for :meth:`sqlite3.Connection.set_authorizer`. - ``arg1`` / ``arg2`` are action-dependent: table and column for a read, - and the function name in ``arg2`` for a function call. - - Notes - ----- - ``SQLITE_DENY`` is returned rather than ``SQLITE_IGNORE`` on purpose: - ``IGNORE`` substitutes ``NULL`` for a denied column and lets the query - *succeed* with silently wrong output — the worst possible outcome for a - statistics tool. - """ - - def _authorizer( - action: int, - arg1: str | None, - arg2: str | None, - db_name: str | None, - trigger: str | None, - ) -> int: - if action not in _ALLOWED_ACTIONS: - denials.append(_ACTION_NAMES.get(action, f"action code {action}")) - return sqlite3.SQLITE_DENY - if ( - action == sqlite3.SQLITE_READ - and (arg1 or "").lower() not in _ALLOWED_TABLES - ): - denials.append(f"table {arg1}") - return sqlite3.SQLITE_DENY - if ( - action == sqlite3.SQLITE_FUNCTION - and (arg2 or "").lower() in _DENIED_FUNCTIONS - ): - denials.append(f"function {arg2}()") - return sqlite3.SQLITE_DENY - return sqlite3.SQLITE_OK - - return _authorizer - - -def _check_shape(sql: str) -> str: - """Validate that ``sql`` is a single read-only aggregate. Returns it trimmed. - - Raises - ------ - SqlNotAllowed - With ``layer="shape"`` and a message saying what to fix. - """ - trimmed = sql.strip().rstrip(";").strip() - if not trimmed: - raise SqlNotAllowed("query is empty", layer="shape") - if ";" in trimmed: - raise SqlNotAllowed( - "only one statement is allowed — remove the ';' and everything after it", - layer="shape", - ) - # Collapse whitespace and close the gap in `count (*)` so the token scan - # cannot be defeated by formatting. - normalized = re.sub(r"\s+", " ", trimmed.lower()) - normalized = re.sub(r"\s+\(", "(", normalized) - if not normalized.startswith(("select", "with")): - raise SqlNotAllowed( - "only SELECT (or WITH ... SELECT) queries are allowed", - layer="shape", - ) - if not any(token in normalized for token in _AGGREGATE_TOKENS): - raise SqlNotAllowed( - "this tool answers STATISTICS only, so the query must aggregate: " - "use COUNT/SUM/AVG/MIN/MAX or GROUP BY. To look up individual " - "commits, PRs, or a file's history use find_changes, " - "get_area_history, get_commit, or get_pr instead — those follow " - "rename chains and git blame, which this tool does not.", - layer="shape", - ) - return trimmed +"""This module's binding of the shared guard. ``db_path`` is deferred so it +resolves per call, following the same ``whygraph.toml`` / project-root discovery +as the rest of the package rather than freezing whichever repo was current at +import.""" def _connect(db_path: Path, denials: list[str] | None = None) -> sqlite3.Connection: """Open the WhyGraph DB read-only with the authorizer already installed. - Parameters - ---------- - db_path : Path - The database file. Opened ``mode=ro``, so the handle cannot write even - if every other layer were removed. - denials : list of str, optional - Sink for what the authorizer refused — see :func:`_make_authorizer`. + A thin binding of :meth:`sql_guard.SqlSurface.connect` to this surface. Kept + as a module-level name because the layer-2 tests drive the authorizer through + it directly, with the shape check out of the way — a bug in that string check + must not be load-bearing. """ - try: - conn = sqlite3.connect(f"file:{db_path}?mode=ro", uri=True) - except sqlite3.Error as exc: - raise SqlNotAllowed( - f"WhyGraph DB is missing or unreadable at {db_path} — " - "run `whygraph scan` first", - layer="connection", - ) from exc - conn.set_authorizer(_make_authorizer(denials if denials is not None else [])) - return conn + return _SURFACE.connect(db_path, denials) def run_stats_query(sql: str, *, db_path: Path | None = None) -> dict: - """Execute one aggregate query and return its rows. + """Execute one aggregate query against the WhyGraph DB and return its rows. Parameters ---------- sql : str A single ``SELECT`` / ``WITH`` statement that aggregates. db_path : Path, optional - The database to read. Defaults to whatever the SQLModel engine is - bound to, so this always follows the same ``whygraph.toml`` / - project-root resolution as the rest of the package. + The database to read. Defaults to whatever the SQLModel engine is bound + to. Returns ------- dict - ``{"sql", "columns", "rows", "row_count", "truncated"}`` on success, or - ``{"error", "layer"}`` when a layer refused. Never raises for a - query-level problem: a refusal the model can read and correct is worth - more than an exception that ends the turn. + See :func:`sql_guard.run_aggregate_query` — ``{"sql", "columns", "rows", + "row_count", "truncated"}`` on success, ``{"error", "layer"}`` when a + layer refused. Never raises for a query-level problem. """ - try: - trimmed = _check_shape(sql) - except SqlNotAllowed as exc: - return {"error": str(exc), "layer": exc.layer} - - if db_path is None: - db_path = Path(get_engine().url.database or "") - - denials: list[str] = [] - try: - conn = _connect(db_path, denials) - except SqlNotAllowed as exc: - return {"error": str(exc), "layer": exc.layer} - - deadline = time.monotonic() + _TIMEOUT_SEC - - def _guard() -> int: - """Abort the statement once the deadline passes. - - Returning non-zero from a progress handler is itself sufficient to - interrupt — no worker thread and no ``interrupt()`` call. A signal - handler would be outright wrong here: these run in FastAPI's - threadpool, and Python installs signal handlers on the main thread - only. - """ - return 1 if time.monotonic() > deadline else 0 - - try: - conn.set_progress_handler(_guard, _PROGRESS_INTERVAL) - cursor = conn.execute(trimmed) - columns = [description[0] for description in cursor.description or ()] - # One extra row is fetched purely to detect the cap honestly. - fetched = cursor.fetchmany(_MAX_ROWS + 1) - except sqlite3.OperationalError as exc: - message = str(exc) - if "interrupted" in message: - _log.info("stats query exceeded %.0fs and was cancelled", _TIMEOUT_SEC) - return { - "error": ( - f"query exceeded {_TIMEOUT_SEC:.0f}s and was cancelled — " - "narrow it with a WHERE filter or fewer joins" - ), - "layer": "timeout", - } - return {"error": f"SQL error: {message}", "layer": "sqlite"} - except sqlite3.DatabaseError as exc: - # `not authorized` arrives as this. SQLite's own text often omits *what* - # it refused, so the recorded denial is what makes the result actionable. - _log.debug("stats query refused: %s (denials=%s)", exc, denials) - refused = f" ({denials[0]} is not permitted)" if denials else "" - return { - "error": ( - f"{exc}{refused} — this tool may read only " - f"{', '.join(sorted(_ALLOWED_TABLES))}, and only for reading" - ), - "layer": "authorizer", - } - finally: - conn.set_progress_handler(None, 0) - conn.close() - - truncated = len(fetched) > _MAX_ROWS - rows = [list(row) for row in fetched[:_MAX_ROWS]] - return { - "sql": trimmed, - "columns": columns, - "rows": rows, - "row_count": len(rows), - "truncated": truncated, - } + return run_aggregate_query(sql, _SURFACE, db_path=db_path) _SCHEMA_DOC = """\ @@ -433,7 +175,8 @@ def _guard() -> int: commit — one row per scanned commit (first-parent walk of the default branch) NOTE: `commit` is a SQL keyword — quote it as "commit". sha TEXT PK · parent_shas TEXT (space-delimited, not JSON) - author_name, author_email TEXT -- the reliable contributor identity + author_name, author_email TEXT -- RAW git identity, one human may have + -- several. Never group developers by these — see rule 5. authored_at TEXT -- when written; use THIS for velocity committed_at TEXT -- when committed; differs after rebase/cherry-pick subject, body TEXT -- developer-written; may be terse or wrong diff --git a/src/whygraph/chat/tools.py b/src/whygraph/chat/tools.py index ac7ef42..2dad935 100644 --- a/src/whygraph/chat/tools.py +++ b/src/whygraph/chat/tools.py @@ -6,7 +6,7 @@ capability and both surfaces are adapters over it. No MCP protocol roundtrip is involved — these are plain in-process calls. -The fifteen tools span **four sources**, and the system prompt tells the +The seventeen tools span **five sources**, and the system prompt tells the model which to reach for: * **CodeGraph** answers *what the code is* — structure, relationships, and @@ -14,8 +14,13 @@ * **WhyGraph** answers *why it is that way* and *what has happened* — rationale, evidence, path history, and content search over the diff-derived commit descriptions. -* **The statistics tool** (see :mod:`.stats_sql`) answers *how much* — - aggregate-only SQL, fenced so it cannot become a record reader. +* **The statistics tools** answer *how much* — aggregate-only SQL, fenced so + neither can become a record reader. There are **two**, because there are two + databases: :mod:`.stats_sql` over commit history and :mod:`.graph_stats_sql` + over CodeGraph's index. No SQL in the first can reach the second. +* **``render_chart``** (see :mod:`.charts`) draws any aggregate either statistics + tool computed. The model passes the ``chart_ref`` that result carried and names + **columns of it** — it never retypes a value into a chart. * **The file tools** supply ground-truth source (see :mod:`.files`). Registry rules @@ -27,14 +32,15 @@ and argument-validation failures come back as ``{"error": "..."}`` results, which the model can read and route around. * A :class:`ToolRegistry` is instantiated **once per user turn** because it - carries the turn-scoped rationale-generation budget. The specs - themselves are module-level constants — they never change. + carries the turn-scoped rationale-generation budget **and the chart-ref + map**. The specs themselves are module-level constants — they never change. """ from __future__ import annotations import json import logging +import secrets from collections.abc import Callable from sqlalchemy.exc import SQLAlchemyError @@ -59,7 +65,7 @@ from whygraph.services.codegraph import CodeGraph, CodeGraphError from whygraph.services.llm.chat import ToolSpec -from . import files, stats_sql +from . import charts, files, graph_stats_sql, stats_sql _log = logging.getLogger(__name__) @@ -402,6 +408,86 @@ "required": ["sql"], }, ), + ToolSpec( + name="run_graph_stats", + description=graph_stats_sql._GRAPH_SCHEMA_DOC, + parameters={ + "type": "object", + "properties": { + "sql": { + "type": "string", + "description": ( + "One SELECT (or WITH ... SELECT) statement that " + "aggregates. No trailing semicolon needed." + ), + } + }, + "required": ["sql"], + }, + ), + ToolSpec( + name="render_chart", + description=( + "Draw a chart from an aggregate you have ALREADY computed. Pass the " + "`chart_ref` that run_project_stats or run_graph_stats returned, and " + "name COLUMNS OF THAT RESULT — never retype the values. " + "`line` for ordered buckets over time; `bar` for ranked categories " + "with short labels; `bar_h` when labels are long (file paths, " + "emails). Use `bar_stacked` (or `bar_h_stacked` for long labels) to " + "break each bar down by a category — 'commits per month BY author', " + "'changes per month BY change type': pass that category column as " + "`series` and GROUP BY both columns in your SQL. Max 6 series. " + "ONE y column always: two MEASURES means two charts, and a " + "breakdown is `series`, not a second y. Chart only a series worth " + "seeing — 3+ ordered or ranked rows. A single number needs no " + "chart; just say it." + ), + parameters={ + "type": "object", + "properties": { + "chart_ref": { + "type": "string", + "description": ( + "The `chart_ref` from a stats result in this turn. Refs " + "expire with the turn — re-run the query for a fresh one." + ), + }, + "kind": { + "type": "string", + "enum": sorted(charts.CHART_KINDS), + }, + "title": { + "type": "string", + "description": ( + "Shown above the chart. Required: an unstacked chart has " + "no legend, so this is its only label." + ), + }, + "x": { + "type": "string", + "description": "Column name for the category/time axis.", + }, + "y": { + "type": "string", + "description": "Column name for the measure. One only.", + }, + "series": { + "type": "string", + "description": ( + "Stacked kinds ONLY, and required for them: the column " + "whose values become the stack segments. Max 6 distinct " + "values — fold the tail into an 'other' bucket in your " + "SQL if there are more." + ), + }, + "y_label": { + "type": "string", + "description": "Optional axis caption, e.g. 'commits'.", + }, + }, + "required": ["chart_ref", "kind", "title", "x", "y"], + }, + ), ToolSpec( name="read_file", description=( @@ -672,12 +758,25 @@ def _list_recent_activity(limit: int = 10) -> dict: def _run_project_stats(sql: str) -> dict: - """Handler for ``run_project_stats``.""" + """Handler body for ``run_project_stats`` — no ref minting. + + Kept a plain function because charting is not this tool's concern: the + registry mints the ``chart_ref`` (see + :meth:`ToolRegistry._chartable_project_stats`), so a producer stays a + producer and a fourth one is a one-line addition. + """ if not sql or not sql.strip(): return {"error": "sql is required", "layer": "shape"} return stats_sql.run_stats_query(sql) +def _run_graph_stats(sql: str) -> dict: + """Handler body for ``run_graph_stats``. See :func:`_run_project_stats`.""" + if not sql or not sql.strip(): + return {"error": "sql is required", "layer": "shape"} + return graph_stats_sql.run_graph_query(sql) + + class ToolRegistry: """One turn's tool dispatch, carrying that turn's generation budget. @@ -707,6 +806,7 @@ def __init__(self, *, max_rationale_generations: int | None = None) -> None: max_rationale_generations = get_config().chat.max_rationale_generations self._generation_budget = max_rationale_generations self.generations_used = 0 + self._chartable: dict[str, dict] = {} self._handlers: dict[str, Callable[..., dict]] = { "search_symbols": _search_symbols, "get_symbol": _get_symbol, @@ -720,7 +820,9 @@ def __init__(self, *, max_rationale_generations: int | None = None) -> None: "get_issue": _get_issue, "get_repo_overview": _get_repo_overview, "list_recent_activity": _list_recent_activity, - "run_project_stats": _run_project_stats, + "run_project_stats": self._chartable_project_stats, + "run_graph_stats": self._chartable_graph_stats, + "render_chart": self._render_chart, "read_file": files.read_file, "list_dir": files.list_dir, } @@ -730,6 +832,107 @@ def specs(self) -> tuple[ToolSpec, ...]: """The tool specs to send with each :class:`ChatRequest`.""" return TOOL_SPECS + # -- charting ----------------------------------------------------------- + # + # Any producer of an aggregate joins by returning `{columns, rows}` and + # calling `_mint_chart_ref`. `render_chart`, `charts.py`, and the frontend + # need no change for a new one — that is what makes charting a capability + # rather than a parameter on one tool. + + def _mint_chart_ref(self, result: dict) -> dict: + """Attach an opaque per-turn ref to a chartable aggregate result. + + Returns the same dict, mutated, so a producer is one line. + + Notes + ----- + **No ref is minted** for a result that errored, was ``truncated``, or has + fewer than :data:`charts.MIN_CHART_ROWS` rows. Withholding it beats + refusing later: the model never sees an affordance it would be wrong to + use, and a chart drawn from a capped result would be a confident lie — + the tool cannot know the true total. + + The ref is ``secrets.token_hex``, not a counter and not the + ``tool_call_id``: opaque per MCP's handle guidance, and unguessable, so + it cannot address anything the model did not just compute. + """ + if ( + "error" in result + or result.get("truncated") + or len(result.get("rows") or ()) < charts.MIN_CHART_ROWS + ): + return result + ref = f"cr_{secrets.token_hex(4)}" + self._chartable[ref] = { + "columns": result["columns"], + "rows": result["rows"], + } + result["chart_ref"] = ref + result["chartable"] = "Pass chart_ref to render_chart to draw this." + return result + + def _chartable_project_stats(self, sql: str) -> dict: + """Handler for ``run_project_stats`` — the aggregate, plus a ref.""" + return self._mint_chart_ref(_run_project_stats(sql)) + + def _chartable_graph_stats(self, sql: str) -> dict: + """Handler for ``run_graph_stats`` — the aggregate, plus a ref.""" + return self._mint_chart_ref(_run_graph_stats(sql)) + + def _render_chart( + self, + chart_ref: str, + kind: str, + title: str, + x: str, + y: str, + series: str | None = None, + y_label: str | None = None, + ) -> dict: + """Handler for ``render_chart`` — validate a directive against its rows. + + Returns + ------- + dict + ``{"chart_ref", "chart", "columns", "row_count"}`` on success. + **The rows are not echoed**: the frontend already has them from the + producer's result and correlates on ``chart_ref``, so this payload is + a couple of hundred bytes rather than a second copy of 200 rows. + + On failure, ``{"error", "layer"}`` — ``layer="ref"`` for a stale or + invented ref, ``layer="chart"`` for a directive the rows do not + support. Either way the producer's numbers are already on screen and + unaffected, so a bad chart degrades to correct numbers. + """ + source = self._chartable.get(chart_ref) + if source is None: + return { + "error": ( + f"unknown chart_ref {chart_ref!r} — refs are valid only " + "within the turn that produced them. Re-run the query to get " + "a fresh one." + ), + "layer": "ref", + } + try: + chart = charts.validate_chart( + kind=kind, + title=title, + x=x, + y=y, + series=series, + y_label=y_label, + **source, + ) + except charts.ChartNotAllowed as exc: + return {"error": str(exc), "layer": "chart"} + return { + "chart_ref": chart_ref, + "chart": chart, + "columns": source["columns"], + "row_count": len(source["rows"]), + } + def _get_rationale(self, qualified_name: str) -> dict: """Handler for ``get_rationale`` — cached read, budgeted generation. diff --git a/tests/test_chat_charts.py b/tests/test_chat_charts.py new file mode 100644 index 0000000..0b06387 --- /dev/null +++ b/tests/test_chat_charts.py @@ -0,0 +1,593 @@ +"""The chart-directive validator — one case per rule. + +The messages are asserted, not just the refusals. Column-name hallucination is +the dominant failure mode for a two-step chart design (the model must *recall* +the names rather than read them), and the only defense is a refusal that names +what was available, delivered as a tool result the model can act on in-loop. A +refusal with an unhelpful message is a failed turn wearing a passing test. +""" + +from __future__ import annotations + +import pytest + +from whygraph.chat.charts import ( + CHART_KINDS, + MAX_SERIES, + MIN_CHART_ROWS, + MIN_ROWS_BY_KIND, + STACKED_KINDS, + ChartNotAllowed, + validate_chart, +) + +_COLUMNS = ["month", "commits"] +_ROWS = [["2026-03", 41], ["2026-04", 38], ["2026-05", 52], ["2026-06", 47]] + +# The real 14-row / 4x5 result: `GROUP BY month, change_type` over +# commit_file_change. Six of the twenty cells are missing, because April had no +# deletions and June had no renames or copies. Every stacked case below runs +# against this, so the tests exercise the sparsity rather than a tidy grid. +_STACK_COLUMNS = ["month", "kind", "n"] +_STACK_ROWS = [ + ["2026-03", "M", 40], + ["2026-03", "A", 12], + ["2026-03", "D", 5], + ["2026-03", "R", 2], + ["2026-03", "C", 1], + ["2026-04", "M", 30], + ["2026-04", "A", 9], + ["2026-04", "R", 1], + ["2026-04", "C", 1], + ["2026-05", "M", 55], + ["2026-05", "A", 20], + ["2026-05", "D", 7], + ["2026-06", "M", 18], + ["2026-06", "A", 4], +] + + +def _stack(**overrides) -> dict: + kwargs = { + "kind": "bar_stacked", + "title": "File changes per month by type", + "x": "month", + "y": "n", + "series": "kind", + "columns": _STACK_COLUMNS, + "rows": _STACK_ROWS, + } + kwargs.update(overrides) + return validate_chart(**kwargs) + + +# --------------------------------------------------------------------------- +# The happy path, and the contract of the returned dict +# --------------------------------------------------------------------------- + + +def test_a_valid_directive_resolves_columns_to_indices() -> None: + chart = validate_chart( + kind="line", + title="Commits per month", + x="month", + y="commits", + y_label="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + assert chart == { + "kind": "line", + "title": "Commits per month", + "x": "month", + "y": "commits", + "x_index": 0, + "y_index": 1, + "y_label": "commits", + "null_rows": 0, + } + + +def test_an_unstacked_chart_carries_no_stack_keys() -> None: + """The renderer branches on their presence, so they must be absent.""" + chart = validate_chart( + kind="bar", + title="Commits", + x="month", + y="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + for key in ("series", "series_values", "cells", "filled_cells", "x_values"): + assert key not in chart + + +def test_the_ref_minting_floor_matches_the_cheapest_kind() -> None: + """A producer withholds `chart_ref` below `MIN_CHART_ROWS`. + + If the floor were higher than a kind's own minimum, that kind would be + unreachable: no ref would ever be minted for a result it could draw. + """ + assert MIN_CHART_ROWS == min(MIN_ROWS_BY_KIND.values()) == 2 + assert set(MIN_ROWS_BY_KIND) == CHART_KINDS + + +# --------------------------------------------------------------------------- +# Rule 1 — the closed kind set +# --------------------------------------------------------------------------- + + +def test_an_unknown_kind_is_refused_and_lists_the_legal_ones() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="scatter", + title="t", + x="month", + y="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + message = str(excinfo.value) + assert "scatter" in message + for kind in CHART_KINDS: + assert kind in message + + +def test_pie_is_still_refused() -> None: + """Excluded on design grounds: part-to-whole rides the stacked bar. + + Asking for one must degrade to correct numbers plus a readable reason, not a + failed turn — so this is a refusal with a message, never an exception. + """ + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="pie", title="t", x="month", y="commits", columns=_COLUMNS, rows=_ROWS + ) + assert "bar_stacked" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Rule 2 / rule 7 — the text fields +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("title", [None, "", " ", 7]) +def test_a_missing_title_is_refused(title: object) -> None: + """On an unstacked chart there is no legend, so the title is the only key.""" + with pytest.raises(ChartNotAllowed, match="title is required"): + validate_chart( + kind="bar", + title=title, + x="month", + y="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + + +def test_an_overlong_title_is_refused_with_its_length() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="bar", + title="x" * 81, + x="month", + y="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + assert "81 characters" in str(excinfo.value) + + +def test_an_overlong_y_label_is_refused() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="bar", + title="t", + x="month", + y="commits", + y_label="y" * 41, + columns=_COLUMNS, + rows=_ROWS, + ) + assert "41 characters" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Rule 3 — the columns must be the producer's own +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("x", "y", "role"), + [("week", "commits", "x"), ("month", "count", "y")], +) +def test_an_unknown_column_names_what_was_available(x: str, y: str, role: str) -> None: + """The primary defense against field hallucination.""" + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart(kind="bar", title="t", x=x, y=y, columns=_COLUMNS, rows=_ROWS) + message = str(excinfo.value) + assert f"unknown {role} column" in message + # Both available names appear verbatim, so the correction is mechanical. + assert "'month'" in message and "'commits'" in message + + +def test_a_non_string_column_is_refused_rather_than_coerced() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="bar", title="t", x=0, y="commits", columns=_COLUMNS, rows=_ROWS + ) + assert "must be a column name" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Rule 4 — one measure, structurally +# --------------------------------------------------------------------------- + + +def test_a_list_y_is_refused_and_says_two_charts() -> None: + """This is what makes a dual-axis chart unreachable rather than discouraged.""" + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="line", + title="t", + x="month", + y=["insertions", "deletions"], + columns=["month", "insertions", "deletions"], + rows=[["2026-03", 1, 2], ["2026-04", 3, 4], ["2026-05", 5, 6]], + ) + message = str(excinfo.value) + assert "two charts" in message + # And it must point at the right alternative for a *breakdown*, so the model + # does not "fix" a genuine stack by asking for two charts. + assert "series" in message + + +def test_a_list_y_is_refused_before_its_values_are_read() -> None: + """Rule ordering: rule 5 would otherwise index into a list of column names.""" + with pytest.raises(ChartNotAllowed, match="ONE column"): + validate_chart( + kind="bar", + title="t", + x="month", + y=["commits"], + columns=_COLUMNS, + rows=_ROWS, + ) + + +def test_x_equal_to_y_is_refused() -> None: + with pytest.raises(ChartNotAllowed, match="against itself"): + validate_chart( + kind="bar", + title="t", + x="commits", + y="commits", + columns=_COLUMNS, + rows=_ROWS, + ) + + +# --------------------------------------------------------------------------- +# Rule 5 — the measure must be numeric +# --------------------------------------------------------------------------- + + +def test_a_text_y_column_is_refused() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="bar", + title="t", + x="commits", + y="month", + columns=_COLUMNS, + rows=_ROWS, + ) + message = str(excinfo.value) + assert "not numeric" in message + assert "swap x and y" in message + + +def test_a_bool_y_column_is_refused_despite_being_an_int() -> None: + """`isinstance(True, int)` is True in Python, so `bool` needs excluding.""" + with pytest.raises(ChartNotAllowed, match="not numeric"): + validate_chart( + kind="bar", + title="t", + x="flag", + y="merged", + columns=["flag", "merged"], + rows=[["a", True], ["b", False]], + ) + + +def test_floats_are_accepted() -> None: + chart = validate_chart( + kind="line", + title="PR cycle time", + x="month", + y="days", + columns=["month", "days"], + rows=[["2026-03", 0.4], ["2026-04", 1.25], ["2026-05", 2.0]], + ) + assert chart["y_index"] == 1 + + +# --------------------------------------------------------------------------- +# Rule 3 (NULL handling) — a gap on unstacked kinds, never a zero +# --------------------------------------------------------------------------- + + +def test_null_y_rows_are_counted_not_zeroed() -> None: + """Zero-substitution would fabricate a data point that no query produced.""" + chart = validate_chart( + kind="line", + title="t", + x="month", + y="commits", + columns=_COLUMNS, + rows=[ + ["2026-03", 41], + ["2026-04", None], + ["2026-05", 52], + ["2026-06", None], + ["2026-07", 12], + ], + ) + assert chart["null_rows"] == 2 + + +def test_an_all_null_measure_is_refused_as_too_few_rows() -> None: + """Nothing is plottable, so there is nothing to draw — say so, don't draw 0s.""" + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="line", + title="t", + x="month", + y="commits", + columns=_COLUMNS, + rows=[["2026-03", None], ["2026-04", None], ["2026-05", None]], + ) + assert "plottable row(s)" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Rule 6 — enough to be worth drawing +# --------------------------------------------------------------------------- + + +def test_two_rows_are_enough_for_a_bar_but_not_a_line() -> None: + """The cataloged anti-pattern is the ONE-bar chart, not the two-bar one. + + "Which of these two developers has been busier" is a real question; a + two-point line is a degenerate trend that should be a sentence. + """ + two = [["alice", 12], ["bob", 9]] + assert ( + validate_chart( + kind="bar", + title="t", + x="month", + y="commits", + columns=_COLUMNS, + rows=two, + )["null_rows"] + == 0 + ) + + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="line", title="t", x="month", y="commits", columns=_COLUMNS, rows=two + ) + assert "use `bar` to compare 2 values" in str(excinfo.value) + + +def test_a_single_row_is_refused_for_every_kind() -> None: + for kind in sorted(CHART_KINDS): + kwargs = { + "kind": kind, + "title": "t", + "x": "month", + "y": "commits", + "columns": _COLUMNS, + "rows": [["2026-03", 41]], + } + if kind in STACKED_KINDS: + kwargs.update( + series="kind", + columns=_STACK_COLUMNS, + rows=[["2026-03", "M", 40], ["2026-03", "A", 12]], + ) + with pytest.raises(ChartNotAllowed): + validate_chart(**kwargs) + + +# --------------------------------------------------------------------------- +# Rules 8-10 and densification — the stacked kinds +# --------------------------------------------------------------------------- + + +def test_a_sparse_stacked_result_is_densified_to_a_full_grid() -> None: + """The case that proves densification necessary: 14 rows over a 4x5 grid. + + `GROUP BY` omits empty groups, so six cells are simply absent. Leaving them + out would shift the segments above them down and mislabel every one. + """ + chart = _stack() + assert chart["x_values"] == ["2026-03", "2026-04", "2026-05", "2026-06"] + assert chart["series_values"] == ["M", "A", "D", "R", "C"] + assert chart["filled_cells"] == 6 + assert sum(len(cells) for cells in chart["cells"].values()) == 20 + # The specific holes, filled with 0 — not omitted, not None. + assert chart["cells"]["2026-04"]["D"] == 0 + assert chart["cells"]["2026-06"]["R"] == 0 + assert chart["cells"]["2026-06"]["C"] == 0 + + +def test_densification_changes_no_number() -> None: + """Filling absent cells with 0 must leave every per-x total untouched. + + This is the assertion that separates "recovered a fact" from "invented one". + """ + chart = _stack() + expected: dict[str, int] = {} + for month, _kind, n in _STACK_ROWS: + expected[month] = expected.get(month, 0) + n + for x_value, cells in chart["cells"].items(): + assert sum(cells.values()) == expected[x_value] + + +def test_a_present_null_y_is_refused_on_a_stacked_kind() -> None: + """The opposite of the absent-cell case, and deliberately so. + + An absent cell means the group was empty; a NULL aggregate means the value is + unknown. Drawing the second as zero makes the bar's total wrong, and the + total is what a stack is for. + """ + with pytest.raises(ChartNotAllowed) as excinfo: + _stack(rows=[*_STACK_ROWS[:-1], ["2026-06", "A", None]]) + message = str(excinfo.value) + assert "COALESCE" in message + assert "cannot show a hole" in message + # And it must say what the unstacked kinds would have done instead. + assert "gaps" in message + + +def test_the_same_null_is_a_gap_on_an_unstacked_kind() -> None: + """The two behaviours, side by side on the same data.""" + chart = validate_chart( + kind="bar", + title="t", + x="month", + y="n", + columns=_STACK_COLUMNS, + rows=[*_STACK_ROWS[:-1], ["2026-06", "A", None]], + ) + assert chart["null_rows"] == 1 + + +def test_series_is_required_on_a_stacked_kind() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + _stack(series=None) + message = str(excinfo.value) + # Names the unstacked kind to use instead, so the model has a way forward. + assert "use that kind instead" in message + assert "bar" in message + assert "GROUP BY" in message + + +def test_the_horizontal_stacked_kind_names_bar_h_as_its_fallback() -> None: + with pytest.raises(ChartNotAllowed, match="bar_h"): + _stack(kind="bar_h_stacked", series=None) + + +def test_series_is_refused_on_an_unstacked_kind() -> None: + """Ignoring it would draw a chart answering a different question.""" + with pytest.raises(ChartNotAllowed) as excinfo: + validate_chart( + kind="bar", + title="t", + x="month", + y="n", + series="kind", + columns=_STACK_COLUMNS, + rows=_STACK_ROWS, + ) + message = str(excinfo.value) + assert "only valid on a stacked kind" in message + assert "different question" in message + + +@pytest.mark.parametrize("collision", ["month", "n"]) +def test_series_cannot_reuse_the_x_or_y_column(collision: str) -> None: + with pytest.raises(ChartNotAllowed, match="third, different column"): + _stack(series=collision) + + +def test_an_unknown_series_column_names_what_was_available() -> None: + with pytest.raises(ChartNotAllowed) as excinfo: + _stack(series="change_type") + message = str(excinfo.value) + assert "unknown series column" in message + assert "'kind'" in message + + +def test_exactly_six_series_is_accepted() -> None: + """The boundary, from the palette's six validated slots.""" + rows = [[f"x{i}", f"s{j}", j + 1] for i in range(2) for j in range(MAX_SERIES)] + chart = _stack(columns=["x", "s", "n"], x="x", y="n", series="s", rows=rows) + assert len(chart["series_values"]) == MAX_SERIES + assert chart["filled_cells"] == 0 + + +def test_a_seventh_series_is_refused_and_says_to_fold_in_sql() -> None: + """The server never folds an 'other' bucket itself. + + Doing so would put a number on screen that no emitted query produced, which + is the one property this whole design exists to protect. + """ + rows = [[f"x{i}", f"s{j}", j + 1] for i in range(2) for j in range(MAX_SERIES + 1)] + with pytest.raises(ChartNotAllowed) as excinfo: + _stack(columns=["x", "s", "n"], x="x", y="n", series="s", rows=rows) + message = str(excinfo.value) + assert f"{MAX_SERIES + 1} distinct" in message + assert "'other'" in message + assert "in your SQL" in message + assert "CASE" in message + + +def test_series_order_is_descending_by_total_with_a_lexicographic_tie_break() -> None: + """Colour slot i is `series_values[i]`, so the order must be deterministic. + + First-appearance order would let a segment change colour between two renders + of the same data. The tie-break is what makes that reproducible. + """ + rows = [ + ["x1", "zulu", 10], + ["x1", "alpha", 10], # ties zulu on total — must sort first + ["x1", "mike", 30], + ["x2", "zulu", 1], + ["x2", "alpha", 1], + ["x2", "mike", 1], + ] + chart = _stack(columns=["x", "s", "n"], x="x", y="n", series="s", rows=rows) + assert chart["series_values"] == ["mike", "alpha", "zulu"] + + +def test_the_same_result_validated_twice_gives_the_same_colour_order() -> None: + assert _stack()["series_values"] == _stack()["series_values"] + + +def test_a_stacked_kind_counts_distinct_x_not_rows() -> None: + """`len(rows)` would let a one-bar chart through on multi-series data.""" + with pytest.raises(ChartNotAllowed) as excinfo: + _stack( + rows=[["2026-03", "M", 40], ["2026-03", "A", 12], ["2026-03", "D", 5]], + ) + message = str(excinfo.value) + assert "distinct 'month' value(s)" in message + assert "this result has 1" in message + + +def test_a_repeated_x_series_pair_is_refused() -> None: + """Two rows for one cell means the query did not group by both columns. + + Summing them, or taking the last, would be a decision the SQL never made. + """ + with pytest.raises(ChartNotAllowed) as excinfo: + _stack(rows=[*_STACK_ROWS, ["2026-03", "M", 99]]) + message = str(excinfo.value) + assert "more than once" in message + assert "GROUP BY both" in message + + +def test_the_x_axis_keeps_the_querys_own_order() -> None: + """The result's ORDER BY is the axis order — not a re-sort here.""" + chart = _stack( + rows=[ + ["2026-06", "M", 18], + ["2026-03", "M", 40], + ["2026-06", "A", 4], + ["2026-03", "A", 12], + ] + ) + assert chart["x_values"] == ["2026-06", "2026-03"] diff --git a/tests/test_chat_graph_stats.py b/tests/test_chat_graph_stats.py new file mode 100644 index 0000000..03a716e --- /dev/null +++ b/tests/test_chat_graph_stats.py @@ -0,0 +1,391 @@ +"""The CodeGraph statistics surface. + +Modelled on ``test_chat_stats_sql.py``'s layer structure, against a temporary +CodeGraph fixture rather than the live index — including its **byte-identical-DB** +assertion after every hostile query, which is the real proof of read-only. The +absence of an exception is not: a query can be refused and still have written. + +The fixture carries the shadow tables CodeGraph actually creates (``nodes_fts`` +and friends, ``project_metadata``, ``schema_versions``, ``unresolved_refs``) so +the deny-by-default property is tested against real table names rather than +invented ones. +""" + +from __future__ import annotations + +import hashlib +import sqlite3 +from pathlib import Path + +import pytest + +from whygraph.chat import graph_stats_sql +from whygraph.chat.graph_stats_sql import _ALLOWED_TABLES, run_graph_query +from whygraph.chat.sql_guard import _MAX_ROWS + +_SCHEMA = """ +CREATE TABLE nodes ( + id TEXT PRIMARY KEY, kind TEXT NOT NULL, name TEXT NOT NULL, + qualified_name TEXT NOT NULL, file_path TEXT NOT NULL, language TEXT NOT NULL, + start_line INTEGER NOT NULL, end_line INTEGER NOT NULL, + start_column INTEGER NOT NULL, end_column INTEGER NOT NULL, + docstring TEXT, signature TEXT, visibility TEXT, + is_exported INTEGER DEFAULT 0, is_async INTEGER DEFAULT 0, + is_static INTEGER DEFAULT 0, is_abstract INTEGER DEFAULT 0, + decorators TEXT, type_parameters TEXT, return_type TEXT, + updated_at INTEGER NOT NULL); +CREATE TABLE edges ( + id INTEGER PRIMARY KEY AUTOINCREMENT, source TEXT NOT NULL, target TEXT NOT NULL, + kind TEXT NOT NULL, metadata TEXT, line INTEGER, col INTEGER, provenance TEXT); +CREATE TABLE files ( + path TEXT PRIMARY KEY, content_hash TEXT NOT NULL, language TEXT NOT NULL, + size INTEGER NOT NULL, modified_at INTEGER NOT NULL, indexed_at INTEGER NOT NULL, + node_count INTEGER DEFAULT 0, errors TEXT); +-- The tables the allowlist must deny by default, under their real names. +CREATE TABLE project_metadata (key TEXT PRIMARY KEY, value TEXT); +CREATE TABLE schema_versions (version INTEGER PRIMARY KEY); +CREATE TABLE unresolved_refs (id INTEGER PRIMARY KEY, name TEXT); +CREATE TABLE name_segment_vocab (segment TEXT PRIMARY KEY); +CREATE VIRTUAL TABLE nodes_fts USING fts5(name, qualified_name, docstring); +""" + + +def _node(nid: str, kind: str, name: str, path: str, language: str = "python") -> tuple: + return ( + nid, + kind, + name, + f"pkg.{name}", + path, + language, + 1, + 10, + 0, + 0, + None, + None, + None, + 0, + 0, + 0, + 0, + None, + None, + None, + 1_750_000_000, + ) + + +@pytest.fixture +def graph_db(tmp_path: Path) -> Path: + """A small but structurally honest CodeGraph index. + + Deliberately includes ``import`` and ``file`` rows: those are the two traps + ``_GRAPH_SCHEMA_DOC`` rules 1 and 2 exist for, so a fixture without them + could not show that an unfiltered count is wrong. + """ + path = tmp_path / "codegraph.db" + conn = sqlite3.connect(path) + conn.executescript(_SCHEMA) + conn.executemany( + "INSERT INTO nodes VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", + [ + _node("n1", "function", "alpha", "src/pkg/a.py"), + _node("n2", "function", "beta", "src/pkg/a.py"), + _node("n3", "method", "gamma", "src/pkg/b.py"), + _node("n4", "class", "Delta", "src/pkg/b.py"), + _node("n5", "import", "os", "src/pkg/a.py"), + _node("n6", "import", "sys", "src/pkg/b.py"), + _node("n7", "file", "a.py", "src/pkg/a.py"), + _node("n8", "function", "epsilon", "tests/test_a.py"), + ], + ) + conn.executemany( + "INSERT INTO edges (source, target, kind) VALUES (?,?,?)", + [("n1", "n2", "calls"), ("n1", "n3", "calls"), ("n4", "n3", "contains")], + ) + conn.executemany( + "INSERT INTO files VALUES (?,?,?,?,?,?,?,?)", + [ + ("src/pkg/a.py", "h1", "python", 400, 1, 1, 4, None), + ("src/pkg/b.py", "h2", "python", 300, 1, 1, 2, None), + ("tests/test_a.py", "h3", "python", 100, 1, 1, 1, None), + ], + ) + conn.commit() + conn.close() + return path + + +# --------------------------------------------------------------------------- +# The statistics the tool exists to answer +# --------------------------------------------------------------------------- + + +def test_symbols_per_module(graph_db: Path) -> None: + """The question `run_project_stats` structurally cannot answer.""" + result = run_graph_query( + "SELECT substr(file_path, 1, instr(file_path, '/') - 1) AS module, " + "count(*) AS n FROM nodes " + "WHERE kind IN ('function','method','class') " + "GROUP BY module ORDER BY n DESC", + db_path=graph_db, + ) + assert result["columns"] == ["module", "n"] + assert result["rows"] == [["src", 4], ["tests", 1]] + assert result["truncated"] is False + + +def test_the_kind_filter_is_what_makes_a_count_honest(graph_db: Path) -> None: + """Rule 1, demonstrated rather than asserted on the doc text. + + Unfiltered, imports and the file-as-node inflate the count by half — the + trap `_GRAPH_SCHEMA_DOC` rule 1 exists to name. + """ + unfiltered = run_graph_query("SELECT count(*) AS n FROM nodes", db_path=graph_db) + definitions = run_graph_query( + "SELECT count(*) AS n FROM nodes WHERE kind IN " + "('function','method','class','interface','route','constant'," + "'type_alias','component')", + db_path=graph_db, + ) + assert unfiltered["rows"] == [[8]] + assert definitions["rows"] == [[5]] + + +def test_call_fan_out_joins_edges_to_nodes(graph_db: Path) -> None: + """Rule 4: `edges.source` is an id, so a readable answer needs the join.""" + result = run_graph_query( + "SELECT n.name AS symbol, count(*) AS calls " + "FROM edges e JOIN nodes n ON n.id = e.source " + "WHERE e.kind = 'calls' GROUP BY n.id ORDER BY calls DESC", + db_path=graph_db, + ) + assert result["rows"] == [["alpha", 2]] + + +def test_language_mix_from_the_files_table(graph_db: Path) -> None: + result = run_graph_query( + "SELECT language, count(*) AS n FROM files GROUP BY language", + db_path=graph_db, + ) + assert result["rows"] == [["python", 3]] + + +# --------------------------------------------------------------------------- +# Layer 3 — the shape check +# --------------------------------------------------------------------------- + + +def test_a_non_aggregate_select_is_refused(graph_db: Path) -> None: + """Aggregate-only is what stops this surface dumping source-derived text. + + `nodes.docstring` and `nodes.signature` hold source, so a record-shaped + read here would be a second `read_file` without its clamps. + """ + result = run_graph_query("SELECT * FROM nodes", db_path=graph_db) + assert result["layer"] == "shape" + assert "aggregate" in result["error"] + + +def test_docstrings_cannot_be_dumped_row_by_row(graph_db: Path) -> None: + result = run_graph_query( + "SELECT name, docstring FROM nodes LIMIT 50", db_path=graph_db + ) + assert result["layer"] == "shape" + + +# --------------------------------------------------------------------------- +# Layer 2 — the authorizer, on this surface's allowlist +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "table", + ["nodes_fts", "project_metadata", "schema_versions", "unresolved_refs"], +) +def test_reads_outside_the_allowlist_are_refused_by_name( + graph_db: Path, table: str +) -> None: + """Deny-by-default: these are never listed anywhere, only omitted.""" + assert table not in _ALLOWED_TABLES + result = run_graph_query(f"SELECT count(*) AS n FROM {table}", db_path=graph_db) + assert result["layer"] == "authorizer" + assert table in result["error"] + # The refusal says which database it reached — with two stats tools, that is + # the difference between "wrong table" and "wrong tool". + assert "CodeGraph database" in result["error"] + + +def test_a_denied_table_cannot_be_smuggled_in_through_a_subquery( + graph_db: Path, +) -> None: + result = run_graph_query( + "SELECT count(*) AS n FROM nodes WHERE id IN " + "(SELECT value FROM project_metadata)", + db_path=graph_db, + ) + assert result["layer"] == "authorizer" + assert "project_metadata" in result["error"] + + +@pytest.mark.parametrize( + "sql", + [ + "DROP TABLE nodes", + "DELETE FROM edges", + "UPDATE nodes SET name = 'x'", + "INSERT INTO nodes (id) VALUES ('x')", + "ALTER TABLE nodes RENAME TO gone", + "CREATE TABLE t (x)", + "ATTACH DATABASE '/tmp/whygraph-graph-evil.db' AS evil", + "PRAGMA journal_mode = delete", + ], +) +def test_non_read_actions_are_refused_inside_the_vm(graph_db: Path, sql: str) -> None: + """Driven through `_connect` so the string shape check is not load-bearing.""" + conn = graph_stats_sql._connect(graph_db) + try: + with pytest.raises(sqlite3.DatabaseError, match="not authorized"): + conn.execute(sql) + finally: + conn.close() + + +def test_with_recursive_is_refused(graph_db: Path) -> None: + """`SQLITE_RECURSIVE` is not in the action allowlist, deliberately.""" + result = run_graph_query( + "WITH RECURSIVE walk(id) AS (" + " SELECT 'n1' UNION SELECT e.target FROM edges e JOIN walk ON walk.id = e.source" + ") SELECT count(*) AS n FROM walk", + db_path=graph_db, + ) + assert result["layer"] == "authorizer" + + +@pytest.mark.parametrize("fn", ["load_extension", "readfile", "writefile"]) +def test_dangerous_functions_are_refused_by_name(graph_db: Path, fn: str) -> None: + conn = graph_stats_sql._connect(graph_db) + try: + with pytest.raises(sqlite3.DatabaseError): + conn.execute(f"SELECT {fn}('/etc/passwd')").fetchone() + finally: + conn.close() + + +# --------------------------------------------------------------------------- +# Layer 1 — the file is never written +# --------------------------------------------------------------------------- + + +def test_the_index_is_byte_identical_after_every_hostile_query( + graph_db: Path, +) -> None: + """The whole point, asserted on the bytes rather than on an exception.""" + before = hashlib.sha256(graph_db.read_bytes()).hexdigest() + for sql in [ + "DROP TABLE nodes", + "DELETE FROM edges", + "UPDATE nodes SET name = 'x'", + "INSERT INTO files (path) VALUES ('x')", + "ATTACH DATABASE '/tmp/whygraph-graph-evil.db' AS evil", + "PRAGMA journal_mode = delete", + "SELECT count(*) FROM project_metadata", + "SELECT * FROM nodes", + ]: + run_graph_query(sql, db_path=graph_db) + assert hashlib.sha256(graph_db.read_bytes()).hexdigest() == before + + +# --------------------------------------------------------------------------- +# Layer 4 — output caps +# --------------------------------------------------------------------------- + + +def test_the_row_cap_is_enforced_and_flagged(graph_db: Path) -> None: + conn = sqlite3.connect(graph_db) + conn.executemany( + "INSERT INTO nodes VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", + [_node(f"p{n}", "function", f"f{n}", f"src/pkg/p{n}.py") for n in range(250)], + ) + conn.commit() + conn.close() + + result = run_graph_query( + "SELECT file_path, count(*) AS n FROM nodes GROUP BY file_path", + db_path=graph_db, + ) + assert result["truncated"] is True + assert result["row_count"] == _MAX_ROWS + + +# --------------------------------------------------------------------------- +# Degradation, not failure +# --------------------------------------------------------------------------- + + +def test_a_missing_index_degrades_instead_of_raising(tmp_path: Path) -> None: + """The WhyGraph and file tools still work without an index.""" + result = run_graph_query( + "SELECT count(*) AS n FROM nodes", db_path=tmp_path / "nope.db" + ) + assert result["layer"] == "connection" + assert result["error"] == graph_stats_sql._NO_CODEGRAPH + assert "whygraph scan" in result["error"] + + +# --------------------------------------------------------------------------- +# The allowlist and the schema doc +# --------------------------------------------------------------------------- + + +def test_the_allowlist_is_a_literal_not_derived_from_the_schema() -> None: + """Risk 16: CodeGraph's schema is upstream and can gain tables. + + A derived allowlist would admit a new upstream table on a version bump, + silently. This one is inert until someone edits the literal. + """ + assert isinstance(_ALLOWED_TABLES, frozenset) + assert _ALLOWED_TABLES == {"nodes", "edges", "files"} + source = Path(graph_stats_sql.__file__).read_text() + # The literal, verbatim — and no query that could reach the schema table. + # ("sqlite_master" appears in the module docstring, saying not to use it.) + assert '_ALLOWED_TABLES = frozenset({"nodes", "edges", "files"})' in source + assert "FROM sqlite_master" not in source + assert "sqlite_master" not in graph_stats_sql._ALLOWED_TABLES + + +def test_the_graph_schema_doc_carries_its_five_rules() -> None: + """Each rule is a trap that produces a *plausible* wrong number.""" + doc = graph_stats_sql._GRAPH_SCHEMA_DOC + # Rule 1 — imports outnumber functions, so an unfiltered count is inflated. + assert "kind IN ('function','method','class','interface','route','constant'," in doc + assert "imports OUTNUMBER functions" in doc + # Rule 2 — files-as-nodes double-count. + assert "kind = 'file'" in doc + # Rule 3 — directory grouping produces junk buckets; '_' is a LIKE wildcard. + assert "truncated fragment" in doc + assert "'_' is a LIKE" in doc + # Rule 4 — edges hold ids, not names. + assert "are `nodes.id`, not names" in doc + # Rule 5 — tests are the largest module, which is true and misleading. + assert "TESTS ARE CODE TOO" in doc + # The two-database boundary, which is the new failure mode. + assert "DIFFERENT DATABASE from run_project_stats" in doc + assert "no history" in doc + # Every readable table is documented. + for table in _ALLOWED_TABLES: + assert table in doc + # An empty table is an index gap, not a finding. + assert "the index has not been built" in doc + + +def test_the_graph_schema_doc_ships_the_measured_cardinalities() -> None: + """Measured, not assumed — so the model need not discover them by query.""" + doc = graph_stats_sql._GRAPH_SCHEMA_DOC + assert "function 1288, import 1111" in doc + assert "contains 3128, calls 2618" in doc + assert "python 182, tsx 23" in doc + # `updated_at` is an index timestamp, and reading it as a commit date is the + # single most likely two-database confusion. + assert "INDEX timestamp, NOT a commit date" in doc diff --git a/tests/test_chat_harness.py b/tests/test_chat_harness.py index ee67e8b..949d58e 100644 --- a/tests/test_chat_harness.py +++ b/tests/test_chat_harness.py @@ -421,7 +421,7 @@ def test_specs_and_bounds_come_from_config_by_default( def test_registry_specs_are_offered_to_the_model() -> None: - """A real registry's 15 specs reach the request.""" + """A real registry's 17 specs reach the request.""" client = ScriptedClient([[TextDelta(text="hi"), TurnDone("stop")]]) registry = ToolRegistry(max_rationale_generations=0) list( @@ -432,7 +432,7 @@ def test_registry_specs_are_offered_to_the_model() -> None: max_tool_rounds=1, ) ) - assert len(client.requests[0].tools) == 15 + assert len(client.requests[0].tools) == 17 # --------------------------------------------------------------------------- diff --git a/tests/test_chat_sql_guard.py b/tests/test_chat_sql_guard.py new file mode 100644 index 0000000..b8ecf29 --- /dev/null +++ b/tests/test_chat_sql_guard.py @@ -0,0 +1,201 @@ +"""The shared SQL guard, tested for **surface isolation**. + +``tests/test_chat_stats_sql.py`` already proves each of the four layers works; +those tests pass unchanged across the extraction, which is what makes it +behaviour-preserving. What they *cannot* prove is the property the extraction +newly creates: that two surfaces do not share one allowlist. + +A cross-wired allowlist is the refactor's worst plausible outcome, and it is +invisible — every existing test still passes, because both surfaces would allow a +superset of what each needs. So the load-bearing test here is the negative one: an +authorizer built for surface A **refuses** a table only surface B allows. +""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +import pytest + +from whygraph.chat import graph_stats_sql, sql_guard, stats_sql +from whygraph.chat.sql_guard import SqlNotAllowed, SqlSurface + +# --------------------------------------------------------------------------- +# Two toy surfaces over one file, differing only in their allowlist +# --------------------------------------------------------------------------- + + +@pytest.fixture +def two_table_db(tmp_path: Path) -> Path: + """A database holding `alpha` and `beta`, so either can be the denied one.""" + path = tmp_path / "two.db" + conn = sqlite3.connect(path) + conn.executescript( + """ + CREATE TABLE alpha (n INTEGER); + CREATE TABLE beta (n INTEGER); + INSERT INTO alpha (n) VALUES (1), (2); + INSERT INTO beta (n) VALUES (3); + """ + ) + conn.commit() + conn.close() + return path + + +def _surface(label: str, tables: set[str], path: Path) -> SqlSurface: + return SqlSurface( + label=label, + allowed_tables=frozenset(tables), + db_path=lambda: path, + missing_db_message="{db_path} is missing", + ) + + +def test_a_surface_refuses_a_table_only_the_other_surface_allows( + two_table_db: Path, +) -> None: + """The cross-wiring guard, in both directions. + + Asserting only one direction would pass against an authorizer that had + accidentally been given the *union* of both allowlists. + """ + only_alpha = _surface("Alpha", {"alpha"}, two_table_db) + only_beta = _surface("Beta", {"beta"}, two_table_db) + + ok = sql_guard.run_aggregate_query("SELECT count(*) AS n FROM alpha", only_alpha) + assert ok["rows"] == [[2]] + refused = sql_guard.run_aggregate_query( + "SELECT count(*) AS n FROM beta", only_alpha + ) + assert refused["layer"] == "authorizer" + assert "table beta" in refused["error"] + + ok = sql_guard.run_aggregate_query("SELECT count(*) AS n FROM beta", only_beta) + assert ok["rows"] == [[1]] + refused = sql_guard.run_aggregate_query( + "SELECT count(*) AS n FROM alpha", only_beta + ) + assert refused["layer"] == "authorizer" + assert "table alpha" in refused["error"] + + +def test_the_authorizer_closes_over_its_own_allowlist_not_a_global( + two_table_db: Path, +) -> None: + """Two live connections, two allowlists, at the same time. + + If the allowlist were still a module global the second ``connect`` would + silently retune the first — the failure mode a sequential test cannot see. + """ + conn_a = _surface("Alpha", {"alpha"}, two_table_db).connect() + conn_b = _surface("Beta", {"beta"}, two_table_db).connect() + try: + assert conn_a.execute("SELECT count(*) FROM alpha").fetchone() == (2,) + assert conn_b.execute("SELECT count(*) FROM beta").fetchone() == (1,) + with pytest.raises(sqlite3.DatabaseError): + conn_a.execute("SELECT count(*) FROM beta").fetchone() + with pytest.raises(sqlite3.DatabaseError): + conn_b.execute("SELECT count(*) FROM alpha").fetchone() + finally: + conn_a.close() + conn_b.close() + + +def test_a_refusal_names_the_database_it_reached(two_table_db: Path) -> None: + """`label` exists so a wrong-tool query is diagnosable. + + With two stats tools, "may read only nodes, edges, files" is ambiguous about + *which* database refused; the label removes the guess. + """ + result = sql_guard.run_aggregate_query( + "SELECT count(*) AS n FROM beta", _surface("Alpha", {"alpha"}, two_table_db) + ) + assert "Alpha database" in result["error"] + + +def test_the_missing_db_message_is_per_surface(tmp_path: Path) -> None: + """The remedy differs per surface: run a scan, versus build an index.""" + surface = _surface("Alpha", {"alpha"}, tmp_path / "nope.db") + result = sql_guard.run_aggregate_query("SELECT count(*) FROM alpha", surface) + assert result["layer"] == "connection" + assert result["error"].endswith("nope.db is missing") + + +# --------------------------------------------------------------------------- +# The layers are shared, so each is exercised once here too +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "sql", + [ + "DROP TABLE alpha", + "DELETE FROM alpha", + "PRAGMA journal_mode = delete", + "ATTACH DATABASE '/tmp/whygraph-guard-evil.db' AS evil", + ], +) +def test_non_read_actions_are_refused_on_any_surface( + two_table_db: Path, sql: str +) -> None: + conn = _surface("Alpha", {"alpha"}, two_table_db).connect() + try: + with pytest.raises(sqlite3.DatabaseError, match="not authorized"): + conn.execute(sql) + finally: + conn.close() + + +def test_recursive_is_not_in_the_action_allowlist() -> None: + """`WITH RECURSIVE` stays unavailable — three action codes, no more. + + Preserved verbatim across the move: the exclusion is easy to lose in a + refactor and impossible to notice afterwards. + """ + assert sqlite3.SQLITE_RECURSIVE not in sql_guard._ALLOWED_ACTIONS + assert sql_guard._ALLOWED_ACTIONS == { + sqlite3.SQLITE_SELECT, + sqlite3.SQLITE_READ, + sqlite3.SQLITE_FUNCTION, + } + + +def test_the_shape_check_is_shared_and_still_demands_an_aggregate() -> None: + with pytest.raises(SqlNotAllowed) as excinfo: + sql_guard._check_shape("SELECT * FROM alpha") + assert excinfo.value.layer == "shape" + + with pytest.raises(SqlNotAllowed) as excinfo: + sql_guard._check_shape("SELECT count(*) FROM alpha; DROP TABLE alpha") + assert excinfo.value.layer == "shape" + + assert sql_guard._check_shape(" SELECT count(*) FROM alpha ; ") == ( + "SELECT count(*) FROM alpha" + ) + + +# --------------------------------------------------------------------------- +# Both real surfaces, in one place +# --------------------------------------------------------------------------- + + +def test_both_shipped_allowlists_are_frozenset_literals() -> None: + """Risk 16: a schema-derived allowlist widens itself on an upstream bump.""" + assert isinstance(stats_sql._ALLOWED_TABLES, frozenset) + assert isinstance(graph_stats_sql._ALLOWED_TABLES, frozenset) + # Disjoint, so a cross-wiring bug cannot hide behind an overlap. + assert not (stats_sql._ALLOWED_TABLES & graph_stats_sql._ALLOWED_TABLES) + # The names each surface must never reach. + for denied in ("chat_message", "chat_session", "rationale_cache"): + assert denied not in stats_sql._ALLOWED_TABLES + assert denied not in graph_stats_sql._ALLOWED_TABLES + + +def test_the_shipped_surfaces_carry_their_own_labels_and_paths() -> None: + assert stats_sql._SURFACE.label == "WhyGraph" + assert graph_stats_sql._SURFACE.label == "CodeGraph" + # `db_path` is deferred — a callable, not a resolved Path bound at import. + assert callable(stats_sql._SURFACE.db_path) + assert callable(graph_stats_sql._SURFACE.db_path) diff --git a/tests/test_chat_stats_sql.py b/tests/test_chat_stats_sql.py index b8d2550..7a02a45 100644 --- a/tests/test_chat_stats_sql.py +++ b/tests/test_chat_stats_sql.py @@ -24,7 +24,7 @@ import pytest -from whygraph.chat import stats_sql +from whygraph.chat import sql_guard, stats_sql from whygraph.chat.stats_sql import _ALLOWED_TABLES, _MAX_ROWS, run_stats_query from whygraph.db import get_session from whygraph.db.models import Commit, CommitFileChange, PullRequest @@ -320,8 +320,11 @@ def test_a_runaway_query_is_aborted_and_returns_an_error_result( statement — no worker thread, no ``interrupt()`` call, and no signal handler (which would be wrong inside FastAPI's threadpool). """ - monkeypatch.setattr(stats_sql, "_TIMEOUT_SEC", 0.05) - monkeypatch.setattr(stats_sql, "_PROGRESS_INTERVAL", 100) + # Layer 4 lives in `sql_guard` (shared with the CodeGraph surface), so that + # is where the deadline is read from — patching a re-export here would not + # reach the code under test. + monkeypatch.setattr(sql_guard, "_TIMEOUT_SEC", 0.05) + monkeypatch.setattr(sql_guard, "_PROGRESS_INTERVAL", 100) # Enough rows that a 6-way cartesian product cannot finish in 50ms. with get_session() as session: for n in range(60): diff --git a/tests/test_chat_tools.py b/tests/test_chat_tools.py index 988d253..8b82f91 100644 --- a/tests/test_chat_tools.py +++ b/tests/test_chat_tools.py @@ -125,10 +125,10 @@ def _result(registry: ToolRegistry, name: str, **arguments) -> dict: # --------------------------------------------------------------------------- -def test_fifteen_tools_with_unique_names_and_object_schemas() -> None: +def test_seventeen_tools_with_unique_names_and_object_schemas() -> None: names = [spec.name for spec in TOOL_SPECS] - assert len(names) == 15 - assert len(set(names)) == 15 + assert len(names) == 17 + assert len(set(names)) == 17 assert set(names) == { "search_symbols", "get_symbol", @@ -143,6 +143,8 @@ def test_fifteen_tools_with_unique_names_and_object_schemas() -> None: "get_repo_overview", "list_recent_activity", "run_project_stats", + "run_graph_stats", + "render_chart", "read_file", "list_dir", } @@ -1358,3 +1360,259 @@ def test_find_changes_on_an_unscanned_db_is_a_result_not_an_exception( registry = ToolRegistry(max_rationale_generations=0) result = _result(registry, "find_changes", query="anything") assert "scan" in result["error"] + + +# --------------------------------------------------------------------------- +# Charting — ref minting, and the payload discipline the split design buys +# --------------------------------------------------------------------------- + + +_CHART_RESULT = { + "sql": "SELECT strftime('%Y-%m', authored_at) AS month, count(*) AS commits …", + "columns": ["month", "commits"], + "rows": [["2026-03", 41], ["2026-04", 38], ["2026-05", 52]], + "row_count": 3, + "truncated": False, +} + + +def _minted(**overrides) -> tuple[ToolRegistry, dict]: + """Run one synthetic aggregate result through the minting policy.""" + registry = ToolRegistry(max_rationale_generations=0) + result = {**_CHART_RESULT, **overrides} + return registry, registry._mint_chart_ref(result) + + +def test_a_chartable_result_carries_an_opaque_ref_and_an_invitation() -> None: + """`columns` and `chart_ref` arrive together — the defense against + hallucinated column names, since the names are on screen in the message + immediately before the render_chart call.""" + _, result = _minted() + assert result["chart_ref"].startswith("cr_") + assert result["columns"] == ["month", "commits"] + assert "render_chart" in result["chartable"] + + +@pytest.mark.parametrize( + ("label", "overrides"), + [ + ("errored", {"error": "nope", "layer": "shape"}), + ("truncated", {"truncated": True}), + ("one row", {"rows": [["2026-03", 41]], "row_count": 1}), + ("no rows", {"rows": [], "row_count": 0}), + ], +) +def test_no_ref_is_minted_for_a_result_not_worth_charting( + label: str, overrides: dict +) -> None: + """Withholding the affordance beats refusing later. + + A truncated result is the important one: the tool cannot know the true total, + so a chart drawn from it would be a confident lie. + """ + _, result = _minted(**overrides) + assert "chart_ref" not in result, label + assert "chartable" not in result, label + + +def test_two_rows_are_chartable_because_a_two_bar_chart_is_legitimate() -> None: + """The minting floor is the cheapest kind's minimum, not a flat 3. + + A higher floor would make `bar` unreachable on a two-row result — "which of + these two developers has been busier" is a real question. + """ + _, result = _minted(rows=[["alice", 12], ["bob", 9]], row_count=2) + assert "chart_ref" in result + + +def test_refs_are_unique_unguessable_and_not_sequential() -> None: + """Risk 18: a ref must not address anything the model did not just compute.""" + registry = ToolRegistry(max_rationale_generations=0) + refs = [ + registry._mint_chart_ref(dict(_CHART_RESULT))["chart_ref"] for _ in range(8) + ] + assert len(set(refs)) == 8 + for index, ref in enumerate(refs): + assert len(ref) == len("cr_") + 8 # token_hex(4) + assert all(char in "0123456789abcdef" for char in ref[3:]) + # A counter would be guessable, and would let the model address a result + # from a turn it never saw. + assert ref != f"cr_{index:08x}" + + +def test_render_chart_returns_the_directive_but_never_the_rows() -> None: + """§5.4's payload discipline, and the answer to risk 1. + + The frontend already has the rows from the producer's result and correlates + on `chart_ref`, so echoing them would double a 200-row payload for nothing — + and truncation mid-array is what makes an oversized payload unparseable. + """ + registry, produced = _minted() + result = _result( + registry, + "render_chart", + chart_ref=produced["chart_ref"], + kind="line", + title="Commits per month", + x="month", + y="commits", + ) + assert result["chart"]["x_index"] == 0 + assert result["chart"]["y_index"] == 1 + assert result["columns"] == ["month", "commits"] + assert result["row_count"] == 3 + assert "rows" not in result + + +def test_an_unknown_ref_is_a_readable_error_not_an_exception() -> None: + registry = ToolRegistry(max_rationale_generations=0) + result = _result( + registry, + "render_chart", + chart_ref="cr_deadbeef", + kind="bar", + title="t", + x="month", + y="commits", + ) + assert result["layer"] == "ref" + assert "Re-run the query" in result["error"] + + +def test_a_ref_from_another_registry_is_rejected() -> None: + """Refs die with the turn, which is what makes `build_window`'s eliding of + older tool results irrelevant to charting rather than something to work + around.""" + _, produced = _minted() + other = ToolRegistry(max_rationale_generations=0) + result = _result( + other, + "render_chart", + chart_ref=produced["chart_ref"], + kind="bar", + title="t", + x="month", + y="commits", + ) + assert result["layer"] == "ref" + + +def test_a_bad_column_is_a_chart_error_naming_the_available_ones() -> None: + registry, produced = _minted() + result = _result( + registry, + "render_chart", + chart_ref=produced["chart_ref"], + kind="bar", + title="t", + x="week", + y="commits", + ) + assert result["layer"] == "chart" + assert "'month'" in result["error"] and "'commits'" in result["error"] + + +def test_a_stacked_directive_round_trips_through_dispatch() -> None: + """The whole stacked path, as the model actually reaches it.""" + registry = ToolRegistry(max_rationale_generations=0) + produced = registry._mint_chart_ref( + { + "columns": ["month", "kind", "n"], + "rows": [ + ["2026-03", "M", 40], + ["2026-03", "A", 12], + ["2026-04", "M", 30], + ], + "row_count": 3, + "truncated": False, + } + ) + result = _result( + registry, + "render_chart", + chart_ref=produced["chart_ref"], + kind="bar_stacked", + title="Changes per month by type", + x="month", + y="n", + series="kind", + ) + chart = result["chart"] + assert chart["series_values"] == ["M", "A"] + assert chart["cells"]["2026-04"]["A"] == 0 # densified, and counted: + assert chart["filled_cells"] == 1 + + +def test_run_project_stats_has_no_chart_parameter() -> None: + """There is exactly one way to draw a chart. + + An earlier design put `chart` on the producer; it was removed so charting is + a capability rather than a parameter that every future producer must grow. + """ + for name in ("run_project_stats", "run_graph_stats"): + spec = next(s for s in TOOL_SPECS if s.name == name) + assert set(spec.parameters["properties"]) == {"sql"} + + +def test_the_render_chart_spec_tells_the_model_to_name_columns() -> None: + """A wording guard, mirroring `_DESCRIPTION_AUTHORITY`. + + "Name columns, never values" is the one instruction that keeps a chart + traceable to the query that produced it. + """ + spec = next(s for s in TOOL_SPECS if s.name == "render_chart") + assert "COLUMNS OF THAT RESULT" in spec.description + assert "never retype the values" in spec.description + # One measure, and the breakdown routed to `series` rather than a second y. + assert "two MEASURES means two charts" in spec.description + assert "a breakdown is `series`, not a second y" in spec.description + # The kind enum is the registered set, not a prose list that could drift. + assert set(spec.parameters["properties"]["kind"]["enum"]) == set( + chat_tools.charts.CHART_KINDS + ) + assert set(spec.parameters["required"]) == {"chart_ref", "kind", "title", "x", "y"} + + +def test_the_graph_stats_spec_ships_its_own_schema_doc() -> None: + """The two stats tools must not share a description — the whole risk is that + the model confuses one database for the other.""" + from whygraph.chat.graph_stats_sql import _GRAPH_SCHEMA_DOC + from whygraph.chat.stats_sql import _SCHEMA_DOC + + spec = next(s for s in TOOL_SPECS if s.name == "run_graph_stats") + assert spec.description == _GRAPH_SCHEMA_DOC + assert spec.description != _SCHEMA_DOC + assert "DIFFERENT DATABASE from run_project_stats" in spec.description + + +def test_a_charted_result_stays_parseable_at_the_row_cap() -> None: + """Risk 1, measured rather than estimated. + + Length alone is not the assertion — `_encode` slices mid-array and appends a + marker, so the defect is unparseable JSON, not size. This is the shape that + once shipped broken on `find_changes`. + """ + long_path = "src/whygraph/playground/src/components/chat/ToolCallCard.tsx" + registry = ToolRegistry(max_rationale_generations=0) + produced = registry._mint_chart_ref( + { + "columns": ["path", "changes"], + "rows": [[f"{long_path}#{n}", n] for n in range(200)], + "row_count": 200, + "truncated": False, + } + ) + raw = registry.dispatch( + "render_chart", + { + "chart_ref": produced["chart_ref"], + "kind": "bar_h", + "title": "Files changed most often", + "x": "path", + "y": "changes", + }, + ) + assert not raw.endswith(TRUNCATION_MARKER) + assert len(raw) < MAX_RESULT_CHARS + result = json.loads(raw) # the assertion that actually matters + assert result["row_count"] == 200 From 869cb8908f1a408d62f3019a737cfb72a42c4a01 Mon Sep 17 00:00:00 2001 From: cvetty Date: Fri, 31 Jul 2026 15:56:00 +0300 Subject: [PATCH 3/3] feat(serve): render charts in the chat transcript (frontend + eval) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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). --- scripts/eval_tool_choice.py | 194 ++++++- src/playground/package-lock.json | 59 +++ src/playground/package.json | 2 + .../src/components/chat/ChartBlock.tsx | 495 ++++++++++++++++++ .../src/components/chat/MessageBubble.tsx | 46 +- .../src/components/chat/chartSpec.ts | 170 ++++++ src/playground/src/components/chat/echarts.ts | 40 ++ 7 files changed, 988 insertions(+), 18 deletions(-) create mode 100644 src/playground/src/components/chat/ChartBlock.tsx create mode 100644 src/playground/src/components/chat/chartSpec.ts create mode 100644 src/playground/src/components/chat/echarts.ts diff --git a/scripts/eval_tool_choice.py b/scripts/eval_tool_choice.py index db2e3ce..8637771 100644 --- a/scripts/eval_tool_choice.py +++ b/scripts/eval_tool_choice.py @@ -7,7 +7,23 @@ selection is the model's judgement, given the descriptions. So this drives live providers through the real HTTP surface, records the ordered -``tool_call`` names from the SSE stream, and prints a matrix. +``tool_call`` names **and arguments** from the SSE stream, and prints a matrix. + +Two stats tools over two databases moved the baseline, and added a failure mode +no single-tool eval could have: the model reaching the *wrong database*. So there +are now two kinds of check here, reported separately because they need different +fixes: + +* **Selection** — which tool it opened with, and whether a structural question + leaked into the history DB or vice versa. +* **Obedience** — whether the tool *description* was followed, read off the + emitted arguments. Did the identity query resolve through the ``author`` table + rather than grouping a raw git identity (and without ``json_each``, which the + authorizer denies, or a ``LIKE`` against the emails array, which silently + misattributes commits)? Did ``render_chart`` follow its producer and name + columns rather than retyping values? Did a breakdown become one stacked chart + rather than two? A miss here means the wording needs work, not that the + feature is broken. **Tool choice is per-model behaviour**, so a result from one model is evidence about that model and nothing else. That is why targets are plural: run every @@ -117,12 +133,54 @@ "What's the average time from PR open to merge?", ("run_project_stats",), ), + # The two-database boundary — the failure mode a second stats tool creates. + # A structural question must not reach the history DB and vice versa, and no + # amount of SQL cleverness in either can cover for the other. + ( + "graph-stats", + "Which modules have the most functions?", + ("run_graph_stats",), + ), + ( + "graph-stats", + "What is the distribution of symbol kinds in this codebase?", + ("run_graph_stats",), + ), + # Identity: the one case that checks a _SCHEMA_DOC rule was OBEYED rather + # than merely present. Possible only because rule 5 is mechanical. + ( + "identity", + "Which developer has been working the most lately?", + ("run_project_stats",), + ), + ( + "identity", + "Who are the top contributors?", + ("run_project_stats",), + ), + # Charting: two rounds, in order, within the round limit. + ( + "chart", + "How has commit volume changed month over month? Show me a chart.", + ("run_project_stats",), + ), + ( + "chart", + "Break file changes down by change type per month, as a chart.", + ("run_project_stats",), + ), ) _STATS_TOOL = "run_project_stats" +_GRAPH_STATS_TOOL = "run_graph_stats" +_CHART_TOOL = "render_chart" _CODEGRAPH_TOOLS = ("get_area_outline", "search_symbols", "get_symbol") _FILE_TOOLS = ("read_file", "list_dir") +# Classes whose questions are about history, so `run_graph_stats` reaching them +# is the two-database confusion rather than a harmless extra call. +_HISTORY_CLASSES = frozenset({"statistics", "identity", "chart", "debugging"}) + def _first_index(names: list[str], wanted: tuple[str, ...]) -> float: """Position of the first name in ``wanted``, or infinity if absent. @@ -147,8 +205,16 @@ def _post(url: str, payload: dict | None) -> object: return json.loads(response.read()) -def _stream_tool_calls(base: str, session_id: int, question: str) -> tuple[list, str]: - """Send one turn and return its ordered tool names plus any error frame.""" +def _stream_tool_calls( + base: str, session_id: int, question: str +) -> tuple[list, list, str]: + """Send one turn and return its tool names, the full calls, and any error. + + The **arguments** matter as much as the names now: a chart case has to check + that `render_chart` named columns rather than retyping values, and the + identity case has to check the emitted SQL resolved identity through the + `author` table. Neither is visible in a list of tool names. + """ body = json.dumps({"content": question}).encode() request = urllib.request.Request( f"{base}/api/chat/sessions/{session_id}/messages", @@ -156,6 +222,7 @@ def _stream_tool_calls(base: str, session_id: int, question: str) -> tuple[list, headers={"Content-Type": "application/json"}, ) names: list[str] = [] + calls: list[dict] = [] error = "" with urllib.request.urlopen(request) as response: # noqa: S310 -- localhost only for raw in response: @@ -165,9 +232,77 @@ def _stream_tool_calls(base: str, session_id: int, question: str) -> tuple[list, frame = json.loads(line[5:].strip()) if frame.get("type") == "tool_call": names.append(frame["name"]) + calls.append( + {"name": frame["name"], "arguments": frame.get("arguments") or {}} + ) elif frame.get("type") == "error": error = frame.get("message", "unknown error") - return names, error + return names, calls, error + + +def _obedience_notes(klass: str, calls: list[dict]) -> list[str]: + """Per-class checks that a *name* cannot express. + + These are the ones that measure whether a tool *description* was obeyed, not + just which tool was picked. A miss here means the wording needs work — it does + not mean the feature is broken, which is why they are reported separately from + the first-tool verdict. + """ + notes: list[str] = [] + sql = " ".join( + str(call["arguments"].get("sql", "")) + for call in calls + if call["name"] in (_STATS_TOOL, _GRAPH_STATS_TOOL) + ).lower() + charts = [call for call in calls if call["name"] == _CHART_TOOL] + + if klass == "identity" and sql: + # Rule 5: identity comes from the `author` table, never from a raw git + # identity — one human routinely has several. + if "author " not in sql and "author\n" not in sql and " author" not in sql: + notes.append("SQL never mentions the author table") + if "group by" in sql and ( + "group by c.author_name" in sql + or "group by author_name" in sql + or "group by c.author_email" in sql + or "group by author_email" in sql + ): + notes.append("grouped by a RAW git identity (rule 5 disobeyed)") + # The two forms the rule explicitly forbids: one is denied by the + # authorizer, the other silently misattributes commits. + if "json_each" in sql: + notes.append("used json_each (denied by the authorizer)") + if "like" in sql and "a.emails" in sql: + notes.append("LIKE against author.emails (false-merges on '_')") + + if klass == "chart": + if not charts: + notes.append("no render_chart call") + else: + spec = charts[0]["arguments"] + # The whole contract: columns, never values. + for key in ("x", "y"): + if not isinstance(spec.get(key), str): + notes.append(f"{key} is not a column name") + if isinstance(spec.get("y"), list): + notes.append("y is a list (two measures, not a breakdown)") + # `render_chart` must follow its producer, not precede it. + order = [call["name"] for call in calls] + if order.index(_CHART_TOOL) == 0: + notes.append("render_chart called before any stats query") + # The stacked case: one chart with a `series`, not two charts. + if ( + charts + and "change type" + in " ".join(str(v) for v in charts[0]["arguments"].values()).lower() + ): + spec = charts[0]["arguments"] + if spec.get("kind") not in ("bar_stacked", "bar_h_stacked"): + notes.append("breakdown asked for, but kind is not stacked") + elif not spec.get("series"): + notes.append("stacked kind without a series column") + + return notes def _run_target(base: str, provider: str | None, model: str | None) -> list[tuple]: @@ -181,13 +316,18 @@ def _run_target(base: str, provider: str | None, model: str | None) -> list[tupl rows: list[tuple] = [] for klass, question, expected in CASES: session = _post(f"{base}/api/chat/sessions", session_body or None) - names, error = _stream_tool_calls(base, session["id"], question) + names, calls, error = _stream_tool_calls(base, session["id"], question) first = names[0] if names else "(none)" - # `run_project_stats` must appear on statistical questions and on NO - # others — a general SQL tool cannibalising the specialized ones is - # risk 1 of the plan, and this is what measures it. - leaked = klass != "statistics" and _STATS_TOOL in names - rows.append((klass, question, first, names, first in expected, leaked, error)) + # A general SQL tool cannibalising the specialized ones is the original + # risk; a *structural* SQL tool answering a temporal question is the new + # one. Both are "the wrong database reached", so both count as a leak. + leaked = ( + klass not in _HISTORY_CLASSES | {"graph-stats"} and _STATS_TOOL in names + ) or (klass in _HISTORY_CLASSES and _GRAPH_STATS_TOOL in names) + notes = _obedience_notes(klass, calls) + rows.append( + (klass, question, first, names, first in expected, leaked, error, notes) + ) return rows @@ -216,14 +356,34 @@ def _clauses(rows: list[tuple]) -> list[tuple[str, bool]]: ), ), ( - f"statistics: {_STATS_TOOL} appears there and nowhere else", - all(not leaked for *_, leaked, _ in rows) + f"statistics: {_STATS_TOOL} appears there, and the wrong DB nowhere", + all(not row[5] for row in rows) and all( _STATS_TOOL in names for klass, _, _, names, *_ in rows if klass == "statistics" ), ), + ( + f"structure: {_GRAPH_STATS_TOOL} answers the code-shape questions", + all( + _GRAPH_STATS_TOOL in names + for klass, _, _, names, *_ in rows + if klass == "graph-stats" + ), + ), + ( + f"charts: {_CHART_TOOL} follows a stats call, naming columns", + all(not row[7] for row in rows if row[0] == "chart"), + ), + ( + "identity: resolved through the author table, not a raw git identity", + all(not row[7] for row in rows if row[0] == "identity"), + ), + ( + "round limit: no charted question hit it", + all(len(row[3]) <= 6 for row in rows if row[0] == "chart"), + ), ] @@ -233,20 +393,24 @@ def _report(label: str, rows: list[tuple]) -> tuple[int, int]: print(f"\n=== {label} ===") print(f"{'class':11} {'question':{width}} {'first tool':22} verdict") print("-" * (11 + width + 34)) - for klass, question, first, names, passed, leaked, error in rows: + for klass, question, first, names, passed, leaked, error, notes in rows: verdict = "PASS" if passed else "MISS" if leaked: - verdict += " +STATS-LEAK" + verdict += " +WRONG-DB" if error: verdict += f" [error: {error[:40]}]" print(f"{klass:11} {question:{width}} {first:22} {verdict}") if len(names) > 1: print(f"{'':11} {'':{width}} \u21b3 then: {', '.join(names[1:])}") + for note in notes: + print(f"{'':11} {'':{width}} \u2718 {note}") passes = sum(1 for row in rows if row[4]) leaks = sum(1 for row in rows if row[5]) print(f"\nfirst-tool correct: {passes}/{len(rows)}") - print(f"{_STATS_TOOL} leaked onto a non-statistical question: {leaks}") + print(f"a stats tool reached the WRONG database: {leaks}") + disobeyed = sum(1 for row in rows if row[7]) + print(f"cases where a tool description was not obeyed: {disobeyed}") for clause, ok in _clauses(rows): print(f" [{'PASS' if ok else 'FAIL'}] {clause}") return passes, leaks diff --git a/src/playground/package-lock.json b/src/playground/package-lock.json index 78d47d1..b5115f2 100644 --- a/src/playground/package-lock.json +++ b/src/playground/package-lock.json @@ -12,6 +12,8 @@ "@xyflow/react": "^12.3.5", "clsx": "^2.1.1", "cmdk": "^1.0.0", + "echarts": "^6.1.0", + "echarts-for-react": "^3.0.6", "elkjs": "^0.9.3", "react": "^18.3.1", "react-dom": "^18.3.1", @@ -2326,6 +2328,36 @@ "dev": true, "license": "MIT" }, + "node_modules/echarts": { + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/echarts/-/echarts-6.1.0.tgz", + "integrity": "sha512-q0yaFPggC9FUdsWH4blavRWFmxdrIodbkoKNAjJudAI6CA9gNPxHtV2RcZNEepZVlk4yvBYkOkbk6HIVpIyHZA==", + "license": "Apache-2.0", + "dependencies": { + "tslib": "2.3.0", + "zrender": "6.1.0" + } + }, + "node_modules/echarts-for-react": { + "version": "3.0.6", + "resolved": "https://registry.npmjs.org/echarts-for-react/-/echarts-for-react-3.0.6.tgz", + "integrity": "sha512-4zqLgTGWS3JvkQDXjzkR1k1CHRdpd6by0988TWMJgnvDytegWLbeP/VNZmMa+0VJx2eD7Y632bi2JquXDgiGJg==", + "license": "MIT", + "dependencies": { + "fast-deep-equal": "^3.1.3", + "size-sensor": "^1.0.1" + }, + "peerDependencies": { + "echarts": "^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.0.0", + "react": "^15.0.0 || >=16.0.0" + } + }, + "node_modules/echarts/node_modules/tslib": { + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.0.tgz", + "integrity": "sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg==", + "license": "0BSD" + }, "node_modules/electron-to-chromium": { "version": "1.5.395", "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.5.395.tgz", @@ -2423,6 +2455,12 @@ "resolved": "https://registry.npmjs.org/extend/-/extend-3.0.2.tgz", "integrity": "sha512-fjquC59cD7CyW6urNXK0FBufkZcoiGG80wTuPujX590cB5Ttln20E2UB4S/WARVqhXffZl2LNgS+gQdPIIim/g==" }, + "node_modules/fast-deep-equal": { + "version": "3.1.3", + "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz", + "integrity": "sha512-f3qQ9oQy9j2AhBe/H9VC91wLmKBCCU/gDOnKNAYG5hswO7BLKj09Hc5HYNz9cGI++xlpDCIgDaitVs03ATR84Q==", + "license": "MIT" + }, "node_modules/fast-glob": { "version": "3.3.3", "resolved": "https://registry.npmjs.org/fast-glob/-/fast-glob-3.3.3.tgz", @@ -4317,6 +4355,12 @@ "semver": "bin/semver.js" } }, + "node_modules/size-sensor": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/size-sensor/-/size-sensor-1.0.3.tgz", + "integrity": "sha512-+k9mJ2/rQMiRmQUcjn+qznch260leIXY8r4FyYKKyRBO/s5UoeMAHGkCJyE1R/4wrIhTJONfyloY55SkE7ve3A==", + "license": "ISC" + }, "node_modules/source-map-js": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/source-map-js/-/source-map-js-1.2.1.tgz", @@ -4839,6 +4883,21 @@ "dev": true, "license": "ISC" }, + "node_modules/zrender": { + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/zrender/-/zrender-6.1.0.tgz", + "integrity": "sha512-oEGMDB6pOP2S6OwRR4PdVv610zrjnA3Bh+JnSG12fYJlBKjtNAoEb5fSUoCOOINlH96I2fU38/A2UpRKs67xYQ==", + "license": "BSD-3-Clause", + "dependencies": { + "tslib": "2.3.0" + } + }, + "node_modules/zrender/node_modules/tslib": { + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.0.tgz", + "integrity": "sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg==", + "license": "0BSD" + }, "node_modules/zustand": { "version": "5.0.14", "resolved": "https://registry.npmjs.org/zustand/-/zustand-5.0.14.tgz", diff --git a/src/playground/package.json b/src/playground/package.json index a0a15d1..6292a11 100644 --- a/src/playground/package.json +++ b/src/playground/package.json @@ -14,6 +14,8 @@ "@xyflow/react": "^12.3.5", "clsx": "^2.1.1", "cmdk": "^1.0.0", + "echarts": "^6.1.0", + "echarts-for-react": "^3.0.6", "elkjs": "^0.9.3", "react": "^18.3.1", "react-dom": "^18.3.1", diff --git a/src/playground/src/components/chat/ChartBlock.tsx b/src/playground/src/components/chat/ChartBlock.tsx new file mode 100644 index 0000000..cff6d4e --- /dev/null +++ b/src/playground/src/components/chat/ChartBlock.tsx @@ -0,0 +1,495 @@ +import ReactEChartsCore from "echarts-for-react/lib/core"; +import { useRef, useState } from "react"; +import { clsx } from "clsx"; +import type { ChartPayload } from "./chartSpec"; +import echarts from "./echarts"; + +// The chart card: header, the plot, the mandatory Table twin, and PNG export. +// +// Colours here are raw hex on purpose — they go inside the ECharts `option` +// object, which Tailwind cannot reach. Every one is a `tailwind.config.js` token +// and the data colour was chosen by running a palette validator against this exact +// surface, not by eye. `accent2` (#818cf8) is deliberately absent: it failed the +// dark lightness band, which is why links and marks do not share a colour here. +const SURFACE = "#171b24"; // panel2 — the assistant bubble, so the chart surface +const PANEL = "#12151c"; // panel — the tooltip, one step back from the surface +const BORDER = "#242a36"; // gridlines and the axis rule, 1px solid, never dashed +const MUTED = "#8b93a7"; // axis ticks and captions, never a mark colour +const FG = "#e6e9f0"; // values and tooltip text + +// Six fixed slots for stack segments, in this order. Slot 1 is the app's accent. +// Validated as a palette against SURFACE on the *adjacent* pairlist — a stack only +// ever places touching segments side by side, which is what makes six slots +// available where a scatter plot would be capped at three. Slot order is fixed and +// assignment comes from the server's `seriesValues`, so a segment keeps its colour +// across a re-render, a reload, and a scroll-past. +const PALETTE = ["#6366f1", "#d95926", "#199e70", "#c98500", "#d55181", "#008300"]; +const MARK = PALETTE[0]; + +const MAX_X_TICKS = 12; +const ROW_HEIGHT = 22; + +// `EChartsOption` would have to come from the package root, and the tree-shaking +// guard greps for exactly that import. A type-only import is erased at build time, +// but a grep cannot tell the difference — so the option is typed structurally here +// and `echarts.ts` stays the only file that reaches into the library. +type Option = Record; + +/** Compact a number for a label: 1,284 / 12.9K / 3.4M. */ +export function formatValue(value: unknown): string { + if (typeof value !== "number" || !Number.isFinite(value)) return "—"; + const magnitude = Math.abs(value); + if (magnitude >= 1_000_000) return `${(value / 1_000_000).toFixed(1)}M`; + if (magnitude >= 10_000) return `${(value / 1000).toFixed(1)}K`; + return value.toLocaleString(undefined, { maximumFractionDigits: 2 }); +} + +function slugify(title: string): string { + return ( + title + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, "") || "chart" + ); +} + +/** + * The tooltip content, built as a DOM element with `textContent`. + * + * ECharts renders a formatter *string* as HTML — that is how people add coloured + * dots — and our labels are repo content: a commit subject, a symbol name, an + * author login. A string formatter would put `` from a commit + * message into an HTML sink. Returning a built element means no HTML is ever + * parsed, so escaping cannot be got wrong. + * + * The swatch colour comes from our own palette by index, never from the params, so + * even the one styled attribute here is not data-derived. + */ +function tooltipFormatter(params: unknown): HTMLElement { + const items = (Array.isArray(params) ? params : [params]) as Array<{ + axisValueLabel?: unknown; + name?: unknown; + seriesName?: unknown; + seriesIndex?: number; + value?: unknown; + }>; + + const root = document.createElement("div"); + root.style.cssText = "font-size:12px;line-height:1.5;"; + + const heading = document.createElement("div"); + heading.textContent = String(items[0]?.axisValueLabel ?? items[0]?.name ?? ""); + heading.style.cssText = `color:${MUTED};margin-bottom:2px;`; + root.append(heading); + + for (const item of items) { + const row = document.createElement("div"); + row.style.cssText = "display:flex;align-items:center;gap:6px;"; + + if (items.length > 1) { + const swatch = document.createElement("span"); + swatch.style.cssText = + `width:8px;height:8px;border-radius:2px;flex:0 0 auto;` + + `background:${PALETTE[(item.seriesIndex ?? 0) % PALETTE.length]};`; + row.append(swatch); + + const label = document.createElement("span"); + label.textContent = String(item.seriesName ?? ""); + label.style.cssText = `color:${MUTED};`; + row.append(label); + } + + const value = document.createElement("span"); + value.textContent = formatValue(item.value); + value.style.cssText = `color:${FG};font-variant-numeric:tabular-nums;`; + row.append(value); + + root.append(row); + } + return root; +} + +/** How many category labels to skip so at most `MAX_X_TICKS` are drawn. */ +function tickInterval(count: number): number { + return Math.max(0, Math.ceil(count / MAX_X_TICKS) - 1); +} + +/** + * The whole visual specification, as one pure function. + * + * Kept pure and separate from the component so the spec is readable in one place + * and a rendering question is answered by reading it rather than by tracing state. + */ +export function buildOption(payload: ChartPayload): Option { + const { kind, rows, xIndex, yIndex, yLabel, stack } = payload; + const horizontal = kind === "bar_h" || kind === "bar_h_stacked"; + const isLine = kind === "line"; + const stacked = !!stack; + + const categories = stacked + ? stack.xValues + : rows.map((row) => String(row[xIndex] ?? "")); + + const radius: [number, number, number, number] = horizontal + ? [0, 4, 4, 0] + : [4, 4, 0, 0]; + + let series: Option[]; + if (stacked) { + // Push in `seriesValues` order, so the largest segment sits at the axis and + // the eye compares it against a straight edge. The seam is a 2px + // surface-coloured border — the one place a border on a bar is correct, and + // the secondary encoding the palette's adjacent-pair margin leans on. + series = stack.seriesValues.map((name, index) => ({ + name, + type: "bar", + stack: "total", + barMaxWidth: 24, + itemStyle: { + color: PALETTE[index % PALETTE.length], + borderColor: SURFACE, + borderWidth: 2, + // Rounded on the outermost segment only. Rounding every segment reads as + // separate floating bars rather than one total. + borderRadius: index === stack.seriesValues.length - 1 ? radius : 0, + }, + data: stack.xValues.map((x) => stack.cells[x]?.[name] ?? 0), + })); + } else if (isLine) { + series = [ + { + type: "line", + data: rows.map((row) => row[yIndex] ?? null), + lineStyle: { width: 2, cap: "round", join: "round" }, + itemStyle: { color: MARK }, + // A 10% wash rather than a distinct `area` kind — the recommended form for + // a single-series trend, without a kind the model could mis-pick. + areaStyle: { color: MARK, opacity: 0.1 }, + showSymbol: false, + emphasis: { itemStyle: { borderColor: SURFACE, borderWidth: 2 } }, + symbolSize: 8, + // Direct labels selectively only: the last point, so a reader has one + // anchored number without 200 of them fighting the line. + endLabel: { + show: true, + color: MUTED, + fontSize: 11, + // A function, not a `{@[1]}` template: label templates are painted onto + // the canvas rather than parsed, so this is not a sink — but keeping + // every formatter in this file a function means "is any formatter a + // string?" stays a one-line answer. + formatter: (params: { value?: unknown }) => formatValue(params.value), + }, + }, + ]; + } else { + // The single largest bar carries a label. Per-datum config rather than a + // `markPoint`, which would need a component this bundle does not register. + const values = rows.map((row) => row[yIndex]); + let peak = -1; + let best = -Infinity; + values.forEach((value, index) => { + if (typeof value === "number" && value > best) { + best = value; + peak = index; + } + }); + series = [ + { + type: "bar", + barMaxWidth: 24, + barCategoryGap: "20%", + itemStyle: { color: MARK, borderRadius: radius }, + data: values.map((value, index) => + index === peak + ? { + value: value ?? null, + label: { + show: true, + position: horizontal ? "right" : "top", + color: MUTED, + fontSize: 11, + formatter: () => formatValue(value), + }, + } + : (value ?? null), + ), + }, + ]; + } + + const categoryAxis: Option = { + type: "category", + data: categories, + axisLabel: { + color: MUTED, + fontSize: 11, + // Never rotate — a rotated label is what `bar_h` exists to avoid. On a + // horizontal chart every category label is shown, since long labels are the + // reason that kind was chosen. + rotate: 0, + interval: horizontal ? 0 : tickInterval(categories.length), + }, + axisLine: { lineStyle: { color: BORDER } }, + axisTick: { show: false }, + splitLine: { show: false }, + // On a horizontal chart the first category would otherwise land at the bottom, + // putting rank #1 furthest from the eye. + ...(horizontal ? { inverse: true } : {}), + }; + + const valueAxis: Option = { + type: "value", + name: yLabel, + nameTextStyle: { color: MUTED, fontSize: 11 }, + splitNumber: 4, + axisLabel: { + color: MUTED, + fontSize: 11, + formatter: (value: number) => value.toLocaleString(), + }, + axisLine: { show: false }, + axisTick: { show: false }, + splitLine: { lineStyle: { color: BORDER, width: 1, type: "solid" } }, + }; + + return { + animation: true, + backgroundColor: "transparent", + grid: { + top: stacked ? 28 : 8, + right: 16, + bottom: 4, + left: 4, + // Constrains the grid *including its axis labels* to this box, so a fixed + // container cannot clip a long file path. `containLabel: true` was the + // ECharts 5 spelling; in 6 it is legacy and warns on every render unless + // `LegacyGridContainLabel` is registered. Measured equivalent: the longest + // category label lands at the same x under either. + outerBounds: { top: stacked ? 28 : 8, right: 16, bottom: 4, left: 4 }, + }, + legend: stacked + ? { + show: true, + top: 0, + left: 0, + itemWidth: 8, + itemHeight: 8, + itemGap: 12, + icon: "roundRect", + textStyle: { color: MUTED, fontSize: 11 }, + data: stack.seriesValues, + } + : // One measure has nothing to key, and the card header already carries the + // title as real DOM text. + { show: false }, + tooltip: { + trigger: "axis", + axisPointer: { + // On bars the shadow makes the hit area the whole category band including + // the 2px gap; on a line a crosshair reads better. + type: isLine ? "line" : "shadow", + lineStyle: { color: BORDER, width: 1 }, + }, + backgroundColor: PANEL, + borderColor: BORDER, + textStyle: { color: FG, fontSize: 12 }, + extraCssText: "box-shadow:none;", + formatter: tooltipFormatter, + }, + xAxis: horizontal ? valueAxis : categoryAxis, + yAxis: horizontal ? categoryAxis : valueAxis, + series, + }; +} + +/** + * The 1-row-by-1-value form: the number *is* the chart. + * + * A one-bar chart is a cataloged anti-pattern, and this case is mostly prevented + * upstream — a producer mints no `chart_ref` below two rows — but the fallback + * stays for anything that slips through. No ECharts instance is created, and no + * download button is offered: there is nothing to save that the sentence beside it + * does not already say. Proportional figures, not `tabular-nums`: equal-width + * digits make a large standalone number look loose. + */ +function StatTile({ payload }: { payload: ChartPayload }) { + const value = payload.rows[0]?.[payload.yIndex]; + return ( +
+
{formatValue(value)}
+
+ {payload.yLabel ?? payload.columns[payload.yIndex]} +
+
+ ); +} + +/** + * The accessible twin, and not optional. + * + * Under the canvas renderer the chart puts no text in the DOM — it is not + * selectable and not screen-readable. This table is the only representation of the + * numbers that is, and it is drawn from the same rows the chart plots, so the two + * cannot disagree. If it is ever dropped, the renderer choice has to be revisited. + */ +function TableView({ payload }: { payload: ChartPayload }) { + return ( +
+ + + + {payload.columns.map((column) => ( + + ))} + + + + {payload.rows.map((row, index) => ( + + {payload.columns.map((column, cell) => ( + + ))} + + ))} + +
+ {column} +
+ {row[cell] === null || row[cell] === undefined + ? "—" + : String(row[cell])} +
+
+ ); +} + +export function ChartBlock({ payload }: { payload: ChartPayload }) { + const [view, setView] = useState<"chart" | "table">("chart"); + const instance = useRef(null); + + const statTile = payload.rows.length === 1 && !payload.stack; + const horizontal = payload.kind === "bar_h" || payload.kind === "bar_h_stacked"; + const categoryCount = payload.stack + ? payload.stack.xValues.length + : payload.rows.length; + // A horizontal chart's categories stack vertically, so its height has to grow + // with them — 20 file paths in 220px is a smear, whatever `containLabel` does. + const height = horizontal + ? Math.min(520, Math.max(220, categoryCount * ROW_HEIGHT + 48)) + : 220; + + const download = () => { + const chart = instance.current?.getEchartsInstance(); + if (!chart) return; + const url = chart.getDataURL({ + type: "png", + pixelRatio: 2, + // Opaque, not transparent: a transparent PNG pasted into a light document + // is unreadable. + backgroundColor: SURFACE, + }); + const anchor = document.createElement("a"); + anchor.href = url; + anchor.download = `${slugify(payload.title)}-${payload.rows.length}-rows.png`; + anchor.click(); + }; + + const captions: string[] = []; + if (payload.nullRows > 0) { + captions.push( + `${payload.nullRows} row${payload.nullRows === 1 ? "" : "s"} had no value and ${ + payload.nullRows === 1 ? "is" : "are" + } not plotted.`, + ); + } + if (payload.stack && payload.stack.filledCells > 0) { + // The zero-fill is correct — an absent GROUP BY group means zero — but it must + // never be silent, or a reader comparing the chart to the table cannot explain + // why the table has fewer rows than the chart has cells. + captions.push( + `${payload.stack.filledCells} combination${ + payload.stack.filledCells === 1 ? "" : "s" + } had no rows and ${payload.stack.filledCells === 1 ? "is" : "are"} shown as zero.`, + ); + } + + return ( +
+
+
{payload.title}
+ {!statTile && ( + <> +
+ {(["chart", "table"] as const).map((option) => ( + + ))} +
+ {view === "chart" && ( + + )} + + )} +
+ + {statTile ? ( + + ) : view === "table" ? ( + + ) : ( +
+ +
+ )} + + {captions.length > 0 && ( +
+ {captions.map((caption) => ( +
{caption}
+ ))} +
+ )} +
+ ); +} diff --git a/src/playground/src/components/chat/MessageBubble.tsx b/src/playground/src/components/chat/MessageBubble.tsx index 0800931..105c29a 100644 --- a/src/playground/src/components/chat/MessageBubble.tsx +++ b/src/playground/src/components/chat/MessageBubble.tsx @@ -1,6 +1,17 @@ +import { Suspense, lazy } from "react"; +import { parseChart } from "./chartSpec"; import { Markdown } from "./Markdown"; import { ToolCallCard, type ToolActivity } from "./ToolCallCard"; +// ECharts is ~196 KB gzipped even fully tree-shaken — measured, against a vendor +// claim of ~100 KB — and a transcript with no chart in it should not pay for that. +// Charts are conditional UI, so the whole renderer loads on first use and the +// initial bundle grows by ~1 KB instead of ~575 KB. `chartSpec.ts` stays eager: it +// decides *whether* there is a chart, and it imports nothing heavy. +const ChartBlock = lazy(() => + import("./ChartBlock").then((module) => ({ default: module.ChartBlock })), +); + /** * One conversational turn: the user's question, or the assistant's reply with * its tool activity interleaved. @@ -57,6 +68,13 @@ function UserBubble({ content }: { content: string }) { function AssistantBubble({ turn }: { turn: AssistantTurn }) { const rows = Math.max(turn.segments.length, turn.activityGroups.length); + // Flattened across groups on purpose. `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, and live too whenever the model says something + // between them. Searching only the chart's own group would find the producer + // exactly when the two happened to share a round, which is the case that does + // not survive a reload. + const turnActivities = turn.activityGroups.flat(); const isEmpty = !turn.error && turn.segments.every((s) => !s) && @@ -68,9 +86,31 @@ function AssistantBubble({ turn }: { turn: AssistantTurn }) { {Array.from({ length: rows }, (_, i) => (
{turn.segments[i] ? {turn.segments[i]} : null} - {(turn.activityGroups[i] ?? []).map((activity) => ( - - ))} + {(turn.activityGroups[i] ?? []).map((activity) => { + // A `render_chart` card renders its chart as a sibling, so the + // "see exactly what the assistant looked at" guarantee survives. + // The whole turn is passed because the rows live on the producing + // stats activity, matched by `chart_ref`. + const chart = parseChart(activity, turnActivities); + return ( + // The key stays on the wrapper: dropping it hands one card's + // expansion state to another as the transcript re-renders. +
+ + {chart && ( + + Loading chart… +
+ } + > + + + )} +
+ ); + })} ))} diff --git a/src/playground/src/components/chat/chartSpec.ts b/src/playground/src/components/chat/chartSpec.ts new file mode 100644 index 0000000..8262a95 --- /dev/null +++ b/src/playground/src/components/chat/chartSpec.ts @@ -0,0 +1,170 @@ +import type { ToolActivity } from "./ToolCallCard"; + +// The parse boundary, and the correlation step the two-step chart design needs. +// +// `render_chart` returns the *directive* — kind, title, resolved column indices — +// but not the rows: the producing stats call already delivered those, so echoing +// them would double a 200-row payload for nothing. That means the rows have to be +// found again here, by matching `chart_ref` against the sibling activities of the +// same turn. +// +// This works identically live and on replay, because `turns.ts` groups activities +// per turn in both paths. It is the one piece of the split design that could have +// been awkward, and isn't. +// +// Nothing here throws. A tool result is a string that arrived over the network and +// may be truncated mid-array by the server's own result cap, so `JSON.parse` is +// the load-bearing `try` — a chart that fails to parse must render as no chart +// beside numbers that are still correct, never as a crashed transcript. + +export interface ChartStack { + seriesIndex: number; + /** Stack order and colour-slot order: descending by total, ties lexicographic. */ + seriesValues: string[]; + /** The x axis, in the query's own row order. */ + xValues: string[]; + /** cells[xValue][seriesValue] -> number. Dense: absent combinations are 0. */ + cells: Record>; + /** How many cells the server filled, for the caption. Never silent. */ + filledCells: number; +} + +export interface ChartPayload { + kind: "line" | "bar" | "bar_h" | "bar_stacked" | "bar_h_stacked"; + title: string; + yLabel?: string; + xIndex: number; + yIndex: number; + columns: string[]; + rows: unknown[][]; + nullRows: number; + /** + * Stacked kinds only. The server sends the densified grid, so the frontend + * never pivots and the chart and the Table view cannot disagree. + */ + stack?: ChartStack; +} + +const KINDS = new Set(["line", "bar", "bar_h", "bar_stacked", "bar_h_stacked"]); +const STACKED = new Set(["bar_stacked", "bar_h_stacked"]); + +/** Parse a tool result, returning null rather than throwing on anything odd. */ +function parseResult(result: string | undefined): Record | null { + if (!result) return null; + try { + const parsed: unknown = JSON.parse(result); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return null; + return parsed as Record; + } catch { + return null; + } +} + +/** The `chart_ref` a stats result carried, if it carried one. */ +function refOf(activity: ToolActivity): string | null { + if (activity.running) return null; + const parsed = parseResult(activity.result); + const ref = parsed?.chart_ref; + return typeof ref === "string" ? ref : null; +} + +function readStack(chart: Record): ChartStack | null { + const seriesIndex = chart.series_index; + const seriesValues = chart.series_values; + const xValues = chart.x_values; + const cells = chart.cells; + if ( + typeof seriesIndex !== "number" || + !Array.isArray(seriesValues) || + !Array.isArray(xValues) || + !cells || + typeof cells !== "object" + ) { + return null; + } + return { + seriesIndex, + seriesValues: seriesValues.map(String), + xValues: xValues.map(String), + cells: cells as Record>, + filledCells: typeof chart.filled_cells === "number" ? chart.filled_cells : 0, + }; +} + +/** + * Build a chart payload from a `render_chart` activity. + * + * @param activity - the `render_chart` card itself. + * @param turnActivities - every activity in the same turn, searched for the + * producing stats call that minted the same `chart_ref`. + * @returns the payload, or `null` when the activity is not a chart, is still + * running, errored, is unparseable, has no matching producer in this turn, or + * resolves to indices outside the producer's columns. + */ +export function parseChart( + activity: ToolActivity, + turnActivities: ToolActivity[], +): ChartPayload | null { + if (activity.name !== "render_chart" || activity.running) return null; + + const result = parseResult(activity.result); + // `error` is the normal failure path, not an exception: a bad directive leaves + // the producer's numbers on screen and tells the model how to fix the call. + if (!result || "error" in result) return null; + + const ref = result.chart_ref; + const chart = result.chart; + if (typeof ref !== "string" || !chart || typeof chart !== "object") return null; + const directive = chart as Record; + + const kind = directive.kind; + const title = directive.title; + const xIndex = directive.x_index; + const yIndex = directive.y_index; + if ( + typeof kind !== "string" || + !KINDS.has(kind) || + typeof title !== "string" || + typeof xIndex !== "number" || + typeof yIndex !== "number" + ) { + return null; + } + + // The correlation step: find the producer that minted this ref, in this turn. + const producer = turnActivities.find( + (candidate) => candidate !== activity && refOf(candidate) === ref, + ); + if (!producer) return null; + const produced = parseResult(producer.result); + const columns = produced?.columns; + const rows = produced?.rows; + if (!Array.isArray(columns) || !Array.isArray(rows)) return null; + + // The server resolved these indices, but the rows travelled separately — so + // re-check the bound rather than trusting two payloads to agree. + if (xIndex < 0 || yIndex < 0 || xIndex >= columns.length || yIndex >= columns.length) { + return null; + } + + let stack: ChartStack | undefined; + if (STACKED.has(kind)) { + const read = readStack(directive); + // A stacked kind with no grid is not drawable as anything else — a bar chart + // of one series would answer a different question. + if (!read) return null; + stack = read; + } + + return { + kind: kind as ChartPayload["kind"], + title, + yLabel: typeof directive.y_label === "string" ? directive.y_label : undefined, + xIndex, + yIndex, + columns: columns.map(String), + rows: rows as unknown[][], + nullRows: typeof directive.null_rows === "number" ? directive.null_rows : 0, + stack, + }; +} diff --git a/src/playground/src/components/chat/echarts.ts b/src/playground/src/components/chat/echarts.ts new file mode 100644 index 0000000..9e385bc --- /dev/null +++ b/src/playground/src/components/chat/echarts.ts @@ -0,0 +1,40 @@ +// The ONLY file permitted to import from `echarts/*`. +// +// Tree-shaken registration. Importing `echarts` wholesale instead pulls ~1MB and +// every chart type we do not use — and it fails *silently*: the chart works, the +// bundle triples. Hence one chokepoint plus a grep and a measured bundle budget, +// rather than vigilance. +// +// The registration list IS the feature list. A kind that is not registered here +// cannot be drawn, which is why `charts.py`'s CHART_KINDS and this file have to +// be changed together. +import { BarChart, LineChart } from "echarts/charts"; +import { GridComponent, LegendComponent, TooltipComponent } from "echarts/components"; +import * as echarts from "echarts/core"; +import { CanvasRenderer } from "echarts/renderers"; + +// CanvasRenderer, NOT SVGRenderer: getDataURL('png') does not work under the SVG +// renderer, and PNG export is a requirement. The cost — chart text is pixels, so +// it is neither selectable nor screen-readable — is paid by the mandatory Table +// view, which is why that view is load-bearing rather than merely correct. +// +// LegendComponent is here for the stacked kinds ONLY; an unstacked chart still +// sets `legend: {show: false}`, because one measure has nothing to key. There is +// no stacked *chart* module to add: stacking is a `stack` property on a bar +// series, so `BarChart` already covers it. +// +// Deliberately unregistered: PieChart (part-to-whole rides the stacked bar), +// ToolboxComponent (the download button is ours, matching the app's chrome), +// TitleComponent (the card header renders the title as real DOM text — +// selectable and searchable, a small win back against the canvas trade-off), +// DatasetComponent, dataZoom, MarkLineComponent. +echarts.use([ + BarChart, + LineChart, + GridComponent, + TooltipComponent, + LegendComponent, + CanvasRenderer, +]); + +export default echarts;