# Code Quality Review — Theseus

> Generated from source analysis of `src/` and `static/`.
> Rust backend: axum, rusqlite, clap. ~12,031 lines of Rust across 16 files.

---

## 1. Naming — **B+**

**Strengths:**
- Types are descriptive: `AppState`, `ApiResponse<T>`, `CreateNodeRequest`, `AgentRuntime`.
- Functions follow Rust conventions: `create_project`, `list_edges`, `resolve_node_id`.
- Constants use `SCREAMING_CASE` (`VALID_VERDICTS`, `VALID_EVIDENCE`).
- Domain abbreviations (`ep`, `obj`, `recon`, `vuln`, `cred`, `svc`, `net`, `exfil`) are documented in `docs/context/conventions.md`.

**Issues:**
- Row deserializers in `db.rs` use cryptic variable names: `ca` (created_at), `nt_s` (node_type string), `id_s` (id string), `pid_s`, `src_s`, `tgt_s` (lines 90–179). These reduce readability in a file that's already 1,992 lines.
- `BoxErr` type alias in `main.rs` (not shown in context but used throughout) is terse.

**Evidence:**
```
src/db.rs:91    let id: String = row.get(0)?;
src/db.rs:92    let ca: String = row.get(2)?;
src/db.rs:101   let id_s: String = row.get(0)?;
src/db.rs:102   let pid_s: String = row.get(1)?;
src/db.rs:103   let nt_s: String = row.get(3)?;
```

**Recommendation:** Rename `ca` → `created_at_str`, `nt_s` → `node_type_str`, `id_s` → `id_str`, etc. The 1–2 extra characters are negligible vs. the clarity gain.

---

## 2. File Organization — **B**

**Strengths:**
- Clean module separation: `routes/` (16 modules), `agent/` (5 modules), `types.rs`, `db.rs`, `lib.rs`, `main.rs`.
- Each route module exports a single `router()` function — consistent and discoverable.
- Agent subsystem is self-contained with clear responsibilities per file.

**Issues:**
| File | Lines | Concern |
|------|-------|---------|
| `main.rs` | 2,656 | CLI binary + full HTTP client. `handle_project()` alone is ~800 lines. |
| `db.rs` | 1,992 | All DB ops + analysis types + tests in one file. |
| `types.rs` | 1,147 | All DTOs in one file — manageable but long. |

- Analysis-only types (`AttackPath`, `PathNode`, `TaggedNode`, `NextStep`, `CrossConnection`, `HealthIssue`, `CheckIssue`, `CheckResult`, `AttackAnalysis`) are defined in `db.rs` (lines 724–799), not `types.rs`. This breaks the "single source of truth" pattern documented in the module breakdown.

**Recommendation:**
1. Extract `handle_project`, `handle_node`, `handle_edge`, `handle_credential` from `main.rs` into `src/cli/` submodules.
2. Move analysis types from `db.rs` to `types.rs`.
3. Consider splitting `db.rs` into `db/crud.rs` and `db/analysis.rs`.

---

## 3. Code Duplication — **B-**

**Strengths:**
- Route modules share error handling via `crate::db_error_status()` / `crate::db_error_response()`.
- Response types use `From<Entity>` impls consistently.

**Issues:**

**a) Test boilerplate (9 copies):**
Every route test module repeats the same setup:
```rust
let db = Box::leak(Box::new(crate::db::Database::open(&db_path).unwrap()));
// ... insert test data ...
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
tokio::spawn(async move { axum::serve(listener, app).await.unwrap() });
```
Found in: `analysis.rs:480`, `credentials.rs:98`, `edges.rs:257`, `evidence.rs:237`, `nodes.rs:512`, `projects.rs:143`, `report.rs:210`, `search.rs:45`, `timeline.rs:38`.

**b) Row deserializer pattern (4 copies):**
Each `row_*` function in `db.rs` repeats: parse string from column → `Uuid::parse_str(&s).unwrap()` → `ca.parse().unwrap()`. The UUID + timestamp parsing is identical across `row_project`, `row_node`, `row_edge`, `row_credential`, `row_evidence_file`.

**c) Tool argument extraction in `agent/tools.rs`:**
Pattern `args["field"].as_str().ok_or_else(|| ToolError("Missing field".to_string()))?` repeats ~15 times across tool implementations (lines 115–120, 197–202, 449–451, 538–546, etc.).

**Recommendation:**
1. Extract a `test_helpers` module with `spawn_test_app() -> (String, &'static Database)`.
2. Create a `parse_uuid_from_row(row, index)` helper in `db.rs`.
3. Create a `required_str_arg(args, "field") -> Result<&str, ToolError>` helper in `agent/tools.rs`.

---

## 4. Complexity — **C+**

**Strengths:**
- Individual route handlers are mostly focused (30–80 lines each).
- Agent tool functions are self-contained and composable.

**Issues:**

