Merge rocm-dash into rocm-cli: unified TUI + chat OAuth + dash demo/replay - #16
Conversation
|
ready to go! |
There was a problem hiding this comment.
Code Review
Architecture is solid and engineering quality is high — the crate layering is clean, the api-key-from-env invariant is structurally enforced, and demo determinism is real. Requesting changes on a handful of objective issues before merging; inline comments mark each one.
Blocking
- Daemon socket security —
_tokenis silently ignored, no socket permissions set after bind, and the default path is world-writable/tmp/rocmdashd.sock. Any local user on a multi-user host can subscribe to all telemetry data. #![allow(dead_code)]on all four new crates — these are now landing in full; a crate-wide suppression hides real dead code and all future rot.- UTF-8 byte-slice panic in
overview.rs—&model[..20]/&r.cell[..10]panic on any multibyte char straddling the boundary;trunc()(char-aware, already imported) is the fix. - Dead module
monitor— declaredpub mod monitorinui/mod.rsbut the file contains only a TODO and is referenced nowhere. Duration::from_secs_f64panic on hostile/typo config — negative or NaN values panic rather than returning a config error.
Non-blocking
LlmConfigderivesDebugwithapi_key: Option<String>in the clear — safe today but a futuretracing::debug!(?cfg)would leak it.- ChatGPT OAuth token cache written without explicit
.token_dir()— lands in~/.config/chatgpt/auth.jsonworld-readable by default.
Positive signals
- api-key-from-env invariant is structurally enforced (single ingestion point, key never reaches wire/persist/replay format — verified at protocol level). ✅
- All six chat agent tools are read-only; mutating actions require the approval modal. ✅
demo.rsdeterminism is real (fixed wall-clock anchor, seeded RNG, byte-identical test). ✅- Config migration is one-shot, non-clobbering, and race-benign. ✅
cfg(unix)gating fails cleanly (explicitErr) everywhere it appears. ✅
Review superseded — internal references removed from branch and re-reviewed.
Fold the standalone rocm-dash telemetry/monitoring app into rocm-cli so that one binary provides the CLI, an interactive TUI, an embedded telemetry daemon, and an LLM chat assistant. `rocm dash` launches the dashboard; `rocm serve <model> --managed` surfaces live gen_tps in it end-to-end. - Unified TUI screens on a shared, pure-reducer base with an async job-bridge and approval gate: services, serve wizard, engine manager, model picker, doctor, update, install, logs, runtime manager, onboarding, automations, freeform command, and config/provider. - Telemetry daemon + collectors (amd-smi, sysfs/cgroup, Docker, vLLM Prometheus/log, lemonade, llama.cpp slots, bench tail) feeding a snapshot/bench ring with persistence and a demo/replay engine. - Managed-service engine registry -> scrape-target seam: the CLI's service records drive collector discovery, with the registry as the authoritative port source for managed services. - Config unification: a dashboard sub-config nested under the rocm-cli config plus a one-shot legacy TOML -> JSON migration onto ~/.rocm. - No-key ChatGPT OAuth chat backend (rig-core native provider, device- code login) behind the swappable agent seam; chat API key stays environment-only across every surface. - `rocm dash --demo/--replay/--chat-mock` for GPU-less and offline use, fully cross-platform including Windows. - Edition-2024 migration of the new crates and a ratcheted llvm-cov coverage gate scoped to them. Notable: live dashboard socket is Unix-only for now (--demo/--replay also run on Windows); the legacy assistant TUI remains until feature parity is reached.
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Restoring inline review comments after cleanup pass that removed internal references from the branch. Blocking issues below are still open; the non-blocking internal-ref items from the prior review have been resolved on the branch.
| /// Benchmark-row history kept for late-joining clients (matches TUI `BENCH_CAP`). | ||
| const BENCH_RING_CAP: usize = 200; | ||
|
|
||
| pub async fn run(listen: &str, _token: Option<&str>, opts: RunnerOptions) -> anyhow::Result<()> { |
There was a problem hiding this comment.
[BLOCKING — security] _token is silently discarded; Command::Hello also discards the client token (~line 139). The DashboardConfig.daemon.token field and the protocol Hello.token field exist — auth was clearly intended. Either enforce it (constant-time compare via the subtle crate) or remove the parameter and document that FS permissions are the ACL.
| if path.exists() { | ||
| std::fs::remove_file(&path).with_context(|| format!("removing stale socket {path:?}"))?; | ||
| } | ||
| let listener = UnixListener::bind(&path).with_context(|| format!("binding {path:?}"))?; |
There was a problem hiding this comment.
[BLOCKING — security] No set_permissions after UnixListener::bind. Combined with the /tmp/rocmdashd.sock default path (world-writable, predictable), any local user on a multi-user host can connect and receive all telemetry. Add std::fs::set_permissions(&path, Permissions::from_mode(0o600))? immediately after this line, or create the parent dir at 0o700. Consider moving the default socket path under paths.telemetry_state_dir() (already ~/.rocm/data/telemetry/, user-owned) instead of /tmp.
| // config keeps its in-place `&mut` mutation convention untouched. | ||
|
|
||
| fn default_dashboard_listen() -> String { | ||
| "unix:/tmp/rocmdashd.sock".to_owned() |
There was a problem hiding this comment.
[BLOCKING — security] World-writable, predictable socket path. Any local user can squat this path before the daemon starts (TOCTOU) or connect to it directly. Move the default to a per-user directory, e.g. paths.telemetry_state_dir().join("rocmdashd.sock") — ~/.rocm/data/telemetry/ is already user-owned by construction.
| //! | ||
| //! See `../wiki/concepts/tea-reducer-pattern.md` for the architectural pattern. | ||
|
|
||
| #![allow(dead_code)] // scaffold; remove as modules flesh out |
There was a problem hiding this comment.
[BLOCKING] Crate-wide #![allow(dead_code)] on all four new crates hides real dead code (e.g. unused ring helpers, Backoff::reset) and will hide all future rot. These crates are landing in full — the scaffold comment no longer applies. Remove this attribute and annotate any genuinely-intentional unused items individually with #[allow(dead_code)] + a reason.
| //! Every collector implements one of the traits in `rocm_dash_core::traits`. | ||
| //! Stubs return `CollectorError::Unsupported` so the daemon can start with nothing wired. | ||
|
|
||
| #![allow(dead_code)] |
There was a problem hiding this comment.
[BLOCKING] Same as rocm-dash-core: remove crate-wide #![allow(dead_code)]. Annotate individual intentional unused items instead.
| model | ||
| }; | ||
| let cell = if r.cell.len() > 10 { | ||
| &r.cell[..10] |
There was a problem hiding this comment.
[BLOCKING — panic] Same: &r.cell[..10] panics on multibyte char boundaries. Use trunc(&r.cell, 10) instead.
| pub mod logs_view; | ||
| pub mod modal; | ||
| pub mod model_picker; | ||
| pub mod monitor; |
There was a problem hiding this comment.
[BLOCKING — dead code] monitor.rs contains only a doc-comment and a TODO — no draw, no on_key, no public API — and is referenced nowhere in the codebase. Delete the file and this declaration until the module is implemented; shipping an empty stub misleads future readers about what's actually available.
|
|
||
| pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<Duration, D::Error> { | ||
| let secs = f64::deserialize(d)?; | ||
| Ok(Duration::from_secs_f64(secs)) |
There was a problem hiding this comment.
[BLOCKING] Duration::from_secs_f64 panics on negative, NaN, or overflow input. A config file with gpu_tick = -1 or gpu_tick = NaN panics at startup instead of producing a user-visible error. Validate before converting:
pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<Duration, D::Error> {
let secs = f64::deserialize(d)?;
if !secs.is_finite() || secs < 0.0 {
return Err(serde::de::Error::custom(
format!("duration must be a non-negative finite number, got {secs}")
));
}
Ok(Duration::from_secs_f64(secs))
}|
|
||
| /// Fully-resolved chat endpoint configuration. `api_key` is sourced from the | ||
| /// environment only — never from TOML/CLI/source (see `main.rs`). | ||
| #[derive(Debug, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
[Non-blocking] LlmConfig derives Debug with api_key: Option<String> in the clear. No logging happens today (the env-only invariant holds), but a future tracing::debug!(?cfg) would silently leak the key. Consider implementing a redacting Debug or wrapping with secrecy::Secret<String>.
| // module is private, so we let inference name it); its `verification_uri` | ||
| // and `user_code` fields are public. | ||
| let client = chatgpt::Client::builder() | ||
| .oauth() |
There was a problem hiding this comment.
[Non-blocking] .oauth() without .token_dir(...) writes the OAuth token cache to ~/.config/chatgpt/auth.json at the process umask (typically 0o644, world-readable). Point the dir at a rocm-owned location created with mode 0o700 so the plaintext access/refresh tokens are not readable by other local users.
What & Why
This PR folds the standalone
rocm-dashtelemetry dashboard into therocmbinary as a unified TUI, adds a no-key ChatGPT-OAuth chat backend, and aligns install/onboarding with thepip→wheelformat rename.Today
rocm-dashlives as a separate project, duplicating config, transport, and engine-discovery logic that therocmCLI already owns. Merging it gives users a single binary (rocm dash) with embedded telemetry, removes the duplication, and lets the dashboard reuse the CLI's serve/model/engine plumbing.What's included
rocm-dash-core— pure reducer, config, metrics, bench rollup/schema, VRAM/partition/efficiency models, protocol, persistence.rocm-dash-collectors— bollard Docker collector, vLLM (Prometheus + log) and Lemonade scrapers, amd-smi/sysfs/cgroup/proc host collectors, engine registry.rocm-dash-daemon— daemon server, runner, registry, snapshot/bench rings, demo session generator, persistence.rocm-dash-tui— Ratatui front end with Overview / Hardware / Instances / Bench / Chat tabs.rocm dashlaunch verb with embedded-daemon auto-start and unifiedconfig.json(with legacyconfig.tomlmigration).agent.rs); chatapi_keyis ENV-only (the OAuth path takes no key).rocm dashflags —--demo(deterministic synthetic session; no GPU/daemon needed, ideal for CI and dev boxes),--replay <FILE>, and--chat-mock.--format wheel.#[cfg(unix)]-gated with graceful Windows stubs; twosh-spawning tests are gated. The live dashboard is unix-only for now;--demo/--replaywork on Windows. Verified on a real MSVC build (0 errors / 0 warnings) plus a windows-gnu cross-check.ROADMAP.md— documentation aligned to the merged state; ROADMAP captures the path to retire the legacy chat-first TUI and the per-process VRAM → model item.Architecture invariants upheld
rocm-dash-corecarries no tokio/ratatui/rocm-core types at the type boundary; the reducer is pure (State::apply → Vec<SideEffect>).api_keyis ENV-only — never read from TOML, CLI flags, source, or logs (the OAuth path takes no key).bollardlives only in the collectors crate; the ureq/reqwest HTTP partition is left intact.agent.rsis the only file that namesrigtypes.Testing
cargo build --workspace --all-targets— clean.cargo test --workspace -- --test-threads=1— 1581 passed, 0 failed.cargo clippy -p rocm-dash-tui --all-targets -- -D warnings— clean; workspace clippy clean.cargo fmt --all --check— clean.Under default parallel
cargo test, two legacy chat tests flake on headless contention but pass in isolation, which is why CI and the gate run single-threaded.Merge note
The branch is pre-squashed to a single signed commit, so any merge method (merge, squash, or rebase) yields exactly one clean commit on
main.