fix: surface conversation run errors inline in chat#711
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste - Elegant, simple solution
This PR fixes a real user-facing issue where conversation errors (e.g., LLM authentication failures) were not visible in the chat. The solution is sound:
- Renders
ConversationErrorEvent/ServerErrorEventinline in the transcript - Removes the fragile error banner approach (which was only set from WebSocket and cleared on reconnect)
- Works for both REST-loaded history and live events
- Good test coverage for both the renderer and filter logic
- No breaking changes or complexity issues
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
UI-only change that makes error display more robust. Simple, localized changes with good test coverage. Fixes a real bug where failed conversations appeared silent to users.
VERDICT:
✅ Worth merging - Solves a real problem with minimal, well-tested changes.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26237093016
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste - Elegant, simple solution
This PR fixes a real user-facing issue where conversation errors (e.g., LLM authentication failures) were not visible in the chat. The solution is sound:
- Renders
ConversationErrorEvent/ServerErrorEventinline in the transcript - Removes the fragile error banner approach (which was only set from WebSocket and cleared on reconnect)
- Works for both REST-loaded history and live events
- Good test coverage for both the renderer and
shouldRenderEventlogic - Clean, low-complexity implementation following existing patterns
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
UI bug fix that surfaces previously hidden error states. No breaking changes, no security concerns, well-tested.
VERDICT:
✅ Worth merging: Fixes silent failure mode with minimal, focused changes
KEY INSIGHT:
Inline rendering of error events is more robust than banner state because it persists in the chat history regardless of how events are loaded (WebSocket vs REST) or subsequent UI state changes.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26237266232
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. Warning One or more snapshot tests crashed during generation — some snapshots below may be incomplete. ✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
| } | ||
|
|
||
| // Conversation/server error events (e.g. an LLM AuthenticationError) | ||
| if (isConversationErrorEvent(event) || isServerErrorEvent(event)) { |
There was a problem hiding this comment.
Just above this, I see agent error events, those fleeting errors like LLM sending the wrong parameters for a tool call. They’re very different than conversation/server errors, in that the first aren’t even errors from the perspective of the application - the agent will just continue and do presumably better next step.
In the video though, it seems this displays the “real” errors under the name “agent error”, and I wonder if we can avoid doing that?
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you for this PR! This is a headache 😓
Code looks good, I do have a tiny question though: I think the video shows that we tell the user and ourselves that conversation/server errors are “agent errors”, but they are not… Confusing them has been a painful source of bugs. Could we display somehow that conversation errors are conversation errors, while agent errors are agent errors?
I sympathize with giving up the banner… on the other hand, it kinda was fulfilling the purpose to make a distinction somehow?
3 participants
Why
The agent-server does emit the error — verified by inspecting a failed conversation's persisted events, which contained a ConversationErrorEvent (code: AuthenticationError, full detail). The gap was purely frontend display:
lands on the conversation the error is already in history → nothing shown.
Summary
Renders ConversationErrorEvent / ServerErrorEvent (e.g. the litellm AuthenticationError raised when an LLM profile has no valid API key) inline in the chat transcript, so a failed run is visible and actionable instead of dying
silently.
Addresses Bug 3 of #640: "Starting a conversation with an incorrectly configured LLM profile doesn't seem to show an error the user can act on."
Issue Number
#640
Testing
Video/Screenshots
Screen.Recording.2026-05-21.at.17.46.07.mov
Type
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.23.0-pythonopenhands-automation==1.0.0a331c197d1c58a98df9ef2e28dc3dd1a310fb7c6f0Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-31c197dRun
All tags pushed for this build
About Multi-Architecture Support
sha-31c197d) is a multi-arch manifest supporting both amd64 and arm64sha-31c197d-amd64) are also available if needed