feat(chat): explain why a reply is stalled when the endpoint isn't ready

[michaelroy-amd]

Stacked on #101 (EAI-7352). Review/merge #101 first; this PR's base is fix/serve-ready-promotion, so its own diff is just the chat-readiness work.

Summary

Sending a message to a local endpoint whose model was still coming up showed only a generic ⠿ waiting for the agent… spinner — indistinguishable from a slow reply or an outright hang. The chat input never consulted the readiness of the instance it was pointed at.

Root Cause

crates/rocm-dash-tui/src/ui/tabs/chat.rs::draw_input() rendered a fixed "waiting for the agent…" line whenever chat_sending was set, regardless of whether the backing endpoint was actually up. The dashboard already tracks each managed instance's status in AppState.instances, but chat made no use of it.

Changes

• draw_input() now matches the chat endpoint to its daemon-surfaced instance (by port parsed from the configured base_url) and, when that instance is not answering, replaces the generic spinner with a specific reason:
  • Starting → "the model is still starting up — hang tight"
  • Stopped → "the endpoint has stopped — restart the service to chat"
  • Error → "the endpoint reported an error — check the service logs"
• A live (Running) instance, a remote gateway URL (no local instance to inspect), an unmatched port, or an Unknown state all fall back to the existing plain spinner — unchanged behavior.
• Parsing and the reason mapping are pure, unit-tested helpers (port_from_base_url, chat_backend_wait_reason), so the render path stays thin.

Scope / Notes

• This PR is based on fix(serve): persist running->ready promotion in CLI liveness refresh #101 (which adds the running→ready promotion), so it sees the original InstanceStatus enum. The phase-aware startup detail (DOWNLOADING/LOADING/WARMUP from EAI-7355 / feat(serve): surface a coarse startup phase while a service comes up #107) lives in the other stack; if both land, the reason text here naturally benefits from the richer Starting state without further changes to this file.

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 — 547 + 16 + 5 green, incl. new tests: port_from_base_url_parses_local_endpoints, backend_wait_reason_reflects_instance_status, sending_input_surfaces_startup_reason_not_generic_spinner, sending_input_falls_back_to_generic_spinner_for_remote_endpoint

Relates to EAI-7348

[rominf]

The core logic is correct and well-tested — the readiness reason is recomputed live each frame (no staleness once the endpoint becomes ready), the match over InstanceStatus is exhaustive (no wildcard), and Running/Unknown return None so a slow-but-live model is never falsely labeled "stalled". Two quality issues worth fixing:

1. Misattached doc comment — crates/rocm-dash-tui/src/ui/tabs/chat.rs:276. The inserted functions pushed draw_input's doc comment (/// Render the single-row input line…) onto port_from_base_url, so cargo doc/hover now describes the port parser as an input renderer, and draw_input is left undocumented. Move the comment back down to draw_input.

2. IPv6 regression from reimplemented URL parsing — chat.rs:281-303. host_from_base_url / port_from_base_url duplicate llm::parse_host_port but drop the IPv6 bracket-stripping it already does: http://[::1]:8000/v1 → host "[::1]" → is_loopback_host returns false → the whole function bails to None, so a user on an IPv6-loopback local instance never gets the readiness reason (the exact case this PR targets). is_loopback_host's own doc says it expects a bracket-stripped host from parse_host_port. Reuse parse_host_port (with an explicit-port check layered on top) instead of the two new parsers.

Neither blocks merge if IPv6 chat targets are out of scope, but both are easy fixes.

[volen-silo]

🔴 Automated review · pr-review-watcher · a068711

Summary

• Change: adds chat_backend_wait_reason to the rocm-dash-tui chat tab so an in-flight request over a not-yet-ready local endpoint shows a specific reason ("still starting up", "stopped", "error") instead of the generic ⠿ waiting for the agent… spinner. Head commit a068711 narrows the endpoint→instance match from port-only to host+port (loopback-gated). Scope: 3 files, +198/-15; THIRD_PARTY_NOTICES.txt reorder is unrelated (disclosed).
• Assessment: Needs work. The tests are green (549 passed locally) and the loopback-gating fix is a genuine correctness improvement, but the feature does not fire in its own headline scenario on this base branch, and the new URL parsers reintroduce an IPv6 bug the crate already solved.
• Blocking: 2. Non-blocking: 3. Tradeoffs: 1.

Blocking

1. Feature is a no-op in its motivating case; Starting/Stopped/Error arms are unreachable in production on this base

crates/rocm-dash-tui/src/ui/tabs/chat.rs:316-323

The whole point (per the PR title/summary) is to explain a stall while a local model is still coming up. That case presents as InstanceStatus::Running, which maps to None (no explanation) — so the user still sees the generic spinner in exactly the scenario this PR targets.

Verified against source: every production Instance reaching the TUI is built by the daemon's instance_from_discovered (crates/rocm-dash-daemon/src/runner.rs:597-599), which sets inst.status = InstanceStatus::Running unconditionally for any scrapeable record. is_scrapeable_status (crates/rocm-dash-daemon/src/registry.rs:72-77) collapses ready | running | starting into that single Running. The TUI takes those verbatim (app/mod.rs:1334-1340 push_snapshot, "treat them as truth"). A grep across the whole non-test tree confirms no production path ever emits Starting, Stopped, or Error — every non-Running InstanceStatus:: reference is inside a #[test] module. So three of the four reason arms are effectively dead code today, and the codebase's own readiness check (pick_managed_chat_endpoint, app/chat.rs:149-154) proves the project treats "running" ≠ "ready" by requiring the literal "ready" string — a distinction InstanceStatus cannot express.

