agent: engine slop pass — DSP, typed errors, regex parsing, tracing, audit fixes
External code review on 2026-05-12 rated the codebase 4/10 across audio DSP, error typing, JS injection, env-var safety, ALSA parsing, and async logging. This commit lands the prognosis-level fixes plus three audit follow-ups. Audio/DSP: - StreamingResampler/rubato confirmed in the live capture path - regression test at 12 kHz (rms < 0.01, ~40 dB) catches naive decimation - near-Nyquist test at 9 kHz (rms < 0.05, ~26 dB) exercises transition band Core errors: - Other(String) removed; ProviderNotRegistered introduced - Io variant restructured as struct with kind/message/raw_os_error - FileNotFound display quotes paths - Configuration variant removed (unused) Core types: - ModelId, EngineName backed by Cow<'static, str>; const borrowed ctor - Megabytes::from_gb takes u64 (was f64) - AudioSamples::sample_rate is NonZeroU32; zero-rate defensive branch removed Capture: - /proc/asound/cards parsing rewritten as anchored regex (OnceLock) - regression test covers product names with embedded colons - monitor_pattern_detection test restored alongside the regex test - DEAD_SILENCE_FLOOR promoted to module-level with rationale - DEVICE_VALIDATION_MS, SILENCE_RMS_FLOOR documented with field-observation rationale - RMS validation loop made idiomatic - eprintln! migrated to tracing with structured fields and targets Tauri startup: - unsafe std::env::set_var removed; ensure_x11_on_wayland renamed to warn_if_x11_env_unset_on_wayland (launcher/wrapper owns env-var contract) - DB init + log prune + preferences load collapsed to one block_on - build_preferences_script rewrites JS injection from JSON.parse string to direct object literal plus malformed-JSON guard and unit tests - WebKitGTK microphone auto-grant logs warning at startup - tracing subscriber initialised at top of run() (warn,magnotia=info,... on stderr; honors RUST_LOG); previously eprintln→tracing migration was silent because no subscriber existed Filename counter: - RECORDING_COUNTER uses SeqCst Tests: cargo test --workspace --lib green (322 passed, 0 failed across 10 crates). Three independent audits (original cleanup → Wren → fresh Codex subagent) concur on no critical findings. Deferred to docs/superpowers/plans/2026-05-12-engine-slop-residuals.md: storage-layer typed errors, remaining eprintln→tracing sweep, capture actor-model refactor, property-based DSP testing, frontend/backend error boundary cleanup.
This commit is contained in:
173
docs/superpowers/plans/2026-05-12-engine-slop-residuals.md
Normal file
173
docs/superpowers/plans/2026-05-12-engine-slop-residuals.md
Normal file
@@ -0,0 +1,173 @@
|
||||
---
|
||||
name: Engine slop residuals (post-prognosis pass)
|
||||
type: plan
|
||||
tags: [engine, slop-cleanup, plan, deferred]
|
||||
description: Roadmap for the deferred code-quality items that did not fit in the 2026-05-12 prognosis-fix pass. Each area is scoped for its own focused plan when ready to execute.
|
||||
---
|
||||
|
||||
# Engine slop residuals — post-prognosis pass
|
||||
|
||||
> **For agentic workers:** This is a meta-plan. It catalogues independent residual work areas, each of which should get its own detailed TDD plan when scheduled. Do **not** treat the sections below as a single execution checklist.
|
||||
|
||||
## Context
|
||||
|
||||
On 2026-05-12 an external code review rated the codebase 4/10 and laid out a four-bucket de-sloppifying plan. That pass shipped (uncommitted in working tree at time of writing). It hit the *prognosis-level* items only:
|
||||
|
||||
- DSP: naive decimation removed, `StreamingResampler`/rubato in place, regression tests at 12 kHz (40 dB) + 9 kHz (26 dB).
|
||||
- Core errors: `Other(String)` removed; `Io` is structured; `FileNotFound` quotes paths; `ProviderNotRegistered` introduced.
|
||||
- Core types: `Cow<'static, str>` for `ModelId`/`EngineName`, `NonZeroU32` for `AudioSamples::sample_rate`, `Megabytes::from_gb` takes `u64`.
|
||||
- Capture cleanup: regex-based `/proc/asound/cards` parser (test covers product names with embedded colons), constants documented, RMS sum idiomatic, Codex-review comments removed.
|
||||
- Tauri startup: `unsafe std::env::set_var` removed (logs warning instead); function renamed `warn_if_x11_env_unset_on_wayland`; setup collapsed to single `block_on`; JS injection rewritten (`var p = {...}` direct, no `JSON.parse`); WebKitGTK mic auto-grant logs warning at startup.
|
||||
- Counter: `RECORDING_COUNTER` uses `SeqCst`.
|
||||
- Tracing: subscriber initialised at top of `run()` with sensible default filter, honors `RUST_LOG`, writes to stderr.
|
||||
|
||||
The code-proposal and macro-architecture captures from the same review session went further (10/10 demands and workspace-level critique). Those items are the residuals below.
|
||||
|
||||
## Residual areas
|
||||
|
||||
Each area is independent. They can be tackled in any order, but the suggested order minimises rework.
|
||||
|
||||
### A. Storage-layer typed errors
|
||||
|
||||
**Status:** identified, not started.
|
||||
|
||||
**Scope:** ~25 `StorageError(String)` sites in `crates/storage/src/{database.rs, migrations.rs}`.
|
||||
|
||||
**Why it matters:** The frontend cannot distinguish "schema migration failed" from "FTS search failed" from "transcript not found" — all surface as one stringly-typed variant. This is the same class of bug that motivated removing `Other(String)`.
|
||||
|
||||
**Approach:** Split `StorageError` into a `StorageError` enum (sub-error in its own right) with variants like `Connect { path, source }`, `MigrationFailed { version, source }`, `TransactionFailed { phase, source }`, `QueryFailed { kind, source }`, `NotFound { table, id }`. Wrap `sqlx::Error` (or whatever `rusqlite::Error` is in use) as `#[source]`.
|
||||
|
||||
**Decisions needed:**
|
||||
- Is `sqlx::Error`/`rusqlite::Error` serializable across the Tauri boundary? If not, what is preserved (kind + display)?
|
||||
- Do we keep one flat error type for the crate or nest by domain (`MigrationError`, `TranscriptError`, `TaskError`)?
|
||||
- Backwards-compat for any existing error-handling code in frontend.
|
||||
|
||||
**Files in scope:**
|
||||
- `crates/core/src/error.rs` — add `StorageError` variant if changing top-level shape
|
||||
- `crates/storage/src/error.rs` — new file (likely)
|
||||
- `crates/storage/src/database.rs` — 19 call sites
|
||||
- `crates/storage/src/migrations.rs` — 6 call sites
|
||||
|
||||
**Acceptance:** All `StorageError(format!(...))` calls replaced with typed variants. Frontend can `match` on storage-error kind. `cargo test --workspace` green.
|
||||
|
||||
### B. eprintln → tracing sweep (rest of repo)
|
||||
|
||||
**Status:** identified, not started.
|
||||
|
||||
**Scope:** ~30 `eprintln!` sites in production code paths (excluding tests and tooling binaries — see below).
|
||||
|
||||
**Why it matters:** Now that the subscriber is wired, every `eprintln!` is a missed observability signal. The startup/capture paths are clean; the runtime hot paths are not.
|
||||
|
||||
**Approach:** Mechanical migration, one file at a time, with appropriate target and structured fields.
|
||||
|
||||
**Files in scope (production):**
|
||||
- `src-tauri/src/commands/live.rs` — 8 sites (highest priority — session lifecycle + inference errors)
|
||||
- `src-tauri/src/commands/models.rs` — 5 sites (Whisper warmup)
|
||||
- `src-tauri/src/commands/power.rs` — 4 sites (macOS App Nap)
|
||||
- `src-tauri/src/commands/transcription.rs` — 1 site
|
||||
- `src-tauri/src/commands/diagnostics.rs` — 1 site
|
||||
- `src-tauri/src/commands/tasks.rs` — 1 site
|
||||
- `crates/hotkey/src/linux.rs` — 2 sites
|
||||
|
||||
**Files explicitly out of scope:**
|
||||
- `crates/mcp/src/main.rs` — separate binary, its own stdio protocol; keep eprintln as-is unless that binary gets its own subscriber.
|
||||
- `crates/*/tests/*.rs` — test diagnostic output, `--nocapture` semantics depend on stdio.
|
||||
|
||||
**Decisions needed:**
|
||||
- New tracing target names for `commands::live`, `commands::models`, etc. — extend the default filter in `src-tauri/src/lib.rs::init_tracing`.
|
||||
|
||||
**Acceptance:** No `eprintln!` in `src-tauri/src/commands/` or `crates/hotkey/src/linux.rs`. Default filter shows live-session events at INFO. `cargo test --workspace` green.
|
||||
|
||||
### C. Actor-model refactor for capture + inference
|
||||
|
||||
**Status:** identified, not started. **Largest scope.**
|
||||
|
||||
**Scope:** Replace `Arc<Mutex<Vec<f32>>>` shared-state pattern in `CaptureWorker` (and analogous patterns in live inference) with isolated actor loops + `mpsc` channels.
|
||||
|
||||
**Why it matters:** Reviewer's core architectural critique. Shared mutable state across async boundaries with `std::sync::Mutex` risks executor stalls under contention. Architecturally also makes it impossible to add features like multi-source capture, parallel inference, or recording-while-streaming without lock contention.
|
||||
|
||||
**Approach:** Spawn dedicated tokio tasks for the capture worker and the inference worker. Communicate exclusively via bounded `mpsc` channels. Tauri command handlers hold only channel senders.
|
||||
|
||||
**Decisions needed (before plan):**
|
||||
- Bounded vs unbounded mpsc? Recommend bounded for backpressure visibility.
|
||||
- Channel capacity? (current ad-hoc cap is `AUDIO_CHANNEL_CAPACITY = 32`)
|
||||
- Backpressure policy when capture outpaces inference: drop oldest, drop newest, await consumer?
|
||||
- Worker shutdown protocol: drop-channel-and-await, explicit stop message, or cancellation token?
|
||||
- How are runtime errors (already typed via `CaptureRuntimeError`) propagated through the actor boundary?
|
||||
- Migration plan: parallel implementation behind a feature flag, or in-place replacement?
|
||||
|
||||
**Risk:** This is a substantial refactor. Likely worth a brainstorming session before plan-writing.
|
||||
|
||||
**Files in scope:**
|
||||
- `crates/audio/src/capture.rs` — worker lifecycle rewrite
|
||||
- `src-tauri/src/commands/audio.rs` — command handler refactor
|
||||
- `src-tauri/src/commands/live.rs` — inference worker integration
|
||||
- Likely new files: `crates/audio/src/capture_actor.rs`, `crates/transcription/src/inference_actor.rs`
|
||||
|
||||
**Acceptance:** No `Arc<Mutex<T>>` across `.await` points in capture or inference code paths. Capture and inference are independent tokio tasks. End-to-end recording flow works in dev mode. `cargo test --workspace` green. Manual dogfood pass.
|
||||
|
||||
### D. Property-based DSP testing
|
||||
|
||||
**Status:** identified, not started.
|
||||
|
||||
**Scope:** Add `proptest` (or similar) generators for audio inputs and assert invariants of the resampling and VAD pipelines.
|
||||
|
||||
**Why it matters:** The current DSP tests are point checks at 12 kHz and 9 kHz. The reviewer's 10/10 demand is property-based testing that covers the input space (any frequency, any chunk size, any noise floor) and asserts invariants (output is bounded, energy is bounded, duration is approximately preserved).
|
||||
|
||||
**Approach:** Add `proptest` dev-dependency. Write generators for `(sample_rate, signal_frequency, amplitude, chunk_size, total_secs)` tuples. Assert:
|
||||
- Output samples are finite (no NaN/Inf).
|
||||
- Output peak ≤ input peak + small tolerance (no amplification).
|
||||
- Output duration within ±5% of expected.
|
||||
- Output energy at frequencies above Nyquist of target rate is at least 30 dB below input energy at those frequencies.
|
||||
|
||||
**Decisions needed:**
|
||||
- `proptest` vs `quickcheck`? Recommend `proptest` (more flexible shrinking).
|
||||
- Run as part of standard `cargo test` or as a separate `cargo test --features slow-tests`?
|
||||
|
||||
**Files in scope:**
|
||||
- `crates/audio/Cargo.toml` — add `proptest` to `[dev-dependencies]`
|
||||
- `crates/audio/src/streaming_resample.rs` — extend `#[cfg(test)]` module
|
||||
- Likely also `crates/transcription/src/streaming/rms_vad.rs` — VAD has equally testable invariants
|
||||
|
||||
**Acceptance:** `cargo test -p magnotia-audio` includes property tests. Property tests have run with at least 1000 cases per invariant.
|
||||
|
||||
### E. Frontend/backend error boundary cleanup
|
||||
|
||||
**Status:** partially complete (orchestrator's `ProviderNotRegistered`).
|
||||
|
||||
**Scope:** Audit every Tauri command's return type and ensure errors propagate with full type information across the IPC boundary. Currently many commands return `Result<T, String>` instead of `Result<T, MagnotiaError>`.
|
||||
|
||||
**Why it matters:** Reviewer's frontend-backend boundary critique. The frontend can't `switch` on error kind if the backend serialises errors as strings.
|
||||
|
||||
**Approach:** Inventory all `#[tauri::command]` functions, identify those returning `Result<T, String>`, convert to `Result<T, MagnotiaError>` (or a command-specific error enum that serializes cleanly). Update frontend handlers to use the typed shape.
|
||||
|
||||
**Decisions needed:**
|
||||
- One `MagnotiaError` for all commands, or per-command enums?
|
||||
- How do we surface the new shape to the Svelte side without manually maintaining TypeScript types? (specta? hand-rolled?)
|
||||
|
||||
**Files in scope:**
|
||||
- All files in `src-tauri/src/commands/`
|
||||
- All call sites in `src/lib/` (Svelte)
|
||||
|
||||
**Acceptance:** No `Result<T, String>` in `src-tauri/src/commands/`. Frontend has typed error access. `cargo test --workspace` green; frontend builds.
|
||||
|
||||
## Suggested sequencing
|
||||
|
||||
1. **B (eprintln sweep)** first — mechanical, low-risk, immediate observability win.
|
||||
2. **A (storage typed errors)** next — well-scoped, no architectural change, unblocks E.
|
||||
3. **D (property-based DSP)** in parallel with A — independent surface, can be done by anyone any time.
|
||||
4. **E (FE/BE error boundary)** after A — depends on storage error shape.
|
||||
5. **C (actor model)** last — biggest surface, deserves its own brainstorm + plan + likely its own branch.
|
||||
|
||||
## Cross-cutting deferrals (Phase 10a-blocking)
|
||||
|
||||
Two things are explicitly **not** in these residuals because they belong to higher-level workstreams:
|
||||
|
||||
- **Lumotia rebrand cascade** (Wyrdnote → Lumotia in code, paths, GitHub repo, bundle ID). Hold until A–B done so rename diff doesn't hide regressions.
|
||||
- **Phase 10a QC dogfood** — `docs/superpowers/audits/2026-05-10-phase10a-dogfood-notes.md` is queued. Best done after B at minimum so live-session logs are visible.
|
||||
|
||||
## What this plan does not do
|
||||
|
||||
- No fix recipes for bugs not yet found. Each area's plan should start by reading the current state and writing failing tests against the current behaviour.
|
||||
- No timeline commitments. Sequencing is suggested, not mandatory.
|
||||
- No architectural decisions deferred to "Phase B" earlier are pre-committed here. Each area writes its own brainstorm + plan when scheduled.
|
||||
Reference in New Issue
Block a user