fix(tui): unify dialog behavior and dimmed-backdrop UX across overlays

[jussielo-amd]

Summary

• Unify dialog behavior across the ROCm dash TUI (Esc consistency across tabs, approval-dialog default, dim backdrop, unified help overlays, Esc→menu discoverability).
• Address review findings: decouple the shared ApprovalChoice default (used by 10 manager confirm flows) from the chat tool-call approval, which now defaults to Deny explicitly; widen should_pane_back_out so Esc closes a manager overlay opened from any tab, not just Rocm/Serving; keep the footer's Esc chip in sync; fix a clippy::too_long_first_doc_paragraph lint; update an e2e assertion to match the current Help-modal title format.
• Extend the dimmed-backdrop treatment (already used by Help/Menu/Palette/Options/manager overlays/approval) to the two remaining popups that lacked it: the theme picker and the Observe instance-detail overlay.

Test plan

• cargo build -p rocm-dash-tui
• cargo test -p rocm-dash-tui --lib (659 passed)
• cargo clippy -p rocm-dash-tui --all-targets -- -D warnings (clean)
• cargo test --workspace --all-targets (131 passed, non-e2e)
• e2e-cucumber dash scenarios (e.g. dash-05) — not runnable in this sandbox (PTY-based harness); the updated assertion was verified statically against draw_help's actual title format.
• Manual TUI check: Esc from idle Chat opens the menu; fresh approval prompt defaults to Deny; theme picker and instance detail now render with a dimmed backdrop like other overlays.

[Copilot]

🟡 Changes recommended

The Help layout clips shortcuts at the standard terminal size, footer Escape behavior is inconsistent in several states, and user-visible flows lack required scenario coverage.

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

Pull request overview

Unifies dashboard overlay behavior, Escape-key routing, approval defaults, help presentation, and modal backdrops.

Changes:

• Standardizes Escape handling and footer guidance across tabs and managers.
• Defaults chat tool-call approvals to Deny.
• Aligns help, theme picker, and instance-detail overlay presentation.

File summaries

File	Description
tests/e2e-cucumber/tests/e2e/dash_steps.rs	Updates the Help assertion for the uppercase tab heading.
crates/rocm-dash-tui/src/ui/tabs/instances.rs	Dims the instance-detail backdrop.
crates/rocm-dash-tui/src/ui/modal.rs	Unifies Help layout and dims additional overlays.
crates/rocm-dash-tui/src/ui/mod.rs	Adds contextual Escape footer guidance.
crates/rocm-dash-tui/src/ui/approval.rs	Documents approval-default semantics.
crates/rocm-dash-tui/src/app/mod.rs	Updates Escape routing, approval defaults, and tests.

Review details

Suppressed comments (2)

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

• The new idle-Chat Esc-to-menu behavior is only asserted through handle_key; there is no user-level scenario proving that focused Esc still blurs first and a subsequent idle Esc opens the menu. Please add this flow to dash.feature so the observable routing and menu rendering are covered end to end.

        KeyCode::Esc if matches!(current, ActiveTab::Rocm | ActiveTab::Serving) => {
            KeyAction::PaneEscape
        }
        KeyCode::Esc => KeyAction::OpenMenu,

crates/rocm-dash-tui/src/ui/modal.rs:188

• The theme picker's new dimmed-backdrop behavior has no scenario or render assertion that distinguishes it from the previous undimmed output. Add user-level coverage that opens the picker and verifies the backdrop styling so this visible UX change cannot regress unnoticed.

    grey_overlay(f);

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

Help scrolling can become stuck after overscrolling, and the mock approval trigger can repeatedly reopen after approval.

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:1127

