fix(desktop): durably retain streamed assistant responses - #95822
JoaoMarcos44 wants to merge 4 commits into
Conversation
|
Topology reconciliation for #95514: this carrier is superseded by #95886 rather than a second landing owner. I re-read both live objects again. #95822 still carries the older split persistence shape ( Exact-head hosted receipts for #95886 are still green: CI The landing edge has moved since that review, so I am not transferring those green receipts to current main. Live Disposition: #95822 remains superseded and should stay out of the landing order; preserve its unique tests/history as provenance. #95886 owns delivery, subject only to current-main reconciliation plus fresh all-green exact-object proof. |
Summary
Fixes #95514
An empty terminal completion must not erase assistant text that was already streamed to the Desktop, and a streamed answer must remain durable when the turn settles after a tool result.
This PR closes both sides of that failure class without adding a second journal, changing prompt/tool caching, or copying the implementation from the related open PRs.
Confirmed root cause
The issue report is a real data-loss report, not only a rendering glitch: it records an empty assistant row in
state.dbafter a tool result. The exact remote Desktop build and logs were not supplied, so the original Windows/secondbrain run cannot be reproduced against its authenticated environment.The current code nevertheless contains a deterministic reproduction of the same loss:
message.deltaadds text to the Desktop's live assistant parts.message.completecan carry empty terminal text.mergeFinalAssistantText(parts, '')treated that empty value as an authoritative replacement and removed the streamed text.completeAssistantMessagethen allowed session hydration, so a stale/empty persisted row could overwrite the only visible copy.final_responsewhile the stream buffer contains text. If the tail was already flushed incrementally, the old fill path had no reliable live_row_idand could append a duplicate while leaving the empty row in SQLite.The invariant fixed here is:
Shift+Tabis a timing trigger rather than a direct Desktop transcript command: plainShift+Tabis not bound as a Hermes action;Ctrl+Shift+Tabis the session-cycle shortcut. Switching sessions can expose the empty-completion/hydration race, but it must not be able to remove durable assistant content.Fix
mergeFinalAssistantTexta no-op for empty or whitespace-only final text, preserving streamed text, reasoning, and tool timeline parts.hasVisibleAssistantTextpredicate and scope the hydration guard to the currentstreamId(or the current interim boundary), so a previous turn's text cannot suppress legitimate hydration for a reasoning-only turn._current_streamed_assistant_textinfinalize_turnwhen the terminal response is empty, before completion/result calculation and persistence.tool_calls.SessionDB.ensure_assistant_message_content, a parameterizedBEGIN IMMEDIATEtransaction that fills only an active blank assistant row and returns the durable canonical content. A concurrent winner is adopted into the live message and returned result instead of creating another row._row_idfrom the temporary batch row back to the live message dict so finalization can update an incrementally persisted row in place.No new state file, model tool, environment variable, toolset, or cache-invalidating behavior is introduced.
Causal flow
flowchart LR D[message.delta\nstreamed text] --> L[Desktop live parts] E[message.complete\nempty text] --> M[non-destructive merge] L --> M M --> H{local current text?} H -- yes --> N[skip destructive hydrate] H -- no --> R[normal hydrate] S[finalizer\nempty response] --> B[stream buffer recovery] B --> Q{persisted empty row id?} Q -- yes --> U[conditional in-place DB update] Q -- no --> A[normal append] U --> V[one canonical assistant row] A --> VNon-duplicate comparison
The following related work was inspected before implementation:
This PR owns the missing cross-layer enforcement point: stream buffer → finalizer → existing SQLite row identity, plus the shared Desktop merge contract. It does not copy those implementations.
Test plan
RED evidence
The new regressions were run before their corresponding production changes and failed for the intended reasons:
Focused GREEN results
scripts/run_tests.sh— 9 affected/adjacent Python files, 59 tests passed, 0 failed.npm exec --workspace apps/desktop -- vitest run --project ui src/lib/chat-messages.test.ts src/app/session/hooks/use-message-stream/interim-sealing.test.tsx src/app/session/hooks/use-message-stream/session-info-side-effects.test.tsx src/app/session/hooks/use-message-stream/delta-flush.test.tsx— 113 tests passed, 0 failed.SessionDBreopen test verifies the persisted final content and absence of a duplicate empty assistant tail.uvx ruff==0.15.10 checkon changed Python files — passed; an existing invalid# noqawarning atrun_agent.py:108remains.Known validation limitations
npm run --workspace apps/desktop typecheckis blocked by an existingorigin/mainerror atapps/desktop/src/app/contrib/hooks/use-background-sync.test.ts:660(updateSessionStateis missing from that test fixture); that file is not changed here. Typecheck passed before the rebase when the branch was based on9aa7530....npm run checkexceeded the local 420-second execution ceiling and has no final result; it is not reported as green.secondbrainprofile or productionstate.dbwas performed. The issue did not provide a Desktop build, logs, or a safe test account, so production data was neither accessed nor mocked.Hardening record
GitHub labels:
P1,type/bug,comp/desktop,platform/windows,area/sessions,sweeper:risk-session-state,sweeper:risk-platform-windows.Three required P1 hardening passes were completed:
Risk: Medium. The change is limited to empty-completion reconciliation and final assistant-row settlement. It uses the existing persistence lock/transaction machinery, preserves tool-call rows and session counters, and falls back to the existing append path when no row identity is available.