diff --git a/nanobot/agent/memory.py b/nanobot/agent/memory.py index cdbc49f..978b1bc 100644 --- a/nanobot/agent/memory.py +++ b/nanobot/agent/memory.py @@ -125,6 +125,13 @@ class MemoryStore: return False args = response.tool_calls[0].arguments + # Some providers return arguments as a JSON string instead of dict + if isinstance(args, str): + args = json.loads(args) + if not isinstance(args, dict): + logger.warning("Memory consolidation: unexpected arguments type %s", type(args).__name__) + return False + if entry := args.get("history_entry"): if not isinstance(entry, str): entry = json.dumps(entry, ensure_ascii=False) diff --git a/tests/test_memory_consolidation_types.py b/tests/test_memory_consolidation_types.py new file mode 100644 index 0000000..375c802 --- /dev/null +++ b/tests/test_memory_consolidation_types.py @@ -0,0 +1,147 @@ +"""Test MemoryStore.consolidate() handles non-string tool call arguments. + +Regression test for https://github.com/HKUDS/nanobot/issues/1042 +When memory consolidation receives dict values instead of strings from the LLM +tool call response, it should serialize them to JSON instead of raising TypeError. +""" + +import json +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from nanobot.agent.memory import MemoryStore +from nanobot.providers.base import LLMResponse, ToolCallRequest + + +def _make_session(message_count: int = 30, memory_window: int = 50): + """Create a mock session with messages.""" + session = MagicMock() + session.messages = [ + {"role": "user", "content": f"msg{i}", "timestamp": "2026-01-01 00:00"} + for i in range(message_count) + ] + session.last_consolidated = 0 + return session + + +def _make_tool_response(history_entry, memory_update): + """Create an LLMResponse with a save_memory tool call.""" + return LLMResponse( + content=None, + tool_calls=[ + ToolCallRequest( + id="call_1", + name="save_memory", + arguments={ + "history_entry": history_entry, + "memory_update": memory_update, + }, + ) + ], + ) + + +class TestMemoryConsolidationTypeHandling: + """Test that consolidation handles various argument types correctly.""" + + @pytest.mark.asyncio + async def test_string_arguments_work(self, tmp_path: Path) -> None: + """Normal case: LLM returns string arguments.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=_make_tool_response( + history_entry="[2026-01-01] User discussed testing.", + 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 True + assert store.history_file.exists() + assert "[2026-01-01] User discussed testing." in store.history_file.read_text() + assert "User likes testing." in store.memory_file.read_text() + + @pytest.mark.asyncio + async def test_dict_arguments_serialized_to_json(self, tmp_path: Path) -> None: + """Issue #1042: LLM returns dict instead of string — must not raise TypeError.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=_make_tool_response( + history_entry={"timestamp": "2026-01-01", "summary": "User discussed testing."}, + memory_update={"facts": ["User likes testing"], "topics": ["testing"]}, + ) + ) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is True + assert store.history_file.exists() + history_content = store.history_file.read_text() + parsed = json.loads(history_content.strip()) + assert parsed["summary"] == "User discussed testing." + + memory_content = store.memory_file.read_text() + parsed_mem = json.loads(memory_content) + assert "User likes testing" in parsed_mem["facts"] + + @pytest.mark.asyncio + async def test_string_arguments_as_raw_json(self, tmp_path: Path) -> None: + """Some providers return arguments as a JSON string instead of parsed dict.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + + # Simulate arguments being a JSON string (not yet parsed) + response = LLMResponse( + content=None, + tool_calls=[ + ToolCallRequest( + id="call_1", + name="save_memory", + arguments=json.dumps({ + "history_entry": "[2026-01-01] User discussed testing.", + "memory_update": "# Memory\nUser likes testing.", + }), + ) + ], + ) + provider.chat = AsyncMock(return_value=response) + session = _make_session(message_count=60) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is True + assert "User discussed testing." in store.history_file.read_text() + + @pytest.mark.asyncio + async def test_no_tool_call_returns_false(self, tmp_path: Path) -> None: + """When LLM doesn't use the save_memory tool, return False.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + provider.chat = AsyncMock( + return_value=LLMResponse(content="I summarized the conversation.", tool_calls=[]) + ) + 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() + + @pytest.mark.asyncio + async def test_skips_when_few_messages(self, tmp_path: Path) -> None: + """Consolidation should be a no-op when messages < keep_count.""" + store = MemoryStore(tmp_path) + provider = AsyncMock() + session = _make_session(message_count=10) + + result = await store.consolidate(session, provider, "test-model", memory_window=50) + + assert result is True + provider.chat.assert_not_called()