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, the
  dash 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
  /detect save command 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, whose
cli_url.or(cfg_url).or(env_url) (llm.rs:76-80) returns it unconditionally;
probe_ok only gates the DEFAULT_CHAT_BASE_URL fallback. The chat backend was
then built against a dead endpoint with no on-screen indication anything had
changed — the user only discovered it from a failed completion request.

Fix

• app/chat.rs: two pure, I/O-free helpers.
  • stale_chat_url_replacement — given the configured base URL, whether its
    probe succeeded, and an already-detected live LlmConfig, returns
    Some(live) only when the configured endpoint is unreachable and a
    genuinely different live endpoint exists.
  • normalize_base_url — drops a trailing / and /v1 so a formatting-only
    difference (http://h:8000 vs http://h:8000/v1/) is not mistaken for a
    server change.
• app/mod.rs: wires this into the Configured-but-unreachable branch of the
  startup path in event_loop(), after discover_configured_chat_model. A
  replacement, if found, is substituted via stale_replacement.or(llm) and a
  ChatTurn::system(...) notice is pushed into the transcript.

Credential safety

The swap additionally requires chat_api_key.is_none() && chat_auth_header.is_none().
The replacement is a keyless detected_llm_config, so a remote gateway URL plus
OPENAI_API_KEY that merely TCP-times-out must never silently fall back to a
local keyless engine and drop the credential. That confines the swap to the true
target: a dead persisted local endpoint.

Precedence preserved

resolve_llm_config's CLI>config>env>probe precedence is not modified. The
substitution happens purely at the call site, swapping in a different
LlmConfig after 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. chat_env_url is a separate, lower-precedence tier and is
untouched.

This PR also corrects the ResolvedArgs::chat_url doc comment, which claimed
the field was a "CLI-flag value already merged over config". There is no
--chat-url flag anywhere in apps/rocm/src/main.rs; apps/rocm/src/dash.rs
sources the field solely from persisted tui.chat_url. That is precisely the
tier this change revalidates, so the comment directly contradicted the
invariant the fix relies on.

Startup cost on the degraded path

When a configured chat_url is unreachable with no key/header, detect_local_chat
now runs after the initial probe, adding a bounded one-time cost (~300 ms probe
plus detection) before the first frame. This is paid only in that specific
failure case; the healthy path is unchanged.

Rebase note

Previously this branch carried a hand-merge of PR #97. #97 has since merged
upstream (8308450), so the branch has been rebased onto current main and the
duplicated #97 commits dropped. What remains is a single logical commit
containing only this fix, replayed against main's current startup flow
(startup_chat_outcome / discover_configured_chat_model).

Relates to EAI-7360.

Test Plan

New unit tests (all in app/chat.rs):

• 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
• stale_chat_url_replacement_ignores_trailing_slash_and_v1_formatting
• normalize_base_url_strips_trailing_slash_and_v1

Local gates on the rebased head:

• cargo clippy --workspace --all-targets -- -D warnings (clean)
• cargo test --workspace --all-targets (all green; rocm-dash-tui lib:
  612 passed, 0 failed)
• python3 scripts/smoke_local.py (smoke: ok)

Not run locally: GPU and Strix Halo E2E lanes (no such hardware on this host) —
those are covered by CI.

[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.

[rominf]

Automated review: re-reviewed the delta since my approval at ed0b00e5 through current head e8f896c6. The unreviewed code is the hand-resolved merge of PR #97 into this branch, concentrated in app/chat.rs and app/mod.rs.

I traced the rewritten startup path: the llm rebinding preserves precedence; configured-endpoint model discovery and stale-URL replacement are mutually exclusive on probe success; neither clobbers the other; detection is not repeated; and the previously approved helpers and tests remain intact. I found no blocking code defect in the delta.

Non-blocking:

1. The PR body still says no overlapping hunks were expected, but both files required manual conflict resolution. Please update that coordination note.
2. The helpers are unit-tested independently, but no integration test covers their composed ordering in event_loop, where the merge occurred. That would be useful follow-up coverage.

Merge readiness: the PR currently conflicts with main, and the GPU and Strix Halo/Windows E2E checks are red. I could not verify those failures as infrastructure flakes from the available log tail, so they should be inspected or rerun before merge.

[rominf]

The core idea is right and the scope is tight. Two things need addressing before this lands: the remediation the notice tells the user to run doesn't work, and the change hand-rolls a narrower version of a mechanism that already exists and would make it work.

Blocking

1. The notice instructs a command that can't work — crates/rocm-dash-tui/src/app/mod.rs:1820-1826

The message ends with "Run /detect save to persist it." But /detect save is gated on chat_detect_offer.is_some() (app/slash.rs:491-496), and this path never populates it — it calls state.set_chat_config(llm, ...) (mod.rs:1828), which only sets chat_consent/chat_llm (app/chat.rs:25-32). A user who follows the instruction gets back "no detected endpoint to act on yet — run /detect first to probe for a local engine." So the swap can't actually be persisted, which is the one thing a user hitting this will want.

2. This duplicates the existing detect-offer flow — mod.rs:1815-1828 vs mod.rs:1143-1166

set_detect_result(Some(cfg)) already stores the offer and pushes a transcript message naming endpoint and model with the working /detect accept | save | dismiss sub-commands. Routing the detected live config through it would reuse the tested reducer, make (1) disappear, and give the user accept/dismiss instead of a silent fait-accompli swap. As written there are two divergent paths for "we found a different local engine", which will drift. (Note set_detect_result only pushes the message when consent is Accepted — worth checking that fits the startup path, but the offer is set unconditionally, which is what /detect save needs.)

3. Doc comment contradicts the code — app/chat.rs:305-306

"…this only ever supplies a replacement config before that call runs." The substitution happens after resolve_llm_config (mod.rs:1766 → 1816 → 1827), and mod.rs:1799 in this same commit correctly says "after that call." One of the two is wrong.

Non-blocking

4. An explicit chat_model is silently dropped on the recovery path — mod.rs:1827, chat.rs:311-321. stale_chat_url_replacement returns the detection result verbatim, whose model comes from /v1/models or the default. The sibling reachable path (discover_configured_chat_model, chat.rs:413-426) goes out of its way to honor an explicit model. Probably the right call for a different server, but it's an undocumented asymmetry and it isn't covered in the long precedence comment above.

5. The adjacent doc line has the exact bug this PR fixes one line above — mod.rs:87-88. The chat_url doc correction is accurate; chat_model still says "CLI-flag value already merged over config" and there's no --chat-model flag in apps/rocm/src/main.rs either. One-line fix, in scope.

6. Dead condition and dead argument — mod.rs:1809-1816. should_detect_local_chat returns false whenever chat_url.is_some() (chat.rs:268), so the == Configured check is redundant given the args.chat_url.is_some() conjunct. And stale_chat_url_replacement(&probe_target, probe_ok, live) passes probe_ok from inside an if !probe_ok branch, so chat.rs:316-318's early return is unreachable from the only production caller — it exists for one unit test. Defensible as a general helper signature, but worth a note.

7. localhost vs 127.0.0.1 reads as a server change — chat.rs:327-330. normalize_base_url compares raw strings; the default base URL uses 127.0.0.1 while the detection candidates use localhost. Same server, different spelling, spurious swap plus notice. Unlikely in practice since persisted URLs come from the candidates, but it's precisely the false-positive this helper exists to prevent, and it's untested.

8. A 300ms TCP-connect probe now has a larger blast radius — llm.rs:176-188. probe_endpoint only does connect_timeout; it says nothing about /v1/chat/completions working. Previously a false "unreachable" just meant "skip the default fallback"; now it substitutes a different endpoint. A server still binding, or an IPv6/IPv4 ordering quirk, can take a user off their real endpoint. Worth a retry or an HTTP-level check before acting on a negative probe.

9. The risky part is untested — mod.rs:1809-1828. All six new tests exercise stale_chat_url_replacement, a two-line string comparison. The four-condition guard, the detect_local_chat call, the notice, and the stale_replacement.or(llm) sequencing — the wiring that can regress — have none. stale_chat_url_replacement_none_when_nothing_live_found (chat.rs:702-708) is effectively asserting Option::filter(None).

10. normalize_base_url allocates needlessly — chat.rs:329. Every step yields a sub-slice; returning &str avoids the to_string(). Cosmetic.

Worth calling out as good

The chat_api_key.is_none() && chat_auth_header.is_none() gate (mod.rs:1812-1813) is the right instinct and correctly reasoned — a keyless local swap replacing a credentialed remote gateway would have been a real regression, and it was anticipated rather than found later. Extracting the decision into a pure I/O-free helper is also the right shape; the gap is that the untested part is the part that isn't pure.

Also noted, not asks

Startup latency on the degraded path grows by a full detect_local_chat on top of the probe (bounded, failure-path only, and disclosed in the description). A server dying mid-session still yields a bare ChatError via on_chat_error (mod.rs:1257) with no re-probe — reasonable scope-narrowing, but the failure class is half-covered.