# API Design Review — Theseus

**Reviewed:** 2025-07-02
**Scope:** REST API (axum, 16 route modules) + CLI (clap)
**Codebase:** single Rust binary (`lib.rs` = server, `main.rs` = CLI HTTP client)

---

## Summary

| Criterion              | Grade | One-line verdict |
|------------------------|-------|------------------|
| Consistency            | B+    | Strong patterns, a few leaks |
| Discoverability        | B     | Good inline docs, no OpenAPI |
| Backward Compatibility | C     | No versioning, no changelog, internal types leak |
| Input Validation       | B+    | Thorough at boundaries, some gaps |
| Documentation          | B-    | Partial doc comments, bilingual but inconsistent |
| Minimalism             | B+    | Lean surface, some dead types |

**Overall: B**

---

## 1. Consistency — Grade: B+
### What works well

- **Uniform router pattern.** Every route module exposes `pub(crate) fn router() -> Router<AppState>`, merged in `lib.rs:814-829`. A new developer can predict where to find any endpoint.
- **Consistent response envelope.** Every handler returns `ApiResponse<T>` with `{ success, data, error }`. Error responses add `error_code`, `error_details`, and `error_messages` (en/zh) via `ApiResponse::err_with_details()`.
- **Uniform ID resolution.** All path params go through `resolve_project_id()`, `resolve_node_id()`, `resolve_edge_id()`, or `parse_uuid()` (`lib.rs:522-781`). Short-ID prefix resolution is transparent.
- **Consistent status codes for CRUD:**
  - Create → `201 Created`
  - Get/List → `200 OK`
  - Delete not found → `404 Not Found`
  - Conflict → `409 Conflict`


### Issues

**Return type inconsistency across handlers.** Three different return signatures coexist:

| Pattern | Example | Used by |
|---------|---------|---------|
| `(StatusCode, Json<ApiResponse<T>>)` | `create_node`, `update_edge` | nodes, edges, some projects |
| `Result<(StatusCode, Json<ApiResponse<T>>), StatusCode>` | `create_credential` | credentials |
| `Result<Json<ApiResponse<T>>, StatusCode>` | `list_projects`, `get_node` | list/get handlers |

The bare-tuple handlers cannot use the `?` operator for DB errors — they must manually call `crate::db_error_response()`. This is error-prone and produces boilerplate. Recommendation: standardize on `Result<(StatusCode, Json<ApiResponse<T>>), AppError>` with a blanket `IntoResponse` impl for `AppError`.

**`check-duplicate` lives in `analysis.rs`, not `nodes.rs`.** The API docs say "src/routes/nodes.rs" (`api.md:86`) but the endpoint is registered in `analysis.rs:15-17`. This breaks discoverability — a developer looking at the nodes module will not find it.

**`agent.rs` uses `pub fn router()` instead of `pub(crate) fn router()`.** Every other route module uses `pub(crate)`. This is a minor visibility leak (`routes/agent.rs:8`).

**Unused Response types.** `ProjectResponse`, `NodeResponse`, `EdgeResponse`, `CredentialResponse`, `EvidenceFileResponse` are defined in `types.rs` with `From` impls but **never used by any route handler**. Routes return the raw DB types (`Project`, `Node`, `Edge`, `Credential`) directly. This means:
- API consumers see internal fields like `node_type: NodeType` (enum) rather than the cleaner `node_type: String`.
- The `From` impls exist for nothing — dead code that misleads developers about the intended response shape.
---

## 2. Discoverability — Grade: B

### What works well

- **Context documentation exists.** `docs/context/api.md` documents every endpoint, method, path, request body, and status code.
- **Error codes are well-structured.** Five namespaces (`db.*`, `request.*`, `auth.*`, `resource.*`, `server.*`) with consistent dotted notation (`lib.rs:153-174`).
- **Structured error details.** `ApiErrorDetails` includes `operation`, `entity`, `field`, `hint` — excellent for debugging.

### Issues

- **No OpenAPI/Swagger spec.** The API has 50+ endpoints but no machine-readable schema. Frontend devs, QA, and external integrators must read source code or the markdown doc.
- **No `rustdoc` on public types.** `Project`, `Node`, `Edge`, `Credential`, `ApiResponse` etc. in `types.rs` have zero `///` doc comments on the struct itself. Only a handful of request fields have `///` annotations (e.g., `CreateNodeRequest.after`, `CreateNodeRequest.edge_label`).
- **Section headers are bilingual (Chinese) only.** `types.rs:321` (`// 1. Credential 凭据管理`), `types.rs:551` (`// 6. Agent 系统`). Inconsistent with English code and English error messages.
- **CLI help text is good.** Clap `#[command(about = "...")]` and `///` doc comments on subcommands provide self-documenting CLI (`main.rs:12-117`).
---

## 3. Backward Compatibility — Grade: C

### What works well

- **Enum-based extensibility.** `NodeType`, `CredentialType`, `CredentialStatus` use `#[serde(rename_all = "snake_case")]` with string serialization. Adding new variants is backward-compatible for consumers.
- **`#[serde(skip_serializing_if = "Option::is_none")]` on optional error fields.** Old clients that do not expect `error_code` or `error_details` will not break.

