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

[michaelroy-amd]

Summary

• Revalidates a persisted tui.chat_url at startup: if it's unreachable
  and not matched by the live managed-services registry, the dash now
  falls back to a freshly-detected local engine instead of silently
  building an agent against a dead endpoint.
• Surfaces a "server changed" notice in the chat transcript pointing at
  the new /detect save command (PR feat(chat): add /detect slash command for mid-session re-detection #102) to persist the replacement.

Root cause

tui.chat_url survives across dash launches once written (e.g. via the
in-TUI "use & save" flow). If the server behind it is later stopped or
replaced — most commonly a managed vLLM/Lemonade instance relaunched on
a different port — the startup path passed the stale URL straight into
resolve_llm_config regardless of reachability. The chat backend was
then built against a dead endpoint with no on-screen indication
anything had changed; the user would only discover it from a failed
completion request.

Fix

• app/chat.rs: new pure decision function stale_chat_url_replacement
  — given the configured base URL, whether its own probe succeeded, and
  an already-detected live LlmConfig (or None), returns
  Some(live) only when the configured endpoint is unreachable and a
  differently-addressed live endpoint exists. No I/O; fully
  unit-testable.
• app/mod.rs (startup block in event_loop()): when chat_url is
  configured, wasn't matched by the managed-services registry, and its
  TCP probe fails, calls detect_local_chat and feeds the result
  through stale_chat_url_replacement. A replacement, if found, is
  substituted before resolve_llm_config runs and a
  ChatTurn::system(...) notice is pushed into the transcript.

Precedence preserved

This only ever revalidates the persisted-config tier:

• chat_env_url (the env tier) is untouched — it's already excluded
  from the managed-registry/stale-check path (both only run when
  chat_url itself is set).
• resolve_llm_config's own CLI>config>env>probe precedence is not
  modified at all; the substitution happens purely at the call site by
  swapping in a different LlmConfig before that function runs. Its
  existing unit tests — cli_wins_over_every_other_tier,
  config_wins_when_no_cli, env_url_wins_when_no_cli_or_config,
  probed_default_used_when_nothing_configured_but_reachable — are
  unmodified and pass unchanged.

(Note: this crate's ResolvedArgs::chat_url is always sourced from
tui.chat_url at the one call site in apps/rocm/src/dash.rs — there
is no separate explicit-CLI flag wired through a different field today
— so "persisted-config tier" is precisely what this revalidates.)

Coordination notes

Complements unmerged PR #97, which addresses the chat_model half of
the same "server changed" gap. No overlapping hunks expected (this PR
touches app/chat.rs + the startup block's chat_url/probe handling
in app/mod.rs).

Relates to EAI-7360.

Test Plan

• cargo build -p rocm-dash-tui
• cargo clippy -p rocm-dash-tui --all-targets --all-features -- -D warnings (clean)
• cargo test -p rocm-dash-tui -- --test-threads=1 (all passing, precedence tests unaffected)
• New tests:
  • stale_chat_url_replacement_none_when_configured_endpoint_reachable
  • stale_chat_url_replacement_none_when_nothing_live_found
  • stale_chat_url_replacement_none_when_live_matches_configured
  • stale_chat_url_replacement_swaps_to_a_different_live_endpoint

[rominf]

LGTM. The stale-persisted-URL revalidation is well-guarded: the swap only fires on !probe_ok and a genuinely-different (trailing-/ and /v1 normalized) URL, and is gated on chat_api_key.is_none() && chat_auth_header.is_none() so a configured remote gateway that merely times out won't silently fall back to a keyless local engine. No loop (re-detecting the same URL returns None, so no spurious "server changed" notice), no panic (unwrap_or(false) / .ok()), and correct precedence (managed.or_else(stale).or_else(resolve_llm_config) — the CLI>config>env>probe contract is untouched). Dropping chat_model on swap is correct since it's a different server. Good test coverage including the normalization cases.

Note: on the degraded path (persisted URL dead) startup now does a bounded one-time extra probe (~300ms + detect_local_chat) before the first frame — worth a sentence in the PR body.

Approving.

[volen-silo]

🔴 Automated review · pr-review-watcher · ed0b00e

Summary

• Change: Adds a startup revalidation for a stale persisted tui.chat_url — when the configured endpoint is unreachable and no auth/key is configured, the dash falls back to a freshly-detected live local engine and surfaces a "server changed" notice. Two files, +147/-3 (app/chat.rs, app/mod.rs).
• Overall assessment: Approve with minor suggestions. The core logic is correct and well-scoped. The substitution genuinely prevents building against a dead endpoint, the central safety claims hold (no credential is ever dropped; no explicit-CLI flag is ever swapped), and the new pure functions are cleanly unit-tested.
• Verification: built clean and ran the 6 new unit tests locally (cargo test -p rocm-dash-tui --lib) — all pass. No blocking findings.

Verified as correct (the load-bearing claims)

• Substitution actually fixes the bug: with chat_url = Some(dead), resolve_llm_config's cli_url.or(...) returns the dead URL verbatim regardless of probe_ok, so pre-PR the backend was built against the dead endpoint. Because managed is always None on this path, managed.or_else(|| stale_replacement) correctly lets the replacement win over that fallback. Confirmed against llm.rs:76-80.
• No credential drop / no explicit-flag swap: traced ResolvedArgs.chat_url — it is sourced only from persisted t.chat_url (apps/rocm/src/dash.rs:188); there is no --chat-url clap arg anywhere in apps/rocm/src/main.rs. The gate additionally requires chat_api_key.is_none() && chat_auth_header.is_none(), and the swap never mutates the key/header in ResolvedArgs. The safety invariant the PR is built on holds.
• Notice accuracy: because the gate requires chat_url.is_some(), probe_target here is always the configured URL (never env/default), so "Configured chat server at {probe_target} is unreachable…" is accurate. Interpolation of live.base_url/live.model is correct.

Non-blocking suggestions

1. Dead/redundant guard conjunct — crates/rocm-dash-tui/src/app/mod.rs:1606. managed.is_none() is always true when this branch is reached: managed (line ~1565) can only be Some when chat_url.is_none() && chat_env_url.is_none(), but the guard already requires chat_url.is_some(). Harmless but misleading — a reader may think it guards a real "managed beats stale persisted URL" case that can't occur. Consider dropping it or adding a one-line "structurally always true" note.

2. probe_ok passed into stale_chat_url_replacement is statically false at this call site — mod.rs:1610. The guard has !probe_ok, so the function's if configured_probe_ok { return None } can never fire from here. Not a bug (the function is still correctly tested standalone with true), but the parameter is redundant/confusing at the call site.

3. normalize_base_url heuristic gaps — crates/rocm-dash-tui/src/app/chat.rs:281. Verified by executing the exact logic against edge cases:

   • http://h:8000/v1/v1 → http://h:8000/v1 (only one /v1 stripped; non-idempotent).
   • http://h:8000/V1 → unchanged (case-sensitive strip, unlike is_loopback_host's eq_ignore_ascii_case elsewhere) → spurious "server changed" for the same server.
   • http://h:8000/v1?x=1 → unchanged (query not handled, unlike parse_host_port, which splits on ['/', '?']).
   • http://h:8000/api/v1 and http://h:8000/api normalize equal → a legitimate swap would be suppressed.

   None of these hit the realistic EAI-7360 target (local vLLM/Lemonade on standard /v1), so impact is limited to a spurious/suppressed notice in the narrow "configured URL unreachable + a live local engine present" window. Worth considering reusing the existing authority-parsing in llm.rs (parse_host_port) rather than a second, divergent URL canonicalizer, and lowercasing before the /v1 strip.

4. Test doesn't assert full-config preservation — crates/rocm-dash-tui/src/app/chat.rs (swap test, ~line 487). stale_chat_url_replacement_swaps_to_a_different_live_endpoint asserts only replacement.base_url; it never checks model/api_key/auth_header, so a broken reconstruction of the returned LlmConfig wouldn't be caught. Add asserts on the other fields, and a case for /v1/v1 / uppercase /V1 if you tighten (3).

5. Stale doc comment adjacent to the change — crates/rocm-dash-tui/src/app/mod.rs:82: chat_url is documented as "CLI-flag value already merged over config," which contradicts the actual sourcing (persisted config only) and the very safety invariant this PR relies on. Since it sits in a file this PR touches and is directly relevant to the reviewer's reasoning, fixing it here would be worthwhile. (Related, out-of-PR-scope: crates/rocm-dash-tui/src/ui/tabs/chat.rs:204 still advertises a --chat-url flag that does not exist — flagged for future cleanup, not this PR.)

Tradeoffs (deliberate choices worth confirming, not change requests)

• Startup latency on the dead-URL path: when a configured chat_url is unreachable with no key/header, detect_local_chat now runs after the initial probe — managed detection (blocking executor call + a fetch_first_model HTTP GET, up to ~3s) plus up to three sequential TCP probes (≈900ms) plus another model fetch. This stacks onto the existing "one probe before first frame" budget, so the first frame can stall noticeably longer than PROBE_TIMEOUT in that specific failure case. Scoped and functionally correct, but a real UX cost.
• Keyless-remote endpoint indistinguishable from local: the gate (chat_url set + no key + no auth-header) matches not only a dead local endpoint but also a keyless/network-authed remote gateway whose key merely isn't in the current shell's env (key is env-only via OPENAI_API_KEY/ROCMDASH_CHAT_API_KEY). With a 300ms TCP timeout, a real-but-slow remote could probe-fail and get redirected to a local engine. Mitigated: it fires only if a different live engine exists, it's visible (system chat turn), and it's non-persistent (only /detect save persists). If false-positive swaps on real remotes show up in practice, a "skip swap when configured host is non-loopback" heuristic would tighten it. Noting for awareness — the credential-safety claim itself is not violated.

Positive signals

• Extracting the decision into a pure, I/O-free stale_chat_url_replacement with focused unit tests is the right call — testable without a socket or tool executor.
• The auth-drop gate and its rationale comment (mod.rs:1596-1602) show the credential-leak edge case was reasoned about deliberately, not by accident.
• Existing resolve_llm_config precedence tests are genuinely untouched, so the substitution-at-call-site approach keeps the precedence contract intact.