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

Code fixes:
- Null manager after cleanup() (prevents stale references, enables safe double-cleanup)
- Re-raise CancelledError in cleanup() (proper async cancellation semantics)
- Add debug log after successful recording stop (observability parity)
- Move RecordingManager import to TYPE_CHECKING in tool_layer.py (dead runtime import)
- Add asyncio import to recording_service.py for CancelledError

Test improvements (14 → 17 tests):
- C1: Assert logger.warning called on exception (not just 'no raise')
- C2: create() twice replaces manager (idempotency coverage)
- H1: cleanup() twice only stops once (double-cleanup safety)
- H2: wire() before create() is safe noop
- Strengthen noop assertions (verify manager is None, not just 'no raise')
- Verify manager nulled after cleanup

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

Author: Brian Krafft

openspace/recording_service.py

@@ -8,6 +8,7 @@
 
 from __future__ import annotations
 
+import asyncio
 from typing import TYPE_CHECKING, Optional
 
 from openspace.recording import RecordingManager
@@ -62,5 +63,9 @@ async def cleanup(self) -> None:
         if self._manager.recording_status:
             try:
                 await self._manager.stop()
+                logger.debug("Recording stopped")
+            except asyncio.CancelledError:
+                raise
             except Exception as e:
                 logger.warning("Failed to stop recording: %s", e)
+        self._manager = None

openspace/tool_layer.py

@@ -4,7 +4,7 @@
 import uuid
 from dataclasses import dataclass, field
 from pathlib import Path
-from typing import Any, Dict, List, Optional
+from typing import TYPE_CHECKING, Any, Dict, List, Optional
 
 from openspace.agents import GroundingAgent
 from openspace.app.container import AppContainer
@@ -13,13 +13,15 @@
 from openspace.grounding.core.grounding_client import GroundingClient
 from openspace.execution_engine import ExecutionEngine
 from openspace.llm import LLMClient
-from openspace.recording import RecordingManager
 from openspace.recording_service import RecordingService
 from openspace.skill_engine import ExecutionAnalyzer, SkillRegistry, SkillStore
 from openspace.skill_engine.evolver import SkillEvolver
 from openspace.tool_registry import ToolRegistry
 from openspace.utils.logging import Logger
 
+if TYPE_CHECKING:
+    from openspace.recording import RecordingManager
+
 logger = Logger.get_logger(__name__)
 
 

tests/test_recording_service.py

@@ -110,6 +110,20 @@ def test_registers_to_llm(self, config, mock_llm_client):
             svc.create(llm_client=mock_llm_client)
             mock_instance.register_to_llm.assert_called_once_with(mock_llm_client)
 
+    def test_create_twice_replaces_manager(self, config, mock_llm_client):
+        """Second create() silently replaces manager."""
+        with patch("openspace.recording_service.RecordingManager") as MockRM:
+            first = MagicMock()
+            second = MagicMock()
+            MockRM.side_effect = [first, second]
+
+            svc = RecordingService(config=config)
+            svc.create(llm_client=mock_llm_client)
+            assert svc.manager is first
+            svc.create(llm_client=mock_llm_client)
+            assert svc.manager is second
+            assert MockRM.call_count == 2
+
 
 # ---------------------------------------------------------------------------
 # RecordingService.wire()
@@ -133,6 +147,12 @@ def test_noop_when_no_manager(self, disabled_config, mock_llm_client, mock_groun
         svc.wire(grounding_client=mock_grounding_client)
         assert mock_grounding_client.recording_manager is None
 
+    def test_wire_without_create_is_noop(self, config, mock_grounding_client):
+        """wire() before create() is a safe noop."""
+        svc = RecordingService(config=config)
+        svc.wire(grounding_client=mock_grounding_client)
+        assert mock_grounding_client.recording_manager is None
+
 
 # ---------------------------------------------------------------------------
 # RecordingService.cleanup()
@@ -141,7 +161,7 @@ def test_noop_when_no_manager(self, disabled_config, mock_llm_client, mock_groun
 class TestRecordingServiceCleanup:
 
     @pytest.mark.asyncio
-    async def test_stops_active_recording(self, config, mock_llm_client):
+    async def test_stops_active_recording_and_nulls_manager(self, config, mock_llm_client):
         with patch("openspace.recording_service.RecordingManager") as MockRM:
             mock_instance = MagicMock()
             mock_instance.recording_status = True
@@ -152,6 +172,7 @@ async def test_stops_active_recording(self, config, mock_llm_client):
             svc.create(llm_client=mock_llm_client)
             await svc.cleanup()
             mock_instance.stop.assert_awaited_once()
+            assert svc.manager is None
 
     @pytest.mark.asyncio
     async def test_noop_when_not_recording(self, config, mock_llm_client):
@@ -169,19 +190,38 @@ async def test_noop_when_not_recording(self, config, mock_llm_client):
     @pytest.mark.asyncio
     async def test_noop_when_no_manager(self, disabled_config):
         svc = RecordingService(config=disabled_config)
-        await svc.cleanup()  # No raise
+        await svc.cleanup()
+        assert svc.manager is None
 
     @pytest.mark.asyncio
     async def test_stop_exception_swallowed_and_logged(self, config, mock_llm_client):
-        with patch("openspace.recording_service.RecordingManager") as MockRM:
+        with patch("openspace.recording_service.RecordingManager") as MockRM, \
+             patch("openspace.recording_service.logger") as mock_logger:
             mock_instance = MagicMock()
             mock_instance.recording_status = True
             mock_instance.stop = AsyncMock(side_effect=RuntimeError("stop boom"))
             MockRM.return_value = mock_instance
 
             svc = RecordingService(config=config)
             svc.create(llm_client=mock_llm_client)
-            await svc.cleanup()  # No raise — exception logged
+            await svc.cleanup()
+            mock_logger.warning.assert_called_once()
+            assert "stop boom" in str(mock_logger.warning.call_args)
+
+    @pytest.mark.asyncio
+    async def test_cleanup_twice_only_stops_once(self, config, mock_llm_client):
+        """Double cleanup is safe — second call is noop after manager nulled."""
+        with patch("openspace.recording_service.RecordingManager") as MockRM:
+            mock_instance = MagicMock()
+            mock_instance.recording_status = True
+            mock_instance.stop = AsyncMock()
+            MockRM.return_value = mock_instance
+
+            svc = RecordingService(config=config)
+            svc.create(llm_client=mock_llm_client)
+            await svc.cleanup()
+            await svc.cleanup()  # second call — manager already None
+            mock_instance.stop.assert_awaited_once()
 
 
 # ---------------------------------------------------------------------------