§1+§2: polish, harden, invariant re-proofs + ROADMAP update

ROADMAP.md

@@ -9,19 +9,46 @@ Bench / Chat), the no-key ChatGPT-OAuth chat backend, the unified `config.json`
 Windows-clean build, and `rocm dash --demo/--replay/--chat-mock`.
 
 This file captures (1) the remaining work to reach full **feature parity** with
-the two surfaces the merge replaces, and (2) the scoped **Per-process VRAM →
-model** dashboard item.
+the two surfaces the merge replaces, (2) the **background-helper (`rocm daemon`)
+wiring** regression, and (3) the scoped **Per-process VRAM → model** dashboard
+item.
+
+> **What shipped (Supergoal §1 + §2).** Items (1) and (2) are now done. The dash
+> Chat tab reached agentic parity (read-only ROCm tools, approval-gated mutating
+> tools, `/plan`, `/provider` + Anthropic, a 30-command parity map) and bare
+> `rocm`/`rocm chat` reroute to it; `tui.rs` is retained behind an explicit
+> retire gate (deletion not performed). `rocm daemon` runs the real loop
+> in-process via the `rocmd` lib with on-demand, double-spawn-guarded autostart.
+> Item (3) (per-model VRAM) remains the open dashboard follow-up.
 
 ---
 
 ## 1. Feature parity — retire `tui.rs` without regression
 
