feat(serve): Chat assistant — streaming tool-calling harness over WhyGraph's tools - #38
Merged
Conversation
…Graph's tools Adds a second top-level view to the `whygraph serve` playground: a chat assistant that answers questions about the repo from live tool calls against CodeGraph, the WhyGraph history, and the source tree. Implements plans/chat-assistant-plan.md Phase 1 (all seven rollout steps). Backend - `services/llm/openrouter.py` — OpenRouter as a first-class provider, mirroring the Deepseek adapter (config section, factory row, template, golden fixture, init wizard, and `OPENROUTER_API_KEY` in all three shim env lines). - `services/llm/chat.py` — a NEW tool-calling + streaming `ChatClient` port, parallel to the untouched `LlmClient`. Sync generators, not async: serve's contract is sync-def-in-threadpool and both SDKs stream synchronously. - `services/llm/openai_chat.py` / `anthropic_chat.py` — the two dialects normalized to one event stream (fragmented `tool_calls` deltas vs `tool_use` content blocks). Zero new runtime dependencies. - `chat/` — the harness. `tools.py` holds 11 tool specs + a per-turn `ToolRegistry` carrying the rationale-generation budget; `files.py` is clamped read-only file access; `harness.py` is `run_turn` plus the token-budgeted, tool-pair-safe `build_window`. The harness is persistence-free. - `serve/chat.py` — session CRUD plus the SSE turn. Rows persist per completed round, so a disconnect loses at most the in-flight turn. - `ChatSession` / `ChatMessage` tables + one migration. Frontend - Zustand `view` switch and a header segmented control (no router). - `components/chat/` — session sidebar, thread, tool-activity cards, composer with Stop, provider picker. - react-markdown with raw HTML disabled; `whygraph://symbol/<qn>` links render as chips that open the symbol in the Explorer. Security - The file tools are the only new attack surface: root clamp via `is_relative_to`, a deny-list for `.git` / `.whygraph` / `.codegraph` / `whygraph.toml` / `.env*` even inside the root, binary refusal, and 400-line / 100 KB caps. No write tools, no shell tool; the registry is a closed allow-list. - The only nested LLM spend is `get_rationale` on a cache miss, capped per turn by `[chat].max_rationale_generations` and running the audited MCP flow verbatim so the cache row can't drift. Notes - Chat is absent from the MCP surface, which stays narrow by design. - `alembic --autogenerate` also proposed dropping `ix_commit_file_change_path_commit_sha` and stripping three `server_default`s — pre-existing model/migration drift, excluded from this migration so area-history queries don't regress. 686 tests pass (+122); ruff check, ruff format --check, and the playground build are green. Verified end-to-end against the live Anthropic API.
Replaces the free-text model input with a real model dropdown, and makes
both provider and model switchable from the thread header at any time
(previously they were fixed for a session's lifetime).
Model options are fetched from the provider rather than hardcoded — a
baked-in list would rot on every model release and could never cover
OpenRouter's ~370-model catalogue.
- `ChatClient.list_models()` on the port, implemented by both adapters
(`client.models.list()` on each SDK). Anthropic is the only provider
that returns display names, so its dropdown shows "Claude Opus 5"
rather than a bare id.
- `GET /api/chat/models?provider=X` — returns `source: "live"`, or
`source: "fallback"` plus the provider's error. Listing is *expected*
to fail sometimes: a scoped Anthropic key can be valid for /messages
and still 401 on /models, so the short built-in `FALLBACK_MODELS` list
keeps the dropdown usable and the UI says why it looks short.
- `PATCH /sessions/{id}` now accepts provider/model. Switching provider
resolves that provider's default model, since the old id is meaningless
on a new provider.
- `chat_message.provider` / `.model` (new migration) record which model
produced each assistant turn. Without them a transcript spanning a
mid-session switch would attribute every turn to whatever is selected
now. Bubbles show the per-turn model.
- Frontend: one shared `ModelSelect` used by both the New-chat panel and
the thread header, so they can't drift. A filter box appears once the
list exceeds 25 entries (OpenRouter).
Two latent bugs found and fixed while testing this:
- Both adapters caught `APIError`, but the SDKs raise their *base* class
(`openai.OpenAIError`) for missing credentials at client-construction
time — which escaped as an unhandled 500 from the new endpoint, and as
a raw SDK message from the streaming path. Both now catch the base.
- The Anthropic SDK raises a bare `TypeError` when no auth is
resolvable. Catching `TypeError` would swallow real bugs in our own
translation code, so the adapters check for a key up front and raise a
`LlmError` naming the provider and its env var.
Verified live: all four providers return 200 from /models and degrade to
the fallback list with an accurate reason.
703 tests pass (+17); ruff check, ruff format --check, and the playground
build are green. Single migration head.
Contributor
Author
Follow-up: provider + model as two dropdowns (a6185a7)Replaces the free-text model input with a real model dropdown, and makes provider and model switchable from the thread header at any time — previously both were fixed for a session's lifetime. Model options are fetched live from each provider, not hardcoded: a baked-in list rots on every model release and could never cover OpenRouter's ~370-model catalogue.
Reviewer notesThis overturns a documented decision from the first commit. The session originally pinned provider+model so a transcript was unambiguous about what produced it. To keep that property under mid-session switching, Second migration, deliberately. Two latent bugs in the first commit, found while testing this and fixed here:
Verification: 703 tests pass (+17), ruff check + format clean, playground build green. A fresh live chat turn could not be re-run for this commit: the ambient |
…lick chat First real use of the Chat assistant surfaced two bugs and two UX gaps. The bugs shared one root: the turn-completion handler trusted a cache-governed fetchQuery and then discarded the only live copy of what the user had just watched stream in. Streaming that sticks. The app-wide `staleTime: 30_000` made the completion `fetchQuery` resolve from cache with the *pre-send* transcript, so clearing `liveTurns` erased the reply — reappearing only after a refresh, which gave a cold cache. A per-call `staleTime: 0` forces the fetch. Verified against the installed @tanstack/query-core: without it the second fetchQuery performs no network call and returns the stale rows; with it, the new rows arrive and the cache updates before the live copy is dropped. Durable errors. `chat_message` gains a nullable `error` column (revision c7d4a1e8b3f2). `_flush_round(error=...)` now writes a row even with empty buffers, so a 401 before the first token no longer leaves the user message with no reply at all — previously the failure existed only as an SSE frame and died with the stream. The unconfigured-provider path persists too, which the plan had left out: it broke the same "no user message without a visible reply" contract stated in this module's own docstring. Thinking indicator. A transient `thinking` flag on the live-turn reducers fills the silent gaps before the first token (the system prompt builds from live repo facts after the HTTP head is sent) and between a tool result and the next round. Client-side by design — no new SSE frame, since the empty assistant turn is already on screen at click time, before any byte could arrive. URL-backed navigation. A ~110-line store<->URL sync module, no router: the store is already the single source of truth with one canonical mutation point per transition, and a router would invert that to URL-as-truth for two views. `/explorer?node=<qn>&file=<path>` and `/chat/<id>` now survive a refresh and back/forward. The SPA catch-all already served every GET path, so no backend change. Exercised against a stubbed window: pushState vs replaceState, popstate re-entrancy (a popstate-driven write regenerates the same URL and no-ops), `/` normalization, dead ids passing through unvalidated by design, and a generic-typed qualified name round-tripping. Chat UX. "+ New chat" creates immediately — the server already resolved every field from config, so the picker step was friction with nothing behind it; ProviderPicker is deleted. The provider/model dropdowns move to just above the composer and are owned by MessageThread, the only component that knows whether a turn is streaming (switching mid-stream would repoint the session row under the in-flight turn). Stable React keys replace index keys so a turn keeps its identity across the live->persisted swap. Also fixes ChatSession's docstring, which still claimed a session is never re-pointed at another model mid-conversation — untrue since a6185a7.
…l names
Turns were exhausting the 8-round tool budget and answering nothing. The
persisted transcripts (readable because the previous commit added the `error`
column) showed where the rounds went: six `get_symbol` calls on directory paths
in a single round, `get_commit {"sha": "HEAD"}`, `read_file` on a directory, and
`get_area_history {"path": "."}` returning zero commits.
Root cause was a tool-surface gap, not a bug. For "what have we developed
lately", every history tool needs an identifier the caller must already know — a
SHA, a PR number, a real file path — and get_repo_overview returns only counts.
So with 219 commits and 38 PRs indexed, the model had no entry point and fell
back to walking the tree with list_dir/read_file until the budget ran out.
list_recent_activity returns the newest commits, PRs, and issues in one call.
Compact by design — subject/title, author, date, description clipped to 240
chars — so it is an index to scan in one round, then drill down with
get_commit/get_pr on what matters. Commits are restricted to the first-parent
main walk, matching area-history and the refactor-walk: "what shipped" is a
default-branch question, and PR-origin commits recovered from squash merges
would double-count. The resource lives beside the other resource bodies to
reuse their session handling and OperationalError contract, but is not
registered as an MCP resource — the chat tool is its only caller, and WhyGraph's
MCP surface deliberately stays narrow.
A round-limited turn now always ends in prose. The loop dispatched the final
round's tools and appended the results to `messages`, then exited without
another provider call — so those results never reached the model and the turn
produced tool cards, an amber banner, and no answer. One more call with
`tools=()` leaves the model nothing to do but write up what it gathered. This
reverses a decision the parent plan stated explicitly (a forced no-tools turn
"is NOT attempted"), on the grounds that it only papers over a misbehaving
loop. Observed behaviour overruled it: these were not loops but ordinary broad
questions with no efficient tool, and the old choice shipped a turn that cost
eight calls and said nothing. RoundLimit's docstring carries the new rationale
and this history. Note the ceiling is now max_tool_rounds + 1 calls, and that
last one carries the turn's largest transcript.
Symbol tools documented the one name shape that cannot resolve. All three
`qualified_name` params said "Dotted symbol name" and the system prompt's own
worked example was whygraph://symbol/whygraph.chat.harness.run_turn. Verified
against a live index: `run_turn` resolves, the dotted path does not, and methods
are `Class::method`. So the docs taught a format that misses every lookup and
produces dead Explorer chips. One shared constant now names the three real
shapes and states that a dotted path will not resolve, with a test that fails if
it drifts back.
Verified live against `whygraph serve`: the question that previously burned all
eight rounds now answers in two (list_recent_activity, then six targeted
get_pr calls) with finish_reason=stop. With max_tool_rounds forced to 1, the
round_limit frame is followed by a 2085-char answer persisted as its own row.
Tool count 11 -> 12.
… project stats
The assistant had three jobs — planning, debugging, statistics — and measurably
failed at all three for the same reason: the specialized tools could not answer
the questions those jobs ask, so the model fell back to reading files or to
trusting developer commit prose. Four changes, no migration, no re-indexing, no
frontend work.
get_area_outline — CodeGraph indexes files, not packages, so "what's in this
subsystem" had no structural answer; a transcript showed the model try
get_symbol on six package paths, say "the files aren't indexed as packages",
and fall back to list_dir. CodeGraph.area() derives the answer from
nodes.file_path, grouped by file with line ranges, cross-linked to per-file
commit counts from commit_file_change. Degrades to a per-file map above the
symbol limit rather than truncating, because a truncated list hides what was
missed while a map redirects the next call.
find_changes — the debugging entry point. search_symbols matches symbol names
only, so a defect described behaviourally ("sessions vanish after a refresh")
was unreachable: nothing was keyed on content. Keyword and/or path filters over
the diff descriptions, reusing resolve_path_aliases so renamed history is not
silently lost, and folding in linked PR titles (1,967 chars across the repo)
but never PR bodies (77,420). Accepts a directory, which get_area_history
cannot.
Field authority — llm_description is generated from the diff alone, so a
careless commit message cannot contaminate it, but every serializer returned
subject first and nothing said which to trust. Reordered _commit_dict and
_hydrate_commit (key order only; the key set is asserted unchanged, so the MCP
contract the planner subagents rely on still holds) and added a shared
_DESCRIPTION_AUTHORITY constant to all five history tools.
run_project_stats — velocity, churn, hotspots, and cycle time were all already
in the schema with no tool to reach them. Read-only SQL behind four independent
layers: a mode=ro connection, a SQLite authorizer allowing only SELECT/READ/
FUNCTION over a frozenset table allowlist (chat_* and rationale_cache denied,
so the assistant cannot read its own transcripts), an aggregate-only shape
check, and row + progress-handler-deadline caps. Layers 1 and 2 are each tested
with the shape check bypassed — a bug in a string check must not be what stands
between the model and a DROP TABLE — and the DB is asserted byte-identical
after every hostile query. Ships an annotated schema description carrying four
measured silent-corruption rules (on_default_branch, strftime over substr,
julianday over string comparison, merged PRs are 'closed'), guarded by a test
so they cannot be trimmed to save tokens.
Two measurements corrected the design as written. Outline cost is ~130 chars
per symbol serialized, not the ~38 a values-only estimate suggested, so the
symbol limit is 150 rather than 500 — at 500 an outline would have been double
the 30,000-char result cap. And find_changes' row-count cap did not bound
payload size at all: limit=30 produced 30,014 chars, truncated mid-string into
JSON the model could not parse, so rows now assemble under a char budget and
report what they withheld.
Tool selection is measured, not assumed: scripts/eval_tool_choice.py drives
live providers through the HTTP surface and scores which tool gets reached
first. It found two real description bugs — list_dir said "use it to orient
yourself before reading files", and nothing said get_repo_overview has no
per-file breakdown. After fixing both, all three of the plan's selection
criteria pass on two models, with run_project_stats leaking onto a
non-statistical question 0 times across 20 questions. The lint gate now covers
scripts/, which pytest does not collect.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements
plans/chat-assistant-plan.mdPhase 1 — all seven rollout steps (§15.1–7).The playground gets a second top-level view: a chat assistant that answers questions about the repo from live tool calls against CodeGraph, the WhyGraph history, and the source tree, with every tool invocation visible in the UI.
Why this shape
The assistant calls the same plain functions the MCP server and the Explorer already call — in-process, no MCP protocol roundtrip. That's the point: its answers can't drift from the rest of WhyGraph, because there's one implementation per capability and three adapters over it (
mcp/,serve/, nowchat/).The chat port is parallel to
LlmClient, not a widening of it. The analyze/rationale path needs exactly one thing — synccomplete()returning text — and its frozen dataclasses have no notion of tool calls or incremental output. Adding streaming there would ripple through all five existing adapters for one new consumer. SoLlmClientand all five adapters are byte-identical in this PR.Zero new Python runtime dependencies. OpenRouter / OpenAI / DeepSeek share one OpenAI-compatible adapter; Anthropic gets its own; both SDKs were already direct deps.
What's here
Backend
services/llm/openrouter.py— OpenRouter as a first-class provider (soanalyze/rationalecan use it too), mirroring the DeepSeek adapter across config, factory, template, golden fixture, the init wizard, andOPENROUTER_API_KEYin all three shim env lines.services/llm/chat.py— the newChatClientport. Sync generators, not async: serve's documented contract is sync-def-in-threadpool, and both SDKs stream synchronously. One consumer, one thread, no event-loop coupling.services/llm/openai_chat.py/anthropic_chat.py— the hard part. Two very different wire dialects (fragmentedtool_callsargument deltas vstool_usecontent blocks with explicit block-stop) normalized to one event stream.chat/—tools.py(11 tool specs + a per-turnToolRegistry),files.py(the clamp),harness.py(run_turn+build_window),prompts/system.md. The harness is persistence-free — it takes messages and yields events, which is what makes the loop testable against a scripted fake client with no DB in the picture.serve/chat.py— session CRUD plus the SSE turn. Rows persist per completed round, so a disconnect loses at most the in-flight turn.ChatSession/ChatMessagetables + one migration, applied automatically by theensure_initialized()callcreate_appalready makes.Frontend
viewswitch + header segmented control (no router dep — matches the house idiom).components/chat/— session sidebar, thread with auto-stick, tool-activity cards (collapsed → expandable to arguments/result), composer with Stop viaAbortController, provider picker.whygraph://symbol/<qn>links render as chips that callopenNode()and jump into the Explorer.Security
The file tools are the only genuinely new attack surface, and they're deliberately paranoid: root clamp via
is_relative_to, plus a deny-list inside the root for.git/.whygraph/.codegraph/whygraph.toml/.env*— the live config holds real API keys and a transcript is a place secrets would be persisted. Binary refusal, 400-line / 100 KB caps. No write tools, no shell tool; the registry is a closed allow-list.The only nested LLM spend is
get_rationaleon a cache miss, capped per turn by[chat].max_rationale_generations(default 2) and runningwhygraph_rationale_briefverbatim — the same audited generate-and-cache flow the MCP tool and the Explorer's Generate button perform, using the[rationale]provider/model so the cache key can't fragment.Chat is absent from the MCP surface, which stays narrow per the CLAUDE.md invariant.
Verification
ruff check src/ tests/ruff format --check src/ tests/pytestnpm --prefix src/playground run buildVerified live against the real Anthropic API, not only in tests: one turn streamed 3 parallel
search_symbolscalls → results → prose; another triangulatedsearch_symbols→list_dir→read_file→get_symboland answered with a workingwhygraph://symbol/…deep-link. Transcript replay shows the correct row shape (user → assistant(3 calls) → 3 tool rows matched by id → assistant) with first-message titling applied.New test files:
test_llm_chat.py(both dialects incl. parallel calls, fragmented and malformed argument JSON, truncated streams),test_chat_tools.py(every AC #6 clamp case + the generation-budget accounting with a spy),test_chat_harness.py(loop termination, ordering, and the "never orphan a tool message" window invariant asserted structurally across the budget range),test_serve_chat.py(frame sequences, which rows land, in-band failures),test_services_llm_openrouter.py.Reviewer notes
Two deviations from the plan as written, both deliberate:
ChatConfig(§9) landed in step 4, not step 6 — the tool registry reads[chat].max_rationale_generations, so the config had to exist before it.chat_session.updated_atis not indexed. §6.2 specified onlyix_chat_message_session_id; autogenerate proposed anupdated_atindex and it was dropped for plan fidelity (a single-user tool never holds enough sessions to need it).Migration scope:
alembic --autogeneratealso wanted to dropix_commit_file_change_path_commit_shaand strip threeserver_defaults. That's pre-existing model↔migration drift, not part of this change, and dropping that composite index would regress area-history queries — so the migration was hand-finished to the two new tables only (verified zerocompare_metadatadiff). The drift is worth a separate focused PR.Two real bugs were caught and fixed during implementation, both worth knowing about if you touch this code:
_load_historyiterated rows afterget_session()'s commit expired them →DetachedInstanceErroron every turn. Conversion now happens inside the session._turn_framesdraft collapsed multi-round turns into a single assistant row and never persisted tool-result rows at all. Rewritten with an explicit per-round buffer.Not in this PR: Phase 2 charts (§12, deferred by design), and no syntax highlighting in chat code blocks (§0.1 #5 —
rehype-highlightis the designated follow-up). The manual smoke foropenai/deepseek/openrouter(AC #5) is outstanding — those three share the one adapter that is unit-tested against recorded streams, but no keys for them are configured locally.Local build note: the playground build needs Node 18+ (vite 5). Node 16 fails with
crypto.getRandomValues is not a functionafter tsc passes, which reads like a build bug but isn't — CI and the Dockerfile already pin Node 22.