| Function | File | Lines | Concern |
|----------|------|-------|---------|
| `handle_project()` | main.rs:981 | ~800 | Massive match with deeply nested sub-handlers |
| `handle_node()` | main.rs:1790 | ~316 | Similar pattern, 13+ parameters in `NodeAction::Add` |
| `create_node()` | routes/nodes.rs:47 | 134 | 5 validation branches + edge creation + WS broadcast |
| `update_node()` | db.rs:279 | 65 | 7 `if let Some(ref ...)` branches, 3 levels deep |
| `NodeAction::Add` | main.rs:1796 | 15 fields | Function has 15 parameters — exceeds cognitive load |

**Evidence (deep nesting in db.rs `update_node`):**
```rust
if let Some(ref props) = req.properties {           // level 1
    if let (Value::Object(ref mut e), Value::Object(ref i)) = ... {  // level 2
        for (k, v) in i {                            // level 3
            e.insert(k.clone(), v.clone());
        }
    }
}
// ... 5 more similar blocks for content, append_content, verdict, evidence
```

**Recommendation:**
1. Split `handle_project` into one function per `ProjectAction` variant.
2. Use a builder pattern for `NodeAction::Add` to reduce parameter count.
3. Refactor `db.rs:update_node` to use a field-merge helper or builder.

---

## 5. Dead Code — **B+**

**Strengths:**
- No `#[allow(dead_code)]` or `#[allow(unused)]` attributes found anywhere.
- No commented-out code blocks in `src/`.
- All imports appear used.

**Issues:**
- `_print_created()` in `main.rs:2592` — prefixed with `_` (Rust convention for intentionally unused), but it's a 37-line function that's never called. Either delete it or remove the `_` prefix if it's needed.

**Evidence:**
```rust
src/main.rs:2592  async fn _print_created(resp: reqwest::Response, verb: Option<&str>) -> Result<(), BoxErr> {
// 37 lines of implementation, never called
src/main.rs:2631  async fn print_created(resp: reqwest::Response) -> Result<(), BoxErr> {
src/main.rs:2632      _print_created(resp, None).await
```

Wait — `print_created` does call `_print_created`. So this is alive code with a misleading `_` prefix. The `verb` parameter is always `None` at the only call site.

**Recommendation:** Remove the `_` prefix and inline the `verb` parameter if it's never used with a value.

---

## 6. Anti-patterns — **A-**

**TODO/FIXME/HACK/XXX density:**
- **Zero** markers in `src/`. Clean.
- 2 TODOs found in vendored third-party JS (`cytoscape-elk.js:267`, `dagre.min.js:427`) — not project code.

**`unwrap()` usage in production code:**

| Location | Count | Severity |
|----------|-------|----------|
| `db.rs` row deserializers | ~20 | **Medium** — panics on corrupt DB data |
| `db.rs:440` `FixedOffset::east_opt(8*3600).unwrap()` | 1 | **Low** — constant is always valid |
| `lib.rs` format strings | ~12 | **Low** — `unwrap_or()` on `Option<&str>`, safe |
| `agent/llm.rs:55` | 1 | **Medium** — `.unwrap()` on serde parse |
| Route handlers (production) | ~5 | **Low** — all `unwrap_or` / `unwrap_or_default` |
| Test code | ~80+ | **Acceptable** — tests should panic on unexpected |

**Evidence (db.rs row deserializers — panic on corrupt data):**
```rust
src/db.rs:94    id: Uuid::parse_str(&id).unwrap(),      // panics if DB has bad UUID
src/db.rs:96    created_at: ca.parse().unwrap(),          // panics if DB has bad timestamp
src/db.rs:111   id: Uuid::parse_str(&id_s).unwrap(),     // same pattern, 4 more instances
```

These `unwrap()` calls are in row deserializers that rusqlite calls for every row. If the DB ever has corrupt data (manual edit, migration bug, bit flip), the server panics instead of returning an error.

**Recommendation:**
1. Replace `unwrap()` in row deserializers with `map_err(|e| rusqlite::Error::InvalidParameterName(e.to_string()))` or similar, so corrupt rows produce a query error instead of a panic.
2. The `agent/llm.rs:55` unwrap should propagate the error via `?`.

---

## Summary Scores

| Criterion | Score | Notes |
|-----------|-------|-------|
| Naming | **B+** | Good overall, cryptic abbreviations in db.rs row deserializers |
| File Organization | **B** | Clean modules, but 3 oversized files; misplaced analysis types |
| Code Duplication | **B-** | Test boilerplate ×9, row deserializer pattern ×4, tool arg extraction ×15 |
| Complexity | **C+** | `handle_project` (800 lines), `NodeAction::Add` (15 params), deep nesting in update logic |
| Dead Code | **B+** | One misleading `_print_created` prefix; otherwise clean |
| Anti-patterns | **A-** | Zero TODO/FIXME; ~20 production `unwrap()`s in row deserializers |

**Overall: B**

The codebase is well-structured with consistent conventions and no technical debt markers. The main concerns are file size (main.rs, db.rs), test boilerplate duplication, and `unwrap()` in row deserializers that could cause panics on corrupt data. These are addressable without architectural changes.
