Repository navigation
feat(chat): server-side conversations, history and query condensing - #9
Draft
VampiricCyborg wants to merge 11 commits into
Draft
VampiricCyborg wants to merge 11 commits into
VampiricCyborg wants to merge 11 commits into
Conversation
Chats have lived only in each browser's localStorage, so a thread vanished on logout and never followed the account to another machine. These two tables move them server-side. Both are new: nothing existing is altered, so the `alembic upgrade head` that railway.json runs on every boot cannot disturb documents, chunks or users. `downgrade()` drops them, which is the only possible inverse of creating a table and therefore discards saved conversations -- the reason the upgrade direction was kept additive in the first place. The ORM models were drafted on the stale local branch feat/conversation-persistence and are ported by hand rather than merged: that branch predates a9f3d6c1b5e8 and still declares EMBEDDING_DIM = 768, so taking its models.py wholesale would revert the vector column against a 384-dim database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChatRequest.conversation_id was ignored and LLMRequest had nowhere to put prior
turns, so the model saw every follow-up as a first message. LLMRequest now carries
a trimmed, strictly-alternating history list and all five providers replay it.
None of them could take a message list before -- each collapsed the exchange into
a single user turn:
groq, openai had a private _build_messages; now share build_chat_messages
anthropic built a one-element messages list; system stays a top-level arg
gemini glued system + context + question into one string; now passes a
real contents list with system_instruction, mapping the
assistant role to Gemini's "model"
ollama posted to /api/generate, which has no messages parameter at
all; switched to /api/chat
Also adds the versioned query-condensing prompt and app/llm/condenser.py, which
rewrites an elliptical follow-up into a standalone retrieval query, and
StreamCapture, so a caller can persist what was streamed without parsing its own
SSE frames back into text.
Behaviour change: with no retrieved context the user turn is now the message
alone. It was previously "\n\nQuestion: <message>" in LLM mode, because the
context f-string was unconditional.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Routes: list (paginated), create, get-with-messages, patch title/pinned/mode,
delete, patch message feedback, and a one-time /conversations/import for chats
still sitting in a browser's localStorage.
Ownership is enforced in the service layer, which folds user_id into every WHERE
clause, so a handler cannot forget it. Another user's conversation id answers 404,
never 403: 403 would confirm the id is real and someone else's, which is a
membership oracle an attacker could enumerate.
/chat now:
- creates a conversation when none is given, and emits `event: conversation`
with its id as the first SSE frame
- rejects an unknown or foreign conversation_id with 404 *before* streaming
starts, because once StreamingResponse is returned the status is already 200
- persists the user message, then loads the last chat_history_turns exchanges
under chat_history_max_tokens -- a budget deliberately separate from
llm_max_context_tokens, so a long conversation cannot crowd out retrieved
passages
- condenses an elliptical follow-up into a standalone query for RETRIEVAL ONLY;
the original message still goes to the answer prompt, and both are logged
- persists the assistant message when the stream ends, and on client disconnect
saves what was generated with status=partial, using a fresh session from the
sessionmaker because the request-scoped one is closed by then
- re-answers in place on `regenerate` instead of appending a duplicate turn
Titles come from the first user message, truncated. No extra LLM call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tial saves
Integration tier (real PostgreSQL):
- every conversation route is scoped to the caller, and a foreign id answers
404 with the row left intact -- proving the 404 is a refusal rather than a
delete that reported oddly
- prior turns reach the provider. Asserted on the message list a recording
provider is handed, not on the fact that history was loaded: a test of the
latter would still pass while a provider silently dropped it, which is the
bug this phase fixes
- a condenser timeout retrieves on the user's literal words, and a successful
rewrite reaches retrieval while the answer prompt still sees the original
- a client that walks away mid-stream leaves a `partial` assistant message
Unit tier (no services): turn pairing, title truncation, the wire format each of
the five providers builds from one history list, and the condenser's fallbacks
for timeout, provider error, an empty reply and a reply long enough to be an
answer rather than a query.
tests/test_health.py's mocked chat test now stubs the persistence seams. It runs
in the unit tier where there is no database, and /chat genuinely needs one now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--mode followup` runs the two-turn slice three ways over the same items, so the columns are comparable: raw the user's literal follow-up -- what shipped before model_rewrite app.llm.condenser's output, the deployed path reference_rewrite the slice's hand-written rewrite, a ceiling for the corpus Reports Recall@1/3/5 at document level plus rewrite latency percentiles and a count of condenser outcomes, so a run where most rewrites timed out cannot be mistaken for one where condensing did not help. Ground truth in this slice names documents by manifest id, never by database document_id, which is regenerated on every ingest; the join is through the filename the corpus was ingested under. All 15 ids resolve against the manifest. The condenser needs an assistant turn to resolve pronouns against. Generating one would put a second non-deterministic LLM call inside a measurement, so the top retrieved passage from turn 1 stands in, and the result file records that it is a proxy rather than a written answer. Also drops the eager conversation_service re-export from app/services/__init__.py. It imports app.llm.models, which runs app/llm/__init__.py, which imports the retrieval pipeline -- and app.retrieval reaches app.services through app.ingestion. Importing app.retrieval first, as this harness does, hit the loop and failed on a partially initialised module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TanStack Query now owns the sidebar list and each conversation's messages;
Zustand keeps only UI state -- sidebar open, the mode a not-yet-created chat will
start in, and the answer currently streaming. The streamed text is deliberately
not in the query cache: it is not server state until the stream ends, at which
point the conversation refetches and the buffer is dropped, so what stays on
screen is the row the server stored, with the real message id that feedback and
retry need.
A new chat no longer invents an id. Sending with no conversation_id makes the
server create one and announce it on the stream; the client then rewrites the URL
with window.history.replaceState. router.replace would move from /chat to
/chat/[id] -- a different route, so the page unmounts and the answer being
streamed into it disappears mid-sentence. Next.js 16 supports the History API
directly and syncs it into usePathname, so both chat routes now render the same
component and read the id from the path rather than from route params.
Pin, rename, delete and feedback are mutations against the server, optimistic
where the gesture is direct manipulation. Retry posts regenerate=true, and the
server replaces its previous answer in place instead of the thread growing a
duplicate turn.
Chats saved by the previous version are uploaded once from
docuquery-chat-store:<userId> to /conversations/import, and the local key is
deleted only after a 2xx -- the endpoint is not idempotent, so a key removed
early would either lose the history or duplicate it on retry.
Side effects worth calling out:
- "New chat" no longer creates a row. The old store wrote one on every click,
which is where empty "New Chat" entries in the sidebar came from.
- Mode is a property of the conversation, so changing it on an open chat is a
PATCH; on a new one it is remembered locally until the first message.
- An answer the server marked `partial` (the browser went away mid-stream) is
shown as real text with a note that it was cut short.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…es local Two defects found reviewing the streaming path. The assistant message's status started at "error" and was only ever corrected to "complete" or "partial", so a client that hung up before the first token left an empty row filed as a generation failure -- a closed tab reported as something that never worked. Completion, failure and disconnect are now three separate outcomes, and a disconnect with nothing generated writes no row at all. The bigger one is in the fixtures. /chat saves the assistant message from the streaming generator's `finally`, where the request-scoped session is already closed, so it opens its own from AsyncSessionLocal. Overriding `get_db` does not touch that sessionmaker: it still points at the application's DATABASE_URL. Every assistant message an integration test provoked would have gone to the developer's own database rather than the disposable one -- invisible to the assertions, and writing into a database this suite was never pointed at. The `client` fixture now redirects it at the test engine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…uffer A provider error is recorded server-side as a message with status "error". The client was keeping its own error bubble as well and then refetching the stored one, so the same failure appeared twice. The buffer is now cleared on failure too, and the server row is what stays on screen; where the request never reached the server there is no row, and the toast is the feedback. The in-flight buffer also rendered into whatever conversation happened to be open while its id was still unknown. Navigating into an existing thread mid-stream therefore showed a different chat's tokens. It now requires both ids to be null, not just the stream's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four fixes to the tests themselves, none to the product. The `client` fixture built its sessionmaker from `db_session.get_bind()`, which on an AsyncSession returns the *sync* Engine proxy that `async_sessionmaker` rejects. The engine is now its own fixture and both take it from there. The two condensing tests stubbed retrieval with an empty result, which sends docuquery mode into NoContextError before the provider is ever called -- so they were measuring the guard, not the query. The shared stub returns one real chunk. The recording provider also keeps `generate` and `stream` calls in separate lists: during a streaming chat `generate` is the condenser and `stream` is the answer, and one combined list made "what did the answer prompt see?" ambiguous. The disconnect test could never have passed as written, and worse, it would have passed for the wrong reason once the timing changed. httpx's ASGITransport buffers the whole response body before yielding a single line, so under it the application always runs to completion and there is no mid-answer disconnect to detect -- the route recorded an ordinary `complete` message. It now runs against uvicorn on a real ephemeral port, reads one token frame and closes the socket, and asserts the stored message is `partial` and holds only the first token. Also points the conftest header at port 5434, matching CLAUDE.md. 160 passed locally (114 unit, 46 integration) against pgvector/pgvector:pg16. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turn-2 Recall@5 at document level, 15 items, same index for every variant: raw 0.533 model_rewrite 1.000 reference_rewrite 0.933 All 15 condensing calls returned a usable rewrite; latency P50 194ms, P95 1121ms. Read with the caveats in the PR, not on its own. The index here is the 15-document subset the follow-up slice actually references, not the full 27-document corpus -- the full ingest was stopped twice by the machine running out of memory. Fewer distractors inflate absolute recall for every variant, so the columns are comparable to each other but not to a full-corpus run. n=15 also means one item is worth 6.7 points, and evaluation/sets/manual.json is still stamped DRAFT with the follow-up slice unverified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Renames the three query-condensing settings to the name the feature is actually called by: chat_condense_enabled -> chat_query_rewrite (default stays true) chat_condense_max_tokens -> chat_query_rewrite_max_tokens chat_condense_timeout -> chat_query_rewrite_timeout All three are new in this PR and unset in every environment, so nothing depends on the old names. Renaming only the flag would have left the group half-consistent. The default stays true. The n=15 smoke run does not settle it either way, but retrieving on an unresolved pronoun is a known failure mode rather than a hypothesis, and every fallback path -- timeout, provider error, empty reply, a reply long enough to be an answer -- has a test. Also adds scripts/persistence_check.sh and its transcript: three cookie jars for one account, proving a chat created in one browser is readable in another and survives a logout/login round trip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 8 — server-side conversations
ChatRequest.conversation_idwas accepted and ignored. Chats lived only inlocalStorage, the LLM never saw prior turns, and retrieval ran on the literalfollow-up text. This makes conversations server state, gives the model history,
and rewrites elliptical follow-ups before they reach the vector search.
Base is
chore/phase-0-baseline, notmain— the integration tests here build onthe fixtures that phase 0 adds to
tests/conftest.py. Review this diff on its own;GitHub shows only the phase-8 commits against that base.
Prior work
A local-only branch
feat/conversation-persistence(c445b69, never pushed) held afirst draft of the two ORM models under the commit message "chore: refresh Railway
deployment". Its column layout is carried forward here. The branch itself is not
merged: it forked before
a9f3d6c1b5e8and still declaresEMBEDDING_DIM = 768, sotaking its
models.pywould revert the vector column against a 384-dim database.The two classes were ported by hand instead.
Turn-2 retrieval — raw vs. rewritten (SMOKE RUN — NOT A RESULT)
Turn 2 of each two-turn item, retrieved three ways against the same index in the
same run. Document-level relevance;
reference_rewriteis the slice'shand-written rewrite and stands as a ceiling.
Turn-2 Recall@5: 0.533 -> 1.000. Eight of fifteen items flip from miss to hit.
All 15 condensing calls returned a usable rewrite (
{'ok': 15}— no timeouts, nofallbacks).
That latency lands in front of retrieval, so it is added to time-to-first-token on
every follow-up.
chat_condense_timeoutis 6s, well above P95.What the condenser actually produced:
Read this table with four caveats
documents the follow-up slice references. The full ingest was stopped twice by
the machine running out of memory. Fewer distractors inflate absolute recall for
every variant, so the three columns are comparable to each other but not to a
full-corpus run.
is well clear of that; the 0.067 by which
model_rewriteexceedsreference_rewriteis one item and means nothing.evaluation/sets/manual.jsonis stamped"DRAFT - not reviewed, not used to produce any reported number.", and its ownverificationblock records the follow-up slice as not mechanically verified — unlike the
unanswerable slice, whose ground truth was checked against the corpus text.
BASELINE.mdin the repository to put a before/after columnagainst. This comparison is internally A/B — every variant runs over the same
items against the same index in one run — so it does not need one, but the
standing rule in CLAUDE.md asks for a baseline and it is worth saying plainly
that none exists yet.
The turn-1 assistant reply the condenser resolves pronouns against is a proxy: the
top retrieved passage from turn 1, truncated. Generating a real answer would have
put a second non-deterministic LLM call inside the measurement. The result file
records this.
Does a chat survive a different browser and a logout?
Three cookie jars for one account, against a real server (
DEBUG=false, a realAUTH_SECRET, PostgreSQL 16 + pgvector). Nothing is shared between the jars butthe credentials — no
localStorage, no in-memory state, separate cookie stores ondisk. Jar A creates the chat; jar B is a second browser that is never told about
it; jar C is jar B after logging out and back in.
If conversations were still client-side, B and C would both be empty. Reproduce
with
backend/scripts/persistence_check.sh; transcript also committed atbackend/evaluation/results/persistence_check_phase8.txt.Full transcript
The last step is the ownership check from the other side: a second account sees
total=0and gets 404, not 403, on the conversation id.A browser-driven check is still outstanding — the Playwright MCP server failed to
connect this session (
CONNECT_TIMEOUT), and I did not substitute a scratchbrowser harness for it.
Migration
e7c4b9d2a6f1addsconversationsandmessages. Purely additive: two new tablesand their indexes, nothing existing altered.
downgrade()drops both, which is theonly possible inverse of creating a table and therefore discards saved
conversations — which is why the upgrade direction was kept additive.
railway.jsonrunsalembic upgrade headon every boot, so merging this PR runsthis migration against production. Round trip evidence below.
upgrade -> downgrade -> upgradeagainst pgvector/pgvector:pg16 on port 5434:upgrade headbtree (user_id, updated_at DESC)andbtree (conversation_id, created_at)present; both FKsON DELETE CASCADEDELETE FROM usersremoved the conversation and its messagesdowngrade -1usersrow survived;alembic_versionback tod5e2c8a7f1b3upgrade heade7c4b9d2a6f1The surviving
usersrow is the point: it makes "additive" a checked fact ratherthan a claim in the docstring.
Tests
160 passed locally — 114 unit, 46 integration — against pgvector/pgvector:pg16.
What changed
Backend
conversations(id, user_id FK cascade, title, mode, pinned, created_at, updated_at)indexed on
(user_id, updated_at DESC);messages(...)indexed on(conversation_id, created_at).app/api/conversations.py: list (paginated), create, get-with-messages, patch(title/pinned/mode), delete, patch message feedback, and a one-time import.
Ownership lives in the service layer, which folds
user_idinto everyWHEREclause. A foreign id returns 404, never 403 — 403 would confirm the id is real
and someone else's, which is a membership oracle.
/chat: creates a conversation when none is given and emitsevent: conversationas the first SSE frame; 404s an unknown or foreign id before streaming starts;
persists the user message; loads the last
CHAT_HISTORY_TURNS(6) exchanges underCHAT_HISTORY_MAX_TOKENS— a budget deliberately separate fromllm_max_context_tokens, so a long conversation cannot crowd out retrievedpassages; drops whole turns from the oldest end, never half a turn.
app/llm/condenser.py, versioned prompt inapp/llm/prompts.py),behind
CHAT_QUERY_REWRITE, defaulttrue, withCHAT_QUERY_REWRITE_MAX_TOKENS=80andCHAT_QUERY_REWRITE_TIMEOUT=6s. The rewrite feeds retrieval only; the originalmessage goes to the answer prompt. Both are logged. Timeout, provider error, an
empty reply or a reply long enough to be an answer all fall back to the raw message.
text is saved with
status=partial, from a fresh session opened inside thegenerator's
finally— the request-scoped session is closed by then.Providers — none of them could take a message list. Each collapsed the exchange
into a single user turn, so "verify every provider handles a multi-message list"
turned out to mean rewriting all five:
_build_messages, one user turnbuild_chat_messagesmessageslistsystemstays top-levelcontentslist,system_instruction, assistant role mapped to"model"POST /api/generate— nomessagesparameter at allPOST /api/chatFrontend
UI state only: sidebar, draft mode, and the in-flight answer. The streamed text is
not in the query cache — it is not server state until the stream ends, at which
point the conversation refetches and the buffer is dropped, so what remains is the
stored row with the real message id that feedback and retry need.
conversationevent gives a new chat its server id, and the URL is updated withwindow.history.replaceState.router.replacewould move/chat→/chat/[id]—a different route, so the page unmounts and the streaming answer vanishes
mid-sentence. Next.js 16 supports the History API directly and syncs it into
usePathname; both chat routes now render the same component and read the id fromthe path rather than from route params.
is direct manipulation. Retry posts
regenerate=trueand the server replaces itsprevious answer in place instead of appending a duplicate turn.
docuquery-chat-store:<userId>is POSTed to/conversations/import(max 200, validated with the same schemas), and the localkey is deleted only after a 2xx — the endpoint is not idempotent, so a key
removed early would either lose the history or duplicate it on retry.
Behaviour changes worth a second look
"\n\nQuestion: <msg>"in LLM mode, because the context f-string was unconditional.LLM_PROVIDER=ollamais now hitting/api/chat.is where empty
New Chatsidebar entries came from.PATCH.partialrenders as real text with a note that it wascut short.
app/services/__init__.pyno longer re-exportsconversation_service. It importsapp.llm.models, which runsapp/llm/__init__.py, which imports the retrievalpipeline — and
app.retrievalreachesapp.servicesthroughapp.ingestion.app.mainimported in an order that hid the cycle; the eval harness enters throughapp.retrievaland hit it.Tests
Integration (real PostgreSQL): ownership isolation on every conversation route, with
a foreign
DELETEleaving the row intact so the 404 is provably a refusal; historyreaching the provider — asserted on the message list a recording provider receives,
because a test that only checked history was loaded would still pass while a
provider silently dropped it; condenser timeout falling back to the raw query; and a
client disconnect leaving a
partialmessage.Unit (no services): turn pairing, title truncation, the wire format each of the five
providers builds, and every condenser fallback.
Out of scope
Summarising long histories. Truncation is enough for now.
🤖 Generated with Claude Code