fix: restore backup import after skills migration (#2980)

routes/backup_routes.py

@@ -101,24 +101,68 @@ def setup_backup_routes(memory_manager, preset_manager, skills_manager) -> APIRo
         # ── Skills ──
         if "skills" in body and isinstance(body["skills"], list):
             existing = skills_manager.load_all()
-            existing_ids = {s.get("id") for s in existing}
-            existing_titles = {s.get("title", "").strip().lower() for s in existing}
+            existing_names = {s.get("name") for s in existing if s.get("name")}
+            existing_ids = {s.get("id") for s in existing if s.get("id")}
+            existing_titles = {
+                (s.get("title") or s.get("description") or "").strip().lower()
+                for s in existing
+            }
             added = 0
             for skill in body["skills"]:
-                if not isinstance(skill, dict) or not skill.get("title"):
+                if not isinstance(skill, dict):
                     continue
-                # Skip if same id or same title already exists
-                if skill.get("id") in existing_ids:
+                title = (
+                    skill.get("title") or skill.get("description")
+                    or skill.get("name") or ""
+                ).strip()
+                if not title:
                     continue
-                if skill["title"].strip().lower() in existing_titles:
+                sid = skill.get("id") or skill.get("name")
+                if sid and sid in existing_ids:
                     continue
-                if user and not skill.get("owner"):
-                    skill["owner"] = user
-                existing.append(skill)
-                existing_ids.add(skill.get("id"))
-                existing_titles.add(skill["title"].strip().lower())
+                nm = skill.get("name")
+                if nm and nm in existing_names:
+                    continue
+                if title.lower() in existing_titles:
+                    continue
+                owner = skill.get("owner")
+                if user and not owner:
+                    owner = user
+                # Skills live on disk as SKILL.md files; the old JSON-era
+                # skills_manager.save() no longer exists. Write each new skill
+                # via add_skill (source="user" skips auto-dedup — this is an
+                # explicit backup restore).
+                result = skills_manager.add_skill(
+                    title=title,
+                    name=skill.get("name"),
+                    description=skill.get("description"),
+                    problem=skill.get("problem", ""),
+                    solution=skill.get("solution", ""),
+                    steps=skill.get("steps"),
+                    tags=skill.get("tags"),
+                    source="user",
+                    teacher_model=skill.get("teacher_model"),
+                    confidence=skill.get("confidence", 0.8),
+                    owner=owner,
+                    category=skill.get("category", "general"),
+                    when_to_use=skill.get("when_to_use"),
+                    procedure=skill.get("procedure"),
+                    pitfalls=skill.get("pitfalls"),
+                    verification=skill.get("verification"),
+                    platforms=skill.get("platforms"),
+                    requires_toolsets=skill.get("requires_toolsets"),
+                    fallback_for_toolsets=skill.get("fallback_for_toolsets"),
+                    status=skill.get("status", "draft"),
+                    version=skill.get("version", "1.0.0"),
+                )
+                if result.get("_deduped"):
+                    continue
+                if result.get("name"):
+                    existing_names.add(result["name"])
+                if result.get("id"):
+                    existing_ids.add(result["id"])
+                existing_titles.add(title.lower())
                 added += 1
-            skills_manager.save(existing)
             imported.append(f"{added} skills")
 
         # ── Presets ──

static/js/admin.js

@@ -2120,14 +2120,22 @@ function initBackup() {
     const btn = el('adm-importDataBtn');
     btn.disabled = true; btn.textContent = 'Importing...'; msg.textContent = '';
     try {
-      const text = await file.text();
-      const data = JSON.parse(text);
+      const text = (await file.text()).replace(/^\uFEFF/, '').trim();
+      let data;
+      try {
+        data = JSON.parse(text);
+      } catch (e) {
+        throw new Error('Invalid backup file: ' + e.message);
+      }
       const res = await fetch('/api/import', {
         method: 'POST', credentials: 'same-origin',
         headers: { 'Content-Type': 'application/json' },
         body: JSON.stringify(data),
       });
-      const result = await res.json();
+      const result = await res.json().catch(() => null);
+      if (!result) {
+        throw new Error(`Import failed: server returned ${res.status}`);
+      }
       if (res.ok && result.ok) {
         msg.textContent = result.message || 'Import successful.'; msg.className = 'admin-success';
       } else {

tests/test_backup_import_skills.py

@@ -0,0 +1,92 @@
+"""Backup import must not call the removed skills_manager.save().
+
+Skills migrated from data/skills.json to on-disk SKILL.md files; save() was
+removed from SkillsManager. Import still always sees a ``skills`` key in
+exported backups (often ``[]``), so calling save() raised AttributeError,
+returned a 500 HTML page, and the UI reported a misleading JSON.parse error
+from res.json().
+"""
+import asyncio
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+import routes.backup_routes as br
+
+
+class _Req:
+    def __init__(self, body):
+        self._body = body
+
+    async def json(self):
+        return self._body
+
+
+def _setup(monkeypatch, skills_manager):
+    monkeypatch.setattr(br, "require_admin", lambda request: None)
+    monkeypatch.setattr(br, "get_current_user", lambda request: "alice")
+
+    mem = MagicMock()
+    mem.load_all.return_value = []
+    mem.save.return_value = None
+
+    presets = MagicMock()
+    presets.get_all.return_value = {}
+    presets.save.return_value = True
+
+    router = br.setup_backup_routes(mem, presets, skills_manager)
+    endpoint = None
+    for r in router.routes:
+        if r.path == "/api/import" and "POST" in getattr(r, "methods", set()):
+            endpoint = r.endpoint
+    assert endpoint is not None
+    return endpoint
+
+
+def test_import_with_empty_skills_list_does_not_call_save(monkeypatch):
+    skills = MagicMock(spec=["load_all", "add_skill"])
+    skills.load_all.return_value = []
+    endpoint = _setup(monkeypatch, skills)
+
+    body = {"settings": {"foo": "bar"}, "skills": []}
+    with monkeypatch.context() as m:
+        m.setattr(br, "load_settings", lambda: {})
+        m.setattr(br, "save_settings", lambda s: None)
+        result = asyncio.run(endpoint(_Req(body)))
+
+    assert result["ok"] is True
+    skills.add_skill.assert_not_called()
+    assert not hasattr(skills, "save") or not getattr(skills, "save", MagicMock()).called
+
+
+def test_import_adds_new_skill_via_add_skill(monkeypatch):
+    skills = MagicMock(spec=["load_all", "add_skill"])
+    skills.load_all.return_value = []
+    skills.add_skill.return_value = {
+        "id": "buy-milk",
+        "name": "buy-milk",
+        "title": "Buy milk",
+    }
+    endpoint = _setup(monkeypatch, skills)
+
+    body = {
+        "skills": [{"name": "buy-milk", "title": "Buy milk", "description": "Buy milk"}],
+        "preferences": {"theme": "dark"},
+    }
+    with monkeypatch.context() as m:
+        m.setattr(br, "load_settings", lambda: {})
+        m.setattr(br, "save_settings", lambda s: None)
+        m.setattr(br, "load_features", lambda: {})
+        m.setattr(br, "save_features", lambda f: None)
+        m.setattr(
+            "routes.prefs_routes._load_for_user",
+            lambda user: {},
+        )
+        m.setattr(
+            "routes.prefs_routes._save_for_user",
+            lambda user, prefs: None,
+        )
+        result = asyncio.run(endpoint(_Req(body)))
+
+    assert result["ok"] is True
+    skills.add_skill.assert_called_once()
+    assert skills.add_skill.call_args.kwargs.get("source") == "user"