diff --git a/docs/code-review-2026-04-22.md b/docs/code-review-2026-04-22.md new file mode 100644 index 0000000..c26a0ea --- /dev/null +++ b/docs/code-review-2026-04-22.md @@ -0,0 +1,247 @@ +--- +name: Code Review — 2026/04/22 +description: Full-sweep audit findings across all Kon crates + src-tauri, with triage buckets for quick wins vs release-blockers +type: reference +tags: [code-review, audit, bugs, kon, release-blockers] +date: 2026/04/22 +--- + +# Kon Code Review — 2026/04/22 + +Full-sweep read-only audit of every `.rs` file across the Kon workspace. Four parallel Codex agents scanned: +- **Agent A** — `crates/transcription/`, `crates/audio/` +- **Agent B** — `crates/ai-formatting/`, `crates/llm/`, `crates/storage/` +- **Agent C** — `src-tauri/src/` (commands layer + lib.rs + main.rs + types.rs) +- **Agent D** — `crates/core/`, `crates/cloud-providers/`, `crates/hotkey/`, `crates/mcp/`, `src-tauri/build.rs` + +## Summary + +| Severity | Count | +|---|---| +| **CRITICAL** | 4 | +| **MAJOR** | 16 | +| **MINOR** | 15 | +| **NIT** | 3 | + +**CRITICAL items are all real bugs** — not speculative. Three were introduced or touched during the whisper-ecosystem sprint; one is a latent data-integrity issue in the storage layer. + +**Recommended path:** +1. Fix the four CRITICALs this session. +2. Log all MAJORs as release-blockers (must land before v0.1). +3. MINORs become a boy-scout backlog — picked up opportunistically when adjacent code is touched. +4. NITs resolve inline when the surrounding file is next edited. + +--- + +## CRITICAL + +### C1 — Racy single-session guard in live.rs +- **Path:** `src-tauri/src/commands/live.rs:193-338` +- **Issue:** `start_live_transcription_session` checks `running` is None before multiple `await`s and only stores the handle at the end; `stop_live_transcription_session` removes `running` before awaiting the worker join. Two overlapping IPC calls can admit a second live session OR expose an empty slot while the first session is still shutting down. +- **Fix scope:** large — requires holding the mutex across the async boundary or restructuring the state machine. +- **Bucket:** RELEASE-BLOCKER (this is the file's core invariant). + +### C2 — `RmsVadChunker::flush()` drops chunks +- **Path:** `crates/transcription/src/streaming/rms_vad.rs:294-311` +- **Issue:** `flush()` zero-pads the final partial frame and calls `consume_frame()` via `let _ = ...`, discarding the returned `VadChunk`. If the padded frame triggers end-of-utterance or `max_chunk_samples`, the emitted chunk is lost and the outer state check either returns `None` or an empty chunk. +- **Fix scope:** small — change `flush` trait signature to return `Vec`, collect chunks from both the `consume_frame` call and the final `emit_active_chunk_and_close`. +- **Bucket:** QUICK WIN. Regression test in the same commit. +- **Attribution:** Introduced in `05eea41` yesterday. + +### C3 — Multi-statement migrations can half-apply +- **Path:** `crates/storage/src/migrations.rs:263-299` +- **Issue:** `run_migrations` executes statements individually and only records schema version after the full migration succeeds. A crash mid-migration leaves the schema half-mutated while still appearing unapplied; the next startup replays it against the partially-mutated DB. +- **Fix scope:** medium — wrap each migration in `BEGIN`/`COMMIT` transaction, update version row within the same transaction. +- **Bucket:** RELEASE-BLOCKER. A user with a mid-migration crash today gets a bricked DB. + +### C4 — Transcript provenance can reference deleted profiles +- **Path:** `crates/storage/src/migrations.rs:208-216`, `crates/storage/src/database.rs:61-89`, `:697-708` +- **Issue:** v8 migration adds `transcripts.profile_id` without a foreign-key constraint. `insert_transcript` accepts any `profile_id`; `delete_profile` doesn't guard against existing transcript references. Transcripts can keep orphaned profile IDs, breaking provenance integrity. +- **Fix scope:** large — v9 migration to add FK constraint + reconcile existing orphans; update delete_profile to either cascade or block. +- **Bucket:** RELEASE-BLOCKER. Silent data-integrity hole. + +--- + +## MAJOR (16) + +### src-tauri — Commands layer + +**[MAJOR] `poll_inference` treats IPC listener loss as session-fatal** +- `src-tauri/src/commands/live.rs:721-813` +- Closing the frontend or reloading it kills the whole live session via `?` on `result_channel.send(...)`. Non-fatal Tauri channel lifecycle should not terminate capture. +- Fix scope: medium. Bucket: RELEASE-BLOCKER. + +**[MAJOR] `run_live_session` is a 200+ line multi-responsibility monolith** +- `src-tauri/src/commands/live.rs:349-579` +- Owns mic startup, runtime error draining, resampling, progressive WAV persistence, overload dropping, inference scheduling, and shutdown finalisation in one function. Known lifecycle bugs trace to this. +- Fix scope: large. Bucket: RELEASE-BLOCKER (refactor enables C1 fix). + +**[MAJOR] Native capture worker is detached and can outlive stop/start** +- `src-tauri/src/commands/audio.rs:46-228` +- `start_native_capture` spawns a worker but never retains a join handle. A previous capture can flush into `all_samples` after `stop_native_capture` clears it — truncation and cross-session contamination possible. +- Fix scope: medium. Bucket: RELEASE-BLOCKER. + +**[MAJOR] `resolve_recording_path` collides within the same second** +- `src-tauri/src/commands/audio.rs:236-257` +- Filename derived from `SystemTime::now().as_secs()`. Two recordings started in the same second get the same path → overwrite or merge. +- Fix scope: small. Bucket: QUICK WIN (append milliseconds + session_id). + +**[MAJOR] `get_runtime_capabilities` advertises wrong accelerators** +- `src-tauri/src/commands/models.rs:435-489` +- Hard-codes `accelerators = ["cpu", "vulkan"]` even when `detect_active_compute_device` would report `metal` on macOS or the binary was compiled without the `whisper` feature. +- Fix scope: medium. Bucket: RELEASE-BLOCKER (frontend shows wrong settings otherwise). + +**[MAJOR] `paste_text_replacing` doesn't snapshot the clipboard** +- `src-tauri/src/commands/paste.rs:181-217` +- Inconsistent with `paste_text`. Replacing leaves the raw transcript on the clipboard and destroys whatever the user had copied before. +- Fix scope: small. Bucket: QUICK WIN. + +**[MAJOR] `PowerAssertion::begin` is a non-functional macOS stub** +- `src-tauri/src/commands/power.rs:41-121` +- `begin_activity` always returns `Err` → guard never acquires an App Nap assertion. The plan for A.1 #9 explicitly deferred this; still flagging so it's not forgotten. +- Fix scope: medium. Bucket: RELEASE-BLOCKER (before macOS ship). + +### Transcription + audio + +**[MAJOR] Decoder returns partial audio on errors** +- `crates/audio/src/decode.rs:58-79` +- Packet-read errors break the loop; decoder errors are skipped; function still returns `Ok` if any samples were produced. Truncated files silently accepted. +- Fix scope: medium. Bucket: RELEASE-BLOCKER. + +**[MAJOR] `read_wav()` silently drops sample decode errors** +- `crates/audio/src/wav.rs:135-145` +- `filter_map(|s| s.ok())` for both integer and float iterators. Corrupt samples silently discarded. +- Fix scope: small. Bucket: QUICK WIN. + +**[MAJOR] Model downloads don't validate non-resume HTTP status** +- `crates/transcription/src/model_manager.rs:161-262` +- Resume branch checks 206/200. Normal downloads never call `error_for_status()` → a 4xx/5xx response body gets written to `.part` and renamed. +- Fix scope: small. Bucket: QUICK WIN. + +### LLM + storage + +**[MAJOR] LLM prompts not preflighted against context window** +- `crates/llm/src/lib.rs:143-166`, `:317-321` +- `generate` tokenises the full prompt; `context_window_size` hard-caps at 8192. Long transcripts reach inference with prompts bigger than context → late runtime failure. +- Fix scope: medium. Bucket: RELEASE-BLOCKER. + +**[MAJOR] `uncomplete_task` doesn't reopen auto-completed parents** +- `crates/storage/src/database.rs:389-449` +- `complete_subtask_and_check_parent` auto-completes a parent when the last child completes. `uncomplete_task` only flips the requested row → reopening a child leaves the parent wrongly marked done. +- Fix scope: small. Bucket: QUICK WIN. + +### Core + small crates + +**[MAJOR] `keystore::store_api_key` is a thread-unsafe safe API** +- `crates/cloud-providers/src/keystore.rs:6-18` +- `std::env::set_var` is UB outside single-threaded init per documented precondition. The safe `pub fn` doesn't enforce this. +- Fix scope: medium. Bucket: RELEASE-BLOCKER. + +**[MAJOR] Hotkey device filtering hard-codes `KEY_A` / `KEY_R`** +- `crates/hotkey/src/linux.rs:236-241` +- `try_attach_device` claims to check for the configured hotkey's key but tests for hard-coded `KEY_A` or `KEY_R`. Hotkeys on other keys get silently dropped. +- Fix scope: small. Bucket: RELEASE-BLOCKER (correctness bug in a feature users rely on). + +**[MAJOR] Malformed JSON-RPC silently dropped** +- `crates/mcp/src/main.rs:26-30` +- stdio entry point logs malformed lines and moves on without sending a JSON-RPC parse-error response. `handle_message` has parse-error handling that never runs. +- Fix scope: small. Bucket: QUICK WIN. + +**[MAJOR] `list_transcripts` accepts invalid params as defaults** +- `crates/mcp/src/lib.rs:188-195` +- `serde_json::from_value(args).unwrap_or_default()` converts malformed args into defaults. Every other handler in the file returns `-32602` instead. Inconsistent behaviour. +- Fix scope: small. Bucket: QUICK WIN. + +**[MAJOR] CSP guard matches `connect-src` by prefix** +- `src-tauri/build.rs:47-64` +- `strip_prefix("connect-src")` would also match `connect-src-elem` (if ever added to CSP3). Defensive: exact directive name match. +- Fix scope: small. Bucket: QUICK WIN. +- **Attribution:** Introduced in `6fd3893` yesterday. + +--- + +## MINOR (15) + +Grouped here for brevity — full details in agent outputs. Bucket: BOY SCOUT (fix when adjacent code touched). + +- `commands/live.rs:341-347` — `pick_engine` duplicates dispatch logic from `commands/models.rs` and `commands/transcription.rs` +- `commands/live.rs:123-145` — stale `#[allow(dead_code)]` on `LiveStatusMessage` (all variants are constructed) +- `crates/audio/src/capture.rs:355-499` — `open_and_validate()` is 145 lines; only one unit test in the file +- `crates/audio/src/lib.rs:14` + `vad.rs:14-34` — `SpeechDetector` re-exported but no in-repo uses (stub awaiting Silero) +- `crates/audio/src/resample.rs:25-39` + `streaming_resample.rs:63-80` — rubato tuning duplicated between offline and streaming +- `crates/transcription/src/local_engine.rs:83-157` — `load`/`unload`/`capabilities`/`transcribe_sync` have no direct tests +- `crates/transcription/src/whisper_rs_backend.rs:54-107` — multi-responsibility function, behaviour-testing limited to `Display` +- `crates/ai-formatting/src/pipeline.rs:38-100` — `post_process_segments` does filtering + formatting + LLM invocation + failure handling in one function +- `crates/storage/src/database.rs` (×4 sites) — repeated `SELECT` column lists invite schema drift +- `crates/storage/src/database.rs` (×3 sites) — `list_transcripts_paged`, `count_transcripts`, `update_transcript`, `uncomplete_task`, `log_error`, `list_recent_errors` all untested +- `crates/storage/src/database.rs:774-775` — TODO flagging that Tauri command failures aren't wired into `error_log` +- `crates/core/src/providers.rs:35-40` — dead `ProviderRegistry` suppressed with `#[allow(dead_code)]` +- `crates/core/src/types.rs:169-184` — dead `TranscriptMetadata` suppressed with `#[allow(dead_code)]` +- `crates/hotkey/src/lib.rs:44-77` — parser silently discards extra triggers (`Ctrl+A+B` parses as `B`); no malformed-combo tests +- `crates/hotkey/src/linux.rs:46-142` — `EvdevHotkeyListener::start` is ~100 lines mixing channel setup + device scanning + watcher + retry + task orchestration +- `crates/mcp/src/lib.rs:168-303` — `list_transcripts`, `get_transcript`, `search_transcripts`, `list_tasks` handlers untested + +--- + +## NIT (3) + +- `crates/ai-formatting/src/llm_client.rs:26-27`, `:59-60` — `#[allow(dead_code)]` on actively-used `CLEANUP_PROMPT` and `format_dictionary_suffix` +- `crates/storage/src/file_storage.rs:12-14` — open TODO for consolidating OS-path helpers +- `src-tauri/src/commands/live.rs:123-145` — covered above (re-flagged by Agent C as NIT) + +--- + +## Triage buckets + +### Quick wins (this session or next) + +One concern per commit. TDD where testable — failing regression test, then fix. + +1. **C2** flush() drops chunks → change return type to `Vec` +2. **paste_text_replacing** clipboard snapshot +3. **resolve_recording_path** collision → append millis + session_id +4. **read_wav** propagate sample errors +5. **model_manager** check HTTP status on non-resume path +6. **uncomplete_task** reopen auto-completed parents +7. **CSP guard** exact-name directive match (Rule: my own commit, Boy Scout) +8. **MCP parse-error** reply on malformed JSON-RPC +9. **list_transcripts** return -32602 on invalid params +10. Dead-code cleanups: `ProviderRegistry`, `TranscriptMetadata`, `CLEANUP_PROMPT`/`format_dictionary_suffix` allows, `LiveStatusMessage` allow + +That's 10 items, ~1 commit each. Maybe 2–3 hours. + +### Release-blockers (before v0.1 ship) + +Tracked items that must land before first public release: + +- **C1** racy single-session guard — needs `run_live_session` refactor first +- **C3** migrations atomicity — BEGIN/COMMIT wrap + version in same tx +- **C4** transcript-profile FK + delete_profile guard (v9 migration) +- `run_live_session` monolith refactor (unblocks C1) +- `poll_inference` IPC channel loss resilience +- Native capture worker join handle +- `get_runtime_capabilities` accelerator correctness +- `PowerAssertion` macOS objc2 bridge (known deferred) +- Decoder error propagation (`audio/src/decode.rs`) +- LLM prompt preflight against context window +- Keystore thread-safety +- Hotkey linux device filtering KEY_A/KEY_R bug + +### Boy Scout backlog + +All MINORs + NITs. Pick up opportunistically when adjacent code is touched. + +### Deferred (quality improvements, not release-blocking) + +- SQL SELECT list refactoring (needs macro or typed query builder) +- Test coverage improvements across `local_engine`, `whisper_rs_backend`, `pipeline`, storage APIs, MCP handlers +- Resampler tuning consolidation +- File-storage path helpers consolidation + +--- + +## Notes + +- No `TODO` / `FIXME` / `HACK` / `XXX` markers in the transcription + audio crates (Agent A confirmed). +- Clean files: `transcription/src/lib.rs`, `transcriber.rs`, `concurrency.rs`, `streaming/buffer_trim.rs`, `streaming/commit_policy.rs`, `streaming/mod.rs`, `audio/src/concurrency.rs`, `ai-formatting/src/{correction_learning,lib,rule_based,to_plain_text}.rs`, `llm/src/{grammars,prompts}.rs`, `storage/src/lib.rs`. +- Most-touched files in the sprint (`streaming/*`, `wav.rs`, `commit_policy`, `buffer_trim`) came back clean from A and B — the sprint code itself is in reasonable shape; the bugs cluster in `live.rs` and older storage surfaces.