Compare commits
1 Commits
main
...
review/p2-
| Author | SHA1 | Date |
|---|---|---|
jay
|
0be079452e |
|
|
@ -20,8 +20,7 @@ final class AppState: ObservableObject {
|
|||
}
|
||||
|
||||
private init() {
|
||||
#if DEBUG
|
||||
// Test-only launch-argument overrides (gated: never active in Release).
|
||||
// Test-only overrides
|
||||
let args = ProcessInfo.processInfo.arguments
|
||||
if args.contains("--reset-state") {
|
||||
Keychain.shared.delete(key: "piremote.credential")
|
||||
|
|
@ -33,12 +32,10 @@ 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(
|
||||
|
|
@ -51,7 +48,6 @@ final class AppState: ObservableObject {
|
|||
pairedAt: Date()
|
||||
)
|
||||
}
|
||||
#endif
|
||||
}
|
||||
|
||||
func didPair(credential: SidecarCredential) {
|
||||
|
|
@ -72,15 +68,11 @@ final class AppState: ObservableObject {
|
|||
}
|
||||
|
||||
func appWillForeground() async {
|
||||
_lifecycleTransitions.send(false) // T-2.10: emit before Face-ID gate
|
||||
_lifecycleTransitions.send(false) // T-2.10: always 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) }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -16,8 +16,7 @@ struct piRemoteApp: App {
|
|||
notificationDelegate.setup()
|
||||
UIApplication.shared.registerForRemoteNotifications()
|
||||
|
||||
#if DEBUG
|
||||
// Test-only: auto-pair if argument present (never active in Release).
|
||||
// Test-only: auto-pair if argument present
|
||||
if let pairArgIndex = ProcessInfo.processInfo.arguments.firstIndex(of: "--pair-with-url"),
|
||||
pairArgIndex + 1 < ProcessInfo.processInfo.arguments.count {
|
||||
let urlString = ProcessInfo.processInfo.arguments[pairArgIndex + 1]
|
||||
|
|
@ -25,7 +24,6 @@ struct piRemoteApp: App {
|
|||
handlePairingURL(url)
|
||||
}
|
||||
}
|
||||
#endif
|
||||
}
|
||||
.onOpenURL { url in
|
||||
handlePairingURL(url)
|
||||
|
|
|
|||
|
|
@ -53,11 +53,9 @@ 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
|
||||
|
||||
|
|
@ -67,7 +65,6 @@ public final class SessionConnection: ObservableObject {
|
|||
// MARK: - Private
|
||||
|
||||
private let credential: SidecarCredential
|
||||
private let cursor: ResumeCursor
|
||||
private var client: WebSocketClient?
|
||||
private var cancellables = Set<AnyCancellable>()
|
||||
private var keepAliveTask: Task<Void, Never>?
|
||||
|
|
@ -77,13 +74,9 @@ public final class SessionConnection: ObservableObject {
|
|||
/// Creates a `SessionConnection` for `id` authenticated with `credential`.
|
||||
///
|
||||
/// Does **not** open a WebSocket. Call `resume(from:)` to connect.
|
||||
///
|
||||
/// - Parameter cursor: Injected `ResumeCursor` (defaults to the shared
|
||||
/// UserDefaults-backed instance; override in tests for isolation).
|
||||
init(id: String, credential: SidecarCredential, cursor: ResumeCursor = ResumeCursor()) {
|
||||
init(id: String, credential: SidecarCredential) {
|
||||
self.id = id
|
||||
self.credential = credential
|
||||
self.cursor = cursor
|
||||
self.scrollback = ScrollbackCache(sessionId: id)
|
||||
}
|
||||
|
||||
|
|
@ -108,7 +101,6 @@ public final class SessionConnection: ObservableObject {
|
|||
}
|
||||
}
|
||||
|
||||
#if DEBUG
|
||||
// T-2.10: stub mode — drive states in-process without a real WebSocket.
|
||||
if stubMode {
|
||||
connectionState = .connecting
|
||||
|
|
@ -122,7 +114,6 @@ public final class SessionConnection: ObservableObject {
|
|||
}
|
||||
return
|
||||
}
|
||||
#endif
|
||||
|
||||
guard let url = streamURL else {
|
||||
#if DEBUG
|
||||
|
|
@ -144,11 +135,13 @@ public final class SessionConnection: ObservableObject {
|
|||
}
|
||||
.store(in: &cancellables)
|
||||
|
||||
// Binary frames → scrollback + cursor + downstream `stream` subject.
|
||||
// Binary frames → scrollback + downstream `stream` subject.
|
||||
ws.incomingBinary
|
||||
.sink { [weak self] frame in
|
||||
guard let self else { return }
|
||||
self.handleBinaryFrame(frame)
|
||||
self.isStreamFrozen = false // T-2.10: first delta clears freeze
|
||||
self.scrollback.append(frame.data)
|
||||
self.stream.send(frame.data)
|
||||
}
|
||||
.store(in: &cancellables)
|
||||
|
||||
|
|
@ -217,40 +210,6 @@ 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://<host>:<port>/sessions/<id>/stream?token=<bearerToken>`.
|
||||
|
|
|
|||
|
|
@ -25,26 +25,14 @@ 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 {
|
||||
#if DEBUG
|
||||
return ProcessInfo.processInfo.arguments.contains("--uitest")
|
||||
#else
|
||||
return false
|
||||
#endif
|
||||
ProcessInfo.processInfo.arguments.contains("--uitest")
|
||||
}
|
||||
|
||||
/// 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 {
|
||||
|
|
@ -130,7 +118,6 @@ 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
|
||||
|
|
@ -144,7 +131,6 @@ struct MainTerminalView: View {
|
|||
}
|
||||
return
|
||||
}
|
||||
#endif
|
||||
statusText = "Looking for sessions…"
|
||||
do {
|
||||
let sessionId = try await resolveSession()
|
||||
|
|
@ -258,11 +244,8 @@ 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
|
||||
|
|
@ -284,7 +267,6 @@ struct MainTerminalView: View {
|
|||
connection = conn
|
||||
await conn.resume(from: nil)
|
||||
}
|
||||
#endif
|
||||
|
||||
// MARK: - Session resolution
|
||||
|
||||
|
|
|
|||
|
|
@ -18,10 +18,11 @@ import XCTest
|
|||
@MainActor
|
||||
final class AppStateLifecycleTests: XCTestCase {
|
||||
|
||||
private var cancellables = Set<AnyCancellable>()
|
||||
private nonisolated(unsafe) var cancellables = Set<AnyCancellable>()
|
||||
|
||||
override func tearDown() async throws {
|
||||
override func tearDown() {
|
||||
cancellables.removeAll()
|
||||
super.tearDown()
|
||||
}
|
||||
|
||||
// =========================================================================
|
||||
|
|
@ -170,83 +171,3 @@ 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<AnyCancellable>()
|
||||
|
||||
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)")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -30,10 +30,11 @@ private func fakeCredential() -> SidecarCredential {
|
|||
@MainActor
|
||||
final class SessionConnectionLifecycleTests: XCTestCase {
|
||||
|
||||
private var cancellables = Set<AnyCancellable>()
|
||||
private nonisolated(unsafe) var cancellables = Set<AnyCancellable>()
|
||||
|
||||
override func tearDown() async throws {
|
||||
override func tearDown() {
|
||||
cancellables.removeAll()
|
||||
super.tearDown()
|
||||
}
|
||||
|
||||
// =========================================================================
|
||||
|
|
@ -233,226 +234,3 @@ 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<AnyCancellable>()
|
||||
|
||||
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<AnyCancellable>()
|
||||
|
||||
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<AnyCancellable>()
|
||||
|
||||
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()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,105 @@
|
|||
# T-2.10 Review
|
||||
|
||||
## Verdict
|
||||
[x] NEEDS CHANGES (blockers listed)
|
||||
|
||||
## Test runs
|
||||
- piRemoteTests/SessionConnectionLifecycleTests: 10/10 passed
|
||||
- piRemoteTests/AppStateLifecycleTests: 6/6 passed
|
||||
- piRemoteUITests/BackgroundLifecycleUITests: 4/4 passed
|
||||
- piRemoteUITests (full): 12/12 passed (regression check)
|
||||
|
||||
All tests pass independently confirmed — impl agent's numbers are accurate.
|
||||
|
||||
## Findings
|
||||
|
||||
### Blockers
|
||||
|
||||
#### B-1: `ResumeCursor.update()` is never called — delta-pull is dead in production
|
||||
|
||||
**Location:** `Sources/Core/Sessions/SessionConnection.swift`, `incomingBinary` sink (~line 139)
|
||||
|
||||
```swift
|
||||
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)
|
||||
// ❌ MISSING: ResumeCursor().update(sessionId: id, seq: frame.seq)
|
||||
}
|
||||
```
|
||||
|
||||
`BinaryFrame` carries a `seq: UInt64` from the server. It is parsed correctly (`FrameCodec.swift:162`), but never passed to `ResumeCursor.update()`. As a result:
|
||||
|
||||
- `ResumeCursor().lastSeq(for: conn.id)` always returns `nil` (UserDefaults key never set).
|
||||
- On background: the `scrollback.sizeBytes > 0` condition can be true, but `ResumeCursor().lastSeq()` → `nil` → `lastCapturedSeq = nil`.
|
||||
- On foreground: `conn.resume(from: nil)` → fresh attach, not delta-pull.
|
||||
- `isStreamFrozen` is always `false` in production.
|
||||
- The "Reconnecting…" status text is never shown in production.
|
||||
|
||||
**Why tests pass anyway:**
|
||||
- Unit tests call `resume(from: 100)` / `resume(from: 42)` directly with a hardcoded non-nil seq — they bypass cursor tracking entirely.
|
||||
- UI tests use `lastCapturedSeq = 1` (hardcoded sentinel in the `isUITestStub` branch) — also bypass cursor tracking.
|
||||
|
||||
**Fix required:** Call `ResumeCursor().update(sessionId: id, seq: frame.seq)` in the `incomingBinary` sink after appending to scrollback.
|
||||
|
||||
---
|
||||
|
||||
### Nits
|
||||
|
||||
#### N-1: `stubMode` and launch-arg hooks not gated by `#if DEBUG`
|
||||
|
||||
`SessionConnection.stubMode` is `internal` (no `#if DEBUG`). `AppState.init()` processes `--uitest-with-stub-connection`, `--reset-state`, `--enable-faceid`, `--force-lock` unconditionally. `MainTerminalView.isUITestStub` and the `bootstrapStubConnection()` path are also in production code.
|
||||
|
||||
iOS sandbox prevents arbitrary launch args from external callers, so the risk is low. Recommend wrapping with `#if DEBUG` for hygiene, particularly `stubMode` which bypasses WebSocket entirely.
|
||||
|
||||
#### N-2: Post-Face-ID-auth reconnect gap
|
||||
|
||||
`appWillForeground()` emits `false` on `lifecycleTransitions` **before** the Face-ID gate executes:
|
||||
|
||||
```swift
|
||||
func appWillForeground() async {
|
||||
_lifecycleTransitions.send(false) // ← fires before isLocked is set
|
||||
let elapsed = Date().timeIntervalSince(lastForegroundedAt)
|
||||
guard elapsed > 60 else { return }
|
||||
isLocked = true
|
||||
let ok = await FaceIDGate.authenticate()
|
||||
isLocked = !ok // ← cleared here, but no reconnect signal
|
||||
}
|
||||
```
|
||||
|
||||
The timing is **safe** (the `.onReceive` `Task { @MainActor }` is enqueued but doesn't run until the main actor yields at `await FaceIDGate.authenticate()`, by which point `isLocked = true` is already set, so `guard !appState.isLocked` correctly blocks the resume). Security is intact.
|
||||
|
||||
However, when elapsed > 60s and Face-ID **succeeds** (`isLocked` becomes `false`), no reconnect signal is ever sent. The terminal stays suspended (status: "Disconnected") until the user triggers another foreground cycle. Not a regression from prior behavior, but worth tracking.
|
||||
|
||||
#### N-3: `nonisolated(unsafe) var cancellables` in test classes
|
||||
|
||||
`@MainActor`-isolated test classes use `nonisolated(unsafe)` to store `cancellables` because XCTest doesn't guarantee main-actor delivery for `tearDown`. The pattern is consistent across both test files and works correctly, but consider using `@MainActor func tearDown() async` to avoid the `nonisolated(unsafe)` escape.
|
||||
|
||||
---
|
||||
|
||||
### Coverage gaps
|
||||
|
||||
**CG-1 (follows from B-1):** No test exercises the full path: binary frame received → `ResumeCursor.update()` → background → `lastCapturedSeq` non-nil → foreground → `resume(from: nonNil)` → `isStreamFrozen = true`. All tests that check `isStreamFrozen = true` inject the seq directly or use a stub sentinel. This path must be covered once B-1 is fixed.
|
||||
|
||||
**CG-2:** Post-Face-ID-auth reconnect (elapsed > 60s, auth succeeds, terminal should reconnect). See N-2.
|
||||
|
||||
**CG-3:** `isStreamFrozen` actually gates frame delivery — only the boolean state is unit-tested. There is no test that checks frames queued/dropped while `isStreamFrozen = true` (N/A for the current design since freeze is informational, not a gate, but should be noted if the spec intends it as a real gate).
|
||||
|
||||
**CG-4:** Multiple reconnect cycles with seq progression (seq₀ → suspend → resume(from: seq₀) → seq₁ → suspend → resume(from: seq₁) → …) are not exercised.
|
||||
|
||||
---
|
||||
|
||||
## Additional checks (from review checklist)
|
||||
|
||||
- **`connection?.lastSeq` captured before suspend:** ✓ `lastCapturedSeq` is assigned from `ResumeCursor().lastSeq()` before `await conn.suspend()` in the `.onReceive` handler.
|
||||
- **Face-ID bypass:** ✓ Not bypassed — timing analysis above confirms the `Task { @MainActor }` sees `isLocked = true` before running.
|
||||
- **StatusBar transitions:** ✓ `isStreamFrozen` is set before `connectionState = .connecting` fires in both stub and real paths, so the sink observes the correct flag value.
|
||||
- **No stuck "Reconnecting…":** ✓ Stub drives `.connected` after 800 ms; real path clears `isStreamFrozen` on first binary frame or snapshot.
|
||||
- **Idempotent suspend/resume:** ✓ Tested and passes.
|
||||
- **Latency <200 ms:** ✓ Passes (no blocking operations in the suspend/resume path).
|
||||
- **Face-ID LockView regression:** ✓ `LockScreenUITests.testLockScreenAppearance` passes.
|
||||
- **No existing UI test regressions:** ✓ All 12 UI tests pass.
|
||||
|
||||
## Branch SHA reviewed: 419ad2fec1ccd5f70997b7bb405e0a10ec1633a7
|
||||
Loading…
Reference in New Issue