173 lines
8.5 KiB
Markdown
173 lines
8.5 KiB
Markdown
# 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.**
|