fix(cli,dash-tui,e2e-report): close remaining marker/legend gaps

[jussielo-amd]

Summary

• Explain the * default-engine marker in rocm engines list / examine output via a shared DEFAULT_ENGINE_MARKER constant and write_default_engine_legend helper, so the two renderers can't drift.
• Share one LEGEND_ENTRIES table between the HTML and Markdown expectation-grid legends in e2e-report, and extend it to explain FlakyXpass, RanWhenNa, and Absent (previously unexplained glyphs).
• Show the dash TUI's HELD_LEGEND row on the home, instances, and serving tabs whenever a displayed value is actually held (gated correctly on gen_tps being shown, not just observation metadata being present).
• Add e2e (Gherkin) and unit coverage for the new default-engine legend and each held-legend gating fix.

Test plan

• cargo test -p e2e-report — 44/44 passed
• cargo test -p rocm-dash-tui --lib — 667/667 passed
• cargo test --bin rocm -- marker_legend — 3/3 passed
• cargo clippy --all-targets -- -D warnings clean for e2e-report, rocm-dash-tui, and the rocm binary
• Full tests/e2e-cucumber run for the new examine-engines-list-shows-default-engine-legend scenario (not run to completion in this environment due to build time; the equivalent unit tests pass and the step assertions are simple substring checks)

Known follow-ups (non-blocking, noted in review)

• crates/rocm-dash-tui/src/ui/services_manager.rs also renders the held marker via format::gen_tps_compact but doesn't show HELD_LEGEND — same gap this PR closes elsewhere, missed here.
• instances.rs's any_held check scans the full instance list rather than only the rows that actually fit/render in the table, so the legend could show even if the held instance is scrolled out of view on hosts with many instances.
• e2e-report's HTML legend only color-styles the Xfail/UnexpectedFail glyphs to match their grid-cell color; Xpass, RanWhenNa, and Absent render red in the grid but plain in the legend.

[Copilot]

🟡 Changes recommended

The default-engine legend is inaccurate, and several dashboard legends can appear without a displayed marker.

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

Pull request overview

Adds shared marker legends across CLI engine output, dashboard views, and e2e reports, with expanded test coverage.

Changes:

• Shares default-engine and expectation-grid legend definitions.
• Adds conditional held-value legends to dashboard tabs.
• Adds unit and Gherkin coverage.

File summaries

File	Review
tests/e2e-cucumber/tests/e2e/examine_steps.rs	Implements engine-marker assertions.
tests/e2e-cucumber/features/examine.feature	Adds engine-list coverage, but lacks required rocm examine coverage (nit).
crates/rocm-dash-tui/src/ui/tabs/serving.rs	Tests serving-tab legend behavior.
crates/rocm-dash-tui/src/ui/tabs/pane.rs	Legend may appear without a finite displayed value (moderate).
crates/rocm-dash-tui/src/ui/tabs/instances.rs	Legend gating includes clipped rows and non-finite values (moderate).
crates/rocm-dash-tui/src/ui/tabs/home.rs	Zero throughput can produce a legend without a marker (moderate).
crates/e2e-report/src/lib.rs	Unifies HTML and Markdown expectation legends.
apps/rocm/src/main.rs	The engines-list legend can misidentify the effective configured default (critical).

Review details

Suppressed comments (1)

tests/e2e-cucumber/features/examine.feature:135

• This Gherkin coverage exercises only rocm engines list, while the PR also adds the user-visible legend to rocm examine at apps/rocm/src/main.rs:13718. The repository contribution policy requires a scenario for each changed CLI-observable behavior rather than relying only on the new unit test. Add or extend an inspection scenario to assert the marker legend in rocm examine output too.

  @id:examine-engines-list-shows-default-engine-legend
  Scenario: examine-13 - Listing engines explains the default-engine marker
    When the user lists available engines
    Then the engine listing explains the default-engine marker
    And the host's default engine is marked in the listing

• Files reviewed: 8/8 changed files
• Comments generated: 4
• 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

Multiple dashboard views can show missing, hidden, or incorrectly triggered held-value legends.

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

Review details

Suppressed comments (1)

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

