# Code Review — sm121-launcher

**Scope:** Full codebase review (33 files, ~8.4k LOC) using the code-reviewer
methodology — pillars: Correctness, Maintainability, Readability, Efficiency,
Security, Edge Cases/Error Handling, Testability.
**Baseline:** `make check` passes (fmt, clippy, 100 tests, ≤500 lines). One test
takes 60s real-time (see finding I-7).

## Summary

The codebase is well-structured: clean module separation, trait-based
abstractions for testability (`NodeProber`/`PowerController`/`NodePinger`),
good i18n groundwork, and thoughtful domain modeling (HF cache path refs,
mod entrypoint wrapping). Tests are thorough (100 passing) and the line-limit
discipline is mostly respected.

The main weakness is **consistency of shell escaping**: a `shq()` helper
exists and is correctly used in `docker/tail.rs` + `docker/mods.rs`, but is
absent everywhere else, so `recipe_name` / `cluster.name` / image tags /
paths are interpolated raw into SSH shell strings. There are also a few real
bugs (a UTF-8 panic, a hardcoded `"temp"` path, an infinite build loop), and
several `#[allow(...)]` attributes that violate AGENTS.md.

---

## Critical (bugs / security)

### C-1. Shell injection via unquoted interpolations in SSH commands
`shq()` (single-quote escaper) exists in `docker/mod.rs` but is **not used**
where it matters most. `recipe_name`, `cluster.name`, image tags, user/host
and paths are interpolated raw into command strings that are exec'd through
the remote shell (`channel.exec` → `sh -c`). A recipe/cluster name containing
`"`, `'`, `$`, `` ` ``, or `;` breaks the command or injects shell code.

Affected sites:
- `src/docker/mod.rs` — `stop_cluster` (recipe_name + cluster.name in
  double-quoted `--filter`), `resolve_cluster_by_container_label`,
  `start_cluster` `cleanup` (`docker rm -f sm121_{recipe_name}_{rank}`),
  `resolve_path_ref_on_node` (`hub_dir`/`sub` in single-quoted context).
- `src/docker/logs.rs` — `logs_tail_cmd` / `logs_follow_cmd`.
- `src/docker/deploy.rs` — `discover_recipe_deployments` filter.
- `src/docker/mods.rs` — `upload_mods_to_cluster` remote path
  (`/tmp/sm121-mods-{recipe_name}`), which then hits `upload_dir` (see C-2).
- `src/docker/build.rs` — `build_image_on_node` (recipe_name, image_tag,
  remote_dir, logfile all unquoted in a double-quoted tmux command).
- `src/docker/sync.rs` — daisy-chain `docker save | ssh ... docker load`
  (image, ssh_user, ssh_ip, key path unquoted).
- `src/ssh.rs` — `upload_dir` (`remote_dir` unquoted in `mkdir -p {dir}`).
- `src/tmux/session.rs` — `session_name` interpolated into `-t` args / greps.

Threat model: these values come from trusted `config.toml` / recipe TOML, so
exploitation requires a malicious recipe/config — but the `shq()` helper
exists precisely for this, the inconsistency is the smell, and a recipe name
with a space already silently breaks commands.

### C-2. `before_serve.rs::truncate` panics on multibyte UTF-8
```rust
fn truncate(s: &str, max: usize) -> String {
    if s.len() <= max { s.to_string() }
    else { format!("{}…", &s[..max]) }   // <-- byte slice, panics off char boundary
}
```
`s.len()` is bytes; `&s[..max]` panics if `max` lands inside a multibyte
char. Any non-ASCII pre-serve command (common: Cyrillic/CJK paths, `–` dashes)
with byte length > 120 crashes the launcher. Fix with `s.chars().take(max)`.

### C-3. SSH host-key verification silently skipped when `host_key` is `None`
`ssh.rs::VerifyingHandler::check_server_key` logs "(insecure)" but returns
`Ok(true)` — full TOFU-skip, a MITM vector. `init-host-keys` exists to
populate these, so the safe default is to **refuse** (or pin on first use)
rather than proceed. At minimum the skip should be opt-in, not the default.

### C-4. `docker/build.rs::build_image_on_node` has no build timeout
The poll loop `sleep(30); tmux has-session?` runs until the session dies. A
hung build (stuck tmux, network stall) loops **forever**, hanging the
launcher. Add a configurable max build duration (e.g. default 60–90 min) with
a clean timeout error.

---

## Improvements (correctness / maintainability / standards)

### I-1. AGENTS.md violation: `#[allow(...)]` attributes (forbidden by project rules)
Three occurrences — the project explicitly forbids `#[allow(...)]` and says to
refactor instead:
- `src/orchestrate/mod.rs:22` — `#[allow(clippy::too_many_arguments)]` on
  `prepare_cluster` (8 args). Fix: group `(auto_power_on, prober, power,
  pinger)` into a `PrepareOpts` / `Ends` struct.
