From e984ad6fdd6eef842770b50b8fbf5c824226b8fa Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:17:28 -0500 Subject: [PATCH 1/5] fix(panel): don't let a dismissed approval silence later requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dismissing an approval hides it but deliberately leaves it queued, so the CLI stays blocked and the prompt stays recoverable. The card-and-sound trigger in handlePermissionRequest was gated on `permissionQueue.count == 1`, which a dismissed entry keeps false forever — so every later permission request, from every session, arrived with no card and no sound until the dismissed one was resolved some other way. Queue size was standing in for "is a card already showing", and dismissal is exactly the state where those two stop agreeing. The gate now asks the same predicate the display path uses, nextVisiblePermissionIndex(), which already skips dismissed sessions. Showing the card via showNextPending() rather than pointing it at this session by hand matters: with a dismissed entry still leading the queue, the approval card renders the head, so a hand-set surface would show the dismissed request's content under the new session's name. The dismissed session's own next request stays hidden — dismissal is per-session and clears when that session's request resolves. That behaviour is unchanged and now pinned by a test so this fix isn't read as altering it. Fixes #309. 689 tests, 0 failures; the new regression test was confirmed to fail against the old gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 21 +++-- .../AppStatePermissionFlowTests.swift | 84 +++++++++++++++++++ 2 files changed, 97 insertions(+), 8 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index d3715c7f..ec194af8 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1310,16 +1310,21 @@ final class AppState { return } + // Queue size is not "is a card showing": dismissing hides a request but + // deliberately leaves it queued, so the CLI stays blocked and the prompt + // stays recoverable. Gating on `count == 1` therefore swallowed every + // later request — from any session — for as long as a dismissed one sat + // in the queue. Ask the same predicate the display path uses. (#309) + let wasShowingPermission = nextVisiblePermissionIndex() != nil permissionQueue.append(request) - // Show UI only if this is the first (or only) queued item - if permissionQueue.count == 1 { - activeSessionId = sessionId - // If user is already browsing the session list, keep them there and - // let inline controls handle approval without stealing focus. - if surface != .sessionList, shouldAutoOpenPendingSurface(for: sessionId) { - surface = .approvalCard(sessionId: sessionId) - } + // Show UI only when nothing was already on screen to be stolen from. + if !wasShowingPermission, nextVisiblePermissionIndex() != nil { + // showNextPending picks the first *visible* request, promotes it to + // the head and applies the session-list / Smart Suppress rules — + // pointing the card at this session by hand would show the dismissed + // request's content whenever a dismissed entry still leads the queue. + showNextPending() SoundManager.shared.handleEvent("PermissionRequest") } refreshDerivedState() diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 3ab9c10d..1b0163c7 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -532,6 +532,90 @@ final class AppStatePermissionFlowTests: XCTestCase { try String(contentsOf: codeIslandRulesPath(in: codexHome), encoding: .utf8) } + /// #309 — a dismissed request stays queued so the CLI stays blocked, which + /// used to make `permissionQueue.count == 1` false forever and swallow every + /// later request, from every session, with no card and no sound. + func testDismissedPermissionDoesNotSilenceALaterSessionsRequest() async throws { + let appState = AppState() + let dismissed = try makePermissionRequestEvent(sessionId: "s-dismissed", toolName: "Bash") + let later = try makePermissionRequestEvent(sessionId: "s-later", toolName: "Edit") + + let dismissedTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(dismissed, continuation: continuation) + } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-dismissed")) + + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual(appState.permissionQueue.count, 1, "dismiss must keep the request queued") + + let laterTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(later, continuation: continuation) + } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-later"), + "a different session's approval must still raise a card while a dismissed one sits in the queue" + ) + // Stop here on failure: under the bug, approvePermission() below resolves + // the dismissed request instead, so `await laterTask.value` would hang + // and the test would report as a timeout rather than by name. + guard appState.surface == .approvalCard(sessionId: "s-later") else { + appState.handlePeerDisconnect(sessionId: "s-dismissed") + appState.handlePeerDisconnect(sessionId: "s-later") + _ = await dismissedTask.value + _ = await laterTask.value + return + } + + appState.approvePermission() + let laterResponse = await laterTask.value + XCTAssertEqual(try extractPermissionBehavior(from: laterResponse), "allow") + + await assertTaskNotResolved(dismissedTask) + appState.handlePeerDisconnect(sessionId: "s-dismissed") + _ = await dismissedTask.value + } + + /// The dismissed session's own next request stays hidden — dismissal is + /// per-session and is only cleared when that session's request resolves. + /// Pinned so the #309 fix is not read as changing it. + func testDismissedSessionsOwnNextRequestStaysHidden() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Bash") + let second = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Edit") + + let firstTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(first, continuation: continuation) + } + } + await Task.yield() + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + + let secondTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(second, continuation: continuation) + } + } + await Task.yield() + + XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual(appState.permissionQueue.count, 2) + + appState.handlePeerDisconnect(sessionId: "s-same") + _ = await firstTask.value + _ = await secondTask.value + } + private func makePermissionRequestEvent( sessionId: String, toolName: String, From 29e1456064204251fe0f4b9bc57793845d4124fb Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:38:48 -0500 Subject: [PATCH 2/5] =?UTF-8?q?fix:=20address=20QA=20round=201=20=E2=80=94?= =?UTF-8?q?=20gate=20on=20the=20surface,=20not=20the=20queue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA found the first attempt incomplete. `nextVisiblePermissionIndex() != nil` is "a non-dismissed request is queued", which is still not "a card is on screen": handlePermissionRequest un-dismisses the session on entry ("session needs user decision again"), so a dismissed session's NEXT request makes its own still-queued earlier request count as visible while nothing is displayed. The silencing then resumed exactly as #309 described, one step later. The gate now asks the surface directly. That also settles what the previous commit's pinning test got backwards: a dismissal is cleared by the session's next request arriving, not by the dismissed request resolving, so that session's next request must bring its card back. The old test asserted the broken state was correct and its doc comment stated an invariant the code does not have; both are corrected. Also strengthened, per QA: - the cross-session regression test's anti-hang guard checked only the surface; an implementation that points the card at the arriving session by hand passes that while the dismissed request still leads the queue, so approve resolves the wrong one and the await hangs. It now checks the queue head too. - the new "asking again doesn't silence others" test passed under the broken gate as originally written (resolving A's requests surfaces B either way). It now asserts a card is on screen at the moment B arrives, which is the state that actually differs. Known limitation, unchanged from main and noted in the code: a card suppressed by Smart Suppress also leaves a visible request undisplayed, so a second session's request waits behind it. Closing that needs showNextPending to skip un-openable entries; tracked separately. 690 tests, 0 failures; both new guards verified red against the incomplete gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 27 ++++-- .../AppStatePermissionFlowTests.swift | 85 +++++++++++++++++-- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index ec194af8..a9bf76b1 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1310,16 +1310,27 @@ final class AppState { return } - // Queue size is not "is a card showing": dismissing hides a request but - // deliberately leaves it queued, so the CLI stays blocked and the prompt - // stays recoverable. Gating on `count == 1` therefore swallowed every - // later request — from any session — for as long as a dismissed one sat - // in the queue. Ask the same predicate the display path uses. (#309) - let wasShowingPermission = nextVisiblePermissionIndex() != nil + // Dismissing hides a request but deliberately leaves it queued, so the + // CLI stays blocked and the prompt stays recoverable. Gating on + // `permissionQueue.count == 1` therefore swallowed every later request — + // from any session — for as long as a dismissed one sat in the queue. + // + // The gate's real question is "is an approval card on screen", so ask + // the surface. Queue-derived proxies do not survive the un-dismiss + // above: a session's own next request clears its dismissal, which makes + // its still-queued earlier request count as visible again while nothing + // is displayed — silencing every later request all over again. (#309) + // + // ponytail: a card suppressed by Smart Suppress also leaves a visible + // request undisplayed, so a second session's request still waits behind + // it. That is pre-existing (`main` behaves the same) and needs + // showNextPending to skip un-openable entries; tracked separately. + let approvalCardOnScreen: Bool + if case .approvalCard = surface { approvalCardOnScreen = true } else { approvalCardOnScreen = false } permissionQueue.append(request) - // Show UI only when nothing was already on screen to be stolen from. - if !wasShowingPermission, nextVisiblePermissionIndex() != nil { + // Show UI only when no approval card is already up to be stolen from. + if !approvalCardOnScreen, nextVisiblePermissionIndex() != nil { // showNextPending picks the first *visible* request, promotes it to // the head and applies the session-list / Smart Suppress rules — // pointing the card at this session by hand would show the dismissed diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 1b0163c7..bec286bd 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -566,8 +566,18 @@ final class AppStatePermissionFlowTests: XCTestCase { ) // Stop here on failure: under the bug, approvePermission() below resolves // the dismissed request instead, so `await laterTask.value` would hang - // and the test would report as a timeout rather than by name. - guard appState.surface == .approvalCard(sessionId: "s-later") else { + // and the test would report as a timeout rather than by name. The head + // check matters as much as the surface one — an implementation that + // points the card at this session by hand shows "s-later" while the + // dismissed request still leads the queue, so the surface assertion + // alone passes and the await still hangs. + XCTAssertEqual( + appState.permissionQueue.first?.event.sessionId, + "s-later", + "the card on screen must be backed by the head of the queue, which is what approve resolves" + ) + guard appState.surface == .approvalCard(sessionId: "s-later"), + appState.permissionQueue.first?.event.sessionId == "s-later" else { appState.handlePeerDisconnect(sessionId: "s-dismissed") appState.handlePeerDisconnect(sessionId: "s-later") _ = await dismissedTask.value @@ -584,10 +594,15 @@ final class AppStatePermissionFlowTests: XCTestCase { _ = await dismissedTask.value } - /// The dismissed session's own next request stays hidden — dismissal is - /// per-session and is only cleared when that session's request resolves. - /// Pinned so the #309 fix is not read as changing it. - func testDismissedSessionsOwnNextRequestStaysHidden() async throws { + /// A dismissal is cleared by that session's NEXT request arriving + /// (`handlePermissionRequest` removes it from `dismissedPermissionSessionIds` + /// on entry — "session needs user decision again"), not by the dismissed + /// request resolving. So the session's next request must bring its card back. + /// + /// This is also the state that silenced everything else: the un-dismiss makes + /// the still-queued earlier request count as visible while nothing is on + /// screen, so a queue-derived "is a card showing" proxy reads true forever. + func testDismissedSessionsNextRequestReRaisesItsCard() async throws { let appState = AppState() let first = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Bash") let second = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Edit") @@ -608,7 +623,11 @@ final class AppStatePermissionFlowTests: XCTestCase { } await Task.yield() - XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-same"), + "the session un-dismissed itself by asking again, so its card must come back" + ) XCTAssertEqual(appState.permissionQueue.count, 2) appState.handlePeerDisconnect(sessionId: "s-same") @@ -616,6 +635,58 @@ final class AppStatePermissionFlowTests: XCTestCase { _ = await secondTask.value } + /// The state F1 described: a dismissed session asking again must not leave + /// the panel silent for everyone else. + func testDismissedSessionAskingAgainDoesNotSilenceOtherSessions() async throws { + let appState = AppState() + let firstA = try makePermissionRequestEvent(sessionId: "s-a", toolName: "Bash") + let secondA = try makePermissionRequestEvent(sessionId: "s-a", toolName: "Edit") + let fromB = try makePermissionRequestEvent(sessionId: "s-b", toolName: "Read") + + let firstATask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(firstA, continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt() + + let secondATask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(secondA, continuation: $0) } + } + await Task.yield() + + let fromBTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(fromB, continuation: $0) } + } + await Task.yield() + + // The panel must be showing *something* — under the incomplete gate the + // un-dismiss left A's card unopened and B arrived to a silent, collapsed + // panel. (Without this the rest of the test passes either way, because + // resolving A's requests surfaces B regardless.) + XCTAssertNotEqual( + appState.surface, + .collapsed, + "a card must be on screen — a silent collapsed panel is the bug" + ) + + // B queues behind A's card rather than stealing it — but it must not be + // lost: as A's requests clear, B's card has to come up. + XCTAssertEqual(appState.permissionQueue.count, 3) + appState.approvePermission() + appState.approvePermission() + _ = await firstATask.value + _ = await secondATask.value + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-b"), + "B's request must surface once A's are resolved, not sit silently forever" + ) + appState.approvePermission() + let bResponse = await fromBTask.value + XCTAssertEqual(try extractPermissionBehavior(from: bResponse), "allow") + } + private func makePermissionRequestEvent( sessionId: String, toolName: String, From f3642dda20d57463f20344fa6c16e3c34a8df1c9 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:52:49 -0500 Subject: [PATCH 3/5] =?UTF-8?q?fix:=20address=20QA=20round=202=20=E2=80=94?= =?UTF-8?q?=20the=20card=20gate=20needs=20both=20halves?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1's surface-only gate had its own hole, found by two lenses independently: drainPermissions (process exit, or a question arriving for the session) empties the queue without clearing `surface`, and the card renders nothing when there is no head request. A bare `.approvalCard` check therefore blocked on a phantom card and swallowed the next request from every session — the same #309 defect, third variant. The gate now requires both an .approvalCard surface and a non-empty queue. Card and sound also stopped sharing a gate. They answer different questions: the card asks "is one already on screen", the sound asks "does this request start a new burst" — which is what `count == 1` used to approximate. Sharing the surface-derived gate made the sound fire per request instead of per burst whenever no card was open, e.g. under Smart Suppress. The sound now keys off whether a visible request was already queued, which keeps burst behaviour and still fixes the dismissed-request silence. Tests, per QA: - new: a request arriving under a stale .approvalCard surface must still raise a card (verified red against round 1's gate). - the cross-session test asserted only "not collapsed", the weakest thing available; it now names the card it expects. - the re-raise test could not tell showNextPending's promotion from a hand-pointed surface, both requests being from one session; it now asserts the card shows the earlier queued request, not the arriving one. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 30 +++++++++--- .../AppStatePermissionFlowTests.swift | 46 +++++++++++++++++-- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index a9bf76b1..be8596d1 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1325,17 +1325,35 @@ final class AppState { // request undisplayed, so a second session's request still waits behind // it. That is pre-existing (`main` behaves the same) and needs // showNextPending to skip un-openable entries; tracked separately. + // + // The surface alone is not enough either: `drainPermissions` empties the + // queue without clearing `surface`, and the card renders nothing when + // there is no head request — so a bare `.approvalCard` check would block + // on a card that is not actually there. Both halves are required. let approvalCardOnScreen: Bool - if case .approvalCard = surface { approvalCardOnScreen = true } else { approvalCardOnScreen = false } + if case .approvalCard = surface, !permissionQueue.isEmpty { + approvalCardOnScreen = true + } else { + approvalCardOnScreen = false + } + + // Card and sound answer different questions and must not share a gate. + // The sound marks the start of a burst of approvals, which is what + // `count == 1` used to approximate; within a burst it stays quiet, and + // a dismissed request sitting in the queue must not count as a burst + // already in progress. + let burstAlreadyInProgress = nextVisiblePermissionIndex() != nil permissionQueue.append(request) // Show UI only when no approval card is already up to be stolen from. - if !approvalCardOnScreen, nextVisiblePermissionIndex() != nil { - // showNextPending picks the first *visible* request, promotes it to - // the head and applies the session-list / Smart Suppress rules — - // pointing the card at this session by hand would show the dismissed - // request's content whenever a dismissed entry still leads the queue. + // showNextPending picks the first *visible* request, promotes it to the + // head and applies the session-list / Smart Suppress rules — pointing + // the card at this session by hand would show the dismissed request's + // content whenever a dismissed entry still leads the queue. + if !approvalCardOnScreen { showNextPending() + } + if !burstAlreadyInProgress { SoundManager.shared.handleEvent("PermissionRequest") } refreshDerivedState() diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index bec286bd..06cb1d19 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -629,12 +629,52 @@ final class AppStatePermissionFlowTests: XCTestCase { "the session un-dismissed itself by asking again, so its card must come back" ) XCTAssertEqual(appState.permissionQueue.count, 2) + // Both requests are from the same session, so the surface alone cannot + // tell "showNextPending promoted the queue head" from "the surface was + // pointed at the arriving request". The card renders the head, so the + // user must be asked about the earlier request, not the newer one. + XCTAssertEqual( + appState.pendingPermission?.event.toolName, + "Bash", + "the card must show the earlier queued request, not the one that just arrived" + ) appState.handlePeerDisconnect(sessionId: "s-same") _ = await firstTask.value _ = await secondTask.value } + /// `drainPermissions` (process exit, or a question arriving for the session) + /// empties the queue without clearing `surface`, and the card renders nothing + /// when there is no head request. A gate that trusts `.approvalCard` alone + /// would block on that phantom card and swallow the next request. + func testRequestArrivingUnderAStaleApprovalSurfaceStillRaisesACard() async throws { + let appState = AppState() + + // The phantom state itself: an .approvalCard surface with an empty queue. + // Production reaches it through the drainPermissions callers that do not + // touch `surface` (process exit; a question arriving for the session). + // Set here directly because those callers are private. + appState.surface = .approvalCard(sessionId: "s-gone") + XCTAssertTrue(appState.permissionQueue.isEmpty) + + let next = try makePermissionRequestEvent(sessionId: "s-next", toolName: "Read") + let nextTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(next, continuation: $0) } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-next"), + "a phantom card must not block the next request from being shown" + ) + + appState.approvePermission() + let response = await nextTask.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + } + /// The state F1 described: a dismissed session asking again must not leave /// the panel silent for everyone else. func testDismissedSessionAskingAgainDoesNotSilenceOtherSessions() async throws { @@ -663,10 +703,10 @@ final class AppStatePermissionFlowTests: XCTestCase { // un-dismiss left A's card unopened and B arrived to a silent, collapsed // panel. (Without this the rest of the test passes either way, because // resolving A's requests surfaces B regardless.) - XCTAssertNotEqual( + XCTAssertEqual( appState.surface, - .collapsed, - "a card must be on screen — a silent collapsed panel is the bug" + .approvalCard(sessionId: "s-a"), + "A's card must be up — a silent collapsed panel is the bug, and it must be A's card since A's request leads the queue" ) // B queues behind A's card rather than stealing it — but it must not be From c86fe891864254270664e169f5b22a2804ffb2ca Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 11:04:35 -0500 Subject: [PATCH 4/5] docs(test): correct a comment that overstated what its assertion pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 finding. The assertion checks that the card renders the queue head, so the user is asked about the earlier request — it pins queue order, not which mechanism set the surface. With both requests from one session no assertion at that level can tell showNextPending's promotion from a hand-pointed surface; the cross-session test's head check is what covers that. Comment only. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- .../CodeIslandTests/AppStatePermissionFlowTests.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 06cb1d19..ed6fd192 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -629,10 +629,12 @@ final class AppStatePermissionFlowTests: XCTestCase { "the session un-dismissed itself by asking again, so its card must come back" ) XCTAssertEqual(appState.permissionQueue.count, 2) - // Both requests are from the same session, so the surface alone cannot - // tell "showNextPending promoted the queue head" from "the surface was - // pointed at the arriving request". The card renders the head, so the - // user must be asked about the earlier request, not the newer one. + // Pins queue order, not the promotion mechanism: the card renders the + // head, so the user is asked about the earlier request rather than the + // one that just arrived. Both requests here are from one session, so no + // assertion at this level can tell showNextPending's promotion from a + // hand-pointed surface — the cross-session test's head check is what + // covers that. XCTAssertEqual( appState.pendingPermission?.event.toolName, "Bash", From 2e497e6e7936b9894dbabac0bd8c56dbdbf92331 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 12:48:22 -0500 Subject: [PATCH 5/5] fix(panel): per-session card gate, and don't un-dismiss on a replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found by reviewing this change merged with the #308 answer-routing fix. Both are present on this branch alone, so they belong here rather than in the integration. 1. The gate asked `!permissionQueue.isEmpty` — a whole-queue question — while staleness is per-session. `drainPermissions` empties one SESSION's requests without clearing `surface` (a question arriving for that session does this), so a card can be left pointing at a session with nothing queued while others still wait. The whole-queue test reads that as "a card is up", and every later request queues silently behind a card for a session that has no pending request. Now asked per session. 2. `dismissedPermissionSessionIds.remove(sessionId)` ran before the replay-dedup early return, so a replayed tool_use_id for a dismissed session resurrected the request the user had hidden — taking the card the arriving session should have received, and counting as a burst already in progress so that session lost its sound too. A replay is the same decision arriving twice, not a new one. A same-id request with *differing* tool inputs is a distinct request (#169) and still clears the dismissal; that path is pinned too. This one is latent on main as well: a resurrected request leads nextVisiblePermissionIndex(), so any later showNextPending() can raise the card the user dismissed. New AppStatePermissionGateTests covers all three cases. Both fixes verified by mutating the production line each names and confirming the test fails — and the replay test stops on that failure instead of hanging on the await that follows. 694 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 27 ++- .../AppStatePermissionGateTests.swift | 202 ++++++++++++++++++ 2 files changed, 221 insertions(+), 8 deletions(-) create mode 100644 Tests/CodeIslandTests/AppStatePermissionGateTests.swift diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index be8596d1..a9412135 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1287,9 +1287,6 @@ final class AppState { return } - // New incoming permission request means session needs user decision again. - dismissedPermissionSessionIds.remove(sessionId) - // Clear any pending questions for THIS session (mutually exclusive within a session) drainQuestions(forSession: sessionId, reason: "newPermissionRequest") @@ -1310,6 +1307,16 @@ final class AppState { return } + // A genuinely new request means this session needs a user decision again, + // so it stops being dismissed. This must come AFTER the replay-dedup + // return above: a replay is the same decision arriving twice, not a new + // one, and un-dismissing on a replay resurrects the request the user + // hid — which then takes the card the arriving session should have got + // and, counting as a burst already in progress, silences its sound too. + // (A same-id request with different tool inputs is a distinct request, + // not a replay — merge returns false for those, so they still land here.) + dismissedPermissionSessionIds.remove(sessionId) + // Dismissing hides a request but deliberately leaves it queued, so the // CLI stays blocked and the prompt stays recoverable. Gating on // `permissionQueue.count == 1` therefore swallowed every later request — @@ -1326,12 +1333,16 @@ final class AppState { // it. That is pre-existing (`main` behaves the same) and needs // showNextPending to skip un-openable entries; tracked separately. // - // The surface alone is not enough either: `drainPermissions` empties the - // queue without clearing `surface`, and the card renders nothing when - // there is no head request — so a bare `.approvalCard` check would block - // on a card that is not actually there. Both halves are required. + // The surface alone is not enough either: `drainPermissions` empties one + // SESSION's requests without clearing `surface`, so a card can be left + // pointing at a session that has nothing queued. Ask per session, not + // per queue — a whole-queue test (`!permissionQueue.isEmpty`) reads as + // "a card is up" whenever some other session is still waiting, which + // leaves the panel showing a card for a session with no pending request + // while later requests queue silently behind it. let approvalCardOnScreen: Bool - if case .approvalCard = surface, !permissionQueue.isEmpty { + if case .approvalCard(let shownSessionId) = surface, + permissionQueue.contains(where: { ($0.event.sessionId ?? "default") == shownSessionId }) { approvalCardOnScreen = true } else { approvalCardOnScreen = false diff --git a/Tests/CodeIslandTests/AppStatePermissionGateTests.swift b/Tests/CodeIslandTests/AppStatePermissionGateTests.swift new file mode 100644 index 00000000..d35bcb8e --- /dev/null +++ b/Tests/CodeIslandTests/AppStatePermissionGateTests.swift @@ -0,0 +1,202 @@ +import XCTest +@testable import CodeIsland +import CodeIslandCore + +/// The enqueue gate decides whether an arriving permission raises a card. It has +/// two failure modes that the #309 tests alone do not reach: asking about the +/// whole queue when staleness is per-session, and letting a replayed request +/// resurrect a session the user dismissed. +@MainActor +final class AppStatePermissionGateTests: XCTestCase { + + /// A card can be left pointing at a session whose own request was drained + /// (a question arriving for that session does this) while other sessions are + /// still queued. A whole-queue test reads that as "a card is up" and queues + /// every later request silently behind a card for a session that has nothing + /// pending. + func testCardForADrainedSessionDoesNotBlockLaterRequests() async throws { + let appState = AppState() + + // Occupy the question queue first, so the question in step 3 does not + // reassign the surface. + let cTask = Task { + await withCheckedContinuation { appState.handleQuestion(try! self.question("s-c"), continuation: $0) } + } + await Task.yield() + + let aTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-a", "Bash"), continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-a")) + + let bTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-b", "Read"), continuation: $0) } + } + await Task.yield() + + // A question for s-a drains s-a's permission; s-b's is untouched. + let aQuestionTask = Task { + await withCheckedContinuation { appState.handleQuestion(try! self.question("s-a"), continuation: $0) } + } + await Task.yield() + _ = await aTask.value + XCTAssertFalse( + appState.permissionQueue.contains { $0.event.sessionId == "s-a" }, + "s-a has nothing queued" + ) + XCTAssertFalse(appState.permissionQueue.isEmpty, "but the queue is not empty") + + let dTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-b"), + "the panel must move to a request that is actually waiting, not stay on the drained session's card" + ) + + appState.handlePeerDisconnect(sessionId: "s-b") + appState.handlePeerDisconnect(sessionId: "s-d") + appState.handlePeerDisconnect(sessionId: "s-c") + appState.handlePeerDisconnect(sessionId: "s-a") + _ = await bTask.value + _ = await dTask.value + _ = await cTask.value + _ = await aQuestionTask.value + } + + /// A replay of the same `tool_use_id` is the same decision arriving twice. + /// Clearing the dismissal on a replay resurrects the request the user hid, + /// which then takes the card the arriving session should have received. + func testReplayOfADismissedRequestDoesNotStealTheNextSessionsCard() async throws { + let appState = AppState() + + let originalTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-replay", "tool-1"), continuation: $0) + } + } + await Task.yield() + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + + let replayTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-replay", "tool-1"), continuation: $0) + } + } + await Task.yield() + _ = await originalTask.value // the replay denies the previous waiter + XCTAssertEqual(appState.permissionQueue.count, 1, "a replay swaps in place, it does not enqueue") + XCTAssertEqual(appState.surface, .collapsed, "a replay must not resurrect the dismissed card") + + let otherTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-other", "Read"), continuation: $0) } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-other"), + "the arriving session must get the card, not the replayed-and-resurrected one" + ) + // Stop on failure. Under the bug the resurrected session leads the queue, + // so the approve below resolves that one instead and the await never + // returns — the test would hang rather than report by name. + guard appState.permissionQueue.first?.event.sessionId == "s-other" else { + appState.handlePeerDisconnect(sessionId: "s-replay") + appState.handlePeerDisconnect(sessionId: "s-other") + _ = await replayTask.value + _ = await otherTask.value + return + } + + appState.approvePermission() + let otherResponse = await otherTask.value + XCTAssertEqual(try extractBehavior(from: otherResponse), "allow") + + appState.handlePeerDisconnect(sessionId: "s-replay") + _ = await replayTask.value + } + + /// The other side of that move: `mergeDuplicatePermissionRequest` returns + /// false when the tool inputs differ (#169 — parallel calls can share an id), + /// so such a request does enqueue and must still clear the dismissal. + func testSameToolUseIdWithDifferentInputStillClearsTheDismissal() async throws { + let appState = AppState() + + let firstTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-parallel", "tool-9"), continuation: $0) + } + } + await Task.yield() + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + + let secondTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest( + try! self.permWithToolUse("s-parallel", "tool-9", command: "echo different"), + continuation: $0 + ) + } + } + await Task.yield() + + XCTAssertEqual(appState.permissionQueue.count, 2, "differing inputs must enqueue, not merge") + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-parallel"), + "a genuinely new request must clear the dismissal and bring the card back" + ) + + appState.handlePeerDisconnect(sessionId: "s-parallel") + _ = await firstTask.value + _ = await secondTask.value + } + + // MARK: - Helpers + + private func perm(_ sessionId: String, _ toolName: String) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": toolName, + "tool_input": ["command": "echo test"], + ]))) + } + + private func permWithToolUse( + _ sessionId: String, + _ toolUseId: String, + command: String = "echo test" + ) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": "Bash", + "tool_use_id": toolUseId, + "tool_input": ["command": command], + ]))) + } + + private func question(_ sessionId: String) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "Notification", + "session_id": sessionId, + "question": "Pick?", + "options": ["A", "B"], + ]))) + } + + private func extractBehavior(from data: Data) throws -> String { + let json = try XCTUnwrap(try JSONSerialization.jsonObject(with: data) as? [String: Any]) + let output = try XCTUnwrap(json["hookSpecificOutput"] as? [String: Any]) + let decision = try XCTUnwrap(output["decision"] as? [String: Any]) + return try XCTUnwrap(decision["behavior"] as? String) + } +}