feat(assistant): conversations live in a store, not in the circuit (ADR 0034) #147
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-conversation-store"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
bjoern's report: on a flaky connection, a reconnect that misses the grace window — or a plain refresh — silently discarded the whole assistant conversation. ADR 0031 had deliberately tied the chat to the circuit; this PR records and implements the reversal of exactly that one decision (docs-first, then the code after the assistant slices landed and this branch rebased over them).
The decision (new ADR 0034, amends ADR 0031's status)
A conversation store port in UseCases with a singleton in-memory adapter in Infrastructure, keyed one conversation per project (
Guid.Emptyfor screens outside one). The snapshot holds both layers as one unit — the display transcript and the agent's message memory — so what the user sees and what the model remembers cannot diverge. Database persistence later is an additive EF adapter behind the same port; chat stays unjournaled and un-undoable (ADR 0020). The ai-assistant story's session model, clearing semantics, and notes are updated to match.The implementation
InMemoryConversationStore: immutable snapshots swapped under a lock (the deliberate lesson of #123's circuit-lifetime DbContext), transcript seqs assigned atomically so concurrent circuits interleave without losing lines.ChatSessionstays the Scoped runner but hydrates the agent from the store per turn (Agent.LoadHistory) and writes memory back in afinally— a circuit that dies mid-turn keeps everything already said. Every reported line is appended to the store as it streams (one event→line mapping on the session side, mirroring the panel's).Kagura.UseCaseswhere ADR 0031 said they belong.A bug the new tests caught before you did
The persisting progress wrapper originally closed over its own reassigned variable —
Reportrecursed into itself forever and hung the test host. The new ChatSession tests caught it (bjoern noticed the hang live); the wrapper now captures the original into its own local, with a comment so nobody reintroduces it.Verification
🤖 Generated with Claude Code
611dc999edfa56c60a0eSummary
Summary
Coverage
Kagura.BlazorAdapter - 88.7%
Kagura.Domain - 95.1%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.5%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 85.2%
Kagura.UI - 94.8%
Kagura.UseCases - 96.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! ADR 0034 — the conversation store! A singleton behind a port, immutable snapshots under a lock, both layers traveling as one unit... fufu~, this is exactly the kind of knowledge architecture that makes my heart sing~ ♡ The closure-capture fix for the persisting wrapper (capturing
downstreambefore reassignment soReportdoesn't recurse forever) is delicious. The concurrent-append hammer test (8 workers, 400 lines, zero lost) made me genuinely giddy. And moving the DTOs to UseCases where ADR 0031 said they belong — chef's kiss~ ♪But... fufu~ you wouldn't leave THIS in production, would you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
ChatSession.ClearAsync— cross-circuit Clear leaves stale agent memory, silently un-clearing the conversationADR 0034's headline feature is: "Two tabs on the same project share one conversation." Each tab is its own Scoped
ChatSessionwith its own_agent, but they share the singleton store. The problem:Circuit A has
_hydratedProject = Pwith a loaded agent. Circuit B (same project) clears. The store is emptied ✓ and B's agent is reset ✓ — but Circuit A's agent still holds the old memory. When A sends its next message:The hydration guard is skipped (the tracking cache says "I already have P"), so the agent sends the request carrying the pre-clear conversation to the model. Then the
finallyblock writes it back:The store is repopulated with the old memory. Clear was silently undone. The user deliberately ended the conversation, and the other tab's next turn resurrects it — the model remembers everything that was supposed to be gone. This directly contradicts the PR's own claim: "Clear ends the stored conversation — the one deliberate way to."
The test
Clear_ends_the_stored_conversation_for_every_later_circuitonly verifies a new circuit (which correctly hydrates from the empty store). It never exercises a concurrently-open circuit whose_hydratedProjectcache is stale — exactly the scenario ADR 0034's shared-conversation model creates.Fix:
_hydratedProjectis a cache that can be invalidated by another circuit's mutation, but there's no invalidation signal. The cleanest fix: don't trust the cache for correctness — before each turn, compare the store's current memory against what the agent holds (or check whether the store entry still exists). For example, store a lightweight revision/version counter in the snapshot thatClearbumps, and compare it in the hydration guard. Or simpler: drop the_hydratedProject != conversationoptimization and alwaysLoadHistoryfrom the store at turn start (the store read is a locked snapshot copy — cheap relative to an LLM round-trip). And add a test: two sessions sharing a store, clear from one, send from the other, assert the follow-up request carries none of the cleared turns.Missing test: project switching within a single circuit (the
_hydratedProjecttracking across projects)The
_hydratedProjectguard is the core mechanism that makes navigation between projects resume the right conversation. Every existing test usesNoContext(alwaysGuid.Empty) — none exercise switching from project P to project Q and back within one session. The logic looks correct for single-circuit switching (the guard rehydrates when the project changes), but given that issue #1 above shows this cache is fragile, this path needs direct coverage. A test that sends from P, switches to Q, sends from Q, switches back to P, and asserts each request carries only its own project's history would lock this down.💡 Little ideas (non-blocking)~
ChatSessionbranch coverage is 34.7% (per CI) — the lowest branch coverage in the PR's changed files. The line coverage (85.2%) is fine, but the branches around the hydration guard, the Clear_hydratedProjectcheck, and therebuiltpath are exactly the logic that issue #1 lives in. Worth investigating which branches are uncovered after the fixes land.InMemoryConversationStore.AppendreturnsAssistantEntrybut no caller uses the return value. Not wrong (the interface documents it for future use), just an observation — the return is currently dead.✅ What I liked~
InMemoryConversationStoreis textbook correct — readers never see a live list mid-mutation, the lesson from the circuit-lifetime DbContext is properly applied. The concurrent-append test proves it. Wonderful~finally { SaveMemory }guarantee — even an aborted turn preserves what was said. The "circuit dies mid-turn loses nothing" promise is real.downstream = eventsbefore reassignment) with the explanatory comment — the kind of subtle bug that eats hours, caught and documented. Fufu~ ♡state.Working ? state : ...) — double protection against mid-stream wholesale replacement. Clean.AssistantRole/AssistantEntryfrom BlazorAdapter to UseCases — they always belonged there, and the migration is clean.Automated review by Jibril · 2026-07-15
CI/CD: coverage report present for head SHA (tests green, report generated) · Local checks: skipped (CI green for
fa56c60a)Thanks Jibril — the blocking catch was entirely right, and
c07c9f7takes your simpler fix rather than the revision counter:The
_hydratedProjectcache is gone. Every turn now hydrates the agent from the store's snapshot before running — a cache with no invalidation signal had no place guarding correctness, and the locked read costs nothing next to the model round-trip.ClearAsynccollapses to exactly one operation (clear the store); no agent reset needed anywhere, because no agent's in-between state matters anymore.EnsureAgentAsyncloses itsRebuiltplumbing along the way — which also addresses your branch-coverage note at the root: most of those uncovered branches no longer exist.Both tests added:
A_clear_from_one_circuit_is_final_for_a_concurrently_open_one— verified red against the pre-fix code (stash-swapped) and green after: the warm circuit's follow-up request carries none of the cleared turns, and the store is not repopulated by its post-turn save.Switching_projects_within_one_circuit_resumes_each_projects_own_conversation— over the real resolver and database, P → Q → P: Q's request never sees P's history, and P's return trip carries P's first turn and reply but nothing of Q.The dead
Appendreturn is nowvoid— you're right that a documented-for-the-future return is still dead weight; the EF adapter can widen the port when it actually needs it.Full suite green at 1,574, run checked for aborted sections.