hope-voice-ios/.artifacts/20260831-105824-plan-code-ui-audit/iterations/04-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:02:29Z
• Verdict: changes-requested

Findings

1. High — 12 newly claimed tests are not in the generated Xcode project; the reported count is impossible

• Evidence: Tests/PinnedMediaResourceLoaderTests.swift defines 7 tests and Tests/ConversationStateTests.swift defines 5, but neither filename appears anywhere in Voice.xcodeproj/project.pbxproj. The current test log starts only MediaAPITests, MediaReconcileTests, and TranscriptDedupeTests. XcodeBuildMCP therefore reports 16 tests: 1 API + 7 media reconcile + 8 transcript tests. It does not run either new file.
• Contradicted claim: iterations/04-implementation.md:88-94 says 16 total while separately claiming 7 transcript + 7 resource-loader + 5 conversation-state tests plus existing media/API tests. Those components total 28, not 16. iterations/03-implementation.md:49-54 likewise claims the undiscovered resource tests ran.
• Additional failures hidden by non-discovery:
  • PinnedMediaResourceLoaderTests.swift:67 expects https://192.168.86.2:9178/stream/track-123, but MediaAPI correctly builds under its /media/api base (Sources/Media/MediaAPI.swift:125-133,185-190), so the expected URL must be https://192.168.86.2:9178/media/api/stream/track-123.
  • ConversationStateTests.swift:18-26 expects listening after two zero ticks. EMA remains 0.02304 after those ticks, above the 0.02 agent threshold, so the test needs a third zero tick or a deterministic threshold-crossing loop.
• Impact: the parser, stream URL, and new state-machine tests have never compiled or executed. Current “16 passed” does not verify the claimed fixes.
• Correction: regenerate Voice.xcodeproj from project.yml, correct the two invalid expectations, rerun all expected 28 tests, and report suites/counts from the actual result bundle.

2. High — visible history duplicates every live final, while legitimate repeated finals are still lost

• Evidence: every accepted final is added both to events and history.entries (Sources/LiveKit/TranscriptHandler.swift:45-61,64-89). TranscriptListView.visibleRows then concatenates all history entries with all live events (Sources/Features/Conversation/OrbView.swift:78-92). A live final therefore renders once as a “restored” history row and once as its live event.
• Repeat semantics: dedupe state records only role/text/time, not source (TranscriptHandler.swift:31-35,45-60,82-88). Two distinct primary utterances with identical text inside five seconds are suppressed even though cross-source dedupe should suppress only the opposite-source copy. After five seconds, TranscriptHistory.add still permanently drops an adjacent same-role/same-text repeat (Sources/LiveKit/TranscriptHistory.swift:25-31), so it will not survive relaunch.
• Test gap: TranscriptDedupeTests.swift:100-120 is named “legitimate repeat after dedupe window,” but does not advance time and explicitly asserts that the repeat is suppressed. There are no restoration/max-50/live-visible-row/clear integration tests requested by iteration 04.
• Impact: current messages appear twice; valid repetitions such as “yes” twice can disappear from events within five seconds or from persisted history indefinitely.
• Correction: snapshot restored entries separately from live-session history, or render a single canonical transcript collection; do not concatenate two stores containing the same finals. Track source in recent dedupe records and consume only an opposite-source duplicate. Remove permanent dedupe from persistence. Inject a clock and test before/after-window same-source repeats, cross-source pairs, restoration, live append, max-50, and clear.

3. High — server URL changes still write/apply per keystroke, active-session changes are dropped rather than deferred, and invalid values still fall back silently for media

• Evidence: Settings continues assigning settings.serverURL from every urlText change (Sources/Features/Settings/SettingsView.swift:18-24), and SettingsStore persists each assignment. Conversation replacement fires for every intermediate value that parses as a valid URL (Sources/Features/Conversation/ConversationView.swift:71-83), so typing a valid hostname can launch repeated unstructured disconnect/replacement tasks.
• The active-session guard simply returns (ConversationView.swift:75); there is no state observer or pending configuration, so the change is never applied when the session later becomes idle.
• makeSession mutates an invalid value to the default before onAppear validation (ConversationView.swift:22-35,58-61), so the promised original-value error cannot appear. Invalid values entered later get no visible validation error. Media still silently uses the default host for invalid visible settings (Sources/Features/Media/MediaPanelView.swift:448-459).
• Impact: the iteration-04 configuration contract is not met; conversation/media can use stale or fallback configuration, and valid typing can race multiple session replacements.
• Correction: keep draft URL state local, validate on explicit Apply/Submit, show validation in Settings, persist only accepted values, serialize one replacement, and queue a pending accepted config until the current session reaches idle. Media must use the last accepted config, never derive a fallback from invalid draft text.

