fix(agent): persist recovered final responses at the finalize_turn chokepoint - #282
Conversation
|
Review Complete Files Reviewed: 3 By Severity:
The new tail-closure code in turn_finalizer.py defeats the existing '(empty)' sentinel stripping mechanism, causing the sentinel content to persist in session storage and potentially create empty-response loops on session replay. Files Reviewed (3 files) |
There was a problem hiding this comment.
Risk: 🟡 Medium (42/100) — 1 medium finding · 135 LOC across 3 files
Summary
This PR adds a tail-closure block in agent/turn_finalizer.py (lines 199-205) that ensures the final assistant response is reflected in the messages list when the tail is not already an assistant message. However, it interacts incorrectly with the existing _drop_trailing_empty_response_scaffolding (line 168) for the empty_response_exhausted recovery path.
Bug: Sentinel (empty) Persistence
When the model returns no content after exhausting retries, conversation_loop.py appends an assistant message with content="(empty)" and _empty_terminal_sentinel=True, then sets final_response = "(empty)". The existing scaffolding stripper correctly removes this flagged message. But the new tail-closure code then detects the tail is no longer assistant, and re-appends {"role": "assistant", "content": "(empty)"} without the sentinel flag. The unflagged message passes the _is_ephemeral_scaffolding filter and gets persisted.
Impact: Subsequent "continue" turns replay this synthetic "(empty)" as a real model response, which can keep long tool-heavy sessions stuck in empty-response loops.
Fix: Guard the new tail-closure against final_response == "(empty)", since this is the explicit sentinel value that the scaffolding stripper is designed to remove.
| if final_response and not interrupted: | ||
| try: | ||
| _tail_role = messages[-1].get("role") if messages else None | ||
| except Exception: | ||
| _tail_role = None | ||
| if _tail_role != "assistant": | ||
| messages.append({"role": "assistant", "content": final_response}) |
There was a problem hiding this comment.
🟡 New tail-closure code defeats '(empty)' sentinel stripping, causing '(empty)' to persist in session (bug)
During the empty_response_exhausted recovery path in conversation_loop.py (lines 4827-4866), the model returns no content after exhausting retries. The loop appends an assistant message with content="(empty)" and the _empty_terminal_sentinel=True flag (line 4838), then sets final_response = "(empty)" (line 4865) and breaks. In finalize_turn, _drop_trailing_empty_response_scaffolding at line 168 correctly strips this flagged message (and pass 2 may rewind trailing tool result/assistant pairs). However, the new code at lines 199-205 then detects the tail is no longer "assistant" (the sentinel was just popped), and re-appends {"role": "assistant", "content": "(empty)"} without the _empty_terminal_sentinel flag. This unflagged message is then persisted by _persist_session at line 207 (_flush_messages_to_session_db only filters messages by _is_ephemeral_scaffolding which checks the flag). The sentinel mechanism — explicitly designed to prevent (empty) from landing in durable storage (per docstring at lines 4832-4836, also per the comment at line 163-166) — is completely bypassed. A subsequent "continue" turn will replay assistant("(empty)") as if it were a real model response, which can keep long tool-heavy sessions stuck in empty-response loops.
💡 Suggestion: Guard the new tail-closure block against the sentinel value so that recovery paths producing "(empty)" do not re-add the sentinel content. The cleanest fix is to exclude final_response == "(empty)" from the guard condition, since this is the explicit sentinel value that _drop_trailing_empty_response_scaffolding is designed to strip.
| if final_response and not interrupted: | |
| try: | |
| _tail_role = messages[-1].get("role") if messages else None | |
| except Exception: | |
| _tail_role = None | |
| if _tail_role != "assistant": | |
| messages.append({"role": "assistant", "content": final_response}) | |
| if final_response and not interrupted and final_response != "(empty)": | |
| try: | |
| _tail_role = messages[-1].get("role") if messages else None | |
| except Exception: | |
| _tail_role = None | |
| if _tail_role != "assistant": | |
| messages.append({"role": "assistant", "content": final_response}) |
📋 Prompt for AI Agents
In agent/turn_finalizer.py line 199, change the guard condition from if final_response and not interrupted: to if final_response and not interrupted and final_response != "(empty)":. This prevents the new tail-closure logic from re-adding the "(empty)" sentinel content that _drop_trailing_empty_response_scaffolding correctly stripped at line 168. The sentinel path is already handled by the conversation loop's own scaffolding mechanism and should not be re-closed here. Add a test in tests/agent/test_turn_finalizer_final_response_persistence.py that passes final_response="(empty)" with _turn_exit_reason="empty_response_exhausted" and asserts the persisted messages list does NOT contain an "(empty)" content message.
Summary
A delivered assistant response now always ends up in the session transcript. Recovery
breakpaths inconversation_loop(partial-stream recovery, prior-turn-content fallback) setfinal_responseand exit the loop without appending an assistant message, so_persist_sessionwrote no assistant row for the turn — even though the user (and the messaging platform) already saw the reply. The next turn then replayed a user-only backlog and the model re-answered every "unanswered" message.This salvages @WXBR's NousResearch#46183, which fixes the class at the single chokepoint every recovery path flows through (
agent/turn_finalizer.py::finalize_turn) rather than patching each individualbreaksite.Changes
agent/turn_finalizer.py: before_persist_session, iffinal_responseis set and the turn wasn't interrupted and the transcript tail isn't already an assistant message, append the response as an assistant row. Placed after the existinginterruptedtool-close handling (which owns that path), inside the persist try/except.scripts/release.py: AUTHOR_MAP entry for @WXBR.Root cause
partial_stream_recovery(conversation_loop.py:~4604) andfallback_prior_turn_content(~4635) dofinal_response = ...; breakwith no assistant append. Both exit intofinalize_turn, whose_flush_messages_to_session_dbonly writes dicts present inmessages— so the delivered text was never persisted. The gateway can't backfill it either: its post-turn writes useskip_db=agent_persisted, which is a true no-op when a SessionDB exists (gateway/session.py:1747). Fixing at the finalizer chokepoint covers all recovery paths at once and makes a gateway change unnecessary.Validation
Live E2E: real
AIAgent+ real SQLiteSessionDBdriving the realfinalize_turn→_persist_sessionpath.user, assistant, user, useruser, assistant, user, assistantRegression cases (with fix): normal turn with an assistant already present → exactly one assistant row, no duplicate; fallback tool-tail
[user, assistant, tool]→ closed with an assistant row, notool→usernext-turn hazard. The_tail_role != "assistant"check prevents double-writes;final_response and not interruptedkeeps the guard out of the interrupt path.Targeted tests: @WXBR's
test_turn_finalizer_final_response_persistence.py+ 53 neighbors (incl.test_turn_finalizer_interrupt_alternation,test_turn_finalizer_cleanup_guard) all pass.Related
Canonical issue NousResearch#43849 (Telegram duplicate NousResearch#44100). Supersedes the cluster of per-site / gateway-only approaches (NousResearch#44120, NousResearch#43853, NousResearch#46471, NousResearch#54114) by fixing the class at the chokepoint. Credit to @WXBR (this fix), @AIalliAI (NousResearch#44120), and @rungmc357 (NousResearch#43849 reporter + NousResearch#43853).
Fixes NousResearch#43849
Infographic
Mirror-of: NousResearch#56279
NousResearch#56279