### Issues

- **No API versioning.** All routes are under `/api/` with no version prefix. A breaking change to any endpoint (e.g., renaming `properties` to `metadata`) breaks all existing clients immediately.
- **No CHANGELOG or MIGRATION document.** No `CHANGELOG.md` exists in the repository.
- **Internal types leak into the API surface.** Routes return `Node` directly, which contains `node_type: NodeType` — an enum with `#[serde(untagged)] Custom(String)`. If the enum is refactored (e.g., adding a new built-in variant), serialization of existing data could change. The unused `NodeResponse` (which flattens `node_type` to `String`) was clearly intended to prevent this but is not wired up.
- **Hardcoded column indices in row deserializers.** `db.rs:90-97` (`row.get(0)`, `row.get(2)`). Any `SELECT` reordering silently breaks deserialization. This is a fragility risk for the DB layer — not directly an API issue, but it affects data integrity which consumers depend on.
- **`edges` table has no FK on `source_node_id`/`target_node_id`.** Documented in `architecture.md:137`. Orphaned edges are possible after node deletion, which would return dangling UUIDs to API consumers.
---

## 4. Input Validation — Grade: B+

### What works well

- **Thorough node validation** (`nodes.rs:62-127`):
  - Empty label → 400
  - Invalid `node_type` (custom pattern `[a-z][a-z0-9_]{0,29}`) → 400
  - `properties` must be JSON object → 400
  - Bare path detection in `content` → 400 (enforces full URLs)
  - Isolated node prevention → 400 (must specify `after` when project has nodes)
- **Edge validation** (`edges.rs:134-155`):
  - Self-loop detection → 400
  - Duplicate detection → 409
  - Label-based node resolution with ambiguity detection
- **Project name validation** (`projects.rs:27-51`):
  - Trimmed, empty check, max 200 chars, no newlines
- **Enum validation at update boundary.** `set_verdict` and `set_evidence` validated against `VALID_VERDICTS` and `VALID_EVIDENCE` arrays (`nodes.rs:16-17, 303-327`).

### Issues

- **Credential validation is minimal.** `create_credential` (`credentials.rs:35-58`) validates `source_node_id` existence but does NOT validate:
  - Empty `label`
  - Empty `secret`
  - Valid `credential_type` (relies entirely on serde enum deserialization — a raw string like `"foo"` returns 500, not 400)
  - Valid `status` (same serde reliance)
- **`content` + `append_content` precedence undefined.** `UpdateNodeRequest` has both `content` and `append_content` as `Option<String>` (`types.rs:125-129`). No documentation or code clarifies what happens when both are provided simultaneously.
- **Bilingual error messages are inconsistent.** Some errors are English-only (`"label cannot be empty"` in `nodes.rs:66`), some are Chinese-only (`"孤立节点：项目已有节点..."` in `nodes.rs:99`, bare path detection in `nodes.rs:120`), and structured errors have both (`AppError` in `lib.rs`). This is confusing for API consumers.
- **`force` query param on node delete uses raw `HashMap`** (`nodes.rs:346`). Other endpoints use typed query structs. Minor inconsistency.
---

## 5. Documentation — Grade: B-

### What works well

- **`docs/context/api.md`** is a comprehensive API reference covering all 50+ endpoints with methods, paths, request bodies, and validation rules.
- **`docs/context/architecture.md`** clearly explains the single-binary design, middleware stack, data flow, and module map.
- **Request type field annotations.** `CreateNodeRequest.after`, `CreateNodeRequest.edge_label`, `CreateEdgeRequest.source_label/target_label` have `///` doc comments.
- **Structured error responses** are self-documenting: `error_code`, `error_details.operation`, `error_details.hint`.

### Issues

- **No `rustdoc` on core types.** `Project`, `Node`, `Edge`, `Credential`, `ApiResponse<T>`, `Database` — none have `///` doc comments.
- **No `rustdoc` on route handler functions.** All handlers are `async fn` with no documentation. A developer reading `src/routes/nodes.rs` has no idea what `POST /api/projects/:pid/nodes` does without consulting the external doc.
- **`Database` methods are undocumented.** Only `search_nodes` and agent memory methods have `///` comments (`db.rs:421, 1272, 1291, 1328`). The other ~30 public methods (`create_project`, `insert_node`, `update_node`, etc.) have zero documentation.
- **Mixed-language comments.** Section headers in `types.rs` are Chinese (`// 1. Credential 凭据管理`). Agent subsystem doc comments are Chinese-only (`/// Agent 运行入口点`, `/// 工具调用记录`). Error messages are mixed. This should be standardized.
- **`api.md` claims `source_label`/`target_label` "Auto-create" nodes** (line 94: "Match by UUID or label"). The actual code in `edges.rs:70-101` only looks up existing nodes by fuzzy label match and returns errors if no match or ambiguous match. The doc comment in `CreateEdgeRequest` (`types.rs:180-185`) says "Auto-create source/target node" — **this is misleading**. No auto-creation occurs.
---

