Repository navigation
fix: keep validation error details JSON-serialisable - #185
Closed
glenrgordon wants to merge 1 commit into
Closed
glenrgordon wants to merge 1 commit into
glenrgordon wants to merge 1 commit into
Conversation
`_execute_request` built an `invalid_params` error from `exc.errors()`, whose `ctx` and `input` fields need not be JSON-native. A handler-origin validation error -- one raised by a handler constructing a model from arbitrary Python values -- therefore produced a payload that `json.dumps` refuses inside the transport's `send`. The response had already been attributed to a request id, so the `TypeError` escaped through a task with no error handler, nothing was written, and the peer waited on that id forever. Serialise the error with pydantic instead: `exc.json(include_context=False)` drops the exception objects in `ctx` and lets pydantic render `input` -- and anything a future pydantic adds -- without the SDK choosing how to stringify someone else's object. Notably pydantic's serialiser never calls `repr`, so an input with a broken `__repr__` cannot take the expression down; a `fallback=str` coercion would, and it sits inside an `except` clause where no sibling handler could catch it. Two regression tests cover the handler-origin path through a real socket pair, where the transport actually serialises. Existing coverage was entirely wire-origin, where every value has already been through `json.loads`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A handler that raises a
ValidationErrorwhile building a model out of Python values canleave the peer waiting on its request id forever.
Connection._execute_requestbuilds the-32602payload fromexc.errors(), whose entriescarry
ctx(validator context, which for a validator that raised holds the exception object)and
input(the offending value, which may be any Python object). The payload is correct andunserialisable.
json.dumpsthen runs insidetransport.send, after the response has beenattributed to a request id, so the
TypeErrorpropagates out of the response task, no replyis ever written, and the caller waits on that id until it gives up.
This replaces
exc.errors()withjson.loads(exc.json(include_context=False)):include_context=Falsedrops the exception objects inctx, and letting pydantic serialiseits own error type handles
input— and anything a future pydantic adds — without the SDKdeciding how to stringify someone else's object.
jsonis already imported, so there is nonew dependency and nothing for
deptryto say.include_context=Falseon its own is not enough, which is worth stating because it is thefix a reader reaches for first: it removes
ctxand leavesinput, andinputis the fieldthat actually carried the offending value in the reproduction.
The existing coverage at
tests/test_rpc.py(test_invalid_params_results_in_error_response)validates
message["params"], which arrived throughjson.loadsand so is JSON-native byconstruction. That is why this has gone unnoticed: every value in a wire-origin validation
error is already safe. A handler-origin one is not.
Related issues
None filed yet. Opened as a draft so the report and the fix can be read together; happy to
file a tracking issue first if maintainers prefer that order.
Testing
Two tests added beside the existing wire-origin one in
tests/test_rpc.py:test_handler_origin_validation_error_reaches_the_peertest_handler_origin_validation_error_survives_an_input_whose_repr_raisesBoth drive a real
Connectionover a socket pair, sojson.dumpsactually runs where it runsin production (
src/acp/task/sender.py). Both fail on unmodifiedmainby timing out —TimeoutErroratserver_reader.readline(), never reaching an assertion — which is the point:the defect is that no line is written at all. Each logs the stranded path on the way down:
connection.py await self._transport.send(payload)→task/sender.py json.dumps→TypeError: Object of type _NotJsonNative is not JSON serializable.The second test exists to pin the choice of expression, not just the fix. The obvious
alternative,
to_jsonable_python(exc.errors(include_context=False), fallback=str), isidentical on ordinary inputs — but
fallback=strcallsstr(), which calls__repr__, so aninputwhose__repr__raises makes the expression raise. It sits inside theexcept ValidationErrorclause, where no siblingexceptcatches it, so it propagates out of_execute_requestand strands the peer again. Measured against this branch: with that formsubstituted in, test 1 passes and test 2 fails with
RuntimeError: repr explodedre-raised atpayload = handler.result()and nothing sent. Pydantic's own serialiser never calls repr; itsubstitutes a placeholder. Without that test, a later simplification back to
fallback=strreads as equivalent and silently restores the bug.
tests/test_request_error_logging.pyis untouched and passes, includingtest_response_send_failure_is_not_mapped_to_a_handler_error. Nothing here goes neartransport.send, so a transport failure still propagates unanswered, which is correct — thereis nobody left to answer a closed pipe.
Verified on the declared pydantic floor as well as the installed version.
pydantic-core 2.18.1has no wheel for Python 3.13 and does not build from source here, so the floor waschecked in a separate Python 3.11.16 environment:
pydantic 2.7.0 / pydantic-core 2.18.1→the new tests pass. CI's matrix is the authority on this.
Environment: Windows 11, CPython 3.13,
uv,mingw32-make.make check: 2tydiagnostics, both pre-existingunresolved-importforhypercorninexamples/http_server.py(undeclared inpyproject.tomlanduv.lock; the target's--exclude "examples/*.py"does not match on this platform). prek/ruff all pass. No newdiagnostics.
make test:2 failed, 490 passed, 2 skipped. Both failures are pre-existing and intests/real_user/test_stdio_limits.py— a Windows CRLF line-ending artifact, and aload-sensitive subprocess test. Measured on unmodified
mainin the same worktree acrossthree runs:
1 failed, 489 passedonce and2 failed, 488 passedtwice. 488 → 490 isexactly these two tests, with the same failure set. CI is the authority on the clean numbers.
Docs & screenshots
None needed. Nothing under
docs/describes the error payload or itsdatafield — greppedfor
invalid_params,-32602andRequestErrorwith no hits — so there is no page to update.No behaviour change for any caller whose validation errors were already serialisable: the
payload keeps its
errorsarray, its codes, and its pydantic docsurl.The
urlfield embeds pydantic's minor version (https://errors.pydantic.dev/2.14/v/...), sothe payload shifts whenever pydantic bumps.
include_url=Falsewould stop that, but it is asecond user-visible change and the URL is genuinely useful in a log, so it is deliberately not
touched here.
Checklist
make gen-allnot involved.