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>

Author: Brian Krafft

openspace/llm_factory.py

@@ -51,6 +51,9 @@ def create_tool_retrieval(self) -> Optional[LLMClient]:
 
         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

tests/test_llm_factory.py

@@ -161,10 +161,45 @@ def test_inherits_llm_kwargs(self, config):
             factory = LLMFactory(config=config)
             factory.create_tool_retrieval()
 
-            _, kwargs = MockLLM.call_args
+            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