feat(4.5): extract LLMFactory from OpenSpace (#34)

* feat(4.5): extract LLMFactory from OpenSpace

New module: openspace/llm_factory.py (66 lines)
- LLMFactory class: centralized LLM client creation from config
- create_main(): primary LLM client with full config params
- create_tool_retrieval(): optional tool retrieval LLM (shares credentials)

tool_layer.py: replaced 18 lines of inline LLM construction
with 6 lines delegating to LLMFactory (-12 net lines)

Tests: 12 tests in test_llm_factory.py
- Init state and config storage
- create_main: full config passthrough, defaults, idempotency
- create_tool_retrieval: enabled/disabled, config passthrough, kwargs inheritance
- OpenSpace backward compatibility

All 1,356 existing tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix(4.5): address review findings from /8eyes + /collab Round 1

- Add docstring note on intentional param asymmetry in create_tool_retrieval()
- Add empty-string model test (M1: treats '' same as None)
- Add create_tool_retrieval twice test (M2: idempotency)
- Add constructor exception test (C2: propagates, client stays None)
- Fix fragile call_args unpacking (use .kwargs)

Tests: 12 → 16

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Brian Krafft <bkrafft@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: 3 people

openspace/llm_factory.py

@@ -0,0 +1,67 @@
+"""LLMFactory — centralized LLM client creation from config.
+
+Extracted from OpenSpace.initialize() in Epic 4.5.  Owns:
+  • Main LLM client creation with full config
+  • Optional tool retrieval LLM creation (shares credentials via llm_kwargs)
+"""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Optional
+
+from openspace.llm import LLMClient
+from openspace.utils.logging import Logger
+
+if TYPE_CHECKING:
+    from openspace.tool_layer import OpenSpaceConfig
+
+logger = Logger.get_logger(__name__)
+
+
+class LLMFactory:
+    """Creates LLM clients from OpenSpaceConfig."""
+
+    def __init__(self, *, config: OpenSpaceConfig) -> None:
+        self._config = config
+        self._llm_client: Optional[LLMClient] = None
+        self._tool_retrieval_llm: Optional[LLMClient] = None
+
+    @property
+    def llm_client(self) -> Optional[LLMClient]:
+        return self._llm_client
+
+    @property
+    def tool_retrieval_llm(self) -> Optional[LLMClient]:
+        return self._tool_retrieval_llm
+
+    def create_main(self) -> LLMClient:
+        """Create the primary LLM client from config."""
+        self._llm_client = LLMClient(
+            model=self._config.llm_model,
+            enable_thinking=self._config.llm_enable_thinking,
+            rate_limit_delay=self._config.llm_rate_limit_delay,
+            max_retries=self._config.llm_max_retries,
+            timeout=self._config.llm_timeout,
+            **self._config.llm_kwargs,
+        )
+        return self._llm_client
+
+    def create_tool_retrieval(self) -> Optional[LLMClient]:
+        """Create optional tool retrieval LLM. Returns None if not configured.
+
+        Inherits llm_kwargs (api_key, api_base, etc.) so credentials
+        from the host agent are shared across all internal LLM clients.
+
+        Note: intentionally omits ``enable_thinking`` and ``rate_limit_delay``
+        since tool retrieval is a simple selection task, not a reasoning task.
+        """
+        if not self._config.tool_retrieval_model:
+            return None
+
+        self._tool_retrieval_llm = LLMClient(
+            model=self._config.tool_retrieval_model,
+            timeout=self._config.llm_timeout,
+            max_retries=self._config.llm_max_retries,
+            **self._config.llm_kwargs,
+        )
+        return self._tool_retrieval_llm

openspace/tool_layer.py

@@ -13,6 +13,7 @@
 from openspace.grounding.core.grounding_client import GroundingClient
 from openspace.execution_engine import ExecutionEngine
 from openspace.llm import LLMClient
+from openspace.llm_factory import LLMFactory
 from openspace.recording_service import RecordingService
 from openspace.skill_engine import ExecutionAnalyzer, SkillRegistry, SkillStore
 from openspace.skill_engine.evolver import SkillEvolver
@@ -125,6 +126,7 @@ def __init__(
         self._container = container or AppContainer()
 
         self._llm_client: Optional[LLMClient] = None
+        self._llm_factory: Optional[LLMFactory] = None
         self._grounding_client: Optional[GroundingClient] = None
         self._grounding_config = None  # GroundingConfig reference for skill settings
         self._grounding_agent: Optional[GroundingAgent] = None
@@ -205,14 +207,8 @@ async def initialize(self) -> None:
         logger.info("Initializing OpenSpace...")
 
         try:
-            self._llm_client = LLMClient(
-                model=self.config.llm_model,
-                enable_thinking=self.config.llm_enable_thinking,
-                rate_limit_delay=self.config.llm_rate_limit_delay,
-                max_retries=self.config.llm_max_retries,
-                timeout=self.config.llm_timeout,
-                **self.config.llm_kwargs,
-            )
+            self._llm_factory = LLMFactory(config=self.config)
+            self._llm_client = self._llm_factory.create_main()
             logger.info(f"✓ LLM Client: {self.config.llm_model}")
 
             # Load grounding config
@@ -300,16 +296,8 @@ async def initialize(self) -> None:
                 logger.info(f"✓ Recording enabled: {len(self._recording_manager.backends or [])} backends")
 
             # Create separate LLM client for tool retrieval if configured
-            # Inherits llm_kwargs (api_key, api_base, etc.) so credentials
-            # from the host agent are shared across all internal LLM clients.
-            tool_retrieval_llm = None
-            if self.config.tool_retrieval_model:
-                tool_retrieval_llm = LLMClient(
-                    model=self.config.tool_retrieval_model,
-                    timeout=self.config.llm_timeout,
-                    max_retries=self.config.llm_max_retries,
-                    **self.config.llm_kwargs,
-                )
+            tool_retrieval_llm = self._llm_factory.create_tool_retrieval()
+            if tool_retrieval_llm:
                 logger.info(f"✓ Tool retrieval LLM: {self.config.tool_retrieval_model}")
 
             self._grounding_agent = GroundingAgent(

tests/test_llm_factory.py

@@ -0,0 +1,217 @@
+"""Tests for LLMFactory — extracted LLM client creation from OpenSpace."""
+
+from __future__ import annotations
+
+import pytest
+from unittest.mock import MagicMock, patch, call
+
+try:
+    from openspace.tool_layer import OpenSpace, OpenSpaceConfig
+    from openspace.llm_factory import LLMFactory
+
+    _HAS_TOOL_LAYER = True
+except Exception:
+    _HAS_TOOL_LAYER = False
+
+pytestmark = pytest.mark.skipif(not _HAS_TOOL_LAYER, reason="tool_layer deps unavailable")
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+@pytest.fixture
+def config():
+    return OpenSpaceConfig(
+        llm_model="openrouter/anthropic/claude-sonnet-4.5",
+        llm_enable_thinking=True,
+        llm_timeout=60.0,
+        llm_max_retries=5,
+        llm_rate_limit_delay=0.5,
+        llm_kwargs={"api_key": "sk-test"},
+        tool_retrieval_model="openrouter/openai/gpt-4o",
+    )
+
+
+@pytest.fixture
+def minimal_config():
+    return OpenSpaceConfig()
+
+
+# ---------------------------------------------------------------------------
+# LLMFactory.__init__
+# ---------------------------------------------------------------------------
+
+class TestLLMFactoryInit:
+
+    def test_initial_state(self, config):
+        factory = LLMFactory(config=config)
+        assert factory.llm_client is None
+        assert factory.tool_retrieval_llm is None
+
+    def test_stores_config(self, config):
+        factory = LLMFactory(config=config)
+        assert factory._config is config
+
+
+# ---------------------------------------------------------------------------
+# LLMFactory.create_main()
+# ---------------------------------------------------------------------------
+
+class TestCreateMain:
+
+    def test_creates_llm_client(self, config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            mock_client = MagicMock()
+            MockLLM.return_value = mock_client
+
+            factory = LLMFactory(config=config)
+            result = factory.create_main()
+
+            assert result is mock_client
+            assert factory.llm_client is mock_client
+
+    def test_passes_all_config_fields(self, config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            MockLLM.return_value = MagicMock()
+
+            factory = LLMFactory(config=config)
+            factory.create_main()
+
+            MockLLM.assert_called_once_with(
+                model="openrouter/anthropic/claude-sonnet-4.5",
+                enable_thinking=True,
+                rate_limit_delay=0.5,
+                max_retries=5,
+                timeout=60.0,
+                api_key="sk-test",
+            )
+
+    def test_uses_defaults_when_minimal_config(self, minimal_config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            MockLLM.return_value = MagicMock()
+
+            factory = LLMFactory(config=minimal_config)
+            factory.create_main()
+
+            MockLLM.assert_called_once_with(
+                model="openrouter/anthropic/claude-sonnet-4.5",
+                enable_thinking=False,
+                rate_limit_delay=0.0,
+                max_retries=3,
+                timeout=120.0,
+            )
+
+    def test_create_main_twice_replaces_client(self, config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            first = MagicMock()
+            second = MagicMock()
+            MockLLM.side_effect = [first, second]
+
+            factory = LLMFactory(config=config)
+            factory.create_main()
+            assert factory.llm_client is first
+            factory.create_main()
+            assert factory.llm_client is second
+
+
+# ---------------------------------------------------------------------------
+# LLMFactory.create_tool_retrieval()
+# ---------------------------------------------------------------------------
+
+class TestCreateToolRetrieval:
+
+    def test_creates_when_model_configured(self, config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            mock_client = MagicMock()
+            MockLLM.return_value = mock_client
+
+            factory = LLMFactory(config=config)
+            result = factory.create_tool_retrieval()
+
+            assert result is mock_client
+            assert factory.tool_retrieval_llm is mock_client
+
+    def test_returns_none_when_no_model(self, minimal_config):
+        factory = LLMFactory(config=minimal_config)
+        result = factory.create_tool_retrieval()
+        assert result is None
+        assert factory.tool_retrieval_llm is None
+
+    def test_passes_correct_config(self, config):
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            MockLLM.return_value = MagicMock()
+
+            factory = LLMFactory(config=config)
+            factory.create_tool_retrieval()
+
+            MockLLM.assert_called_once_with(
+                model="openrouter/openai/gpt-4o",
+                timeout=60.0,
+                max_retries=5,
+                api_key="sk-test",
+            )
+
+    def test_inherits_llm_kwargs(self, config):
+        """Tool retrieval LLM inherits credentials from llm_kwargs."""
+        config.llm_kwargs = {"api_key": "sk-shared", "api_base": "https://custom"}
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            MockLLM.return_value = MagicMock()
+
+            factory = LLMFactory(config=config)
+            factory.create_tool_retrieval()
+
+            kwargs = MockLLM.call_args.kwargs
+            assert kwargs["api_key"] == "sk-shared"
+            assert kwargs["api_base"] == "https://custom"
+
+    def test_returns_none_when_empty_string_model(self, config):
+        """Empty string model treated as unconfigured."""
+        config.tool_retrieval_model = ""
+        factory = LLMFactory(config=config)
+        result = factory.create_tool_retrieval()
+        assert result is None
+        assert factory.tool_retrieval_llm is None
+
+    def test_create_tool_retrieval_twice_replaces(self, config):
+        """Second call replaces the tool retrieval client."""
+        with patch("openspace.llm_factory.LLMClient") as MockLLM:
+            first, second = MagicMock(), MagicMock()
+            MockLLM.side_effect = [first, second]
+
+            factory = LLMFactory(config=config)
+            factory.create_tool_retrieval()
+            assert factory.tool_retrieval_llm is first
+            factory.create_tool_retrieval()
+            assert factory.tool_retrieval_llm is second
+
+
+# ---------------------------------------------------------------------------
+# Error handling
+# ---------------------------------------------------------------------------
+
+class TestErrorHandling:
+
+    def test_create_main_exception_propagates(self, config):
+        """LLMClient constructor failure propagates, client stays None."""
+        with patch("openspace.llm_factory.LLMClient", side_effect=RuntimeError("boom")):
+            factory = LLMFactory(config=config)
+            with pytest.raises(RuntimeError, match="boom"):
+                factory.create_main()
+            assert factory.llm_client is None
+
+
+# ---------------------------------------------------------------------------
+# OpenSpace backward compatibility
+# ---------------------------------------------------------------------------
+
+class TestOpenSpaceDelegation:
+
+    def test_openspace_has_llm_factory_attr(self):
+        os_instance = OpenSpace()
+        assert hasattr(os_instance, "_llm_factory")
+
+    def test_llm_client_still_accessible(self):
+        os_instance = OpenSpace()
+        assert hasattr(os_instance, "_llm_client")
+        assert os_instance._llm_client is None