- `src/tmux/mod.rs:14` — `#[allow(dead_code)]` on `session_prefix`. The field
  is genuinely unused (`session_name` builds the name directly). Remove the
  field (and the struct field) entirely.
- `src/tmux/mod.rs:63` — `#[allow(dead_code)]` on `is_degraded`. It **is**
  used (`health.rs::classify_status` calls `health.is_degraded()`). The
  attribute is stale — just delete it.

### I-2. `doctor.rs::run_cluster_repair` hardcodes literal `"temp"`
```rust
let dockerfile_path = format!("recipes/{}/Dockerfile.repaired", "temp");
```
Should be `recipe_name`. Worse, the local write is pointless: the content is
already sent to the node via a quoted heredoc, and the file is removed at the
end of the function. Drop the local write entirely (or use `std::env::temp_dir`
if a local copy is ever needed).

### I-3. `doctor.rs` / `docker/build.rs` use `.expect()` (panic) for control flow
- `docker/build.rs:198` `.expect("build-node already validated")` → use
  `ok_or_else(|| bail!(...))` for a clean error.
- `host_keys.rs:80` `.expect("valid TOML config")` → return `Result` (the
  config is already parsed so it's safe today, but a panic is the wrong tool).

### I-4. 60-second test smell — boot timing not injectable
`orchestrate/mod.rs::wait_for_all_nodes` hardcodes `boot_wait_secs=60`,
`poll_interval_secs=10`, `max_poll_secs=120`. The test
`cluster_off_auto_power_on_powers_and_selects` exercises the power-on path,
so it sleeps a **real 60s** (`tokio::time::sleep`) — the whole `cargo test`
run takes 60s because of this one test. Inject the timing (a trait method on
`NodePinger`, or a `BootTiming` struct param) so tests can use 0s.

### I-5. `main.rs::handle_stop` `--power-off` only powers off the first node
`stop` accepts `--node a --node b` (repeatable, `Vec<String>`), but:
```rust
let node_str = if node.is_empty() { None } else { Some(node[0].as_str()) };
crate::power::power_off(config, &cluster, node_str, dry_run).await?;
```
Only `node[0]` is passed to `power_off`. `power_off` itself takes a single
`Option<&str>`. So `stop --node a --node b --off` stops a+b but powers off
only a. Either pass all nodes (change `power_off` to accept `&[String]`) or
document the single-node limitation and validate.

### I-6. `power.rs::power_switch` dead `if dry_run` branches + inconsistent errors
The function returns early on `dry_run` at the top, yet the later HA block
re-checks `if dry_run { ... } else { let _ = ha.turn_off(...) }` — the
`dry_run` arm is unreachable. Additionally the disable phase uses `let _ =`
(swallows errors) while the enable phase uses `toggle_entities` (which logs
them). Collapse the manual disable loop into `toggle_entities("turn_off", …)`
for consistency and remove the dead branch.

### I-7. Inconsistent i18n — ~20 raw strings bypass `fluent_i18n::t!`
89 FTL keys exist, but many user-facing strings are hardcoded (defeating the
6-language localization):
- `main.rs:325` — hardcoded **Cyrillic** `=== Кластер: {} ===` (worst: a
  Russian string in an i18n-aware binary).
- `power.rs`, `docker/sync.rs`, `docker/health.rs`, `tmux/ops.rs`,
  `tmux/session.rs`, `tmux/serve.rs`, `doctor.rs`, `orchestrate/mod.rs` —
  raw English with emoji (`🛑`/`🔌`/`💡`/`⏳`/`🩺`/`✓`/`✗`).

Either route all of these through FTL or drop the i18n system. The mixed
state is the worst option.

### I-8. `main.rs` mid-file `use std::collections::HashSet;`
Placed after `resolve_cluster_for_op`'s doc comment instead of at module top.
Unidiomatic; move to the top `use` block.

### I-9. `config/types.rs::resolved_hf_cache_dir` is convoluted
Asymmetric: non-root early-returns the full path; root sets `home` then falls
through to format. Simplify to one expression:
```rust
let home = if self.ssh_user == "root" { "/root".to_string() }
           else { format!("/home/{}", self.ssh_user) };
PathBuf::from(format!("{home}/.cache/huggingface"))
```

### I-10. `tmux/session.rs::check_session` uses regex `grep -c`, not `grep -Fc`
```rust
format!("tmux list-sessions 2>/dev/null | grep -c '{}'", session_name)
```
`session_name` = `{recipe.name}-{engine}-r{rank}`; recipe names like
`GLM-5.2` contain `.` (regex any-char) → can match the wrong session. Use
`grep -Fc` (fixed-string) or `grep -F -c`.

### I-11. `cluster_state.rs::probe` / `health.rs` silently drop `JoinError`
```rust
_ => {}        // cluster_state
Err(_) => {}  // health
```
A panicked probe task is swallowed and the node treated as absent (→ "not
free"). At least `warn!` the join error so a node disappearing from
selection is diagnosable.

### I-12. `fetch_host_key` lacks the connect timeout that `ssh_session` has
`handle_health --fingerprint` calls `fetch_host_key`, which uses a bare
`russh::client::connect` (no `tokio::time::timeout`), so it hangs on
unreachable nodes while `ssh_session` correctly times out at 5s. Wrap it.

### I-13. `docker/build.rs` hardcoded `"glm52-build-"` session prefix
`session_name = format!("glm52-build-{}", recipe_name)` — a leftover from a
specific model. Should be `sm121-build-{recipe_name}` (generic, namespaced).

### I-14. `PowerController` trait only models power-on
`turn_off` is done directly via `HaClient` in `power.rs`, outside the trait
abstraction that `orchestrate` uses for power-on. Either add `turn_off` to the
trait (and route `power_off` through it) or document why it's asymmetric.

---

## Nitpicks

- **N-1.** Redundant `default_value = "false"` on 7 bool flags in `cli.rs` —
  clap defaults `bool` to `false`; only `default_value_t = true` flags need a
  default. Remove the `"false"` defaults.
- **N-2.** `power_on_if_off(_state: &ClusterState, …)` — unused `_state` param.
- **N-3.** AGENTS.md says files ≤800 (extract at 700) but Makefile/DEVELOPMENT
  enforce ≤500. Align the docs: `docker/command.rs` (495) and `power.rs` (482)
  are at the edge of the stricter limit.
- **N-4.** `docker/mod.rs::start_cluster` spawns all nodes and `join_all`s — a
  failure on node 0 doesn't cancel nodes 1–3 (they still launch, leaving a
  partial cluster). Intentional (reports all failures) but worth a comment.
- **N-5.** `docker/sync.rs` daisy-chain `ssh -o StrictHostKeyChecking=no` for
  the inner hop bypasses host-key verification (documented trade-off). Acceptable
  for trusted clusters; flagged for awareness.

---

## Conclusion

**Request Changes.** The code is high quality overall, but before the next
release I'd want at least C-2 (panic), C-4 (infinite loop), I-1 (the three
`#[allow]` violations — a project-standard hard rule), and the C-1 escaping
sweep addressed. C-3 (host-key skip) is a policy decision — confirm with the
maintainer whether "skip when unconfigured" is intended or should be TOFU/refuse.

