From 0be079452e53c1432e99d54acb332c9443fbaa9b Mon Sep 17 00:00:00 2001 From: jay Date: Sun, 17 May 2026 03:03:45 +0200 Subject: [PATCH] review: T-2.10 background lifecycle --- review.md | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 review.md diff --git a/review.md b/review.md new file mode 100644 index 0000000..4f9700a --- /dev/null +++ b/review.md @@ -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