mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-09-01 15:32:11 +08:00
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.
This commit is contained in:
+28
-26
@@ -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
|
||||
|
||||
+19
@@ -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"))
|
||||
|
||||
+3
@@ -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))
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user