diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index d3715c7f..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,16 +1307,64 @@ 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 — + // 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. + // + // 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(let shownSessionId) = surface, + permissionQueue.contains(where: { ($0.event.sessionId ?? "default") == shownSessionId }) { + 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 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 no approval card is already up to be stolen from. + // 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 3ab9c10d..ed6fd192 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -532,6 +532,203 @@ 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. 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 + _ = 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 + } + + /// 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") + + 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, + .approvalCard(sessionId: "s-same"), + "the session un-dismissed itself by asking again, so its card must come back" + ) + XCTAssertEqual(appState.permissionQueue.count, 2) + // 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", + "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 { + 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.) + XCTAssertEqual( + appState.surface, + .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 + // 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, 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) + } +}