fix(3.3): address /8eyes R1 findings — cycle bug, diamond DAG, shared-conn, layering

- Fix 1: get_ancestors() cycle bug — seed visited with starting skill_id to prevent cycles
- Fix 2: get_lineage_tree() diamond DAG — pass copy of visited to each sibling subtree to preserve all paths
- Fix 3: shared-conn _reader() dirty reads — acquire lock when using shared connection
- Fix 4: SkillRepository._to_record layering violation — make public as to_record()
- Fix 5: recent_analyses hydration regression — re-hydrate in SkillStore delegation methods
- Fix 6: missing test coverage — add cycle, diamond, shared-conn isolation, and facade tests

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

Author: Brian Krafft

openspace/skill_engine/lineage_tracker.py

@@ -143,7 +143,9 @@ def _reader(self) -> Generator[sqlite3.Connection, None, None]:
         """Open a temporary read-only connection (WAL parallel reads)."""
         self._ensure_open()
         if not self._owns_conn:
-            yield self._conn
+            # Fix 3: Acquire lock when using shared connection to prevent dirty reads
+            with self._mu:
+                yield self._conn
             return
         conn = self._make_connection(read_only=True)
         try:
@@ -267,7 +269,7 @@ def get_ancestors(
             generation (oldest first).
         """
         with self._reader() as conn:
-            visited: set[str] = set()
+            visited: set[str] = {skill_id}  # Fix 1: Seed with starting skill_id to prevent cycles
             ancestors: List[SkillRecord] = []
             frontier = [skill_id]
 
@@ -289,7 +291,7 @@ def get_ancestors(
                         ).fetchone()
                         if row:
                             ancestors.append(
-                                SkillRepository._to_record(conn, row)
+                                SkillRepository.to_record(conn, row)
                             )
                             next_frontier.append(pid)
                 frontier = next_frontier
@@ -318,7 +320,7 @@ def get_evolution_chain(self, name: str) -> List[SkillRecord]:
                 "ORDER BY lineage_generation ASC",
                 (name,),
             ).fetchall()
-            return [SkillRepository._to_record(conn, r) for r in rows]
+            return [SkillRepository.to_record(conn, r) for r in rows]
 
     @_db_retry()
     def get_lineage_tree(
@@ -370,7 +372,8 @@ def _subtree(
         ).fetchall():
             cid = cr["skill_id"]
             if cid not in visited:
+                # Fix 2: Pass copy of visited to prevent diamond DAG edge loss
                 node["children"].append(
-                    self._subtree(conn, cid, depth - 1, visited)
+                    self._subtree(conn, cid, depth - 1, visited.copy())
                 )
         return node

openspace/skill_engine/skill_repository.py

@@ -276,7 +276,7 @@ def get(self, skill_id: str) -> Optional[SkillRecord]:
                 "SELECT * FROM skill_records WHERE skill_id=?",
                 (skill_id,),
             ).fetchone()
-            return self._to_record(conn, row) if row else None
+            return self.to_record(conn, row) if row else None
 
     # ── CRUD: Delete ──────────────────────────────────────────────────
 
@@ -316,7 +316,7 @@ def list_all(self, *, active_only: bool = False) -> Dict[str, SkillRecord]:
                 ).fetchall()
             result: Dict[str, SkillRecord] = {}
             for row in rows:
-                rec = self._to_record(conn, row)
+                rec = self.to_record(conn, row)
                 result[rec.skill_id] = rec
             return result
 
@@ -375,7 +375,7 @@ def search(
                 params,
             ).fetchall()
 
-            return [self._to_record(conn, row) for row in rows]
+            return [self.to_record(conn, row) for row in rows]
 
     # ── CRUD: Count ───────────────────────────────────────────────────
 
@@ -525,7 +525,7 @@ def _upsert(self, record: SkillRecord) -> None:
     # ── Internal: Deserialization ─────────────────────────────────────
 
     @staticmethod
-    def _to_record(conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
+    def to_record(conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
         """Deserialize a ``skill_records`` row + related rows → SkillRecord."""
         sid = row["skill_id"]
 

openspace/skill_engine/store.py

@@ -699,7 +699,9 @@ def get_versions(self, name: str) -> List[SkillRecord]:
 
         Delegates to :class:`LineageTracker` (Epic 3.3).
         """
-        return self._lineage.get_evolution_chain(name)
+        # Fix 5: Hydrate recent_analyses after delegation
+        records = self._lineage.get_evolution_chain(name)
+        return [self._hydrate_recent_analyses(record) for record in records]
 
     @_db_retry()
     def load_by_category(self, category: SkillCategory, *, active_only: bool = True) -> List[SkillRecord]:
@@ -998,7 +1000,9 @@ def get_ancestry(self, skill_id: str, max_depth: int = 10) -> List[SkillRecord]:
 
         Delegates to :class:`LineageTracker` (Epic 3.3).
         """
-        return self._lineage.get_ancestors(skill_id, max_depth=max_depth)
+        # Fix 5: Hydrate recent_analyses after delegation
+        records = self._lineage.get_ancestors(skill_id, max_depth=max_depth)
+        return [self._hydrate_recent_analyses(record) for record in records]
 
     @_db_retry()
     def get_lineage_tree(self, skill_id: str, max_depth: int = 5) -> Dict[str, Any]:
@@ -1198,6 +1202,41 @@ def _insert_analysis(self, a: ExecutionAnalysis) -> int:
 
         return analysis_id
 
+    # Fix 5: Helper method to hydrate recent_analyses for records from LineageTracker
+    def _hydrate_recent_analyses(self, record: SkillRecord) -> SkillRecord:
+        """Hydrate recent_analyses for a SkillRecord from LineageTracker delegation."""
+        with self._reader() as conn:
+            analysis_rows = conn.execute(
+                "SELECT ea.* FROM execution_analyses ea "
+                "JOIN skill_judgments sj ON ea.id = sj.analysis_id "
+                "WHERE sj.skill_id = ? "
+                "ORDER BY ea.timestamp DESC LIMIT ?",
+                (record.skill_id, SkillRecord.MAX_RECENT),
+            ).fetchall()
+            
+            # Create a new record with hydrated recent_analyses
+            return SkillRecord(
+                skill_id=record.skill_id,
+                name=record.name,
+                description=record.description,
+                path=record.path,
+                is_active=record.is_active,
+                category=record.category,
+                tags=record.tags,
+                visibility=record.visibility,
+                creator_id=record.creator_id,
+                lineage=record.lineage,
+                tool_dependencies=record.tool_dependencies,
+                critical_tools=record.critical_tools,
+                total_selections=record.total_selections,
+                total_applied=record.total_applied,
+                total_completions=record.total_completions,
+                total_fallbacks=record.total_fallbacks,
+                recent_analyses=[self._to_analysis(conn, r) for r in reversed(analysis_rows)],
+                first_seen=record.first_seen,
+                last_updated=record.last_updated,
+            )
+
     # Deserialization
     def _to_record(self, conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
         """Deserialize a skill_records row + related rows → SkillRecord."""