---

## Resolution Status

All findings below have been **implemented** (gates green: fmt + clippy +
108 tests in 0.01s + ≤500 lines). Test count went 100 → 108; the suite
dropped from **60s to 0.01s** (the 60s sleep was made injectable).

| # | Finding | Status |
|---|---------|--------|
| C-1 | Shell-escaping sweep | ✅ `shq` moved to `ssh.rs`, threaded through all SSH command builders (stop/resolve/cleanup/path-ref/logs/deploy/build/sync/upload_dir/tmux); +2 regression tests |
| C-2 | `truncate` multibyte panic | ✅ char-based slicing; +4 tests |
| C-3 | Host-key skip → refuse by default | ✅ `ssh_session_with_timeout` bails with an `init-host-keys` hint; `fetch_host_key` exempt |
| C-4 | Build loop no timeout | ✅ `max_build_secs` param (default 90 min) → clean bail |
| I-1 | Three `#[allow]` attributes | ✅ `PrepareRequest` struct; `TmuxManager` → unit struct; stale `is_degraded` allow removed |
| I-2 | `doctor` hardcoded `"temp"` | ✅ pointless local write removed |
| I-3 | `.expect()` panics | ✅ `build.rs` + `host_keys.rs` → `Result` |
| I-4 | 60s test sleep | ✅ `BootTiming` injectable; suite 60s → 0.01s |
| I-5 | `stop --power-off` multi-node | ✅ `power_off`/`power_on`/`select_targets` accept `&[String]`; +2 tests |
| I-6 | `power_switch` dead branches | ✅ routed through `toggle_entities` |
| I-7 | i18n bypass | ✅ ~25 raw strings routed through `t!` (Cyrillic removed); new FTL keys in `en` (others fall back) |
| I-8 | Mid-file `use` | ✅ moved to module top |
| I-9 | `resolved_hf_cache_dir` | ✅ simplified |
| I-10 | `grep -c` regex | ✅ `grep -Fc` fixed-string |
| I-11 | Silent `JoinError` drop | ✅ `warn!` in `cluster_state::probe` + `health.rs` |
| I-12 | `fetch_host_key` no timeout | ✅ 5s connect timeout |
| I-13 | `glm52-build-` prefix | ✅ → `sm121-build-` |
| I-14 | `PowerController` asymmetry | ✅ documented on the trait |
| N-1 | Redundant `default_value="false"` | ✅ removed (7 flags) |
| N-2 | Unused `_state` param | ✅ removed |
| N-3 | AGENTS.md line-limit doc | ✅ aligned to ≤500 |
| N-4 | Partial-cluster comment | ✅ added |
| N-5 | Daisy-chain SSH (sync) | acknowledged — documented trade-off in `sync.rs` |

