From 3f20530060f9abe56fffe5eb7bbc39697dc30874 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 08:58:19 -0500 Subject: [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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() { From e984ad6fdd6eef842770b50b8fbf5c824226b8fa Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:17:28 -0500 Subject: [PATCH 05/10] fix(panel): don't let a dismissed approval silence later requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dismissing an approval hides it but deliberately leaves it queued, so the CLI stays blocked and the prompt stays recoverable. The card-and-sound trigger in handlePermissionRequest was gated on `permissionQueue.count == 1`, which a dismissed entry keeps false forever — so every later permission request, from every session, arrived with no card and no sound until the dismissed one was resolved some other way. Queue size was standing in for "is a card already showing", and dismissal is exactly the state where those two stop agreeing. The gate now asks the same predicate the display path uses, nextVisiblePermissionIndex(), which already skips dismissed sessions. Showing the card via showNextPending() rather than pointing it at this session by hand matters: with a dismissed entry still leading the queue, the approval card renders the head, so a hand-set surface would show the dismissed request's content under the new session's name. The dismissed session's own next request stays hidden — dismissal is per-session and clears when that session's request resolves. That behaviour is unchanged and now pinned by a test so this fix isn't read as altering it. Fixes #309. 689 tests, 0 failures; the new regression test was confirmed to fail against the old gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 21 +++-- .../AppStatePermissionFlowTests.swift | 84 +++++++++++++++++++ 2 files changed, 97 insertions(+), 8 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index d3715c7f..ec194af8 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1310,16 +1310,21 @@ final class AppState { return } + // Queue size is not "is a card showing": dismissing hides a request but + // deliberately leaves it queued, so the CLI stays blocked and the prompt + // stays recoverable. Gating on `count == 1` therefore swallowed every + // later request — from any session — for as long as a dismissed one sat + // in the queue. Ask the same predicate the display path uses. (#309) + let wasShowingPermission = nextVisiblePermissionIndex() != nil permissionQueue.append(request) - // Show UI only if this is the first (or only) queued item - if permissionQueue.count == 1 { - activeSessionId = sessionId - // If user is already browsing the session list, keep them there and - // let inline controls handle approval without stealing focus. - if surface != .sessionList, shouldAutoOpenPendingSurface(for: sessionId) { - surface = .approvalCard(sessionId: sessionId) - } + // Show UI only when nothing was already on screen to be stolen from. + if !wasShowingPermission, nextVisiblePermissionIndex() != nil { + // showNextPending picks the first *visible* request, promotes it to + // the head and applies the session-list / Smart Suppress rules — + // pointing the card at this session by hand would show the dismissed + // request's content whenever a dismissed entry still leads the queue. + showNextPending() SoundManager.shared.handleEvent("PermissionRequest") } refreshDerivedState() diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 3ab9c10d..1b0163c7 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -532,6 +532,90 @@ final class AppStatePermissionFlowTests: XCTestCase { try String(contentsOf: codeIslandRulesPath(in: codexHome), encoding: .utf8) } + /// #309 — a dismissed request stays queued so the CLI stays blocked, which + /// used to make `permissionQueue.count == 1` false forever and swallow every + /// later request, from every session, with no card and no sound. + func testDismissedPermissionDoesNotSilenceALaterSessionsRequest() async throws { + let appState = AppState() + let dismissed = try makePermissionRequestEvent(sessionId: "s-dismissed", toolName: "Bash") + let later = try makePermissionRequestEvent(sessionId: "s-later", toolName: "Edit") + + let dismissedTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(dismissed, continuation: continuation) + } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-dismissed")) + + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual(appState.permissionQueue.count, 1, "dismiss must keep the request queued") + + let laterTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(later, continuation: continuation) + } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-later"), + "a different session's approval must still raise a card while a dismissed one sits in the queue" + ) + // Stop here on failure: under the bug, approvePermission() below resolves + // the dismissed request instead, so `await laterTask.value` would hang + // and the test would report as a timeout rather than by name. + guard appState.surface == .approvalCard(sessionId: "s-later") else { + appState.handlePeerDisconnect(sessionId: "s-dismissed") + appState.handlePeerDisconnect(sessionId: "s-later") + _ = await dismissedTask.value + _ = await laterTask.value + return + } + + appState.approvePermission() + let laterResponse = await laterTask.value + XCTAssertEqual(try extractPermissionBehavior(from: laterResponse), "allow") + + await assertTaskNotResolved(dismissedTask) + appState.handlePeerDisconnect(sessionId: "s-dismissed") + _ = await dismissedTask.value + } + + /// The dismissed session's own next request stays hidden — dismissal is + /// per-session and is only cleared when that session's request resolves. + /// Pinned so the #309 fix is not read as changing it. + func testDismissedSessionsOwnNextRequestStaysHidden() async throws { + let appState = AppState() + let first = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Bash") + let second = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Edit") + + let firstTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(first, continuation: continuation) + } + } + await Task.yield() + appState.dismissPermissionPrompt() + XCTAssertEqual(appState.surface, .collapsed) + + let secondTask = Task { + await withCheckedContinuation { continuation in + appState.handlePermissionRequest(second, continuation: continuation) + } + } + await Task.yield() + + XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual(appState.permissionQueue.count, 2) + + appState.handlePeerDisconnect(sessionId: "s-same") + _ = await firstTask.value + _ = await secondTask.value + } + private func makePermissionRequestEvent( sessionId: String, toolName: String, From 29e1456064204251fe0f4b9bc57793845d4124fb Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:38:48 -0500 Subject: [PATCH 06/10] =?UTF-8?q?fix:=20address=20QA=20round=201=20?= =?UTF-8?q?=E2=80=94=20gate=20on=20the=20surface,=20not=20the=20queue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA found the first attempt incomplete. `nextVisiblePermissionIndex() != nil` is "a non-dismissed request is queued", which is still not "a card is on screen": handlePermissionRequest un-dismisses the session on entry ("session needs user decision again"), so a dismissed session's NEXT request makes its own still-queued earlier request count as visible while nothing is displayed. The silencing then resumed exactly as #309 described, one step later. The gate now asks the surface directly. That also settles what the previous commit's pinning test got backwards: a dismissal is cleared by the session's next request arriving, not by the dismissed request resolving, so that session's next request must bring its card back. The old test asserted the broken state was correct and its doc comment stated an invariant the code does not have; both are corrected. Also strengthened, per QA: - the cross-session regression test's anti-hang guard checked only the surface; an implementation that points the card at the arriving session by hand passes that while the dismissed request still leads the queue, so approve resolves the wrong one and the await hangs. It now checks the queue head too. - the new "asking again doesn't silence others" test passed under the broken gate as originally written (resolving A's requests surfaces B either way). It now asserts a card is on screen at the moment B arrives, which is the state that actually differs. Known limitation, unchanged from main and noted in the code: a card suppressed by Smart Suppress also leaves a visible request undisplayed, so a second session's request waits behind it. Closing that needs showNextPending to skip un-openable entries; tracked separately. 690 tests, 0 failures; both new guards verified red against the incomplete gate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 27 ++++-- .../AppStatePermissionFlowTests.swift | 85 +++++++++++++++++-- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index ec194af8..a9bf76b1 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1310,16 +1310,27 @@ final class AppState { return } - // Queue size is not "is a card showing": dismissing hides a request but - // deliberately leaves it queued, so the CLI stays blocked and the prompt - // stays recoverable. Gating on `count == 1` therefore swallowed every - // later request — from any session — for as long as a dismissed one sat - // in the queue. Ask the same predicate the display path uses. (#309) - let wasShowingPermission = nextVisiblePermissionIndex() != nil + // Dismissing hides a request but deliberately leaves it queued, so the + // CLI stays blocked and the prompt stays recoverable. Gating on + // `permissionQueue.count == 1` therefore swallowed every later request — + // from any session — for as long as a dismissed one sat in the queue. + // + // The gate's real question is "is an approval card on screen", so ask + // the surface. Queue-derived proxies do not survive the un-dismiss + // above: a session's own next request clears its dismissal, which makes + // its still-queued earlier request count as visible again while nothing + // is displayed — silencing every later request all over again. (#309) + // + // ponytail: a card suppressed by Smart Suppress also leaves a visible + // request undisplayed, so a second session's request still waits behind + // it. That is pre-existing (`main` behaves the same) and needs + // showNextPending to skip un-openable entries; tracked separately. + let approvalCardOnScreen: Bool + if case .approvalCard = surface { approvalCardOnScreen = true } else { approvalCardOnScreen = false } permissionQueue.append(request) - // Show UI only when nothing was already on screen to be stolen from. - if !wasShowingPermission, nextVisiblePermissionIndex() != nil { + // Show UI only when no approval card is already up to be stolen from. + if !approvalCardOnScreen, nextVisiblePermissionIndex() != nil { // showNextPending picks the first *visible* request, promotes it to // the head and applies the session-list / Smart Suppress rules — // pointing the card at this session by hand would show the dismissed diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 1b0163c7..bec286bd 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -566,8 +566,18 @@ final class AppStatePermissionFlowTests: XCTestCase { ) // Stop here on failure: under the bug, approvePermission() below resolves // the dismissed request instead, so `await laterTask.value` would hang - // and the test would report as a timeout rather than by name. - guard appState.surface == .approvalCard(sessionId: "s-later") else { + // and the test would report as a timeout rather than by name. The head + // check matters as much as the surface one — an implementation that + // points the card at this session by hand shows "s-later" while the + // dismissed request still leads the queue, so the surface assertion + // alone passes and the await still hangs. + XCTAssertEqual( + appState.permissionQueue.first?.event.sessionId, + "s-later", + "the card on screen must be backed by the head of the queue, which is what approve resolves" + ) + guard appState.surface == .approvalCard(sessionId: "s-later"), + appState.permissionQueue.first?.event.sessionId == "s-later" else { appState.handlePeerDisconnect(sessionId: "s-dismissed") appState.handlePeerDisconnect(sessionId: "s-later") _ = await dismissedTask.value @@ -584,10 +594,15 @@ final class AppStatePermissionFlowTests: XCTestCase { _ = await dismissedTask.value } - /// The dismissed session's own next request stays hidden — dismissal is - /// per-session and is only cleared when that session's request resolves. - /// Pinned so the #309 fix is not read as changing it. - func testDismissedSessionsOwnNextRequestStaysHidden() async throws { + /// A dismissal is cleared by that session's NEXT request arriving + /// (`handlePermissionRequest` removes it from `dismissedPermissionSessionIds` + /// on entry — "session needs user decision again"), not by the dismissed + /// request resolving. So the session's next request must bring its card back. + /// + /// This is also the state that silenced everything else: the un-dismiss makes + /// the still-queued earlier request count as visible while nothing is on + /// screen, so a queue-derived "is a card showing" proxy reads true forever. + func testDismissedSessionsNextRequestReRaisesItsCard() async throws { let appState = AppState() let first = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Bash") let second = try makePermissionRequestEvent(sessionId: "s-same", toolName: "Edit") @@ -608,7 +623,11 @@ final class AppStatePermissionFlowTests: XCTestCase { } await Task.yield() - XCTAssertEqual(appState.surface, .collapsed) + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-same"), + "the session un-dismissed itself by asking again, so its card must come back" + ) XCTAssertEqual(appState.permissionQueue.count, 2) appState.handlePeerDisconnect(sessionId: "s-same") @@ -616,6 +635,58 @@ final class AppStatePermissionFlowTests: XCTestCase { _ = await secondTask.value } + /// The state F1 described: a dismissed session asking again must not leave + /// the panel silent for everyone else. + func testDismissedSessionAskingAgainDoesNotSilenceOtherSessions() async throws { + let appState = AppState() + let firstA = try makePermissionRequestEvent(sessionId: "s-a", toolName: "Bash") + let secondA = try makePermissionRequestEvent(sessionId: "s-a", toolName: "Edit") + let fromB = try makePermissionRequestEvent(sessionId: "s-b", toolName: "Read") + + let firstATask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(firstA, continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt() + + let secondATask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(secondA, continuation: $0) } + } + await Task.yield() + + let fromBTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(fromB, continuation: $0) } + } + await Task.yield() + + // The panel must be showing *something* — under the incomplete gate the + // un-dismiss left A's card unopened and B arrived to a silent, collapsed + // panel. (Without this the rest of the test passes either way, because + // resolving A's requests surfaces B regardless.) + XCTAssertNotEqual( + appState.surface, + .collapsed, + "a card must be on screen — a silent collapsed panel is the bug" + ) + + // B queues behind A's card rather than stealing it — but it must not be + // lost: as A's requests clear, B's card has to come up. + XCTAssertEqual(appState.permissionQueue.count, 3) + appState.approvePermission() + appState.approvePermission() + _ = await firstATask.value + _ = await secondATask.value + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-b"), + "B's request must surface once A's are resolved, not sit silently forever" + ) + appState.approvePermission() + let bResponse = await fromBTask.value + XCTAssertEqual(try extractPermissionBehavior(from: bResponse), "allow") + } + private func makePermissionRequestEvent( sessionId: String, toolName: String, From f3642dda20d57463f20344fa6c16e3c34a8df1c9 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 10:52:49 -0500 Subject: [PATCH 07/10] =?UTF-8?q?fix:=20address=20QA=20round=202=20?= =?UTF-8?q?=E2=80=94=20the=20card=20gate=20needs=20both=20halves?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1's surface-only gate had its own hole, found by two lenses independently: drainPermissions (process exit, or a question arriving for the session) empties the queue without clearing `surface`, and the card renders nothing when there is no head request. A bare `.approvalCard` check therefore blocked on a phantom card and swallowed the next request from every session — the same #309 defect, third variant. The gate now requires both an .approvalCard surface and a non-empty queue. Card and sound also stopped sharing a gate. They answer different questions: the card asks "is one already on screen", the sound asks "does this request start a new burst" — which is what `count == 1` used to approximate. Sharing the surface-derived gate made the sound fire per request instead of per burst whenever no card was open, e.g. under Smart Suppress. The sound now keys off whether a visible request was already queued, which keeps burst behaviour and still fixes the dismissed-request silence. Tests, per QA: - new: a request arriving under a stale .approvalCard surface must still raise a card (verified red against round 1's gate). - the cross-session test asserted only "not collapsed", the weakest thing available; it now names the card it expects. - the re-raise test could not tell showNextPending's promotion from a hand-pointed surface, both requests being from one session; it now asserts the card shows the earlier queued request, not the arriving one. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 30 +++++++++--- .../AppStatePermissionFlowTests.swift | 46 +++++++++++++++++-- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index a9bf76b1..be8596d1 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1325,17 +1325,35 @@ final class AppState { // request undisplayed, so a second session's request still waits behind // it. That is pre-existing (`main` behaves the same) and needs // showNextPending to skip un-openable entries; tracked separately. + // + // The surface alone is not enough either: `drainPermissions` empties the + // queue without clearing `surface`, and the card renders nothing when + // there is no head request — so a bare `.approvalCard` check would block + // on a card that is not actually there. Both halves are required. let approvalCardOnScreen: Bool - if case .approvalCard = surface { approvalCardOnScreen = true } else { approvalCardOnScreen = false } + if case .approvalCard = surface, !permissionQueue.isEmpty { + approvalCardOnScreen = true + } else { + approvalCardOnScreen = false + } + + // Card and sound answer different questions and must not share a gate. + // The sound marks the start of a burst of approvals, which is what + // `count == 1` used to approximate; within a burst it stays quiet, and + // a dismissed request sitting in the queue must not count as a burst + // already in progress. + let burstAlreadyInProgress = nextVisiblePermissionIndex() != nil permissionQueue.append(request) // Show UI only when no approval card is already up to be stolen from. - if !approvalCardOnScreen, nextVisiblePermissionIndex() != nil { - // showNextPending picks the first *visible* request, promotes it to - // the head and applies the session-list / Smart Suppress rules — - // pointing the card at this session by hand would show the dismissed - // request's content whenever a dismissed entry still leads the queue. + // showNextPending picks the first *visible* request, promotes it to the + // head and applies the session-list / Smart Suppress rules — pointing + // the card at this session by hand would show the dismissed request's + // content whenever a dismissed entry still leads the queue. + if !approvalCardOnScreen { showNextPending() + } + if !burstAlreadyInProgress { SoundManager.shared.handleEvent("PermissionRequest") } refreshDerivedState() diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index bec286bd..06cb1d19 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -629,12 +629,52 @@ final class AppStatePermissionFlowTests: XCTestCase { "the session un-dismissed itself by asking again, so its card must come back" ) XCTAssertEqual(appState.permissionQueue.count, 2) + // Both requests are from the same session, so the surface alone cannot + // tell "showNextPending promoted the queue head" from "the surface was + // pointed at the arriving request". The card renders the head, so the + // user must be asked about the earlier request, not the newer one. + XCTAssertEqual( + appState.pendingPermission?.event.toolName, + "Bash", + "the card must show the earlier queued request, not the one that just arrived" + ) appState.handlePeerDisconnect(sessionId: "s-same") _ = await firstTask.value _ = await secondTask.value } + /// `drainPermissions` (process exit, or a question arriving for the session) + /// empties the queue without clearing `surface`, and the card renders nothing + /// when there is no head request. A gate that trusts `.approvalCard` alone + /// would block on that phantom card and swallow the next request. + func testRequestArrivingUnderAStaleApprovalSurfaceStillRaisesACard() async throws { + let appState = AppState() + + // The phantom state itself: an .approvalCard surface with an empty queue. + // Production reaches it through the drainPermissions callers that do not + // touch `surface` (process exit; a question arriving for the session). + // Set here directly because those callers are private. + appState.surface = .approvalCard(sessionId: "s-gone") + XCTAssertTrue(appState.permissionQueue.isEmpty) + + let next = try makePermissionRequestEvent(sessionId: "s-next", toolName: "Read") + let nextTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(next, continuation: $0) } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-next"), + "a phantom card must not block the next request from being shown" + ) + + appState.approvePermission() + let response = await nextTask.value + XCTAssertEqual(try extractPermissionBehavior(from: response), "allow") + } + /// The state F1 described: a dismissed session asking again must not leave /// the panel silent for everyone else. func testDismissedSessionAskingAgainDoesNotSilenceOtherSessions() async throws { @@ -663,10 +703,10 @@ final class AppStatePermissionFlowTests: XCTestCase { // un-dismiss left A's card unopened and B arrived to a silent, collapsed // panel. (Without this the rest of the test passes either way, because // resolving A's requests surfaces B regardless.) - XCTAssertNotEqual( + XCTAssertEqual( appState.surface, - .collapsed, - "a card must be on screen — a silent collapsed panel is the bug" + .approvalCard(sessionId: "s-a"), + "A's card must be up — a silent collapsed panel is the bug, and it must be A's card since A's request leads the queue" ) // B queues behind A's card rather than stealing it — but it must not be From c86fe891864254270664e169f5b22a2804ffb2ca Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 11:04:35 -0500 Subject: [PATCH 08/10] docs(test): correct a comment that overstated what its assertion pins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 finding. The assertion checks that the card renders the queue head, so the user is asked about the earlier request — it pins queue order, not which mechanism set the surface. With both requests from one session no assertion at that level can tell showNextPending's promotion from a hand-pointed surface; the cross-session test's head check is what covers that. Comment only. 691 tests, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- .../CodeIslandTests/AppStatePermissionFlowTests.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index 06cb1d19..ed6fd192 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -629,10 +629,12 @@ final class AppStatePermissionFlowTests: XCTestCase { "the session un-dismissed itself by asking again, so its card must come back" ) XCTAssertEqual(appState.permissionQueue.count, 2) - // Both requests are from the same session, so the surface alone cannot - // tell "showNextPending promoted the queue head" from "the surface was - // pointed at the arriving request". The card renders the head, so the - // user must be asked about the earlier request, not the newer one. + // Pins queue order, not the promotion mechanism: the card renders the + // head, so the user is asked about the earlier request rather than the + // one that just arrived. Both requests here are from one session, so no + // assertion at this level can tell showNextPending's promotion from a + // hand-pointed surface — the cross-session test's head check is what + // covers that. XCTAssertEqual( appState.pendingPermission?.event.toolName, "Bash", From e03edca4c6e24bb9db3cdccef39f2334cc460b3c Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 11:37:45 -0500 Subject: [PATCH 09/10] fix: address QA findings on the merged #308 + #309 pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewing the two fixes together found three majors that neither branch's own cycle could have seen. 1. #309's enqueue gate asked `!permissionQueue.isEmpty` — a whole-queue question — while staleness is per-session. With another session still queued, a card whose own request had been drained read as "on screen", so the gate skipped showNextPending() and with it #308's stale-card collapse: the panel wedged behind a card rendering nothing and swallowed every later request. The lenses split on whether this was reachable; it is, without Smart Suppress — a question arriving for the card's session drains its permission, and if the question queue is already non-empty the surface is never reassigned. The gate now asks exactly what the card asks: is this session's request still queued. 2. The un-dismiss ran before the replay-dedup early return, so a replayed tool_use_id for a dismissed session resurrected the request the user had hidden — taking the card the arriving session should have received, and counting as a burst already in progress so that session lost its sound too. A replay is the same decision arriving twice, not a new one; the un-dismiss now happens only on the path that actually enqueues. 3. The merged tests were the strict union of the two branches — the gate and the session-routed actions never met. Added AppStateIntegrationRoutingTests covering the drained-card-with-others-queued state, an inline session-list approval routed to a session that is not the queue head, and the replay case; the existing #309 burst test now routes its approvals by session. Both production fixes verified by mutating them back and watching the new tests fail. 710 tests, 0 failures. Local integration branch; not for upstream. The two PRs stay independent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 27 ++- .../AppStateIntegrationRoutingTests.swift | 190 ++++++++++++++++++ .../AppStatePermissionFlowTests.swift | 7 +- 3 files changed, 214 insertions(+), 10 deletions(-) create mode 100644 Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index 20ce7904..6949eb3e 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -1308,9 +1308,6 @@ final class AppState { return } - // New incoming permission request means session needs user decision again. - dismissedPermissionSessionIds.remove(sessionId) - // Clear any pending questions for THIS session (mutually exclusive within a session) drainQuestions(forSession: sessionId, reason: "newPermissionRequest") @@ -1331,6 +1328,14 @@ final class AppState { return } + // A genuinely new request means this session needs a user decision again, + // so it stops being dismissed. This must come AFTER the replay-dedup + // return above: a replay is the same decision arriving twice, not a new + // one, and un-dismissing on a replay resurrects the hidden request — + // which then suppresses the next session's sound and can put the + // resurrected session's card on screen instead of the arriving one. + dismissedPermissionSessionIds.remove(sessionId) + // Dismissing hides a request but deliberately leaves it queued, so the // CLI stays blocked and the prompt stays recoverable. Gating on // `permissionQueue.count == 1` therefore swallowed every later request — @@ -1347,12 +1352,18 @@ final class AppState { // it. That is pre-existing (`main` behaves the same) and needs // showNextPending to skip un-openable entries; tracked separately. // - // The surface alone is not enough either: `drainPermissions` empties the - // queue without clearing `surface`, and the card renders nothing when - // there is no head request — so a bare `.approvalCard` check would block - // on a card that is not actually there. Both halves are required. + // The surface alone is not enough either: `drainPermissions` empties one + // SESSION's requests without clearing `surface`, and a card renders + // nothing when its own session has no queued request — so a bare + // `.approvalCard` check would block on a card that is not actually + // there. Ask exactly what the card asks: is this session's request + // still queued? A whole-queue test (`!permissionQueue.isEmpty`) gets + // this wrong whenever another session still has one, which also + // suppresses the stale-card collapse inside showNextPending() and + // wedges the panel behind a dead card. (#308 + #309) let approvalCardOnScreen: Bool - if case .approvalCard = surface, !permissionQueue.isEmpty { + if case .approvalCard(let shownSessionId) = surface, + pendingPermission(forSession: shownSessionId) != nil { approvalCardOnScreen = true } else { approvalCardOnScreen = false diff --git a/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift new file mode 100644 index 00000000..ad365bf4 --- /dev/null +++ b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift @@ -0,0 +1,190 @@ +import XCTest +@testable import CodeIsland +import CodeIslandCore + +/// Where the #308 answer-routing fix and the #309 enqueue-gate fix meet. +/// +/// Each branch's own suite exercises one side: #308 checks that an action +/// resolves the card's session, #309 checks that a request still raises a card. +/// Neither covers the state where both matter at once — a card left pointing at +/// a session whose request was drained while OTHER sessions still have queued +/// requests. +@MainActor +final class AppStateIntegrationRoutingTests: XCTestCase { + + /// Staleness is per-session, so the enqueue gate must be too. A whole-queue + /// test reads "a card is on screen" as true whenever any session has a + /// queued request, which suppresses the stale-card collapse inside + /// showNextPending() and wedges the panel behind a card that renders + /// nothing — swallowing every later request. + func testDrainedCardWithOtherSessionsQueuedDoesNotWedgeThePanel() async throws { + let appState = AppState() + + // A question from another session occupies the question queue, so the + // question that arrives in step 3 will not reassign the surface. + _ = Task { + await withCheckedContinuation { appState.handleQuestion(try! self.question("s-c"), continuation: $0) } + } + await Task.yield() + + let aTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-a", "Bash"), continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-a")) + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-b", "Read"), continuation: $0) } + } + await Task.yield() + + // A question for s-a drains s-a's permission. s-b's is untouched, so the + // queue is still non-empty while s-a's card has nothing behind it. + _ = Task { + await withCheckedContinuation { appState.handleQuestion(try! self.question("s-a"), continuation: $0) } + } + await Task.yield() + _ = await aTask.value + XCTAssertNil(appState.pendingPermission(forSession: "s-a"), "test models a card with no backing request") + XCTAssertFalse(appState.permissionQueue.isEmpty, "and another session still queued behind it") + + _ = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } + } + await Task.yield() + + XCTAssertNotEqual( + appState.surface, + .approvalCard(sessionId: "s-a"), + "the dead card must not survive a later request arriving" + ) + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-b"), + "the panel must move to the queued request that is actually waiting" + ) + } + + /// The routing half, exercised on the card the gate just raised: acting on + /// it must resolve that session, not whatever leads the queue. + func testCardRaisedAfterADrainResolvesItsOwnSession() async throws { + let appState = AppState() + + let bTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-b", "Read"), continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-b") + XCTAssertEqual(appState.surface, .collapsed) + + // A different session's request arrives while s-b sits dismissed. + let dTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } + } + await Task.yield() + XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-d"), "#309: the later session still gets a card") + + // Raising the card promoted s-d to the head, so the card and the head + // agree here. To exercise routing the two must differ — which is the + // session list's inline approval: the user expands the panel and acts on + // the dismissed session while a different one leads the queue. + XCTAssertEqual(appState.permissionQueue.map { $0.event.sessionId }, ["s-d", "s-b"]) + appState.surface = .sessionList + appState.approvePermission(expectedSessionId: "s-b") + + XCTAssertEqual( + appState.permissionQueue.map { $0.event.sessionId }, + ["s-d"], + "the acted-on session must be resolved, not the queue head" + ) + let bResponse = await bTask.value + XCTAssertEqual(try extractBehavior(from: bResponse), "allow") + + appState.approvePermission(expectedSessionId: "s-d") + let dResponse = await dTask.value + XCTAssertEqual(try extractBehavior(from: dResponse), "allow") + } + + /// A replay of the same `tool_use_id` is the same decision arriving twice, + /// not a new one. Clearing the session's dismissal on a replay resurrects + /// the request the user hid, which then takes the card the arriving session + /// should have got — and, because it now counts as a burst already in + /// progress, silences that session's sound too. + func testReplayOfADismissedRequestDoesNotStealTheNextSessionsCard() async throws { + let appState = AppState() + let original = try permWithToolUse("s-replay", "Bash", "tool-1") + + let originalTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(original, continuation: $0) } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-replay") + XCTAssertEqual(appState.surface, .collapsed) + + // The bridge replays the same tool_use_id for the dismissed session. + let replayTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-replay", "Bash", "tool-1"), continuation: $0) + } + } + await Task.yield() + _ = await originalTask.value // the replay denies the previous waiter + XCTAssertEqual(appState.permissionQueue.count, 1, "a replay swaps in place, it does not enqueue") + XCTAssertEqual(appState.surface, .collapsed, "a replay must not resurrect the card the user dismissed") + + let otherTask = Task { + await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-other", "Read"), continuation: $0) } + } + await Task.yield() + + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-other"), + "the arriving session must get the card, not the replayed-and-resurrected one" + ) + + appState.approvePermission(expectedSessionId: "s-other") + let otherResponse = await otherTask.value + XCTAssertEqual(try extractBehavior(from: otherResponse), "allow") + + appState.handlePeerDisconnect(sessionId: "s-replay") + _ = await replayTask.value + } + + // MARK: - Helpers + + private func permWithToolUse(_ sessionId: String, _ toolName: String, _ toolUseId: String) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": toolName, + "tool_use_id": toolUseId, + "tool_input": ["command": "echo test"], + ]))) + } + + private func perm(_ sessionId: String, _ toolName: String) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "PermissionRequest", + "session_id": sessionId, + "tool_name": toolName, + "tool_input": ["command": "echo test"], + ]))) + } + + private func question(_ sessionId: String) throws -> HookEvent { + try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ + "hook_event_name": "Notification", + "session_id": sessionId, + "question": "Pick?", + "options": ["A", "B"], + ]))) + } + + private func extractBehavior(from data: Data) throws -> String { + let json = try XCTUnwrap(try JSONSerialization.jsonObject(with: data) as? [String: Any]) + let output = try XCTUnwrap(json["hookSpecificOutput"] as? [String: Any]) + let decision = try XCTUnwrap(output["decision"] as? [String: Any]) + return try XCTUnwrap(decision["behavior"] as? String) + } +} diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index ed6fd192..f897de4b 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -714,8 +714,11 @@ final class AppStatePermissionFlowTests: XCTestCase { // B queues behind A's card rather than stealing it — but it must not be // lost: as A's requests clear, B's card has to come up. XCTAssertEqual(appState.permissionQueue.count, 3) - appState.approvePermission() - appState.approvePermission() + // Routed by session (#308) rather than by queue position, so this also + // covers the two fixes meeting: the gate raised A's card, and approving + // it must resolve A's requests specifically. + appState.approvePermission(expectedSessionId: "s-a") + appState.approvePermission(expectedSessionId: "s-a") _ = await firstATask.value _ = await secondATask.value From 1cb2acc693ebbe24b73b4ac0c2cc27895a64345b Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Wed, 12 Aug 2026 11:59:26 -0500 Subject: [PATCH 10/10] fix: address QA round 2 on the merged pair MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 found no blocking defects — both round-1 fixes verified correct by construction (every surface writer enumerated for the per-session gate's one candidate hole; all three mergeDuplicatePermissionRequest false paths checked for the moved un-dismiss). Four minors, all taken: - a question arriving for the card's own session drained that session's permission and left the island expanded on a card rendering nothing. Fixed in drainPermissions itself, which is where every drain path converges, rather than at the two call sites that happen to reach it today. - the #309 burst test's new expectedSessionId comment claimed coverage the test does not provide (both of A's requests are at the front, so it cannot show the routing picked the right index); reworded to point at the test that does. - the integration test leaked three continuations, producing SWIFT TASK CONTINUATION MISUSE noise that would hide a real one. - pinned the merge==false branch of the moved un-dismiss: same tool_use_id with differing tool inputs is a distinct request (#169), so it must still clear the dismissal. 711 tests, 0 failures; the drain-collapse verified red without its fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01K4myY8wXiJtDb1xAjsxP5h --- Sources/CodeIsland/AppState.swift | 6 ++ .../AppStateIntegrationRoutingTests.swift | 76 +++++++++++++++++-- .../AppStatePermissionFlowTests.swift | 8 +- 3 files changed, 79 insertions(+), 11 deletions(-) diff --git a/Sources/CodeIsland/AppState.swift b/Sources/CodeIsland/AppState.swift index 6949eb3e..2de439d5 100644 --- a/Sources/CodeIsland/AppState.swift +++ b/Sources/CodeIsland/AppState.swift @@ -2057,6 +2057,12 @@ final class AppState { item.continuation.resume(returning: denyResponse) return true } + // Every drain path routes through here, so this is the one place that + // can guarantee a card is never left rendering a request that no longer + // exists. `handlePeerDisconnect` reaches showNextPending() on its own, + // but the question paths do not, and an .approvalCard whose session was + // just drained renders nothing at all — an expanded, empty island. + collapseStaleCardSurface() } /// Called when the bridge socket disconnects — the question/permission was answered externally (e.g. user replied in terminal) diff --git a/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift index ad365bf4..8d263991 100644 --- a/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift +++ b/Tests/CodeIslandTests/AppStateIntegrationRoutingTests.swift @@ -22,7 +22,7 @@ final class AppStateIntegrationRoutingTests: XCTestCase { // A question from another session occupies the question queue, so the // question that arrives in step 3 will not reassign the surface. - _ = Task { + let cTask = Task { await withCheckedContinuation { appState.handleQuestion(try! self.question("s-c"), continuation: $0) } } await Task.yield() @@ -33,22 +33,27 @@ final class AppStateIntegrationRoutingTests: XCTestCase { await Task.yield() XCTAssertEqual(appState.surface, .approvalCard(sessionId: "s-a")) - _ = Task { + let bTask = Task { await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-b", "Read"), continuation: $0) } } await Task.yield() // A question for s-a drains s-a's permission. s-b's is untouched, so the // queue is still non-empty while s-a's card has nothing behind it. - _ = Task { + let aQuestionTask = Task { await withCheckedContinuation { appState.handleQuestion(try! self.question("s-a"), continuation: $0) } } await Task.yield() _ = await aTask.value - XCTAssertNil(appState.pendingPermission(forSession: "s-a"), "test models a card with no backing request") - XCTAssertFalse(appState.permissionQueue.isEmpty, "and another session still queued behind it") + XCTAssertNil(appState.pendingPermission(forSession: "s-a"), "s-a's request is gone") + XCTAssertFalse(appState.permissionQueue.isEmpty, "and another session is still queued behind it") + XCTAssertNotEqual( + appState.surface, + .approvalCard(sessionId: "s-a"), + "the drain must not leave the island expanded on a card that renders nothing" + ) - _ = Task { + let dTask = Task { await withCheckedContinuation { appState.handlePermissionRequest(try! self.perm("s-d", "Edit"), continuation: $0) } } await Task.yield() @@ -63,6 +68,17 @@ final class AppStateIntegrationRoutingTests: XCTestCase { .approvalCard(sessionId: "s-b"), "the panel must move to the queued request that is actually waiting" ) + + // Resolve the remaining waiters: an unresumed CheckedContinuation is a + // runtime misuse warning, and the noise hides real ones. + appState.handlePeerDisconnect(sessionId: "s-b") + appState.handlePeerDisconnect(sessionId: "s-d") + appState.handlePeerDisconnect(sessionId: "s-c") + appState.handlePeerDisconnect(sessionId: "s-a") + _ = await bTask.value + _ = await dTask.value + _ = await cTask.value + _ = await aQuestionTask.value } /// The routing half, exercised on the card the gate just raised: acting on @@ -151,15 +167,59 @@ final class AppStateIntegrationRoutingTests: XCTestCase { _ = await replayTask.value } + /// The other side of the moved un-dismiss: `mergeDuplicatePermissionRequest` + /// returns false when the tool inputs differ (#169 — parallel tool calls can + /// share an id), so that request DOES enqueue and must still clear the + /// dismissal. Moving the un-dismiss must not strand a session as dismissed. + func testSameToolUseIdWithDifferentInputStillClearsTheDismissal() async throws { + let appState = AppState() + + let firstTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest(try! self.permWithToolUse("s-parallel", "Read", "tool-9"), continuation: $0) + } + } + await Task.yield() + appState.dismissPermissionPrompt(expectedSessionId: "s-parallel") + XCTAssertEqual(appState.surface, .collapsed) + + // Same tool_use_id, different input: a distinct request, not a replay. + let secondTask = Task { + await withCheckedContinuation { + appState.handlePermissionRequest( + try! self.permWithToolUse("s-parallel", "Read", "tool-9", command: "echo different"), + continuation: $0 + ) + } + } + await Task.yield() + + XCTAssertEqual(appState.permissionQueue.count, 2, "differing inputs must enqueue, not merge") + XCTAssertEqual( + appState.surface, + .approvalCard(sessionId: "s-parallel"), + "a genuinely new request must clear the dismissal and bring the card back" + ) + + appState.handlePeerDisconnect(sessionId: "s-parallel") + _ = await firstTask.value + _ = await secondTask.value + } + // MARK: - Helpers - private func permWithToolUse(_ sessionId: String, _ toolName: String, _ toolUseId: String) throws -> HookEvent { + private func permWithToolUse( + _ sessionId: String, + _ toolName: String, + _ toolUseId: String, + command: String = "echo test" + ) throws -> HookEvent { try XCTUnwrap(HookEvent(from: try JSONSerialization.data(withJSONObject: [ "hook_event_name": "PermissionRequest", "session_id": sessionId, "tool_name": toolName, "tool_use_id": toolUseId, - "tool_input": ["command": "echo test"], + "tool_input": ["command": command], ]))) } diff --git a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift index f897de4b..b87b0e4d 100644 --- a/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift +++ b/Tests/CodeIslandTests/AppStatePermissionFlowTests.swift @@ -714,9 +714,11 @@ final class AppStatePermissionFlowTests: XCTestCase { // B queues behind A's card rather than stealing it — but it must not be // lost: as A's requests clear, B's card has to come up. XCTAssertEqual(appState.permissionQueue.count, 3) - // Routed by session (#308) rather than by queue position, so this also - // covers the two fixes meeting: the gate raised A's card, and approving - // it must resolve A's requests specifically. + // Routed by session (#308) rather than by queue position. Both of A's + // requests are at the front here, so this does not by itself prove the + // routing picked the right index — the session-list case in + // AppStateIntegrationRoutingTests is what covers that. It does keep this + // test honest about which session it means to approve. appState.approvePermission(expectedSessionId: "s-a") appState.approvePermission(expectedSessionId: "s-a") _ = await firstATask.value