62 lines
8.0 KiB
Markdown
62 lines
8.0 KiB
Markdown
# 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.
|