---

## Fix Plan (prioritized)

| # | Finding | Effort | Risk | Suggested approach |
|---|---------|--------|------|-------------------|
| 1 | **C-2** truncate panic | XS | low | `s.chars().take(max)` + test with multibyte |
| 2 | **C-4** build timeout | S | low | add `max_build_secs` param (default 5400) → `bail!` on exceed |
| 3 | **I-1** remove 3× `#[allow]` | S | low | `PrepareOpts` struct for `prepare_cluster`; delete `session_prefix` field + stale `is_degraded` allow |
| 4 | **I-2** doctor `"temp"` | XS | low | use `recipe_name`; drop pointless local write |
| 5 | **I-3** `.expect()` → `Result` | XS | low | `ok_or_else` in build.rs; return `Result` in host_keys |
| 6 | **C-1** shell-escaping sweep | M | med | thread `shq()` through all SSH command builders; add tests with `"`/`'`/space in recipe_name |
| 7 | **I-4** injectable boot timing | S | low | `BootTiming` struct or trait method; test uses 0s |
| 8 | **I-5** stop `--power-off` multi-node | S | low | `power_off` accepts `&[String]`; pass all nodes |
| 9 | **I-6** power_switch dead branch | XS | low | route disable through `toggle_entities`; drop dead `if dry_run` |
| 10 | **I-7** i18n consistency | M | low | add FTL keys for the ~20 raw strings; route through `t!` |
| 11 | **I-10** `grep -Fc` | XS | low | fixed-string grep |
| 12 | **I-12** `fetch_host_key` timeout | XS | low | wrap `connect` in `tokio::time::timeout(5s)` |
| 13 | **I-13** `glm52-` → `sm121-` | XS | low | rename session prefix |
| 14 | **C-3** host-key policy | S | med | **confirm intent first**; default to refuse or TOFU |
| 15 | **N-1..N-4** nitpicks | XS | low | batch cleanup |

Recommended batching: do **1–5** (the trivial bug/standards fixes) in one PR,
**6** (escaping sweep) as its own PR with tests, **7–9** in a refactor PR,
**10** (i18n) separately (string-review heavy), and **14** only after the
maintainer confirms the host-key policy.