fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint by michaelroy-amd · Pull Request #105 · ROCm/rocm-cli

michaelroy-amd · 2026-07-11T00:40:09Z

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

A configured chat URL (OPENAI_BASE_URL / --chat-url) with no explicit model resolved to the "local-model" placeholder, which 404s on servers that register the model under its real id. Query /v1/models on startup to adopt the served model, mirroring the managed-service path. Gated on a reachable probe so an unreachable endpoint adds no fetch timeout to launch, and an explicit --chat-model/config value still wins. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>

…ive endpoint A persisted tui.chat_url survives across dash launches, but the server it points at may have been stopped or replaced since it was written (e.g. a managed vLLM/Lemonade instance was restarted on a different port). Previously the startup path passed a stale chat_url straight into resolve_llm_config regardless of reachability, so the agent was built against a dead endpoint with no indication anything had changed. Add stale_chat_url_replacement (app/chat.rs): a pure decision function that, only when the persisted chat_url's own TCP probe fails and no managed-services entry matched it, checks whether detect_local_chat finds a live, differently-addressed endpoint. If so, the startup path in event_loop() substitutes that live LlmConfig before calling resolve_llm_config and pushes a "server changed" ChatTurn::system notice into the transcript pointing at `/detect save` to persist it. This only ever revalidates the persisted-config tier: chat_env_url is untouched (it's excluded from the managed-registry check already), an explicit --chat-url wired through the same field would be revalidated identically today since this crate never distinguishes CLI from config at this layer, and resolve_llm_config's own CLI>config>env>probe precedence is not modified at all — the substitution happens purely at the call site, and 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) all remain green untouched. Complements unmerged PR #97, which addresses the chat_model half of this gap. Relates to EAI-7360. Signed-off-by: Michael Roy <michael.roy@amd.com>

…ormatting Two refinements to the stale persisted-chat_url revalidation: 1. Guard the substitution on chat_api_key.is_none() && chat_auth_header.is_none(). The replacement is a keyless detected_llm_config, and ResolvedArgs cannot distinguish an explicit --chat-url from persisted config, so a remote gateway URL plus OPENAI_API_KEY whose TCP connect merely times out (the 300ms budget is tight) could silently swap to a local keyless engine and drop the credential. Requiring no configured auth confines the swap to the true EAI-7360 target — a dead persisted local endpoint with no auth. 2. Normalize base_url before the change-detection compare (trim 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. Relates to EAI-7360. Signed-off-by: Michael Roy <michael.roy@amd.com>

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 · 2026-07-14T08:36:27Z

🔴 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

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

Reconcile PR #97 (configured-endpoint served-model discovery) with main's PR #100 startup local-first detection (EAI-7347). Conflicts in crates/rocm-dash-tui/src/app/{chat.rs,mod.rs} resolved as a union that keeps both behaviors: - Preserve main's local-engine detection: should_detect_local_chat, StartupChatOutcome/startup_chat_outcome, detect_local_chat_with_probe, and the #89 Press-only overlay key gate (is_actionable_key). - Port PR #97's /v1/models discovery for a *configured* chat endpoint into a new helper, chat::discover_configured_chat_model, wired only on the StartupChatOutcome::Configured path. Startup matrix (verified by unit tests in app::chat::tests): - Configured URL + no explicit model + reachable -> adopt served /v1/models id. - Configured URL + explicit model -> config precedence, never overridden. - Configured URL + unreachable -> never probed, never replaced (no fetch timeout; an unreachable explicit CLI/env URL is left untouched). - Local detected / OAuth paths unchanged from main. RED before the port: configured_endpoint_adopts_served_model_when_no_model_set failed against the merged-main stub (got "local-model", expected served id); GREEN after wiring the helper. Full workspace tests, clippy -D warnings, and scripts/smoke_local.py all pass. Signed-off-by: Michael Roy <michael.roy@amd.com>

…ale chat_url recovery) Layer PR #105's stale persisted-URL recovery onto PR #97's configured-endpoint model discovery (which already includes current main). Manually resolve the chat.rs/mod.rs conflicts, preserving both behaviors: - PR #97: discover_configured_chat_model() adopts a served /v1/models id for a reachable configured endpoint that has no explicit model (probe_ok path). - PR #105 (EAI-7360): stale_chat_url_replacement() swaps a dead persisted tui.chat_url for a genuinely-different live local engine on the Configured + !probe_ok path, emits a "server changed" notice pointing at `/detect save`, and normalize_base_url() treats trailing slash and /v1 formatting as the same endpoint. Gated on chat_url.is_some() + no api_key/auth_header so env URLs and authenticated remote gateways are never silently replaced. The two paths are mutually exclusive on the Configured branch's probe_ok, so they compose without interference; resolve_llm_config's CLI>config>env>probe precedence is untouched. Matrix verified: no URL/model -> local detection; reachable configured -> /v1/models discovery; stale persisted -> live replacement + notice; unreachable env/explicit URL -> fail honestly; explicit auth -> no silent fallback. Signed-off-by: Michael Roy <michael.roy@amd.com>

volen-silo and others added 3 commits July 10, 2026 10:58

rominf approved these changes Jul 13, 2026

View reviewed changes

michaelroy-amd added 2 commits July 15, 2026 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(chat): revalidate a stale persisted chat_url and fall back to a live endpoint#105
michaelroy-amd wants to merge 5 commits into
mainfrom
fix/chat-stale-url

michaelroy-amd commented Jul 11, 2026

Uh oh!

rominf left a comment

Uh oh!

volen-silo commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

michaelroy-amd commented Jul 11, 2026

Summary

Root cause

Fix

Precedence preserved

Coordination notes

Test Plan

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

volen-silo commented Jul 14, 2026

Summary

Verified as correct (the load-bearing claims)

Non-blocking suggestions

Tradeoffs (deliberate choices worth confirming, not change requests)

Positive signals

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants