fix(agent): persist delivered assistant text on interrupted turns#43962
fix(agent): persist delivered assistant text on interrupted turns#43962konsisumer wants to merge 1 commit into
Conversation
|
Verification review — reviewed the diff for correctness. What I checked:
Verdict: Clean. The fix correctly addresses the data loss scenario where interrupted or partially-recovered streams leave no assistant message in the persisted session. |
|
Verified clean — LGTM Reviewed the diff: What I checked:
Edge case note: The |
|
Heads up that this overlaps in intent with #45277 (which I just opened for #43936) but they fix different layers and are complementary — flagging so a reviewer can consider them together.
They don't conflict: a row this PR appends carries no |
…nt drops (NousResearch#43936) The session-DB flush in `_flush_messages_to_session_db` was position-based (`messages[max(start_idx, _last_flushed_db_idx):]`). It assumes `messages == conversation_history + this turn's new messages`, which breaks two ways, both reproduced live: 1. Overlapping turns on the cached agent corrupt the shared `_last_flushed_db_idx` (it indexes the turn-local `messages` array) → the earlier completed turn flushes an empty slice and its delivered assistant row is never written. 2. `repair_message_sequence` compacts `messages` in place below `len(conversation_history)`, so `start_idx > len(messages)` permanently → self-reinforcing drop loop. The merged NousResearch#44837 fix (NousResearch#45260) clamps `_last_flushed_db_idx` to `len(messages)` but leaves `start_idx = len(conversation_history)` unbounded, so shape 2 is still an empty slice. No index arithmetic survives the two arrays diverging. Replace the positional slice with identity-based dedup: stamp each persisted dict with `_db_persisted`, recognize this turn's `conversation_history` entries by object identity (`id()`) and stamp instead of re-write. A message is written exactly once regardless of how the arrays shift; the NousResearch#860 and NousResearch#31507 guards hold by construction. `_db_persisted` is underscore-prefixed and already stripped from API payloads. The compression split clears the stamps so the surviving messages re-write into the new session row. `_last_flushed_db_idx` is kept updated for legacy readers but no longer decides what is written. Complementary to NousResearch#43962 (backfills delivered text after an interrupt in turn_finalizer.py); this prevents the drop at the flush layer and also covers the non-interrupt repair-shrink path. Regression: tests/run_agent/test_identity_flush.py (5, incl. an explicit test that the NousResearch#44837 clamp still drops where identity flush persists) + test_message_sequence_repair, test_860_dedup, test_compression_persistence all GREEN.
What does this PR do?
Fixes a transcript-persistence gap where Hermes can deliver assistant text to the user before the canonical assistant message is appended to the in-memory transcript. If the turn is then interrupted, or a partial stream is recovered without a final assistant append,
state.dbcould miss text that was already shown to the user. This change makes turn finalization backfill that visible assistant text into the transcript before persistence when it is missing from the current turn.Related Issue
Fixes #43936
Type of Change
Changes Made
agent/turn_finalizer.py: before persisting the turn, detect visible assistant text that was already delivered forpartial_stream_recoveryor an interrupted streamed turn, and append a synthetic assistant transcript row only when the current turn does not already contain that text.tests/run_agent/test_run_agent.py: added regression coverage for recovered partial-stream persistence and interrupt-after-delivery persistence, alongside the existing partial-stream recovery tests.How to Test
TMP_HERMES_HOME=$(mktemp -d /private/tmp/hermes-test-XXXXXX) && HERMES_HOME="$TMP_HERMES_HOME" /opt/homebrew/bin/timeout -k 30 480 pytest tests/run_agent/test_run_agent.py -q -k 'partial_stream_recovery_persists_recovered_assistant_message or interrupted_stream_persists_visible_assistant_text or partial_stream_recovery_uses_streamed_content or partial_stream_recovery_on_empty_stub or partial_stream_recovery_preempts_prior_turn_fallback'.TMP_HERMES_HOME=$(mktemp -d /private/tmp/hermes-test-XXXXXX) && HERMES_HOME="$TMP_HERMES_HOME" /opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' sh.ModuleNotFoundError: No module named 'fastapi'fromtests/hermes_cli/test_dashboard_auth_401_reauth.py.python scripts/check-windows-footguns.py agent/turn_finalizer.py tests/run_agent/test_run_agent.py,ruff check agent/turn_finalizer.py tests/run_agent/test_run_agent.py, andgit diff --check.What platforms tested on
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
ModuleNotFoundError: No module named 'fastapi'while collectingtests/hermes_cli/test_dashboard_auth_401_reauth.py.ruff check,git diff --check, and the Windows footguns scan passed.