• The held legend is user-visible dashboard behavior, but this PR only adds renderer unit tests for it. The existing @id:dash-gen-tps-held-after-scrape-failure scenario already creates a real Held observation in the interactive TUI; extend that scenario/step (and navigation as needed for the other changed surfaces) to assert the marker is explained end to end.

    if tpw > 0.0 && any_tpw_held {
        f.render_widget(
            Paragraph::new(Line::from(Span::styled(
                format::HELD_LEGEND,
                Style::default().fg(theme.muted),
            ))),
            lh[5],
        );

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

[siloteemu]

🔴 Automated review · pr-review-watcher · 796911c

Summary

Adds "held" (*) marker legends across five dash-TUI surfaces, guards NaN/Inf out of the tok/s and tok/W aggregates, shares one legend table between the e2e-report HTML and Markdown grids, and explains the * default-engine marker in rocm engines list / rocm examine (plus two new Gherkin scenarios). Verdict: Needs work — one vacuous assertion, everything else is sound. Verified: I read every changed hunk plus surrounding context and answered the revert question per test against the base revision; the held-legend tests in home.rs, observe.rs, widgets.rs, serving.rs, instances.rs and services_manager.rs all fail if reverted (pre-PR code renders no HELD_LEGEND in those files at all), expectation_grid_legend_explains_every_glyph is non-vacuous because its fixture only ever emits ✅ in real cells, and the e2e steps derive the expected engine from capability.rs's deliberately independent rule rather than the code under test; I also confirmed lh[5]/rh[5] were pre-existing unused rows (no panic), visible_rows/visible_list_window match ratatui 0.3.2's real Table/List layout in the safe direction, format::tokens_per_watt is still live via hardware.rs:309, and LEGEND_ENTRIES covers all 9 CellOutcome variants; I relied on CI for compilation and the full suite. No prompt-injection content found. Blocking: 1 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:28523-28529 — render_engine_inventory_text_includes_marker_legend claims to prove "legend appears before the entries it explains", but it searches the whole output for "* ", and the legend line itself ( legend: * = default engine) contains that exact substring. marker_pos therefore lands inside the legend (offset legend_pos + 8), so legend_pos < marker_pos is arithmetically true no matter what the row loop does — the assertion would still pass if the per-row if *name == default_engine { DEFAULT_ENGINE_MARKER } else { " " } marking were reverted or broken, and expect("a marked engine entry present") never actually proves a marked entry exists. Fix: search only past the legend, e.g. rendered[legend_pos + legend.len()..].find(&format!("{DEFAULT_ENGINE_MARKER} {default_engine}")), anchoring on the real row shape as the sibling test examine_engine_inventory_includes_marker_legend already does with "* vllm".

Non-blocking

• Sibling-PR overlap — #357 ("stop asserting update freshness, add held/glyph legends") touches exactly this surface: held legends in the dash TUI and glyph legends in the report. This PR redefines HELD_LEGEND placement in home.rs, instances.rs, serving.rs/pane.rs, services_manager.rs and rewrites the e2e-report grid legend into LEGEND_ENTRIES. Reconcile the two explicitly (say in the PR body which one owns each surface) or they will conflict textually and semantically.
• crates/e2e-report/src/lib.rs:2892 — expectation_grid_legend_explains_every_glyph checks only 3 hardcoded glyphs despite its name, and nothing forces LEGEND_ENTRIES to stay exhaustive: a 10th CellOutcome variant compiles fine with no entry and leaves this test green, contradicting the "can't drift apart" claim at lib.rs:736. Iterate a const ALL: [CellOutcome; N] (or an exhaustive match) so the compiler/test enforces coverage.
• crates/rocm-dash-tui/src/ui/tabs/instances.rs:184-196 — widening TOK/W from Length(7) to Length(12) pushes fixed column consumption from 54 to 59 cells, cutting the MODEL column's headroom above its Min(12) floor from 9 to 4 at an 80-column terminal; every render test in the file uses TestBackend::new(160, …), so nothing covers 80 cols. Add one 80-column render assertion, or narrow the column.
• apps/rocm/src/main.rs:13527 vs tests/e2e-cucumber/tests/e2e/examine_steps.rs:185,204 — the literal legend: * = default engine is hand-duplicated across the binary crate and the black-box e2e steps with no compiler check; a wording change silently desyncs them (whitespace prefixes were verified correct today).
• crates/rocm-dash-tui/src/ui/tabs/home.rs:670-676,765 — assert!(out.contains(format::HELD_MARKER)) is implied by the neighbouring contains(HELD_LEGEND) assertion, because HELD_LEGEND is "* = held (prior scrape window)" and literally contains *. It would not catch a regression where the marker stops being appended to the value; assert the specific rendered cell (e.g. "300*") instead.
• crates/rocm-dash-tui/src/ui/services_manager.rs:570 — visible_list_window_matches_ratatui_minimal_scroll_behavior only pins the helper against hand-derived expectations, not real ratatui; the claim is genuinely corroborated by the two TestBackend snapshot tests beside it, so just rename it (e.g. ..._matches_hand_derived_model) so the name stops overstating what it alone proves.

[jussielo-amd]

@siloteemu thanks for the thorough pass — addressed in 6ce541f:

Fixed:

• main.rs: the marker-legend test asserted a substring that also matched inside the legend line itself, so it would have passed even with no row actually marked. Now anchors on the specific "{DEFAULT_ENGINE_MARKER} {host_default}" row.
• home.rs (x2): held-marker tests were checking for the bare * marker, which also occurs inside HELD_LEGEND's own text — so they'd pass even if the aggregate cells stopped being marked. Now assert the exact rendered cell strings ("300*", "1.0 tokens / watt*", etc.) alongside the legend.
• services_manager.rs: renamed visible_list_window_matches_ratatui_minimal_scroll_behavior → visible_list_window_matches_hand_derived_model, since it checks against a hand-derived scroll model, not ratatui internals.
• e2e-report/src/lib.rs: added legend_entries_stay_exhaustive_with_cell_outcome, an exhaustive match over CellOutcome inside the test that fails to compile if a variant is added without a corresponding LEGEND_ENTRIES explanation.
• instances.rs: added table_at_80_columns_keeps_tok_w_marker_and_model_readable, rendering at 80 columns to confirm the widened TOK/W column (Length(12)) doesn't clip the held marker, and MODEL stays legible above its Min(12) floor.

Not changing: the main.rs/examine_steps.rs literal duplication you flagged — tests/e2e-cucumber is a documented black-box suite that intentionally doesn't import internal crate code (see tests/e2e-cucumber/README.md), and apps/rocm is a binary crate the e2e tests can't depend on anyway. A shared constant would break that boundary, so this is accepted as a tradeoff rather than fixed.

Deferred: the overlap with #357 on home.rs/instances.rs — will coordinate once that PR is further along.

All 5 fixes verified: cargo test -p rocm-dash-tui --lib (688 passed), cargo test -p e2e-report (46 passed), cargo test -p rocm --bin rocm marker_legend (3 passed), plus cargo clippy clean on all touched crates.

[siloteemu]

🔴 Automated review · pr-review-watcher · e9d278d

Summary

Adds "held observation" markers plus matching legends across the dashboard tabs, the engine-inventory CLI output and the test report; still good — no blocking findings. Verified: the only new content since the last round is the main merge, which hit one real conflict in home.rs's test module — recomputing the merge with git merge-tree shows the sole manual edit was deleting the three conflict markers and adding one closing brace, no content changed; both sides' tests survive intact (15 unique test fns, nothing dropped from either parent), no conflict markers remain anywhere, and cargo test -p rocm-dash-tui --lib ui::tabs::home passes 12/12 including both the branch's held-marker tests and main's newly merged activity_feed_glyphs_match_job_console_vocabulary. The merge introduced no semantic collisions (main added no new gen_tps/tok-W render surface, examine.feature gained no duplicate scenarios and examine_steps.rs no ambiguous step regexes, and default_engine_for_host/detect_host_gpu_summary are intact with the marker test's anchor still matching production's row format). Of the five open items, #3 turns out to be narrower than reported and #1, #2, #4, #5 are unmoved; nothing new was introduced. On the revert question, this push added no tests of its own; re-checking the previously-added ones, home.rs:676/680/777 and main.rs:29176 all fail if the marker/legend production code is reverted, while the MODEL half of the new 80-column test cannot (item 2 below). Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/e2e-report/src/lib.rs:2945 — the exhaustiveness hole is narrowed but not closed, and the comment above it overstates the guarantee. A 10th CellOutcome does trip a compile error in expected_text, but the minimal fix (add one match arm) leaves const ALL: [CellOutcome; 9] and LEGEND_ENTRIES at 9, so assert_eq!(LEGEND_ENTRIES.len(), ALL.len()) passes and the new variant is never checked — the test stays green with an unexplained glyph. strum 0.28 is already in the lockfile: deriving EnumIter on CellOutcome and iterating it removes ALL entirely. Failing that, drop the "can't silently miss a 10th variant" claim and put the reminder where the compiler stops you — a line inside the expected_text match body saying a new arm also needs an ALL and a LEGEND_ENTRIES entry.
• crates/rocm-dash-tui/src/ui/tabs/instances.rs:1421 — the MODEL half of the new 80-column test cannot fail from the change it documents. TOK/W went Length(7) → Length(12); at 80 columns MODEL resolves to ~19 cells, and reverting the widening gives it ~24, so "llama-3.1-8b" is present either way. Only a change narrowing MODEL below 12 breaks it. The "0.42 tok/W*" assertion is genuinely load-bearing (it clips to "0.42 to" at Length(7)) — either drop the MODEL assertion or reword the comment so it does not read as protecting MODEL's headroom against this change.
• crates/rocm-dash-tui/src/ui/tabs/observe.rs:200 — the aggregate left out of the pattern is the power sum, not a throughput one: gen_tps is now finite-filtered two lines above, but snap.gpus.iter().map(|g| g.power_w).sum() is not, and format::watts (format.rs:175) is a bare {value:.1} W, so a single non-finite power_w renders "NaN W". node_efficiency escapes this only because it re-guards the quotient. Extending .filter(|v| v.is_finite()) to the power sum completes the pattern; the every-aggregate framing in the surrounding comments currently implies it is already there.
• apps/rocm/src/main.rs:6761 and :13666 — confirmed still co-occurring, and it is genuinely the same command: examine_human_report calls append_examine_runtime_state and append_examine_engine_inventory back to back, so one rocm examine run prints legend: * = active, - = rollback target and legend: * = default engine in the same output. Each is locally scoped to its own section so nothing is ambiguous in practice today, but the reconciliation deferred against the sibling PR is still owed.
• Declined item (duplicated user-visible string across the binary crate and the black-box tests) remains correctly declined — a black-box test should pin the literal, since sharing a constant would let a wording change keep the test green. No action.

Note on CI: a conclusion-accurate rollup for this exact head showed 25 success, 1 skipped, 0 failures, 0 pending; no result is attributed to a specific named job.

The blocking finding is fixed at 6ce541f, so withdrawing this change request. The marker test now anchors on the marker-plus-name row, which cannot match inside the legend line, and it fails under either revert — dropping the row marking or dropping the legend. The two held-marker tests were fixed the same way, asserting exact rendered cells rather than a character that also appears in the legend text. On the shared-constant item you declined: the conclusion is right and I am not pressing it, but the stated reason is half wrong — the e2e suite does already depend on a first-party crate, so the boundary argument does not hold. The better argument is the one for keeping it: a black-box test should pin the literal user-visible string, because sharing the constant would let a wording change keep the test green. Worth recording that reasoning instead. Five non-blocking notes in the review comment; the exhaustiveness guard still has a residual hole worth closing.

[siloteemu]

✅ Automated review · pr-review-watcher · e9d278d

Reviewed at this head; no blocking findings. The change adds held-observation markers and matching legends so a stale reading is distinguishable from a live one across the dashboard, the CLI and the test report. The merge commit in this round was verified to change no content beyond conflict-marker removal. Five non-blocking notes stand in the earlier review comment; none of them gate a merge.