Repository navigation
Conversation
hasProviderError walks a frame's whole payload, so an MCP server this session could not use promoted the entire system/init handshake to error-surface and the roster reached users as `claude · message:system:init`. Own the kind the way the background-task frames are owned: the handshake writes no row, and an unavailable MCP server writes one warning row in the CLI's own words instead (stablyai#25477).
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Init frames get a typed translator —
src/main/claude/claude-init-row.tsreturns atone: 'warning'status row naming each unavailable MCP server (its ownerrorsentence when present, else"MCP server <name> failed to start"), ornullfor a clean handshake. - The fallback owns the kind before the generic path —
src/main/claude/claude-structured-provider-fallback.ts:170short-circuitsmessage:system:init, sohasProviderErrorcan no longer promote the roster to anerror-surfacecard titled after the opcode. - Tests pin the regression —
src/main/claude/claude-init-row.test.tscovers the clean roster, the failed server with and without its own sentence, and the pure translator.
ℹ️ Failed-server mapping matches the pinned SDK contract
The PR body flags the one thing the author could not verify locally — that a failed mcp_servers entry is shaped as { name, status, error } with status: 'failed'. The pinned @anthropic-ai/claude-agent-sdk@0.3.284 type confirms it, so the warning row will render for the real failure and no field mapping is outstanding.
Technical details
# SDK init-frame schema (0.3.284)
## Evidence
- `SDKSystemMessage.mcp_servers`: `{ name: string; status: string; source?: string }[]`
- `McpServerStatus.status`: `'connected' | 'failed' | 'needs-auth' | 'pending' | 'disabled'`
- `McpServerStatus.error`: `string?` — documented "available when status is 'failed'"
## Why `needs-auth` / `disabled` are correctly excluded
Neither value starts with `error`/`fail` and `error` is only set for `'failed'`, so neither
promoted the frame to an error card before this change. Not handling them introduces no regression.
## Note (not a request)
Init frames are emitted at the start of each turn/cycle, so a persistently failed server now yields a
warning row per turn — the same cadence as the error card it replaces.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Claude init-frame translator that creates a warning journal row for unavailable MCP servers. The provider fallback handles init frames with this translator instead of the generic unhandled-frame path. Tests cover empty, connected, failed, and multiple-server cases. Priority: ➖ Normal Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Healthy or empty server rosters produce no warning row, while unavailable servers are surfaced as bounded warnings. No material merge-blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change preserves existing access and publication controls while replacing an unhelpful error card with server-specific warnings. No security regression was demonstrated, but handling of failures outside the server roster remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c2bea301-bc1c-4296-ad0a-6b415955d36a
📒 Files selected for processing (3)
src/main/claude/claude-init-row.test.tssrc/main/claude/claude-init-row.tssrc/main/claude/claude-structured-provider-fallback.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
- Report an MCP server the CLI could not authenticate: it is a dependency the session cannot use, and unlike a crash the missing action is the user's. - Close the test journal before afterEach removes its directory. The SQLite handle is still open there, and Windows refuses to remove an open one. - Pin the captured shape (name/status/source, no sentence), which is the arm that actually fires.
|
Both CodeRabbit findings are addressed in 1. 2. Test journal (stability) — Third, from the reporter — they posted the
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
needs-authservers now get a warning row —claude-init-row.tsmapsstatus: 'needs-auth'toMCP server <name> needs authentication, so a server the session cannot use without a login is named instead of silently dropped.- The test harness closes its journal database before cleanup —
claude-init-row.test.tsnow usescreateTrackedJournalOpenerand awaitscloseAll()beforerm, so the open SQLite handle no longer blocks removing the temp directory on Windows. - A captured-shape test pins the real frame — a new case reproduces the #25477 roster (
name/status/source, noerrorfield) and asserts both failed servers render asfailed to startlines, which fails if the status arm regresses.
Both CodeRabbit threads from the prior review are addressed here: the Windows cleanup change and the needs-auth handling. The new tests pass.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing to keep my queue honest: the repository merges within about a day and this has sat five with no review, so it is not being picked up. #25477 stays open for anyone who wants to take it, and the branch stays in my fork. |
Author: @paolovalletta_
ELI5
Every Claude session starts with a "here is what I loaded" handshake from the CLI — its tools, model, and MCP servers. Orca was showing that handshake as a red error card titled
claude · message:system:init, because Orca's own error detector looks anywhere inside a frame for a word likefailed, and one MCP server that could not start is enough. The card said nothing a user could act on. Now the handshake stays out of the transcript, and a server the session could not use gets one ordinary warning row naming it — in the CLI's own words.What Changed
Before, in the chat of a freshly started Claude session with one MCP server that failed to start:
After:
…and a handshake whose servers all connected produces no row at all, which is what the catalogue always intended.
Mechanism.
unhandledProviderFrameJournalItemtitles its row from the wire opcode (unhandled-provider-frame.ts:148), andmessage:system:initis cataloguedstatus-chrome— no row — inprovider-frame-disposition.ts:98. ButclassifyProviderFramechecks the payload first:hasProviderErrorwalks the whole payload recursively: any nested value undertype | status | state | subtype | outcomestarting witherror/fail, or any non-emptyerror | failureReason | failure_reason, promotes the frame. The init frame carries its whole roster, so one failed MCP server outranked the catalogue and the handshake rendered as a failure row.This gives the kind a dedicated translator, which is the mechanism the repository already uses for exactly this failure mode —
message:system:task_*reached users asclaude · message:system:task_notificationfor the same reason, andprovider-frame-disposition.test.tsdocuments that coverage as the fix. The new arm runs before the generic fallback, so the opcode row cannot come back whatever the payload contains.src/main/claude/claude-init-row.tsis the translator (same shape as the existingclaude-informational-row.ts):claudeInitRowBodyreturns atone: 'warning'status row naming every MCP server the frame reports unavailable, andnullfor a clean handshake.Why
Why not just suppress the kind.
unhandled-provider-frame.ts:43records that a failed dependency reports itself there "and nowhere else". Muting the kind would delete the only report of a real failure — including the case this issue is about, if the trigger is a failed MCP server. Naming the server keeps the signal and removes the opcode.Why
tone: 'warning'and not a failure row. The failure vocabulary inagent-session-failure-words.tsis a typed, client-read contract (AgentSessionFailureFact); a per-server dependency fault is not a fact that vocabulary has, and a whole session's handshake is not what failed. A warning row is what the reporter asked for — "processed silently or shown as a normal informational entry, not as an error" — and it is the same shape the informational note already uses.Why not narrow
hasProviderErrorinstead. It outranks the catalogue on purpose, for frames that genuinely report a failure under an innocuous name, and the containment the repository uses for the false positives is translator coverage. Changing the predicate would have re-ranked every provider's frames to fix one kind.Linked Issue
Fixes #25477
Visual Proof
N/A— no new UI surface, so there is nothing to screenshot: the existing status-row component that already renderstone: 'warning'notes renders this one, and what changes is which rows exist. The before/after at the row level:{ kind: 'status', text: 'claude · message:system:init', failure: { … } }— error surface, payload behind the disclosure{ kind: 'status', tone: 'warning', text: 'MCP server github: auth expired' }Testing
claude-init-row.test.ts(new) — clean handshake, failed server with a sentence, failed server without one, status-as-failure, two servers at oncetext: "claude · message:system:init"claude-structured-provider-fallback.test.ts,claude-informational-row.test.tspnpm test src/main/claude/pnpm test src/main/native-chat/agent-session-wire/ORCA_REAL_CLAUDE_CLI_TEST=1 pnpm test src/main/claude/claude-structured-real-cli.test.ts src/main/claude/claude-structured-real-cli-fold.test.tspnpm tc(node + cli + web)oxlint+oxfmton the three files (pre-commit)Platforms tested: macOS. The change is provider-wire translation with no platform branch and no filesystem or process dependency; the frames arrive the same way from a local or a remote execution host. Windows and Linux were not run.
AI Disclosure
Written with an AI coding agent, session model
deepseek-v4.1-flash(provideropencode-go). All commands and results above were produced on this machine.Review
message:system:initkind the Claude schema declares. The Codex path and the sharedhasProviderErrorpredicate are untouched, so no other provider's rows change.boundInlineTextlimits.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
mcp_servers: [], so I have no capture of a failed server and the row readsname,statusanderroras the frame's contract describes them. I asked the reporter on the issue for the payload behind their card's Details disclosure. If a real entry names its fields differently, the card is still gone — only the row text would need one line adjusted.needs-authis deliberately not treated as a failure. The repository has no spelling of that state anywhere (needs-authappears nowhere insrc/), so treating it as one would be vocabulary I cannot support. Say the word and it is one line.failed-looking value. I did not generalise beyond the kind in the report: which other kinds legitimately carry nested per-item states is a decision I have no evidence for.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) —tc, both affected suites, the real-CLI suite,oxlintandoxfmtpass locally;pnpm lintandbuildare left to CI