fix(3.1): address /8eyes findings — defaults, validation, type coercion, recursion

- F1: SkillVersion default minor=0 (was 1), from_dict default minor=0
- F2: Store._upsert() and _insert_analysis() call validate() before SQL
- F3: Type coercion (int/str) in from_dict for SkillVersion, SkillRecord,
  ExecutionAnalysis with TypeError/ValueError → ValidationError
- F4: Recursive validation: ExecutionAnalysis validates judgments/suggestions,
  SkillRecord guards lineage None check
- F5: 16 hostile-input tests (wrong types, partial payloads, adversarial values)

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

Author: Brian Krafft

openspace/skill_engine/store.py

@@ -35,6 +35,7 @@
     SkillOrigin,
     SkillRecord,
     SkillVisibility,
+    ValidationError,
 )
 
 logger = Logger.get_logger(__name__)
@@ -1089,6 +1090,12 @@ def _upsert(self, record: SkillRecord) -> None:
 
         Called within a transaction holding ``self._mu``.
         """
+        try:
+            record.validate()
+        except ValidationError as exc:
+            raise ValidationError(
+                f"Cannot persist invalid SkillRecord '{record.skill_id}': {exc}"
+            ) from exc
         lin = record.lineage
         # content_snapshot is Dict[str, str]; store as JSON text
         snapshot_json = json.dumps(lin.content_snapshot, ensure_ascii=False)
@@ -1204,6 +1211,12 @@ def _insert_analysis(self, a: ExecutionAnalysis) -> int:
         Returns:
             int: The ``execution_analyses.id`` of the newly inserted row.
         """
