diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegate.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegate.kt index 99c3094c..0144d71b 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegate.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegate.kt @@ -81,8 +81,12 @@ class ChatApprovalsDelegate( @Suppress("UNCHECKED_CAST") val approvals = (result as? Map<*, *>)?.get("approvals") as? List<*> val first = approvals?.filterIsInstance>()?.firstOrNull() ?: return + val requestId = first["request_id"] as? String ?: return val sessionId = runtimeSessionId() ?: uiState.value.currentSessionId - maybeSurfacePendingApproval(first, sessionId) + val alreadyShown = + uiState.value.messages.any { it.approvalInfo?.requestId == requestId } + if (alreadyShown) return + handleApprovalRequest(parseApprovalMap(first, sessionId)) } fun handleApprovalRespondResult(result: Any?) { diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt index 44e2996c..b295f6ef 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatModelSwitchDelegate.kt @@ -19,7 +19,7 @@ import java.util.concurrent.ConcurrentHashMap * Tracks an in-flight `config.set key=model` RPC and the model it would replace * so confirmation dialog dismiss can roll back optimistically. */ -data class ActiveModelSwitch( +private data class ActiveModelSwitch( val spec: String, val previousModel: String?, ) @@ -47,10 +47,10 @@ class ChatModelSwitchDelegate( private val getPinnedModels: () -> List = { AuthManager.getPinnedModels() }, private val savePinnedModels: (List) -> Unit = { AuthManager.savePinnedModels(it) }, ) { - val pendingModelSwitchRequests = ConcurrentHashMap() - var activeModelSwitchConfirmation: ActiveModelSwitch? = null - var optimisticPreviousModel: String? = null - var cachedModelOptions: List = emptyList() + private val pendingModelSwitchRequests = ConcurrentHashMap() + private var activeModelSwitchConfirmation: ActiveModelSwitch? = null + private var optimisticPreviousModel: String? = null + private var cachedModelOptions: List = emptyList() fun isModelPickerCommand(command: String): Boolean { val trimmed = command.trim() diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTracker.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTracker.kt deleted file mode 100644 index 8492ab54..00000000 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTracker.kt +++ /dev/null @@ -1,111 +0,0 @@ -package com.m57.hermescontrol.ui.chat - -import java.util.concurrent.ConcurrentHashMap - -/** - * Tracks an in-flight RPC request associated with a specific session generation - * and resume sequence to guard against stale results/errors from previous sessions. - */ -data class TrackedSessionRequest( - val generation: Long, - val resumeSequence: Long = 0L, - val sessionId: String? = null, -) - -/** - * Owns RPC id-to-method mapping, session request generations, hydration sequences, - * and staleness detection. - * - * Extracted behavior-preservingly from [ChatViewModel] to ensure deterministic - * RPC tracking across session creation, switches, resumes, and reconnects. - */ -class ChatRpcRequestTracker( - private val getCurrentSessionId: () -> String?, -) { - /** Maps an in-flight RPC id to its method for UI error labeling. */ - private val idToMethod = ConcurrentHashMap() - private val sessionRequestById = ConcurrentHashMap() - - var sessionGeneration: Long = 0L - private set - - var resumeRequestSequence: Long = 0L - private set - var activeResumeRequestSequence: Long = 0L - private set - - var hydrationRequestSequence: Long = 0L - private set - var activeHydrationRequestSequence: Long = 0L - private set - - var resumedGeneration: Long = -1L - var hydratedGeneration: Long = -1L - - fun nextSessionGeneration(): Long { - sessionGeneration++ - return sessionGeneration - } - - fun nextResumeSequence(): Long { - resumeRequestSequence++ - activeResumeRequestSequence = resumeRequestSequence - return resumeRequestSequence - } - - fun nextHydrationSequence(): Long { - hydrationRequestSequence++ - activeHydrationRequestSequence = hydrationRequestSequence - return hydrationRequestSequence - } - - fun trackRequest( - id: String, - method: String, - ) { - idToMethod[id] = method - } - - fun trackSessionRequest( - id: String, - method: String, - generation: Long, - resumeSequence: Long = 0L, - sessionId: String? = null, - ) { - sessionRequestById[id] = TrackedSessionRequest(generation, resumeSequence, sessionId) - trackRequest(id, method) - } - - fun isCurrentSessionRequest( - sessionId: String, - generation: Long, - ): Boolean = generation == sessionGeneration && sessionId == getCurrentSessionId() - - fun isCurrentHydration( - sessionId: String, - generation: Long, - requestSequence: Long, - ): Boolean = requestSequence == activeHydrationRequestSequence && isCurrentSessionRequest(sessionId, generation) - - fun isStaleSessionRequest(id: String): Boolean = sessionRequestById[id]?.let(::isStaleSessionRequest) == true - - fun isStaleSessionRequest(request: TrackedSessionRequest): Boolean = - request.generation != sessionGeneration || - (request.sessionId != null && request.sessionId != getCurrentSessionId()) || - (request.resumeSequence != 0L && request.resumeSequence != activeResumeRequestSequence) - - fun removeMethod(id: String): String? = idToMethod.remove(id) - - fun removeSessionRequest(id: String): TrackedSessionRequest? = sessionRequestById.remove(id) - - fun forgetRequest(id: String) { - idToMethod.remove(id) - sessionRequestById.remove(id) - } - - fun resetSessionStateTracking() { - resumedGeneration = -1L - hydratedGeneration = -1L - } -} 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 bdf18ffa..828a53a9 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 @@ -510,11 +510,6 @@ class ChatViewModel( private val sessionRequestById = ConcurrentHashMap() private var sessionGeneration = 0L - private data class ActiveModelSwitch( - val spec: String, - val previousModel: String?, - ) - private var resumeRequestSequence = 0L private var activeResumeRequestSequence = 0L private var hydrationRequestSequence = 0L diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegateTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegateTest.kt index 4f3d298d..6623c4bf 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegateTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatApprovalsDelegateTest.kt @@ -161,6 +161,36 @@ class ChatApprovalsDelegateTest { ) } + @Test + fun handleApprovalPendingResult_dropsMissingRequestId_andDedupesAlreadyShown() { + val initialCount = uiState.value.messages.size + + // No request_id -> dropped entirely + delegate.handleApprovalPendingResult( + mapOf("approvals" to listOf(mapOf("command" to "rm -rf /"))), + ) + assertEquals(initialCount, uiState.value.messages.size) + + // Valid request_id -> surfaced + delegate.handleApprovalPendingResult( + mapOf("approvals" to listOf(mapOf("command" to "git pull", "request_id" to "r-valid"))), + ) + assertEquals(initialCount + 1, uiState.value.messages.size) + assertEquals( + "r-valid", + uiState.value.messages + .last() + .approvalInfo + ?.requestId, + ) + + // Duplicate request_id -> ignored + delegate.handleApprovalPendingResult( + mapOf("approvals" to listOf(mapOf("command" to "git pull", "request_id" to "r-valid"))), + ) + assertEquals(initialCount + 1, uiState.value.messages.size) + } + @Test fun handleApprovalRespondResult_appendsDesktopParitySystemMessage() { delegate.handleApprovalRespondResult(mapOf("resolved" to 1)) diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTrackerTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTrackerTest.kt deleted file mode 100644 index 78eac772..00000000 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/ChatRpcRequestTrackerTest.kt +++ /dev/null @@ -1,94 +0,0 @@ -package com.m57.hermescontrol.ui.chat - -import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertNull -import org.junit.Assert.assertTrue -import org.junit.Test - -class ChatRpcRequestTrackerTest { - private var currentSessionId: String? = "session-a" - private val tracker = ChatRpcRequestTracker { currentSessionId } - - @Test - fun trackRequest_andRemoveMethod() { - tracker.trackRequest("req-1", "session.create") - assertEquals("session.create", tracker.removeMethod("req-1")) - assertNull(tracker.removeMethod("req-1")) - } - - @Test - fun generationIncrement_detectsStaleGenerations() { - val gen1 = tracker.nextSessionGeneration() - assertEquals(1L, gen1) - - tracker.trackSessionRequest( - id = "req-resume-1", - method = "session.resume", - generation = gen1, - sessionId = "session-a", - ) - assertFalse(tracker.isStaleSessionRequest("req-resume-1")) - - val gen2 = tracker.nextSessionGeneration() - assertEquals(2L, gen2) - assertTrue(tracker.isStaleSessionRequest("req-resume-1")) - } - - @Test - fun sessionMismatch_marksRequestStale() { - val gen = tracker.nextSessionGeneration() - tracker.trackSessionRequest( - id = "req-2", - method = "session.resume", - generation = gen, - sessionId = "session-a", - ) - assertFalse(tracker.isStaleSessionRequest("req-2")) - - currentSessionId = "session-b" - assertTrue(tracker.isStaleSessionRequest("req-2")) - } - - @Test - fun resumeSequence_marksSupersededResumeStale() { - val gen = tracker.nextSessionGeneration() - val seq1 = tracker.nextResumeSequence() - tracker.trackSessionRequest( - id = "req-resume-seq1", - method = "session.resume", - generation = gen, - resumeSequence = seq1, - sessionId = "session-a", - ) - assertFalse(tracker.isStaleSessionRequest("req-resume-seq1")) - - val seq2 = tracker.nextResumeSequence() - assertEquals(2L, seq2) - assertTrue(tracker.isStaleSessionRequest("req-resume-seq1")) - } - - @Test - fun hydrationSequence_guardsActiveHydration() { - val gen = tracker.nextSessionGeneration() - val hyd1 = tracker.nextHydrationSequence() - assertTrue(tracker.isCurrentHydration("session-a", gen, hyd1)) - - val hyd2 = tracker.nextHydrationSequence() - assertFalse(tracker.isCurrentHydration("session-a", gen, hyd1)) - assertTrue(tracker.isCurrentHydration("session-a", gen, hyd2)) - } - - @Test - fun forgetRequest_removesBothTrackingEntries() { - tracker.trackSessionRequest( - id = "req-forget", - method = "session.resume", - generation = tracker.sessionGeneration, - sessionId = "session-a", - ) - tracker.forgetRequest("req-forget") - assertNull(tracker.removeMethod("req-forget")) - assertNull(tracker.removeSessionRequest("req-forget")) - } -}