fix(agent): legacy hidden redirect placeholders get the neutral wire payload at projection time (#88955)
The salvaged writer-side fix stamps api_content on NEW hidden redirect placeholders, but rows persisted before it (content="" + display_kind=hidden, no sidecar) would keep re-triggering repair_empty_non_final_messages on every call forever. Substitute [response interrupted] on the wire copy at the api_content/display_kind projection stage so legacy sessions converge too. Never the interrupt scaffold (#81841). Durable transcript untouched. Regression tests drive run_conversation end-to-end with a spied sanitizer: the projection must leave the sanitizer nothing to heal (its per-turn warning spam is the bug), verified failing via sabotage run against the writer-only fix. Projection-side approach credit: @JoaoMarcos44 (PR #88996).
This commit is contained in:
Teknium
@@ -2136,9 +2136,29 @@ def run_conversation(
|
||||
# from every outgoing copy so strict OpenAI-compatible backends
|
||||
# don't reject the request after a model switch or resumed typed
|
||||
# event row enters the live history.
|
||||
api_msg.pop("display_kind", None)
|
||||
_display_kind = api_msg.pop("display_kind", None)
|
||||
api_msg.pop("display_metadata", None)
|
||||
|
||||
# Legacy hidden redirect placeholders (#88955): rows persisted
|
||||
# BEFORE the writer-side api_content stamp in
|
||||
# _apply_active_turn_redirect are content="" with no sidecar.
|
||||
# Once display_kind is stripped the pre-call sanitizer
|
||||
# (repair_empty_non_final_messages) would re-heal such a row on
|
||||
# every call forever, since the durable transcript is never
|
||||
# mutated. Give the wire copy the same neutral payload here so
|
||||
# old sessions converge too. Never the interrupt scaffold —
|
||||
# replaying scaffold bytes as assistant text is #81841.
|
||||
if (
|
||||
_display_kind == "hidden"
|
||||
and api_msg.get("role") == "assistant"
|
||||
and not _api_content
|
||||
and not (api_msg.get("content") or "").strip()
|
||||
and not api_msg.get("tool_calls")
|
||||
):
|
||||
from agent.agent_runtime_helpers import _INTERRUPTED_PLACEHOLDER
|
||||
|
||||
api_msg["content"] = _INTERRUPTED_PLACEHOLDER
|
||||
|
||||
# Durable row identity stamped by _rows_to_conversation so the
|
||||
# desktop can address a specific persisted message (reactions).
|
||||
# Bookkeeping, never a provider field — only the chat-completions
|
||||
|
||||
@@ -711,3 +711,139 @@ class TestSteerCommandRegistry:
|
||||
|
||||
if __name__ == "__main__": # pragma: no cover
|
||||
pytest.main([__file__, "-v"])
|
||||
|
||||
|
||||
class TestLegacyHiddenPlaceholderWireSubstitution:
|
||||
"""Projection-side half of #88955: rows persisted BEFORE the writer-side
|
||||
``api_content`` stamp are ``content=""`` + ``display_kind="hidden"`` with
|
||||
no sidecar. The send-time projection must give the WIRE copy the neutral
|
||||
``[response interrupted]`` payload so legacy sessions converge instead of
|
||||
re-healing forever — while the durable row stays hidden and empty."""
|
||||
|
||||
def _loop_agent(self):
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from run_agent import AIAgent
|
||||
|
||||
with (
|
||||
patch("run_agent.get_tool_definitions", return_value=[]),
|
||||
patch("run_agent.check_toolset_requirements", return_value={}),
|
||||
patch("run_agent.OpenAI"),
|
||||
):
|
||||
agent = AIAgent(
|
||||
api_key="test-key-1234567890",
|
||||
base_url="https://openrouter.ai/api/v1",
|
||||
quiet_mode=True,
|
||||
skip_context_files=True,
|
||||
skip_memory=True,
|
||||
)
|
||||
agent.client = MagicMock()
|
||||
agent._cached_system_prompt = "You are helpful."
|
||||
agent._use_prompt_caching = False
|
||||
agent.tool_delay = 0
|
||||
agent.compression_enabled = False
|
||||
agent.save_trajectories = False
|
||||
return agent
|
||||
|
||||
def test_legacy_empty_hidden_assistant_row_gets_neutral_wire_payload(self):
|
||||
"""The projection itself must fill the row — the sanitizer must have
|
||||
NOTHING left to heal (its per-turn warning spam IS the bug)."""
|
||||
from unittest.mock import patch
|
||||
|
||||
import agent.agent_runtime_helpers as _arh
|
||||
|
||||
from tests.run_agent.test_run_agent import _mock_response
|
||||
|
||||
agent = self._loop_agent()
|
||||
agent.client.chat.completions.create.side_effect = [
|
||||
_mock_response(content="ok", finish_reason="stop"),
|
||||
]
|
||||
sanitizer_inputs = []
|
||||
_real_repair = _arh.repair_empty_non_final_messages
|
||||
|
||||
def _spy_repair(messages, *a, **k):
|
||||
sanitizer_inputs.append(
|
||||
[
|
||||
(m.get("role"), m.get("content"))
|
||||
for m in messages
|
||||
if isinstance(m, dict)
|
||||
]
|
||||
)
|
||||
return _real_repair(messages, *a, **k)
|
||||
# Legacy pre-fix row: no api_content sidecar.
|
||||
history = [
|
||||
{"role": "user", "content": "start"},
|
||||
{"role": "assistant", "content": "", "display_kind": "hidden"},
|
||||
{"role": "user", "content": "correction", "finish_reason": "stop"},
|
||||
{"role": "assistant", "content": "earlier reply", "finish_reason": "stop"},
|
||||
]
|
||||
|
||||
with (
|
||||
patch.object(agent, "_flush_messages_to_session_db"),
|
||||
patch.object(agent, "_persist_session"),
|
||||
patch.object(agent, "_save_trajectory"),
|
||||
patch.object(agent, "_cleanup_task_resources"),
|
||||
patch.object(
|
||||
_arh, "repair_empty_non_final_messages", side_effect=_spy_repair
|
||||
),
|
||||
):
|
||||
agent.run_conversation("next question", conversation_history=history)
|
||||
|
||||
# Precondition: the sanitizer actually ran on this call path.
|
||||
assert sanitizer_inputs, "sanitizer was never invoked — test is vacuous"
|
||||
# The projection already filled the legacy row BEFORE sanitization:
|
||||
# every assistant row the sanitizer saw carried payload, so it healed 0.
|
||||
for snapshot in sanitizer_inputs:
|
||||
for role, content in snapshot:
|
||||
if role == "assistant":
|
||||
assert (content or "").strip(), (
|
||||
"sanitizer still received an empty assistant row — "
|
||||
"the re-heal loop is back (#88955)"
|
||||
)
|
||||
|
||||
wire = agent.client.chat.completions.create.call_args.kwargs["messages"]
|
||||
wire_assistants = [m for m in wire if m.get("role") == "assistant"]
|
||||
legacy = wire_assistants[0]
|
||||
# Substituted on the wire by the projection (not the sanitizer):
|
||||
assert legacy["content"] == "[response interrupted]"
|
||||
assert "display_kind" not in legacy
|
||||
# #81841: never the interrupt scaffold.
|
||||
assert "[This response was interrupted" not in legacy["content"]
|
||||
# Durable history untouched.
|
||||
assert history[1]["content"] == ""
|
||||
assert history[1]["display_kind"] == "hidden"
|
||||
assert "api_content" not in history[1]
|
||||
|
||||
def test_hidden_row_with_tool_calls_or_text_is_not_touched(self):
|
||||
from agent.conversation_loop import _clone_message_for_send # noqa: F401
|
||||
from unittest.mock import patch
|
||||
|
||||
from tests.run_agent.test_run_agent import _mock_response
|
||||
|
||||
agent = self._loop_agent()
|
||||
agent.client.chat.completions.create.side_effect = [
|
||||
_mock_response(content="ok", finish_reason="stop"),
|
||||
]
|
||||
history = [
|
||||
{"role": "user", "content": "start"},
|
||||
{
|
||||
"role": "assistant",
|
||||
"content": "visible text",
|
||||
"display_kind": "hidden",
|
||||
"finish_reason": "stop",
|
||||
},
|
||||
{"role": "user", "content": "more"},
|
||||
{"role": "assistant", "content": "reply", "finish_reason": "stop"},
|
||||
]
|
||||
|
||||
with (
|
||||
patch.object(agent, "_flush_messages_to_session_db"),
|
||||
patch.object(agent, "_persist_session"),
|
||||
patch.object(agent, "_save_trajectory"),
|
||||
patch.object(agent, "_cleanup_task_resources"),
|
||||
):
|
||||
agent.run_conversation("next", conversation_history=history)
|
||||
|
||||
wire = agent.client.chat.completions.create.call_args.kwargs["messages"]
|
||||
wire_assistants = [m for m in wire if m.get("role") == "assistant"]
|
||||
assert wire_assistants[0]["content"] == "visible text"
|
||||
|
||||
Reference in New Issue
Block a user