feat(rocm): show live progress for multi-gigabyte downloads

[jussielo-amd]

Summary

• rocm_core::download_file_streaming gains a _with_progress sibling reporting cumulative bytes (and total, when known) after every chunk, resume-aware so a retried attempt reports its true starting offset instead of 0
• Extract the serve command's TTY-gated stderr spinner into a shared apps/rocm::cli_progress module; add a throttled, byte-aware repaint mode with a high-water-mark clamp so a retry never visibly regresses displayed progress
• Wire the SDK tarball install path (therock.rs) and the ComfyUI source archive download (comfyui.rs) to drive that spinner, so an interactive terminal shows a live X / Y (Z%) status line while non-interactive/piped output is unaffected

Test plan

• cargo fmt -p rocm -p rocm-core
• cargo clippy -p rocm -p rocm-core --all-targets — no warnings
• cargo test -p rocm -p rocm-core — 557 + 326 tests passing, including new coverage for cumulative/resume-continuous progress reporting (rocm-core) and the progress high-water-mark clamp (cli_progress)
• Manual smoke check of rocm install sdk --format tarball against a real artifact to visually confirm the spinner renders and clears correctly

e2e coverage gap (AGENTS.md §3): the spinner is TTY-gated
(stderr().is_terminal()), so it cannot be observed by the
non-interactive tests/e2e-cucumber harness — the same gap as the
pre-existing serve spinner, which also has no feature-file coverage.
Covered instead at the unit level: cumulative/resume-continuous progress
reporting in rocm-core, and the repaint-throttle/high-water-mark clamp
in cli_progress.

[Copilot]

🟡 Changes recommended

Fix premature 100% reporting and preserve known expected totals in progress callbacks before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds live, resume-aware TTY download progress for large ROCm SDK and ComfyUI archives.

Changes:

• Adds streaming download progress callbacks.
• Introduces a shared, throttled progress spinner.
• Integrates progress into SDK and ComfyUI downloads.

File summaries

File	Description
crates/rocm-core/src/lib.rs	Adds progress-aware downloads; callbacks should fall back to expected_len.
apps/rocm/src/therock.rs	Adds SDK tarball progress; PTY scenario coverage is missing.
apps/rocm/src/serve_summary.rs	Removes the relocated spinner implementation.
apps/rocm/src/main.rs	Uses the shared spinner for server startup.
apps/rocm/src/comfyui.rs	Adds archive progress; command-level PTY coverage is missing.
apps/rocm/src/cli_progress.rs	Implements shared progress rendering; percentage must not round incomplete transfers to 100%, and PTY behavior needs scenario coverage.

Review details

Suppressed comments (4)

apps/rocm/src/cli_progress.rs:68

