diff --git a/docs/audits/2026-07-26-code-quality-audit.md b/docs/audits/2026-07-26-code-quality-audit.md new file mode 100644 index 00000000..858810ce --- /dev/null +++ b/docs/audits/2026-07-26-code-quality-audit.md @@ -0,0 +1,145 @@ +# Code Quality Audit — 2026-07-26 + +Four parallel reviews of the codebase at `0.6-dictation-fixes` (`88e72d5`): Python backend, React frontend, Tauri/Rust shell, and CI/repo hygiene. Line references are against that tree. A follow-up section at the bottom lists what was fixed the same day. + +**Scope:** first-party code only (~20k LOC Python, ~29k LOC TypeScript, ~6k LOC Rust, ~4.4k LOC backend tests). + +| Area | Grade | One-liner | +|------|-------|-----------| +| Python backend | B | Well-layered local-inference server with above-average concurrency engineering; test hygiene and ~1,000 self-inflicted lint violations drag it down | +| React frontend | B | Strict typing and clean state architecture; zero tests and 1,748 lines of dead generated API client | +| Tauri/Rust shell | B− | The dictation/clipboard FFI is excellent; `main.rs` god-file and audio modules a tier below | +| CI / repo hygiene | B− | Strong release engineering and docs; PR gate was typecheck + web build only | + +**Consensus:** the hard 10% — GPU lifecycles, clipboard/focus FFI, release packaging — is done unusually well. The gap to A-territory is verification: broken test hygiene, a near-empty CI gate, and second-tier modules that never got the rigor of the flagship paths. + +--- + +## Python backend — B + +`main.py` is 45 lines; the routes-refactor (commit `88536d2`) split routes into 21 domain modules under `routes/`, with real layering: routes → services → backends → database. (`PROJECT_STATUS.md` still described the pre-refactor 2,850-line god-file.) + +### Strengths + +- **MLX serialization** (`services/mlx_thread.py`): single-worker executor with a docstring explaining why (Metal streams are thread-local, issue #699). Load-and-infer submitted as one job so unload can't interleave (`backends/mlx_backend.py:270-274`). +- **Serial generation queue** (`services/task_queue.py`): distinguishes cancel-while-queued vs cancel-while-running (`:105-117`), keeps background-task references against GC (`:31-36`), force-fails orphaned DB rows when the worker dies mid-write (`:69-93`), with matching route-layer recovery (`routes/generations.py:245-259`) and real behavioral tests. +- **Shared backend plumbing** (`backends/base.py`): centralized HF cache checks, device detection, CUDA compute-capability validation with actionable errors, and a `model_load_progress()` context manager all 7 engines use (`:234-295`). +- **Lifespan care**: FastAPI + FastMCP lifespans composed with an explicit LIFO-teardown comment (`app.py:134-151`); startup marks stale `generating` rows failed after a crash (`app.py:295-303`). +- **Hardening details**: RFC 5987 filenames (`app.py:112-120`), SPA path-traversal guard (`app.py:219-225`), upload limits enforced while streaming (`routes/generations.py:422-434`), documented float64→float32 patches for upstream chatterbox bugs (`backends/base.py:298-332`). +- Comments cite issues and explain decisions (no-Alembic rationale in `database/migrations.py:1-18`). + +### Weaknesses + +- **Broken test file committed**: `tests/test_profile_duplicate_names.py` imported the pre-refactor layout and killed collection — nobody ran the suite green since the routes refactor. *(fixed, see follow-up)* +- **No `conftest.py`**; six files did per-file `sys.path` hacks, making tests order-dependent. *(fixed)* +- **`test_cors.py` tested a hand-copied mirror** of the origin list that had already drifted from `app.py` (missing `http://tauri.localhost`). *(fixed — now uses the real `create_app()`)* +- **API routes untested**: no TestClient coverage of `/generate`, `/profiles`, `/stories`; the retry/regenerate/cancel state machine in `routes/generations.py` has zero coverage. +- **~1,000 ruff violations** against its own config. *(auto-fixed ~900; remainder baselined in `pyproject.toml` — see follow-up)* +- **Silent clone degradation**: `mlx_backend.py:252-257` catches any generate exception and retries without the voice prompt — the user gets the default voice and the generation records as successful. **Open.** +- **Inconsistent load-race protection**: Chatterbox/Hume double-check with an `asyncio.Lock`; Kokoro (`kokoro_backend.py:156-160`) and LuxTTS (`luxtts_backend.py:65`) don't. `get_stt_backend` (`backends/__init__.py:739-753`) unlocked while TTS/LLM factories are locked. **Open.** +- **SQLite has no WAL / busy_timeout** (`database/session.py:37-40`) while the code works around "SQLite lock racing" in two places. Backlog PRs #666/#667 add exactly this. **Open.** +- **Registry didn't deliver on its promise**: `backends/__init__.py` still has five per-engine if/elif chains (`:513-656`); adding an engine touches at least four places. **Open.** +- Assorted: 46 deprecated `datetime.utcnow()` calls, 14 Pydantic v1-style `class Config` blocks, pervasive defensive `getattr()` on own ORM columns, `except Exception: pass` on corrupt effects-chain JSON (`routes/generations.py:122-124`). + +--- + +## React frontend — B + +`web/` is not a duplicate: 182 LOC of platform adapters injecting a `Platform` interface via `PlatformProvider` (`app/src/platform/PlatformContext.tsx`), with the entire app shared through a Vite alias. Best structural decision in the frontend. + +### Strengths + +- **Typing discipline**: strict mode + `noUnusedLocals/Parameters`; exactly one `as any` in ~29k LOC (`AudioPlayer.tsx:183`). +- **State architecture**: server state in 19 react-query hooks, client state in 8 small zustand stores; persistence partialized so audio drafts never hit localStorage (`uiStore.ts:100-103`). +- **`useAudioRecording.ts`** is genuinely strong concurrency work: generation counters for stale `getUserMedia` results, coalesced in-flight acquisition, deferred release during capture, with comments explaining why. +- Every mutation has an `onError` toast; `ProfileForm` does client-side rollback with rollback-failure reporting (`:709-739`). + +### Weaknesses + +- **Zero tests, zero test tooling** — riskiest for the pixel-math in `StoryTrackEditor.tsx` trim/zoom (`:595-640`, `:1035-1065`), the 4-branch `onSubmit` state machine in `ProfileForm.tsx` (`:491-754`), and Web Audio scheduling in `useStoryPlayback.ts`. **Open.** +- **1,748 lines of dead generated API client** (`app/src/lib/api/{core,models,schemas,services}`) imported by nothing, with stale types (missing `engine`, `personality`, `effects_chain`) sitting beside the real hand-written client. Delete it or commit to codegen. **Open.** +- **`client.ts` DRY violations**: the same `if (!response.ok)` multipart block copy-pasted ~11 times; `useStories.ts:79-229` is 12 near-identical mutation hooks. **Open.** +- **35 `console.log` calls in production paths** (model download, story store, SSE lifecycle) despite an unused `debug.ts` gate. **Open.** +- **i18n two-thirds done**: 6 locales ship but ~24 feature components are hardcoded English, including all of `StoryTrackEditor`, `AudioPlayer`, `DictateWindow`. **Open.** +- **Effect hygiene**: `ClipWaveform` recreates WaveSurfer (re-fetching audio) on every zoom step; `getEffectiveDuration` recreated per render defeats the memos that list it; `App.tsx:218-225` failure timeout never cancelled; three dead `eslint-disable` comments in a biome repo. **Open.** +- **God components**: `ProfileForm.tsx` (1,317), `ModelManagement.tsx` (1,102 — embeds a 75-line SSE migration workflow in a JSX `onClick`, categorizes models by name-prefix string match), `HistoryTable.tsx` (915), `CapturesTab.tsx` (909). `StoryTrackEditor.tsx` (1,531) is coherent but is three components in one file. **Open.** + +--- + +## Tauri/Rust shell — B− + +Two codebases live here. The dictation/paste path is disciplined, documented, RAII-guarded FFI; the server-lifecycle half of `main.rs` and both audio modules are a tier below. + +### Strengths + +- **`clipboard.rs`**: full-fidelity multi-format snapshot/restore on both platforms, RAII guards throughout, correct HGLOBAL ownership semantics (`:548-563`). Conditional restore keyed on `NSPasteboard.changeCount` / `GetClipboardSequenceNumber` (`main.rs:1411-1421`) correctly yields to clipboard managers writing mid-paste. +- **`keyboard_layout.rs`**: Dvorak/AZERTY Cmd+V via `UCKeyTranslate` + input-source-change observer; hot path reads one `AtomicU16`. +- **`input_monitoring.rs:42-58`**: declares `IOHIDCheckAccess` as `c_uint` not `bool` with a comment explaining the UB risk — exemplary FFI care. +- **`focus_capture.rs`**: macOS 14 cooperative-activation handled with a cached `respondsToSelector:` probe; refused activation aborts before clobbering the clipboard (`:325-331`). +- **Hotkey lifecycle**: single dispatcher thread serializes Start/Stop/Restart; shutdown is flag + join (`hotkey_monitor.rs:102-136`); focus snapshotted before any window mutation. + +### Weaknesses + +- **`main.rs` god-file** (1,828 lines): NSPanel surgery + ~660 lines of sidecar process management + paste pipeline + app builder; `start_server` is 560 lines with the dev-mode fallback copy-pasted three times. `lib.rs:1` declares a vestigial duplicate `audio_capture` module. **Open.** +- **Blocking calls in async commands**: `reqwest::blocking::Client` inside async `stop_server` (`main.rs:993-1000`); blocking health checks and `thread::sleep` inside async `start_server`. **Open.** +- **`println!` logging with per-packet spam**: `audio_output.rs:217` logs every decoded packet; no `log`/`tracing` facade anywhere. **Open.** +- **`audio_output.rs` bugs**: stop-flag set-then-reset race a 10 ms poll can miss (`:105-108` vs `:413-419`); "multi-device" playback is actually serial (`:111-117`); `resample` is sample-and-hold while the comment claims linear interpolation (`:268`). **Open.** +- **`audio_capture/` copy-paste divergence**: `samples_to_wav` triplicated verbatim; all three `stop_capture`s do `let _ = tx.send(())` on a *tokio* mpsc sender — the future is dropped and the signal never sent (works only because dropping the sender closes the channel); Windows stop is a fixed 500 ms sleep that can truncate the tail; `linux.rs:113` calls `env::set_var` from a spawned thread (the setenv data race that is `unsafe` in Rust 2024) and never unsets it; Windows/macOS assume Float32 sample format unchecked. **Open.** +- **Plausible deadlock**: sync `enable_hotkey`/`disable_hotkey` run on the main thread and join the dispatcher while holding the state lock (`hotkey_monitor.rs:108-111`); the dispatcher's effect path does main-thread round-trips (`:252-260`). Marking the commands async closes it. **Open.** +- **`start_server` TOCTOU on itself**: `child.is_some()` guard and store separated by multiple awaits — two concurrent invocations double-spawn. **Open.** + +--- + +## CI / repo hygiene — B− + +### Strengths + +- **Release pipeline** (`release.yml`, 404 lines): 3-platform matrix, changelog extraction, Tauri updater JSON, DMG notarization + staple with `spctl` verification, SHA-pinned third-party action with written rationale, checksummed CUDA/ROCm sidecar archives with torch-compat metadata. +- **Developer docs**: `docs/content/docs/developer/tts-engines.mdx` (703 lines) with a mandatory dependency-research phase built from real scar tissue. 4,022 lines of MDX across 14 files. +- **justfile**: fully cross-platform including native PowerShell, GPU auto-detection for CUDA/ROCm torch indexes, venv guards. +- **Security claims check out**: server binds `127.0.0.1` by default, CORS is a real allowlist with a test, Docker binds host port to `127.0.0.1`. +- Tracked-file state clean: 671 files, no venvs/worktrees/data tracked. + +### Weaknesses + +- **PR gate was ~15 effective lines**: typecheck + web build. No biome, no ruff, no pytest, no cargo check — the entire 27-file backend suite ungated. This is how 100+ unverifiable PRs pile up. *(fixed, see follow-up)* +- **`.gitignore` tail was UTF-16LE** — the `.claude/settings.local.json` pattern was garbage bytes git can't parse; the whole file was CRLF. *(fixed)* +- **Unpinned git dependencies**: `linacodec` and `Zipvoice` (`backend/requirements.txt:21-22`, third-party personal account) and `Qwen3-TTS` (justfile, Dockerfile) have no commit pins — a force-push upstream silently changes what every release ships. The single biggest supply-chain exposure. **Open.** +- **Stale contributor docs**: `CONTRIBUTING.md` references a nonexistent autoupdater doc, Black instead of ruff, Python 3.11+, `com.voicebox.app`, and the pre-refactor layout. `SECURITY.md` says 0.3.x is the supported version. **Open.** +- **Vestigial root `requirements.txt`**: 9 unpinned lines including unused `torchvision`; nothing references it; it exists to mislead. **Open.** +- **`backend/pyproject.toml` version stuck at 0.2.3** — missing from `.bumpversion.cfg`. **Open.** +- `build-windows.yml` uses floating `tauri-action@v0` and a hardcoded stale release body. Release `bun install` isn't `--frozen-lockfile` while CI is. Two venvs (`backend/venv` + `backend/.venv`) on dev machines. **Open.** + +--- + +## Priorities + +1. ~~CI: biome + ruff + pytest + cargo check on every PR~~ *(done 2026-07-26)* +2. ~~Pin the three git deps to commit SHAs~~ *(done 2026-07-26)* +3. ~~Fix the silent voice-prompt fallback in `mlx_backend.py`~~ *(done 2026-07-26)* +4. ~~Delete the dead generated API client (1,748 LOC)~~ *(done 2026-07-26)* +5. `audio_capture`/`audio_output` rigor pass (dropped futures, stop races, `set_var`) +6. ~~SQLite WAL + busy_timeout~~ *(done 2026-07-26)* +7. ~~Refresh `CONTRIBUTING.md` / `SECURITY.md`; delete root `requirements.txt`~~ *(done 2026-07-26)* +8. Frontend test tooling + first unit tests for the pure pixel-math functions *(in progress)* +9. Burn down the ruff/biome baselines (tracked in `backend/pyproject.toml` and `biome.jsonc`) + +--- + +## Follow-up — fixed 2026-07-26 + +Same-day fixes landed on `0.6-dictation-fixes`: + +- **CI** (`.github/workflows/ci.yml`): added biome lint to the frontend job, a `backend-quality` job (macOS arm64: `just setup-python`, `ruff check`, `pytest`), a `rust-quality` job (`cargo check` with stub sidecars), and concurrency cancellation. +- **Tests green**: fixed `test_profile_duplicate_names.py` imports, added `tests/conftest.py` (kills the order-dependence), rewrote `test_cors.py` against the real `create_app()` factory (now covers `http://tauri.localhost`), fixed the stale 1,000-byte simulation in `test_progress.py` (tracker's 1 MB reporting threshold). Suite: 134 passed, 2 skipped. +- **Ruff green**: ~900 violations auto-fixed; remaining 151 baselined in `pyproject.toml` with per-rule counts and per-file carve-outs for deliberate env-before-import patterns. +- **Biome green**: `biome.json` → `biome.jsonc` with the failing rules baselined at `warn` (annotated against issue #421); scanner ignores `.worktrees/` so local worktrees no longer break `bun run lint`. +- **Real bug fixed**: `pyi_rth_numpy_compat.py` referenced `_t` before binding, so the `NameError` was swallowed by `except Exception: pass` and the torch `from_numpy` patch silently never applied in frozen builds. +- **`.gitignore`**: rewritten as UTF-8/LF; added `.worktrees/`, `.hermes/`, `mlx-test/`. +- **Git deps pinned to commits**: `linacodec@c0ae7c7` and `Zipvoice@381b160` (the commits resolved in the working venv) in `backend/requirements.txt`; `Qwen3-TTS@022e286` (upstream HEAD, verified as the installed 0.1.1) in the justfile and Dockerfile. +- **Dead generated API client deleted** (~1,750 LOC): `app/src/lib/api/{core,models,schemas,services,index.ts}`, plus its generator (`scripts/generate-api.sh`, the `generate:api` script, the `just generate-api` recipe) and the doc sections describing the codegen workflow. The client is hand-written in `client.ts`/`types.ts`; docs now say so. +- **Voice-prompt fallback removed** (`mlx_backend.py`): a cloning failure now fails the generation with the real error instead of silently retrying with the model's default voice; the empty-audio-as-success path and the silent no-`ref_audio`-parameter fallback raise too. +- **Load races closed**: Kokoro and LuxTTS `load_model` now double-check under an `asyncio.Lock` (same pattern as Chatterbox); `get_stt_backend` got the lock the TTS/LLM factories already had. +- **SQLite hardened** (`database/session.py`): WAL journal mode + `synchronous=NORMAL` via connect listener, 30 s busy timeout via `connect_args` — mitigates the lock racing the orphan-recovery machinery works around. +- **Hotkey deadlock closed**: `enable_hotkey` / `disable_hotkey` / `update_chord_bindings` are now async commands, so joining the dispatcher no longer happens on the main thread the dispatcher may be waiting on. +- **Docs refreshed**: `CONTRIBUTING.md` (ruff not Black, Python 3.12, `sh.voicebox.app`, real backend layout, real testing story, fixed autoupdater link), `SECURITY.md` (0.5.x supported; describes actual CI enforcement), root `requirements.txt` deleted (two stale references repointed at `backend/requirements.txt`), `backend/pyproject.toml` version synced to 0.5.0 and added to `.bumpversion.cfg`.