Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions agent/conversation_loop.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,30 @@ def _restore_or_build_system_prompt(agent, system_message, conversation_history)
)


def _ensure_final_response_in_messages(messages: list, final_response) -> None:
"""Inject final_response into messages if it's a non-empty assistant reply
that isn't already represented by the tail message.

Several exit paths (partial_stream_recovery, fallback_prior_turn_content,
max_iterations) set final_response but break without appending a structured
assistant message dict. The normal text_response path already appended at
~L3833; skip re-injection when the tail already matches.
"""
if (
final_response
and isinstance(final_response, str)
and final_response.strip()
and final_response != "(empty)"
):
_tail = messages[-1] if messages else None
if not (
isinstance(_tail, dict)
and _tail.get("role") == "assistant"
and _tail.get("content") == final_response
):
messages.append({"role": "assistant", "content": final_response})


def run_conversation(
agent,
user_message: str,
Expand Down Expand Up @@ -3960,6 +3984,9 @@ def _stop_spinner():
# can replay assistant("(empty)") / recovery nudges and fall into the
# same empty-response loop again.
agent._drop_trailing_empty_response_scaffolding(messages)

_ensure_final_response_in_messages(messages, final_response)

agent._persist_session(messages, conversation_history)

# ── Turn-exit diagnostic log ─────────────────────────────────────
Expand Down
114 changes: 114 additions & 0 deletions tests/run_agent/test_bridge_assistant_persist.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
"""Regression tests for #31269 — bridge-worker silently drops assistant replies.

The fix ensures `final_response` is injected into the messages list before
`_persist_session()` when exit paths like partial_stream_recovery or
max_iterations set final_response without appending a structured assistant
message dict.
"""

from unittest.mock import MagicMock, patch, PropertyMock
import pytest


# ---------------------------------------------------------------------------
# Helpers
# ---------------------------------------------------------------------------

def _make_agent(messages=None, final_response="Hello!"):
"""Build a minimal mock agent with the attributes run_conversation uses."""
agent = MagicMock()
agent.messages = messages or [
{"role": "user", "content": "hi"},
]
agent.model = "test-model"
agent.tools = []
agent.provider = MagicMock()
agent.provider.model = "test-model"
agent._last_flushed_db_idx = 0
agent._session_db = MagicMock()
agent._drop_trailing_empty_response_scaffolding = MagicMock()
agent._persist_session = MagicMock()
return agent


# ---------------------------------------------------------------------------
# Tests
# ---------------------------------------------------------------------------

class TestFinalResponseInjectionBeforePersist:
"""Verify assistant message is injected when missing from messages."""

def test_final_response_injected_when_messages_has_no_assistant(self):
"""If messages only has user msg and final_response is set, assistant
should be appended before _persist_session."""
import agent.conversation_loop as cl

messages = [{"role": "user", "content": "hello"}]
agent = _make_agent(messages=messages)

# Simulate the tail of run_conversation: drop scaffolding, then persist.
with patch.object(cl, "final_response", "Hello from assistant", create=True):
# Directly test the injection logic
cl._ensure_final_response_in_messages(messages, "Hello from assistant")

assert messages[-1]["role"] == "assistant"
assert messages[-1]["content"] == "Hello from assistant"

def test_no_duplicate_when_assistant_already_present(self):
"""If messages tail already has the assistant response, don't re-inject."""
import agent.conversation_loop as cl

messages = [
{"role": "user", "content": "hello"},
{"role": "assistant", "content": "Hello from assistant"},
]
original_len = len(messages)

cl._ensure_final_response_in_messages(messages, "Hello from assistant")

assert len(messages) == original_len

def test_empty_final_response_not_injected(self):
"""Empty or whitespace-only final_response should NOT be injected."""
import agent.conversation_loop as cl

messages = [{"role": "user", "content": "hello"}]

cl._ensure_final_response_in_messages(messages, " ")

assert len(messages) == 1

def test_placeholder_empty_not_injected(self):
"""The '(empty)' placeholder should NOT be injected."""
import agent.conversation_loop as cl

messages = [{"role": "user", "content": "hello"}]

cl._ensure_final_response_in_messages(messages, "(empty)")

assert len(messages) == 1

def test_non_string_final_response_not_injected(self):
"""Non-string final_response (e.g. None) should not be injected."""
import agent.conversation_loop as cl

messages = [{"role": "user", "content": "hello"}]

cl._ensure_final_response_in_messages(messages, None)

assert len(messages) == 1

def test_different_assistant_tail_still_injects(self):
"""If messages tail is an assistant message but with DIFFERENT content,
the new final_response should still be injected."""
import agent.conversation_loop as cl

messages = [
{"role": "user", "content": "hello"},
{"role": "assistant", "content": "old response"},
]

cl._ensure_final_response_in_messages(messages, "new response")

assert messages[-1]["content"] == "new response"
assert messages[-2]["content"] == "old response"
Loading