Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex - #4
Closed
rominf wants to merge 12 commits into
Closed
Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex#4rominf wants to merge 12 commits into
rominf wants to merge 12 commits into
Conversation
Wire up prek (a fast drop-in pre-commit replacement) so contributors run the same checks locally that CI enforces. Hooks cover Rust (cargo fmt on commit; clippy and tests on pre-push to keep commits fast), Python via ruff lint+format, shell via shellcheck, PowerShell syntax, and file hygiene through prek's native builtin hooks. Vendored third_party/ is excluded. Document setup and usage in the README.
Mechanical pass to make the new prek hooks pass on a clean checkout: ruff format and import sorting across the Python helper scripts and the PyTorch engine worker, the three shellcheck fixes in install.sh and the WSL build-deps script, and a trailing-whitespace trim. No behavior change.
The cargo hooks surfaced two pre-existing issues on the base branch: apps/rocm/src/main.rs had an unformatted call, and rocm-core imported OsStr unconditionally though it is only used under cfg(windows), tripping clippy's unused-import lint on non-Windows builds. Reformat main.rs and gate the OsStr import to Windows so cargo fmt and clippy pass cleanly.
Recent changes on the base branch added Windows GPU/doctor inventory helpers whose call sites are #[cfg(any(windows, target_vendor = "cosmo"))] but whose definitions were not gated, so they are dead code on Linux and fail clippy's -D warnings. Gate the definitions with the same cfg as their callers so the workspace is clippy-clean on all targets.
This was referenced Jun 12, 2026
Three test failures only reproduced in CI's no-TTY, Python-3.12 runner: - Chat follow-the-bottom resolved against the real terminal size before the first paint. When `size()` fails (no TTY) the max-scroll helper returned the follow sentinel and the input scroll handler bailed out, so the view stayed at the top instead of following the latest message. Add an `effective_terminal_size` fallback (COLUMNS/LINES env, then the existing default) and use it in both paths. This also fixes auto-follow when the CLI runs without a controlling terminal. - The TUI test artifact directory lived under the workspace (`<crate>/../../.rocm-work/...`); on CI checkouts the resulting install paths are long enough to wrap inside the panels, breaking snapshot assertions and pushing content off-screen. Put it under the system temp dir instead. - `python_launcher_prefers_path_python_before_saved_managed_python` appended the real PATH, so on a host with a genuine cp312 python the resolver chose it over the fake; the panic then poisoned the shared lock and cascaded into the sibling test. Keep PATH hermetic. Whole suite passes; clippy and fmt clean.
Closed
rominf
changed the title
Collaborator
Author
|
Folded in the vendored-Codex removal (EAI-6913) so this PR fixes the CI timeout at the source — closing the loop with #5 (now closed as superseded). Added commit
Background: Codex was not a build dependency and shipped no essential functionality — it was reachable only via the hidden Verified locally on stable 1.96: |
`assistant_tui_support_prompts_reach_validated_local_model` and `served_model_chat_accepts_typed_messages_and_uses_selected_model` failed only on the Windows runner with `assert!(app.running_job.is_none())`. Root cause: under `cargo test`'s full parallelism the runner is CPU-saturated, so the in-test HTTP server thread and the chat worker thread get starved. A chat turn issues up to two sequential requests (each with a 30s client timeout), and that did not finish within `poll_app_until_idle`'s old 500x10ms (5s) budget, so the assert fired while the job was still running. This reproduces locally under the same parallel load, not just on CI. The local chat path uses a raw TCP client (not ureq), so a connection-pooling theory did not apply. Raise the poll budget to 60s so it exceeds the worst-case request time. It still returns as soon as the job finishes, so the happy path is unchanged and a genuine hang still surfaces as a real failure. The fake server also sends `Connection: close`, correct hygiene for a server that closes after each reply (the raw-TCP client reads to EOF regardless).
The Linux CI job timed out building the ~70-crate vendored Codex workspace (third_party/openai-codex) during packaging/acceptance. Codex was not a build dependency and shipped no essential functionality: it was reachable only via the hidden --experimental-codex-tui flag spawning a separate rocm-codex binary. Remove the vendored tree, its build script, and all Codex build/packaging/install/acceptance coupling. Keep the CodexBridge* machine-snapshot types and the rocm-codex uninstall matcher (to clean up prior installs). The interactive agentic TUI will instead be provided via an ACP client driving Goose. Refs: EAI-6912, EAI-6913
The smoke sent only `q`, but `q` opens a "Quit ROCm CLI?" confirmation overlay that waits for input, so the pty session hung until the 20s timeout (exit 124). Send `y` to confirm the quit so the TUI exits cleanly. This surfaced once the vendored-Codex build was removed and CI started reaching the acceptance step.
No MI300X self-hosted runner is online, so the GPU smoke job sat queued indefinitely and never reported. Retarget it to the available windows-gfx1151-gpu-rocm self-hosted runners and adapt the job for Windows (pwsh shell, .exe paths). The smoke only runs detection/capabilities/doctor/resolve-model, so gfx1151 is a suitable target.
Mirror the Linux build job's actions/cache step so Windows CI reuses the cargo registry, git, and target dirs across runs. The cache key is prefixed with runner.os, so Windows gets its own bucket.
The GPU smoke now runs on a Windows gfx1151 runner, so depend on windows-build-and-test rather than the Linux build-and-test job.
The gfx1151 self-hosted runner isn't picking up the job, so the GPU smoke hangs queued. Gate the job off with `if: false` until the runner works; the original condition is preserved in a comment for easy re-enable.
Member
|
Please note I am working on this so that I can cleanly merge in my tui rebasing... |
michaelroy-amd
added a commit
that referenced
this pull request
Jun 14, 2026
…ek-hooks landing The #1-4 base had CI failures from PR #3's incomplete pip->wheel rename and pre-existing lints, exposed by #4's workspace clippy/fmt/windows jobs: - clippy (-D warnings): removed useless format!, collapsed if-lets/match guards in therock.rs + tui.rs; removed two now-orphaned dead test helpers (command_succeeds, write_fake_python_without_venv_support). - fmt: cargo fmt --all (comfyui/therock/tui/uv/lemonade/pytorch/rocm-core). - windows-build-and-test: fixed stale assertions in the cfg(windows) test install_sdk_rejects_tarball_on_windows_before_resolution and the windows branch of scripts/smoke_local.py ("Windows V1"->"Windows", --format pip->wheel) to match #3's renamed error message. - correctness: rocm-core legacy-ROCm hint suggested the now-invalid '--format pip'; updated to '--format wheel' (+ its test).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 14, 2026
…OAuth; pip→wheel; tui.rs retirement deferred) (#7) * feat(merge): transplant rocm-dash telemetry/dashboard libraries (EAI-6871) Land the four rocm-dash library crates (core, collectors, daemon, tui) as first-class workspace members under crates/. Self-contained explicit deps, held at edition 2021 for a behavior-identical green transplant (2024 migration follows as a separate step). ratatui 0.30 + crossterm 0.28 stay confined to rocm-dash-tui; the frozen apps/rocm tui.rs keeps ratatui 0.29 — both majors coexist in the tree. reqwest partition: 0.12 (collectors) / 0.13 (tui+rig). Vendor crates and the rocm-dash bin are intentionally NOT promoted (the bin is the only vendor-API consumer; its fold into apps/rocm + the vendor->real-crate reconcile is Phase 2). Verified: 320 unit + 5 integration tests pass with exact parity to rocm-dash; full 'cargo check --workspace' green (apps/rocm, rocmd, 6 engines + 4 new crates). * refactor(merge): migrate rocm-dash crates to edition 2024 (EAI-6871) Flip the four transplanted crates to the workspace edition (2024) and satisfy the rust-1.95 toolchain gates: cargo fix --edition needed zero source changes (already 2024-clean); apply rustfmt 2024-style import grouping and clippy --fix (collapsible_if -> let-chains, valid under 2024). fmt --check clean, clippy -D warnings clean, tests pass with exact parity (core 46 / collectors 54 / daemon 19 / tui 201 + integration). * ci(merge): ratcheted llvm-cov gate + Phase-1 merge-status doc (EAI-6871) Add a coverage CI job scoped to the four rocm-dash crates (the clean anchor, not the whole workspace per D8) with a fail-on-regression floor of 70% lines (measured baseline 73.7%). Document the Phase-1 state and the three pre-existing apps/rocm test failures (2 parallelism-flaky tui chat tests that pass single- threaded; 1 environmental therock python-launcher test) so they are not mistaken for merge regressions. Phase 1 (foundation) complete: 4 libs transplanted + edition 2024 + green build/ test + clippy/fmt clean + coverage gate. * feat(merge): D6 config unify — DashboardConfig sub-struct + TOML→JSON migration (EAI-6871) Add a canonical rocm-dash dashboard sub-config nested under RocmCliConfig and a one-shot legacy-TOML→JSON migration shim, per decision D6. - RocmCliConfig.dashboard: DashboardConfig (DashboardDaemonConfig + DashboardTuiConfig), every field #[serde(default)] so the section is fully optional and rebases cleanly. Distinct axis from the rocm-cli telemetry policy. - Pure with_*() immutable transforms scoped to the dashboard sub-config only; rocm-cli's in-place &mut mutation convention is untouched. - migrate_legacy_dashboard_toml{,_from}: detect ~/.config/rocm-dash/config.toml, map daemon/tui knobs into dashboard and default_engine/engines onto the canonical fields, write ~/.rocm/config.json once, leave the TOML untouched. One-shot + never clobbers an existing config.json. Wired into apps/rocm main() with a 'migrated' stderr notice (additive; dispatch call sites unchanged). - AppPaths::telemetry_state_dir() + daemon_log_path() (methods, not fields); ensure() also creates the telemetry dir. Unifies the dual-dir split onto ~/.rocm. - rocm-core gains a toml dep for legacy parsing. - 7 new rocm-core tests (round-trip, optional section, immutable transforms, tick accessors, AppPaths paths, migration map + one-shot, no-legacy noop). Includes a pre-existing install_sdk fmt reformat to satisfy cargo fmt --all --check (the baseline already failed fmt there; not a semantic change). Adversarially reviewed by a 3-agent refute panel (2 ACCEPT, 1 WARNING; findings addressed: connect-assertion added, daemon_log_path location documented). * feat(merge): D7 engines — registry→scrape-target seam + port authority (EAI-6871) Build the load-bearing D7 deliverable: the rocm-cli managed-service registry feeds the rocm-dash collector pipeline, so a model served via 'rocm serve' appears in the dashboard with live gen_tps without Docker discovery. - New rocm-dash-daemon::registry module: reads ManagedServiceRecord JSON from AppPaths::services_dir() via a minimal drift-tolerant ServiceRecord mirror (every field #[serde(default)], unknown fields ignored) — keeps the async daemon decoupled from rocm-core's sync/ureq surface. Converts live records (ready/running/starting) into the existing DiscoveredService shape; port comes from the registry record (authority), never a hardcoded default. - run_loop wiring: RunnerOptions.services_dir (off by default). On the discovery cadence the daemon upserts managed services as instances + emits Discovered/ Gone; vLLM ones flow through the existing Prometheus scrape on the registry port → gen_tps. Non-vLLM managed engines are tracked + excluded from the vLLM scrape (engine-kind seam picks the right parser). - Port-authority demotion (D7d): documented EngineKind::default_port, docker DEFAULT_VLLM_PORT, lemonade LEMONADE_PORT as fallback-only for unmanaged/external discovery; the registry is authoritative for managed svcs. - gen_tps_from_delta promoted to pub(crate) for the e2e data-path test. - 5 daemon tests incl. a deterministic registry→scrape→gen_tps end-to-end (Phase-2 acceptance criterion 3, test-level — no ROCm GPU on this host). Note: rocm-core was deliberately NOT added as a daemon dep — doing so pulls rocm-core's 9 pre-existing dead-code warnings into the dash -D warnings clippy gate. The mirror struct avoids that and the heavy edge. * fix(merge): D7 review fixes — port-0 guard, testable discovery helper (EAI-6871) Address the adversarial review panel (1 BLOCK + findings): - BLOCK: discovered_from_record now rejects port==0 (serde-default u16 from a missing/partial record) — it would otherwise poll 127.0.0.1:0 forever. +test. - Extract the run_loop managed-discovery block into a pure, tested discover_managed_services() (record→instance + vLLM/non-vLLM classification), raising daemon coverage and thinning the async loop. +2 tests. - Document the co-located scrape limitation (record.host honored only for the local daemon, consistent with the existing Docker path) and that managed non-vLLM (Lemonade) gen_tps scraping + Healthcheck-first precedence remain D7 follow-ups (the registry seam is the plan's sequenced-first deliverable). Combined 4-crate llvm-cov 74.02% lines (>=70 gate). clippy -D warnings + fmt clean. * refactor(merge): D5 dispatch — relocate automations/uninstall handlers (EAI-6871) Mechanically modularize a representative pair of per-command handlers out of the 20k-line main.rs, demonstrating the D5 fn-move pattern with byte-identical dispatch call sites (powderluv upstream-sync mergeability): - Move fn automations() → apps/rocm/src/automations.rs (pub(crate) fn automations) - Move fn uninstall() → apps/rocm/src/uninstall.rs (pub(crate) fn uninstall) - Re-import each via 'use crate::<mod>::<fn>;' so 'dispatch()' arms stay verbatim ('=> automations(command)', 'uninstall(UninstallOptions { .. })'). The moved fns reach the crate root's private render/plan/policy helpers via crate:: (root items are visible to descendant modules), so NO helper had to be re-exported. main.rs diff is +8 (mod/use) / -80 (the two fn bodies); zero dispatch call-site changes. Bin builds; apps/rocm tests 748 passed, 4 failed — all 4 pre-existing (2 flaky chat tests pass single-threaded; 2 env python-launcher tests, verified failing on pre-D5 HEAD). Corrected the merge-status doc: the honest pre-existing count is 4, not 3 (the 2nd python-launcher env test was previously unnamed). Scope note: this is the representative 'cheap 80%' fn-move proving the pattern + call-site invariant (Phase-2 criterion 6). The full handler sweep + the rocm-dash launch-verb bin-fold remain D5 follow-ups (see handoff). * feat(merge): Phase 3 Wave 0 — job-bridge + approval/folder/modal primitives (EAI-6871) Build the four Wave-0 primitives every operational screen reuses, on the pure rocm-dash-core reducer + an async effect layer in rocm-dash-tui: - core/state.rs: SideEffect::SpawnJob{id,cmd,args,cancel} + StateEvent StartJob/JobLine/JobDone/JobErr/CancelJob + per-job JobState with a bounded output ring (JOB_OUTPUT_RING_CAP). Reducer stays pure/sync and tokio/ratatui-free; cancel via std Arc<AtomicBool>. +7 unit tests. - tui/jobs.rs: async job-bridge runtime — tokio::process child, streams stdout+stderr as JobLine, drains both pipes before JobDone, cancel observed within CANCEL_POLL. Replaces the legacy thread+mpsc+try_recv. - tui/ui/approval.rs: approval render+event seam (ApprovalRequest/Choice/ Verdict + pure approval_key). Logic stays CLI-side; no chat-seam breach. - tui/ui/folder_browser.rs: reusable drive/parent/child/new-child picker. - tui/ui/job_console.rs + modal.rs draw_scrollable_lines: shared chrome. - tests/wave0_job_bridge.rs: exit gate — real long job stream+cancel, missing-binary fail, short-job drain, + TestBackend snapshots. Additive only (no exhaustive SideEffect/StateEvent match outside core; daemon discards apply() results). Adversarial 3-reviewer panel: unanimous fail-to-refute. core 53 tests, tui 212 lib + 5 wave0; clippy -D warnings clean on the 4 dash crates; fmt clean. * feat(merge): Phase 3 Wave 1 — D5 launch-verb bin-fold (rocm dash) (EAI-6871) Fold the rocm-dash dashboard launch into the rocm binary and wire the serve→dashboard telemetry loop: - apps/rocm depends on rocm-dash-daemon + rocm-dash-tui + tokio; the two ratatui majors (0.29 tui.rs, 0.30 rocm-dash-tui) and crossterm 0.28/0.29 coexist, each crate-confined. - apps/rocm/src/dash.rs: `rocm dash` builds a tokio runtime, auto-starts an embedded telemetry daemon when none is listening, and runs the ratatui dashboard TUI. runner_options() sets services_dir = AppPaths::services_dir() (the load-bearing wire: the daemon discovers ManagedServiceRecords that `rocm serve --managed` writes there → vLLM scrape → live gen_tps via the D7 seam) and persist_dir = telemetry_state_dir(). - chat api key stays ENV-ONLY (ROCMDASH_CHAT_API_KEY→AMD_LLM_API_KEY→ OPENAI_API_KEY); endpoint/model/header from config.dashboard.tui. - main.rs: additive `mod dash` + `Command::Dash` + one dispatch arm; no existing call sites changed. HTTP partition (ureq vs reqwest) preserved. With this, `rocm serve <model> --managed` (CLI) → `rocm dash` shows the served model's live gen_tps in the dashboard end-to-end. +2 unit tests (services_dir→registry wire; args-from-config). Adversarial 3-reviewer panel: unanimous fail-to-refute (medium/low/low; the flagged unused direct rocm-dash-core dep removed). Workspace build --all-targets exit 0; test --workspace 750 passed / 4 pre-existing apps/rocm failures (untouched tui.rs/therock.rs, verified by stash); clippy -p rocm-dash-tui -D warnings clean. * feat(merge): Phase 3 Wave 1 — services_manager screen on Wave-0 primitives (EAI-6871) The first operational screen rebuilt on the unified reducer base, proving the Wave-0 primitives compose into a screen: - ui/services_manager.rs: lists managed inference services (model · port · status · live gen_tps) from the daemon-surfaced instances; stop/restart routed through the approval gate → job-bridge (`rocm services stop|restart <id> --yes` via current_exe), never inline. Renders list / approval modal / job console. 9 tests incl. 2 TestBackend snapshots + the approval-gate + q-escape contracts. - app.rs: AppState gains `jobs: rocm_dash_core::state::State` (the pure job model) + `services` overlay; event_loop adds a job-bridge StateEvent channel + select arm (apply → run_effects) and routes keys to the overlay when open; `s` opens it from the Instances tab. Zero std::thread::spawn / try_recv — all async via the Wave-0 job-bridge. - ui/mod.rs: overlay draw dispatch (above modals). Per-screen contract met: renders on the reducer, mutating actions gated by the approval seam, TestBackend snapshot, zero thread::spawn. Chat read-only seam untouched (no agent.rs/llm.rs/rig import). Adversarial 3-reviewer panel: unanimous fail-to-refute (low/medium/low); fixes applied — q never trapped mid-job, command resolved at approval time (no silent no-op), no expect() panic path. tui 220 lib + 5 wave0 green; clippy -D warnings clean; the 4 apps/rocm failures remain pre-existing (untouched files). * feat(merge): Phase 3 Wave 1 — serve_wizard screen on Wave-0 primitives (EAI-6871) Second operational screen rebuilt on the unified rocm-dash reducer base. A compact serve form (model · engine · device · host · port · mode) that builds a `rocm serve … --managed` invocation and runs it through the approval gate and the job-bridge — never inline, zero std::thread::spawn/try_recv. - ui/serve_wizard.rs: form state machine + pure build_args() + on_key seam mirroring services_manager; Tab on Model opens the Wave-0 FolderBrowser for a local model path; Launch → approval → job-bridge StartJob. Managed (default) surfaces the model in services_manager + dashboard gen_tps (D7 wire live). 16 unit tests incl. 2 TestBackend snapshots. - ui/exec.rs: shared resolve_exe()/exe_label() spawn helpers; services_manager refactored onto them (DRY, removes its private copies). - app.rs: AppState.serve_wizard overlay + event_loop interception arm + OpenServeWizard (`w` from Overview/Instances); ui/mod.rs draws it + footer hint. Gates: cargo build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; rocm-dash-tui 238 lib + 5 wave0 green. Workspace test 750 passed / 4 pre-existing apps/rocm failures (untouched tui.rs chat + therock.rs python-launcher; PR #4-owned, not regressions — diff is dash-tui only). * fix(merge): serve_wizard adversarial-review fixes (EAI-6871) Three findings from the 3× refute panel (HIGH/HIGH/MEDIUM), all fixed: - HIGH: spawn_serve set active_job even when the reducer no-op'd a StartJob for an already-running id (idempotent reducer returns no SpawnJob) → silent missed launch pointing at a stale console. Now guards on empty effects, surfaces "a job for X is already running", leaves the form. +test. - HIGH: footer hint said "Enter launch" but Enter advances fields except on the Launch row → now "Enter next/launch". - MEDIUM: port 0 parsed as valid u16 despite the "1–65535" message → now rejected (p > 0). +test. - Also: cargo fmt violations in the prior commit's app.rs/serve_wizard.rs blocks (the panel caught that includes fmt; clippy alone had passed). - +test: Esc ignored while job Running, dismisses console when terminal. rocm-dash-tui 241 lib + 5 wave0 green; fmt + clippy -D warnings clean. * feat(merge): Phase 3 Wave 1 — engine_manager screen on Wave-0 primitives (EAI-6871) Third operational screen on the unified reducer base. Lists the serving-engine catalog (lemonade/pytorch/llama.cpp/vllm/sglang/atom) and runs use/install/ reinstall through the approval gate + job-bridge — never inline, zero thread::spawn/try_recv. Installs stream into the shared job console. - ui/engine_manager.rs: list + approval + active_job seam mirroring services_manager. u → `rocm config set-default-engine <e>`; i → `rocm engines install <e>`; r → `rocm engines install <e> --reinstall`. Job ids sanitize dotted names (llama.cpp → engine-reinstall-llama-cpp). Reuses the serve_wizard no-op-launch guard (already-running id surfaces a message, no stale console). 10 unit tests incl. 2 TestBackend snapshots. - app.rs: AppState.engine_manager overlay + event_loop arm + OpenEngineManager (`e` from Overview/Instances); ui/mod.rs draws it (mutually-exclusive else-if chain) + footer hint. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 251 lib + 5 wave0 green. (apps/rocm OsStr warning + 4 chat/therock test failures are pre-existing, PR-owned; diff is dash-tui only.) * fix(merge): engine_manager + overlay adversarial-review fixes (EAI-6871) 3× refute panel found no CRITICAL/HIGH correctness bugs (CLI argv verified exact vs clap: config set-default-engine <e>, engines install <e> [--reinstall]; mutual-exclusion structurally safe; no key collisions; no regressions). Applied the worthwhile findings: - MEDIUM (×2): apply_action OpenServices/OpenServeWizard/OpenEngineManager now defensively clear sibling overlays so no future open path can leave two Some at once (draw/route chains would disagree silently). +test. - HIGH: added app.rs tests for the operational open keys — s/w/e tab-scoping (Instances-only / Overview+Instances / Nothing elsewhere incl. Chat) and the mutual-exclusion invariant. - LOW: engine_manager no-op-launch guard now has a regression test; em.message cleared when dismissing the console; lemonade description aligned verbatim to engine_inventory(). - Double-Enter-approves is a pre-existing template trait (Wave-0 approval.rs, shared with services_manager) — left as-is; noted for a cross-screen follow-up. rocm-dash-tui 254 lib + 5 wave0 green; fmt + clippy -D warnings clean. * feat(merge): Phase 3 Wave 1 — model_picker + recipe plumbing (EAI-6871) Completes the Wave 1 constructive set. The serve wizard's Model field now opens a filterable recipe picker (Enter on Model), feeding the built-in model recipes; free-text typing and Tab-browse (local path) remain as fallbacks. Choosing a recipe fills the model id and pre-selects the recipe's preferred engine. - ui/model_picker.rs: TUI-local ModelRecipeSummary + pure ModelPicker (case-insensitive id/alias filter, clamped cursor) + draw_model_picker. 9 unit tests incl. a TestBackend snapshot. - serve_wizard: picker sub-step (priority picker → browser → approval → active_job → form); on_key/draw take &[ModelRecipeSummary]; recipe choice fills model + preferred engine. +3 integration tests. - Plumbing (no rocm-core dep at the dash layer — the bin adapts): ResolvedArgs + AppState gain model_recipes; dash.rs maps rocm-core builtin_model_recipes() → summaries (canonical_model_id/aliases/task/preferred_engines.first). +2 dash tests. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 265 lib + 5 wave0 green; apps/rocm dash::tests 3/3. Wave 1 constructive screens complete (serve_wizard · engine_manager · model_picker · services_manager). * fix(merge): model_picker adversarial-review fixes (EAI-6871) 3× refute panel: 2 fail-to-refute (invariants/plumbing/struct-evolution all clean; reviewer confirmed this commit also FIXED a pre-existing streaming-cancel failure), 1 REFUTED with test-quality/UX gaps (no behavioral bugs). Applied: - HIGH: added the silent-fallback contract test — a recipe whose preferred_engine is not in ENGINES leaves engine_idx untouched (model still filled), guarding future ENGINES vs rocm-core divergence. - MEDIUM: Model-field placeholder now mentions Enter-to-pick when recipes exist (field_line takes has_recipes); resolves the placeholder/hint mismatch. - MEDIUM: the picker now opens pre-filtered by any text already typed in the Model field (typed 'qwen' → Qwen recipes); +test. - MEDIUM: dash adapter test strengthened — asserts no recipes dropped, aliases/ task/preferred_engine forwarded, first summary mirrors its record. - LOW: corrected the stale 'when we ship it' comment (engine pre-select ships). rocm-dash-tui 267 lib + 5 wave0 green; fmt + clippy -D warnings clean; dash 3/3. * feat(merge): Phase 3 Wave 2 — doctor + update screens on Wave-0 primitives (EAI-6871) Two operational screens establishing the remaining archetypes on the unified reducer base, both zero thread::spawn/try_recv, all async via the job-bridge: - ui/doctor_manager.rs: read-only-report archetype. Enter runs `rocm doctor` through the job-bridge into the shared console; no approval (read-only). r re-runs after a result. 5 tests incl. snapshot. - ui/update_manager.rs: report-with-gated-apply archetype. Check (`rocm update`) and Preview (`--apply --dry-run`) are read-only (no gate); Apply (`--apply`) and Apply-and-activate (`--apply --activate`) route through the approval gate → job-bridge. 8 tests incl. snapshot. - app.rs: doctor_manager/update_manager overlays + event_loop arms + OpenDoctor/OpenUpdate (`d`/`u` from Overview/Instances). Refactored the per-open sibling-clear into AppState::close_overlays() (DRY across 5 overlays; mutual-exclusion invariant). ui/mod.rs draws them + footer hints. +tests for d/u tab-scoping and the extended mutual-exclusion set. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 280 lib + 5 wave0 green. (apps/rocm pre-existing chat/therock failures unchanged; diff is dash-tui + app.rs only.) * fix(merge): doctor/update review fixes + console-key seam consolidation (EAI-6871) 3x refute panel on the Wave-2 doctor/update screens: 2 fail-to-refute (CLI argv exact, invariants/regression/clippy clean), 1 REFUTED with a HIGH DRY finding + test gaps (no behavioral bugs). Applied: - HIGH (DRY): extracted the per-screen active_job console-key block (Ctrl+C cancel / q close / Esc-Enter dismiss-when-terminal) into job_console::on_console_key -> ConsoleOutcome, and refactored ALL five operational screens (services/serve/engine/doctor/update) onto it. ~80 lines of near-duplicate logic collapsed to one tested seam (+helper test). - MEDIUM (systemic key-trap): approval_key now treats q/Q as Cancel (not just Esc), so q is never silently swallowed while any approval modal is up. Help text -> 'Esc/q cancel'; wave0 approval snapshot assertion updated. +test. - Tests added: update relaunch-while-running no-op guard (highest-value gap), doctor r-rerun-after-terminal + r-at-idle. - Polish: doctor no-op now documents the intentional re-attach (vs distinct-id screens); doctor footer 'Enter/r run'; update tag '(gated)'->'(needs approval)'. rocm-dash-tui 284 lib + 5 wave0 green; fmt + clippy -D warnings clean; workspace --all-targets builds. Diff is dash-tui only. * feat(merge): Phase 3 — install + logs screens on Wave-0 primitives (EAI-6871) Two more operational screens on the unified reducer base + shared console seam: - ui/install_manager.rs (Wave 2): `rocm install sdk` form (channel · format · folder · mode). Defaults to dry-run (read-only → spawns straight through the job-bridge); a real install is mutating → approval gate first. Folder picked with the Wave-0 FolderBrowser (Tab on the Folder field). Distinct dry-run vs install job ids. 10 tests incl. snapshot. - ui/logs_view.rs (Wave 3): `rocm logs [--search WORDS]` — read-only browser with an optional search box; no approval. 6 tests incl. snapshot. - Both consume job_console::on_console_key (the consolidated seam). - app.rs: install_manager/logs_view overlays + event_loop arms + OpenInstall (`i`)/OpenLogs (`l`) from Overview/Instances; close_overlays() extended; ui/mod.rs draws them + footer hints. Tests: i is ChatFocus on Chat (guarded from OpenInstall), l/d/u/w/e Nothing on Chat, extended mutual-exclusion set. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 300 lib + 5 wave0 green. Diff is dash-tui + app.rs only. Wave coverage now: serve_wizard · model_picker · engine_manager · services_manager (W1) + doctor · update · install (W2) + logs (W3). Remaining for the unified TUI: runtime/onboarding (W2), automations/command/config/provider (W3), the D3 chat split (rig-core chatgpt OAuth), then tui.rs retirement. Phase 4 deferred behind upstream PR #4 (it also touches ci.yml + the single-exe/universal-binary scripts). * fix(merge): install/logs adversarial-review fixes (EAI-6871) 3x refute panel: 2 fail-to-refute (CLI argv exact incl. InstallFormat lowercase + logs --search num_args=1..; no shell injection; invariants/struct-evolution/ mutual-exclusion across all 7 overlays/regression/footer-panic clean), 1 REFUTED. The 2 HIGH (q-at-idle) are the form-archetype trait already accepted for serve_wizard: text fields (Channel/Prefix, logs search box) capture q as input, Esc closes — not a trap, consistent with the form screens (not the menu screens doctor/update). Documented as such; applied the valuable MEDIUM fixes: - install: added the no-op-guard test (relaunch-while-running surfaces a message, no stale console) mirroring update/engine — the highest-value gap. - logs: added the second-Enter re-attach test (read-only re-attach like doctor). - install: '←→ change' hint corrected to '←→ cycle (Format/Mode)' (it only acts on the cycle fields). - Documented the deliberate Esc-closes / text-field-q-capture form behavior in both screens. rocm-dash-tui 302 lib + 5 wave0 green; fmt + clippy -D warnings clean. * feat(merge): Phase 3 Wave 2 — runtime_manager screen + RuntimeSummary plumbing (EAI-6871) list/activate/rollback/uninstall/adopt/import ROCm runtimes on the Wave-0 primitives. Runtime list plumbed bin→ResolvedArgs→AppState like model_recipes (dash.rs adapts therock::load_runtime_manifests + config active/rollback status; no rocm-core dep in the TUI crate). Refresh read-only; mutating verbs gated by the approval seam; adopt via FolderBrowser, import via typed-path prompt. Zero thread::spawn/try_recv. 15 tests incl TestBackend snapshots + no-op guard. rocm-dash-tui 317 lib + 5 wave0 green; core 53; clippy -D warnings clean. * test(merge): cover OpenRuntimes in overlay mutual-exclusion test (EAI-6871) Adversarial panel LOW finding: opening_an_overlay_closes_the_others stopped at OpenLogs. Extend it to assert OpenRuntimes clears the prior overlay too, so a future close_overlays regression on that arm is caught. * feat(merge): Phase 3 Wave 2 — onboarding wizard (minimal install+adopt) (EAI-6871) First-run setup rebuilt on the Wave-0 primitives: Welcome → Choose {Install ROCm SDK (release·pip) | Adopt existing folder} → approval → job-bridge → Done. Install is a one-shot gated 'install sdk'; adopt picks an env root via FolderBrowser then approves 'runtimes adopt' (cfg!-derived python). Both gated; zero thread::spawn/try_recv. Reinstall/uninstall/show-log sub-modals are documented fast-follows. 'n' opens it from Overview/Instances. 10 tests incl TestBackend snapshots (welcome+choose) + no-op guard. 327 lib + 5 wave0 green; clippy -D warnings clean. * fix(merge): onboarding adversarial-review fixes (EAI-6871) Panel HIGH: the wizard advanced to 'Done — you're ready to go' on ANY terminal console dismiss, so a FAILED 'rocm install sdk' falsely claimed success. Now only a clean exit (code 0) reaches Done; failure/cancel returns to Choose with an honest, retry-able message. +test for the code:1 path. Panel MEDIUM: 'Esc skip' welcome hint actually closed → 'Esc close'. Panel MEDIUM: documented the post-install engine-preference reconciliation + first-run persistence as explicit fast-follows in the module doc. * feat(merge): Phase 3 Wave 3 — automations + command + config/provider screens (EAI-6871) Three operational overlays on the Wave-0 primitives, completing the screen waves: - automations_manager: list/enable/disable background checks. AutomationSummary plumbed bin→ResolvedArgs→AppState (dash.rs adapts builtin_watchers + config enabled/mode). Refresh read-only; toggles gated. 'a' opens. - command_screen: general 'rocm <args>' runner; EVERY freeform command routes through approval (safe default for unclassifiable subcommands) → job-bridge. 'c' opens. - config_manager: folds config + provider. Show read-only; provider enable/disable gated. API keys are NEVER entered/stored here — env-only per the chat invariant, with a persistent notice + a structural test asserting no set-provider-key argv is ever produced. 'p' opens. All reducer-only (zero thread::spawn/try_recv); TestBackend snapshots + no-op guards; close_overlays mutual-exclusion extended (+test). rocm-dash-tui 355 lib + 5 wave0 green; clippy -D warnings clean; workspace build --all-targets exit 0. * fix(merge): Wave 3 adversarial-review fixes (EAI-6871) Panel MEDIUM (env-only hardening): command_screen is the one place the TUI takes freeform text, so a user could type 'config set-provider-key …' and leak a secret through argv/job logs. Add looks_secret_bearing() → the approval body now warns when an argv looks secret-bearing (key/token/secret/password), steering keys back to the environment. config_manager still structurally refuses key entry. Panel MEDIUM (coverage): add command_screen q-escapes-while-job test + automations toggle relaunch-while-running no-op-guard test. Panel LOW: automations snapshot now asserts the on/off badges render. rocm-dash-tui 358 lib + 5 wave0 green; clippy -D warnings clean. * feat(merge): Phase 3 D3 — no-key ChatGPT OAuth chat backend (EAI-6871) Wire rig-core 0.38.1 native chatgpt provider as a new AgentClient (ChatGptAgentClient) behind the existing swappable seam in agent.rs — the no-key default that restores the ChatGPT device-login the vendored Codex path provided. - Builds via Client::builder().oauth().on_device_code(handler); authorize() runs the device-code flow on first complete() and the provider caches the token. - Takes NO api_key: the env-only key invariant is structurally preserved (this path authenticates by OAuth, not a key) — asserted by a construction test. - Same six READ-ONLY telemetry/skills tools as RigAgentClient; mutating CLI tier stays behind the approval seam (command_screen) — full chat→approval wiring is a documented fast-follow. - app.rs selects it as the no-key default ONLY when no local endpoint is reachable AND no api_key/url is configured; the device-code URL+code surface in the chat tab via the existing ClientMsg::ChatReply plumbing. - Live interactive round-trip test is #[ignore] (network + browser login). agent.rs stays the only file naming rig types. 360 lib + 5 wave0 green; clippy -D warnings clean; workspace build --all-targets exit 0. * fix(merge): D3 chat-split review fixes (EAI-6871) Panel LOW: add AgentError::Auth so a failed/declined ChatGPT sign-in surfaces as 'chat sign-in failed: …' (not the misleading 'failed to build chat client'); the token is never embedded. Tighten chatgpt_oauth_client_defaults_model_when_none to assert the exact GPT_5_3_CODEX default rather than mere non-emptiness. * fix(merge): pip->wheel sweep + rocm-core test clippy (EAI-6871 Phase 3) After rebasing onto #1-4, #3's InstallFormat rename (pip->wheel; valid = {wheel, tarball}) makes our hardcoded 'pip' invalid. Sweep: - rocm-dash-tui install_manager.rs: FORMATS = ["wheel","tarball"]; default + build_args + tests emit --format wheel. - rocm-dash-tui onboarding.rs: install argv + doc + test --format pip -> wheel. - apps/rocm main.rs: chat system prompt argv and the install-validation error messages (Windows guard + version/build-date guard) referenced the now-invalid 'pip' though the logic already requires wheel -> aligned to wheel. - rocm-core lib.rs: regenerated Cargo.lock + fixed a field_reassign_with_default clippy lint in a dashboard-daemon test (struct-literal init). Workspace build/test/clippy -D warnings + fmt all green.
Member
michaelroy-amd
added a commit
that referenced
this pull request
Jun 15, 2026
…ek-hooks landing The #1-4 base had CI failures from PR #3's incomplete pip->wheel rename and pre-existing lints, exposed by #4's workspace clippy/fmt/windows jobs: - clippy (-D warnings): removed useless format!, collapsed if-lets/match guards in therock.rs + tui.rs; removed two now-orphaned dead test helpers (command_succeeds, write_fake_python_without_venv_support). - fmt: cargo fmt --all (comfyui/therock/tui/uv/lemonade/pytorch/rocm-core). - windows-build-and-test: fixed stale assertions in the cfg(windows) test install_sdk_rejects_tarball_on_windows_before_resolution and the windows branch of scripts/smoke_local.py ("Windows V1"->"Windows", --format pip->wheel) to match #3's renamed error message. - correctness: rocm-core legacy-ROCm hint suggested the now-invalid '--format pip'; updated to '--format wheel' (+ its test).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 15, 2026
…s (EAI-6871) Second operational screen rebuilt on the unified rocm-dash reducer base. A compact serve form (model · engine · device · host · port · mode) that builds a `rocm serve … --managed` invocation and runs it through the approval gate and the job-bridge — never inline, zero std::thread::spawn/try_recv. - ui/serve_wizard.rs: form state machine + pure build_args() + on_key seam mirroring services_manager; Tab on Model opens the Wave-0 FolderBrowser for a local model path; Launch → approval → job-bridge StartJob. Managed (default) surfaces the model in services_manager + dashboard gen_tps (D7 wire live). 16 unit tests incl. 2 TestBackend snapshots. - ui/exec.rs: shared resolve_exe()/exe_label() spawn helpers; services_manager refactored onto them (DRY, removes its private copies). - app.rs: AppState.serve_wizard overlay + event_loop interception arm + OpenServeWizard (`w` from Overview/Instances); ui/mod.rs draws it + footer hint. Gates: cargo build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; rocm-dash-tui 238 lib + 5 wave0 green. Workspace test 750 passed / 4 pre-existing apps/rocm failures (untouched tui.rs chat + therock.rs python-launcher; PR #4-owned, not regressions — diff is dash-tui only).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 15, 2026
…AI-6871) Two more operational screens on the unified reducer base + shared console seam: - ui/install_manager.rs (Wave 2): `rocm install sdk` form (channel · format · folder · mode). Defaults to dry-run (read-only → spawns straight through the job-bridge); a real install is mutating → approval gate first. Folder picked with the Wave-0 FolderBrowser (Tab on the Folder field). Distinct dry-run vs install job ids. 10 tests incl. snapshot. - ui/logs_view.rs (Wave 3): `rocm logs [--search WORDS]` — read-only browser with an optional search box; no approval. 6 tests incl. snapshot. - Both consume job_console::on_console_key (the consolidated seam). - app.rs: install_manager/logs_view overlays + event_loop arms + OpenInstall (`i`)/OpenLogs (`l`) from Overview/Instances; close_overlays() extended; ui/mod.rs draws them + footer hints. Tests: i is ChatFocus on Chat (guarded from OpenInstall), l/d/u/w/e Nothing on Chat, extended mutual-exclusion set. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 300 lib + 5 wave0 green. Diff is dash-tui + app.rs only. Wave coverage now: serve_wizard · model_picker · engine_manager · services_manager (W1) + doctor · update · install (W2) + logs (W3). Remaining for the unified TUI: runtime/onboarding (W2), automations/command/config/provider (W3), the D3 chat split (rig-core chatgpt OAuth), then tui.rs retirement. Phase 4 deferred behind upstream PR #4 (it also touches ci.yml + the single-exe/universal-binary scripts).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 15, 2026
…s (EAI-6871) Second operational screen rebuilt on the unified rocm-dash reducer base. A compact serve form (model · engine · device · host · port · mode) that builds a `rocm serve … --managed` invocation and runs it through the approval gate and the job-bridge — never inline, zero std::thread::spawn/try_recv. - ui/serve_wizard.rs: form state machine + pure build_args() + on_key seam mirroring services_manager; Tab on Model opens the Wave-0 FolderBrowser for a local model path; Launch → approval → job-bridge StartJob. Managed (default) surfaces the model in services_manager + dashboard gen_tps (D7 wire live). 16 unit tests incl. 2 TestBackend snapshots. - ui/exec.rs: shared resolve_exe()/exe_label() spawn helpers; services_manager refactored onto them (DRY, removes its private copies). - app.rs: AppState.serve_wizard overlay + event_loop interception arm + OpenServeWizard (`w` from Overview/Instances); ui/mod.rs draws it + footer hint. Gates: cargo build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; rocm-dash-tui 238 lib + 5 wave0 green. Workspace test 750 passed / 4 pre-existing apps/rocm failures (untouched tui.rs chat + therock.rs python-launcher; PR #4-owned, not regressions — diff is dash-tui only).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 15, 2026
…AI-6871) Two more operational screens on the unified reducer base + shared console seam: - ui/install_manager.rs (Wave 2): `rocm install sdk` form (channel · format · folder · mode). Defaults to dry-run (read-only → spawns straight through the job-bridge); a real install is mutating → approval gate first. Folder picked with the Wave-0 FolderBrowser (Tab on the Folder field). Distinct dry-run vs install job ids. 10 tests incl. snapshot. - ui/logs_view.rs (Wave 3): `rocm logs [--search WORDS]` — read-only browser with an optional search box; no approval. 6 tests incl. snapshot. - Both consume job_console::on_console_key (the consolidated seam). - app.rs: install_manager/logs_view overlays + event_loop arms + OpenInstall (`i`)/OpenLogs (`l`) from Overview/Instances; close_overlays() extended; ui/mod.rs draws them + footer hints. Tests: i is ChatFocus on Chat (guarded from OpenInstall), l/d/u/w/e Nothing on Chat, extended mutual-exclusion set. Gates: build --workspace --all-targets exit 0; clippy -p rocm-dash-tui --all-targets -D warnings clean; fmt clean; rocm-dash-tui 300 lib + 5 wave0 green. Diff is dash-tui + app.rs only. Wave coverage now: serve_wizard · model_picker · engine_manager · services_manager (W1) + doctor · update · install (W2) + logs (W3). Remaining for the unified TUI: runtime/onboarding (W2), automations/command/config/provider (W3), the D3 chat split (rig-core chatgpt OAuth), then tui.rs retirement. Phase 4 deferred behind upstream PR #4 (it also touches ci.yml + the single-exe/universal-binary scripts).
michaelroy-amd
added a commit
that referenced
this pull request
Jun 29, 2026
… scroll, doc fixes - #3: guard the Esc menu on a pure menu_fits() helper so it only renders when the logo AND all three items (Options/Help/Quit) fit; the old inner.height < 6 guard painted the logo with no reachable items at heights 6..=10. New unit test exercises the broken range + boundary. - #4: remove the dead 't theme' launcher footer hint (no t handler). - #5: global Help jump-to-tab now reads '1 .. 5' (5 tabs), matching draw_help. - #6: scroll-offset casts use u16::try_from(..).unwrap_or(u16::MAX) instead of 'as u16' (console v/h, dock logs, bench detail, chat) — no wrap >65535. - #7: split misattributed doc comments so apply_action and clamp_selectors each carry their own doc (and drop a stale gpu_scroll line). Signed-off-by: Michael Roy <michael.roy@amd.com>
6 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add prek (a fast, drop-in
pre-commitreplacement) so contributors run the same checks locally that CI enforces, catching fmt/clippy/lint failures before they reach a PR..pre-commit-config.yamlwires up:repo: builtinhooks (trailing-whitespace, end-of-file-fixer, check-yaml, check-merge-conflict, mixed-line-ending) — zero-setup, no external clone.ruff-check --fix+ruff-format(scripts/, engine workers).shellcheck(install.sh,scripts/*.sh).cargo fmton commit;cargo clippy -D warningsandcargo teston pre-push to keep commits fast.scripts/check_powershell_syntax.py— cross-platform, runs wherepwshexists and skips cleanly otherwise (CI covers Windows).Vendored
third_party/is excluded. Setup/usage documented in the README.This PR also includes the mechanical cleanup needed to make the hooks pass on a clean checkout (repo-wide ruff format + import sort, three shellcheck fixes) and fixes pre-existing fmt/clippy failures the new hooks surface — notably gating
rocm-core's Windows-onlyOsStrimport and the Windows GPU/doctor inventory helpers behindcfgso the workspace is clippy-clean on all targets.CI test fixes
Wiring
cargo testinto the workflow surfaced pre-existing test failures (red onmaintoo) that only reproduce in CI's environment. Fixed here so the suite is green:size()before the first paint; with no TTY (CI) this fell back to the follow sentinel / bailed out, leaving the view at the top. Added aneffective_terminal_sizefallback used by both scroll paths — also fixes auto-follow when the CLI runs without a controlling terminal.python_launcher_prefers_path_python…appended the realPATH, so a genuine cp312 Python on the runner won over the fake one (and poisoned a shared lock, cascading into a sibling test). KeptPATHhermetic.Connection: close, so ureq's pooled agent reused a dead socket and blocked until the HTTP timeout on Windows. The server now sendsConnection: close.Setup
Risk
Low — tooling, mechanical formatting, and test/headless-robustness fixes; no runtime behavior change beyond auto-follow working without a TTY.
Test plan
prek run --all-files— all hooks green (hygiene, ruff, shellcheck, cargo fmt, powershell)cargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all --check— cleancargo test --workspace— green (752 passing locally)--self-testsuite passes;py_compileclean