fix(session): preserve history during idle compaction (#5167)

nanobot/agent/memory.py

@@ -807,7 +807,7 @@ _HISTORY_ENTRY_HARD_CAP = 64_000      # emergency cap in append_history
 
 
 class Consolidator:
-    """Lightweight consolidation: summarizes evicted messages into history.jsonl."""
+    """Summarize compacted messages into history.jsonl."""
 
     _MAX_CONSOLIDATION_ROUNDS = 5
 
@@ -998,14 +998,9 @@ class Consolidator:
         session_key: str | None = None,
         summary_messages: list[dict[str, Any]] | None = None,
     ) -> str | None:
-        """Summarize messages via LLM and append to history.jsonl.
+        """Summarize messages and append the result 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_messages``, when given, lets callers include retained
-        messages in the summary without archiving them.
-
-        Returns the summary text on success, None if nothing to archive.
+        ``summary_messages`` adds context but is excluded from raw fallback.
         """
         if not messages:
             return None
@@ -1166,13 +1161,7 @@ class Consolidator:
         runtime: LLMRuntime,
         max_suffix: int = 8,
     ) -> str | None:
-        """Hard-truncate an idle session under the consolidation lock.
-
-        Used by AutoCompact so all session mutation goes through a single
-        lock-protected path.  Returns the summary text on success, ``None``
-        if the LLM failed (raw_archive fallback), or ``""`` if there was
-        nothing to archive.
-        """
+        """Archive an idle prefix and hide it from replay without deleting it."""
         lock = self.get_lock(session_key)
         async with lock:
             self.sessions.invalidate(session_key)
@@ -1192,24 +1181,21 @@ class Consolidator:
                 last_consolidated=0,
             )
             result = probe.retain_recent_legal_suffix(max_suffix, extend_to_user=True)
-            messages_to_keep = probe.messages
-            messages_to_remove = result.dropped[result.already_consolidated_count:]
+            visible_suffix = probe.messages
+            messages_to_remove = result.dropped
 
-            if not messages_to_remove and not messages_to_keep:
+            if not messages_to_remove:
                 self.sessions.save(session)
                 return ""
 
             last_active = session.updated_at
-            summary: str | None = ""
-            if messages_to_remove:
-                # Summarize the retained suffix too, but only remove/raw-dump
-                # the messages that are no longer kept in the live session.
-                summary = await self.archive(
-                    messages_to_remove,
-                    runtime=runtime,
-                    session_key=session_key,
-                    summary_messages=messages_to_summarize,
-                )
+            # The visible suffix informs the summary but stays out of raw fallback.
+            summary = await self.archive(
+                messages_to_remove,
+                runtime=runtime,
+                session_key=session_key,
+                summary_messages=messages_to_summarize,
+            )
 
             if summary and summary != "(nothing)":
                 session.metadata["_last_summary"] = {
@@ -1217,17 +1203,17 @@ class Consolidator:
                     "last_active": last_active.isoformat(),
                 }
 
-            session.messages = messages_to_keep
-            session.last_consolidated = 0
+            # Preserve history and advance only the replay boundary.
+            session.last_consolidated = len(session.messages) - len(visible_suffix)
             self.sessions.save(session)
 
-            if messages_to_remove:
-                logger.info(
-                    "Idle-session compact for {}: archived={}, kept={}, summary={}",
-                    session_key,
-                    len(messages_to_remove),
-                    len(messages_to_keep),
-                    bool(summary),
-                )
+            logger.info(
+                "Idle-session compact for {}: archived={}, visible={}, retained={}, summary={}",
+                session_key,
+                len(messages_to_remove),
+                len(visible_suffix),
+                len(session.messages),
+                bool(summary),
+            )
 
             return summary

tests/agent/test_auto_compact.py

@@ -80,7 +80,6 @@ def _make_fake_compact(
     track_archived: list | None = None,
     track_count: bool = False,
 ):
-    """Return a fake compact_idle_session that mirrors the real method's session mutation."""
     from nanobot.session.manager import Session as _Session
 
     state = {"count": 0}
@@ -106,21 +105,20 @@ def _make_fake_compact(
             max_suffix,
             extend_to_user=True,
         )
-        kept = probe.messages
-        archive_msgs = result.dropped[result.already_consolidated_count:]
+        visible_suffix = probe.messages
+        archive_msgs = result.dropped
 
