5.9 KiB

Raw Blame History

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)

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:

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.

5.9 KiB Raw Blame History