Repository navigation
fix(acp): show agent details when a chat message fails - #26666
Conversation
Review summaryThe problem. When a chat agent (OpenCode, Grok, OMP or a future agent) fails a message, it often sends a generic "Internal error" and keeps its real reason in the error's data field. Orca read that field only for agents with a custom reader, so other agents' failures could show up as a bare "Internal error". What changes for you. Every agent's failure row now uses one rule: the agent's own reader if it has one, otherwise the text in the data field, otherwise its How this PR got here. The first version predated main's #26544 and #26649, which added sign-in handling and already show OMP's no-model steps. It was rebuilt on main as one shared rule. The rebuild dropped the earlier agent-name prefix and the "Internal error:" prefix so the sign-in row doesn't name the agent twice, and dropped a raw-JSON fallback after a real OpenCode error showed its data is only metadata. Fixed in review: nothing; the fresh adversarial review found no proven issues.
Verified:
Not verified live:
|
Agents often answer a failed session/prompt with a generic JSON-RPC message ("Internal error") and keep the real reason in error.data. Only OMP and Grok had hooks that read it, so OpenCode and future ACP agents showed the bare message.
The prompt error detail now reads, for every ACP agent: the agent's hook, else string data, else data.details, else the message. Other structured data is metadata and is ignored. OMP's details hook and Grok's string-data branch are now covered by that rule and removed.
3bfd1d5 to
45a92e5
Compare
📝 WalkthroughWalkthroughACP prompt error details now use dialect-specific handling, followed by nonblank string data or details, and then Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Failure rows now show agent-supplied details. A narrow edge case remains where Grok sends a blank message alongside details. It is cosmetic and safely falls back to blank text. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
df9365b6-b91f-4225-96d9-ee421cb3b017
📒 Files selected for processing (5)
config/scripts/vitest-sqlite-runtime-files.mjssrc/main/acp/acp-dialects/grok-dialect.tssrc/main/acp/acp-dialects/omp-dialect.tssrc/main/acp/acp-structured-prompt-failure.test.tssrc/main/acp/acp-turn-failures.ts
💤 Files with no reviewable changes (1)
- src/main/acp/acp-dialects/omp-dialect.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| * message ("Internal error") and keep their own words in `data`. */ | ||
| export function acpPromptErrorDetail(dialect: AcpDialect, error: AcpAgentError): string { | ||
| return dialect.promptErrorDetail?.(error) ?? error.message | ||
| return dialect.promptErrorDetail?.(error) ?? acpErrorDataWords(error.data) ?? error.message |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat a blank dialect detail as absent.
If Grok returns data: { message: '', details: 'Quota exceeded' }, its reader returns ''. The ?? expression then skips acpErrorDataWords, so the failure row loses the readable detail. Check that the dialect result is nonblank before selecting it.
1 participant
ELI5
When a chat agent such as OpenCode, Grok or OMP fails a message, it often answers with a generic "Internal error" and puts its real reason in a separate details field. Orca only read that field for agents that had their own custom reader, so any other agent's failure could show up as a bare "Internal error". Now Orca reads the agent's reason the same way for every agent.
What Changed
Problem. These agents talk to Orca over the Agent Client Protocol (ACP). When an agent fails a message, its answer carries a short message (often just "Internal error") and an optional data field. Many agents put their actual reason in that data field: as plain text, or as a
detailstext field. Until now Orca only looked there for agents that had a custom reader written for them (OMP readdetails; Grok read plain text ormessage). Every other agent, including future ones, showed only the short message.Change. One shared rule now decides the words in the failure row, for every ACP agent:
data.messagereader);data.details, when it is non-empty text;Structured data that has no words for a person (for example OpenCode's
{"service":"session","errorName":"APIError"}) is ignored, so the row never shows raw JSON. OMP's custom reader and Grok's plain-text branch are removed because the shared rule now covers them. "Not signed in" detection and its "Sign in to ." row are unchanged.What you see. For the agent failures recorded so far, the row reads the same as before: OMP's no-model failure still shows "Sign in to OMP." followed by OMP's own steps, and OpenCode still shows its own message. The difference is for any agent (or a future version of one) that puts its reason in the data field without a custom reader: you now see its words instead of "Internal error".
Why
The bug was per-agent: each agent needed its own reader, and an agent without one lost its reason. One rule for every agent removes that class of bug instead of adding a fourth reader. It lives where the failure row's text is already built, so failed turns, messages refused before a turn starts, and sign-in failures all get the same words.
Intended difference from the common pattern: the common pattern appends the error data as raw JSON when it has no text field. Orca leaves structured data out instead. A real OpenCode failure's data is only metadata (
service,errorName), and raw JSON in a row the user reads is noise; the agent's own message is kept.Intended: text in the data field takes priority over the short message. An agent that put a specific message in
messageand only a code indetailswould show the code. None of OMP, Grok or OpenCode is known to answer that way.Earlier drafts of this PR also prefixed the agent name and kept "Internal error:" in front of the details. Both were dropped when this was rebuilt on main after #26544 and #26649 added sign-in handling, so the sign-in row doesn't name the agent twice.
Linked Issue
Follow-up to #26544 and #26649 (sign-in guidance, which already show OMP's no-model steps). Independent of #26587, which fixes starting an OMP chat when OMP reports no model.
Visual Proof
Windows high spec, real OpenCode 2.0.14 (signed out, isolated home), hidden Orca built from this head. The model "opencode/Claude Haiku 4.5" was picked in the chat's model picker, then one "Hello" was sent. OpenCode answered with message
Internal error: Model unavailable: opencode/claude-haiku-4-5and data{"service":"session","errorName":"provider.no-route"}. The row shows OpenCode's message and no JSON:There is no before/after pair: for this failure the row text is the same as before. What changes is that this structured data is deliberately left out, and that agents with words in their data field now show them; no installed agent we have answers that way, so that part is shown by tests only.
OMP's live row is not shown here. On current main an OMP with no model can't start a chat at all ("Chat could not be started."), which #26587 fixes. Once #26587 lands, OMP's failure row comes through the shared rule. Until then, main's sign-in test, which is unchanged here, covers it.
Testing
I manually tested these changes locally (Windows, OpenCode, above)
Automated tests added/updated
New
acp-structured-prompt-failure.test.ts(12 tests), through the real protocol reader, translator and SQLite chat journal. It covers OpenCode and Grok with the reason indata.details, as plain-text data, and with no data, plus the recorded real OpenCode error shape (no JSON in the row). It also covers a sign-in failure with details ("Sign in to …" plus the agent's words), and a message refused before its turn starts.103 of 103 tests pass across the 12 related ACP test files, including main's
acp-structured-sign-in.test.ts, unchanged (OMP's "No model selected" row still contains OMP's full steps).orca-ci-checks23/23, including typecheck. CI on this head: 33 passed, 20 skipped (skipped is not counted as passed), none failed.Live check above: Windows only. Not checked live: macOS, Linux, SSH, mobile, Grok, and OMP (blocked by fix(acp): start chats for agents that report no model #26587, see above).
Review
A fresh adversarial review, with an independent design sketch, found no proven issues. Deferred minor points: Grok's reader returns a blank
messagewithout falling through todetails; that is the same as before. Some edge cases have no tests yet: whitespace-only data, and the 512-character limit on this path.Agent skill upstream boundary
Notes
The change runs on the machine that runs the agent and only changes the words in an existing row. There is no new wire field, stored field, migration or setting, so older paired clients and SSH hosts just show the new words. Folder workspaces and git worktrees behave the same. One small change: if an agent sends blank text as its data or details, the row now shows its short message, where before OMP's blank details fell through to "OMP ended this turn with an error."
Checklist