From b24d6ffc941f7ff755898fa94485bab51e4415d4 Mon Sep 17 00:00:00 2001 From: shenchengtsi Date: Tue, 10 Mar 2026 11:32:11 +0800 Subject: [PATCH 1/2] fix(memory): validate save_memory payload before persisting --- nanobot/agent/memory.py | 33 ++++++--- tests/test_memory_consolidation_types.py | 94 +++++++++++++++++++++++- 2 files changed, 116 insertions(+), 11 deletions(-) diff --git a/nanobot/agent/memory.py b/nanobot/agent/memory.py index 21fe77d..add014b 100644 --- a/nanobot/agent/memory.py +++ b/nanobot/agent/memory.py @@ -139,15 +139,30 @@ class MemoryStore: logger.warning("Memory consolidation: unexpected arguments type {}", type(args).__name__) return False - if entry := args.get("history_entry"): - if not isinstance(entry, str): - entry = json.dumps(entry, ensure_ascii=False) - self.append_history(entry) - if update := args.get("memory_update"): - if not isinstance(update, str): - update = json.dumps(update, ensure_ascii=False) - if update != current_memory: - self.write_long_term(update) + if "history_entry" not in args or "memory_update" not in args: + logger.warning("Memory consolidation: save_memory payload missing required fields") + return False + + entry = args["history_entry"] + update = args["memory_update"] + + if entry is None or update is None: + logger.warning("Memory consolidation: save_memory payload contains null required fields") + return False + + if not isinstance(entry, str): + entry = json.dumps(entry, ensure_ascii=False) + if not isinstance(update, str): + update = json.dumps(update, ensure_ascii=False) + + entry = entry.strip() + if not entry: + logger.warning("Memory consolidation: history_entry is empty after normalization") + return False + + self.append_history(entry) + if update != current_memory: + self.write_long_term(update) session.last_consolidated = 0 if archive_all else len(session.messages) - keep_count logger.info("Memory consolidation done: {} messages, last_consolidated={}", len(session.messages), session.last_consolidated) diff --git a/tests/test_memory_consolidation_types.py b/tests/test_memory_consolidation_types.py index ff15584..4ba1ecd 100644 --- a/tests/test_memory_consolidation_types.py +++ b/tests/test_memory_consolidation_types.py @@ -97,7 +97,6 @@ class TestMemoryConsolidationTypeHandling: store = MemoryStore(tmp_path) provider = AsyncMock() - # Simulate arguments being a JSON string (not yet parsed) response = LLMResponse( content=None, tool_calls=[ @@ -152,7 +151,6 @@ class TestMemoryConsolidationTypeHandling: store = MemoryStore(tmp_path) provider = AsyncMock() - # Simulate arguments being a list containing a dict response = LLMResponse( content=None, tool_calls=[ @@ -220,3 +218,95 @@ class TestMemoryConsolidationTypeHandling: result = await store.consolidate(session, provider, "test-model", memory_window=50) assert result is False + + @pytest.mark.asyncio + async def test_missing_history_entry_returns_false_without_writing(self, tmp_path: Path) -> None: + """Do not persist partial results when required fields are missing.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=LLMResponse( + content=None, + tool_calls=[ + ToolCallRequest( + id="call_1", + name="save_memory", + arguments={"memory_update": "# Memory\nOnly memory update"}, + ) + ], + ) + ) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is False + assert not store.history_file.exists() + assert not store.memory_file.exists() + assert session.last_consolidated == 0 + + @pytest.mark.asyncio + async def test_missing_memory_update_returns_false_without_writing(self, tmp_path: Path) -> None: + """Do not append history if memory_update is missing.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=LLMResponse( + content=None, + tool_calls=[ + ToolCallRequest( + id="call_1", + name="save_memory", + arguments={"history_entry": "[2026-01-01] Partial output."}, + ) + ], + ) + ) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is False + assert not store.history_file.exists() + assert not store.memory_file.exists() + assert session.last_consolidated == 0 + + @pytest.mark.asyncio + async def test_null_required_field_returns_false_without_writing(self, tmp_path: Path) -> None: + """Null required fields should be rejected before persistence.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=_make_tool_response( + history_entry=None, + memory_update="# Memory\nUser likes testing.", + ) + ) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is False + assert not store.history_file.exists() + assert not store.memory_file.exists() + assert session.last_consolidated == 0 + + @pytest.mark.asyncio + async def test_empty_history_entry_returns_false_without_writing(self, tmp_path: Path) -> None: + """Empty history entries should be rejected to avoid blank archival records.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=_make_tool_response( + history_entry=" ", + memory_update="# Memory\nUser likes testing.", + ) + ) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is False + assert not store.history_file.exists() + assert not store.memory_file.exists() + assert session.last_consolidated == 0 From 6d3a0ab6c93a7df0b04137b85ca560aba855bf83 Mon Sep 17 00:00:00 2001 From: Xubin Ren Date: Fri, 13 Mar 2026 03:53:50 +0000 Subject: [PATCH 2/2] fix(memory): validate save_memory payload and raw-archive on repeated failure - Require both history_entry and memory_update, reject null/empty values - Fallback to tool_choice=auto when provider rejects forced function call - After 3 consecutive consolidation failures, raw-archive messages to HISTORY.md without LLM summarization to prevent context window overflow --- nanobot/agent/memory.py | 35 +++++++++++++++--- tests/test_memory_consolidation_types.py | 45 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 5 deletions(-) diff --git a/nanobot/agent/memory.py b/nanobot/agent/memory.py index 8cc68bc..f220f23 100644 --- a/nanobot/agent/memory.py +++ b/nanobot/agent/memory.py @@ -5,6 +5,7 @@ from __future__ import annotations import asyncio import json import weakref +from datetime import datetime from pathlib import Path from typing import TYPE_CHECKING, Any, Callable @@ -74,10 +75,13 @@ def _is_tool_choice_unsupported(content: str | None) -> bool: class MemoryStore: """Two-layer memory: MEMORY.md (long-term facts) + HISTORY.md (grep-searchable log).""" + _MAX_FAILURES_BEFORE_RAW_ARCHIVE = 3 + def __init__(self, workspace: Path): self.memory_dir = ensure_dir(workspace / "memory") self.memory_file = self.memory_dir / "MEMORY.md" self.history_file = self.memory_dir / "HISTORY.md" + self._consecutive_failures = 0 def read_long_term(self) -> str: if self.memory_file.exists(): @@ -159,39 +163,60 @@ class MemoryStore: len(response.content or ""), (response.content or "")[:200], ) - return False + return self._fail_or_raw_archive(messages) args = _normalize_save_memory_args(response.tool_calls[0].arguments) if args is None: logger.warning("Memory consolidation: unexpected save_memory arguments") - return False + return self._fail_or_raw_archive(messages) if "history_entry" not in args or "memory_update" not in args: logger.warning("Memory consolidation: save_memory payload missing required fields") - return False + return self._fail_or_raw_archive(messages) entry = args["history_entry"] update = args["memory_update"] if entry is None or update is None: logger.warning("Memory consolidation: save_memory payload contains null required fields") - return False + return self._fail_or_raw_archive(messages) entry = _ensure_text(entry).strip() if not entry: logger.warning("Memory consolidation: history_entry is empty after normalization") - return False + return self._fail_or_raw_archive(messages) self.append_history(entry) update = _ensure_text(update) if update != current_memory: self.write_long_term(update) + self._consecutive_failures = 0 logger.info("Memory consolidation done for {} messages", len(messages)) return True except Exception: logger.exception("Memory consolidation failed") + return self._fail_or_raw_archive(messages) + + def _fail_or_raw_archive(self, messages: list[dict]) -> bool: + """Increment failure count; after threshold, raw-archive messages and return True.""" + self._consecutive_failures += 1 + if self._consecutive_failures < self._MAX_FAILURES_BEFORE_RAW_ARCHIVE: return False + self._raw_archive(messages) + self._consecutive_failures = 0 + return True + + def _raw_archive(self, messages: list[dict]) -> None: + """Fallback: dump raw messages to HISTORY.md without LLM summarization.""" + ts = datetime.now().strftime("%Y-%m-%d %H:%M") + self.append_history( + f"[{ts}] [RAW] {len(messages)} messages\n" + f"{self._format_messages(messages)}" + ) + logger.warning( + "Memory consolidation degraded: raw-archived {} messages", len(messages) + ) class MemoryConsolidator: diff --git a/tests/test_memory_consolidation_types.py b/tests/test_memory_consolidation_types.py index a7c872e..d63cc90 100644 --- a/tests/test_memory_consolidation_types.py +++ b/tests/test_memory_consolidation_types.py @@ -431,3 +431,48 @@ class TestMemoryConsolidationTypeHandling: assert result is False assert not store.history_file.exists() + + @pytest.mark.asyncio + async def test_raw_archive_after_consecutive_failures(self, tmp_path: Path) -> None: + """After 3 consecutive failures, raw-archive messages and return True.""" + store = MemoryStore(tmp_path) + no_tool = LLMResponse(content="No tool call.", finish_reason="stop", tool_calls=[]) + provider = AsyncMock() + provider.chat_with_retry = AsyncMock(return_value=no_tool) + messages = _make_messages(message_count=10) + + assert await store.consolidate(messages, provider, "m") is False + assert await store.consolidate(messages, provider, "m") is False + assert await store.consolidate(messages, provider, "m") is True + + assert store.history_file.exists() + content = store.history_file.read_text() + assert "[RAW]" in content + assert "10 messages" in content + assert "msg0" in content + assert not store.memory_file.exists() + + @pytest.mark.asyncio + async def test_raw_archive_counter_resets_on_success(self, tmp_path: Path) -> None: + """A successful consolidation resets the failure counter.""" + store = MemoryStore(tmp_path) + no_tool = LLMResponse(content="Nope.", finish_reason="stop", tool_calls=[]) + ok_resp = _make_tool_response( + history_entry="[2026-01-01] OK.", + memory_update="# Memory\nOK.", + ) + messages = _make_messages(message_count=10) + + provider = AsyncMock() + provider.chat_with_retry = AsyncMock(return_value=no_tool) + assert await store.consolidate(messages, provider, "m") is False + assert await store.consolidate(messages, provider, "m") is False + assert store._consecutive_failures == 2 + + provider.chat_with_retry = AsyncMock(return_value=ok_resp) + assert await store.consolidate(messages, provider, "m") is True + assert store._consecutive_failures == 0 + + provider.chat_with_retry = AsyncMock(return_value=no_tool) + assert await store.consolidate(messages, provider, "m") is False + assert store._consecutive_failures == 1