Repository navigation
Fix #2524: Chat rewrites the system message from each turn's retrieval and places history a - #2596
Open
syzsunshine219 wants to merge 3 commits into
Open
syzsunshine219 wants to merge 3 commits into
syzsunshine219 wants to merge 3 commits into
Conversation
…sor#2524) Chat used to splice the current turn's retrieval and current minute into the system message and send that first, with chat history after it. The first message therefore changed on every turn, so provider prefix caches could not reuse the identical history that followed. Split the single system blob into (a) a stable system prompt that is byte-identical across turns and (b) a per-turn dynamic context carrying the current time, retrieved memories, and preferences. Place the stable prompt first, then chat history, then the dynamic context, then the user query — so the shared prefix across requests now contains the chat history and the fixed instructions. Applied in ChatHandler.handle_chat_complete / handle_chat_stream / handle_chat_stream_playground / handle_chat_stream_for_business_user, MOSCore.chat, MOS._generate_synthesis_response, and SimpleMemChat.run. Legacy single-string callers (base_prompt with inline {memories}) keep the old shape for backward compatibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix eight issues from Open Code Review on the Issue MemTensor#2524 patch: 1. `MOS._generate_synthesis_response` was still concatenating `synthesis_tail` (per-call sub-Q/A) into the leading system message, which broke the whole point of the stable prefix. Emit the synthesis tail as its own system message after the chat history and dynamic context, keeping `stable_system` byte-identical across turns. 2. `_build_stable_from_cloud_prompt` depended on exact-string `str.replace` calls against the cloud chat templates. Swap in regex-based substitution so small template rewording still works, and raise `ValueError` loudly if any per-turn placeholder would survive (instead of silently leaking the raw `{current_time}` / `{memories}` tokens to the model). 3. `get_cloud_chat_dynamic_context` appended sections that each started with `"\n"` and joined them with `"\n"`, producing double blank lines. Drop the leading `"\n"` from each section and switch the separator to `"\n\n"` so section boundaries render as one blank line. 4. `MOSCore._build_cacheable_system_parts` logged "Unexpected memory type." and then still appended the non-string object's `__repr__` as a memory entry. Skip the bad entry with `continue` so corrupted data cannot reach the LLM. 5. `_build_enhance_stable_prompt` was passing a long literal sentence ("see the context message immediately before…") as the `{date}` value, so the stable prompt rendered "Current Date: see the context message immediately before…". Use a short, neutral placeholder ("(see dynamic context)"); the live timestamp is still emitted by `_build_enhance_dynamic_context` in the per-turn message. 7. `_build_enhance_dynamic_context` had the same leading-`"\n"` + `"\n"`-join issue as the cloud dynamic context helper. Same fix: strip leading newlines from parts, join with `"\n\n"`. 8. The business-chat log of `current_system_prompt` was moved out of the `if chat_req.need_search` branch, so it fired on every request regardless of retrieval. Move it back inside the retrieval branch to stop flooding the logs for no-search requests. 10. `_build_dynamic_context_message` guarded the early `None` return with `not memories_text and not pref_text and not include_time`. Because `include_time` defaults to `True`, the guard never fired on the default call path — a timestamp-only system message was always injected even when there was nothing useful to say. Drop the `include_time` check so the function honours its `Optional` return type and only emits a dynamic context message when there is memory or preference content. Deliberately not changing OCR findings 6 and 9 (mid-conversation `{"role": "system"}` messages). OpenAI's Chat Completions API accepts multiple system messages at any position — a mid-conversation system message does not raise 400 as the OCR claims. The existing test suite also pins `messages[-2]["role"] == "system"` as the intended shape (see `tests/api/test_chat_message_order.py` and `tests/mem_os/test_memos_core.py`). Changing the role to "user" with a marker would blur the semantic boundary and require rewriting those tests, which exceeds the fix-only scope. Verified with `pytest tests/api/test_chat_message_order.py tests/mem_os/test_memos_core.py` — all 34 tests pass. Pre-existing env failures in `tests/memories/activation/test_kv.py`, `tests/parsers/test_markitdown.py`, `tests/mem_reader/*`, and `tests/vec_dbs/test_qdrant.py` are unrelated to this diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ensor#2524) Three findings from the second automatic OCR pass on PR MemTensor#2529: * Finding MemTensor#6 (chat_handler.py L623-L628 + L1253) and finding MemTensor#9 (mem_chat/simple.py L95) — a mid-conversation ``{"role": "system"}`` message is ambiguous for several provider APIs (Azure OpenAI, some Anthropic routers, several OpenAI-compatible proxies) and can be silently merged or rejected. Round 1 kept the system role on the argument that OpenAI's spec permits it; OCR re-flagged the pattern as not converging. Switch the per-turn dynamic context to a ``{"role": "user"}`` message with an explicit "[Context for this turn]" header so the model still distinguishes it from the actual user question. Also applied to the matching patterns in ``_assemble_cacheable_messages`` and ``mos_core.chat`` for consistency (the latter is directly pinned by the test suite). * Finding MemTensor#5 (chat_handler.py L1272-L1278) — round 1 passed ``date="(see dynamic context)"`` into ``get_memos_prompt``, which still renders the literal line ``# Current Date: (see dynamic context)``. Switch to ``"N/A — see context below"``, a clearly synthetic sentinel that is unlikely to be parsed as a real date. Tests updated: * tests/api/test_chat_message_order.py — assert ``user`` role + "[Context for this turn]" marker. * tests/mem_os/test_memos_core.py — same, in both the custom base-prompt case and the chat-history-order case. Verification: ``pytest tests/api tests/mem_chat tests/mem_os/test_memos_core.py`` (all pass, 164 tests). ``ruff check`` / ``ruff format --check`` clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
syzsunshine219
marked this pull request as ready for review
November 9, 2026 15:48
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
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.
Restores the change proposed in #2529 for review against
mainafter its former base branch,dev-v2.0.36, was deleted. The source commits and authors are preserved.Source head:
6c654fc9a1bee1d2eb827e7f098848101f8d7982This PR is open for review. Review and CI against the current base are pending; validation claims in the original description below refer to the earlier snapshot. The original discussion and evidence remain in #2529.
Original change description
Description
Fix issue #2524: chat prompt rewrite busted provider prefix caches. The handler used to splice the current turn's retrieval and current minute into the system message and send that first, with chat history after it — so the first message changed every turn and the identical history behind it could not be reused.
Split the system blob into a stable prompt (byte-identical across turns in one conversation) plus a per-turn dynamic context message carrying the current time, retrieved memories, and preferences. New message layout is
[stable system, ...history, dynamic context, user], so the shared cross-turn prefix now grows with the chat history. Applied inChatHandler.handle_chat_complete,handle_chat_stream,handle_chat_stream_playground,handle_chat_stream_for_business_user,MOSCore.chat,MOS._generate_synthesis_response, andSimpleMemChat.run. Legacy callers that pass a custombase_promptcontaining{memories}keep the single-system shape for backward compatibility.Tests: added
tests/api/test_chat_message_order.py(9 cases covering stable/dynamic split, cacheable assembly, cross-turn shared prefix, and legacy-base-prompt compat), updatedtests/mem_os/test_memos_core.py::TestMOSChat::test_chat_with_custom_base_promptfor the new ordering, and addedtest_chat_places_history_before_dynamic_context_issue_2524. Full targeted runpytest tests/mem_os/ tests/api/ tests/mem_chat/is green: 167 passed. Ruff lint + format clean on all modified files. No API schema change (nomake openapineeded).Related Issue (Required): Fixes #2524
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@WeiminLeeplease review this PR.Reviewer Checklist