fix(5.3): preserve MRO for helper calls, add happy-path tests

Review fixes from /8eyes + /collab gate:
- Route helper calls through evolver._method() to preserve MRO (GPT-5.4 MEDIUM)
- Add 2 happy-path tests: confirm->execute for triggers 2 & 3 (Opus M-2)
- Remove unused import logging (Opus L-1)
- Move _ANALYSIS_NOTE_MAX_CHARS to evolver.py where used (Opus L-2)
- Restore diagnose_skill_health return type annotation (Opus L-3)

24 trigger tests pass; 1,470 total pass, 127 skipped.

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

Author: Brian Krafft

openspace/skill_engine/evolution/triggers.py

@@ -11,7 +11,6 @@
 
 from __future__ import annotations
 
-import logging
 from pathlib import Path
 from typing import TYPE_CHECKING, List, Optional
 
@@ -35,7 +34,6 @@
 # ---------------------------------------------------------------------------
 
 _ANALYSIS_CONTEXT_MAX = 5  # Max recent analyses to include in prompt
-_ANALYSIS_NOTE_MAX_CHARS = 500  # Per-analysis note truncation
 
 # Rule-based thresholds for candidate screening (relaxed — LLM confirms)
 _FALLBACK_THRESHOLD = 0.4
@@ -64,7 +62,7 @@ async def process_analysis(
 
     contexts: List[EvolutionContext] = []
     for suggestion in analysis.evolution_suggestions:
-        ctx = build_context_from_analysis(evolver, analysis, suggestion)
+        ctx = evolver._build_context_from_analysis(analysis, suggestion)
         if ctx is not None:
             contexts.append(ctx)
 
@@ -137,7 +135,7 @@ async def process_tool_degradation(
             recent = evolver._store.load_analyses(
                 skill_id=skill_record.skill_id, limit=_ANALYSIS_CONTEXT_MAX,
             )
-            content = load_skill_content(evolver, skill_record)
+            content = evolver._load_skill_content(skill_record)
             if not content:
                 continue
 
@@ -225,11 +223,11 @@ async def process_metric_check(
         if record.total_selections < min_selections:
             continue
 
-        evo_type, direction = diagnose_skill_health(record)
+        evo_type, direction = evolver._diagnose_skill_health(record)
         if evo_type is None:
             continue
 
-        content = load_skill_content(evolver, record)
+        content = evolver._load_skill_content(record)
         if not content:
             continue
 
@@ -313,7 +311,7 @@ def build_context_from_analysis(
             if not rec:
                 logger.warning("Target skill not found: %s", target_id)
                 return None
-            content = load_skill_content(evolver, rec)
+            content = evolver._load_skill_content(rec)
             if not content:
                 logger.warning("Cannot load content for skill: %s", target_id)
                 return None
@@ -360,7 +358,7 @@ def load_skill_content(evolver, record: "SkillRecord") -> str:
 
 def diagnose_skill_health(
     record: "SkillRecord",
-) -> tuple:
+) -> tuple[Optional[EvolutionType], str]:
     """Diagnose what type of evolution a skill needs based on metrics.
 
     Returns ``(EvolutionType, direction_str)`` or ``(None, "")`` if healthy.

openspace/skill_engine/evolver.py

@@ -41,7 +41,6 @@
 )
 from .evolution.triggers import (
     _ANALYSIS_CONTEXT_MAX,
-    _ANALYSIS_NOTE_MAX_CHARS,
     build_context_from_analysis as _build_context_from_analysis_impl,
     diagnose_skill_health as _diagnose_skill_health_impl,
     load_skill_content as _load_skill_content_impl,
@@ -1055,6 +1054,8 @@ def _format_skill_dir_content(skill_dir: Path) -> str:
     @staticmethod
     def _format_analysis_context(analyses: List[ExecutionAnalysis]) -> str:
         """Format recent analyses into a concise context block for prompts."""
+        _ANALYSIS_NOTE_MAX_CHARS = 500  # Per-analysis note truncation
+
         if not analyses:
             return "(no execution history available)"
 

tests/test_evolution_triggers.py

@@ -79,7 +79,6 @@ class _FakeToolQualityRecord:
 
 from openspace.skill_engine.evolution.triggers import (
     _ANALYSIS_CONTEXT_MAX,
-    _ANALYSIS_NOTE_MAX_CHARS,
     _FALLBACK_THRESHOLD,
     _HIGH_APPLIED_FOR_FIX,
     _LOW_COMPLETION_THRESHOLD,
@@ -208,7 +207,7 @@ def test_fix_loads_single_target(self):
         evolver = MagicMock()
         evolver._available_tools = []
         evolver._store.load_record.return_value = _FakeSkillRecord(path="/skills/s1/SKILL.md")
-        evolver._registry.load_skill_content.return_value = "# Content"
+        evolver._load_skill_content.return_value = "# Content"
 
         analysis = _FakeAnalysis()
         suggestion = _FakeSuggestion(
@@ -241,8 +240,8 @@ async def test_no_candidates_returns_empty(self):
     async def test_suggestions_dispatched(self):
         evolver = MagicMock()
         evolver._available_tools = []
-        evolver._store.load_record.return_value = _FakeSkillRecord(path="/s/SKILL.md")
-        evolver._registry.load_skill_content.return_value = "# Content"
+        # After MRO fix: process_analysis calls evolver._build_context_from_analysis
+        evolver._build_context_from_analysis.return_value = MagicMock()  # truthy context
         evolver._execute_contexts = AsyncMock(return_value=[_FakeSkillRecord(name="evolved")])
 
         analysis = _FakeAnalysis(
@@ -251,9 +250,7 @@ async def test_suggestions_dispatched(self):
             ],
         )
 
-        with patch("openspace.skill_engine.evolution.triggers.EvolutionType", _FakeEvolutionType), \
-             patch("openspace.skill_engine.evolution.triggers.EvolutionTrigger", _FakeTrigger):
-            result = await process_analysis(evolver, analysis)
+        result = await process_analysis(evolver, analysis)
 
         assert len(result) == 1
         evolver._execute_contexts.assert_awaited_once()
@@ -300,6 +297,28 @@ async def test_prunes_recovered_tools(self):
         assert "old_tool" not in evolver._addressed_degradations
 
 
+    @pytest.mark.asyncio
+    async def test_confirmed_skill_triggers_execution(self):
+        """Happy path: LLM confirms → context built → _execute_contexts fires."""
+        evolver = MagicMock()
+        evolver._addressed_degradations = {}
+        evolver._store.find_skills_by_tool.return_value = ["skill-1"]
+        evolver._store.load_record.return_value = _FakeSkillRecord(path="/s/SKILL.md")
+        evolver._load_skill_content.return_value = "# Content"
+        evolver._store.load_analyses.return_value = []
+        evolver._llm_confirm_evolution = AsyncMock(return_value=True)
+        evolver._execute_contexts = AsyncMock(return_value=[_FakeSkillRecord(name="fixed")])
+        evolver._available_tools = []
+
+        tool = _FakeToolQualityRecord(tool_key="shell_exec")
+        result = await process_tool_degradation(evolver, [tool])
+
+        assert len(result) == 1
+        assert result[0].name == "fixed"
+        evolver._llm_confirm_evolution.assert_awaited_once()
+        evolver._execute_contexts.assert_awaited_once()
+
+
 # ---------------------------------------------------------------------------
 # process_metric_check tests
 # ---------------------------------------------------------------------------
@@ -327,12 +346,37 @@ async def test_healthy_skills_skipped(self):
             ),
         }
         evolver._available_tools = []
+        # After MRO fix: process_metric_check calls evolver._diagnose_skill_health
+        evolver._diagnose_skill_health.return_value = (None, "")
 
-        with patch("openspace.skill_engine.evolution.triggers.EvolutionType", _FakeEvolutionType):
-            result = await process_metric_check(evolver, min_selections=5)
+        result = await process_metric_check(evolver, min_selections=5)
 
         assert result == []
 
+    @pytest.mark.asyncio
+    async def test_confirmed_metric_triggers_execution(self):
+        """Happy path: diagnose → LLM confirm → _execute_contexts fires."""
+        evolver = MagicMock()
+        evolver._store.load_active.return_value = {
+            "s1": _FakeSkillRecord(total_selections=10),
+        }
+        evolver._available_tools = []
+        evolver._diagnose_skill_health.return_value = (_FakeEvolutionType.FIX, "needs fix")
+        evolver._load_skill_content.return_value = "# Skill Content"
+        evolver._store.load_analyses.return_value = []
+        evolver._llm_confirm_evolution = AsyncMock(return_value=True)
+        evolver._execute_contexts = AsyncMock(return_value=[_FakeSkillRecord(name="improved")])
+
+        with patch("openspace.skill_engine.evolution.triggers.EvolutionType", _FakeEvolutionType), \
+             patch("openspace.skill_engine.evolution.triggers.EvolutionTrigger", _FakeTrigger), \
+             patch("openspace.skill_engine.evolution.triggers.EvolutionSuggestion", _FakeSuggestion):
+            result = await process_metric_check(evolver, min_selections=5)
+
+        assert len(result) == 1
+        assert result[0].name == "improved"
+        evolver._llm_confirm_evolution.assert_awaited_once()
+        evolver._execute_contexts.assert_awaited_once()
+
 
 # ---------------------------------------------------------------------------
 # Constants integrity
@@ -348,7 +392,6 @@ def test_thresholds_are_numbers(self):
 
     def test_analysis_constants(self):
         assert isinstance(_ANALYSIS_CONTEXT_MAX, int)
-        assert isinstance(_ANALYSIS_NOTE_MAX_CHARS, int)
 
     def test_constants_no_longer_in_evolver(self):
         """Constants should be imported, not defined, in evolver.py."""