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..54b0017d 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 { @@ -1325,9 +1346,44 @@ final class AppState { 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 +1576,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 +1605,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 +1796,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 +1821,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 +1864,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 +1894,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 +1968,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). @@ -1967,9 +2052,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: [[