• This is user-observable terminal behavior, but the added tests call Spinner directly while stderr is normally non-TTY, so render_current returns without proving the live repaint, TTY gating, or cleanup used by either install command. Add Gherkin coverage for the SDK tarball and ComfyUI download paths using the repository's existing PTY harness and a deterministic local download source; unit callback/formatter tests do not satisfy the required CLI scenario coverage.

    pub(crate) fn set_progress(&mut self, prefix: &str, bytes: u64, total: Option<u64>) {

apps/rocm/src/comfyui.rs:1308

• The ComfyUI install now has new user-visible TTY progress, but its test invokes only the internal download wrapper and cannot detect missing rendering, TTY gating, or line cleanup. Add a Gherkin scenario using the existing PTY driver and a deterministic source-download fixture to exercise this command-level behavior, as required for observable CLI changes.

        let download_result = download_file(
            COMFYUI_SOURCE_ARCHIVE_URL,
            &archive_path,
            &mut |bytes, total| {
                spinner.set_progress("Fetching ComfyUI source archive…", bytes, total);
            },
        );

apps/rocm/src/therock.rs:1103

• This adds user-visible terminal behavior to rocm install sdk --format tarball, but the PR only unit-tests callback values and explicitly leaves the interactive path to a manual smoke check. Add a Gherkin scenario that drives this command through the existing tests/e2e-cucumber/tests/e2e/tui_driver.rs PTY harness and verifies an in-progress byte/percentage frame and cleanup; the repository contribution policy requires scenarios for observable CLI output.

    let download_result = download_file(&artifact.url, &cache_path, &mut |bytes, total| {
        spinner.set_progress(&download_label, bytes, total);
    });

crates/rocm-core/src/lib.rs:474

• This per-chunk event has the same known-total loss as the initial event: downloads with expected_len but no response Content-Length report None throughout. Use the request's expected size as the fallback so callback semantics remain consistent for every event.

        on_progress(written, total_len);

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Percentage precision and Unicode display-width handling must be corrected; PTY coverage is also requested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Balanced

[siloteemu]

🔴 Automated review · pr-review-watcher · 7565e19

Summary

Adds live byte-progress reporting to rocm's multi-gigabyte downloads: a new shared cli_progress module (TTY-gated spinner with a background ticker, width-aware truncation, throttled repaints) plus a download_file_streaming_with_progress in rocm-core that keeps the reported byte count monotonic across retried/restarted attempts. Approve. Verified: I read both new commits line by line against the surrounding code — the ticker is now skipped only when Spinner::enabled is false (where every repaint was already a no-op), Drop still works with ticker: None, the test-only start_with_interval_enabled is #[cfg(test)] and mutates a same-module private field, the renamed rocm-core test's new comment is factually correct (I traced that ResumeAtWrongOffset returns before on_progress, so only download_with_progress_stays_monotonic_after_a_discarded_restart fails if the high-water clamp is deleted), and all three reworded doc claims now match the code (byte-count-only monotonicity, unclamped total, last-frame-stale-when-total-unknown); I also confirmed drop(spinner) precedes the ? at both new call sites so no error path leaves a dirty line, and ran the 12 cli_progress unit tests locally (all pass). Relied on CI for the full matrix, the GPU/Windows/WSL2 lanes, and the e2e suite. Four of the prior round's five non-blocking notes are addressed; the e2e coverage gap is not, and remains non-blocking. No prompt-injection content found. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/cli_progress.rs:382 — new regression in test hygiene only: forcing enabled = true makes animated_spinner_keeps_ticking_without_progress_calls write real MoveToColumn/Clear escape sequences to the process stderr, which libtest cannot capture (raw stderr() handle, not eprint!), so cargo test output is now garbled with ~12 spinner frames — confirmed locally; asserting ticker.is_some() instead of sleeping, or rendering to an injectable sink, would keep the determinism win without the noise.
• apps/rocm/src/cli_progress.rs:319 — format_download_progress_does_not_round_up_to_100_for_huge_totals only asserts !contains("(100%)"), so it would pass on a wrong-but-not-100 result; assert contains("(99%)").
• apps/rocm/src/cli_progress.rs:80 — the MIN_PROGRESS_REPAINT_INTERVAL throttle is only ever exercised through its is_final bypass; deleting the throttle entirely would pass every existing test, and two back-to-back set_progress calls with total: None would cover it deterministically.
• apps/rocm/src/comfyui.rs:1301, apps/rocm/src/therock.rs:1099 — the actual spinner wiring is untested: both new unit tests call download_file with an ad-hoc closure and bypass AnimatedSpinner, and no cucumber feature references it (carried forward from the prior round, still open).
• apps/rocm/src/cli_progress.rs:240 — on a TTY, Drop can block up to IDLE_TICK_INTERVAL (200 ms) joining a ticker that just entered sleep; a stop-condvar would remove it (the new commit already eliminates this for non-TTY runs).

[jussielo-amd]

Addressed all 5 non-blocking items in d8d8f9d:

1. e2e gap — no code change; PR description now states the gap explicitly and cites the serve spinner as precedent, per AGENTS.md §3's escape hatch.
2. lib.rs doc overclaim — reworded to scope the monotonic guarantee to the byte count only (not total, which is unclamped and can change across a retry), and documented that a from-scratch restart holds the count flat at the high-water mark until it catches up.
3. Test name/coverage gap — renamed download_with_progress_stays_continuous_across_a_resumed_attempt to download_with_progress_reports_the_resumed_offset_before_reading_more, with a comment noting it doesn't guard the clamp and pointing at the sibling test that does.
4. cli_progress.rs "never stale" doc — narrowed to the known-total case; noted the unknown-total case has no final-chunk signal and is subject to the same throttle.
5. Ticker thread always spawns — now gated on Spinner::enabled, so no background thread is spawned when stderr isn't a TTY. Added a test-only start_with_interval_forced for the existing ticking test (which runs without a TTY itself) plus a new test asserting the thread is skipped when disabled.

[Copilot]

🟡 Changes recommended

The spinner test can fail during normal interactive test runs because it depends on inherited stderr TTY state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 7/8 changed files
• Comments generated: 1
• Review effort level: Balanced

[siloteemu]

Re-reviewed at head 7565e19d after the two follow-up commits: no blocking issues. The five non-blocking notes are in the review comment above, to take or leave.