From 18ad51e21e2600aa4566ed4dfb150e2b55a30b8c Mon Sep 17 00:00:00 2001 From: Junyan Qin Date: Tue, 19 May 2026 22:56:25 +0800 Subject: [PATCH] test: repair stale skill/sandbox tests for feat/sandbox The skill subsystem moved to Tool-Call activation and a Box-managed skill store; several tests still asserted removed APIs and a sys.modules stub leaked across the suite. Full unit suite now green (was 23 failing). - test_skill_tools: drop TestSkillManagerActivation (text-marker API removed); rewrite TestSkillActivationHelper around the current skill.activation.register_activated_skill; replace the CRUD TestSkillAuthoringToolLoader with TestSkillToolLoader covering the current activate/register_skill tools and sandbox-availability gating - test_tool_manager_native: ToolManager attr is skill_tool_loader (not skill_authoring_tool_loader); native loader now exposes 6 tools (exec/read/write/edit/glob/grep) and requires initialize() with a backend-available get_status() - test_localagent_sandbox_exec: remove obsolete activation-marker leakage tests and their helper providers - test_model_service / pipeline conftest: give the mocks skill_mgr=None so PreProcessor's local-agent skill-binding guard short-circuits - test_n8nsvapi: stop permanently overwriting sys.modules ('langbot.pkg.provider.runner' etc.); save and restore around the import so other modules get the real LocalAgentRunner base class Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit_tests/pipeline/conftest.py | 3 + tests/unit_tests/pipeline/test_n8nsvapi.py | 39 +- .../provider/test_localagent_sandbox_exec.py | 202 -------- .../unit_tests/provider/test_model_service.py | 1 + tests/unit_tests/provider/test_skill_tools.py | 445 +++++++----------- .../provider/test_tool_manager_native.py | 31 +- 6 files changed, 214 insertions(+), 507 deletions(-) diff --git a/tests/unit_tests/pipeline/conftest.py b/tests/unit_tests/pipeline/conftest.py index a10e0aba..7abc5b70 100644 --- a/tests/unit_tests/pipeline/conftest.py +++ b/tests/unit_tests/pipeline/conftest.py @@ -34,6 +34,9 @@ class MockApplication: self.query_pool = self._create_mock_query_pool() self.instance_config = self._create_mock_instance_config() self.task_mgr = self._create_mock_task_manager() + # Skill manager is optional; PreProcessor only touches it for the + # local-agent runner. None keeps the skill-binding branch inert. + self.skill_mgr = None def _create_mock_logger(self): logger = Mock() diff --git a/tests/unit_tests/pipeline/test_n8nsvapi.py b/tests/unit_tests/pipeline/test_n8nsvapi.py index 68f3cdcc..b9296e99 100644 --- a/tests/unit_tests/pipeline/test_n8nsvapi.py +++ b/tests/unit_tests/pipeline/test_n8nsvapi.py @@ -14,24 +14,40 @@ import json import sys from unittest.mock import AsyncMock, MagicMock, Mock, patch -# Break the circular import chain before importing n8nsvapi: -# n8nsvapi → runner → app → pipelinemgr → all runners → runner (partially init) -_mock_runner = MagicMock() -_mock_runner.runner_class = lambda name: (lambda cls: cls) # no-op decorator -_mock_runner.RequestRunner = object -sys.modules.setdefault('langbot.pkg.provider.runner', _mock_runner) -sys.modules.setdefault('langbot.pkg.core.app', MagicMock()) -sys.modules.setdefault('langbot.pkg.utils.httpclient', MagicMock()) - import pytest import langbot_plugin.api.entities.builtin.provider.message as provider_message -from langbot.pkg.provider.runners.n8nsvapi import N8nServiceAPIRunner + +# Break the circular import chain while importing n8nsvapi: +# n8nsvapi → runner → app → pipelinemgr → all runners → runner (partially init) +# The stubs are restored immediately afterwards so this module does NOT pollute +# sys.modules for other test modules (e.g. ones importing the real +# LocalAgentRunner, which would otherwise inherit ``object`` and break). +_runner_stub = MagicMock() +_runner_stub.runner_class = lambda name: (lambda cls: cls) # no-op decorator +_runner_stub.RequestRunner = object +_import_stubs = { + 'langbot.pkg.provider.runner': _runner_stub, + 'langbot.pkg.core.app': MagicMock(), + 'langbot.pkg.utils.httpclient': MagicMock(), +} +_saved_modules = {name: sys.modules.get(name) for name in _import_stubs} +for _name, _stub in _import_stubs.items(): + sys.modules[_name] = _stub +try: + from langbot.pkg.provider.runners.n8nsvapi import N8nServiceAPIRunner +finally: + for _name, _original in _saved_modules.items(): + if _original is None: + sys.modules.pop(_name, None) + else: + sys.modules[_name] = _original # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- + def make_runner(output_key: str = 'response') -> N8nServiceAPIRunner: ap = Mock() ap.logger = Mock() @@ -74,6 +90,7 @@ async def collect_chunks(runner: N8nServiceAPIRunner, chunks: list[bytes | str]) # _process_response: stream format (type:item/end) # --------------------------------------------------------------------------- + @pytest.mark.asyncio async def test_stream_format_single_item(): """Single item + end in one chunk yields final chunk with full content.""" @@ -153,6 +170,7 @@ async def test_stream_format_no_spurious_empty_yield(): # _process_response: plain JSON fallback # --------------------------------------------------------------------------- + @pytest.mark.asyncio async def test_plain_json_with_output_key(): """Plain JSON with matching output_key extracts value via output_key.""" @@ -223,6 +241,7 @@ async def test_invalid_json_returns_raw_text(): # _call_webhook: output type depends on is_stream # --------------------------------------------------------------------------- + def make_query(is_stream: bool): """Build a minimal Query mock.""" query = Mock() diff --git a/tests/unit_tests/provider/test_localagent_sandbox_exec.py b/tests/unit_tests/provider/test_localagent_sandbox_exec.py index cd4507ae..daa4eb2d 100644 --- a/tests/unit_tests/provider/test_localagent_sandbox_exec.py +++ b/tests/unit_tests/provider/test_localagent_sandbox_exec.py @@ -91,101 +91,6 @@ class RecordingStreamProvider: return _stream() -class ActivationProvider: - def __init__(self): - self.requests: list[dict] = [] - - async def invoke_llm(self, query, model, messages, funcs, extra_args=None, remove_think=None): - self.requests.append( - { - 'messages': list(messages), - 'funcs': list(funcs), - 'remove_think': remove_think, - } - ) - if len(self.requests) == 1: - return provider_message.Message( - role='assistant', - content='[ACTIVATE_SKILL: demo]\nI will use the skill.', - ) - return provider_message.Message( - role='assistant', - content='final answer after activation', - ) - - -class FailingActivationProvider: - def __init__(self): - self.requests: list[dict] = [] - - async def invoke_llm(self, query, model, messages, funcs, extra_args=None, remove_think=None): - self.requests.append( - { - 'messages': list(messages), - 'funcs': list(funcs), - 'remove_think': remove_think, - } - ) - if len(self.requests) == 1: - return provider_message.Message( - role='assistant', - content='[ACTIVATE_SKILL: demo]\nI will use the skill.', - ) - raise RuntimeError('activation failed') - - -class ActivationStreamProvider: - def __init__(self): - self.stream_requests: list[dict] = [] - - def invoke_llm_stream(self, query, model, messages, funcs, extra_args=None, remove_think=None): - self.stream_requests.append( - { - 'messages': list(messages), - 'funcs': list(funcs), - 'remove_think': remove_think, - } - ) - - async def _stream(): - if len(self.stream_requests) == 1: - yield provider_message.MessageChunk( - role='assistant', - content='[ACTIVATE_SKILL: demo]\nI will use the skill.', - is_final=True, - ) - return - - yield provider_message.MessageChunk( - role='assistant', - content='final answer after activation', - is_final=True, - ) - - return _stream() - - -def make_skill_manager(): - skill_data = { - 'uuid': 'skill-demo', - 'name': 'demo', - 'instructions': 'Do the demo task.', - 'type': 'skill', - 'package_root': '/tmp/demo-skill', - 'sandbox_timeout_sec': 120, - 'sandbox_network': False, - } - return SimpleNamespace( - SKILL_ACTIVATION_MARKER='[ACTIVATE_SKILL:', - detect_skill_activations=Mock( - side_effect=lambda content: ['demo'] if '[ACTIVATE_SKILL: demo]' in (content or '') else [] - ), - build_activation_prompt_for_skills=Mock(return_value='skill prompt'), - get_skill_by_name=Mock(side_effect=lambda name: skill_data if name == 'demo' else None), - remove_activation_marker=Mock(side_effect=lambda content: (content or '').replace('[ACTIVATE_SKILL: demo]\n', '')), - ) - - def make_query() -> pipeline_query.Query: adapter = AsyncMock() adapter.is_stream_output_supported = AsyncMock(return_value=False) @@ -335,110 +240,3 @@ async def test_localagent_streaming_tool_error_yields_message_chunks(): assert all(isinstance(message, provider_message.MessageChunk) for message in results) assert any(message.role == 'tool' and message.content == 'err: boom' for message in results) - - -@pytest.mark.asyncio -async def test_localagent_hides_activation_marker_before_follow_up_request(): - provider = ActivationProvider() - model = SimpleNamespace( - provider=provider, - model_entity=SimpleNamespace( - uuid='test-model-uuid', - name='test-model', - abilities=['func_call'], - extra_args={}, - ), - ) - - app = SimpleNamespace( - logger=Mock(), - model_mgr=SimpleNamespace(get_model_by_uuid=AsyncMock(return_value=model)), - tool_mgr=SimpleNamespace(execute_func_call=AsyncMock()), - rag_mgr=SimpleNamespace(), - box_service=SimpleNamespace(get_system_guidance=Mock(return_value='sandbox guidance')), - skill_mgr=make_skill_manager(), - ) - - runner = LocalAgentRunner(app, pipeline_config={}) - query = make_query() - query.use_funcs = [] - - results = [message async for message in runner.run(query)] - - assert [(message.role, message.content) for message in results] == [ - ('assistant', 'final answer after activation') - ] - assert len(provider.requests) == 2 - assert provider.requests[1]['messages'][-2].content == 'I will use the skill.' - assert '[ACTIVATE_SKILL:' not in provider.requests[1]['messages'][-2].content - - -@pytest.mark.asyncio -async def test_localagent_activation_failure_rolls_back_query_state_and_sanitizes_response(): - provider = FailingActivationProvider() - model = SimpleNamespace( - provider=provider, - model_entity=SimpleNamespace( - uuid='test-model-uuid', - name='test-model', - abilities=['func_call'], - extra_args={}, - ), - ) - - app = SimpleNamespace( - logger=Mock(), - model_mgr=SimpleNamespace(get_model_by_uuid=AsyncMock(return_value=model)), - tool_mgr=SimpleNamespace(execute_func_call=AsyncMock()), - rag_mgr=SimpleNamespace(), - box_service=SimpleNamespace(get_system_guidance=Mock(return_value='sandbox guidance')), - skill_mgr=make_skill_manager(), - ) - - runner = LocalAgentRunner(app, pipeline_config={}) - query = make_query() - query.use_funcs = [] - - results = [message async for message in runner.run(query)] - - assert [(message.role, message.content) for message in results] == [ - ('assistant', 'I will use the skill.') - ] - assert query.use_funcs == [] - assert query.variables == {} - - -@pytest.mark.asyncio -async def test_localagent_streaming_activation_does_not_leak_marker(): - provider = ActivationStreamProvider() - model = SimpleNamespace( - provider=provider, - model_entity=SimpleNamespace( - uuid='test-model-uuid', - name='test-model', - abilities=['func_call'], - extra_args={}, - ), - ) - - adapter = AsyncMock() - adapter.is_stream_output_supported = AsyncMock(return_value=True) - - app = SimpleNamespace( - logger=Mock(), - model_mgr=SimpleNamespace(get_model_by_uuid=AsyncMock(return_value=model)), - tool_mgr=SimpleNamespace(execute_func_call=AsyncMock()), - rag_mgr=SimpleNamespace(), - box_service=SimpleNamespace(get_system_guidance=Mock(return_value='sandbox guidance')), - skill_mgr=make_skill_manager(), - ) - - runner = LocalAgentRunner(app, pipeline_config={}) - query = make_query() - query.adapter = adapter - query.use_funcs = [] - - results = [message async for message in runner.run(query)] - - assert all(isinstance(message, provider_message.MessageChunk) for message in results) - assert [message.content for message in results] == ['final answer after activation'] diff --git a/tests/unit_tests/provider/test_model_service.py b/tests/unit_tests/provider/test_model_service.py index 8fac8278..d37834ce 100644 --- a/tests/unit_tests/provider/test_model_service.py +++ b/tests/unit_tests/provider/test_model_service.py @@ -69,6 +69,7 @@ async def test_updated_llm_model_is_immediately_usable_by_local_agent_pipeline() ap.logger = Mock() ap.persistence_mgr = SimpleNamespace(execute_async=AsyncMock()) ap.tool_mgr = SimpleNamespace(get_all_tools=AsyncMock(return_value=[])) + ap.skill_mgr = None # PreProcessor only uses skill_mgr for the local-agent skill-binding branch ap.plugin_connector = SimpleNamespace( emit_event=AsyncMock(return_value=SimpleNamespace(event=SimpleNamespace(default_prompt=[], prompt=[]))) ) diff --git a/tests/unit_tests/provider/test_skill_tools.py b/tests/unit_tests/provider/test_skill_tools.py index 867d5dc1..5c8f901f 100644 --- a/tests/unit_tests/provider/test_skill_tools.py +++ b/tests/unit_tests/provider/test_skill_tools.py @@ -79,52 +79,18 @@ class TestSkillManagerPackageLoading: assert skill_data['description'] == 'Second' -class TestSkillManagerActivation: - def test_detect_skill_activations_returns_unique_ordered_skills(self): - from langbot.pkg.skill.manager import SkillManager - - ap = _make_ap() - mgr = SkillManager(ap) - mgr.skills = { - 'alpha': _make_skill_data(name='alpha'), - 'beta': _make_skill_data(name='beta'), - } - - response = '[ACTIVATE_SKILL: alpha]\n[ACTIVATE_SKILL: beta]\n[ACTIVATE_SKILL: alpha]\nLet me handle this.' - - assert mgr.detect_skill_activations(response) == ['alpha', 'beta'] - assert mgr.detect_skill_activation(response) == 'alpha' - - def test_build_activation_prompt_for_skills_includes_runtime_guidance(self): - from langbot.pkg.skill.manager import SkillManager - - ap = _make_ap() - mgr = SkillManager(ap) - mgr.skills = { - 'primary': _make_skill_data(name='primary', instructions='Primary instructions'), - 'aux': _make_skill_data(name='aux', instructions='Aux instructions'), - } - - prompt = mgr.build_activation_prompt_for_skills(['primary', 'aux']) - - assert 'Activated skills: primary, aux' in prompt - assert 'role="primary"' in prompt - assert 'role="auxiliary"' in prompt - assert '/workspace/.skills/' in prompt - - def test_remove_activation_marker_removes_multiple_markers(self): - from langbot.pkg.skill.manager import SkillManager - - ap = _make_ap() - mgr = SkillManager(ap) - - response = '[ACTIVATE_SKILL: alpha]\n[ACTIVATE_SKILL: beta]\nFinal answer' - assert mgr.remove_activation_marker(response) == 'Final answer' - - class TestSkillActivationHelper: - def test_prepare_skill_activation_registers_only_explicit_activated_skills(self): - from langbot.pkg.skill.activation import prepare_skill_activation + """Skill activation is now Tool-Call based. + + The legacy text-marker mechanism (``[ACTIVATE_SKILL: x]`` detection, + ``build_activation_prompt_for_skills``, ``remove_activation_marker``, + ``prepare_skill_activation``) has been removed. Activation now goes + through ``skill.activation.register_activated_skill``, invoked by the + ``activate`` Tool Call. + """ + + def test_register_activated_skill_records_known_skill(self): + from langbot.pkg.skill.activation import register_activated_skill from langbot.pkg.provider.tools.loaders.skill import ACTIVATED_SKILLS_KEY from langbot.pkg.skill.manager import SkillManager @@ -132,45 +98,37 @@ class TestSkillActivationHelper: mgr = SkillManager(ap) mgr.skills = { 'primary': _make_skill_data(name='primary', instructions='Primary instructions'), - 'aux': _make_skill_data(name='aux', instructions='Aux instructions'), } ap.skill_mgr = mgr - query = SimpleNamespace(variables={}, use_funcs=[]) - activation = prepare_skill_activation( - ap, - query, - '[ACTIVATE_SKILL: primary]\n[ACTIVATE_SKILL: aux]\nWorking on it.', - ) + query = SimpleNamespace(variables={}) - assert activation is not None - assert activation.activated_skill_names == ['primary', 'aux'] - assert activation.cleaned_content == 'Working on it.' - assert set(query.variables[ACTIVATED_SKILLS_KEY].keys()) == {'primary', 'aux'} + assert register_activated_skill(ap, query, 'primary') is True + assert set(query.variables[ACTIVATED_SKILLS_KEY].keys()) == {'primary'} + assert query.variables[ACTIVATED_SKILLS_KEY]['primary']['name'] == 'primary' - def test_prepare_skill_activation_ignores_skills_not_bound_to_pipeline(self): - from langbot.pkg.skill.activation import prepare_skill_activation - from langbot.pkg.provider.tools.loaders.skill import ACTIVATED_SKILLS_KEY, PIPELINE_BOUND_SKILLS_KEY + def test_register_activated_skill_rejects_unknown_skill(self): + from langbot.pkg.skill.activation import register_activated_skill + from langbot.pkg.provider.tools.loaders.skill import ACTIVATED_SKILLS_KEY from langbot.pkg.skill.manager import SkillManager ap = _make_ap() mgr = SkillManager(ap) - mgr.skills = { - 'primary': _make_skill_data(name='primary', instructions='Primary instructions'), - 'hidden': _make_skill_data(name='hidden', instructions='Hidden instructions'), - } + mgr.skills = {'primary': _make_skill_data(name='primary')} ap.skill_mgr = mgr - query = SimpleNamespace(variables={PIPELINE_BOUND_SKILLS_KEY: ['primary']}, use_funcs=[]) - activation = prepare_skill_activation( - ap, - query, - '[ACTIVATE_SKILL: hidden]\n[ACTIVATE_SKILL: primary]\nWorking on it.', - ) + query = SimpleNamespace(variables={}) - assert activation is not None - assert activation.activated_skill_names == ['primary'] - assert set(query.variables[ACTIVATED_SKILLS_KEY].keys()) == {'primary'} + assert register_activated_skill(ap, query, 'missing') is False + assert ACTIVATED_SKILLS_KEY not in query.variables + + def test_register_activated_skill_without_skill_manager_returns_false(self): + from langbot.pkg.skill.activation import register_activated_skill + + ap = _make_ap() # no skill_mgr attribute + query = SimpleNamespace(variables={}) + + assert register_activated_skill(ap, query, 'primary') is False class TestSkillPathHelpers: @@ -247,199 +205,97 @@ class TestSkillPathHelpers: assert command.rstrip().endswith('python scripts/run.py') -class TestSkillAuthoringToolLoader: +class TestSkillToolLoader: + """The skill tool surface is now just ``activate`` + ``register_skill``. + + The legacy CRUD authoring tools (create/list/get/update/delete/ + import_skill_from_directory/reload_skills) were removed; skill CRUD is + handled by SkillService via the HTTP API / web UI instead. + """ + @pytest.mark.asyncio - async def test_create_skill_creates_managed_prompt_only_skill(self): + async def test_activate_returns_instructions_and_registers_skill(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( - CREATE_SKILL_TOOL_NAME, - SkillAuthoringToolLoader, + ACTIVATE_SKILL_TOOL_NAME, + SkillToolLoader, + ) + from langbot.pkg.provider.tools.loaders.skill import ACTIVATED_SKILLS_KEY + + skill = _make_skill_data(name='demo', package_root='/data/skills/demo', instructions='Step 1') + ap = _make_ap() + ap.skill_mgr = SimpleNamespace( + skills={'demo': skill}, + get_skill_by_name=lambda name: skill if name == 'demo' else None, + ) + + loader = SkillToolLoader(ap) + query = SimpleNamespace(variables={}) + + result = await loader.invoke_tool(ACTIVATE_SKILL_TOOL_NAME, {'skill_name': 'demo'}, query) + + assert result['activated'] is True + assert result['skill_name'] == 'demo' + assert result['mount_path'] == '/workspace/.skills/demo' + assert 'Step 1' in result['content'] + assert set(query.variables[ACTIVATED_SKILLS_KEY].keys()) == {'demo'} + + @pytest.mark.asyncio + async def test_activate_unknown_skill_raises(self): + from langbot.pkg.provider.tools.loaders.skill_authoring import ( + ACTIVATE_SKILL_TOOL_NAME, + SkillToolLoader, ) ap = _make_ap() - ap.skill_service = SimpleNamespace( - create_skill=AsyncMock( - return_value=_make_skill_data(name='prompt-skill', package_root='/data/skills/prompt-skill') - ), - reload_skills=AsyncMock(), - list_skills=AsyncMock(return_value=[]), + ap.skill_mgr = SimpleNamespace( + skills={'demo': _make_skill_data(name='demo')}, + get_skill_by_name=lambda name: None, ) - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() + loader = SkillToolLoader(ap) - result = await loader.invoke_tool( - CREATE_SKILL_TOOL_NAME, - { - 'name': 'prompt-skill', - 'display_name': 'Prompt Skill', - 'description': 'Prompt only skill', - 'instructions': 'Follow these steps carefully.', - }, - SimpleNamespace(), - ) - - ap.skill_service.create_skill.assert_awaited_once_with( - { - 'name': 'prompt-skill', - 'display_name': 'Prompt Skill', - 'description': 'Prompt only skill', - 'instructions': 'Follow these steps carefully.', - } - ) - assert result == { - 'created': True, - 'skill': _make_skill_data(name='prompt-skill', package_root='/data/skills/prompt-skill'), - } + with pytest.raises(ValueError, match='not found'): + await loader.invoke_tool( + ACTIVATE_SKILL_TOOL_NAME, + {'skill_name': 'ghost'}, + SimpleNamespace(variables={}), + ) @pytest.mark.asyncio - async def test_list_skills_returns_managed_skills(self): + async def test_register_skill_scans_directory_and_creates_skill(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( - LIST_SKILLS_TOOL_NAME, - SkillAuthoringToolLoader, + REGISTER_SKILL_TOOL_NAME, + SkillToolLoader, ) - ap = _make_ap() - ap.skill_service = SimpleNamespace( - list_skills=AsyncMock(return_value=[_make_skill_data(name='alpha'), _make_skill_data(name='beta')]), - ) - - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() - - result = await loader.invoke_tool(LIST_SKILLS_TOOL_NAME, {}, SimpleNamespace()) - - assert result == { - 'skills': [_make_skill_data(name='alpha'), _make_skill_data(name='beta')], - 'skill_names': ['alpha', 'beta'], - 'count': 2, - } - - @pytest.mark.asyncio - async def test_get_skill_returns_one_managed_skill(self): - from langbot.pkg.provider.tools.loaders.skill_authoring import ( - GET_SKILL_TOOL_NAME, - SkillAuthoringToolLoader, - ) - - ap = _make_ap() - ap.skill_service = SimpleNamespace( - get_skill=AsyncMock(return_value=_make_skill_data(name='time-now', package_root='/data/skills/time-now')), - ) - - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() - - result = await loader.invoke_tool(GET_SKILL_TOOL_NAME, {'name': 'time-now'}, SimpleNamespace()) - - ap.skill_service.get_skill.assert_awaited_once_with('time-now') - assert result == { - 'skill': _make_skill_data(name='time-now', package_root='/data/skills/time-now'), - } - - @pytest.mark.asyncio - async def test_update_skill_updates_managed_prompt_only_skill(self): - from langbot.pkg.provider.tools.loaders.skill_authoring import ( - UPDATE_SKILL_TOOL_NAME, - SkillAuthoringToolLoader, - ) - - ap = _make_ap() - ap.skill_service = SimpleNamespace( - create_skill=AsyncMock(), - update_skill=AsyncMock( - return_value=_make_skill_data(name='time-now', package_root='/data/skills/time-now') - ), - reload_skills=AsyncMock(), - list_skills=AsyncMock(return_value=[]), - ) - - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() - - result = await loader.invoke_tool( - UPDATE_SKILL_TOOL_NAME, - { - 'name': 'time-now', - 'description': 'Fixed to Beijing time', - 'instructions': 'Always use Asia/Shanghai and never offer other timezones.', - }, - SimpleNamespace(), - ) - - ap.skill_service.update_skill.assert_awaited_once_with( - 'time-now', - { - 'name': 'time-now', - 'description': 'Fixed to Beijing time', - 'instructions': 'Always use Asia/Shanghai and never offer other timezones.', - }, - ) - assert result == { - 'updated': True, - 'skill': _make_skill_data(name='time-now', package_root='/data/skills/time-now'), - } - - @pytest.mark.asyncio - async def test_delete_skill_deletes_managed_skill(self): - from langbot.pkg.provider.tools.loaders.skill_authoring import ( - DELETE_SKILL_TOOL_NAME, - SkillAuthoringToolLoader, - ) - - ap = _make_ap() - ap.skill_service = SimpleNamespace( - delete_skill=AsyncMock(return_value=True), - ) - - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() - - result = await loader.invoke_tool(DELETE_SKILL_TOOL_NAME, {'name': 'time-now'}, SimpleNamespace()) - - ap.skill_service.delete_skill.assert_awaited_once_with('time-now') - assert result == { - 'deleted': True, - 'skill_name': 'time-now', - } - - @pytest.mark.asyncio - async def test_import_skill_from_directory_uses_workspace_path_and_service_import(self): - from langbot.pkg.provider.tools.loaders.skill_authoring import ( - IMPORT_SKILL_FROM_DIRECTORY_TOOL_NAME, - SkillAuthoringToolLoader, - ) - - ap = _make_ap() - ap.box_service = SimpleNamespace(default_workspace='/tmp/langbot-workspace') - ap.skill_service = SimpleNamespace( - scan_directory=Mock( - return_value={ - 'name': 'cloned-skill', - 'display_name': 'Cloned Skill', - 'description': 'Imported from clone', - 'instructions': 'Do work', - } - ), - create_skill=AsyncMock(return_value=_make_skill_data(name='cloned-skill', package_root='/repo/root')), - reload_skills=AsyncMock(), - list_skills=AsyncMock(return_value=[]), - ) - - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() - with tempfile.TemporaryDirectory() as tmpdir: - ap.box_service.default_workspace = tmpdir - repo_dir = os.path.join(tmpdir, 'repos', 'cloned-skill') + repo_dir = os.path.join(tmpdir, 'repo') os.makedirs(repo_dir) + ap = _make_ap() + ap.box_service = SimpleNamespace(default_workspace=tmpdir, available=True) + ap.skill_service = SimpleNamespace( + scan_directory_async=AsyncMock( + return_value={ + 'name': 'cloned-skill', + 'display_name': 'Cloned Skill', + 'description': 'Imported from clone', + 'instructions': 'Do work', + } + ), + create_skill=AsyncMock( + return_value=_make_skill_data(name='cloned-skill', package_root=os.path.realpath(repo_dir)) + ), + ) + + loader = SkillToolLoader(ap) result = await loader.invoke_tool( - IMPORT_SKILL_FROM_DIRECTORY_TOOL_NAME, - {'path': '/workspace/repos/cloned-skill'}, + REGISTER_SKILL_TOOL_NAME, + {'path': '/workspace/repo'}, SimpleNamespace(), ) - ap.skill_service.scan_directory.assert_called_once_with(os.path.realpath(repo_dir)) + ap.skill_service.scan_directory_async.assert_awaited_once_with(os.path.realpath(repo_dir)) ap.skill_service.create_skill.assert_awaited_once_with( { 'name': 'cloned-skill', @@ -449,59 +305,88 @@ class TestSkillAuthoringToolLoader: 'package_root': os.path.realpath(repo_dir), } ) - assert result['imported'] is True - assert result['source_path'] == '/workspace/repos/cloned-skill' + assert result['registered'] is True + assert result['skill_name'] == 'cloned-skill' + assert result['source_path'] == '/workspace/repo' @pytest.mark.asyncio - async def test_import_skill_from_directory_rejects_workspace_escape(self): + async def test_register_skill_rejects_workspace_escape(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( - IMPORT_SKILL_FROM_DIRECTORY_TOOL_NAME, - SkillAuthoringToolLoader, + REGISTER_SKILL_TOOL_NAME, + SkillToolLoader, ) - ap = _make_ap() - ap.box_service = SimpleNamespace(default_workspace='/tmp/langbot-workspace') - ap.skill_service = SimpleNamespace( - scan_directory=Mock(), - create_skill=AsyncMock(), - reload_skills=AsyncMock(), - list_skills=AsyncMock(return_value=[]), - ) + with tempfile.TemporaryDirectory() as tmpdir: + ap = _make_ap() + ap.box_service = SimpleNamespace(default_workspace=tmpdir, available=True) + ap.skill_service = SimpleNamespace(scan_directory_async=AsyncMock(), create_skill=AsyncMock()) - loader = SkillAuthoringToolLoader(ap) - await loader.initialize() + loader = SkillToolLoader(ap) - with pytest.raises(ValueError, match='escapes the workspace boundary'): - await loader.invoke_tool( - IMPORT_SKILL_FROM_DIRECTORY_TOOL_NAME, - {'path': '/workspace/../../etc'}, - SimpleNamespace(), - ) + with pytest.raises(ValueError, match='escapes the workspace boundary'): + await loader.invoke_tool( + REGISTER_SKILL_TOOL_NAME, + {'path': '/workspace/../../etc'}, + SimpleNamespace(), + ) @pytest.mark.asyncio - async def test_reload_skills_rescans_filesystem_and_returns_current_names(self): + async def test_register_skill_requires_skill_service(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( - RELOAD_SKILLS_TOOL_NAME, - SkillAuthoringToolLoader, + REGISTER_SKILL_TOOL_NAME, + SkillToolLoader, ) + with tempfile.TemporaryDirectory() as tmpdir: + ap = _make_ap() # no skill_service attribute + ap.box_service = SimpleNamespace(default_workspace=tmpdir, available=True) + + loader = SkillToolLoader(ap) + + with pytest.raises(ValueError, match='Skill service not available'): + await loader.invoke_tool( + REGISTER_SKILL_TOOL_NAME, + {'path': '/workspace/foo'}, + SimpleNamespace(), + ) + + @pytest.mark.asyncio + async def test_tools_hidden_when_sandbox_backend_unavailable(self): + from langbot.pkg.provider.tools.loaders.skill_authoring import SkillToolLoader + ap = _make_ap() - ap.skill_service = SimpleNamespace( - reload_skills=AsyncMock(), - list_skills=AsyncMock(return_value=[_make_skill_data(name='alpha'), _make_skill_data(name='beta')]), + ap.skill_mgr = SimpleNamespace(skills={}) + ap.box_service = SimpleNamespace( + available=True, + get_status=AsyncMock(return_value={'backend': {'available': False}}), ) - loader = SkillAuthoringToolLoader(ap) + loader = SkillToolLoader(ap) await loader.initialize() - result = await loader.invoke_tool(RELOAD_SKILLS_TOOL_NAME, {}, SimpleNamespace()) + assert await loader.get_tools() == [] + assert await loader.has_tool('activate') is False + assert await loader.has_tool('register_skill') is False - assert result == { - 'reloaded': True, - 'skill_names': ['alpha', 'beta'], - 'count': 2, - } - ap.skill_service.reload_skills.assert_awaited_once_with() + @pytest.mark.asyncio + async def test_tools_exposed_when_sandbox_backend_available(self): + from langbot.pkg.provider.tools.loaders.skill_authoring import SkillToolLoader + + ap = _make_ap() + ap.skill_mgr = SimpleNamespace(skills={'demo': _make_skill_data(name='demo')}) + ap.box_service = SimpleNamespace( + available=True, + get_status=AsyncMock(return_value={'backend': {'available': True}}), + ) + + loader = SkillToolLoader(ap) + await loader.initialize() + + tools = await loader.get_tools() + + assert sorted(tool.name for tool in tools) == ['activate', 'register_skill'] + assert await loader.has_tool('activate') is True + assert await loader.has_tool('register_skill') is True class TestNativeToolLoaderSkillPaths: diff --git a/tests/unit_tests/provider/test_tool_manager_native.py b/tests/unit_tests/provider/test_tool_manager_native.py index 2c6bf503..117a20fd 100644 --- a/tests/unit_tests/provider/test_tool_manager_native.py +++ b/tests/unit_tests/provider/test_tool_manager_native.py @@ -3,7 +3,7 @@ from __future__ import annotations import os import tempfile from types import SimpleNamespace -from unittest.mock import Mock +from unittest.mock import AsyncMock, Mock import pytest @@ -45,7 +45,7 @@ def make_tool(name: str) -> resource_tool.LLMTool: async def test_tool_manager_omits_skill_authoring_tools_by_default(): manager = ToolManager(SimpleNamespace()) manager.native_tool_loader = StubLoader([make_tool('exec')]) - manager.skill_authoring_tool_loader = StubLoader([make_tool('reload_skills')]) + manager.skill_tool_loader = StubLoader([make_tool('activate')]) manager.plugin_tool_loader = StubLoader([make_tool('plugin_tool')]) manager.mcp_tool_loader = StubLoader([make_tool('mcp_tool')]) @@ -58,13 +58,13 @@ async def test_tool_manager_omits_skill_authoring_tools_by_default(): async def test_tool_manager_includes_skill_authoring_tools_when_requested(): manager = ToolManager(SimpleNamespace()) manager.native_tool_loader = StubLoader([make_tool('exec')]) - manager.skill_authoring_tool_loader = StubLoader([make_tool('reload_skills')]) + manager.skill_tool_loader = StubLoader([make_tool('activate')]) manager.plugin_tool_loader = StubLoader([make_tool('plugin_tool')]) manager.mcp_tool_loader = StubLoader([make_tool('mcp_tool')]) tools = await manager.get_all_tools(include_skill_authoring=True) - assert [tool.name for tool in tools] == ['exec', 'reload_skills', 'plugin_tool', 'mcp_tool'] + assert [tool.name for tool in tools] == ['exec', 'activate', 'plugin_tool', 'mcp_tool'] @pytest.mark.asyncio @@ -72,7 +72,7 @@ async def test_tool_manager_routes_native_tool_calls(): app = SimpleNamespace() manager = ToolManager(app) manager.native_tool_loader = StubLoader([make_tool('exec')], invoke_result={'backend': 'fake'}) - manager.skill_authoring_tool_loader = StubLoader([make_tool('reload_skills')]) + manager.skill_tool_loader = StubLoader([make_tool('activate')]) manager.plugin_tool_loader = StubLoader([make_tool('plugin_tool')]) manager.mcp_tool_loader = StubLoader([make_tool('mcp_tool')]) @@ -86,23 +86,24 @@ async def test_native_tool_loader_hides_tools_when_box_unavailable(): loader = NativeToolLoader(SimpleNamespace(box_service=SimpleNamespace(available=False))) assert await loader.get_tools() == [] - assert await loader.has_tool('exec') is False - assert await loader.has_tool('read') is False - assert await loader.has_tool('write') is False - assert await loader.has_tool('edit') is False + for tool_name in ('exec', 'read', 'write', 'edit', 'glob', 'grep'): + assert await loader.has_tool(tool_name) is False @pytest.mark.asyncio async def test_native_tool_loader_exposes_all_tools_when_box_available(): - loader = NativeToolLoader(SimpleNamespace(box_service=SimpleNamespace(available=True))) + box_service = SimpleNamespace( + available=True, + get_status=AsyncMock(return_value={'backend': {'available': True}}), + ) + loader = NativeToolLoader(SimpleNamespace(box_service=box_service, logger=Mock())) + await loader.initialize() tools = await loader.get_tools() - assert [tool.name for tool in tools] == ['exec', 'read', 'write', 'edit'] - assert await loader.has_tool('exec') is True - assert await loader.has_tool('read') is True - assert await loader.has_tool('write') is True - assert await loader.has_tool('edit') is True + assert [tool.name for tool in tools] == ['exec', 'read', 'write', 'edit', 'glob', 'grep'] + for tool_name in ('exec', 'read', 'write', 'edit', 'glob', 'grep'): + assert await loader.has_tool(tool_name) is True # ── read/write/edit file tool tests ─────────────────────────────