4. High — terminal cleanup can recursively react to its own disconnect, agent-timeout retry can be mistaken for terminal loss, and non-timeout connect failures still bypass cleanup

• Evidence: teardownForFailure() calls teardownRoom() while session state remains .ready/.waitingAgent (Sources/LiveKit/VoiceSession.swift:182-212). teardownRoom() calls room.disconnect() before replacing room (:177-180). The resulting .disconnected callback sees the same room and active state and can invoke teardownForFailure() again (:515-533).
• The intended first agent-timeout retry also calls teardownRoom() while state is .waitingAgent (:192-196), so its intentional disconnect can enter terminal-failure cleanup instead of retrying.
• A non-timeout room.connect failure still only assigns .failed (:134-166) after media scope may already have started; it does not recreate the room or clean media state. A user retry can therefore reuse that room.
• Impact: retries and terminal teardown are race-dependent and can recurse, abort the single retry, or leave stale media/room state.
• Correction: introduce an idempotent teardown flag/state before awaiting disconnect; distinguish intentional disconnects from remote terminal disconnects; set the externally visible failure/transitional state before disconnect callbacks can run; and route every post-token/connect failure through fresh-room cleanup. Add a fake-room lifecycle seam or deterministic integration tests.

5. High — no-resume media interruption can still restart before the pause control completes

• Evidence: on .ended, defer clears isInterruptionActive synchronously (Sources/Media/MediaPlayerController.swift:77-98), while the server pause is sent in an unstructured asynchronous task (:91-96). Until that request completes, polling may still receive desired == "playing"; reconciliation sees the interruption flag already false and calls player.play() (:188-200,237-252). If there is no room or the request fails, server desired remains playing indefinitely.
• Impact: media can restart immediately after an interruption that explicitly did not authorize resume.
• Correction: retain a separate resume-suppression state through successful server reconciliation (or synchronous local desired override plus acknowledged pause), and do not permit reconcile-driven play until a later explicit resume/play action. Add deterministic begin/end-without-resume/poll-before-ack/control-failure tests.

6. Medium — resource-loader cleanup is unused and task bookkeeping is data-racy

• Evidence: removeLoader(_:) has no caller; repository search finds only its declaration (Sources/Media/PinnedMediaResourceLoader.swift:70-75). MediaPlayerController.wireSource and clearMedia never remove the previous asset mapping (Sources/Media/MediaPlayerController.swift:274-283,477-486), so loaderURLs remains unbounded.
• activeTasks is written on the main resource-loader delegate queue (PinnedMediaResourceLoader.swift:134-135,139-149) and also from URLSession completion callbacks on another queue (:98-101) with no actor, lock, or serial queue. It uses a hashValue integer instead of ObjectIdentifier directly (:98,146), allowing collisions.
• For 206 responses with missing/malformed/unknown total, code falls back to expectedContentLength, which is the returned range length, and incorrectly labels it as the full resource (:117-127,153-165). The authored tests endorse this fallback and, in any event, are not discovered. Status/cancellation behavior requested in iteration 04 has no test.
• Impact: loader mappings leak across track changes, cancellation/completion can race dictionary mutation, and AVPlayer can receive an incorrect total length for partial content.
• Correction: invoke removal when replacing/clearing an item, serialize all dictionaries on one queue/lock, key by ObjectIdentifier, and assign expectedContentLength only for 200; for 206 set content length only from a known Content-Range total. Add discovered status/cancellation/bookkeeping tests.

7. Medium — microphone interruption ownership is still set before the mute succeeds and mute/unmute tasks can cross

