fix(memory): summarize full session tail during idle compaction (#4264)
Idle compaction summarized only the dropped prefix, excluding the recent suffix it retains. On a finished conversation a late user correction or final result lands in that kept suffix, so it never reached the persisted summary and history kept the stale pre-correction conclusion — which, for idle sessions that are rarely resumed, is never fixed. Summarize over the full unconsolidated tail instead, while still removing (and raw-dumping on LLM failure) only the dropped messages. Adds an opt-in summary_context argument to Consolidator.archive so the summarization window and the archived set can differ without affecting other callers.
This commit is contained in:
committed by
Xubin Ren
parent
04ed7554c0
commit
0863e6e5ab
+20
-2
@@ -799,15 +799,23 @@ class Consolidator:
|
||||
messages: list[dict],
|
||||
*,
|
||||
session_key: str | None = None,
|
||||
summary_context: list[dict] | None = None,
|
||||
) -> str | None:
|
||||
"""Summarize messages via LLM and append to history.jsonl.
|
||||
|
||||
``messages`` are the messages being archived (removed from the live
|
||||
session); they are what gets raw-dumped if the LLM call fails.
|
||||
``summary_context``, when given, is fed to the summarizer in place of
|
||||
``messages`` so a caller can summarize over a wider window (e.g. the
|
||||
full conversation tail) while still only archiving ``messages``.
|
||||
|
||||
Returns the summary text on success, None if nothing to archive.
|
||||
"""
|
||||
if not messages:
|
||||
return None
|
||||
context = summary_context if summary_context is not None else messages
|
||||
try:
|
||||
formatted = MemoryStore._format_messages(messages)
|
||||
formatted = MemoryStore._format_messages(context)
|
||||
formatted = self._truncate_to_token_budget(formatted)
|
||||
response = await self.provider.chat_with_retry(
|
||||
model=self.model,
|
||||
@@ -990,7 +998,17 @@ class Consolidator:
|
||||
last_active = session.updated_at
|
||||
summary: str | None = ""
|
||||
if archive_msgs:
|
||||
summary = await self.archive(archive_msgs, session_key=session_key)
|
||||
# Summarize over the full unconsolidated tail — including the
|
||||
# recent suffix we retain — not just the dropped prefix. Idle
|
||||
# compaction usually runs on a finished conversation, so a late
|
||||
# user correction or final result that landed in the kept suffix
|
||||
# must still reach the persisted summary; otherwise history keeps
|
||||
# the stale pre-correction conclusion that never gets fixed
|
||||
# (#4264). Only archive_msgs are removed/raw-dumped; kept stays
|
||||
# in the session.
|
||||
summary = await self.archive(
|
||||
archive_msgs, session_key=session_key, summary_context=tail
|
||||
)
|
||||
|
||||
if summary and summary != "(nothing)":
|
||||
session.metadata["_last_summary"] = {
|
||||
|
||||
@@ -401,6 +401,56 @@ class TestCompactIdleSession:
|
||||
assert meta["text"] == "Summary of old conversation."
|
||||
assert "last_active" in meta
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_summarizes_retained_suffix_not_just_dropped_prefix(
|
||||
self, real_consolidator, mock_provider
|
||||
):
|
||||
"""idleCompact must summarize over the full unconsolidated tail, including
|
||||
the recent suffix it retains. Otherwise a late user correction / final
|
||||
result that lands in the kept suffix is excluded from the persisted
|
||||
summary, leaving a stale wrong conclusion in history. Regression for #4264."""
|
||||
mock_provider.chat_with_retry.return_value = MagicMock(
|
||||
content="Summary.", finish_reason="stop"
|
||||
)
|
||||
sessions = real_consolidator.sessions
|
||||
session = sessions.get_or_create("cli:correction")
|
||||
for i in range(18):
|
||||
session.add_message("user", f"user msg {i}")
|
||||
session.add_message("assistant", f"assistant msg {i}")
|
||||
# Final correction exchange lands inside the retained max_suffix window.
|
||||
session.add_message("user", "no, that's wrong, use approach B")
|
||||
session.add_message("assistant", "CORRECTED_FINAL_RESULT_alpha")
|
||||
sessions.save(session)
|
||||
|
||||
await real_consolidator.compact_idle_session("cli:correction", max_suffix=8)
|
||||
|
||||
summarized = mock_provider.chat_with_retry.call_args.kwargs["messages"][1]["content"]
|
||||
assert "CORRECTED_FINAL_RESULT_alpha" in summarized
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_raw_dumps_only_dropped_messages_on_llm_failure(
|
||||
self, real_consolidator, mock_provider, store
|
||||
):
|
||||
"""Summarizing over the full tail must not widen what gets raw-dumped on
|
||||
LLM failure: the breadcrumb should contain only the removed prefix, not
|
||||
the retained suffix that stays live in the session. Regression for #4264."""
|
||||
mock_provider.chat_with_retry.side_effect = RuntimeError("LLM unavailable")
|
||||
sessions = real_consolidator.sessions
|
||||
session = sessions.get_or_create("cli:rawdrop")
|
||||
for i in range(18):
|
||||
session.add_message("user", f"user msg {i}")
|
||||
session.add_message("assistant", f"assistant msg {i}")
|
||||
session.add_message("user", "final user follow-up")
|
||||
session.add_message("assistant", "RETAINED_SUFFIX_marker")
|
||||
sessions.save(session)
|
||||
|
||||
await real_consolidator.compact_idle_session("cli:rawdrop", max_suffix=8)
|
||||
|
||||
raw = "\n".join(e["content"] for e in store.read_unprocessed_history(since_cursor=0))
|
||||
assert "[RAW]" in raw
|
||||
assert "user msg 0" in raw # removed prefix is the breadcrumb
|
||||
assert "RETAINED_SUFFIX_marker" not in raw # retained suffix not dumped
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_idle_compact_writes_session_key_to_history(
|
||||
self,
|
||||
@@ -537,11 +587,15 @@ class TestCompactIdleSession:
|
||||
"assistant-04",
|
||||
]
|
||||
|
||||
# #4264: idle compaction now summarizes the full unconsolidated tail, so
|
||||
# the dropped head (user-00), the non-contiguous dropped tail
|
||||
# (assistant-09), and the retained suffix (user-14) are all summarized.
|
||||
# Retention above still proves the non-contiguous suffix is handled.
|
||||
archived_call = mock_provider.chat_with_retry.call_args
|
||||
user_content = archived_call.kwargs["messages"][1]["content"]
|
||||
assert "user-14" not in user_content
|
||||
assert "assistant-00" not in user_content
|
||||
assert "user-00" in user_content
|
||||
assert "assistant-09" in user_content
|
||||
assert "user-14" in user_content
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_acquires_consolidation_lock(self, real_consolidator, mock_provider):
|
||||
|
||||
Reference in New Issue
Block a user