From b03d934ec6367074d8f4a9d63e95a7b08d878fad Mon Sep 17 00:00:00 2001 From: muhamed hamed <111616619+muhamedhamedvl@users.noreply.github.com> Date: Sat, 6 Jun 2026 23:46:32 +0300 Subject: [PATCH] fix: restore backup import after skills migration (#2980) --- routes/backup_routes.py | 68 ++++++++++++++++++---- static/js/admin.js | 14 ++++- tests/test_backup_import_skills.py | 92 ++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 15 deletions(-) create mode 100644 tests/test_backup_import_skills.py diff --git a/routes/backup_routes.py b/routes/backup_routes.py index 2b92a1529..5ca403f81 100644 --- a/routes/backup_routes.py +++ b/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 ── diff --git a/static/js/admin.js b/static/js/admin.js index 5d3d4a356..b4a1c7399 100644 --- a/static/js/admin.js +++ b/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 { diff --git a/tests/test_backup_import_skills.py b/tests/test_backup_import_skills.py new file mode 100644 index 000000000..35cfdf87d --- /dev/null +++ b/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"