refactor: single owner for empty-content wire repair (class fix)

The concept 'never send a turn that strict wire validation rejects as
empty' was forked across four sites, each with its own predicate and its
own blind spots:

1. build_assistant_message write-time ' ' pad — broke codex commentary
   turns (content:'' is a designed state), and a DB-side pad can't
   survive _rows_to_conversation's whitespace strip anyway. REMOVED.
2. conversation_loop send-time ' ' pad — main-loop only (summary path
   uncovered), ordering-fragile (had to run after whitespace
   normalization), assistant-only. REMOVED.
3. stream-stub '[response interrupted]' substitution — defeated the
   loop's empty-stub guard (the stub no longer looked empty, entered
   history, and the placeholder leaked into the stitched final
   response via truncated_response_parts). REMOVED.
4. repair_empty_non_final_messages in sanitize_api_messages — the
   unconditional pre-send chokepoint shared by the main loop AND the
   summary path, covers user and assistant turns, non-final only,
   copy-on-write. This is now the SINGLE OWNER.

The owner's payload predicate (_msg_has_payload) is extended to treat
codex_message_items / codex_reasoning_items as payload, so
designed-empty codex commentary turns are never rewritten on any
api_mode — the failure shape that broke site 1 in CI is encoded in the
owner, not special-cased at a call site.

Tests updated to pin the new contracts: builder stores textless turns
as-is; the empty stream stub stays recognizably empty for the loop
guard; poisoned resumed histories are repaired to the placeholder at
the send boundary; codex item carriers are never rewritten.
Sabotage-verified: unwiring the owner fails 3 regression tests.
This commit is contained in:
Hermes Agent 2026-07-27 19:54:21 -07:00 committed by Teknium
parent df45811198
commit 725c7ba534
8 changed files with 143 additions and 164 deletions

View File

@ -2819,6 +2819,18 @@ def _msg_has_payload(msg: Dict[str, Any]) -> bool:
return True
if msg.get("reasoning") or msg.get("reasoning_details"):
return True
# Codex Responses item carriers: a commentary-phase assistant turn
# persists with content:"" by DESIGN — its text lives in
# ``codex_message_items`` (delivered via the interim callback) and the
# structured items are replayed for prefix-cache hits. Same for
# ``codex_reasoning_items``. These turns are never wire-empty on any
# api_mode: the codex transport replays the items, and the
# chat-completions transport strips the carriers only after this repair
# pass has already run. Treat them as payload so the repair never
# rewrites a designed-empty codex turn (July 2026: a write-time pad that
# ignored this broke codex commentary replay in CI).
if msg.get("codex_message_items") or msg.get("codex_reasoning_items"):
return True
return False

View File

