AkurAI-Build

public
Latest change 2a1193541cf008c406be9f347e44150a58a66130 - docs: preserve the architecture review, backlog, and schema policy by Olafur Bui
# Architect Review 1/10 — Rust Language Usage & Idioms

Scope: `src/main.rs` (1120 LOC), `src/server.rs` (1693), `src/db.rs` (1434), `src/artifact.rs` (770), `src/lib.rs` (1), `Cargo.toml`. Read-only review; no files modified.

## Overall assessment
Strong, security-conscious Rust. Good use of `Zeroizing`, `subtle::ConstantTimeEq`, `ensure!` invariants, a single well-documented `#[allow(unsafe_code)]` block with a SAFETY comment (db.rs:589), edition-2024 let-chains, and modern std (`is_multiple_of`, `total_cmp`). Lint policy (`unsafe_code`/`unwrap_used`/`todo` deny) is enforced and honored. The main weaknesses are structural (binary/library split) and error-typing, plus one per-request performance anti-pattern.

## Prioritized findings

1. **P1 — Per-request template environment rebuild and router clone.** `render_page` (server.rs:969-976) does `state.pages.as_ref().clone()` in production, deep-cloning `Vec<Page>` and the templates `BTreeMap` on every fallback request; `render_native_template` (server.rs:1031-1041) then rebuilds a fresh minijinja `Environment` and recompiles every shared template (`template_environment`, server.rs:1354, copies each `Arc<str>` via `to_string()`) per request. Idiomatic fix: build one `Environment<'static>` at `PageRouter::scan` time, store it in `AppState`, and borrow (`&PageRouter` / `Cow`) instead of cloning; only the dev path needs a rescan.

2. **P1 — Library/binary split is inverted.** `lib.rs` exports only `artifact`; `db` and `server` are binary-private (`main.rs:1-2`), so the HTTP surface and database layer are unreachable from integration tests and external consumers, while `main.rs` carries ~500 lines of deployment/copy/sync logic (`deploy`, `sync_tree`, `copy_tree`, main.rs:559-1046) that belongs in a `deploy` module. Move `db`, `server`, and the deployment logic into the library and keep `main.rs` as CLI parsing + dispatch.

3. **P1 — Stringly-typed error classification across the API boundary.** `VectorInputError(String)` (db.rs:65) plus `vector_input` (db.rs:410) flattens `anyhow` chains into strings, and `ApiError::database` (server.rs) relies on `downcast_ref::<VectorInputError>()` to pick 400 vs 500 — fragile if any call site forgets the `.map_err(vector_input)` wrapper. Since `artifact` and `db` are (or should be) library modules, give them typed errors (`thiserror` enum, e.g. `DbError::{Input, Internal}`) and keep `anyhow` for `main.rs` only.

4. **P2 — Secret material copied into non-zeroizing buffers.** `load_secret` (db.rs:414-426) reads the key into a plain `String`, then `value.trim().to_owned()` — both the read buffer and the trimmed copy predate the `Zeroizing` wrapper, so intermediates are dropped unwiped. Read into `Zeroizing<String>` and trim in place (truncate) to keep the zeroize guarantee end-to-end.

5. **P2 — Duplicated CLI mirror enums and manual mapping.** `ArtifactMode`/`ArtifactTrailingSlash` (main.rs:197-208) duplicate `artifact::Mode`/`TrailingSlash` with a hand-written match in `artifact_options` (main.rs:361-377). Implement `From<ArtifactMode> for Mode` (compile-time exhaustive) or derive `ValueEnum` directly on the library enums to remove the drift risk.

6. **P2 — `DeploymentManifest.mode` is stringly-typed.** main.rs:264 stores `mode: String` validated by `matches!(manifest.mode.as_str(), "native" | "static")` (main.rs:861) and compared with `manifest.mode == "static"` elsewhere. A `#[serde(rename_all = "lowercase")] enum DeployMode { Native, Static }` makes invalid states unrepresentable and centralizes the check.

7. **P2 — `expect` panic paths in `artifact::resolve`.** artifact.rs:201, 213, 219 use `contains_key` + `expect("file existence checked before resolution")`. Safe today (in-memory map), but `unwrap_used` is denied while `expect_used` is not — restructure to a single `match file_resolution(...)?` so the panic path disappears, or deny `clippy::expect_used` for non-test code.

8. **P2 — ~80 lines of duplicated response assembly.** `static_file_response` (server.rs:440) and `public_asset` (server.rs:794) duplicate If-None-Match/ETag/Content-Length/Cache-Control/HEAD handling with minor divergence (Vary header present only in the static path). Extract one `cached_file_response(file, content_type, cache_control, method, headers)` helper.

## Notes
- Lint config in Cargo.toml is good; consider adding `clippy::expect_used = "warn"` and `rust.missing_debug_implementations`.
- `is_better` on `&(String, f32)` tuples (db.rs) would read better as a tiny `Candidate` struct — cosmetic only.
- Tests are substantive and idiomatic (`Result`-returning tests, `#[cfg(unix)]` gating).