refactor: move generation settings to provider level, eliminate parameter passthrough

2026-03-11 09:47:04 +00:00
parent a180e84536
commit c72c2ce7e2
8 changed files with 120 additions and 223 deletions
--- a/tests/test_memory_consolidation_types.py
+++ b/tests/test_memory_consolidation_types.py
@@ -265,3 +265,26 @@ class TestMemoryConsolidationTypeHandling:
        assert result is True
        assert provider.calls == 2
        assert delays == [1]
+
+    @pytest.mark.asyncio
+    async def test_consolidation_delegates_to_provider_defaults(self, tmp_path: Path) -> None:
+        """Consolidation no longer passes generation params — the provider owns them."""
+        store = MemoryStore(tmp_path)
+        provider = AsyncMock()
+        provider.chat_with_retry = AsyncMock(
+            return_value=_make_tool_response(
+                history_entry="[2026-01-01] User discussed testing.",
+                memory_update="# Memory\nUser likes testing.",
+            )
+        )
+        messages = _make_messages(message_count=60)
+
+        result = await store.consolidate(messages, provider, "test-model")
+
+        assert result is True
+        provider.chat_with_retry.assert_awaited_once()
+        _, kwargs = provider.chat_with_retry.await_args
+        assert kwargs["model"] == "test-model"
+        assert "temperature" not in kwargs
+        assert "max_tokens" not in kwargs
+        assert "reasoning_effort" not in kwargs
--- a/tests/test_provider_retry.py
+++ b/tests/test_provider_retry.py
@@ -2,7 +2,7 @@ import asyncio

 import pytest

-from nanobot.providers.base import LLMProvider, LLMResponse
+from nanobot.providers.base import GenerationSettings, LLMProvider, LLMResponse


 class ScriptedProvider(LLMProvider):
@@ -10,9 +10,11 @@ class ScriptedProvider(LLMProvider):
        super().__init__()
        self._responses = list(responses)
        self.calls = 0
+        self.last_kwargs: dict = {}

    async def chat(self, *args, **kwargs) -> LLMResponse:
        self.calls += 1
+        self.last_kwargs = kwargs
        response = self._responses.pop(0)
        if isinstance(response, BaseException):
            raise response
@@ -90,3 +92,34 @@ async def test_chat_with_retry_preserves_cancelled_error() -> None:

    with pytest.raises(asyncio.CancelledError):
        await provider.chat_with_retry(messages=[{"role": "user", "content": "hello"}])
