--- name: Area A — storage error inventory and proposed taxonomy type: survey tags: [engine, residuals, area-a, storage, errors, survey] description: Pre-migration survey for engine-slop residuals Area A. Catalogues every MagnotiaError::StorageError construction site in the storage crate, groups by failure mode, proposes a typed lumotia_storage::Error enum with a serializable MagnotiaError::Storage variant on top, and flags ambiguities + migration risks. No code changes yet. status: draft --- ## TL;DR There is no `StorageError` enum to extend. The storage crate has zero local error type and produces failures directly as `MagnotiaError::StorageError(String)` via 78 `format!()` sites across `database.rs` (72) and `migrations.rs` (6). The residuals plan's framing of "Storage-layer typed errors (~25 sites)" undercounted — actual count is 3× higher — because the plan came from a residuals pass that did not survey the storage crate. The Tauri command layer stringifies every error before crossing into the frontend (`Result` everywhere via `.map_err(|e| e.to_string())?`), so `MagnotiaError`'s `Serialize` impl is presently unused at the FE/BE boundary. Area E is where that ossifies; Area A's job is to give **backend** code something to branch on — not to fix the frontend. Proposed: introduce a new `lumotia_storage::Error` enum in the storage crate, replace the `String` tail of `MagnotiaError::StorageError(String)` with a structured `MagnotiaError::Storage { kind, operation, detail }` variant, and wire the conversion through a `From for MagnotiaError` impl that flattens the structured error into a serde-friendly shape. Backend code can pattern-match on `storage::Error`; the frontend keeps receiving (slightly nicer) strings until Area E. ## Inventory ### Headline numbers | File | Sites | What they wrap | |---|---|---| | `crates/storage/src/database.rs` | 72 | sqlx CRUD operations, pre-condition checks, post-update invariant checks | | `crates/storage/src/migrations.rs` | 6 | Schema version table creation/query, per-migration tx/exec/record/commit | | `crates/storage/src/file_storage.rs` | 0 | Uses `From` via `?` — already typed | | `crates/storage/src/lib.rs` | 0 | Re-exports only | | **Total** | **78** | | ### Storage crate context - Uses `lumotia_core::error::{MagnotiaError, Result}` directly — no local error type. - No `From` impl exists, which is why every sqlx call has an explicit `.map_err(...)`. - `From for MagnotiaError` does exist (in `crates/core/src/error.rs`), so directory-creation paths use `?` cleanly. ### Grouped by failure mode, not by file #### Bucket 1 — Database connection / init / pragma (3 sites) | Site | Current message prefix | |---|---| | `database.rs:22` | `Database connect failed: {e}` | | `database.rs:27` | `foreign_keys pragma failed: {e}` | | `database.rs:50` | `Read-only connect failed: {e}` | Source: `sqlx::Error`. **Backend interest:** could plausibly want to distinguish read-only fallback failure from primary connect failure for telemetry, but no code currently branches on this. #### Bucket 2 — Migration framework (6 sites, all in migrations.rs) | Site | Step | Has version? | |---|---|---| | `migrations.rs:557` | `schema_version_table_create` | no | | `migrations.rs:563` | `schema_version_query` | no | | `migrations.rs:570` | `tx_begin` | yes | | `migrations.rs:578` | `apply` | yes | | `migrations.rs:588` | `record_version` | yes | | `migrations.rs:592` | `commit` | yes | Source: `sqlx::Error`. **Backend interest:** the migration retry-poison test (`migrations.rs:1082`) wants to assert these specifically, which already implies the type-system benefit of a dedicated variant. #### Bucket 3 — Query execution (sqlx wrap) (~64 sites) Every CRUD operation — INSERT, SELECT, UPDATE, DELETE — wrapped with `.map_err(|e| MagnotiaError::StorageError(format!(" failed: {e}")))`. A non-exhaustive sample (the full 64 share the same shape): - `database.rs:113` — Insert transcript - `database.rs:124` — Get transcript - `database.rs:147` — List transcripts - `database.rs:157` — Count transcripts - `database.rs:186/197/208` — Update transcript (three branches in one function) - `database.rs:256` — update_transcript_meta - `database.rs:268` — Delete transcript - `database.rs:292` — FTS search - `database.rs:330/342/355` — task CRUD - `database.rs:393/414` — task update / energy - `database.rs:433/446/463/470/479/494` — subtask CRUD + auto-complete-parent transaction sub-steps - `database.rs:506-553` — complete_task / uncomplete_task transaction sub-steps - `database.rs:610/621/633` — analytics queries - `database.rs:666/683/700/720/745/761` — implementation_rule CRUD - `database.rs:774/783` — setting get/set - `database.rs:969-1122` — profile + profile_term CRUD - `database.rs:1156/1188/1200` — error_log - `database.rs:1305/1342` — feedback_log Source: `sqlx::Error`. **Backend interest:** programmatic branching on sqlx error kind (e.g., `RowNotFound`, `Database` with constraint name, connection drops) is meaningful at this layer. The operation label is purely descriptive — not a useful axis for an enum variant. #### Bucket 4 — Invariant violation (post-update rows-affected check) (3 sites) | Site | Entity | Condition | |---|---|---| | `database.rs:259` | transcript | `update_transcript_meta` UPDATE affected 0 rows | | `database.rs:396` | task | `update_task` UPDATE affected 0 rows | | `database.rs:417` | task | `set_task_energy` UPDATE affected 0 rows | These are **logically distinct from query failure** — the SQL succeeded; the row simply isn't there. Today they're conflated with sqlx errors in `StorageError(String)`. Worth typing because (a) callers might want to surface a different message for "not found" vs "DB error", and (b) it removes ambiguity in logs. **Subtlety:** there's no guard against the race "row existed at check time, was deleted before update" — these errors fire correctly in that race but the caller has no way to distinguish that from a stale `id` passed in by the user. #### Bucket 5 — Pre-condition validation (1 site) | Site | Reason | |---|---| | `database.rs:85` | `insert_transcript` called with a `profile_id` that doesn't exist in `profiles` table | Today: `return Err(MagnotiaError::StorageError(format!("Insert transcript failed: unknown profile id '{}'", params.profile_id)))`. This is **not a sqlx failure**. It's an integrity check we perform manually before issuing the INSERT. Properly speaking, this is "invalid reference" — distinct from "query failed" and from "row not found". ### Out-of-scope (already typed at MagnotiaError level) - `std::fs::create_dir_all(parent)?` at `database.rs:11` — propagates through `From for MagnotiaError` to `MagnotiaError::Io { kind, message, raw_os_error }`. Already structured. ## Proposed taxonomy ### Where it lives **Option A (recommended): new `lumotia_storage::Error` enum in the storage crate.** ```rust // crates/storage/src/error.rs use std::borrow::Cow; use thiserror::Error; #[derive(Debug, Error)] pub enum Error { #[error("database open failed during {operation}: {source}")] DatabaseOpen { operation: OpenOp, #[source] source: sqlx::Error, }, #[error("migration step {step} (version {version:?}) failed: {source}")] Migration { version: Option, step: MigrationStep, #[source] source: sqlx::Error, }, #[error("query failed during {operation}: {source}")] Query { operation: Cow<'static, str>, #[source] source: sqlx::Error, }, #[error("{entity} not found: '{key}'")] NotFound { entity: Entity, key: String, }, #[error("invalid {entity} reference: {reason}")] InvalidReference { entity: Entity, reason: Cow<'static, str>, }, } #[derive(Debug, Clone, Copy)] pub enum OpenOp { Connect, ReadOnlyConnect, ForeignKeysPragma } #[derive(Debug, Clone, Copy)] pub enum MigrationStep { SchemaVersionTableCreate, SchemaVersionQuery, TxBegin, Apply, RecordVersion, Commit, } #[derive(Debug, Clone, Copy)] pub enum Entity { Transcript, Task, Subtask, Profile, ProfileTerm, ImplementationRule, Setting, ErrorLogRow, } pub type Result = std::result::Result; ``` **Why option A:** - The taxonomy lives where the failures originate. Storage owns its own error vocabulary. - The storage crate can be tested in isolation without `lumotia_core` understanding its internals. - `From for MagnotiaError` keeps propagation automatic via `?`. - Sets up Area E to selectively expose storage-error kinds to the frontend without a second migration of call sites. **Option B (rejected): structured variant inside `MagnotiaError` directly.** Mentioned for completeness: ```rust // In crates/core/src/error.rs pub enum MagnotiaError { // ... Storage { kind: StorageKind, operation: String, detail: String, }, // ... } ``` - Less ceremony, one file changes. - But couples the storage taxonomy into `lumotia_core`, which is a leaky abstraction. - Harder to extend later when, say, we add a vocabulary crate (per D1 in the architecture spec) that also wants typed errors — every crate ends up dumping its enum into `MagnotiaError`. ### How it crosses the MagnotiaError boundary `sqlx::Error` does not implement `Serialize`. `MagnotiaError` does — that contract has to be preserved for any future Tauri serialisation path (Area E). Proposed: flatten on the boundary. ```rust // In crates/core/src/error.rs #[derive(Debug, thiserror::Error, Serialize)] pub enum MagnotiaError { // ... existing variants unchanged ... #[error("storage error: {detail}")] Storage { kind: StorageKind, // serializable enum: Open | Migration | Query | NotFound | InvalidReference operation: String, // human-readable, e.g. "insert_transcript" or "migration_apply_v7" detail: String, // current Display output of storage::Error }, } #[derive(Debug, Clone, Copy, Serialize)] #[serde(rename_all = "snake_case")] pub enum StorageKind { DatabaseOpen, Migration, Query, NotFound, InvalidReference, } impl From for MagnotiaError { fn from(e: lumotia_storage::Error) -> Self { let kind = match &e { lumotia_storage::Error::DatabaseOpen { .. } => StorageKind::DatabaseOpen, lumotia_storage::Error::Migration { .. } => StorageKind::Migration, lumotia_storage::Error::Query { .. } => StorageKind::Query, lumotia_storage::Error::NotFound { .. } => StorageKind::NotFound, lumotia_storage::Error::InvalidReference { .. } => StorageKind::InvalidReference, }; let operation = e.operation_label().into_owned(); let detail = e.to_string(); MagnotiaError::Storage { kind, operation, detail } } } ``` `operation_label()` is a helper on `storage::Error` returning a `Cow<'static, str>` from the inner enum data. Backend code that wants programmatic recovery downcasts (or rather, the storage call signatures return `Result<_, lumotia_storage::Error>` directly — only public Tauri-facing wrappers eat the `From` conversion). ### Function-signature decision The storage crate has 70+ public functions returning `lumotia_core::Result<...>`. Two options: 1. **Change all to `lumotia_storage::Result<...>`** and let `?` in callers do the From conversion. Pros: backend code that wants typed errors gets them. Cons: ~70 signatures to touch, and any backend code that wanted the structured error has to import `lumotia_storage::Error`. 2. **Keep public signatures as `lumotia_core::Result<...>`, do the conversion at the storage-crate boundary** via an internal `Result = std::result::Result` for internal helpers. Public functions then end with `? `(or `.map_err(Into::into))`. Pros: caller signatures unchanged. Cons: callers wanting to pattern-match on storage failure type have nothing to match against (back to square one). **Recommended: option 1.** It's the more honest signature. ~70 public functions get changed, but it's mechanical: `Result` → `lumotia_storage::Result` and the body's `MagnotiaError::StorageError(format!(...))` is what we're replacing anyway. Callers (Tauri commands) that currently `.map_err(|e| e.to_string())?` keep working because the storage error From-converts into MagnotiaError, which is what Display'd into the string they're already producing. This also means **Area E later only has to delete `.map_err(|e| e.to_string())?` and change return types** to receive structured errors — no second pass over the storage call sites. ## Sites → proposed variant For the migration pass itself. Format: `:` → variant + fields. ### → `Error::DatabaseOpen` - `database.rs:22` → `DatabaseOpen { operation: OpenOp::Connect, source }` - `database.rs:27` → `DatabaseOpen { operation: OpenOp::ForeignKeysPragma, source }` - `database.rs:50` → `DatabaseOpen { operation: OpenOp::ReadOnlyConnect, source }` ### → `Error::Migration` - `migrations.rs:557` → `Migration { version: None, step: SchemaVersionTableCreate, source }` - `migrations.rs:563` → `Migration { version: None, step: SchemaVersionQuery, source }` - `migrations.rs:570` → `Migration { version: Some(v), step: TxBegin, source }` - `migrations.rs:578` → `Migration { version: Some(v), step: Apply, source }` - `migrations.rs:588` → `Migration { version: Some(v), step: RecordVersion, source }` - `migrations.rs:592` → `Migration { version: Some(v), step: Commit, source }` ### → `Error::Query` The 64 sites in Bucket 3. Operation labels match the current message stem in snake_case: - `Insert transcript failed` → `Query { operation: "insert_transcript".into(), source }` - `Get transcript failed` → `Query { operation: "get_transcript".into(), source }` - `List transcripts failed` → `Query { operation: "list_transcripts".into(), source }` - ... (mechanical for the rest) ### → `Error::NotFound` - `database.rs:259` → `NotFound { entity: Entity::Transcript, key: id.to_string() }` - `database.rs:396` → `NotFound { entity: Entity::Task, key: id.to_string() }` - `database.rs:417` → `NotFound { entity: Entity::Task, key: id.to_string() }` ### → `Error::InvalidReference` - `database.rs:85` → `InvalidReference { entity: Entity::Profile, reason: format!("unknown profile id '{}'", params.profile_id).into() }` ## Ambiguous cases / decisions needed 1. **Migration "Schema version query" — Migration step or Query?** Currently classified as `MigrationStep::SchemaVersionQuery` (within `Error::Migration`) because it's logically part of the migration framework even though it executes a SELECT. Alternative: treat it as a `Query { operation: "schema_version_query" }`. **Recommendation:** keep under Migration. The migration test (`migrations.rs:1082`) asserts on migration-level failures and would benefit from the unified variant. Decision needed before implementation. 2. **Sub-step transactions inside CRUD operations.** `complete_subtask_and_check_parent` (database.rs:453) issues four sqlx calls inside one transaction. Each currently has its own `.map_err(...)` ("Begin transaction failed", "Complete subtask failed", "Get parent_task_id failed", "Count pending subtasks failed", "Auto-complete parent failed", "Commit transaction failed"). Two ways to type: - One `Query` variant per call, six unique operation labels — preserves current granularity. - One `Query { operation: "complete_subtask_and_check_parent::" }` style, with step in the label — slightly more readable from the FE but loses some debuggability. **Recommendation:** preserve current granularity (option a). Cheap and matches existing logs. 3. **`Entity` enum cardinality.** Proposal includes 8 entities. Realistic check: do we need all 8? Going by NotFound + InvalidReference sites, only `Transcript`, `Task`, and `Profile` actually appear. The other 5 (Subtask, ProfileTerm, ImplementationRule, Setting, ErrorLogRow) are speculative. **Recommendation:** start with the 3 we need and add more only when a `NotFound` for that entity actually materialises. Avoid taxonomy theatre. 4. **`Cow<'static, str>` vs `&'static str` vs `String` for `Query::operation`.** With ~64 unique labels, we have a choice: - All literal `&'static str` — most efficient, but requires every call site to use a string literal (which it would anyway). - `Cow<'static, str>` — accommodates future dynamic operation names without a heap allocation for the common case. - `String` — flexible, one alloc per error. **Recommendation:** `Cow<'static, str>`. The common case is a literal; the escape hatch is free. 5. **Serialize implementation strategy.** Three options for `storage::Error`: - Derive Serialize and `#[serde(skip)]` the `sqlx::Error` source. Loses source detail in serialised form. - Custom Serialize impl that stringifies the source. - Don't derive Serialize — flatten only at the `MagnotiaError` boundary as proposed above. **Recommendation:** the third. Keeps the storage crate's API surface backend-only; serialisation concerns live in `lumotia_core`. Already reflected in the proposal. 6. **Public re-exports from storage crate.** Need to decide: do we re-export `Error`, `Result`, `OpenOp`, `MigrationStep`, `Entity`, `StorageKind` (or its storage-side equivalent) from `crates/storage/src/lib.rs`? Yes — backend code (Tauri commands, future test code) will want to pattern-match. Suggest a `pub mod error;` + `pub use error::{Error, Result, OpenOp, MigrationStep, Entity};` block. ## Migration risk notes ### Visible behaviour changes - **Display messages change.** Current "Insert transcript failed: unique constraint violated" becomes "query failed during insert_transcript: unique constraint violated". Functionally equivalent but slightly different. Logs and the Tauri error toast strings will reflect the new format. Worth eyeballing on the Settings → Profiles page, Files page, and Tasks page once landed. - **Tauri command return shape unchanged.** All `Result` signatures stay the same. `.map_err(|e| e.to_string())?` keeps working because `From for MagnotiaError` produces a Display-able `MagnotiaError`. ### Compile-time gotchas - **`?` operator semantics depend on which `Result` is in scope.** If function signature returns `lumotia_core::Result` and body uses `lumotia_storage::Result` internally, every helper boundary needs an explicit `Into::into` or a `?` cascade with `From` plumbed. Doable but easy to miss — recommend going function-by-function, not file-by-file. - **Test ergonomics.** Storage crate tests at `crates/storage/src/migrations.rs:1027+` currently match on `MagnotiaError::StorageError` patterns. They'll need updating to match on `storage::Error::Migration { step: MigrationStep::Apply, .. }` style. Roughly 5–10 test assertions, all bounded to that file. ### Scope creep guards - **Do not retitle error messages.** Goal is to preserve the current Display output verbatim where possible, with the operation label carrying the same information as the current message stem. Wording changes belong in a separate pass. - **Do not change `MagnotiaError::StorageError(String)` → `MagnotiaError::Storage { ... }` in one PR alongside the storage-crate work.** Two commits: 1. Add `lumotia_storage::Error`, `From for MagnotiaError`, keep the old `StorageError(String)` variant temporarily as a no-op (compile-only). 2. Remove the old `StorageError(String)` variant; all consumers now go through `Storage { ... }`. Two commits means we can verify the type-system migration is sound before deleting the safety net. - **Resist adding `Other(String)`.** The whole point. If a case doesn't fit, surface the ambiguity and decide explicitly. ## Verification plan Once the migration lands: ``` cargo fmt --all -- --check cargo check -p lumotia-storage cargo check -p lumotia-core cargo check --workspace --all-targets cargo test -p lumotia-storage cargo test --workspace --lib rg 'StorageError\(' crates/ src-tauri/src/ # must be zero rg 'Other\(String\)' crates/ src-tauri/src/ # must remain zero rg 'format!\("[A-Z][^"]+failed' crates/storage/src/ # must be zero (catches missed map_errs) ``` The third grep is the structural assertion: there should be no string-formatted "Whatever failed" messages emerging from the storage crate post-migration — every one becomes structured. ## Out of scope (explicit deferrals) - **Area E** — frontend/backend error boundary cleanup. The Tauri commands stay on `Result` for now. Once Area A lands, Area E becomes mechanical: change return types, delete `.map_err(|e| e.to_string())?`, plumb `MagnotiaError::Storage { kind, operation, detail }` into typed frontend handlers. - **Wording rewrites of user-visible error toasts.** The current strings will read slightly differently post-migration. Not changing copy as part of this; copy review is its own task. - **`From for storage::Error`.** Tempting because it would eliminate the `.map_err(...)` boilerplate, but it would either lose the operation label (single `From` impl can't know which operation it's wrapping) or require a thin helper macro. Leaving the explicit `.map_err(..., op: "insert_transcript")` per site for now. Macro investigation can be a tiny follow-up. - **`Other(String)` introduction.** Already absent. Stays absent.