From 4d438f88211e0c677ce59e094fe43547ba577e17 Mon Sep 17 00:00:00 2001 From: huanghuoguoguo <60681390+huanghuoguoguo@users.noreply.github.com> Date: Sat, 12 Sep 2026 13:04:11 +0800 Subject: [PATCH] feat(skill): pin run revision bindings --- ARCHITECTURE.md | 13 +- .../pkg/api/http/controller/groups/skills.py | 14 +- src/langbot/pkg/api/http/service/skill.py | 43 ++- .../pkg/provider/tools/loaders/native.py | 74 ++--- .../pkg/provider/tools/loaders/skill.py | 97 ++++-- .../provider/tools/loaders/skill_authoring.py | 21 +- src/langbot/pkg/skill/activation.py | 44 --- src/langbot/pkg/skill/repository.py | 66 +++- .../test_cloud_box_admission_integration.py | 20 +- tests/unit_tests/box/test_box_service.py | 79 +++-- tests/unit_tests/provider/test_skill_tools.py | 295 ++++++++++++------ tests/unit_tests/test_skill_repository.py | 130 +++++++- tests/unit_tests/test_skill_service.py | 35 ++- .../components/skill-form/SkillForm.tsx | 5 +- web/src/app/infra/entities/api/index.ts | 1 + web/src/app/infra/http/BackendClient.ts | 4 +- 16 files changed, 657 insertions(+), 284 deletions(-) delete mode 100644 src/langbot/pkg/skill/activation.py diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index f2780dfb0..732a3045b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -178,7 +178,7 @@ In this repo: - `pkg/provider/tools/loaders/native.py` is the Core orchestration seam: the Skill loader supplies generic read-only mounts to Box execution. `mcp_stdio.py` and execution-backed tools depend on Box availability. -- `pkg/skill/repository.py` is the thin async/Workspace adapter over the Plugin SDK's execution-independent `SkillStore`; `skills.root` owns its location independently of Box. +- `pkg/skill/repository.py` is the thin async/Workspace adapter over the Plugin SDK's execution-independent `SkillStore`; `skills.root` owns its location independently of Box. Core's registry points to immutable content-addressed publications, and every update carries `base_revision` so concurrent writers fail instead of overwriting each other. - `pkg/skill/manager.py` caches the Core repository catalog for progressive disclosure. Activation and read-only resource tools do not require Box; script execution and Workspace mutation still do. Durable Box Workspace storage is shared across placement generations, but @@ -192,9 +192,14 @@ In `langbot-plugin-sdk`: - `src/langbot_plugin/box/server.py` implements `lbp box` and the WebSocket endpoints on `:5410`. - `src/langbot_plugin/box/runtime.py` owns sandbox sessions, generic read-only mounts, and managed processes. - `backend.py`, `nsjail_backend.py`, and `e2b_backend.py` implement sandbox backends. -- `src/langbot_plugin/skill_store.py` is consumed by Core, not Box. Core turns - selected package roots into generic read-only mounts; Box does not understand - Skill names, metadata, revisions, files, or CRUD. +- `src/langbot_plugin/skill_store.py` is consumed by Core, not Box. Activation + pins the first revision for a run; instructions, resource reads, restart + recovery, execution mounts, and revision-scoped dependency state all use that + exact publication. Core mounts only activated and authorized Skills. Published + trees are never agent write targets: authoring happens in Workspace drafts and + an explicit registration/update atomically publishes the next revision. Core + turns those package roots and content digests into generic read-only mounts; + Box does not understand Skill names, metadata, revisions, files, or CRUD. Skill storage uses `skills.root`. Box execution config lives under `box:`: `box.enabled`, `box.backend`, `box.runtime.endpoint`, and `box.local.*`. The old diff --git a/src/langbot/pkg/api/http/controller/groups/skills.py b/src/langbot/pkg/api/http/controller/groups/skills.py index 995146610..77d35b601 100644 --- a/src/langbot/pkg/api/http/controller/groups/skills.py +++ b/src/langbot/pkg/api/http/controller/groups/skills.py @@ -4,6 +4,7 @@ import quart from ...authz import Permission from ...context import RequestContext +from .....skill.repository import SkillRevisionConflictError from .. import group @@ -69,6 +70,8 @@ class SkillsRouterGroup(group.RouterGroup): try: skill = await self.ap.skill_service.update_skill(request_context, skill_name, data) return self.success(data={'skill': skill}) + except SkillRevisionConflictError as exc: + return self.http_status(409, -1, str(exc)) except ValueError as exc: return self.http_status(400, -1, str(exc)) @@ -97,6 +100,8 @@ class SkillsRouterGroup(group.RouterGroup): include_hidden=include_hidden, ) return self.success(data=result) + except SkillRevisionConflictError as exc: + return self.http_status(409, -1, str(exc)) except ValueError as exc: return self.http_status(400, -1, str(exc)) @@ -122,11 +127,18 @@ class SkillsRouterGroup(group.RouterGroup): async def write_skill_file(skill_name: str, path: str, request_context: RequestContext) -> quart.Response: data = await quart.request.json content = data.get('content', '') + base_revision = str(data.get('base_revision', '') or '').strip() or None if content is None: return self.http_status(400, -1, 'Missing required field: content') try: - result = await self.ap.skill_service.write_skill_file(request_context, skill_name, path, content) + result = await self.ap.skill_service.write_skill_file( + request_context, + skill_name, + path, + content, + base_revision=base_revision, + ) return self.success(data=result) except ValueError as exc: return self.http_status(400, -1, str(exc)) diff --git a/src/langbot/pkg/api/http/service/skill.py b/src/langbot/pkg/api/http/service/skill.py index fcd4e5313..bc613e7dc 100644 --- a/src/langbot/pkg/api/http/service/skill.py +++ b/src/langbot/pkg/api/http/service/skill.py @@ -103,15 +103,34 @@ class SkillService: await self._reload_skills(execution_context) return self._serialize_skill(created) - async def import_skill_directory(self, context: TenantContext, path: str, data: dict) -> dict: + async def import_skill_directory( + self, + context: TenantContext, + path: str, + data: dict, + *, + base_revision: str | None = None, + ) -> dict: execution_context = await self._execution_context(context) - created = await self._repository().import_skill_directory(execution_context, path, data) + created = await self._repository().import_skill_directory( + execution_context, + path, + data, + base_revision=base_revision, + ) await self._reload_skills(execution_context) return self._serialize_skill(created) async def update_skill(self, context: TenantContext, skill_name: str, data: dict) -> dict: execution_context = await self._execution_context(context) - updated = await self._repository().update_skill(execution_context, skill_name, data) + payload = dict(data) + base_revision = str(payload.pop('base_revision', '') or '').strip() or None + updated = await self._repository().update_skill( + execution_context, + skill_name, + payload, + base_revision=base_revision, + ) await self._reload_skills(execution_context) return self._serialize_skill(updated) @@ -142,9 +161,23 @@ class SkillService: execution_context = await self._execution_context(context) return await self._repository().read_skill_file(execution_context, skill_name, path) - async def write_skill_file(self, context: TenantContext, skill_name: str, path: str, content: str) -> dict: + async def write_skill_file( + self, + context: TenantContext, + skill_name: str, + path: str, + content: str, + *, + base_revision: str | None, + ) -> dict: execution_context = await self._execution_context(context) - result = await self._repository().write_skill_file(execution_context, skill_name, path, content) + result = await self._repository().write_skill_file( + execution_context, + skill_name, + path, + content, + base_revision=base_revision, + ) await self._reload_skills(execution_context) return result diff --git a/src/langbot/pkg/provider/tools/loaders/native.py b/src/langbot/pkg/provider/tools/loaders/native.py index e3ada52af..2bae17c21 100644 --- a/src/langbot/pkg/provider/tools/loaders/native.py +++ b/src/langbot/pkg/provider/tools/loaders/native.py @@ -336,11 +336,14 @@ class NativeToolLoader(loader.ToolLoader): if 'python_project' not in selected_skill: python_project = skill_loader.should_prepare_skill_python_env(package_root) if python_project: + revision_key = str(selected_skill.get('revision', '') or '').removeprefix('sha256:') + if not revision_key: + raise ValueError(f'Activated skill "{selected_skill_name}" has no pinned revision.') parameters = dict(parameters) parameters['command'] = skill_loader.wrap_skill_command_with_python_env( command, mount_path=skill_mount, - state_path=f'/workspace/.skill-envs/{selected_skill_name}', + state_path=f'/workspace/.skill-envs/{selected_skill_name}/{revision_key}', ) # All exec calls (with or without skills) go through the same container @@ -1107,25 +1110,27 @@ else: skill_request = self._resolve_skill_relative_path( query, path, - include_visible=True, + include_visible=False, include_activated=True, ) skill_repository = getattr(self.ap, 'skill_repository', None) if skill_request is not None and skill_repository is not None: selected_skill, relative = skill_request try: - result = await skill_repository.read_skill_file( + result = await skill_repository.read_skill_resource( self._execution_context(query), selected_skill['name'], relative, + expected_revision=selected_skill.get('revision'), ) return self._build_read_result_from_text(str(result.get('content', '')), parameters) except Exception: try: - result = await skill_repository.list_skill_files( + result = await skill_repository.list_skill_resources( self._execution_context(query), selected_skill['name'], relative, + expected_revision=selected_skill.get('revision'), ) entries = [entry['name'] for entry in result.get('entries', [])] return self._build_directory_result(entries) @@ -1135,7 +1140,7 @@ else: host_location = self._resolve_host_location( query, path, - include_visible=True, + include_visible=False, include_activated=True, ) if self._should_use_box_workspace_files(host_location.selected_skill): @@ -1149,22 +1154,22 @@ else: path = parameters['path'] content = parameters['content'] self.ap.logger.info(f'write tool invoked: query_id={query.query_id} path={path} length={len(content)}') - encoding, _mode = self._write_options(parameters) + self._write_options(parameters) skill_request = self._resolve_skill_relative_path( query, path, include_visible=False, include_activated=True, ) - skill_repository = getattr(self.ap, 'skill_repository', None) - if skill_request is not None and skill_repository is not None: - if encoding != 'text': - return {'ok': False, 'error': 'base64 writes to skill packages are not supported.'} - selected_skill, relative = skill_request - execution_context = self._execution_context(query) - await skill_repository.write_skill_file(execution_context, selected_skill['name'], relative, content) - await self.ap.skill_mgr.reload_skills(execution_context) - return {'ok': True, 'path': path} + if skill_request is not None: + return { + 'ok': False, + 'error': ( + 'Published Skill revisions are immutable. Copy the package to a writable ' + 'directory under /workspace/skill-drafts, edit it there, then call register_skill ' + 'with the activated revision as base_revision.' + ), + } host_location = self._resolve_host_location( query, @@ -1195,32 +1200,15 @@ else: include_visible=False, include_activated=True, ) - if skill_request is not None and getattr(self.ap, 'skill_repository', None) is not None: - selected_skill, relative = skill_request - try: - result = await self.ap.skill_repository.read_skill_file( - self._execution_context(query), - selected_skill['name'], - relative, - ) - except Exception: - return {'ok': False, 'error': f'File not found: {path}'} - content = result.get('content', '') - count = content.count(old_string) - if count == 0: - return {'ok': False, 'error': 'old_string not found in file.'} - if count > 1: - return {'ok': False, 'error': f'old_string matches {count} locations; provide a more unique string.'} - new_content = content.replace(old_string, new_string, 1) - execution_context = self._execution_context(query) - await self.ap.skill_repository.write_skill_file( - execution_context, - selected_skill['name'], - relative, - new_content, - ) - await self.ap.skill_mgr.reload_skills(execution_context) - return {'ok': True, 'path': path} + if skill_request is not None: + return { + 'ok': False, + 'error': ( + 'Published Skill revisions are immutable. Copy the package to a writable ' + 'directory under /workspace/skill-drafts, edit it there, then call register_skill ' + 'with the activated revision as base_revision.' + ), + } host_location = self._resolve_host_location( query, @@ -1503,7 +1491,7 @@ else: host_location = self._resolve_host_location( query, path, - include_visible=True, + include_visible=False, include_activated=True, ) if self._should_use_box_workspace_files(host_location.selected_skill): @@ -1525,7 +1513,7 @@ else: host_location = self._resolve_host_location( query, path, - include_visible=True, + include_visible=False, include_activated=True, ) if self._should_use_box_workspace_files(host_location.selected_skill): diff --git a/src/langbot/pkg/provider/tools/loaders/skill.py b/src/langbot/pkg/provider/tools/loaders/skill.py index fec3b6e00..59d669950 100644 --- a/src/langbot/pkg/provider/tools/loaders/skill.py +++ b/src/langbot/pkg/provider/tools/loaders/skill.py @@ -62,24 +62,31 @@ def get_visible_skill(ap: app.Application, query: pipeline_query.Query, skill_na def build_execution_mounts(ap: app.Application, query: pipeline_query.Query) -> list[dict]: - """Translate visible Core-owned packages into generic read-only mounts.""" + """Mount only immutable revisions pinned by this run's activations.""" mounts: list[dict] = [] - for skill_name, skill_data in get_visible_skills(ap, query).items(): + for skill_name, skill_data in get_activated_skills(query).items(): package_root = str(skill_data.get('package_root', '') or '').strip() + manifest_path = str(skill_data.get('manifest_path', '') or '').strip() + revision = str(skill_data.get('revision', '') or '').strip() if not package_root: - continue + raise ValueError(f'Activated skill "{skill_name}" has no immutable package root.') + if not revision: + raise ValueError(f'Activated skill "{skill_name}" has no pinned revision.') if not os.path.isdir(package_root): - ap.logger.warning( - f'Skill "{skill_name}" package_root missing on the Core filesystem ' - f'({package_root}); skipping its execution mount. Reload the skill catalog.' + raise ValueError( + f'Activated skill "{skill_name}" pinned revision {revision} is unavailable; ' + 'the run cannot be recovered safely.' ) - continue + if not manifest_path or not os.path.isfile(manifest_path): + raise ValueError(f'Activated skill "{skill_name}" pinned revision {revision} has no publication manifest.') mounts.append( { 'host_path': package_root, 'mount_path': get_virtual_skill_mount_path(skill_name), 'mode': 'ro', + 'content_digest': revision, + 'manifest_path': manifest_path, } ) return mounts @@ -99,14 +106,18 @@ def get_activated_skill(query: pipeline_query.Query, skill_name: str) -> dict | return get_activated_skills(query).get(skill_name) -def register_activated_skill(query: pipeline_query.Query, skill_data: dict) -> None: +def register_activated_skill(query: pipeline_query.Query, skill_data: dict) -> dict: if query.variables is None: query.variables = {} activated = query.variables.setdefault(ACTIVATED_SKILLS_KEY, {}) skill_name = str(skill_data.get('name', '') or '').strip() - if skill_name and skill_name not in activated: - activated[skill_name] = skill_data + revision = str(skill_data.get('revision', '') or '').strip() + if not skill_name or not revision: + raise ValueError('Activated Skills require a name and immutable revision.') + if skill_name not in activated: + activated[skill_name] = dict(skill_data) + return activated[skill_name] def normalize_skill_names(value: typing.Any) -> list[str]: @@ -127,22 +138,68 @@ def get_activated_skill_names(query: pipeline_query.Query) -> list[str]: return normalize_skill_names(list(get_activated_skills(query).keys())) -def restore_activated_skills( +def get_activated_skill_bindings(query: pipeline_query.Query) -> list[dict[str, str]]: + """Return exact bindings suitable for persistence and restart recovery.""" + + return [ + {'name': name, 'revision': str(skill.get('revision', '') or '')} + for name, skill in get_activated_skills(query).items() + ] + + +def normalize_skill_bindings(value: typing.Any) -> list[dict[str, str]]: + if not isinstance(value, list): + raise ValueError('Activated Skill recovery requires revision bindings.') + bindings: list[dict[str, str]] = [] + seen: dict[str, str] = {} + for item in value: + if not isinstance(item, dict): + raise ValueError('Activated Skill recovery cannot use names without revisions.') + name = str(item.get('name', '') or '').strip() + revision = str(item.get('revision', '') or '').strip() + if not name or not revision: + raise ValueError('Each activated Skill binding requires name and revision.') + previous = seen.get(name) + if previous is not None and previous != revision: + raise ValueError(f'Conflicting pinned revisions supplied for Skill "{name}".') + if previous is None: + seen[name] = revision + bindings.append({'name': name, 'revision': revision}) + return bindings + + +async def restore_activated_skills( ap: app.Application, query: pipeline_query.Query, - skill_names: typing.Any, + skill_bindings: typing.Any, ) -> list[str]: - """Restore caller-provided activated skill names into Query variables. + """Restore exact revisions or fail explicitly; never resolve latest by name.""" - Persistence and state scope ownership belong to higher-level flows. This - helper only rebuilds current Query state from pipeline-visible skills, so - removed or unbound skills stay unavailable to native exec/write/edit. - """ + repository = getattr(ap, 'skill_repository', None) + if repository is None: + raise ValueError('Skill repository is unavailable during run recovery.') + context = ExecutionContext( + instance_uuid=str(getattr(query, 'instance_uuid', '') or ''), + workspace_uuid=str(getattr(query, 'workspace_uuid', '') or ''), + placement_generation=getattr(query, 'placement_generation', 0) or 0, + bot_uuid=getattr(query, 'bot_uuid', None), + pipeline_uuid=getattr(query, 'pipeline_uuid', None), + query_uuid=getattr(query, 'query_uuid', None), + ) + bound_names = get_bound_skill_names(query) restored: list[str] = [] - for skill_name in normalize_skill_names(skill_names): - skill_data = get_visible_skill(ap, query, skill_name) + for binding in normalize_skill_bindings(skill_bindings): + skill_name = binding['name'] + if bound_names is not None and skill_name not in bound_names: + raise ValueError(f'Skill "{skill_name}" is no longer authorized for this recoverable run.') + skill_data = await repository.get_skill( + context, + skill_name, + snapshot=True, + revision=binding['revision'], + ) if skill_data is None: - continue + raise ValueError(f'Skill "{skill_name}" pinned revision {binding["revision"]} is unavailable.') register_activated_skill(query, skill_data) restored.append(skill_name) return restored diff --git a/src/langbot/pkg/provider/tools/loaders/skill_authoring.py b/src/langbot/pkg/provider/tools/loaders/skill_authoring.py index bf7ab94d3..a681d3233 100644 --- a/src/langbot/pkg/provider/tools/loaders/skill_authoring.py +++ b/src/langbot/pkg/provider/tools/loaders/skill_authoring.py @@ -172,7 +172,9 @@ class SkillToolLoader(loader.ToolLoader): if skill_data is None: raise ValueError(f'Skill "{skill_name}" is no longer available; reload the skill catalog.') - skill_loader.register_activated_skill(query, skill_data) + # Re-activation in the same run keeps the first pinned revision even if + # a newer publication appeared in the meantime. + skill_data = skill_loader.register_activated_skill(query, skill_data) instructions = skill_data.get('instructions', '') revision = str(skill_data.get('revision', '') or '') @@ -286,8 +288,8 @@ class SkillToolLoader(loader.ToolLoader): if not skill_name: raise ValueError('skill name is required') - # Create the skill - created = await skill_service.import_skill_directory( + base_revision = str(parameters.get('base_revision', '') or '').strip() or None + published = await skill_service.import_skill_directory( execution_context, host_path, { @@ -296,13 +298,15 @@ class SkillToolLoader(loader.ToolLoader): 'description': str(parameters.get('description') or scanned.get('description', '')).strip(), 'instructions': str(parameters.get('instructions') or scanned.get('instructions', '')), }, + base_revision=base_revision, ) return { 'registered': True, 'skill_name': skill_name, 'source_path': sandbox_path, - 'skill': created, + 'revision': published.get('revision'), + 'skill': published, } def _resolve_workspace_directory( @@ -401,9 +405,10 @@ class SkillToolLoader(loader.ToolLoader): name=REGISTER_SKILL_TOOL_NAME, human_desc='Register a skill from sandbox', description=( - "Register a skill package from a directory under /workspace into LangBot's skill store. " - 'Use this after creating or preparing a skill in the sandbox with exec/read/write/edit. ' + "Publish a skill draft from a directory under /workspace into LangBot's skill store. " + 'Use this after creating or preparing the draft with exec/read/write/edit. ' 'The directory must contain a SKILL.md file. ' + 'Updating an existing Skill requires its current base_revision; conflicting updates are rejected. ' 'After registration, the skill can be activated with the activate tool.' ), parameters={ @@ -429,6 +434,10 @@ class SkillToolLoader(loader.ToolLoader): 'type': 'string', 'description': 'Optional instructions override.', }, + 'base_revision': { + 'type': 'string', + 'description': 'Required current revision when publishing an update; omit for a new Skill.', + }, }, 'required': ['path'], 'additionalProperties': False, diff --git a/src/langbot/pkg/skill/activation.py b/src/langbot/pkg/skill/activation.py deleted file mode 100644 index 27ce1b3da..000000000 --- a/src/langbot/pkg/skill/activation.py +++ /dev/null @@ -1,44 +0,0 @@ -from __future__ import annotations - -import typing - -from ..provider.tools.loaders import skill as skill_loader -from ..api.http.context import ExecutionContext - -if typing.TYPE_CHECKING: - from ..core import app - import langbot_plugin.api.entities.builtin.pipeline.query as pipeline_query - - -# Skill activation is now handled through Tool Call mechanism (activate tool). -# This file is kept for potential future extensions but the text marker -# detection mechanism has been removed. - - -def register_activated_skill( - ap: app.Application, - query: pipeline_query.Query, - skill_name: str, -) -> bool: - """Register an activated skill for sandbox mount path resolution. - - This is called by the activate tool when a skill is activated via Tool Call. - """ - skill_mgr = getattr(ap, 'skill_mgr', None) - if skill_mgr is None: - return False - - execution_context = ExecutionContext( - instance_uuid=str(getattr(query, 'instance_uuid', '') or ''), - workspace_uuid=str(getattr(query, 'workspace_uuid', '') or ''), - placement_generation=getattr(query, 'placement_generation', 0) or 0, - bot_uuid=getattr(query, 'bot_uuid', None), - pipeline_uuid=getattr(query, 'pipeline_uuid', None), - query_uuid=getattr(query, 'query_uuid', None), - ) - skill_data = skill_mgr.get_skill_by_name(execution_context, skill_name) - if skill_data is None: - return False - - skill_loader.register_activated_skill(query, skill_data) - return True diff --git a/src/langbot/pkg/skill/repository.py b/src/langbot/pkg/skill/repository.py index 86e0ba6ee..6aaaa0c95 100644 --- a/src/langbot/pkg/skill/repository.py +++ b/src/langbot/pkg/skill/repository.py @@ -5,7 +5,9 @@ import os import weakref from langbot_plugin.skill_store import ( + SkillRevisionConflictError, SkillRevisionMismatchError, + SkillRevisionNotFoundError, SkillStore, skill_namespace, ) @@ -116,13 +118,29 @@ class SkillRepository: async def list_skills(self, context: TenantContext) -> list[dict]: return await self._call(context, 'list_skills') - async def get_skill(self, context: TenantContext, name: str, *, snapshot: bool = False) -> dict | None: + async def get_skill( + self, + context: TenantContext, + name: str, + *, + snapshot: bool = False, + revision: str | None = None, + ) -> dict | None: + if revision is not None: + return await self._call(context, 'get_skill_snapshot', name, revision) return await self._call(context, 'get_skill_snapshot' if snapshot else 'get_skill', name) async def create_skill(self, context: TenantContext, skill: dict) -> dict: return await self._call(context, 'create_skill', skill) - async def import_skill_directory(self, context: TenantContext, path: str, skill: dict) -> dict: + async def import_skill_directory( + self, + context: TenantContext, + path: str, + skill: dict, + *, + base_revision: str | None = None, + ) -> dict: namespace = self._namespace(context) return await self._call( context, @@ -130,10 +148,24 @@ class SkillRepository: path, skill, source_root=self._workspace_root(namespace), + base_revision=base_revision, ) - async def update_skill(self, context: TenantContext, name: str, skill: dict) -> dict: - return await self._call(context, 'update_skill', name, skill) + async def update_skill( + self, + context: TenantContext, + name: str, + skill: dict, + *, + base_revision: str | None, + ) -> dict: + return await self._call( + context, + 'update_skill', + name, + skill, + base_revision=base_revision, + ) async def delete_skill(self, context: TenantContext, name: str) -> None: await self._call(context, 'delete_skill', name) @@ -194,8 +226,23 @@ class SkillRepository: expected_revision=expected_revision, ) - async def write_skill_file(self, context: TenantContext, name: str, path: str, content: str) -> dict: - return await self._call(context, 'write_skill_file', name, path, content) + async def write_skill_file( + self, + context: TenantContext, + name: str, + path: str, + content: str, + *, + base_revision: str | None, + ) -> dict: + return await self._call( + context, + 'write_skill_file', + name, + path, + content, + base_revision=base_revision, + ) async def preview_skill_zip(self, context: TenantContext, file_bytes: bytes, filename: str, **kwargs) -> list[dict]: return await self._call(context, 'preview_zip_upload', file_bytes=file_bytes, filename=filename, **kwargs) @@ -204,4 +251,9 @@ class SkillRepository: return await self._call(context, 'install_zip_upload', file_bytes=file_bytes, filename=filename, **kwargs) -__all__ = ['SkillRepository', 'SkillRevisionMismatchError'] +__all__ = [ + 'SkillRepository', + 'SkillRevisionConflictError', + 'SkillRevisionMismatchError', + 'SkillRevisionNotFoundError', +] diff --git a/tests/integration_tests/box/test_cloud_box_admission_integration.py b/tests/integration_tests/box/test_cloud_box_admission_integration.py index f38289789..69eec3e10 100644 --- a/tests/integration_tests/box/test_cloud_box_admission_integration.py +++ b/tests/integration_tests/box/test_cloud_box_admission_integration.py @@ -351,13 +351,26 @@ async def test_cloud_core_skills_mount_generically_and_do_not_require_box_entitl 'instructions': 'Run scripts/main.py', }, ) - await repository.write_skill_file(first, 'runner', 'scripts/main.py', "print('ok')") - await repository.write_skill_file(first, 'runner', 'requirements.txt', 'requests==2.32.0\n') + script_revision = await repository.write_skill_file( + first, + 'runner', + 'scripts/main.py', + "print('ok')", + base_revision=own_skill['revision'], + ) + await repository.write_skill_file( + first, + 'runner', + 'requirements.txt', + 'requests==2.32.0\n', + base_revision=script_revision['revision'], + ) refreshed_skill = await repository.get_skill(first, 'runner') assert refreshed_skill is not None assert refreshed_skill['python_project'] is True await service.ap.skill_mgr.reload_skills(first) query = _query(first, 91) + skill_loader.register_activated_skill(query, refreshed_skill) await service.execute_tool( { 'command': 'python /workspace/.skills/runner/scripts/main.py', @@ -369,9 +382,10 @@ async def test_cloud_core_skills_mount_generically_and_do_not_require_box_entitl mounted_spec = backend.started_specs[-1] assert len(mounted_spec.extra_mounts) == 1 - assert mounted_spec.extra_mounts[0].host_path == own_skill['package_root'] + assert mounted_spec.extra_mounts[0].host_path == refreshed_skill['package_root'] assert mounted_spec.extra_mounts[0].mount_path == '/workspace/.skills/runner' assert mounted_spec.extra_mounts[0].mode.value == 'ro' + assert mounted_spec.extra_mounts[0].content_digest == refreshed_skill['revision'] assert await repository.get_skill(first, 'private') is None await repository.create_skill( diff --git a/tests/unit_tests/box/test_box_service.py b/tests/unit_tests/box/test_box_service.py index b9615a75d..d9297923b 100644 --- a/tests/unit_tests/box/test_box_service.py +++ b/tests/unit_tests/box/test_box_service.py @@ -1939,29 +1939,36 @@ def test_disconnect_callback_does_not_schedule_without_running_event_loop(): class TestBuildSkillExecutionMounts: - """Robustness of skill mount construction against a stale skill cache. - - The three sandbox backends behave inconsistently when a skill's - package_root no longer exists on disk (nsjail aborts the whole sandbox - start, Docker silently auto-creates a root-owned empty directory, E2B - silently skips). Mount construction must filter these out up front so - the backend never sees a bad mount. - """ + """Execution materializes only revisions pinned by this run.""" def _make_app(self, logger, skills): app = make_app(logger) app.skill_mgr = SimpleNamespace(skills=skills, get_skills=Mock(return_value=skills)) return app - def test_skips_skill_with_missing_package_root(self): + def test_mounts_only_activated_revision(self): logger = Mock() with tempfile.TemporaryDirectory() as live_dir: + manifest_path = os.path.join(live_dir, 'manifest.json') + with open(manifest_path, 'w', encoding='utf-8') as file: + file.write('{}') skills = { - 'alive': {'name': 'alive', 'package_root': live_dir}, - 'ghost': {'name': 'ghost', 'package_root': '/nonexistent/path/should/never/exist'}, + 'alive': { + 'name': 'alive', + 'package_root': live_dir, + 'manifest_path': manifest_path, + 'revision': 'sha256:' + '1' * 64, + }, + 'visible-not-activated': { + 'name': 'visible-not-activated', + 'package_root': live_dir, + 'manifest_path': manifest_path, + 'revision': 'sha256:' + '2' * 64, + }, } app = self._make_app(logger, skills) query = make_query() + skill_loader.register_activated_skill(query, skills['alive']) mounts = skill_loader.build_execution_mounts(app, query) @@ -1970,42 +1977,56 @@ class TestBuildSkillExecutionMounts: 'host_path': live_dir, 'mount_path': '/workspace/.skills/alive', 'mode': 'ro', + 'content_digest': 'sha256:' + '1' * 64, + 'manifest_path': manifest_path, } ] - # Warning logged so operators can see what was dropped - assert any( - 'ghost' in str(call.args[0]) and 'package_root missing' in str(call.args[0]) - for call in logger.warning.call_args_list - ) - def test_rejects_missing_core_paths_when_filesystem_not_shared(self): - """Core owns package paths even when Box is a separate process.""" + def test_missing_pinned_revision_fails_instead_of_being_skipped(self): logger = Mock() skills = { - 'a': {'name': 'a', 'package_root': '/box/skills/a'}, - 'b': {'name': 'b', 'package_root': '/box/skills/b'}, + 'a': { + 'name': 'a', + 'package_root': '/box/skills/a', + 'manifest_path': '/box/skills/manifest.json', + 'revision': 'sha256:' + '1' * 64, + } } app = self._make_app(logger, skills) + query = make_query() + skill_loader.register_activated_skill(query, skills['a']) - mounts = skill_loader.build_execution_mounts(app, make_query()) + with pytest.raises(ValueError, match='cannot be recovered safely'): + skill_loader.build_execution_mounts(app, query) - assert mounts == [] - assert len(logger.warning.call_args_list) == 2 - - def test_skips_skill_with_empty_package_root(self): + def test_rejects_activated_skill_with_empty_package_root(self): logger = Mock() skills = { - 'no_root': {'name': 'no_root', 'package_root': ''}, - 'whitespace': {'name': 'whitespace', 'package_root': ' '}, + 'no_root': { + 'name': 'no_root', + 'package_root': '', + 'manifest_path': '', + 'revision': 'sha256:' + '1' * 64, + } } app = self._make_app(logger, skills) + query = make_query() + skill_loader.register_activated_skill(query, skills['no_root']) - assert skill_loader.build_execution_mounts(app, make_query()) == [] + with pytest.raises(ValueError, match='no immutable package root'): + skill_loader.build_execution_mounts(app, query) def test_empty_package_root_skipped_even_when_not_shared(self): """An empty package_root is always invalid regardless of topology.""" logger = Mock() - skills = {'no_root': {'name': 'no_root', 'package_root': ''}} + skills = { + 'no_root': { + 'name': 'no_root', + 'package_root': '', + 'manifest_path': '', + 'revision': 'sha256:' + '1' * 64, + } + } app = self._make_app(logger, skills) assert skill_loader.build_execution_mounts(app, make_query()) == [] diff --git a/tests/unit_tests/provider/test_skill_tools.py b/tests/unit_tests/provider/test_skill_tools.py index 46db84f71..eec0f7d01 100644 --- a/tests/unit_tests/provider/test_skill_tools.py +++ b/tests/unit_tests/provider/test_skill_tools.py @@ -61,7 +61,9 @@ def _make_skill_data( 'description': kwargs.pop('description', f'Description of {name}'), 'instructions': instructions, 'package_root': package_root, + 'manifest_path': kwargs.pop('manifest_path', ''), 'entry_file': entry_file, + 'revision': kwargs.pop('revision', 'sha256:' + '1' * 64), **kwargs, } @@ -109,58 +111,6 @@ class TestSkillManagerCache: repository.list_skills.assert_awaited_once_with(_CONTEXT) -class TestSkillActivationHelper: - """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 - - ap = _make_ap() - mgr = SkillManager(ap) - mgr._skills_by_scope[mgr._scope_key(_CONTEXT)] = { - 'primary': _make_skill_data(name='primary', instructions='Primary instructions'), - } - ap.skill_mgr = mgr - - query = _make_query() - - 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_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_by_scope[mgr._scope_key(_CONTEXT)] = {'primary': _make_skill_data(name='primary')} - ap.skill_mgr = mgr - - query = _make_query() - - 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 = _make_query() - - assert register_activated_skill(ap, query, 'primary') is False - - class TestSkillPathHelpers: def test_get_visible_skills_filters_by_bound_names(self): from langbot.pkg.provider.tools.loaders.skill import PIPELINE_BOUND_SKILLS_KEY, get_visible_skills @@ -178,28 +128,49 @@ class TestSkillPathHelpers: assert list(result.keys()) == ['visible'] - def test_restore_activated_skills_uses_caller_provided_names_and_visibility(self): + @pytest.mark.asyncio + async def test_restore_activated_skills_uses_exact_revision_bindings(self): from langbot.pkg.provider.tools.loaders.skill import ( ACTIVATED_SKILLS_KEY, PIPELINE_BOUND_SKILLS_KEY, + get_activated_skill_bindings, get_activated_skill_names, restore_activated_skills, ) ap = _make_ap() - ap.skill_mgr = _make_skill_manager( - { - 'visible': _make_skill_data(name='visible'), - 'hidden': _make_skill_data(name='hidden'), - } + pinned = _make_skill_data(name='visible', revision='sha256:' + '2' * 64) + ap.skill_repository = SimpleNamespace( + get_skill=AsyncMock(return_value=pinned), ) query = _make_query(variables={PIPELINE_BOUND_SKILLS_KEY: ['visible']}) - restored = restore_activated_skills(ap, query, ['visible', 'hidden', 'visible', '']) + restored = await restore_activated_skills( + ap, + query, + [{'name': 'visible', 'revision': pinned['revision']}], + ) assert restored == ['visible'] assert list(query.variables[ACTIVATED_SKILLS_KEY].keys()) == ['visible'] assert get_activated_skill_names(query) == ['visible'] + assert get_activated_skill_bindings(query) == [{'name': 'visible', 'revision': pinned['revision']}] + ap.skill_repository.get_skill.assert_awaited_once_with( + _CONTEXT, + 'visible', + snapshot=True, + revision=pinned['revision'], + ) + + @pytest.mark.asyncio + async def test_restore_rejects_name_only_state(self): + from langbot.pkg.provider.tools.loaders.skill import restore_activated_skills + + ap = _make_ap() + ap.skill_repository = SimpleNamespace(get_skill=AsyncMock()) + + with pytest.raises(ValueError, match='cannot use names without revisions'): + await restore_activated_skills(ap, _make_query(), ['visible']) def test_resolve_virtual_skill_path_allows_visible_skill_reads(self): from langbot.pkg.provider.tools.loaders.skill import ( @@ -314,6 +285,37 @@ class TestSkillToolLoader: assert '' not in result['content'] assert set(query.variables[ACTIVATED_SKILLS_KEY].keys()) == {'demo'} + @pytest.mark.asyncio + async def test_reactivation_keeps_first_revision_and_instructions(self): + from langbot.pkg.provider.tools.loaders.skill_authoring import SkillToolLoader + + v1 = _make_skill_data( + name='demo', + instructions='version one', + revision='sha256:' + '1' * 64, + ) + v2 = _make_skill_data( + name='demo', + instructions='version two', + revision='sha256:' + '2' * 64, + ) + ap = _make_ap() + ap.skill_mgr = _make_skill_manager({'demo': v1}) + ap.skill_repository = SimpleNamespace( + get_skill=AsyncMock(side_effect=[v1, v2]), + ) + ap.box_service = SimpleNamespace(is_workspace_sandbox_available=AsyncMock(return_value=False)) + loader = SkillToolLoader(ap) + query = _make_query() + + first = await loader.invoke_tool('activate', {'skill_name': 'demo'}, query) + second = await loader.invoke_tool('activate', {'skill_name': 'demo'}, query) + + assert first['revision'] == v1['revision'] + assert second['revision'] == v1['revision'] + assert 'version one' in second['content'] + assert 'version two' not in second['content'] + @pytest.mark.asyncio async def test_activate_unknown_skill_raises(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( @@ -381,11 +383,55 @@ class TestSkillToolLoader: 'description': 'Imported from clone', 'instructions': 'Do work', }, + base_revision=None, ) assert result['registered'] is True assert result['skill_name'] == 'cloned-skill' assert result['source_path'] == '/workspace/repo' + @pytest.mark.asyncio + async def test_register_skill_forwards_base_revision_for_update(self): + from langbot.pkg.provider.tools.loaders.skill_authoring import ( + REGISTER_SKILL_TOOL_NAME, + SkillToolLoader, + ) + + with tempfile.TemporaryDirectory() as tmpdir: + draft = os.path.join(tmpdir, 'skill-drafts', 'demo') + os.makedirs(draft) + revision = 'sha256:' + '3' * 64 + ap = _make_ap() + ap.box_service = SimpleNamespace( + default_workspace=tmpdir, + available=True, + require_workspace_sandbox=AsyncMock(return_value=_CONTEXT), + ) + ap.skill_service = SimpleNamespace( + scan_directory_async=AsyncMock( + return_value={ + 'name': 'demo', + 'display_name': 'Demo', + 'description': 'Updated', + 'instructions': 'v2', + } + ), + import_skill_directory=AsyncMock( + return_value=_make_skill_data(name='demo', revision='sha256:' + '4' * 64) + ), + ) + + result = await SkillToolLoader(ap).invoke_tool( + REGISTER_SKILL_TOOL_NAME, + { + 'path': '/workspace/skill-drafts/demo', + 'base_revision': revision, + }, + _make_query(), + ) + + assert result['revision'] == 'sha256:' + '4' * 64 + assert ap.skill_service.import_skill_directory.await_args.kwargs['base_revision'] == revision + @pytest.mark.asyncio async def test_register_skill_rejects_workspace_escape(self): from langbot.pkg.provider.tools.loaders.skill_authoring import ( @@ -552,7 +598,7 @@ class TestNativeToolLoaderSkillPaths: @pytest.mark.asyncio async def test_read_visible_skill_file(self): from langbot.pkg.provider.tools.loaders.native import NativeToolLoader - from langbot.pkg.provider.tools.loaders.skill import PIPELINE_BOUND_SKILLS_KEY + from langbot.pkg.provider.tools.loaders.skill import register_activated_skill with tempfile.TemporaryDirectory() as tmpdir: skill_md = os.path.join(tmpdir, 'SKILL.md') @@ -568,10 +614,12 @@ class TestNativeToolLoaderSkillPaths: ap.skill_mgr = _make_skill_manager({'demo': _make_skill_data(name='demo', package_root=tmpdir)}) loader = NativeToolLoader(ap) + query = _make_query(query_id='q1') + register_activated_skill(query, ap.skill_mgr.skills['demo']) result = await loader.invoke_tool( 'read', {'path': '/workspace/.skills/demo/SKILL.md'}, - _make_query(query_id='q1', variables={PIPELINE_BOUND_SKILLS_KEY: ['demo']}), + query, ) assert result['ok'] is True @@ -581,7 +629,7 @@ class TestNativeToolLoaderSkillPaths: @pytest.mark.asyncio async def test_external_runtime_read_uses_core_skill_repository(self): from langbot.pkg.provider.tools.loaders.native import NativeToolLoader - from langbot.pkg.provider.tools.loaders.skill import PIPELINE_BOUND_SKILLS_KEY + from langbot.pkg.provider.tools.loaders.skill import register_activated_skill with tempfile.TemporaryDirectory() as tmpdir: with open(os.path.join(tmpdir, 'SKILL.md'), 'w', encoding='utf-8') as file_obj: @@ -593,14 +641,12 @@ class TestNativeToolLoaderSkillPaths: shares_filesystem_with_box=False, ) ap.skill_repository = SimpleNamespace( - read_skill_file=AsyncMock(return_value={'content': 'repository-content'}) + read_skill_resource=AsyncMock(return_value={'content': 'repository-content'}) ) ap.skill_mgr = _make_skill_manager({'demo': _make_skill_data(name='demo', package_root=tmpdir)}) loader = NativeToolLoader(ap) - query = _make_query( - query_id='q-external-read', - variables={PIPELINE_BOUND_SKILLS_KEY: ['demo']}, - ) + query = _make_query(query_id='q-external-read') + register_activated_skill(query, ap.skill_mgr.skills['demo']) result = await loader.invoke_tool( 'read', @@ -611,12 +657,17 @@ class TestNativeToolLoaderSkillPaths: assert result['ok'] is True assert result['content'] == 'repository-content' assert 'core-host-secret' not in repr(result) - ap.skill_repository.read_skill_file.assert_awaited_once_with(_CONTEXT, 'demo', 'SKILL.md') + ap.skill_repository.read_skill_resource.assert_awaited_once_with( + _CONTEXT, + 'demo', + 'SKILL.md', + expected_revision='sha256:' + '1' * 64, + ) @pytest.mark.asyncio async def test_core_owned_skill_path_does_not_depend_on_runtime_topology(self): from langbot.pkg.provider.tools.loaders.native import NativeToolLoader - from langbot.pkg.provider.tools.loaders.skill import PIPELINE_BOUND_SKILLS_KEY + from langbot.pkg.provider.tools.loaders.skill import register_activated_skill with tempfile.TemporaryDirectory() as tmpdir: with open(os.path.join(tmpdir, 'secret.txt'), 'w', encoding='utf-8') as file_obj: @@ -629,10 +680,8 @@ class TestNativeToolLoaderSkillPaths: ) ap.skill_mgr = _make_skill_manager({'demo': _make_skill_data(name='demo', package_root=tmpdir)}) loader = NativeToolLoader(ap) - query = _make_query( - query_id='q-external-no-protocol', - variables={PIPELINE_BOUND_SKILLS_KEY: ['demo']}, - ) + query = _make_query(query_id='q-external-no-protocol') + register_activated_skill(query, ap.skill_mgr.skills['demo']) result = await loader.invoke_tool( 'grep', @@ -652,13 +701,20 @@ class TestNativeToolLoaderSkillPaths: from langbot.pkg.provider.tools.loaders.skill import register_activated_skill with tempfile.TemporaryDirectory() as tmpdir: + manifest_path = os.path.join(tmpdir, 'manifest.json') + with open(manifest_path, 'w', encoding='utf-8') as file: + file.write('{}') ap = _make_ap() ap.box_service = SimpleNamespace( available=True, default_workspace=tmpdir, execute_tool=AsyncMock(return_value={'ok': True}), ) - skill_data = _make_skill_data(name='demo', package_root=tmpdir) + skill_data = _make_skill_data( + name='demo', + package_root=tmpdir, + manifest_path=manifest_path, + ) ap.skill_mgr = _make_skill_manager( {'demo': skill_data}, refresh_skill_from_disk=Mock(), @@ -689,40 +745,45 @@ class TestNativeToolLoaderSkillPaths: from langbot.pkg.provider.tools.loaders.native import NativeToolLoader from langbot.pkg.provider.tools.loaders.skill import register_activated_skill - ap = _make_ap() - ap.box_service = SimpleNamespace( - available=True, - shares_filesystem_with_box=False, - execute_tool=AsyncMock(return_value={'ok': True}), - ) - skill_data = _make_skill_data( - name='demo', - package_root='/box-runtime/skills/tenants/workspace/demo', - python_project=True, - ) - ap.skill_mgr = _make_skill_manager( - {'demo': skill_data}, - refresh_skill_from_disk=Mock(), - ) - loader = NativeToolLoader(ap) - query = _make_query(query_id='q-external', launcher_type='person', launcher_id='123') - register_activated_skill(query, skill_data) + with tempfile.TemporaryDirectory() as tmpdir: + manifest_path = os.path.join(tmpdir, 'manifest.json') + with open(manifest_path, 'w', encoding='utf-8') as file: + file.write('{}') + ap = _make_ap() + ap.box_service = SimpleNamespace( + available=True, + shares_filesystem_with_box=False, + execute_tool=AsyncMock(return_value={'ok': True}), + ) + skill_data = _make_skill_data( + name='demo', + package_root=tmpdir, + manifest_path=manifest_path, + python_project=True, + ) + ap.skill_mgr = _make_skill_manager( + {'demo': skill_data}, + refresh_skill_from_disk=Mock(), + ) + loader = NativeToolLoader(ap) + query = _make_query(query_id='q-external', launcher_type='person', launcher_id='123') + register_activated_skill(query, skill_data) - result = await loader.invoke_tool( - 'exec', - { - 'command': 'python /workspace/.skills/demo/scripts/run.py', - 'workdir': '/workspace/.skills/demo', - }, - query, - ) + result = await loader.invoke_tool( + 'exec', + { + 'command': 'python /workspace/.skills/demo/scripts/run.py', + 'workdir': '/workspace/.skills/demo', + }, + query, + ) assert result['ok'] is True tool_parameters = ap.box_service.execute_tool.await_args.args[0] wrapped = tool_parameters['command'] - assert '_LB_VENV_DIR="/workspace/.skill-envs/demo/.venv"' in wrapped + assert f'_LB_VENV_DIR="/workspace/.skill-envs/demo/{"1" * 64}/.venv"' in wrapped assert 'root = "/workspace/.skills/demo"' in wrapped - assert '/box-runtime/skills/tenants/workspace/demo' not in wrapped + assert tmpdir not in wrapped assert 'skill_name' not in ap.box_service.execute_tool.await_args.kwargs @pytest.mark.asyncio @@ -744,3 +805,33 @@ class TestNativeToolLoaderSkillPaths: {'path': '/workspace/.skills/demo/notes.txt', 'content': 'hi'}, query, ) + + @pytest.mark.asyncio + @pytest.mark.parametrize('tool_name', ['write', 'edit']) + async def test_published_revision_rejects_direct_mutation(self, tool_name): + from langbot.pkg.provider.tools.loaders.native import NativeToolLoader + from langbot.pkg.provider.tools.loaders.skill import register_activated_skill + + with tempfile.TemporaryDirectory() as tmpdir: + ap = _make_ap() + ap.box_service = SimpleNamespace(available=True, default_workspace=tmpdir) + skill = _make_skill_data(name='demo', package_root=tmpdir) + ap.skill_mgr = _make_skill_manager({'demo': skill}) + loader = NativeToolLoader(ap) + query = _make_query(query_id='immutable') + register_activated_skill(query, skill) + parameters = ( + {'path': '/workspace/.skills/demo/notes.txt', 'content': 'hi'} + if tool_name == 'write' + else { + 'path': '/workspace/.skills/demo/notes.txt', + 'old_string': 'old', + 'new_string': 'new', + } + ) + + result = await loader.invoke_tool(tool_name, parameters, query) + + assert result['ok'] is False + assert 'immutable' in result['error'] + assert '/workspace/skill-drafts' in result['error'] diff --git a/tests/unit_tests/test_skill_repository.py b/tests/unit_tests/test_skill_repository.py index 9878878b6..765ee6a92 100644 --- a/tests/unit_tests/test_skill_repository.py +++ b/tests/unit_tests/test_skill_repository.py @@ -1,9 +1,10 @@ from types import SimpleNamespace +from unittest.mock import Mock import pytest from langbot.pkg.api.http.context import ExecutionContext -from langbot.pkg.skill.repository import SkillRepository, SkillRevisionMismatchError +from langbot.pkg.skill.repository import SkillRepository, SkillRevisionConflictError _CONTEXT = ExecutionContext( @@ -79,7 +80,7 @@ def test_repository_keeps_old_box_root_only_for_online_upgrade(tmp_path): async def test_repository_crud_and_reads_do_not_require_box(tmp_path): repository = _repository(tmp_path) - await repository.create_skill( + created = await repository.create_skill( _CONTEXT, { 'name': 'docs-only', @@ -93,11 +94,12 @@ async def test_repository_crud_and_reads_do_not_require_box(tmp_path): 'docs-only', 'references/guide.md', '# Guide\n\nNo execution needed.', + base_revision=created['revision'], ) skill = await repository.get_skill(_CONTEXT, 'docs-only', snapshot=True) assert skill is not None - assert skill['revision'].startswith('stat-v1:') + assert skill['revision'].startswith('sha256:') assert [item['name'] for item in await repository.list_skills(_CONTEXT)] == ['docs-only'] listed = await repository.list_skill_resources( @@ -120,27 +122,47 @@ async def test_repository_crud_and_reads_do_not_require_box(tmp_path): @pytest.mark.asyncio -async def test_repository_rejects_traversal_and_stale_revision(tmp_path): +async def test_repository_rejects_traversal_and_conflicting_update(tmp_path): repository = _repository(tmp_path) - await repository.create_skill( + created = await repository.create_skill( _CONTEXT, {'name': 'safe', 'description': 'Safe', 'instructions': 'Use the reference.'}, ) - await repository.write_skill_file(_CONTEXT, 'safe', 'reference.md', 'first') + first = await repository.write_skill_file( + _CONTEXT, + 'safe', + 'reference.md', + 'first', + base_revision=created['revision'], + ) skill = await repository.get_skill(_CONTEXT, 'safe', snapshot=True) assert skill is not None with pytest.raises(ValueError, match='stay within'): await repository.read_skill_resource(_CONTEXT, 'safe', '../secret.txt') - await repository.write_skill_file(_CONTEXT, 'safe', 'reference.md', 'second') - with pytest.raises(SkillRevisionMismatchError, match='reactivate'): - await repository.read_skill_resource( + second = await repository.write_skill_file( + _CONTEXT, + 'safe', + 'reference.md', + 'second', + base_revision=first['revision'], + ) + pinned = await repository.read_skill_resource( + _CONTEXT, + 'safe', + 'reference.md', + expected_revision=skill['revision'], + ) + assert pinned['content'] == 'first' + with pytest.raises(SkillRevisionConflictError, match='changed since'): + await repository.update_skill( _CONTEXT, 'safe', - 'reference.md', - expected_revision=skill['revision'], + {'instructions': 'stale'}, + base_revision=first['revision'], ) + assert second['revision'] != first['revision'] @pytest.mark.asyncio @@ -207,3 +229,89 @@ async def test_repository_imports_only_from_the_fenced_workspace(tmp_path): (outside / 'SKILL.md').write_text('Outside', encoding='utf-8') with pytest.raises(ValueError, match='trusted source root'): await repository.scan_skill_directory(_CONTEXT, str(outside)) + + +@pytest.mark.asyncio +async def test_existing_run_keeps_v1_while_new_run_resolves_v2(tmp_path): + from langbot.pkg.provider.tools.loaders import skill as skill_loader + + repository = _repository(tmp_path) + created = await repository.create_skill( + _CONTEXT, + {'name': 'runner', 'instructions': 'Run scripts/main.py'}, + ) + v1_write = await repository.write_skill_file( + _CONTEXT, + 'runner', + 'scripts/main.py', + "print('v1')", + base_revision=created['revision'], + ) + v1 = await repository.get_skill(_CONTEXT, 'runner', snapshot=True) + old_run = SimpleNamespace(variables={}) + skill_loader.register_activated_skill(old_run, v1) + + await repository.write_skill_file( + _CONTEXT, + 'runner', + 'scripts/main.py', + "print('v2')", + base_revision=v1_write['revision'], + ) + v2 = await repository.get_skill(_CONTEXT, 'runner', snapshot=True) + new_run = SimpleNamespace(variables={}) + skill_loader.register_activated_skill(new_run, v2) + + old_resource = await repository.read_skill_resource( + _CONTEXT, + 'runner', + 'scripts/main.py', + expected_revision=v1['revision'], + ) + new_resource = await repository.read_skill_resource( + _CONTEXT, + 'runner', + 'scripts/main.py', + expected_revision=v2['revision'], + ) + app = SimpleNamespace(logger=Mock()) + old_mount = skill_loader.build_execution_mounts(app, old_run)[0] + new_mount = skill_loader.build_execution_mounts(app, new_run)[0] + + assert old_resource['content'] == "print('v1')" + assert new_resource['content'] == "print('v2')" + assert old_mount['host_path'] == v1['package_root'] + assert new_mount['host_path'] == v2['package_root'] + assert old_mount['content_digest'] == v1['revision'] + assert new_mount['content_digest'] == v2['revision'] + + +@pytest.mark.asyncio +async def test_deleted_skill_can_restore_exact_recoverable_run_revision(tmp_path): + from langbot.pkg.provider.tools.loaders import skill as skill_loader + + repository = _repository(tmp_path) + published = await repository.create_skill( + _CONTEXT, + {'name': 'recoverable', 'instructions': 'Pinned instructions'}, + ) + await repository.delete_skill(_CONTEXT, 'recoverable') + app = SimpleNamespace(skill_repository=repository) + query = SimpleNamespace( + variables={skill_loader.PIPELINE_BOUND_SKILLS_KEY: ['recoverable']}, + instance_uuid=_CONTEXT.instance_uuid, + workspace_uuid=_CONTEXT.workspace_uuid, + placement_generation=_CONTEXT.placement_generation, + bot_uuid=None, + pipeline_uuid=None, + query_uuid='recovered-query', + ) + + restored = await skill_loader.restore_activated_skills( + app, + query, + [{'name': 'recoverable', 'revision': published['revision']}], + ) + + assert restored == ['recoverable'] + assert skill_loader.get_activated_skill(query, 'recoverable')['revision'] == published['revision'] diff --git a/tests/unit_tests/test_skill_service.py b/tests/unit_tests/test_skill_service.py index b551d1566..be839ce4e 100644 --- a/tests/unit_tests/test_skill_service.py +++ b/tests/unit_tests/test_skill_service.py @@ -31,11 +31,11 @@ class TestSkillRepositoryBoundary: repository = SimpleNamespace( list_skills=AsyncMock(return_value=[{'name': 'x', 'instructions': 'Do work'}]), get_skill=AsyncMock(return_value={'name': 'x', 'instructions': 'Do work', 'revision': 'sha256:x'}), - create_skill=AsyncMock(return_value={'name': 'x', 'instructions': 'Do work'}), - update_skill=AsyncMock(return_value={'name': 'x', 'instructions': 'Updated'}), + create_skill=AsyncMock(return_value={'name': 'x', 'instructions': 'Do work', 'revision': 'sha256:v1'}), + update_skill=AsyncMock(return_value={'name': 'x', 'instructions': 'Updated', 'revision': 'sha256:v2'}), delete_skill=AsyncMock(), read_skill_file=AsyncMock(return_value={'path': 'a.txt', 'content': 'hello'}), - write_skill_file=AsyncMock(return_value={'path': 'a.txt'}), + write_skill_file=AsyncMock(return_value={'path': 'a.txt', 'revision': 'sha256:v3'}), ) return SimpleNamespace( skill_mgr=SimpleNamespace(reload_skills=AsyncMock()), @@ -62,12 +62,33 @@ class TestSkillRepositoryBoundary: service = SkillService(ap) await service.create_skill(_CONTEXT, {'name': 'x'}) - await service.update_skill(_CONTEXT, 'x', {'instructions': 'Updated'}) - await service.write_skill_file(_CONTEXT, 'x', 'a.txt', 'hello') + await service.update_skill( + _CONTEXT, + 'x', + {'instructions': 'Updated', 'base_revision': 'sha256:v1'}, + ) + await service.write_skill_file( + _CONTEXT, + 'x', + 'a.txt', + 'hello', + base_revision='sha256:v2', + ) ap.skill_repository.create_skill.assert_awaited_once_with(_CONTEXT, {'name': 'x'}) - ap.skill_repository.update_skill.assert_awaited_once_with(_CONTEXT, 'x', {'instructions': 'Updated'}) - ap.skill_repository.write_skill_file.assert_awaited_once_with(_CONTEXT, 'x', 'a.txt', 'hello') + ap.skill_repository.update_skill.assert_awaited_once_with( + _CONTEXT, + 'x', + {'instructions': 'Updated'}, + base_revision='sha256:v1', + ) + ap.skill_repository.write_skill_file.assert_awaited_once_with( + _CONTEXT, + 'x', + 'a.txt', + 'hello', + base_revision='sha256:v2', + ) @pytest.mark.asyncio async def test_get_skill_returns_repository_revision(self): diff --git a/web/src/app/home/skills/components/skill-form/SkillForm.tsx b/web/src/app/home/skills/components/skill-form/SkillForm.tsx index b11286663..73338b9fd 100644 --- a/web/src/app/home/skills/components/skill-form/SkillForm.tsx +++ b/web/src/app/home/skills/components/skill-form/SkillForm.tsx @@ -805,7 +805,10 @@ export default function SkillForm({ try { if (initSkillName) { - const resp = await httpClient.updateSkill(initSkillName, baseSkillData); + const resp = await httpClient.updateSkill(initSkillName, { + ...baseSkillData, + base_revision: skill.revision || '', + }); toast.success(t('skills.saveSuccess')); onSkillUpdated(resp.skill.name); } else { diff --git a/web/src/app/infra/entities/api/index.ts b/web/src/app/infra/entities/api/index.ts index 7c66f6be1..c19f2f28f 100644 --- a/web/src/app/infra/entities/api/index.ts +++ b/web/src/app/infra/entities/api/index.ts @@ -737,6 +737,7 @@ export interface Skill { description: string; instructions?: string; package_root?: string; + revision?: string; is_builtin?: boolean; created_at?: string; updated_at?: string; diff --git a/web/src/app/infra/http/BackendClient.ts b/web/src/app/infra/http/BackendClient.ts index c9d742c4c..2d96b4eeb 100644 --- a/web/src/app/infra/http/BackendClient.ts +++ b/web/src/app/infra/http/BackendClient.ts @@ -1726,7 +1726,7 @@ export class BackendClient extends BaseHttpClient { public updateSkill( name: string, - skill: Partial, + skill: Partial & { base_revision: string }, ): Promise { return this.put(`/api/v1/skills/${name}`, skill); } @@ -1790,6 +1790,7 @@ export class BackendClient extends BaseHttpClient { skillName: string, filePath: string, content: string, + baseRevision: string, ): Promise<{ skill: { name: string }; path: string; @@ -1797,6 +1798,7 @@ export class BackendClient extends BaseHttpClient { }> { return this.put(`/api/v1/skills/${skillName}/files/${filePath}`, { content, + base_revision: baseRevision, }); } }