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

Fixes from 5 reviewers (security, implementer, test-writer, GPT-5.4, Opus):

CRITICAL:
- C-1: Initialize execution_time=0.0 before try (NameError on CancelledError)
- C-2: Copy caller context dict (dict(context)) to prevent mutation

HIGH:
- H-1: Propagate cleanup() to engine state (_running, _task_done)
- H-2: Clear active_skills on fallback path, add attempted_skills key
- H-3: Remove dead fields from OpenSpace (_execution_count, _last_evolved_skills, _task_done)

MEDIUM:
- M-2: __repr__ uses is_running() instead of stale _running field
- M-3: Replace deprecated get_event_loop() with get_running_loop()
- MED-2: Log save_execution_outcome failures instead of silent swallow

LOW:
- LOW-3: Remove dead 'import traceback' from tool_layer.py

TESTS: 20 -> 31 tests
- _resolve_workspace: 4 branches (explicit, config, recording, tempdir)
- _cleanup_workspace: file removal + empty-path noop
- Concurrency: exception resets _task_done, concurrent timeout
- Quality triggers: metric check every 5, quality evolution
- Analysis evolution: candidate_for_evolution triggers skill evolution

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

Author: Brian Krafft

openspace/execution_engine.py

@@ -147,16 +147,17 @@ async def execute(
         self._running = True
         self._task_done.clear()
         self._last_evolved_skills = []
-        start_time = asyncio.get_event_loop().time()
+        start_time = asyncio.get_running_loop().time()
 
         if task_id is None:
             task_id = f"task_{uuid.uuid4().hex[:12]}"
         logger.info(f"Task ID: {task_id}")
 
         result: Dict[str, Any] = {}
+        execution_time = 0.0
 
         try:
-            execution_context = context or {}
+            execution_context = dict(context) if context else {}
             execution_context["task_id"] = task_id
             execution_context["instruction"] = task
 
@@ -217,11 +218,11 @@ async def execute(
                 )
                 result = await self._grounding_agent.process(execution_context)
 
-            execution_time = asyncio.get_event_loop().time() - start_time
+            execution_time = asyncio.get_running_loop().time() - start_time
             self._log_result(result, execution_time)
 
         except Exception as e:
-            execution_time = asyncio.get_event_loop().time() - start_time
+            execution_time = asyncio.get_running_loop().time() - start_time
             tb = traceback.format_exc(limit=10)
             logger.error(f"Task execution failed: {e}", exc_info=True)
 
@@ -245,14 +246,14 @@ async def execute(
                 recording_dir = self._recording_manager.trajectory_dir
 
                 try:
-                    exec_time = asyncio.get_event_loop().time() - start_time
+                    exec_time = asyncio.get_running_loop().time() - start_time
                     await self._recording_manager.save_execution_outcome(
                         status=result.get("status", "unknown"),
                         iterations=result.get("iterations", 0),
                         execution_time=exec_time,
                     )
-                except Exception:
-                    pass
+                except Exception as e:
+                    logger.warning("Failed to save execution outcome: %s", e)
 
                 try:
                     await self._recording_manager.stop()
@@ -335,7 +336,8 @@ async def _execute_skill_first(
         execution_context_p2["max_iterations"] = max_iterations
 
         result = await self._grounding_agent.process(execution_context_p2)
-        result["active_skills"] = injected_skill_ids
+        result["active_skills"] = []
+        result["attempted_skills"] = injected_skill_ids
         logger.info(
             f"[Phase 2 — Fallback] {result.get('status', 'unknown')} "
             f"({result.get('iterations', 0)} iterations)"

openspace/tool_layer.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import asyncio
-import traceback
 import uuid
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -133,13 +132,8 @@ def __init__(
         self._skill_store: Optional[SkillStore] = None
         self._execution_analyzer: Optional[ExecutionAnalyzer] = None
         self._skill_evolver: Optional[SkillEvolver] = None
-        self._execution_count: int = 0  # For periodic metric-based evolution
-        self._last_evolved_skills: List[Dict[str, Any]] = []  # Tracks skills evolved during last execute()
-
         self._initialized = False
-        self._running = False
-        self._task_done = asyncio.Event()
-        self._task_done.set()  # Initially not running, so "done"
+        self._running = False  # Fallback for pre-init; delegates to engine after init
 
         logger.debug("OpenSpace instance created")
 
@@ -477,7 +471,9 @@ async def cleanup(self) -> None:
 
             self._initialized = False
             self._running = False
-            self._task_done.set()
+            if self._execution_engine:
+                self._execution_engine._running = False
+                self._execution_engine._task_done.set()
 
             logger.info("OpenSpace cleanup complete")
 
@@ -517,7 +513,7 @@ async def __aexit__(self, exc_type, exc_val, exc_tb):
 
     def __repr__(self) -> str:
         status = "initialized" if self._initialized else "not initialized"
-        if self._running:
+        if self.is_running():
             status = "running"
         backends = ", ".join(self.config.backend_scope) if self.config.backend_scope else "all"
         return f"<OpenSpace(status={status}, backends={backends}, model={self.config.llm_model})>"

tests/test_execution_engine.py

@@ -320,6 +320,159 @@ async def test_no_quality_manager_is_noop(self, engine):
         """No-op when quality_manager is None."""
         await engine._maybe_evolve_quality()  # Should not raise
 
+    @pytest.mark.asyncio
+    async def test_metric_check_on_every_5(self, engine):
+        """Trigger 3 fires every 5 executions."""
+        evolver = MagicMock()
+        evolver.schedule_background = MagicMock()
+        evolver.process_metric_check = MagicMock(return_value=AsyncMock())
+        evolver.set_available_tools = MagicMock()
+        engine._skill_evolver = evolver
+        engine._execution_count = 4  # next call → 5
+
+        await engine._maybe_evolve_quality()
+        evolver.schedule_background.assert_called_once()
+
+    @pytest.mark.asyncio
+    async def test_quality_evolution_trigger(self, engine, mock_grounding_client):
+        """Trigger 2 fires when quality manager detects problems."""
+        qm = MagicMock()
+        qm.should_evolve = MagicMock(return_value=True)
+        qm.get_problematic_tools = MagicMock(return_value=["tool_a"])
+        mock_grounding_client.quality_manager = qm
+        mock_grounding_client.evolve_quality = AsyncMock(return_value={"recommendations": ["r1"]})
+
+        evolver = MagicMock()
+        evolver.set_available_tools = MagicMock()
+        evolver.schedule_background = MagicMock()
+        evolver.process_tool_degradation = MagicMock(return_value=AsyncMock())
+        engine._skill_evolver = evolver
+
+        await engine._maybe_evolve_quality()
+        mock_grounding_client.evolve_quality.assert_awaited_once()
+        evolver.schedule_background.assert_called_once()
+
+
+# ---------------------------------------------------------------------------
+# _resolve_workspace
+# ---------------------------------------------------------------------------
+
+class TestResolveWorkspace:
+
+    def test_explicit_workspace_dir(self, engine):
+        """Explicit workspace_dir argument wins."""
+        ctx = {}
+        engine._resolve_workspace(ctx, "/explicit/dir", "t1")
+        assert ctx["workspace_dir"] == "/explicit/dir"
+
+    def test_config_workspace_dir(self, engine, mock_config):
+        """Falls back to config.workspace_dir."""
+        mock_config.workspace_dir = "/config/ws"
+        ctx = {}
+        engine._resolve_workspace(ctx, None, "t1")
+        assert ctx["workspace_dir"] == "/config/ws"
+
+    def test_recording_manager_trajectory_dir(self, engine, mock_recording_manager, mock_config):
+        """Falls back to recording_manager.trajectory_dir."""
+        mock_config.workspace_dir = None
+        engine._recording_manager = mock_recording_manager
+        mock_recording_manager.trajectory_dir = "/traj/dir"
+        ctx = {}
+        engine._resolve_workspace(ctx, None, "t1")
+        assert ctx["workspace_dir"] == "/traj/dir"
+
+    def test_tempdir_fallback(self, engine, mock_config):
+        """Creates temp directory when nothing else available."""
+        mock_config.workspace_dir = None
+        ctx = {}
+        engine._resolve_workspace(ctx, None, "task_abc")
+        assert "openspace_workspace" in ctx["workspace_dir"]
+        assert "task_abc" in ctx["workspace_dir"]
+
+
+# ---------------------------------------------------------------------------
+# _cleanup_workspace
+# ---------------------------------------------------------------------------
+
+class TestCleanupWorkspace:
+
+    def test_removes_new_files_preserves_old(self, tmp_path):
+        """Removes files not in pre_skill_files set, preserves originals."""
+        from openspace.execution_engine import ExecutionEngine
+
+        (tmp_path / "existing.txt").write_text("keep me")
+        (tmp_path / "new_file.txt").write_text("remove me")
+        new_dir = tmp_path / "new_dir"
+        new_dir.mkdir()
+
+        ExecutionEngine._cleanup_workspace(str(tmp_path), {"existing.txt"})
+        assert (tmp_path / "existing.txt").exists()
+        assert not (tmp_path / "new_file.txt").exists()
+        assert not new_dir.exists()
+
+    def test_empty_path_is_noop(self):
+        """Empty workspace path does nothing."""
+        from openspace.execution_engine import ExecutionEngine
+        ExecutionEngine._cleanup_workspace("", set())  # no raise
+
+
+# ---------------------------------------------------------------------------
+# Concurrency guards
+# ---------------------------------------------------------------------------
+
+class TestConcurrencyGuards:
+
+    @pytest.mark.asyncio
+    async def test_exception_resets_task_done(self, engine, mock_grounding_agent):
+        """_task_done event is re-set after exception (prevents deadlock)."""
+        mock_grounding_agent.process = AsyncMock(side_effect=RuntimeError("boom"))
+        await engine.execute("task")
+        assert engine._task_done.is_set()
+        assert engine.is_running is False
+
+    @pytest.mark.asyncio
+    async def test_concurrent_execute_raises_on_timeout(self, engine):
+        """Raises RuntimeError if busy-wait exceeds timeout."""
+        engine._running = True
+        engine._task_done.clear()
+        with patch("openspace.execution_engine.asyncio.wait_for", side_effect=asyncio.TimeoutError):
+            with pytest.raises(RuntimeError, match="still running"):
+                await engine.execute("task")
+
+
+# ---------------------------------------------------------------------------
+# _maybe_analyze_execution — evolution path
+# ---------------------------------------------------------------------------
+
+class TestAnalyzeExecutionEvolution:
+
+    @pytest.mark.asyncio
+    async def test_triggers_evolution(self, engine):
+        """Skills are evolved when analysis has candidate_for_evolution=True."""
+        analyzer = MagicMock()
+        analysis = MagicMock()
+        analysis.candidate_for_evolution = True
+        analysis.evolution_suggestions = [MagicMock()]
+        analyzer.analyze_execution = AsyncMock(return_value=analysis)
+
+        evolved_rec = MagicMock(
+            skill_id="s1", name="Skill1", description="desc", path=None,
+            lineage=MagicMock(
+                origin=MagicMock(value="synthesized"),
+                generation=1, parent_skill_ids=[], change_summary="init"
+            ),
+        )
+        evolver = MagicMock()
+        evolver.process_analysis = AsyncMock(return_value=[evolved_rec])
+        evolver.set_available_tools = MagicMock()
+
+        engine._execution_analyzer = analyzer
+        engine._skill_evolver = evolver
+
+        await engine._maybe_analyze_execution("t1", "/dir", {"status": "ok"})
+        assert len(engine._last_evolved_skills) == 1
+        assert engine._last_evolved_skills[0]["skill_id"] == "s1"
+
 
 # ---------------------------------------------------------------------------
 # OpenSpace backward compatibility