[jussielo-amd]

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. — no rows reference issue [Feature]: Stop serving a model #145.
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. — N/A, this extends existing app/ modules; the live registry-read seam got its own file (crates/rocm-dash-tui/src/registry_probe.rs).
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording, per AGENTS.md §3. — see "Test plan".

Fixes #145

Summary

Quitting the dashboard (q, the Esc menu's Quit row, /quit//exit in chat — including rocm chat, which is the same dashboard with the Chat tab focused), or the pre-dashboard launcher (bare rocm), used to exit immediately regardless of whether a model was still being served, leaving it running in the background with no warning and no in-app way to stop it.

When at least one managed instance is actively serving, quitting now shows a confirm prompt first, naming the serving model(s) and — for any instance this host's own registry manages — the real rocm services stop <id> --yes command to stop it (a Docker- or Lemonade-discovered instance from an external daemon gets the warning but not that command, since it genuinely can't be reached that way). Declining returns to where you were; approving quits anyway, same as before — the model keeps running either way, it just won't happen by surprise. Neither the title nor the trailer assert present-tense that a model is serving: both say "may still be" rather than "is"/"keeps", because nothing on this path — not the launcher's raw on-disk read, and not the dashboard's own daemon-sourced instance snapshot either — is actually re-verified against the live process. The daemon's registry discovery only ever promotes a record forward on a successful scrape and never demotes one on a failed scrape, and nothing on the rocm dash path re-runs the CLI's own liveness reconcile (refresh_managed_service_runtime_liveness, wired only into load_managed_services), so a model that crashed since its last successful scrape can still read as "serving" here. Hedging the claim, rather than adding a second, duplicate liveness check in rocm-dash-tui, matches the deliberate call already made on ManagedRegistryProbe::snapshot's own doc comment: read the registry's belief, not a re-derived verdict that could drift from the CLI's own.

From the dashboard's own copy of the prompt, pressing s instead jumps straight to the Services overlay, pre-focused on the serving model, so it can be stopped there through the overlay's own approval+job flow without quitting at all. The launcher's copy of the prompt doesn't offer this: it has no live Services overlay to jump to, so it shows the warning and the stop command but not the s hint, rather than advertising a key it can't honor.

Whether anything is "still serving" is answered by a live, on-demand read of the managed-service registry (ManagedRegistryProbe), not a snapshot frozen at launch. This closes two real gaps: a model started mid-session through the dashboard's own Serve wizard is picked up the next time the gate is consulted (it used to get the warning but never the stop command or the s hint until the session restarted), and a host whose dashboard client never connects to a daemon (Windows) still gets a correct answer for the whole session instead of only a launch-time guess. A --demo/--replay session never shows the prompt — the data it displays is synthesized, so nothing is actually running to warn about.

A dashboard or chat session opened from inside the launcher no longer asks twice: its own q/Esc (or /quit in chat) now exits straight back to the launcher menu (since that's "back to menu", not "quit"), and it's the launcher's own gate — re-entered when control returns to it — that actually governs process exit and shows the one prompt that matters. Both of the launcher's hosted escalations — opening the full dashboard, and opening Chat — get this treatment identically, derived from the route rather than two independent true literals at the call sites.

The prompt reuses the existing approval seam (ui/approval.rs) rather than introducing new overlay UI, and is wired at the same priority as the dashboard's other screen-owning overlay (the chat tool-call approval): it owns every keypress and mouse gesture ahead of any tab or manager, a chat tool call that surfaces for approval while the prompt is open is discarded rather than silently taking over input, and the footer's Esc chip reflects it instead of falling through to a generic action. The chat tool-call approval modal shares the same "owns the screen" gate, so it also swallows tab-bar and footer-chip clicks the same way the quit-confirm prompt does. The bare-rocm launcher's in-place focused flows (Set up/Serve/Diagnose, ui::draw_focused) render the prompt too, defensively — no focused flow opens it today, but the renderer has an arm for it regardless, so a future one that did would not be left showing an invisible, key-eating prompt.

The gate reads the registry once per decision rather than once per question: the title, the body, and the s hint all share a single snapshot, so a registry write landing between two reads can no longer make them disagree — it used to be possible for s to silently no-op if a service stopped between the "is anything stoppable" check and the row lookup. The prompt's serving-instance listing sorts by container_id (the same key the s key's own row lookup sorts by), so the listing is stable across renders rather than ordered by whatever a HashMap happens to yield — not that s focuses the Nth model named: it always focuses the first managed, still-serving row in that row lookup's own order, which can include an unmanaged (Docker- or Lemonade-discovered) entry the listing also shows.

Ctrl-C is not gated — it already exits unconditionally above every overlay, mirroring an external SIGINT, and stays that way here, except from a running job's console, where it keeps its existing meaning of "cancel this job" instead.

Scope

