feat(P3): Epic 3.5 — TagSearch extraction + /8eyes fixes (#21)

* feat(3.5): extract TagIndex + SearchEngine from monolithic SkillStore

Epic 3.5: Decompose monolithic SkillStore by extracting tag indexing and search operations into a dedicated TagSearch module.

Changes:
- Created openspace/skill_engine/tag_search.py with TagSearch class
- Supports standalone usage (owns connection) and shared usage (injected connection/lock)
- Extracted methods: sync_tags, get_tags, find_skills_by_tool, find_skills_by_tags, search_skills, get_summary, get_stats, get_top_skills, get_count_and_timestamp
- Added comprehensive tag-based search with category/visibility/text filtering
- Proper SQL injection prevention with LIKE wildcard escaping
- Updated SkillStore to delegate tag/search operations to TagSearch
- Updated __init__.py to export TagSearch class
- Created comprehensive test suite (37 tests) covering:
  - Tag CRUD operations
  - Tag-based skill search (any/all match)
  - Tool dependency search
  - Skill discovery and statistics
  - Complex multi-criteria search
  - Shared connection patterns (SkillStore integration)
  - Error handling and edge cases
- All 1268 existing tests continue passing
- Cleaned up stale diff files

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

* fix(3.5): address /8eyes R1 — dedup tags, match_all, facade completeness, field compat

Fixes all 6 findings from /8eyes reviewers:

Finding 1 (MAJOR): Fixed duplicate tags crash in sync_tags
- Added deduplication at start: tags = list(set(tags))
- Added proper rollback on PK constraint failure

Finding 2 (MAJOR): Fixed match_all counts with duplicate input
- Use unique_tags = list(set(tags)) in find_skills_by_tags()
- Use len(unique_tags) in HAVING clause for both find_skills_by_tags and search_skills

Finding 3 (MAJOR): Added missing SkillStore facade methods
- Added find_skills_by_tags() facade delegation
- Added search_skills() facade delegation
- Added get_tags() facade delegation
- Added get_all_tags() facade delegation
- Added sync_tags() facade delegation

Finding 4 (MAJOR): Fixed get_stats field name for backward compatibility
- Changed 'by_lineage_origin' back to 'by_origin' in TagSearch.get_stats()

Finding 5 (MINOR): Fixed f-string parameterization in search_skills HAVING clause
- Use parameterized query: 'HAVING COUNT(DISTINCT st.tag) = ?' with params.append(len(unique_tags))

Finding 6 (MINOR): Added comprehensive integration tests
- test_sync_tags_with_duplicates — verify deduplication works
- test_find_by_tags_with_duplicate_input — verify match_all with dupes
- test_search_skills_with_duplicate_tags — verify search_skills match_all
- test_skill_store_facade_completeness — verify ALL TagSearch methods accessible
- test_facade_method_delegation — verify delegation works
- test_skill_store_get_stats_field_names — verify backward-compatible field names
- test_tag_search_direct_call_uses_original_field — verify TagSearch field name

All tests pass: 1275 passed, 31 skipped

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

---------

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

Author: 3 people

openspace/skill_engine/__init__.py

@@ -20,6 +20,7 @@
     "SkillRepository",
     "LineageTracker",
     "AnalysisStore",
+    "TagSearch",
     "ExecutionAnalyzer",
     "SkillEvolver",
     "EvolutionTrigger",
@@ -46,6 +47,7 @@
     "SkillRepository": (".skill_repository", "SkillRepository"),
     "LineageTracker": (".lineage_tracker", "LineageTracker"),
     "AnalysisStore": (".analysis_store", "AnalysisStore"),
+    "TagSearch": (".tag_search", "TagSearch"),
     "ExecutionAnalyzer": (".analyzer", "ExecutionAnalyzer"),
     "SkillEvolver": (".evolver", "SkillEvolver"),
     "EvolutionTrigger": (".evolver", "EvolutionTrigger"),

openspace/skill_engine/store.py

@@ -29,6 +29,7 @@
 from .lineage_tracker import LineageTracker
 from .patch import collect_skill_snapshot, compute_unified_diff
 from .skill_repository import SkillRepository
+from .tag_search import TagSearch
 from .types import (
     EvolutionSuggestion,
     ExecutionAnalysis,
@@ -205,6 +206,9 @@ def __init__(self, db_path: Optional[Path] = None) -> None:
         # Analysis store (Epic 3.4) — delegates execution analyses and judgments.
         self._analyses = AnalysisStore(conn=self._conn, lock=self._mu)
 
+        # Tag search (Epic 3.5) — delegates tag indexing and skill search operations.
+        self._tag_search = TagSearch(conn=self._conn, lock=self._mu)
+
         logger.debug(f"SkillStore ready at {self._db_path}")
 
     def _make_connection(self, *, read_only: bool) -> sqlite3.Connection:
@@ -290,6 +294,14 @@ def close(self) -> None:
             self._lineage.close()
         except Exception:
             pass
+        try:
+            self._analyses.close()
+        except Exception:
+            pass
+        try:
+            self._tag_search.close()
+        except Exception:
+            pass
         try:
             # Flush WAL → main DB so external readers see all data
             self._conn.execute("PRAGMA wal_checkpoint(TRUNCATE)")
@@ -765,16 +777,10 @@ def find_skills_by_tool(self, tool_key: str) -> List[str]:
         """
         Only returns active records — deactivated (superseded) versions
         are excluded so that Trigger 2 never re-processes old versions.
+        
+        Delegates to :class:`TagSearch` (Epic 3.5).
         """
-        with self._reader() as conn:
-            rows = conn.execute(
-                "SELECT sd.skill_id "
-                "FROM skill_tool_deps sd "
-                "JOIN skill_records sr ON sd.skill_id = sr.skill_id "
-                "WHERE sd.tool_key=? AND sr.is_active=1",
-                (tool_key,),
-            ).fetchall()
-            return [r["skill_id"] for r in rows]
+        return self._tag_search.find_skills_by_tool(tool_key)
 
     @_db_retry()
     def find_children(self, parent_skill_id: str) -> List[str]:
@@ -795,72 +801,29 @@ def get_summary(self, *, active_only: bool = True) -> List[Dict[str, Any]]:
         """Lightweight summary of skills (no analyses/deps loaded).
 
         Default filters to active skills only.
+        
+        Delegates to :class:`TagSearch` (Epic 3.5).
         """
-        with self._reader() as conn:
-            where = "WHERE is_active=1 " if active_only else ""
-            rows = conn.execute(
-                f"""
-                SELECT skill_id, name, description, category, is_active,
-                       visibility, creator_id,
-                       lineage_origin, lineage_generation,
-                       total_selections, total_applied,
-                       total_completions, total_fallbacks,
-                       first_seen, last_updated
-                FROM skill_records
-                {where}
-                ORDER BY last_updated DESC
-                """
-            ).fetchall()
-            return [dict(r) for r in rows]
+        return self._tag_search.get_summary(active_only=active_only)
 
     @_db_retry()
     def get_stats(self, *, active_only: bool = True) -> Dict[str, Any]:
-        """Aggregate statistics across skills."""
-        with self._reader() as conn:
-            where = " WHERE is_active=1" if active_only else ""
-            total = conn.execute(f"SELECT COUNT(*) FROM skill_records{where}").fetchone()[0]
-
-            by_category = {
-                r["category"]: r["cnt"]
-                for r in conn.execute(
-                    f"SELECT category, COUNT(*) AS cnt FROM skill_records{where} GROUP BY category"
-                ).fetchall()
-            }
-            by_origin = {
-                r["lineage_origin"]: r["cnt"]
-                for r in conn.execute(
-                    f"SELECT lineage_origin, COUNT(*) AS cnt FROM skill_records{where} GROUP BY lineage_origin"
-                ).fetchall()
-            }
-            # Delegate analysis stats to AnalysisStore (Epic 3.4)
-            analysis_stats = self._analyses.get_analysis_stats()
-            n_analyses = analysis_stats["total_analyses"]
-            n_candidates = analysis_stats["evolution_candidates"]
-            agg = conn.execute(
-                f"""
-                SELECT SUM(total_selections)  AS sel,
-                       SUM(total_applied)      AS app,
-                       SUM(total_completions)  AS comp,
-                       SUM(total_fallbacks)    AS fb
-                FROM skill_records{where}
-                """
-            ).fetchone()
-
-            # Also report total (including inactive) for context
-            total_all = conn.execute("SELECT COUNT(*) FROM skill_records").fetchone()[0]
-
-            return {
-                "total_skills": total,
-                "total_skills_all": total_all,
-                "by_category": by_category,
-                "by_origin": by_origin,
-                "total_analyses": n_analyses,
-                "evolution_candidates": n_candidates,
-                "total_selections": agg["sel"] or 0,
-                "total_applied": agg["app"] or 0,
-                "total_completions": agg["comp"] or 0,
-                "total_fallbacks": agg["fb"] or 0,
-            }
+        """Aggregate statistics across skills.
+        
+        Delegates to :class:`TagSearch` (Epic 3.5) for skill stats and
+        :class:`AnalysisStore` (Epic 3.4) for analysis stats.
+        """
+        # Get skill-related stats from TagSearch
+        stats = self._tag_search.get_stats(active_only=active_only)
+        
+        # Merge in analysis stats from AnalysisStore
+        analysis_stats = self._analyses.get_analysis_stats()
+        stats.update({
+            "total_analyses": analysis_stats["total_analyses"],
+            "evolution_candidates": analysis_stats["evolution_candidates"],
+        })
+        
+        return stats
 
     @_db_retry()
     def get_task_skill_summary(self, task_id: str) -> Dict[str, Any]:
@@ -892,45 +855,20 @@ def get_top_skills(
             ``applied_rate``    — applied / selections
             ``completion_rate`` — completions / applied
             ``total_selections``— raw count
+            
+        Delegates to :class:`TagSearch` (Epic 3.5).
         """
-        rate_exprs = {
-            "effective_rate": ("CAST(total_completions AS REAL) / total_selections"),
-            "applied_rate": ("CAST(total_applied AS REAL) / total_selections"),
-            "completion_rate": (
-                "CASE WHEN total_applied > 0 THEN CAST(total_completions AS REAL) / total_applied ELSE 0.0 END"
-            ),
-            "total_selections": "total_selections",
-        }
-        expr = rate_exprs.get(metric, rate_exprs["effective_rate"])
-        active_clause = " AND is_active=1" if active_only else ""
-
-        with self._reader() as conn:
-            rows = conn.execute(
-                f"SELECT *, ({expr}) AS _rank "
-                f"FROM skill_records "
-                f"WHERE total_selections >= ?{active_clause} "
-                f"ORDER BY _rank DESC LIMIT ?",
-                (min_selections, n),
-            ).fetchall()
-            results = []
-            for r in rows:
-                d = dict(r)
-                d.pop("_rank", None)
-                results.append(d)
-            return results
+        return self._tag_search.get_top_skills(
+            n=n, metric=metric, min_selections=min_selections, active_only=active_only
+        )
 
     @_db_retry()
     def get_count_and_timestamp(self, *, active_only: bool = True) -> Dict[str, Any]:
-        """Skill count + newest ``last_updated`` for cheap change detection."""
-        with self._reader() as conn:
-            where = " WHERE is_active=1" if active_only else ""
-            row = conn.execute(
-                f"SELECT COUNT(*) AS cnt, MAX(last_updated) AS max_ts FROM skill_records{where}"
-            ).fetchone()
-            return {
-                "count": row["cnt"] if row else 0,
-                "max_last_updated": row["max_ts"] if row else None,
-            }
+        """Skill count + newest ``last_updated`` for cheap change detection.
+        
+        Delegates to :class:`TagSearch` (Epic 3.5).
+        """
+        return self._tag_search.get_count_and_timestamp(active_only=active_only)
 
     # Lineage / Ancestry
     @_db_retry()
@@ -1073,16 +1011,8 @@ def _upsert(self, record: SkillRecord) -> None:
                 (record.skill_id, tk, 1 if tk in critical_set else 0),
             )
 
-        # Sync tags
-        self._conn.execute(
-            "DELETE FROM skill_tags WHERE skill_id=?",
-            (record.skill_id,),
-        )
-        for tag in record.tags:
-            self._conn.execute(
-                "INSERT INTO skill_tags(skill_id, tag) VALUES(?,?)",
-                (record.skill_id, tag),
-            )
+        # Sync tags - delegate to TagSearch (Epic 3.5)
+        self._tag_search.sync_tags(record.skill_id, record.tags)
 
         # Sync analyses (insert only NEW ones, dedup by task_id) - delegate to AnalysisStore
         self._analyses.bulk_upsert_analyses(record.recent_analyses)
@@ -1130,7 +1060,8 @@ def _to_record(self, conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
             (sid,),
         ).fetchall()
 
-        tag_rows = conn.execute("SELECT tag FROM skill_tags WHERE skill_id=?", (sid,)).fetchall()
+        # Load tags - delegate to TagSearch (Epic 3.5)
+        tags = self._tag_search.get_tags(sid)
 
         # Load recent analyses involving this skill - delegate to AnalysisStore (Epic 3.4)
         recent_analyses = self._analyses.load_recent_analyses_for_skill(
@@ -1144,7 +1075,7 @@ def _to_record(self, conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
             path=row["path"],
             is_active=bool(row["is_active"]),
             category=SkillCategory(row["category"]),
-            tags=[r["tag"] for r in tag_rows],
+            tags=tags,
             visibility=(SkillVisibility(row["visibility"]) if row["visibility"] else SkillVisibility.PRIVATE),
             creator_id=row["creator_id"] or "",
             lineage=lineage,
@@ -1158,3 +1089,52 @@ def _to_record(self, conn: sqlite3.Connection, row: sqlite3.Row) -> SkillRecord:
             first_seen=datetime.fromisoformat(row["first_seen"]),
             last_updated=datetime.fromisoformat(row["last_updated"]),
         )
+
+    # ── Tag Search Facade Methods (Epic 3.5) ──────────────────────────────
+    # Fix Finding 3: Complete SkillStore facade for all extracted TagSearch methods
+
+    def find_skills_by_tags(
+        self,
+        tags: List[str],
+        *,
+        match_all: bool = False,
+        active_only: bool = True,
+    ) -> List[str]:
+        """Find skills by tags (facade to TagSearch)."""
+        return self._tag_search.find_skills_by_tags(
+            tags, match_all=match_all, active_only=active_only
+        )
+
+    def search_skills(
+        self,
+        query: Optional[str] = None,
+        *,
+        category: Optional[SkillCategory] = None,
+        visibility: Optional[SkillVisibility] = None,
+        tags: Optional[List[str]] = None,
+        match_all_tags: bool = False,
+        active_only: bool = True,
+        limit: Optional[int] = None,
+    ) -> List[Dict[str, Any]]:
+        """Search skills by multiple criteria (facade to TagSearch)."""
+        return self._tag_search.search_skills(
+            query,
+            category=category,
+            visibility=visibility,
+            tags=tags,
+            match_all_tags=match_all_tags,
+            active_only=active_only,
+            limit=limit,
+        )
+
+    def get_tags(self, skill_id: str) -> List[str]:
+        """Get all tags for a skill (facade to TagSearch)."""
+        return self._tag_search.get_tags(skill_id)
+
+    def get_all_tags(self) -> List[Dict[str, Any]]:
+        """Get all tags with usage counts (facade to TagSearch)."""
+        return self._tag_search.get_all_tags()
+
+    def sync_tags(self, skill_id: str, tags: List[str]) -> None:
+        """Synchronize tags for a skill (facade to TagSearch)."""
+        return self._tag_search.sync_tags(skill_id, tags)