refactor(agent): one outbound-kwargs sanitizer seam for the main loop and the summary
The summary path had grown a verbatim copy of turn_api_request's 3-line surrogate/ASCII chokepoint — the same drift class this PR removes for the hand-rolled kwargs builder. Move the two lines and the #50959 rationale into `message_sanitization.sanitize_outbound_kwargs` and call it from both sites, so the next sanitizer step added to the main loop cannot miss the summary. Tighten two comments: "same kwargs builder" (cache_control redecoration is not re-applied here) and a `_summary_text` note that is true for all three summary branches, not just the chat one.
This commit is contained in:
@@ -38,8 +38,8 @@ from agent.model_metadata import is_local_endpoint
|
||||
from agent.message_content import flatten_message_text
|
||||
from agent.message_metadata import append_message, stamp_message_timestamp
|
||||
from agent.message_sanitization import (
|
||||
_sanitize_structure_non_ascii, _sanitize_structure_surrogates, _sanitize_surrogates,
|
||||
_repair_tool_call_arguments, normalize_finish_reason as _normalize_finish_reason,
|
||||
_sanitize_surrogates, _repair_tool_call_arguments, normalize_finish_reason as _normalize_finish_reason,
|
||||
sanitize_outbound_kwargs,
|
||||
)
|
||||
from agent.reasoning_summaries import append_streamed_reasoning_detail, separate_glued_reasoning_blocks
|
||||
from agent.stream_single_writer import claim_stream_writer, stream_writer_is_current
|
||||
@@ -2015,8 +2015,8 @@ def _managed_summary_call(agent, api_request_id: str, request, callback, *, retr
|
||||
def _summary_text(agent, response, **normalize_kwargs) -> str:
|
||||
normalized = agent._get_transport().normalize_response(response, **normalize_kwargs)
|
||||
if normalized.tool_calls:
|
||||
# The summary request carries tools (cache lineage), but this path never executes a
|
||||
# call; log so a tool-only response that lands in the empty-summary retry is diagnosable.
|
||||
# No summary path executes tool calls; log so a tool-only response that falls into the
|
||||
# empty-summary retry is diagnosable.
|
||||
logger.warning("Iteration summary emitted tool calls; discarding them")
|
||||
return (normalized.content or "").strip()
|
||||
|
||||
@@ -2046,16 +2046,14 @@ def _anthropic_summary_attempt(agent, api_messages: list, api_request_id: str):
|
||||
|
||||
|
||||
def _chat_summary_attempt(agent, api_messages: list, api_request_id: str):
|
||||
# Same builder as the main loop so the summary keeps the cached prefix (tools, prompt_cache_key,
|
||||
# xAI alias, Moonshot sanitization). Do not omit tools or force tool_choice="none" here:
|
||||
# SGLang renders the prompt with tools=None in that mode and the KV prefix diverges.
|
||||
# Same kwargs builder as the main loop so the summary keeps the cached prefix (tools,
|
||||
# prompt_cache_key, xAI alias, Moonshot sanitization). Do not omit tools or force
|
||||
# tool_choice="none" here: SGLang renders the prompt with tools=None in that mode and the KV
|
||||
# prefix diverges. (cache_control breakpoint decoration is not re-applied on this path.)
|
||||
summary_kwargs = agent._build_api_kwargs(api_messages)
|
||||
# Same outbound chokepoint as turn_api_request: the summary now carries ``tools``, and on
|
||||
# cache-planned routes the main loop scrubbed a deep copy, so ``agent.tools`` may still hold
|
||||
# the lone surrogates / non-ASCII bytes the provider 400s on (#50959 class).
|
||||
_sanitize_structure_surrogates(summary_kwargs)
|
||||
if agent._force_ascii_payload:
|
||||
_sanitize_structure_non_ascii(summary_kwargs)
|
||||
# The summary now carries ``tools``; on cache-planned routes the main loop scrubbed a deep
|
||||
# copy, so ``agent.tools`` may still hold bytes the provider 400s on.
|
||||
sanitize_outbound_kwargs(agent, summary_kwargs)
|
||||
|
||||
def _attempt(retry_count: int) -> str:
|
||||
summary_client = agent._ensure_primary_openai_client(reason="iteration_limit_summary_retry" if retry_count else "iteration_limit_summary")
|
||||
|
||||
@@ -95,6 +95,19 @@ _sanitize_messages_non_ascii = partial(_sanitize_messages, fix=_strip_non_ascii,
|
||||
_sanitize_tools_non_ascii = _sanitize_structure_non_ascii
|
||||
|
||||
|
||||
def sanitize_outbound_kwargs(agent: Any, api_kwargs: dict) -> None:
|
||||
"""Outbound-request chokepoint for every built kwargs dict (main loop and iteration summary).
|
||||
|
||||
Tool descriptions, extra_body and kwargs strings can carry invalid code points that
|
||||
providers reject with a non-retryable 400 (#50959); one in-place walk makes the whole
|
||||
payload json.dumps()-safe. The ASCII strip is opt-in via the recovery flag set after an
|
||||
ASCII-codec rejection.
|
||||
"""
|
||||
_sanitize_structure_surrogates(api_kwargs)
|
||||
if agent._force_ascii_payload:
|
||||
_sanitize_structure_non_ascii(api_kwargs)
|
||||
|
||||
|
||||
def _escape_invalid_chars_in_json_strings(raw: str) -> str:
|
||||
"""Escape literal control chars (0x00-0x1F) inside JSON string values as ``\\uXXXX``
|
||||
(for llama.cpp-style output mixing control chars with other malformations)."""
|
||||
@@ -315,7 +328,7 @@ __all__ = [
|
||||
"_sanitize_surrogates", "_sanitize_structure_surrogates", "_sanitize_messages_surrogates",
|
||||
"_escape_invalid_chars_in_json_strings", "_repair_tool_call_arguments",
|
||||
"_strip_non_ascii", "_sanitize_messages_non_ascii", "_sanitize_tools_non_ascii",
|
||||
"_strip_images_from_messages", "_sanitize_structure_non_ascii",
|
||||
"_strip_images_from_messages", "_sanitize_structure_non_ascii", "sanitize_outbound_kwargs",
|
||||
# call_id policy owners
|
||||
"deterministic_call_id", "coalesce_tool_call_id", "tool_call_id_variants",
|
||||
"tool_result_id_variants", "uniquify_tool_call_ids",
|
||||
|
||||
@@ -12,9 +12,7 @@ from dataclasses import dataclass
|
||||
import logging
|
||||
from typing import Any
|
||||
|
||||
from agent.message_sanitization import (
|
||||
_sanitize_structure_non_ascii, _sanitize_structure_surrogates
|
||||
)
|
||||
from agent.message_sanitization import sanitize_outbound_kwargs
|
||||
from utils import env_var_enabled
|
||||
|
||||
logger = logging.getLogger("agent.conversation_loop")
|
||||
@@ -120,17 +118,9 @@ def build_api_request(
|
||||
api_kwargs = agent._build_api_kwargs(api_messages)
|
||||
else:
|
||||
api_kwargs = agent._build_api_kwargs(api_messages, tools_for_api=tools_for_api)
|
||||
# Surrogate chokepoint: tool descriptions, extra_body and kwargs strings can carry
|
||||
# invalid code points (HTTP 400). One walk makes the payload json.dumps()-safe.
|
||||
# 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)
|
||||
# Messages were scrubbed above; this walk covers the rest of the payload (tool descriptions,
|
||||
# extra_body, kwargs strings) — see sanitize_outbound_kwargs for the #50959 rationale.
|
||||
sanitize_outbound_kwargs(agent, api_kwargs)
|
||||
if agent.api_mode == "codex_responses":
|
||||
api_kwargs = agent._get_transport().preflight_kwargs(
|
||||
api_kwargs, allow_stream=False, is_github_responses=agent._is_copilot_url(),
|
||||
|
||||
@@ -183,7 +183,7 @@ def test_conversation_loop_sanitizes_api_kwargs_after_build():
|
||||
|
||||
src = inspect.getsource(rq.build_api_request)
|
||||
build_idx = src.index("api_kwargs = agent._build_api_kwargs(api_messages)")
|
||||
sanitize_idx = src.index("_sanitize_structure_surrogates(api_kwargs)")
|
||||
sanitize_idx = src.index("sanitize_outbound_kwargs(agent, api_kwargs)")
|
||||
assert build_idx < sanitize_idx
|
||||
loop_src = inspect.getsource(cl._run_api_retry_loop)
|
||||
assert loop_src.index("build_api_request,") < loop_src.index("perform_api_call,")
|
||||
|
||||
Reference in New Issue
Block a user