Edison Jwa
2f6d45fb04
* feat(audio): add Silero ONNX VAD with WebRTC fallback Introduce SileroOnnxVad and SileroOnnxVadWorker for desktop targets. The worker runs Silero v6 ONNX inference on a dedicated thread, accumulating 10 ms frames into the 512-sample 16 kHz input the model expects. Add VadOutput, VoiceActivityDetector trait, and WebRtcFallbackVad to provide a uniform VAD interface with graceful fallback when the ONNX model is unavailable. Wire the new VadBackend variants through AudioProcessingConfig and the snapshot stats so the bridge can report which detector is active. * feat(audio): integrate desktop VAD worker into capture engine Wire SileroOnnxVadWorker into the desktop capture path so voice activity can open the transmit gate before encoding. The capture callback now processes all audio through resample, downmix, and VAD unconditionally; transmit_active still gates Opus encoding. Add new_desktop_audio_processing_state() to construct the config/stats/worker triple, and apply_desktop_vad_backend() to synchronously load or clear the worker on config changes. Override processing_backend to Noop for desktop so bridge diagnostics report the correct backend rather than the iOS-oriented PlatformVoiceProcessing default. Includes review-driven cleanups: StreamConfig clone to deref per clippy, and a comment explaining why two try_lock calls on silero_vad_worker are structurally necessary (borrow checker requires the policy probe and the fallback path to not share a lock guard because mark_vad_fallback_active takes &mut self). * fix(audio): modernize Windows PTT to current windows-rs API Port the Raw Input plus low-level keyboard hook PTT backend to the newer windows-rs patterns: OptionalHandle, Result-returning CreateWindowExW, and None for CallNextHookEx. Replaces the old HHOOK(0) pointer casts. Add deterministic tests for mouse button 4 and 5 press and release driving the gate. * build(windows): force MSVC release CRT for audiopus cmake builds audiopus_sys calls cmake::build(opus_path), so downstream Cargo env cannot use cmake-rs Config::define() to override CMake's MSVC Debug CRT defaults. Point cmake-rs at a small wrapper that injects the policy and cache variables during configure while passing cmake --build, --version, and -E through unchanged. Keeps Opus Debug builds on Rust's release dynamic CRT (/MD) instead of CMake's default debug CRT (/MDd), which otherwise pulls in unresolved __imp__CrtDbgReportW symbols at test link. Document that the iOS deployment target is intentionally absent from this file. It is enforced by tools/build-ios.sh and the Xcode project; setting it globally here would make native macOS cargo check runs try to link iPhone objects against the macOS SDK. * build(flutter): update pubspec.lock after plugin additions Regenerated lockfile reflecting the local_notifications and connectivity_plus plugin additions from the poke-notifications feature. * fix(audio): address PR #37 review findings Six fixes from independent PR review: 1. BLOCKER: Replace Windows-only cmake .cmd wrapper with cross-platform CMake env vars. Setting CMAKE=tools/cmake-msvc-release-crt.cmd globally broke non-Windows hosts because cmake-rs would try to execute a .cmd file on macOS/Linux. Instead, set CMAKE_POLICY_DEFAULT_CMP0091=NEW and CMAKE_MSVC_RUNTIME_LIBRARY= MultiThreadedDLL as env vars that CMake reads natively. MSVC- specific vars are safely ignored by GCC/Clang toolchains. Delete the now-unnecessary wrapper script. 2. IMPORTANT: Join the Silero worker thread in Drop instead of detaching it. The old code dropped the JoinHandle which detaches the thread; the new code calls handle.join() after closing the channel, ensuring the ONNX session is cleaned up before the worker is replaced during config changes. 3. IMPORTANT: Single-try_lock refactor of the capture VAD callback. The double try_lock (policy probe + send) is replaced by a single scoped try_lock that both probes availability and sends the frame. The guard is dropped before the fallback path, which needs &mut self for mark_vad_fallback_active. This also eliminates the VadWorkerPolicy enum and callback_vad_worker_policy function, whose behavior is now inlined into the callback. 4. IMPORTANT: Remove tracing from the realtime capture callback. mark_vad_fallback_active and sync_vad_backend emitted info!/warn! from the audio thread. Replace with silent atomic state publishing via SharedAudioProcessingStats; the bridge stats stream already exposes vad_fallback_active for diagnostics. 5. IMPORTANT: Defer ONNX model load outside the worker mutex. apply_desktop_vad_backend_to_worker now constructs the new worker before taking the lock, then swaps it in under a short hold. This prevents the realtime callback from being blocked during model I/O + thread spawn. 6. MINOR: Remove unused VadBackend import from vad/mod.rs after deleting the policy code. * fix(audio): address PR #37 second-pass review findings 5-agent review found 5 blocking issues. All addressed: 1. BLOCKER: CMake env vars don't reach CMake cache. Restored .cmd wrapper but scoped to Windows MSVC targets only via [target.x86_64-pc-windows-msvc] and [target.aarch64-pc-windows-msvc] in .cargo/config.toml. Non-Windows hosts are unaffected. 2. BLOCKER: processing_backend normalized in set_audio_processing_config on desktop (cfg-gated override to Noop), mirroring startup default. 3. BLOCKER: Model-path reload was already wired via reload_audio_processing_config. Fixed misleading doc comment in core/lib.rs. 4. BLOCKER: DEC-030 updated to reflect desktop VoiceActivity enablement. Traceability docs (SRS, SysDes, SAD, SDD, implementation-status) updated. 5. Silero ONNX cfg narrowed to desktop-only (excludes macOS/Android). Cargo.toml ort dependency target cfg narrowed similarly. 6. Realtime callback debt documented as TODO at CaptureState::ingest. * fix(audio): exclude ort dep on Android target ort does not provide first-class Android prebuilts in our pin, mirror the iOS/macOS exclusion so cargo metadata succeeds for android targets. * test(audio): fix stale select_ptt_backend import in ptt_privacy The helper moved out of the ptt_backends submodule onto the crate root; update the integration test imports so the test compiles again. * build(windows): scope MSVC release CRT cmake wrapper via Cargo [env] Cargo's [target.<triple>] table only forwards a fixed allowlist (linker, runner, rustflags, rustdocflags, ar), so setting CMAKE there was silently dropped and audiopus_sys kept linking the debug CRT, producing LNK4098 'MSVCRTD conflicts' and __imp__CrtDbgReportW errors on x86_64-pc-windows-msvc test builds. Move the override to Cargo's [env] table using cc/cmake-rs's target-suffixed CMAKE_<triple> lookup (force=true, relative=true) so it applies to MSVC targets only and not to host tooling. Add stdout markers to the wrapper so its invocation is provable in cargo -vv logs. Verified: cargo test -p chanora_audio --target x86_64-pc-windows-msvc --lib --no-run now links cleanly; CMakeCache.txt records CMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL and CMP0091=NEW. * fix(flutter): gate VoiceActivity transmit mode by platform support VoiceActivity relies on the native VAD worker, which is only wired up on Windows, Linux, and Android. Showing the option on iOS, macOS, or web let users select a mode that silently never transmitted. Add voiceActivityTransmitAvailable + transmitModeSegmentsFor() helpers in voice_settings_controls.dart, hide the VAD row in voice_compact.dart and drop the VAD segment from the settings dialog when unsupported. Keep the legacy const transmitModeSegments for the existing widget test and add two new tests covering the gated helper.
11 KiB
11 KiB
Chanora Maintainability Review — 2026-06-08
Document status: Working-branch review record
Branch: simplify-project-review
Scope: Project-wide simplification, fail-safe, and verification review
1. Purpose
This record captures the current maintainability review so implementation, verification, and release documents do not drift behind the code. It focuses on unnecessary Modules, shallow Interfaces, duplicate Implementations, built-in replacement opportunities, and fail-safe gaps that need explicit evidence before release claims.
2. Changes Already Applied on the Branch
| Area | Files | Maintainability result |
|---|---|---|
| Core event DTO locality | core/chanora_core/src/events.rs, core/chanora_core/src/lib.rs |
Public Core event DTOs moved out of the oversized Core integration Module while preserving the public chanora_core::* Interface through re-exports. |
| Core network diagnostics locality | core/chanora_core/src/network_diagnostics.rs, core/chanora_core/src/lib.rs |
Private network diagnostic ring-buffer state and its regression test now live next to the Implementation they protect. |
| Bounded queues | core/chanora_core/src/network_diagnostics.rs, crates/chanora_diagnostics/src/lib.rs |
Replaced Vec + remove(0) queue behaviour with VecDeque, reducing custom queue code and avoiding O(n) front removal. |
| PTT backend errors | crates/chanora_audio/src/ptt_backends/mod.rs |
Replaced manual Display / Error Implementation with existing thiserror::Error; regression test keeps user-facing strings stable. |
| Render downmix | crates/chanora_audio/src/voice_render.rs, crates/chanora_audio/src/ios_raw_unit.rs |
Removed duplicate mono-i16 downmix loop by using the interleaved helper with one output channel. |
| State reducer | crates/chanora_state/src/lib.rs |
Reused the main snapshot reducer for reconnect snapshots instead of duplicating normalization and delta construction. |
| Workspace metadata | crates/chanora_resolver/Cargo.toml, Cargo.lock |
Resolver inherits workspace package metadata, improving release metadata Locality. |
| Flutter voice fail-safes | apps/chanora_flutter/lib/main.dart, apps/chanora_flutter/lib/widgets/voice_compact.dart, apps/chanora_flutter/lib/services/ios_audio_session_controller.dart |
Commit d835394 preserves independent mute owners, releases touch PTT on disposal while held, and catches iOS audio-session MissingPluginException / activation failures so they do not become unhandled async errors. |
| Rust realtime callback hardening | crates/chanora_audio/src/android_voice_unit.rs, crates/chanora_audio/src/ios_raw_unit.rs, crates/chanora_audio/src/engine.rs |
Commit 8606eb4 hardens Android/iOS realtime callback paths. The current branch also migrates the Android-only JNI paths to jni-rs 0.22 so the supported ARM64 Android debug build compiles. Full lock-free audio-handler/config/debug-recorder redesign remains follow-up work. |
3. Remaining Simplification Opportunities
| Recommendation | Candidate files | Strength | Notes |
|---|---|---|---|
| Continue splitting Core internals by responsibility | core/chanora_core/src/lib.rs |
Strong | Next slices should be reconnect/session, voice projection, storage helpers, and diagnostics export. Keep public re-exports stable. |
| Make Bridge depend on Core rather than Audio where possible | crates/chanora_bridge/Cargo.toml, crates/chanora_bridge/src/api.rs, core/chanora_core/src/lib.rs |
Worth exploring | The Bridge currently has a direct audio edge. Apply the deletion test before removing it. |
| Decide whether prefetch deserves a crate-level Seam | crates/chanora_prefetch/src/lib.rs, crates/chanora_resolver/src/lib.rs, core/chanora_core/src/lib.rs |
Worth exploring | Prefetch is a small TTL cache and fire-and-forget resolver Adapter. Merge into resolver if it is resolver policy; merge into Core if it is app orchestration policy. |
| Consolidate protocol/core/bridge event catalogues | crates/chanora_protocol/src/dto.rs, core/chanora_core/src/events.rs, crates/chanora_bridge/src/api.rs |
Worth exploring | Protocol-owned deltas and Core-owned lifecycle events are currently mirrored through multiple DTO layers. |
| Reduce bridge DTO mirror boilerplate | crates/chanora_bridge/src/api.rs |
Worth exploring | Verify Flutter Rust Bridge support before deleting mirrors. If mirrors remain required, centralize conversion patterns and keep field order aligned with Core DTOs. |
| Clarify protocol voice packet exception | crates/chanora_protocol/src/lib.rs, crates/chanora_audio/Cargo.toml |
Worth exploring | The protocol crate documents tsclientlib isolation but deliberately re-exports voice packet types for audio. Document this as an explicit voice wire Seam or move packet construction fully into protocol. |
| Remove shallow audio helpers only after public API check | crates/chanora_audio/src/processor/noop.rs, crates/chanora_audio/src/processor/platform.rs, crates/chanora_audio/src/frame.rs |
Speculative | These Modules are shallow, but deletion must wait until external/public API expectations are checked. |
| Redesign remaining audio shared state outside realtime callbacks | crates/chanora_audio/src/engine.rs, platform voice units, debug recorder/config paths |
Strong follow-up | The focused callback hardening is complete, but a full lock-free AudioHandler / config / debug-recorder redesign should be planned separately and verified on device. |
| Review protocol/core disconnect and control-plane bounds | core/chanora_core/src/lib.rs, crates/chanora_protocol/src/adapter.rs |
Strong follow-up | Unless closed by a later code slice, sustained voice traffic and broken transport should be reviewed for bounded control request and disconnect progress. |
4. Fail-Safe Gaps That Need Evidence
| Gap | Risk | Required evidence before release claim |
|---|---|---|
| Android Keystore-backed DEK remains deferred | Android identity/bookmark encryption has weaker fail-safe properties than final target secure-storage design. | Android secure-storage audit or waiver; explicit release-readiness limitation. |
| Android permission/audio lifecycle needs deeper route exercise | Build/install/launch smoke now passes on the emulator, but full permission-flow and audio-route lifecycle behavior still need an interactive scenario or device test before release. | Device/emulator scenario covering permission request/denial/grant, voice controls, foreground service, audio focus, and route/SCO transitions. |
| iOS device runtime verification not executed in this review | The iOS audio-session error path is hardened, but VoiceProcessingIO/session ordering and runtime audio behavior still need device evidence. | iOS device or simulator build/run plus audio-session smoke evidence before iOS runtime success is claimed. |
| VAD / VoiceActivity wording drift | VAD assets, tests, and Windows/Linux desktop runtime wiring exist, but product-enabled VoiceActivity must remain platform-scoped per DEC-030. |
Release, README, and verification wording must distinguish verified desktop behavior from unsupported mobile/macOS/unverified-platform behavior. |
| Protocol voice packet re-export is an intentional exception | Future maintainers may assume complete protocol isolation and accidentally widen the Seam. | Architecture note in SAD/SDD or a decision-register entry. |
| Bridge DTO mirror drift | Field additions can be missed across Core, Bridge, and Dart generated DTOs. | Bridge generation check plus Flutter analyze/test after bridge DTO changes. |
| Full live reducer integration remains separate from reducer unit coverage | State reducer tests are strong, but runtime UI still has snapshot/probe paths. | SWE.5 integration run proving live protocol events fold through the intended state path, or explicit P1 deferral. |
| Event replay tooling remains absent | Replay-based diagnosis and regression reproduction are limited. | Event replay tool implementation or waiver. |
5. Verification Policy for Future Code Changes
| Change type | Required verification |
|---|---|
| Rust-only change | cargo fmt --all, cargo check --workspace, cargo test --workspace |
| Bridge DTO/API change | Rust verification plus bridge generation check, flutter analyze, and flutter test --exclude-tags e2e in apps/chanora_flutter |
| Android platform/audio/permission change | Rust/Flutter verification plus NDK target compilation, adb devices -l, Android build/install, and a device or emulator smoke test |
| Documentation-only change | Read affected docs and ensure cross-links/document index stay current; code tests are not required unless docs describe a code change just made |
6. Android ADB Status for This Review
adb devices -l now reports an authorized emulator target:
emulator-5554 device product:sdk_gphone64_arm64 model:sdk_gphone64_arm64 device:emu64a transport_id:1
Android default debug build still fails because SDD-118 excludes armeabi-v7a; use a supported ABI target. ARM64 debug build/install/launch smoke evidence from 2026-06-08:
flutter build apk --debug --target-platform android-arm64
✓ Built build/app/outputs/flutter-apk/app-debug.apk
adb -s emulator-5554 install -r apps/chanora_flutter/build/app/outputs/flutter-apk/app-debug.apk
Success
adb -s emulator-5554 shell am start -W -n app.chanora.chanora_flutter/.MainActivity
Status: ok
LaunchState: COLD
Activity: app.chanora.chanora_flutter/.MainActivity
TotalTime: 6514
adb -s emulator-5554 shell pidof app.chanora.chanora_flutter
7287
dumpsys window app.chanora.chanora_flutter showed MainActivity visible with isReadyForDisplay()=true, and dumpsys activity top showed app.chanora.chanora_flutter/.MainActivity resumed with window focus. This is build/install/launch smoke evidence only; permission-flow success and audio-lifecycle success are not claimed by this review.
7. Release and Documentation Alignment Notes
- Android minimum runtime baseline is API 28 (Android 9.0) per SysRS-288, SRS-187, DEC-004, and the Gradle
minSdk = 28configuration. Documents must not revive the older API 24 baseline. - Flutter app version/build is
0.3.0+100inapps/chanora_flutter/pubspec.yaml. Rust workspace package version remains0.2.0-beta.1. Release documents must distinguish these values instead of treating them as one candidate version. - The v0.3.0 changelog entry may mention Windows/Linux desktop VAD-backed
VoiceActivityonly with matching runtime evidence; mobile, macOS, and unverified-platformVoiceActivityremain disabled/unavailable until DEC-030 is superseded and runtime verification exists. - README wording must describe the existing Flutter/Rust workspace and app scaffold, not a future scaffold that has not been created.
8. Git Policy
No commit is created automatically. Commit only on explicit user demand, after reviewing git status, git diff, and recent log output.