Skip to content

fix(dash): rebuild live chat agent when a local endpoint is accepted - #81

Merged
michaelroy-amd merged 8 commits into
mainfrom
fix/chat-stale-agent-routing
Jul 10, 2026
Merged

fix(dash): rebuild live chat agent when a local endpoint is accepted#81
michaelroy-amd merged 8 commits into
mainfrom
fix/chat-stale-agent-routing

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Problem

In the dash TUI chat, accepting a detected local endpoint updated the config (chat_llm) but never rebuilt the live agent object that actually issues completions. The agent is built once at startup and only ever reassigned on a /provider switch. So after a user selected a local server (e.g. a Vulkan-backed lemonade running qwen3-0.6b), chat kept routing to the startup backend.

When that startup backend was a cloud gateway (an Azure API Management endpoint configured via chat_url + tui.chat_auth_header = "Ocp-Apim-Subscription-Key" + an env API key), every submit hit the gateway and returned:

chat request failed: ... 401 Unauthorized "Access denied due to invalid subscription key"

The UI showed the local endpoint as selected; the request physically went to Azure. chat_llm and the live agent had silently diverged.

Fix

Close the divergence with a one-shot chat_endpoint_rebuild edge (mirrors the existing chat_persist_dispatch / provider_switch edge-drain pattern):

  • accept_detect_offer (and save_detect_offer via delegation) raises the edge and selects ChatProvider::Local.
  • A new event_loop drain — placed after the /provider switch drain and before the chat_dispatch drain (ordering is load-bearing: otherwise the first post-accept submit would still use the stale agent) — rebuilds the live agent from the auth-free local chat_llm and refreshes the local_agent restore snapshot, so a later /provider local restores the accepted backend rather than the stale startup one. On build failure the agent is left unchanged and an error turn surfaces (mirrors /provider failure semantics).

Defense-in-depth

resolve_llm_config now strips api_key/auth_header when the resolved base_url host is loopback (127.0.0.0/8, localhost, ::1). This prevents a startup-configured local endpoint from leaking a cloud credential — a distinct code path the detect-accept fix does not cover.

Note: this is an intentional behavior change. A user pointing at a local authenticating proxy that genuinely requires an auth header (e.g. a corp gateway bound to 127.0.0.1) will have that header stripped. This matches detected_llm_config's existing no-auth-for-local stance and is documented in-code.

Tests

  • Extended detect_offer_lifecycle_accept_switches_chat and save_detect_offer_accepts_and_raises_persist_edge to assert the rebuild edge is raised and the provider realigns to Localstarting from a non-Local provider so the realignment assertion is load-bearing rather than tautological.
  • Added loopback_base_url_strips_cloud_auth and remote_base_url_keeps_cloud_auth unit tests for resolve_llm_config.

Verification

  • cargo build -p rocm-dash-tui
  • cargo clippy -p rocm-dash-tui --all-targets -- -D warnings ✅ (clean)
  • cargo test -p rocm-dash-tui -- --test-threads=1 ✅ — 533 lib + 16 + 5 tests pass, 0 failed

Notes for reviewers

The fix was developed with an adversarial multi-agent review pass. The rebuild-drain ordering (before chat_dispatch) and the local_agent snapshot refresh are the subtle, load-bearing pieces. The event-loop drain itself is covered indirectly (consistent with the pre-existing /provider drain, which is also not unit-tested beyond build_chat_agent invariants).

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auto-detect-and-switch flow is a nice UX addition. Several correctness issues in the new drain path and the loopback auth stripping below.

Comment thread crates/rocm-dash-tui/src/app/mod.rs Outdated
Comment thread crates/rocm-dash-tui/src/app/mod.rs Outdated
Comment thread crates/rocm-dash-tui/src/app/mod.rs Outdated
Comment thread crates/rocm-dash-tui/src/app/mod.rs Outdated
Comment thread crates/rocm-dash-tui/src/llm.rs
Comment thread crates/rocm-dash-tui/src/llm.rs
Comment thread crates/rocm-dash-tui/src/llm.rs Outdated
Accepting a detected local endpoint updated `chat_llm` but never rebuilt
the live `agent` that actually issues completions. The agent is built once
at startup and only reassigned on a `/provider` switch, so after selecting
a local server (e.g. lemonade) chat kept routing to the startup backend —
a cloud gateway with an `Ocp-Apim-Subscription-Key` header — surfacing a
401 "Access denied due to invalid subscription key". The UI showed local;
the wire went to Azure.

Fix the divergence with a one-shot `chat_endpoint_rebuild` edge:
- `accept_detect_offer` (and `save_detect_offer` via delegation) raises the
  edge and selects `ChatProvider::Local`.
- A new `event_loop` drain — placed after the `/provider` drain and before
  `chat_dispatch` (ordering is load-bearing) — rebuilds the live `agent`
  from the auth-free local `chat_llm` and refreshes the `local_agent`
  restore snapshot so a later `/provider local` restores the accepted
  backend, not the stale startup one. Build failure surfaces an error turn
  and leaves the agent unchanged (mirrors `/provider` semantics).

