Skip to content

fix(desktop): stop resurrecting completed stream bubbles - #70209

Closed
Dolverin wants to merge 1 commit into
NousResearch:mainfrom
Dolverin:fix/desktop-completed-replies
Closed

fix(desktop): stop resurrecting completed stream bubbles#70209
Dolverin wants to merge 1 commit into
NousResearch:mainfrom
Dolverin:fix/desktop-completed-replies

Conversation

@Dolverin

Copy link
Copy Markdown
Contributor

Summary

  • treat the explicit pending flag as the authority when reconciling local assistant projections
  • stop a completed assistant-stream-* bubble from being re-appended after authoritative history has caught up
  • add a focused regression test for the completed-stream case

Why

preserveLocalPendingTurnMessages currently considers any assistant message whose ID starts with assistant-stream- to be pending, even after terminal stream paths have explicitly set pending: false.

If such a completed local bubble has no same-ordinal assistant in the refreshed authoritative transcript, the reconciliation step appends it again. This can resurrect an already completed assistant bubble alongside the persisted response.

All live stream constructors set pending: true, while completion, interim sealing, interruption, and error paths set pending: false; the explicit state is therefore the correct authority. This is a separate reconciliation invariant from the upstream stream/final merge work in #70173.

Related: #70108

Verification

  • focused regression fails on current origin/main and passes with this patch
  • npm exec vitest run -- --project ui src/app/session/hooks/use-session-actions/utils.test.ts — 33 passed
  • npm exec vitest run -- --project ui src/app/session/hooks/use-session-actions.test.tsx — 33 passed
  • npm run typecheck — passed
  • npm exec eslint -- src/app/session/hooks/use-session-actions/utils.ts src/app/session/hooks/use-session-actions/utils.test.ts — passed
  • npm exec prettier -- --check src/app/session/hooks/use-session-actions/utils.ts src/app/session/hooks/use-session-actions/utils.test.ts — passed

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #70173 deduplicates an interim stream bubble and its final completion; this patch separately prevents an already-completed local stream projection from being re-appended during history reconciliation.

@Dolverin
Dolverin force-pushed the fix/desktop-completed-replies branch from b2917fc to af3cf98 Compare July 24, 2026 15:06
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still has the reported predicate: apps/desktop/src/app/session/hooks/use-session-actions/utils.ts:403-404 classifies every assistant-stream-* row as pending even when its explicit pending state is false. Terminal stream paths explicitly settle rows with pending: false at apps/desktop/src/app/session/hooks/use-message-stream/index.ts:434, :504, and :634-645; when no same-ordinal authoritative assistant is present, the reconciliation appends that local row at utils.ts:422-437.

The proposed change makes the explicit flag authoritative and adds the missing completed-stream case. The PR is currently conflicting with main, but its two-file change is localized and should be mechanically salvageable without broad rework.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/streaming Streaming responses: gateway delivery, provider wire labels Jul 30, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #77644, with credit — the duplicate settled bubble is real and it's fixed there.

Your diagnosis of the mechanism holds; I took a narrower fix than the predicate change. Keying isPendingAssistant on the explicit pending flag alone also discards a sibling case in the same area: a reply that finished locally before the gateway committed it, where the settled assistant-stream-* row is the only copy that exists. I verified that directly — that row survives on main today and is dropped under this patch. #77644 instead drops a settled stream row only when authoritative history already carries that exact text, so both cases hold at once.

The conflict with main is moot now; no rebase needed on your side.

@OutThisLife OutThisLife closed this Aug 3, 2026
xrbs00 added a commit to xrbs00/hermes-agent that referenced this pull request Aug 6, 2026
…ative transcript

Third residue class behind NousResearch#70209: when the renderer kept both the committed user row and its optimistic duplicate, the duplicate's role-ordinal shifts past the committed user onto a LATER user (e.g. a background process notice) — newest-user check and ordinal pairing both miss it, and it gets re-appended after the transcript. Gate: a live optimistic user row whose exact text already exists anywhere in the authoritative transcript is dropped. 84/84 focused tests (RED→GREEN verified).
teknium1 pushed a commit that referenced this pull request Aug 15, 2026
…eady carries

A still-pending assistant stream row (id `assistant-stream-*`) whose reply
the authoritative transcript already committed used to fall through to
`preserved.push` when ordinal pairing missed it — the commit shifted the
row's ordinal under compaction/history rewrites, so `nextByRoleOrdinal`
returned nothing and the local copy was appended to the tail, rendering the
same answer twice (reported as A B C D E C D tail duplication).

The #70209 guard only covers SETTLED local rows (`pending !== true`);
pending rows were unprotected. Match pending rows against SETTLED
authoritative rows before appending:

- identical answer text            -> authoritative already carries it
- authoritative extends local text -> authoritative is the settled final
  version of the still-streaming local copy
- local extends authoritative text -> replace the committed row with the
  richer local body instead of appending

Live projection shells (still-pending candidates) never match, so the
traces-only local row keeps replacing the empty shell.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history area/streaming Streaming responses: gateway delivery, provider wire comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants