# Error Handling Review — Theseus

> Reviewed against `src/lib.rs`, `src/db.rs`, `src/routes/*.rs`, `src/main.rs`, `src/agent/*.rs`

---

## 1. Error Propagation — **B+**

**How it works:** Server routes return either `(StatusCode, Json<ApiResponse<T>>)` directly or `Result<T, StatusCode>`. The `Result<T, StatusCode>` variant relies on `json_error_middleware` to catch bare status codes and wrap them in `ApiResponse` via `AppError::fallback_for_status()`. The `db_error_response()` helper converts `rusqlite::Error` → `AppError` → `(StatusCode, Json<ApiResponse<T>>)`.

**Evidence:**
- `src/lib.rs:500-520` — `json_error_middleware` catches unhandled error responses and wraps in `ApiResponse`
- `src/lib.rs:318-335` — `db_error_status()` and `db_error_response()` are used consistently across all route modules
- `src/routes/nodes.rs:93, 140, 339, 387, 496` — DB errors always converted via `db_error_response()`
- `src/routes/edges.rs:165, 241` — same pattern
- `src/routes/evidence.rs:85, 90-103, 116-129, 130-143, 155-157` — filesystem + DB errors wrapped with `AppError` + logged

**Strengths:**
- No errors silently swallowed in server code — every `?` and `map_err` propagates
- Filesystem operations in evidence upload (mkdir, create, write) each produce distinct `AppError` with operation tag and source
- DeepSeek API errors (`lib.rs:570-584`) log details server-side but return generic 500 to client — correct security posture

**Weakness:**
- Agent errors collapse to generic `StatusCode::INTERNAL_SERVER_ERROR` (`src/routes/agent.rs:35-38`). The `AgentError` variants (`LlmError`, `MemoryError`, `DatabaseError`) are logged but the client sees only "Internal Server Error". For a debugging tool, surfacing the agent error category would help.

**Recommendation:** Map `AgentError` variants to distinct `AppError` codes (e.g. `agent.llm_failed`, `agent.memory_failed`) so the CLI/UX can show more specific messages.

---

## 2. Error Types — **A-**

**How it works:** `AppError` (`src/lib.rs:27-37`) is a rich struct with: `status`, `code` (dot-notation), bilingual `message_en`/`message_zh`, `operation`, `entity`, `field`, `hint`, `source`. Builder methods (`.with_entity()`, `.with_hint()`, `.with_source()`) compose errors incrementally. `from_db()` auto-maps SQLite extended codes to HTTP status (404/409/500).

**Evidence:**
- `src/lib.rs:140-185` — `from_db()` handles `QueryReturnedNoRows` → 404, unique violation (2067) → 409, FK violation (787) → 409, PK violation (1555) → 409
- `src/lib.rs:187-236` — `fallback_for_status()` provides bilingual fallbacks for 400, 401, 404, 409, and catch-all
- `src/lib.rs:255-308` — `log()` uses structured tracing with all fields; server errors capture backtrace
- Agent uses separate hierarchy: `AgentError` (`loop.rs:236-240`), `LlmError` (`llm.rs:169-173`), `ToolError` (`tools.rs:24`)

**Strengths:**
- Bilingual messages on every error — no generic "something went wrong"
- `code` field uses consistent dot-notation (`db.not_found`, `auth.invalid_api_key`, `project_id.ambiguous`)
- Operation tracing (`operation` + `entity` + `field`) makes debugging from logs straightforward
- `hint` gives actionable advice (e.g. "Provide the x-api-key header, Authorization: Bearer token, or api_key query parameter")

**Weaknesses:**
- Some route handlers construct errors with raw strings instead of `AppError`:
  - `src/routes/nodes.rs:57` — `ApiResponse::err("invalid project_id")` (no code, no bilingual)
  - `src/routes/nodes.rs:66-67` — `ApiResponse::err("label cannot be empty")` (Chinese-only error at line 99 mixed with English-only here)
  - `src/routes/edges.rs:34, 63-65, 87-89, 106-108, 138` — all use `ApiResponse::err(format!(...))` without structured codes
- These bypass `AppError.log()` → no structured logging, no backtrace on 500s

**Recommendation:** Replace `ApiResponse::err("...")` calls in route handlers with `AppError` constructors to get consistent logging and bilingual messages.

---

## 3. Edge Cases — **B**

**Evidence of good handling:**
- `src/routes/nodes.rs:63-68` — empty label validation
- `src/routes/nodes.rs:70-78` — custom type regex `[a-z][a-z0-9_]{0,29}`
- `src/routes/nodes.rs:81-88` — properties must be JSON object
- `src/routes/nodes.rs:95-101` — no isolated nodes (must specify `after`)
- `src/routes/nodes.rs:104-127` — bare path detection in content
- `src/routes/edges.rs:134-141` — self-loop rejection
- `src/routes/edges.rs:143-148` — duplicate edge detection
- `src/routes/evidence.rs:174-176` — path traversal rejection (`..` and leading `/`)
- `src/lib.rs:608` — short ID prefix minimum 6 chars
- `src/lib.rs:642-657` — ambiguous prefix detection

**Gaps:**
- **No file upload size limit** — `src/routes/evidence.rs:114` reads `field.bytes().await` with no cap. A multi-GB upload would exhaust memory.
- **No request body size middleware** — no explicit `ContentLengthLimit` on any route. Axum defaults apply, but multipart handling bypasses them.
- **Broadcast channel capacity 256** (`architecture.md:33`) — if clients are slow, `tx.send()` returns `Err` but the result is discarded (`let _ = state.tx.send(...)` at `nodes.rs:179`). This is acceptable (best-effort broadcast) but means slow WebSocket clients silently miss events.
- **SQLite lock contention** — all operations share one `Mutex<Connection>`. Under high write concurrency, threads will block. No timeout on mutex acquisition.

**Recommendation:**
1. Add `ContentLengthLimit` or per-multipart-field size check to evidence upload.
2. Consider `tokio::sync::Mutex` or connection pooling if concurrency grows.

---

## 4. Panic Safety — **B+**

**Production unwrap inventory (non-test code):**

| Location | Expression | Risk |
|----------|-----------|------|
| `db.rs:94,111,112,131-134,152,153,172-174` | `Uuid::parse_str(&...).unwrap()` in row deserializers | **Low** — data comes from SQLite; corrupted UUID = corrupted DB, panicking is defensible |
| `db.rs:96,120,136,161,179` | `ca.parse::<DateTime>().unwrap()` in row deserializers | **Low** — same reasoning; RFC3339 TEXT stored by the app itself |
| `lib.rs:557` | `body.as_object_mut().unwrap()` on just-constructed `json!({...})` | **None** — value is guaranteed to be an object |
| `llm.rs:55` | `body.as_object_mut().unwrap()` on just-constructed `json!({...})` | **None** — same |
| `evidence.rs:187` | `.parse::<Mime>().unwrap()` with fallback | **None** — fallback is `"application/octet-stream"` which is always valid |

**Test-only panics:**
- `db.rs:1739, 1784` — `panic!("unexpected error: {other:?}")` in match arms of `#[test]` functions. Acceptable.

**Mutex poison recovery:**
- `db.rs:17-18` — `self.conn.lock().unwrap_or_else(|e| e.into_inner())`. Correct: recovers from panicked threads rather than cascading poison.

**Assessment:** No production unwrap can trigger on valid user input. Row deserializer unwraps would only fire on DB corruption — crashing is arguably better than returning garbage. The `as_object_mut().unwrap()` calls are provably safe.

**Recommendation:** Consider replacing `Uuid::parse_str(...).unwrap()` in row deserializers with `.map_err(|e| rusqlite::Error::InvalidParameterName(e.to_string()))` to return a proper `Result` instead of panicking. This is a hardening improvement, not a bug.

---

## 5. User-Facing Errors — **A**

**Evidence:**
- Every `AppError` carries bilingual `message_en` + `message_zh` (`src/lib.rs:26-37`)
- `ApiResponse` includes `ApiErrorDetails` with `operation`, `entity`, `field`, `hint` + `LocalizedErrorMessages` (`src/lib.rs:238-253`)
- `unauthorized_response()` (`lib.rs:436-448`) tells the user exactly how to provide the API key
- CLI errors use `eprintln!("Error: {e}")` + `std::process::exit(1)` (`main.rs:471-478` and all subcommands) — clean single-line output
- CLI HTTP error parsing (`main.rs:2451-2652`) extracts JSON error bodies, handles non-JSON responses, and shows previews of malformed bodies
- Validation errors are specific: "label cannot be empty", "Self-loop edges not allowed", "must provide source_node_id or source_label", etc.
- `AppError.log()` (`lib.rs:255-308`) uses structured tracing with different levels: ERROR for 5xx, WARN for 409, INFO for other 4xx — plus backtrace capture for server errors

**Minor nit:**
- Mixed language in some raw `ApiResponse::err()` calls (Chinese at `nodes.rs:99`, English elsewhere in same handler). `AppError` usage avoids this; raw strings don't.

**Recommendation:** Enforce `AppError` usage via a lint or convention to eliminate mixed-language raw string errors.

---

## 6. Recovery — **B+**

**Evidence:**
- **Mutex poison** — `db.rs:17-18` recovers via `unwrap_or_else(|e| e.into_inner())`
- **WebSocket reconnect** — `static/js/ws.js` auto-reconnects on close (2s delay per architecture doc)
- **Agent tool errors** — `loop.rs:167-179` catches `ToolError`, records it, and feeds the error back to the LLM as a tool result message. The loop continues — one bad tool call doesn't kill the run.
- **Agent max-turns safety** — `loop.rs:80-82` stops after `max_turns` and returns partial results
- **Broadcast channel overflow** — `tx.send()` errors are discarded (`let _ = ...`). No crash, but events are lost.
- **Config fallback** — `lib.rs:353-410` cascades: CLI args → env vars → `theseus.toml` → `~/.config/theseus/config.toml`. Missing config is graceful (all fields are `Option`).
- **API key is optional** — `lib.rs:490-492` skips auth middleware if no key is configured. Works without auth for local dev.

**Gaps:**
- **Evidence upload partial failure** — `evidence.rs:106-160` processes multipart fields in a loop. If the second field fails (e.g. write error), the first file is already saved on disk + in DB, but the request returns an error. No rollback of the first file.
- **DeepSeek API failures** — `lib.rs:570-584` returns generic 500. No retry logic. No circuit breaker. If the API is down, every `tell`/`ask`/`agent` call fails individually.
- **Orphaned files** — architecture doc notes: "deleting the DB record does NOT delete the file." Evidence files accumulate on disk after `DELETE /evidence`.

**Recommendation:**
1. Add retry with exponential backoff for DeepSeek API calls (or at least document the absence).
2. Consider a cleanup mechanism for orphaned evidence files.
3. Wrap multipart upload in a transaction-like pattern: write all files to a temp dir, then move + DB insert atomically.

---

## Summary

| Criterion | Score | Notes |
|-----------|-------|-------|
| Error propagation | **B+** | Consistent in routes; agent errors collapse to generic 500 |
| Error types | **A-** | Excellent `AppError`; some routes still use raw `ApiResponse::err()` |
| Edge cases | **B** | Strong validation; missing upload size limits |
| Panic safety | **B+** | No user-triggerable panics; DB deserializer unwraps are defensible |
| User-facing errors | **A** | Bilingual, structured, actionable — best-in-class for a CLI+server tool |
| Recovery | **B+** | Mutex poison + agent tool errors handled; no retry/orphan cleanup |
| **Overall** | **B+** | Solid error handling infrastructure with room to tighten consistency |
