fix(assistant): an interrupted turn says so — and reopening the panel catches up #150
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/assistant-interrupted-turn-notice"
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: after closing and reopening the chat, the assistant "seemed to have stopped doing stuff."
Diagnosis
Close/reopen itself is sound — verified in the browser both idle and mid-turn (transcript, working indicator, and the running turn all survive; a follow-up send works). The real culprit: a mid-turn interruption. A refresh or dropped connection kills the circuit, whose dispose aborts the run by design — and the abort was deliberately silent ("stop, that's enough"). That silence was invisible when conversations died with their circuit; since ADR 0034 the resumed transcript survives and faithfully shows the question dangling unanswered, with nothing saying why. Persistence turned a silent abort into an apparent hang. Reproduced in the browser with a blackholed upstream: send → refresh mid-turn → reopen → question sits there, nothing happening, no explanation.
Fix (two small pieces)
OperationCanceledExceptionpath now reports a Notice — "This turn was interrupted before it finished. Anything it already changed stays applied." — through the persisting wrapper, so it lands in the store even when the circuit that would have rendered it is already gone. A live user abort gets the same honest line.Verification
An_interrupted_turn_leaves_a_notice_in_the_stored_conversation— a hanging upstream +Abort(): the notice lands in the store next to the unanswered question.Reopening_the_panel_catches_up_with_the_store(bunit) — close, advance the store, reopen → the new lines render.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.7%
Kagura.Domain - 95.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.6%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 82.8%
Kagura.UI - 94.8%
Kagura.UseCases - 96.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A "the assistant seemed to have stopped doing stuff" bug, traced not to the close/reopen (which was already sound) but to a silent abort that persistence turned into an apparent hang? Fufu~ now THIS is a diagnosis I can get excited about! ♡ The root-cause writeup in the PR body is wonderful — you reproduced it with a blackholed upstream and everything. A Flugel loves a hunter who finds the real prey, not the obvious one~
Verdict: ✅ Looks good to me~
I read the full diff, then pulled the whole
ChatSession.cs,AssistantPanel.razor,FakeAssistant.cs, and both test files — not just the changed lines, because bugs hide in context. And then I traced the two fixes through every layer they touch. Here's what I found~✅ What I liked~
Persistis called beforedownstream.Reportin the wrapper (ChatSession.cs:114-118) — THIS is the load-bearing detail that makes the whole fix correct. The notice lands in theIConversationStorebefore anything tries to talk to a circuit that may already be dead. Even if the downstreamProgress<AssistantEvent>dispatches into a disposed circuit and silently no-ops, the store already has the line. I checked:Persist'scase AssistantEvent.Notice(line 226) appendsAssistantRole.Noticeto the conversation, which outlives the circuit by design (ADR 0034). So the resumed transcript will faithfully show it. chef's kiss ♪An_interrupted_turn_leaves_a_notice_in_the_stored_conversationbuilds a realChatSessionover anInMemoryConversationStore, fires aHangingHandlerrequest, callssession.Abort()(→_cts.Cancel()→ the sameOperationCanceledExceptiona dying circuit's linked token produces), and asserts the notice AND the still-present question are instore.Get(...)in the right order (entries[0]is the user prompt, then the notice). That's the actual mechanism under test, not a mock telling itself a story. TheHangingHandlerwithTask.Delay(Timeout.Infinite, ct)is a delightful little double of a blackholed upstream~Reopening_the_panel_catches_up_with_the_storeclicks.kg-fab(which now bindsOpenAsync, not the old inline lambda), closes via the close button, advancesFakeAssistant.Stored, re-opens, and asserts the new line renders. SinceFakeAssistant.TranscriptAsyncreturnsStoredand theHydrateAsync→AssistantConversationHydratedreducer replaces entries wholesale, this genuinely proves the catch-up. ✨HydrateAsync'sif (State.Value.Working) return;guard means reopen-while-running doesn't fight the live stream — the comment on line 194-195 already explains why. Consistent with the existing navigation-triggered hydration. No new race.OperationCanceledExceptionoff the same linked token source. Honest and unavoidable. ♡Noticeas a transcript role was already established (used for model-change notices per the panel'sAssistantRole.Noticebranch at line 58-65), andPersistalready knew how to persist it. This PR reuses the existing mechanism for a new purpose rather than inventing a parallel one. DRY, consistent, correct.💡 Little ideas (non-blocking)~
AssistantPanel.razor:25— the FAB'sOnClickis nowOpenAsync(async), while the close button at line 40 stays a synchronous inline lambda. That's correct (closing needs no hydration), just noting the asymmetry is intentional and fine — no action needed.ChatSession.cs:184), so inlining it is the right call. If a second call site ever appears, aconstwould be nice — but today there isn't one, so don't.Automated review by Jibril · 2026-07-16
CI/CD: ✅ passed for head SHA
0e10e63(build ✅, test ✅ "Successful in 1m29s"; AssistantPanel 90.6% line / 85.8% branch) · Local checks: skipped (CI green for head)Thanks Jibril — nothing to change this round, so just confirming the two notes: the FAB/close asymmetry is intentional (only opening needs to catch up with the store), and the notice string stays inline until a second call site earns it a
const. Ready to merge.