diff --git a/Sources/CodeIsland/AppDelegate.swift b/Sources/CodeIsland/AppDelegate.swift index 15b13d63..56e03851 100644 --- a/Sources/CodeIsland/AppDelegate.swift +++ b/Sources/CodeIsland/AppDelegate.swift @@ -231,14 +231,16 @@ class AppDelegate: NSObject, NSApplicationDelegate { } } } + // Shortcuts act on the card currently on screen, so they target that + // card's session rather than the head of the queue. (#308) case .approve: - appState.approvePermission() + appState.approvePermission(expectedSessionId: appState.surface.approvalSessionId) case .approveAlways: - appState.approvePermission(always: true) + appState.approvePermission(always: true, expectedSessionId: appState.surface.approvalSessionId) case .deny: - appState.denyPermission() + appState.denyPermission(expectedSessionId: appState.surface.approvalSessionId) case .skipQuestion: - appState.skipQuestion() + appState.skipQuestion(expectedSessionId: appState.surface.questionSessionId) case .jumpToTerminal: if let id = appState.activeSessionId, let session = appState.sessions[id] { TerminalActivator.activate(session: session, sessionId: id) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index d3715c7f..2de439d5 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -125,6 +125,27 @@ final class AppState { var pendingPermission: PermissionRequest? { permissionQueue.first } /// Computed: first item in question queue var pendingQuestion: QuestionRequest? { questionQueue.first } + + /// The queued request belonging to a specific session. A card is addressed + /// by session, so it must render (and resolve) that session's request + /// rather than whatever currently sits at the head of the queue. (#308) + func pendingPermission(forSession sessionId: String) -> PermissionRequest? { + permissionQueue.first { ($0.event.sessionId ?? "default") == sessionId } + } + + func pendingQuestion(forSession sessionId: String) -> QuestionRequest? { + questionQueue.first { ($0.event.sessionId ?? "default") == sessionId } + } + + /// 1-based position for a card's "N of M" label. The card may be showing a + /// request that is not the head, so the position has to be looked up. (#308) + func permissionQueuePosition(forSession sessionId: String) -> Int { + (permissionQueue.firstIndex { ($0.event.sessionId ?? "default") == sessionId } ?? 0) + 1 + } + + func questionQueuePosition(forSession sessionId: String) -> Int { + (questionQueue.firstIndex { ($0.event.sessionId ?? "default") == sessionId } ?? 0) + 1 + } /// Preview-only: mock question payload for DebugHarness (no continuation needed) var previewQuestionPayload: QuestionPayload? var surface: IslandSurface = .collapsed { @@ -1287,9 +1308,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,24 +1328,107 @@ 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 hidden request — + // which then suppresses the next session's sound and can put the + // resurrected session's card on screen instead of the arriving one. + 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`, and a card renders + // nothing when its own session has no queued request — so a bare + // `.approvalCard` check would block on a card that is not actually + // there. Ask exactly what the card asks: is this session's request + // still queued? A whole-queue test (`!permissionQueue.isEmpty`) gets + // this wrong whenever another session still has one, which also + // suppresses the stale-card collapse inside showNextPending() and + // wedges the panel behind a dead card. (#308 + #309) + let approvalCardOnScreen: Bool + if case .approvalCard(let shownSessionId) = surface, + pendingPermission(forSession: shownSessionId) != nil { + 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() } - func approvePermission(always: Bool = false) { - guard !permissionQueue.isEmpty else { return } - let pending = permissionQueue.removeFirst() + /// Index of the queued request the user actually acted on. + /// + /// The card on screen is identified by its session, but the answer used to + /// be applied to `queue.removeFirst()`. Anything that mutates the head + /// while a card is open — a peer disconnect draining another session, a + /// stale tool-use eviction, the reorder in `showNextPending()` — would then + /// resolve whichever request happened to be first, delivering the answer to + /// the wrong CLI. Callers that know which session the card belongs to pass + /// it in; `nil` keeps the head-of-queue behaviour for surfaces that only + /// ever mirror the head (keyboard shortcuts, iPhone/Watch Buddy). (#308) + private func permissionIndex(expecting expected: String?) -> Int? { + guard let expected else { return permissionQueue.isEmpty ? nil : 0 } + return permissionQueue.firstIndex { ($0.event.sessionId ?? "default") == expected } + } + + /// Question-queue counterpart of `permissionIndex(expecting:)`. (#308) + private func questionIndex(expecting expected: String?) -> Int? { + guard let expected else { return questionQueue.isEmpty ? nil : 0 } + return questionQueue.firstIndex { ($0.event.sessionId ?? "default") == expected } + } + + /// The request the card was showing is no longer queued (answered in the + /// terminal, drained on disconnect). `showNextPending()` drops the dead card + /// and re-opens whatever is genuinely waiting. (#308) + private func discardStalePanelAction(expected: String, kind: String) { + log.notice("⚠️ ignored \(kind, privacy: .public) for session=\(expected, privacy: .public) — request no longer queued") + showNextPending() + refreshDerivedState() + } + + func approvePermission(always: Bool = false, expectedSessionId: String? = nil) { + guard let index = permissionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "approve") + } + return + } + let pending = permissionQueue.remove(at: index) let sessionId = pending.event.sessionId ?? "default" dismissedPermissionSessionIds.remove(sessionId) let responseData: Data @@ -1520,9 +1621,14 @@ final class AppState { })?.key } - func denyPermission() { - guard !permissionQueue.isEmpty else { return } - let pending = permissionQueue.removeFirst() + func denyPermission(expectedSessionId: String? = nil) { + guard let index = permissionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "deny") + } + return + } + let pending = permissionQueue.remove(at: index) let sessionId = pending.event.sessionId ?? "default" dismissedPermissionSessionIds.remove(sessionId) let response = #"{"hookSpecificOutput":{"hookEventName":"PermissionRequest","decision":{"behavior":"deny"}}}"# @@ -1544,8 +1650,14 @@ final class AppState { refreshDerivedState() } - func dismissPermissionPrompt() { - guard let pending = permissionQueue.first else { return } + func dismissPermissionPrompt(expectedSessionId: String? = nil) { + guard let index = permissionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "dismiss") + } + return + } + let pending = permissionQueue[index] let sessionId = pending.event.sessionId ?? "default" dismissedPermissionSessionIds.insert(sessionId) @@ -1729,17 +1841,22 @@ final class AppState { refreshDerivedState() } - func answerQuestion(_ answer: String) { - guard !questionQueue.isEmpty else { return } + func answerQuestion(_ answer: String, expectedSessionId: String? = nil) { + guard let index = questionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "answer") + } + return + } // Multi-question wizards (AskUserQuestion, Codex app-server) use the batch // path — direct single answers are not processed. - if questionQueue[0].askUserQuestionState != nil, - (questionQueue[0].isFromPermission || questionQueue[0].isCodexAppServer) { + if questionQueue[index].askUserQuestionState != nil, + (questionQueue[index].isFromPermission || questionQueue[index].isCodexAppServer) { return } // Codex app-server questions reply over the JSON-RPC client, not a hook. - if questionQueue[0].isCodexAppServer { - let pending = questionQueue.removeFirst() + if questionQueue[index].isCodexAppServer { + let pending = questionQueue.remove(at: index) let answerKey = pending.askUserQuestionState?.items.first?.answerKey ?? pending.question.header ?? "answer" pending.resolveCodexAppServer([answerKey: [answer]]) @@ -1749,7 +1866,7 @@ final class AppState { refreshDerivedState() return } - let pending = questionQueue.removeFirst() + let pending = questionQueue.remove(at: index) let responseData: Data if pending.isFromPermission { let answerKey = pending.question.header ?? "answer" @@ -1792,11 +1909,19 @@ final class AppState { refreshDerivedState() } - func answerQuestionMulti(_ answers: [(question: String, answer: String)]) { - guard !questionQueue.isEmpty else { return } + func answerQuestionMulti( + _ answers: [(question: String, answer: String)], + expectedSessionId: String? = nil + ) { + guard let index = questionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "answer") + } + return + } // Codex app-server questions reply over the JSON-RPC client, not a hook. - if questionQueue[0].isCodexAppServer { - let pending = questionQueue.removeFirst() + if questionQueue[index].isCodexAppServer { + let pending = questionQueue.remove(at: index) var answersByKey: [String: [String]] = [:] if let askState = pending.askUserQuestionState { // Match by position — the wizard collects answers in item order. @@ -1814,7 +1939,7 @@ final class AppState { refreshDerivedState() return } - let pending = questionQueue.removeFirst() + let pending = questionQueue.remove(at: index) let responseData: Data if pending.isFromPermission { var answersDict: [String: String] = [:] @@ -1888,9 +2013,14 @@ final class AppState { return updatedInput } - func skipQuestion() { - guard !questionQueue.isEmpty else { return } - let pending = questionQueue.removeFirst() + func skipQuestion(expectedSessionId: String? = nil) { + guard let index = questionIndex(expecting: expectedSessionId) else { + if let expectedSessionId { + discardStalePanelAction(expected: expectedSessionId, kind: "skip") + } + return + } + let pending = questionQueue.remove(at: index) if pending.isCodexAppServer { // No "skip" verb in the Codex protocol — abandon the request so the // server stops waiting (it will re-prompt or fall back to its TUI). @@ -1927,6 +2057,12 @@ final class AppState { item.continuation.resume(returning: denyResponse) return true } + // Every drain path routes through here, so this is the one place that + // can guarantee a card is never left rendering a request that no longer + // exists. `handlePeerDisconnect` reaches showNextPending() on its own, + // but the question paths do not, and an .approvalCard whose session was + // just drained renders nothing at all — an expanded, empty island. + collapseStaleCardSurface() } /// Called when the bridge socket disconnects — the question/permission was answered externally (e.g. user replied in terminal) @@ -1967,9 +2103,32 @@ final class AppState { } } + /// A card the user can no longer act on must never stay on screen: the panel + /// would sit expanded showing a request that is gone or dismissed, and any + /// click landing on it can only be discarded. Auto-open suppression decides + /// whether to open a *new* card, not whether to keep a dead one, so this + /// runs unconditionally. (#308) + /// + /// "Dead" is the same predicate `nextVisiblePermissionIndex()` applies: + /// dismissed counts as not visible. Testing queue membership alone would + /// keep a dismissed card up, because dismissing hides without dequeuing. + private func collapseStaleCardSurface() { + switch surface { + case .approvalCard(let sid) + where pendingPermission(forSession: sid) == nil + || dismissedPermissionSessionIds.contains(sid): + surface = .collapsed + case .questionCard(let sid) where pendingQuestion(forSession: sid) == nil: + surface = .collapsed + default: + break + } + } + /// After dequeuing, show next pending item or collapse @discardableResult func showNextPending() -> Bool { + collapseStaleCardSurface() if let idx = nextVisiblePermissionIndex() { let next = permissionQueue.remove(at: idx) permissionQueue.insert(next, at: 0) diff --git a/Sources/CodeIsland/IslandSurface.swift b/Sources/CodeIsland/IslandSurface.swift index d813c7d6..1984a500 100644 --- a/Sources/CodeIsland/IslandSurface.swift +++ b/Sources/CodeIsland/IslandSurface.swift @@ -20,4 +20,17 @@ enum IslandSurface: Equatable { case .approvalCard(let id), .questionCard(let id), .completionCard(let id): return id } } + + /// Session of the surface only when it is the matching card kind. A + /// permission shortcut fired while a question card is up must not address + /// that session's (non-existent) approval and discard the live card. (#308) + var approvalSessionId: String? { + if case .approvalCard(let id) = self { return id } + return nil + } + + var questionSessionId: String? { + if case .questionCard(let id) = self { return id } + return nil + } } diff --git a/Sources/CodeIsland/NotchPanelView.swift b/Sources/CodeIsland/NotchPanelView.swift index a9c2983f..78e2a813 100644 --- a/Sources/CodeIsland/NotchPanelView.swift +++ b/Sources/CodeIsland/NotchPanelView.swift @@ -204,26 +204,28 @@ struct NotchPanelView: View { switch appState.surface { case .approvalCard(let sid): - if let pending = appState.pendingPermission { + // Card is addressed by session — render that session's + // request, not whatever is at the head of the queue. (#308) + if let pending = appState.pendingPermission(forSession: sid) { let session = appState.sessions[sid] ApprovalBar( tool: pending.event.toolName ?? "Unknown", toolInput: pending.event.toolInput, - queuePosition: 1, + queuePosition: appState.permissionQueuePosition(forSession: sid), queueTotal: appState.permissionQueue.count, session: session, sessionId: sid, appState: appState, - onAllow: { appState.approvePermission(always: false) }, - onAlwaysAllow: { appState.approvePermission(always: true) }, - onDeny: { appState.denyPermission() }, - onDismiss: { appState.dismissPermissionPrompt() } + onAllow: { appState.approvePermission(always: false, expectedSessionId: sid) }, + onAlwaysAllow: { appState.approvePermission(always: true, expectedSessionId: sid) }, + onDeny: { appState.denyPermission(expectedSessionId: sid) }, + onDismiss: { appState.dismissPermissionPrompt(expectedSessionId: sid) } ) .transition(.blurFade.combined(with: .scale(scale: 0.96, anchor: .top))) } case .questionCard(let sid): let session = appState.sessions[sid] - if let q = appState.pendingQuestion { + if let q = appState.pendingQuestion(forSession: sid) { QuestionBar( question: q.question.question, options: q.question.options, @@ -231,11 +233,11 @@ struct NotchPanelView: View { allQuestions: q.askUserQuestionState?.items ?? [], sessionSource: session?.source, sessionContext: session?.cwd, - queuePosition: 1, + queuePosition: appState.questionQueuePosition(forSession: sid), queueTotal: appState.questionQueue.count, - onAnswer: { appState.answerQuestion($0) }, - onAnswerMulti: { appState.answerQuestionMulti($0) }, - onSkip: { appState.skipQuestion() } + onAnswer: { appState.answerQuestion($0, expectedSessionId: sid) }, + onAnswerMulti: { appState.answerQuestionMulti($0, expectedSessionId: sid) }, + onSkip: { appState.skipQuestion(expectedSessionId: sid) } ) .transition(.blurFade.combined(with: .scale(scale: 0.96, anchor: .top))) } else if let preview = appState.previewQuestionPayload { @@ -2279,21 +2281,21 @@ private struct SessionCard: View { fg: .white, bg: Color(red: 0.25, green: 0.65, blue: 0.35), enabled: isActiveApproval, - action: { appState.approvePermission(always: false) } + action: { appState.approvePermission(always: false, expectedSessionId: sessionId) } ) inlineActionButton( L10n.shared["always"], fg: .white, bg: Color(red: 0.25, green: 0.55, blue: 0.85), enabled: isActiveApproval, - action: { appState.approvePermission(always: true) } + action: { appState.approvePermission(always: true, expectedSessionId: sessionId) } ) inlineActionButton( L10n.shared["deny"], fg: .white, bg: Color(red: 0.85, green: 0.3, blue: 0.3), enabled: isActiveApproval, - action: { appState.denyPermission() } + action: { appState.denyPermission(expectedSessionId: sessionId) } ) } diff --git a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift new file mode 100644 index 00000000..43f616af --- /dev/null +++ b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift @@ -0,0 +1,522 @@ +import XCTest +import AppKit +@testable import CodeIsland +import CodeIslandCore + +/// Answers must reach the session whose card the user acted on, not whatever +/// request happens to sit at the head of the queue. (#308) +@MainActor +final class AppStateAnswerRoutingTests: XCTestCase { + + // MARK: - Questions + + func testAnswerGoesToTheCardsSessionWhenAnotherSessionIsQueuedFirst() async throws { + let appState = AppState() + let first = try makeAskUserQuestionEvent(sessionId: "gitops-ansible", text: "Deploy which env?") + let second = try makeAskUserQuestionEvent(sessionId: "liverpool-cleanup", text: "Delete the branch?") + + let firstResponse = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(first, continuation: $0) } + } + await Task.yield() + let secondResponse = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(second, continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.questionQueue.count, 2) + + // The user is looking at the second session's card. + appState.answerQuestionMulti( + [(question: "Delete the branch?", answer: "Yes")], + expectedSessionId: "liverpool-cleanup" + ) + + // Assert on the queue BEFORE awaiting, and stop on failure: a routing + // regression resolves the wrong continuation, so the await below would + // hang forever and report as a CI timeout instead of a named failure. + guard assertQueue( + appState.questionQueue.map { $0.event.sessionId }, + ["gitops-ansible"], + "the addressed session must be the one dequeued, and the other must stay queued" + ) else { return } + + let answers = try extractAnswers(from: await secondResponse.value) + XCTAssertEqual(answers["Delete the branch?"] as? String, "Yes") + + firstResponse.cancel() + } + + func testAnswerIsDroppedWhenTheCardsRequestIsNoLongerQueued() async throws { + let appState = AppState() + let stale = try makeAskUserQuestionEvent(sessionId: "gitops-ansible", text: "Deploy which env?") + let other = try makeAskUserQuestionEvent(sessionId: "liverpool-cleanup", text: "Delete the branch?") + + let staleResponse = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(stale, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(other, continuation: $0) } + } + await Task.yield() + + // The first session answered in its own terminal and dropped its socket, + // which drains its queue entry and promotes the other session to head. + appState.handlePeerDisconnect(sessionId: "gitops-ansible") + _ = await staleResponse.value + XCTAssertEqual(appState.questionQueue.count, 1) + XCTAssertEqual(appState.questionQueue[0].event.sessionId, "liverpool-cleanup") + + // A click on the now-stale card must not answer the surviving session. + appState.surface = .questionCard(sessionId: "gitops-ansible") + appState.answerQuestionMulti( + [(question: "Deploy which env?", answer: "staging")], + expectedSessionId: "gitops-ansible" + ) + + XCTAssertEqual(appState.questionQueue.count, 1, "surviving session must still be waiting") + XCTAssertEqual(appState.questionQueue[0].event.sessionId, "liverpool-cleanup") + XCTAssertNotEqual( + appState.surface, + .questionCard(sessionId: "gitops-ansible"), + "a card with no queued request must not stay on screen — it would sit expanded and empty" + ) + } + + /// The case the empty-panel bug actually needs: another session is still + /// waiting (so the queue is not empty), but Smart Suppress declines to + /// auto-open its card. `showNextPending` used to leave the old card's + /// surface untouched, and with the card rendering only its own session's + /// request that means an expanded, blank notch. (#308) + func testStaleCardCollapsesWhenAutoOpenIsSuppressed() async throws { + UserDefaults.standard.set(true, forKey: SettingsKey.smartSuppress) + defer { UserDefaults.standard.removeObject(forKey: SettingsKey.smartSuppress) } + + let appState = AppState() + var suppressed = SessionSnapshot() + suppressed.termApp = "Ghostty" + suppressed.termBundleId = try XCTUnwrap(NSWorkspace.shared.frontmostApplication?.bundleIdentifier) + appState.sessions["s-other"] = suppressed + XCTAssertFalse( + appState.shouldAutoOpenPendingSurface(for: "s-other"), + "test setup must model Smart Suppress declining to auto-open this session" + ) + + let stale = try makePermissionRequestEvent(sessionId: "s-stale", command: "echo 1") + let other = try makePermissionRequestEvent(sessionId: "s-other", command: "echo 2") + + let staleResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(stale, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(other, continuation: $0) } + } + await Task.yield() + + appState.surface = .approvalCard(sessionId: "s-stale") + appState.handlePeerDisconnect(sessionId: "s-stale") + _ = await staleResponse.value + + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-other"]) + XCTAssertEqual( + appState.surface, + .collapsed, + "the drained card must not stay up — it has no request left to render" + ) + } + + func testStaleCardCollapsesWhenItsRequestIsDrained() async throws { + let appState = AppState() + let only = try makeAskUserQuestionEvent(sessionId: "s-only", text: "Proceed?") + + let response = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(only, continuation: $0) } + } + await Task.yield() + appState.surface = .questionCard(sessionId: "s-only") + + // Answered in the terminal instead: the socket drops and the entry drains. + appState.handlePeerDisconnect(sessionId: "s-only") + _ = await response.value + + XCTAssertEqual( + appState.surface, + .collapsed, + "nothing is queued, so the panel must collapse rather than render an empty card" + ) + } + + func testSkipTargetsTheCardsSession() async throws { + let appState = AppState() + let first = try makeAskUserQuestionEvent(sessionId: "s-first", text: "First?") + let second = try makeAskUserQuestionEvent(sessionId: "s-second", text: "Second?") + + _ = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(first, continuation: $0) } + } + await Task.yield() + let secondResponse = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(second, continuation: $0) } + } + await Task.yield() + + appState.skipQuestion(expectedSessionId: "s-second") + + guard assertQueue( + appState.questionQueue.map { $0.event.sessionId }, + ["s-first"], + "skip must dequeue the addressed session" + ) else { return } + let behavior = try extractPermissionBehavior(from: await secondResponse.value) + XCTAssertEqual(behavior, "deny") + } + + // MARK: - Permissions + + func testApproveGoesToTheCardsSessionWhenAnotherSessionIsQueuedFirst() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-first", command: "echo 1") + let second = try makePermissionRequestEvent(sessionId: "s-second", command: "echo 2") + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(first, continuation: $0) } + } + await Task.yield() + let secondResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(second, continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.permissionQueue.count, 2) + + appState.approvePermission(expectedSessionId: "s-second") + + // Queue first — see the note in the question-routing test above. + guard assertQueue( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-first"], + "approve must dequeue the addressed session" + ) else { return } + let response = await secondResponse.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + } + + func testDenyIsDroppedWhenTheCardsRequestIsNoLongerQueued() async throws { + let appState = AppState() + let stale = try makePermissionRequestEvent(sessionId: "s-stale", command: "echo 1") + let other = try makePermissionRequestEvent(sessionId: "s-other", command: "echo 2") + + let staleResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(stale, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(other, continuation: $0) } + } + await Task.yield() + + appState.handlePeerDisconnect(sessionId: "s-stale") + _ = await staleResponse.value + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-other"]) + + appState.surface = .approvalCard(sessionId: "s-stale") + appState.denyPermission(expectedSessionId: "s-stale") + + XCTAssertEqual( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-other"], + "the surviving session's approval must remain pending" + ) + XCTAssertNotEqual(appState.surface, .approvalCard(sessionId: "s-stale")) + } + + func testDismissTargetsTheCardsSession() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-first", command: "echo 1") + let second = try makePermissionRequestEvent(sessionId: "s-second", command: "echo 2") + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(first, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(second, continuation: $0) } + } + await Task.yield() + + appState.surface = .approvalCard(sessionId: "s-second") + appState.dismissPermissionPrompt(expectedSessionId: "s-second") + + // Dismissing hides that session's prompt and hands the panel to the + // session that is still visible. Had it dismissed the head instead, the + // panel would have swung to the session the user just dismissed. + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-first")) + XCTAssertEqual(appState.permissionQueue.count, 2, "dismiss hides, it must not resolve") + } + + // MARK: - Card rendering + + func testCardLookupReturnsTheAddressedSessionsRequest() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-first", command: "echo 1") + let second = try makePermissionRequestEvent(sessionId: "s-second", command: "echo 2") + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(first, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(second, continuation: $0) } + } + await Task.yield() + + XCTAssertEqual(appState.pendingPermission(forSession: "s-second")?.event.sessionId, "s-second") + XCTAssertNil(appState.pendingPermission(forSession: "s-missing")) + XCTAssertEqual(appState.permissionQueuePosition(forSession: "s-second"), 2) + } + + func testQuestionCardLookupReturnsTheAddressedSessionsRequest() async throws { + let appState = AppState() + let first = try makeAskUserQuestionEvent(sessionId: "s-first", text: "First?") + let second = try makeAskUserQuestionEvent(sessionId: "s-second", text: "Second?") + + _ = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(first, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handleAskUserQuestion(second, continuation: $0) } + } + await Task.yield() + + XCTAssertEqual(appState.pendingQuestion(forSession: "s-second")?.question.question, "Second?") + XCTAssertNil(appState.pendingQuestion(forSession: "s-missing")) + XCTAssertEqual(appState.questionQueuePosition(forSession: "s-second"), 2) + } + + // MARK: - Head-of-queue behaviour is preserved for surfaces that mirror it + + func testOmittedSessionStillResolvesTheHead() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-first", command: "echo 1") + let second = try makePermissionRequestEvent(sessionId: "s-second", command: "echo 2") + + let firstResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(first, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(second, continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.permissionQueue.count, 2, "a one-element queue could not detect a change here") + + // Buddy/companion surfaces mirror the head and pass no session. + appState.approvePermission() + + guard assertQueue( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-second"], + "with no session passed, the head must be the one resolved" + ) else { return } + let response = await firstResponse.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + } + + /// The single-answer path (`Notification` questions, not the AskUserQuestion + /// wizard) routes by session too. + func testSingleAnswerQuestionTargetsTheCardsSession() async throws { + let appState = AppState() + let first = try makeNotificationQuestionEvent(sessionId: "s-first", text: "First?") + let second = try makeNotificationQuestionEvent(sessionId: "s-second", text: "Second?") + + _ = Task { + await withCheckedContinuation { appState.handleQuestion(first, continuation: $0) } + } + await Task.yield() + let secondResponse = Task { + await withCheckedContinuation { appState.handleQuestion(second, continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.questionQueue.count, 2) + + appState.answerQuestion("B", expectedSessionId: "s-second") + + guard assertQueue( + appState.questionQueue.map { $0.event.sessionId }, + ["s-first"], + "the single-answer path must dequeue the addressed session" + ) else { return } + let responseData = await secondResponse.value + let json = try XCTUnwrap( + try JSONSerialization.jsonObject(with: responseData) as? [String: Any] + ) + let output = try XCTUnwrap(json["hookSpecificOutput"] as? [String: Any]) + XCTAssertEqual(output["answer"] as? String, "B") + } + + /// A dismissed approval is hidden but stays queued, so a liveness check based + /// on queue membership alone would keep its card on screen — re-rendering the + /// request the user just dismissed. + func testDismissedCardDoesNotStayOnScreenWhenAutoOpenIsSuppressed() async throws { + UserDefaults.standard.set(true, forKey: SettingsKey.smartSuppress) + defer { UserDefaults.standard.removeObject(forKey: SettingsKey.smartSuppress) } + + let appState = AppState() + var suppressed = SessionSnapshot() + suppressed.termApp = "Ghostty" + suppressed.termBundleId = try XCTUnwrap(NSWorkspace.shared.frontmostApplication?.bundleIdentifier) + appState.sessions["s-other"] = suppressed + XCTAssertFalse(appState.shouldAutoOpenPendingSurface(for: "s-other")) + + let dismissed = try makePermissionRequestEvent(sessionId: "s-dismissed", command: "echo 1") + let other = try makePermissionRequestEvent(sessionId: "s-other", command: "echo 2") + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(dismissed, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(other, continuation: $0) } + } + await Task.yield() + + appState.surface = .approvalCard(sessionId: "s-dismissed") + appState.dismissPermissionPrompt(expectedSessionId: "s-dismissed") + + XCTAssertNotEqual( + appState.surface, + .approvalCard(sessionId: "s-dismissed"), + "a dismissed card must not stay up just because its request is still queued" + ) + XCTAssertEqual(appState.permissionQueue.count, 2, "dismiss hides, it must not resolve") + } + + func testDismissIsDroppedWhenTheCardsRequestIsNoLongerQueued() async throws { + let appState = AppState() + let stale = try makePermissionRequestEvent(sessionId: "s-stale", command: "echo 1") + let other = try makePermissionRequestEvent(sessionId: "s-other", command: "echo 2") + + let staleResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(stale, continuation: $0) } + } + await Task.yield() + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(other, continuation: $0) } + } + await Task.yield() + + appState.handlePeerDisconnect(sessionId: "s-stale") + _ = await staleResponse.value + + appState.surface = .approvalCard(sessionId: "s-stale") + appState.dismissPermissionPrompt(expectedSessionId: "s-stale") + + XCTAssertEqual( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-other"], + "dismissing a card whose request is gone must not hide a different session's prompt" + ) + XCTAssertNotEqual(appState.surface, .approvalCard(sessionId: "s-stale")) + + // The queue assertions above cannot see the damage a head-based dismiss + // would do: dismissing hides without dequeuing, so the queue looks the + // same either way. Whether s-other was wrongly marked dismissed only + // shows up in whether the panel will still offer its card. + appState.showNextPending() + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-other"), + "s-other must remain offerable — a wrongly-dismissed session is filtered out and the panel stays collapsed" + ) + } + + // MARK: - Surface accessors + + func testSurfaceSessionAccessorsAreKindMatched() { + XCTAssertEqual(IslandSurface.approvalCard(sessionId: "a").approvalSessionId, "a") + XCTAssertNil(IslandSurface.questionCard(sessionId: "q").approvalSessionId) + XCTAssertEqual(IslandSurface.questionCard(sessionId: "q").questionSessionId, "q") + XCTAssertNil(IslandSurface.approvalCard(sessionId: "a").questionSessionId) + // A completion card is neither: a shortcut fired over it addresses nothing. + XCTAssertNil(IslandSurface.completionCard(sessionId: "c").approvalSessionId) + XCTAssertNil(IslandSurface.completionCard(sessionId: "c").questionSessionId) + } + + // MARK: - Helpers + + /// Assert the post-action queue, and report whether it held. Every await in + /// this suite only completes when the *right* continuation was resolved, so + /// a test that keeps going after this fails hangs instead of reporting. + private func assertQueue( + _ actual: [String?], + _ expected: [String], + _ message: String, + file: StaticString = #filePath, + line: UInt = #line + ) -> Bool { + let expectedOptionals = expected.map { Optional($0) } + XCTAssertEqual(actual, expectedOptionals, message, file: file, line: line) + return actual == expectedOptionals + } + + private func makeAskUserQuestionEvent(sessionId: String, text: String) throws -> HookEvent { + let payload: [String: Any] = [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": "AskUserQuestion", + "tool_input": [ + "questions": [[ + "question": text, + "header": "Pick", + "options": [["label": "Yes", "description": ""], ["label": "No", "description": ""]], + ]] + ], + ] + return try makeEvent(payload) + } + + private func makeNotificationQuestionEvent(sessionId: String, text: String) throws -> HookEvent { + let payload: [String: Any] = [ + "hook_event_name": "Notification", + "session_id": sessionId, + "question": text, + "options": ["A", "B"], + ] + return try makeEvent(payload) + } + + private func makePermissionRequestEvent(sessionId: String, command: String) throws -> HookEvent { + let payload: [String: Any] = [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": "Bash", + "tool_input": ["command": command, "description": command], + ] + return try makeEvent(payload) + } + + private func makeEvent(_ payload: [String: Any]) throws -> HookEvent { + let data = try JSONSerialization.data(withJSONObject: payload) + guard let event = HookEvent(from: data) else { + XCTFail("Failed to parse HookEvent") + throw NSError(domain: "AppStateAnswerRoutingTests", code: 1) + } + return event + } + + private func extractAnswers(from responseData: Data) throws -> [String: Any] { + let json = try XCTUnwrap(try JSONSerialization.jsonObject(with: responseData) as? [String: Any]) + let hookSpecificOutput = try XCTUnwrap(json["hookSpecificOutput"] as? [String: Any]) + let decision = try XCTUnwrap(hookSpecificOutput["decision"] as? [String: Any]) + let updatedInput = try XCTUnwrap(decision["updatedInput"] as? [String: Any]) + return try XCTUnwrap(updatedInput["answers"] as? [String: Any]) + } + + private func extractPermissionBehavior(from responseData: Data) throws -> String { + let json = try XCTUnwrap(try JSONSerialization.jsonObject(with: responseData) as? [String: Any]) + let hookSpecificOutput = try XCTUnwrap(json["hookSpecificOutput"] as? [String: Any]) + let decision = try XCTUnwrap(hookSpecificOutput["decision"] as? [String: Any]) + return try XCTUnwrap(decision["behavior"] as? String) + } +} diff --git a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift index 78cd25b3..040f19f8 100644 --- a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift +++ b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift @@ -74,6 +74,70 @@ final class AppStateCodexRequestUserInputTests: XCTestCase { XCTAssertEqual(appState.questionQueue.count, 0) } + /// The Codex branches used to be reachable only at the head of the queue. + /// Session-addressed answers can land on any index, so a Codex request + /// queued behind another session's question must still reply over the + /// JSON-RPC path rather than the hook path. (#308) + func testCodexQuestionIsAnsweredWhileQueuedBehindAnotherSession() async throws { + let appState = AppState() + + let hookPayload: [String: Any] = [ + "hook_event_name": "Notification", + "session_id": "s-hook", + "question": "First?", + "options": ["A", "B"], + ] + let hookEvent = try XCTUnwrap( + HookEvent(from: try JSONSerialization.data(withJSONObject: hookPayload)) + ) + _ = Task { + await withCheckedContinuation { appState.handleQuestion(hookEvent, continuation: $0) } + } + await Task.yield() + + // Enqueued with a capturing reply closure rather than through the live + // client: dequeuing is not the half that was at risk. A head-anchored + // Codex check sends the answer down the hook path while the indexed + // remove still dequeues it — the queue looks identical and the Codex + // server waits forever. Only invoking this closure proves which path ran. + var repliedAnswers: [String: [String]]? + var replyCalled = false + let codexEvent = try XCTUnwrap( + HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "Notification", + "session_id": "codexapp:t-behind", + ])) + ) + let payload = QuestionPayload(question: "Pick", options: ["A"], header: "Plan") + appState.questionQueue.append(QuestionRequest( + event: codexEvent, + question: payload, + resolution: .codexAppServer { answers in + replyCalled = true + repliedAnswers = answers + }, + askUserQuestionState: AskUserQuestionState( + items: [AskUserQuestionItem(payload: payload, answerKey: "q1", multiSelect: false)], + answers: [:] + ) + )) + XCTAssertEqual(appState.questionQueue.count, 2) + XCTAssertTrue(appState.questionQueue[1].isCodexAppServer) + + appState.answerQuestionMulti( + [(question: "Pick", answer: "A")], + expectedSessionId: "codexapp:t-behind" + ) + + XCTAssertEqual( + appState.questionQueue.map { $0.event.sessionId }, + ["s-hook"], + "the Codex request must be the one dequeued, and the hook question must stay queued" + ) + XCTAssertTrue(replyCalled, "the reply must go out over the Codex JSON-RPC path, not the hook path") + XCTAssertEqual(repliedAnswers?["q1"], ["A"]) + } + func testServerRequestResolvedDropsQueuedQuestion() { let appState = AppState() let message = makeRequest(threadId: "t-resolve", questions: [[ diff --git a/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift new file mode 100644 index 00000000..8d263991 --- /dev/null +++ b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift @@ -0,0 +1,250 @@ +import XCTest +@testable import CodeIsland +import CodeIslandCore + +/// Where the #308 answer-routing fix and the #309 enqueue-gate fix meet. +/// +/// Each branch's own suite exercises one side: #308 checks that an action +/// resolves the card's session, #309 checks that a request still raises a card. +/// Neither covers the state where both matter at once — a card left pointing at +/// a session whose request was drained while OTHER sessions still have queued +/// requests. +@MainActor +final class AppStateIntegrationRoutingTests: XCTestCase { + + /// Staleness is per-session, so the enqueue gate must be too. A whole-queue + /// test reads "a card is on screen" as true whenever any session has a + /// queued request, which suppresses the stale-card collapse inside + /// showNextPending() and wedges the panel behind a card that renders + /// nothing — swallowing every later request. + func testDrainedCardWithOtherSessionsQueuedDoesNotWedgeThePanel() async throws { + let appState = AppState() + + // A question from another session occupies the question queue, so the + // question that arrives in step 3 will 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, so the + // queue is still non-empty while s-a's card has nothing behind it. + let aQuestionTask = Task { + await withCheckedContinuation { appState.handleQuestion(try! self.question("s-a"), continuation: $0) } + } + await Task.yield() + _ = await aTask.value + XCTAssertNil(appState.pendingPermission(forSession: "s-a"), "s-a's request is gone") + XCTAssertFalse(appState.permissionQueue.isEmpty, "and another session is still queued behind it") + XCTAssertNotEqual( + appState.surface, + .approvalCard(sessionId: "s-a"), + "the drain must not leave the island expanded on a card that renders nothing" + ) + + let dTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } + } + await Task.yield() + + XCTAssertNotEqual( + appState.surface, + .approvalCard(sessionId: "s-a"), + "the dead card must not survive a later request arriving" + ) + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-b"), + "the panel must move to the queued request that is actually waiting" + ) + + // Resolve the remaining waiters: an unresumed CheckedContinuation is a + // runtime misuse warning, and the noise hides real ones. + 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 + } + + /// The routing half, exercised on the card the gate just raised: acting on + /// it must resolve that session, not whatever leads the queue. + func testCardRaisedAfterADrainResolvesItsOwnSession() async throws { + let appState = AppState() + + let bTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-b", "Read"), continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-b") + XCTAssertEqual(appState.surface, .collapsed) + + // A different session's request arrives while s-b sits dismissed. + let dTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-d"), "#309: the later session still gets a card") + + // Raising the card promoted s-d to the head, so the card and the head + // agree here. To exercise routing the two must differ — which is the + // session list's inline approval: the user expands the panel and acts on + // the dismissed session while a different one leads the queue. + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-d", "s-b"]) + appState.surface = .sessionList + appState.approvePermission(expectedSessionId: "s-b") + + XCTAssertEqual( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-d"], + "the acted-on session must be resolved, not the queue head" + ) + let bResponse = await bTask.value + XCTAssertEqual(try extractBehavior(from: bResponse), "allow") + + appState.approvePermission(expectedSessionId: "s-d") + let dResponse = await dTask.value + XCTAssertEqual(try extractBehavior(from: dResponse), "allow") + } + + /// A replay of the same `tool_use_id` is the same decision arriving twice, + /// not a new one. Clearing the session's dismissal on a replay resurrects + /// the request the user hid, which then takes the card the arriving session + /// should have got — and, because it now counts as a burst already in + /// progress, silences that session's sound too. + func testReplayOfADismissedRequestDoesNotStealTheNextSessionsCard() async throws { + let appState = AppState() + let original = try permWithToolUse("s-replay", "Bash", "tool-1") + + let originalTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(original, continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-replay") + XCTAssertEqual(appState.surface, .collapsed) + + // The bridge replays the same tool_use_id for the dismissed session. + let replayTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-replay", "Bash", "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 card the user dismissed") + + 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" + ) + + appState.approvePermission(expectedSessionId: "s-other") + let otherResponse = await otherTask.value + XCTAssertEqual(try extractBehavior(from: otherResponse), "allow") + + appState.handlePeerDisconnect(sessionId: "s-replay") + _ = await replayTask.value + } + + /// The other side of the moved un-dismiss: `mergeDuplicatePermissionRequest` + /// returns false when the tool inputs differ (#169 — parallel tool calls can + /// share an id), so that request DOES enqueue and must still clear the + /// dismissal. Moving the un-dismiss must not strand a session as dismissed. + func testSameToolUseIdWithDifferentInputStillClearsTheDismissal() async throws { + let appState = AppState() + + let firstTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-parallel", "Read", "tool-9"), continuation: $0) + } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-parallel") + XCTAssertEqual(appState.surface, .collapsed) + + // Same tool_use_id, different input: a distinct request, not a replay. + let secondTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest( + try! self.permWithToolUse("s-parallel", "Read", "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 permWithToolUse( + _ sessionId: String, + _ toolName: 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": toolName, + "tool_use_id": toolUseId, + "tool_input": ["command": command], + ]))) + } + + 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 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) + } +} diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 3ab9c10d..b87b0e4d 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -532,6 +532,208 @@ 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) + // Routed by session (#308) rather than by queue position. Both of A's + // requests are at the front here, so this does not by itself prove the + // routing picked the right index — the session-list case in + // AppStateIntegrationRoutingTests is what covers that. It does keep this + // test honest about which session it means to approve. + appState.approvePermission(expectedSessionId: "s-a") + appState.approvePermission(expectedSessionId: "s-a") + _ = 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,