• Evidence: .began sets micMutedByInterruption = true before launching asynchronous setMuted(true) (Sources/LiveKit/VoiceSession.swift:435-448). .ended can then launch setMuted(false) without awaiting/cancelling the begin task (:449-461). The flag therefore means “mute requested,” not “this interruption successfully muted the mic.”
• Impact: rapid or malformed begin/end sequences can leave SDK mute operations completing out of order, including ending muted after a should-resume event. A failed begin-mute still causes an interruption-owned unmute attempt.
• Correction: serialize interruption microphone operations under a generation/token, mark ownership only after successful mute, and cancel/ignore superseded completions. Device qualification remains required.

Acceptance Criteria

Prior finding disposition

Prior finding	Iteration-04 disposition
1. Compile failure	Fixed; current build succeeds
2. Failure/disconnect lifecycle	Not fixed; Finding 4
3. Media interruption	Not fixed; Finding 5
4. Text-mode mic privacy	Fixed by API guards + disabled toggle; device-unverified
5. Server configuration	Not fixed; Finding 3
6. Transcript dedupe/repeats	Partial; symmetry improved, legitimate repeats still wrong; Finding 2
7. Visible history/Clear	Partial; Clear is truthful, rendering duplicates live finals; Finding 2
8. Mic interruption ownership	Partial; lifecycle resets added, async ownership race remains; Finding 7
9. Conversation state	Source fix is correct; claimed tests are undiscovered and currently contain an invalid EMA expectation
10. Stale media publish	Fixed by room/session/poll generation capture; device-unverified
11. Resource loader	Partial; Finding 6
12. Generic-host TLS	Fixed by host-scoped pin/default handling; network/device-unverified
13. UI/accessibility parity	Correctly documented as pending separate redesign
14. Model picker copy	Fixed by readiness check

Iteration 04 request disposition

Request item	Verdict
1. Complete iteration 03	Partial: stream helper and parser compile; dedupe/interruption/resource tests and semantics remain incomplete
2. Terminal failure cleanup	Changes required (Finding 4)
3. Text-mode mic invariant	Implemented; device-unverified
4. Server config semantics	Changes required (Finding 3)
5. Visible history/Clear	Changes required (Finding 2)
6. Current-reference state inference	Source implemented; test target/expectation correction required
7. Stale-safe media publish	Implemented by inspection; device-unverified
8. Resource-loader hardening	Changes required (Findings 1 and 6)
9. Configurable HTTPS trust	Implemented by inspection; external-host/device test absent
10. Model picker copy	Implemented
11. PLAN/README qualification	Implemented; docs now classify redesign/device work as pending
12. Idle-timer wording	Implemented correctly

Verification Evidence

• Loaded the repository XcodeBuildMCP skill and confirmed defaults: project /Users/shane/dev/voice-ios/Voice.xcodeproj, scheme Voice_iOS, iPhone 17 Pro simulator AFDA137E-1657-428E-BF01-E5CD7989772C.
• XcodeBuildMCP test_sim → 16 passed, 0 failed, 0 skipped in 9.6 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/test_sim_2026-08-31T12-58-36-052Z_pid94423_12510633.log. The log lists only MediaAPITests, MediaReconcileTests, and TranscriptDedupeTests; it proves the two new test files were omitted.
• XcodeBuildMCP build_sim → Build succeeded in 2.8 s. Log: ~/Library/Developer/XcodeBuildMCP/workspaces/voice-ios-9eb036fdea81/logs/build_sim_2026-08-31T12-58-53-892Z_pid94423_5f92c809.log.
• git diff --check → clean.
• Inspected the complete current diff, all five test files, generated project membership, iteration 03/04 reports, product sources, PLAN/README/device docs, and prior review findings.
• Reviewer changed only artifact reports/manifest, not product implementation.

Residual Risks

• Real-device microphone, interruption, LiveKit terminal failure, wake-lock, TLS alternate-host, and supported-media behavior remain unqualified.
• Current YouTube library webm remains server-blocked for AVPlayer; a supported MP3/AAC stream is still needed for client qualification.
• UI redesign/accessibility/app icon remain intentionally pending and out of this correction scope.
• Plain LAN WebSocket signaling and rotating self-signed pin remain documented deployment risks.

Required Actions

1. Regenerate the Xcode project, fix undiscovered test expectations, and prove the full expected 28-test suite runs.
2. Correct Findings 2–7 and add deterministic tests or explicit integration seams for the high-risk lifecycle paths.
3. Update implementation reports/test counts to match actual result-bundle evidence.
4. Request another independent review before approval; retain device-only qualifications.