fix(agent): inject streamed final_response into messages before persistence (#31269) - #31314
fix(agent): inject streamed final_response into messages before persistence (#31269)#31314xxxigm wants to merge 2 commits into
Conversation
…stence (NousResearch#31269) The bridge worker (``hermes_bridge.py``) and ``state.db`` flush by index — ``_flush_messages_to_session_db`` writes ``messages[_last_flushed_db_idx:]`` so anything not in the structured ``messages`` list silently never reaches disk. A few break paths in ``run_conversation`` set ``final_response`` from already-streamed bytes WITHOUT appending the matching structured ``{"role": "assistant", "content": ...}`` dict: * partial-stream recovery (line ~3543) — stream died mid-flight, the streamed buffer is recovered as the reply. * prior-turn-content fallback (line ~3569) — empty follow-up after housekeeping tools, the earlier turn's text is reused. In both cases the user *saw* the reply in the WebUI (it streamed through the Socket.IO callback) but ``state.db`` ended up with only the user message. The diagnostic in NousResearch#31269 captured this exactly: ``[DBG-BUG2] NOTHING TO FLUSH! flush_from=490 msg_len=490 last_asst_role=assistant`` — the slice was empty because the dict never landed. Add a safety net ``_ensure_final_response_in_messages`` helper called right before the FINAL ``_persist_session`` (after scaffolding cleanup so we don't fight the empty-response sentinel pop). Idempotent on the happy text-response path: when the loop already appended the structured dict (line ~3833), the helper sees a matching tail and no-ops. Whitespace differences between ``final_response`` and the existing tail content are tolerated to avoid spurious double-injection. Skips injection for empty / whitespace-only text and for the ``"(empty)"`` user-facing failure sentinel — those paths have their own persistence semantics that must not be overridden. The injected message carries an ``_injected_from_final_response`` flag so future debugging / analytics can tell apart genuine model dicts from safety-net rebuilds without affecting any existing consumer of the messages list.
…e injection 15 focused tests on ``_ensure_final_response_in_messages`` plus the end-to-end ``_persist_session`` round-trip: * ``TestEnsureFinalResponseInMessages`` — direct unit coverage of the helper's branches: missing tail, partial-stream-recovery shape (tool result tail), happy-path no-op, whitespace-tolerant matching, empty / whitespace / non-string ``final_response``, ``"(empty)"`` sentinel passthrough, ``assistant(tool_calls)`` tail injection, empty messages list, content-disagreement preservation. * ``TestEndToEndBridgePersistence`` — replays the diagnostic from the issue (``_last_flushed_db_idx=1`` covering the user turn, no assistant dict in messages) and asserts the SQLite append actually receives the assistant row after injection. Also asserts the happy path produces exactly one assistant write (no duplication when the conversation loop already appended its own dict).
|
Thanks @xxxigm — clean root-cause writeup, and the two break paths you identified ( The origin issue #31269 was closed NOT_PLANNED, and the competing fix #31291 (same helper, same two paths) was closed as The real bug was at the flush layer, and that's where it was fixed:
Backfilling the recovered fragment into durable history as a complete assistant turn also works against #53987, which intentionally leaves Closing as superseded — the observable persistence failure is covered on current |
What does this PR do?
Fixes #31269: bridge-worker silently drops assistant replies from
state.db.The bridge worker (
hermes_bridge.py) andstate.dbflush by index —_flush_messages_to_session_dbwritesmessages[_last_flushed_db_idx:], so anything not in the structuredmessageslist silently never reaches disk. A few break paths inrun_conversationsetfinal_responsefrom already-streamed bytes WITHOUT appending the matching{"role": "assistant", "content": ...}dict:conversation_loop.py:~3543) — stream died mid-flight, the streamed buffer is recovered as the reply,break.conversation_loop.py:~3569) — empty follow-up after housekeeping tools, the earlier turn's text is reused,break.In both cases the user saw the reply in the WebUI (it streamed through the Socket.IO callback) but
state.dbended up with only the user message. The diagnostic in the issue captured this exactly:— the slice was empty because the dict never landed.
Two small additions wired together:
Safety-net helper (
agent/conversation_loop.py) — adds module-level_ensure_final_response_in_messages(messages, final_response)that appends a structured{"role": "assistant", "content": <text>, "_injected_from_final_response": True}dict when the messages tail doesn't already carry the streamed reply. Idempotent on the happy text-response path (the loop already appended its own dict at line ~3833). Whitespace-tolerant matching avoids spurious double-injection. Skipped forNone/ empty / whitespacefinal_responseand for the"(empty)"user-facing failure sentinel — those have their own persistence semantics. Wired in right before the FINAL_persist_session(after scaffolding cleanup so it doesn't fight the empty-response sentinel pop).Regression tests (
tests/run_agent/test_final_response_injection_31269.py) — 15 focused tests including the bridge-worker diagnostic replay (_last_flushed_db_idx=1covering the user turn, no assistant dict, asserts the SQLite append actually receives the assistant row).Related Issue
Fixes #31269
Type of Change
Changes Made
agent/conversation_loop.py— new_EMPTY_RESPONSE_SENTINELconstant and_ensure_final_response_in_messageshelper; integration call between_drop_trailing_empty_response_scaffoldingand the FINAL_persist_sessionatrun_conversation's tail; debug-level log when injection happened so post-mortems can correlate with the turn's exit reason.tests/run_agent/test_final_response_injection_31269.py— 15 unit + end-to-end regression tests (NEW FILE).Backwards compatible: every existing exit path keeps working, the helper is idempotent, the new
_injected_from_final_responseflag is a transparent debugging marker that no existing consumer reads.How to Test
End-to-end behaviour after the fix (TypeScript WebUI → bridge worker →
state.db):Checklist
fix(agent):,test(agent):)final_responseso empty-response semantics are preserved