fix(intelligence): replace unsafe number-grabbing fallback in _parse_importance_response

src/powermem/intelligence/importance_evaluator.py

@@ -5,9 +5,11 @@
 """
 
 import logging
+import re
 from typing import Any, Dict, Optional
-import json
+
 from ..prompts.importance_evaluation import ImportanceEvaluationPrompts
+from ..utils.utils import parse_json_from_text
 
 logger = logging.getLogger(__name__)
 
@@ -192,6 +194,13 @@ def _llm_based_evaluation(
             # Parse the response to extract importance score
             importance_score = self._parse_importance_response(response)
 
+            if importance_score is None:
+                logger.warning(
+                    "LLM response could not be parsed reliably, "
+                    "falling back to rule-based evaluation"
+                )
+                return self._rule_based_evaluation(content, metadata, context)
+
             logger.debug(f"LLM evaluated importance: {importance_score}")
 
             return importance_score
@@ -310,43 +319,105 @@ def _evaluate_factual(self, content: str) -> float:
 
         return min(score, 1.0)
 
-    def _parse_importance_response(self, response: str) -> float:
+    def _parse_importance_response(self, response: str) -> Optional[float]:
         """
         Parse LLM response to extract importance score.
-
+
+        Uses a three-level fallback strategy where each level only accepts
+        verifiable signals. Returns None when no reliable score can be
+        extracted, allowing the caller to fall back to rule-based evaluation.
+
         Args:
             response: LLM response string
-            
+
         Returns:
