From 389ceb458313bd00900a6f963b1e8f54e954214c Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Jr <48456829+IBJunior@users.noreply.github.com> Date: Sat, 12 Sep 2026 13:52:29 +0200 Subject: [PATCH 1/2] feat(charts): add the render_chart tool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Takes a spec — a read-only SELECT plus how to encode it — runs the query through analyticsRepository, and returns a two-part result: a short receipt for the model, the resolved rows as an artifact for the renderer. Refuses rather than draws when the result would mislead: a column the query does not return (naming the real ones), a truncated or oversized result, or an empty one that may be a misspelling rather than missing data. Adds callToolWithArtifact, since a plain invoke returns only the content and silently drops the artifact half. Not registered with the agent yet — that is the next commit. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/charts/Chart.tsx | 10 +- src/components/charts/geometry.ts | 7 +- src/components/charts/types.ts | 22 +-- src/lib/agent/tools/charts.test.ts | 214 +++++++++++++++++++++++++++++ src/lib/agent/tools/charts.ts | 142 +++++++++++++++++++ src/lib/agent/tools/testing.ts | 24 ++++ 6 files changed, 390 insertions(+), 29 deletions(-) create mode 100644 src/lib/agent/tools/charts.test.ts create mode 100644 src/lib/agent/tools/charts.ts diff --git a/src/components/charts/Chart.tsx b/src/components/charts/Chart.tsx index 0cfa9cf..caf0502 100644 --- a/src/components/charts/Chart.tsx +++ b/src/components/charts/Chart.tsx @@ -5,14 +5,8 @@ import { buildModel, ticks, yScale } from "./geometry"; import { seriesColor, type ChartPayload } from "./types"; /** - * A chart, drawn as plain SVG from a frozen payload. - * - * No charting library: the mark specs (capped bar width, rounded data-end, 2px surface gaps, - * surface rings, recessive hairline axes) are specific enough that wrapping a library to obey them - * is more code than drawing them, and a library brings its own styling system to fight the tokens. - * - * Colors come from --chart-N, which are CATEGORICAL (one hue per slot, fixed order). They are - * deliberately not amber: amber marks the approval boundary in this app and nothing else. + * Draws a ChartPayload as SVG. Series colors come from the categorical --chart-N tokens; never + * use amber here, which this app reserves for the approval boundary. */ const H = 200; // plot height diff --git a/src/components/charts/geometry.ts b/src/components/charts/geometry.ts index cab6985..a75e2dd 100644 --- a/src/components/charts/geometry.ts +++ b/src/components/charts/geometry.ts @@ -1,11 +1,6 @@ import type { ChartPayload, ChartRow } from "./types"; -/** - * Turning a payload into coordinates. Pure and DOM-free, so the interesting decisions — scale - * domains, series grouping, empty and single-point handling — are unit-testable without a browser. - * - * The component draws what these return and adds nothing of its own. - */ +/** Turns a payload into coordinates: series grouping, scale domains, ticks. Pure and DOM-free. */ export interface Series { key: string; diff --git a/src/components/charts/types.ts b/src/components/charts/types.ts index dd41875..8148612 100644 --- a/src/components/charts/types.ts +++ b/src/components/charts/types.ts @@ -1,15 +1,8 @@ /** - * The payload a chart renders from. - * - * A ZERO-IMPORT leaf: the client renderer and the server tool both name this shape, and the - * renderer must not drag anything server-side into the browser bundle. - * - * This is the FROZEN snapshot. `rows` are the numbers as they were when the chart was made — a - * chart in a thread is a record of what was said, so reopening a month later must not re-query and - * silently restate history with today's data. + * The payload a chart renders from — shared by the server tool that builds it and the client + * component that draws it. Keep this a ZERO-IMPORT leaf so the renderer pulls in nothing + * server-side. */ - -/** The two forms we render. See docs — pie and table are deliberate omissions, not gaps. */ export type ChartType = "bar" | "line"; export interface ChartSpec { @@ -30,17 +23,16 @@ export type ChartRow = Record; export interface ChartPayload { spec: ChartSpec; columns: string[]; + /** The rows as queried. Frozen: never re-fetched when a thread is reopened. */ rows: ChartRow[]; - /** ISO timestamp — a chart is a snapshot, and a snapshot says when. */ generatedAt: string; } -/** Categorical slots, in FIXED order. Never cycle, never reorder: the order is the CVD-safety - * mechanism (validated adjacent-pair separation), not a style choice. */ +/** Number of categorical color slots. Their ORDER is validated for colorblind separation — + * re-run the dataviz palette validator before reordering or changing a value. */ export const SERIES_SLOTS = 8; -/** Resolve a series index to its CSS custom property. Past the 8th slot, callers fold to "Other" - * rather than inventing a hue. */ +/** The CSS custom property for a series index, wrapping past the last slot. */ export function seriesColor(index: number): string { return `var(--chart-${(index % SERIES_SLOTS) + 1})`; } diff --git a/src/lib/agent/tools/charts.test.ts b/src/lib/agent/tools/charts.test.ts new file mode 100644 index 0000000..4159bb8 --- /dev/null +++ b/src/lib/agent/tools/charts.test.ts @@ -0,0 +1,214 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { callToolWithArtifact } from "./testing"; + +/** + * Contract tests for render_chart — repository stubbed. See docs/TESTING.md. + * + * Success cases assert on BOTH halves: content and artifact are different payloads, so checking + * one leaves the other unverified. + */ + +vi.mock("@/lib/repositories/analyticsRepository", () => ({ runReadOnlyQuery: vi.fn() })); + +const repo = await import("@/lib/repositories/analyticsRepository"); +const { renderChart, MAX_CHART_ROWS } = await import("./charts"); + +const ARGS = { + query: "SELECT c.name AS category, SUM(t.amount_minor) AS total FROM transaction t GROUP BY 1", + chartType: "bar", + x: "category", + y: "total", +}; + +function result( + rows: Record[], + columns = ["category", "total"], + truncated = false, +) { + return { columns, rows, rowCount: rows.length, truncated }; +} + +const TWO_ROWS = [ + { category: "Dining", total: 28100 }, + { category: "Groceries", total: 42350 }, +]; + +beforeEach(() => { + vi.resetAllMocks(); +}); + +describe("render_chart", () => { + it("returns a receipt to the model and the rows to the client", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(TWO_ROWS)); + + const { content, artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect(content).toMatchObject({ ok: true, chartType: "bar", points: 2, x: "category" }); + expect(artifact.rows).toEqual(TWO_ROWS); + expect(artifact.spec).toMatchObject({ chartType: "bar", x: "category", y: "total" }); + expect(artifact.columns).toEqual(["category", "total"]); + }); + + /** + * The whole reason this tool exists. If rows leaked into content the agent would pay for data it + * never reads, which is the cost the artifact split removes. + */ + it("keeps row data out of the model's payload entirely", async () => { + const many = Array.from({ length: 50 }, (_, i) => ({ category: `cat-${i}`, total: i * 100 })); + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(many)); + + const { content, artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect(JSON.stringify(content)).not.toContain("cat-49"); + expect(JSON.stringify(content).length).toBeLessThan(150); + expect(artifact.rows).toHaveLength(50); + }); + + it("stamps the artifact with a generation time", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(TWO_ROWS)); + + const { artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect(Number.isNaN(Date.parse(artifact.generatedAt))).toBe(false); + }); + + it("carries optional series and titles into the spec", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue( + result( + [{ month: "2026-07", total: 100, category: "Dining" }], + ["month", "total", "category"], + ), + ); + + const { artifact } = await callToolWithArtifact(renderChart, { + ...ARGS, + chartType: "line", + x: "month", + series: "category", + title: "Spending over time", + subtitle: "Last 3 months", + }); + + expect(artifact.spec).toMatchObject({ + chartType: "line", + series: "category", + title: "Spending over time", + subtitle: "Last 3 months", + }); + }); + + it("omits absent optional fields rather than setting them undefined", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(TWO_ROWS)); + + const { artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect("series" in artifact.spec).toBe(false); + expect("title" in artifact.spec).toBe(false); + }); +}); + +describe("render_chart failures", () => { + /** + * The agent writes both the SQL and the encoding, so naming a column its own query didn't + * select is the likely slip. Undefined values would draw as empty bars — a chart that looks + * fine and shows nothing — so this must fail loudly and name the real columns. + */ + it.each([ + ["x", { x: "categorie" }], + ["y", { y: "amount" }], + ["series", { series: "bucket" }], + ])("rejects a %s column the query does not return", async (_label, override) => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(TWO_ROWS)); + + const { content, artifact } = await callToolWithArtifact(renderChart, { ...ARGS, ...override }); + + expect(content.ok).toBe(false); + expect(content.availableColumns).toEqual(["category", "total"]); + expect(artifact).toBeNull(); + }); + + it("names every missing column at once, not just the first", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(TWO_ROWS)); + + const { content } = await callToolWithArtifact(renderChart, { + ...ARGS, + x: "nope", + y: "alsonope", + }); + + expect(content.error).toContain("nope"); + expect(content.error).toContain("alsonope"); + }); + + /** + * A chart of a capped result looks complete — the reader cannot see the missing rows — so it + * misstates the total. Refuse rather than draw a lie. This is the same defect class as the + * truncation bug, handled differently because a chart has no room for a caveat. + */ + it("refuses to chart a truncated result", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue( + result(TWO_ROWS, ["category", "total"], true), + ); + + const { content, artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect(content.ok).toBe(false); + expect(content.error).toMatch(/partial|complete/i); + expect(artifact).toBeNull(); + }); + + it("refuses more rows than a chart can show", async () => { + const many = Array.from({ length: MAX_CHART_ROWS + 1 }, (_, i) => ({ + category: `cat-${i}`, + total: i, + })); + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(many)); + + const { content } = await callToolWithArtifact(renderChart, ARGS); + + expect(content.ok).toBe(false); + expect(content.error).toContain(String(MAX_CHART_ROWS)); + }); + + it("accepts exactly the maximum", async () => { + const many = Array.from({ length: MAX_CHART_ROWS }, (_, i) => ({ + category: `cat-${i}`, + total: i, + })); + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result(many)); + + const { content } = await callToolWithArtifact(renderChart, ARGS); + + expect(content.ok).toBe(true); + }); + + /** An empty result is byte-identical whether the category exists or is misspelled. */ + it("warns that an empty result may be a misspelling, not missing data", async () => { + vi.mocked(repo.runReadOnlyQuery).mockResolvedValue(result([])); + + const { content, artifact } = await callToolWithArtifact(renderChart, ARGS); + + expect(content.ok).toBe(false); + expect(content.error).toMatch(/spelling/i); + expect(artifact).toBeNull(); + }); + + it("rejects a non-SELECT before touching the database", async () => { + const { content } = await callToolWithArtifact(renderChart, { + ...ARGS, + query: "DELETE FROM transaction", + }); + + expect(content.ok).toBe(false); + expect(repo.runReadOnlyQuery).not.toHaveBeenCalled(); + }); + + it("surfaces the database error so the agent can fix its query", async () => { + vi.mocked(repo.runReadOnlyQuery).mockRejectedValue(new Error('column "bogus" does not exist')); + + const { content } = await callToolWithArtifact(renderChart, ARGS); + + expect(content.ok).toBe(false); + expect(content.error).toContain("bogus"); + }); +}); diff --git a/src/lib/agent/tools/charts.ts b/src/lib/agent/tools/charts.ts new file mode 100644 index 0000000..ba12a63 --- /dev/null +++ b/src/lib/agent/tools/charts.ts @@ -0,0 +1,142 @@ +import { tool } from "@langchain/core/tools"; +import { z } from "zod"; +import { runReadOnlyQuery } from "@/lib/repositories/analyticsRepository"; +import { enforceLimit, MAX_SQL_ROWS, validateSelect } from "@/lib/finance/sqlGuard"; +import type { ChartPayload, ChartType } from "@/components/charts/types"; + +/** + * `render_chart` — runs an agent-authored SELECT and returns a chart for the client to draw. + * + * Returns a two-part result (`content_and_artifact`): `content` is a short receipt for the model, + * `artifact` is the resolved rows for the renderer. The agent never sees row data. + */ + +/** Cap chart rows well below the SQL cap: past this a chart is unreadable, not just large. */ +export const MAX_CHART_ROWS = 60; + +const CHART_TYPES = ["bar", "line"] as const; + +function fail(error: string, extra: Record = {}) { + // A failed render returns content only — there is nothing for the client to draw. + return [JSON.stringify({ ok: false, error, ...extra }), null] as const; +} + +export const renderChart = tool( + async (input) => { + const verdict = validateSelect(input.query); + if (!verdict.ok) return fail(verdict.reason); + + let result; + try { + result = await runReadOnlyQuery(enforceLimit(input.query, MAX_SQL_ROWS), MAX_SQL_ROWS); + } catch (err) { + // Surface the DB error (e.g. unknown column) so the agent can fix its own SQL. + return fail(`Query failed: ${err instanceof Error ? err.message : String(err)}`); + } + + if (result.rowCount === 0) { + return fail( + "The query returned no rows, so there is nothing to chart. This does NOT confirm the " + + "filtered values exist — a misspelled category name returns an empty result too. Check " + + "the spelling (e.g. `list_categories`) before telling the user they have no such data.", + ); + } + + // Fail loud on a column the query doesn't produce, naming the real ones. The agent writes both + // the SQL and the encoding, so naming a column its own query didn't select is the likely slip + // — and a chart drawn from undefined values would render as empty bars rather than an error. + const wanted = [ + ["x", input.x], + ["y", input.y], + ...(input.series ? [["series", input.series]] : []), + ] as [string, string][]; + const missing = wanted.filter(([, col]) => !result.columns.includes(col)); + if (missing.length > 0) { + return fail( + `${missing.map(([f, c]) => `\`${f}\` refers to "${c}"`).join(", ")}, which the query does ` + + "not return. Nothing was rendered. Use one of `availableColumns`, or change the SELECT " + + "to produce that column (alias it if it is computed).", + { availableColumns: result.columns }, + ); + } + + // A chart built from a capped result would misstate the total while looking complete — the + // reader cannot see that rows are missing. Refuse rather than draw a lie. + if (result.truncated || result.rowCount > MAX_CHART_ROWS) { + return fail( + `The query returned ${result.truncated ? `more than ${MAX_SQL_ROWS}` : result.rowCount} ` + + `rows; a chart takes at most ${MAX_CHART_ROWS}. A chart of a partial result looks ` + + "complete and misstates the total, so nothing was rendered. Aggregate further " + + "(GROUP BY), filter to a narrower period, or take a top-N with ORDER BY + LIMIT.", + ); + } + + const payload: ChartPayload = { + spec: { + chartType: input.chartType as ChartType, + x: input.x, + y: input.y, + ...(input.series ? { series: input.series } : {}), + ...(input.title ? { title: input.title } : {}), + ...(input.subtitle ? { subtitle: input.subtitle } : {}), + }, + columns: result.columns, + rows: result.rows, + generatedAt: new Date().toISOString(), + }; + + // Content stays a receipt no matter how large the artifact is. + const content = JSON.stringify({ + ok: true, + chartType: input.chartType, + points: result.rowCount, + x: input.x, + y: input.y, + ...(input.series ? { series: input.series } : {}), + }); + + return [content, payload] as const; + }, + { + name: "render_chart", + description: + "Draw a chart from a read-only SQL SELECT and show it to the user. Read-only — runs without " + + "approval. You supply the query and how to encode it; the chart is rendered for the user " + + "and you get back only a short confirmation, NOT the rows — so do not call this to see " + + "data (use `run_sql` for that) and do not describe values you have not read. Pick " + + "`chartType` by the question: `line` when the x-axis is time (a trend), `bar` when it is " + + "categories (a comparison). `x`, `y` and `series` must name columns your SELECT actually " + + "returns — alias computed columns (e.g. `SUM(amount_minor) AS total`). Aggregate in SQL: at " + + `most ${MAX_CHART_ROWS} rows can be charted. Not every answer needs a chart — a single ` + + "figure is a sentence and a short ranking is better read as a table from `run_sql`.", + schema: z.object({ + query: z + .string() + .min(1) + .describe( + "A single read-only SQL SELECT producing one row per point, already aggregated " + + "(e.g. SELECT c.name AS category, SUM(t.amount_minor) AS total ... GROUP BY c.name)", + ), + chartType: z + .enum(CHART_TYPES) + .describe("'line' for a measure over time, 'bar' for a comparison across categories"), + x: z.string().min(1).describe("Column for the category or time step, e.g. 'category'"), + y: z.string().min(1).describe("Column holding the measure to plot, e.g. 'total'"), + series: z + .string() + .min(1) + .optional() + .describe("Optional column that splits the measure into multiple lines/bars"), + title: z.string().optional().describe("Short chart title, e.g. 'Spending by category'"), + subtitle: z + .string() + .optional() + .describe("Optional line under the title — units or the period covered"), + }), + // Splits the return tuple: content reaches the model, artifact reaches the client only. + responseFormat: "content_and_artifact", + }, +); + +/** Chart tools, registered into the agent in agent/index.ts. Read-only — not gated. */ +export const chartTools = [renderChart]; diff --git a/src/lib/agent/tools/testing.ts b/src/lib/agent/tools/testing.ts index 29a4888..1fbfa0c 100644 --- a/src/lib/agent/tools/testing.ts +++ b/src/lib/agent/tools/testing.ts @@ -16,6 +16,30 @@ export async function callTool>( return JSON.parse(typeof raw === "string" ? raw : JSON.stringify(raw)) as T; } +/** + * Invoke a `content_and_artifact` tool and get BOTH halves. + * + * A plain-object invoke returns only the content string and silently drops the artifact — so a + * test written with `callTool` would assert on the model's receipt while the client's payload went + * unchecked. Passing a tool-call shape returns a ToolMessage instead, which carries both. + */ +export async function callToolWithArtifact, A = any>( + tool: StructuredToolInterface, + input: Record = {}, +): Promise<{ content: C; artifact: A }> { + const msg = (await tool.invoke({ + name: tool.name, + args: input, + id: "test-call", + type: "tool_call", + } as never)) as { content: unknown; artifact: A }; + const raw = msg.content; + return { + content: JSON.parse(typeof raw === "string" ? raw : JSON.stringify(raw)) as C, + artifact: msg.artifact, + }; +} + /** Build a Transaction with sensible defaults; override only what the test cares about. */ export function makeTransaction(overrides: Partial = {}): Transaction { const now = new Date("2026-07-05T00:00:00.000Z"); From 9393c191f207ccc64a037cf34c6fe512d5897a86 Mon Sep 17 00:00:00 2001 From: Ali Ibrahim Jr <48456829+IBJunior@users.noreply.github.com> Date: Sat, 12 Sep 2026 17:31:12 +0200 Subject: [PATCH 2/2] feat(charts): register render_chart and forward artifacts to the client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Binds the tool to the agent and carries its artifact through the stream, so a chart request now renders end to end. The artifact is not on the toolCalls stream — `call.output` resolves to the content string alone — so it is collected from the ToolMessages in `run.values`, keyed by call id, with a fallback to the final state for the case where the snapshot has not arrived by the time the call resolves. A chart renders above the collapsed tool disclosure rather than inside it: it is the answer, not a detail of it. The receipt stays collapsed below. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/ToolMessage.tsx | 31 +++++++++++++++++++++++++- src/components/toolRenderers/config.ts | 4 +++- src/lib/agent/capabilities.test.ts | 1 + src/lib/agent/capabilities.ts | 2 ++ src/lib/agent/index.ts | 2 ++ src/services/agentService.ts | 28 +++++++++++++++++++++-- src/types/message.ts | 3 ++- 7 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/components/ToolMessage.tsx b/src/components/ToolMessage.tsx index e508fde..3635430 100644 --- a/src/components/ToolMessage.tsx +++ b/src/components/ToolMessage.tsx @@ -2,7 +2,15 @@ import React, { useState } from "react"; import type { MessageResponse } from "@/types/message"; import { ChevronDownIcon, ChevronRightIcon, CopyIcon, CheckIcon } from "lucide-react"; import { getToolName } from "@/services/messageUtils"; -import { ToolResult } from "./toolRenderers"; +import { ToolResult, renderersFor } from "./toolRenderers"; +import { Chart } from "./charts/Chart"; +import type { ChartPayload } from "./charts/types"; + +/** The artifact is `unknown` off the wire, so check the shape before drawing it. */ +const isChartPayload = (v: unknown): v is ChartPayload => { + const p = v as ChartPayload | undefined; + return !!p && typeof p === "object" && !!p.spec && Array.isArray(p.rows); +}; interface ToolMessageProps { message: MessageResponse; @@ -46,6 +54,11 @@ export const ToolMessage = ({ message }: ToolMessageProps) => { const toolName = getToolName(message); const content = getContentAsString(message.data?.content); const summary = summarize(content); + // A chart is the answer, not a detail of it, so it renders outside the collapsed disclosure — + // the receipt stays inside for inspection. + const artifact = (message.data as { artifact?: unknown })?.artifact; + const chart = + renderersFor(toolName).result === "chart" && isChartPayload(artifact) ? artifact : null; const handleCopy = async (e: React.MouseEvent) => { e.stopPropagation(); @@ -58,6 +71,22 @@ export const ToolMessage = ({ message }: ToolMessageProps) => { } }; + if (chart) { + return ( +
+ +
+ + {toolName ?? "tool"} · result + +
+ +
+
+
+ ); + } + return (