docs(area-a): storage error inventory + proposed typed taxonomy

Pre-migration survey for engine-slop residuals Area A. Catalogues every MagnotiaError::StorageError construction site in the storage crate (78 total across database.rs and migrations.rs), groups by failure mode, proposes a typed magnotia_storage::Error enum with a serde- friendly MagnotiaError::Storage { kind, operation, detail } variant on top, and flags 6 ambiguities + 3 migration risk classes. Key findings the residuals plan did not capture: - StorageError is a String variant on MagnotiaError, not a separate enum — there is nothing called StorageError in the storage crate. - Actual site count is 78, not the ~25 the residuals plan estimated. - Tauri commands stringify every error before crossing into the frontend (Result<T, String>), so MagnotiaError's Serialize impl is presently unused at the FE/BE boundary. Area E owns that fix. - Zero Other(String) sites in storage — already cleaned in db654de. No code changes. Migration awaits decisions on the 6 ambiguous cases in §"Ambiguous cases / decisions needed". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-12 22:39:27 +01:00
parent 48c483894b
commit fdab77776c
1 changed files with 392 additions and 0 deletions
--- a/docs/superpowers/plans/2026-05-12-area-a-storage-errors-survey.md
+++ b/docs/superpowers/plans/2026-05-12-area-a-storage-errors-survey.md
@@ -0,0 +1,392 @@
+---
+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.