| 1 |
# mnw-cli AI Anti-Pattern Cleanup |
| 2 |
|
| 3 |
Audit of mnw-cli (Rust SSH server with ratatui TUI) for silent error handling and AI-induced anti-patterns. |
| 4 |
|
| 5 |
**Summary:** 3 MEDIUM, 2 LOW. Zero HIGH. No dead code, no stubs, no string-typing, no `#[allow(dead_code)]`, no `todo!()`/`unimplemented!()`. One memory leak in render code. |
| 6 |
|
| 7 |
## Fixes (MEDIUM) |
| 8 |
|
| 9 |
### M1. Memory leak via `.leak()` in upload render |
| 10 |
|
| 11 |
`src/tui/upload.rs:169` — `staging::derive_title(&sf.filename).leak()` converts an owned `String` to `&'static str` by permanently leaking the allocation. Called on every render of the upload screen for every staged file without metadata. Since the result is immediately consumed by `title.to_string()` on line 188, the leak produces no benefit. |
| 12 |
|
| 13 |
**Fix:** Use owned `String` instead of `&str` reference to avoid the leak: |
| 14 |
```rust |
| 15 |
let title = meta |
| 16 |
.and_then(|m| m.title.clone()) |
| 17 |
.unwrap_or_else(|| staging::derive_title(&sf.filename)); |
| 18 |
``` |
| 19 |
|
| 20 |
### M2. Silent error response body loss in API client |
| 21 |
|
| 22 |
`src/api.rs:217,230` — `resp.text().await.unwrap_or_default()` in `json_response` and `empty_response`. If body extraction itself fails (connection reset mid-read, encoding issue), the error message becomes "HTTP 500" instead of "HTTP 500 — connection reset during body read". Same pattern as GO/BB L1. |
| 23 |
|
| 24 |
**Fix:** Replace with `.unwrap_or_else(|e| format!("[body read failed: {e}]"))`. |
| 25 |
|
| 26 |
### M3. Silent staging file deletion failure after publish |
| 27 |
|
| 28 |
`src/tui/mod.rs:1956` — `tokio::fs::remove_file(file_path).await.ok()` after a successful publish. If deletion fails (permissions, file locked), the file stays in the staging directory, still counting against the staging quota. The user sees it reappear in the upload list and might accidentally re-publish (creating a duplicate item). |
| 29 |
|
| 30 |
**Fix:** Replace `.ok()` with `if let Err(e)` + `tracing::warn!` including the filename. |
| 31 |
|
| 32 |
## Fixes (LOW) |
| 33 |
|
| 34 |
### L1. Silent API data load failures in TUI |
| 35 |
|
| 36 |
`src/tui/mod.rs` — Eight `load_*` functions silently swallow API errors with `.unwrap_or_default()` or `.ok()`: `load_home_data` (line 1962-1975), `load_project_items` (1991), `load_staged_files` (2005), `load_blog_posts` (2053), `load_promo_codes` (2063), `load_license_keys` (2077), `load_transactions` (2107), `load_settings` (2114-2115). When the API call fails, the user sees empty data with no indication that loading failed. Note: `load_analytics` and `load_item_detail` already handle errors correctly by sending `GenericError`/`ItemActionError` payloads. |
| 37 |
|
| 38 |
**Fix:** Add `tracing::warn!` before the fallback in each function. Keep the `.unwrap_or_default()` behavior (showing empty is fine for a TUI). |
| 39 |
|
| 40 |
### L2. Silent git_authorize error body loss |
| 41 |
|
| 42 |
`src/api.rs:863-868` — `resp.text().await.unwrap_or_default()` followed by JSON parse with `.ok()`. If the body read fails, the error becomes a generic "HTTP 403" instead of the actual authorization error message. |
| 43 |
|
| 44 |
**Fix:** Same as M2 — replace with `.unwrap_or_else(|e| format!("[body read failed: {e}]"))`. |
| 45 |
|
| 46 |
## Skipping (intentional design) |
| 47 |
|
| 48 |
**SSH channel cleanup (handler.rs, 14 instances):** All `let _ = handle.data/close/eof/exit_status_request/extended_data` calls in exec_request, SCP error response, and command output. These are fire-and-forget SSH protocol sequences — if the channel is already closed (client disconnected), these fail harmlessly. |
| 49 |
|
| 50 |
**Git subprocess I/O (handler.rs:330, git.rs:106,121,132-133,136-137,140-142):** `let _ = stdin.write_all(data).await` pipes SSH input to git subprocess. Read errors in stdout/stderr forwarding loops break the loop (normal EOF). JoinHandle awaits are cleanup. Exit code `.unwrap_or(1)` handles signal kills (no exit code). All correct. |
| 51 |
|
| 52 |
**TUI event channel sends (~40 instances in tui/mod.rs):** All `let _ = tx.send(AppEvent::DataLoaded(...)).await` are channel sends from background tasks to the TUI event loop. If the receiver is dropped (TUI exiting), these fail, which is expected. |
| 53 |
|
| 54 |
**AppHandle channel sends (tui/mod.rs:111,115):** `let _ = self.tx.send(AppEvent::Input/Resize).await` — fire-and-forget input forwarding. If TUI is shutting down, benign. |
| 55 |
|
| 56 |
**Session close on quit (tui/mod.rs:364):** `let _ = session_handle.close(channel_id).await` — best-effort SSH session close when user presses `q`. |
| 57 |
|
| 58 |
**Terminal resize (tui/mod.rs:428):** `let _ = terminal.resize(rect)` — ratatui terminal resize. Failure means the next render uses the old size, which is harmless. |
| 59 |
|
| 60 |
**Initialization panics (main.rs:92, api.rs:252):** `.expect()` on SIGTERM handler registration and HTTP client construction. Correct — these are process-fatal startup conditions. |
| 61 |
|
| 62 |
**Ctrl+C fallback (main.rs:99):** `ctrl_c.await.ok()` — non-Unix signal handling fallback. |
| 63 |
|
| 64 |
**Git path parsing (git.rs:34,43,51):** `.unwrap_or(path)` / `.unwrap_or(repo_name)` for stripping quotes/prefix/suffix. Correct fallback — returns unmodified input. |
| 65 |
|
| 66 |
**User input parsing (tui/mod.rs:1581,2136-2141):** `.parse().unwrap_or(0)` on discount percentage and price input. Correct — invalid user input defaults to 0. |
| 67 |
|
| 68 |
**JSON serialization (commands.rs, multiple):** `.unwrap_or_default()` on `serde_json::to_vec_pretty()` — infallible for valid `Serialize` types. |
| 69 |
|
| 70 |
**Environment variable defaults (config.rs, 5 instances):** `.unwrap_or_else(|_| ...)` on env var reads with sensible defaults. |
| 71 |
|
| 72 |
**System time fallbacks (staging.rs:58,117):** `.unwrap_or(UNIX_EPOCH)` on metadata.modified(), `.unwrap_or_default()` on duration_since. Platform guarantees make failure impossible in practice. |
| 73 |
|
| 74 |
**Best-effort empty dir cleanup (staging.rs:130):** `let _ = fs::remove_dir(user_dir.path()).await` — removes empty user staging dirs during periodic cleanup. Failure is harmless. |
| 75 |
|
| 76 |
**Display-only formatting (all TUI render files):** `.unwrap_or("...")` / `.get(..10).unwrap_or(...)` on date truncation, tier labels, display names, etc. Pure display fallbacks with no state impact. |
| 77 |
|
| 78 |
**SFTP spawn (handler.rs:218-220):** `russh_sftp::server::run(stream, sftp_session).await` runs in a spawned task without error handling. Session cleanup is russh_sftp's responsibility. |
| 79 |
|
| 80 |
**RUSSH SFTP handler (sftp.rs):** Returns `StatusCode` errors to the SFTP client — correct protocol behavior, not silent swallowing. |
| 81 |
|
| 82 |
## Verification |
| 83 |
|
| 84 |
```sh |
| 85 |
cd ~/Code/MNW/mnw-cli && cargo check && cargo test |
| 86 |
``` |
| 87 |
|