The PR description discloses this ("if both land, the reason text here naturally benefits from the richer Starting state" — depending on sibling #107 / EAI-7355). That is honest, but it means the merged-on-its-own result delivers no user-visible change in the target scenario and ships unreachable branches whose only coverage is tests that inject statuses production never produces (chat.rs:553-561). Recommend either landing the readiness-signal dependency first, or having chat_backend_wait_reason consult the registry's raw "ready" vs "running" string (as pick_managed_chat_endpoint does) rather than the collapsed InstanceStatus enum.

2. New URL parsers silently drop IPv6 loopback — reimplementing an existing correct helper

crates/rocm-dash-tui/src/ui/tabs/chat.rs:281-292

port_from_base_url and host_from_base_url hand-roll authority parsing with rsplit/rsplit_once(':'), which is wrong for bracketed IPv6:

• host_from_base_url("http://[::1]:8000/v1") → "[::1]" (brackets retained), and is_loopback_host("[::1]") is false (it only matches bare "::1"). The loopback gate rejects a legitimate IPv6-loopback local endpoint → feature silently disabled.
• port_from_base_url("http://[::1]/v1") → rsplit(':').next() yields "1]", .parse::<u16>() fails → None, so chat_backend_wait_reason short-circuits before the host is even considered.

The crate already has pub fn parse_host_port(base_url: &str) -> Option<(String, u16)> (crates/rocm-dash-tui/src/llm.rs:121-160) that handles scheme, path/query stripping, default ports and bracketed IPv6 (with a dedicated regression test parse_host_port_handles_bracketed_ipv6, added precisely because portless bracketed IPv6 used to bypass stripping). is_loopback_host's own doc says it "Expects a bare host as returned by parse_host_port, which strips the brackets." This file already calls crate::llm::is_loopback_host, so parse_host_port is equally reachable. Replacing both new helpers with a single let (host, port) = crate::llm::parse_host_port(base_url)?; removes ~20 lines, fixes the IPv6 bug, and eliminates a contract divergence (missing-port: parse_host_port defaults 80/443, port_from_base_url returns None). Note this also means most of the new parser unit tests become redundant with the existing llm.rs suite.

Non-blocking

• Doc comment overstates the Running guarantee (chat.rs:299-306): "returns None when the endpoint looks live" — but Running does not guarantee the endpoint answers HTTP (see finding 1). Comment rot that will mislead maintainers into trusting Running as a readiness signal.
• Test coverage quality (chat.rs:539-597): backend_wait_reason_reflects_instance_status and sending_input_surfaces_startup_reason... inject InstanceStatus::Starting directly, a state production never produces — so they pass while the real code path they represent cannot occur. No test uses a bracketed-IPv6 URL, so the finding-2 regressions ship undetected. The is_loopback gate test (backend_wait_reason_ignores_remote_endpoint_sharing_a_local_port_number) is genuinely meaningful and does exercise the head-commit fix.
• Sibling-PR coupling not enforced (chat.rs): the value of these arms depends on feat(serve): surface a coarse startup phase while a service comes up #107 / EAI-7355 landing. Nothing in-tree records that dependency; if feat(serve): surface a coarse startup phase while a service comes up #107 slips, this code sits dormant with no signal. Consider a code comment or a linked-issue guard so the gap is discoverable.

Tradeoffs

• Forward-compatible stub vs. no-op-until-dependency. The author deliberately wrote the full Starting/Stopped/Error mapping now so it "naturally benefits" once a richer status lands, rather than growing the match later. The cost is shipping arms that are unreachable on the current base and a feature that is invisible in its headline case until the sibling stack merges. Neither is wrong — but reviewers should confirm the intent to merge this ahead of the status-signal work is deliberate, since on its own it changes no user-visible behavior for a starting local model.

Positive signals

• Head-commit fix (host+port matching, loopback-gated) correctly closes a real false-match: a remote gateway sharing a local port number no longer borrows an unrelated instance's status. The accompanying ..._ignores_remote_endpoint_sharing_a_local_port_number test pins exactly that scenario.
• is_loopback_host visibility change is a clean, minimal pub(crate) (no collision — it was already crate-internal and used at llm.rs:90).
• PR description is thorough and honestly discloses the stacked base and the status-signal dependency.

---

Automated review, REVIEW-ONLY. No approval/request-changes/merge. Findings verified against source at a068711f; local cargo test -p rocm-dash-tui = 549 passed / 0 failed.

[rominf]

Please have a look at automated review by Eugene above.

[rominf]

Automated review: re-reviewed at head 72391ae6, covering the commit added since my previous review. Both requested fixes are resolved.

• The draw_input documentation is attached to the correct function again.
• chat_backend_wait_reason now uses the shared parse_host_port path, which correctly strips brackets from IPv6 authorities before loopback detection. The new bracketed-IPv6 regression test exercises the previously broken input and passes.

The relevant rocm-dash-tui tests pass. I found no remaining code defect in this follow-up. The PR is still conflicting with main, so it must be rebased and the mechanical conflicts resolved before merge.