diff --git a/crates/rocm-dash-tui/src/app/chat.rs b/crates/rocm-dash-tui/src/app/chat.rs index 54f70010..c028eea2 100644 --- a/crates/rocm-dash-tui/src/app/chat.rs +++ b/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, +) -> Option { + 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, +) -> 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 { +pub(super) async fn fetch_first_model(base_url: &str) -> Option { 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 { 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 diff --git a/crates/rocm-dash-tui/src/app/mod.rs b/crates/rocm-dash-tui/src/app/mod.rs index 64cb4da9..f995126e 100644 --- a/crates/rocm-dash-tui/src/app/mod.rs +++ b/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).