Lumotia/docs/superpowers/plans/2026-05-12-area-a-storage-errors-survey.md

---
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 magnotia_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<T, String>` 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 `magnotia_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<storage::Error> 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<io::Error>` via `?` — already typed |
| `crates/storage/src/lib.rs` | 0 | Re-exports only |
| **Total** | **78** | |

### Storage crate context

- Uses `magnotia_core::error::{MagnotiaError, Result}` directly — no local error type.
- No `From<sqlx::Error>` impl exists, which is why every sqlx call has an explicit `.map_err(...)`.
- `From<std::io::Error> 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!("<Operation name> 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<std::io::Error> for MagnotiaError` to `MagnotiaError::Io { kind, message, raw_os_error }`. Already structured.

## Proposed taxonomy

### Where it lives

**Option A (recommended): new `magnotia_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<i64>,
        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<T> = std::result::Result<T, Error>;
```

**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 `magnotia_core` understanding its internals.
- `From<storage::Error> 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 `magnotia_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<magnotia_storage::Error> for MagnotiaError {
    fn from(e: magnotia_storage::Error) -> Self {
        let kind = match &e {
            magnotia_storage::Error::DatabaseOpen { .. } => StorageKind::DatabaseOpen,
            magnotia_storage::Error::Migration { .. } => StorageKind::Migration,
            magnotia_storage::Error::Query { .. } => StorageKind::Query,
            magnotia_storage::Error::NotFound { .. } => StorageKind::NotFound,
            magnotia_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<_, magnotia_storage::Error>` directly — only public Tauri-facing wrappers eat the `From` conversion).

### Function-signature decision

The storage crate has 70+ public functions returning `magnotia_core::Result<...>`. Two options:

1. **Change all to `magnotia_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 `magnotia_storage::Error`.

2. **Keep public signatures as `magnotia_core::Result<...>`, do the conversion at the storage-crate boundary** via an internal `Result<T> = std::result::Result<T, magnotia_storage::Error>` 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<T>` → `magnotia_storage::Result<T>` 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: `<file>:<line>` → 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::<step>" }` 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 `magnotia_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<T, String>` signatures stay the same. `.map_err(|e| e.to_string())?` keeps working because `From<storage::Error> for MagnotiaError` produces a Display-able `MagnotiaError`.

### Compile-time gotchas

- **`?` operator semantics depend on which `Result` is in scope.** If function signature returns `magnotia_core::Result<T>` and body uses `magnotia_storage::Result<T>` 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 `magnotia_storage::Error`, `From<storage::Error> 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 magnotia-storage
cargo check -p magnotia-core
cargo check --workspace --all-targets
cargo test -p magnotia-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<T, String>` 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<sqlx::Error> 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.