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

Review Report

• Task: 20260831-105824-plan-code-ui-audit
• Agent: reviewer
• Model: openai-codex/gpt-5.6-sol
• Actual model ID: openai/gpt-5.6-sol
• Effort: high
• Reviewed: 2026-08-31T21:52:04Z
• Verdict: changes-requested

Findings

1. Medium — old-room callbacks can repopulate fields after terminal-failure reset

• Verified source facts: teardownForFailure(message:) clears agent, isReconnecting, and the other ephemeral fields before its asynchronous media/room teardown (Sources/LiveKit/VoiceSession.swift:260-289), then publishes .failed without another reset (:291-293). While teardownRoom() is suspended in room.disconnect() and the old room is still the current identity (:222-226), participantDidConnect can call foundAgent with no lifecycle-state guard (:675-680). Likewise, a .connected → .reconnecting callback sets isReconnecting = true without checking isTearingDown or the session lifecycle (:717-726); the intentional .disconnected branch returns without clearing it (:727-734). The new handleControlEvent allowlist at :114-138 correctly blocks model/task control events, but it does not govern these direct RoomDelegate callbacks.
• Impact: a callback delivered during failure teardown can leave the final .failed session carrying the old agent or reconnect state. ConversationView renders agent without requiring .ready (Sources/Features/Conversation/ConversationView.swift:147-151), and start() does not clear agent, so stale identity can remain visible into retry. This is the same class of ephemeral-state bleed that iteration 08 item 3 was required to close.
• Correction: either perform a final ephemeral-field reset after teardownRoom() and immediately before publishing .failed, or gate every state-mutating room callback to the explicit active lifecycle states (and ignore all connection-state updates while isTearingDown); doing both is safest. Add a deterministic reset/callback seam covering an agent/reconnecting callback while failure teardown is suspended.

Acceptance Criteria

Iteration 08 disposition

Item	Result	Evidence / qualification
1. Mic user-intent and operation ordering	Implemented by source inspection	userWantsMicMuted is changed only after successful public mute/enable operations or text-mode setup; interruption begin awaits its predecessor before evaluating current intent/state and then orders a final mute. The begin-A/end-A/begin-B predicate tests pass. The tests exercise a duplicated pure predicate rather than the real SDK operation chain, so device qualification remains residual, not a simulator code finding.
2. Arbitrary A→B→C server replacement	Implemented by source inspection	Every valid change updates pendingServerURL; one main-actor worker consumes a captured URL, disconnects the exact current session, constructs from that captured config, atomically updates session identity, and loops until pending is empty (ConversationView.swift:81-123). Five coordinator tests pass. The tests model rather than invoke the SwiftUI worker, but the production loop is code-correct for arbitrary pending replacements.
3. Terminal failure reset/control guards	Changes required	The direct fields are cleared and handleControlEvent has the requested active-state allowlist, but direct old-room participant/connection callbacks can repopulate fields during the teardown await (Finding 1).
4. Full verification	Implemented	All 59 discovered simulator tests pass in six suites; separate simulator build succeeds; project membership, source counts, and diff check were independently verified.

Previously approved finding regression check

• No regression was found in the previously accepted transcript consume-on-suppress behavior, media interruption sequence/suppression checks, stale-safe media publishing, resource-loader serialization/lifecycle, explicit server Apply validation, failed-state server correction, text-mode mic invariant, state inference, TLS host scoping, history/Clear behavior, or test discovery.
• The current source counts and run agree: Transcript 24, ConversationState 10, MediaReconcile 7, ServerReplacementCoordinator 5, PinnedMediaResourceLoader 12, MediaAPI 1 = 59.
• The sole remaining issue is the incomplete callback-safe terminal reset above; it does not reopen the independently accepted mic-chain or arbitrary server-worker findings.

The task cannot be approved while Finding 1 remains because it is a source-level correctness defect, even though the current simulator suite and build pass.

Verification Evidence

• Read manifest.md, brief.md, aggregate implementation.md/review.md, every prior review (iterations/02-review.md, 04-review.md, 06-review.md, 07-review.md), and iteration 08 request/implementation reports.
• Inspected the current cumulative worktree diff and exact current VoiceSession, ConversationView, control-channel, media, transcript, resource-loader, settings, TLS, generated-project, and test sources. Reviewer changed no product source, tests, configuration, generated code, or lockfiles.
• Loaded the repository XcodeBuildMCP skill. session_show_defaults confirmed /Users/shane/dev/voice-ios/Voice.xcodeproj, scheme Voice_iOS, iPhone 17 Pro simulator AFDA137E-1657-428E-BF01-E5CD7989772C.
• XcodeBuildMCP test_sim → 59 passed, 0 failed, 0 skipped in 9.7 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T21-49-50-739Z_pid94423_e7565563.log; result bundle: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/result-bundles/test_sim_2026-08-31T21-49-50-739Z_pid94423_c82a15cd.xcresult. Log line 449 confirms 59 tests/6 suites; lines 362, 367, 426, 441, 442, and 448 confirm every suite completed.
• XcodeBuildMCP build_sim → Build succeeded in 3.3 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T21-50-04-874Z_pid94423_dc756cb2.log. The only warning was AppIntents metadata extraction being skipped because no AppIntents dependency exists; no Swift compiler warning was found.
• rg -c '@Test\\(' Tests/*.swift → ServerReplacement 5, MediaAPI 1, ConversationState 10, Pinned Loader 12, Transcript 24, MediaReconcile 7 (59 total).
• Generated Voice.xcodeproj/project.pbxproj contains all six test files in the test Sources phase.
• git diff --check → clean before artifact writes. git status --short showed the reported 18 modified tracked product/config/doc files plus untracked Tests/ and .artifacts/; no unrelated reviewer product change was introduced.

Residual Risks

• Real device: microphone capture/permission, actual AVAudioSession mic and media interruption timing, remote TTS audibility, wake lock/side-button behavior, mic-denied fallback, and supported-format playback remain unqualified.
• Server format: the current YouTube library is webm and cannot qualify AVPlayer playback; use a supported MP3/AAC endpoint on a device. Resource-loader network callback behavior still lacks a true server integration test.
• Separate UI implementation: the Hope LED-wall redesign, complete accessibility/Dynamic Type/reduced-motion pass, visualizer/barge-in refinement, and app icon remain intentionally separate and unimplemented.
• Plain LAN ws:// signaling and self-signed certificate rotation remain documented deployment risks.
• The new mic and server tests are pure models/seams rather than end-to-end production-path tests. Source inspection supports those two fixes, but device/UI integration evidence is still absent.

Required Actions

1. Prevent direct old-room participant/connection callbacks from repopulating ephemeral fields during failure teardown, and add focused deterministic coverage.
2. Re-run all XcodeBuildMCP tests, separate simulator build, suite/source count, and git diff --check after the correction.
3. Keep device, server-format, and separate Hope UI implementation explicitly qualified; they are not blockers to the next code-correctness approval.