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