refactor(config): make max messages default explicit
Use 120 as the config-level default and normalize zero back to that limit so session replay always receives an explicit message cap. Made-with: Cursor
This commit is contained in:
@@ -202,7 +202,7 @@ class AgentLoop:
|
||||
timezone: str | None = None,
|
||||
session_ttl_minutes: int = 0,
|
||||
consolidation_ratio: float = 0.5,
|
||||
max_messages: int = 0,
|
||||
max_messages: int = 120,
|
||||
hooks: list[AgentHook] | None = None,
|
||||
unified_session: bool = False,
|
||||
disabled_skills: list[str] | None = None,
|
||||
@@ -260,7 +260,7 @@ class AgentLoop:
|
||||
disabled_skills=disabled_skills,
|
||||
)
|
||||
self._unified_session = unified_session
|
||||
self._max_messages = max_messages if max_messages > 0 else 0
|
||||
self._max_messages = max_messages if max_messages > 0 else 120
|
||||
self._running = False
|
||||
self._mcp_servers = mcp_servers or {}
|
||||
self._mcp_stacks: dict[str, AsyncExitStack] = {}
|
||||
@@ -887,11 +887,10 @@ class AgentLoop:
|
||||
msg.metadata, session_key=key,
|
||||
)
|
||||
_hist_kwargs: dict[str, Any] = {
|
||||
"max_messages": self._max_messages,
|
||||
"max_tokens": self._replay_token_budget(),
|
||||
"include_timestamps": True,
|
||||
}
|
||||
if self._max_messages > 0:
|
||||
_hist_kwargs["max_messages"] = self._max_messages
|
||||
history = session.get_history(**_hist_kwargs)
|
||||
current_role = "assistant" if is_subagent else "user"
|
||||
|
||||
@@ -977,11 +976,10 @@ class AgentLoop:
|
||||
message_tool.start_turn()
|
||||
|
||||
_hist_kwargs: dict[str, Any] = {
|
||||
"max_messages": self._max_messages,
|
||||
"max_tokens": self._replay_token_budget(),
|
||||
"include_timestamps": True,
|
||||
}
|
||||
if self._max_messages > 0:
|
||||
_hist_kwargs["max_messages"] = self._max_messages
|
||||
history = session.get_history(**_hist_kwargs)
|
||||
|
||||
pending_ask_id = pending_ask_user_id(history)
|
||||
|
||||
@@ -91,9 +91,9 @@ class AgentDefaults(Base):
|
||||
serialization_alias="idleCompactAfterMinutes",
|
||||
) # Auto-compact idle threshold in minutes (0 = disabled)
|
||||
max_messages: int = Field(
|
||||
default=0,
|
||||
default=120,
|
||||
ge=0,
|
||||
) # Max messages to replay from session history (0 = use default, respects token budget)
|
||||
) # Max messages to replay from session history (0 = use default 120, respects token budget)
|
||||
consolidation_ratio: float = Field(
|
||||
default=0.5,
|
||||
ge=0.1,
|
||||
|
||||
@@ -12,15 +12,13 @@ from loguru import logger
|
||||
|
||||
from nanobot.config.paths import get_legacy_sessions_dir
|
||||
from nanobot.utils.helpers import (
|
||||
estimate_message_tokens,
|
||||
ensure_dir,
|
||||
estimate_message_tokens,
|
||||
find_legal_message_start,
|
||||
image_placeholder_text,
|
||||
safe_filename,
|
||||
)
|
||||
|
||||
|
||||
HISTORY_MAX_MESSAGES = 120
|
||||
FILE_MAX_MESSAGES = 2000
|
||||
|
||||
|
||||
@@ -74,7 +72,7 @@ class Session:
|
||||
|
||||
def get_history(
|
||||
self,
|
||||
max_messages: int = HISTORY_MAX_MESSAGES,
|
||||
max_messages: int = 120,
|
||||
*,
|
||||
max_tokens: int = 0,
|
||||
include_timestamps: bool = False,
|
||||
@@ -85,6 +83,7 @@ class Session:
|
||||
token budget from the tail (``max_tokens``) when provided.
|
||||
"""
|
||||
unconsolidated = self.messages[self.last_consolidated:]
|
||||
max_messages = max_messages if max_messages > 0 else 120
|
||||
sliced = unconsolidated[-max_messages:]
|
||||
|
||||
# Avoid starting mid-turn when possible, except for proactive
|
||||
|
||||
@@ -10,8 +10,8 @@ import pytest
|
||||
from nanobot.agent.loop import AgentLoop
|
||||
from nanobot.bus.events import InboundMessage
|
||||
from nanobot.bus.queue import MessageBus
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
from nanobot.command import CommandContext
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
from nanobot.providers.base import LLMResponse
|
||||
|
||||
|
||||
@@ -75,10 +75,9 @@ class TestSessionTTLConfig:
|
||||
assert data["idleCompactAfterMinutes"] == 30
|
||||
assert "sessionTtlMinutes" not in data
|
||||
|
||||
def test_session_history_and_file_cap_are_internal_constants(self):
|
||||
"""Session history/file cap should be internal constants, not config fields."""
|
||||
from nanobot.session.manager import HISTORY_MAX_MESSAGES, FILE_MAX_MESSAGES
|
||||
assert HISTORY_MAX_MESSAGES == 120
|
||||
def test_session_file_cap_is_internal_constant(self):
|
||||
"""Session file cap should remain an internal constant, not a config field."""
|
||||
from nanobot.session.manager import FILE_MAX_MESSAGES
|
||||
assert FILE_MAX_MESSAGES == 2000
|
||||
|
||||
|
||||
@@ -265,7 +264,6 @@ class TestAutoCompact:
|
||||
async def test_auto_compact_empty_session(self, tmp_path):
|
||||
"""_archive on empty session should not archive."""
|
||||
loop = _make_loop(tmp_path, session_ttl_minutes=15)
|
||||
session = loop.sessions.get_or_create("cli:test")
|
||||
|
||||
archive_called = False
|
||||
|
||||
|
||||
@@ -3,17 +3,20 @@
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from unittest.mock import MagicMock, patch
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from nanobot.agent.loop import AgentLoop
|
||||
from nanobot.bus.events import InboundMessage
|
||||
from nanobot.bus.queue import MessageBus
|
||||
from nanobot.session.manager import HISTORY_MAX_MESSAGES, Session
|
||||
from nanobot.providers.base import LLMResponse
|
||||
from nanobot.session.manager import Session
|
||||
|
||||
DEFAULT_MAX_MESSAGES = 120
|
||||
|
||||
|
||||
def _make_loop(tmp_path: Path, max_messages: int = 0) -> AgentLoop:
|
||||
def _make_loop(tmp_path: Path, max_messages: int = DEFAULT_MAX_MESSAGES) -> AgentLoop:
|
||||
provider = MagicMock()
|
||||
provider.get_default_model.return_value = "test-model"
|
||||
return AgentLoop(
|
||||
@@ -37,33 +40,31 @@ def _populated_session(n: int) -> Session:
|
||||
class TestMaxMessagesInit:
|
||||
"""Verify AgentLoop stores the config value correctly."""
|
||||
|
||||
def test_default_is_zero(self, tmp_path: Path) -> None:
|
||||
def test_default_is_builtin_limit(self, tmp_path: Path) -> None:
|
||||
loop = _make_loop(tmp_path)
|
||||
assert loop._max_messages == 0
|
||||
assert loop._max_messages == DEFAULT_MAX_MESSAGES
|
||||
|
||||
def test_positive_value_stored(self, tmp_path: Path) -> None:
|
||||
loop = _make_loop(tmp_path, max_messages=25)
|
||||
assert loop._max_messages == 25
|
||||
|
||||
def test_zero_means_unlimited(self, tmp_path: Path) -> None:
|
||||
"""max_messages=0 should not constrain get_history (uses default)."""
|
||||
def test_zero_uses_builtin_limit(self, tmp_path: Path) -> None:
|
||||
loop = _make_loop(tmp_path, max_messages=0)
|
||||
assert loop._max_messages == 0
|
||||
assert loop._max_messages == DEFAULT_MAX_MESSAGES
|
||||
|
||||
def test_negative_treated_as_zero(self, tmp_path: Path) -> None:
|
||||
def test_negative_treated_as_builtin_limit(self, tmp_path: Path) -> None:
|
||||
"""Negative values should not produce negative slicing."""
|
||||
loop = _make_loop(tmp_path, max_messages=-5)
|
||||
assert loop._max_messages == 0
|
||||
assert loop._max_messages == DEFAULT_MAX_MESSAGES
|
||||
|
||||
|
||||
class TestGetHistoryWithMaxMessages:
|
||||
"""Verify get_history respects max_messages parameter."""
|
||||
|
||||
def test_default_uses_constant(self) -> None:
|
||||
def test_default_uses_builtin_limit(self) -> None:
|
||||
session = _populated_session(80)
|
||||
history = session.get_history()
|
||||
# Default HISTORY_MAX_MESSAGES=120, 80 pairs = 160 msgs, sliced to 120
|
||||
assert len(history) <= HISTORY_MAX_MESSAGES
|
||||
assert len(history) <= DEFAULT_MAX_MESSAGES
|
||||
|
||||
def test_explicit_max_messages_limits_output(self) -> None:
|
||||
session = _populated_session(40) # 80 messages total
|
||||
@@ -76,13 +77,10 @@ class TestGetHistoryWithMaxMessages:
|
||||
history = session.get_history(max_messages=25)
|
||||
assert history[0]["role"] == "user"
|
||||
|
||||
def test_max_messages_zero_returns_all(self) -> None:
|
||||
"""max_messages=0 with the default constant returns up to the constant."""
|
||||
session = _populated_session(10) # 20 messages
|
||||
# When we pass 0 explicitly, unconsolidated[-0:] returns everything
|
||||
# but the default is HISTORY_MAX_MESSAGES so this tests the default path
|
||||
history = session.get_history()
|
||||
assert len(history) == 20
|
||||
def test_max_messages_zero_uses_builtin_limit(self) -> None:
|
||||
session = _populated_session(80) # 160 messages total
|
||||
history = session.get_history(max_messages=0)
|
||||
assert len(history) <= DEFAULT_MAX_MESSAGES
|
||||
|
||||
def test_small_session_unaffected(self) -> None:
|
||||
"""When session has fewer messages than max_messages, all are returned."""
|
||||
@@ -94,41 +92,43 @@ class TestGetHistoryWithMaxMessages:
|
||||
class TestMaxMessagesIntegration:
|
||||
"""Verify the config flows from AgentLoop into get_history calls."""
|
||||
|
||||
def test_config_wired_to_history_call(self, tmp_path: Path) -> None:
|
||||
"""When max_messages > 0, get_history should receive it."""
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_message_passes_config_to_history_call(self, tmp_path: Path) -> None:
|
||||
"""The real message path should pass max_messages into session history replay."""
|
||||
loop = _make_loop(tmp_path, max_messages=25)
|
||||
session = _populated_session(40) # 80 messages
|
||||
loop.provider.chat_with_retry = AsyncMock(
|
||||
return_value=LLMResponse(content="ok", tool_calls=[], usage={})
|
||||
)
|
||||
loop.tools.get_definitions = MagicMock(return_value=[])
|
||||
loop.consolidator.maybe_consolidate_by_tokens = AsyncMock(return_value=False) # type: ignore[method-assign]
|
||||
|
||||
session = loop.sessions.get_or_create("cli:test")
|
||||
with patch.object(session, "get_history", wraps=session.get_history) as mock_hist:
|
||||
# Call the internal method that builds history kwargs
|
||||
kwargs: dict[str, Any] = {
|
||||
"max_tokens": loop._replay_token_budget(),
|
||||
"include_timestamps": True,
|
||||
}
|
||||
if loop._max_messages > 0:
|
||||
kwargs["max_messages"] = loop._max_messages
|
||||
session.get_history(**kwargs)
|
||||
result = await loop._process_message(
|
||||
InboundMessage(channel="cli", sender_id="user", chat_id="test", content="hello")
|
||||
)
|
||||
|
||||
assert mock_hist.call_count == 1
|
||||
call_kwargs = mock_hist.call_args
|
||||
# max_messages is positional arg (first) or keyword
|
||||
if call_kwargs.args:
|
||||
assert call_kwargs.args[0] == 25
|
||||
else:
|
||||
assert call_kwargs.kwargs.get("max_messages") == 25
|
||||
assert result is not None
|
||||
assert mock_hist.call_count == 1
|
||||
assert mock_hist.call_args.kwargs["max_messages"] == 25
|
||||
|
||||
def test_zero_config_omits_max_messages_kwarg(self, tmp_path: Path) -> None:
|
||||
"""When max_messages=0, get_history should use its default."""
|
||||
@pytest.mark.asyncio
|
||||
async def test_zero_config_passes_builtin_limit_to_history_call(self, tmp_path: Path) -> None:
|
||||
loop = _make_loop(tmp_path, max_messages=0)
|
||||
loop.provider.chat_with_retry = AsyncMock(
|
||||
return_value=LLMResponse(content="ok", tool_calls=[], usage={})
|
||||
)
|
||||
loop.tools.get_definitions = MagicMock(return_value=[])
|
||||
loop.consolidator.maybe_consolidate_by_tokens = AsyncMock(return_value=False) # type: ignore[method-assign]
|
||||
|
||||
kwargs: dict[str, Any] = {
|
||||
"max_tokens": loop._replay_token_budget(),
|
||||
"include_timestamps": True,
|
||||
}
|
||||
if loop._max_messages > 0:
|
||||
kwargs["max_messages"] = loop._max_messages
|
||||
session = loop.sessions.get_or_create("cli:test")
|
||||
with patch.object(session, "get_history", wraps=session.get_history) as mock_hist:
|
||||
result = await loop._process_message(
|
||||
InboundMessage(channel="cli", sender_id="user", chat_id="test", content="hello")
|
||||
)
|
||||
|
||||
assert "max_messages" not in kwargs
|
||||
assert result is not None
|
||||
assert mock_hist.call_args.kwargs["max_messages"] == DEFAULT_MAX_MESSAGES
|
||||
|
||||
|
||||
class TestSchemaConfig:
|
||||
@@ -138,6 +138,12 @@ class TestSchemaConfig:
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
|
||||
defaults = AgentDefaults()
|
||||
assert defaults.max_messages == DEFAULT_MAX_MESSAGES
|
||||
|
||||
def test_schema_accepts_zero_as_builtin_limit(self) -> None:
|
||||
from nanobot.config.schema import AgentDefaults
|
||||
|
||||
defaults = AgentDefaults(max_messages=0)
|
||||
assert defaults.max_messages == 0
|
||||
|
||||
def test_schema_accepts_positive(self) -> None:
|
||||
|
||||
Reference in New Issue
Block a user