From 566b5b16a996cdefb443597ca581b495f3a192ee Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 8 Aug 2026 11:57:11 -0700 Subject: [PATCH] fix(agent,gateway): class-level lone-surrogate chokepoints (#80366 #55143 #55309 #50959 #19819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Own the surrogate-crash class at three chokepoints instead of leaf sites: - finalize_turn scrubs final_response once where model text leaves the conversation loop — covers oneshot stdout (#80366), NIM/any-provider responses (#19819), and every delivery consumer of the turn result. - _sanitize_gateway_final_response scrubs at the gateway chat-surface boundary — Telegram utf16_len (#55309) and Signal formatting (#55143) can no longer see a lone surrogate; raw-text surfaces keep passthrough. - run_conversation walks the fully-built api_kwargs with _sanitize_structure_surrogates so tool descriptions (session_search, #50959) and every other request-body leaf are JSON-encodable before any provider sees them. Regression tests pin all three chokepoints plus helper semantics. Cherry-picked alongside #79240 (TheophilusChinomona) and #80374 (rainbowgore) whose commits precede this one with authorship preserved. --- agent/conversation_loop.py | 10 + agent/turn_finalizer.py | 13 ++ contributors/emails/executus.ahli@gmail.com | 1 + gateway/run.py | 13 ++ tests/agent/test_surrogate_chokepoints.py | 194 ++++++++++++++++++++ 5 files changed, 231 insertions(+) create mode 100644 contributors/emails/executus.ahli@gmail.com create mode 100644 tests/agent/test_surrogate_chokepoints.py diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 05981bca86..0ba4a0684c 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -2404,6 +2404,16 @@ def run_conversation( api_messages, tools_for_api=tools_for_api, ) + # Outbound-request surrogate chokepoint (#50959): the messages + # were scrubbed above, but the rest of the request body — + # tool/function descriptions (session_search's ±-heavy text is + # the recorded repro), extra_body, system strings routed via + # kwargs — can still carry invalid code points that providers + # reject with a non-retryable HTTP 400 ("invalid unicode code + # point"). One in-place walk here guarantees the entire + # payload json.dumps()-safe regardless of which leaf produced + # the string. Fast no-op when the payload is clean. + _sanitize_structure_surrogates(api_kwargs) if agent._force_ascii_payload: _sanitize_structure_non_ascii(api_kwargs) if agent.api_mode == "codex_responses": diff --git a/agent/turn_finalizer.py b/agent/turn_finalizer.py index d51b8b22cf..de31bbabd4 100644 --- a/agent/turn_finalizer.py +++ b/agent/turn_finalizer.py @@ -26,6 +26,7 @@ import os from agent.codex_responses_adapter import _summarize_user_message_for_log from agent.message_content import flatten_message_text +from agent.message_sanitization import _sanitize_surrogates def _is_pure_tool_call_tail(msg: dict) -> bool: @@ -639,6 +640,18 @@ def finalize_turn( last_reasoning = msg["reasoning"] break + # Class-level surrogate chokepoint (#80366, #55143, #55309, #19819): + # ``final_response`` is often the RAW SDK content + # (``assistant_message.content``), not the sanitized copy stored in + # history by ``build_assistant_message``. Any lone UTF-16 surrogate + # (U+D800–U+DFFF) in it crashes downstream consumers — oneshot stdout + # writes, Telegram's ``utf16_len`` length check, Signal formatting, + # JSON envelope encodes — on every provider (Ollama, NVIDIA NIM, …). + # Scrub once here, where model text leaves the conversation loop, so + # every delivery surface receives valid Unicode. + if isinstance(final_response, str): + final_response = _sanitize_surrogates(final_response) + # Build result with interrupt info if applicable result = { "final_response": final_response, diff --git a/contributors/emails/executus.ahli@gmail.com b/contributors/emails/executus.ahli@gmail.com new file mode 100644 index 0000000000..b2a14995b5 --- /dev/null +++ b/contributors/emails/executus.ahli@gmail.com @@ -0,0 +1 @@ +TheophilusChinomona diff --git a/gateway/run.py b/gateway/run.py index 3446fda9d5..31e4e17e86 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -717,6 +717,19 @@ def _sanitize_gateway_final_response(platform: Any, text: str) -> str: if _gateway_surface_passes_raw_text(platform): return text + # Lone UTF-16 surrogates (U+D800–U+DFFF) in model output crash chat + # surfaces downstream: Telegram's ``utf16_len`` length check and Signal + # formatting both ``.encode()`` the reply and raise UnicodeEncodeError + # before any send (#55143, #55309). The stored-history copy is already + # sanitized by ``build_assistant_message`` and ``finalize_turn`` scrubs + # the returned ``final_response``, but this boundary is the last line of + # defense for every legacy/plugin delivery path that hands us raw text. + # Raw-text/programmatic surfaces above keep passthrough — their JSON + # consumers escape surrogates safely. + from agent.message_sanitization import _sanitize_surrogates + + text = _sanitize_surrogates(str(text)) + # Cancellation metadata, not assistant prose. ACP/TUI already suppress # this sentinel; chat surfaces should too (#7921). if str(text).strip().startswith(INTERRUPT_WAITING_FOR_MODEL_PREFIX): diff --git a/tests/agent/test_surrogate_chokepoints.py b/tests/agent/test_surrogate_chokepoints.py new file mode 100644 index 0000000000..8aae81e20d --- /dev/null +++ b/tests/agent/test_surrogate_chokepoints.py @@ -0,0 +1,194 @@ +"""Lone-surrogate chokepoint regression tests. + +One class of bug, many crash sites: a lone UTF-16 surrogate (U+D800–U+DFFF) +in model output or a request payload crashes whichever consumer encodes it +first — oneshot stdout (#80366), Telegram's utf16_len check (#55309), Signal +formatting (#55143), provider request bodies / tool descriptions (#50959), +and NIM responses that bypass the Ollama-era sanitize pass (#19819). + +The fix is owned at chokepoints, not leaf sites: + +* ``finalize_turn`` scrubs ``final_response`` once, where model text leaves + the conversation loop (covers oneshot, gateway, API server, subagents). +* ``_sanitize_gateway_final_response`` scrubs at the gateway delivery + boundary for chat surfaces (defense in depth for legacy/plugin paths). +* ``run_conversation`` walks the fully-built ``api_kwargs`` with + ``_sanitize_structure_surrogates`` so tool descriptions and every other + request-body leaf are json-encodable before any provider sees them. +""" + +from types import SimpleNamespace + +import json + +import pytest + +from agent.message_sanitization import ( + _sanitize_structure_surrogates, + _sanitize_surrogates, +) +from agent.turn_finalizer import finalize_turn +from tests.agent.test_turn_finalizer_final_response_persistence import FakeAgent + +LONE_HIGH = "\ud83d" # unpaired high surrogate (half of an emoji pair) +LONE_LOW = "\udce7" # the exact code point reported in #19819 + + +# --------------------------------------------------------------------------- +# Chokepoint 1: model text leaving the conversation loop (finalize_turn) +# --------------------------------------------------------------------------- + + +def test_finalize_turn_scrubs_lone_surrogate_from_final_response(monkeypatch): + """#80366 / #19819: the returned final_response must be valid Unicode. + + ``final_response`` is often raw SDK content (``assistant_message.content``) + — not the sanitized history copy — so without the chokepoint a lone + surrogate reaches oneshot stdout / gateway delivery and crashes there. + """ + monkeypatch.setattr("hermes_cli.plugins.invoke_hook", lambda *_a, **_kw: []) + agent = FakeAgent() + dirty = f"answer {LONE_HIGH} and {LONE_LOW} here" + messages = [ + {"role": "user", "content": "q"}, + {"role": "assistant", "content": dirty}, + ] + + result = finalize_turn( + agent, + final_response=dirty, + api_call_count=1, + interrupted=False, + failed=False, + messages=messages, + conversation_history=[], + effective_task_id="t", + turn_id="tid", + user_message="q", + original_user_message="q", + _should_review_memory=False, + _turn_exit_reason="text_response(final)", + ) + + final = result["final_response"] + # Encodable everywhere a delivery surface needs it to be. + final.encode("utf-8") + final.encode("utf-16-le") + assert "\ufffd" in final + assert "answer " in final and " here" in final + + +def test_finalize_turn_leaves_non_string_final_response_alone(monkeypatch): + monkeypatch.setattr("hermes_cli.plugins.invoke_hook", lambda *_a, **_kw: []) + agent = FakeAgent() + result = finalize_turn( + agent, + final_response=None, + api_call_count=1, + interrupted=True, + failed=False, + messages=[{"role": "user", "content": "q"}], + conversation_history=[], + effective_task_id="t", + turn_id="tid", + user_message="q", + original_user_message="q", + _should_review_memory=False, + _turn_exit_reason="interrupted", + ) + assert result["final_response"] is None + + +# --------------------------------------------------------------------------- +# Chokepoint 2: gateway delivery boundary (mock platform send) +# --------------------------------------------------------------------------- + + +def test_gateway_final_response_sanitized_for_chat_surfaces(): + """#55309 / #55143: chat-surface replies must survive utf16_len/encode.""" + from gateway.platforms.base import utf16_len + from gateway.run import _sanitize_gateway_final_response + + dirty = f"Here is your answer {LONE_HIGH} done" + cleaned = _sanitize_gateway_final_response("telegram", dirty) + + # The raw text raises; the sanitized boundary output must not. + with pytest.raises(UnicodeEncodeError): + utf16_len(dirty) + assert utf16_len(cleaned) > 0 + cleaned.encode("utf-8") + assert "\ufffd" in cleaned + assert "Here is your answer" in cleaned + + +def test_gateway_raw_text_surfaces_keep_passthrough(): + """Programmatic surfaces keep raw text — their JSON consumers escape + surrogates safely, and byte fidelity matters there.""" + from gateway.run import _sanitize_gateway_final_response + + dirty = f"raw {LONE_HIGH} text" + assert _sanitize_gateway_final_response("local", dirty) == dirty + + +# --------------------------------------------------------------------------- +# Chokepoint 3: outbound API request assembly (#50959) +# --------------------------------------------------------------------------- + + +def test_api_kwargs_walk_makes_tool_descriptions_json_safe(): + """A surrogate anywhere in the request body (tool descriptions included) + must be scrubbed by one structure walk so json.dumps cannot fail.""" + api_kwargs = { + "model": "test", + "messages": [{"role": "user", "content": "hi"}], + "tools": [ + { + "type": "function", + "function": { + "name": "session_search", + "description": f"±5 message window {LONE_HIGH} around the match", + "parameters": { + "type": "object", + "properties": { + "query": {"description": f"nested {LONE_LOW} leaf"}, + }, + }, + }, + } + ], + "extra_body": {"note": f"deep {LONE_HIGH}"}, + } + + assert _sanitize_structure_surrogates(api_kwargs) is True + encoded = json.dumps(api_kwargs) # would raise UnicodeEncodeError-class failure before + assert "\\ud83d" not in encoded.lower() + desc = api_kwargs["tools"][0]["function"]["description"] + assert desc.startswith("±5 message window") + assert "\ufffd" in desc + # Second walk is a no-op on the now-clean payload. + assert _sanitize_structure_surrogates(api_kwargs) is False + + +def test_conversation_loop_sanitizes_api_kwargs_after_build(): + """Wiring pin: the structure walk runs on the fully-built api_kwargs + (after _build_api_kwargs, before any transport/provider sees it).""" + import inspect + + import agent.conversation_loop as cl + + src = inspect.getsource(cl.run_conversation) + build_idx = src.index("api_kwargs = agent._build_api_kwargs(api_messages)") + sanitize_idx = src.index("_sanitize_structure_surrogates(api_kwargs)") + perform_idx = src.index("def _perform_api_call") + assert build_idx < sanitize_idx < perform_idx + + +# --------------------------------------------------------------------------- +# Helper semantics shared by every chokepoint +# --------------------------------------------------------------------------- + + +def test_sanitize_surrogates_preserves_valid_astral_pairs(): + """Valid non-BMP text (proper emoji, CJK extension chars) is untouched.""" + text = "ok 😀 你好 𝕏" + assert _sanitize_surrogates(text) == text