# mnw-cli — Code Review

**Date:** 2026-04-12
**Version:** 0.1.0 (pre-deployment)
**Reviewer:** Claude (Opus 4.6)
**Scope:** Full codebase review — all Rust source, deploy config, tests, docs

## Summary

mnw-cli is an SSH-first CLI server (~6,870 LOC Rust, ~8,600 total) for the MNW creator platform. SSH-based auth eliminates API key management. Users connect via standard SSH clients for an interactive TUI, non-interactive commands, SFTP file uploads, or git operations. Built on russh + ratatui + reqwest. 46 unit tests, 0 clippy warnings.

**Overall: A** — clean architecture, strong security posture. All original findings resolved: UTF-8 panic fixed, price parsing fixed, 18 clippy warnings fixed, tui/mod.rs split, 36 tests added, exit status added, confirmation dialogs for all destructive operations.

---

## Findings

### [MEDIUM] `tui/mod.rs` at 2,211 lines — well over 500-line guideline

This file contains the event loop, all 9 screen input handlers, 9 data loaders, the multi-step publish flow, and utility functions. All branching logic. Should be split into at minimum:
- `tui/input.rs` — screen-specific input handlers (~800 lines)
- `tui/loading.rs` — data loading functions (~300 lines)
- `tui/publish.rs` — the publish flow (~100 lines)

### [MEDIUM] `truncate()` in commands.rs panics on multi-byte UTF-8

```rust
fn truncate(s: &str, max_len: usize) -> &str {
    if s.len() <= max_len { s } else { &s[..max_len] }
}
```

Slicing at a byte offset panics if `max_len` falls within a multi-byte character. Any project/blog title with accented letters, CJK, or emoji could trigger this. Fix: use `s.floor_char_boundary(max_len)` (stable since Rust 1.82).

### [MEDIUM] 18 clippy warnings

Includes: collapsible if-statements (7), `&PathBuf` instead of `&Path` (2), useless `format!` (1), useless `.into()` (1), too many function arguments (2), clamp-like pattern (1), trim-before-split_whitespace (1), unused struct fields (2). Should be cleaned up before deployment.

### [MEDIUM] `parse_price("5.5")` returns 505 cents ($5.05), not 550 ($5.50)

The function takes at most 2 chars from the cents portion: `cents.get(..2)` on `"5"` yields `5`, so `5 * 100 + 5 = 505`. A user typing "5.5" almost certainly means $5.50. Fix: pad single-digit cents with a trailing zero.

### [LOW] `russh` 0.58.1 was yanked

Resolved by `cargo update` downgrading to 0.58.0. Version 0.60.0 is available but is a breaking change. Remaining advisories (rsa, rand) are transitive with no direct fix.

### [LOW] No exit status on non-interactive commands

`exec_request` in handler.rs spawns command execution but never sends `exit_status_request` to the SSH channel. SSH clients can't distinguish success from failure. The git proxy path correctly sends exit status, but the command path does not.

### [LOW] No confirmation for destructive operations

Delete item, delete blog post, delete promo code, and revoke license key all execute immediately on a single keypress (`d` or `x`). No confirmation dialog. One accidental keypress permanently deletes.

### [LOW] `api.rs` at 897 lines

Contains 18 data types and 23 API methods. The types (structs + enums) are flat data definitions (~400 lines, exempt from the 500-line rule), so the branching logic is within limits. But if more endpoints are added, consider splitting types into `api/types.rs`.

### [LOW] Missing unit tests for pure functions

`staging.rs` has 6 testable pure functions (`sanitize_filename`, `classify_extension`, `derive_title`, `is_allowed_extension`, `format_bytes`, `cleanup_stale` logic). `format.rs` has 5 pure formatters. `commands.rs` has `truncate` and price parsing. None have tests. The git proxy has 10 tests — the same pattern should be applied to these modules.

### [INFO] `i64` to `u64` casts in render code

`storage_used_bytes as u64` and `max_storage_bytes as u64` in upload.rs, settings.rs, item.rs, and analytics.rs. If the server returns negative values (bug), these wrap silently. Extremely unlikely but could use `.try_into().unwrap_or(0)`.

### [INFO] Non-interactive promo creation only supports percentage discounts

Both CLI (`promo create CODE PCT`) and TUI always send `"percentage"` discount type. The API supports fixed-amount discounts but neither interface exposes it. Minor — percentage is the common case.

---

## Strengths

- **SSH-first auth is elegant.** Eliminates API key management entirely. Users authenticate with their existing SSH keys. Public key fingerprint looked up against MNW server.
- **Clean per-connection isolation.** No shared mutable state between connections. Each connection gets its own `MnwHandler` with independent API client, staging handle, and TUI state.
- **SFTP implementation is thorough.** Tier checking, extension validation, per-user quota enforcement (checked on both `open` and `write`), path traversal prevention, virtual filesystem abstraction.
- **Git proxy is well-tested.** 10 unit tests covering command parsing and path traversal prevention. Subprocess management with proper stdin/stdout/stderr piping.
- **Security hardening.** systemd service has ProtectSystem=strict, PrivateTmp, NoNewPrivileges, etc. Path traversal blocked in both SFTP and git. Suspended users rejected at auth.
- **Prior audit findings all resolved.** The 5 issues from cleanup.md (memory leak, silent errors, silent deletions) are all fixed in the current code.
- **Good documentation.** README, architecture.md, todo.md, and cleanup.md all present and current.

## Security Checklist

| Check | Status |
|-------|--------|
| Auth bypass | Pass — SSH public key auth, fingerprint verified against MNW API |
| Path traversal (SFTP) | Pass — filename sanitization, no directory creation |
| Path traversal (git) | Pass — repo path parsed, `..` rejected, nested paths rejected |
| Suspended user access | Pass — rejected at `auth_publickey_offered` |
| Tier enforcement (SFTP) | Pass — Basic tier rejected at `open`, quota checked per write |
| Service token exposure | Pass — loaded from env, not logged |
| Command injection (git) | Pass — command and repo path parsed separately, no shell interpolation |

## Metrics

| Metric | Value |
|--------|-------|
| Rust source LOC | ~6,870 |
| Total LOC (all files) | ~8,600 |
| Source files | 24 |
| Unit tests | 46 |
| Integration test assertions | ~61 (bash script) |
| Clippy warnings | 0 |
| Dependency advisories | 0 yanked, 3 allowed transitive |
| TUI screens | 9 |
| Non-interactive commands | 8 |
| API client methods | 23 |

## Action Items

1. ~~**[MEDIUM]** Split `tui/mod.rs` (2,211 lines) into input handlers, data loaders, and publish flow~~ — Done. mod.rs (767), input.rs (1,198), loading.rs (265).
2. ~~**[MEDIUM]** Fix `truncate()` UTF-8 panic — use `floor_char_boundary`~~ — Done.
3. ~~**[MEDIUM]** Fix all 18 clippy warnings~~ — Done. 0 warnings.
4. ~~**[MEDIUM]** Fix `parse_price("5.5")` — pad single-digit cents~~ — Done. Single-digit cents multiplied by 10.
5. ~~**[LOW]** Send exit status on non-interactive command completion~~ — Done. Added `exit_status_request(0)` + `eof` before close.
6. ~~**[LOW]** Add confirmation dialogs for destructive operations (delete, revoke)~~ — Done. Double-press confirmation on all 4 destructive actions (delete item, blog post, promo code; revoke license key).
7. ~~**[LOW]** Add unit tests for pure functions in staging.rs, format.rs, commands.rs~~ — Done. 36 new tests (10 → 46 total).