-        if not archive_msgs and not kept:
+        if not archive_msgs:
             loop.sessions.save(session)
             return ""
 
         last_active = session.updated_at
         s = summary
-        if archive_msgs:
-            if on_archive:
-                result = on_archive(archive_msgs)
-                s = result if isinstance(result, str) else summary
-            if track_archived is not None:
-                track_archived.extend(archive_msgs)
+        if on_archive:
+            result = on_archive(archive_msgs)
+            s = result if isinstance(result, str) else summary
+        if track_archived is not None:
+            track_archived.extend(archive_msgs)
 
         if s and s != "(nothing)":
             session.metadata["_last_summary"] = {
@@ -128,8 +126,7 @@ def _make_fake_compact(
                 "last_active": last_active.isoformat(),
             }
 
-        session.messages = kept
-        session.last_consolidated = 0
+        session.last_consolidated = len(session.messages) - len(visible_suffix)
         loop.sessions.save(session)
         return s
 
@@ -368,8 +365,7 @@ class TestAutoCompact:
         await loop.close_mcp()
 
     @pytest.mark.asyncio
-    async def test_auto_compact_archives_prefix_and_keeps_recent_suffix(self, tmp_path):
-        """_archive should summarize the old prefix and keep a recent legal suffix."""
+    async def test_auto_compact_archives_prefix_without_deleting_history(self, tmp_path):
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         _add_turns(session, 6)
@@ -384,9 +380,12 @@ class TestAutoCompact:
 
         assert len(archived_messages) == 4
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
-        assert session_after.messages[0]["content"] == "msg user 2"
-        assert session_after.messages[-1]["content"] == "msg assistant 5"
+        assert len(session_after.messages) == 12
+        assert session_after.messages[0]["content"] == "msg user 0"
+        visible = session_after.get_history(max_messages=12)
+        assert len(visible) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert visible[0]["content"] == "msg user 2"
+        assert visible[-1]["content"] == "msg assistant 5"
         await loop.close_mcp()
 
     @pytest.mark.asyncio
@@ -403,17 +402,19 @@ class TestAutoCompact:
         await loop.auto_compact._archive("cli:test", runtime=loop.llm_runtime())
 
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) > loop.auto_compact._RECENT_SUFFIX_MESSAGES
-        assert session_after.messages[0]["content"] == "record this"
-        assert session_after.messages[-1]["content"] == "done"
+        assert session_after.messages[0]["content"] == "old user 0"
+        visible = session_after.get_history(max_messages=len(session_after.messages))
+        assert len(visible) > loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert visible[0]["content"] == "record this"
+        assert visible[-1]["content"] == "done"
         tool_results = {
             m.get("tool_call_id")
-            for m in session_after.messages
+            for m in visible
             if m.get("role") == "tool"
         }
         assert all(
             tc["id"] in tool_results
-            for m in session_after.messages
+            for m in visible
             for tc in (m.get("tool_calls") or [])
         )
         await loop.close_mcp()
