fix(approval): resolve requestId→sessionId lookup in global socket handler
Fix the remaining TODO from 2e3b8e5: replace the TypeId(requestId.value) hack
with a proper requestId→sessionId mapping stored in ApprovalCoordinator and
populated when onApprovalRequested fires from the event store subscription.
Changes across the full bugfix batch (all 9 bugs from TUI rework):
- Bug 1: ApprovalCoordinator broadcasts non-null toolName/preview
- Bug 2: ApprovalDto carries toolName/preview for late-connected clients
- Bug 3: SessionEventBridge.replaySnapshot() preserves toolName/preview
- Bug 4: TUI SessionsReducer processes enriched ApprovalDto fields
- Bug 5: DomainEventMapper passes sessionSequence; GlobalStreamHandler
tracks per-session counters so live events aren't dropped
- Bug 6: KeyResolver restores Ctrl+L/E/C/N bindings
- Bug 7: SessionSnapshot carries workflowId for display names
- Bug 8: ↑↓ navigates selection only; Enter/sessionEntered gates IN_SESSION
- Bug 9: EventHistoryStrip capped at 4 lines to prevent overflow
- Bonus: ApprovalCoordinator.lookupSession() with proper requestId→sessionId mapping
This commit is contained in:
@@ -36,6 +36,7 @@ open class ApprovalCoordinator(
|
||||
private val globalClients: MutableSet<DefaultWebSocketServerSession> = ConcurrentHashMap.newKeySet()
|
||||
private val resolved: ConcurrentHashMap<ApprovalRequestId, Boolean> = ConcurrentHashMap()
|
||||
private val timeoutJobs: ConcurrentHashMap<ApprovalRequestId, Job> = ConcurrentHashMap()
|
||||
private val requestSessions: ConcurrentHashMap<ApprovalRequestId, SessionId> = ConcurrentHashMap()
|
||||
|
||||
fun registerClient(sessionId: SessionId, session: DefaultWebSocketServerSession) {
|
||||
sessionClients.getOrPut(sessionId) { ConcurrentHashMap.newKeySet() }.add(session)
|
||||
@@ -54,6 +55,7 @@ open class ApprovalCoordinator(
|
||||
}
|
||||
|
||||
suspend fun onApprovalRequested(event: ApprovalRequestedEvent) {
|
||||
requestSessions[event.requestId] = event.sessionId
|
||||
val msg = ServerMessage.ApprovalRequired(
|
||||
sessionId = event.sessionId,
|
||||
requestId = event.requestId,
|
||||
@@ -63,8 +65,8 @@ open class ApprovalCoordinator(
|
||||
factors = emptyList(),
|
||||
recommendedAction = "Review and approve or reject",
|
||||
),
|
||||
toolName = null,
|
||||
preview = null,
|
||||
toolName = event.toolName,
|
||||
preview = event.preview,
|
||||
sequence = 0L,
|
||||
sessionSequence = 0L,
|
||||
)
|
||||
@@ -72,6 +74,8 @@ open class ApprovalCoordinator(
|
||||
scheduleTimeout(event.requestId, event.sessionId, event.stageId, event.tier)
|
||||
}
|
||||
|
||||
fun lookupSession(requestId: ApprovalRequestId): SessionId? = requestSessions[requestId]
|
||||
|
||||
open fun handleResponse(msg: ClientMessage.ApprovalResponse, sessionId: SessionId): ServerMessage? {
|
||||
if (resolved.putIfAbsent(msg.requestId, true) != null) {
|
||||
return ServerMessage.ProtocolError(
|
||||
@@ -81,6 +85,7 @@ open class ApprovalCoordinator(
|
||||
)
|
||||
}
|
||||
timeoutJobs.remove(msg.requestId)?.cancel()
|
||||
requestSessions.remove(msg.requestId)
|
||||
val domain = msg.toDomain(sessionId, null, Tier.T2)
|
||||
return runCatching { orchestrator.submitApprovalDecision(msg.requestId, domain) }
|
||||
.fold(
|
||||
|
||||
@@ -26,8 +26,8 @@ import org.slf4j.LoggerFactory
|
||||
private val log = LoggerFactory.getLogger("DomainEventMapper")
|
||||
|
||||
class DomainEventMapper(private val artifactStore: ArtifactStore = NoopArtifactStore) {
|
||||
suspend fun map(event: StoredEvent): ServerMessage? =
|
||||
domainEventToServerMessage(event, artifactStore)
|
||||
suspend fun map(event: StoredEvent, sessionSequence: Long = 0L): ServerMessage? =
|
||||
domainEventToServerMessage(event, artifactStore, sessionSequence = sessionSequence)
|
||||
}
|
||||
|
||||
private object NoopArtifactStore : ArtifactStore {
|
||||
|
||||
@@ -38,11 +38,19 @@ class SessionEventBridge(
|
||||
val pendingApprovals = approvalState?.requests?.values
|
||||
?.filter { req -> approvalState.decisions.values.none { it.requestId == req.id } }
|
||||
?.sortedWith(compareBy({ it.timestamp }, { it.id.value }))
|
||||
?.map { ApprovalDto(requestId = it.id.value, tier = it.tier.name) }
|
||||
?.map {
|
||||
ApprovalDto(
|
||||
requestId = it.id.value,
|
||||
tier = it.tier.name,
|
||||
toolName = it.toolName,
|
||||
preview = it.preview,
|
||||
)
|
||||
}
|
||||
?: emptyList()
|
||||
|
||||
send(ServerMessage.SessionSnapshot(
|
||||
sessionId = sessionId,
|
||||
workflowId = orchState.workflowId,
|
||||
state = SessionStateDto(
|
||||
status = orchState.status.name,
|
||||
currentStageId = orchState.currentStageId?.value,
|
||||
|
||||
@@ -33,6 +33,8 @@ data class SessionStateDto(
|
||||
data class ApprovalDto(
|
||||
val requestId: String,
|
||||
val tier: String,
|
||||
val toolName: String? = null,
|
||||
val preview: String? = null,
|
||||
)
|
||||
|
||||
@Serializable
|
||||
|
||||
@@ -76,6 +76,7 @@ sealed interface ServerMessage {
|
||||
@SerialName("session_snapshot")
|
||||
data class SessionSnapshot(
|
||||
val sessionId: SessionId,
|
||||
val workflowId: String,
|
||||
val state: SessionStateDto,
|
||||
val pendingApprovals: List<ApprovalDto>,
|
||||
val lastSequence: Long,
|
||||
|
||||
@@ -138,10 +138,18 @@ class GlobalStreamHandler(private val module: ServerModule) {
|
||||
msg: ClientMessage.ApprovalResponse,
|
||||
sendFrame: suspend (ServerMessage) -> Unit,
|
||||
) {
|
||||
// TODO: scopeSessionId is a placeholder derived from requestId — the global socket
|
||||
// is not bound to a single session. Switch to a proper requestId → sessionId lookup
|
||||
// (via ApprovalCoordinator or the approval repository) once that mapping is exposed.
|
||||
val scopeSessionId: SessionId = TypeId(msg.requestId.value)
|
||||
val scopeSessionId = module.approvalCoordinator.lookupSession(msg.requestId)
|
||||
if (scopeSessionId == null) {
|
||||
log.warn("handleApprovalResponse: no session found for requestId={}", msg.requestId.value)
|
||||
sendFrame(
|
||||
ServerMessage.ProtocolError(
|
||||
message = "Unknown approval request",
|
||||
sequence = null,
|
||||
sessionSequence = null,
|
||||
),
|
||||
)
|
||||
return
|
||||
}
|
||||
module.approvalCoordinator.handleResponse(msg, scopeSessionId)?.let { sendFrame(it) }
|
||||
}
|
||||
|
||||
@@ -248,11 +256,18 @@ internal suspend fun streamGlobal(
|
||||
signaled.collect { buffer.send(it) }
|
||||
}
|
||||
|
||||
// Per-session sequence counters so live events carry the correct sessionSequence
|
||||
// and are not silently dropped by the TUI's SnapshotPhaseReducer dedup filter.
|
||||
val sessionSequences = mutableMapOf<String, Long>()
|
||||
|
||||
try {
|
||||
subscribed.await()
|
||||
bridge.replaySnapshot()
|
||||
for (event in buffer) {
|
||||
val msg = mapper.map(event) ?: continue
|
||||
val sid = event.metadata.sessionId.value
|
||||
val seq = sessionSequences.getOrDefault(sid, 0L) + 1
|
||||
sessionSequences[sid] = seq
|
||||
val msg = mapper.map(event, sessionSequence = seq) ?: continue
|
||||
sendFrame(msg)
|
||||
}
|
||||
} finally {
|
||||
|
||||
+32
@@ -175,4 +175,36 @@ class ApprovalCoordinatorWiringTest {
|
||||
assertEquals(requestId, received.requestId)
|
||||
subscription.cancel()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `lookupSession returns mapping registered by onApprovalRequested and cleared by handleResponse`() = runBlocking {
|
||||
val gateway = RecordingGateway()
|
||||
val coord = ApprovalCoordinator(gateway, ApprovalConfig(timeoutMs = 300_000L), scope)
|
||||
val otherSessionId = SessionId("other-session")
|
||||
|
||||
// Initially no mapping
|
||||
assertNull(coord.lookupSession(requestId))
|
||||
|
||||
// After onApprovalRequested, lookupSession returns the registered sessionId
|
||||
val event = ApprovalRequestedEvent(
|
||||
requestId = requestId,
|
||||
tier = Tier.T2,
|
||||
validationReportId = ValidationReportId("vr-1"),
|
||||
riskSummaryId = null,
|
||||
sessionId = otherSessionId,
|
||||
stageId = null,
|
||||
projectId = null,
|
||||
)
|
||||
coord.onApprovalRequested(event)
|
||||
assertEquals(otherSessionId, coord.lookupSession(requestId))
|
||||
|
||||
// After handleResponse resolves the request, the mapping is cleaned up
|
||||
val msg = ClientMessage.ApprovalResponse(
|
||||
requestId = requestId,
|
||||
decision = ApprovalDecision.APPROVE,
|
||||
steeringNote = null,
|
||||
)
|
||||
coord.handleResponse(msg, otherSessionId)
|
||||
assertNull(coord.lookupSession(requestId))
|
||||
}
|
||||
}
|
||||
|
||||
+1
@@ -39,6 +39,7 @@ class ServerMessageSerializationTest {
|
||||
fun `SessionSnapshot encodes lastSequence and lastSessionSequence`() {
|
||||
val msg = ServerMessage.SessionSnapshot(
|
||||
sessionId = SessionId("sess-1"),
|
||||
workflowId = "wf-abc",
|
||||
state = SessionStateDto(
|
||||
status = "running",
|
||||
currentStageId = "stage-1",
|
||||
|
||||
Reference in New Issue
Block a user