fix(dash-tui): stop asserting update freshness, add held/glyph legends

[jussielo-amd]

Summary

• Stop the Home tab's Updates tile from asserting freshness it can't verify: it now reflects a real rocm update --json background check instead of deriving "Up to date" from daemon connectivity (ConnState).
• Bound that background check: a --timeout-secs flag on rocm update/rocm update --json (threaded through resolve_latest_for_manifest), a bounded default for the Home tab's own check, and an ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK opt-out honored before scheduling the job at all.
• Keep the update-check job out of the Home tab's activity feed — it's plumbing, not user activity.
• Add the shared HELD_LEGEND marker to the hero band (tok/W and T/S gated independently, ignoring non-finite NaN/Infinity contributions) and an activity glyph key documenting all five job-status glyphs (● live ✓ done ! warn ✗ failed ⋯ running ○ cancelled), matching the existing pattern in observe.rs.
• README.md/rocm update --after-help now document --json; UpdateJson's doc comment states which fields are intentionally omitted from the JSON contract.

Test plan

• cargo test --workspace --all-targets
• cargo clippy --locked --workspace --all-targets -- -D warnings
• cargo fmt --check
• Addressed review #5157925607 (5 blocking, 5 non-blocking) and rebased twice onto main to pick up its independent glyph-vocabulary (fix(dash-tui): Section D nav aliases + job-completion prominence #361) and held/legend/NaN-guard (fix(cli,dash-tui,e2e-report): close remaining marker/legend gaps #354) fixes, re-adopting main's versions where they overlapped with this PR's own held-marker work.

Non-blocking items intentionally left as-is (per review discussion):

