From a36ae7e068c883683a7d15b52b842bd310fa5716 Mon Sep 17 00:00:00 2001 From: Jake Date: Tue, 12 May 2026 23:08:56 +0100 Subject: [PATCH] agent: remove legacy string storage error variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 52565ea migrated storage to magnotia_storage::Error and flattened typed storage failures into MagnotiaError::Storage { kind, operation, detail }. No production code constructs the old MagnotiaError::StorageError String variant anymore. Remove the legacy variant so new storage failures cannot regress back to stringly-typed errors. Also updates living architecture-map docs that referenced the old variant (core-error.md variant table, storage-overview.md SQLITE_BUSY note, storage-crud-profiles.md duplicate-name + default- profile-rename notes, storage-crud-transcripts.md pre-flight FK check note) and one stale code comment in crates/storage/src/database.rs's duplicate-name test. Survey doc + old residuals plan + phase8 historical plan deliberately left alone — they're audit trail of how the migration was decided, not living docs. Pre-existing doc rot flagged but not fixed (Other(String) and Io(std::io::Error) rows in core-error.md are about variants that already don't match the actual enum shape — separate doc cleanup pass). Verification: - cargo fmt --all -- --check - cargo check -p magnotia-core - cargo check -p magnotia-storage - cargo check --workspace --all-targets - cargo test -p magnotia-storage — 60 passed, 0 failed - cargo test --workspace --lib — all green - rg 'StorageError\(' crates/ src-tauri/src/ — zero hits - rg 'StorageError' crates/ src-tauri/src/ docs/architecture-map/ — zero - rg 'Other\(String\)' crates/ src-tauri/src/ — zero Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/core/src/error.rs | 3 --- crates/storage/src/database.rs | 2 +- .../05-core-storage-hotkey-build/core-error.md | 6 +++--- .../05-core-storage-hotkey-build/storage-crud-profiles.md | 4 ++-- .../storage-crud-transcripts.md | 2 +- .../05-core-storage-hotkey-build/storage-overview.md | 2 +- 6 files changed, 8 insertions(+), 11 deletions(-) diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index b8c32f9..0a318ab 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -34,9 +34,6 @@ pub enum MagnotiaError { #[error("file not found: '{}'", .0.display())] FileNotFound(PathBuf), - #[error("storage error: {0}")] - StorageError(String), - /// Structured storage failure flowed up from `magnotia_storage::Error` via /// its `From` impl. Display reads through to `detail` so the operation + /// source context produced by the storage crate isn't double-prefixed. diff --git a/crates/storage/src/database.rs b/crates/storage/src/database.rs index 7414744..87fc35b 100644 --- a/crates/storage/src/database.rs +++ b/crates/storage/src/database.rs @@ -2129,7 +2129,7 @@ mod tests { #[tokio::test] async fn create_profile_rejects_duplicate_name() { - // Guardrail: UNIQUE(name) collision surfaces as StorageError. + // Guardrail: UNIQUE(name) collision surfaces as Error::Query. let pool = test_pool().await; create_profile(&pool, "Personal", "").await.unwrap(); let res = create_profile(&pool, "Personal", "").await; diff --git a/docs/architecture-map/05-core-storage-hotkey-build/core-error.md b/docs/architecture-map/05-core-storage-hotkey-build/core-error.md index 0af6247..67b223e 100644 --- a/docs/architecture-map/05-core-storage-hotkey-build/core-error.md +++ b/docs/architecture-map/05-core-storage-hotkey-build/core-error.md @@ -34,7 +34,7 @@ last_verified: 2026/05/09 | `AudioCaptureFailed(String)` | `audio capture failed: {0}` | cpal / native capture failures (slice 3). | | `DownloadFailed(String)` | `model download failed: {0}` | Resumable download errors. | | `FileNotFound(PathBuf)` | `file not found: {}` | `PathBuf::display()` interpolated. | -| `StorageError(String)` | `storage error: {0}` | sqlx, profile FK violations, migration failures. | +| `Storage { kind, operation, detail }` | `{detail}` | Boundary shape produced by `From`. `kind` is the serialisable discriminator (`StorageKind`), `operation` is the typed operation label, `detail` is the storage crate's own `Display` output (no double prefix). | | `Io(std::io::Error)` | `io error: {0}` | `#[from]` so `?`-conversion from `std::io::Error` is automatic. | | `Other(String)` | `{0}` | Catch-all bucket. | @@ -49,12 +49,12 @@ Every public function in the workspace that can fail returns `magnotia_core::Res ## Data flow / contract - All variants are `Serialize`-able. `std::io::Error` does not derive `Serialize`, so the `Io` variant uses a custom `serialize_with` adaptor (`serialize_io_error` at `crates/core/src/error.rs:53`) that emits the error's `Display` string. -- Variants do not carry source-location information. If you need a stack-style trace, attach context at the call site by wrapping in `StorageError(format!("{action} failed: {e}"))` — the storage CRUD layer follows this convention universally. +- Variants do not carry source-location information. The storage CRUD layer attaches context via the typed `magnotia_storage::Error::Query { operation, source }` shape; the `operation` label survives into `MagnotiaError::Storage.operation` at the boundary. - Tauri serialises the enum verbatim. The frontend can switch on the discriminant by reading the JSON tag (the variant name). ## Watch-outs -- **No `From` impl.** The storage crate manually converts every sqlx error to `MagnotiaError::StorageError(format!(...))`. Adding an automatic `From` would let raw sqlx error strings leak into the frontend; the explicit map step is intentional. +- **No `From` impl in core.** The storage crate manually converts every sqlx error to a typed `magnotia_storage::Error::Query` with an operation label, then `From for MagnotiaError` (defined inside the storage crate to avoid a `core -> storage` dependency cycle) flattens it into `MagnotiaError::Storage { kind, operation, detail }` at the boundary. Adding an automatic `From` would erase the per-site operation label; the explicit map step is intentional. - **No `Source` chain.** `thiserror` would let you wrap source errors in fields with `#[source]` for chained `Display`. Today every wrapped error is flattened to `String` to keep the JSON shape simple. - **`Other(String)` is a leaky bucket.** New error categories should get their own variant rather than reaching for `Other`. Audit `Other` usage if the error log starts hiding distinct failure modes behind the same string. diff --git a/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-profiles.md b/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-profiles.md index 8708320..ec81698 100644 --- a/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-profiles.md +++ b/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-profiles.md @@ -56,11 +56,11 @@ Single-row select. ### `create_profile(pool, id, name, initial_prompt) -> Result` — `crates/storage/src/database.rs:968` -UUID is generated by the caller. `name` has a `UNIQUE` constraint at the column level (migration v6); a duplicate name returns a friendly `MagnotiaError::StorageError("Profile name already exists: ...")`. +UUID is generated by the caller. `name` has a `UNIQUE` constraint at the column level (migration v6); a duplicate name returns `magnotia_storage::Error::Query { operation: "create_profile", source }` carrying the sqlx UNIQUE-constraint error. ### `update_profile(pool, id, name, initial_prompt)` — `crates/storage/src/database.rs:995` -Renames the profile and / or rewrites the initial prompt. **Updating the default profile's name** raises `MagnotiaError::StorageError` because the `trg_protect_default_profile_rename` trigger (migration v6) calls `RAISE(ABORT, 'cannot rename the default profile')` on any `UPDATE OF id, name` where `OLD.id = DEFAULT_PROFILE_ID`. Updating only `initial_prompt` is allowed. +Renames the profile and / or rewrites the initial prompt. **Updating the default profile's name** is short-circuited in Rust before hitting sqlite, raising `magnotia_storage::Error::InvalidReference { entity: Entity::Profile, reason: "Default profile cannot be renamed" }`. The `trg_protect_default_profile_rename` trigger (migration v6) is the structural backstop. Updating only `initial_prompt` is allowed. ### `delete_profile(pool, id) -> Result<()>` — `crates/storage/src/database.rs:1024` diff --git a/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-transcripts.md b/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-transcripts.md index 369cc3a..e6d7bb8 100644 --- a/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-transcripts.md +++ b/docs/architecture-map/05-core-storage-hotkey-build/storage-crud-transcripts.md @@ -81,7 +81,7 @@ pub struct TranscriptRow { ### `insert_transcript(pool, ¶ms) -> Result<()>` — `crates/storage/src/database.rs:80` -1. **Pre-flight FK check.** `profile_exists(pool, params.profile_id)` (private, `database.rs:1094`) runs `SELECT 1 FROM profiles WHERE id = ?`. If the profile is not present, returns a friendly `MagnotiaError::StorageError("Insert transcript failed: unknown profile id '...'")`. Without this, sqlite would raise `FOREIGN KEY constraint failed` which the frontend cannot easily disambiguate. +1. **Pre-flight FK check.** `profile_exists(pool, params.profile_id)` (private, `database.rs:1094`) runs `SELECT 1 FROM profiles WHERE id = ?`. If the profile is not present, returns a typed `magnotia_storage::Error::InvalidReference { entity: Entity::Profile, reason: "unknown profile id '...'" }`. Without this, sqlite would raise `FOREIGN KEY constraint failed` which the frontend cannot easily disambiguate. 2. Single `INSERT INTO transcripts (...) VALUES (...)` with all 16 fields. ### `get_transcript(pool, id) -> Result>` — `crates/storage/src/database.rs:117` diff --git a/docs/architecture-map/05-core-storage-hotkey-build/storage-overview.md b/docs/architecture-map/05-core-storage-hotkey-build/storage-overview.md index fd492de..699ef58 100644 --- a/docs/architecture-map/05-core-storage-hotkey-build/storage-overview.md +++ b/docs/architecture-map/05-core-storage-hotkey-build/storage-overview.md @@ -82,7 +82,7 @@ Per-table CRUD is split across the per-page docs in this slice. See: ## Watch-outs - **`PRAGMA foreign_keys = ON` is per-connection, not per-database.** The pool's `max_connections = 5` means we run the pragma once at init on the first connection. SQLite re-applies the pragma on each new pool connection because we set it via the connect options... but actually we don't, we set it after `connect_with`. **This is a latent issue worth verifying:** if a second pool connection opens later, foreign keys may not be enforced on it. Audit candidate. -- **No connection-level retry on locked DB.** `SQLITE_BUSY` propagates as `MagnotiaError::StorageError(...)`. With WAL mode + 5 max connections this is rare, but a long-running write under a slow filesystem could trigger it. +- **No connection-level retry on locked DB.** `SQLITE_BUSY` propagates as `magnotia_storage::Error::Query { ... }` (flattened to `MagnotiaError::Storage { kind: Query, ... }` at the boundary). With WAL mode + 5 max connections this is rare, but a long-running write under a slow filesystem could trigger it. - **Custom migration runner.** sqlx's bundled `migrate!` macro is not used. The custom runner is documented in [`storage-schema-and-migrations.md`](storage-schema-and-migrations.md) and was the subject of the C3 critical-issue write-up at `docs/issues/c3-migrations-atomicity.md`. ## Existing in-repo docs