# Phase 2 Implementation Review Notes

**Reviewer:** T-2.1/2.2/2.3 Test Agent  
**Date:** 2026-05-15  
**Branch read:** `origin/feat/p2-t2-1-websocket`, `origin/feat/p2-t2-2-pairing`, `origin/feat/p2-t2-3-terminal`

---

## Branch Status at Discovery

| Branch | Appeared at attempt | Files |
|--------|---|---|
| `feat/p2-t2-2-pairing` | Attempt 1 (~0 min) | **EMPTY** — only base scaffold |
| `feat/p2-t2-3-terminal` | Attempt 10 (~4.5 min) | Terminal + Auth (both T-2.2 and T-2.3 files) |
| `feat/p2-t2-1-websocket` | Attempt 13 (~6 min) | Network layer + partial FrameCodecTests |

---

## Implementation Summary

### `feat/p2-t2-1-websocket` — Network Layer

**Files:**
- `Sources/Core/Network/FrameCodec.swift` — BinaryFrame, ClientToServer, ServerToClient, FrameCodec namespace
- `Sources/Core/Network/ResumeCursor.swift` — UserDefaults-backed UInt64 cursor
- `Sources/Core/Network/WebSocketClient.swift` — @MainActor Starscream wrapper with Combine subjects
- `Tests/CoreTests/FrameCodecTests.swift` — Partial test file committed to this branch

**Quality:** High. IC-1 wire format is faithfully implemented. All JSON field names match the spec. BinaryFrame.decode correctly uses big-endian byte shifting.

### `feat/p2-t2-2-pairing` — EMPTY

This branch contains **no implementation files** — only the base Xcode scaffold. The pairing code was found on `feat/p2-t2-3-terminal` instead (see below).

### `feat/p2-t2-3-terminal` — Terminal + Auth (both T-2.2 and T-2.3)

**Files:**
- `Sources/Core/Auth/Keychain.swift` — Generic Codable Keychain wrapper
- `Sources/Core/Auth/Pairing.swift` — PairingService with parseQR and exchange
- `Sources/Core/Auth/SidecarCredential.swift` — Codable credential model
- `Sources/UI/Pairing/PairingFlowView.swift` — SwiftUI pairing flow
- `Sources/UI/Pairing/QRScannerView.swift` — AVFoundation QR scanner
- `Sources/UI/Terminal/TerminalTheme.swift` — ThemeColor + TerminalTheme + built-ins
- `Sources/UI/Terminal/ThemeStore.swift` — @MainActor ObservableObject
- `Sources/UI/Terminal/FontStore.swift` — @MainActor ObservableObject
- `Sources/UI/Terminal/TerminalFont.swift` — Static font presets
- `Sources/UI/Terminal/TerminalViewController.swift` — SwiftTerm wrapper
- `Sources/UI/Terminal/TerminalViewRepresentable.swift` — UIViewControllerRepresentable

**Quality:** High. All types are Sendable-annotated correctly.

---

## IC-1 Compliance Issues

### ✅ All Field Names Correct

Every JSON field name in `FrameCodec.swift` matches IC-1:

| Frame | Type field | Extra fields | Status |
|-------|-----------|--------------|--------|
| resume | `"resume"` | `lastSeq` (null or UInt64) | ✅ |
| key | `"key"` | `name` | ✅ |
| keys | `"keys"` | `data` | ✅ |
| paste | `"paste"` | `data` | ✅ |
| snapshot-request | `"snapshot-request"` | none | ✅ |
| state (server) | `"state"` | `value`, `tool?`, `ts` | ✅ |
| snapshot (server) | `"snapshot"` | `seq`, `data` | ✅ |
| session-meta (server) | `"session-meta"` | `name`, `description?`, `createdAt` | ✅ |
| error (server) | `"error"` | `code`, `message` | ✅ |

### ✅ `awaiting-input` Raw Value Correct

`PiState.awaitingInput` has `rawValue = "awaiting-input"` (hyphenated), which is correct per IC-1. This is a common mistake point — the implementation got it right.

### ✅ `lastSeq: null` Encoding

`ClientToServer.resume(lastSeq: nil)` explicitly encodes JSON `null` (not omitting the key), which is what IC-1 requires. The comment in the code explains this intentional choice.

---

## Swift 6 Concurrency Issues

### ⚠️ Minor: `FrameCodec.encoder/decoder` — static mutable-ish singletons

```swift
// FrameCodec.swift
private static let encoder: JSONEncoder = { ... }()
private static let decoder = JSONDecoder()
```

`JSONEncoder` and `JSONDecoder` conform to `Sendable` as of Swift 5.7/Foundation updates. These are `static let` (not `var`), so they are initialized once and never mutated. **No Swift 6 compile error is expected.** However, if the Foundation version on the CI runner predates the `Sendable` conformance, a warning may appear. Recommend verifying with `-strict-concurrency=complete` on the build server.

### ⚠️ `WebSocketClient` — `DelegateAdapter.owner` is a non-isolated var

From the partial view of `WebSocketClient.swift`:

```swift
private let delegateAdapter = DelegateAdapter()
// ...
delegateAdapter.owner = self  // set in init
```

If `DelegateAdapter` stores `owner` as `weak var owner: WebSocketClient?` (not actor-isolated), and Starscream calls the delegate from a background thread, there could be a data race in Swift 6. The comment says callbacks are hopped through `Task { @MainActor in … }` but `owner` assignment itself may not be safe. **Needs audit** when the full file is reviewed during PR.

### ✅ `ThemeStore` and `FontStore` — `@MainActor` Correct

Both stores are `@MainActor final class ObservableObject`. Singleton access via `.shared` from non-actor-isolated contexts will require `await` in Swift 6. Tests in this repo mark their ThemeStore test functions with `@MainActor` to handle this correctly.

### ✅ `PairingService` — `Sendable` Struct

`PairingService` is a `struct Sendable`, and `parseQR` is a `static` function. No concurrency issues.

### ✅ `Keychain` — `final class Sendable`

The implementation declares `final class Keychain: Sendable`. Since all methods delegate to thread-safe `Security` framework APIs and there is no mutable stored state, this is correct. No issues.

### ⚠️ `Keychain.load` throws `.encodingFailed` on decode failure

```swift
} catch {
    throw KeychainError.encodingFailed  // misleading name
}
```

The error case `KeychainError.encodingFailed` is thrown when **decoding** fails (reading from Keychain, not writing). The name is misleading — it should ideally be `.decodingFailed`. This is a minor naming issue, not a correctness problem, but callers that pattern-match `.encodingFailed` might be confused.

---

## Structural Issues

### 🚨 `feat/p2-t2-2-pairing` Branch is Empty

The T-2.2 pairing agent pushed a branch with no implementation files. The Auth and Pairing UI code was implemented by the T-2.3 terminal agent instead. This means:

1. The pairing branch PR will be empty/trivially mergeable.
2. The terminal branch PR contains code for both T-2.2 and T-2.3.
3. **Recommendation:** Credit the terminal agent with T-2.2 work, or have the pairing agent cherry-pick the Auth files before their PR is reviewed.

### ⚠️ `TerminalFont` is not `Equatable`

`TerminalFont` is `Identifiable, Sendable` but **not `Equatable`**. This makes it harder to assert equality in tests and may cause issues if SwiftUI `ForEach` needs to diff fonts. Recommend adding `Equatable` conformance (or at least equality on `id`).

### ⚠️ `JetBrains Mono` font not yet bundled

Documented in the source (`T-2.12 deferred`). Tests must not assume `UIFont(name: "JetBrainsMono-Regular", size:)` succeeds — the fallback to system monospace is correct behavior for now.

---

## Recommendations for Merge Order

1. **`feat/p2-t2-1-websocket`** — Merge first. Pure Foundation, no UI dependencies. All IC-1 wire types defined here; T-2.2 and T-2.3 code may eventually import from this layer.

2. **`feat/p2-t2-3-terminal`** — Merge second. Contains both T-2.2 (Auth) and T-2.3 (Terminal) implementations. Depends on nothing from T-2.2's (empty) branch.

3. **`feat/p2-t2-2-pairing`** — Merge last (or close without merge). The branch is empty and the implementation already landed via T-2.3. If the pairing agent re-uses the branch to deliver a QR-scanning integration test or additional pairing UI, it can be rebased on top of T-2.3's merge.

4. **`feat/p2-tests`** (this branch) — Can merge any time after T-2.1; the tests import `piRemote` which will contain all source files after the three implementation branches are merged.

---

## Test Files Created

| File | Tests | Coverage |
|------|-------|----------|
| `FrameCodecTests.swift` | 21 | BinaryFrame.decode, ClientToServer encoding (all 5 cases), ServerToClient decoding (all 4 types), round-trip |
| `ResumeCursorTests.swift` | 11 | Save/load, update overwrites, zero seq, UInt64.max, clear, session isolation |
| `PairingTests.swift` | 15 | Valid URLs, missing params (3 cases), wrong scheme (2), empty/missing port, error type assertions |
| `KeychainTests.swift` | 7 | Round-trip, upsert, missing→notFound, delete clears, delete no-op, production key not touched, generic Codable |
| `ThemeTests.swift` | 18 | ansiColors count, color range, dark≠github, id values, background values, SwiftTerm conversion, ThemeStore select, Codable round-trip |

**Total: 72 unit tests across 5 files.**