From 9e768fdde8bca79baaf09950beb30a4c1089f95e Mon Sep 17 00:00:00 2001 From: Glen Gordon <18452241+glenrgordon@users.noreply.github.com> Date: Thu, 5 Nov 2026 09:15:38 -0600 Subject: [PATCH] fix: answer the peer when a response payload cannot be encoded A response payload is attributed to a request id before it is encoded, and json.dumps runs inside transport.send. So any payload the serialiser refuses makes the TypeError escape the response task, no reply is ever written, and the peer waits on that id forever. Two routes reach it today with no hostile input at all: - a handler returning a raw value that is not JSON-native, e.g. {"x": {1, 2}}; a BaseModel result is safe because model_dump(mode="json") coerces, but a raw return value was assigned unchecked; - a handler raising RequestError(..., data=...), whose data is typed Any and passed straight through to the error object. Gate the finished payload once, at the end of _execute_request, with the same json.dumps the transports call. Pydantic's to_jsonable_python would not be a faithful gate here: it coerces a set or bytes rather than rejecting them, so it would pass payloads json.dumps then refuses. The check is deliberately not at the send. A transport failure means the peer is unreachable and propagating it unanswered is correct, which test_response_send_failure_is_not_mapped_to_a_handler_error asserts; an encoding failure means the peer is reachable and the payload is our fault. Catching around transport.send would conflate the two and would convert a transport implementation's own TypeError into a protocol error reported to the peer. That file is untouched here. Co-Authored-By: Claude Opus 5 --- src/acp/connection.py | 10 ++++++ tests/test_rpc.py | 71 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/src/acp/connection.py b/src/acp/connection.py index 84d009cf..013fd538 100644 --- a/src/acp/connection.py +++ b/src/acp/connection.py @@ -283,6 +283,16 @@ async def _execute_request(self, message: dict[str, Any]) -> dict[str, Any]: except Exception: data = {"details": str(exc)} payload["error"] = RequestError.internal_error(data).to_error_obj() + # One gate for every payload shape. The response is already attributed to a + # request id, and ``json.dumps`` is what each transport calls, so anything it + # refuses here would otherwise strand the peer inside ``transport.send``, where + # the failure escapes the response task with no handler left to answer. + try: + json.dumps(payload) + except Exception as exc: + logging.exception("Response for method=%s could not be encoded", method, exc_info=exc) + payload.pop("result", None) + payload["error"] = RequestError.internal_error({"details": str(exc)}).to_error_obj() return payload async def _run_notification(self, message: dict[str, Any]) -> None: diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 43c37fd1..0a8a843a 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -975,3 +975,74 @@ async def test_connection_init_under_eager_task_factory(server): await asyncio.sleep(0) assert conn._disconnected is False await conn.close() + + +@pytest.mark.asyncio +async def test_unserialisable_raw_result_answers_the_peer(server): + # Regression: a handler returning a raw (non-BaseModel) value that json.dumps cannot + # encode used to build a payload that died in the transport, so no response line was + # ever written and the peer waited on the request id forever. + async def handler(method: str, params: Any, is_notification: bool) -> Any: + return {"x": {1, 2}} + + conn = Connection(handler, server.client_writer, server.client_reader) + + req = {"jsonrpc": "2.0", "id": 1, "method": "custom/thing", "params": {}} + server.server_writer.write((json.dumps(req) + "\n").encode()) + await server.server_writer.drain() + + line = await asyncio.wait_for(server.server_reader.readline(), timeout=1) + resp = json.loads(line) + assert resp["id"] == 1 + assert resp["error"]["code"] == -32603 + details = resp["error"]["data"]["details"] + assert "set" in details and "JSON serializable" in details + + await conn.close() + + +@pytest.mark.asyncio +async def test_base_model_result_with_a_set_is_coerced_and_still_answers(server): + # Pins why the BaseModel branch above needs no representability check: + # model_dump(mode="json") already coerces an Any-typed set into a list. If that ever + # stops being true, this test says so instead of the check silently becoming too narrow. + async def handler(method: str, params: Any, is_notification: bool) -> Any: + return AuthenticateResponse(field_meta={"tags": {1, 2}}) + + conn = Connection(handler, server.client_writer, server.client_reader) + + req = {"jsonrpc": "2.0", "id": 2, "method": "authenticate", "params": {}} + server.server_writer.write((json.dumps(req) + "\n").encode()) + await server.server_writer.drain() + + line = await asyncio.wait_for(server.server_reader.readline(), timeout=1) + resp = json.loads(line) + assert resp["id"] == 2 + assert "error" not in resp + assert sorted(resp["result"]["_meta"]["tags"]) == [1, 2] + + await conn.close() + + +@pytest.mark.asyncio +async def test_unserialisable_request_error_data_answers_the_peer(server): + # Regression: ``RequestError.data`` is typed ``Any``, so a handler raising a structured + # protocol error whose data json.dumps cannot encode used to build a payload that died in + # the transport, leaving no response line and the peer waiting on the request id forever. + async def handler(method: str, params: Any, is_notification: bool) -> Any: + raise RequestError(-32000, "boom", data={"x": {1, 2}}) + + conn = Connection(handler, server.client_writer, server.client_reader) + + req = {"jsonrpc": "2.0", "id": 3, "method": "custom/thing", "params": {}} + server.server_writer.write((json.dumps(req) + "\n").encode()) + await server.server_writer.drain() + + line = await asyncio.wait_for(server.server_reader.readline(), timeout=1) + resp = json.loads(line) + assert resp["id"] == 3 + assert resp["error"]["code"] == -32603 + details = resp["error"]["data"]["details"] + assert "set" in details and "JSON serializable" in details + + await conn.close()