|
@@ -0,0 +1,281 @@
|
|
|
|
|
+# 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.
|