Lumotia/docs/code-review-2026-04-22.md

---
name: Code Review — 2026/04/22
description: Full-sweep audit findings across all Lumotia crates + src-tauri, with triage buckets for quick wins vs release-blockers
type: reference
tags: [code-review, audit, bugs, lumotia, release-blockers]
date: 2026/04/22
---

# Lumotia Code Review — 2026/04/22

Full-sweep read-only audit of every `.rs` file across the Lumotia 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.