From 3f20530060f9abe56fffe5eb7bbc39697dc30874 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 08:58:19 -0500 Subject: [PATCH 1/4] fix(panel): route answers to the card's session, not the queue head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approving, denying, answering, or skipping from the notch resolved `queue.removeFirst()`, while the card itself is addressed by session id. Anything that mutated the head under an open card — a peer disconnect draining another session, a stale tool-use eviction, the reorder inside showNextPending() — delivered the answer to whichever request happened to be first, i.e. a different CLI than the one shown. With two sessions waiting this silently answered the wrong project. The answer paths now take the acting card's session and resolve that session's queued request; when it is gone (answered in the terminal, drained on disconnect) the action is discarded and the panel resyncs instead of falling through to the head. Cards also render the addressed session's request rather than the head, so what is shown and what is resolved cannot disagree. Head-of-queue behaviour is unchanged for the surfaces that only ever mirror the head (iPhone/Watch Buddy, Codex/companion paths). Upstream issue: wxtsky/CodeIsland#308 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppDelegate.swift | 10 +- Sources/CodeIsland/AppState.swift | 118 +++++++-- Sources/CodeIsland/NotchPanelView.swift | 26 +- .../AppStateAnswerRoutingTests.swift | 234 ++++++++++++++++++ 4 files changed, 349 insertions(+), 39 deletions(-) create mode 100644 Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift diff --git a/Sources/CodeIsland/AppDelegate.swift b/Sources/CodeIsland/AppDelegate.swift index 15b13d63..b5aceb5c 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.sessionId) case .approveAlways: - appState.approvePermission(always: true) + appState.approvePermission(always: true, expectedSessionId: appState.surface.sessionId) case .deny: - appState.denyPermission() + appState.denyPermission(expectedSessionId: appState.surface.sessionId) case .skipQuestion: - appState.skipQuestion() + appState.skipQuestion(expectedSessionId: appState.surface.sessionId) 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..a2ff715d 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -125,6 +125,17 @@ 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 } + } /// Preview-only: mock question payload for DebugHarness (no continuation needed) var previewQuestionPayload: QuestionPayload? var surface: IslandSurface = .collapsed { @@ -1325,9 +1336,46 @@ 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). Collapse first so a dead card can't + /// stay on screen, then let `showNextPending()` re-open 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") + surface = .collapsed + 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 +1568,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 +1597,9 @@ 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 { return } + let pending = permissionQueue[index] let sessionId = pending.event.sessionId ?? "default" dismissedPermissionSessionIds.insert(sessionId) @@ -1729,17 +1783,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 +1808,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 +1851,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 +1881,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 +1955,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). diff --git a/Sources/CodeIsland/NotchPanelView.swift b/Sources/CodeIsland/NotchPanelView.swift index a9c2983f..f41445d3 100644 --- a/Sources/CodeIsland/NotchPanelView.swift +++ b/Sources/CodeIsland/NotchPanelView.swift @@ -204,7 +204,9 @@ 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", @@ -214,16 +216,16 @@ struct NotchPanelView: View { 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, @@ -233,9 +235,9 @@ struct NotchPanelView: View { sessionContext: session?.cwd, queuePosition: 1, 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..96c6e33a --- /dev/null +++ b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift @@ -0,0 +1,234 @@ +import XCTest +@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" + ) + + let answers = try extractAnswers(from: await secondResponse.value) + XCTAssertEqual(answers["Delete the branch?"] as? String, "Yes") + + XCTAssertEqual(appState.questionQueue.count, 1, "the untouched session must stay queued") + XCTAssertEqual(appState.questionQueue[0].event.sessionId, "gitops-ansible") + XCTAssertFalse(firstResponse.isCancelled) + 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.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") + } + + 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") + + let behavior = try extractPermissionBehavior(from: await secondResponse.value) + XCTAssertEqual(behavior, "deny") + XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) + } + + // 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") + + let response = await secondResponse.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-first"]) + } + + 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.denyPermission(expectedSessionId: "s-stale") + + XCTAssertEqual( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-other"], + "the surviving session's approval must remain pending" + ) + } + + // 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")) + } + + // 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 firstResponse = Task { + await withCheckedContinuation { appState.handlePermissionRequest(first, continuation: $0) } + } + await Task.yield() + + appState.approvePermission() + + let response = await firstResponse.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + XCTAssertTrue(appState.permissionQueue.isEmpty) + } + + // MARK: - Helpers + + 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 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) + } +} From eb5ef930923ded019336572e5f4b4ff50285a321 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 09:17:09 -0500 Subject: [PATCH 2/4] fix: address QA round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 confirmed one major and eight minor findings against the #308 fix. - major: a card whose request had been drained left the notch expanded and empty. Cards now render only their own session's request, so the previous "wrong content" case became "no content" whenever showNextPending() declined to reassign the surface (Smart Suppress). showNextPending() now collapses a card surface whose session has nothing queued, before deciding what to open — one guard covering every drain path, not just the answer paths. - keyboard shortcuts passed surface.sessionId for any card kind, so a permission shortcut fired over a question card addressed a non-existent approval and discarded the live card. They now use kind-matched accessors. - dismissPermissionPrompt got the stale-discard treatment the other actions had. - the cards' "N of M" position was hardcoded to 1 while they may render a non-head request. Tests: the stale-card case is now pinned by the suppression path that actually reproduces it (verified red without the guard); routing tests assert queue state before awaiting so a regression fails by name instead of hanging; the head-of-queue test uses a two-element queue so it can fail; added coverage for the single-answer path, dismiss routing, and the session-scoped lookups. 699 tests, 0 failures. Not fixed, deliberately: within-session render-vs-resolve keying (hypothetical, mechanism unreproduced) and pinning the view-layer call-site wiring, which needs a SwiftUI view-test harness this project does not have. Both noted on the PR. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppDelegate.swift | 8 +- Sources/CodeIsland/AppState.swift | 34 +++- Sources/CodeIsland/IslandSurface.swift | 13 ++ Sources/CodeIsland/NotchPanelView.swift | 4 +- .../AppStateAnswerRoutingTests.swift | 181 +++++++++++++++++- 5 files changed, 226 insertions(+), 14 deletions(-) diff --git a/Sources/CodeIsland/AppDelegate.swift b/Sources/CodeIsland/AppDelegate.swift index b5aceb5c..56e03851 100644 --- a/Sources/CodeIsland/AppDelegate.swift +++ b/Sources/CodeIsland/AppDelegate.swift @@ -234,13 +234,13 @@ 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(expectedSessionId: appState.surface.sessionId) + appState.approvePermission(expectedSessionId: appState.surface.approvalSessionId) case .approveAlways: - appState.approvePermission(always: true, expectedSessionId: appState.surface.sessionId) + appState.approvePermission(always: true, expectedSessionId: appState.surface.approvalSessionId) case .deny: - appState.denyPermission(expectedSessionId: appState.surface.sessionId) + appState.denyPermission(expectedSessionId: appState.surface.approvalSessionId) case .skipQuestion: - appState.skipQuestion(expectedSessionId: appState.surface.sessionId) + 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 a2ff715d..a408111f 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -136,6 +136,16 @@ final class AppState { 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 { @@ -1363,7 +1373,6 @@ final class AppState { /// 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") - surface = .collapsed showNextPending() refreshDerivedState() } @@ -1598,7 +1607,12 @@ final class AppState { } func dismissPermissionPrompt(expectedSessionId: String? = nil) { - guard let index = permissionIndex(expecting: expectedSessionId) else { return } + 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" @@ -2039,9 +2053,25 @@ final class AppState { } } + /// A card whose request is no longer queued must never stay on screen: the + /// panel would sit expanded and empty, and the click that lands 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) + private func collapseStaleCardSurface() { + switch surface { + case .approvalCard(let sid) where pendingPermission(forSession: sid) == nil: + 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 f41445d3..78e2a813 100644 --- a/Sources/CodeIsland/NotchPanelView.swift +++ b/Sources/CodeIsland/NotchPanelView.swift @@ -211,7 +211,7 @@ struct NotchPanelView: View { 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, @@ -233,7 +233,7 @@ 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, expectedSessionId: sid) }, onAnswerMulti: { appState.answerQuestionMulti($0, expectedSessionId: sid) }, diff --git a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift index 96c6e33a..cfdb0567 100644 --- a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift +++ b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift @@ -1,4 +1,5 @@ import XCTest +import AppKit @testable import CodeIsland import CodeIslandCore @@ -30,12 +31,18 @@ final class AppStateAnswerRoutingTests: XCTestCase { expectedSessionId: "liverpool-cleanup" ) + // Assert on the queue BEFORE awaiting: a routing regression resolves the + // wrong continuation, which would leave the await below hanging forever. + // A hung test reports as a CI timeout instead of a named failure. + XCTAssertEqual( + appState.questionQueue.map { $0.event.sessionId }, + ["gitops-ansible"], + "the addressed session must be the one dequeued, and the other must stay queued" + ) + let answers = try extractAnswers(from: await secondResponse.value) XCTAssertEqual(answers["Delete the branch?"] as? String, "Yes") - XCTAssertEqual(appState.questionQueue.count, 1, "the untouched session must stay queued") - XCTAssertEqual(appState.questionQueue[0].event.sessionId, "gitops-ansible") - XCTAssertFalse(firstResponse.isCancelled) firstResponse.cancel() } @@ -61,6 +68,7 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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" @@ -68,6 +76,75 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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 { @@ -86,9 +163,9 @@ final class AppStateAnswerRoutingTests: XCTestCase { appState.skipQuestion(expectedSessionId: "s-second") + XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) let behavior = try extractPermissionBehavior(from: await secondResponse.value) XCTAssertEqual(behavior, "deny") - XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) } // MARK: - Permissions @@ -110,9 +187,10 @@ final class AppStateAnswerRoutingTests: XCTestCase { appState.approvePermission(expectedSessionId: "s-second") + // Queue first — see the note in the question-routing test above. + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-first"]) let response = await secondResponse.value XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") - XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-first"]) } func testDenyIsDroppedWhenTheCardsRequestIsNoLongerQueued() async throws { @@ -133,6 +211,7 @@ final class AppStateAnswerRoutingTests: XCTestCase { _ = await staleResponse.value XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-other"]) + appState.surface = .approvalCard(sessionId: "s-stale") appState.denyPermission(expectedSessionId: "s-stale") XCTAssertEqual( @@ -140,6 +219,31 @@ final class AppStateAnswerRoutingTests: XCTestCase { ["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 @@ -160,6 +264,26 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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 @@ -167,17 +291,52 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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() + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-second"]) let response = await firstResponse.value XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") - XCTAssertTrue(appState.permissionQueue.isEmpty) + } + + /// 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") + + XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) + 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") } // MARK: - Helpers @@ -198,6 +357,16 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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", From c65b9d9e9d06c058886887ad84abd27f15da69b2 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 09:34:11 -0500 Subject: [PATCH 3/4] fix: address QA round 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 confirmed one major and five minor findings; three of the six sat on code round 1 wrote. - major (introduced by eb5ef93): collapseStaleCardSurface tested card liveness by queue membership, but dismissPermissionPrompt hides a request without dequeuing it. Under Smart Suppress a dismissed approval card therefore stayed on screen, re-rendering the request the user had just dismissed. The guard now applies the same predicate nextVisiblePermissionIndex() uses: dismissed counts as not visible. No request was ever misrouted by this. - the routing tests failed by name but then deadlocked on the await that follows; each pre-await assertion now stops the test instead. - added coverage for the kind-matched surface accessors, both dismiss paths (routing and stale-discard), and a Codex app-server request answered while queued behind another session — those branches were only ever exercised at index 0. - corrected a doc comment describing a collapse that moved to another function. Both new guards were verified to fail without their fix before committing. 703 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 22 +-- .../AppStateAnswerRoutingTests.swift | 126 ++++++++++++++++-- .../AppStateCodexRequestUserInputTests.swift | 39 ++++++ 3 files changed, 170 insertions(+), 17 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index a408111f..54b0017d 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1368,9 +1368,8 @@ final class AppState { } /// The request the card was showing is no longer queued (answered in the - /// terminal, drained on disconnect). Collapse first so a dead card can't - /// stay on screen, then let `showNextPending()` re-open whatever is - /// genuinely waiting. (#308) + /// 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() @@ -2053,13 +2052,20 @@ final class AppState { } } - /// A card whose request is no longer queued must never stay on screen: the - /// panel would sit expanded and empty, and the click that lands 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) + /// 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: + 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 diff --git a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift index cfdb0567..a4548d05 100644 --- a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift +++ b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift @@ -31,14 +31,14 @@ final class AppStateAnswerRoutingTests: XCTestCase { expectedSessionId: "liverpool-cleanup" ) - // Assert on the queue BEFORE awaiting: a routing regression resolves the - // wrong continuation, which would leave the await below hanging forever. - // A hung test reports as a CI timeout instead of a named failure. - XCTAssertEqual( + // 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") @@ -163,7 +163,11 @@ final class AppStateAnswerRoutingTests: XCTestCase { appState.skipQuestion(expectedSessionId: "s-second") - XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) + 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") } @@ -188,7 +192,11 @@ final class AppStateAnswerRoutingTests: XCTestCase { appState.approvePermission(expectedSessionId: "s-second") // Queue first — see the note in the question-routing test above. - XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-first"]) + 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") } @@ -306,7 +314,11 @@ final class AppStateAnswerRoutingTests: XCTestCase { // Buddy/companion surfaces mirror the head and pass no session. appState.approvePermission() - XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-second"]) + 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") } @@ -330,7 +342,11 @@ final class AppStateAnswerRoutingTests: XCTestCase { appState.answerQuestion("B", expectedSessionId: "s-second") - XCTAssertEqual(appState.questionQueue.map { $0.event.sessionId }, ["s-first"]) + 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] @@ -339,8 +355,100 @@ final class AppStateAnswerRoutingTests: XCTestCase { 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")) + } + + // 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", diff --git a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift index 78cd25b3..f9e75077 100644 --- a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift +++ b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift @@ -74,6 +74,45 @@ 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() + + appState.handleCodexAppServerMessage(makeRequest(threadId: "t-behind", questions: [[ + "id": "q1", "question": "Pick", "options": [["label": "A", "description": ""]], + ]])) + 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" + ) + } + func testServerRequestResolvedDropsQueuedQuestion() { let appState = AppState() let message = makeRequest(threadId: "t-resolve", questions: [[ From f40cee5a8b82e0e269617bfd0c27953940c9ed5f Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 09:50:55 -0500 Subject: [PATCH 4/4] test: strengthen the two assertions QA round 3 found weak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 found no blocking defects. Both findings were assertion-strength gaps in tests round 2 added — each passed whether or not the behaviour it named still held. - the stale-dismiss test observed only the queue, which is unchanged by a head-based dismiss (dismiss hides without dequeuing). It now drives showNextPending() and asserts the other session's card is still offerable, which is where a wrongly-dismissed session actually shows up. - the Codex-behind-another-session test asserted the dequeue, which a head-anchored Codex check would also satisfy while replying down the hook path and leaving the server waiting forever. The request now carries a capturing reply closure, so the test asserts the JSON-RPC path was the one taken. The session status it previously asserted could not discriminate: both paths land on .processing. Both were verified by mutating the production path each one names and watching it fail. 703 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- .../AppStateAnswerRoutingTests.swift | 11 +++++++ .../AppStateCodexRequestUserInputTests.swift | 31 +++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift index a4548d05..43f616af 100644 --- a/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift +++ b/Tests/CodeIslandTests/AppStateAnswerRoutingTests.swift @@ -418,6 +418,17 @@ final class AppStateAnswerRoutingTests: XCTestCase { "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 diff --git a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift index f9e75077..040f19f8 100644 --- a/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift +++ b/Tests/CodeIslandTests/AppStateCodexRequestUserInputTests.swift @@ -95,9 +95,32 @@ final class AppStateCodexRequestUserInputTests: XCTestCase { } await Task.yield() - appState.handleCodexAppServerMessage(makeRequest(threadId: "t-behind", questions: [[ - "id": "q1", "question": "Pick", "options": [["label": "A", "description": ""]], - ]])) + // 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) @@ -111,6 +134,8 @@ final class AppStateCodexRequestUserInputTests: XCTestCase { ["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() {