+
+
+@pytest.mark.asyncio
+async def test_chat_with_retry_uses_provider_generation_defaults() -> None:
+    """When callers omit generation params, provider.generation defaults are used."""
+    provider = ScriptedProvider([LLMResponse(content="ok")])
+    provider.generation = GenerationSettings(temperature=0.2, max_tokens=321, reasoning_effort="high")
+
+    await provider.chat_with_retry(messages=[{"role": "user", "content": "hello"}])
+
+    assert provider.last_kwargs["temperature"] == 0.2
+    assert provider.last_kwargs["max_tokens"] == 321
+    assert provider.last_kwargs["reasoning_effort"] == "high"
+
+
+@pytest.mark.asyncio
+async def test_chat_with_retry_explicit_override_beats_defaults() -> None:
+    """Explicit kwargs should override provider.generation defaults."""
+    provider = ScriptedProvider([LLMResponse(content="ok")])
+    provider.generation = GenerationSettings(temperature=0.2, max_tokens=321, reasoning_effort="high")
+
+    await provider.chat_with_retry(
+        messages=[{"role": "user", "content": "hello"}],
+        temperature=0.9,
+        max_tokens=9999,
+        reasoning_effort="low",
+    )
+
+    assert provider.last_kwargs["temperature"] == 0.9
+    assert provider.last_kwargs["max_tokens"] == 9999
+    assert provider.last_kwargs["reasoning_effort"] == "low"
--- a/tests/test_subagent_reasoning.py
+++ b/tests/test_subagent_reasoning.py
@@ -1,144 +0,0 @@
-"""Tests for subagent reasoning_content and thinking_blocks handling."""
-
-from __future__ import annotations
-
-import asyncio
-from pathlib import Path
-from unittest.mock import AsyncMock, MagicMock, patch
-
-import pytest
-
-
-class TestSubagentReasoningContent:
-    """Test that subagent properly handles reasoning_content and thinking_blocks."""
-
-    @pytest.mark.asyncio
-    async def test_subagent_message_includes_reasoning_content(self):
-        """Verify reasoning_content is included in assistant messages with tool calls.
-
-        This is the fix for issue #1834: Spawn/subagent tool fails with
-        Deepseek Reasoner due to missing reasoning_content field.
-        """
-        from nanobot.agent.subagent import SubagentManager
-        from nanobot.bus.queue import MessageBus
-        from nanobot.providers.base import LLMResponse, ToolCallRequest
-
-        bus = MessageBus()
-        provider = MagicMock()
-        provider.get_default_model.return_value = "deepseek-reasoner"
-
-        # Create a real Path object for workspace
-        workspace = Path("/tmp/test_workspace")
-        workspace.mkdir(parents=True, exist_ok=True)
-
-        # Capture messages that are sent to the provider
-        captured_messages = []
-
-        async def mock_chat(*args, **kwargs):
-            captured_messages.append(kwargs.get("messages", []))
-            # Return response with tool calls and reasoning_content
-            tool_call = ToolCallRequest(
-                id="test-1",
-                name="read_file",
-                arguments={"path": "/test.txt"},
-            )
-            return LLMResponse(
-                content="",
-                tool_calls=[tool_call],
-                reasoning_content="I need to read this file first",
-            )
-
-        provider.chat_with_retry = AsyncMock(side_effect=mock_chat)
-
-        mgr = SubagentManager(provider=provider, workspace=workspace, bus=bus)
-
-        # Mock the tools registry
-        with patch("nanobot.agent.subagent.ToolRegistry") as MockToolRegistry:
-            mock_registry = MagicMock()
-            mock_registry.get_definitions.return_value = []
-            mock_registry.execute = AsyncMock(return_value="file content")
-            MockToolRegistry.return_value = mock_registry
-
-            result = await mgr.spawn(
-                task="Read a file",
-                label="test",
-                origin_channel="cli",
-                origin_chat_id="direct",
-                session_key="cli:direct",
-            )
-
-            # Wait for the task to complete
-            await asyncio.sleep(0.5)
-
-        # Check the captured messages
-        assert len(captured_messages) >= 1
-        # Find the assistant message with tool_calls
-        found = False
-        for msg_list in captured_messages:
-            for msg in msg_list:
-                if msg.get("role") == "assistant" and msg.get("tool_calls"):
-                    assert "reasoning_content" in msg, "reasoning_content should be in assistant message with tool_calls"
-                    assert msg["reasoning_content"] == "I need to read this file first"
-                    found = True
-        assert found, "Should have found an assistant message with tool_calls"
-
-    @pytest.mark.asyncio
-    async def test_subagent_message_includes_thinking_blocks(self):
-        """Verify thinking_blocks is included in assistant messages with tool calls."""
-        from nanobot.agent.subagent import SubagentManager
-        from nanobot.bus.queue import MessageBus
-        from nanobot.providers.base import LLMResponse, ToolCallRequest
-
-        bus = MessageBus()
-        provider = MagicMock()
-        provider.get_default_model.return_value = "claude-sonnet"
-
-        workspace = Path("/tmp/test_workspace2")
-        workspace.mkdir(parents=True, exist_ok=True)
-
-        captured_messages = []
-
-        async def mock_chat(*args, **kwargs):
-            captured_messages.append(kwargs.get("messages", []))
-            tool_call = ToolCallRequest(
-                id="test-2",
-                name="read_file",
-                arguments={"path": "/test.txt"},
-            )
-            return LLMResponse(
-                content="",
-                tool_calls=[tool_call],
-                thinking_blocks=[
-                    {"signature": "sig1", "thought": "thinking step 1"},
-                    {"signature": "sig2", "thought": "thinking step 2"},
-                ],
-            )
-
-        provider.chat_with_retry = AsyncMock(side_effect=mock_chat)
-
-        mgr = SubagentManager(provider=provider, workspace=workspace, bus=bus)
-
-        with patch("nanobot.agent.subagent.ToolRegistry") as MockToolRegistry:
-            mock_registry = MagicMock()
-            mock_registry.get_definitions.return_value = []
-            mock_registry.execute = AsyncMock(return_value="file content")
-            MockToolRegistry.return_value = mock_registry
-
-            result = await mgr.spawn(
-                task="Read a file",
-                label="test",
-                origin_channel="cli",
-                origin_chat_id="direct",
-            )
-
-            await asyncio.sleep(0.5)
-
-        # Check the captured messages
-        found = False
-        for msg_list in captured_messages:
-            for msg in msg_list:
-                if msg.get("role") == "assistant" and msg.get("tool_calls"):
-                    assert "thinking_blocks" in msg, "thinking_blocks should be in assistant message with tool_calls"
-                    assert len(msg["thinking_blocks"]) == 2
-                    found = True
-        assert found, "Should have found an assistant message with tool_calls"