hope-voice-ios/.artifacts/20260831-105824-plan-code-ui-audit/iterations/06-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-31T13:55:00Z
• Verdict: changes-requested

Findings

1. High — retry is exposed before failure teardown finishes and can be torn down by the old failure task

• Verified source facts: teardownForFailure(message:) publishes .failed before its first teardown awaits (Sources/LiveKit/VoiceSession.swift:229-247). The failed UI immediately exposes Start (Sources/Features/Conversation/ConversationView.swift:187-193), and connect() accepts .failed without checking or awaiting teardown (VoiceSession.swift:125-131). The old failure task subsequently calls mediaController.endConversation() and teardownRoom() against mutable shared session state, with no generation check before that teardown.
• Impact: while failure cleanup is suspended, a user can tap Start and begin a fresh token/room attempt; the older cleanup can then stop its media scope, disconnect its room, and replace it. Retry therefore does not reliably start from the promised fully fresh, stable room.
• Correction: retain one teardown task/state and prevent or await connect() while it is active. Set the teardown guard before publishing .failed, capture/tear down the exact failed room, and expose retry only after cleanup is complete. Add a deterministic lifecycle seam/test for “failure publishes → immediate retry.”

2. High — microphone begin/end operations are still not one serialized chain

• Verified source facts: a new .began cancels but does not await the previous task before starting another SDK mute (Sources/LiveKit/VoiceSession.swift:496-526). More importantly, .ended creates an unretained task (:538-575). It checks the sequence at :551-556, then suspends in setMicrophone(enabled: true) at :562-566. A newer .began arriving after that check can only cancel micMuteTask (the begin task) at :499; it cannot cancel or order behind the in-flight end/unmute task.
• Impact: an old end can complete its unmute after a newer interruption's mute, leaving the microphone enabled during the newer interruption and publishing isMuted = false. This is the out-of-order privacy race iteration 05/06 required the operation chain to eliminate.
• Correction: retain and serialize both begin and end operations in one chain/actor. Every operation must await its predecessor; a newer begin must supersede or order after an in-flight end/unmute, and stale unmute completion must not leave hardware enabled. Add a fake microphone-operation seam covering begin A → end A → begin B while A's unmute is suspended.

3. High — “consume-on-suppress” transfers ownership instead of consuming it and can drop an entire repeated utterance

• Verified source facts: on suppression, the implementation replaces the opposite-source record with the arriving source (Sources/LiveKit/TranscriptHandler.swift:85-92,120-127) rather than removing the matched pair. For the common same-order sequence primary1, data1, primary2, data2 with identical text: primary1 is accepted; data1 is suppressed and records .data; primary2 is then suppressed and records .primary; data2 is then suppressed and records .data. Only the first utterance remains.
• Test evidence: Tests/TranscriptDedupeTests.swift:309-327 explicitly expects the next primary after a primary/data pair to be suppressed, so the suite codifies the loss instead of testing two complete repeated source pairs.
• Impact: a legitimate second identical utterance can disappear from both the visible transcript and persistence when both transport sources deliver each utterance in the same order.
• Correction: actually consume/remove the matched opposite-source record when suppressing its duplicate, then let the next arrival establish a new pair. Add deterministic P1,D1,P2,D2 and inverse-order tests expecting two canonical/persisted rows.

4. High — repeated accepted server Applies can still produce a session/URL identity mismatch

