fix(jetbrains): address permission review feedback

This commit is contained in:
kirillk
2026-07-20 12:16:37 -04:00
parent 3ab9e489e4
commit 1a456f403f
10 changed files with 85 additions and 35 deletions
@@ -1166,7 +1166,8 @@ object KiloCliDataParser {
}?.toMap() ?: emptyMap()
val path = metaObj.path()
val diffs = metaObj.permissionDiffs(path)
val rules = metaObj.rules()
val rawRules = metaObj.ruleDecisions()
val rules = rawRules.map { it.pattern }
return PermissionRequestDto(
id = id,
sessionID = sid,
@@ -1178,7 +1179,7 @@ object KiloCliDataParser {
message = obj.str("message") ?: metaObj?.str("message"),
command = metaObj?.str("command") ?: obj.str("command"),
rules = rules,
ruleDecisions = metaObj.ruleDecisions(rules.ifEmpty { always }),
ruleDecisions = rawRules.ifEmpty { always.map { PermissionRuleDecisionDto(it) } },
filePath = path,
fileDiffs = diffs,
)
@@ -1596,30 +1597,10 @@ private fun JsonObject?.path(): String? {
return str("filepath") ?: str("filePath") ?: str("file") ?: str("path")
}
private fun JsonObject?.rules(): List<String> {
private fun JsonObject?.ruleDecisions(): List<PermissionRuleDecisionDto> {
if (this == null) return emptyList()
val raw = this["rules"] ?: return emptyList()
val arr = raw.arr()
if (arr != null) {
return arr.mapNotNull { elem ->
val obj = elem.obj()
if (obj != null) return@mapNotNull obj.str("pattern") ?: obj.str("rule") ?: obj.str("text")
runCatching { elem.jsonPrimitive.contentOrNull }.getOrNull()
}
}
val text = runCatching { raw.jsonPrimitive.contentOrNull }.getOrNull() ?: return emptyList()
if (text.startsWith("[")) {
return runCatching {
KiloCliDataParser.parseRulesJson(text)
}.getOrElse { listOf(text) }
}
return listOf(text)
}
private fun JsonObject?.ruleDecisions(fallback: List<String>): List<PermissionRuleDecisionDto> {
if (this == null) return fallback.map { PermissionRuleDecisionDto(it) }
val raw = this["rules"] ?: return fallback.map { PermissionRuleDecisionDto(it) }
val arr = raw.arr()
if (arr != null) {
return arr.mapNotNull { elem ->
val obj = elem.obj()
@@ -1632,7 +1613,7 @@ private fun JsonObject?.ruleDecisions(fallback: List<String>): List<PermissionRu
PermissionRuleDecisionDto(pattern)
}
}
val text = runCatching { raw.jsonPrimitive.contentOrNull }.getOrNull() ?: return fallback.map { PermissionRuleDecisionDto(it) }
val text = runCatching { raw.jsonPrimitive.contentOrNull }.getOrNull() ?: return emptyList()
if (text.startsWith("[")) return KiloCliDataParser.parseRulesJson(text).map { PermissionRuleDecisionDto(it) }
return listOf(PermissionRuleDecisionDto(text))
}
@@ -64,10 +64,11 @@ class KiloBackendCliManagerEnvTest {
}
@Test
fun `isolation disabled - default CLI config asks for edit permissions`() {
fun `isolation disabled - default CLI config asks for edit permissions without forcing bash`() {
val env = manager.buildEnv("pwd123", emptyMap())
assertEquals("""{"permission":{"edit":"ask"}}""", env["KILO_CONFIG_CONTENT"])
assertFalse(env["KILO_CONFIG_CONTENT"]?.contains("bash") == true)
}
@Test
@@ -2357,6 +2357,7 @@ class KiloCliDataParserTest {
assertNotNull(result)
val asked = result as? ChatEventDto.PermissionAsked ?: error("Expected PermissionAsked")
assertEquals(listOf("git *", "git add *", "git add ."), asked.request.rules)
assertEquals(asked.request.rules, asked.request.ruleDecisions.map { it.pattern })
assertEquals("git *", asked.request.ruleDecisions[0].pattern)
assertEquals("approved", asked.request.ruleDecisions[0].decision)
assertEquals("pending", asked.request.ruleDecisions[0].defaultDecision)
@@ -2390,6 +2391,28 @@ class KiloCliDataParserTest {
assertEquals(listOf("pending"), asked.request.ruleDecisions.map { it.decision })
}
@Test
fun `parsePermissionRequest - uses always when metadata rules are empty`() {
val data = globalEvent("""
"type": "permission.asked",
"properties": {
"id": "perm_empty_rules",
"sessionID": "ses_1",
"permission": "bash",
"patterns": ["git add ."],
"always": ["git add *"],
"metadata": {"rules": []}
}
""")
val result = KiloCliDataParser.parseChatEvent("permission.asked", data)
assertNotNull(result)
val asked = result as? ChatEventDto.PermissionAsked ?: error("Expected PermissionAsked")
assertEquals(emptyList(), asked.request.rules)
assertEquals(listOf("git add *"), asked.request.ruleDecisions.map { it.pattern })
assertEquals(listOf("pending"), asked.request.ruleDecisions.map { it.decision })
}
@Test
fun `parsePermissionRequest - diff and filepath fallback`() {
val data = globalEvent("""
@@ -216,12 +216,17 @@ class KiloWorkspaceService internal constructor(
}
}
fun refreshConfigFiles(directory: String) {
cs.launch {
call { refreshConfigFiles(directory) }
localConfigTarget(directory)
globalConfigTarget()
ActivityTracker.getInstance().inc()
fun refreshConfigFiles(directory: String): Job {
return cs.launch {
try {
call { refreshConfigFiles(directory) }
localConfigTarget(directory)
globalConfigTarget()
} catch (e: Exception) {
LOG.warn("config file refresh failed for directory=$directory", e)
} finally {
ActivityTracker.getInstance().inc()
}
}
}
@@ -660,8 +660,10 @@ class SessionController(
updatePermission(requestId, PermissionRequestState.RESPONDING)
cs.launch {
try {
if (rules != null) sessions.savePermissionRules(requestId, directory, rules)
if (rules != null) workspace.refreshConfigFiles()
if (rules != null) {
sessions.savePermissionRules(requestId, directory, rules)
workspace.refreshConfigFiles()
}
sessions.replyPermission(requestId, directory, reply)
capture("Approval Answered", sessionProps() + mapOf(
"requestId" to requestId,
@@ -76,6 +76,7 @@ class PermissionView(
override val sessionViewKind = SessionView.Kind.Default
private var requestId: String? = null
private var responding = false
private var style = SessionEditorStyle.current()
private val card = BaseQuestionView(selection, focus)
@@ -125,11 +126,11 @@ class PermissionView(
val target = if (tool == "bash") permission.meta.command else resolveTarget(permission)
syncCode(tool, target)
syncDiffs(permission.meta.fileDiffs)
responding = permission.state == PermissionRequestState.RESPONDING || permission.state == PermissionRequestState.RESOLVED
rules.update(permission.meta.ruleDecisions, reset = prev != permission.id)
syncState(permission)
syncPrimaryText()
val responding = permission.state == PermissionRequestState.RESPONDING || permission.state == PermissionRequestState.RESOLVED
syncButtons(responding)
rules.setControlsEnabled(!responding)
@@ -141,6 +142,7 @@ class PermissionView(
@RequiresEdt
fun hideView() {
requestId = null
responding = false
disposeMd()
diffViews.clear()
diffRow.removeAll()
@@ -386,7 +388,7 @@ class PermissionView(
ID_DENY,
KiloBundle.message("session.permission.reject"),
)
syncButtons(false)
syncButtons(responding)
}
private fun refresh() {
@@ -97,4 +97,16 @@ class KiloWorkspaceServiceTest : BasePlatformTestCase() {
assertEquals(err.message, seen?.message)
assertEquals(listOf("dep"), rpc.searchQueries)
}
fun `test refreshConfigFiles logs backend failure and completes`() = runBlocking {
rpc.refreshConfigThrows = IllegalStateException("backend unavailable")
val job = service.refreshConfigFiles("/test")
job.join()
assertTrue(job.isCompleted)
assertEquals(listOf("/test"), rpc.refreshedConfigs.toList())
assertEquals(0, rpc.localConfigPathCalls)
assertEquals(0, rpc.globalConfigPathCalls)
}
}
@@ -563,6 +563,7 @@ class PromptLifecycleTest : SessionControllerTestBase() {
flush()
assertTrue(rpc.permissionRulesSaved.isEmpty())
assertTrue(projectRpc.refreshedConfigs.isEmpty())
assertEquals(1, rpc.permissionReplies.size)
}
@@ -386,6 +386,27 @@ class PermissionViewTest : BasePlatformTestCase() {
assertFalse(view.denyButtonForTest().isEnabled)
}
fun `test responding state keeps buttons disabled when rules change`() {
view.show(
Permission(
id = "perm_responding_rules",
sessionId = "ses",
name = "bash",
patterns = listOf("git status"),
always = listOf("git status"),
meta = PermissionMeta(
ruleDecisions = listOf(PermissionRuleCandidate("git status")),
),
state = PermissionRequestState.RESPONDING,
)
)
view.rulesForTest().update(listOf(PermissionRuleCandidate("git status", PermissionRuleDecision.APPROVED)))
assertFalse(view.runButtonForTest().isEnabled)
assertFalse(view.denyButtonForTest().isEnabled)
}
fun `test responding state shows responding message`() {
view.show(
Permission(
@@ -43,6 +43,7 @@ class FakeWorkspaceRpcApi : KiloWorkspaceRpcApi {
var globalConfigExists = true
var beforeLocalConfigTarget: (suspend () -> Unit)? = null
var beforeGlobalConfigTarget: (suspend () -> Unit)? = null
var refreshConfigThrows: Exception? = null
val fileCalls = CopyOnWriteArrayList<Pair<String, String>>()
val searchQueries = CopyOnWriteArrayList<String>()
val opened = CopyOnWriteArrayList<String>()
@@ -117,6 +118,7 @@ class FakeWorkspaceRpcApi : KiloWorkspaceRpcApi {
override suspend fun refreshConfigFiles(directory: String) {
assertNotEdt("refreshConfigFiles")
refreshedConfigs.add(directory)
refreshConfigThrows?.let { throw it }
}
override suspend fun openLocalConfig(directory: String): Boolean {