@@ -436,7 +437,10 @@ class TestAutoCompact:
         assert entry is not None
         assert entry[0] == "User said hello."
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(session_after.messages) == 12
+        assert len(session_after.get_history(max_messages=12)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
         await loop.close_mcp()
 
     @pytest.mark.asyncio
@@ -474,11 +478,10 @@ class TestAutoCompact:
 
 
 class TestAutoCompactIdleDetection:
-    """Test idle detection triggers auto-new in _process_message."""
+    """Idle detection tests."""
 
     @pytest.mark.asyncio
     async def test_no_auto_compact_when_ttl_disabled(self, tmp_path):
-        """No auto-new should happen when TTL is 0 (disabled)."""
         loop = _make_loop(tmp_path, session_ttl_minutes=0)
         session = loop.sessions.get_or_create("cli:test")
         session.add_message("user", "old message")
@@ -494,7 +497,6 @@ class TestAutoCompactIdleDetection:
 
     @pytest.mark.asyncio
     async def test_auto_compact_triggers_on_idle(self, tmp_path):
-        """Proactive auto-new archives expired session; _process_message reloads it."""
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         _add_turns(session, 6, prefix="old")
@@ -514,13 +516,16 @@ class TestAutoCompactIdleDetection:
 
         session_after = loop.sessions.get_or_create("cli:test")
         assert len(archived_messages) == 4
-        assert not any(m["content"] == "old user 0" for m in session_after.messages)
+        assert any(m["content"] == "old user 0" for m in session_after.messages)
+        assert not any(
+            m["content"] == "old user 0"
+            for m in session_after.get_history(max_messages=len(session_after.messages))
+        )
         assert any(m["content"] == "new msg" for m in session_after.messages)
         await loop.close_mcp()
 
     @pytest.mark.asyncio
     async def test_no_auto_compact_when_active(self, tmp_path):
-        """No auto-new should happen when session is recently active."""
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         session.add_message("user", "recent message")
@@ -558,7 +563,6 @@ class TestAutoCompactIdleDetection:
 
     @pytest.mark.asyncio
     async def test_auto_compact_with_slash_new(self, tmp_path):
-        """Auto-new fires before /new dispatches; session is cleared twice but idempotent."""
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         for i in range(4):
@@ -576,7 +580,6 @@ class TestAutoCompactIdleDetection:
         assert "new session started" in response.content.lower()
 
         session_after = loop.sessions.get_or_create("cli:test")
-        # Session is empty (auto-new archived and cleared, /new cleared again)
         assert len(session_after.messages) == 0
         await loop.close_mcp()
 
@@ -617,11 +620,10 @@ class TestAutoCompactIdleDetection:
 
 
 class TestAutoCompactSystemMessages:
-    """Test that auto-new also works for system messages."""
+    """System-message idle compaction tests."""
 
     @pytest.mark.asyncio
     async def test_auto_compact_triggers_for_system_messages(self, tmp_path):
-        """Proactive auto-new archives expired session; system messages reload it."""
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         _add_turns(session, 6, prefix="old")
@@ -640,9 +642,10 @@ class TestAutoCompactSystemMessages:
         await loop._process_message(msg)
 
         session_after = loop.sessions.get_or_create("cli:test")
+        assert any(m["content"] == "old user 0" for m in session_after.messages)
         assert not any(
             m["content"] == "old user 0"
-            for m in session_after.messages
+            for m in session_after.get_history(max_messages=len(session_after.messages))
         )
         await loop.close_mcp()
 
@@ -652,7 +655,6 @@ class TestAutoCompactEdgeCases:
 
     @pytest.mark.asyncio
     async def test_auto_compact_with_nothing_summary(self, tmp_path):
-        """Auto-new should not inject when archive produces '(nothing)'."""
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         _add_turns(session, 6, prefix="thanks")
@@ -666,15 +668,17 @@ class TestAutoCompactEdgeCases:
         await loop.auto_compact._archive("cli:test", runtime=loop.llm_runtime())
 
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(session_after.messages) == 12
+        assert len(session_after.get_history(max_messages=12)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
         # "(nothing)" summary should not be stored
         assert "cli:test" not in loop.auto_compact._summaries
 
         await loop.close_mcp()
 
     @pytest.mark.asyncio
-    async def test_auto_compact_archive_failure_still_keeps_recent_suffix(self, tmp_path):
-        """Auto-new should keep the recent suffix even if LLM archive falls back to raw dump."""
+    async def test_auto_compact_archive_failure_preserves_raw_history(self, tmp_path):
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
         _add_turns(session, 6, prefix="important")
@@ -687,7 +691,10 @@ class TestAutoCompactEdgeCases:
         await loop.auto_compact._archive("cli:test", runtime=loop.llm_runtime())
 
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(session_after.messages) == 12
+        assert len(session_after.get_history(max_messages=12)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
 
         await loop.close_mcp()
 
@@ -725,13 +732,10 @@ class TestAutoCompactEdgeCases:
 
 
 class TestAutoCompactIntegration:
-    """End-to-end test of auto session new feature."""
+    """Idle compaction integration tests."""
 
     @pytest.mark.asyncio
     async def test_full_lifecycle(self, tmp_path):
-        """
-        Full lifecycle: messages -> idle -> auto-new -> archive -> clear -> summary injected as runtime context.
-        """
         loop = _make_loop(tmp_path, session_ttl_minutes=15)
         session = loop.sessions.get_or_create("cli:test")
 
@@ -759,6 +763,7 @@ class TestAutoCompactIntegration:
                 tool_calls=[],
             )
         )
+        await loop.auto_compact._archive("cli:test", runtime=loop.llm_runtime())
 
         msg = InboundMessage(
             channel="cli", sender_id="user", chat_id="test",
@@ -769,9 +774,13 @@ class TestAutoCompactIntegration:
         # Phase 4: Verify
         session_after = loop.sessions.get_or_create("cli:test")
 
-        # The oldest messages should be trimmed from live session history
+        assert any(
+            "past tense is used" in str(m.get("content", "")).lower()
+            for m in session_after.messages
+        )
         assert not any(
-            "past tense is used" in str(m.get("content", "")) for m in session_after.messages
+            "past tense is used" in str(m.get("content", "")).lower()
+            for m in session_after.get_history(max_messages=len(session_after.messages))
         )
 
         # Summary should NOT be persisted in session (ephemeral, one-shot)
@@ -821,7 +830,7 @@ class TestAutoCompactIntegration:
 
 
 class TestProactiveAutoCompact:
-    """Test proactive auto-new on idle ticks (TimeoutError path in run loop)."""
+    """Proactive idle compaction tests."""
 
     @staticmethod
     async def _run_check_expired(loop, active_session_keys=()):
@@ -899,7 +908,10 @@ class TestProactiveAutoCompact:
         await self._run_check_expired(loop)
 
         session_after = loop.sessions.get_or_create("cli:test")
-        assert len(session_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(session_after.messages) == 10
+        assert len(session_after.get_history(max_messages=10)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
         assert len(archived_messages) == 2
         entry = loop.auto_compact._summaries.get("cli:test")
         assert entry is not None
@@ -1082,7 +1094,10 @@ class TestProactiveAutoCompact:
 
         assert _fake_compact.state["count"] == 1
         s1_after = loop.sessions.get_or_create("cli:expired_idle")
-        assert len(s1_after.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(s1_after.messages) == 12
+        assert len(s1_after.get_history(max_messages=12)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
         s2_after = loop.sessions.get_or_create("cli:expired_active")
         assert len(s2_after.messages) == 12  # Preserved
         s3_after = loop.sessions.get_or_create("cli:recent")
@@ -1211,7 +1226,10 @@ class TestSummaryPersistence:
 
         # prepare_session should recover summary from metadata
         reloaded = loop.sessions.get_or_create("cli:test")
-        assert len(reloaded.messages) == loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        assert len(reloaded.messages) == 12
+        assert len(reloaded.get_history(max_messages=12)) == (
+            loop.auto_compact._RECENT_SUFFIX_MESSAGES
+        )
         _, summary = loop.auto_compact.prepare_session(reloaded, "cli:test")
 
         assert summary is not None

tests/agent/test_consolidator.py

@@ -586,7 +586,7 @@ class TestConsolidatorTokenBudget:
 
 
 class TestCompactIdleSession:
-    """Tests for Consolidator.compact_idle_session — lock-protected idle truncation."""
+    """Idle compaction tests."""
 
     @pytest.fixture
     def real_consolidator(self, store, mock_provider):
@@ -602,11 +602,9 @@ class TestCompactIdleSession:
         )
 
     @pytest.mark.asyncio
-    async def test_archives_prefix_keeps_suffix(
+    async def test_archives_prefix_preserves_messages_and_hides_prefix(
         self, real_consolidator, mock_provider, runtime
     ):
-        """20 user/assistant turns → compact with max_suffix=8 → messages ≤ 8,
-        last_consolidated=0, _last_summary stored."""
         mock_provider.chat_with_retry.return_value = MagicMock(
             content="Summary of old conversation.", finish_reason="stop"
         )
@@ -624,9 +622,15 @@ class TestCompactIdleSession:
         )
         assert result == "Summary of old conversation."
 
+        sessions.invalidate("cli:test")
         reloaded = sessions.get_or_create("cli:test")
-        assert len(reloaded.messages) <= 8
-        assert reloaded.last_consolidated == 0
+        assert len(reloaded.messages) == 40
+        assert reloaded.messages[0]["content"] == "user msg 0"
+        assert reloaded.last_consolidated == 32
+        visible = reloaded.get_history(max_messages=40)
+        assert len(visible) == 8
+        assert visible[0]["content"] == "user msg 16"
+        assert visible[-1]["content"] == "assistant msg 19"
         meta = reloaded.metadata.get("_last_summary")
         assert meta is not None
         assert meta["text"] == "Summary of old conversation."
@@ -665,9 +669,7 @@ class TestCompactIdleSession:
     async def test_raw_dumps_only_dropped_messages_on_llm_failure(
         self, real_consolidator, mock_provider, store, runtime
     ):
-        """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."""
+        """Extra summary context must not enter raw fallback. Regression for #4264."""
         mock_provider.chat_with_retry.side_effect = RuntimeError("LLM unavailable")
         sessions = real_consolidator.sessions
         session = sessions.get_or_create("cli:rawdrop")
@@ -684,8 +686,11 @@ class TestCompactIdleSession:
 
         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
+        assert "user msg 0" in raw
+        assert "RETAINED_SUFFIX_marker" not in raw
+        reloaded = sessions.get_or_create("cli:rawdrop")
+        assert len(reloaded.messages) == 38
+        assert reloaded.messages[-1]["content"] == "RETAINED_SUFFIX_marker"
 
     @pytest.mark.asyncio
     async def test_idle_compact_writes_session_key_to_history(
@@ -757,10 +762,9 @@ class TestCompactIdleSession:
         assert "_last_summary" not in reloaded.metadata
 
     @pytest.mark.asyncio
-    async def test_llm_failure_still_truncates(
+    async def test_llm_failure_preserves_history_but_advances_replay_boundary(
         self, real_consolidator, mock_provider, store, runtime
     ):
-        """LLM raises RuntimeError → raw_archive fires, session still truncated, returns None."""
         mock_provider.chat_with_retry.side_effect = RuntimeError("LLM unavailable")
         sessions = real_consolidator.sessions
         session = sessions.get_or_create("cli:fail")
@@ -778,9 +782,16 @@ class TestCompactIdleSession:
         entries = store.read_unprocessed_history(since_cursor=0)
         assert any("[RAW]" in e["content"] for e in entries)
 
-        # Session should still be truncated
         reloaded = sessions.get_or_create("cli:fail")
-        assert len(reloaded.messages) <= 4
+        assert len(reloaded.messages) == 20
+        assert reloaded.messages[0]["content"] == "u0"
+        assert reloaded.last_consolidated == 16
+        assert [m["content"] for m in reloaded.get_history(max_messages=20)] == [
+            "u8",
+            "a8",
+            "u9",
+            "a9",
+        ]
 
     @pytest.mark.asyncio
     async def test_respects_last_consolidated(
@@ -802,6 +813,9 @@ class TestCompactIdleSession:
             "cli:offset", runtime=runtime, max_suffix=4
         )
         assert result == "Tail summary."
+        reloaded = sessions.get_or_create("cli:offset")
+        assert len(reloaded.messages) == 60
+        assert reloaded.last_consolidated == 56
 
         # Verify only the unconsolidated tail was processed:
         # 10 unconsolidated messages (50-59), keep suffix of 4 → archive 6
@@ -812,14 +826,12 @@ class TestCompactIdleSession:
         assert "u25" in user_content or "a25" in user_content
 
     @pytest.mark.asyncio
-    async def test_non_contiguous_suffix_archives_actual_dropped_messages(
+    async def test_extended_suffix_archives_only_hidden_prefix(
         self,
         real_consolidator,
         mock_provider,
         runtime,
     ):
-        """Assistant-only tails extend back to the latest user turn, so archive
-        the actual dropped messages rather than a computed prefix."""
         mock_provider.chat_with_retry.return_value = MagicMock(
             content="Tail summary.", finish_reason="stop"
         )
@@ -837,7 +849,9 @@ class TestCompactIdleSession:
         assert result == "Tail summary."
 
         reloaded = sessions.get_or_create("cli:noncontiguous")
-        assert [m["content"] for m in reloaded.messages] == [
+        assert len(reloaded.messages) == 25
+        assert reloaded.last_consolidated == 14
+        assert [m["content"] for m in reloaded.get_history(max_messages=25)] == [
             "user-14",
             "assistant-00",
             "assistant-01",
@@ -987,23 +1001,21 @@ class TestConsolidatorSessionRefresh:
         # Simulate: background consolidation captures old reference
         old_ref = session
 
-        # AutoCompact runs first and truncates to 8
         await consolidator.compact_idle_session(
             "cli:test",
             runtime=runtime,
             max_suffix=8,
         )
 
-        # Background consolidation runs with stale reference —
-        # should detect the session was replaced and not undo the compact.
         await consolidator.maybe_consolidate_by_tokens(
             old_ref,
             runtime=runtime,
         )
 
         session_after = sessions.get_or_create("cli:test")
-        # Messages should still be truncated (not restored to 40)
-        assert len(session_after.messages) <= 8
+        assert len(session_after.messages) == 40
+        assert session_after.last_consolidated == 32
+        assert len(session_after.get_history(max_messages=40)) == 8
 
 
 class TestRawArchiveTruncation: