hope-voice-api/ios/.artifacts/20260831-105824-plan-code-ui-audit/iterations/07-review.md

# Review Report

- Task: 20260831-105824-plan-code-ui-audit
- Agent: reviewer
- Model: openai-codex/gpt-5.6-sol
- Effort: high
- Reviewed: 2026-08-31T14:25:00Z
- Verdict: changes-requested

## Findings

### 1. High — a newer microphone interruption can still finish with the microphone enabled

- **Verified source facts:** `.began` computes `wasActive` before awaiting the prior operation (`Sources/LiveKit/VoiceSession.swift:515-533`). In the required begin A → end A/unmute in flight → begin B sequence, A has left `isMuted == true` until its SDK unmute completes, so begin B captures `wasActive == false`. B then waits for A's end task; A completes, enables the microphone, and sets `isMuted = false` (`:580-585`). B resumes but exits at `guard wasActive` (`:531-535`) without muting.
- **Impact:** the retained task chain orders operations, but the final state can still be microphone-on during interruption B—the privacy failure iteration 07 was intended to remove.
- **Correction:** do not snapshot transient `isMuted` before the predecessor completes. Track the user's intended pre-interruption mute state separately, or decide/force the newer begin's mute after its predecessor finishes. Add a deterministic microphone-operation seam for begin A → end A suspended in unmute → begin B, asserting the final hardware/UI state is muted.

### 2. High — server replacement coalescing stops after one pending replacement

- **Verified source facts:** the first replacement consumes one `pendingServerURL` and launches a nested second task (`Sources/Features/Conversation/ConversationView.swift:122-147`). If URL C is accepted while that second replacement is awaiting `currentSession.disconnect()`, the top-level `.onChange` sees `isReplacingSession == true` and stores C at `:92-96`; however, the nested task only sets `isReplacingSession = false` at `:146` and never checks pending again.
- **Impact:** rapid A → B → C Applies can finish with a session and `activeServerURL` on B while the persisted/visible accepted setting is C. No further `onChange` occurs, so conversation and media can target different servers until another distinct edit or relaunch.
- **Correction:** use one retained replacement worker that loops until no pending latest URL remains. Each loop must capture/validate the URL, disconnect the exact current session, construct from that captured config, and atomically update session/active URL. Add an A → B → C coordinator test, including C arriving while B disconnects.

### 3. Medium — terminal failure cleanup leaves old session identity and error state visible into retry

- **Verified source facts:** explicit `disconnect()` clears `agent`, `isMuted`, `micError`, `models`, reconnect flags, and interruption ownership (`Sources/LiveKit/VoiceSession.swift:464-486`). `teardownForFailure(message:)` clears only wait/generation/interruption/state-model/wake-lock/media/room state (`:243-265`); it does not clear `agent`, `isMuted`, `micError`, `models`, or `isReconnecting`. `ConversationView` displays `agent` without requiring `.ready` (`Sources/Features/Conversation/ConversationView.swift:173-177`).
- **Impact:** after a terminal disconnect, the failed and subsequent connecting/waiting UI can show the old agent and microphone error. A retry can also retain the previous server's model list until replacement events arrive, allowing stale session identity/configuration data to bleed into a fresh lifecycle.
- **Correction:** make failure teardown reset the same per-room/session fields as explicit disconnect before publishing `.failed`, while preserving only deliberately persistent settings/history/tasks. Ignore control events outside active connecting/waiting/ready states. Add a small state-reset test seam if the Room cannot be faked.

## Acceptance Criteria

### Iteration 07 disposition

| Item | Result | Evidence / qualification |
|---|---|---|
| 1. Failure teardown blocks retry | Partial | `.tearingDown` correctly withholds `.failed`/Start until media and room cleanup finish, and `connect()` rejects that state. Per-session published fields are not fully reset (Finding 3). |
| 2. One retained mic operation chain | Changes required | Begin and end are retained and ordered, but begin snapshots transient mute state too early and can skip the final mute (Finding 1). |
| 3. Consume matched dedupe record | Implemented | Both suppress paths remove the record; P1/D1/P2/D2 and inverse tests pass with canonical and persistence counts of two. |
| 4. Coalesced captured-config replacement and failed Apply | Changes required | Captured configs, direct observation, and failed-state Apply are implemented; arbitrary latest-wins coalescing stops after the second replacement (Finding 2). |
| 5. Retained media interruption task/token | Implemented by inspection | A newer begin increments sequence, cancels prior work, and the resume path rechecks cancellation, sequence, active interruption, scope, and room immediately before play. Device qualification remains residual. |
| 6. Accurate suite evidence/title | Implemented | Current source/log breakdown is Transcript 24, MediaReconcile 7, ConversationState 5, MediaAPI 1, Pinned Loader 12; total 49. Unknown-total title is corrected. |

