From b6b8ab4851b77f08a8cdde669730172e4e077b20 Mon Sep 17 00:00:00 2001 From: arelchan <204152633+arelchan@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:05:05 +0800 Subject: [PATCH] fix(tui_rpc): surface the cause of an internal_error, not just the code A -32603 reached the user as the bare line `error: [rpc -32603] internal_error`, with nothing about what failed or where to look. The cause was collected and then thrown away three times over: - the dispatcher dropped `RpcError.detail` whenever the raiser also passed `data`, which `_build_tui_agent_loop` always does; - `turn.send` emitted a latched init crash as `{code, message}` only, even though the ErrorEvent schema already has a `detail` field the front end renders; - the client formatted `[rpc ] `, where `message` is a fixed code name, and every call site prints `err.message`. So an AgentLoop that cannot start (a config the running branch cannot parse, for instance) produced an error that named neither the file nor the log. Fold `detail` into `error.data` in a shared `error_data` helper used by both the dispatcher and the turn events, pass the init-crash detail plus its `log_path` into the emitted event, and build the client-side message from `detail` / `exception_message` / `reason` with the log path appended. A multi-line cause (a config error listing each offending field) keeps its line breaks below the summary line. Co-authored-by: Claude (claude-opus-5) --- raven/tui_rpc/dispatcher.py | 8 ++--- raven/tui_rpc/errors.py | 14 ++++++++ raven/tui_rpc/methods/turn.py | 41 ++++++++++++++++++---- tests/test_tui_rpc_system.py | 22 +++++++++++- tests/test_tui_rpc_turn_send.py | 41 ++++++++++++++++++++++ ui-tui/src/__tests__/rpc.test.ts | 60 ++++++++++++++++++++++++++++++++ ui-tui/src/rpc/errors.ts | 34 +++++++++++++++++- 7 files changed, 207 insertions(+), 13 deletions(-) diff --git a/raven/tui_rpc/dispatcher.py b/raven/tui_rpc/dispatcher.py index 4b64e5bd..d09eddaf 100644 --- a/raven/tui_rpc/dispatcher.py +++ b/raven/tui_rpc/dispatcher.py @@ -26,6 +26,7 @@ METHOD_NOT_FOUND, PARSE_ERROR, RpcError, + error_data, ) Handler = Callable[[dict[str, Any]], Awaitable[dict[str, Any]]] @@ -106,10 +107,9 @@ async def dispatch(self, frame: dict[str, Any]) -> dict[str, Any]: "code": exc.code, "message": exc.message, } - if exc.data is not None: - err_payload["data"] = exc.data - elif exc.detail: - err_payload["data"] = {"detail": exc.detail} + payload_data = error_data(exc) + if payload_data is not None: + err_payload["data"] = payload_data return {"jsonrpc": "2.0", "id": frame_id, "error": err_payload} except SystemExit as exc: # Click/Typer can leak SystemExit even with standalone_mode=False; diff --git a/raven/tui_rpc/errors.py b/raven/tui_rpc/errors.py index 85be4b8d..5b9c6008 100644 --- a/raven/tui_rpc/errors.py +++ b/raven/tui_rpc/errors.py @@ -61,6 +61,20 @@ def message(self) -> str: return self.MESSAGE +def error_data(exc: RpcError) -> dict[str, Any] | None: + """Wire ``error.data`` for an exception: ``data`` with ``detail`` folded in. + + ``message`` is a fixed code name, so ``detail`` is the only place the cause + is spelled out; callers that set both must not lose it. Shared by the + dispatcher's error frames and the per-turn error events so both carry the + same context. + """ + data = dict(exc.data) if exc.data is not None else {} + if exc.detail: + data.setdefault("detail", exc.detail) + return data or None + + class SessionNotFoundError(RpcError): CODE = -32001 MESSAGE = "session_not_found" diff --git a/raven/tui_rpc/methods/turn.py b/raven/tui_rpc/methods/turn.py index e75fa42f..de9eeb3d 100644 --- a/raven/tui_rpc/methods/turn.py +++ b/raven/tui_rpc/methods/turn.py @@ -26,7 +26,7 @@ from raven.spine import ChatType, Media, Origin, Source, TurnHandle, TurnRequest from raven.spine.scheduler import Scheduler, SchedulerDrainingError -from raven.tui_rpc.errors import RpcError, TurnInProgressError +from raven.tui_rpc.errors import RpcError, TurnInProgressError, error_data from raven.tui_rpc.models import ( TurnCancelParams, TurnSendParams, @@ -134,16 +134,38 @@ def _resolve_model(parsed: TurnSendParams) -> str: # --------------------------------------------------------------------------- +def _build_error_detail(exc: RpcError) -> str | None: + """Human-readable cause of a latched init crash. + + ``message`` is only the code name (``internal_error``), so without this the + TUI shows a turn failing for no stated reason. ``log_path`` rides along + because an init crash is usually only fully diagnosable from the log. + """ + data = error_data(exc) or {} + detail = data.get("detail") or data.get("exception_message") + if not isinstance(detail, str) or not detail.strip(): + return None + log_path = data.get("log_path") + if isinstance(log_path, str) and log_path.strip(): + return f"{detail.strip()} (details in {log_path.strip()})" + return detail.strip() + + async def _emit_start_then_error( - emitter: SubscriptionEmitter, session_key: str, turn_id: str, code: int, message: str + emitter: SubscriptionEmitter, + session_key: str, + turn_id: str, + code: int, + message: str, + detail: str | None = None, ) -> None: # message.start first so the front-end has a turn to clear, then the error # clears it (its onError resets turnId) — same shape the old per-turn task used. await emitter.emit(session_key, {"type": "message.start", "payload": {"turn_id": turn_id}}) - await emitter.emit( - session_key, - {"type": "error", "payload": {"code": code, "message": message, "reason": "internal"}}, - ) + payload: dict[str, Any] = {"code": code, "message": message, "reason": "internal"} + if detail: + payload["detail"] = detail + await emitter.emit(session_key, {"type": "error", "payload": payload}) async def turn_send( @@ -182,7 +204,12 @@ async def turn_send( if emitter is not None: if build_error is not None: await _emit_start_then_error( - emitter, parsed.session_key, turn_id, build_error.code, build_error.message + emitter, + parsed.session_key, + turn_id, + build_error.code, + build_error.message, + _build_error_detail(build_error), ) else: await _emit_start_then_error(emitter, parsed.session_key, turn_id, -32008, "model_not_available") diff --git a/tests/test_tui_rpc_system.py b/tests/test_tui_rpc_system.py index a8660bb8..3f58026e 100644 --- a/tests/test_tui_rpc_system.py +++ b/tests/test_tui_rpc_system.py @@ -17,7 +17,7 @@ import pytest from raven.tui_rpc.dispatcher import Dispatcher -from raven.tui_rpc.errors import ConfigValidationError +from raven.tui_rpc.errors import ConfigValidationError, InternalError from raven.tui_rpc.methods.system import ( register_system_methods, system_hello, @@ -162,6 +162,26 @@ async def boom(params: dict) -> dict: assert "traceback_tail" in resp["error"]["data"] +async def test_dispatcher_keeps_detail_alongside_structured_data(): + # `message` is only a code name, so dropping `detail` when a raiser also set + # `data` leaves the client with nothing to show. Both must reach the wire. + d = Dispatcher() + + async def boom(params: dict) -> dict: + raise InternalError( + detail="Config at ~/.raven/config.json fails schema validation", + data={"reason": "tui_init_crash", "log_path": "~/.raven/logs/tui.log"}, + ) + + d.register("test.boom", boom) + resp = await d.dispatch({"jsonrpc": "2.0", "id": 7, "method": "test.boom", "params": {}}) + + assert resp["error"]["code"] == -32603 + assert resp["error"]["data"]["detail"] == "Config at ~/.raven/config.json fails schema validation" + assert resp["error"]["data"]["reason"] == "tui_init_crash" + assert resp["error"]["data"]["log_path"] == "~/.raven/logs/tui.log" + + async def test_dispatcher_parse_response_id_echoed(): d = _build_dispatcher() frame = { diff --git a/tests/test_tui_rpc_turn_send.py b/tests/test_tui_rpc_turn_send.py index 181153d3..b545a540 100644 --- a/tests/test_tui_rpc_turn_send.py +++ b/tests/test_tui_rpc_turn_send.py @@ -188,6 +188,47 @@ class _BuildErr(RpcError): assert emitter.emitted[-1][1]["payload"]["code"] == -32603 +async def test_turn_send_emits_the_build_error_cause_not_just_its_code() -> None: + # -32603 internal_error names no cause on its own; the init crash detail and + # the log path are what make the failure diagnosable in the transcript. + class _BuildErr(RpcError): + CODE = -32603 + MESSAGE = "internal_error" + + emitter = FakeEmitter() + build_error = _BuildErr( + "Config at ~/.raven/config.json fails schema validation", + {"reason": "tui_init_crash", "log_path": "~/.raven/logs/tui.log"}, + ) + await turn_send( + {"session_key": "tui:default", "content": "x"}, + emitter=emitter, + scheduler=None, + build_error=build_error, + ) + + payload = emitter.emitted[-1][1]["payload"] + assert payload["detail"] == ( + "Config at ~/.raven/config.json fails schema validation (details in ~/.raven/logs/tui.log)" + ) + + +async def test_turn_send_omits_detail_when_the_build_error_has_no_cause() -> None: + class _BuildErr(RpcError): + CODE = -32603 + MESSAGE = "internal_error" + + emitter = FakeEmitter() + await turn_send( + {"session_key": "tui:default", "content": "x"}, + emitter=emitter, + scheduler=None, + build_error=_BuildErr(), + ) + + assert "detail" not in emitter.emitted[-1][1]["payload"] + + # --- Params validation --- diff --git a/ui-tui/src/__tests__/rpc.test.ts b/ui-tui/src/__tests__/rpc.test.ts index 7980093a..ae8cd7b6 100644 --- a/ui-tui/src/__tests__/rpc.test.ts +++ b/ui-tui/src/__tests__/rpc.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import { asRpcResult, rpcErrorMessage } from '../lib/rpc.js' +import { RpcError, rpcErrorFromFrame, SessionNotFoundError } from '../rpc/errors.js' describe('asRpcResult', () => { it('keeps plain object payloads', () => { @@ -25,3 +26,62 @@ describe('rpcErrorMessage', () => { expect(rpcErrorMessage({ code: 500 })).toBe('request failed') }) }) + +describe('rpcErrorFromFrame', () => { + it('keeps the bare code name when the frame carries no context', () => { + const err = rpcErrorFromFrame({ code: -32603, message: 'internal_error' }) + expect(err.message).toBe('[rpc -32603] internal_error') + }) + + it('surfaces the cause the server put in data, plus where to read more', () => { + const err = rpcErrorFromFrame({ + code: -32603, + message: 'internal_error', + data: { + reason: 'tui_init_crash', + detail: 'Config at ~/.raven/config.json fails schema validation', + log_path: '~/.raven/logs/tui.log' + } + }) + expect(err.message).toBe( + '[rpc -32603] internal_error: Config at ~/.raven/config.json fails schema validation\n' + + '(details in ~/.raven/logs/tui.log)' + ) + }) + + it('reads exception_message and reason when detail is absent', () => { + expect( + rpcErrorFromFrame({ code: -32603, message: 'internal_error', data: { exception_message: 'boom' } }).message + ).toBe('[rpc -32603] internal_error: boom') + expect(rpcErrorFromFrame({ code: -32603, message: 'internal_error', data: { reason: 'uncaught' } }).message).toBe( + '[rpc -32603] internal_error: uncaught' + ) + }) + + it('keeps a multi-line cause readable below the summary', () => { + const err = rpcErrorFromFrame({ + code: -32011, + message: 'config_validation_error', + data: { detail: '2 validation errors\nsubagents: extra inputs are not permitted' } + }) + expect(err.message).toBe( + '[rpc -32011] config_validation_error:\n2 validation errors\nsubagents: extra inputs are not permitted' + ) + }) + + it('ignores non-object and blank data without losing the code name', () => { + for (const data of [undefined, null, ['detail'], 'detail', { detail: ' ' }, { detail: 7 }]) { + expect(rpcErrorFromFrame({ code: -32603, message: 'internal_error', data }).message).toBe( + '[rpc -32603] internal_error' + ) + } + }) + + it('still selects the typed subclass and exposes raw data', () => { + const err = rpcErrorFromFrame({ code: -32001, message: 'session_not_found', data: { detail: 'no such key' } }) + expect(err).toBeInstanceOf(SessionNotFoundError) + expect(err).toBeInstanceOf(RpcError) + expect(err.code).toBe(-32001) + expect(err.data).toEqual({ detail: 'no such key' }) + }) +}) diff --git a/ui-tui/src/rpc/errors.ts b/ui-tui/src/rpc/errors.ts index a2d93b2d..8cfe9348 100644 --- a/ui-tui/src/rpc/errors.ts +++ b/ui-tui/src/rpc/errors.ts @@ -7,13 +7,45 @@ import type { JsonRpcErrorObject } from './generated.js' +/** Fields the server puts in `error.data` to explain a failure (see + * `raven/tui_rpc/errors.py` and `_build_tui_agent_loop`). `frame.message` is a + * fixed code name like `internal_error`, so without these the user is told + * nothing actionable. */ +const DETAIL_KEYS = ['detail', 'exception_message', 'reason'] as const + +const readString = (data: Record, key: string): string | undefined => { + const value = data[key] + return typeof value === 'string' && value.trim() ? value.trim() : undefined +} + +/** `[rpc -32603] internal_error: (see ~/.raven/logs/tui.log)`. + * Callers render `err.message` directly, so the cause has to live there. */ +export function formatRpcError(frame: JsonRpcErrorObject): string { + let text = `[rpc ${frame.code}] ${frame.message}` + if (typeof frame.data !== 'object' || frame.data === null || Array.isArray(frame.data)) { + return text + } + const data = frame.data as Record + const detail = DETAIL_KEYS.map(key => readString(data, key)).find(Boolean) + // A one-line detail reads inline; a multi-line one (a config error listing + // every offending field, say) keeps its shape below the summary. + if (detail) { + text += detail.includes('\n') ? `:\n${detail}` : `: ${detail}` + } + const logPath = readString(data, 'log_path') + if (logPath) { + text += `\n(details in ${logPath})` + } + return text +} + /** Base class for all JSON-RPC error responses surfaced to callers. */ export class RpcError extends Error { readonly code: number readonly data: unknown constructor(frame: JsonRpcErrorObject) { - super(`[rpc ${frame.code}] ${frame.message}`) + super(formatRpcError(frame)) this.name = 'RpcError' this.code = frame.code this.data = frame.data