Skip to content

chat: recovery compares message counts and reply text, so a recovered reply can be dropped or duplicated #710

Description

@Aristocles

What

Two comparisons in health-chat.js decide whether a turn recovered from the
server is new. Both are proxies for identity, and both are wrong in a reachable
case.

1. _refreshConversation compares counts.

const localCount = this._messages.filter(m => m.role !== 'error').length;
if (convo.messages.length <= localCount) return false;

The local transcript can hold messages the server never stored. The
voice-unconfigured notice is one: it is role: 'assistant', and
_flushConversation deliberately filters it out when persisting (a reload would
otherwise replay it as a real reply). So a client carrying that notice counts one
higher than the server, and a server that is genuinely one message ahead compares
as not ahead. _refreshConversation returns false, _reattachIfRunning returns
'none', and the caller takes 'none' as proof there is nothing to collect and
writes a "failed to connect" bubble over a reply that is sitting on the server.

2. _reattachIfRunning dedupes by content.

const last = this._messages.filter(m => m.role === 'assistant').at(-1);
if (!last || last.content !== (this._turnReply.reply || '')) {
  await this._applyTurnOutcome(false);
}

Drops a real reply when the model legitimately repeats itself: two "Done." replies
in a row, or the same answer to a re-asked question, and the second is discarded
as an echo of the first.

Duplicates a reply in voice mode. _applyTurnOutcome stores displayText
(data.reply || data.display || data.speak) as content, but the comparison
reads this._turnReply.reply. For a reply whose text arrived under display or
speak, stored content never equals reply, so every reattach appends the same
reply again.

Why

Both are the same mistake as the bug #696 fixed, one level down: inferring "did
we already see this" from something that is not an identity. The server ids every
message and every turn event; the client already tracks _lastEventId for exactly
this reason after #696. Comparing ids removes both failure modes at once, which
is why these are one issue and not two.

Acceptance criteria

  • "Is the server ahead of us" is decided by message identity, not by a count
  • A transcript carrying the voice-unconfigured notice does not suppress a
    refresh, and no other local-only message can either
  • A reattached reply is folded in exactly once, decided by its identity, not
    by comparing its text to the last assistant bubble
  • Two identical consecutive replies both appear
  • A voice-shaped reply (text under display/speak, no reply) is not
    duplicated by a reattach
  • e2e coverage for the notice-inflated refresh and for the voice-shaped
    reattach, each proven to fail before the fix

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:chatChat widget + gateway integration + voicebugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions