From fdab77776cebc484ebb05204b141a8fd3c83c56c Mon Sep 17 00:00:00 2001 From: Jake Date: Tue, 12 May 2026 22:39:27 +0100 Subject: [PATCH] docs(area-a): storage error inventory + proposed typed taxonomy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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), 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) --- ...2026-05-12-area-a-storage-errors-survey.md | 392 ++++++++++++++++++ 1 file changed, 392 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-12-area-a-storage-errors-survey.md diff --git 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 new file mode 100644 index 0000000..4f84d01 --- /dev/null +++ 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` 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 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 `magnotia_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 `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, + 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 `magnotia_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 `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 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 = 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` → `magnotia_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 `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` 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 `magnotia_core::Result` and body uses `magnotia_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 `magnotia_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 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` 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.