Repository navigation
fix(native-chat): decide provider failures by kind, never by searching the payload - #29092
Conversation
…g the payload A Claude chat opened with a red "claude · message:system:init" card whenever one of the user's MCP servers failed to connect. The classifier searched every nested field of a frame for failure-looking values and let that outrank the per-kind catalogue, so the session-start roster's `status: "failed"` turned the whole frame into an error row. The same search caused the earlier failed-background-task opcode cards. A frame is now a failure only because its kind is one, or because a field its kind declares as its outcome says so (Claude `hook_response.outcome`, Codex `hook/completed` `run.status`). A Claude turn result is a failure when the result journaling says so, since that depends on whether the user stopped the turn. With nothing left to override, the typed-translator coverage list, its option and the task-frame sentence seam go. Failed MCP servers write no row for Claude or Codex.
|
A failed MCP server made new Claude chats show a red The two-round review found and fixed one regression introduced by the change: a Claude sign-in status error had lost its row. The final code reads that frame's declared error field, and its focused regression test passed. Deferred concerns are future uncatalogued frames with undeclared errors, malformed background-task frames without task identity, and a pre-existing Codex blocked/stopped hook visibility difference. Plugin-install progress remains quiet by design. Exact-head QA passed on M4Air. Fresh hidden Electron builds compared PR head Current-head CI passed. The full PR Checks workflow run For Brennan's review: Compare the main and PR chat screenshots, then the Notes and Idle screenshots in the PR body. The main judgment point is the intentional silence for MCP startup failures in frame classification; Claude sign-in errors and stopped-versus-failed turn handling show the preserved failure paths. No merge was performed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 12 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check No active directly linked issue remains. Linked issue #25514is closed and provides historical context only. Therefore, this pull request has no active linked-issue coding requirements to fail.Out of Scope Changes check The changes remain within the pull request scope. They remove payload-wide failure promotion, add declared-outcome handling, pass Claude result failure decisions explicitly, and update Claude and Code… Title check The title clearly and concisely describes the main change: provider failures are classified by message kind instead of arbitrary payload contents. Description check The description is detailed and covers the user impact, implementation, rationale, linked issue reference, visual proof, testing, compatibility notes, and checklist. The required sections are present,… Full details: Out of Scope Changes check
-
Explanation
The changes remain within the pull request scope. They remove payload-wide failure promotion, add declared-outcome handling, pass Claude result failure decisions explicitly, and update Claude and Codex tests and fallback paths. The Codex MCP-startup behavior and background-task fallback changes are stated objectives, not unrelated changes.
Full details: Description check
-
Explanation
The description is detailed and covers the user impact, implementation, rationale, linked issue reference, visual proof, testing, compatibility notes, and checklist. The required sections are present, although the linked issue is referenced as
#25477rather than provided as an explicit issue link.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Author: @BrennanKB5
ELI5
If one of your MCP servers can't connect, every Claude chat opened with a red error card titled
claude · message:system:init. Nothing had actually gone wrong with the chat. Orca was reading a list inside Claude's "session started" message, saw the wordfailednext to one server, and decided the whole message was an error. Orca no longer hunts through a message for failure-looking words. A message is an error only when its type is an error, or when the field that message type uses to report its own outcome says it failed.What Changed
What users see
claude · message:system:initcard. Its Details held the raw startup message. Users in [Bug]: Agent chat shows error card "claude · message:system:init" instead of session init message #25477 saw it on every session.claude mcp list/codex mcp listshow why).error,warning, Claudepermission_denied, …).How it works
classifyProviderFrameused to runhasProviderErrorfirst. That function walked every nested field of a message looking forstatus: "failed",error: …oris_error: true, and its answer overrode the per-type table. That function is deleted. Classification now comes only from the message type, which is the common pattern.hook_response.outcome === 'error', Claudeauth_status.errorwhen it has a message, and Codexhook/completedrun.status === 'failed'.claudeResultFailure), so it now passes its verdict to the fallback row asreportsFailureinstead of relying on the search to agree.coveredByTypedTranslatoroption, and the sentence helper for background-task messages that name no task. That previous bug was the failed background task that showed asclaude · message:system:task_notification(fix(native-chat): give a failed Claude background task a typed row instead of an opcode #20519).Why
This is the second bug with the same cause. The first was the failed background-task card. It was fixed by exempting those message types from the search, and this one would have been fixed by exempting
system:init(#28857). The next message that carries a list of things with their own statuses would hit it again. Removing the search removes the whole class.Dropped alternatives:
system:init(fix(native-chat): replace the Claude init error card with an actionable MCP connect warning #28857, and the closed fix(claude): keep the init handshake out of the error card #25514): fixes this one message type and leaves the search in place for the next one.statusdescribing something other than itself would misfire.Behavior removed on purpose:
Linked Issue
Tracked in #25477
An alternative to #28857.
Visual Proof
Same Claude chat, same broken MCP server (one user server whose command does not exist), built from source on a separate Mac with a throwaway home. Both versions also show a genuine "Claude isn't signed in" card, because the test used a deliberately invalid API key.
mainef563bc)The startup message the before card came from carried only
mcp_servers: [{"name":"broken","status":"failed","source":"user"}]. Reopening the old chat in the fixed build still shows its stored card (history is not rewritten); its stored row was byte-for-byte unchanged.Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
provider-frame-disposition.test.ts: the startup message with the reporter's realmcp_serversroster writes no row. Nested failure-looking values (task notification, Codex MCP startup, an uncatalogued type) never become error rows. Hook failures and a Claude sign-in status error still surface from their declared outcome fields.Tests that pinned the old search (failed startup variants, failed delta-shaped messages, covered-type options) are removed or rewritten to the new rule.
Electron, macOS: reproduced on
mainand verified on this branch in a real Claude native chat, driven only through the DevTools protocol on a hidden window, with an isolated home. A new chat on the fix writes no startup card; the reopened old chat keeps its stored one.Review
Agent skill upstream boundary
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)M4Air screenshot comparison
Hidden DevTools screenshots of the two built apps, using the same workspace, the same real Claude CLI, and the same prompt. One configured MCP server command was missing, and that server failed to start on both. Full access stayed on, and no MCP approval dialog appeared.
93095c2f67447474c1894e477f6695cdf5e98c16a9f7ed99ff132c44ea369c9376b554d6289910a9On current main the chat shows the real SessionStart hook failure and a red
claude · message:system:initcard. On this branch the hook failure and the replypongremain, and themessage:system:initcard is absent. The auth_status image and the plugin-install note above are unchanged.Earlier M4Air Electron QA
Built and compared earlier PR head
7ce7f9c0689fae9250fb8890db44c4a039fcd733with current main79b916395150eeee2954f03c30cc1744d95f6bcfin separate fresh profiles, using the same fixture repository, setup script, default tabs, failed MCP server, deliberately failing SessionStart hook, and signed-in real Claude CLI. Hidden whole-window Playwright screenshots show that both builds run setup, open Notes → Idle → Setup → Claude Chat with Claude active, and returnpong; only main adds the false redclaude · message:system:initrow. The genuine hook error remains visible on both. Real agent configuration hashes were unchanged and the test processes were stopped.The two named real-Claude test files ran on that earlier head with the single no-hooks test excluded because the signed-in agent's real home was accessible. Ten tests passed; three assertions failed because they expect older hardcoded Claude model IDs (
claude-haiku-4-5-20251001andclaude-sonnet-5) while the real CLI returnedclaude-haiku-5-5andclaude-sonnet-5-5. Those test files are unchanged by this PR and identical on the main revision used for QA; the full suites are not green.Exact-head M4Air Electron QA after the main lint fix
Fresh builds compared PR head
14937ee770790baabb0191331f5795b93c47eac7with mainf8089c787b04b85b5d796e94c330858b5c2c0496using separate profiles, the same fixture repository, setup and default-tab commands, a failed MCP server, a failing SessionStart hook, and real signed-in Claude. Both opened Notes → Idle → Setup → Claude Chat with Claude active and repliedpong. Only main showed the false redclaude · message:system:initrow; both preserved the genuine hook error. Whole-window hidden-renderer screenshots were captured through Playwright CDP. All 50 recorded test processes exited, the QA ports are free, and real agent configuration hashes match their pretest values.