docs(cr-2026-04-22): commit source code-review document

Every issue under docs/issues/ links to this file as its Source. It was created for 592b894 but not staged, leaving dangling links in the release-blocker tracker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-22 10:18:45 +01:00
parent 592b894790
commit 1250a70ba2
1 changed files with 247 additions and 0 deletions
--- a/docs/code-review-2026-04-22.md
+++ 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<VadChunk>`, 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<VadChunk>`
+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.