• Verified source facts: while the current session is idle, Settings permits Apply (Sources/Features/Settings/SettingsView.swift:23-28,42-48). The first change starts replacement and captures URL A, while a second change to URL B is simply dropped by guard !isReplacingSession (Sources/Features/Conversation/ConversationView.swift:76-96). The first task then assigns activeServerURL = A but calls makeSession(settings:), which reads the now-mutable settings.serverURL (:30-40,93-96) and can therefore construct the session for B.
• Impact: state can become activeServerURL == A, visible setting/session == B. A later Apply of A is skipped by guard trimmed != activeServerURL`, leaving the session on B while the visible accepted setting and ad-hoc media APIs target A. This can send connection/media traffic to a server different from the one shown.
• Correction: do not drop accepted changes. Disable Apply while replacement is active or coalesce exactly one latest pending URL; construct the replacement from the captured validated config rather than rereading mutable settings; and set activeServerURL from the exact config used to create the new session. Add a rapid A→B Apply test.

5. High — an old media resume task can restart playback during a newer interruption

• Verified source facts: the should-resume path starts an unretained async server-control task and, after the await, gates local player.play() only on room/poll generation (Sources/Media/MediaPlayerController.swift:98-117). A subsequent .began sets isInterruptionActive = true and pauses (:87-92), but it neither cancels that resume task nor changes the checked generation. The old task can therefore reach player.play() at :115 during the newer interruption.
• Impact: audio can restart during a phone/Siri interruption despite the reconciliation guard. The no-resume race was improved, but overlapping should-resume/begin ordering remains unsafe.
• Correction: give media interruptions an operation token/sequence, retain/cancel resume/pause work, and recheck the current sequence plus !isInterruptionActive immediately before local play. Add begin A → end A/shouldResume → begin B-before-control-completes coverage through an injected control seam.

6. Medium — failed sessions cannot change servers, and Settings does not directly observe session state

• Verified source facts: server Apply is allowed only for exactly .idle (Sources/Features/Settings/SettingsView.swift:23-28). Failure teardown intentionally leaves the state .failed after cleanup (Sources/LiveKit/VoiceSession.swift:229-248), while the failed composer offers Start but no End/reset action (Sources/Features/Conversation/ConversationView.swift:187-193). A syntactically valid but unreachable server therefore cannot be corrected without recreating the view/app. Also, SettingsView.session is a plain property, not an observed object (SettingsView.swift:3-9); its comment relies on indirect parent rendering rather than fulfilling iteration 06's direct observation requirement.
• Impact: users can be trapped retrying the wrong server until relaunch, and the Apply disabled state is not owned reactively by the view that renders it.
• Correction: distinguish “teardown in progress” from “fully torn-down failure” and permit replacement in the latter (or provide an explicit reset-to-idle action). Make Settings directly observe a nonoptional session or an observable wrapper/state projection.

7. Low — the reported per-suite test counts remain inaccurate

• Verified evidence: the current run really passes 47 tests in 5 suites, but source counts are TranscriptDedupeTests 22, MediaReconcileTests 7, ConversationStateTests 5, MediaAPITests 1, and PinnedMediaResourceLoaderTests 12. iterations/06-implementation.md:41-45, implementation.md:121-126, and manifest.md:101-102 instead report 22/6/5/3/11. The incorrect components happen to total 47.
• Impact: total discovery is now sound, but the evidence does not accurately identify what was tested, contrary to correction item 8.
• Correction: update the reports with counts taken from discovered tests/log output. Rename the stale “206 with unknown total returns nil (falls back...)” test title (Tests/PinnedMediaResourceLoaderTests.swift:29) because the implementation correctly does not fall back.

Acceptance Criteria

Iteration 06 disposition

Item	Result	Evidence / qualification
1. Preserve restored transcript on Start	Implemented	ConversationView.startSession() no longer clears; canonical restoration tests pass. The test simulates handler/session creation rather than invoking the view action, but source inspection confirms the removal.
2. Consume-on-suppress dedupe	Changes required	The requested focused examples pass, but full repeated source pairs lose the second utterance (Finding 3).
3. Central failure teardown/message	Changes required	Call sites route through the helper and set failure first, but immediate retry can race unfinished cleanup (Finding 1).
4. Publish idle after explicit teardown	Implemented by inspection	.idle is assigned after media/room teardown at VoiceSession.swift:446-472. Repeated disconnect calls remain untested.
5. Safe server Apply	Changes required	Active calls are blocked, but replacement coalescing/identity and failed-state recovery remain unsafe (Findings 4 and 6).
6. Serialize mic interruption work	Changes required	End/unmute is not part of the retained operation chain (Finding 2).
7. Serialize resource-loader maps	Implemented for current production call paths	AVAsset delegate and URLSession completion queues are both main; current asset creation/removal callers are main-actor isolated. Actual success/error/cancel bookkeeping is not exercised by the parser/lifecycle tests.
8. Verification/report accuracy	Partial	Independent build and all 47 tests pass; per-suite counts in reports are false (Finding 7).

Prior finding disposition

Iteration-04 finding	Current disposition
Test discovery/count	All five files now discovered; total 47 passes, but suite breakdown remains inaccurate.
Canonical transcript/repeats	Canonical storage/rendering fixed; repeat pairing remains incorrect (Finding 3).
Server configuration	Explicit draft/Apply implemented; replacement and failed-state behavior remain incorrect (Findings 4 and 6).
Failure teardown	Centralized and intentional-disconnect recursion improved; retry/teardown race remains (Finding 1).
No-resume media race	The no-resume suppression path is improved; a newer overlapping interruption can still be defeated by an old resume task (Finding 5).
Resource-loader lifecycle/threading	Production map queues and asset cleanup improved; network callback cleanup lacks direct tests.
Mic interruption ordering	Changes still required (Finding 2).

The broader task cannot be approved while correctness findings remain. Original real-device phase exits, Hope UI implementation/accessibility/app icon, and supported-format audible media qualification also remain explicitly pending rather than complete.

Verification Evidence

• Read the task manifest/brief, aggregate implementation/review, iteration 04 review, iteration 05 request/report, iteration 06 request/report, the complete relevant current sources, all five test files, and generated project membership.
• git status --short && git diff --check && git diff --stat → 18 tracked product/config/doc files modified plus untracked Tests/ and .artifacts/; diff check clean; reviewer did not modify product files.
• XcodeBuildMCP skill loaded; session_show_defaults → project /Users/shane/dev/voice-ios/Voice.xcodeproj, scheme Voice_iOS, iPhone 17 Pro simulator AFDA137E-1657-428E-BF01-E5CD7989772C.
• XcodeBuildMCP test_sim → 47 passed, 0 failed, 0 skipped in 9.4 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T13-52-53-924Z_pid94423_58a6793f.log; result bundle: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/result-bundles/test_sim_2026-08-31T13-52-53-924Z_pid94423_3468939d.xcresult. Log line 420 confirms 47 tests/5 suites.
• XcodeBuildMCP build_sim → Build succeeded in 2.9 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T13-53-07-806Z_pid94423_6bf6e3a5.log. Only AppIntents metadata-skip noise was observed; no Swift compiler warning.
• rg -c '@Test\(' Tests/*.swift → Transcript 22, MediaReconcile 7, ConversationState 5, MediaAPI 1, PinnedMediaResourceLoader 12 (47 total).
• Lifecycle/interruption findings are deterministic source-order analyses; no runtime reproduction was claimed because the code has no fake Room/microphone/media-control seams and real interruption qualification requires a device.

Residual Risks

• Real-device microphone capture, mic/media interruption ordering, remote TTS audibility, wake lock, mic-denied behavior, and supported-format media playback remain unqualified.
• Current YouTube/webm library content remains unsuitable for AVPlayer qualification; use a supported MP3/AAC stream for the device checklist.
• Resource-loader tests cover parsing, URL rewriting, and idempotent removal, not successful/error/cancel callback map cleanup or a server ignoring Range with HTTP 200.
• Hope visual redesign, complete accessibility work, reduced motion, and the app icon remain a separate unimplemented UI scope.
• Plain LAN ws:// signaling and rotation of the pinned self-signed certificate remain documented deployment risks.

Required Actions

1. Correct Findings 1–6 and add deterministic seams/tests for failure/retry, repeated transcript source pairs, rapid server Apply, microphone operation ordering, and overlapping media interruptions.
2. Correct suite counts/test naming in the implementation artifacts after the next verified run.
3. Re-run the complete XcodeBuildMCP test suite, separate simulator build, and diff check, then request another independent review.
4. Retain all real-device, server-format, and UI-redesign qualifications; do not mark the original PLAN exits/full parity complete without that evidence.