From 9aefc5bfc78d8c36296fd816adf175b028ee7681 Mon Sep 17 00:00:00 2001 From: m57 <41645758+Hy4ri@users.noreply.github.com> Date: Mon, 28 Sep 2026 19:57:35 +0300 Subject: [PATCH] refactor(chat): pure server message mapper (#1363) --- .../ui/chat/ChatReadNotificationTest.kt | 10 ++- .../ui/chat/ChatServerMessageMapper.kt | 33 ++++--- .../hermescontrol/ui/chat/ChatViewModel.kt | 21 +++-- .../chat/ChatServerMessageMapperPurityTest.kt | 62 ++++++++++++++ .../ReadNotificationReviewRegressionTest.kt | 85 +++++++++++++++++-- 5 files changed, 179 insertions(+), 32 deletions(-) create mode 100644 app/src/test/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapperPurityTest.kt diff --git a/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/ChatReadNotificationTest.kt b/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/ChatReadNotificationTest.kt index 946125fc..29fe3dac 100644 --- a/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/ChatReadNotificationTest.kt +++ b/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/ChatReadNotificationTest.kt @@ -251,7 +251,15 @@ class ChatReadNotificationTest { SessionMessage(id = 10, role = "assistant", content = JsonPrimitive("Done")), SessionMessage(id = 52, role = "assistant", content = JsonPrimitive("Done")), ) - val messages = mapServerMessages(sessionId, history, 0, true, emptyList(), context = context) + val messages = + mapServerMessages( + sessionId, + history, + 0, + true, + emptyList(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals("Older duplicate must stay unmapped", null, messages[0].completionId) assertEquals("Exact row must receive the completion id", "comp-exact-row", messages[1].completionId) diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt index 1e67bfc2..56040ff8 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapper.kt @@ -1,20 +1,24 @@ package com.m57.hermescontrol.ui.chat -import com.m57.hermescontrol.data.local.AuthManager import com.m57.hermescontrol.data.model.Attachment import com.m57.hermescontrol.data.model.AttachmentSource import com.m57.hermescontrol.data.model.SessionMessage -import com.m57.hermescontrol.data.remote.GatewayFileClient -import com.m57.hermescontrol.notification.ReplyNotificationTracker +import com.m57.hermescontrol.notification.ReplyNotificationTarget import com.m57.hermescontrol.ui.chat.tool.ToolJson import kotlinx.serialization.json.JsonObject /** * Maps REST transcript rows ([SessionMessage]) into UI [ChatMessage]s. * - * Reads the supplied transcript snapshot and recovers notification identity through - * ReplyNotificationTracker's synchronized lookup. Safe on the history dispatcher; - * it never mutates ViewModel state or dismisses a notification. + * Pure: every input is plain data. Callers resolve the active reply-notification + * target and the gateway media URL builder, so this never touches Android, + * AuthManager, or the notification tracker (#1337). + * + * @param activeReplyTarget the reply notification currently posted, if any; its + * durable server row id reserves that row's completion identity. + * @param mediaUrl builds an authenticated gateway URL for a `MEDIA:` path, or null + * when no gateway is configured. + * @param nowMs fallback timestamp for rows without one. */ internal fun mapServerMessages( sessionId: String, @@ -24,7 +28,9 @@ internal fun mapServerMessages( liveMessages: List, isPagingOlder: Boolean = false, stableRowIds: Boolean = latestPaging, - context: android.content.Context? = null, + activeReplyTarget: ReplyNotificationTarget? = null, + mediaUrl: (path: String) -> String? = { null }, + nowMs: Long = System.currentTimeMillis(), ): List { val existingById = liveMessages.associateBy { it.canonicalRestId ?: it.id } val liveByExactId = @@ -55,7 +61,7 @@ internal fun mapServerMessages( if (!isPagingOlder) { // #129: reserve durable identity before matching any repeated live prose. // REST-only recovery remains fail-closed when the target row is absent. - val activeTarget = ReplyNotificationTracker.getActiveTarget(context)?.takeIf { it.sessionId == sessionId } + val activeTarget = activeReplyTarget?.takeIf { it.sessionId == sessionId } val durableTarget = activeTarget?.takeIf { it.serverMessageId != null } if (durableTarget != null) { val exactIndex = @@ -179,7 +185,7 @@ internal fun mapServerMessages( ?.times(1000) ?.toLong() ?: existingById[restId]?.timestamp - ?: System.currentTimeMillis() + ?: nowMs val rawContent = if (role == MessageRole.TOOL && msg.display_metadata != null) { @@ -229,18 +235,11 @@ internal fun mapServerMessages( if (role == MessageRole.ASSISTANT && rawContent.contains("MEDIA:")) { val items = HostMediaExtractor.extract(rawContent) if (items.isNotEmpty()) { - val baseUrl = AuthManager.getBaseUrl() - val token = AuthManager.getToken().orEmpty() finalContent = HostMediaExtractor.strip(rawContent) attachments = items .mapNotNull { item -> - val url = - GatewayFileClient.buildMediaUrl( - baseUrl, - token, - item.path, - ) ?: return@mapNotNull null + val url = mediaUrl(item.path) ?: return@mapNotNull null Attachment( uri = url, name = mediaNameFromPath(item.path), 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 14763576..59284f8b 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 @@ -21,6 +21,7 @@ import com.m57.hermescontrol.data.model.parseContextBreakdown import com.m57.hermescontrol.data.model.parseUsageSnapshot import com.m57.hermescontrol.data.model.reasoningSupport import com.m57.hermescontrol.data.remote.ApiClient +import com.m57.hermescontrol.data.remote.GatewayFileClient import com.m57.hermescontrol.data.remote.NetworkResult import com.m57.hermescontrol.data.remote.OkHttpProvider import com.m57.hermescontrol.data.remote.safeApiCall @@ -38,6 +39,7 @@ import com.m57.hermescontrol.data.ws.WsEvent import com.m57.hermescontrol.data.ws.WsMethods import com.m57.hermescontrol.data.ws.toAny import com.m57.hermescontrol.data.ws.toJsonElement +import com.m57.hermescontrol.notification.ReplyNotificationTracker import com.m57.hermescontrol.notification.captureTurnBoundary import com.m57.hermescontrol.notification.correlationScopeId import com.m57.hermescontrol.ui.chat.fullbleed.TranscriptUiState @@ -2249,6 +2251,10 @@ class ChatViewModel( // ── Send message ───────────────────────────────────────────────────── + /** Authenticated gateway URL for a host `MEDIA:` path; kept out of the pure mapper (#1337). */ + private fun gatewayMediaUrl(path: String): String? = + GatewayFileClient.buildMediaUrl(AuthManager.getBaseUrl(), AuthManager.getToken().orEmpty(), path) + private fun sendScope(): String = listOf( AuthManager.getBaseUrl(), @@ -4242,7 +4248,8 @@ class ChatViewModel( offset = 0, latestPaging = latestPaging, liveMessages = emptyList(), - context = getApplication(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(getApplication()), + mediaUrl = ::gatewayMediaUrl, ) } _uiState.update { it.copy(messages = replacementMessages) } @@ -4736,7 +4743,8 @@ class ChatViewModel( liveMessages = _uiState.value.messages, isPagingOlder = true, stableRowIds = true, - context = getApplication(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(getApplication()), + mediaUrl = ::gatewayMediaUrl, ) } if (!valid()) return@launch @@ -5046,7 +5054,8 @@ class ChatViewModel( serverOffset, useLatest, current, - context = getApplication(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(getApplication()), + mediaUrl = ::gatewayMediaUrl, ) } ?: return@launch persistHistoryPage(page, sessionId) @@ -5503,7 +5512,8 @@ class ChatViewModel( useLatest, current, isPagingOlder = true, - context = getApplication(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(getApplication()), + mediaUrl = ::gatewayMediaUrl, ) } ?: return@launch persistHistoryPage(page, sessionId) @@ -5584,7 +5594,8 @@ class ChatViewModel( current, // Sync fetches recent replies, so confirm live completion identities. isPagingOlder = false, - context = getApplication(), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(getApplication()), + mediaUrl = ::gatewayMediaUrl, ) } ?: return@launch persistHistoryPage(page, sessionId) diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapperPurityTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapperPurityTest.kt new file mode 100644 index 00000000..81feb1bf --- /dev/null +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatServerMessageMapperPurityTest.kt @@ -0,0 +1,62 @@ +package com.m57.hermescontrol.ui.chat + +import com.m57.hermescontrol.data.model.SessionMessage +import com.m57.hermescontrol.notification.ReplyNotificationTarget +import kotlinx.serialization.json.JsonPrimitive +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** Issue #1337: the REST mapper runs on plain inputs, with no Android, AuthManager, or tracker state. */ +class ChatServerMessageMapperPurityTest { + private fun assistant( + id: Int, + text: String, + ) = SessionMessage(id = id, role = "assistant", content = JsonPrimitive(text)) + + @Test + fun mediaAttachmentsUseTheInjectedUrlBuilder() { + val mapped = + mapServerMessages( + "s", + listOf(assistant(1, "Chart MEDIA:/opt/hermes/chart.png")), + 0, + true, + emptyList(), + mediaUrl = { path -> "gw://$path" }, + ).single() + + assertEquals("Chart", mapped.content.trim()) + assertEquals(listOf("gw:///opt/hermes/chart.png"), mapped.attachments?.map { it.uri }) + } + + @Test + fun mediaWithoutAGatewayStripsTheTokenAndAddsNoAttachment() { + val mapped = + mapServerMessages("s", listOf(assistant(1, "Chart MEDIA:/opt/hermes/chart.png")), 0, true, emptyList()) + .single() + + assertEquals("Chart", mapped.content.trim()) + assertNull(mapped.attachments) + } + + @Test + fun activeReplyTargetIsAPlainInputScopedToItsSession() { + val rows = listOf(assistant(100, "Done"), assistant(200, "Done")) + val target = ReplyNotificationTarget("default", "s", "comp", 1L, serverMessageId = 100) + + val bound = mapServerMessages("s", rows, 0, true, emptyList(), activeReplyTarget = target) + val otherSession = + mapServerMessages("s", rows, 0, true, emptyList(), activeReplyTarget = target.copy(sessionId = "x")) + + assertEquals(listOf("comp", null), bound.map { it.completionId }) + assertEquals(listOf(null, null), otherSession.map { it.completionId }) + } + + @Test + fun rowsWithoutTimestampsUseTheInjectedClock() { + val mapped = mapServerMessages("s", listOf(assistant(1, "Hi")), 0, true, emptyList(), nowMs = 42L) + + assertEquals(42L, mapped.single().timestamp) + } +} diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ReadNotificationReviewRegressionTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ReadNotificationReviewRegressionTest.kt index b06cbaba..30c3b379 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ReadNotificationReviewRegressionTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ReadNotificationReviewRegressionTest.kt @@ -169,11 +169,29 @@ class ReadNotificationReviewRegressionTest { SessionMessage(id = 200, role = "assistant", content = JsonPrimitive("Done")), ) for (latest in listOf(true, false)) { - val mapped = mapServerMessages("session", rows, 10, latest, listOf(live), context = context) + val mapped = + mapServerMessages( + "session", + rows, + 10, + latest, + listOf(live), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals(listOf("comp", null), mapped.map { it.completionId }) val merged = mergeTranscriptWithLive(mapped, listOf(live), preserveLiveIds = true) assertEquals(if (latest) "rest-session-100" else "rest-session-10", merged.single { it.id == "ws" }.restId) - assertNull(mapServerMessages("session", rows.takeLast(1), 11, latest, listOf(live)).single().completionId) + // Target row absent from this page: the reserved completion must fail closed, not rebind by text. + val targetRowAbsent = + mapServerMessages( + "session", + rows.takeLast(1), + 11, + latest, + listOf(live), + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) + assertNull(targetRowAbsent.single().completionId) } } @@ -214,6 +232,7 @@ class ReadNotificationReviewRegressionTest { 0, true, current, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(), ) val merged = mergeTranscriptWithLive(mapped, current, preserveLiveIds = true).single() assertEquals("ws", merged.id) @@ -256,7 +275,7 @@ class ReadNotificationReviewRegressionTest { latestPaging = true, liveMessages = current, isPagingOlder = false, - context = context, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), ) val merged = mergeTranscriptWithLive(mapped, current, preserveLiveIds = true) @@ -685,7 +704,15 @@ class ReadNotificationReviewRegressionTest { ) val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals("Older message must have null completionId", null, mapped[0].completionId) assertEquals("Newest message must receive target completionId", "comp-newest-done", mapped[1].completionId) @@ -747,7 +774,15 @@ class ReadNotificationReviewRegressionTest { ) val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals("comp-long-2", mapped[0].completionId) assertEquals(null, mapped[1].completionId) @@ -796,7 +831,15 @@ class ReadNotificationReviewRegressionTest { ) val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) val row = mapped.single() assertEquals("comp-media-cold", row.completionId) assertEquals("Here is the image", row.content) @@ -839,7 +882,15 @@ class ReadNotificationReviewRegressionTest { ) val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) val row = mapped.single() assertEquals("comp-media-only-cold", row.completionId) assertEquals("", row.content) @@ -887,7 +938,15 @@ class ReadNotificationReviewRegressionTest { ) val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals("comp-media-dup-2", mapped[0].completionId) assertEquals(null, mapped[1].completionId) @@ -955,7 +1014,15 @@ class ReadNotificationReviewRegressionTest { } val mapped = - mapServerMessages("session", history, 0, true, emptyList(), isPagingOlder = false, context = context) + mapServerMessages( + "session", + history, + 0, + true, + emptyList(), + isPagingOlder = false, + activeReplyTarget = ReplyNotificationTracker.getActiveTarget(context), + ) assertEquals(listOf(null, null, "comp-turn"), mapped.map { it.completionId }) assertFalse(