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

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

Startup only single-probed DEFAULT_CHAT_BASE_URL (:8000) before falling
back to the ChatGPT cloud OAuth flow, even though the manual 'd' detect
path already knew how to find a local engine on any of its well-known
ports (Lemonade :13305, vLLM :8000, rocm serve :11435) plus the
managed-services registry. A server running on any port other than
:8000 was invisible at startup and users landed in the cloud login flow
instead.

Reuse detect_local_chat() (registry-first, then the 3-port probe, then
a best-effort model fetch) in the startup branch that already gated on
"no explicit chat_url/env_url configured" — the same condition that
previously ran only detect_managed_chat(). Parallelize
llm::detect_local_endpoint()'s port probe (std::thread::scope) so a
cold start with nothing listening still costs one PROBE_TIMEOUT
(~300ms) instead of three, keeping startup latency bounded.

Unmerged PR #97 also touches this startup branch (model discovery
inside resolve_llm_config's own fallback when chat_model is None);
this change is conflict-minimal — it swaps detect_managed_chat for
detect_local_chat without touching the resolve_llm_config call itself.

Relates to EAI-7347.

Signed-off-by: Michael Roy <michael.roy@amd.com>

* fix(chat): do not run local-detection swap when an api key is configured

Startup local-engine detection returns a keyless detected_llm_config
(api_key/auth_header forced to None). Firing it whenever no explicit
URL was set — even when the user configured OPENAI_API_KEY /
ROCMDASH_CHAT_API_KEY — silently dropped that credential and produced a
401 at request time.

Gate detection on chat_api_key.is_none() as well (extracted into the
pure should_detect_local_chat helper for unit testing): a configured
key means "use my configured backend", so skip the swap and let
resolve_llm_config carry the key through its normal precedence.

Also skip the redundant fallback probe: when detection ran and found
nothing it already probed the well-known vLLM :8000 port (==
DEFAULT_CHAT_BASE_URL), so re-probing that same target just burned
another PROBE_TIMEOUT on a no-server cold start; treat it as
unreachable directly.

Relates to EAI-7347.

Signed-off-by: Michael Roy <michael.roy@amd.com>

* fix(dash): guard well-known ports free in local-detect port-priority tests

detect_local_chat_prefers_local_server_over_no_endpoint (and the sibling
detect_local_endpoint_probes_rocm_serve_default_port) only bound the
rocm-serve port and assumed the higher-priority Lemonade/vLLM ports were
unreachable. On the Windows CI runner that isn't guaranteed, so an
ambient listener on :13305 or :8000 made detection return that endpoint
instead of rocm-serve's, failing the assertion.

Both tests now bind-and-release the higher-priority ports first to
confirm they're free before asserting on the rocm-serve fallback.

Signed-off-by: Michael Roy <michael.roy@amd.com>

* fix(dash): make local-detect priority tests port-independent

The previous fix (bind-then-release the higher-priority ports before
asserting) still flaked on the Windows CI runner: something answered on
the Lemonade port during the probe despite the guard, most likely an
ambient listener the test doesn't control. TCP-connect-based detection
means any listener on a well-known port — even a bare guard listener
held open, as the very first attempt at this fix did — registers as
"reachable", so real ports can't be made deterministic from a test.

Split the port-priority logic in llm.rs into probe_first_reachable,
tested directly against OS-assigned ephemeral ports instead of the real
well-known ports. Gave detect_local_chat an injectable probe seam
(detect_local_chat_with_probe) so its own test can verify the
no-managed-executor wiring without depending on any real network state.

Signed-off-by: Michael Roy <michael.roy@amd.com>

* fix(dash): widen latency margin in detect_local_endpoint timing test

detect_local_endpoint_bounded_latency_when_nothing_listening's 2x
PROBE_TIMEOUT margin was still too tight for the Windows CI runner
(observed 624ms against a 600ms threshold). A true sequential-probing
regression takes ~3x PROBE_TIMEOUT, so widen the margin to 4x to absorb
scheduling jitter on slower CI machines while still catching that
regression.

Signed-off-by: Michael Roy <michael.roy@amd.com>

* test(dash): strengthen local chat startup coverage

Signed-off-by: Michael Roy <michael.roy@amd.com>

---------

Signed-off-by: Michael Roy <michael.roy@amd.com>

Author: michaelroy-amd

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,44 @@ 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()
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(super) enum StartupChatOutcome {
+    Local,
+    OAuth,
+    Configured,
+}
+
+/// Select the startup backend path after optional local detection.
+///
+/// A successful detection is already reachable. A detection miss means the
+/// well-known endpoints were exhausted and startup should proceed to OAuth.
+/// When detection was gated off, the configured target still needs its normal
+/// liveness probe before backend construction.
+pub(super) const fn startup_chat_outcome(
+    detection_ran: bool,
+    detected: bool,
+) -> StartupChatOutcome {
+    match (detection_ran, detected) {
+        (_, true) => StartupChatOutcome::Local,
+        (true, false) => StartupChatOutcome::OAuth,
+        (false, false) => StartupChatOutcome::Configured,
+    }
+}
+
 /// 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 +454,64 @@ 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_wires_probed_endpoint_through() {
