fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint

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

@@ -292,6 +292,43 @@ pub(super) const fn startup_chat_outcome(
     }
 }
 
+/// Decide whether a persisted `chat_url` should be replaced by a freshly
+/// detected live endpoint (EAI-7360).
+///
+/// Pure — takes the already-computed reachability/detection results rather
+/// than doing I/O itself, so it is unit-testable without a tool executor or a
+/// live socket. The caller only feeds this a `live` detection result when
+/// `chat_url` came from persisted config and neither the managed-services
+/// registry nor a direct TCP probe already confirmed it reachable, so this
+/// never revalidates an explicit env override, and `resolve_llm_config`'s own
+/// CLI>config>env>probe precedence (tested directly in `llm.rs`) is
+/// completely untouched — this only ever supplies a *replacement* config
+/// before that call runs.
+///
+/// Returns `Some(live)` only when the configured endpoint is unreachable and
+/// a *genuinely different*, live endpoint was found; `None` otherwise (nothing
+/// to replace, or detection re-found the same URL modulo a trailing `/`/`/v1`).
+pub(super) fn stale_chat_url_replacement(
+    configured_base_url: &str,
+    configured_probe_ok: bool,
+    live: Option<crate::llm::LlmConfig>,
+) -> Option<crate::llm::LlmConfig> {
+    if configured_probe_ok {
+        return None;
+    }
+    let configured = normalize_base_url(configured_base_url);
+    live.filter(|l| normalize_base_url(&l.base_url) != configured)
+}
+
+/// Normalize an OpenAI-style base URL for change-detection comparison: drop a
+/// trailing `/` and a trailing `/v1` so a formatting-only difference (e.g.
+/// `http://h:8000` vs `http://h:8000/v1/`) is not mistaken for a server change
+/// and does not emit a spurious "server changed" notice.
+fn normalize_base_url(base_url: &str) -> String {
+    let trimmed = base_url.trim().trim_end_matches('/');
+    trimmed.strip_suffix("/v1").unwrap_or(trimmed).to_string()
+}
+
 /// 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.
@@ -328,8 +365,28 @@ pub(super) fn config_with_chat(
     cfg
 }
 
+/// Fold a `/v1/models`-discovered model id into a resolved [`LlmConfig`].
+///
+/// Only overrides when the user did **not** configure a model explicitly (so the
+/// resolver filled in the neutral `DEFAULT_CHAT_MODEL` placeholder). An explicit
+/// `--chat-model`/config value always wins; a failed discovery (`None`) leaves
+/// the placeholder in place so endpoints that ignore the model field keep
+/// working. Pure — the unit-test anchor for the startup model-discovery wiring.
+pub(super) fn with_discovered_model(
+    mut llm: crate::llm::LlmConfig,
+    explicit_model: Option<&str>,
+    discovered: Option<String>,
+) -> crate::llm::LlmConfig {
+    if explicit_model.is_none()
+        && let Some(model) = discovered
+    {
+        llm.model = model;
+    }
+    llm
+}
+
 /// GET `{base}/models` and return the first served model id, or `None`.
