agent: remove legacy string storage error variant

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) <noreply@anthropic.com>
This commit is contained in:
2026-05-12 23:08:56 +01:00
parent 52565ea8b8
commit a36ae7e068
6 changed files with 8 additions and 11 deletions

View File

@@ -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.

View File

@@ -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;

View File

@@ -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: {<displayed>}` | `PathBuf::display()` interpolated. |
| `StorageError(String)` | `storage error: {0}` | sqlx, profile FK violations, migration failures. |
| `Storage { kind, operation, detail }` | `{detail}` | Boundary shape produced by `From<magnotia_storage::Error>`. `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<sqlx::Error>` 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<sqlx::Error>` impl in core.** The storage crate manually converts every sqlx error to a typed `magnotia_storage::Error::Query` with an operation label, then `From<storage::Error> 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<sqlx::Error>` 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.

View File

@@ -56,11 +56,11 @@ Single-row select.
### `create_profile(pool, id, name, initial_prompt) -> Result<ProfileRow>` — `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`

View File

@@ -81,7 +81,7 @@ pub struct TranscriptRow {
### `insert_transcript(pool, &params) -> 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<Option<TranscriptRow>>` — `crates/storage/src/database.rs:117`

View File

@@ -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