From 36fbfbc5c24029407d66f8b1d4fe81b048f9d751 Mon Sep 17 00:00:00 2001 From: kirillk Date: Fri, 31 Jul 2026 09:03:00 -0400 Subject: [PATCH] fix(jetbrains): keep permission queue deterministic and authoritative Follow-up review on the permission queue: - Decide the skill-shell "needs a human" case synchronously on the EDT in approve() and enqueue there, so back-to-back auto-approve asks keep arrival (FIFO) order instead of racing two independent coroutines. Only the replyPermission RPC stays in a coroutine. - Route permission enqueue/promote through a show() helper wrapped in updateModel, so cards added from approve()/abort() preserve the transcript's bottom-follow like the drain and child-recovery paths. - Queue the auto-approve error card too, so pending stays the single source of truth and Stop / TurnClose / idle purge can clear it rather than stranding a card that can only fail with NotFoundError. Add coverage for the purged auto-approve error card. --- .../session/controller/SessionController.kt | 54 ++++++++++--------- .../session/controller/PermissionQueueTest.kt | 19 +++++++ .../client/testing/FakeSessionRpcApi.kt | 3 ++ 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/controller/SessionController.kt b/packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/controller/SessionController.kt index f9efd9eb1f..cd09ba532f 100644 --- a/packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/controller/SessionController.kt +++ b/packages/kilo-jetbrains/frontend/src/main/kotlin/ai/kilocode/client/session/controller/SessionController.kt @@ -364,7 +364,7 @@ class SessionController( return } val id = sid ?: return - (childIds + id).forEach(::purgePending) + updateModel { (childIds + id).forEach(::purgePending) } capture("Session Stop Clicked", sessionProps(id)) cs.launch { try { @@ -723,36 +723,29 @@ class SessionController( private fun approve(id: String, restore: () -> Permission) { assertEdt() LOG.debug { "${ChatLogSummary.sid(sid ?: ref?.key ?: "pending")} kind=permission-auto rid=$id" } + // Skill-shell batches must be answered by a human: the server refuses non-interactive + // approvals, so show the card (its manual reply sets interactive=true) rather than send a + // machine reply. Decide and enqueue synchronously on the EDT so back-to-back asks keep + // arrival (FIFO) order, matching asked()'s non-auto path; only the RPC needs a coroutine. + if (!autoApprove || restore().meta.raw["skillShell"] == "true") { + show(restore()) + return + } + updateModel { model.setState(SessionState.Busy(KiloBundle.message("session.status.considering"))) } cs.launch { try { - // Skill-shell batches must be answered by a human: the server refuses - // non-interactive approvals, so auto-approve must show the card (whose - // manual reply sets interactive=true) rather than send a machine reply. - if (!autoApprove || restore().meta.raw["skillShell"] == "true") { - edt { - if (disposed) return@edt - enqueue(restore()) - if (model.state !is SessionState.AwaitingPermission && model.state !is SessionState.AwaitingQuestion) { - promote() - } - } - return@launch - } - edt { - if (disposed) return@edt - model.setState(SessionState.Busy(KiloBundle.message("session.status.considering"))) - } sessions.replyPermission(id, directory, PermissionReplyDto("once")) capture("Permission Auto Approved", sessionProps() + mapOf("tool" to restore().name, "source" to "single")) LOG.debug { "${ChatLogSummary.sid(sid ?: ref?.key ?: "pending")} kind=permission-auto rid=$id ok=true" } } catch (e: Exception) { LOG.warn("${ChatLogSummary.sid(sid ?: ref?.key ?: "pending")} kind=permission-auto rid=$id dir=${ChatLogSummary.dir(directory)} failed message=${e.message}", e) edt { - if (disposed) return@edt - model.setState(SessionState.AwaitingPermission(restore().copy( + // Queue the error card too, so pending stays the single source of truth and a + // later Stop / TurnClose / idle purge can clear it instead of stranding it. + show(restore().copy( state = PermissionRequestState.ERROR, message = e.message ?: KiloBundle.message("session.permission.error"), - ))) + )) } } } @@ -1535,11 +1528,7 @@ class SessionController( approve(event.request) return } - val perm = toPermission(event.request) - enqueue(perm) - if (model.state !is SessionState.AwaitingPermission && model.state !is SessionState.AwaitingQuestion) { - promote() - } + show(toPermission(event.request)) } private fun replied(event: ChatEventDto.PermissionReplied) { @@ -1589,6 +1578,19 @@ class SessionController( model.setState(SessionState.AwaitingPermission(perm)) } + /** + * Queue [perm] and surface it if no card/question is already up. Wrapped in updateModel so the + * transcript's bottom-follow is preserved (permission cards live inside the scroll pane), and + * kept synchronous so callers on the EDT enqueue in arrival (FIFO) order. + */ + @RequiresEdt + private fun show(perm: Permission) = updateModel { + enqueue(perm) + if (model.state !is SessionState.AwaitingPermission && model.state !is SessionState.AwaitingQuestion) { + promote() + } + } + /** * Drop queued permissions for [session] and clear/re-promote the visible card when it belonged to * one of them. The CLI deletes an outstanding permission server-side on turn interruption without diff --git a/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/session/controller/PermissionQueueTest.kt b/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/session/controller/PermissionQueueTest.kt index 2487f4ed3c..2e10f152e8 100644 --- a/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/session/controller/PermissionQueueTest.kt +++ b/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/session/controller/PermissionQueueTest.kt @@ -1,6 +1,7 @@ package ai.kilocode.client.session.controller import ai.kilocode.client.plugin.KiloPluginSettings +import ai.kilocode.client.session.model.PermissionRequestState import ai.kilocode.client.session.model.SessionState import ai.kilocode.rpc.dto.ChatEventDto import ai.kilocode.rpc.dto.ConfigDto @@ -141,6 +142,24 @@ class PermissionQueueTest : SessionControllerTestBase() { assertPermission(m, "perm2") } + fun `test auto approve failure card is queued and purged by stop`() { + edt { KiloPluginSettings.setAutoApprove(true) } + rpc.replyPermissionThrows = RuntimeException("boom") + val (m, _, _) = prompted() + + emit(ChatEventDto.PermissionAsked("ses_test", permission("perm1"))) + flush() + + // The failed auto-approval surfaces as an error card; it must be in the queue so purge sees it. + val state = m.model.state as? SessionState.AwaitingPermission ?: error("Expected error card") + assertEquals("perm1", state.permission.id) + assertEquals(PermissionRequestState.ERROR, state.permission.state) + + edt { m.abort() } + flush() + assertTrue(m.model.state is SessionState.Idle) + } + fun `test auto approve drain queues multiple skill shell permissions`() { rpc.pendingPermissionList.add(skillPermission("perm1")) rpc.pendingPermissionList.add(skillPermission("perm2")) diff --git a/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/testing/FakeSessionRpcApi.kt b/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/testing/FakeSessionRpcApi.kt index 18ccd3d093..ce540a25d4 100644 --- a/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/testing/FakeSessionRpcApi.kt +++ b/packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/testing/FakeSessionRpcApi.kt @@ -283,8 +283,11 @@ class FakeSessionRpcApi : KiloSessionRpcApi { configs.add(directory to config) } + var replyPermissionThrows: Exception? = null + override suspend fun replyPermission(requestId: String, directory: String, reply: PermissionReplyDto) { assertNotEdt("replyPermission") + replyPermissionThrows?.let { throw it } permissionReplies.add(Triple(requestId, directory, reply)) }