+        // 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
+        ));
+    }
+
+    #[test]
+    fn startup_uses_detected_local_endpoint() {
+        assert_eq!(startup_chat_outcome(true, true), StartupChatOutcome::Local);
+    }
+
+    #[test]
+    fn startup_uses_oauth_after_empty_detection() {
+        assert_eq!(startup_chat_outcome(true, false), StartupChatOutcome::OAuth);
+    }
+
+    #[test]
+    fn startup_uses_configured_endpoint_when_detection_is_skipped() {
+        assert_eq!(
+            startup_chat_outcome(false, false),
+            StartupChatOutcome::Configured
+        );
+    }
 }

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

@@ -39,8 +39,8 @@ mod slash;
 mod summary;
 
 use chat::{
-    build_chat_agent, build_local_agent, detect_local_chat, detect_managed_chat,
-    persist_chat_endpoint,
+    StartupChatOutcome, build_chat_agent, build_local_agent, detect_local_chat,
+    persist_chat_endpoint, startup_chat_outcome,
 };
 use summary::{parse_plan_result, summarize_json_value, summarize_slash_tool};
 
@@ -1562,8 +1562,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 +1598,23 @@ 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() {
-            true
-        } else {
-            tokio::task::spawn_blocking(move || {
+        // 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 startup_outcome = startup_chat_outcome(detection_ran, detected.is_some());
+        let probe_ok = match startup_outcome {
+            StartupChatOutcome::Local => true,
+            StartupChatOutcome::OAuth => false,
+            StartupChatOutcome::Configured => tokio::task::spawn_blocking(move || {
                 crate::llm::probe_endpoint(&probe_target, crate::llm::PROBE_TIMEOUT)
             })
             .await
-            .unwrap_or(false)
+            .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(),
@@ -1599,10 +1631,7 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu
         // ChatGPT OAuth default (device-code login surfaced in the chat tab).
         // This restores the no-key login the vendored Codex path provided; it
         // takes NO api_key (env-only invariant untouched — OAuth, not a key).
-        let no_key_no_endpoint = !probe_ok
-            && args.chat_api_key.is_none()
-            && args.chat_url.is_none()
-            && args.chat_env_url.is_none();
+        let no_key_no_endpoint = startup_outcome == StartupChatOutcome::OAuth;
         if no_key_no_endpoint {
             let oauth_tx = chat_tx.clone();
             crate::agent::ChatGptAgentClient::new(

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

@@ -188,20 +188,49 @@ 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, P>(candidates: &[&'a str], probe: P) -> Option<&'a str>
+where
+    P: Fn(&str, Duration) -> bool + Sync,
+{
+    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(ep, PROBE_TIMEOUT))))
+            .collect();
+        handles
+            .into_iter()
+            .find_map(|(ep, handle)| handle.join().unwrap_or(false).then_some(ep))
+    })
+}
+
+const LOCAL_ENDPOINT_CANDIDATES: [&str; 3] = [
+    crate::skills::LEMONADE_ENDPOINT,
+    crate::skills::VLLM_ENDPOINT,
+    crate::skills::ROCM_SERVE_ENDPOINT,
+];
+
+/// 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> {
-    [
-        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(&LOCAL_ENDPOINT_CANDIDATES, probe_endpoint)
 }
 
 /// Pick the first model id from an OpenAI-compatible `/v1/models` response (`{"data":[{"id":"…"}]}`).
@@ -438,20 +467,83 @@ 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()], probe_endpoint),
+            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!(
+            probe_first_reachable(&["http://127.0.0.1:1", url.as_str()], probe_endpoint,),
+            Some(url.as_str())
+        );
+    }
+
+    #[test]
+    fn probe_first_reachable_none_when_nothing_listening() {
+        assert_eq!(
+            probe_first_reachable(&["http://127.0.0.1:1"], probe_endpoint),
+            None
+        );
+    }
+
+    #[test]
+    fn probe_first_reachable_starts_all_probes_concurrently() {
+        use std::sync::atomic::{AtomicUsize, Ordering};
+
+        let active = AtomicUsize::new(0);
+        let max_active = AtomicUsize::new(0);
+        let probe = |_: &str, _: Duration| {
+            let now_active = active.fetch_add(1, Ordering::SeqCst) + 1;
+            max_active.fetch_max(now_active, Ordering::SeqCst);
+            std::thread::sleep(Duration::from_millis(50));
+            active.fetch_sub(1, Ordering::SeqCst);
+            false
         };
+
+        assert_eq!(probe_first_reachable(&["a", "b", "c"], probe), None);
+        assert_eq!(
+            max_active.load(Ordering::SeqCst),
+            3,
+            "all probes must be in flight before any probe is joined"
+        );
+    }
+
+    #[test]
+    fn local_endpoint_candidates_preserve_supported_order() {
         assert_eq!(
-            detect_local_endpoint(),
-            Some(crate::skills::ROCM_SERVE_ENDPOINT)
+            LOCAL_ENDPOINT_CANDIDATES,
+            [
+                crate::skills::LEMONADE_ENDPOINT,
+                crate::skills::VLLM_ENDPOINT,
+                crate::skills::ROCM_SERVE_ENDPOINT,
+            ]
         );
-        drop(listener);
     }
 }