status

.artifacts/20260831-105824-plan-code-ui-audit/iterations/06-review.md

@@ -0,0 +1,107 @@
+# 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.