-**Status: the only real parity gap.** Bare `rocm` and `rocm chat` still launch
-the legacy chat-first assistant (`apps/rocm/src/tui.rs`); its retirement was
-**deferred** because the new dashboard **Chat** tab is a read-only telemetry
-chat and lacks the legacy assistant's agentic capabilities. Closing this gap is
-the prerequisite for deleting `tui.rs` and routing bare `rocm`/`rocm chat` to
-the unified TUI.
+**Status: parity REACHED; bare `rocm`/`rocm chat` REROUTED to the dash;
+deletion GATED (not performed).** The dash **Chat** tab now matches the legacy
+assistant's agentic capabilities, and bare `rocm` + interactive `rocm chat`
+route to the unified dash chat (`dash::run_chat`). `apps/rocm/src/tui.rs` is
+**retained** (anchored by `_RETAINED_TUI_ENTRY`) behind an explicit accept/retire
+gate — the actual file deletion is intentionally not done in this supergoal.
+
+**What shipped (Supergoal §1, Phases 3–9):**
+- Read-only ROCm tools through a `rocm-core`-free execution seam
+  (`tool_exec.rs` / `dash_seam.rs`): doctor, engines, services, logs, snapshots,
+  automations, path/port checks, update-check, `rocm_command`.
+- Mutating tools gated by the existing `ui/approval.rs` modal (`install_sdk`,
+  `install_engine`, `launch_server`, `stop_server`, `watcher_enable/disable`)
+  plus update/comfyui/uninstall/setup and automations review/approve/reject/edit.
+- Natural-language **`/plan`** (Ask → Plan → Review → Run).
+- **`/provider`** live backend switching + the **Anthropic** backend; the full
+  slash-command set.
+- A **30-command parity map** + accept/retire checklists:
+  [`docs/dash-parity-map.md`](docs/dash-parity-map.md),
+  [`docs/dash-parity-checklist.md`](docs/dash-parity-checklist.md),
+  [`docs/tui-retirement-checklist.md`](docs/tui-retirement-checklist.md).
+
+The original gap analysis and build order below are kept for historical context;
+all stages are now done except the **gated** Stage-8 deletion.
 
 **What the dash Chat tab is missing vs `tui.rs`:**
 - Mutating ROCm tool-calls with **in-chat approval** (`install_sdk`,
@@ -77,7 +104,67 @@ ureq/reqwest partition intact.
 
 ---
 
-## 2. Per-process VRAM → model attribution (rocm dash)
+## 2. Background-helper (`rocm daemon`) wiring — regression
+
+**Status: DONE.** `rocm daemon` now runs the real foreground loop **in-process**
+via the `rocmd` library (`rocm` → `rocmd` lib; acyclic), matching the "built into
+rocm" policy. The helper is **autostarted on demand**, detached and
+double-spawn-guarded (`ensure_background_helper_running` /
+`background_helper_already_running`), from both `automations enable` and
+`rocm serve --managed`. `render_daemon_text` is retained as the `--status` view
+only. The original regression analysis below is kept for historical context.
+
+**Status (historical): regression. The background helper is dormant.**
+`rocm daemon` is documented as *"Start the background helper in the foreground"*
+(`apps/rocm/src/main.rs:250`), but its handler only renders a status panel:
+
+```rust
+// apps/rocm/src/main.rs:1298
+Some(Command::Daemon) => {
+    print!("{}", render_daemon_text(&paths, &config));   // status only — no loop
+    Ok(())
+}
+```
+
+The real foreground loop, `run_daemon()`, exists **only in the separate `rocmd`
+binary** (`apps/rocmd/src/lib.rs:2808`, reached via `rocmd run`). But:
+
+- `apps/rocm` does **not** depend on `rocmd` (its `Cargo.toml` pulls
+  `rocm-core`, `rocm-dash-daemon`, `rocm-dash-tui` — not `rocmd`).
+- **Nothing in `apps/rocm` ever spawns `rocmd run` or `rocm daemon` as a running
+  loop.** The only `"daemon"` references are status renderers and tests.
+- `daemon_binary_path()` (`crates/rocm-core/src/lib.rs:5859`) resolves to the
+  **`rocm`** binary itself, and `main.rs:10666` states *"policy: built into
+  rocm; no separate rocmd binary is required"* — i.e. the intended design is
+  that `rocm daemon` **is** the helper loop, re-executing itself. That loop
+  logic is missing; it is stubbed to the status panel.
+
+**Effect:** automation checks and on-demand local model servers have no helper
+to run, regardless of the §1 chat-parity work. `run_daemon()` in `rocmd` is
+orphaned code the product never invokes.
+
+**Build order:**
+1. **Decide the model** — either (a) fold `rocmd`'s `run_daemon()` into the
+   `rocm` crate so `rocm daemon` runs the loop in-process (matches the
+   "built into rocm" policy), or (b) have `rocm daemon` spawn `rocmd run`, with
+   automations/managed-serve starting it on demand. (a) is the stated direction.
+2. **Wire the chosen entry** so `rocm daemon` actually starts the loop in the
+   foreground; keep `render_daemon_text` as the *status* view only.
+3. **On-demand start** — have automation-enable and `rocm serve --managed`
+   ensure the helper is running (spawn detached if not), then update
+   `AutomationRuntimeState` so the status panel reflects reality.
+4. **Reconcile `rocmd`** — once the loop lives in `rocm`, either remove the
+   orphaned `rocmd` binary or make it a thin alias; don't ship two daemons.
+5. **Verify** — `rocm daemon` runs and accepts work; automation checks fire;
+   status panel shows `running`; clean shutdown.
+
+**Invariants:** keep the user-owned unix socket (mode 0600) hardening from #17;
+no second listener/socket competing with the `rocm dash` telemetry daemon
+(`rocm-dash-daemon`) — these are distinct daemons and must stay distinct.
+
+---
+
+## 3. Per-process VRAM → model attribution (rocm dash)
 
 **Goal:** attribute each running model/serving instance's GPU VRAM by joining
 `amd-smi` per-process VRAM to the owning model/container — so the dashboard

apps/rocm/src/dash.rs

@@ -166,15 +166,15 @@ fn automation_summaries(config: &RocmCliConfig) -> Vec<AutomationSummary> {
 
 /// Resolve the TUI args from the unified config + environment.
 ///
-/// `anthropic_api_key` is resolved by the caller *before* any tokio runtime is
-/// entered: the secure-store fallback uses a blocking zbus client that spins its
-/// own runtime, which panics ("cannot start a runtime from within a runtime") if
-/// invoked from inside `run_async`. See [`anthropic_api_key_for_dash`].
+/// MUST be called on a synchronous thread *before* any tokio runtime is entered:
+/// the Anthropic-key secure-store fallback ([`anthropic_api_key_for_dash`]) uses
+/// a blocking zbus client that spins its own runtime, which panics ("cannot
+/// start a runtime from within a runtime") if invoked from inside `run_async`.
+/// The sync entry points `run`/`run_chat` call this and pass the result in.
 pub fn resolved_args(
     config: &RocmCliConfig,
     paths: &AppPaths,
     initial_tab: ActiveTab,
-    anthropic_api_key: Option<String>,
 ) -> ResolvedArgs {
     let t = &config.dashboard.tui;
     ResolvedArgs {
@@ -190,7 +190,7 @@ pub fn resolved_args(
             .ok()
             .filter(|v| !v.is_empty()),
         chat_api_key: chat_api_key_from_env(),
-        anthropic_api_key,
+        anthropic_api_key: anthropic_api_key_for_dash(),
         chat_auto_consent: false,
         chat_mock: false,
         model_recipes: model_recipe_summaries(),
@@ -202,6 +202,16 @@ pub fn resolved_args(
     }
 }
 
+/// Build the multi-thread tokio runtime the async daemon/TUI run on. Shared by
+/// the synchronous [`run`] and [`run_chat`] entry points (the rest of `rocm` is
+/// synchronous; only the dashboard needs an async reactor).
+fn build_dashboard_runtime() -> Result<tokio::runtime::Runtime> {
+    tokio::runtime::Builder::new_multi_thread()
+        .enable_all()
+        .build()
+        .context("building tokio runtime for the dashboard")
+}
+
 /// Entry point for `rocm dash`. Builds a tokio runtime and runs the dashboard.
 pub fn run(replay: Option<PathBuf>, demo: bool, chat_mock: bool) -> Result<()> {
     let paths = AppPaths::discover()?;
@@ -219,21 +229,15 @@ pub fn run(replay: Option<PathBuf>, demo: bool, chat_mock: bool) -> Result<()> {
     } else {
         replay
     };
-    // Resolve the Anthropic key before the runtime exists; the secure-store
-    // fallback blocks on its own zbus runtime and would panic inside `run_async`.
-    let anthropic_api_key = anthropic_api_key_for_dash();
-    let rt = tokio::runtime::Builder::new_multi_thread()
-        .enable_all()
-        .build()
-        .context("building tokio runtime for the dashboard")?;
-    rt.block_on(run_async(
-        config,
-        paths,
-        replay,
-        chat_mock,
-        ActiveTab::Overview,
-        anthropic_api_key,
-    ))
+    // Resolve TUI args — including the OS secure-store (keyring) lookup for the
+    // Anthropic key — on this plain synchronous thread, BEFORE entering the tokio
+    // runtime. The secure-store path (`provider_keys` → secret-service) uses
+    // `zbus::blocking`, which builds its own runtime and `block_on`s internally;
+    // doing that on a dash runtime worker thread panics with "Cannot start a
+    // runtime from within a runtime". See `run_async`.
+    let args = resolved_args(&config, &paths, ActiveTab::Overview);
+    let rt = build_dashboard_runtime()?;
+    rt.block_on(run_async(config, paths, args, replay, chat_mock))
 }
 
 /// Entry point for bare `rocm` and interactive `rocm chat`. Opens the unified
@@ -242,32 +246,23 @@ pub fn run(replay: Option<PathBuf>, demo: bool, chat_mock: bool) -> Result<()> {
 pub fn run_chat(chat_mock: bool) -> Result<()> {
     let paths = AppPaths::discover()?;
     let config = RocmCliConfig::load(&paths)?;
-    // Resolve the Anthropic key before the runtime exists; the secure-store
-    // fallback blocks on its own zbus runtime and would panic inside `run_async`.
-    let anthropic_api_key = anthropic_api_key_for_dash();
-    let rt = tokio::runtime::Builder::new_multi_thread()
-        .enable_all()
-        .build()
-        .context("building tokio runtime for the dashboard")?;
-    rt.block_on(run_async(
-        config,
-        paths,
-        None,
-        chat_mock,
-        ActiveTab::Chat,
-        anthropic_api_key,
-    ))
+    // See `run`: resolve args (incl. the keyring lookup) before the runtime so the
+    // secure-store `zbus::blocking` path never runs on a runtime worker thread.
+    let args = resolved_args(&config, &paths, ActiveTab::Chat);
+    let rt = build_dashboard_runtime()?;
+    rt.block_on(run_async(config, paths, args, None, chat_mock))
 }
 
 async fn run_async(
     config: RocmCliConfig,
     paths: AppPaths,
+    mut args: ResolvedArgs,
     replay: Option<PathBuf>,
     chat_mock: bool,
-    initial_tab: ActiveTab,
-    anthropic_api_key: Option<String>,
 ) -> Result<()> {
-    let mut args = resolved_args(&config, &paths, initial_tab, anthropic_api_key);
+    // `args` is built by the synchronous caller (`run`/`run_chat`) so the keyring
+    // lookup inside `resolved_args` never runs on a runtime worker thread (it uses
+    // `zbus::blocking`, which would otherwise panic: runtime-within-a-runtime).
     args.replay = replay.clone();
     args.chat_mock = chat_mock;
     // Inject the bin-side tool-execution seam for a live dash only. Demo/replay
@@ -375,7 +370,7 @@ mod tests {
     #[test]
     fn resolved_args_take_connect_and_theme_from_config() {
         let c = cfg();
-        let args = resolved_args(&c, &paths(), ActiveTab::Overview, None);
+        let args = resolved_args(&c, &paths(), ActiveTab::Overview);
         assert_eq!(args.connect, c.dashboard.tui.connect);
         assert_eq!(args.theme, c.dashboard.tui.theme);
         assert!(!args.chat_mock);

apps/rocm/src/main.rs

@@ -1347,6 +1347,13 @@ fn dispatch(cli: Cli) -> Result<()> {
                 // honored only on the non-interactive render path below. The
                 // legacy `tui::run` assistant is retained but no longer invoked
                 // here (see docs/tui-retirement-checklist.md).
+                if provider.is_some() {
+                    // --provider isn't threaded into the interactive dash; say so
+                    // instead of dropping it silently — the user can switch live.
+                    eprintln!(
+                        "note: launching the dash chat; switch providers with /provider <name>"
+                    );
+                }
                 return dash::run_chat(chat_mock);
             }
             match prompt {
@@ -4216,6 +4223,16 @@ fn start_managed_service(
     Ok(())
 }
 
+/// The "should we spawn?" decision for [`ensure_background_helper_running`],
+/// factored out so it is testable hermetically (no spawn side effect). Returns
+/// `true` when the file-based runtime state says the daemon is `running` AND its
+/// recorded `daemon_pid` is a live process — i.e. a second spawn must be guarded.
+/// A missing state file, `running=false`, or a dead/zero pid returns `false`.
+pub(crate) fn background_helper_already_running(paths: &AppPaths) -> Result<bool> {
+    Ok(AutomationRuntimeState::load(paths)?
+        .is_some_and(|state| state.running && rocm_core::process_is_running(state.daemon_pid)))
+}
+
 /// Shared daemon-lifecycle entrypoint: ensures the background automation helper
 /// (`rocm daemon`) is running, spawning it detached if not. Liveness is read from
 /// the file-based automation runtime state. Intentionally `pub(crate)` — reused by
@@ -4224,10 +4241,7 @@ fn start_managed_service(
 /// propagated; setup errors (path discovery, stdio attach) still return `Err`.
 pub(crate) fn ensure_background_helper_running() -> Result<()> {
     let paths = AppPaths::discover()?;
-    if let Some(state) = AutomationRuntimeState::load(&paths)?
-        && state.running
-        && rocm_core::process_is_running(state.daemon_pid)
-    {
+    if background_helper_already_running(&paths)? {
         return Ok(());
     }
 
@@ -22263,6 +22277,64 @@ VERSION_ID="41"
         )
     }
 
+    /// Build an `AutomationRuntimeState` for the no-double-spawn guard tests.
+    fn runtime_state(running: bool, daemon_pid: u32) -> AutomationRuntimeState {
+        AutomationRuntimeState {
+            running,
+            automations_enabled: true,
+            daemon_pid,
+            started_at_unix_ms: 1,
+            last_tick_unix_ms: 1,
+            local_webhook_endpoint: None,
+            active_watchers: Vec::new(),
+        }
+    }
+
+    #[test]
+    fn background_helper_already_running_true_for_live_pid() {
+        // Phase-10 daemon no-double-spawn: a runtime-state.json with running=true
+        // and a LIVE daemon_pid (this very test process) means the helper is
+        // already up — the "should spawn?" decision must say NO (true ⇒ skip).
+        // Hermetic + offline: no spawn, just the file-based liveness check.
+        let (root, paths) = test_paths("helper-live-pid");
+        runtime_state(true, std::process::id())
+            .write(&paths)
+            .expect("write runtime state");
+        assert!(
+            background_helper_already_running(&paths).expect("liveness check ok"),
+            "live recorded pid + running=true ⇒ do not spawn a second daemon"
+        );
+        let _ = fs::remove_dir_all(&root);
+    }
+
+    #[test]
+    fn background_helper_already_running_false_for_dead_or_missing() {
+        // The inverse guard cases — each must report NOT running (false ⇒ spawn):
+        // (1) no state file at all, (2) running=true but a dead/zero pid,
+        // (3) a live pid but running=false. None must spawn from this decision.
+        let (root, paths) = test_paths("helper-dead-or-missing");
+        // (1) No state file yet.
+        assert!(
+            !background_helper_already_running(&paths).expect("missing state ⇒ ok"),
+            "no runtime state ⇒ not running"
+        );
+        // (2) running=true but pid 0 is never a live process.
+        runtime_state(true, 0).write(&paths).expect("write state");
+        assert!(
+            !background_helper_already_running(&paths).expect("dead pid ⇒ ok"),
+            "running=true + dead pid ⇒ not running (spawn)"
+        );
+        // (3) live pid but running flag is false.
+        runtime_state(false, std::process::id())
+            .write(&paths)
+            .expect("write state");
+        assert!(
+            !background_helper_already_running(&paths).expect("not-running flag ⇒ ok"),
+            "running=false ⇒ not running even with a live pid"
+        );
+        let _ = fs::remove_dir_all(&root);
+    }
+
     // ---- Phase 9: reroute dispatch (bare `rocm` + interactive `rocm chat`) ----
     //
     // The interactive branches require a real TTY (`interactive_terminal()`),
@@ -22404,6 +22476,24 @@ VERSION_ID="41"
         );
     }
 
+    #[test]
+    fn command_chat_interactive_notes_dropped_provider_flag() {
+        // Phase-9 polish: --provider on interactive `rocm chat` is no longer
+        // silently ignored — the handler emits a one-line note when provider is
+        // set before rerouting to the dash. Proven by reading the handler body
+        // (the interactive branch requires a TTY, unavailable in CI).
+        let src = main_rs_source();
+        let body = strip_line_comments(&command_chat_handler_body(&src));
+        assert!(
+            body.contains("provider.is_some()"),
+            "handler must gate the note on a set --provider; body:\n{body}"
+        );
+        assert!(
+            body.contains("/provider"),
+            "the note must point the user at /provider for live switching; body:\n{body}"
+        );
+    }
+
     #[test]
     fn command_chat_honors_chat_mock_and_keeps_prompt_passthrough() {
         let src = main_rs_source();