• The renderer clamps only the value it draws, leaving help_scroll overscrolled. After G sets this to 32767, pressing k changes it to 32766, which is still rendered as the last page; repeated downward scrolling at the bottom has the same effect. Users can therefore need thousands of reverse-scroll presses before the help view moves. Keep the stored offset clamped to the rendered max_scroll (or represent Home/End as absolute actions) so reverse scrolling works immediately.

    /// (no upper bound — the renderer clamps against the actual line count).
    pub fn scroll_help(&mut self, delta: i16) {
        let cur = i32::from(self.help_scroll);
        let next = u16::try_from((cur + i32::from(delta)).max(0)).unwrap_or(u16::MAX);
        self.help_scroll = next;

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

[siloteemu]

🔴 Automated review · pr-review-watcher · d54e7a7

Summary

This PR makes the dash TUI's help overlays scrollable, dims the backdrop behind the instance-detail and theme-picker popups, flips the chat tool-call approval default to Deny, generalizes Esc's overlay back-out to every tab, rebuilds the footer's Esc chip as a priority chain, and adds five e2e scenarios plus a mock approval trigger. Verdict: Needs work — two blocking issues, both small fixes. Verified: cargo clippy -p rocm-dash-tui --all-targets -- -D warnings and -p e2e-cucumber both clean; cargo test --workspace --exclude e2e-cucumber green (665 + 552 + 329 + … , 0 failures); I independently confirmed (a) help_scroll is never clamped in state and no code path writes a clamped value back, (b) every one of the 13 overlay types has a dedicated event-loop arm ahead of the generic handler and each self-closes on root Esc, (c) the footer chip chain matches the real routing order in event_loop/handle_key, (d) grey_overlay's wash 0x1c1e22 differs from default_dark.bg 0x131416 so the e2e backdrop assertion is not vacuous, (e) no duplicate Cucumber step definitions across tests/e2e-cucumber/tests/e2e/, and (f) no prompt-injection content anywhere in the diff. Blocking: 2 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. crates/rocm-dash-tui/src/app/mod.rs:1124 — scroll_help never clamps state, so "jump to end" then scrolling back up is dead.
scroll_help clamps at 0 but has no upper bound; the only clamp is inside draw_scrollable_lines (ui/modal.rs:83-86), which is display-only and never written back. G/End sends ScrollModal(i16::MAX), setting help_scroll = 32767 while the real max is ~10 rows. Pressing k/Up afterwards decrements 32767 → 32766 → … , so the view does not move for ~32 700 keypresses; PageUp needs ~3 270. Only g/Home recovers. A second G saturates to 65535, after which g (delta = i16::MIN = -32768) lands on 32767 instead of 0 — so even the escape hatch stops working. Neither the unit tests (which only exercise the renderer clamp) nor the new e2e step (which scrolls to end and immediately closes help) cover the round trip. The doc comment at mod.rs:1121 ("no upper bound — the renderer clamps") states the design that causes the bug. This is also a "don't inherit a bad pattern" case: it was copied from scroll_bench_detail, whose modal the PR itself notes is unreachable, so the latent bug never surfaced there. Fix: follow the pattern already in this crate — ui/tabs/chat.rs:294 writes the measured max_scroll back into state.chat_max_scroll, and set_chat_scroll / scroll_console / scroll_dock clamp against it. Add a help_max_scroll: u16 set by draw_scrollable_lines (or by draw_help/draw_global_help) and clamp in scroll_help with .clamp(0, max), treating i16::MIN/i16::MAX as absolute jumps to 0 / help_max_scroll. Add a unit test asserting G then k moves the offset by exactly one row.

2. crates/rocm-dash-tui/src/app/mod.rs:1049-1053 — the stated justification for dropping the tab guard from should_pane_back_out is factually wrong, and dash-11 is presented as regression coverage for a bug that does not exist.
The new comment claims that with a manager open on a non-domain tab, Esc "falls through to the global OpenMenu arm while the manager overlay keeps rendering on top, leaving Modal set but invisible." That cannot happen. All 13 overlays have their own Some(Ok(CtEvent::Key(k))) if state.<overlay>.is_some() arms at mod.rs:2038-2152, all placed before the generic handle_key arm at mod.rs:2153, and none is gated on active_tab; every one self-closes on root Esc (services_manager.rs:162, engine_manager.rs:149, examine_manager.rs:64, runtime_manager.rs:244, install_manager.rs:218, config_manager.rs:129, automations_manager.rs:136, update_manager.rs:149, command_screen.rs:114, bench_run.rs:95, serve_wizard.rs:303, …). Modal is orthogonal — opening a manager never sets it and close_overlays never touches it. Consequently dash-11 (features/dash.feature:129) and the reworded unit test back_out_requires_an_open_manager_on_any_tab (mod.rs:3721, comment: "item #35: no dead corner where an overlay survives a tab switch and swallows Esc silently") would both pass unchanged on the base branch, which conflicts with AGENTS.md §3's "test fails before fix and passes after fix". The code change itself is harmless (it just moves the close from the manager's on_key to close_overlays() + pane_focus = Actions). Fix: reword the comment to describe what the change actually is — a harmless generalization that removes a tab-specific special case and guards future managers that might not self-close — and reframe dash-11 and the unit-test comment as new coverage of existing behavior rather than a regression test for item #35, or say in the PR body which lane/commit the regression actually reproduces on.

