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) + } }