+        try:
+            a.validate()
+        except ValidationError as exc:
+            raise ValidationError(
+                f"Cannot persist invalid ExecutionAnalysis '{a.task_id}': {exc}"
+            ) from exc
         cur = self._conn.execute(
             """
             INSERT INTO execution_analyses (

openspace/skill_engine/types.py

@@ -30,7 +30,7 @@ class SkillVersion:
     """
 
     major: int = 0
-    minor: int = 1
+    minor: int = 0
     patch: int = 0
 
     def __post_init__(self) -> None:
@@ -94,11 +94,14 @@ def to_dict(self) -> Dict[str, int]:
 
     @classmethod
     def from_dict(cls, data: Dict[str, Any]) -> "SkillVersion":
-        return cls(
-            major=data.get("major", 0),
-            minor=data.get("minor", 1),
-            patch=data.get("patch", 0),
-        )
+        try:
+            return cls(
+                major=int(data.get("major", 0)),
+                minor=int(data.get("minor", 0)),
+                patch=int(data.get("patch", 0)),
+            )
+        except (TypeError, ValueError) as exc:
+            raise ValidationError(f"Invalid SkillVersion data: {exc}") from exc
 
 
 class SkillCategory(str, Enum):
@@ -428,9 +431,13 @@ def suggestions_by_type(self, evo_type: EvolutionType) -> List[EvolutionSuggesti
         return [s for s in self.evolution_suggestions if s.evolution_type == evo_type]
 
     def validate(self) -> None:
-        """Validate analysis fields."""
+        """Validate analysis fields and nested objects."""
         if not self.task_id:
             raise ValidationError("task_id must be non-empty")
+        for j in self.skill_judgments:
+            j.validate()
+        for s in self.evolution_suggestions:
+            s.validate()
 
     def to_dict(self) -> Dict[str, Any]:
         return {
@@ -447,17 +454,20 @@ def to_dict(self) -> Dict[str, Any]:
 
     @classmethod
     def from_dict(cls, data: Dict[str, Any]) -> "ExecutionAnalysis":
-        return cls(
-            task_id=data["task_id"],
-            timestamp=datetime.fromisoformat(data["timestamp"]),
-            task_completed=data.get("task_completed", False),
-            execution_note=data.get("execution_note", ""),
-            tool_issues=data.get("tool_issues", []),
-            skill_judgments=[SkillJudgment.from_dict(j) for j in data.get("skill_judgments", [])],
-            evolution_suggestions=[EvolutionSuggestion.from_dict(s) for s in data.get("evolution_suggestions", [])],
-            analyzed_by=data.get("analyzed_by", ""),
-            analyzed_at=(datetime.fromisoformat(data["analyzed_at"]) if data.get("analyzed_at") else datetime.now()),
-        )
+        try:
+            return cls(
+                task_id=str(data["task_id"]),
+                timestamp=datetime.fromisoformat(data["timestamp"]),
+                task_completed=data.get("task_completed", False),
+                execution_note=data.get("execution_note", ""),
+                tool_issues=data.get("tool_issues", []),
+                skill_judgments=[SkillJudgment.from_dict(j) for j in data.get("skill_judgments", [])],
+                evolution_suggestions=[EvolutionSuggestion.from_dict(s) for s in data.get("evolution_suggestions", [])],
+                analyzed_by=data.get("analyzed_by", ""),
+                analyzed_at=(datetime.fromisoformat(data["analyzed_at"]) if data.get("analyzed_at") else datetime.now()),
+            )
+        except (TypeError, ValueError) as exc:
+            raise ValidationError(f"Invalid ExecutionAnalysis data: {exc}") from exc
 
 
 # Full skill profile (identity + lineage + deps + quality)
@@ -555,7 +565,8 @@ def validate(self) -> None:
                 f"total_completions ({self.total_completions}) must not exceed "
                 f"total_applied ({self.total_applied})"
             )
-        self.lineage.validate()
+        if self.lineage is not None:
+            self.lineage.validate()
 
     def to_dict(self) -> Dict[str, Any]:
         return {
@@ -582,30 +593,33 @@ def to_dict(self) -> Dict[str, Any]:
 
     @classmethod
     def from_dict(cls, data: Dict[str, Any]) -> "SkillRecord":
-        record = cls(
-            skill_id=data["skill_id"],
-            name=data["name"],
-            description=data.get("description", ""),
-            path=data.get("path", ""),
-            is_active=data.get("is_active", True),
-            category=SkillCategory(data["category"]) if data.get("category") else SkillCategory.WORKFLOW,
-            tags=data.get("tags", []),
-            visibility=(SkillVisibility(data["visibility"]) if data.get("visibility") else SkillVisibility.PRIVATE),
-            creator_id=data.get("creator_id", ""),
-            lineage=(
-                SkillLineage.from_dict(data["lineage"])
-                if data.get("lineage")
-                else SkillLineage(origin=SkillOrigin.IMPORTED)
-            ),
-            tool_dependencies=data.get("tool_dependencies", []),
-            critical_tools=data.get("critical_tools", []),
-            total_selections=data.get("total_selections", 0),
-            total_applied=data.get("total_applied", 0),
-            total_completions=data.get("total_completions", 0),
-            total_fallbacks=data.get("total_fallbacks", 0),
-            first_seen=(datetime.fromisoformat(data["first_seen"]) if data.get("first_seen") else datetime.now()),
-            last_updated=(datetime.fromisoformat(data["last_updated"]) if data.get("last_updated") else datetime.now()),
-        )
+        try:
+            record = cls(
+                skill_id=str(data["skill_id"]),
+                name=str(data["name"]),
+                description=data.get("description", ""),
+                path=data.get("path", ""),
+                is_active=data.get("is_active", True),
+                category=SkillCategory(data["category"]) if data.get("category") else SkillCategory.WORKFLOW,
+                tags=data.get("tags", []),
+                visibility=(SkillVisibility(data["visibility"]) if data.get("visibility") else SkillVisibility.PRIVATE),
+                creator_id=data.get("creator_id", ""),
+                lineage=(
+                    SkillLineage.from_dict(data["lineage"])
+                    if data.get("lineage")
+                    else SkillLineage(origin=SkillOrigin.IMPORTED)
+                ),
+                tool_dependencies=data.get("tool_dependencies", []),
+                critical_tools=data.get("critical_tools", []),
+                total_selections=int(data.get("total_selections", 0)),
+                total_applied=int(data.get("total_applied", 0)),
+                total_completions=int(data.get("total_completions", 0)),
+                total_fallbacks=int(data.get("total_fallbacks", 0)),
+                first_seen=(datetime.fromisoformat(data["first_seen"]) if data.get("first_seen") else datetime.now()),
+                last_updated=(datetime.fromisoformat(data["last_updated"]) if data.get("last_updated") else datetime.now()),
+            )
+        except (TypeError, ValueError) as exc:
+            raise ValidationError(f"Invalid SkillRecord data: {exc}") from exc
         for a in data.get("recent_analyses", []):
             record.recent_analyses.append(ExecutionAnalysis.from_dict(a))
         return record

tests/test_skill_types.py

@@ -43,7 +43,7 @@ class TestSkillVersion:
     def test_default_version(self):
         v = SkillVersion()
         assert v.major == 0
-        assert v.minor == 1
+        assert v.minor == 0
         assert v.patch == 0
 
     def test_explicit_construction(self):
@@ -746,3 +746,141 @@ def test_datetime_round_trip_precision(self):
         restored = SkillLineage.from_dict(lin.to_dict())
         assert restored.created_at.year == 2025
         assert restored.created_at.second == 59
+
+
+# ═══════════════════════════════════════════════════════════════════════
+#  Hostile / adversarial input tests (F5)
+# ═══════════════════════════════════════════════════════════════════════
+
+
+class TestFromDictHostileInput:
+    """from_dict() with wrong types, partial payloads, and adversarial values."""
+
+    # --- Wrong types: int where string expected ---
+
+    def test_skill_record_numeric_skill_id_coerced(self):
+        """Numeric skill_id should be coerced to str."""
+        d = {"skill_id": 12345, "name": "test"}
+        r = SkillRecord.from_dict(d)
+        assert r.skill_id == "12345"
+        assert isinstance(r.skill_id, str)
+
+    def test_skill_record_numeric_name_coerced(self):
+        """Numeric name should be coerced to str."""
+        d = {"skill_id": "s1", "name": 42}
+        r = SkillRecord.from_dict(d)
+        assert r.name == "42"
+        assert isinstance(r.name, str)
+
+    def test_execution_analysis_numeric_task_id_coerced(self):
+        """Numeric task_id should be coerced to str."""
+        d = {"task_id": 999, "timestamp": "2025-01-01T00:00:00"}
+        a = ExecutionAnalysis.from_dict(d)
+        assert a.task_id == "999"
+        assert isinstance(a.task_id, str)
+
+    def test_skill_version_string_components_coerced(self):
+        """String version components should be coerced to int."""
+        d = {"major": "2", "minor": "3", "patch": "4"}
+        v = SkillVersion.from_dict(d)
+        assert v == SkillVersion(2, 3, 4)
+
+    def test_skill_record_string_counters_coerced(self):
+        """String counter values should be coerced to int."""
+        d = {
+            "skill_id": "s1",
+            "name": "test",
+            "total_selections": "10",
+            "total_applied": "8",
+            "total_completions": "7",
+            "total_fallbacks": "1",
+        }
+        r = SkillRecord.from_dict(d)
+        assert r.total_selections == 10
+        assert isinstance(r.total_selections, int)
+        assert r.total_applied == 8
+        assert r.total_completions == 7
+        assert r.total_fallbacks == 1
+
+    # --- Partial payloads: missing keys ---
+
+    def test_skill_version_from_dict_empty(self):
+        """Empty dict should yield 0.0.0."""
+        v = SkillVersion.from_dict({})
+        assert v == SkillVersion(0, 0, 0)
+
+    def test_skill_version_from_dict_major_only(self):
+        """Only major → minor and patch default to 0."""
+        v = SkillVersion.from_dict({"major": 2})
+        assert v == SkillVersion(2, 0, 0)
+
+    def test_skill_record_from_dict_minimal(self):
+        """Only required keys; all optional fields get safe defaults."""
+        d = {"skill_id": "s1", "name": "n"}
+        r = SkillRecord.from_dict(d)
+        assert r.total_selections == 0
+        assert r.total_applied == 0
+        assert r.total_completions == 0
+        assert r.total_fallbacks == 0
+        assert r.description == ""
+        assert r.is_active is True
+
+    def test_execution_analysis_from_dict_minimal(self):
+        """Only required keys for ExecutionAnalysis."""
+        d = {"task_id": "t1", "timestamp": "2025-01-01T00:00:00"}
+        a = ExecutionAnalysis.from_dict(d)
+        assert a.task_id == "t1"
+        assert a.skill_judgments == []
+        assert a.evolution_suggestions == []
+
+    # --- Adversarial values ---
+
+    def test_skill_record_empty_string_skill_id(self):
+        """Empty string skill_id should be coerced to str, validation catches it."""
+        d = {"skill_id": "", "name": "test"}
+        r = SkillRecord.from_dict(d)
+        assert r.skill_id == ""
+        with pytest.raises(ValidationError, match="skill_id"):
+            r.validate()
+
+    def test_skill_record_very_long_name(self):
+        """Very long name should still round-trip without crash."""
+        long_name = "x" * 10_000
+        d = {"skill_id": "s1", "name": long_name}
+        r = SkillRecord.from_dict(d)
+        assert r.name == long_name
+        assert len(r.name) == 10_000
+
+    def test_skill_version_non_numeric_raises(self):
+        """Non-numeric version components should raise ValidationError."""
+        with pytest.raises(ValidationError):
+            SkillVersion.from_dict({"major": "abc"})
+
+    def test_skill_record_non_numeric_counter_raises(self):
+        """Non-numeric counter should raise ValidationError."""
+        with pytest.raises(ValidationError):
+            SkillRecord.from_dict({
+                "skill_id": "s1",
+                "name": "test",
+                "total_selections": "not_a_number",
+            })
+
+    # --- Type coercion correctness ---
+
+    def test_skill_version_float_coerced(self):
+        """Float version components should be truncated to int."""
+        d = {"major": 1.9, "minor": 2.1, "patch": 0.0}
+        v = SkillVersion.from_dict(d)
+        assert v == SkillVersion(1, 2, 0)
+
+    def test_skill_record_float_counters_coerced(self):
+        """Float counter values should be truncated to int."""
+        d = {
+            "skill_id": "s1",
+            "name": "test",
+            "total_selections": 5.7,
+            "total_applied": 3.2,
+        }
+        r = SkillRecord.from_dict(d)
+        assert r.total_selections == 5
+        assert r.total_applied == 3