-            Importance score between 0 and 1
+            Importance score between 0 and 1, or None if parsing fails
         """
-        try:
-            # Try to extract JSON from response
-            if "{" in response and "}" in response:
-                start = response.find("{")
-                end = response.rfind("}") + 1
-                json_str = response[start:end]
-
-                result = json.loads(json_str)
-
-                if "importance_score" in result:
-                    score = float(result["importance_score"])
-                    # Ensure score is within valid range
-                    return max(0.0, min(1.0, score))
-
-            # Fallback: try to extract number from response
-            import re
-            numbers = re.findall(r'\d+\.?\d*', response)
-            if numbers:
-                score = float(numbers[0])
-                return max(0.0, min(1.0, score))
-
-            logger.warning(f"Could not parse importance score from response: {response}")
-            return 0.5  # Default medium importance
-
-        except Exception as e:
-            logger.error(f"Failed to parse importance response: {e}")
-            return 0.5  # Default medium importance
+        # L1: Structured JSON parsing via shared utility
+        score = self._parse_importance_from_json(response)
+        if score is not None:
+            return score
+
+        # L2: Field-name-anchored regex (only accepts numbers next to known keys)
+        score = self._parse_importance_from_field_regex(response)
+        if score is not None:
+            return score
+
+        # L3: Safe failure — return None so caller can fall back to rule-based
+        logger.warning(
+            "Could not parse importance score from LLM response, "
+            "will fall back to rule-based evaluation"
+        )
+        return None
+
+    def _parse_importance_from_json(self, response: str) -> Optional[float]:
+        """L1: Extract importance score from JSON in the response."""
+        result = parse_json_from_text(response, expected_type=dict)
+        if result is None:
+            return None
+
+        # Try primary field names: importance_score, overall_score
+        for field in ("importance_score", "overall_score"):
+            if field in result:
+                try:
+                    score = float(result[field])
+                    if 0.0 <= score <= 1.0:
+                        return score
+                    logger.warning(
+                        f"Parsed '{field}' = {score} is outside [0, 1], ignoring"
+                    )
+                except (TypeError, ValueError):
+                    pass
+
+        # Fallback: synthesize from criteria_scores using weights
+        criteria = result.get("criteria_scores")
+        if isinstance(criteria, dict) and criteria:
+            return self._synthesize_from_criteria(criteria)
+
+        return None
+
+    def _synthesize_from_criteria(self, criteria: Dict[str, Any]) -> Optional[float]:
+        """Compute weighted importance score from criteria_scores dict."""
+        weighted_sum = 0.0
+        total_weight = 0.0
+
+        for key, weight in self.criteria_weights.items():
+            raw = criteria.get(key)
+            # criteria_scores may be either flat floats or nested {"score": float}
+            if isinstance(raw, dict):
+                raw = raw.get("score")
+            if raw is None:
+                continue
+            try:
+                val = float(raw)
+                if 0.0 <= val <= 1.0:
+                    weighted_sum += val * weight
+                    total_weight += weight
+            except (TypeError, ValueError):
+                continue
+
+        if total_weight == 0.0:
+            return None
+
+        score = weighted_sum / total_weight
+        return max(0.0, min(1.0, score))
+
+    def _parse_importance_from_field_regex(self, response: str) -> Optional[float]:
+        """L2: Extract score only when anchored to a recognized field name."""
+        patterns = [
+            r'(?:importance_score|overall_score)\s*[":]\s*(\d+\.?\d*)',
+            r'(?:importance|score)\s*[:=]\s*(\d+\.?\d*)',
+        ]
+        for pattern in patterns:
+            match = re.search(pattern, response, re.IGNORECASE)
+            if match:
+                try:
+                    score = float(match.group(1))
+                    if 0.0 <= score <= 1.0:
+                        return score
+                except (ValueError, IndexError):
+                    continue
+        return None
 
     def _evaluate_personal(self, content: str, metadata: Optional[Dict[str, Any]]) -> float:
         """Evaluate if content is personal."""

tests/unit/intelligence/test_importance_evaluator.py

@@ -0,0 +1,165 @@
+"""Unit tests for ImportanceEvaluator._parse_importance_response and fallback logic."""
+
+import json
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from powermem.intelligence.importance_evaluator import ImportanceEvaluator
+
+
+@pytest.fixture
+def evaluator():
+    config = {}
+    llm_config = {}
+    ev = ImportanceEvaluator(config, llm_config)
+    return ev
+
+
+class TestParseImportanceResponse:
+    """Tests for _parse_importance_response three-level fallback."""
+
+    def test_valid_json_with_importance_score(self, evaluator):
+        response = json.dumps({
+            "importance_score": 0.75,
+            "reasoning": "High relevance",
+            "criteria_scores": {
+                "relevance": 0.8, "novelty": 0.6,
+                "emotional_impact": 0.5, "actionable": 0.7,
+                "factual": 0.6, "personal": 0.4
+            }
+        })
+        assert evaluator._parse_importance_response(response) == pytest.approx(0.75)
+
+    def test_valid_json_with_overall_score(self, evaluator):
+        """detailed_importance_breakdown template uses 'overall_score'."""
+        response = json.dumps({
+            "overall_score": 0.62,
+            "reasoning": "Moderate",
+            "criteria_scores": {}
+        })
+        assert evaluator._parse_importance_response(response) == pytest.approx(0.62)
+
+    def test_json_only_criteria_scores_synthesizes_weighted(self, evaluator):
+        """When no total score field, synthesize from criteria_scores."""
+        response = json.dumps({
+            "reasoning": "No top-level score",
+            "criteria_scores": {
+                "relevance": 0.8,
+                "novelty": 0.6,
+                "emotional_impact": 0.4,
+                "actionable": 0.5,
+                "factual": 0.7,
+                "personal": 0.3
+            }
+        })
+        # Expected: 0.8*0.3 + 0.6*0.2 + 0.4*0.15 + 0.5*0.15 + 0.7*0.1 + 0.3*0.1
+        #         = 0.24 + 0.12 + 0.06 + 0.075 + 0.07 + 0.03 = 0.595
+        result = evaluator._parse_importance_response(response)
+        assert result == pytest.approx(0.595)
+
+    def test_json_criteria_scores_nested_format(self, evaluator):
+        """criteria_scores with nested {"score": float} format."""
+        response = json.dumps({
+            "reasoning": "detailed breakdown",
+            "criteria_scores": {
+                "relevance": {"score": 0.9, "reasoning": "very relevant"},
+                "novelty": {"score": 0.5, "reasoning": "somewhat new"},
+                "emotional_impact": {"score": 0.3, "reasoning": "low"},
+                "actionable": {"score": 0.6, "reasoning": "usable"},
+                "factual": {"score": 0.8, "reasoning": "verified"},
+                "personal": {"score": 0.2, "reasoning": "general"}
+            }
+        })
+        # 0.9*0.3 + 0.5*0.2 + 0.3*0.15 + 0.6*0.15 + 0.8*0.1 + 0.2*0.1
+        # = 0.27 + 0.10 + 0.045 + 0.09 + 0.08 + 0.02 = 0.605
+        result = evaluator._parse_importance_response(response)
+        assert result == pytest.approx(0.605)
+
+    def test_malformed_json_with_field_name_regex(self, evaluator):
+        """Malformed JSON but field name pattern present — L2 extracts it."""
+        response = 'I think the importance_score: 0.72 based on analysis...'
+        assert evaluator._parse_importance_response(response) == pytest.approx(0.72)
+
+    def test_text_with_score_equals(self, evaluator):
+        """Alternative pattern: score = 0.65"""
+        response = 'After evaluation, the importance score = 0.65'
+        assert evaluator._parse_importance_response(response) == pytest.approx(0.65)
+
+    def test_rejects_out_of_range_number_in_field_regex(self, evaluator):
+        """Number > 1.0 next to field name should be rejected."""
+        response = 'importance_score: 85 (out of 100)'
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_rejects_number_slightly_above_one(self, evaluator):
+        """1.5 is outside [0, 1], should not clamp."""
+        response = json.dumps({"importance_score": 1.5})
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_reasoning_with_dimension_count_not_grabbed(self, evaluator):
+        """'6 dimensions' should not be treated as importance score."""
+        response = 'I evaluated across 6 dimensions and found moderate relevance.'
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_percentage_expression_not_grabbed(self, evaluator):
+        """'85%' in text should not be misinterpreted."""
+        response = 'The content appears to be about 85% factual with high novelty.'
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_threshold_reference_not_grabbed(self, evaluator):
+        """References to config thresholds should not be used as score."""
+        response = 'Since the short_term threshold is 0.6, this seems moderate.'
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_completely_unparseable_returns_none(self, evaluator):
+        """Gibberish text returns None for safe fallback."""
+        response = 'This content is interesting and noteworthy.'
+        assert evaluator._parse_importance_response(response) is None
+
+    def test_empty_response_returns_none(self, evaluator):
+        assert evaluator._parse_importance_response("") is None
+
+    def test_json_wrapped_in_markdown_code_block(self, evaluator):
+        """JSON inside ```json ... ``` should be parsed via parse_json_from_text."""
+        payload = json.dumps({"importance_score": 0.88, "reasoning": "critical"})
+        response = f"```json\n{payload}\n```"
+        assert evaluator._parse_importance_response(response) == pytest.approx(0.88)
+
+
+class TestLLMBasedEvaluationFallback:
+    """Tests that _llm_based_evaluation falls back correctly on parse failure."""
+
+    def test_unparseable_response_falls_back_to_rule_based(self, evaluator):
+        mock_llm = MagicMock()
+        mock_llm.is_noop = False
+        mock_llm.generate_response.return_value = "No numbers or JSON here at all."
+        evaluator.set_llm(mock_llm)
+
+        with patch.object(evaluator, '_rule_based_evaluation', return_value=0.35) as mock_rule:
+            result = evaluator._llm_based_evaluation("test content", None, None)
+            assert result == 0.35
+            mock_rule.assert_called_once_with("test content", None, None)
+
+    def test_valid_response_does_not_fallback(self, evaluator):
+        mock_llm = MagicMock()
+        mock_llm.is_noop = False
+        mock_llm.generate_response.return_value = json.dumps({
+            "importance_score": 0.9, "reasoning": "critical"
+        })
+        evaluator.set_llm(mock_llm)
+
+        with patch.object(evaluator, '_rule_based_evaluation') as mock_rule:
+            result = evaluator._llm_based_evaluation("test content", None, None)
+            assert result == pytest.approx(0.9)
+            mock_rule.assert_not_called()
+
+    def test_llm_exception_falls_back_to_rule_based(self, evaluator):
+        mock_llm = MagicMock()
+        mock_llm.is_noop = False
+        mock_llm.generate_response.side_effect = RuntimeError("API timeout")
+        evaluator.set_llm(mock_llm)
+
+        with patch.object(evaluator, '_rule_based_evaluation', return_value=0.4) as mock_rule:
+            result = evaluator._llm_based_evaluation("test content", None, None)
+            assert result == 0.4
+            mock_rule.assert_called_once()