fix(5.4): fix parse negation bugs, harden all recording paths

Round 2+3 fixes:
- BUG FIX: parse_confirmation now handles negation patterns (not/never/don't)
  'do not confirm', 'never confirm', 'don't proceed' correctly return False
- BUG FIX: JSON string 'false' no longer truthy (strict bool/string check)
- BUG FIX: Negative keywords checked FIRST (conservative fail-safe)
- SEC: All 4 recording paths in evolver.py now truncate to 2000 chars
- SEC: copy.deepcopy removed from all recording paths
- OBS: skill_id added to iteration response_metadata for correlation
- OBS: Logger namespace preserved (evolver) for filter compatibility

44 confirmation tests; 1,514 total pass, 127 skipped.

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

Author: Brian Krafft

openspace/skill_engine/evolution/confirmation.py

@@ -96,7 +96,7 @@ async def llm_confirm_evolution(
         content = result["message"].get("content", "").strip().lower()
         confirmed = evolver._parse_confirmation(content)
 
-        # Record truncated confirmation response
+        # Record truncated confirmation response (include skill_id for correlation)
         await RecordingManager.record_iteration_context(
             iteration=1,
             delta_messages=[
@@ -105,6 +105,7 @@ async def llm_confirm_evolution(
             response_metadata={
                 "has_tool_calls": False,
                 "confirmed": confirmed,
+                "skill_id": skill_record.skill_id,
             },
             agent_name="SkillEvolver.confirm",
         )
@@ -124,7 +125,7 @@ def parse_confirmation(response: str) -> bool:
 
     Parsing order:
     1. JSON ``{"proceed": true/false}`` (with markdown-fence stripping)
-    2. Keyword matching (yes/confirm → True, no/reject/skip → False)
+    2. Keyword matching — negatives checked FIRST (conservative: err toward skip)
     3. Default: False (ambiguous → skip costly evolution)
     """
     # Try JSON parse first
@@ -135,30 +136,42 @@ def parse_confirmation(response: str) -> bool:
             cleaned = re.sub(r"\n?```\s*$", "", cleaned)
         data = json.loads(cleaned)
         if isinstance(data, dict):
-            return bool(data.get("proceed", False))
+            proceed = data.get("proceed", False)
+            # Strict boolean check — "false" (string) must not be truthy
+            if isinstance(proceed, bool):
+                return proceed
+            if isinstance(proceed, (int, float)):
+                return bool(proceed)
+            # String values: explicit true/false
+            return str(proceed).lower() in ("true", "1", "yes")
     except (json.JSONDecodeError, ValueError):
         pass
 
     # Fallback: keyword matching.
-    # - yes/no use strict word boundaries to avoid false positives
-    #   (e.g. "know" matching "no").
-    # - confirm/reject/skip use stem-style matching so that common
-    #   LLM variants like "confirmed", "rejected", "skipping" still
-    #   parse correctly.
+    # Negatives checked FIRST — conservative (err toward skipping).
+    # yes/no use strict word boundaries to avoid false positives
+    # (e.g. "know" matching "no").
+    # confirm/reject/skip use stem-style matching so that common
+    # LLM variants like "confirmed", "rejected", "skipping" still
+    # parse correctly.
+    # Negation words (not/never/don't) catch "do not confirm" patterns.
     _wb = re.search  # shorthand
-    if (
-        any(w in response for w in ('"proceed": true', "proceed: true"))
-        or _wb(r"\byes\b", response)
-        or _wb(r"\bconfirm\w*\b", response)
-    ):
-        return True
     if (
         any(w in response for w in ('"proceed": false', "proceed: false"))
         or _wb(r"\bno\b", response)
+        or _wb(r"\bnot\b", response)
+        or _wb(r"\bnever\b", response)
+        or _wb(r"\bdon'?t\b", response)
         or _wb(r"\breject\w*\b", response)
         or _wb(r"\bskip\w*\b", response)
     ):
         return False
+    if (
+        any(w in response for w in ('"proceed": true', "proceed: true"))
+        or _wb(r"\byes\b", response)
+        or _wb(r"\bconfirm\w*\b", response)
+    ):
+        return True
 
     # Default: skip — ambiguous response should not trigger costly evolution
     logger.debug("LLM confirmation response was ambiguous, defaulting to skip")

openspace/skill_engine/evolver.py

@@ -40,6 +40,7 @@
     schedule_background as _schedule_background_impl,
 )
 from .evolution.confirmation import (
+    _RECORDING_MAX_CHARS,
     _SKILL_CONTENT_MAX_CHARS,
     llm_confirm_evolution as _llm_confirm_evolution_impl,
     parse_confirmation as _parse_confirmation_impl,
@@ -640,9 +641,13 @@ async def _run_evolution_loop(
             {"role": "user", "content": prompt},
         ]
 
-        # Record initial conversation setup
+        # Record initial conversation setup (truncated for data minimization)
+        recorded_setup = [
+            {"role": m["role"], "content": _truncate(m["content"], _RECORDING_MAX_CHARS)}
+            for m in messages
+        ]
         await RecordingManager.record_conversation_setup(
-            setup_messages=copy.deepcopy(messages),
+            setup_messages=recorded_setup,
             tools=evolution_tools if evolution_tools else None,
             agent_name="SkillEvolver",
             extra={
@@ -691,11 +696,15 @@ async def _run_evolution_loop(
             updated_messages = result["messages"]
             has_tool_calls = result.get("has_tool_calls", False)
 
-            # Record iteration delta
+            # Record iteration delta (truncated for data minimization)
             delta = updated_messages[msg_count_before:]
+            recorded_delta = [
+                {"role": m["role"], "content": _truncate(m.get("content", ""), _RECORDING_MAX_CHARS)}
+                for m in delta
+            ]
             await RecordingManager.record_iteration_context(
                 iteration=iteration + 1,
-                delta_messages=copy.deepcopy(delta),
+                delta_messages=recorded_delta,
                 response_metadata={
                     "has_tool_calls": has_tool_calls,
                     "tool_calls_count": len(result.get("tool_results", [])),
@@ -866,8 +875,12 @@ async def _apply_with_retry(
 
             # Record retry setup on first retry attempt
             if not retry_setup_recorded:
+                recorded_retry = [
+                    {"role": m["role"], "content": _truncate(m.get("content", ""), _RECORDING_MAX_CHARS)}
+                    for m in msg_history
+                ]
                 await RecordingManager.record_conversation_setup(
-                    setup_messages=copy.deepcopy(msg_history),
+                    setup_messages=recorded_retry,
                     agent_name="SkillEvolver.retry",
                     extra={
                         "evolution_type": ctx.suggestion.evolution_type.value,
@@ -913,8 +926,8 @@ async def _apply_with_retry(
                 await RecordingManager.record_iteration_context(
                     iteration=attempt + 1,
                     delta_messages=[
-                        {"role": "user", "content": retry_prompt},
-                        {"role": "assistant", "content": new_content},
+                        {"role": "user", "content": _truncate(retry_prompt, _RECORDING_MAX_CHARS)},
+                        {"role": "assistant", "content": _truncate(new_content, _RECORDING_MAX_CHARS)},
                     ],
                     response_metadata={
                         "has_tool_calls": False,

tests/test_evolution_confirmation.py

@@ -43,7 +43,15 @@ def test_json_with_markdown_fences(self):
     def test_json_missing_proceed_key(self):
         assert parse_confirmation('{"analysis": "looks good"}') is False
 
-    def test_json_proceed_zero_is_false(self):
+    def test_json_proceed_string_false_is_false(self):
+        """String 'false' must NOT be truthy — was a real bug."""
+        assert parse_confirmation('{"proceed": "false"}') is False
+
+    def test_json_proceed_string_true_is_true(self):
+        assert parse_confirmation('{"proceed": "true"}') is True
+
+    def test_json_proceed_string_no_is_false(self):
+        assert parse_confirmation('{"proceed": "no"}') is False
         assert parse_confirmation('{"proceed": 0}') is False
 
     def test_json_proceed_one_is_true(self):
@@ -93,10 +101,34 @@ def test_case_sensitivity(self):
         # Uppercase does NOT match \byes\b — by design, caller lowercases
         assert parse_confirmation("YES") is False
 
-    def test_conflicting_keywords_first_wins(self):
-        """When both yes and no keywords appear, positive match wins
-        because the positive branch is checked first."""
-        assert parse_confirmation("yes but also no") is True
+    def test_conflicting_keywords_negative_wins(self):
+        """When both yes and no keywords appear, negative wins (conservative —
+        err toward skipping costly evolution)."""
+        assert parse_confirmation("yes but also no") is False
+
+    def test_not_confirmed_returns_false(self):
+        """'not confirmed' should NOT trigger positive confirm match."""
+        assert parse_confirmation("not confirmed, skip this") is False
+
+    def test_negated_yes_returns_false(self):
+        """'no, I would not say yes' — 'no' wins."""
+        assert parse_confirmation("no, I would not say yes to this") is False
+
+    def test_do_not_confirm(self):
+        """'do not confirm' — 'not' is a negation keyword."""
+        assert parse_confirmation("do not confirm this evolution") is False
+
+    def test_never_confirm(self):
+        """'never confirm' — 'never' is a negation keyword."""
+        assert parse_confirmation("never confirm; leave the skill unchanged") is False
+
+    def test_dont_proceed(self):
+        """'don't proceed' — contraction negation."""
+        assert parse_confirmation("don't proceed with this change") is False
+
+    def test_negation_with_json_example(self):
+        """Negation in prose even with JSON example should fail-safe."""
+        assert parse_confirmation('do not proceed. Example JSON: {"proceed": true}') is False
 
     def test_json_array_falls_through_to_keywords(self):
         """JSON array is not a dict — falls through to keyword matching.