fix(dash): prefer a local chat server over the cloud default at startup

crates/rocm-dash-tui/src/app/chat.rs

@@ -228,16 +228,24 @@ pub(super) async fn detect_managed_chat(
 /// Lemonade/vLLM endpoints when no managed service is available.
 pub(super) async fn detect_local_chat(
     executor: Option<crate::tool_exec::SharedRocmToolExecutor>,
+) -> Option<crate::llm::LlmConfig> {
+    detect_local_chat_with_probe(executor, crate::llm::detect_local_endpoint).await
+}
+
+/// Same as [`detect_local_chat`], but with the well-known-port probe passed
+/// in rather than hard-coded, so a test can substitute a deterministic probe
+/// instead of depending on the real well-known ports being free on the
+/// machine running the test.
+async fn detect_local_chat_with_probe(
+    executor: Option<crate::tool_exec::SharedRocmToolExecutor>,
+    probe: impl Fn() -> Option<&'static str> + Send + 'static,
 ) -> Option<crate::llm::LlmConfig> {
     if let Some(cfg) = detect_managed_chat(executor).await {
         return Some(cfg);
     }
 
     // TCP probe is blocking; keep it off the async reactor.
-    let base = tokio::task::spawn_blocking(crate::llm::detect_local_endpoint)
-        .await
-        .ok()
-        .flatten()?;
+    let base = tokio::task::spawn_blocking(probe).await.ok().flatten()?;
 
     // Best-effort model query; fall back to the neutral default on any failure.
     let model = fetch_first_model(base)
@@ -246,6 +254,20 @@ pub(super) async fn detect_local_chat(
     Some(crate::llm::detected_llm_config(base, &model))
 }
 
+/// Whether startup should run the local-engine auto-detection swap.
+///
+/// Detection produces a keyless [`crate::llm::detected_llm_config`], so it may
+/// only fire when the user configured NO endpoint (URL/env URL) AND NO api key
+/// — otherwise the swap would silently drop a configured credential and 401 at
+/// request time. Pure; the anchor for the startup-gate unit test.
+pub(super) const fn should_detect_local_chat(
+    chat_url: Option<&str>,
+    chat_env_url: Option<&str>,
+    chat_api_key: Option<&str>,
+) -> bool {
+    chat_url.is_none() && chat_env_url.is_none() && chat_api_key.is_none()
+}
+
 /// Persist an accepted local endpoint to the user's `config.toml`: load the
 /// existing config (or defaults), set `tui.chat_url`/`tui.chat_model`, and write
 /// it back. Best-effort — returns a human error string on failure.
@@ -408,4 +430,46 @@ mod tests {
         let env = services_envelope(serde_json::json!([service("vllm", "", "ready", "m", 100)]));
         assert_eq!(pick_managed_chat_endpoint(&env), None);
     }
+
+    /// EAI-7347: startup now reuses `detect_local_chat` (the same detection the
+    /// manual 'd' path already used) instead of a single well-known-port probe,
+    /// so a local server on a non-default well-known port (e.g. `rocm serve`'s
+    /// :11435) is preferred over falling back to the ChatGPT cloud default.
+    #[tokio::test]
+    async fn detect_local_chat_prefers_local_server_over_no_endpoint() {
+        // Priority order among the well-known ports (Lemonade > vLLM > rocm
+        // serve's default) is covered deterministically in `llm.rs`'s own
+        // tests, against OS-assigned ephemeral ports. This test's job is
+        // narrower: prove `detect_local_chat`'s no-managed-executor path
+        // wires whatever the well-known-port probe finds into the resulting
+        // config, rather than falling back to the cloud default — so the
+        // probe is injected rather than exercised against real ports (real
+        // ports it doesn't own would make this flake on a CI runner where an
+        // ambient listener happens to occupy one of them).
+        const PROBED: &str = "http://127.0.0.1:1/v1";
+        let cfg = detect_local_chat_with_probe(None, || Some(PROBED))
+            .await
+            .expect("local server detected");
+        assert_eq!(cfg.base_url, PROBED);
+    }
+
+    #[test]
+    fn should_detect_only_when_no_url_env_or_key_configured() {
+        // The clean slate: nothing configured → auto-detect a local engine.
+        assert!(should_detect_local_chat(None, None, None));
+        // A configured api key must SUPPRESS the swap — detection returns a
+        // keyless config and would otherwise silently drop the key (401).
+        assert!(!should_detect_local_chat(None, None, Some("sk-configured")));
+        // An explicit URL or env URL also suppresses it (config precedence).
+        assert!(!should_detect_local_chat(
+            Some("http://cfg:1/v1"),
+            None,
+            None
+        ));
+        assert!(!should_detect_local_chat(
+            None,
+            Some("http://env:2/v1"),
+            None
+        ));
+    }
 }

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

@@ -38,10 +38,7 @@ mod chat;
 mod slash;
 mod summary;
 
-use chat::{
-    build_chat_agent, build_local_agent, detect_local_chat, detect_managed_chat,
-    persist_chat_endpoint,
-};
+use chat::{build_chat_agent, build_local_agent, detect_local_chat, persist_chat_endpoint};
 use summary::{parse_plan_result, summarize_json_value, summarize_slash_tool};
 
 /// Which single flow a *focused host* runs.
@@ -1562,8 +1559,34 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu
         // NOT override an explicitly configured `chat_url`/env URL, so config
         // precedence is preserved (we only consult the registry when neither is
         // set, i.e. where the well-known default would otherwise be probed).
-        let managed = if args.chat_url.is_none() && args.chat_env_url.is_none() {
-            detect_managed_chat(state.tool_executor.clone()).await
+        // When neither an explicit URL (CLI/config) nor an env URL is set, run
+        // the SAME full local-engine detection the manual 'd' path uses:
+        // registry-first (an engine we launched ourselves, on whatever port it
+        // bound), then a probe of the well-known Lemonade/vLLM/rocm-serve
+        // ports (parallelized — see `llm::detect_local_endpoint` — so a cold
+        // start with no server doesn't pay 3x the probe timeout), plus a
+        // best-effort served-model fetch. This is what lets a local server win
+        // over the ChatGPT cloud default at startup instead of only the single
+        // well-known :8000 port that a bare `resolve_llm_config` probe covers.
+        //
+        // NOTE: unmerged PR #97 also touches this branch (model discovery when
+        // `chat_model` is None, inside `resolve_llm_config`'s own fallback
+        // path) — this change is conflict-minimal by leaving the
+        // `resolve_llm_config` call below untouched.
+        //
+        // Gate on `chat_api_key.is_none()` too: local detection returns a
+        // keyless `detected_llm_config` (api_key/auth_header forced to None),
+        // so firing it when the user configured a key would SILENTLY DROP that
+        // key and 401 at request time. A configured key means "use my
+        // configured backend", so skip the swap and let `resolve_llm_config`
+        // carry the key through its normal precedence.
+        let detection_ran = chat::should_detect_local_chat(
+            args.chat_url.as_deref(),
+            args.chat_env_url.as_deref(),
+            args.chat_api_key.as_deref(),
+        );
+        let detected = if detection_ran {
+            detect_local_chat(state.tool_executor.clone()).await
         } else {
             None
         };
@@ -1572,17 +1595,24 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu
             .clone()
             .or_else(|| args.chat_env_url.clone())
             .unwrap_or_else(|| crate::llm::DEFAULT_CHAT_BASE_URL.to_string());
-        // A managed endpoint is already readiness-verified; otherwise TCP-probe.
-        let probe_ok = if managed.is_some() {
+        // A detected endpoint (managed or probed) is already verified. When
+        // detection ran and found nothing it already probed the well-known
+        // vLLM :8000 port (== `DEFAULT_CHAT_BASE_URL`), so re-probing the same
+        // fallback target here is redundant and just burns another probe
+        // timeout on a cold start — treat that as unreachable directly.
+        // Otherwise (an explicit URL/env/key path) TCP-probe the target.
+        let probe_ok = if detected.is_some() {
             true
+        } else if detection_ran {
+            false
         } else {
             tokio::task::spawn_blocking(move || {
                 crate::llm::probe_endpoint(&probe_target, crate::llm::PROBE_TIMEOUT)
             })
             .await
             .unwrap_or(false)
         };
-        let llm = managed.or_else(|| {
+        let llm = detected.or_else(|| {
             crate::llm::resolve_llm_config(
                 args.chat_url.as_deref(),
                 args.chat_model.as_deref(),

crates/rocm-dash-tui/src/llm.rs

@@ -188,20 +188,45 @@ pub fn probe_endpoint(base_url: &str, timeout: Duration) -> bool {
     false
 }
 
-/// Probe the known local serving endpoints in priority order (Lemonade, then
-/// vLLM, then `rocm serve`'s non-managed default) and return the first
-/// reachable one.
+/// Probe `candidates` (in priority order) concurrently and return the
+/// highest-priority one that answered.
 ///
-/// Used by the TUI's in-app "detect a local engine" action so the user need not run the CLI skill first.
-/// TCP-only (no HTTP); never blocks longer than [`PROBE_TIMEOUT`] per candidate.
+/// Split out from [`detect_local_endpoint`] so priority-order behavior is
+/// testable against OS-assigned ephemeral ports instead of the real
+/// well-known ports, which may be occupied by an unrelated ambient listener
+/// on the machine running the test. Probing all candidates in parallel
+/// (rather than sequentially) bounds the worst case (no server listening) to
+/// one [`PROBE_TIMEOUT`] instead of one per candidate. TCP-only (no HTTP);
+/// never blocks longer than [`PROBE_TIMEOUT`] total.
+fn probe_first_reachable<'a>(candidates: &[&'a str]) -> Option<&'a str> {
+    std::thread::scope(|scope| {
+        // The collect is load-bearing, not needless: every probe must be
+        // *spawned* before any is *joined*, or they run one at a time and the
+        // whole point (bounding total latency to one PROBE_TIMEOUT) is lost.
+        #[allow(clippy::needless_collect)]
+        let handles: Vec<_> = candidates
+            .iter()
+            .map(|ep| (*ep, scope.spawn(|| probe_endpoint(ep, PROBE_TIMEOUT))))
+            .collect();
+        handles
+            .into_iter()
+            .find_map(|(ep, handle)| handle.join().unwrap_or(false).then_some(ep))
+    })
+}
+
+/// Probe the known local serving endpoints (Lemonade, vLLM, then `rocm
+/// serve`'s non-managed default) concurrently and return the highest-priority
+/// one that answered.
+///
+/// Used by the TUI's in-app "detect a local engine" action, and by chat
+/// startup, so the user need not run the CLI skill first.
 pub fn detect_local_endpoint() -> Option<&'static str> {
-    [
+    const CANDIDATES: [&str; 3] = [
         crate::skills::LEMONADE_ENDPOINT,
         crate::skills::VLLM_ENDPOINT,
         crate::skills::ROCM_SERVE_ENDPOINT,
-    ]
-    .into_iter()
-    .find(|ep| probe_endpoint(ep, PROBE_TIMEOUT))
+    ];
+    probe_first_reachable(&CANDIDATES)
 }
 
 /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`).
@@ -438,20 +463,63 @@ mod tests {
         assert!(!probe_endpoint("not a url", Duration::from_millis(100)));
     }
 
+    /// Binds an OS-assigned ephemeral port and returns its listener (kept
+    /// alive so the port stays reachable) plus its `http://host:port` URL.
+    ///
+    /// Ephemeral ports are used instead of the real well-known ports
+    /// (Lemonade/vLLM/rocm serve) so priority-order tests are deterministic:
+    /// an ambient listener elsewhere on the machine (e.g. a real local
+    /// engine, or — on some CI runners — a pre-installed one) can otherwise
+    /// make well-known-port-based tests flake by answering a probe the test
+    /// didn't intend to exercise.
+    fn ephemeral_endpoint() -> (std::net::TcpListener, String) {
+        let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).expect("bind ephemeral port");
+        let url = format!("http://{}", listener.local_addr().expect("local_addr"));
+        (listener, url)
+    }
+
     #[test]
-    fn detect_local_endpoint_probes_rocm_serve_default_port() {
-        // A listener on rocm serve's (non-managed) default port must be picked
-        // up by the fallback probe, not just Lemonade/vLLM's ports.
-        let Ok(listener) =
-            std::net::TcpListener::bind(("127.0.0.1", crate::skills::ROCM_SERVE_PORT))
-        else {
-            // Port already bound in this environment; skip rather than flake.
-            return;
-        };
+    fn probe_first_reachable_prefers_earlier_candidate_when_multiple_reachable() {
+        // Two reachable candidates: priority order (first in the list) must
+        // win even though probes run concurrently, not sequentially.
+        let (_first, first_url) = ephemeral_endpoint();
+        let (_second, second_url) = ephemeral_endpoint();
+        assert_eq!(
+            probe_first_reachable(&[first_url.as_str(), second_url.as_str()]),
+            Some(first_url.as_str())
+        );
+    }
+
+    #[test]
+    fn probe_first_reachable_skips_unreachable_earlier_candidates() {
+        // The first candidate (port 1) is essentially never open; the probe
+        // must fall through to the second, reachable one.
+        let (_listener, url) = ephemeral_endpoint();
         assert_eq!(
-            detect_local_endpoint(),
-            Some(crate::skills::ROCM_SERVE_ENDPOINT)
+            probe_first_reachable(&["http://127.0.0.1:1", url.as_str()]),
+            Some(url.as_str())
+        );
+    }
+
+    #[test]
+    fn probe_first_reachable_none_when_nothing_listening() {
+        assert_eq!(probe_first_reachable(&["http://127.0.0.1:1"]), None);
+    }
+
+    #[test]
+    fn detect_local_endpoint_bounded_latency_when_nothing_listening() {
+        // Regression guard for the startup-latency fix: with no server
+        // reachable, probing all 3 well-known ports must take roughly one
+        // PROBE_TIMEOUT (parallel), not three (sequential). A true regression
+        // back to sequential probing takes ~3x PROBE_TIMEOUT, so a 4x margin
+        // still catches it while giving plenty of headroom for scheduling
+        // jitter on loaded/slower CI machines (observed >2x on Windows CI).
+        let start = std::time::Instant::now();
+        let _ = detect_local_endpoint();
+        assert!(
+            start.elapsed() < PROBE_TIMEOUT * 4,
+            "expected parallel probing to bound latency to ~1 PROBE_TIMEOUT, took {:?}",
+            start.elapsed()
         );
-        drop(listener);
     }
 }