mirror of
https://github.com/langbot-app/LangBot.git
synced 2026-06-10 07:46:02 +00:00
refactor(skill): remove all local-filesystem fallbacks; Box is the sole source
Skills now flow exclusively through the Box runtime. Every read and write method funnels through ``_box_service()``; when Box is unavailable (disabled in config, connection failed, or simply not installed) the operation either returns an empty surface (``list_skills`` → []) or raises with a clear ``Box runtime ... not initialised / disabled / unavailable: ...`` message via the new ``_require_box(action)`` helper. Why: the legacy local-fallback path scanned ``data/skills/``, but Box manages its own ``box.local.skills_root`` (default ``data/box/skills/``). The two diverging directories caused stale / phantom skill lists when Box flapped, and the local-fallback writes silently bypassed all the sandboxing the operator had configured. SkillService (``api/http/service/skill.py``): - New ``_require_box(action)`` returns the box service or raises a structured ValueError. ``_require_box_for_write`` kept as alias - ``list_skills`` → returns [] when Box is down so the UI can render the disabled banner cleanly - ``get_skill`` / ``get_skill_by_name`` → return None - All read-file / write-file / scan-dir / create / update / delete / install / preview methods → ``_require_box`` then box delegate. Local fallback bodies (shutil.copytree, tempfile.mkdtemp, preview pipelines) removed entirely SkillManager (``pkg/skill/manager.py``): - ``reload_skills`` returns early with empty cache when Box is down. data/skills/ discovery loop removed - ``refresh_skill_from_disk`` now just reports cache presence; the on-disk re-parse is gone since Box is the only writer Tests: - Drop 11 obsolete test_skill_service.py tests that exercised the removed local-fallback paths (create/install/file/delete/update) - Add list-empty + read-refused tests; flip the legacy-allow test to legacy-refuses-too - Rewrite refresh_skill_from_disk test to match the new behaviour Several helper methods (_managed_skill_path, _resolve_skill_path, _preview_skill_candidates, _install_preview_candidates, etc.) are now unreachable; a follow-up commit will prune them so this diff stays reviewable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Junyan Qin
@@ -54,29 +54,25 @@ class TestSkillManagerPackageLoading:
|
||||
assert skill_data['instructions'] == '# Test Skill\nDo things.'
|
||||
assert skill_data['description'] == 'Test skill'
|
||||
|
||||
def test_refresh_skill_from_disk_updates_cached_dict_in_place(self):
|
||||
def test_refresh_skill_from_disk_reports_cache_presence(self):
|
||||
"""Box is the only source of truth for skill content. refresh_skill_from_disk
|
||||
now just reports whether the skill is still in the in-memory cache —
|
||||
the actual content refresh is driven by SkillService awaiting
|
||||
``reload_skills`` after every Box mutation."""
|
||||
from langbot.pkg.skill.manager import SkillManager
|
||||
|
||||
ap = _make_ap()
|
||||
mgr = SkillManager(ap)
|
||||
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
skill_md = os.path.join(tmpdir, 'SKILL.md')
|
||||
with open(skill_md, 'w', encoding='utf-8') as f:
|
||||
f.write('---\ndescription: First\n---\n\nOriginal instructions')
|
||||
# Empty cache → returns False
|
||||
assert mgr.refresh_skill_from_disk('test-skill') is False
|
||||
|
||||
skill_data = _make_skill_data(name='test-skill', package_root=tmpdir)
|
||||
assert mgr._load_skill_file(skill_data) is True
|
||||
|
||||
mgr.skills['test-skill'] = skill_data
|
||||
|
||||
with open(skill_md, 'w', encoding='utf-8') as f:
|
||||
f.write('---\ndescription: Second\n---\n\nUpdated instructions')
|
||||
|
||||
assert mgr.refresh_skill_from_disk('test-skill') is True
|
||||
assert mgr.skills['test-skill'] is skill_data
|
||||
assert skill_data['instructions'] == 'Updated instructions'
|
||||
assert skill_data['description'] == 'Second'
|
||||
# Cache populated → returns True; method does NOT mutate the cache
|
||||
cached = _make_skill_data(name='test-skill', instructions='Cached')
|
||||
mgr.skills['test-skill'] = cached
|
||||
assert mgr.refresh_skill_from_disk('test-skill') is True
|
||||
assert mgr.skills['test-skill'] is cached
|
||||
assert mgr.refresh_skill_from_disk('') is False
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reload_skills_drops_box_skills_with_missing_package_root(self):
|
||||
|
||||
@@ -1,12 +1,9 @@
|
||||
import io
|
||||
import os
|
||||
import zipfile
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from src.langbot.pkg.api.http.service.skill import SkillService
|
||||
from langbot.pkg.api.http.service.skill import SkillService
|
||||
|
||||
|
||||
def _create_skill_file(
|
||||
@@ -73,9 +70,9 @@ def test_scan_directory_errors_when_skill_is_deeper_than_two_levels(skill_servic
|
||||
|
||||
|
||||
class TestRequireBoxForWrite:
|
||||
"""Writes must refuse when ``ap.box_service`` is installed but unavailable
|
||||
(disabled in config OR connection failed). Legacy setups without
|
||||
``ap.box_service`` continue to use the local fallback."""
|
||||
"""Box is the only source of truth for skills — there is no local
|
||||
filesystem fallback. Every write and (most) read methods refuse cleanly
|
||||
when the Box runtime is disabled, unreachable, or simply not installed."""
|
||||
|
||||
def _ap_with_disabled_box(self):
|
||||
return SimpleNamespace(
|
||||
@@ -134,347 +131,22 @@ class TestRequireBoxForWrite:
|
||||
await service.install_from_zip_upload(file_bytes=b'', filename='x.zip')
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_legacy_setup_without_box_service_still_allows_local_create(self, tmp_path, monkeypatch):
|
||||
"""Setups that never installed ap.box_service (pre-Box dev mode) keep
|
||||
using the local-skills fallback path — that's the whole point of the
|
||||
``getattr`` distinguisher in _require_box_for_write."""
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
async def test_create_skill_refused_when_box_service_missing_entirely(self):
|
||||
"""No ap.box_service attribute at all (truly minimal setup):
|
||||
Box is the only source of truth, so creation must still refuse."""
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill_by_name = AsyncMock(return_value=None)
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'local-skill',
|
||||
'package_root': str(tmp_path / 'data' / 'skills' / 'local-skill'),
|
||||
'description': '',
|
||||
'instructions': '',
|
||||
}
|
||||
)
|
||||
|
||||
# Does not raise — gate is a no-op without ap.box_service.
|
||||
await service.create_skill(
|
||||
{'name': 'local-skill', 'display_name': 'Local', 'description': '', 'instructions': 'hi'}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_skill_import_preserves_existing_skill_content_when_form_fields_blank(tmp_path, monkeypatch):
|
||||
source_dir = tmp_path / 'external-skills' / 'manual-skill'
|
||||
source_dir.mkdir(parents=True)
|
||||
_create_skill_file(
|
||||
source_dir / 'SKILL.md',
|
||||
display_name='Imported Skill',
|
||||
description='Imported description',
|
||||
body='Original instructions',
|
||||
)
|
||||
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill_by_name = AsyncMock(return_value=None)
|
||||
managed_root = tmp_path / 'data' / 'skills' / 'imported-skill'
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'imported-skill',
|
||||
'package_root': str(managed_root.resolve()),
|
||||
'description': 'Imported description',
|
||||
'instructions': 'Original instructions',
|
||||
}
|
||||
)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
|
||||
await service.create_skill(
|
||||
{
|
||||
'name': 'imported-skill',
|
||||
'package_root': str(source_dir),
|
||||
'display_name': '',
|
||||
'description': '',
|
||||
'instructions': '',
|
||||
}
|
||||
)
|
||||
|
||||
content = (managed_root / 'SKILL.md').read_text(encoding='utf-8')
|
||||
assert 'display_name: Imported Skill' in content
|
||||
assert 'description: Imported description' in content
|
||||
assert content.endswith('Original instructions')
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_skill_reuses_existing_managed_directory_without_copying(tmp_path, monkeypatch):
|
||||
managed_root = tmp_path / 'data' / 'skills' / 'demo-repo' / 'skills' / 'nested-skill'
|
||||
managed_root.mkdir(parents=True)
|
||||
_create_skill_file(
|
||||
managed_root / 'SKILL.md',
|
||||
name='nested-skill',
|
||||
display_name='Nested Skill',
|
||||
description='Already managed',
|
||||
body='Managed instructions',
|
||||
)
|
||||
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill_by_name = AsyncMock(return_value=None)
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'nested-skill',
|
||||
'package_root': str(managed_root.resolve()),
|
||||
'description': 'Already managed',
|
||||
'instructions': 'Managed instructions',
|
||||
}
|
||||
)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
|
||||
await service.create_skill(
|
||||
{
|
||||
'name': 'nested-skill',
|
||||
'package_root': str(managed_root),
|
||||
'display_name': '',
|
||||
'description': '',
|
||||
'instructions': '',
|
||||
}
|
||||
)
|
||||
|
||||
copied_root = tmp_path / 'data' / 'skills' / 'nested-skill'
|
||||
assert not copied_root.exists()
|
||||
content = (managed_root / 'SKILL.md').read_text(encoding='utf-8')
|
||||
assert 'display_name: Nested Skill' in content
|
||||
assert content.endswith('Managed instructions')
|
||||
|
||||
|
||||
def _build_skill_archive() -> bytes:
|
||||
stream = io.BytesIO()
|
||||
with zipfile.ZipFile(stream, 'w') as archive:
|
||||
archive.writestr(
|
||||
'demo-repo-main/skills/nested-skill/SKILL.md',
|
||||
'---\nname: imported-skill\ndescription: Imported from GitHub archive\n---\n\nSkill instructions\n',
|
||||
)
|
||||
return stream.getvalue()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_install_from_github_supports_nested_skill_archive(skill_service, tmp_path, monkeypatch):
|
||||
archive_bytes = _build_skill_archive()
|
||||
|
||||
class _FakeResponse:
|
||||
def __init__(self, content: bytes) -> None:
|
||||
self.content = content
|
||||
|
||||
def raise_for_status(self) -> None:
|
||||
return None
|
||||
|
||||
class _FakeAsyncClient:
|
||||
def __init__(self, *args, **kwargs) -> None:
|
||||
pass
|
||||
|
||||
async def __aenter__(self):
|
||||
return self
|
||||
|
||||
async def __aexit__(self, exc_type, exc, tb):
|
||||
return None
|
||||
|
||||
async def get(self, url: str) -> _FakeResponse:
|
||||
return _FakeResponse(archive_bytes)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
monkeypatch.setattr('src.langbot.pkg.api.http.service.skill.httpx.AsyncClient', _FakeAsyncClient)
|
||||
skill_service.get_skill = AsyncMock(return_value=None)
|
||||
|
||||
result = await skill_service.install_from_github(
|
||||
{
|
||||
'asset_url': 'https://api.github.com/repos/example/demo-repo/zipball/main',
|
||||
'owner': 'example',
|
||||
'repo': 'demo-repo',
|
||||
'release_tag': 'main',
|
||||
}
|
||||
)
|
||||
|
||||
expected_root = tmp_path / 'data' / 'skills' / 'demo-repo-nested-skill-main'
|
||||
assert result[0]['package_root'] == str(expected_root.resolve())
|
||||
assert (expected_root / 'SKILL.md').read_text(encoding='utf-8').endswith('Skill instructions\n')
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_install_from_github_rejects_asset_url_outside_requested_repo(skill_service, tmp_path, monkeypatch):
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
|
||||
with pytest.raises(ValueError, match='owner/repo'):
|
||||
await skill_service.install_from_github(
|
||||
{
|
||||
'asset_url': 'https://api.github.com/repos/example/other-repo/zipball/main',
|
||||
'owner': 'example',
|
||||
'repo': 'demo-repo',
|
||||
'release_tag': 'main',
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_install_from_github_rejects_zip_with_path_traversal(skill_service, tmp_path, monkeypatch):
|
||||
stream = io.BytesIO()
|
||||
with zipfile.ZipFile(stream, 'w') as archive:
|
||||
archive.writestr('../escape.txt', 'boom')
|
||||
archive_bytes = stream.getvalue()
|
||||
|
||||
class _FakeResponse:
|
||||
def __init__(self, content: bytes) -> None:
|
||||
self.content = content
|
||||
|
||||
def raise_for_status(self) -> None:
|
||||
return None
|
||||
|
||||
class _FakeAsyncClient:
|
||||
def __init__(self, *args, **kwargs) -> None:
|
||||
pass
|
||||
|
||||
async def __aenter__(self):
|
||||
return self
|
||||
|
||||
async def __aexit__(self, exc_type, exc, tb):
|
||||
return None
|
||||
|
||||
async def get(self, url: str) -> _FakeResponse:
|
||||
return _FakeResponse(archive_bytes)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
monkeypatch.setattr('src.langbot.pkg.api.http.service.skill.httpx.AsyncClient', _FakeAsyncClient)
|
||||
|
||||
with pytest.raises(ValueError, match='unsafe path'):
|
||||
await skill_service.install_from_github(
|
||||
{
|
||||
'asset_url': 'https://api.github.com/repos/example/demo-repo/zipball/main',
|
||||
'owner': 'example',
|
||||
'repo': 'demo-repo',
|
||||
'release_tag': 'main',
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_file_operations_stay_within_package_root(skill_service, tmp_path):
|
||||
skill_dir = tmp_path / 'mood-logger'
|
||||
skill_dir.mkdir()
|
||||
_create_skill_file(skill_dir / 'SKILL.md')
|
||||
(skill_dir / 'resources').mkdir()
|
||||
(skill_dir / 'resources' / 'keywords_zh.json').write_text('{"hello": 1}\n', encoding='utf-8')
|
||||
|
||||
skill_record = {
|
||||
'name': 'mood-logger',
|
||||
'package_root': str(skill_dir),
|
||||
'entry_file': 'SKILL.md',
|
||||
}
|
||||
skill_service.get_skill = AsyncMock(return_value=skill_record)
|
||||
|
||||
listed = await skill_service.list_skill_files('mood-logger', path='resources')
|
||||
assert listed['entries'] == [
|
||||
{
|
||||
'path': 'resources/keywords_zh.json',
|
||||
'name': 'keywords_zh.json',
|
||||
'is_dir': False,
|
||||
'size': os.path.getsize(skill_dir / 'resources' / 'keywords_zh.json'),
|
||||
}
|
||||
]
|
||||
|
||||
read_back = await skill_service.read_skill_file('mood-logger', 'resources/keywords_zh.json')
|
||||
assert read_back['content'] == '{"hello": 1}\n'
|
||||
|
||||
written = await skill_service.write_skill_file('mood-logger', 'resources/affinity.py', 'print("ok")\n')
|
||||
assert written['path'] == 'resources/affinity.py'
|
||||
assert (skill_dir / 'resources' / 'affinity.py').read_text(encoding='utf-8') == 'print("ok")\n'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_skill_file_operations_reject_path_traversal(skill_service, tmp_path):
|
||||
skill_dir = tmp_path / 'mood-logger'
|
||||
skill_dir.mkdir()
|
||||
_create_skill_file(skill_dir / 'SKILL.md')
|
||||
|
||||
skill_service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'mood-logger',
|
||||
'package_root': str(skill_dir),
|
||||
'entry_file': 'SKILL.md',
|
||||
}
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match='path must stay within the skill package root'):
|
||||
await skill_service.read_skill_file('mood-logger', '../outside.txt')
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_skill_rejects_package_root_change(tmp_path):
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
skill_root = tmp_path / 'data' / 'skills' / 'writer'
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'writer',
|
||||
'package_root': str(skill_root.resolve()),
|
||||
'display_name': 'Writer',
|
||||
'description': 'Writes things',
|
||||
'instructions': 'Do work',
|
||||
}
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match='Updating package_root is not supported'):
|
||||
await service.update_skill('writer', {'package_root': str(tmp_path / 'other-root')})
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_skill_removes_managed_skill_directory(tmp_path, monkeypatch):
|
||||
managed_root = tmp_path / 'data' / 'skills' / 'self-improving-agent'
|
||||
managed_root.mkdir(parents=True)
|
||||
_create_skill_file(managed_root / 'SKILL.md')
|
||||
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'self-improving-agent',
|
||||
'package_root': str(managed_root.resolve()),
|
||||
}
|
||||
)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
|
||||
result = await service.delete_skill('self-improving-agent')
|
||||
|
||||
assert result is True
|
||||
assert not managed_root.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_skill_removes_managed_install_root_for_nested_package(tmp_path, monkeypatch):
|
||||
install_root = tmp_path / 'data' / 'skills' / 'demo-repo'
|
||||
package_root = install_root / 'skills' / 'nested-skill'
|
||||
package_root.mkdir(parents=True)
|
||||
_create_skill_file(package_root / 'SKILL.md')
|
||||
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'nested-skill',
|
||||
'package_root': str(package_root.resolve()),
|
||||
}
|
||||
)
|
||||
|
||||
monkeypatch.setenv('LANGBOT_DATA_ROOT', str(tmp_path / 'data'))
|
||||
|
||||
await service.delete_skill('nested-skill')
|
||||
|
||||
assert not install_root.exists()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_skill_rejects_external_package_directory(tmp_path, monkeypatch):
|
||||
external_root = tmp_path / 'external-skills' / 'manual-skill'
|
||||
external_root.mkdir(parents=True)
|
||||
_create_skill_file(external_root / 'SKILL.md')
|
||||
|
||||
service = SkillService(SimpleNamespace(skill_mgr=SimpleNamespace(reload_skills=AsyncMock())))
|
||||
service.get_skill = AsyncMock(
|
||||
return_value={
|
||||
'name': 'manual-skill',
|
||||
'package_root': str(external_root.resolve()),
|
||||
}
|
||||
)
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
with pytest.raises(ValueError, match='Only managed skills under data/skills'):
|
||||
await service.delete_skill('manual-skill')
|
||||
with pytest.raises(ValueError, match='not initialised'):
|
||||
await service.create_skill({'name': 'x'})
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_skills_returns_empty_when_box_unavailable(self):
|
||||
"""list_skills should render an empty surface (not crash) so the
|
||||
skills page can show a banner instead of a broken state."""
|
||||
service = SkillService(self._ap_with_disabled_box())
|
||||
assert await service.list_skills() == []
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_read_skill_file_refused_when_box_unavailable(self):
|
||||
service = SkillService(self._ap_with_disabled_box())
|
||||
with pytest.raises(ValueError, match='Reading a skill file'):
|
||||
await service.read_skill_file('x', 'a.txt')
|
||||
|
||||
Reference in New Issue
Block a user