The launcher front door (bare rocm, before entering the dashboard) is a second, independent quit entry point from the dashboard's own — same underlying bug, same fix, same shared request_quit gate (app/actions.rs). It renders the identical prompt (ui::launcher's own quit-confirm arm calls the same ui::quit_confirm_request) from its own, simpler key loop, with the s-to-manage jump out of scope there for the reason above. Chat opened from the launcher is the second hosted route through that same gate (run_chat's own launcher_hosted), alongside the full dashboard.

The Esc-menu Quit row now has its own end-to-end coverage (dash-34); the launcher's own Esc still relies on the same shared unit-level coverage as its q, since it routes straight through request_quit with no menu of its own to navigate.

Kept the quit-confirm logic in app/mod.rs/ui/mod.rs rather than extracting dedicated quit_confirm.rs files: the AppState methods involved are entangled with the general overlay system already living in app/mod.rs (has_open_overlay/close_overlays), and the ui-side functions share their test module and helpers with the rest of ui/mod.rs. A clean extraction would mean relocating that shared test infrastructure too, which felt like a separate, lower-risk-on-its-own follow-up rather than something to bundle into this round.

Accepted tradeoff: a launcher_hosted session's own q/Esc exits straight back to the launcher without consulting the dashboard's own gate at all (AppState::request_quit returns immediately when launcher_hosted is set) — it's the launcher's own re-entered gate that actually decides whether to warn and whether to quit. That gate never offers the s-to-Services jump, so a user who quits a dashboard (or chat) session that was opened from the launcher loses that shortcut even though the full dashboard was open moments earlier, same as the launcher's own front-door prompt. Correctness here rests on an invariant held by the launcher's key loop rather than enforced by a type: every hosted route must actually return control to that loop rather than exiting on its own. A structured "how the session ended" return value threaded back through run/run_chat would make that invariant explicit instead of implicit; not taken on here, to avoid widening this change's already broad surface for a design question that doesn't block the fix this PR makes.

Accepted tradeoff: the gate only counts actively ready/running managed instances (LiveManagedRegistryProbe::snapshot's is_serving() filter), not starting/recovering ones. rocm serve --managed detaches its supervisor, so quitting while a model is still coming up (e.g. straight out of the Serve wizard) leaves it loading unattended with no warning — the same surprise #145 is about, and arguably the likeliest moment to hit it. Kept narrow deliberately for this round: widening it means deciding what the prompt should say about a model that isn't being served yet (quit_confirm_body_with only ever says "serving"), and the existing tests (launcher_state_does_not_warn_on_a_starting_only_registry, resolved_args_reports_no_live_managed_service_when_none_are_ready_or_running) pin the current exclusion on purpose. A follow-up, not taken on here.

Accepted tradeoff: a stop already in flight still reads as serving. stop_internal_managed_service_confirmed_with writes stop_requested_unix_ms and keeps the record ready/running for as long as the stop takes (around 40s by its own comment) before flipping the status; neither rocm-dash-daemon's ServiceRecord nor LiveManagedRegistryProbe look at that field. So s → stop → q inside that window still asks to stop something already stopping. The hedged wording ("may still be serving") keeps the claim technically true, but the remedy it prints is already underway. Not closed here — it needs either a new field on ServiceRecord or a different way to read in-flight state, which is more surface than this round's fix warrants.

Test plan

• cargo test -p rocm-dash-tui, cargo test -p rocm, cargo clippy -p rocm-dash-tui -p rocm --all-targets -- -D warnings, cargo test -p e2e-cucumber --test feature_naming all pass locally.
• Unit tests cover: the q key, the Esc-menu Quit row, and the /quit chat command all open the prompt, and all still exit immediately with nothing serving (the regression guard for the common case); a launcher-hosted session (dashboard or chat) exits unconditionally even while a model serves; the prompt's y/n/Esc key handling, driven through the same verdict-resolution path the event loop and launcher actually call; a chat tool-call approval is discarded rather than opened while the prompt is pending; mouse drag/scrollbar-track/wheel/tab-bar/footer-chip input is swallowed while the prompt (or a chat approval) owns the screen; the startup-race fallback and the live registry read are each exercised directly, including a non-serving (Starting/Stopped/Error/Unknown) instance correctly not tripping the gate on every predicate that filters on it (has_live_instance, has_stoppable_managed_instance_with, quit_confirm_open_services_key, and the launcher's own static probe); the prompt's title and body are derived from the same "is anything still serving" decision, so the two can't print a contradiction, and both stay hedged ("may still be serving"/"may still be running") once a real daemon snapshot has landed, not only during the pre-snapshot fallback; the serving-instance listing is asserted sorted by container_id, not insertion/HashMap order; the stop command names the real service id and only appears for instances the registry actually manages, rendered through the real ui::draw entry point for both the dashboard ("Press s" hint shown) and the launcher (hint absent); ui::draw_focused renders the prompt if opened, even though nothing opens it there today; a --demo/--replay session never trips the gate even with a serving-looking instance in it, while --chat-mock (which only mocks the chat completion backend, not instance state) deliberately still does; and s opens the Services overlay focused on a genuinely stoppable managed row, verified by mutation.
• tests/e2e-cucumber/features/dash.feature: dash-26 (dashboard) and dash-27 (launcher) drive the prompt through a real pty — open, assert the real stop command text, decline, confirm — and assert via rocm services list --json that the model's record is actually ready/running afterward. dash-26 additionally asserts the "Press s" hint, presses s, and asserts the Services overlay opens showing the fixture's own service id; dash-27 (the launcher) has no s hint to assert, per the launcher's own copy of the prompt omitting it. dash-28 goes one step further than the string match: it runs the actual printed rocm services stop <id> --yes command, asserts via the same --json read that the service is no longer ready/running afterward, and asserts the real disposable process it pointed the record at has actually exited — not only that the registry record was rewritten (AGENTS.md §3 — a remediation command's claim is proven against the resulting state, not just its text). dash-29 covers /quit in chat. dash-30 covers a dashboard opened from the launcher: only one prompt appears (the launcher's), and it's the one that actually governs exit. dash-31 covers the same thing for Chat opened from the launcher — the other hosted route, exercised through its own /quit slash command rather than a bare q. dash-32 drives Ctrl-C through the real pty on both sides of the quit-confirm gate — before q has ever been pressed, and again after it has opened the prompt — and asserts exit code 130 either way, since production routes Ctrl-C through one match arm ranked above both the approval modal and the quit-confirm prompt. dash-33 covers the negative case directly, bypassing the shared quit_and_wait test driver (whose own retry loop answers "y" to any quit-confirm prompt it finds, which would otherwise paper over a regression that opened one): with nothing serving, q must exit on the very first keystroke, with the prompt never appearing at all. dash-34 covers the Esc-menu Quit row directly (AGENTS.md §3): Esc opens the menu, navigating to and activating Quit asks the same prompt the other entry points do — previously relying on unit coverage alone, per the "Scope" section above before this round.
• A direct proof that the injected registry probe re-reads on every call rather than caching its first read (every other registry-backed test here writes its record(s) before building ResolvedArgs and never mutates afterward, so a caching regression would otherwise pass all of them); an Esc-menu Quit-row test with nothing serving (the existing one only ever seeds a Running instance); a mixed serving/non-serving-row test for the s key, proving its position lookup's is_serving() filter actually matters rather than the short-circuit above it doing all the work; and Esc/q/d-while-pending coverage for the launcher's own copy of the prompt.
• Several existing dash.feature/chat.feature scenarios open the dashboard (or rocm chat) with a model already serving, so the shared quit_and_wait test driver needed a matching fix for both the dash and chat quit gestures: once the prompt is open, q/Q cancel it (same as any approval-style modal), so blindly resending the normal quit gesture would have flipped it open/closed forever (dash), or typed the remainder of /quit into chat as a message (chat), instead of ever confirming it. Fixed once in the driver rather than touching each scenario.
• Not verified: the self-hosted GPU/Windows/WSL CI lanes require this repo's own runners and are exercised there, not in this environment — except the WSL lane's hardware-telemetry scenario, which fails in this sandbox for lack of a real GPU (unrelated environment gap, not this change). More specifically: every dash.feature scenario above (dash-26–dash-34) is tagged @requires-os:linux and runs only on Linux lanes — none of them exercise the Windows-specific quit-prompt path (quit_confirm_body_with's total == 0 branch with no daemon snapshot: ids listed without model names, the hedged trailer, no s hint). That path is unit-tested only; the pty harness driving dash.feature has no native-Windows-console equivalent, so closing this gap would need a different harness, not just an added scenario.

[jussielo-amd]

review-coverage · cde16ef

Full-scope high-effort review (fresh Claude Opus context, able to fan out) ran three rounds against this change: round 1 (8 findings, all fixed), round 2 re-reviewed the fix and found 4 more items (1 real bug, 1 stale doc reference, 1 test gap, 1 duplication tradeoff — all fixed/resolved), round 3 re-reviewed that and found 1 non-blocking mechanical duplication (fixed) with 0 blocking findings otherwise. The final mechanical fix (extracting a shared quit_confirm_request builder) was verified via full rebuild + clippy + the 837-test unit suite + the full dash.feature e2e suite (25/25 scenarios, including new e2e coverage for the launcher's own quit-confirm prompt) rather than a fourth agent review round, since it's a behavior-preserving extraction the prior round's own reviewer specified verbatim.

[Copilot]

🟡 Changes recommended

Unresolved quit-confirmation behavior and regression-coverage gaps need correction.

4 open findings

• Prevent exit before initial service discovery completes · New
• Block tab clicks while quit confirmation is open · New
• Wait for quit prompt to disappear before validating refusal · New
• Verify service is running, not merely present · New

What changed in this PR

Adds quit confirmation to the dashboard, chat, and launcher when models are serving, while leaving those services running after exit.

Changes:

• Shares quit gating and approval-style prompt handling.
• Integrates confirmation with input routing and footer state.
• Adds regression coverage, TUI-driver support, and documentation.

Validation was limited to source inspection; tests were not run.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​tui_driver.rs	Handles confirmation during test teardown.
tests/​e2e-cucumber/​tests/​e2e/​dash_steps.rs	Adds confirmation and service-check steps.
tests/​e2e-cucumber/​features/​dash.feature	Adds dashboard and launcher scenarios.
README.md	Documents confirmation behavior.
crates/​rocm-dash-tui/​src/​ui/​tabs/​pane.rs	Reuses overflow-list formatting.
crates/​rocm-dash-tui/​src/​ui/​tabs/​instances.rs	Updates test state initialization.
crates/​rocm-dash-tui/​src/​ui/​mod.rs	Renders confirmation and updates footer handling.
crates/​rocm-dash-tui/​src/​ui/​launcher.rs	Adds launcher confirmation handling and tests.
crates/​rocm-dash-tui/​src/​ui/​format.rs	Adds shared overflow formatting.
crates/​rocm-dash-tui/​src/​app/​slash.rs	Gates chat quit commands.
crates/​rocm-dash-tui/​src/​app/​scrollbar.rs	Suppresses mouse interactions during confirmation.
crates/​rocm-dash-tui/​src/​app/​mod.rs	Adds confirmation state and resolution methods.
crates/​rocm-dash-tui/​src/​app/​event_loop.rs	Routes keys to pending confirmation.
crates/​rocm-dash-tui/​src/​app/​actions.rs	Centralizes quit gating and adds tests.

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[jussielo-amd]

review-coverage · 77414fb

Three full-scope closing reviews (fresh Claude Opus 4 context each, fanning out, different model from the fixer) run back-to-back against Copilot's automated review and its own fallout, closing out all four of its findings plus what each pass itself turned up:

• Round 1 (c662c7d head): reviewed the initial fix for all four Copilot findings (startup-discovery race, tab-bar/footer-chip modal bypass, two e2e assertion gaps). Found a stale PR description (launcher scope + test plan out of date) and a self-contradictory quit-confirm prompt body (claimed "nothing is being served" next to "it keeps running") for the startup-fallback trigger. Both fixed; description corrected live via gh pr edit.
• Round 2 (c662c7d → 77414fb): verified round 1's two fixes directly against the live PR body and the diff, found round 1's contradiction fix only covered one of two triggers — the same message pair was still reachable when a model stopped on its own while the prompt sat open. Fixed, with a strengthened test asserting the trailer's absence. Also added a docs/manual-testing.md step for the new quit flow (AGENTS.md §5).
• Round 3 (77414fb, final): zero blocking or non-blocking findings. Independently re-verified both contradiction fixes by enumerating all 8 state combinations and running direct mutation tests (breaking each fix in isolation, confirming each is caught by exactly one of the two covering tests, restoring byte-identical). Two items left for the author's own call, not defects: whether "warn + surface the stop command" is the intended resolution of [Feature]: Stop serving a model #145 (vs. making quit actually stop the model), and a minor Option-vs-enum modal-state note explicitly marked out of scope.

All 25 CI checks green on 77414fb (Skill checks/skillscope SKIPPED, advisory only). The four original Copilot inline comments have been replied to individually with the specific fixing commit and resolved.

[rominf]

🔴 Automated review · pr-review-watcher · 77414fb

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

First automated round on this PR: every finding below is new.

Review — needs work

Full review of the whole change.
The description says "Fixes #145", but #145 asks for a way to stop a served model. This PR adds a warning before quitting, not a way to stop (see Blocking).

Blocking

• --demo/--replay now open the quit prompt for simulated instances, and its text is false there — crates/rocm-dash-tui/src/app/mod.rs:630, crates/rocm-dash-tui/src/app/actions.rs:68, crates/rocm-dash-tui/src/ui/mod.rs:235-236

  • Evidence:
    • has_live_instance never checks state.simulated. That field exists "so simulated data can never be presented as live".
    • Demo snapshots carry InstanceStatus::Running (rocm-dash-daemon/src/demo.rs:615). The PR's own driver comment confirms the effect (tui_driver.rs:674: "Demo data … opens the quit-confirm prompt").
    • So on every q in demo mode the prompt says "It keeps running in the background after you quit. Stop it first: rocm services stop <id> --yes". Nothing is running.
    • This breaks the AGENTS.md §3 rule that a printed claim must be true of the behaviour. It also contradicts the description's own scope: "when at least one managed instance is actively serving".
  • Confidence 85 · logic · Fix: return false from the gate when state.simulated is set, and add a unit test fed a demo snapshot.

• The stop advice in the prompt can't be followed, and nothing checks it — crates/rocm-dash-tui/src/ui/mod.rs:228,236

  • Evidence:
    • The body lists only model_name. It then prints rocm services stop <id> --yes, but <id> is the service id (container_id), which the prompt never shows.
    • Nothing tests that the command clears the condition. AGENTS.md §3 requires that for remediation advice that names a command: "the named command must exist, accept those flags, and actually clear the condition that printed it".
    • instances can also hold Docker- or Lemonade-discovered entries when connected to an external daemon. rocm services stop cannot stop those.
  • Confidence 85 · logic · Fix:
    • Print each service's id next to its model.
    • Print the stop line only for registry-managed instances.
    • Add a test that runs the printed command, or a unit-level equivalent, and shows that the next q exits without a prompt.

• New behaviour that no test reaches — crates/rocm-dash-tui/src/app/event_loop.rs:810, crates/rocm-dash-tui/src/ui/mod.rs:227-232

  • Evidence: I applied both mutations together on a clean tree and ran cargo test -p rocm-dash-tui --lib. The result was 840 passed and 3 failed. The 3 failures are the pre-existing agent::clients::tests::*, which workers saw fail unmutated as well. The two mutations were:
    1. Replace the args.startup_has_live_service → state hand-off with a no-op.
    2. Remove the • {model} bullet push from the prompt body.
  • So the startup-race fix itself is untested. Each half is tested (dash.rs produces the flag, has_live_instance consumes it), but not the wiring between them.
  • dash-25 waits for "the managed model is displayed" before pressing q, so the first snapshot has already retired the fallback by then. The model listing and the "…and N more" overflow (worker mutation: cap 5→500 stays green) are likewise never asserted.
  • Confidence 100 · mechanical · Fix:
    • Move the args→state seeding into a helper and test it, as apply_startup_focus already is.
    • Add a body test with 6 serving instances that asserts 5 bullets plus "…and 1 more".

• declining_quit_from_the_esc_menu_returns_to_the_dashboard_not_the_menu does not test declining — crates/rocm-dash-tui/src/app/actions.rs:901

  • Evidence:
    • The test calls on_quit_confirm_key('n'), then clears the prompt itself with s.quit_confirm = None. It never goes through resolve_quit_confirm_key, which is what both event loops call.
    • I mutated the Deny/Cancel arm so it no longer clears quit_confirm. This test stayed green (25 passed, 1 failed). Only launcher_quit_confirm_approve_exits_deny_and_cancel_stay failed.
    • Its modal == None assertion repeats the previous test.
    • quit_confirm_key_approves_denies_and_cancels likewise only exercises the on_quit_confirm_key passthrough, never the verdict's side effect.
  • Confidence 100 · mechanical · Fix: drive both tests through resolve_quit_confirm_key and assert Some(false) and quit_confirm.is_none().

• The description claims to close #145, but this change doesn't close it — PR description, "Fixes #145"

  • Evidence:
    • The issue reads: "After serving a model, the process keeps running when I exit rocm. I don't see any obvious way to stop the model serving service."
    • This change warns before quitting and leaves the model running. It adds no stop action, and the hint it prints lacks the id (see above).
  • Confidence 70 · architectural · Fix: either change "Fixes" to "Refs", or finish the work by offering a stop option from the prompt and making the hint usable.

Non-blocking

• Leaving a dashboard opened from bare rocm asks "quit anyway?" and then asks again at the launcher — apps/rocm/src/dash.rs:635, crates/rocm-dash-tui/src/app/actions.rs:68

  • The launcher loop re-enters run_launcher after run(None, ..) returns. So d → q → y lands on the launcher, where q prompts a second time.
  • The first prompt says "quit" and "after you quit", but the user is only going back to the menu.
  • dash-25 and dash-26 test rocm dash and the bare launcher separately, never this combined path.
  • Confidence 85 · logic · Fix: pass a "returns to launcher" flag through ResolvedArgs, and either skip the gate or reword the prompt in that case.

• Native Windows: the dashboard gate reflects only the state at launch — crates/rocm-dash-tui/src/client.rs:181-187, crates/rocm-dash-tui/src/app/mod.rs:630

  • The Windows connect_and_run always errors, so has_received_snapshot never flips.
  • A model started in-session (serve wizard) quits with no prompt. One stopped in-session still prompts.
  • The README's "If a model is still being served when you press q, it asks first" doesn't hold there. Both scenarios are @requires-os:linux.
  • Confidence 85 · logic · Fix: re-read the registry at quit time on that platform (a seam like tool_executor), or document the limitation.

• startup_has_live_service duplicates registry classification that is already public, and its doc comment says otherwise — apps/rocm/src/dash.rs:303-348

  • The doc justifies the copy with "instance_status_for_record … not pub, so re-derived here".
  • But registry::discover_managed_services is pub and already applies the port-0 filter and the status/phase mapping. The same file already uses it, in services_past_attempts over the same records and in launcher_serving_instances, the launcher half of this gate.
  • The dashboard gate and the launcher gate therefore classify "serving" through two copies. A worker's mutation dropping .trim().to_ascii_lowercase() kept all 9 resolved_args tests green.
  • Confidence 85 · architectural · Fix: discover_managed_services(records).svcs.iter().any(|s| s.status.is_serving()). Delete the hand-written match and the paragraph.

• The prompt title can contradict its body — crates/rocm-dash-tui/src/ui/mod.rs:185-186 against :211-213

  • The title is always "Still serving — quit anyway?". The total == 0 && !startup_fallback branch returns "Nothing is being served anymore." under it.
  • The code's own comment says that state is reachable: the model stops while the prompt is open.
  • launcher_renders_the_quit_confirm_prompt_when_open renders exactly this state and only checks for "Still serving".
  • Confidence 85 · logic · Fix: derive the title from the same decision as the body, or close the prompt when has_live_instance() turns false. Assert title and body together.

• quit_and_wait now answers y to any quit prompt it sees, in every scenario — tests/e2e-cucumber/tests/e2e/tui_driver.rs:688

  • A regression that opened the prompt with nothing serving would stay green across all dash/chat scenarios.
  • "With nothing being served, q exits immediately with no prompt" is user-observable, but only unit tests cover it.
  • The confirm half of dash-25/26 is also hidden inside the generic "the user quits …" step, so the Gherkin never shows it.
  • Confidence 85 · mechanical · Fix: add an explicit When the user confirms the quit prompt step, and have quit_and_wait fail if a prompt appears that the scenario did not opt into.

• Docs claims that don't match the code

  • docs/manual-testing.md:316: declining "returns to exactly where you were". open_quit_confirm deliberately closes any modal and overlay, and tests pin that (actions.rs:839, :901). Say "returns to the dashboard".
  • README.md:165-166: "Ctrl-C is the one gesture that always quits immediately, even then". Two sentences earlier, the README says Ctrl-C cancels a running job instead of quitting. "The same as today" will also go stale once merged.
  • Confidence 85 · mechanical · Fix: reword both.

• Comments that are stale or point at reasoning that isn't there

  • app/scrollbar.rs:62-64 says the gate lives in "scrollbar_hit's own blocks_body_absolutely() check". The check is actually the Down branch's early return.
  • ui/mod.rs:169-172 points at "quit_confirm's doc comment on AppState" for the claim that the approval and quit-confirm modals are never open together. That doc doesn't say it. The guarantee comes from the open_approval guard.
  • app/mod.rs:270-273 lists only "q, or /quit//exit". It omits the Esc-menu Quit row and the launcher.
  • apps/rocm/src/dash.rs ready-test doc says its sibling "pairs running with other records". It writes one record.
  • dash_steps.rs:581-586 says the next assertion is the same "in both scenarios". It isn't the same in dash-26.
  • Review-round narration ("this round's other refactors", "a prior round's fix … missed this one", "Closing-review regression") won't mean anything after the squash-merge.
  • Confidence 85 · mechanical · Fix: correct the pointers and drop the narration.

Decisions for the author

• Enter confirms quitting by default — tradeoff
  • open_quit_confirm uses ApprovalChoice::default() (Approve). That fits "the user already asked to quit".
  • The other side: a prompt that exists to stop an accidental quit can be passed by a reflexive Enter.
  • No scenario presses Enter here. #358 required a scenario for the chat approval's default-plus-Enter outcome.
• Tab-bar and footer-chip clicks are now swallowed during the chat approval as well — tradeoff
  • blocks_body_absolutely() reverses #358's documented "Tab-bar and footer-chip clicks above still work" for the approval modal. A new test pins the new behaviour.
  • This is consistent with the quit prompt, but it is a behaviour change to a different modal. The test plan mentions it in passing.
• Check liveness when the user quits rather than relying on snapshot or disk state — non-blocking-improvement
  • A registry read at quit time would remove both the startup-race flag machinery and the Windows gap in one stroke, at the cost of a synchronous file read on q.

Positive signals

• One request_quit gate serves every quit entry point: q, the Esc-menu Quit row, /quit//exit, the footer chip, and the launcher's q/Esc. No path bypasses it except the intentional Ctrl-C.
• resolve_quit_confirm_key is shared by the dashboard event loop and the launcher, so what a verdict does can't drift between them.
• It reuses ui/approval.rs (draw_approval/approval_key) instead of a new modal, as AGENTS.md asks.
• open_approval refuses while the quit prompt is open. Otherwise an approval arriving asynchronously would capture a y meant to confirm the quit and approve an unreviewed tool call.
• decline_quit_prompt waits for the prompt to leave the screen. So the next "model is displayed" check can't pass on the prompt's own text.
• managed_model_still_listed_as_running parses the JSON status field rather than substring-matching --all, which also lists stopped records.

Deployment notes

None

What this covered

• What was read: every file in the change, completely, prw-base (7f72556)…77414fb5. Also the callers and siblings:
  • rocm-dash-daemon/src/registry.rs and demo.rs
  • client.rs
  • the launcher hub loop in apps/rocm/src/dash.rs
  • ui/approval.rs
• How the work was split: 4 independent read-only workers. One for the state/gate logic plus dash.rs, one for the UI/mouse/launcher files, one for the e2e/docs files, and one for the source-reading passes (agent-instruction adherence, history, prior PRs #358 and others, code comments).
• Tests run:
  • CI rollup for 77414fb: all checks green. Skill checks were skipped, which is not relevant to this change.
  • A worker ran e2e dash-03/25/26: 3 of 3 passed. The full dash-* run passed 25 of 26; dash-24 needs a WSL GPU and is unrelated.
  • feature_naming: 4 of 4 passed.
• Mutation evidence: the workers' mutation runs overlapped in this shared checkout, and at least one worker reset others' files mid-run. Every mutation result cited above as decisive was therefore re-run by me, sequentially, on a clean tree.
• Did not run: Step 7's design questions were answered from the fan-out workers' evidence and my own reading, not by separate design workers. There was no hardware check on native Windows; that finding is traced from client.rs, not run.
• Not reconciled here: this report has not been checked against the PR's discussion or its earlier reviews.

[jussielo-amd]

Thanks for the thorough pass. Responding to each item (commits since this review: a33dfe6e, 1cafcd39, a67a936a).

Blocking — all five addressed

• --demo/--replay triggering the prompt with false text: fixed. has_live_instance now returns false unconditionally when state.simulated, with a test seeding both a Running instance and startup_has_live_service = true to prove the short-circuit order.
• Stop command unusable / wrong for non-registry instances: fixed. The body now prints the real container_id (no more <id> placeholder) and only offers the stop line for an instance whose id is in a new AppState::managed_service_ids set — sourced from the same registry read that already powers startup_has_live_service, via discover_managed_services. A Docker/Lemonade-discovered instance from an external daemon gets the warning but not a command it can't honor.
• Untested startup-race wiring: fixed. Extracted seed_startup_live_service (mirroring apply_startup_focus's own pattern) and added a direct test for the hand-off itself, not just each half in isolation.
• Decline tests bypassing the real verdict path: fixed. Both now go through resolve_quit_confirm_key and assert quit_confirm.is_none() after Deny — confirmed by mutation (reverting the Deny arm's clear makes both fail).
• Fixes #145 not actually closing it: addressed by finishing the work rather than downgrading to Refs. Pressing s on the dashboard's quit-confirm prompt now jumps to the Services overlay, pre-focused on the serving (managed) instance, so it can be stopped there through the overlay's own proven approval+job flow. The launcher's copy of the prompt doesn't offer this — it has no live Services overlay to jump to — and says so honestly rather than advertising a key it can't act on. Two fresh full-scope reviews on separate commits caught real gaps in this (the launcher's own managed_service_ids was never seeded, and the new behavior had no e2e coverage) — both fixed, and dash-25/dash-26 now exercise the stop command and the s action through a real pty.

Non-blocking

Fixed: the startup_has_live_service duplication (now delegates to discover_managed_services instead of hand-rolling the same classification), the title/body contradiction (both derived from one nothing_is_currently_being_served decision — including the hedged trailer in the startup-fallback branch, which a later self-review caught was still overclaiming), and the stale comments/docs you listed (scrollbar.rs, ui/mod.rs, app/mod.rs, dash.rs's test doc, dash_steps.rs, README's Ctrl-C section, docs/manual-testing.md's "returns to exactly where you were", and the review-round narration comments).

Not addressed, left open:

• The bare-rocm → dashboard → launcher double quit-confirm (d → q → y prompts again at the launcher) and its wording.
• The native-Windows launch-time-only gate.
• quit_and_wait's unconditional y and the missing explicit "confirms the quit prompt" Gherkin step.

Decisions for the author

• Enter still defaults to Approve — kept, by request.
• Tab-bar/footer-chip clicks swallowed during the chat-approval modal too — kept broadened, by request; now called out explicitly in the PR description rather than only in a test.
• Checking liveness at quit-time via a registry read instead of snapshot/flag machinery — not done. managed_service_ids remains a one-shot pre-TUI snapshot (same staleness startup_has_live_service already accepted), now documented in the PR description: a model served mid-session won't get the stop command or s hint until restart, though it still gets the plain warning correctly (that part uses the live snapshot). No false claim is printed either way.

Full verification (cargo test -p rocm-dash-tui/rocm, clippy -D warnings, cargo fmt --check) is clean locally; CI will run fresh against these commits.

[jussielo-amd]

review-coverage · a67a936

Two full-scope closing reviews (fresh Claude Opus context each, fanning out, different model from the fixer) ran back-to-back against this review's five blocking + six non-blocking findings and their own fallout: round 1 (commit a33dfe6e, against parent 77414fb5) found the fixes real but caught two new gaps of its own (launcher never got managed_service_ids, so its copy of the prompt never printed a stop command; the new s-to-stop action had no Gherkin coverage) plus two minor non-blocking items (a hedged-trailer inconsistency, a stale comment) — all fixed in round 2 (commit 1cafcd39). Round 2's own closing review, against a33dfe6e..1cafcd39, came back with no blocking findings; its two non-blocking notes (an e2e assertion that could pass on background text instead of the overlay's own content, and the launcher's scenario missing a stop-command assertion dash-25 already has) were fixed in commit a67a936a and verified locally (tests/clippy/fmt) but not re-reviewed by a fresh pass — that last round is self-verified only, not independently closing-reviewed.

Left open by design, not by oversight: the bare-rocm→dashboard→launcher double quit-confirm and its wording, the native-Windows launch-time-only gate, and quit_and_wait's unconditional y with no explicit "confirms the quit prompt" Gherkin step — none were in this review's scope to begin with.

[siloteemu]

🔴 Automated review · pr-review-watcher · 37ed114

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

Review — needs work

Full review of the whole change (prw-base...HEAD, 16 commits, 17 files).
Fixes #145 ("Stop serving a model"). The description's scope matches the diff. It discloses that managed_service_ids is a launch-time snapshot and that the approval modal now also swallows tab-bar and footer-chip clicks.

Blocking

• The startup_has_live_service hand-off into AppState is covered by no test, and the test that claims to cover the wiring does not — crates/rocm-dash-tui/src/app/event_loop.rs:826-830, :229-248, :2186-2206

  • seed_startup_live_service_wires_both_fields_onto_state calls the helper directly. Its comment says it guards against "the call site deleted, or the field names swapped", and the doc on seed_startup_live_service makes the same claim.
  • I ran two mutations: deleting the call site, and passing a hard-coded false in place of args.startup_has_live_service. cargo test -p rocm-dash-tui --lib stayed at 854 passed both times.
  • Scenario dash-25 does not reach the startup-race half either. A real snapshot has landed before q is pressed, so startup_has_live_service is retired by then. Only the managed_service_ids half has any end-to-end coverage, in the CI-run scenario.
  • The description's test plan presents the fallback as covered through this helper.
  • Confidence 100 · logic · Fix: move the whole ResolvedArgs to AppState seeding into one function that takes &ResolvedArgs and test that, so the call site cannot regress unseen. Narrow the comment either way.

• The is_serving() filter in has_live_instance is untested — crates/rocm-dash-tui/src/app/mod.rs:650

  • Replacing self.instances.values().any(|i| i.status.is_serving()) with !self.instances.is_empty() leaves all 854 unit tests green. No test seeds an instance whose status is Starting, Stopped, Error or Unknown. The only "no prompt" test uses an empty map.
  • This filter is the rule that decides when the prompt appears, and the PR introduces it.
  • The same gap applies to the is_serving() filters in has_stoppable_managed_instance (app/mod.rs:654-660) and quit_confirm_open_services_key. Every seeded instance in those tests is Running.
  • Confidence 100 · logic · Fix: add a test where a non-serving instance, including a managed one, neither opens the prompt nor offers or honours s.

• Two added tests cannot fail on the code they name — crates/rocm-dash-tui/src/ui/mod.rs:1056, :1213-1227

  • In quit_confirm_body_says_nothing_is_being_served_once_a_snapshot_confirms_it, the assertion !contains("Stop it first") checks a string the code never produces. The code writes lowercase "… — stop it first:" (ui/mod.rs:294). That state also returns early with a single line, and no managed ids are seeded. The assertion's message claims it guards against pointing at a stop command for a service that isn't running.
  • quit_confirm_open_services_key_ignores_other_keys_and_a_closed_prompt seeds no instances. Deleting the !quit_confirm_pending() || code != Char('s') guard entirely leaves the suite at 854 passed, because the later "no stoppable row" path returns false anyway. The test claims to cover both guards.
  • Confidence 100 (the guard test by mutation) and 85 (the string, by reading) · mechanical · Fix: seed a stoppable managed instance so each guard is the only thing preventing the jump, and assert the lowercase string in a state where the body prints a stop line.

• The two decisions offer_stop_action controls are untested at both call sites — crates/rocm-dash-tui/src/ui/mod.rs:179 (dashboard passes true), crates/rocm-dash-tui/src/ui/launcher.rs:163 (launcher passes false)

  • "The launcher must not advertise s" is stated in the description, the README and the manual-testing doc.
  • I flipped each argument alone, and both runs stayed at 854 passed.
  • launcher_renders_the_quit_confirm_prompt_when_open seeds an instance with an empty container_id and no managed ids, so it passes with either value. Nothing renders ui::draw with quit_confirm open.
  • Neither scenario asserts the "Press s to open Services" line, or its absence on the launcher.
  • Confidence 100 · logic · Fix: assert the hint is present in a rendered dashboard prompt and absent in a rendered launcher prompt, using a seeded managed instance.

• User-observable quit paths this PR changes have no Gherkin scenario, and the description does not argue why — crates/rocm-dash-tui/src/app/slash.rs:55, crates/rocm-dash-tui/src/app/actions.rs:279, tests/e2e-cucumber/features/dash.feature:333-371

  • AGENTS.md §3 says a unit test asserting an internal helper does not discharge a scenario for changed CLI behaviour.
  • dash-25 and dash-26 only press q. /quit and /exit in chat (the description calls out rocm chat) and the Esc-menu Quit row are separate code paths with no scenario. The same goes for the launcher's Esc and for Ctrl-C with a model served, which the README now documents.
  • chat-03 and chat-04 go through quit_and_wait, which sends y only if the prompt is on screen. With the slash.rs gate reverted, /quit exits at once and those scenarios still pass.
  • Confidence 85 · architectural · Fix: add a chat scenario that asserts the prompt on /quit and the model still running afterwards. For the other paths, add scenarios or say in the PR text, per path, why unit coverage is enough.

Non-blocking

• The staleness justification for managed_service_ids is wrong, and the user docs do not mention the limit — crates/rocm-dash-tui/src/app/types.rs:121-127, app/mod.rs:208, README.md:159-173, docs/manual-testing.md

  • The comment says the id only changes "when the user runs rocm serve again, which this process does not observe". The dashboard's own Serve wizard runs rocm serve --managed as a job in this process (ui/serve_wizard.rs:190-218). The embedded daemon re-reads the registry each tick (rocm-dash-daemon/src/runner.rs:649-653), so the instance appears in instances but never in managed_service_ids.
  • For a model started that way, the prompt shows no stop command and no s hint. The PR description discloses this, but the README says without qualification that s jumps to the serving model.
  • Which side governs: the code. Correct the comment and add the qualification to the README and manual-testing text. See the decision below for whether to refresh the set instead.
  • Confidence 85 · mechanical.

• A step comment claims the whitespace collapse reassembles a wrapped stop command, but a later commit made the opposite point — tests/e2e-cucumber/tests/e2e/dash_steps.rs:553-566, crates/rocm-dash-tui/src/ui/mod.rs:283-293

  • The step comment says collapsing whitespace "reassembles the original command losslessly". The ui/mod.rs comment (commit c30f00d) says a wrap lands the second half next to unrelated text outside the popup, which is why the command was moved onto its own line.
  • The ui/mod.rs comment also says the command "never needs to wrap". Real ids come from generate_service_id (rocm-core/src/lib.rs:4523) as {engine}-{slug up to 24 chars}-{13-digit ms}, so a typical command is well over 60 columns. Only the fixture id e2e-mock is short. I did not render a real-length id.
  • Which side governs: the layout facts in ui/mod.rs. Fix the step comment, and soften the production one.
  • Confidence 70 · mechanical.

• The quit-gate comment about demo data is false — tests/e2e-cucumber/tests/e2e/tui_driver.rs:674

  • It says "Demo data and several fixtures leave a model serving, which opens the quit-confirm prompt". has_live_instance returns false under simulated (app/mod.rs:647), which --demo and --replay set. The PR description says the same, so the comment contradicts it. Only the registry-backed fixtures prompt. Commit a6c7151's body repeats the claim.
  • Confidence 85 · mechanical · Fix: drop "Demo data and".

• managed_model_still_listed_as_running cannot fail on any branch of this change — tests/e2e-cucumber/tests/e2e/dash_steps.rs:680-709, dash.feature:353,371

  • The step's comment says it proves the prompt's "keeps running" claim against actual state. The fixture record is static JSON with no supervisor process, and no production path stops a service on quit. Dash-25 and dash-26 fail at the earlier prompt steps if the gate is reverted, so this step never decides the outcome.
  • It is a legitimate guard for unchanged behaviour. Say that rather than claiming it proves the message.
  • Confidence 70 · mechanical.

• The stop-prompt body comments say "managed instance" but the gate counts every serving instance — app/mod.rs:277 (the quit_confirm field doc) and :627-628 (the has_live_instance doc)

  • The code is instances.values().any(is_serving), which includes Docker- and Lemonade-discovered entries. Unmanaged instances are meant to prompt, per quit_confirm_body_omits_the_stop_command_for_an_unmanaged_instance. The code governs.
  • Confidence 85 · mechanical.

• A third copy of the "is anything serving" predicate — ui/mod.rs:224-232

  • nothing_is_currently_being_served is !has_live_instance() for every state in which the prompt can open. The PR repoints the launcher's is_running at has_live_instance to stop exactly this kind of drift, then adds another independent copy.
  • Confidence 70 · mechanical.

• overflow_line's doc overclaims — ui/format.rs:340-349

  • It says sharing the helper means a cap bump in one place "can't quietly leave another's overflow arithmetic behind". The cap is still a separate literal at each call site, so changing take(5) without overflow_line(_, 5) diverges. A shared constant per list would deliver the claim.
  • Confidence 70 · mechanical.

• launcher_quit_confirm_approve_exits_deny_and_cancel_stay is named for three verdicts but exercises two — ui/launcher.rs:896

  • Only n and y are driven. Cancel (Esc/q) and the None (cursor move) arm are not, so changing Some(false) | None => Continue so that None exits stays green.
  • Confidence 70 · mechanical.

• Comments narrate this PR's own earlier revisions as history on main — apps/rocm/src/dash.rs:259-263 ("three walks … once instead of three times"; there was one walk on prw-base), ui/mod.rs:192 ("the way they used to"), app/scrollbar.rs:105-110 ("once let a tab-bar click still land"), app/actions.rs:840-844 and :902-906 ("used to"). Readers will look for a history that is not there.

  • The Regression: / "used to" style in test comments matches existing tests and is fine. Production comments should describe the current behaviour.
  • Confidence 70 · mechanical.

• Commit history carries noise, and one message names internal tooling — 92c73a96, a33dfe6e, and the fix-up trail

  • 92c73a96 is an empty "chore: empty commit to re-trigger GitHub's PR sync".
  • About six "close review gaps" and "closing review" commits sit on one logical feature, plus f65d44ed (fmt) and 1de1db7f (harden assertions) that belong in the commits they fix.
  • The body of a33dfe6e names the "pr-review-watcher" tool, which does not belong in public history (AGENTS.md §2).
  • All commits carry a Signed-off-by trailer. I did not verify signatures cryptographically. Fix: squash or fixup before merge and reword that body.
  • Confidence 85 · mechanical.

Decisions for the author

• Launch-time managed_service_ids snapshot — tradeoff
  • Keeping the snapshot is cheap, and the description discloses the gap. The cost is that a model served from the dashboard's own Serve wizard, likely the most common path for a dashboard user, gets a warning with no stop command and no s.
  • Refreshing the set when the prompt opens, or on each snapshot, fixes that at the cost of a registry read or a daemon-side flag. The issue's ask is a way to stop a served model, so decide whether the first path meets it.
• Quit prompt defaults to Approve, unlike the chat tool-call approval's Deny — tradeoff
  • The prompt reuses the shared widget, so q and Esc cancel. A double q declines the quit, and Enter on the default button quits.
  • The default is documented, and the user asked to quit, so Approve is defensible. Nothing tests Enter against the default.
• Launcher to dashboard to quit now asks twice — tradeoff
  • In the hub (apps/rocm/src/dash.rs:617-631), quitting the dashboard returns to the launcher loop, which gates its own q. A user with a model serving confirms once per layer. That is consistent with the gate being per entry point, but it is a visible change.
• overlay_or_approval() is now misnamed, and the second operand of its use at scrollbar.rs:137 is unreachable — non-blocking-improvement
  • After the early blocks_body_absolutely() return it reduces to has_open_overlay(). The comment says the indirection is deliberate. A rename would remove the need for that comment.
• The approval modal now swallows tab-bar and footer-chip clicks — tradeoff
  • The earlier comment said those clicks "still work" during an approval, and the description discloses this change. It is consistent with keyboard behaviour, where those controls are already inert. tui_driver.rs has no mouse support, so only unit tests cover it, which the PR text could state.

Positive signals

• Routing every quit entry point through one request_quit gate, with resolve_quit_confirm_key shared between the dashboard loop and the launcher, avoids the two loops drifting apart.
• Replacing the hand-rolled startup classification with discover_managed_services, and building ManagedDiscovery once in dash.rs, keeps the pre-launch read in step with the daemon's own rules. The no bound port and startup_phase tests pin that.
• open_approval refuses while the quit prompt is open. Without it a y meant to confirm the quit could approve an unreviewed mutating tool call, and the test covers it.
• dash-25 matches e2e-mock, the service id, rather than the model name, to prove the Services overlay opened. The model name was already visible behind the dimmed modal.

Deployment notes

None

What this covered

• All 17 changed files were read in full, with their diffs and the surrounding code. That included ui/approval.rs, ui/services_manager.rs, ui/serve_wizard.rs, the daemon registry.rs and runner.rs, rocm-core's generate_service_id, the mock server fixture, and AGENTS.md.
• The review was split as follows:
  • App state and routing (app/*).
  • UI, launcher and apps/rocm (ui/mod.rs, ui/launcher.rs, ui/format.rs, tabs/*, dash.rs).
  • E2E steps, feature file and docs.
  • The source-reading passes: agent-instruction adherence, history, prior changes, code comments.
• Reviewed against prw-base...HEAD at 37ed114.
• I ran cargo test -p rocm-dash-tui --lib unmodified (854 passed) and six single-line mutations, each reverted afterwards:
  • Delete the seed_startup_live_service call.
  • Hard-code false for the startup flag.
  • Drop the is_serving filter.
  • Delete the s-key guards.
  • Flip the dashboard's offer_stop_action argument.
  • Flip the launcher's offer_stop_action argument.
• Not run:
  • The cucumber scenarios (dash-25 and dash-26 need a pty and the repo's runners). I took CI's reported state as the signal. Every check completed SUCCESS except "E2E tests (MI300X)", which was QUEUED, and "Skill checks (skillscope)", which was SKIPPED. Those two did not run and are not counted as passes.
  • cargo clippy, the full workspace test run, and scripts/smoke_local.py.
  • Windows behaviour. Both scenarios are @requires-os:linux, like every scenario in dash.feature, and the file header documents why.
• The pull request description and issue #145 were read after the findings were settled. Earlier review rounds and discussion were not read, so nothing here is reconciled against them. The author of the fixes is not the reviewer.

[rominf]

🔴 Automated review · pr-review-watcher · 37ed114

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

Round 2 (round 1 was the change request at 77414fb). Each finding is marked [new] or [standing]. This round reviewed the whole change blind, then put each round-1 blocker it did not re-find back to the same reviewer, one at a time, together with the author's reply. Those verdicts are anchored evidence from reading the code; nothing was run (disk was below the build floor).

Round-1 blockers, settled:

• Demo/replay opens the prompt: resolved. has_live_instance returns false under simulated (app/mod.rs:647), and simulated is set before the first key (event_loop.rs:839). The launcher never sees simulated data.
• Decline tests bypass the real path: resolved. Both tests now go through resolve_quit_confirm_key and assert quit_confirm.is_none() (actions.rs:923-925, :946-951).
• Stop command needs an id the prompt never shows: the id is now printed, and only for registry-managed instances. Whether the printed command actually clears the condition is still untested. This round re-found that independently; see Blocking.
• Untested startup-race wiring and model list:
  • The wiring half was re-found independently; see Blocking.
  • The list cap and the "…and N more" line are now covered (ui/mod.rs:1285).
  • Still uncovered: the managed-instance bullet "• {} — stop it first:" (ui/mod.rs:294). Its only unit test (:1230) sets a model name and never asserts it. The other assertion on that wording is the case-mismatched one below. [standing]
• "Fixes #145": partly holds. rocm services stop <real-id> on the launcher and dashboard, plus s → Services on the dashboard, now give a way to stop a model that was serving when the session started.
  • A model served from the dashboard's own Serve wizard still gets only the warning (stoppable ids are frozen at launch, ui/mod.rs:282). On Windows it gets no prompt at all.
  • "Fixes #145" in the PR body closes the issue on merge.
  • To resolve: change it to "Refs #145" and track the mid-session gap separately, or re-read the registry in request_quit, which closes both gaps. [standing, narrowed]

What this change request gates on: the standing items above, the remediation-command test, and the two checks that cannot fail, below. The launcher → dashboard double prompt was rated non-blocking in round 1, and nothing new has been found about it. So although the method lists it under Blocking this round, it does not count toward this change request; it remains the author's call.

Review — needs work

Full review of the whole change.
Says it fixes #145 ("Stop serving a model": the model keeps running after you exit rocm, with no obvious way to stop it). It warns at quit time and makes the existing stop path easier to find. The description scopes out the stop affordance for models served mid-session, and that part is retired under Decisions for the author.

Blocking

• [standing: non-blocking in round 1, not counted toward the gate] Opening the dashboard (or Chat) from the launcher and quitting it asks "quit anyway?", but y goes back to the launcher, which then asks again — apps/rocm/src/dash.rs:617-630, crates/rocm-dash-tui/src/app/actions.rs (request_quit), crates/rocm-dash-tui/src/ui/launcher.rs (handle_launcher_key)

  • run_launcher loops. LauncherRoute::Dashboard => run(None, …) and LauncherRoute::Chat return into that loop, so y on "Still serving — quit anyway? … It keeps running in the background after you quit." does not quit.
  • The loop then re-reads the registry, and q at the launcher calls request_quit again, which opens a second prompt.
  • So the first prompt makes a claim about quitting that is false on this path. That contradicts the description's "approving quits anyway". Bare rocm → Open dashboard is the documented front door. Neither dash-25 nor dash-26 enters the dashboard through the launcher.
  • Confidence 85 · logic · Fix: tell a dashboard hosted by the launcher that it is hosted (e.g. a field on ResolvedArgs) and skip the gate there, since leaving it is "back to menu", not quit. Add a scenario for launcher → dashboard → q.

• [standing, re-found] The remediation command is only checked as text; nothing shows that running it clears the condition (AGENTS.md §3) — tests/e2e-cucumber/tests/e2e/dash_steps.rs:557-568 (quit_confirm_shows_real_stop_command), crates/rocm-dash-tui/src/ui/mod.rs (quit_confirm_body_prints_a_working_stop_command_for_a_managed_instance)

  • §3 says a command named in remediation advice "must exist, accept those flags, and actually clear the condition that printed it", and the covering test asserts the message and the resulting state together.
  • Both tests match the string rocm services stop e2e-mock --yes. The World teardown runs that command but asserts nothing about the result.
  • The step's doc comment and the description's §3 checkbox both say this is satisfied.
  • The command itself does exist with those arguments (apps/rocm/src/main.rs:960, dispatched at :7988), and container_id == service_id (registry.rs:166).
  • Confidence 85 · architectural (cited rule) · Fix: in dash-25 or dash-26, run the printed command and assert the service is no longer ready/running (and that the prompt no longer appears). Otherwise mark the pin with a comment naming the test that proves the behavior, as §3 allows.

• Two checks claim coverage they cannot fail on

  • [standing, re-found] seed_startup_live_service_wires_both_fields_onto_state — crates/rocm-dash-tui/src/app/event_loop.rs:2187, call site :826.
    • The test's comment says it exists because deleting the call site "would not fail any other test". But it calls the helper directly, so deleting the call at :826 leaves it green.
    • dash-25 waits for "the managed model is displayed" before pressing q, so nothing reaches the startup-race path end to end.
  • [new] A case-mismatched negative assertion — crates/rocm-dash-tui/src/ui/mod.rs:1056.
    • quit_confirm_body_says_nothing_is_being_served_once_a_snapshot_confirms_it asserts !contains("Stop it first"), but the code prints "— stop it first:" in lower case (ui/mod.rs:294). The assertion can never fail.
  • Confidence 85 · mechanical · Fix: make the wiring test go through the code that event_loop actually runs (e.g. extract the whole state-from-args setup and test that), or reword its claim. Match the real lower-case text.

Non-blocking

• [new] The stop command wraps at 80 columns for real service ids, though a comment says it never will; with several managed models the popup also cuts off its own warning — crates/rocm-dash-tui/src/ui/mod.rs:283-295, crates/rocm-dash-tui/src/ui/approval.rs:115-131

  • The wrap: generate_service_id (crates/rocm-core/src/lib.rs:4523) builds ids from the engine, a slug of up to 24 characters, and 13 digits of milliseconds. A typical id is 36–43 characters, so " rocm services stop <id> --yes" is 63–70 characters.
  • draw_approval uses centered_rect(72,70,96,24,…). At 80 columns that leaves an inner width of about 55, and Wrap{trim:false} breaks the command across two rows.
  • The comment's "keeps the command short enough to never need to wrap" only holds for the 8-character e2e id e2e-mock.
  • The cut-off: at 80x24 about 10 text rows remain. Each managed instance takes 2–3 lines, plus 4 trailer/hint lines, so with 3 managed instances the lines that get dropped are "It keeps running…" and "Press s…". Nothing marks the cut.
  • Confidence 85 (wrap) / 70 (cut-off), from arithmetic, not a render · logic · Fix: correct the comment, and render with a realistic id and several instances at 80x24 in a TestBackend test.

• [standing] On Windows, the dashboard's quit gate only ever sees the launch-time read, so a model served mid-session gets no prompt there at all — crates/rocm-dash-tui/src/client.rs:181-187, apps/rocm/src/dash.rs:994-998, crates/rocm-dash-tui/src/app/mod.rs (has_live_instance)

  • On Windows the dashboard client never connects, so has_received_snapshot stays false and instances stays empty.
  • A model started from the Serve wizard: q exits with no prompt. That contradicts the description's "will show the quit-confirm warning (that part does use the live snapshot)" and README's "it asks first — from the launcher or the dashboard alike".
  • A model already serving at launch: it gets only the hedged "A model was serving when this started", even after it stops.
  • Both scenarios are @requires-os:linux. Not established by a run.
  • Confidence 70 · logic · Fix: re-read the registry in request_quit. That also fixes the frozen-id problem under Decisions. Otherwise qualify README and the description for Windows.

• [new] Doc comments contradict the code

  • ResolvedArgs::managed_service_ids (crates/rocm-dash-tui/src/app/types.rs): it justifies the staleness because a new service_id appears only when the user runs rocm serve, "which this process does not observe". The dashboard's own Serve wizard runs rocm serve … --managed (ui/serve_wizard.rs:190). The PR description itself states the opposite.
  • has_stoppable_managed_instance (app/mod.rs): the doc says the s key handler shares it "so the two can't drift". quit_confirm_open_services_key re-implements the predicate inline instead.
    • Separately, the startup-fallback condition !has_received_snapshot && startup_has_live_service is written twice, in has_live_instance and in nothing_is_currently_being_served (ui/mod.rs:230). That is the twin-copy pattern.
  • The has_live_instance and quit_confirm field docs say "managed instance". The code counts any serving instance, including external Docker/Lemonade ones.
  • quit_confirm_shows_real_stop_command (dash_steps.rs): its doc describes the whitespace-collapsing reassembly that commit c30f00d replaced.
  • quit_and_wait (tui_driver.rs): it says demo data opens the prompt. Since simulated short-circuits has_live_instance, it cannot.
  • quit_confirm_body: its doc says the double iteration "avoids a per-render-frame allocation", but every frame allocates a Vec<String> of formatted lines.
  • Which side governs: in each case the code is the intended behavior, so the comments are what need correcting.
  • Confidence 85 · mechanical · Fix: call has_stoppable_managed_instance from the key handler, express nothing_is_currently_being_served through has_live_instance, and correct the remaining docs.

• [new] docs/manual-testing.md describes two states a tester cannot reach — docs/manual-testing.md:321-324

  • It says declining closes "a manager overlay" that was open when you pressed q. But every manager overlay consumes q and closes itself (services_manager.rs:162, engine_manager.rs:149, config_manager.rs:129), so the prompt never opens over one.
  • "Returns to the dashboard" is wrong for the rocm (launcher) half of the same step, which returns to the launcher.
  • Confidence 70 · mechanical · Fix: drop the overlay example and say "returns to where you were".

• [standing in part] Scenario wording — tests/e2e-cucumber/features/dash.feature:349-350

  • Two When lines are consecutive; the second should be And.
  • The final y confirm is hidden inside "the user quits the dashboard/launcher", so the scenario never states the confirm step it relies on.
  • The "Press s to open Services" hint is never asserted on screen.
  • Confidence 70 · mechanical.

• [new] Commit history

  • 92c73a9 is an empty "re-trigger" commit and f65d44e is an fmt fix-up.
  • About ten "close review gaps" fix-of-the-fix commits make the history noisy.
  • a33dfe6 names an internal automation tool in its message, which carries into a squash body.
  • Confidence 70 · mechanical · Fix: squash, and edit that message.

Decisions for the author

• A model served from the dashboard's own Serve wizard gets no stop command and no s hint until restart — answered-by-intent

  • The description says: "a model served mid-session through the dashboard's own Serve wizard will show the quit-confirm warning … but not yet the stop command or the s hint, until the session is restarted. No false claim is printed either way."
  • This would otherwise have been blocking. managed_service_ids is frozen at launch, and this is probably the most common path behind #145.
  • Re-reading the registry in request_quit is cheap, and it also closes the Windows gap above. Confirm that leaving this out is still intended.

• The quit prompt reuses the tool-approval component verbatim — tradeoff

  • Reusing it follows AGENTS.md and keeps the key handling in one place.
  • The cost is that the quit prompt is titled "Review: …" with "Approve (y)" / "Deny" buttons, which reads oddly for a quit.

• A model that is Starting (downloading or loading) doesn't trigger the prompt — tradeoff

  • is_serving() covers Ready/Running only, which matches "still serving".
  • But a model that has just been launched also keeps loading after quit, and nothing says this exclusion is deliberate.

• The quit-confirm builders live in ui/mod.rs (about 140 lines plus tests) rather than a ui/quit_confirm.rs — non-blocking-improvement. Other prompts have their own files.

Positive signals

• open_approval refuses while the quit prompt is open, so a y meant for quitting can't approve an unreviewed tool call. A regression test covers it.
• All the quit entry points go through one request_quit gate: q, the Esc-menu Quit row, /quit, /exit, and the launcher's q/Esc. The twins were kept in sync.
• The title and body are derived from one predicate, so "Still serving" can never sit above "Nothing is being served anymore."
• resolved_args reads and classifies the registry once and delegates to discover_managed_services, instead of hand-copying the status, startup_phase and port-0 rules.
• The stop line is printed only for ids this host's own registry manages, so external Docker/Lemonade instances never get a command that can't stop them.

Deployment notes

None

What this covered

• Read: all 17 changed files, at prw-base...HEAD (head 37ed114), across four coupled-group workers, plus the code around them: the registry, the serve wizard, the services manager, the launcher loop, and the Windows client stub.
• Partial reads: the #[cfg(test)] modules of app/mod.rs and event_loop.rs were read where the diff touched them; slash.rs past line 330 was grep-scanned only; for ui/tabs/instances.rs only its test-fixture hunk was read.
• Stale base: the PR's merge-base is 25 commits behind prw-base.
• Did not run:
  • No tests or mutations. Free disk was below the 40G floor (36G at the end), so every test verdict and the wrap and cut-off arithmetic are static.
  • Prior-changes pass: partial. It covered earlier PRs' commits, not their review comments.
  • Design review: the strategic questions were investigated in the driver's own context, not by independent workers.
• CI: green for 37ed114, except the MI300X E2E lane, which is still queued.
• Not reconciled against any discussion here.

[jussielo-amd]

Thanks for the thorough pass. Note up front: the commit history on this branch has been squashed (the ~15 incremental commits you cited by hash, e.g. 1cafcd39, a33dfe6e, d4ec0d51, are gone — folded into one commit, 3e3a9324, at the current head). Responding to every item below against that commit.

Blocking — all five addressed

• startup_has_live_service hand-off untested, and the covering test doesn't exercise the real call site: the whole mechanism this gap lived in is gone. startup_has_live_service/managed_service_ids (the frozen launch-time snapshot) are replaced by a live, on-demand registry read (ManagedRegistryProbe, new file registry_probe.rs) — there's no longer a one-shot seed to silently drop. The remaining hand-off (managed_registry_probe/launcher_hosted → AppState) is pulled into its own fn, seed_registry_probe_and_hosting, with a direct test (seed_registry_probe_and_hosting_wires_both_fields_onto_state) that constructs the real inputs and calls the real fn, the same way apply_startup_focus's own test does.
• is_serving() filter untested in has_live_instance/has_stoppable_managed_instance/quit_confirm_open_services_key: fixed. Added has_live_instance_ignores_a_non_serving_instance (seeds Starting/Stopped/Error/Unknown and asserts none trips the gate) and the equivalent for has_stoppable_managed_instance and quit_confirm_open_services_key (quit_confirm_open_services_key_ignores_a_non_serving_managed_instance).
• Two added tests can't fail on the code they name: the case-mismatched assertion (!contains("Stop it first") vs. the actual lowercase "stop it first:") is gone — removed rather than just case-corrected, because a fresh review caught that even case-corrected it would still be structurally vacuous (the snapshot-confirmed branch returns early before the registry is ever read, so no seeded state can make it fail). Real stop-line coverage lives in the sibling tests you'd expect (quit_confirm_body_prints_a_working_stop_command_for_a_managed_instance / …_omits_…_for_an_unmanaged_instance). For the s-key guard: quit_confirm_open_services_key_is_a_no_op_without_a_stoppable_managed_instance and …_focuses_the_stoppable_managed_row now isolate each guard with a seeded stoppable instance (verified by mutation — deleting either guard alone fails exactly one of these).
• offer_stop_action untested at both call sites: fixed. Added draw_renders_the_press_s_hint_for_a_stoppable_managed_instance (dashboard, via the real ui::draw entry point, hint present) and strengthened the launcher's existing render test to assert the hint's absence there.
• No scenario for /quit//exit, the Esc-menu Quit row, or the launcher's Esc: /quit and /exit dispatch through the identical "quit" | "exit" => request_quit(self) match arm in slash.rs — one new scenario (dash-28) drives /quit through a real pty; /exit is the same line, not a separate path. For the Esc-menu Quit row and the launcher's own Esc, both route through the same shared request_quit gate the q paths do with no behavior of their own beyond the UI gesture that invokes it, so per AGENTS.md §3 I'm relying on the existing unit coverage there rather than adding scenarios — now stated explicitly in the PR's "Scope" section rather than left silent.

Non-blocking

• managed_service_ids staleness comment was wrong, and the README didn't mention the limit: moot — the live-read replacement means there's no snapshot to be stale, and the limitation it was describing (a Serve-wizard-started model getting no stop command until restart) no longer exists. README updated to describe the one behavior change that is real now: a dashboard/chat session opened from the launcher skips its own prompt.
• Step comment claiming whitespace-collapse reassembly, contradicted by a later commit: fixed — the stale doc comment is corrected to describe the actual line-split layout.
• Quit-gate comment falsely saying demo data opens the prompt: fixed, "Demo data and" dropped from the tui_driver.rs comment.
• managed_model_still_listed_as_running can't fail on any branch of this change: left as-is per your own read — it's a legitimate guard for unchanged behavior, not a proof of the new claim; no change needed.
• "managed instance" wording when the gate counts any serving instance: fixed in the doc comments that had it wrong.
• Third copy of the "is anything serving" predicate: nothing_is_currently_being_served now expresses itself through has_live_instance instead of re-deriving the condition.
• overflow_line's doc overclaim: left open, out of scope for this pass — not touched by the live-read/launcher-hosted work; happy to take it as a separate small fix if you'd like.
• launcher_quit_confirm_approve_exits_deny_and_cancel_stay named for three verdicts, exercises two: left open, same reasoning — pre-existing test, not touched here.
• Comments narrating this PR's own revisions as history on main: resolved by the squash — there's no longer an intermediate history for those comments to (mis)reference.
• Commit history noise, and a33dfe6e's body naming an internal automation tool: the whole branch is squashed into one commit now; the internal-tool-name reference is gone with it.

Decisions you flagged for the author

• Launch-time managed_service_ids snapshot (tradeoff): resolved in favor of making it live rather than picking a side — see above.
• Quit prompt defaults to Approve (tradeoff): kept as-is; the user already asked to quit, so Approve stays the sensible default.
• Launcher → dashboard → quit asks twice (tradeoff): fixed, not left as a tradeoff — a dashboard/chat session opened from the launcher now exits unconditionally on its own q/Esc (AppState::launcher_hosted), and it's the launcher's own gate that governs the real exit. New scenario dash-29.
• overlay_or_approval() misnamed / unreachable second operand (non-blocking-improvement): left open, out of scope for this pass.
• Approval modal now swallows tab-bar/footer-chip clicks (tradeoff): unchanged from the original PR's disclosed behavior; no action needed.

All tests/clippy/fmt/crate-edges green on the current head; full details in the PR description, which I've updated to describe the final state rather than the superseded snapshot-based scope.

[jussielo-amd]

Responding to round 2. Same note as on the other review: the commit history is squashed — every hash you cited (37ed114a, and the earlier 77414fb5 round-1 head) is gone, folded into one commit, 3e3a9324, at the current head.

Standing items, settled

• Launcher → dashboard → quit asks twice: fixed outright rather than left as the author's call. A dashboard/chat session opened from the launcher now exits unconditionally on its own q/Esc (AppState::launcher_hosted), since that's "back to the launcher menu," not "quit" — the launcher's own gate, re-entered when control returns, is what actually governs process exit. New scenario dash-29 proves only one prompt appears and it's the one that governs the real exit.
• Remediation command only checked as text, not that running it clears the condition: fixed. New scenario dash-27 runs the actual printed rocm services stop <id> --yes and asserts via services list --json that the service is no longer ready/running afterward — the AGENTS.md §3 proof you were after, not a comment pinning the gap.
• seed_startup_live_service_wires_both_fields_onto_state doesn't cover the real call site: the mechanism it tested is gone — replaced by the live registry read. Its replacement (seed_registry_probe_and_hosting) has a direct test that constructs the real inputs and calls the real fn.
• Case-mismatched negative assertion (ui/mod.rs:1056): removed rather than case-corrected — a fresh review caught that even with the right case it would still be structurally vacuous (the branch it's in returns early before the registry is ever read). Real stop-line coverage lives in the sibling tests.

Blocking items from this round

• Stop command wraps at 80 columns despite a comment claiming it never will, and the popup can cut off its own warning at 3+ instances: comment corrected (generate_service_id produces roughly 23–47 characters, not the "never wraps" claim); the wrap/cut-off behavior itself is unchanged — flagging this is useful, not asking the layout to change blind. If you want the popup widened or the id elided, happy to take that as a follow-up rather than block this fix on it.
• Windows: dashboard's quit gate only sees the launch-time read, no prompt at all for a mid-session model: fixed as part of the live-read change — the registry probe is a local file read on every host, not a daemon round-trip, so Windows (where the daemon client never connects) gets the same live answer for the whole session as Linux does, not a frozen launch-time guess.
• Doc comments contradicting code (managed_service_ids staleness justification, has_stoppable_managed_instance's "can't drift" claim while the key handler reimplements it inline, duplicate "is anything live" predicates, "managed instance" wording, stale e2e-step comment, demo-data quit-gate comment, quit_confirm_body's per-frame-allocation overclaim): all fixed. The staleness-justification one is moot rather than corrected — there's no snapshot left to justify.
• docs/manual-testing.md describes unreachable states: fixed — the "closes a manager overlay" example is dropped (every manager overlay already consumes q and self-closes before the prompt could ever open over one) and the launcher's own copy now correctly says "returns to where you were" instead of "returns to the dashboard".
• Scenario wording (consecutive When, hidden confirm step, unasserted "Press s" hint): fixed — dash-25 now asserts the hint on-screen explicitly and the grammar issue is corrected.
• Commit history: the whole branch is squashed into one commit; the internal-tool-name reference in that one commit's body is gone with it.

Decisions for the author

• Serve-wizard-started model gets no stop command until restart (answered-by-intent in your read): resolved rather than left as a known gap — this is exactly what the live-read change fixes. A model started mid-session via the Serve wizard is picked up the next time any gate consults the registry.
• Quit prompt reuses the tool-approval component verbatim (tradeoff): kept as-is; this was a deliberate reuse decision (AGENTS.md's "reuse shared components" guidance), not an oversight.
• A Starting (downloading/loading) model doesn't trigger the prompt (tradeoff): kept as-is — is_serving() (Ready/Running) matching "still serving" is the intended scope; a Starting instance isn't yet doing the thing the prompt warns about.
• Quit-confirm builders living in ui/mod.rs rather than their own file (non-blocking-improvement): left open, out of scope for this pass.

All tests/clippy/fmt/crate-edges green on the current head (3e3a9324); PR description rewritten to describe the final state.

[rominf]

🔴 Automated review · pr-review-watcher · 3940e91

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

Round 3. Round 1 was the change request at 77414fb and round 2 the one at 37ed114; both commits are gone after the squash. Each finding is marked [new] or [standing]. This round reviewed the whole change blind, statically: disk was below the build floor, so nothing was built or run here. It then put each earlier blocker that the blind pass did not re-find back to the same reviewer, one at a time, together with the author's reply. Those verdicts are anchored evidence from reading the code.

Earlier blockers, settled:

• Remediation command checked only as text (AGENTS.md §3): the approach is now right, since dash-27 runs the printed command. But the proof does not exist yet, because dash-27 kills the test runner. This was re-found independently and is merged into the first Blocking item. [standing, re-found]
• Wiring test that cannot fail: re-found independently against the renamed seed_registry_probe_and_hosting. See Blocking. [standing, re-found]
• Case-mismatched !contains("Stop it first") assertion: resolved. The assertion is gone, and the stop line is covered at ui/mod.rs:1258 and :1284.
• Managed-instance bullet untested: still holds. See Blocking. [standing]
• "Fixes #145": narrowed again, and it still holds in part.
  • The live registry read fixes the launcher on both platforms, and probably the Linux dashboard.
  • A Windows user who runs rocm dash or rocm chat directly still gets only the generic warning, with no model name, no stop command and no s (Blocking item 3).
  • The claim is in the commit message as well as the description, so it closes #145 on main. See Blocking. [standing, narrowed]
• Launcher → dashboard double prompt (non-blocking in rounds 1 and 2): not re-found this round. dash-29 now covers it, though that scenario did not run to completion on this head (see the CI note below).

Driver note on CI (checked directly in the job logs): all three self-hosted Linux lanes on this head died at the same point. They are Strix Halo WSL2, Strix Halo Ubuntu (release) and Strix Halo Ubuntu (nightly). In each, Scenario: dash-27 - Running the quit prompt's printed stop command actually stops the model starts, the step And the quit prompt shows the real stop command for the managed model passes, and then process didn't exit successfully: …/e2e-… (signal: 15, SIGTERM) follows. The hosted E2E tests job fails with the same SIGTERM, and that job passes on main at 213c2cb and 1e5aac8. So the two Ubuntu lanes are not separate GPU-preflight or pre-warm failures, as the "Separately" sub-bullet in the first Blocking item below says: their logs show normal VRAM/GTT readings and the same dash-27 death. Strix Halo Windows passed. MI300X had not reported.

Review — needs work

Full review of the whole change.
It says "Fixes #145" (issue: "After serving a model, the process keeps running when I exit rocm. I don't see any obvious way to stop the model serving service."). The warning, the printed stop command and the s jump to Services do answer that. Whether the PR holds up on Windows is covered under Blocking.

Blocking

• [new; carries the standing §3 item] dash-27 sends SIGTERM to the e2e test runner itself, so the required E2E tests check is red — tests/e2e-cucumber/tests/e2e/dash_steps.rs:771 (run_quit_prompt_stop_command), tests/e2e-cucumber/tests/e2e.rs:566-569, apps/rocm/src/main.rs:15474 (terminate_recorded_service_pids filter), crates/rocm-core/src/proc_lifecycle.rs:103

  • What happens:
    • register_mock_service_with writes supervisor_pid: std::process::id() and engine_pid: Some(std::process::id()) into the fixture record. That is the PID of the cucumber harness.
    • dash-27 then runs rocm services stop e2e-mock --yes. terminate_recorded_service_pids skips only PID 0 and the rocm child's own PID.
    • The record has no *_start_ticks, so identity_state falls through (None, _) => IdentityState::Matches ("best-effort proceed").
    • terminate_verified(.., KillScope::Tree, ..) then signals the harness and its whole process tree.
  • CI evidence:
    • Strix Halo WSL2 lane: Scenario: dash-27 … started at 14:27:55, then process didn't exit successfully: …/e2e-… (signal: 15, SIGTERM) at 14:27:57.
    • Hosted E2E tests job (run 38262186241): the same SIGTERM about 30 s into the suite.
    • main passed the same job at 213c2cb.
    • Because the suite aborted, the hosted lane and the WSL2 lane did not run the rest of their e2e scenarios, dash-25..29 included, on this head.
  • The repo already knows about this hazard:
    • e2e.rs:658-661 skips e2e-mock in teardown because "The planted mock record has no real process to stop".
    • service_cleanup_steps.rs plants supervisor_pid: 0 because a live PID "makes the World's teardown services stop kill this test process".
  • Until this is fixed, the §3 proof that the printed stop command actually works does not exist.
  • Separately: both Strix Halo Ubuntu lanes are also red, on la03-s04 hosts. The release lane failed GPU preflight on the VRAM/GTT floor. The nightly log stops during pre-warm. Nothing ties either to this change, but they are still red and need a rerun.
  • Confidence 100 · logic · Fix: plant dash-27's record with the PID of a disposable child the step spawns (for example sleep), then assert that child is gone. Never point a stop at the harness PID.

• [new] The live registry probe does not check process liveness the way rocm services does, so a crashed service still triggers the prompt and its "keeps running" claim — apps/rocm/src/dash.rs:341-351, crates/rocm-dash-tui/src/registry_probe.rs:62, crates/rocm-dash-tui/src/ui/mod.rs:311

  • LiveManagedRegistryProbe::snapshot does load_service_records + discover_managed_services and checks svc.status.is_serving(). That reads the raw on-disk status string.
  • rocm services list goes through load_managed_services / refresh_managed_service_runtime_liveness in main.rs, which demote a ready/running record whose PIDs are dead.
  • So after a crash that leaves "status": "ready" on disk, quitting prompts "Still serving — quit anyway?" and prints "It keeps running in the background after you quit." It also gives a stop command (and on the dashboard, s) for a service services list reports as stopped.
  • On the launcher, and on Windows where the probe decides alone for the whole session, nothing corrects this.
  • The trait doc says the probe will "classify it the same way rocm services does", which the code contradicts. The code is what runs, so the doc is the wrong side.
  • This is the outcome-claim shape AGENTS.md §3 names. The pre-existing raw-status read is now behind a blocking prompt that makes that claim.
  • Confidence 70 · logic · Fix: add a local-only PID check (recorded_service_pids + process_is_running, no network) to the probe. Otherwise hedge the printed claim and correct the trait doc.

• [new] On the Windows dashboard the prompt never names the model, never prints the stop command and never offers s, yet the description and README say it does — crates/rocm-dash-tui/src/ui/mod.rs:265-276, crates/rocm-dash-tui/src/app/mod.rs (has_stoppable_managed_instance), README.md:169-172

  • On Windows instances never fills: the client connect path is unix-only, and dash.rs gets no daemon client there.
  • quit_confirm_body therefore always takes the total == 0 branch and prints only "A model was serving when this started." / "It may still be running…".
  • That holds even though managed_ids was already read from the live probe on line 252.
  • has_stoppable_managed_instance iterates instances, so the s hint and key never appear on Windows.
  • Claims this contradicts:
    • Description: "quitting now shows a confirm prompt first, naming the serving model(s) and … the real rocm services stop <id> --yes command". It also says Windows "still gets a correct answer for the whole session".
    • README: "From the dashboard's own copy of that prompt, pressing s instead jumps straight to the Services overlay". It states this without condition.
  • "When this started" is also wrong for a model started mid-session, which the probe exists to catch.
  • The comment at lines 266-274 still describes the old launch-time snapshot ("a model was recorded live on disk at launch").
  • Confidence 85 · logic · Fix:
    • Either finish the work: when !has_received_snapshot, build the list and stop commands from the probe's managed_ids, and say "A model is serving".
    • Or narrow the description and README to what Windows actually shows.

• [standing, re-found] The comment on seed_registry_probe_and_hosting says its test catches a deleted call site, but it cannot — crates/rocm-dash-tui/src/app/event_loop.rs:229-236, event_loop.rs:2179-2205

  • The doc says the helper exists "so a regression test can exercise the actual call site". The test comment says that without it "a silent no-op here (the call site deleted, or the fields swapped) would not fail any other test".
  • The test calls the helper directly. Deleting the call at event_loop.rs:823, or passing launcher_hosted: false there, leaves it green.
  • What it actually checks is two field assignments.
  • Verdicts: (1) it passes with the wiring reverted; (2) the call site is the branch it misses.
  • Confidence 85 · mechanical · Fix: say honestly what the test covers, or test the wiring through a constructor the event loop itself uses.

• [standing, not re-found blind; settled as still holding] Nothing asserts the managed-instance line that names the serving model — crates/rocm-dash-tui/src/ui/mod.rs:302 ("• {} — stop it first:"), test at ui/mod.rs:1258-1281

  • The managed-instance unit test sets model_name: "Qwen2.5-7B" but asserts only the rocm services stop svc-managed --yes line. The only model name asserted anywhere is the unmanaged bullet's (ui/mod.rs:1303).
  • dash-25 and dash-26 match the "quit anyway" marker and the stop command against the whole screen. Their the managed model is displayed checks run before the prompt opens and after it closes, so nothing ties the model name to the popup.
  • Fix: assert a line equal to "• Qwen2.5-7B — stop it first:" in the existing unit test. An e2e check would not prove it, because the dashboard behind the popup already shows the model name.
  • This verdict came from putting the earlier wording to the reviewer, so it is anchored rather than blind.

• [standing, narrowed] "Fixes #145" in the commit message closes the issue on main, but a Windows user who opens rocm dash or rocm chat directly still gets no way to stop the model from the prompt — commit 3940e91 message, PR description; mechanism as in the Windows item above (ui/mod.rs:265-276, app/mod.rs:691-694)

  • The launcher on both platforms now re-reads the registry on every pass (apps/rocm/src/dash.rs:643, ui/launcher.rs:303-318), so that part of the earlier objection is resolved.
  • On the Windows direct path the prompt opens, but shows only the generic warning. That is #145's own complaint: "I don't see any obvious way to stop".
  • To resolve: fix the Windows item above, which also makes this claim true. Otherwise change the commit message to "Refs #145" and track the Windows gap separately.
  • This verdict came from putting the earlier wording and the author's reply to the reviewer, so it is anchored rather than blind.

Non-blocking

• [new] The prompt body is silently clipped once several managed models serve with real IDs — crates/rocm-dash-tui/src/ui/mod.rs:302-303, ui/approval.rs:115-131

  • Each managed model takes 2 lines, and a 23-47-character real id wraps the command line to 3.
  • The body area is the popup height (centered_rect(72, 70, 96, 24)) minus 2 button rows. At 80x24 that is about 12 rows.
  • With about 3 managed models, the trailer and the "Press s" hint fall below the clip with no marker. Paragraph has no scroll.
  • quit_confirm_popup_with_realistic_ids_does_not_panic_at_a_narrow_width asserts only that each id appears, so it stays green.
  • Confidence 70 · logic · Fix: put the trailer and hint before the list, or cap the list to the rows available.

• [new] Each frame reads the registry up to about four times while the prompt is open, and the title and body can disagree — ui/mod.rs:206, ui/mod.rs:212, ui/mod.rs:252-253, app/mod.rs (has_live_instance, has_stoppable_managed_instance)

  • Each call is a full read_dir + JSON parse of services/, which grows with stopped records.
  • On Windows this happens for the whole session.
  • Because the reads are separate, the title and body can be built from different registry states. That is the self-contradiction the change tests against elsewhere.
  • Confidence 70 · mechanical · Fix: take one snapshot per frame in quit_confirm_request and pass it down.

• [new] If the registry can't be read, the warning is silently skipped — crates/rocm-dash-daemon/src/registry.rs:123-137, apps/rocm/src/dash.rs:344-349

  • load_service_records returns an empty Vec when read_dir fails and skips unreadable or half-written files.
  • The gate then reads "nothing serving" and exits without asking, which is the original bug. There is no log line either.
  • Confidence 70 · logic · Fix: have the snapshot carry an "unknown" state, and make the gate ask when it is set.

• [new] Several quit paths can break while every scenario stays green

  • Launcher-hosted Chat route — apps/rocm/src/dash.rs:653, dash.rs:715: dash-29 drives only the launcher → Dashboard route. Changing run_chat(chat_mock, …, true) to false, or dropping args.launcher_hosted = launcher_hosted; at line 715, breaks nothing that is tested.
  • Esc-decline and Enter-confirm: docs/manual-testing.md promises both, but no scenario or test covers them; quit_and_wait always confirms with y.
  • Ctrl-C while serving: the new README text promises "Ctrl-C always quits immediately without asking", but dash-20 uses --demo (never serving) and dash-21 has no serving model. This is the "will not happen" promise shape AGENTS.md §3 names.
  • Confidence 70 · logic · Fix: a launcher → Chat scenario that quits back, plus Ctrl-C-while-serving and Esc/Enter variants on an existing scenario.

• [new] A step comment cites a commit hash that will not survive the squash merge — tests/e2e-cucumber/tests/e2e/dash_steps.rs:590 ("since commit c30f00d")

  • That object is not in the base history. It was an earlier head of this PR's branch, and the repo squash-merges.
  • Confidence 85 · mechanical · Fix: name ui::quit_confirm_body instead of the hash.

Decisions for the author

• Reusing the shared approval component for the quit prompt — tradeoff

  • Reusing ui/approval.rs follows AGENTS.md §6 and keeps the key handling consistent.
  • The cost is that the quit question shows "Approve (y)" / "Deny (n)" and "Esc/q cancel". A user who presses q, reads "quit anyway?" and presses q again is sent back to the dashboard, not out.
  • A per-request label override in the shared component would keep the reuse and fix the wording.

• managed_registry_probe: Option<…> is documented "never None", and None silently means "nothing live" — non-blocking-improvement

  • A required field with a null-probe implementation would make that invariant hold by construction rather than by a doc sentence.

Positive signals

• One request_quit gate shared by q, the Esc-menu Quit row, /quit//exit and the launcher, so the entry points cannot re-decide differently.
• open_approval refuses while the quit prompt is open, so a pending y can never approve a tool call nobody reviewed.
• The stop command is printed only for ids in the registry's managed set, so a Docker- or Lemonade-discovered instance never gets advice that cannot work.
• dash-27 runs the literal printed command and checks registry state, which is the right AGENTS.md §3 shape once the fixture PID is fixed.
• decline_quit_prompt waits for the marker to leave the screen, so the next "model is displayed" check cannot pass on the prompt's own text.

Deployment notes

None

What this covered

What was read: every file in prw-base…3940e91c (fe47c48…3940e91c). Four workers each took a coupled group:

• the app state and quit gate;
• UI rendering, the launcher and the scrollbar;
• the bin's dash.rs probe and launcher;
• the e2e tests and docs.

Each read its group's changed code completely, along with callers in apps/rocm/src/main.rs, crates/rocm-core/src/proc_lifecycle.rs and crates/rocm-dash-daemon/src/registry.rs. The very large app/mod.rs and event_loop.rs were read at the changed regions and their surrounding arms, not every unrelated line.

What did not run, and why:

• The review was static only. Free disk on this host is below the 40G floor, so no cargo build, test or clippy ran. CI results were read with gh instead.
• The finding scored 100 rests on CI logs plus a code trace, not a local run.
• The bin dash.rs worker hit the concurrent-subagent cap on its first spawn and was re-spawned once, successfully.
• The agent-instruction, history and code-comment passes ran inside the tests/docs worker.
• The prior-changes pass did not run: reading earlier PR reviews is outside this review's remit.
• Step 7 (design questions) was answered by the driver from the workers' evidence, not by independent workers.
• The self-hosted E2E (MI300X) lane was still queued when CI was read.

Discussion: not reconciled against any PR discussion here.

[jussielo-amd]

Thanks for the thorough pass. Responding against the new commit, 6b7ac3cd (on top of 3940e91c).

Blocking — all five addressed

• dash-27 sends SIGTERM to the e2e test runner itself: confirmed and fixed. register_mock_service_with deliberately points supervisor_pid/engine_pid at the harness's own PID for every other scenario (so startup liveness checks see a live process), but dash-27 goes on to run the literal rocm services stop e2e-mock --yes the prompt prints — which then signals whatever PID the record names. Added a new step, the managed model's record points at a disposable process, that spawns a sleep child and re-points the record at it just for dash-27; the child is reaped in E2eWorld::drop the same way tui is. Verified: ran dash-27 standalone and as part of dash-2[5-9] and the full dash- suite — all green, no SIGTERM.
• Live registry probe doesn't check process liveness: on reflection I didn't add a local PID check — apps/rocm::dash::services_past_attempts, a few lines above LiveManagedRegistryProbe in the same file, already explains why one was deliberately not added there (duplicates the CLI's liveness classification in a second place that could drift from it, trading an undercount for an overcount). Adding one to the probe would repeat exactly that tradeoff. Instead corrected the trait doc's overclaim ("classify it the same way rocm services does" → it classifies from raw on-disk status, which is not the same verdict) and added a matching "known divergence" note to LiveManagedRegistryProbe pointing at the same reasoning, so a future reader hits the explanation instead of re-discovering the gap.
• Windows dashboard prompt never names the model/stop command/s: narrowed is not what I did — quit_confirm_body's total == 0 branch (which Windows always takes, for the whole session, since instances never fills there) now lists each id the live registry read already has, with its real stop command, same as the normal branch — just without the pretty model name, which only a daemon snapshot can supply. Added quit_confirm_body_prints_the_stop_command_during_the_startup_fallback_when_the_registry_names_an_id to cover it (also exercises the pre-existing Linux startup-race window, which gets the same improvement). README and docs/manual-testing.md now say explicitly that the model name and the s hint need a daemon snapshot the Windows dashboard never gets, rather than claiming them unconditionally.
• seed_registry_probe_and_hosting's test doesn't cover the real call site: agreed, said so now instead of overclaiming. Doc comments on both the function and the test spell out what the unit test actually covers (the field assignment) versus what only the @requires-os:linux e2e lane covers (the real run_async call site).
• Managed-instance bullet's model name untested: added the missing assertion ("• Qwen2.5-7B — stop it first:") to the existing test.

Non-blocking

• Stale commit hash in a step comment (c30f00d4): fixed, now names ui::quit_confirm_body instead.
• Prompt body clipping at ~3 managed models, redundant per-frame registry reads, silent skip on an unreadable registry, several untested quit paths (launcher-hosted Chat, Esc/Enter, Ctrl-C-while-serving): left open for a follow-up — none are regressions from this round's changes, and bundling them into this round risks delaying the blocking fixes above. Happy to open a tracking issue if you'd like, or take them in a follow-up PR.

Decisions for the author

• Approval component reusing "Approve (y)"/"Deny (n)" wording for quit: left as-is this round — real but minor, and a per-request label override is its own small change I'd rather not bundle in here.
• managed_registry_probe: Option<...> documented "never None": left as-is — agree a required field with a null-probe impl would be stronger, but it's a structural change orthogonal to this round's fixes.

CI

• The dash-27 SIGTERM was killing the hosted E2E tests job and the Strix Halo WSL2/Ubuntu lanes before they reached dash-28/29; that's fixed now.
• The two Strix Halo Ubuntu lanes you flagged as unrelated (VRAM/GTT preflight and a nightly pre-warm stall) are still worth a rerun once this head's CI completes, if they're still red.

All of cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all --check, and cargo xtask check-crate-edges pass locally on this head, plus the full dash.feature suite (cargo xtask e2e -- -n "dash-") — 28/29 scenarios pass; the one failure (dash-24, WSL GPU telemetry) reproduces identically on the unmodified prior head in this sandbox, which has no real GPU under its WSL host-capability detection, so it's an environment gap, not a regression from this change.

fixed

[rominf]

🔴 Automated review · pr-review-watcher · 6b7ac3c

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

Round 4. Earlier rounds: the change request at 77414fb, the one at 37ed114, and the one at 3940e91, which the author dismissed with "fixed". Each finding below is marked [new] or [standing]. This round reviewed the whole change blind, at 6b7ac3c. This time it built the code, ran mutation checks and ran the dash- e2e scenarios locally. It then put each of the six earlier blockers back to the same reviewer, one at a time, together with the author's round reply. Those verdicts are anchored evidence from reading the code, not blind findings.

Earlier blockers, settled:

• dash-27 SIGTERMs the e2e harness: resolved. The record now points at a disposable sleep child, KillScope::Tree cannot reach the harness from there, and dash-25 to dash-29 passed on the WSL2 lane at this head. The second half of the earlier fix still holds: nothing asserts that the child process is gone. See Blocking. [standing, narrowed]
• The probe counts a crashed service as serving: the trait doc is now honest, and the probe-only path hedges with "may still be running". Still holds on the launcher path. See Blocking. [standing, narrowed]
• The Windows prompt shows no model name or stop command: resolved in substance. On the probe-only path the prompt now lists each managed id with its real stop command, a unit test covers it, and the README and manual-testing docs carry the Windows caveat. Leftovers, none of them gating:
  • "A model was serving when this started." is still at ui/mod.rs:282. It is reachable only through a race between two registry reads.
  • The PR description still claims the model name and the s jump with no Windows caveat. That mismatch is in the description only.
  • The first commit message makes the same unconditional claim, and the second commit message corrects it.
• The wiring test cannot fail: resolved. The comments now say what the unit test covers. A mutation run confirmed that dash-29 catches the call site passing false.
• The managed-instance bullet's model name is not asserted: resolved. ui/mod.rs:1329-1333 asserts the exact line, and the fixture's name differs from its id.
• "Fixes #145" is false on Windows when dash or chat is opened directly: resolved as far as the code shows. On Windows both rocm dash and rocm chat now reach the prompt and print the stop command. No scenario exercises that on Windows; see the Windows scenario item below.

Driver notes on gating:

• CI: the red E2E tests (Strix Halo, WSL2) failure does not gate here.
  • Its one unexpected failure is comfyui-04, which timed out waiting for a download-progress frame. This change touches no ComfyUI or install code.
  • dash-01 to dash-29 all passed on that lane, and the same lane passed on recent main merge-queue runs, for example run 38368744433.
  • Re-running the job should clear it.
• Not escalated from earlier rounds:
  • The launcher → Chat route and Ctrl-C-while-serving were non-blocking at 3940e91. They are not escalated here.
  • Under the "quit paths not covered" item, the parts that gate are the new ones: the Esc-menu Quit arm with nothing serving, the s row selection, and the launcher prompt's Esc and q keys.
  • The popup clipping at three or more managed models stays non-blocking, as before. The test that cannot fail is new and gates.
• Description-only items: the Windows-scenario gap and the description's coverage wording do not gate on their own. Fixing the commit messages and the PR text together would clear them.

Review — needs work

Full review of the whole change.
Implements #145 (the confirm-before-quit prompt). The description and commit messages claim more test coverage than exists, and the first commit message claims Ctrl-C is gated when it is not.

Blocking

• [new] The first commit message says Ctrl-C now opens the prompt, but Ctrl-C still quits without asking — commit 3940e91 message; crates/rocm-dash-tui/src/app/event_loop.rs:1104 against :1166.
  • The message says: "Pressing q (or Ctrl-C with no job console open, /quit, /exit, or the Esc-menu Quit row) ... Every quit entry point ... now routes through one shared gate ... that opens a confirm prompt instead".
  • The code says otherwise. The ctrl_c_should_exit arm at :1104 runs before the quit_confirm_pending arm, and Ctrl-C never reaches request_quit.
  • The code governs. The README and the PR body both say Ctrl-C is not gated.
  • rocm-cli builds the squash commit from the commit messages, so this claim would land on main.
  • Confidence 85 · logic · Fix: reword the commit message to drop Ctrl-C from the gated list.
• [new] Nothing tests that the probe re-reads the registry, which is the main thing this change claims — apps/rocm/src/dash.rs:344-358 (LiveManagedRegistryProbe), and the six resolved_args_* tests in dash.rs.
  • Every test writes its records before resolved_args and never changes them. A probe that cached its first read would pass all of them.
  • The e2e scenarios don't catch it either. dash-25 and dash-27 wait for the daemon snapshot, after which has_live_instance stops consulting the probe. managed_ids is unchanged from launch in every scenario.
  • The PR body says "the live registry read [is] exercised directly", and it names a model started mid-session as one of the two gaps this closes. No test exercises that case.
  • Confidence 85 · logic · Fix: build the args against an empty services dir, then write a running record and assert snapshot().has_live_service flips to true; remove it and assert it flips back.
• [new; the launcher → Chat part was non-blocking at 3940e91 and is not escalated] Some quit paths this change rewires are not covered by any test, although the description says they are — apps/rocm/src/dash.rs (LauncherRoute::Chat => run_chat(.., true)); crates/rocm-dash-tui/src/app/actions.rs:294; crates/rocm-dash-tui/src/app/mod.rs:794-799; crates/rocm-dash-tui/src/ui/launcher.rs (handle_launcher_key's None arm).
  • Launcher → Chat route. A worker flipped launcher_hosted to false for both launcher routes and ran the tests: all unit tests stayed green. dash-29 catches the Dashboard route only. Nothing opens chat from the launcher, yet the body says "a dashboard or chat session opened from inside the launcher no longer asks twice".
  • Esc-menu Quit with nothing serving. The arm changed from return true to return request_quit(state). Both new menu tests seed a Running instance, and no test before or after activates the Quit row with nothing serving. The PR body says the q key, the Esc-menu Quit row and /quit "all still exit immediately with nothing serving".
  • s key row selection. The .is_some_and(|i| i.status.is_serving()) check in the position closure can become .is_some() and every test stays green. No test has a stopped managed row sorting before a serving one.
  • Launcher prompt keys. launcher_quit_confirm_approve_exits_deny_and_cancel_stay sends only n and y. It never sends Esc or q (the Cancel its name claims), and never sends a non-verdict key while the prompt is open.
  • Confidence 100 for the launcher → Chat result (a worker ran it); 85 for the rest (traced) · logic · Fix: add a launcher → chat → /quit scenario (or unit-test the route → launcher_hosted mapping); an Esc-menu Quit test with no instances asserting true; a stopped-then-serving managed-row case; Esc, q and d-while-open cases for the launcher.
• [new] Unit tests pin the stop-command wording without pointing at the test that proves the command works (AGENTS.md §3) — crates/rocm-dash-tui/src/ui/mod.rs quit_confirm_body_prints_a_working_stop_command_for_a_managed_instance and quit_confirm_body_prints_the_stop_command_during_the_startup_fallback_when_the_registry_names_an_id.
  • §3 says: "Where the text genuinely has to be pinned on its own, the assertion carries a comment naming the test that proves the behavior."
  • dash-27 proves the behaviour, but neither test names it. The first test's name says the command is "working", which a string match cannot show.
  • Confidence 85 · mechanical · Fix: rename it to "names the real stop command" and point both tests at @id:dash-quit-prompt-stop-command-actually-works.
• [new; the clipping half was non-blocking at 3940e91 and is not escalated] The narrow-width popup test cannot fail on what its comment says it checks, and real ids clip the prompt's last lines — crates/rocm-dash-tui/src/ui/mod.rs:1437-1489 (the test); quit_confirm_body, rendered through approval.rs centered_rect(72, 70, 96, 24, ..).
  • The comment says the test checks that every id's text still appears. But the fixture sets model_name: format!("model-{id}"), so the id appears in the bullet line even if every rocm services stop … --yes line is dropped.
  • Its ids are UUID-shaped, not the <engine>-<slug>-<millis> format the comment cites from generate_service_id.
  • It uses only 2 instances. At 80x24 the popup has about 10 text rows, and each real-length managed id takes about 3 rows because the stop line wraps. With 3 or more services, the trailer ("It keeps running in the background after you quit.") and the s hint are cut off with no marker. That geometry is argued from the code and was not rendered (70).
  • Confidence 85 for the test (traced) · logic · Fix: give the fixture a model name that doesn't contain the id; assert the full stop command and the trailer; use real-format ids with 3 or more services. Then size or reorder the body so the trailer always shows.
• [new; does not gate on its own, see driver notes] No scenario covers the Windows prompt, and the description does not name that gap (AGENTS.md §3) — README.md:170-179; docs/manual-testing.md (new bullets); ui/mod.rs quit_confirm_body's total == 0 branch; dash-25 to dash-29 are all @requires-os:linux.
  • On Windows the prompt behaves differently: it lists ids without model names, uses the hedged "It may still be running…" trailer, and offers no s hint. All of that is user-visible and only unit-tested.
  • The body's "Not verified" line says the pty scenarios and the "GPU/Windows/WSL lanes … are exercised there". That reads as if the Windows lane runs these scenarios. It runs none of them.
  • Confidence 85 · mechanical · Fix: state in the PR text that the Windows prompt has no scenario and why (the pty harness is Linux-only), or add a Windows-runnable scenario.
• [new; does not gate, see driver notes] A required check fails on this head: "E2E tests (Strix Halo, WSL2)" — job 115169790782.
  • The log shows 180 scenarios, 176 passed and 4 failed. Three of the failures are expected-fail (xfail) rows.
  • The one unexpected failure is comfyui-04 (comfyui-source-download-shows-live-progress). It timed out after 30s while the screen still showed "Downloading uv (latest) from https://github.com/…" / "Installing Python 3.12 via uv...".
  • This change touches no ComfyUI or install code, and dash-01 to dash-29, including all five new scenarios, passed on that lane. So it is very likely a host or network timing failure, not this change; comparing against main settles it. "E2E tests (MI300X)" was still queued.
  • Confidence 100 (read from the log) · mechanical · Fix: rerun the job, or show the same failure on main.
• [standing, narrowed] dash-27 never checks that the stopped process is actually gone — tests/e2e-cucumber/tests/e2e/dash_steps.rs:810-818, tests/e2e-cucumber/tests/e2e.rs:594-600.
  • Earlier wording (3940e91), whose fix still applies in part: "plant dash-27's record with the PID of a disposable child the step spawns (for example sleep), then assert that child is gone."
  • The first half is done. The second is not.
  • The scenario's only check after the stop reads the services list --json status. E2eWorld::drop then kills and reaps the child without checking anything.
  • So a rocm services stop that rewrote the record to stopped without signalling anything would leave dash-27, "Running the quit prompt's printed stop command actually stops the model", green. AGENTS.md §3 asks that the named command "actually clear the condition".
  • Fix: before teardown, assert that the child has exited (try_wait() returns Some).
  • This verdict came from putting the earlier wording and the author's reply to the reviewer, so it is anchored rather than blind.
• [standing, narrowed] On the launcher path, a crashed service still gets the unhedged "It keeps running in the background after you quit." — apps/rocm/src/dash.rs:673-688 (launcher_serving_instances), crates/rocm-dash-tui/src/ui/launcher.rs:324, crates/rocm-dash-tui/src/ui/mod.rs:329, title at ui/mod.rs:209.
  • Resolved since 3940e91: the trait doc (registry_probe.rs:62-78) and the "known divergence" note no longer overclaim, and the probe-only branch hedges its trailer (ui/mod.rs:294).
  • Still holds:
    • The launcher fills instances from the raw on-disk status, with no liveness check.
    • For a crashed record still marked ready, bare rocm → q takes the total > 0 branch.
    • It then prints the unhedged outcome claim, labels the service "stop it first", and gives a stop command for a service rocm services list reports as stopped.
    • The title "Still serving — quit anyway?" is unhedged on every path, including the probe-only one whose trailer says "may still be running".
  • That is the AGENTS.md §3 outcome-claim shape the earlier objection named.
  • Neither branch of the earlier fix (a local PID check, or a hedged message) reaches the launcher path.
  • Fix: hedge the launcher's trailer and title the same way the probe-only path is hedged, or add the PID check.
  • Not settled: whether the Linux dashboard is also affected once the daemon's first snapshot lands. That depends on how the daemon marks an instance whose endpoint stopped answering, which was not read.
  • This verdict came from putting the earlier wording and the author's reply to the reviewer, so it is anchored rather than blind.

Non-blocking

• [new] The launcher decides from a snapshot that is stale while the user sits at the menu — apps/rocm/src/dash.rs:650 (launcher_serving_instances); ui/launcher.rs launcher_state (StaticManagedRegistrySnapshot).
  • The snapshot is rebuilt once per menu pass. The menu then blocks on event::read() with no refresh.
  • So a model started from another terminal, or a record that goes from starting to ready while the menu is up, can be followed by q with no prompt.
  • In the reverse case, the prompt says "Still serving" and prints a stop command for a service that already stopped.
  • The comment saying the snapshot is "exactly as live as the launcher itself ever is" (registry_probe.rs:89-94) overclaims.
  • Confidence 85 · logic · Fix: pass the live probe into run_launcher.
• [standing, non-blocking since 3940e91] The title and body read the registry separately, so the "can't contradict" claim is false — ui/mod.rs quit_confirm_request, quit_confirm_body, nothing_is_currently_being_served.
  • On Windows, or before the first snapshot, each frame reads the registry up to 4 times. Frames are drawn at least every 250 ms tick.
  • A registry write between two of those reads can still produce "Still serving" over "Nothing is being served anymore.", the exact contradiction the doc comment says is impossible.
  • Confidence 70 · architectural · Fix: take one ManagedRegistrySnapshot per frame and pass it to the title, body and hint.
• [standing, non-blocking since 3940e91] A failed registry read means "nothing is serving", so q exits silently — dash.rs:349-357; registry_probe.rs (snapshot() cannot return an error).
  • load_service_records returns an empty list on a read_dir error and skips unreadable files. That behaviour is pre-existing, but the quit gate now depends on it, and on Windows the probe is its only input.
  • Confidence 70 · architectural · Fix: let the snapshot carry "unknown" and show the hedged prompt, or document the fail-open choice.
• [new] Several comments contradict the code; the code governs in each case:
  • dash.rs:265-270: "shared by the three derived fields below", but discovery has one consumer (:305).
  • app/mod.rs:670-671: "when none was injected (demo/replay/mock)", but the field doc at :204-205 and resolved_args say the probe is always Some.
  • app/mod.rs:1247-1248: the sibling case is "below", but it is above.
  • app/types.rs:42-46 and actions.rs:72-74: call the launcher's read "live", but it is static.
  • ui/launcher.rs:307: "every managed-service record regardless of status", but discover_managed_services drops failed, stopped and port-0 records.
  • tui_driver.rs:672 (quit_and_wait): still says "nothing is resent", but the loop now sends y.
  • More generally, many new comments narrate review history ("used to claim…", "Regression guard…") and run to 15–25 lines. That makes them expensive to keep accurate, which is how the drift above happened.
  • Confidence 85 · mechanical.
• [new] The quit prompt contradicts docs/ux-guidelines.md:101-105. The guideline says Quit cards should "return the user to the same row they were on" and that letter shortcuts "must not be required". This change closes the open modal on decline, as docs/manual-testing.md now documents, and makes s the only way to reach the stop action. Nothing says which document governs. Confidence 70 · logic · Fix: restore the previous modal and add a selectable Stop choice, or update the guideline with the reason.
• [standing, non-blocking since 3940e91] No scenario tests the new README promise that Ctrl-C "always quits immediately without asking" while a model is serving. dash-20 runs --demo, dash-21 has no model. §3 lists "will not happen" claims as needing the behaviour asserted. Confidence 70 · mechanical.
• [new] An approval that arrives during the quit prompt is dropped, and its message is not asserted or actionable — app/mod.rs:1255-1260.
  • The model has already been told the action was surfaced for approval.
  • The error turn gives no way to recover (e.g. "ask again"), and approval_request_is_discarded_while_quit_confirm_is_pending checks only role == Error.
  • Confidence 70 · logic.
• [new] dash-27 does not check that the dashboard stops asking after the stop command runs; one step exposes harness plumbing.
  • After running the printed command it checks the registry but never that the dashboard's own prompt clears. A worker's temporary probe found that it does clear once the instance leaves, so this is assertable.
  • "the managed model's record points at a disposable process" is harness detail and belongs inside a Given.
  • Confidence 70 · mechanical.

Decisions for the author

• Launcher-hosted sessions skip the prompt — tradeoff. One prompt instead of two, at the cost of the s-to-stop shortcut, which only the dashboard's own copy offers. The design is sound because every launcher-hosted exit returns to a registry re-read.
• Two independent Options that each own the screen — non-blocking-improvement. approval and quit_confirm are kept apart only by convention, and resolve_quit_confirm_key returns an Option<bool> tri-state. A single ScreenGate enum plus a QuitVerdict type would make the bad states impossible to represent.
• The root cause on Windows is that the dashboard has no live instance feed at all — tradeoff. The cfg(windows) client stub means the Serving tab and Services overlay are empty there too. This change patches only the quit gate. That is a reasonable scope, but it is worth saying explicitly.

Positive signals

• dash-27 runs the printed rocm services stop <id> --yes and checks the registry afterwards. That is §3 done properly: the remediation advice is proven, not just its wording.
• The prompt reuses ui/approval.rs instead of adding new modal UI, and the registry seam sits in its own file without breaking the crate-layering rules.
• The new auto-y in quit_and_wait does not hide an unwanted prompt. A worker made the prompt always open and ran the e2e suite: 12 of 20 non-serving scenarios failed.

Deployment notes

• User-visible behaviour change outside the quit prompt: while a chat tool-call approval is pending, tab-bar and footer-chip clicks are now swallowed. The PR body mentions this.

What this covered

• Every file in prw-base...HEAD (merge-base 213c2cb…6b7ac3cd), read completely, plus the callers and callees of the changed symbols (registry.rs, the ServicesCommand::Stop dispatch, the event_loop key and mouse routing, approval.rs).
• Five parallel workers grouped by coupling plus one design worker. The mutation checks and the e2e run (-n dash- on the PR head: 28 of 29 passed; dash-24 failed only because the name filter bypasses its @requires-wsl/@requires-gpu gating) were done by one worker in this checkout.
• History and prior-review passes ran against earlier merged PRs that touched these files.
• Not run:
  • No Windows build or Windows-target check.
  • No e2e run of the Windows or WSL GPU lanes beyond reading the WSL2 job log.
  • Three agent::clients unit tests were skipped because they fail in this environment with a permission error, whether or not anything is mutated.
• The review itself was not reconciled against any PR discussion. The six earlier blockers were settled afterwards, outside the review, as described at the top.

[jussielo-amd]

Thanks for the thorough pass. Responding against the new commit, 45c597f7 (on top of the reworded 447707f1).

Blocking — all addressed

• Ctrl-C commit-message overclaim: fixed. 3940e91c's message (now 447707f1 after a reword) dropped Ctrl-C from the "routes through the shared gate" list and added an explicit paragraph: Ctrl-C stays an unconditional, immediate quit, same established meaning it already has over the chat-approval modal (event_loop.rs's ctrl_c_should_exit arm is checked before quit_confirm_pending, unchanged). The PR body already said this correctly; only the commit message was wrong.
• Nothing tests that the probe re-reads the registry: fixed. New test live_registry_probe_reflects_a_record_written_after_resolved_args_was_built (apps/rocm/src/dash.rs) builds ResolvedArgs against an empty services dir, writes a running record after, asserts snapshot().has_live_service flips to true without rebuilding args, removes it, asserts it flips back.
• Launcher → Chat route untested: addressed structurally rather than with an e2e scenario. launcher_route_hosts_session(&route) now derives launcher_hosted for both the Chat and Dashboard escalation arms from the route itself, with a unit test pinning both — a wrong literal at either call site is now a single source of truth instead of two independent trues, per AGENTS.md §3's "derive it from the same value the branch is taken on" preference. I didn't add a launcher→chat→/quit e2e scenario on top of that; happy to if you'd still like one.
• Esc-menu Quit with nothing serving untested: fixed, quitting_from_the_esc_menu_exits_immediately_when_nothing_is_serving (app/actions.rs).
• s-key row selection guard untested (the .is_some_and(is_serving()) → .is_some() mutation): fixed, quit_confirm_open_services_key_skips_a_non_serving_row_that_sorts_first (app/mod.rs) — a non-serving row sorting first by id, a serving one second; proves position must actually skip row 0.
• Launcher prompt keys (Esc/q as Cancel untested; a genuine non-verdict key like d untested): fixed with two tests — launcher_quit_confirm_esc_and_q_also_cancel and launcher_quit_confirm_swallows_a_non_verdict_key_while_open.
• Unit tests pin the stop-command wording without pointing at the proof (AGENTS.md §3): fixed. Renamed quit_confirm_body_prints_a_working_stop_command_for_a_managed_instance → quit_confirm_body_names_the_real_stop_command_for_a_managed_instance (dropped the unprovable "working" claim from the name) and added a doc comment on both it and the startup-fallback sibling pointing at dash-27 as the test that actually proves the command works.
• Narrow-width popup test vacuous (fixture's model_name embedded the id): fixed. Model names no longer embed the id ("Model A"/"Model B"/"Model C"), ids are now in the real <engine>-<slug>-<millis> format (not UUID-shaped), and the test uses 3 instances. Left the full-command-string assertion out deliberately — at this width a real id's stop-command line can legitimately wrap, which is the clipping behavior you separately flagged as not escalated this round; asserting the full line would make the test depend on that unresolved layout question. The bare-id check is now non-vacuous on its own since the model name can no longer manufacture it.
• No scenario covers the Windows prompt, description doesn't name the gap: addressed via description, not a new scenario — the pty harness driving dash.feature is Linux-only, so a Windows scenario needs a different harness, not an added one. PR body's "Not verified" section now says explicitly that dash-25–dash-29 are all @requires-os:linux and none of them exercise the Windows-specific total == 0 path, rather than reading as if the Windows lane covers them.
• E2E tests (Strix Halo, WSL2) red: agreed it doesn't gate here — comfyui-04 (issue e2e: comfyui-04 times out when managed Python bootstraps inside its 30s spinner budget #580), unrelated. Re-running should clear it; happy to ask a maintainer to re-run if I can't trigger it myself.
• dash-27 never checks the process actually exited: fixed. New step "the disposable process has exited" (tests/e2e-cucumber/tests/e2e/dash_steps.rs), polling try_wait() on the disposable child, added after the registry-status assertion in the scenario.
• Launcher path: crashed service gets the unhedged "keeps running" claim: fixed. quit_confirm_body's total > 0 branch now picks the trailer by offer_stop_action (already true only on the dashboard's own call site, false only on the launcher's) — the launcher gets the same "may still be running" hedge the dashboard's own pre-snapshot fallback already uses for the equivalent no-verified-liveness case, instead of a present-tense claim the raw on-disk read can't back up. No PID check added — same reasoning services_past_attempts/LiveManagedRegistryProbe already document for not duplicating that classification.

Non-blocking

• Stale/contradicting comments: fixed the ones you listed — dash.rs's discovery consumer count, app/mod.rs's "when none was injected" overclaim and the has_stoppable_managed_instance reference (renamed to _with), app/types.rs/actions.rs calling the launcher's one-shot-per-pass read "live", ui/launcher.rs's "regardless of status" claim, and tui_driver.rs's stale "nothing is resent" comment.
• Launcher menu-loop stale snapshot (no live refresh while idle at the menu): left open per your own note — not escalated this round.
• Failed-registry-read fail-open behavior: left as documented behavior, not redesigned — agreed this is architectural, out of scope here.
• ux-guidelines.md letter-shortcut conflict: leaving this for you/a maintainer to weigh in on — it's a real tension between two documents, not something I think I should resolve unilaterally by changing either the modal or the guideline.
• Dropped approval during quit, message not actionable: left open, agreed non-blocking.
• dash-27's "dashboard stops asking" / Given-step wording nit: left open this round to keep the round bounded; happy to take as a quick follow-up.

Verification

cargo test --workspace --all-targets (0 failures, 33 binaries), cargo clippy --workspace --all-targets -- -D warnings (clean), cargo fmt --all --check (clean), cargo xtask check-crate-edges (clean), and cargo xtask e2e -- -n "dash-" — 28/29 pass; the one failure (dash-24, WSL GPU telemetry) reproduces for lack of a real GPU in this sandbox and is unrelated to this change (same conclusion as your own driver notes on the CI failure).

issues addressed

[jussielo-amd]

Addressed round-8's gating and reported findings in cf4d9eb, re-confirmed against the current tree before fixing (not from the review's own evidence), with a follow-up correction in 1f76c5a after a second independent review pass:

1. draw_focused's quit-confirm arm was unreachable (gating) — confirmed: open_quit_confirm's close_overlays call makes has_open_overlay() false immediately, so a focused host reaching request_quit would have hit focused_should_exit and exited before the next frame ever drew the prompt. AppState::focused_should_exit now also checks quit_confirm_pending(), so the gate stays shut until the prompt resolves. Added focused_should_exit_stays_shut_while_quit_confirm_is_pending (app::event_loop tests), which fails if the new clause is reverted. Corrected the three doc comments (ui::draw_focused, actions::request_quit, the existing draw_focused_renders_the_quit_confirm_prompt_when_open test) that had claimed the arm alone was sufficient — it needed the gate's own exemption too.
2. Sort-order regression test could pass without the sort (reported, not gating) — confirmed by reproducing: commenting out the sort_by in quit_confirm_body_with and re-running quit_confirm_body_lists_services_sorted_by_container_id 6 times with only 3 seeded ids failed every time locally too, but 3 ids leaves real odds of a HashMap order matching sorted order by chance. Reseeded with 8 ids (8! = 40320 orderings); re-ran the same regression check (sort disabled, 6/6 runs) to confirm it now reliably fails without the sort, and passes with it restored.
3. New e2e coverage for the README's Ctrl-C promise — added dash-31 (dash.feature): a model serving, Ctrl-C pressed, asserts exit code 130 and terminal restoration, with no step for a quit-confirm prompt in between (press_ctrl_c_and_wait would time out if one intercepted the keystroke, rather than silently passing).
4. Non-blocking doc/comment findings — all fixed: registry_probe.rs's "including"/"excluding" flip for a Starting record; the three stale cross-references (wrong test name + retired "startup-race fix" framing in dash.rs, the false "test-only helper" claim in launcher.rs's launcher_state — run_launcher calls it in production, and event_loop.rs's seed_registry_probe_and_hosting doc wrongly naming run_async as the caller, rather than this crate's own event_loop); the development-history framing in four comments (actions.rs, ui/mod.rs ×2, dash.rs, dash_steps.rs) rewritten to state the invariant each protects instead of the review round that found it; the README/manual-testing.md Windows sentence narrowed to the dashboard-opened-directly case (the launcher's own copy of the prompt always names the model, since it reads the registry directly rather than through the dashboard's daemon connection); and request_quit's doc now names the Ctrl-C exception.
5. Follow-up (1f76c5a) — a second independent review pass (different model, fresh context) on the committed round-8 diff caught that my own fix to the event_loop.rs/seed_registry_probe_and_hosting doc had corrected one of two paired sentences naming the call site, but left the other saying run, which doesn't call it either (run only calls event_loop, which has the one real call site at line 828). Both sentences now say event_loop.

Left as-is, per the review's own framing — no change intended:

• The two tradeoffs (starting-but-not-yet-serving models aren't covered by the gate; the registry is read synchronously on every render while the prompt is open) — both already weighed in the review itself, nothing new to decide.
• The Windows quit prompt having no e2e scenario — still accepted for the same reason given earlier: the pty harness driving dash.feature has no native-Windows-console equivalent, so that path stays unit-tested only.

Verification: cargo test -p rocm-dash-tui --lib (871 passed), cargo test -p rocm --bin rocm dash:: (35 passed), cargo clippy -p rocm-dash-tui -p rocm --all-targets -- -D warnings (clean), cargo fmt --check (clean), and cargo xtask e2e -- -n "dash-07|dash-10|dash-2[5-9]|dash-30|dash-31" (9 scenarios / 94 steps, all passed, including the new dash-31). Two independent fresh-context review passes (different model from the one that wrote the fixes, each with its own sub-agent fan-out) ran against this work before and after commit; both came back with nothing blocking.

[rominf]

🔴 Automated review · pr-review-watcher · 1f76c5a

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

Review — needs work

Full review of the whole change.
Implements issue #145 ("Stop serving a model"). It warns before quitting, prints the stop command and adds the s jump to Services. It does not add a stop action to quit itself. "Fixes #145" is a fair claim for that request.

Round 9 status: nothing gates this round, so the standing change requests are lifted. Both standing objections from round 8 are settled (see the end of this report). The review reported three new findings as blocking. All three are test-strength gaps, and none of them gates. The production behaviour each one names is correct today:

• q with nothing serving exits at once. The demonstrated regression is caught by the unit test has_live_instance_is_always_false_under_simulated.
• Ctrl-C's arm runs ahead of the quit-confirm arm.
• dash-28's decline is checked by the step before the vacuous one.

Merging therefore leaves weaker tests behind but no defect, which is below the floor for a merge gate. Please still fix them.

Blocking

• [new · reported, not gating] The e2e quit helper now presses y through any quit-confirm prompt, so existing scenarios no longer prove that q exits without asking when nothing is serving — tests/e2e-cucumber/tests/e2e/tui_driver.rs (quit_and_wait: let confirming = …contains(QUIT_CONFIRM_MARKER); self.send(if confirming { "y" } else { gesture }))
  Every pre-existing "the user quits the dashboard/chat/launcher" step now accepts a prompt it never expected. The test was a mutation: remove the simulated short-circuit in has_live_instance_with, so that --demo falsely reports a serving model. dash-01 and dash-05 stayed green, and a log line in quit_and_wait showed the false "May still be serving — quit anyway?" prompt opening and being answered y. docs/manual-testing.md says that with nothing being served, q exits immediately with no prompt. No scenario guards that anymore; only the unit test has_live_instance_is_always_false_under_simulated does. AGENTS.md §8 treats weakening a test as a change in its own right.
  Confidence 100 · logic · Fix: press y only when the scenario opted in (for example a world flag set by "a running managed model is available locally"), and fail fast if the prompt appears otherwise. Or add a scenario where q with nothing serving (or under --demo) exits on the first keystroke and the prompt never appears.

• [new · reported, not gating] Nothing tests that Ctrl-C quits while the quit-confirm prompt is open — crates/rocm-dash-tui/src/app/event_loop.rs:1105 (Ctrl-C arm) vs :1167 (quit-confirm arm)
  This behaviour depends only on match-arm order. One mutation moved the quit-confirm arm above the Ctrl-C arm. The full rocm-dash-tui --lib suite still passed (868 passed; the 3 failures are the known agent::clients::* ones). dash-31 presses Ctrl-C only while the prompt is closed. The change claims this behaviour in three places:

  • the request_quit doc comment;
  • commit 9c38d9d ("remains an unconditional, immediate quit … even while either prompt is open");
  • the description ("exits unconditionally above every overlay").

  AGENTS.md §3 requires a promise like this to be exercised by a test.
  Confidence 85 · logic · Fix: extend dash-31 (or add a scenario) to press q, wait for the prompt, press Ctrl-C and expect exit 130. Alternatively, pull arm selection into a pure function and unit-test it.

• [new · reported, not gating] One dash-28 step cannot fail — tests/e2e-cucumber/features/dash.feature dash-28, When the user declines the quit prompt / Then the managed model is still listed as running
  The record's pid is the harness itself, and nothing on the decline path touches services. That Then holds even if declining did nothing at all. The step that actually tests the decline is the prompt-closed wait inside the When.
  Confidence 85 · mechanical · Fix: drop this Then, or replace it with an assertion that chat is still interactive.

Non-blocking

• [new] The has_stoppable_managed_instance_with short-circuit in quit_confirm_open_services_key does nothing, and its doc says it prevents drift — crates/rocm-dash-tui/src/app/mod.rs:800-823
  When the short-circuit was removed, every lib test still passed. The position closure re-checks the same managed_ids.contains && is_serving condition over the same instances, so the result is identical either way.
  Confidence 100 · mechanical · Fix: remove it and its doc paragraph, or describe it as an early exit with no behaviour of its own.

• [new] Comments describe this PR's own earlier design as if it had been on main — crates/rocm-dash-tui/src/registry_probe.rs:13-14, crates/rocm-dash-tui/src/ui/mod.rs (quit_confirm_body_with's debug_assert! comment)
  The module doc says the probe "replaces the one-shot, launch-time registry read ResolvedArgs used to carry (startup_has_live_service / managed_service_ids)". Neither field exists anywhere on prw-base (git grep is empty). Commit 9c38d9d added these strings only in that doc. ui/mod.rs also refers to "this PR's retired launch-time snapshot". A reader on main will look for code that never existed.
  Confidence 85 · mechanical · Fix: describe what the probe does, not what it replaced inside this PR.

• [new] On Windows and during startup, the prompt lists models that are still starting under "stop it first" — crates/rocm-dash-tui/src/ui/mod.rs (quit_confirm_body_with, total == 0 branch); apps/rocm/src/dash.rs (LiveManagedRegistryProbe::snapshot: managed_ids: discovery.seen)
  discovery.seen includes starting/recovering records (is_scrapeable_status). has_live_service is filtered by is_serving(), but managed_ids is not. Once one record is serving, starting records are listed beside it and inflate the overflow count. The stop command still works for them, so the impact is wording only.
  Confidence 70 · logic · Fix: add a separate serving_ids to the snapshot, or filter in the probe.

• [new] The ManagedRegistryProbe doc overstates how often it is called — crates/rocm-dash-tui/src/registry_probe.rs:62-67
  It says the gate may call snapshot() "on every render … for the entire life of the session" on Windows. In the dashboard it is called only from request_quit, and from quit_confirm_request while the prompt is open. Per-frame calls happen only in the launcher, which uses the static probe.
  Confidence 70 · mechanical · Fix: state the actual call sites.

• [new] The dash-31 comment cites the wrong README section — tests/e2e-cucumber/features/dash.feature (dash-31 comment: README's "Interactive interfaces" section)
  The Ctrl-C text it refers to is under README's "First run" section. app/actions.rs cites it correctly.
  Confidence 85 · mechanical · Fix: correct the section name.

• [new] The squash-commit body will carry an AI co-author trailer and review-round history — commits cf4d9eb, dd4579b, 4ec28b7, ad2c4b4, 1f76c5a
  rocm-cli builds the squash body from these commit messages:

  • cf4d9eb ends with Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>. AGENTS.md §11 says "avoid AI-generated boilerplate footers".
  • Several messages narrate review rounds ("close round-8 review gaps", "rominf's automated review (CHANGES_REQUESTED at 86ab905)", "Closes rominf's round-5 review").
  • 1f76c5a is a doc fix-up of the previous commit.

  None of that describes the change to a reader of main.
  Confidence 85 · mechanical · Fix: rewrite the squash message at merge so it states the feature, and drop the trailer.

• [new] The description's test plan omits dash-31 — PR description, "Test plan" / "Not verified"
  Both sections list dash-25–dash-30. dash-31 (Ctrl-C while serving), added in cf4d9eb, is not mentioned.
  Confidence 85 · mechanical · Fix: add dash-31 to the description.

Decisions for the author

• The dashboard trusts only the daemon once its first snapshot lands, even when that daemon may not read this host's registry — non-blocking-improvement
  has_live_instance_with stops consulting the live registry read once has_received_snapshot is set. With --connect to a daemon that does not discover the local services_dir, a model this host serves would not open the prompt. The local managed_ids would also be matched against another daemon's container_ids. The probe is a live read, so OR-ing it in permanently would still track a stop. This was reasoned from the code but not run; confirm whether --connect to such a daemon is a supported case.
• The quit prompt reuses the tool-approval chrome — tradeoff
  Reuse keeps one modal path with a shared key policy. The cost is that the user sees "Review: … quit anyway?" with "Approve (y) / Deny (n)" buttons, and q inside the prompt means Cancel. Pressing q twice therefore keeps the dashboard open. Confirm that this is acceptable, or let ApprovalRequest carry its own button labels.
• The launcher's prompt is only as fresh as the last menu redraw — tradeoff
  run_launcher reads the registry once per menu pass and wraps it in StaticManagedRegistrySnapshot. A stop or start from another terminal while the menu sits open is not seen. The "may still be serving" wording softens this. docs/manual-testing.md's "a fresh, on-disk read" says more than that.

Positive signals

• Every quit entry point (q, the Esc-menu Quit row, the footer q chip, /quit//exit, the launcher's q/Esc) goes through one request_quit. A grep found no should_quit = true that bypasses it.
• open_approval refuses while the quit prompt is open. Without that, a y meant to confirm the quit could approve a chat tool call instead.
• launcher_route_hosts_session derives the hosted flag from the route. Mutating it to false failed dash-29 and dash-30.
• dash-27 runs the printed rocm services stop <id> --yes against a disposable process that carries real supervisor_start_ticks. It asserts both the registry state and that the process exited, which is the §3 shape done properly.
• The title, body and s hint are built from one registry snapshot (quit_confirm_request), so the parts of the prompt cannot disagree.

Deployment notes

None

What this covered

Read against prw-base…1f76c5a6. Every changed file was reviewed in two coupled groups by independent workers:

• rocm-dash-tui app/ui/probe;
• apps/rocm dash/main, the e2e harness, features, README and manual-testing.

The dash-tui worker read every diff hunk and the surrounding code for the feature. It did not read the unrelated remainder of the largest files (app/mod.rs, about 3,960 lines, and ui/tabs/instances.rs, about 2,280). Commit messages, the PR description and issue #145 were also read.

Evidence runs:

• 21 single-branch mutations against cargo test -p rocm-dash-tui --lib (baseline 868 passed / 3 known failures). The mutations that survived are cited above.
• dash-25..31 run locally: 7 scenarios, 83 steps, all passed.
• feature_naming: 4/4 passed; no @id/index collisions against prw-base.
• e2e mutations: the route mutation was caught; the simulated-gate mutation was hidden by quit_and_wait.

CI rollup for the head is green on every reported check (skillscope skipped). No self-hosted GPU, Strix or WSL lane appears in the rollup.

Did not run:

• The prior-changes pass (comments on earlier PRs touching these files): reading review discussion is outside this run.
• Design review (Step 7) as independent workers: the session's two-worker cap was spent on the code passes, so the design questions were judged in the driver's own context and are not independent.

Not reconciled against any discussion on the PR.

Standing objections from round 8 — settled

The blind review above re-found neither objection. After its report was frozen, each was put to the reviewer together with the author's reply of 07:36 UTC. That check is anchored: it was given the objection to judge. Both kinds of evidence agree, so both objections are dropped:

• [gating] The draw_focused quit-prompt arm could never be drawn — resolved. focused_should_exit now also checks !self.quit_confirm_pending() (app/mod.rs:898). The reviewer removed that clause, and focused_should_exit_stays_shut_while_quit_confirm_is_pending failed. With the clause restored it passes. Read against the final code, the sentence in commit 4ec28b7's message about an "invisible, key-eating prompt" is now true. In the squash body it sits next to cf4d9eb's message, which describes the gate fix.
  Observation: the arm is still reachable only by a future focused caller, because no production path opens the prompt with a focus set. The new test also pins that declining in a focused flow exits that flow, so for such a caller "declining returns to where you were" would not hold.
• [reported, not gating] The sort-order test caught its regression only about half the time — resolved. With the sort_by removed, the test failed 12 times out of 12; restored, it passed 12 times out of 12.
  Observation: the test's comment (ui/mod.rs:~1552) still narrates the earlier "9 runs passed 4" history instead of stating the invariant.

The reply on this pull request (issue comment 6187010008) reported fixes and asked no question. It was recorded with a 👍 reaction, which GitHub confirmed.

Round 9 at 1f76c5a: both standing objections are settled. The draw_focused quit-prompt arm is now reachable because focused_should_exit checks quit_confirm_pending, and a test pins that. The sort-order test now fails 12 out of 12 runs without the sort. Nothing new gates. See review 5530044935.

[tomastola]

Read this against #145 and the earlier rounds. I'm not re-raising anything those settled (Ctrl-C staying ungated, Fixes #145, the registry probe, the launcher_hosted tradeoff).

Every quit entry point I could find (q, the footer q chip, the Esc-menu Quit row, /quit//exit, the launcher's q/Esc) goes through request_quit, confirm/cancel behave as documented, and the terminal restore reuses the existing exits. I ran cargo test -p rocm-dash-tui (all targets) and cargo test -p rocm --bin rocm, plus clippy with -D warnings, all green on 848db9d, and I ran a few single-branch mutations. These five were caught by the unit tests: dropping the launcher_hosted early return, always OR-ing the registry read, /quit bypassing the gate, the Esc-menu row returning true, and the probe counting any record. Three wiring points survive the unit tier and are held only by the e2e scenarios, which is what the comments say: the event loop's Approve → break, launcher_hosted seeded as false, and dash::run dropping the hand-off.

Please fix (description only)

• The scenario ids in the description are off by one. The last commit renumbered them to dash-26..dash-33, but the Test plan and "Not verified" still say dash-25..dash-32. dash-25 on this branch is the unrelated "Adopt existing…" scenario, so the description points a reader at the wrong test. Please update the ids (dashboard = dash-26, launcher = dash-27, stop command = dash-28, /quit = dash-29, hosted dashboard = dash-30, hosted chat = dash-31, Ctrl-C = dash-32, nothing serving = dash-33).

Non-blocking

• starting/recovering records don't count as serving, so quitting mid-load gives no prompt, and the supervisor is detached, so the model still comes up afterwards. See the inline comment.
• A stop in flight still counts as serving. See the inline comment.
• Ctrl-C is a tradeoff to confirm (below).
• I'd like a scenario for the Esc-menu Quit row (AGENTS.md §3). See the inline comment.
• README paragraph is too heavy. See the inline comment.
• Squash message: 848db9d and 2abb72f carry Co-Authored-By: Claude … trailers and several messages narrate review rounds ("close round-8…"). AGENTS.md §11 says to avoid AI footers, so please rewrite the squash message at merge to state the feature.
• Still open from the earlier round: once the first snapshot lands, only instances decides, so --connect to a daemon that doesn't read this host's services_dir would not prompt for a model served here. Fine if that isn't a supported setup; a sentence saying so would do.

Ctrl-C and SIGTERM (crates/rocm-dash-tui/src/app/event_loop.rs:1106, tradeoff, not a change request)

Ctrl-C (and SIGTERM via await_termination) skip the prompt by design, and README and dash-32 say so, so I'm not asking to gate it. Just noting what it does to the PR's own goal, "won't happen by surprise": Ctrl-C is probably the most common way out of the dashboard. If you want to cover it without a prompt, a one-line notice on stderr after the terminal is restored, on any exit path where the probe says something is serving, would do it. Your call.

Positive

• One request_quit gate for every key/menu/chip/slash/launcher path, with no stray should_quit = true.
• open_approval refuses while the prompt is open, so a y meant for the quit can't approve a tool call.
• dash-32's second half (Ctrl-C with the prompt open) and dash-33 (q with nothing serving, bypassing quit_and_wait's auto-y) close the masking gaps from the last round.

[tomastola]

Non-blocking. Only ready/running count here, so a managed starting/recovering record never opens the prompt. But rocm serve --managed starts a detached supervisor (spawn_detached_no_inherit), so quitting mid-load leaves the model coming up after you've gone. That is the same surprise #145 describes, and it's the likeliest moment to quit: Serve wizard → q → launcher → q. The tests pin the exclusion on purpose (launcher_state_does_not_warn_on_a_starting_only_registry, resolved_args_reports_no_live_managed_service_when_none_are_ready_or_running), so I read it as a wording call (the prompt says "serving") rather than an oversight. Would you count managed starting records and say "starting" in the title/body? If you'd rather keep it narrow, say so in the description.

[jussielo-amd]

Agreed this is a real gap, and the likeliest moment to hit it, per your framing. Kept narrow for this round rather than widening the gate: quit_confirm_body_with only ever says "serving", so counting starting/recovering means deciding new wording too, and the two tests you cited pin the current exclusion on purpose. Documented as an accepted tradeoff in the description (new paragraph right before "Test plan") rather than silently left — happy to pick this up as a follow-up if you'd rather not carry it as a known gap.

[tomastola]

Non-blocking. A stop in flight still reads as serving. stop_internal_managed_service_confirmed_with writes stop_requested_unix_ms first, keeps the record ready/running while it terminates (its own comment says that can take around 40s), and only then flips the status. rocm-dash-daemon's ServiceRecord and the probe never look at that field. So s → stop → close the overlay → q inside that window asks "May still be serving — stop it first: rocm services stop " about something already stopping. The hedged wording keeps it true, but the remedy is already in flight. Traced from the code, not run. Treating a record with the marker set as not serving, or saying "stopping", would close it.

[jussielo-amd]

Confirmed — traced it the same way: stop_requested_unix_ms is set and the record stays ready/running for the duration, and neither rocm-dash-daemon::registry::ServiceRecord nor LiveManagedRegistryProbe look at that field, so the gate can't see it. Not closing it this round — it needs either a new field on ServiceRecord or a different way to read in-flight state, more surface than this round's fix warrants — but it's now written up as an accepted tradeoff in the description rather than left silent.

[jussielo-amd]

Responding to the rest of the review (replies on each inline thread cover those individually; commit d32b144):

Please fix — description scenario ids: fixed. The description now says dash-26 (dashboard), dash-27 (launcher), dash-28 (stop command), dash-29 (/quit), dash-30 (hosted dashboard), dash-31 (hosted chat), dash-32 (Ctrl-C), dash-33 (nothing serving), matching the current dash.feature — and now also dash-34 for the new Esc-menu scenario. While re-checking the description against the diff I also found and fixed two more stale claims: a sentence crediting both dash-26 and dash-27 with the "Press s" hint assertion when only dash-26 has it, and the "Not verified" paragraph's Linux-lane range (now dash-26–dash-34).

Squash message / AI footer: agreed, and noted for whoever does the squash at merge time — this branch's own commits will keep their normal history, but the squash commit body should state the feature and drop both the Co-Authored-By trailer and the review-round narration.

--connect / non-local-daemon gap: written up now — not as a sentence alone, but as a doc-comment paragraph on AppState::has_live_instance_with naming the actual mechanism (dashboard.tui.connect, there's no CLI flag) and the PR description's own new "Accepted tradeoff" paragraph.

Ctrl-C/SIGTERM stderr notice: noted, no change for now — agree it's the most common way out and that a one-line notice is a reasonable follow-up, but not folding a new exit-path behavior into this round.

[tomastola]

Re-reviewed the latest commit (the dash-34 Esc-menu Quit row scenario, the README rewrite, and the new doc comment on has_live_instance). The scenario drives the real Esc → menu → jj + Enter path and checks the registry afterwards; CI is green on this head. One small docs inline comment, nothing blocking.

Question for the author: the new "Known gap" comment on has_live_instance says that when dashboard.tui.connect points at a daemon that doesn't discover this host's services_dir, a locally served model never opens the prompt. That reads accurate against the code, but it isn't among the accepted tradeoffs listed in the PR description. Should it be listed there too, or is the code comment alone the intended home for it?

[tomastola]

The PR description deliberately hedges the prompt ("may still be serving") because nothing on this path re-verifies liveness, but this paragraph states it as fact. Suggest matching the prompt's wording here (the "keeps running in the background" a few lines below could be "may keep running" for the same reason):

Suggested change

	that job finishes, `Ctrl-C` quits there too. If a model is still being served,
	that job finishes, `Ctrl-C` quits there too. If a model may still be serving,