launcher/CODE_REVIEW.md

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

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"

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:

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:

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

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

_ => {}        // 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_alls — 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.