### Iteration 06 finding disposition

| Prior finding | Iteration 07 result |
|---|---|
| Failure/retry overlap | Core retry race fixed with `.tearingDown`; state reset remains incomplete (Finding 3). |
| Mic begin/end ordering | Not fully fixed (Finding 1). |
| Repeated-pair transcript loss | Fixed and tested in both orders. |
| Server replacement identity | Two-value case fixed; three-or-more coalescing remains incorrect (Finding 2). |
| Old media resume during newer interruption | Fixed by cancellation/sequence/active-state checks; device-unverified. |
| Failed server lockout/direct observation | Fixed: failed Apply is allowed and Settings directly observes the session. |
| Suite count accuracy | Fixed for iteration 07/current aggregate evidence. |

The current code cannot be approved while Findings 1–3 remain. Real-device, current-library media-format, and separate Hope UI-redesign work are qualifications below, not code findings in this review.

## Verification Evidence

- Read `iterations/07-request.md`, `iterations/07-implementation.md`, prior `iterations/06-review.md`, aggregate implementation/manifest, and the exact current lifecycle, interruption, server replacement, transcript, resource-loader, media, and test sources.
- `git status --short && git diff --check && git diff --stat` → 18 tracked product/config/doc files modified plus untracked `Tests/` and `.artifacts/`; diff check clean before reviewer artifact writes. Reviewer did not modify product code.
- Loaded the repository XcodeBuildMCP skill. `session_show_defaults` → `/Users/shane/dev/voice-ios/Voice.xcodeproj`, scheme `Voice_iOS`, iPhone 17 Pro simulator `AFDA137E-1657-428E-BF01-E5CD7989772C`.
- XcodeBuildMCP `test_sim` → **49 passed, 0 failed, 0 skipped** in 9.3 s. Log: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T14-20-50-489Z_pid94423_32855de5.log`; result bundle: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/result-bundles/test_sim_2026-08-31T14-20-50-489Z_pid94423_fd18ee90.xcresult`. Log line 276 confirms 49 tests/5 suites and lines 198/258/270/271/275 confirm all suite executions.
- XcodeBuildMCP `build_sim` → **Build succeeded** in 3.2 s. Log: `~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T14-21-04-018Z_pid94423_8f26cb11.log`. Only AppIntents metadata-skip noise appeared; no Swift compiler warning.
- `rg -c '@Test\(' Tests/*.swift` → ConversationState 5, MediaAPI 1, MediaReconcile 7, PinnedMediaResourceLoader 12, TranscriptDedupe 24 (49 total).
- Generated `Voice.xcodeproj/project.pbxproj` contains all five test files in the test Sources phase.
- Findings 1–3 are deterministic source-order/state analyses. Runtime reproduction was not claimed because no fake microphone, Room, or server-replacement coordinator seams exist in the current test target.

## Residual Risks

- **Real device:** microphone capture/permission, actual AVAudioSession mic and media interruption timing, remote TTS audibility, wake lock, side-button behavior, and supported-format playback remain unqualified.
- **Server format:** the current YouTube library is webm and cannot qualify AVPlayer playback; use an MP3/AAC endpoint on device. Resource-loader status/cancel/range behavior still lacks a true network integration test.
- **Separate UI scope:** Hope LED-wall redesign, complete accessibility/Dynamic Type/reduced-motion work, visualizer/barge-in refinement, and app icon remain intentionally separate and unimplemented.
- Plain LAN `ws://` signaling and rotation of the self-signed pinned certificate remain documented deployment risks.

## Required Actions

1. Correct Findings 1–3 without broad UI redesign.
2. Add pure/deterministic tests for the mic chain's transient-state case and an arbitrary latest-wins replacement loop (at least A→B→C); cover failure field reset where practical.
3. Re-run all XcodeBuildMCP tests, separate simulator build, suite-count inspection, and `git diff --check`, then request another independent review.
4. Keep device, server-format, and Hope UI work explicitly qualified rather than treating them as blockers to this code-correctness loop.