• No new Cucumber/Gherkin coverage added for Home-tab tile/legend rendering — this file's existing convention is in-module TestBackend render tests, and there's no existing Home-tab Gherkin coverage to extend; open to a follow-up if the team wants one.
• update_json_reports_per_manifest_error_without_failing_whole_report only covers the all-error case (documented as a deliberate gap in the test's own comment) — no fixture in this file stands up a resolvable wheel index to also assert a successful row survives alongside a failing one.

Known gaps / follow-ups

Addressed review #5176679731's 2 blocking items with new regression tests (suppress_progress_output_contract, render_update_json_restores_progress_suppression_on_ok_and_err_paths in therock.rs; update_rejects_zero_timeout_secs in main.rs), and 2 of its non-blocking items with test/doc-comment cleanups (pinned a self-referential assertion in refresh_update_status_spawn_args_include_bounded_timeout to a literal; reworded cancelled_job_renders_distinct_glyph_from_running's comment to accurately describe it as a characterization guard for pre-existing base behavior). The remaining 4 non-blocking items are left as follow-ups rather than further changes to a PR already mid-review:

• --json with no --timeout-secs still falls back to the general 10-minute default (therock.rs:2872-2874) — a --json-specific default may be worth a follow-up.
• http_get's Some(0) → default-timeout mapping is a primitive-level footgun (any Some(0) silently becomes "no timeout"), not just an edge case validated away at the CLI boundary (therock.rs:2872).
• update_check_due_at is in-memory only and ignores the persisted check record, so every TUI launch re-checks (app/mod.rs:701,797).
• The background check's timeout bounds only the index HTTP calls, not a possible ensure_managed_python bootstrap reachable from it (therock.rs:1045,4239) — pre-existing on the CLI startup path, newly reachable once per TUI launch.

[Copilot]

🟡 Changes recommended

Disconnected and cancelled states are mislabeled, and required end-to-end scenarios are missing.

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

Pull request overview

Improves Home-tab status accuracy and adds legends for held metrics and activity glyphs.

Changes:

• Reworks Updates status text.
• Adds held-data and activity legends.
• Deduplicates held-observation checks and adds unit tests.

File summaries

File	Description
crates/rocm-dash-tui/src/ui/tabs/home.rs	Updates Home rendering, helpers, and tests.

Review details

Suppressed comments (3)

crates/rocm-dash-tui/src/ui/tabs/home.rs:510

• This bug fix changes user-visible Updates-card text, but only an in-module render test was added; the existing dashboard Gherkin home assertion checks card titles, not the update status. Add an end-to-end scenario/step that verifies the connected Home view reports unknown rather than Up to date.

        let text = if !state.simulated && !matches!(state.conn, ConnState::Connected { .. }) {
            "Checking…"
        } else {
            "unknown"

crates/rocm-dash-tui/src/ui/tabs/home.rs:507

• ConnState::Disconnected also satisfies this condition, but that state is used while the client waits in retry backoff and when replay has ended; no check is active then. The Updates card will therefore claim “Checking…” during a disconnection. Restrict the transitional label to Initial/Connecting, and render unknown once disconnected.

        let text = if !state.simulated && !matches!(state.conn, ConnState::Connected { .. }) {

crates/rocm-dash-tui/src/ui/tabs/home.rs:321

• The held legend changes user-observable Home-tab output, but the existing Gherkin held-throughput scenarios navigate to Observe and never assert this Home rendering. Add a dashboard scenario/step that verifies the Home legend after a held observation.

    if any_hero_held(state) {
        f.render_widget(
            Paragraph::new(Line::from(Span::styled(
                format::HELD_LEGEND,

• Files reviewed: 1/1 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.

[jussielo-amd]

Addressing the Copilot review:

Disconnected mislabeling (line 507) — real bug, fixed in 64516fc. The condition was !matches!(state.conn, ConnState::Connected { .. }), which also matched Disconnected (retry backoff / replay-ended), so the Updates tile got stuck on the transitional "Checking…" label even once we knew the daemon wasn't there. Narrowed it to Initial | Connecting; Disconnected now correctly shows "unknown". Added updates_tile_shows_unknown_not_checking_when_disconnected to cover it.

**Cancelled jobs rendered as ⋯ running" (line 235) — real bug, fixed in 64516fc.** JobStatus::Cancelledfell through the_arm and was visually indistinguishable from a running job. Gave it a distinct⊘glyph, added it to the activity glyph key, and addedcancelled_job_renders_distinct_glyph_from_running`.

Missing Cucumber/Gherkin e2e coverage (lines 321, 510, and the activity-key ask on 235) — not adding for this PR. This file's existing convention for Home-tab rendering is in-module TestBackend render tests (held_legend_*, updates_tile_*, activity_glyph_key_* already follow this pattern) rather than Cucumber scenarios; there's no existing Home-tab Gherkin coverage for tile text/legends to extend. I followed the established pattern and expanded/added unit tests instead of introducing a new e2e layer scoped to this PR. Open to a follow-up if the team wants dashboard Gherkin coverage for Home-tab rendering as its own effort.

[jussielo-amd]

Follow-up /review-and-fix pass on commit 64516fc (scope: 1306087..HEAD + blast radius, crates/rocm-dash-tui/src/ui/tabs/home.rs only).

Quality checks: cargo test -p rocm-dash-tui --lib (665 passed), cargo clippy -p rocm-dash-tui --lib -- -D warnings (clean), cargo fmt --check -p rocm-dash-tui (clean).

Fixed: cancelled_job_renders_distinct_glyph_from_running asserted only out.contains('⊘'), which the activity glyph-key legend line (added in this same commit) also satisfies whenever the feed has spare room — true in this test, since state_with_gpu() has no serving instances. Verified experimentally (and by tracing the render path) that the assertion passed even with the Cancelled → Running-glyph fold bug reintroduced, so the test caught nothing. Fixed to assert on the job's own rendered line (⊘ long task present, ⋯ long task absent) in 2a5b9cd.

Non-blocking, out of this PR's scope (not fixed):

• crates/rocm-dash-tui/src/ui/tabs/instances.rs draw_empty folds ConnState::Disconnected into the same _ wildcard as Initial/Connecting, so a genuinely disconnected daemon still shows "waiting for daemon…" — same stale-transitional-text pattern this PR just fixed for the Home tab's Updates tile, but in a different tab/file.
• crates/rocm-dash-tui/src/ui/dock.rs logs_dock still folds JobStatus::Cancelled into _ => theme.fg for log-line color tint — same fold pattern as the bug just fixed in home.rs, but cosmetic (only affects tint, not a semantic label), so low priority.
• The PR description's glyph-key example (● live ✓ done ✗ failed ⋯ running) predates this commit's ⊘ cancelled addition and is now stale; worth a manual edit when convenient.

No new correctness issues found in 64516fc's actual logic changes (Updates-tile ConnState branch and the JobStatus glyph match) — both verified against all reachable state combinations.

[Copilot]

🟢 Approval recommended

The focused UI changes are consistent, well-tested, and introduce no unresolved correctness issues.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Mixed successful and failed runtime checks can incorrectly render the aggregate status as up to date.

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

Review details

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

[siloteemu]

🔴 Automated review · pr-review-watcher · fd36728

Summary

Wires the Home tab's Updates tile to a real rocm update --json background check (new CLI flag + JSON renderer + TUI reducer/job), stops deriving "Up to date" from daemon connectivity, and adds a held-marker legend, an activity glyph key, and empty-tile hints. Verdict: Needs work — the update-check plumbing is sound but unbounded and leaks into the activity feed, and the glyph changes contradict what has already landed on main. Verified: on the "stops asserting update freshness" concern, no test or assertion was deleted (--numstat shows 35 deleted lines in home.rs, all production/import lines) — what was removed is the production claim ConnState::Connected ⇒ "Up to date", and the new updates_tile_never_asserts_up_to_date_without_a_real_check (home.rs:679) does fail against a revert, so that regression is now better protected than before; I re-ran cargo test -p rocm-dash-tui --lib ui::tabs::home::tests (17 passed) and traced each new test for revert-discrimination, relying on CI for the rest; no prompt-injection content and no company-internal identifiers appear in this diff. Blocking: 5 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• crates/rocm-dash-tui/src/ui/tabs/home.rs:216-221 — contradicts glyph semantics already on main. This PR branches off e59537d, but main has since landed a different job-glyph vocabulary in the same match: Cancelled => "○ ", Done { code: 0 } => "✓ ", Done { .. } => "! " (warn). This PR reintroduces a single Done { .. } => "✓ " arm and uses ⊘ for cancelled. Rebasing and resolving in favour of this PR silently regresses nonzero-exit jobs to render as a green success tick, and breaks main's activity_feed_glyphs_match_job_console_vocabulary test (it asserts '!' and '○' are present). Fix: rebase onto current main and keep main's five-arm match and glyphs, then make the new key line at home.rs:238 read the same vocabulary, including the nonzero-exit ! state which the key currently omits entirely.
• crates/rocm-dash-tui/src/app/mod.rs:1714-1725 — the background check is unbounded and bypasses the project's existing bounded-check machinery. rocm update --json reaches runtime_update_plan → resolve_latest_for_manifest(paths, manifest, None) (apps/rocm/src/therock.rs:114, 661), i.e. http_get's 10-minute default timeout (therock.rs:2301), per index URL, per runtime — while the spawned job has no timeout at all (crates/rocm-dash-tui/src/jobs.rs, only a cancel flag that nothing ever sets for this id). Meanwhile apps/rocm/src/therock.rs:36-37,729-745 already implements exactly this concern: a 12-hour on-disk cached record, a 2-second STARTUP_UPDATE_CHECK_TIMEOUT_SECS, and a ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK opt-out — none of which this path honours. Additionally update_check_due_at is in-memory (Instant), so the 6-hour interval resets on every TUI launch and a check fires on every startup regardless of the cached record. Fix: pass a bounded timeout into the check (or reuse maybe_refresh_startup_update_check_at's cached record), and honour the existing disable env var.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:216 — the tile's own plumbing job is rendered as user activity. draw_activity iterates state.jobs.jobs.values() unfiltered, and nothing ever removes jobs from that map, so home-update-check will appear in the Home "Activity · node" feed as ⋯ <path to rocm> at startup and again every 6 hours, competing for the take(feed.height) slots with genuine user-initiated actions (and at a nondeterministic position, since HashMap::values() is unordered). Fix: skip HOME_UPDATE_CHECK_JOB_ID in draw_activity's job loop, and add a test asserting the feed does not list it.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:899-913 — activity_glyph_key_absent_when_feed_full is vacuous: it passes with the production change reverted. The guard it claims to protect (if (feed.height as usize) > lines.len(), home.rs:236) is unobservable, because lines.truncate(feed.height as usize) at home.rs:242 runs after the push — an unconditional push is clipped to exactly the same output in every configuration (lines.len() below, equal to, or above feed.height). Delete the if and this test still passes. Fix: either drop the redundant guard and the test, or compute the key before the job loop so "no spare room" is a real branch, and assert that the last feed row still shows a real job.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:700-708 — updates_tile_shows_checking_while_pending still passes against a full revert. It leaves conn at the default ConnState::Initial, which the pre-PR conn-derived tile also rendered as "Checking…", so the assertion cannot tell the new pending branch from the deleted code it replaced. Fix (one line): set s.conn = ConnState::Connected { .. } as in the sibling test at home.rs:683 — the old implementation would then render "Up to date" and the test would fail.

Non-blocking

• README.md:257 — the rocm update usage line still omits --json, unlike the examine/storage report lines that do list their --json flag; the after_help examples (apps/rocm/src/main.rs:292) also lack a --json example.
• apps/rocm/src/therock.rs:587-606 — the doc calls UpdateJson the "structured counterpart to render_update_report", but it silently drops format, install_root, and source; either say so or add them.
• apps/rocm/src/therock.rs:5356-5377 — the test named ..._without_failing_whole_report registers only one manifest, so it never demonstrates that a successful row survives alongside a failing one; add a second, resolvable manifest and assert both rows appear.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:828-838 — held_legend_absent_when_hero_data_is_fresh passes if the whole legend feature is deleted; it is only meaningful paired with the "visible" test, which does discriminate. Acceptable as a truth-table pair, worth noting.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:231-241 vs main — main already replaced the empty-activity placeholder with an actionable hint; this PR's "no dead ends" work overlaps that area and will conflict, so rebase before merging rather than after.

[jussielo-amd]

Update since the last review comments:

• Merged origin/main to resolve the branch's merge conflict (content + one silent test-helper collision surfaced by the build); adopted main's newer NaN-safe held/legend logic from fix(cli,dash-tui,e2e-report): close remaining marker/legend gaps #354 over this branch's own helpers.
• Rebuilt, retested, and pushed (8fcd74d).
• Fixed a flaky windows-build-and-test failure: several refresh_update_status tests weren't serialized against the test that toggles ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK (a process-global env var), so they could race and spuriously skip the spawn they assert on. All now share the existing lock (10de0a3).
• Updated the PR description to reflect current state.

All CI checks are green.

[Copilot]

🟡 Changes recommended

Repair results are misclassified, focused hosts perform unintended checks, and timeout validation is incomplete.

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

Review details

Suppressed comments (1)

crates/rocm-dash-tui/src/app/mod.rs:2010

• This also schedules the Home-only check in focused hosts (args.focus.is_some()), including rocm bootstrap setup. dispatch deliberately excludes Bootstrap from startup update checking because resolution can provision Python/download data (apps/rocm/src/main.rs:1655-1668), but the first TUI tick reintroduces that hidden work even though focused hosts have no Home tile. Restrict this to the full dashboard.

                if !state.simulated {

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

Withdrawing this change request as superseded: all five blocking points from fd36728 are answered at 924cf0b. The glyph-vocabulary objection in particular no longer applies — the base branch resolved it and the author was right that it was not theirs to fix. A fresh review at the current head is being filed separately; only that newer one is operative.

[siloteemu]

🔴 Automated review · pr-review-watcher · 924cf0b

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Wires the dash TUI Home tab's Updates tile to a real rocm update --json background check (new CLI flag, JSON report, TUI reducer/job), plus a glyph key, a held-marker legend and empty-tile hints. Outcome: Needs work — all five prior objections are answered, but two of the remediation commits ship behaviour fixes with no regression test at any level, which AGENTS.md:82 forbids. Verified: cargo test -p rocm-dash-tui --lib → 726 passed, 0 failed; I traced every println! and Stdio::inherit reachable from render_update_json and confirmed stdout purity holds, confirmed --timeout-secs 0 is rejected at the clap boundary, confirmed the HOME_UPDATE_CHECK_JOB_ID feed filter and the revert-discrimination of each new TUI test, and confirmed by grep that SuppressProgressOutput and --timeout-secs have no test references anywhere in the tree. No prompt-injection content and no internal identifiers in the diff. Blocking: 2 · Non-blocking: 6.

Prior objection status

• Glyph semantics contradicting what had landed on the base branch — RESOLVED. The match arms are no longer in the diff at all (the merge resolved in favour of the base's five-arm vocabulary), and the new key line at home.rs:228 documents all six states including the nonzero-exit !.
• Unbounded background check bypassing the existing bounded machinery — PARTIALLY RESOLVED, and no longer blocking. The job now passes --timeout-secs 5 (app/mod.rs:497,1747) and honours ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK (app/mod.rs:1735), which was the blocking half. Two residues remain, both non-blocking below: the 6h interval is still an in-memory Instant that ignores the persisted record, and a bare rocm update --json (no --timeout-secs) still falls through to the 10-minute-per-call default.
• The tile's own plumbing job rendered as user activity — RESOLVED. home.rs:202 filters the job id out of the feed, and home_update_check_job_never_shown_in_activity_feed (home.rs:1109) fails if the filter is removed.
• activity_glyph_key_absent_when_feed_full vacuous — RESOLVED. The unobservable guard was deleted, home.rs:224-226 now carries a comment explaining why push-then-truncate is sufficient, and the replacement test asserts the key's presence, so it fails if the key line is removed.
• updates_tile_shows_checking_while_pending vacuous — RESOLVED. It now sets conn = Connected (home.rs:919) with a comment stating exactly why, so the old conn-derived implementation would render "Up to date" and the test would fail on revert.

🚫 Blocking (must fix before merge)

• apps/rocm/src/therock.rs:3718-3760, 4085-4110 — the stdout-purity fix ships with no regression test at any level. AGENTS.md:82 requires every bug fix to ship with a test that fails before and passes after. The commit fixes a real defect (wheel-manifest resolution falls through to Python resolution/bootstrap, which printed progress lines and an installer's raw stdout ahead of the JSON, breaking the documented single-line contract), but SuppressProgressOutput has exactly two references in the tree — its definition and the single call site at therock.rs:904 — and no test anywhere. The e2e scenario that looks like it covers this (update-02, "stdout is a single line of JSON") runs on a machine with no managed runtimes, so it never loads a manifest, never resolves, and never reaches any progress_line call; it passes identically with this commit reverted. The false sense of coverage is itself the problem: the next reader sees a single-line-stdout assertion in the feature file and concludes the contract is protected. Fix: add a unit test in therock.rs's test module asserting the guard's contract — progress_output_suppressed() false outside, true inside a SuppressProgressOutput scope, still true under nesting, false again after both drop — plus an assertion that render_update_json leaves the flag restored on both the Ok and Err paths. That fails before the commit and passes after. If the wheel path genuinely cannot be covered in default CI, state that gap in the PR text and name what covers it instead, per AGENTS.md:84-86.

• apps/rocm/src/main.rs:316 — "reject zero timeout" ships untested, and the repo already has the exact idiom for it. Nothing in the tree constructs rocm update --json --timeout-secs 0: grep finds no test reference to --timeout-secs at all. If .range(1..) were dropped from the timeout_secs arg, zero would silently reach http_get, which treats Some(0) as "use the 10-minute default" (therock.rs:2872-2874) — i.e. the exact silent fallback the commit says it is preventing — and no test would fail. The fix is three lines, mirroring chat_rejects_zero_max_tokens (main.rs:20163): Cli::try_parse_from(["rocm","update","--json","--timeout-secs","0"]).expect_err(...) and assert ErrorKind::ValueValidation.

Non-blocking

• apps/rocm/src/therock.rs:2872-2874 — a bare rocm update --json (no --timeout-secs) uses the general-purpose 10-minute-per-call default, and one wheel manifest costs 5+ sequential index calls, so the documented JSON entry point can stall for a very long time for any consumer that is not the TUI; consider a small --json-specific default.
• apps/rocm/src/therock.rs:2872 — http_get maps Some(0) to the default rather than an error, so "zero is rejected" is an artefact of one call site's validator, not an invariant of the primitive.
• crates/rocm-dash-tui/src/app/mod.rs:701,797 — update_check_due_at is an in-memory Instant reset by AppState::new, so a check fires on every TUI launch and the persisted record at therock.rs:1137+ is never consulted.
• apps/rocm/src/therock.rs:1045,4239 — a background, now fully silenced update check can reach ensure_managed_python, which downloads uv and installs a Python toolchain; --timeout-secs bounds only the index HTTP calls, not that bootstrap. Pre-existing on the CLI startup-check path, but newly reachable once per TUI launch.
• crates/rocm-dash-tui/src/app/mod.rs:7081 — refresh_update_status_spawn_args_include_bounded_timeout compares against the very constant it tests, so it proves a bound exists but not that it is a sane one.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:1088 — cancelled_job_renders_distinct_glyph_from_running exercises match arms that already exist in the base, so it still passes with this PR's home.rs production changes reverted; it is a characterization test, not coverage of anything this PR adds.

Superseding this change request with a fresh one at the current head, so only one objection is live. Half of the original objection is now answered; the new review states exactly which half remains and why.

[siloteemu]

🔴 Automated review · pr-review-watcher · c654709

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds rocm update --json (plus a --timeout-secs bound), suppresses progress output on that path so the JSON stays a single clean line, and wires the dash TUI's Home Updates tile to a real periodic background check instead of inferring freshness from connection state. Outcome: Needs work. Verified: targeted cargo test revert experiments in a scratch copy (the checkout was left untouched) — reverting .range(1..) fails update_rejects_zero_timeout_secs; reverting the Updates-tile logic fails all six updates_tile_* tests; reverting the activity-feed glyph key and the update-check job filter each fail their test; but deleting SuppressProgressOutput::new() from render_update_json and/or the early return in progress_line, singly and together, leaves all 18 matching progress/update_json tests green. Also confirmed by reading prw-base that HELD_LEGEND rendering already exists on the base, and that a commit body in this range names an internal tracking document. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/therock.rs:904 and :3750 — the progress-output suppression fix has no regression test; the tests that read as covering it do not.

This was the substance of the open change request and it is still live for this half of the PR. Execution-verified in a scratch copy:

• Delete let _quiet = SuppressProgressOutput::new(); from render_update_json → cargo test -p rocm --bin rocm -- progress update_json render_update_json (18 tests) all pass.
• Delete the if progress_output_suppressed() { return; } early return in progress_line → same 18 tests all pass.
• Delete both → same 18 tests all pass.

Root cause: every render_update_json fixture uses either an empty manifest set or a manifest whose channel fails TheRockChannel::parse synchronously, so the loop body never reaches resolve_latest_for_manifest → the wheel/Python-bootstrap path that is the only thing that calls progress_line or ensure_managed_python. The two tests added for this — suppress_progress_output_contract (:4655) and render_update_json_restores_progress_suppression_on_ok_and_err_paths (:4692) — only exercise the thread-local flag's own bookkeeping: the latter asserts !progress_output_suppressed() before and after the call and never during, so it is trivially true when the guard is never installed. The new e2e scenario cannot close the gap either (see blocking item 2).

Concrete fix: make progress_line write through an injectable sink rather than println! directly (the install_stdio() closure already establishes half of that seam), then unit-test that a SuppressProgressOutput guard produces an empty buffer while an unguarded call does not. A cheaper interim option: assert progress_output_suppressed() from inside the resolution path — e.g. a #[cfg(test)] observation point in runtime_update_plan — so the assertion fires while render_update_json is on the stack. Either way the test must fail when line 904 is deleted.

2. tests/e2e-cucumber/features/update.feature:16-20 — the new scenario cannot fail from the bug it is presented as covering, and its Then step violates the suite's documented convention.

load_runtime_manifests returns Ok(Vec::new()) when the registry directory does not exist, so on "a machine with no managed runtimes" render_update_json's loop has zero iterations and neither progress_line nor ensure_managed_python is ever reached — with or without the suppression guard. The scenario proves the empty-array JSON envelope (already covered by the unit test update_json_reports_no_managed_runtimes_as_empty_list) and nothing about stdout cleanliness. Presenting it as e2e coverage for the suppression fix makes the gap in item 1 look closed when it is not.

Separately, Then stdout is a single line of JSON with an empty runtimes array states implementation detail in the feature file. tests/e2e-cucumber/README.md:241 is explicit: "Feature files describe what users care about, not implementation details. How steps are implemented … stays in the step functions." Every sibling Then in this suite follows that (e.g. examine.feature's "the machine-readable form states everything the readable one does").

Concrete fix: rephrase the Then behaviourally — "the machine-readable check reports no runtimes to update" — and keep the single-line/valid-JSON mechanics in the existing Rust step body, which already asserts them correctly. Then either add a scenario with a managed runtime whose resolution takes the wheel/Python path (so the suppression fix is genuinely exercised), or say plainly in the PR body that this scenario covers the JSON envelope only and that the stdout-suppression fix is covered elsewhere — once it actually is.

3. Commit 1306087 message body names an internal tracking document.

The body opens with a reference to an internal "UX Action Plan" in a named internal wiki system. This repo's own AGENTS.md §2 forbids exactly that: "do not include links to internal tracking systems … apply this rule to … commit messages." This is public, permanent history once merged. Concrete fix: reword that line to describe the change on its own terms (e.g. "Home tab honesty fixes:") and amend/rebase the branch before merge. The rest of the diff and the other commit messages scanned clean.

Non-blocking

• apps/rocm/src/therock.rs:1018 — the new download_timeout_secs parameter is also untested: reverting resolve_latest_for_manifest(paths, manifest, download_timeout_secs) back to None leaves every targeted test green. The workspace clippy gate runs with -D warnings, so the resulting unused-parameter warning would likely fail CI — I am inferring that from .github/workflows/ci.yml and cannot confirm which lane exercises it; a lint is not a behavioural guard either way.
• crates/rocm-dash-tui/src/ui/tabs/home.rs:1026-1043 — held_legend_visible_when_hero_data_is_held / _absent_when_hero_data_is_fresh characterize HELD_LEGEND rendering that already exists at the base (home.rs:315, :424 are unchanged by this diff), so they pass with every production change here reverted. The sibling cancelled_job_renders_distinct_glyph_from_running labels itself as a characterization guard; these two do not, and the PR title's "add held … legends" reads as if the legend is new in this diff. Add the same one-line label, and scope the title to the glyph key.
• crates/rocm-dash-tui/src/app/mod.rs (reduce_update_json) — a row with status update_available/repair_available but a null latest_version is silently skipped by the .then(...).flatten() chain and then fails the all-up_to_date check, so an actionable update renders as "check failed". Unreachable today because the producer always sets Some, but it is a silent swallow across a cross-crate JSON boundary; prefer an explicit branch.
• crates/rocm-dash-tui/src/app/mod.rs (refresh_update_status) — the "pending but job vanished" branch clears update_status_pending without re-arming update_check_due_at, so a check would re-spawn on the very next tick. Not reachable today (jobs are never removed), but a one-line due_at bump makes the no-busy-loop property hold unconditionally.
• apps/rocm/src/therock.rs:4680 — render_update_json_test_paths duplicates the existing test_paths helper but roots under std::env::temp_dir() instead of the repo-local work dir, and cleans up only on the non-panicking path; the two tests immediately below it use test_paths. Reuse the existing helper. Also worth one line on UpdateJson's doc comment: a load_runtime_manifests failure emits no JSON at all and exits non-zero, which is a third outcome consumers must handle alongside runtimes: [] and all-error rows.

On the standing objection

Partially resolved. The timeout-rejection fix is now genuinely protected — that half of the objection is answered, and the previously misleading tests are honest (the timeout-args assertion is pinned to a literal rather than to the constant, and the cancelled-glyph test says in its own comment that it guards pre-existing behaviour). The progress-output-suppression half is not resolved: it remains unprotected, and the remediation reproduced the exact defect the objection named — render_update_json_restores_progress_suppression_on_ok_and_err_paths reads as guarding the fix and does not. A competent reader will trip on that name again; the cheap prevention is to rename it to what it actually proves (the guard releases on both exit paths) and add one line saying it does not prove that render_update_json installs the guard.