Non-blocking

• crates/rocm-dash-tui/src/ui/modal.rs:94-98 — draw_help's doc comment is garbled: the scroll sentence runs into a leftover fragment ("scroll is the first visible line offset …: a fixed two-column split used to clip content at small terminal sizes"), describing the old layout as if it were the parameter's meaning.
• crates/rocm-dash-tui/src/ui/modal.rs:581 — flattening draw_global_help from two columns to one makes it 20 lines where each old column was ≤11; at the e2e-standard 80x24 the two-column version fit without scrolling and the new one does not, so this trades a fits-on-screen reference for a scrollable one (the per-tab Help genuinely needed it; the global one didn't).
• crates/rocm-dash-tui/src/agent.rs:1611 — let _ = trigger.tx.send(...) silently drops the approval request if the channel is closed; AGENTS.md §"no silent failures" argues for at least a debug!/warn! on the error arm.
• docs/demos.md:51 — still says "--chat-mock provides the deterministic offline chat response", but --chat-mock now also carries a hidden "install the sdk" phrase that pops an approval modal; the demo doc should mention the trigger or the scaffolding should be gated out of the shipped demo path.
• crates/rocm-dash-tui/src/ui/mod.rs:433 and :599 — the "Esc back out" chip fires on has_open_overlay() alone, but the real back-out additionally requires active_overlay_at_root(), so with a sub-popup or job console open the chip advertises an action Esc won't perform; and the new footer test ends in let _ = esc;, which exists only to dodge an unused-variable error — bind directly with let _ = chips.iter().find(...).expect(...) instead.

[jussielo-amd]

Pushed 251d5ae addressing this review, in scope order:

Blocking

1. help_scroll had no upper clamp — jumping to the end of Help (G) set it to u16::MAX, so scrolling back up took ~32k keypresses. Added help_max_scroll, written back by the renderer each frame (mirroring chat_max_scroll), and clamp scroll_help against [0, help_max_scroll]. New unit test reproduces the jump-to-end-then-scroll-up sequence.
2. Corrected the should_pane_back_out doc comment, its unit test comment, and confirmed the dash-11 e2e scenario title don't claim a bug that doesn't exist — every overlay already self-closed on root Esc via its own event-loop arm regardless of tab, so the tab-guard removal is a harmless generalization, not a fix for a "Modal stays set but invisible" bug. No production logic changed for this item.

Non-blocking

1. Removed the garbled trailing clause in draw_help's doc comment (folded into fix 1's touch of that area).
2. Global-help two-column → one-column: left as-is — it's an intentional consequence of unifying both help screens onto the shared scrollable renderer, not an oversight. Happy to restore the two-column layout if you'd rather keep it.
3. agent.rs: the mock approval trigger's send failure now logs via warn! instead of being silently dropped.
4. docs/demos.md now documents --chat-mock's hidden "install the sdk" phrase that triggers the approval-modal demo path.
5. The footer's "Esc back out" chip now also checks active_overlay_at_root(), so it doesn't over-promise while a sub-popup/job console is open inside a manager; simplified the associated test per your suggestion.

