From 71157cbf668e01b10a900435c1de24876c227227 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 19 Jul 2026 06:59:30 +0530 Subject: [PATCH] refactor(turn_finalizer): extract _is_pure_tool_call_tail, fix SQLite durability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the inline pure-tool-call tail check to a named helper using flatten_message_text (canonical content extraction). Fix a SQLite durability regression: the incremental tool-call persist (conversation_loop.py:4990) stamps _DB_PERSISTED_MARKER on the assistant row, so the next _persist_session flush skips it — the filled content reaches the in-memory transcript but NOT the durable store, and /resume reloads content="". Pop the marker so the next flush re-writes the row. Tests pass, ruff clean. --- agent/turn_finalizer.py | 43 ++++++++++++----- ...rn_finalizer_final_response_persistence.py | 48 +++++++++++++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) diff --git a/agent/turn_finalizer.py b/agent/turn_finalizer.py index 519459b69d9ef..ccbaac9f6190f 100644 --- a/agent/turn_finalizer.py +++ b/agent/turn_finalizer.py @@ -25,6 +25,21 @@ from __future__ import annotations import os from agent.codex_responses_adapter import _summarize_user_message_for_log +from agent.message_content import flatten_message_text + + +def _is_pure_tool_call_tail(msg: dict) -> bool: + """An assistant row with ``tool_calls`` but no visible text content of its own. + + Such a row satisfies the role check (``tail role == "assistant"``) while + carrying none of the delivered answer — see the #43849/#44100 invariant + block in :func:`finalize_turn`. Uses :func:`flatten_message_text` so that + multimodal (list-type) content is evaluated by its text parts, not just + its type. + """ + if not msg.get("tool_calls"): + return False + return not flatten_message_text(msg.get("content")).strip() def finalize_turn( @@ -228,19 +243,25 @@ def finalize_turn( _tail_role = _tail.get("role") if isinstance(_tail, dict) else None if _tail_role != "assistant": messages.append({"role": "assistant", "content": final_response}) - elif _tail.get("tool_calls") and not ( - _tail.get("content") if isinstance(_tail.get("content"), str) else "" - ).strip(): + elif isinstance(_tail, dict) and _is_pure_tool_call_tail(_tail): # The tail IS an assistant row, but a *pure tool-call turn*: - # tool_calls with no text of its own. It carries none of the - # delivered answer, so the role check alone leaves the invariant - # unmet — the user saw a response that never reached the - # transcript, and the next turn replays the user backlog and - # re-answers it (the very symptom this block was added for). - # Fill that row's empty content instead of appending, so the - # durable turn ends with the answer without disturbing the - # tool-call structure or creating an assistant→assistant pair. + # tool_calls with no text of its own. The role check alone + # leaves the #43849/#44100 invariant unmet — the user saw a + # response that never reached the transcript, and the next turn + # replays the user backlog and re-answers it (the very symptom + # this block was added for). Fill that row's empty content + # instead of appending, so the durable turn ends with the answer + # without disturbing the tool-call structure or creating an + # assistant→assistant pair. _tail["content"] = final_response + # The row may have already been flushed to SQLite by the + # incremental tool-call persist (conversation_loop.py:4990), + # which stamps ``_DB_PERSISTED_MARKER`` so subsequent flushes + # skip it. Pop the marker so the next ``_persist_session`` + # re-writes the filled content to the durable store — + # otherwise ``/resume`` reloads ``content=""`` and the bug + # resurfaces cross-session. + _tail.pop("_db_persisted", None) # The model has completed its request, so replace API-local # voice/model/skill guidance with the clean user input before writing the diff --git a/tests/agent/test_turn_finalizer_final_response_persistence.py b/tests/agent/test_turn_finalizer_final_response_persistence.py index 6c91ecba969c4..ef72cc107c64c 100644 --- a/tests/agent/test_turn_finalizer_final_response_persistence.py +++ b/tests/agent/test_turn_finalizer_final_response_persistence.py @@ -280,3 +280,51 @@ def test_final_response_does_not_clobber_tool_call_tail_with_text(monkeypatch): ) assert agent.persisted_messages[-1]["content"] == "partial text" + + +def test_fill_pops_db_persisted_marker_for_durable_rewrite(monkeypatch): + """The incremental tool-call persist stamps ``_db_persisted`` on the row. + + If finalize_turn fills the tail's content but leaves the marker, the next + ``_flush_messages_to_session_db`` skips the row and the durable SQLite + store keeps ``content=""`` — so ``/resume`` reloads the empty content and + the bug resurfaces cross-session. The fix pops the marker so the filled + content is re-written. + """ + agent = FakeAgent() + messages = [ + {"role": "user", "content": "q"}, + { + "role": "assistant", + "content": "", + "tool_calls": [ + {"id": "t1", "type": "function", + "function": {"name": "f", "arguments": "{}"}} + ], + "_db_persisted": True, # stamped by conversation_loop.py:4990 + }, + ] + + finalize_turn( + agent, + final_response="Here is your answer.", + api_call_count=3, + 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)", + ) + + persisted = agent.persisted_messages + assert persisted is not None + assert persisted[-1]["content"] == "Here is your answer." + assert persisted[-1]["tool_calls"] + assert "_db_persisted" not in persisted[-1], ( + "marker must be popped so the next flush re-writes the filled content" + )