## 6. Minimalism — Grade: B+

### What works well

- **`AppState` is `pub(crate)`.** Internal state is not exposed (`lib.rs:427-432`).
- **`Database` is module-private.** `db.rs` is `mod db` (not `pub mod`), so consumers cannot reach into the DB layer.
- **Lean route module surface.** Each module exposes exactly one function: `router()`.
- **`types.rs` is `pub`.** This is correct — the CLI needs to deserialize API responses.
- **Reasonable field count on update requests.** `UpdateNodeRequest` has 7 optional fields, which is on the edge but justified for a pentest graph tool.

### Issues

- **Dead response types.** `ProjectResponse`, `NodeResponse`, `EdgeResponse`, `CredentialResponse`, `EvidenceFileResponse` are defined and have `From` impls but are never used. This is ~100 lines of dead code that should either be wired up or removed.
- **`ProjectResponse` is a structural duplicate of `Project`.** Both have `id: Uuid`, `name: String`, `created_at: DateTime<Utc>`. The `From` impl copies fields 1:1. This adds indirection with no value.
- **`EdgeResponse` duplicates `Edge`** — same situation.
- **`CredentialResponse` leaks `secret` field.** The response type includes `pub secret: String` (`types.rs:405`). The route handler returns `Credential` directly, which also contains `secret`. API consumers see plaintext secrets in every list/get response. Consider omitting `secret` from list responses or masking it.
- **`DuplicateCheckRequest` and `DuplicateCandidate` live in `analysis.rs`** (`analysis.rs:27-38`) as `pub struct` — they are public types in a non-public module, which is inconsistent.
---

## Recommendations (Prioritized)

### High Impact

1. **Wire up response types.** Routes should return `NodeResponse`, `EdgeResponse`, `CredentialResponse` etc. instead of raw DB types. This decouples the API surface from internal data model changes and is essential for backward compatibility.

2. **Add API versioning.** Prefix all routes with `/api/v1/`. This is a one-time migration cost that prevents future breakage. Update `api.md` accordingly.

3. **Remove or mask `secret` from credential responses.** At minimum, omit from `list_credentials`; ideally redact with `"***"` for `get_credential` unless explicitly requested (e.g., `?reveal=true`).

4. **Fix the `source_label`/`target_label` documentation.** The doc comments claim auto-creation but the code only does lookup. Either update the docs or implement auto-creation.
### Medium Impact

5. **Standardize handler return types.** Adopt `AppError` as the unified error type with `IntoResponse`, eliminating the three-pattern inconsistency.

6. **Generate OpenAPI spec.** Use `utoipa` or `aide` to annotate handlers and types. Auto-generate the spec from code so it stays in sync.

7. **Document public types.** Add `///` doc comments to all structs/enums in `types.rs` and all `Database` methods in `db.rs`.

8. **Fix credential validation.** Validate `label` (non-empty), `secret` (non-empty), and handle invalid enum variants with 400 instead of 500.

9. **Standardize language.** Pick English or bilingual for all error messages and doc comments. Current mix of English-only, Chinese-only, and bilingual is confusing.
### Low Impact

10. **Move `check-duplicate` to `nodes.rs`.** It is a node operation, not an analysis operation. The API docs already imply this.

11. **Remove dead response types** or document them as the intended future contract.

12. **Add `CHANGELOG.md`.** Even a minimal log helps consumers track breaking changes.

13. **Use typed query structs everywhere.** Replace `Query<HashMap<String, String>>` in `delete_node` and `search_nodes` with typed structs.

14. **Clarify `content` vs `append_content` precedence.** Document or enforce mutual exclusivity in `UpdateNodeRequest`.
---

## Evidence Index

| Finding | File | Lines |
|---------|------|-------|
| Unused `NodeResponse` type | `src/types.rs` | 138-161 |
| Unused `ProjectResponse` type | `src/types.rs` | 23-38 |
| Unused `EdgeResponse` type | `src/types.rs` | 188-209 |
| Unused `CredentialResponse` type | `src/types.rs` | 398-427 |
| `check-duplicate` in wrong module | `src/routes/analysis.rs` | 15-17 |
| `agent.rs` `pub` vs `pub(crate)` | `src/routes/agent.rs` | 8 |
| Handler return type inconsistency | `src/routes/nodes.rs` | 51 vs 194 vs 241 |
| Chinese-only error messages | `src/routes/nodes.rs` | 99, 120-121 |
| No validation on credential create | `src/routes/credentials.rs` | 35-58 |
| `secret` leaked in responses | `src/types.rs` | 405 |
| Misleading auto-create docs | `src/types.rs` | 180-185 |
| No version prefix on routes | `src/lib.rs` | 814-829 |
| Hardcoded column indices | `src/db.rs` | 90-97 |
| Missing `rustdoc` on core types | `src/types.rs` | 6-164 |
| Missing `rustdoc` on `Database` methods | `src/db.rs` | 186-399 |
| `content`/`append_content` ambiguity | `src/types.rs` | 125-135 |