@ -1373,31 +1373,16 @@ def build_assistant_message(agent, assistant_message, finish_reason: str) -> dic
from agent.redact import redact_sensitive_text
_san_content = redact_sensitive_text(_san_content)
# Defence-in-depth: never serialize a TEXTLESS assistant turn with an
# empty content string. Providers with strict validation (Moonshot/Kimi
# via OpenRouter: "the message at position N with role 'assistant' must
# not be empty") reject the replay with HTTP 400, which permanently
# poisons the persisted session — every subsequent turn re-sends the
# offending message. Reachable via the partial-stream-stub path when a
# stream drops before delivering any text (the loop now skips that case,
# but other callers of this builder get the same guarantee). A single
# space satisfies non-empty validation without fabricating content —
# the same trick the reasoning_content pad uses above (#15250, #17400).
# Tool-call turns are exempt: ``content: ""`` alongside ``tool_calls``
# is accepted everywhere and normalizing it would alter cache keys.
# codex_responses is exempt too: empty assistant content is a designed
# first-class state there (commentary-phase messages persist with
# content:"" while their text is delivered via the interim callback),
# and the Responses wire has no "assistant must not be empty"
# validation. A codex session later replayed through a strict
# chat-completions provider is still repaired by the send-time pad,
# which keys on the ACTIVE api_mode at replay time.
if (
not _san_content
and not assistant_tool_calls
and getattr(agent, "api_mode", None) != "codex_responses"
):
_san_content = " "
# NOTE (empty-content class fix): textless assistant turns are NOT padded
# here. The single owner for "never send a turn strict wire validation
# rejects as empty" is ``repair_empty_non_final_messages`` in
# agent_runtime_helpers, which runs inside ``sanitize_api_messages`` — the
# unconditional pre-send chokepoint for both the main loop and the summary
# path. Padding at write time was tried (a single-space pad, later a
# placeholder) and rejected: it forked the concept across three sites,
# broke codex commentary turns (content:'' is a designed state there), and
# a DB-side pad can't survive ``_rows_to_conversation``'s whitespace strip
# anyway. Repair belongs at the send boundary, once.
msg = {
"role": "assistant",
@ -3944,29 +3929,17 @@ def interruptible_streaming_api_call(agent, api_kwargs: dict, *, on_first_delta=
result["error"],
)
_stub_finish_reason = FINISH_REASON_LENGTH
# Never persist a content-less, tool-call-less assistant stub.
# When a stream dies before any text arrives (and no partial tool
# calls were captured), _partial_text is None → the stub becomes an
# empty assistant message. The Anthropic message schema (and the
# litellm/Bedrock proxies in front of it) reject any request whose
# transcript contains an empty non-final message:
# "all messages must have non-empty content except for the
# optional final assistant message" (400 INVALID_REQUEST_BODY)
# Once such a stub lands mid-transcript, EVERY subsequent turn 400s
# until it scrolls out of context — and the 400 gets misread as a
# context-overflow "Cannot compress further" loop. Substitute a
# minimal, honest placeholder so the message is always API-valid
# and the continuation prompt still reads as an interrupted turn.
if not _partial_text:
_partial_text = "[response interrupted]"
logger.warning(
"Empty partial-stream stub (0 chars recovered, no tool "
"call) — substituting placeholder content so the assistant "
"message is not persisted empty (would otherwise 400 every "
"later request with 'messages must have non-empty content' "
"/ INVALID_REQUEST_BODY). error=%s",
result["error"],
)
# NOTE (empty-content class fix): the stub is deliberately allowed
# to carry empty content here. The conversation loop's truncation
# path detects an EMPTY partial-stream stub (PARTIAL_STREAM_STUB_ID
# + no content) and skips appending it to history entirely — only
# the continuation nudge is sent. Substituting placeholder text at
# this site was tried and reverted: it defeats that guard (the stub
# no longer looks empty), gets appended to history, and the
# placeholder leaks into the stitched final response via
# truncated_response_parts. Transcripts that already carry a
# persisted empty turn are healed at the send boundary by
# ``repair_empty_non_final_messages`` (the single owner).
_stub_msg = SimpleNamespace(
role="assistant", content=_partial_text, tool_calls=None,
reasoning_content=None,

View File

@ -1638,37 +1638,14 @@ def run_conversation(
# the OpenAI SDK. Sanitizing here prevents the 3-retry cycle.
_sanitize_messages_surrogates(api_messages)
# Pad a textless assistant turn's empty content to a single space.
# Strict providers (Moonshot/Kimi via OpenRouter: "the message at
# position N with role 'assistant' must not be empty") reject the
# replay with HTTP 400 — and the session is poisoned for every
# subsequent turn. This is the DURABLE repair for ALREADY-poisoned
# persisted sessions: the partial-stream-stub rows older builds
# wrote (content:'' finish_reason:'length') are rebuilt to '' on
# every reload — ``_rows_to_conversation`` strips whitespace, so a
# DB-side pad can't survive — and only a SEND-time pad repairs
# them. It must run AFTER the whitespace-normalization pass above
# (which would strip the pad back to '') and after
# _drop_thinking_only_and_merge_users (which can leave a textless
# turn), but BEFORE apply_anthropic_cache_control rewrites content
# into list blocks. Tool-call turns are exempt: ``content: ''``
# alongside ``tool_calls`` is accepted everywhere and normalizing
# it would alter prompt-cache keys. codex_responses is exempt:
# empty assistant content is a designed first-class state there
# (commentary-phase turns persist with content:'') and the
# Responses wire has no empty-content validation. Keying on the
# ACTIVE api_mode means a codex-written empty turn is still
# repaired the moment the session replays through a strict
# chat-completions provider.
if getattr(agent, "api_mode", None) != "codex_responses":
for am in api_messages:
if (
am.get("role") == "assistant"
and not am.get("tool_calls")
and isinstance(am.get("content"), str)
and not am["content"].strip()
):
am["content"] = " "
# NOTE (empty-content class fix): no send-time pad loop here. The
# single owner for "never send a turn strict wire validation rejects
# as empty" is ``repair_empty_non_final_messages``, which runs inside
# ``_sanitize_api_messages`` above — the unconditional pre-send
# chokepoint shared with the summary path. Its placeholder is
# non-whitespace, so it survives the whitespace-normalization pass
# regardless of ordering (a single-space pad here previously had to
# be sequenced after normalization to survive, forking the concept).
# Apply Anthropic prompt caching for Claude models on native
# Anthropic, OpenRouter, and third-party Anthropic-compatible

View File

@ -0,0 +1 @@
aweiker

View File

@ -868,3 +868,35 @@ def test_sanitize_preserves_reasoning_only_and_toolcall_turns():
assert tc_asst.get("tool_calls") and tc_asst["tool_calls"][0]["id"] == "call_R"
assert tc_asst["content"] != "[response interrupted]"
def test_sanitize_preserves_codex_item_carrier_turns():
"""Negative control: a codex commentary-phase assistant turn persists with
content:'' by DESIGN its text lives in codex_message_items (delivered
via the interim callback, replayed as structured items for prefix-cache
hits). The empty-content repair must treat the item carriers as payload
and never rewrite these turns (July 2026: a write-time pad that ignored
this broke codex commentary replay in CI)."""
from agent.agent_runtime_helpers import sanitize_api_messages
messages = [
{"role": "user", "content": "analyze repo"},
{"role": "assistant", "content": "", "codex_message_items": [
{"id": "msg_1", "phase": "commentary",
"content": [{"type": "output_text", "text": "I'll inspect first."}]},
]},
{"role": "user", "content": "go on"},
{"role": "assistant", "content": "", "codex_reasoning_items": [
{"type": "reasoning", "id": "rs_1", "encrypted_content": "opaque"},
]},
{"role": "user", "content": "final"},
]
out = sanitize_api_messages(list(messages))
commentary = out[1]
reasoning_carrier = out[3]
assert commentary["content"] == "", (
"codex_message_items carrier must keep its designed-empty content"
)
assert reasoning_carrier["content"] == "", (
"codex_reasoning_items carrier must keep its designed-empty content"
)

View File

@ -716,11 +716,11 @@ class TestEmptyPartialStreamStubNotPersisted:
class TestBuildAssistantMessageEmptyContentPad:
"""Regression layer 2 (chat_completion_helpers.build_assistant_message):
never serialize a textless assistant turn with ``content: ""`` pad to
a single space, the same trick as the reasoning_content pad (#15250).
Tool-call turns are exempt (``content: ""`` + ``tool_calls`` is accepted
everywhere)."""
"""Layer 2 was consolidated into the class owner: the builder stores
textless turns AS-IS (no write-time pad a pad here broke codex
commentary turns and forked the concept). Wire safety is owned by
``repair_empty_non_final_messages`` inside ``sanitize_api_messages``.
These tests pin the builder's store-as-is contract."""
def _agent_for_builder(self):
from run_agent import AIAgent
@ -738,24 +738,24 @@ class TestBuildAssistantMessageEmptyContentPad:
)
return a
def test_empty_content_padded_to_space(self):
def test_empty_content_stored_as_is(self):
from agent.chat_completion_helpers import build_assistant_message
from tests.run_agent.test_run_agent import _mock_assistant_msg
agent = self._agent_for_builder()
msg = build_assistant_message(agent, _mock_assistant_msg(content=""), "stop")
assert msg["content"] == " ", (
"Textless assistant turn must be padded to a single space — "
"Moonshot/Kimi reject empty assistant content with HTTP 400."
assert msg["content"] == "", (
"Builder must store textless turns as-is — wire repair is owned "
"by repair_empty_non_final_messages at the send boundary."
)
def test_none_content_padded_to_space(self):
def test_none_content_stored_as_empty(self):
from agent.chat_completion_helpers import build_assistant_message
from tests.run_agent.test_run_agent import _mock_assistant_msg
agent = self._agent_for_builder()
msg = build_assistant_message(agent, _mock_assistant_msg(content=None), "stop")
assert msg["content"] == " "
assert msg["content"] == ""
def test_tool_call_turn_content_left_empty(self):
from agent.chat_completion_helpers import build_assistant_message
@ -767,10 +767,7 @@ class TestBuildAssistantMessageEmptyContentPad:
_mock_assistant_msg(content="", tool_calls=[_mock_tool_call()]),
"tool_calls",
)
assert msg["content"] == "", (
"Tool-call turns are exempt from the pad: content:'' alongside "
"tool_calls is accepted by every provider."
)
assert msg["content"] == ""
assert msg["tool_calls"]
def test_non_empty_content_unchanged(self):
@ -787,11 +784,12 @@ class TestSendTimeEmptyAssistantPad:
-stream-stub row written by an older build (content:'' ,
finish_reason:'length') is rebuilt to content:'' on every reload
``_rows_to_conversation`` strips whitespace, so a DB-side pad cannot
survive. The send-time pad in conversation_loop's api_messages loop
must therefore repair the empty textless assistant turn at the
serialization boundary, so a RESUMED poisoned session replays
cleanly against strict providers (Moonshot/Kimi HTTP 400 "message ...
with role 'assistant' must not be empty")."""
survive. The class owner ``repair_empty_non_final_messages`` (inside
``sanitize_api_messages``, the pre-send chokepoint) must repair the
empty textless assistant turn at the serialization boundary, so a
RESUMED poisoned session replays cleanly against strict providers
(Moonshot/Kimi HTTP 400 "message ... with role 'assistant' must not
be empty" / Anthropic "all messages must have non-empty content")."""
def _run_one_turn_with_history(self, loop_agent, history):
from tests.run_agent.test_run_agent import _mock_response
@ -809,7 +807,7 @@ class TestSendTimeEmptyAssistantPad:
kwargs = loop_agent.client.chat.completions.create.call_args_list[0]
return kwargs.kwargs.get("messages") or kwargs.args[0].get("messages")
def test_poisoned_resumed_history_padded_on_send(self, loop_agent):
def test_poisoned_resumed_history_repaired_on_send(self, loop_agent):
# Byte-shape of a persisted poisoned session:
# user -> assistant('' , finish_reason='length', NO tool_calls) -> user.
poisoned = [
@ -822,7 +820,7 @@ class TestSendTimeEmptyAssistantPad:
m for m in sent
if m.get("role") == "assistant"
and not m.get("tool_calls")
and m.get("content") == ""
and not (m.get("content") or "").strip()
]
assert empties == [], (
"A resumed session carrying a persisted empty partial-stream "
@ -834,7 +832,7 @@ class TestSendTimeEmptyAssistantPad:
and not m.get("tool_calls")),
None,
)
assert stub is not None and stub["content"] == " "
assert stub is not None and stub["content"] == "[response interrupted]"
def test_tool_call_turn_not_padded_on_send(self, loop_agent):
history = [
@ -864,18 +862,16 @@ class TestSendTimeEmptyAssistantPad:
class TestSendTimePadMultimodalSafety:
"""Regression: the send-time pad must skip non-string (list) assistant
"""Regression: the send-time repair must skip non-string (list) assistant
content instead of crashing a forked session whose new user turn
attaches an image hit AttributeError: 'list' object has no attribute
'strip' inside the pad loop.
'strip' inside an earlier pad loop.
Note: current main flattens multimodal assistant list-content to a
plain string upstream of the send boundary, so the list shape rarely
survives to the pad loop in this path but other builders/callers can
still produce list content, and the ``isinstance(str)`` guard must hold
regardless of upstream flattening. This test drives a multimodal
The repair is now owned by ``repair_empty_non_final_messages``, whose
``_msg_has_payload`` treats a list with any typed block as payload
multimodal turns are never rewritten. This test drives a multimodal
history through the loop and asserts (a) no crash, and (b) the
assistant turn's text is neither dropped nor replaced by the pad.
assistant turn's text is neither dropped nor replaced.
"""
def test_multimodal_assistant_content_not_touched(self, loop_agent):
@ -917,27 +913,23 @@ class TestSendTimePadMultimodalSafety:
)
else:
assert "I see an image" in (c or ""), (
"Flattened multimodal assistant text must survive the pad loop."
"Flattened multimodal assistant text must survive the repair."
)
assert c != " ", "The pad must never replace real multimodal content."
def test_pad_loop_skips_list_content_directly(self):
"""Unit-shape check: the pad predicate itself must skip list content
(the exact AttributeError shape) and pad only textless str turns."""
def test_repair_owner_skips_list_content_directly(self):
"""Unit-shape check against the REAL owner: multimodal list content
(the exact AttributeError shape) passes through untouched; a textless
str turn is repaired; tool-call turns are exempt."""
from agent.agent_runtime_helpers import repair_empty_non_final_messages
api_messages = [
{"role": "assistant", "content": [{"type": "text", "text": "hi"}]},
{"role": "assistant", "content": ""},
{"role": "assistant", "content": "", "tool_calls": [{"id": "c1"}]},
{"role": "user", "content": "trailing turn keeps the above non-final"},
]
# Mirror of the send-boundary pad in conversation_loop.
for am in api_messages:
if (
am.get("role") == "assistant"
and not am.get("tool_calls")
and isinstance(am.get("content"), str)
and not am["content"].strip()
):
am["content"] = " "
assert api_messages[0]["content"] == [{"type": "text", "text": "hi"}]
assert api_messages[1]["content"] == " "
assert api_messages[2]["content"] == ""
out = repair_empty_non_final_messages(api_messages)
assert out[0]["content"] == [{"type": "text", "text": "hi"}]
assert out[1]["content"] == "[response interrupted]"
assert out[2]["content"] == ""
# input list untouched (repair is copy-on-write)
assert api_messages[1]["content"] == ""

View File

@ -2215,13 +2215,12 @@ class TestBuildAssistantMessage:
assert result["reasoning_details"][0]["text"] == "step1"
def test_empty_content(self, agent):
# Textless assistant turns are padded to a single space: strict
# providers (Moonshot/Kimi via OpenRouter) reject empty assistant
# content with HTTP 400 ("message ... with role 'assistant' must
# not be empty") on replay, permanently poisoning the session.
# The builder stores textless turns as-is; wire safety for strict
# providers ("assistant must not be empty" 400s) is owned by
# repair_empty_non_final_messages at the send boundary, not here.
msg = _mock_assistant_msg(content=None)
result = agent._build_assistant_message(msg, "stop")
assert result["content"] == " "
assert result["content"] == ""
def test_streaming_only_reasoning_promoted_to_reasoning_content(self, agent):
"""Refs #16844 / #16884. Streaming-only providers (glm, MiniMax,
@ -2350,9 +2349,9 @@ class TestBuildAssistantMessage:
result = agent._build_assistant_message(msg, "stop")
assert "<think>" not in result["content"]
assert "reasoning that never closes" not in result["content"]
# Stripped-to-empty textless turns are padded to a single space
# (Moonshot/Kimi reject empty assistant content on replay).
assert result["content"] == " "
# Stripped-to-empty content is stored as-is; wire safety for strict
# providers is owned by repair_empty_non_final_messages at send time.
assert result["content"] == ""
class TestFormatToolsForSystemMessage:

View File

@ -1585,36 +1585,29 @@ class TestPartialToolCallWarning:
@patch("run_agent.AIAgent._create_request_openai_client")
@patch("run_agent.AIAgent._close_request_openai_client")
def test_empty_partial_stream_never_yields_empty_content(
def test_empty_partial_stream_stub_stays_empty_for_loop_guard(
self, mock_close, mock_create,
):
"""Stream dies with 0 recovered chars and no tool call → stub content
must be a non-empty placeholder, never None/''.
"""Stream dies with 0 recovered chars and no tool call → the stub
keeps its empty content ON PURPOSE.
Root-cause regression for the empty-assistant-stub bug: a stream
delivered some deltas, then died before any text landed in
``_current_streamed_assistant_text`` (and no tool call was captured).
The stub was built with ``content=None`` an empty assistant message
got persisted mid-transcript. The Anthropic message schema (and the
litellm/Bedrock proxies in front of it) then reject EVERY subsequent
request:
"all messages must have non-empty content except for the optional
final assistant message" (400 INVALID_REQUEST_BODY)
which the loop misreads as a context-overflow "Cannot compress
further" spiral. The stub must carry a minimal placeholder so the
message is always API-valid.
The conversation loop's truncation path detects an EMPTY
partial-stream stub (PARTIAL_STREAM_STUB_ID + no content) and skips
appending it to history entirely only the continuation nudge is
sent (the #68041 class fix). An earlier iteration substituted
'[response interrupted]' placeholder text HERE, which defeated that
guard: the stub no longer looked empty, entered history, and the
placeholder leaked into the stitched final response. Transcripts
that already carry a persisted empty turn are healed at the send
boundary by repair_empty_non_final_messages instead.
"""
from run_agent import AIAgent
from hermes_constants import PARTIAL_STREAM_STUB_ID
class _StallError(RuntimeError):
pass
def _stalling_stream():
# A real content delta fires (deltas_were_sent=True → the
# post-delivery stub path runs), but the recovered-text
# accumulator is empty by the time the stub is built (cleared on
# reset in prod), and no tool call was captured — the exact
# "0 chars recovered, no tool call" production condition.
yield _make_stream_chunk(content="partial token")
raise _StallError("simulated upstream stall after a delta")
@ -1635,8 +1628,8 @@ class TestPartialToolCallWarning:
agent.api_mode = "chat_completions"
agent._interrupt_requested = False
agent._fire_stream_delta = lambda text: None
# Empty recovered text — this is what produced the empty stub in prod
# even though a delta was delivered to the platform above.
# Empty recovered text — the exact "0 chars recovered, no tool call"
# production condition.
agent._current_streamed_assistant_text = ""
import os as _os
@ -1650,14 +1643,14 @@ class TestPartialToolCallWarning:
else:
_os.environ["HERMES_STREAM_RETRIES"] = _prev
# The stub must be RECOGNIZABLY empty so the loop guard can skip it.
assert getattr(response, "id", "") == PARTIAL_STREAM_STUB_ID
content = response.choices[0].message.content
assert content, (
f"Empty-partial-stream stub must NOT have empty/None content "
f"(it poisons the transcript and 400s every later request). "
f"Got content={content!r}"
)
assert content.strip() != "", (
f"Stub content is whitespace-only, still API-invalid: {content!r}"
assert not content, (
f"Empty-partial-stream stub must keep empty content so the "
f"conversation loop's empty-stub guard can detect and skip it — "
f"substituted text defeats the guard and leaks into the final "
f"response. Got content={content!r}"
)
assert response.choices[0].message.tool_calls is None