Repository navigation
fix(rpc): keep the error sentence when a refusal also carries data - #560
Conversation
A handler that raised an RpcError with both a detail and a data dict lost
the detail on the wire: the dispatcher wrote data in place of
{"detail": ...}. The web page reads error.data.detail, so every refusal
from the plugins page (plug.configure on a plugin that takes no
credential, plug.auth on an unknown server) toasted
"config_validation_error" and nothing else. The frame now carries the
sentence beside the fields, unless the handler already put one there.
The hermes-only stubs stop passing their message as the detail: it is
already the data's "error" field, and StubResult forbids extra keys.
Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the actual PR delta from base b63f35087b1d through this head, plus the dispatcher/error contracts, PlugHub and model callers, web error consumers, stub schema validation, and the relevant history. The merge preserves every structured field, adds the human sentence only when data.detail is absent, does not mutate the exception payload, and leaves the strict hermes stub frames in their declared {error, hint?} shape. The additive error-data key is backward compatible with opaque clients and matches the web client's documented data.detail convention.
Coverage included AGENTS.md/CLAUDE.md and the runtime/web CONTEXT architecture rules, the complete three-file PR diff, callers and history, backward compatibility, and test-integrity review. No tests were weakened; the change adds a reproducing end-to-end dispatcher test while the existing strict stub-shape checks remain intact.
Verification:
uv run pytest tests/test_rpc_plughub.py tests/test_rpc_stubs.py tests/test_rpc_system.py -x: 117 passed.uv run pytest 'tests/test_rpc_contract_shapes.py::test_a_stub_refuses_in_the_declared_shape' -x: 11 passed.uv run pytest tests/test_rpc_*.py: 2,200 passed, 12 failed, 94 errors. All failures/errors are from the missing optionalraven_everosdistribution in unchanged memory/settings tests; the files and dependency metadata involved are byte-identical to the PR base. GitHub's four unit shards andui rpc contractcheck are green.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision adds only A56 and its T9.8 manual acceptance case. The case matches the reachable ledgerless, catalog-shaped GitHub stanza path: the settings page exposes the API-key panel from auth: apikey, plug.configure refuses because the install ledger is absent, the backend sentence matches the documented text, and the reviewed dispatcher change carries that sentence in data.detail for the toast. The previously reviewed implementation and automated regression test are unchanged.
For this revision I covered the full target diff and revision delta, AGENTS.md/CLAUDE.md and the relevant runtime/web CONTEXT rules, the acceptance/spec conventions, backend and UI callers, history, backward compatibility, architecture boundaries, and whether tests were weakened. No tests were weakened and no compatibility or layering change was introduced by the documentation commit.
Verification:
uv run pytest tests/test_rpc_plughub.py -x: 35 passed.- The repository large-file check passed for
b63f35087b1d..HEADby invoking its exact script directly (makeis unavailable in this environment). git diff --check github/refactor/ui_web_architecture...HEAD: passed.
The settings page's acceptance list had no requirement saying a refused write must tell the person why, so none of its cases exercised a refusal and the dispatcher dropping the sentence went unnoticed through the whole pass. A56 states it and T9.8 is the case, run on a real host as part of this change. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
702ad75 to
bf4d9b8
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The revision rewrite changed only the second commit's message: 702ad75e186a and this head have the same parent and identical tree hash. The new docs: header conforms to the repository's documented scope rule; the full target diff, callers, history, backward compatibility, architecture boundaries, and test coverage remain unchanged. AGENTS.md/CLAUDE.md and the relevant CONTEXT rules were rechecked, and no test was weakened.
Verification for this head: uv run pytest tests/test_rpc_plughub.py -x passed all 35 tests, and git diff --check github/refactor/ui_web_architecture...HEAD passed.
) ## Summary A handler that raised an `RpcError` with both a sentence (`detail`) and structured `data` lost the sentence on the wire: `Dispatcher` wrote `data` in place of `{"detail": ...}`. The web page reads `error.data.detail` for its toasts, so every refusal from the plugins page arrived as `config_validation_error` and nothing else -- `plug.configure` on a plugin that takes no credential, `plug.configure` on a server with no ledger, `plug.auth` on an unknown server, and likewise every `model.*` refusal that names its provider slug. The frame now carries the sentence beside the fields, unless the handler already put one there (the auth rollback in `plug.install` does). The hermes-only stubs stop passing their message as the detail: it is already the data's `error` field, and `StubResult` forbids extra keys, so their frames are unchanged. Before, on a `raven serve` host built from the base: ``` {"code": -32011, "message": "config_validation_error", "data": {"field": "name", "name": "context7"}} ``` After: ``` {"code": -32011, "message": "config_validation_error", "data": {"field": "name", "name": "context7", "detail": "this plugin takes no credential"}} ``` The second commit records what was missing from the settings page's acceptance list: no requirement said a refused write must tell the person why, so none of its cases exercised a refusal and a whole pass went green over this. A56 states the requirement and T9.8 is the case. Found while tracing a tester's report that the plugins page fails with a bare `config_validation_error` toast. One thing the toast was hiding is left as is: a server written into `tools.mcpServers` by hand under a catalog name (for example `github`) gets the credential panel from the catalog, but `plug.configure` refuses it ("not installed from the catalog; edit the config file instead"). With this fix that sentence reaches the toast. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification ``` uv run --frozen pytest tests/test_rpc_plughub.py tests/test_rpc_stubs.py tests/test_rpc_contract_shapes.py tests/test_acp_protocol.py tests/test_acp_methods.py -q 320 passed uv run --frozen --extra dev ruff check raven/rpc/dispatcher.py raven/rpc/methods/_stubs.py tests/test_rpc_plughub.py All checks passed! uv run --frozen --extra dev ruff format --check raven/rpc/dispatcher.py raven/rpc/methods/_stubs.py tests/test_rpc_plughub.py 3 files already formatted ``` The new test `test_a_refusal_with_structured_data_still_carries_its_sentence` fails on the base (the frame's `data` lacks `detail`) and passes here. T9.8 was run on a real host, in the browser, before and after. A temporary `RAVEN_HOME` with a hand-written `github` stanza (a catalog name with no ledger entry), the page built from this branch, then Settings > Plugins > `github` > type a token > update, and the same for clear: base code (dispatcher and stubs checked out at b63f350): toast: "operation failed: config_validation_error" this branch: toast: "operation failed: not installed from the catalog; edit the config file instead" Both toasts were read from the page's accessibility tree and captured as screenshots. The same two calls over the socket return the frames shown above. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk - [x] Security impact considered: `detail` was already sent whenever `data` was absent; no new information leaves the server. - [x] Backward compatibility considered: `data` gains one key and every existing key is unchanged. The web client already prefers `data.detail`; the TUI's typed `RpcError` keeps `data` opaque; the stub frames are byte-identical. - [x] Rollback path is clear for risky changes: revert the commit. ## Related Issues N/A --------- Co-authored-by: gloryfromca <23442919+gloryfromca@users.noreply.github.com> Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
Summary
A handler that raised an
RpcErrorwith both a sentence (detail) and structureddatalost the sentence on the wire:Dispatcherwrotedatain place of{"detail": ...}. The web page readserror.data.detailfor its toasts, so every refusal from the plugins page arrived asconfig_validation_errorand nothing else --plug.configureon a plugin that takes no credential,plug.configureon a server with no ledger,plug.authon an unknown server, and likewise everymodel.*refusal that names its provider slug.The frame now carries the sentence beside the fields, unless the handler already put one there (the auth rollback in
plug.installdoes). The hermes-only stubs stop passing their message as the detail: it is already the data'serrorfield, andStubResultforbids extra keys, so their frames are unchanged.Before, on a
raven servehost built from the base:After:
The second commit records what was missing from the settings page's acceptance list: no requirement said a refused write must tell the person why, so none of its cases exercised a refusal and a whole pass went green over this. A56 states the requirement and T9.8 is the case.
Found while tracing a tester's report that the plugins page fails with a bare
config_validation_errortoast. One thing the toast was hiding is left as is: a server written intotools.mcpServersby hand under a catalog name (for examplegithub) gets the credential panel from the catalog, butplug.configurerefuses it ("not installed from the catalog; edit the config file instead"). With this fix that sentence reaches the toast.Type
Verification
The new test
test_a_refusal_with_structured_data_still_carries_its_sentencefails on the base (the frame'sdatalacksdetail) and passes here.T9.8 was run on a real host, in the browser, before and after. A temporary
RAVEN_HOMEwith a hand-writtengithubstanza (a catalog name with no ledger entry), the page built from this branch, then Settings > Plugins >github> type a token > update, and the same for clear:Both toasts were read from the page's accessibility tree and captured as screenshots. The same two calls over the socket return the frames shown above.
Risk
detailwas already sent wheneverdatawas absent; no new information leaves the server.datagains one key and every existing key is unchanged. The web client already prefersdata.detail; the TUI's typedRpcErrorkeepsdataopaque; the stub frames are byte-identical.Related Issues
N/A