Also harden `resolve_llm_config`: strip `api_key`/`auth_header` for
loopback base_urls so a startup-configured local endpoint cannot leak a
cloud credential (a distinct path the detect-accept fix does not cover).

Tests: extend the detect/save-offer tests to assert the rebuild edge and
the Local realignment (starting off-Local so the assertion is not
tautological); add loopback strip / remote keep unit tests for
`resolve_llm_config`.

Signed-off-by: Michael Roy <michael.roy@amd.com>
A portless bracketed IPv6 base_url like `http://[::1]/v1` bypassed the
loopback auth-stripping guard: `parse_host_port` fell through to
`rsplit_once(':')`, which splits *inside* the address (`[::1]` → host
`[:`, port `1]`), failed the port parse, and returned None — so the URL
was treated as remote and a cloud credential was forwarded to localhost.

Handle the bracketed IPv6 authority (`[host]` / `[host]:port`) explicitly
before the rsplit fallback, returning the bare host (`::1`). This also
fixes probe_endpoint, since to_socket_addrs rejects bracketed literals.

Addresses review comment (juhovainio) on llm.rs:133.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Now that parse_host_port strips the brackets from a bracketed IPv6
authority, is_loopback_host only ever sees a bare host, so the
`host == "[::1]"` comparison is unreachable. Keep the bare `::1` match
(now the live path) and drop the bracketed form; update the doc to state
the bare-host expectation.

Addresses review comment (juhovainio) on llm.rs:143.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Stripping api_key/auth_header for a loopback base_url is silent, so a user
running an authenticating local proxy just sees 401s with no on-host clue.
Emit a tracing::warn! naming the base_url, fired only when a credential was
actually present to discard.

Addresses review comment (juhovainio) on llm.rs:91.

Signed-off-by: Michael Roy <michael.roy@amd.com>
Operational notices like "switched to local" were pushed as
ChatRole::Agent, which build_messages maps to Message::assistant — so the
literal string was replayed to the model as a prior assistant turn on every
subsequent submit, corrupting its context with text it never generated.

Add a ChatRole::System variant (rendered with a `··` prefix in the muted
tone) that build_messages drops alongside Error. Route the operational
"switched to …" notices (both /provider drains and the endpoint-rebuild
drain) through ChatTurn::system.

Addresses review comment (juhovainio) on mod.rs:1930.

Signed-off-by: Michael Roy <michael.roy@amd.com>
The local-agent construction (RigAgentClient::new + Arc-cast to
dyn AgentClient) was spelled out inline at the event-loop startup build and
again in the endpoint-rebuild drain, so any change had to be kept in sync in
two places. Extract build_local_agent() as the single construction path and
use it at startup; it returns the AgentError so callers can discard it
(.ok()) or surface it as an error turn.

Addresses review comment (juhovainio) on mod.rs:1912.

Signed-off-by: Michael Roy <michael.roy@amd.com>
accept_detect_offer set active_provider = Local unconditionally before the
rebuild, but the drain's Err arm never restored it — so a failed
RigAgentClient::new left the tab showing "Local" while agent still pointed
at the old backend, silently routing submits to the wrong place.

Carry the previous provider on the rebuild edge
(chat_endpoint_rebuild: Option<ChatProvider>); on build failure revert
active_provider to it and surface an actionable error turn, mirroring the
provider_switch drain. Tests assert the edge carries the prior provider.

Addresses review comment (juhovainio) on mod.rs:990.

Signed-off-by: Michael Roy <michael.roy@amd.com>
The rebuild drain consumed the edge inside `if let Some(cfg) = chat_llm`,
so if chat_llm was None the flag was cleared, no agent was built, no error
turn was pushed, and active_provider stayed stuck on `Local` with no
feedback. Fold the None case into the shared `revert` path so it restores
the previous provider and surfaces an actionable error like every other
failure.

Addresses review comment (juhovainio) on mod.rs:1914.

Signed-off-by: Michael Roy <michael.roy@amd.com>
@michaelroy-amd
michaelroy-amd force-pushed the fix/chat-stale-agent-routing branch from 827fad9 to e51b984 Compare July 7, 2026 22:57

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested locally on WSL Ubuntu with Radeon 780M iGPU

Local model was detected:
Image

And responses are received as expected:
Image

@fredespi

fredespi commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Cross-reference: #88 (EAI-7220) touches the same chat-endpoint flow but at a different layer, so the two are complementary rather than overlapping.

End to end you need both: #88 makes rocm serve discoverable, #81 makes an accepted endpoint get used.

Overlap is minimal — the shared files (llm.rs, ui/tabs/chat.rs) touch different functions/regions and don't conflict. The one spot to watch is app/mod.rs: #88's edit to the refresh_detect failure message sits a couple of lines above this PR's accept_detect_offer rewrite, so whichever merges second will need a trivial adjacency fixup (no semantic conflict).

@michaelroy-amd
michaelroy-amd added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit 5bcd8b1 Jul 10, 2026
15 checks passed
@michaelroy-amd
michaelroy-amd deleted the fix/chat-stale-agent-routing branch July 10, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants