# windmark v0.6.1 — Final Audit Report
## 1. Verdict
The library is fundamentally healthy: the default-feature build, 3 unit tests, and 26 doctests all pass cleanly under an aggressive `#![deny(clippy::all, nursery, pedantic, …, warnings)]` lint set, and `rossweisse` clippy passes. The defects are concentrated at two edges — the request-read loop (a genuine wire-protocol correctness bug) and the example/feature packaging (broken `cargo build --examples` out of the box). **Fix first: the request-reading loop in `src/router.rs:126-145` silently corrupts any Gemini request that splits across TCP/TLS reads, rejecting legitimate clients with a 59.**
> **Post-synthesis correction (orchestrator, empirically verified):** the `rossweisse` `#[route(index)]` special-case is broken for its documented usage, not merely whitespace-fragile. See the first HIGH item below. The synthesis pass under-weighted this; it has been promoted.
## 2. Correctness (ranked)
### HIGH — `rossweisse` `#[route(index)]` never mounts at `/` (broken root route)
- **Location:** `rossweisse/src/implementations/router/methods.rs:18-29` (esp. the `arguments == "index"` test at line 24)
- **Impact:** The attribute argument is recovered by stringifying tokens: `attribute.into_token_stream().to_string().trim_end_matches(")]").trim_start_matches("#[route(")`. But `proc-macro2`'s `Display` inserts spaces between tokens, so `#[route(index)]` stringifies to `"# [route (index)]"` — after the trims this yields `"# [route (index"`, which is **never** `== "index"`. The rename to `__router_index` therefore never happens, and the path is built as `format!("/{}", ident)` → the index method mounts at **`/index`, not `/`**. In the `examples/struct_router.rs` capsule, `gemini://host/` falls through to the not-found handler and the index page is only reachable at `/index`. Verified with a standalone `proc-macro2`/`syn` reproduction against the resolved crate versions.
- **Fix:** Stop parsing tokens via strings. Parse the attribute's argument with `syn` instead, e.g. in the `filter_map` use `attribute.parse_args::<syn::Ident>().ok()` and compare that ident to `index`; or match `Meta::List` and read the nested ident. This also fixes the whitespace variants (`#[route( index )]`) for free.
### HIGH — Split-read request corruption
- **Location:** `src/router.rs:126-145`
- **Impact:** The 1024-byte `buffer` (line 121) is never accumulated; each iteration rebuilds `request` from only the current chunk (`&buffer[0..size]`, line 129) and re-parses the URL from it. A request split across reads (e.g. chunk1 `gemini://example.com/path`, chunk2 `\r\n`) parses `Url::parse("")` → `Err(RelativeUrlWithoutBase)` → `or_error!` writes `59 … bad request` and returns; the valid URL is lost. Triggers under normal TLS-record/slow-client conditions.
- **Fix:** Accumulate across reads into one `String`, then parse once when `\r\n` is seen: `let mut accumulated = String::new(); while let Ok(size) = stream.read(&mut buffer).await { accumulated.push_str(std::str::from_utf8(&buffer[0..size])?); if let Some(pos) = accumulated.find("\r\n") { url = Url::parse(&accumulated[..pos])?; break; } }`
### HIGH — `set_ssl_acceptor()` is dead; `run()` always overwrites it
- **Location:** `src/router.rs:681-685` (setter), `522-523` (call), `618-657` (rebuild, esp. `self.ssl_acceptor = Arc::new(builder.build())` at 654)
- **Impact:** `run()`'s first statement `self.create_acceptor()?` unconditionally rebuilds the acceptor from cert/key files, discarding any acceptor set via the documented `set_ssl_acceptor()`. The public method has zero effect under its own documented usage (doc example 666-679).
- **Fix:** Guard the rebuild: add a `manual_acceptor_set: bool` (false by default, set true in `set_ssl_acceptor`) and change line 523 to `if !self.manual_acceptor_set { self.create_acceptor()?; }`.
### MEDIUM — No 1024-byte request-size cap; oversized requests mishandled
- **Location:** `src/router.rs:121-145`
- **Impact:** No cumulative size check (`1024` appears only at line 121 as the buffer size). The spec says servers SHOULD reject >1024-byte requests; instead the buffer is overwritten per read, so an oversized or multi-chunk request is either rejected-by-accident with a 59 or mis-routed from incomplete bytes (nondeterministic by segment boundary). No memory-safety issue.
- **Fix:** Track total bytes in the accumulation loop above; if it exceeds 1024 before a `\r\n`, write `59 …` and `return Ok(())`. Folds naturally into the HIGH fix.
### MEDIUM — Newline injection in error/redirect/input/cert META
- **Location:** `src/router.rs:300-302` — `_ => { format!("{} {}", status_code, content.content) }`
- **Impact:** The catch-all status line for statuses 10/11, 30/31, 40-53, 59, 60-62 interpolates user-controlled `content.content` with no sanitization. `Response::temporary_failure("Error\nLine 2")` emits wire bytes `40 Error\nLine 2\r\n` — a malformed two-line status line, violating spec §3.1 (Status-line is single-line) and enabling log injection. Scope is broader than just errors.
- **Fix:** Take the first line only: `_ => format!("{} {}", status_code, content.content.lines().next().unwrap_or_default())`.
### LOW — Handler runs after a failed TLS `accept()`
- **Location:** `src/router.rs:601-607`
- **Impact:** `if let Err(e) = Pin::new(&mut stream).accept().await { warn!(...) }` does not return; `handler.handle(&mut stream)` runs on the unhandshaked stream. No security/plaintext leak (OpenSSL refuses to write app data pre-handshake; `read`/`write_all` error and propagate) — the real cost is wasted work: pre/post-route hooks and modules with side effects fire, plus a redundant warn+error log pair per dead connection.
- **Fix:** Add `return;` after the `warn!` on accept error.
### LOW — Inconsistent `modules` Mutex poison policy
- **Location:** `src/router.rs:203-207, 257-261` (silent `if let Ok(mut modules) = self.modules.lock()`) vs `951` (`.expect("modules lock poisoned")`)
- **Impact:** A panic in a user `Module::on_pre_route` poisons the std `Mutex`; thereafter every request silently skips all sync module hooks, while `attach()` would panic on the same poison. Only triggerable by a user-module bug; no data/protocol/security impact. (Headers/footers are `Arc<[…]>` slices with no lock — not a poison comparison.)
- **Fix:** Pick one policy. Either make the request path `.expect(...)` to match `attach()`, or accept the graceful-degradation as intentional and leave it.
### LOW — Dead status-23 remap branch
- **Location:** `src/router.rs:266` — `if content.status == 21 || content.status == 22 || content.status == 23 {`
- **Impact:** No constructor or path ever produces status 23 (`serialize_body` only matches `20` and `21 | 22`). Vestigial dead code; remap is harmless.
- **Fix:** Delete the `|| content.status == 23` clause (one OR-clause on one line).
### LOW — `Router::port` is `i32`, accepts invalid ports
- **Location:** `src/router.rs:93` (`port: i32`), `1008` (`set_port(port: i32)`), used in `format!("{}:{}", …, self.port)` at 533-536/539-542
- **Impact:** Negative or >65535 ports are accepted silently and only fail at `run()`/bind time. Startup-detected type-strength footgun; no runtime/protocol/security impact. (The companion `Response.status: i32` enum suggestion is speculative and against the minimal-change philosophy — skip it.)
- **Fix:** `port: u16` and `set_port(port: u16)`.
### LOW — `enable_default_logger()` clobbers prior `set_log_level()`
- **Location:** `src/router.rs:693` (`self.log_filter = "windmark=trace".to_string();`) vs `723-727`
- **Impact:** Calling `set_log_level(...)` then `enable_default_logger(true)` discards the custom filter. The documented order (enable first, then `set_log_level`, doctest 710-716) works; only the reverse order loses config. Verbosity-only, not data/protocol.
- **Fix:** Guard: `if self.log_filter.is_empty() { self.log_filter = "windmark=trace".to_string(); }`.
### MEDIUM (API footgun) — `set_character_set()` / `set_languages()` ineffective for `success()`
- **Location:** `src/response.rs:65-71` vs `src/router.rs:274-281`, setters at `968`/`988`
- **Impact:** `Response::success()` always calls `.with_mime("text/gemini").with_languages(["en"]).with_character_set("utf-8")`, so `content.character_set`/`languages` are always `Some`; the router's `unwrap_or(&self.character_set)` / `map_or_else(self.languages_joined, …)` fallbacks never fire. The router-level setters are silently ignored on the recommended success path (and via `success!`/`success_async!`). Wire output stays spec-valid (utf-8/en), so impact is bounded. The docstring at `router.rs:959` is *accurate* about precedence — it just omits that `success()` always sets the field.
- **Fix:** Either drop the three `.with_*` calls from `success()` so router defaults apply (changes default behavior, deletes ~1 line), or document on `success()` and the setters that per-response values win. Choose deliberately.
## 3. Bloat (ranked by lines saved / clarity gain)
1. **`handler/*` module fragmentation — ~13 lines of pure plumbing, 8 files → 1.** `src/handler.rs` + `response.rs`/`response/{route,error}.rs` + `hooks.rs`/`hooks/{pre_route,post_route}.rs` + `partial.rs` (99 lines total) are each one trait + a single `Fn` blanket impl, glued by `mod`+`pub use` barrels. `router.rs:27-33` already imports all five flat, and the intermediate modules are private — collapsing into one `handler.rs` is API-preserving. **~13 plumbing lines removed, 7 files deleted.**
2. **`Arc<Box<dyn T>>` → `Arc<dyn T>` at 7 sites.** Fields at `src/router.rs:76-77, 89-90, 101-102, 105-106`; creation `Arc::new(Box::new(…))` at `431, 458, 752, 778, 1095, 1115-1117`. The Box is redundant double indirection (`Arc<dyn T>` is already a fat pointer; closures unsize via `CoerceUnsized`). Empirically compiles + passes strict clippy with the Box removed. **Removes one allocation/indirection per handler and 7 `Box::new` wrappers.**
3. **`context/*` fragmentation — 5 files → fewer.** `src/context.rs` barrel + `context/{route,hook,error}.rs` (3 near-identical small structs, ~31/31/25 lines). Co-locate the three context structs in `context.rs`. **~4-8 plumbing lines.** Note: `Parameters` (`context/parameters.rs`) has real behavior (`from_parameters`/`get`/`is_empty`/`iter`) and reasonably keeps its own file.
4. **11 scattered `#[allow(clippy::module_name_repetitions)]` → 1 crate-level allow.** At `context.rs:1`, `context/{error:4,hook:7,route:7}.rs`, `handler/partial.rs:3`, `handler/hooks/{pre_route:3,post_route:3}.rs`, `handler/response/{error:5,route:5}.rs`, `handler/response.rs:1`, `module.rs:4`. Add one `#![allow(clippy::module_name_repetitions)]` to `lib.rs` (note: `lib.rs:1-11` is a `#![deny(...)]` block, so this is a *new* line, not an extension) and delete the 11. The repeated names (`RouteContext`, etc.) are intentional per the naming philosophy. **~10 lines.**
5. **`default_expressions` Vec lifetime-dance in rossweisse.** `rossweisse/src/implementations/router/fields.rs:26-50`: a `mut Vec<syn::Expr>` exists only to extend the lifetime of a freshly-parsed default so both `map_or_else` arms return `&syn::Expr`. Replace with owned exprs in one pass: `.map(|i| i.expr.clone()).unwrap_or_else(|| syn::parse_quote!{ Default::default() })`. **~4-6 lines, clearer intent.**
6. **`handle()` is 197 lines with 3 suppressed lints.** `src/router.rs:115-119` allows `too_many_lines, significant_drop_in_scrutinee, cognitive_complexity` over `handle` (120-317). Genuine multi-responsibility, but the lock-lifetime/borrow interleaving resists clean extraction in Rust — extraction may *add* lines. `significant_drop_in_scrutinee` is an intentional, unrelated suppression. **Low priority; not a guaranteed line win.**
7. **Footer join logic.** `src/router.rs:224-239`: per-item `write!` with `if length > 1 && i != length - 1 { "\n" } else { "" }`. (Format string is `"{}{}"`, *not* `"{}{}\n"`.) Replace with `self.footers.iter().map(|f| f.call(&route_context)).collect::<Vec<_>>().join("\n")`. The `length > 1` guard is itself redundant. **~1-3 lines.**
8. **Request `String` alloc when a `&str` suffices.** `src/router.rs:127-134`: `std::str::from_utf8(&buffer[0..size]).map(ToString::to_string)` allocates the whole request just to `find("\r\n")` and slice. Drop `.map(ToString::to_string)` — `or_error!` works on the `&str`. **1 alloc/request.** (Folds into the HIGH accumulation rewrite.)
9. **Path `Cow<str>` for trailing-slash handling.** `src/router.rs:151-189`: unconditional `to_string()` (151) + reallocs at 157/172/180-183. Use `Cow::Borrowed(url.path())`, only `Cow::Owned` on modification. **1-2 allocs/request on non-default options.** Minor.
10. **One-line dead-code/polish items:** delete commented `// $stream.shutdown().await?;` at `src/router.rs:59`; drop the unused `proc-macro2 = "1.0.56"` direct dep at `rossweisse/Cargo.toml:22` (pulled transitively anyway — tidiness only); the `prelude` feature (`Cargo.toml:27`, `lib.rs:18-19`, `src/prelude.rs`) gates an already-public re-export aggregation that nothing uses — either drop the feature (3 lines + file) or, better for a prelude, ungate it; `#![recursion_limit = "128"]` at `src/lib.rs:13` and `rossweisse/src/lib.rs:12` is verified unnecessary (builds at default 64) — remove or comment the reason.
## 4. Packaging / build
### Examples break `cargo build --examples` / `cargo clippy --all-targets` out of the box
12 examples use feature-gated response macros (`src/response.rs:3` `#[cfg(feature = "response-macros")] mod macros;`) but no `[[example]]` declares `required-features`. Reproduced: 27 build errors (E0432/E0433 "cannot find `success`/`success_async`/`binary_success`/`temporary_failure`/`permanent_redirect`/`certificate_not_valid` in `windmark`"). Affected: `async`, `async_stateful_module`, `binary`, `callbacks`, `certificate`, `default_logger`, `fix_path`, `parameters`, `partial`, `query`, `responses`, `stateful_module`. Add to `Cargo.toml`:
```toml
[[example]]
name = "async"
required-features = ["response-macros"]
[[example]]
name = "async_stateful_module"
required-features = ["response-macros"]
[[example]]
name = "binary"
required-features = ["response-macros"]
[[example]]
name = "callbacks"
required-features = ["response-macros"]
[[example]]
name = "certificate"
required-features = ["response-macros"]
[[example]]
name = "default_logger"
required-features = ["logger", "response-macros"]
[[example]]
name = "fix_path"
required-features = ["response-macros"]
[[example]]
name = "parameters"
required-features = ["response-macros"]
[[example]]
name = "partial"
required-features = ["response-macros"]
[[example]]
name = "query"
required-features = ["response-macros"]
[[example]]
name = "responses"
required-features = ["response-macros"]
[[example]]
name = "stateful_module"
required-features = ["response-macros"]
```
Also fix the two example headers whose documented command omits the flag: `callbacks.rs:1` and `partial.rs:1`.
### `tokio` + `async-std` are mutually exclusive but unguarded; `--all-features` fails with 13 errors
Both define `type Stream` (`src/router.rs:67-70`), `AsyncMutex` (`15-21`), and `pub use … main` (`lib.rs:28-31`) under non-exclusive `#[cfg]`s; `--all-features` produces E0428/E0252/E0061/E0277/E0308/E0382. (Note: the `block!` macro at `router.rs:39-48` is a *single* definition — it contributes downstream type errors, not the E0428s.) Default `["tokio"]` means a user enabling `async-std` without `default-features = false` silently gets both. Add to the top of `src/lib.rs` (after the `#![deny(...)]` block):
```rust
#[cfg(all(feature = "tokio", feature = "async-std"))]
compile_error!("features `tokio` and `async-std` are mutually exclusive; enable exactly one");
#[cfg(not(any(feature = "tokio", feature = "async-std")))]
compile_error!("a runtime feature must be enabled: `tokio` (default) or `async-std`");
```
### `[package.metadata.docs.rs]` missing
`Cargo.toml` has no docs.rs metadata, so published docs at `https://docs.rs/windmark` (line 12) omit the `response-macros`/`logger`/`auto-deduce-mime` API surface. Do **not** use `all-features = true` (it triggers the mutual-exclusion failure above). Add:
```toml
[package.metadata.docs.rs]
all-features = false
features = ["response-macros", "logger", "auto-deduce-mime"]
```
### Doc mismatch
`examples/query.rs:1` and `examples/README.md:96` both say `cargo run --example input …` for the query example. Change both to `cargo run --example query --features response-macros`.
## 5. Deliberate non-issues (do not spend time)
- **Statuses 21/22 as internal success codes** remapped to wire-20 (`router.rs:266`) — intentional windmark extension, correct. (Only the `23` clause is dead.)
- **`module_name_repetitions` suppressions** — the lint is pedantic/style-only; the spelled-out names (`RouteContext`, `RouteResponse`) are deliberate and match the naming philosophy. Consolidating is tidiness, not a fix.
- **`significant_drop_in_scrutinee` on `handle()`** (`router.rs:115`) — covers intentional `self.modules.lock()` / `self.async_modules.lock().await` patterns; not smell.
- **Mutual exclusivity of `tokio`/`async-std`** — intentional design; the gap is only the missing `compile_error!` guard, not the exclusivity itself.
- **Silent skip of module hooks on a poisoned lock** (`router.rs:203`) — defensible graceful-degradation; the only real wart is inconsistency with `attach()`.
- **`Response.status: i32` → enum** — speculative; status codes are a discrete non-interval set and an enum is "more design," against the minimal-change philosophy. The `port: u16` change is the worthwhile half.
- **`recursion_limit = 128`** — could be a deliberate proactive guard; it is merely undocumented and currently unneeded.
## 6. Suggested fix order (quickest-highest-impact first)
1. **Add `[[example]]` + `required-features` to `Cargo.toml`** (§4) — unblocks `cargo build --examples` / `clippy --all-targets`; pure config, ~12 entries. Fix `callbacks.rs:1`/`partial.rs:1` headers and the `query.rs`/`README.md:96` mismatch in the same pass.
2. **Add the `tokio`/`async-std` `compile_error!` guard** (§4) — 4 lines; converts 13 cryptic errors into a clear message.
3. **Rewrite the request-read loop** (`router.rs:126-145`) — accumulate across reads, parse once, enforce the 1024-byte cap, and drop the per-request `to_string()`. Fixes the HIGH split-read corruption + MEDIUM size-cap + a bloat alloc together.
4. **Sanitize error/META status line** — `router.rs:301` → `content.content.lines().next().unwrap_or_default()`.
5. **Guard `create_acceptor()` behind `manual_acceptor_set`** (`router.rs:523`) — make `set_ssl_acceptor()` actually work.
6. **Decide `success()` charset/lang behavior** (`response.rs:65-71`) — drop the `.with_*` calls or document the override.
7. **Add `[package.metadata.docs.rs]`** (§4) — explicit feature set, not `all-features`.
8. **Cheap polish batch:** `Arc<Box<dyn T>>` → `Arc<dyn T>` (7 sites); add `return;` after TLS-accept warn (`router.rs:603`); delete the `|| == 23` clause (266); delete commented shutdown (59); `port: u16`; `enable_default_logger` empty-guard; remove unused `proc-macro2` dep.
9. **Structural consolidation (optional, when touching the area):** collapse `handler/*` (8→1) and the three small `context/*` structs; centralize `module_name_repetitions`; simplify the rossweisse `default_expressions` Vec and the footer join.
Source files of record: `/Users/ebisu/Developer/GitHub/gemrest/windmark/src/router.rs`, `/Users/ebisu/Developer/GitHub/gemrest/windmark/src/response.rs`, `/Users/ebisu/Developer/GitHub/gemrest/windmark/Cargo.toml`, `/Users/ebisu/Developer/GitHub/gemrest/windmark/src/lib.rs`, `/Users/ebisu/Developer/GitHub/gemrest/windmark/rossweisse/src/implementations/router/fields.rs`, `/Users/ebisu/Developer/GitHub/gemrest/windmark/rossweisse/Cargo.toml`.