Repository navigation
fix(chat): reload fidelity for streamed replies — model/sources, turn order, backfill (#13292, #13293, #13295, #13296) - #13303
Merged
Merged
Conversation
…r streamed replies (#13292, #13295, #13296) - #13292: the completed streamed reply persisted with sources: [] and no model in metadata because that data lived only on the discarded streaming chunks. _build_final_response_entry now threads selected_model/ rag_citations/used_knowledge from the same LLMIterationContext/graph state the caller already has, so the persisted turn carries the model badge and KB citations the user actually saw. - #13295: on tool-using turns the completed reply is chronologically the FIRST thing the user watched stream (prose, then any tool call/output) but was appended LAST in the persisted batch, inverting the reload order. _persist_workflow_messages now inserts it at the front of the batch. Updated the enshrining test (test_reply_appended_after_tool_output -> test_reply_precedes_tool_output_on_reload) which locked in the inversion. - #13296: extracted the duplicated citation-mapping block into a shared _kb_sources_from_citations() helper, reused by both the per-message citation list and the new final-entry sources. - Updated the _FakeManager test double in internal_prompt_filter_test.py (positional/no-arg stubs) to match the new call signature.
…13214 (#13293) Sessions written while the #13214 bug was live have chat:session:* holding user turns with no assistant reply — the completed streamed reply was never persisted. chat:conversation:* (the short-lived {user, assistant} pair cache used for LLM context) was unaffected and still holds the missing text while its TTL has not expired, so it is the backfill source. session_reply_backfill.py is a pure repair path: compute_backfilled_messages is I/O-free and independently tested; repair_session owns the Redis read + chat_mgr load/save seam and is injectable for tests. The CLI entry point requires explicit --session-id arguments (no bulk auto-scan) and is not invoked anywhere in this change — this PR adds the repair logic and its tests only, per #13293, and does not run it against any system. Also extracted _build_workflow_message_batch out of _persist_workflow_messages (Extract Method, continuing #13296) now that the latter has three more keyword arguments to thread through.
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…x, fix backfill double-post, add transcript fallback BLOCKER 1 (#13295): the front-insertion fix assumed the reply is always chronologically first, which only holds for a single-iteration tool turn. The common case is 2+ iterations (prose1, tool_output, prose2-the-answer, collapsed into one string by "\n\n".join(all_llm_responses)), where the combined prose legitimately belongs AFTER the tool output. Reverted to append (matches pre-PR); rewrote _persist_workflow_messages/ _build_persist_batch docstrings to record the finding; restored test_reply_appended_after_tool_output's original assertion with a docstring explaining both cases, and added test_two_iteration_turn_reload_order_matches_live_order as a baseline for the real interleaving fix (needs per-iteration data on a future signature - tracked on #13295, not fixed here). BLOCKER 2: compute_backfilled_messages checked only the immediately-following message slot for an existing reply, so a healthy tool turn persisted as [user, system(terminal_output), assistant] was misjudged damaged and got a duplicate assistant message inserted. Now scans the full turn window (up to the next user message, or end of session) for ANY assistant entry, and inserts a genuinely-missing reply at the end of that window - after tool/ system entries, not immediately after the user message. BLOCKER 3: chat:conversation:* (Redis, 24h TTL) is not "the only remaining source" - every session damaged by #13214 is in the past, so this key has almost certainly expired by the time anyone runs this repair. Added _load_transcript_pairs, a fallback to the durable data/conversation_transcripts/{id}.json file (no TTL, full history - unlike the production loader's last-10 truncation, which exists only to bound LLM context). repair_session now tries Redis first, then the transcript file. Also (non-blocking, requested): - Extracted REDIS_KEY.CHAT_CONVERSATION_PREFIX in autobot_shared/ssot_constants.py as the single source for the chat:conversation: prefix, previously duplicated in chat_workflow/conversation.py, tasks/chat_retention.py, and this module. - session_reply_backfill_test.py's fake chat-history store now subclasses the real MessagesMixin (matching streamed_reply_persistence_test.py) instead of a hand-rolled duck-typed _build_message_dict that silently omitted author_id. - Added an idempotency test (repair_session called twice backfills 0 the second time) at both the pure-logic and end-to-end level. - Documented repair_session as an unlocked read-modify-write - run only against a session with no concurrent writer. - Decomposed graph.py's persist_conversation (extracted _emit_before_response_send_safe/_emit_after_response_send_safe/ _build_workflow_messages_from_state) and manager.py's _persist_workflow_messages (extracted _build_persist_batch) to shrink both toward the project's function-length convention. Revised scope: only #13292 and #13296 are fully delivered by this PR. #13295 (ordering) and #13293 (backfill) remain open per review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
#13214 made the completed streamed reply reach
chat:session:*at all. What it wrote was bare-bones: metadata that existed only on the discarded streaming chunks never made it into the persisted entry. This PR fixes that, and — after review — deliberately does not attempt two of the four issues it started with._build_final_response_entryhardcodedsources: []and never wrotemetadata.model. Those lived only on the dropped streaming chunks (manager.py:456,:632,:673). Fix: threadselected_model/rag_citations/used_knowledgefrom the sameLLMIterationContext(legacy loop) / graphstate(LangGraph path) the caller already holds. Both paths, not one._persist_workflow_messages— extracted the duplicated citation-mapping block into_kb_sources_from_citations(), and the per-message loop into_build_workflow_message_batch().#13295 and #13293 are NOT closed here — review round 2
#13295 (reload order) was originally "fixed" by inserting the reply at the front of the batch. Review showed the premise is false for the common case. The reply is chronologically first only on a single-iteration turn. A tool-using turn normally runs two iterations (
manager.py:2841;_check_continuation_decisionat:2515-2555returns True after a normal tool execution), each appending its own prose (:2864), and every segment is collapsed into one string by"\n\n".join(all_llm_responses)(:3338,graph.py:1315). Live order isprose1 -> tool output -> prose2(the answer). Front-insertion therefore put the final answer before the tool output it was derived from, and left the session ending on asender="system"terminal dump — a user-visible regression on the most common tool path.Reverted to
batch.append(final_entry).test_reply_appended_after_tool_outputkeeps its original assertion, with a docstring recording why front-insertion is wrong. Addedtest_two_iteration_turn_reload_order_matches_live_orderas a documented baseline for the real fix, which needs per-iteration data (all_llm_responses: list[str]plus an iteration marker onworkflow_messages) and is its own change.#13293 (backfill) had two defects, both found by the reviewer running the module:
i+1, but pre-bug(chat): streamed AutoBot replies are never persisted — session history stores user turns only, so every reply vanishes on reload #13214 tool turns persistterminal_outputassender="system"(manager.py:2967-2978), so a turn that did save its reply reads back as[user, system, assistant]and looked damaged — inserting a duplicate reply into a session needing no repair.chat:conversation:{id}, which has a 24h TTL, while every session damaged by bug(chat): streamed AutoBot replies are never persisted — session history stores user turns only, so every reply vanishes on reload #13214 is by definition in the past. It would have reported0and exited cleanly.Both fixed below, but the module has still never run against real data, so it does not clear closure Gate 3. #13293 stays open with a task to run it against real damaged sessions and attach before/after evidence.
What Changed
autobot-backend/chat_workflow/manager.py_kb_sources_from_citations()helper (_persist_workflow_messages is 80 lines — extract the citation-mapping block #13296)._build_final_response_entry()acceptsselected_model/rag_citations/used_knowledge, setsmetadata.modeland KB-derivedsources(bug(chat): reloaded streamed replies lose their KB citations — persisted assistant turn has empty sources #13292)._build_workflow_message_batch()and_build_persist_batch()extracted from_persist_workflow_messages()(_persist_workflow_messages is 80 lines — extract the citation-mapping block #13296)._persist_workflow_messages()gains keyword-onlyselected_model/rag_citations/used_knowledge(defaulted — existing callers unaffected) and appends the completed-reply entry, unchanged from pre-PR order.autobot-backend/chat_workflow/graph.py—persist_conversationthreads the same three values fromstate; decomposed into_emit_before_response_send_safe,_emit_after_response_send_safe,_build_workflow_messages_from_state.autobot-backend/chat_history/session_reply_backfill.py(new, unwired)_find_turn_end()+ turn-window scan: a turn is damaged only when no assistant entry exists before the next user message. Insertion atturn_end, after tool/system entries._load_conversation_pairs(): Redischat:conversation:*first, falling back to_load_transcript_pairs()reading the fulldata/conversation_transcripts/{id}.json(durable, no TTL — unlike production's last-10-truncated loader).autobot_shared/ssot_constants.py— addedREDIS_KEY.CHAT_CONVERSATION_PREFIX;chat_workflow/conversation.pyandtasks/chat_retention.pynow import it, removing a triplicated literal.Verification
After:
chat_workflow/ chat_history/ api/chat_phase2_test.py api/chat_phase4_test.py-> 460 passed.Before (red state): production code reverted via
git checkoutwith test files kept at their fixed versions -> 10 failures, including the double-post reproduction (assert 1 == 0) andTypeError: unexpected keyword argument 'transcript_dir'on every transcript-fallback test. All 10 pass after restoring the fix.The backfill fake store subclasses the real
MessagesMixin, so_build_message_dict/add_messages_batchare production code and only the load/save seam is in-memory. Idempotency is covered both as pure logic and end-to-end.black,isort,flake8clean on every touched file.Not verified: the repair module against a real Redis instance or real filesystem — only
tmp_pathand fake-Redis/fake-store paths were exercised, per the constraint never to run it against a live system. That is precisely why #13293 remains open.Model Used
Sonnet 5
Closes #13292, #13296