Skip to content

fix(tui_rpc): surface the cause of an internal_error, not just the code - #296

Open
arelchan wants to merge 1 commit into
mainfrom
fix/tui_surface_internal_error_detail
Open

fix(tui_rpc): surface the cause of an internal_error, not just the code#296
arelchan wants to merge 1 commit into
mainfrom
fix/tui_surface_internal_error_detail

Conversation

@arelchan

Copy link
Copy Markdown
Contributor

Summary

A -32603 reached the user as exactly this, and nothing else:

error: [rpc -32603] internal_error

No cause, no file, no log path. The information existed the whole time and was discarded
three separate times:

  1. The dispatcher dropped detail. raven/tui_rpc/dispatcher.py used
    if exc.data is not None: ... elif exc.detail: ..., so a raiser that set both lost the
    detail. _build_tui_agent_loop always sets both.
  2. turn.send emitted only {code, message} for a latched init crash, although the
    ErrorEvent schema already has a detail field and chatStream.onError already renders
    it.
  3. The client formatted [rpc <code>] <message>, where message is a fixed code name
    (internal_error), and every call site prints err.message.

So an AgentLoop that cannot start produced a dead end. A real instance: a config file
containing a key the running branch does not know about fails validation, _build_tui_agent_loop
raises InternalError with the full pydantic message and log_path, and the TUI showed the
line above.

The fix keeps message protocol-faithful and puts the cause where callers already look:

  • error_data(exc) in raven/tui_rpc/errors.py folds detail into data, and both the
    dispatcher and the turn error events use it, so the two paths carry the same context.
  • turn.send passes the init-crash detail plus its log_path into the emitted event.
  • formatRpcError in ui-tui/src/rpc/errors.ts builds the message from detail,
    exception_message or reason, and appends the log path. A multi-line cause (a config
    error listing each offending field) keeps its line breaks below the summary line.

Same failure after the change, rendered by feeding a real dispatcher frame through the client
path:

error: [rpc -32603] internal_error:
Config at /Users/admin/.raven/config.json fails schema validation:
1 validation error for Config
subagents
  Extra inputs are not permitted
(details in ~/.raven/logs/tui.log)

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

uv run pytest tests/ -k "tui_rpc or dispatcher or tui_bootstrap" -> 523 passed. New cases:
the dispatcher keeps detail next to reason and log_path in one frame; turn.send emits
the build-error cause with its log path; and it omits detail entirely when the error has no
cause to report.

npx vitest run in ui-tui/ -> 86 files, 988 tests passed, including six new
rpcErrorFromFrame cases: a bare frame keeps the old one-line form; data context is
surfaced with the log path; exception_message and reason are read when detail is absent;
a multi-line cause keeps its shape; non-object or blank data cannot corrupt the message; and
typed subclass selection plus raw data access are unchanged.

npm run type-check, npx eslint, npx prettier --check, ruff check, ruff format --check
all clean.

End to end, the exact production shape: an InternalError raised the way
_build_tui_agent_loop raises it was dispatched to a JSON-RPC frame, and that frame was fed
through rpcErrorFromFrame with tsx, producing the output shown above.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Notes: error.code and error.message are unchanged on the wire, so nothing that branches on
them is affected; the added data.detail and payload.detail are both already in the schema.
err.message gains a suffix, which the one message-matching call site
(SESSION_BUSY_RE in useSubmission.ts) is unaffected by, since it matches text that still
appears. Detail text originates from server-side exceptions and is already written to
~/.raven/logs/tui.log, so this exposes nothing new to the client; the client accepts it only
when it is a string and ignores any other shape. Rollback is a revert of this commit.

Related Issues

N/A

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 <code>] <message>`, 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) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned. One non-blocking note inline about the log_path suffix not reaching the user in the multi-line case.

What I checked

  • The diff itself, plus the callers on both sides: _build_tui_agent_loop (raven/cli/tui_commands.py:483-525) is the only producer of these InternalErrors, and it always passes data, which is exactly the case the old dispatcher dropped detail for. The consumer of the new event field is onError in ui-tui/src/app/chatStream.ts:210-224, which is live (wired through createChatStream in useMainApp.ts), so the server half is not dead code.
  • Schema: ErrorEventPayload.detail already exists in raven/tui_rpc/models.py:212-216 and in the checked-in ui-tui/src/rpc/generated.ts, so nothing needed regenerating and the strict payload model still accepts the emitted dict.
  • Backward compatibility: both new fields are additive. An older client ignores the extra detail key; a newer client against an older server just formats the bare code name, which is what the first test in rpc.test.ts pins.
  • error_data semantics: setdefault means a raiser that sets both data["detail"] and exc.detail keeps the data one, and data or None now omits error.data when a raiser passes an empty dict. Neither has a caller in the tree, and no consumer requires data to be present, so I am not raising them.
  • Project rules: commit header follows Conventional Commits with a real scope, the whole message is ASCII (git log -1 --format=%B | grep -nP "[^\x00-\x7F]" prints nothing), no new test files were created where an existing one exists (AGENTS.md 5.4), no assets.
  • Tests were not weakened. Both new Python tests assert real behaviour, and the negative one (..._omits_detail_when_the_build_error_has_no_cause) is a genuine assertion, not a skip. The TS ignores non-object and blank data case loops over six hostile shapes rather than asserting one.

What I ran

uv run pytest tests -k tui -q
611 passed, 7 skipped, 5335 deselected in 40.42s

The 7 skips are the pre-existing optional-channel import guards (dingtalk_stream, lark_oapi, nio, botpy, slack_sdk, telegram, wecom_aibot_sdk) and are unrelated to this change.

cd ui-tui && npx vitest run
Test Files  86 passed (86)
     Tests  988 passed (988)

npx tsc --noEmit      # clean
uv run ruff check raven/tui_rpc tests/test_tui_rpc_system.py tests/test_tui_rpc_turn_send.py
All checks passed!

(The suite needs packages/hermes-ink built first -- npx esbuild src/entry-exports.ts --bundle --platform=node --format=esm --packages=external --outdir=dist -- otherwise 41 files fail on a missing ./dist/entry-exports.js. That is an environment prerequisite, not this branch.)

Nit, take it or leave it

The doc comment at ui-tui/src/rpc/errors.ts:21 writes the example as (see ~/.raven/logs/tui.log) but the code emits (details in ...).

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()})"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: the log_path suffix does not survive to the user in the case that motivated it.

The renderer on the other end keeps only the first line of detail, capped at 200 chars (ui-tui/src/app/chatStream.ts:221, pre-existing):

const extra = detail ? `: ${detail.split('\n')[0].slice(0, 200)}` : ''

ValidationError is one of the caught init-crash types and its str() is always multi-line -- I checked rather than assumed:

$ uv run python -c "...M(a='x', b='y')..."
"2 validation errors for M\na\n  Input should be a valid integer ..."
LINES 7

So for the exact scenario the commit message cites -- a config the running branch cannot parse -- the appended (details in ~/.raven/logs/tui.log) lands on line 7 and the user sees only error: internal_error (code=-32603): 2 validation errors for RavenConfig. No log path, which is the one thing this docstring says the path is riding along to provide.

Single-line causes (FileNotFoundError, OSError, MissingCredentialsError) are fine, so this is a partial gap rather than a regression -- main showed nothing at all. Cheapest fix is to put the pointer where the truncation cannot reach it, e.g. f"{log_path} <- {first_line_of_detail}", or prepend it:

return f"(details in {log_path.strip()}) {detail.strip()}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants