From 5eaa8ef2c89380a546f0fcae2dd6f06cb04531a3 Mon Sep 17 00:00:00 2001 From: jay Date: Sun, 17 May 2026 12:27:08 +0200 Subject: [PATCH] =?UTF-8?q?fix(ios):=20T-2.10=20review=20follow-up=20?= =?UTF-8?q?=E2=80=94=20B-1=20blocker=20+=20nits=20+=20coverage=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/App/AppState.swift | 12 +- Sources/App/piRemoteApp.swift | 4 +- Sources/Core/Sessions/SessionConnection.swift | 51 +++- Sources/UI/Terminal/MainTerminalView.swift | 20 +- Tests/CoreTests/AppStateLifecycleTests.swift | 85 ++++++- .../SessionConnectionLifecycleTests.swift | 228 +++++++++++++++++- 6 files changed, 385 insertions(+), 15 deletions(-) diff --git a/Sources/App/AppState.swift b/Sources/App/AppState.swift index 301f530..198d792 100644 --- a/Sources/App/AppState.swift +++ b/Sources/App/AppState.swift @@ -20,7 +20,8 @@ final class AppState: ObservableObject { } private init() { - // Test-only overrides +#if DEBUG + // Test-only launch-argument overrides (gated: never active in Release). let args = ProcessInfo.processInfo.arguments if args.contains("--reset-state") { Keychain.shared.delete(key: "piremote.credential") @@ -32,10 +33,12 @@ final class AppState: ObservableObject { if args.contains("--force-lock") { isLocked = true } +#endif // Try loading persisted credential on launch credential = try? Keychain.shared.load(key: "piremote.credential") +#if DEBUG // T-2.10: inject stub credential for UI tests that need MainTerminalView if args.contains("--uitest-with-stub-connection") { credential = SidecarCredential( @@ -48,6 +51,7 @@ final class AppState: ObservableObject { pairedAt: Date() ) } +#endif } func didPair(credential: SidecarCredential) { @@ -68,11 +72,15 @@ final class AppState: ObservableObject { } func appWillForeground() async { - _lifecycleTransitions.send(false) // T-2.10: always emit before Face-ID gate + _lifecycleTransitions.send(false) // T-2.10: emit before Face-ID gate let elapsed = Date().timeIntervalSince(lastForegroundedAt) guard elapsed > 60 else { return } // within 60s → no re-auth isLocked = true let ok = await FaceIDGate.authenticate() isLocked = !ok + // N-2: after successful Face-ID auth the first emission fired when + // isLocked=true (so MainTerminalView skipped the reconnect). Re-emit + // now that isLocked=false so the connection actually resumes. + if ok { _lifecycleTransitions.send(false) } } } diff --git a/Sources/App/piRemoteApp.swift b/Sources/App/piRemoteApp.swift index f161e95..28024c0 100644 --- a/Sources/App/piRemoteApp.swift +++ b/Sources/App/piRemoteApp.swift @@ -16,7 +16,8 @@ struct piRemoteApp: App { notificationDelegate.setup() UIApplication.shared.registerForRemoteNotifications() - // Test-only: auto-pair if argument present +#if DEBUG + // Test-only: auto-pair if argument present (never active in Release). if let pairArgIndex = ProcessInfo.processInfo.arguments.firstIndex(of: "--pair-with-url"), pairArgIndex + 1 < ProcessInfo.processInfo.arguments.count { let urlString = ProcessInfo.processInfo.arguments[pairArgIndex + 1] @@ -24,6 +25,7 @@ struct piRemoteApp: App { handlePairingURL(url) } } +#endif } .onOpenURL { url in handlePairingURL(url) diff --git a/Sources/Core/Sessions/SessionConnection.swift b/Sources/Core/Sessions/SessionConnection.swift index b44bea1..9fc4559 100644 --- a/Sources/Core/Sessions/SessionConnection.swift +++ b/Sources/Core/Sessions/SessionConnection.swift @@ -53,9 +53,11 @@ public final class SessionConnection: ObservableObject { // MARK: - Internal test/UI-test hook +#if DEBUG /// Set to `true` in UI-test stub mode to bypass the real WebSocket and /// drive connection-state transitions in-process. internal var stubMode = false +#endif // MARK: - Scrollback @@ -65,6 +67,7 @@ public final class SessionConnection: ObservableObject { // MARK: - Private private let credential: SidecarCredential + private let cursor: ResumeCursor private var client: WebSocketClient? private var cancellables = Set() private var keepAliveTask: Task? @@ -74,9 +77,13 @@ public final class SessionConnection: ObservableObject { /// Creates a `SessionConnection` for `id` authenticated with `credential`. /// /// Does **not** open a WebSocket. Call `resume(from:)` to connect. - init(id: String, credential: SidecarCredential) { + /// + /// - Parameter cursor: Injected `ResumeCursor` (defaults to the shared + /// UserDefaults-backed instance; override in tests for isolation). + init(id: String, credential: SidecarCredential, cursor: ResumeCursor = ResumeCursor()) { self.id = id self.credential = credential + self.cursor = cursor self.scrollback = ScrollbackCache(sessionId: id) } @@ -101,6 +108,7 @@ public final class SessionConnection: ObservableObject { } } +#if DEBUG // T-2.10: stub mode — drive states in-process without a real WebSocket. if stubMode { connectionState = .connecting @@ -114,6 +122,7 @@ public final class SessionConnection: ObservableObject { } return } +#endif guard let url = streamURL else { #if DEBUG @@ -135,13 +144,11 @@ public final class SessionConnection: ObservableObject { } .store(in: &cancellables) - // Binary frames → scrollback + downstream `stream` subject. + // Binary frames → scrollback + cursor + downstream `stream` subject. ws.incomingBinary .sink { [weak self] frame in guard let self else { return } - self.isStreamFrozen = false // T-2.10: first delta clears freeze - self.scrollback.append(frame.data) - self.stream.send(frame.data) + self.handleBinaryFrame(frame) } .store(in: &cancellables) @@ -210,6 +217,40 @@ public final class SessionConnection: ObservableObject { connectionState = .disconnected } + // MARK: - Binary frame processing + + /// Central handler for incoming binary frames. + /// + /// Called from the `incomingBinary` sink (production) and from the + /// `#if DEBUG` test helper below (unit tests). + /// + /// **CG-3 decision — informational, not blocking:** + /// `isStreamFrozen` is cleared on the first binary frame and the frame IS + /// forwarded to `stream`. In the IC-1 protocol the server only starts + /// sending data after it has processed our `resume` frame, so there are no + /// "stale" bytes that arrive while frozen — the first binary frame IS the + /// first meaningful delta. Stream delivery is therefore never blocked; + /// `isStreamFrozen` serves the status bar ("Reconnecting…") and the + /// `isStreamFrozen` unit tests, not as a gate on `stream`. + private func handleBinaryFrame(_ frame: BinaryFrame) { + isStreamFrozen = false // T-2.10: first delta thaws freeze + scrollback.append(frame.data) + cursor.update(sessionId: id, seq: frame.seq) // B-1: persist seq + stream.send(frame.data) + } + +#if DEBUG + /// Test-only: simulate a binary frame arriving via the full production + /// code path (`handleBinaryFrame`), without needing a real WebSocket. + /// + /// This drives the same `cursor.update` + `stream.send` logic that the + /// `incomingBinary` sink uses, so a regression in either is caught by + /// both production and test paths. + func _testOnly_receiveBinaryFrame(_ frame: BinaryFrame) { + handleBinaryFrame(frame) + } +#endif + // MARK: - URL construction /// Builds `ws://:/sessions//stream?token=`. diff --git a/Sources/UI/Terminal/MainTerminalView.swift b/Sources/UI/Terminal/MainTerminalView.swift index 6c118ed..2fbec3e 100644 --- a/Sources/UI/Terminal/MainTerminalView.swift +++ b/Sources/UI/Terminal/MainTerminalView.swift @@ -25,14 +25,26 @@ struct MainTerminalView: View { /// True when running under XCUITest. Skips the live SwiftTerm view + WS /// connection, which otherwise keep the app non-idle and cause every /// XCUITest interaction to block for ~120 s. + /// + /// Always `false` in Release builds (test hooks are `#if DEBUG` only). private var isUITest: Bool { - ProcessInfo.processInfo.arguments.contains("--uitest") + #if DEBUG + return ProcessInfo.processInfo.arguments.contains("--uitest") + #else + return false + #endif } /// True when the stub-connection mode is active (uitest + background lifecycle). + /// + /// Always `false` in Release builds. private var isUITestStub: Bool { + #if DEBUG let args = ProcessInfo.processInfo.arguments return args.contains("--uitest") && args.contains("--uitest-with-stub-connection") + #else + return false + #endif } var body: some View { @@ -118,6 +130,7 @@ struct MainTerminalView: View { // MARK: - Bootstrap private func initialBootstrap() async { +#if DEBUG // UI-test mode: skip the live WebSocket. SwiftTerm's constant redraw // on incoming frames prevents XCUITest's idle wait from completing, // making every .tap() block for ~120 s. Status bar, modifier bar, and @@ -131,6 +144,7 @@ struct MainTerminalView: View { } return } +#endif statusText = "Looking for sessions…" do { let sessionId = try await resolveSession() @@ -244,8 +258,11 @@ struct MainTerminalView: View { // MARK: - Stub connection (uitest-with-stub-connection) +#if DEBUG /// Creates an in-process stub SessionConnection for UI tests that need /// to exercise the background/foreground lifecycle without a real sidecar. + /// + /// Gated with `#if DEBUG` — never compiled into Release builds. private func bootstrapStubConnection() async { let stubId = "stub" sessionName = stubId @@ -267,6 +284,7 @@ struct MainTerminalView: View { connection = conn await conn.resume(from: nil) } +#endif // MARK: - Session resolution diff --git a/Tests/CoreTests/AppStateLifecycleTests.swift b/Tests/CoreTests/AppStateLifecycleTests.swift index 66428cf..b7e8fde 100644 --- a/Tests/CoreTests/AppStateLifecycleTests.swift +++ b/Tests/CoreTests/AppStateLifecycleTests.swift @@ -18,11 +18,10 @@ import XCTest @MainActor final class AppStateLifecycleTests: XCTestCase { - private nonisolated(unsafe) var cancellables = Set() + private var cancellables = Set() - override func tearDown() { + override func tearDown() async throws { cancellables.removeAll() - super.tearDown() } // ========================================================================= @@ -171,3 +170,83 @@ final class AppStateLifecycleTests: XCTestCase { // MARK: - Stub removed // lifecycleTransitions is now implemented on AppState directly (T-2.10 impl). + +// MARK: - CG-2: Post-Face-ID reconnect signal + +@MainActor +final class PostFaceIDReconnectTests: XCTestCase { + + private var cancellables = Set() + + override func tearDown() async throws { + cancellables.removeAll() + } + + /// CG-2 (depends on N-2): When elapsed > 60 s and Face-ID succeeds, + /// appWillForeground must emit TWO `false` transitions: + /// 1. The first emission fires before Face-ID (isLocked=true at that + /// point — MainTerminalView skips the reconnect via the guard). + /// 2. After successful auth isLocked=false, a second emission fires + /// so MainTerminalView actually re-resumes the connection. + /// + /// Face-ID is disabled in this test (faceid.enabled not set), so + /// FaceIDGate.authenticate() returns `true` immediately without UI. + func test_cg2_postFaceID_emitsForegroundTwiceOnAuthSuccess() async throws { + let state = AppState.shared + + // Ensure Face ID is disabled → FaceIDGate.authenticate() returns true. + UserDefaults.standard.removeObject(forKey: "faceid.enabled") + + // Prime a "recent" background event, then rewind the timestamp so + // elapsed > 60 s (bypasses the grace-period guard). + state.appDidBackground() + state.lastForegroundedAt = Date().addingTimeInterval(-70) + + var falseEmissions = 0 + let exp = expectation(description: "two false emissions after Face-ID success") + exp.expectedFulfillmentCount = 2 + + state.lifecycleTransitions + .filter { $0 == false } + .sink { _ in + falseEmissions += 1 + if falseEmissions <= 2 { exp.fulfill() } + } + .store(in: &cancellables) + + await state.appWillForeground() + + await fulfillment(of: [exp], timeout: 3.0) + + XCTAssertEqual(falseEmissions, 2, + "CG-2 / N-2: exactly two false emissions expected " + + "(one pre-auth, one post-auth-success)") + XCTAssertFalse(state.isLocked, + "CG-2: isLocked must be false after successful Face-ID auth") + } + + /// CG-2 guard: within the 60 s grace period only ONE false emission occurs + /// (no spurious second emission from the N-2 fix). + func test_cg2_withinGracePeriod_emitsForegroundOnce() async throws { + let state = AppState.shared + UserDefaults.standard.removeObject(forKey: "faceid.enabled") + + state.appDidBackground() + // Do NOT rewind timestamp — elapsed ≈ 0 s, within grace period. + + var falseCount = 0 + state.lifecycleTransitions + .filter { $0 == false } + .sink { _ in falseCount += 1 } + .store(in: &cancellables) + + await state.appWillForeground() + + // Allow any async propagation to settle. + try await Task.sleep(nanoseconds: 100_000_000) + + XCTAssertEqual(falseCount, 1, + "CG-2 guard: within grace period must emit exactly ONE false " + + "(no spurious second emission from N-2 fix)") + } +} diff --git a/Tests/CoreTests/SessionConnectionLifecycleTests.swift b/Tests/CoreTests/SessionConnectionLifecycleTests.swift index 9632a44..5b2a943 100644 --- a/Tests/CoreTests/SessionConnectionLifecycleTests.swift +++ b/Tests/CoreTests/SessionConnectionLifecycleTests.swift @@ -30,11 +30,10 @@ private func fakeCredential() -> SidecarCredential { @MainActor final class SessionConnectionLifecycleTests: XCTestCase { - private nonisolated(unsafe) var cancellables = Set() + private var cancellables = Set() - override func tearDown() { + override func tearDown() async throws { cancellables.removeAll() - super.tearDown() } // ========================================================================= @@ -234,3 +233,226 @@ final class SessionConnectionLifecycleTests: XCTestCase { // isStreamFrozen and isKeepAliveActive are now implemented on SessionConnection // directly (T-2.10 impl). The extension stubs above would cause a redeclaration // error and are therefore removed. + +// MARK: - CG-1: End-to-end real-seq path + +@MainActor +final class EndToEndSeqPathTests: XCTestCase { + + private var cancellables = Set() + + override func tearDown() async throws { + cancellables.removeAll() + } + + /// CG-1: Verifies the full production path: + /// binary frame received + /// → ResumeCursor.update (B-1 fix) + /// → background (lastCapturedSeq = cursor.lastSeq) + /// → foreground resume(from: nonNil) + /// → isStreamFrozen = true + /// → first delta received → isStreamFrozen = false, forwarded to stream + /// + /// Uses no hardcoded seq sentinels and exercises the real cursor update + /// path via `_testOnly_receiveBinaryFrame` (which calls `handleBinaryFrame`, + /// the same private method the production `incomingBinary` sink invokes). + func test_cg1_endToEnd_realSeqFlowsThrough() async throws { + // Isolated cursor so this test doesn't pollute real UserDefaults. + let suiteName = "CG1.\(UUID().uuidString)" + let defaults = UserDefaults(suiteName: suiteName)! + let cursor = ResumeCursor(defaults: defaults) + defer { defaults.removePersistentDomain(forName: suiteName) } + + let conn = SessionConnection( + id: "cg1-session", + credential: fakeCredential(), + cursor: cursor + ) + + // ── Step 1: receive a binary frame with a real seq ──────────────── + let seq1: UInt64 = 42 + let frame1 = BinaryFrame(seq: seq1, data: Data("hello".utf8)) + conn._testOnly_receiveBinaryFrame(frame1) + + // ── Step 2: cursor is now updated (what MainTerminalView reads on bg) + let persistedSeq = cursor.lastSeq(for: "cg1-session") + XCTAssertEqual(persistedSeq, seq1, + "CG-1 / B-1: ResumeCursor must be updated with the frame's seq.") + + // ── Step 3: simulate background — capture the cursor value ──────── + let lastCapturedSeq = persistedSeq // non-nil because B-1 is fixed + XCTAssertNotNil(lastCapturedSeq, + "CG-1: lastCapturedSeq must be non-nil after a real binary frame") + + // ── Step 4: foreground — resume from captured seq ───────────────── + await conn.resume(from: lastCapturedSeq) + try await Task.sleep(nanoseconds: 50_000_000) // 50 ms + + XCTAssertTrue(conn.isStreamFrozen, + "CG-1: isStreamFrozen must be true immediately after resume(from: nonNil)") + + // ── Step 5: first delta arrives → freeze clears, stream receives data + var receivedData: [Data] = [] + let streamExp = expectation(description: "first delta forwarded to stream") + conn.stream + .first() + .sink { data in + receivedData.append(data) + streamExp.fulfill() + } + .store(in: &cancellables) + + let frame2 = BinaryFrame(seq: seq1 + 1, data: Data("world".utf8)) + conn._testOnly_receiveBinaryFrame(frame2) + + await fulfillment(of: [streamExp], timeout: 1.0) + + XCTAssertFalse(conn.isStreamFrozen, + "CG-1: isStreamFrozen must clear on first binary frame (first delta IS forwarded)") + XCTAssertEqual(receivedData.count, 1, + "CG-1: Exactly one frame must reach stream") + XCTAssertEqual(receivedData.first, Data("world".utf8), + "CG-1: The delta payload must be forwarded unchanged") + XCTAssertEqual(cursor.lastSeq(for: "cg1-session"), seq1 + 1, + "CG-1: Cursor must be updated to the new seq after the first delta") + + await conn.suspend() + } +} + +// MARK: - CG-3: isStreamFrozen gating behaviour + +@MainActor +final class StreamFrozenGatingTests: XCTestCase { + + private var cancellables = Set() + + override func tearDown() async throws { + cancellables.removeAll() + } + + /// CG-3 decision: INFORMATIONAL (not a hard gate on stream delivery). + /// + /// In the IC-1 protocol the server only starts sending bytes after it + /// processes our `resume` frame, so there are no stale bytes that could + /// arrive while `isStreamFrozen == true`. The first binary frame IS + /// the first meaningful delta and IS forwarded to `stream`. + /// + /// `isStreamFrozen` drives the status-bar label ("Reconnecting…" vs + /// "Connecting…") and exposes the freeze state for observability, but + /// does not technically block bytes from reaching `stream.send()`. + /// + /// This test verifies: + /// - After resume(from: nonNil): isStreamFrozen = true + /// - After the first binary frame: isStreamFrozen = false + /// - The first binary frame IS forwarded to stream (first delta IS forwarded) + func test_cg3_firstDeltaThawsAndIsForwarded() async throws { + let conn = SessionConnection(id: "cg3", credential: fakeCredential()) + + await conn.resume(from: 50) // non-nil → freeze + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertTrue(conn.isStreamFrozen, "Must be frozen after resume(from: nonNil)") + + var received: [Data] = [] + conn.stream + .sink { received.append($0) } + .store(in: &cancellables) + + // First delta arrives while frozen: must thaw AND be forwarded. + let firstDelta = BinaryFrame(seq: 51, data: Data("delta".utf8)) + conn._testOnly_receiveBinaryFrame(firstDelta) + + XCTAssertFalse(conn.isStreamFrozen, + "CG-3: First binary frame must clear isStreamFrozen") + XCTAssertEqual(received.count, 1, + "CG-3: First delta must be forwarded to stream (not dropped)") + + // Subsequent frames also flow normally. + conn._testOnly_receiveBinaryFrame(BinaryFrame(seq: 52, data: Data("more".utf8))) + XCTAssertEqual(received.count, 2, + "CG-3: Subsequent frames must also be forwarded") + + await conn.suspend() + } + + /// Additional CG-3 guard: fresh attach (nil seq) must never set isStreamFrozen. + func test_cg3_freshAttach_neverFreezes() async throws { + let conn = SessionConnection(id: "cg3-fresh", credential: fakeCredential()) + await conn.resume(from: nil) // nil → no freeze + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertFalse(conn.isStreamFrozen, + "CG-3: Fresh attach must not freeze stream") + await conn.suspend() + } +} + +// MARK: - CG-4: Multi-cycle reconnect with seq progression + +@MainActor +final class MultiCycleSeqTests: XCTestCase { + + private var cancellables = Set() + + override func tearDown() async throws { + cancellables.removeAll() + } + + /// CG-4: Two full suspend/resume cycles with seq advancing on each. + /// Cycle 1: receive seq=100 → suspend → resume(from:100) → frozen + /// Cycle 2: receive seq=120 → suspend → resume(from:120) → frozen + /// Verifies cursor advances and isStreamFrozen toggles correctly. + func test_cg4_multiCycle_seqProgression() async throws { + let suiteName = "CG4.\(UUID().uuidString)" + let defaults = UserDefaults(suiteName: suiteName)! + let cursor = ResumeCursor(defaults: defaults) + defer { defaults.removePersistentDomain(forName: suiteName) } + + let conn = SessionConnection( + id: "cg4-session", + credential: fakeCredential(), + cursor: cursor + ) + + // ──────────────────────────────── Cycle 1 ──────────────────────────── + // Receive a frame with seq=100. + conn._testOnly_receiveBinaryFrame(BinaryFrame(seq: 100, data: Data("a".utf8))) + XCTAssertEqual(cursor.lastSeq(for: "cg4-session"), 100, + "CG-4 cycle 1: cursor must be 100 after receiving seq=100") + + // Suspend (isStreamFrozen cleared by suspend). + await conn.suspend() + XCTAssertFalse(conn.isStreamFrozen, "suspend() must clear isStreamFrozen") + + // Resume from captured seq=100 → frozen. + await conn.resume(from: 100) + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertTrue(conn.isStreamFrozen, + "CG-4 cycle 1: must be frozen after resume(from: 100)") + + // Receive seq=120 → thaws. + conn._testOnly_receiveBinaryFrame(BinaryFrame(seq: 120, data: Data("b".utf8))) + XCTAssertFalse(conn.isStreamFrozen, + "CG-4 cycle 1: first delta must clear freeze") + XCTAssertEqual(cursor.lastSeq(for: "cg4-session"), 120, + "CG-4 cycle 1: cursor must advance to 120") + + // ──────────────────────────────── Cycle 2 ──────────────────────────── + await conn.suspend() + XCTAssertFalse(conn.isStreamFrozen, "suspend() must clear isStreamFrozen") + + // Resume from captured seq=120 → frozen. + await conn.resume(from: 120) + try await Task.sleep(nanoseconds: 50_000_000) + XCTAssertTrue(conn.isStreamFrozen, + "CG-4 cycle 2: must be frozen after resume(from: 120)") + + // One more frame → thaws and cursor advances. + conn._testOnly_receiveBinaryFrame(BinaryFrame(seq: 150, data: Data("c".utf8))) + XCTAssertFalse(conn.isStreamFrozen, + "CG-4 cycle 2: second cycle delta must also clear freeze") + XCTAssertEqual(cursor.lastSeq(for: "cg4-session"), 150, + "CG-4 cycle 2: cursor must advance to 150") + + await conn.suspend() + } +}