Verified: cargo test -p rocm-dash-tui --lib (666 passed), cargo clippy -p rocm-dash-tui --all-targets -- -D warnings (clean), cargo test --workspace --exclude e2e-cucumber (all green, matching your baseline), and cargo xtask e2e -- -n dash-05/-n dash-11 both pass.

[siloteemu]

🔴 Automated review · pr-review-watcher · 251d5ae

Summary

Unifies both help screens onto one scrollable popup renderer, clamps help scroll to the measured content length, dims modal backdrops, defaults tool approvals to Deny, and adds e2e coverage. Needs work. Verified: finding 1 is FIXED (the renderer writes help_max_scroll back for both help modals, scroll_help clamps against it, and the new test genuinely fails on revert — i16::MAX would land on 32767, not 10); finding 2 is PARTIALLY FIXED — the doc comment and unit-test comment are corrected and I confirmed their factual claims against the event loop, and should_pane_back_out/active_overlay_at_root bodies are byte-identical to d54e7a72, but the "no production logic" claim does not hold for the commit as a whole (it also newly gates the footer's Esc chip on active_overlay_at_root(), which is why that function was widened to pub(crate)), and the scenario half of the finding was left untouched and still passes on the base; on the revert question, every other added test discriminates correctly except that scenario, and the renderer write-back itself is unit-untested; the CI failure is real and I reproduced it locally — cargo fmt --all --check fails on the new clamp test. Blocking: 3 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• crates/rocm-dash-tui/src/app/mod.rs:5540 — the new clamp test's assert_eq!(s.help_scroll, 9, "…") line is not rustfmt-clean; rustfmt wants it split across three lines. I ran cargo fmt --all --check locally and this is the only diff in the whole workspace, so it is a deterministic, reproducible failure — not a flake. I am inferring this is the one genuine CI failure (the lint/hygiene job runs cargo fmt --all --check, and the local pre-commit config has the same hook); I cannot see which check id failed, so I cannot confirm the attribution — but the formatting breakage is confirmed regardless and must be fixed. Fix: run cargo fmt --all.

• crates/rocm-dash-tui/src/ui/mod.rs:438 — new production regression introduced by this head commit: the footer Esc chip changed from state.has_open_overlay() to state.has_open_overlay() && state.active_overlay_at_root(). When a manager has a sub-popup, approval, or running job console open, the condition is now false and the chain falls through to the final else, emitting Seg::Key("Esc", Some(KeyAction::OpenMenu)) — a clickable chip labelled "menu". But Esc there never reaches the generic handler: app/mod.rs:2018's should_pane_back_out arm declines, and the per-overlay arms (app/mod.rs:2045–2168) take the key and cancel the sub-layer. So the chip both mislabels Esc and dispatches a genuinely different action on click, violating the invariant stated verbatim in the comment four lines above it ("the chip never advertises menu while a click on it would actually do something else"). Before this commit the chip read "back out" with None (no click target), so this is strictly worse. It is also entirely untested — the only footer test covers the no-overlay case. Fix: restore has_open_overlay() alone for this branch, or add a distinct non-clickable "Esc cancel" arm for the not-at-root case; either way add a draw_footer test with a manager sub-popup open.

• tests/e2e-cucumber/features/dash.feature:129-136 — finding 2's scenario half was not addressed; the head commit does not touch dash.feature. The scenario name and its step "the services manager is closed without opening the menu" still frame the change as guarding the menu-fallthrough bug that the PR's own corrected doc comment now states cannot occur. Would it pass if the production change were reverted? Yes — restoring the tab guard makes should_pane_back_out return false on that tab, but execution falls through to the state.services.is_some() arm and services_manager.rs:162 (Esc | 'q' => *services = None) closes it anyway; the observable screen state is identical. That contradicts AGENTS.md §3 ("test fails before fix and passes after fix"). Fix: reword to the behaviour actually being characterised (e.g. "Escape closes a manager overlay on any tab") and drop the "instead of / without opening the menu" framing, or note in-file that it is a characterisation test rather than a regression test — the discriminating coverage already exists in back_out_requires_an_open_manager_on_any_tab (app/mod.rs:3739), which does fail on revert.

Non-blocking

• app/mod.rs:5533 — the clamp test sets help_max_scroll by hand, so reverting only the renderer write-back (ui/mod.rs:126/143) leaves it green; that wiring is exercised only by the e2e addition. A draw-level unit test asserting help_max_scroll becomes non-zero after rendering Help would close it cheaply.
• The two-column → one-column decline is reasonable: the PR now ships scrolling keys on both help screens, a wrapped-row-accurate clamp so the last page is reachable, and a scroll reset on open — the 80x24 content is navigable rather than silently clipped, which was the original defect. Caveat below.
• ui/modal.rs:77 draw_scrollable_lines renders no scrollbar or "more below" affordance, and the footer shows only "Esc close" while a modal is open — on 80x24 a user sees a truncated list with no signal that scrolling exists. Worth a small hint in the popup title.
• reset_help_scroll (app/mod.rs:1133) zeroes help_scroll but not help_max_scroll, so the bound is stale between an old Help and a newly-opened global help. Benign today only because draw precedes key handling each frame — resetting both would remove the ordering dependency.

[jussielo-amd]

Addressed in b370b3e (pushed on top of 251d5ae), replying to #358 (review):

Blocking #1 — rustfmt. Ran cargo fmt --all; the assert_eq! in the help-scroll clamp test is now formatted. cargo fmt --all --check is clean.

Blocking #2 — footer Esc chip regression. Confirmed: my previous fix changed the "back out" condition to has_open_overlay() && active_overlay_at_root() but left no fallback, so when an overlay was open but not at its root (sub-popup/picker/approval/job console), the chip fell through to a clickable Esc → menu even though Esc is actually consumed by that inner layer. Added a distinct has_open_overlay()-only branch that renders a non-clickable "Esc cancel" chip (action: None), plus a new unit test, footer_esc_chip_is_not_clickable_menu_when_overlay_has_a_sub_popup_open, that opens a ServeWizardState with active_job: Some(...) and asserts no rendered chip carries KeyAction::OpenMenu.

Blocking #3 — dash-11 test framing. Reworded the scenario to "Escape closes a manager overlay on any tab", dropped the "instead of opening the menu" framing and the without opening the menu step text, and added an in-file comment noting this is characterization coverage — the discriminating regression test for the tab-guard removal is the existing back_out_requires_an_open_manager_on_any_tab unit test, which does fail on revert. Re-ran the scenario against the built binary; still passes.

Non-blocking:

• Reset help_max_scroll alongside help_scroll in reset_help_scroll, removing the draw-before-key-handling ordering dependency.
• Left the two-column→one-column global-help layout and the modal's scrollbar/"more below" affordance as-is — both are intentional consequences of unifying onto the shared scrollable renderer, not regressions.

On the "merge conflict": there isn't one at the git level — git merge-tree against origin/main shows no conflict markers, and mergeable has stayed MERGEABLE throughout. The BLOCKED merge state was solely CHANGES_REQUESTED plus the failing prek (lint / hygiene) check (the rustfmt issue above), which is now fixed.

Full verification: cargo fmt --all --check clean, cargo clippy -p rocm-dash-tui --all-targets -- -D warnings clean, cargo test -p rocm-dash-tui --lib 667 passed, cargo test --workspace --exclude e2e-cucumber all green, and cargo xtask e2e -- -n dash-11 passes against the release binary.

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in help-scroll normalization and Observe popup e2e synchronization.

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

Review details

Suppressed comments (6)

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

• When the terminal is enlarged (or the help content changes via a tab-bar click), help_max_scroll can shrink below the existing help_scroll. Rendering clamps only the local paragraph offset, while this reducer continues from the stale offset, so pressing k/Up decrements it one row at a time while the screen remains pinned to the last page. Normalize the current value against help_max_scroll before applying the delta so the first Up works after a geometry/content change.

        let cur = i32::from(self.help_scroll);

crates/rocm-dash-tui/src/ui/mod.rs:447

• This branch labels every manager sub-layer's Esc action as cancel, but an active job console handles Esc as Closed/Dismissed (ui/job_console.rs:50-66); only Ctrl+C cancels the job. The new footer therefore misstates what Esc does while a console is open. Use close for state.has_active_console() and reserve cancel for pickers/approval dialogs.

        segs.push(Seg::Sep(" cancel  "));

crates/rocm-dash-tui/src/ui/modal.rs:118

• These rows are now shown in draw_help for every tab, including live/non-replay sessions, but the corresponding Replay* actions are no-ops when no replay is active. The previous help text explicitly marked each control as replay-only; removing that qualifier makes the unified help advertise unavailable controls. Restore the qualifier (or conditionally render this group when replay state exists).

        ("Space", "pause / resume"),
        ("+ / -", "speed up / slow down"),
        ("[ / ]", "jump ±10s"),
        ("{ / }", "jump ±60s"),

crates/rocm-dash-tui/src/ui/modal.rs:621

• This changes draw_global_help from a fixed layout to a scrollable user-facing overlay, but the new Gherkin coverage only opens the contextual Help modal (?) and never opens the menu's GlobalHelp path or verifies that its lower rows are reachable. The unit test for j/k/G only checks action mapping, so it does not cover the rendered behavior; add a dashboard scenario that opens Menu → Help, scrolls to the end, and asserts the final global-help group.

    let lines = help_group_lines(groups, theme);
    draw_scrollable_lines(f, popup, "Keyboard", lines, scroll, theme)

tests/e2e-cucumber/tests/e2e/dash_steps.rs:222

• open_dashboard_demo only spawns the process, so this toggle can be written before the event loop is ready. TuiSession::send_until documents that a bare send during startup may be consumed and lost; because t toggles the picker it cannot safely be retried. Wait for a stable home marker (for example, Updates) before sending this key.

    tui.send("t")
        .unwrap_or_else(|e| panic!("failed to open the theme picker: {e}"));

tests/e2e-cucumber/tests/e2e/dash_steps.rs:545

• This step is also used by dash-15 for the theme picker, but the failure text always says the instance-detail popup was not dimmed. A theme-picker regression would therefore produce a misleading diagnostic; make the message generic to the popup under test.

        "the screen behind the instance detail popup was not dimmed:\n{}",
        tui.screen_text()

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

Withdrawing this change request: all three blocking points are resolved at e8246c8. Formatting is clean workspace-wide, the footer Esc chip can no longer fall through to a clickable menu action from any overlay sub-state, and the scenario that was framed as a regression guard has been renamed and annotated to say plainly that it is characterization coverage — which is exactly the remedy asked for. We were not wrong on any of the three; all reproduced and all are now addressed. Full re-review posted as a comment.

[siloteemu]

🔴 Automated review · pr-review-watcher · e8246c8

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

Unifies both help screens onto one scrollable popup renderer, clamps help scroll to the measured wrapped-row count, dims modal backdrops, defaults tool approvals to Deny, and adds e2e coverage; this head addresses all three of our prior blocking points. No blocking findings. Verified: cargo fmt --all --check now exits 0 across the workspace (the sole prior formatting breakage is gone); I traced the new footer Esc ladder arm-by-arm and confirmed it captures every "overlay open but not at its root layer" combination — not just the one the test constructs — so no not-at-root state can reach the clickable OpenMenu fallback any more; the new footer_esc_chip_is_not_clickable_menu_when_overlay_has_a_sub_popup_open test genuinely fails on revert (with modal == Modal::None and active_tab == Home, deleting the arm routes to the else that pushes Seg::Key("Esc", Some(KeyAction::OpenMenu)), and FooterChip.action is a plain KeyAction, so the loop assertion fires) and is not vacuous through width truncation at 90 columns; the dash-11 comment's factual claims both hold (services_manager.rs:162 self-closes on root Esc, and back_out_requires_an_open_manager_on_any_tab is a real discriminator whose assertion flipped from assert!(!…) to assert!(…) in this PR); the tag and step renames leave zero dangling references and no cucumber ambiguity; "Instances · AI metrics" is rendered only on the populated path (draw_table early-returns to draw_empty, which titles the panel "Instances"), so the added wait really does prove the table populated; and docs/demos.md's "install the sdk" claim matches the mock wiring at app/mod.rs:1758-1761. CI state at this head was supplied as 26 success / 1 skipped / 0 failure / 0 pending. Blocking: 0 · Non-blocking: 5.

Prior objection status

• app/mod.rs:5540 — clamp test not rustfmt-clean (blocking): RESOLVED. The assertion is now split across three lines and cargo fmt --all --check exits 0 with no diff anywhere in the workspace. (I am inferring nothing about which named CI lane covered this; I confirmed it locally instead.)
• ui/mod.rs:438 — footer Esc chip fell through to a clickable OpenMenu when an overlay had a sub-layer open (blocking): RESOLVED. A new else if state.has_open_overlay() arm (ui/mod.rs:441-447) emits a non-clickable Seg::Key("Esc", None) + " cancel ". Because arm 2's conjunction and arm 3's predicate are over the same overlay fields, the pair is exhaustive over all open-overlay states, and the accompanying test discriminates on revert. Our prior objection was correct and is now fixed.
• dash.feature:129-136 — dash-11 framed as a regression test for a bug that cannot occur, passes on revert (blocking): RESOLVED. Scenario renamed to "Escape closes a manager overlay on any tab", tag renamed, the misleading step "the services manager is closed without opening the menu" reduced to "the services manager is closed", and an in-file comment now states plainly that this is characterization coverage and names the discriminating unit test. That is exactly the remedy we asked for; the scenario is still non-discriminating, but it is no longer claiming to be a regression guard, which satisfies the AGENTS.md §3 concern.
• Prior non-blocking "reset_help_scroll leaves help_max_scroll stale": also RESOLVED (app/mod.rs:1134), removing the draw-before-key-handling ordering dependency. Prior non-blocking items on the untested renderer write-back and the missing scroll affordance remain open (below).

We were not wrong on any of the three blocking points; all three reproduced and all three are now addressed.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-dash-tui/src/ui/mod.rs:447 — the new arm labels Esc "cancel" for every not-at-root state, but for a running job console Esc maps ConsoleOutcome::Closed to *mgr = None, closing the whole overlay — behaviourally identical to the root "back out" case, so the label is inaccurate for that one sub-state (non-clickable either way, so no functional bug).
• crates/rocm-dash-tui/src/app/mod.rs:1134 — standing focus (a): the new self.help_max_scroll = 0; line has no test; slash_help_resets_stale_scroll_offset asserts only help_scroll == 0, so reverting that line leaves the suite green. One extra assertion in the existing test would close it.
• crates/rocm-dash-tui/src/ui/mod.rs:613 — the new footer test can only ever prove absence of an OpenMenu chip, since None-action segs emit no FooterChip at all; asserting the literal " cancel " in term.backend().buffer() would also pin the label and separator.
• tests/e2e-cucumber/tests/e2e/dash_steps.rs:534 — the retained !screen.contains("Options") && !screen.contains("Quit") check reads like menu-fallthrough regression coverage, but on revert execution never reaches the OpenMenu arm (the services arm self-closes first), so it is defensive only; this re-confused a reader during this review even with the new comment present, so a one-line "defensive: the menu can't open here either way" note next to the assertion would stop it recurring.
• crates/rocm-dash-tui/src/ui/mod.rs:127/144 — still no draw-level test that help_max_scroll becomes non-zero after rendering Help, so the renderer write-back wiring is covered only by the e2e addition; and ui/modal.rs:77 still renders no scrollbar or "more below" affordance, leaving an 80x24 user with no signal that scrolling exists.