-async fn fetch_first_model(base_url: &str) -> Option<String> {
+pub(super) async fn fetch_first_model(base_url: &str) -> Option<String> {
     let url = format!("{}/models", base_url.trim_end_matches('/'));
     let client = reqwest::Client::builder()
         .timeout(std::time::Duration::from_secs(3))
@@ -340,6 +397,34 @@ async fn fetch_first_model(base_url: &str) -> Option<String> {
     crate::llm::pick_first_model(&json)
 }
 
+/// Startup served-model discovery for a *configured* chat endpoint (an explicit
+/// CLI/env URL, i.e. the [`StartupChatOutcome::Configured`] path that skips
+/// local auto-detection).
+///
+/// A configured URL with no explicit model resolves to the `local-model`
+/// placeholder, which 404s on servers that register the model under its real id
+/// (`The model 'local-model' does not exist`). When the endpoint is reachable
+/// (`probe_ok`) and the user set no explicit model, query `/v1/models` and adopt
+/// the served id — mirroring the discovery [`detect_local_chat`] already does
+/// for the auto-detected path. Otherwise the config is returned untouched: an
+/// explicit model always wins, and an unreachable endpoint is never probed
+/// (no fetch timeout) nor silently replaced. The `/v1/models` fold is delegated
+/// to the pure [`with_discovered_model`].
+pub(super) async fn discover_configured_chat_model(
+    cfg: crate::llm::LlmConfig,
+    explicit_model: Option<&str>,
+    probe_ok: bool,
+) -> crate::llm::LlmConfig {
+    // An explicit model always wins (config precedence), and an unreachable
+    // endpoint is never probed (no fetch timeout on startup) nor silently
+    // replaced — return the resolved config untouched in both cases.
+    if !probe_ok || explicit_model.is_some() {
+        return cfg;
+    }
+    let discovered = fetch_first_model(&cfg.base_url).await;
+    with_discovered_model(cfg, explicit_model, discovered)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -455,6 +540,170 @@ mod tests {
         assert_eq!(pick_managed_chat_endpoint(&env), None);
     }
 
+    fn live(base_url: &str) -> crate::llm::LlmConfig {
+        crate::llm::detected_llm_config(base_url, "m")
+    }
+
+    #[test]
+    fn stale_chat_url_replacement_none_when_configured_endpoint_reachable() {
+        // Reachable configured endpoint must never be swapped out, regardless
+        // of what a stray `live` detection result would have found.
+        assert_eq!(
+            stale_chat_url_replacement(
+                "http://127.0.0.1:9/v1",
+                true,
+                Some(live("http://127.0.0.1:11435/v1"))
+            ),
+            None
+        );
+    }
+
+    #[test]
+    fn stale_chat_url_replacement_none_when_nothing_live_found() {
+        assert_eq!(
+            stale_chat_url_replacement("http://127.0.0.1:9/v1", false, None),
+            None
+        );
+    }
+
+    #[test]
+    fn stale_chat_url_replacement_none_when_live_matches_configured() {
+        // Detection simply re-confirming the same (currently-unreachable-by-
+        // TCP-probe-timing) URL is not a "server changed" situation.
+        let url = "http://127.0.0.1:11435/v1";
+        assert_eq!(
+            stale_chat_url_replacement(url, false, Some(live(url))),
+            None
+        );
+    }
+
+    #[test]
+    fn stale_chat_url_replacement_swaps_to_a_different_live_endpoint() {
+        let found = live("http://127.0.0.1:13305/v1");
+        let replacement =
+            stale_chat_url_replacement("http://127.0.0.1:9/v1", false, Some(found.clone()))
+                .expect("a different live endpoint replaces the stale one");
+        assert_eq!(replacement.base_url, found.base_url);
+    }
+
+    #[test]
+    fn stale_chat_url_replacement_ignores_trailing_slash_and_v1_formatting() {
+        // A formatting-only difference (trailing `/`, presence/absence of the
+        // `/v1` suffix) is the SAME endpoint — no spurious "server changed".
+        for (configured, found) in [
+            ("http://127.0.0.1:8000/v1", "http://127.0.0.1:8000/v1/"),
+            ("http://127.0.0.1:8000/v1", "http://127.0.0.1:8000"),
+            ("http://127.0.0.1:8000", "http://127.0.0.1:8000/v1"),
+        ] {
+            assert_eq!(
+                stale_chat_url_replacement(configured, false, Some(live(found))),
+                None,
+                "{configured} vs {found} must be treated as the same endpoint"
+            );
+        }
+    }
+
+    #[test]
+    fn normalize_base_url_strips_trailing_slash_and_v1() {
+        assert_eq!(normalize_base_url("http://h:8000/v1/"), "http://h:8000");
+        assert_eq!(normalize_base_url("http://h:8000/v1"), "http://h:8000");
+        assert_eq!(normalize_base_url("http://h:8000/"), "http://h:8000");
+        assert_eq!(normalize_base_url("http://h:8000"), "http://h:8000");
+    }
+
+    fn placeholder_config() -> crate::llm::LlmConfig {
+        crate::llm::detected_llm_config("http://127.0.0.1:11435/v1", crate::llm::DEFAULT_CHAT_MODEL)
+    }
+
+    #[test]
+    fn discovered_model_replaces_placeholder_when_none_configured() {
+        // The 404 regression: no explicit model → the served id must be adopted.
+        let out = with_discovered_model(
+            placeholder_config(),
+            None,
+            Some("Qwen/Qwen2.5-1.5B-Instruct".to_string()),
+        );
+        assert_eq!(out.model, "Qwen/Qwen2.5-1.5B-Instruct");
+    }
+
+    #[test]
+    fn explicit_model_is_never_overridden() {
+        let mut cfg = placeholder_config();
+        cfg.model = "my-model".to_string();
+        let out = with_discovered_model(cfg, Some("my-model"), Some("served-id".to_string()));
+        assert_eq!(out.model, "my-model");
+    }
+
+    #[test]
+    fn failed_discovery_keeps_placeholder() {
+        let out = with_discovered_model(placeholder_config(), None, None);
+        assert_eq!(out.model, crate::llm::DEFAULT_CHAT_MODEL);
+    }
+
+    /// One-shot HTTP server on an ephemeral loopback port that answers a single
+    /// `GET .../models` with `body`, returning its `http://127.0.0.1:{port}/v1`
+    /// base URL. Lets the behavioral tests drive `fetch_first_model`'s real
+    /// reqwest path against a deterministic `/v1/models` response without a new
+    /// dev-dependency. The accept loop runs on a detached thread and closes
+    /// after the single request.
+    fn spawn_models_server(body: &'static str) -> String {
+        use std::io::{Read, Write};
+        let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).expect("bind loopback");
+        let port = listener.local_addr().expect("local addr").port();
+        std::thread::spawn(move || {
+            if let Ok((mut stream, _)) = listener.accept() {
+                let mut buf = [0u8; 1024];
+                let _ = stream.read(&mut buf);
+                let resp = format!(
+                    "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}",
+                    body.len(),
+                    body
+                );
+                let _ = stream.write_all(resp.as_bytes());
+                let _ = stream.flush();
+            }
+        });
+        format!("http://127.0.0.1:{port}/v1")
+    }
+
+    /// Matrix (configured URL + no model + reachable): the startup path queries
+    /// `/v1/models` and adopts the first served id. Fails on merged-main, whose
+    /// configured-endpoint path leaves the `DEFAULT_CHAT_MODEL` placeholder in
+    /// place (the `The model 'local-model' does not exist` 404).
+    #[tokio::test]
+    async fn configured_endpoint_adopts_served_model_when_no_model_set() {
+        let base = spawn_models_server(r#"{"data":[{"id":"served-model-id"}]}"#);
+        let cfg = crate::llm::detected_llm_config(&base, crate::llm::DEFAULT_CHAT_MODEL);
+        let out = discover_configured_chat_model(cfg, None, true).await;
+        assert_eq!(out.model, "served-model-id");
+    }
+
+    /// Matrix (configured URL + explicit model + reachable): discovery must not
+    /// override the user's model — config precedence is preserved even when the
+    /// endpoint serves a different id.
+    #[tokio::test]
+    async fn configured_endpoint_preserves_explicit_model() {
+        let base = spawn_models_server(r#"{"data":[{"id":"served-model-id"}]}"#);
+        let cfg = crate::llm::detected_llm_config(&base, "user-model");
+        let out = discover_configured_chat_model(cfg, Some("user-model"), true).await;
+        assert_eq!(out.model, "user-model");
+    }
+
+    /// Matrix (configured URL + no model + unreachable): fail honestly — the
+    /// configured endpoint is never swapped and no `/v1/models` fetch is even
+    /// attempted (`probe_ok == false` gates it out, so startup pays no fetch
+    /// timeout). base_url and the placeholder model are left untouched.
+    #[tokio::test]
+    async fn configured_endpoint_unreachable_is_never_replaced() {
+        let cfg = crate::llm::detected_llm_config(
+            "http://127.0.0.1:9/v1",
+            crate::llm::DEFAULT_CHAT_MODEL,
+        );
+        let out = discover_configured_chat_model(cfg.clone(), None, false).await;
+        assert_eq!(out.base_url, cfg.base_url);
+        assert_eq!(out.model, crate::llm::DEFAULT_CHAT_MODEL);
+    }
+
     /// 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

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

@@ -40,7 +40,8 @@ mod summary;
 
 use chat::{
     StartupChatOutcome, build_chat_agent, build_local_agent, detect_local_chat,
-    persist_chat_endpoint, startup_chat_outcome,
+    discover_configured_chat_model, persist_chat_endpoint, stale_chat_url_replacement,
+    startup_chat_outcome,
 };
 use summary::{parse_plan_result, summarize_json_value, summarize_slash_tool};
 
@@ -1608,11 +1609,16 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu
         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),
+            StartupChatOutcome::Configured => {
+                // Clone so `probe_target` stays available for the EAI-7360
+                // stale-URL recovery and its notice below.
+                let target = probe_target.clone();
+                tokio::task::spawn_blocking(move || {
+                    crate::llm::probe_endpoint(&target, crate::llm::PROBE_TIMEOUT)
+                })
+                .await
+                .unwrap_or(false)
+            }
         };
         let llm = detected.or_else(|| {
             crate::llm::resolve_llm_config(
@@ -1626,6 +1632,57 @@ async fn event_loop(terminal: &mut Tui, args: &ResolvedArgs) -> color_eyre::Resu
                 probe_ok,
             )
         });
+        // PR #97 port onto PR #100's startup flow: a *configured* URL (CLI/env)
+        // with no explicit model resolves to the `local-model` placeholder,
+        // which 404s on servers that register the model under its real id. Only
+        // the `Configured` outcome needs this — the `Local` outcome already
+        // carries a `/v1/models`-discovered model from `detect_local_chat`, and
+        // `OAuth` has no config. Discovery is gated inside the helper on
+        // `probe_ok` (an unreachable endpoint is never probed nor replaced) and
+        // on the absence of an explicit model (config precedence wins).
+        let llm = match llm {
+            Some(cfg) if startup_outcome == StartupChatOutcome::Configured => Some(
+                discover_configured_chat_model(cfg, args.chat_model.as_deref(), probe_ok).await,
+            ),
+            other => other,
+        };
+        // EAI-7360: a persisted `tui.chat_url` can go stale — the server it
+        // pointed at may have been stopped or replaced since the dash last wrote
+        // it. This is the `Configured`-but-unreachable case: the reachable
+        // `Configured` case already ran `discover_configured_chat_model` above,
+        // and a `Local` outcome auto-detected its own live endpoint. Only this
+        // persisted-config tier is revalidated — `chat_env_url` is a separate
+        // tier, and `resolve_llm_config`'s CLI>config>env>probe precedence (the
+        // literal contract unit-tested in `llm.rs`) is untouched: this only
+        // substitutes a *replacement* `LlmConfig` after that call, never changes
+        // how the call itself resolves.
+        //
+        // Require NO configured api_key/auth_header: the replacement is a
+        // keyless `detected_llm_config`, and since `ResolvedArgs` can't
+        // distinguish an explicit `--chat-url` from persisted config, a remote
+        // gateway URL + `OPENAI_API_KEY` that merely TCP-times-out must never
+        // silently swap to a local keyless engine and drop the credential.
+        // Confining the swap to a no-auth `chat_url` keeps it on the true
+        // EAI-7360 target: a dead *persisted* local endpoint.
+        let stale_replacement = if startup_outcome == StartupChatOutcome::Configured
+            && !probe_ok
+            && args.chat_url.is_some()
+            && args.chat_api_key.is_none()
+            && args.chat_auth_header.is_none()
+        {
+            let live = detect_local_chat(state.tool_executor.clone()).await;
+            stale_chat_url_replacement(&probe_target, probe_ok, live)
+        } else {
+            None
+        };
+        if let Some(live) = &stale_replacement {
+            state.chat.push(ChatTurn::system(format!(
+                "Configured chat server at {probe_target} is unreachable; switched to the live \
+                 local engine at {} (model: {}). Run `/detect save` to persist it.",
+                live.base_url, live.model
+            )));
+        }
+        let llm = stale_replacement.or(llm);
         state.set_chat_config(llm, args.chat_auto_consent);
         // No reachable local endpoint AND no key/url configured → the no-key
         // ChatGPT OAuth default (device-code login surfaced in the chat tab).