From adde03a4deba141de071b4bd1c71baed3328bf46 Mon Sep 17 00:00:00 2001 From: M57 <41645758+Hy4ri@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:34:58 +0300 Subject: [PATCH] fix(chat): stop duplicating interim commentary before tool calls (issue #842) (#854) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(chat): stop duplicating interim commentary before tool calls (issue #842) Interim commentary the agent narrates before each tool call rendered TWICE. Two mechanisms, both now covered: 1. The old tool.start orphan seal + message.complete's full text: when the complete payload repeats the commentary, the final bubble duplicated it. onMessageComplete now strips the sealed-orphan prefix (tracked in StreamingState) from the final text. 2. On-device capture + DB forensics (2026-08-08) exposed the real repeat offender: the app seals the RAW streamed text — which carries leading blank lines the model emits before its narration — while the backend persists a CLEANED copy (leading whitespace stripped). A transcript reload ~10s later merged the REST page, exact-content matching failed, and the cleaned REST copy rendered on top of the live bubble. sameLogicalMessage now compares trimmed content, so the sealed live bubble is covered by the REST row. Also: - session.interrupt seals the in-flight streaming message before clearing streaming state, so interrupting a commentary turn keeps the partial text - regression tests cover both turn shapes (commentary in the final row → stripped; absent → kept) and the whitespace-drift reload case * fix(chat): match tool rows by gateway call id, drop reasoning-echo dupes (#842) REST transcript rows carry tool_call_id; live WS bubbles keep tool_id from tool.start. Match tool rows on that 1:1 identity so MCP/web rows (stored as raw text with no JSON key) dedupe during the post-turn transcript merge instead of ghosting after the final message. reasoning.available echo (narration text == message body, sent ~40ms before each tool.start) no longer overwrites the real reasoning trace, so narration bubbles don't render their own text twice (body + Reasoning card). Room v5 migration adds tool_call_id; regression tests use captured on-device payload shapes. * fix(chat): flush throttled tokens before orphan seal + prefix-safe dedupe (#842) The tool.start seal raced the 33ms throttled token flush: deltas landing just before the transition stayed in the buffer, so the sealed narration orphan came out truncated (on-device capture: orphan ended '...chickpea', backend persisted '...chickpea goodness:') and failed the logical-match against the clean REST row — the commentary ghosted on the next transcript sync. - ChatStreamingController.flushPendingTokens(): force-flushes the throttled TOKEN buffer on MessageStart/MessageComplete/MessageDone/ToolStart so the reducer seals the COMPLETE narration. - sameLogicalMessage: accept prefix-covering (both sides >=40 chars) so a residual seal drift can never re-add a substantial narration. - Regression tests: controller flush + truncated-orphan prefix match. --------- Co-authored-by: M57 --- .../5.json | 95 +++++++++ .../data/local/ChatMessageEntity.kt | 2 + .../data/local/ChatMessageMapper.kt | 2 + .../data/local/HermesDatabase.kt | 18 +- .../data/model/SessionMessagesResponse.kt | 4 + .../m57/hermescontrol/ui/chat/ChatMessage.kt | 9 + .../ui/chat/ChatStreamingController.kt | 38 +++- .../hermescontrol/ui/chat/ChatViewModel.kt | 94 ++++++++- .../ui/chat/ChatWsEventReducer.kt | 114 ++++++++-- .../hermescontrol/ui/chat/StreamingState.kt | 8 + .../ui/chat/ChatStreamingControllerTest.kt | 38 ++++ .../ui/chat/ChatToolDedupeTest.kt | 175 +++++++++++++++ .../ui/chat/ChatViewModelTest.kt | 22 +- .../ui/chat/ChatWsEventReducerTest.kt | 199 +++++++++++++++++- 14 files changed, 780 insertions(+), 38 deletions(-) create mode 100644 app/schemas/com.m57.hermescontrol.data.local.HermesDatabase/5.json diff --git a/app/schemas/com.m57.hermescontrol.data.local.HermesDatabase/5.json b/app/schemas/com.m57.hermescontrol.data.local.HermesDatabase/5.json new file mode 100644 index 00000000..4901f510 --- /dev/null +++ b/app/schemas/com.m57.hermescontrol.data.local.HermesDatabase/5.json @@ -0,0 +1,95 @@ +{ + "formatVersion": 1, + "database": { + "version": 5, + "identityHash": "906ca45ef187dc6c219f94803925d20a", + "entities": [ + { + "tableName": "chat_messages", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `session_id` TEXT NOT NULL, `role` TEXT NOT NULL, `content` TEXT NOT NULL, `reasoning_text` TEXT NOT NULL, `timestamp` INTEGER NOT NULL, `tool_name` TEXT, `tool_call_id` TEXT NOT NULL, `tool_status` TEXT, `is_streaming` INTEGER NOT NULL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sessionId", + "columnName": "session_id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "role", + "columnName": "role", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "content", + "columnName": "content", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "reasoningText", + "columnName": "reasoning_text", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "timestamp", + "columnName": "timestamp", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "toolName", + "columnName": "tool_name", + "affinity": "TEXT" + }, + { + "fieldPath": "toolCallId", + "columnName": "tool_call_id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "toolStatus", + "columnName": "tool_status", + "affinity": "TEXT" + }, + { + "fieldPath": "isStreaming", + "columnName": "is_streaming", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_chat_messages_session_id_timestamp", + "unique": false, + "columnNames": [ + "session_id", + "timestamp" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_chat_messages_session_id_timestamp` ON `${TABLE_NAME}` (`session_id`, `timestamp`)" + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '906ca45ef187dc6c219f94803925d20a')" + ] + } +} \ No newline at end of file diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt index 98cd564d..ad43b2f9 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageEntity.kt @@ -34,6 +34,8 @@ data class ChatMessageEntity( val timestamp: Long, @ColumnInfo(name = "tool_name") val toolName: String? = null, + @ColumnInfo(name = "tool_call_id") + val toolCallId: String = "", @ColumnInfo(name = "tool_status") val toolStatus: String? = null, @ColumnInfo(name = "is_streaming") diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageMapper.kt b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageMapper.kt index db8f3796..dc603ef0 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageMapper.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/ChatMessageMapper.kt @@ -26,6 +26,7 @@ fun ChatMessageEntity.toUiModel(): ChatMessage = timestamp = timestamp, isStreaming = isStreaming, toolName = toolName, + toolCallId = toolCallId, toolStatus = when (toolStatus) { "RUNNING" -> ToolStatus.RUNNING @@ -44,6 +45,7 @@ fun ChatMessage.toEntity(sessionId: String): ChatMessageEntity = reasoningText = reasoningText, timestamp = timestamp, toolName = toolName, + toolCallId = toolCallId, toolStatus = toolStatus?.name, isStreaming = isStreaming, ) diff --git a/app/src/main/java/com/m57/hermescontrol/data/local/HermesDatabase.kt b/app/src/main/java/com/m57/hermescontrol/data/local/HermesDatabase.kt index fd774413..9b6c3982 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/local/HermesDatabase.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/local/HermesDatabase.kt @@ -11,7 +11,7 @@ import java.io.File @Database( entities = [ChatMessageEntity::class], - version = 4, + version = 5, exportSchema = true, ) abstract class HermesDatabase : RoomDatabase() { @@ -54,13 +54,27 @@ abstract class HermesDatabase : RoomDatabase() { } } + val migration4to5 = + object : Migration(4, 5) { + override fun migrate(db: SupportSQLiteDatabase) { + // Issue #842: tool rows now carry the gateway's + // tool call id (`call_00_...`) so REST transcript + // rows can be matched 1:1 against their live WS + // bubbles instead of fragile result-content + // canonicalization. + db.execSQL( + "ALTER TABLE `chat_messages` ADD COLUMN `tool_call_id` TEXT NOT NULL DEFAULT ''", + ) + } + } + instance ?: Room .databaseBuilder( context.applicationContext, HermesDatabase::class.java, "hermes_control.db", ).openHelperFactory(factory) - .addMigrations(migration2to3, migration3to4) + .addMigrations(migration2to3, migration3to4, migration4to5) .fallbackToDestructiveMigration(false) .build() .also { instance = it } diff --git a/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt b/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt index 5feae4a1..4ea4cf71 100644 --- a/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt +++ b/app/src/main/java/com/m57/hermescontrol/data/model/SessionMessagesResponse.kt @@ -19,6 +19,7 @@ data class SessionMessage( val type: String? = null, val reasoning: JsonElement? = null, val reasoning_text: JsonElement? = null, + val tool_call_id: String? = null, ) { val timestampText: String? get() = (timestamp as? JsonPrimitive)?.content @@ -38,4 +39,7 @@ data class SessionMessage( null -> "" else -> r.toString() } + + val toolCallId: String + get() = tool_call_id.orEmpty() } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt index b416f463..c1b6b81f 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatMessage.kt @@ -34,6 +34,15 @@ data class ChatMessage( val isStreaming: Boolean = false, val toolName: String? = null, val toolStatus: ToolStatus? = null, + /** + * Gateway tool call id (`call_00_...`, from `tool.start` payload `tool_id` + * / REST `tool_call_id`). The REST transcript and the WS stream carry the + * SAME id for one tool call, so it is the robust 1:1 identity for + * deduplicating tool rows (issue #842) — result-content canonicalization + * fails for MCP/web tools because the REST side stores the payload as raw + * `` text, not JSON. + */ + val toolCallId: String = "", val approvalInfo: ApprovalInfo? = null, /** Transient clarify request data — when present, renders [ClarifyBubble] inline. */ val clarifyInfo: ClarifyUi? = null, diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatStreamingController.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatStreamingController.kt index f1770373..0d06ccc5 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatStreamingController.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatStreamingController.kt @@ -52,7 +52,14 @@ class ChatStreamingController( // Issue #755: the shared streaming state must not carry the previous // message's reasoning into the next one. The finalized message keeps // its own copy (persisted to Room), so clearing here is safe. - streamingState.update { it.copy(isReasoning = false, reasoningText = "") } + streamingState.update { + it.copy( + isReasoning = false, + reasoningText = "", + // Issue #842: sealed-orphan tracking belongs to one turn only. + sealedOrphanIds = emptyList(), + ) + } } /** @@ -90,6 +97,35 @@ class ChatStreamingController( flushReasoning() } + /** + * Force-flushes any throttled TOKEN buffer onto the streaming message + * before a state-transition event (ToolStart / MessageComplete / + * MessageDone). `handleMessageToken` only flushes when >=33ms have passed + * since the last flush, so a delta that lands just before the transition + * can still be sitting in the buffer. The reducer seals the streaming + * message into a finalized orphan at tool.start (issue #842) — sealing a + * stale copy truncated the narration's tail (observed on-device: the + * orphan ended "...chickpea" while the REST row ended "...chickpea + * goodness:"), which then failed the logical-match against the clean + * REST row and re-added the commentary as a ghost bubble. Flushing the + * tokens synchronously first makes the sealed orphan complete. + */ + fun flushPendingTokens() { + val currentContent = streamingBuffer.toString() + if (currentContent.isEmpty()) return + streamingState.update { state -> + val current = state.streamingMessage ?: return@update state + state.copy( + streamingMessage = + current.copy( + content = currentContent, + reasoningText = + if (current.reasoningText.isNotBlank()) current.reasoningText else state.reasoningText, + ), + ) + } + } + fun handleMessageToken(event: WsEvent.MessageToken) { if (!isCurrentSession(event.sessionId)) return diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt index c753300c..66a681a6 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatViewModel.kt @@ -107,7 +107,15 @@ internal fun canonicalToolResultKey(content: String): String? { * True when [a] and [b] are the same logical message (the WS-persisted and * REST-persisted copies of one row — they carry different ids, see #771). * Tool messages match on their normalized result payload; other roles on - * exact content. + * exact content, IGNORING leading/trailing whitespace. + * + * Issue #842: the app seals the RAW streamed text (which can carry leading + * blank lines the model emits before its narration), while the backend + * persists a CLEANED copy (leading whitespace stripped). An exact-content + * compare made the reload merge treat them as different messages and add + * the REST copy on top — the commentary duplicated ~10s after the stream + * ended. Trim closes the drift: the sealed live bubble is covered by the + * REST row and the duplicate never renders. */ internal fun sameLogicalMessage( a: ChatMessage, @@ -115,11 +123,31 @@ internal fun sameLogicalMessage( ): Boolean { if (a.role != b.role) return false if (a.role == MessageRole.TOOL) { + // Issue #842: prefer the gateway's tool call id when both sides carry + // it — the REST transcript stores `tool_call_id` and the live WS + // bubble keeps it from `tool.start`. Content canonicalization cannot + // cover MCP/web tools: the REST side stores the payload as raw + // `` text (not JSON), so it has no key at all. + if (a.toolCallId.isNotBlank() && b.toolCallId.isNotBlank()) { + return a.toolCallId == b.toolCallId + } val ka = canonicalToolResultKey(a.content) val kb = canonicalToolResultKey(b.content) return ka != null && ka == kb } - return a.content == b.content + val ta = a.content.trim() + val tb = b.content.trim() + if (ta == tb) return true + // Issue #842: a seal race can leave the live orphan a few trailing tokens + // short of the streamed narration (the last delta was still in the + // throttled buffer when tool.start sealed the message). The backend + // persists the COMPLETE copy, so the orphan is a strict prefix of the + // REST row. Accept prefix-covering only for substantial texts (>=40 + // chars) so a short reply can never be swallowed by a longer message + // that merely starts with it. + return ta.length >= 40 && + tb.length >= 40 && + (tb.startsWith(ta) || ta.startsWith(tb)) } /** @@ -620,7 +648,15 @@ class ChatViewModel( is WsEvent.MessageComplete, is WsEvent.MessageDone, is WsEvent.ToolStart, - -> streamingController.flushPendingReasoning() + -> { + streamingController.flushPendingReasoning() + // Issue #842: the token buffer can hold deltas that landed + // <33ms before the transition. The reducer seals the + // streaming message into the orphan at tool.start — flush + // first so the seal carries the COMPLETE narration (a + // truncated seal fails the later REST dedupe and ghosts). + streamingController.flushPendingTokens() + } else -> Unit } @@ -987,6 +1023,12 @@ class ChatViewModel( } WsMethods.SESSION_INTERRUPT -> { + // Issue #842 follow-up: seal whatever the agent streamed so far + // (interim commentary + partial answer) BEFORE clearing the + // streaming state. The old tool.start orphan seal used to leave + // pre-tool text behind on interrupt; with that seal gone, the + // partial would otherwise vanish entirely. + sealStreamingMessageIfAny() _uiState.update { it.copy( isAgentTyping = false, @@ -1907,6 +1949,27 @@ class ChatViewModel( // ── Session resume recovery (desktop parity) ───────────────────────── + /** + * Persists the in-flight streaming message as-is (isStreaming=false) so an + * interrupted turn keeps the text the user already saw on screen. No-op + * when there is no streaming content/reasoning to save. (Issue #842 + * follow-up: replaces the old tool.start orphan seal — the streaming + * message now survives tool calls, so interrupts are the only path that + * would otherwise drop the partial text.) + */ + private fun sealStreamingMessageIfAny() { + val streaming = _streamingState.value.streamingMessage ?: return + if (streaming.content.isBlank() && streaming.reasoningText.isBlank()) return + val finalized = streaming.copy(isStreaming = false) + _uiState.update { it.copy(messages = (it.messages + finalized).dedupeById()) } + val sid = _uiState.value.currentSessionId + if (sid != null) { + viewModelScope.launch(ioDispatcher) { + repo.persistMessage(finalized, sid) + } + } + } + private fun resetSessionState( sessionId: String?, title: String, @@ -2281,7 +2344,11 @@ class ChatViewModel( } } val merged = mergedList.distinctBy { it.id } - if (sameMessages(current.messages, merged)) current else current.copy(messages = merged) + if (sameMessages(current.messages, merged)) { + current + } else { + current.copy(messages = merged) + } } } @@ -2510,6 +2577,18 @@ class ChatViewModel( } } + // Issue #842: REST transcript rows carry the gateway's `tool_call_id` + // — prefer matching live bubbles by that 1:1 identity. It works for + // EVERY tool shape, including MCP/web rows whose REST copy is raw + // `` text with no JSON key to canonicalize. + val liveToolByCallId = linkedMapOf() + _uiState.value.messages + .filter { it.role == MessageRole.TOOL && it.toolCallId.isNotBlank() } + .sortedBy { it.id.startsWith("rest-") } // prefer live WS copies + .forEach { msg -> + liveToolByCallId.putIfAbsent(msg.toolCallId, msg) + } + val mapped = mutableListOf() // The gateway stores a reasoning-model's thinking as its OWN assistant // row (content = "", reasoning = trace) directly before the answer row. @@ -2593,6 +2672,12 @@ class ChatViewModel( // keeps the real name, the rich WS payload, AND the same id so // Room upserts instead of accumulating a duplicate `rest-` row. if (role == MessageRole.TOOL) { + // Prefer the gateway call id (1:1, works for every tool + // shape), then fall back to result-content matching. + liveToolByCallId[msg.toolCallId]?.let { live -> + mapped.add(live) + return@forEachIndexed + } canonicalToolResultKey(rawContent)?.let { key -> liveToolByResult[key]?.let { live -> mapped.add(live) @@ -2607,6 +2692,7 @@ class ChatViewModel( role = role, content = finalContent, reasoningText = finalReasoning, + toolCallId = msg.toolCallId, attachments = attachments, timestamp = timestamp, isStreaming = false, diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducer.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducer.kt index 600310fa..2564e361 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducer.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducer.kt @@ -12,6 +12,33 @@ private fun List.upsertById(message: ChatMessage): List, + sealedOrphanIds: List, +): String { + if (sealedOrphanIds.isEmpty()) return text + val prefix = + sealedOrphanIds + .mapNotNull { id -> messages.firstOrNull { it.id == id }?.content } + .joinToString("") + if (prefix.isEmpty() || !text.startsWith(prefix)) return text + return text.removePrefix(prefix) +} + /** * Pure state reducer for WebSocket events. * @@ -263,15 +290,31 @@ object ChatWsEventReducer { // message.complete carries it into the finalized bubble. val text = event.text return if (!text.isNullOrBlank() && streamingState.streamingMessage != null) { - ReducerResult( - state = state, - streamingState = - streamingState.copy( - isReasoning = true, - reasoningText = text, - streamingMessage = streamingState.streamingMessage.copy(reasoningText = text), - ), - ) + // Issue #842: the gateway ALSO re-sends the agent's interim + // NARRATION as reasoning.available ~40ms before each tool.start + // (the commentary already rendered as the message body, echoed + // verbatim). Attaching the echo as reasoning would make every + // narration bubble render its own text twice (body + Reasoning + // card). Real reasoning never equals the message content, so the + // exact-content echo is skipped and the reasoning.delta trace + // (already in streamingState) stays authoritative. + val content = streamingState.streamingMessage.content + if (text.trim() == content.trim()) { + ReducerResult( + state = state, + streamingState = streamingState.copy(isReasoning = true), + ) + } else { + ReducerResult( + state = state, + streamingState = + streamingState.copy( + isReasoning = true, + reasoningText = text, + streamingMessage = streamingState.streamingMessage.copy(reasoningText = text), + ), + ) + } } else { ReducerResult( state = state, @@ -297,14 +340,31 @@ object ChatWsEventReducer { event.reasoning ?.takeIf { it.isNotBlank() } ?: streamingState.reasoningText.ifBlank { streaming?.reasoningText ?: "" } + // Issue #842: the complete payload carries the FULL final text, which + // often repeats commentary already sealed into its own bubble(s) at + // each tool.start. Strip the sealed prefix so the final bubble shows + // only the part not already rendered — every narration line appears + // exactly once, matching the persisted transcript (sealed orphans + + // stripped answer). Turns whose final text does NOT contain the + // commentary are left untouched (orphans stay, final keeps its text). + val text = stripSealedOrphanPrefix(event.text, state.messages, streamingState.sealedOrphanIds) + // Edge: the complete payload was entirely covered by sealed orphans + // (the interim text already rendered as its own bubble(s)) — nothing + // new to show, so don't add an empty final bubble. + if (text.isBlank()) { + return ReducerResult( + state = state.copy(isAgentTyping = false), + streamingState = streamingState.copy(streamingMessage = null), + ) + } val msg = streaming?.copy( - content = event.text, + content = text, isStreaming = false, reasoningText = reasoning, ) ?: ChatMessage( role = MessageRole.ASSISTANT, - content = event.text, + content = text, reasoningText = reasoning, ) val effects = mutableListOf() @@ -389,9 +449,17 @@ object ChatWsEventReducer { content = contentJson, toolName = event.name, toolStatus = ToolStatus.RUNNING, + // Issue #842: keep the gateway's call id so REST transcript + // rows (which carry the same `tool_call_id`) can be matched + // 1:1 against this live bubble during merges. + toolCallId = event.data?.get("tool_id") as? String ?: "", ) - // Finalize any orphan streaming message + // Finalize any orphan streaming message (issue #771 + #842): interim + // text streamed BEFORE the tool is sealed as its own bubble (desktop + // parity). The sealed id is tracked in StreamingState so + // onMessageComplete can strip the repeated prefix — the complete + // payload often re-carries the same commentary text. var orphanToPersist: ChatMessage? = null val newState = if (streamingState.streamingMessage?.content?.isNotEmpty() == true) { @@ -422,19 +490,19 @@ object ChatWsEventReducer { effects.add(ReducerEffect.PersistMessage(toolMessage, sid)) } - // Issue #771: KEEP the streaming state across a tool call instead of - // resetting it. A reasoning-model turn streams its thinking BEFORE the - // tool starts (content is still empty here) — wiping the streaming - // message at tool.start made the reasoning bubble vanish mid-turn and - // left the finalized answer with no reasoning card. The stream stays - // alive; post-tool message.delta continues the SAME message and - // message.complete finalizes it with the reasoning intact. - // When an orphan WAS sealed (interim text before the tool), the - // streaming message is done — null it, but keep the reasoning text in - // the shared state so the post-tool fallback message still picks it up. + // Issue #771: KEEP the streaming state across a tool call when the + // stream has nothing to seal yet (a reasoning-model turn streams its + // thinking BEFORE the tool starts) — wiping it made the reasoning + // bubble vanish mid-turn. When an orphan WAS sealed (interim text + // before the tool), the streaming message is done — null it, keep the + // reasoning text in shared state, and remember the sealed orphan so + // message.complete can strip the repeated prefix. val finalStreamingState = if (orphanToPersist != null) { - streamingState.copy(streamingMessage = null) + streamingState.copy( + streamingMessage = null, + sealedOrphanIds = streamingState.sealedOrphanIds + orphanToPersist.id, + ) } else { streamingState } diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/StreamingState.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/StreamingState.kt index dcb0722e..db5e9a48 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/StreamingState.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/StreamingState.kt @@ -6,4 +6,12 @@ data class StreamingState( val thinkingText: String = "", val isReasoning: Boolean = false, val reasoningText: String = "", + /** + * Ids of assistant messages sealed at tool.start during the CURRENT turn + * (interim commentary before a tool call). Consumed by + * [com.m57.hermescontrol.ui.chat.ChatWsEventReducer.onMessageComplete] to + * strip the repeated prefix from the final text (issue #842). Reset on + * every fresh StreamingState (message.start / interrupt / session switch). + */ + val sealedOrphanIds: List = emptyList(), ) diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatStreamingControllerTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatStreamingControllerTest.kt index a9d0ee2e..3bc70e35 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatStreamingControllerTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatStreamingControllerTest.kt @@ -43,6 +43,44 @@ class ChatStreamingControllerTest { assertEquals("AB", streamingState.value.streamingMessage?.reasoningText) } + @Test + fun flushPendingTokens_flushesThrottledTokensBeforeSeal() = + runTest { + // Issue #842: a delta landing <33ms before tool.start stays in the + // throttled buffer; flushing the tokens before the reducer seals the + // orphan must push the COMPLETE narration onto the message. + val uiState = MutableStateFlow(ChatUiState()) + val streamingState = + MutableStateFlow( + StreamingState( + streamingMessage = + ChatMessage( + role = MessageRole.ASSISTANT, + content = "", + isStreaming = true, + ), + ), + ) + val controller = controller(this, uiState, streamingState, isTestEnvironment = { false }) + + controller.handleMessageToken(WsEvent.MessageToken("tool's loaded! 🔍 now searchin'", "session")) + // First delta flushes immediately. + assertEquals("tool's loaded! 🔍 now searchin'", streamingState.value.streamingMessage?.content) + + // Second delta arrives <33ms later -> throttled, still buffered. + controller.handleMessageToken(WsEvent.MessageToken(" for the best hummus", "session")) + assertEquals("tool's loaded! 🔍 now searchin'", streamingState.value.streamingMessage?.content) + + // The tool.start transition forces the buffered tokens out so the + // sealed orphan carries the complete narration. + controller.flushPendingTokens() + + assertEquals( + "tool's loaded! 🔍 now searchin' for the best hummus", + streamingState.value.streamingMessage?.content, + ) + } + @Test fun resetStreaming_clearsReasoningBufferSoStaleReasoningDoesNotResurrect() = runTest { diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatToolDedupeTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatToolDedupeTest.kt index 035c9038..5a4c1413 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatToolDedupeTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatToolDedupeTest.kt @@ -82,6 +82,50 @@ class ChatToolDedupeTest { assertTrue(sameLogicalMessage(ws, rest)) } + @Test + fun sameLogicalMessage_whitespaceDrift_stillSame() { + // Issue #842: the sealed live bubble holds the RAW streamed text + // (leading blank lines), the backend row is CLEANED — same logical + // message, must not duplicate on reload. + val live = ChatMessage(role = MessageRole.ASSISTANT, content = "\n\noooh fresh angle this time 🤤") + val rest = ChatMessage(role = MessageRole.ASSISTANT, content = "oooh fresh angle this time 🤤") + assertTrue(sameLogicalMessage(live, rest)) + assertFalse(sameLogicalMessage(live, ChatMessage(role = MessageRole.ASSISTANT, content = "different reply"))) + } + + @Test + fun sameLogicalMessage_sealRaceTruncatedNarration_prefixCovered() { + // Issue #842 (on-device capture): the seal raced the throttled flush and + // the orphan missed the last deltas — it ends "...dreamy chickpea" while + // the backend persisted the COMPLETE narration "...dreamy chickpea + // goodness:". The orphan must count as covered (strict prefix, both + // sides substantial) so the reload doesn't ghost the commentary. + val orphan = + ChatMessage( + role = MessageRole.ASSISTANT, + content = + "\n\ntool's loaded! 🔍 now searchin' for the **best hummus recipe**" + + " — gimme dat creamy dreamy chickpea", + ) + val rest = + ChatMessage( + role = MessageRole.ASSISTANT, + content = + "tool's loaded! 🔍 now searchin' for the **best hummus recipe**" + + " — gimme dat creamy dreamy chickpea goodness:", + ) + assertTrue(sameLogicalMessage(orphan, rest)) + + // A SHORT truncated prefix must NOT swallow a longer message that merely + // starts with it ("ok" is not the same message as "okay bestie..."). + assertFalse( + sameLogicalMessage( + ChatMessage(role = MessageRole.ASSISTANT, content = "ok"), + ChatMessage(role = MessageRole.ASSISTANT, content = "okay bestie, 3 searches comin' up!! 🔍"), + ), + ) + } + @Test fun sameLogicalMessage_differentRoles_notSame() { val ws = ChatMessage(role = MessageRole.TOOL, content = wsToolContent) @@ -155,6 +199,34 @@ class ChatToolDedupeTest { assertEquals("terminal", merged[1].toolName) } + @Test + fun merge_sealedCommentaryWithLeadingWhitespace_noDuplicate() { + // Issue #842 (on-device capture 2026-08-08): the app seals the RAW + // streamed commentary — which can carry leading blank lines the model + // emits before its narration — while the backend persists a CLEANED + // copy. A reload merge with exact-content matching treated them as + // different messages and added the REST copy on top: the commentary + // duplicated ~10s after the stream ended. Trim-based matching covers + // the sealed live bubble, so no second copy renders. + val liveOrphan = + ChatMessage( + role = MessageRole.ASSISTANT, + content = "\n\noooh fresh angle this time — the **Lebanese method** and even hummus fatteh 🤤", + timestamp = 3, + ) + val restRow = + ChatMessage( + role = MessageRole.ASSISTANT, + content = "oooh fresh angle this time — the **Lebanese method** and even hummus fatteh 🤤", + id = "rest-s-19", + timestamp = 3, + ) + + val merged = mergeTranscriptWithLive(listOf(restRow), listOf(liveOrphan)) + + assertEquals("cleaned REST row covers the whitespace-drifting live bubble", 1, merged.size) + } + @Test fun merge_matchingUser_keepsLiveOnlyMetadata() { val live = ChatMessage(role = MessageRole.USER, content = "redirect", isStreaming = true, timestamp = 1) @@ -436,4 +508,107 @@ class ChatToolDedupeTest { assertEquals(listOf("retry", "retry"), merged.map { it.content }) assertSame(live, merged.last()) } + + // ── Issue #842: MCP/web tool rows (raw `` text) ── + // + // REAL captured shapes (on-device 2026-08-08): the live WS bubble holds + // the full tool.complete payload (JSON), while the REST transcript row + // stores the SAME call's payload as RAW blob TEXT — not JSON, so content + // canonicalization can never produce a key. The gateway `tool_call_id` + // (present on BOTH sides) is the 1:1 identity that closes the gap. + + private val wsMcpToolPayload = + """{"tool_id":"call_00_1GgRcqEFKA2TR0EkdkN85844","name":"mcp__ddgs__search_text","args":{"query":"Amman weather today"},"duration_s":3.2,"result":{"result":"\nThe following content was retrieved from an external source.\n{\"title\":\"Amman weather\"}\n"}}""" + + private val restMcpToolBlob = + """ +The following content was retrieved from an external source. +{"title":"Amman weather"} +""" + + @Test + fun sameLogicalMessage_mcpBlobRestRow_matchesByCallId() { + val live = + ChatMessage( + role = MessageRole.TOOL, + content = wsMcpToolPayload, + toolName = "mcp__ddgs__search_text", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + ) + val rest = + ChatMessage( + role = MessageRole.TOOL, + content = restMcpToolBlob, + id = "rest-s-3", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + ) + assertTrue(sameLogicalMessage(live, rest)) + } + + @Test + fun sameLogicalMessage_differentCallIds_notSameEvenWithSameBlob() { + val live = ChatMessage(role = MessageRole.TOOL, content = wsMcpToolPayload, toolCallId = "call_00_aaa") + val rest = ChatMessage(role = MessageRole.TOOL, content = restMcpToolBlob, toolCallId = "call_00_bbb") + assertFalse(sameLogicalMessage(live, rest)) + } + + @Test + fun sameLogicalMessage_callIdMissing_fallsBackToContentCanonical() { + // Old cached rows may lack the call id — terminal-style content + // canonicalization must still match. + val ws = ChatMessage(role = MessageRole.TOOL, content = wsToolContent, toolName = "terminal") + val rest = ChatMessage(role = MessageRole.TOOL, content = restToolContent) + assertTrue(sameLogicalMessage(ws, rest)) + } + + @Test + fun mergeTranscriptWithLive_mcpToolRows_collapseToOne() { + // Issue #842 on-device repro: the REST page carries the same MCP + // search call as raw blob text; with the call id present the merge + // must NOT add a second tool bubble. + val live = + ChatMessage( + role = MessageRole.TOOL, + content = wsMcpToolPayload, + toolName = "mcp__ddgs__search_text", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + timestamp = 2, + ) + val rest = + ChatMessage( + role = MessageRole.TOOL, + content = restMcpToolBlob, + id = "rest-s-3", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + timestamp = 2, + ) + val merged = mergeTranscriptWithLive(listOf(rest), listOf(live)) + // One copy survives (the merge keeps the REST row as server truth and + // drops the covered live bubble — in the real flow mapServerMessages + // already substituted the live row before this merge, preserving the + // tool name + rich payload). The invariant: never two bubbles. + assertEquals(1, merged.size) + assertEquals(MessageRole.TOOL, merged.single().role) + } + + @Test + fun dedupeCached_mcpBlobRestRow_droppedWhenWsCopyExists() { + val wsTool = + ChatMessage( + role = MessageRole.TOOL, + content = wsMcpToolPayload, + toolName = "mcp__ddgs__search_text", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + ) + val restTool = + ChatMessage( + role = MessageRole.TOOL, + content = restMcpToolBlob, + id = "rest-s-3", + toolCallId = "call_00_1GgRcqEFKA2TR0EkdkN85844", + ) + val deduped = dedupeCachedMessages(listOf(wsTool, restTool)) + assertEquals(1, deduped.size) + assertEquals(wsTool, deduped.single()) + } } diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt index 721a0127..f8c954ab 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatViewModelTest.kt @@ -839,7 +839,7 @@ class ChatViewModelTest { } @Test - fun testToolExecution_finalizesPreviousStreamingMessage() = + fun testToolExecution_sealsInterimTextAndStripsCompletePrefix() = runTest { val (viewModel, sessionId) = createViewModelWithSession() @@ -850,6 +850,9 @@ class ChatViewModelTest { mockEventsFlow.emit(WsEvent.ToolStart("calculator", mapOf("input" to "2+2"))) advanceUntilIdle() + // Issue #842: the interim text is sealed as its own bubble at + // tool.start (desktop parity), the stream tail clears, and the + // sealed id is tracked for the complete-prefix strip. // messages[0] = "Session created" system message assertEquals( "Calculating sum", @@ -861,12 +864,29 @@ class ChatViewModelTest { viewModel.uiState.value.messages[1] .role, ) + assertFalse(viewModel.uiState.value.messages[1].isStreaming) assertEquals( MessageRole.TOOL, viewModel.uiState.value.messages[2] .role, ) assertNull(viewModel.streamingState.value.streamingMessage) + assertEquals( + listOf(viewModel.uiState.value.messages[1].id), + viewModel.streamingState.value.sealedOrphanIds, + ) + + // Finalize: the complete payload repeats the sealed commentary as + // a prefix — the final bubble strips it, so each line appears once. + mockEventsFlow.emit(WsEvent.MessageComplete("Calculating sum = 4", sessionId)) + advanceUntilIdle() + val assistant = + viewModel.uiState.value.messages.filter { it.role == MessageRole.ASSISTANT } + assertEquals(2, assistant.size) + assertEquals("Calculating sum", assistant[0].content) + assertEquals(" = 4", assistant[1].content) + assertFalse(assistant[1].isStreaming) + assertNull(viewModel.streamingState.value.streamingMessage) } @Test diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducerTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducerTest.kt index 03322aba..eb984ba5 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducerTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatWsEventReducerTest.kt @@ -610,6 +610,44 @@ class ChatWsEventReducerTest { assertEquals(1, result.state.messages.count { it.role == MessageRole.ASSISTANT }) } + @Test + fun testReasoningAvailable_narrationEcho_notAttachedAsReasoning() { + // Issue #842 on-device capture (2026-08-08): the gateway re-sends the + // agent's interim NARRATION as reasoning.available ~40ms before each + // tool.start — payload text == the streaming message body. Attaching + // the echo as reasoningText would render every narration bubble's + // text twice (body + Reasoning card). Real reasoning never equals the + // message content, so the echo is skipped and the reasoning.delta + // trace stays authoritative. + val state = ChatUiState(currentSessionId = "session-1") + val realReasoning = "The user wants 3 searches" + val stream = + StreamingState( + streamingMessage = + ChatMessage( + role = MessageRole.ASSISTANT, + content = "\n\n✅ search #2 done!! Amman's cookin 🔥", + isStreaming = true, + reasoningText = realReasoning, + ), + reasoningText = realReasoning, + ) + + // Echo arrives with the CLEANED narration text (backend strips the + // leading blank lines the raw stream carried). + val result = + ChatWsEventReducer.reduce( + state, + stream, + WsEvent.ReasoningAvailable("session-1", "✅ search #2 done!! Amman's cookin 🔥"), + "session-1", + ) + + // The echo must NOT overwrite the real reasoning trace. + assertEquals(realReasoning, result.streamingState.reasoningText) + assertEquals(realReasoning, result.streamingState.streamingMessage?.reasoningText) + } + @Test fun testMessageComplete_reasoningPayloadField_isAuthoritativeFallback() { // Even if reasoning.delta events were entirely lost (throttled/wipe), @@ -630,11 +668,10 @@ class ChatWsEventReducerTest { } @Test - fun testToolStart_withInterimText_sealsOrphan_butClearsStreamTail() { - // When the agent streams interim text BEFORE calling the tool, the - // orphan is sealed into messages (desktop parity), and the streaming - // tail is cleared — but the reasoning text stays in shared state so - // the post-tool answer can still pick it up. + fun testToolStart_withInterimText_sealsOrphanAndTracksIt() { + // Issue #842: interim commentary BEFORE a tool call is sealed as its + // own bubble (desktop parity) and its id tracked in streamingState so + // message.complete can strip the repeated prefix from the final text. val state = ChatUiState(currentSessionId = "session-1") val streaming = StreamingState( @@ -657,12 +694,160 @@ class ChatWsEventReducerTest { "session-1", ) - assertEquals(2, result.state.messages.size) // sealed orphan + tool + // Sealed orphan + tool row. + assertEquals(2, result.state.messages.size) assertEquals("Let me run that for you.", result.state.messages[0].content) assertFalse(result.state.messages[0].isStreaming) assertEquals("thinking...", result.state.messages[0].reasoningText) + assertEquals(MessageRole.TOOL, result.state.messages[1].role) + // Stream tail cleared, orphan id tracked for the complete-strip. assertEquals(null, result.streamingState.streamingMessage) - // Reasoning preserved for the post-tool message + assertEquals(listOf(result.state.messages[0].id), result.streamingState.sealedOrphanIds) + // Reasoning preserved for the post-tool message. assertEquals("thinking...", result.streamingState.reasoningText) } + + @Test + fun testMultiToolCommentaryTurn_noDuplicateInterimText() { + // Regression for issue #842 (m57 repro, 2026-08-08): a turn where the + // agent narrates commentary before EACH tool call, and the final + // message.complete text repeats that commentary as a prefix. Each + // commentary line used to render TWICE — once as the sealed orphan + // bubble, once inside the final bubble. Now the final bubble strips + // the sealed prefix: every line appears exactly once. + val state = ChatUiState(currentSessionId = "session-1") + val start = + ChatWsEventReducer.reduce(state, StreamingState(), WsEvent.MessageStart("session-1"), "session-1") + + val line1 = "say less — full scrub it is 🧹 lemme pull context around each hit so the patches land clean:" + val line2 = "context's clear — patching all 4 spots now:" + val line3 = "all 4 patches landed ✅ — final verification sweep to make sure zero sandalwood survives anywhere:" + val answer = "Done — zero sandalwood left. 🫡" + // The final row is the concatenation of the streamed text — the same + // characters the sealed orphans hold (no extra separators). + val fullText = line1 + line2 + line3 + answer + + // Token accumulation is ViewModel-side; the reducer sees flushed content. + // After a tool.start seals the orphan, post-tool deltas recreate the + // streaming message via the controller fallback — mirror that here. + fun withContent( + content: String, + streamingState: StreamingState, + ) = streamingState.copy( + streamingMessage = + streamingState.streamingMessage?.copy(content = content) + ?: ChatMessage( + role = MessageRole.ASSISTANT, + content = content, + isStreaming = true, + ), + ) + + // 1. commentary line 1, then tool call 1 → orphan sealed + tracked + var result = + ChatWsEventReducer.reduce( + start.state, + withContent(line1, start.streamingState), + WsEvent.ToolStart("grep", mapOf("args_text" to "sandalwood"), "session-1"), + "session-1", + ) + assertEquals(1, result.state.messages.count { it.role == MessageRole.ASSISTANT }) + assertEquals(1, result.state.messages.count { it.role == MessageRole.TOOL }) + + // 2. commentary line 2, then tool call 2 + result = + ChatWsEventReducer.reduce( + result.state, + withContent(line2, result.streamingState), + WsEvent.ToolStart("patch", mapOf("args_text" to "4 spots"), "session-1"), + "session-1", + ) + + // 3. commentary line 3, then tool call 3 + result = + ChatWsEventReducer.reduce( + result.state, + withContent(line3, result.streamingState), + WsEvent.ToolStart("grep", mapOf("args_text" to "verify"), "session-1"), + "session-1", + ) + val orphans = result.state.messages.filter { it.role == MessageRole.ASSISTANT } + assertEquals(listOf(line1, line2, line3), orphans.map { it.content }) + assertEquals(3, result.state.messages.count { it.role == MessageRole.TOOL }) + assertEquals(orphans.map { it.id }, result.streamingState.sealedOrphanIds) + + // 4. message.complete repeats the commentary as a prefix → the final + // bubble strips it and shows only the answer. + result = + ChatWsEventReducer.reduce( + result.state, + result.streamingState, + WsEvent.MessageComplete(text = fullText, sessionId = "session-1"), + "session-1", + ) + val allAssistant = result.state.messages.filter { it.role == MessageRole.ASSISTANT } + assertEquals( + "each commentary line exactly once (sealed bubble) + the stripped answer", + listOf(line1, line2, line3, answer), + allAssistant.map { it.content }, + ) + assertFalse(allAssistant.last().isStreaming) + assertEquals(null, result.streamingState.streamingMessage) + } + + @Test + fun testMultiToolCommentaryTurn_completeWithoutCommentary_keepsOrphansAndFinal() { + // Issue #842 variant caught on-device (2026-08-08): some turns emit + // commentary ONLY as streamed deltas + message.interim — the final + // message.complete text does NOT contain it. The sealed orphan + // bubbles must survive and the final bubble must keep its full text + // (no stripping when there is no prefix overlap). + val state = ChatUiState(currentSessionId = "session-1") + val start = + ChatWsEventReducer.reduce(state, StreamingState(), WsEvent.MessageStart("session-1"), "session-1") + + val line1 = "okie let's go!! 🌸 first up — **hummus**:" + val line2 = "yum yum, got a whole buffet 🥙 **search #2**:" + val summary = "done!! all three searches landed 🎯 here's your summary." + + fun withContent( + content: String, + streamingState: StreamingState, + ) = streamingState.copy( + streamingMessage = + streamingState.streamingMessage?.copy(content = content) + ?: ChatMessage( + role = MessageRole.ASSISTANT, + content = content, + isStreaming = true, + ), + ) + + var result = + ChatWsEventReducer.reduce( + start.state, + withContent(line1, start.streamingState), + WsEvent.ToolStart("mcp__ddgs__search_text", mapOf("query" to "hummus"), "session-1"), + "session-1", + ) + result = + ChatWsEventReducer.reduce( + result.state, + withContent(line2, result.streamingState), + WsEvent.ToolStart("mcp__ddgs__search_text", mapOf("query" to "amman"), "session-1"), + "session-1", + ) + + // Final text does NOT contain the commentary → no stripping. + result = + ChatWsEventReducer.reduce( + result.state, + result.streamingState, + WsEvent.MessageComplete(text = summary, sessionId = "session-1"), + "session-1", + ) + val allAssistant = result.state.messages.filter { it.role == MessageRole.ASSISTANT } + assertEquals(listOf(line1, line2, summary), allAssistant.map { it.content }) + assertEquals(null, result.streamingState.streamingMessage) + } }