hope-voice-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.