Skip to content

fix: keep validation error details JSON-serialisable - #185

Closed
glenrgordon wants to merge 1 commit into
agentclientprotocol:mainfrom
glenrgordon:fix/validation-error-details-json-serialisable
Closed

glenrgordon wants to merge 1 commit into
agentclientprotocol:mainfrom
glenrgordon:fix/validation-error-details-json-serialisable

Conversation

@glenrgordon

Copy link
Copy Markdown

Summary

A handler that raises a ValidationError while building a model out of Python values can
leave the peer waiting on its request id forever.

Connection._execute_request builds the -32602 payload from exc.errors(), whose entries
carry 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 and
unserialisable. json.dumps then runs inside transport.send, after the response has been
attributed to a request id, so the TypeError propagates out of the response task, no reply
is ever written
, and the caller waits on that id until it gives up.

This replaces exc.errors() with json.loads(exc.json(include_context=False)):
include_context=False drops the exception objects in ctx, and letting pydantic serialise
its own error type handles input — and anything a future pydantic adds — without the SDK
deciding how to stringify someone else's object. json is already imported, so there is no
new dependency and nothing for deptry to say.

include_context=False on its own is not enough, which is worth stating because it is the
fix a reader reaches for first: it removes ctx and leaves input, and input is the field
that 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 through json.loads and so is JSON-native by
construction. 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_peer
  • test_handler_origin_validation_error_survives_an_input_whose_repr_raises

Both drive a real Connection over a socket pair, so json.dumps actually runs where it runs
in production (src/acp/task/sender.py). Both fail on unmodified main by timing out —
TimeoutError at server_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), is
identical on ordinary inputs — but fallback=str calls str(), which calls __repr__, so an
input whose __repr__ raises makes the expression raise. It sits inside the
except ValidationError clause, where no sibling except catches it, so it propagates out of
_execute_request and strands the peer again. Measured against this branch: with that form
substituted in, test 1 passes and test 2 fails with RuntimeError: repr exploded re-raised at
payload = handler.result() and nothing sent. Pydantic's own serialiser never calls repr; it
substitutes a placeholder. Without that test, a later simplification back to fallback=str
reads as equivalent and silently restores the bug.

tests/test_request_error_logging.py is untouched and passes, including
test_response_send_failure_is_not_mapped_to_a_handler_error. Nothing here goes near
transport.send, so a transport failure still propagates unanswered, which is correct — there
is nobody left to answer a closed pipe.

Verified on the declared pydantic floor as well as the installed version. pydantic-core 2.18.1 has no wheel for Python 3.13 and does not build from source here, so the floor was
checked 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: 2 ty diagnostics, both pre-existing unresolved-import for hypercorn in
    examples/http_server.py (undeclared in pyproject.toml and uv.lock; the target's
    --exclude "examples/*.py" does not match on this platform). prek/ruff all pass. No new
    diagnostics.
  • make test: 2 failed, 490 passed, 2 skipped. Both failures are pre-existing and in
    tests/real_user/test_stdio_limits.py — a Windows CRLF line-ending artifact, and a
    load-sensitive subprocess test. Measured on unmodified main in the same worktree across
    three runs: 1 failed, 489 passed once and 2 failed, 488 passed twice. 488 → 490 is
    exactly 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 its data field — grepped
for invalid_params, -32602 and RequestError with 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 errors array, its codes, and its pydantic docs url.

The url field embeds pydantic's minor version (https://errors.pydantic.dev/2.14/v/...), so
the payload shifts whenever pydantic bumps. include_url=False would stop that, but it is a
second user-visible change and the URL is genuinely useful in a log, so it is deliberately not
touched here.

Checklist

  • Conventional Commit title.
  • Tests cover the change.
  • Docs/examples updated when behaviour is user-facing — explained above.
  • No schema regeneration; make gen-all not involved.

`_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>
@glenrgordon glenrgordon closed this Nov 5, 2026
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.

1 participant