Skip to content

fix(desktop): preserve pre-tool assistant text on turn complete - #69202

Open
erict16 wants to merge 2 commits into
NousResearch:mainfrom
erict16:fix/desktop-preserve-pretool-assistant-text
Open

erict16 wants to merge 2 commits into
NousResearch:mainfrom
erict16:fix/desktop-preserve-pretool-assistant-text

Conversation

@erict16

@erict16 erict16 commented Jul 22, 2026

Copy link
Copy Markdown

What does this PR do?

Desktop was dropping the first half of multi-step assistant turns once the turn finished.

You would see the model narrate, call tools, answer - then on message.complete the pre-tool narration vanished. It was still in state.db and came back after restart. Display-only bug.

mergeFinalAssistantText replaced every text part in the live bubble with the complete payload. That payload is usually only the last model reply. When earlier narration shared the bubble with tools and message.interim never sealed it first, the pre-tool half got wiped.

Related issue

Fixes the pre-tool narration wipe path described on #46606 (see mxcrowe's comment).

Does not claim to fix pure long-message head truncation / live render bounds. Separate problem.

Type of change

  • Bug fix

Changes

  • apps/desktop/src/lib/chat-messages.ts - segment-aware mergeFinalAssistantText: tool/image parts seal earlier text; only the open trailing segment is replaced by the final reply. Full-transcript dedupe only when the final is the sealed prefix itself or a real long-enough prefix continuation (no bare substring match, so short "OK." openers do not wipe the turn).
  • Unit tests for multi-tool segments, empty final, full-transcript dedupe, short-opener false positive, mid-string mention false positive.
  • Stream test: deltas + tools without message.interim, then complete - both halves stay.

Image-echo stripping stays on the caller (use-message-stream), same as main today.

Why this instead of #53350

Same bug class. Different shape:

#53350 this PR
Hook new reconcileCompletedTextParts + complete path extends existing mergeFinalAssistantText
Fits current main older use-message-stream.ts layout matches today's index.ts choke point
No-interim path yes yes + stream test
Multi-tool sealed segments partial explicit test
Short-prefix false positive n/a (always keeps head) guarded (min length + real prefix)

Happy to close this if you prefer #53350's shape, or pull any of their cases over. Just pick one.

How to test

cd apps/desktop
npx vitest run src/lib/chat-messages.test.ts src/app/session/hooks/use-message-stream/interim-sealing.test.tsx

51 passed here.

Manual: model that narrates, tools, then answers. Pre-tool text should still be there after the turn settles. Plain text-only turns should still collapse to the final reply.

Checklist

message.complete only carries the last model reply, but mergeFinalAssistantText
replaced every text part in the live bubble. Multi-step turns without a reliable
message.interim seal lost earlier narration until restart (still in state.db).

Seal text before tool/image boundaries and only supersede the open segment.
Falls back to full replace when the final already restates sealed narration.

Fixes NousResearch#46606
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation labels Jul 22, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to #53350 and #61447, which address the same Desktop completion-time narration loss. This patch uses segment sealing and a full-transcript dedupe fallback; the existing alternatives preserve earlier segments with different reconciliation semantics, so this needs a maintainer choice rather than a duplicate label.

@erict16

erict16 commented Jul 24, 2026

Copy link
Copy Markdown
Author

Follow-up push on the dedupe path.

Earlier version used a bare substring check (final contains sealed text). That was too loose - a short sealed opener, or a phrase the final merely quotes, could force a full text replace and drop multi-step narration again.

Now full-replace only when:

  • final equals the sealed prefix, or
  • final starts with the sealed prefix, sealed is at least 20 chars, and the rest looks like a real continuation

Added tests for short "OK." openers and mid-string mentions.

Vitest: chat-messages.test.ts + interim-sealing.test.tsx - 51 passed.

Still sitting on needs-decision next to #53350. This PR patches the current mergeFinalAssistantText choke point on main; #53350 adds a parallel helper. Either is fine - I'd rather one lands cleanly than both sit.

Drop bare substring matching. Short openers like "OK." were enough to
trigger a full text wipe when the final reply happened to contain them.
Require an exact match or a real long-enough prefix continuation.
@erict16
erict16 force-pushed the fix/desktop-preserve-pretool-assistant-text branch from fe46b04 to 91ee934 Compare July 24, 2026 12:17
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused fix. The reported live-path loss is present on current main: mergeFinalAssistantText removes every text part at apps/desktop/src/lib/chat-messages.ts:230-254, while tool events deliberately flush prior deltas into the same bubble at apps/desktop/src/app/session/hooks/use-message-stream/index.ts:368-400; completion then invokes that helper at :495-512.

The proposed last-boundary segmentation addresses that exact state shape without introducing a new surface, and its unit and stream tests cover both the direct no-message.interim path and the dedupe false-positive cases described in the PR.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants